Lazy FPU for SVM

Attached patch implements lazy FPU save/restore for SVM. It's much more conservative than my previous patch. We can now mark the guest FPU as inactive whenever we want which will trigger CR0.TS to be set in the guest's shadowed CR0.

Right now, we only mark the FPU as inactive when the guest does (via a mov %cr0, clts, etc.) or during a mov %cr3. There may be a better heuristic out there but this seemed like the obvious one.

I'm still playing around with the VT version of this patch so I'll post that tomorrow.

I've tested on a 32bit and 64bit SVM host using Avi's previously posted fpu-test and some others. Everything seems to be fine.

Regards,

Anthony Liguori
------------------------------------------------------------------------

Author: Anthony Liguori <aliguori-r/[email protected]>
Date:   Sun Apr 22 20:34:03 2007 -0500

Lazy FPU support for SVM.

Signed-off-by Anthony Liguori <aliguori-r/[email protected]>

drivers/kvm/kvm.h +0 -0

index d1a90c5..4859c32 100644

drivers/kvm/kvm.h +0 -0

 #define DF_VECTOR 8
 #define TS_VECTOR 10

#define NP_VECTOR 11

drivers/kvm/kvm.h +1 -0

@@ -301,6 +304,7 @@ struct kvm_vcpu {
 	char fx_buf[FX_BUF_SIZE];
 	char *host_fx_image;
 	char *guest_fx_image;
+	int fpu_active;
 
 	int mmio_needed;
 	int mmio_read_completed;

drivers/kvm/svm.c +6 -4

index 644efc5..bbde031 100644
@@ -30,10 +30,6 @@ MODULE_LICENSE("GPL");
 #define IOPM_ALLOC_ORDER 2
 #define MSRPM_ALLOC_ORDER 1
 
-#define DB_VECTOR 1
-#define UD_VECTOR 6
-#define GP_VECTOR 13
-
 #define DR7_GD_MASK (1 << 13)
 #define DR6_BD_MASK (1 << 13)
 #define CR4_DE_MASK (1UL << 3)

@@ -587,6 +583,7 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 	init_vmcb(vcpu->svm->vmcb);
 
 	fx_init(vcpu);
+	vcpu->fpu_active = 1;
 	vcpu->apic_base = 0xfee00000 |
 			/*for vcpu 0*/ MSR_IA32_APICBASE_BSP |
 			MSR_IA32_APICBASE_ENABLE;

@@ -756,6 +753,11 @@ static void svm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 		}
 	}
 #endif
+	if ((vcpu->cr0 & CR0_TS_MASK) && !(cr0 & CR0_TS_MASK)) {
+		vcpu->svm->vmcb->control.intercept_exceptions &= ~(1 << NM_VECTOR);
+		vcpu->fpu_active = 1;
+	}
+ 
 	vcpu->cr0 = cr0;
 	cr0 |= CR0_PG_MASK | CR0_WP_MASK;
 	cr0 &= ~(CR0_CD_MASK | CR0_NW_MASK);

drivers/kvm/svm.c +11 -0

