| Message ID | 20211006113707.96907-1-damien.hedde@greensocs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | generic-loader: remove the ram_size limit when a loading binary file | expand |
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? > > I checked the original discussion: it seems to me there was no > intention to specifically limit to the ram size but we had to > put some limit. > > See this: > https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04668.html > https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04681.html > > Thanks for your feedback, > Damien > --- > hw/core/generic-loader.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c > index d14f932eea..102605c07b 100644 > --- a/hw/core/generic-loader.c > +++ b/hw/core/generic-loader.c > @@ -34,7 +34,6 @@ > #include "hw/core/cpu.h" > #include "sysemu/dma.h" > #include "sysemu/reset.h" > -#include "hw/boards.h" > #include "hw/loader.h" > #include "hw/qdev-properties.h" > #include "qapi/error.h" > @@ -153,8 +152,8 @@ static void generic_loader_realize(DeviceState *dev, Error **errp) > } > > if (size < 0 || s->force_raw) { > - /* Default to the maximum size being the machine's ram size */ > - size = load_image_targphys_as(s->file, s->addr, current_machine->ram_size, as); > + /* Do not limit the file size */ > + size = load_image_targphys_as(s->file, s->addr, -1, as); > } else { > s->addr = entry; > } >
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. -- Damien
Damien Hedde <damien.hedde@greensocs.com> writes: > 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. It does beg the question of why you don't just construct your ROM file with the image in place there? Is this just a development convenience?
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. What error do you get if you try to load a binary that is too large with this patch applied? Alistair > > -- > Damien > > >
On 10/6/21 17:40, Alex Bennée wrote: > > Damien Hedde <damien.hedde@greensocs.com> writes: > >> 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. > > It does beg the question of why you don't just construct your ROM file > with the image in place there? Is this just a development convenience? generic-loader is designed from a CPU perspective, it uses the CPU AS to load the image. If your image is in ROM, I'm not sure this is the correct API. I'd try to do this without considering any CPU in the picture. The rom_add_*() API might be more appropriate. My 2 cents anyway...
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; } > > What error do you get if you try to load a binary that is too large > with this patch applied? > > Alistair > >> >> -- >> Damien >> >> >> >
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 do agree for the error, but right now if you give a wrong base address, it does not fail (see below). I can give a look to see if such check would be possible. > > What error do you get if you try to load a binary that is too large > with this patch applied? Nothing if you pass the ram_size check. You can even try to load something to an unmapped area, it doesn't report an error. But it does not do anything visible to the guest I suppose (in the monitor, x/'xp' commands still tell you it cannot access the memory if you tried to load it somewhere that does not exist). If you try to load something bigger than the target region, the region seems to be initialized. Thanks, Damien > > Alistair > >> >> -- >> Damien >> >> >>
On 10/7/21 09:54, Philippe Mathieu-Daudé wrote: > On 10/6/21 17:40, Alex Bennée wrote: >> >> Damien Hedde <damien.hedde@greensocs.com> writes: >> >>> 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. >> >> It does beg the question of why you don't just construct your ROM file >> with the image in place there? Is this just a development convenience? > > generic-loader is designed from a CPU perspective, it uses the CPU AS > to load the image. > > If your image is in ROM, I'm not sure this is the correct API. I'd try > to do this without considering any CPU in the picture. The rom_add_*() > API might be more appropriate. > > My 2 cents anyway... > I was looking for a user way of loading data in a memory-mapped area so I cannot use rom_add_*(). I though the loader goal was to load something to any memory. But maybe I am mistaken. Damien
On 10/7/21 12:12, Damien Hedde wrote: > > > On 10/7/21 09:54, Philippe Mathieu-Daudé wrote: >> On 10/6/21 17:40, Alex Bennée wrote: >>> >>> Damien Hedde <damien.hedde@greensocs.com> writes: >>> >>>> 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. >>> >>> It does beg the question of why you don't just construct your ROM file >>> with the image in place there? Is this just a development convenience? >> >> generic-loader is designed from a CPU perspective, it uses the CPU AS >> to load the image. >> >> If your image is in ROM, I'm not sure this is the correct API. I'd try >> to do this without considering any CPU in the picture. The rom_add_*() >> API might be more appropriate. >> >> My 2 cents anyway... >> > > I was looking for a user way of loading data in a memory-mapped area so > I cannot use rom_add_*(). > I though the loader goal was to load something to any memory. But maybe > I am mistaken. I don't think you are mistaken, you likely found a design limitation with this device, which isn't as generic as it aims to be.
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
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
diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c index d14f932eea..102605c07b 100644 --- a/hw/core/generic-loader.c +++ b/hw/core/generic-loader.c @@ -34,7 +34,6 @@ #include "hw/core/cpu.h" #include "sysemu/dma.h" #include "sysemu/reset.h" -#include "hw/boards.h" #include "hw/loader.h" #include "hw/qdev-properties.h" #include "qapi/error.h" @@ -153,8 +152,8 @@ static void generic_loader_realize(DeviceState *dev, Error **errp) } if (size < 0 || s->force_raw) { - /* Default to the maximum size being the machine's ram size */ - size = load_image_targphys_as(s->file, s->addr, current_machine->ram_size, as); + /* Do not limit the file size */ + size = load_image_targphys_as(s->file, s->addr, -1, as); } else { s->addr = entry; }
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. I checked the original discussion: it seems to me there was no intention to specifically limit to the ram size but we had to put some limit. See this: https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04668.html https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04681.html Thanks for your feedback, Damien --- hw/core/generic-loader.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)