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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/01/17 14:02, Jan Beulich wrote:
> FPU insns writing to memory must not touch memory if they latch #MF (to
> be delivered on the next waiting FPU insn). Note that inspecting FSW.ES
> needs to be avoided for all FNST* insns, as they don't raise exceptions
> themselves, but may instead be invoked with the bit already set.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> While #MF and memory access faults are all listed in the same priority
> group, it is not entirely clear how FPU insns reading memory operate
> when an exception to be delivered doesn't depend on the memory operand
> (which would namely be FPU register stack overflows). It is therefore
> possible that memory reads would need to be suppressed in some
> situations, too. Of course this only matters for reads which have side
> effects. Otoh SNaN operand detection and stack overflow/underflow are
> listed as having the same priority, so the memory read may well be
> performed unconditionally.

This can be checked by using pagetable access bits, although I can't see
any sensible case where one would point the x87 FPU at MMIO (at all, let
alone) with read side effects.

I think it is reasonable to leave reads as they currently are.

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

(I can't see what the problem is here.)

>  For MXCSR that will be possible by saving the initial
> value and re-loading it in case ->write() fails (and iirc there's
> exactly one affected insn - VCVTPS2PH). There's no suitable way to load
> FSW, though - all existing mechanisms have further effects we don't
> really want (albeit arguably the side effects of going through a
> {F,}XSAVE/{F,}XRSTOR cycle could occur at any time as a scheduling side
> effect).
>
> --- 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?

>          }
>          put_fpu(&fic);
>          break;
> @@ -3946,6 +3963,8 @@ x86_emulate(
>              default:
>                  generate_exception(EXC_UD);
>              }
> +            if ( dst.type == OP_MEM && dst.bytes == 8 && !fpu_check_write() )
> +                dst.type = OP_NONE;

Same again here.

Otherwise, it looks fine.

~Andrew

>          }
>          put_fpu(&fic);
>          break;

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