[PATCH 0/5] Renesas RX target fixes

[PATCH 0/5] Renesas RX target fixes

[Keith Packard via]

I'm getting a Renesas toolchain working and found a couple of bugs
and a few fixes in the qemu target code for this device.

The two critical bugs which are fixed:

 1. Exception vector base address is incorrect. The
    right value is 0xffffff80.

 2. A bunch of opcode helper functions are incorrectly labeled as
    TCG_CALL_NO_WG. These helpers read and write virtual registers out
    of the global environment and so must not be marked with this flag.

The other changes included are sufficient to use qemu without needing
to start gdb as well, starting the machine using the reset vector
found in the exception table and then re-loading that vector during
subsequent reset operations.

With these fixes, the picolibc CI tests are now passing.

Keith Packard (5):
  hw/rx: Allow execution without either bios or kernel
  target/rx: Set exception vector base to 0xffffff80
  target/rx: Reset the CPU at qemu reset time
  target/rx: Load reset vector from memory after first run
  target/rx: Remove TCG_CALL_NO_WG from helpers which write env

 hw/rx/rx-gdbsim.c  |  3 ---
 target/rx/cpu.c    | 35 +++++++++++++++++++++++++++++------
 target/rx/helper.c |  2 +-
 target/rx/helper.h | 14 +++++++-------
 4 files changed, 37 insertions(+), 17 deletions(-)

-- 
2.47.2

[PATCH 1/5] hw/rx: Allow execution without either bios or kernel

[Keith Packard via]

Users can use -device loader to get an ELF file loaded to
memory, so we don't need to require one of these options.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 hw/rx/rx-gdbsim.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/rx/rx-gdbsim.c b/hw/rx/rx-gdbsim.c
index 88c8f12c10..4afd77efd5 100644
--- a/hw/rx/rx-gdbsim.c
+++ b/hw/rx/rx-gdbsim.c
@@ -110,9 +110,6 @@ static void rx_gdbsim_init(MachineState *machine)
     if (!kernel_filename) {
         if (machine->firmware) {
             rom_add_file_fixed(machine->firmware, RX62N_CFLASH_BASE, 0);
-        } else if (!qtest_enabled()) {
-            error_report("No bios or kernel specified");
-            exit(1);
         }
     }
 
-- 
2.47.2

[PATCH 2/5] target/rx: Set exception vector base to 0xffffff80

[Keith Packard via]

The documentation says the vector is at 0xffffff80, instead of the
previous value of 0xffffffc0. That value must have been a bug because
the standard vector values (20, 21, 23, 25, 30) were all
past the end of the array.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 target/rx/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/rx/helper.c b/target/rx/helper.c
index 80912e8dcb..55e2ae4a11 100644
--- a/target/rx/helper.c
+++ b/target/rx/helper.c
@@ -90,7 +90,7 @@ void rx_cpu_do_interrupt(CPUState *cs)
         cpu_stl_data(env, env->isp, env->pc);
 
         if (vec < 0x100) {
-            env->pc = cpu_ldl_data(env, 0xffffffc0 + vec * 4);
+            env->pc = cpu_ldl_data(env, 0xffffff80 + vec * 4);
         } else {
             env->pc = cpu_ldl_data(env, env->intb + (vec & 0xff) * 4);
         }
-- 
2.47.2

[PATCH 3/5] target/rx: Reset the CPU at qemu reset time

[Keith Packard via]

This ensure that the CPU gets reset every time QEMU resets.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 target/rx/cpu.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index 37a6fdd569..04dd34b310 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -27,6 +27,7 @@
 #include "hw/loader.h"
 #include "fpu/softfloat.h"
 #include "tcg/debug-assert.h"
+#include "system/reset.h"
 
 static void rx_cpu_set_pc(CPUState *cs, vaddr value)
 {
@@ -129,6 +130,13 @@ static ObjectClass *rx_cpu_class_by_name(const char *cpu_model)
     return oc;
 }
 
+static void rx_cpu_reset(void *opaque)
+{
+    RXCPU *cpu = opaque;
+
+    cpu_reset(CPU(cpu));
+}
+
 static void rx_cpu_realize(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
@@ -142,9 +150,10 @@ static void rx_cpu_realize(DeviceState *dev, Error **errp)
     }
 
     qemu_init_vcpu(cs);
-    cpu_reset(cs);
 
     rcc->parent_realize(dev, errp);
+
+    qemu_register_reset(rx_cpu_reset, RX_CPU(cs));
 }
 
 static void rx_cpu_set_irq(void *opaque, int no, int request)
-- 
2.47.2

[PATCH 4/5] target/rx: Load reset vector from memory after first run

[Keith Packard via]

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(-)

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;
-- 
2.47.2

[PATCH 5/5] target/rx: Remove TCG_CALL_NO_WG from helpers which write env

[Keith Packard via]

Functions which modify virtual machine state (such as virtual
registers stored in memory) must not be marked TCG_CALL_NO_WG as that
tells the optimizer that virtual registers values already loaded in
machine registers are still valid, hence discards any changes which
these helpers may have made.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 target/rx/helper.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/target/rx/helper.h b/target/rx/helper.h
index ebb4739474..ac21f4fddd 100644
--- a/target/rx/helper.h
+++ b/target/rx/helper.h
@@ -13,18 +13,18 @@ DEF_HELPER_FLAGS_2(ftoi, TCG_CALL_NO_WG, i32, env, f32)
 DEF_HELPER_FLAGS_2(round, TCG_CALL_NO_WG, i32, env, f32)
 DEF_HELPER_FLAGS_2(itof, TCG_CALL_NO_WG, f32, env, i32)
 DEF_HELPER_2(set_fpsw, void, env, i32)
-DEF_HELPER_FLAGS_2(racw, TCG_CALL_NO_WG, void, env, i32)
-DEF_HELPER_FLAGS_2(set_psw_rte, TCG_CALL_NO_WG, void, env, i32)
-DEF_HELPER_FLAGS_2(set_psw, TCG_CALL_NO_WG, void, env, i32)
+DEF_HELPER_2(racw, void, env, i32)
+DEF_HELPER_2(set_psw_rte, void, env, i32)
+DEF_HELPER_2(set_psw, void, env, i32)
 DEF_HELPER_1(pack_psw, i32, env)
 DEF_HELPER_FLAGS_3(div, TCG_CALL_NO_WG, i32, env, i32, i32)
 DEF_HELPER_FLAGS_3(divu, TCG_CALL_NO_WG, i32, env, i32, i32)
