Re: [PATCH] libxc/x86: XSAVE related adjustments

["Jan Beulich" <JBeulich@suse.com>]

>>> On 22.03.16 at 14:48, <andrew.cooper3@citrix.com> 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.

>> @@ -325,16 +329,20 @@ static void xc_cpuid_config_xsave(xc_int
> 
> Between these two hunks, there is a loop bound which is also wrong.

But seeing that your patches fix it I didn't bother stealing the fix
from your patches.

>>          if ( !info->hvm )
>>              regs[0] &= ~XSAVES;
>>          regs[2] &= info->xfeature_mask;
>> -        regs[3] = 0;
>> +        regs[3] &= info->xfeature_mask >> 32;
>>          break;
>> -    case 2 ... 63: /* sub-leaves */
>> +    case 2 ... 62: /* per-component sub-leaves */
>>          if ( !(info->xfeature_mask & (1ULL << input[1])) )
> 
> Now I think about it, this check is incomplete.  xfeature_mask doesn't
> contain xss values.

For now the XSS bitmask is blank. Looking at everything together I
do think though that once it becomes non-zero, info->xfeature_mask
will need to become the OR of both masks.

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

Jan


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