I guess you meant that vmx_apicv_sync() should be called instead saving pir_desc directly ? I understand what you mean, but it will also require saving of vlapic_vtx. Current implementation in FreeBSD also requies saving vlapic_vtx, but just one field pending_prio. I am going to create review for that, once current review is finished.

Sorry, If vlapic_set_intr_ready() is called, it will change pir_desc->pir in some condition:
I assume that it can lead that pir_desc->pir is only container that keeps a interrupt request. Therefore if suspend VM and then restore, that interrupt will be lost.

But this commit should also suffer from not saved pir_desc after vmx_apicv_set_ready() is performed. Am I right?

In illumos bhyve, we chose to "flatten" queued interrupts in the PIR descriptor into the normal vlapic state, so there was only one source of truth for the IRR data:

In D26003#578623, @jhb wrote:

I agree btw with not having these be per-vCPU capabilities that can be toggled. It would be really surprising to a guest for rdtscp to transition from "working" to "not-working". OS's expect rdtscp to either work or not work for the duration of a given boot.

Abandoned, as it is to be replaced by the approach documented in illumos #13007.

I discovered the lack of a #define for this in FreeBSD while investigating 12998 on illumos bhyve (related to OpenBSD guests attempting to access that MSR on AMD hardware)

This was integrated as 12746 into illumos bhyve.

If that seems reasonable, it's now I intend to commit the change to illumos bhyve.

Updated to include feedback from @jhb

In D24727#544327, @jhb wrote:

Hmm, it's a bit odd to have the variables still called 'eax', etc while being 64-bit. I think I might prefer that we instead fix the callers to use local uint32_t variables that are passed to x86_emulate_cpuid and then assigned back to the destination registers after the function returns.

I do not have a machine available to test on FreeBSD at the moment, but the equivalent patch for SmartOS bhyve does appear to have the desired effect.

We had similar issues regarding vm_smp_rendezvous() and thread interruption in SmartOS. The easiest path forward for us was to totally eliminate the need for vm_smp_rendezvous(), which has been floated for upstreaming in D20389.

After discussion with @markj and @jhb, I drafted up a change which involves using an sxlock to guard against racing vioapic writes while allowing the main vioapic mutex to be dropped during the sensitive portion of the TMR update process. I don't have test hardware I can dedicate to running FreeBSD bhyve, so its performance is speculative.

Thanks for pointing that out @markj. This wasn't a problem in SmartOS bhyve since all of the mutexes there are adaptive, rather than spinlocks. (Due to interface differences, we couldn't guarantee a lack of voluntary context switching inside the critical_enter/critical_exit section, and spinlocks came with their own host of deadlock problems.) Considering the constraints in play on the FreeBSD side, I'm not sure this possible without serious restructuring. The vioapic lock would need to be sleeping, so the EOI processing would require a full exit from the VMRUN critical section. Other consumers (hpet, etc) would probably require similar treatment.

You're correct that illumos bhyve is not being built with any kind of Capsicum support. If the WITHOUT_CAPSICUM guards are removed, we'll just shim the cap_/caph_ interfaces as successful no-ops.

Looks good, with all of @kib's comments addressed.

Looks good, once the 7F->F7 fix is made.

Merged downstream in SmartOS as ec6f18e9

I agree with @jhb that this absolutely should be placed behind #ifdef guards for now.

Rebased to reflect upstream changes

Merged downstream in SmartOS as aa2898c4.

Eliminate the ILLEGAL_VECTOR qualification in vlapic_set_error per @jhb's feedback and rebase.

Fix excess whitespace and add attribution

Fixed include ordering per @kib's comment.

Added missing atomics header

Implemented @kib's suggestions.

I'm fine with @kib's suggested changes, with the caveat that we (Joyent) only positively tested the mfence version.

This issue was addressed when it was effectively side-pulled in commit #345158, despite having been posted here and accepted a week prior.

I think this fix is good for the short term. I can confirm seeing failed writes to that MSR when running a Linux guest on Zen hardware (which were non-fatal to Linux).

Yes, he was concerned about my use of the reserved bits. For a VMX-only case, it was probably fine, but if we ever want PCI-passthru to post interrupts directly to the guest, we'll need those bits. The write-up for OS-7354, which switched to the separate pending_prio field, covers the reasoning.

We do, in a sense, clear those bits on interrupt delivering since it involves clearing the pending bit, meaning the next 0 -> 1 transition would incur a clean of pending_prio. Clearing the lower bits in pending_prio as part of vmx_pending_intr carries no risk of incurring extra work, since interrupt notification is triggered from having an incoming prio greater than what is cached in pending_prio. Besides, spurious wake-ups from the HLT loop to check for interrupts are much preferred to missing one.

Fixed copyright attribution per Rod's comment.

Updated diff to include full context

I used git format-patch assuming phabricator would pick up on the context stuff, but apparently not. I'll be sure to increase the context for the next revision.

This is the fix we're applying to SmartOS bhyve, after syncing from upstream.

This is the fix we're applying to SmartOS bhyve, after syncing from upstream.

That stores the type definitions for the statistics. Storage of the actual statistic data should still be per-vcpu, hung off of struct vcpu`stats. Requiring a type definition per-vcpu doesn't make sense in that case.

Since the statistic tables are allocated on a per-vCPU basis already, it doesn't make sense to me why the tables themselves need to be expanded like this.

This is effectively what we did in SmartOS/illumos to address the issue:
https://github.com/joyent/illumos-joyent/commit/192e1e6405f98e4b0a12f9488793c5dd000f3f7e

In D18755#400855, @rgrimes wrote:

re-write the code using an accessor function (called vmm_vm_maxcpus()) to vmm.c that does the vmx->vm->maxcpus dereference and call that. The most common use of this is in vmm.c itself, and would probably end up inlined by the compiler anyway, though there are at least 4 other files that have the need for this, and iirc there is userland code that uses the VM_MAXCPU constant that needs to NOT do that!

Up until now, those structs have been kept opaque to the rest of the system, with accessor functions to perform necessary tasks/queries against them. Could you go into more detail about how your plans for dynamic VM_MAXCPU are impeded by similar constraints?