[PATCH] Ignore DEBUGCTL MSRs

[PATCH] Ignore DEBUGCTL MSRs

[Alexander Graf]

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

Netware writes and reads to the DEBUGCTL and LAST*IP MSRs without 
further checks and is really confused to receive a #GP during that. To 
make it happy we should just make them stubs, which is exactly what SVM 
already does.

To support VMX too, I put these in the generic code. Maybe the SVM code 
could be cleaned up to use generic code too.

Signed-off-by: Alexander Graf <agraf@suse.de>



[-- Attachment #2: kvm-netware.patch --]
[-- Type: text/x-patch, Size: 902 bytes --]

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fc0721e..02f8490 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -609,6 +609,11 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 		pr_unimpl(vcpu, "%s: MSR_IA32_MCG_CTL 0x%llx, nop\n",
 			__func__, data);
 		break;
+	case MSR_IA32_DEBUGCTLMSR:
+	case MSR_IA32_LASTBRANCHFROMIP:
+	case MSR_IA32_LASTBRANCHTOIP:
+	case MSR_IA32_LASTINTFROMIP:
+	case MSR_IA32_LASTINTTOIP:
 	case MSR_IA32_UCODE_REV:
 	case MSR_IA32_UCODE_WRITE:
 		break;
@@ -705,6 +710,11 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 	case MSR_IA32_MC0_MISC+16:
 	case MSR_IA32_UCODE_REV:
 	case MSR_IA32_EBL_CR_POWERON:
+	case MSR_IA32_DEBUGCTLMSR:
+	case MSR_IA32_LASTBRANCHFROMIP:
+	case MSR_IA32_LASTBRANCHTOIP:
+	case MSR_IA32_LASTINTFROMIP:
+	case MSR_IA32_LASTINTTOIP:
 		data = 0;
 		break;
 	case MSR_MTRRcap:

Re: [PATCH] Ignore DEBUGCTL MSRs

[Joerg Roedel]

Hi Alex,

On Tue, Jun 24, 2008 at 07:04:45AM +0200, Alexander Graf wrote:
> Netware writes and reads to the DEBUGCTL and LAST*IP MSRs without
> further checks and is really confused to receive a #GP during that. To
> make it happy we should just make them stubs, which is exactly what
> SVM already does.
> 
> To support VMX too, I put these in the generic code. Maybe the SVM
> code could be cleaned up to use generic code too.

I would prefer if you put that into the VMX specific code. We can't move
the SVM parts of it into generic code because Barcelona has hardware
support to virtualize these registers. Therefore SVM don't need that
in generic code.

> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> 

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fc0721e..02f8490 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -609,6 +609,11 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  		pr_unimpl(vcpu, "%s: MSR_IA32_MCG_CTL 0x%llx, nop\n",
>  			__func__, data);
>  		break;
> +	case MSR_IA32_DEBUGCTLMSR:
> +	case MSR_IA32_LASTBRANCHFROMIP:
> +	case MSR_IA32_LASTBRANCHTOIP:
> +	case MSR_IA32_LASTINTFROMIP:
> +	case MSR_IA32_LASTINTTOIP:
>  	case MSR_IA32_UCODE_REV:
>  	case MSR_IA32_UCODE_WRITE:
>  		break;
> @@ -705,6 +710,11 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
>  	case MSR_IA32_MC0_MISC+16:
>  	case MSR_IA32_UCODE_REV:
>  	case MSR_IA32_EBL_CR_POWERON:
> +	case MSR_IA32_DEBUGCTLMSR:
> +	case MSR_IA32_LASTBRANCHFROMIP:
> +	case MSR_IA32_LASTBRANCHTOIP:
> +	case MSR_IA32_LASTINTFROMIP:
> +	case MSR_IA32_LASTINTTOIP:
>  		data = 0;
>  		break;
>  	case MSR_MTRRcap:


-- 
           |           AMD Saxony Limited Liability Company & Co. KG
 Operating |         Wilschdorfer Landstr. 101, 01109 Dresden, Germany
 System    |                  Register Court Dresden: HRA 4896
 Research  |              General Partner authorized to represent:
 Center    |             AMD Saxony LLC (Wilmington, Delaware, US)
           | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

Re: [PATCH] Ignore DEBUGCTL MSRs

[Alexander Graf]

Hi Joerg,

On Jun 24, 2008, at 3:40 PM, Joerg Roedel wrote:

> Hi Alex,
>
> On Tue, Jun 24, 2008 at 07:04:45AM +0200, Alexander Graf wrote:
>> Netware writes and reads to the DEBUGCTL and LAST*IP MSRs without
>> further checks and is really confused to receive a #GP during that.  
>> To
>> make it happy we should just make them stubs, which is exactly what
>> SVM already does.
>>
>> To support VMX too, I put these in the generic code. Maybe the SVM
>> code could be cleaned up to use generic code too.
>
> I would prefer if you put that into the VMX specific code. We can't  
> move
> the SVM parts of it into generic code because Barcelona has hardware
> support to virtualize these registers. Therefore SVM don't need that
> in generic code.

Hum, I'd actually prefer not to handle this specifically in the VMX  
code. MSRs should be handled in a generic way if they do not need  
special treatment from the extension side, as in your case.
For example if a third virtualization extension might come to life, I  
would rather see more things handled in x86.c than in all three targets.

Any objections to this? If not, please apply, as it makes Netware work  
in KVM.

Alex

>
>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>
>>
>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index fc0721e..02f8490 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -609,6 +609,11 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu,  
>> u32 msr, u64 data)
>> 		pr_unimpl(vcpu, "%s: MSR_IA32_MCG_CTL 0x%llx, nop\n",
>> 			__func__, data);
>> 		break;
>> +	case MSR_IA32_DEBUGCTLMSR:
>> +	case MSR_IA32_LASTBRANCHFROMIP:
>> +	case MSR_IA32_LASTBRANCHTOIP:
>> +	case MSR_IA32_LASTINTFROMIP:
>> +	case MSR_IA32_LASTINTTOIP:
>> 	case MSR_IA32_UCODE_REV:
>> 	case MSR_IA32_UCODE_WRITE:
>> 		break;
>> @@ -705,6 +710,11 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu,  
>> u32 msr, u64 *pdata)
>> 	case MSR_IA32_MC0_MISC+16:
>> 	case MSR_IA32_UCODE_REV:
>> 	case MSR_IA32_EBL_CR_POWERON:
>> +	case MSR_IA32_DEBUGCTLMSR:
>> +	case MSR_IA32_LASTBRANCHFROMIP:
>> +	case MSR_IA32_LASTBRANCHTOIP:
>> +	case MSR_IA32_LASTINTFROMIP:
>> +	case MSR_IA32_LASTINTTOIP:
>> 		data = 0;
>> 		break;
>> 	case MSR_MTRRcap:
>

