Re: [Xen-devel] [PATCH 2/5] X86 architecture instruction set extension definiation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.11.13 at 16:14, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> On 19/11/13 10:49, Liu, Jinsong wrote:
>> @@ -327,14 +321,33 @@ unsigned int xstate_ctxt_size(u64 xcr0)
>>      return ebx;
>>  }
>>  
>> +static bool_t valid_xcr0(u64 xcr0)
>> +{
> 
> Valid states in xcr0 ave very complicated, and are really not helped by
> having the dependencies split across several manuals.
> 
> I feel that for the sanity of someone trying to follow the code, there
> should be comments, and bits are validated in position order.
> 
> So,
> 
> /* XSTATE_FP must be unconditionally set */
> 
>> +    if ( !(xcr0 & XSTATE_FP) )
>> +        return 0;
>> +
> 
> /* XSTATE_YMM depends on XSTATE_SSE */
> 
>> +    if ( (xcr0 & XSTATE_YMM) && !(xcr0 & XSTATE_SSE) )
>> +        return 0;
> 
> /* XSTATE_BNDREGS and BNDCSR must be the same */
> if ( (xcr0 & XSTATE_BNDREGS) ^ (xcr0 & XSTATE_BNDCSR) )
>      return 0;
> 
> 
> /* XSTATE_{OPMASK,ZMM,HI_ZMM} must be the same, and require XSTATE_YMM */

Can be done of course (albeit I'm not inclined to change the
ordering - I'd rather keep XMM/YMM/ZMM stuff together, and
handle independent things separately).

>> +
>> +    if ( xcr0 & (XSTATE_OPMASK | XSTATE_ZMM | XSTATE_HI_ZMM) )
>> +    {
>> +        if ( !(xcr0 & XSTATE_YMM) )
>> +            return 0;
>> +
>> +        if ( ~xcr0 & (XSTATE_OPMASK | XSTATE_ZMM | XSTATE_HI_ZMM) )
>> +            return 0;
>> +    }
>> +
> 
> return 1;

Why?

> Shouldn't there also be a test against the xfeat_mask here, rather than
> at all callers ?

No, the purpose of the function is to validate a single set of flags
for internal consistency. The way validate_xstate() works makes it
so validating xcr0 against xfeature_mask is unnecessary (as xcr0
is already verified to be a subset of xcr0_accum).

>> -#define XSTATE_ALL     (~0)
>> -#define XSTATE_NONLAZY (XSTATE_LWP)
>> +#define XSTATE_ALL     (~(1ULL << 63))
> 
> Why has XSTATE_ALL changed to ~XSTATE_LWP ?

LWP is bit 62. Bit 63 is reserved.

Jan


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