Re: [Linaro-uefi] [PATCH V3 15/15] Add ARM EFI boot support

On Sun, 2014-09-07 at 20:54 -0700, Roy Franz wrote:

@@ -618,6 +731,32 @@ ENTRY(lookup_processor_type) mov x0, #0 ret +ENTRY(efi_xen_start)

•    /*
  

•     * Turn off cache and MMU as XEN expects. EFI enables them, but also
  

It's just "Xen", not "XEN" (throughout).

•     * mandates a 1:1 (unity) VA->PA mapping, so we can turn off the
  

•     * MMU while executing EFI code before entering XEN.
  

•     * The EFI loader calls this to start XEN.
  

•     */
  

Last time there was a handy comment about preserving x0 here. I think it would also (or perhaps instead) be good to have a comment before the entry giving the "prototype" for this function, since it is called from C.

•    mov   x20, x0
  

•    bl    __flush_dcache_all
  

•    ic      ialluis
  

Two too many spaces before iall.

diff --git a/xen/common/efi/Makefile b/xen/common/efi/Makefile index 4313a4e..fea712e 100644 --- a/xen/common/efi/Makefile +++ b/xen/common/efi/Makefile @@ -1,5 +1,5 @@ CFLAGS += -fshort-wchar

+ifneq ($(XEN_TARGET_ARCH),arm64)

This does still build on arm32, right?

I suppose the discussion on patch #1 will have some knock on effects here.

diff --git a/xen/include/asm-arm/arm64/efibind.h b/xen/include/asm-arm/arm64/efibind.h new file mode 100644 index 0000000..2b0bf40 --- /dev/null +++ b/xen/include/asm-arm/arm64/efibind.h

diff --git a/xen/include/asm-arm/efi-boot.h b/xen/include/asm-arm/efi-boot.h new file mode 100644 index 0000000..91a637e --- /dev/null +++ b/xen/include/asm-arm/efi-boot.h

+static void __init *lookup_fdt_config_table(EFI_SYSTEM_TABLE *sys_table) +{

• const EFI_GUID fdt_guid = DEVICE_TREE_GUID;
• EFI_CONFIGURATION_TABLE *tables;
• void *fdt = NULL;
• int i;
• tables = sys_table->ConfigurationTable;
• for ( i = 0; i < sys_table->NumberOfTableEntries; i++ )
• {

•    if ( match_guid(&tables[i].VendorGuid, &fdt_guid) )
  

•    {
  

•        fdt = tables[i].VendorTable;
  

•        break;
  

•    }
  

• }
• return fdt;

+} +static EFI_STATUS __init efi_get_memory_map(void **map,

Missing a blank line after the previous function, here and elsewhere. I see you've gone from 2 blanks to 0 since last time ;-)

• fdt_val64 = cpu_to_fdt64((u64)(unsigned long)sys_table);

(uintptr_t) is arguably more correct than (unsigned long) for this purpose. and again below.

• status = fdt_setprop(fdt, node, "linux,uefi-system-table",

•                     &fdt_val64, sizeof(fdt_val64));
  

• if ( status )

•    goto fdt_set_fail;
  

• fdt_val64 = cpu_to_fdt64((u64)(unsigned long)memory_map);
• status = fdt_setprop(fdt, node, "linux,uefi-mmap-start",

•                     &fdt_val64,  sizeof(fdt_val64));
  

[...]

++static void __init efi_arch_post_exit_boot(void) +{

• efi_xen_start((unsigned long)fdt);

You might as well make the pseudoprototype be a pointer if that is what fdt actually is.

+static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image) +{

• if ( (unsigned long)loaded_image->ImageBase & ((1 << 20) - 1) )

•    blexit(L"Xen must be loaded at a 2MByte boundary.");
  

This isn't true any more, Xen only needs to be at a 4K boundary now.

Looking good, all of the above is pretty minor stuff, thanks!

Ian.