LGTM.

In the future please upload the diff with full context and please don't remove the group reviewers automatically added by the system.

In D60072#1379055, @markj wrote:

I suspect that using pm_active to store hart IDs instead of CPU IDs is too clever and will cause more problems down the road. We should instead store CPU IDs there and make the SBI routines handle translation to hart IDs.

I agree, by using hart IDs we've internalized an SBI implementation detail which is not ideal.

LGTM, one minor comment aside.

CCing @jhb and @jrtc27 since they've reviewed a similar patch (D58542) recently.

In D59710#1385442, @code_fedang.net wrote:

since the code that caused the panic has already been fixed on main, maybe we should rename this commit to "fix edgecases" or similar?

Could you please rebase the patch to a recent main? I can't cleanly apply the patch for testing.

LGTM, one minor inline comment aside.

LGTM (one minor suggestion inline), I'll wait for @jhb 's comments before accepting.

Generally LGTM, two nits aside. I'll test this a bit more in the coming days and land it if nothing obvious pops up.

In D57624#1377340, @markj wrote:

There is an unfortunate bug in this change: the pm_active bitmask is indexed by hart IDs, not CPU IDs, and these are not the same in general.
So, when we pass pm_active as the target cpuset to smp_rendezvous_cpus(), we end up signaling an unrelated set of CPUs.

LGTM (just the sdiff.c bits).

In D59840#1373358, @hayzam_gmail.com wrote:

In D59840#1373292, @bnovkov wrote:

LGTM, I'll give the patch a spin tomorrow before landing it. Have you tested this change somehow already?

Yep, I tested with a small 16-bit payload that loads a null IDT and executes ud2, so the guest takes a hardware triple fault (VMX exit reason 2).

I can attach the payload image and both serial logs if that's useful.

LGTM, I'll give the patch a spin tomorrow before landing it. Have you tested this change somehow already?

One minor comment, LGTM otherwise.

Address @markj's comments

Thank you for working on this, this is a useful feature.
Aside from the inline comments, I have one remark regarding config_schema_merge_options.

Hoist 'splitnum' check

Looks good now, thanks! I only have one minor inline comment.

Thanks, LGTM now. I won't be able to test the patch series on a non- SiFive FU74 board until EuroBSDCon finishes, I'll let you know if anything comes up during testing.

Propagate error from bounds checking instead of aborting

In D59068#1357257, @bnovkov wrote:

In D59068#1356482, @kp wrote:

I'm also struggling to make sense of the baseline flame graph. It seems to be spending a lot of time removing interfaces (5% of samples in pf_kkif_free()!) and I don't see any calls to pf_test(), which we should see if we were handling traffic.
Maybe the address decoding went wrong, because those stacks just look wrong.

Hm, that's odd. I'll try to find out what went wrong.

In D59068#1356482, @kp wrote:

I'm also struggling to make sense of the baseline flame graph. It seems to be spending a lot of time removing interfaces (5% of samples in pf_kkif_free()!) and I don't see any calls to pf_test(), which we should see if we were handling traffic.
Maybe the address decoding went wrong, because those stacks just look wrong.

Address @kp 's comments

This is a good feature, but I don't think that this is the correct approach design-wise. Adding an ioctl handler to just to fetch a VM's PID is overkill IMO, new ioctl commands should only be added when there's no other way to obtain the needed information (e.g., fetching memsegs and other metadata that is stored in the vmm module).

In D54641#1353031, @jhb wrote:

In D54641#1344584, @bnovkov wrote:

In D54641#1344263, @jhb wrote:

When a new PCI device shows up in hotplug both for native PCI-e hotplug and other systems like the ACPI approach you are using, the new device just shows up with all decoding disabled and the BARs set to their reset values (address of 0). It is the guest OS's job to allocate address space for the BARs and manage the address space.

Thank you, this clears up a lot and I think I'll be able to simplify the whole patch series greatly since we can completely ditch vmem.
Before I start purging the vmem-related parts from this patch stack, do you think that there's a future use for this application of vmem that would justify keeping it?

Probably not TBH as this is just a one-time allocation vs an address space that needs updating.

In D59068#1354800, @bofh wrote:

Can you check or host the results somewhere else? Maybe freefall?