Re: [Xen-devel] [PATCH] libxc/x86: XSAVE related adjustments

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.03.16 at 18:23, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 22/03/16 14:46, Jan Beulich wrote:
>>>>> On 22.03.16 at 14:48, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> @@ -300,9 +304,9 @@ static void xc_cpuid_config_xsave(xc_int
>>>>      {
>>>>      case 0: 
>>>>          /* EAX: low 32bits of xfeature_enabled_mask */
>>>> -        regs[0] = info->xfeature_mask & 0xFFFFFFFF;
>>>> +        regs[0] &= info->xfeature_mask;
>>>>          /* EDX: high 32bits of xfeature_enabled_mask */
>>>> -        regs[3] = (info->xfeature_mask >> 32) & 0xFFFFFFFF;
>>>> +        regs[3] &= info->xfeature_mask >> 32;
>>>>          /* ECX: max size required by all HW features */
>>>>          {
>>>>              unsigned int _input[2] = {0xd, 0x0}, _regs[4];
>>> This is an improvement on the code currently present, but is still
>>> superseded by the final patch of my cpuid series.
>> Is it? I did check your tree before sending, and you do only
>> mechanical adjustments. In particular you don't switch to
>> &= and you don't drop the pointless and-ing with 0xFFFFFFFF.
> 
> Using &= is specifically wrong and buggy.  My patch replaces
> info->xfeature_mask with guest_xfeature_mask, which itself is calculated
> from the guest feature availability.

Oh, right - if you imply buggy just if it was done with or after your
change.

>>>>          {
>>>>              regs[0] = regs[1] = regs[2] = regs[3] = 0;
>>>>              break;
>>>>          }
>>>>          /* Don't touch EAX, EBX. Also cleanup ECX and EDX */
>>>> -        regs[2] = regs[3] = 0;
>>>> +        regs[2] &= XSTATE_XSS | XSTATE_ALIGN64;
>>>> +        regs[3] = 0;
>>>> +        break;
>>>> +    default:
>>>> +        regs[0] = regs[1] = regs[2] = regs[3] = 0;
>>>>          break;
>>> If you wish, I can fold this patch into the final patch of my cpuid series.
>> I'd be fine with that, albeit (as said in the submission) the changes
>> are independent of one another despite them causing conflicts.
> 
> It would be clearer than having two different patches both fixing part
> of the code.

Especially with the above in mind - go ahead.

Jan


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