[v1] target/riscv: profile handling fixes

[PATCH 1/3] target/riscv/tcg: restrict satp_mode changes in cpu_set_profile

Posted by Daniel Henrique Barboza 5 months, 4 weeks ago

We're changing 'mmu' to true regardless of whether the profile is
being enabled or not, and at the same time we're changing satp_mode to
profile->enabled.

This will promote a situation where we'll set mmu=on without a virtual
memory mode, which is a mistake.

Only touch 'mmu' and satp_mode if the profile is being enabled.

Suggested-by: Andrew Jones <ajones@ventanamicro.com>
Fixes: 55398025e7 ("target/riscv: add satp_mode profile support")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/tcg/tcg-cpu.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 55e00972b7..7f93414a76 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -1338,16 +1338,16 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
 
     if (profile->enabled) {
         cpu->env.priv_ver = profile->priv_spec;
-    }
 
 #ifndef CONFIG_USER_ONLY
-    if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) {
-        object_property_set_bool(obj, "mmu", true, NULL);
-        const char *satp_prop = satp_mode_str(profile->satp_mode,
-                                              riscv_cpu_is_32bit(cpu));
-        object_property_set_bool(obj, satp_prop, profile->enabled, NULL);
-    }
+        if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) {
+            object_property_set_bool(obj, "mmu", true, NULL);
+            const char *satp_prop = satp_mode_str(profile->satp_mode,
+                                                  riscv_cpu_is_32bit(cpu));
+            object_property_set_bool(obj, satp_prop, true, NULL);
+        }
 #endif
+    }
 
     for (i = 0; misa_bits[i] != 0; i++) {
         uint32_t bit = misa_bits[i];
-- 
2.49.0

Re: [PATCH 1/3] target/riscv/tcg: restrict satp_mode changes in cpu_set_profile

Posted by Alistair Francis 5 months, 2 weeks ago

On Wed, May 21, 2025 at 3:24 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> We're changing 'mmu' to true regardless of whether the profile is
> being enabled or not, and at the same time we're changing satp_mode to
> profile->enabled.
>
> This will promote a situation where we'll set mmu=on without a virtual
> memory mode, which is a mistake.
>
> Only touch 'mmu' and satp_mode if the profile is being enabled.
>
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Fixes: 55398025e7 ("target/riscv: add satp_mode profile support")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/tcg/tcg-cpu.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 55e00972b7..7f93414a76 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -1338,16 +1338,16 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
>
>      if (profile->enabled) {
>          cpu->env.priv_ver = profile->priv_spec;
> -    }
>
>  #ifndef CONFIG_USER_ONLY
> -    if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) {
> -        object_property_set_bool(obj, "mmu", true, NULL);
> -        const char *satp_prop = satp_mode_str(profile->satp_mode,
> -                                              riscv_cpu_is_32bit(cpu));
> -        object_property_set_bool(obj, satp_prop, profile->enabled, NULL);
> -    }
> +        if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) {
> +            object_property_set_bool(obj, "mmu", true, NULL);
> +            const char *satp_prop = satp_mode_str(profile->satp_mode,
> +                                                  riscv_cpu_is_32bit(cpu));
> +            object_property_set_bool(obj, satp_prop, true, NULL);
> +        }
>  #endif
> +    }
>
>      for (i = 0; misa_bits[i] != 0; i++) {
>          uint32_t bit = misa_bits[i];
> --
> 2.49.0
>
>

Re: [PATCH 1/3] target/riscv/tcg: restrict satp_mode changes in cpu_set_profile

Posted by Björn Töpel 5 months, 4 weeks ago

Daniel Henrique Barboza <dbarboza@ventanamicro.com> writes:

> We're changing 'mmu' to true regardless of whether the profile is
> being enabled or not, and at the same time we're changing satp_mode to
> profile->enabled.
>
> This will promote a situation where we'll set mmu=on without a virtual
> memory mode, which is a mistake.
>
> Only touch 'mmu' and satp_mode if the profile is being enabled.
>
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Fixes: 55398025e7 ("target/riscv: add satp_mode profile support")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Björn Töpel <bjorn@rivosinc.com>

Re: [PATCH 1/3] target/riscv/tcg: restrict satp_mode changes in cpu_set_profile

Posted by Andrew Jones 5 months, 4 weeks ago

On Tue, May 20, 2025 at 02:23:34PM -0300, Daniel Henrique Barboza wrote:
> We're changing 'mmu' to true regardless of whether the profile is
> being enabled or not, and at the same time we're changing satp_mode to
> profile->enabled.
> 
> This will promote a situation where we'll set mmu=on without a virtual
> memory mode, which is a mistake.
> 
> Only touch 'mmu' and satp_mode if the profile is being enabled.
> 
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Fixes: 55398025e7 ("target/riscv: add satp_mode profile support")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/tcg/tcg-cpu.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 55e00972b7..7f93414a76 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -1338,16 +1338,16 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
>  
>      if (profile->enabled) {
>          cpu->env.priv_ver = profile->priv_spec;
> -    }
>  
>  #ifndef CONFIG_USER_ONLY
> -    if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) {
> -        object_property_set_bool(obj, "mmu", true, NULL);
> -        const char *satp_prop = satp_mode_str(profile->satp_mode,
> -                                              riscv_cpu_is_32bit(cpu));
> -        object_property_set_bool(obj, satp_prop, profile->enabled, NULL);
> -    }
> +        if (profile->satp_mode != RISCV_PROFILE_ATTR_UNUSED) {
> +            object_property_set_bool(obj, "mmu", true, NULL);
> +            const char *satp_prop = satp_mode_str(profile->satp_mode,
> +                                                  riscv_cpu_is_32bit(cpu));
> +            object_property_set_bool(obj, satp_prop, true, NULL);
> +        }
>  #endif
> +    }
>  
>      for (i = 0; misa_bits[i] != 0; i++) {
>          uint32_t bit = misa_bits[i];
> -- 
> 2.49.0
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>