PPC: e500: Fix duplicate kernel load and device tree overlap

PATCH Superseded David Engraf 3 participants 14 messages Jan 30, 2018 – Mar 5, 2018

Share & tools

David Engraf

author

2018-01-30 07:59 UTC #0 lore raw

PATCH 1/1 PPC: e500: Fix duplicate kernel load and device tree overlap

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 <[email protected]>

---
 hw/ppc/e500.c | 89 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 48 insertions(+), 41 deletions(-)

hw/ppc/e500.c +48 ~~-41~~

index c4fe06ea2a..0321bd66a8 100644
@@ -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) {
+        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;
+    }
+
+    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,

Signature · 1 line

2.14.1

David Engraf

author

2018-02-08 08:36 UTC #1 lore raw

PATCH RESEND 1/1 PPC: e500: Fix duplicate kernel load and device tree 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 <[email protected]>

---
 hw/ppc/e500.c | 89 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 48 insertions(+), 41 deletions(-)

hw/ppc/e500.c +48 ~~-41~~

index c4fe06ea2a..0321bd66a8 100644
@@ -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) {
+        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;
+    }
+
+    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,

Signature · 1 line

2.14.1

David Gibson

2018-02-09 05:34 UTC #2 lore raw

Quoted 11 lines · David Engraf

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 <[email protected]>

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.

Quoted 82 lines

---
 hw/ppc/e500.c | 89 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 48 insertions(+), 41 deletions(-)

hw/ppc/e500.c +3 ~~-40~~

index c4fe06ea2a..0321bd66a8 100644
@@ -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);

hw/ppc/e500.c void ppce500_init(MachineState *machine, PPCE500Params *params) · line 991

Quoted 10 lines

hw/ppc/e500.c +4 -0

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

Quoted 4 lines

hw/ppc/e500.c +4 -0

+        kernel_base = cur_base;
+        kernel_size = load_image_targphys(machine->kernel_filename,
+                                          cur_base,
+                                          ram_size - cur_base);

hw/ppc/e500.c void ppce500_init(MachineState *machine, PPCE500Params *params) · line 1005

Quoted 10 lines

hw/ppc/e500.c +10 -0

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

Quoted 32 lines

hw/ppc/e500.c +27 -1

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

Signature · 4 lines

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

David Engraf

author

2018-02-09 07:49 UTC #3 lore raw

Hello David,

Am 09.02.2018 um 06:33 schrieb David Gibson:

Quoted 109 lines · David Engraf

Quoted 11 lines

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 <[email protected]>

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.

Quoted 90 lines

---
  hw/ppc/e500.c | 89 ++++++++++++++++++++++++++++++++---------------------------
  1 file changed, 48 insertions(+), 41 deletions(-)

hw/ppc/e500.c +5 ~~-40~~

index c4fe06ea2a..0321bd66a8 100644
@@ -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);

Quoted 7 lines

