[PATCH] discard MSR writes

[PATCH] discard MSR writes

[Markus Rechberger]

[-- Attachment #1: Type: text/plain, Size: 381 bytes --]

this patch discards MSR writes to the Performance Event-Select 
Registers, this is the first issue why vista seems to fail although now 
vista ends up in an endless loop a bit later.
Qemu currently also discards those writes.

Signed-off-by: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
Signed-off-by: Markus Rechberger <markus.rechberger-5C7GfCeVMHo@public.gmane.org>

[-- Attachment #2: MSR_K7_EVNTSEL.diff --]
[-- Type: text/plain, Size: 536 bytes --]

diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
index 1c3cc3e..1f34501 100644
--- a/drivers/kvm/svm.c
+++ b/drivers/kvm/svm.c
@@ -1143,6 +1143,12 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 data)
 	case MSR_SYSCALL_MASK:
 		svm->vmcb->save.sfmask = data;
 		break;
+	case MSR_K7_EVNTSEL0:
+	case MSR_K7_EVNTSEL1:
+	case MSR_K7_EVNTSEL2:
+	case MSR_K7_EVNTSEL3:
+		/* discard those writes for now, required by vista 64bit */
+		break;
 #endif
 	case MSR_IA32_SYSENTER_CS:
 		svm->vmcb->save.sysenter_cs = data;

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] discard MSR writes

[Amit Shah]

On Tuesday 20 November 2007 00:38:05 Markus Rechberger wrote:
> this patch discards MSR writes to the Performance Event-Select
> Registers, this is the first issue why vista seems to fail although now
> vista ends up in an endless loop a bit later.
> Qemu currently also discards those writes.

Won't this make the corresponding rdmsrs fail? What happens when the rdmsr 
returns an error, but windows then uses some garbage value (as it thinks the 
wrmsr succeeded, so the rdmsr also should)?

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] discard MSR writes

[Avi Kivity]

Markus Rechberger wrote:
> this patch discards MSR writes to the Performance Event-Select 
> Registers, this is the first issue why vista seems to fail although 
> now vista ends up in an endless loop a bit later.
> Qemu currently also discards those writes.
>
> Signed-off-by: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
> Signed-off-by: Markus Rechberger <markus.rechberger-5C7GfCeVMHo@public.gmane.org>
> diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
> index 1c3cc3e..1f34501 100644
> --- a/drivers/kvm/svm.c
> +++ b/drivers/kvm/svm.c
> @@ -1143,6 +1143,12 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 data)
>  	case MSR_SYSCALL_MASK:
>  		svm->vmcb->save.sfmask = data;
>  		break;
> +	case MSR_K7_EVNTSEL0:
> +	case MSR_K7_EVNTSEL1:
> +	case MSR_K7_EVNTSEL2:
> +	case MSR_K7_EVNTSEL3:
> +		/* discard those writes for now, required by vista 64bit */
> +		break;
>  #endif
>  	case MSR_IA32_SYSENTER_CS:
>  		svm->vmcb->save.sysenter_cs = data;

What value is actually written into those MSRs?  If the value causes the 
counter to be disabled (e.g. the interrupt enabled bit is cleared) we 
can be more clever: discard the write if it disables interrupts, and 
print a message and return -ESOMETHING if it enables interrupts.  This 
way, if some other guest depends on the performance counters, we'll have 
some indication that something is wrong.

btw, is this Vista x86 or x64?



-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] discard MSR writes

[Avi Kivity]

Amit Shah wrote:
> On Tuesday 20 November 2007 00:38:05 Markus Rechberger wrote:
>   
>> this patch discards MSR writes to the Performance Event-Select
>> Registers, this is the first issue why vista seems to fail although now
>> vista ends up in an endless loop a bit later.
>> Qemu currently also discards those writes.
>>     
>
> Won't this make the corresponding rdmsrs fail? What happens when the rdmsr 
> returns an error, but windows then uses some garbage value (as it thinks the 
> wrmsr succeeded, so the rdmsr also should)?
>   

rdmsr will inject #GP for these msrs.  Implementing set_msr() doesn't 
affect rdmsr.


-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] discard MSR writes

[Amit Shah]

On Tuesday 20 November 2007 15:17:54 Avi Kivity wrote:
> Amit Shah wrote:
> > On Tuesday 20 November 2007 00:38:05 Markus Rechberger wrote:
> >> this patch discards MSR writes to the Performance Event-Select
> >> Registers, this is the first issue why vista seems to fail although now
> >> vista ends up in an endless loop a bit later.
> >> Qemu currently also discards those writes.
> >
> > Won't this make the corresponding rdmsrs fail? What happens when the
> > rdmsr returns an error, but windows then uses some garbage value (as it
> > thinks the wrmsr succeeded, so the rdmsr also should)?
>
> rdmsr will inject #GP for these msrs.  Implementing set_msr() doesn't
> affect rdmsr.

>From the AMD programming manual, vol 2:

The performance event-select registers can be read and written only by system 
software running at CPL = 0 using the RDMSR and WRMSR instructions, 
respectively. Any attempt to read or write these registers at CPL > 0 causes 
a general-protection exception to occur.

Also, a grep through the Linux code shows a rdmsrl on EVNTSEL3.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] discard MSR writes

[Avi Kivity]

Amit Shah wrote:
> On Tuesday 20 November 2007 15:17:54 Avi Kivity wrote:
>   
>> Amit Shah wrote:
>>     
>>> On Tuesday 20 November 2007 00:38:05 Markus Rechberger wrote:
>>>       
>>>> this patch discards MSR writes to the Performance Event-Select
>>>> Registers, this is the first issue why vista seems to fail although now
>>>> vista ends up in an endless loop a bit later.
>>>> Qemu currently also discards those writes.
>>>>         
>>> Won't this make the corresponding rdmsrs fail? What happens when the
>>> rdmsr returns an error, but windows then uses some garbage value (as it
>>> thinks the wrmsr succeeded, so the rdmsr also should)?
>>>       
>> rdmsr will inject #GP for these msrs.  Implementing set_msr() doesn't
>> affect rdmsr.
>>     
>
> >From the AMD programming manual, vol 2:
>
> The performance event-select registers can be read and written only by system 
> software running at CPL = 0 using the RDMSR and WRMSR instructions, 
> respectively. Any attempt to read or write these registers at CPL > 0 causes 
> a general-protection exception to occur.
>   

Look through the code that implements rdmsr, it doesn't care about the 
manuals and will happily inject a #GP for rdmsr of any unimplemented msr 
(like PerfEvtSel)  wrmsr and rdmsr implementations are not linked.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] discard MSR writes

[Amit Shah]

On Tuesday 20 November 2007 15:42:35 Avi Kivity wrote:
> Amit Shah wrote:
> > On Tuesday 20 November 2007 15:17:54 Avi Kivity wrote:
> >> Amit Shah wrote:
> >>> On Tuesday 20 November 2007 00:38:05 Markus Rechberger wrote:
> >>>> this patch discards MSR writes to the Performance Event-Select
> >>>> Registers, this is the first issue why vista seems to fail although
> >>>> now vista ends up in an endless loop a bit later.
> >>>> Qemu currently also discards those writes.
> >>>
> >>> Won't this make the corresponding rdmsrs fail? What happens when the
> >>> rdmsr returns an error, but windows then uses some garbage value (as it
> >>> thinks the wrmsr succeeded, so the rdmsr also should)?
> >>
> >> rdmsr will inject #GP for these msrs.  Implementing set_msr() doesn't
> >> affect rdmsr.
> >>
> > >From the AMD programming manual, vol 2:
> >
> > The performance event-select registers can be read and written only by
> > system software running at CPL = 0 using the RDMSR and WRMSR
> > instructions, respectively. Any attempt to read or write these registers
> > at CPL > 0 causes a general-protection exception to occur.
>
> Look through the code that implements rdmsr, it doesn't care about the
> manuals and will happily inject a #GP for rdmsr of any unimplemented msr
> (like PerfEvtSel)  wrmsr and rdmsr implementations are not linked.

That's right; but isn't that wrong if we cause it? I mean if we just allow the 
wrmsr access to go through (and if they're actually used, not disabled as you 
mentioned separately), then there'll be no interrupts when the guest expects 
them to occur, or the rdmsr will fail, when the guest thinks it shouldn't 
have.

I guess we're putting forth the same point: if the wrmsr is not for disabling 
interrupts, we shouldn't let it go through, or just implement the required 
emulation.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] discard MSR writes

[Avi Kivity]

Amit Shah wrote:
> On Tuesday 20 November 2007 15:42:35 Avi Kivity wrote:
>   
>> Amit Shah wrote:
>>     
>>> On Tuesday 20 November 2007 15:17:54 Avi Kivity wrote:
>>>       
>>>> Amit Shah wrote:
>>>>         
>>>>> On Tuesday 20 November 2007 00:38:05 Markus Rechberger wrote:
>>>>>           
>>>>>> this patch discards MSR writes to the Performance Event-Select
>>>>>> Registers, this is the first issue why vista seems to fail although
>>>>>> now vista ends up in an endless loop a bit later.
>>>>>> Qemu currently also discards those writes.
>>>>>>             
>>>>> Won't this make the corresponding rdmsrs fail? What happens when the
>>>>> rdmsr returns an error, but windows then uses some garbage value (as it
>>>>> thinks the wrmsr succeeded, so the rdmsr also should)?
>>>>>           
>>>> rdmsr will inject #GP for these msrs.  Implementing set_msr() doesn't
>>>> affect rdmsr.
>>>>
>>>>         
>>> >From the AMD programming manual, vol 2:
>>>
>>> The performance event-select registers can be read and written only by
>>> system software running at CPL = 0 using the RDMSR and WRMSR
>>> instructions, respectively. Any attempt to read or write these registers
>>> at CPL > 0 causes a general-protection exception to occur.
>>>       
>> Look through the code that implements rdmsr, it doesn't care about the
>> manuals and will happily inject a #GP for rdmsr of any unimplemented msr
>> (like PerfEvtSel)  wrmsr and rdmsr implementations are not linked.
>>     
>
> That's right; but isn't that wrong if we cause it? I mean if we just allow the 
> wrmsr access to go through (and if they're actually used, not disabled as you 
> mentioned separately), then there'll be no interrupts when the guest expects 
> them to occur, or the rdmsr will fail, when the guest thinks it shouldn't 
> have.
>
>   

