[PATCH] Lazy FPU save/restore

[PATCH] Lazy FPU save/restore

[Anthony Liguori]

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

The following patch saves the host FPU state and loads the guests FPU 
state if !(CR0.MP == 1 && CR0.TS == 1).

When CR0.MP == 1 && CR0.TS == 1, all FPU activity will generate 
exceptions.  OS's use these exceptions to implement lazy FPU loading to 
improve context switch time.  Since any FPU activity will generate 
traps, we don't have to worry about the guest modifying the host FPU state.

My microbenchmark of choice uses FPU operations so I think the results 
are currently tainted.  I've only tested on a 32bit SVM system.

Signed-off-by: Anthony Liguori <anthony-rdkfGonbjUSkNkDKm+mE6A@public.gmane.org>

Regards,

Anthony Liguori



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

diff -r 962b18bbd6d9 kernel/kvm.h
--- a/kernel/kvm.h	Thu Feb 15 13:58:59 2007 +0000
+++ b/kernel/kvm.h	Thu Feb 15 13:23:56 2007 -0600
@@ -17,6 +17,7 @@
 #include <linux/kvm_para.h>
 
 #define CR0_PE_MASK (1ULL << 0)
+#define CR0_MP_MASK (1ULL << 1)
 #define CR0_TS_MASK (1ULL << 3)
 #define CR0_NE_MASK (1ULL << 5)
 #define CR0_WP_MASK (1ULL << 16)
@@ -263,6 +264,7 @@ struct kvm_vcpu {
 
 	struct kvm_guest_debug guest_debug;
 
+	int fx_active;
 	char fx_buf[FX_BUF_SIZE];
 	char *host_fx_image;
 	char *guest_fx_image;
diff -r 962b18bbd6d9 kernel/kvm_main.c
--- a/kernel/kvm_main.c	Thu Feb 15 13:58:59 2007 +0000
+++ b/kernel/kvm_main.c	Thu Feb 15 13:27:17 2007 -0600
@@ -404,6 +404,7 @@ void set_cr0(struct kvm_vcpu *vcpu, unsi
 
 	}
 
+	vcpu->fx_active = !((cr0 & CR0_TS_MASK) && (cr0 & CR0_MP_MASK));
 	kvm_arch_ops->set_cr0(vcpu, cr0);
 	vcpu->cr0 = cr0;
 
@@ -530,6 +531,7 @@ void fx_init(struct kvm_vcpu *vcpu)
 	fx_image->mxcsr = 0x1f80;
 	memset(vcpu->guest_fx_image + sizeof(struct fx_image_s),
 	       0, FX_IMAGE_SIZE - sizeof(struct fx_image_s));
+	vcpu->fx_active = 1;
 }
 EXPORT_SYMBOL_GPL(fx_init);
 
diff -r 962b18bbd6d9 kernel/svm.c
--- a/kernel/svm.c	Thu Feb 15 13:58:59 2007 +0000
+++ b/kernel/svm.c	Thu Feb 15 13:30:52 2007 -0600
@@ -1461,8 +1461,10 @@ again:
 		load_db_regs(vcpu->svm->db_regs);
 	}
 
-	fx_save(vcpu->host_fx_image);
-	fx_restore(vcpu->guest_fx_image);
+	if (vcpu->fx_active) {
+		fx_save(vcpu->host_fx_image);
+		fx_restore(vcpu->guest_fx_image);
+	}
 
 	asm volatile (
 #ifdef CONFIG_X86_64
@@ -1573,8 +1575,10 @@ again:
 #endif
 		: "cc", "memory" );
 
-	fx_save(vcpu->guest_fx_image);
-	fx_restore(vcpu->host_fx_image);
+	if (vcpu->fx_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);
diff -r 962b18bbd6d9 kernel/vmx.c
--- a/kernel/vmx.c	Thu Feb 15 13:58:59 2007 +0000
+++ b/kernel/vmx.c	Thu Feb 15 13:30:20 2007 -0600
@@ -1764,8 +1764,10 @@ again:
 	if (vcpu->guest_debug.enabled)
 		kvm_guest_debug_pre(vcpu);
 
-	fx_save(vcpu->host_fx_image);
-	fx_restore(vcpu->guest_fx_image);
+	if (vcpu->fx_active) {
+		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);
@@ -1894,8 +1896,11 @@ again:
 	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);
+	if (vcpu->fx_active) {
+		fx_save(vcpu->guest_fx_image);
+		fx_restore(vcpu->host_fx_image);
+	}
+
 	vcpu->interrupt_window_open = (vmcs_read32(GUEST_INTERRUPTIBILITY_INFO) & 3) == 0;
 
 	asm ("mov %0, %%ds; mov %0, %%es" : : "r"(__USER_DS));

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

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[-- 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 save/restore

[Rusty Russell]

On Thu, 2007-02-15 at 19:02 -0600, Anthony Liguori wrote:
> My microbenchmark of choice uses FPU operations so I think the results 
> are currently tainted.  I've only tested on a 32bit SVM system.

So did you have that virtbench patch for me? 8)

