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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 27.10.15 at 02:06, <shuai.ruan@linux.intel.com> wrote:
> On Mon, Oct 26, 2015 at 08:03:55AM -0600, Jan Beulich wrote:
>> >>> On 23.10.15 at 11:48, <shuai.ruan@linux.intel.com> wrote:
>> > -            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.

Oh, right, you mean to hide more features than those Xen supports.

> So will the way I used in V7  ok ?

Conceptionally yes, but the first paragraph of my reply still applies,
i.e. the use of cpu_has_xgetbv1 should go away.

>> > +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.

See how I limited my reply by saying "at least one", which I added
after realizing that fact? IOW I'd expect you to use memcpy() and
then only zero that one field. Or you'd memset() the whole header
to zero, and then only store xstate_bv. As said, otherwise it looks
like you're adding an information leak.

Jan