Re: [PATCH] x86/intel: Protect set_cpuidmask() against #GP faults

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

On 05/06/14 15:05, Jan Beulich wrote:
>>>> On 05.06.14 at 13:19, <andrew.cooper3@citrix.com> wrote:
>> Virtual environments such as Xen HVM containers and VirtualBox do not
>> necessarily provide support for feature masking MSRs.
>>
>> As their presence is detected by model numbers alone, and their use 
>> predicated
>> on command line parameters, use the safe() variants of {wr,rd}msr() to avoid
>> dying with an early #GP fault.
> I'm tempted to say "Then just don't pass these options." There are
> other options that can lead to boot failure if not used properly.

That is one opinion, but I would disagree.  There are few things as bad
as making a mistake on the command line and ending with a broken server
somewhere on the other side of the world because it failed to boot,
especially if Xen can deal sensibly with the failure and still boot the
server.

>
>> * Call set_cpuidmask() unconditionally so faulting-capable hardware still gets
>>   a log message indicating to the user why their command line arguments are
>>   not taking effect.
> I don't think Intel will particularly like this part.

Why not?  Until someone (most likely myself, following the migration
series is accepted) fixes faulting to actually be able to provide a
policy, users attempting to use cpuid_mask_XXX to perform levelling end
up with an unlevelled server and no hint as to why.

>
>> -	/* Only family 6 supports this feature  */
>> -	switch ((c->x86 == 6) * c->x86_model) {
>> -	case 0x17:
>> -		if ((c->x86_mask & 0x0f) < 4)
>> -			break;
> I had been inquiring about this stepping specific check here too,
> without ever getting clarification. I think we shouldn't blindly
> drop it.

I did some archaeology in the history.  This check was introduced
exactly as-is with the introduction of the masking code, without
specific with regard to the stepping.

The latest spec states "Extended model ID 1H and models 7H and DH (for
all values of stepping ID)", so I would opt for dropping the check.

I wonder whether I can find hardware in XenRT which would normally fail
that specific check.

>
>> + setmask:
>> +	if (msr_basic &&
>> +	    wrmsr_safe(msr_basic,
>> +		       ((u64)opt_cpuid_mask_edx << 32) | opt_cpuid_mask_ecx)){
>> +		msr_basic = 0;
>> +		printk("Failed to set CPUID feature mask\n");
>> +	}
>> +
>> +	if (msr_ext &&
>> +	    wrmsr_safe(msr_ext,
>> +		       ((u64)opt_cpuid_mask_ext_edx << 32) | opt_cpuid_mask_ext_ecx)){
>> +		msr_ext = 0;
>> +		printk("Failed to set CPUID extended feature mask\n");
>> +	}
>> +
>> +	if (msr_xsave &&
>> +	    (rdmsr_safe(msr_xsave, msr_val) ||
>> +	     wrmsr_safe(msr_xsave,
>> +			((u64)msr_val << 32) | opt_cpuid_mask_xsave_eax))){
> I'm afraid copy'n'paste went a little too far here: Neither does msr_val
> need a u64 cast, nor do you want to write into the high half what you
> read from the low one.
>
> Jan
>

Oops - indeed it did.

This turns out to be safe as the MSRs initial value is ~0ULL, which is
why it passed my dev test.  I will fix up for v2.

~Andrew