Re: [Qemu-devel] [PATCH 10/10] PPC: e500: Move to u-boot as firmware

[Scott Wood]

On Thu, 2014-01-23 at 11:40 +0100, Alexander Graf wrote:
> On 23.01.2014, at 01:23, Scott Wood <address@hidden> wrote:
> 
> > On Mon, 2014-01-20 at 00:44 +0100, Alexander Graf wrote:
> >> Almost all platforms QEMU emulates have some sort of firmware they can load
> >> to expose a guest environment that closely resembles the way it would look
> >> like on real hardware.
> >> 
> >> This patch introduces such a firmware on our e500 platforms. U-boot is the
> >> default firmware for most of these systems and as such our preferred 
> >> choice.
> >> 
> >> For now, it is able to expose the same functionality to the user as the 
> >> direct
> >> -kernel option was, just that it prints some nice messages beforehand.
> >> 
> >> However, if you abort the boot (press any key) or boot without -kernel you
> >> are actually able to modify the boot environment, execute a networked boot
> >> through the e1000 emulation and execute u-boot payloads.
> > 
> > Have you removed support for loading the kernel directly?  I thought you
> > said you were only going to load U-Boot in the absence of -kernel.
> > Given the absence of runtime services, and the presence of a
> > standardized OS entry mechanism (ePAPR), inserting firmware into the
> > process seems like a niche use case rather than something that should be
> > the default, much less mandatory.
> 
> It's definitely not mandatory, but it makes things consistent. I'm not
> sure it's the better approach, but I figured we should definitely be
> able to go through the full u-boot cycle before we enter the guest
> kernel.

Able to, sure, but it should be optional.  As you noted on our call,
supporting U-Boot requires KVM modifications, so requiring U-Boot would
break old host kernels.  Plus, we'd lose ePAPR-ish ELF support, add at
least one second to boot time, and add complexity to the system (another
thing to possibly go wrong).

> >> +    /* Load u-boot (ELF) */
> >> +    if (bios_name == NULL) {
> >> +        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, ELF_MACHINE, 0);
> >> +    if (bios_size < 0) {
> >> +        /*
> >> +         * Hrm. No ELF image? Try a uImage, maybe someone is giving us an
> >> +         * ePAPR compliant kernel
> >> +         */
> > 
> > ePAPR specifies ELF, not uImage (though QEMU does currently support
> > ePAPR-ish loading of uImages, as U-Boot does) -- and you still seem to
> > be trying to load firmware here, not a kernel.
> 
> Hrm. How do we load an ELF image when we have non-0 mapped memory? In
> fact, I'm wondering the same on uImages now that I think of it :).

Oh right, for mainline the answer is "we don't" -- that's currently only
in the SDK (and there, only for uImage).

But if you mean conceptually how would you do it, either you'd have the
image contain the proper load address for where memory is, or consider
it an offset into the beginning of RAM, or override it somehow.

> So what this code does is it treats the "-bios" parameter the same as
> we used to treat -kernel, just in reverse probe order. So we check if
> -bios is an ELF image, if so, load it. If not, we load it as uImage and
> run it.

OK, but the comment should be fixed.  Either the ELF or the uImage could
be an ePAPR-ish OS, and to be properly ePAPR compliant it should be ELF.

> That means if you do -bios uImage you get a direct kernel boot, the
> same way we used to with -kernel before this patch set.

Oh, I thought what you said on the call was that firmware would be used
when either -kernel is absent or -bios is specified.

The main issue with "-bios uImage", besides semantic awkwardness, is
that existing QEMU invocations will break with existing kernels when
used with newer QEMU versions.

-Scott