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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.02.16 at 11:43, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 16/02/16 10:06, Jan Beulich wrote:
>>>>> On 15.02.16 at 18:12, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 15/02/16 15:43, Jan Beulich wrote:
>>>>>>> On 05.02.16 at 14:42, <andrew.cooper3@xxxxxxxxxx> 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.

>>>>> +    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.

Jan


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