Re: [PATCH v5 1/2] semihosting/arm-compat: replace heuristic for softmmu SYS_HEAPINFO

[Philippe Mathieu-Daudé]

On 11/2/22 14:22, Alex Bennée wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Thu, 10 Feb 2022 at 11:48, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > > Hi Alex,
> > >
> > > On 10/2/22 12:30, Alex Bennée wrote:
> > > > The previous numbers were a guess at best and rather arbitrary without
> > > > taking into account anything that might be loaded. Instead of using
> > > > guesses based on the state of registers implement a new function that:
> > > >
> > > >    a) scans the MemoryRegions for the largest RAM block
> > > >    b) iterates through all "ROM" blobs looking for the biggest gap
> > > >
> > > > The "ROM" blobs include all code loaded via -kernel and the various
> > > > -device loader techniques.
> > > >
> > > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > > > Cc: Andrew Strauss <astrauss11@gmail.com>
> > > > Cc: Keith Packard <keithp@keithp.com>
> > > > Message-Id: <20210601090715.22330-1-alex.bennee@linaro.org>
> > >
> > > > +static LayoutInfo common_semi_find_bases(CPUState *cs)
> > > >    {
> > > > -    MemoryRegion *subregion;
> > > > +    FlatView *fv;
> > > > +    LayoutInfo info = { 0, 0, 0, 0 };
> > > > +
> > > > +    RCU_READ_LOCK_GUARD();
> > > > +
> > > > +    fv = address_space_to_flatview(cs->as);
> > >
> > > Why are we using the CPU view and not address_space_memory?
> >
> > If you have a choice between "use the right view of an
> > address space" and "use the global address_space_memory",
> > it's better to prefer the former, I think. We use the
> > latter in lots of places, but it's not conceptually the
> > right way to think about how the memory system works IMHO.

Yes I agree.

For user-mode, this patch makes sense. For system-mode it is
not obvious to make sense of SYS_HEAPINFO (except focusing in
cores targeting embedded systems eventually).

> In this case the addresses have to be as the CPU sees them because it's
> between the CPU and the semihosting backend to share data.

Beside the sysemu curiosity, the patch logic is fine, so:

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks,

Phil.