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/[email protected]/ > 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.
