Xen devel - Oct 2010 - [Patch 2/4] Refining Xsave/Xrestore support

Hi, Keir,

This is patch #2, which adds PV guest Xsave support.

Shan Haitao


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

>@@ -1367,6 +1384,8 @@ static void __context_switch(void)
>         memcpy(stack_regs,
>                &n->arch.guest_context.user_regs,
>                CTXT_SWITCH_STACK_BYTES);
>+        if ( cpu_has_xsave )
>+            set_xcr0(n->arch.xcr0);
How slow is a write to xcr0? I.e. is it worth checking whether
current xcr0 matches the to be written value?

Jan


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

On 10/27/2010 12:04 AM, Haitao Shan wrote:
> Hi, Keir,
>
> This is patch #2, which adds PV guest Xsave support.
How does a PV guest know whether Xsave support is available?  Previous
versions of Xen left the xsave cpu feature flag set even though xsave
wasn''t usable by the domain, so I had to forceably mask it from the
cpuid features within the domain.  Given that a PV domain can''t rely on
X86_FEATURE_XSAVE, how can it tell that the feature is actually usable?

Thanks,
    J

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

I don''t know but I suppose this won''t be a costing instruction
at
lease no larger than writing to other CRx registers. But agreed that a
comparison to minimize the cost would be great. I can add that.

Thanks for pointing out.

Shan Haitao

2010/10/27 Jan Beulich <JBeulich@novell.com>:
>>@@ -1367,6 +1384,8 @@ static void __context_switch(void)
>>         memcpy(stack_regs,
>>                &n->arch.guest_context.user_regs,
>>                CTXT_SWITCH_STACK_BYTES);
>>+        if ( cpu_has_xsave )
>>+            set_xcr0(n->arch.xcr0);
>
> How slow is a write to xcr0? I.e. is it worth checking whether
> current xcr0 matches the to be written value?
>
> Jan
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi, Jeremy,

My approach is based an old PV-OPS kernel source. Kernel tries to set
CR4.OSXSAVE and read it back to determine whether Xsave is actually
available.
Could you still use that?

Shan Haitao

2010/10/28 Jeremy Fitzhardinge <jeremy@goop.org>:
>  On 10/27/2010 12:04 AM, Haitao Shan wrote:
>> Hi, Keir,
>>
>> This is patch #2, which adds PV guest Xsave support.
>
> How does a PV guest know whether Xsave support is available?  Previous
> versions of Xen left the xsave cpu feature flag set even though xsave
> wasn''t usable by the domain, so I had to forceably mask it from
the
> cpuid features within the domain.  Given that a PV domain can''t
rely on
> X86_FEATURE_XSAVE, how can it tell that the feature is actually usable?
>
> Thanks,
>    J
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi, Jan,

This is the updated patch#2. Thanks.

Shan Haitao

2010/10/27 Jan Beulich <JBeulich@novell.com>:
>>@@ -1367,6 +1384,8 @@ static void __context_switch(void)
>>         memcpy(stack_regs,
>>                &n->arch.guest_context.user_regs,
>>                CTXT_SWITCH_STACK_BYTES);
>>+        if ( cpu_has_xsave )
>>+            set_xcr0(n->arch.xcr0);
>
> How slow is a write to xcr0? I.e. is it worth checking whether
> current xcr0 matches the to be written value?
>
> Jan
>
>

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

>>> On 28.10.10 at 06:58, Haitao Shan <maillists.shan@gmail.com>
wrote:
> This is the updated patch#2. Thanks.
Sorry, but this is worse than not checking at all: You didn''t consider
the idle vCPU case here, and hence you may end up having more
features enabled in xcr0 for a guest than it should have.

Also I only now noticed that you''re leaking the xsave_area allocation
in vcpu_initialize() if hvm_vcpu_initialise() fails.

Jan


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

>>> On 28.10.10 at 04:57, Haitao Shan <maillists.shan@gmail.com>
wrote:
> My approach is based an old PV-OPS kernel source. Kernel tries to set
> CR4.OSXSAVE and read it back to determine whether Xsave is actually
> available.
Hmm, in our kernels we check the OSXSAVE feature bit instead,
expecting (just like for FXSAVE) the hypervisor to enable the bit
in CR4 when it supports the feature.

I think a mechanism needs to be found in the hypervisor that
accommodates both current pv-ops and current forward port
kernels'' behaviors.

Jan
> 2010/10/28 Jeremy Fitzhardinge <jeremy@goop.org>:
>>  On 10/27/2010 12:04 AM, Haitao Shan wrote:
>>> Hi, Keir,
>>>
>>> This is patch #2, which adds PV guest Xsave support.
>>
>> How does a PV guest know whether Xsave support is available?  Previous
>> versions of Xen left the xsave cpu feature flag set even though xsave
>> wasn''t usable by the domain, so I had to forceably mask it
from the
>> cpuid features within the domain.  Given that a PV domain
can''t rely on
>> X86_FEATURE_XSAVE, how can it tell that the feature is actually usable?
>>
>> Thanks,
>>    J
>>
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com 
> http://lists.xensource.com/xen-devel 



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

Then I would prefer to write XCR0 unconditionally. Otherwise, I can
only refer to the approach for handling CR4 switches: reading CR4
first and checking whether there is a need to write actually.
But I don''t think <a read to XCR0 plus a data comparison> can
save any
compared with one unconditional write to XCR0.
Are you OK with this?

Thanks for pointing out the memory leak when hvm_vcpu_initialize
fails. I will update accordingly.

Shan Haitao

2010/10/28 Jan Beulich <JBeulich@novell.com>:
>>>> On 28.10.10 at 06:58, Haitao Shan
<maillists.shan@gmail.com> wrote:
>> This is the updated patch#2. Thanks.
>
> Sorry, but this is worse than not checking at all: You didn''t
consider
> the idle vCPU case here, and hence you may end up having more
> features enabled in xcr0 for a guest than it should have.
>
> Also I only now noticed that you''re leaking the xsave_area
allocation
> in vcpu_initialize() if hvm_vcpu_initialise() fails.
>
> Jan
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

In my patch, if processor supports CR4.OSXSAVE, Xen will enable it and
won''t clear it as long as we are in ROOT mode.

Shan Haitao

2010/10/28 Jan Beulich <JBeulich@novell.com>:
>>>> On 28.10.10 at 04:57, Haitao Shan
<maillists.shan@gmail.com> wrote:
>> My approach is based an old PV-OPS kernel source. Kernel tries to set
>> CR4.OSXSAVE and read it back to determine whether Xsave is actually
>> available.
>
> Hmm, in our kernels we check the OSXSAVE feature bit instead,
> expecting (just like for FXSAVE) the hypervisor to enable the bit
> in CR4 when it supports the feature.
>
> I think a mechanism needs to be found in the hypervisor that
> accommodates both current pv-ops and current forward port
> kernels'' behaviors.
>
> Jan
>
>> 2010/10/28 Jeremy Fitzhardinge <jeremy@goop.org>:
>>>  On 10/27/2010 12:04 AM, Haitao Shan wrote:
>>>> Hi, Keir,
>>>>
>>>> This is patch #2, which adds PV guest Xsave support.
>>>
>>> How does a PV guest know whether Xsave support is available?
 Previous
>>> versions of Xen left the xsave cpu feature flag set even though
xsave
>>> wasn''t usable by the domain, so I had to forceably mask it
from the
>>> cpuid features within the domain.  Given that a PV domain
can''t rely on
>>> X86_FEATURE_XSAVE, how can it tell that the feature is actually
usable?
>>>
>>> Thanks,
>>>    J
>>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> On 28.10.10 at 09:55, Haitao Shan <maillists.shan@gmail.com>
wrote:
> In my patch, if processor supports CR4.OSXSAVE, Xen will enable it and
> won''t clear it as long as we are in ROOT mode.
Ah, okay. There''s one problem with this, however - a pv domain the
kernel of which doesn''t support xsave would still see CPUID report
the OSXSAVE bit set, and hence applications could be lead to
believe they can use the wider registers. I realize this also puts
under question our model of having the kernel look at CPUID''s
OSXSAVE bit, but that could possibly be dealt with by having
pv_cpuid() (and perhaps xc_cpuid_pv_policy()) set the bit in case
Xen supports xsave.

Jan


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

>>> On 28.10.10 at 09:52, Haitao Shan <maillists.shan@gmail.com>
wrote:
> Then I would prefer to write XCR0 unconditionally. Otherwise, I can
> only refer to the approach for handling CR4 switches: reading CR4
> first and checking whether there is a need to write actually.
> But I don''t think <a read to XCR0 plus a data comparison>
can save any
> compared with one unconditional write to XCR0.
> Are you OK with this?
Depends on the performance expectations of xsetbv and xgetbv
(and its comparison to moves from/to control registers). At least
there''s no word in the documentation that xsetbv would be
serializing. I would hope Intel could at least provide approximate
numbers...

Jan


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

I once considered this problem, too. Originally, I suppose I can use
CR4 switching when entering/leaving PV guest, as what HW is doing for
us in VMX. But I suspect this will bring a lot of overhead.
Later I picked the current implementation, hoping guest applications
can have a check on XCR0 first before it uses extended states. And
according to spec, in order to use extend states, XCR0 must be set,
but this is a ring 0 instruction only.

Shan Haitao

2010/10/28 Jan Beulich <JBeulich@novell.com>:
>>>> On 28.10.10 at 09:55, Haitao Shan
<maillists.shan@gmail.com> wrote:
>> In my patch, if processor supports CR4.OSXSAVE, Xen will enable it and
>> won''t clear it as long as we are in ROOT mode.
>
> Ah, okay. There''s one problem with this, however - a pv domain the
> kernel of which doesn''t support xsave would still see CPUID report
> the OSXSAVE bit set, and hence applications could be lead to
> believe they can use the wider registers. I realize this also puts
> under question our model of having the kernel look at CPUID''s
> OSXSAVE bit, but that could possibly be dealt with by having
> pv_cpuid() (and perhaps xc_cpuid_pv_policy()) set the bit in case
> Xen supports xsave.
>
> Jan
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> On 28.10.10 at 10:33, Haitao Shan <maillists.shan@gmail.com>
wrote:
> I once considered this problem, too. Originally, I suppose I can use
> CR4 switching when entering/leaving PV guest, as what HW is doing for
> us in VMX. But I suspect this will bring a lot of overhead.
> Later I picked the current implementation, hoping guest applications
> can have a check on XCR0 first before it uses extended states. And
> according to spec, in order to use extend states, XCR0 must be set,
> but this is a ring 0 instruction only.
Ah, yes, that''s correct of course. So I guess pv-ops could use the
CPUID.OSXSAVE approach then, too.

Jan


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

I have done a quick test on my box, setting XCR0 is around 130 tsc
cycles while reading XCR0 is around 80 tsc cycles.

Shan Haitao

2010/10/28 Jan Beulich <JBeulich@novell.com>:
>>>> On 28.10.10 at 09:52, Haitao Shan
<maillists.shan@gmail.com> wrote:
>> Then I would prefer to write XCR0 unconditionally. Otherwise, I can
>> only refer to the approach for handling CR4 switches: reading CR4
>> first and checking whether there is a need to write actually.
>> But I don''t think <a read to XCR0 plus a data
comparison> can save any
>> compared with one unconditional write to XCR0.
>> Are you OK with this?
>
> Depends on the performance expectations of xsetbv and xgetbv
> (and its comparison to moves from/to control registers). At least
> there''s no word in the documentation that xsetbv would be
> serializing. I would hope Intel could at least provide approximate
> numbers...
>
> Jan
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 28/10/2010 08:52, "Haitao Shan" <maillists.shan@gmail.com>
wrote:
> Then I would prefer to write XCR0 unconditionally. Otherwise, I can
> only refer to the approach for handling CR4 switches: reading CR4
> first and checking whether there is a need to write actually.
> But I don''t think <a read to XCR0 plus a data comparison>
can save any
> compared with one unconditional write to XCR0.
> Are you OK with this?
Note that read_cr4() actually returns a cached copy of cr4, as stashed by
write_cr4(). You should use the same trick for XCR0, and then do the
cached-read-and-compare on context switch, again just as we do for cr4.

 -- Keir
> Thanks for pointing out the memory leak when hvm_vcpu_initialize
> fails. I will update accordingly.
> 
> Shan Haitao
> 
> 2010/10/28 Jan Beulich <JBeulich@novell.com>:
>>>>> On 28.10.10 at 06:58, Haitao Shan
<maillists.shan@gmail.com> wrote:
>>> This is the updated patch#2. Thanks.
>> 
>> Sorry, but this is worse than not checking at all: You didn''t
consider
>> the idle vCPU case here, and hence you may end up having more
>> features enabled in xcr0 for a guest than it should have.
>> 
>> Also I only now noticed that you''re leaking the xsave_area
allocation
>> in vcpu_initialize() if hvm_vcpu_initialise() fails.
>> 
>> Jan
>> 
>> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


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

Thanks! I will update and send the patch out later.

Shan Haitao

2010/10/28 Keir Fraser <keir@xen.org>:
> On 28/10/2010 08:52, "Haitao Shan"
<maillists.shan@gmail.com> wrote:
>
>> Then I would prefer to write XCR0 unconditionally. Otherwise, I can
>> only refer to the approach for handling CR4 switches: reading CR4
>> first and checking whether there is a need to write actually.
>> But I don''t think <a read to XCR0 plus a data
comparison> can save any
>> compared with one unconditional write to XCR0.
>> Are you OK with this?
>
> Note that read_cr4() actually returns a cached copy of cr4, as stashed by
> write_cr4(). You should use the same trick for XCR0, and then do the
> cached-read-and-compare on context switch, again just as we do for cr4.
>
>  -- Keir
>
>> Thanks for pointing out the memory leak when hvm_vcpu_initialize
>> fails. I will update accordingly.
>>
>> Shan Haitao
>>
>> 2010/10/28 Jan Beulich <JBeulich@novell.com>:
>>>>>> On 28.10.10 at 06:58, Haitao Shan
<maillists.shan@gmail.com> wrote:
>>>> This is the updated patch#2. Thanks.
>>>
>>> Sorry, but this is worse than not checking at all: You
didn''t consider
>>> the idle vCPU case here, and hence you may end up having more
>>> features enabled in xcr0 for a guest than it should have.
>>>
>>> Also I only now noticed that you''re leaking the xsave_area
allocation
>>> in vcpu_initialize() if hvm_vcpu_initialise() fails.
>>>
>>> Jan
>>>
>>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 10/27/2010 07:57 PM, Haitao Shan wrote:
> Hi, Jeremy,
>
> My approach is based an old PV-OPS kernel source. Kernel tries to set
> CR4.OSXSAVE and read it back to determine whether Xsave is actually
> available.
> Could you still use that?
No, because some older versions of Xen kill the domain when they try to
set unknown bits in CR4.

    J
> Shan Haitao
>
> 2010/10/28 Jeremy Fitzhardinge <jeremy@goop.org>:
>>  On 10/27/2010 12:04 AM, Haitao Shan wrote:
>>> Hi, Keir,
>>>
>>> This is patch #2, which adds PV guest Xsave support.
>> How does a PV guest know whether Xsave support is available?  Previous
>> versions of Xen left the xsave cpu feature flag set even though xsave
>> wasn''t usable by the domain, so I had to forceably mask it
from the
>> cpuid features within the domain.  Given that a PV domain
can''t rely on
>> X86_FEATURE_XSAVE, how can it tell that the feature is actually usable?
>>
>> Thanks,
>>    J
>>

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

Then what about the approach that Jan uses in 2.6.27 tree, which is
reading CPUID-OSXSAVE? If supported, Xen will always set CR4.OSXSAVE.

Shan Haitao

2010/10/29 Jeremy Fitzhardinge <jeremy@goop.org>:
>  On 10/27/2010 07:57 PM, Haitao Shan wrote:
>> Hi, Jeremy,
>>
>> My approach is based an old PV-OPS kernel source. Kernel tries to set
>> CR4.OSXSAVE and read it back to determine whether Xsave is actually
>> available.
>> Could you still use that?
>
> No, because some older versions of Xen kill the domain when they try to
> set unknown bits in CR4.
>
>    J
>
>> Shan Haitao
>>
>> 2010/10/28 Jeremy Fitzhardinge <jeremy@goop.org>:
>>>  On 10/27/2010 12:04 AM, Haitao Shan wrote:
>>>> Hi, Keir,
>>>>
>>>> This is patch #2, which adds PV guest Xsave support.
>>> How does a PV guest know whether Xsave support is available?
 Previous
>>> versions of Xen left the xsave cpu feature flag set even though
xsave
>>> wasn''t usable by the domain, so I had to forceably mask it
from the
>>> cpuid features within the domain.  Given that a PV domain
can''t rely on
>>> X86_FEATURE_XSAVE, how can it tell that the feature is actually
usable?
>>>
>>> Thanks,
>>>    J
>>>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 10/28/2010 05:57 PM, Haitao Shan wrote:
> Then what about the approach that Jan uses in 2.6.27 tree, which is
> reading CPUID-OSXSAVE? If supported, Xen will always set CR4.OSXSAVE.
Sure, if CR4.OSXSAVE is set iff xsave is available, then it should work
fine if we test that.

    J
> Shan Haitao
>
> 2010/10/29 Jeremy Fitzhardinge <jeremy@goop.org>:
>>  On 10/27/2010 07:57 PM, Haitao Shan wrote:
>>> Hi, Jeremy,
>>>
>>> My approach is based an old PV-OPS kernel source. Kernel tries to
set
>>> CR4.OSXSAVE and read it back to determine whether Xsave is actually
>>> available.
>>> Could you still use that?
>> No, because some older versions of Xen kill the domain when they try to
>> set unknown bits in CR4.
>>
>>    J
>>
>>> Shan Haitao
>>>
>>> 2010/10/28 Jeremy Fitzhardinge <jeremy@goop.org>:
>>>>  On 10/27/2010 12:04 AM, Haitao Shan wrote:
>>>>> Hi, Keir,
>>>>>
>>>>> This is patch #2, which adds PV guest Xsave support.
>>>> How does a PV guest know whether Xsave support is available? 
Previous
>>>> versions of Xen left the xsave cpu feature flag set even though
xsave
>>>> wasn''t usable by the domain, so I had to forceably
mask it from the
>>>> cpuid features within the domain.  Given that a PV domain
can''t rely on
>>>> X86_FEATURE_XSAVE, how can it tell that the feature is actually
usable?
>>>>
>>>> Thanks,
>>>>    J
>>>>
>>

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