Re: [PATCH v2 04/13] libx86: Share struct cpuid_policy with userspace

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Sergey Dyasli <sergey.dyasli@citrix.com>,
	Xen-devel <xen-devel@lists.xen.org>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH v2 04/13] libx86: Share struct cpuid_policy with userspace
Date: Mon, 16 Jul 2018 11:16:10 +0100	[thread overview]
Message-ID: <e8fb6303-1d5d-71b9-7f74-ca53fcedca9e@citrix.com> (raw)
In-Reply-To: <5B4C6DB402000078001D4416@prv1-mh.provo.novell.com>

On 16/07/18 11:04, Jan Beulich wrote:
>>>> On 16.07.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
>> On 16/07/18 10:38, Jan Beulich wrote:
>>>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>>>> +static inline void cpuid_featureset_to_policy(
>>>> +    const uint32_t fs[FEATURESET_NR_ENTRIES], struct cpuid_policy *p)
>>>> +{
>>>> +    p->basic._1d  = fs[FEATURESET_1d];
>>>> +    p->basic._1c  = fs[FEATURESET_1c];
>>>> +    p->extd.e1d   = fs[FEATURESET_e1d];
>>>> +    p->extd.e1c   = fs[FEATURESET_e1c];
>>>> +    p->xstate.Da1 = fs[FEATURESET_Da1];
>>>> +    p->feat._7b0  = fs[FEATURESET_7b0];
>>>> +    p->feat._7c0  = fs[FEATURESET_7c0];
>>>> +    p->extd.e7d   = fs[FEATURESET_e7d];
>>>> +    p->extd.e8b   = fs[FEATURESET_e8b];
>>>> +    p->feat._7d0  = fs[FEATURESET_7d0];
>>>> +}
>>> I realize this is only code movement, but since you didn't answer the
>>> question raised on the Intel Process Trace thread (v2 03/10) yet, I'll
>>> raise it here again: Shouldn't other fields of p be set to zero here?
>> No - why should it?
>>
>> (In fact, it very deliberately does not, and changing this will break
>> all of the policy derivation logic.)
> Did you look at the context in which I've raised the question originally?

Yes, but I still don't understand what prompted the question.

There is not a single use of caller cpuid_featureset_to_policy() where
zeroing the rest of the policy would be an appropriate or reasonable
thing to do.

> I did in particular ask about this effectively still being a form of black
> listing (I said "white listing" there by mistake), just taking leaf 6 (and
> Intel hardware) as example. guest_cpuid() takes whatever is there in
> the policy, and update_domain_cpuid_info() does nothing either.

See the head of recalculate_misc() which unilaterally zeroes that leaf,
irrespective of toolstack settings.

> It was my understanding that with the switch to the new model we're
> now strictly while listing features. Luwei's patch would extend the
> un-audited handing through to RDT and SGX leaves.

That is one reason why I won't ack the patch in that shape.  But as I'm
currently rewriting this logic for a different reason, explaining the
correct steps to extend MAX_*_LEAF will cause unnecessary work for Luwei
as the implementation is about to change under his feet.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel