Re: [V4] x86/xsaves: fix overwriting between non-lazy/lazy xsaves

[Shuai Ruan <shuai.ruan@linux.intel.com>]

On Thu, Mar 10, 2016 at 02:30:34AM -0700, Jan Beulich wrote:
> I'm not sure about the "also" here. Perhaps just drop it? Or replace
> it by "yet"? A native speaker's input would be appreciated.
> 
Thanks. I will drop it . 
> > --- a/xen/arch/x86/xstate.c
> > +++ b/xen/arch/x86/xstate.c
> > @@ -165,7 +165,7 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
> >      u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
> >      u64 valid;
> >  
> > -    if ( !cpu_has_xsaves && !cpu_has_xsavec )
> > +    if ( !(xsave->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) )
> >      {
> >          memcpy(dest, xsave, size);
> >          return;
> 
> This one looks correct, but ...
> 
> > @@ -206,7 +206,7 @@ void compress_xsave_states(struct vcpu *v, const void 
> > *src, unsigned int size)
> >      u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
> >      u64 valid;
> >  
> > -    if ( !cpu_has_xsaves && !cpu_has_xsavec )
> > +    if ( !(xsave->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) )
> >      {
> >          memcpy(xsave, src, size);
> >          return;
> 
> ... how can this one be? You are in the process of compressing
> known uncompressed input.
I think this one is corret, here this check means whether we use
xsaves in xen or not (actually when we use xsaves in xen 
xsave->xsave_hdr.xcomp_bv will set XSTATE_COMPACTION_ENABLED).
For more clearly, I can add 
if ( !(xsave->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) &&
     !xsave_area_compressed(src) )
But I do think !xsave_area_compressed(src) is useless. 
There is a "ASSERT(!xsave_area_compressed(src))" follow "if ()".
> 
> > @@ -370,7 +368,7 @@ void xrstor(struct vcpu *v, uint64_t mask)
> >                         "   .previous\n" \
> >                         _ASM_EXTABLE(1b, 2b), \
> >                         ".byte " pfx "0x0f,0xc7,0x1f\n", \
> > -                       X86_FEATURE_XSAVES, \
> > +                       X86_FEATURE_XSAVE_COMPACT, \
> >                         ASM_OUTPUT2([mem] "+m" (*ptr), [faults] "+g" (faults)), \
> >                         [lmask] "a" (lmask), [hmask] "d" (hmask), \
> >                         [ptr] "D" (ptr))
> 
> I don't think you can stick to alternative patching here - whether
> to use XRSTORS now depends on what state is to be restored.
> 
X86_FEATURE_XSAVE_COMPACT is confusing. I will change
X86_FEATURE_XSAVE_COMPACT -> X86_FEATURE_USE_XSAVES
Then, XRSTORS in the alternative patch can depend on 
X86_FEATURE_USE_XSAVES. 
> Or maybe (to amend the first comment above)
> "using_xsave_compact" is actually the wrong term now, and this
> really needs to become "using_xsaves" (in which case the change
> suggested in that first comment wouldn't be needed anymore). In
The term using_xsave_compact is confusing(actually here using_xsave_compact
means using_xsaves). Will change using_xsave_compact -> using_xsaves.
> the end, at least the code outside of xstate.c should be in a state
> where xstate.c's choice of whether to use XSAVEC doesn't matter
XSAVEC? 
Oh, I now realise that I simply drop xsavec support code is
too much of a step backwards(what you want here is using a synthetic CPU 
feature X86_FEATRUE_USE_XSAVEC and using_xsavec to disable xsavec like
xsaves, code depend on cpu_has_xsavec will depend on useing_xsavec).
The code should be ok even if we use xsavc in xen.  
Is that what you mean ?
> (and ideally this would also extend to all code in that file except
> for the relevant parts of xsave()).
If I understand you clearly (my comments above is right), I think we can
also add xsavec aternative patching depend on X86_FEATRUE_USE_XSAVEC 
in xsave(), and just keep X86_FEATRUE_USE_XSAVEC clear in x86_cap.

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

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