Skip to contentSkip to conversation

KVM: x86: accessors for guest registers

PATCH Marcelo Tosatti 2 participants 4 messages Jun 27, 2008 – Jun 30, 2008

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Marcelo Tosatti

author
#0 lore raw

As suggested by Avi, introduce accessors to read/write guest registers. This simplifies the ->cache_regs/->decache_regs interface, and improves register caching which is important for VMX, where the cost of vmcs_read/vmcs_write is significant.

Signed-off-by Marcelo Tosatti <[email protected]>
arch/x86/kvm/kvm_cache_regs.h new file +36 -0
@@ -0,0 +1,36 @@
+#ifndef ASM_KVM_CACHE_REGS_H
+#define ASM_KVM_CACHE_REGS_H
+
+static inline unsigned long kvm_register_read(struct kvm_vcpu *vcpu,
+					      enum kvm_reg reg)
+{
+	if (!__test_and_set_bit(reg, &vcpu->arch.regs_available))
+		kvm_x86_ops->cache_reg(vcpu, reg);
+
+	return vcpu->arch.regs[reg];
+}
+
+static inline void kvm_register_write(struct kvm_vcpu *vcpu,
+				      enum kvm_reg reg,
+				      unsigned long val)
+{
+	vcpu->arch.regs[reg] = val;
+	__set_bit(reg, &vcpu->arch.regs_dirty);
+}
+
+static inline unsigned long kvm_rip_read(struct kvm_vcpu *vcpu)
+{
+	if (!__test_and_set_bit(VCPU_REGS_RIP, &vcpu->arch.regs_available))
+		kvm_x86_ops->cache_reg(vcpu, VCPU_REGS_RIP);
+
+	return vcpu->arch.rip;
+}
+
+static inline void kvm_rip_write(struct kvm_vcpu *vcpu,
+			 	 unsigned long val)
+{
+	vcpu->arch.rip = val;
+	__set_bit(VCPU_REGS_RIP, &vcpu->arch.regs_dirty);
+}
+
+#endif
arch/x86/kvm/lapic.c +2 -2
index 73f43de..9fde0ac 100644
@@ -32,6 +32,7 @@
 #include <asm/current.h>
 #include <asm/apicdef.h>
 #include <asm/atomic.h>
+#include "kvm_cache_regs.h"
 #include "irq.h"
 
 #define PRId64 "d"

@@ -558,8 +559,7 @@ static void __report_tpr_access(struct kvm_lapic *apic, bool write)
 	struct kvm_run *run = vcpu->run;
 
 	set_bit(KVM_REQ_REPORT_TPR_ACCESS, &vcpu->requests);
-	kvm_x86_ops->cache_regs(vcpu);
-	run->tpr_access.rip = vcpu->arch.rip;
+	run->tpr_access.rip = kvm_rip_read(vcpu);
 	run->tpr_access.is_write = write;
 }
arch/x86/kvm/svm.c +52 -11

112 lines not on this page · raw message

arch/x86/kvm/vmx.c +61 -49

235 lines not on this page · raw message

arch/x86/kvm/x86.c +143 -124

450 lines not on this page · raw message

arch/x86/kvm/x86_emulate.c +10 -9
index 38926b7..64fe207 100644
@@ -26,6 +26,7 @@
 #define DPRINTF(_f, _a ...) printf(_f , ## _a)
 #else
 #include <linux/kvm_host.h>
+#include "kvm_cache_regs.h"
 #define DPRINTF(x...) do {} while (0)
 #endif
 #include <linux/module.h>

@@ -806,7 +807,7 @@ x86_decode_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 	/* Shadow copy of register state. Committed on successful emulation. */
 
 	memset(c, 0, sizeof(struct decode_cache));
