Re: [Xen-devel] [PATCH] x86emul: suppress memory writes after faulting FPU insns
>>> On 12.01.17 at 17:43, wrote: > On 12/01/17 16:12, Jan Beulich wrote: > On 12.01.17 at 16:04, 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. And apparently one which even hardware gets wrong: While FPU insns (I've tried just one) look to work as expected, VCVTPS2PH updates MXCSR even when raising #PF. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86emul: suppress memory writes after faulting FPU insns
>>> On 12.01.17 at 17:43, wrote: > On 12/01/17 16:12, Jan Beulich wrote: > On 12.01.17 at 16:04, 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. Not exactly - we don't need a full save as long as insns writing to memory don't alter other registers (that'll become an issue with vscatter). It would be sufficient to store FSW up front, leaving most of the additional overhead to just the failed-write path. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86emul: suppress memory writes after faulting FPU insns
On 12/01/17 16:12, Jan Beulich wrote: On 12.01.17 at 16:04, 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@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86emul: suppress memory writes after faulting FPU insns
>>> On 12.01.17 at 16:04, 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@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86emul: suppress memory writes after faulting FPU insns
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
> ---
> 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@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH] x86emul: suppress memory writes after faulting FPU insns
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
---
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.
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. 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
@@ -457,6 +457,9 @@ typedef union {
#define EFLG_MBS (1<<1)
#define EFLG_CF (1<<0)
+/* Floating point status word definitions. */
+#define FSW_ES(1U << 7)
+
/* MXCSR bit definitions. */
#define MXCSR_MM (1U << 17)
@@ -868,6 +871,15 @@ do {
(_fic)->exn_raised); \
} while (0)
+static inline bool fpu_check_write(void)
+{
+uint16_t fsw;
+
+asm ( "fnstsw %0" : "=am" (fsw) );
+
+return !(fsw & FSW_ES);
+}
+
#define emulate_fpu_insn(_op) \
asm volatile ( \
"movb $2f-1f,%0 \n" \
@@ -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;
}
put_fpu(&fic);
break;
@@ -3835,7 +3849,8 @@ x86_emulate(
case 7: /* fstp m80fp */
fail_if(!ops->write);
emulate_fpu_insn_memdst("fstpt", *mmvalp);
-if ( (rc = ops->write(ea.mem.seg, ea.mem.off, mmvalp,
+if ( fpu_check_write() &&
+ (rc = ops->write(ea.mem.seg, ea.mem.off, mmvalp,
10, ctxt)) != X86EMUL_OKAY )
goto done;
dst.type = OP_NONE;
@@ -3843,6 +3858,8 @@ x86_emulate(
default:
generate_exception(EXC_UD);
}
+if ( dst.type == OP_MEM && !fpu_check_write() )
+dst.type = OP_NONE;
}
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;
}
put_fpu(&fic);
break;
@@ -4063,7 +4082,8 @@ x86_emulate(
case 6: /* fbstp packed bcd */
fail_if(!ops->write);
emulate_fpu_insn_memdst("fbstp", *mmvalp);
-if ( (rc = ops->write(ea.mem.seg, ea.mem.off, mmvalp,
+if ( fpu_check_write() &&
+ (rc = ops->write(ea.mem.seg, ea.mem.off, mmvalp,
10, ctxt)) != X86EMUL_OKAY )
goto done;
dst.type = OP_NONE;
@@ -4073,6 +4093,8 @@ x86_emulate(
dst.bytes = 8;
break;
}
+if ( dst.type == OP_MEM && !fpu_check_write() )
+dst.type = OP_NONE;
}
put_fpu(&fic);
break;
x86emul: suppress memory writes after faulting FPU insns
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
---
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
