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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.03.16 at 10:35, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> On Tue, Mar 15, 2016 at 07:33:40AM -0600, Jan Beulich wrote:
>> >>> On 15.03.16 at 10:40, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
>> >    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;
>> 
>> This makes me imply that "using_xsaves" is still a global variable,
>> set depending on CPU features. That's exactly what I've said
>> would presumably not be sufficient in code like xrstor(). What
>> point is there in using XSTORS if the guest never touched XSS?
>> I would much rather have expected for you to introduce a
>> flag paralleling v->arch.nonlazy_xstate_used indicating
>> whether for a particular vCPU XSAVES/XRSTORS really need to be
>> used (or maybe just looking at xcr0_accum would be sufficient,
>> and no new flag is needed; in fact I think that flag would also
>> better go away in favor of just inspecting xcr0_accum).
> 
> if xrstor() side depend on checking xcr0_accum and using_xsaves,
> then xsave() can not only depend on using_xsaves (or
> X86_FEATURE_USE_XSAVES). So I will drop alternativer patching 
> in xsave() side.
> And both xsave() xrestor() will depend on using_xsaves and checking 
> xcr0_accum. And compress_xsave_states() will check this too.
> 
> Detail is :
> 
> #define XSTATE_SUPER 0

Not an ideal name I would say. XSTATE_XSAVES_ONLY maybe?

> #define using_xsaves 0
> 
> if ( using_xsaves && (v->arch.xcr0_accum & XSTATE_SUPER) )
> {
> 
> .....
>       XSAVES/XRSTORS;
> }

So what does the left side of the && then do that the right side
doesn't already cover? When there's no XSAVES support, then
code elsewhere should (and already does afaict) guarantee that
the respective bits in xcr0_accum can't ever get turned on.

Jan


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