hw/ppc/e500.c void ppce500_init(MachineState *machine, PPCE500Params *params) · line 991
Quoted 2 lines
hw/ppc/e500.c +2 -0
+    /* 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?

Quoted 15 lines

Quoted 4 lines

hw/ppc/e500.c +4 -0

+        kernel_base = cur_base;
+        kernel_size = load_image_targphys(machine->kernel_filename,
+                                          cur_base,
+                                          ram_size - cur_base);

hw/ppc/e500.c void ppce500_init(MachineState *machine, PPCE500Params *params) · line 1005

Quoted 10 lines

hw/ppc/e500.c +10 -0

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

Quoted 4 lines

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

Quoted 33 lines

Quoted 32 lines

hw/ppc/e500.c +27 -1

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

2018-02-13 03:52 UTC #4 lore raw

Quoted 115 lines · David Engraf

Hello David,

Am 09.02.2018 um 06:33 schrieb David Gibson:

Quoted 103 lines · David Engraf

Quoted 11 lines

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 <[email protected]>

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.

Quoted 84 lines

---
  hw/ppc/e500.c | 89 ++++++++++++++++++++++++++++++++---------------------------
  1 file changed, 48 insertions(+), 41 deletions(-)

hw/ppc/e500.c +5 ~~-40~~

index c4fe06ea2a..0321bd66a8 100644
@@ -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;

Quoted 8 lines

hw/ppc/e500.c void ppce500_init(MachineState *machine, PPCE500Params *params) · line 991
Quoted 3 lines
hw/ppc/e500.c +2 -0
      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?

Quoted 4 lines

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?

Quoted 60 lines

Quoted 16 lines

Quoted 5 lines

hw/ppc/e500.c +4 -0

+        kernel_base = cur_base;
+        kernel_size = load_image_targphys(machine->kernel_filename,
+                                          cur_base,
+                                          ram_size - cur_base);

hw/ppc/e500.c void ppce500_init(MachineState *machine, PPCE500Params *params) · line 1005

Quoted 10 lines

hw/ppc/e500.c +10 -0

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

Quoted 4 lines

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

Quoted 32 lines

Quoted 31 lines

hw/ppc/e500.c +27 -1

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

Signature · 4 lines

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

David Engraf

author

2018-02-13 08:06 UTC #5 lore raw

Am 13.02.2018 um 04:51 schrieb David Gibson:

Quoted 121 lines · David Engraf

Quoted 115 lines

Hello David,

Am 09.02.2018 um 06:33 schrieb David Gibson:

Quoted 103 lines · David Engraf

Quoted 11 lines

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 <[email protected]>

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.

Quoted 84 lines

---
   hw/ppc/e500.c | 89 ++++++++++++++++++++++++++++++++---------------------------
   1 file changed, 48 insertions(+), 41 deletions(-)

hw/ppc/e500.c +5 ~~-40~~

index c4fe06ea2a..0321bd66a8 100644
@@ -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;

Quoted 8 lines

hw/ppc/e500.c void ppce500_init(MachineState *machine, PPCE500Params *params) · line 991
Quoted 3 lines
hw/ppc/e500.c +2 -0
       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?

Quoted 4 lines

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?

Quoted 4 lines

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?

You need to differ between the two load algorithms. The payload is loaded by using load_elf or load_uimage. The latter images are loaded as binary by using load_image_targphys. This is required because QEMU needs to know the entry point of the first image (bios or kernel).

Bet regards
- David

Quoted 60 lines

Quoted 59 lines

Quoted 15 lines

Quoted 4 lines

hw/ppc/e500.c +4 -0

+        kernel_base = cur_base;
+        kernel_size = load_image_targphys(machine->kernel_filename,
+                                          cur_base,
+                                          ram_size - cur_base);

hw/ppc/e500.c void ppce500_init(MachineState *machine, PPCE500Params *params) · line 1005

Quoted 10 lines

hw/ppc/e500.c +10 -0

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

Quoted 4 lines

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

Quoted 32 lines

Quoted 31 lines

hw/ppc/e500.c +27 -1

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

author

2018-02-13 10:23 UTC #6 lore raw

PATCH v2 1/1 PPC: e500: Fix duplicate kernel load and device tree overlap

In reply to David Engraf

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.

When here do not use bios_name/size for the kernel and use a more generic name called payload_name/size.

Signed-off-by David Engraf <[email protected]>

---
 hw/ppc/e500.c | 112 ++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 65 insertions(+), 47 deletions(-)

hw/ppc/e500.c +65 ~~-47~~

index c4fe06ea2a..d12b9d6121 100644
@@ -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;

@@ -784,8 +783,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 +914,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 +948,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!
      *

@@ -997,33 +963,85 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
      * 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) {
-            bios_name = machine->kernel_filename;
+            payload_name = machine->kernel_filename;
+            kernel_as_payload = true;
         } else {
-            bios_name = "u-boot.e500";
+            payload_name = "u-boot.e500";
         }
+    } else {
+        payload_name = bios_name;
     }
-    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) {
+    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, payload_name);
+
+    payload_size = load_elf(filename, NULL, NULL, &bios_entry, &cur_base, 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
          */
-        kernel_size = load_uimage(filename, &bios_entry, &loadaddr, NULL,
-                                  NULL, NULL);
-        if (kernel_size < 0) {
+        payload_size = load_uimage(filename, &bios_entry, &cur_base, 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 = cur_base;
+        kernel_size = payload_size;
+    }
+
+    cur_base += 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,
+                                          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;
+    }
+
+    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,

Signature · 1 line

2.14.1

David Engraf

author

2018-02-15 09:36 UTC #7 lore raw

PATCH v3 1/1 PPC: e500: Fix duplicate kernel load and device tree 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 <[email protected]>

---
 hw/ppc/e500.c | 116 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 70 insertions(+), 46 deletions(-)

hw/ppc/e500.c +70 ~~-46~~

index c4fe06ea2a..414c4beaab 100644
@@ -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,

Signature · 1 line

2.14.1

David Gibson

Reviewed-by on v3 1/1

2018-03-02 01:46 UTC #8 lore raw

Quoted 9 lines · David Engraf

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.

Quoted 8 lines

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 <[email protected]>

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 <[email protected]>

Quoted 163 lines

---
 hw/ppc/e500.c | 116 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 70 insertions(+), 46 deletions(-)

hw/ppc/e500.c +70 ~~-46~~

