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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/01/17 16:45, Jan Beulich wrote:
>>>> On 16.01.17 at 12:40, <andrew.cooper3@xxxxxxxxxx> wrote:
>> All data in the xstate union, other than the Da1 feature word, is derived 
>> from
>> other state; either feature bits from other words, or layout information 
>> which
>> has already been collected by Xen's xstate driver.
>>
>> Recalculate the xstate information for each policy object when the feature
>> bits may have changed.
>>
>> The XSTATE_XSAVES_ONLY define needs extending to a 64bit value to avoid
>> problems when taking its converse for masking purposes.
> I don't understand this part - plain 0 is a signed quantity, so ~0 would
> be sign extended to 64 bits as needed.

Hmm, now you point this out, I can't see how I came to this conclusion
to start with.  I will drop it.

>
>> --- 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.  Everything
>> +     * else is derived from other feature state.
>> +     */
> Almost.
>
>> +    memset(&p->xstate, 0, sizeof(p->xstate));
>> +
>> +    if ( !p->basic.xsave )
>> +        return;
> You're clobbering it here (but in all reality it should be zero in this
> case).

sanitise_featureset() should have made it all zero in the absence of xsave.

>
>> @@ -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 /* c */:30, /* d */:32;
>> +        } comp[CPUID_GUEST_NR_XSTATE];
> Hmm, can we rely on this functioning on varying complier variants?
> I think the standard doesn't exclude a uint32_t type bitfield to
> start on a 4-byte boundary if not following another uint32_t one.
> IOW I think we'd be better off giving the same type to all fields we
> want to share a storage unit.

Hmm.  In this case, something like:

bool xss:1, align:1;
uint32_t _res_d;

ought to work.

~Andrew

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