[PATCH 0/2] perf-kvm support for SVM

[PATCH 0/2] perf-kvm support for SVM

[Joerg Roedel]

Hi Avi, Marcelo,

these two patches finally implement perf-kvm support for AMD machines.
The meat is in the second patch. The first one is an important fix
which, when missing, causes system crashes when NMI happen while in
guest mode. So the first patch should also make it to the various
stable-queues.

Regards,

	Joerg

[PATCH 1/2] KVM: SVM: Fix NMI path when NMI happens in guest mode

[Joerg Roedel]

The vmexit path on SVM needs to restore the KERNEL_GS_BASE
MSR in order to savely execute the NMI handler. Otherwise a
pending NMI can occur after the STGI instruction and crash
the machine.
This makes it impossible to run perf and kvm in parallel on
an AMD machine in a stable way.

Cc: stable@kernel.org
Signed-off-by: Joerg Roedel <joerg.roedel@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..8b9bc72 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3637,6 +3637,7 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
 
 #ifdef CONFIG_X86_64
 	wrmsrl(MSR_GS_BASE, svm->host.gs_base);
+	wrmsrl(MSR_KERNEL_GS_BASE, current->thread.gs);
 #else
 	loadsegment(fs, svm->host.fs);
 #endif
-- 
1.7.1

[PATCH 2/2] KVM: SVM: Add support for perf-kvm

[Joerg Roedel]

This patch adds the necessary code to run perf-kvm on AMD
machines.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 8b9bc72..2415129 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3646,13 +3646,21 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
 
 	local_irq_disable();
 
-	stgi();
-
 	vcpu->arch.cr2 = svm->vmcb->save.cr2;
 	vcpu->arch.regs[VCPU_REGS_RAX] = svm->vmcb->save.rax;
 	vcpu->arch.regs[VCPU_REGS_RSP] = svm->vmcb->save.rsp;
 	vcpu->arch.regs[VCPU_REGS_RIP] = svm->vmcb->save.rip;
 
+	if (unlikely(svm->vmcb->control.exit_code == SVM_EXIT_NMI))
+		kvm_before_handle_nmi(&svm->vcpu);
+
+	stgi();
+
+	/* Any pending NMI will happen here */
+
+	if (unlikely(svm->vmcb->control.exit_code == SVM_EXIT_NMI))
+		kvm_after_handle_nmi(&svm->vcpu);
+
 	sync_cr8_to_lapic(vcpu);
 
 	svm->next_rip = 0;
-- 
1.7.1

Re: [PATCH 1/2] KVM: SVM: Fix NMI path when NMI happens in guest mode

[Avi Kivity]

On 01/13/2011 05:22 PM, Joerg Roedel wrote:
> The vmexit path on SVM needs to restore the KERNEL_GS_BASE
> MSR in order to savely execute the NMI handler. Otherwise a
> pending NMI can occur after the STGI instruction and crash
> the machine.
> This makes it impossible to run perf and kvm in parallel on
> an AMD machine in a stable way.
>
> Cc: stable@kernel.org
> Signed-off-by: Joerg Roedel<joerg.roedel@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..8b9bc72 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -3637,6 +3637,7 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
>
>   #ifdef CONFIG_X86_64
>   	wrmsrl(MSR_GS_BASE, svm->host.gs_base);
> +	wrmsrl(MSR_KERNEL_GS_BASE, current->thread.gs);
>   #else
>   	loadsegment(fs, svm->host.fs);
>   #endif

Why would an NMI crash if MSR_KERNEL_GS_BASE is bad?

I see save_paranoid depends on MSR_GS_BASE (specifically its sign, which 
is bad for the new instructions that allow userspace to write gsbase), 
but not on MSR_KERNEL_GS_BASE.

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

Re: [PATCH 1/2] KVM: SVM: Fix NMI path when NMI happens in guest mode

[Jan Kiszka]

