[PATCH] Lazy FPU for SVM

[PATCH] Lazy FPU for SVM

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 683 bytes --]

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

[-- Attachment #2: kvm-svm-lazy-fpu.diff --]
[-- Type: text/x-patch, Size: 3774 bytes --]

Author: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Date:   Sun Apr 22 20:34:03 2007 -0500

    Lazy FPU support for SVM.
    
    Signed-off-by: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
index d1a90c5..4859c32 100644
--- a/drivers/kvm/kvm.h
+++ b/drivers/kvm/kvm.h
@@ -63,6 +63,9 @@
 #define FX_BUF_SIZE (2 * FX_IMAGE_SIZE + FX_IMAGE_ALIGN)
 
 #define DE_VECTOR 0
+#define DB_VECTOR 2
+#define UD_VECTOR 6
+#define NM_VECTOR 7
 #define DF_VECTOR 8
 #define TS_VECTOR 10
 #define NP_VECTOR 11
@@ -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;
diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
index 644efc5..bbde031 100644
--- a/drivers/kvm/svm.c
+++ b/drivers/kvm/svm.c
@@ -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);
@@ -928,6 +930,20 @@ static int pf_interception(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	return 0;
 }
 
+static int nm_interception(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
+{
+       spin_lock(&vcpu->kvm->lock);
+
+       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)
 {
 	/*
@@ -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;
 	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;
+	}
 }
 
 static void svm_inject_page_fault(struct kvm_vcpu *vcpu,

[-- Attachment #3: Type: text/plain, Size: 286 bytes --]

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

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] Lazy FPU for SVM

[Avi Kivity]

Anthony Liguori wrote:
> 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/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Date:   Sun Apr 22 20:34:03 2007 -0500
>
>     Lazy FPU support for SVM.
>     
>     Signed-off-by: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>
> diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
> index d1a90c5..4859c32 100644
> --- a/drivers/kvm/kvm.h
> +++ b/drivers/kvm/kvm.h
> @@ -63,6 +63,9 @@
>  #define FX_BUF_SIZE (2 * FX_IMAGE_SIZE + FX_IMAGE_ALIGN)
>  
>  #define DE_VECTOR 0
> +#define DB_VECTOR 2
> +#define UD_VECTOR 6
> +#define NM_VECTOR 7
>   

This, while a nice cleanup, is unrelated.

>  #define DF_VECTOR 8
>  #define TS_VECTOR 10
>  #define NP_VECTOR 11
> @@ -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;
> diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
> index 644efc5..bbde031 100644
> --- a/drivers/kvm/svm.c
> +++ b/drivers/kvm/svm.c
> @@ -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);
> @@ -928,6 +930,20 @@ static int pf_interception(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  	return 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.

> +
> +       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)
>  {
>  	/*
> @@ -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;
>  	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;
> +	}
>  }
>  
>  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.


-- 
error compiling committee.c: too many arguments to function


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

Re: [PATCH] Lazy FPU for SVM

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 2223 bytes --]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> Author: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>> Date:   Sun Apr 22 20:34:03 2007 -0500
>>
>>     Lazy FPU support for SVM.
>>         Signed-off-by: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>>
>> diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
>> index d1a90c5..4859c32 100644
>> --- a/drivers/kvm/kvm.h
>> +++ b/drivers/kvm/kvm.h
>> @@ -63,6 +63,9 @@
>>  #define FX_BUF_SIZE (2 * FX_IMAGE_SIZE + FX_IMAGE_ALIGN)
>>  
>>  #define DE_VECTOR 0
>> +#define DB_VECTOR 2
>> +#define UD_VECTOR 6
>> +#define NM_VECTOR 7
>>   
>
> This, while a nice cleanup, is unrelated.

I had to add the NM_VECTOR so I moved the others too.  Attached is a 
patch that doesn't move those.  I noticed that the #define's in general 
need some love.  There's quite a lot in kvm.h that's specific to vmx and 
there's a bit of asymmetry between vmx and svm (svm has kvm_svm.h, vmx 
used to but it was removed recently).  There's also a ton of unused 
#define's (quite a few that are typos).

I was planning on submitting a cleanup patch series today so I'll just 
fold the above changes into that.

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

You're right, removed in attached patch.

>> @@ -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;
>> +    }
>>  }
>>  
>>  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.

Regards,

Anthony Liguori

[-- Attachment #2: kvm-svm-lazy-fpu-2.diff --]
[-- Type: text/x-patch, Size: 3391 bytes --]

Author: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Date:   Mon Apr 23 09:17:21 2007 -0500

    Lazy FPU support for SVM.
    
    Signed-off-by: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
index d1a90c5..61ff085 100644
--- a/drivers/kvm/kvm.h
+++ b/drivers/kvm/kvm.h
@@ -63,6 +63,7 @@
 #define FX_BUF_SIZE (2 * FX_IMAGE_SIZE + FX_IMAGE_ALIGN)
 
 #define DE_VECTOR 0
+#define NM_VECTOR 7
 #define DF_VECTOR 8
 #define TS_VECTOR 10
 #define NP_VECTOR 11
@@ -301,6 +302,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;
diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
index 644efc5..9438931 100644
--- a/drivers/kvm/svm.c
+++ b/drivers/kvm/svm.c
@@ -587,6 +587,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 +757,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);
@@ -928,6 +934,16 @@ static int pf_interception(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	return 0;
 }
 
+static int nm_interception(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
+{
+       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;
+
+       return 1;
+}
+
 static int shutdown_interception(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 {
 	/*
@@ -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;
 	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;
+	}
 }
 
 static void svm_inject_page_fault(struct kvm_vcpu *vcpu,

[-- Attachment #3: Type: text/plain, Size: 286 bytes --]

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

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] Lazy FPU for SVM

[Avi Kivity]

Anthony Liguori wrote:

[updated patch]

Applied.

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

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.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


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

Re: [PATCH] Lazy FPU for SVM

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>
> [updated patch]
>
> Applied.
>
>   
>> 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.
>>
>>     
>
> 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.

Regards,

Anthony Liguori


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

Re: [PATCH] Lazy FPU for SVM

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Anthony Liguori wrote:
>>
>> [updated patch]
>>
>> Applied.
>>
>>  
>>> 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.
>>>
>>>     
>>
>> 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.

Well, we're calling it twice per vmexit.

A save/restore pair is 313 cycles on a Woodcrest; two of those make 626
out of ~4000 cycles should get us a nice 15% gain.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


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

[PATCH] Remove heavy is_empty_shadow_page call.

[Dong, Eddie]

A minor patch to avoid heavy is_empty_shadow_page for release version. 


diff --git a/drivers/kvm/mmu.c b/drivers/kvm/mmu.c
index e85b4c7..58fdd7b 100644
--- a/drivers/kvm/mmu.c
+++ b/drivers/kvm/mmu.c
@@ -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)
+#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/

[PATCH] Remove redundant MSR save/restore

[Dong, Eddie]

[-- Attachment #1: Type: text/plain, Size: 302 bytes --]

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

[-- Attachment #2: msr.diff --]
[-- Type: application/octet-stream, Size: 4130 bytes --]

commit 11554c848e14f5322210c640e4a7ddeabe961282
Author: Yaozu Dong <Eddie.dong@intel.com>
Date:   Wed Apr 25 14:14:17 2007 +0800

    Avoid MSR save/restore for lightweight VM EXIT where no
    task switch nor qemu user mode is entered.

diff --git a/drivers/kvm/kvm_vmx.h b/drivers/kvm/kvm_vmx.h
index d139f73..f1bf7ad 100644
--- a/drivers/kvm/kvm_vmx.h
+++ b/drivers/kvm/kvm_vmx.h
@@ -1,14 +1,6 @@
 #ifndef __KVM_VMX_H
 #define __KVM_VMX_H
 
-#ifdef CONFIG_X86_64
-/*
- * avoid save/load MSR_SYSCALL_MASK and MSR_LSTAR by std vt
- * mechanism (cpu bug AA24)
- */
-#define NR_BAD_MSRS 2
-#else
-#define NR_BAD_MSRS 0
-#endif
+#define NR_HW_SAVE_MSRS  1
 
 #endif
diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
index fbbf9d6..3d2176f 100644
--- a/drivers/kvm/vmx.c
+++ b/drivers/kvm/vmx.c
@@ -71,10 +71,11 @@ static struct kvm_vmx_segment_field {
 };
 
 static const u32 vmx_msr_index[] = {
+	MSR_K6_STAR,
 #ifdef CONFIG_X86_64
 	MSR_SYSCALL_MASK, MSR_LSTAR, MSR_CSTAR, MSR_KERNEL_GS_BASE,
 #endif
-	MSR_EFER, MSR_K6_STAR,
+	MSR_EFER,
 };
 #define NR_VMX_MSR ARRAY_SIZE(vmx_msr_index)
 
@@ -991,7 +992,7 @@ static int vmx_vcpu_setup(struct kvm_vcpu *vcpu)
 	struct descriptor_table dt;
 	int i;
 	int ret = 0;
-	int nr_good_msrs;
+	int nr_sw_save_msrs;
 	extern asmlinkage void kvm_vmx_return(void);
 
 	if (!init_rmode_tss(vcpu->kvm)) {
@@ -1140,18 +1141,18 @@ static int vmx_vcpu_setup(struct kvm_vcpu *vcpu)
 	}
 	printk(KERN_DEBUG "kvm: msrs: %d\n", vcpu->nmsrs);
 
-	nr_good_msrs = vcpu->nmsrs - NR_BAD_MSRS;
+	nr_sw_save_msrs = vcpu->nmsrs - NR_HW_SAVE_MSRS;
 	vmcs_writel(VM_ENTRY_MSR_LOAD_ADDR,
-		    virt_to_phys(vcpu->guest_msrs + NR_BAD_MSRS));
+		    virt_to_phys(vcpu->guest_msrs + nr_sw_save_msrs));
 	vmcs_writel(VM_EXIT_MSR_STORE_ADDR,
-		    virt_to_phys(vcpu->guest_msrs + NR_BAD_MSRS));
+		    virt_to_phys(vcpu->guest_msrs + nr_sw_save_msrs));
 	vmcs_writel(VM_EXIT_MSR_LOAD_ADDR,
-		    virt_to_phys(vcpu->host_msrs + NR_BAD_MSRS));
+		    virt_to_phys(vcpu->host_msrs + nr_sw_save_msrs));
 	vmcs_write32_fixedbits(MSR_IA32_VMX_EXIT_CTLS, VM_EXIT_CONTROLS,
 		     	       (HOST_IS_64 << 9));  /* 22.2,1, 20.7.1 */
