Re: [PATCH] generic-loader: remove the ram_size limit when a loading binary file

[Alistair Francis]

On Fri, Oct 8, 2021 at 8:38 PM Damien Hedde <damien.hedde@greensocs.com> wrote:
>
>
>
> On 10/7/21 09:59, Philippe Mathieu-Daudé wrote:
> > On 10/7/21 08:41, Alistair Francis wrote:
> >> On Wed, Oct 6, 2021 at 10:04 PM Damien Hedde <damien.hedde@greensocs.com> 
> >> wrote:
> >>> On 10/6/21 13:49, Philippe Mathieu-Daudé wrote:
> >>>> On 10/6/21 13:37, Damien Hedde wrote:
> >>>>> Right now, we cannot load some binary file if it is bigger than the
> >>>>> machine's ram size. This limitation only occurs when loading a
> >>>>> binary file: we can load a corresponding elf file without this
> >>>>> limitation.
> >>>>>
> >>>>> This is an issue for machines that have small ram or do not use the
> >>>>> ram_size feature at all.
> >>>>>
> >>>>> Also get rid of "hw/boards.h" include, since we needed it only
> >>>>> to access `current_machine`.
> >>>>>
> >>>>> Fixes: e481a1f63c9 ("generic-loader: Add a generic loader")
> >>>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> >>>>> ---
> >>>>>
> >>>>> Hi Alistair,
> >>>>>
> >>>>> I found this while experimenting with a ram_size=0 machine.
> >>>
> >>>
> >>>
> >>>>
> >>>> Where are you loading your file?
> >>>>
> >>>
> >>> In a rom.
> >>>
> >>> The loader does not check at all that we are loading to the machine's
> >>> ram. It just check the size for the raw binary file format.
> >>
> >> Hmmm... This is probably correct, in that a user might want to load a
> >> binary into ROM and doesn't want to be blocked by the ram size.
> >>
> >> In general though a user would expect an error if they are loading a
> >> file into RAM that is larger then the RAM. So I'm not fully convinced
> >> we want this change.
> >
> > I agree with Damien using current_machine->ram_size is not ideal,
> > for example some machines have the RAM split and mapped at different
> > regions, so even if ram_size is enough for the image to load,
> > a region might not be big enough and I'd expect load_image_targphys_as()
> > to fail.
> >
> > Maybe we can call memory_region_find(s->addr) then on match:
> >
> >   if (memory_region_is_ram*(match)) {
> >     size = memory_region_size(match);
> >   } else {
> >     size = -1;
> >   }
> >
>
> So I worked a bit on this.
>
> We could call memory_region_find(get_system_memory(), addr, ...) like
> Philippe proposed.
> and check that the memory is big enough and has the proper "type" (ram,
> rom, ...)
>
> Note that we will check only the current state of the address space. So
> it means the region must already exists (sounds reasonable to me).
>
> If this sounds like a good direction to you, I'll propose an updated
> version of the patch.

This sounds like the right direction to me. It also seems like a
significant improvement over what we do now.

Alistair

>
> regarding relying on load_image_targphys to check this. I don't know...
> all these functions (load_targphys_...() and rom_add_...()) just put
> rom(s) in a list. The list is checked afterward against overlap and
> loaded at reset.
> It is possible but it changes the behavior of all rom_add_...() functions.
>
> We could also check, during the reset, that the rom loading works. But
> maybe some part of qemu expects that we silently skip some missing bits.
> Maybe a log/warning there ?
>
> Thanks,
> Damien