It is wrong; but at least it fails loudly.  We can't implement all msrs 
(Intel and AMD are adding them faster than we can code), so we must make 
sure that where we don't implement things, at least we have visibility 
if the guest tries to use them.

> I guess we're putting forth the same point: if the wrmsr is not for disabling 
> interrupts, we shouldn't let it go through, or just implement the required 
> emulation.
>   

Yes.  Ignoring an msr will "fix" one guest but kill another.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] discard MSR writes

[Markus Rechberger]

Avi Kivity wrote:
> Amit Shah wrote:
>> On Tuesday 20 November 2007 15:42:35 Avi Kivity wrote:
>>  
>>> Amit Shah wrote:
>>>    
>>>> On Tuesday 20 November 2007 15:17:54 Avi Kivity wrote:
>>>>      
>>>>> Amit Shah wrote:
>>>>>        
>>>>>> On Tuesday 20 November 2007 00:38:05 Markus Rechberger wrote:
>>>>>>          
>>>>>>> this patch discards MSR writes to the Performance Event-Select
>>>>>>> Registers, this is the first issue why vista seems to fail although
>>>>>>> now vista ends up in an endless loop a bit later.
>>>>>>> Qemu currently also discards those writes.
>>>>>>>             
>>>>>> Won't this make the corresponding rdmsrs fail? What happens when the
>>>>>> rdmsr returns an error, but windows then uses some garbage value 
>>>>>> (as it
>>>>>> thinks the wrmsr succeeded, so the rdmsr also should)?
>>>>>>           
>>>>> rdmsr will inject #GP for these msrs.  Implementing set_msr() doesn't
>>>>> affect rdmsr.
>>>>>
>>>>>         
>>>> >From the AMD programming manual, vol 2:
>>>>
>>>> The performance event-select registers can be read and written only by
>>>> system software running at CPL = 0 using the RDMSR and WRMSR
>>>> instructions, respectively. Any attempt to read or write these 
>>>> registers
>>>> at CPL > 0 causes a general-protection exception to occur.
>>>>       
>>> Look through the code that implements rdmsr, it doesn't care about the
>>> manuals and will happily inject a #GP for rdmsr of any unimplemented 
>>> msr
>>> (like PerfEvtSel)  wrmsr and rdmsr implementations are not linked.
>>>     
>>
>> That's right; but isn't that wrong if we cause it? I mean if we just 
>> allow the wrmsr access to go through (and if they're actually used, 
>> not disabled as you mentioned separately), then there'll be no 
>> interrupts when the guest expects them to occur, or the rdmsr will 
>> fail, when the guest thinks it shouldn't have.
>>
>>   
>
> It is wrong; but at least it fails loudly.  We can't implement all 
> msrs (Intel and AMD are adding them faster than we can code), so we 
> must make sure that where we don't implement things, at least we have 
> visibility if the guest tries to use them.
>
>> I guess we're putting forth the same point: if the wrmsr is not for 
>> disabling interrupts, we shouldn't let it go through, or just 
>> implement the required emulation.
>>   
>
> Yes.  Ignoring an msr will "fix" one guest but kill another.
>
I also discussed this with Joerg, since Qemu doesn't handle those MSR 
writes at the moment we think it's ok temporary. Lateron it should be 
emulated (but we're hunting a different issue at the moment). Our 
perfmon guys would also prefer a proper emulation.

Markus



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] discard MSR writes

[Avi Kivity]

Markus Rechberger wrote:
> I also discussed this with Joerg, since Qemu doesn't handle those MSR 
> writes at the moment we think it's ok temporary. Lateron it should be 
> emulated (but we're hunting a different issue at the moment). Our 
> perfmon guys would also prefer a proper emulation.
>

There's no need to copy mistakes from qemu.  I too would prefer proper 
emulation, but meanwhile, if Vista disables performance monitoring when 
writing to these msrs, we can allow it, and error out if some other 
guest attempts to enable performance monitoring (which will fail silently).

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/