[PATCH 0/8] More fixes for nested svm

[PATCH 0/8] More fixes for nested svm

[Joerg Roedel]

Hi Avi, Marcelo,

here is another set of nested svm fixes. They fix NMI code to make UP Hyper-V
root domain booting. The patches also add better handling for nested entry
failures and mce intercepts.
Also in this patchset are the fixes for the supported cpuid reporting for svm
features. These patches were taken from the nested-npt patchset and slightly
modified. These patches are also marked for -stable backporting.
The probably most important fix is about exception reinjection. This didn't
work reliably before and is fixed with the patch in this series now. This fix
also touches common x86 code but that should be ok because it could be reused
by nested-vmx later.
Please review and give comments (or apply ;-).

Thanks,

	Joerg

Diffstat:

 arch/x86/include/asm/kvm_host.h |    8 ++++-
 arch/x86/kvm/svm.c              |   72 +++++++++++++++++++++++++--------------
 arch/x86/kvm/vmx.c              |    9 ++++-
 arch/x86/kvm/x86.c              |   26 ++++++++++++--
 4 files changed, 83 insertions(+), 32 deletions(-)

Shortlog:

Joerg Roedel (8):
      KVM: SVM: Fix nested nmi handling
      KVM: SVM: Make sure rip is synced to vmcb before nested vmexit
      KVM: SVM: Sync cr0 and cr3 to kvm state before nested handling
      KVM: SVM: Propagate nested entry failure into guest hypervisor
      KVM: X86: Add callback to let modules decide over some supported cpuid bits
      KVM: SVM: Report emulated SVM features to userspace
      KVM: x86: Allow marking an exception as reinjected
      KVM: SVM: Handle MCE intercepts always on host level

[PATCH 1/8] KVM: SVM: Fix nested nmi handling

[Joerg Roedel]

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.roedel@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);
 }
 
 static int svm_set_tss_addr(struct kvm *kvm, unsigned int addr)
-- 
1.7.0.4

[PATCH 2/8] KVM: SVM: Make sure rip is synced to vmcb before nested vmexit

[Joerg Roedel]

This patch fixes a bug where a nested guest always went over
the same instruction because the rip was not advanced on a
nested vmexit.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index ec20584..c480d7f 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2960,6 +2960,10 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
 	u16 gs_selector;
 	u16 ldt_selector;
 
