Re: [Xen-devel] [V8 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Shuai Ruan <shuai.ruan@xxxxxxxxxxxxxxx>]

On Mon, Oct 26, 2015 at 08:03:55AM -0600, Jan Beulich wrote:
> >>> On 23.10.15 at 11:48, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> > Signed-off-by: Shuai Ruan <shuai.ruan@xxxxxxxxxxxxxxx>
> > Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> 
> There were actual bugs fixed from v7 to v8, plus there's a public
> interface change, so retaining this tag is wrong.
> 
Ok
> > -            a &= boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32];
> > -            if ( !cpu_has_xsaves )
> > -                b = c = d = 0;
> > +            a &= cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> > +                 cpufeat_mask(X86_FEATURE_XSAVEC) |
> > +                 (cpu_has_xgetbv1 ? cpufeat_mask(X86_FEATURE_XGETBV1) : 0);
> 
> Why the cpu_has_xgetbv1 dependency? If you want to do it this
> way, it will get unreadable with two or three more features. Why
> don't you simply and with the known mask on top of the and with
> the capability flags that was already there?
> 
> And actually I realize I may have misguided you: xstate_init()
> already makes sure boot_cpu_data.x86_capability[] doesn't
> contain any unsupported flags, so keeping just the masking
> that's already there should be enough.
> 
In this patch in xstate_init I have add xsaves understood by xen. This
boot_cpu_data.x86_capability[] contain support for xsaves. And in my
previous patch V7 I use 
"a &= (boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32] & 
~cpufeat_mask(X86_FEATURE_XSAVES))"
to mask out xsaves for pv guest. You think this should use white listing
way. So will the way I used in V7  ok ?

> > +void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
> > +{
> > +    struct xsave_struct *xsave = v->arch.xsave_area;
> > +    u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
> > +    u64 valid;
> > +
> > +    if ( !cpu_has_xsaves && !cpu_has_xsavec )
> > +    {
> > +        memcpy(dest, xsave, size);
> > +        return;
> > +    }
> > +
> > +    ASSERT(xsave_area_compressed(xsave));
> > +    /*
> > +     * Copy legacy XSAVE area, to avoid complications with CPUID
> > +     * leaves 0 and 1 in the loop below.
> > +     */
> > +    memcpy(dest, xsave, FXSAVE_SIZE);
> > +
> > +    ((struct xsave_struct *)dest)->xsave_hdr.xstate_bv = xstate_bv;
> > +    ((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv =  0;
> 
> Wouldn't it be more logical to also memcpy() the header? Which
> would do away with at least one of these ugly casts, would
> allow folding with the memcpy() done right before, _and_ would
> eliminate an (apparent or real I'm not sure without looking in
> more detail) information leak (the remainder of the header).
> 
For machine with no-xsaves support, xcomp_bv should be zero or it will cause
GP fault. So we can not just memcpy the header when perform save.

Thanks 
Shuai
> 

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