Am 13.01.2011 16:22, Joerg Roedel wrote:
> The vmexit path on SVM needs to restore the KERNEL_GS_BASE
> MSR in order to savely execute the NMI handler. Otherwise a
> pending NMI can occur after the STGI instruction and crash
> the machine.
> This makes it impossible to run perf and kvm in parallel on
> an AMD machine in a stable way.
> 
> Cc: stable@kernel.org
> Signed-off-by: Joerg Roedel <joerg.roedel@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..8b9bc72 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -3637,6 +3637,7 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
>  
>  #ifdef CONFIG_X86_64
>  	wrmsrl(MSR_GS_BASE, svm->host.gs_base);
> +	wrmsrl(MSR_KERNEL_GS_BASE, current->thread.gs);
>  #else
>  	loadsegment(fs, svm->host.fs);
>  #endif

Doesn't this also obsolete the wrmsrl(MSR_KERNEL_GS_BASE) in svm_vcpu_put?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/2] KVM: SVM: Fix NMI path when NMI happens in guest mode

[Roedel, Joerg]

On Thu, Jan 13, 2011 at 10:42:01AM -0500, Avi Kivity wrote:
> On 01/13/2011 05:22 PM, Joerg Roedel wrote:
> > The vmexit path on SVM needs to restore the KERNEL_GS_BASE
> > MSR in order to savely execute the NMI handler. Otherwise a
> > pending NMI can occur after the STGI instruction and crash
> > the machine.
> > This makes it impossible to run perf and kvm in parallel on
> > an AMD machine in a stable way.
> >
> > Cc: stable@kernel.org
> > Signed-off-by: Joerg Roedel<joerg.roedel@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..8b9bc72 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -3637,6 +3637,7 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
> >
> >   #ifdef CONFIG_X86_64
> >   	wrmsrl(MSR_GS_BASE, svm->host.gs_base);
> > +	wrmsrl(MSR_KERNEL_GS_BASE, current->thread.gs);
> >   #else
> >   	loadsegment(fs, svm->host.fs);
> >   #endif
> 
> Why would an NMI crash if MSR_KERNEL_GS_BASE is bad?
> 
> I see save_paranoid depends on MSR_GS_BASE (specifically its sign, which 
> is bad for the new instructions that allow userspace to write gsbase), 
> but not on MSR_KERNEL_GS_BASE.

Thats a good question. I have not idea. I spent some time trying to
figure this out (after I found out that wrong KERNEL_GS_BASE was the
cause of the crashes) but had no luck.

This also doesn't happen every time an NMI is delivered in svm_vcpu_run.
Sometimes it runs perfectly in parallel for a few minutues before the
machine triple-faults.

I also had a look at entry_64.S. The save_paranoid could not be the
cause because MSR_GS_BASE is already negative at this point. But the
re-schedule condition check at the end of the NMI handler code could
also not be the cause because the NMI happens while preemption (and
interrupts) are disabled (a re-schedule should also trigger
preempt-notifiers and restore KERNEL_GS_BASE).

	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 1/2] KVM: SVM: Fix NMI path when NMI happens in guest mode

[Roedel, Joerg]

On Thu, Jan 13, 2011 at 10:48:33AM -0500, Jan Kiszka wrote:
> Am 13.01.2011 16:22, Joerg Roedel wrote:
> >  #ifdef CONFIG_X86_64
> >  	wrmsrl(MSR_GS_BASE, svm->host.gs_base);
> > +	wrmsrl(MSR_KERNEL_GS_BASE, current->thread.gs);
> >  #else
> >  	loadsegment(fs, svm->host.fs);
> >  #endif
> 
> Doesn't this also obsolete the wrmsrl(MSR_KERNEL_GS_BASE) in svm_vcpu_put?

Yes it does. Thanks.

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 1/2] KVM: SVM: Fix NMI path when NMI happens in guest mode

[Avi Kivity]

