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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.09.2019 17:27, Andrew Cooper wrote:
> 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.

In which case, why don't you move the three variables into the do/while
scope? Anyway, with the annotation correction and the variables non-
static (in whichever scope)
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan

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