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

[Tsung-Han Lin <tsunghan.tw@gmail.com>]

2016-06-24 1:26 GMT+08:00 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.
>
> Hi, Peter,

Thanks for the comments.
Well, actually I was just trying to throw out some of my thoughts and get
some feedbacks and comments.
(not intend to get merged :p) since the approach I used involved some API
changes and I am not sure if that's a good idea or not. But I will make
sure next time I send out something that will just as the normal patch sets
should be.
Thank you for all those comments.


> > 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?
>
Yes, I always use an ELF file to do some experiments.

> 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
>

Seems like you already have the solutions! thanks anyway :p
(was just still trying to get more understanding of the code base :p)


-- 
Tsung-Han "*Johnny*" Lin

Page: http://tsunghanlin.github.com/
Email: tsunghan.tw@gmail.com