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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

At the moment, the toolstack won't ever generate subleaves beyond 63.

~Andrew

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