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

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

On 16/07/18 10:38, Jan Beulich wrote:
>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>> --- a/tools/include/xen-tools/libs.h
>> +++ b/tools/include/xen-tools/libs.h
>> @@ -13,4 +13,8 @@
>>  #define ARRAY_SIZE(a) (sizeof(a) / sizeof(*a))
>>  #endif
>>  
>> +#ifndef MAX
>> +#define MAX(x, y) ((x) > (y) ? (x) : (y))
>> +#endif
> I find asymmetries like this odd: There should then also be MIN() imo.

Patch 7, where it is first used.

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

~Andrew

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