Message-Id: <20131205200054.ED87FC40785@trevor.secretlab.ca>
Date:	Thu, 05 Dec 2013 20:00:54 +0000
From:	Grant Likely <grant.likely@...aro.org>
To:	Roy Franz <roy.franz@...aro.org>, linux-kernel@...r.kernel.org,
	linux-efi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	matt.fleming@...el.com, linux@....linux.org.uk
Cc:	leif.lindholm@...aro.org, dave.martin@....com, msalter@...hat.com,
	patches@...aro.org, Roy Franz <roy.franz@...aro.org>
Subject: Re: [PATCH V5 4/6] Add EFI stub for ARM

On Wed, 27 Nov 2013 15:31:53 -0800, Roy Franz <roy.franz@...aro.org> wrote:
> This patch adds EFI stub support for the ARM Linux kernel.  The EFI stub
> operates similarly to the x86 stub: it is a shim between the EFI firmware
> and the normal zImage entry point, and sets up the environment that the
> zImage is expecting.  This includes loading the initrd (optionaly) and
> device tree from the system partition based on the kernel command line.
> The stub updates the device tree as necessary, adding entries for EFI
> runtime services. The PE/COFF "MZ" header at offset 0 results in the
> first instruction being an add that corrupts r5, which is not used by
> the zImage interface.
> 
> Signed-off-by: Roy Franz <roy.franz@...aro.org>

I'm going to review this patch side-by-side with the ARM64 patch written
by Mark. There are things that can still be consolidated. Otherwise the
patch looks good.

> ---
> diff --git a/arch/arm/boot/compressed/efi-stub.c b/arch/arm/boot/compressed/efi-stub.c
> new file mode 100644
> index 0000000..2bd559d
> --- /dev/null
> +++ b/arch/arm/boot/compressed/efi-stub.c
> @@ -0,0 +1,291 @@
> +/*
> + * linux/arch/arm/boot/compressed/efi-stub.c
> + *
> + * Copyright (C) 2013 Linaro Ltd;  <roy.franz@...aro.org>
> + *
> + * This file implements the EFI boot stub for the ARM kernel
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +#include <linux/efi.h>
> +#include <libfdt.h>
> +#include "efi-stub.h"
> +
> +/* EFI function call wrappers.  These are not required for
> + * ARM, but wrappers are required for X86 to convert between
> + * ABIs.  These wrappers are provided to allow code sharing
> + * between X86 and ARM.  Since these wrappers directly invoke the
> + * EFI function pointer, the function pointer type must be properly
> + * defined, which is not the case for X86  One advantage of this is
> + * it allows for type checking of arguments, which is not
> + * possible with the X86 wrappers.
> + */
> +#define efi_call_phys0(f)			f()
> +#define efi_call_phys1(f, a1)			f(a1)
> +#define efi_call_phys2(f, a1, a2)		f(a1, a2)
> +#define efi_call_phys3(f, a1, a2, a3)		f(a1, a2, a3)
> +#define efi_call_phys4(f, a1, a2, a3, a4)	f(a1, a2, a3, a4)
> +#define efi_call_phys5(f, a1, a2, a3, a4, a5)	f(a1, a2, a3, a4, a5)

Identical to ARM64. I would put this into a common header usable by
platforms that can call the functions directly.

> +
> +/* The maximum uncompressed kernel size is 32 MBytes, so we will reserve
> + * that for the decompressed kernel.  We have no easy way to tell what
> + * the actuall size of code + data the uncompressed kernel will use.
> + */
> +#define MAX_UNCOMP_KERNEL_SIZE	0x02000000
> +
> +/* The kernel zImage should be located between 32 Mbytes
> + * and 128 MBytes from the base of DRAM.  The min
> + * address leaves space for a maximal size uncompressed image,
> + * and the max address is due to how the zImage decompressor
> + * picks a destination address.
> + */
> +#define ZIMAGE_OFFSET_LIMIT	0x08000000
> +#define MIN_ZIMAGE_OFFSET	MAX_UNCOMP_KERNEL_SIZE
> +
> +#define PRINTK_PREFIX		"EFIstub: "
> +
> +struct fdt_region {
> +	u64 base;
> +	u64 size;
> +};

Identical to ARM64; move to header.

> +
> +
> +/* Include shared EFI stub code, and required headers. */
> +#include "../../../../include/generated/compile.h"
> +#include "../../../../include/generated/utsrelease.h"
> +#include "../../../../drivers/firmware/efi/efi-stub-helper.c"
> +#include "../../../../drivers/firmware/efi/fdt.c"
> +
> +
> +int efi_entry(void *handle, efi_system_table_t *sys_table,
> +	      unsigned long *zimage_addr)
> +{
> +	efi_loaded_image_t *image;
> +	int status;
> +	unsigned long nr_pages;
> +	const struct fdt_region *region;
> +
> +	void *fdt;
> +	int err;
> +	int node;
> +	unsigned long zimage_size = 0;
> +	unsigned long dram_base;
> +	/* addr/point and size pairs for memory management*/
> +	unsigned long initrd_addr;
> +	unsigned long initrd_size = 0;
> +	unsigned long fdt_addr;
> +	unsigned long fdt_size = 0;
> +	efi_physical_addr_t kernel_reserve_addr;
> +	unsigned long kernel_reserve_size = 0;
> +	char *cmdline_ptr;
> +	int cmdline_size = 0;
> +
> +	unsigned long map_size, desc_size;
> +	u32 desc_ver;
> +	unsigned long mmap_key;
> +	efi_memory_desc_t *memory_map;
> +
> +	unsigned long new_fdt_size;
> +	unsigned long new_fdt_addr;
> +
> +	efi_guid_t proto = LOADED_IMAGE_PROTOCOL_GUID;
> +
> +	/* Check if we were booted by the EFI firmware */
> +	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
> +		goto fail;
> +
> +	efi_printk(sys_table, PRINTK_PREFIX"Booting Linux using EFI stub.\n");

Gratuitously different between ARM and ARM64. efi_printk() vs. pr_efi()
and the string is different.

> +	/* get the command line from EFI, using the LOADED_IMAGE protocol */
> +	status = efi_call_phys3(sys_table->boottime->handle_protocol,
> +				handle, &proto, (void *)&image);
> +	if (status != EFI_SUCCESS) {
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Failed to get handle for LOADED_IMAGE_PROTOCOL.\n");
> +		goto fail;
> +	}
> +
> +	/* We are going to copy the command line into the device tree,
> +	 * so this memory just needs to not conflict with boot protocol
> +	 * requirements.
> +	 */
> +	cmdline_ptr = efi_convert_cmdline_to_ascii(sys_table, image,
> +						   &cmdline_size);
> +	if (!cmdline_ptr) {
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Unable to allocate memory for command line.\n");
> +		goto fail;
> +	}
> +
> +	/* We first load the device tree, as we need to get the base address of
> +	 * DRAM from the device tree.  The zImage, device tree, and initrd
> +	 * have address restrictions that are relative to the base of DRAM.
> +	 */
> +	status = handle_cmdline_files(sys_table, image, cmdline_ptr, "dtb=",
> +				      0xffffffff, &fdt_addr, &fdt_size);
> +	if (status != EFI_SUCCESS) {
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Unable to load device tree blob.\n");
> +		goto fail_free_cmdline;
> +	}
> +
> +	err = fdt_check_header((void *)fdt_addr);
> +	if (err != 0) {
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Device tree header not valid.\n");
> +		goto fail_free_fdt;
> +	}
> +	if (fdt_totalsize((void *)fdt_addr) > fdt_size) {
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Incomplete device tree.\n");
> +		goto fail_free_fdt;
> +
> +	}

All of the above is basically identical.

> +	/* Look up the base of DRAM from the device tree. */
> +	fdt = (void *)fdt_addr;
> +	node = fdt_subnode_offset(fdt, 0, "memory");
> +	region = fdt_getprop(fdt, node, "reg", NULL);
> +	if (region) {
> +		dram_base = fdt64_to_cpu(region->base);
> +	} else {
> +		/* There is no way to get amount or addresses of physical
> +		 * memory installed using EFI calls.  If the device tree
> +		 * we read from disk doesn't have this, there is no way
> +		 * for us to construct this informaion.
> +		 */
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: No 'memory' node in device tree.\n");
> +		goto fail_free_fdt;
> +	}

ARM and ARM64 differ here. ARM64 uses get_dram_base(), this open codes
some stuff.

> +
> +	/* Reserve memory for the uncompressed kernel image. This is
> +	 * all that prevents any future allocations from conflicting
> +	 * with the kernel.  Since we can't tell from the compressed
> +	 * image how much DRAM the kernel actually uses (due to BSS
> +	 * size uncertainty) we allocate the maximum possible size.
> +	 */
> +	kernel_reserve_addr = dram_base;
> +	kernel_reserve_size = MAX_UNCOMP_KERNEL_SIZE;
> +	nr_pages = round_up(kernel_reserve_size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE;
> +	status = efi_call_phys4(sys_table->boottime->allocate_pages,
> +				EFI_ALLOCATE_ADDRESS, EFI_LOADER_DATA,
> +				nr_pages, &kernel_reserve_addr);
> +	if (status != EFI_SUCCESS) {
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Unable to allocate memory for uncompressed kernel.\n");
> +		goto fail_free_fdt;
> +	}

This is ARM only since arm64 doesn't have a compressed version. It would
be possible to stub out.

> +
> +	/* Relocate the zImage, if required.  ARM doesn't have a
> +	 * preferred address, so we set it to 0, as we want to allocate
> +	 * as low in memory as possible.
> +	 */
> +	zimage_size = image->image_size;
> +	status = efi_relocate_kernel(sys_table, zimage_addr, zimage_size,
> +				     zimage_size, 0, 0);
> +	if (status != EFI_SUCCESS) {
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Failed to relocate kernel.\n");
> +		goto fail_free_kernel_reserve;
> +	}

Slight differences here.

> +
> +	/* Check to see if we were able to allocate memory low enough
> +	 * in memory.
> +	 */
> +	if (*zimage_addr + zimage_size > dram_base + ZIMAGE_OFFSET_LIMIT) {
> +		efi_printk(sys_table, PRINTK_PREFIX"ERROR: Failed to relocate kernel, no low memory available.\n");
> +		goto fail_free_zimage;
> +	}

Back to same code below this point, and is the same through to the
bottom of the function. That is a lot of identical code. I think you can
generalize the whole function with arch specific callouts in a couple of
places.

I really don't want to hold up the merging of this series. Aside from
the duplication I think it is ready to be merged. However, the
duplication is so obvious that I'm not comfortable with it. If I were
an ARM or ARM64 core maintainer then I would push back on it.

Go ahead an add my Acked-by for this patch. I'll support merging it as
is provided that you promise me to merge the two versions with a
follow-up patch ASAP. However, if you can I would still recommend
respinning the series with the common bits split out right away since
there's a much greater chance of getting the arm and arm64 maintainers
to accept them.

Acked-by: Grant Likely <grant.likely@...aro.org>

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists