Re: [Xen-devel] [V3] x86/xsaves: calculate the xstate_comp_offsets base on xcomp_bv

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.03.16 at 12:00, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -934,8 +934,14 @@ long arch_do_domctl(
>                      goto vcpuextstate_out;
>                  }
>  
> -                expand_xsave_states(v, xsave_area,
> -                                    size - 2 * sizeof(uint64_t));
> +                ret = expand_xsave_states(v, xsave_area,
> +                                          size - 2 * sizeof(uint64_t));
> +                if ( ret )
> +                {
> +                    xfree(xsave_area);
> +                    vcpu_unpause(v);
> +                    goto vcpuextstate_out;
> +                }

Well, while this is one way to deal with the problem, it's certainly
not the most desirable one: We should try to avoid runtime
allocations, failures of which then cause other things to fail (in
perhaps not very graceful ways). And doing so is pretty simple
here, and you even have two options: Either allocate a per-CPU
array, or - considering that XCNTXT_MASK has only a limited
number of bits set - even use an on-stack array of suitable
(compile time determined from XCNTXT_MASK) size. If you
want to save space, this could presumably even be uint16_t[],
in which case I think it would further be okay if all 63 possible
features were known (totaling to a variable size of 126 bytes).

This is even more so considering that you forgot to adjust
hvm.c in a similar manner.

> @@ -111,21 +111,27 @@ static int setup_xstate_features(bool_t bsp)
>      for ( leaf = 2; leaf < xstate_features; leaf++ )
>      {
>          if ( bsp )
> +        {
>              cpuid_count(XSTATE_CPUID, leaf, &xstate_sizes[leaf],
> -                        &xstate_offsets[leaf], &tmp, &tmp);
> +                        &xstate_offsets[leaf], &ecx, &edx);
> +            if ( ecx & XSTATE_ALIGN64 )
> +                set_bit(leaf, &xstate_align);

__set_bit()

> @@ -134,22 +140,26 @@ static void __init setup_xstate_comp(void)
>       * 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++ )
>      {
> -        xstate_comp_offsets[i] = xstate_comp_offsets[i - 1] +
> -                                 (((1ul << i) & xfeature_mask)
> -                                  ? xstate_sizes[i - 1] : 0);
> -        ASSERT(xstate_comp_offsets[i] + xstate_sizes[i] <= xsave_cntxt_size);
> +        if ( (1ul << i) & xcomp_bv )
> +        {
> +            xstate_comp_offsets[i] = test_bit(i, &xstate_align) ?
> +                                     ROUNDUP(xstate_comp_offsets[i - 1], 64) 
> :
> +                                     xstate_comp_offsets[i -1];
> +            xstate_comp_offsets[i] += xstate_sizes[i - 1];
> +            ASSERT(xstate_comp_offsets[i] + xstate_sizes[i] <= 
> xsave_cntxt_size);
> +        }

This now seems wrong for the case when there are discontiguous
bits in xcomp_bv, as you leave zeros in the array (which by itself
is fine, but confuses the subsequent iteration). I think you'd
benefit from having a local variable holding the offset you're
currently at:

    offset = xstate_comp_offsets[2];
    for ( i = 2; i < xstate_features; i++ )
    {
        if ( (1ul << i) & xcomp_bv )
        {
            if ( test_bit(i, &xstate_align) )
                offset = ROUNDUP(offset, 64);
            xstate_comp_offsets[i] = offset;
            offset += xstate_sizes[i];
            ASSERT(offset <= xsave_cntxt_size);
        }
    }

or something along those lines.

>  static void *get_xsave_addr(struct xsave_struct *xsave,
> -        unsigned int xfeature_idx)
> +                            unsigned int *xstate_comp_offsets,

const

> @@ -94,8 +96,8 @@ bool_t xsave_enabled(const struct vcpu *v);
>  int __must_check validate_xstate(u64 xcr0, u64 xcr0_accum,
>                                   const struct xsave_hdr *);
>  int __must_check handle_xsetbv(u32 index, u64 new_bv);
> -void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size);
> -void compress_xsave_states(struct vcpu *v, const void *src, unsigned int 
> size);
> +int expand_xsave_states(struct vcpu *v, void *dest, unsigned int size);
> +int compress_xsave_states(struct vcpu *v, const void *src, unsigned int 
> size);

While with the above I assume these are going to remain what
they have been, a general advice: If you convert a function
from having void return type to something else, and that
something else can indicate an error, use __must_check so the
compiler can tell you whether you've forgotten to update some
caller.

Jan

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