[Patch] Allowing PV-OPS kernel to detect whether XSAVE is supported

[Patch] Allowing PV-OPS kernel to detect whether XSAVE is supported

[Haitao Shan]

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

Hi, Jeremy,

This patch allows pv-ops kernel to detect whether XSAVE is supported
(before masking it unconditionally through xen_cpuid).
Can you please have review? Thanks!

Signed-off-by: Shan Haitao <haitao.shan@intel.com>

Shan Haitao

[-- Attachment #2: xsave_pvops_kernel.patch --]
[-- Type: application/octet-stream, Size: 688 bytes --]

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index fd3803e..03bfaf7 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -252,6 +252,13 @@ static __init void xen_init_cpuid_mask(void)
 			  (1 << X86_FEATURE_MCA)  |  /* disable MCA */
 			  (1 << X86_FEATURE_APIC) |  /* disable local APIC */
 			  (1 << X86_FEATURE_ACPI));  /* disable ACPI */
+	ax = 1;
+	xen_cpuid(&ax, &bx, &cx, &dx);
+
+	/* Xen will set CR4.OSXSAVE if supported and not disabled by force */
+	if ( cx & (1 << (X86_FEATURE_XSAVE % 32)) &&
+		cx & (1 << (X86_FEATURE_OSXSAVE % 32)) )
+		return;
 
 	cpuid_leaf1_ecx_mask &= ~(1 << (X86_FEATURE_XSAVE % 32)); /* disable XSAVE */
 }

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [Patch] Allowing PV-OPS kernel to detect whether XSAVE is supported

[Ian Campbell]

On Tue, 2010-11-09 at 06:22 +0000, Haitao Shan wrote:
> Hi, Jeremy,
> 
> This patch allows pv-ops kernel to detect whether XSAVE is supported
> (before masking it unconditionally through xen_cpuid).
> Can you please have review? Thanks!
> 
> Signed-off-by: Shan Haitao <haitao.shan@intel.com>
> 
> Shan Haitao 
> 

> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index fd3803e..03bfaf7 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -252,6 +252,13 @@ static __init void xen_init_cpuid_mask(void)
>                           (1 << X86_FEATURE_MCA)  |  /* disable MCA */
>                           (1 << X86_FEATURE_APIC) |  /* disable local APIC */
>                           (1 << X86_FEATURE_ACPI));  /* disable ACPI */
> +       ax = 1;
> +       xen_cpuid(&ax, &bx, &cx, &dx);
> +
> +       /* Xen will set CR4.OSXSAVE if supported and not disabled by force */

For how long has the hypervisor had this behaviour? IIRC older
hypervisors did not correctly expose/mask the *XSAVE CPUID flags and
would causes PV guests to crash when they used *XSAVE features which
weren't actually available.

In other words have you confirmed that this patch does not break the
kernel running on older hypervisors such as Xen 4.0?


> +       if ( cx & (1 << (X86_FEATURE_XSAVE % 32)) &&
> +               cx & (1 << (X86_FEATURE_OSXSAVE % 32)) )
> +               return;
>  
>         cpuid_leaf1_ecx_mask &= ~(1 << (X86_FEATURE_XSAVE % 32)); /* disable XSAVE */
>  } 

Re: [Patch] Allowing PV-OPS kernel to detect whether XSAVE is supported

[Jan Beulich]

>>> On 09.11.10 at 11:43, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2010-11-09 at 06:22 +0000, Haitao Shan wrote:
>> Hi, Jeremy,
>> 
>> This patch allows pv-ops kernel to detect whether XSAVE is supported
>> (before masking it unconditionally through xen_cpuid).
>> Can you please have review? Thanks!
>> 
>> Signed-off-by: Shan Haitao <haitao.shan@intel.com>
>> 
>> Shan Haitao 
>> 
> 
>> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
>> index fd3803e..03bfaf7 100644
>> --- a/arch/x86/xen/enlighten.c
>> +++ b/arch/x86/xen/enlighten.c
>> @@ -252,6 +252,13 @@ static __init void xen_init_cpuid_mask(void)
>>                           (1 << X86_FEATURE_MCA)  |  /* disable MCA */
>>                           (1 << X86_FEATURE_APIC) |  /* disable local APIC */
>>                           (1 << X86_FEATURE_ACPI));  /* disable ACPI */
>> +       ax = 1;
>> +       xen_cpuid(&ax, &bx, &cx, &dx);
>> +
>> +       /* Xen will set CR4.OSXSAVE if supported and not disabled by force 
> */
> 
> For how long has the hypervisor had this behaviour? IIRC older
> hypervisors did not correctly expose/mask the *XSAVE CPUID flags and
> would causes PV guests to crash when they used *XSAVE features which
> weren't actually available.
> 
> In other words have you confirmed that this patch does not break the
> kernel running on older hypervisors such as Xen 4.0?

The problem was only with the XSAVE cpuid bit, not the OSXSAVE
one (which gets turned on only when CR4.OSXSAVE gets set).

Jan

>> +       if ( cx & (1 << (X86_FEATURE_XSAVE % 32)) &&
>> +               cx & (1 << (X86_FEATURE_OSXSAVE % 32)) )
>> +               return;
>>  
>>         cpuid_leaf1_ecx_mask &= ~(1 << (X86_FEATURE_XSAVE % 32)); /* disable 
> XSAVE */
>>  } 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com 
> http://lists.xensource.com/xen-devel 

Re: [Patch] Allowing PV-OPS kernel to detect whether XSAVE is supported

[Ian Campbell]

On Tue, 2010-11-09 at 10:51 +0000, Jan Beulich wrote:
> >>> On 09.11.10 at 11:43, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Tue, 2010-11-09 at 06:22 +0000, Haitao Shan wrote:
> >> Hi, Jeremy,
> >> 
> >> This patch allows pv-ops kernel to detect whether XSAVE is supported
> >> (before masking it unconditionally through xen_cpuid).
> >> Can you please have review? Thanks!
> >> 
> >> Signed-off-by: Shan Haitao <haitao.shan@intel.com>
> >> 
> >> Shan Haitao 
> >> 
> > 
> >> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> >> index fd3803e..03bfaf7 100644
> >> --- a/arch/x86/xen/enlighten.c
> >> +++ b/arch/x86/xen/enlighten.c
> >> @@ -252,6 +252,13 @@ static __init void xen_init_cpuid_mask(void)
> >>                           (1 << X86_FEATURE_MCA)  |  /* disable MCA */
> >>                           (1 << X86_FEATURE_APIC) |  /* disable local APIC */
> >>                           (1 << X86_FEATURE_ACPI));  /* disable ACPI */
> >> +       ax = 1;
> >> +       xen_cpuid(&ax, &bx, &cx, &dx);
> >> +
> >> +       /* Xen will set CR4.OSXSAVE if supported and not disabled by force 
> > */
> > 
> > For how long has the hypervisor had this behaviour? IIRC older
> > hypervisors did not correctly expose/mask the *XSAVE CPUID flags and
> > would causes PV guests to crash when they used *XSAVE features which
> > weren't actually available.
> > 
> > In other words have you confirmed that this patch does not break the
> > kernel running on older hypervisors such as Xen 4.0?
> 
> The problem was only with the XSAVE cpuid bit, not the OSXSAVE
> one (which gets turned on only when CR4.OSXSAVE gets set).

So if OSXSAVE is enabled we can also infer that XSAVE is safe to use,
because XSAVE was fixed/implemented before OSXSAVE was? Seems
reasonable.

Ian.

> 
> Jan
> 
> >> +       if ( cx & (1 << (X86_FEATURE_XSAVE % 32)) &&
> >> +               cx & (1 << (X86_FEATURE_OSXSAVE % 32)) )
> >> +               return;
> >>  
> >>         cpuid_leaf1_ecx_mask &= ~(1 << (X86_FEATURE_XSAVE % 32)); /* disable 
> > XSAVE */
> >>  } 
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com 
> > http://lists.xensource.com/xen-devel 
> 
> 
> 

Re: [Patch] Allowing PV-OPS kernel to detect whether XSAVE is supported

[Jan Beulich]

>>> On 09.11.10 at 11:54, Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
> So if OSXSAVE is enabled we can also infer that XSAVE is safe to use,
> because XSAVE was fixed/implemented before OSXSAVE was? Seems
> reasonable.

Yes, that's my understanding (prior to the recent changes, CR4.OSXSAVE
got turned on only in HVM context if I followed things correctly).

Jan

Re: [Patch] Allowing PV-OPS kernel to detect whether XSAVE is supported

