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

From: Andrew Cooper <Andrew.Cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Henry.Wang@arm.com" <Henry.Wang@arm.com>,
	Paul Durrant <paul@xen.org>, Wei Liu <wl@xen.org>,
	Jun Nakajima <jun.nakajima@intel.com>,
	Kevin Tian <kevin.tian@intel.com>,
	Roger Pau Monne <roger.pau@citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH for-4.17 v2] hvm/apic: repurpose the reporting of the APIC assist options
Date: Fri, 11 Nov 2022 17:47:02 +0000	[thread overview]
Message-ID: <1051eec8-8074-39e8-299d-a540d12bc997@citrix.com> (raw)
In-Reply-To: <4d0edf44-bb7c-85d9-0890-79c999428ef6@citrix.com>

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.  It has
successfully fooled all the other maintainers into doing the wrong
thing, even after recently discussing the complexity and subtly in full
as part of the XSA-412 security fix.

~Andrew