Re: [PATCH 08/10] x86/cpuid: Handle leaf 0xb in guest_cpuid()

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Xen-devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH 08/10] x86/cpuid: Handle leaf 0xb in guest_cpuid()
Date: Mon, 27 Feb 2017 15:05:41 +0000	[thread overview]
Message-ID: <08f706cb-bd5f-b185-2b3a-4dc439b0f29c@citrix.com> (raw)
In-Reply-To: <58AD77F5020000780013CAF3@prv-mh.provo.novell.com>

On 22/02/17 10:37, Jan Beulich wrote:
>>>> On 22.02.17 at 11:22, <andrew.cooper3@citrix.com> wrote:
>> On 22/02/17 09:16, Jan Beulich wrote:
>>>>>> On 20.02.17 at 12:00, <andrew.cooper3@citrix.com> wrote:
>>>> Leaf 0xb is reserved by AMD, and uniformly hidden from guests by the 
>> toolstack
>>>> logic and hypervisor PV logic.
>>>>
>>>> The previous dynamic logic filled in the x2APIC ID for all HVM guests.  This
>>>> is modified to respect the entire leaf being reserved by AMD, but is altered
>>>> to include PV Intel guests, so they get more sensible values in their emulated
>>>> and faulted view of CPUID.
>>> Don't we expose x2APIC to HVM guests even on AMD systems? In
>>> which case we surely will want to also expose the x2APIC ID.
>> The x2apic feature bit is still listed as reserved in the latest AMD
>> SDM, and I haven't seen a piece of hardware which supports it.
> This doesn't seem to answer my question, as hardware behavior is
> of no interest here (our emulation works on old Intel hardware too).
> Just to be sure I've checked a HVM guest on an AMD box, and this
> is what Linux reports:
>
> <6>Enabling x2apic
> <6>Enabled x2apic
> <6>Switched APIC routing to physical x2apic.

Hmm - that is a good point - we do offer x2apic even to AMD HVM guests.

However, this is problematic and highlights another can of worms.  We
cannot turn on APICV/AVIC for guests if our cpuid features claim a
greater featureset than hardware can support.  (Looking at c/s
3e0c8272f, TSC_DEADLINE is a feature in a similar position.)

I think the only feasible way of making this work is to ensure that the
default policy exactly matches hardware, and the max policy contains all
features we are able to emulate.  Determination of whether to use
APICV/AVIC or not must be on whether the chosen featureset is compatible
with hardware or not.

>
>> Also, x2apic or not, this leaf is documented as fully reserved, so we
>> shouldn't be filling it in anyway.
> While guests appear to do fine without, I think this is inconsistent
> with providing x2APIC support. But as current behavior is the same,
> we can as well leave this for the future.
>
>>>> @@ -959,6 +950,14 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>>>>          }
>>>>          break;
>>>>  
>>>> +    case 0xb:
>>>> +        if ( p->x86_vendor == X86_VENDOR_INTEL )
>>>> +        {
>>>> +            /* Fix the x2APIC identifier. */
>>>> +            res->d = v->vcpu_id * 2;
>>>> +        }
>>>> +        break;
>>> Irrespective of the comment above, wouldn't the if() here better
>>> look at the x2APIC feature flag of the domain?
>> Perhaps.  There isn't a formal link called out, but looking at the
>> content, I am guessing that real hardware is consistent with their
>> support of x2apic and the availability of this leaf?
> I suppose so, at least they've got introduced together iirc.

Ok.  I will change the determination to be based exclusively on x2apic
being advertised, and leave a comment explaining why.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel