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

["Roedel, Joerg" <Joerg.Roedel@amd.com>]

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