Re: [PATCH] Ignore DEBUGCTL MSRs

[Avi Kivity]

Joerg Roedel wrote:
> Hi Alex,
>
> On Tue, Jun 24, 2008 at 07:04:45AM +0200, Alexander Graf wrote:
>   
>> Netware writes and reads to the DEBUGCTL and LAST*IP MSRs without
>> further checks and is really confused to receive a #GP during that. To
>> make it happy we should just make them stubs, which is exactly what
>> SVM already does.
>>
>> To support VMX too, I put these in the generic code. Maybe the SVM
>> code could be cleaned up to use generic code too.
>>     
>
> I would prefer if you put that into the VMX specific code. We can't move
> the SVM parts of it into generic code because Barcelona has hardware
> support to virtualize these registers. Therefore SVM don't need that
> in generic code.
>   

The code path is svm_set_msr -> kvm_set_msr_common, so even if it is in 
the common code path, you can still override it in svm specific code.

-- 

I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH] Ignore DEBUGCTL MSRs

[Avi Kivity]

Alexander Graf wrote:
> Netware writes and reads to the DEBUGCTL and LAST*IP MSRs without 
> further checks and is really confused to receive a #GP during that. To 
> make it happy we should just make them stubs, which is exactly what 
> SVM already does.
>
> To support VMX too, I put these in the generic code. Maybe the SVM 
> code could be cleaned up to use generic code too.
>

Please add a pr_unimpl() when bits that cause a real processor to do 
something are set.



-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH] Ignore DEBUGCTL MSRs

[Alexander Graf]

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

Avi Kivity wrote:
> Alexander Graf wrote:
>> Netware writes and reads to the DEBUGCTL and LAST*IP MSRs without 
>> further checks and is really confused to receive a #GP during that. 
>> To make it happy we should just make them stubs, which is exactly 
>> what SVM already does.
>>
>> To support VMX too, I put these in the generic code. Maybe the SVM 
>> code could be cleaned up to use generic code too.
>>
>
> Please add a pr_unimpl() when bits that cause a real processor to do 
> something are set.

Like this? I also removed the set handlers for the *IP MSRs, as these 
are read only and made it only handle debug bits, no perfmon bits.

Signed-off-by: Alexander Graf <agraf@suse.de>



[-- Attachment #2: kvm-netware.patch --]
[-- Type: text/x-patch, Size: 1045 bytes --]

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fc0721e..10f5e95 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -609,6 +609,15 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 		pr_unimpl(vcpu, "%s: MSR_IA32_MCG_CTL 0x%llx, nop\n",
 			__func__, data);
 		break;
+	case MSR_IA32_DEBUGCTLMSR:
+		if (data > (DEBUGCTLMSR_LBR | DEBUGCTLMSR_BTF)) {
+			/* Values other than LBR and BTF are vendor-specific,
+			   thus reserved and should throw a #GP */
+			return 1;
+		}
+		pr_unimpl(vcpu, "%s: MSR_IA32_DEBUGCTLMSR 0x%llx, nop\n",
+			__func__, data);
+		break;
 	case MSR_IA32_UCODE_REV:
 	case MSR_IA32_UCODE_WRITE:
 		break;
@@ -705,6 +714,11 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 	case MSR_IA32_MC0_MISC+16:
 	case MSR_IA32_UCODE_REV:
 	case MSR_IA32_EBL_CR_POWERON:
+	case MSR_IA32_DEBUGCTLMSR:
+	case MSR_IA32_LASTBRANCHFROMIP:
+	case MSR_IA32_LASTBRANCHTOIP:
+	case MSR_IA32_LASTINTFROMIP:
+	case MSR_IA32_LASTINTTOIP:
 		data = 0;
 		break;
 	case MSR_MTRRcap:

Re: [PATCH] Ignore DEBUGCTL MSRs

[Avi Kivity]

Alexander Graf wrote:
> Avi Kivity wrote:
>> Alexander Graf wrote:
>>> Netware writes and reads to the DEBUGCTL and LAST*IP MSRs without 
>>> further checks and is really confused to receive a #GP during that. 
>>> To make it happy we should just make them stubs, which is exactly 
>>> what SVM already does.
>>>
>>> To support VMX too, I put these in the generic code. Maybe the SVM 
>>> code could be cleaned up to use generic code too.
>>>
>>
>> Please add a pr_unimpl() when bits that cause a real processor to do 
>> something are set.
>
> Like this? I also removed the set handlers for the *IP MSRs, as these 
> are read only and made it only handle debug bits, no perfmon bits.
>

With a changelog entry.

> Signed-off-by: Alexander Graf <agraf@suse.de>
>
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fc0721e..10f5e95 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -609,6 +609,15 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  		pr_unimpl(vcpu, "%s: MSR_IA32_MCG_CTL 0x%llx, nop\n",
>  			__func__, data);
>  		break;
> +	case MSR_IA32_DEBUGCTLMSR:
> +		if (data > (DEBUGCTLMSR_LBR | DEBUGCTLMSR_BTF)) {
> +			/* Values other than LBR and BTF are vendor-specific,
> +			   thus reserved and should throw a #GP */
> +			return 1;
> +		}
>   
'>' is too clever. '& ~(u64)(...)' is clearer.

Arithmetic and logical ops don't mix well.


> +		pr_unimpl(vcpu, "%s: MSR_IA32_DEBUGCTLMSR 0x%llx, nop\n",
> +			__func__, data);
>   

We can avoid the printout if data == 0, since we support that case fully.

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

Re: [PATCH] Ignore DEBUGCTL MSRs

[Alexander Graf]

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

Avi Kivity wrote:
> Alexander Graf wrote:
>> Avi Kivity wrote:
>>> Alexander Graf wrote:
>>>> Netware writes and reads to the DEBUGCTL and LAST*IP MSRs without 
>>>> further checks and is really confused to receive a #GP during that. 
>>>> To make it happy we should just make them stubs, which is exactly 
>>>> what SVM already does.
>>>>
>>>> To support VMX too, I put these in the generic code. Maybe the SVM 
>>>> code could be cleaned up to use generic code too.
>>>>
>>>
>>> Please add a pr_unimpl() when bits that cause a real processor to do 
>>> something are set.
>>
>> Like this? I also removed the set handlers for the *IP MSRs, as these 
>> are read only and made it only handle debug bits, no perfmon bits.
>>
>
> With a changelog entry.

ok.

>
>> +        pr_unimpl(vcpu, "%s: MSR_IA32_DEBUGCTLMSR 0x%llx, nop\n",
>> +            __func__, data);
>>   
>
> We can avoid the printout if data == 0, since we support that case fully.

I was thinking a lot about that. Even though we support data == 0, 
usually the kernel log output is useful for people trying to find if 
something is cause a problem. If they see that DEBUGCTL gets set, but 
won't see it getting unset, they'd get confused IMHO.
So the current behavior is on purpose, but if you oppose to that idea, 
please tell me.


---------------

Netware writes to DEBUGCTL and reads from the DEBUGCTL and LAST*IP MSRs 
without further checks and is really confused to receive a #GP during 
that. To make it happy we should just make them stubs, which is exactly 
what SVM already does.

Writes to DEBUGCTL that are vendor-specific are resembled to behave as 
if the virtual CPU does not know them.

Signed-off-by: Alexander Graf <agraf@suse.de>


[-- Attachment #2: kvm-netware.patch --]
[-- Type: text/x-patch, Size: 1051 bytes --]

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fc0721e..10f5e95 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -609,6 +609,15 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 		pr_unimpl(vcpu, "%s: MSR_IA32_MCG_CTL 0x%llx, nop\n",
 			__func__, data);
 		break;
+	case MSR_IA32_DEBUGCTLMSR:
+		if (data & ~(u64)(DEBUGCTLMSR_LBR | DEBUGCTLMSR_BTF)) {
+			/* Values other than LBR and BTF are vendor-specific,
+			   thus reserved and should throw a #GP */
+			return 1;
+		}
+		pr_unimpl(vcpu, "%s: MSR_IA32_DEBUGCTLMSR 0x%llx, nop\n",
+			__func__, data);
+		break;
 	case MSR_IA32_UCODE_REV:
 	case MSR_IA32_UCODE_WRITE:
 		break;
@@ -705,6 +714,11 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 	case MSR_IA32_MC0_MISC+16:
 	case MSR_IA32_UCODE_REV:
 	case MSR_IA32_EBL_CR_POWERON:
+	case MSR_IA32_DEBUGCTLMSR:
+	case MSR_IA32_LASTBRANCHFROMIP:
+	case MSR_IA32_LASTBRANCHTOIP:
+	case MSR_IA32_LASTINTFROMIP:
+	case MSR_IA32_LASTINTTOIP:
 		data = 0;
 		break;
 	case MSR_MTRRcap:

Re: [PATCH] Ignore DEBUGCTL MSRs

[Avi Kivity]

Alexander Graf wrote:
>>
>>> +        pr_unimpl(vcpu, "%s: MSR_IA32_DEBUGCTLMSR 0x%llx, nop\n",
>>> +            __func__, data);
>>>   
>>
>> We can avoid the printout if data == 0, since we support that case 
>> fully.
>
> I was thinking a lot about that. Even though we support data == 0, 
> usually the kernel log output is useful for people trying to find if 
> something is cause a problem. If they see that DEBUGCTL gets set, but 
> won't see it getting unset, they'd get confused IMHO.
> So the current behavior is on purpose, but if you oppose to that idea, 
> please tell me.
>

Once it gets set, you can expect brokenness.  It doesn't matter if it 
gets unset later.  So IMO not printing on data == 0 is best: quiet on 
the cases we support, and loud on cases we don't.  So yes, I'd prefer it 
changed.


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

Re: [PATCH] Ignore DEBUGCTL MSRs

[Alexander Graf]

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

Avi Kivity wrote:
> Alexander Graf wrote:
>>>
>>>> +        pr_unimpl(vcpu, "%s: MSR_IA32_DEBUGCTLMSR 0x%llx, nop\n",
>>>> +            __func__, data);
>>>>   
>>>
>>> We can avoid the printout if data == 0, since we support that case 
>>> fully.
>>
>> I was thinking a lot about that. Even though we support data == 0, 
>> usually the kernel log output is useful for people trying to find if 
>> something is cause a problem. If they see that DEBUGCTL gets set, but 
>> won't see it getting unset, they'd get confused IMHO.
>> So the current behavior is on purpose, but if you oppose to that 
>> idea, please tell me.
>>
>
> Once it gets set, you can expect brokenness.  It doesn't matter if it 
> gets unset later.  So IMO not printing on data == 0 is best: quiet on 
> the cases we support, and loud on cases we don't.  So yes, I'd prefer 
> it changed.
>
>

Corrected patch follows...

---

Netware writes to DEBUGCTL and reads from the DEBUGCTL and LAST*IP MSRs 
without further checks and is really confused to receive a #GP during 
that. To make it happy we should just make them stubs, which is exactly 
what SVM already does.

Writes to DEBUGCTL that are vendor-specific are resembled to behave as 
if the virtual CPU does not know them.

Signed-off-by: Alexander Graf <agraf@suse.de>


[-- Attachment #2: kvm-netware.patch --]
[-- Type: text/x-patch, Size: 1132 bytes --]

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fc0721e..10f5e95 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -609,6 +609,15 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 		pr_unimpl(vcpu, "%s: MSR_IA32_MCG_CTL 0x%llx, nop\n",
 			__func__, data);
 		break;
+	case MSR_IA32_DEBUGCTLMSR:
+		if (!data) {
+			/* We support the non-activated case already */
+			break;
+		} else if (data & ~(DEBUGCTLMSR_LBR | DEBUGCTLMSR_BTF)) {
+			/* Values other than LBR and BTF are vendor-specific,
+			   thus reserved and should throw a #GP */
+			return 1;
+		}
+		pr_unimpl(vcpu, "%s: MSR_IA32_DEBUGCTLMSR 0x%llx, nop\n",
+			__func__, data);
+		break;
 	case MSR_IA32_UCODE_REV:
 	case MSR_IA32_UCODE_WRITE:
 		break;
@@ -705,6 +714,11 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 	case MSR_IA32_MC0_MISC+16:
 	case MSR_IA32_UCODE_REV:
 	case MSR_IA32_EBL_CR_POWERON:
+	case MSR_IA32_DEBUGCTLMSR:
+	case MSR_IA32_LASTBRANCHFROMIP:
+	case MSR_IA32_LASTBRANCHTOIP:
+	case MSR_IA32_LASTINTFROMIP:
+	case MSR_IA32_LASTINTTOIP:
 		data = 0;
 		break;
 	case MSR_MTRRcap:

Re: [PATCH] Ignore DEBUGCTL MSRs

[Avi Kivity]

Alexander Graf wrote:
>
> Corrected patch follows...
>
> ---
>
> Netware writes to DEBUGCTL and reads from the DEBUGCTL and LAST*IP 
> MSRs without further checks and is really confused to receive a #GP 
> during that. To make it happy we should just make them stubs, which is 
> exactly what SVM already does.
>
> Writes to DEBUGCTL that are vendor-specific are resembled to behave as 
> if the virtual CPU does not know them.
>

Applied, thanks.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.