1. Patchew
  2. QEMU
  3. [PATCH v2 00/11] single-binary: compile once semihosting
  4. View patch

[PATCH v2 04/11] semihosting/arm-compat-semi: remove common_semi_sys_exit_extended

Pierrick Bouvier posted 11 patches 2 months, 2 weeks ago
Maintainers: "Alex Bennée" <alex.bennee@linaro.org>, Peter Maydell <peter.maydell@linaro.org>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
There is a newer version of this series
[PATCH v2 04/11] semihosting/arm-compat-semi: remove common_semi_sys_exit_extended
Posted by Pierrick Bouvier 2 months, 2 weeks ago 
This allows to get rid of sizeof(target_ulong) for riscv, without
changing the semantic.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 target/arm/common-semi-target.h   | 5 -----
 target/riscv/common-semi-target.h | 5 -----
 semihosting/arm-compat-semi.c     | 4 +++-
 3 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/target/arm/common-semi-target.h b/target/arm/common-semi-target.h
index da51f2d7f54..7ebd2136d93 100644
--- a/target/arm/common-semi-target.h
+++ b/target/arm/common-semi-target.h
@@ -34,11 +34,6 @@ static inline void common_semi_set_ret(CPUState *cs, target_ulong ret)
     }
 }
 
-static inline bool common_semi_sys_exit_extended(CPUState *cs, int nr)
-{
-    return nr == TARGET_SYS_EXIT_EXTENDED || is_a64(cpu_env(cs));
-}
-
 static inline bool is_64bit_semihosting(CPUArchState *env)
 {
     return is_a64(env);
diff --git a/target/riscv/common-semi-target.h b/target/riscv/common-semi-target.h
index 7c8a59e0cc3..63bbcd2d5fa 100644
--- a/target/riscv/common-semi-target.h
+++ b/target/riscv/common-semi-target.h
@@ -25,11 +25,6 @@ static inline void common_semi_set_ret(CPUState *cs, target_ulong ret)
     env->gpr[xA0] = ret;
 }
 
