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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

>>> @@ -4694,6 +4687,9 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, 
>>> unsigned int *ebx,
>>>          break;
>>>  
>>>      case 0x80000001:
>>> +        *ecx &= hvm_featureset[FEATURESET_e1c];
>>> +        *edx &= hvm_featureset[FEATURESET_e1d];
>> Looking at the uses here, wouldn't it be better (less ambiguous) for
>> these to be named FEATURESET_x1c and FEATURESET_x1d
>> respectively, since x is not a hex digit, but e is?
> 
> I originally chose e because these are commonly known as the extended
> cpuid leaves.  'e' being a hex digit is why the number is specified as
> an upper case hex number, as shown with FEATURESET_Da1.
> 
> I suppose x could work here, but I don't see being less ambiguous. 
> (Perhaps I am clouded by having this syntax firmly embedded in my
> expectation).

Okay then, I just wanted to point out that the current naming may
be confusing to others (and to a degree it is to me).

>>>      switch ( leaf )
>>>      {
>>>      case 0x00000001:
>>> -        /* Modify Feature Information. */
>>> -        if ( !cpu_has_sep )
>>> -            __clear_bit(X86_FEATURE_SEP, &d);
>>> -        __clear_bit(X86_FEATURE_DS, &d);
>>> -        __clear_bit(X86_FEATURE_ACC, &d);
>>> -        __clear_bit(X86_FEATURE_PBE, &d);
>>> -        if ( is_pvh_domain(currd) )
>>> -            __clear_bit(X86_FEATURE_MTRR, &d);
>>> -
>>> -        __clear_bit(X86_FEATURE_DTES64 % 32, &c);
>>> -        __clear_bit(X86_FEATURE_MWAIT % 32, &c);
>>> -        __clear_bit(X86_FEATURE_DSCPL % 32, &c);
>>> -        __clear_bit(X86_FEATURE_VMXE % 32, &c);
>>> -        __clear_bit(X86_FEATURE_SMXE % 32, &c);
>>> -        __clear_bit(X86_FEATURE_TM2 % 32, &c);
>>> +        c &= pv_featureset[FEATURESET_1c];
>>> +        d &= pv_featureset[FEATURESET_1d];
>>> +
>>>          if ( is_pv_32bit_domain(currd) )
>>> -            __clear_bit(X86_FEATURE_CX16 % 32, &c);
>>> -        __clear_bit(X86_FEATURE_XTPR % 32, &c);
>>> -        __clear_bit(X86_FEATURE_PDCM % 32, &c);
>>> -        __clear_bit(X86_FEATURE_PCID % 32, &c);
>>> -        __clear_bit(X86_FEATURE_DCA % 32, &c);
>>> -        if ( !cpu_has_xsave )
>>> -        {
>>> -            __clear_bit(X86_FEATURE_XSAVE % 32, &c);
>>> -            __clear_bit(X86_FEATURE_AVX % 32, &c);
>>> -        }
>>> -        if ( !cpu_has_apic )
>>> -           __clear_bit(X86_FEATURE_X2APIC % 32, &c);
>>> -        __set_bit(X86_FEATURE_HYPERVISOR % 32, &c);
>>> +            c &= ~cpufeat_mask(X86_FEATURE_CX16);
>> Shouldn't this be taken care of by clearing LM and then applying
>> your dependencies correction? Or is that meant to only get
>> enforced later? Is it maybe still worth having both pv64_featureset[]
>> and pv32_featureset[]?
> 
> Again, this is just used as a halfway house.  Longterm, I plan to read
> the featureset straight from the per-domain policy, which won't be
> stored as an adhoc array.

Okay, that's certainly fine in this case I suppose.

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

Jan

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