Looks good.

A reminder to make sure you add the appropriate "Approved by" note in the actual commit log.

First commit looks good!

Looks good. Thanks for fixing.

Sorry about that - the delay wasn't intentional :-(

Thanks for the quick turnaround, but this idiom is intentional (I updated the CID a couple of hours ago).

Regarding libxo:

This patch will need to be updated after r284539
https://svnweb.freebsd.org/base?view=revision&revision=284539

In D2762#52988, @tychon wrote:

In D2762#52919, @neel wrote:

In D2762#52915, @tychon wrote:

Thanks Tycho. My responses inline:

This restructuring will be really handy. However, I think it's possible
to unify the handling of 'sysmem' and 'devmem' such that 'sysmem'
differs from 'devmem' only in persistance and accessibility (PROT_*).

Since alloc_memseg() is already called by both vm_setup_memory() and
vm_create_devmem() you'll simply need to provide a vanity name for
"lower", "low" and "high" memory to vm_alloc_memseg() when it's
created. Then you can delete the inference that a memsesg is 'RAM'
you get when you encounter a VM_MEMSEG_NAME. You'll also need to
augment vm_alloc_memseg() will to take a persistance argument
which will be set for 'sysmem' and clear for 'devmem'.

I think knowing which memory segments represent system memory is useful.

For e.g., I use this in iommu_modify() to only map 'sysmem' in the passthru
device's address space. This enforces consistency with the treatment of other
(emulated) device memory (e.g. AHCI BAR) from the point of view of the
passthru device.

It appears beyond just useful but rather an implementation requirement :-)

As a side benefit to treating all memory roughly equal is that you'll
be able to expose the entire guest's memory PA-wise in
/dev/vmm/testvmm.lowermem, /dev/vmm/testvmm.lowmem and
/dev/vmm/testvmm.highmem!

That's one way to do it although I don't see it as being functionally
different than mmap(/dev/vmm/vmname).

It's not different so there isn't much point.

My point really was that the API as proposed would be impossible for someone with a copy of vmm_dev.h and a binary only vmm.ko to use properly. Not that that someone like that exists but libvmmapi.so and bhyve shouldn't contain too much embedded knowledge of the kernel internals.

Specifically, VM_ALLOC_MEMSEG takes a struct vm_memseg which in no way makes it obvious that when you supply a name you get a devseg and that it's necessary to omit the name when you want to get a "true" memseg. It you add 'int ismemseg' then it would be obvious.

In D2762#52915, @tychon wrote:

Looks good.

There are many reasons why 'vm_activate_cpu()' might fail.

Looks good.

Assert that AHCI reads are naturally aligned and do not straddle a 32-bit
boundary.

In D2469#45441, @mav wrote:

As I see, reading single byte at offset multiple to 4 this function still return 32-bit value, at next -- 24-bit, etc. Is this expected/acceptable behavior?

In D2428#44714, @tychon wrote:

Hi,

In most cases it was treated as such, but you've actually got the information to do better. Specifically around line 624 in the newvmm_instruction_emul.c you could set *fault to IDT_SS or IDT_GP. If you think no one will ever care about the specific fault, perhaps renaming fault to is_fault would further cement it's boolean nature.

In D2428#44714, @tychon wrote:

Hi,

This looks very nice. My only feedback is that I got confused as to whether or not the "return fault (*fault)" provided by vmm_fetch_instruction was boolean or not. In most cases it was treated as such, but you've actually got the information to do better. Specifically around line 624 in the new vmm_instruction_emul.c you could set *fault to IDT_SS or IDT_GP. If you think no one will ever care about the specific fault, perhaps renaming fault to is_fault would further cement it's boolean nature.

Also as a mini-nit I'd suggest changing 'guest_fault' in the comment to 'fault' to match the function prototype vmm_instruction_emul.h

Tycho

Update all extern function declarations to use 'int *is_fault' consistently.

Looks good. Thanks for catching this.

Closed by commit rS281559 (authored by @neel).

In D2294#6, @rstone wrote:

I think that it would be sufficient to test that this still allows ppt to attach to a device dynamically:

1. devctl detach ixl4
2. devctl set driver pci11:0:3 ppt

(replace ixl4/pci11:0:3 with some unused PCI device from your test machine)

devctl also uses the same BUS_PROBE_NOWILDCARD feature that SR-IOV uses.