+	svm->vmcb->save.rax = vcpu->arch.regs[VCPU_REGS_RAX];
+	svm->vmcb->save.rsp = vcpu->arch.regs[VCPU_REGS_RSP];
+	svm->vmcb->save.rip = vcpu->arch.regs[VCPU_REGS_RIP];
+
 	/*
 	 * A vmexit emulation is required before the vcpu can be executed
 	 * again.
@@ -2967,10 +2971,6 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
 	if (unlikely(svm->nested.exit_required))
 		return;
 
-	svm->vmcb->save.rax = vcpu->arch.regs[VCPU_REGS_RAX];
-	svm->vmcb->save.rsp = vcpu->arch.regs[VCPU_REGS_RSP];
-	svm->vmcb->save.rip = vcpu->arch.regs[VCPU_REGS_RIP];
-
 	pre_svm_run(svm);
 
 	sync_lapic_to_cr8(vcpu);
-- 
1.7.0.4

[PATCH 3/8] KVM: SVM: Sync cr0 and cr3 to kvm state before nested handling

[Joerg Roedel]

This patch syncs cr0 and cr3 from the vmcb to the kvm state
before nested intercept handling is done. This allows to
simplify the vmexit path.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index c480d7f..5ad9d80 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1799,10 +1799,7 @@ static int nested_svm_vmexit(struct vcpu_svm *svm)
 	nested_vmcb->save.gdtr   = vmcb->save.gdtr;
 	nested_vmcb->save.idtr   = vmcb->save.idtr;
 	nested_vmcb->save.cr0    = kvm_read_cr0(&svm->vcpu);
-	if (npt_enabled)
-		nested_vmcb->save.cr3    = vmcb->save.cr3;
-	else
-		nested_vmcb->save.cr3    = svm->vcpu.arch.cr3;
+	nested_vmcb->save.cr3    = svm->vcpu.arch.cr3;
 	nested_vmcb->save.cr2    = vmcb->save.cr2;
 	nested_vmcb->save.cr4    = svm->vcpu.arch.cr4;
 	nested_vmcb->save.rflags = vmcb->save.rflags;
@@ -2641,6 +2638,11 @@ static int handle_exit(struct kvm_vcpu *vcpu)
 
 	trace_kvm_exit(exit_code, vcpu);
 
+	if (!(svm->vmcb->control.intercept_cr_write & INTERCEPT_CR0_MASK))
+		vcpu->arch.cr0 = svm->vmcb->save.cr0;
+	if (npt_enabled)
+		vcpu->arch.cr3 = svm->vmcb->save.cr3;
+
 	if (unlikely(svm->nested.exit_required)) {
 		nested_svm_vmexit(svm);
 		svm->nested.exit_required = false;
@@ -2668,11 +2670,6 @@ static int handle_exit(struct kvm_vcpu *vcpu)
 
 	svm_complete_interrupts(svm);
 
-	if (!(svm->vmcb->control.intercept_cr_write & INTERCEPT_CR0_MASK))
-		vcpu->arch.cr0 = svm->vmcb->save.cr0;
-	if (npt_enabled)
-		vcpu->arch.cr3 = svm->vmcb->save.cr3;
-
 	if (svm->vmcb->control.exit_code == SVM_EXIT_ERR) {
 		kvm_run->exit_reason = KVM_EXIT_FAIL_ENTRY;
 		kvm_run->fail_entry.hardware_entry_failure_reason
-- 
1.7.0.4

[PATCH 4/8] KVM: SVM: Propagate nested entry failure into guest hypervisor

[Joerg Roedel]

This patch implements propagation of a failes guest vmrun
back into the guest instead of killing the whole guest.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 5ad9d80..b10d163 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1715,6 +1715,10 @@ static int nested_svm_intercept(struct vcpu_svm *svm)
 			vmexit = NESTED_EXIT_DONE;
 		break;
 	}
+	case SVM_EXIT_ERR: {
+		vmexit = NESTED_EXIT_DONE;
+		break;
+	}
 	default: {
 		u64 exit_bits = 1ULL << (exit_code - SVM_EXIT_INTR);
 		if (svm->nested.intercept & exit_bits)
-- 
1.7.0.4

[PATCH 5/8] KVM: X86: Add callback to let modules decide over some supported cpuid bits

[Joerg Roedel]

This patch adds the get_supported_cpuid callback to
kvm_x86_ops. It will be used in do_cpuid_ent to delegate the
decission about some supported cpuid bits to the
architecture modules.

Cc: stable@kernel.org
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/kvm_host.h |    2 ++
 arch/x86/kvm/svm.c              |    6 ++++++
 arch/x86/kvm/vmx.c              |    6 ++++++
 arch/x86/kvm/x86.c              |    3 +++
 4 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index d47d087..357573a 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -528,6 +528,8 @@ struct kvm_x86_ops {
 	int (*get_lpage_level)(void);
 	bool (*rdtscp_supported)(void);
 
+	void (*set_supported_cpuid)(u32 func, struct kvm_cpuid_entry2 *entry);
+
 	const struct trace_print_flags *exit_reasons_str;
 };
 
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index b10d163..0fa2035 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3152,6 +3152,10 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu)
 {
 }
 
+static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
+{
+}
+
 static const struct trace_print_flags svm_exit_reasons_str[] = {
 	{ SVM_EXIT_READ_CR0,			"read_cr0" },
 	{ SVM_EXIT_READ_CR3,			"read_cr3" },
@@ -3297,6 +3301,8 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.cpuid_update = svm_cpuid_update,
 
 	.rdtscp_supported = svm_rdtscp_supported,
+
+	.set_supported_cpuid = svm_set_supported_cpuid,
 };
 
 static int __init svm_init(void)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index d0a10b5..dc023bc 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4111,6 +4111,10 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
 	}
 }
 
+static void vmx_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
+{
+}
+
 static struct kvm_x86_ops vmx_x86_ops = {
 	.cpu_has_kvm_support = cpu_has_kvm_support,
 	.disabled_by_bios = vmx_disabled_by_bios,
@@ -4183,6 +4187,8 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.cpuid_update = vmx_cpuid_update,
 
 	.rdtscp_supported = vmx_rdtscp_supported,
+
+	.set_supported_cpuid = vmx_set_supported_cpuid,
 };
 
 static int __init vmx_init(void)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cf37ac6..c2c3c29 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1960,6 +1960,9 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		entry->ecx &= kvm_supported_word6_x86_features;
 		break;
 	}
+
+	kvm_x86_ops->set_supported_cpuid(function, entry);
+
 	put_cpu();
 }
 
-- 
1.7.0.4

[PATCH 6/8] KVM: SVM: Report emulated SVM features to userspace

[Joerg Roedel]

This patch implements the reporting of the emulated SVM
features to userspace instead of the real hardware
capabilities. Every real hardware capability needs emulation
in nested svm so the old behavior was broken.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 0fa2035..65fc114 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3154,6 +3154,16 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu)
 
 static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
 {
+	switch (func) {
+	case 0x8000000A:
+		entry->eax = 1; /* SVM revision 1 */
+		entry->ebx = 8; /* Lets support 8 ASIDs in case we add proper
+				   ASID emulation to nested SVM */
+		entry->ecx = 0; /* Reserved */
+		entry->edx = 0; /* Do not support any additional features */
+
+		break;
+	}
 }
 
 static const struct trace_print_flags svm_exit_reasons_str[] = {
-- 
1.7.0.4

[PATCH 7/8] KVM: x86: Allow marking an exception as reinjected

[Joerg Roedel]

This patch adds logic to kvm/x86 which allows to mark an
injected exception as reinjected. This allows to remove an
ugly hack from svm_complete_interrupts that prevented
exceptions from being reinjected at all in the nested case.
The hack was necessary because an reinjected exception into
the nested guest could cause a nested vmexit emulation. But
reinjected exceptions must not intercept. The downside of
the hack is that a exception that in injected could get
lost.
This patch fixes the problem and puts the code for it into
generic x86 files because. Nested-VMX will likely have the
same problem and could reuse the code.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/kvm_host.h |    6 +++++-
 arch/x86/kvm/svm.c              |   12 ++++++------
 arch/x86/kvm/vmx.c              |    3 ++-
 arch/x86/kvm/x86.c              |   23 +++++++++++++++++++----
 4 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 357573a..3f0007b 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -312,6 +312,7 @@ struct kvm_vcpu_arch {
 	struct kvm_queued_exception {
 		bool pending;
 		bool has_error_code;
+		bool reinject;
 		u8 nr;
 		u32 error_code;
 	} exception;
@@ -514,7 +515,8 @@ struct kvm_x86_ops {
 	void (*set_irq)(struct kvm_vcpu *vcpu);
 	void (*set_nmi)(struct kvm_vcpu *vcpu);
 	void (*queue_exception)(struct kvm_vcpu *vcpu, unsigned nr,
-				bool has_error_code, u32 error_code);
+				bool has_error_code, u32 error_code,
+				bool reinject);
 	int (*interrupt_allowed)(struct kvm_vcpu *vcpu);
 	int (*nmi_allowed)(struct kvm_vcpu *vcpu);
 	bool (*get_nmi_mask)(struct kvm_vcpu *vcpu);
@@ -617,6 +619,8 @@ void kvm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags);
 
 void kvm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr);
 void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code);