index c4fe06ea2a..414c4beaab 100644
@@ -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,

Signature · 4 lines

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

David Engraf

author

2018-03-02 08:53 UTC #9 lore raw

Am 02.03.2018 um 02:45 schrieb David Gibson:

Quoted 17 lines · David Engraf

Quoted 16 lines
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.

Quoted 5 lines

Quoted 1 line
Signed-off-by David Engraf <[email protected]>
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

Quoted 167 lines

Reviewed-by David Gibson <[email protected]>

Quoted 163 lines

---
  hw/ppc/e500.c | 116 +++++++++++++++++++++++++++++++++++-----------------------
  1 file changed, 70 insertions(+), 46 deletions(-)

hw/ppc/e500.c +70 ~~-46~~

index c4fe06ea2a..414c4beaab 100644
@@ -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,

Signature · 23 lines

Mit freundlichen Grüßen/Best regards,

David Engraf
Product Engineer

SYSGO AG
Office Mainz
Am Pfaffenstein 14 / D-55270 Klein-Winternheim / Germany

Phone: +49-6136-9948-0 / Fax: +49-6136-9948-10
E-mail: [email protected]
_________________________________________________________________________________

Web: https://www.sysgo.com
Blog: https://www.sysgo.com/blog
Events: https://www.sysgo.com/events
Newsletter: https://www.sysgo.com/newsletter
_________________________________________________________________________________

Handelsregister/Commercial Registry: HRB Mainz 90 HRB 8066
Vorstand/Executive Board: Etienne Butery (CEO), Kai Sablotny (COO)
Aufsichtsratsvorsitzender/Supervisory Board Chairman: Marc Darmon
USt-Id-Nr./VAT-Id-No.: DE 149062328

Mark Cave-Ayland

2018-03-02 09:12 UTC #10 lore raw

Quoted 18 lines · David Engraf

Am 02.03.2018 um 02:45 schrieb David Gibson:
Quoted 17 lines · David Engraf
Quoted 16 lines
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.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Signed-off-by David Engraf <[email protected]>
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.

David Engraf

author

2018-03-02 11:11 UTC #11 lore raw

Am 02.03.2018 um 10:11 schrieb Mark Cave-Ayland:

Quoted 26 lines · David Engraf

Quoted 25 lines
Am 02.03.2018 um 02:45 schrieb David Gibson:
Quoted 22 lines · David Engraf
Quoted 9 lines
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.
Quoted 8 lines
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 <[email protected]>
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.

Quoted 7 lines

Quoted 1 line
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

David Engraf

author

2018-03-02 11:20 UTC #12 lore raw

PATCH v4 1/1 PPC: e500: Fix duplicate kernel load and device tree overlap

In reply to David Gibson

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 <[email protected]>

Reviewed-by David Gibson <[email protected]>

---
 hw/ppc/e500.c | 116 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 70 insertions(+), 46 deletions(-)

hw/ppc/e500.c +70 ~~-46~~

index ef541a00be..43c15d18c4 100644
@@ -792,8 +792,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;

@@ -921,11 +923,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;

@@ -960,8 +957,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) {
+            error_report("qemu: could not load firmware '%s'", 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,

@@ -975,6 +1025,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;

@@ -991,47 +1046,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) {
-            error_report("could not load firmware '%s'", filename);
+    dt_base = (loadaddr + payload_size + DTC_LOAD_PAD) & ~DTC_PAD_MASK;
+    if (dt_base + DTB_MAX_SIZE > ram_size) {
+            error_report("qemu: not enough memory for device tree");
             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,

Signature · 1 line

2.14.1

David Gibson

Applied

2018-03-05 01:38 UTC #13 lore raw

Quoted 12 lines · David Engraf

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.

Quoted 6 lines

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 <[email protected]>
Reviewed-by David Gibson <[email protected]>

Applied, thanks.

Quoted 163 lines

---
 hw/ppc/e500.c | 116 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 70 insertions(+), 46 deletions(-)

hw/ppc/e500.c +70 ~~-46~~

index ef541a00be..43c15d18c4 100644
@@ -792,8 +792,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;

@@ -921,11 +923,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;

@@ -960,8 +957,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) {
+            error_report("qemu: could not load firmware '%s'", 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,

@@ -975,6 +1025,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;

@@ -991,47 +1046,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) {
-            error_report("could not load firmware '%s'", filename);
+    dt_base = (loadaddr + payload_size + DTC_LOAD_PAD) & ~DTC_PAD_MASK;
+    if (dt_base + DTB_MAX_SIZE > ram_size) {
+            error_report("qemu: not enough memory for device tree");
             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,

Signature · 4 lines

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