[PATCH 0/9] KVM: Make the instruction emulator aware of Nested Virtualization

[PATCH 0/9] KVM: Make the instruction emulator aware of Nested Virtualization

[Joerg Roedel]

Hi Avi, Hi Marcelo,

here is a patch-set to make the instruction emulator aware of nested
virtualization. It basically works by introducing a new callback into
the x86_ops to check if a decoded instruction must be intercepted. If it
is intercepted the instruction emulator returns straight into the guest.

I am not entirely happy with this solution because it partially
duplicates the code in the x86_emulate_insn function. But there are so
many SVM specific cases that need to be taken care of that I consider
this solution the better one (even when looking at the diff-stat).
Keeping this (SVM-specific) complexity in the SVM specific code is
better than extending the generic instruction emulator code path.

The last patch removes the ugly hacks which were required without this
patch-set to correctly handle the selective-cr0-write intercept.

I appreciate your feedback.

Thanks,

	Joerg

Diffstat:

 arch/x86/include/asm/kvm_emulate.h |    2 +
 arch/x86/include/asm/kvm_host.h    |    3 +
 arch/x86/kvm/svm.c                 |  330 ++++++++++++++++++++++++++++++------
 arch/x86/kvm/vmx.c                 |    8 +
 arch/x86/kvm/x86.c                 |    5 +
 5 files changed, 297 insertions(+), 51 deletions(-)

Shortlog:

Joerg Roedel (9):
      KVM: Add infrastructure to emulate instruction intercepts
      KVM: SVM: Add checks for CRx read and write intercepts
      KVM: SVM: Add checks for DRx read and write intercepts
      KVM: SVM: Add intercept checks for descriptor table accesses
      KVM: SVM: Add checks for all group 7 instructions
      KVM: SVM: Add intercept checks for remaining twobyte instructions
      KVM: SVM: Add intercept checks for one-byte instructions
      KVM: SVM: Add checks for IO instructions
      KVM: SVM: Remove nested sel_cr0_write handling code

[PATCH 1/9] KVM: Add infrastructure to emulate instruction intercepts

[Joerg Roedel]

This patch adds the necessary infrastructure to KVM to
implement instruction intercepts when the vcpu in in
emulated guest mode.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/kvm_emulate.h |    2 ++
 arch/x86/include/asm/kvm_host.h    |    3 +++
 arch/x86/kvm/svm.c                 |    8 ++++++++
 arch/x86/kvm/vmx.c                 |    8 ++++++++
 arch/x86/kvm/x86.c                 |    5 +++++
 5 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index b48c133..3498431 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -54,6 +54,8 @@ struct x86_emulate_ctxt;
 #define X86EMUL_RETRY_INSTR     3 /* retry the instruction for some reason */
 #define X86EMUL_CMPXCHG_FAILED  4 /* cmpxchg did not see expected value */
 #define X86EMUL_IO_NEEDED       5 /* IO is needed to complete emulation */
