Re: [PATCH v3 29/29] acpi: Add an acpi command

[Leif Lindholm]

On Mon, Mar 30, 2020 at 17:13:05 -0600, Simon Glass wrote:
> It is useful to dump ACPI tables in U-Boot to see what has been generated.
> Add a command to handle this.
> 
> To allow the command to find the tables, add a position into the global
> data.
> 
> Support subcommands to list and dump the tables.
> 
> Signed-off-by: Simon Glass 
> Reviewed-by: Wolfgang Wallner 
> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  arch/sandbox/include/asm/global_data.h |   1 +
>  arch/x86/include/asm/global_data.h |   1 +
>  cmd/Kconfig|  14 ++
>  cmd/Makefile   |   1 +
>  cmd/acpi.c | 179 +
>  lib/acpi/acpi_table.c  |   1 +
>  test/dm/acpi.c |  73 ++
>  7 files changed, 270 insertions(+)
>  create mode 100644 cmd/acpi.c
> 
> diff --git a/arch/sandbox/include/asm/global_data.h 
> b/arch/sandbox/include/asm/global_data.h
> index f4ce72d5660..f95ddb058a2 100644
> --- a/arch/sandbox/include/asm/global_data.h
> +++ b/arch/sandbox/include/asm/global_data.h
> @@ -13,6 +13,7 @@
>  struct arch_global_data {
>   uint8_t *ram_buf;   /* emulated RAM buffer */
>   void*text_base; /* pointer to base of text region */
> + ulong acpi_start;   /* Start address of ACPI tables */
>  };
>  
>  #include 
> diff --git a/arch/x86/include/asm/global_data.h 
> b/arch/x86/include/asm/global_data.h
> index f4c1839104e..4aee2f3e8c4 100644
> --- a/arch/x86/include/asm/global_data.h
> +++ b/arch/x86/include/asm/global_data.h
> @@ -123,6 +123,7 @@ struct arch_global_data {
>  #ifdef CONFIG_FSP_VERSION2
>   struct fsp_header *fsp_s_hdr;   /* Pointer to FSP-S header */
>  #endif
> + ulong acpi_start;   /* Start address of ACPI tables */
>  };
>  
>  #endif
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 6403bc45a5e..2d3bfe0ab91 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -190,6 +190,20 @@ comment "Commands"
>  
>  menu "Info commands"
>  
> +config CMD_ACPI
> + bool "acpi"
> + default y if ACPIGEN
> + help
> +   List and dump ACPI tables. ACPI (Advanced Configuration and Power
> +   Interface) is used mostly on x86 for providing information to the
> +   Operating System about devices in the system. The tables are set up
> +   by the firmware, typically U-Boot but possibly an earlier firmware
> +   module, if U-Boot is chain-loaded from something else. ACPI tables
> +   can also include code, to perform hardware-specific tasks required
> +   by the Operating Systems. This allows some amount of separation
> +   between the firmware and OS, and is particularly useful when you
> +   want to make hardware changes without the OS needing to be adjusted.
> +
>  config CMD_BDI
>   bool "bdinfo"
>   default y
> diff --git a/cmd/Makefile b/cmd/Makefile
> index f1dd513a4b4..15a9693ed0e 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -11,6 +11,7 @@ obj-y += help.o
>  obj-y += version.o
>  
>  # command
> +obj-$(CONFIG_CMD_ACPI) += acpi.o
>  obj-$(CONFIG_CMD_AES) += aes.o
>  obj-$(CONFIG_CMD_AB_SELECT) += ab_select.o
>  obj-$(CONFIG_CMD_ADC) += adc.o
> diff --git a/cmd/acpi.c b/cmd/acpi.c
> new file mode 100644
> index 000..8a320435736
> --- /dev/null
> +++ b/cmd/acpi.c
> @@ -0,0 +1,179 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2019 Google LLC
> + * Written by Simon Glass 
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static void dump_hdr(struct acpi_table_header *hdr)
> +{
> + bool has_hdr = memcmp(hdr->signature, "FACS", ACPI_NAME_LEN);
> +
> + printf("%.*s %08lx %06x", ACPI_NAME_LEN, hdr->signature,
> +(ulong)map_to_sysmem(hdr), hdr->length);
> + if (has_hdr) {
> + printf(" (v%02d %.6s %.8s %u %.4s %d)\n", hdr->revision,
> +hdr->oem_id, hdr->oem_table_id, hdr->oem_revision,
> +hdr->aslc_id, hdr->aslc_revision);
> + } else {
> + printf("\n");
> + }
> +}
> +
> +/**
> + * find_table() - Look up an ACPI table
> + *
> + * @sig: Signature of table (4 characters, upper case)
> + * @return pointer to table header, or NULL if not found
> + */
> +struct acpi_table_header *find_table(const char *sig)
> +{
> + struct acpi_rsdp *rsdp;
> + struct acpi_rsdt *rsdt;
> + int len, i, count;
> +
> + rsdp = map_sysmem(gd->arch.acpi_start, 0);
> + if (!rsdp)
> + return NULL;
> + rsdt = map_sysmem(rsdp->rsdt_address, 0);
> + len = rsdt->header.length - sizeof(rsdt->header);
> + count = len / sizeof(u32);
> + for (i = 0; i < count; i++) {
> + struct acpi_table_header *hdr;
> +
> + hdr = map_sysmem(rsdt->entry[i], 0);
> + if (!memcmp(hdr->signature, sig, ACPI_NAME_LEN))
> + 

Re: [PATCH 084/108] x86: acpi: Support generation of the DBG2 table

[Leif Lindholm]

Hi Simon,

A portion of this set is very much not x86-specific. (more below)

On Sun, Jan 26, 2020 at 22:06:31 -0700, Simon Glass wrote:
> Add an implementation of the DBG2 (Debug Port Table 2) ACPI table.
> Adjust one of the header includes to be in the correct order, before
> adding more.
> 
> Signed-off-by: Simon Glass 
> ---
> 
>  arch/x86/lib/acpi_table.c | 112 +-
>  include/acpi_table.h  |  31 +++
>  2 files changed, 142 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> index a810b12cfe..f12344de50 100644
> --- a/arch/x86/lib/acpi_table.c
> +++ b/arch/x86/lib/acpi_table.c
> @@ -8,6 +8,8 @@
>  
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -15,7 +17,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -52,6 +53,50 @@ int acpi_write_hpet(struct acpi_ctx *ctx, const struct 
> udevice *dev)
>   return 0;
>  }
>  
> +int acpi_write_dbg2_pci_uart(struct acpi_ctx *ctx, struct udevice *dev,
> +  uint access_size)
> +{

So, this one could possibly be argued either way (it is certainly not
fully generic).

> + struct acpi_dbg2_header *dbg2 = ctx->current;
> + char path[ACPI_PATH_MAX];
> + struct acpi_gen_regaddr address;
> + phys_addr_t addr;
> + int ret;
> +
> + if (!dev) {
> + log_err("Device not found\n");
> + return -ENODEV;
> + }
> + if (!device_active(dev)) {
> + log_info("Device not enabled\n");
> + return -EACCES;
> + }
> + /*
> +  * PCI devices don't remember their resource allocation information in
> +  * U-Boot at present. We assume that MMIO is used for the UART and that
> +  * the address space is 32 bytes: ns16550 uses 8 registers of up to
> +  * 32-bits each. This is only for debugging so it is not a big deal.
> +  */
> + addr = dm_pci_read_bar32(dev, 0);
> + printf("UART addr %lx\n", (ulong)addr);
> +
> + memset(, '\0', sizeof(address));
> + address.space_id = ACPI_ADDRESS_SPACE_MEMORY;
> + address.addrl = (uint32_t)addr;
> + address.addrh = (uint32_t)((addr >> 32) & 0x);
> + address.access_size = access_size;
> +
> + ret = acpi_device_path(dev, path, sizeof(path));
> + if (ret)
> + return log_msg_ret("path", ret);
> + acpi_create_dbg2(dbg2, ACPI_DBG2_SERIAL_PORT,
> +  ACPI_DBG2_16550_COMPATIBLE, , 0x1000, path);
> +
> + acpi_inc_align(ctx, dbg2->header.length);
> + acpi_add_table(ctx, dbg2);
> +
> + return 0;
> +}
> +
>  static void acpi_create_facs(struct acpi_facs *facs)
>  {
>   memset((void *)facs, 0, sizeof(struct acpi_facs));
> @@ -439,6 +484,71 @@ int acpi_create_hpet(struct acpi_hpet *hpet)
>   return 0;
>  }
>  
> +void acpi_create_dbg2(struct acpi_dbg2_header *dbg2,
> +   int port_type, int port_subtype,
> +   struct acpi_gen_regaddr *address, u32 address_size,
> +   const char *device_path)

But this one would be usable by any of the architectures supported by
ACPI. Could the acpi_create_* functions be moved somewhere
non-arch-specific, like a new lib/acpi?

/
Leif

> +{
> + uintptr_t current;
> + struct acpi_dbg2_device *device;
> + u32 *dbg2_addr_size;
> + struct acpi_table_header *header;
> + size_t path_len;
> + const char *path;
> + char *namespace;
> +
> + /* Fill out header fields. */
> + current = (uintptr_t)dbg2;
> + memset(dbg2, '\0', sizeof(struct acpi_dbg2_header));
> + header = >header;
> +
> + header->revision = acpi_get_table_revision(ACPITAB_DBG2);
> + acpi_fill_header(header, "DBG2");
> + header->aslc_revision = ASL_REVISION;
> +
> + /* One debug device defined */
> + dbg2->devices_offset = sizeof(struct acpi_dbg2_header);
> + dbg2->devices_count = 1;
> + current += sizeof(struct acpi_dbg2_header);
> +
> + /* Device comes after the header */
> + device = (struct acpi_dbg2_device *)current;
> + memset(device, 0, sizeof(struct acpi_dbg2_device));
> + current += sizeof(struct acpi_dbg2_device);
> +
> + device->revision = 0;
> + device->address_count = 1;
> + device->port_type = port_type;
> + device->port_subtype = port_subtype;
> +
> + /* Base Address comes after device structure */
> + memcpy((void *)current, address, sizeof(struct acpi_gen_regaddr));
> + device->base_address_offset = current - (uintptr_t)device;
> + current += sizeof(struct acpi_gen_regaddr);
> +
> + /* Address Size comes after address structure */
> + dbg2_addr_size = (uint32_t *)current;
> + device->address_size_offset = current - (uintptr_t)device;
> + *dbg2_addr_size = address_size;
> + current += sizeof(uint32_t);
> +
> + /* Namespace string comes last, use '.' if not 

Re: [PATCH 034/108] x86: acpi: Move MADT up a bit

[Leif Lindholm]

On Sun, Jan 26, 2020 at 22:05:41 -0700, Simon Glass wrote:
> Put this table before MCFG so that it matches the order that coreboot uses
> when passing tables to Linux. This is a cosmetic change since the order of
> the tables does not otherwise matter.

The patch looks like it's doing the opposite of what the commit
message says. Rebasing issue, or am I being daft? (If this is a stack
operation, use of stack terminology in the commit message would be
more daft-friendly.)

/
Leif

> Signed-off-by: Simon Glass 
> ---
> 
>  arch/x86/lib/acpi_table.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> index 83b92e2a4c..694e92c158 100644
> --- a/arch/x86/lib/acpi_table.c
> +++ b/arch/x86/lib/acpi_table.c
> @@ -408,18 +408,18 @@ ulong write_acpi_tables(ulong start_addr)
>   acpi_create_fadt(fadt, facs, dsdt);
>   acpi_add_table(ctx, fadt);
>  
> - debug("ACPI:* MADT\n");
> - madt = ctx->current;
> - acpi_create_madt(madt);
> - acpi_inc_align(ctx, madt->header.length);
> - acpi_add_table(ctx, madt);
> -
>   debug("ACPI:* MCFG\n");
>   mcfg = ctx->current;
>   acpi_create_mcfg(mcfg);
>   acpi_inc_align(ctx, mcfg->header.length);
>   acpi_add_table(ctx, mcfg);
>  
> + debug("ACPI:* MADT\n");
> + madt = ctx->current;
> + acpi_create_madt(madt);
> + acpi_inc_align(ctx, madt->header.length);
> + acpi_add_table(ctx, madt);
> +
>   debug("ACPI:* CSRT\n");
>   csrt = ctx->current;
>   acpi_create_csrt(csrt);
> -- 
> 2.25.0.341.g760bfbb309-goog
> 

Re: [U-Boot] EFI_PXE_BASE_CODE_PROTOCOL

[Leif Lindholm]

On Tue, Aug 06, 2019 at 08:52:09PM +0200, Heinrich Schuchardt wrote:
> > > > EFI_PXE_BASE_CODE_PACKET DhcpAck is a union:
> > > > 
> > > > typedef union {
> > > >   UINT8 Raw[1472];
> > > >   EFI_PXE_BASE_CODE_DHCPV4_PACKET   Dhcpv4;
> > > >   EFI_PXE_BASE_CODE_DHCPV6_PACKET   Dhcpv6;
> > > > } EFI_PXE_BASE_CODE_PACKET;
> > > > 
> > > > Should the check be done in grub_efi_net_config_real()?
> > > 
> > > Possibly. I've cc:d Peter since he's the last person I know who took a
> > > proper look at this.
> > 
> > That's actually what we've got in our current patch set, based on
> > Michael Chang at SuSE's https work.  Javier Martinez (cc'd) is working
> > on getting those ready for upstream, but in the mean time, check out the
> > patches nearby to:
> > 
> > https://github.com/rhboot/grub2/commit/4f977935a9271727bf21889526fdf82f0fc8fed9
> 
> Thank you for the link.
> 
> There are currently no plans to implement these device paths in U-Boot:
> 
> * IPv4 Device Path
> * IPv6 Device Path
> * Uniform Resource Identifiers (URI) Device Path
> 
> I assume that these device paths would only be installed on handles
> implementing the EFI DHCPv4 Protocol or the EFI DHCPv6 Protocol but
> could not identify the relevant information in the specification.

10.3.4.12/13 (UEFI 2.8 spec) for IPv4/IPv6.
10.3.4.23 for URI.
(And 10.3.4.21 for iSCSI.)

But if you are only asking because you found the (NULLed-out) PXE
protocol implemented, then I would suggest we can ignore this for now.

I guess it could be useful for netbooting GRUB (the device paths, and
passing DHCP information retrieved by U-Boot into GRUB), not the
EFI_PXE_BASE_CODE_PROTOCOL).

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] EFI_PXE_BASE_CODE_PROTOCOL

[Leif Lindholm]

+Peter Jones (sorry Peter)

On Tue, Aug 06, 2019 at 08:34:58AM +0200, Heinrich Schuchardt wrote:
> iPXE uses the EFI simple network protocol to execute DHCP.

OK.

> Can GRUB already do the same when the EFI_PXE_BASE_CODE_PROTOCOL is not
> present?

Yes. As of very recently (proper* DHCP support was only merged in
March 2019, so is included in 2.04 release, prior to that it
technically performed BOOTP).

SNP means you do your own networking - it gives you access to the raw
(usually) Ethernet packets.

* proper as in "it now conceptually does the correct thing", not as in
  "I have extensively tested this".

> What I do not understand about GRUB's grub_net_configure_by_dhcp_ack()
> is that it silently assumes IPv4 being used without even checking. This
> contradicts the definition of the PXE base code protocol in the UEFI
> standard:

Well, it would not surprise me if this function predates GRUB's UEFI
support.

It actually gets even slightly messier when you look at what GRUB does
when netbooting itself; it starts out using MNP (and hence IP
addresses assigned by UEFI) to load its modules, switching to SNP once
it loads efinet.mod.

> EFI_PXE_BASE_CODE_PACKET DhcpAck is a union:
> 
> typedef union {
>  UINT8 Raw[1472];
>  EFI_PXE_BASE_CODE_DHCPV4_PACKET   Dhcpv4;
>  EFI_PXE_BASE_CODE_DHCPV6_PACKET   Dhcpv6;
> } EFI_PXE_BASE_CODE_PACKET;
> 
> Should the check be done in grub_efi_net_config_real()?

Possibly. I've cc:d Peter since he's the last person I know who took a
proper look at this.

Certainly, it would be useful if you could raise a bug on Savannah on
the ipv4 assumption.

Best Regards,

Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [RFC 1/1] efi_loader: usage of cleanup_before_linux()

[Leif Lindholm]

On Fri, Jul 19, 2019 at 07:26:19AM +0100, Peter Robinson wrote:
> > As of GRUB 2.04 release (and cherry-picked into debian Buster before
> > that), the 32-bit and 64-bit UEFI ports use the same loader.
> 
> Do you have a reference to this patch set handy?

I figured it might be good to write a proper summary and collect this
info somewhere, so I just posted that to:
https://lists.linaro.org/pipermail/cross-distro/2019-July/000933.html

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [RFC 1/1] efi_loader: usage of cleanup_before_linux()

[Leif Lindholm]

On Thu, Jul 18, 2019 at 08:53:01PM +0200, Alexander Graf wrote:
> 
> On 18.07.19 19:46, Heinrich Schuchardt wrote:
> > Always call cleanup_before_linux() on ARM 32bit during ExitBootServices().
> > 
> > This fixes a problem problem for booting BSD on ARM 32bit.
> > 
> > Reported-by: Jonathan Gray 
> > Signed-off-by: Heinrich Schuchardt 
> 
> NAK. Instead this should never call cleanup_before_linux() and we declare
> ourselves incompatible to old grub versions. That way we don't lure others
> into believing you could boot with caches disabled in a UEFI world.

Agreed.

This is my fault by the way, for merging a loader in GRUB that was
designed to be used without the linux EFI stub (way, wy back).
Feel free to shout.

As of GRUB 2.04 release (and cherry-picked into debian Buster before
that), the 32-bit and 64-bit UEFI ports use the same loader.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] U-Boot/EBBR plugfest at ELC-EU?

[Leif Lindholm]

On Mon, Jul 08, 2019 at 12:13:07PM +0200, Daniel Kiper wrote:
> > I don't know yet - UEFI Asia plugfest date hasn't been decided yet,
> > and is likely to end up around the same time. And actually another
> > unrelated event too.
> >
> > Certainly, Lyon is a quite convenient train journey from here :)
> >
> > But I'm also happy to look into GRUB issues on 32-bit systems remotely
> > if someone could point me at them.
> 
> I am not planning to be at ELC-E but I can help remotely if it is
> needed. However, there is another option. There is pretty good chance
> that I will get a MC slot at LPC. I am looking for people who want to
> talk. The overall plan is to devote this MC for boot stuff with focus
> on security. So, this maybe good place to discuss this. However, I am
> not ARM expert, so, I would like to see Leif and/or Alex or somebody
> else familiar with ARM stuff there too.

If I can get a ticket, I'm already intending to attend plumbers.

Registered on the waiting list.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [BUG] Wandboard fails to boot via U-Boot bootefi, GRUB

[Leif Lindholm]

On Tue, Jul 02, 2019 at 07:35:31PM +0200, Heinrich Schuchardt wrote:
> > This always felt somewhat precarious to me.
> > 
> > Could you possibly:
> > - add 'set debug=linux' in your grub.cfg and paste the logs
> > - add some printouts to arm32-stub.c
> > ?
> 
> That debug output is ever so strange. Why would loader/arm64/linux.c be
> invoked on a 32bit system?

Yes, this is a bit counterintuitive (and my fault).
Effectively, the arm64 loader is now effectively a generic UEFI linux
loader for DT platforms. Once the 2.04 release is out, I intend to
drop the last mentions of ARM*, shuffle it out to loader/efi, and
enable it also for Risc-V.

> This is the output on my Wandboard that fails to boot (I do not see much
> of a difference to the output for the OrangePi PC below):
> 
> Loading Linux 4.19.55-armmp ...
> loader/arm64/linux.c:60: UEFI stub kernel:
> loader/arm64/linux.c:61: PE/COFF header @ 4550
> loader/arm64/linux.c:313: kernel file size: 4231680
> loader/arm64/linux.c:316: kernel numpages: 1034
> loader/arm64/linux.c:332: kernel @ 0x6e202000

Hmm...

I will investigate this off-list with Heinrich, since it's far from
certain the problem is with U-Boot.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [BUG] Wandboard fails to boot via U-Boot bootefi, GRUB

[Leif Lindholm]

On Tue, Jul 02, 2019 at 07:12:11PM +0200, Heinrich Schuchardt wrote:
> > After LoadImage is called, the EFI stub of the kernel image determines
> > where the runtime kernel is going to be decompressed to (and I think
> > relocates zImage if necessary), and reserves this area, before
> > actually jumping to the zImage:
> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/firmware/efi/libstub/arm32-stub.c?h=v4.19.55#n194
> > 
> > This always felt somewhat precarious to me.
> > 
> > Could you possibly:
> > - add 'set debug=linux' in your grub.cfg and paste the logs
> > - add some printouts to arm32-stub.c
> 
> Is kernel v4.19.55 fine or should I use 5.2-rc7?

Well, you're seeing the issue on 4.19.55, so I think we should
investigate that. The only value I would see would be a sanity check
if the behaviour still exists with 5.2-rc7 (and if it doesn't use that
as help to determine what's going on).

I would be surprised if the behaviour has changed.

Regards,

Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [BUG] Wandboard fails to boot via U-Boot bootefi, GRUB

[Leif Lindholm]

Hello Heinrich,

On Sat, Jun 29, 2019 at 07:47:10PM +0200, Heinrich Schuchardt wrote:
> Hello Leif,
> 
> the Wandboard Quad rev B1 cannot be booted via U-Boot, GRUB-efi-arm.
> GRUB loads the initial ramdisk into an area which the the mainline
> 4.19.55 kernel simply does not accept because it thinks the minimum
> address is 0x6800 and not 0x1000. Booting via bootz works
> without problem.
> 
> Did you see a similar problems before?

Rereading your original report, I realise that the OF error messages
comletely distracted me from what you say in the text above: The
kernel thinks the minimum address is 0x6800 (suggesting that is
the address the zImage decompresses the runtime kernel to?).

Presumably when booting with 'bootz', the minimum address is correctly
determined to be 0x1000?

Where the 32-bit ARM kernel locates itself is unfortunately a bit of a
Rube Goldberg machine:
1) Grub/U-Boot loads the zImage *somewhere*
2) zImage decompresses itself to *somewhere*else* and jumps to the
  decompressed copy.

When booting with UEFI, *upstream* arm/arm64 GRUB loads the kernel
image with grub_efi_allocate_any_pages() and then calls
LoadImage/StartImage. (This step goes in between 1 and 2 above.)
Some of the distros carry addional patches for "shim" support that
modify this behaviour.

After LoadImage is called, the EFI stub of the kernel image determines
where the runtime kernel is going to be decompressed to (and I think
relocates zImage if necessary), and reserves this area, before
actually jumping to the zImage:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/firmware/efi/libstub/arm32-stub.c?h=v4.19.55#n194

This always felt somewhat precarious to me.

Could you possibly:
- add 'set debug=linux' in your grub.cfg and paste the logs
- add some printouts to arm32-stub.c
?

Also, what is the uncompressed size of your kernel image?

Best Regards,

Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [BUG] Wandboard fails to boot via U-Boot bootefi, GRUB

[Leif Lindholm]

On Mon, Jul 01, 2019 at 10:22:46PM +0200, Heinrich Schuchardt wrote:
> > Hmm. Well, my main concern is that we *should* be ignoring whatever is
> > in the DT when booting through UEFI (see Linux commit 500899c2cc3e3).
> > 
> > Could you try deleting the "memory" and "memory@1000" nodes from
> > the DT and see if that changes behaviour? (You probably want to delete
> > one of them regardless, since they describe the same region :)
> 
> setenv bootargs console=$console earlyprintk
> load mmc 2:2 $fdt_addr_r dtb
> fdt addr $fdt_addr_r
> fdt rm /memory
> fdt rm /memory@1000
> load mmc 2:1 $kernel_addr_r EFI/debian/grubarm.efi
> bootefi $kernel_addr_r $fdt_addr_r
> 
> leads to the same result.

Well, it was a bit optimistic hoping it would work...

> > If that doesn't change anything, how far back would we be able to
> > bisect and still be able to boot on this platform?
> 
> I do not know if it ever worked for this board. U-Boot v2019.01 fails too.

I was basically wondering if 500899c2cc3e3 would be likely to work on
this board in any environment?

> I have not seen the problem on 32bit Amlogic (OrangePi PC).
> 
> > Is this a distro kernel? Is the behaviour the same on upstream?
> 
> I have used 4.19.55 upstream together with the Debian config file but
> with earlyprintk enabled.

Ah, that makes sense.

Best Regards,

Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [BUG] Wandboard fails to boot via U-Boot bootefi, GRUB

[Leif Lindholm]

Hi,

On Mon, Jul 01, 2019 at 07:19:10PM +0200, Heinrich Schuchardt wrote:
> > According to this, we have an allocation of somewhat below 8MB, I
> > assume this matches the size of the initrd?
> 
> Thanks a lot for taking a look at this.
> 
> 31516827 bytes actually.
> 74715648 bytes after unzipping.

Yeah, so probably grub unzips it.

> > 
> > > 0007, 17f0c000, 17f0c000,162e5,8
> > > 0004, 17f0, 17f0,c,8
> > > 0002, 17d0, 17d0,  200,8
> > > 0007, 1240b000, 1240b000, 58f5,8
> > > 0002, 1200, 1200,  40b,8
> > > 0004, 1000, 1000, 2000,8
> > > 
> > > The initial ramdisk is loaded at 2e1f1000.
> > > 
> > > The problem occurs in drivers/of/fdt.c where some memory areas including
> > > the one containig the initial ramdisk are excluded. I have added some
> > > extra debug lines to early_init_dt_add_memory_arch().
> > 
> > Do you have a pointer to the device tree sources?
> > If the DT is explicitly excluding regions not marked such in the UEFI
> > memory map ... that would cause problems.
> 
> Please, find appended the device tree passed to U-Boot (dtb) and the
> printout of the devicetree upon entering SetVirtualAddressMap.

Thank you.

Hmm. Well, my main concern is that we *should* be ignoring whatever is
in the DT when booting through UEFI (see Linux commit 500899c2cc3e3).

Could you try deleting the "memory" and "memory@1000" nodes from
the DT and see if that changes behaviour? (You probably want to delete
one of them regardless, since they describe the same region :)

If that doesn't change anything, how far back would we be able to
bisect and still be able to boot on this platform?

Is this a distro kernel? Is the behaviour the same on upstream?

Regards,

Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [BUG] Wandboard fails to boot via U-Boot bootefi, GRUB

[Leif Lindholm]

Hi Heinrich,

On Sat, Jun 29, 2019 at 07:47:10PM +0200, Heinrich Schuchardt wrote:
> Hello Leif,
> 
> the Wandboard Quad rev B1 cannot be booted via U-Boot, GRUB-efi-arm.
> GRUB loads the initial ramdisk into an area which the the mainline
> 4.19.55 kernel simply does not accept because it thinks the minimum
> address is 0x6800 and not 0x1000. Booting via bootz works
> without problem.
> 
> Did you see a similar problems before?

Hmm...
It's been about 5 years since I looked at this code in Linux last, so
I may need to start with some stupid questions.

> This is the memory map when it is last read from U-Boot.
> 
> typ, phys, virt, pages, attrib
> 0002, 8f797000, 8f797000,  869,8
> 0005, 8f796000, 8f796000,1, 8008
> 0002, 8dd8c000, 8dd8c000, 1a0a,8
> , 8dd8b000, 8dd8b000,1,8
> 0006, 8dd8a000, 8dd8a000,1, 8008
> , 8dd83000, 8dd83000,7,8
> 0006, 8dd82000, 8dd82000,1, 8008
> , 8dd7e000, 8dd7e000,4,8
> 0001, 8dd67000, 8dd67000,   17,8
> 0004, 8dd66000, 8dd66000,1,8
> , 8dd64000, 8dd64000,2,8
> 0002, 8dd63000, 8dd63000,1,8
> 0006, 8dd62000, 8dd62000,1, 8008
> 0002, 8dd61000, 8dd61000,1,8
> 0001, 6e60c000, 6e60c000,1f755,8
> 0002, 6e1f8000, 6e1f8000,  414,8
> 0001, 6dded000, 6dded000,  40b,8
> 0002, 6ddec000, 6ddec000,1,8
> 0004, 6ddeb000, 6ddeb000,1,8
> 0002, 6dde9000, 6dde9000,2,8
> 0007, 2000, 2000,3ddea,8
> 0002, 2e1f1000, 2e1f1000, 1e0e,8

According to this, we have an allocation of somewhat below 8MB, I
assume this matches the size of the initrd?

> 0007, 17f0c000, 17f0c000,162e5,8
> 0004, 17f0, 17f0,c,8
> 0002, 17d0, 17d0,  200,8
> 0007, 1240b000, 1240b000, 58f5,8
> 0002, 1200, 1200,  40b,8
> 0004, 1000, 1000, 2000,8
> 
> The initial ramdisk is loaded at 2e1f1000.
> 
> The problem occurs in drivers/of/fdt.c where some memory areas including
> the one containig the initial ramdisk are excluded. I have added some
> extra debug lines to early_init_dt_add_memory_arch().

Do you have a pointer to the device tree sources?
If the DT is explicitly excluding regions not marked such in the UEFI
memory map ... that would cause problems.

Best Regards,

Leif

> [0.00] Booting Linux on physical CPU 0x0
> [0.00] Linux version 4.19.55-armmp (zfsdt@family) (gcc version
> 8.3.0 (Debian 8.3.0-6)) #8 SMP Sat Jun 29 17:04:52 CES9
> [0.00] CPU: ARMv7 Processor [412fc09a] revision 10 (ARMv7),
> cr=10c5387d
> [0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
> instruction cache
> [0.00] OF: fdt: Machine model: Wandboard i.MX6 Quad Board rev B1
> [0.00] OF: fdt: base 1000 < phys_offset 6800
> [0.00] OF: fdt: Ignoring memory range 0x1000 - 0x6800
> [0.00] bootconsole [earlycon0] enabled
> [0.00] Memory policy: Data cache writealloc
> [0.00] efi: Getting EFI parameters from FDT:
> [0.00] efi: EFI v2.80 by Das U-Boot
> [0.00] efi:  SMBIOS=0x8dd82000
> [0.00] OF: fdt: base 1000 + size 0200 <
> phys_offset 6800
> [0.00] OF: fdt: Ignoring memory block 0x1000 - 0x1200
> [0.00] OF: fdt: base 1200 + size 0040b000 <
> phys_offset 6800
> [0.00] OF: fdt: Ignoring memory block 0x1200 - 0x1240b000
> [0.00] OF: fdt: base 1240b000 + size 058f5000 <
> phys_offset 6800
> [0.00] OF: fdt: Ignoring memory block 0x1240b000 - 0x17d0
> [0.00] OF: fdt: base 17d0 + size 0020 <
> phys_offset 6800
> [0.00] OF: fdt: Ignoring memory block 0x17d0 - 0x17f0
> [0.00] OF: fdt: phys_offset 6800
> [0.00] OF: fdt: base 17f0 + size c000 <
> phys_offset 6800
> [0.00] OF: fdt: Ignoring memory block 0x17f0 - 0x17f0c000
> [0.00] OF: fdt: phys_offset 6800
> [0.00] OF: fdt: base 17f0c000 + size 162e5000 <
> phys_offset 

Re: [U-Boot] U-Boot/EBBR plugfest at ELC-EU?

[Leif Lindholm]

On Fri, Jun 28, 2019 at 12:17:54PM +0200, Alexander Graf wrote:
> On 28.06.19 12:03, Heinrich Schuchardt wrote:
> > I would be interested in joining. I hope that for the plugfest no ELC 
> > conference ticket will be needed.
> 
> The easiest option for that would be to submit a talk to ELC-E. IIRC the CfP
> is still open until July 1st. You could easily give one on "Progress in UEFI
> support on U-Boot" or "What it takes to run the edk2 Shell outside of edk2"
> or whatever :). You have plenty content to pull from.
> 
> If the talk goes in, that helps visibility and gets you free ticket - maybe
> even travel sponsorship! :)
> 
> > The problems with booting Linux via U-Boot UEFI I know of are on
> > 32bit ARM systems.  It could be helpful to have GRUB developers
> > joining.
> 
> Let's make sure Daniel and Leif are on CC then :).
> 
> Daniel, Leif, would you be available?

I don't know yet - UEFI Asia plugfest date hasn't been decided yet,
and is likely to end up around the same time. And actually another
unrelated event too.

Certainly, Lyon is a quite convenient train journey from here :)

But I'm also happy to look into GRUB issues on 32-bit systems remotely
if someone could point me at them.

Regards,

Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 0/2] efi_loader: implement deprecated Unicode collation protocol

[Leif Lindholm]

On Wed, May 29, 2019 at 12:09:43PM +0200, Alexander Graf wrote:
> > > > Do you want to change this policy?
> > > > (I'm just asking.)
> > > I am using SCT a lot to test my patches. I want to be able to run the
> > > tests on the final code.
> > Wouldn't it be better to patch/fork the upstream SCT that blindly
> > pulling in obsolete code that is basically already dead?
> > 
> > > I have raised a ticket for upstream SCT but did not see any reaction up
> > > to now.
> > Got a reference to the ticket?
> > 
> > > Making this deprecated protocol a config option deselected by default is
> > > the most plausible solution to me. In the Kconfig comment I made it
> > > clear that this protocol is going to be removed when the SCT has been
> > > corrected.
> > I disagree, I think if it's obsolete not having the code in the first
> > case in the most plausible option IMO.
> 
> Since it's hidden behind a default-n config option, I'd not be terribly
> opposed to it. I do agree that fixing SCT is the better path forward.
> 
> Leif, how long do you think fixing this properly in SCT is going to take?

Well, as usual, patches welcome :)

However, I think the problem is more that the separate SCT area of
bugzilla is a fairly recent addition and they forgot to change the
default assignee to someone who actually works on SCT...

I have now added the SCT maintainers to cc on
https://bugzilla.tianocore.org/show_bug.cgi?id=1802

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] [U-boot]: Change FDT memory typpe from runtime data to acpi reclaim

[Leif Lindholm]

On Thu, Apr 11, 2019 at 10:08:10PM +0200, Heinrich Schuchardt wrote:
> > > How about systab.tables assigned in efi_initialize_system_table()? Which
> > > of the addresses in systab.tables should be updated upon relocation.
> > > 
> > > The EFI Spec is really hazy: "A pointer to the table associated with
> > > VendorGuid. Whether this pointer is a physical address or a
> > > virtual address during runtime is determined by the VendorGuid."
> > > 
> > 
> > Indeed. So it is up to the publisher to update the reference, but I am
> > not aware of any firmware implementations that do this in practice. It
> > is typically assumed that a firmware component that is still active at
> > runtime holds its own reference to data exposed via a configuration
> > table, and updates the reference during SetVirtualAddressMap.
> > 
> > There is also a known bug in EDK2 where the ESRT table is passed in
> > boot services memory, but the capsule update code actually tries to
> > access it at runtime, so this isn't as clean as we'd like it to be.
> > 
> > > The FDT_TABLE_GUID or DEVICE_TREE_GUID as Linux calls it is not defined
> > > in the UEFI spec. So we can marvel about expected behavior. Is there any
> > > other document specifying it?
> > > 
> > 
> > No, its de facto specification is in the EDK2 source tree.

Well, in reality, it appeared simultanously-ish in Linux and GRUB, and
I think is entry into the EDK2 codebase came later :)

> As all ARM systems use it I guess this GUID should move into the UEFI
> spec. Maybe Linaro could raise this issue.

While the UEFI specification does list the ACPI, ACPI_20, SMBIOS, and
some other common GUIDs, this is not where they are defined. If it
needs to go into some specification, that would probably be the
devicetree.org one.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PULL] efi patch queue 2019-01-28

[Leif Lindholm]

Hi Tom, Alex,

On Tue, Jan 29, 2019 at 09:39:50AM -0500, Tom Rini wrote:
> > for you to fetch changes up to b90a387650970fa9dbda7403070dbbf564cf81b8:
> > 
> >   efi_loader: debug output for HII protocols (2019-01-26 11:48:18 +0100)
> > 
> 
> I'm adding a few people to the mail here.  First, we have:
> commit ae4eb8232d927e4f630408f2533f06a5fb1b32a4
> Author: Leif Lindholm 
> Date:   Mon Jan 21 12:12:57 2019 +0900
> 
> efi_loader: Initial HII database protocols
> 
> which comes in without his, but with others Signed-off-by lines.  This
> is just an oversight on Leif's part and this code can come in just fine
> and we don't need to revert things, right?  This is a good bit more code
> than is in the occasional bugfix that I let in that's missing the SoB
> line.

My contribution was a bunch of structs, enums, #defines and stub
functions to demonstrate what needed to be implemented in order for
the EDK2 UEFI Shell to run. Which the others fixed up and filled in
with actual functionality. So, fwiw, I'm happy for my SoB to be added
(and I wouldn't be in the least bit offended if someone wanted to
reassign the authorship).

(for ae4eb8232d927e4f630408f2533f06a5fb1b32a4:
Signed-off-by: Leif Lindholm )

/
Leif

> Next, we have:
> https://travis-ci.org/trini/u-boot/jobs/485695447
> which is because, oh, right, we need both:
> https://patchwork.ozlabs.org/patch/1031448/ which is a "me" patch and
> Stephen was OK with too but also:
> https://patchwork.ozlabs.org/patch/1030669/ which is an x86 patch I
> assigned to Bin and since he's not acked/reviewed it I don't feel
> comfortable grabbing it directly.
> 
> So for the moment, this PR is on hold.
> 
> -- 
> Tom


___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [RESEND PATCH v2 2/6] efi_loader: Initial HII database protocols

[Leif Lindholm]

Thanks Liming, this exactly the reply I was hoping for.

On Tue, Jan 08, 2019 at 03:12:11PM +, Gao, Liming wrote:
> EFI_GUID structure definition follows RFC UUID
> https://www.ietf.org/rfc/rfc4122.txt. This RFC has no 64 bit
> boundary requirement. I don't know the background why UEFI spec
> requires to align on 64-bit boundary. This may be true for early IPF
> arch. UEFI forum can clarify its purpose. If no specific reason, I
> suggest to follow the industry standard GUID format, and update UEFI
> spec.

Since there would be no 64-bit alignment requirements for IPF either
for correctness reasons, I expect it was added for performance reasons.

> On pack in structure EFI_HII_KEYBOARD_LAYOUT, UEFI2.7 32.3 Code
> Definitions has one sentence that this chapter describes the binary
> encoding of the different package types. 32.3.3 Font Package has the
> additional statement that structures described here are byte
> packed. Base on those description, we can infer HII package data is
> the byte packed. I agree to obviously specify that structures
> described here are byte packed in 32.3 section.

That sounds good to me.

> Last, EFI_HII_KEYBOARD_PACKAGE_HDR structure definition doesn't
> follow UEFI spec. I remember we ever meet with the compiler issue
> for below style. GCC49 may complaint it. I need to double confirm.
> typedef struct {
>   EFI_HII_PACKAGE_HEADER  Header;
>   UINT16  LayoutCount;
>   EFI_HII_KEYBOARD_LAYOUT Layout[];
> } EFI_HII_KEYBOARD_PACKAGE_HDR;

I did remark on this to Ard, and he pointed out the old compiler
issue. If I delete those comment markers, I cannot reproduce a problem
with either GCC48 or GCC49 (on those actual compiler versions) on ARM.

Right, I will put together an email to USWG with you on cc.

Regards,

Leif

> 
> Thanks
> Liming
> > -Original Message-
> > From: Laszlo Ersek [mailto:ler...@redhat.com]
> > Sent: Tuesday, January 8, 2019 7:56 PM
> > To: Leif Lindholm 
> > Cc: AKASHI Takahiro ; Alexander Graf 
> > ; Heinrich Schuchardt ;
> > tr...@konsulko.com; robdcl...@gmail.com; u-boot@lists.denx.de; 
> > edk2-de...@lists.01.org; Wang, Jian J ; Wu,
> > Hao A ; Ni, Ray ; Zeng, Star 
> > ; Andrew Fish ; Kinney,
> > Michael D ; Ard Biesheuvel 
> > ; Gao, Liming 
> > Subject: Re: [RESEND PATCH v2 2/6] efi_loader: Initial HII database 
> > protocols
> > 
> > On 01/08/19 10:51, Leif Lindholm wrote:
> > > MdePkg/MdeModulePkg maintainers - any comments?
> > >
> > > On Tue, Jan 08, 2019 at 01:28:00AM +0100, Laszlo Ersek wrote:
> > >> On 01/07/19 20:22, Leif Lindholm wrote:
> > >>> On Mon, Jan 07, 2019 at 07:29:47PM +0100, Laszlo Ersek wrote:
> > >>
> > >>>> The UEFI spec (v2.7) explicitly requires EFI_GUID to be 64-bit aligned,
> > >>>> unless specified otherwise. See in "Table 5. Common UEFI Data Types":
> > >>>>
> > >>>>   EFI_GUID -- 128-bit buffer containing a unique identifier value.
> > >>>>   Unless otherwise specified, aligned on a 64-bit
> > >>>>   boundary.
> > >>>
> > >>> Indeed.
> > >>>
> > >>>> Whether edk2 satisfies that, and if so, how (by chance / by general
> > >>>> build flags), I don't know. The code says,
> > >>>>
> > >>>> ///
> > >>>> /// 128 bit buffer containing a unique identifier value.
> > >>>> /// Unless otherwise specified, aligned on a 64 bit boundary.
> > >>>> ///
> > >>>> typedef struct {
> > >>>>   UINT32  Data1;
> > >>>>   UINT16  Data2;
> > >>>>   UINT16  Data3;
> > >>>>   UINT8   Data4[8];
> > >>>> } GUID;
> > >>>>
> > >>>> I think there may have been an expectation in "MdePkg/Include/Base.h"
> > >>>> that the supported compilers would automatically ensure the specified
> > >>>> alignment, given the structure definition.
> > >>>
> > >>> But that would be expecting things not only not guaranteed by C, but
> > >>> something there is no semantic information suggesting would be useful
> > >>> for the compiler to do above. [...]
> > >>
> > >> Agreed. I'm not saying the edk2 code is right, just guessing why the
> > >> code might look like it does. This would not be the first silent
> > >> assumption, I think.
> > >>
> > >> Anyhow, I think it would be better to chang

Re: [U-Boot] [RESEND PATCH v2 2/6] efi_loader: Initial HII database protocols

[Leif Lindholm]

MdePkg/MdeModulePkg maintainers - any comments?

On Tue, Jan 08, 2019 at 01:28:00AM +0100, Laszlo Ersek wrote:
> On 01/07/19 20:22, Leif Lindholm wrote:
> > On Mon, Jan 07, 2019 at 07:29:47PM +0100, Laszlo Ersek wrote:
> 
> >> The UEFI spec (v2.7) explicitly requires EFI_GUID to be 64-bit aligned,
> >> unless specified otherwise. See in "Table 5. Common UEFI Data Types":
> >>
> >>   EFI_GUID -- 128-bit buffer containing a unique identifier value.
> >>   Unless otherwise specified, aligned on a 64-bit
> >>   boundary.
> > 
> > Indeed.
> > 
> >> Whether edk2 satisfies that, and if so, how (by chance / by general
> >> build flags), I don't know. The code says,
> >>
> >> ///
> >> /// 128 bit buffer containing a unique identifier value.
> >> /// Unless otherwise specified, aligned on a 64 bit boundary.
> >> ///
> >> typedef struct {
> >>   UINT32  Data1;
> >>   UINT16  Data2;
> >>   UINT16  Data3;
> >>   UINT8   Data4[8];
> >> } GUID;
> >>
> >> I think there may have been an expectation in "MdePkg/Include/Base.h"
> >> that the supported compilers would automatically ensure the specified
> >> alignment, given the structure definition.
> > 
> > But that would be expecting things not only not guaranteed by C, but
> > something there is no semantic information suggesting would be useful
> > for the compiler to do above. [...]
> 
> Agreed. I'm not saying the edk2 code is right, just guessing why the
> code might look like it does. This would not be the first silent
> assumption, I think.
> 
> Anyhow, I think it would be better to change the code than the spec.

Of course it would be better to change the code than the spec.

But as Ard points out off-thread, doing (as a hack, with gcc)

diff --git a/MdePkg/Include/Uefi/UefiBaseType.h
b/MdePkg/Include/Uefi/UefiBaseType.h
index 8c9d571eb1..75409f3460 100644
--- a/MdePkg/Include/Uefi/UefiBaseType.h
+++ b/MdePkg/Include/Uefi/UefiBaseType.h
@@ -26,7 +26,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
EITHER EXPRESS OR IMPLIED.
 ///
 /// 128-bit buffer containing a unique identifier value.
 ///
-typedef GUID  EFI_GUID;
+typedef GUID  EFI_GUID __attribute__((aligned (8)));
 ///
 /// Function return status for EFI API.
 ///

breaks Linux boot on ARM (32-bit), since it inserts 32-bits of padding
between ConfigurationTable entries in the system table. So I don't see
how that can realistically be fixed in the EDK2 codebase.

And with things like the EFI_HII_KEYBOARD_LAYOUT struct, if there has
ever been compatibility between EDK2 and commercial BIOSes, then that
struct has always been treated as packed (not just 32-bit aligned
GUIDs), and the spec just needs to reflect reality. If there hasn't,
then indeed the code change here would be trivial.

(Adding Liming as well, since we're now discussing MdePkg also.)

Yes, this discussion belongs on USWG (UEFI specification working group
mailing list), but I want to hear some comment from the package
maintainers first.

Either way, I see a bunch of new SCT tests coming up.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [RESEND PATCH v2 2/6] efi_loader: Initial HII database protocols

[Leif Lindholm]

On Mon, Jan 07, 2019 at 07:29:47PM +0100, Laszlo Ersek wrote:
> On 01/07/19 15:09, Leif Lindholm wrote:
> > Apologies for late reply, back from holidays today.
> 
> I'm going to snip a whole lot of context below, since I have no idea
> what project this is about, and/or what files in that project (no diff
> hunk headers in the context). Judged from the address list, is this
> about u-boot perhaps? And adding type definitions to u-boot so they
> conform to the UEFI spec, and (assuming this "and" is possible) match
> edk2 practice?

Well, the u-boot UEFI interface is what triggered the questions.
And the answer is relevant to the people asking, so I kept the list on
cc.

But I'm more concerned with regards to whether EDK2 is compliant, and
what impacts this has on the spec if not.

> > My understanding is this:
> > - The EDK2 implementation does not conform to the specification; it
> >   completely packs the EFI_HII_KEYBOARD_LAYOUT, which the
> >   specification does not mention anything about. Since this code is
> >   well in the wild, and drivers tested against the current layout need
> >   to continuw eorking, I expect the only possible solution is to
> >   update the specification to say EFI_HII_KEYBOARD_LAYOUT must be
> >   packed.
> 
> I'm going to take a pass on this. :)

Be my guest :)

> > - The default EDK2 definition of GUID  (and hence EFI_GUID) gives it a
> >   32-bit alignment requirement also on 64-bit architectures. In
> >   practice, I expect this would only affect (some of the) GUIDs that
> >   are members of structs used in UEFI interfaces. But that may already
> >   be too common an occurrence to audit and address in EDK2. Does the
> >   spec need to change on this also?
> 
> The UEFI spec (v2.7) explicitly requires EFI_GUID to be 64-bit aligned,
> unless specified otherwise. See in "Table 5. Common UEFI Data Types":
> 
>   EFI_GUID -- 128-bit buffer containing a unique identifier value.
>   Unless otherwise specified, aligned on a 64-bit
>   boundary.

Indeed.

> Whether edk2 satisfies that, and if so, how (by chance / by general
> build flags), I don't know. The code says,
> 
> ///
> /// 128 bit buffer containing a unique identifier value.
> /// Unless otherwise specified, aligned on a 64 bit boundary.
> ///
> typedef struct {
>   UINT32  Data1;
>   UINT16  Data2;
>   UINT16  Data3;
>   UINT8   Data4[8];
> } GUID;
> 
> I think there may have been an expectation in "MdePkg/Include/Base.h"
> that the supported compilers would automatically ensure the specified
> alignment, given the structure definition.

But that would be expecting things not only not guaranteed by C, but
something there is no semantic information suggesting would be useful
for the compiler to do above. C is quite explicit that the above would
be given a 32-bit alignment requirement. The most aligned member is
Data1, and its alignment is 32 bits.

Now, technically, it would be absolutely fine for this struct to be
32-but aligned, if the EFI_GUID typedef in
MdePkg/Include/Uefi/UefiBaseType.h added this constraint. It does not.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [RESEND PATCH v2 2/6] efi_loader: Initial HII database protocols

[Leif Lindholm]

Apologies for late reply, back from holidays today.

On Tue, Dec 25, 2018 at 05:30:25PM +0900, AKASHI Takahiro wrote:
> > >>> +struct efi_key_descriptor {
> > >>> +   efi_key key;
> > >>
> > >> Hello Takahiro,
> > >>
> > >> with the patch I can start the EFI shell. But I am still trying to check
> > >> the different definitions in this file.
> > >>
> > >> As mentioned in prior mail the size of enum is not defined in the C
> > >> spec. So better use u32.
> > > 
> > > Sure, but I still wonder whether this should be u32 or u16 (or even u8)
> > > as UEFI spec doesn't clearly define this.
> > 
> > Enums may hold up to INT_MAX, so just make it u32.
> 
> OK.
> 
> > > 
> > >>> +   u16 unicode;
> > >>> +   u16 shifted_unicode;
> > >>> +   u16 alt_gr_unicode;
> > >>> +   u16 shifted_alt_gr_unicode;
> > >>> +   u16 modifier;
> > >>> +   u16 affected_attribute;
> > >>> +};
> > >>> +
> > >>> +struct efi_hii_keyboard_layout {
> > >>> +   u16 layout_length;
> > >>> +   efi_guid_t guid;
> > >>
> > >> A patch to change the alignment of efi_guid_t to __alinged(8) has been
> > >> merged into efi-next.
> > > 
> > > I have one concern here;
> > > This structure will also be used as a data format/layout in a file,
> > > a package list, given the fact that UEFI defines ExportPackageLists().
> > > So extra six bytes after layout_length, for example, may not be very
> > > useful in general.
> > > I'm not very much sure if UEFI spec intends so.
> > 
> > I'm not sure I fully follow. We ideally should be compatible with edk2,
> > no? So if the spec isn't clear, let's make sure we clarify the spec to
> > the behavior edk2 exposes today.

The spec cannot simply be changed to be incompatible with a previous
version of the spec, regardless of what EDK2 happens to do. (But...)

> I'm not sure that EDK2 is very careful about data alignment.
> Regarding guid, in MdePkg/Include/Base.h,
>   ///
>   /// 128 bit buffer containing a unique identifier value.
>   /// Unless otherwise specified, aligned on a 64 bit boundary.
>   ///
>   typedef struct {
> UINT32  Data1;
> UINT16  Data2;
> UINT16  Data3;
> UINT8   Data4[8];
>   } GUID;
> in MdePkg/Include/Uefi/UefiBaseType.h,
>   typedef GUID  EFI_GUID;
> 
> There is no explicit "aligned()" attribute contrary to Heinrich's patch.

No, so the alignment when building for (any) ARM architecture will end
up being 32-bit. Which I agree does not seem to live up to the
specification's requirement on 64-bit alignment where nothing else is
said.

Since, obviously, u-boot and edk2 disagreeing about the layout of
structs that are exposed to external applications/drivers would defeat
this whole exercise, I think we should start by taking this question
to edk2-devel (which I have).

> Regarding hii_keyboard_layout,
> in  Include/Uefi/UefiInternalFormRepresentation.h,
>   typedef struct {
> UINT16  LayoutLength;
> EFI_GUIDGuid;
> UINT32  LayoutDescriptorStringOffset;
> UINT8   DescriptorCount;
> // EFI_KEY_DESCRIPTORDescriptors[];
>   } EFI_HII_KEYBOARD_LAYOUT;
> 
> No "packed" attribute, so neither in my code.

There is a #pragma pack(1) near the start of this file and a #pragma
pack() near the end.

Interestingly, UEFI 2.7a describes the EFI_HII_KEYBOARD_LAYOUT struct
as containing the EFI_KEY_DESCRIPTOR array at the end, whereas the
EDK2 code above has it commented it out.
EDK2 itself treats the EFI_HII_KEYBOARD_LAYOUT as a header, which is
discarded.

So, continuning the (But...)
My understanding is this:
- The EDK2 implementation does not conform to the specification; it
  completely packs the EFI_HII_KEYBOARD_LAYOUT, which the
  specification does not mention anything about. Since this code is
  well in the wild, and drivers tested against the current layout need
  to continuw eorking, I expect the only possible solution is to
  update the specification to say EFI_HII_KEYBOARD_LAYOUT must be
  packed.
- The default EDK2 definition of GUID  (and hence EFI_GUID) gives it a
  32-bit alignment requirement also on 64-bit architectures. In
  practice, I expect this would only affect (some of the) GUIDs that
  are members of structs used in UEFI interfaces. But that may already
  be too common an occurrence to audit and address in EDK2. Does the
  spec need to change on this also?

Can the TianoCore MdeModulePkg Maintainers on cc please comment?
(I also cc:d the other stewards as well as Ard, in case they have
further input.)

> > >>> +   u32 layout_descriptor_string_offset;
> > >>> +   u8 descriptor_count;
> > >>> +   struct efi_key_descriptor descriptors[];
> > >>> +};
> > >>> +
> > >>> +struct efi_hii_package_list_header {
> > >>> +   efi_guid_t package_list_guid;
> > >>> +   u32 package_length;
> > >>> +} __packed;
> > >>
> > >> You are defining several 

Re: [U-Boot] [PATCH 02/11] efi_loader: Initial HII protocols

[Leif Lindholm]

On Fri, Oct 05, 2018 at 05:52:09PM +0900, AKASHI, Takahiro wrote:
> > > 2. If a platform includes a configuration infrastructure, then the
> > > EFI_HII_DATABASE_PROTOCOL, EFI_HII_STRING_PROTOCOL,
> > > EFI_HII_CONFIG_ROUTING_PROTOCOL, and EFI_HII_CONFIG_ACCESS_PROTOCOL are
> > > required.
> >
> > Do you think efi implementation on u-boot also wants those protocols?
> > (Apparently, config access protocol can be optional for shell unless
> > we want driver configuration?)
> 
> One more question:
> Does u-boot support any kind of "UEFI driver"?
> If yes, is there any good example?
> If no, do you expect that it should be supported?

I don't think full-on option ROMs with configuration menus are
something we care about for EBBR-style implementations.
What could be useful is things like shim - a simple driver installing
a protocol that lets other applications running at boot-time access
it. But I think that is already (mostly?) supported.

If someone at a later date decides that they want to support option
ROMs, basically using U-Boot for an SBBR implementation, that will
come with additional work required for the menu support. And should be
possible to configure out at build time for users who don't want it.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 4/5] efi_loader: EFI_UNICODE_COLLATION_PROTOCOL

[Leif Lindholm]

On Wed, Sep 05, 2018 at 12:04:04AM +0200, Alexander Graf wrote:
> > I just removed your patch
> > XXX efi_loader collation: Expose v1 GUID as well
> > from https://github.com/agraf/u-boot/commits/ebbr-demo
> > and I am still able to run Shell.efi.
> 
> I just double checked and Shell.efi definitely fails for me without the
> v1 protocol patch:
> 
>   $ wget
> 'https://github.com/ARM-software/edk2/raw/master/ShellBinPkg/UefiShell/AArch64/Shell.efi'

I'm not even going to ask why you are using that repository.
I want to know how you _found_ that repository!?

Latest commit 16 July 2015.

Please use only https://github.com/tianocore/edk2.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [RFC] efi_loader: workaround for EDK2's shell.efi

[Leif Lindholm]

On Thu, Aug 09, 2018 at 03:15:38PM +0900, AKASHI Takahiro wrote:
> The commit 21b3edfc964 ("efi_loader: check parameters of CreateEvent")
> enforces a strict parameter check at CreateEvent().  Unfortunately,
> however, EDK2's Shell.efi calls this function with notify_tpl == 0.

I find this done in CreatePopulateInstallShellProtocol() in
Application/Shell/ShellProtocol.c, is that the one you see?

> The patch above does right thing and we'd better fix the issue on EDK2
> side, and yet we might want a workaround allowing for running un-modified
> version of EDK2 in short-term solution.

Where we find non-spec-compliant code in EDK2, we want to fix EDK2.
That doesn't mean that we don't perhaps want to work around it in
U-Boot anyway. But if we do, I would prefer if we could spam the
console a bit as well, to warn people of badly behaving apps.

However...

> The patch provides a minimum mitigation of parameter check.
> 
> Signed-off-by: AKASHI Takahiro 
> ---
>  lib/efi_loader/efi_boottime.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 2281703f261..e7a19c35415 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -627,7 +627,8 @@ efi_status_t efi_create_event(uint32_t type, efi_uintn_t 
> notify_tpl,
>   return EFI_INVALID_PARAMETER;
>   }
>  
> - if (is_valid_tpl(notify_tpl) != EFI_SUCCESS)
> + /* notify_tpl == 0: workaround for EDK2's Shell.efi */
> + if (notify_tpl && (is_valid_tpl(notify_tpl) != EFI_SUCCESS))

From the UEFI spec (2.7) description of CreateEvent() boot service:
---
The EVT_NOTIFY_WAIT and EVT_NOTIFY_SIGNAL flags are exclusive. If
neither flag is specified, the caller does not require any
notification concerning the event and the NotifyTpl, NotifyFunction,
and NotifyContext parameters are ignored.
---

So it's not a workaround for Shell specifically.
However, based on that text, something like

if (type & (EVT_NOTIFY_WAIT | EVT_NOTIFY_SIGNAL))
if ((is_valid_tpl(notify_tpl) != EFI_SUCCESS))

may resolve this in a more compliant way.

Of course, this may require additional changes to the remainder of the
function.

/
Leif

>   return EFI_INVALID_PARAMETER;
>  
>   evt = calloc(1, sizeof(struct efi_event));
> -- 
> 2.18.0
> 
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/3] arm: armv7: allow unaligned memory access

[Leif Lindholm]

On Wed, Apr 04, 2018 at 11:46:45AM +0200, Alexander Graf wrote:
> On 03.04.18 21:59, Heinrich Schuchardt wrote:
> > The UEFI spec mandates that unaligned memory access should be enabled if
> > supported by the CPU architecture.
> > 
> > This patch implements the function unaligned_access() to reset the aligned
> > access flag in the system control register (SCTLR). It is called when the
> > bootefi command is invoked.
> > 
> > Signed-off-by: Heinrich Schuchardt 
> 
> This will not work unfortunately. Unaligned accesses are only handled by
> hardware on armv7 (and armv8) when the underlying memory is mapped as
> cached. Given the extremely wild situation on armv7 for page table
> setups, I'm not terribly confident to give anyone even the slightest
> hint that unaligned accesses would work on armv7.

Then having RAM mapped as Normal (not necessarily cacheable) should be
an automatic prerequisite for CONFIG_EFI, and if possible we should
write scary error messages to the console if this is not the case.

> IMHO we should really update the UEFI spec and mandate that all memory
> accesses have to be aligned on armv7.

Changing ABIs in backwards-incompatible ways is the opposite of what
UEFI is for.

> It's what grub is doing today already:
> 
>   http://git.savannah.gnu.org/cgit/grub.git/tree/configure.ac#n1279

Don't read too much into that. That comes for three reasons:
1) The U-boot API port.
2) Support for pre-v7 devices (like raspberry pi 1).
3) The wild changes in toolchain behaviour seen between
   various releases between gcc ~4.5 and gcc5.

There is no reason for that to be used for the arm-efi target, it's
just pure laziness on my behalf not to have changed it when the
arm-efi port was added.

> My recommendation would be to just pass the same compiler flags to iPXE
> on armv7.

Please don't.

/
Leif

> 
> Alex
> 
> > ---
> >  arch/arm/cpu/armv7/Makefile |  4 
> >  arch/arm/cpu/armv7/sctlr.S  | 23 +++
> >  2 files changed, 27 insertions(+)
> >  create mode 100644 arch/arm/cpu/armv7/sctlr.S
> > 
> > diff --git a/arch/arm/cpu/armv7/Makefile b/arch/arm/cpu/armv7/Makefile
> > index b14ee54519..cdb56e490b 100644
> > --- a/arch/arm/cpu/armv7/Makefile
> > +++ b/arch/arm/cpu/armv7/Makefile
> > @@ -12,6 +12,10 @@ obj-y+= cache_v7.o cache_v7_asm.o
> >  obj-y  += cpu.o cp15.o
> >  obj-y  += syslib.o
> >  
> > +ifneq ($(CONFIG_SPL_BUILD),y)
> > +obj-$(CONFIG_EFI_LOADER) += sctlr.o
> > +endif
> > +
> >  ifneq ($(CONFIG_SKIP_LOWLEVEL_INIT),y)
> >  obj-y  += lowlevel_init.o
> >  endif
> > diff --git a/arch/arm/cpu/armv7/sctlr.S b/arch/arm/cpu/armv7/sctlr.S
> > new file mode 100644
> > index 00..cfdb7d2a52
> > --- /dev/null
> > +++ b/arch/arm/cpu/armv7/sctlr.S
> > @@ -0,0 +1,23 @@
> > +/*
> > + *  Routines to access the system control register
> > + *
> > + *  Copyright (c) 2018 Heinrich Schuchardt
> > + *
> > + *  SPDX-License-Identifier: GPL-2.0+
> > + */
> > +
> > +#include 
> > +
> > +/*
> > + * void allow_unaligned(void) - allow unaligned access
> > + *
> > + * This routine clears the aligned flag in the system control register.
> > + * After calling this routine unaligned access does no longer lead to a
> > + * data abort but is handled by the CPU.
> > + */
> > +ENTRY(allow_unaligned)
> > +   mrc p15, 0, r0, c1, c0, 0   @ load system control register
> > +   bic r0, r0, #2  @ clear aligned flag
> > +   mcr p15, 0, r0, c1, c0, 0   @ write system control register
> > +   bx  lr  @ return
> > +ENDPROC(allow_unaligned)
> > 
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 2/2] efi_loader: provide new doc/README.uefi

