Re: [PULL 24/30] semihosting: Change internal common-semi interfaces to use CPUState *

[Peter Maydell]

On Fri, 15 Jan 2021 at 13:33, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> From: Keith Packard <keithp@keithp.com>
>
> This makes all of the internal interfaces architecture-independent and
> renames the internal functions to use the 'common_semi' prefix instead
> of 'arm' or 'arm_semi'.

Hi; it looks like this commit broke the implementation of
the SYS_HEAPINFO call (see https://bugs.launchpad.net/qemu/+bug/1915925).

Specifically:

> @@ -1064,23 +1089,19 @@ target_ulong do_common_semihosting(CPUState *cs)
>              for (i = 0; i < ARRAY_SIZE(retvals); i++) {
>                  bool fail;
>
> -                if (is_a64(env)) {
> -                    fail = put_user_u64(retvals[i], arg0 + i * 8);
> -                } else {
> -                    fail = put_user_u32(retvals[i], arg0 + i * 4);
> -                }
> +                fail = SET_ARG(i, retvals[i]);

this code which is writing the heap info into guest memory
should not be using SET_ARG(). The SYS_HEAPINFO API says:
# On entry, the PARAMETER REGISTER contains the address of a pointer
# to a four-field data block.

which is to say that the parameter register points to a 1-word
argument block, whose single constituent is a value giving the
address of the (heapbase, heaplimit, stackbase, stacklimit)
structure we need to fill in.

The change here to use SET_ARG() is making us write the
retvals to the argument block, when we should be writing them
to the memory pointed to by arg0. (This is why the original
code wasn't using SET_ARG...)

Could you look at a fix for this, please?

I think you probably need to abstract out "figure out the
size of a 'field' for this target", which for Arm is "is_a64() ? 8 : 4",
and for riscv seems to be "sizeof(target_ulong)" [*]. That
then as a followup would let you have a common code implementation
of GET_ARG() and SET_ARG().

[*] side note -- shouldn't this be checking riscv_cpu_is_32bit() now ?

thanks
-- PMM