[Haitao Shan]

Yes. According to spec, while CPUID leaf 1 ECX.XSAVE reports whether
processor supports XSAVE, CPUID leaf 1 ECX.OSXSAVE just reflects
CR4.OSXSAVE, which is served as an indication from kernel (and hence
VMM, in Xen's case) to apps (including PV kernels, in xen's case) that
XSAVE is enabled and can be used safely.

Shan Haitao

2010/11/9 Ian Campbell <Ian.Campbell@eu.citrix.com>:
> On Tue, 2010-11-09 at 10:51 +0000, Jan Beulich wrote:
>> >>> On 09.11.10 at 11:43, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Tue, 2010-11-09 at 06:22 +0000, Haitao Shan wrote:
>> >> Hi, Jeremy,
>> >>
>> >> This patch allows pv-ops kernel to detect whether XSAVE is supported
>> >> (before masking it unconditionally through xen_cpuid).
>> >> Can you please have review? Thanks!
>> >>
>> >> Signed-off-by: Shan Haitao <haitao.shan@intel.com>
>> >>
>> >> Shan Haitao
>> >>
>> >
>> >> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
>> >> index fd3803e..03bfaf7 100644
>> >> --- a/arch/x86/xen/enlighten.c
>> >> +++ b/arch/x86/xen/enlighten.c
>> >> @@ -252,6 +252,13 @@ static __init void xen_init_cpuid_mask(void)
>> >>                           (1 << X86_FEATURE_MCA)  |  /* disable MCA */
>> >>                           (1 << X86_FEATURE_APIC) |  /* disable local APIC */
>> >>                           (1 << X86_FEATURE_ACPI));  /* disable ACPI */
>> >> +       ax = 1;
>> >> +       xen_cpuid(&ax, &bx, &cx, &dx);
>> >> +
>> >> +       /* Xen will set CR4.OSXSAVE if supported and not disabled by force
>> > */
>> >
>> > For how long has the hypervisor had this behaviour? IIRC older
>> > hypervisors did not correctly expose/mask the *XSAVE CPUID flags and
>> > would causes PV guests to crash when they used *XSAVE features which
>> > weren't actually available.
>> >
>> > In other words have you confirmed that this patch does not break the
>> > kernel running on older hypervisors such as Xen 4.0?
>>
>> The problem was only with the XSAVE cpuid bit, not the OSXSAVE
>> one (which gets turned on only when CR4.OSXSAVE gets set).
>
> So if OSXSAVE is enabled we can also infer that XSAVE is safe to use,
> because XSAVE was fixed/implemented before OSXSAVE was? Seems
> reasonable.
>
> Ian.
>
>>
>> Jan
>>
>> >> +       if ( cx & (1 << (X86_FEATURE_XSAVE % 32)) &&
>> >> +               cx & (1 << (X86_FEATURE_OSXSAVE % 32)) )
>> >> +               return;
>> >>
>> >>         cpuid_leaf1_ecx_mask &= ~(1 << (X86_FEATURE_XSAVE % 32)); /* disable
>> > XSAVE */
>> >>  }
>> >
>> >
>> > _______________________________________________
>> > Xen-devel mailing list
>> > Xen-devel@lists.xensource.com
>> > http://lists.xensource.com/xen-devel
>>
>>
>>
>
>
>

Re: [Patch] Allowing PV-OPS kernel to detect whether XSAVE is supported

[Jeremy Fitzhardinge]

On 11/08/2010 10:22 PM, Haitao Shan wrote:
> Hi, Jeremy,
>
> This patch allows pv-ops kernel to detect whether XSAVE is supported
> (before masking it unconditionally through xen_cpuid).
> Can you please have review? Thanks!
>
> Signed-off-by: Shan Haitao <haitao.shan@intel.com>
>
> Shan Haitao
>

For future reference:

Please post patches inline if possible.

If you must use an attachment to prevent your mail system from
corrupting the patch, please include a complete description of the patch
(what is it trying to do, how does it do it, what is the outcome?) with
signed-off-bys in the the patch itself.

> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c index
> fd3803e..03bfaf7 100644 --- a/arch/x86/xen/enlighten.c +++
> b/arch/x86/xen/enlighten.c @@ -252,6 +252,13 @@ static __init void
> xen_init_cpuid_mask(void) (1 << X86_FEATURE_MCA) | /* disable MCA */
> (1 << X86_FEATURE_APIC) | /* disable local APIC */ (1 <<
> X86_FEATURE_ACPI)); /* disable ACPI */ + ax = 1; + xen_cpuid(&ax, &bx,
> &cx, &dx); + + /* Xen will set CR4.OSXSAVE if supported and not
> disabled by force */ + if ( cx & (1 << (X86_FEATURE_XSAVE % 32)) && +
> cx & (1 << (X86_FEATURE_OSXSAVE % 32)) ) + return;
> cpuid_leaf1_ecx_mask &= ~(1 << (X86_FEATURE_XSAVE % 32)); /* disable
> XSAVE */ 


I cleaned this up a bit (fixed formatting to Linux style, reversed the
sense of the if() and masked OSXSAVE as well, even though it won't make
a difference).  But I'm still a bit concerned about the back-compat
issues around this.

What happens if the guest OS does not support XSAVE (older versions of
Linux)?  Could usermode code see OSXSAVE set in the cpuid feature flags
and attempt to use XSAVE, even if XSAVE isn't set?  In other words, can
usermode ever normally see OSXSAVE set, but XSAVE clear?

We can't mask OSXSAVE to usermode because they can run the naked CPUID
instruction, so I guess the only way to cause it to be cleared would be
to clear CR4.OSXSAVE?  But that seems like a very expensive thing to do
on a vcpu context switch.

How much testing of real usermode code have you done with this?  How
many combinations of XSAVE support in Xen, Linux and usermode have you
tried?

    J

Re: [Patch] Allowing PV-OPS kernel to detect whether XSAVE is supported

[Haitao Shan]

2010/11/10 Jeremy Fitzhardinge <jeremy@goop.org>:
> On 11/08/2010 10:22 PM, Haitao Shan wrote:
>> Hi, Jeremy,
>>
>> This patch allows pv-ops kernel to detect whether XSAVE is supported
>> (before masking it unconditionally through xen_cpuid).
>> Can you please have review? Thanks!
>>
>> Signed-off-by: Shan Haitao <haitao.shan@intel.com>
>>
>> Shan Haitao
>>
>
> For future reference:
>
> Please post patches inline if possible.
>
> If you must use an attachment to prevent your mail system from
> corrupting the patch, please include a complete description of the patch
> (what is it trying to do, how does it do it, what is the outcome?) with
> signed-off-bys in the the patch itself.
OK. I will follow.

>
>> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c index
>> fd3803e..03bfaf7 100644 --- a/arch/x86/xen/enlighten.c +++
>> b/arch/x86/xen/enlighten.c @@ -252,6 +252,13 @@ static __init void
>> xen_init_cpuid_mask(void) (1 << X86_FEATURE_MCA) | /* disable MCA */
>> (1 << X86_FEATURE_APIC) | /* disable local APIC */ (1 <<
>> X86_FEATURE_ACPI)); /* disable ACPI */ + ax = 1; + xen_cpuid(&ax, &bx,
>> &cx, &dx); + + /* Xen will set CR4.OSXSAVE if supported and not
>> disabled by force */ + if ( cx & (1 << (X86_FEATURE_XSAVE % 32)) && +
>> cx & (1 << (X86_FEATURE_OSXSAVE % 32)) ) + return;
>> cpuid_leaf1_ecx_mask &= ~(1 << (X86_FEATURE_XSAVE % 32)); /* disable
>> XSAVE */
>
>
> I cleaned this up a bit (fixed formatting to Linux style, reversed the
> sense of the if() and masked OSXSAVE as well, even though it won't make
> a difference).  But I'm still a bit concerned about the back-compat
> issues around this.
>
> What happens if the guest OS does not support XSAVE (older versions of
> Linux)?  Could usermode code see OSXSAVE set in the cpuid feature flags
> and attempt to use XSAVE, even if XSAVE isn't set?  In other words, can
> usermode ever normally see OSXSAVE set, but XSAVE clear?
I think that is not possible. If usermode does not use native CPUID
instruction, or CPUID can be virtualized any way, this won't happen.
Otherwise, usermode will either see both are set or unset. Since on a
NON-XSAVE processor, you can not set CR4.OSXSAVE and won't be
reflected to CPUID.