[Leif Lindholm]

On Fri, Mar 02, 2018 at 07:58:50PM +0100, Heinrich Schuchardt wrote:
> Provides information about
> 
> - usage of the bootefi command
> - overview of UEFI
> - interaction between U-Boot and EFI drivers
> 
> Signed-off-by: Heinrich Schuchardt <xypron.g...@gmx.de>

Well, from my point of view:
Reviewed-by: Leif Lindholm <leif.lindh...@linaro.org>
or
Acked-by: Leif Lindholm <leif.lindh...@linaro.org>
(whichever makes more sense).

Many thanks!

/
Leif

> ---
> v3
>   rename README.efi to README.uefi
>   use UEFI instead of EFI where applicable
> v2
>   split the patch in two: one deleteing all old lines, the other
>   adding the new ones
>   update README contents
> ---
>  MAINTAINERS |   2 +-
>  doc/README.efi  |   0
>  doc/README.uefi | 332 
> 
>  3 files changed, 333 insertions(+), 1 deletion(-)
>  delete mode 100644 doc/README.efi
>  create mode 100644 doc/README.uefi
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 077828cf1d4..da799b551d9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -290,7 +290,7 @@ EFI PAYLOAD
>  M:   Alexander Graf <ag...@suse.de>
>  S:   Maintained
>  T:   git git://github.com/agraf/u-boot.git
> -F:   doc/README.efi
> +F:   doc/README.uefi
>  F:   doc/README.iscsi
>  F:   include/efi*
>  F:   include/pe.h
> diff --git a/doc/README.efi b/doc/README.efi
> deleted file mode 100644
> index e69de29bb2d..000
> diff --git a/doc/README.uefi b/doc/README.uefi
> new file mode 100644
> index 000..7403be36146
> --- /dev/null
> +++ b/doc/README.uefi
> @@ -0,0 +1,332 @@
> +
> +
> +# UEFI on U-Boot
> +
> +The Unified Extensible Firmware Interface Specification (UEFI) [1] has become
> +the default for booting on AArch64 and x86 systems. It provides a stable API 
> for
> +the interaction of drivers and applications with the firmware. The API 
> comprises
> +access to block storage, network, and console to name a few. The Linux kernel
> +and boot loaders like GRUB or the FreeBSD loader can be executed.
> +
> +## Building for UEFI
> +
> +The UEFI standard supports only little endian systems. The UEFI support can 
> be
> +activated for ARM and x86 by specifying
> +
> +CONFIG_CMD_BOOTEFI=y
> +CONFIG_EFI_LOADER=y
> +
> +in the .config file.
> +
> +Support for attaching virtual block devices, e.g. iSCSI drives connected by 
> the
> +loaded UEFI application [3], requires
> +
> +CONFIG_BLK=y
> +CONFIG_PARTITIONS=y
> +
> +### Executing a UEFI binary
> +
> +The bootefi command is used to start UEFI applications or to install UEFI
> +drivers. It takes two parameters
> +
> +bootefi  [fdt address]
> +
> +* image address - the memory address of the UEFI binary
> +* fdt address - the memory address of the flattened device tree
> +
> +Below you find the output of an example session starting GRUB.
> +
> +=> load mmc 0:2 ${fdt_addr_r} boot/dtb
> +29830 bytes read in 14 ms (2 MiB/s)
> +=> load mmc 0:1 ${kernel_addr_r} efi/debian/grubaa64.efi
> +reading efi/debian/grubaa64.efi
> +120832 bytes read in 7 ms (16.5 MiB/s)
> +=> bootefi ${kernel_addr_r} ${fdt_addr_r}
> +
> +The environment variable 'bootargs' is passed as load options in the UEFI 
> system
> +table. The Linux kernel EFI stub uses the load options as command line
> +arguments.
> +
> +### Executing the boot manager
> +
> +The UEFI specfication foresees to define boot entries and boot sequence via 
> UEFI
> +variables. Booting according to these variables is possible via
> +
> +bootefi bootmgr [fdt address]
> +
> +As of U-Boot v2018.03 UEFI variables are not persisted and cannot be set at
> +runtime.
> +
> +### Executing the built in hello world application
> +
> +A hello world UEFI application can be built with
> +
> +CONFIG_CMD_BOOTEFI_HELLO_COMPILE=y
> +
> +It can be embedded into the U-Boot binary with
> +
> +CONFIG_CMD_BOOTEFI_HELLO=y
> +
> +The bootefi command is used to start the embedded hello world application.
> +
> +bootefi hello [fdt address]
> +
> +Below you find the output of an example session.
> +
> +=> bootefi hello ${fdtcontroladdr}
> +## Starting EFI application at 0100 ...
> +WARNING: using memory device/image path, this may confuse some payloads!
> +Hello, world!
> +Running on UEFI 2.7
> +Have SMBIOS table
> +Have device tree
> +Load options: root=/dev/sdb3 init=/sbin/init rootwait ro
> +## Application terminated, r = 0
> +
> +The environment variable fdtcontroladdr points to U-Boot's interna

Re: [U-Boot] Fwd: [PATCH v2 2/2][for v2018.03] efi_loader: provide new doc/README.efi

[Leif Lindholm]

On Mon, Feb 26, 2018 at 07:37:15PM +0100, Heinrich Schuchardt wrote:
> On 02/26/2018 06:38 PM, Leif Lindholm wrote:
> > Apologies for formatting.
> > 
> > On Mon, Feb 26, 2018 at 04:01:39PM +0100, Alexander Graf wrote:
> > >  Forwarded Message 
> > > Subject: [PATCH v2 2/2][for v2018.03] efi_loader: provide new 
> > > doc/README.efi
> > > Date: Fri, 16 Feb 2018 12:44:27 +0100
> > > From: Heinrich Schuchardt <xypron.g...@gmx.de>
> > > To: Alexander Graf <ag...@suse.de>
> > > CC: Simon Glass <s...@chromium.org>, u-boot@lists.denx.de, Heinrich
> > > Schuchardt <xypron.g...@gmx.de>
> > > 
> > > Provides information about
> > > 
> > > - usage of the bootefi command
> > > - overview of UEFI
> > > - interaction between U-Boot and EFI drivers
> > 
> > So I'll mention it once and then forever hold my peace:
> > 
> > Any specification supporting other that IA-64, (32-bit) ix86
> > is UEFI (released 12 years ago).
> > The last release of _EFI_ was in 2002.
> > The support in Linux actually dates back to those days.
> 
> Hello Leif,
> 
> thanks for reviewing.
> 
> The UEFI spec itself is comfortable to use the term EFI again and again even
> for new items added in 2017, e.g.
> 
> - Add new data type to EFI Supplicant Protocol
> - Add EFI HTTP Boot Callback Protocol
> - EFI regular expression syntax type definitions
> - 4.7.4 EFI Driver Model Example

