[Qemu-devel] [for-2.6 PATCH 0/3] target-i386: Use C struct for xsave area layout, offsets & sizes

[Qemu-devel] [for-2.6 PATCH 0/3] target-i386: Use C struct for xsave area layout, offsets & sizes

[Eduardo Habkost]

target-i386/cpu.c:ext_save_area uses magic numbers for the xsave
area offets and sizes, and target-i386/kvm.c:kvm_{put,get}_xsave()
uses offset macros and bit manipulation to access the xsave area.
This series changes both to use C structs for the same
operations.

I still need to figure out a way to write unit tests for the new
code. Maybe I will just copy and paste the new and old functions,
and test them locally (checking if they give the same results
when translating blobs of random bytes).

Eduardo Habkost (3):
  target-i386: Define structs for layout of xsave area
  target-i386: Use xsave structs for ext_save_area
  target-i386: kvm: Use X86XSaveArea struct for xsave save/load

 target-i386/cpu.c |  18 +++++---
 target-i386/cpu.h |  85 ++++++++++++++++++++++++++++++++++++++
 target-i386/kvm.c | 121 +++++++++++++++++++++++-------------------------------
 3 files changed, 149 insertions(+), 75 deletions(-)

-- 
2.1.0

[Qemu-devel] [for-2.6 PATCH 1/3] target-i386: Define structs for layout of xsave area

[Eduardo Habkost]

Add structs that define the layout of the xsave areas used by
Intel processors. Add some QEMU_BUILD_BUG_ON lines to ensure the
structs match the XSAVE_* macros in target-i386/kvm.c and the
offsets and sizes at target-i386/cpu.c:ext_save_areas.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.h | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 target-i386/kvm.c | 23 +++++++++++++++
 2 files changed, 108 insertions(+)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 84edfd0..3d1d01e 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -806,6 +806,91 @@ typedef struct {
 
 #define NB_OPMASK_REGS 8
 
+typedef union X86LegacyXSaveArea {
+    struct {
+        uint16_t fcw;
+        uint16_t fsw;
+        uint8_t ftw;
+        uint8_t reserved;
+        uint16_t fpop;
+        uint64_t fpip;
+        uint64_t fpdp;
+        uint32_t mxcsr;
+        uint32_t mxcsr_mask;
+        FPReg fpregs[8];
+        uint64_t xmm_regs[16][2];
+    };
+    uint8_t data[512];
+} X86LegacyXSaveArea;
+
+typedef struct X86XSaveHeader {
+    uint64_t xstate_bv;
+    uint64_t xcomp_bv;
+    uint8_t reserved[48];
+} X86XSaveHeader;
+
+/* Ext. save area 2: AVX State */
+typedef struct XSaveAVX {
+    uint64_t ymmh[16][2];
+} XSaveAVX;
+
+/* Ext. save area 3: BNDREG */
+typedef struct XSaveBNDREG {
+    BNDReg bnd_regs[4];
+} XSaveBNDREG;
+
+/* Ext. save area 4: BNDCSR */
+typedef union XSaveBNDCSR {
+    BNDCSReg bndcsr;
+    uint8_t data[64];
+} XSaveBNDCSR;
+
+/* Ext. save area 5: Opmask */
+typedef struct XSaveOpmask {
+    uint64_t opmask_regs[NB_OPMASK_REGS];
+} XSaveOpmask;
+
+/* Ext. save area 6: ZMM_Hi256 */
+typedef struct XSaveZMM_Hi256 {
+    uint64_t zmm_hi256[16][4];
+} XSaveZMM_Hi256;
+
+/* Ext. save area 7: Hi16_ZMM */
+typedef struct XSaveHi16_ZMM {
+    XMMReg hi16_zmm[16];
+} XSaveHi16_ZMM;
+
+typedef struct X86XSaveArea {
+    X86LegacyXSaveArea legacy;
+    X86XSaveHeader header;
+
+    /* Extended save areas: */
+
+    /* AVX State: */
+    XSaveAVX avx_state;
+    uint8_t padding[960-576-sizeof(XSaveAVX)];
+    /* MPX State: */
+    XSaveBNDREG bndreg_state;
+    XSaveBNDCSR bndcsr_state;
+    /* AVX-512 State: */
+    XSaveOpmask opmask_state;
+    XSaveZMM_Hi256 zmm_hi256_state;
+    XSaveHi16_ZMM hi16_zmm_state;
+} X86XSaveArea;
+
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, avx_state) != 0x240);
+QEMU_BUILD_BUG_ON(sizeof(XSaveAVX) != 0x100);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, bndreg_state) != 0x3c0);
+QEMU_BUILD_BUG_ON(sizeof(XSaveBNDREG) != 0x40);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, bndcsr_state) != 0x400);
+QEMU_BUILD_BUG_ON(sizeof(XSaveBNDCSR) != 0x40);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, opmask_state) != 0x440);
+QEMU_BUILD_BUG_ON(sizeof(XSaveOpmask) != 0x40);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, zmm_hi256_state) != 0x480);
+QEMU_BUILD_BUG_ON(sizeof(XSaveZMM_Hi256) != 0x200);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, hi16_zmm_state) != 0x680);
+QEMU_BUILD_BUG_ON(sizeof(XSaveHi16_ZMM) != 0x400);
+
 typedef enum TPRAccess {
     TPR_ACCESS_READ,
     TPR_ACCESS_WRITE,
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 6dc9846..ee6c213 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1218,6 +1218,29 @@ static int kvm_put_fpu(X86CPU *cpu)
 #define XSAVE_ZMM_Hi256   288
 #define XSAVE_Hi16_ZMM    416
 
+#define XSAVE_BYTE_OFFSET(word_offset) \
+    ((word_offset)*sizeof(((struct kvm_xsave*)0)->region[0]))
+
+#define ASSERT_OFFSET(word_offset, field) \
+    QEMU_BUILD_BUG_ON(XSAVE_BYTE_OFFSET(word_offset) != \
+                      offsetof(X86XSaveArea, field))
+
+ASSERT_OFFSET(XSAVE_FCW_FSW, legacy.fcw);
+ASSERT_OFFSET(XSAVE_FTW_FOP, legacy.ftw);
+ASSERT_OFFSET(XSAVE_CWD_RIP, legacy.fpip);
+ASSERT_OFFSET(XSAVE_CWD_RDP, legacy.fpdp);
+ASSERT_OFFSET(XSAVE_MXCSR, legacy.mxcsr);
+ASSERT_OFFSET(XSAVE_ST_SPACE, legacy.fpregs);
+ASSERT_OFFSET(XSAVE_XMM_SPACE, legacy.xmm_regs);
+ASSERT_OFFSET(XSAVE_XSTATE_BV, header.xstate_bv);
+ASSERT_OFFSET(XSAVE_YMMH_SPACE, avx_state);
+ASSERT_OFFSET(XSAVE_BNDREGS, bndreg_state);
+ASSERT_OFFSET(XSAVE_BNDCSR, bndcsr_state);
+ASSERT_OFFSET(XSAVE_OPMASK, opmask_state);
+ASSERT_OFFSET(XSAVE_ZMM_Hi256, zmm_hi256_state);
+ASSERT_OFFSET(XSAVE_Hi16_ZMM, hi16_zmm_state);
+
+
 static int kvm_put_xsave(X86CPU *cpu)
 {
     CPUX86State *env = &cpu->env;
-- 
2.1.0

Re: [Qemu-devel] [for-2.6 PATCH 1/3] target-i386: Define structs for layout of xsave area

[Paolo Bonzini]

On 28/11/2015 20:56, Eduardo Habkost wrote:
> +/* Ext. save area 2: AVX State */
> +typedef struct XSaveAVX {
> +    uint64_t ymmh[16][2];
> +} XSaveAVX;
> +

Because this is always little endian, I would write it as uint8_t[16][16].

> +/* Ext. save area 6: ZMM_Hi256 */
> +typedef struct XSaveZMM_Hi256 {
> +    uint64_t zmm_hi256[16][4];
> +} XSaveZMM_Hi256;

Same here (uint8_t[16][32]).

> +/* Ext. save area 7: Hi16_ZMM */
> +typedef struct XSaveHi16_ZMM {
> +    XMMReg hi16_zmm[16];
> +} XSaveHi16_ZMM;

This is uint8_t[16][64] or uint64_t[16][8].

Paolo

Re: [Qemu-devel] [for-2.6 PATCH 1/3] target-i386: Define structs for layout of xsave area

[Eduardo Habkost]

On Mon, Nov 30, 2015 at 12:18:33PM +0100, Paolo Bonzini wrote:
> On 28/11/2015 20:56, Eduardo Habkost wrote:
> > +/* Ext. save area 2: AVX State */
> > +typedef struct XSaveAVX {
> > +    uint64_t ymmh[16][2];
> > +} XSaveAVX;
> > +
> 
> Because this is always little endian, I would write it as uint8_t[16][16].

The main reason I used uint64_t was to allow the put/save code to
simply use field[0], field[1], field[2] instead of field,
field+8, field+16. But I think your suggestion makes sense: the
fact that we load/save the register data in 8-byte chunks in
kvm_{get,put}_xsave() has nothing to do with the layout of the
data itself (so it doesn't need to be encoded in the struct
definition).

> 
> > +/* Ext. save area 6: ZMM_Hi256 */
> > +typedef struct XSaveZMM_Hi256 {
> > +    uint64_t zmm_hi256[16][4];
> > +} XSaveZMM_Hi256;
> 
> Same here (uint8_t[16][32]).
> 
> > +/* Ext. save area 7: Hi16_ZMM */
> > +typedef struct XSaveHi16_ZMM {
> > +    XMMReg hi16_zmm[16];
> > +} XSaveHi16_ZMM;
> 
> This is uint8_t[16][64] or uint64_t[16][8].

While writing this series, I had a version that redefined the
XMMReg, YMMReg, ZMMReg structs with the correct sizes, and reused
them for ymmh, zmm_hi256, and hi16_zmm. I still planned to
propose that later. You don't think it would make sense?

-- 
Eduardo

Re: [Qemu-devel] [for-2.6 PATCH 1/3] target-i386: Define structs for layout of xsave area

[Richard Henderson]

On 11/30/2015 03:18 AM, Paolo Bonzini wrote:
> Because this is always little endian, I would write it as uint8_t[16][16].

Maybe.  That isn't altogether handy for TCG, since we'll be wanting to bswap 
these buffers (probably in uint64_t chunks).


r~

Re: [Qemu-devel] [for-2.6 PATCH 1/3] target-i386: Define structs for layout of xsave area

[Eduardo Habkost]

On Tue, Dec 01, 2015 at 09:09:47AM -0800, Richard Henderson wrote:
> On 11/30/2015 03:18 AM, Paolo Bonzini wrote:
> >Because this is always little endian, I would write it as uint8_t[16][16].
> 
> Maybe.  That isn't altogether handy for TCG, since we'll be wanting to bswap
> these buffers (probably in uint64_t chunks).

X86XSaveArea will be used only when loading/saving state using
xsave, not for executing regular instructions. In X86CPU, the
data is already stored as XMMReg unions (the one with the
XMM_[BWDQ] helpers).

-- 
Eduardo

Re: [Qemu-devel] [for-2.6 PATCH 1/3] target-i386: Define structs for layout of xsave area

[Richard Henderson]

On 12/01/2015 09:15 AM, Eduardo Habkost wrote:
> On Tue, Dec 01, 2015 at 09:09:47AM -0800, Richard Henderson wrote:
>> On 11/30/2015 03:18 AM, Paolo Bonzini wrote:
>>> Because this is always little endian, I would write it as uint8_t[16][16].
>>
>> Maybe.  That isn't altogether handy for TCG, since we'll be wanting to bswap
>> these buffers (probably in uint64_t chunks).
>
> X86XSaveArea will be used only when loading/saving state using
> xsave, not for executing regular instructions.

... like the regular instruction xsave?

https://patchwork.ozlabs.org/patch/493318/

> In X86CPU, the
> data is already stored as XMMReg unions (the one with the
> XMM_[BWDQ] helpers).

Of course.  But those unions are arranged to be in big-endian format on 
big-endian hosts.  So we need to swap the data back to little-endian format for 
storage into guest memory.


r~

Re: [Qemu-devel] [for-2.6 PATCH 1/3] target-i386: Define structs for layout of xsave area

[Paolo Bonzini]

On 01/12/2015 18:20, Richard Henderson wrote:
>>
>> X86XSaveArea will be used only when loading/saving state using
>> xsave, not for executing regular instructions.
> 
> ... like the regular instruction xsave?
> 
> https://patchwork.ozlabs.org/patch/493318/

Right, but that's a helper anyway.

>> In X86CPU, the
>> data is already stored as XMMReg unions (the one with the
>> XMM_[BWDQ] helpers).
> 
> Of course.  But those unions are arranged to be in big-endian format on
> big-endian hosts.  So we need to swap the data back to little-endian
> format for storage into guest memory.

Yes, you can use byte moves with XMM_B (more obvious), or stq_le_p with
XMM_Q (faster I guess---though the compiler might optimize the former on
little-endian hosts).  Either works with an uint8_t[] destination.

Paolo

Re: [Qemu-devel] [for-2.6 PATCH 1/3] target-i386: Define structs for layout of xsave area

[Eduardo Habkost]

On Tue, Dec 01, 2015 at 06:27:17PM +0100, Paolo Bonzini wrote:
> On 01/12/2015 18:20, Richard Henderson wrote:
> >>
> >> X86XSaveArea will be used only when loading/saving state using
> >> xsave, not for executing regular instructions.
> > 
> > ... like the regular instruction xsave?
> > 
> > https://patchwork.ozlabs.org/patch/493318/
> 
> Right, but that's a helper anyway.
> 
> >> In X86CPU, the
> >> data is already stored as XMMReg unions (the one with the
> >> XMM_[BWDQ] helpers).
> > 
> > Of course.  But those unions are arranged to be in big-endian format on
> > big-endian hosts.  So we need to swap the data back to little-endian
> > format for storage into guest memory.
> 
> Yes, you can use byte moves with XMM_B (more obvious), or stq_le_p with
> XMM_Q (faster I guess---though the compiler might optimize the former on
> little-endian hosts).  Either works with an uint8_t[] destination.

stq_le_p() (more exactly, stq_p()) is exactly what is already
done by kvm_{get,put}_xsave(), using uint8_t pointers.

BTW, if we are going to implement xsave in TCG, the
X86CPU<->xsave translation logic in kvm_{get,put}_xsave() could
be moved to generic code and reused by TCG instead of being
reimplemented.

-- 
Eduardo

Re: [Qemu-devel] [for-2.6 PATCH 1/3] target-i386: Define structs for layout of xsave area

[Richard Henderson]

On 12/01/2015 10:34 AM, Eduardo Habkost wrote:
> BTW, if we are going to implement xsave in TCG, the
> X86CPU<->xsave translation logic in kvm_{get,put}_xsave() could
> be moved to generic code and reused by TCG instead of being
> reimplemented.

That's not trivial.

In particular, stq_p isn't what the tcg helper needs to use, but rather 
cpu_stq_data_ra.  Given the differing parameters, we'd have to resort to some 
sort of macro-ization.  It's probably easiest to simply keep the two 
implementations separate.


r~

[Qemu-devel] [for-2.6 PATCH 2/3] target-i386: Use xsave structs for ext_save_area

[Eduardo Habkost]

This doesn't introduce any change in the code, as the offsets and
struct sizes match what was present in the table. This can be
validated by the QEMU_BUILD_BUG_ON lines on target-i386/cpu.h,
which ensures the struct sizes and offsets match the existing
values in ext_save_area.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 11e5e39..bc95437 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -458,17 +458,23 @@ typedef struct ExtSaveArea {
 
 static const ExtSaveArea ext_save_areas[] = {
     [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX,
-            .offset = 0x240, .size = 0x100 },
+            .offset = offsetof(X86XSaveArea, avx_state),
+            .size = sizeof(XSaveAVX) },
     [3] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX,
-            .offset = 0x3c0, .size = 0x40  },
+            .offset = offsetof(X86XSaveArea, bndreg_state),
+            .size = sizeof(XSaveBNDREG)  },
     [4] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX,
-            .offset = 0x400, .size = 0x40  },
+            .offset = offsetof(X86XSaveArea, bndcsr_state),
+            .size = sizeof(XSaveBNDCSR)  },
     [5] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_AVX512F,
-            .offset = 0x440, .size = 0x40 },
+            .offset = offsetof(X86XSaveArea, opmask_state),
+            .size = sizeof(XSaveOpmask) },
     [6] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_AVX512F,
-            .offset = 0x480, .size = 0x200 },
+            .offset = offsetof(X86XSaveArea, zmm_hi256_state),
+            .size = sizeof(XSaveZMM_Hi256) },
     [7] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_AVX512F,
-            .offset = 0x680, .size = 0x400 },
+            .offset = offsetof(X86XSaveArea, hi16_zmm_state),
+            .size = sizeof(XSaveHi16_ZMM) },
 };
 
 const char *get_register_name_32(unsigned int reg)
-- 
2.1.0

[Qemu-devel] [for-2.6 PATCH 3/3] target-i386: kvm: Use X86XSaveArea struct for xsave save/load

[Eduardo Habkost]

Instead of using offset macros and bit operations in a uint32_t
array, use the X86XSaveArea struct to perform the loading/saving
operations in kvm_put_xsave() and kvm_get_xsave().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 144 ++++++++++++++++++++----------------------------------
 1 file changed, 52 insertions(+), 92 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index ee6c213..5e7ec70 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1203,50 +1203,11 @@ static int kvm_put_fpu(X86CPU *cpu)
     return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_FPU, &fpu);
 }
 
-#define XSAVE_FCW_FSW     0
-#define XSAVE_FTW_FOP     1
-#define XSAVE_CWD_RIP     2
-#define XSAVE_CWD_RDP     4
-#define XSAVE_MXCSR       6
-#define XSAVE_ST_SPACE    8
-#define XSAVE_XMM_SPACE   40
-#define XSAVE_XSTATE_BV   128
-#define XSAVE_YMMH_SPACE  144
-#define XSAVE_BNDREGS     240
-#define XSAVE_BNDCSR      256
-#define XSAVE_OPMASK      272
-#define XSAVE_ZMM_Hi256   288
-#define XSAVE_Hi16_ZMM    416
-
-#define XSAVE_BYTE_OFFSET(word_offset) \
-    ((word_offset)*sizeof(((struct kvm_xsave*)0)->region[0]))
-
-#define ASSERT_OFFSET(word_offset, field) \
-    QEMU_BUILD_BUG_ON(XSAVE_BYTE_OFFSET(word_offset) != \
-                      offsetof(X86XSaveArea, field))
-
-ASSERT_OFFSET(XSAVE_FCW_FSW, legacy.fcw);
-ASSERT_OFFSET(XSAVE_FTW_FOP, legacy.ftw);
-ASSERT_OFFSET(XSAVE_CWD_RIP, legacy.fpip);
-ASSERT_OFFSET(XSAVE_CWD_RDP, legacy.fpdp);
-ASSERT_OFFSET(XSAVE_MXCSR, legacy.mxcsr);
-ASSERT_OFFSET(XSAVE_ST_SPACE, legacy.fpregs);
-ASSERT_OFFSET(XSAVE_XMM_SPACE, legacy.xmm_regs);
-ASSERT_OFFSET(XSAVE_XSTATE_BV, header.xstate_bv);
-ASSERT_OFFSET(XSAVE_YMMH_SPACE, avx_state);
-ASSERT_OFFSET(XSAVE_BNDREGS, bndreg_state);
-ASSERT_OFFSET(XSAVE_BNDCSR, bndcsr_state);
-ASSERT_OFFSET(XSAVE_OPMASK, opmask_state);
-ASSERT_OFFSET(XSAVE_ZMM_Hi256, zmm_hi256_state);
-ASSERT_OFFSET(XSAVE_Hi16_ZMM, hi16_zmm_state);
-
-
 static int kvm_put_xsave(X86CPU *cpu)
 {
     CPUX86State *env = &cpu->env;
-    struct kvm_xsave* xsave = env->kvm_xsave_buf;
+    X86XSaveArea *xsave = env->kvm_xsave_buf;
     uint16_t cwd, swd, twd;
-    uint8_t *xmm, *ymmh, *zmmh;
     int i, r;
 
     if (!has_xsave) {
@@ -1261,37 +1222,38 @@ static int kvm_put_xsave(X86CPU *cpu)
     for (i = 0; i < 8; ++i) {
         twd |= (!env->fptags[i]) << i;
     }
-    xsave->region[XSAVE_FCW_FSW] = (uint32_t)(swd << 16) + cwd;
-    xsave->region[XSAVE_FTW_FOP] = (uint32_t)(env->fpop << 16) + twd;
-    memcpy(&xsave->region[XSAVE_CWD_RIP], &env->fpip, sizeof(env->fpip));
-    memcpy(&xsave->region[XSAVE_CWD_RDP], &env->fpdp, sizeof(env->fpdp));
-    memcpy(&xsave->region[XSAVE_ST_SPACE], env->fpregs,
+    xsave->legacy.fcw = cwd;
+    xsave->legacy.fsw = swd;
+    xsave->legacy.ftw = twd;
+    xsave->legacy.fpop = env->fpop;
+    xsave->legacy.fpip = env->fpip;
+    xsave->legacy.fpdp = env->fpdp;
+    memcpy(&xsave->legacy.fpregs, env->fpregs,
             sizeof env->fpregs);
-    xsave->region[XSAVE_MXCSR] = env->mxcsr;
-    *(uint64_t *)&xsave->region[XSAVE_XSTATE_BV] = env->xstate_bv;
-    memcpy(&xsave->region[XSAVE_BNDREGS], env->bnd_regs,
+    xsave->legacy.mxcsr = env->mxcsr;
+    xsave->header.xstate_bv = env->xstate_bv;
+    memcpy(&xsave->bndreg_state.bnd_regs, env->bnd_regs,
             sizeof env->bnd_regs);
-    memcpy(&xsave->region[XSAVE_BNDCSR], &env->bndcs_regs,
-            sizeof(env->bndcs_regs));
-    memcpy(&xsave->region[XSAVE_OPMASK], env->opmask_regs,
+    xsave->bndcsr_state.bndcsr = env->bndcs_regs;
+    memcpy(&xsave->opmask_state.opmask_regs, env->opmask_regs,
             sizeof env->opmask_regs);
 
-    xmm = (uint8_t *)&xsave->region[XSAVE_XMM_SPACE];
-    ymmh = (uint8_t *)&xsave->region[XSAVE_YMMH_SPACE];
-    zmmh = (uint8_t *)&xsave->region[XSAVE_ZMM_Hi256];
-    for (i = 0; i < CPU_NB_REGS; i++, xmm += 16, ymmh += 16, zmmh += 32) {
-        stq_p(xmm,     env->xmm_regs[i].XMM_Q(0));
-        stq_p(xmm+8,   env->xmm_regs[i].XMM_Q(1));
-        stq_p(ymmh,    env->xmm_regs[i].XMM_Q(2));
-        stq_p(ymmh+8,  env->xmm_regs[i].XMM_Q(3));
-        stq_p(zmmh,    env->xmm_regs[i].XMM_Q(4));
-        stq_p(zmmh+8,  env->xmm_regs[i].XMM_Q(5));
-        stq_p(zmmh+16, env->xmm_regs[i].XMM_Q(6));
-        stq_p(zmmh+24, env->xmm_regs[i].XMM_Q(7));
+    for (i = 0; i < CPU_NB_REGS; i++) {
+        X86LegacyXSaveArea *legacy = &xsave->legacy;
+        XSaveAVX *avx = &xsave->avx_state;
+        XSaveZMM_Hi256 *zmm_hi256 = &xsave->zmm_hi256_state;
+        stq_p(&legacy->xmm_regs[i][0],     env->xmm_regs[i].XMM_Q(0));
+        stq_p(&legacy->xmm_regs[i][1],     env->xmm_regs[i].XMM_Q(1));
+        stq_p(&avx->ymmh[i][0],            env->xmm_regs[i].XMM_Q(2));
+        stq_p(&avx->ymmh[i][1],            env->xmm_regs[i].XMM_Q(3));
+        stq_p(&zmm_hi256->zmm_hi256[i][0], env->xmm_regs[i].XMM_Q(4));
+        stq_p(&zmm_hi256->zmm_hi256[i][1], env->xmm_regs[i].XMM_Q(5));
+        stq_p(&zmm_hi256->zmm_hi256[i][2], env->xmm_regs[i].XMM_Q(6));
+        stq_p(&zmm_hi256->zmm_hi256[i][3], env->xmm_regs[i].XMM_Q(7));
     }
 
 #ifdef TARGET_X86_64
-    memcpy(&xsave->region[XSAVE_Hi16_ZMM], &env->xmm_regs[16],
+    memcpy(&xsave->hi16_zmm_state.hi16_zmm, &env->xmm_regs[16],
             16 * sizeof env->xmm_regs[16]);
 #endif
     r = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_XSAVE, xsave);
@@ -1627,9 +1589,8 @@ static int kvm_get_fpu(X86CPU *cpu)
 static int kvm_get_xsave(X86CPU *cpu)
 {
     CPUX86State *env = &cpu->env;
-    struct kvm_xsave* xsave = env->kvm_xsave_buf;
+    X86XSaveArea *xsave = env->kvm_xsave_buf;
     int ret, i;
-    const uint8_t *xmm, *ymmh, *zmmh;
     uint16_t cwd, swd, twd;
 
     if (!has_xsave) {
@@ -1641,45 +1602,44 @@ static int kvm_get_xsave(X86CPU *cpu)
         return ret;
     }
 
-    cwd = (uint16_t)xsave->region[XSAVE_FCW_FSW];
-    swd = (uint16_t)(xsave->region[XSAVE_FCW_FSW] >> 16);
-    twd = (uint16_t)xsave->region[XSAVE_FTW_FOP];
-    env->fpop = (uint16_t)(xsave->region[XSAVE_FTW_FOP] >> 16);
+    cwd = xsave->legacy.fcw;
+    swd = xsave->legacy.fsw;
+    twd = xsave->legacy.ftw;
+    env->fpop = xsave->legacy.fpop;
     env->fpstt = (swd >> 11) & 7;
     env->fpus = swd;
     env->fpuc = cwd;
     for (i = 0; i < 8; ++i) {
         env->fptags[i] = !((twd >> i) & 1);
     }
-    memcpy(&env->fpip, &xsave->region[XSAVE_CWD_RIP], sizeof(env->fpip));
-    memcpy(&env->fpdp, &xsave->region[XSAVE_CWD_RDP], sizeof(env->fpdp));
-    env->mxcsr = xsave->region[XSAVE_MXCSR];
-    memcpy(env->fpregs, &xsave->region[XSAVE_ST_SPACE],
+    env->fpip = xsave->legacy.fpip;
+    env->fpdp = xsave->legacy.fpdp;
+    env->mxcsr = xsave->legacy.mxcsr;
+    memcpy(env->fpregs, &xsave->legacy.fpregs,
             sizeof env->fpregs);
-    env->xstate_bv = *(uint64_t *)&xsave->region[XSAVE_XSTATE_BV];
-    memcpy(env->bnd_regs, &xsave->region[XSAVE_BNDREGS],
+    env->xstate_bv = xsave->header.xstate_bv;
+    memcpy(env->bnd_regs, &xsave->bndreg_state.bnd_regs,
             sizeof env->bnd_regs);
-    memcpy(&env->bndcs_regs, &xsave->region[XSAVE_BNDCSR],
-            sizeof(env->bndcs_regs));
-    memcpy(env->opmask_regs, &xsave->region[XSAVE_OPMASK],
+    env->bndcs_regs = xsave->bndcsr_state.bndcsr;
+    memcpy(env->opmask_regs, &xsave->opmask_state.opmask_regs,
             sizeof env->opmask_regs);
 
-    xmm = (const uint8_t *)&xsave->region[XSAVE_XMM_SPACE];
-    ymmh = (const uint8_t *)&xsave->region[XSAVE_YMMH_SPACE];
-    zmmh = (const uint8_t *)&xsave->region[XSAVE_ZMM_Hi256];
-    for (i = 0; i < CPU_NB_REGS; i++, xmm += 16, ymmh += 16, zmmh += 32) {
-        env->xmm_regs[i].XMM_Q(0) = ldq_p(xmm);
-        env->xmm_regs[i].XMM_Q(1) = ldq_p(xmm+8);
-        env->xmm_regs[i].XMM_Q(2) = ldq_p(ymmh);
-        env->xmm_regs[i].XMM_Q(3) = ldq_p(ymmh+8);
-        env->xmm_regs[i].XMM_Q(4) = ldq_p(zmmh);
-        env->xmm_regs[i].XMM_Q(5) = ldq_p(zmmh+8);
-        env->xmm_regs[i].XMM_Q(6) = ldq_p(zmmh+16);
-        env->xmm_regs[i].XMM_Q(7) = ldq_p(zmmh+24);
+    for (i = 0; i < CPU_NB_REGS; i++) {
+        X86LegacyXSaveArea *legacy = &xsave->legacy;
+        XSaveAVX *avx = &xsave->avx_state;
+        XSaveZMM_Hi256 *zmm_hi256 = &xsave->zmm_hi256_state;
+        env->xmm_regs[i].XMM_Q(0) = ldq_p(&legacy->xmm_regs[i][0]);
+        env->xmm_regs[i].XMM_Q(1) = ldq_p(&legacy->xmm_regs[i][1]);
+        env->xmm_regs[i].XMM_Q(2) = ldq_p(&avx->ymmh[i][0]);
+        env->xmm_regs[i].XMM_Q(3) = ldq_p(&avx->ymmh[i][1]);
+        env->xmm_regs[i].XMM_Q(4) = ldq_p(&zmm_hi256->zmm_hi256[i][0]);
+        env->xmm_regs[i].XMM_Q(5) = ldq_p(&zmm_hi256->zmm_hi256[i][1]);
+        env->xmm_regs[i].XMM_Q(6) = ldq_p(&zmm_hi256->zmm_hi256[i][2]);
+        env->xmm_regs[i].XMM_Q(7) = ldq_p(&zmm_hi256->zmm_hi256[i][3]);
     }
 
 #ifdef TARGET_X86_64
-    memcpy(&env->xmm_regs[16], &xsave->region[XSAVE_Hi16_ZMM],
+    memcpy(&env->xmm_regs[16], &xsave->hi16_zmm_state.hi16_zmm,
            16 * sizeof env->xmm_regs[16]);
 #endif
     return 0;
-- 
2.1.0

Re: [Qemu-devel] [for-2.6 PATCH 0/3] target-i386: Use C struct for xsave area layout, offsets & sizes

[Paolo Bonzini]

On 28/11/2015 20:56, Eduardo Habkost wrote:
> I still need to figure out a way to write unit tests for the new
> code. Maybe I will just copy and paste the new and old functions,
> and test them locally (checking if they give the same results
> when translating blobs of random bytes).

Aren't the QEMU_BUILD_BUG_ON enough?  No need to delete them in patch 3,
though perhaps you can remove the #defines.

Paolo

Re: [Qemu-devel] [for-2.6 PATCH 0/3] target-i386: Use C struct for xsave area layout, offsets & sizes

[Eduardo Habkost]

On Mon, Nov 30, 2015 at 12:21:23PM +0100, Paolo Bonzini wrote:
> 
> 
> On 28/11/2015 20:56, Eduardo Habkost wrote:
> > I still need to figure out a way to write unit tests for the new
> > code. Maybe I will just copy and paste the new and old functions,
> > and test them locally (checking if they give the same results
> > when translating blobs of random bytes).
> 
> Aren't the QEMU_BUILD_BUG_ON enough?  No need to delete them in patch 3,
> though perhaps you can remove the #defines.

Just wanted to be 100% sure. Even if the offets are all correct,
I might have made other mistakes when translating the get/save
code.

About the QEMU_BUILD_BUG_ON lines, we can keep them if you like.
We could translate the uint32_t offsets to byte offsets after
ptach 3/3, to make them easier to compare to the Intel docs.

-- 
Eduardo