On 01/13/2011 05:51 PM, Roedel, Joerg wrote:
> On Thu, Jan 13, 2011 at 10:42:01AM -0500, Avi Kivity wrote:
> >  On 01/13/2011 05:22 PM, Joerg Roedel wrote:
> >  >  The vmexit path on SVM needs to restore the KERNEL_GS_BASE
> >  >  MSR in order to savely execute the NMI handler. Otherwise a
> >  >  pending NMI can occur after the STGI instruction and crash
> >  >  the machine.
> >  >  This makes it impossible to run perf and kvm in parallel on
> >  >  an AMD machine in a stable way.
> >  >
> >  >  Cc: stable@kernel.org
> >  >  Signed-off-by: Joerg Roedel<joerg.roedel@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..8b9bc72 100644
> >  >  --- a/arch/x86/kvm/svm.c
> >  >  +++ b/arch/x86/kvm/svm.c
> >  >  @@ -3637,6 +3637,7 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
> >  >
> >  >    #ifdef CONFIG_X86_64
> >  >    	wrmsrl(MSR_GS_BASE, svm->host.gs_base);
> >  >  +	wrmsrl(MSR_KERNEL_GS_BASE, current->thread.gs);
> >  >    #else
> >  >    	loadsegment(fs, svm->host.fs);
> >  >    #endif
> >
> >  Why would an NMI crash if MSR_KERNEL_GS_BASE is bad?
> >
> >  I see save_paranoid depends on MSR_GS_BASE (specifically its sign, which
> >  is bad for the new instructions that allow userspace to write gsbase),
> >  but not on MSR_KERNEL_GS_BASE.
>
> Thats a good question. I have not idea. I spent some time trying to
> figure this out (after I found out that wrong KERNEL_GS_BASE was the
> cause of the crashes) but had no luck.
>
> This also doesn't happen every time an NMI is delivered in svm_vcpu_run.
> Sometimes it runs perfectly in parallel for a few minutues before the
> machine triple-faults.
>
> I also had a look at entry_64.S. The save_paranoid could not be the
> cause because MSR_GS_BASE is already negative at this point. But the
> re-schedule condition check at the end of the NMI handler code could
> also not be the cause because the NMI happens while preemption (and
> interrupts) are disabled (a re-schedule should also trigger
> preempt-notifiers and restore KERNEL_GS_BASE).
>

I have it:

ENTRY(native_load_gs_index)
     CFI_STARTPROC
     pushfq_cfi
     DISABLE_INTERRUPTS(CLBR_ANY & ~CLBR_RDI)
     SWAPGS
gs_change:
     movl %edi,%gs
2:    mfence        /* workaround */
     SWAPGS
     popfq_cfi
     ret

If an nmi hits between the two SWAPGSs, it sees the guest's 
MSR_KERNEL_GS_BASE as the host's MSR_GS_BASE.

An alternative to your fix would be to disable GIF around 
load_gs_index() in kvm.  I imagine it would be slower than your fix (not 
a trivial tradeoff - wrmsr every lightweight exit, vs. clgi/stgi every 
heavyweight exit).

Please update the changelog, and add a comment.

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

Re: [PATCH 1/2] KVM: SVM: Fix NMI path when NMI happens in guest mode

[Roedel, Joerg]

On Thu, Jan 13, 2011 at 02:27:00PM -0500, Avi Kivity wrote:
> On 01/13/2011 05:51 PM, Roedel, Joerg wrote:
> > I also had a look at entry_64.S. The save_paranoid could not be the
> > cause because MSR_GS_BASE is already negative at this point. But the
> > re-schedule condition check at the end of the NMI handler code could
> > also not be the cause because the NMI happens while preemption (and
> > interrupts) are disabled (a re-schedule should also trigger
> > preempt-notifiers and restore KERNEL_GS_BASE).
> >
> 
> I have it:

Cool, good catch. Thanks :)

The only use of load_gs_index in svm is the vcpu_put function. It is
sufficient to just swap the KERNEL_GS_BASE wrmsr and the load_gs_index
function calls in there to be safe.

	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