+#define X86EMUL_INTERCEPTED     6 /* VCPU is in guest mode and the
+				     instruction is intercepted */
 
 struct x86_emulate_ops {
 	/*
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 54e42c8..bcc781b 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -596,6 +596,9 @@ struct kvm_x86_ops {
 
 	void (*get_exit_info)(struct kvm_vcpu *vcpu, u64 *info1, u64 *info2);
 	const struct trace_print_flags *exit_reasons_str;
+
+	int (*insn_intercepted)(struct kvm_vcpu *vcpu,
+				struct x86_emulate_ctxt *ctxt);
 };
 
 struct kvm_arch_async_pf {
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 2fd2f4d..d1721c2 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3619,6 +3619,12 @@ static void svm_fpu_deactivate(struct kvm_vcpu *vcpu)
 	update_cr0_intercept(svm);
 }
 
+static int svm_insn_intercepted(struct kvm_vcpu *vcpu,
+				struct x86_emulate_ctxt *ctxt)
+{
+	return X86EMUL_CONTINUE;
+}
+
 static struct kvm_x86_ops svm_x86_ops = {
 	.cpu_has_kvm_support = has_svm,
 	.disabled_by_bios = is_disabled,
@@ -3703,6 +3709,8 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.adjust_tsc_offset = svm_adjust_tsc_offset,
 
 	.set_tdp_cr3 = set_tdp_cr3,
+
+	.insn_intercepted = svm_insn_intercepted,
 };
 
 static int __init svm_init(void)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index caa967e..81de3a9 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4278,6 +4278,12 @@ static void vmx_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
 {
 }
 
+static int vmx_insn_intercepted(struct kvm_vcpu *vcpu,
+				struct x86_emulate_ctxt *ctxt)
+{
+	return X86EMUL_CONTINUE;
+}
+
 static struct kvm_x86_ops vmx_x86_ops = {
 	.cpu_has_kvm_support = cpu_has_kvm_support,
 	.disabled_by_bios = vmx_disabled_by_bios,
@@ -4362,6 +4368,8 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.adjust_tsc_offset = vmx_adjust_tsc_offset,
 
 	.set_tdp_cr3 = vmx_set_cr3,
+
+	.insn_intercepted = vmx_insn_intercepted,
 };
 
 static int __init vmx_init(void)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 410d2d1..759cc19 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4383,6 +4383,11 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 		if (r == X86EMUL_PROPAGATE_FAULT)
 			goto done;
 
+		r = kvm_x86_ops->insn_intercepted(vcpu,
+						  &vcpu->arch.emulate_ctxt);
+		if (r == X86EMUL_INTERCEPTED)
+			return EMULATE_DONE;
+
 		trace_kvm_emulate_insn_start(vcpu);
 
 		/* Only allow emulation of specific instructions on #UD
-- 
1.7.1

[PATCH 2/9] KVM: SVM: Add checks for CRx read and write intercepts

[Joerg Roedel]

This patch add code to check for any CRx read, write, and
the selective-cr0 intercepts for instructions emulated in
software by KVM.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index d1721c2..29f0491 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3622,7 +3622,78 @@ static void svm_fpu_deactivate(struct kvm_vcpu *vcpu)
 static int svm_insn_intercepted(struct kvm_vcpu *vcpu,
 				struct x86_emulate_ctxt *ctxt)
 {
-	return X86EMUL_CONTINUE;
+	struct decode_cache *c = &ctxt->decode;
+	struct vcpu_svm *svm = to_svm(vcpu);
+	struct vmcb *vmcb = svm->vmcb;
+	int vmexit, ret;
+
+	if (!is_nested(svm))
+		return X86EMUL_CONTINUE;
+
+	ret = X86EMUL_CONTINUE;
+
+	if (!c->twobyte)
+		goto out;
+
+	switch (c->b) {
+	case 0x01:
+		/* 0x0f 0x01 and modrm_mod == 3 encodes special instructions */
+		if (c->modrm_mod == 3)
+			break;
+
+		switch (c->modrm_reg) {
+		case 0x04: /* SMSW */
+			vmcb->control.exit_code = SVM_EXIT_READ_CR0;
+			break;
+		case 0x06:  { /* LMSW */
+			u64 cr0, val;
+
+			vmcb->control.exit_code = SVM_EXIT_WRITE_CR0;
+
+			if (svm->nested.intercept_cr_write & INTERCEPT_CR0_MASK)
+				break;
+
+			/* check for selective-cr0 special case */
+			cr0 = vcpu->arch.cr0 & ~SVM_CR0_SELECTIVE_MASK & 0xf;
+			val = c->src.val     & ~SVM_CR0_SELECTIVE_MASK & 0xf;
+
+			if (cr0 ^ val)
+				vmcb->control.exit_code = SVM_EXIT_CR0_SEL_WRITE;
+
+			break;
+			}
+		}
+	case 0x06: /* CLTS */
+		vmcb->control.exit_code = SVM_EXIT_WRITE_CR0;
+		break;
+	case 0x20: /* CR read  */
+		vmcb->control.exit_code = SVM_EXIT_READ_CR0 + c->modrm_reg;
+		break;
+	case 0x22: /* CR write */
+		vmcb->control.exit_code = SVM_EXIT_WRITE_CR0 + c->modrm_reg;
+		if (c->modrm_reg == 0 &&
+		    !(svm->nested.intercept_cr_write & INTERCEPT_CR0_MASK)) {
+			/* check for selective-cr0 special case */
+			u64 cr0, val;
+
+			cr0 = vcpu->arch.cr0 & ~SVM_CR0_SELECTIVE_MASK;
+			val = c->src.val     & ~SVM_CR0_SELECTIVE_MASK;
+
+			if (cr0 ^ val)
+				vmcb->control.exit_code = SVM_EXIT_CR0_SEL_WRITE;
+		}
+		break;
+	}
+
+	vmcb->control.next_rip = ctxt->eip;
+	vmexit = nested_svm_exit_handled(svm);
+
+	ret = (vmexit == NESTED_EXIT_DONE) ? X86EMUL_INTERCEPTED
+					   : X86EMUL_CONTINUE;
+
+out:
+
+	return ret;
 }
 
 static struct kvm_x86_ops svm_x86_ops = {
-- 
1.7.1

[PATCH 3/9] KVM: SVM: Add checks for DRx read and write intercepts

[Joerg Roedel]

This patch adds checking for read and write intercepts
from/to DRx registers from the KVM instruction emulator
path.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 29f0491..16ff569 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3669,6 +3669,9 @@ static int svm_insn_intercepted(struct kvm_vcpu *vcpu,
 	case 0x20: /* CR read  */
 		vmcb->control.exit_code = SVM_EXIT_READ_CR0 + c->modrm_reg;
 		break;
+	case 0x21:
+		vmcb->control.exit_code = SVM_EXIT_READ_DR0 + c->modrm_reg;
+		break;
 	case 0x22: /* CR write */
 		vmcb->control.exit_code = SVM_EXIT_WRITE_CR0 + c->modrm_reg;
 		if (c->modrm_reg == 0 &&
@@ -3683,6 +3686,9 @@ static int svm_insn_intercepted(struct kvm_vcpu *vcpu,
 				vmcb->control.exit_code = SVM_EXIT_CR0_SEL_WRITE;
 		}
 		break;
+	case 0x23:
+		vmcb->control.exit_code = SVM_EXIT_WRITE_DR0 + c->modrm_reg;
+		break;
 	}
 
 	vmcb->control.next_rip = ctxt->eip;
-- 
1.7.1

[PATCH 4/9] KVM: SVM: Add intercept checks for descriptor table accesses

[Joerg Roedel]

This patch adds checks for accesses to the descriptor table
base registers to the intruction emulator path of KVM.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 16ff569..035c5b5 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3636,12 +3636,40 @@ static int svm_insn_intercepted(struct kvm_vcpu *vcpu,
 		goto out;
 
 	switch (c->b) {
+	case 0x00:
+		switch (c->modrm_reg) {
+		case 0x00: /* SLDT */
+			vmcb->control.exit_code = SVM_EXIT_LDTR_READ;
+			break;
+		case 0x01: /* STR */
+			vmcb->control.exit_code = SVM_EXIT_TR_READ;
+			break;
+		case 0x02: /* LLDT */
+			vmcb->control.exit_code = SVM_EXIT_LDTR_WRITE;
+			break;
+		case 0x03: /* LTR */
+			vmcb->control.exit_code = SVM_EXIT_TR_WRITE;
+			break;
+		}
+		break;
 	case 0x01:
 		/* 0x0f 0x01 and modrm_mod == 3 encodes special instructions */
 		if (c->modrm_mod == 3)
 			break;
 
 		switch (c->modrm_reg) {
+		case 0x00: /* SGDT */
+			vmcb->control.exit_code = SVM_EXIT_GDTR_READ;
+			break;
+		case 0x01: /* SIDT */
+			vmcb->control.exit_code = SVM_EXIT_IDTR_READ;
+			break;
+		case 0x02: /* LGDT */
+			vmcb->control.exit_code = SVM_EXIT_GDTR_WRITE;
+			break;
+		case 0x03: /* LIDT */
+			vmcb->control.exit_code = SVM_EXIT_IDTR_WRITE;
+			break;
 		case 0x04: /* SMSW */
 			vmcb->control.exit_code = SVM_EXIT_READ_CR0;
 			break;
-- 
1.7.1

[PATCH 5/9] KVM: SVM: Add checks for all group 7 instructions

[Joerg Roedel]

This patch adds intercept checks for all the group 7
instructions to the KVM instruction emulator path.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 035c5b5..7284193 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3619,6 +3619,55 @@ static void svm_fpu_deactivate(struct kvm_vcpu *vcpu)
 	update_cr0_intercept(svm);
 }
 
+static void svm_check_group7(struct vmcb *vmcb, struct x86_emulate_ctxt *ctxt)
+{
+	struct decode_cache *c = &ctxt->decode;
+
+	switch (c->modrm_rm) {
+	case 0:
+		switch (c->modrm_reg) {
+		case 1:
+			vmcb->control.exit_code = SVM_EXIT_MONITOR;
+			break;
+		case 3:
+			vmcb->control.exit_code = SVM_EXIT_VMRUN;
+			break;
+		}
+		break;
+	case 1:
+		switch (c->modrm_reg) {
+		case 1:
+			vmcb->control.exit_code = SVM_EXIT_MWAIT;
+			break;
+		case 3:
+			vmcb->control.exit_code = SVM_EXIT_VMMCALL;
+			break;
+		case 7:
+			vmcb->control.exit_code = SVM_EXIT_RDTSCP;
+			break;
+		}
+		break;
+	case 2:
+		vmcb->control.exit_code = SVM_EXIT_VMLOAD;
+		break;
+	case 3:
+		vmcb->control.exit_code = SVM_EXIT_VMSAVE;
+		break;
+	case 4:
+		vmcb->control.exit_code = SVM_EXIT_STGI;
+		break;
+	case 5:
+		vmcb->control.exit_code = SVM_EXIT_CLGI;
+		break;
+	case 6:
+		vmcb->control.exit_code = SVM_EXIT_SKINIT;
+		break;
+	case 7:
+		vmcb->control.exit_code = SVM_EXIT_INVLPGA;
+		break;
+	}
+}
+
 static int svm_insn_intercepted(struct kvm_vcpu *vcpu,
 				struct x86_emulate_ctxt *ctxt)
 {
@@ -3654,8 +3703,10 @@ static int svm_insn_intercepted(struct kvm_vcpu *vcpu,
 		break;
 	case 0x01:
 		/* 0x0f 0x01 and modrm_mod == 3 encodes special instructions */
-		if (c->modrm_mod == 3)
+		if (c->modrm_mod == 3) {
+			svm_check_group7(vmcb, ctxt);
 			break;
+		}
 
 		switch (c->modrm_reg) {
 		case 0x00: /* SGDT */
-- 
1.7.1

[PATCH 6/9] KVM: SVM: Add intercept checks for remaining twobyte instructions

[Joerg Roedel]

This patch adds intercepts checks for the remaining twobyte
instructions to the KVM instruction emulator.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 7284193..77b344e 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3741,10 +3741,19 @@ static int svm_insn_intercepted(struct kvm_vcpu *vcpu,
 
 			break;
 			}
+		case 0x07: /* INVLPG */
+			vmcb->control.exit_code = SVM_EXIT_INVLPG;
+			break;
 		}
 	case 0x06: /* CLTS */
 		vmcb->control.exit_code = SVM_EXIT_WRITE_CR0;
 		break;
+	case 0x08: /* INVD */
+		vmcb->control.exit_code = SVM_EXIT_INVD;
+		break;
+	case 0x09: /* WBINVD */
+		vmcb->control.exit_code = SVM_EXIT_WBINVD;
+		break;
 	case 0x20: /* CR read  */
 		vmcb->control.exit_code = SVM_EXIT_READ_CR0 + c->modrm_reg;
 		break;
@@ -3768,6 +3777,26 @@ static int svm_insn_intercepted(struct kvm_vcpu *vcpu,
 	case 0x23:
 		vmcb->control.exit_code = SVM_EXIT_WRITE_DR0 + c->modrm_reg;
 		break;
+	case 0x30: /* WRMSR */
+		vmcb->control.exit_code = SVM_EXIT_MSR;
+		vmcb->control.exit_info_1 = 1;
+		break;
+	case 0x31: /* RDTSC */
+		vmcb->control.exit_code = SVM_EXIT_RDTSC;
+		break;
+	case 0x32: /* RDMSR */
+		vmcb->control.exit_code = SVM_EXIT_MSR;
+		vmcb->control.exit_info_1 = 0;
+		break;
+	case 0x33: /* RDPMC */
+		vmcb->control.exit_code = SVM_EXIT_RDPMC;
+		break;
+	case 0xa2: /* CPUID */
+		vmcb->control.exit_code = SVM_EXIT_CPUID;
+		break;
+	case 0xaa: /* RSM */
+		vmcb->control.exit_code = SVM_EXIT_RSM;
+		break;
 	}
 
 	vmcb->control.next_rip = ctxt->eip;
-- 
1.7.1

[PATCH 7/9] KVM: SVM: Add intercept checks for one-byte instructions

[Joerg Roedel]

This patch add intercept checks for emulated one-byte
instructions to the KVM instruction emulation path.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 77b344e..2a30b5b 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3668,6 +3668,35 @@ static void svm_check_group7(struct vmcb *vmcb, struct x86_emulate_ctxt *ctxt)
 	}
 }
 
+static void svm_check_onebyte(struct vmcb *vmcb, struct x86_emulate_ctxt *ctxt)
+{
+	struct decode_cache *c = &ctxt->decode;
+
+	switch (c->b) {
+	case 0x90: /* PAUSE */
+		if (c->rep_prefix == REPE_PREFIX)
+			vmcb->control.exit_code = SVM_EXIT_PAUSE;
+		break;
+	case 0x9c: /* PUSHF */
+		break;
+	case 0x9d: /* POPF */
+		vmcb->control.exit_code = SVM_EXIT_POPF;
+		break;
+	case 0xcd: /* INTn */
+		vmcb->control.exit_code = SVM_EXIT_SWINT;
+		break;
+	case 0xcf: /* IRET */
+		vmcb->control.exit_code = SVM_EXIT_IRET;
+		break;
+	case 0xf1: /* ICEBP */
+		vmcb->control.exit_code = SVM_EXIT_ICEBP;
+		break;
+	case 0xf4: /* HLT */
+		vmcb->control.exit_code = SVM_EXIT_HLT;
+		break;
+	}
+}
+
 static int svm_insn_intercepted(struct kvm_vcpu *vcpu,
 				struct x86_emulate_ctxt *ctxt)
 {
@@ -3681,8 +3710,10 @@ static int svm_insn_intercepted(struct kvm_vcpu *vcpu,
 
 	ret = X86EMUL_CONTINUE;
 
-	if (!c->twobyte)
+	if (!c->twobyte) {
+		svm_check_onebyte(vmcb, ctxt);
 		goto out;
+	}
 
 	switch (c->b) {
 	case 0x00:
@@ -3799,14 +3830,13 @@ static int svm_insn_intercepted(struct kvm_vcpu *vcpu,
 		break;
 	}
 
+out:
 	vmcb->control.next_rip = ctxt->eip;
 	vmexit = nested_svm_exit_handled(svm);
 
 	ret = (vmexit == NESTED_EXIT_DONE) ? X86EMUL_INTERCEPTED
 					   : X86EMUL_CONTINUE;
 
-out:
-
 	return ret;
 }
 
-- 
1.7.1

[PATCH 8/9] KVM: SVM: Add checks for IO instructions

[Joerg Roedel]

This patch adds code to check for IOIO intercepts on
instructions decoded by the KVM instruction emulator.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 2a30b5b..a2c513d 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3668,6 +3668,47 @@ static void svm_check_group7(struct vmcb *vmcb, struct x86_emulate_ctxt *ctxt)
 	}
 }
 
+static void svm_check_ioio(struct vmcb *vmcb, struct x86_emulate_ctxt *ctxt)
+{
+	struct decode_cache *c;
+	u32 bytes, seg;
+	u32 exit_info;
+
+	c         = &ctxt->decode;
+	/* Encode Port */
+	exit_info = (c->regs[VCPU_REGS_RDX] & 0xffff) << 16;
+	seg       = 0;
+
+	/* Encode Address Size */
+	exit_info |= (u32)c->ad_bytes << (SVM_IOIO_ASIZE_SHIFT - 1);
+
+	if (c->rep_prefix)
+		exit_info |= SVM_IOIO_REP_MASK;
+
+	if ((c->b & 0x06) == 0x06) {
+		/* OUT* Instruction */
+		bytes = c->dst.bytes;
+	} else {
+		/* IN* Instruction */
+		bytes      = c->src.bytes;
+		exit_info |= SVM_IOIO_TYPE_MASK;
+	}
+
+	/* Encode Data Size */
+	bytes      = min(bytes, 4u);
+	exit_info |= bytes << SVM_IOIO_SIZE_SHIFT;
+
+	/* Check for the string variants */
+	if ((c->b & 0xf0) == 0x60) {
+		exit_info |= SVM_IOIO_STR_MASK;
+		exit_info |= (seg & 0x07) << 10;
+	}
+
+	vmcb->control.exit_info_1 = exit_info;
+	vmcb->control.exit_info_2 = ctxt->eip;
+	vmcb->control.exit_code   = SVM_EXIT_IOIO;
+}
+
 static void svm_check_onebyte(struct vmcb *vmcb, struct x86_emulate_ctxt *ctxt)
 {
 	struct decode_cache *c = &ctxt->decode;
@@ -3685,6 +3726,20 @@ static void svm_check_onebyte(struct vmcb *vmcb, struct x86_emulate_ctxt *ctxt)
 	case 0xcd: /* INTn */
 		vmcb->control.exit_code = SVM_EXIT_SWINT;
 		break;
+	case 0x6c: /* INS */
+	case 0x6d:
+	case 0xe4: /* IN */
+	case 0xe5:
+	case 0xec:
+	case 0xed:
+	case 0x6e: /* OUTS */
+	case 0x6f:
+	case 0xe6: /* OUT */
+	case 0xe7:
+	case 0xee:
+	case 0xef:
+		svm_check_ioio(vmcb, ctxt);
+		break;
 	case 0xcf: /* IRET */
 		vmcb->control.exit_code = SVM_EXIT_IRET;
 		break;
-- 
1.7.1

[PATCH 9/9] KVM: SVM: Remove nested sel_cr0_write handling code

[Joerg Roedel]

This patch removes all the old code which handled the nested
selective cr0 write intercepts. This code was only in place
as a work-around until the instruction emulator is capable
of doing the same. This is the case with this patch-set and
so the code can be removed.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index a2c513d..b34e905 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -89,14 +89,6 @@ struct nested_state {
 	/* A VMEXIT is required but not yet emulated */
 	bool exit_required;
 
-	/*
-	 * If we vmexit during an instruction emulation we need this to restore
-	 * the l1 guest rip after the emulation
-	 */
-	unsigned long vmexit_rip;
-	unsigned long vmexit_rsp;
-	unsigned long vmexit_rax;
-
 	/* cache for intercepts of the guest */
 	u16 intercept_cr_read;
 	u16 intercept_cr_write;
@@ -1233,31 +1225,6 @@ static void svm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
-	if (is_nested(svm)) {
-		/*
-		 * We are here because we run in nested mode, the host kvm
-		 * intercepts cr0 writes but the l1 hypervisor does not.
-		 * But the L1 hypervisor may intercept selective cr0 writes.
-		 * This needs to be checked here.
-		 */
-		unsigned long old, new;
-
-		/* Remove bits that would trigger a real cr0 write intercept */
-		old = vcpu->arch.cr0 & SVM_CR0_SELECTIVE_MASK;
-		new = cr0 & SVM_CR0_SELECTIVE_MASK;
-
-		if (old == new) {
-			/* cr0 write with ts and mp unchanged */
-			svm->vmcb->control.exit_code = SVM_EXIT_CR0_SEL_WRITE;
-			if (nested_svm_exit_handled(svm) == NESTED_EXIT_DONE) {
-				svm->nested.vmexit_rip = kvm_rip_read(vcpu);
-				svm->nested.vmexit_rsp = kvm_register_read(vcpu, VCPU_REGS_RSP);
-				svm->nested.vmexit_rax = kvm_register_read(vcpu, VCPU_REGS_RAX);
-				return;
-			}
-		}
-	}
-
 #ifdef CONFIG_X86_64
 	if (vcpu->arch.efer & EFER_LME) {
 		if (!is_paging(vcpu) && (cr0 & X86_CR0_PG)) {
@@ -2546,23 +2513,6 @@ static int emulate_on_interception(struct vcpu_svm *svm)
 	return emulate_instruction(&svm->vcpu, 0, 0, 0) == EMULATE_DONE;
 }
 
-static int cr0_write_interception(struct vcpu_svm *svm)
-{
-	struct kvm_vcpu *vcpu = &svm->vcpu;
-	int r;
-
-	r = emulate_instruction(&svm->vcpu, 0, 0, 0);
-
-	if (svm->nested.vmexit_rip) {
-		kvm_register_write(vcpu, VCPU_REGS_RIP, svm->nested.vmexit_rip);
-		kvm_register_write(vcpu, VCPU_REGS_RSP, svm->nested.vmexit_rsp);
-		kvm_register_write(vcpu, VCPU_REGS_RAX, svm->nested.vmexit_rax);
-		svm->nested.vmexit_rip = 0;
-	}
-
-	return r == EMULATE_DONE;
-}
-
 static int cr8_write_interception(struct vcpu_svm *svm)
 {
 	struct kvm_run *kvm_run = svm->vcpu.run;
@@ -2826,7 +2776,7 @@ static int (*svm_exit_handlers[])(struct vcpu_svm *svm) = {
 	[SVM_EXIT_READ_CR4]			= emulate_on_interception,
 	[SVM_EXIT_READ_CR8]			= emulate_on_interception,
 	[SVM_EXIT_CR0_SEL_WRITE]		= emulate_on_interception,
-	[SVM_EXIT_WRITE_CR0]			= cr0_write_interception,
+	[SVM_EXIT_WRITE_CR0]			= emulate_on_interception,
 	[SVM_EXIT_WRITE_CR3]			= emulate_on_interception,
 	[SVM_EXIT_WRITE_CR4]			= emulate_on_interception,
 	[SVM_EXIT_WRITE_CR8]			= cr8_write_interception,
-- 
1.7.1

Re: [PATCH 0/9] KVM: Make the instruction emulator aware of Nested Virtualization

[Avi Kivity]

On 11/24/2010 08:18 PM, Joerg Roedel wrote:
> Hi Avi, Hi Marcelo,
>
> here is a patch-set to make the instruction emulator aware of nested
> virtualization. It basically works by introducing a new callback into
> the x86_ops to check if a decoded instruction must be intercepted. If it
> is intercepted the instruction emulator returns straight into the guest.
>
> I am not entirely happy with this solution because it partially
> duplicates the code in the x86_emulate_insn function.

My big worry is that it makes svm.c aware of internal emulator variable, 
so it makes it harder to hack on the emulator.

> But there are so
> many SVM specific cases that need to be taken care of that I consider
> this solution the better one (even when looking at the diff-stat).
> Keeping this (SVM-specific) complexity in the SVM specific code is
> better than extending the generic instruction emulator code path.

I don't think there's a problem with svm specific code in the emulator 
for this.  My reasoning is that there are two classes of svm code: the 
common one is using svm to implement kvm, and the other one is emulating 
the svm instruction set.  Most of the current svm code belongs to the 
first class, even the nested svm code.  For example the code that 
emulates VMRUN is kvm-specific, while the code that decides whether to 
#GP on VMRUN or not is generic.

So I don't think there's a problem with coding the svm intercepts in 
emulate.c.  This is no different than emulating any AMD-specific 
instruction in the emulator - we're emulating an instruction in exactly 
the way it is specified in the manual.

Something you could do is allocate bits for the intercept bit number and 
exit code in opcode->flags.  This way most unconditional intercepts 
happen outside the instruction switch: generic code reads the intercept 
bit, the intercept word (via a callback), if the bit is set, returns the 
exit code.  That should completely kill the diffstat.  We only need to 
be careful wrt the order of the intercept check and the other permission 
checks.

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

Re: [PATCH 0/9] KVM: Make the instruction emulator aware of Nested Virtualization

[Roedel, Joerg]

On Wed, Nov 24, 2010 at 02:13:32PM -0500, Avi Kivity wrote:
> On 11/24/2010 08:18 PM, Joerg Roedel wrote:
> > Hi Avi, Hi Marcelo,
> >
> > here is a patch-set to make the instruction emulator aware of nested
> > virtualization. It basically works by introducing a new callback into
> > the x86_ops to check if a decoded instruction must be intercepted. If it
> > is intercepted the instruction emulator returns straight into the guest.
> >
> > I am not entirely happy with this solution because it partially
> > duplicates the code in the x86_emulate_insn function.
> 
> My big worry is that it makes svm.c aware of internal emulator variable, 
> so it makes it harder to hack on the emulator.

I don't think so, the structure of the code in svm.c follows the same
structures (even in a simpler way) as in the x86_emulate_insn()
function. Someone who changes the internal data structures of the
emulator can easily change svm.c too. This person will even recognize
the need for this automatically because svm.c will not compile anymore
when the data structure is changed.
On the other side, implementing this in the emulator itself would
require a person to learn about very low-level svm internals to get
everything right (or the changes easily break the code which is more
likely).

> So I don't think there's a problem with coding the svm intercepts in 
> emulate.c.  This is no different than emulating any AMD-specific 
> instruction in the emulator - we're emulating an instruction in exactly 
> the way it is specified in the manual.

That would make sense if the Nested-SVM code is implemented in the
generic code so that it is usable from VMX too. But that is not the case
and also not really doable.

> Something you could do is allocate bits for the intercept bit number and 
> exit code in opcode->flags.  This way most unconditional intercepts 
> happen outside the instruction switch: generic code reads the intercept 
> bit, the intercept word (via a callback), if the bit is set, returns the 
> exit code.  That should completely kill the diffstat.  We only need to 
> be careful wrt the order of the intercept check and the other permission 
> checks.

We have a lot of intercepts where this does not work. There is no 1-1
mapping between an opcode and an intercept. Some opcodes can result in
multiple different intercepts (mov cr, mov dr), sometimes multiple
intructions result in one intercept (rdmsr/wrmsr, in/out). The later
ones even need special handling because the differences between the
different instructions are encoded in the exit_info fields. All this
would expose svm-internals like the vmcb structure into the generic
code.
I think hacking all this in the emulator itself also makes it more
complex than it is today and the changes will likely break at some point
when somone hacks on the emulator. And the situation will not get better
when Nested-VMX gets merged and needs to do the same.

We basically have two choices here:

	a) We expose svm internals into the emulator
	b) We expose emulator internals into svm

Both choices are not really good from a software-design point-of-view.
But I think option b) is the better one because it is easier to cope with
and thus less likely to break when changing the emulator code.

	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 0/9] KVM: Make the instruction emulator aware of Nested Virtualization

[Roedel, Joerg]

On Thu, Nov 25, 2010 at 12:46:40PM +0100, Roedel, Joerg wrote:
> We basically have two choices here:
> 
> 	a) We expose svm internals into the emulator
> 	b) We expose emulator internals into svm
> 
> Both choices are not really good from a software-design point-of-view.
> But I think option b) is the better one because it is easier to cope with
> and thus less likely to break when changing the emulator code.

What we could do probably is to define the interface between the
emulator and the architecture code in a better way. This would take the
burden of going into architecture code for emulator changes away.

The current patch-set only needs a subset of the decode-cache (in the
future probably also a subset of the fetch-cache). We could put this
information into a seperate struct and give it to the architecture code.

I planned to make the guest_mode flag a generic x86 vcpu property
anyway, so building this structure could be limited to instructions
emulated while the vcpu is in guest mode thus avoiding the overhead for
the default case.

	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 0/9] KVM: Make the instruction emulator aware of Nested Virtualization

[Avi Kivity]

On 11/25/2010 01:46 PM, Roedel, Joerg wrote:
> On Wed, Nov 24, 2010 at 02:13:32PM -0500, Avi Kivity wrote:
> >  On 11/24/2010 08:18 PM, Joerg Roedel wrote:
> >  >  Hi Avi, Hi Marcelo,
> >  >
> >  >  here is a patch-set to make the instruction emulator aware of nested
> >  >  virtualization. It basically works by introducing a new callback into
> >  >  the x86_ops to check if a decoded instruction must be intercepted. If it
> >  >  is intercepted the instruction emulator returns straight into the guest.
> >  >
> >  >  I am not entirely happy with this solution because it partially
> >  >  duplicates the code in the x86_emulate_insn function.
> >
> >  My big worry is that it makes svm.c aware of internal emulator variable,
> >  so it makes it harder to hack on the emulator.
>
> I don't think so, the structure of the code in svm.c follows the same
> structures (even in a simpler way) as in the x86_emulate_insn()
> function. Someone who changes the internal data structures of the
> emulator can easily change svm.c too. This person will even recognize
> the need for this automatically because svm.c will not compile anymore
> when the data structure is changed.

Eventually the emulator will be used outside kvm.  We don't want to tie 
the two together.

> On the other side, implementing this in the emulator itself would
> require a person to learn about very low-level svm internals to get
> everything right (or the changes easily break the code which is more
> likely).

All that's needed is to read the svm chapter in the AMD manual; you 
don't need to understand kvm or out nested svm implementation.  On the 
other hand, some information needs to be encoded in the emulator (the 
order of the intercept check vs exception check) or we need to duplicate 
checks.  We also do a split decode.

> >  So I don't think there's a problem with coding the svm intercepts in
> >  emulate.c.  This is no different than emulating any AMD-specific
> >  instruction in the emulator - we're emulating an instruction in exactly
> >  the way it is specified in the manual.
>
> That would make sense if the Nested-SVM code is implemented in the
> generic code so that it is usable from VMX too. But that is not the case
> and also not really doable.

Nested VMX could do the same thing.  Sometimes the checks would be 
shared and sometimes not.

> >  Something you could do is allocate bits for the intercept bit number and
> >  exit code in opcode->flags.  This way most unconditional intercepts
> >  happen outside the instruction switch: generic code reads the intercept
> >  bit, the intercept word (via a callback), if the bit is set, returns the
> >  exit code.  That should completely kill the diffstat.  We only need to
> >  be careful wrt the order of the intercept check and the other permission
> >  checks.
>
> We have a lot of intercepts where this does not work. There is no 1-1
> mapping between an opcode and an intercept. Some opcodes can result in
> multiple different intercepts (mov cr, mov dr),

We can extend the group mechanism to make these separate opcodes.

>   sometimes multiple
> intructions result in one intercept (rdmsr/wrmsr, in/out). The later
> ones even need special handling because the differences between the
> different instructions are encoded in the exit_info fields.

So they get special treatment.  Decode bits are for the general case.

Let's see:

   CRx/DRx checks - need group mechanism extension, can use decode bits
   Selective CR0 - special
   LIDT/SIDT/LGDT/SGDT/LLDT/SLDT/LTR/STR - decode bits
   RDTSC/RDPMC/CPUID - decode bits
   PUSHF/POPF/RSM/IRET/INTn - decode bits, + flag to check before exceptions
   INVD /HLT/INVLPG/INVLPGA - decode bits
   PAUSE - special
   VMRUN/VMLOAD/VMSAVE/VMMCALL/STGI/CLGI/SKINIT - decode bits (VMMCALL 
preempts exceptions)
   RDTSCP/ICEBP/WBINVD/MONITOR/MWAIT - decode bits
   IOIO/MSR - very special
   Exception intercepts - outside emulator

So the majority (by far) can be handled by decode bits.  Selective CR0, 
IOIO, MSR, and PAUSE need special handling, can be done via callbacks 
into kvm (and into vendor specific code).  These will be useful for 
nested vmx as well.

Come to think of it, CR0, IOIO, and MSR already have callbacks into 
kvm.  So all we need to do is add X86EMUL_INTERCEPTED to the callback 
(provided it's at the right place in terms of intercept/exception 
priority - haven't checked).

>   All this
> would expose svm-internals like the vmcb structure into the generic
> code.
> I think hacking all this in the emulator itself also makes it more
> complex than it is today and the changes will likely break at some point
> when somone hacks on the emulator. And the situation will not get better
> when Nested-VMX gets merged and needs to do the same.
>
> We basically have two choices here:
>
> 	a) We expose svm internals into the emulator
> 	b) We expose emulator internals into svm
>
> Both choices are not really good from a software-design point-of-view.
> But I think option b) is the better one because it is easier to cope with
> and thus less likely to break when changing the emulator code.

svm specific infomation will have to be exposed anyway, because the 
checks need to be made in different places.  That's especially true when 
the emulation itself can generate exceptions, you may have to redo the 
exception check in svm.c.

-- 

error compiling committee.c: too many arguments to function

Re: [PATCH 0/9] KVM: Make the instruction emulator aware of Nested Virtualization

[Avi Kivity]

On 11/25/2010 03:13 PM, Roedel, Joerg wrote:
> On Thu, Nov 25, 2010 at 12:46:40PM +0100, Roedel, Joerg wrote:
> >  We basically have two choices here:
> >
> >  	a) We expose svm internals into the emulator
> >  	b) We expose emulator internals into svm
> >
> >  Both choices are not really good from a software-design point-of-view.
> >  But I think option b) is the better one because it is easier to cope with
> >  and thus less likely to break when changing the emulator code.
>
> What we could do probably is to define the interface between the
> emulator and the architecture code in a better way. This would take the
> burden of going into architecture code for emulator changes away.

What about things like adding instructions and forgetting to add the 
corresponding svm.c code?

Of course it can happen with everything in the emulator, but at least 
there's a chance you will see the decode bits in nearby instructions.

> The current patch-set only needs a subset of the decode-cache (in the
> future probably also a subset of the fetch-cache). We could put this
> information into a seperate struct and give it to the architecture code.
>
> I planned to make the guest_mode flag a generic x86 vcpu property
> anyway, so building this structure could be limited to instructions
> emulated while the vcpu is in guest mode thus avoiding the overhead for
> the default case.
>

