Re: [PATCH v2 15/30] xen/x86: Improvements to in-hypervisor cpuid sanity checks

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 17/02/16 14:45, Jan Beulich wrote:
>>>> On 17.02.16 at 15:02, <andrew.cooper3@citrix.com> wrote:
>> On 17/02/16 10:55, Jan Beulich wrote:
>>>>>> On 17.02.16 at 11:43, <andrew.cooper3@citrix.com> wrote:
>>>> On 16/02/16 10:06, Jan Beulich wrote:
>>>>>>>> On 15.02.16 at 18:12, <andrew.cooper3@citrix.com> wrote:
>>>>>> On 15/02/16 15:43, Jan Beulich wrote:
>>>>>>>>>> On 05.02.16 at 14:42, <andrew.cooper3@citrix.com> wrote:
>>>>>>>> @@ -4617,50 +4618,39 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
>>>>>>>>          /* Fix up VLAPIC details. */
>>>>>>>>          *ebx &= 0x00FFFFFFu;
>>>>>>>>          *ebx |= (v->vcpu_id * 2) << 24;
>>>>>>>> +
>>>>>>>> +        *ecx &= hvm_featureset[FEATURESET_1c];
>>>>>>>> +        *edx &= hvm_featureset[FEATURESET_1d];
>>>>>>> Looks like I've overlooked an issue in patch 11, which becomes
>>>>>>> apparent here: How can you use a domain-independent featureset
>>>>>>> here, when features vary between HAP and shadow mode guests?
>>>>>>> I.e. in the earlier patch I suppose you need to calculate two
>>>>>>> hvm_*_featureset[]s, with the HAP one perhaps empty when
>>>>>>> !hvm_funcs.hap_supported.
>>>>>> Their use here is a halfway house between nothing and the planned full
>>>>>> per-domain policies.
>>>>>>
>>>>>> In this case, the "don't expose $X to a non-hap domain" checks have been
>>>>>> retained, to cover the difference.
>>>>> Well, doesn't it seem to you that doing only half of the HAP/shadow
>>>>> separation is odd/confusing? I.e. could I talk you into not doing any
>>>>> such separation (enforcing the non-HAP overrides as is done now)
>>>>> or finishing the separation to become visible/usable here?
>>>> The HAP/shadow distinction is needed in the toolstack to account for the
>>>> hap=<bool> option.
>>>>
>>>> The distinction will disappear when per-domain policies are introduced. 
>>>> If you notice, the distinction is private to the data generated by the
>>>> autogen script, and does not form a part of any API/ABI.  The sysctl
>>>> only has a single hvm featureset.
>>> I don't see this as being in line with
>>>
>>>     hvm_featuremask = hvm_funcs.hap_supported ?
>>>         hvm_hap_featuremask : hvm_shadow_featuremask;
>>>
>>> in patch 11. A shadow mode guest should see exactly the same
>>> set of features, regardless of whether HAP was available (and
>>> enabled) on a host.
>> A shadow mode guest will see the same features, independently of whether
>> HAP was available.
> I'm afraid I'm being dense: Either the guest sees the same features,
> which to me implies both of hvm_{hap,shadow}_featuremask are
> identical, or the two masks are different, resulting in different guest
> feature masks (and hence different guest features) depending on
> HAP availability. What am I missing?

A guest booted with hap and a guest booted with shadow will see
different features when booted on the same host.  Hap includes 1GB
superpages, PCID, etc.

The problem comes with a shadow guest booted on a hap-capable host. 
Such a guest can safely be migrated to a non-hap capable host, but only
if the toolstack knows that the guest saw a reduced featureset.

As there is still no interface to query what a guest can actually see
(depends on full per-domain policys and no dynamic hiding), the shadow
featuremask is used by the toolstack as a promise of what the Xen
dynamic checks will do.

~Andrew