Yes, the existing definitions were never renamed, and new ones are
still defined to match the old pattern.

> 
> What is the change you suggest?

Ideally, to use UEFI wherever not referring specifically to an
interface described by the UEFI specification.
For clarity, I will nitpick below. Feel free to include or ignore.

> > 
> > > 
> > > Signed-off-by: Heinrich Schuchardt <xypron.g...@gmx.de>
> > > ---
> > >   doc/README.efi | 323
> > > +
> > >   1 file changed, 323 insertions(+)
> > > 
> > > diff --git a/doc/README.efi b/doc/README.efi
> > > index e69de29bb2d..92f7587a1af 100644
> > > --- a/doc/README.efi
> > > +++ b/doc/README.efi

README.uefi

> > > @@ -0,0 +1,323 @@
> > > +
> > > +
> > > +# EFI on U-Boot

"UEFI on U-Boot"

> > > +
> > > +The Unified Extensible Firmware Interface Specification (UEFI) [1] has
> > > become
> > > +the default for booting on AArch64 and x86 systems. It provides a
> > > stable API for
> > > +the interaction of drivers and applications with the firmware. The API
> > > comprises
> > > +access to block storage, network, and console to name a few. The Linux
> > > kernel and
> > > +boot loaders like Grub or the FreeBSD loader can be executed.
> > 
> > If you care about capitalisation, it should be GRUB.
> 
> Yes.
> 
> > 
> > > +
> > > +## Building for EFI

"Building for UEFI"

> > > +
> > > +The UEFI standard supports only little endian systems. The EFI support

"The UEFI support"

> > > can be
> > > +activated for ARM and x86 by specifying
> > > +
> > > +CONFIG_CMD_BOOTEFI=y
> > > +CONFIG_EFI_LOADER=y
> > > +
> > > +in the .config file.
> > > +
> > > +Support for attaching virtual block devices, e.g. iSCSI drives
> > > connected by the
> > > +loaded EFI application [3], requires

"loaded UEFI application"

> > > +
> > > +CONFIG_BLK=y
> > > +CONFIG_PARTITIONS=y
> > > +
> > > +### Executing an EFI binary
> > > +
> > > +The bootefi command is used to start EFI applications or to install EFI

"start UEFI applications or to install UEFI"

> > > drivers.
> > > +It takes two parameters
> > > +
> > > +bootefi  [fdt address]
> > > +
> > > +* image address - the memory address of the EFI binary

"the memory address of the UEFI binary"

> > > +* fdt address - the memory address of the flattened device tree
> > > +
> > > +Below you find the output of an example session starting Grub.
> > > +
> > > +=> load mmc 0:2 ${fdt_addr_r} boot/dtb
> > > +29830 bytes read in 14 ms (2 MiB/s)
> > > +=> load mmc 0:1 ${kernel_addr_r} efi/debian/grubaa64.efi
> > > +reading efi/debian/grubaa64.efi
> > > +120832 bytes read in 7 ms (16.5 MiB/s)
> > > +  

Re: [U-Boot] Fwd: [PATCH v2 2/2][for v2018.03] efi_loader: provide new doc/README.efi

[Leif Lindholm]

Apologies for formatting.

On Mon, Feb 26, 2018 at 04:01:39PM +0100, Alexander Graf wrote:
>  Forwarded Message 
> Subject: [PATCH v2 2/2][for v2018.03] efi_loader: provide new doc/README.efi
> Date: Fri, 16 Feb 2018 12:44:27 +0100
> From: Heinrich Schuchardt 
> To: Alexander Graf 
> CC: Simon Glass , u-boot@lists.denx.de, Heinrich
> Schuchardt 
> 
> Provides information about
> 
> - usage of the bootefi command
> - overview of UEFI
> - interaction between U-Boot and EFI drivers

So I'll mention it once and then forever hold my peace:

Any specification supporting other that IA-64, (32-bit) ix86
is UEFI (released 12 years ago).
The last release of _EFI_ was in 2002.
The support in Linux actually dates back to those days.

> 
> Signed-off-by: Heinrich Schuchardt 
> ---
>  doc/README.efi | 323
> +
>  1 file changed, 323 insertions(+)
> 
> diff --git a/doc/README.efi b/doc/README.efi
> index e69de29bb2d..92f7587a1af 100644
> --- a/doc/README.efi
> +++ b/doc/README.efi
> @@ -0,0 +1,323 @@
> +
> +
> +# EFI on U-Boot
> +
> +The Unified Extensible Firmware Interface Specification (UEFI) [1] has
> become
> +the default for booting on AArch64 and x86 systems. It provides a
> stable API for
> +the interaction of drivers and applications with the firmware. The API
> comprises
> +access to block storage, network, and console to name a few. The Linux
> kernel and
> +boot loaders like Grub or the FreeBSD loader can be executed.

If you care about capitalisation, it should be GRUB.

> +
> +## Building for EFI
> +
> +The UEFI standard supports only little endian systems. The EFI support
> can be
> +activated for ARM and x86 by specifying
> +
> +CONFIG_CMD_BOOTEFI=y
> +CONFIG_EFI_LOADER=y
> +
> +in the .config file.
> +
> +Support for attaching virtual block devices, e.g. iSCSI drives
> connected by the
> +loaded EFI application [3], requires
> +
> +CONFIG_BLK=y
> +CONFIG_PARTITIONS=y
> +
> +### Executing an EFI binary
> +
> +The bootefi command is used to start EFI applications or to install EFI
> drivers.
> +It takes two parameters
> +
> +bootefi  [fdt address]
> +
> +* image address - the memory address of the EFI binary
> +* fdt address - the memory address of the flattened device tree
> +
> +Below you find the output of an example session starting Grub.
> +
> +=> load mmc 0:2 ${fdt_addr_r} boot/dtb
> +29830 bytes read in 14 ms (2 MiB/s)
> +=> load mmc 0:1 ${kernel_addr_r} efi/debian/grubaa64.efi
> +reading efi/debian/grubaa64.efi
> +120832 bytes read in 7 ms (16.5 MiB/s)
> +=> bootefi ${kernel_addr_r} ${fdt_addr_r}
> +
> +The environment variable 'bootargs' is passed as load options in the
> EFI system
> +table. The Linux kernel EFI stub uses the load options as command line
> +arguments.
> +
> +### Executing the boot manager
> +
> +The UEFI specfication foresees to define boot entries and boot sequence
> via EFI
> +variables. Booting according to these variables is possible via
> +
> +bootefi bootmgr [fdt address]
> +
> +As of U-Boot v2018.03 EFI variables are not persisted and cannot be set at
> +runtime.

So SetVariable is not supported even non-persistently?
That would be useful to add to "open issues" section below.

> +
> +### Executing the built in hello world application
> +
> +A hello world EFI application can be built with
> +
> +CONFIG_CMD_BOOTEFI_HELLO_COMPILE=y
> +
> +It can be embedded into the U-Boot binary with
> +
> +CONFIG_CMD_BOOTEFI_HELLO=y
> +
> +The bootefi command is used to start the embedded hello world application.
> +
> +bootefi hello [fdt address]
> +
> +Below you find the output of an example session.
> +
> +=> bootefi hello ${fdtcontroladdr}
> +## Starting EFI application at 0100 ...
> +WARNING: using memory device/image path, this may confuse some
> payloads!
> +Hello, world!
> +Running on UEFI 2.7
> +Have SMBIOS table
> +Have device tree
> +Load options: root=/dev/sdb3 init=/sbin/init rootwait ro
> +## Application terminated, r = 0
> +
> +The environment variable fdtcontroladdr points to U-Boots internal
> device tree
> +(if available).
> +
> +### Executing the built-in selftest
> +
> +An EFI selftest suite can be embedded in U-Boot by building with
> +
> +CONFIG_SELFTEST=y
> +
> +For testing the EFI implementation the bootefi command can be used to
> start the
> +selftest.
> +
> +bootefi selftest [fdt address]
> +
> +The environment variable 'efi_selftest' can be used to select a single
> test. If
> +it is not provided all tests are executed except those marked as 'on
> request'.
> +If the environment variable is set to 'list' a list of all tests is shown.
> +
> +Below you find the output of an example session.
> +
> +=> setenv efi_selftest simple network protocol
> +=> bootefi selftest
> +Testing EFI 

Re: [U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Leif Lindholm]

On Wed, Aug 30, 2017 at 08:59:31AM +0100, Leif Lindholm wrote:
> On Wed, Aug 30, 2017 at 12:03:16AM +0200, Heinrich Schuchardt wrote:
> > I am able to provide a test application that will cover the API
> > functions that I have focused on (events, protocols, (dis)connect
> > controllers). To limit my effort I would like to write it for the HEAD
> > of my patch queue and not break it down into one test patch per
> > implementation patch. I hope that is ok for you. My effort estimate is
> > 40h+ so, please, be patient.
> 
> I am not going to claim that getting SCT to run under U-Boot is going
> to come near the top of my priority queue any time soon, and it would
> certainly be useful for some sort of "make sure we don't break what we
> have" tests.

Well, I spent a few hours looking at what the immediate issues may be
with running the UEFI Shell (built with -D NO_SHELL_PROFILES), same as
edk2/ShellBinPkg/MinUefiShell. Alex suggested I post an update.

I've implemented stubs for the missing protocols preventing the Shell
from starting properly, and pushed everything to
https://git.linaro.org/people/leif.lindholm/u-boot.git/log/?h=uefi-shell

(As Alex points out, I forgot about the EFI_EXIT/EFI_ENTER macros.)

With this, execution fails with
Found 2 disks
new_package_list: 99

ASSERT_EFI_ERROR (Status = Device Error)
ASSERT [Shell]
/work/git/edk2/Build/Shell/DEBUG_GCC5/AARCH64/ShellPkg/Application/Shell/Shell/DEBUG/AutoGen.c(615):
!EFI_ERROR (Status)
"Synchronous Abort" handler, esr 0x5600dbdb

The AutoGen.c failure is a library constructor
(ShellLevel2CommandsLibConstructor) returning error because
MdeModulePkg/Library/UefiHiiLib/HiiLib.c : HiiAddPackages() received
an error return from new_package_list (in the non-camel-case u-boot
form).

If you could look into implementing something useful in that function,
I could look into what causes the next stumbling block after that.

I did overrun the "maximum number of open protocols" limit, so
randomly bumped that up to 12. It will need to go higher (and
preferably dynamic). As long as we're not planning to support
protocols staying around at runtime, I'd say that shouldn't be too
hard to resolve properly.

I have pushed an update to the pre-built versions of UEFI Shell in edk2:
https://github.com/tianocore/edk2/tree/master/ShellBinPkg/MinUefiShell
These are built without ASSERTs, so won't actually give the above
messages, but should otherwise be identical to the one I've used here.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Leif Lindholm]

On Wed, Aug 30, 2017 at 12:03:16AM +0200, Heinrich Schuchardt wrote:
> On 08/29/2017 10:38 PM, Alexander Graf wrote:
> >> Am 29.08.2017 um 22:16 schrieb Simon Glass :
> >> That seems more useful long term than re-inventing comprehensive UEFI
> >> test suite.  (Also, beyond just running shim/fallback/grub, I don't
> >> really have time to invent new tests for the stack of efi_loader
> >> patches I have.)
> >
> > Yes and no - it depends on the availability of the official suite :/.
> 
>  UEFI SCT is not yet open source/free software. Obviously, this is
>  something Linaro has been lobbying for since day one of our
>  involvement. There used to be little understanding for this, but that
>  attitude has shifted substantially.
> >>>
> >>> So, if/until UEFI SCT is not an option, what about:
> >>>
> >>>  https://01.org/linux-uefi-validation
> >>>
> >>> (thx to pjones for pointing that out to me)
> >>
> >> Well in any case I'm not looking for a large functional test suite at
> >> this stage. It certainly could be useful, but not as a replacement for
> >> unit tests. The latter is for fast verification (so that everyone can
> >> run it as part of 'make tests') and easy identification of the
> >> location of bugs.
> > 
> > I fail to see the point here to be honest. The thing we care most
> > about in the uefi implementation is abi compatibility with the
> > spec. That's basically what SCT is supposed to get you. Running it
> > should be a matter of seconds with kvm enabled, so 'make tests'
> > should easily be able to run it if you have the correct target
> > compiled.
> > 
> >>
> >> These new tests should be written in C, run very quickly (similar to
> >> the existing tests) to verify that the code works. Then when each new
> >> feature is added, the test are expanded to cover the new
> >> functionality. Should the code be refactored, the tests should provide
> >> comfort that nothing broke.
> > 
> > So far I've never seen that approach work well in (low level) open
> > source projects, as you start to heavily raise the barrier for
> > participation. Most people in this world do these tasks in their
> > spare time and won't spend x2 the time to also write and update
> > unit tests.
> > 
> > I really think having a working functional test suite that is easy
> > to run is the best way forward in this case. We can then start
> > thinking of ways to test hard to reach code sections to increase
> > test coverage.
> > 
> > Alex
> 
> Simon is right in that test code is needed to ensure that refactoring
> does not break existing functionality. The current git HEAD cannot read
> from an IDE disk anymore. This problem could have been caught by a
> functional test four weeks ago.
> 
> Unit tests should be based on exposed APIs. Tests for lower level
> functions would not survive refactoring.

Completely agreed. UEFI is an interface, not an implementation.

> We implement only part of the UEFI spec. If we do not want to blow up
> binary size we may even want to keep it that way. So a test suite for
> full UEFI compatibility will always fail partially if it runs at all.

SCT is (almost?) entirely implemented in the form of:
- Does the implementation claim to support X?
- If so, determine if X behaves in conformance with the specification.

> I am able to provide a test application that will cover the API
> functions that I have focused on (events, protocols, (dis)connect
> controllers). To limit my effort I would like to write it for the HEAD
> of my patch queue and not break it down into one test patch per
> implementation patch. I hope that is ok for you. My effort estimate is
> 40h+ so, please, be patient.

I am not going to claim that getting SCT to run under U-Boot is going
to come near the top of my priority queue any time soon, and it would
certainly be useful for some sort of "make sure we don't break what we
have" tests.

But there will be additional bits of UEFI functionality that will be
useful to add. Some of that functionality will be generically useful,
some may not be relevant for certain systems and if the effect on code
size is non-negligable, its inclusion in the final image should be
made conditional.

And surely it would be preferable to be able to reuse an existing
testsuite when adding new bits, instead of having to implement new
tests for every feature? Especially when the goal is compatibility
with something that uses that existing test suite.

Alex got me up and running with a test environment, so I will put
"investigate why Shell.efi won't run" on my background task list.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Leif Lindholm]

On Tue, Aug 29, 2017 at 05:59:41PM +0200, Heinrich Schuchardt wrote:
> On 08/29/2017 02:57 PM, Leif Lindholm wrote:
> > UEFI SCT is not yet open source/free software. Obviously, this is
> > something Linaro has been lobbying for since day one of our
> > involvement. There used to be little understanding for this, but that
> > attitude has shifted substantially.
> > 
> > I will let Dong fill in on what the current status actually get the
> > code into the open is.
> > 
> According to the accompanying information UEFI SCT requires a working
> UEFI shell. This rules out this test suite.

Hey, I was only replying to:

> > On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
> >> The official test suite definitely needs the UEFI Shell. Is the suite
> >> publicly available by now?

But also, why does requiring a working UEFI Shell rule it out?
I mean, are there any reasons beyond UEFI Shell not currently being
functional under U-Boot?

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 00/23] efi_loader implement missing functions

[Leif Lindholm]

On Tue, Aug 29, 2017 at 02:26:48PM +0200, Alexander Graf wrote:
> > > > I would add command
> > > > bootefi selftest.efi
> > > > to run the tests and provide the python wrapper code to add it to the
> > > > test suite.
> > > 
> > > I think that's a great idea, yes.
> > I wonder how far we are from running UEFI tests (either the official
> > ones, or I seem to remember hearing about some other test suite which
> > didn't require UEFI shell)?
> 
> Let's ask Leif, Ard and Dong.
> 
> The official test suite definitely needs the UEFI Shell. Is the suite
> publicly available by now?

In binary form, you can access it already from the links on
http://uefi.org/testtools 

Yes, 2.5 is latest release. No this is not a restriction ... the SCT
releases have been lagging the specification releases a fair bit.

The 2.5a package contains AARCH64, IA32 and X64 support (not ARM).
Not that it couldn't contain ARM, it just hasn't been packaged.

> > That seems more useful long term than re-inventing comprehensive UEFI
> > test suite.  (Also, beyond just running shim/fallback/grub, I don't
> > really have time to invent new tests for the stack of efi_loader
> > patches I have.)
> 
> Yes and no - it depends on the availability of the official suite :/.

UEFI SCT is not yet open source/free software. Obviously, this is
something Linaro has been lobbying for since day one of our
involvement. There used to be little understanding for this, but that
attitude has shifted substantially.

I will let Dong fill in on what the current status actually get the
code into the open is.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v0 21/20] efi_loader: hack for archs that cannot do unaligned accesses

[Leif Lindholm]

On Tue, Aug 08, 2017 at 08:01:10AM -0400, Rob Clark wrote:
> On Tue, Aug 8, 2017 at 7:32 AM, Leif Lindholm <leif.lindh...@linaro.org> 
> wrote:
> > On Tue, Aug 08, 2017 at 09:11:14AM +0100, Ard Biesheuvel wrote:
> >> On 8 August 2017 at 07:52, Alexander Graf <ag...@suse.de> wrote:
> >> >> Am 07.08.2017 um 23:18 schrieb Rob Clark <robdcl...@gmail.com>:
> >> >>
> >> >> This is problematic around file nodes in the device path.  Adding the
> >> >> padding bytes to the end of each device-path struct would "solve"
> >> >> that, and if pre-aarch64 we are aiming at "good enough to work", I
> >> >> kinda think that this the approach we should go for.  Other than
> >> >> file-path nodes, the rest of the issues in u-boot should be solved by
> >> >> addition of missing __packed on 'struct efi_device_path' (which I've
> >> >> added locally and will be in the next revision of the patchset).
> >> >
> >> > Let's ask Leif and Ard if that is actually correct. If I remember
> >> > correctly, edk2 some times leverages automatic padding from the
> >> > compiler on structs.
> >>
> >> I guess that that might work, if U-boot is the only agent
> >> instantiating device path structures. But what do you mean by
> >> 'correct' in this context?
> >
> > Well, if that is the case, are we risking the ability to in the future
> > support loading drivers or protocols at runtime. (This would for
> > example exclude Shim compatibility or running the UEFI Shell.)
> 
> My proposal (and this is only for <=armv6 and armv7 until someone gets
> around to enabling mmu and disabling alignment faults) is to add
> padding bytes at the end of the various device-path structs to at
> least keep the structs (and things like utf16 string in file-path
> struct) aligned, and rely on efi payload and u-boot to be compiled
> with -mno-unaligned-access if it needs to access fields within the
> device-path structs.
> 
> This is *essentially* what u-boot does implicitly at the moment (by
> missing __packed attribute on certain structs).  I want to fix that on
> aarch64, but without the padding bytes it causes a some unaligned
> accesses in u-boot on armv7 devices.
>
> I think the goal for armv7 is more to have enough uefi for grub and
> OpenBSD's bootloader.  If fancy things like loading drivers and
> protocols at runtime doesn't work, well these didn't work before so I
> won't loose much sleep.  But I would like that to work properly on
> aarch64.

I'm all for the just enough approach (I just keep hoping for feature
creep). If this means certain aspects will not be supportable, what I
want is for it to be not supportable in a predictable manner.

So I guess what I'd like is that if we do this, then we either turn
the efi_*install_* functions back into just returning
EFI_OUT_OF_RESOURCES on these platforms, or worst case make them
scream bloody murder (but progress and hope for the best - like [1]).

[1] https://lists.denx.de/pipermail/u-boot/2016-January/241454.html

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v0 21/20] efi_loader: hack for archs that cannot do unaligned accesses

[Leif Lindholm]

On Tue, Aug 08, 2017 at 09:11:14AM +0100, Ard Biesheuvel wrote:
> On 8 August 2017 at 07:52, Alexander Graf  wrote:
> >
> >
> >> Am 07.08.2017 um 23:18 schrieb Rob Clark :
> >>
> >> On Mon, Aug 7, 2017 at 5:14 PM, Mark Kettenis  
> >> wrote:
>  From: Alexander Graf 
>  Date: Mon, 7 Aug 2017 21:19:37 +0100
> 
> > On 05.08.17 21:31, Rob Clark wrote:
> >> On Sat, Aug 5, 2017 at 4:05 PM, Heinrich Schuchardt 
> >>  wrote:
> >>> On 08/05/2017 08:43 PM, Rob Clark wrote:
>  On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark  
>  wrote:
> > On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt 
> >  wrote:
> >> On 08/05/2017 06:16 PM, Rob Clark wrote:
> >>> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt 
> >>>  wrote:
>  On 08/05/2017 05:58 PM, Rob Clark wrote:
>  Some arch's have trouble with unaligned accesses.  Technically
>  EFI device-path structs should be byte aligned, and the next node
>  in the path starts immediately after the previous.  Meaning that
>  a pointer to an 'struct efi_device_path' is not necessarily word
>  aligned.  See section 10.3.1 in v2.7 of UEFI spec.
> 
>  This causes problems not just for u-boot, but also most/all EFI
>  payloads loaded by u-boot on these archs.  Fortunately the common
>  practice for traversing a device path is to rely on the length
>  field in the header, rather than the specified length of the
>  particular device path type+subtype.  So the EFI_DP_PAD() macro
>  will add the specified number of bytes to the tail of device path
>  structs to pad them to word alignment.
> 
>  Technically this is non-compliant, BROKEN_UNALIGNED should *only*
>  be defined on archs that cannot do unaligned accesses.
> 
>  Signed-off-by: Rob Clark 
>  ---
>  I'm not sure if there are other arch's that need 
>  -DBROKEN_UNALIGNED
> 
>  Mark, this is untested but I think it should solve your crash on 
>  the
>  Banana Pi.  Could you give it a try when you get a chance?
> 
>   arch/arm/config.mk   |  2 +-
>   include/efi_api.h| 30 
>  ++
>   lib/efi_loader/efi_device_path.c |  3 +++
>   3 files changed, 34 insertions(+), 1 deletion(-)
> 
>  diff --git a/arch/arm/config.mk b/arch/arm/config.mk
>  index 1a9db4..067dc93a9d 100644
>  --- a/arch/arm/config.mk
>  +++ b/arch/arm/config.mk
>  @@ -28,7 +28,7 @@ LLVMS_RELFLAGS  := $(call 
>  cc-option,-mllvm,) \
>    $(call cc-option,-arm-use-movt=0,)
>   PLATFORM_RELFLAGS+= $(LLVM_RELFLAGS)
> 
>  -PLATFORM_CPPFLAGS += -D__ARM__
>  +PLATFORM_CPPFLAGS += -D__ARM__ -DBROKEN_UNALIGNED
> >>>
> >>> NAK
> >>>
> >>> We have more then ARM. And other architectures also create 
> >>> exceptions
> >>> for unaligned access.
> >>>
> >>> I hate platform specific code. It should not be used outside 
> >>> /arch.
> >>>
> >>> To play it save you should not use _packed at all!
> >>> Use memcpy to transfer between aligned and unaligned memory.
> >>
> >> except for reasons I explained in the thread on the patch that 
> >> added
> >> the __packed in the first place.  Sorry, this is ugly but we have 
> >> to
> >> do it.
> >>
> >> BR,
> >> -R
> >
> > According to the UEFI standard the nodes in paths are not to be 
> > assumed
> > to be aligned.
> >
> > So even if you use padding bytes in paths that you pass to the EFI
> > application you should not expect that the EFI application does the
> > same. Expect the EFI application either to remove them or send new 
> > nodes
> > without padding.
> >
> > To the idea of padding bytes and __packed does not make sense.
> 
>  Ok, to be fair, you are right about device-paths passed too u-boot.
>  On BROKEN_UNALIGNED archs, we should sanitise with a copy to an
>  aligned device-path in *addition* to what I proposed.  I can make a
>  patch to add a helper to do this a bit later.
> >>>
> >>> so thinking about this a bit, I have two options in mind:
> >>>
> 

