Re: [Xen-devel] fsincos emulation on AMD CPUs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.12.11 at 11:34, Paolo Bonzini <pbonzini@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel