Fix mips jazz vs constant TCGCPUOps

[PATCH 1/2] target/mips: Fold jazz behaviour into mips_cpu_do_transaction_failed

Posted by Richard Henderson 4 years, 2 months ago

Add a flag to MIPSCPUClass in order to avoid needing to
replace mips_tcg_ops.do_transaction_failed.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/mips/cpu-qom.h   |  3 +++
 hw/mips/jazz.c          | 35 +++--------------------------------
 target/mips/op_helper.c |  3 ++-
 3 files changed, 8 insertions(+), 33 deletions(-)

diff --git a/target/mips/cpu-qom.h b/target/mips/cpu-qom.h
index 826ab13019..dda0c911fa 100644
--- a/target/mips/cpu-qom.h
+++ b/target/mips/cpu-qom.h
@@ -47,6 +47,9 @@ struct MIPSCPUClass {
     DeviceRealize parent_realize;
     DeviceReset parent_reset;
     const struct mips_def_t *cpu_def;
+
+    /* Used for the jazz board to modify mips_cpu_do_transaction_failed. */
+    bool no_data_aborts;
 };
 
 
diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index 83c8086062..7b22a9b511 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -120,30 +120,6 @@ static const MemoryRegionOps dma_dummy_ops = {
 #define MAGNUM_BIOS_SIZE                                                       \
         (BIOS_SIZE < MAGNUM_BIOS_SIZE_MAX ? BIOS_SIZE : MAGNUM_BIOS_SIZE_MAX)
 
-#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
-static void (*real_do_transaction_failed)(CPUState *cpu, hwaddr physaddr,
-                                          vaddr addr, unsigned size,
-                                          MMUAccessType access_type,
-                                          int mmu_idx, MemTxAttrs attrs,
-                                          MemTxResult response,
-                                          uintptr_t retaddr);
-
-static void mips_jazz_do_transaction_failed(CPUState *cs, hwaddr physaddr,
-                                            vaddr addr, unsigned size,
-                                            MMUAccessType access_type,
-                                            int mmu_idx, MemTxAttrs attrs,
-                                            MemTxResult response,
-                                            uintptr_t retaddr)
-{
-    if (access_type != MMU_INST_FETCH) {
-        /* ignore invalid access (ie do not raise exception) */
-        return;
-    }
-    (*real_do_transaction_failed)(cs, physaddr, addr, size, access_type,
-                                  mmu_idx, attrs, response, retaddr);
-}
-#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */
-
 static void mips_jazz_init(MachineState *machine,
                            enum jazz_model_e jazz_model)
 {
@@ -152,7 +128,7 @@ static void mips_jazz_init(MachineState *machine,
     int bios_size, n;
     Clock *cpuclk;
     MIPSCPU *cpu;
-    CPUClass *cc;
+    MIPSCPUClass *mcc;
     CPUMIPSState *env;
     qemu_irq *i8259;
     rc4030_dma *dmas;
@@ -199,8 +175,6 @@ static void mips_jazz_init(MachineState *machine,
      * However, we can't simply add a global memory region to catch
      * everything, as this would make all accesses including instruction
      * accesses be ignored and not raise exceptions.
-     * So instead we hijack the do_transaction_failed method on the CPU, and
-     * do not raise exceptions for data access.
      *
      * NOTE: this behaviour of raising exceptions for bad instruction
      * fetches but not bad data accesses was added in commit 54e755588cf1e9
@@ -210,11 +184,8 @@ static void mips_jazz_init(MachineState *machine,
      * we could replace this hijacking of CPU methods with a simple global
      * memory region that catches all memory accesses, as we do on Malta.
      */
-    cc = CPU_GET_CLASS(cpu);
-#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
-    real_do_transaction_failed = cc->tcg_ops->do_transaction_failed;
-    cc->tcg_ops->do_transaction_failed = mips_jazz_do_transaction_failed;
-#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */
+    mcc = MIPS_CPU_GET_CLASS(cpu);
+    mcc->no_data_aborts = true;
 
     /* allocate RAM */
     memory_region_add_subregion(address_space, 0, machine->ram);
diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index b80e8f7540..7626fc5254 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -1164,11 +1164,12 @@ void mips_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
                                     MemTxResult response, uintptr_t retaddr)
 {
     MIPSCPU *cpu = MIPS_CPU(cs);
+    MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(cpu);
     CPUMIPSState *env = &cpu->env;
 
     if (access_type == MMU_INST_FETCH) {
         do_raise_exception(env, EXCP_IBE, retaddr);
-    } else {
+    } else if (!mcc->no_data_aborts) {
         do_raise_exception(env, EXCP_DBE, retaddr);
     }
 }
-- 
2.25.1

Re: [PATCH 1/2] target/mips: Fold jazz behaviour into mips_cpu_do_transaction_failed

Posted by Philippe Mathieu-Daudé 4 years, 2 months ago

On 2/28/21 12:25 AM, Richard Henderson wrote:
> Add a flag to MIPSCPUClass in order to avoid needing to
> replace mips_tcg_ops.do_transaction_failed.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/mips/cpu-qom.h   |  3 +++
>  hw/mips/jazz.c          | 35 +++--------------------------------
>  target/mips/op_helper.c |  3 ++-
>  3 files changed, 8 insertions(+), 33 deletions(-)
...
> diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
> index 83c8086062..7b22a9b511 100644
> --- a/hw/mips/jazz.c
> +++ b/hw/mips/jazz.c
...> @@ -152,7 +128,7 @@ static void mips_jazz_init(MachineState *machine,
>      int bios_size, n;
>      Clock *cpuclk;
>      MIPSCPU *cpu;
> -    CPUClass *cc;
> +    MIPSCPUClass *mcc;
>      CPUMIPSState *env;
>      qemu_irq *i8259;
>      rc4030_dma *dmas;
> @@ -199,8 +175,6 @@ static void mips_jazz_init(MachineState *machine,
>       * However, we can't simply add a global memory region to catch
>       * everything, as this would make all accesses including instruction
>       * accesses be ignored and not raise exceptions.
> -     * So instead we hijack the do_transaction_failed method on the CPU, and
> -     * do not raise exceptions for data access.
>       *
>       * NOTE: this behaviour of raising exceptions for bad instruction
>       * fetches but not bad data accesses was added in commit 54e755588cf1e9
> @@ -210,11 +184,8 @@ static void mips_jazz_init(MachineState *machine,
>       * we could replace this hijacking of CPU methods with a simple global
>       * memory region that catches all memory accesses, as we do on Malta.
>       */
> -    cc = CPU_GET_CLASS(cpu);
> -#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
> -    real_do_transaction_failed = cc->tcg_ops->do_transaction_failed;
> -    cc->tcg_ops->do_transaction_failed = mips_jazz_do_transaction_failed;

We don't need the "hw/core/tcg-cpu-ops.h" header anymore.

> -#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */
> +    mcc = MIPS_CPU_GET_CLASS(cpu);
> +    mcc->no_data_aborts = true;

Re: [PATCH 1/2] target/mips: Fold jazz behaviour into mips_cpu_do_transaction_failed

Posted by Philippe Mathieu-Daudé 4 years, 2 months ago

Hi Richard,

On 2/28/21 12:25 AM, Richard Henderson wrote:
> Add a flag to MIPSCPUClass in order to avoid needing to
> replace mips_tcg_ops.do_transaction_failed.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/mips/cpu-qom.h   |  3 +++
>  hw/mips/jazz.c          | 35 +++--------------------------------
>  target/mips/op_helper.c |  3 ++-
>  3 files changed, 8 insertions(+), 33 deletions(-)
> 
> diff --git a/target/mips/cpu-qom.h b/target/mips/cpu-qom.h
> index 826ab13019..dda0c911fa 100644
> --- a/target/mips/cpu-qom.h
> +++ b/target/mips/cpu-qom.h
> @@ -47,6 +47,9 @@ struct MIPSCPUClass {
>      DeviceRealize parent_realize;
>      DeviceReset parent_reset;
>      const struct mips_def_t *cpu_def;
> +
> +    /* Used for the jazz board to modify mips_cpu_do_transaction_failed. */

Isn't it possible to have other (old) boards doing something similar?

If so any target can overload its CPUClass with the same boolean,
so:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +    bool no_data_aborts;
>  };

Re: [PATCH 1/2] target/mips: Fold jazz behaviour into mips_cpu_do_transaction_failed

Posted by Richard Henderson 4 years, 2 months ago

On 2/28/21 8:14 AM, Philippe Mathieu-Daudé wrote:
>> +    /* Used for the jazz board to modify mips_cpu_do_transaction_failed. */
> 
> Isn't it possible to have other (old) boards doing something similar?

It's possible, but I doubt any need to.

I think the comment in hw/mips/jazz.c is correct, in essence.  That we're
simply being bug-compatible with older qemu, and that real jazz hw does not
discriminate between instruction and data loads.

r~

Re: [PATCH 1/2] target/mips: Fold jazz behaviour into mips_cpu_do_transaction_failed

Posted by Philippe Mathieu-Daudé 4 years, 2 months ago

On 2/28/21 10:39 PM, Richard Henderson wrote:
> On 2/28/21 8:14 AM, Philippe Mathieu-Daudé wrote:
>>> +    /* Used for the jazz board to modify mips_cpu_do_transaction_failed. */
>>
>> Isn't it possible to have other (old) boards doing something similar?
> 
> It's possible, but I doubt any need to.
> 
> I think the comment in hw/mips/jazz.c is correct, in essence.  That we're
> simply being bug-compatible with older qemu, and that 

"... we don't know if ..."

> real jazz hw does not
> discriminate between instruction and data loads.
We could figure it out by suggesting a test to linux-mips list,
one subscriber still has a working Jazz board.