Re: [Qemu-arm] [PATCH v2 3/4] hw/arm/boot: Avoid placing the initrd on top of the kernel

[Mark Rutland]

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