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~