-DEF_HELPER_FLAGS_1(scmpu, TCG_CALL_NO_WG, void, env)
+DEF_HELPER_1(scmpu, void, env)
 DEF_HELPER_1(smovu, void, env)
 DEF_HELPER_1(smovf, void, env)
 DEF_HELPER_1(smovb, void, env)
 DEF_HELPER_2(sstr, void, env, i32)
-DEF_HELPER_FLAGS_2(swhile, TCG_CALL_NO_WG, void, env, i32)
-DEF_HELPER_FLAGS_2(suntil, TCG_CALL_NO_WG, void, env, i32)
-DEF_HELPER_FLAGS_2(rmpa, TCG_CALL_NO_WG, void, env, i32)
+DEF_HELPER_2(swhile, void, env, i32)
+DEF_HELPER_2(suntil, void, env, i32)
+DEF_HELPER_2(rmpa, void, env, i32)
 DEF_HELPER_1(satr, void, env)
-- 
2.47.2

Re: [PATCH 5/5] target/rx: Remove TCG_CALL_NO_WG from helpers which write env

[Keith Packard via]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]


> Functions which modify virtual machine state (such as virtual
> registers stored in memory) must not be marked TCG_CALL_NO_WG as that
> tells the optimizer that virtual registers values already loaded in
> machine registers are still valid, hence discards any changes which
> these helpers may have made.

I still don't understand the restrictions on using these flags. I just
had to disable this flag for other helpers which only set conditions
codes in PSW and FPSW. Is that expected? When are these flags supposed
to be valid?

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 5/5] target/rx: Remove TCG_CALL_NO_WG from helpers which write env

[Richard Henderson]

On 2/15/25 01:21, Keith Packard via wrote:
> 
>> Functions which modify virtual machine state (such as virtual
>> registers stored in memory) must not be marked TCG_CALL_NO_WG as that
>> tells the optimizer that virtual registers values already loaded in
>> machine registers are still valid, hence discards any changes which
>> these helpers may have made.
> 
> I still don't understand the restrictions on using these flags. I just
> had to disable this flag for other helpers which only set conditions
> codes in PSW and FPSW. Is that expected? When are these flags supposed
> to be valid?

Yes, that's expected.

The state of affairs is not helped by the rx target's misuse of tcg globals.

A target should define tcg globals for values that are used frequently for emulation.  The 
bits of the PSW certainly fit that bill, because they're touched by most arithmetic 
operations.

However, fpsw, bpsw, bpc, isp, fintv, and intb are only used in move_to/from_cr and RTFI. 
This is infrequent, so simply loading and storing to env is preferred.  E.g.

     tcg_gen_ld_i32(value, tcg_env, offsetof(CPURXState, fpsw));


r~

Re: [PATCH 2/5] target/rx: Set exception vector base to 0xffffff80

[Richard Henderson]

On 2/14/25 18:16, Keith Packard via wrote:
> The documentation says the vector is at 0xffffff80, instead of the
> previous value of 0xffffffc0. That value must have been a bug because
> the standard vector values (20, 21, 23, 25, 30) were all
> past the end of the array.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>   target/rx/helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/rx/helper.c b/target/rx/helper.c
> index 80912e8dcb..55e2ae4a11 100644
> --- a/target/rx/helper.c
> +++ b/target/rx/helper.c
> @@ -90,7 +90,7 @@ void rx_cpu_do_interrupt(CPUState *cs)
>           cpu_stl_data(env, env->isp, env->pc);
>   
>           if (vec < 0x100) {
> -            env->pc = cpu_ldl_data(env, 0xffffffc0 + vec * 4);
> +            env->pc = cpu_ldl_data(env, 0xffffff80 + vec * 4);
>           } else {
>               env->pc = cpu_ldl_data(env, env->intb + (vec & 0xff) * 4);
>           }

This does appear to match the unnamed constants used as operands to raise_exception, 
comparing to the vector addresses in the manual: (0xffffffd0, etc).

It would be nice to have them named, e.g. per the named list in rx_cpu_do_interrupt.  The 
0x100 constant would probably be better numbered 32, so that

   vec < 0x100

checks themselves don't imply wraparound from 0xffffff80.


r~

Re: [PATCH 1/5] hw/rx: Allow execution without either bios or kernel

[Richard Henderson]

On 2/14/25 18:16, Keith Packard via wrote:
> Users can use -device loader to get an ELF file loaded to
> memory, so we don't need to require one of these options.
> 
> Signed-off-by: Keith Packard<keithp@keithp.com>
> ---
>   hw/rx/rx-gdbsim.c | 3 ---
>   1 file changed, 3 deletions(-)

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

r~

Re: [PATCH 4/5] target/rx: Load reset vector from memory after first run

[Richard Henderson]

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~

Re: [PATCH 5/5] target/rx: Remove TCG_CALL_NO_WG from helpers which write env

[Richard Henderson]

On 2/15/25 09:09, Richard Henderson wrote:
> On 2/15/25 01:21, Keith Packard via wrote:
>>
>>> Functions which modify virtual machine state (such as virtual
>>> registers stored in memory) must not be marked TCG_CALL_NO_WG as that
>>> tells the optimizer that virtual registers values already loaded in
>>> machine registers are still valid, hence discards any changes which
>>> these helpers may have made.
>>
>> I still don't understand the restrictions on using these flags. I just
>> had to disable this flag for other helpers which only set conditions
>> codes in PSW and FPSW. Is that expected? When are these flags supposed
>> to be valid?
> 
> Yes, that's expected.
> 
> The state of affairs is not helped by the rx target's misuse of tcg globals.
> 
> A target should define tcg globals for values that are used frequently for emulation.  The 
> bits of the PSW certainly fit that bill, because they're touched by most arithmetic 
> operations.
> 
> However, fpsw, bpsw, bpc, isp, fintv, and intb are only used in move_to/from_cr and RTFI. 
> This is infrequent, so simply loading and storing to env is preferred.  E.g.
> 
>      tcg_gen_ld_i32(value, tcg_env, offsetof(CPURXState, fpsw));
> 

To finish my thought here,

> +DEF_HELPER_2(set_psw_rte, void, env, i32)
> +DEF_HELPER_2(set_psw, void, env, i32)
> +DEF_HELPER_1(scmpu, void, env)
> +DEF_HELPER_2(swhile, void, env, i32)
> +DEF_HELPER_2(suntil, void, env, i32)
> +DEF_HELPER_2(rmpa, void, env, i32)

are absolutely correct, in that they modify all of the psw_* globals.

> +DEF_HELPER_2(racw, void, env, i32)

is currently correct, in that it modifies cpu_acc.  That said, there's no reason that 
function could not be expanded inline:

     tcg_gen_shli_i64(cpu_acc, cpu_acc, imm);
     tcg_gen_addi_i64(cpu_acc, cpu_acc, 0x0000000080000000ull);
     tcg_gen_smin_i64(cpu_acc, cpu_acc, tcg_constant_i64(0x00007fff00000000ull));
     tcg_gen_smax_i64(cpu_acc, cpu_acc, tcg_constant_i64(0xffff800000000000ull));
     tcg_gen_andi_i64(cpu_acc, cpu_acc, 0xffffffff00000000ull);


r~

Re: [PATCH 3/5] target/rx: Reset the CPU at qemu reset time

[Philippe Mathieu-Daudé]

+Peter/Igor

On 15/2/25 03:16, Keith Packard via wrote:
> This ensure that the CPU gets reset every time QEMU resets.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>   target/rx/cpu.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/target/rx/cpu.c b/target/rx/cpu.c
> index 37a6fdd569..04dd34b310 100644
> --- a/target/rx/cpu.c
> +++ b/target/rx/cpu.c
> @@ -27,6 +27,7 @@
>   #include "hw/loader.h"
>   #include "fpu/softfloat.h"
>   #include "tcg/debug-assert.h"
> +#include "system/reset.h"
>   
>   static void rx_cpu_set_pc(CPUState *cs, vaddr value)
>   {
> @@ -129,6 +130,13 @@ static ObjectClass *rx_cpu_class_by_name(const char *cpu_model)
>       return oc;
>   }
>   
> +static void rx_cpu_reset(void *opaque)
> +{
> +    RXCPU *cpu = opaque;
> +
> +    cpu_reset(CPU(cpu));
> +}
> +
>   static void rx_cpu_realize(DeviceState *dev, Error **errp)
>   {
>       CPUState *cs = CPU(dev);
> @@ -142,9 +150,10 @@ static void rx_cpu_realize(DeviceState *dev, Error **errp)
>       }
>   
>       qemu_init_vcpu(cs);
> -    cpu_reset(cs);
>   
>       rcc->parent_realize(dev, errp);
> +
> +    qemu_register_reset(rx_cpu_reset, RX_CPU(cs));

Yeah. I guess this is part of some generic effort to generalize
CPU reset, previously started here
https://lore.kernel.org/qemu-devel/20230918160257.30127-1-philmd@linaro.org/
which led to another issue Igor is trying to solves:
https://lore.kernel.org/qemu-devel/20250207162048.1890669-1-imammedo@redhat.com/

>   }
>   
>   static void rx_cpu_set_irq(void *opaque, int no, int request)

Re: [PATCH 4/5] target/rx: Load reset vector from memory after first run

[Peter Maydell]

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

Re: [PATCH 3/5] target/rx: Reset the CPU at qemu reset time

[Peter Maydell]

On Sat, 15 Feb 2025 at 02:17, Keith Packard via <qemu-devel@nongnu.org> wrote:
>
> This ensure that the CPU gets reset every time QEMU resets.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  target/rx/cpu.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/target/rx/cpu.c b/target/rx/cpu.c
> index 37a6fdd569..04dd34b310 100644
> --- a/target/rx/cpu.c
> +++ b/target/rx/cpu.c
> @@ -27,6 +27,7 @@
>  #include "hw/loader.h"
>  #include "fpu/softfloat.h"
>  #include "tcg/debug-assert.h"
> +#include "system/reset.h"
>
>  static void rx_cpu_set_pc(CPUState *cs, vaddr value)
>  {
> @@ -129,6 +130,13 @@ static ObjectClass *rx_cpu_class_by_name(const char *cpu_model)
>      return oc;
>  }
>
> +static void rx_cpu_reset(void *opaque)
> +{
> +    RXCPU *cpu = opaque;
> +
> +    cpu_reset(CPU(cpu));
> +}
> +
>  static void rx_cpu_realize(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
> @@ -142,9 +150,10 @@ static void rx_cpu_realize(DeviceState *dev, Error **errp)
>      }
>
>      qemu_init_vcpu(cs);
> -    cpu_reset(cs);
>
>      rcc->parent_realize(dev, errp);
> +
> +    qemu_register_reset(rx_cpu_reset, RX_CPU(cs));
>  }

Reset of devices not plugged into buses (of which CPUs
are the most common kind) is a mess. But having them
call qemu_register_reset() themselves in their own
realize method isn't the usual workaround. Instead we
get the board code to do it (usually in the same function
that handles arranging to sort out the in-QEMU boot
loader, see eg hw/arm/boot.c).

thanks
-- PMM

Re: [PATCH 3/5] target/rx: Reset the CPU at qemu reset time

[Keith Packard via]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

From: Peter Maydell <peter.maydell@linaro.org>
Date: Mon, 17 Feb 2025 09:53:58 +0000

> Reset of devices not plugged into buses (of which CPUs
> are the most common kind) is a mess. But having them
> call qemu_register_reset() themselves in their own
> realize method isn't the usual workaround. Instead we
> get the board code to do it (usually in the same function
> that handles arranging to sort out the in-QEMU boot
> loader, see eg hw/arm/boot.c).

Oh, thanks so much! I was confused about how to select the right
starting PC value -- when loading a kernel image, the code wanted to
set the PC to the kernel entry point, but otherwise it wanted to use the
reset vector. That "worked" because the kernel loading code set the PC
after the reset vector was loaded from ROM.

With your advice, I've made the hardware layer register the reset
handler which just calls cpu_reset and then sets the initial PC value,
either from the loaded kernel or using the reset vector contents. So
much cleaner and "obviously" doing what we want.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 4/5] target/rx: Load reset vector from memory after first run