-static inline bool common_semi_sys_exit_extended(CPUState *cs, int nr)
-{
-    return (nr == TARGET_SYS_EXIT_EXTENDED || sizeof(target_ulong) == 8);
-}
-
 static inline bool is_64bit_semihosting(CPUArchState *env)
 {
     return riscv_cpu_mxl(env) != MXL_RV32;
diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index 3f653c6e7a9..ef57d7205c8 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -755,7 +755,9 @@ void do_common_semihosting(CPUState *cs)
     {
         uint32_t ret;
 
-        if (common_semi_sys_exit_extended(cs, nr)) {
+        bool extended = (nr == TARGET_SYS_EXIT_EXTENDED ||
+                         is_64bit_semihosting(cpu_env(cs)));
+        if (extended) {
             /*
              * The A64 version of SYS_EXIT takes a parameter block,
              * so the application-exit type can return a subcode which
-- 
2.47.2
Re: [PATCH v2 04/11] semihosting/arm-compat-semi: remove common_semi_sys_exit_extended
Posted by Daniel Henrique Barboza 2 months, 2 weeks ago 

On 8/1/25 8:26 PM, Pierrick Bouvier wrote:
> This allows to get rid of sizeof(target_ulong) for riscv, without
> changing the semantic.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   target/arm/common-semi-target.h   | 5 -----
>   target/riscv/common-semi-target.h | 5 -----
>   semihosting/arm-compat-semi.c     | 4 +++-
>   3 files changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/target/arm/common-semi-target.h b/target/arm/common-semi-target.h
> index da51f2d7f54..7ebd2136d93 100644
> --- a/target/arm/common-semi-target.h
> +++ b/target/arm/common-semi-target.h
> @@ -34,11 +34,6 @@ static inline void common_semi_set_ret(CPUState *cs, target_ulong ret)
>       }
>   }
>   
> -static inline bool common_semi_sys_exit_extended(CPUState *cs, int nr)
> -{
> -    return nr == TARGET_SYS_EXIT_EXTENDED || is_a64(cpu_env(cs));
> -}
> -
>   static inline bool is_64bit_semihosting(CPUArchState *env)
>   {
>       return is_a64(env);
> diff --git a/target/riscv/common-semi-target.h b/target/riscv/common-semi-target.h
> index 7c8a59e0cc3..63bbcd2d5fa 100644
> --- a/target/riscv/common-semi-target.h
> +++ b/target/riscv/common-semi-target.h
> @@ -25,11 +25,6 @@ static inline void common_semi_set_ret(CPUState *cs, target_ulong ret)
>       env->gpr[xA0] = ret;
>   }
>   
> -static inline bool common_semi_sys_exit_extended(CPUState *cs, int nr)
> -{
> -    return (nr == TARGET_SYS_EXIT_EXTENDED || sizeof(target_ulong) == 8);
> -}
> -
>   static inline bool is_64bit_semihosting(CPUArchState *env)
>   {
>       return riscv_cpu_mxl(env) != MXL_RV32;
> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> index 3f653c6e7a9..ef57d7205c8 100644
> --- a/semihosting/arm-compat-semi.c
> +++ b/semihosting/arm-compat-semi.c
> @@ -755,7 +755,9 @@ void do_common_semihosting(CPUState *cs)
>       {
>           uint32_t ret;
>   
> -        if (common_semi_sys_exit_extended(cs, nr)) {
> +        bool extended = (nr == TARGET_SYS_EXIT_EXTENDED ||
> +                         is_64bit_semihosting(cpu_env(cs)));
> +        if (extended) {
>               /*
>                * The A64 version of SYS_EXIT takes a parameter block,
>                * so the application-exit type can return a subcode which
Re: [PATCH v2 04/11] semihosting/arm-compat-semi: remove common_semi_sys_exit_extended
Posted by Peter Maydell 2 months, 2 weeks ago 
On Sat, 2 Aug 2025 at 00:26, Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> This allows to get rid of sizeof(target_ulong) for riscv, without
> changing the semantic.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>  target/arm/common-semi-target.h   | 5 -----
>  target/riscv/common-semi-target.h | 5 -----
>  semihosting/arm-compat-semi.c     | 4 +++-
>  3 files changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/target/arm/common-semi-target.h b/target/arm/common-semi-target.h
> index da51f2d7f54..7ebd2136d93 100644
> --- a/target/arm/common-semi-target.h
> +++ b/target/arm/common-semi-target.h
> @@ -34,11 +34,6 @@ static inline void common_semi_set_ret(CPUState *cs, target_ulong ret)
>      }
>  }
>
> -static inline bool common_semi_sys_exit_extended(CPUState *cs, int nr)
> -{
> -    return nr == TARGET_SYS_EXIT_EXTENDED || is_a64(cpu_env(cs));
> -}
> -
>  static inline bool is_64bit_semihosting(CPUArchState *env)
>  {
>      return is_a64(env);
> diff --git a/target/riscv/common-semi-target.h b/target/riscv/common-semi-target.h
> index 7c8a59e0cc3..63bbcd2d5fa 100644
> --- a/target/riscv/common-semi-target.h
> +++ b/target/riscv/common-semi-target.h
> @@ -25,11 +25,6 @@ static inline void common_semi_set_ret(CPUState *cs, target_ulong ret)
>      env->gpr[xA0] = ret;
>  }
>
> -static inline bool common_semi_sys_exit_extended(CPUState *cs, int nr)
> -{
> -    return (nr == TARGET_SYS_EXIT_EXTENDED || sizeof(target_ulong) == 8);
> -}
> -
>  static inline bool is_64bit_semihosting(CPUArchState *env)
>  {
>      return riscv_cpu_mxl(env) != MXL_RV32;
> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> index 3f653c6e7a9..ef57d7205c8 100644
> --- a/semihosting/arm-compat-semi.c
> +++ b/semihosting/arm-compat-semi.c
> @@ -755,7 +755,9 @@ void do_common_semihosting(CPUState *cs)
>      {
>          uint32_t ret;
>
> -        if (common_semi_sys_exit_extended(cs, nr)) {
> +        bool extended = (nr == TARGET_SYS_EXIT_EXTENDED ||
> +                         is_64bit_semihosting(cpu_env(cs)));

This doesn't look right. Whether a target supports the sensible
(extended) semantics for SYS_EXIT is a different question from
whether it's 32 bit or not. For Arm, it happens that the point
where we defined the newer semantics coincided with the A64
architecture. For riscv I don't know -- if they made the 32-bit
riscv not have SYS_EXIT_EXTENDED that was an unfortunate choice
but they're probably stuck with it now. For any future architecture
that decides to adopt Arm-compatible semihosting the right
choice will be to provide the extended semantics regardless of
bit width.

This is why there's a common_semi_sys_exit_extended() function
that each target architecture needs to implement.

thanks
-- PMM
Re: [PATCH v2 04/11] semihosting/arm-compat-semi: remove common_semi_sys_exit_extended
Posted by Pierrick Bouvier 2 months, 1 week ago 
On 8/2/25 12:58 PM, Peter Maydell wrote:
> On Sat, 2 Aug 2025 at 00:26, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>>
>> This allows to get rid of sizeof(target_ulong) for riscv, without
>> changing the semantic.
>>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>   target/arm/common-semi-target.h   | 5 -----
>>   target/riscv/common-semi-target.h | 5 -----
>>   semihosting/arm-compat-semi.c     | 4 +++-
>>   3 files changed, 3 insertions(+), 11 deletions(-)
>>
>> diff --git a/target/arm/common-semi-target.h b/target/arm/common-semi-target.h
>> index da51f2d7f54..7ebd2136d93 100644
>> --- a/target/arm/common-semi-target.h
>> +++ b/target/arm/common-semi-target.h
>> @@ -34,11 +34,6 @@ static inline void common_semi_set_ret(CPUState *cs, target_ulong ret)
>>       }
>>   }
>>
>> -static inline bool common_semi_sys_exit_extended(CPUState *cs, int nr)
>> -{
>> -    return nr == TARGET_SYS_EXIT_EXTENDED || is_a64(cpu_env(cs));
>> -}
>> -
>>   static inline bool is_64bit_semihosting(CPUArchState *env)
>>   {
>>       return is_a64(env);
>> diff --git a/target/riscv/common-semi-target.h b/target/riscv/common-semi-target.h
>> index 7c8a59e0cc3..63bbcd2d5fa 100644
>> --- a/target/riscv/common-semi-target.h
>> +++ b/target/riscv/common-semi-target.h
>> @@ -25,11 +25,6 @@ static inline void common_semi_set_ret(CPUState *cs, target_ulong ret)
>>       env->gpr[xA0] = ret;
>>   }
>>
>> -static inline bool common_semi_sys_exit_extended(CPUState *cs, int nr)
>> -{
>> -    return (nr == TARGET_SYS_EXIT_EXTENDED || sizeof(target_ulong) == 8);
>> -}
>> -
>>   static inline bool is_64bit_semihosting(CPUArchState *env)
>>   {
>>       return riscv_cpu_mxl(env) != MXL_RV32;
>> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
>> index 3f653c6e7a9..ef57d7205c8 100644
>> --- a/semihosting/arm-compat-semi.c
>> +++ b/semihosting/arm-compat-semi.c
>> @@ -755,7 +755,9 @@ void do_common_semihosting(CPUState *cs)
>>       {
>>           uint32_t ret;
>>
>> -        if (common_semi_sys_exit_extended(cs, nr)) {
>> +        bool extended = (nr == TARGET_SYS_EXIT_EXTENDED ||
>> +                         is_64bit_semihosting(cpu_env(cs)));
> 
> This doesn't look right. Whether a target supports the sensible
> (extended) semantics for SYS_EXIT is a different question from
> whether it's 32 bit or not. For Arm, it happens that the point
> where we defined the newer semantics coincided with the A64
> architecture. For riscv I don't know -- if they made the 32-bit
> riscv not have SYS_EXIT_EXTENDED that was an unfortunate choice
> but they're probably stuck with it now. For any future architecture
> that decides to adopt Arm-compatible semihosting the right
> choice will be to provide the extended semantics regardless of
> bit width.
> 
> This is why there's a common_semi_sys_exit_extended() function
> that each target architecture needs to implement.
>

It happens to coincide for now, so I felt it was useless to leave this 
function given both architecture have the exact same implementation:
nr == TARGET_SYS_EXIT_EXTENDED ||
is_64bit_semihosting(cpu_env(cs))

As you prefer we keep it for a potential new architecture, I will remove 
this patch and keep it.

> thanks
> -- PMM

Regards,
Pierrick

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.