| Message ID | 20190516144733.32399-4-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/boot: handle large Images more gracefully | expand |
Peter Maydell <peter.maydell@linaro.org> writes: > We currently put the initrd at the smaller of: > * 128MB into RAM > * halfway into the RAM > (with the dtb following it). > > However for large kernels this might mean that the kernel > overlaps the initrd. For some kinds of kernel (self-decompressing > 32-bit kernels, and ELF images with a BSS section at the end) > we don't know the exact size, but even there we have a > minimum size. Put the initrd at least further into RAM than > that. For image formats that can give us an exact kernel size, this > will mean that we definitely avoid overlaying kernel and initrd. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > --- > hw/arm/boot.c | 34 ++++++++++++++++++++-------------- > 1 file changed, 20 insertions(+), 14 deletions(-) > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c > index 935be3b92a5..e441393fdf5 100644 > --- a/hw/arm/boot.c > +++ b/hw/arm/boot.c > @@ -999,20 +999,6 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu, > if (info->nb_cpus == 0) > info->nb_cpus = 1; > > - /* > - * We want to put the initrd far enough into RAM that when the > - * kernel is uncompressed it will not clobber the initrd. However > - * on boards without much RAM we must ensure that we still leave > - * enough room for a decent sized initrd, and on boards with large > - * amounts of RAM we must avoid the initrd being so far up in RAM > - * that it is outside lowmem and inaccessible to the kernel. > - * So for boards with less than 256MB of RAM we put the initrd > - * halfway into RAM, and for boards with 256MB of RAM or more we put > - * the initrd at 128MB. > - */ > - info->initrd_start = info->loader_start + > - MIN(info->ram_size / 2, 128 * 1024 * 1024); > - > /* Assume that raw images are linux kernels, and ELF images are not. */ > kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr, > &elf_high_addr, elf_machine, as); > @@ -1064,6 +1050,26 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu, > } > > info->entry = entry; > + > + /* > + * We want to put the initrd far enough into RAM that when the > + * kernel is uncompressed it will not clobber the initrd. However > + * on boards without much RAM we must ensure that we still leave > + * enough room for a decent sized initrd, and on boards with large > + * amounts of RAM we must avoid the initrd being so far up in RAM > + * that it is outside lowmem and inaccessible to the kernel. > + * So for boards with less than 256MB of RAM we put the initrd > + * halfway into RAM, and for boards with 256MB of RAM or more we put > + * the initrd at 128MB. > + * We also refuse to put the initrd somewhere that will definitely > + * overlay the kernel we just loaded, though for kernel formats which > + * don't tell us their exact size (eg self-decompressing 32-bit kernels) > + * we might still make a bad choice here. > + */ > + info->initrd_start = info->loader_start + > + MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size); > + info->initrd_start = TARGET_PAGE_ALIGN(info->initrd_start); > + > if (is_linux) { > uint32_t fixupcontext[FIXUP_MAX]; -- Alex Bennée
Hi Peter, I've just been testing on QEMU v4.1.0-rc1, and found a case where the DTB overlapped the end of the kernel, and I think there's a bug in this patch -- explanation below. On Thu, May 16, 2019 at 03:47:32PM +0100, Peter Maydell wrote: > We currently put the initrd at the smaller of: > * 128MB into RAM > * halfway into the RAM > (with the dtb following it). > > However for large kernels this might mean that the kernel > overlaps the initrd. For some kinds of kernel (self-decompressing > 32-bit kernels, and ELF images with a BSS section at the end) > we don't know the exact size, but even there we have a > minimum size. Put the initrd at least further into RAM than > that. For image formats that can give us an exact kernel size, this > will mean that we definitely avoid overlaying kernel and initrd. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/arm/boot.c | 34 ++++++++++++++++++++-------------- > 1 file changed, 20 insertions(+), 14 deletions(-) > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c > index 935be3b92a5..e441393fdf5 100644 > --- a/hw/arm/boot.c > +++ b/hw/arm/boot.c > @@ -999,20 +999,6 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu, > if (info->nb_cpus == 0) > info->nb_cpus = 1; > > - /* > - * We want to put the initrd far enough into RAM that when the > - * kernel is uncompressed it will not clobber the initrd. However > - * on boards without much RAM we must ensure that we still leave > - * enough room for a decent sized initrd, and on boards with large > - * amounts of RAM we must avoid the initrd being so far up in RAM > - * that it is outside lowmem and inaccessible to the kernel. > - * So for boards with less than 256MB of RAM we put the initrd > - * halfway into RAM, and for boards with 256MB of RAM or more we put > - * the initrd at 128MB. > - */ > - info->initrd_start = info->loader_start + > - MIN(info->ram_size / 2, 128 * 1024 * 1024); > - > /* Assume that raw images are linux kernels, and ELF images are not. */ > kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr, > &elf_high_addr, elf_machine, as); > @@ -1064,6 +1050,26 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu, > } > > info->entry = entry; Note: this is the start of the kernel image... > + > + /* > + * We want to put the initrd far enough into RAM that when the > + * kernel is uncompressed it will not clobber the initrd. However > + * on boards without much RAM we must ensure that we still leave > + * enough room for a decent sized initrd, and on boards with large > + * amounts of RAM we must avoid the initrd being so far up in RAM > + * that it is outside lowmem and inaccessible to the kernel. > + * So for boards with less than 256MB of RAM we put the initrd > + * halfway into RAM, and for boards with 256MB of RAM or more we put > + * the initrd at 128MB. > + * We also refuse to put the initrd somewhere that will definitely > + * overlay the kernel we just loaded, though for kernel formats which > + * don't tell us their exact size (eg self-decompressing 32-bit kernels) > + * we might still make a bad choice here. > + */ > + info->initrd_start = info->loader_start + > + MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size); ... but here we add kernel_size to the start of the loader, which is below the kernel. Should that be info->entry? I've seen this trigger a case where: * The kernel's image_size is 0x0a7a8000 * The kernel was loaded at 0x40080000 * The end of the kernel is 0x4A828000 * The DTB was loaded at 0x4a800000 ... and the kernel is unable to find a usable DTB. Thanks, Mark. > + info->initrd_start = TARGET_PAGE_ALIGN(info->initrd_start); > + > if (is_linux) { > uint32_t fixupcontext[FIXUP_MAX]; > > -- > 2.20.1 >
On Fri, 19 Jul 2019 at 17:47, Mark Rutland <mark.rutland@arm.com> wrote: > > Hi Peter, > > I've just been testing on QEMU v4.1.0-rc1, and found a case where the > DTB overlapped the end of the kernel, and I think there's a bug in this > patch -- explanation below. > > On Thu, May 16, 2019 at 03:47:32PM +0100, Peter Maydell wrote: > > We currently put the initrd at the smaller of: > > * 128MB into RAM > > * halfway into the RAM > > (with the dtb following it). > > > > However for large kernels this might mean that the kernel > > overlaps the initrd. For some kinds of kernel (self-decompressing > > 32-bit kernels, and ELF images with a BSS section at the end) > > we don't know the exact size, but even there we have a > > minimum size. Put the initrd at least further into RAM than > > that. For image formats that can give us an exact kernel size, this > > will mean that we definitely avoid overlaying kernel and initrd. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > hw/arm/boot.c | 34 ++++++++++++++++++++-------------- > > 1 file changed, 20 insertions(+), 14 deletions(-) > > > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c > > index 935be3b92a5..e441393fdf5 100644 > > --- a/hw/arm/boot.c > > +++ b/hw/arm/boot.c > > @@ -999,20 +999,6 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu, > > if (info->nb_cpus == 0) > > info->nb_cpus = 1; > > > > - /* > > - * We want to put the initrd far enough into RAM that when the > > - * kernel is uncompressed it will not clobber the initrd. However > > - * on boards without much RAM we must ensure that we still leave > > - * enough room for a decent sized initrd, and on boards with large > > - * amounts of RAM we must avoid the initrd being so far up in RAM > > - * that it is outside lowmem and inaccessible to the kernel. > > - * So for boards with less than 256MB of RAM we put the initrd > > - * halfway into RAM, and for boards with 256MB of RAM or more we put > > - * the initrd at 128MB. > > - */ > > - info->initrd_start = info->loader_start + > > - MIN(info->ram_size / 2, 128 * 1024 * 1024); > > - > > /* Assume that raw images are linux kernels, and ELF images are not. */ > > kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr, > > &elf_high_addr, elf_machine, as); > > @@ -1064,6 +1050,26 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu, > > } > > > > info->entry = entry; > > Note: this is the start of the kernel image... It's the entry point, which isn't quite the same thing as the start of the image (if we just loaded an ELF file then 'entry' is whatever the ELF file said the entrypoint is, which could be a long way into the image). > > + > > + /* > > + * We want to put the initrd far enough into RAM that when the > > + * kernel is uncompressed it will not clobber the initrd. However > > + * on boards without much RAM we must ensure that we still leave > > + * enough room for a decent sized initrd, and on boards with large > > + * amounts of RAM we must avoid the initrd being so far up in RAM > > + * that it is outside lowmem and inaccessible to the kernel. > > + * So for boards with less than 256MB of RAM we put the initrd > > + * halfway into RAM, and for boards with 256MB of RAM or more we put > > + * the initrd at 128MB. > > + * We also refuse to put the initrd somewhere that will definitely > > + * overlay the kernel we just loaded, though for kernel formats which > > + * don't tell us their exact size (eg self-decompressing 32-bit kernels) > > + * we might still make a bad choice here. > > + */ > > + info->initrd_start = info->loader_start + > > + MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size); > > ... but here we add kernel_size to the start of the loader, which is > below the kernel. Should that be info->entry? loader_start here really means "base of RAM". I think what we want here is something like info->initrd_start = info->loader_start + MIN(info->ram_size / 2, 128 * 1024 * 1024); info->initrd_start = MAX(info->initrd_start, kernel_end); where kernel_end is just past whatever the highest addr of the kernel is, which is not something that's totally trivial to calculate with the variables we have to hand at this point. thanks -- PMM
On Mon, Jul 22, 2019 at 12:59:01PM +0100, Peter Maydell wrote: > On Fri, 19 Jul 2019 at 17:47, Mark Rutland <mark.rutland@arm.com> wrote: > > > > Hi Peter, > > > > I've just been testing on QEMU v4.1.0-rc1, and found a case where the > > DTB overlapped the end of the kernel, and I think there's a bug in this > > patch -- explanation below. > > > > On Thu, May 16, 2019 at 03:47:32PM +0100, Peter Maydell wrote: > > > We currently put the initrd at the smaller of: > > > * 128MB into RAM > > > * halfway into the RAM > > > (with the dtb following it). > > > > > > However for large kernels this might mean that the kernel > > > overlaps the initrd. For some kinds of kernel (self-decompressing > > > 32-bit kernels, and ELF images with a BSS section at the end) > > > we don't know the exact size, but even there we have a > > > minimum size. Put the initrd at least further into RAM than > > > that. For image formats that can give us an exact kernel size, this > > > will mean that we definitely avoid overlaying kernel and initrd. > > > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > > --- > > > hw/arm/boot.c | 34 ++++++++++++++++++++-------------- > > > 1 file changed, 20 insertions(+), 14 deletions(-) > > > > > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c > > > index 935be3b92a5..e441393fdf5 100644 > > > --- a/hw/arm/boot.c > > > +++ b/hw/arm/boot.c > > > @@ -999,20 +999,6 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu, > > > if (info->nb_cpus == 0) > > > info->nb_cpus = 1; > > > > > > - /* > > > - * We want to put the initrd far enough into RAM that when the > > > - * kernel is uncompressed it will not clobber the initrd. However > > > - * on boards without much RAM we must ensure that we still leave > > > - * enough room for a decent sized initrd, and on boards with large > > > - * amounts of RAM we must avoid the initrd being so far up in RAM > > > - * that it is outside lowmem and inaccessible to the kernel. > > > - * So for boards with less than 256MB of RAM we put the initrd > > > - * halfway into RAM, and for boards with 256MB of RAM or more we put > > > - * the initrd at 128MB. > > > - */ > > > - info->initrd_start = info->loader_start + > > > - MIN(info->ram_size / 2, 128 * 1024 * 1024); > > > - > > > /* Assume that raw images are linux kernels, and ELF images are not. */ > > > kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr, > > > &elf_high_addr, elf_machine, as); > > > @@ -1064,6 +1050,26 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu, > > > } > > > > > > info->entry = entry; > > > > Note: this is the start of the kernel image... > > It's the entry point, which isn't quite the same thing as > the start of the image (if we just loaded an ELF file then > 'entry' is whatever the ELF file said the entrypoint is, which > could be a long way into the image). Ah, I see; thanks for correcting me! > > > + /* > > > + * We want to put the initrd far enough into RAM that when the > > > + * kernel is uncompressed it will not clobber the initrd. However > > > + * on boards without much RAM we must ensure that we still leave > > > + * enough room for a decent sized initrd, and on boards with large > > > + * amounts of RAM we must avoid the initrd being so far up in RAM > > > + * that it is outside lowmem and inaccessible to the kernel. > > > + * So for boards with less than 256MB of RAM we put the initrd > > > + * halfway into RAM, and for boards with 256MB of RAM or more we put > > > + * the initrd at 128MB. > > > + * We also refuse to put the initrd somewhere that will definitely > > > + * overlay the kernel we just loaded, though for kernel formats which > > > + * don't tell us their exact size (eg self-decompressing 32-bit kernels) > > > + * we might still make a bad choice here. > > > + */ > > > + info->initrd_start = info->loader_start + > > > + MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size); > > > > ... but here we add kernel_size to the start of the loader, which is > > below the kernel. Should that be info->entry? > > loader_start here really means "base of RAM". I think what > we want here is something like > > info->initrd_start = info->loader_start + MIN(info->ram_size / 2, > 128 * 1024 * 1024); > info->initrd_start = MAX(info->initrd_start, kernel_end); From what I understand, I can't see a way of breaking that, so it looks good to me. > where kernel_end is just past whatever the highest addr of the kernel > is, which is not something that's totally trivial to calculate > with the variables we have to hand at this point. Sure; I assume you might need to drop that into arm_boot_info. As previously, I'm happy to test patches for this. At the moment I have a local hack relying on info->entry, and I understand this isn't correct. Thanks, Mark.
diff --git a/hw/arm/boot.c b/hw/arm/boot.c index 935be3b92a5..e441393fdf5 100644 --- a/hw/arm/boot.c +++ b/hw/arm/boot.c @@ -999,20 +999,6 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu, if (info->nb_cpus == 0) info->nb_cpus = 1; - /* - * We want to put the initrd far enough into RAM that when the - * kernel is uncompressed it will not clobber the initrd. However - * on boards without much RAM we must ensure that we still leave - * enough room for a decent sized initrd, and on boards with large - * amounts of RAM we must avoid the initrd being so far up in RAM - * that it is outside lowmem and inaccessible to the kernel. - * So for boards with less than 256MB of RAM we put the initrd - * halfway into RAM, and for boards with 256MB of RAM or more we put - * the initrd at 128MB. - */ - info->initrd_start = info->loader_start + - MIN(info->ram_size / 2, 128 * 1024 * 1024); - /* Assume that raw images are linux kernels, and ELF images are not. */ kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr, &elf_high_addr, elf_machine, as); @@ -1064,6 +1050,26 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu, } info->entry = entry; + + /* + * We want to put the initrd far enough into RAM that when the + * kernel is uncompressed it will not clobber the initrd. However + * on boards without much RAM we must ensure that we still leave + * enough room for a decent sized initrd, and on boards with large + * amounts of RAM we must avoid the initrd being so far up in RAM + * that it is outside lowmem and inaccessible to the kernel. + * So for boards with less than 256MB of RAM we put the initrd + * halfway into RAM, and for boards with 256MB of RAM or more we put + * the initrd at 128MB. + * We also refuse to put the initrd somewhere that will definitely + * overlay the kernel we just loaded, though for kernel formats which + * don't tell us their exact size (eg self-decompressing 32-bit kernels) + * we might still make a bad choice here. + */ + info->initrd_start = info->loader_start + + MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size); + info->initrd_start = TARGET_PAGE_ALIGN(info->initrd_start); + if (is_linux) { uint32_t fixupcontext[FIXUP_MAX];
We currently put the initrd at the smaller of: * 128MB into RAM * halfway into the RAM (with the dtb following it). However for large kernels this might mean that the kernel overlaps the initrd. For some kinds of kernel (self-decompressing 32-bit kernels, and ELF images with a BSS section at the end) we don't know the exact size, but even there we have a minimum size. Put the initrd at least further into RAM than that. For image formats that can give us an exact kernel size, this will mean that we definitely avoid overlaying kernel and initrd. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/boot.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)