[Keith Packard via]

[-- Attachment #1: Type: text/plain, Size: 369 bytes --]

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.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 4/5] target/rx: Load reset vector from memory after first run

[Keith Packard via]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]


> > 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.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 4/5] target/rx: Load reset vector from memory after first run

[Keith Packard via]

[-- Attachment #1: Type: text/plain, Size: 965 bytes --]


> 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.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[PATCH 0/4] Renesas RX target fixes (v2)

[Keith Packard via]

With feedback from Peter Maydell and Richard Henderson, I've updated
this series to address two concerns:

 1. The hardware model is now responsible for guiding the CPU reset
    process.

 2. Loading the reset vector from memory is now delayed until cpu_reset
    is finished to ensure memory_dispatch is initialized.

First, there are two critical flaws in the emulation. These are
needed for this model to work correctly:

 1. The exception vector base is 0xffffff80 not 0xffffffc0. This
    prevents exceptions from working at all.

 2. Many tcg helpers inappropriately used TCG_CALL_NO_WG even though
    they modified virtual registers stored in global memory. This
    causes these operations to fail unless one-insn-per-tb was enabled.

The third patch changes how the cpu is reset so that it is driven by
the hw code instead of the target code. Now the cpu is reset each time
qemu is reset and the initial PC value is either set from the loaded
kernel or from the reset vector. This should look a lot more like how
other models manage this process.

The final patch adds the ability to load an ELF file rather than
a binary memory image. It's purely a new feature and not required for
this model to be usable; without this, it's fairly easy to use
the loader device; that just requires the loaded image to include the
exception vectors with the correct reset vector value.

Keith Packard (4):
  target/rx: Set exception vector base to 0xffffff80
  target/rx: Remove TCG_CALL_NO_WG from helpers which write env
  hw/rx: Reset the CPU at qemu reset time
  rx: Support loading of ELF files too

 hw/rx/rx-gdbsim.c  | 72 +++++++++++++++++++++++++++++++++++++++++++++-
 target/rx/cpu.c    |  9 ++----
 target/rx/cpu.h    |  3 ++
 target/rx/helper.c |  2 +-
 target/rx/helper.h | 34 +++++++++++-----------
 5 files changed, 94 insertions(+), 26 deletions(-)

-- 
2.47.2

[PATCH 1/4] target/rx: Set exception vector base to 0xffffff80

[Keith Packard via]

The documentation says the vector is at 0xffffff80, instead of the
previous value of 0xffffffc0. That value must have been a bug because
the standard vector values (20, 21, 23, 25, 30) were all
past the end of the array.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 target/rx/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/rx/helper.c b/target/rx/helper.c
index 80912e8dcb..55e2ae4a11 100644
--- a/target/rx/helper.c
+++ b/target/rx/helper.c
@@ -90,7 +90,7 @@ void rx_cpu_do_interrupt(CPUState *cs)
         cpu_stl_data(env, env->isp, env->pc);
 
         if (vec < 0x100) {
-            env->pc = cpu_ldl_data(env, 0xffffffc0 + vec * 4);
+            env->pc = cpu_ldl_data(env, 0xffffff80 + vec * 4);
         } else {
             env->pc = cpu_ldl_data(env, env->intb + (vec & 0xff) * 4);
         }
-- 
2.47.2

[PATCH 2/4] target/rx: Remove TCG_CALL_NO_WG from helpers which write env

[Keith Packard via]

Functions which modify virtual machine state (such as virtual
registers stored in memory) must not be marked TCG_CALL_NO_WG as that
tells the optimizer that virtual registers values already loaded in
machine registers are still valid, hence discards any changes which
these helpers may have made. This seems to also mean that functions which
set condition codes may also not use this flag

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 target/rx/helper.h | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/target/rx/helper.h b/target/rx/helper.h
index ebb4739474..8cc38b0cb7 100644
--- a/target/rx/helper.h
+++ b/target/rx/helper.h
@@ -4,27 +4,27 @@ DEF_HELPER_1(raise_privilege_violation, noreturn, env)
 DEF_HELPER_1(wait, noreturn, env)
 DEF_HELPER_2(rxint, noreturn, env, i32)
 DEF_HELPER_1(rxbrk, noreturn, env)
-DEF_HELPER_FLAGS_3(fadd, TCG_CALL_NO_WG, f32, env, f32, f32)
-DEF_HELPER_FLAGS_3(fsub, TCG_CALL_NO_WG, f32, env, f32, f32)
-DEF_HELPER_FLAGS_3(fmul, TCG_CALL_NO_WG, f32, env, f32, f32)
-DEF_HELPER_FLAGS_3(fdiv, TCG_CALL_NO_WG, f32, env, f32, f32)
-DEF_HELPER_FLAGS_3(fcmp, TCG_CALL_NO_WG, void, env, f32, f32)
-DEF_HELPER_FLAGS_2(ftoi, TCG_CALL_NO_WG, i32, env, f32)
-DEF_HELPER_FLAGS_2(round, TCG_CALL_NO_WG, i32, env, f32)
-DEF_HELPER_FLAGS_2(itof, TCG_CALL_NO_WG, f32, env, i32)
+DEF_HELPER_3(fadd, f32, env, f32, f32)
+DEF_HELPER_3(fsub, f32, env, f32, f32)
+DEF_HELPER_3(fmul, f32, env, f32, f32)
+DEF_HELPER_3(fdiv, f32, env, f32, f32)
+DEF_HELPER_3(fcmp, void, env, f32, f32)
+DEF_HELPER_2(ftoi, i32, env, f32)
+DEF_HELPER_2(round, i32, env, f32)
+DEF_HELPER_2(itof, f32, env, i32)
 DEF_HELPER_2(set_fpsw, void, env, i32)
-DEF_HELPER_FLAGS_2(racw, TCG_CALL_NO_WG, void, env, i32)
-DEF_HELPER_FLAGS_2(set_psw_rte, TCG_CALL_NO_WG, void, env, i32)
-DEF_HELPER_FLAGS_2(set_psw, TCG_CALL_NO_WG, void, env, i32)
+DEF_HELPER_2(racw, void, env, i32)
+DEF_HELPER_2(set_psw_rte, void, env, i32)
+DEF_HELPER_2(set_psw, void, env, i32)
 DEF_HELPER_1(pack_psw, i32, env)
-DEF_HELPER_FLAGS_3(div, TCG_CALL_NO_WG, i32, env, i32, i32)
-DEF_HELPER_FLAGS_3(divu, TCG_CALL_NO_WG, i32, env, i32, i32)
-DEF_HELPER_FLAGS_1(scmpu, TCG_CALL_NO_WG, void, env)
+DEF_HELPER_3(div, i32, env, i32, i32)
+DEF_HELPER_3(divu, i32, env, i32, i32)
+DEF_HELPER_1(scmpu, void, env)
 DEF_HELPER_1(smovu, void, env)
 DEF_HELPER_1(smovf, void, env)
 DEF_HELPER_1(smovb, void, env)
 DEF_HELPER_2(sstr, void, env, i32)
-DEF_HELPER_FLAGS_2(swhile, TCG_CALL_NO_WG, void, env, i32)
-DEF_HELPER_FLAGS_2(suntil, TCG_CALL_NO_WG, void, env, i32)
-DEF_HELPER_FLAGS_2(rmpa, TCG_CALL_NO_WG, void, env, i32)
+DEF_HELPER_2(swhile, void, env, i32)
+DEF_HELPER_2(suntil, void, env, i32)
+DEF_HELPER_2(rmpa, void, env, i32)
 DEF_HELPER_1(satr, void, env)
-- 
2.47.2

[PATCH 3/4] hw/rx: Reset the CPU at qemu reset time

[Keith Packard via]

This ensure that the CPU gets reset every time QEMU resets. Use either
the kernel entry point or the reset vector if no kernel was loaded.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 hw/rx/rx-gdbsim.c | 36 +++++++++++++++++++++++++++++++++++-
 target/rx/cpu.c   |  9 ++-------
 target/rx/cpu.h   |  3 +++
 3 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/hw/rx/rx-gdbsim.c b/hw/rx/rx-gdbsim.c
index 4afd77efd5..9e395ae345 100644
--- a/hw/rx/rx-gdbsim.c
+++ b/hw/rx/rx-gdbsim.c
@@ -22,6 +22,7 @@
 #include "qemu/guest-random.h"
 #include "qemu/units.h"
 #include "qapi/error.h"
+#include "exec/cpu_ldst.h"
 #include "hw/loader.h"
 #include "hw/rx/rx62n.h"
 #include "system/qtest.h"
@@ -56,6 +57,34 @@ DECLARE_OBJ_CHECKERS(RxGdbSimMachineState, RxGdbSimMachineClass,
                      RX_GDBSIM_MACHINE, TYPE_RX_GDBSIM_MACHINE)
 
 
+static void rx_cpu_reset(void *opaque)
+{
+    RXCPU *cpu = opaque;
+    CPUState *cs = CPU(cpu);
+    CPURXState *env = cpu_env(cs);
+
+    cpu_reset(cs);
+
+    if (env->use_reset_pc) {
+        /*
+         * Load the PC with the starting address for the kernel
+         */
+        env->pc = env->reset_pc;
+    } else {
+        /*
+         * Load the initial PC from the reset vector. If there is
+         * a ROM containing that vector use that, otherwise read
+         * it from target memory.
+         */
+        uint32_t *resetvec_p = rom_ptr_for_as(cs->as, 0xfffffffc, 4);
+        if (resetvec_p) {
+            env->pc = ldl_p(resetvec_p);
+        } else {
+            env->pc = cpu_ldl_data(env, 0xfffffffc);
+        }
+    }
+}
+
 static void rx_load_image(RXCPU *cpu, const char *filename,
                           uint32_t start, uint32_t size)
 {
@@ -68,7 +97,8 @@ static void rx_load_image(RXCPU *cpu, const char *filename,
         fprintf(stderr, "qemu: could not load kernel '%s'\n", filename);
         exit(1);
     }
-    cpu->env.pc = start;
+    cpu->env.reset_pc = start;
+    cpu->env.use_reset_pc = true;
 
     /* setup exception trap trampoline */
     /* linux kernel only works little-endian mode */
@@ -87,6 +117,7 @@ static void rx_gdbsim_init(MachineState *machine)
     const char *kernel_filename = machine->kernel_filename;
     const char *dtb_filename = machine->dtb;
     uint8_t rng_seed[32];
+    CPUState *cs;
 
     if (machine->ram_size < mc->default_ram_size) {
         char *sz = size_to_str(mc->default_ram_size);
@@ -153,6 +184,9 @@ static void rx_gdbsim_init(MachineState *machine)
             s->mcu.cpu.env.regs[1] = SDRAM_BASE + dtb_offset;
         }
     }
+    for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
+        qemu_register_reset(rx_cpu_reset, RX_CPU(cs));
+    }
 }
 
 static void rx_gdbsim_class_init(ObjectClass *oc, void *data)
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index 37a6fdd569..528cda486c 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -76,7 +76,6 @@ 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;
 
     if (rcc->parent_phases.hold) {
         rcc->parent_phases.hold(obj, type);
@@ -84,11 +83,6 @@ 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);
-    }
     rx_cpu_unpack_psw(env, 0, 1);
     env->regs[0] = env->isp = env->usp = 0;
     env->fpsw = 0;
