[PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements

[PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements

[Han, Weidong]

Change logs from v1 -> v2:
	Due to not guarantee backward compatibility, drop the guest save/restore patch here. Will re-implement it later. In addition, split the original fix frozen states patch into XSAVE/XRSTOR cleanup patch and fix frozen state patch.

Patch 1/3: XSAVE/XRSTOR: some cleanups
	Replace xfeature_low and xfeature_high with a u64 variable xfeature_mask. 
	In structure hvm_vcpu, rename xfeature_mask to xcr0
	Provide EDX:EAX with all bits set to 1 for XSAVE and XRSTOR as spec recommends.

Patch 2/3: Fix frozen states 
	If a guest sets a state and dirties the state, but later temporarily clears the state, and at this time if this vcpu is scheduled out, then other vcpus may corrupt the state before the vcpu is scheduled in again, thus the state cannot be restored correctly. To solve this issue, this patch save/restore all states unconditionally on vcpu context switch.

Patch 3/3. Enable guest AVX
	This patch enables Intel(R) Advanced Vector Extension (AVX) for guest.


Signed-off-by: Weidong Han <weidong.han@intel.com>

Re: [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements

[Tim Deegan]

At 15:52 +0100 on 31 Aug (1283269931), Han, Weidong wrote:
> Change logs from v1 -> v2: Due to not guarantee backward
> 	compatibility, drop the guest save/restore patch here. Will
> 	re-implement it later.

That's OK for review but we can't apply the rest of these patches
without the save/restore one; it'll just silently break save/restore for
HVM guests.

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements

[Weidong Han]

Tim Deegan wrote:
> At 15:52 +0100 on 31 Aug (1283269931), Han, Weidong wrote:
>   
>> Change logs from v1 -> v2: Due to not guarantee backward
>> 	compatibility, drop the guest save/restore patch here. Will
>> 	re-implement it later.
>>     
>
> That's OK for review but we can't apply the rest of these patches
> without the save/restore one; it'll just silently break save/restore for
> HVM guests.
>
> XSAVE/XRSTOR was already enabled by Dexuan for months, but it misses save/restore xstates. Actually it already breaks hvm guests save/restore on platforms which supports XSAVE/XRSTOR. 
>   
Regards,
Weidong
>  
>
>   

Re: [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements

[Keir Fraser]

On 31/08/2010 15:52, "Han, Weidong" <weidong.han@intel.com> wrote:

> Change logs from v1 -> v2:
> Due to not guarantee backward compatibility, drop the guest save/restore patch
> here. Will re-implement it later. In addition, split the original fix frozen
> states patch into XSAVE/XRSTOR cleanup patch and fix frozen state patch.
> 
> Patch 1/3: XSAVE/XRSTOR: some cleanups
> Replace xfeature_low and xfeature_high with a u64 variable xfeature_mask.
> In structure hvm_vcpu, rename xfeature_mask to xcr0
> Provide EDX:EAX with all bits set to 1 for XSAVE and XRSTOR as spec
> recommends.
> 
> Patch 2/3: Fix frozen states
> If a guest sets a state and dirties the state, but later temporarily clears
> the state, and at this time if this vcpu is scheduled out, then other vcpus
> may corrupt the state before the vcpu is scheduled in again, thus the state
> cannot be restored correctly. To solve this issue, this patch save/restore all
> states unconditionally on vcpu context switch.

Performance overhead of this fix? Is there no other lazy save technique that
can work?

> Patch 3/3. Enable guest AVX
> This patch enables Intel(R) Advanced Vector Extension (AVX) for guest.

If we enable this but don't implement save/restore then don't guests lose
state across s/r with unpredictable results?

 -- Keir

Re: [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements

[Weidong Han]

Keir Fraser wrote:
> On 31/08/2010 15:52, "Han, Weidong" <weidong.han@intel.com> wrote:
>
>   
>> Change logs from v1 -> v2:
>> Due to not guarantee backward compatibility, drop the guest save/restore patch
>> here. Will re-implement it later. In addition, split the original fix frozen
>> states patch into XSAVE/XRSTOR cleanup patch and fix frozen state patch.
>>
>> Patch 1/3: XSAVE/XRSTOR: some cleanups
>> Replace xfeature_low and xfeature_high with a u64 variable xfeature_mask.
>> In structure hvm_vcpu, rename xfeature_mask to xcr0
>> Provide EDX:EAX with all bits set to 1 for XSAVE and XRSTOR as spec
>> recommends.
>>
>> Patch 2/3: Fix frozen states
>> If a guest sets a state and dirties the state, but later temporarily clears
>> the state, and at this time if this vcpu is scheduled out, then other vcpus
>> may corrupt the state before the vcpu is scheduled in again, thus the state
>> cannot be restored correctly. To solve this issue, this patch save/restore all
>> states unconditionally on vcpu context switch.
>>     
>
> Performance overhead of this fix? Is there no other lazy save technique that
> can work?
>   
I think the cost of set_xcr0 which just changes some bits in XCR0 
register should be little.  I don't have any optimization for it now.
>   
>> Patch 3/3. Enable guest AVX
>> This patch enables Intel(R) Advanced Vector Extension (AVX) for guest.
>>     
>
> If we enable this but don't implement save/restore then don't guests lose
> state across s/r with unpredictable results?
>   
Yes. As I said in another email, actually it already breaks hvm guests 
save/restore on platforms which supports XSAVE/XRSTOR.

Regards,
Weidong
>
>
>
>   

Re: [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements

[Keir Fraser]

On 01/09/2010 02:53, "Weidong Han" <weidong.han@intel.com> wrote:

>> Performance overhead of this fix? Is there no other lazy save technique that
>> can work?
>>   
> I think the cost of set_xcr0 which just changes some bits in XCR0
> register should be little.  I don't have any optimization for it now.

I obviously don't mean that. What about the increased cost of XSAVE and
XRSTOR s/r'ing more state unconditionally? At least it is conditional on
v->fpu_dirtied I suppose?

>>> Patch 3/3. Enable guest AVX
>>> This patch enables Intel(R) Advanced Vector Extension (AVX) for guest.
>>>     
>> 
>> If we enable this but don't implement save/restore then don't guests lose
>> state across s/r with unpredictable results?
>>   
> Yes. As I said in another email, actually it already breaks hvm guests
> save/restore on platforms which supports XSAVE/XRSTOR.

Wow, so the last couple of Xen releases are broken for the latest Intel
platforms unless you specify no-xsave. Handy to know I guess.

Why is the feature flag stuff all stuffed in Xen itself rather than
xc_cpuid_x86.c, by the way? Shouldn't your change also be in the same place,
or (much preferably) all XSAVE related stuff be moved out into
libxc/xc_cpuid_x86.c?

 -- Keir

> Regards,
> Weidong
>> 
>> 
>> 
>>   
> 

Re: [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements

[Keir Fraser]

On 01/09/2010 08:26, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

>> Yes. As I said in another email, actually it already breaks hvm guests
>> save/restore on platforms which supports XSAVE/XRSTOR.
> 
> Wow, so the last couple of Xen releases are broken for the latest Intel
> platforms unless you specify no-xsave. Handy to know I guess.

Actually, hang on, Dexuan's patch only enabled XSAVE of FPU/SSE and didn't
make any other new state visible. We already save/restore FPU/SSE of course,
so I don't see why the code we already have is broken for HVM save/restore.
It's the adding of new state that we don't s/r that would be broken --- like
your patch to make AVX visible.

Well that's how it was explained to me at the time. Was that in fact wrong
and we are already broken for save/restore for some subtle unexplained
reason?

 -- Keir

> Why is the feature flag stuff all stuffed in Xen itself rather than
> xc_cpuid_x86.c, by the way? Shouldn't your change also be in the same place,
> or (much preferably) all XSAVE related stuff be moved out into
> libxc/xc_cpuid_x86.c?

Re: [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements

[Weidong Han]

Keir Fraser wrote:
> On 01/09/2010 08:26, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:
>
>   
>>> Yes. As I said in another email, actually it already breaks hvm guests
>>> save/restore on platforms which supports XSAVE/XRSTOR.
>>>       
>> Wow, so the last couple of Xen releases are broken for the latest Intel
>> platforms unless you specify no-xsave. Handy to know I guess.
>>     
>
> Actually, hang on, Dexuan's patch only enabled XSAVE of FPU/SSE and didn't
> make any other new state visible. We already save/restore FPU/SSE of course,
> so I don't see why the code we already have is broken for HVM save/restore.
> It's the adding of new state that we don't s/r that would be broken --- like
> your patch to make AVX visible.
>
> Well that's how it was explained to me at the time. Was that in fact wrong
> and we are already broken for save/restore for some subtle unexplained
> reason?
>   

When XSAVE is enabled, it saves states to xsave_area in struct hvm_vcpu. 
But hvm_save_cpu_ctxt and hvm_load_cpu_ctxt don't save/restore FPU/SSE 
from xsave_area. So FPU/SSE states are incorrect after guest save/restore.

Regards,
Weidong
>  -- Keir
>
>   
>> Why is the feature flag stuff all stuffed in Xen itself rather than
>> xc_cpuid_x86.c, by the way? Shouldn't your change also be in the same place,
>> or (much preferably) all XSAVE related stuff be moved out into
>> libxc/xc_cpuid_x86.c?
>>     
>
>
>   

Re: [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements

[Keir Fraser]

On 01/09/2010 08:56, "Weidong Han" <weidong.han@intel.com> wrote:

>> Well that's how it was explained to me at the time. Was that in fact wrong
>> and we are already broken for save/restore for some subtle unexplained
>> reason?
>>   
> 
> When XSAVE is enabled, it saves states to xsave_area in struct hvm_vcpu.
> But hvm_save_cpu_ctxt and hvm_load_cpu_ctxt don't save/restore FPU/SSE
> from xsave_area. So FPU/SSE states are incorrect after guest save/restore.

Ignoring AVX, that needs fixing in 4.0 branch. The state needs pulling out
of the xsave area and into the existing hvm_hw_cpu structure. Or, we disable
xsave by default on 4.0.x branch. What do you think?

 -- Keir

Re: [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements

[Weidong Han]

Keir Fraser wrote:
> On 01/09/2010 08:56, "Weidong Han" <weidong.han@intel.com> wrote:
>
>   
>>> Well that's how it was explained to me at the time. Was that in fact wrong
>>> and we are already broken for save/restore for some subtle unexplained
>>> reason?
>>>   
>>>       
>> When XSAVE is enabled, it saves states to xsave_area in struct hvm_vcpu.
>> But hvm_save_cpu_ctxt and hvm_load_cpu_ctxt don't save/restore FPU/SSE
>> from xsave_area. So FPU/SSE states are incorrect after guest save/restore.
>>     
>
> Ignoring AVX, that needs fixing in 4.0 branch. The state needs pulling out
> of the xsave area and into the existing hvm_hw_cpu structure. Or, we disable
> xsave by default on 4.0.x branch. What do you think?
>
>   
I prefer to disable it by default on 4.0 branch now. After finish guest save/restore with XSAVE/XRSTOR in xen-unstable, we can port code to 4.0 branch and enable it again. 

Regards,
Weidong

Re: [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements

[Weidong Han]

Keir Fraser wrote:
> On 01/09/2010 02:53, "Weidong Han" <weidong.han@intel.com> wrote:
>
>   
>>> Performance overhead of this fix? Is there no other lazy save technique that
>>> can work?
>>>   
>>>       
>> I think the cost of set_xcr0 which just changes some bits in XCR0
>> register should be little.  I don't have any optimization for it now.
>>     
>
> I obviously don't mean that. What about the increased cost of XSAVE and
> XRSTOR s/r'ing more state unconditionally? At least it is conditional on
> v->fpu_dirtied I suppose?
>   
One possible optimization is that only save/restore legacy states (FPU 
and SSE) for guests which don't enable XSAVE.

Both xsave() and xrstor are invoked only when v->fpu_dirtied.
>   
>>>> Patch 3/3. Enable guest AVX
>>>> This patch enables Intel(R) Advanced Vector Extension (AVX) for guest.
>>>>     
>>>>         
>>> If we enable this but don't implement save/restore then don't guests lose
>>> state across s/r with unpredictable results?
>>>   
>>>       
>> Yes. As I said in another email, actually it already breaks hvm guests
>> save/restore on platforms which supports XSAVE/XRSTOR.
>>     
>
> Wow, so the last couple of Xen releases are broken for the latest Intel
> platforms unless you specify no-xsave. Handy to know I guess.
>
> Why is the feature flag stuff all stuffed in Xen itself rather than
> xc_cpuid_x86.c, by the way? Shouldn't your change also be in the same place,
> or (much preferably) all XSAVE related stuff be moved out into
> libxc/xc_cpuid_x86.c?
>   
I'm afraid XSAVE related stuff cannot be move out into 
libxc/xc_cpuid_x86.c completely? At least Xen needs to control 
X86_CR4_OSXSAVE for guests which don't support XSAVE. I will have a look 
at it.

Regards,
Weidong
>  -- Keir
>
>   
>> Regards,
>> Weidong
>>     
>>>
>>>   
>>>       
>
>
>