Good idea.  Needed for the decode bits thing as well.

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

Re: [PATCH 0/9] KVM: Make the instruction emulator aware of Nested Virtualization

[Roedel, Joerg]

On Thu, Nov 25, 2010 at 10:17:53AM -0500, Avi Kivity wrote:
> On 11/25/2010 03:13 PM, Roedel, Joerg wrote:
> > On Thu, Nov 25, 2010 at 12:46:40PM +0100, Roedel, Joerg wrote:
> > >  We basically have two choices here:
> > >
> > >  	a) We expose svm internals into the emulator
> > >  	b) We expose emulator internals into svm
> > >
> > >  Both choices are not really good from a software-design point-of-view.
> > >  But I think option b) is the better one because it is easier to cope with
> > >  and thus less likely to break when changing the emulator code.
> >
> > What we could do probably is to define the interface between the
> > emulator and the architecture code in a better way. This would take the
> > burden of going into architecture code for emulator changes away.
> 
> What about things like adding instructions and forgetting to add the 
> corresponding svm.c code?

Cannot happen. Every instruction that can be intercepted with SVM is
already handled in this patch-set.

> Good idea.  Needed for the decode bits thing as well.

Especially needed to not kill the L1 guest when an L2 instruction
emulation fails :)

	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 0/9] KVM: Make the instruction emulator aware of Nested Virtualization

