XSAVE flavors

XSAVE flavors

[Jan Beulich]

Shuai,

originally I only meant to inquire about the state of the promised
alternatives improvement to the XSAVE code. However, while
looking over the code in question again I stumbled across a
separate issue: XSAVES, just like XSAVEOPT, may use the
"modified" optimization. However, the fcs and fds handling code
that has been present around the use of XSAVEOPT did not also
get applied to the XSAVES path. I suppose this was just an
oversight?

With this another question then is whether, when both XSAVEC
and XSAVEOPT are available, it is indeed always better to use
XSAVEC (as the code is doing after your enabling).

Thanks, Jan

Re: XSAVE flavors

[Jan Beulich]

>>> On 26.01.16 at 15:33, <JBeulich@suse.com> wrote:
> originally I only meant to inquire about the state of the promised
> alternatives improvement to the XSAVE code. However, while
> looking over the code in question again I stumbled across a
> separate issue: XSAVES, just like XSAVEOPT, may use the
> "modified" optimization. However, the fcs and fds handling code
> that has been present around the use of XSAVEOPT did not also
> get applied to the XSAVES path. I suppose this was just an
> oversight?
> 
> With this another question then is whether, when both XSAVEC
> and XSAVEOPT are available, it is indeed always better to use
> XSAVEC (as the code is doing after your enabling).

And I'm afraid there's yet one more issue: If my reading of the
SDM is right, then the offsets at which components get saved
by XSAVEC / XSAVES aren't fixed, but depend on RFBM (as that's
what gets stored into xcomp_bv[62:0]). xstate_comp_offsets[],
otoh, gets computed based on all available features, irrespective
of vcpu_xsave_mask() returning four different values depending
on current guest state. I can't see how get_xsave_addr() can
work correctly without honoring xcomp_bv. Nor can I convince
myself that state can't get corrupted / lost, e.g. when a save
with v->fpu_dirtied set is followed by one with v->fpu_dirtied
clear.

Am I misunderstanding what the SDM writes?

Jan

Re: XSAVE flavors

[Shuai Ruan]

On Tue, Jan 26, 2016 at 08:12:20AM -0700, Jan Beulich wrote:
> >>> On 26.01.16 at 15:33, <JBeulich@suse.com> wrote:
> > originally I only meant to inquire about the state of the promised
> > alternatives improvement to the XSAVE code. However, while
> > looking over the code in question again I stumbled across a
> > separate issue: XSAVES, just like XSAVEOPT, may use the
> > "modified" optimization. However, the fcs and fds handling code
> > that has been present around the use of XSAVEOPT did not also
> > get applied to the XSAVES path. I suppose this was just an
> > oversight?
Really sorry for late response. The alternatives on xsave code is ok a couples 
weeks ago, the patch solve xsaves use modified optimization problem.
I will send it now.
> > 
> > With this another question then is whether, when both XSAVEC
> > and XSAVEOPT are available, it is indeed always better to use
> > XSAVEC (as the code is doing after your enabling).
Yes.
But current no machine only support xsavec not support xsaves.  
I enable xsavec for "xsavec is a feature".
> 
> And I'm afraid there's yet one more issue: If my reading of the
> SDM is right, then the offsets at which components get saved
> by XSAVEC / XSAVES aren't fixed, but depend on RFBM (as that's
> what gets stored into xcomp_bv[62:0]). xstate_comp_offsets[],
> otoh, gets computed based on all available features, irrespective
> of vcpu_xsave_mask() returning four different values depending
> on current guest state. I can't see how get_xsave_addr() can
> work correctly without honoring xcomp_bv. Nor can I convince
> myself that state can't get corrupted / lost, e.g. when a save
> with v->fpu_dirtied set is followed by one with v->fpu_dirtied
> clear.
> 
> Am I misunderstanding what the SDM writes?
> 
Yes. you are right. This is a issue. I will find a way to solve
this.



> Jan
> 

Re: XSAVE flavors

[Jan Beulich]

>>> On 02.02.16 at 07:31, <shuai.ruan@linux.intel.com> wrote:
> On Tue, Jan 26, 2016 at 08:12:20AM -0700, Jan Beulich wrote:
>> >>> On 26.01.16 at 15:33, <JBeulich@suse.com> wrote:
>> > originally I only meant to inquire about the state of the promised
>> > alternatives improvement to the XSAVE code. However, while
>> > looking over the code in question again I stumbled across a
>> > separate issue: XSAVES, just like XSAVEOPT, may use the
>> > "modified" optimization. However, the fcs and fds handling code
>> > that has been present around the use of XSAVEOPT did not also
>> > get applied to the XSAVES path. I suppose this was just an
>> > oversight?
> Really sorry for late response. The alternatives on xsave code is ok a 
> couples 
> weeks ago, the patch solve xsaves use modified optimization problem.
> I will send it now.

Thanks, But this response of yours covers only one half of what
I've pointed out.

>> > With this another question then is whether, when both XSAVEC
>> > and XSAVEOPT are available, it is indeed always better to use
>> > XSAVEC (as the code is doing after your enabling).
> Yes.
> But current no machine only support xsavec not support xsaves.  
> I enable xsavec for "xsavec is a feature".

But this shouldn't preclude the code being in reasonable shape
also for the case where a CPU has XSAVEC but no XSAVES. The
more that right now we don't really need XSAVES (since we don't
yet allow any bit to get set in XSS).

>> And I'm afraid there's yet one more issue: If my reading of the
>> SDM is right, then the offsets at which components get saved
>> by XSAVEC / XSAVES aren't fixed, but depend on RFBM (as that's
>> what gets stored into xcomp_bv[62:0]). xstate_comp_offsets[],
>> otoh, gets computed based on all available features, irrespective
>> of vcpu_xsave_mask() returning four different values depending
>> on current guest state. I can't see how get_xsave_addr() can
>> work correctly without honoring xcomp_bv. Nor can I convince
>> myself that state can't get corrupted / lost, e.g. when a save
>> with v->fpu_dirtied set is followed by one with v->fpu_dirtied
>> clear.
>> 
>> Am I misunderstanding what the SDM writes?
>> 
> Yes. you are right. This is a issue. I will find a way to solve
> this.

Thanks.

Jan

Re: XSAVE flavors

[Shuai Ruan]

On Tue, Feb 02, 2016 at 02:42:38AM -0700, Jan Beulich wrote:
> >> > With this another question then is whether, when both XSAVEC
> >> > and XSAVEOPT are available, it is indeed always better to use
> >> > XSAVEC (as the code is doing after your enabling).
> > Yes.
> > But current no machine only support xsavec not support xsaves.  
> > I enable xsavec for "xsavec is a feature".
> 
> But this shouldn't preclude the code being in reasonable shape
> also for the case where a CPU has XSAVEC but no XSAVES. The
> more that right now we don't really need XSAVES (since we don't
> yet allow any bit to get set in XSS).
> 
Actually, when I enable xsaves/xsavec, I have put xsavec into
consideration. If xsavec is used we also need to guarntee that xcomp_bv 
never has any bits clear which are set in xstate_bv and the compaction
bit is set. 

Those guarntee and xsavec specific code in my patch is always behind "if( cpu_has_xsavec )" 
or " if ( cpu_has_xsaves || cpu_has_xsavec )".
Please remind me if there is some other things I am not aware.

> >> And I'm afraid there's yet one more issue: If my reading of the
> >> SDM is right, then the offsets at which components get saved
> >> by XSAVEC / XSAVES aren't fixed, but depend on RFBM (as that's
> >> what gets stored into xcomp_bv[62:0]). xstate_comp_offsets[],
> >> otoh, gets computed based on all available features, irrespective
> >> of vcpu_xsave_mask() returning four different values depending
> >> on current guest state. I can't see how get_xsave_addr() can
> >> work correctly without honoring xcomp_bv. Nor can I convince
> >> myself that state can't get corrupted / lost, e.g. when a save
> >> with v->fpu_dirtied set is followed by one with v->fpu_dirtied
> >> clear.
> >> 
> >> Am I misunderstanding what the SDM writes?
> >> 
> > Yes. you are right. This is a issue. I will find a way to solve
> > this.
> 
> Thanks.

For xstate_comp_offsets is only used in get_xsave_addr when performing migration. 
I intend to recaculte xstate_comp_offsets based on the vcpu->arch.xsavec_area.save_hdr.xcomp_bv 
before get_xsave_addr is called. 

The patch will be sent out after Chinese New Year holiday.

Thanks 
> 
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: XSAVE flavors

[Jan Beulich]

>>> On 04.02.16 at 07:49, <shuai.ruan@linux.intel.com> wrote:
> On Tue, Feb 02, 2016 at 02:42:38AM -0700, Jan Beulich wrote:
>> >> > With this another question then is whether, when both XSAVEC
>> >> > and XSAVEOPT are available, it is indeed always better to use
>> >> > XSAVEC (as the code is doing after your enabling).
>> > Yes.
>> > But current no machine only support xsavec not support xsaves.  
>> > I enable xsavec for "xsavec is a feature".
>> 
>> But this shouldn't preclude the code being in reasonable shape
>> also for the case where a CPU has XSAVEC but no XSAVES. The
>> more that right now we don't really need XSAVES (since we don't
>> yet allow any bit to get set in XSS).
>> 
> Actually, when I enable xsaves/xsavec, I have put xsavec into
> consideration. If xsavec is used we also need to guarntee that xcomp_bv 
> never has any bits clear which are set in xstate_bv and the compaction
> bit is set. 
> 
> Those guarntee and xsavec specific code in my patch is always behind "if( 
> cpu_has_xsavec )" 
> or " if ( cpu_has_xsaves || cpu_has_xsavec )".
> Please remind me if there is some other things I am not aware.

I'm not pointing out any correctness issue here, all I'm asking is
whether the current model is really the best one performance
wise.

>> >> And I'm afraid there's yet one more issue: If my reading of the
>> >> SDM is right, then the offsets at which components get saved
>> >> by XSAVEC / XSAVES aren't fixed, but depend on RFBM (as that's
>> >> what gets stored into xcomp_bv[62:0]). xstate_comp_offsets[],
>> >> otoh, gets computed based on all available features, irrespective
>> >> of vcpu_xsave_mask() returning four different values depending
>> >> on current guest state. I can't see how get_xsave_addr() can
>> >> work correctly without honoring xcomp_bv. Nor can I convince
>> >> myself that state can't get corrupted / lost, e.g. when a save
>> >> with v->fpu_dirtied set is followed by one with v->fpu_dirtied
>> >> clear.
>> >> 
>> >> Am I misunderstanding what the SDM writes?
>> >> 
>> > Yes. you are right. This is a issue. I will find a way to solve
>> > this.
>> 
>> Thanks.
> 
> For xstate_comp_offsets is only used in get_xsave_addr when performing 
> migration. 
> I intend to recaculte xstate_comp_offsets based on the 
> vcpu->arch.xsavec_area.save_hdr.xcomp_bv 
> before get_xsave_addr is called. 

I don't think that'll suffice, as it won't deal with the lazy XSAVE[SC]
possibly overwriting data written by the non-lazy one. See the
effectively three different values returned by vcpu_xsave_mask()
(the fourth one is impossible since the function won't ever get
called with both v->fpu_dirtied and v->arch.nonlazy_xstate_used
clear).

> The patch will be sent out after Chinese New Year holiday.

That's fine of course.

Jan

Re: XSAVE flavors

[Shuai Ruan]

On Thu, Feb 04, 2016 at 01:51:34AM -0700, Jan Beulich wrote:
> >> >> And I'm afraid there's yet one more issue: If my reading of the
> >> >> SDM is right, then the offsets at which components get saved
> >> >> by XSAVEC / XSAVES aren't fixed, but depend on RFBM (as that's
> >> >> what gets stored into xcomp_bv[62:0]). xstate_comp_offsets[],
> >> >> otoh, gets computed based on all available features, irrespective
> >> >> of vcpu_xsave_mask() returning four different values depending
> >> >> on current guest state. I can't see how get_xsave_addr() can
> >> >> work correctly without honoring xcomp_bv. Nor can I convince
> >> >> myself that state can't get corrupted / lost, e.g. when a save
> >> >> with v->fpu_dirtied set is followed by one with v->fpu_dirtied
> >> >> clear.
> >> >> 
> >> >> Am I misunderstanding what the SDM writes?
> >> >> 
> >> > Yes. you are right. This is a issue. I will find a way to solve
> >> > this.
> >> 
> >> Thanks.
> > 
> > For xstate_comp_offsets is only used in get_xsave_addr when performing 
> > migration. 
> > I intend to recaculte xstate_comp_offsets based on the 
> > vcpu->arch.xsavec_area.save_hdr.xcomp_bv 
> > before get_xsave_addr is called. 
> 
> I don't think that'll suffice, as it won't deal with the lazy XSAVE[SC]
> possibly overwriting data written by the non-lazy one. See the
> effectively three different values returned by vcpu_xsave_mask()
> (the fourth one is impossible since the function won't ever get
> called with both v->fpu_dirtied and v->arch.nonlazy_xstate_used
> clear).
Oh, Yes, Thanks.
The way I think to solve the problem is:
if v->fpu_dirtied is clear and v->arch.nonlazy_xstate_used is set,  
vcpu_xsave_mask will return XSTATE_ALL (only if xsave[sc] is support).
But this will do some overhead save.
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>