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

[David Gibson]

On Thu, Feb 15, 2018 at 10:36:00AM +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.
> 
> When here do not use bios_name/size for the kernel and use a more generic
> name called payload_name/size.
> 
> New in v3: dtb must be stored between kernel and initrd because Linux can
>            handle the dtb only within the first 64MB. Add a comment to
>            clarify the behavior.
> 
> Signed-off-by: David Engraf <address@hidden>

Sorry I've taken so long to reply to this.  It looks fine to me,
however, other changes mean it longer quite applies to the
ppc-for-2.12 tree.  Can you fix that up and repost please.

Reviewed-by: David Gibson <address@hidden>

> ---
>  hw/ppc/e500.c | 116 
> +++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 70 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index c4fe06ea2a..414c4beaab 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -784,8 +784,10 @@ void ppce500_init(MachineState *machine, PPCE500Params 
> *params)
>      int initrd_size = 0;
>      hwaddr cur_base = 0;
>      char *filename;
> +    const char *payload_name;
> +    bool kernel_as_payload;
>      hwaddr bios_entry = 0;
> -    target_long bios_size;
> +    target_long payload_size;
>      struct boot_info *boot_info;
>      int dt_size;
>      int i;
> @@ -913,11 +915,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,8 +949,61 @@ void ppce500_init(MachineState *machine, PPCE500Params 
> *params)
>                                      sysbus_mmio_get_region(s, 0));
>      }
>  
> -    /* Load kernel. */
> -    if (machine->kernel_filename) {
> +    /*
> +     * Smart firmware defaults ahead!
> +     *
> +     * We follow the following table to select which payload we execute.
> +     *
> +     *  -kernel | -bios | payload
> +     * ---------+-------+---------
> +     *     N    |   Y   | u-boot
> +     *     N    |   N   | u-boot
> +     *     Y    |   Y   | u-boot
> +     *     Y    |   N   | kernel
> +     *
> +     * This ensures backwards compatibility with how we used to expose
> +     * -kernel to users but allows them to run through u-boot as well.
> +     */
> +    kernel_as_payload = false;
> +    if (bios_name == NULL) {
> +        if (machine->kernel_filename) {
> +            payload_name = machine->kernel_filename;
> +            kernel_as_payload = true;
> +        } else {
> +            payload_name = "u-boot.e500";
> +        }
> +    } else {
> +        payload_name = bios_name;
> +    }
> +
> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, payload_name);
> +
> +    payload_size = load_elf(filename, NULL, NULL, &bios_entry, &loadaddr, 
> NULL,
> +                            1, PPC_ELF_MACHINE, 0, 0);
> +    if (payload_size < 0) {
> +        /*
> +         * Hrm. No ELF image? Try a uImage, maybe someone is giving us an
> +         * ePAPR compliant kernel
> +         */
> +        payload_size = load_uimage(filename, &bios_entry, &loadaddr, NULL,
> +                                   NULL, NULL);
> +        if (payload_size < 0) {
> +            fprintf(stderr, "qemu: could not load firmware '%s'\n", 
> filename);
> +            exit(1);
> +        }
> +    }
> +
> +    g_free(filename);
> +
> +    if (kernel_as_payload) {
> +        kernel_base = loadaddr;
> +        kernel_size = payload_size;
> +    }
> +
> +    cur_base = loadaddr + payload_size;
> +
> +    /* Load bare kernel only if no bios/u-boot has been provided */
> +    if (machine->kernel_filename && !kernel_as_payload) {
>          kernel_base = cur_base;
>          kernel_size = load_image_targphys(machine->kernel_filename,
>                                            cur_base,
> @@ -967,6 +1017,11 @@ void ppce500_init(MachineState *machine, PPCE500Params 
> *params)
>          cur_base += kernel_size;
>      }
>  
> +    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;
> @@ -983,47 +1038,16 @@ void ppce500_init(MachineState *machine, PPCE500Params 
> *params)
>      }
>  
>      /*
> -     * Smart firmware defaults ahead!
> -     *
> -     * We follow the following table to select which payload we execute.
> -     *
> -     *  -kernel | -bios | payload
> -     * ---------+-------+---------
> -     *     N    |   Y   | u-boot
> -     *     N    |   N   | u-boot
> -     *     Y    |   Y   | u-boot
> -     *     Y    |   N   | kernel
> -     *
> -     * This ensures backwards compatibility with how we used to expose
> -     * -kernel to users but allows them to run through u-boot as well.
> +     * Reserve space for dtb behind the kernel image because Linux has a bug
> +     * where it can only handle the dtb if it's within the first 64MB of 
> where
> +     * <kernel> starts. dtb cannot not reach initrd_base because 
> INITRD_LOAD_PAD
> +     * ensures enough space between kernel and initrd.
>       */
> -    if (bios_name == NULL) {
> -        if (machine->kernel_filename) {
> -            bios_name = machine->kernel_filename;
> -        } else {
> -            bios_name = "u-boot.e500";
> -        }
> -    }
> -    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> -
> -    bios_size = load_elf(filename, NULL, NULL, &bios_entry, &loadaddr, 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) {
> -            fprintf(stderr, "qemu: could not load firmware '%s'\n", 
> filename);
> +    dt_base = (loadaddr + payload_size + 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);
> -        }
>      }
> -    g_free(filename);
> -
> -    /* Reserve space for dtb */
> -    dt_base = (loadaddr + bios_size + DTC_LOAD_PAD) & ~DTC_PAD_MASK;
>  
>      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