Re: [Xen-devel] [PATCH v2] x86/cpuid: Extend the cpuid= option to support all named features

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/09/2019 16:18, Jan Beulich wrote:
> On 05.09.2019 21:49, Andrew Cooper wrote:
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -21,45 +21,62 @@ static const uint32_t deep_features[] = 
>> INIT_DEEP_FEATURES;
>>  
>>  static int __init parse_xen_cpuid(const char *s)
>>  {
>> +    static const struct feature {
>> +        const char *name;
>> +        unsigned int bit;
>> +    } features[] __initconst = INIT_FEATURE_NAMES, *lhs, *mid, *rhs;
> The pointer field want this to use __initconstrel.

Ok.

> And I don't think you mean lhs, mid, and rhs to also be static?

No - not intentional.

>  Albeit ...
>
>>      const char *ss;
>>      int val, rc = 0;
>>  
>>      do {
>> +        const char *feat;
>> +
>>          ss = strchr(s, ',');
>>          if ( !ss )
>>              ss = strchr(s, '\0');
>>  
>> -        if ( (val = parse_boolean("md-clear", s, ss)) >= 0 )
>> -        {
>> -            if ( !val )
>> -                setup_clear_cpu_cap(X86_FEATURE_MD_CLEAR);
>> -        }
>> -        else if ( (val = parse_boolean("ibpb", s, ss)) >= 0 )
>> -        {
>> -            if ( !val )
>> -                setup_clear_cpu_cap(X86_FEATURE_IBPB);
>> -        }
>> -        else if ( (val = parse_boolean("ibrsb", s, ss)) >= 0 )
>> -        {
>> -            if ( !val )
>> -                setup_clear_cpu_cap(X86_FEATURE_IBRSB);
>> -        }
>> -        else if ( (val = parse_boolean("stibp", s, ss)) >= 0 )
>> -        {
>> -            if ( !val )
>> -                setup_clear_cpu_cap(X86_FEATURE_STIBP);
>> -        }
>> -        else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
>> -        {
>> -            if ( !val )
>> -                setup_clear_cpu_cap(X86_FEATURE_L1D_FLUSH);
>> -        }
>> -        else if ( (val = parse_boolean("ssbd", s, ss)) >= 0 )
>> +        /* Skip the 'no-' prefix for name comparisons. */
>> +        feat = s;
>> +        if ( strncmp(s, "no-", 3) == 0 )
>> +            feat += 3;
>> +
>> +        /* (Re)initalise lhs and rhs for binary search. */
>> +        lhs = features;
>> +        rhs = features + ARRAY_SIZE(features);
> ... the comment here suggests you do, but I don't currently see why.

We are inside a do { } () while loop, parsing something such as
cpuid=avx512,ss,smx

The binary search over the feature names needs to start again from
scratch for each new cpuid= list item.

Otherwise, the while ( lhs < rhs ) binary search will never be entered
for the second cpuid= item.

~Andrew

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