[RFC PATCH 0/7] Support protection keys in an AMD EPYC-Milan VM

[RFC PATCH 0/7] Support protection keys in an AMD EPYC-Milan VM

[David Edmondson]

AMD EPYC-Milan CPUs introduced support for protection keys, previously
available only with Intel CPUs.

AMD chose to place the XSAVE state component for the protection keys
at a different offset in the XSAVE state area than that chosen by
Intel.

To accommodate this, modify QEMU to behave appropriately on AMD
systems, allowing a VM to properly take advantage of the new feature.

Further, avoid manipulating XSAVE state components that are not
present on AMD systems.

The code in patch 6 that changes the CPUID 0x0d leaf is mostly dumped
somewhere that seemed to work - I'm not sure where it really belongs.

David Edmondson (7):
  target/i386: Declare constants for XSAVE offsets
  target/i386: Use constants for XSAVE offsets
  target/i386: Clarify the padding requirements of X86XSaveArea
  target/i386: Prepare for per-vendor X86XSaveArea layout
  target/i386: Introduce AMD X86XSaveArea sub-union
  target/i386: Adjust AMD XSAVE PKRU area offset in CPUID leaf 0xd
  target/i386: Manipulate only AMD XSAVE state on AMD

 target/i386/cpu.c            | 19 +++++----
 target/i386/cpu.h            | 80 ++++++++++++++++++++++++++++--------
 target/i386/kvm/kvm.c        | 57 +++++++++----------------
 target/i386/tcg/fpu_helper.c | 20 ++++++---
 target/i386/xsave_helper.c   | 70 +++++++++++++++++++------------
 5 files changed, 152 insertions(+), 94 deletions(-)

-- 
2.30.2

[RFC PATCH 1/7] target/i386: Declare constants for XSAVE offsets

[David Edmondson]

Declare and use manifest constants for the XSAVE state component
offsets.

Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 target/i386/cpu.h | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index e6836393f7..1fb732f366 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1305,6 +1305,22 @@ typedef struct XSavePKRU {
     uint32_t padding;
 } XSavePKRU;
 
+#define XSAVE_FCW_FSW_OFFSET    0x000
+#define XSAVE_FTW_FOP_OFFSET    0x004
+#define XSAVE_CWD_RIP_OFFSET    0x008
+#define XSAVE_CWD_RDP_OFFSET    0x010
+#define XSAVE_MXCSR_OFFSET      0x018
+#define XSAVE_ST_SPACE_OFFSET   0x020
+#define XSAVE_XMM_SPACE_OFFSET  0x0a0
+#define XSAVE_XSTATE_BV_OFFSET  0x200
+#define XSAVE_AVX_OFFSET        0x240
+#define XSAVE_BNDREG_OFFSET     0x3c0
+#define XSAVE_BNDCSR_OFFSET     0x400
+#define XSAVE_OPMASK_OFFSET     0x440
+#define XSAVE_ZMM_HI256_OFFSET  0x480
+#define XSAVE_HI16_ZMM_OFFSET   0x680
+#define XSAVE_PKRU_OFFSET       0xa80
+
 typedef struct X86XSaveArea {
     X86LegacyXSaveArea legacy;
     X86XSaveHeader header;
@@ -1325,19 +1341,19 @@ typedef struct X86XSaveArea {
     XSavePKRU pkru_state;
 } X86XSaveArea;
 
-QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, avx_state) != 0x240);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, avx_state) != XSAVE_AVX_OFFSET);
 QEMU_BUILD_BUG_ON(sizeof(XSaveAVX) != 0x100);
-QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, bndreg_state) != 0x3c0);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, bndreg_state) != XSAVE_BNDREG_OFFSET);
 QEMU_BUILD_BUG_ON(sizeof(XSaveBNDREG) != 0x40);
-QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, bndcsr_state) != 0x400);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, bndcsr_state) != XSAVE_BNDCSR_OFFSET);
 QEMU_BUILD_BUG_ON(sizeof(XSaveBNDCSR) != 0x40);
-QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, opmask_state) != 0x440);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, opmask_state) != XSAVE_OPMASK_OFFSET);
 QEMU_BUILD_BUG_ON(sizeof(XSaveOpmask) != 0x40);
-QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, zmm_hi256_state) != 0x480);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, zmm_hi256_state) != XSAVE_ZMM_HI256_OFFSET);
 QEMU_BUILD_BUG_ON(sizeof(XSaveZMM_Hi256) != 0x200);
-QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, hi16_zmm_state) != 0x680);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, hi16_zmm_state) != XSAVE_HI16_ZMM_OFFSET);
 QEMU_BUILD_BUG_ON(sizeof(XSaveHi16_ZMM) != 0x400);
-QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, pkru_state) != 0xA80);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, pkru_state) != XSAVE_PKRU_OFFSET);
 QEMU_BUILD_BUG_ON(sizeof(XSavePKRU) != 0x8);
 
 typedef enum TPRAccess {
-- 
2.30.2

[RFC PATCH 2/7] target/i386: Use constants for XSAVE offsets

[David Edmondson]

Where existing constants for XSAVE offsets exists, use them.

Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 target/i386/kvm/kvm.c | 56 ++++++++++++++-----------------------------
 1 file changed, 18 insertions(+), 38 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index d972eb4705..aff0774fef 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2397,44 +2397,24 @@ 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_PKRU        672
-
-#define XSAVE_BYTE_OFFSET(word_offset) \
-    ((word_offset) * sizeof_field(struct kvm_xsave, 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);
-ASSERT_OFFSET(XSAVE_PKRU, pkru_state);
+#define ASSERT_OFFSET(offset, field) \
+    QEMU_BUILD_BUG_ON(offset != offsetof(X86XSaveArea, field))
+
+ASSERT_OFFSET(XSAVE_FCW_FSW_OFFSET, legacy.fcw);
+ASSERT_OFFSET(XSAVE_FTW_FOP_OFFSET, legacy.ftw);
+ASSERT_OFFSET(XSAVE_CWD_RIP_OFFSET, legacy.fpip);
+ASSERT_OFFSET(XSAVE_CWD_RDP_OFFSET, legacy.fpdp);
+ASSERT_OFFSET(XSAVE_MXCSR_OFFSET, legacy.mxcsr);
+ASSERT_OFFSET(XSAVE_ST_SPACE_OFFSET, legacy.fpregs);
+ASSERT_OFFSET(XSAVE_XMM_SPACE_OFFSET, legacy.xmm_regs);
+ASSERT_OFFSET(XSAVE_XSTATE_BV_OFFSET, header.xstate_bv);
+ASSERT_OFFSET(XSAVE_AVX_OFFSET, avx_state);
+ASSERT_OFFSET(XSAVE_BNDREG_OFFSET, bndreg_state);
+ASSERT_OFFSET(XSAVE_BNDCSR_OFFSET, bndcsr_state);
+ASSERT_OFFSET(XSAVE_OPMASK_OFFSET, opmask_state);
+ASSERT_OFFSET(XSAVE_ZMM_HI256_OFFSET, zmm_hi256_state);
+ASSERT_OFFSET(XSAVE_HI16_ZMM_OFFSET, hi16_zmm_state);
+ASSERT_OFFSET(XSAVE_PKRU_OFFSET, pkru_state);
 
 static int kvm_put_xsave(X86CPU *cpu)
 {
-- 
2.30.2

[RFC PATCH 3/7] target/i386: Clarify the padding requirements of X86XSaveArea

[David Edmondson]

Replace the hard-coded size of offsets or structure elements with
defined constants or sizeof().

Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 target/i386/cpu.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 1fb732f366..0bb365bddf 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1329,7 +1329,13 @@ typedef struct X86XSaveArea {
 
     /* AVX State: */
     XSaveAVX avx_state;
-    uint8_t padding[960 - 576 - sizeof(XSaveAVX)];
+
+    /* Ensure that XSaveBNDREG is properly aligned. */
+    uint8_t padding[XSAVE_BNDREG_OFFSET
+                    - sizeof(X86LegacyXSaveArea)
+                    - sizeof(X86XSaveHeader)
+                    - sizeof(XSaveAVX)];
+
     /* MPX State: */
     XSaveBNDREG bndreg_state;
     XSaveBNDCSR bndcsr_state;
-- 
2.30.2

[RFC PATCH 4/7] target/i386: Prepare for per-vendor X86XSaveArea layout

[David Edmondson]

Move Intel specific components of X86XSaveArea into a sub-union.

Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 target/i386/cpu.c            | 12 ++++----
 target/i386/cpu.h            | 55 +++++++++++++++++++++---------------
 target/i386/kvm/kvm.c        | 12 ++++----
 target/i386/tcg/fpu_helper.c | 12 ++++----
 target/i386/xsave_helper.c   | 24 ++++++++--------
 5 files changed, 63 insertions(+), 52 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index c496bfa1c2..4f481691b4 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1418,27 +1418,27 @@ static const ExtSaveArea x86_ext_save_areas[] = {
             .size = sizeof(XSaveAVX) },
     [XSTATE_BNDREGS_BIT] =
           { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX,
-            .offset = offsetof(X86XSaveArea, bndreg_state),
+            .offset = offsetof(X86XSaveArea, intel.bndreg_state),
             .size = sizeof(XSaveBNDREG)  },
     [XSTATE_BNDCSR_BIT] =
           { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX,
-            .offset = offsetof(X86XSaveArea, bndcsr_state),
+            .offset = offsetof(X86XSaveArea, intel.bndcsr_state),
             .size = sizeof(XSaveBNDCSR)  },
     [XSTATE_OPMASK_BIT] =
           { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_AVX512F,
-            .offset = offsetof(X86XSaveArea, opmask_state),
+            .offset = offsetof(X86XSaveArea, intel.opmask_state),
             .size = sizeof(XSaveOpmask) },
     [XSTATE_ZMM_Hi256_BIT] =
           { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_AVX512F,
-            .offset = offsetof(X86XSaveArea, zmm_hi256_state),
+            .offset = offsetof(X86XSaveArea, intel.zmm_hi256_state),
             .size = sizeof(XSaveZMM_Hi256) },
     [XSTATE_Hi16_ZMM_BIT] =
           { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_AVX512F,
-            .offset = offsetof(X86XSaveArea, hi16_zmm_state),
+            .offset = offsetof(X86XSaveArea, intel.hi16_zmm_state),
             .size = sizeof(XSaveHi16_ZMM) },
     [XSTATE_PKRU_BIT] =
           { .feature = FEAT_7_0_ECX, .bits = CPUID_7_0_ECX_PKU,
-            .offset = offsetof(X86XSaveArea, pkru_state),
+            .offset = offsetof(X86XSaveArea, intel.pkru_state),
             .size = sizeof(XSavePKRU) },
 };
 
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 0bb365bddf..f1ce4e3008 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1330,36 +1330,47 @@ typedef struct X86XSaveArea {
     /* AVX State: */
     XSaveAVX avx_state;
 
-    /* Ensure that XSaveBNDREG is properly aligned. */
-    uint8_t padding[XSAVE_BNDREG_OFFSET
-                    - sizeof(X86LegacyXSaveArea)
-                    - sizeof(X86XSaveHeader)
-                    - 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;
-    /* PKRU State: */
-    XSavePKRU pkru_state;
+    union {
+        struct {
+            /* Ensure that XSaveBNDREG is properly aligned. */
+            uint8_t padding[XSAVE_BNDREG_OFFSET
+                            - sizeof(X86LegacyXSaveArea)
+                            - sizeof(X86XSaveHeader)
+                            - 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;
+            /* PKRU State: */
+            XSavePKRU pkru_state;
+        } intel;
+    };
 } X86XSaveArea;
 
-QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, avx_state) != XSAVE_AVX_OFFSET);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, avx_state)
+                  != XSAVE_AVX_OFFSET);
 QEMU_BUILD_BUG_ON(sizeof(XSaveAVX) != 0x100);
-QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, bndreg_state) != XSAVE_BNDREG_OFFSET);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, intel.bndreg_state)
+                  != XSAVE_BNDREG_OFFSET);
 QEMU_BUILD_BUG_ON(sizeof(XSaveBNDREG) != 0x40);
-QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, bndcsr_state) != XSAVE_BNDCSR_OFFSET);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, intel.bndcsr_state)
+                  != XSAVE_BNDCSR_OFFSET);
 QEMU_BUILD_BUG_ON(sizeof(XSaveBNDCSR) != 0x40);
-QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, opmask_state) != XSAVE_OPMASK_OFFSET);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, intel.opmask_state)
+                  != XSAVE_OPMASK_OFFSET);
 QEMU_BUILD_BUG_ON(sizeof(XSaveOpmask) != 0x40);
-QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, zmm_hi256_state) != XSAVE_ZMM_HI256_OFFSET);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, intel.zmm_hi256_state)
+                  != XSAVE_ZMM_HI256_OFFSET);
 QEMU_BUILD_BUG_ON(sizeof(XSaveZMM_Hi256) != 0x200);
-QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, hi16_zmm_state) != XSAVE_HI16_ZMM_OFFSET);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, intel.hi16_zmm_state)
+                  != XSAVE_HI16_ZMM_OFFSET);
 QEMU_BUILD_BUG_ON(sizeof(XSaveHi16_ZMM) != 0x400);
-QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, pkru_state) != XSAVE_PKRU_OFFSET);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, intel.pkru_state)
+                  != XSAVE_PKRU_OFFSET);
 QEMU_BUILD_BUG_ON(sizeof(XSavePKRU) != 0x8);
 
 typedef enum TPRAccess {
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index aff0774fef..417776a635 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2409,12 +2409,12 @@ ASSERT_OFFSET(XSAVE_ST_SPACE_OFFSET, legacy.fpregs);
 ASSERT_OFFSET(XSAVE_XMM_SPACE_OFFSET, legacy.xmm_regs);
 ASSERT_OFFSET(XSAVE_XSTATE_BV_OFFSET, header.xstate_bv);
 ASSERT_OFFSET(XSAVE_AVX_OFFSET, avx_state);
-ASSERT_OFFSET(XSAVE_BNDREG_OFFSET, bndreg_state);
-ASSERT_OFFSET(XSAVE_BNDCSR_OFFSET, bndcsr_state);
-ASSERT_OFFSET(XSAVE_OPMASK_OFFSET, opmask_state);
-ASSERT_OFFSET(XSAVE_ZMM_HI256_OFFSET, zmm_hi256_state);
-ASSERT_OFFSET(XSAVE_HI16_ZMM_OFFSET, hi16_zmm_state);
-ASSERT_OFFSET(XSAVE_PKRU_OFFSET, pkru_state);
+ASSERT_OFFSET(XSAVE_BNDREG_OFFSET, intel.bndreg_state);
+ASSERT_OFFSET(XSAVE_BNDCSR_OFFSET, intel.bndcsr_state);
+ASSERT_OFFSET(XSAVE_OPMASK_OFFSET, intel.opmask_state);
+ASSERT_OFFSET(XSAVE_ZMM_HI256_OFFSET, intel.zmm_hi256_state);
+ASSERT_OFFSET(XSAVE_HI16_ZMM_OFFSET, intel.hi16_zmm_state);
+ASSERT_OFFSET(XSAVE_PKRU_OFFSET, intel.pkru_state);
 
 static int kvm_put_xsave(X86CPU *cpu)
 {
diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c
index 1b30f1bb73..fba2de5b04 100644
--- a/target/i386/tcg/fpu_helper.c
+++ b/target/i386/tcg/fpu_helper.c
@@ -2637,13 +2637,13 @@ static void do_xsave(CPUX86State *env, target_ulong ptr, uint64_t rfbm,
         do_xsave_sse(env, ptr, ra);
     }
     if (opt & XSTATE_BNDREGS_MASK) {
-        do_xsave_bndregs(env, ptr + XO(bndreg_state), ra);
+        do_xsave_bndregs(env, ptr + XO(intel.bndreg_state), ra);
     }
     if (opt & XSTATE_BNDCSR_MASK) {
-        do_xsave_bndcsr(env, ptr + XO(bndcsr_state), ra);
+        do_xsave_bndcsr(env, ptr + XO(intel.bndcsr_state), ra);
     }
     if (opt & XSTATE_PKRU_MASK) {
-        do_xsave_pkru(env, ptr + XO(pkru_state), ra);
+        do_xsave_pkru(env, ptr + XO(intel.pkru_state), ra);
     }
 
     /* Update the XSTATE_BV field.  */
@@ -2836,7 +2836,7 @@ void helper_xrstor(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
     }
     if (rfbm & XSTATE_BNDREGS_MASK) {
         if (xstate_bv & XSTATE_BNDREGS_MASK) {
-            do_xrstor_bndregs(env, ptr + XO(bndreg_state), ra);
+            do_xrstor_bndregs(env, ptr + XO(intel.bndreg_state), ra);
             env->hflags |= HF_MPX_IU_MASK;
         } else {
             memset(env->bnd_regs, 0, sizeof(env->bnd_regs));
@@ -2845,7 +2845,7 @@ void helper_xrstor(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
     }
     if (rfbm & XSTATE_BNDCSR_MASK) {
         if (xstate_bv & XSTATE_BNDCSR_MASK) {
-            do_xrstor_bndcsr(env, ptr + XO(bndcsr_state), ra);
+            do_xrstor_bndcsr(env, ptr + XO(intel.bndcsr_state), ra);
         } else {
             memset(&env->bndcs_regs, 0, sizeof(env->bndcs_regs));
         }
@@ -2854,7 +2854,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) {
-            do_xrstor_pkru(env, ptr + XO(pkru_state), ra);
+            do_xrstor_pkru(env, ptr + XO(intel.pkru_state), ra);
         } else {
             env->pkru = 0;
         }
diff --git a/target/i386/xsave_helper.c b/target/i386/xsave_helper.c
index 818115e7d2..97dbab85d1 100644
--- a/target/i386/xsave_helper.c
+++ b/target/i386/xsave_helper.c
@@ -31,16 +31,16 @@ void x86_cpu_xsave_all_areas(X86CPU *cpu, X86XSaveArea *buf)
             sizeof env->fpregs);
     xsave->legacy.mxcsr = env->mxcsr;
     xsave->header.xstate_bv = env->xstate_bv;
-    memcpy(&xsave->bndreg_state.bnd_regs, env->bnd_regs,
+    memcpy(&xsave->intel.bndreg_state.bnd_regs, env->bnd_regs,
             sizeof env->bnd_regs);
-    xsave->bndcsr_state.bndcsr = env->bndcs_regs;
-    memcpy(&xsave->opmask_state.opmask_regs, env->opmask_regs,
+    xsave->intel.bndcsr_state.bndcsr = env->bndcs_regs;
+    memcpy(&xsave->intel.opmask_state.opmask_regs, env->opmask_regs,
             sizeof env->opmask_regs);
 
     for (i = 0; i < CPU_NB_REGS; i++) {
         uint8_t *xmm = xsave->legacy.xmm_regs[i];
         uint8_t *ymmh = xsave->avx_state.ymmh[i];
-        uint8_t *zmmh = xsave->zmm_hi256_state.zmm_hi256[i];
+        uint8_t *zmmh = xsave->intel.zmm_hi256_state.zmm_hi256[i];
         stq_p(xmm,     env->xmm_regs[i].ZMM_Q(0));
         stq_p(xmm+8,   env->xmm_regs[i].ZMM_Q(1));
         stq_p(ymmh,    env->xmm_regs[i].ZMM_Q(2));
@@ -52,9 +52,9 @@ void x86_cpu_xsave_all_areas(X86CPU *cpu, X86XSaveArea *buf)
     }
 
 #ifdef TARGET_X86_64
-    memcpy(&xsave->hi16_zmm_state.hi16_zmm, &env->xmm_regs[16],
+    memcpy(&xsave->intel.hi16_zmm_state.hi16_zmm, &env->xmm_regs[16],
             16 * sizeof env->xmm_regs[16]);
-    memcpy(&xsave->pkru_state, &env->pkru, sizeof env->pkru);
+    memcpy(&xsave->intel.pkru_state, &env->pkru, sizeof env->pkru);
 #endif
 
 }
@@ -83,16 +83,16 @@ void x86_cpu_xrstor_all_areas(X86CPU *cpu, const X86XSaveArea *buf)
     memcpy(env->fpregs, &xsave->legacy.fpregs,
             sizeof env->fpregs);
     env->xstate_bv = xsave->header.xstate_bv;
-    memcpy(env->bnd_regs, &xsave->bndreg_state.bnd_regs,
+    memcpy(env->bnd_regs, &xsave->intel.bndreg_state.bnd_regs,
             sizeof env->bnd_regs);
-    env->bndcs_regs = xsave->bndcsr_state.bndcsr;
-    memcpy(env->opmask_regs, &xsave->opmask_state.opmask_regs,
+    env->bndcs_regs = xsave->intel.bndcsr_state.bndcsr;
+    memcpy(env->opmask_regs, &xsave->intel.opmask_state.opmask_regs,
             sizeof env->opmask_regs);
 
     for (i = 0; i < CPU_NB_REGS; i++) {
         const uint8_t *xmm = xsave->legacy.xmm_regs[i];
         const uint8_t *ymmh = xsave->avx_state.ymmh[i];
-        const uint8_t *zmmh = xsave->zmm_hi256_state.zmm_hi256[i];
+        const uint8_t *zmmh = xsave->intel.zmm_hi256_state.zmm_hi256[i];
         env->xmm_regs[i].ZMM_Q(0) = ldq_p(xmm);
         env->xmm_regs[i].ZMM_Q(1) = ldq_p(xmm+8);
         env->xmm_regs[i].ZMM_Q(2) = ldq_p(ymmh);
@@ -104,9 +104,9 @@ void x86_cpu_xrstor_all_areas(X86CPU *cpu, const X86XSaveArea *buf)
     }
 
 #ifdef TARGET_X86_64
-    memcpy(&env->xmm_regs[16], &xsave->hi16_zmm_state.hi16_zmm,
+    memcpy(&env->xmm_regs[16], &xsave->intel.hi16_zmm_state.hi16_zmm,
            16 * sizeof env->xmm_regs[16]);
-    memcpy(&env->pkru, &xsave->pkru_state, sizeof env->pkru);
+    memcpy(&env->pkru, &xsave->intel.pkru_state, sizeof env->pkru);
 #endif
 
 }
-- 
2.30.2

[RFC PATCH 5/7] target/i386: Introduce AMD X86XSaveArea sub-union

[David Edmondson]

AMD stores the pkru_state at a different offset to Intel.

Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 target/i386/cpu.h     | 17 +++++++++++++++--
 target/i386/kvm/kvm.c |  3 ++-
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index f1ce4e3008..99f0d5d851 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1319,7 +1319,8 @@ typedef struct XSavePKRU {
 #define XSAVE_OPMASK_OFFSET     0x440
 #define XSAVE_ZMM_HI256_OFFSET  0x480
 #define XSAVE_HI16_ZMM_OFFSET   0x680
-#define XSAVE_PKRU_OFFSET       0xa80
+#define XSAVE_INTEL_PKRU_OFFSET 0xa80
+#define XSAVE_AMD_PKRU_OFFSET   0x980
 
 typedef struct X86XSaveArea {
     X86LegacyXSaveArea legacy;
@@ -1348,6 +1349,16 @@ typedef struct X86XSaveArea {
             /* PKRU State: */
             XSavePKRU pkru_state;
         } intel;
+        struct {
+            /* Ensure that XSavePKRU is properly aligned. */
+            uint8_t padding[XSAVE_AMD_PKRU_OFFSET
+                            - sizeof(X86LegacyXSaveArea)
+                            - sizeof(X86XSaveHeader)
+                            - sizeof(XSaveAVX)];
+
+            /* PKRU State: */
+            XSavePKRU pkru_state;
+        } amd;
     };
 } X86XSaveArea;
 
@@ -1370,7 +1381,9 @@ QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, intel.hi16_zmm_state)
                   != XSAVE_HI16_ZMM_OFFSET);
 QEMU_BUILD_BUG_ON(sizeof(XSaveHi16_ZMM) != 0x400);
 QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, intel.pkru_state)
-                  != XSAVE_PKRU_OFFSET);
+                  != XSAVE_INTEL_PKRU_OFFSET);
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, amd.pkru_state)
+                  != XSAVE_AMD_PKRU_OFFSET);
 QEMU_BUILD_BUG_ON(sizeof(XSavePKRU) != 0x8);
 
 typedef enum TPRAccess {
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 417776a635..9dd7db060d 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2414,7 +2414,8 @@ ASSERT_OFFSET(XSAVE_BNDCSR_OFFSET, intel.bndcsr_state);
 ASSERT_OFFSET(XSAVE_OPMASK_OFFSET, intel.opmask_state);
 ASSERT_OFFSET(XSAVE_ZMM_HI256_OFFSET, intel.zmm_hi256_state);
 ASSERT_OFFSET(XSAVE_HI16_ZMM_OFFSET, intel.hi16_zmm_state);
-ASSERT_OFFSET(XSAVE_PKRU_OFFSET, intel.pkru_state);
+ASSERT_OFFSET(XSAVE_INTEL_PKRU_OFFSET, intel.pkru_state);
+ASSERT_OFFSET(XSAVE_AMD_PKRU_OFFSET, amd.pkru_state);
 
 static int kvm_put_xsave(X86CPU *cpu)
 {
-- 
2.30.2

[RFC PATCH 6/7] target/i386: Adjust AMD XSAVE PKRU area offset in CPUID leaf 0xd

[David Edmondson]

AMD stores the pkru_state at a different offset to Intel, so update
the CPUID leaf which indicates such.

Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 target/i386/cpu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 4f481691b4..9340a477a3 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1397,7 +1397,7 @@ typedef struct ExtSaveArea {
     uint32_t offset, size;
 } ExtSaveArea;
 
-static const ExtSaveArea x86_ext_save_areas[] = {
+static ExtSaveArea x86_ext_save_areas[] = {
     [XSTATE_FP_BIT] = {
         /* x87 FP state component is always enabled if XSAVE is supported */
         .feature = FEAT_1_ECX, .bits = CPUID_EXT_XSAVE,
@@ -6088,6 +6088,11 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
             mark_unavailable_features(cpu, FEAT_7_0_EBX, CPUID_7_0_EBX_INTEL_PT, prefix);
         }
     }
+
+    if (IS_AMD_CPU(env)) {
+        x86_ext_save_areas[XSTATE_PKRU_BIT].offset =
+            offsetof(X86XSaveArea, amd.pkru_state);
+    }
 }
 
 static void x86_cpu_hyperv_realize(X86CPU *cpu)
-- 
2.30.2

[RFC PATCH 7/7] target/i386: Manipulate only AMD XSAVE state on AMD

[David Edmondson]

On AMD CPUs, ensure to save/load only the relevant XSAVE state.

Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 target/i386/tcg/fpu_helper.c | 12 +++++--
 target/i386/xsave_helper.c   | 70 ++++++++++++++++++++++--------------
 2 files changed, 54 insertions(+), 28 deletions(-)

diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c
index fba2de5b04..f1d4704b34 100644
--- a/target/i386/tcg/fpu_helper.c
+++ b/target/i386/tcg/fpu_helper.c
@@ -2643,7 +2643,11 @@ static void do_xsave(CPUX86State *env, target_ulong ptr, uint64_t rfbm,
         do_xsave_bndcsr(env, ptr + XO(intel.bndcsr_state), ra);
     }
     if (opt & XSTATE_PKRU_MASK) {
-        do_xsave_pkru(env, ptr + XO(intel.pkru_state), ra);
+        if (IS_AMD_CPU(env)) {
+            do_xsave_pkru(env, ptr + XO(amd.pkru_state), ra);
+        } else {
+            do_xsave_pkru(env, ptr + XO(intel.pkru_state), ra);
+        }
     }
 
     /* Update the XSTATE_BV field.  */
@@ -2854,7 +2858,11 @@ 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) {
-            do_xrstor_pkru(env, ptr + XO(intel.pkru_state), ra);
+            if (IS_AMD_CPU(env)) {
+                do_xrstor_pkru(env, ptr + XO(amd.pkru_state), ra);
+            } else {
+                do_xrstor_pkru(env, ptr + XO(intel.pkru_state), ra);
+            }
         } else {
             env->pkru = 0;
         }
diff --git a/target/i386/xsave_helper.c b/target/i386/xsave_helper.c
index 97dbab85d1..6b4501cf29 100644
--- a/target/i386/xsave_helper.c
+++ b/target/i386/xsave_helper.c
@@ -10,6 +10,7 @@ void x86_cpu_xsave_all_areas(X86CPU *cpu, X86XSaveArea *buf)
 {
     CPUX86State *env = &cpu->env;
     X86XSaveArea *xsave = buf;
+    const bool is_amd = IS_AMD_CPU(env);
 
     uint16_t cwd, swd, twd;
     int i;
@@ -31,30 +32,38 @@ void x86_cpu_xsave_all_areas(X86CPU *cpu, X86XSaveArea *buf)
             sizeof env->fpregs);
     xsave->legacy.mxcsr = env->mxcsr;
     xsave->header.xstate_bv = env->xstate_bv;
-    memcpy(&xsave->intel.bndreg_state.bnd_regs, env->bnd_regs,
-            sizeof env->bnd_regs);
-    xsave->intel.bndcsr_state.bndcsr = env->bndcs_regs;
-    memcpy(&xsave->intel.opmask_state.opmask_regs, env->opmask_regs,
-            sizeof env->opmask_regs);
+    if (!is_amd) {
+        memcpy(&xsave->intel.bndreg_state.bnd_regs, env->bnd_regs,
+               sizeof env->bnd_regs);
+        xsave->intel.bndcsr_state.bndcsr = env->bndcs_regs;
+        memcpy(&xsave->intel.opmask_state.opmask_regs, env->opmask_regs,
+               sizeof env->opmask_regs);
+    }
 
     for (i = 0; i < CPU_NB_REGS; i++) {
         uint8_t *xmm = xsave->legacy.xmm_regs[i];
         uint8_t *ymmh = xsave->avx_state.ymmh[i];
-        uint8_t *zmmh = xsave->intel.zmm_hi256_state.zmm_hi256[i];
         stq_p(xmm,     env->xmm_regs[i].ZMM_Q(0));
         stq_p(xmm+8,   env->xmm_regs[i].ZMM_Q(1));
         stq_p(ymmh,    env->xmm_regs[i].ZMM_Q(2));
         stq_p(ymmh+8,  env->xmm_regs[i].ZMM_Q(3));
-        stq_p(zmmh,    env->xmm_regs[i].ZMM_Q(4));
-        stq_p(zmmh+8,  env->xmm_regs[i].ZMM_Q(5));
-        stq_p(zmmh+16, env->xmm_regs[i].ZMM_Q(6));
-        stq_p(zmmh+24, env->xmm_regs[i].ZMM_Q(7));
+        if (!is_amd) {
+            uint8_t *zmmh = xsave->intel.zmm_hi256_state.zmm_hi256[i];
+            stq_p(zmmh,    env->xmm_regs[i].ZMM_Q(4));
+            stq_p(zmmh+8,  env->xmm_regs[i].ZMM_Q(5));
+            stq_p(zmmh+16, env->xmm_regs[i].ZMM_Q(6));
+            stq_p(zmmh+24, env->xmm_regs[i].ZMM_Q(7));
+        }
     }
 
 #ifdef TARGET_X86_64
-    memcpy(&xsave->intel.hi16_zmm_state.hi16_zmm, &env->xmm_regs[16],
-            16 * sizeof env->xmm_regs[16]);
-    memcpy(&xsave->intel.pkru_state, &env->pkru, sizeof env->pkru);
+    if (is_amd) {
+        memcpy(&xsave->amd.pkru_state, &env->pkru, sizeof env->pkru);
+    } else {
+        memcpy(&xsave->intel.hi16_zmm_state.hi16_zmm, &env->xmm_regs[16],
+               16 * sizeof env->xmm_regs[16]);
+        memcpy(&xsave->intel.pkru_state, &env->pkru, sizeof env->pkru);
+    }
 #endif
 
 }
@@ -64,6 +73,7 @@ void x86_cpu_xrstor_all_areas(X86CPU *cpu, const X86XSaveArea *buf)
 
     CPUX86State *env = &cpu->env;
     const X86XSaveArea *xsave = buf;
+    const bool is_amd = IS_AMD_CPU(env);
 
     int i;
     uint16_t cwd, swd, twd;
@@ -83,30 +93,38 @@ void x86_cpu_xrstor_all_areas(X86CPU *cpu, const X86XSaveArea *buf)
     memcpy(env->fpregs, &xsave->legacy.fpregs,
             sizeof env->fpregs);
     env->xstate_bv = xsave->header.xstate_bv;
-    memcpy(env->bnd_regs, &xsave->intel.bndreg_state.bnd_regs,
-            sizeof env->bnd_regs);
-    env->bndcs_regs = xsave->intel.bndcsr_state.bndcsr;
-    memcpy(env->opmask_regs, &xsave->intel.opmask_state.opmask_regs,
-            sizeof env->opmask_regs);
+    if (!is_amd) {
+        memcpy(env->bnd_regs, &xsave->intel.bndreg_state.bnd_regs,
+               sizeof env->bnd_regs);
+        env->bndcs_regs = xsave->intel.bndcsr_state.bndcsr;
+        memcpy(env->opmask_regs, &xsave->intel.opmask_state.opmask_regs,
+               sizeof env->opmask_regs);
+    }
 
     for (i = 0; i < CPU_NB_REGS; i++) {
         const uint8_t *xmm = xsave->legacy.xmm_regs[i];
         const uint8_t *ymmh = xsave->avx_state.ymmh[i];
-        const uint8_t *zmmh = xsave->intel.zmm_hi256_state.zmm_hi256[i];
         env->xmm_regs[i].ZMM_Q(0) = ldq_p(xmm);
         env->xmm_regs[i].ZMM_Q(1) = ldq_p(xmm+8);
         env->xmm_regs[i].ZMM_Q(2) = ldq_p(ymmh);
         env->xmm_regs[i].ZMM_Q(3) = ldq_p(ymmh+8);
-        env->xmm_regs[i].ZMM_Q(4) = ldq_p(zmmh);
-        env->xmm_regs[i].ZMM_Q(5) = ldq_p(zmmh+8);
-        env->xmm_regs[i].ZMM_Q(6) = ldq_p(zmmh+16);
-        env->xmm_regs[i].ZMM_Q(7) = ldq_p(zmmh+24);
+        if (!is_amd) {
+            const uint8_t *zmmh = xsave->intel.zmm_hi256_state.zmm_hi256[i];
+            env->xmm_regs[i].ZMM_Q(4) = ldq_p(zmmh);
+            env->xmm_regs[i].ZMM_Q(5) = ldq_p(zmmh+8);
+            env->xmm_regs[i].ZMM_Q(6) = ldq_p(zmmh+16);
+            env->xmm_regs[i].ZMM_Q(7) = ldq_p(zmmh+24);
+        }
     }
 
 #ifdef TARGET_X86_64
-    memcpy(&env->xmm_regs[16], &xsave->intel.hi16_zmm_state.hi16_zmm,
-           16 * sizeof env->xmm_regs[16]);
-    memcpy(&env->pkru, &xsave->intel.pkru_state, sizeof env->pkru);
+    if (is_amd) {
+        memcpy(&env->pkru, &xsave->amd.pkru_state, sizeof env->pkru);
+    } else {
+        memcpy(&env->xmm_regs[16], &xsave->intel.hi16_zmm_state.hi16_zmm,
+               16 * sizeof env->xmm_regs[16]);
+        memcpy(&env->pkru, &xsave->intel.pkru_state, sizeof env->pkru);
+    }
 #endif
 
 }
-- 
2.30.2

Re: [RFC PATCH 0/7] Support protection keys in an AMD EPYC-Milan VM

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210520145647.3483809-1-david.edmondson@oracle.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210520145647.3483809-1-david.edmondson@oracle.com
Subject: [RFC PATCH 0/7] Support protection keys in an AMD EPYC-Milan VM

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20210520145647.3483809-1-david.edmondson@oracle.com -> patchew/20210520145647.3483809-1-david.edmondson@oracle.com
Switched to a new branch 'test'
9761ad4 target/i386: Manipulate only AMD XSAVE state on AMD
fcba7d5 target/i386: Adjust AMD XSAVE PKRU area offset in CPUID leaf 0xd
0493da2 target/i386: Introduce AMD X86XSaveArea sub-union
e8d400c target/i386: Prepare for per-vendor X86XSaveArea layout
5a78b06 target/i386: Clarify the padding requirements of X86XSaveArea
ba3c3af target/i386: Use constants for XSAVE offsets
844afa9 target/i386: Declare constants for XSAVE offsets

=== OUTPUT BEGIN ===
1/7 Checking commit 844afa9929e3 (target/i386: Declare constants for XSAVE offsets)
WARNING: line over 80 characters
#60: FILE: target/i386/cpu.h:1352:
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, zmm_hi256_state) != XSAVE_ZMM_HI256_OFFSET);

WARNING: line over 80 characters
#63: FILE: target/i386/cpu.h:1354:
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, hi16_zmm_state) != XSAVE_HI16_ZMM_OFFSET);

total: 0 errors, 2 warnings, 48 lines checked

Patch 1/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/7 Checking commit ba3c3afe9795 (target/i386: Use constants for XSAVE offsets)
3/7 Checking commit 5a78b069f6d4 (target/i386: Clarify the padding requirements of X86XSaveArea)
4/7 Checking commit e8d400c3e40e (target/i386: Prepare for per-vendor X86XSaveArea layout)
5/7 Checking commit 0493da29bcd9 (target/i386: Introduce AMD X86XSaveArea sub-union)
6/7 Checking commit fcba7d58090d (target/i386: Adjust AMD XSAVE PKRU area offset in CPUID leaf 0xd)
7/7 Checking commit 9761ad41cd68 (target/i386: Manipulate only AMD XSAVE state on AMD)
ERROR: spaces required around that '+' (ctx:VxV)
#90: FILE: target/i386/xsave_helper.c:53:
+            stq_p(zmmh+8,  env->xmm_regs[i].ZMM_Q(5));
                       ^

ERROR: spaces required around that '+' (ctx:VxV)
#91: FILE: target/i386/xsave_helper.c:54:
+            stq_p(zmmh+16, env->xmm_regs[i].ZMM_Q(6));
                       ^

ERROR: spaces required around that '+' (ctx:VxV)
#92: FILE: target/i386/xsave_helper.c:55:
+            stq_p(zmmh+24, env->xmm_regs[i].ZMM_Q(7));
                       ^

ERROR: spaces required around that '+' (ctx:VxV)
#150: FILE: target/i386/xsave_helper.c:114:
+            env->xmm_regs[i].ZMM_Q(5) = ldq_p(zmmh+8);
                                                   ^

ERROR: spaces required around that '+' (ctx:VxV)
#151: FILE: target/i386/xsave_helper.c:115:
+            env->xmm_regs[i].ZMM_Q(6) = ldq_p(zmmh+16);
                                                   ^

ERROR: spaces required around that '+' (ctx:VxV)
#152: FILE: target/i386/xsave_helper.c:116:
+            env->xmm_regs[i].ZMM_Q(7) = ldq_p(zmmh+24);
                                                   ^

total: 6 errors, 0 warnings, 140 lines checked

Patch 7/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210520145647.3483809-1-david.edmondson@oracle.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [RFC PATCH 0/7] Support protection keys in an AMD EPYC-Milan VM

[David Edmondson]

On Thursday, 2021-05-20 at 15:56:40 +01, David Edmondson wrote:

> AMD EPYC-Milan CPUs introduced support for protection keys, previously
> available only with Intel CPUs.
>
> AMD chose to place the XSAVE state component for the protection keys
> at a different offset in the XSAVE state area than that chosen by
> Intel.
>
> To accommodate this, modify QEMU to behave appropriately on AMD
> systems, allowing a VM to properly take advantage of the new feature.
>
> Further, avoid manipulating XSAVE state components that are not
> present on AMD systems.
>
> The code in patch 6 that changes the CPUID 0x0d leaf is mostly dumped
> somewhere that seemed to work - I'm not sure where it really belongs.

Ping - any thoughts about this approach?

> David Edmondson (7):
>   target/i386: Declare constants for XSAVE offsets
>   target/i386: Use constants for XSAVE offsets
>   target/i386: Clarify the padding requirements of X86XSaveArea
>   target/i386: Prepare for per-vendor X86XSaveArea layout
>   target/i386: Introduce AMD X86XSaveArea sub-union
>   target/i386: Adjust AMD XSAVE PKRU area offset in CPUID leaf 0xd
>   target/i386: Manipulate only AMD XSAVE state on AMD
>
>  target/i386/cpu.c            | 19 +++++----
>  target/i386/cpu.h            | 80 ++++++++++++++++++++++++++++--------
>  target/i386/kvm/kvm.c        | 57 +++++++++----------------
>  target/i386/tcg/fpu_helper.c | 20 ++++++---
>  target/i386/xsave_helper.c   | 70 +++++++++++++++++++------------
>  5 files changed, 152 insertions(+), 94 deletions(-)
>
> -- 
> 2.30.2

dme.
-- 
You know your green from your red.

Re: [RFC PATCH 0/7] Support protection keys in an AMD EPYC-Milan VM

[Paolo Bonzini]

First of all, sorry for the delayed review.

On 20/05/21 16:56, David Edmondson wrote:
> AMD EPYC-Milan CPUs introduced support for protection keys, previously
> available only with Intel CPUs.
> 
> AMD chose to place the XSAVE state component for the protection keys
> at a different offset in the XSAVE state area than that chosen by
> Intel.
> 
> To accommodate this, modify QEMU to behave appropriately on AMD
> systems, allowing a VM to properly take advantage of the new feature.

Uff, that sucks. :(

If I understand correctly, the problem is that the layout of 
KVM_GET_XSAVE/KVM_SET_XSAVE depends on the host CPUID, which in 
retrospect would be obvious.  Is that correct?  If so, it would make 
sense and might even be easier to drop all usage of X86XSaveArea:

* update ext_save_areas based on CPUID information in kvm_cpu_instance_init

* make x86_cpu_xsave_all_areas and x86_cpu_xrstor_all_areas use the 
ext_save_areas offsets to build pointers to XSaveAVX, XSaveBNDREG, etc.

What do you think?

Paolo

> Further, avoid manipulating XSAVE state components that are not
> present on AMD systems.
> 
> The code in patch 6 that changes the CPUID 0x0d leaf is mostly dumped
> somewhere that seemed to work - I'm not sure where it really belongs.
> 
> David Edmondson (7):
>    target/i386: Declare constants for XSAVE offsets
>    target/i386: Use constants for XSAVE offsets
>    target/i386: Clarify the padding requirements of X86XSaveArea
>    target/i386: Prepare for per-vendor X86XSaveArea layout
>    target/i386: Introduce AMD X86XSaveArea sub-union
>    target/i386: Adjust AMD XSAVE PKRU area offset in CPUID leaf 0xd
>    target/i386: Manipulate only AMD XSAVE state on AMD
> 
>   target/i386/cpu.c            | 19 +++++----
>   target/i386/cpu.h            | 80 ++++++++++++++++++++++++++++--------
>   target/i386/kvm/kvm.c        | 57 +++++++++----------------
>   target/i386/tcg/fpu_helper.c | 20 ++++++---
>   target/i386/xsave_helper.c   | 70 +++++++++++++++++++------------
>   5 files changed, 152 insertions(+), 94 deletions(-)
> 

Re: [RFC PATCH 0/7] Support protection keys in an AMD EPYC-Milan VM

[David Edmondson]

On Friday, 2021-06-11 at 18:01:55 +02, Paolo Bonzini wrote:

> First of all, sorry for the delayed review.
>
> On 20/05/21 16:56, David Edmondson wrote:
>> AMD EPYC-Milan CPUs introduced support for protection keys, previously
>> available only with Intel CPUs.
>> 
>> AMD chose to place the XSAVE state component for the protection keys
>> at a different offset in the XSAVE state area than that chosen by
>> Intel.
>> 
>> To accommodate this, modify QEMU to behave appropriately on AMD
>> systems, allowing a VM to properly take advantage of the new feature.
>
> Uff, that sucks. :(
>
> If I understand correctly, the problem is that the layout of 
> KVM_GET_XSAVE/KVM_SET_XSAVE depends on the host CPUID, which in 
> retrospect would be obvious.  Is that correct?

Yes.

> If so, it would make sense and might even be easier to drop all usage
> of X86XSaveArea:
>
> * update ext_save_areas based on CPUID information in kvm_cpu_instance_init
>
> * make x86_cpu_xsave_all_areas and x86_cpu_xrstor_all_areas use the 
> ext_save_areas offsets to build pointers to XSaveAVX, XSaveBNDREG, etc.
>
> What do you think?

I will produce a patch and send it out.

> Paolo
>
>> Further, avoid manipulating XSAVE state components that are not
>> present on AMD systems.
>> 
>> The code in patch 6 that changes the CPUID 0x0d leaf is mostly dumped
>> somewhere that seemed to work - I'm not sure where it really belongs.
>> 
>> David Edmondson (7):
>>    target/i386: Declare constants for XSAVE offsets
>>    target/i386: Use constants for XSAVE offsets
>>    target/i386: Clarify the padding requirements of X86XSaveArea
>>    target/i386: Prepare for per-vendor X86XSaveArea layout
>>    target/i386: Introduce AMD X86XSaveArea sub-union
>>    target/i386: Adjust AMD XSAVE PKRU area offset in CPUID leaf 0xd
>>    target/i386: Manipulate only AMD XSAVE state on AMD
>> 
>>   target/i386/cpu.c            | 19 +++++----
>>   target/i386/cpu.h            | 80 ++++++++++++++++++++++++++++--------
>>   target/i386/kvm/kvm.c        | 57 +++++++++----------------
>>   target/i386/tcg/fpu_helper.c | 20 ++++++---
>>   target/i386/xsave_helper.c   | 70 +++++++++++++++++++------------
>>   5 files changed, 152 insertions(+), 94 deletions(-)
>> 

dme.
-- 
Oliver darling, call Mister Haney, I think our speakers are blown.

Re: [RFC PATCH 0/7] Support protection keys in an AMD EPYC-Milan VM

[Babu Moger]

David, Are you still working on v2 of these series? I was going to test
and review. Thanks

> -----Original Message-----
> From: David Edmondson <dme@dme.org>
> Sent: Tuesday, June 8, 2021 3:25 AM
> To: qemu-devel@nongnu.org
> Cc: kvm@vger.kernel.org; Eduardo Habkost <ehabkost@redhat.com>; Paolo
> Bonzini <pbonzini@redhat.com>; Marcelo Tosatti <mtosatti@redhat.com>;
> Richard Henderson <richard.henderson@linaro.org>; Moger, Babu
> <Babu.Moger@amd.com>
> Subject: Re: [RFC PATCH 0/7] Support protection keys in an AMD EPYC-Milan
> VM
> 
> On Thursday, 2021-05-20 at 15:56:40 +01, David Edmondson wrote:
> 
> > AMD EPYC-Milan CPUs introduced support for protection keys, previously
> > available only with Intel CPUs.
> >
> > AMD chose to place the XSAVE state component for the protection keys
> > at a different offset in the XSAVE state area than that chosen by
> > Intel.
> >
> > To accommodate this, modify QEMU to behave appropriately on AMD
> > systems, allowing a VM to properly take advantage of the new feature.
> >
> > Further, avoid manipulating XSAVE state components that are not
> > present on AMD systems.
> >
> > The code in patch 6 that changes the CPUID 0x0d leaf is mostly dumped
> > somewhere that seemed to work - I'm not sure where it really belongs.
> 
> Ping - any thoughts about this approach?
> 
> > David Edmondson (7):
> >   target/i386: Declare constants for XSAVE offsets
> >   target/i386: Use constants for XSAVE offsets
> >   target/i386: Clarify the padding requirements of X86XSaveArea
> >   target/i386: Prepare for per-vendor X86XSaveArea layout
> >   target/i386: Introduce AMD X86XSaveArea sub-union
> >   target/i386: Adjust AMD XSAVE PKRU area offset in CPUID leaf 0xd
> >   target/i386: Manipulate only AMD XSAVE state on AMD
> >
> >  target/i386/cpu.c            | 19 +++++----
> >  target/i386/cpu.h            | 80 ++++++++++++++++++++++++++++--------
> >  target/i386/kvm/kvm.c        | 57 +++++++++----------------
> >  target/i386/tcg/fpu_helper.c | 20 ++++++---
> >  target/i386/xsave_helper.c   | 70 +++++++++++++++++++------------
> >  5 files changed, 152 insertions(+), 94 deletions(-)
> >
> > --
> > 2.30.2
> 
> dme.
> --
> You know your green from your red.

Re: [RFC PATCH 0/7] Support protection keys in an AMD EPYC-Milan VM

[David Edmondson]

On Thursday, 2021-07-01 at 16:24:51 -05, Babu Moger wrote:

> David, Are you still working on v2 of these series? I was going to test
> and review. Thanks

Yes. I have something that works, but it's messy in places. I hope to
get it out in a couple of days.

>> -----Original Message-----
>> From: David Edmondson <dme@dme.org>
>> Sent: Tuesday, June 8, 2021 3:25 AM
>> To: qemu-devel@nongnu.org
>> Cc: kvm@vger.kernel.org; Eduardo Habkost <ehabkost@redhat.com>; Paolo
>> Bonzini <pbonzini@redhat.com>; Marcelo Tosatti <mtosatti@redhat.com>;
>> Richard Henderson <richard.henderson@linaro.org>; Moger, Babu
>> <Babu.Moger@amd.com>
>> Subject: Re: [RFC PATCH 0/7] Support protection keys in an AMD EPYC-Milan
>> VM
>> 
>> On Thursday, 2021-05-20 at 15:56:40 +01, David Edmondson wrote:
>> 
>> > AMD EPYC-Milan CPUs introduced support for protection keys, previously
>> > available only with Intel CPUs.
>> >
>> > AMD chose to place the XSAVE state component for the protection keys
>> > at a different offset in the XSAVE state area than that chosen by
>> > Intel.
>> >
>> > To accommodate this, modify QEMU to behave appropriately on AMD
>> > systems, allowing a VM to properly take advantage of the new feature.
>> >
>> > Further, avoid manipulating XSAVE state components that are not
>> > present on AMD systems.
>> >
>> > The code in patch 6 that changes the CPUID 0x0d leaf is mostly dumped
>> > somewhere that seemed to work - I'm not sure where it really belongs.
>> 
>> Ping - any thoughts about this approach?
>> 
>> > David Edmondson (7):
>> >   target/i386: Declare constants for XSAVE offsets
>> >   target/i386: Use constants for XSAVE offsets
>> >   target/i386: Clarify the padding requirements of X86XSaveArea
>> >   target/i386: Prepare for per-vendor X86XSaveArea layout
>> >   target/i386: Introduce AMD X86XSaveArea sub-union
>> >   target/i386: Adjust AMD XSAVE PKRU area offset in CPUID leaf 0xd
>> >   target/i386: Manipulate only AMD XSAVE state on AMD
>> >
>> >  target/i386/cpu.c            | 19 +++++----
>> >  target/i386/cpu.h            | 80 ++++++++++++++++++++++++++++--------
>> >  target/i386/kvm/kvm.c        | 57 +++++++++----------------
>> >  target/i386/tcg/fpu_helper.c | 20 ++++++---
>> >  target/i386/xsave_helper.c   | 70 +++++++++++++++++++------------
>> >  5 files changed, 152 insertions(+), 94 deletions(-)
>> >
>> > --
>> > 2.30.2
>> 
>> dme.
>> --
>> You know your green from your red.

dme.
-- 
When you were the brightest star, who were the shadows?