Re: [PATCH] V2 Handle guest access to BBL_CR_CTL3 MSR

[Marcelo Tosatti <mtosatti@redhat.com>]

On Mon, Jan 31, 2011 at 03:48:36PM -0500, john cooper wrote:
> Marcelo Tosatti wrote:
> > On Fri, Jan 21, 2011 at 12:21:00AM -0500, john cooper wrote:
> >> [Resubmit of prior version which contained a wayward
> >> patch hunk.  Thanks Marcelo]
> >>
> >> A correction to Intel cpu model CPUID data (patch queued)
> >> caused winxp 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 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 4d0dfa0..5bfafb6 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 bcc0efc..04d6c55 100644
> >> --- a/arch/x86/kvm/x86.c
> >> +++ b/arch/x86/kvm/x86.c
> >> @@ -1592,6 +1592,12 @@ 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:
> >> +		/* 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 == vcpu->kvm->arch.xen_hvm_config.msr))
> >>  			return xen_hvm_config(vcpu, data);
> >> @@ -1846,6 +1852,19 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
> >>  		} else
> >>  			return get_msr_hyperv(vcpu, msr, pdata);
> >>  		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;
> > 
> > Why bits 26-29 and 31 enabled?
> 
> That was a judgment call.  Intel's documentation for msr 0x11e
> defines them as reserved bits, however they are returned as 0xbe
> for the Penryn class processors I'd checked.  Not exhaustive, yet
> bit #19 at the root of the problem is also documented as reserved.
> I was attempting to err in favor of actual deployed silicon.
> 
> Thanks,
> 
> -john

OK. Applied, thanks.