From the semihosting point of view what we want to know is the current
mode of the processor. Unify this into a single helper and allow us to
use the same GET/SET_ARG helpers for the rest of the code.

Note: we aren't currently testing riscv32 due to missing toolchain for
check-tcg tests.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 semihosting/arm-compat-semi.c | 54 ++++++++++++-----------------------
 1 file changed, 18 insertions(+), 36 deletions(-)

diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index 94950b6c56..733eea1e2d 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -767,15 +767,28 @@ static const GuestFDFunctions guestfd_fns[] = {
     },
 };
 
-/* Read the input value from the argument block; fail the semihosting
- * call if the memory read fails.
+/*
+ * Read the input value from the argument block; fail the semihosting
+ * call if the memory read fails. Eventually we could use a generic
+ * CPUState helper function here.
  */
+static inline bool is_64bit_semihosting(CPUArchState *env)
+{
 #ifdef TARGET_ARM
+    return is_a64(env);
+#elif defined(TARGET_RISCV)
+    return !riscv_cpu_is_32bit(env);
+#else
+#error un-handled architecture
+#endif
+}
+
+
 #define GET_ARG(n) do {                                 \
-    if (is_a64(env)) {                                  \
+    if (is_64bit_semihosting(env)) {                    \
         if (get_user_u64(arg ## n, args + (n) * 8)) {   \
             errno = EFAULT;                             \
-            return set_swi_errno(cs, -1);              \
+            return set_swi_errno(cs, -1);               \
         }                                               \
     } else {                                            \
         if (get_user_u32(arg ## n, args + (n) * 4)) {   \
@@ -786,41 +799,10 @@ static const GuestFDFunctions guestfd_fns[] = {
 } while (0)
 
 #define SET_ARG(n, val)                                 \
-    (is_a64(env) ?                                      \
+    (is_64bit_semihosting(env) ?                        \
      put_user_u64(val, args + (n) * 8) :                \
      put_user_u32(val, args + (n) * 4))
-#endif
 
-#ifdef TARGET_RISCV
-
-/*
- * get_user_ual is defined as get_user_u32 in softmmu-semi.h,
- * we need a macro that fetches a target_ulong
- */
-#define get_user_utl(arg, p)                    \
-    ((sizeof(target_ulong) == 8) ?              \
-     get_user_u64(arg, p) :                     \
-     get_user_u32(arg, p))
-
-/*
- * put_user_ual is defined as put_user_u32 in softmmu-semi.h,
- * we need a macro that stores a target_ulong
- */
-#define put_user_utl(arg, p)                    \
-    ((sizeof(target_ulong) == 8) ?              \
-     put_user_u64(arg, p) :                     \
-     put_user_u32(arg, p))
-
-#define GET_ARG(n) do {                                                 \
-        if (get_user_utl(arg ## n, args + (n) * sizeof(target_ulong))) { \
-            errno = EFAULT;                                             \
-            return set_swi_errno(cs, -1);                              \
-        }                                                               \
-    } while (0)
-
-#define SET_ARG(n, val)                                 \
-    put_user_utl(val, args + (n) * sizeof(target_ulong))
-#endif
 
 /*
  * Do a semihosting call.
-- 
2.20.1

On Tue, 9 Mar 2021 at 14:24, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> From the semihosting point of view what we want to know is the current
> mode of the processor. Unify this into a single helper and allow us to
> use the same GET/SET_ARG helpers for the rest of the code.
>
> Note: we aren't currently testing riscv32 due to missing toolchain for
> check-tcg tests.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  semihosting/arm-compat-semi.c | 54 ++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 36 deletions(-)
>
> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> index 94950b6c56..733eea1e2d 100644
> --- a/semihosting/arm-compat-semi.c
> +++ b/semihosting/arm-compat-semi.c
> @@ -767,15 +767,28 @@ static const GuestFDFunctions guestfd_fns[] = {
>      },
>  };
>
> -/* Read the input value from the argument block; fail the semihosting
> - * call if the memory read fails.
> +/*
> + * Read the input value from the argument block; fail the semihosting
> + * call if the memory read fails. Eventually we could use a generic
> + * CPUState helper function here.
>   */
> +static inline bool is_64bit_semihosting(CPUArchState *env)
> +{
>  #ifdef TARGET_ARM
> +    return is_a64(env);
> +#elif defined(TARGET_RISCV)

can we be consistent about whether we use defined() or not, please ?

> +    return !riscv_cpu_is_32bit(env);
> +#else
> +#error un-handled architecture
> +#endif

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

thanks
-- PMM

Alex Bennée <alex.bennee@linaro.org> writes:

> Note: we aren't currently testing riscv32 due to missing toolchain for
> check-tcg tests.

That's surprising -- the usual risc-v toolchain supports both 64- and
32- bit targets.

Othewise, this patch is

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
-keith

Keith Packard <keithp@keithp.com> writes:

> Alex Bennée <alex.bennee@linaro.org> writes:
>
>> Note: we aren't currently testing riscv32 due to missing toolchain for
>> check-tcg tests.
>
> That's surprising -- the usual risc-v toolchain supports both 64- and
> 32- bit targets.

If you can give me the runes for it I can tweak check-tcg to generate
the 32 bit binaries.

>
> Othewise, this patch is
>
> Reviewed-by: Keith Packard <keithp@keithp.com>


-- 
Alex Bennée