Marcelo Tosatti wrote:
Looks good, but we can aim higher. The cache_regs() API was always
confusing (I usually swap the two parts). If we replace all ->regs
access with accessors, we can make it completely transparent.
It will be tricky in the emulator, but worthwhile, no?
OK, in the emulator an interface on top of guest_register_write() is
needed to save registers so that the original contents can be restored
on failure. Some brave soul can do it later, so I added a TODO in x86.c.
Smells better now?
--- dev/null 2008-06-24 14:36:42.383774904 -0300
+++ b/arch/x86/kvm/kvm_cache_regs.h 2008-06-24 15:26:02.000000000 -0300
@@ -0,0 +1,21 @@
+#ifndef ASM_KVM_CACHE_REGS_H
+#define ASM_KVM_CACHE_REGS_H
+
+static inline unsigned long guest_register_read(struct kvm_vcpu *vcpu,
+ enum kvm_reg reg)
+{
+ if (!__test_and_set_bit(reg, &vcpu->arch.regs_available))
+ kvm_x86_ops->cache_regs(vcpu, reg);
+
+ return vcpu->arch.regs[reg];
+}
+
+static inline void guest_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);
+}
+
+#endif
A new header file is excessive. Also, these are global names, so please
prefix with kvm_.
@@ -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;
+ guest_register_write(vcpu, VCPU_REGS_RIP, svm->vmcb->save.rip);
svm->vmcb->control.int_state &= ~SVM_INTERRUPT_SHADOW_MASK;
No need to write into save.rip, is there?
-static void svm_cache_regs(struct kvm_vcpu *vcpu)
+static void svm_cache_regs(struct kvm_vcpu *vcpu, enum kvm_reg reg)
{
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;
+ switch (reg) {
+ case VCPU_REGS_RAX:
+ vcpu->arch.regs[VCPU_REGS_RAX] = svm->vmcb->save.rax;
+ break;
+ case VCPU_REGS_RSP:
+ vcpu->arch.regs[VCPU_REGS_RSP] = svm->vmcb->save.rsp;
+ break;
+ case VCPU_REGS_RIP:
+ vcpu->arch.regs[VCPU_REGS_RIP] = svm->vmcb->save.rip;
+ break;
+ default:
+ break;
+ }
}
For svm we ought to unconditionally copy all the registers and mark all
registers as available, since it's so cheap. This will avoid some callbacks.
We can even to it on the vmexit path, so there will be no
svm_cache_regs() at all.
@@ -707,9 +708,9 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
unsigned long rip;
u32 interruptibility;
- rip = vmcs_readl(GUEST_RIP);
+ rip = guest_register_read(vcpu, VCPU_REGS_RIP);
Perhaps we ought to have a guest_rip_read() since rip is not truly a GPR.
static int set_guest_debug(struct kvm_vcpu *vcpu, struct kvm_debug_guest *dbg)
@@ -2370,22 +2379,18 @@ static int handle_cr(struct kvm_vcpu *vcpu, struct
kvm_run *kvm_run)
(u32)((u64)vcpu->arch.regs[reg] >> 32), handler);
switch (cr) {
case 0:
- vcpu_load_rsp_rip(vcpu);
kvm_set_cr0(vcpu, vcpu->arch.regs[reg]);
What if reg points at rsp? You need to replace arch.regs[*] with the
accessor.
@@ -61,6 +62,7 @@ static int kvm_dev_ioctl_get_supported_cpuid(struct
kvm_cpuid2 *cpuid,
struct kvm_cpuid_entry2 __user *entries);
struct kvm_x86_ops *kvm_x86_ops;
+EXPORT_SYMBOL(kvm_x86_ops);
_GPL
+static void flush_regs(struct kvm_vcpu *vcpu)
+{
+ if (__test_and_clear_bit(VCPU_REGS_RSP, &vcpu->arch.regs_dirty))
+ kvm_x86_ops->decache_regs(vcpu, VCPU_REGS_RSP);
+ if (__test_and_clear_bit(VCPU_REGS_RIP, &vcpu->arch.regs_dirty))
+ kvm_x86_ops->decache_regs(vcpu, VCPU_REGS_RIP);
+ if (__test_and_clear_bit(VCPU_REGS_RAX, &vcpu->arch.regs_dirty))
+ kvm_x86_ops->decache_regs(vcpu, VCPU_REGS_RAX);
+}
This is better done in $subarch_vcpu_run, as it avoids callbacks and
knows exactly which regs to look at. We can do it unconditionally for
svm, too.
+void cache_all_regs(struct kvm_vcpu *vcpu)
+{
+ kvm_x86_ops->cache_regs(vcpu, VCPU_REGS_RAX);
+ kvm_x86_ops->cache_regs(vcpu, VCPU_REGS_RSP);
+ kvm_x86_ops->cache_regs(vcpu, VCPU_REGS_RIP);
+}
+
+void decache_all_regs(struct kvm_vcpu *vcpu)
+{
+ guest_register_write(vcpu, VCPU_REGS_RAX,
+ vcpu->arch.regs[VCPU_REGS_RAX]);
+ guest_register_write(vcpu, VCPU_REGS_RSP,
+ vcpu->arch.regs[VCPU_REGS_RSP]);
+ guest_register_write(vcpu, VCPU_REGS_RIP,
+ vcpu->arch.regs[VCPU_REGS_RIP]);
+}
+
static
@@ -2865,6 +2904,8 @@ again:
local_irq_enable();
++vcpu->stat.exits;
+ vcpu->arch.regs_available = KVM_CACHED_REGS;
+ vcpu->arch.regs_dirty = 0;
How can you have a constant for this? Each subarch has different cached
regs. This ought to be set in $subarch_vcpu_run.
diff --git a/include/asm-x86/kvm_host.h b/include/asm-x86/kvm_host.h
index 851184d..cc5c94b 100644
--- a/include/asm-x86/kvm_host.h
+++ b/include/asm-x86/kvm_host.h
@@ -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,
@@ -106,9 +106,21 @@ enum {
VCPU_REGS_R14 = 14,
VCPU_REGS_R15 = 15,
#endif
+ VCPU_REGS_RIP = 16,
NR_VCPU_REGS
};
No, rip is not a GPR.
+/*
+ * List of registers already read by kvm_x86_ops->run().
+ */
+#define KVM_CACHED_REGS ((1 << VCPU_REGS_RCX) | (1 << VCPU_REGS_RDX) |
\
+ (1 << VCPU_REGS_RBX) | (1 << VCPU_REGS_RBP) |
\
+ (1 << VCPU_REGS_RSI) | (1 << VCPU_REGS_RDI) |
\
+ (1 << VCPU_REGS_R8) | (1 << VCPU_REGS_R9) |
\
+ (1 << VCPU_REGS_R10) | (1 << VCPU_REGS_R11) |
\
+ (1 << VCPU_REGS_R12) | (1 << VCPU_REGS_R13) |
\
+ (1 << VCPU_REGS_R14) | (1 << VCPU_REGS_R15))
+
As mentioned earlier, this is subarch specific (and better written as
~(1 << VCPU_REGS_R?X)).
@@ -410,8 +423,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_regs)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
+ void (*decache_regs)(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);
Remove the plural s as the callbacks now affect a single register.
--
Do not meddle in the internals of kernels, for they are subtle and quick to
panic.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
