Re: [Xen-devel] [PATCH V2 12/12] Add EFI stub for arm64

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-07-21 at 17:43 -0700, Roy Franz wrote:

Overall looks good, thanks, I've a bunch of mostly minor comments.

> +ifeq ($(EFI),y)
> +AFLAGS += -DCONFIG_EFI_STUB

AFAICT this is only used in arm64/head.S where there is no need for the
stub to be conditional.

So this can go away, unless there is some common code which I'm missing.


> @@ -84,7 +86,6 @@
>  #endif /* !CONFIG_EARLY_PRINTK */
>  
>          /*.aarch64*/
> -

Spurious change.

>          /*
>           * Kernel startup entry point.
>           * ---------------------------
> @@ -101,11 +102,22 @@
>  
>          .global start
>  start:
> +#ifdef CONFIG_EFI_STUB

As I said above I don't think this needs to be conditional.

> @@ -561,9 +685,62 @@ putn:   ret
>   * TODO: For now, the implementation return NULL every time
>   */
>  GLOBAL(lookup_processor_type)
> -        mov  x0, #0
> +        mov   x0, #0
> +        ret
> +
> +
> +

Please avoid multiple blank lines, just one will do.

> +ENTRY(efi_stub_entry)
> +        stp   x29, x30, [sp, #-32]!
> +
> +        /*
> +         * Call efi_entry to do the real work.
> +         * x0 and x1 are already set up by firmware.
> +         * EFI mandates a 1:1 (unity) VA->PA mapping,
> +         * so we can turn off the MMU before entering
> +         * XEN.
> +         *
> +         * unsigned long efi_entry(EFI_HANDLE handle,
> +         *                             EFI_SYSTEM_TABLE *sys_table);
> +         */
> +
> +        bl    efi_entry
> +        cmp   x0, EFI_STUB_ERROR
> +        b.eq  efi_load_fail
> +
> +        /*
> +         * efi_entry() will return here with device tree address in x0.
> +         *  Save value in register which is preserved by __flush_dcache_all.
> +         */
> +
> +

The comment should abut the mov which it refers to please.

> +        mov   x20, x0
> +        bl    __flush_dcache_all
> +        ic      ialluis

iall... is indented a bit to far.

> diff --git a/xen/arch/arm/efi/Makefile b/xen/arch/arm/efi/Makefile
> new file mode 100644
> index 0000000..74863ba
> --- /dev/null
> +++ b/xen/arch/arm/efi/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(EFI) += efi.o

I assume you don't recurse into here unless efi == y? So you can use
obj-y I think.


> diff --git a/xen/arch/arm/efi/efi.c b/xen/arch/arm/efi/efi.c
> new file mode 100644
> index 0000000..832b324
> --- /dev/null
> +++ b/xen/arch/arm/efi/efi.c
> @@ -0,0 +1,714 @@
> +#include <asm/efibind.h>
> +#include <efi/efidef.h>
> +#include <efi/efierr.h>
> +#include <efi/eficon.h>
> +#include <efi/efidevp.h>
> +#include <efi/eficapsule.h>
> +#include <efi/efiapi.h>
> +#include <xen/efi.h>
> +#include <xen/spinlock.h>
> +#include <asm/page.h>
> +#include <efi/efiprot.h>
> +#include <efi/efi-shared.h>
> +#include <public/xen.h>
> +#include <xen/compile.h>
> +#include <xen/ctype.h>
> +#include <xen/init.h>
> +#include <xen/keyhandler.h>
> +#include <xen/lib.h>
> +#include <xen/mm.h>
> +#include <xen/pfn.h>

Do you really use all of these? e.g. I don't see why keyhandler is
needed. I didn't check the others.

> +#if EFI_PAGE_SIZE != PAGE_SIZE
> +# error Cannot use xen/pfn.h here!
> +#endif
> +#include <xen/string.h>
> +#include <xen/stringify.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <asm/setup.h>
> +
> 
> +void __init noreturn blexit(const CHAR16 *str);

Should this be static?

> +#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;

Put externs in a header please. Aren't these defined in common code and
therefore belong in a common header?

> +/*
> + * Hacky way to make sure EFI allocations end up in memory that XEN
> + * includes in its mappings.

This was due to the discard_initial_modules thing, right?

> + * RFRANZ_TODO - this needs to be resolved properly.
> + */
> +static EFI_PHYSICAL_ADDRESS max_addr = 0xffffffff;
> +
> +static void *new_fdt;
> +
> +static EFI_STATUS __init 
> efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *map,
> +                                                unsigned long mmap_size,
> +                                                unsigned long desc_size)
> +{
> +    int Index;
> +    int i = 0;
> +
> +    EFI_MEMORY_DESCRIPTOR *desc_ptr = map;
> +
> +    for ( Index = 0; Index < (mmap_size / desc_size); Index++ )

Perhaps this is my ignorance of EFI showing, but is Index really not
used within the loop?

It almost looks like you want a test on desc_ptr for the end of the list
or something?

> +    {
> +        if ( desc_ptr->Type == EfiConventionalMemory
> +             || desc_ptr->Type == EfiBootServicesCode
> +             || desc_ptr->Type == EfiBootServicesData )
> +        {
> +            bootinfo.mem.bank[i].start = desc_ptr->PhysicalStart;
> +            bootinfo.mem.bank[i].size = desc_ptr->NumberOfPages * 
> EFI_PAGE_SIZE;

Having both Index and i is a bit confusing. How about s/i/bank/? Or
perhaps
                
        struct thingumy *bank;

        if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
        {
             exhausted...
        }
        bank = &bootinfo.mem.bank[bootinfo.mem.nr_banks++];
        bank->start = ...

?

> +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;
> +
> +    *map = NULL;
> +    *mmap_size = EFI_PAGE_SIZE;
> +again:
> +    *mmap_size += EFI_PAGE_SIZE;  /* Page size is allocation granularity */

These two assignments together mean that on the first pass you will
allocate two pages, is that what you intended?

> +    status = sys_table_arg->BootServices->AllocatePool(EfiLoaderData,
> +                                                       *mmap_size, (void 
> **)&m);

I don't think you need the cast here.


> +    fdt_val64 = cpu_to_fdt64((u64)(unsigned long)sys_table);
> +    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));

Are the bindings for linux,uefi-* described somewhere?

These are used entirely within the stub->kernel (Xen or Linux) interface
boundary, right? Since they are "internal" I wonder if we should use
"xen,uefi-*" (with the same semantics as linux,uefi-*) to avoid any
potential confusion in the future?

If there is any requirement for these to be exposed outside of the
Xen<->Stub interface then we would be better off using the linux naming
to remain compatible.

> +    pages = PFN_UP(fdt_size) + PFN_UP(add_size);

pages = PFN_UP(fdt_size + add_size) ?


> +    /*
> +     * Allocate space for new FDT, making sure we have enough space
> +     * for the fields we are adding, so we don't have to deal
> +     * with increasing the size again later, which complicates
> +     * things.  Use the size of the configuration file as an uppper

Too many pppps.

> +     * bound on how much size can be added based on configuration
> +     * file contents.

The size of the configuration file doesn't seem like a very good
estimate, it doesn't account for the linux,uefi-* properties and
a /chosen/module node could be larger than the string in the cfg file
needed to specify it. I suppose the extra EFI_PAGE_SIZE slop, plus the
rounding up to a page probably accounts for this sufficiently.

FWIW in the equivalent Xen side code we just allocate guessed size and
if it isn't enough throw it away and try again from scratch with a
larger size, rather than trying to increase the size on the fly which as
you say gets complicated.

> +    /* Check if we were booted by the EFI firmware */
> +    if ( SystemTable->Hdr.Signature != EFI_SYSTEM_TABLE_SIGNATURE )
> +        goto fail;

You only use goto fail once in this function AFAICT, everywhere else
calls blexit directly with a specific error message. I think you may as
well do the same here.

> +
> +    /* Get loaded image protocol */
> +    status = efi_bs->HandleProtocol(ImageHandle, &loaded_image_guid,
> +                                    (void **)&loaded_image);
> +    if ( status != EFI_SUCCESS )
> +        blexit(L"ERROR - no loaded image protocol\r\n");
> +
> +    PrintStr(L"Xen " __stringify(XEN_VERSION)"." __stringify(XEN_SUBVERSION)
> +             XEN_EXTRAVERSION " (c/s " XEN_CHANGESET ") EFI loader\r\n");
> +
> +    if ( (unsigned long)loaded_image->ImageBase & ((1 << 20) - 1) )
> +        blexit(L"Xen must be loaded at a 2MByte boundary.");

This restriction has been relaxed to only require 4K alignment now. See
ca59618967fe "xen: arm: Handle 4K aligned hypervisor load address".

> +    compat_len = 0;
> +    compat_len += snprintf(compat_buf + compat_len,
> +                           COMPAT_BUF_SIZE - compat_len,
> +                           "multiboot,kernel") + 1;
> +    if ( compat_len > COMPAT_BUF_SIZE )
> +        blexit(L"FDT string overflow");
> +    compat_len += snprintf(compat_buf + compat_len,
> +                           COMPAT_BUF_SIZE - compat_len,
> +                           "multiboot,module") + 1;
> +    if ( compat_len > COMPAT_BUF_SIZE )
> +        blexit(L"FDT string overflow");

In most places we use
        const char *compat = "multiboot,module\0" "multiboot,kernel";
to avoid the faffing around with buffers, snprintf and off by one errors. 

> +    /*
> +     * cmdline has remaining options from EFI command line.  Prepend these
> +     * to the options from the configuration file.  Put the image name at
> +     * the beginning of the bootargs.

Is the prepending of the image name an EFI-ism?

> +void __init noreturn blexit(const CHAR16 *str)
> +{
> +    if ( str )
> +        PrintStr((CHAR16 *)str);
> +    PrintStr(newline);
> +
> +    if ( cfg.addr )
> +        efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
> +    if ( kernel.addr )
> +        efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
> +    if ( ramdisk.addr )
> +        efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
> +    if ( dtb.addr && dtb.size )
> +        efi_bs->FreePages(dtb.addr, PFN_UP(dtb.size));
> +    if ( mmap_ptr )
> +        efi_bs->FreePool(mmap_ptr);
> +
> +    efi_bs->Exit(efi_ih, EFI_SUCCESS, 0, NULL);

Is EFI_SUCCESS correct? I thought this was an error path.

> +
> +//
> +// When build similiar to FW, then link everything together as

"similar" (cut-n-paste error from elsewhere?)

> +// one big module.

> diff --git a/xen/include/asm-arm/efibind.h b/xen/include/asm-arm/efibind.h
> new file mode 100644
> index 0000000..09dca7a
> --- /dev/null
> +++ b/xen/include/asm-arm/efibind.h
> @@ -0,0 +1,2 @@
> +#include <xen/types.h>
> +#include <asm/arm64/efibind.h>

Should this include some sort of CONFIG_ARM_64 based guard against using
this code from an arm32 build?

#ifndef CONFIG_ARM_64
#error "EFI is arm64 only"
#endif

perhaps?

Ian.


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