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 <david.eng...@sysgo.com>
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
