Re: [Xen-devel] [V5 1/4] x86/xsaves: add basic definitions/helpers to support xsaves

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.09.15 at 13:33, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> This patch add basic definitions/helpers which will be used in
> later patches.
> 
> Signed-off-by: Shuai Ruan <shuai.ruan@xxxxxxxxxxxxxxx>
> ---

Missing brief description of changes in v5 here.

> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -29,12 +29,21 @@ static u32 __read_mostly xsave_cntxt_size;
>  /* A 64-bit bitmask of the XSAVE/XRSTOR features supported by processor. */
>  u64 __read_mostly xfeature_mask;
>  
> +unsigned int * __read_mostly xstate_offsets;
> +unsigned int * __read_mostly xstate_sizes;
> +static unsigned int __read_mostly xstate_features;
> +static unsigned int __read_mostly 
> xstate_comp_offsets[sizeof(xfeature_mask)*8];
> +
> +/* Cached msr_xss for fast read */
> +static DEFINE_PER_CPU(uint64_t, msr_xss);

Any reason not to call this just xss?

>  /* Cached xcr0 for fast read */
>  static DEFINE_PER_CPU(uint64_t, xcr0);
>  
>  /* Because XCR0 is cached for each CPU, xsetbv() is not exposed. Users 
> should 
>   * use set_xcr0() instead.
>   */
> +
>  static inline bool_t xsetbv(u32 index, u64 xfeatures)

Stray addition of a blank line.

> +bool_t xsave_area_compressed(const struct xsave_struct *xsave_area)
> +{
> +    if ( xsave_area && (xsave_area->xsave_hdr.xcomp_bv
> +                          & XSTATE_COMPACTION_ENABLED))
> +         return 1;
> +    return 0;
> +}

The body of the function could a single return statement.

> +static int setup_xstate_features(bool_t bsp)
> +{
> +    unsigned int leaf, tmp, eax, ebx;
> +
> +    if ( bsp )
> +        xstate_features = fls(xfeature_mask);
> +
> +    if ( bsp )

The two if()s should be combined.

> +    {
> +        xstate_offsets = xzalloc_array(unsigned int, xstate_features);
> +        if( !xstate_offsets )
> +            return -ENOMEM;
> +
> +        xstate_sizes = xzalloc_array(unsigned int, xstate_features);
> +        if( !xstate_sizes )
> +            return -ENOMEM;
> +    }
> +
> +    for (leaf = 2; leaf < xstate_features; leaf++)

Coding style.

> +    {
> +        if( bsp )

Again.

> +            cpuid_count(XSTATE_CPUID, leaf, &xstate_sizes[leaf],
> +                        &xstate_offsets[leaf], &tmp, &tmp);
> +        else
> +        {
> +             cpuid_count(XSTATE_CPUID, leaf, &eax,
> +                        &ebx, &tmp, &tmp);
> +             BUG_ON(eax != xstate_sizes[leaf]);
> +             BUG_ON(ebx != xstate_offsets[leaf]);
> +        }
> +     }
> +     return 0;

Blank line before final return statement please.

> +}
> +
> +static void setup_xstate_comp(void)
> +{
> +    unsigned int i;
> +
> +    /*
> +     * The FP xstates and SSE xstates are legacy states. They are always
> +     * in the fixed offsets in the xsave area in either compacted form
> +     * or standard form.
> +     */
> +    xstate_comp_offsets[0] = 0;
> +    xstate_comp_offsets[1] = XSAVE_SSE_OFFSET;
> +
> +    xstate_comp_offsets[2] = FXSAVE_SIZE + XSAVE_HDR_SIZE;
> +
> +    for (i = 3; i < xstate_features; i++)

Coding style again.

> +    {
> +        xstate_comp_offsets[i] = xstate_comp_offsets[i-1] + (((1ul << i)
> +                                 & xfeature_mask) ? xstate_sizes[i-1] : 0);

Indentation should match up with the parentheses. Which likely
means you'll want to break the line after the +.

> +        ASSERT(xstate_comp_offsets[i] <= xsave_cntxt_size);

How about the last component's offset + size exceeding
xsave_cntxt_size?

Also just considering what the function does, it looks like it ought to
be __init.

> +void save_xsave_states(struct vcpu *v, void *dest, unsigned int size)

Iirc it was suggested before that the function's name isn't adequate:
There's no saving of anything here. You're expanding already saved
state.

> +{
> +    struct xsave_struct *xsave = v->arch.xsave_area;

const?

> +    u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
> +    u64 valid;
> +
> +    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);
> +
> +    /* Set XSTATE_BV */
> +    *(u64 *)(dest + XSAVE_HDR_OFFSET) = xstate_bv;

Can't you avoid fragile casts like this by using a proper structure
type for dest?

> +    /*
> +     * Copy each region from the possibly compacted offset to the
> +     * non-compacted offset.
> +     */
> +    valid = xstate_bv & ~XSTATE_FP_SSE;
> +    while ( valid )
> +    {
> +        u64 feature = valid & -valid;
> +        int index = fls(feature) - 1;

unsigned int

> +        void *src = get_xsave_addr(xsave, index);

const

> +        if ( src )
> +        {
> +            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
> +            memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]);
> +        }
> +
> +        valid -= feature;

While correct this way, I would consider "&= ~feature" more natural.

> +    }
> +
> +}
> +
> +void load_xsave_states(struct vcpu *v, const void *src, unsigned int size)

Similar comments as above apply to this function.

> +    /* Set XSTATE_BV and possibly XCOMP_BV.  */
> +    xsave->xsave_hdr.xstate_bv = xstate_bv;
> +    xsave->xsave_hdr.xcomp_bv = v->arch.xcr0_accum | 
> XSTATE_COMPACTION_ENABLED;

Why does the comment say "possibly" when it's done unconditionally?

Also, as a general comment - both functions get added without any
user, but they place constraints on the values they may be passed
(the src/dest pointers must not overlap the v->arch.xsave_area),
which is ugly to validate without seeing the intended callers. Perhaps,
especially if you want to keep the patch splitting as it is, adding
respective ASSERT()s would make this more explicit.

Jan

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