[PATCH 0/3] tests/x86emul: Fix register corruption in the test harness

[PATCH 0/3] tests/x86emul: Fix register corruption in the test harness

[Andrew Cooper]

The main bugfix is in patch 2.  Patch 3 is some extra debugging developed
while investigating the issue.

Andrew Cooper (3):
  tests/x86emul: Helpers to save and restore FPU state
  tests/x86emul: Save and restore FPU state in the emulator callbacks
  tests/x86emul: Improve the utility of verbose mode

 tools/tests/x86_emulator/test_x86_emulator.c | 185 +++++++++++++++++++++++----
 tools/tests/x86_emulator/x86-emulate.c       |  70 ++++++++--
 tools/tests/x86_emulator/x86-emulate.h       |   4 +
 3 files changed, 222 insertions(+), 37 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/3] tests/x86emul: Helpers to save and restore FPU state

[Andrew Cooper]

Introduce common helpers for saving and restoring FPU state.  During
emul_test_init(), calculate whether to use xsave or fxsave, and tweak the
existing mxcsr_mask logic to avoid using another large static buffer.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 tools/tests/x86_emulator/x86-emulate.c | 70 +++++++++++++++++++++++++++-------
 tools/tests/x86_emulator/x86-emulate.h |  4 ++
 2 files changed, 60 insertions(+), 14 deletions(-)

diff --git a/tools/tests/x86_emulator/x86-emulate.c b/tools/tests/x86_emulator/x86-emulate.c
index 9056610..47e503d 100644
--- a/tools/tests/x86_emulator/x86-emulate.c
+++ b/tools/tests/x86_emulator/x86-emulate.c
@@ -26,26 +26,68 @@
 
 uint32_t mxcsr_mask = 0x0000ffbf;
 
+static char fpu_save_area[4096] __attribute__((__aligned__((64))));
+static bool use_xsave;
+
+void emul_save_fpu_state(void)
+{
+    if ( use_xsave )
+        asm volatile ( "xsave" __OS " %[ptr]"
+                       : [ptr] "=m" (fpu_save_area)
+                       : "a" (~0ull), "d" (~0ull) );
+    else
+        asm volatile ( "fxsave %0" : "=m" (fpu_save_area) );
+}
+
+void emul_restore_fpu_state(void)
+{
+    if ( use_xsave )
+        asm volatile ( "xrstor" __OS " %[ptr]"
+                       :: [ptr] "m" (fpu_save_area), "a" (~0ull), "d" (~0ull) );
+    else
+        asm volatile ( "fxrstor %0" :: "m" (fpu_save_area) );
+}
+
 bool emul_test_init(void)
 {
+    union {
+        char x[464];
+        struct {
+            uint32_t other[6];
+            uint32_t mxcsr;
+            uint32_t mxcsr_mask;
+            /* ... */
+        };
+    } *fxs = (void *)fpu_save_area;
+
     unsigned long sp;
 
-    if ( cpu_has_fxsr )
+    if ( cpu_has_xsave )
     {
-        static union __attribute__((__aligned__(16))) {
-            char x[464];
-            struct {
-                uint32_t other[6];
-                uint32_t mxcsr;
-                uint32_t mxcsr_mask;
-                /* ... */
-            };
-        } fxs;
-
-        asm ( "fxsave %0" : "=m" (fxs) );
-        if ( fxs.mxcsr_mask )
-            mxcsr_mask = fxs.mxcsr_mask;
+        unsigned int tmp, ebx;
+
+        asm ( "cpuid"
+              : "=a" (tmp), "=b" (ebx), "=c" (tmp), "=d" (tmp)
+              : "a" (0xd), "c" (0) );
+
+        /*
+         * Sanity check that fpu_save_area[] is large enough.  This assertion
+         * will trip eventually, at which point fpu_save_area[] needs to get
+         * larger.
+         */
+        assert(ebx < sizeof(fpu_save_area));
+
+        /* Use xsave if available... */
+        use_xsave = true;
     }
+    else
+        /* But use fxsave if xsave isn't available. */
+        assert(cpu_has_fxsr);
+
+    /* Reuse the save state buffer to find mcxsr_mask. */
+    asm ( "fxsave %0" : "=m" (*fxs) );
+    if ( fxs->mxcsr_mask )
+        mxcsr_mask = fxs->mxcsr_mask;
 
     /*
      * Mark the entire stack executable so that the stub executions
diff --git a/tools/tests/x86_emulator/x86-emulate.h b/tools/tests/x86_emulator/x86-emulate.h
index a1c4774..45acea4 100644
--- a/tools/tests/x86_emulator/x86-emulate.h
+++ b/tools/tests/x86_emulator/x86-emulate.h
@@ -52,6 +52,10 @@ extern uint32_t mxcsr_mask;
 #define MMAP_SZ 16384
 bool emul_test_init(void);
 
+/* Must save and restore FPU state between any call into libc. */
+void emul_save_fpu_state(void);
+void emul_restore_fpu_state(void);
+
 #include "x86_emulate/x86_emulate.h"
 
 static inline uint64_t xgetbv(uint32_t xcr)
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/3] tests/x86emul: Save and restore FPU state in the emulator callbacks

[Andrew Cooper]

Currently with then native toolchain on Debian Jessie ./test_x86_emulator
yeilds:

  Testing AVX2 256bit single native execution...okay
  Testing AVX2 256bit single 64-bit code sequence...[line 933] failed!

The bug is that libc's memcpy() in read() uses %xmm8 (specifically, in
__memcpy_sse2_unaligned()), which corrupts %ymm8 behind the back of the AVX2
test code.

Switch all hooks to use "goto out" style returns, and use
emul_{save,restore}_fpu_state().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 tools/tests/x86_emulator/test_x86_emulator.c | 94 +++++++++++++++++++++++-----
 1 file changed, 79 insertions(+), 15 deletions(-)

diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c
index 625cd2a..a764d99 100644
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -226,6 +226,10 @@ static int read(
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
+    int rc = X86EMUL_OKAY;
+
+    emul_save_fpu_state();
+
     if ( verbose )
         printf("** %s(%u, %p,, %u,)\n", __func__, seg, (void *)offset, bytes);
 
@@ -236,42 +240,61 @@ static int read(
     case x86_seg_gdtr:
         /* Fake system segment type matching table index. */
         if ( (offset & 7) || (bytes > 8) )
-            return X86EMUL_UNHANDLEABLE;
+        {
+            rc = X86EMUL_UNHANDLEABLE;
+            goto out;
+        }
 #ifdef __x86_64__
         if ( !(offset & 8) )
         {
             memset(p_data, 0, bytes);
-            return X86EMUL_OKAY;
+            goto out;
         }
         value = (offset - 8) >> 4;
 #else
         value = (offset - 8) >> 3;
 #endif
         if ( value >= 0x10 )
-            return X86EMUL_UNHANDLEABLE;
+        {
+            rc = X86EMUL_UNHANDLEABLE;
+            goto out;
+        }
         value |= value << 40;
         memcpy(p_data, &value, bytes);
-        return X86EMUL_OKAY;
+        goto out;
 
     case x86_seg_ldtr:
         /* Fake user segment type matching table index. */
         if ( (offset & 7) || (bytes > 8) )
-            return X86EMUL_UNHANDLEABLE;
+        {
+            rc = X86EMUL_UNHANDLEABLE;
+            goto out;
+        }
         value = offset >> 3;
         if ( value >= 0x10 )
-            return X86EMUL_UNHANDLEABLE;
+        {
+            rc = X86EMUL_UNHANDLEABLE;
+            goto out;
+        }
         value |= (value | 0x10) << 40;
         memcpy(p_data, &value, bytes);
-        return X86EMUL_OKAY;
+        goto out;
 
     default:
         if ( !is_x86_user_segment(seg) )
-            return X86EMUL_UNHANDLEABLE;
+        {
+            rc = X86EMUL_UNHANDLEABLE;
+            goto out;
+        }
         bytes_read += bytes;
         break;
     }
     memcpy(p_data, (void *)offset, bytes);
-    return X86EMUL_OKAY;
+
+ out:
+    emul_restore_fpu_state();
+
+    return rc;
 }
 
 static int fetch(
@@ -281,10 +304,15 @@ static int fetch(
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
+    emul_save_fpu_state();
+
     if ( verbose )
         printf("** %s(%u, %p,, %u,)\n", __func__, seg, (void *)offset, bytes);
 
     memcpy(p_data, (void *)offset, bytes);
+
+    emul_restore_fpu_state();
+
     return X86EMUL_OKAY;
 }
 
@@ -295,13 +323,25 @@ static int write(
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
+    int rc = X86EMUL_OKAY;
+
+    emul_save_fpu_state();
+
     if ( verbose )
         printf("** %s(%u, %p,, %u,)\n", __func__, seg, (void *)offset, bytes);
 
     if ( !is_x86_user_segment(seg) )
-        return X86EMUL_UNHANDLEABLE;
+    {
+        rc = X86EMUL_UNHANDLEABLE;
+        goto out;
+    }
+
     memcpy((void *)offset, p_data, bytes);
-    return X86EMUL_OKAY;
+
+ out:
+    emul_restore_fpu_state();
+
+    return rc;
 }
 
 static int cmpxchg(
@@ -312,13 +352,25 @@ static int cmpxchg(
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
+    int rc = X86EMUL_OKAY;
+
+    emul_save_fpu_state();
+
     if ( verbose )
         printf("** %s(%u, %p,, %u,)\n", __func__, seg, (void *)offset, bytes);
 
     if ( !is_x86_user_segment(seg) )
-        return X86EMUL_UNHANDLEABLE;
+    {
+        rc = X86EMUL_UNHANDLEABLE;
+        goto out;
+    }
+
     memcpy((void *)offset, new, bytes);
-    return X86EMUL_OKAY;
+
+ out:
+    emul_restore_fpu_state();
+
+    return rc;
 }
 
 static int read_segment(
@@ -326,11 +378,23 @@ static int read_segment(
     struct segment_register *reg,
     struct x86_emulate_ctxt *ctxt)
 {
+    int rc = X86EMUL_OKAY;
+
+    emul_save_fpu_state();
+
     if ( !is_x86_user_segment(seg) )
-        return X86EMUL_UNHANDLEABLE;
+    {
+        rc = X86EMUL_UNHANDLEABLE;
+        goto out;
+    }
+
     memset(reg, 0, sizeof(*reg));
     reg->p = 1;
-    return X86EMUL_OKAY;
+
+ out:
+    emul_restore_fpu_state();
+
+    return rc;
 }
 
 static int read_msr(
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/3] tests/x86emul: Improve the utility of verbose mode

[Andrew Cooper]

 * Align the function call names, which aligns all subsequent data on the
   line.
 * Convert x86_segment and X86EMUL_ constants to strings rather than printing
   raw numbers.
 * Move the printing to the end of the function, and either hexdump the result
   or print the failure code.

No change by default as verbose is off.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 tools/tests/x86_emulator/test_x86_emulator.c | 99 ++++++++++++++++++++++++----
 1 file changed, 87 insertions(+), 12 deletions(-)

diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c
index a764d99..6e24637 100644
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -16,6 +16,53 @@
 #include "xop.h"
 
 #define verbose false /* Switch to true for far more logging. */
+#define fn_width (int)(sizeof("cmpxchg") - 1)
+
+static const char *seg_to_str(enum x86_segment seg)
+{
+    switch ( seg )
+    {
+#define CASE(x) case x86_seg_ ## x: return # x
+        CASE(es);
+        CASE(cs);
+        CASE(ss);
+        CASE(ds);
+        CASE(fs);
+        CASE(gs);
+        CASE(tr);
+        CASE(ldtr);
+        CASE(gdtr);
+        CASE(idtr);
+        CASE(none);
+#undef CASE
+    default: return "??";
+    }
+}
+
+static const char *x86emul_to_str(int rc)
+{
+    switch ( rc )
+    {
+#define CASE(x) case X86EMUL_ ## x: return # x
+        CASE(OKAY);
+        CASE(UNHANDLEABLE);
+        CASE(EXCEPTION);
+        CASE(RETRY);
+        CASE(DONE);
+        CASE(UNIMPLEMENTED);
+#undef CASE
+    default: return "??";
+    }
+}
+
+static void hexdump_newline(const void *ptr, size_t size)
+{
+    const unsigned char *p = ptr;
+
+    for ( ; size; --size, ++p )
+        printf(" %02x", *p);
+    printf("\n");
+}
 
 static void blowfish_set_regs(struct cpu_user_regs *regs)
 {
@@ -230,9 +277,6 @@ static int read(
 
     emul_save_fpu_state();
 
-    if ( verbose )
-        printf("** %s(%u, %p,, %u,)\n", __func__, seg, (void *)offset, bytes);
-
     switch ( seg )
     {
         uint64_t value;
@@ -292,6 +336,17 @@ static int read(
     memcpy(p_data, (void *)offset, bytes);
 
  out:
+    if ( verbose )
+    {
+        printf("** %*s(%s, %p,, %u,) =>",
+               fn_width, __func__, seg_to_str(seg), (void *)offset, bytes);
+
+        if ( rc )
+            printf(" fail %s\n", x86emul_to_str(rc));
+        else
+            hexdump_newline(p_data, bytes);
+    }
+
     emul_restore_fpu_state();
 
     return rc;
@@ -306,11 +361,15 @@ static int fetch(
 {
     emul_save_fpu_state();
 
-    if ( verbose )
-        printf("** %s(%u, %p,, %u,)\n", __func__, seg, (void *)offset, bytes);
-
     memcpy(p_data, (void *)offset, bytes);
 
+    if ( verbose )
+    {
+        printf("** %*s(%s, %p,, %u,) =>",
+               fn_width, __func__, seg_to_str(seg), (void *)offset, bytes);
+        hexdump_newline(p_data, bytes);
+    }
+
     emul_restore_fpu_state();
 
     return X86EMUL_OKAY;
@@ -327,9 +386,6 @@ static int write(
 
     emul_save_fpu_state();
 
-    if ( verbose )
-        printf("** %s(%u, %p,, %u,)\n", __func__, seg, (void *)offset, bytes);
-
     if ( !is_x86_user_segment(seg) )
     {
         rc = X86EMUL_UNHANDLEABLE;
@@ -339,6 +395,17 @@ static int write(
     memcpy((void *)offset, p_data, bytes);
 
  out:
+    if ( verbose )
+    {
+        printf("** %*s(%s, %p,, %u,) =>",
+               fn_width, __func__, seg_to_str(seg), (void *)offset, bytes);
+
+        if ( rc )
+            printf(" fail %s\n", x86emul_to_str(rc));
+        else
+            hexdump_newline(p_data, bytes);
+    }
+
     emul_restore_fpu_state();
 
     return rc;
@@ -356,9 +423,6 @@ static int cmpxchg(
 
     emul_save_fpu_state();
 
-    if ( verbose )
-        printf("** %s(%u, %p,, %u,)\n", __func__, seg, (void *)offset, bytes);
-
     if ( !is_x86_user_segment(seg) )
     {
         rc = X86EMUL_UNHANDLEABLE;
@@ -368,6 +432,17 @@ static int cmpxchg(
     memcpy((void *)offset, new, bytes);
 
  out:
+    if ( verbose )
+    {
+        printf("** %*s(%s, %p,, %u,) =>",
+               fn_width, __func__, seg_to_str(seg), (void *)offset, bytes);
+
+        if ( rc )
+            printf(" fail %s\n", x86emul_to_str(rc));
+        else
+            hexdump_newline(new, bytes);
+    }
+
     emul_restore_fpu_state();
 
     return rc;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 4/3] tests/x86emul: Save and restore FPU state in the middle of emulation

[Andrew Cooper]

Using libc functions in the middle of emulation may corrupt FPU state.  Save
and restore FPU state around the progress marker which is the only current
libc function on the success path.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 tools/tests/x86_emulator/test_x86_emulator.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c
index 6e24637..894a44c 100644
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -3491,7 +3491,12 @@ int main(int argc, char **argv)
                 regs.eip < (unsigned long)res + blobs[j].size )
         {
             if ( (i++ & 8191) == 0 )
+            {
+                emul_save_fpu_state();
                 printf(".");
+                emul_restore_fpu_state();
+            }
+
             rc = x86_emulate(&ctxt, &emulops);
             if ( rc != X86EMUL_OKAY )
             {
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] tests/x86emul: Helpers to save and restore FPU state

[Jan Beulich]

>>> On 06.03.18 at 21:24, <andrew.cooper3@citrix.com> wrote:
> Introduce common helpers for saving and restoring FPU state.  During
> emul_test_init(), calculate whether to use xsave or fxsave, and tweak the
> existing mxcsr_mask logic to avoid using another large static buffer.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] tests/x86emul: Save and restore FPU state in the emulator callbacks

[Jan Beulich]

>>> On 06.03.18 at 21:24, <andrew.cooper3@citrix.com> wrote:
> Currently with then native toolchain on Debian Jessie ./test_x86_emulator
> yeilds:
> 
>   Testing AVX2 256bit single native execution...okay
>   Testing AVX2 256bit single 64-bit code sequence...[line 933] failed!
> 
> The bug is that libc's memcpy() in read() uses %xmm8 (specifically, in
> __memcpy_sse2_unaligned()), which corrupts %ymm8 behind the back of the AVX2
> test code.
> 
> Switch all hooks to use "goto out" style returns, and use
> emul_{save,restore}_fpu_state().

"Switch hooks to  use "goto out" style returns as necessary, and ..."?
You don't even touch all of them, and even one of those that you
touch doesn't obtain any "goto".

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

As an immediate workaround
Reviewed-by: Jan Beulich <jbeulich@suse.com>
(also for patch 4)

But of course this doesn't fully deal with the problem: Structure
assignments may still cause library functions to be invoked. Plus
there are explicit uses of memcpy() [which look safe] and
memset() [most or even all of which don't] in the core emulator.
I was therefore considering to instead provide hidden visibility
wrappers inside the binary, which would save/forward/restore.
That would also deal with someone wanting to add some printf()
in the middle of e.g. x86_emulate() for debugging purposes.

Obviously sooner or later we'll need the same for the fuzzer hooks;
that alternative approach would perhaps result in less code churn
there as well (the source to provide the wrappers could likely be
shared).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] tests/x86emul: Save and restore FPU state in the emulator callbacks

[Andrew Cooper]

On 09/03/18 11:41, Jan Beulich wrote:
>>>> On 06.03.18 at 21:24, <andrew.cooper3@citrix.com> wrote:
>> Currently with then native toolchain on Debian Jessie ./test_x86_emulator
>> yeilds:
>>
>>   Testing AVX2 256bit single native execution...okay
>>   Testing AVX2 256bit single 64-bit code sequence...[line 933] failed!
>>
>> The bug is that libc's memcpy() in read() uses %xmm8 (specifically, in
>> __memcpy_sse2_unaligned()), which corrupts %ymm8 behind the back of the AVX2
>> test code.
>>
>> Switch all hooks to use "goto out" style returns, and use
>> emul_{save,restore}_fpu_state().
> "Switch hooks to  use "goto out" style returns as necessary, and ..."?
> You don't even touch all of them, and even one of those that you
> touch doesn't obtain any "goto".
>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> As an immediate workaround
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> (also for patch 4)
>
> But of course this doesn't fully deal with the problem: Structure
> assignments may still cause library functions to be invoked. Plus
> there are explicit uses of memcpy() [which look safe] and
> memset() [most or even all of which don't] in the core emulator.
> I was therefore considering to instead provide hidden visibility
> wrappers inside the binary, which would save/forward/restore.
> That would also deal with someone wanting to add some printf()
> in the middle of e.g. x86_emulate() for debugging purposes.
>
> Obviously sooner or later we'll need the same for the fuzzer hooks;
> that alternative approach would perhaps result in less code churn
> there as well (the source to provide the wrappers could likely be
> shared).

I'm afraid that I don't understand what you mean here.  Are you
proposing that we wrap all libc functions, and ifso, how?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] tests/x86emul: Improve the utility of verbose mode

[Jan Beulich]

>>> On 06.03.18 at 21:24, <andrew.cooper3@citrix.com> wrote:
> --- a/tools/tests/x86_emulator/test_x86_emulator.c
> +++ b/tools/tests/x86_emulator/test_x86_emulator.c
> @@ -16,6 +16,53 @@
>  #include "xop.h"
>  
>  #define verbose false /* Switch to true for far more logging. */
> +#define fn_width (int)(sizeof("cmpxchg") - 1)

Strictly speaking this needs another pair of parentheses. But you
can avoid this by simply moving the cast into the existing ones.

> +static const char *seg_to_str(enum x86_segment seg)
> +{
> +    switch ( seg )
> +    {
> +#define CASE(x) case x86_seg_ ## x: return # x
> +        CASE(es);

Here and also in the other helper the indentation of the CASE()es
is one level too deep.

With these addressed
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] tests/x86emul: Save and restore FPU state in the emulator callbacks

[Jan Beulich]

>>> On 09.03.18 at 12:45, <andrew.cooper3@citrix.com> wrote:
> On 09/03/18 11:41, Jan Beulich wrote:
>>>>> On 06.03.18 at 21:24, <andrew.cooper3@citrix.com> wrote:
>>> Currently with then native toolchain on Debian Jessie ./test_x86_emulator
>>> yeilds:
>>>
>>>   Testing AVX2 256bit single native execution...okay
>>>   Testing AVX2 256bit single 64-bit code sequence...[line 933] failed!
>>>
>>> The bug is that libc's memcpy() in read() uses %xmm8 (specifically, in
>>> __memcpy_sse2_unaligned()), which corrupts %ymm8 behind the back of the AVX2
>>> test code.
>>>
>>> Switch all hooks to use "goto out" style returns, and use
>>> emul_{save,restore}_fpu_state().
>> "Switch hooks to  use "goto out" style returns as necessary, and ..."?
>> You don't even touch all of them, and even one of those that you
>> touch doesn't obtain any "goto".
>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> As an immediate workaround
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> (also for patch 4)
>>
>> But of course this doesn't fully deal with the problem: Structure
>> assignments may still cause library functions to be invoked. Plus
>> there are explicit uses of memcpy() [which look safe] and
>> memset() [most or even all of which don't] in the core emulator.
>> I was therefore considering to instead provide hidden visibility
>> wrappers inside the binary, which would save/forward/restore.
>> That would also deal with someone wanting to add some printf()
>> in the middle of e.g. x86_emulate() for debugging purposes.
>>
>> Obviously sooner or later we'll need the same for the fuzzer hooks;
>> that alternative approach would perhaps result in less code churn
>> there as well (the source to provide the wrappers could likely be
>> shared).
> 
> I'm afraid that I don't understand what you mean here.  Are you
> proposing that we wrap all libc functions, and ifso, how?

Yes - all the ones we use, or that the compiler may be reasonably
expected to produce accesses to them, and that we see any risk
they might touch {x,y,z}mm registers. As to how - let me see if I
can make this work.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] tests/x86emul: Helpers to save and restore FPU state

[Jan Beulich]

>>> On 06.03.18 at 21:24, <andrew.cooper3@citrix.com> wrote:
> +void emul_save_fpu_state(void)
> +{
> +    if ( use_xsave )
> +        asm volatile ( "xsave" __OS " %[ptr]"
> +                       : [ptr] "=m" (fpu_save_area)
> +                       : "a" (~0ull), "d" (~0ull) );

Wait, this doesn't build as 32-bit binary. Needs to be ~0ul, and
__OS also can't be used here.

> +    else
> +        asm volatile ( "fxsave %0" : "=m" (fpu_save_area) );

Whereas if you want something like __OS above, you'd want the
same here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] tests/x86emul: Helpers to save and restore FPU state

[Jan Beulich]

>>> On 09.03.18 at 13:37, <JBeulich@suse.com> wrote:
>>>> On 06.03.18 at 21:24, <andrew.cooper3@citrix.com> wrote:
>> +void emul_save_fpu_state(void)
>> +{
>> +    if ( use_xsave )
>> +        asm volatile ( "xsave" __OS " %[ptr]"
>> +                       : [ptr] "=m" (fpu_save_area)
>> +                       : "a" (~0ull), "d" (~0ull) );
> 
> Wait, this doesn't build as 32-bit binary. Needs to be ~0ul, and
> __OS also can't be used here.
> 
>> +    else
>> +        asm volatile ( "fxsave %0" : "=m" (fpu_save_area) );
> 
> Whereas if you want something like __OS above, you'd want the
> same here.

Here's the full incremental diff I've used for now, so that things
would build everywhere I've tried:

--- unstable.orig/tools/tests/x86_emulator/x86-emulate.c
+++ unstable/tools/tests/x86_emulator/x86-emulate.c
@@ -32,20 +32,22 @@ static bool use_xsave;
 void emul_save_fpu_state(void)
 {
     if ( use_xsave )
-        asm volatile ( "xsave" __OS " %[ptr]"
+        asm volatile ( "xsave %[ptr]"
                        : [ptr] "=m" (fpu_save_area)
-                       : "a" (~0ull), "d" (~0ull) );
+                       : "a" (~0ul), "d" (~0ul) );
     else
         asm volatile ( "fxsave %0" : "=m" (fpu_save_area) );
 }
 
 void emul_restore_fpu_state(void)
 {
+    /* Older gcc can't deal with "m" array inputs; make them outputs instead. */
     if ( use_xsave )
-        asm volatile ( "xrstor" __OS " %[ptr]"
-                       :: [ptr] "m" (fpu_save_area), "a" (~0ull), "d" (~0ull) );
+        asm volatile ( "xrstor %[ptr]"
+                       : [ptr] "+m" (fpu_save_area)
+                       : "a" (~0ul), "d" (~0ul) );
     else
-        asm volatile ( "fxrstor %0" :: "m" (fpu_save_area) );
+        asm volatile ( "fxrstor %0" : "+m" (fpu_save_area) );
 }
 
 bool emul_test_init(void)

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] tests/x86emul: Helpers to save and restore FPU state

[Andrew Cooper]

On 09/03/18 13:38, Jan Beulich wrote:
>>>> On 09.03.18 at 13:37, <JBeulich@suse.com> wrote:
>>>>> On 06.03.18 at 21:24, <andrew.cooper3@citrix.com> wrote:
>>> +void emul_save_fpu_state(void)
>>> +{
>>> +    if ( use_xsave )
>>> +        asm volatile ( "xsave" __OS " %[ptr]"
>>> +                       : [ptr] "=m" (fpu_save_area)
>>> +                       : "a" (~0ull), "d" (~0ull) );
>> Wait, this doesn't build as 32-bit binary. Needs to be ~0ul, and
>> __OS also can't be used here.
>>
>>> +    else
>>> +        asm volatile ( "fxsave %0" : "=m" (fpu_save_area) );
>> Whereas if you want something like __OS above, you'd want the
>> same here.
> Here's the full incremental diff I've used for now, so that things
> would build everywhere I've tried:
>
> --- unstable.orig/tools/tests/x86_emulator/x86-emulate.c
> +++ unstable/tools/tests/x86_emulator/x86-emulate.c
> @@ -32,20 +32,22 @@ static bool use_xsave;
>  void emul_save_fpu_state(void)
>  {
>      if ( use_xsave )
> -        asm volatile ( "xsave" __OS " %[ptr]"
> +        asm volatile ( "xsave %[ptr]"
>                         : [ptr] "=m" (fpu_save_area)
> -                       : "a" (~0ull), "d" (~0ull) );
> +                       : "a" (~0ul), "d" (~0ul) );
>      else
>          asm volatile ( "fxsave %0" : "=m" (fpu_save_area) );
>  }
>  
>  void emul_restore_fpu_state(void)
>  {
> +    /* Older gcc can't deal with "m" array inputs; make them outputs instead. */
>      if ( use_xsave )
> -        asm volatile ( "xrstor" __OS " %[ptr]"
> -                       :: [ptr] "m" (fpu_save_area), "a" (~0ull), "d" (~0ull) );
> +        asm volatile ( "xrstor %[ptr]"
> +                       : [ptr] "+m" (fpu_save_area)
> +                       : "a" (~0ul), "d" (~0ul) );
>      else
> -        asm volatile ( "fxrstor %0" :: "m" (fpu_save_area) );
> +        asm volatile ( "fxrstor %0" : "+m" (fpu_save_area) );
>  }
>  
>  bool emul_test_init(void)

Ok - I'll merge this in.

My worry with the __OS was to make sure that we matched how the kernel
would save and restore context, so the exception pointers don't get
lost.  However, that only matters at the point that we attempt to
memcmp(), and thinking about it, we'd need a better algorithm anyway.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel