Re: RFC: cache_regs in kvm_emulate_pio
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/null2008-06-24 14:36:42.383774904 -0300
+++ b/arch/x86/kvm/kvm_cache_regs.h 2008-06-24 15:26:02.0 -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,
+
Re: RFC: cache_regs in kvm_emulate_pio
On Thu, Jun 26, 2008 at 12:18:07PM +0300, Avi Kivity wrote:
A new header file is excessive. Also, these are global names, so please
prefix with kvm_.
The reason for a separate header is because these accessors need both
kvm_vcpu (linux/kvm_host.h) and kvm_vcpu_arch (asm/kvm_host.h).
I couldnt think of a better location to put them. Ideas?
@@ -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?
I'm not sure why it was there in the first place. Perhaps there's code
which reads svm-vmcb-save.rip directly as the current RIP after this
point?
Also kvm_guest_register_read is too long, so I dropped guest.
--- /dev/null 2008-06-26 10:56:31.025001212 -0300
+++ b/arch/x86/kvm/kvm_cache_regs.h 2008-06-26 18:38:48.0 -0300
@@ -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
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 73f43de..9fde0ac 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -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;
}
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 238e8f3..532a393 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -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;
@@ -709,21 +711,42 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu)
rdtscll(vcpu-arch.host_tsc);
}
-static void svm_cache_regs(struct kvm_vcpu *vcpu)
+static void svm_cache_reg(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.rip = svm-vmcb-save.rip;
+ break;
+ default:
+ break;
+ }
}
-static void svm_decache_regs(struct kvm_vcpu *vcpu)
+static void svm_decache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
{
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;
+
+ switch (reg) {
+ case VCPU_REGS_RAX:
+ svm-vmcb-save.rax = vcpu-arch.regs[VCPU_REGS_RAX];
+ break;
+ case VCPU_REGS_RSP:
+ svm-vmcb-save.rsp =
Re: RFC: cache_regs in kvm_emulate_pio
On Thu, Jun 26, 2008 at 07:15:45PM -0300, Marcelo Tosatti wrote:
On Thu, Jun 26, 2008 at 12:18:07PM +0300, Avi Kivity wrote:
A new header file is excessive. Also, these are global names, so please
prefix with kvm_.
The reason for a separate header is because these accessors need both
kvm_vcpu (linux/kvm_host.h) and kvm_vcpu_arch (asm/kvm_host.h).
I couldnt think of a better location to put them. Ideas?
@@ -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?
I'm not sure why it was there in the first place. Perhaps there's code
which reads svm-vmcb-save.rip directly as the current RIP after this
point?
Also kvm_guest_register_read is too long, so I dropped guest.
There was a missing convertion in vmx_vcpu_reset(). And haven't tested
AMD.
--- /dev/null 2008-06-26 10:56:31.025001212 -0300
+++ b/arch/x86/kvm/kvm_cache_regs.h 2008-06-26 23:21:28.0 -0300
@@ -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
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 73f43de..9fde0ac 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -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;
}
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 238e8f3..532a393 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -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;
@@ -709,21 +711,42 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu)
rdtscll(vcpu-arch.host_tsc);
}
-static void svm_cache_regs(struct kvm_vcpu *vcpu)
+static void svm_cache_reg(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.rip = svm-vmcb-save.rip;
+ break;
+ default:
+ break;
+ }
}
-static void svm_decache_regs(struct kvm_vcpu *vcpu)
+static void svm_decache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
{
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;
+
+ switch (reg) {
+ case VCPU_REGS_RAX:
+
Re: RFC: cache_regs in kvm_emulate_pio
On Sun, Jun 22, 2008 at 08:16:19AM +0300, Avi Kivity wrote:
Marcelo Tosatti wrote:
On Sat, Jun 21, 2008 at 10:04:18AM +0300, Avi Kivity wrote:
/*
* Sync the rsp and rip registers into the vcpu structure. This allows
* registers to be accessed by indexing vcpu-arch.regs.
*/
But I think it just refers to the interface in general, so that nobody
would try to access RSP or RIP (and RAX in AMD's case) before calling
-cache_regs().
It refers to the fact that sometimes you don't know which registers
you refer to, e.g. in the emulator.
How's this?
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/null2008-06-24 14:36:42.383774904 -0300
+++ b/arch/x86/kvm/kvm_cache_regs.h 2008-06-24 15:26:02.0 -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
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 73f43de..97919b6 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -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 = guest_register_read(vcpu, VCPU_REGS_RIP);
run-tpr_access.is_write = write;
}
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 238e8f3..acd96f6 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -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;
+ guest_register_write(vcpu, VCPU_REGS_RIP, svm-vmcb-save.rip);
svm-vmcb-control.int_state = ~SVM_INTERRUPT_SHADOW_MASK;
vcpu-arch.interrupt_window_open = 1;
@@ -709,21 +711,42 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu)
rdtscll(vcpu-arch.host_tsc);
}
-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;
+ }
}
-static void svm_decache_regs(struct kvm_vcpu *vcpu)
+static void svm_decache_regs(struct kvm_vcpu *vcpu, enum kvm_reg reg)
{
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;
+
+ switch (reg) {
+ case VCPU_REGS_RAX:
+ svm-vmcb-save.rax = vcpu-arch.regs[VCPU_REGS_RAX];
+ break;
+ case VCPU_REGS_RSP:
+ svm-vmcb-save.rsp = vcpu-arch.regs[VCPU_REGS_RSP];
+ break;
+ case VCPU_REGS_RIP:
+ svm-vmcb-save.rip = vcpu-arch.regs[VCPU_REGS_RIP];
+ break;
+ default:
+ break;
+ }
}
static unsigned long svm_get_rflags(struct kvm_vcpu *vcpu)
diff --git
Re: RFC: cache_regs in kvm_emulate_pio
On Sun, Jun 22, 2008 at 08:16:19AM +0300, Avi Kivity 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? Yes, agree. Will go for accessors. Thanks. -- 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
Re: RFC: cache_regs in kvm_emulate_pio
On Sat, Jun 21, 2008 at 10:04:18AM +0300, Avi Kivity wrote:
/*
* Sync the rsp and rip registers into the vcpu structure. This allows
* registers to be accessed by indexing vcpu-arch.regs.
*/
But I think it just refers to the interface in general, so that nobody
would try to access RSP or RIP (and RAX in AMD's case) before calling
-cache_regs().
It refers to the fact that sometimes you don't know which registers you
refer to, e.g. in the emulator.
How's this?
Test performed with idle UP guest booted with nohz=off
clocksource=acpi_pm, so most of the exits are acpi timer reads. This
average is from available entries in the dmesg buffer, about 1500.
regs_available and regs_dirty could be 8 bits, and placed in a hole
instead of in the middle of longs.
avg cycles to exit to qemu:
before: 3376
after: 3181
195 cycles (~= 6.1% improvement)
avg cycles to exit to qemu, handle exit and return to __vcpu_run before
irq_disable:
before: 7482 cycles.
after: 7227 cycles.
255 cycles (~= 3.5% improvement)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 73f43de..ecf26e3 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -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,7 +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);
+ cache_regs(vcpu, REGS_RIP);
run-tpr_access.rip = vcpu-arch.rip;
run-tpr_access.is_write = write;
}
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 238e8f3..8554b37 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -709,21 +709,38 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu)
rdtscll(vcpu-arch.host_tsc);
}
-static void svm_cache_regs(struct kvm_vcpu *vcpu)
+static void svm_cache_regs(struct kvm_vcpu *vcpu, enum kvm_reg_set regs)
{
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 (regs) {
+ case REGS_GPR:
+ vcpu-arch.regs[VCPU_REGS_RAX] = svm-vmcb-save.rax;
+ break;
+ case REGS_RSP:
+ vcpu-arch.regs[VCPU_REGS_RSP] = svm-vmcb-save.rsp;
+ break;
+ case REGS_RIP:
+ vcpu-arch.rip = svm-vmcb-save.rip;
+ break;
+ }
}
-static void svm_decache_regs(struct kvm_vcpu *vcpu)
+static void svm_decache_regs(struct kvm_vcpu *vcpu, enum kvm_reg_set regs)
{
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;
+
+ switch (regs) {
+ case REGS_GPR:
+ svm-vmcb-save.rax = vcpu-arch.regs[VCPU_REGS_RAX];
+ break;
+ case REGS_RSP:
+ svm-vmcb-save.rsp = vcpu-arch.regs[VCPU_REGS_RSP];
+ break;
+ case REGS_RIP:
+ svm-vmcb-save.rip = vcpu-arch.rip;
+ break;
+ }
}
static unsigned long svm_get_rflags(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 6e4278d..b7a988c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -26,6 +26,7 @@
#include linux/highmem.h
#include linux/sched.h
#include linux/moduleparam.h
+#include kvm_cache_regs.h
#include asm/io.h
#include asm/desc.h
@@ -707,9 +708,11 @@ static void skip_emulated_instruction(struct kvm_vcpu
*vcpu)
unsigned long rip;
u32 interruptibility;
- rip = vmcs_readl(GUEST_RIP);
+ cache_regs(vcpu, REGS_RIP);
+ rip = vcpu-arch.rip;
rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
- vmcs_writel(GUEST_RIP, rip);
+ vcpu-arch.rip = rip;
+ decache_regs(vcpu, REGS_RIP);
/*
* We emulated an instruction, so temporary interrupt blocking
@@ -935,20 +938,48 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, u32
msr_index, u64 data)
* Sync the rsp and rip registers into the vcpu structure. This allows
* registers to be accessed by indexing vcpu-arch.regs.
*/
-static void vcpu_load_rsp_rip(struct kvm_vcpu *vcpu)
+static void vmx_cache_regs(struct kvm_vcpu *vcpu, enum kvm_reg_set regs)
{
- vcpu-arch.regs[VCPU_REGS_RSP] = vmcs_readl(GUEST_RSP);
- vcpu-arch.rip = vmcs_readl(GUEST_RIP);
+ switch (regs) {
+ case REGS_RSP:
+ vcpu-arch.regs[VCPU_REGS_RSP] = vmcs_readl(GUEST_RSP);
+ break;
+ case REGS_RIP:
+ vcpu-arch.rip = vmcs_readl(GUEST_RIP);
+ break;
+ case REGS_GPR:
+ break;
+ }
}
/*
* Syncs rsp and rip back into the vmcs. Should
Re: RFC: cache_regs in kvm_emulate_pio
Marcelo Tosatti wrote: On Sat, Jun 21, 2008 at 10:04:18AM +0300, Avi Kivity wrote: /* * Sync the rsp and rip registers into the vcpu structure. This allows * registers to be accessed by indexing vcpu-arch.regs. */ But I think it just refers to the interface in general, so that nobody would try to access RSP or RIP (and RAX in AMD's case) before calling -cache_regs(). It refers to the fact that sometimes you don't know which registers you refer to, e.g. in the emulator. How's this? 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? -- 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
RFC: cache_regs in kvm_emulate_pio
Hi,
From my understanding the -cache_regs call on kvm_emulate_pio() is
necessary only on AMD, where vcpu-arch.regs[RAX] is not copied during
exit in svm_vcpu_load().
On both architectures, the remaining general purpose registers are saved
on exit.
The following patch saves 100 cycles out of both light and heavy exits
on Intel (if correct, kvm_emulate_hypercall and complete_pio could also
benefit, thus saving 200 cycles for in-kernel devices).
BTW, the decache_regs(vcpu) call at the end of complete_pio() could also
be a noop on Intel from what I can tell ?
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 238e8f3..6f247cc 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -709,6 +709,13 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu)
rdtscll(vcpu-arch.host_tsc);
}
+static void svm_cache_rax(struct kvm_vcpu *vcpu)
+{
+ struct vcpu_svm *svm = to_svm(vcpu);
+
+ vcpu-arch.regs[VCPU_REGS_RAX] = svm-vmcb-save.rax;
+}
+
static void svm_cache_regs(struct kvm_vcpu *vcpu)
{
struct vcpu_svm *svm = to_svm(vcpu);
@@ -1949,6 +1956,7 @@ static struct kvm_x86_ops svm_x86_ops = {
.set_gdt = svm_set_gdt,
.get_dr = svm_get_dr,
.set_dr = svm_set_dr,
+ .cache_rax = svm_cache_rax,
.cache_regs = svm_cache_regs,
.decache_regs = svm_decache_regs,
.get_rflags = svm_get_rflags,
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 6e4278d..0d9a148 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -932,6 +932,13 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, u32
msr_index, u64 data)
}
/*
+ * vcpu-arch.regs[RAX] already loaded by vmx_vcpu_run().
+ */
+static void vcpu_load_rax(struct kvm_vcpu *vcpu)
+{
+}
+
+/*
* Sync the rsp and rip registers into the vcpu structure. This allows
* registers to be accessed by indexing vcpu-arch.regs.
*/
@@ -3213,6 +3220,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
.set_idt = vmx_set_idt,
.get_gdt = vmx_get_gdt,
.set_gdt = vmx_set_gdt,
+ .cache_rax = vcpu_load_rax,
.cache_regs = vcpu_load_rsp_rip,
.decache_regs = vcpu_put_rsp_rip,
.get_rflags = vmx_get_rflags,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 26b051b..6111946 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2302,7 +2302,7 @@ int kvm_emulate_pio(struct kvm_vcpu *vcpu, struct kvm_run
*run, int in,
KVMTRACE_2D(IO_WRITE, vcpu, vcpu-run-io.port, (u32)size,
handler);
- kvm_x86_ops-cache_regs(vcpu);
+ kvm_x86_ops-cache_rax(vcpu);
memcpy(vcpu-arch.pio_data, vcpu-arch.regs[VCPU_REGS_RAX], 4);
kvm_x86_ops-skip_emulated_instruction(vcpu);
diff --git a/include/asm-x86/kvm_host.h b/include/asm-x86/kvm_host.h
index 851184d..95a0736 100644
--- a/include/asm-x86/kvm_host.h
+++ b/include/asm-x86/kvm_host.h
@@ -410,6 +410,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_rax)(struct kvm_vcpu *vcpu);
void (*cache_regs)(struct kvm_vcpu *vcpu);
void (*decache_regs)(struct kvm_vcpu *vcpu);
unsigned long (*get_rflags)(struct kvm_vcpu *vcpu);
--
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
