Re: [Xen-devel] [PATCH 2/4] x86emul: centralize put_fpu() invocations

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.03.17 at 12:55, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 13/03/17 11:03, Jan Beulich wrote:
>> @@ -1006,22 +1007,31 @@ do {
>>      rc = _get_fpu(_type, _fic, ctxt, ops);                      \
>>      if ( rc ) goto done;                                        \
>>  } while (0)
>> -#define _put_fpu()                                              \
>> +
>> +#define check_fpu(_fic)                                         \
>>  do {                                                            \
>> -    if ( ops->put_fpu != NULL )                                 \
>> -        (ops->put_fpu)(ctxt);                                   \
>> +    generate_exception_if((_fic)->exn_raised >= 0,              \
>> +                          (_fic)->exn_raised);                  \
>>  } while (0)
>> -#define put_fpu(_fic)                                           \
>> +
>> +#define check_xmm(_fic)                                         \
>>  do {                                                            \
>> -    _put_fpu();                                                 \
>>      if ( (_fic)->exn_raised == EXC_XM && ops->read_cr &&        \
>>           ops->read_cr(4, &cr4, ctxt) == X86EMUL_OKAY &&         \
>>           !(cr4 & X86_CR4_OSXMMEXCPT) )                              \
>>          (_fic)->exn_raised = EXC_UD;                            \
>> -    generate_exception_if((_fic)->exn_raised >= 0,              \
>> -                          (_fic)->exn_raised);                  \
>> +    check_fpu(_fic);                                            \
>>  } while (0)
> 
> These two check macros seem like they should have an _exn() suffix to
> make it clearer what they are actually doing.

Will do.

>> @@ -7905,7 +7912,8 @@ x86_emulate(
>>      ctxt->regs->eflags &= ~X86_EFLAGS_RF;
>>  
>>   done:
>> -    _put_fpu();
>> +    if ( rc != X86EMUL_OKAY )
>> +        put_fpu(&fic, ctxt, ops);
> 
> Unless you assert at complete_insn that rc == X86EMUL_OKAY, you still
> might call put_fpu() twice.

Well, reaching complete_insn with rc != X86EMUL_OKAY is
possible, and hence can't be asserted. See the X86EMUL_DONE
handling there. One could assert right before the done label, but
then again reaching complete_insn with any other X86EMUL_*
result would have been a bug even before this series. But
anyway, I guess I'll rather do ...

> You can avoid the conditional here by setting fic->type to
> X86EMUL_FPU_none at the end of put_fpu(), which looks like it is
> compatible with your later modifications.

... this.

>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>> @@ -161,7 +161,9 @@ enum x86_emulate_fpu_type {
>>      X86EMUL_FPU_wait, /* WAIT/FWAIT instruction */
>>      X86EMUL_FPU_mmx, /* MMX instruction set (%mm0-%mm7) */
>>      X86EMUL_FPU_xmm, /* SSE instruction set (%xmm0-%xmm7/15) */
>> -    X86EMUL_FPU_ymm  /* AVX/XOP instruction set (%ymm0-%ymm7/15) */
>> +    X86EMUL_FPU_ymm, /* AVX/XOP instruction set (%ymm0-%ymm7/15) */
>> +    /* This sentinel will never be passed to ->get_fpu(). */
> 
> Don't you mean ->put_fpu() here?

Definitely not, as patch 3 would be violating this then immediately.

>  Either way, can we assert this fact?

I did consider this, but decided against because of the scalability
aspect (every get_fpu() implementation would then have to have
this). But then again, perhaps you'd be fine with the assertion
being put in _get_fpu(), right next to the call of the hook?

Jan


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