fsincos emulation on AMD CPUs

fsincos emulation on AMD CPUs

[Jan Beulich]

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

Re: fsincos emulation on AMD CPUs

[Paolo Bonzini]

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

Re: fsincos emulation on AMD CPUs

[Jan Beulich]

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

Re: fsincos emulation on AMD CPUs

[Paolo Bonzini]

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

Re: fsincos emulation on AMD CPUs

[Jan Beulich]

>>> 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(&current_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

Re: fsincos emulation on AMD CPUs

[Keir Fraser]

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

Re: fsincos emulation on AMD CPUs

[Keir Fraser]

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

Re: fsincos emulation on AMD CPUs

[Jan Beulich]

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

Re: fsincos emulation on AMD CPUs

[Keir Fraser]

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

Re: fsincos emulation on AMD CPUs

[Jan Beulich]

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

Re: fsincos emulation on AMD CPUs

[Keir Fraser]

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
>