+void kvm_requeue_exception(struct kvm_vcpu *vcpu, unsigned nr);
+void kvm_requeue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code);
 void kvm_inject_page_fault(struct kvm_vcpu *vcpu, unsigned long cr2,
 			   u32 error_code);
 bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 65fc114..30e49fe 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -338,7 +338,8 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 }
 
 static void svm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
-				bool has_error_code, u32 error_code)
+				bool has_error_code, u32 error_code,
+				bool reinject)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
@@ -346,7 +347,8 @@ static void svm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
 	 * If we are within a nested VM we'd better #VMEXIT and let the guest
 	 * handle the exception
 	 */
-	if (nested_svm_check_exception(svm, nr, has_error_code, error_code))
+	if (!reinject &&
+	    nested_svm_check_exception(svm, nr, has_error_code, error_code))
 		return;
 
 	if (nr == BP_VECTOR && !svm_has(SVM_FEATURE_NRIP)) {
@@ -2918,8 +2920,6 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
 		svm->vcpu.arch.nmi_injected = true;
 		break;
 	case SVM_EXITINTINFO_TYPE_EXEPT:
-		if (is_nested(svm))
-			break;
 		/*
 		 * In case of software exceptions, do not reinject the vector,
 		 * but re-execute the instruction instead. Rewind RIP first
@@ -2935,10 +2935,10 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
 		}
 		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
 			u32 err = svm->vmcb->control.exit_int_info_err;
-			kvm_queue_exception_e(&svm->vcpu, vector, err);
+			kvm_requeue_exception_e(&svm->vcpu, vector, err);
 
 		} else
-			kvm_queue_exception(&svm->vcpu, vector);
+			kvm_requeue_exception(&svm->vcpu, vector);
 		break;
 	case SVM_EXITINTINFO_TYPE_INTR:
 		kvm_queue_interrupt(&svm->vcpu, vector, false);
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index dc023bc..285fe1a 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -919,7 +919,8 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 }
 
 static void vmx_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
-				bool has_error_code, u32 error_code)
+				bool has_error_code, u32 error_code,
+				bool reinject)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	u32 intr_info = nr | INTR_INFO_VALID_MASK;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c2c3c29..9c183cd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -265,7 +265,8 @@ static int exception_class(int vector)
 }
 
 static void kvm_multiple_exception(struct kvm_vcpu *vcpu,
-		unsigned nr, bool has_error, u32 error_code)
+		unsigned nr, bool has_error, u32 error_code,
+		bool reinject)
 {
 	u32 prev_nr;
 	int class1, class2;
@@ -276,6 +277,7 @@ static void kvm_multiple_exception(struct kvm_vcpu *vcpu,
 		vcpu->arch.exception.has_error_code = has_error;
 		vcpu->arch.exception.nr = nr;
 		vcpu->arch.exception.error_code = error_code;
+		vcpu->arch.exception.reinject = true;
 		return;
 	}
 
@@ -304,10 +306,16 @@ static void kvm_multiple_exception(struct kvm_vcpu *vcpu,
 
 void kvm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr)
 {
-	kvm_multiple_exception(vcpu, nr, false, 0);
+	kvm_multiple_exception(vcpu, nr, false, 0, false);
 }
 EXPORT_SYMBOL_GPL(kvm_queue_exception);
 
+void kvm_requeue_exception(struct kvm_vcpu *vcpu, unsigned nr)
+{
+	kvm_multiple_exception(vcpu, nr, false, 0, true);
+}
+EXPORT_SYMBOL_GPL(kvm_requeue_exception);
+
 void kvm_inject_page_fault(struct kvm_vcpu *vcpu, unsigned long addr,
 			   u32 error_code)
 {
@@ -324,10 +332,16 @@ EXPORT_SYMBOL_GPL(kvm_inject_nmi);
 
 void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code)
 {
-	kvm_multiple_exception(vcpu, nr, true, error_code);
+	kvm_multiple_exception(vcpu, nr, true, error_code, false);
 }
 EXPORT_SYMBOL_GPL(kvm_queue_exception_e);
 
+void kvm_requeue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code)
+{
+	kvm_multiple_exception(vcpu, nr, true, error_code, true);
+}
+EXPORT_SYMBOL_GPL(kvm_requeue_exception_e);
+
 /*
  * Checks if cpl <= required_cpl; if true, return true.  Otherwise queue
  * a #GP and return false.
@@ -4404,7 +4418,8 @@ static void inject_pending_event(struct kvm_vcpu *vcpu)
 					vcpu->arch.exception.error_code);
 		kvm_x86_ops->queue_exception(vcpu, vcpu->arch.exception.nr,
 					  vcpu->arch.exception.has_error_code,
-					  vcpu->arch.exception.error_code);
+					  vcpu->arch.exception.error_code,
+					  vcpu->arch.exception.reinject);
 		return;
 	}
 
-- 
1.7.0.4

[PATCH 8/8] KVM: SVM: Handle MCE intercepts always on host level

[Joerg Roedel]

This patch prevents MCE intercepts from being propagated
into the L1 guest if they happened in an L2 guest.

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 30e49fe..889f660 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1651,6 +1651,7 @@ static int nested_svm_exit_special(struct vcpu_svm *svm)
 	switch (exit_code) {
 	case SVM_EXIT_INTR:
 	case SVM_EXIT_NMI:
+	case SVM_EXIT_EXCP_BASE + MC_VECTOR:
 		return NESTED_EXIT_HOST;
 	case SVM_EXIT_NPF:
 		/* For now we are always handling NPFs when using them */
-- 
1.7.0.4

Re: [PATCH 0/8] More fixes for nested svm

[Avi Kivity]

On 04/22/2010 01:33 PM, Joerg Roedel wrote:
> Hi Avi, Marcelo,
>
> here is another set of nested svm fixes. They fix NMI code to make UP Hyper-V
> root domain booting. The patches also add better handling for nested entry
> failures and mce intercepts.
> Also in this patchset are the fixes for the supported cpuid reporting for svm
> features. These patches were taken from the nested-npt patchset and slightly
> modified. These patches are also marked for -stable backporting.
> The probably most important fix is about exception reinjection. This didn't
> work reliably before and is fixed with the patch in this series now. This fix
> also touches common x86 code but that should be ok because it could be reused
> by nested-vmx later.
> Please review and give comments (or apply ;-).
>    

Looks good.  I'll wait a day or two for more reviews (esp. Alex).

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

Re: [PATCH 0/8] More fixes for nested svm

[Alexander Graf]

Hey Joerg,

On 22.04.2010, at 12:33, Joerg Roedel wrote:

> Hi Avi, Marcelo,
> 
> here is another set of nested svm fixes. They fix NMI code to make UP Hyper-V
> root domain booting. The patches also add better handling for nested entry
> failures and mce intercepts.
> Also in this patchset are the fixes for the supported cpuid reporting for svm
> features. These patches were taken from the nested-npt patchset and slightly
> modified. These patches are also marked for -stable backporting.
> The probably most important fix is about exception reinjection. This didn't
> work reliably before and is fixed with the patch in this series now. This fix
> also touches common x86 code but that should be ok because it could be reused
> by nested-vmx later.
> Please review and give comments (or apply ;-).


Nice work. Please remember to keep me CC'ed when posting nested SVM patches.


Alex

Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling

[Alexander Graf]

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.roedel@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

Re: [PATCH 0/8] More fixes for nested svm

[Alexander Graf]

On 22.04.2010, at 13:04, Avi Kivity wrote:

> On 04/22/2010 01:33 PM, Joerg Roedel wrote:
>> Hi Avi, Marcelo,
>> 
>> here is another set of nested svm fixes. They fix NMI code to make UP Hyper-V
>> root domain booting. The patches also add better handling for nested entry
>> failures and mce intercepts.
>> Also in this patchset are the fixes for the supported cpuid reporting for svm
>> features. These patches were taken from the nested-npt patchset and slightly
>> modified. These patches are also marked for -stable backporting.
>> The probably most important fix is about exception reinjection. This didn't
>> work reliably before and is fixed with the patch in this series now. This fix
>> also touches common x86 code but that should be ok because it could be reused
>> by nested-vmx later.
>> Please review and give comments (or apply ;-).
>>   
> 
> Looks good.  I'll wait a day or two for more reviews (esp. Alex).


Looking over it, but can't promise I'll be able to fully go through the feedback. Flying off for 2 weeks of vacation tomorrow morning.


Alex

Re: [PATCH 3/8] KVM: SVM: Sync cr0 and cr3 to kvm state before nested handling

[Alexander Graf]

On 22.04.2010, at 12:33, Joerg Roedel wrote:

> This patch syncs cr0 and cr3 from the vmcb to the kvm state
> before nested intercept handling is done. This allows to
> simplify the vmexit path.
> 
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
> arch/x86/kvm/svm.c |   15 ++++++---------
> 1 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index c480d7f..5ad9d80 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1799,10 +1799,7 @@ static int nested_svm_vmexit(struct vcpu_svm *svm)
> 	nested_vmcb->save.gdtr   = vmcb->save.gdtr;
> 	nested_vmcb->save.idtr   = vmcb->save.idtr;
> 	nested_vmcb->save.cr0    = kvm_read_cr0(&svm->vcpu);
> -	if (npt_enabled)
> -		nested_vmcb->save.cr3    = vmcb->save.cr3;
> -	else
> -		nested_vmcb->save.cr3    = svm->vcpu.arch.cr3;
> +	nested_vmcb->save.cr3    = svm->vcpu.arch.cr3;


Why don't we need this anymore again?


Alex

Re: [PATCH 4/8] KVM: SVM: Propagate nested entry failure into guest hypervisor

[Alexander Graf]

On 22.04.2010, at 12:33, Joerg Roedel wrote:

> This patch implements propagation of a failes guest vmrun
> back into the guest instead of killing the whole guest.

Oh, nice one.

Ack.


Alex

Re: [PATCH 5/8] KVM: X86: Add callback to let modules decide over some supported cpuid bits

[Alexander Graf]

On 22.04.2010, at 12:33, Joerg Roedel wrote:

> This patch adds the get_supported_cpuid callback to
> kvm_x86_ops. It will be used in do_cpuid_ent to delegate the
> decission about some supported cpuid bits to the
> architecture modules.
> 
> Cc: stable@kernel.org

Please don't CC stable. There is a KVM stable tag now, so every stable submission goes through Avi/Marcelo.


Alex

Re: [PATCH 6/8] KVM: SVM: Report emulated SVM features to userspace

[Alexander Graf]

On 22.04.2010, at 12:33, Joerg Roedel wrote:

> This patch implements the reporting of the emulated SVM
> features to userspace instead of the real hardware
> capabilities. Every real hardware capability needs emulation
> in nested svm so the old behavior was broken.
> 
> Cc: stable@kernel.org

Again, please don't CC stable directly.

> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
> arch/x86/kvm/svm.c |   10 ++++++++++
> 1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 0fa2035..65fc114 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -3154,6 +3154,16 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu)
> 
> static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
> {
> +	switch (func) {
> +	case 0x8000000A:
> +		entry->eax = 1; /* SVM revision 1 */
> +		entry->ebx = 8; /* Lets support 8 ASIDs in case we add proper
> +				   ASID emulation to nested SVM */

I completely forgot what we do now. What do we do? It shouldn't be too hard to keep a table around and just assign 8 host ASIDs to the guest. If possible lazily, so we can just flush the whole thing when we run out of entries.

It's basically the same as my VSID mapping on ppc64 actually. See arch/powerpc/kvm/book3s_64_mmu_host.c and search for "slb".

> +		entry->ecx = 0; /* Reserved */
> +		entry->edx = 0; /* Do not support any additional features */

What about nnpt? Hasn't that been accepted yet?


Alex

Re: [PATCH 7/8] KVM: x86: Allow marking an exception as reinjected

[Alexander Graf]

On 22.04.2010, at 12:33, Joerg Roedel wrote:

> This patch adds logic to kvm/x86 which allows to mark an
> injected exception as reinjected. This allows to remove an
> ugly hack from svm_complete_interrupts that prevented
> exceptions from being reinjected at all in the nested case.
> The hack was necessary because an reinjected exception into
> the nested guest could cause a nested vmexit emulation. But
> reinjected exceptions must not intercept. The downside of
> the hack is that a exception that in injected could get
> lost.
> This patch fixes the problem and puts the code for it into
> generic x86 files because. Nested-VMX will likely have the
> same problem and could reuse the code.

So we always handle the reinjection from KVM code? Shouldn't the l1 hypervisor do this?

Alex

Re: [PATCH 8/8] KVM: SVM: Handle MCE intercepts always on host level

[Alexander Graf]

On 22.04.2010, at 12:33, Joerg Roedel wrote:

> This patch prevents MCE intercepts from being propagated
> into the L1 guest if they happened in an L2 guest.

Good catch. How did you stumble over this?

Ack.

Alex

Re: [PATCH 5/8] KVM: X86: Add callback to let modules decide over some supported cpuid bits

[Avi Kivity]

On 04/23/2010 04:52 PM, Alexander Graf wrote:
> On 22.04.2010, at 12:33, Joerg Roedel wrote:
>
>    
>> This patch adds the get_supported_cpuid callback to
>> kvm_x86_ops. It will be used in do_cpuid_ent to delegate the
>> decission about some supported cpuid bits to the
>> architecture modules.
>>
>> Cc: stable@kernel.org
>>      
> Please don't CC stable. There is a KVM stable tag now, so every stable submission goes through Avi/Marcelo.
>
>
>    

It's not a problem.  stable@ will ignore kvm patches not coming from the 
maintainers, and we will recognize cc: stable as a stable request.

(the new approach is to collect the patches in kvm-updates/2.6.x, 
autotest, and forward to stable@).

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling

[Joerg Roedel]

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.roedel@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

Re: [PATCH 3/8] KVM: SVM: Sync cr0 and cr3 to kvm state before nested handling

[Joerg Roedel]

On Fri, Apr 23, 2010 at 03:50:20PM +0200, Alexander Graf wrote:
> 
> On 22.04.2010, at 12:33, Joerg Roedel wrote:
> 
> > This patch syncs cr0 and cr3 from the vmcb to the kvm state
> > before nested intercept handling is done. This allows to
> > simplify the vmexit path.
> > 
> > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> > ---
> > arch/x86/kvm/svm.c |   15 ++++++---------
> > 1 files changed, 6 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index c480d7f..5ad9d80 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -1799,10 +1799,7 @@ static int nested_svm_vmexit(struct vcpu_svm *svm)
> > 	nested_vmcb->save.gdtr   = vmcb->save.gdtr;
> > 	nested_vmcb->save.idtr   = vmcb->save.idtr;
> > 	nested_vmcb->save.cr0    = kvm_read_cr0(&svm->vcpu);
> > -	if (npt_enabled)
> > -		nested_vmcb->save.cr3    = vmcb->save.cr3;
> > -	else
> > -		nested_vmcb->save.cr3    = svm->vcpu.arch.cr3;
> > +	nested_vmcb->save.cr3    = svm->vcpu.arch.cr3;
> 
> 
> Why don't we need this anymore again?

Because arch.cr3 contains always the current l2-cr3. This wasn't the
case before because the sync was done after the nested handling. But now
we do it before a vmexit can happen and are safe to just use arch.cr3.

	Joerg

Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling

[Alexander Graf]

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.roedel@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

Re: [PATCH 3/8] KVM: SVM: Sync cr0 and cr3 to kvm state before nested handling

[Alexander Graf]

On 23.04.2010, at 16:17, Joerg Roedel wrote:

> On Fri, Apr 23, 2010 at 03:50:20PM +0200, Alexander Graf wrote:
>> 
>> On 22.04.2010, at 12:33, Joerg Roedel wrote:
>> 
>>> This patch syncs cr0 and cr3 from the vmcb to the kvm state
>>> before nested intercept handling is done. This allows to
>>> simplify the vmexit path.
>>> 
>>> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
>>> ---
>>> arch/x86/kvm/svm.c |   15 ++++++---------
>>> 1 files changed, 6 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>> index c480d7f..5ad9d80 100644
>>> --- a/arch/x86/kvm/svm.c
>>> +++ b/arch/x86/kvm/svm.c
>>> @@ -1799,10 +1799,7 @@ static int nested_svm_vmexit(struct vcpu_svm *svm)
>>> 	nested_vmcb->save.gdtr   = vmcb->save.gdtr;
>>> 	nested_vmcb->save.idtr   = vmcb->save.idtr;
>>> 	nested_vmcb->save.cr0    = kvm_read_cr0(&svm->vcpu);
>>> -	if (npt_enabled)
>>> -		nested_vmcb->save.cr3    = vmcb->save.cr3;
>>> -	else
>>> -		nested_vmcb->save.cr3    = svm->vcpu.arch.cr3;
>>> +	nested_vmcb->save.cr3    = svm->vcpu.arch.cr3;
>> 
>> 
>> Why don't we need this anymore again?
> 
> Because arch.cr3 contains always the current l2-cr3. This wasn't the
> case before because the sync was done after the nested handling. But now
> we do it before a vmexit can happen and are safe to just use arch.cr3.

I see.


Alex

Re: [PATCH 6/8] KVM: SVM: Report emulated SVM features to userspace

[Joerg Roedel]

On Fri, Apr 23, 2010 at 03:55:15PM +0200, Alexander Graf wrote:
> 
> On 22.04.2010, at 12:33, Joerg Roedel wrote:
> > static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
> > {
> > +	switch (func) {
> > +	case 0x8000000A:
> > +		entry->eax = 1; /* SVM revision 1 */
> > +		entry->ebx = 8; /* Lets support 8 ASIDs in case we add proper
> > +				   ASID emulation to nested SVM */
> 
> I completely forgot what we do now. What do we do? It shouldn't be too
> hard to keep a table around and just assign 8 host ASIDs to the guest.
> If possible lazily, so we can just flush the whole thing when we run
> out of entries.

Currently we have no ASID emulation. We just assign a new asid at every
vmrun/vmexit. But I have a rough idea of how we can emulate ASIDs for
the L2 guest with an per-vcpu ASID cache.

> It's basically the same as my VSID mapping on ppc64 actually. See
> arch/powerpc/kvm/book3s_64_mmu_host.c and search for "slb".

Thanks, will have a look there.

> > +		entry->ecx = 0; /* Reserved */
> > +		entry->edx = 0; /* Do not support any additional features */
> 
> What about nnpt? Hasn't that been accepted yet?

I am currently working on it. If all goes well I can submit a new
version next week.

	Joerg

Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling

[Joerg Roedel]

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.roedel@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

Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling

[Alexander Graf]

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.roedel@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

Re: [PATCH 7/8] KVM: x86: Allow marking an exception as reinjected

[Joerg Roedel]

On Fri, Apr 23, 2010 at 03:57:32PM +0200, Alexander Graf wrote:
> 
> On 22.04.2010, at 12:33, Joerg Roedel wrote:
> 
> > This patch adds logic to kvm/x86 which allows to mark an
> > injected exception as reinjected. This allows to remove an
> > ugly hack from svm_complete_interrupts that prevented
> > exceptions from being reinjected at all in the nested case.
> > The hack was necessary because an reinjected exception into
> > the nested guest could cause a nested vmexit emulation. But
> > reinjected exceptions must not intercept. The downside of
> > the hack is that a exception that in injected could get
> > lost.
> > This patch fixes the problem and puts the code for it into
> > generic x86 files because. Nested-VMX will likely have the
> > same problem and could reuse the code.
> 
> So we always handle the reinjection from KVM code? Shouldn't the l1
> hypervisor do this?

No. We only have the problem if we need to handle a nested intercept on
the host level instead of reinjecting it. So the nested hypervisor
couldn't be involved in the reinjection.

	Joerg

Re: [PATCH 8/8] KVM: SVM: Handle MCE intercepts always on host level

[Joerg Roedel]

On Fri, Apr 23, 2010 at 03:58:18PM +0200, Alexander Graf wrote:
> 
> On 22.04.2010, at 12:33, Joerg Roedel wrote:
> 
> > This patch prevents MCE intercepts from being propagated
> > into the L1 guest if they happened in an L2 guest.
> 
> Good catch. How did you stumble over this?

While thinking about another issue which I can't disclose
right now ;-) Stay tuned...

	Joerg

Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling

[Joerg Roedel]

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

Re: [PATCH 7/8] KVM: x86: Allow marking an exception as reinjected

[Alexander Graf]

On 23.04.2010, at 16:27, Joerg Roedel wrote:

> On Fri, Apr 23, 2010 at 03:57:32PM +0200, Alexander Graf wrote:
>> 
>> On 22.04.2010, at 12:33, Joerg Roedel wrote:
>> 
>>> This patch adds logic to kvm/x86 which allows to mark an
>>> injected exception as reinjected. This allows to remove an
>>> ugly hack from svm_complete_interrupts that prevented
>>> exceptions from being reinjected at all in the nested case.
>>> The hack was necessary because an reinjected exception into
>>> the nested guest could cause a nested vmexit emulation. But
>>> reinjected exceptions must not intercept. The downside of
>>> the hack is that a exception that in injected could get
>>> lost.
>>> This patch fixes the problem and puts the code for it into
>>> generic x86 files because. Nested-VMX will likely have the
>>> same problem and could reuse the code.
>> 
>> So we always handle the reinjection from KVM code? Shouldn't the l1
>> hypervisor do this?
> 
> No. We only have the problem if we need to handle a nested intercept on
> the host level instead of reinjecting it. So the nested hypervisor
> couldn't be involved in the reinjection.

Hrm, makes sense. Not pretty.


Alex

Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling

[Alexander Graf]

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

Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling

[Joerg Roedel]

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

Re: [PATCH 1/8] KVM: SVM: Fix nested nmi handling

[Alexander Graf]

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

Re: [PATCH 0/8] More fixes for nested svm

[Alexander Graf]

On 22.04.2010, at 13:04, Avi Kivity wrote:

> On 04/22/2010 01:33 PM, Joerg Roedel wrote:
>> Hi Avi, Marcelo,
>> 
>> here is another set of nested svm fixes. They fix NMI code to make UP Hyper-V
>> root domain booting. The patches also add better handling for nested entry
>> failures and mce intercepts.
>> Also in this patchset are the fixes for the supported cpuid reporting for svm
>> features. These patches were taken from the nested-npt patchset and slightly
>> modified. These patches are also marked for -stable backporting.
>> The probably most important fix is about exception reinjection. This didn't
>> work reliably before and is fixed with the patch in this series now. This fix
>> also touches common x86 code but that should be ok because it could be reused
>> by nested-vmx later.
>> Please review and give comments (or apply ;-).
>>   
> 
> Looks good.  I'll wait a day or two for more reviews (esp. Alex).

No complaints from me.


Alex

Re: [PATCH 0/8] More fixes for nested svm

[Avi Kivity]

On 04/22/2010 01:33 PM, Joerg Roedel wrote:
> Hi Avi, Marcelo,
>
> here is another set of nested svm fixes. They fix NMI code to make UP Hyper-V
> root domain booting. The patches also add better handling for nested entry
> failures and mce intercepts.
> Also in this patchset are the fixes for the supported cpuid reporting for svm
> features. These patches were taken from the nested-npt patchset and slightly
> modified. These patches are also marked for -stable backporting.
> The probably most important fix is about exception reinjection. This didn't
> work reliably before and is fixed with the patch in this series now. This fix
> also touches common x86 code but that should be ok because it could be reused
> by nested-vmx later.
> Please review and give comments (or apply ;-).
>
>    

All applied, thanks.

Regarding stable, it should be easy to backport the patches to 
2.6.34-rc6, but for 2.6.33 and earlier, it's murky.  And we certainly 
don't have the means to test them.

So please prepare _tested_ patchsets (git is fine) for 2.6.3[432].  I 
suggest you hold off for 2.6.3[32] until their next release, since there 
are a lot of kvm patches in review for them.  In general patches should 
be prepared against kvm.git kvm-updates/2.6.foo; we'll keep them up to 
date against stable.

Longer term, we need kvm-autotest support for nsvm, and unit tests in 
qemu-kvm/kvm/user/test.

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