| Message ID | 20180215093600.18505-1-david.engraf@sysgo.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] PPC: e500: Fix duplicate kernel load and device tree overlap | expand |
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 <david.engraf@sysgo.com> 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 <david@gibson.dropbear.id.au> > --- > 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,
Am 02.03.2018 um 02:45 schrieb 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 <david.engraf@sysgo.com> > > 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. What other changes do you mean? In v3 I just restored the old behavior by putting the dtb right after the kernel image. This behavior has been changed by mistake in v1/v2. v3 now uses the correct storage location again. Best regards - David > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > >> --- >> 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, >
On 02/03/18 08:53, David Engraf wrote: > Am 02.03.2018 um 02:45 schrieb 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 <david.engraf@sysgo.com> >> >> 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. > > What other changes do you mean? In v3 I just restored the old behavior > by putting the dtb right after the kernel image. This behavior has been > changed by mistake in v1/v2. v3 now uses the correct storage location > again. Hi David, I think what David means is that when he tried to apply your v3 patch to his ppc-for-2.12 tree at https://github.com/dgibson/qemu/commits/ppc-for-2.12 using git-am then it failed - presumably because someone else has made other changes to that code since you submitted your v3 patch. Please can you submit a v4 rebased on top of the current ppc-for-2.12 branch? ATB, Mark.
Am 02.03.2018 um 10:11 schrieb Mark Cave-Ayland: > On 02/03/18 08:53, David Engraf wrote: > >> Am 02.03.2018 um 02:45 schrieb 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 <david.engraf@sysgo.com> >>> >>> 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. >> >> What other changes do you mean? In v3 I just restored the old behavior >> by putting the dtb right after the kernel image. This behavior has >> been changed by mistake in v1/v2. v3 now uses the correct storage >> location again. > > Hi David, > > I think what David means is that when he tried to apply your v3 patch to > his ppc-for-2.12 tree at > https://github.com/dgibson/qemu/commits/ppc-for-2.12 using git-am then > it failed - presumably because someone else has made other changes to > that code since you submitted your v3 patch. > > Please can you submit a v4 rebased on top of the current ppc-for-2.12 > branch? Hi Mark, thanks for the info. I will provide an updated version. Best regards - David
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,
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 <david.engraf@sysgo.com> --- hw/ppc/e500.c | 116 +++++++++++++++++++++++++++++++++++----------------------- 1 file changed, 70 insertions(+), 46 deletions(-)