Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling
On 22.04.2010, at 12:33, Joerg Roedel wrote:
The patch introducing nested nmi handling had a bug. The
check does not belong to enable_nmi_window but must be in
nmi_allowed. This patch fixes this.
Signed-off-by: Joerg Roedel joerg.roe...@amd.com
---
arch/x86/kvm/svm.c | 16 +---
1 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index ab78eb8..ec20584 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2771,8 +2771,12 @@ static int svm_nmi_allowed(struct kvm_vcpu *vcpu)
{
struct vcpu_svm *svm = to_svm(vcpu);
struct vmcb *vmcb = svm-vmcb;
- return !(vmcb-control.int_state SVM_INTERRUPT_SHADOW_MASK)
- !(svm-vcpu.arch.hflags HF_NMI_MASK);
+ int ret;
+ ret = !(vmcb-control.int_state SVM_INTERRUPT_SHADOW_MASK)
+ !(svm-vcpu.arch.hflags HF_NMI_MASK);
+ ret = ret gif_set(svm) nested_svm_nmi(svm);
+
+ return ret;
}
static bool svm_get_nmi_mask(struct kvm_vcpu *vcpu)
@@ -2841,11 +2845,9 @@ static void enable_nmi_window(struct kvm_vcpu *vcpu)
* Something prevents NMI from been injected. Single step over possible
* problem (IRET or exception injection or interrupt shadow)
*/
- if (gif_set(svm) nested_svm_nmi(svm)) {
- svm-nmi_singlestep = true;
- svm-vmcb-save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
- update_db_intercept(vcpu);
- }
+ svm-nmi_singlestep = true;
+ svm-vmcb-save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
+ update_db_intercept(vcpu);
So we're always messing with the nested guest state when the host wants to
inject an nmi into the l1 guest? Is that safe?
Alex
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling
On Fri, Apr 23, 2010 at 03:46:07PM +0200, Alexander Graf wrote:
On 22.04.2010, at 12:33, Joerg Roedel wrote:
The patch introducing nested nmi handling had a bug. The
check does not belong to enable_nmi_window but must be in
nmi_allowed. This patch fixes this.
Signed-off-by: Joerg Roedel joerg.roe...@amd.com
---
arch/x86/kvm/svm.c | 16 +---
1 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index ab78eb8..ec20584 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2771,8 +2771,12 @@ static int svm_nmi_allowed(struct kvm_vcpu *vcpu)
{
struct vcpu_svm *svm = to_svm(vcpu);
struct vmcb *vmcb = svm-vmcb;
- return !(vmcb-control.int_state SVM_INTERRUPT_SHADOW_MASK)
- !(svm-vcpu.arch.hflags HF_NMI_MASK);
+ int ret;
+ ret = !(vmcb-control.int_state SVM_INTERRUPT_SHADOW_MASK)
+ !(svm-vcpu.arch.hflags HF_NMI_MASK);
+ ret = ret gif_set(svm) nested_svm_nmi(svm);
+
+ return ret;
}
static bool svm_get_nmi_mask(struct kvm_vcpu *vcpu)
@@ -2841,11 +2845,9 @@ static void enable_nmi_window(struct kvm_vcpu *vcpu)
* Something prevents NMI from been injected. Single step over possible
* problem (IRET or exception injection or interrupt shadow)
*/
- if (gif_set(svm) nested_svm_nmi(svm)) {
- svm-nmi_singlestep = true;
- svm-vmcb-save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
- update_db_intercept(vcpu);
- }
+ svm-nmi_singlestep = true;
+ svm-vmcb-save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
+ update_db_intercept(vcpu);
So we're always messing with the nested guest state when the host
wants to inject an nmi into the l1 guest? Is that safe?
Why not? We can't inject an NMI directly into L2 if the nested
hypervisor intercepts it.
Joerg
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling
On 23.04.2010, at 16:13, Joerg Roedel wrote:
On Fri, Apr 23, 2010 at 03:46:07PM +0200, Alexander Graf wrote:
On 22.04.2010, at 12:33, Joerg Roedel wrote:
The patch introducing nested nmi handling had a bug. The
check does not belong to enable_nmi_window but must be in
nmi_allowed. This patch fixes this.
Signed-off-by: Joerg Roedel joerg.roe...@amd.com
---
arch/x86/kvm/svm.c | 16 +---
1 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index ab78eb8..ec20584 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2771,8 +2771,12 @@ static int svm_nmi_allowed(struct kvm_vcpu *vcpu)
{
struct vcpu_svm *svm = to_svm(vcpu);
struct vmcb *vmcb = svm-vmcb;
- return !(vmcb-control.int_state SVM_INTERRUPT_SHADOW_MASK)
- !(svm-vcpu.arch.hflags HF_NMI_MASK);
+ int ret;
+ ret = !(vmcb-control.int_state SVM_INTERRUPT_SHADOW_MASK)
+ !(svm-vcpu.arch.hflags HF_NMI_MASK);
+ ret = ret gif_set(svm) nested_svm_nmi(svm);
+
+ return ret;
}
static bool svm_get_nmi_mask(struct kvm_vcpu *vcpu)
@@ -2841,11 +2845,9 @@ static void enable_nmi_window(struct kvm_vcpu *vcpu)
* Something prevents NMI from been injected. Single step over possible
* problem (IRET or exception injection or interrupt shadow)
*/
- if (gif_set(svm) nested_svm_nmi(svm)) {
- svm-nmi_singlestep = true;
- svm-vmcb-save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
- update_db_intercept(vcpu);
- }
+ svm-nmi_singlestep = true;
+ svm-vmcb-save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
+ update_db_intercept(vcpu);
So we're always messing with the nested guest state when the host
wants to inject an nmi into the l1 guest? Is that safe?
Why not? We can't inject an NMI directly into L2 if the nested
hypervisor intercepts it.
So where did the code go that does the #vmexit in case the nested hypervisor
does intercept it? It used to be nested_svm_nmi(), right?
Alex
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling
On Fri, Apr 23, 2010 at 04:19:40PM +0200, Alexander Graf wrote:
On 23.04.2010, at 16:13, Joerg Roedel wrote:
On Fri, Apr 23, 2010 at 03:46:07PM +0200, Alexander Graf wrote:
On 22.04.2010, at 12:33, Joerg Roedel wrote:
The patch introducing nested nmi handling had a bug. The
check does not belong to enable_nmi_window but must be in
nmi_allowed. This patch fixes this.
Signed-off-by: Joerg Roedel joerg.roe...@amd.com
---
arch/x86/kvm/svm.c | 16 +---
1 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index ab78eb8..ec20584 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2771,8 +2771,12 @@ static int svm_nmi_allowed(struct kvm_vcpu *vcpu)
{
struct vcpu_svm *svm = to_svm(vcpu);
struct vmcb *vmcb = svm-vmcb;
- return !(vmcb-control.int_state SVM_INTERRUPT_SHADOW_MASK)
- !(svm-vcpu.arch.hflags HF_NMI_MASK);
+ int ret;
+ ret = !(vmcb-control.int_state SVM_INTERRUPT_SHADOW_MASK)
+ !(svm-vcpu.arch.hflags HF_NMI_MASK);
+ ret = ret gif_set(svm) nested_svm_nmi(svm);
+
+ return ret;
}
static bool svm_get_nmi_mask(struct kvm_vcpu *vcpu)
@@ -2841,11 +2845,9 @@ static void enable_nmi_window(struct kvm_vcpu
*vcpu)
* Something prevents NMI from been injected. Single step over possible
* problem (IRET or exception injection or interrupt shadow)
*/
- if (gif_set(svm) nested_svm_nmi(svm)) {
- svm-nmi_singlestep = true;
- svm-vmcb-save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
- update_db_intercept(vcpu);
- }
+ svm-nmi_singlestep = true;
+ svm-vmcb-save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
+ update_db_intercept(vcpu);
So we're always messing with the nested guest state when the host
wants to inject an nmi into the l1 guest? Is that safe?
Why not? We can't inject an NMI directly into L2 if the nested
hypervisor intercepts it.
So where did the code go that does the #vmexit in case the nested
hypervisor does intercept it? It used to be nested_svm_nmi(), right?
No, nested_svm_nmi runs in atomic context where we can't emulate a
vmexit. We set exit_required and emulate the vmexit later.
Joerg
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling
On 23.04.2010, at 16:22, Joerg Roedel wrote:
On Fri, Apr 23, 2010 at 04:19:40PM +0200, Alexander Graf wrote:
On 23.04.2010, at 16:13, Joerg Roedel wrote:
On Fri, Apr 23, 2010 at 03:46:07PM +0200, Alexander Graf wrote:
On 22.04.2010, at 12:33, Joerg Roedel wrote:
The patch introducing nested nmi handling had a bug. The
check does not belong to enable_nmi_window but must be in
nmi_allowed. This patch fixes this.
Signed-off-by: Joerg Roedel joerg.roe...@amd.com
---
arch/x86/kvm/svm.c | 16 +---
1 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index ab78eb8..ec20584 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2771,8 +2771,12 @@ static int svm_nmi_allowed(struct kvm_vcpu *vcpu)
{
struct vcpu_svm *svm = to_svm(vcpu);
struct vmcb *vmcb = svm-vmcb;
- return !(vmcb-control.int_state SVM_INTERRUPT_SHADOW_MASK)
- !(svm-vcpu.arch.hflags HF_NMI_MASK);
+ int ret;
+ ret = !(vmcb-control.int_state SVM_INTERRUPT_SHADOW_MASK)
+ !(svm-vcpu.arch.hflags HF_NMI_MASK);
+ ret = ret gif_set(svm) nested_svm_nmi(svm);
+
+ return ret;
}
static bool svm_get_nmi_mask(struct kvm_vcpu *vcpu)
@@ -2841,11 +2845,9 @@ static void enable_nmi_window(struct kvm_vcpu
*vcpu)
* Something prevents NMI from been injected. Single step over possible
* problem (IRET or exception injection or interrupt shadow)
*/
- if (gif_set(svm) nested_svm_nmi(svm)) {
- svm-nmi_singlestep = true;
- svm-vmcb-save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
- update_db_intercept(vcpu);
- }
+ svm-nmi_singlestep = true;
+ svm-vmcb-save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
+ update_db_intercept(vcpu);
So we're always messing with the nested guest state when the host
wants to inject an nmi into the l1 guest? Is that safe?
Why not? We can't inject an NMI directly into L2 if the nested
hypervisor intercepts it.
So where did the code go that does the #vmexit in case the nested
hypervisor does intercept it? It used to be nested_svm_nmi(), right?
No, nested_svm_nmi runs in atomic context where we can't emulate a
vmexit. We set exit_required and emulate the vmexit later.
So we modify the L2 rflags and then trigger a #vmexit, leaving the l2 state
broken?
Alex
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling
On Fri, Apr 23, 2010 at 04:24:54PM +0200, Alexander Graf wrote: On 23.04.2010, at 16:22, Joerg Roedel wrote: No, nested_svm_nmi runs in atomic context where we can't emulate a vmexit. We set exit_required and emulate the vmexit later. So we modify the L2 rflags and then trigger a #vmexit, leaving the l2 state broken? No, the rflags are changed in enable_nmi_window which isn't called when we run nested and the nested hypervisor intercepts nmi. So it only runs in the !nested case where it can't corrupt L2 state. Joerg -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling
On 23.04.2010, at 16:31, Joerg Roedel wrote: On Fri, Apr 23, 2010 at 04:24:54PM +0200, Alexander Graf wrote: On 23.04.2010, at 16:22, Joerg Roedel wrote: No, nested_svm_nmi runs in atomic context where we can't emulate a vmexit. We set exit_required and emulate the vmexit later. So we modify the L2 rflags and then trigger a #vmexit, leaving the l2 state broken? No, the rflags are changed in enable_nmi_window which isn't called when we run nested and the nested hypervisor intercepts nmi. So it only runs in the !nested case where it can't corrupt L2 state. Last time I checked the code enable_nmi_window was the function triggering the #vmexit, so it should run in that exact scenario. If what you say is true, where do we #vmexit instead then? Alex -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling
On Fri, Apr 23, 2010 at 04:42:52PM +0200, Alexander Graf wrote: On 23.04.2010, at 16:31, Joerg Roedel wrote: On Fri, Apr 23, 2010 at 04:24:54PM +0200, Alexander Graf wrote: On 23.04.2010, at 16:22, Joerg Roedel wrote: No, nested_svm_nmi runs in atomic context where we can't emulate a vmexit. We set exit_required and emulate the vmexit later. So we modify the L2 rflags and then trigger a #vmexit, leaving the l2 state broken? No, the rflags are changed in enable_nmi_window which isn't called when we run nested and the nested hypervisor intercepts nmi. So it only runs in the !nested case where it can't corrupt L2 state. Last time I checked the code enable_nmi_window was the function triggering the #vmexit, Yes, thats the bug which this patch fixes :-) so it should run in that exact scenario. If what you say is true, where do we #vmexit instead then? After setting exit_required we run into svm.c:svm_vcpu_run. There the exit_required flag is checked and if set, the function immediatly returns without doing a vmrun. A few cycles later we run into svm.c:handle_exit() where at the beginning exit_required is checked, and if set the vmexit is emulated. Joerg -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling
On 23.04.2010, at 16:51, Joerg Roedel wrote: On Fri, Apr 23, 2010 at 04:42:52PM +0200, Alexander Graf wrote: On 23.04.2010, at 16:31, Joerg Roedel wrote: On Fri, Apr 23, 2010 at 04:24:54PM +0200, Alexander Graf wrote: On 23.04.2010, at 16:22, Joerg Roedel wrote: No, nested_svm_nmi runs in atomic context where we can't emulate a vmexit. We set exit_required and emulate the vmexit later. So we modify the L2 rflags and then trigger a #vmexit, leaving the l2 state broken? No, the rflags are changed in enable_nmi_window which isn't called when we run nested and the nested hypervisor intercepts nmi. So it only runs in the !nested case where it can't corrupt L2 state. Last time I checked the code enable_nmi_window was the function triggering the #vmexit, Yes, thats the bug which this patch fixes :-) so it should run in that exact scenario. If what you say is true, where do we #vmexit instead then? After setting exit_required we run into svm.c:svm_vcpu_run. There the exit_required flag is checked and if set, the function immediatly returns without doing a vmrun. A few cycles later we run into svm.c:handle_exit() where at the beginning exit_required is checked, and if set the vmexit is emulated. Oh well, I guess I have to trust you on this one :) Alex -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
