Message-ID: <CAHCPf3t5dEzVWbYZ6DWcFCENNO2dbrx-YeZvKRk0MkEsJMTE3A@mail.gmail.com>
Date:	Mon, 2 Dec 2013 13:51:22 -0600
From:	Matt Sealey <neko@...uhatsu.net>
To:	Leif Lindholm <leif.lindholm@...aro.org>
Cc:	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>, linux-efi@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Russell King <linux@....linux.org.uk>, matt.fleming@...el.com,
	Grant Likely <grant.likely@...aro.org>, roy.franz@...aro.org,
	msalter@...hat.com, Patch Tracking <patches@...aro.org>,
	linaro-uefi@...aro.org, Mark Rutland <mark.rutland@....com>,
	Rob Landley <rob@...dley.net>, linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation

> +UEFI kernel support on ARM
> +==========================
> +UEFI kernel support on the ARM architectures (arm and arm64) is only available
> +when boot is performed through the stub.
> +
> +The stub populates the FDT /chosen node with (and the kernel scans for) the
> +following parameters:
> +________________________________________________________________________________
> +Name                      | Size   | Description
> +================================================================================
> +linux,uefi-system-table   | 64-bit | Physical address of the UEFI System Table.
> +--------------------------------------------------------------------------------
> +linux,uefi-mmap-start     | 64-bit | Physical address of the UEFI memory map,
> +                          |        | populated by the UEFI GetMemoryMap() call.
> +--------------------------------------------------------------------------------
> +linux,uefi-mmap-size      | 32-bit | Size in bytes of the UEFI memory map
> +                          |        | pointed to in previous entry.
> +--------------------------------------------------------------------------------
> +linux,uefi-mmap-desc-size | 32-bit | Size in bytes of each entry in the UEFI
> +                          |        | memory map.
> +--------------------------------------------------------------------------------
> +linux,uefi-mmap-desc-ver  | 32-bit | Version of the mmap descriptor format.
> +--------------------------------------------------------------------------------
> +linux,uefi-stub-kern-ver  | string | Copy of linux_banner from build.
> +--------------------------------------------------------------------------------

This flies in the face of actually using #address-cells and
#size-cells to define how big addresses and sizes are. You're limited
here by the root node definitions... that's the spec.

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?

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)

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.

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.

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.

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.

Ta,
Matt Sealey <neko@...uhatsu.net>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists