Skip to contentSkip to conversation

RFC: cache_regs in kvm_emulate_pio

PATCH Marcelo Tosatti 2 participants 4 messages Jun 19, 2008 – Jun 21, 2008

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

Marcelo Tosatti

author
#0 lore raw

Hi,

Quoted 1 line

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 ?

arch/x86/kvm/svm.c +8 -0
index 238e8f3..6f247cc 100644
@@ -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,
arch/x86/kvm/vmx.c +8 -0
index 6e4278d..0d9a148 100644
@@ -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,
arch/x86/kvm/x86.c +1 -1
index 26b051b..6111946 100644
@@ -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);
include/asm-x86/kvm_host.h +1 -0
index 851184d..95a0736 100644
@@ -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);
#1 lore raw
Quoted 6 lines · Marcelo Tosatti

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().

Quoted 7 lines

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).

ISTR vmwrite as 50 cycles and vmread as much lower.

Quoted 4 lines

BTW, the decache_regs(vcpu) call at the end of complete_pio() could also be a noop on Intel from what I can tell ?

I think so. decache_regs() is actually more important.

include/asm-x86/kvm_host.h struct kvm_x86_ops { · line 416

Quoted 13 lines
include/asm-x86/kvm_host.h +1 -0
index 851184d..95a0736 100644
@@ -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);

ugh, another callback. how about instead

/* in vcpu structure */
u16 regs_available;
u16 regs_dirty;

/* read from cache if possible */
if (!test_bit(VCPU_REG_RAX, &regs_available))

      ->cache_regs();

printk("%d\n", regs[VCPU_REGS_RAX]);

/* write to cache, ->vcpu_run() will flush */
regs[VCPU_REGS_RAX] = 17;
__set_bit(VCPU_REGS_RAX, &regs_dirty);

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
author
#2 lore raw
Quoted 10 lines · Avi Kivity
Quoted 9 lines · Marcelo Tosatti

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.

Quoted 6 lines
Quoted 4 lines

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).

ISTR vmwrite as 50 cycles and vmread as much lower.

On my 1.60GHz textbox ->cache_regs takes 114 cycles, measured with rdtscll() before and after (rdtscll() takes 90 cycles by itself, due to the barriers I guess, so the exact number was 204 cycles). Calling the empty ->cache_rax takes 6 cycles.

Quoted 17 lines
Quoted 4 lines

BTW, the decache_regs(vcpu) call at the end of complete_pio() could also be a noop on Intel from what I can tell ?

I think so. decache_regs() is actually more important.

include/asm-x86/kvm_host.h struct kvm_x86_ops { · line 414

Quoted 3 lines
include/asm-x86/kvm_host.h +1 -0
 		       int *exception);
+	void (*cache_rax)(struct kvm_vcpu *vcpu);
 	void (*cache_regs)(struct kvm_vcpu *vcpu);

ugh, another callback. how about instead

/* in vcpu structure */
u16 regs_available;
u16 regs_dirty;

Quoted 8 lines

/* read from cache if possible */
if (!test_bit(VCPU_REG_RAX, &regs_available))

      ->cache_regs();

printk("%d\n", regs[VCPU_REGS_RAX]);

/* write to cache, ->vcpu_run() will flush */
regs[VCPU_REGS_RAX] = 17;
__set_bit(VCPU_REGS_RAX, &regs_dirty);

I think that hiding whether registers are cached or not behing wrappers makes a lot of sense, but having the ->cache_regs interface split can also result in gains. An index argument to ->cache_regs() would do the trick.

For example, there's no need to read GUEST_RSP for skip_emulated_instruction, thats another 50+ cycles.

Unless there's something obscure that means you need to read RSP/RIP before accessing the now in-memory guest registers saved with "mov" in vmx_vcpu_run(). The comment on vcpu_load_rsp_rip seems a little ambiguous to me:

/*
* 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().

#3 lore raw
Quoted 17 lines · Marcelo Tosatti
Quoted 7 lines

ugh, another callback. how about instead

/* in vcpu structure */
u16 regs_available;
u16 regs_dirty;

Quoted 9 lines

/* read from cache if possible */
if (!test_bit(VCPU_REG_RAX, &regs_available))

      ->cache_regs();

printk("%d\n", regs[VCPU_REGS_RAX]);

/* write to cache, ->vcpu_run() will flush */
regs[VCPU_REGS_RAX] = 17;
__set_bit(VCPU_REGS_RAX, &regs_dirty);

Quoted 6 lines

I think that hiding whether registers are cached or not behing wrappers makes a lot of sense, but having the ->cache_regs interface split can also result in gains. An index argument to ->cache_regs() would do the trick.

Yes and yes.

Quoted 8 lines

For example, there's no need to read GUEST_RSP for skip_emulated_instruction, thats another 50+ cycles.

Unless there's something obscure that means you need to read RSP/RIP before accessing the now in-memory guest registers saved with "mov" in vmx_vcpu_run(). The comment on vcpu_load_rsp_rip seems a little ambiguous to me:

Quoted 9 lines

/*
* 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.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.