Linaro-uefi

linaro-uefi@lists.linaro.org

811 discussions

[PATCH] Move RTSM VExpress variable storage to 256k flash blocks

by Roy Franz

Change the addresses/sizes of the variable storage areas to use 256k blocks so UEFI is compatible with both the RTSM models and QEMU. The VExpress flash has non-uniform block sizes, with most blocks being 256k and the top 4 blocks being 64k. UEFI has been using these top 64k blocks for persistent variable storage. The RTSM models the non-uniform sizes, while QEMU only supports emulating flash with uniform block sizes which results in the top 256k (the 4 64k blocks) of flash being unusable for writing in QEMU. Note that this change will require RTSM flash images to be updated, as the variable storage has moved. Currently on the A15 model is supported by QEMU, but the A9 configuration is being updated as well to keep all RTSM VExpress configurations consistent. Signed-off-by: Roy Franz <roy.franz(a)linaro.org> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Steven Kinney <steven.kinney(a)linaro.org> --- ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15.dsc | 12 ++++++------ .../ArmVExpressPkg/ArmVExpress-RTSM-A15_MPCore.dsc | 12 ++++++------ ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A9x4.dsc | 12 ++++++------ 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15.dsc b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15.dsc index 2d12f4b..8883213 100644 --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15.dsc +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15.dsc @@ -77,12 +77,12 @@ # # NV Storage PCDs. Use base of 0x0C000000 for NOR1 # - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x0FFC0000 - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00010000 - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x0FFD0000 - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00010000 - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x0FFE0000 - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00010000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x0FF00000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00040000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x0FF40000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00040000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x0FF80000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00040000 gArmTokenSpaceGuid.PcdVFPEnabled|1 diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15_MPCore.dsc b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15_MPCore.dsc index efd80ab..ae42de2 100644 --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15_MPCore.dsc +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A15_MPCore.dsc @@ -79,12 +79,12 @@ # # NV Storage PCDs. Use base of 0x0C000000 for NOR1 # - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x0FFC0000 - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00010000 - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x0FFD0000 - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00010000 - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x0FFE0000 - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00010000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x0FF00000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00040000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x0FF40000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00040000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x0FF80000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00040000 gArmTokenSpaceGuid.PcdVFPEnabled|1 diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A9x4.dsc b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A9x4.dsc index b635502..4d4d8b1 100644 --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A9x4.dsc +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-RTSM-A9x4.dsc @@ -81,12 +81,12 @@ # # NV Storage PCDs. Use base of 0x0C000000 for NOR1 # - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x0FFC0000 - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00010000 - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x0FFD0000 - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00010000 - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x0FFE0000 - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00010000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x0FF00000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00040000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x0FF40000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00040000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x0FF80000 + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00040000 gArmTokenSpaceGuid.PcdVFPEnabled|1 -- 1.7.10.4

11 years

Re: [Linaro-uefi] Updated QEMU flash topic branch

by Roy Franz

On Wed, Dec 11, 2013 at 2:56 AM, Olivier Martin <olivier.martin(a)arm.com> wrote: > Thanks Roy for the investigation. > > FYI, I am working on fixing our UEFI in our CI infrastructure to validate > your patch. > The 'restriction' you found out is interesting, I need to review the code to > see if it can be fixed or if it is really a restriction. > > Olivier > Hi Olivier, I'm still a bit unsure how all the NOR and Fvb initialization fits together, but it looks like the problem is at least partially in: NorFlashFvbInitialize(), which calls ValidateFvHeader() with a NOR flash instance. The ARM version of ValidateFvHeader is defined as: EFI_STATUS ValidateFvHeader ( IN NOR_FLASH_INSTANCE *Instance ) And then checks for a header at Instance->RegionBaseAddress. Others are: EFI_STATUS ValidateFvHeader ( IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader ) I have not looked at the others to see if the volume header is tied to the start of flash or not, but at least the APIs allow for it to be elsewhere. Since each set of block sizes gets its own flash region, the original flash addresses worked because it was at the start of the boot block region. Roy >> -----Original Message----- >> From: linaro-uefi-bounces(a)lists.linaro.org [mailto:linaro-uefi- >> bounces(a)lists.linaro.org] On Behalf Of Roy Franz >> Sent: 11 December 2013 03:24 >> To: Steven Kinney; ryan.harkin(a)linaro.org; linaro-uefi >> Subject: [Linaro-uefi] Updated QEMU flash topic branch >> >> I have pushed an updated flash topic branch that fixes the persistent >> storage on RTSM and QEMU. >> The ARM NOR support in UEFI only supports Firmware volumes that start >> at the beginning of a NOR flash region, so I have moved the variable >> storage to be at the base of the secondary flash. This seems like an >> odd restriction, and I'm unsure if this was an intentional design >> decision of just a simplification to get NOR working. >> >> Topic branch: >> >> https://git.linaro.org/people/roy.franz/uefi- >> next.git/shortlog/refs/heads/topic-qemu-flash-v2 >> >> >> I will need to send out updated patches to the list based on these >> changes as well. >> >> Roy >> >> _______________________________________________ >> Linaro-uefi mailing list >> Linaro-uefi(a)lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/linaro-uefi > > > >

11 years

Updated QEMU flash topic branch

by Roy Franz

I have pushed an updated flash topic branch that fixes the persistent storage on RTSM and QEMU. The ARM NOR support in UEFI only supports Firmware volumes that start at the beginning of a NOR flash region, so I have moved the variable storage to be at the base of the secondary flash. This seems like an odd restriction, and I'm unsure if this was an intentional design decision of just a simplification to get NOR working. Topic branch: https://git.linaro.org/people/roy.franz/uefi-next.git/shortlog/refs/heads/t… I will need to send out updated patches to the list based on these changes as well. Roy

11 years

Re: [Linaro-uefi] [PATCH v3 1/3] Documentation: arm: add UEFI support documentation

by Leif Lindholm

On Mon, Dec 02, 2013 at 01:51:22PM -0600, Matt Sealey wrote: > Here's where I think this whole thing falls down as being the weirdest > possible implementation of this. It defies logic to put this > information in the device tree /chosen node while also attempting to > boot the kernel using an EFI stub; the stub is going to have this > information because it is going to have the pointer to the system > System Table (since it was called by StartImage()). Why not stash the > System Table pointer somewhere safe in the stub? We do. In the DT. > The information in the device tree is all accessible from Boot > Services and as long as the System Table isn't being thrown away (my > suggestion would be.. stuff it in r2, and set r1 = "EFI\0" then work > with arch/arm/kernel/head{-common,}.S code to do the right thing) You left out the bit of redefining the kernel boot protocol to permit calling it with caches, MMU and interrupts enabled - also known as before ExitBootServices(). > It seems like the advantages of booting from UEFI and having all this > information and API around are being thrown away very early, and > picked up when it's no longer relevant to gain access to the very > minimal runtime services. What's missing is a UUID for a "Device Tree > Blob" in the Configuration Table, so you can very easily go grab that > information from the firmware. Which is what we are going to implement anyway in order to permit firmware to supply DT hardware description in the same way as with ACPI. Yes, we could pass the system table pointer directly - but that doesn't get us the memory map. > As implemented, these patches employ a very long-winded and complex > method of recovering UEFI after throwing the system table pointer away > early in boot, and then implements an EFI calling convention which > isn't strictly necessary according to the UEFI spec - the question is, > is this a workaround for SetVirtualAddressMap() not actually doing the > right thing on ARM UEFI implementations? If you can't guarantee that > most of the calls from Boot Services or Runtime Services are going to > allow this, then having any UEFI support in the kernel at all seems > rather weird. No, it is a workaround for it being explicitly against the kernel boot protocol (not to mention slightly hairy) to enter head.S with MMU and caches enabled and interrupts firing. The EFI calling convention (as pointed out in the patch itself) is there in order to not have to duplicate code already there for x86. > What I'm worried about is that this is basically a hack to try and > shoehorn an existing UEFI implementation to an existing Linux boot > method - and implements it in a way nobody is ever going to be able to > justify improving. Part of the reason the OpenFirmware CIF got thrown > away early in SPARC/PowerPC boot (after "flattening" the device tree > using the CIF calls to parse it out) was because you had to disable > the MMU, caches, interrupts etc. which caused all kinds of slow > firmware code to be all kinds of extra-slow. I prefer to see it as a way to not reinvent things that do not need reinventing, while not adding more special-case code to the kernel. > What that meant is nobody bothered to implement working, re-entrant, > re-locatable firmware to a great degree. This ended up being a > self-fulfilling prophecy of "don't trust the bootloader" and "get rid > of it as soon as we can," which essentially meant Linux never took > advantage of the resources available. In OF's case, the CIF sucked by > specification. In UEFI's case here, it's been implemented in Linux in > such a way that guarantees poor-performing firmware code with huge > penalties to call them, which isn't even required by UEFI if the > earlier boot code did the right things in the first place. I don't follow. In which way does this implementation result in poor performance or reduced functionality? We deal with a highly quirky set of requirements for calling SetVirtualAddressMap() in a clunky way - after which calls into UEFI are direct and cachable. / Leif

11 years

[PATCH 0/5] Update default kernel commandline

by Ryan Harkin

The kernel commandline has been simplified to remove deprecated and unwanted options. The patch has been split into 5 due to the relevant files being managed in different topic branches in the tree: linaro-topic-tc2: [PATCH 1/5] TC2: update default kernel commandline linaro-topic-tc1: [PATCH 2/5] TC1: update default kernel commandline linaro-topic-a5: [PATCH 3/5] VEA5: update default kernel commandline linaro-topic-a9: [PATCH 4/5] VEA9: update default kernel commandline linaro-topic-bds: [PATCH 5/5] RTSM: update default kernel commandline

11 years

[PATCH 1/1] FVP: update Android FDT config

by Ryan Harkin

Android does not use the Foundation model, so the extra step of linking fdt.dtb to the correct DTB file is not needed. Therefore, default the Android config to use fvp-base-gicv2-psci.dtb directly. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ryan Harkin <ryan.harkin(a)linaro.org> --- .../ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc index 1b681b8..db61aa8 100644 --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc @@ -163,11 +163,12 @@ !ifdef $(EDK2_USE_ANDROID_CONFIG) gArmPlatformTokenSpaceGuid.PcdDefaultBootDevicePath|L"VenHw(C5B9C74A-6D72-4719-99AB-C59F199091EB)/kernel" gArmPlatformTokenSpaceGuid.PcdDefaultBootInitrdPath|L"VenHw(C5B9C74A-6D72-4719-99AB-C59F199091EB)/ramdisk.img" + gArmPlatformTokenSpaceGuid.PcdDefaultFdtLocalDevicePath|L"VenHw(C5B9C74A-6D72-4719-99AB-C59F199091EB)/fvp-base-gicv2-psci.dtb" !else gArmPlatformTokenSpaceGuid.PcdDefaultBootDevicePath|L"VenHw(C5B9C74A-6D72-4719-99AB-C59F199091EB)/Image" gArmPlatformTokenSpaceGuid.PcdDefaultBootArgument|"console=ttyAMA0 earlyprintk=pl011,0x1c090000 debug user_debug=31 loglevel=9 root=/dev/vda2" -!endif gArmPlatformTokenSpaceGuid.PcdDefaultFdtLocalDevicePath|L"VenHw(C5B9C74A-6D72-4719-99AB-C59F199091EB)/fdt.dtb" +!endif gArmPlatformTokenSpaceGuid.PcdDefaultBootType|3 gArmPlatformTokenSpaceGuid.PcdFdtDevicePath|L"VenHw(C5B9C74A-6D72-4719-99AB-C59F199091EB)/fdt.dtb" -- 1.7.9.5

11 years

[PATCH 0/3] Fix build failures since 2013.12-rc1

by Ryan Harkin

The following patches fix various Versatile Express BSP build issues happening since 2013.12-rc1 was pushed. [PATCH 1/3] VEA5: ARM Packages: Renamed PL390Gic driver into ArmGic [PATCH 2/3] TC1: ARM Packages: Renamed PL390Gic driver into ArmGic [PATCH 3/3] TC2: fix debug builds after ACPI was added

11 years

Re: [Linaro-uefi] [PATCH v3 2/3] arm: Add [U]EFI runtime services support

by Leif Lindholm

Hi Arnd, Thank you for your comments. On Fri, Dec 06, 2013 at 02:59:48AM +0100, Arnd Bergmann wrote: > On Thursday 28 November 2013, Leif Lindholm wrote: > > @@ -898,6 +900,10 @@ void __init setup_arch(char **cmdline_p) > > sanity_check_meminfo(); > > arm_memblock_init(&meminfo, mdesc); > > > > +#ifdef CONFIG_EFI > > + uefi_memblock_arm_reserve_range(); > > +#endif > > + > > Better use > > if (IS_ENABLED(CONFIG_EFI)) > > here for readability and better build-time checking of the interface when > CONFIG_EFI is disabled. I think I'll do what Grant suggested and stub it out in asm/uefi.h. > > +/* > > + * Returns 1 if 'facility' is enabled, 0 otherwise. > > + */ > > +int efi_enabled(int facility) > > +{ > > + return test_bit(facility, &arm_uefi_facility) != 0; > > +} > > +EXPORT_SYMBOL(efi_enabled); > > I'd use EXPORT_SYMBOL_GPL() unless there is a documented reason why > a symbol should be available to GPL-incompatible modules. So ... this is duplicating x86 code which Matt Fleming has promised to make generic. I'd prefer to keep it identical to x86 until this copy goes away. > > +static __init int is_discardable_region(efi_memory_desc_t *md) > > +{ > > +#ifdef KEEP_ALL_REGIONS > > + return 0; > > +#endif > > IS_ENABLED() again. Ok. > > + if (md->attribute & EFI_MEMORY_RUNTIME) > > + return 0; > > + > > + switch (md->type) { > > +#ifdef KEEP_BOOT_SERVICES_REGIONS > > + case EFI_BOOT_SERVICES_CODE: > > + case EFI_BOOT_SERVICES_DATA: > > +#endif > > and I think it can be used here too: > > switch (md->type) { > case EFI_BOOT_SERVICES_CODE: > case EFI_BOOT_SERVICES_DATA: > if (IS_ENABLED(KEEP_BOOT_SERVICES_REGIONS)) > return 1; > /* fallthrough */ > case EFI_ACPI_RECLAIM_MEMORY: Will redesign for next version. > > + memmap.phys_map = early_memremap((phys_addr_t)(u32) uefi_boot_mmap, > > + uefi_boot_mmap_size); > > + if (!memmap.phys_map) > > + return 1; > > + > > + p = memmap.phys_map; > > + e = (void *)((u32)p + uefi_boot_mmap_size); > > You are doing a lot of type casts here, which is normally an indication > that you have the types wrong in some way. I can't spot a mistake > here, but maybe you can give it some more thought and see if it can be > changed. Grant made much the same comments here. It is possible the change to the DT bindings to have all addresses 64-bit actually makes this go away. I will revisit for next version. > > +static int __init remap_region(efi_memory_desc_t *md, efi_memory_desc_t *entry) > > +{ > > + u64 va; > > + u64 paddr; > > + u64 size; > > + > > + *entry = *md; > > + paddr = entry->phys_addr; > > + size = entry->num_pages << EFI_PAGE_SHIFT; > > + > > + /* > > + * Map everything writeback-capable as coherent memory, > > + * anything else as device. > > + */ > > + if (md->attribute & EFI_MEMORY_WB) > > + va = (u64)((u32)uefi_remap(paddr, size) & 0xffffffffUL); > > + else > > + va = (u64)((u32)uefi_ioremap(paddr, size) & 0xffffffffUL); > > Same here. Why is 'va' a u64? Because the field in the structure (defined by UEFI) is 64-bit: entry->virt_addr = va; / Leif

11 years

Re: [Linaro-uefi] [PATCH v3 2/3] arm: Add [U]EFI runtime services support

by Leif Lindholm

Hi Will, Thanks for having a look. On Fri, Nov 29, 2013 at 04:10:16PM +0000, Will Deacon wrote: > > This patch implements basic support for UEFI runtime services in the > > ARM architecture - a requirement for using efibootmgr to read and update > > the system boot configuration. > > > > It uses the generic configuration table scanning to populate ACPI and > > SMBIOS pointers. > > I've got a bunch of implementation questions, so hopefully you can help me > to understand what this is doing! I can try :) > > diff --git a/arch/arm/kernel/uefi.c b/arch/arm/kernel/uefi.c > > new file mode 100644 > > index 0000000..f771026 > > --- /dev/null > > +++ b/arch/arm/kernel/uefi.c > > +/* > > + * Returns 1 if 'facility' is enabled, 0 otherwise. > > + */ > > +int efi_enabled(int facility) > > +{ > > + return test_bit(facility, &arm_uefi_facility) != 0; > > !test_bit(...) ? Could do. Cloned from the x86 implementation. Matt Fleming has indicated some rework coming in this area. > > +static int __init uefi_init(void) > > +{ > > + efi_char16_t *c16; > > + char vendor[100] = "unknown"; > > + int i, retval; > > + > > + efi.systab = early_memremap(uefi_system_table, > > + sizeof(efi_system_table_t)); > > + > > + /* > > + * Verify the UEFI System Table > > + */ > > + if (efi.systab == NULL) > > + panic("Whoa! Can't find UEFI system table.\n"); > > + if (efi.systab->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE) > > + panic("Whoa! UEFI system table signature incorrect\n"); > > + if ((efi.systab->hdr.revision >> 16) == 0) > > + pr_warn("Warning: UEFI system table version %d.%02d, expected 1.00 or greater\n", > > + efi.systab->hdr.revision >> 16, > > + efi.systab->hdr.revision & 0xffff); > > + > > + /* Show what we know for posterity */ > > + c16 = (efi_char16_t *)early_memremap(efi.systab->fw_vendor, > > + sizeof(vendor)); > > + if (c16) { > > + for (i = 0; i < (int) sizeof(vendor) - 1 && *c16; ++i) > > + vendor[i] = c16[i]; > > + vendor[i] = '\0'; > > + } > > + > > + pr_info("UEFI v%u.%.02u by %s\n", > > + efi.systab->hdr.revision >> 16, > > + efi.systab->hdr.revision & 0xffff, vendor); > > + > > + retval = efi_config_init(NULL); > > + if (retval == 0) > > + set_bit(EFI_CONFIG_TABLES, &arm_uefi_facility); > > Does this actually need to be atomic? Not necessarily, but it's neater than masking, and only called once. > > + > > + early_iounmap(c16, sizeof(vendor)); > > + early_iounmap(efi.systab, sizeof(efi_system_table_t)); > > + > > + return retval; > > +} > > + > > +static __init int is_discardable_region(efi_memory_desc_t *md) > > +{ > > +#ifdef KEEP_ALL_REGIONS > > + return 0; > > +#endif > > Maybe just have a dummy is_discardable_region in this case, like we usually > do instead of inlining the #ifdef? OK. > > + if (md->attribute & EFI_MEMORY_RUNTIME) > > + return 0; > > + > > + switch (md->type) { > > +#ifdef KEEP_BOOT_SERVICES_REGIONS > > + case EFI_BOOT_SERVICES_CODE: > > + case EFI_BOOT_SERVICES_DATA: > > +#endif > > + /* Keep tables around for any future kexec operations */ > > + case EFI_ACPI_RECLAIM_MEMORY: > > + return 0; > > + /* Preserve */ > > + case EFI_RESERVED_TYPE: > > + return 0; > > + } > > + > > + return 1; > > +} > > [...] > > > +int __init uefi_memblock_arm_reserve_range(void) > > +{ > > + if (!of_scan_flat_dt(fdt_find_uefi_params, NULL)) > > + return 0; > > + > > + set_bit(EFI_BOOT, &arm_uefi_facility); > > Similar comment wrt atomicity here (and in the rest of this patch). Similar response. > > + uefi_init(); > > + > > + remove_regions(); > > + > > + return 0; > > +} > > + > > +/* > > + * Disable instrrupts, enable idmap and disable caches. > > interrupts Yeah. > > + */ > > +static void __init phys_call_prologue(void) > > +{ > > + local_irq_disable(); > > + > > + /* Take out a flat memory mapping. */ > > + setup_mm_for_reboot(); > > + > > + /* Clean and invalidate caches */ > > + flush_cache_all(); > > + > > + /* Turn off caching */ > > + cpu_proc_fin(); > > + > > + /* Push out any further dirty data, and ensure cache is empty */ > > + flush_cache_all(); > > Do we care about outer caches here? I think we do. We jump into UEFI and make it relocate itself to the new virtual addresses, with MMU disabled (so all accesses NC). > This all looks suspiciously like > copy-paste from __soft_restart; Yeah, 'cause you told me to :) > perhaps some refactoring/reuse is in order? Could do. Turn this into a process.c:idcall_prepare(), or something less daftly named? > > +} > > + > > +/* > > + * Restore original memory map and re-enable interrupts. > > + */ > > +static void __init phys_call_epilogue(void) > > +{ > > + static struct mm_struct *mm = &init_mm; > > + > > + /* Restore original memory mapping */ > > + cpu_switch_mm(mm->pgd, mm); > > + > > + /* Flush branch predictor and TLBs */ > > + local_flush_bp_all(); > > +#ifdef CONFIG_CPU_HAS_ASID > > + local_flush_tlb_all(); > > +#endif > > ... and this looks like copy-paste from setup_mm_for_reboot. You told me that too! Only this goes the other way. Is the refactoring worth the extra code? > > + local_irq_enable(); > > +} > > + > > +static int __init remap_region(efi_memory_desc_t *md, efi_memory_desc_t *entry) > > +{ > > + u64 va; > > + u64 paddr; > > + u64 size; > > + > > + *entry = *md; > > + paddr = entry->phys_addr; > > + size = entry->num_pages << EFI_PAGE_SHIFT; > > + > > + /* > > + * Map everything writeback-capable as coherent memory, > > + * anything else as device. > > + */ > > + if (md->attribute & EFI_MEMORY_WB) > > + va = (u64)((u32)uefi_remap(paddr, size) & 0xffffffffUL); > > + else > > + va = (u64)((u32)uefi_ioremap(paddr, size) & 0xffffffffUL); > > Do you really need all those casts/masking? I didn't find a better way to avoid warnings when building this code for both LPAE/non-LPAE. We are guaranteed by the UEFI spec that all addresses will be <4GB, but the descriptor format is still 64-bit physical/virtual addresses, and we need to pass them back as such. > > + if (!va) > > + return 0; > > + entry->virt_addr = va; > > + > > + if (uefi_debug) > > + pr_info(" %016llx-%016llx => 0x%08x : (%s)\n", > > + paddr, paddr + size - 1, (u32)va, > > + md->attribute & EFI_MEMORY_WB ? "WB" : "I/O"); > > + > > + return 1; > > +} > > + > > +static int __init remap_regions(void) > > +{ > > + void *p, *next; > > + efi_memory_desc_t *md; > > + > > + memmap.phys_map = uefi_remap(uefi_boot_mmap, uefi_boot_mmap_size); > > + if (!memmap.phys_map) > > + return 0; > > + memmap.map_end = (void *)memmap.phys_map + uefi_boot_mmap_size; > > + memmap.desc_size = uefi_mmap_desc_size; > > + memmap.desc_version = uefi_mmap_desc_ver; > > + > > + /* Allocate space for the physical region map */ > > + memmap.map = kzalloc(memmap.nr_map * memmap.desc_size, GFP_KERNEL); > > + if (!memmap.map) > > + return 0; > > + > > + next = memmap.map; > > + for (p = memmap.phys_map; p < memmap.map_end; p += memmap.desc_size) { > > + md = p; > > + if (is_discardable_region(md) || md->type == EFI_RESERVED_TYPE) > > + continue; > > + > > + if (!remap_region(p, next)) > > + return 0; > > What about the kzalloc above, does that need freeing? In case of failure? Yes, good point. > > +/* > > + * This function switches the UEFI runtime services to virtual mode. > > + * This operation must be performed only once in the system's lifetime, > > + * including any kecec calls. > > + * > > + * This must be done with a 1:1 mapping. The current implementation > > + * resolves this by disabling the MMU. > > + */ > > +efi_status_t __init phys_set_virtual_address_map(u32 memory_map_size, > > + u32 descriptor_size, > > + u32 descriptor_version, > > + efi_memory_desc_t *dsc) > > +{ > > + uefi_phys_call_t *phys_set_map; > > + efi_status_t status; > > + > > + phys_call_prologue(); > > + > > + phys_set_map = (void *)(unsigned long)virt_to_phys(uefi_phys_call); > > + > > + /* Called with caches disabled, returns with caches enabled */ > > + status = phys_set_map(memory_map_size, descriptor_size, > > + descriptor_version, dsc, > > + efi.set_virtual_address_map) > > +; > > Guessing this relies on a physically-tagged cache? Wouldn't hurt to put > something in the epilogue code to deal with vivt caches if they're not > prohibited by construction (e.g. due to some build dependency magic). Sorry, I don't quite follow? How would it depend on a physically-tagged cache? > > + phys_call_epilogue(); > > + > > + return status; > > +} > > diff --git a/arch/arm/kernel/uefi_phys.S b/arch/arm/kernel/uefi_phys.S > > new file mode 100644 > > index 0000000..e9a1542 > > --- /dev/null > > +++ b/arch/arm/kernel/uefi_phys.S > > @@ -0,0 +1,59 @@ > > +/* > > + * arch/arm/kernel/uefi_phys.S > > + * > > + * Copyright (C) 2013 Linaro Ltd. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + */ > > + > > +#include <linux/linkage.h> > > +#define PAR_MASK 0xfff > > + > > + .text > > +@ uefi_phys_call(a, b, c, d, *f) > > + .align 5 > > + .pushsection .idmap.text, "ax" > > +ENTRY(uefi_phys_call) > > + @ Save physical context > > + mov r12, sp > > + push {r4-r5, r12, lr} > > + > > + @ Extract function pointer (don't write r12 beyond this) > > + ldr r12, [sp, #16] > > + > > + @ Convert sp to 32-bit physical > > + mov lr, sp > > + ldr r4, =PAR_MASK > > + and r5, lr, r4 @ Extract lower 12 bits of sp > > + mcr p15, 0, lr, c7, c8, 1 @ Write VA -> ATS1CPW > > This is broken without an isb but, more to the point, why can't we just do > the standard lowmem virt_to_phys conversion here instead? I can't use that from within asm (right?). Are you suggesting that I should duplicate the mechanism of virt_to_phys here? > > + mrc p15, 0, lr, c7, c4, 0 @ Physical Address Register > > + mvn r4, r4 > > + and lr, lr, r4 @ Clear lower 12 bits of PA > > + add lr, lr, r5 @ Calculate phys sp > > + mov sp, lr @ Update > > + > > + @ Disable MMU > > + mrc p15, 0, lr, c1, c0, 0 @ ctrl register > > + bic lr, lr, #0x1 @ ...............m > > + mcr p15, 0, lr, c1, c0, 0 @ disable MMU > > + isb > > + > > + @ Make call > > + blx r12 > > This is basically a copy-paste of cpu_v7_reset. Perhaps using some assembler > macros would help here? Well, I explicitly don't want to touch SCTLR.TE. We could macro-ize it, but I think that would increase the amount of code. > > + > > + pop {r4-r5, r12, lr} > > + > > + @ Enable MMU + Caches > > + mrc p15, 0, r1, c1, c0, 0 @ ctrl register > > + orr r1, r1, #0x1000 @ ...i............ > > + orr r1, r1, #0x0005 @ .............c.m > > + mcr p15, 0, r1, c1, c0, 0 @ enable MMU > > + isb > > Why do we have to enable the caches so early? You'd prefer it being done back in phys_call_epilogue()? / Leif

11 years

Re: [Linaro-uefi] [PATCH v3 0/3] (U)EFI runtime services for arm

by Leif Lindholm

On Fri, Nov 29, 2013 at 11:53:19AM +0000, Matt Fleming wrote: > On Thu, 28 Nov, at 04:41:20PM, Leif Lindholm wrote: > > In systems based on [U]EFI-conformant firmware, runtime services provide > > a standardised way for the kernel to update firmware environment > > variables. This is used for example by efibootmgr to update which image > > should be loaded on next boot. > > > > This patchset implements basic support for UEFI runtime services on ARM > > platforms, as well as the basic underlying EFI support. It also defines > > a mechanism by which the required information is passed from the > > bootloader (the EFI stub submitted separately) to the kernel via FDT > > entries. > > This all looks pretty good to me. Is this series going through the ARM > trees? Thanks. That's the plan. / Leif

11 years

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

Linaro-uefi