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

[Shuai Ruan <shuai.ruan@xxxxxxxxxxxxxxx>]

On Fri, Mar 11, 2016 at 03:16:05AM -0700, Jan Beulich wrote:
> I don't think this is what we want. In no case is this what I have
> been asking for (which also applies to the remainder of your reply).
> Just to re-iterate: Code outside of the code xsave() / xrstor()
> functions should not be concerned at all what specific save and
> restore instructions are being used. All it needs to care about is
> to know what layout the data is in, and whether compaction or
> expansion is needed while transferring state from / to a guest.
> 
> The fact that we introduce a synthetic feature here is solely to
> satisfy the alternative instruction patching mechanism (and it
> could be dropped if both the save and restore paths came to
> use further conditionals, which may well be desirable - I think I
> had suggested this for one of the two paths already). And
> perhaps it was a mistake to scatter around the setting of
> XSTATE_COMPACTION_ENABLED.
> 
> May I ask that you take a little step back and think about what
> our needs here really are? For this please consider that we want
> to save/restore state with as little overhead as possible (i.e. it
> may be warranted to make the choice of instruction depend on
> the set of components that need saving/restoring, rather than
> just the availability of certain instructions). And that choice of
> instruction(s) should be as transparent to the rest of the
> hypervisor as possible. Which for example means ...
> 
> >> 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.
> 
> ... that "using_xsaves" is not what the rest of the hypervisor is
> in need of knowing/checking. All that other code a most needs to
> know/check whether the state is / needs to be in compacted form.
> 
> Jan
> 
Sure. I write a few key points here.

1. For when to use "XSTATE_COMPACTION_ENABLED"
1). it will only be set in xrstor().
2). all code outside xsave()/xrstor() (exclude compress_xsave_states()) 
    only check whether XSTATE_COMPACTION_ENABLED is set or not.

2. For when to use "using_xsaves"
1). only used in xrstor()/xsave().
2). xrstor will not stick to alternative patching. Will use
    if(use_xsaves) instead.

3. For save/restore(migration)
1). for save, it is ok to check XSTATE_COMPACTION_ENABLED of
    xsave->xsave_hdr.xcomp_bv to decide whether expanded is 
    needed or not.
2). for restore, in compress_xsave_states(), we can not check
    XSTATE_COMPACTION_ENABLED of xsave->xsave_hdr.xcomp_bv 
    to decide whether compress is needed or not (for                          
    XSTATE_COMPACTION_ENABLED will only be set when perform                     
    first xrstor()).
    we should use "using_xsaves" is tell whether compact is needed
    or not.(this is the only place outside xsave()/xrstor()
    depend on "using_xsaves")
    Code in compress_xsave_states looks as follow.
    ....
    if ( !using_xsaves && !xsave_area_compress(src) )
    {
        memcpy
        return 
    }
    .....
    compress src
    

For more detail:
        xrstor() will look as follow:
        if ( using_xsaves )
        {
                if ( unlikely(!(prt->xsave_hdr->xcom_bv &
                              XSTATE_COMPACTION_ENABLED)) )
                       ptr->xsave_hdr->xcomp_bv =
                                    ptr->xsave_hdr->xstate_bv |
                                    XSTATE_COMPACTION_ENABLED;
                XRSTORS;
        }
        else
                XRSTOR;

Any comments on this? 

I know you are busy :) and really thanks for 
your time spent on making this clear to me.

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

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