Re: [Xen-devel] [PATCH 3/4] x86/xstate: fix fault behavior on XRSTORS

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.01.16 at 17:48, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 29/01/16 10:29, Jan Beulich wrote:
>> --- a/xen/arch/x86/xstate.c
>> +++ b/xen/arch/x86/xstate.c
>> @@ -29,6 +29,8 @@ unsigned int *__read_mostly xstate_sizes
>>  static unsigned int __read_mostly xstate_features;
>>  static unsigned int __read_mostly 
>> xstate_comp_offsets[sizeof(xfeature_mask)*8];
>>  
>> +static uint32_t __read_mostly mxcsr_mask = MXCSR_DEFAULT;
> 
> The default mxcsr_mask value is 0xFFBF, which is different to the
> default mxcsr value.

Oh, right, of course. Clearly I have no old enough box in regular
use anymore to actually notice the difference.

>> -    case 4: case 2:
>> -        XRSTOR("");
>> -        break;
>> +                       ASM_OUTPUT2([mem] "+m" (*ptr), [faults] "+g" 
>> (faults)), \
>> +                       [lmask] "a" (lmask), [hmask] "d" (hmask), \
>> +                       [ptr] "D" (ptr))
>> +
>> +        default:
>> +            XRSTOR("0x48,");
> 
> Not relevant to this patch specifically, but this would be clearer if it
> actually used the rex64 mnemonic

Prefixing a .byte with rex64 is, well, odd?

>> +            break;
>> +        case 4: case 2:
>> +            XRSTOR("");
>> +            break;
>>  #undef XRSTOR
>> +        }
>> +        if ( likely(faults == prev_faults) )
>> +            break;
>> +#ifndef NDEBUG
>> +        gprintk(XENLOG_WARNING, "fault#%u: mxcsr=%08x\n",
>> +                faults, ptr->fpu_sse.mxcsr);
>> +        gprintk(XENLOG_WARNING, "xs=%016lx xc=%016lx\n",
>> +                ptr->xsave_hdr.xstate_bv, ptr->xsave_hdr.xcomp_bv);
>> +        gprintk(XENLOG_WARNING, "r0=%016lx r1=%016lx\n",
>> +                ptr->xsave_hdr.reserved[0], ptr->xsave_hdr.reserved[1]);
>> +        gprintk(XENLOG_WARNING, "r2=%016lx r3=%016lx\n",
>> +                ptr->xsave_hdr.reserved[2], ptr->xsave_hdr.reserved[3]);
>> +        gprintk(XENLOG_WARNING, "r4=%016lx r5=%016lx\n",
>> +                ptr->xsave_hdr.reserved[4], ptr->xsave_hdr.reserved[5]);
>> +#endif
>> +        switch ( faults )
>> +        {
>> +        case 1:
>> +            /* Stage 1: Reset state to be loaded. */
>> +            ptr->xsave_hdr.xstate_bv &= ~mask;
>> +            /*
>> +             * Also try to eliminate fault reasons, even if this shouldn't 
>> be
>> +             * needed here (other code should ensure the sanity of the 
>> data).
>> +             */
>> +            if ( ((mask & XSTATE_SSE) ||
>> +                  ((mask & XSTATE_YMM) &&
>> +                   !(ptr->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED))) 
>> )
>> +                ptr->fpu_sse.mxcsr &= mxcsr_mask;
>> +            if ( cpu_has_xsaves || cpu_has_xsavec )
>> +            {
>> +                ptr->xsave_hdr.xcomp_bv &= this_cpu(xcr0) | this_cpu(xss);
>> +                ptr->xsave_hdr.xstate_bv &= ptr->xsave_hdr.xcomp_bv;
>> +                ptr->xsave_hdr.xcomp_bv |= XSTATE_COMPACTION_ENABLED;
>> +            }
>> +            else
>> +            {
>> +                ptr->xsave_hdr.xstate_bv &= this_cpu(xcr0);
>> +                ptr->xsave_hdr.xcomp_bv = 0;
>> +            }
>> +            memset(ptr->xsave_hdr.reserved, 0, 
>> sizeof(ptr->xsave_hdr.reserved));
>> +            continue;
> 
> Newline here please.
> 
>> +        case 2:
>> +            /* Stage 2: Reset all state. */
>> +            ptr->fpu_sse.mxcsr = MXCSR_DEFAULT;
>> +            ptr->xsave_hdr.xstate_bv = 0;
>> +            ptr->xsave_hdr.xcomp_bv = cpu_has_xsaves
>> +                                      ? XSTATE_COMPACTION_ENABLED : 0;
>> +            continue;
> 
> And here.
> 
>> +        default:
>> +            domain_crash(current->domain);
>> +            break;
> 
> This breaks out of the switch statement, not out of the loop.  It looks
> like it will end in an infinite loop of calling domain_crash().

Oh, indeed, I forgot that: After all, the whole point of the
"continue"s was for there to be a break after the switch()'s end.

Jan


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