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

[Philippe Mathieu-Daudé]

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