[Roedel, Joerg]

On Thu, Nov 25, 2010 at 10:15:43AM -0500, Avi Kivity wrote:
> On 11/25/2010 01:46 PM, Roedel, Joerg wrote:

> Eventually the emulator will be used outside kvm.  We don't want to tie 
> the two together.

Does any user outside of KVM care about nested virtualization?

> All that's needed is to read the svm chapter in the AMD manual; you 
> don't need to understand kvm or out nested svm implementation.  On the 
> other hand, some information needs to be encoded in the emulator (the 
> order of the intercept check vs exception check) or we need to duplicate 
> checks.  We also do a split decode.

Is that person also required to read through the 500 pages of VMX
documentation when nested VMX gets merged?

> So they get special treatment.  Decode bits are for the general case.
> 
> Let's see:
> 
>    CRx/DRx checks - need group mechanism extension, can use decode bits

The CRx writes are mostly special because exceptions for validity of the
values written take precedence over the intercept. Implementing these
checks also requires to put the intercept check into the kvm_set_crX
functions, which, by themselves, needs to be reworked in an SVM specific
way for this.

>    Selective CR0 - special

Needs to be handled in the write-cr0 path

>    LIDT/SIDT/LGDT/SGDT/LLDT/SLDT/LTR/STR - decode bits

Check for a valid address before the intercept check. Thus special too.

>    RDTSC/RDPMC/CPUID - decode bits

RDTSC and RDPMC check all exceptions before the intercept too.

>    PUSHF/POPF/RSM/IRET/INTn - decode bits, + flag to check before exceptions

Should work with decode-bits.

>    INVD /HLT/INVLPG/INVLPGA - decode bits

Exceptions are only caused on cpl > 0 and take precedence over the
intercept. Should work with decode bits.


>    VMRUN/VMLOAD/VMSAVE/VMMCALL/STGI/CLGI/SKINIT - decode bits (VMMCALL 
> preempts exceptions)

VMRUN/VMLOAD/VMSAVE need to check rax for a valid physical address
before the intercept is taken. All SVM instructions are not allowed in
real-mode which needs to be checkd too. The realmode-check may be
generic but with the address check this is harder. So at least
VMRUN/VMLOAD/VMSAVE are special too.

Further the SVM instructions are not implemented in the emulator at all
(like some other instructions which can be intercepted). Proper
emulation of these instructions would require new callbacks.

>    RDTSCP/ICEBP/WBINVD/MONITOR/MWAIT - decode bits

RDTSCP needs special handling like RDTSC. MONITOR is special too because
it checks all exceptions before the intercept.

All this can be done, but I doubt the result will look better or is
better maintainable than the current the solution in this patch-set.

	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 0/9] KVM: Make the instruction emulator aware of Nested Virtualization

[Avi Kivity]

On 11/25/2010 08:21 PM, Roedel, Joerg wrote:
> On Thu, Nov 25, 2010 at 10:15:43AM -0500, Avi Kivity wrote:
> >  On 11/25/2010 01:46 PM, Roedel, Joerg wrote:
>
> >  Eventually the emulator will be used outside kvm.  We don't want to tie
> >  the two together.
>
> Does any user outside of KVM care about nested virtualization?

No.

> >  All that's needed is to read the svm chapter in the AMD manual; you
> >  don't need to understand kvm or out nested svm implementation.  On the
> >  other hand, some information needs to be encoded in the emulator (the
> >  order of the intercept check vs exception check) or we need to duplicate
> >  checks.  We also do a split decode.
>
> Is that person also required to read through the 500 pages of VMX
> documentation when nested VMX gets merged?

Yes.

> >  So they get special treatment.  Decode bits are for the general case.
> >
> >  Let's see:
> >
> >     CRx/DRx checks - need group mechanism extension, can use decode bits
>
> The CRx writes are mostly special because exceptions for validity of the
> values written take precedence over the intercept.

We can have three checks, controlled by the decode bits:

     // decode instruction

     if ((c->d & SvmMask) == SvmInterceptBefore)
           ... do intercept check

     // do privivilge level checks

     if ((c->d & SvmMask) == SvmInterceptAfterPriv)
           ... do intercept check

     // fetch operands

     if ((c->d & SvmMask) == SvmInterceptAfterMemory)
           ... do intercept check


>   Implementing these
> checks also requires to put the intercept check into the kvm_set_crX
> functions, which, by themselves, needs to be reworked in an SVM specific
> way for this.

Add a kvm_x86_ops callback for this (vmx as usual is pretty complicated 
here)

