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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/01/17 12:52, Jan Beulich wrote:
>>>> 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.

Ah yes - will fix.

>
>> +    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 put this in to try and ensure that we don't put junk into the policy,
but thinking about it more, by the time do have junk in these arrays, we
have bigger xstate problems.  I will drop these from here and focus on
beefing up the xstate driver itself.

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

Oh - I misinterpreted what you meant then.

Did you mean

struct {
    uint32_t size, offset;
    union {
        struct {
            bool xss:1, align:1;
        };
        uint32_t c;
    };
    uint32_t /* d */:32;
};

Then?

~Andrew

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