[PATCH] target/sh4: Fix TB_FLAG_UNALIGN

[PATCH] target/sh4: Fix TB_FLAG_UNALIGN

[Richard Henderson]

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)
 
 #define GUSA_SHIFT             4
 #ifdef CONFIG_USER_ONLY
-- 
2.34.1

Re: [PATCH] target/sh4: Fix TB_FLAG_UNALIGN

[Richard Henderson]

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

Re: [PATCH] target/sh4: Fix TB_FLAG_UNALIGN

[BALATON Zoltan]

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
>

Re: [PATCH] target/sh4: Fix TB_FLAG_UNALIGN

[Richard Henderson]

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~

Re: [PATCH] target/sh4: Fix TB_FLAG_UNALIGN

[Yoshinori Sato]

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.

-- 
Yosinori Sato

Re: [PATCH] target/sh4: Fix TB_FLAG_UNALIGN

[Richard Henderson]

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~

Re: [PATCH] target/sh4: Fix TB_FLAG_UNALIGN

[Yoshinori Sato]

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.  */
-- 
2.30.2

-- 
Yosinori Sato

Re: [PATCH] target/sh4: Fix TB_FLAG_UNALIGN

[Richard Henderson]

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~