On Mon, Dec 02, 2013 at 01:51:22PM -0600, Matt Sealey wrote:

Here's where I think this whole thing falls down as being the weirdest
possible implementation of this. It defies logic to put this
information in the device tree /chosen node while also attempting to
boot the kernel using an EFI stub; the stub is going to have this
information because it is going to have the pointer to the system
System Table (since it was called by StartImage()). Why not stash the
System Table pointer somewhere safe in the stub?

We do. In the DT.

The information in the device tree is all accessible from Boot
Services and as long as the System Table isn't being thrown away (my
suggestion would be.. stuff it in r2, and set r1 = "EFI\0" then work
with arch/arm/kernel/head{-common,}.S code to do the right thing)

You left out the bit of redefining the kernel boot protocol to permit
calling it with caches, MMU and interrupts enabled - also known as
before ExitBootServices().

It seems like the advantages of booting from UEFI and having all this
information and API around are being thrown away very early, and
picked up when it's no longer relevant to gain access to the very
minimal runtime services. What's missing is a UUID for a "Device Tree
Blob" in the Configuration Table, so you can very easily go grab that
information from the firmware.

Which is what we are going to implement anyway in order to permit
firmware to supply DT hardware description in the same way as with
ACPI. Yes, we could pass the system table pointer directly - but that
doesn't get us the memory map.

As implemented, these patches employ a very long-winded and complex
method of recovering UEFI after throwing the system table pointer away
early in boot, and then implements an EFI calling convention which
isn't strictly necessary according to the UEFI spec - the question is,
is this a workaround for SetVirtualAddressMap() not actually doing the
right thing on ARM UEFI implementations? If you can't guarantee that
most of the calls from Boot Services or Runtime Services are going to
allow this, then having any UEFI support in the kernel at all seems
rather weird.

No, it is a workaround for it being explicitly against the kernel boot
protocol (not to mention slightly hairy) to enter head.S with MMU and
caches enabled and interrupts firing.

The EFI calling convention (as pointed out in the patch itself) is
there in order to not have to duplicate code already there for x86.

What I'm worried about is that this is basically a hack to try and
shoehorn an existing UEFI implementation to an existing Linux boot
method - and implements it in a way nobody is ever going to be able to
justify improving. Part of the reason the OpenFirmware CIF got thrown
away early in SPARC/PowerPC boot (after "flattening" the device tree
using the CIF calls to parse it out) was because you had to disable
the MMU, caches, interrupts etc. which caused all kinds of slow
firmware code to be all kinds of extra-slow.

I prefer to see it as a way to not reinvent things that do not need
reinventing, while not adding more special-case code to the kernel.

What that meant is nobody bothered to implement working, re-entrant,
re-locatable firmware to a great degree. This ended up being a
self-fulfilling prophecy of "don't trust the bootloader" and "get rid
of it as soon as we can," which essentially meant Linux never took
advantage of the resources available. In OF's case, the CIF sucked by
specification. In UEFI's case here, it's been implemented in Linux in
such a way that guarantees poor-performing firmware code with huge
penalties to call them, which isn't even required by UEFI if the
earlier boot code did the right things in the first place.

I don't follow. In which way does this implementation result in poor
performance or reduced functionality?

We deal with a highly quirky set of requirements for calling
SetVirtualAddressMap() in a clunky way - after which calls into UEFI
are direct and cachable.

/
    Leif