On Fri, Nov 04, 2022 at 04:05:05PM +0000, Paul Durrant wrote: > On 04/11/2022 16:01, Roger Pau Monné wrote: > > 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é <roger....@citrix.com> > > > > --- > > > > 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. > > > > Yeah, I think they predate the existence of x2apic. > > > I withdraw patch 1 and adjust patch 2 accordingly then. > > > Cool. Thanks,
How does Windows know whether to use xAPIC or x2APIC? I would assume CPUID4A_MSR_BASED_APIC only makes sense when in xAPIC mode, as otherwise the registers are already accesses using MSRs. Thanks, Roger.
