qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Damien Hedde
Subject: Re: [PATCH] generic-loader: remove the ram_size limit when a loading binary file
Date: Fri, 8 Oct 2021 12:38:41 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.2



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.

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]