@@ -142,7 +136,6 @@ static void rx_cpu_realize(DeviceState *dev, Error **errp)
     }
 
     qemu_init_vcpu(cs);
-    cpu_reset(cs);
 
     rcc->parent_realize(dev, errp);
 }
@@ -189,6 +182,8 @@ static void rx_cpu_init(Object *obj)
 {
     RXCPU *cpu = RX_CPU(obj);
 
+    cpu->env.reset_pc = 0;
+    cpu->env.use_reset_pc = false;
     qdev_init_gpio_in(DEVICE(cpu), rx_cpu_set_irq, 2);
 }
 
diff --git a/target/rx/cpu.h b/target/rx/cpu.h
index 5ba1874bd7..c42a03efb3 100644
--- a/target/rx/cpu.h
+++ b/target/rx/cpu.h
@@ -98,6 +98,9 @@ typedef struct CPUArchState {
     uint32_t ack_ipl;           /* execute ipl */
     float_status fp_status;
     qemu_irq ack;               /* Interrupt acknowledge */
+
+    bool use_reset_pc;          /* Use reset_pc instead of reset vector */
+    uint32_t reset_pc;          /* PC reset value when use_reset_pc */
 } CPURXState;
 
 /*
-- 
2.47.2

[PATCH 4/4] rx: Support loading of ELF files too

[Keith Packard via]

The existing loader supports raw binary blobs with the entry point
defined as the start of the blob. Add support for loading ELF files by
first checking if the provided filename has a valid ELF header,
falling back to the existing loader code when that fails.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 hw/rx/rx-gdbsim.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/hw/rx/rx-gdbsim.c b/hw/rx/rx-gdbsim.c
index 9e395ae345..58492b713f 100644
--- a/hw/rx/rx-gdbsim.c
+++ b/hw/rx/rx-gdbsim.c
@@ -25,6 +25,7 @@
 #include "exec/cpu_ldst.h"
 #include "hw/loader.h"
 #include "hw/rx/rx62n.h"
+#include "elf.h"
 #include "system/qtest.h"
 #include "system/device_tree.h"
 #include "system/reset.h"
@@ -85,6 +86,37 @@ static void rx_cpu_reset(void *opaque)
     }
 }
 
+static bool rx_load_elf(RXCPU *cpu, const char *filename)
+{
+    CPUState *cs = CPU(cpu);
+    Elf32_Ehdr elf_header;
+    bool elf_is64;
+    Error *err = NULL;
+    uint64_t lowaddr, highaddr, entry;
+    ssize_t ret;
+
+    load_elf_hdr(filename, &elf_header, &elf_is64, &err);
+    if (err) {
+        error_free(err);
+        return false;
+    }
+
+    ret = load_elf_as(filename, NULL, NULL, NULL,
+                      &entry, &lowaddr, &highaddr, NULL, false,
+                      EM_RX, 1, 0, cs->as);
+
+    if (ret <= 0) {
+        /* The header loaded but the image didn't */
+        error_report("qemu: could not load elf '%s': %s",
+                     filename, load_elf_strerror(ret));
+        exit(1);
+    }
+
+    cpu->env.reset_pc = entry;
+    cpu->env.use_reset_pc = true;
+    return true;
+}
+
 static void rx_load_image(RXCPU *cpu, const char *filename,
                           uint32_t start, uint32_t size)
 {
@@ -92,6 +124,10 @@ static void rx_load_image(RXCPU *cpu, const char *filename,
     long kernel_size;
     int i;
 
+    if (rx_load_elf(cpu, filename)) {
+        return;
+    }
+
     kernel_size = load_image_targphys(filename, start, size);
     if (kernel_size < 0) {
         fprintf(stderr, "qemu: could not load kernel '%s'\n", filename);
-- 
2.47.2

Re: [PATCH 2/4] target/rx: Remove TCG_CALL_NO_WG from helpers which write env

[Peter Maydell]

On Tue, 18 Feb 2025 at 21:22, Keith Packard via <qemu-devel@nongnu.org> wrote:
>
> Functions which modify virtual machine state (such as virtual
> registers stored in memory) must not be marked TCG_CALL_NO_WG

More accurately, functions which write to TCG globals.
It's fine for a function which modifies virtual machine
state to be marked TCG_CALL_NO_WG as long as that
virtual machine state isn't stored in a TCG global.

> as that
> tells the optimizer that virtual registers values already loaded in
> machine registers are still valid, hence discards any changes which
> these helpers may have made. This seems to also mean that functions which
> set condition codes may also not use this flag

...because target/rx makes the (sensible) choice to put its
condition codes in TCG globals.

> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  target/rx/helper.h | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 1/4] target/rx: Set exception vector base to 0xffffff80

[Peter Maydell]

On Tue, 18 Feb 2025 at 21:22, Keith Packard via <qemu-devel@nongnu.org> wrote:
>
> The documentation says the vector is at 0xffffff80, instead of the
> previous value of 0xffffffc0. That value must have been a bug because
> the standard vector values (20, 21, 23, 25, 30) were all
> past the end of the array.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  target/rx/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/rx/helper.c b/target/rx/helper.c
> index 80912e8dcb..55e2ae4a11 100644
> --- a/target/rx/helper.c
> +++ b/target/rx/helper.c
> @@ -90,7 +90,7 @@ void rx_cpu_do_interrupt(CPUState *cs)
>          cpu_stl_data(env, env->isp, env->pc);
>
>          if (vec < 0x100) {
> -            env->pc = cpu_ldl_data(env, 0xffffffc0 + vec * 4);
> +            env->pc = cpu_ldl_data(env, 0xffffff80 + vec * 4);
>          } else {
>              env->pc = cpu_ldl_data(env, env->intb + (vec & 0xff) * 4);
>          }
> --

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 3/4] hw/rx: Reset the CPU at qemu reset time

[Peter Maydell]

On Tue, 18 Feb 2025 at 21:22, Keith Packard via <qemu-devel@nongnu.org> wrote:
>
> This ensure that the CPU gets reset every time QEMU resets. Use either
> the kernel entry point or the reset vector if no kernel was loaded.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  hw/rx/rx-gdbsim.c | 36 +++++++++++++++++++++++++++++++++++-
>  target/rx/cpu.c   |  9 ++-------
>  target/rx/cpu.h   |  3 +++
>  3 files changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/hw/rx/rx-gdbsim.c b/hw/rx/rx-gdbsim.c
> index 4afd77efd5..9e395ae345 100644
> --- a/hw/rx/rx-gdbsim.c
> +++ b/hw/rx/rx-gdbsim.c
> @@ -22,6 +22,7 @@
>  #include "qemu/guest-random.h"
>  #include "qemu/units.h"
>  #include "qapi/error.h"
> +#include "exec/cpu_ldst.h"
>  #include "hw/loader.h"
>  #include "hw/rx/rx62n.h"
>  #include "system/qtest.h"
> @@ -56,6 +57,34 @@ DECLARE_OBJ_CHECKERS(RxGdbSimMachineState, RxGdbSimMachineClass,
>                       RX_GDBSIM_MACHINE, TYPE_RX_GDBSIM_MACHINE)
>
>
> +static void rx_cpu_reset(void *opaque)
> +{
> +    RXCPU *cpu = opaque;
> +    CPUState *cs = CPU(cpu);
> +    CPURXState *env = cpu_env(cs);
> +
> +    cpu_reset(cs);
> +
> +    if (env->use_reset_pc) {
> +        /*
> +         * Load the PC with the starting address for the kernel
> +         */
> +        env->pc = env->reset_pc;
> +    } else {
> +        /*
> +         * Load the initial PC from the reset vector. If there is
> +         * a ROM containing that vector use that, otherwise read
> +         * it from target memory.
> +         */
> +        uint32_t *resetvec_p = rom_ptr_for_as(cs->as, 0xfffffffc, 4);
> +        if (resetvec_p) {
> +            env->pc = ldl_p(resetvec_p);
> +        } else {
> +            env->pc = cpu_ldl_data(env, 0xfffffffc);
> +        }
> +    }
> +}

Unless there's a strong reason for doing something different,
I would favour following the same pattern arm does for this.
(Or were you following existing code in some other target?
I certainly wouldn't be surprised if we already did this in
multiple different ways...)

Anyway, Arm splits up the work like this:
 * the CPU reset function does the "load initial PC from
   reset vector table" part (including using rom_ptr_for_as()
   to decide whether to do cpu_ldl_data() or not)
 * the board boot code's reset function does:
    cpu_reset();
    if (need to override the start PC because of the image loaded) {
        cpu_set_pc(cs, image_pc);
    }
    /* and any other CPU setup that's specific to kernel load etc */

That way if the user chooses to use the 'generic loader'
(-device loader) to load their guest image rather than
-kernel, we will correctly load the reset PC out
of their image.

You might then prefer to put the initial image_pc into
the RxGdbSimMachineState instead of the CPURXState,
since the code that cares about it directly is all
in hw/rx/ rather than target/rx/.

thanks
-- PMM

Re: [PATCH 4/4] rx: Support loading of ELF files too

[Peter Maydell]

On Tue, 18 Feb 2025 at 21:23, Keith Packard via <qemu-devel@nongnu.org> wrote:
>
> The existing loader supports raw binary blobs with the entry point
> defined as the start of the blob. Add support for loading ELF files by
> first checking if the provided filename has a valid ELF header,
> falling back to the existing loader code when that fails.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>

Ths code does what it intends to, and I'm not saying we should
definitely *not* have it. I do think it's worth considering whether
we need it, given that you can already load an ELF image via the
generic loader (-device loader).

-kernel is a very "do what I mean" option that differs from target to
target, but what it's ideally supposed to mean is "this is a Linux
kernel, boot it like a Linux kernel". Some of our targets also
overload it to additionally have the behaviour "but if it's an ELF
file, load it as an ELF file, not like a kernel" (notably at least
arm M-profile). But I think that's something I'd consider a mistake
in retrospect, which we can't change now for backwards compatibility
reasons. (In particular it means we can't have "-kernel kernel.elf"
mean "this is a Linux kernel which is an ELF file and it wants
all the usual CPU setup that Linux's booting requirements mandate".)

So for target/rx, I guess what I'm saying is that if Linux
kernels for this architecture really are usually floating around
as ELF files then we should support that, but if this is just
"some users would like a way to load an ELF file, start at the
ELF entry point, and do no other setup of the CPU or system before
running code" then it might be better to instead point those
users at the generic loader (which does exactly that job
and does the same thing on all target architectures),
rather than adding a second mechanism for doing that.

thanks
-- PMM

Re: [PATCH 0/4] Renesas RX target fixes (v2)

[Peter Maydell]

On Tue, 18 Feb 2025 at 21:22, Keith Packard via <qemu-devel@nongnu.org> wrote:
>
> With feedback from Peter Maydell and Richard Henderson, I've updated
> this series to address two concerns:
>
>  1. The hardware model is now responsible for guiding the CPU reset
>     process.
>
>  2. Loading the reset vector from memory is now delayed until cpu_reset
>     is finished to ensure memory_dispatch is initialized.
>
> First, there are two critical flaws in the emulation. These are
> needed for this model to work correctly:
>
>  1. The exception vector base is 0xffffff80 not 0xffffffc0. This
>     prevents exceptions from working at all.
>
>  2. Many tcg helpers inappropriately used TCG_CALL_NO_WG even though
>     they modified virtual registers stored in global memory. This
>     causes these operations to fail unless one-insn-per-tb was enabled.
>
> The third patch changes how the cpu is reset so that it is driven by
> the hw code instead of the target code. Now the cpu is reset each time
> qemu is reset and the initial PC value is either set from the loaded
> kernel or from the reset vector. This should look a lot more like how
> other models manage this process.
>
> The final patch adds the ability to load an ELF file rather than
> a binary memory image. It's purely a new feature and not required for
> this model to be usable; without this, it's fairly easy to use
> the loader device; that just requires the loaded image to include the
> exception vectors with the correct reset vector value.
>
> Keith Packard (4):
>   target/rx: Set exception vector base to 0xffffff80
>   target/rx: Remove TCG_CALL_NO_WG from helpers which write env
>   hw/rx: Reset the CPU at qemu reset time
>   rx: Support loading of ELF files too

Apologies for having taken nearly a month to get to this series.
I had review comments on patches 3 and 4, but patches 1 and 2
are good to go and so I've taken them inte target-arm.next.

-- PMM

Re: [PATCH 4/4] rx: Support loading of ELF files too

[Keith Packard via]

[-- Attachment #1: Type: text/plain, Size: 614 bytes --]


> Ths code does what it intends to, and I'm not saying we should
> definitely *not* have it. I do think it's worth considering whether
> we need it, given that you can already load an ELF image via the
> generic loader (-device loader).

Also sorry for not getting back to this; we've been busy with other
Zephyr toolchain adventures. Of course, I've lost a lot of state related
to this, but to the best of my memory, I added this change while I was
trying to get the reset logic sorted out; using -kernel made that
easier, but this patch is no longer necessary. So let's drop this one.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 3/4] hw/rx: Reset the CPU at qemu reset time

[Keith Packard via]

[-- Attachment #1.1: Type: text/plain, Size: 277 bytes --]

From: Peter Maydell <peter.maydell@linaro.org>
Date: Fri, 07 Mar 2025 13:26:14 +0000

> Unless there's a strong reason for doing something different,
> I would favour following the same pattern arm does for this.

Thanks for the suggested cleanup; this looks a lot nicer now.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-hw-rx-Reset-the-CPU-at-qemu-reset-time.patch --]
[-- Type: text/x-diff, Size: 5188 bytes --]

From 02d0f2b006500dec62e91bd571a8722c354133e5 Mon Sep 17 00:00:00 2001
From: Keith Packard <keithp@keithp.com>
Date: Wed, 12 Feb 2025 16:42:38 -0800
Subject: [PATCH] hw/rx: Reset the CPU at qemu reset time

This ensure that the CPU gets reset every time QEMU resets. Use either
the kernel entry point or the reset vector if no kernel was loaded.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 hw/rx/rx-gdbsim.c | 31 ++++++++++++++++++++++++++++++-
 target/rx/cpu.c   | 20 +++++++++++++-------
 2 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/hw/rx/rx-gdbsim.c b/hw/rx/rx-gdbsim.c
index 5b9004e9e1..5765cb91c6 100644
--- a/hw/rx/rx-gdbsim.c
+++ b/hw/rx/rx-gdbsim.c
@@ -22,6 +22,7 @@
 #include "qemu/guest-random.h"
 #include "qemu/units.h"
 #include "qapi/error.h"
+#include "accel/tcg/cpu-ldst.h"
 #include "hw/loader.h"
 #include "hw/rx/rx62n.h"
 #include "system/qtest.h"
@@ -47,6 +48,9 @@ struct RxGdbSimMachineState {
     MachineState parent_obj;
     /*< public >*/
     RX62NState mcu;
+
+    bool use_reset_pc;          /* Use reset_pc instead of reset vector */
+    uint32_t reset_pc;          /* PC reset value when use_reset_pc */
 };
 typedef struct RxGdbSimMachineState RxGdbSimMachineState;
 
@@ -56,9 +60,29 @@ DECLARE_OBJ_CHECKERS(RxGdbSimMachineState, RxGdbSimMachineClass,
                      RX_GDBSIM_MACHINE, TYPE_RX_GDBSIM_MACHINE)
 
 
+static void rx_cpu_reset(void *opaque)
+{
+    RXCPU *cpu = opaque;
+    CPUState *cs = CPU(cpu);
+    CPURXState *env = cpu_env(cs);
+    MachineState *machine = MACHINE(qdev_get_machine());
+    RxGdbSimMachineState *s = RX_GDBSIM_MACHINE(machine);
+
+    cpu_reset(cs);
+
+    if (s->use_reset_pc) {
+        /*
+         * Load the PC with the starting address for the kernel
+         */
+        env->pc = s->reset_pc;
+    }
+}
+
 static void rx_load_image(RXCPU *cpu, const char *filename,
                           uint32_t start, uint32_t size)
 {
+    MachineState *machine = MACHINE(qdev_get_machine());
+    RxGdbSimMachineState *s = RX_GDBSIM_MACHINE(machine);
     static uint32_t extable[32];
     long kernel_size;
     int i;
@@ -68,7 +92,8 @@ static void rx_load_image(RXCPU *cpu, const char *filename,
         fprintf(stderr, "qemu: could not load kernel '%s'\n", filename);
         exit(1);
     }
-    cpu->env.pc = start;
+    s->reset_pc = start;
+    s->use_reset_pc = true;
 
     /* setup exception trap trampoline */
     /* linux kernel only works little-endian mode */
@@ -87,6 +112,7 @@ static void rx_gdbsim_init(MachineState *machine)
     const char *kernel_filename = machine->kernel_filename;
     const char *dtb_filename = machine->dtb;
     uint8_t rng_seed[32];
+    CPUState *cs;
 
     if (machine->ram_size < mc->default_ram_size) {
         char *sz = size_to_str(mc->default_ram_size);
@@ -153,6 +179,9 @@ static void rx_gdbsim_init(MachineState *machine)
             s->mcu.cpu.env.regs[1] = SDRAM_BASE + dtb_offset;
         }
     }
+    for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
+        qemu_register_reset(rx_cpu_reset, RX_CPU(cs));
+    }
 }
 
 static void rx_gdbsim_class_init(ObjectClass *oc, const void *data)
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index c6dd5d6f83..4d568bbd92 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -29,6 +29,7 @@
 #include "fpu/softfloat.h"
 #include "tcg/debug-assert.h"
 #include "accel/tcg/cpu-ops.h"
+#include "accel/tcg/cpu-ldst.h"
 
 static void rx_cpu_set_pc(CPUState *cs, vaddr value)
 {
@@ -89,7 +90,6 @@ 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;
 
     if (rcc->parent_phases.hold) {
         rcc->parent_phases.hold(obj, type);
@@ -97,11 +97,6 @@ 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);
-    }
     rx_cpu_unpack_psw(env, 0, 1);
     env->regs[0] = env->isp = env->usp = 0;
     env->fpsw = 0;
@@ -124,6 +119,18 @@ static void rx_cpu_reset_hold(Object *obj, ResetType type)
      * be the correct setting.
      */
     set_float_ftz_detection(float_ftz_before_rounding, &env->fp_status);
+
+    /*
+     * Load the initial PC from the reset vector. If there is
+     * a ROM containing that vector use that, otherwise read
+     * it from target memory.
+     */
+    uint32_t *resetvec_p = rom_ptr_for_as(cs->as, 0xfffffffc, 4);
+    if (resetvec_p) {
+        env->pc = ldl_p(resetvec_p);
+    } else {
+        env->pc = cpu_ldl_data(env, 0xfffffffc);
+    }
 }
 
 static ObjectClass *rx_cpu_class_by_name(const char *cpu_model)
@@ -155,7 +162,6 @@ static void rx_cpu_realize(DeviceState *dev, Error **errp)
     }
 
     qemu_init_vcpu(cs);
-    cpu_reset(cs);
 
     rcc->parent_realize(dev, errp);
 }
-- 
2.49.0


[-- Attachment #1.3: Type: text/plain, Size: 243 bytes --]


Btw, I went and looked at other qemu target support for -kernel; a whole
bunch support loading of arbitrary .elf files using that parameter. I
had to temporarily add that patch back in to test the enclosed reset
change...

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]