-	c->eip = ctxt->vcpu->arch.rip;
+	c->eip = kvm_rip_read(ctxt->vcpu);
 	memcpy(c->regs, ctxt->vcpu->arch.regs, sizeof c->regs);
 
 	switch (mode) {

@@ -1245,7 +1246,7 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 	if (c->rep_prefix && (c->d & String)) {
 		/* All REP prefixes have the same first termination condition */
 		if (c->regs[VCPU_REGS_RCX] == 0) {
-			ctxt->vcpu->arch.rip = c->eip;
+			kvm_rip_write(ctxt->vcpu, c->eip);
 			goto done;
 		}
 		/* The second termination condition only applies for REPE

@@ -1259,17 +1260,17 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 				(c->b == 0xae) || (c->b == 0xaf)) {
 			if ((c->rep_prefix == REPE_PREFIX) &&
 				((ctxt->eflags & EFLG_ZF) == 0)) {
-					ctxt->vcpu->arch.rip = c->eip;
+					kvm_rip_write(ctxt->vcpu, c->eip);
 					goto done;
 			}
 			if ((c->rep_prefix == REPNE_PREFIX) &&
 				((ctxt->eflags & EFLG_ZF) == EFLG_ZF)) {
-				ctxt->vcpu->arch.rip = c->eip;
+				kvm_rip_write(ctxt->vcpu, c->eip);
 				goto done;
 			}
 		}
 		c->regs[VCPU_REGS_RCX]--;
-		c->eip = ctxt->vcpu->arch.rip;
+		c->eip = kvm_rip_read(ctxt->vcpu);
 	}
 
 	if (c->src.type == OP_MEM) {

@@ -1750,7 +1751,7 @@ writeback:
 
 	/* Commit shadow register state. */
 	memcpy(ctxt->vcpu->arch.regs, c->regs, sizeof c->regs);
-	ctxt->vcpu->arch.rip = c->eip;
+	kvm_rip_write(ctxt->vcpu, c->eip);
 
 done:
 	if (rc == X86EMUL_UNHANDLEABLE) {

@@ -1775,7 +1776,7 @@ twobyte_insn:
 				goto done;
 
 			/* Let the processor re-execute the fixed hypercall */
-			c->eip = ctxt->vcpu->arch.rip;
+			c->eip = kvm_rip_read(ctxt->vcpu);
 			/* Disable writeback. */
 			c->dst.type = OP_NONE;
 			break;

@@ -1871,7 +1872,7 @@ twobyte_insn:
 		rc = kvm_set_msr(ctxt->vcpu, c->regs[VCPU_REGS_RCX], msr_data);
 		if (rc) {
 			kvm_inject_gp(ctxt->vcpu, 0);
-			c->eip = ctxt->vcpu->arch.rip;
+			c->eip = kvm_rip_read(ctxt->vcpu);
 		}
 		rc = X86EMUL_CONTINUE;
 		c->dst.type = OP_NONE;

@@ -1881,7 +1882,7 @@ twobyte_insn:
 		rc = kvm_get_msr(ctxt->vcpu, c->regs[VCPU_REGS_RCX], &msr_data);
 		if (rc) {
 			kvm_inject_gp(ctxt->vcpu, 0);
-			c->eip = ctxt->vcpu->arch.rip;
+			c->eip = kvm_rip_read(ctxt->vcpu);
 		} else {
 			c->regs[VCPU_REGS_RAX] = (u32)msr_data;
 			c->regs[VCPU_REGS_RDX] = msr_data >> 32;
include/asm-x86/kvm_host.h +13 -5
index 851184d..515add9 100644
@@ -87,7 +87,7 @@ extern struct list_head vm_list;
 struct kvm_vcpu;
 struct kvm;
 
-enum {
+enum kvm_reg {
 	VCPU_REGS_RAX = 0,
 	VCPU_REGS_RCX = 1,
 	VCPU_REGS_RDX = 2,

@@ -109,6 +109,8 @@ enum {
 	NR_VCPU_REGS
 };
 
+#define VCPU_REGS_RIP NR_VCPU_REGS
+
 enum {
 	VCPU_SREG_ES,
 	VCPU_SREG_CS,

@@ -217,8 +219,14 @@ struct kvm_vcpu_arch {
 	int interrupt_window_open;
 	unsigned long irq_summary; /* bit vector: 1 per word in irq_pending */
 	DECLARE_BITMAP(irq_pending, KVM_NR_INTERRUPTS);
-	unsigned long regs[NR_VCPU_REGS]; /* for rsp: vcpu_load_rsp_rip() */
-	unsigned long rip;      /* needs vcpu_load_rsp_rip() */
+	/*
+ 	 * rip and regs accesses must go through
+ 	 * kvm_{register,rip}_{read,write} functions.
+ 	 */
+	unsigned long regs[NR_VCPU_REGS];
+	unsigned long rip;
+	u32 regs_available;
+	u32 regs_dirty;
 
 	unsigned long cr0;
 	unsigned long cr2;

@@ -410,8 +418,8 @@ struct kvm_x86_ops {
 	unsigned long (*get_dr)(struct kvm_vcpu *vcpu, int dr);
 	void (*set_dr)(struct kvm_vcpu *vcpu, int dr, unsigned long value,
 		       int *exception);
-	void (*cache_regs)(struct kvm_vcpu *vcpu);
-	void (*decache_regs)(struct kvm_vcpu *vcpu);
+	void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
+	void (*decache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
 	unsigned long (*get_rflags)(struct kvm_vcpu *vcpu);
 	void (*set_rflags)(struct kvm_vcpu *vcpu, unsigned long rflags);
#1 lore raw
Quoted 5 lines · Marcelo Tosatti

As suggested by Avi, introduce accessors to read/write guest registers. This simplifies the ->cache_regs/->decache_regs interface, and improves register caching which is important for VMX, where the cost of vmcs_read/vmcs_write is significant.

I made some changes, which I think simplify things:

- svm always caches registers, and all registers are dirty, since cache/decache is cheap
- kvm_register_write() sets the avail bit to avoid read-after-write corruption (like forwarding in a real cpu :)
- made rip a GPR in spite of my own objections, it does simplify things
- init avail and dirty bitmasks on cpu reset
- convert a couple mode GUEST_RIP references
- remove decache_all_regs
- inline decache_reg() into vmx/svm

How does the attached look?

Signature · 1 line
error compiling committee.c: too many arguments to function

commit f0971532697d33915fb83b6b465e2428b7676b68

Author: Marcelo Tosatti <[email protected]>
Date:   Fri Jun 27 14:58:02 2008 -0300

KVM: x86: accessors for guest registers

As suggested by Avi, introduce accessors to read/write guest registers. This simplifies the ->cache_regs/->decache_regs interface, and improves register caching which is important for VMX, where the cost of vmcs_read/vmcs_write is significant.

Signed-off-by Marcelo Tosatti <[email protected]>
Signed-off-by Avi Kivity <[email protected]>
arch/x86/kvm/kvm_cache_regs.h new file +32 -0
new file mode 100644
index 0000000..f21ba41
@@ -0,0 +1,32 @@
+#ifndef ASM_KVM_CACHE_REGS_H
+#define ASM_KVM_CACHE_REGS_H
+
+static inline unsigned long kvm_register_read(struct kvm_vcpu *vcpu,
+					      enum kvm_reg reg)
+{
+	if (!test_bit(reg, &vcpu->arch.regs_avail))
+		kvm_x86_ops->cache_reg(vcpu, reg);
+
+	return vcpu->arch.regs[reg];
+}
+
+static inline void kvm_register_write(struct kvm_vcpu *vcpu,
+				      enum kvm_reg reg,
+				      unsigned long val)
+{
+	vcpu->arch.regs[reg] = val;
+	__set_bit(reg, &vcpu->arch.regs_dirty);
+	__set_bit(reg, &vcpu->arch.regs_avail);
+}
+
+static inline unsigned long kvm_rip_read(struct kvm_vcpu *vcpu)
+{
+	return kvm_register_read(vcpu, VCPU_REGS_RIP);
+}
+
+static inline void kvm_rip_write(struct kvm_vcpu *vcpu, unsigned long val)
+{
+	kvm_register_write(vcpu, VCPU_REGS_RIP, val);
+}
+
+#endif
arch/x86/kvm/lapic.c +2 -2
index 73f43de..9fde0ac 100644
@@ -32,6 +32,7 @@
 #include <asm/current.h>
 #include <asm/apicdef.h>
 #include <asm/atomic.h>
+#include "kvm_cache_regs.h"
 #include "irq.h"
 
 #define PRId64 "d"

@@ -558,8 +559,7 @@ static void __report_tpr_access(struct kvm_lapic *apic, bool write)
 	struct kvm_run *run = vcpu->run;
 
 	set_bit(KVM_REQ_REPORT_TPR_ACCESS, &vcpu->requests);
-	kvm_x86_ops->cache_regs(vcpu);
-	run->tpr_access.rip = vcpu->arch.rip;
+	run->tpr_access.rip = kvm_rip_read(vcpu);
 	run->tpr_access.is_write = write;
 }
arch/x86/kvm/svm.c +12 -20
index 238e8f3..944a92a 100644
@@ -18,6 +18,7 @@
 #include "kvm_svm.h"
 #include "irq.h"
 #include "mmu.h"
+#include "kvm_cache_regs.h"
 
 #include <linux/module.h>
 #include <linux/kernel.h>

@@ -241,7 +242,8 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 		       svm->vmcb->save.rip,
 		       svm->next_rip);
 
-	vcpu->arch.rip = svm->vmcb->save.rip = svm->next_rip;
+	svm->vmcb->save.rip = svm->next_rip;
+	kvm_rip_write(vcpu, svm->vmcb->save.rip);
 	svm->vmcb->control.int_state &= ~SVM_INTERRUPT_SHADOW_MASK;
 
 	vcpu->arch.interrupt_window_open = 1;

@@ -607,6 +609,8 @@ static int svm_vcpu_reset(struct kvm_vcpu *vcpu)
 		svm->vmcb->save.cs.base = svm->vcpu.arch.sipi_vector << 12;
 		svm->vmcb->save.cs.selector = svm->vcpu.arch.sipi_vector << 8;
 	}
+	vcpu->arch.regs_avail = ~0;
+	vcpu->arch.regs_dirty = ~0;
 
 	return 0;
 }

@@ -709,23 +713,6 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu)
 	rdtscll(vcpu->arch.host_tsc);
 }
 
-static void svm_cache_regs(struct kvm_vcpu *vcpu)
-{
-	struct vcpu_svm *svm = to_svm(vcpu);
-
-	vcpu->arch.regs[VCPU_REGS_RAX] = svm->vmcb->save.rax;
-	vcpu->arch.regs[VCPU_REGS_RSP] = svm->vmcb->save.rsp;
-	vcpu->arch.rip = svm->vmcb->save.rip;
-}
-
-static void svm_decache_regs(struct kvm_vcpu *vcpu)
-{
-	struct vcpu_svm *svm = to_svm(vcpu);
-	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.rip;
-}
-
 static unsigned long svm_get_rflags(struct kvm_vcpu *vcpu)
 {
 	return to_svm(vcpu)->vmcb->save.rflags;

@@ -1695,6 +1682,10 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	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];
+
 	pre_svm_run(svm);
 
 	sync_lapic_to_cr8(vcpu);

@@ -1830,6 +1821,9 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 		load_db_regs(svm->host_db_regs);
 
 	vcpu->arch.cr2 = svm->vmcb->save.cr2;
+	vcpu->arch.regs[VCPU_REGS_RAX] = svm->vmcb->save.rax;
+	vcpu->arch.regs[VCPU_REGS_RSP] = svm->vmcb->save.rsp;
+	vcpu->arch.regs[VCPU_REGS_RIP] = svm->vmcb->save.rip;
 
 	write_dr6(svm->host_dr6);
 	write_dr7(svm->host_dr7);

@@ -1949,8 +1943,6 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.set_gdt = svm_set_gdt,
 	.get_dr = svm_get_dr,
 	.set_dr = svm_set_dr,
-	.cache_regs = svm_cache_regs,
-	.decache_regs = svm_decache_regs,
 	.get_rflags = svm_get_rflags,
 	.set_rflags = svm_set_rflags,
arch/x86/kvm/vmx.c +48 -54

232 lines not on this page · raw message

arch/x86/kvm/x86.c +133 -124

440 lines not on this page · raw message

arch/x86/kvm/x86_emulate.c +10 -9
index 18ca25c..dd4efe1 100644
@@ -26,6 +26,7 @@
 #define DPRINTF(_f, _a ...) printf(_f , ## _a)
 #else
 #include <linux/kvm_host.h>
+#include "kvm_cache_regs.h"
 #define DPRINTF(x...) do {} while (0)
 #endif
 #include <linux/module.h>

@@ -839,7 +840,7 @@ x86_decode_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 	/* Shadow copy of register state. Committed on successful emulation. */
 
 	memset(c, 0, sizeof(struct decode_cache));
-	c->eip = ctxt->vcpu->arch.rip;
+	c->eip = kvm_rip_read(ctxt->vcpu);
 	ctxt->cs_base = seg_base(ctxt, VCPU_SREG_CS);
 	memcpy(c->regs, ctxt->vcpu->arch.regs, sizeof c->regs);
 

@@ -1267,7 +1268,7 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 	if (c->rep_prefix && (c->d & String)) {
 		/* All REP prefixes have the same first termination condition */
 		if (c->regs[VCPU_REGS_RCX] == 0) {
-			ctxt->vcpu->arch.rip = c->eip;
+			kvm_rip_write(ctxt->vcpu, c->eip);
 			goto done;
 		}
 		/* The second termination condition only applies for REPE

@@ -1281,17 +1282,17 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 				(c->b == 0xae) || (c->b == 0xaf)) {
 			if ((c->rep_prefix == REPE_PREFIX) &&
 				((ctxt->eflags & EFLG_ZF) == 0)) {
-					ctxt->vcpu->arch.rip = c->eip;
+					kvm_rip_write(ctxt->vcpu, c->eip);
 					goto done;
 			}
 			if ((c->rep_prefix == REPNE_PREFIX) &&
 				((ctxt->eflags & EFLG_ZF) == EFLG_ZF)) {
-				ctxt->vcpu->arch.rip = c->eip;
+				kvm_rip_write(ctxt->vcpu, c->eip);
 				goto done;
 			}
 		}
 		c->regs[VCPU_REGS_RCX]--;
-		c->eip = ctxt->vcpu->arch.rip;
+		c->eip = kvm_rip_read(ctxt->vcpu);
 	}
 
 	if (c->src.type == OP_MEM) {

@@ -1768,7 +1769,7 @@ writeback:
 
 	/* Commit shadow register state. */
 	memcpy(ctxt->vcpu->arch.regs, c->regs, sizeof c->regs);
-	ctxt->vcpu->arch.rip = c->eip;
+	kvm_rip_write(ctxt->vcpu, c->eip);
 
 done:
 	if (rc == X86EMUL_UNHANDLEABLE) {

@@ -1793,7 +1794,7 @@ twobyte_insn:
 				goto done;
 
 			/* Let the processor re-execute the fixed hypercall */
-			c->eip = ctxt->vcpu->arch.rip;
+			c->eip = kvm_rip_read(ctxt->vcpu);
 			/* Disable writeback. */
 			c->dst.type = OP_NONE;
 			break;

@@ -1889,7 +1890,7 @@ twobyte_insn:
 		rc = kvm_set_msr(ctxt->vcpu, c->regs[VCPU_REGS_RCX], msr_data);
 		if (rc) {
 			kvm_inject_gp(ctxt->vcpu, 0);
-			c->eip = ctxt->vcpu->arch.rip;
+			c->eip = kvm_rip_read(ctxt->vcpu);
 		}
 		rc = X86EMUL_CONTINUE;
 		c->dst.type = OP_NONE;

@@ -1899,7 +1900,7 @@ twobyte_insn:
 		rc = kvm_get_msr(ctxt->vcpu, c->regs[VCPU_REGS_RCX], &msr_data);
 		if (rc) {
 			kvm_inject_gp(ctxt->vcpu, 0);
-			c->eip = ctxt->vcpu->arch.rip;
+			c->eip = kvm_rip_read(ctxt->vcpu);
 		} else {
 			c->regs[VCPU_REGS_RAX] = (u32)msr_data;
 			c->regs[VCPU_REGS_RDX] = msr_data >> 32;
include/asm-x86/kvm_host.h +10 -5
index c64d124..bae1b76 100644
@@ -88,7 +88,7 @@ extern struct list_head vm_list;
 struct kvm_vcpu;
 struct kvm;
 
-enum {
+enum kvm_reg {
 	VCPU_REGS_RAX = 0,
 	VCPU_REGS_RCX = 1,
 	VCPU_REGS_RDX = 2,

@@ -107,6 +107,7 @@ enum {
 	VCPU_REGS_R14 = 14,
 	VCPU_REGS_R15 = 15,
 #endif
+	VCPU_REGS_RIP,
 	NR_VCPU_REGS
 };
 

@@ -218,8 +219,13 @@ struct kvm_vcpu_arch {
 	int interrupt_window_open;
 	unsigned long irq_summary; /* bit vector: 1 per word in irq_pending */
 	DECLARE_BITMAP(irq_pending, KVM_NR_INTERRUPTS);
-	unsigned long regs[NR_VCPU_REGS]; /* for rsp: vcpu_load_rsp_rip() */
-	unsigned long rip;      /* needs vcpu_load_rsp_rip() */
+	/*
+	 * rip and regs accesses must go through
+	 * kvm_{register,rip}_{read,write} functions.
+	 */
+	unsigned long regs[NR_VCPU_REGS];
+	u32 regs_avail;
+	u32 regs_dirty;
 
 	unsigned long cr0;
 	unsigned long cr2;

@@ -412,8 +418,7 @@ struct kvm_x86_ops {
 	unsigned long (*get_dr)(struct kvm_vcpu *vcpu, int dr);
 	void (*set_dr)(struct kvm_vcpu *vcpu, int dr, unsigned long value,
 		       int *exception);
-	void (*cache_regs)(struct kvm_vcpu *vcpu);
-	void (*decache_regs)(struct kvm_vcpu *vcpu);
+	void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
 	unsigned long (*get_rflags)(struct kvm_vcpu *vcpu);
 	void (*set_rflags)(struct kvm_vcpu *vcpu, unsigned long rflags);
author
#2 lore raw
Quoted 8 lines · Avi Kivity
Quoted 5 lines · Marcelo Tosatti

As suggested by Avi, introduce accessors to read/write guest registers. This simplifies the ->cache_regs/->decache_regs interface, and improves register caching which is important for VMX, where the cost of vmcs_read/vmcs_write is significant.

I made some changes, which I think simplify things:

Stripped down, looks great.

Quoted 2 lines

- svm always caches registers, and all registers are dirty, since cache/decache is cheap

Accurate regs_dirty information is useful for converting the emulator, so that you can do something like:

emul_register_write(ctxt, reg, val)

{
    if (!__test_and_set_bit(reg, &ctxt->vcpu->regs_dirty))
        ctxt->original_regs[reg] = kvm_register_read(ctxt->vcpu, reg);
    ctxt->vcpu->regs[reg] = val;
}

Because restoring the original reg contents on failure is necessary. Otherwise you need to cache all regs on emulation entry. RIP is always read anyway, but RSP not so frequently.

Well, might not be worth the complexity for saving just one vmcs_read(). Or it can be changed later during conversion.

Quoted 2 lines

- kvm_register_write() sets the avail bit to avoid read-after-write corruption (like forwarding in a real cpu :)

Eek.

Quoted 10 lines

- made rip a GPR in spite of my own objections, it does simplify things
- init avail and dirty bitmasks on cpu reset
- convert a couple mode GUEST_RIP references
- remove decache_all_regs
- inline decache_reg() into vmx/svm

How does the attached look?

(quoted diff) +0 -2
-	.cache_regs = svm_cache_regs,
-	.decache_regs = svm_decache_regs,

Review

Quoted 10 lines
(quoted diff) +1 -1
 	.get_rflags = svm_get_rflags,
 	.set_rflags = svm_set_rflags,
 
 		if (io->in) {
 			r = pio_copy_data(vcpu);
 			if (r) {
-				kvm_x86_ops->cache_regs(vcpu);
+				kvm_x86_ops->cache_reg(vcpu, VCPU_REGS_RAX);
 				return r;
 			}

These two don't go well together. Apparently the intent of this ->cache_regs call on failure was to restore the original registers in case they were modified by pio_copy_data? But pio_copy_data does not write to any guest register (and even if it did, this ->cache_regs call assumes what registers are fetched from the guest's originals).

        kvm_x86_ops->cache_regs(vcpu);

        if (!io->string) {
                if (io->in)
                        memcpy(&vcpu->arch.regs[VCPU_REGS_RAX], vcpu->arch.pio_data,
                               io->size);
        } else {
                if (io->in) {
                        r = pio_copy_data(vcpu);
                        if (r) {
                                kvm_x86_ops->cache_regs(vcpu);
                                return r;
                        }
                }

Unless I'm mistaken you can just remove it.

#3 lore raw
Quoted 15 lines · Marcelo Tosatti
Quoted 4 lines

- svm always caches registers, and all registers are dirty, since cache/decache is cheap

Accurate regs_dirty information is useful for converting the emulator, so that you can do something like:

emul_register_write(ctxt, reg, val)

{
    if (!__test_and_set_bit(reg, &ctxt->vcpu->regs_dirty))
        ctxt->original_regs[reg] = kvm_register_read(ctxt->vcpu, reg);
    ctxt->vcpu->regs[reg] = val;
}
Quoted 8 lines

Because restoring the original reg contents on failure is necessary. Otherwise you need to cache all regs on emulation entry. RIP is always read anyway, but RSP not so frequently.

Well, might not be worth the complexity for saving just one vmcs_read(). Or it can be changed later during conversion.

That doesn't work, because some of the registers may already be dirty when the emulator is invoked (say, if we're emulating several instructions back-to-back). I think the best way to change the emulator is to let it have its own set of dirty/available bits.

Quoted 30 lines

arch/x86/kvm/x86.c int complete_pio(struct kvm_vcpu *vcpu) · line 2253

Quoted 8 lines
arch/x86/kvm/x86.c +1 -1
 		if (io->in) {
 			r = pio_copy_data(vcpu);
 			if (r) {
-				kvm_x86_ops->cache_regs(vcpu);
+				kvm_x86_ops->cache_reg(vcpu, VCPU_REGS_RAX);
 				return r;
 			}

These two don't go well together. Apparently the intent of this ->cache_regs call on failure was to restore the original registers in case they were modified by pio_copy_data? But pio_copy_data does not write to any guest register (and even if it did, this ->cache_regs call assumes what registers are fetched from the guest's originals).

        kvm_x86_ops->cache_regs(vcpu);

        if (!io->string) {
                if (io->in)
                        memcpy(&vcpu->arch.regs[VCPU_REGS_RAX], vcpu->arch.pio_data,
                               io->size);
        } else {
                if (io->in) {
                        r = pio_copy_data(vcpu);
                        if (r) {
                                kvm_x86_ops->cache_regs(vcpu);
                                return r;
                        }
                }
Quoted 3 lines

Unless I'm mistaken you can just remove it.

Right.

Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.