Re: [PATCH 1/2] x86/cpuid: Infrastructure for leaves 7:1{ecx,edx}

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.03.2023 19:32, Andrew Cooper wrote:
> On 03/03/2023 7:23 am, Jan Beulich wrote:
>> On 04.01.2023 12:11, Andrew Cooper wrote:
>>> --- a/xen/include/public/arch-x86/cpufeatureset.h
>>> +++ b/xen/include/public/arch-x86/cpufeatureset.h
>>> @@ -288,6 +288,9 @@ XEN_CPUFEATURE(NSCB,               11*32+ 6) /*A  Null 
>>> Selector Clears Base (and
>>>  /* Intel-defined CPU features, CPUID level 0x00000007:1.ebx, word 12 */
>>>  XEN_CPUFEATURE(INTEL_PPIN,         12*32+ 0) /*   Protected Processor 
>>> Inventory Number */
>>>  
>>> +/* Intel-defined CPU features, CPUID level 0x00000007:1.ecx, word 14 */
>>> +/* Intel-defined CPU features, CPUID level 0x00000007:1.edx, word 15 */
>> While committing the backports of this (where I normally test-build
>> every commit individually) I came to notice that this introduces a
>> transient (until the next commit) build breakage: FEATURESET_NR_ENTRIES
>> is calculated from the highest entry found; the comments here don't
>> matter at all. Therefore ...
>>
>>> @@ -343,6 +352,8 @@ static inline void cpuid_policy_to_featureset(
>>>      fs[FEATURESET_e21a] = p->extd.e21a;
>>>      fs[FEATURESET_7b1] = p->feat._7b1;
>>>      fs[FEATURESET_7d2] = p->feat._7d2;
>>> +    fs[FEATURESET_7c1] = p->feat._7c1;
>>> +    fs[FEATURESET_7d1] = p->feat._7d1;
>>>  }
>>>  
>>>  /* Fill in a CPUID policy from a featureset bitmap. */
>>> @@ -363,6 +374,8 @@ static inline void cpuid_featureset_to_policy(
>>>      p->extd.e21a  = fs[FEATURESET_e21a];
>>>      p->feat._7b1  = fs[FEATURESET_7b1];
>>>      p->feat._7d2  = fs[FEATURESET_7d2];
>>> +    p->feat._7c1  = fs[FEATURESET_7c1];
>>> +    p->feat._7d1  = fs[FEATURESET_7d1];
>>>  }
>> ... the compiler legitimately complains about out-of-bounds array
>> accesses here. This is just fyi for the future (to arrange patch
>> splitting differently); I've left the backports as they were.
> 
> Hmm.  c/s e3662437eb43 was designed to specifically allow CPUID patches
> to be split like this.
> 
> Which compiler?  I think I agree with your analysis, but I've never seen
> a complaint, hence not noticing.

gcc 12

> I suspect we actually want FEATURESET_NR_ENTRIES defined in C, next to
> the FEATURESET_* defines, and we want to BUILD_BUG_ON() if the autogen
> length is larger than the C length.

Hmm, yes, this may be the best we can do.

Jan