Rusty.



-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] Lazy FPU save/restore

[Avi Kivity]

Anthony Liguori wrote:
> The following patch saves the host FPU state and loads the guests FPU 
> state if !(CR0.MP == 1 && CR0.TS == 1).
>
> When CR0.MP == 1 && CR0.TS == 1, all FPU activity will generate 
> exceptions.  OS's use these exceptions to implement lazy FPU loading 
> to improve context switch time.  Since any FPU activity will generate 
> traps, we don't have to worry about the guest modifying the host FPU 
> state.
>
> My microbenchmark of choice uses FPU operations so I think the results 
> are currently tainted.  I've only tested on a 32bit SVM system.
>
> Signed-off-by: Anthony Liguori <anthony-rdkfGonbjUSkNkDKm+mE6A@public.gmane.org>
>

Applied, thanks.

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] Lazy FPU save/restore

[Avi Kivity]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> The following patch saves the host FPU state and loads the guests FPU 
>> state if !(CR0.MP == 1 && CR0.TS == 1).
>>
>> When CR0.MP == 1 && CR0.TS == 1, all FPU activity will generate 
>> exceptions.  OS's use these exceptions to implement lazy FPU loading 
>> to improve context switch time.  Since any FPU activity will generate 
>> traps, we don't have to worry about the guest modifying the host FPU 
>> state.
>>
>> My microbenchmark of choice uses FPU operations so I think the 
>> results are currently tainted.  I've only tested on a 32bit SVM system.
>>
>> Signed-off-by: Anthony Liguori <anthony-rdkfGonbjUSkNkDKm+mE6A@public.gmane.org>
>>
>
> Applied, thanks.
>

... and reverted.

Running an fpu load on both guest and host shows corruption (both on the 
guest and host).  The load is


> #include <stdio.h>
>
> void test_fpu()
> {
>     int i;
>     double f = 0;
>
>     for (i = 0; i < 10000000; ++i)
>     f += 1 / (1.0 + i);
>     printf("%20.16f\n", f);
> }
>
> int main(int ac, char **av)
> {
>     while (1)
>     test_fpu();
> }

Compiled statically with -O2 on 64-bit.  I don't know what's wrong -- 
the code looks correct (well, on Intel it wants KVM_GUEST_CR0_MASK to 
include CR0_MP_MASK and CR0_TS_MASK, but it fails with that too, and it 
fails on AMD as well).

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] Lazy FPU save/restore

[Anthony Liguori]

Avi Kivity wrote:
> Avi Kivity wrote:
>> Anthony Liguori wrote:
>>> The following patch saves the host FPU state and loads the guests 
>>> FPU state if !(CR0.MP == 1 && CR0.TS == 1).
>>>
>>> When CR0.MP == 1 && CR0.TS == 1, all FPU activity will generate 
>>> exceptions.  OS's use these exceptions to implement lazy FPU loading 
>>> to improve context switch time.  Since any FPU activity will 
>>> generate traps, we don't have to worry about the guest modifying the 
>>> host FPU state.
>>>
>>> My microbenchmark of choice uses FPU operations so I think the 
>>> results are currently tainted.  I've only tested on a 32bit SVM system.
>>>
>>> Signed-off-by: Anthony Liguori <anthony-rdkfGonbjUSkNkDKm+mE6A@public.gmane.org>
>>>
>>
>> Applied, thanks.
>>
>
> ... and reverted.
>
> Running an fpu load on both guest and host shows corruption (both on 
> the guest and host).  The load is
>
>
>> #include <stdio.h>
>>
>> void test_fpu()
>> {
>>     int i;
>>     double f = 0;
>>
>>     for (i = 0; i < 10000000; ++i)
>>     f += 1 / (1.0 + i);
>>     printf("%20.16f\n", f);
>> }
>>
>> int main(int ac, char **av)
>> {
>>     while (1)
>>     test_fpu();
>> }
>
> Compiled statically with -O2 on 64-bit.  I don't know what's wrong -- 
> the code looks correct (well, on Intel it wants KVM_GUEST_CR0_MASK to 
> include CR0_MP_MASK and CR0_TS_MASK, but it fails with that too, and 
> it fails on AMD as well).

I'll have to try and setup a 64 bit system.  I cannot reproduce on my 
32bit system with your test program.

Thanks for figuring this out!

Regards,

Anthony Liguori


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] Lazy FPU save/restore

[Avi Kivity]

Anthony Liguori wrote:
>>
>> ... and reverted.
>>
>> Running an fpu load on both guest and host shows corruption (both on 
>> the guest and host).  The load is
>>
>>
>>> #include <stdio.h>
>>>
>>> void test_fpu()
>>> {
>>>     int i;
>>>     double f = 0;
>>>
>>>     for (i = 0; i < 10000000; ++i)
>>>     f += 1 / (1.0 + i);
>>>     printf("%20.16f\n", f);
>>> }
>>>
>>> int main(int ac, char **av)
>>> {
>>>     while (1)
>>>     test_fpu();
>>> }
>>
>> Compiled statically with -O2 on 64-bit.  I don't know what's wrong -- 
>> the code looks correct (well, on Intel it wants KVM_GUEST_CR0_MASK to 
>> include CR0_MP_MASK and CR0_TS_MASK, but it fails with that too, and 
>> it fails on AMD as well).
>
> I'll have to try and setup a 64 bit system.  I cannot reproduce on my 
> 32bit system with your test program.

