Re: [Xen-devel] [PATCH V3 15/15] Add ARM EFI boot support

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel