diff mbox

[v2] target-i386: Use struct X86XSaveArea in fpu_helper.c

Message ID 1467837300-17528-1-git-send-email-rth@twiddle.net (mailing list archive)
State New, archived
Headers show

Commit Message

Richard Henderson July 6, 2016, 8:35 p.m. UTC 
This avoids a double hand-full of magic numbers in the
xsave and xrstor helper functions.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-i386/cpu.c        |   7 ++-
 target-i386/cpu.h        |  10 +----
 target-i386/fpu_helper.c | 108 ++++++++++++++++++++++++++---------------------
 3 files changed, 67 insertions(+), 58 deletions(-)
---

V2: Adjust struct X86XSaveHeader so as to retain the test for 16 bytes of
zeros within xrstor.  Comment on the slight inconsistency in two sections
of the current Intel documentation.

Comments

Richard Henderson Sept. 7, 2016, 4:47 p.m. UTC | #1 
On 07/06/2016 01:35 PM, Richard Henderson wrote:
> This avoids a double hand-full of magic numbers in the
> xsave and xrstor helper functions.
>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  target-i386/cpu.c        |   7 ++-
>  target-i386/cpu.h        |  10 +----
>  target-i386/fpu_helper.c | 108 ++++++++++++++++++++++++++---------------------
>  3 files changed, 67 insertions(+), 58 deletions(-)
> ---
>
> V2: Adjust struct X86XSaveHeader so as to retain the test for 16 bytes of
> zeros within xrstor.  Comment on the slight inconsistency in two sections
> of the current Intel documentation.

Ping.


r~
Eduardo Habkost Sept. 12, 2016, 11:30 p.m. UTC | #2 
On Wed, Sep 07, 2016 at 09:47:07AM -0700, Richard Henderson wrote:
> On 07/06/2016 01:35 PM, Richard Henderson wrote:
> > This avoids a double hand-full of magic numbers in the
> > xsave and xrstor helper functions.
> > 
> > Signed-off-by: Richard Henderson <rth@twiddle.net>
> > ---
> >  target-i386/cpu.c        |   7 ++-
> >  target-i386/cpu.h        |  10 +----
> >  target-i386/fpu_helper.c | 108 ++++++++++++++++++++++++++---------------------
> >  3 files changed, 67 insertions(+), 58 deletions(-)
> > ---
> > 
> > V2: Adjust struct X86XSaveHeader so as to retain the test for 16 bytes of
> > zeros within xrstor.  Comment on the slight inconsistency in two sections
> > of the current Intel documentation.
> 
> Ping.

Applied to x86-next, thanks!
diff mbox

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3bd3cfc..7726310 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -472,7 +472,12 @@  static const X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = {
 };
 #undef REGISTER
 
-const ExtSaveArea x86_ext_save_areas[] = {
+typedef struct ExtSaveArea {
+    uint32_t feature, bits;
+    uint32_t offset, size;
+} ExtSaveArea;
+
+static const ExtSaveArea x86_ext_save_areas[] = {
     [XSTATE_YMM_BIT] =
           { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX,
             .offset = offsetof(X86XSaveArea, avx_state),
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 474b0b9..c012811 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -855,7 +855,8 @@  typedef union X86LegacyXSaveArea {
 typedef struct X86XSaveHeader {
     uint64_t xstate_bv;
     uint64_t xcomp_bv;
-    uint8_t reserved[48];
+    uint64_t reserve0;
+    uint8_t reserved[40];
 } X86XSaveHeader;
 
 /* Ext. save area 2: AVX State */
@@ -1345,13 +1346,6 @@  int cpu_x86_signal_handler(int host_signum, void *pinfo,
                            void *puc);
 
 /* cpu.c */
-typedef struct ExtSaveArea {
-    uint32_t feature, bits;
-    uint32_t offset, size;
-} ExtSaveArea;
-
-extern const ExtSaveArea x86_ext_save_areas[];
-
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx);
diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c
index 929489b..2049a8c 100644
--- a/target-i386/fpu_helper.c
+++ b/target-i386/fpu_helper.c
@@ -1110,6 +1110,8 @@  void cpu_x86_frstor(CPUX86State *env, target_ulong ptr, int data32)
 }
 #endif
 
+#define XO(X)  offsetof(X86XSaveArea, X)
+
 static void do_xsave_fpu(CPUX86State *env, target_ulong ptr, uintptr_t ra)
 {
     int fpus, fptag, i;
@@ -1120,17 +1122,18 @@  static void do_xsave_fpu(CPUX86State *env, target_ulong ptr, uintptr_t ra)
     for (i = 0; i < 8; i++) {
         fptag |= (env->fptags[i] << i);
     }
-    cpu_stw_data_ra(env, ptr, env->fpuc, ra);
-    cpu_stw_data_ra(env, ptr + 2, fpus, ra);
-    cpu_stw_data_ra(env, ptr + 4, fptag ^ 0xff, ra);
+
+    cpu_stw_data_ra(env, ptr + XO(legacy.fcw), env->fpuc, ra);
+    cpu_stw_data_ra(env, ptr + XO(legacy.fsw), fpus, ra);
+    cpu_stw_data_ra(env, ptr + XO(legacy.ftw), fptag ^ 0xff, ra);
 
     /* In 32-bit mode this is eip, sel, dp, sel.
        In 64-bit mode this is rip, rdp.
        But in either case we don't write actual data, just zeros.  */
-    cpu_stq_data_ra(env, ptr + 0x08, 0, ra); /* eip+sel; rip */
-    cpu_stq_data_ra(env, ptr + 0x10, 0, ra); /* edp+sel; rdp */
+    cpu_stq_data_ra(env, ptr + XO(legacy.fpip), 0, ra); /* eip+sel; rip */
+    cpu_stq_data_ra(env, ptr + XO(legacy.fpdp), 0, ra); /* edp+sel; rdp */
 
-    addr = ptr + 0x20;
+    addr = ptr + XO(legacy.fpregs);
     for (i = 0; i < 8; i++) {
         floatx80 tmp = ST(i);
         helper_fstt(env, tmp, addr, ra);
@@ -1140,8 +1143,8 @@  static void do_xsave_fpu(CPUX86State *env, target_ulong ptr, uintptr_t ra)
 
 static void do_xsave_mxcsr(CPUX86State *env, target_ulong ptr, uintptr_t ra)
 {
-    cpu_stl_data_ra(env, ptr + 0x18, env->mxcsr, ra); /* mxcsr */
-    cpu_stl_data_ra(env, ptr + 0x1c, 0x0000ffff, ra); /* mxcsr_mask */
+    cpu_stl_data_ra(env, ptr + XO(legacy.mxcsr), env->mxcsr, ra);
+    cpu_stl_data_ra(env, ptr + XO(legacy.mxcsr_mask), 0x0000ffff, ra);
 }
 
 static void do_xsave_sse(CPUX86State *env, target_ulong ptr, uintptr_t ra)
@@ -1155,7 +1158,7 @@  static void do_xsave_sse(CPUX86State *env, target_ulong ptr, uintptr_t ra)
         nb_xmm_regs = 8;
     }
 
-    addr = ptr + 0xa0;
+    addr = ptr + XO(legacy.xmm_regs);
     for (i = 0; i < nb_xmm_regs; i++) {
         cpu_stq_data_ra(env, addr, env->xmm_regs[i].ZMM_Q(0), ra);
         cpu_stq_data_ra(env, addr + 8, env->xmm_regs[i].ZMM_Q(1), ra);
@@ -1163,8 +1166,9 @@  static void do_xsave_sse(CPUX86State *env, target_ulong ptr, uintptr_t ra)
     }
 }
 
-static void do_xsave_bndregs(CPUX86State *env, target_ulong addr, uintptr_t ra)
+static void do_xsave_bndregs(CPUX86State *env, target_ulong ptr, uintptr_t ra)
 {
+    target_ulong addr = ptr + offsetof(XSaveBNDREG, bnd_regs);
     int i;
 
     for (i = 0; i < 4; i++, addr += 16) {
@@ -1173,15 +1177,17 @@  static void do_xsave_bndregs(CPUX86State *env, target_ulong addr, uintptr_t ra)
     }
 }
 
-static void do_xsave_bndcsr(CPUX86State *env, target_ulong addr, uintptr_t ra)
+static void do_xsave_bndcsr(CPUX86State *env, target_ulong ptr, uintptr_t ra)
 {
-    cpu_stq_data_ra(env, addr, env->bndcs_regs.cfgu, ra);
-    cpu_stq_data_ra(env, addr + 8, env->bndcs_regs.sts, ra);
+    cpu_stq_data_ra(env, ptr + offsetof(XSaveBNDCSR, bndcsr.cfgu),
+                    env->bndcs_regs.cfgu, ra);
+    cpu_stq_data_ra(env, ptr + offsetof(XSaveBNDCSR, bndcsr.sts),
+                    env->bndcs_regs.sts, ra);
 }
 
-static void do_xsave_pkru(CPUX86State *env, target_ulong addr, uintptr_t ra)
+static void do_xsave_pkru(CPUX86State *env, target_ulong ptr, uintptr_t ra)
 {
-    cpu_stq_data_ra(env, addr, env->pkru, ra);
+    cpu_stq_data_ra(env, ptr, env->pkru, ra);
 }
 
 void helper_fxsave(CPUX86State *env, target_ulong ptr)
@@ -1250,22 +1256,19 @@  static void do_xsave(CPUX86State *env, target_ulong ptr, uint64_t rfbm,
         do_xsave_sse(env, ptr, ra);
     }
     if (opt & XSTATE_BNDREGS_MASK) {
-        target_ulong off = x86_ext_save_areas[XSTATE_BNDREGS_BIT].offset;
-        do_xsave_bndregs(env, ptr + off, ra);
+        do_xsave_bndregs(env, ptr + XO(bndreg_state), ra);
     }
     if (opt & XSTATE_BNDCSR_MASK) {
-        target_ulong off = x86_ext_save_areas[XSTATE_BNDCSR_BIT].offset;
-        do_xsave_bndcsr(env, ptr + off, ra);
+        do_xsave_bndcsr(env, ptr + XO(bndcsr_state), ra);
     }
     if (opt & XSTATE_PKRU_MASK) {
-        target_ulong off = x86_ext_save_areas[XSTATE_PKRU_BIT].offset;
-        do_xsave_pkru(env, ptr + off, ra);
+        do_xsave_pkru(env, ptr + XO(pkru_state), ra);
     }
 
     /* Update the XSTATE_BV field.  */
-    old_bv = cpu_ldq_data_ra(env, ptr + 512, ra);
+    old_bv = cpu_ldq_data_ra(env, ptr + XO(header.xstate_bv), ra);
     new_bv = (old_bv & ~rfbm) | (inuse & rfbm);
-    cpu_stq_data_ra(env, ptr + 512, new_bv, ra);
+    cpu_stq_data_ra(env, ptr + XO(header.xstate_bv), new_bv, ra);
 }
 
 void helper_xsave(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
@@ -1281,12 +1284,13 @@  void helper_xsaveopt(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
 
 static void do_xrstor_fpu(CPUX86State *env, target_ulong ptr, uintptr_t ra)
 {
-    int i, fpus, fptag;
+    int i, fpuc, fpus, fptag;
     target_ulong addr;
 
-    cpu_set_fpuc(env, cpu_lduw_data_ra(env, ptr, ra));
-    fpus = cpu_lduw_data_ra(env, ptr + 2, ra);
-    fptag = cpu_lduw_data_ra(env, ptr + 4, ra);
+    fpuc = cpu_lduw_data_ra(env, ptr + XO(legacy.fcw), ra);
+    fpus = cpu_lduw_data_ra(env, ptr + XO(legacy.fsw), ra);
+    fptag = cpu_lduw_data_ra(env, ptr + XO(legacy.ftw), ra);
+    cpu_set_fpuc(env, fpuc);
     env->fpstt = (fpus >> 11) & 7;
     env->fpus = fpus & ~0x3800;
     fptag ^= 0xff;
@@ -1294,7 +1298,7 @@  static void do_xrstor_fpu(CPUX86State *env, target_ulong ptr, uintptr_t ra)
         env->fptags[i] = ((fptag >> i) & 1);
     }
 
-    addr = ptr + 0x20;
+    addr = ptr + XO(legacy.fpregs);
     for (i = 0; i < 8; i++) {
         floatx80 tmp = helper_fldt(env, addr, ra);
         ST(i) = tmp;
@@ -1304,7 +1308,7 @@  static void do_xrstor_fpu(CPUX86State *env, target_ulong ptr, uintptr_t ra)
 
 static void do_xrstor_mxcsr(CPUX86State *env, target_ulong ptr, uintptr_t ra)
 {
-    cpu_set_mxcsr(env, cpu_ldl_data_ra(env, ptr + 0x18, ra));
+    cpu_set_mxcsr(env, cpu_ldl_data_ra(env, ptr + XO(legacy.mxcsr), ra));
 }
 
 static void do_xrstor_sse(CPUX86State *env, target_ulong ptr, uintptr_t ra)
@@ -1318,7 +1322,7 @@  static void do_xrstor_sse(CPUX86State *env, target_ulong ptr, uintptr_t ra)
         nb_xmm_regs = 8;
     }
 
-    addr = ptr + 0xa0;
+    addr = ptr + XO(legacy.xmm_regs);
     for (i = 0; i < nb_xmm_regs; i++) {
         env->xmm_regs[i].ZMM_Q(0) = cpu_ldq_data_ra(env, addr, ra);
         env->xmm_regs[i].ZMM_Q(1) = cpu_ldq_data_ra(env, addr + 8, ra);
@@ -1326,8 +1330,9 @@  static void do_xrstor_sse(CPUX86State *env, target_ulong ptr, uintptr_t ra)
     }
 }
 
-static void do_xrstor_bndregs(CPUX86State *env, target_ulong addr, uintptr_t ra)
+static void do_xrstor_bndregs(CPUX86State *env, target_ulong ptr, uintptr_t ra)
 {
+    target_ulong addr = ptr + offsetof(XSaveBNDREG, bnd_regs);
     int i;
 
     for (i = 0; i < 4; i++, addr += 16) {
@@ -1336,16 +1341,18 @@  static void do_xrstor_bndregs(CPUX86State *env, target_ulong addr, uintptr_t ra)
     }
 }
 
-static void do_xrstor_bndcsr(CPUX86State *env, target_ulong addr, uintptr_t ra)
+static void do_xrstor_bndcsr(CPUX86State *env, target_ulong ptr, uintptr_t ra)
 {
     /* FIXME: Extend highest implemented bit of linear address.  */
-    env->bndcs_regs.cfgu = cpu_ldq_data_ra(env, addr, ra);
-    env->bndcs_regs.sts = cpu_ldq_data_ra(env, addr + 8, ra);
+    env->bndcs_regs.cfgu
+        = cpu_ldq_data_ra(env, ptr + offsetof(XSaveBNDCSR, bndcsr.cfgu), ra);
+    env->bndcs_regs.sts
+        = cpu_ldq_data_ra(env, ptr + offsetof(XSaveBNDCSR, bndcsr.sts), ra);
 }
 
-static void do_xrstor_pkru(CPUX86State *env, target_ulong addr, uintptr_t ra)
+static void do_xrstor_pkru(CPUX86State *env, target_ulong ptr, uintptr_t ra)
 {
-    env->pkru = cpu_ldq_data_ra(env, addr, ra);
+    env->pkru = cpu_ldq_data_ra(env, ptr, ra);
 }
 
 void helper_fxrstor(CPUX86State *env, target_ulong ptr)
@@ -1373,7 +1380,7 @@  void helper_fxrstor(CPUX86State *env, target_ulong ptr)
 void helper_xrstor(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
 {
     uintptr_t ra = GETPC();
-    uint64_t xstate_bv, xcomp_bv0, xcomp_bv1;
+    uint64_t xstate_bv, xcomp_bv, reserve0;
 
     rfbm &= env->xcr0;
 
@@ -1387,7 +1394,7 @@  void helper_xrstor(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
         raise_exception_ra(env, EXCP0D_GPF, ra);
     }
 
-    xstate_bv = cpu_ldq_data_ra(env, ptr + 512, ra);
+    xstate_bv = cpu_ldq_data_ra(env, ptr + XO(header.xstate_bv), ra);
 
     if ((int64_t)xstate_bv < 0) {
         /* FIXME: Compact form.  */
@@ -1396,15 +1403,19 @@  void helper_xrstor(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
 
     /* Standard form.  */
 
-    /* The XSTATE field must not set bits not present in XCR0.  */
+    /* The XSTATE_BV field must not set bits not present in XCR0.  */
     if (xstate_bv & ~env->xcr0) {
         raise_exception_ra(env, EXCP0D_GPF, ra);
     }
 
-    /* The XCOMP field must be zero.  */
-    xcomp_bv0 = cpu_ldq_data_ra(env, ptr + 520, ra);
-    xcomp_bv1 = cpu_ldq_data_ra(env, ptr + 528, ra);
-    if (xcomp_bv0 || xcomp_bv1) {
+    /* The XCOMP_BV field must be zero.  Note that, as of the April 2016
+       revision, the description of the XSAVE Header (Vol 1, Sec 13.4.2)
+       describes only XCOMP_BV, but the description of the standard form
+       of XRSTOR (Vol 1, Sec 13.8.1) checks bytes 23:8 for zero, which
+       includes the next 64-bit field.  */
+    xcomp_bv = cpu_ldq_data_ra(env, ptr + XO(header.xcomp_bv), ra);
+    reserve0 = cpu_ldq_data_ra(env, ptr + XO(header.reserve0), ra);
+    if (xcomp_bv || reserve0) {
         raise_exception_ra(env, EXCP0D_GPF, ra);
     }
 
@@ -1430,8 +1441,7 @@  void helper_xrstor(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
     }
     if (rfbm & XSTATE_BNDREGS_MASK) {
         if (xstate_bv & XSTATE_BNDREGS_MASK) {
-            target_ulong off = x86_ext_save_areas[XSTATE_BNDREGS_BIT].offset;
-            do_xrstor_bndregs(env, ptr + off, ra);
+            do_xrstor_bndregs(env, ptr + XO(bndreg_state), ra);
             env->hflags |= HF_MPX_IU_MASK;
         } else {
             memset(env->bnd_regs, 0, sizeof(env->bnd_regs));
@@ -1440,8 +1450,7 @@  void helper_xrstor(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
     }
     if (rfbm & XSTATE_BNDCSR_MASK) {
         if (xstate_bv & XSTATE_BNDCSR_MASK) {
-            target_ulong off = x86_ext_save_areas[XSTATE_BNDCSR_BIT].offset;
-            do_xrstor_bndcsr(env, ptr + off, ra);
+            do_xrstor_bndcsr(env, ptr + XO(bndcsr_state), ra);
         } else {
             memset(&env->bndcs_regs, 0, sizeof(env->bndcs_regs));
         }
@@ -1450,8 +1459,7 @@  void helper_xrstor(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
     if (rfbm & XSTATE_PKRU_MASK) {
         uint64_t old_pkru = env->pkru;
         if (xstate_bv & XSTATE_PKRU_MASK) {
-            target_ulong off = x86_ext_save_areas[XSTATE_PKRU_BIT].offset;
-            do_xrstor_pkru(env, ptr + off, ra);
+            do_xrstor_pkru(env, ptr + XO(pkru_state), ra);
         } else {
             env->pkru = 0;
         }
@@ -1462,6 +1470,8 @@  void helper_xrstor(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
     }
 }
 
+#undef XO
+
 uint64_t helper_xgetbv(CPUX86State *env, uint32_t ecx)
 {
     /* The OS must have enabled XSAVE.  */