It may be related to 64-bit (as that uses sse for floating point), or to 
the fact that I use ssh on a remote host, which causes more context 
switches.  I'll try to reproduce on 32 bits with my setup.

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] Lazy FPU save/restore

[Avi Kivity]

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

Avi Kivity wrote:
>>
>> I'll have to try and setup a 64 bit system.  I cannot reproduce on my 
>> 32bit system with your test program.
>
> It may be related to 64-bit (as that uses sse for floating point), or 
> to the fact that I use ssh on a remote host, which causes more context 
> switches.  I'll try to reproduce on 32 bits with my setup.
>

It doesn't reproduce on a 32-bit host.  However, it does reproduce on a 
32-bit guest on a 64-bit host, and on a 32-bit user load running in a 
64-bit guest on a 64-bit host.

I'm attaching an updated test program which makes it easier to spot the 
failures.  You only need to run it on the guest, although running it on 
the host as well increases the failure rate significantly.

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


[-- Attachment #2: fpu.c --]
[-- Type: text/x-csrc, Size: 506 bytes --]


#include <stdio.h>
#include <math.h>

double test_fpu_once()
{
    int i;
    double f = 0;

    for (i = 0; i < 10000000; ++i)
	f += 1 / (1.0 + i);
    return f;
}

void test_fpu()
{
    double a, b;
    int runs;

    runs = 0;
    a = test_fpu_once();
    while (1) {
	b = test_fpu_once();
	if (fabs(a - b) > 1e-9)
	    printf("error: %20.16f -> %20.16f\n", a, b);
	a = b;
	if (++runs % 100 == 0)
	    printf("runs: %8d\n", runs);
    }
}

int main(int ac, char **av)
{
    test_fpu();
    return 0;
}

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

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[-- 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 save/restore

[Anthony Liguori]

Avi Kivity wrote:
> Avi Kivity wrote:
>>>
>>> I'll have to try and setup a 64 bit system.  I cannot reproduce on 
>>> my 32bit system with your test program.
>>
>> It may be related to 64-bit (as that uses sse for floating point), or 
>> to the fact that I use ssh on a remote host, which causes more 
>> context switches.  I'll try to reproduce on 32 bits with my setup.
>>
>
> It doesn't reproduce on a 32-bit host.  However, it does reproduce on 
> a 32-bit guest on a 64-bit host, and on a 32-bit user load running in 
> a 64-bit guest on a 64-bit host.

Thanks, I'll setup a 64 bit partition and start debugging.  I'm a bit 
confused though, I thought CR0.TS even covers SIMD instructions?

I'll take a look at Xen and see if it's doing something else.

Regards,

Anthony Liguori

> I'm attaching an updated test program which makes it easier to spot 
> the failures.  You only need to run it on the guest, although running 
> it on the host as well increases the failure rate significantly.
>
> ------------------------------------------------------------------------
>
>
> #include <stdio.h>
> #include <math.h>
>
> double test_fpu_once()
> {
>     int i;
>     double f = 0;
>
>     for (i = 0; i < 10000000; ++i)
> 	f += 1 / (1.0 + i);
>     return f;
> }
>
> void test_fpu()
> {
>     double a, b;
>     int runs;
>
>     runs = 0;
>     a = test_fpu_once();
>     while (1) {
> 	b = test_fpu_once();
> 	if (fabs(a - b) > 1e-9)
> 	    printf("error: %20.16f -> %20.16f\n", a, b);
> 	a = b;
> 	if (++runs % 100 == 0)
> 	    printf("runs: %8d\n", runs);
>     }
> }
>
> int main(int ac, char **av)
> {
>     test_fpu();
>     return 0;
> }
>   


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] Lazy FPU save/restore

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Avi Kivity wrote:
>>>>
>>>> I'll have to try and setup a 64 bit system.  I cannot reproduce on 
>>>> my 32bit system with your test program.
>>>
>>> It may be related to 64-bit (as that uses sse for floating point), 
>>> or to the fact that I use ssh on a remote host, which causes more 
>>> context switches.  I'll try to reproduce on 32 bits with my setup.
>>>
>>
>> It doesn't reproduce on a 32-bit host.  However, it does reproduce on 
>> a 32-bit guest on a 64-bit host, and on a 32-bit user load running in 
>> a 64-bit guest on a 64-bit host.
>
> Thanks, I'll setup a 64 bit partition and start debugging.  I'm a bit 
> confused though, I thought CR0.TS even covers SIMD instructions?

It does, and the fact that a 32-bit guest suffers from the same issue 
indicate that sse is not the cause.

Locking the vcpu to a cpu does not change the failures, so it isn't 
related to vcpu migration.

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV