Re: [Linaro-uefi] [PATCH RFC 19/19] Add EFI stub for ARM64

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