Re: [PATCH for-4.17 2/2] hvm/apic: repurpose the reporting of the APIC assist options

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Nov 04, 2022 at 05:35:23PM +0000, Paul Durrant wrote:
> On 04/11/2022 16:10, Roger Pau Monné wrote:
> > 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.pau@xxxxxxxxxx>
> > > > > > ---
> > > > > > 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?
> > 
> 
> cpuid? TBH I'm not sure why this recommendation would ever trump x2apic
> anyway.

OK, so the recommendation is ignored when running in x2APIC mode,
which should be the default since Xen does always expose x2APIC by
default to guests.

Thanks, Roger.