[PATCH 0/3] target/mips: misc microMIPS fixes

[PATCH 0/3] target/mips: misc microMIPS fixes

[Marcin Nowakowski]

Marcin Nowakowski (3):
  target/mips: fix JALS32/J32 instruction handling for microMIPS
  target/mips: fix SWM32 handling for micromips
  target/mips: implement CP0.Config7.WII bit support

 target/mips/cpu-defs.c.inc    | 1 +
 target/mips/cpu.c             | 6 ++++--
 target/mips/cpu.h             | 1 +
 target/mips/tcg/ldst_helper.c | 4 ++--
 target/mips/tcg/translate.c   | 6 ++++++
 5 files changed, 14 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 1/3] target/mips: fix JALS32/J32 instruction handling for microMIPS

[Marcin Nowakowski]

microMIPS J & JAL instructions perform a jump in a 128MB region and 5
top bits of the address need to be preserved. This is different behavior
compared to standard mips systems, where the jump is executed within a
256MB region.
Note that microMIPS32 instruction set documentation appears to have
inconsistent information regarding JALX32 instruction - it is written in
the doc that:

"To execute a procedure call within the current 256 MB-aligned region
(...)
The low 26 bits of the target address is the target field shifted left
2 bits."

But the target address is already 26 bits. Moreover, the operation
description indicates that 28 bits are copied, so the statement about
use of 26 bits is _most likely_ incorrect and the corresponding code
remains the same as for standard mips instruction set.

Signed-off-by: Marcin Nowakowski <marcin.nowakowski@fungible.com>
---
 target/mips/tcg/translate.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
index 624e6b7786..5d46f24141 100644
--- a/target/mips/tcg/translate.c
+++ b/target/mips/tcg/translate.c
@@ -4860,6 +4860,7 @@ static void gen_compute_branch(DisasContext *ctx, uint32_t opc,
     target_ulong btgt = -1;
     int blink = 0;
     int bcond_compute = 0;
+    int jal_mask = 0;
     TCGv t0 = tcg_temp_new();
     TCGv t1 = tcg_temp_new();
 
@@ -4917,6 +4918,11 @@ static void gen_compute_branch(DisasContext *ctx, uint32_t opc,
         break;
     case OPC_J:
     case OPC_JAL:
+        /* Jump to immediate */
+        jal_mask = ctx->hflags & MIPS_HFLAG_M16 ? 0xF8000000 : 0xF0000000;
+        btgt = ((ctx->base.pc_next + insn_bytes) & jal_mask) |
+            (uint32_t)offset;
+        break;
     case OPC_JALX:
         /* Jump to immediate */
         btgt = ((ctx->base.pc_next + insn_bytes) & (int32_t)0xF0000000) |
-- 
2.25.1

[PATCH 2/3] target/mips: fix SWM32 handling for micromips

[Marcin Nowakowski]

SWM32 should store a sequence of 32-bit words from the GPRs, but it was
incorrectly coded to store 16-bit words only. As a result, an LWM32 that
usually follows would restore invalid register values.

Signed-off-by: Marcin Nowakowski <marcin.nowakowski@fungible.com>
---
 target/mips/tcg/ldst_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/mips/tcg/ldst_helper.c b/target/mips/tcg/ldst_helper.c
index d0bd0267b2..c1a8380e34 100644
--- a/target/mips/tcg/ldst_helper.c
+++ b/target/mips/tcg/ldst_helper.c
@@ -248,14 +248,14 @@ void helper_swm(CPUMIPSState *env, target_ulong addr, target_ulong reglist,
         target_ulong i;
 
         for (i = 0; i < base_reglist; i++) {
-            cpu_stw_mmuidx_ra(env, addr, env->active_tc.gpr[multiple_regs[i]],
+            cpu_stl_mmuidx_ra(env, addr, env->active_tc.gpr[multiple_regs[i]],
                               mem_idx, GETPC());
             addr += 4;
         }
     }
 
     if (do_r31) {
-        cpu_stw_mmuidx_ra(env, addr, env->active_tc.gpr[31], mem_idx, GETPC());
+        cpu_stl_mmuidx_ra(env, addr, env->active_tc.gpr[31], mem_idx, GETPC());
     }
 }
 
-- 
2.25.1

[PATCH 3/3] target/mips: implement CP0.Config7.WII bit support

[Marcin Nowakowski]

Some older cores use CP0.Config7.WII bit to indicate that a disabled
interrupt should wake up a sleeping CPU.
Enable this bit by default for M14Kc, which supports that. There are
potentially other cores that support this feature, but I do not have a
complete list.

Signed-off-by: Marcin Nowakowski <marcin.nowakowski@fungible.com>
---
 target/mips/cpu-defs.c.inc | 1 +
 target/mips/cpu.c          | 6 ++++--
 target/mips/cpu.h          | 1 +
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/target/mips/cpu-defs.c.inc b/target/mips/cpu-defs.c.inc
index 480e60aeec..57856e2e72 100644
--- a/target/mips/cpu-defs.c.inc
+++ b/target/mips/cpu-defs.c.inc
@@ -354,6 +354,7 @@ const mips_def_t mips_defs[] =
                        (0 << CP0C1_DS) | (3 << CP0C1_DL) | (1 << CP0C1_DA),
         .CP0_Config2 = MIPS_CONFIG2,
         .CP0_Config3 = MIPS_CONFIG3 | (0x2 << CP0C3_ISA) | (0 << CP0C3_VInt),
+        .CP0_Config7 = 0x1 << CP0C7_WII,
         .CP0_LLAddr_rw_bitmask = 0,
         .CP0_LLAddr_shift = 4,
         .SYNCI_Step = 32,
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 7a565466cb..7ba359696f 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -144,12 +144,14 @@ static bool mips_cpu_has_work(CPUState *cs)
     /*
      * Prior to MIPS Release 6 it is implementation dependent if non-enabled
      * interrupts wake-up the CPU, however most of the implementations only
-     * check for interrupts that can be taken.
+     * check for interrupts that can be taken. For pre-release 6 CPUs,
+     * check for CP0 Config7 'Wait IE ignore' bit.
      */
     if ((cs->interrupt_request & CPU_INTERRUPT_HARD) &&
         cpu_mips_hw_interrupts_pending(env)) {
         if (cpu_mips_hw_interrupts_enabled(env) ||
-            (env->insn_flags & ISA_MIPS_R6)) {
+            (env->insn_flags & ISA_MIPS_R6) ||
+            (env->CP0_Config7 & (1 << CP0C7_WII))) {
             has_work = true;
         }
     }
diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 0a085643a3..abee7a99d7 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -980,6 +980,7 @@ typedef struct CPUArchState {
 #define CP0C6_DATAPREF        0
     int32_t CP0_Config7;
     int64_t CP0_Config7_rw_bitmask;
+#define CP0C7_WII          31
 #define CP0C7_NAPCGEN       2
 #define CP0C7_UNIMUEN       1
 #define CP0C7_VFPUCGEN      0
-- 
2.25.1

Re: [PATCH 2/3] target/mips: fix SWM32 handling for micromips

[Philippe Mathieu-Daudé]

On 15/2/23 09:47, Marcin Nowakowski wrote:
> SWM32 should store a sequence of 32-bit words from the GPRs, but it was
> incorrectly coded to store 16-bit words only. As a result, an LWM32 that
> usually follows would restore invalid register values.
> 

Fixes: 7dd547e5ab ("target/mips: Use cpu_*_mmuidx_ra instead of 
MMU_MODE*_SUFFIX")

(I suppose a typo S[W] -> ST[W], since LW correctly converted to LDL)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Signed-off-by: Marcin Nowakowski <marcin.nowakowski@fungible.com>
> ---
>   target/mips/tcg/ldst_helper.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/mips/tcg/ldst_helper.c b/target/mips/tcg/ldst_helper.c
> index d0bd0267b2..c1a8380e34 100644
> --- a/target/mips/tcg/ldst_helper.c
> +++ b/target/mips/tcg/ldst_helper.c
> @@ -248,14 +248,14 @@ void helper_swm(CPUMIPSState *env, target_ulong addr, target_ulong reglist,
>           target_ulong i;
>   
>           for (i = 0; i < base_reglist; i++) {
> -            cpu_stw_mmuidx_ra(env, addr, env->active_tc.gpr[multiple_regs[i]],
> +            cpu_stl_mmuidx_ra(env, addr, env->active_tc.gpr[multiple_regs[i]],
>                                 mem_idx, GETPC());
>               addr += 4;
>           }
>       }
>   
>       if (do_r31) {
> -        cpu_stw_mmuidx_ra(env, addr, env->active_tc.gpr[31], mem_idx, GETPC());
> +        cpu_stl_mmuidx_ra(env, addr, env->active_tc.gpr[31], mem_idx, GETPC());
>       }
>   }
>   

Re: [PATCH 3/3] target/mips: implement CP0.Config7.WII bit support

[Philippe Mathieu-Daudé]

Hi Marcin,

On 15/2/23 09:47, Marcin Nowakowski wrote:
> Some older cores use CP0.Config7.WII bit to indicate that a disabled
> interrupt should wake up a sleeping CPU.
> Enable this bit by default for M14Kc, which supports that. There are
> potentially other cores that support this feature, but I do not have a
> complete list.

Also the P5600 (MIPS-MD01025-2B-P5600-Software-TRM-01.60.pdf,
"MIPS32® P5600 Multiprocessing System Software UM, Revision 01.60).

> Signed-off-by: Marcin Nowakowski <marcin.nowakowski@fungible.com>
> ---
>   target/mips/cpu-defs.c.inc | 1 +
>   target/mips/cpu.c          | 6 ++++--
>   target/mips/cpu.h          | 1 +
>   3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/target/mips/cpu-defs.c.inc b/target/mips/cpu-defs.c.inc
> index 480e60aeec..57856e2e72 100644
> --- a/target/mips/cpu-defs.c.inc
> +++ b/target/mips/cpu-defs.c.inc
> @@ -354,6 +354,7 @@ const mips_def_t mips_defs[] =
>                          (0 << CP0C1_DS) | (3 << CP0C1_DL) | (1 << CP0C1_DA),
>           .CP0_Config2 = MIPS_CONFIG2,
>           .CP0_Config3 = MIPS_CONFIG3 | (0x2 << CP0C3_ISA) | (0 << CP0C3_VInt),

Per the P5600 doc on Config5.M:

   Configuration continuation bit. Even though the Config6 and Config7
   registers are used in the P5600 Multiprocessing System, they are both
   defined as implementation-specific registers. As such, this bit is
   zero and is not used to indicate the presence of Config6.

Still I suppose we need to set at least Config4.M:

   +        .CP0_Config4 = MIPS_CONFIG4,
   +        .CP0_Config4_rw_bitmask = 0,

I'm not sure about:

   +        .CP0_Config5 = MIPS_CONFIG5,
   +        .CP0_Config5_rw_bitmask = 0,

> +        .CP0_Config7 = 0x1 << CP0C7_WII,
>           .CP0_LLAddr_rw_bitmask = 0,
>           .CP0_LLAddr_shift = 4,
>           .SYNCI_Step = 32,

Could you also set CP0C7_WII to the P5600 definition?

> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index 7a565466cb..7ba359696f 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -144,12 +144,14 @@ static bool mips_cpu_has_work(CPUState *cs)
>       /*
>        * Prior to MIPS Release 6 it is implementation dependent if non-enabled
>        * interrupts wake-up the CPU, however most of the implementations only
> -     * check for interrupts that can be taken.
> +     * check for interrupts that can be taken. For pre-release 6 CPUs,
> +     * check for CP0 Config7 'Wait IE ignore' bit.
>        */
>       if ((cs->interrupt_request & CPU_INTERRUPT_HARD) &&
>           cpu_mips_hw_interrupts_pending(env)) {
>           if (cpu_mips_hw_interrupts_enabled(env) ||
> -            (env->insn_flags & ISA_MIPS_R6)) {
> +            (env->insn_flags & ISA_MIPS_R6) ||
> +            (env->CP0_Config7 & (1 << CP0C7_WII))) {
>               has_work = true;
>           }
>       }
> diff --git a/target/mips/cpu.h b/target/mips/cpu.h
> index 0a085643a3..abee7a99d7 100644
> --- a/target/mips/cpu.h
> +++ b/target/mips/cpu.h
> @@ -980,6 +980,7 @@ typedef struct CPUArchState {
>   #define CP0C6_DATAPREF        0
>       int32_t CP0_Config7;
>       int64_t CP0_Config7_rw_bitmask;
> +#define CP0C7_WII          31
>   #define CP0C7_NAPCGEN       2
>   #define CP0C7_UNIMUEN       1
>   #define CP0C7_VFPUCGEN      0

Re: [PATCH 3/3] target/mips: implement CP0.Config7.WII bit support

[Marcin Nowakowski]

On Wed, Feb 15, 2023 at 7:33 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Hi Marcin,
>
> On 15/2/23 09:47, Marcin Nowakowski wrote:
> > Some older cores use CP0.Config7.WII bit to indicate that a disabled
> > interrupt should wake up a sleeping CPU.
> > Enable this bit by default for M14Kc, which supports that. There are
> > potentially other cores that support this feature, but I do not have a
> > complete list.
>
> Also the P5600 (MIPS-MD01025-2B-P5600-Software-TRM-01.60.pdf,
> "MIPS32® P5600 Multiprocessing System Software UM, Revision 01.60).
>
> > Signed-off-by: Marcin Nowakowski <marcin.nowakowski@fungible.com>
> > ---
> >   target/mips/cpu-defs.c.inc | 1 +
> >   target/mips/cpu.c          | 6 ++++--
> >   target/mips/cpu.h          | 1 +
> >   3 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/mips/cpu-defs.c.inc b/target/mips/cpu-defs.c.inc
> > index 480e60aeec..57856e2e72 100644
> > --- a/target/mips/cpu-defs.c.inc
> > +++ b/target/mips/cpu-defs.c.inc
> > @@ -354,6 +354,7 @@ const mips_def_t mips_defs[] =
> >                          (0 << CP0C1_DS) | (3 << CP0C1_DL) | (1 << CP0C1_DA),
> >           .CP0_Config2 = MIPS_CONFIG2,
> >           .CP0_Config3 = MIPS_CONFIG3 | (0x2 << CP0C3_ISA) | (0 << CP0C3_VInt),
>
> Per the P5600 doc on Config5.M:
>
>    Configuration continuation bit. Even though the Config6 and Config7
>    registers are used in the P5600 Multiprocessing System, they are both
>    defined as implementation-specific registers. As such, this bit is
>    zero and is not used to indicate the presence of Config6.
>
> Still I suppose we need to set at least Config4.M:
>
>    +        .CP0_Config4 = MIPS_CONFIG4,
>    +        .CP0_Config4_rw_bitmask = 0,

The definition of MIPS_CONFIG4 doesn't set M-bit, so I assume what you
really meant here is
.CP0_Config4 = MIPS_CONFIG4 | (1U << CP0C4_M)
Config3 also doesn't have M-bit set right now, I'll fix that in the
next patch revision.

>
> I'm not sure about:
>
>    +        .CP0_Config5 = MIPS_CONFIG5,
>    +        .CP0_Config5_rw_bitmask = 0,

M14Kc specification (MD00674-2B-M14Kc-SUM-02.04.pdf) notes the following:
"This bit is reserved. With the current architectural definition, this
bit should always read as a 0."
But I'll add
.CP0_Config5 = MIPS_CONFIG5 | (1U << CP0C5_NFExists)
for completeness of the definition.

> > +        .CP0_Config7 = 0x1 << CP0C7_WII,
> >           .CP0_LLAddr_rw_bitmask = 0,
> >           .CP0_LLAddr_shift = 4,
> >           .SYNCI_Step = 32,
>
> Could you also set CP0C7_WII to the P5600 definition?

OK, will add that.

Marcin

> > diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> > index 7a565466cb..7ba359696f 100644
> > --- a/target/mips/cpu.c
> > +++ b/target/mips/cpu.c
> > @@ -144,12 +144,14 @@ static bool mips_cpu_has_work(CPUState *cs)
> >       /*
> >        * Prior to MIPS Release 6 it is implementation dependent if non-enabled
> >        * interrupts wake-up the CPU, however most of the implementations only
> > -     * check for interrupts that can be taken.
> > +     * check for interrupts that can be taken. For pre-release 6 CPUs,
> > +     * check for CP0 Config7 'Wait IE ignore' bit.
> >        */
> >       if ((cs->interrupt_request & CPU_INTERRUPT_HARD) &&
> >           cpu_mips_hw_interrupts_pending(env)) {
> >           if (cpu_mips_hw_interrupts_enabled(env) ||
> > -            (env->insn_flags & ISA_MIPS_R6)) {
> > +            (env->insn_flags & ISA_MIPS_R6) ||
> > +            (env->CP0_Config7 & (1 << CP0C7_WII))) {
> >               has_work = true;
> >           }
> >       }
> > diff --git a/target/mips/cpu.h b/target/mips/cpu.h
> > index 0a085643a3..abee7a99d7 100644
> > --- a/target/mips/cpu.h
> > +++ b/target/mips/cpu.h
> > @@ -980,6 +980,7 @@ typedef struct CPUArchState {
> >   #define CP0C6_DATAPREF        0
> >       int32_t CP0_Config7;
> >       int64_t CP0_Config7_rw_bitmask;
> > +#define CP0C7_WII          31
> >   #define CP0C7_NAPCGEN       2
> >   #define CP0C7_UNIMUEN       1
> >   #define CP0C7_VFPUCGEN      0
>

Re: [PATCH 3/3] target/mips: implement CP0.Config7.WII bit support

[Philippe Mathieu-Daudé]

On 15/2/23 20:19, Marcin Nowakowski wrote:
> On Wed, Feb 15, 2023 at 7:33 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> Hi Marcin,
>>
>> On 15/2/23 09:47, Marcin Nowakowski wrote:
>>> Some older cores use CP0.Config7.WII bit to indicate that a disabled
>>> interrupt should wake up a sleeping CPU.
>>> Enable this bit by default for M14Kc, which supports that. There are
>>> potentially other cores that support this feature, but I do not have a
>>> complete list.
>>
>> Also the P5600 (MIPS-MD01025-2B-P5600-Software-TRM-01.60.pdf,
>> "MIPS32® P5600 Multiprocessing System Software UM, Revision 01.60).
>>
>>> Signed-off-by: Marcin Nowakowski <marcin.nowakowski@fungible.com>
>>> ---
>>>    target/mips/cpu-defs.c.inc | 1 +
>>>    target/mips/cpu.c          | 6 ++++--
>>>    target/mips/cpu.h          | 1 +
>>>    3 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target/mips/cpu-defs.c.inc b/target/mips/cpu-defs.c.inc
>>> index 480e60aeec..57856e2e72 100644
>>> --- a/target/mips/cpu-defs.c.inc
>>> +++ b/target/mips/cpu-defs.c.inc
>>> @@ -354,6 +354,7 @@ const mips_def_t mips_defs[] =
>>>                           (0 << CP0C1_DS) | (3 << CP0C1_DL) | (1 << CP0C1_DA),
>>>            .CP0_Config2 = MIPS_CONFIG2,
>>>            .CP0_Config3 = MIPS_CONFIG3 | (0x2 << CP0C3_ISA) | (0 << CP0C3_VInt),
>>
>> Per the P5600 doc on Config5.M:
>>
>>     Configuration continuation bit. Even though the Config6 and Config7
>>     registers are used in the P5600 Multiprocessing System, they are both
>>     defined as implementation-specific registers. As such, this bit is
>>     zero and is not used to indicate the presence of Config6.
>>
>> Still I suppose we need to set at least Config4.M:
>>
>>     +        .CP0_Config4 = MIPS_CONFIG4,
>>     +        .CP0_Config4_rw_bitmask = 0,
> 
> The definition of MIPS_CONFIG4 doesn't set M-bit, so I assume what you
> really meant here is
> .CP0_Config4 = MIPS_CONFIG4 | (1U << CP0C4_M)

Yes :)

> Config3 also doesn't have M-bit set right now, I'll fix that in the
> next patch revision.
> 
>>
>> I'm not sure about:
>>
>>     +        .CP0_Config5 = MIPS_CONFIG5,
>>     +        .CP0_Config5_rw_bitmask = 0,
> 
> M14Kc specification (MD00674-2B-M14Kc-SUM-02.04.pdf) notes the following:
> "This bit is reserved. With the current architectural definition, this
> bit should always read as a 0."
> But I'll add
> .CP0_Config5 = MIPS_CONFIG5 | (1U << CP0C5_NFExists)
> for completeness of the definition.

Thanks.

>>> +        .CP0_Config7 = 0x1 << CP0C7_WII,
>>>            .CP0_LLAddr_rw_bitmask = 0,
>>>            .CP0_LLAddr_shift = 4,
>>>            .SYNCI_Step = 32,
>>
>> Could you also set CP0C7_WII to the P5600 definition?
> 
> OK, will add that.

Great, thank you :)

Re: [PATCH 1/3] target/mips: fix JALS32/J32 instruction handling for microMIPS

[Richard Henderson]

On 2/14/23 22:47, Marcin Nowakowski wrote:
> @@ -4860,6 +4860,7 @@ static void gen_compute_branch(DisasContext *ctx, uint32_t opc,
>       target_ulong btgt = -1;
>       int blink = 0;
>       int bcond_compute = 0;
> +    int jal_mask = 0;

Better to limit the scope of the variable to the block below.

> @@ -4917,6 +4918,11 @@ static void gen_compute_branch(DisasContext *ctx, uint32_t opc,
>           break;
>       case OPC_J:
>       case OPC_JAL:
> +        /* Jump to immediate */
> +        jal_mask = ctx->hflags & MIPS_HFLAG_M16 ? 0xF8000000 : 0xF0000000;
> +        btgt = ((ctx->base.pc_next + insn_bytes) & jal_mask) |
> +            (uint32_t)offset;

Ideally we wouldn't have one huge helper function, and could pass down the mask from the 
translator.  But that's on-going cleanup.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 1/3] target/mips: fix JALS32/J32 instruction handling for microMIPS

[Philippe Mathieu-Daudé]

On 15/2/23 21:21, Richard Henderson wrote:
> On 2/14/23 22:47, Marcin Nowakowski wrote:
>> @@ -4860,6 +4860,7 @@ static void gen_compute_branch(DisasContext 
>> *ctx, uint32_t opc,
>>       target_ulong btgt = -1;
>>       int blink = 0;
>>       int bcond_compute = 0;
>> +    int jal_mask = 0;
> 
> Better to limit the scope of the variable to the block below.
> 
>> @@ -4917,6 +4918,11 @@ static void gen_compute_branch(DisasContext 
>> *ctx, uint32_t opc,
>>           break;
>>       case OPC_J:
>>       case OPC_JAL:
>> +        /* Jump to immediate */
>> +        jal_mask = ctx->hflags & MIPS_HFLAG_M16 ? 0xF8000000 : 
>> 0xF0000000;
>> +        btgt = ((ctx->base.pc_next + insn_bytes) & jal_mask) |
>> +            (uint32_t)offset;
> 
> Ideally we wouldn't have one huge helper function, and could pass down 
> the mask from the translator.  But that's on-going cleanup.

Yes, this is the approach taken in decodetree conversion.

I hope to rebase / respin incorporating Jiaxun patches some day...

> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
> r~

Re: [PATCH 1/3] target/mips: fix JALS32/J32 instruction handling for microMIPS

[Jiaxun Yang]

在2023年2月15日二月 下午8:50，Philippe Mathieu-Daudé写道：
> On 15/2/23 21:21, Richard Henderson wrote:
>> On 2/14/23 22:47, Marcin Nowakowski wrote:
>>> @@ -4860,6 +4860,7 @@ static void gen_compute_branch(DisasContext 
>>> *ctx, uint32_t opc,
>>>       target_ulong btgt = -1;
>>>       int blink = 0;
>>>       int bcond_compute = 0;
>>> +    int jal_mask = 0;
>> 
>> Better to limit the scope of the variable to the block below.
>> 
>>> @@ -4917,6 +4918,11 @@ static void gen_compute_branch(DisasContext 
>>> *ctx, uint32_t opc,
>>>           break;
>>>       case OPC_J:
>>>       case OPC_JAL:
>>> +        /* Jump to immediate */
>>> +        jal_mask = ctx->hflags & MIPS_HFLAG_M16 ? 0xF8000000 : 
>>> 0xF0000000;
>>> +        btgt = ((ctx->base.pc_next + insn_bytes) & jal_mask) |
>>> +            (uint32_t)offset;
>> 
>> Ideally we wouldn't have one huge helper function, and could pass down 
>> the mask from the translator.  But that's on-going cleanup.
>
> Yes, this is the approach taken in decodetree conversion.
>
> I hope to rebase / respin incorporating Jiaxun patches some day...

Which series are you referring?
Just caught some time so I might able to help.

>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> 
>> 
>> r~

-- 
- Jiaxun