Re: [PATCH] Handle guest access to BBL_CR_CTL3 MSR

[john cooper <john.cooper@third-harmonic.com>]

Marcelo Tosatti wrote:
> On Sat, Jan 08, 2011 at 12:05:14AM -0500, john cooper wrote:
>   
>> A correction to Intel cpu model CPUID data (patch queued)
>> caused winxp-64 to BSOD when booted with a Penryn model.
>> This was traced to the CPUID "model" field correction from
>> 6 -> 23 (as is proper for a Penryn class of cpu).  Only in
>> this case does the problem surface.
>>
>> The cause for this failure is winxp accessing the BBL_CR_CTL3
>> MSR which is unsupported by current kvm, appears to be a
>> legacy MSR not fully characterized yet existing in current
>> silicon, and is apparently carried forward in MSR space to
>> accommodate vintage code as here.  It is not yet conclusive
>> whether this MSR implements any of its legacy functionality
>> or is just an ornamental dud for compatibility.  While I
>> found no silicon version specific documentation link to
>> this MSR, a general description exists in Intel's developer's
>> reference which agrees with the functional behavior of
>> other bootloader/kernel code I've examined accessing
>> BBL_CR_CTL3.  Regrettably winxp-64 appears to be setting bit #19
>> called out as "reserved" in the above document.
>>
>> So to minimally accommodate this MSR, kvm msr get will provide
>> the equivalent mock data and kvm msr write will simply toss the
>> guest passed data without interpretation.  While this treatment
>> of BBL_CR_CTL3 addresses the immediate problem, the approach may
>> be modified pending clarification from Intel.
>>
>> Signed-off-by: john cooper <john.cooper@redhat.com>
>> ---
>>
>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> index 6b89f5e..145cd60 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -38,6 +38,7 @@
>>  
>>  #define MSR_MTRRcap			0x000000fe
>>  #define MSR_IA32_BBL_CR_CTL		0x00000119
>> +#define MSR_IA32_BBL_CR_CTL3		0x0000011e
>>  
>>  #define MSR_IA32_SYSENTER_CS		0x00000174
>>  #define MSR_IA32_SYSENTER_ESP		0x00000175
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index fa708c9..9a8331c 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -1283,6 +1283,12 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>>  			return -1;
>>  		vcpu->arch.mcg_ctl = data;
>>  		break;
>> +	case MSR_IA32_BBL_CR_CTL3:
>> +		/* Drop writes to this legacy MSR -- see rdmsr
>> +		 * counterpart for further detail.
>> +		 */
>> +		pr_unimpl(vcpu, "ignored wrmsr: 0x%x data %llx\n", msr, data);
>> +		break;
>>  	default:
>>  		if (msr >= MSR_IA32_MC0_CTL &&
>>  		    msr < MSR_IA32_MC0_CTL + 4 * bank_num) {
>> @@ -1592,6 +1598,19 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>>  		} else
>>  			return set_msr_hyperv(vcpu, msr, data);
>>  		break;
>> +	case MSR_IA32_BBL_CR_CTL3:
>> +		/* This legacy MSR exists but isn't fully documented in current
>> +		 * silicon.  It is however accessed by winxp in very narrow
>> +		 * scenarios where it sets bit #19, itself documented as
>> +		 * a "reserved" bit.  Best effort attempt to source coherent
>> +		 * read data here should the balance of the register be
>> +		 * interpreted by the guest:
>> +		 *
>> +		 * L2 cache control register 3: 64GB range, 256KB size,
>> +		 * enabled, latency 0x1, configured
>> +		 */ 
>> +		data = 0xbe702111;
>> +		break;
>>  	default:
>>  		if (msr && (msr == vcpu->kvm->arch.xen_hvm_config.msr))
>>  			return xen_hvm_config(vcpu, data);
>>     
>
> This is the MSR write path ?
>   
The write path ignores the data. The MSR read returns a
mock-up of BBL_CR_CTL3 data. The above addresses the
narrow usage leading to the immediate bug. From the
feedback thus far from Intel I believe the above is sufficient
and minimal.

-john



-- 
john.cooper@third-harmonic.com