The test plan should also verify that VF passthrough still works after this change.

Apologies for the delayed review. I have some comments inline.

Closed by commit rS279970 (authored by @neel).

Closed by commit rS279468 (authored by @neel).

In D1983#7, @kostikbel wrote:

In D1983#6, @neel wrote:

In D1983#3, @kostikbel wrote:

Does IPI_STOP_HARD value must fit into 8 bits ? It is not the same as the spurious vector, which should be innocent as well, but slightly confusing.

There were two restrictions when choosing the value for the IPI_STOP_HARD value:

• 'native_lapic_ipi_vectored()' wants the vector to be less than 256

So if we change the assertion in native_lapic_ipi_vectored(), the IPI_STOP_HARD value could be moved outside the [0,255] range ? I mean, explicitly allow a value in the valid range OR IPI_STOP_HARD, in the assertion.

In D1983#3, @kostikbel wrote:

Does IPI_STOP_HARD value must fit into 8 bits ? It is not the same as the spurious vector, which should be innocent as well, but slightly confusing.

Looks good. A couple of minor comments inline.

Closed by commit rS277310 (authored by @neel).

Tycho, can you accept the revision in phabric?

In D1385#7, @tychon wrote:

In D1385#6, @neel wrote:

In D1385#5, @tychon wrote:

Then with respect to the other code, I'm a bit curious about the VM_RTC_READ/VM_RTC_WRITE interface. I'd sort of expected a size or length parameter -- perhaps length so the NVRAM component can be bulk import/exported -- but even size would be fine. Another alternative is to make the 'value' or 'retval' uint8_t so it's obvious it's a byte-based.

Does the following seem alright?

int vm_rtc_write(struct vmctx *ctx, int offset, uint8_t value);
int vm_rtc_read(struct vmctx *ctx, int offset, uint8_t *retval);
int vm_rtc_size(struct vmctx *ctx); /* return size of the nvram including the RTC control/status registers */

In rereading what I wrote, maybe I was too vague. I think that vm_rtc_write() and vm_rtc_read() are sufficient as the size of the device is well known or at least the consumer of these interfaces would be best served by knowing what they were reading or writing.

My suggestion of 'size' was along the lines of a read/write interface which allowed the access of multiple bytes at once. For one at a time interface, I think vm_rtc_size() can be omitted for simplicity.

In D1385#5, @tychon wrote:

In D1385#4, @neel wrote:

In D1385#3, @tychon wrote:

I'm still in the process of reviewing this, however at first glance I'm not sure I see any reason to remove the HPET Legacy Routing support.

When you first mentioned it, I assumed that some interface change which made it difficult to support and that it added the to the test matrix. Now seeing the implementation it appears entirely independent from the RTC support.

The reason it appears independent is because I removed LegacyRouting :-)

Since it's not broken and provides additional functionality -- which I can't help bug imagine that some guest out there is relying on -- I don't see the justification from deleting it as there are several other examples of "redundant mechanisms".

LegacyRouting is an optional capability so guests shouldn't be upset if they don't see this capability. I haven't run into any issues with the different guests that I have tested.

Also, LegacyRouting doesn't obviate the need for a complete PIT and RTC device emulation since we'll always need to support guests that depend on them.

The interrupt routing gets complicated with LegacyRouting. For e.g., if LegacyRouting is enabled then RTC periodic interrupts are disconnected from the PIC but the alarm and update-ended interrupts are now routed to the SCI. This is not insurmountable but I don't see a tangible benefit.

In any case the most important reason to deprecate LegacyRouting is because doubles the test matrix.

That is a valid reason... out it goes.

Then with respect to the other code, I'm a bit curious about the VM_RTC_READ/VM_RTC_WRITE interface. I'd sort of expected a size or length parameter -- perhaps length so the NVRAM component can be bulk import/exported -- but even size would be fine. Another alternative is to make the 'value' or 'retval' uint8_t so it's obvious it's a byte-based.

In D1385#3, @tychon wrote:

I'm still in the process of reviewing this, however at first glance I'm not sure I see any reason to remove the HPET Legacy Routing support.

When you first mentioned it, I assumed that some interface change which made it difficult to support and that it added the to the test matrix. Now seeing the implementation it appears entirely independent from the RTC support.

Addressed all issues raised in Peter's comments in the new diff.