[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RESEND PATCH] PPC: e500: Fix duplicate kernel load and
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [RESEND PATCH] PPC: e500: Fix duplicate kernel load and device tree overlap |
Date: |
Tue, 13 Feb 2018 14:51:59 +1100 |
User-agent: |
Mutt/1.9.2 (2017-12-15) |
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 <address@hidden>
> >
> > 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
signature.asc
Description: PGP signature