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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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