On Fri, Nov 04, 2022 at 03:55:54PM +0000, Paul Durrant wrote: > On 04/11/2022 14:22, Roger Pau Monne wrote: > > The current reporting of the hardware assisted APIC options is done by > > checking "virtualize APIC accesses" which is not very helpful, as that > > feature doesn't avoid a vmexit, instead it does provide some help in > > order to detect APIC MMIO accesses in vmexit processing. > > > > Repurpose the current reporting of xAPIC assistance to instead report > > such feature as present when there's support for "TPR shadow" and > > "APIC register virtualization" because in that case some xAPIC MMIO > > register accesses are handled directly by the hardware, without > > requiring a vmexit. > > > > For symetry also change assisted x2APIC reporting to require > > "virtualize x2APIC mode" and "APIC register virtualization", dropping > > the option to also be reported when "virtual interrupt delivery" is > > available. Presence of the "virtual interrupt delivery" feature will > > be reported using a different option. > > > > Signed-off-by: Roger Pau Monné <[email protected]> > > --- > > I find the logic in vmx_vlapic_msr_changed() hard to follow, but I > > don't want to rewrite the function logic at this point. > > --- > > xen/arch/x86/hvm/viridian/viridian.c | 2 +- > > xen/arch/x86/hvm/vmx/vmcs.c | 8 ++++---- > > xen/arch/x86/hvm/vmx/vmx.c | 25 ++++++++++++++++++------- > > xen/arch/x86/traps.c | 4 +--- > > 4 files changed, 24 insertions(+), 15 deletions(-) > > > > diff --git a/xen/arch/x86/hvm/viridian/viridian.c > > b/xen/arch/x86/hvm/viridian/viridian.c > > index c4fa0a8b32..bafd8e90de 100644 > > --- a/xen/arch/x86/hvm/viridian/viridian.c > > +++ b/xen/arch/x86/hvm/viridian/viridian.c > > @@ -201,7 +201,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, > > uint32_t leaf, > > * Suggest x2APIC mode by default, unless xAPIC registers are > > hardware > > * virtualized and x2APIC ones aren't. > > */ > > - if ( !cpu_has_vmx_apic_reg_virt || > > cpu_has_vmx_virtualize_x2apic_mode ) > > + if ( !has_assisted_xapic(d) || has_assisted_x2apic(d) ) > > So, not sure why this is separated from patch 1 but stated this way it seems > counterintuitive. We only want to use the viridian MSRs if they are going to > be more efficient.. which I think is only in the case where we have neither > an x2apic not an assisted xapic (hence we would trap for MMIO).
I've read the MS HTLFS and I guess I got confused, the section about this CPUID bit states: "Bit 3: Recommend using MSRs for accessing APIC registers EOI, ICR and TPR rather than their memory-mapped" So I've (wrongly) understood that MSRs for accessing APIC registers was meant to be a recommendation to use x2APIC mode in order to access those registers. Didn't realize Viridian had a way to expose certain APIC registers using MSRs when the APIC is in xAPIC mode. I withdraw patch 1 and adjust patch 2 accordingly then. Thanks, Roger.
