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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 22/11/13 11:25, Jan Beulich wrote:
>>>> On 21.11.13 at 16:14, Andrew Cooper <andrew.cooper3@citrix.com> 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?

That was based on the re-ordering of BNDREGS/BNDCSR, but is subject to
the choice of reordering.

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

Ok.

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

Oops - so it is.  In which case this change is certainly correct.

~Andrew