hw/ppc/e500.c | 89 ++++++++++++++++++++++++++++++++--------------------------- 1 file changed, 48 insertions(+), 41 deletions(-)
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.engraf@sysgo.com>
---
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) {
+ 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,
--
2.14.1
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.engraf@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
Hello David,
Am 09.02.2018 um 06:33 schrieb David Gibson:
> On Thu, Feb 08, 2018 at 09:36:21AM +0100, David Engraf wrote:
>> This patch fixes an incorrect behavior when the -kernel argument has been
>> specified without -bios. In this case the kernel was loaded twice. At address
>> 32M as a raw image and afterwards by load_elf/load_uimage at the
>> corresponding load address. In this case the region for the device tree and
>> the raw kernel image may overlap.
>>
>> The patch fixes the behavior by loading the kernel image once with
>> load_elf/load_uimage and skips loading the raw image. It also ensures that
>> the device tree is generated behind bios/kernel/initrd.
>>
>> Signed-off-by: David Engraf <david.engraf@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?
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?
>> + 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.
Right, kernel_base should point to the start of the kernel.
Best regards
- David
>> + }
>> +
>> + 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,
>
On Fri, Feb 09, 2018 at 08:49:45AM +0100, David Engraf wrote:
> Hello David,
>
> Am 09.02.2018 um 06:33 schrieb David Gibson:
> > On Thu, Feb 08, 2018 at 09:36:21AM +0100, David Engraf wrote:
> > > This patch fixes an incorrect behavior when the -kernel argument has been
> > > specified without -bios. In this case the kernel was loaded twice. At address
> > > 32M as a raw image and afterwards by load_elf/load_uimage at the
> > > corresponding load address. In this case the region for the device tree and
> > > the raw kernel image may overlap.
> > >
> > > The patch fixes the behavior by loading the kernel image once with
> > > load_elf/load_uimage and skips loading the raw image. It also ensures that
> > > the device tree is generated behind bios/kernel/initrd.
> > >
> > > Signed-off-by: David Engraf <david.engraf@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?
>
> 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?
>
> > > + 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.
>
> Right, kernel_base should point to the start of the kernel.
>
> Best regards
> - David
>
>
> > > + }
> > > +
> > > + 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
Am 13.02.2018 um 04:51 schrieb David Gibson:
> On Fri, Feb 09, 2018 at 08:49:45AM +0100, David Engraf wrote:
>> Hello David,
>>
>> Am 09.02.2018 um 06:33 schrieb David Gibson:
>>> On Thu, Feb 08, 2018 at 09:36:21AM +0100, David Engraf wrote:
>>>> This patch fixes an incorrect behavior when the -kernel argument has been
>>>> specified without -bios. In this case the kernel was loaded twice. At address
>>>> 32M as a raw image and afterwards by load_elf/load_uimage at the
>>>> corresponding load address. In this case the region for the device tree and
>>>> the raw kernel image may overlap.
>>>>
>>>> The patch fixes the behavior by loading the kernel image once with
>>>> load_elf/load_uimage and skips loading the raw image. It also ensures that
>>>> the device tree is generated behind bios/kernel/initrd.
>>>>
>>>> Signed-off-by: David Engraf <david.engraf@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?
>>
>> 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?
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
>>>> + 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.
>>
>> Right, kernel_base should point to the start of the kernel.
>>
>> Best regards
>> - David
>>
>>
>>>> + }
>>>> +
>>>> + 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,
>>>
>>
>
© 2016 - 2024 Red Hat, Inc.