Re: [Xen-devel] [PATCH v2 2/6] x86/cpuid: Introduce recalculate_xstate()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.01.17 at 12:27, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -80,6 +80,103 @@ static void sanitise_featureset(uint32_t *fs)
>                            (fs[FEATURESET_e1d] & ~CPUID_COMMON_1D_FEATURES));
>  }
>  
> +static void recalculate_xstate(struct cpuid_policy *p)
> +{
> +    uint64_t xstates = XSTATE_FP_SSE;
> +    uint32_t xstate_size = XSTATE_AREA_MIN_SIZE;
> +    unsigned int i, Da1 = p->xstate.Da1;
> +
> +    /*
> +     * The Da1 leaf is the only piece if information preserved in the common
> +     * case.  Everything else is derived from other feature state.
> +     */

"piece of" I think.

> +    memset(&p->xstate, 0, sizeof(p->xstate));
> +
> +    if ( !p->basic.xsave )
> +        return;
> +
> +    if ( p->basic.avx )
> +    {
> +        xstates |= XSTATE_YMM;
> +        xstate_size = max(xstate_size,
> +                          xstate_offsets[_XSTATE_YMM] +
> +                          xstate_sizes[_XSTATE_YMM]);
> +    }
> +
> +    if ( p->feat.mpx )
> +    {
> +        xstates |= XSTATE_BNDREGS | XSTATE_BNDCSR;
> +        xstate_size = max(xstate_size,
> +                          xstate_offsets[_XSTATE_BNDCSR] +
> +                          xstate_sizes[_XSTATE_BNDCSR]);
> +    }
> +
> +    if ( p->feat.avx512f )
> +    {
> +        xstates |= XSTATE_OPMASK | XSTATE_ZMM | XSTATE_HI_ZMM;
> +        xstate_size = max(xstate_size,
> +                          xstate_offsets[_XSTATE_HI_ZMM] +
> +                          xstate_sizes[_XSTATE_HI_ZMM]);
> +    }
> +
> +    if ( p->feat.pku )
> +    {
> +        xstates |= XSTATE_PKRU;
> +        xstate_size = max(xstate_size,
> +                          xstate_offsets[_XSTATE_PKRU] +
> +                          xstate_sizes[_XSTATE_PKRU]);
> +    }
> +
> +    if ( p->extd.lwp )
> +    {
> +        xstates |= XSTATE_LWP;
> +        xstate_size = max(xstate_size,
> +                          xstate_offsets[_XSTATE_LWP] +
> +                          xstate_sizes[_XSTATE_LWP]);
> +    }
> +
> +    /* Sanity check we aren't advertising unknown states. */
> +    ASSERT((xstates & ~XCNTXT_MASK) == 0);
> +
> +    p->xstate.max_size  =  xstate_size;
> +    p->xstate.xcr0_low  =  xstates & ~XSTATE_XSAVES_ONLY;
> +    p->xstate.xcr0_high = (xstates & ~XSTATE_XSAVES_ONLY) >> 32;
> +
> +    p->xstate.Da1 = Da1;
> +    if ( p->xstate.xsaves )
> +    {
> +        p->xstate.xss_low   =  xstates & XSTATE_XSAVES_ONLY;
> +        p->xstate.xss_high  = (xstates & XSTATE_XSAVES_ONLY) >> 32;
> +    }
> +    else
> +        xstates &= ~XSTATE_XSAVES_ONLY;
> +
> +    for ( i = 2; i < min(63ul, ARRAY_SIZE(p->xstate.comp)); ++i )
> +    {
> +        uint64_t curr_xstate = 1ul << i;
> +
> +        if ( !(xstates & curr_xstate) )
> +            continue;
> +
> +        p->xstate.comp[i].size   = xstate_sizes[i];
> +        p->xstate.comp[i].offset = xstate_offsets[i];
> +        p->xstate.comp[i].xss    = curr_xstate & XSTATE_XSAVES_ONLY;
> +        p->xstate.comp[i].align  = curr_xstate & xstate_align;
> +
> +        /*
> +         * Sanity checks:
> +         * - All valid components should have non-zero size.
> +         * - All xcr0 components should have non-zero offset.
> +         * - All xss components should report 0 offset.
> +         */
> +        ASSERT(xstate_sizes[i]);
> +        if ( curr_xstate & XSTATE_XSAVES_ONLY )
> +            ASSERT(xstate_offsets[i] == 0);
> +        else
> +            ASSERT(xstate_offsets[i]);
> +    }

Hmm, now that I look at this again - what business do these
assertions have here? They're checking host information, which
isn't going to change post boot. Such checking, if indeed wanted,
should be done once during system boot.

I also think such checks should be consistent in style - either both
explicitly comparing with zero, or using ! in the if() branch to match
the else one.

> @@ -154,6 +152,13 @@ struct cpuid_policy
>              };
>              uint32_t /* b */:32, xss_low, xss_high;
>          };
> +
> +        /* Per-component common state.  Valid for i >= 2. */
> +        struct {
> +            uint32_t size, offset;
> +            bool xss:1, align:1;
> +            uint32_t _res_d;

I see you've decided against an inner union. Should be fine of
course, at least until we have a need to access the full ECX value
by name.

Jan

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