-	vmcs_write32(VM_EXIT_MSR_STORE_COUNT, nr_good_msrs); /* 22.2.2 */
-	vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, nr_good_msrs);  /* 22.2.2 */
-	vmcs_write32(VM_ENTRY_MSR_LOAD_COUNT, nr_good_msrs); /* 22.2.2 */
+	vmcs_write32(VM_EXIT_MSR_STORE_COUNT, NR_HW_SAVE_MSRS); /* 22.2.2 */
+	vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, NR_HW_SAVE_MSRS);  /* 22.2.2 */
+	vmcs_write32(VM_ENTRY_MSR_LOAD_COUNT, NR_HW_SAVE_MSRS); /* 22.2.2 */
 
 
 	/* 22.2.1, 20.8.1 */
@@ -1732,6 +1733,9 @@ static int vmx_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	int fs_gs_ldt_reload_needed;
 	int r;
 
+	save_msrs(vcpu->host_msrs, vcpu->nmsrs);
+	load_msrs(vcpu->guest_msrs, vcpu->nmsrs - NR_HW_SAVE_MSRS);
+
 again:
 	/*
 	 * Set host fs and gs selectors.  Unfortunately, 22.2.3 does not
@@ -1766,9 +1770,6 @@ again:
 	fx_save(vcpu->host_fx_image);
 	fx_restore(vcpu->guest_fx_image);
 
-	save_msrs(vcpu->host_msrs, vcpu->nmsrs);
-	load_msrs(vcpu->guest_msrs, NR_BAD_MSRS);
-
 	asm (
 		/* Store host registers */
 		"pushf \n\t"