If user mode sees both are set, they can use it actually. So we
initially set FP and SSE in XCR0 for guest. If user mode wants to use
it, guest kernel still can manage the state using traditional FPU
save/restore mechanism. If user mode wants to access more extended
states, it has to acquire kernel's support for turning on related bit
in XCR0, which is finally controlled by Xen now.

That's the reason I chose to turn on OSXSAVE in Xen and don't
dynamically change it.

>
> We can't mask OSXSAVE to usermode because they can run the naked CPUID
> instruction, so I guess the only way to cause it to be cleared would be
> to clear CR4.OSXSAVE?  But that seems like a very expensive thing to do
> on a vcpu context switch.
Yes. This is the biggest concern when I wrote XSAVE patches.
Otherwise, you will need to switch CR4 on entry to/exit from guest
mode. Or as you said, switching CR4 when do a vcpu context switch, but
whenever Xen itself want to use XSAVE instructions, it needs to turns
on it and off it on the fly.

>
> How much testing of real usermode code have you done with this?  How
> many combinations of XSAVE support in Xen, Linux and usermode have you
> tried?
I have a few AVX test programs running both inside PV guest and HVM.
However, I have to admit that my aim is to test Xen's fpu context
switching using Xsave and guest save/restore.

>
>    J
>

Re: [Patch] Allowing PV-OPS kernel to detect whether XSAVE is supported

[Haitao Shan]

> Otherwise, usermode will either see both are set or unset. Since on a

Oh, that is not correct. I mean whenever user mode sees OSXSAVE, it
sees both are set or unset. If user mode sees XSAVE only, that is
legal of course.

Shan Haitao

2010/11/10 Haitao Shan <maillists.shan@gmail.com>:
> 2010/11/10 Jeremy Fitzhardinge <jeremy@goop.org>:
>> On 11/08/2010 10:22 PM, Haitao Shan wrote:
>>> Hi, Jeremy,
>>>
>>> This patch allows pv-ops kernel to detect whether XSAVE is supported
>>> (before masking it unconditionally through xen_cpuid).
>>> Can you please have review? Thanks!
>>>
>>> Signed-off-by: Shan Haitao <haitao.shan@intel.com>
>>>
>>> Shan Haitao
>>>
>>
>> For future reference:
>>
>> Please post patches inline if possible.
>>
>> If you must use an attachment to prevent your mail system from
>> corrupting the patch, please include a complete description of the patch
>> (what is it trying to do, how does it do it, what is the outcome?) with
>> signed-off-bys in the the patch itself.
> OK. I will follow.
>
>>
>>> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c index
>>> fd3803e..03bfaf7 100644 --- a/arch/x86/xen/enlighten.c +++
>>> b/arch/x86/xen/enlighten.c @@ -252,6 +252,13 @@ static __init void
>>> xen_init_cpuid_mask(void) (1 << X86_FEATURE_MCA) | /* disable MCA */
>>> (1 << X86_FEATURE_APIC) | /* disable local APIC */ (1 <<
>>> X86_FEATURE_ACPI)); /* disable ACPI */ + ax = 1; + xen_cpuid(&ax, &bx,
>>> &cx, &dx); + + /* Xen will set CR4.OSXSAVE if supported and not
>>> disabled by force */ + if ( cx & (1 << (X86_FEATURE_XSAVE % 32)) && +
>>> cx & (1 << (X86_FEATURE_OSXSAVE % 32)) ) + return;
>>> cpuid_leaf1_ecx_mask &= ~(1 << (X86_FEATURE_XSAVE % 32)); /* disable
>>> XSAVE */
>>
>>
>> I cleaned this up a bit (fixed formatting to Linux style, reversed the
>> sense of the if() and masked OSXSAVE as well, even though it won't make
>> a difference).  But I'm still a bit concerned about the back-compat
>> issues around this.
>>
>> What happens if the guest OS does not support XSAVE (older versions of
>> Linux)?  Could usermode code see OSXSAVE set in the cpuid feature flags
>> and attempt to use XSAVE, even if XSAVE isn't set?  In other words, can
>> usermode ever normally see OSXSAVE set, but XSAVE clear?
> I think that is not possible. If usermode does not use native CPUID
> instruction, or CPUID can be virtualized any way, this won't happen.
> Otherwise, usermode will either see both are set or unset. Since on a
> NON-XSAVE processor, you can not set CR4.OSXSAVE and won't be
> reflected to CPUID.
>
> If user mode sees both are set, they can use it actually. So we
> initially set FP and SSE in XCR0 for guest. If user mode wants to use
> it, guest kernel still can manage the state using traditional FPU
> save/restore mechanism. If user mode wants to access more extended
> states, it has to acquire kernel's support for turning on related bit
> in XCR0, which is finally controlled by Xen now.
>
> That's the reason I chose to turn on OSXSAVE in Xen and don't
> dynamically change it.
>
>>
>> We can't mask OSXSAVE to usermode because they can run the naked CPUID
>> instruction, so I guess the only way to cause it to be cleared would be
>> to clear CR4.OSXSAVE?  But that seems like a very expensive thing to do
>> on a vcpu context switch.
> Yes. This is the biggest concern when I wrote XSAVE patches.
> Otherwise, you will need to switch CR4 on entry to/exit from guest
> mode. Or as you said, switching CR4 when do a vcpu context switch, but
> whenever Xen itself want to use XSAVE instructions, it needs to turns
> on it and off it on the fly.
>
>>
>> How much testing of real usermode code have you done with this?  How
>> many combinations of XSAVE support in Xen, Linux and usermode have you
>> tried?
> I have a few AVX test programs running both inside PV guest and HVM.
> However, I have to admit that my aim is to test Xen's fpu context
> switching using Xsave and guest save/restore.
>
>>
>>    J
>>
>

Re: [Patch] Allowing PV-OPS kernel to detect whether XSAVE is supported

[Jeremy Fitzhardinge]

On 11/09/2010 04:15 PM, Haitao Shan wrote:
>
> I think that is not possible. If usermode does not use native CPUID
> instruction, or CPUID can be virtualized any way, this won't happen.
> Otherwise, usermode will either see both are set or unset. Since on a
> NON-XSAVE processor, you can not set CR4.OSXSAVE and won't be
> reflected to CPUID.

I see.

> If user mode sees both are set, they can use it actually. So we
> initially set FP and SSE in XCR0 for guest. If user mode wants to use
> it, guest kernel still can manage the state using traditional FPU
> save/restore mechanism. If user mode wants to access more extended
> states, it has to acquire kernel's support for turning on related bit
> in XCR0, which is finally controlled by Xen now.

Are you saying that usermode can use XSAVE, AVX and other instruction
set extensions successfully if Xen supports them, even if the guest OS
doesn't?  That sounds unlikely - what happens when, for example, an old
Linux wants to context switch from one Linux task to another on a VCPU? 
How will the task's context get saved/reloaded properly?

Your kernel changes seem fine, and should allow a modern kernel to
support XSAVE and all the CPU features that go along with it.  But I'm
really concerned that your Xen changes will cause previously working
usermode programs to start erroneously using XSAVE when the guest kernel
cannot support it.

> I have a few AVX test programs running both inside PV guest and HVM.
> However, I have to admit that my aim is to test Xen's fpu context
> switching using Xsave and guest save/restore.
>

Could you make them available for testing?

IanJ: It looks like it would be useful to add some tests to the suite to
make sure all this extended context is save/restored properly...

Thanks,
    J

Re: [Patch] Allowing PV-OPS kernel to detect whether XSAVE is supported

[Haitao Shan]

>> If user mode sees both are set, they can use it actually. So we
>> initially set FP and SSE in XCR0 for guest. If user mode wants to use
>> it, guest kernel still can manage the state using traditional FPU
>> save/restore mechanism. If user mode wants to access more extended
>> states, it has to acquire kernel's support for turning on related bit
>> in XCR0, which is finally controlled by Xen now.
>
> Are you saying that usermode can use XSAVE, AVX and other instruction
> set extensions successfully if Xen supports them, even if the guest OS
> doesn't?  That sounds unlikely - what happens when, for example, an old
> Linux wants to context switch from one Linux task to another on a VCPU?
> How will the task's context get saved/reloaded properly?
Actually not. I mean user mode can use XSAVE instruction itself but
not AVX or any future instruction extensions. Using the latter would
require setting XCR0 properly, which is owned and managed by Xen
itself. As I said, initially Xen sets XCR0 to be FPU/SSE only for
guest (we do do XCR0 switch when vcpu context switch, this is a
per-vcpu setting). Unless guest kernel requires to enable more
extended states through XSETBV (this is ROOT ring 0 instruction only),
it won't be changed.
And in this case, guest kernel can still use original FXSAVE mechanism
to provide context switching support to its user space apps.
It just means user mode can use XSAVE instruction if it wans to use
this instruction to do save/restore itself.
If user mode wants more, it has to check kernel's SW interface support
for turning on more extended states management (via XSETBV in kernel
finally). But old kernels just do not have such kind of interface.

>
> Your kernel changes seem fine, and should allow a modern kernel to
> support XSAVE and all the CPU features that go along with it.  But I'm
> really concerned that your Xen changes will cause previously working
> usermode programs to start erroneously using XSAVE when the guest kernel
> cannot support it.
Well, I am concerned too. But I still think the current approach
should be architecturally viable. But there may be bugs in my code
........

>
>> I have a few AVX test programs running both inside PV guest and HVM.
>> However, I have to admit that my aim is to test Xen's fpu context
>> switching using Xsave and guest save/restore.
>>
>
> Could you make them available for testing?
I will return to you later for answer to this question. I needs to
judge the correct Intel policy and channel when doing such a release.

>
> IanJ: It looks like it would be useful to add some tests to the suite to
> make sure all this extended context is save/restored properly...
>
> Thanks,
>    J
>

Re: [Patch] Allowing PV-OPS kernel to detect whether XSAVE is supported

[Jeremy Fitzhardinge]

On 11/09/2010 05:45 PM, Haitao Shan wrote:
>>> If user mode sees both are set, they can use it actually. So we
>>> initially set FP and SSE in XCR0 for guest. If user mode wants to use
>>> it, guest kernel still can manage the state using traditional FPU
>>> save/restore mechanism. If user mode wants to access more extended
>>> states, it has to acquire kernel's support for turning on related bit
>>> in XCR0, which is finally controlled by Xen now.
>> Are you saying that usermode can use XSAVE, AVX and other instruction
>> set extensions successfully if Xen supports them, even if the guest OS
>> doesn't?  That sounds unlikely - what happens when, for example, an old
>> Linux wants to context switch from one Linux task to another on a VCPU?
>> How will the task's context get saved/reloaded properly?
> Actually not. I mean user mode can use XSAVE instruction itself but
> not AVX or any future instruction extensions. Using the latter would
> require setting XCR0 properly, which is owned and managed by Xen
> itself. As I said, initially Xen sets XCR0 to be FPU/SSE only for
> guest (we do do XCR0 switch when vcpu context switch, this is a
> per-vcpu setting). Unless guest kernel requires to enable more
> extended states through XSETBV (this is ROOT ring 0 instruction only),
> it won't be changed.

Ah, OK.

>> Your kernel changes seem fine, and should allow a modern kernel to
>> support XSAVE and all the CPU features that go along with it.  But I'm
>> really concerned that your Xen changes will cause previously working
>> usermode programs to start erroneously using XSAVE when the guest kernel
>> cannot support it.
> Well, I am concerned too. But I still think the current approach
> should be architecturally viable. But there may be bugs in my code
> ........

Well, that's not as bad as baking in a flawed interface.

>>> I have a few AVX test programs running both inside PV guest and HVM.
>>> However, I have to admit that my aim is to test Xen's fpu context
>>> switching using Xsave and guest save/restore.
>>>
>> Could you make them available for testing?
> I will return to you later for answer to this question. I needs to
> judge the correct Intel policy and channel when doing such a release.

Well, if you can't release your actual test programs (which might not be
appropriate in any case), then perhaps you could write something new to
exercise this?  Something that would enable as many instruction
extensions as possible and continually makes sure the contexts are being
properly saved and restored?

Thanks,
    J

Re: [Patch] Allowing PV-OPS kernel to detect whether XSAVE is supported

[Ian Jackson]

Jeremy Fitzhardinge writes ("Re: [Xen-devel] [Patch] Allowing PV-OPS kernel to detect whether XSAVE is supported"):
> Well, if you can't release your actual test programs (which might not be
> appropriate in any case), then perhaps you could write something new to
> exercise this?  Something that would enable as many instruction
> extensions as possible and continually makes sure the contexts are being
> properly saved and restored?

This would be a very good idea.

Ian.