Re: [Qemu-devel] [RFC] target-arm: fix semihosting ram base issue

[Peter Maydell <peter.maydell@linaro.org>]

On 17 June 2016 at 03:37, Tsung-Han Lin <tsunghan.tw@gmail.com> wrote:
> Hi, I made some changes to TRY TO fix the ARM semihosting issue in
> SYS_HEAPINFO handling.
> This problem has been bothering me for quite a while.
>
> A new global variable 'main_ram_base' is added while a new memory
> API, memory_region_add_subregion_main, is also provided to let
> SoC/board creator to initialize this variable.
> I am not sure if this is a good idea (to add a new API)
> or maybe we just let SoC/board creator to simply
> set 'main_ram_base' in their 'xxx_realize' functions?
>
> As for Cortex-M series, 'main_ram_base' is set during cpu initialization.
> A64 semihosting handling is also added and use zynqmp as an example.
>
> Any comments/reviews are big welcome!
> Thanks in advance!

Hi. First of all, unfortunately we can't accept any
patch from you unless you provide a signed-off-by: line
(which is basically saying you have the legal right to
provide it to us under QEMU's license terms; see
http://wiki.qemu.org/Contribute/SubmitAPatch#Patch_emails_must_include_a_Signed-off-by:_line
for more detail). We can fix up most other stuff, but
this one is a hard requirement.

> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 23c719986715..8124f71992b4 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -206,7 +206,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>          memory_region_init_alias(&s->ddr_ram_high, NULL,
>                                   "ddr-ram-high", s->ddr_ram,
>                                    ddr_low_size, ddr_high_size);
> -        memory_region_add_subregion(get_system_memory(),
> +        memory_region_add_subregion_main(get_system_memory(),
>                                      XLNX_ZYNQMP_HIGH_RAM_START,
>                                      &s->ddr_ram_high);

This isn't necessarily the main RAM for this board --
if you don't pass more than XLNX_ZYNQMP_MAX_LOW_RAM_SIZE
as the RAM size then the only RAM is the low ram at
address 0. In any case, even if you do have enough RAM
to go through this code path, the executable being loaded
might be linked so it goes into the low RAM alias at 0,
in which case using this address as the heap start/limit
would be wrong.

>      } else {

If we can avoid having to change every board to specify this
that would be nice. (Most of them already specify the RAM
base in vbi->bootinfo.loader_start.)

Is your use case passing an ELF file to QEMU to run?
I suspect what we actually need to do for boards like
the Xilinx with more than one RAM area is address the
/* TODO: Make this use the limit of the loaded application.  */
and actually use the values from the loaded executable,
rather than guessing them.

This would also address problems with the Cortex-M cores,
where the application being loaded might be linked to
be in RAM (non-zero start) or to be in flash (zero start).

We should also be able to do a better job of guessing for
simple boards with one RAM area at a non-zero offset,
but if we look at the ELF files we're loading we might
not need to bother...

> diff --git a/target-arm/arm-semi.c b/target-arm/arm-semi.c
> index 8be0645eb08b..d30469688b01 100644
> --- a/target-arm/arm-semi.c
> +++ b/target-arm/arm-semi.c
> @@ -599,17 +599,32 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>              unlock_user(ptr, arg0, 16);
>  #else
>              limit = ram_size;
> -            ptr = lock_user(VERIFY_WRITE, arg0, 16, 0);
> -            if (!ptr) {
> -                /* FIXME - should this error code be -TARGET_EFAULT ? */
> -                return (uint32_t)-1;
> -            }
> -            /* TODO: Make this use the limit of the loaded application.  */
> -            ptr[0] = tswap32(limit / 2);
> -            ptr[1] = tswap32(limit);
> -            ptr[2] = tswap32(limit); /* Stack base */
> -            ptr[3] = tswap32(0); /* Stack limit.  */
> -            unlock_user(ptr, arg0, 16);
> +                       if (is_a64(env)) {
> +                               uint64_t *ptrx;
> +                               ptrx = lock_user(VERIFY_WRITE, arg0, 32, 0);
> +                               if (!ptrx) {
> +                                       /* FIXME - should this error code be -TARGET_EFAULT ? */
> +                                       return (uint32_t)-1;
> +                               }
> +                               /* TODO: Make this use the limit of the loaded application.  */
> +                               ptrx[0] = tswap64(main_ram_base + ram_size / 2); /* Heap base */
> +                               ptrx[1] = tswap64(main_ram_base + ram_size);         /* limit */
> +                               ptrx[2] = tswap64(main_ram_base + ram_size); /* Stack base */
> +                               ptrx[3] = tswap64(main_ram_base + ram_size / 2);  /* limit */
> +                               unlock_user(ptrx, arg0, 32);
> +                       } else {
> +                               ptr = lock_user(VERIFY_WRITE, arg0, 16, 0);
> +                               if (!ptr) {
> +                                       /* FIXME - should this error code be -TARGET_EFAULT ? */
> +                                       return (uint32_t)-1;
> +                               }
> +                               /* TODO: Make this use the limit of the loaded application.  */
> +                               ptr[0] = tswap32(main_ram_base + limit / 2);
> +                               ptr[1] = tswap32(main_ram_base + limit);
> +                               ptr[2] = tswap32(main_ram_base + limit); /* Stack base */
> +                               ptr[3] = tswap32(main_ram_base); /* Stack limit.  */
> +                               unlock_user(ptr, arg0, 16);
> +                       }
>  #endif

This is making two bug fixes at once. The part of
this which is fixing the 64-bit code path to write
64-bit values into the data block is a simple
non-controversial bugfix, and it should be in its
own patch. Making better guesses at limit values
for system emulation is trickier (see remarks above).

You've also got some problems with your code indent,
which should be four-space. scripts/checkpatch.pl can
tell you about some style issues with patches.

I suggest you start by sending a patch which just fixes
the 64-bit case to write 64-bit values, since that's the
easy bit.

thanks
-- PMM