[PATCH] target/rx/cpu: Use address_space_ldl() to read reset vector address

Philippe Mathieu-Daudé posted 1 patch 4 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200315134859.9547-1-f4bug@amsat.org
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test checkpatch passed
Test FreeBSD passed
Test asan passed
target/rx/cpu.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
[PATCH] target/rx/cpu: Use address_space_ldl() to read reset vector address
Posted by Philippe Mathieu-Daudé 4 years ago
From: Philippe Mathieu-Daudé <philmd@redhat.com>

The RX code flash is not a Masked ROM but a EEPROM (electrically
erasable programmable flash memory).
When implementing the flash hardware, the rom_ptr() returns NULL
and the reset vector is not set.
Instead, use the address_space ld/st API to fetch the reset vector
address from the code flash.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Based-on: <20200315132810.7022-1-f4bug@amsat.org>

Same issue might occurs in Cortex-M arm_cpu_reset()
---
 target/rx/cpu.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index 9c224a273c..d3bd09e753 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -26,6 +26,8 @@
 #include "hw/loader.h"
 #include "fpu/softfloat.h"
 
+#define CPU_RESET_VECTOR 0xfffffffc
+
 static void rx_cpu_set_pc(CPUState *cs, vaddr value)
 {
     RXCPU *cpu = RXCPU(cs);
@@ -51,17 +53,13 @@ static void rx_cpu_reset(CPUState *s)
     RXCPU *cpu = RXCPU(s);
     RXCPUClass *rcc = RXCPU_GET_CLASS(cpu);
     CPURXState *env = &cpu->env;
-    uint32_t *resetvec;
 
     rcc->parent_reset(s);
 
     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);
-    }
+    env->pc = address_space_ldl(cpu_get_address_space(s, 0),
+                                CPU_RESET_VECTOR, MEMTXATTRS_UNSPECIFIED, NULL);
     rx_cpu_unpack_psw(env, 0, 1);
     env->regs[0] = env->isp = env->usp = 0;
     env->fpsw = 0;
-- 
2.21.1


Re: [PATCH] target/rx/cpu: Use address_space_ldl() to read reset vector address
Posted by Philippe Mathieu-Daudé 4 years ago
On 3/15/20 2:48 PM, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>

Hmm author email should be <f4bug@amsat.org>...

> 
> The RX code flash is not a Masked ROM but a EEPROM (electrically
> erasable programmable flash memory).
> When implementing the flash hardware, the rom_ptr() returns NULL
> and the reset vector is not set.
> Instead, use the address_space ld/st API to fetch the reset vector
> address from the code flash.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Based-on: <20200315132810.7022-1-f4bug@amsat.org>
> 
> Same issue might occurs in Cortex-M arm_cpu_reset()
> ---
>   target/rx/cpu.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/target/rx/cpu.c b/target/rx/cpu.c
> index 9c224a273c..d3bd09e753 100644
> --- a/target/rx/cpu.c
> +++ b/target/rx/cpu.c
> @@ -26,6 +26,8 @@
>   #include "hw/loader.h"
>   #include "fpu/softfloat.h"
>   
> +#define CPU_RESET_VECTOR 0xfffffffc
> +
>   static void rx_cpu_set_pc(CPUState *cs, vaddr value)
>   {
>       RXCPU *cpu = RXCPU(cs);
> @@ -51,17 +53,13 @@ static void rx_cpu_reset(CPUState *s)
>       RXCPU *cpu = RXCPU(s);
>       RXCPUClass *rcc = RXCPU_GET_CLASS(cpu);
>       CPURXState *env = &cpu->env;
> -    uint32_t *resetvec;
>   
>       rcc->parent_reset(s);
>   
>       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);
> -    }
> +    env->pc = address_space_ldl(cpu_get_address_space(s, 0),
> +                                CPU_RESET_VECTOR, MEMTXATTRS_UNSPECIFIED, NULL);
>       rx_cpu_unpack_psw(env, 0, 1);
>       env->regs[0] = env->isp = env->usp = 0;
>       env->fpsw = 0;
> 


Re: [PATCH] target/rx/cpu: Use address_space_ldl() to read reset vector address
Posted by Peter Maydell 4 years ago
On Sun, 15 Mar 2020 at 13:49, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> The RX code flash is not a Masked ROM but a EEPROM (electrically
> erasable programmable flash memory).
> When implementing the flash hardware, the rom_ptr() returns NULL
> and the reset vector is not set.
> Instead, use the address_space ld/st API to fetch the reset vector
> address from the code flash.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Based-on: <20200315132810.7022-1-f4bug@amsat.org>
>
> Same issue might occurs in Cortex-M arm_cpu_reset()

rom_ptr() does not mean "I'm trying to get this from ROM",
it means "I'm trying to get this from a user-supplied ELF
file or similar which hasn't been loaded into guest memory
yet". (This is a workaround for a reset ordering issue where
CPU reset happens before rom_reset() runs.)

Removing the usage of rom_ptr() altogether here doesn't
look right -- have you tested the case where the initial
reset vector contents are provided via -kernel or
-device loader,... ?

thanks
-- PMM

Re: [PATCH] target/rx/cpu: Use address_space_ldl() to read reset vector address
Posted by Richard Henderson 4 years ago
On 3/15/20 6:48 AM, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> The RX code flash is not a Masked ROM but a EEPROM (electrically
> erasable programmable flash memory).
> When implementing the flash hardware, the rom_ptr() returns NULL
> and the reset vector is not set.
> Instead, use the address_space ld/st API to fetch the reset vector
> address from the code flash.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Based-on: <20200315132810.7022-1-f4bug@amsat.org>
> 
> Same issue might occurs in Cortex-M arm_cpu_reset()
> ---
>  target/rx/cpu.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)

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

r~