| Message ID | 1448740611-3096-2-git-send-email-ehabkost@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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~
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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).
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~ -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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~ -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(+)