On Mon, Jul 22, 2019 at 12:59:01PM +0100, Peter Maydell wrote: > On Fri, 19 Jul 2019 at 17:47, Mark Rutland <mark.rutl...@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.mayd...@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.
