Re: [Xen-devel] [PATCH] x86emul: suppress memory writes after faulting FPU insns

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/01/17 16:12, Jan Beulich wrote:
>>>> On 12.01.17 at 16:04, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 12/01/17 14:02, Jan Beulich wrote:
>>> Furthermore I think we have another issue with writes: If the write
>>> faults, the FSW (or MXCSR, albeit there only for instructions we don't
>>> emulate yet) register may have been updated already, so we'd need to
>>> undo that update.
>> Do you mean restore the value before we sample it, or before the guest
>> gets to see it?
> Read it, run the stub, call ->write(), and upon failure restore the
> value read in the first step.
>
>> (I can't see what the problem is here.)
> The stub execution may modify FSW/MXCSR, if the operation causes
> an exception to be latched (for MXCSR this would need to be a
> masked exception), but if ->write() fails architecturally the update to
> FSW/MXCSR should not be committed.

Ok - I see now.  Yes - this is ugly corner case.  Short of doing a
pre-emptive fpu save before emulation, I don't see an alternative.  This
at least makes us no worse than taking a context switch.

>
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -3723,6 +3735,8 @@ x86_emulate(
>>>              default:
>>>                  generate_exception(EXC_UD);
>>>              }
>>> +            if ( dst.type == OP_MEM && dst.bytes == 4 && 
>>> !fpu_check_write() )
>>> +                dst.type = OP_NONE;
>> This dst.bytes check is rather suspicious, as the size of the operand
>> has nothing to do with whether the write should be surpressed.
>>
>> I presume you actually mean (modrm_reg & 7) < 6 to exclude fnstenv and
>> fnstcw from triggering the fpu_check_write() logic?
> I had it this way first, and then thought it's better the way it is now:
> The cases we want to exclude are the non-register-data stores,
> and in both groups all register stores are respectively uniform in size.
> Plus this way the conditional is slightly shorter (i.e. doesn't require
> splitting across lines). Yet if you strongly prefer the other variant, I
> can of course switch back. Just let me know.

As with everything here, clarity of code is the most important.

I'd prefer the modrm_reg check over dst.bytes, although would settle for
a comment describing the situations when we shouldn't suppress a write
despite an exception occuring.

~Andrew

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