Re: [Xen-devel] [PATCH v2 2/3] x86emul: correct handling of FPU insns faulting on memory write

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.03.17 at 14:48, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 03/15/2017 09:31 AM, Jan Beulich wrote:
>>>>> On 15.03.17 at 14:24, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> On 03/15/2017 06:28 AM, Jan Beulich wrote:
>>>> @@ -3716,9 +3720,9 @@ x86_emulate(
>>>>          break;
>>>>  
>>>>      case 0x9b:  /* wait/fwait */
>>>> -        fic.insn_bytes = 1;
>>>>          host_and_vcpu_must_have(fpu);
>>>>          get_fpu(X86EMUL_FPU_wait, &fic);
>>>> +        fic.insn_bytes = 1;
>>>>          asm volatile ( "fwait" ::: "memory" );
>>>>          check_fpu_exn(&fic);
>>>>          break;
>>> Why is this needed?
>> This isn't strictly needed, but desirable, due to the conditional being
>> added in
>>
>> @@ -7916,7 +7920,7 @@ x86_emulate(
>>      ctxt->regs->eflags &= ~X86_EFLAGS_RF;
>>  
>>   done:
>> -    put_fpu(&fic, ctxt, ops);
>> +    put_fpu(&fic, fic.insn_bytes > 0 && dst.type == OP_MEM, ctxt, ops);
>>      put_stub(stub);
>>      return rc;
>>  #undef state
>>
>> (both host_and_vcpu_must_have() and get_fpu() may end up
>> branching to "done"). Everywhere else the field is already being
>> set after such basic checks.
> 
> Ah, OK.
> 
> But fic is a local variable that is not initialized (is it?) so
> insn_bytes may be non-zero anyway?

We have this at the top of x86_emulate():

    struct fpu_insn_ctxt fic = { .type = X86EMUL_FPU_none, .exn_raised = -1 };

(introduced by patch 1).

Jan


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