[Qemu-devel] [PATCH v3 0/3] target-i386: add memory protection-key support

[Qemu-devel] [PATCH v3 0/3] target-i386: add memory protection-key support

[Huaitong Han]

Changes in v3:
*Fix cpuid_7_0_ecx_feature_name error.

Changes in v2:
*Fix memcpy error for xsave state.
*Fix TCG_7_0_ECX_FEATURES to 0.
*Make subjects more readable.

The protection-key feature provides an additional mechanism by which IA-32e
paging controls access to usermode addresses.

Hardware support for protection keys for user pages is enumerated with CPUID
feature flag CPUID.7.0.ECX[3]:PKU. Software support is CPUID.7.0.ECX[4]:OSPKE
with the setting of CR4.PKE(bit 22).

The PKRU register is XSAVE-managed state CPUID.D.0.EAX[9], the size of XSAVE
state component for PKRU is 8 bytes, the offset is 0xa80.

The specification of Protection Keys can be found at SDM (4.6.2, volume 3)
http://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-manual-325462.pdf.

Huaitong Han (3):
  target-i386: add pkeys support for cpuid handling
  target-i386: add pkeys support for xsave state handling
  target-i386: add pkeys support for vm migration

 target-i386/cpu.c     | 23 ++++++++++++++++++++++-
 target-i386/cpu.h     |  7 +++++++
 target-i386/kvm.c     |  3 +++
 target-i386/machine.c | 23 +++++++++++++++++++++++
 4 files changed, 55 insertions(+), 1 deletion(-)

-- 
2.4.3

[Qemu-devel] [PATCH v3 1/3] target-i386: add pkeys support for cpuid handling

[Huaitong Han]

This patch adds pkeys support for cpuid handling.

Signed-off-by: Huaitong Han <huaitong.han@intel.com>

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4d1b085..3c11e02 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -264,6 +264,17 @@ static const char *cpuid_7_0_ebx_feature_name[] = {
     NULL, NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
 };
 
+static const char *cpuid_7_0_ecx_feature_name[] = {
+    NULL, NULL, NULL, "pku",
+    "ospke", NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+};
+
 static const char *cpuid_apm_edx_feature_name[] = {
     NULL, NULL, NULL, NULL,
     NULL, NULL, NULL, NULL,
@@ -351,6 +362,7 @@ static const char *cpuid_6_feature_name[] = {
           CPUID_7_0_EBX_FSGSBASE, CPUID_7_0_EBX_HLE, CPUID_7_0_EBX_AVX2,
           CPUID_7_0_EBX_ERMS, CPUID_7_0_EBX_INVPCID, CPUID_7_0_EBX_RTM,
           CPUID_7_0_EBX_RDSEED */
+#define TCG_7_0_ECX_FEATURES 0
 #define TCG_APM_FEATURES 0
 #define TCG_6_EAX_FEATURES CPUID_6_EAX_ARAT
 
@@ -408,6 +420,13 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
         .cpuid_reg = R_EBX,
         .tcg_features = TCG_7_0_EBX_FEATURES,
     },
+    [FEAT_7_0_ECX] = {
+        .feat_names = cpuid_7_0_ecx_feature_name,
+        .cpuid_eax = 7,
+        .cpuid_needs_ecx = true, .cpuid_ecx = 0,
+        .cpuid_reg = R_ECX,
+        .tcg_features = TCG_7_0_ECX_FEATURES,
+    },
     [FEAT_8000_0007_EDX] = {
         .feat_names = cpuid_apm_edx_feature_name,
         .cpuid_eax = 0x80000007,
@@ -2401,7 +2420,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         if (count == 0) {
             *eax = 0; /* Maximum ECX value for sub-leaves */
             *ebx = env->features[FEAT_7_0_EBX]; /* Feature flags */
-            *ecx = 0; /* Reserved */
+            *ecx = env->features[FEAT_7_0_ECX]; /* Feature flags */
             *edx = 0; /* Reserved */
         } else {
             *eax = 0;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index ead2832..c2e7501 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -408,6 +408,7 @@ typedef enum FeatureWord {
     FEAT_1_EDX,         /* CPUID[1].EDX */
     FEAT_1_ECX,         /* CPUID[1].ECX */
     FEAT_7_0_EBX,       /* CPUID[EAX=7,ECX=0].EBX */
+    FEAT_7_0_ECX,       /* CPUID[EAX=7,ECX=0].ECX */
     FEAT_8000_0001_EDX, /* CPUID[8000_0001].EDX */
     FEAT_8000_0001_ECX, /* CPUID[8000_0001].ECX */
     FEAT_8000_0007_EDX, /* CPUID[8000_0007].EDX */
@@ -576,6 +577,9 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 #define CPUID_7_0_EBX_AVX512ER (1U << 27) /* AVX-512 Exponential and Reciprocal */
 #define CPUID_7_0_EBX_AVX512CD (1U << 28) /* AVX-512 Conflict Detection */
 
+#define CPUID_7_0_ECX_PKU      (1U << 3)
+#define CPUID_7_0_ECX_OSPKE    (1U << 4)
+
 #define CPUID_XSAVE_XSAVEOPT   (1U << 0)
 #define CPUID_XSAVE_XSAVEC     (1U << 1)
 #define CPUID_XSAVE_XGETBV1    (1U << 2)
-- 
2.4.3

Re: [Qemu-devel] [PATCH v3 1/3] target-i386: add pkeys support for cpuid handling

[Eduardo Habkost]

On Wed, Nov 18, 2015 at 10:20:15AM +0800, Huaitong Han wrote:
[...]
> @@ -408,6 +420,13 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>          .cpuid_reg = R_EBX,
>          .tcg_features = TCG_7_0_EBX_FEATURES,
>      },
> +    [FEAT_7_0_ECX] = {
> +        .feat_names = cpuid_7_0_ecx_feature_name,
> +        .cpuid_eax = 7,
> +        .cpuid_needs_ecx = true, .cpuid_ecx = 0,
> +        .cpuid_reg = R_ECX,
> +        .tcg_features = TCG_7_0_ECX_FEATURES,
> +    },

The patch looks good, but when we add the feature names to
cpuid_7_0_ecx_feature_name, QEMU will consider them as
migratable, but they are truly migratable only after we add the
ext_save_areas entry.

We can fix this by moving cpuid_7_0_ecx_feature_name to patch 2/3
(or to a separate patch, to be applied after 2/3).

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 1/3] target-i386: add pkeys support for cpuid handling

[Han, Huaitong]

On Wed, 2015-11-18 at 13:58 -0200, Eduardo Habkost wrote:
> On Wed, Nov 18, 2015 at 10:20:15AM +0800, Huaitong Han wrote:
> [...]
> > @@ -408,6 +420,13 @@ static FeatureWordInfo
> > feature_word_info[FEATURE_WORDS] = {
> >          .cpuid_reg = R_EBX,
> >          .tcg_features = TCG_7_0_EBX_FEATURES,
> >      },
> > +    [FEAT_7_0_ECX] = {
> > +        .feat_names = cpuid_7_0_ecx_feature_name,
> > +        .cpuid_eax = 7,
> > +        .cpuid_needs_ecx = true, .cpuid_ecx = 0,
> > +        .cpuid_reg = R_ECX,
> > +        .tcg_features = TCG_7_0_ECX_FEATURES,
> > +    },
> 
> The patch looks good, but when we add the feature names to
> cpuid_7_0_ecx_feature_name, QEMU will consider them as
> migratable, but they are truly migratable only after we add the
> ext_save_areas entry.

> We can fix this by moving cpuid_7_0_ecx_feature_name to patch 2/3
> (or to a separate patch, to be applied after 2/3).

I understand it has always been that QEMU considers the feature of
 cpuid_7_0_ecx_feature_name as migratable. If the feature is
 unmigratable, it will been added to unmigratable_flags.

A series of patches do complete a full function, moving
 cpuid_7_0_ecx_feature_name to 2/3 patch may make 2/3 patch look
better, but make 1/3 patch look somewhat incomplete.

Maybe it is a solution that adding the feature to unmigratable_flags in
1/3 patch, and deleting unmigratable_flags in 2/3 patch, but I think it
is pointless.

Re: [Qemu-devel] [PATCH v3 1/3] target-i386: add pkeys support for cpuid handling

[Paolo Bonzini]

On 19/11/2015 07:36, Han, Huaitong wrote:
> I understand it has always been that QEMU considers the feature of
>  cpuid_7_0_ecx_feature_name as migratable. If the feature is
>  unmigratable, it will been added to unmigratable_flags.
> 
> A series of patches do complete a full function, moving
>  cpuid_7_0_ecx_feature_name to 2/3 patch may make 2/3 patch look
> better, but make 1/3 patch look somewhat incomplete.
> 
> Maybe it is a solution that adding the feature to unmigratable_flags in
> 1/3 patch, and deleting unmigratable_flags in 2/3 patch, but I think it
> is pointless.

Or just squash everything together.  After all we're talking of

 4 files changed, 55 insertions(+), 1 deletion(-)

It's not a large patch.

Paolo

Re: [Qemu-devel] [PATCH v3 1/3] target-i386: add pkeys support for cpuid handling

[Eduardo Habkost]

On Thu, Nov 19, 2015 at 12:10:49PM +0100, Paolo Bonzini wrote:
> 
> 
> On 19/11/2015 07:36, Han, Huaitong wrote:
> > I understand it has always been that QEMU considers the feature of
> >  cpuid_7_0_ecx_feature_name as migratable. If the feature is
> >  unmigratable, it will been added to unmigratable_flags.
> > 
> > A series of patches do complete a full function, moving
> >  cpuid_7_0_ecx_feature_name to 2/3 patch may make 2/3 patch look
> > better, but make 1/3 patch look somewhat incomplete.
> > 
> > Maybe it is a solution that adding the feature to unmigratable_flags in
> > 1/3 patch, and deleting unmigratable_flags in 2/3 patch, but I think it
> > is pointless.
> 
> Or just squash everything together.  After all we're talking of
> 
>  4 files changed, 55 insertions(+), 1 deletion(-)
> 
> It's not a large patch.

It makes sense. Adding the state to X86CPU (2/3) is useful only
if we migrate it (3/3), and adding the feature names (1/3) is
useful only if we can handle the new state.

I will squash everything together when applying, in case there's
no new version.

-- 
Eduardo

[Qemu-devel] [PATCH v3 2/3] target-i386: add pkeys support for xsave state handling

[Huaitong Han]

This patch adds pkeys support for xsave state handling.

Signed-off-by: Huaitong Han <huaitong.han@intel.com>

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3c11e02..456cb3b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -487,6 +487,8 @@ static const ExtSaveArea ext_save_areas[] = {
             .offset = 0x480, .size = 0x200 },
     [7] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_AVX512F,
             .offset = 0x680, .size = 0x400 },
+    [9] = { .feature = FEAT_7_0_ECX, .bits = CPUID_7_0_ECX_PKU,
+            .offset = 0xA80, .size = 0x8 },
 };
 
 const char *get_register_name_32(unsigned int reg)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index c2e7501..2230b3e 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -401,6 +401,7 @@
 #define XSTATE_OPMASK                   (1ULL << 5)
 #define XSTATE_ZMM_Hi256                (1ULL << 6)
 #define XSTATE_Hi16_ZMM                 (1ULL << 7)
+#define XSTATE_PKRU                     (1ULL << 9)
 
 
 /* CPUID feature words */
@@ -984,6 +985,8 @@ typedef struct CPUX86State {
     uint64_t xcr0;
     uint64_t xss;
 
+    uint32_t pkru;
+
     TPRAccess tpr_access_type;
 } CPUX86State;
 
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 066d03d..16a8eff 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1092,6 +1092,7 @@ static int kvm_put_fpu(X86CPU *cpu)
 #define XSAVE_OPMASK      272
 #define XSAVE_ZMM_Hi256   288
 #define XSAVE_Hi16_ZMM    416
+#define XSAVE_PKRU        672
 
 static int kvm_put_xsave(X86CPU *cpu)
 {
@@ -1145,6 +1146,7 @@ static int kvm_put_xsave(X86CPU *cpu)
 #ifdef TARGET_X86_64
     memcpy(&xsave->region[XSAVE_Hi16_ZMM], &env->xmm_regs[16],
             16 * sizeof env->xmm_regs[16]);
+    memcpy(&xsave->region[XSAVE_PKRU], &env->pkru, sizeof env->pkru);
 #endif
     r = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_XSAVE, xsave);
     return r;
@@ -1516,6 +1518,7 @@ static int kvm_get_xsave(X86CPU *cpu)
 #ifdef TARGET_X86_64
     memcpy(&env->xmm_regs[16], &xsave->region[XSAVE_Hi16_ZMM],
            16 * sizeof env->xmm_regs[16]);
+    memcpy(&env->pkru, &xsave->region[XSAVE_PKRU], sizeof env->pkru);
 #endif
     return 0;
 }
-- 
2.4.3

[Qemu-devel] [PATCH v3 3/3] target-i386: add pkeys support for vm migration

[Huaitong Han]

This patch adds pkeys support for vm migration.

Signed-off-by: Huaitong Han <huaitong.han@intel.com>

diff --git a/target-i386/machine.c b/target-i386/machine.c
index a0df64b..1b190c7 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -725,6 +725,26 @@ static const VMStateDescription vmstate_xss = {
         VMSTATE_END_OF_LIST()
     }
 };
+#ifdef TARGET_X86_64
+static bool pkru_needed(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    CPUX86State *env = &cpu->env;
+
+    return env->pkru != 0;
+}
+
+static const VMStateDescription vmstate_pkru = {
+    .name = "cpu/pkru",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = pkru_needed,
+    .fields = (VMStateField[]){
+        VMSTATE_UINT32(env.pkru, X86CPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+#endif
 
 VMStateDescription vmstate_x86_cpu = {
     .name = "cpu",
@@ -844,6 +864,9 @@ VMStateDescription vmstate_x86_cpu = {
         &vmstate_msr_hyperv_time,
         &vmstate_avx512,
         &vmstate_xss,
+#ifdef TARGET_X86_64
+        &vmstate_pkru,
+#endif
         NULL
     }
 };
-- 
2.4.3

Re: [Qemu-devel] [PATCH v3 0/3] target-i386: add memory protection-key support

[Eduardo Habkost]

On Wed, Nov 18, 2015 at 10:20:14AM +0800, Huaitong Han wrote:
> Changes in v3:
> *Fix cpuid_7_0_ecx_feature_name error.
> 
> Changes in v2:
> *Fix memcpy error for xsave state.
> *Fix TCG_7_0_ECX_FEATURES to 0.
> *Make subjects more readable.
> 
> The protection-key feature provides an additional mechanism by which IA-32e
> paging controls access to usermode addresses.
> 
> Hardware support for protection keys for user pages is enumerated with CPUID
> feature flag CPUID.7.0.ECX[3]:PKU. Software support is CPUID.7.0.ECX[4]:OSPKE
> with the setting of CR4.PKE(bit 22).
> 
> The PKRU register is XSAVE-managed state CPUID.D.0.EAX[9], the size of XSAVE
> state component for PKRU is 8 bytes, the offset is 0xa80.

Is every CPU supporting PKU guaranteed to have
CPUID.(EAX=0DH,ECX=9):EBX = 0xa80? Where is the PKRU state
offset/layout documented?

> 
> The specification of Protection Keys can be found at SDM (4.6.2, volume 3)
> http://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-manual-325462.pdf.
> 
[...]

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 0/3] target-i386: add memory protection-key support

[Paolo Bonzini]

On 18/11/2015 18:51, Eduardo Habkost wrote:
> Is every CPU supporting PKU guaranteed to have
> CPUID.(EAX=0DH,ECX=9):EBX = 0xa80?

We asked Intel a while ago when reorganizing XSAVE support in KVM and
QEMU.  Unfortunately, Intel is not listing the offsets anymore in the
documentation, but they confirmed at the time that offsets won't change.

http://lists.xen.org/archives/html/xen-devel/2013-09/msg00484.html is
pretty clear in this respect:

>> [adding H. Peter Anvin... the context is whether the layout of the
>> XSAVE/XRSTOR area is fixed, including the offset of each separate
>> Ext_SAVE_Area].
> 
> It is.
> 
>> So please Intel, pretty please do not modify the XSAVE offsets, and
>> clarify this as soon as possible.
> 
> They will not change.
> 
>         -hpa


This of course doesn't mean that the 0xa80 is correct; it only means
that if it is correct, it will always stay correct. :)

It makes sense that 0xa80 is correct, since ECX=8 is a supervisor state
(thus never saved by XSAVE/XSAVEOPT).

The fact that standard format does not account for supervisor states
means that supporting supervisor states in KVM might encounter a few
small complications.  In particular we might have to modify KVM and QEMU
to support compacted format in KVM_GET/SET_XSAVE.  For now the only
supervisor state is just a bunch of MSRs, so it would not be necessary
to include it in KVM_GET/SET_XSAVE.  Problem averted for now, then.

> Where is the PKRU state offset/layout documented?

Volume 1 of the SDM, section 13.5.7 ("PKRU State") documents the layout.
 (Careful: chapter 13 of volume 1 is "Managing State Using the XSAVE
Feature Set"; chapter 13 of volume 3 is "System Programming for
Instruction Set Extensions and Processor Extended States", and also
deals with XSAVE).

Paolo

Re: [Qemu-devel] [PATCH v3 0/3] target-i386: add memory protection-key support

[Eduardo Habkost]

On Wed, Nov 18, 2015 at 10:20:14AM +0800, Huaitong Han wrote:
> Changes in v3:
> *Fix cpuid_7_0_ecx_feature_name error.
> 
> Changes in v2:
> *Fix memcpy error for xsave state.
> *Fix TCG_7_0_ECX_FEATURES to 0.
> *Make subjects more readable.
> 
> The protection-key feature provides an additional mechanism by which IA-32e
> paging controls access to usermode addresses.
> 
> Hardware support for protection keys for user pages is enumerated with CPUID
> feature flag CPUID.7.0.ECX[3]:PKU. Software support is CPUID.7.0.ECX[4]:OSPKE
> with the setting of CR4.PKE(bit 22).
> 
> The PKRU register is XSAVE-managed state CPUID.D.0.EAX[9], the size of XSAVE
> state component for PKRU is 8 bytes, the offset is 0xa80.
> 
> The specification of Protection Keys can be found at SDM (4.6.2, volume 3)
> http://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-manual-325462.pdf.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

The patches were squashed together and queued in x86-next branch
for 2.6.

> 
> Huaitong Han (3):
>   target-i386: add pkeys support for cpuid handling
>   target-i386: add pkeys support for xsave state handling
>   target-i386: add pkeys support for vm migration
> 
>  target-i386/cpu.c     | 23 ++++++++++++++++++++++-
>  target-i386/cpu.h     |  7 +++++++
>  target-i386/kvm.c     |  3 +++
>  target-i386/machine.c | 23 +++++++++++++++++++++++
>  4 files changed, 55 insertions(+), 1 deletion(-)
> 
> -- 
> 2.4.3
> 
> 

-- 
Eduardo