+
+       vcpu->svm->vmcb->control.intercept_exceptions &= ~(1 << NM_VECTOR);
+       if (!(vcpu->cr0 & CR0_TS_MASK))
+               vcpu->svm->vmcb->save.cr0 &= ~CR0_TS_MASK;
+       vcpu->fpu_active = 1;
+
+       spin_unlock(&vcpu->kvm->lock);
+
+       return 1;
+}
+
 static int shutdown_interception(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 {

drivers/kvm/svm.c +9 -4

 	/*

@@ -1292,6 +1308,7 @@ static int (*svm_exit_handlers[])(struct kvm_vcpu *vcpu,
 	[SVM_EXIT_WRITE_DR5]			= emulate_on_interception,
 	[SVM_EXIT_WRITE_DR7]			= emulate_on_interception,
 	[SVM_EXIT_EXCP_BASE + PF_VECTOR] 	= pf_interception,
+	[SVM_EXIT_EXCP_BASE + NM_VECTOR] 	= nm_interception,
 	[SVM_EXIT_INTR] 			= nop_on_interception,
 	[SVM_EXIT_NMI]				= nop_on_interception,
 	[SVM_EXIT_SMI]				= nop_on_interception,

@@ -1481,8 +1498,10 @@ again:
 		load_db_regs(vcpu->svm->db_regs);
 	}
 
-	fx_save(vcpu->host_fx_image);
-	fx_restore(vcpu->guest_fx_image);
+	if (vcpu->fpu_active) {
+		fx_save(vcpu->host_fx_image);
+		fx_restore(vcpu->guest_fx_image);
+	}
 
 	asm volatile (
 #ifdef CONFIG_X86_64

@@ -1593,8 +1612,10 @@ again:
 #endif
 		: "cc", "memory" );
 
-	fx_save(vcpu->guest_fx_image);
-	fx_restore(vcpu->host_fx_image);
+	if (vcpu->fpu_active) {
+		fx_save(vcpu->guest_fx_image);
+		fx_restore(vcpu->host_fx_image);
+	}
 
 	if ((vcpu->svm->vmcb->save.dr7 & 0xff))
 		load_db_regs(vcpu->svm->host_db_regs);

@@ -1664,6 +1685,12 @@ static void svm_set_cr3(struct kvm_vcpu *vcpu, unsigned long root)
 {
 	vcpu->svm->vmcb->save.cr3 = root;

Quoted 13 lines · Anthony Liguori

Author: Anthony Liguori <aliguori-r/[email protected]>
Date:   Sun Apr 22 20:34:03 2007 -0500

Lazy FPU support for SVM.

Signed-off-by Anthony Liguori <aliguori-r/[email protected]>

drivers/kvm/kvm.h +0 -0

index d1a90c5..4859c32 100644
@@ -63,6 +63,9 @@
 #define FX_BUF_SIZE (2 * FX_IMAGE_SIZE + FX_IMAGE_ALIGN)
 

drivers/kvm/kvm.h line 69

Quoted 5 lines

drivers/kvm/kvm.h +3 -0

 #define DE_VECTOR 0
+#define DB_VECTOR 2
+#define UD_VECTOR 6
+#define NM_VECTOR 7
 

This, while a nice cleanup, is unrelated.

drivers/kvm/kvm.h

Quoted 6 lines

drivers/kvm/kvm.h +3 -0

+static int nm_interception(struct kvm_vcpu *vcpu, struct kvm_run 
 *kvm_run)
+{
+       spin_lock(&vcpu->kvm->lock);

Why is the lock needed? everything below is vcpu-local AFAICS.

Quoted 13 lines

drivers/kvm/kvm.h +5 -0

@@ -1664,6 +1685,12 @@ static void svm_set_cr3(struct kvm_vcpu *vcpu, 
 unsigned long root)
 {
     vcpu->svm->vmcb->save.cr3 = root;
     force_new_asid(vcpu);
+ +    if (vcpu->fpu_active) {
+        vcpu->svm->vmcb->control.intercept_exceptions |= (1 << 
 NM_VECTOR);
+        vcpu->svm->vmcb->save.cr0 |= CR0_TS_MASK;
+        vcpu->fpu_active = 0;
+    }
 }

Quoted 2 lines

static void svm_inject_page_fault(struct kvm_vcpu *vcpu,

Any numbers? I use user/test/vmexit.c to test as it gives the highest relative speedup.

Hard to say exactly because of the noise. I did two runs of 4 test/vmexit:

Before 4091, 4194, 4559, 4439
After: 3979, 4324, 3918, 3910

So there's definitely a speedup, but probably only 100-200 cycles.

Anthony Liguori wrote:

[updated patch]

Applied.

Quoted 6 lines

Hard to say exactly because of the noise. I did two runs of 4 test/vmexit:

Before 4091, 4194, 4559, 4439
After: 3979, 4324, 3918, 3910

Quoted 3 lines

So there's definitely a speedup, but probably only 100-200 cycles.

Running on an idle, headless server with the vcpu pinned reduces variability to a few cycles in the after case. Strangely, the variability is high in the before case. I get a similar disappointing result of 100-200 cycles.

Quoted 17 lines · Avi Kivity

Anthony Liguori wrote:

[updated patch]

Applied.

Quoted 9 lines

Hard to say exactly because of the noise. I did two runs of 4 test/vmexit:

Before 4091, 4194, 4559, 4439
After: 3979, 4324, 3918, 3910

So there's definitely a speedup, but probably only 100-200 cycles.

Quoted 5 lines

Running on an idle, headless server with the vcpu pinned reduces variability to a few cycles in the after case. Strangely, the variability is high in the before case. I get a similar disappointing result of 100-200 cycles.

I previously measured how long fx_save/fx_restore took and it was right around 150 cycles on AMD.

This patch is to remove the redundant MSR save/restore at lightweight VM EXIT time. With this patch VMX guest KB performance can increase 17-37%.

(I only tested it on KVM-18 since the git tree still has problem in my side, but I'd like to post this earlier to collect more feedback) Thanks, eddie

Avi:
Thanks!
Another found is that HOST_FS/GS_BASE & MSR_FS/GS_BASE
save/restore is quit expansive. I am not sure why it needs to be save/restored given that the VMCS regster will be loaded into FS/gs at VM EXIT time automatically by HW and will not be changed during guest execution time. The MSR is also same (guest change will be trap and emulated in guest context only).
If I remove HOST_FS/GS_BASE save/restore out to be only matter
to userspace, I can get 15% performance gain for KB. But I am not sure yet if there is any potential issue.

thanks, eddie

A minor patch to avoid heavy is_empty_shadow_page for release version.

A minor patch to avoid heavy is_empty_shadow_page for release version.

Sorry for that, attached now.

A minor patch to avoid heavy is_empty_shadow_page for release version.

drivers/kvm/mmu.c +0 -0

index e85b4c7..58fdd7b 100644

drivers/kvm/mmu.c +1 -0

+#else
 #define ASSERT(x)
 \
        if (!(x)) {
 \
                printk(KERN_WARNING "assertion failed %s:%d: %s\n",
 \
                       __FILE__, __LINE__, #x);

\
        }

+#endif

#define PT64_PT_BITS 9
#define PT64_ENT_PER_PAGE (1 << PT64_PT_BITS)

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Quoted 9 lines · Eddie

A minor patch to avoid heavy is_empty_shadow_page for release version.

drivers/kvm/mmu.c +0 -0

index e85b4c7..58fdd7b 100644

drivers/kvm/mmu.c static void kvm_mmu_audit(struct kvm_vcpu *vcpu, · line 56

Quoted 5 lines

drivers/kvm/mmu.c +2 -0

@@ -52,11 +52,15 @@ static void kvm_mmu_audit(struct kvm_vcpu *vcpu,
 const char *msg) {} static int dbg = 1;
 #endif
 
+#if defined(MMU_DEBUG)
+#define ASSERT(x) do { } while (0)

I suppose you meant the opposite, !defined(...)?

Why would anyone want to turn off assertions in debug mode?

Quoted 11 lines

drivers/kvm/mmu.c +2 -0

+#else
 #define ASSERT(x)
 \
        if (!(x)) {
 \
                printk(KERN_WARNING "assertion failed %s:%d: %s\n", \
                       __FILE__, __LINE__, #x);
 \
        }
+#endif

Quoted 4 lines

#define PT64_PT_BITS 9
#define PT64_ENT_PER_PAGE (1 << PT64_PT_BITS)

Quoted 8 lines

This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Quoted 12 lines · Eddie

A minor patch to avoid heavy is_empty_shadow_page for release version.

drivers/kvm/mmu.c +0 -0

index e85b4c7..58fdd7b 100644
@@ -52,11 +52,15 @@ static void kvm_mmu_audit(struct kvm_vcpu *vcpu,
 const char *msg) {}
 static int dbg = 1;
 #endif
 

drivers/kvm/mmu.c static void kvm_mmu_audit(struct kvm_vcpu *vcpu, · line 58

Quoted 3 lines

drivers/kvm/mmu.c +2 -0

+#if defined(MMU_DEBUG)
+#define ASSERT(x) do { } while (0)
 

I suppose you meant the opposite, !defined(...)?

Why would anyone want to turn off assertions in debug mode?