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

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

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.

This example could in principle be replaced with a clause similar to the
vmx side, but it doesn't remove the need for the shadow mask.

There is currently no way for the toolstack to query the cpuid policy,
which means it must have a correct idea of the eventual policy handed to
the guest.  The pv32/64 split isn't interesting in this regard as it
won't change.  HAP vs shadow however is very important to permit
migrating between hap and non-hap capable hosts.

>
>>>>>> +    case 0x80000007:
>>>>>> +        d &= pv_featureset[FEATURESET_e7d];
>>>>>> +        break;
>>>>> By not clearing eax and ebx (not sure about ecx) here you would
>>>>> again expose flags to guests without proper white listing.
>>>>>
>>>>>> +    case 0x80000008:
>>>>>> +        b &= pv_featureset[FEATURESET_e8b];
>>>>>>          break;
>>>>> Same here for ecx and edx and perhaps the upper 8 bits of eax.
>>>> Both of these would be changes to how these things are currently
>>>> handled, whereby a guest gets to see whatever the toolstack managed to
>>>> find in said leaf.  I was hoping to put off some of these decisions, but
>>>> they probably need making now.  On the PV side they definitely can't be
>>>> fully hidden, as these leaves are not maskable.
>>> Right, but many are meaningful primarily to kernels, and there we
>>> can hide them.
>>>
>>> Since you're switching from black to white listing here, I also think
>>> we need a default label alongside the "unsupported" one here.
>>> Similarly I would think XSTATE sub-leaves beyond 63 need hiding
>>> now.
>> I would prefer not to do that now.  It is conflated with the future
>> work, deliberately deferred to make this work a manageable size.
> I understand that you need to set some boundaries for this
> first step. But I also think that we shouldn't stop in the middle
> of switching from black listing to white listing here.

It was always a mixed black/white list which has turned a little less
like a blacklist.  There are still elements of both.

All this series is intended to do is turn the levellable feature bits
themselves to being strictly controlled.

~Andrew