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

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

On Mon, Nov 14, 2022 at 03:31:39PM +0000, Andrew Cooper wrote:
> On 14/11/2022 13:15, Roger Pau Monne wrote:
> > On Fri, Nov 11, 2022 at 05:47:02PM +0000, Andrew Cooper wrote:
> >> On 11/11/2022 17:35, Andrew Cooper wrote:
> >>> On 11/11/2022 07:45, Jan Beulich wrote:
> >>>> On 10.11.2022 23:47, Andrew Cooper wrote:
> >>>>> On 04/11/2022 16:18, Roger Pau Monne wrote:
> >>>>>> --- a/xen/arch/x86/hvm/viridian/viridian.c
> >>>>>> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> >>>>>> @@ -197,7 +197,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, 
> >>>>>> uint32_t leaf,
> >>>>>>          res->a = CPUID4A_RELAX_TIMER_INT;
> >>>>>>          if ( viridian_feature_mask(d) & HVMPV_hcall_remote_tlb_flush )
> >>>>>>              res->a |= CPUID4A_HCALL_REMOTE_TLB_FLUSH;
> >>>>>> -        if ( !cpu_has_vmx_apic_reg_virt )
> >>>>>> +        if ( !has_assisted_xapic(d) )
> >>>>>>              res->a |= CPUID4A_MSR_BASED_APIC;
> >>>>> This check is broken before and after.  It needs to be keyed on
> >>>>> virtualised interrupt delivery, not register acceleration.
> >>>> To me this connection you suggest looks entirely unobvious, so would
> >>>> you mind expanding as to why you're thinking so? The hint to the guest
> >>>> here is related to how it would best access certain registers (aiui),
> >>>> which to me looks orthogonal to how interrupt delivery works.
> >>> I refer you again to the diagram.  (For everyone else on xen-devel, I
> >>> put this together when fixing XSA-412 because Intel's APIC acceleration
> >>> controls are entirely unintuitive.)
> >>>
> >>> It is "virtual interrupt delivery" which controls EOI/ICR acceleration,
> >>> and not "apic register virtualisation".  There's a decade worth of
> >>> hardware where this logic is an anti-optimsiation, by telling windows to
> >>> use a VMExit-ing mechanism even when microcode would have avoided the
> >>> VMExit.
> >>>
> >>> It is not by accident that "virtual interrupt delivery", introduced in
> >>> IvyBridge, is exactly the missing registers (on top of "use TPR Shadow"
> >>> which is even older) to make windows performance less bad.
> >> Sorry, sent too early.
> >>
> >> This also firmly highlights why the API/ABI is broken. 
> > I'm not seeing how you are making this connection: the context here is
> > strictly about a Viridian hint which Xen has been wrongly reporting,
> > but has nothing to do with the APIC assist API/ABI stuff.  It was
> > wrong before the introduction of APIC assist, and it's also wrong
> > after.
> 
> And now it has a layer of obfuscation which makes the bug less obvious.

Still, that's not an argument as to why the API/ABI is broken, just a
remark that the current usage here needs fixing (which it does).

> > Also see my other reply - I'm dubious whether this hint is useful for
> > any Windows version that supports x2APIC (which seems to be >= Windows
> > Server 2008), because we expose x2APIC to guests regardless of whether
> > the underlying platform APIC supports such mode.
> 
> It's not about whether a version of Windows supports x2APIC.  Its
> whether windows turns x2APIC on.

>From my previous conversation with Paul I got the impression that
Windows would choose x2APIC based solely on the CPUID bit:

https://lore.kernel.org/xen-devel/2c2d8b2b-e607-6d9d-b991-d1c065aac95d@xxxxxxx/

> Short of having an emulated IOMMU, or an absence of an IO-APIC (neither
> of which we do), guests wont turn x2APIC on.
> 
> I know we have the magic hack for IO-APIC with >8 bit destinations, but
> that only got enumerated in the Xen leaves (IIRC?), so only Linux can
> pick it up.

We don't have the hack yet, just the CPUID bit reserved.

Thanks, Roger.