[PATCH] kvm/svm: fix DR interception handling on upcoming AMD CPUs

[PATCH] kvm/svm: fix DR interception handling on upcoming AMD CPUs

[Andre Przywara]

Somehow the code line advancing the RIP and checking for exceptions
got dropped between the post on the ML and the commit.
Add it again to let guests boot on upcoming AMD CPUs again.

Reported-by: Joerg Roedel <joerg.roedel@amd.com>
Signed-off-by: Andre Przywara <andre.przywara@amd.com>
---
 arch/x86/kvm/svm.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 25bd1bc..be06e58 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2776,6 +2776,7 @@ static int dr_interception(struct vcpu_svm *svm)
 		if (!err)
 			kvm_register_write(&svm->vcpu, reg, val);
 	}
+	kvm_complete_insn_gp(&svm->vcpu, err);
 
 	return 1;
 }
-- 
1.6.4

Re: [PATCH] kvm/svm: fix DR interception handling on upcoming AMD CPUs

[Roedel, Joerg]

On Tue, Feb 08, 2011 at 07:22:29PM -0500, Andre Przywara wrote:
> Somehow the code line advancing the RIP and checking for exceptions
> got dropped between the post on the ML and the commit.
> Add it again to let guests boot on upcoming AMD CPUs again.
> 
> Reported-by: Joerg Roedel <joerg.roedel@amd.com>
> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
> ---
>  arch/x86/kvm/svm.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 25bd1bc..be06e58 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2776,6 +2776,7 @@ static int dr_interception(struct vcpu_svm *svm)
>  		if (!err)
>  			kvm_register_write(&svm->vcpu, reg, val);
>  	}
> +	kvm_complete_insn_gp(&svm->vcpu, err);

Any exception check takes presedence over the intercept. So a call to
skip_emulated_instruction should be sufficient here.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH] kvm/svm: fix DR interception handling on upcoming AMD CPUs

[Andre Przywara]

Roedel, Joerg wrote:
> On Tue, Feb 08, 2011 at 07:22:29PM -0500, Andre Przywara wrote:
>> Somehow the code line advancing the RIP and checking for exceptions
>> got dropped between the post on the ML and the commit.
>> Add it again to let guests boot on upcoming AMD CPUs again.
>>
>> Reported-by: Joerg Roedel <joerg.roedel@amd.com>
>> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
>> ---
>>  arch/x86/kvm/svm.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 25bd1bc..be06e58 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -2776,6 +2776,7 @@ static int dr_interception(struct vcpu_svm *svm)
>>  		if (!err)
>>  			kvm_register_write(&svm->vcpu, reg, val);
>>  	}
>> +	kvm_complete_insn_gp(&svm->vcpu, err);
> 
> Any exception check takes presedence over the intercept. So a call to
> skip_emulated_instruction should be sufficient here.
You are right, but I think this way the implementation is cleaner and 
more generic. If there is no exception condition detected (err==0), then 
the call will just do skip_emulated_instruction anyway.
But as I write this, I see that err is not initialized, so I'd propose 
this corrected version of the patch instead.

Regards,
Andre.

--

 From 8c1dced7b4f80e4968b03656aee2452535b2ef2f Mon Sep 17 00:00:00 2001
From: Andre Przywara <andre.przywara@amd.com>
Date: Wed, 9 Feb 2011 01:10:45 +0100
Subject: [PATCH] kvm/svm: fix DR interception handling on upcoming AMD CPUs

Somehow the code line advancing the RIP and checking for exceptions
got dropped between the mail on the ML and the commit.
Add it again to let guests boot on upcoming AMD CPUs again.
While at it, properly initialize the err variable to fix the write DR
case.

Reported-by: Joerg Roedel <joerg.roedel@amd.com>
Signed-off-by: Andre Przywara <andre.przywara@amd.com>
---
  arch/x86/kvm/svm.c |    3 ++-
  1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 25bd1bc..d558121 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2760,7 +2760,7 @@ static int dr_interception(struct vcpu_svm *svm)
  {
  	int reg, dr;
  	unsigned long val;
-	int err;
+	int err = 0;

  	if (!boot_cpu_has(X86_FEATURE_DECODEASSISTS))
  		return emulate_on_interception(svm);
@@ -2776,6 +2776,7 @@ static int dr_interception(struct vcpu_svm *svm)
  		if (!err)
  			kvm_register_write(&svm->vcpu, reg, val);
  	}
+	kvm_complete_insn_gp(&svm->vcpu, err);

  	return 1;
  }
-- 
1.6.4

Re: [PATCH] kvm/svm: fix DR interception handling on upcoming AMD CPUs

[Roedel, Joerg]

On Wed, Feb 09, 2011 at 05:43:28AM -0500, Andre Przywara wrote:
> Roedel, Joerg wrote:
> > On Tue, Feb 08, 2011 at 07:22:29PM -0500, Andre Przywara wrote:
> >> Somehow the code line advancing the RIP and checking for exceptions
> >> got dropped between the post on the ML and the commit.
> >> Add it again to let guests boot on upcoming AMD CPUs again.
> >>
> >> Reported-by: Joerg Roedel <joerg.roedel@amd.com>
> >> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
> >> ---
> >>  arch/x86/kvm/svm.c |    1 +
> >>  1 files changed, 1 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> >> index 25bd1bc..be06e58 100644
> >> --- a/arch/x86/kvm/svm.c
> >> +++ b/arch/x86/kvm/svm.c
> >> @@ -2776,6 +2776,7 @@ static int dr_interception(struct vcpu_svm *svm)
> >>  		if (!err)
> >>  			kvm_register_write(&svm->vcpu, reg, val);
> >>  	}
> >> +	kvm_complete_insn_gp(&svm->vcpu, err);
> > 
> > Any exception check takes presedence over the intercept. So a call to
> > skip_emulated_instruction should be sufficient here.

> You are right, but I think this way the implementation is cleaner and 
> more generic. If there is no exception condition detected (err==0), then 
> the call will just do skip_emulated_instruction anyway.
> But as I write this, I see that err is not initialized, so I'd propose 
> this corrected version of the patch instead.

One function call is not less clean than another and this function call
has more overhead.
Beside that, looking at kvm_set_dr this function only returns 1 if one
tries to set dr4 or dr5 while CR4.DE is not set. In this case the
function you call here would inject a #GP but architecturally a #UD
is required.
This problem doesn't matter too much because the exceptions take
precedence over the intercept anyway so it is sufficient to call
skip_emulated_instruction here.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH] kvm/svm: fix DR interception handling on upcoming AMD CPUs

[Avi Kivity]

On 02/09/2011 02:56 PM, Roedel, Joerg wrote:
> On Wed, Feb 09, 2011 at 05:43:28AM -0500, Andre Przywara wrote:
> >  Roedel, Joerg wrote:
> >  >  On Tue, Feb 08, 2011 at 07:22:29PM -0500, Andre Przywara wrote:
> >  >>  Somehow the code line advancing the RIP and checking for exceptions
> >  >>  got dropped between the post on the ML and the commit.
> >  >>  Add it again to let guests boot on upcoming AMD CPUs again.
> >  >>
> >  >>  Reported-by: Joerg Roedel<joerg.roedel@amd.com>
> >  >>  Signed-off-by: Andre Przywara<andre.przywara@amd.com>
> >  >>  ---
> >  >>   arch/x86/kvm/svm.c |    1 +
> >  >>   1 files changed, 1 insertions(+), 0 deletions(-)
> >  >>
> >  >>  diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> >  >>  index 25bd1bc..be06e58 100644
> >  >>  --- a/arch/x86/kvm/svm.c
> >  >>  +++ b/arch/x86/kvm/svm.c
> >  >>  @@ -2776,6 +2776,7 @@ static int dr_interception(struct vcpu_svm *svm)
> >  >>   		if (!err)
> >  >>   			kvm_register_write(&svm->vcpu, reg, val);
> >  >>   	}
> >  >>  +	kvm_complete_insn_gp(&svm->vcpu, err);
> >  >
> >  >  Any exception check takes presedence over the intercept. So a call to
> >  >  skip_emulated_instruction should be sufficient here.
>
> >  You are right, but I think this way the implementation is cleaner and
> >  more generic. If there is no exception condition detected (err==0), then
> >  the call will just do skip_emulated_instruction anyway.
> >  But as I write this, I see that err is not initialized, so I'd propose
> >  this corrected version of the patch instead.
>
> One function call is not less clean than another and this function call
> has more overhead.
> Beside that, looking at kvm_set_dr this function only returns 1 if one
> tries to set dr4 or dr5 while CR4.DE is not set. In this case the
> function you call here would inject a #GP but architecturally a #UD
> is required.
> This problem doesn't matter too much because the exceptions take
> precedence over the intercept anyway so it is sufficient to call
> skip_emulated_instruction here.

Given that the exception is incorrect I agree 
skip_emulated_instruction() is a better choice here.

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