Re: [U-Boot] [PATCH 3/6] arm: efi: Add a hello world test program

[Leif Lindholm]

On 19 October 2016 at 21:22, Leif Lindholm <leif.lindh...@linaro.org> wrote:
> On Mon, Oct 17, 2016 at 07:55:02PM -0600, Simon Glass wrote:
>> Hi Leif,
>>
>> On 26 September 2016 at 19:53, Leif Lindholm <leif.lindh...@linaro.org> 
>> wrote:
>> >> Thanks for the pointer. Unfortunately that patch appears to make no
>> >> differences for me. Are you able to build and send me a 64-bit
>> >> HelloWorld.efi please?
>> >
>> > So, I probably could, but if that isn't working for you, I'd quite
>> > like to know why.
>> >
>> > To make that a little less painful though, I've added support for
>> > building the helloworld app to my set of scripts:
>> > https://git.linaro.org/uefi/uefi-tools.git
>> >
>> > This still depends on this (updated) patch.
>> > https://lists.01.org/pipermail/edk2-devel/2016-September/002112.html
>> >
>> > But with a current edk2, and a uefi-tools placed in the same directory
>> > as the edk2 clone, could you try executing:
>> >
>> > ../uefi-tools/uefi-build.sh -A AARCH64 hello
>> >
>> > If the build fails and creates messy output due to being parallel,
>> > could you stick a -1 on that command line and send me the output (or
>> > pastebin)?
>>
>> OK thanks. Please see:
>>
>> http://pastebin.com/DmixdA4C
>
> Right ... so your terminal appears to be discarding stderr, but I'm
> guessing with that enabled it would look like:
> ---
> Processing meta-data ..
>
> build.py...
> /work/git/edk2/MdeModulePkg/MdeModulePkg.dsc(...): error 4000: Instance of 
> library class [ArmMmuLib] is not found
> in [/work/git/edk2/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf] [AARCH64]
> consumed by module 
> [/work/git/edk2/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf]
>
>
> - Failed -
> Build end time: 16:51:18, Oct.19 2016
> Build total time: 00:00:03
> ---
>
> Which is what it looks without the aforementioned patch applied.

As a follow-up: I have now pushed the required patch to edk2, so with
a fresh pull it should work without any manually added patches.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 3/6] arm: efi: Add a hello world test program

[Leif Lindholm]

On Mon, Oct 17, 2016 at 07:55:02PM -0600, Simon Glass wrote:
> Hi Leif,
> 
> On 26 September 2016 at 19:53, Leif Lindholm <leif.lindh...@linaro.org> wrote:
> >> Thanks for the pointer. Unfortunately that patch appears to make no
> >> differences for me. Are you able to build and send me a 64-bit
> >> HelloWorld.efi please?
> >
> > So, I probably could, but if that isn't working for you, I'd quite
> > like to know why.
> >
> > To make that a little less painful though, I've added support for
> > building the helloworld app to my set of scripts:
> > https://git.linaro.org/uefi/uefi-tools.git
> >
> > This still depends on this (updated) patch.
> > https://lists.01.org/pipermail/edk2-devel/2016-September/002112.html
> >
> > But with a current edk2, and a uefi-tools placed in the same directory
> > as the edk2 clone, could you try executing:
> >
> > ../uefi-tools/uefi-build.sh -A AARCH64 hello
> >
> > If the build fails and creates messy output due to being parallel,
> > could you stick a -1 on that command line and send me the output (or
> > pastebin)?
> 
> OK thanks. Please see:
> 
> http://pastebin.com/DmixdA4C

Right ... so your terminal appears to be discarding stderr, but I'm
guessing with that enabled it would look like:
---
Processing meta-data ..

build.py...
/work/git/edk2/MdeModulePkg/MdeModulePkg.dsc(...): error 4000: Instance of 
library class [ArmMmuLib] is not found
in [/work/git/edk2/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf] [AARCH64]
consumed by module 
[/work/git/edk2/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf]


- Failed -
Build end time: 16:51:18, Oct.19 2016
Build total time: 00:00:03
---

Which is what it looks without the aforementioned patch applied.

(But the no error messages thing really spooks me.)

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 3/6] arm: efi: Add a hello world test program

[Leif Lindholm]

On Mon, Sep 26, 2016 at 08:09:40AM -0600, Simon Glass wrote:
> >> >> If so, can we just remove this for arm64?
> >> >
> >> > Actually I was hoping that Alexander might have a suitable arm64
> >> > HelloWorld.efi lying around. When I tried building UEFI for arm64, for
> >> > some reason it did not create it.
> >>
> >> Is it part of edk2? If so, Leif (CC'ed) might have one :). I usually
> >> use grub as my hello world application.
> >
> > There is a hello world application in EDK2, but it does not get built
> > as part of a normal platform build.
> > Currently it also fails to build for ARM* on its own :|
> > See
> > http://patchew.org/EDK2/1471021908-3509-1-git-send-email-leif.lindholm%40linaro.org/
> > for a hack of how to build one before upstream is resolved.
> >
> > If cross compiling, prepend GCC5_AARCH64_PREFIX=aarch64-linux-gnu- to
> > build command line.
> 
> Thanks for the pointer. Unfortunately that patch appears to make no
> differences for me. Are you able to build and send me a 64-bit
> HelloWorld.efi please?

So, I probably could, but if that isn't working for you, I'd quite
like to know why.

To make that a little less painful though, I've added support for
building the helloworld app to my set of scripts:
https://git.linaro.org/uefi/uefi-tools.git

This still depends on this (updated) patch.
https://lists.01.org/pipermail/edk2-devel/2016-September/002112.html

But with a current edk2, and a uefi-tools placed in the same directory
as the edk2 clone, could you try executing:

../uefi-tools/uefi-build.sh -A AARCH64 hello

If the build fails and creates messy output due to being parallel,
could you stick a -1 on that command line and send me the output (or
pastebin)?

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 3/6] arm: efi: Add a hello world test program

[Leif Lindholm]

On Tue, Aug 09, 2016 at 10:55:31PM +0200, Alexander Graf wrote:
> 
> 
> > Am 09.08.2016 um 20:16 schrieb Simon Glass :
> > 
> > Hi Bin,
> > 
> >> On 9 August 2016 at 00:50, Bin Meng  wrote:
> >> Hi Simon,
> >> 
> >>> On Sun, Aug 7, 2016 at 7:23 AM, Simon Glass  wrote:
> >>> It is useful to have a basic sanity check for EFI loader support. Add a
> >>> 'bootefi hello' command which loads HelloWord.efi and runs it under 
> >>> U-Boot.
> >>> 
> >>> Signed-off-by: Simon Glass 
> >>> ---
> >>> 
> >>> arch/arm/lib/HelloWorld32.efi  | Bin 0 -> 11712 bytes
> >>> arch/arm/lib/Makefile  |   6 ++
> >>> cmd/Kconfig|  10 ++
> >>> cmd/bootefi.c  |  26 --
> >>> include/asm-generic/sections.h |   2 ++
> >>> scripts/Makefile.lib   |  19 +++
> >>> 6 files changed, 57 insertions(+), 6 deletions(-)
> >>> create mode 100644 arch/arm/lib/HelloWorld32.efi
> >> 
> >> [snip]
> >> 
> >>> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
> >>> index a8d1557..0f3ea0c 100644
> >>> --- a/arch/arm/lib/Makefile
> >>> +++ b/arch/arm/lib/Makefile
> >>> @@ -29,6 +29,12 @@ obj-$(CONFIG_OF_LIBFDT) += bootm-fdt.o
> >>> obj-$(CONFIG_CMD_BOOTM) += bootm.o
> >>> obj-$(CONFIG_CMD_BOOTM) += zimage.o
> >>> obj-$(CONFIG_SYS_L2_PL310) += cache-pl310.o
> >>> +ifdef CONFIG_ARM64
> >>> +# This option does not work for arm64, as there is no binary.
> >> 
> >> If so, can we just remove this for arm64?
> > 
> > Actually I was hoping that Alexander might have a suitable arm64
> > HelloWorld.efi lying around. When I tried building UEFI for arm64, for
> > some reason it did not create it.
> 
> Is it part of edk2? If so, Leif (CC'ed) might have one :). I usually
> use grub as my hello world application.

There is a hello world application in EDK2, but it does not get built
as part of a normal platform build.
Currently it also fails to build for ARM* on its own :|
See
http://patchew.org/EDK2/1471021908-3509-1-git-send-email-leif.lindholm%40linaro.org/
for a hack of how to build one before upstream is resolved.

If cross compiling, prepend GCC5_AARCH64_PREFIX=aarch64-linux-gnu- to
build command line.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 3/4] net: Move the VCI and client arch values to Kconfig

[Leif Lindholm]

On Fri, May 06, 2016 at 10:54:23AM -0400, Tom Rini wrote:
> On Wed, May 04, 2016 at 07:10:52PM +0200, Alexander Graf wrote:
> 
> > We have a bunch of boards that define their vendor class identifier and
> > client archs in the board files or in the distro config. Move everything
> > to the generic Kconfig options.
> 
> This part is good and fine, thanks.  Can you please do a follow-up to
> convert CONFIG_SPL_NET_VCI_STRING as well?
> 
> > We're missing the distinction between i386 and x86_64, as I couldn't find
> > any config variable that would tell us the difference. Is that really 
> > important
> > to people? I guess not, so I left it out.
> 
> This part however is is from the RFC (and ARM isn't in it, so 0x100 is
> special magic).  See https://tools.ietf.org/html/rfc4578 section 2.1.  I
> suppose reading that now, the right value to use here for your use case
> actually makes what we're doing now a little harder as "Intel x86PC" is
> 0x0, and "EFI IA32" is 0x6.  Or we could, if I'm reading the RFC right,
> just drop this part of the code entirely.

As far as I can tell (and referenced from EDK2 codebase), the
canonical list is
http://www.ietf.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xml#processor-architecture

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] efi_loader: Reserve 2 additional pages for fdt

[Leif Lindholm]

On Fri, Mar 04, 2016 at 10:19:10AM +0100, Alexander Graf wrote:
> We mark the device tree as reserved today, spanning exactly the amount
> of space it needs. If some other piece of code (like grub2) comes in and
> wants to modify the device tree to for example add a kernel command line
> though, it might assume that it has some space to do so.
> 
> So let's just reserve 2 additional pages for the device tree to play nicely.

I presume this was triggered by something you found on AArch32 grub,
booting a kernel without EFI stub?
If so, the issue will go away once I fix the AArch32 grub to use the
AArch64 loader for kernel images with the EFI stub. Could you possibly
send an email to grub-devel, or raise a ticket directly on the
savannah bugtracker if you have an account?
This fix really should go into 2.02 release.

Regardless, this seems like it provides a potential optimisation
opportunity for such agents, to use the tree in place instead of
always allocating more memory.

> Signed-off-by: Alexander Graf 
> ---
>  cmd/bootefi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index de17e49..faa6978 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -119,6 +119,8 @@ static unsigned long do_bootefi_exec(void *efi)
>   fdt_end = ((ulong)working_fdt) + fdt_totalsize(working_fdt);
>   fdt_size = (fdt_end - fdt_start) + EFI_PAGE_MASK;
>   fdt_pages = fdt_size >> EFI_PAGE_SHIFT;
> + /* Give a bootloader the chance to modify the device tree */
> + fdt_pages += 2;
>   efi_add_memory_map(fdt_start, fdt_pages,
>  EFI_BOOT_SERVICES_DATA, true);
>  
> -- 
> 1.8.5.6
> 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 14/16] efi_loader: Add distro boot script for removable media

[Leif Lindholm]

On Tue, Feb 02, 2016 at 03:45:12AM +0100, Alexander Graf wrote:
> UEFI defines a simple boot protocol for removable media. There we should look
> at the EFI (first GPT FAT) partition and search for /efi/boot/bootXXX.efi with
> XXX being different between different platforms (x86, x64, arm, aa64, ...).
> 
> This patch implements a simple version of that protocol for the default distro
> boot script. With this we can automatically boot from valid UEFI enabled
> removable media.
> 
> Because from all I could see U-Boot by default doesn't deliver device tree
> blobs with its firmware, we also need to load the dtb from somewhere. Traverse
> the same EFI partition for an fdt file that fits our current board so that
> an OS receives a valid device tree when booted automatically.
> 
> Signed-off-by: Alexander Graf 
> Reviewed-by: Simon Glass 
> ---
>  include/config_distro_bootcmd.h | 47 
> -
>  1 file changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> index 37c6b43..c19f1b0 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -90,6 +90,48 @@
>   BOOT_TARGET_DEVICES_references_UBIFS_without_CONFIG_CMD_UBIFS
>  #endif
>  
> +#ifdef CONFIG_EFI_LOADER
> +#if defined(CONFIG_ARM64)
> +#define BOOTEFI_NAME "bootaa64.efi"
> +#elif defined(CONFIG_ARM)
> +#define BOOTEFI_NAME "bootarm.efi"
> +#endif
> +#endif

Actually, since Simon is testing this series also on Minnowboard
(at least MAX), maybe add bootx64.efi and/or bootia32.efi as well?

I presume U-Boot for ia64 is not a thing? ;)

> +
> +#ifdef BOOTEFI_NAME
> +#define BOOTENV_SHARED_EFI\
> + "boot_efi_binary="\
> + "load ${devtype} ${devnum}:${distro_bootpart} "   \
> + "${kernel_addr_r} efi/boot/"BOOTEFI_NAME"; "  \
> + "bootefi ${kernel_addr_r}\0"  \
> + \
> + "load_efi_dtb="   \
> + "load ${devtype} ${devnum}:${distro_bootpart} "   \
> + "${fdt_addr_r} ${prefix}${fdt_name}; "   \
> + "fdt addr ${fdt_addr_r}\0"\
> + \
> + "efi_dtb_prefixes=/ /dtb/ /dtb/current/\0"\
> + "scan_dev_for_efi="   \
> + "for prefix in ${efi_dtb_prefixes}; do "  \
> + "if test -e ${devtype} "  \
> + "${devnum}:${distro_bootpart} "   \
> + "${prefix}${fdt_name}; then " \
> + "run load_efi_dtb; "  \
> + "fi;" \
> + "done;"   \
> + "if test -e ${devtype} ${devnum}:${distro_bootpart} " \
> + "efi/boot/"BOOTEFI_NAME"; then "  \
> + "echo Found EFI removable media binary "  \
> + "efi/boot/"BOOTEFI_NAME"; "   \
> + "run boot_efi_binary; "   \
> + "echo EFI LOAD FAILED: continuing...; "   \
> + "fi; "
> +#define SCAN_DEV_FOR_EFI "run scan_dev_for_efi;"
> +#else
> +#define BOOTENV_SHARED_EFI
> +#define SCAN_DEV_FOR_EFI
> +#endif
> +
>  #ifdef CONFIG_CMD_SATA
>  #define BOOTENV_SHARED_SATA  BOOTENV_SHARED_BLKDEV(sata)
>  #define BOOTENV_DEV_SATA BOOTENV_DEV_BLKDEV
> @@ -217,6 +259,7 @@
>   BOOTENV_SHARED_SCSI \
>   BOOTENV_SHARED_IDE \
>   BOOTENV_SHARED_UBIFS \
> + BOOTENV_SHARED_EFI \
>   "boot_prefixes=/ /boot/\0" \
>   "boot_scripts=boot.scr.uimg boot.scr\0" \
>   "boot_script_dhcp=boot.scr.uimg\0" \
> @@ -258,7 +301,9 @@
>   "for prefix in ${boot_prefixes}; do " \
>   "run scan_dev_for_extlinux; " \
>   "run scan_dev_for_scripts; "  \
> - "done\0"  \
> + "done;"   \
> + SCAN_DEV_FOR_EFI  \
> + "\0"  \
>   \
>   "scan_dev_for_boot_part=" \
>   "part list ${devtype} ${devnum} -bootable devplist; " \
> -- 
> 2.6.2
> 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 04/16] efi_loader: Add boot time services

[Leif Lindholm]

On Tue, Feb 02, 2016 at 03:45:02AM +0100, Alexander Graf wrote:
> When an EFI application runs, it has access to a few descriptor and callback
> tables to instruct the EFI compliant firmware to do things for it. The bulk
> of those interfaces are "boot time services". They handle all object 
> management,
> and memory allocation.
> 
> This patch adds support for the boot time services and also exposes a system
> table, which is the point of entry descriptor table for EFI payloads.
> 
> Signed-off-by: Alexander Graf 
> Reviewed-by: Simon Glass 
> 
> ---
> 
> v1 -> v2:
> 
>   - Fix typo s/does now/does not/
>   - Add #ifdefs around header to allow inclusion when efi_loader is disabled
>   - Add stub efi_restore_gd() function when efi_loader is disabled
>   - Disable debug
>   - Mark runtime region as such
>   - Fix up memory map
>   - Allow efi_restore_gd to be called before first efi entry
>   - Add 32bit arm cache workaround
>   - Move memory map to separate patch
>   - Change BTS version to 2.5
>   - Fix return values for a few callbacks to more EFI compliant ones
>   - Change vendor to "Das U-Boot"
>   - Add warning when truncating timer trigger
>   - Move to GPLv2+
> 
> v2 -> v3:
> 
>   - Use external efi_memory helpers
>   - Add EFIAPI to function prototypes
>   - Initialize event timer to -1ULL to prevent early firing
>   - Document header
>   - Move obj list to lib
>   - Remove implicit guid table
>   - Add guid compare function
>   - Fix return values
>   - Implement efi_wait_for_event
>   - Implement efi_install_configuration_table
> ---
>  include/efi_loader.h  |  84 +
>  lib/efi_loader/efi_boottime.c | 781 
> ++
>  2 files changed, 865 insertions(+)
>  create mode 100644 lib/efi_loader/efi_boottime.c
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 5618185..a7f033e 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -6,15 +6,99 @@
>   *  SPDX-License-Identifier: GPL-2.0+
>   */
>  
> +#include 
>  #include 
>  #include 
> +
> +#ifdef CONFIG_EFI_LOADER
> +
>  #include 
>  
> +/* #define DEBUG_EFI */
> +
> +#ifdef DEBUG_EFI
> +#define EFI_ENTRY(format, ...) do { \
> + efi_restore_gd(); \
> + printf("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
> + } while(0)
> +#else
> +#define EFI_ENTRY(format, ...) do { \
> + efi_restore_gd(); \
> + } while(0)
> +#endif
> +
> +#define EFI_EXIT(ret) efi_exit_func(ret);
> +
> +extern struct efi_system_table systab;
> +
>  extern const efi_guid_t efi_guid_device_path;
>  extern const efi_guid_t efi_guid_loaded_image;
>  
> +/*
> + * While UEFI objects can have callbacks, you can also call functions on
> + * protocols (classes) themselves. This struct maps a protocol GUID to its
> + * interface (usually a struct with callback functions).
> + */
> +struct efi_class_map {
> + const efi_guid_t *guid;
> + const void *interface;
> +};
> +
> +/*
> + * When the UEFI payload wants to open a protocol on an object to get its
> + * interface (usually a struct with callback functions), this struct maps the
> + * protocol GUID to the respective protocol handler open function for that
> + * object protocol combination.
> + */
> +struct efi_handler {
> + const efi_guid_t *guid;
> + efi_status_t (EFIAPI *open)(void *handle,
> + efi_guid_t *protocol, void **protocol_interface,
> + void *agent_handle, void *controller_handle,
> + uint32_t attributes);
> +};
> +
> +/*
> + * UEFI has a poor man's OO model where one "object" can be polimorphic and 
> have

Polymorphic.
Also, somewhat long lines in this comment block?

> + * multiple different protocols (classes) attached to it.
> + *
> + * This struct is the parent struct for all of our actual implementation 
> objects
> + * that can include it to make themselves an EFI object
> + */
> +struct efi_object {
> + /* Every UEFI object is part of a global object list */
> + struct list_head link;
> + /* We support up to 4 "protocols" an object can be accessed through */
> + struct efi_handler protocols[4];
> + /* The object spawner can either use this for data or as identifier */
> + void *handle;
> +};
> +
> +/* This list contains all UEFI objects we know of */
> +extern struct list_head efi_obj_list;
> +
> +/*
> + * Stub implementation for a protocol opener that just returns the handle as
> + * interface
> + */
>  efi_status_t efi_return_handle(void *handle,
>   efi_guid_t *protocol, void **protocol_interface,
>   void *agent_handle, void *controller_handle,
>   uint32_t attributes);
> +/* Called from places to check whether a timer expired */
> +void efi_timer_check(void);
> +/* PE loader implementation */
>  void *efi_load_pe(void *efi, struct efi_loaded_image *loaded_image_info);
> +/* Called once to store the pristine gd pointer */
> +void 

Re: [U-Boot] [PATCH 09/16] efi_loader: Implement memory allocation and map

[Leif Lindholm]

On Tue, Feb 02, 2016 at 03:45:07AM +0100, Alexander Graf wrote:
> The EFI loader needs to maintain views of memory - general system memory
> windows as well as used locations inside those and potential runtime service
> MMIO windows.
> 
> To manage all of these, add a few helpers that maintain an internal
> representation of the map the similar to how the EFI API later on reports
> it to the application.
> 
> For allocations, the scheme is very simple. We basically allow allocations
> to replace chunks of previously done maps, so that a new LOADER_DATA
> allocation for example can remove a piece of the RAM map. When no specific
> address is given, we just take the highest possible address in the lowest
> RAM map that fits the allocation size.
> 
> Signed-off-by: Alexander Graf 
> 
> ---
> 
> v2 -> v3:
> 
>   - Rewrite memory allocation and map
>   - Document header
> ---
>  common/board_r.c|   3 +
>  include/efi_loader.h|  19 +++
>  lib/efi_loader/efi_memory.c | 314 
> 
>  3 files changed, 336 insertions(+)
>  create mode 100644 lib/efi_loader/efi_memory.c
> 
> diff --git a/common/board_r.c b/common/board_r.c
> index 420e2c8..2ed8e4b 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -785,6 +785,9 @@ init_fnc_t init_sequence_r[] = {
>  #ifdef CONFIG_CLOCKS
>   set_cpu_clk_info, /* Setup clock information */
>  #endif
> +#ifdef CONFIG_EFI_LOADER
> + efi_memory_init,
> +#endif
>   stdio_init_tables,
>   initr_serial,
>   initr_announce,
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 91ab6cb..e38be12 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -109,6 +109,25 @@ efi_status_t efi_exit_func(efi_status_t ret);
>  /* Call this to relocate the runtime section to an address space */
>  void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map);
>  
> +/* Generic EFI memory allocator, call this to get memory */
> +void *efi_alloc(uint64_t len, int memory_type);
> +/* More specific EFI memory allocator, called by EFI payloads */
> +efi_status_t efi_allocate_pages(int type, int memory_type, unsigned long 
> pages,
> + uint64_t *memory);
> +/* EFI memory free function. Not implemented today */
> +efi_status_t efi_free_pages(uint64_t memory, unsigned long pages);
> +/* Returns the EFI memory map */
> +efi_status_t efi_get_memory_map(unsigned long *memory_map_size,
> + struct efi_mem_desc *memory_map,
> + unsigned long *map_key,
> + unsigned long *descriptor_size,
> + uint32_t *descriptor_version);
> +/* Adds a range into the EFI memory map */
> +uint64_t efi_add_memory_map(uint64_t start, uint64_t pages, int memory_type,
> + bool overlap_only_ram);
> +/* Called by board init to initialize the EFI memory map */
> +int efi_memory_init(void);
> +
>  /*
>   * Use these to indicate that your code / data should go into the EFI runtime
>   * section and thus still be available when the OS is running
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> new file mode 100644
> index 000..e58d190
> --- /dev/null
> +++ b/lib/efi_loader/efi_memory.c
> @@ -0,0 +1,314 @@
> +/*
> + *  EFI application memory management
> + *
> + *  Copyright (c) 2016 Alexander Graf
> + *
> + *  SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +/* #define DEBUG_EFI */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +struct efi_mem_list {
> + struct list_head link;
> + struct efi_mem_desc desc;
> +};
> +
> +/* This list contains all memory map items */
> +LIST_HEAD(efi_mem);
> +
> +/*
> + * Unmaps all memory occupied by the carve_desc region from the
> + * list entry pointed to by map.
> + *
> + * Returns 1 if carving was performed or 0 if the regions don't overlap.
> + * Returns -1 if it would affect non-RAM regions but overlap_only_ram is set.
> + * Carving is only guaranteed to complete when all regions return 0.
> + */
> +static int efi_mem_carve_out(struct efi_mem_list *map,
> +  struct efi_mem_desc *carve_desc,
> +  bool overlap_only_ram)
> +{
> + struct efi_mem_list *newmap;
> + struct efi_mem_desc *map_desc = >desc;
> + uint64_t map_start = map_desc->physical_start;
> + uint64_t map_end = map_start + (map_desc->num_pages << 12);

Can we replace the magic 12 with an EFI_PAGE_SHIFT define?

> + uint64_t carve_start = carve_desc->physical_start;
> + uint64_t carve_end = carve_start + (carve_desc->num_pages << 12);
> +
> + /* check whether we're overlapping */
> + if ((carve_end <= map_start) || (carve_start >= map_end))
> + return 0;
> +
> + /* We're overlapping with non-RAM, warn the caller if desired */
> + if 

Re: [U-Boot] [PATCH 14/16] efi_loader: Add distro boot script for removable media

[Leif Lindholm]

On Tue, Feb 02, 2016 at 03:45:12AM +0100, Alexander Graf wrote:
> UEFI defines a simple boot protocol for removable media. There we should look
> at the EFI (first GPT FAT) partition and search for /efi/boot/bootXXX.efi with
> XXX being different between different platforms (x86, x64, arm, aa64, ...).

One comment, one question.
Comment:
It's not really "the first GPT FAT" - it's the partition marked with
the EFI System Partition type (containing a FAT filesystem).
This is actually defined for MBR partition tables too, but I'd be
quite happy for that bit of legacy to be left out.

Question:
Does U-Boot support El Torito for iso images?

> 
> This patch implements a simple version of that protocol for the default distro
> boot script. With this we can automatically boot from valid UEFI enabled
> removable media.
> 
> Because from all I could see U-Boot by default doesn't deliver device tree
> blobs with its firmware, we also need to load the dtb from somewhere. Traverse
> the same EFI partition for an fdt file that fits our current board so that
> an OS receives a valid device tree when booted automatically.
> 
> Signed-off-by: Alexander Graf 
> Reviewed-by: Simon Glass 
> ---
>  include/config_distro_bootcmd.h | 47 
> -
>  1 file changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> index 37c6b43..c19f1b0 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -90,6 +90,48 @@
>   BOOT_TARGET_DEVICES_references_UBIFS_without_CONFIG_CMD_UBIFS
>  #endif
>  
> +#ifdef CONFIG_EFI_LOADER
> +#if defined(CONFIG_ARM64)
> +#define BOOTEFI_NAME "bootaa64.efi"
> +#elif defined(CONFIG_ARM)
> +#define BOOTEFI_NAME "bootarm.efi"
> +#endif
> +#endif
> +
> +#ifdef BOOTEFI_NAME
> +#define BOOTENV_SHARED_EFI\
> + "boot_efi_binary="\
> + "load ${devtype} ${devnum}:${distro_bootpart} "   \
> + "${kernel_addr_r} efi/boot/"BOOTEFI_NAME"; "  \
> + "bootefi ${kernel_addr_r}\0"  \
> + \
> + "load_efi_dtb="   \
> + "load ${devtype} ${devnum}:${distro_bootpart} "   \
> + "${fdt_addr_r} ${prefix}${fdt_name}; "   \
> + "fdt addr ${fdt_addr_r}\0"\
> + \
> + "efi_dtb_prefixes=/ /dtb/ /dtb/current/\0"\
> + "scan_dev_for_efi="   \
> + "for prefix in ${efi_dtb_prefixes}; do "  \
> + "if test -e ${devtype} "  \
> + "${devnum}:${distro_bootpart} "   \
> + "${prefix}${fdt_name}; then " \
> + "run load_efi_dtb; "  \
> + "fi;" \
> + "done;"   \
> + "if test -e ${devtype} ${devnum}:${distro_bootpart} " \
> + "efi/boot/"BOOTEFI_NAME"; then "  \
> + "echo Found EFI removable media binary "  \
> + "efi/boot/"BOOTEFI_NAME"; "   \
> + "run boot_efi_binary; "   \
> + "echo EFI LOAD FAILED: continuing...; "   \
> + "fi; "
> +#define SCAN_DEV_FOR_EFI "run scan_dev_for_efi;"
> +#else
> +#define BOOTENV_SHARED_EFI
> +#define SCAN_DEV_FOR_EFI
> +#endif
> +
>  #ifdef CONFIG_CMD_SATA
>  #define BOOTENV_SHARED_SATA  BOOTENV_SHARED_BLKDEV(sata)
>  #define BOOTENV_DEV_SATA BOOTENV_DEV_BLKDEV
> @@ -217,6 +259,7 @@
>   BOOTENV_SHARED_SCSI \
>   BOOTENV_SHARED_IDE \
>   BOOTENV_SHARED_UBIFS \
> + BOOTENV_SHARED_EFI \
>   "boot_prefixes=/ /boot/\0" \
>   "boot_scripts=boot.scr.uimg boot.scr\0" \
>   "boot_script_dhcp=boot.scr.uimg\0" \
> @@ -258,7 +301,9 @@
>   "for prefix in ${boot_prefixes}; do " \
>   "run scan_dev_for_extlinux; " \
>   "run scan_dev_for_scripts; "  \
> - "done\0"  \
> + "done;"   \
> + SCAN_DEV_FOR_EFI  \
> + "\0"  \
>   \
>   "scan_dev_for_boot_part=" \
>   "part list ${devtype} ${devnum} -bootable devplist; "  

Re: [U-Boot] [PATCH 03/16] efi_loader: Add PE image loader

[Leif Lindholm]

On Tue, Feb 02, 2016 at 03:45:01AM +0100, Alexander Graf wrote:
> EFI uses the PE binary format for its application images. Add support to EFI 
> PE
> binaries as well as all necessary bits for the "EFI image loader" interfaces.
> 
> Signed-off-by: Alexander Graf 
> 
> ---
> 
> v1 -> v2:
> 
>   - move memory allocation to separate patch
>   - limit 32/64 to hosts that support it
>   - check 32bit optional nt header magic
>   - switch to GPL2+
> 
> v2 -> v3:
> 
>   - use efi_alloc
>   - add EFIAPI to function prototypes
>   - remove unused macros
>   - reorder header inclusion
>   - split relocation code into function
>   - flush cache after loading
> ---
>  include/efi_loader.h  |  20 +++
>  include/pe.h  | 263 
> ++
>  lib/efi_loader/efi_image_loader.c | 182 ++
>  3 files changed, 465 insertions(+)
>  create mode 100644 include/efi_loader.h
>  create mode 100644 include/pe.h
>  create mode 100644 lib/efi_loader/efi_image_loader.c
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> new file mode 100644
> index 000..5618185
> --- /dev/null
> +++ b/include/efi_loader.h
> @@ -0,0 +1,20 @@
> +/*
> + *  EFI application loader
> + *
> + *  Copyright (c) 2016 Alexander Graf
> + *
> + *  SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +extern const efi_guid_t efi_guid_device_path;
> +extern const efi_guid_t efi_guid_loaded_image;
> +
> +efi_status_t efi_return_handle(void *handle,
> + efi_guid_t *protocol, void **protocol_interface,
> + void *agent_handle, void *controller_handle,
> + uint32_t attributes);
> +void *efi_load_pe(void *efi, struct efi_loaded_image *loaded_image_info);
> diff --git a/include/pe.h b/include/pe.h
> new file mode 100644
> index 000..6379ae1
> --- /dev/null
> +++ b/include/pe.h
> @@ -0,0 +1,263 @@
> +/*
> + *  Portable Executable binary format structures
> + *
> + *  Copyright (c) 2016 Alexander Graf
> + *
> + *  Based on wine code
> + *
> + *  SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#ifndef _PE_H
> +#define _PE_H
> +
> +typedef struct _IMAGE_DOS_HEADER {
> + uint16_t e_magic;  /* 00: MZ Header signature */
> + uint16_t e_cblp;   /* 02: Bytes on last page of file */
> + uint16_t e_cp; /* 04: Pages in file */
> + uint16_t e_crlc;   /* 06: Relocations */
> + uint16_t e_cparhdr;/* 08: Size of header in paragraphs */
> + uint16_t e_minalloc;   /* 0a: Minimum extra paragraphs needed */
> + uint16_t e_maxalloc;   /* 0c: Maximum extra paragraphs needed */
> + uint16_t e_ss; /* 0e: Initial (relative) SS value */
> + uint16_t e_sp; /* 10: Initial SP value */
> + uint16_t e_csum;   /* 12: Checksum */
> + uint16_t e_ip; /* 14: Initial IP value */
> + uint16_t e_cs; /* 16: Initial (relative) CS value */
> + uint16_t e_lfarlc; /* 18: File address of relocation table */
> + uint16_t e_ovno;   /* 1a: Overlay number */
> + uint16_t e_res[4]; /* 1c: Reserved words */
> + uint16_t e_oemid;  /* 24: OEM identifier (for e_oeminfo) */
> + uint16_t e_oeminfo;/* 26: OEM information; e_oemid specific */
> + uint16_t e_res2[10];   /* 28: Reserved words */
> + uint32_t e_lfanew; /* 3c: Offset to extended header */
> +} IMAGE_DOS_HEADER, *PIMAGE_DOS_HEADER;
> +
> +#define IMAGE_DOS_SIGNATURE  0x5A4D /* MZ   */
> +#define IMAGE_NT_SIGNATURE   0x4550 /* PE00 */
> +
> +#define IMAGE_FILE_MACHINE_ARM   0x01c0
> +#define IMAGE_FILE_MACHINE_THUMB 0x01c2
> +#define IMAGE_FILE_MACHINE_ARMNT 0x01c4
> +#define IMAGE_FILE_MACHINE_AMD64 0x8664
> +#define IMAGE_FILE_MACHINE_ARM64 0xaa64
> +#define IMAGE_NT_OPTIONAL_HDR32_MAGIC0x10b
> +#define IMAGE_NT_OPTIONAL_HDR64_MAGIC0x20b
> +#define IMAGE_SUBSYSTEM_EFI_APPLICATION  10
> +
> +typedef struct _IMAGE_FILE_HEADER {
> + uint16_t  Machine;
> + uint16_t  NumberOfSections;
> + uint32_t TimeDateStamp;
> + uint32_t PointerToSymbolTable;
> + uint32_t NumberOfSymbols;
> + uint16_t  SizeOfOptionalHeader;
> + uint16_t  Characteristics;
> +} IMAGE_FILE_HEADER, *PIMAGE_FILE_HEADER;

There is something fishy going on with tabs vs. spaces in this file,
for variable name alignment. Above is one example, several more are
below. No further comments on functionality.

> +
> +typedef struct _IMAGE_DATA_DIRECTORY {
> + uint32_t VirtualAddress;
> + uint32_t Size;
> +} IMAGE_DATA_DIRECTORY, *PIMAGE_DATA_DIRECTORY;
> +
> +#define IMAGE_NUMBEROF_DIRECTORY_ENTRIES 16
> +
> +typedef struct _IMAGE_OPTIONAL_HEADER64 {
> + uint16_t  Magic; /* 0x20b */
> + uint8_t MajorLinkerVersion;
> + uint8_t MinorLinkerVersion;
> + uint32_t SizeOfCode;
> + uint32_t SizeOfInitializedData;
> + uint32_t 

Re: [U-Boot] [PATCH 06/14] efi_loader: Add runtime services

[Leif Lindholm]

On Fri, Jan 15, 2016 at 06:06:12AM +0100, Alexander Graf wrote:
> After booting has finished, EFI allows firmware to still interact with the OS
> using the "runtime services". These callbacks live in a separate address 
> space,
> since they are available long after U-Boot has been overwritten by the OS.
> 
> This patch adds enough framework for arbitrary code inside of U-Boot to become
> a runtime service with the right section attributes set. For now, we don't 
> make
> use of it yet though.
> 
> We could maybe in the future map U-boot environment variables to EFI variables
> here.
> 
> Signed-off-by: Alexander Graf 

Just a couple of return value issues:

> ---
> 
> v1 -> v2:
> 
>   - Fix runtime service sections
>   - Add runtime detach
>   - Enable runtime relocations
>   - Add get_time
>   - Fix relocation
>   - Fix 32bit
>   - Add am335x support
>   - Move section definition to header
>   - Add systab to runtime section
>   - Add self-relocation hook table
>   - Fix self-relocation
>   - Relocate efi_runtime section early during bootup
>   - Fix return values for a number of callbacks to be more UEFI compliant
>   - Move to GPLv2+
> ---
>  arch/arm/config.mk|   4 +
>  arch/arm/cpu/armv8/u-boot.lds |  16 +++
>  arch/arm/cpu/u-boot.lds   |  30 +
>  arch/arm/lib/sections.c   |   4 +
>  board/ti/am335x/u-boot.lds|  30 +
>  common/board_r.c  |   4 +
>  include/efi_loader.h  |  10 ++
>  lib/efi_loader/efi_boottime.c |   6 +-
>  lib/efi_loader/efi_runtime.c  | 300 
> ++
>  9 files changed, 401 insertions(+), 3 deletions(-)
>  create mode 100644 lib/efi_loader/efi_runtime.c
> 

...

> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> new file mode 100644
> index 000..b7aa1e9
> --- /dev/null
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -0,0 +1,300 @@
> +/*
> + *  EFI application runtime services
> + *
> + *  Copyright (c) 2016 Alexander Graf
> + *
> + *  SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* For manual relocation support */
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static efi_status_t EFI_RUNTIME_TEXT efi_unimplemented(void);
> +static efi_status_t EFI_RUNTIME_TEXT efi_device_error(void);
> +static efi_status_t EFI_RUNTIME_TEXT efi_invalid_parameter(void);
> +
> +#if defined(CONFIG_ARM64)
> +#define R_RELATIVE   1027
> +#define R_MASK   0xULL
> +#define IS_RELA  1
> +#elif defined(CONFIG_ARM)
> +#define R_RELATIVE   23
> +#define R_MASK   0xffULL
> +#else
> +#error Need to add relocation awareness
> +#endif
> +
> +struct elf_rel {
> + ulong *offset;
> + ulong info;
> +};
> +
> +struct elf_rela {
> + ulong *offset;
> + ulong info;
> + long addend;
> +};
> +
> +/*
> + * EFI Runtime code lives in 2 stages. In the first stage, U-Boot and an EFI
> + * payload are running concurrently at the same time. In this mode, we can
> + * handle a good number of runtime callbacks
> + */
> +
> +static void efi_reset_system(enum efi_reset_type reset_type,
> + efi_status_t reset_status, unsigned long data_size,
> + void *reset_data)
> +{
> + EFI_ENTRY("%d %lx %lx %p", reset_type, reset_status, data_size, 
> reset_data);
> +
> + switch (reset_type) {
> + case EFI_RESET_COLD:
> + case EFI_RESET_WARM:
> + do_reset(NULL, 0, 0, NULL);
> + break;
> + case EFI_RESET_SHUTDOWN:
> + /* We don't have anything to map this to */
> + break;
> + }
> +
> + EFI_EXIT(EFI_SUCCESS);
> +}
> +
> +static efi_status_t efi_get_time(struct efi_time *time,
> +   struct efi_time_cap *capabilities)
> +{
> +#ifdef CONFIG_CMD_DATE
> +
> + struct rtc_time tm;
> + int r;
> +#ifdef CONFIG_DM_RTC
> + struct udevice *dev;
> +#endif
> +
> + EFI_ENTRY("%p %p", time, capabilities);
> +
> +#ifdef CONFIG_DM_RTC
> + r = uclass_get_device(UCLASS_RTC, 0, );
> + if (r)
> + return EFI_EXIT(EFI_UNSUPPORTED);

EFI_DEVICE_ERROR?

> +#endif
> +
> +#ifdef CONFIG_DM_RTC
> + r = dm_rtc_get(dev, );
> +#else
> + r = rtc_get();
> +#endif
> + if (r)
> + return EFI_EXIT(EFI_UNSUPPORTED);

EFI_DEVICE_ERROR?

> +
> + memset(time, 0, sizeof(*time));
> + time->year = tm.tm_year;
> + time->month = tm.tm_mon;
> + time->day = tm.tm_mday;
> + time->hour = tm.tm_hour;
> + time->minute = tm.tm_min;
> + time->daylight = tm.tm_isdst;
> +
> + return EFI_EXIT(EFI_SUCCESS);
> +
> +#else /* CONFIG_CMD_DATE */
> +
> + return EFI_DEVICE_ERROR;
> +
> +#endif /* CONFIG_CMD_DATE */
> +}


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 04/14] efi_loader: Add boot time services

[Leif Lindholm]

On Fri, Jan 15, 2016 at 06:06:10AM +0100, Alexander Graf wrote:
> When an EFI application runs, it has access to a few descriptor and callback
> tables to instruct the EFI compliant firmware to do things for it. The bulk
> of those interfaces are "boot time services". They handle all object 
> management,
> and memory allocation.
> 
> This patch adds support for the boot time services and also exposes a system
> table, which is the point of entry descriptor table for EFI payloads.
> 
> Signed-off-by: Alexander Graf 
> 
> ---
> 
> v1 -> v2:
> 
>   - Fix typo s/does now/does not/
>   - Add #ifdefs around header to allow inclusion when efi_loader is disabled
>   - Add stub efi_restore_gd() function when efi_loader is disabled
>   - Disable debug
>   - Mark runtime region as such
>   - Fix up memory map
>   - Allow efi_restore_gd to be called before first efi entry
>   - Add 32bit arm cache workaround
>   - Move memory map to separate patch
>   - Change BTS version to 2.5
>   - Fix return values for a few callbacks to more EFI compliant ones
>   - Change vendor to "Das U-Boot"
>   - Add warning when truncating timer trigger
>   - Move to GPLv2+
> ---
>  include/efi_loader.h  |  51 +++
>  lib/efi_loader/efi_boottime.c | 761 
> ++
>  2 files changed, 812 insertions(+)
>  create mode 100644 lib/efi_loader/efi_boottime.c
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index bf77573..391459e 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -6,18 +6,69 @@
>   *  SPDX-License-Identifier: GPL-2.0+
>   */
>  
> +#include 
>  #include 
>  #include 
> +
> +#ifdef CONFIG_EFI_LOADER
> +
>  #include 
>  
> +/* #define DEBUG_EFI */
> +
> +#ifdef DEBUG_EFI
> +#define EFI_ENTRY(format, ...) do { \
> + efi_restore_gd(); \
> + printf("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
> + } while(0)
> +#else
> +#define EFI_ENTRY(format, ...) do { \
> + efi_restore_gd(); \
> + } while(0)
> +#endif
> +
> +#define EFI_EXIT(ret) efi_exit_func(ret);
> +
> +extern struct efi_system_table systab;
> +
>  extern const efi_guid_t efi_guid_device_path;
>  extern const efi_guid_t efi_guid_loaded_image;
>  
> +struct efi_class_map {
> + const efi_guid_t *guid;
> + const void *interface;
> +};
> +
> +struct efi_handler {
> + const efi_guid_t *guid;
> + efi_status_t (EFIAPI *open)(void *handle,
> + efi_guid_t *protocol, void **protocol_interface,
> + void *agent_handle, void *controller_handle,
> + uint32_t attributes);
> +};
> +
> +struct efi_object {
> + struct list_head link;
> + struct efi_handler protocols[4];
> + void *handle;
> +};
> +extern struct list_head efi_obj_list;
> +
>  efi_status_t efi_return_handle(void *handle,
>   efi_guid_t *protocol, void **protocol_interface,
>   void *agent_handle, void *controller_handle,
>   uint32_t attributes);
> +void efi_timer_check(void);
>  void *efi_load_pe(void *efi, struct efi_loaded_image *loaded_image_info);
> +void efi_save_gd(void);
> +void efi_restore_gd(void);
> +efi_status_t efi_exit_func(efi_status_t ret);
>  
>  #define EFI_LOADER_POOL_SIZE (128 * 1024 * 1024)
>  void *efi_loader_alloc(uint64_t len);
> +
> +#else /* defined(EFI_LOADER) */
> +
> +static inline void efi_restore_gd(void) { }
> +
> +#endif
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> new file mode 100644
> index 000..5756c9c
> --- /dev/null
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -0,0 +1,761 @@
> +/*
> + *  EFI application boot time services
> + *
> + *  Copyright (c) 2016 Alexander Graf
> + *
> + *  SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +/* #define DEBUG_EFI */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/*
> + * If we're running on nasty systems (32bit ARM booting into non-EFI Linux)
> + * we need to do trickery with caches. Since we don't want to break the EFI
> + * aware boot path, only apply hacks when loading exiting directly (breaking
> + * direct Linux EFI booting along the way - oh well).
> + */
> +static bool efi_is_direct_boot = true;
> +
> +/*
> + * EFI can pass arbitrary additional "tables" containing vendor specific
> + * information to the payload. One such table is the FDT table which contains
> + * a pointer to a flattened device tree blob.
> + *
> + * In most cases we want to pass an FDT to the payload, so reserve one slot 
> of
> + * config table space for it. The pointer gets populated by 
> do_bootefi_exec().
> + */
> +static struct efi_configuration_table efi_conf_table[] = {
> + {
> + .guid = EFI_FDT_GUID,
> + },
> +};
> +
> +/*
> + * The "gd" pointer lives in a register on ARM and AArch64 that we declare
> + * fixed when compiling U-Boot. However, the payload 

Re: [U-Boot] [PATCH 3/9] efi_loader: Add PE image loader

[Leif Lindholm]

On Fri, Jan 15, 2016 at 12:45:46AM +0100, Alexander Graf wrote:
> On 26.12.15 17:26, Leif Lindholm wrote:
> >> new file mode 100644
> >> index 000..688e177
> >> --- /dev/null
> >> +++ b/lib/efi_loader/efi_image_loader.c
> >> @@ -0,0 +1,203 @@
> >> +/*
> >> + *  EFI image loader
> >> + *
> >> + *  based partly on wine code
> >> + *
> >> + *  Copyright (c) 2015 Alexander Graf
> >> + *
> >> + *  This library is free software; you can redistribute it and/or
> >> + *  modify it under the terms of the GNU Lesser General Public
> >> + *  License as published by the Free Software Foundation; either
> >> + *  version 2.1 of the License, or (at your option) any later version.
> >> + *
> >> + *  This library is distributed in the hope that it will be useful,
> >> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> + *  Lesser General Public License for more details.
> >> + *
> >> + *  You should have received a copy of the GNU Lesser General Public
> >> + *  License along with this library; if not, write to the Free Software
> >> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, 
> >> USA
> >> + *
> >> + *  SPDX-License-Identifier: LGPL-2.1+
> >> + */
> >> +
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +
> >> +DECLARE_GLOBAL_DATA_PTR;
> >> +
> >> +#define ROUND_UP(val, round) ((val + (round - 1)) & ~(round - 1))
> >> +#define MB (1024 * 1024)
> >> +
> >> +const efi_guid_t efi_guid_device_path = DEVICE_PATH_GUID;
> >> +const efi_guid_t efi_guid_loaded_image = LOADED_IMAGE_GUID;
> >> +
> >> +efi_status_t efi_return_handle(void *handle, efi_guid_t *protocol,
> >> +  void **protocol_interface, void *agent_handle,
> >> +  void *controller_handle, uint32_t attributes)
> >> +{
> >> +  *protocol_interface = handle;
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +/*
> >> + * EFI payloads potentially want to load pretty big images into memory,
> >> + * so our small malloc region isn't enough for them. However, they usually
> >> + * don't need a smart allocator either.
> >> + *
> >> + * So instead give them a really dumb one. We just reserve 
> >> EFI_LOADER_POOL_SIZE
> >> + * bytes from 16MB below the malloc region start to give the stack some 
> >> space.
> > 
> > Is there any chance you could break out the memory allocation and
> > memory map management as a separate patch, rather than spread across
> > 3,4 and 6?
> 
> Uh, sure, I can try. I'm not sure it'll end up more readable than now
> though :).

Heh, ok. It was a theory.

> > Assigning a specific 128MB pool for LOADER_DATA memory can have
> > unexpected side effects.
> 
> Like for example?

Like applications running out of memory when there's plenty left in
the system. OK, so not "unexpected" unexpected, but "may cause valid
programs to break". My initial comment was because I was scratching my
head over how GRUB managed to allocate its heap at all.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 4/9] efi_loader: Add boot time services

[Leif Lindholm]

On Fri, Jan 15, 2016 at 01:13:15AM +0100, Alexander Graf wrote:
> On 26.12.15 19:09, Leif Lindholm wrote:
> >> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> >> new file mode 100644
> >> index 000..ed95962
> >> --- /dev/null
> >> +++ b/lib/efi_loader/efi_boottime.c
> >> @@ -0,0 +1,838 @@
> >> +/*
> >> + *  EFI application boot time services
> >> + *
...
> >> +
> >> +static unsigned long efi_raise_tpl(unsigned long new_tpl)
> >> +{
> >> +  EFI_ENTRY("0x%lx", new_tpl);
> >> +  return EFI_EXIT(efi_unsupported(__func__));
> > 
> > "Unlike other UEFI interface functions, EFI_BOOT_SERVICES.RaiseTPL()
> > does not return a status code. Instead, it returns the previous task
> > priority level, which is to be restored later with a matching call to
> > RestoreTPL()."
> 
> Since we don't do TPLs (or IRQs for that matter), I'll just return 0 here.

Sure.

> >> +}
> >> +
> >> +static void efi_restore_tpl(unsigned long old_tpl)
> >> +{
> >> +  EFI_ENTRY("0x%lx", old_tpl);
> >> +  EFI_EXIT(efi_unsupported(__func__));
> > 
> > (void function, nothing to return)
> 
> Yes, hence no return. EFI_EXIT deals with the gd swapping and
> efi_unsupported() gives me a nice debug message :).

Ah, ok.

> >> +static efi_status_t efi_allocate_pages(int type, int memory_type,
> >> + unsigned long pages, uint64_t *memory)
> >> +{
> >> +  u64 len = pages << 12;
> >> +  efi_status_t r = EFI_SUCCESS;
> >> +
> >> +  EFI_ENTRY("%d, %d, 0x%lx, %p", type, memory_type, pages, memory);
> >> +
> >> +  switch (type) {
> >> +  case 0:
> >> +  /* Any page means we can go to efi_alloc */
> >> +  *memory = (unsigned long)efi_alloc(len, memory_type);
> >> +  break;
> >> +  case 1:
> >> +  /* Max address */
> >> +  if (gd->relocaddr < *memory) {
> >> +  *memory = (unsigned long)efi_alloc(len, memory_type);
> >> +  break;
> >> +  }
> >> +  r = EFI_UNSUPPORTED;
> > 
> > EFI_OUT_OF_RESOURCES/EFI_NOT_FOUND?
> > 
> >> +  break;
> >> +  case 2:
> >> +  /* Exact address, grant it. The addr is already in *memory. */
> > 
> > As far as I can tell, this is why GRUB works. Because it filters
> > through the memory map manually, requesting to allocate its heap at an
> > exact address in a region of free memory in the UEFI memory map.
> 
> Yes.
> 
> > The key is that EFI_LOADER_MEMORY will be used by applications loaded
> > as well as by U-Boot to load applications into. A simple example where
> > this could be problematic would be a large(ish) initrd loaded via initrd=
> > on kernel (stub loader) command line rather than via GRUB.
> 
> Ah, so here the 128MB limit on the LOADER_DATA section might bite us?

Yeah, I think so.

> >> +  runtime_start = (ulong)&__efi_runtime_start & ~0xfffULL;
> >> +  runtime_end = ((ulong)&__efi_runtime_stop + 0xfff) & ~0xfffULL;
> >> +  runtime_len_pages = (runtime_end - runtime_start) >> 12;
> >> +  runtime_len = runtime_len_pages << 12;
> >> +
> >> +  /* Fill in where normal RAM is (up to U-Boot) */
> >> +  efi_memory_map[0].num_pages = gd->relocaddr >> 12;
> > 
> > U-Boot question: is gd->relocaddr always the offset from start of RAM?
> > How does this work with gaps in memory map?
> 
> U-Boot always relocates itself at TOM (or at least what we consider TOM
> here). gd->relocaddr is the physical address of the start of U-Boot
> which is right below TOM.

Still ... would need additional handling if memory has holes (like,
fitted with multiple DIMMs smaller than the individual memory window
reserved for them). Or even on something like Juno, which has 2GB of
RAM at 0x00_8000_, and a further 6GB at 0x08_8000_ (I think).

> >> +#ifdef CONFIG_SYS_SDRAM_BASE
> >> +  efi_memory_map[0].physical_start = CONFIG_SYS_SDRAM_BASE;
> >> +  efi_memory_map[0].virtual_start = CONFIG_SYS_SDRAM_BASE;
> >> +  efi_memory_map[0].num_pages -= CONFIG_SYS_SDRAM_BASE >> 12;
> > #else
> > #error "..."
> > ?
> 
> If it's not defined, it's 0 :).

Make it
#if CONFIG_SYS_SDRAM_BASE != 0
for clarity?

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 4/9] efi_loader: Add boot time services

[Leif Lindholm]

On Fri, Jan 15, 2016 at 03:14:54PM +0100, Alexander Graf wrote:
> On 15.01.16 14:02, Leif Lindholm wrote:
> >>> U-Boot question: is gd->relocaddr always the offset from start of RAM?
> >>> How does this work with gaps in memory map?
> >>
> >> U-Boot always relocates itself at TOM (or at least what we consider TOM
> >> here). gd->relocaddr is the physical address of the start of U-Boot
> >> which is right below TOM.
> > 
> > Still ... would need additional handling if memory has holes (like,
> > fitted with multiple DIMMs smaller than the individual memory window
> > reserved for them). Or even on something like Juno, which has 2GB of
> > RAM at 0x00_8000_, and a further 6GB at 0x08_8000_ (I think).
> 
> Yes. I think we'll have to just implement that when we hit the first
> board that does this (and has awareness in U-Boot for it).

Yeah, sure.

> With the UEFI entry path, Linux will ignore memory nodes in dt, right?

Yep.

> >>>> +#ifdef CONFIG_SYS_SDRAM_BASE
> >>>> +efi_memory_map[0].physical_start = CONFIG_SYS_SDRAM_BASE;
> >>>> +efi_memory_map[0].virtual_start = CONFIG_SYS_SDRAM_BASE;
> >>>> +efi_memory_map[0].num_pages -= CONFIG_SYS_SDRAM_BASE >> 12;
> >>> #else
> >>> #error "..."
> >>> ?
> >>
> >> If it's not defined, it's 0 :).
> > 
> > Make it
> > #if CONFIG_SYS_SDRAM_BASE != 0
> > for clarity?
> 
> I really don't think we'll make it more readable with more #iffery here :).

It isn't really the kind of thing I'm here to review, but ...
Current version requires knowledge of state of certain macros, the
alternative is explicit without familiarity with the codebase.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 6/9] efi_loader: Add runtime services

[Leif Lindholm]

On Fri, Jan 15, 2016 at 03:15:37PM +0100, Alexander Graf wrote:
> On 15.01.16 14:52, Leif Lindholm wrote:
> >>> "The ResetSystem() function does not return."
> >>
> >> Hrm, I think returning EFI_UNSUPPORTED is still better than while(1) {
> >> }. With the return an OS at least has the chance to fix things up itself.
> > 
> > I'm not saying it isn't better, I'm saying it's not compliant - there
> > is no valid return value. I would prefer simply having the pointer set
> > to NULL.
> 
> Is a NULL function pointer valid here?

More than returning is.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 6/9] efi_loader: Add runtime services

[Leif Lindholm]

On Fri, Jan 15, 2016 at 01:26:42AM +0100, Alexander Graf wrote:
> On 26.12.15 19:33, Leif Lindholm wrote:
> >> +  .reset_system = (void *)_unimplemented,
> > 
> > "The ResetSystem() function does not return."
> 
> Hrm, I think returning EFI_UNSUPPORTED is still better than while(1) {
> }. With the return an OS at least has the chance to fix things up itself.

I'm not saying it isn't better, I'm saying it's not compliant - there
is no valid return value. I would prefer simply having the pointer set
to NULL.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 0/9] EFI payload / application support

[Leif Lindholm]

On Sun, Dec 27, 2015 at 01:10:39PM -0500, Tom Rini wrote:
> So, my only "big" concern here is, are we as a community able to view
> and implement the relevant parts of the UEFI spec (without having to
> agree to a potentially complicated enough license to have to bug a
> lawyer)?  It's been a while since I tried to view a copy so I'm hoping
> the answer is now yes.  Thanks!

So:
1) You can certainly read the specification without agreeing to
   anything (click-through is now gone - docs accessible straight from
   http://uefi.org/specifications/).
2) The potential complication would be with regards to implementing
   and distributing - which requires signing the Adopters Agreement:
   
http://uefi.org/sites/default/files/resources/UEFI_Adopters_Agreement_100907.pdf

I actually think the implications of 2) for a project like U-Boot
would be a useful thing to bring up for discussion on the FW/OS forum
mailing list at http://uefi.org/FWOSForum.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 4/9] efi_loader: Add boot time services

[Leif Lindholm]

On Tue, Dec 22, 2015 at 02:57:51PM +0100, Alexander Graf wrote:
> When an EFI application runs, it has access to a few descriptor and callback
> tables to instruct the EFI compliant firmware to do things for it. The bulk
> of those interfaces are "boot time services". They handle all object 
> management,
> and memory allocation.
> 
> This patch adds support for the boot time services and also exposes a system
> table, which is the point of entry descriptor table for EFI payloads.

One overall observation, and I may help track these down - but not all
for this review: this code uses EFI_UNSUPPORTED as a default
"something went wrong" error code, but this is not actually supported
by the specification. I'm pointing out a few of these, but it would be
preferable if we could crowdsource this a bit since there are quire a
few instances...

