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

[Roy Franz <roy.franz@linaro.org>]

On Mon, Sep 8, 2014 at 9:12 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 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).

OK.
>
>> +         * 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.

OK, I'll add that.
>
>> +        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.

Most of the stuff should be common with arm32, although I haven't
tried it.  The main thing
missing for arm32 is the PE/COFF header.  I'm not intending to build
the EFI stuff for arm32, but
it's possible I've borked the build - I forgot to try that on this patchset.


>
>> 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 ;-)

I'll add the proper number of blank lines :)

>
>> +    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.

Yup, I'll fix that.
>
>> +    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.
OK.  That will save a cast, and the value in the register is the same.
>
>> +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.
I will update this (along with the alignment requested in the PE/COFF
header) and give it a spin.

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

I'm hoping this is in good enough shape for 4.5 - I should be able to
address all the feedback received so far quite quickly.
Should I wait for more feedback, or prepare an updated patchset
addressing the feedback so far?

Thanks,
Roy