* [Qemu-devel] [PATCH] target-i386: fix confusion in xcr0 bit position vs. mask
@ 2016-02-22 10:19 Paolo Bonzini
2016-02-22 19:14 ` Richard Henderson
0 siblings, 1 reply; 5+ messages in thread
From: Paolo Bonzini @ 2016-02-22 10:19 UTC (permalink / raw)
To: qemu-devel; +Cc: ehabkost, rth
The xsave and xrstor helpers are accessing the x86_ext_save_areas array
using a bit mask instead of a bit position. Provide two sets of XSTATE_*
definitions and use XSTATE_*_BIT when a bit position is requested.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target-i386/cpu.c | 29 ++++++++++++++++++-----------
target-i386/cpu.h | 29 +++++++++++++++++++----------
target-i386/fpu_helper.c | 33 +++++++++++++++++----------------
target-i386/mpx_helper.c | 2 +-
4 files changed, 55 insertions(+), 38 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0af43a3..912a376 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -473,19 +473,26 @@ static const X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = {
#undef REGISTER
const ExtSaveArea x86_ext_save_areas[] = {
- [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX,
+ [XSTATE_YMM_BIT] =
+ { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX,
.offset = 0x240, .size = 0x100 },
- [3] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX,
+ [XSTATE_BNDREGS_BIT] =
+ { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX,
.offset = 0x3c0, .size = 0x40 },
- [4] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX,
+ [XSTATE_BNDCSR_BIT] =
+ { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX,
.offset = 0x400, .size = 0x40 },
- [5] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_AVX512F,
+ [XSTATE_OPMASK_BIT] =
+ { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_AVX512F,
.offset = 0x440, .size = 0x40 },
- [6] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_AVX512F,
+ [XSTATE_ZMM_Hi256_BIT] =
+ { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_AVX512F,
.offset = 0x480, .size = 0x200 },
- [7] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_AVX512F,
+ [XSTATE_Hi16_ZMM_BIT] =
+ { .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,
+ [XSTATE_PKRU_BIT] =
+ { .feature = FEAT_7_0_ECX, .bits = CPUID_7_0_ECX_PKU,
.offset = 0xA80, .size = 0x8 },
};
@@ -2483,7 +2490,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
*ecx = MAX(*ecx, esa->offset + esa->size);
}
}
- *eax |= ena_mask & (XSTATE_FP | XSTATE_SSE);
+ *eax |= ena_mask & (XSTATE_FP_MASK | XSTATE_SSE_MASK);
*ebx = *ecx;
} else if (count == 1) {
*eax = env->features[FEAT_XSAVE];
@@ -2717,15 +2724,15 @@ static void x86_cpu_reset(CPUState *s)
cpu_watchpoint_remove_all(s, BP_CPU);
cr4 = 0;
- xcr0 = XSTATE_FP;
+ xcr0 = XSTATE_FP_MASK;
#ifdef CONFIG_USER_ONLY
/* Enable all the features for user-mode. */
if (env->features[FEAT_1_EDX] & CPUID_SSE) {
- xcr0 |= XSTATE_SSE;
+ xcr0 |= XSTATE_SSE_MASK;
}
if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_MPX) {
- xcr0 |= XSTATE_BNDREGS | XSTATE_BNDCSR;
+ xcr0 |= XSTATE_BNDREGS_MASK | XSTATE_BNDCSR_MASK;
}
if (env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) {
cr4 |= CR4_OSFXSR_MASK | CR4_OSXSAVE_MASK;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 94cb4db..03c00d5 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -405,16 +405,25 @@
#define MSR_IA32_BNDCFGS 0x00000d90
#define MSR_IA32_XSS 0x00000da0
-#define XSTATE_FP (1ULL << 0)
-#define XSTATE_SSE (1ULL << 1)
-#define XSTATE_YMM (1ULL << 2)
-#define XSTATE_BNDREGS (1ULL << 3)
-#define XSTATE_BNDCSR (1ULL << 4)
-#define XSTATE_OPMASK (1ULL << 5)
-#define XSTATE_ZMM_Hi256 (1ULL << 6)
-#define XSTATE_Hi16_ZMM (1ULL << 7)
-#define XSTATE_PKRU (1ULL << 9)
-
+#define XSTATE_FP_BIT 0
+#define XSTATE_SSE_BIT 1
+#define XSTATE_YMM_BIT 2
+#define XSTATE_BNDREGS_BIT 3
+#define XSTATE_BNDCSR_BIT 4
+#define XSTATE_OPMASK_BIT 5
+#define XSTATE_ZMM_Hi256_BIT 6
+#define XSTATE_Hi16_ZMM_BIT 7
+#define XSTATE_PKRU_BIT 9
+
+#define XSTATE_FP_MASK (1ULL << XSTATE_FP_BIT)
+#define XSTATE_SSE_MASK (1ULL << XSTATE_SSE_BIT)
+#define XSTATE_YMM_MASK (1ULL << XSTATE_YMM_BIT)
+#define XSTATE_BNDREGS_MASK (1ULL << XSTATE_BNDREGS_BIT)
+#define XSTATE_BNDCSR_MASK (1ULL << XSTATE_BNDCSR_BIT)
+#define XSTATE_OPMASK_MASK (1ULL << XSTATE_OPMASK_BIT)
+#define XSTATE_ZMM_Hi256_MASK (1ULL << XSTATE_ZMM_Hi256_BIT)
+#define XSTATE_Hi16_ZMM_MASK (1ULL << XSTATE_Hi16_ZMM_BIT)
+#define XSTATE_PKRU_MASK (1ULL << XSTATE_PKRU_BIT)
/* CPUID feature words */
typedef enum FeatureWord {
diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c
index 9dfbc4c..80e9bc9 100644
--- a/target-i386/fpu_helper.c
+++ b/target-i386/fpu_helper.c
@@ -1215,7 +1215,7 @@ static uint64_t get_xinuse(CPUX86State *env)
indicate in use. That said, the state of BNDREGS is important
enough to track in HFLAGS, so we might as well use that here. */
if ((env->hflags & HF_MPX_IU_MASK) == 0) {
- inuse &= ~XSTATE_BNDREGS;
+ inuse &= ~XSTATE_BNDREGS_MASK;
}
return inuse;
}
@@ -1239,22 +1239,22 @@ static void do_xsave(CPUX86State *env, target_ulong ptr, uint64_t rfbm,
rfbm &= env->xcr0;
opt &= rfbm;
- if (opt & XSTATE_FP) {
+ if (opt & XSTATE_FP_MASK) {
do_xsave_fpu(env, ptr, ra);
}
- if (rfbm & XSTATE_SSE) {
+ if (rfbm & XSTATE_SSE_MASK) {
/* Note that saving MXCSR is not suppressed by XSAVEOPT. */
do_xsave_mxcsr(env, ptr, ra);
}
- if (opt & XSTATE_SSE) {
+ if (opt & XSTATE_SSE_MASK) {
do_xsave_sse(env, ptr, ra);
}
- if (opt & XSTATE_BNDREGS) {
- target_ulong off = x86_ext_save_areas[XSTATE_BNDREGS].offset;
+ if (opt & XSTATE_BNDREGS_MASK) {
+ target_ulong off = x86_ext_save_areas[XSTATE_BNDREGS_BIT].offset;
do_xsave_bndregs(env, ptr + off, ra);
}
- if (opt & XSTATE_BNDCSR) {
- target_ulong off = x86_ext_save_areas[XSTATE_BNDCSR].offset;
+ if (opt & XSTATE_BNDCSR_MASK) {
+ target_ulong off = x86_ext_save_areas[XSTATE_BNDCSR_BIT].offset;
do_xsave_bndcsr(env, ptr + off, ra);
}
@@ -1419,9 +1419,9 @@ void helper_xrstor(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
memset(env->xmm_regs, 0, sizeof(env->xmm_regs));
}
}
- if (rfbm & XSTATE_BNDREGS) {
- if (xstate_bv & XSTATE_BNDREGS) {
- target_ulong off = x86_ext_save_areas[XSTATE_BNDREGS].offset;
+ if (rfbm & XSTATE_BNDREGS_MASK) {
+ if (xstate_bv & XSTATE_BNDREGS_MASK) {
+ target_ulong off = x86_ext_save_areas[XSTATE_BNDREGS_BIT].offset;
do_xrstor_bndregs(env, ptr + off, ra);
env->hflags |= HF_MPX_IU_MASK;
} else {
@@ -1429,9 +1429,9 @@ void helper_xrstor(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
env->hflags &= ~HF_MPX_IU_MASK;
}
}
- if (rfbm & XSTATE_BNDCSR) {
- if (xstate_bv & XSTATE_BNDCSR) {
- target_ulong off = x86_ext_save_areas[XSTATE_BNDCSR].offset;
+ if (rfbm & XSTATE_BNDCSR_MASK) {
+ if (xstate_bv & XSTATE_BNDCSR_MASK) {
+ target_ulong off = x86_ext_save_areas[XSTATE_BNDCSR_BIT].offset;
do_xrstor_bndcsr(env, ptr + off, ra);
} else {
memset(&env->bndcs_regs, 0, sizeof(env->bndcs_regs));
@@ -1470,7 +1470,7 @@ void helper_xsetbv(CPUX86State *env, uint32_t ecx, uint64_t mask)
}
/* Only XCR0 is defined at present; the FPU may not be disabled. */
- if (ecx != 0 || (mask & XSTATE_FP) == 0) {
+ if (ecx != 0 || (mask & XSTATE_FP_MASK) == 0) {
goto do_gpf;
}
@@ -1482,7 +1482,8 @@ void helper_xsetbv(CPUX86State *env, uint32_t ecx, uint64_t mask)
}
/* Disallow enabling only half of MPX. */
- if ((mask ^ (mask * (XSTATE_BNDCSR / XSTATE_BNDREGS))) & XSTATE_BNDCSR) {
+ if ((mask ^ (mask * (XSTATE_BNDCSR_MASK / XSTATE_BNDREGS_MASK)))
+ & XSTATE_BNDCSR_MASK) {
goto do_gpf;
}
diff --git a/target-i386/mpx_helper.c b/target-i386/mpx_helper.c
index 1bf717a..8c6c2e7 100644
--- a/target-i386/mpx_helper.c
+++ b/target-i386/mpx_helper.c
@@ -35,7 +35,7 @@ void cpu_sync_bndcs_hflags(CPUX86State *env)
}
if ((env->cr[4] & CR4_OSXSAVE_MASK)
- && (env->xcr0 & XSTATE_BNDCSR)
+ && (env->xcr0 & XSTATE_BNDCSR_MASK)
&& (bndcsr & BNDCFG_ENABLE)) {
hflags |= HF_MPX_EN_MASK;
} else {
--
2.5.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: fix confusion in xcr0 bit position vs. mask
2016-02-22 10:19 [Qemu-devel] [PATCH] target-i386: fix confusion in xcr0 bit position vs. mask Paolo Bonzini
@ 2016-02-22 19:14 ` Richard Henderson
2016-02-22 21:56 ` Eduardo Habkost
0 siblings, 1 reply; 5+ messages in thread
From: Richard Henderson @ 2016-02-22 19:14 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: ehabkost
On 02/22/2016 02:19 AM, Paolo Bonzini wrote:
> The xsave and xrstor helpers are accessing the x86_ext_save_areas array
> using a bit mask instead of a bit position. Provide two sets of XSTATE_*
> definitions and use XSTATE_*_BIT when a bit position is requested.
Whoops. This patch is fine,
Reviewed-by: Richard Henderson <rth@twiddle.net>
Dosen't Eduardo have a pending patch set to make xsave use a struct? That
would fix this as well, by not requiring fpu_helper.c to use the
x86_ext_save_areas array at all...
r~
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: fix confusion in xcr0 bit position vs. mask
2016-02-22 19:14 ` Richard Henderson
@ 2016-02-22 21:56 ` Eduardo Habkost
2016-02-23 10:16 ` Paolo Bonzini
0 siblings, 1 reply; 5+ messages in thread
From: Eduardo Habkost @ 2016-02-22 21:56 UTC (permalink / raw)
To: Richard Henderson; +Cc: Paolo Bonzini, qemu-devel
On Mon, Feb 22, 2016 at 11:14:44AM -0800, Richard Henderson wrote:
> On 02/22/2016 02:19 AM, Paolo Bonzini wrote:
> > The xsave and xrstor helpers are accessing the x86_ext_save_areas array
> > using a bit mask instead of a bit position. Provide two sets of XSTATE_*
> > definitions and use XSTATE_*_BIT when a bit position is requested.
>
> Whoops. This patch is fine,
>
> Reviewed-by: Richard Henderson <rth@twiddle.net>
>
> Dosen't Eduardo have a pending patch set to make xsave use a struct? That
> would fix this as well, by not requiring fpu_helper.c to use the
> x86_ext_save_areas array at all...
That's true, but the bug fix have priority, so I will queue it
first.
--
Eduardo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: fix confusion in xcr0 bit position vs. mask
2016-02-22 21:56 ` Eduardo Habkost
@ 2016-02-23 10:16 ` Paolo Bonzini
2016-02-23 18:50 ` Eduardo Habkost
0 siblings, 1 reply; 5+ messages in thread
From: Paolo Bonzini @ 2016-02-23 10:16 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: qemu-devel, Richard Henderson
----- Original Message -----
> From: "Eduardo Habkost" <ehabkost@redhat.com>
> To: "Richard Henderson" <rth@twiddle.net>
> Cc: "Paolo Bonzini" <pbonzini@redhat.com>, qemu-devel@nongnu.org
> Sent: Monday, February 22, 2016 10:56:03 PM
> Subject: Re: [PATCH] target-i386: fix confusion in xcr0 bit position vs. mask
>
> On Mon, Feb 22, 2016 at 11:14:44AM -0800, Richard Henderson wrote:
> > On 02/22/2016 02:19 AM, Paolo Bonzini wrote:
> > > The xsave and xrstor helpers are accessing the x86_ext_save_areas array
> > > using a bit mask instead of a bit position. Provide two sets of XSTATE_*
> > > definitions and use XSTATE_*_BIT when a bit position is requested.
> >
> > Whoops. This patch is fine,
> >
> > Reviewed-by: Richard Henderson <rth@twiddle.net>
> >
> > Dosen't Eduardo have a pending patch set to make xsave use a struct? That
> > would fix this as well, by not requiring fpu_helper.c to use the
> > x86_ext_save_areas array at all...
>
> That's true, but the bug fix have priority, so I will queue it
> first.
Actually, I'm sending out a pull request today so I was thinking of
including this.
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: fix confusion in xcr0 bit position vs. mask
2016-02-23 10:16 ` Paolo Bonzini
@ 2016-02-23 18:50 ` Eduardo Habkost
0 siblings, 0 replies; 5+ messages in thread
From: Eduardo Habkost @ 2016-02-23 18:50 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Richard Henderson
On Tue, Feb 23, 2016 at 05:16:55AM -0500, Paolo Bonzini wrote:
>
>
> ----- Original Message -----
> > From: "Eduardo Habkost" <ehabkost@redhat.com>
> > To: "Richard Henderson" <rth@twiddle.net>
> > Cc: "Paolo Bonzini" <pbonzini@redhat.com>, qemu-devel@nongnu.org
> > Sent: Monday, February 22, 2016 10:56:03 PM
> > Subject: Re: [PATCH] target-i386: fix confusion in xcr0 bit position vs. mask
> >
> > On Mon, Feb 22, 2016 at 11:14:44AM -0800, Richard Henderson wrote:
> > > On 02/22/2016 02:19 AM, Paolo Bonzini wrote:
> > > > The xsave and xrstor helpers are accessing the x86_ext_save_areas array
> > > > using a bit mask instead of a bit position. Provide two sets of XSTATE_*
> > > > definitions and use XSTATE_*_BIT when a bit position is requested.
> > >
> > > Whoops. This patch is fine,
> > >
> > > Reviewed-by: Richard Henderson <rth@twiddle.net>
> > >
> > > Dosen't Eduardo have a pending patch set to make xsave use a struct? That
> > > would fix this as well, by not requiring fpu_helper.c to use the
> > > x86_ext_save_areas array at all...
> >
> > That's true, but the bug fix have priority, so I will queue it
> > first.
>
> Actually, I'm sending out a pull request today so I was thinking of
> including this.
No problem.
Acked-by: Eduardo Habkost <ehabkost@redhat.com>
--
Eduardo
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-02-23 18:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-22 10:19 [Qemu-devel] [PATCH] target-i386: fix confusion in xcr0 bit position vs. mask Paolo Bonzini
2016-02-22 19:14 ` Richard Henderson
2016-02-22 21:56 ` Eduardo Habkost
2016-02-23 10:16 ` Paolo Bonzini
2016-02-23 18:50 ` Eduardo Habkost
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).