[Xen-devel,RFC,19/19] Add EFI stub for ARM64

This patch adds the real EFI application entry point in head.S,
and adds the efi.c file that will contain the EFI stub implementation.
The assembly wrapper is responsible for returning the processor state
to what XEN expects when starting.  The EFI stub processes the
XEN EFI configuration file to load the dom0 kernel, ramdisk, etc and
constructs a device tree for XEN to use, then transfers control to
the normal XEN image entry point.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/arm/Makefile     |   2 +
 xen/arch/arm/arm64/head.S |  62 ++++-
 xen/arch/arm/efi-shared.c |   1 +
 xen/arch/arm/efi.c        | 686 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 750 insertions(+), 1 deletion(-)
 create mode 120000 xen/arch/arm/efi-shared.c
 create mode 100644 xen/arch/arm/efi.c

On Sat, 2014-06-28 at 02:25 +0100, Roy Franz wrote:
> This patch adds the real EFI application entry point in head.S,
> and adds the efi.c file that will contain the EFI stub implementation.
> The assembly wrapper is responsible for returning the processor state
> to what XEN expects when starting.  The EFI stub processes the
> XEN EFI configuration file to load the dom0 kernel, ramdisk, etc and
> constructs a device tree for XEN to use, then transfers control to
> the normal XEN image entry point.
> 
> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> ---
>  xen/arch/arm/Makefile     |   2 +
>  xen/arch/arm/arm64/head.S |  62 ++++-
>  xen/arch/arm/efi-shared.c |   1 +
>  xen/arch/arm/efi.c        | 686 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 750 insertions(+), 1 deletion(-)
>  create mode 120000 xen/arch/arm/efi-shared.c
>  create mode 100644 xen/arch/arm/efi.c
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 63e0460..c3ed74f 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -33,6 +33,8 @@ obj-y += hvm.o
>  obj-y += device.o
>  obj-y += decode.o
>  obj-y += processor.o
> +obj-y += efi.o
> +obj-y += efi-shared.o

While the latter will eventually be in xen/common/efi/something.c and
hence -fshort-char is taken care of I suppose efi.o needs it too?

Probably the easiest thing to do there is to create
xen/arch/arm/efi/stub.c and do the fshort-char trick for that entire
folder, since I don't think our build system makes it especially easy to
change CFLAGS for individual object files.

> +
> +	/*
> +	 * efi_entry() will return here with device tree address in x0.
> +	 *  Save value in register which is preserved by __flush_dcache_all.
> +	 */
> +	mov	x20, x0
> +
> +	/* Turn off Dcache and MMU */

AIUI EFI leaves us in a 1:1 mapping, hence this is safe to do.

> +	mrs	x0, CurrentEL
> +	cmp	x0, #PSR_MODE_EL2t
> +	ccmp	x0, #PSR_MODE_EL2h, #0x4, ne
> +	b.ne	1f
> +	mrs	x0, sctlr_el2
> +	bic	x0, x0, #1 << 0	// clear SCTLR.M
> +	bic	x0, x0, #1 << 2	// clear SCTLR.C
> +	msr	sctlr_el2, x0
> +	isb
> +	b	2f
> +1:
> +	mrs	x0, sctlr_el1

I hope there is no way to get here under Xen ;-) I suppose you do it
this way so that we reach the regular head.S which then prints all the
error messages etc?

> +	bic	x0, x0, #1 << 0	// clear SCTLR.M
> +	bic	x0, x0, #1 << 2	// clear SCTLR.C
> +	msr	sctlr_el1, x0
> +	isb
> +2:
> +/*	bl	__flush_dcache_all */  /* RFRANZ_TODO */

We just don't have this function right now, correct? I suppose you are
running on models without cache modelling turned on?

Eventually this will implement a total by set/way, right? That's safe
because we are uniprocessor at this point etc etc.

> +/*
> + * This define and the code associated with it is a temporary, and will
> + * be replaced by changes to the XEN kernel to use the EFI memory map
> + * that is passed in the device tree instead of the normal FDT memory map.
> + * The XEN kernel changes are largely indpendent of the stub, so this code

FWIW given this is totally temporary: "independent".

> + * has been added to allow review and some testing of the stub code while
> + * the XEN kernel code is being developed.
> + */
> +#define ADD_EFI_MEMORY_TO_FDT
> +
> +#define DEVICE_TREE_GUID \
> +{0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
> +
> +extern CHAR16 __initdata newline[];
> +extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
> +extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdERR;
> +extern EFI_BOOT_SERVICES *__initdata efi_bs;
> +
> +
> +static EFI_HANDLE __initdata efi_ih;
> +
> +static struct file __initdata cfg;
> +static struct file __initdata kernel;
> +static struct file __initdata ramdisk;
> +static struct file __initdata dtb;
> +
> +static unsigned long mmap_size;
> +static EFI_MEMORY_DESCRIPTOR *mmap_ptr;
> +
> +static void *new_fdt;
> +
> +#ifdef ADD_EFI_MEMORY_TO_FDT
> +static int IsLinuxReservedRegion(int MemoryType)

Linux? I guess this is a hangover from the Linux stub?

(Even there I'd have thought the regions were reserved by EFI not Linux
though, perhaps I'm looking at it backwards though)

> +{
> +    switch ( MemoryType )
> +    {
> +        case EfiMemoryMappedIO:
> +        case EfiMemoryMappedIOPortSpace:
> +        case EfiRuntimeServicesCode:
> +        case EfiRuntimeServicesData:
> +        case EfiUnusableMemory:
> +        case EfiACPIReclaimMemory:
> +        case EfiACPIMemoryNVS:
> +        case EfiLoaderData:
> +            return 1;
> +        default:
> +            return 0;
> +    }
> +}
> +
> +
> +static EFI_STATUS __init efi_process_memory_map(EFI_MEMORY_DESCRIPTOR *map,
> +                                                unsigned long mmap_size,
> +                                                unsigned long desc_size,
> +                                                void *fdt)
> +{
> +    int Index;
> +    int err;
> +
> +    EFI_MEMORY_DESCRIPTOR *desc_ptr = map;
> +
> +    /* Go through the list and add the reserved region to the Device Tree */
> +    for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
> +    {
> +        if ( IsLinuxReservedRegion(desc_ptr->Type) )
> +        {
> +            err = fdt_add_mem_rsv(fdt, desc_ptr->PhysicalStart, desc_ptr->NumberOfPages * EFI_PAGE_SIZE);
> +            if ( err != 0 )
> +                PrintStr(L"Warning: Fail to add 'memreserve'\n\n");
> +        }
> +        desc_ptr = NextMemoryDescriptor(desc_ptr, desc_size);
> +    }
> +
> +    return EFI_SUCCESS;
> +
> +}
> +#endif
> +
> +static EFI_STATUS __init efi_get_memory_map(EFI_SYSTEM_TABLE *sys_table_arg,
> +                                            EFI_MEMORY_DESCRIPTOR **map,
> +                                            unsigned long *mmap_size,
> +                                            unsigned long *desc_size,
> +                                            UINT32 *desc_ver,
> +                                            unsigned long *key_ptr)
> +{
> +    EFI_MEMORY_DESCRIPTOR *m = NULL;
> +    EFI_STATUS status;
> +    unsigned long key;
> +    u32 desc_version;
> +
> +    *mmap_size = sizeof(*m) * 32;
> +again:
> +    /*
> +     * Add an additional 2 efi_memory_desc_t because we're doing an
> +         * allocation which may be in a new descriptor region, and this
> +         * may create additional entries.
> +     */
> +    *mmap_size += 2 * sizeof(*m);
> +    status = sys_table_arg->BootServices->AllocatePool(EfiLoaderData,
> +                                                       *mmap_size, (void **)&m);
> +    if ( status != EFI_SUCCESS )
> +        goto fail;
> +
> +    *desc_size = 0;
> +    key = 0;
> +    status = sys_table_arg->BootServices->GetMemoryMap(mmap_size, m,
> +                                                       &key, desc_size, &desc_version);
> +    if ( status == EFI_BUFFER_TOO_SMALL )
> +    {
> +        sys_table_arg->BootServices->FreePool(m);
> +        goto again;
> +    }
> +
> +    if ( status != EFI_SUCCESS )
> +        sys_table_arg->BootServices->FreePool(m);
> +
> +    if ( key_ptr && status == EFI_SUCCESS )
> +        *key_ptr = key;
> +    if ( desc_ver && status == EFI_SUCCESS )
> +        *desc_ver = desc_version;
> +
> +fail:
> +    *map = m;

I think if status != EFI_SUCCESS m is a free'd pointer? Probably best
not to tempt the caller with such a thing on failure.

> +    return status;
> +}
> +
> +
> +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;
> +    int i;
> +
> +    tables = sys_table->ConfigurationTable;
> +    fdt = NULL;
> +
> +    for ( i = 0; i < sys_table->NumberOfTableEntries; i++ ) if ( match_guid(&tables[i].VendorGuid, &fdt_guid) == 0 )

I think this long line is the result of an accidentally omitted \n?

> +        {
> +            fdt = tables[i].VendorTable;
> +            break;
> +        }
> +    return fdt;
> +}
> +
> +/*
> + * Get (or set if not present) the #addr-cells and #size cells
> + * properties of the chose node.  We need to know these to properly

                             ^chosen

> + * construct the address ranges used to describe the files loaded
> + * by the stub.
> + */
> +static int setup_chosen_node(void *fdt, int *addr_cells, int *size_cells)

These bits aren't __init?

> +
> +/*
> + * Set a single 'reg' property taking into account the
> + * configured addr and size cell sizes.
> + */
> +static int fdt_set_reg(void *fdt, int node, int addr_cells, int size_cells,
> +                       uint64_t addr, uint64_t len)

FWIW I think we have such a helper already somewhere, but perhaps this
file with -fshort-char etc is unable to call it?

> +{
> +    uint8_t data[16]; /* at most 2 64 bit words */
> +    void *p = data;
> +
> +    /* Make sure that the values provided can be represented in
> +     * the reg property.
> +     */
> +    if (addr_cells == 1 && (addr >> 32))

Xen coding style puts spaces more often than you may be used to. In
particular inside the brackets of an if (and while etc). So here:
        if ( addr_cells == 1 && ( addr >> 32 ) )


> +
> +/*
> + * Add the FDT nodes for the standard EFI information, which consist
> + * of the System table address, the address of the final EFI memory map,
> + * and memory map information.
> + */
> +static EFI_STATUS fdt_add_uefi_nodes(EFI_SYSTEM_TABLE *sys_table, void *fdt,
> +                               EFI_MEMORY_DESCRIPTOR *memory_map,
> +                               unsigned long map_size, unsigned long desc_size,
> +                               u32 desc_ver)
> +{
> +    int node;
> +    int status;
> +    u32 fdt_val32;
> +    u64 fdt_val64;
> +
> +#ifndef ADD_EFI_MEMORY_TO_FDT
> +    int prev;
> +    /*
> +     * Delete any memory nodes present.  The EFI memory map is the only
> +     * memory description provided to XEN.
> +     */
> +    prev = 0;
> +    for (;;)
> +    {
> +        const char *type;
> +        int len;
> +
> +        node = fdt_next_node(fdt, prev, NULL);
> +        if (node < 0)
> +        break;
> +
> +        type = fdt_getprop(fdt, node, "device_type", &len);
> +        if (type && strncmp(type, "memory", len) == 0)
> +        {
> +            fdt_del_node(fdt, node);
> +            continue;
> +        }
> +
> +        prev = node;
> +    }
> +#endif
> +
> +    node = fdt_subnode_offset(fdt, 0, "chosen");
> +    if ( node < 0 )
> +    {
> +        node = fdt_add_subnode(fdt, 0, "chosen");
> +        if ( node < 0 )
> +        {
> +            status = node; /* node is error code when negative */
> +            goto fdt_set_fail;
> +        }
> +    }
> +
> +    /* Add FDT entries for EFI runtime services in chosen node. */
> +    node = fdt_subnode_offset(fdt, 0, "chosen");

You looked this up above (or created it), and with proper error
handling. I think you don't need to look again?

> [...]
> +}

Please can you include the emacs magic block you'll find in other files
at the end (and any other new files too)

On Wed, Jul 2, 2014 at 5:34 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Sat, 2014-06-28 at 02:25 +0100, Roy Franz wrote:
>> This patch adds the real EFI application entry point in head.S,
>> and adds the efi.c file that will contain the EFI stub implementation.
>> The assembly wrapper is responsible for returning the processor state
>> to what XEN expects when starting.  The EFI stub processes the
>> XEN EFI configuration file to load the dom0 kernel, ramdisk, etc and
>> constructs a device tree for XEN to use, then transfers control to
>> the normal XEN image entry point.
>>
>> Signed-off-by: Roy Franz <roy.franz@linaro.org>
>> ---
>>  xen/arch/arm/Makefile     |   2 +
>>  xen/arch/arm/arm64/head.S |  62 ++++-
>>  xen/arch/arm/efi-shared.c |   1 +
>>  xen/arch/arm/efi.c        | 686 ++++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 750 insertions(+), 1 deletion(-)
>>  create mode 120000 xen/arch/arm/efi-shared.c
>>  create mode 100644 xen/arch/arm/efi.c
>>
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 63e0460..c3ed74f 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -33,6 +33,8 @@ obj-y += hvm.o
>>  obj-y += device.o
>>  obj-y += decode.o
>>  obj-y += processor.o
>> +obj-y += efi.o
>> +obj-y += efi-shared.o
>
> While the latter will eventually be in xen/common/efi/something.c and
> hence -fshort-char is taken care of I suppose efi.o needs it too?
>
> Probably the easiest thing to do there is to create
> xen/arch/arm/efi/stub.c and do the fshort-char trick for that entire
> folder, since I don't think our build system makes it especially easy to
> change CFLAGS for individual object files.

Yes and yes.  I will likely end up with some single file directories due to the
build option differences.  Is there a XEN build system expert that I can ask
for help if needed?  (Or should I just post to the list?)

>
>> +
>> +     /*
>> +      * efi_entry() will return here with device tree address in x0.
>> +      *  Save value in register which is preserved by __flush_dcache_all.
>> +      */
>> +     mov     x20, x0
>> +
>> +     /* Turn off Dcache and MMU */
>
> AIUI EFI leaves us in a 1:1 mapping, hence this is safe to do.

Yes, EFI specifies a 1:1 mapping.  The assembly wrapper here is the same as we
use for Linux, so it still needs some tweaks.  Linux expects MMU/DCACHE off.

>
>> +     mrs     x0, CurrentEL
>> +     cmp     x0, #PSR_MODE_EL2t
>> +     ccmp    x0, #PSR_MODE_EL2h, #0x4, ne
>> +     b.ne    1f
>> +     mrs     x0, sctlr_el2
>> +     bic     x0, x0, #1 << 0 // clear SCTLR.M
>> +     bic     x0, x0, #1 << 2 // clear SCTLR.C
>> +     msr     sctlr_el2, x0
>> +     isb
>> +     b       2f
>> +1:
>> +     mrs     x0, sctlr_el1
>
> I hope there is no way to get here under Xen ;-) I suppose you do it
> this way so that we reach the regular head.S which then prints all the
> error messages etc?

I'll take a closer look at this.

>
>> +     bic     x0, x0, #1 << 0 // clear SCTLR.M
>> +     bic     x0, x0, #1 << 2 // clear SCTLR.C
>> +     msr     sctlr_el1, x0
>> +     isb
>> +2:
>> +/*   bl      __flush_dcache_all */  /* RFRANZ_TODO */
>
> We just don't have this function right now, correct? I suppose you are
> running on models without cache modelling turned on?
>
> Eventually this will implement a total by set/way, right? That's safe
> because we are uniprocessor at this point etc etc.

This works on the model since I'm not modeling the cache, but we do need it
if we are going to turn the dcache off.  If we can leave the dcache enabled
for XEN then I don't think we need to flush it here.

>
>> +/*
>> + * This define and the code associated with it is a temporary, and will
>> + * be replaced by changes to the XEN kernel to use the EFI memory map
>> + * that is passed in the device tree instead of the normal FDT memory map.
>> + * The XEN kernel changes are largely indpendent of the stub, so this code
>
> FWIW given this is totally temporary: "independent".

They will become very dependent once XEN uses the EFI memory map instead of
the FDT memory map.  The code protected with ADD_EFI_MEMORY_TO_FDT is all
a temporary crutch to allow testing/development of the stub before the changes
to the XEN kernel to use the EFI memory map directly are made.

>
>> + * has been added to allow review and some testing of the stub code while
>> + * the XEN kernel code is being developed.
>> + */
>> +#define ADD_EFI_MEMORY_TO_FDT
>> +
>> +#define DEVICE_TREE_GUID \
>> +{0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
>> +
>> +extern CHAR16 __initdata newline[];
>> +extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
>> +extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdERR;
>> +extern EFI_BOOT_SERVICES *__initdata efi_bs;
>> +
>> +
>> +static EFI_HANDLE __initdata efi_ih;
>> +
>> +static struct file __initdata cfg;
>> +static struct file __initdata kernel;
>> +static struct file __initdata ramdisk;
>> +static struct file __initdata dtb;
>> +
>> +static unsigned long mmap_size;
>> +static EFI_MEMORY_DESCRIPTOR *mmap_ptr;
>> +
>> +static void *new_fdt;
>> +
>> +#ifdef ADD_EFI_MEMORY_TO_FDT
>> +static int IsLinuxReservedRegion(int MemoryType)
>
> Linux? I guess this is a hangover from the Linux stub?
>
> (Even there I'd have thought the regions were reserved by EFI not Linux
> though, perhaps I'm looking at it backwards though)

Since this is part of a temporary hack, I didn't rename anything :)
All this will go away.

>
>> +{
>> +    switch ( MemoryType )
>> +    {
>> +        case EfiMemoryMappedIO:
>> +        case EfiMemoryMappedIOPortSpace:
>> +        case EfiRuntimeServicesCode:
>> +        case EfiRuntimeServicesData:
>> +        case EfiUnusableMemory:
>> +        case EfiACPIReclaimMemory:
>> +        case EfiACPIMemoryNVS:
>> +        case EfiLoaderData:
>> +            return 1;
>> +        default:
>> +            return 0;
>> +    }
>> +}
>> +
>> +
>> +static EFI_STATUS __init efi_process_memory_map(EFI_MEMORY_DESCRIPTOR *map,
>> +                                                unsigned long mmap_size,
>> +                                                unsigned long desc_size,
>> +                                                void *fdt)
>> +{
>> +    int Index;
>> +    int err;
>> +
>> +    EFI_MEMORY_DESCRIPTOR *desc_ptr = map;
>> +
>> +    /* Go through the list and add the reserved region to the Device Tree */
>> +    for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
>> +    {
>> +        if ( IsLinuxReservedRegion(desc_ptr->Type) )
>> +        {
>> +            err = fdt_add_mem_rsv(fdt, desc_ptr->PhysicalStart, desc_ptr->NumberOfPages * EFI_PAGE_SIZE);
>> +            if ( err != 0 )
>> +                PrintStr(L"Warning: Fail to add 'memreserve'\n\n");
>> +        }
>> +        desc_ptr = NextMemoryDescriptor(desc_ptr, desc_size);
>> +    }
>> +
>> +    return EFI_SUCCESS;
>> +
>> +}
>> +#endif
>> +
>> +static EFI_STATUS __init efi_get_memory_map(EFI_SYSTEM_TABLE *sys_table_arg,
>> +                                            EFI_MEMORY_DESCRIPTOR **map,
>> +                                            unsigned long *mmap_size,
>> +                                            unsigned long *desc_size,
>> +                                            UINT32 *desc_ver,
>> +                                            unsigned long *key_ptr)
>> +{
>> +    EFI_MEMORY_DESCRIPTOR *m = NULL;
>> +    EFI_STATUS status;
>> +    unsigned long key;
>> +    u32 desc_version;
>> +
>> +    *mmap_size = sizeof(*m) * 32;
>> +again:
>> +    /*
>> +     * Add an additional 2 efi_memory_desc_t because we're doing an
>> +         * allocation which may be in a new descriptor region, and this
>> +         * may create additional entries.
>> +     */
>> +    *mmap_size += 2 * sizeof(*m);
>> +    status = sys_table_arg->BootServices->AllocatePool(EfiLoaderData,
>> +                                                       *mmap_size, (void **)&m);
>> +    if ( status != EFI_SUCCESS )
>> +        goto fail;
>> +
>> +    *desc_size = 0;
>> +    key = 0;
>> +    status = sys_table_arg->BootServices->GetMemoryMap(mmap_size, m,
>> +                                                       &key, desc_size, &desc_version);
>> +    if ( status == EFI_BUFFER_TOO_SMALL )
>> +    {
>> +        sys_table_arg->BootServices->FreePool(m);
>> +        goto again;
>> +    }
>> +
>> +    if ( status != EFI_SUCCESS )
>> +        sys_table_arg->BootServices->FreePool(m);
>> +
>> +    if ( key_ptr && status == EFI_SUCCESS )
>> +        *key_ptr = key;
>> +    if ( desc_ver && status == EFI_SUCCESS )
>> +        *desc_ver = desc_version;
>> +
>> +fail:
>> +    *map = m;
>
> I think if status != EFI_SUCCESS m is a free'd pointer? Probably best
> not to tempt the caller with such a thing on failure.

Yeah, a freed pointer, or an undefined one if AllocatePool fails.  I'll update
this to set m to NULL on failure.

>
>> +    return status;
>> +}
>> +
>> +
>> +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;
>> +    int i;
>> +
>> +    tables = sys_table->ConfigurationTable;
>> +    fdt = NULL;
>> +
>> +    for ( i = 0; i < sys_table->NumberOfTableEntries; i++ ) if ( match_guid(&tables[i].VendorGuid, &fdt_guid) == 0 )
>
> I think this long line is the result of an accidentally omitted \n?
>
>> +        {
>> +            fdt = tables[i].VendorTable;
>> +            break;
>> +        }
>> +    return fdt;
>> +}
>> +
>> +/*
>> + * Get (or set if not present) the #addr-cells and #size cells
>> + * properties of the chose node.  We need to know these to properly
>
>                              ^chosen
>
>> + * construct the address ranges used to describe the files loaded
>> + * by the stub.
>> + */
>> +static int setup_chosen_node(void *fdt, int *addr_cells, int *size_cells)
>
> These bits aren't __init?

They should be...
>
>> +
>> +/*
>> + * Set a single 'reg' property taking into account the
>> + * configured addr and size cell sizes.
>> + */
>> +static int fdt_set_reg(void *fdt, int node, int addr_cells, int size_cells,
>> +                       uint64_t addr, uint64_t len)
>
> FWIW I think we have such a helper already somewhere, but perhaps this
> file with -fshort-char etc is unable to call it?

I'll try to find it.  I think it should be callable, since wide
characters aren't used
within this function of in its interface.

>
>> +{
>> +    uint8_t data[16]; /* at most 2 64 bit words */
>> +    void *p = data;
>> +
>> +    /* Make sure that the values provided can be represented in
>> +     * the reg property.
>> +     */
>> +    if (addr_cells == 1 && (addr >> 32))
>
> Xen coding style puts spaces more often than you may be used to. In
> particular inside the brackets of an if (and while etc). So here:
>         if ( addr_cells == 1 && ( addr >> 32 ) )
>
I tried to be good about that :)  I was hoping for a checkpatch script
to catch all
that kind of stuff for me :)
I'll review this and the use of tabs again.

>
>> +
>> +/*
>> + * Add the FDT nodes for the standard EFI information, which consist
>> + * of the System table address, the address of the final EFI memory map,
>> + * and memory map information.
>> + */
>> +static EFI_STATUS fdt_add_uefi_nodes(EFI_SYSTEM_TABLE *sys_table, void *fdt,
>> +                               EFI_MEMORY_DESCRIPTOR *memory_map,
>> +                               unsigned long map_size, unsigned long desc_size,
>> +                               u32 desc_ver)
>> +{
>> +    int node;
>> +    int status;
>> +    u32 fdt_val32;
>> +    u64 fdt_val64;
>> +
>> +#ifndef ADD_EFI_MEMORY_TO_FDT
>> +    int prev;
>> +    /*
>> +     * Delete any memory nodes present.  The EFI memory map is the only
>> +     * memory description provided to XEN.
>> +     */
>> +    prev = 0;
>> +    for (;;)
>> +    {
>> +        const char *type;
>> +        int len;
>> +
>> +        node = fdt_next_node(fdt, prev, NULL);
>> +        if (node < 0)
>> +        break;
>> +
>> +        type = fdt_getprop(fdt, node, "device_type", &len);
>> +        if (type && strncmp(type, "memory", len) == 0)
>> +        {
>> +            fdt_del_node(fdt, node);
>> +            continue;
>> +        }
>> +
>> +        prev = node;
>> +    }
>> +#endif
>> +
>> +    node = fdt_subnode_offset(fdt, 0, "chosen");
>> +    if ( node < 0 )
>> +    {
>> +        node = fdt_add_subnode(fdt, 0, "chosen");
>> +        if ( node < 0 )
>> +        {
>> +            status = node; /* node is error code when negative */
>> +            goto fdt_set_fail;
>> +        }
>> +    }
>> +
>> +    /* Add FDT entries for EFI runtime services in chosen node. */
>> +    node = fdt_subnode_offset(fdt, 0, "chosen");
>
> You looked this up above (or created it), and with proper error
> handling. I think you don't need to look again?

That does appear to be redundant.
>
>> [...]
>> +}
>
> Please can you include the emacs magic block you'll find in other files
> at the end (and any other new files too)
>
I will add this..

Thanks Ian, Stefano and Jan for all your feedback.

Roy

On Wed, 2014-07-09 at 12:15 -0700, Roy Franz wrote:
> On Wed, Jul 2, 2014 at 5:34 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Sat, 2014-06-28 at 02:25 +0100, Roy Franz wrote:
> >> This patch adds the real EFI application entry point in head.S,
> >> and adds the efi.c file that will contain the EFI stub implementation.
> >> The assembly wrapper is responsible for returning the processor state
> >> to what XEN expects when starting.  The EFI stub processes the
> >> XEN EFI configuration file to load the dom0 kernel, ramdisk, etc and
> >> constructs a device tree for XEN to use, then transfers control to
> >> the normal XEN image entry point.
> >>
> >> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> >> ---
> >>  xen/arch/arm/Makefile     |   2 +
> >>  xen/arch/arm/arm64/head.S |  62 ++++-
> >>  xen/arch/arm/efi-shared.c |   1 +
> >>  xen/arch/arm/efi.c        | 686 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  4 files changed, 750 insertions(+), 1 deletion(-)
> >>  create mode 120000 xen/arch/arm/efi-shared.c
> >>  create mode 100644 xen/arch/arm/efi.c
> >>
> >> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> >> index 63e0460..c3ed74f 100644
> >> --- a/xen/arch/arm/Makefile
> >> +++ b/xen/arch/arm/Makefile
> >> @@ -33,6 +33,8 @@ obj-y += hvm.o
> >>  obj-y += device.o
> >>  obj-y += decode.o
> >>  obj-y += processor.o
> >> +obj-y += efi.o
> >> +obj-y += efi-shared.o
> >
> > While the latter will eventually be in xen/common/efi/something.c and
> > hence -fshort-char is taken care of I suppose efi.o needs it too?
> >
> > Probably the easiest thing to do there is to create
> > xen/arch/arm/efi/stub.c and do the fshort-char trick for that entire
> > folder, since I don't think our build system makes it especially easy to
> > change CFLAGS for individual object files.
> 
> Yes and yes.  I will likely end up with some single file directories due to the
> build option differences.  Is there a XEN build system expert that I can ask
> for help if needed?  (Or should I just post to the list?)

I'm not sure anyone is an expert on this stuff but feel free to prod me
either here or on #xenarm.

I think you just need something like, in xen/arch/arm/Makefile
   subdir-$(arm64) += efi # or CONFIG_EFI etc
and in xen/arch/arm/efi/Makefile:
   CFLAGS += -fshort-wchar
   obj-y += foo.o etc.o

much the same for xen/common/efi.

> >
> >> +
> >> +     /*
> >> +      * efi_entry() will return here with device tree address in x0.
> >> +      *  Save value in register which is preserved by __flush_dcache_all.
> >> +      */
> >> +     mov     x20, x0
> >> +
> >> +     /* Turn off Dcache and MMU */
> >
> > AIUI EFI leaves us in a 1:1 mapping, hence this is safe to do.
> 
> Yes, EFI specifies a 1:1 mapping.

Could you append to the comment ", we are running on EFI's 1:1 mapping"
or something please.

>   The assembly wrapper here is the same as we
> use for Linux, so it still needs some tweaks.  Linux expects MMU/DCACHE off.

Xen's main entry point is the same. I was wondering recently about
allowing the stub to enter it with them on, I think all that would be
needed is some cache maintenance as we build the boot page tables
(which would be harmless in the zImage case).

I think that's very much a thing for a future enhancement though...

> >
> >> +     bic     x0, x0, #1 << 0 // clear SCTLR.M
> >> +     bic     x0, x0, #1 << 2 // clear SCTLR.C
> >> +     msr     sctlr_el1, x0
> >> +     isb
> >> +2:
> >> +/*   bl      __flush_dcache_all */  /* RFRANZ_TODO */
> >
> > We just don't have this function right now, correct? I suppose you are
> > running on models without cache modelling turned on?
> >
> > Eventually this will implement a total by set/way, right? That's safe
> > because we are uniprocessor at this point etc etc.
> 
> This works on the model since I'm not modeling the cache, but we do need it
> if we are going to turn the dcache off.  If we can leave the dcache enabled
> for XEN then I don't think we need to flush it here.

It's up to you I think, if you want to just fill in __flush_dcache_all
(which I expect can be cribbed from Linux) that's fine, but if you
really want to you could instead put the necessary flushes into head.S
to make it safe to run with or without mmu+caches.

> >> + * has been added to allow review and some testing of the stub code while
> >> + * the XEN kernel code is being developed.
> >> + */
> >> +#define ADD_EFI_MEMORY_TO_FDT
> >> +
> >> +#define DEVICE_TREE_GUID \
> >> +{0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0}}
> >> +
> >> +extern CHAR16 __initdata newline[];
> >> +extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdOut;
> >> +extern SIMPLE_TEXT_OUTPUT_INTERFACE *__initdata StdERR;
> >> +extern EFI_BOOT_SERVICES *__initdata efi_bs;
> >> +
> >> +
> >> +static EFI_HANDLE __initdata efi_ih;
> >> +
> >> +static struct file __initdata cfg;
> >> +static struct file __initdata kernel;
> >> +static struct file __initdata ramdisk;
> >> +static struct file __initdata dtb;
> >> +
> >> +static unsigned long mmap_size;
> >> +static EFI_MEMORY_DESCRIPTOR *mmap_ptr;
> >> +
> >> +static void *new_fdt;
> >> +
> >> +#ifdef ADD_EFI_MEMORY_TO_FDT
> >> +static int IsLinuxReservedRegion(int MemoryType)
> >
> > Linux? I guess this is a hangover from the Linux stub?
> >
> > (Even there I'd have thought the regions were reserved by EFI not Linux
> > though, perhaps I'm looking at it backwards though)
> 
> Since this is part of a temporary hack, I didn't rename anything :)

Ah yes, the #ifdef right above should have clued me in to that!

> >> +/*
> >> + * Set a single 'reg' property taking into account the
> >> + * configured addr and size cell sizes.
> >> + */
> >> +static int fdt_set_reg(void *fdt, int node, int addr_cells, int size_cells,
> >> +                       uint64_t addr, uint64_t len)
> >
> > FWIW I think we have such a helper already somewhere, but perhaps this
> > file with -fshort-char etc is unable to call it?
> 
> I'll try to find it.  I think it should be callable, since wide
> characters aren't used
> within this function of in its interface.

I just had a look and I think I imagined it...

There is one in the userspace domain builder, but that is no use to you
here.

I expect that xen/arch/arm/domain_build.c could make use of this helper
too, but don't worry about that in this series.

> >> +{
> >> +    uint8_t data[16]; /* at most 2 64 bit words */
> >> +    void *p = data;
> >> +
> >> +    /* Make sure that the values provided can be represented in
> >> +     * the reg property.
> >> +     */
> >> +    if (addr_cells == 1 && (addr >> 32))
> >
> > Xen coding style puts spaces more often than you may be used to. In
> > particular inside the brackets of an if (and while etc). So here:
> >         if ( addr_cells == 1 && ( addr >> 32 ) )
> >
> I tried to be good about that :)  I was hoping for a checkpatch script
> to catch all that kind of stuff for me :)

I'm afraid I don't think we have one :-(

> I'll review this and the use of tabs again.

Thanks. 

>  Thanks Ian, Stefano and Jan for all your feedback.

Thanks for you work on this!

Ian.

Hi Ian & Roy,

On 10/07/14 09:13, Ian Campbell wrote:
>>> Probably the easiest thing to do there is to create
>>> xen/arch/arm/efi/stub.c and do the fshort-char trick for that entire
>>> folder, since I don't think our build system makes it especially easy to
>>> change CFLAGS for individual object files.
>>
>> Yes and yes.  I will likely end up with some single file directories due to the
>> build option differences.  Is there a XEN build system expert that I can ask
>> for help if needed?  (Or should I just post to the list?)
>
> I'm not sure anyone is an expert on this stuff but feel free to prod me
> either here or on #xenarm.
>
> I think you just need something like, in xen/arch/arm/Makefile
>     subdir-$(arm64) += efi # or CONFIG_EFI etc
> and in xen/arch/arm/efi/Makefile:
>     CFLAGS += -fshort-wchar
>     obj-y += foo.o etc.o
>
> much the same for xen/common/efi.

The Xen buildsystem doesn't provide CFLAGS facilities as the Kernel, but 
the Makefile has a syntax to append data in variables for a specific target.

If your efi.c is in xen/arch/arm/, you need to add in xen/arch/arm/Makefile:

efi.o: CFLAGS += -fshort-wchar

Regards,

On Thu, 2014-07-10 at 10:21 +0100, Julien Grall wrote:
> Hi Ian & Roy,
> 
> On 10/07/14 09:13, Ian Campbell wrote:
> >>> Probably the easiest thing to do there is to create
> >>> xen/arch/arm/efi/stub.c and do the fshort-char trick for that entire
> >>> folder, since I don't think our build system makes it especially easy to
> >>> change CFLAGS for individual object files.
> >>
> >> Yes and yes.  I will likely end up with some single file directories due to the
> >> build option differences.  Is there a XEN build system expert that I can ask
> >> for help if needed?  (Or should I just post to the list?)
> >
> > I'm not sure anyone is an expert on this stuff but feel free to prod me
> > either here or on #xenarm.
> >
> > I think you just need something like, in xen/arch/arm/Makefile
> >     subdir-$(arm64) += efi # or CONFIG_EFI etc
> > and in xen/arch/arm/efi/Makefile:
> >     CFLAGS += -fshort-wchar
> >     obj-y += foo.o etc.o
> >
> > much the same for xen/common/efi.
> 
> The Xen buildsystem doesn't provide CFLAGS facilities as the Kernel, but 
> the Makefile has a syntax to append data in variables for a specific target.
> 
> If your efi.c is in xen/arch/arm/, you need to add in xen/arch/arm/Makefile:
> 
> efi.o: CFLAGS += -fshort-wchar

That would work but I think I'd rather keep the efi stuff in its own
subdirectory, even if right now it is only a single C file.

Ian.