Re: [Qemu-devel] [PATCH 5/5] target-i386: implement PKE for TCG

[Richard Henderson]

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~