| Message ID | 20250215021654.1786679-5-keithp@keithp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Renesas RX target fixes | expand |
On 2/14/25 18:16, Keith Packard via wrote: > The ROM images all get deleted as they've been loaded to memory, so we > can't go fetch the reset vector from there. Instead, fetch it from > memory. To make that work, we need to execute the delayed mmu setup > function tcg_commit_cpu as that wires up memory dispatching. > > Signed-off-by: Keith Packard <keithp@keithp.com> > --- > target/rx/cpu.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) IIRC this is where the cpu needs to be part of the 3-phase reset process. ROM gets reset too, but with unspecified ordering wrt the cpu itself. By delaying the load of the reset vector to the reset_exit phase, you can always load from rom. I believe Peter most recently handled a very similar situation with armv7m. r~
On Sat, 15 Feb 2025 at 18:24, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 2/14/25 18:16, Keith Packard via wrote: > > The ROM images all get deleted as they've been loaded to memory, so we > > can't go fetch the reset vector from there. Instead, fetch it from > > memory. To make that work, we need to execute the delayed mmu setup > > function tcg_commit_cpu as that wires up memory dispatching. > > > > Signed-off-by: Keith Packard <keithp@keithp.com> > > --- > > target/rx/cpu.c | 24 +++++++++++++++++++----- > > 1 file changed, 19 insertions(+), 5 deletions(-) > > IIRC this is where the cpu needs to be part of the 3-phase reset process. > ROM gets reset too, but with unspecified ordering wrt the cpu itself. > By delaying the load of the reset vector to the reset_exit phase, > you can always load from rom. > > I believe Peter most recently handled a very similar situation with armv7m. v7m still does the same thing this patch does, where you call rom_ptr_for_as() and look at the return value to see what's going on -- see the code and comments in target/arm/cpu.c:arm_cpu_reset_hold() ("Load the initial SP and PC"). Now that we do a full three phase reset for everything, I think this probably could be cleaned up, but there's a fair chance that there's some unexpected issue in there that I won't find out until I try it. So I'm definitely not going to require that somebody else does that before I've had a go at it on the v7m code. So I'm OK with this patch doing this the way it does, except that I have one question: what's that process_queued_cpu_work() call doing? We don't need that on the Arm equivalent... Also, if() statements always have braces in QEMU, even if the body of the if is a single statement. But since if you don't set env->pc it is zero anyway, you don't need the if() at all I think and can directly set env->pc unconditionally. thanks -- PMM
From: Richard Henderson <richard.henderson@linaro.org> Date: Sat, 15 Feb 2025 10:24:05 -0800 > By delaying the load of the reset vector to the reset_exit phase, > you can always load from rom. I'm not sure how -- the ROM image is discarded when it gets loaded into read-only memory. If loaded to read-write memory, I bet it would stay around.
> > By delaying the load of the reset vector to the reset_exit phase, > > you can always load from rom. > I'm not sure how -- the ROM image is discarded when it gets loaded into > read-only memory. If loaded to read-write memory, I bet it would > stay around. Ah, but by delaying until after cpu_reset has finished, I can always load it from target memory. I bet that's what you meant, and yes it works fine.
> So I'm OK with this patch doing this the way it does, > except that I have one question: what's that > process_queued_cpu_work() call doing? We don't need > that on the Arm equivalent... Yup, I needed that because I was running this bit at cpu_reset_hold time, not waiting until after cpu reset was finished. At cpu_reset_hold time, the re-initialization of memory_dispatch hadn't been executed yet, so attempts to resolve addresses crashed unless I synchronously flushed the work queue where the call to tcg_commit_cpu was pending. All fixed now that I'm setting the pc after the call to cpu_reset has completed. > Also, if() statements always have braces in QEMU, even > if the body of the if is a single statement. But since > if you don't set env->pc it is zero anyway, you don't > need the if() at all I think and can directly set > env->pc unconditionally. Thanks for the formatting advice; I'll get it cleaned up.
diff --git a/target/rx/cpu.c b/target/rx/cpu.c index 04dd34b310..a32b410bb1 100644 --- a/target/rx/cpu.c +++ b/target/rx/cpu.c @@ -24,6 +24,7 @@ #include "exec/exec-all.h" #include "exec/page-protection.h" #include "exec/translation-block.h" +#include "exec/cpu_ldst.h" #include "hw/loader.h" #include "fpu/softfloat.h" #include "tcg/debug-assert.h" @@ -77,7 +78,8 @@ static void rx_cpu_reset_hold(Object *obj, ResetType type) CPUState *cs = CPU(obj); RXCPUClass *rcc = RX_CPU_GET_CLASS(obj); CPURXState *env = cpu_env(cs); - uint32_t *resetvec; + uint32_t *resetvec_p; + vaddr resetvec; if (rcc->parent_phases.hold) { rcc->parent_phases.hold(obj, type); @@ -85,11 +87,23 @@ static void rx_cpu_reset_hold(Object *obj, ResetType type) memset(env, 0, offsetof(CPURXState, end_reset_fields)); - resetvec = rom_ptr(0xfffffffc, 4); - if (resetvec) { - /* In the case of kernel, it is ignored because it is not set. */ - env->pc = ldl_p(resetvec); + /* + * During the first reset phase, the memory dispatching hook + * hasn't been set, so we can't fetch the reset vector from + * memory. After that, the ROM image will have been discarded, so + * we can't fetch the reset vector from there. So we have two + * paths here. + */ + resetvec_p = rom_ptr_for_as(cs->as, 0xfffffffc, 4); + if (resetvec_p) + resetvec = ldl_p(resetvec_p); + else { + process_queued_cpu_work(cs); + resetvec = cpu_ldl_data(env, 0xfffffffc); } + if (resetvec) + env->pc = resetvec; + rx_cpu_unpack_psw(env, 0, 1); env->regs[0] = env->isp = env->usp = 0; env->fpsw = 0;
The ROM images all get deleted as they've been loaded to memory, so we can't go fetch the reset vector from there. Instead, fetch it from memory. To make that work, we need to execute the delayed mmu setup function tcg_commit_cpu as that wires up memory dispatching. Signed-off-by: Keith Packard <keithp@keithp.com> --- target/rx/cpu.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)