diff mbox series

[PATCH-for-10.1,1/4] target/riscv: Restrict RV128 MTTCG check on system emulation

Message ID 20250321155925.96626-2-philmd@linaro.org
State Superseded
Headers show
Series tcg: Convert TARGET_SUPPORTS_MTTCG to TCGCPUOps::mttcg_supported field | expand

Commit Message

Philippe Mathieu-Daudé March 21, 2025, 3:59 p.m. UTC 
Multi-threaded TCG only concerns system emulation.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/riscv/tcg/tcg-cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Pierrick Bouvier March 21, 2025, 5:32 p.m. UTC | #1 
On 3/21/25 08:59, Philippe Mathieu-Daudé wrote:
> Multi-threaded TCG only concerns system emulation.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/riscv/tcg/tcg-cpu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index fb903992faa..60a26acc503 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -1050,6 +1050,7 @@ static bool riscv_tcg_cpu_realize(CPUState *cs, Error **errp)
>           return false;
>       }
>   
> +#ifndef CONFIG_USER_ONLY
>       if (mcc->misa_mxl_max >= MXL_RV128 && qemu_tcg_mttcg_enabled()) {
>           /* Missing 128-bit aligned atomics */
>           error_setg(errp,
> @@ -1058,7 +1059,6 @@ static bool riscv_tcg_cpu_realize(CPUState *cs, Error **errp)
>           return false;
>       }
>   
> -#ifndef CONFIG_USER_ONLY
>       CPURISCVState *env = &cpu->env;
>   
>       tcg_cflags_set(CPU(cs), CF_PCREL);

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Richard Henderson March 23, 2025, 6:08 p.m. UTC | #2 
On 3/21/25 08:59, Philippe Mathieu-Daudé wrote:
> Multi-threaded TCG only concerns system emulation.

That's not really true.  User emulation simply has no option to
run in a single-threaded context.

I really don't think we should allow RV128 in user-mode at all.
Certainly not until there's a kernel abi for it.


r~
Philippe Mathieu-Daudé April 2, 2025, 2:25 p.m. UTC | #3 
On 23/3/25 19:08, Richard Henderson wrote:
> On 3/21/25 08:59, Philippe Mathieu-Daudé wrote:
>> Multi-threaded TCG only concerns system emulation.
> 
> That's not really true.  User emulation simply has no option to
> run in a single-threaded context.
> 
> I really don't think we should allow RV128 in user-mode at all.
> Certainly not until there's a kernel abi for it.

It seems to be safe since commit 905b9fcde1f ("target/riscv: Replace
is_32bit with get_xl/get_xlen"):

  #ifdef TARGET_RISCV32
  #define get_xl(ctx)    MXL_RV32
  #elif defined(CONFIG_USER_ONLY)
  #define get_xl(ctx)    MXL_RV64
  #else
  #define get_xl(ctx)    ((ctx)->xl)
  #endif

Should we undefine MXL_RV128 on user-mode?
Philippe Mathieu-Daudé April 2, 2025, 2:40 p.m. UTC | #4 
On 2/4/25 16:25, Philippe Mathieu-Daudé wrote:
> On 23/3/25 19:08, Richard Henderson wrote:
>> On 3/21/25 08:59, Philippe Mathieu-Daudé wrote:
>>> Multi-threaded TCG only concerns system emulation.
>>
>> That's not really true.  User emulation simply has no option to
>> run in a single-threaded context.
>>
>> I really don't think we should allow RV128 in user-mode at all.
>> Certainly not until there's a kernel abi for it.
> 
> It seems to be safe since commit 905b9fcde1f ("target/riscv: Replace
> is_32bit with get_xl/get_xlen"):
> 
>   #ifdef TARGET_RISCV32
>   #define get_xl(ctx)    MXL_RV32
>   #elif defined(CONFIG_USER_ONLY)
>   #define get_xl(ctx)    MXL_RV64
>   #else
>   #define get_xl(ctx)    ((ctx)->xl)
>   #endif
> 
> Should we undefine MXL_RV128 on user-mode?

Indeed the CPU is exposed on user-mode...

$ qemu-riscv64 -cpu help
Available CPUs:
   max
   rv64
   rv64e
   rv64i
   rva22s64
   rva22u64
   rva23s64
   rva23u64
   shakti-c
   sifive-e51
   sifive-u54
   thead-c906
   tt-ascalon
   veyron-v1
   x-rv128           <---------
   xiangshan-nanhu

Per commit 6df3747a274 ("riscv: Introduce satp mode hw
capabilities") I wonder if this is expected.

Anyhow, I'll post a patch disabling it as:

-- >8 --
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 430b02d2a58..33abcef0073 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -699,3 +699,3 @@ static void rv64_xiangshan_nanhu_cpu_init(Object *obj)

-#ifdef CONFIG_TCG
+#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
  static void rv128_base_cpu_init(Object *obj)
@@ -710,7 +710,6 @@ static void rv128_base_cpu_init(Object *obj)
      env->priv_ver = PRIV_VERSION_LATEST;
-#ifndef CONFIG_USER_ONLY
+
      set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV57);
-#endif
  }
-#endif /* CONFIG_TCG */
+#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */

@@ -3257,3 +3256,3 @@ static const TypeInfo riscv_cpu_type_infos[] = {
                                                   MXL_RV64, 
rv64_xiangshan_nanhu_cpu_init),
-#ifdef CONFIG_TCG
+#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
      DEFINE_DYNAMIC_CPU(TYPE_RISCV_CPU_BASE128,   MXL_RV128, 
rv128_base_cpu_init),

---
Alistair Francis April 4, 2025, 3:22 a.m. UTC | #5 
On Thu, Apr 3, 2025 at 12:41 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 2/4/25 16:25, Philippe Mathieu-Daudé wrote:
> > On 23/3/25 19:08, Richard Henderson wrote:
> >> On 3/21/25 08:59, Philippe Mathieu-Daudé wrote:
> >>> Multi-threaded TCG only concerns system emulation.
> >>
> >> That's not really true.  User emulation simply has no option to
> >> run in a single-threaded context.
> >>
> >> I really don't think we should allow RV128 in user-mode at all.
> >> Certainly not until there's a kernel abi for it.
> >
> > It seems to be safe since commit 905b9fcde1f ("target/riscv: Replace
> > is_32bit with get_xl/get_xlen"):
> >
> >   #ifdef TARGET_RISCV32
> >   #define get_xl(ctx)    MXL_RV32
> >   #elif defined(CONFIG_USER_ONLY)
> >   #define get_xl(ctx)    MXL_RV64
> >   #else
> >   #define get_xl(ctx)    ((ctx)->xl)
> >   #endif
> >
> > Should we undefine MXL_RV128 on user-mode?
>
> Indeed the CPU is exposed on user-mode...
>
> $ qemu-riscv64 -cpu help
> Available CPUs:
>    max
>    rv64
>    rv64e
>    rv64i
>    rva22s64
>    rva22u64
>    rva23s64
>    rva23u64
>    shakti-c
>    sifive-e51
>    sifive-u54
>    thead-c906
>    tt-ascalon
>    veyron-v1
>    x-rv128           <---------
>    xiangshan-nanhu
>
> Per commit 6df3747a274 ("riscv: Introduce satp mode hw
> capabilities") I wonder if this is expected.

We probably didn't really think about it at the time.

I agree that we don't need it

>
> Anyhow, I'll post a patch disabling it as:

Thanks

Alistair

>
> -- >8 --
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 430b02d2a58..33abcef0073 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -699,3 +699,3 @@ static void rv64_xiangshan_nanhu_cpu_init(Object *obj)
>
> -#ifdef CONFIG_TCG
> +#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
>   static void rv128_base_cpu_init(Object *obj)
> @@ -710,7 +710,6 @@ static void rv128_base_cpu_init(Object *obj)
>       env->priv_ver = PRIV_VERSION_LATEST;
> -#ifndef CONFIG_USER_ONLY
> +
>       set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV57);
> -#endif
>   }
> -#endif /* CONFIG_TCG */
> +#endif /* CONFIG_TCG && !CONFIG_USER_ONLY */
>
> @@ -3257,3 +3256,3 @@ static const TypeInfo riscv_cpu_type_infos[] = {
>                                                    MXL_RV64,
> rv64_xiangshan_nanhu_cpu_init),
> -#ifdef CONFIG_TCG
> +#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
>       DEFINE_DYNAMIC_CPU(TYPE_RISCV_CPU_BASE128,   MXL_RV128,
> rv128_base_cpu_init),
>
> ---
>
diff mbox series

Patch

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index fb903992faa..60a26acc503 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -1050,6 +1050,7 @@  static bool riscv_tcg_cpu_realize(CPUState *cs, Error **errp)
         return false;
     }
 
+#ifndef CONFIG_USER_ONLY
     if (mcc->misa_mxl_max >= MXL_RV128 && qemu_tcg_mttcg_enabled()) {
         /* Missing 128-bit aligned atomics */
         error_setg(errp,
@@ -1058,7 +1059,6 @@  static bool riscv_tcg_cpu_realize(CPUState *cs, Error **errp)
         return false;
     }
 
-#ifndef CONFIG_USER_ONLY
     CPURISCVState *env = &cpu->env;
 
     tcg_cflags_set(CPU(cs), CF_PCREL);