Re: [PATCH 1/1] x86/fpu: Allow clearcpuid= to clear several bits

[John Haxby]

> On 22 Apr 2020, at 15:35, Andi Kleen <ak@xxxxxxxxxxxxxxx> wrote:
> 
> 
> Thanks good catch.
> 
>> 	if (cmdline_find_option(boot_command_line, "clearcpuid", arg,
>> -				sizeof(arg)) &&
>> -	    get_option(&argptr, &bit) &&
>> -	    bit >= 0 &&
>> -	    bit < NCAPINTS * 32)
>> -		setup_clear_cpu_cap(bit);
>> +				sizeof(arg))) {
>> +		/* cpuid bit numbers are mostly three digits */
>> +		enum  { nints = sizeof(arg)/(3+1) + 1 };
> 
> Not sure what the digits have to do with the stack space of an int array.
> 
> We should have enough stack to afford some more than 8.

sizeof(arg) == 32; room enough for eight three-digit with their trailing commas.   If sizeof(arg) == 1024 instead then there'd be more than enough room to remove every single feature.   TBH, 512 is more than enough for the 89 flags I have listed on this machine I'm looking at here.   I'll grow sizeof(arg) and nints accordingly.

> 
> Would be good to have a warning if the arguments are longer.
> 

Yes, I should definitely do that -- coming to a V2 soon.


> Maybe it would be simpler to fix the early arg parser
> to allow multiple instances again? That would also avoid the limit,
> and keep everything compatible.
> 

I did wonder about that.   However, cmdline_find_option() is specifically documented as 

 * Find a non-boolean option (i.e. option=argument). In accordance with
 * standard Linux practice, if this option is repeated, this returns the
 * last instance on the command line.

And since that appeared in 2017 I decided to stick with the new-fangled interface :)   This is a little-used feature; I'm not sure it's worth the effort of parsing the command line for the old style.  What do you think?

jch


> -Andi
> 
> 
>> +		int i, bits[nints];
>> +
>> +		get_options(arg, nints, bits);
>> +		for (i = 1; i <= bits[0]; i++) {
>> +			if (bits[i] >= 0 && bits[i] < NCAPINTS * 32)
>> +				setup_clear_cpu_cap(bits[i]);
>> +		}
>> +	}
>> }
>> 
>> /*
>> -- 
>> 2.25.3
>>