All, in the light of erratum #573 I''m wondering if we need to tweak or conditionally suppress fsincos emulation. The question is whether there is any possibility for getting the emulator to hit this instruction on AMD (as no real mode emulation ought to be taking place there), i.e. whether there are places where emulation gets continued eagerly in anticipation of the need for emulation on a nearby instruction. I don''t think that''s happening, but I''d like to be certain. Even in the absence of any present possibility I would question whether it wouldn''t make sense to filter this situation so that there''s no latent potential for running into problems here (read: opening a security hole) in the future. Jan
On 12/15/2011 09:38 AM, Jan Beulich wrote:> All, > > in the light of erratum #573 I''m wondering if we need to tweak or > conditionally suppress fsincos emulation. The question is whether there > is any possibility for getting the emulator to hit this instruction on AMD > (as no real mode emulation ought to be taking place there), i.e. > whether there are places where emulation gets continued eagerly > in anticipation of the need for emulation on a nearby instruction.This can happen with PAE + shadow pagetables. There''s also the case when a user process issues an instruction to an MMIO region, and another thread replaces the instruction with another (fsincos in this case), racing with the emulator until the emulator sees fsincos instead of the MMIO instruction. If you really cared, perhaps fsincos can be replaced by this sequence in the emulator: ; x fld %st ; x x fsin ; x sin(x) fxch %st(1) ; sin(x) x fcos ; sin(x) cos(x) Paolo
>>> On 15.12.11 at 09:54, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 12/15/2011 09:38 AM, Jan Beulich wrote: >> in the light of erratum #573 I''m wondering if we need to tweak or >> conditionally suppress fsincos emulation. The question is whether there >> is any possibility for getting the emulator to hit this instruction on AMD >> (as no real mode emulation ought to be taking place there), i.e. >> whether there are places where emulation gets continued eagerly >> in anticipation of the need for emulation on a nearby instruction. > > This can happen with PAE + shadow pagetables.Ah okay. In that case we ought to add some workaround here.> There''s also the case when a user process issues an instruction to an > MMIO region, and another thread replaces the instruction with another > (fsincos in this case), racing with the emulator until the emulator sees > fsincos instead of the MMIO instruction.Indeed, didn''t think of this possibility.> If you really cared, perhaps fsincos can be replaced by this sequence in > the emulator: > > ; x > fld %st ; x x > fsin ; x sin(x) > fxch %st(1) ; sin(x) x > fcos ; sin(x) cos(x)I had thought of this at first too, but this is problematic in terms of exception handling: fpu_handle_exception() expects to see an exception only on the very first instruction (as it''s assumed to be the only one), and aborts the rest of the sequence if the exception doesn''t happen on the last instruction. All of this can be fixed of course, but I wonder whether it''s worth it when we really could just bail from the emulator without causing any harm. Jan
On 12/15/2011 11:15 AM, Jan Beulich wrote:>> > If you really cared, perhaps fsincos can be replaced by this sequence in >> > the emulator: >> > >> > ; x >> > fld %st ; x x >> > fsin ; x sin(x) >> > fxch %st(1) ; sin(x) x >> > fcos ; sin(x) cos(x) > I had thought of this at first too, but this is problematic in terms of > exception handling: fpu_handle_exception() expects to see an > exception only on the very first instruction (as it''s assumed to be > the only one), and aborts the rest of the sequence if the exception > doesn''t happen on the last instruction.Can it just be (%0 is fic.insn_bytes): movb $4f-1f,%0 ; do nothing on exception here 1: fld %st ; x x movb $3f-1f,%0 ; pop on exception here 1: fsin ; x sin(x) fxch %st(1) ; sin(x) x movb $2f-1f,%0 ; xch+pop on exception here 1: fcos ; sin(x) cos(x) jmp 2f 4: fxch %st(1) ; x sin(x) 3: fstp %st ; x 2: Paolo
>>> On 15.12.11 at 11:34, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 12/15/2011 11:15 AM, Jan Beulich wrote: >>> > If you really cared, perhaps fsincos can be replaced by this sequence in >>> > the emulator: >>> > >>> > ; x >>> > fld %st ; x x >>> > fsin ; x sin(x) >>> > fxch %st(1) ; sin(x) x >>> > fcos ; sin(x) cos(x) >> I had thought of this at first too, but this is problematic in terms of >> exception handling: fpu_handle_exception() expects to see an >> exception only on the very first instruction (as it''s assumed to be >> the only one), and aborts the rest of the sequence if the exception >> doesn''t happen on the last instruction. > > Can it just be (%0 is fic.insn_bytes): > > movb $4f-1f,%0 ; do nothing on exception here > 1: fld %st ; x x > movb $3f-1f,%0 ; pop on exception here > 1: fsin ; x sin(x) > fxch %st(1) ; sin(x) x > movb $2f-1f,%0 ; xch+pop on exception here > 1: fcos ; sin(x) cos(x) > jmp 2f > 4: fxch %st(1) ; x sin(x) > 3: fstp %st ; x > 2:Ugly, but possible (with some further corrections). I''d still prefer to just suppress the emulation: --- atools/tests/x86_emulator/x86_emulate.c +++ b/tools/tests/x86_emulator/x86_emulate.c @@ -9,5 +9,7 @@ typedef bool bool_t; #define BUG() abort() +#define cpu_has_amd_erratum(nr) 0 + #include "x86_emulate/x86_emulate.h" #include "x86_emulate/x86_emulate.c" --- a/xen/arch/x86/x86_emulate.c +++ b/xen/arch/x86/x86_emulate.c @@ -10,8 +10,14 @@ */ #include <asm/x86_emulate.h> +#include <asm/processor.h> /* current_cpu_info */ +#include <asm/amd.h> /* cpu_has_amd_erratum() */ /* Avoid namespace pollution. */ #undef cmpxchg +#undef cpuid + +#define cpu_has_amd_erratum(nr) \ + cpu_has_amd_erratum(¤t_cpu_data, AMD_ERRATUM_##nr) #include "x86_emulate/x86_emulate.c" --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -2761,6 +2761,9 @@ x86_emulate( case 0xd9: /* FPU 0xd9 */ switch ( modrm ) { + case 0xfb: /* fsincos */ + fail_if(cpu_has_amd_erratum(573)); + /* fall through */ case 0xc0 ... 0xc7: /* fld %stN */ case 0xc8 ... 0xcf: /* fxch %stN */ case 0xd0: /* fnop */ @@ -2786,7 +2789,6 @@ x86_emulate( case 0xf8: /* fprem */ case 0xf9: /* fyl2xp1 */ case 0xfa: /* fsqrt */ - case 0xfb: /* fsincos */ case 0xfc: /* frndint */ case 0xfd: /* fscale */ case 0xfe: /* fsin */ --- a/xen/include/asm-x86/amd.h +++ b/xen/include/asm-x86/amd.h @@ -134,6 +134,12 @@ AMD_OSVW_ERRATUM(3, AMD_MODEL_RANGE(0x10, 0x2, 0x1, 0xff, 0xf), \ AMD_MODEL_RANGE(0x12, 0x0, 0x0, 0x1, 0x0)) +#define AMD_ERRATUM_573 \ + AMD_LEGACY_ERRATUM(AMD_MODEL_RANGE(0x0f, 0x0, 0x0, 0xff, 0xf), \ + AMD_MODEL_RANGE(0x10, 0x0, 0x0, 0xff, 0xf), \ + AMD_MODEL_RANGE(0x11, 0x0, 0x0, 0xff, 0xf), \ + AMD_MODEL_RANGE(0x12, 0x0, 0x0, 0xff, 0xf)) + struct cpuinfo_x86; int cpu_has_amd_erratum(const struct cpuinfo_x86 *, int, ...); Keir, what''s your opinion here? Jan
On 15/12/2011 11:15, "Jan Beulich" <JBeulich@suse.com> wrote:> +#define AMD_ERRATUM_573 \ > + AMD_LEGACY_ERRATUM(AMD_MODEL_RANGE(0x0f, 0x0, 0x0, 0xff, 0xf), \ > + AMD_MODEL_RANGE(0x10, 0x0, 0x0, 0xff, 0xf), \ > + AMD_MODEL_RANGE(0x11, 0x0, 0x0, 0xff, 0xf), \ > + AMD_MODEL_RANGE(0x12, 0x0, 0x0, 0xff, 0xf)) > + > struct cpuinfo_x86; > int cpu_has_amd_erratum(const struct cpuinfo_x86 *, int, ...); > > Keir, what''s your opinion here?Bail. :-) -- Keir
On 15/12/2011 12:27, "Keir Fraser" <keir@xen.org> wrote:> On 15/12/2011 11:15, "Jan Beulich" <JBeulich@suse.com> wrote: > >> +#define AMD_ERRATUM_573 \ >> + AMD_LEGACY_ERRATUM(AMD_MODEL_RANGE(0x0f, 0x0, 0x0, 0xff, 0xf), \ >> + AMD_MODEL_RANGE(0x10, 0x0, 0x0, 0xff, 0xf), \ >> + AMD_MODEL_RANGE(0x11, 0x0, 0x0, 0xff, 0xf), \ >> + AMD_MODEL_RANGE(0x12, 0x0, 0x0, 0xff, 0xf)) >> + >> struct cpuinfo_x86; >> int cpu_has_amd_erratum(const struct cpuinfo_x86 *, int, ...); >> >> Keir, what''s your opinion here? > > Bail. :-)More detail: the full patch is ugly and hard to test all cases. And there''s no practical scenario where we want to emulate FSINCOS on AMD -- we don''t emulate realmode on AMD, FSINCOS on a shadowed page certainly indicates that we should unshadow the page, FSINCOS on MMIO is mad or malicious. Pretty much the whole x87 emulation thing is for realmode on Intel. -- Keir> -- Keir > >
>>> On 15.12.11 at 13:33, Keir Fraser <keir@xen.org> wrote: > More detail: the full patch is ugly and hard to test all cases. And there''s > no practical scenario where we want to emulate FSINCOS on AMD -- we don''t > emulate realmode on AMD, FSINCOS on a shadowed page certainly indicates that > we should unshadow the page, FSINCOS on MMIO is mad or malicious.Those latter two cases can''t really happen, as fsincos has no memory operand. Jan
On 15/12/2011 13:08, "Jan Beulich" <JBeulich@suse.com> wrote:>>>> On 15.12.11 at 13:33, Keir Fraser <keir@xen.org> wrote: >> More detail: the full patch is ugly and hard to test all cases. And there''s >> no practical scenario where we want to emulate FSINCOS on AMD -- we don''t >> emulate realmode on AMD, FSINCOS on a shadowed page certainly indicates that >> we should unshadow the page, FSINCOS on MMIO is mad or malicious. > > Those latter two cases can''t really happen, as fsincos has no memory > operand.Possibly if the instruction itself was in a recycled page-table page? Or in an MMIO page, or the malicious race that Paolo described --- definitely malicious either way. Anyhow the short answer is we never want to emulate it on AMD. :-) -- Keir
>>> On 15.12.11 at 14:13, Keir Fraser <keir@xen.org> wrote: > On 15/12/2011 13:08, "Jan Beulich" <JBeulich@suse.com> wrote: > >>>>> On 15.12.11 at 13:33, Keir Fraser <keir@xen.org> wrote: >>> More detail: the full patch is ugly and hard to test all cases. And there''s >>> no practical scenario where we want to emulate FSINCOS on AMD -- we don''t >>> emulate realmode on AMD, FSINCOS on a shadowed page certainly indicates that >>> we should unshadow the page, FSINCOS on MMIO is mad or malicious. >> >> Those latter two cases can''t really happen, as fsincos has no memory >> operand. > > Possibly if the instruction itself was in a recycled page-table page? Or in > an MMIO page, or the malicious race that Paolo described --- definitely > malicious either way. > > Anyhow the short answer is we never want to emulate it on AMD. :-)I just sent out the patch as quoted in the reply to Paolo, but you''re suggesting to be even more drastic and ignore the CPU family. If you really want it done that way, I wonder whether we should bail on AMD for *all* x87 operations not having memory operands. Jan
On 15/12/2011 13:19, "Jan Beulich" <JBeulich@suse.com> wrote:>>>> On 15.12.11 at 14:13, Keir Fraser <keir@xen.org> wrote: >> On 15/12/2011 13:08, "Jan Beulich" <JBeulich@suse.com> wrote: >> >>>>>> On 15.12.11 at 13:33, Keir Fraser <keir@xen.org> wrote: >>>> More detail: the full patch is ugly and hard to test all cases. And there''s >>>> no practical scenario where we want to emulate FSINCOS on AMD -- we don''t >>>> emulate realmode on AMD, FSINCOS on a shadowed page certainly indicates >>>> that >>>> we should unshadow the page, FSINCOS on MMIO is mad or malicious. >>> >>> Those latter two cases can''t really happen, as fsincos has no memory >>> operand. >> >> Possibly if the instruction itself was in a recycled page-table page? Or in >> an MMIO page, or the malicious race that Paolo described --- definitely >> malicious either way. >> >> Anyhow the short answer is we never want to emulate it on AMD. :-) > > I just sent out the patch as quoted in the reply to Paolo, but you''re > suggesting to be even more drastic and ignore the CPU family. If > you really want it done that way, I wonder whether we should bail on > AMD for *all* x87 operations not having memory operands.Your patch is fine. -- Keir> Jan >