> >     Selective CR0 - special
>
> Needs to be handled in the write-cr0 path

In the appropriate callback

> >     LIDT/SIDT/LGDT/SGDT/LLDT/SLDT/LTR/STR - decode bits
>
> Check for a valid address before the intercept check. Thus special too.

See above - we can regularize it by encoding where the check takes place.

> >     RDTSC/RDPMC/CPUID - decode bits
>
> RDTSC and RDPMC check all exceptions before the intercept too.
>
> >     PUSHF/POPF/RSM/IRET/INTn - decode bits, + flag to check before exceptions
>
> Should work with decode-bits.
>
> >     INVD /HLT/INVLPG/INVLPGA - decode bits
>
> Exceptions are only caused on cpl>  0 and take precedence over the
> intercept. Should work with decode bits.
>
>
> >     VMRUN/VMLOAD/VMSAVE/VMMCALL/STGI/CLGI/SKINIT - decode bits (VMMCALL
> >  preempts exceptions)
>
> VMRUN/VMLOAD/VMSAVE need to check rax for a valid physical address
> before the intercept is taken.

Add an SrcPhys/DstPhys decode, it becomes regular.

> All SVM instructions are not allowed in
> real-mode which needs to be checkd too. The realmode-check may be
> generic but with the address check this is harder. So at least
> VMRUN/VMLOAD/VMSAVE are special too.
>
> Further the SVM instructions are not implemented in the emulator at all
> (like some other instructions which can be intercepted). Proper
> emulation of these instructions would require new callbacks.

Sure.

> >     RDTSCP/ICEBP/WBINVD/MONITOR/MWAIT - decode bits
>
> RDTSCP needs special handling like RDTSC.

Why?

> MONITOR is special too because
> it checks all exceptions before the intercept.
>
> All this can be done, but I doubt the result will look better or is
> better maintainable than the current the solution in this patch-set.

With proper infrastructure I think all the modifications needed will be 
the three checks above and the decode bits (assuming the current 
crx/drx/pio callbacks are in the right place).

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

Re: [PATCH 0/9] KVM: Make the instruction emulator aware of Nested Virtualization

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 513 bytes --]

(Sorry for late reply...)

On Thu, 25 Nov 2010 17:23:13 +0100, "Roedel, Joerg" said:
> On Thu, Nov 25, 2010 at 10:17:53AM -0500, Avi Kivity wrote:
> > On 11/25/2010 03:13 PM, Roedel, Joerg wrote:
> > What about things like adding instructions and forgetting to add the 
> > corresponding svm.c code?
> Cannot happen. Every instruction that can be intercepted with SVM is
> already handled in this patch-set.

Call us back when Intel releases the i9 and i11 with new instructions
that need intercept handling. ;)


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 0/9] KVM: Make the instruction emulator aware of Nested Virtualization

[Joerg Roedel]

On Mon, Nov 29, 2010 at 12:23:38PM -0500, Valdis.Kletnieks@vt.edu wrote:
> (Sorry for late reply...)
> 
> On Thu, 25 Nov 2010 17:23:13 +0100, "Roedel, Joerg" said:
> > On Thu, Nov 25, 2010 at 10:17:53AM -0500, Avi Kivity wrote:
> > > On 11/25/2010 03:13 PM, Roedel, Joerg wrote:
> > > What about things like adding instructions and forgetting to add the 
> > > corresponding svm.c code?
> > Cannot happen. Every instruction that can be intercepted with SVM is
> > already handled in this patch-set.
> 
> Call us back when Intel releases the i9 and i11 with new instructions
> that need intercept handling. ;)

How does that affect SVM?

Re: [PATCH 0/9] KVM: Make the instruction emulator aware of Nested Virtualization

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 959 bytes --]

On Mon, 29 Nov 2010 19:32:12 +0100, Joerg Roedel said:
> On Mon, Nov 29, 2010 at 12:23:38PM -0500, Valdis.Kletnieks@vt.edu wrote:
> > (Sorry for late reply...)
> > 
> > On Thu, 25 Nov 2010 17:23:13 +0100, "Roedel, Joerg" said:
> > > On Thu, Nov 25, 2010 at 10:17:53AM -0500, Avi Kivity wrote:
> > > > On 11/25/2010 03:13 PM, Roedel, Joerg wrote:
> > > > What about things like adding instructions and forgetting to add the 
> > > > corresponding svm.c code?
> > > Cannot happen. Every instruction that can be intercepted with SVM is
> > > already handled in this patch-set.
> > 
> > Call us back when Intel releases the i9 and i11 with new instructions
> > that need intercept handling. ;)
> 
> How does that affect SVM?

It will quite possibly include instructions that can be intercepted with SVM
that are not in this patch set.  At which point Joerg's comment can apply - it
will be possible to add it in one place and forget to add it in the svm.c code.


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 0/9] KVM: Make the instruction emulator aware of Nested Virtualization

[Roedel, Joerg]

On Mon, Nov 29, 2010 at 03:01:10PM -0500, Valdis.Kletnieks@vt.edu wrote:
> On Mon, 29 Nov 2010 19:32:12 +0100, Joerg Roedel said:
> > On Mon, Nov 29, 2010 at 12:23:38PM -0500, Valdis.Kletnieks@vt.edu wrote:
> > > (Sorry for late reply...)
> > > 
> > > On Thu, 25 Nov 2010 17:23:13 +0100, "Roedel, Joerg" said:
> > > > On Thu, Nov 25, 2010 at 10:17:53AM -0500, Avi Kivity wrote:
> > > > > On 11/25/2010 03:13 PM, Roedel, Joerg wrote:
> > > > > What about things like adding instructions and forgetting to add the 
> > > > > corresponding svm.c code?
> > > > Cannot happen. Every instruction that can be intercepted with SVM is
> > > > already handled in this patch-set.
> > > 
> > > Call us back when Intel releases the i9 and i11 with new instructions
> > > that need intercept handling. ;)
> > 
> > How does that affect SVM?
> 
> It will quite possibly include instructions that can be intercepted with SVM
> that are not in this patch set.  At which point Joerg's comment can apply - it
> will be possible to add it in one place and forget to add it in the svm.c code.

SVM is AMD-only. So if an instruction does not exist on AMD there will
also be no specific intercept for it. For newly added instructions to
the AMD ISA which can then be intercepted I have to do bringup work
anyway. This will include adding these intercepts to the code in this
patch-set.

	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