Re: [Qemu-devel] [RESEND PATCH] PPC: e500: Fix duplicate kernel load and device tree overlap

[David Gibson]

On Fri, Feb 09, 2018 at 08:49:45AM +0100, David Engraf wrote:
> Hello David,
> 
> Am 09.02.2018 um 06:33 schrieb David Gibson:
> > On Thu, Feb 08, 2018 at 09:36:21AM +0100, David Engraf wrote:
> > > This patch fixes an incorrect behavior when the -kernel argument has been
> > > specified without -bios. In this case the kernel was loaded twice. At 
> > > address
> > > 32M as a raw image and afterwards by load_elf/load_uimage at the
> > > corresponding load address. In this case the region for the device tree 
> > > and
> > > the raw kernel image may overlap.
> > > 
> > > The patch fixes the behavior by loading the kernel image once with
> > > load_elf/load_uimage and skips loading the raw image. It also ensures that
> > > the device tree is generated behind bios/kernel/initrd.
> > > 
> > > Signed-off-by: David Engraf <address@hidden>
> > 
> > Sorry I've taken so long to respond to this.  I've been busy, then
> > away, then busy, then recovering from surgery, then...
> > 
> > I think this looks good overall, just a couple of details I'd like to
> > check, see below.
> > 
> > > ---
> > >   hw/ppc/e500.c | 89 
> > > ++++++++++++++++++++++++++++++++---------------------------
> > >   1 file changed, 48 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> > > index c4fe06ea2a..0321bd66a8 100644
> > > --- a/hw/ppc/e500.c
> > > +++ b/hw/ppc/e500.c
> > > @@ -776,7 +776,6 @@ void ppce500_init(MachineState *machine, 
> > > PPCE500Params *params)
> > >       MemoryRegion *ram = g_new(MemoryRegion, 1);
> > >       PCIBus *pci_bus;
> > >       CPUPPCState *env = NULL;
> > > -    uint64_t loadaddr;
> > >       hwaddr kernel_base = -1LL;
> > >       int kernel_size = 0;
> > >       hwaddr dt_base = 0;
> > > @@ -913,11 +912,6 @@ void ppce500_init(MachineState *machine, 
> > > PPCE500Params *params)
> > >       /* Register spinning region */
> > >       sysbus_create_simple("e500-spin", params->spin_base, NULL);
> > > -    if (cur_base < (32 * 1024 * 1024)) {
> > > -        /* u-boot occupies memory up to 32MB, so load blobs above */
> > > -        cur_base = (32 * 1024 * 1024);
> > > -    }
> > > -
> > >       if (params->has_mpc8xxx_gpio) {
> > >           qemu_irq poweroff_irq;
> > > @@ -952,36 +946,6 @@ void ppce500_init(MachineState *machine, 
> > > PPCE500Params *params)
> > >                                       sysbus_mmio_get_region(s, 0));
> > >       }
> > > -    /* Load kernel. */
> > > -    if (machine->kernel_filename) {
> > > -        kernel_base = cur_base;
> > > -        kernel_size = load_image_targphys(machine->kernel_filename,
> > > -                                          cur_base,
> > > -                                          ram_size - cur_base);
> > > -        if (kernel_size < 0) {
> > > -            fprintf(stderr, "qemu: could not load kernel '%s'\n",
> > > -                    machine->kernel_filename);
> > > -            exit(1);
> > > -        }
> > > -
> > > -        cur_base += kernel_size;
> > > -    }
> > > -
> > > -    /* Load initrd. */
> > > -    if (machine->initrd_filename) {
> > > -        initrd_base = (cur_base + INITRD_LOAD_PAD) & ~INITRD_PAD_MASK;
> > > -        initrd_size = load_image_targphys(machine->initrd_filename, 
> > > initrd_base,
> > > -                                          ram_size - initrd_base);
> > > -
> > > -        if (initrd_size < 0) {
> > > -            fprintf(stderr, "qemu: could not load initial ram disk 
> > > '%s'\n",
> > > -                    machine->initrd_filename);
> > > -            exit(1);
> > > -        }
> > > -
> > > -        cur_base = initrd_base + initrd_size;
> > > -    }
> > > -
> > >       /*
> > >        * Smart firmware defaults ahead!
> > >        *
> > > @@ -1006,24 +970,67 @@ void ppce500_init(MachineState *machine, 
> > > PPCE500Params *params)
> > >       }
> > >       filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> > > -    bios_size = load_elf(filename, NULL, NULL, &bios_entry, &loadaddr, 
> > > NULL,
> > > +    bios_size = load_elf(filename, NULL, NULL, &bios_entry, &cur_base, 
> > > NULL,
> > >                            1, PPC_ELF_MACHINE, 0, 0);
> > >       if (bios_size < 0) {
> > >           /*
> > >            * Hrm. No ELF image? Try a uImage, maybe someone is giving us 
> > > an
> > >            * ePAPR compliant kernel
> > >            */
> > > -        kernel_size = load_uimage(filename, &bios_entry, &loadaddr, NULL,
> > > -                                  NULL, NULL);
> > > -        if (kernel_size < 0) {
> > > +        bios_size = load_uimage(filename, &bios_entry, &cur_base, NULL,
> > > +                                NULL, NULL);
> > > +        if (bios_size < 0) {
> > >               fprintf(stderr, "qemu: could not load firmware '%s'\n", 
> > > filename);
> > >               exit(1);
> > >           }
> > >       }
> > > +    cur_base += bios_size;
> > >       g_free(filename);
> > > +    /* Load bare kernel only if no bios/u-boot has been provided */
> > > +    if (machine->kernel_filename != bios_name) {
> > 
> > This condition seems weird.  Why would the kernel and bios images be
> > the same.  I guess this because of the logic setting bios_name above,
> > which also seems kind of weird.  Can you clarify what's going on here,
> > changing that bios_name setting logic, if necessary?
> 
> Basically I didn't change the logic to store the kernel name into bios_name
> when the -bios options was not provided. In this case the kernel will be
> used as payload. Indeed the name is a bit weird. What do you think about
> changing the names from bios to payload?

That might be useful.  I'm still a bit confused, though - why does
combining bios and kernel as a single "payload" make sense?  From the
looks of the code it seems that they are separate things and either or
both could be loaded separately.  What am I missing?

> 
> > > +        kernel_base = cur_base;
> > > +        kernel_size = load_image_targphys(machine->kernel_filename,
> > > +                                          cur_base,
> > > +                                          ram_size - cur_base);
> > > +        if (kernel_size < 0) {
> > > +            fprintf(stderr, "qemu: could not load kernel '%s'\n",
> > > +                    machine->kernel_filename);
> > > +            exit(1);
> > > +        }
> > > +
> > > +        cur_base += kernel_size;
> > > +    } else {
> > > +        kernel_base = cur_base;
> > > +        kernel_size = bios_size;
> > 
> > Is this right?  You've already advanced cur_base by bios_size from
> > where you loaded the bios image, so kernel_base here will point
> > *after* the bios, not into it, but kernel_size is set to bios_size.
> > This seems strange.
> 
> Right, kernel_base should point to the start of the kernel.
> 
> Best regards
> - David
> 
> 
> > > +    }
> > > +
> > > +    if (cur_base < (32 * 1024 * 1024)) {
> > > +        /* u-boot occupies memory up to 32MB, so load blobs above */
> > > +        cur_base = (32 * 1024 * 1024);
> > > +    }
> > > +
> > > +    /* Load initrd. */
> > > +    if (machine->initrd_filename) {
> > > +        initrd_base = (cur_base + INITRD_LOAD_PAD) & ~INITRD_PAD_MASK;
> > > +        initrd_size = load_image_targphys(machine->initrd_filename, 
> > > initrd_base,
> > > +                                          ram_size - initrd_base);
> > > +
> > > +        if (initrd_size < 0) {
> > > +            fprintf(stderr, "qemu: could not load initial ram disk 
> > > '%s'\n",
> > > +                    machine->initrd_filename);
> > > +            exit(1);
> > > +        }
> > > +
> > > +        cur_base = initrd_base + initrd_size;
> > > +    }
> > > +
> > >       /* Reserve space for dtb */
> > > -    dt_base = (loadaddr + bios_size + DTC_LOAD_PAD) & ~DTC_PAD_MASK;
> > > +    dt_base = (cur_base + DTC_LOAD_PAD) & ~DTC_PAD_MASK;
> > > +    if (dt_base + DTB_MAX_SIZE > ram_size) {
> > > +            fprintf(stderr, "qemu: not enough memory for device tree\n");
> > > +            exit(1);
> > > +    }
> > >       dt_size = ppce500_prep_device_tree(machine, params, dt_base,
> > >                                          initrd_base, initrd_size,
> > 
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature