|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC 19/19] Add EFI stub for ARM64
On Wed, Jul 2, 2014 at 5:34 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |