| Message ID | 20210227232519.222663-2-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix mips jazz vs constant TCGCPUOps | expand |
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; > };
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~
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.
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;
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); } }
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(-) -- 2.25.1