@@ -1911,9 +1912,6 @@ again:
 	}
 	++kvm_stat.exits;
 
-	save_msrs(vcpu->guest_msrs, NR_BAD_MSRS);
-	load_msrs(vcpu->host_msrs, NR_BAD_MSRS);
-
 	fx_save(vcpu->guest_fx_image);
 	fx_restore(vcpu->host_fx_image);
 	vcpu->interrupt_window_open = (vmcs_read32(GUEST_INTERRUPTIBILITY_INFO) & 3) == 0;
@@ -1939,14 +1937,14 @@ again:
 			/* Give scheduler a change to reschedule. */
 			if (signal_pending(current)) {
 				++kvm_stat.signal_exits;
-				post_kvm_run_save(vcpu, kvm_run);
-				return -EINTR;
+ 				r = -EINTR;
+ 				goto out;
 			}
 
 			if (dm_request_for_irq_injection(vcpu, kvm_run)) {
 				++kvm_stat.request_irq_exits;
-				post_kvm_run_save(vcpu, kvm_run);
-				return -EINTR;
+ 				r = -EINTR;
+ 				goto out;
 			}
 
 			kvm_resched(vcpu);
@@ -1954,6 +1952,10 @@ again:
 		}
 	}
 
+out:
+	save_msrs(vcpu->guest_msrs, vcpu->nmsrs - NR_HW_SAVE_MSRS);
+	load_msrs(vcpu->host_msrs, vcpu->nmsrs - NR_HW_SAVE_MSRS);
+
 	post_kvm_run_save(vcpu, kvm_run);
 	return r;
 }

[-- Attachment #3: Type: text/plain, Size: 286 bytes --]

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

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] Remove heavy is_empty_shadow_page call.

[Avi Kivity]

Dong, Eddie wrote:
> A minor patch to avoid heavy is_empty_shadow_page for release version. 
>
>
>   

Please resend as attachment, you email client mangled the patch.

-- 
error compiling committee.c: too many arguments to function


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

Re: [PATCH] Remove heavy is_empty_shadow_page call.

[Dong, Eddie]

[-- Attachment #1: Type: text/plain, Size: 518 bytes --]

Sorry for that, attached now.
Eddie

-----Original Message-----
From: Avi Kivity [mailto:avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org] 
Sent: 2007年4月25日 16:03
To: Dong, Eddie
Cc: kvm-devel
Subject: Re: [kvm-devel] [PATCH] Remove heavy is_empty_shadow_page call.

Dong, Eddie wrote:
> A minor patch to avoid heavy is_empty_shadow_page for release version. 
>
>
>   

Please resend as attachment, you email client mangled the patch.

-- 
error compiling committee.c: too many arguments to function

[-- Attachment #2: assert.diff --]
[-- Type: application/octet-stream, Size: 720 bytes --]

commit 2578b40853dc8601ffaf76bac40a40653b2e8639
Author: Yaozu Dong <eddie.dong@intel.com>
Date:   Wed Apr 25 14:17:25 2007 +0800

    Avoid heavy ASSERT at non debug mode.

diff --git a/drivers/kvm/mmu.c b/drivers/kvm/mmu.c
index e85b4c7..58fdd7b 100644
--- a/drivers/kvm/mmu.c
+++ b/drivers/kvm/mmu.c
@@ -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)
+#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)

[-- Attachment #3: Type: text/plain, Size: 286 bytes --]

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

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] Remove redundant MSR save/restore

[Avi Kivity]

Dong, Eddie wrote:
> 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%.
>   

There is a similar patch already in kvm-20 (which takes a slightly 
different approach of changing nr_good_msrs depending on the mode).

However, the part that hoists the software msr saving above the vmentry 
loop is not in my tree, and it does look good.  It also makes sense to 
use software save/load for the msrs that only matter to userspace.


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

Please let me know if I can do anything to help with the git problem.  I 
also suggest subscribing to kvm-commits so that you can see the work on 
my tree.

Also, please post patches *both* inline and as attachments :) - inline 
makes it easy to review and comment, and attachments to prevent mangling 
(this only applies to those with broken mailers -- a normal mailer will 
set the attachments content disposition to 'inline', which lets 
thunderbird show it as normal text).

-- 
error compiling committee.c: too many arguments to function


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

Re: [PATCH] Remove heavy is_empty_shadow_page call.

[Avi Kivity]

[-- Attachment #1: Type: text/plain, Size: 185 bytes --]

Dong, Eddie wrote:
> Sorry for that, attached now.
>   

Applied. Please provide a Signed-off-by: line in the future.

-- 
error compiling committee.c: too many arguments to function



[-- Attachment #2: Type: text/plain, Size: 286 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] Remove heavy is_empty_shadow_page call.

[Michael Riepe]

Dong, Eddie wrote:
> A minor patch to avoid heavy is_empty_shadow_page for release version. 
> 
> 
> diff --git a/drivers/kvm/mmu.c b/drivers/kvm/mmu.c
> index e85b4c7..58fdd7b 100644
> --- a/drivers/kvm/mmu.c
> +++ b/drivers/kvm/mmu.c
> @@ -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?

> +#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
> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/kvm-devel

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

Re: [PATCH] Remove heavy is_empty_shadow_page call.

[Dong, Eddie]

O, yes typo at different tree porting time.

Michael Riepe wrote:
> Dong, Eddie wrote:
>> A minor patch to avoid heavy is_empty_shadow_page for release
>> version. 
>> 
>> 
>> diff --git a/drivers/kvm/mmu.c b/drivers/kvm/mmu.c
>> index e85b4c7..58fdd7b 100644
>> --- a/drivers/kvm/mmu.c
>> +++ b/drivers/kvm/mmu.c
>> @@ -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?
> 
>> +#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
>> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>> https://lists.sourceforge.net/lists/listinfo/kvm-devel

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

Re: [PATCH] Remove heavy is_empty_shadow_page call.

[Avi Kivity]

Michael Riepe wrote:
> Dong, Eddie wrote:
>   
>> A minor patch to avoid heavy is_empty_shadow_page for release version. 
>>
>>
>> diff --git a/drivers/kvm/mmu.c b/drivers/kvm/mmu.c
>> index e85b4c7..58fdd7b 100644
>> --- a/drivers/kvm/mmu.c
>> +++ b/drivers/kvm/mmu.c
>> @@ -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?
>
>   

Good catch.  I committed a fix.

-- 
error compiling committee.c: too many arguments to function


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

Re: [PATCH] Remove redundant MSR save/restore

[Dong, 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

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

Re: [PATCH] Remove redundant MSR save/restore

[Avi Kivity]

Dong, Eddie wrote:
> 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
>   

I don't see issues with moving them outside the loop, as long as 
preemption is disabled.

-- 
error compiling committee.c: too many arguments to function


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