This patchset attempts to fix https://bugs.launchpad.net/qemu/+bug/1823998
which reports that we don't handle kernels larger than 128MB
correctly, because we allow the initrd to be placed over the
tail end of the kernel. AArch64 kernel Image files (since v3.17)
report the total size they require (including any BSS area that
isn't in the Image itself), so we can use that to be sure we
place the initrd sufficiently far into the RAM.

Patches 1 and 2 are new since v1; patches 3 and 4 are the old
patches 1 and 2 (and are basically unchanged since v1).

Patches 1 and 2 in this series are new. Patch 1 fixes bugs
in the existing code where we were assuming that we could
treat info->ram_size as the address of the end of RAM, which
isn't true if the RAM doesn't start at address 0. (This
generally went unnoticed thanks to the magic of unsigned integer
underflow turning end-start calculations into very large max_size
values for load_ramdisk_as() and friends.)
Patch 2 adds some explicit checks that we don't try to put things
entirely off the end of RAM (which avoids those accidental
underflows).
Patch 3 in this series adjusts our "where do we put the initrd"
heuristic so that it always places it at least after whatever
our best guess at the kernel size is. (This might still not
be right for images like self-decompressing 32-bit kernels, where
there's no way to know how big the kernel will be after
decompression.)
Patch 4 makes load_aarch64_image() return the
kernel size as indicated in the Image file header, so that for
the specific case of AArch64 Image files we will definitely not
put the initrd on top of them.

thanks
-- PMM

Peter Maydell (4):
  hw/arm/boot: Don't assume RAM starts at address zero
  hw/arm/boot: Diagnose layouts that put initrd or DTB off the end of
    RAM
  hw/arm/boot: Avoid placing the initrd on top of the kernel
  hw/arm/boot: Honour image size field in AArch64 Image format kernels

 hw/arm/boot.c | 83 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 62 insertions(+), 21 deletions(-)

-- 
2.20.1

Hi Peter,

Sorry for the delay in replying to this...

On Thu, May 16, 2019 at 03:47:29PM +0100, Peter Maydell wrote:
> 
> This patchset attempts to fix https://bugs.launchpad.net/qemu/+bug/1823998
> which reports that we don't handle kernels larger than 128MB
> correctly, because we allow the initrd to be placed over the
> tail end of the kernel. AArch64 kernel Image files (since v3.17)
> report the total size they require (including any BSS area that
> isn't in the Image itself), so we can use that to be sure we
> place the initrd sufficiently far into the RAM.
> 
> Patches 1 and 2 are new since v1; patches 3 and 4 are the old
> patches 1 and 2 (and are basically unchanged since v1).
> 
> Patches 1 and 2 in this series are new. Patch 1 fixes bugs
> in the existing code where we were assuming that we could
> treat info->ram_size as the address of the end of RAM, which
> isn't true if the RAM doesn't start at address 0. (This
> generally went unnoticed thanks to the magic of unsigned integer
> underflow turning end-start calculations into very large max_size
> values for load_ramdisk_as() and friends.)
> Patch 2 adds some explicit checks that we don't try to put things
> entirely off the end of RAM (which avoids those accidental
> underflows).
> Patch 3 in this series adjusts our "where do we put the initrd"
> heuristic so that it always places it at least after whatever
> our best guess at the kernel size is. (This might still not
> be right for images like self-decompressing 32-bit kernels, where
> there's no way to know how big the kernel will be after
> decompression.)
> Patch 4 makes load_aarch64_image() return the
> kernel size as indicated in the Image file header, so that for
> the specific case of AArch64 Image files we will definitely not
> put the initrd on top of them.

With all 4 patches applied, I'm able to boot kernels with large BSS
segments (~128M, ~512M, and ~1G), and I get sensible warnings when they
are impossible to boot, e.g.

# 124M of RAM
[mark@gravadlaks:~/repro]% ./vmboot.sh ~/Image.test-128M
qemu-system-aarch64: kernel '/home/mark/Image.test-128M' is too large to fit in RAM (kernel size 155500544, RAM size 130023424)

# 150M of RAM
[mark@gravadlaks:~/repro]% ./vmboot.sh ~/Image.test-128M
qemu-system-aarch64: Not enough space for DTB after kernel/initrd

So feel free to add:

Tested-by: Mark Rutland <mark.rutland@arm.com>

Thanks for putting this together; this is _really_ useful for my testing
setup, and the warnings above are likely to save people a lot of
head-scratching in future. It would be great to see this merged. :)

Thanks,
Mark.

Ping for code review, please?

thanks
-- PMM

On Thu, 16 May 2019 at 15:47, Peter Maydell <peter.maydell@linaro.org> wrote:
>
>
> This patchset attempts to fix https://bugs.launchpad.net/qemu/+bug/1823998
> which reports that we don't handle kernels larger than 128MB
> correctly, because we allow the initrd to be placed over the
> tail end of the kernel. AArch64 kernel Image files (since v3.17)
> report the total size they require (including any BSS area that
> isn't in the Image itself), so we can use that to be sure we
> place the initrd sufficiently far into the RAM.
>
> Patches 1 and 2 are new since v1; patches 3 and 4 are the old
> patches 1 and 2 (and are basically unchanged since v1).
>
> Patches 1 and 2 in this series are new. Patch 1 fixes bugs
> in the existing code where we were assuming that we could
> treat info->ram_size as the address of the end of RAM, which
> isn't true if the RAM doesn't start at address 0. (This
> generally went unnoticed thanks to the magic of unsigned integer
> underflow turning end-start calculations into very large max_size
> values for load_ramdisk_as() and friends.)
> Patch 2 adds some explicit checks that we don't try to put things
> entirely off the end of RAM (which avoids those accidental
> underflows).
> Patch 3 in this series adjusts our "where do we put the initrd"
> heuristic so that it always places it at least after whatever
> our best guess at the kernel size is. (This might still not
> be right for images like self-decompressing 32-bit kernels, where
> there's no way to know how big the kernel will be after
> decompression.)
> Patch 4 makes load_aarch64_image() return the
> kernel size as indicated in the Image file header, so that for
> the specific case of AArch64 Image files we will definitely not
> put the initrd on top of them.
>
> thanks
> -- PMM
>
> Peter Maydell (4):
>   hw/arm/boot: Don't assume RAM starts at address zero
>   hw/arm/boot: Diagnose layouts that put initrd or DTB off the end of
>     RAM
>   hw/arm/boot: Avoid placing the initrd on top of the kernel
>   hw/arm/boot: Honour image size field in AArch64 Image format kernels
>
>  hw/arm/boot.c | 83 ++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 62 insertions(+), 21 deletions(-)
>
> --
> 2.20.1
>

Cc'ing Drew and Eric

On 6/7/19 3:07 PM, Peter Maydell wrote:
> Ping for code review, please?
> 
> thanks
> -- PMM
> 
> On Thu, 16 May 2019 at 15:47, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>>
>> This patchset attempts to fix https://bugs.launchpad.net/qemu/+bug/1823998
>> which reports that we don't handle kernels larger than 128MB
>> correctly, because we allow the initrd to be placed over the
>> tail end of the kernel. AArch64 kernel Image files (since v3.17)
>> report the total size they require (including any BSS area that
>> isn't in the Image itself), so we can use that to be sure we
>> place the initrd sufficiently far into the RAM.
>>
>> Patches 1 and 2 are new since v1; patches 3 and 4 are the old
>> patches 1 and 2 (and are basically unchanged since v1).
>>
>> Patches 1 and 2 in this series are new. Patch 1 fixes bugs
>> in the existing code where we were assuming that we could
>> treat info->ram_size as the address of the end of RAM, which
>> isn't true if the RAM doesn't start at address 0. (This
>> generally went unnoticed thanks to the magic of unsigned integer
>> underflow turning end-start calculations into very large max_size
>> values for load_ramdisk_as() and friends.)
>> Patch 2 adds some explicit checks that we don't try to put things
>> entirely off the end of RAM (which avoids those accidental
>> underflows).
>> Patch 3 in this series adjusts our "where do we put the initrd"
>> heuristic so that it always places it at least after whatever
>> our best guess at the kernel size is. (This might still not
>> be right for images like self-decompressing 32-bit kernels, where
>> there's no way to know how big the kernel will be after
>> decompression.)
>> Patch 4 makes load_aarch64_image() return the
>> kernel size as indicated in the Image file header, so that for
>> the specific case of AArch64 Image files we will definitely not
>> put the initrd on top of them.
>>
>> thanks
>> -- PMM
>>
>> Peter Maydell (4):
>>   hw/arm/boot: Don't assume RAM starts at address zero
>>   hw/arm/boot: Diagnose layouts that put initrd or DTB off the end of
>>     RAM
>>   hw/arm/boot: Avoid placing the initrd on top of the kernel
>>   hw/arm/boot: Honour image size field in AArch64 Image format kernels
>>
>>  hw/arm/boot.c | 83 ++++++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 62 insertions(+), 21 deletions(-)
>>
>> --
>> 2.20.1
>>
>