Re: [PATCH v2 05/30] KVM: Provide callback to get/set control registers in emulator ops.

[Gleb Natapov <gleb@redhat.com>]

On Mon, Mar 15, 2010 at 02:06:48PM +0100, Andre Przywara wrote:
> Gleb Natapov wrote:
> >Use this callback instead of directly call kvm function. Also rename
> >realmode_(set|get)_cr to emulator_(set|get)_cr since function has nothing
> >to do with real mode.
> Do you mind removing the static before emulator_{set,get}_cr and
I don't, but this is not the goal of this patch series.

> marking it EXPORT_SYMBOL? Then one could use it in vmx.c (and soon
> in svm.c ;-) while handling MOV-CR intercepts. Currently most of the
> code is actually duplicated.
> 
> Also, shouldn't mk_cr_64() not be called mask_cr_64() for better
> readability?
This is how it is called now, the patch only moves it. But this code
will be reworked by later patches anyway since functions called from
emulator should not inject exceptions behind emulator's back.

> 
> Regards,
> Andre.
> 
> >Signed-off-by: Gleb Natapov <gleb@redhat.com>
> >---
> > arch/x86/include/asm/kvm_emulate.h |    3 +-
> > arch/x86/include/asm/kvm_host.h    |    2 -
> > arch/x86/kvm/emulate.c             |    7 +-
> > arch/x86/kvm/x86.c                 |  114 ++++++++++++++++++------------------
> > 4 files changed, 63 insertions(+), 63 deletions(-)
> >
> >diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
> >index 2666d7a..0c5caa4 100644
> >--- a/arch/x86/include/asm/kvm_emulate.h
> >+++ b/arch/x86/include/asm/kvm_emulate.h
> >@@ -108,7 +108,8 @@ struct x86_emulate_ops {
> > 				const void *new,
> > 				unsigned int bytes,
> > 				struct kvm_vcpu *vcpu);
> >-
> >+	ulong (*get_cr)(int cr, struct kvm_vcpu *vcpu);
> >+	void (*set_cr)(int cr, ulong val, struct kvm_vcpu *vcpu);
> > };
> > /* Type, address-of, and value of an instruction's operand. */
> >diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> >index 3b178d8..e8e108a 100644
> >--- a/arch/x86/include/asm/kvm_host.h
> >+++ b/arch/x86/include/asm/kvm_host.h
> >@@ -585,8 +585,6 @@ void realmode_lidt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);
> > void realmode_lmsw(struct kvm_vcpu *vcpu, unsigned long msw,
> > 		   unsigned long *rflags);
> >-unsigned long realmode_get_cr(struct kvm_vcpu *vcpu, int cr);
> >-void realmode_set_cr(struct kvm_vcpu *vcpu, int cr, unsigned long value);
> > void kvm_enable_efer_bits(u64);
> > int kvm_get_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *data);
> > int kvm_set_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data);
> >diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> >index 91450b5..5b060e4 100644
> >--- a/arch/x86/kvm/emulate.c
> >+++ b/arch/x86/kvm/emulate.c
> >@@ -2483,7 +2483,7 @@ twobyte_insn:
> > 			break;
> > 		case 4: /* smsw */
> > 			c->dst.bytes = 2;
> >-			c->dst.val = realmode_get_cr(ctxt->vcpu, 0);
> >+			c->dst.val = ops->get_cr(0, ctxt->vcpu);
> > 			break;
> > 		case 6: /* lmsw */
> > 			realmode_lmsw(ctxt->vcpu, (u16)c->src.val,
> >@@ -2519,8 +2519,7 @@ twobyte_insn:
> > 	case 0x20: /* mov cr, reg */
> > 		if (c->modrm_mod != 3)
> > 			goto cannot_emulate;
> >-		c->regs[c->modrm_rm] =
> >-				realmode_get_cr(ctxt->vcpu, c->modrm_reg);
> >+		c->regs[c->modrm_rm] = ops->get_cr(c->modrm_reg, ctxt->vcpu);
> > 		c->dst.type = OP_NONE;	/* no writeback */
> > 		break;
> > 	case 0x21: /* mov from dr to reg */
> >@@ -2534,7 +2533,7 @@ twobyte_insn:
> > 	case 0x22: /* mov reg, cr */
> > 		if (c->modrm_mod != 3)
> > 			goto cannot_emulate;
> >-		realmode_set_cr(ctxt->vcpu, c->modrm_reg, c->modrm_val);
> >+		ops->set_cr(c->modrm_reg, c->modrm_val, ctxt->vcpu);
> > 		c->dst.type = OP_NONE;
> > 		break;
> > 	case 0x23: /* mov from reg to dr */
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index a1e671a..bf714df 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -3370,12 +3370,70 @@ void kvm_report_emulation_failure(struct kvm_vcpu *vcpu, const char *context)
> > }
> > EXPORT_SYMBOL_GPL(kvm_report_emulation_failure);
> >+static u64 mk_cr_64(u64 curr_cr, u32 new_val)
> >+{
> >+	return (curr_cr & ~((1ULL << 32) - 1)) | new_val;
> >+}
> >+
> >+static unsigned long emulator_get_cr(int cr, struct kvm_vcpu *vcpu)
> >+{
> >+	unsigned long value;
> >+
> >+	switch (cr) {
> >+	case 0:
> >+		value = kvm_read_cr0(vcpu);
> >+		break;
> >+	case 2:
> >+		value = vcpu->arch.cr2;
> >+		break;
> >+	case 3:
> >+		value = vcpu->arch.cr3;
> >+		break;
> >+	case 4:
> >+		value = kvm_read_cr4(vcpu);
> >+		break;
> >+	case 8:
> >+		value = kvm_get_cr8(vcpu);
> >+		break;
> >+	default:
> >+		vcpu_printf(vcpu, "%s: unexpected cr %u\n", __func__, cr);
> >+		return 0;
> >+	}
> >+
> >+	return value;
> >+}
> >+
> >+static void emulator_set_cr(int cr, unsigned long val, struct kvm_vcpu *vcpu)
> >+{
> >+	switch (cr) {
> >+	case 0:
> >+		kvm_set_cr0(vcpu, mk_cr_64(kvm_read_cr0(vcpu), val));
> >+		break;
> >+	case 2:
> >+		vcpu->arch.cr2 = val;
> >+		break;
> >+	case 3:
> >+		kvm_set_cr3(vcpu, val);
> >+		break;
> >+	case 4:
> >+		kvm_set_cr4(vcpu, mk_cr_64(kvm_read_cr4(vcpu), val));
> >+		break;
> >+	case 8:
> >+		kvm_set_cr8(vcpu, val & 0xfUL);
> >+		break;
> >+	default:
> >+		vcpu_printf(vcpu, "%s: unexpected cr %u\n", __func__, cr);
> >+	}
> >+}
> >+
> > static struct x86_emulate_ops emulate_ops = {
> > 	.read_std            = kvm_read_guest_virt_system,
> > 	.fetch               = kvm_fetch_guest_virt,
> > 	.read_emulated       = emulator_read_emulated,
> > 	.write_emulated      = emulator_write_emulated,
> > 	.cmpxchg_emulated    = emulator_cmpxchg_emulated,
> >+	.get_cr              = emulator_get_cr,
> >+	.set_cr              = emulator_set_cr,
> > };
> > static void cache_all_regs(struct kvm_vcpu *vcpu)
> >@@ -3973,11 +4031,6 @@ int kvm_fix_hypercall(struct kvm_vcpu *vcpu)
> > 	return emulator_write_emulated(rip, instruction, 3, vcpu);
> > }
> >-static u64 mk_cr_64(u64 curr_cr, u32 new_val)
> >-{
> >-	return (curr_cr & ~((1ULL << 32) - 1)) | new_val;
> >-}
> >-
> > void realmode_lgdt(struct kvm_vcpu *vcpu, u16 limit, unsigned long base)
> > {
> > 	struct desc_ptr dt = { limit, base };
> >@@ -3999,57 +4052,6 @@ void realmode_lmsw(struct kvm_vcpu *vcpu, unsigned long msw,
> > 	*rflags = kvm_get_rflags(vcpu);
> > }
> >-unsigned long realmode_get_cr(struct kvm_vcpu *vcpu, int cr)
> >-{
> >-	unsigned long value;
> >-
> >-	switch (cr) {
> >-	case 0:
> >-		value = kvm_read_cr0(vcpu);
> >-		break;
> >-	case 2:
> >-		value = vcpu->arch.cr2;
> >-		break;
> >-	case 3:
> >-		value = vcpu->arch.cr3;
> >-		break;
> >-	case 4:
> >-		value = kvm_read_cr4(vcpu);
> >-		break;
> >-	case 8:
> >-		value = kvm_get_cr8(vcpu);
> >-		break;
> >-	default:
> >-		vcpu_printf(vcpu, "%s: unexpected cr %u\n", __func__, cr);
> >-		return 0;
> >-	}
> >-
> >-	return value;
> >-}
> >-
> >-void realmode_set_cr(struct kvm_vcpu *vcpu, int cr, unsigned long val)
> >-{
> >-	switch (cr) {
> >-	case 0:
> >-		kvm_set_cr0(vcpu, mk_cr_64(kvm_read_cr0(vcpu), val));
> >-		break;
> >-	case 2:
> >-		vcpu->arch.cr2 = val;
> >-		break;
> >-	case 3:
> >-		kvm_set_cr3(vcpu, val);
> >-		break;
> >-	case 4:
> >-		kvm_set_cr4(vcpu, mk_cr_64(kvm_read_cr4(vcpu), val));
> >-		break;
> >-	case 8:
> >-		kvm_set_cr8(vcpu, val & 0xfUL);
> >-		break;
> >-	default:
> >-		vcpu_printf(vcpu, "%s: unexpected cr %u\n", __func__, cr);
> >-	}
> >-}
> >-
> > static int move_to_next_stateful_cpuid_entry(struct kvm_vcpu *vcpu, int i)
> > {
> > 	struct kvm_cpuid_entry2 *e = &vcpu->arch.cpuid_entries[i];
> 
> 
> -- 
> Andre Przywara
> AMD-OSRC (Dresden)
> Tel: x29712

--
			Gleb.