> Signed-off-by: Alexander Graf 
> ---
>  include/efi_loader.h  |  41 +++
>  lib/efi_loader/efi_boottime.c | 838 
> ++
>  2 files changed, 879 insertions(+)
>  create mode 100644 lib/efi_loader/efi_boottime.c
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index da82354..ed7c389 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -24,14 +24,55 @@
>  #include 
>  #include 
>  
> +/* #define DEBUG_EFI */
> +
> +#ifdef DEBUG_EFI
> +#define EFI_ENTRY(format, ...) do { \
> + efi_restore_gd(); \
> + printf("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
> + } while(0)
> +#else
> +#define EFI_ENTRY(format, ...) do { \
> + efi_restore_gd(); \
> + } while(0)
> +#endif
> +
> +#define EFI_EXIT(ret) efi_exit_func(ret);
> +
> +extern struct efi_system_table systab;
> +
>  extern const efi_guid_t efi_guid_device_path;
>  extern const efi_guid_t efi_guid_loaded_image;
>  
> +struct efi_class_map {
> + const efi_guid_t *guid;
> + const void *interface;
> +};
> +
> +struct efi_handler {
> + const efi_guid_t *guid;
> + efi_status_t (EFIAPI *open)(void *handle,
> + efi_guid_t *protocol, void **protocol_interface,
> + void *agent_handle, void *controller_handle,
> + uint32_t attributes);
> +};
> +
> +struct efi_object {
> + struct list_head link;
> + struct efi_handler protocols[4];
> + void *handle;
> +};
> +extern struct list_head efi_obj_list;
> +
>  efi_status_t efi_return_handle(void *handle,
>   efi_guid_t *protocol, void **protocol_interface,
>   void *agent_handle, void *controller_handle,
>   uint32_t attributes);
> +void efi_timer_check(void);
>  void *efi_load_pe(void *efi, struct efi_loaded_image *loaded_image_info);
> +void efi_save_gd(void);
> +void efi_restore_gd(void);
> +efi_status_t efi_exit_func(efi_status_t ret);
>  
>  #define EFI_LOADER_POOL_SIZE (128 * 1024 * 1024)
>  void *efi_loader_alloc(uint64_t len);
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> new file mode 100644
> index 000..ed95962
> --- /dev/null
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -0,0 +1,838 @@
> +/*
> + *  EFI application boot time services
> + *
> + *  Copyright (c) 2015 Alexander Graf
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> + *
> + *  SPDX-License-Identifier: LGPL-2.1+
> + */
> +
> +#define DEBUG_EFI
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/*
> + * EFI can pass arbitrary additional "tables" containing vendor specific
> + * information to the payload. One such table is the FDT table which contains
> + * a pointer to a flattened device tree blob.
> + *
> + * In most cases we want to pass an FDT to the payload, so reserve one slot 
> of
> + * config table space for it. The pointer gets populated by 
> do_bootefi_exec().
> + */
> +static struct efi_configuration_table efi_conf_table[] = {
> + {
> + .guid = EFI_FDT_GUID,
> + },
> +};
> +
> +/*
> + * The "gd" pointer lives in a register on ARM and AArch64 that we declare
> + * fixed when compiling U-Boot. However, the payload does now know about that
> + * restriction so we need to manually 

Re: [U-Boot] [PATCH 8/9] efi_loader: Add "bootefi" command

[Leif Lindholm]

On Fri, Dec 25, 2015 at 10:02:44AM +0100, Alexander Graf wrote:
> On 24.12.15 12:15, Matwey V. Kornilov wrote:
> > Why just not to implement standard EFI behaviour when EFI looks for
> > boot-efi partition and proceed?
> 
> Well, what is "standard EFI behavior"?
> 
> There are 2 standard ways I'm aware of:
> 
>   1) NVRAM
> 
> The default case for 99.9% of the boots on normal EFI systems is based
> on variables in NVRAM that tell EFI which boot device to boot from.
> Since we don't implement EFI variables today, we can't really make use
> of this feature. And because you want to change the default boot device
> at runtime, we'd have to have runtime services be able to modify them
> after exiting boot services.
> 
>   2) Removable Media
> 
> There is another way implemented for "Removable Media" - mostly intended
> for USB sticks and the likes. Here EFI searches for a defined file name
> (EFI/boot/boot{arm,a64,x64}.efi) on the ESP partition and boots it.
> 
> Part 1 is very difficult to do without major rework of a few U-Boot
> components. If EFI becomes the de-facto standard way of booting with
> U-Boot, I think we'll walk down that road, but it's nothing I want to
> have to deal with in the initial enablement discussion.
> 
> Part 2 is easy to do. But then again it's also easy to do it using a
> boot script. Or a compiled in bootcmd. If it's really desired.

Sure, I've seen a lot more complicated boot operations than that
shipped in the default environment of many platforms.

> Which brings me to the next idea. What if we just implement exlinux.conf
> support for EFI binaries? Then all you need to do is have an
> extlinux.conf available on your generic EFI media that tells U-Boot
> where to load the grub binary from.

The problem with this is that you're adding things unrelated to UEFI
into a UEFI boot scenario. Now, depending on what you're looking for,
this may be fine - but it will not leave you with the ability to rely
on being able to "just work" with the UEFI support provided by
UEFI-aware operating systems and their installation media.

Basically, it comes down to preference - is this about:
- Leveraging the benefits of UEFI while staying with the U-Boot
  codebase.
or
- Leveraging some of the benefits of UEFI in order to give distros a
  more device-independent way of supporting U-Boot platforms.

> That way we wouldn't bend U-Boot completely away from its heritage, make
> use of its flexibility and all distributions that actually care about
> booting from U-Boot would easily be able to just put such a file in an
> rpm an install it always.

You would also need a way to distinguish whether booting with UEFI or
U-Boot efiload.

Sure, compared to stuff like flash-kernel, this is hardly rocket
science, but would be worth standardising early and hard.

> > If ARM board developers will enable EFI support in the future, we can
> > have single one JeOS having all possible dtb in KIWI image.
> > BeagleBone Black has its own u-boot on eMMC, and the user need to push
> > S2 button to force hardware to use our openSUSE u-boot from SD card.
> > Maybe something like that is for other boards. If the single one
> > required u-boot feature is to run EFI grub, then we can even don't
> > touch preinstalled bootloader, that is not possible now, because we
> > need our openSUSE boot scripts.
> 
> There are more things we need to solve before we can truly get to a
> universal booting solution. But one step at a time :).
> 
> The reason I implemented "bootefi" was really because it's the natural
> fit into how U-Boot handles all other formats today. I don't think this
> is going to be the last patch set around EFI support.
> 
> 
> Alex
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 8/9] efi_loader: Add "bootefi" command

[Leif Lindholm]

On Fri, Dec 25, 2015 at 10:25:22AM +0100, Andreas Färber wrote:
> Am 25.12.2015 um 10:02 schrieb Alexander Graf:
> [snip]
> > The reason I implemented "bootefi" was really because it's the natural
> > fit into how U-Boot handles all other formats today. I don't think this
> > is going to be the last patch set around EFI support.
> 
> I think what Matwey was suggesting is integrating your "bootefi" into
> the standard "distro" boot sequence environment, so that it probes each
> device for an EFI binary and if it finds one runs load and bootefi,
> without the need for any boot.scr.
> 
> That would be a follow-up patch.
> 
> It however conflicts with your idea of having some potentially
> board-specific code mess with "fdt addr" command before running "bootefi".

This could however be resolved by moving to a model where the
device-tree was considered a component of the firmware (which is how
we treat it in UEFI). If U-Boot had an awareness (if it does not
already) of an FDT being something it should have and know about, then
this could trivially be passed onto bootefi.
This could means a "default_fdt" environment variable which is loaded
automatically before bootcmd is executed.

> My solution would be to give boot.scr preference over *.efi, so that the
> user has a way to load dtb and run "bootefi" on his own, and otherwise
> fall back to just "bootefi" which'll spit a warning about lack of fdt if
> I read that correctly.

An explicit bootscript should always override a fallback boot option.
And booting the "removable boot path" is a fallback boot option.

Regards,

Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 0/9] EFI payload / application support

[Leif Lindholm]

On Sat, Dec 26, 2015 at 05:27:48PM +0100, Alexander Graf wrote:
> >> This patch set is the result of pursuing this endeavor.
> > 
> > Thanks, this is a very cool thing.
> > I meant to reply sooner, but Christmas got in the way.
> > 
> >>   - I am successfully able to run grub2 and Linux EFI binaries with this 
> >> code.
> >>   - When enabled, the resulting U-Boot binary only grows by ~10kb,
> >> so it's very light weight.
> >>   - It works on 32bit ARM and AArch64. 
> >>   - All storage devices are directly accessible
> >>   - No runtime services (all calls return unimplemented)
> > 
> > Yeah, this is a bit of a pain point. The time services, virtual memory
> > services and reset being the key ones.
> 
> I guess reset should be pretty well doable. What are virtual memory
> services? The bits that translate RTS code to run in virtual address
> space somewhere else?

Yup.

> > 
> >>   - No EFI variables
> > 
> > This would obviously (from my point of view) be desirable, but at
> > least initially, we can do most things without persistent variables.
> 
> Doing EFI variables before exiting boot services is easy - we could even
> just map U-Boot variables to EFI variables. That could come in handy for
> cases where you want U-Boot to tell you which board you're on so you can
> refer to different dtb files in your grub.cfg (if you need to override
> the dtb).
> 
> Making them persistent is another difficult question - U-Boot usually
> splits "modify variable" from "store variable pool to nvram".

Indeed.
UEFI might as well, but in more convoluted ways.

> And the hardest part would obviously be to make all of this work while
> Linux is running ;). I'd definitely prefer to defer that bit to later.

Of course.

And again - depending on ambition levels, implementing a version of
efibootmgr that ended up simply tweaking a /boot/uEnv.txt (or similar)
if U-Boot boot environment was detected would be entirely achievable.

> >> Of course, there are still a few things one could do on top:
> >>
> >>   - Implement removable media booting (search for 
> >> /efi/boot/boota{a64,rm}.efi)
> > 
> > Yeah, that would be top of my wishlist.
> 
> That should be pretty simple to do - maybe we can even get away without
> any C code for it.

Should be possible.

> > 
> >>   - Improve disk media detection (don't scan, use what information we have)
> >>   - Add EFI variable support using NVRAM
> >>   - Add GFX support
> > 
> > GFX support was actually never implemented for U-Boot GRUB, so from
> > this p.o.v. it is not a shortcoming over the existing impementation.
> 
> Heh ;). My goal here really is to make U-Boot based systems be en par
> with TianoCore based ones in terms of usability.

Sure. Just setting the nearer goal post of "where can we replace
U-Boot API in GRUB".

> > 
> >>   - Make EFI Shell work ;)
> > 
> > - Network device support.
> 
> Oh, good point. I forgot about that one.
> 
> > I also spotted a couple of minor things while playing around (things
> > like image exit being missing), but these will be easy to flush out.
> 
> Awesome, looking forward to the review! :)
> 
> Thanks a lot for taking the time to look at this patch set.

Sure, this is a very useful thing.

Even at the most cynical possible position (which isn't where I am),
this means additional interface testing using a completely separate
codebase.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 0/9] EFI payload / application support

[Leif Lindholm]

On Tue, Dec 22, 2015 at 02:57:47PM +0100, Alexander Graf wrote:
> This is my Christmas present for my openSUSE friends :).
> 
> U-Boot is a great project for embedded devices. However, convincing
> everyone involved that only for "a few oddball ARM devices" we need to
> support different configuration formats from grub2 when all other platforms
> (PPC, System Z, x86) are standardized on a single format is a nightmare.
> 
> So we started to explore alternatives. At first, people tried to get
> grub2 running using the u-boot api interface. However, FWIW that one
> doesn't support relocations, so you need to know where to link grub2 to
> at compile time. It also seems to be broken more often than not. And on
> top of it all, it's a one-off interface, so yet another thing to maintain.
> 
> That led to a nifty idea. What if we can just implement the EFI application
> protocol on top of U-Boot? Then we could compile a single grub2 binary for
> uEFI based systems and U-Boot based systems and as soon as that one's loaded,
> everything looks and feels (almost) the same.
> 
> This patch set is the result of pursuing this endeavor.

Thanks, this is a very cool thing.
I meant to reply sooner, but Christmas got in the way.

>   - I am successfully able to run grub2 and Linux EFI binaries with this code.
>   - When enabled, the resulting U-Boot binary only grows by ~10kb,
> so it's very light weight.
>   - It works on 32bit ARM and AArch64. 
>   - All storage devices are directly accessible
>   - No runtime services (all calls return unimplemented)

Yeah, this is a bit of a pain point. The time services, virtual memory
services and reset being the key ones.

>   - No EFI variables

This would obviously (from my point of view) be desirable, but at
least initially, we can do most things without persistent variables.

> Of course, there are still a few things one could do on top:
> 
>   - Implement removable media booting (search for /efi/boot/boota{a64,rm}.efi)

Yeah, that would be top of my wishlist.

>   - Improve disk media detection (don't scan, use what information we have)
>   - Add EFI variable support using NVRAM
>   - Add GFX support

GFX support was actually never implemented for U-Boot GRUB, so from
this p.o.v. it is not a shortcoming over the existing impementation.

>   - Make EFI Shell work ;)

- Network device support.

I also spotted a couple of minor things while playing around (things
like image exit being missing), but these will be easy to flush out.


I'll leave reviewing of the u-boot side of things to people who know
the codebase better, and restrict myself to commenting on the
UEFIness.

/
Leif

> But so far, I'm very happy with the state of the patches. They completely
> eliminate potential arguments against U-Boot internally and give users the
> chance to run with the same level of comfort on all firmware types.
> 
> 
> Alex
> 
> Alexander Graf (9):
>   disk/part.c: Expose a list of available block drivers
>   include/efi_api.h: Add more detailed API definitions
>   efi_loader: Add PE image loader
>   efi_loader: Add boot time services
>   efi_loader: Add console interface
>   efi_loader: Add runtime services
>   efi_loader: Add disk interfaces
>   efi_loader: Add "bootefi" command
>   efi_loader: hook up in build environment
> 
>  arch/arm/cpu/armv8/u-boot.lds |   8 +
>  arch/arm/cpu/u-boot.lds   |  13 +
>  arch/arm/lib/sections.c   |   2 +
>  common/Makefile   |   1 +
>  common/cmd_bootefi.c  | 168 
>  disk/part.c   |  25 ++
>  include/efi_api.h | 197 +++--
>  include/efi_loader.h  |  87 
>  include/part.h|   2 +
>  include/pe.h  | 277 +
>  lib/Kconfig   |   1 +
>  lib/Makefile  |   1 +
>  lib/efi_loader/Kconfig|   8 +
>  lib/efi_loader/Makefile   |  11 +
>  lib/efi_loader/efi_boottime.c | 838 
> ++
>  lib/efi_loader/efi_console.c  | 371 +
>  lib/efi_loader/efi_disk.c | 227 +++
>  lib/efi_loader/efi_image_loader.c | 203 +
>  lib/efi_loader/efi_runtime.c  |  59 +++
>  19 files changed, 2462 insertions(+), 37 deletions(-)
>  create mode 100644 common/cmd_bootefi.c
>  create mode 100644 include/efi_loader.h
>  create mode 100644 include/pe.h
>  create mode 100644 lib/efi_loader/Kconfig
>  create mode 100644 lib/efi_loader/Makefile
>  create mode 100644 lib/efi_loader/efi_boottime.c
>  create mode 100644 lib/efi_loader/efi_console.c
>  create mode 100644 lib/efi_loader/efi_disk.c
>  create mode 100644 lib/efi_loader/efi_image_loader.c
>  create mode 100644 lib/efi_loader/efi_runtime.c
> 
> -- 
> 2.1.4
> 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 3/9] efi_loader: Add PE image loader

[Leif Lindholm]

On Tue, Dec 22, 2015 at 02:57:50PM +0100, Alexander Graf wrote:
> EFI uses the PE binary format for its application images. Add support to EFI 
> PE
> binaries as well as all necessary bits for the "EFI image loader" interfaces.
> 
> Signed-off-by: Alexander Graf 
> ---
>  include/efi_loader.h  |  37 +
>  include/pe.h  | 277 
> ++
>  lib/efi_loader/efi_image_loader.c | 203 
>  3 files changed, 517 insertions(+)
>  create mode 100644 include/efi_loader.h
>  create mode 100644 include/pe.h
>  create mode 100644 lib/efi_loader/efi_image_loader.c
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> new file mode 100644
> index 000..da82354
> --- /dev/null
> +++ b/include/efi_loader.h
> @@ -0,0 +1,37 @@
> +/*
> + *  EFI application loader
> + *
> + *  Copyright (c) 2015 Alexander Graf
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> + *
> + *  SPDX-License-Identifier: LGPL-2.1+
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +extern const efi_guid_t efi_guid_device_path;
> +extern const efi_guid_t efi_guid_loaded_image;
> +
> +efi_status_t efi_return_handle(void *handle,
> + efi_guid_t *protocol, void **protocol_interface,
> + void *agent_handle, void *controller_handle,
> + uint32_t attributes);
> +void *efi_load_pe(void *efi, struct efi_loaded_image *loaded_image_info);
> +
> +#define EFI_LOADER_POOL_SIZE (128 * 1024 * 1024)
> +void *efi_loader_alloc(uint64_t len);
> diff --git a/include/pe.h b/include/pe.h
> new file mode 100644
> index 000..009b0c5
> --- /dev/null
> +++ b/include/pe.h
> @@ -0,0 +1,277 @@
> +/*
> + *  Portable Executable binary format structures
> + *
> + *  Copyright (c) 2015 Alexander Graf
> + *
> + *  Based on wine code
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> + *
> + *  SPDX-License-Identifier: LGPL-2.1+
> + */
> +
> +#ifndef _PE_H
> +#define _PE_H
> +
> +typedef struct _IMAGE_DOS_HEADER {
> + uint16_t e_magic;  /* 00: MZ Header signature */
> + uint16_t e_cblp;   /* 02: Bytes on last page of file */
> + uint16_t e_cp; /* 04: Pages in file */
> + uint16_t e_crlc;   /* 06: Relocations */
> + uint16_t e_cparhdr;/* 08: Size of header in paragraphs */
> + uint16_t e_minalloc;   /* 0a: Minimum extra paragraphs needed */
> + uint16_t e_maxalloc;   /* 0c: Maximum extra paragraphs needed */
> + uint16_t e_ss; /* 0e: Initial (relative) SS value */
> + uint16_t e_sp; /* 10: Initial SP value */
> + uint16_t e_csum;   /* 12: Checksum */
> + uint16_t e_ip; /* 14: Initial IP value */
> + uint16_t e_cs; /* 16: Initial (relative) CS value */
> + uint16_t e_lfarlc; /* 18: File address of relocation table */
> + uint16_t e_ovno;   /* 1a: Overlay number */
> + uint16_t e_res[4]; /* 1c: Reserved words */
> + uint16_t e_oemid;  /* 24: OEM identifier (for e_oeminfo) */
> + uint16_t e_oeminfo;/* 26: OEM information; e_oemid specific */
> + uint16_t e_res2[10];   /* 28: Reserved words */
> + uint32_t e_lfanew; /* 3c: Offset to extended header */
> +} IMAGE_DOS_HEADER, *PIMAGE_DOS_HEADER;
> +
> +#define IMAGE_DOS_SIGNATURE  0x5A4D /* MZ   */
> +#define IMAGE_NT_SIGNATURE   0x4550 /* PE00 */
> +
> +#define IMAGE_FILE_MACHINE_ARM   0x01c0
> +#define 

Re: [U-Boot] [PATCH 6/9] efi_loader: Add runtime services

[Leif Lindholm]

On Tue, Dec 22, 2015 at 02:57:53PM +0100, Alexander Graf wrote:
> After booting has finished, EFI allows firmware to still interact with the OS
> using the "runtime services". These callbacks live in a separate address 
> space,
> since they are available long after U-Boot has been overwritten by the OS.
> 
> However, since U-Boot has no notion of RTS, we just create an extremely 
> minimal
> RTS stub that just declares all functions as unsupported. We could in the 
> future
> map U-boot environment variables to EFI variables here.
> 
> Signed-off-by: Alexander Graf 
> ---
>  arch/arm/cpu/armv8/u-boot.lds |  8 ++
>  arch/arm/cpu/u-boot.lds   | 13 ++
>  arch/arm/lib/sections.c   |  2 ++
>  include/efi_loader.h  |  3 +++
>  lib/efi_loader/efi_runtime.c  | 59 
> +++
>  5 files changed, 85 insertions(+)
>  create mode 100644 lib/efi_loader/efi_runtime.c
> 
> diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> index 4c1..7c5b032 100644
> --- a/arch/arm/cpu/armv8/u-boot.lds
> +++ b/arch/arm/cpu/armv8/u-boot.lds
> @@ -42,6 +42,14 @@ SECTIONS
>  
>   . = ALIGN(8);
>  
> + .efi_runtime : {
> +__efi_runtime_start = .;
> + *(efi_runtime)
> +__efi_runtime_stop = .;
> + }
> +
> + . = ALIGN(8);
> +
>   .image_copy_end :
>   {
>   *(.__image_copy_end)
> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> index d48a905..b5198d0 100644
> --- a/arch/arm/cpu/u-boot.lds
> +++ b/arch/arm/cpu/u-boot.lds
> @@ -89,6 +89,19 @@ SECTIONS
>  
>   . = ALIGN(4);
>  
> + .__efi_runtime_start : {
> + *(.__efi_runtime_start)
> + }
> +
> + .efi_runtime : {
> + *(efi_runtime)
> + }
> +
> + .__efi_runtime_stop : {
> + *(.__efi_runtime_stop)
> + }
> + . = ALIGN(4);
> +
>   .image_copy_end :
>   {
>   *(.__image_copy_end)
> diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> index a1205c3..21b3066 100644
> --- a/arch/arm/lib/sections.c
> +++ b/arch/arm/lib/sections.c
> @@ -27,4 +27,6 @@ char __rel_dyn_start[0] 
> __attribute__((section(".__rel_dyn_start")));
>  char __rel_dyn_end[0] __attribute__((section(".__rel_dyn_end")));
>  char __secure_start[0] __attribute__((section(".__secure_start")));
>  char __secure_end[0] __attribute__((section(".__secure_end")));
> +char __efi_runtime_start[0] __attribute__((section(".__efi_runtime_start")));
> +char __efi_runtime_stop[0] __attribute__((section(".__efi_runtime_stop")));
>  char _end[0] __attribute__((section(".__end")));
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 7fb2106..af1c88f 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -39,6 +39,7 @@
>  
>  #define EFI_EXIT(ret) efi_exit_func(ret);
>  
> +extern const struct efi_runtime_services efi_runtime_services;
>  extern struct efi_system_table systab;
>  
>  extern const struct efi_simple_text_output_protocol efi_con_out;
> @@ -49,6 +50,8 @@ extern const efi_guid_t efi_guid_console_control;
>  extern const efi_guid_t efi_guid_device_path;
>  extern const efi_guid_t efi_guid_loaded_image;
>  
> +extern unsigned int __efi_runtime_start, __efi_runtime_stop;
> +
>  struct efi_class_map {
>   const efi_guid_t *guid;
>   const void *interface;
> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> new file mode 100644
> index 000..214e1f5
> --- /dev/null
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -0,0 +1,59 @@
> +/*
> + *  EFI application runtime services
> + *
> + *  Copyright (c) 2015 Alexander Graf
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> + *
> + *  SPDX-License-Identifier: LGPL-2.1+
> + */
> +
> +#include 
> +#include 
> +
> +/*
> + * EFI Runtime code is still alive when U-Boot is long overwritten. To 
> isolate
> + * this code from the rest, we put it into a special section.
> + *
> + *!!WARNING!!
> + *
> + * This means that we can not rely on any code outside of this file at 
> runtime.
> + * Please keep it fully self-contained.
> + */
> +asm(".section efi_runtime,\"a\"");
> +
> +static efi_status_t 

Re: [U-Boot] config: enable CONFIG_API in distro config

[Leif Lindholm]

On Fri, Apr 18, 2014 at 05:54:51PM -0500, Rob Herring wrote:
  a) It was excruciatingly slow, to the point of not being useful.
 
  b) The binary had hard-coded memory layout, and hence couldn't be
  generic across multiple boards, since SoCs put their RAM in different
  places, boards have differing amounts of RAM, etc. This kinda defeats
  the whole purpose, since distros would need to install board-specific
  Grub binaries. Perhaps this could be solved similarly to the kernel's
  CONFIG_ARM_PATCH_PHYS_VIRT?
 
 Uggg, That certainly sounds pointless to use.
 
 Leif, comments?

a)
As for speed, the u-boot grub used to run with caches disabled (default
mode for kernel image loaded by bootm). Vladimir got fed up just
before Christmas and changed that, so it's now fast even on the RPi..

b)
There is no real restriction on RAM size (other than you having enough),
but the link address is currently fixed at build time.
Optimally, the grub kernel should be position independent, but that
wasn't trivial, and by the time the basic port was upstream, Linaro
Enterprise Group had de-prioritised work on 32-bit ARM.

Ian Campbell wrote some neat patches for patching the (grub) kernel
link addres at grub-install time, which would be an improvement, but is
also quite invasive over several ports:
http://lists.gnu.org/archive/html/grub-devel/2013-12/msg00426.html

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] arm: U-Boot API - clang support broke ABI

[Leif Lindholm]

Hi Jeroen,

On Sat, Nov 23, 2013 at 12:20:54PM +0100, Jeroen Hofstee wrote:
 Commit fe1378a - ARM: use r9 for gd - broke the ABI for users of the
 U-Boot API on ARM. Users I am aware of are GRUB and the FreeBSD loader.
 Since I only spotted this on Saturday, this code is well and truly in
 the wild, so any users of the API will need to preserve both r8 and r9
 on syscalls for the foreseeable future (which is not the end of the
 world).
 
 After looking into it a bit better it seems the standalone applications
 don't use the api, but poke into u-boot directly to look for a
 separate jump-table in gd. I am still a bit surprised it works like that,
 but at least the api doesn't. (I sent a patch to fix them)

 The api issue is therefore only related to requirement that r9 needs
 to be preserved. The README states since the beginning of the git
 repro (2002-11-03) [1] that r8 _and_ r9 have special meaning in U-boot.

[1] missing.

Upon inspection: the top-level README also states that R10 can be used
as the stack limit, which is not descibed by the AAPCS. Nor is the use
of R11 as a frame pointer.

 Albert removed the need for the GOT pointer (r9) over time by limiting
 the supported relocation types, so the move from r8 to r9 was not
 expected to cause an ABI problem, since both were special register
 already. Anyway, I will have a look if it did break the ubldr.

http://svnweb.freebsd.org/base?view=revisionrevision=258527

 Sometime last year I proposed a change to make the API syscall entry
 and exit points responsible for maintaining full procedure-call standard
 compliance, although the patch I provided was on the hackier side of
 good (and probably wrong).
 
 After the switch to r9 the call on ARM is eabi compliant, with r9 being a
 platform register as allowed by the specs.

EABI compliant for a specific platform that defines its use as such,
yes. It's not a free-for-all: unless explicitly defined by the platform,
r9 is 'variable register 6', a callee-saved register. (And here is where
it all gets messy because U-Boot calls into the application, and then
further down that call stack the application calls into U-Boot.)

 Whether u-boot or the payload
 is responsible for preserving r9 / in general should preserve and restore
 its special registers in API calls is better answered by the maintainers.

There is only one agent involved in this equation which can always
know how U-Boot deviates from the base AAPCS, and that is U-Boot.
The most helpful thing it can do is to hide these deviations, which may
deviate in different ways from that of the default behaviour of the
toolchain used to build it based on which operating system that is
running under.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] arm: U-Boot API - clang support broke ABI

[Leif Lindholm]

Hi,

Commit fe1378a - ARM: use r9 for gd - broke the ABI for users of the
U-Boot API on ARM. Users I am aware of are GRUB and the FreeBSD loader.
Since I only spotted this on Saturday, this code is well and truly in
the wild, so any users of the API will need to preserve both r8 and r9
on syscalls for the foreseeable future (which is not the end of the
world).

Sometime last year I proposed a change to make the API syscall entry
and exit points responsible for maintaining full procedure-call standard
compliance, although the patch I provided was on the hackier side of
good (and probably wrong).

I would now like to ask in a more open-ended way - is there any chance
that we could solve this in a generic way, such that consumers of the
API need not be aware of U-Boot internal ABI changes?

If the assumtions that the global data pointer is:
a) a pointer
b) writable
are not both safe, then this could be achieved by arch-specific asm
entry/exit points.

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Save/restore global data pointer on API boundary

[Leif Lindholm]

On 08/07/12 19:30, Wolfgang Denk wrote:

Most architectures keep the global data pointer (gd) in a register.


This may, or may not be.  You should not make any assumptions on how
gd is implemented.


The comment did, the code didn't, as long as gd was a pointer (which
should be a safe assumption). But that's irrelevant as I seem to have
gotten the wrong end of the stick.


When using the external app API, because they are calling us rather
than we calling them, this register can be corrupted.


How can this be?  The caller should always use the same register
convention as we do - otherwise we are in a much deeper trouble.
It is up to the caller to make sure it uses the published API (resp.
ABI).


Hmm, ok - this was not clear to me from the docs and example.

Indeed, the example does not reserve r8 (on ARM) or r2 (on PPC). Nor
does it save/restore it on entry/syscall. I don't know the exact ABI
semantics of r2 for PPC, but on ARM this is an error.

This could be worked around by something like:

diff --git a/examples/api/crt0.S b/examples/api/crt0.S
index 6daf127..5f956e4 100644
--- a/examples/api/crt0.S
+++ b/examples/api/crt0.S
@@ -49,13 +49,21 @@ syscall:
 _start:
ldr ip, =search_hint
str sp, [ip]
+   ldr ip, =gd_backup
+   str r8, [ip]
b   main


.globl syscall
 syscall:
+   push{r6-r8, lr}
+   ldr r6, =gd_backup
+   ldr r8, [r6]
ldr ip, =syscall_ptr
+   mov lr, pc
ldr pc, [ip]
+   str r8, [r6]
+   pop {r6-r8, pc}

 #else
 #error No support for this arch!
@@ -69,3 +77,7 @@ syscall_ptr:
.globl search_hint
 search_hint:
.long   0
+
+   .globl gd_backup
+gd_backup:
+   .long   0


An alternative would be to mandate DECLARE_GLOBAL_DATA_PTR for all API
applications, but I'd frankly prefer not providing direct access to gd.

Best Regards,

Leif

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] Save/restore global data pointer on API boundary

[Leif Lindholm]

Most architectures keep the global data pointer (gd) in a register.
When using the external app API, because they are calling us rather
than we calling them, this register can be corrupted.

The attached (trivial) patch saves the gd pointer at api_init(),
and restores it on every entry to syscall(). This may want to be
put behind an ifdef for those architectures that don't use a
dedicated register.

Signed-off-by: Leif Lindholm leif.lindh...@arm.com
---
diff --git a/api/api.c b/api/api.c
index a3bf60a..b911270 100644
--- a/api/api.c
+++ b/api/api.c
@@ -33,6 +33,8 @@

 #include api_private.h

+DECLARE_GLOBAL_DATA_PTR;
+
 #define DEBUG
 #undef DEBUG

@@ -600,6 +602,13 @@ static int API_display_clear(va_list ap)
 static cfp_t calls_table[API_MAXCALL] = { NULL, };

 /*
+ * The global data pointer is held in a register on most if not all
+ * architectures. Its value is not retained across the API boundary,
+ * so must be manually restored.
+ */
+static void volatile *gd_backup;
+
+/*
  * The main syscall entry point - this is not reentrant, only one call is
  * serviced until finished.
  *
@@ -620,6 +629,8 @@ int syscall(int call, int *retval, ...)
va_list ap;
int rv;

+   gd = gd_backup;
+
if (call  0 || call = calls_no) {
debugf(invalid call #%d\n, call);
return 0;
@@ -686,6 +697,7 @@ void api_init(void)
sig-checksum = crc32(0, (unsigned char *)sig,
  sizeof(struct api_signature));
debugf(syscall entry: 0x%08x\n, sig-syscall);
+   gd_backup = gd;
 }

 void platform_set_mr(struct sys_info *si, unsigned long start, unsigned long 
size,

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot