| Message ID | 20220829021325.154978-1-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/sh4: Fix TB_FLAG_UNALIGN | expand |
On 8/28/22 19:13, Richard Henderson wrote: > The value previously chosen overlaps GUSA_MASK. ... which meant that we didn't translate the gusa sequence into an atomic operation, which meant the multi-threaded tests fail. > > Cc: qemu-stable@nongnu.org > Fixes: 4da06fb3062 ("target/sh4: Implement prctl_unalign_sigbus") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/856 > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/sh4/cpu.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/sh4/cpu.h b/target/sh4/cpu.h > index 9f15ef913c..e79cbc59e2 100644 > --- a/target/sh4/cpu.h > +++ b/target/sh4/cpu.h > @@ -84,7 +84,7 @@ > #define DELAY_SLOT_RTE (1 << 2) > > #define TB_FLAG_PENDING_MOVCA (1 << 3) > -#define TB_FLAG_UNALIGN (1 << 4) > +#define TB_FLAG_UNALIGN (1 << 13) > > #define GUSA_SHIFT 4 > #ifdef CONFIG_USER_ONLY
On Sun, 28 Aug 2022, Richard Henderson wrote: > The value previously chosen overlaps GUSA_MASK. > > Cc: qemu-stable@nongnu.org > Fixes: 4da06fb3062 ("target/sh4: Implement prctl_unalign_sigbus") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/856 > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/sh4/cpu.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/sh4/cpu.h b/target/sh4/cpu.h > index 9f15ef913c..e79cbc59e2 100644 > --- a/target/sh4/cpu.h > +++ b/target/sh4/cpu.h > @@ -84,7 +84,7 @@ > #define DELAY_SLOT_RTE (1 << 2) > > #define TB_FLAG_PENDING_MOVCA (1 << 3) > -#define TB_FLAG_UNALIGN (1 << 4) > +#define TB_FLAG_UNALIGN (1 << 13) Is it worth a comment to note why that value to avoid the same problem if another flag is added in the future? Regards, BALATON Zoltan > > #define GUSA_SHIFT 4 > #ifdef CONFIG_USER_ONLY >
On 8/29/22 02:05, BALATON Zoltan wrote: > On Sun, 28 Aug 2022, Richard Henderson wrote: >> The value previously chosen overlaps GUSA_MASK. >> >> Cc: qemu-stable@nongnu.org >> Fixes: 4da06fb3062 ("target/sh4: Implement prctl_unalign_sigbus") >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/856 >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/sh4/cpu.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/target/sh4/cpu.h b/target/sh4/cpu.h >> index 9f15ef913c..e79cbc59e2 100644 >> --- a/target/sh4/cpu.h >> +++ b/target/sh4/cpu.h >> @@ -84,7 +84,7 @@ >> #define DELAY_SLOT_RTE (1 << 2) >> >> #define TB_FLAG_PENDING_MOVCA (1 << 3) >> -#define TB_FLAG_UNALIGN (1 << 4) >> +#define TB_FLAG_UNALIGN (1 << 13) > > Is it worth a comment to note why that value to avoid the same problem if another flag is > added in the future? Hmm, or perhaps move it down below, so that we see bit 3 used, then bits 4-12, then bit 13. r~
On Tue, 30 Aug 2022 01:10:29 +0900, Richard Henderson wrote: > > On 8/29/22 02:05, BALATON Zoltan wrote: > > On Sun, 28 Aug 2022, Richard Henderson wrote: > >> The value previously chosen overlaps GUSA_MASK. > >> > >> Cc: qemu-stable@nongnu.org > >> Fixes: 4da06fb3062 ("target/sh4: Implement prctl_unalign_sigbus") > >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/856 > >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > >> --- > >> target/sh4/cpu.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/target/sh4/cpu.h b/target/sh4/cpu.h > >> index 9f15ef913c..e79cbc59e2 100644 > >> --- a/target/sh4/cpu.h > >> +++ b/target/sh4/cpu.h > >> @@ -84,7 +84,7 @@ > >> #define DELAY_SLOT_RTE (1 << 2) > >> > >> #define TB_FLAG_PENDING_MOVCA (1 << 3) > >> -#define TB_FLAG_UNALIGN (1 << 4) > >> +#define TB_FLAG_UNALIGN (1 << 13) > > > > Is it worth a comment to note why that value to avoid the same > > problem if another flag is added in the future? > > Hmm, or perhaps move it down below, so that we see bit 3 used, then bits 4-12, then bit 13. > > > r~ It looks like the gUSA and unalign access flags are mixed. I think the flags should also be separated as the two features are not related.
On Tue, 30 Aug 2022 01:10:29 +0900, Richard Henderson wrote: > > On 8/29/22 02:05, BALATON Zoltan wrote: > > On Sun, 28 Aug 2022, Richard Henderson wrote: > >> The value previously chosen overlaps GUSA_MASK. > >> > >> Cc: qemu-stable@nongnu.org > >> Fixes: 4da06fb3062 ("target/sh4: Implement prctl_unalign_sigbus") > >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/856 > >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > >> --- > >> target/sh4/cpu.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/target/sh4/cpu.h b/target/sh4/cpu.h > >> index 9f15ef913c..e79cbc59e2 100644 > >> --- a/target/sh4/cpu.h > >> +++ b/target/sh4/cpu.h > >> @@ -84,7 +84,7 @@ > >> #define DELAY_SLOT_RTE (1 << 2) > >> > >> #define TB_FLAG_PENDING_MOVCA (1 << 3) > >> -#define TB_FLAG_UNALIGN (1 << 4) > >> +#define TB_FLAG_UNALIGN (1 << 13) > > > > Is it worth a comment to note why that value to avoid the same > > problem if another flag is added in the future? > > Hmm, or perhaps move it down below, so that we see bit 3 used, then bits 4-12, then bit 13. > > > r~ How about this fix? From 69fc46c0e439026cabedc8ddfa0a880d0df09a6b Mon Sep 17 00:00:00 2001 From: Yoshinori Sato <ysato@users.sourceforge.jp> Date: Wed, 31 Aug 2022 17:12:59 +0900 Subject: [PATCH] sh4: cleanup for flags definition. Fix conflict TB_FLAG_UNALIGN and GUSA field. Add comment for gUSA operations. Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp> --- target/sh4/cpu.h | 9 +++++++-- target/sh4/translate.c | 5 ++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/target/sh4/cpu.h b/target/sh4/cpu.h index 9f15ef913c..91810fda9b 100644 --- a/target/sh4/cpu.h +++ b/target/sh4/cpu.h @@ -86,9 +86,14 @@ #define TB_FLAG_PENDING_MOVCA (1 << 3) #define TB_FLAG_UNALIGN (1 << 4) -#define GUSA_SHIFT 4 #ifdef CONFIG_USER_ONLY -#define GUSA_EXCLUSIVE (1 << 12) +/* gUSA information field in CPUArchState.flags */ +/* + b16 - b23: Exclusive region range (negative) + b24: pc in exclusive region flag (use normal decode) +*/ +#define GUSA_SHIFT 16 +#define GUSA_EXCLUSIVE (1 << 24) #define GUSA_MASK ((0xff << GUSA_SHIFT) | GUSA_EXCLUSIVE) #else /* Provide dummy versions of the above to allow tests against tbflags diff --git a/target/sh4/translate.c b/target/sh4/translate.c index f1b190e7cf..1d79a0721b 100644 --- a/target/sh4/translate.c +++ b/target/sh4/translate.c @@ -516,7 +516,7 @@ static void _decode_opc(DisasContext * ctx) /* Detect the start of a gUSA region. If so, update envflags and end the TB. This will allow us to see the end of the region (stored in R0) in the next TB. */ - if (B11_8 == 15 && B7_0s < 0 && + if (B11_8 == 15 && B7_0s < 0 && /* mov #-xxx, r15 */ (tb_cflags(ctx->base.tb) & CF_PARALLEL)) { ctx->envflags = deposit32(ctx->envflags, GUSA_SHIFT, 8, B7_0s); ctx->base.is_jmp = DISAS_STOP; @@ -2267,7 +2267,9 @@ static void sh4_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) (tbflags & (1 << SR_RB))) * 0x10; ctx->fbank = tbflags & FPSCR_FR ? 0x10 : 0; +#ifdef CONFIG_USER_ONLY if (tbflags & GUSA_MASK) { + /* In gUSA exclusive region */ uint32_t pc = ctx->base.pc_next; uint32_t pc_end = ctx->base.tb->cs_base; int backup = sextract32(ctx->tbflags, GUSA_SHIFT, 8); @@ -2285,6 +2287,7 @@ static void sh4_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) return; } } +#endif /* Since the ISA is fixed-width, we can bound by the number of instructions remaining on the page. */
On 8/30/22 18:30, Yoshinori Sato wrote: > On Tue, 30 Aug 2022 01:10:29 +0900, > Richard Henderson wrote: >> >> On 8/29/22 02:05, BALATON Zoltan wrote: >>> On Sun, 28 Aug 2022, Richard Henderson wrote: >>>> The value previously chosen overlaps GUSA_MASK. >>>> >>>> Cc: qemu-stable@nongnu.org >>>> Fixes: 4da06fb3062 ("target/sh4: Implement prctl_unalign_sigbus") >>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/856 >>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>>> --- >>>> target/sh4/cpu.h | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/target/sh4/cpu.h b/target/sh4/cpu.h >>>> index 9f15ef913c..e79cbc59e2 100644 >>>> --- a/target/sh4/cpu.h >>>> +++ b/target/sh4/cpu.h >>>> @@ -84,7 +84,7 @@ >>>> #define DELAY_SLOT_RTE (1 << 2) >>>> >>>> #define TB_FLAG_PENDING_MOVCA (1 << 3) >>>> -#define TB_FLAG_UNALIGN (1 << 4) >>>> +#define TB_FLAG_UNALIGN (1 << 13) >>> >>> Is it worth a comment to note why that value to avoid the same >>> problem if another flag is added in the future? >> >> Hmm, or perhaps move it down below, so that we see bit 3 used, then bits 4-12, then bit 13. >> >> >> r~ > > It looks like the gUSA and unalign access flags are mixed. > I think the flags should also be separated as the two features are not related. Well, of course. That's what the first patch is fixing. Balaton is merely discussing the order in which the bits are defined. r~
On 8/31/22 01:30, Yoshinori Sato wrote: > +/* gUSA information field in CPUArchState.flags */ > +/* > + b16 - b23: Exclusive region range (negative) > + b24: pc in exclusive region flag (use normal decode) > +*/ > +#define GUSA_SHIFT 16 > +#define GUSA_EXCLUSIVE (1 << 24) No good. These now overlap *flags = env->flags /* TB_FLAG_ENVFLAGS_MASK: bits 0-2, 4-12 */ | (env->fpscr & (FPSCR_FR | FPSCR_SZ | FPSCR_PR)) /* Bits 19-21 */ the fpscr bits. r~
diff --git a/target/sh4/cpu.h b/target/sh4/cpu.h index 9f15ef913c..e79cbc59e2 100644 --- a/target/sh4/cpu.h +++ b/target/sh4/cpu.h @@ -84,7 +84,7 @@ #define DELAY_SLOT_RTE (1 << 2) #define TB_FLAG_PENDING_MOVCA (1 << 3) -#define TB_FLAG_UNALIGN (1 << 4) +#define TB_FLAG_UNALIGN (1 << 13) #define GUSA_SHIFT 4 #ifdef CONFIG_USER_ONLY
The value previously chosen overlaps GUSA_MASK. Cc: qemu-stable@nongnu.org Fixes: 4da06fb3062 ("target/sh4: Implement prctl_unalign_sigbus") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/856 Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/sh4/cpu.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)