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

[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?

> +        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.

> +    }
> +
> +    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