diff mbox

[5/5] target-i386: implement PKE for TCG

Message ID 1455037993-25461-6-git-send-email-pbonzini@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paolo Bonzini Feb. 9, 2016, 5:13 p.m. UTC 
This includes setting up TLB permissions, the new RDPKRU/WRPKRU
instructions, and XSAVE support.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/cpu.c         |  2 +-
 target-i386/cpu.h         |  8 ++++++-
 target-i386/fpu_helper.c  | 54 +++++++++++++++++++++++++++++++++++++++++++----
 target-i386/helper.c      | 28 ++++++++++++++++++++++++
 target-i386/helper.h      |  2 ++
 target-i386/misc_helper.c | 32 ++++++++++++++++++++++++++++
 target-i386/translate.c   |  6 ++++++
 7 files changed, 126 insertions(+), 6 deletions(-)

Comments

Richard Henderson Feb. 9, 2016, 6:13 p.m. UTC | #1 
On 02/10/2016 04:13 AM, Paolo Bonzini wrote:
> @@ -157,6 +157,7 @@
>   #define HF_SMAP_SHIFT       23 /* CR4.SMAP */
>   #define HF_IOBPT_SHIFT      24 /* an io breakpoint enabled */
>   #define HF_OSXSAVE_SHIFT    25 /* CR4.OSXSAVE */
> +#define HF_PKE_SHIFT        26 /* CR4.PKE enabled */

I don't believe you need this bit at all.  Certainly you don't use it in 
translate.c, where any such test ought to have been.

>   static uint64_t get_xinuse(CPUX86State *env)
>   {
> -    /* We don't track XINUSE.  We could calculate it here, but it's
> -       probably less work to simply indicate all components in use.  */
> -    return -1;
> +    uint64_t inuse = -1;
> +
> +    /* For the most part, we don't track XINUSE.  We could calculate it
> +     * here for all components, but it's probably less work to simply
> +     * indicate in use.  That said, for state that is important
> +     * enough to track in HFLAGS, we might as well use that here.
> +     */
> +    if ((env->hflags & HF_PKE_MASK) == 0) {
> +       inuse &= ~XSTATE_PKRU;
> +    }
> +    return inuse;
> +}

That does change this of course.  But the entire state of the feature is one 
32-bit integer -- it's just as easy to test that.

> @@ -1357,6 +1399,10 @@ void helper_xrstor(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
>               memset(env->xmm_regs, 0, sizeof(env->xmm_regs));
>           }
>       }
> +
> +    if (rfbm & XSTATE_PKRU) {
> +        do_xrstor_pkru(env, ptr, GETPC());
> +    }

There should be an "ra" variable at the top of this function that already holds 
GETPC.  Are you forgetting a check on xstate_bv here, to clear pkru?

> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index f5f0bec..a55628f 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -689,6 +689,16 @@ void cpu_x86_update_cr4(CPUX86State *env, uint32_t new_cr4)
>           hflags |= HF_SMAP_MASK;
>       }
>
> +    if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_PKU)) {
> +        new_cr4 &= ~CR4_PKE_MASK;
> +    }
> +    env->hflags &= ~HF_PKE_MASK;
> +    env->features[FEAT_7_0_ECX] &= ~CPUID_7_0_ECX_OSPKE;
> +    if (new_cr4 & CR4_PKE_MASK) {
> +        env->hflags |= HF_PKE_MASK;
> +	env->features[FEAT_7_0_ECX] |= CPUID_7_0_ECX_OSPKE;
> +    }

Don't change features bits.  Do this test in cpu_x86_cpuid instead.  See where 
we give the same treatment to OSXSAVE.

> diff --git a/target-i386/misc_helper.c b/target-i386/misc_helper.c
> index 460257f..b517559 100644
> --- a/target-i386/misc_helper.c
> +++ b/target-i386/misc_helper.c
> @@ -600,3 +600,35 @@ void helper_debug(CPUX86State *env)
>       cs->exception_index = EXCP_DEBUG;
>       cpu_loop_exit(cs);
>   }
> +
> +void helper_rdpkru(CPUX86State *env)
> +{
> +    if ((env->cr[4] & CR4_PKE_MASK) == 0) {
> +        raise_exception_err_ra(env, EXCP06_ILLOP, 0, GETPC());
> +        return;
> +    }

The document I have says #GP for this case, not #UD.

> +    if (env->regs[R_ECX] != 0) {
> +        raise_exception_err_ra(env, EXCP0D_GPF, 0, GETPC());
> +        return;
> +    }

In 64-bit mode, ecx 63:32 are ignored.

> +
> +    env->regs[R_EAX] = env->pkru;
> +    env->regs[R_EDX] = 0;
> +}

It would be better to return a 64-bit value, split and assign it to eax:edx in 
the translator, so that this function does not modify tcg registers.

> +
> +void helper_wrpkru(CPUX86State *env)
> +{
> +    CPUState *cs = CPU(x86_env_get_cpu(env));
> +
> +    if ((env->cr[4] & CR4_PKE_MASK) == 0) {
> +        raise_exception_err_ra(env, EXCP06_ILLOP, 0, GETPC());
> +        return;
> +    }

Similarly.

> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index b84ce3b..e2726d4 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -7262,6 +7262,16 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
>   #endif
>               gen_ldst_modrm(env, s, modrm, MO_16, OR_TMP0, 1);
>               break;
> +        case 0xee: /* rdpkru */
> +            gen_update_cc_op(s);
> +            gen_jmp_im(pc_start - s->cs_base);
> +            gen_helper_rdpkru(cpu_env);
> +            break;

No need for either update_cc_op or gen_jmp_im, now that we've got 
raise_exception_err_ra.

Missing check for lock prefix.

If you make the change to return a 64-bit value, this would look like

   gen_helper_rdpkru(cpu_tmp1_i64, cpu_env);
   tcg_gen_extr_i64_tl(cpu_regs[REG_EAX], cpu_regs[REG_EDX], cpu_tmp1_i64);


> +        case 0xef: /* wrpkru */
> +            gen_update_cc_op(s);
> +            gen_jmp_im(pc_start - s->cs_base);
> +            gen_helper_wrpkru(cpu_env);
> +            break;
>           CASE_MEM_OP(6): /* lmsw */
>               if (s->cpl != 0) {
>                   gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
>



r~
Paolo Bonzini Feb. 17, 2016, 10:33 a.m. UTC | #2 
On 09/02/2016 19:13, Richard Henderson wrote:
>>
>> +{
>> +    if ((env->cr[4] & CR4_PKE_MASK) == 0) {
>> +        raise_exception_err_ra(env, EXCP06_ILLOP, 0, GETPC());
>> +        return;
>> +    }
> 
> The document I have says #GP for this case, not #UD.

The text says #GP, the "Protected Mode Exceptions" section says #UD. :(

I think #UD is more likely, as it's similar to XSAVE.

> If you make the change to return a 64-bit value, this would look like
> 
>   gen_helper_rdpkru(cpu_tmp1_i64, cpu_env);
>   tcg_gen_extr_i64_tl(cpu_regs[REG_EAX], cpu_regs[REG_EDX], cpu_tmp1_i64); 

Yup, I can steal all that's needed from xgetbv and xsetbv.

Paolo
diff mbox

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b500419..267a492 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -363,7 +363,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_7_0_ECX_FEATURES (CPUID_7_0_ECX_PKU | CPUID_7_0_ECX_OSPKE)
 #define TCG_APM_FEATURES 0
 #define TCG_6_EAX_FEATURES CPUID_6_EAX_ARAT
 
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index e32df8a..d932293 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -157,6 +157,7 @@ 
 #define HF_SMAP_SHIFT       23 /* CR4.SMAP */
 #define HF_IOBPT_SHIFT      24 /* an io breakpoint enabled */
 #define HF_OSXSAVE_SHIFT    25 /* CR4.OSXSAVE */
+#define HF_PKE_SHIFT        26 /* CR4.PKE enabled */
 
 #define HF_CPL_MASK          (3 << HF_CPL_SHIFT)
 #define HF_SOFTMMU_MASK      (1 << HF_SOFTMMU_SHIFT)
@@ -182,6 +183,7 @@ 
 #define HF_SMAP_MASK         (1 << HF_SMAP_SHIFT)
 #define HF_IOBPT_MASK        (1 << HF_IOBPT_SHIFT)
 #define HF_OSXSAVE_MASK      (1 << HF_OSXSAVE_SHIFT)
+#define HF_PKE_MASK          (1 << HF_PKE_SHIFT)
 
 /* hflags2 */
 
@@ -229,6 +231,7 @@ 
 #define CR4_OSXSAVE_MASK (1U << 18)
 #define CR4_SMEP_MASK   (1U << 20)
 #define CR4_SMAP_MASK   (1U << 21)
+#define CR4_PKE_MASK   (1U << 22)
 
 #define DR6_BD          (1 << 13)
 #define DR6_BS          (1 << 14)
@@ -257,6 +260,7 @@ 
 #define PG_PSE_BIT      7
 #define PG_GLOBAL_BIT   8
 #define PG_PSE_PAT_BIT  12
+#define PG_PKRU_BIT     59
 #define PG_NX_BIT       63
 
 #define PG_PRESENT_MASK  (1 << PG_PRESENT_BIT)
@@ -272,7 +276,8 @@ 
 #define PG_ADDRESS_MASK  0x000ffffffffff000LL
 #define PG_HI_RSVD_MASK  (PG_ADDRESS_MASK & ~PHYS_ADDR_MASK)
 #define PG_HI_USER_MASK  0x7ff0000000000000LL
-#define PG_NX_MASK       (1LL << PG_NX_BIT)
+#define PG_PKRU_MASK     (15ULL << PG_PKRU_BIT)
+#define PG_NX_MASK       (1ULL << PG_NX_BIT)
 
 #define PG_ERROR_W_BIT     1
 
@@ -281,6 +286,7 @@ 
 #define PG_ERROR_U_MASK    0x04
 #define PG_ERROR_RSVD_MASK 0x08
 #define PG_ERROR_I_D_MASK  0x10
+#define PG_ERROR_PK_MASK   0x20
 
 #define MCG_CTL_P       (1ULL<<8)   /* MCG_CAP register available */
 #define MCG_SER_P       (1ULL<<24) /* MCA recovery/new status bits */
diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c
index a8512ea..f0f14f4 100644
--- a/target-i386/fpu_helper.c
+++ b/target-i386/fpu_helper.c
@@ -1169,6 +1169,11 @@  static void do_xsave_sse(CPUX86State *env, target_ulong ptr, uintptr_t retaddr)
     }
 }
 
+static void do_xsave_pkru(CPUX86State *env, target_ulong ptr, uintptr_t retaddr)
+{
+    cpu_stq_data_ra(env, ptr + 0xA80, env->pkru, retaddr);
+}
+
 void helper_fxsave(CPUX86State *env, target_ulong ptr)
 {
     /* The operand must be 16 byte aligned */
@@ -1191,9 +1196,18 @@  void helper_fxsave(CPUX86State *env, target_ulong ptr)
 
 static uint64_t get_xinuse(CPUX86State *env)
 {
-    /* We don't track XINUSE.  We could calculate it here, but it's
-       probably less work to simply indicate all components in use.  */
-    return -1;
+    uint64_t inuse = -1;
+
+    /* For the most part, we don't track XINUSE.  We could calculate it
+     * here for all components, but it's probably less work to simply
+     * indicate in use.  That said, for state that is important
+     * enough to track in HFLAGS, we might as well use that here.
+     */
+    if ((env->hflags & HF_PKE_MASK) == 0) {
+       inuse &= ~XSTATE_PKRU;
+    }
+    return inuse;
+}
 }
 
 static void do_xsave(CPUX86State *env, target_ulong ptr,
@@ -1220,6 +1248,9 @@  static void do_xsave(CPUX86State *env, target_ulong ptr,
     if (opt & XSTATE_SSE) {
         do_xsave_sse(env, ptr, retaddr);
     }
+    if (opt & XSTATE_PKRU) {
+        do_xsave_pkru(env, ptr, retaddr);
+    }
 
     /* Update the XSTATE_BV field.  */
     old_bv = cpu_ldq_data_ra(env, ptr + 512, retaddr);
@@ -1285,6 +1317,16 @@  static void do_xrstor_sse(CPUX86State *env, target_ulong ptr, uintptr_t retaddr)
     }
 }
 
+static void do_xrstor_pkru(CPUX86State *env, target_ulong ptr, uintptr_t retaddr)
+{
+    uint64_t old_pkru = env->pkru;
+    env->pkru = cpu_ldq_data_ra(env, ptr + 0xA80, retaddr);
+    if (env->pkru != old_pkru) {
+        CPUState *cs = CPU(x86_env_get_cpu(env));
+        tlb_flush(cs, 1);
+    }
+}
+
 void helper_fxrstor(CPUX86State *env, target_ulong ptr)
 {
     /* The operand must be 16 byte aligned */
@@ -1357,6 +1399,10 @@  void helper_xrstor(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
             memset(env->xmm_regs, 0, sizeof(env->xmm_regs));
         }
     }
+
+    if (rfbm & XSTATE_PKRU) {
+        do_xrstor_pkru(env, ptr, GETPC());
+    }
 }
 
 uint64_t helper_xgetbv(CPUX86State *env, uint32_t ecx)
diff --git a/target-i386/helper.c b/target-i386/helper.c
index f5f0bec..a55628f 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -689,6 +689,16 @@  void cpu_x86_update_cr4(CPUX86State *env, uint32_t new_cr4)
         hflags |= HF_SMAP_MASK;
     }
 
+    if (!(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_PKU)) {
+        new_cr4 &= ~CR4_PKE_MASK;
+    }
+    env->hflags &= ~HF_PKE_MASK;
+    env->features[FEAT_7_0_ECX] &= ~CPUID_7_0_ECX_OSPKE;
+    if (new_cr4 & CR4_PKE_MASK) {
+        env->hflags |= HF_PKE_MASK;
+	env->features[FEAT_7_0_ECX] |= CPUID_7_0_ECX_OSPKE;
+    }
+
     env->cr[4] = new_cr4;
     env->hflags = hflags;
     return;
@@ -935,6 +945,24 @@  do_check_protect_pse36:
         goto do_fault_protect;
     }
 
+    if ((env->cr[4] & CR4_PKE_MASK) && (env->hflags & HF_LMA_MASK) &&
+        (ptep & PG_USER_MASK) && env->pkru) {
+        uint32_t pk = (pte & PG_PKRU_MASK) >> PG_PKRU_BIT;
+        uint32_t pkru_ad = (env->pkru >> pk * 2) & 1;
+        uint32_t pkru_wd = (env->pkru >> pk * 2) & 2;
+
+        if (pkru_ad) {
+            prot &= ~(PAGE_READ | PAGE_WRITE);
+        } else if (pkru_wd && (is_user || env->cr[0] & CR0_WP_MASK)) {
+            prot &= ~PAGE_WRITE;
+        }
+        if ((prot & (1 << is_write1)) == 0) {
+            assert(is_write1 != 2);
+            error_code |= PG_ERROR_PK_MASK;
+            goto do_fault_protect;
+        }
+    }
+
     /* yes, it can! */
     is_dirty = is_write && !(pte & PG_DIRTY_MASK);
     if (!(pte & PG_ACCESSED_MASK) || is_dirty) {
diff --git a/target-i386/helper.h b/target-i386/helper.h
index 9a83955..89af181 100644
--- a/target-i386/helper.h
+++ b/target-i386/helper.h
@@ -79,6 +79,8 @@  DEF_HELPER_1(rdtscp, void, env)
 DEF_HELPER_1(rdpmc, void, env)
 DEF_HELPER_1(rdmsr, void, env)
 DEF_HELPER_1(wrmsr, void, env)
+DEF_HELPER_1(rdpkru, void, env)
+DEF_HELPER_1(wrpkru, void, env)
 
 DEF_HELPER_2(check_iob, void, env, i32)
 DEF_HELPER_2(check_iow, void, env, i32)
diff --git a/target-i386/misc_helper.c b/target-i386/misc_helper.c
index 460257f..b517559 100644
--- a/target-i386/misc_helper.c
+++ b/target-i386/misc_helper.c
@@ -600,3 +600,35 @@  void helper_debug(CPUX86State *env)
     cs->exception_index = EXCP_DEBUG;
     cpu_loop_exit(cs);
 }
+
+void helper_rdpkru(CPUX86State *env)
+{
+    if ((env->cr[4] & CR4_PKE_MASK) == 0) {
+        raise_exception_err_ra(env, EXCP06_ILLOP, 0, GETPC());
+        return;
+    }
+    if (env->regs[R_ECX] != 0) {
+        raise_exception_err_ra(env, EXCP0D_GPF, 0, GETPC());
+        return;
+    }
+
+    env->regs[R_EAX] = env->pkru;
+    env->regs[R_EDX] = 0;
+}
+
+void helper_wrpkru(CPUX86State *env)
+{
+    CPUState *cs = CPU(x86_env_get_cpu(env));
+
+    if ((env->cr[4] & CR4_PKE_MASK) == 0) {
+        raise_exception_err_ra(env, EXCP06_ILLOP, 0, GETPC());
+        return;
+    }
+    if (env->regs[R_ECX] != 0) {
+        raise_exception_err_ra(env, EXCP0D_GPF, 0, GETPC());
+        return;
+    }
+
+    env->pkru = env->regs[R_EAX];
+    tlb_flush(cs, 1);
+}
diff --git a/target-i386/translate.c b/target-i386/translate.c
index b84ce3b..e2726d4 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7262,6 +7262,16 @@  static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
 #endif
             gen_ldst_modrm(env, s, modrm, MO_16, OR_TMP0, 1);
             break;
+        case 0xee: /* rdpkru */
+            gen_update_cc_op(s);
+            gen_jmp_im(pc_start - s->cs_base);
+            gen_helper_rdpkru(cpu_env);
+            break;
+        case 0xef: /* wrpkru */
+            gen_update_cc_op(s);
+            gen_jmp_im(pc_start - s->cs_base);
+            gen_helper_wrpkru(cpu_env);
+            break;
         CASE_MEM_OP(6): /* lmsw */
             if (s->cpl != 0) {
                 gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);