[PATCH RESEND] efi_stub: update documentation on dtb= parameter
The dtb= parameter is no longer the primary mechanism for providing a devicetree to the kernel. Now either firmware or the boot selector (ex. Grub) should provide the devicetree and dtb= should only be used for debug or when using firmware that doesn't understand DT. Update the EFI stub documentation to reflect the current usage. Signed-off-by: Grant Likely Reviewed-by: Alexander Graf Acked-by: Leif Lindholm Acked-by: Olof Johansson Cc: Ard Biesheuvel Cc: Jonathan Corbet --- Resending because original post included Arm corporate disclaimer. Documentation/efi-stub.txt | 17 ++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/Documentation/efi-stub.txt b/Documentation/efi-stub.txt index 41df801f9a50..833edb0d0bc4 100644 --- a/Documentation/efi-stub.txt +++ b/Documentation/efi-stub.txt @@ -83,7 +83,18 @@ is passed to bzImage.efi. The "dtb=" option - -For the ARM and arm64 architectures, we also need to be able to provide a -device tree to the kernel. This is done with the "dtb=" command line option, -and is processed in the same manner as the "initrd=" option that is +For the ARM and arm64 architectures, a device tree must be provided to +the kernel. Normally firmware shall supply the device tree via the +EFI CONFIGURATION TABLE. However, the "dtb=" command line option can +be used to override the firmware supplied device tree, or to supply +one when firmware is unable to. + +Please note: Firmware adds runtime configuration information to the +device tree before booting the kernel. If dtb= is used to override +the device tree, then any runtime data provided by firmware will be +lost. The dtb= option should only be used either as a debug tool, or +as a last resort when a device tree is not provided in the EFI +CONFIGURATION TABLE. + +"dtb=" is processed in the same manner as the "initrd=" option that is described above. -- 2.11.0
Re: [PATCH] efi_stub: update documentation on dtb= parameter
> On 6 Sep 2018, at 16:51, Jonathan Corbet wrote: > > On Wed, 5 Sep 2018 20:07:50 +0100 > Grant Likely wrote: > >> The dtb= parameter is no longer the primary mechanism for providing a >> devicetree to the kernel. Now either firmware or the boot selector (ex. >> Grub) should provide the devicetree and dtb= should only be used for >> debug or when using firmware that doesn't understand DT. >> Update the EFI stub documentation to reflect the current usage. > > So I hate to be obnoxious, but... > >> IMPORTANT NOTICE: The contents of this email and any attachments are >> confidential and may also be privileged. If you are not the intended >> recipient, please notify the sender immediately and do not disclose the >> contents to any other person, use it for any purpose, or store or copy >> the information in any medium. Thank you. > > Can I get a version of the patch without this language? Then I'll be glad > to apply it. Ugh. Well that was dumb. There is a magic trick I have to perform to get rid of the disclaimer, but it didn’t work this time. I’ll fix and get a new one out. g. > > Thanks, > > jon
[PATCH] efi_stub: update documentation on dtb= parameter
The dtb= parameter is no longer the primary mechanism for providing a devicetree to the kernel. Now either firmware or the boot selector (ex. Grub) should provide the devicetree and dtb= should only be used for debug or when using firmware that doesn't understand DT. Update the EFI stub documentation to reflect the current usage. Signed-off-by: Grant Likely Cc: Ard Biesheuvel Cc: Jonathan Corbet --- Documentation/efi-stub.txt | 17 ++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/Documentation/efi-stub.txt b/Documentation/efi-stub.txt index 41df801f9a50..833edb0d0bc4 100644 --- a/Documentation/efi-stub.txt +++ b/Documentation/efi-stub.txt @@ -83,7 +83,18 @@ is passed to bzImage.efi. The "dtb=" option - -For the ARM and arm64 architectures, we also need to be able to provide a -device tree to the kernel. This is done with the "dtb=" command line option, -and is processed in the same manner as the "initrd=" option that is +For the ARM and arm64 architectures, a device tree must be provided to +the kernel. Normally firmware shall supply the device tree via the +EFI CONFIGURATION TABLE. However, the "dtb=" command line option can +be used to override the firmware supplied device tree, or to supply +one when firmware is unable to. + +Please note: Firmware adds runtime configuration information to the +device tree before booting the kernel. If dtb= is used to override +the device tree, then any runtime data provided by firmware will be +lost. The dtb= option should only be used either as a debug tool, or +as a last resort when a device tree is not provided in the EFI +CONFIGURATION TABLE. + +"dtb=" is processed in the same manner as the "initrd=" option that is described above. -- 2.11.0 IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Re: [PATCH 2/2] drivers: firmware: efi: install new fdt in configuration table
On 25/04/2018 15:48, Udit Kumar wrote: -Original Message- From: Leif Lindholm [mailto:leif.lindh...@linaro.org] Sent: Wednesday, April 25, 2018 4:20 PM To: Grant Likely Cc: Udit Kumar ; Ard Biesheuvel ; Pankaj Bansal ; mark.rutl...@arm.com; linux-efi@vger.kernel.org; Varun Sethi ; Wasim Khan ; n...@arm.com; Rod Dorris Subject: Re: [PATCH 2/2] drivers: firmware: efi: install new fdt in configuration table On Wed, Apr 25, 2018 at 11:04:13AM +0100, Grant Likely wrote: On 25/04/2018 07:49, Udit Kumar wrote: 2) Kernel provides/overrides DTB The kernel always has the option of loading it's own DTB, either because firmware doesn't provide one, or it needs a newer DTB. This is similar to CONFIG_ACPI_CUSTOM_DSDT in ACPI land. However, if the kernel is overriding the DTB, then firmware has no part of it, and it should not be allowed to modify the DTB at ExitBootServices() time. For our platforms at least, this will not work unless firmware is doing modifications. I think this I likely true for Ard as well. Firmware has a logic to modify default DTB for available devices, reading board configuration programming required muxes, based upon this removing/adding devices into DTB. IMO, this will not be an optimal way to use untouched DTB provided by kernel. Right, which is part of the reason for insisting on the firmare being responsible. If the kernel or Grub overrides the DTB, then it is an explicit *rejection* of anything firmware provides; presumable because something is broken and it has to be worked around. So, the functionality in GRUB to replace a firmware-provided devicetree needs to go. And dtb= loading should ideally be made dependent on some sort of DEBUG config option. In my view , dtb= sort of option should go away. If we are booting some debug image in development environment then why not to re-flash DTB I don't want to drop the prototyping use cases on the floor. ie. Wiring up a development board to external hardware. The development board is already supported, but the user is going to be experimenting with DT changes as they go. Reflashing the DT is not a good option in this case. It is more usefule to be able to load the DT modifications at runtime before booting the kernel, because they will change every boot. I can see this scenario playing out for both full DT replacement and DTBO scenarios. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] drivers: firmware: efi: install new fdt in configuration table
On 25/04/2018 07:49, Udit Kumar wrote: 2) Kernel provides/overrides DTB The kernel always has the option of loading it's own DTB, either because firmware doesn't provide one, or it needs a newer DTB. This is similar to CONFIG_ACPI_CUSTOM_DSDT in ACPI land. However, if the kernel is overriding the DTB, then firmware has no part of it, and it should not be allowed to modify the DTB at ExitBootServices() time. For our platforms at least, this will not work unless firmware is doing modifications. I think this I likely true for Ard as well. Firmware has a logic to modify default DTB for available devices, reading board configuration programming required muxes, based upon this removing/adding devices into DTB. IMO, this will not be an optimal way to use untouched DTB provided by kernel. Right, which is part of the reason for insisting on the firmare being responsible. If the kernel or Grub overrides the DTB, then it is an explicit *rejection* of anything firmware provides; presumable because something is broken and it has to be worked around. If the DTB update is given to firmware first, then presumably the work has been done to make sure the new DTB (or base DTB if you will) has been verified to work with firmware's behavour. This is where Ard will argue that if you're updating the DT, then you should update the entirety of firmware because they are closely linked. I don't go quite that far as I can think of a lot of scenarios where just a DTB change is desired that does not conflict with firmware behavour. The details of this are of course implementation specfic and I don't want to create a specification of what can and cannot be changed when doing a DTB updated; but I don't want to discard the use case out of hand. [...] - Firmware should have a built-in DTB that will boot a mainline kernel - Firmware should provide mechanism for installing updated DTB - Reflashing entire firmware just for DTB updates is discouraged - DTB update could be either temporary or persistant - If persistent, DTB should be stored in firmware flash region - ie. not in system partitition. - Firmware can require DTB update to be signed - Built-in DTB remains as failsafe that can always recovered to - Firmware remains 'responsible' for DTB - Firmware allowed to modify updated DTB - Firmware update mechanism logically separate from OS boot - Though it could be scripted by Grub - Firmware 'vendor' responsible for official updates to DTB - Question: should there be a standard capsule update mechanism for DTB updates? - By default, OS distributions should use firmware provided DTB - Require user intervention DTB override is needed I see, this in two ways 1/ Replacement of DTB is needed, In this case, my preference will be to update whole firmware. Fair enough. That is the choice of the firmware vendor 2/ Update in DTB is needed. I think above could be as above, In addition, I see - OS is providing DTB update as DTBO for given distribution - Which could lend as runtime variable in firmware. - Firmware is checking that and storing on flash with give Id - Boot-loader (grub) is checking for runtime variable for DTBO for given distribution - grub is merging firmware provided (DTB) and DTBO before handing over to OS That has possibilities. I would view that as a bug mitigation technique when firmware cannot be updated, and I'm not opposed to it. However, if the DTB update mechanism is standard, and if we have a common database of DTBs, it would be possible for the distribution to automatically manage DTB updates. This way , will not be standard for non-UEFI boot . We don't have any standard for non-UEFI boot regardless. :-) I'd like to tackle the EDK2 & U-Boot based UEFI scenarios first. After that we should take a look at LittleKernel and see if the same things can be applied there. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] drivers: firmware: efi: install new fdt in configuration table
On 18/04/2018 15:26, Udit Kumar wrote: Hi Grant > I am not also in favor to do DTB update like this patch , but DTB update is needed feature !! Oh! Yes. I 100% agree. I am not arguing against DTB updates.. Ideally DTB defined once should be done forever but unfortunately we are not in ideal world. Also agree. This isn't about whether or not DTB updates are required. Of course they are! However, there needs to be a clear division of responsibilities. There are basically two scenarios we need to support: 1) Firmware provides DTB In this case the firmware has a copy of the DTB, which I may modify at boot time and then passes to the kernel. That DTB might be replaced with a different version, or it might have overlays applied, but regardless the firmware is entirely responsible for the DTB. We can have tooling in firmware to make it easy to manage the DTB, including loading a newer DTB, but that step must be distinct from the kernel's OS loader stub. 2) Kernel provides/overrides DTB The kernel always has the option of loading it's own DTB, either because firmware doesn't provide one, or it needs a newer DTB. This is similar to CONFIG_ACPI_CUSTOM_DSDT in ACPI land. However, if the kernel is overriding the DTB, then firmware has no part of it, and it should not be allowed to modify the DTB at ExitBootServices() time. Similarly, Grub might decide to provide the override DTB instead of the kernel, but that is effectively the same scenario. If the OS boot process overrides the platform description, then the firmware no longer has any part of it. The second case is by far the most common scenario today, but it is unfriendly to distributions, which is why it is important to change the behaviour. Here is how I think it should be designed: - Firmware should have a built-in DTB that will boot a mainline kernel - Firmware should provide mechanism for installing updated DTB - Reflashing entire firmware just for DTB updates is discouraged - DTB update could be either temporary or persistant - If persistent, DTB should be stored in firmware flash region - ie. not in system partitition. - Firmware can require DTB update to be signed - Built-in DTB remains as failsafe that can always recovered to - Firmware remains 'responsible' for DTB - Firmware allowed to modify updated DTB - Firmware update mechanism logically separate from OS boot - Though it could be scripted by Grub - Firmware 'vendor' responsible for official updates to DTB - Question: should there be a standard capsule update mechanism for DTB updates? - By default, OS distributions should use firmare provided DTB - Require user intervention DTB override is needed However, if the DTB update mechanism is standard, and if we have a common database of DTBs, it would be possible for the distribution to automatically manage DTB updates. More inline please Regards Udit -----Original Message- From: Grant Likely [mailto:grant.lik...@arm.com] Sent: Wednesday, April 18, 2018 5:03 PM To: Udit Kumar ; Ard Biesheuvel ; Pankaj Bansal ; mark.rutl...@arm.com; leif.lindh...@linaro.org Cc: linux-efi@vger.kernel.org; Varun Sethi ; Wasim Khan ; n...@arm.com; Rod Dorris Subject: Re: [PATCH 2/2] drivers: firmware: efi: install new fdt in configuration table On 18/04/2018 09:44, Udit Kumar wrote: -----Original Message- From: Grant Likely [mailto:grant.lik...@arm.com] Sent: Monday, March 26, 2018 4:10 PM To: Ard Biesheuvel ; Pankaj Bansal ; mark.rutl...@arm.com; leif.lindh...@linaro.org Cc: linux-efi@vger.kernel.org; Varun Sethi ; Wasim Khan ; Udit Kumar ; n...@arm.com Subject: Re: [PATCH 2/2] drivers: firmware: efi: install new fdt in configuration table On 25/03/2018 16:29, Ard Biesheuvel wrote: On 25 Mar 2018, at 14:56, Pankaj Bansal wrote: Bootloader may need to fixup the device tree before OS can use it. e.g. a UEFI/DXE driver that has initialized a controller can add controller's clock frequency in controller node. This way OS need not to call get/set clock for that controller. Therefore, install fdt used by OS in configuration tables and associate it with device tree guid. The firmware should be providing the dtb in the first place. On a uefi system, you should not use the dtbs shipped with the kernel, or use the dtb= command line parameter. This may sound overly strict, but it is the only maintainable way: the dt bindings are the contract between firmware and os, and the os shipped dtbs essentially implement the platform side of the contract, pushing the firmware-os boundary down to a layer that is entirely unstandardized. Exposing the bundled dtb to the firmware is fragile, since the firmware has no guarantees whatsoever about its contents. (dt bindings are [supposed to be] backward compatible, but the device trees themselves are not) So please find another way to solve this, and move away from dtb= Comple
Re: [PATCH 2/2] drivers: firmware: efi: install new fdt in configuration table
On 18/04/2018 09:44, Udit Kumar wrote: -Original Message- From: Grant Likely [mailto:grant.lik...@arm.com] Sent: Monday, March 26, 2018 4:10 PM To: Ard Biesheuvel ; Pankaj Bansal ; mark.rutl...@arm.com; leif.lindh...@linaro.org Cc: linux-efi@vger.kernel.org; Varun Sethi ; Wasim Khan ; Udit Kumar ; n...@arm.com Subject: Re: [PATCH 2/2] drivers: firmware: efi: install new fdt in configuration table On 25/03/2018 16:29, Ard Biesheuvel wrote: On 25 Mar 2018, at 14:56, Pankaj Bansal wrote: Bootloader may need to fixup the device tree before OS can use it. e.g. a UEFI/DXE driver that has initialized a controller can add controller's clock frequency in controller node. This way OS need not to call get/set clock for that controller. Therefore, install fdt used by OS in configuration tables and associate it with device tree guid. The firmware should be providing the dtb in the first place. On a uefi system, you should not use the dtbs shipped with the kernel, or use the dtb= command line parameter. This may sound overly strict, but it is the only maintainable way: the dt bindings are the contract between firmware and os, and the os shipped dtbs essentially implement the platform side of the contract, pushing the firmware-os boundary down to a layer that is entirely unstandardized. Exposing the bundled dtb to the firmware is fragile, since the firmware has no guarantees whatsoever about its contents. (dt bindings are [supposed to be] backward compatible, but the device trees themselves are not) So please find another way to solve this, and move away from dtb= Completely agree here. I do agree that it is sometimes important to override the firmware provided data. We've got precidence for doing this with ACPI also. However, this approach is something different entirely. Sorry to come back late on this topic, With ACPI, we have clear abstraction on code location and ownership. Normally device tree are copied from kernel and build into UEFI , This is different situation here where code location is different than ownership. This is unrelated but u-boot ITB boot, everything is assumed external to boot-loader Kernel and device tree are tied together. That's the model we're trying to break away from. Having the DTB tied to the kernel is a bad model for supporting distributions. I want to make sure the model supports both: 1) DTB responsibilty of firmware - in which case the kernel either uses it or ignores it; but the kernel cannot provide a new dtb that the firmware manipulates 2) DTB responsibility of the kernel - in which case the firmware won't modify the DTB at all. Allowing firmware to modify a kernel-supplied DTB can result in side effects that cannot be overridden by the dtb= kernel parameter. This patch tries to take a kernel-provided .dtb, register it *back with firmware* so that firmware can modify it. That is a completely different contract, and not one that should be encouraged. If firmware provides a DTB, then the acceptable options are either accept that DTB as is, or completely replace it*, with a strong preference for trusting the firmware provided DTB. If the upstream kernel breaks with the firmware- provided DTB, then that is a kernel bug and should be fixed.** Well, kernel must boot even with old device tree But say some driver is adding new feature then with old device tree, this feature may not be used. Therefore device-tree update and needed, and platform specific information needs to be fixed before handing over to OS I don't disagree with needing to update the DTB, but I disagree with the approach. If a DTB update is needed, then the right model is to update the DTB in firmwre *before* going to the DTB boot stage. We should create a standard interface for doing this, but it should not be part of the the kernel UEFI stub, and it should not magically change the DTB based on the kernel's dtb= parameters line. The firmware DTB update could be either temporary or persistant, but it needs to be explicitly a separate step from the kernel boot. Exactly what this looks like can be discussed in the EBBR call. * I'm not talking about overlays here. That's an extension scenario which builds on what firmware provides instead of replaces it. ** Two notes on this; 1) it is okay to require a firmware dtb update to enable new functionality, but it is not okay for the kernel to break things that work. For first part I agree and second is not understood my me ' but it is not okay for the kernel to break things that work' Using kernel provided device tree and fixing by UEFI, why you think kernel will break the things. I only mean that once a platform ships with a DTB embedded in firmware (presuming that DTB will boot a mainline kernel), future releases of that kernel need to continue supporting that platform DTB with the same level of functionality. We'v
Re: [PATCH 2/2] drivers: firmware: efi: install new fdt in configuration table
On 25/03/2018 16:29, Ard Biesheuvel wrote:
On 25 Mar 2018, at 14:56, Pankaj Bansal wrote:
Bootloader may need to fixup the device tree before OS can use it.
e.g. a UEFI/DXE driver that has initialized a controller can add
controller's clock frequency in controller node. This way OS need not to
call get/set clock for that controller.
Therefore, install fdt used by OS in configuration tables and associate it
with device tree guid.
The firmware should be providing the dtb in the first place. On a uefi system,
you should not use the dtbs shipped with the kernel, or use the dtb= command
line parameter.
This may sound overly strict, but it is the only maintainable way: the dt
bindings are the contract between firmware and os, and the os shipped dtbs
essentially implement the platform side of the contract, pushing the
firmware-os boundary down to a layer that is entirely unstandardized. Exposing
the bundled dtb to the firmware is fragile, since the firmware has no
guarantees whatsoever about its contents. (dt bindings are [supposed to be]
backward compatible, but the device trees themselves are not)
So please find another way to solve this, and move away from dtb=
Completely agree here. I do agree that it is sometimes important to
override the firmware provided data. We've got precidence for doing this
with ACPI also. However, this approach is something different entirely.
This patch tries to take a kernel-provided .dtb, register it *back with
firmware* so that firmware can modify it. That is a completely different
contract, and not one that should be encouraged.
If firmware provides a DTB, then the acceptable options are either
accept that DTB as is, or completely replace it*, with a strong
preference for trusting the firmware provided DTB. If the upstream
kernel breaks with the firmware-provided DTB, then that is a kernel bug
and should be fixed.**
* I'm not talking about overlays here. That's an extension scenario
which builds on what firmware provides instead of replaces it.
** Two notes on this;
1) it is okay to require a firmware dtb update to enable new
functionality, but it is not okay for the kernel to break
things that work.
2) We really should have a standard way in Tianocore to update
the built in DTB without reflashing the firmware image.
Cheers,
g.
UEFI/DXE drivers can fixup this device tree in their respective
ExitBootServices events.
Cc: Ard Biesheuvel
Cc: linux-efi@vger.kernel.org
Signed-off-by: Pankaj Bansal
---
drivers/firmware/efi/libstub/fdt.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/firmware/efi/libstub/fdt.c
b/drivers/firmware/efi/libstub/fdt.c
index 177654e..df862e6 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -265,6 +265,7 @@ efi_status_t
allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table_arg,
int runtime_entry_count = 0;
struct efi_boot_memmap map;
struct exit_boot_struct priv;
+efi_guid_t fdt_guid = DEVICE_TREE_GUID;
map.map =&runtime_map;
map.map_size =&map_size;
@@ -314,6 +315,13 @@ efi_status_t
allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table_arg,
goto fail_free_new_fdt;
}
+status = efi_call_early(install_configuration_table, &fdt_guid,
+(void *)*new_fdt_addr);
+if (status != EFI_SUCCESS) {
+pr_efi_err(sys_table_arg, "Unable to install new device tree.\n");
+goto fail_free_new_fdt;
+}
+
priv.runtime_map = runtime_map;
priv.runtime_entry_count = &runtime_entry_count;
priv.new_fdt_addr = (void *)*new_fdt_addr;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] arm64/efi: remove idmap manipulations from UEFI code
On Fri, Oct 24, 2014 at 2:53 PM, Ard Biesheuvel wrote: > On 24 October 2014 15:41, Grant Likely wrote: >> On Fri, Oct 24, 2014 at 1:39 PM, Ard Biesheuvel >> wrote: >>> Now that we have moved the call to SetVirtualAddressMap() to the stub, >>> UEFI has no use for the ID map, so we can drop the code that installs >>> ID mappings for UEFI memory regions. >>> >>> Signed-off-by: Ard Biesheuvel >> >> I have to say, this series makes me happy. :-) >> >> This method will go a long way to catching UEFI implementations that >> do incorrect things after exitbootservices is called. I'm assuming >> that any attempt to access a region that boot services has not >> requested will get trapped by the kernel, correct? >> > > If we really want to catch firmware problems, we should probably wipe > all boot services regions between the calls to ExitBootServices() and > SetVirtualAddressMap(). Mark Salter's original approach here was > fairly cautious here, i.e., reserving boot services regions until > after the call to SetVirtualAddressMap(), but there is no point in > doing that for kexec, that's why I removed it. I quite like that idea. Let's do that and see if anyone screams in agony. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] arm64/efi: remove idmap manipulations from UEFI code
On Fri, Oct 24, 2014 at 1:39 PM, Ard Biesheuvel
wrote:
> Now that we have moved the call to SetVirtualAddressMap() to the stub,
> UEFI has no use for the ID map, so we can drop the code that installs
> ID mappings for UEFI memory regions.
>
> Signed-off-by: Ard Biesheuvel
I have to say, this series makes me happy. :-)
This method will go a long way to catching UEFI implementations that
do incorrect things after exitbootservices is called. I'm assuming
that any attempt to access a region that boot services has not
requested will get trapped by the kernel, correct?
g.
> ---
> arch/arm64/include/asm/efi.h | 4 ++--
> arch/arm64/include/asm/mmu.h | 2 --
> arch/arm64/kernel/efi.c | 26 +-
> arch/arm64/kernel/setup.c| 2 +-
> arch/arm64/mm/mmu.c | 10 --
> 5 files changed, 4 insertions(+), 40 deletions(-)
>
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index d752e5480096..09854df5a2a1 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -6,10 +6,10 @@
>
> #ifdef CONFIG_EFI
> extern void efi_init(void);
> -extern void efi_idmap_init(void);
> +extern void efi_virtmap_init(void);
> #else
> #define efi_init()
> -#define efi_idmap_init()
> +#define efi_virtmap_init()
> #endif
>
> void efi_load_rt_mapping(void);
> diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
> index bcf166043a8b..df33cf02300a 100644
> --- a/arch/arm64/include/asm/mmu.h
> +++ b/arch/arm64/include/asm/mmu.h
> @@ -31,8 +31,6 @@ extern void paging_init(void);
> extern void setup_mm_for_reboot(void);
> extern void __iomem *early_io_map(phys_addr_t phys, unsigned long virt);
> extern void init_mem_pgprot(void);
> -/* create an identity mapping for memory (or io if map_io is true) */
> -extern void create_id_mapping(phys_addr_t addr, phys_addr_t size, int
> map_io);
> extern void create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
>unsigned long virt, phys_addr_t size,
>int map_io, int map_xn);
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index cbff3e8eff11..02bd7e877f0b 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -53,27 +53,6 @@ static int __init is_normal_ram(efi_memory_desc_t *md)
> return 0;
> }
>
> -static void __init efi_setup_idmap(void)
> -{
> - struct memblock_region *r;
> - efi_memory_desc_t *md;
> - u64 paddr, npages, size;
> -
> - for_each_memblock(memory, r)
> - create_id_mapping(r->base, r->size, 0);
> -
> - /* map runtime io spaces */
> - for_each_efi_memory_desc(&memmap, md) {
> - if (!(md->attribute & EFI_MEMORY_RUNTIME) ||
> is_normal_ram(md))
> - continue;
> - paddr = md->phys_addr;
> - npages = md->num_pages;
> - memrange_efi_to_native(&paddr, &npages);
> - size = npages << PAGE_SHIFT;
> - create_id_mapping(paddr, size, 1);
> - }
> -}
> -
> /*
> * Translate a EFI virtual address into a physical address: this is
> necessary,
> * as some data members of the EFI system table are virtually remapped after
> @@ -251,16 +230,13 @@ static struct mm_struct efi_mm = {
> INIT_MM_CONTEXT(efi_mm)
> };
>
> -void __init efi_idmap_init(void)
> +void __init efi_virtmap_init(void)
> {
> efi_memory_desc_t *md;
>
> if (!efi_enabled(EFI_BOOT))
> return;
>
> - /* boot time idmap_pg_dir is incomplete, so fill in missing parts */
> - efi_setup_idmap();
> -
> for_each_efi_memory_desc(&memmap, md) {
> u64 paddr, npages, size;
>
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 2437196cc5d4..a6028490a28f 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -392,7 +392,7 @@ void __init setup_arch(char **cmdline_p)
> paging_init();
> request_standard_resources();
>
> - efi_idmap_init();
> + efi_virtmap_init();
>
> unflatten_device_tree();
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index f7d17a5a1f56..19eae06aab81 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -282,16 +282,6 @@ static void __init create_mapping(phys_addr_t phys,
> unsigned long virt,
> size, 0, 0);
> }
>
> -void __init create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io)
> -{
> - if ((addr >> PGDIR_SHIFT) >= ARRAY_SIZE(idmap_pg_dir)) {
> - pr_warn("BUG: not creating id mapping for %pa\n", &addr);
> - return;
> - }
> - __create_mapping(&init_mm, &idmap_pg_dir[pgd_index(addr)],
> -addr, addr, size, map_io, 0);
> -}
> -
> void __init create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
>unsigned l
Re: [PATCH 4/6] arm64/efi: move SetVirtualAddressMap() to UEFI stub
On Fri, Oct 24, 2014 at 1:39 PM, Ard Biesheuvel
wrote:
> In order to support kexec, the kernel needs to be able to deal with the
> state of the UEFI firmware after SetVirtualAddressMap() has been called.
> To avoid having separate code paths for non-kexec and kexec, let's move
> the call to SetVirtualAddressMap() to the stub: this will guarantee us
> that it will only be called once (since the stub is not executed during
> kexec), and ensures that the UEFI state is identical between kexec and
> normal boot.
>
> This implies that the layout of the virtual mapping needs to be created
> by the stub as well. All regions are rounded up to a naturally aligned
> multiple of 64 KB (for compatibility with 64k pages kernels) and recorded
> in the UEFI memory map. The kernel proper reads those values and installs
> the mappings in a dedicated set of page tables that are swapped in during
> UEFI Runtime Services calls.
>
> Signed-off-by: Ard Biesheuvel
A questions grounded in my ignorance below...
> ---
> arch/arm64/include/asm/efi.h | 19 +++-
> arch/arm64/kernel/efi.c| 205
> +++--
> drivers/firmware/efi/libstub/fdt.c | 104 ++-
> 3 files changed, 224 insertions(+), 104 deletions(-)
>
> @@ -69,9 +74,36 @@ static void __init efi_setup_idmap(void)
> }
> }
>
> +/*
> + * Translate a EFI virtual address into a physical address: this is
> necessary,
> + * as some data members of the EFI system table are virtually remapped after
> + * SetVirtualAddressMap() has been called.
> + */
> +static phys_addr_t __init efi_to_phys(unsigned long addr)
> +{
> + efi_memory_desc_t *md;
> +
> + for_each_efi_memory_desc(&memmap, md) {
> + if (!(md->attribute & EFI_MEMORY_RUNTIME))
> + continue;
> + if (md->virt_addr == 0)
> + /* no virtual mapping has been installed by the stub
> */
> + break;
> + if (md->virt_addr < addr &&
> + (addr - md->virt_addr) < (md->num_pages <<
> EFI_PAGE_SHIFT))
> + return md->phys_addr + addr - md->virt_addr;
> + }
> +
> + WARN_ONCE(1, "UEFI virtual mapping incomplete or missing -- no entry
> found for 0x%lx\n",
> + addr);
> + return addr;
> +}
> +
Is function applicable to all EFI implementations? Should it be in the
common EFI code?
How much of this patch series is (theoretically) applicable to aarch32?
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] arm64/efi: use id mapping for Runtime Services
I had missed this conversation, but Ard pointed me at it yesterday, so
I'll chime in now.
Ard, you are correct that SetVirtualAddressMap() is optional in the
spec and we could simply not call it. By a strict reading of the spec
that is completely valid. However, the problem is not with the spec,
it is with actual implementations. Vendors are notorious for shipping
hardware that works enough to boot the operating system(s) they care
about even though the implementation is out of spec. If we don't call
SetVirtualAddressMap() it pretty much guarantees that we'll never be
able to reliably call SetVirtualAddressMap() on ARM platforms because
there will be a non-trivial set of platforms that implement it
completely wrong.
In the ARM server space, Linux will be the dominant OS for the near
future, not Windows, so we are in the happy position of hardware
vendors making sure their platform boots Linux instead of trying to
run Linux on a platform only tested with Windows. We want to call
SetVirtualAddressMap() because it is a pretty fundamental part of
runtime services, and we want to make sure it works. I'd even argue
that it would be a good idea to randomize the map layout on each boot
to flush out buggy assumptions about the map it will be given.
That said, putting runtime services into a separate mapping apart from
the kernel mappings is exactly the right thing to do. If runtime
services access any data outside of declared address map, we want to
know about it and prevent it from happening. Otherwise we've got no
idea if UEFI is corrupting kernel data structures.
As for kexec, we can pass the assigned mapping on to the next kernel.
We already are passing the address map via the boot dt. As long as the
kernel can detect if a mapping has already been set, which should be
easy, the new kernel can use it instead of assigning one of it's own.
g.
On Wed, Aug 6, 2014 at 4:15 PM, Ard Biesheuvel
wrote:
> On 6 August 2014 16:36, Will Deacon wrote:
>> On Thu, Jul 31, 2014 at 03:11:49PM +0100, Ard Biesheuvel wrote:
>>> There are 2 interesting pieces of information in the UEFI spec section 2.3.6
>>> regarding the mapping of runtime regions:
>>> (a) the firmware should not request a virtual mapping for configuration
>>> tables,
>>> even though they are marked as EfiRuntimeServicesData;
>>> (b) calling SetVirtualAddressMap() is optional, and it is equally
>>> appropriate to
>>> call Runtime Services using an identity mapping.
>>>
>>> So we can eliminate some of the complexity around UEFI Runtime Services by
>>> not
>>> using a virtual mapping at all, and calling the services at their physical
>>> address. This is especially useful under kexec, as SetVirtualAddressMap()
>>> may
>>> only be called once, and there is no guarantee that mappings are stable
>>> between
>>> different kexec'd kernels.
>>>
>>> The fallout for other in-kernel users of UEFI data structures should be
>>> negligible, as they cannot legally access those data structures through
>>> pre-existing virtual mappings anyway (point (a) above)
>>>
>>> It should also be noted that, as the kernel side of the address space
>>> (TTBR1) is
>>> retained, the stack and pointer function arguments remain accessible to the
>>> runtime service while the id mapping is active.
>>>
>>> Signed-off-by: Ard Biesheuvel
>>> ---
>>> arch/arm64/include/asm/efi.h | 24 --
>>> arch/arm64/kernel/efi.c | 106
>>> ++-
>>> 2 files changed, 23 insertions(+), 107 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
>>> index a34fd3b12e2b..d42a21e79b39 100644
>>> --- a/arch/arm64/include/asm/efi.h
>>> +++ b/arch/arm64/include/asm/efi.h
>>> @@ -1,8 +1,10 @@
>>> #ifndef _ASM_EFI_H
>>> #define _ASM_EFI_H
>>>
>>> +#include
>>> #include
>>> #include
>>> +#include
>>>
>>> #ifdef CONFIG_EFI
>>> extern void efi_init(void);
>>> @@ -12,23 +14,37 @@ extern void efi_idmap_init(void);
>>> #define efi_idmap_init()
>>> #endif
>>>
>>> +static inline void switch_pgd(pgd_t *pgd, struct mm_struct *mm)
>>> +{
>>> + cpu_switch_mm(pgd, mm);
>>> + flush_tlb_all();
>>> + if (icache_is_aivivt())
>>> + __flush_icache_all();
>>> +}
>>> +
>>> #define efi_call_virt(f, ...)
>>> \
>>> ({ \
>>> - efi_##f##_t *__f = efi.systab->runtime->f; \
>>> + efi_##f##_t *__f; \
>>> efi_status_t __s; \
>>> \
>>> - kernel_neon_begin();\
>>> + kernel_neon_begin(); /* disables preemption */ \
>>> + switch_pgd(idmap_pg_dir, &init_mm); \
>>> + __f = efi.systab->runtime->f;
Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation
On Tue, 10 Dec 2013 10:29:34 -0800, Roy Franz wrote: > On Tue, Dec 10, 2013 at 4:30 AM, Grant Likely wrote: > > On Fri, 6 Dec 2013 11:20:45 -0600, Matt Sealey wrote: > >> On Wed, Dec 4, 2013 at 4:44 PM, Matthew Garrett > >> wrote: > >> > On Wed, Dec 04, 2013 at 03:06:47PM -0600, Matt Sealey wrote: > >> Grant suggested I should propose some patches; sure, if I'm not otherwise > >> busy. > >> > >> Maybe the Linaro guys can recommend a platform (real or emulated) that > >> would be best to test it on with the available UEFI? > > > > Roy Franz (cc'd) has got UEFI running under QEMU. A few modifications > > were required to both stock UEFI and QEMU. I'm not sure what the status > > of mainlining those patches is. I think there are still a few things > > that Roy has to fix, but you should be able to get the current patches > > from him to get going. > > > > g. > > > > Hi Grant and Matt, > > I have put together a quick wiki page describing the current status, > with git trees for > UEFI and QEMU, and instructions for running the model. I just whipped > this up now, so it > is pretty basic, but should have all the required information. > > https://wiki.linaro.org/LEG/Engineering/Kernel/UEFI/VersatileExpress/QEMU > > Please let me know if you have any questions. Thanks Roy. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation
On Fri, 6 Dec 2013 11:20:45 -0600, Matt Sealey wrote: > On Wed, Dec 4, 2013 at 4:44 PM, Matthew Garrett wrote: > > On Wed, Dec 04, 2013 at 03:06:47PM -0600, Matt Sealey wrote: > Grant suggested I should propose some patches; sure, if I'm not otherwise > busy. > > Maybe the Linaro guys can recommend a platform (real or emulated) that > would be best to test it on with the available UEFI? Roy Franz (cc'd) has got UEFI running under QEMU. A few modifications were required to both stock UEFI and QEMU. I'm not sure what the status of mainlining those patches is. I think there are still a few things that Roy has to fix, but you should be able to get the current patches from him to get going. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] arm64: add EFI stub
On Thu, 5 Dec 2013 15:28:06 +, Catalin Marinas wrote: > On Thu, Dec 05, 2013 at 02:43:23PM +, Mark Salter wrote: > > On Thu, 2013-12-05 at 14:18 +, Catalin Marinas wrote: > > > On Fri, Nov 29, 2013 at 10:05:10PM +, Mark Salter wrote: > > > > This patch adds PE/COFF header fields to the start of the Image > > > > so that it appears as an EFI application to EFI firmware. An EFI > > > > stub is included to allow direct booting of the kernel Image. Due > > > > to EFI firmware limitations, only little endian kernels with 4K > > > > page sizes are supported at this time. > > > > > > I don't fully understand the EFI firmware limitations but for big endian > > > we could have the EFI_STUB wrapper in little endian and get the kernel > > > to switch to big endian once booted. The image header should always be > > > little endian. > > > > That would be fun. :) You'd also have to switch back and forth to make > > EFI runtime services calls. > > OK, we'll have to live with this restriction. Or just disable runtime services on the switch to big ending. Big endian should not disable the stub (but getting it to work could be a follow-up patch) g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] arm64: add EFI stub
On Fri, 29 Nov 2013 17:05:10 -0500, Mark Salter wrote: > This patch adds PE/COFF header fields to the start of the Image > so that it appears as an EFI application to EFI firmware. An EFI > stub is included to allow direct booting of the kernel Image. Due > to EFI firmware limitations, only little endian kernels with 4K > page sizes are supported at this time. Support in the COFF header > for signed images was provided by Ard Biesheuvel. > > Signed-off-by: Mark Salter > Signed-off-by: Ard Biesheuvel Reviewed-by: Grant Likely I've already made comments on Roy's arm32 version of this code. I don't like the duplication and it needs to be consolidated, but I would be fine with consolidation being done as follow-on patches if that expedites getting the code in. g. > CC: Catalin Marinas > CC: Will Deacon > CC: linux-arm-ker...@lists.infradead.org > CC: matt.flem...@intel.com > CC: linux-efi@vger.kernel.org > CC: Leif Lindholm > CC: roy.fr...@linaro.org > --- > arch/arm64/Kconfig| 10 ++ > arch/arm64/kernel/Makefile| 3 + > arch/arm64/kernel/efi-entry.S | 81 > arch/arm64/kernel/efi-stub.c | 280 > ++ > arch/arm64/kernel/head.S | 112 + > 5 files changed, 486 insertions(+) > create mode 100644 arch/arm64/kernel/efi-entry.S > create mode 100644 arch/arm64/kernel/efi-stub.c > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 809c1b8..10b0e93 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -250,6 +250,16 @@ config CMDLINE_FORCE > This is useful if you cannot or don't want to change the > command-line options your boot loader passes to the kernel. > > +config EFI_STUB > + bool "EFI stub support" > + depends on !CPU_BIG_ENDIAN && !ARM64_64K_PAGES && OF > + select LIBFDT > + default y > + help > + This kernel feature allows an Image to be loaded directly > + by EFI firmware without the use of a bootloader. > + See Documentation/efi-stub.txt for more information. > + > endmenu > > menu "Userspace binary formats" > diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile > index 5ba2fd4..1c52b84 100644 > --- a/arch/arm64/kernel/Makefile > +++ b/arch/arm64/kernel/Makefile > @@ -4,6 +4,8 @@ > > CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$(TEXT_OFFSET) > AFLAGS_head.o:= -DTEXT_OFFSET=$(TEXT_OFFSET) > +CFLAGS_efi-stub.o:= -DTEXT_OFFSET=$(TEXT_OFFSET) \ > +-I$(src)/../../../scripts/dtc/libfdt > > # Object file lists. > arm64-obj-y := cputable.o debug-monitors.o entry.o irq.o fpsimd.o > \ > @@ -18,6 +20,7 @@ arm64-obj-$(CONFIG_SMP) += smp.o > smp_spin_table.o > arm64-obj-$(CONFIG_HW_PERF_EVENTS) += perf_event.o > arm64-obj-$(CONFIG_HAVE_HW_BREAKPOINT)+= hw_breakpoint.o > arm64-obj-$(CONFIG_EARLY_PRINTK) += early_printk.o > +arm64-obj-$(CONFIG_EFI_STUB) += efi-stub.o efi-entry.o > > obj-y+= $(arm64-obj-y) vdso/ > obj-m+= $(arm64-obj-m) > diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S > new file mode 100644 > index 000..5f6d179 > --- /dev/null > +++ b/arch/arm64/kernel/efi-entry.S > @@ -0,0 +1,81 @@ > +/* > + * EFI entry point. > + * > + * Copyright (C) 2013 Red Hat, Inc. > + * Author: Mark Salter > + * > + * 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 > +#include > + > +#include > + > +#define EFI_LOAD_ERROR 0x8001 > + > + __INIT > + > + /* > + * We arrive here from the EFI boot manager with: > + * > + ** MMU on with identity-mapped RAM. > + ** Icache and Dcache on > + * > + * We will most likely be running from some place other than where > + * we want to be. The kernel image wants to be placed at TEXT_OFFSET > + * from start of RAM. > + */ > +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. Current runtime > + * address of image is calculated and passed via *image_addr. > + * > + * unsigned long efi_entry(void *handle, > + * efi_system_table_t *sys_table, > + *
Re: [PATCH V5 6/6] Add config EFI_STUB for ARM to Kconfig
On Wed, 27 Nov 2013 15:31:55 -0800, Roy Franz wrote: > Signed-off-by: Roy Franz Dude. Must have commit text. You know better. :-) > --- > arch/arm/Kconfig | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index a1b4758..6601985 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -1857,6 +1857,16 @@ config EFI > This option is only useful on systems that have UEFI firmware. > However, even with this option, the resultant kernel will > continue to boot on non-UEFI platforms. > +config EFI_STUB Need a blank line between the previous stanza and config EFI_STUB > + bool "EFI stub support" > + depends on EFI && !CPU_BIG_ENDIAN > + ---help--- > + This kernel feature allows a zImage to be loaded directly > + by EFI firmware without the use of a bootloader. A PE/COFF > + header is added to the zImage in a way that makes the binary > + both a Linux zImage and an PE/COFF executable that can be > + executed directly by EFI firmware. > + See Documentation/efi-stub.txt for more information. Nit: 80 column word wrap is customary. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 4/6] Add EFI stub for ARM
image_size,
> + zimage_size, 0, 0);
> + if (status != EFI_SUCCESS) {
> + efi_printk(sys_table, PRINTK_PREFIX"ERROR: Failed to relocate
> kernel.\n");
> + goto fail_free_kernel_reserve;
> + }
Slight differences here.
> +
> + /* Check to see if we were able to allocate memory low enough
> + * in memory.
> + */
> + if (*zimage_addr + zimage_size > dram_base + ZIMAGE_OFFSET_LIMIT) {
> + efi_printk(sys_table, PRINTK_PREFIX"ERROR: Failed to relocate
> kernel, no low memory available.\n");
> + goto fail_free_zimage;
> + }
Back to same code below this point, and is the same through to the
bottom of the function. That is a lot of identical code. I think you can
generalize the whole function with arch specific callouts in a couple of
places.
I really don't want to hold up the merging of this series. Aside from
the duplication I think it is ready to be merged. However, the
duplication is so obvious that I'm not comfortable with it. If I were
an ARM or ARM64 core maintainer then I would push back on it.
Go ahead an add my Acked-by for this patch. I'll support merging it as
is provided that you promise me to merge the two versions with a
follow-up patch ASAP. However, if you can I would still recommend
respinning the series with the common bits split out right away since
there's a much greater chance of getting the arm and arm64 maintainers
to accept them.
Acked-by: Grant Likely
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 2/6] Add shared update_fdt() function for ARM/ARM64
On Wed, 27 Nov 2013 15:31:51 -0800, Roy Franz wrote: > Both ARM and ARM64 stubs will update the device tree > that they pass to the kernel. In both cases they > primarily need to add the same UEFI related information, > so the function can be shared. > Create a new FDT related file for this to avoid use > of architecture #ifdefs in efi-stub-helper.c > Change EFI allocation type for memory map to > EFI_RUNTIME_SERVICES_DATA, since we are passing > this buffer to the kernel, or immediately freeing it. Nit: You're using a very short line length for the commit block. 72 columns is more customary. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 2/6] Add shared update_fdt() function for ARM/ARM64
On Wed, 27 Nov 2013 15:31:51 -0800, Roy Franz wrote:
> Both ARM and ARM64 stubs will update the device tree
> that they pass to the kernel. In both cases they
> primarily need to add the same UEFI related information,
> so the function can be shared.
> Create a new FDT related file for this to avoid use
> of architecture #ifdefs in efi-stub-helper.c
> Change EFI allocation type for memory map to
> EFI_RUNTIME_SERVICES_DATA, since we are passing
> this buffer to the kernel, or immediately freeing it.
>
> Signed-off-by: Roy Franz
Hi Roy,
Comments below...
> +static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
> +void *fdt, int new_fdt_size, char *cmdline_ptr,
> +u64 initrd_addr, u64 initrd_size,
> +efi_memory_desc_t *memory_map,
> +unsigned long map_size, unsigned long desc_size,
> +u32 desc_ver)
> +{
> + int node;
> + int status;
> + u32 fdt_val32;
> + u64 fdt_val64;
> + /* Copy definition of linux_banner here. Since this code is
> + * built as part of the decompressor for ARM v7, pulling
> + * in version.c where linux_banner is defined for the
> + * kernel brings other kernel dependencies with it.
> + */
> + const char linux_banner[] =
> + "Linux version " UTS_RELEASE " (" LINUX_COMPILE_BY "@"
> + LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION "\n";
This is inherently fragile because the string is now defined in two
places. This issue shouldn't hold off merging the patch, but a follow up
patch should be crafted to make it possible to pull in version.c without
the other dependencies
Then again, the version.c strings have a very big "DON'T TOUCH" warning
on them so I'm probably worrying about nothing. :-)
> +
> + status = fdt_open_into(orig_fdt, fdt, new_fdt_size);
> + if (status != 0)
> + goto fdt_set_fail;
> +
> + 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;
> + }
> + }
> +
> + if ((cmdline_ptr != NULL) && (strlen(cmdline_ptr) > 0)) {
> + status = fdt_setprop(fdt, node, "bootargs", cmdline_ptr,
> + strlen(cmdline_ptr) + 1);
> + if (status)
> + goto fdt_set_fail;
> + }
> +
> + /* Set intird address/end in device tree, if present */
sp. initrd
None of the above comments are blockers though.
Acked-by: Grant Likely
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 1/6] efi-stub.txt updates for ARM
On Wed, 27 Nov 2013 15:31:50 -0800, Roy Franz wrote: > Update efi-stub.txt documentation to be more general > and not x86 specific. Add ARM only "dtb=" command > line option description. > > Signed-off-by: Roy Franz Acked-by: Grant Likely > --- > Documentation/efi-stub.txt | 27 --- > 1 file changed, 20 insertions(+), 7 deletions(-) > > diff --git a/Documentation/efi-stub.txt b/Documentation/efi-stub.txt > index 44e6bb6..19e897c 100644 > --- a/Documentation/efi-stub.txt > +++ b/Documentation/efi-stub.txt > @@ -1,13 +1,16 @@ > The EFI Boot Stub >--- > > -On the x86 platform, a bzImage can masquerade as a PE/COFF image, > -thereby convincing EFI firmware loaders to load it as an EFI > -executable. The code that modifies the bzImage header, along with the > -EFI-specific entry point that the firmware loader jumps to are > -collectively known as the "EFI boot stub", and live in > +On the x86 and ARM platforms, a kernel zImage/bzImage can masquerade > +as a PE/COFF image, thereby convincing EFI firmware loaders to load > +it as an EFI executable. The code that modifies the bzImage header, > +along with the EFI-specific entry point that the firmware loader > +jumps to are collectively known as the "EFI boot stub", and live in > arch/x86/boot/header.S and arch/x86/boot/compressed/eboot.c, > -respectively. > +respectively. For ARM the EFI stub is implemented in > +arch/arm/boot/compressed/efi-header.S and > +arch/arm/boot/compressed/efi-stub.c. EFI stub code that is shared > +between architectures is in drivers/firmware/efi/efi-stub-helper.c. > > By using the EFI boot stub it's possible to boot a Linux kernel > without the use of a conventional EFI boot loader, such as grub or > @@ -23,7 +26,9 @@ The bzImage located in arch/x86/boot/bzImage must be copied > to the EFI > System Partiion (ESP) and renamed with the extension ".efi". Without > the extension the EFI firmware loader will refuse to execute it. It's > not possible to execute bzImage.efi from the usual Linux file systems > -because EFI firmware doesn't have support for them. > +because EFI firmware doesn't have support for them. For ARM the > +arch/arm/boot/zImage should be copied to the system partition, and it > +may not need to be renamed. > > > Passing kernel parameters from the EFI shell > @@ -63,3 +68,11 @@ Notice how bzImage.efi can be specified with a relative > path. That's > because the image we're executing is interpreted by the EFI shell, > which understands relative paths, whereas the rest of the command line > is passed to bzImage.efi. > + > + > + The "dtb=" option > + > +For the ARM architecture, we also need to be able to provide a device > +tree to the kernel. This is done with the "dtb=" command line option, > +and is process in the same manner as the "initrd=" option that is described > +above. > -- > 1.7.10.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] doc: arm64: add description of EFI stub support
On Fri, 29 Nov 2013 17:05:11 -0500, Mark Salter wrote: Even documentation updates should have a mildly sane commit text. State why these changes came about. Give a future reader at least some clues as to why these changes were made now. Otherwise: Acked-by: Grant Likely g. > Signed-off-by: Mark Salter > CC: Catalin Marinas > CC: Will Deacon > CC: linux-arm-ker...@lists.infradead.org > CC: matt.flem...@intel.com > CC: linux-efi@vger.kernel.org > CC: linux-...@vger.kernel.org > CC: Rob Landley > CC: Leif Lindholm > CC: roy.fr...@linaro.org > --- > Documentation/arm64/booting.txt | 4 > Documentation/efi-stub.txt | 10 -- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt > index a9691cc..aa95d38c 100644 > --- a/Documentation/arm64/booting.txt > +++ b/Documentation/arm64/booting.txt > @@ -85,6 +85,10 @@ The decompressed kernel image contains a 64-byte header as > follows: > Header notes: > > - code0/code1 are responsible for branching to stext. > +- when booting through EFI, code0/code1 are initially skipped. > + res5 is an offset to the PE header and the PE header has the EFI > + entry point (efi_stub_entry). When the stub has done its work, it > + jumps to code0 to resume the normal boot process. > > The image must be placed at the specified offset (currently 0x8) > from the start of the system RAM and called there. The start of the > diff --git a/Documentation/efi-stub.txt b/Documentation/efi-stub.txt > index 19e897c..c2a4b11 100644 > --- a/Documentation/efi-stub.txt > +++ b/Documentation/efi-stub.txt > @@ -12,6 +12,11 @@ arch/arm/boot/compressed/efi-header.S and > arch/arm/boot/compressed/efi-stub.c. EFI stub code that is shared > between architectures is in drivers/firmware/efi/efi-stub-helper.c. > > +For arm64, there is no compressed kernel support, so the Image itself > +masquerades as a PE/COFF image and the EFI stub is linked into the > +kernel. The arm64 EFI stub lives in arch/arm64/kernel/efi-entry.S > +and arch/arm64/kernel/efi-stub.c. > + > By using the EFI boot stub it's possible to boot a Linux kernel > without the use of a conventional EFI boot loader, such as grub or > elilo. Since the EFI boot stub performs the jobs of a boot loader, in > @@ -28,7 +33,8 @@ the extension the EFI firmware loader will refuse to > execute it. It's > not possible to execute bzImage.efi from the usual Linux file systems > because EFI firmware doesn't have support for them. For ARM the > arch/arm/boot/zImage should be copied to the system partition, and it > -may not need to be renamed. > +may not need to be renamed. Similarly for arm64, arch/arm64/boot/Image > +should be copied but not necessarily renamed. > > > Passing kernel parameters from the EFI shell > @@ -72,7 +78,7 @@ is passed to bzImage.efi. > > The "dtb=" option > > -For the ARM architecture, we also need to be able to provide a device > +For ARM and arm64 architecture, we also need to be able to provide a device > tree to the kernel. This is done with the "dtb=" command line option, > and is process in the same manner as the "initrd=" option that is described > above. > -- > 1.8.3.1 > > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/3] (U)EFI runtime services for arm
On Fri, 29 Nov 2013 18:58:51 +0100, Leif Lindholm wrote: > On Fri, Nov 29, 2013 at 11:53:19AM +, 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. FWIW, I would like to see this merged this cycle. I think it is ready enough and can be fixed up in-place. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] init: efi: arm: enable (U)EFI runtime services on arm
On Thu, 28 Nov 2013 16:41:23 +, Leif Lindholm wrote: > Since the efi_set_virtual_address_map call has strict init ordering > requirements, add an explicit hook in the required place. > > Signed-off-by: Leif Lindholm > --- > init/main.c |4 > 1 file changed, 4 insertions(+) > > diff --git a/init/main.c b/init/main.c > index febc511..1331829 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -905,6 +905,10 @@ static noinline void __init kernel_init_freeable(void) > smp_prepare_cpus(setup_max_cpus); > > do_pre_smp_initcalls(); > + > + if (IS_ENABLED(CONFIG_ARM) && efi_enabled(EFI_BOOT)) > + efi_enter_virtual_mode(); > + Personally, I would put the IS_ENABLED() and efi_enabled() tests into efi_enter_virtual_mode() itself (or an empty stub for the !IS_ENABLED() case), but that is mostly a nit. Acked-by: Grant Likely > lockup_detector_init(); > > smp_init(); > -- > 1.7.10.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] arm: Add [U]EFI runtime services support
On Thu, 28 Nov 2013 16:41:22 +, Leif Lindholm 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. > > Changes since v2: > - Updated FDT bindings. > - Preserve regions marked RESERVED (but don't map them). > - Rename 'efi' -> 'uefi' within this new port (leaving core code as is). > > Signed-off-by: Leif Lindholm Hi Leif, I've made a bunch of comments below. I've got concerns about the amount of casting going on in this code, but the rest are pretty minor Reviewed-by: Grant Likely > --- > arch/arm/Kconfig| 15 ++ > arch/arm/include/asm/uefi.h | 22 ++ > arch/arm/kernel/Makefile|2 + > arch/arm/kernel/setup.c |6 + > arch/arm/kernel/uefi.c | 469 > +++ > arch/arm/kernel/uefi_phys.S | 59 ++ > include/linux/efi.h |2 +- > 7 files changed, 574 insertions(+), 1 deletion(-) > create mode 100644 arch/arm/include/asm/uefi.h > create mode 100644 arch/arm/kernel/uefi.c > create mode 100644 arch/arm/kernel/uefi_phys.S > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index 78a79a6a..db8d212 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -1853,6 +1853,19 @@ config EARLY_IOREMAP > the same virtual memory range as kmap so all early mappings must > be unapped before paging_init() is called. > > +config EFI > + bool "UEFI runtime service support" > + depends on OF && !CPU_BIG_ENDIAN > + select UCS2_STRING > + select EARLY_IOREMAP > + ---help--- > + This enables the kernel to use UEFI runtime services that are > + available (such as the UEFI variable services). > + > + This option is only useful on systems that have UEFI firmware. > + However, even with this option, the resultant kernel will > + continue to boot on non-UEFI platforms. > + > config SECCOMP > bool > prompt "Enable seccomp to safely compute untrusted bytecode" > @@ -2272,6 +2285,8 @@ source "net/Kconfig" > > source "drivers/Kconfig" > > +source "drivers/firmware/Kconfig" > + > source "fs/Kconfig" > > source "arch/arm/Kconfig.debug" > diff --git a/arch/arm/include/asm/uefi.h b/arch/arm/include/asm/uefi.h > new file mode 100644 > index 000..519ca18 > --- /dev/null > +++ b/arch/arm/include/asm/uefi.h > @@ -0,0 +1,22 @@ > +#ifndef _ASM_ARM_EFI_H > +#define _ASM_ARM_EFI_H > + > +#include > + > +extern int uefi_memblock_arm_reserve_range(void); > + > +typedef efi_status_t uefi_phys_call_t(u32 memory_map_size, > + u32 descriptor_size, > + u32 descriptor_version, > + efi_memory_desc_t *dsc, > + efi_set_virtual_address_map_t *f); > + > +extern efi_status_t uefi_phys_call(u32, u32, u32, efi_memory_desc_t *, > +efi_set_virtual_address_map_t *); > + > +#define uefi_remap(cookie, size) __arm_ioremap((cookie), (size), MT_MEMORY) > +#define uefi_ioremap(cookie, size) __arm_ioremap((cookie), (size), MT_DEVICE) > +#define uefi_unmap(cookie) __arm_iounmap((cookie)) > +#define uefi_iounmap(cookie) __arm_iounmap((cookie)) > + > +#endif /* _ASM_ARM_EFI_H */ > diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile > index a30fc9b..736cce4 100644 > --- a/arch/arm/kernel/Makefile > +++ b/arch/arm/kernel/Makefile > @@ -98,4 +98,6 @@ obj-y += psci.o > obj-$(CONFIG_SMP)+= psci_smp.o > endif > > +obj-$(CONFIG_EFI)+= uefi.o uefi_phys.o > + > extra-y := $(head-y) vmlinux.lds > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c > index 04c1757..9d44edd 100644 > --- a/arch/arm/kernel/setup.c > +++ b/arch/arm/kernel/setup.c > @@ -30,6 +30,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -57,6 +58,7 @@ > #include > #include > #include > +#include > > #include "atags.h" > > @@ -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 > + #ifdef block in code? You know better. :-) Make an emp
Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation
On Thu, 28 Nov 2013 16:41:21 +, Leif Lindholm wrote: > This patch provides documentation of the [U]EFI runtime service and > configuration features for the arm architecture. > > Changes since v1/v2: > - Complete rewrite. > - New FDT bindings. > > Cc: Rob Landley > Cc: linux-...@vger.kernel.org > > Signed-off-by: Leif Lindholm Acked-by: Grant Likely With a few minor comments below. g. > --- > Documentation/arm/00-INDEX |3 +++ > Documentation/arm/uefi.txt | 61 > > 2 files changed, 64 insertions(+) > create mode 100644 Documentation/arm/uefi.txt > > diff --git a/Documentation/arm/00-INDEX b/Documentation/arm/00-INDEX > index 36420e1..b3af704 100644 > --- a/Documentation/arm/00-INDEX > +++ b/Documentation/arm/00-INDEX > @@ -34,3 +34,6 @@ nwfpe/ > - NWFPE floating point emulator documentation > swp_emulation > - SWP/SWPB emulation handler/logging description > + > +uefi.txt > + - [U]EFI configuration and runtime services documentation > diff --git a/Documentation/arm/uefi.txt b/Documentation/arm/uefi.txt > new file mode 100644 > index 000..9ba59509 > --- /dev/null > +++ b/Documentation/arm/uefi.txt > @@ -0,0 +1,61 @@ > +UEFI, the Unified Extensible Firmware Interface is, a specification Nit: Comma in the wrong place. > +governing the behaviours of compatible firmware interfaces. It is > +maintained by the UEFI Forum - http://www.uefi.org/. > + > +UEFI is an evolution of its predecessor 'EFI', so the terms EFI and > +UEFI are used somewhat interchangeably in this document and associated > +source code. As a rule, anything new uses 'UEFI', whereas 'EFI' refers > +to legacy code or specifications. > + > +UEFI support in Linux > += > +Booting on a platform with firmware compliant with the UEFI specification > +makes it possible for the kernel to support additional features: > +- UEFI Runtime Services > +- Retrieving various configuration information through the standardised > + interface of UEFI configuration tables. (ACPI, SMBIOS, ...) > + > +For actually enabling [U]EFI support, enable: > +- CONFIG_EFI=y > +- CONFIG_EFI_VARS=y or m > + > +The implementation depends on receiving information about the UEFI > environment > +in a Flattened Device Tree (FDT) - so is only available with CONFIG_OF. > + > +UEFI stub > += > +The "stub" is a feature that turns the Image/zImage into a valid UEFI PE/COFF Nit: s/turns/extends/ > +executable, including a loader application that makes it possible to load the > +kernel directly from the UEFI shell, boot menu, or one of the lightweight > +bootloaders like Gummiboot or rEFInd. > + > +The kernel image built with stub support remains a valid kernel image for > +booting in non-UEFI environments. > + > +UEFI kernel support on ARM > +== > +UEFI kernel support on the ARM architectures (arm and arm64) is only > available > +when boot is performed through the stub. > + > +The stub populates the FDT /chosen node with (and the kernel scans for) the > +following parameters: > + > +Name | Size | Description > + > +linux,uefi-system-table | 64-bit | Physical address of the UEFI System > Table. > + > +linux,uefi-mmap-start | 64-bit | Physical address of the UEFI memory map, > + || populated by the UEFI GetMemoryMap() > call. > + > +linux,uefi-mmap-size | 32-bit | Size in bytes of the UEFI memory map > + || pointed to in previous entry. > + > +linux,uefi-mmap-desc-size | 32-bit | Size in bytes of each entry in the UEFI > + || memory map. > + > +linux,uefi-mmap-desc-ver | 32-bit | Version of the mmap descriptor format. > + > +linux,uefi-stub-kern-ver | string | Copy of linux_banner from build. > + > + > +For verbose debug messages, specify 'uefi_debug' on the kernel command line. > -- > 1.7.10.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation
On Wed, 4 Dec 2013 15:06:47 -0600, Matt Sealey wrote: > If your platform has UEFI, then your platform has UEFI - if you built > a multiplatform kernel that needs to boot on U-Boot, then you glued an > EFI stub to it to make it boot. At some point between the stub and the > runtime services driver, you're going through 10,000 lines of code > with the information that it *is* running on top of UEFI completely > lost to the boot process. > > I believe I am also objecting to the idea that the way this is BEST > implemented is to take a stock zImage (decompressor+Image payload) and > glue a stub in front to resolve the interface issue when the > implication is extra complication to the boot process. Adding UEFI support to an existing image type was a design goal when we started. Having yet another image format which is not compatibile with existing firmware adds yet another barrier to migrating from U-Boot to UEFI, or to supporting multiplatforms. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] Documentation: arm: add UEFI support documentation
On Mon, 2 Dec 2013 13:51:22 -0600, Matt Sealey wrote: > > +UEFI kernel support on ARM > > +== > > +UEFI kernel support on the ARM architectures (arm and arm64) is only > > available > > +when boot is performed through the stub. > > + > > +The stub populates the FDT /chosen node with (and the kernel scans for) the > > +following parameters: > > + > > +Name | Size | Description > > + > > +linux,uefi-system-table | 64-bit | Physical address of the UEFI System > > Table. > > + > > +linux,uefi-mmap-start | 64-bit | Physical address of the UEFI memory > > map, > > + || populated by the UEFI GetMemoryMap() > > call. > > + > > +linux,uefi-mmap-size | 32-bit | Size in bytes of the UEFI memory map > > + || pointed to in previous entry. > > + > > +linux,uefi-mmap-desc-size | 32-bit | Size in bytes of each entry in the > > UEFI > > + || memory map. > > + > > +linux,uefi-mmap-desc-ver | 32-bit | Version of the mmap descriptor format. > > + > > +linux,uefi-stub-kern-ver | string | Copy of linux_banner from build. > > + > > This flies in the face of actually using #address-cells and > #size-cells to define how big addresses and sizes are. You're limited > here by the root node definitions... that's the spec. > > 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? Everything here is about getting from the stub to the kernel. We have a boot interface for the kernel proper. It works, and this patch set works with it. Also, this is effectively a kernel-internal interface between the stub and the kernel so there aren't any ABI issues that we need to deail with. Go ahead and propose patches for a better interface, but in the mean time I see no reason not to merge this series. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: arm/arm64 UEFI boot protocol
On Wed, Nov 13, 2013 at 3:00 AM, Leif Lindholm wrote: > On Tue, Nov 12, 2013 at 04:42:41PM +, Will Deacon wrote: >> > __ >> > Name | Size | Description >> > >> > linux,uefi-system-table | 64-bit | Physical address of the UEFI System >> > Table. >> > >> > linux,uefi-mmap | 64-bit | Physical address of the UEFI memory >> > map, >> > || populated by the UEFI GetMemoryMap() >> > call. >> > >> > linux,uefi-mmap-size | 32-bit | Size in bytes of the UEFI memory map >> > || pointed to in previous entry. >> > >> > linux,uefi-mmap-desc-size | 32-bit | Size in bytes of each entry in the >> > UEFI >> > || memory map. >> >> Do we actually need to define these sizes here, or can they be dealt with >> using the usual #address-cells property? Also, I think you should describe >> the binding in a separate document somewhere under Documentation/devicetree, >> then cross-reference it from here. > > This is not a generic ABI, so I don't think it belongs in there (it will > never be in a .dtb, and the only way it can be generated by something > that isn't the stub is by lying). > But I don't really care either way - as long as some general agreement > can be had. There is precedence with inserting the initrd location into the kernel with similar properties. I think the binding does the right thing here. In the interest of bike shedding, I would name the properties "linux,uefi-mmap-start" and "linux,uefi-mmap-size", but otherwise it is fine by me. Should you have a property for the descriptor version as returned by GetMemoryMap()? > >> > >> > linux,uefi-stub-kern-ver | string | Copy of linux_banner from build. >> > >> >> Are you sure you want to refer to kernel symbols here? If somebody renames >> that variable, they're not going to fix this file. > > There is a comment above the definition of linux_banner that says: > /* FIXED STRINGS! Don't touch! */ > > Sounded reliable enough to me :) I think this is just fine. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/16] Fix types in EFI calls to match EFI function definitions.
On Sat, Aug 31, 2013 at 12:12 AM, Roy Franz wrote: > On Fri, Aug 30, 2013 at 6:39 AM, Grant Likely > wrote: >> I think you need some description in the commit text about why the type >> of chunksize is changing. It's not clear why you're making this change. >> It is a bug fix? > > The type was changed to match the EFI_FILE_PROTOCOL specification - > chunk size is > defined as a native width unsigned integer. So yes, this is a bug in > that this is wrong for > ia32, since that really should be a u32 in that case. > Since the x86 wrappers break type checking by the compiler these types > were never checked at compile time > before. chunksize is the the only change that isn't a cast to a type > of void pointer. > > I can break this change out as a separate patch if desired. I don't think it needs to be in a separate patch, but it does need to be described in the patch description. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/16] Add minimum address parameter to efi_low_alloc()
On Fri, Aug 30, 2013 at 11:12 PM, Roy Franz wrote: > On Fri, Aug 30, 2013 at 6:01 AM, Grant Likely > wrote: >> On Fri, 9 Aug 2013 16:26:05 -0700, Roy Franz wrote: >>> This allows allocations to be made low in memory while >>> avoiding allocations at the base of memory. >> >> Your commit message should include /why/ the change is needed. From the >> above I understand what the patch does, but I don't understand why it is >> necessary. > > This was used to avoid relocating the zImage so low in memory that it > would conflict > with memory range that it was decompressed into. This is now handled > by allocating > the memory for the decompressed kernel, so the lower limit is no > longer required. > I'll remove it, as it is not necessary any more. Even better! :-) g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/16] Add EFI stub for ARM
On Fri, 9 Aug 2013 16:26:16 -0700, Roy Franz wrote: > This patch adds EFI stub support for the ARM Linux kernel. The EFI stub > operations similarly to the x86 stub: it is a shim between the EFI firmware > and the normal zImage entry point, and sets up the environment that the > zImage is expecting. This includes loading the initrd (optionaly) and > device tree from the system partition based on the kernel command line. > The stub updates the device tree as necessary, including adding reserved > memory regions and adding entries for EFI runtime services. The PE/COFF > "MZ" header at offset 0 results in the first instruction being an add > that corrupts r5, which is not used by the zImage interface. > > Signed-off-by: Roy Franz Hi Roy, Looks like nice tight code. I've got some comments below, but in general I'm pretty happy with it. g. > --- > arch/arm/boot/compressed/Makefile | 15 +- > arch/arm/boot/compressed/efi-header.S | 111 > arch/arm/boot/compressed/efi-stub.c | 448 > + > arch/arm/boot/compressed/efi-stub.h |5 + > arch/arm/boot/compressed/head.S | 90 ++- > 5 files changed, 661 insertions(+), 8 deletions(-) > create mode 100644 arch/arm/boot/compressed/efi-header.S > create mode 100644 arch/arm/boot/compressed/efi-stub.c > create mode 100644 arch/arm/boot/compressed/efi-stub.h > > diff --git a/arch/arm/boot/compressed/Makefile > b/arch/arm/boot/compressed/Makefile > index 7ac1610..5fad8bd 100644 > --- a/arch/arm/boot/compressed/Makefile > +++ b/arch/arm/boot/compressed/Makefile > @@ -103,11 +103,22 @@ libfdt_objs := $(addsuffix .o, $(basename > $(libfdt))) > $(addprefix $(obj)/,$(libfdt) $(libfdt_hdrs)): $(obj)/%: > $(srctree)/scripts/dtc/libfdt/% > $(call cmd,shipped) > > -$(addprefix $(obj)/,$(libfdt_objs) atags_to_fdt.o): \ > +$(addprefix $(obj)/,$(libfdt_objs) atags_to_fdt.o efi-stub.o): \ > $(addprefix $(obj)/,$(libfdt_hdrs)) > > ifeq ($(CONFIG_ARM_ATAG_DTB_COMPAT),y) > -OBJS += $(libfdt_objs) atags_to_fdt.o > +OBJS += atags_to_fdt.o > +USE_LIBFDT = y > +endif > + > +ifeq ($(CONFIG_EFI_STUB),y) > +CFLAGS_efi-stub.o += -DTEXT_OFFSET=$(TEXT_OFFSET) > +OBJS += efi-stub.o > +USE_LIBFDT = y > +endif > + > +ifeq ($(USE_LIBFDT),y) > +OBJS += $(libfdt_objs) > endif > > targets := vmlinux vmlinux.lds \ > diff --git a/arch/arm/boot/compressed/efi-header.S > b/arch/arm/boot/compressed/efi-header.S > new file mode 100644 > index 000..6965e0f > --- /dev/null > +++ b/arch/arm/boot/compressed/efi-header.S > @@ -0,0 +1,111 @@ > +@ Copyright (C) 2013 Linaro Ltd; > +@ > +@ This file contains the PE/COFF header that is part of the > +@ EFI stub. > +@ > + > + .org0x3c > + @ > + @ The PE header can be anywhere in the file, but for > + @ simplicity we keep it together with the MSDOS header > + @ The offset to the PE/COFF header needs to be at offset > + @ 0x3C in the MSDOS header. > + @ The only 2 fields of the MSDOS header that are used are this > + @ PE/COFF offset, and the "MZ" bytes at offset 0x0. > + @ > + .long pe_header @ Offset to the PE header. > + > + .align 3 > +pe_header: > + .ascii "PE" > + .short 0 > + > +coff_header: > + .short 0x01c2 @ ARM or Thumb > + .short 2 @ nr_sections > + .long 0 @ TimeDateStamp > + .long 0 @ PointerToSymbolTable > + .long 1 @ NumberOfSymbols > + .short section_table - optional_header @ SizeOfOptionalHeader > + .short 0x306 @ Characteristics. > + @ IMAGE_FILE_32BIT_MACHINE | > + @ IMAGE_FILE_DEBUG_STRIPPED | > + @ IMAGE_FILE_EXECUTABLE_IMAGE | > + @ IMAGE_FILE_LINE_NUMS_STRIPPED > + > +optional_header: > + .short 0x10b @ PE32 format > + .byte 0x02@ MajorLinkerVersion > + .byte 0x14@ MinorLinkerVersion > + > + .long _edata - efi_stub_entry @ SizeOfCode > + > + .long 0 @ SizeOfInitializedData > + .long 0 @ SizeOfUninitializedData > + > + .long efi_stub_entry @ AddressOfEntryPoint > + .long efi_stub_entry @ BaseOfCode > + .long 0 @ data > + > +extra_header_fields: > + .long 0 @ ImageBase > + .long 0x20@ SectionAlignment > + .long 0x20@ FileAlignment > + .short 0 @ MajorOperatingSystemVersion > +
Re: [PATCH 14/16] Add strstr to compressed string.c for ARM.
On Fri, Aug 30, 2013 at 2:47 PM, Russell King - ARM Linux wrote: > On Fri, Aug 30, 2013 at 02:43:25PM +0100, Grant Likely wrote: >> On Fri, 9 Aug 2013 16:26:15 -0700, Roy Franz wrote: >> > The shared efi-stub-helper.c functions require a strstr >> > implementation. >> > Implementation copied from arch/x86/boot/string.c >> > >> > Signed-off-by: Roy Franz >> >> Okay, but at some point arch/arm/boot/compressed/string.c should be >> reworked into a common place. > > Only if the common place can be built with arch specific compile options, > like -fpic, without impacting the rest of the kernel. agreed. It would be a bit of a science project to see how feasible. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/16] Add strstr to compressed string.c for ARM.
On Fri, 9 Aug 2013 16:26:15 -0700, Roy Franz wrote:
> The shared efi-stub-helper.c functions require a strstr
> implementation.
> Implementation copied from arch/x86/boot/string.c
>
> Signed-off-by: Roy Franz
Okay, but at some point arch/arm/boot/compressed/string.c should be
reworked into a common place.
Reviewed-by: Grant Likely
> ---
> arch/arm/boot/compressed/string.c | 21 +
> 1 file changed, 21 insertions(+)
>
> diff --git a/arch/arm/boot/compressed/string.c
> b/arch/arm/boot/compressed/string.c
> index 36e53ef..5397792 100644
> --- a/arch/arm/boot/compressed/string.c
> +++ b/arch/arm/boot/compressed/string.c
> @@ -111,6 +111,27 @@ char *strchr(const char *s, int c)
> return (char *)s;
> }
>
> +/**
> + * strstr - Find the first substring in a %NUL terminated string
> + * @s1: The string to be searched
> + * @s2: The string to search for
> + */
> +char *strstr(const char *s1, const char *s2)
> +{
> + size_t l1, l2;
> +
> + l2 = strlen(s2);
> + if (!l2)
> + return (char *)s1;
> + l1 = strlen(s1);
> + while (l1 >= l2) {
> + l1--;
> + if (!memcmp(s1, s2, l2))
> + return (char *)s1;
> + s1++;
> + }
> + return NULL;
> +}
> #undef memset
>
> void *memset(void *s, int c, size_t count)
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/16] resolve warnings found on ARM compile
On Fri, 9 Aug 2013 16:26:14 -0700, Roy Franz wrote:
> 2 unused labels
> 1 "value computed is not used"
>
>
> Signed-off-by: Roy Franz
When posting a patch to fix compile errors or warnings, it is helpful
inlude the output from the compiler with the error/warning message.
g.
> ---
> drivers/firmware/efi/efi-stub-helper.c |4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi-stub-helper.c
> b/drivers/firmware/efi/efi-stub-helper.c
> index 4bb542f..3e82cb0 100644
> --- a/drivers/firmware/efi/efi-stub-helper.c
> +++ b/drivers/firmware/efi/efi-stub-helper.c
> @@ -166,7 +166,6 @@ again:
> *addr = max_addr;
> }
>
> -free_pool:
> efi_call_phys1(sys_table_arg->boottime->free_pool, map);
>
> fail:
> @@ -242,7 +241,6 @@ static efi_status_t efi_low_alloc(efi_system_table_t
> *sys_table_arg,
> if (i == map_size / desc_size)
> status = EFI_NOT_FOUND;
>
> -free_pool:
> efi_call_phys1(sys_table_arg->boottime->free_pool, map);
> fail:
> return status;
> @@ -357,7 +355,7 @@ static efi_status_t
> handle_cmdline_files(efi_system_table_t *sys_table_arg,
>
> if (*str == '/') {
> *p++ = '\\';
> - *str++;
> + str++;
> } else {
> *p++ = *str++;
> }
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/16] Fix types in EFI calls to match EFI function definitions.
On Fri, 9 Aug 2013 16:26:13 -0700, Roy Franz wrote:
> EFI calls can made directly on ARM, so the function pointers
> are directly invoked. This allows types to be checked at
> compile time, so here we ensure that the parameters match
> the function signature.
>
> Signed-off-by: Roy Franz
> ---
> drivers/firmware/efi/efi-stub-helper.c | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi-stub-helper.c
> b/drivers/firmware/efi/efi-stub-helper.c
> index b707a9f..4bb542f 100644
> --- a/drivers/firmware/efi/efi-stub-helper.c
> +++ b/drivers/firmware/efi/efi-stub-helper.c
> @@ -321,7 +321,7 @@ static efi_status_t
> handle_cmdline_files(efi_system_table_t *sys_table_arg,
> status = efi_call_phys3(sys_table_arg->boottime->allocate_pool,
> EFI_LOADER_DATA,
> nr_files * sizeof(*files),
> - &files);
> + (void **)&files);
> if (status != EFI_SUCCESS) {
> efi_printk(sys_table_arg, "Failed to alloc mem for file handle
> list\n");
> goto fail;
> @@ -372,7 +372,8 @@ static efi_status_t
> handle_cmdline_files(efi_system_table_t *sys_table_arg,
> boottime = sys_table_arg->boottime;
>
> status = efi_call_phys3(boottime->handle_protocol,
> - image->device_handle, &fs_proto, &io);
> + image->device_handle, &fs_proto,
> + (void **)&io);
> if (status != EFI_SUCCESS) {
> efi_printk(sys_table_arg, "Failed to handle
> fs_proto\n");
> goto free_files;
> @@ -406,7 +407,8 @@ static efi_status_t
> handle_cmdline_files(efi_system_table_t *sys_table_arg,
>
> grow:
> status = efi_call_phys3(sys_table_arg->boottime->allocate_pool,
> - EFI_LOADER_DATA, info_sz, &info);
> + EFI_LOADER_DATA, info_sz,
> + (void **)&info);
> if (status != EFI_SUCCESS) {
> efi_printk(sys_table_arg, "Failed to alloc mem for file
> info\n");
> goto close_handles;
> @@ -456,18 +458,19 @@ grow:
>
> addr = file_addr;
> for (j = 0; j < nr_files; j++) {
> - u64 size;
> + unsigned long size;
>
> size = files[j].size;
> while (size) {
> - u64 chunksize;
> + unsigned long chunksize;
> if (size > EFI_READ_CHUNK_SIZE)
> chunksize = EFI_READ_CHUNK_SIZE;
> else
> chunksize = size;
> status = efi_call_phys3(fh->read,
> files[j].handle,
> - &chunksize, addr);
> + &chunksize,
> + (void *)addr);
I think you need some description in the commit text about why the type
of chunksize is changing. It's not clear why you're making this change.
It is a bug fix?
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/16] Add proper definitions for some EFI function pointers.
On Fri, 9 Aug 2013 16:26:12 -0700, Roy Franz wrote:
> The x86/AMD64 EFI stubs must us a call wrapper to convert between
> the Linux and EFI ABIs, so void pointers are sufficient. For ARM,
> the ABIs are compatible, so we can directly invoke the function
> pointers. The functions that are used by the ARM stub are updated
> to match the EFI definitions.
>
> Signed-off-by: Roy Franz
Looks reasonable.
Reviewed-by: Grant Likely
> ---
> arch/x86/boot/compressed/eboot.h |2 --
> include/linux/efi.h | 45
> --
> 2 files changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/eboot.h
> b/arch/x86/boot/compressed/eboot.h
> index bafbd94..81b6b65 100644
> --- a/arch/x86/boot/compressed/eboot.h
> +++ b/arch/x86/boot/compressed/eboot.h
> @@ -11,8 +11,6 @@
>
> #define DESC_TYPE_CODE_DATA (1 << 0)
>
> -#define EFI_PAGE_SIZE(1UL << EFI_PAGE_SHIFT)
> -
> #define EFI_CONSOLE_OUT_DEVICE_GUID\
> EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4, 0x9a, 0x46, 0x0, 0x90, 0x27, \
> 0x3f, 0xc1, 0x4d)
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 51f5641..1a7ae34 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -39,6 +39,8 @@
> typedef unsigned long efi_status_t;
> typedef u8 efi_bool_t;
> typedef u16 efi_char16_t;/* UNICODE character */
> +typedef u64 efi_physical_addr_t;
> +typedef void *efi_handle_t;
>
>
> typedef struct {
> @@ -96,6 +98,7 @@ typedef struct {
> #define EFI_MEMORY_DESCRIPTOR_VERSION1
>
> #define EFI_PAGE_SHIFT 12
> +#define EFI_PAGE_SIZE(1UL << EFI_PAGE_SHIFT)
>
> typedef struct {
> u32 type;
> @@ -157,11 +160,13 @@ typedef struct {
> efi_table_hdr_t hdr;
> void *raise_tpl;
> void *restore_tpl;
> - void *allocate_pages;
> - void *free_pages;
> - void *get_memory_map;
> - void *allocate_pool;
> - void *free_pool;
> + efi_status_t (*allocate_pages)(int, int, unsigned long,
> +efi_physical_addr_t *);
> + efi_status_t (*free_pages)(efi_physical_addr_t, unsigned long);
> + efi_status_t (*get_memory_map)(unsigned long *, void *, unsigned long *,
> +unsigned long *, u32 *);
> + efi_status_t (*allocate_pool)(int, unsigned long, void **);
> + efi_status_t (*free_pool)(void *);
> void *create_event;
> void *set_timer;
> void *wait_for_event;
> @@ -171,7 +176,7 @@ typedef struct {
> void *install_protocol_interface;
> void *reinstall_protocol_interface;
> void *uninstall_protocol_interface;
> - void *handle_protocol;
> + efi_status_t (*handle_protocol)(efi_handle_t, efi_guid_t *, void **);
> void *__reserved;
> void *register_protocol_notify;
> void *locate_handle;
> @@ -181,7 +186,7 @@ typedef struct {
> void *start_image;
> void *exit;
> void *unload_image;
> - void *exit_boot_services;
> + efi_status_t (*exit_boot_services)(efi_handle_t, unsigned long);
> void *get_next_monotonic_count;
> void *stall;
> void *set_watchdog_timer;
> @@ -488,10 +493,6 @@ typedef struct {
> unsigned long unload;
> } efi_loaded_image_t;
>
> -typedef struct {
> - u64 revision;
> - void *open_volume;
> -} efi_file_io_interface_t;
>
> typedef struct {
> u64 size;
> @@ -504,20 +505,30 @@ typedef struct {
> efi_char16_t filename[1];
> } efi_file_info_t;
>
> -typedef struct {
> +typedef struct _efi_file_handle {
> u64 revision;
> - void *open;
> - void *close;
> + efi_status_t (*open)(struct _efi_file_handle *,
> + struct _efi_file_handle **,
> + efi_char16_t *, u64, u64);
> + efi_status_t (*close)(struct _efi_file_handle *);
> void *delete;
> - void *read;
> + efi_status_t (*read)(struct _efi_file_handle *, unsigned long *,
> + void *);
> void *write;
> void *get_position;
> void *set_position;
> - void *get_info;
> + efi_status_t (*get_info)(struct _efi_file_handle *, efi_guid_t *,
> + unsigned long *, void *);
> void *set_info;
> void *flush;
> } efi_file_handle_t;
>
> +typedef struct _efi_file_io_interface {
> + u64 revision;
> + int (*open_volume)(struct _efi_file_io_interface *,
> +efi_file_handle_t **);
> +} efi_file_io_interface_t;
>
Re: [PATCH 10/16] Move EFI_READ_CHUNK_SIZE define to shared location.
On Fri, 9 Aug 2013 16:26:11 -0700, Roy Franz wrote:
> This #define is only used the the shared code, so move
> it there.
>
> Signed-off-by: Roy Franz
Can this be squashed into patch 1?
g.
> ---
> arch/x86/boot/compressed/eboot.h |1 -
> drivers/firmware/efi/efi-stub-helper.c |2 +-
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/eboot.h
> b/arch/x86/boot/compressed/eboot.h
> index faa0bdf..bafbd94 100644
> --- a/arch/x86/boot/compressed/eboot.h
> +++ b/arch/x86/boot/compressed/eboot.h
> @@ -12,7 +12,6 @@
> #define DESC_TYPE_CODE_DATA (1 << 0)
>
> #define EFI_PAGE_SIZE(1UL << EFI_PAGE_SHIFT)
> -#define EFI_READ_CHUNK_SIZE (1024 * 1024)
>
> #define EFI_CONSOLE_OUT_DEVICE_GUID\
> EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4, 0x9a, 0x46, 0x0, 0x90, 0x27, \
> diff --git a/drivers/firmware/efi/efi-stub-helper.c
> b/drivers/firmware/efi/efi-stub-helper.c
> index 0f4d6e6..b707a9f 100644
> --- a/drivers/firmware/efi/efi-stub-helper.c
> +++ b/drivers/firmware/efi/efi-stub-helper.c
> @@ -9,7 +9,7 @@
> * under the terms of the GNU General Public License version 2.
> *
> */
> -
> +#define EFI_READ_CHUNK_SIZE (1024 * 1024)
>
> struct file_info {
> efi_file_handle_t *handle;
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/16] Generalize handle_ramdisks() and rename to handle_cmdline_files().
On Fri, 9 Aug 2013 16:26:09 -0700, Roy Franz wrote:
> The handle_cmdline_files now takes the option to handle as a string,
> and returns the loaded data through parameters, rather than taking
> an x86 specific setup_header structure. For ARM, this will be used
> to load a device tree blob in addition to initrd images.
>
> Signed-off-by: Roy Franz
Minor comment below, but otherwise looks good:
Reviewed-by: Grant Likely
> ---
> @@ -380,7 +388,7 @@ static efi_status_t handle_ramdisks(efi_system_table_t
> *sys_table_arg,
> status = efi_call_phys5(fh->open, fh, &h, filename_16,
> EFI_FILE_MODE_READ, (u64)0);
> if (status != EFI_SUCCESS) {
> - efi_printk(sys_table_arg, "Failed to open initrd file:
> ");
> + efi_printk(sys_table_arg, "Failed to open file file: ");
"file file:"? Search and replace artifact?
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/16] Renames in handle_cmdline_files() to complete generalization.
On Fri, 9 Aug 2013 16:26:10 -0700, Roy Franz wrote:
> Rename variables to be not initrd specific, as now the function
> loads arbitrary files.
>
> Signed-off-by: Roy Franz
Reviewed-by: Grant Likely
> ---
> drivers/firmware/efi/efi-stub-helper.c | 92
>
> 1 file changed, 46 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi-stub-helper.c
> b/drivers/firmware/efi/efi-stub-helper.c
> index b5ef766..0f4d6e6 100644
> --- a/drivers/firmware/efi/efi-stub-helper.c
> +++ b/drivers/firmware/efi/efi-stub-helper.c
> @@ -11,7 +11,7 @@
> */
>
>
> -struct initrd {
> +struct file_info {
> efi_file_handle_t *handle;
> u64 size;
> };
> @@ -262,10 +262,10 @@ static void efi_free(efi_system_table_t *sys_table_arg,
> unsigned long size,
>
>
> /*
> - * Check the cmdline for a LILO-style initrd= arguments.
> + * Check the cmdline for a LILO-style file= arguments.
> *
> - * We only support loading an initrd from the same filesystem as the
> - * kernel image.
> + * We only support loading a file from the same filesystem as
> + * the kernel image.
> */
> static efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
>efi_loaded_image_t *image,
> @@ -273,19 +273,19 @@ static efi_status_t
> handle_cmdline_files(efi_system_table_t *sys_table_arg,
>u64 max_addr,
>u64 *load_addr, u64 *load_size)
> {
> - struct initrd *initrds;
> - unsigned long initrd_addr;
> + struct file_info *files;
> + unsigned long file_addr;
> efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID;
> - u64 initrd_total;
> + u64 file_size_total;
> efi_file_io_interface_t *io;
> efi_file_handle_t *fh;
> efi_status_t status;
> - int nr_initrds;
> + int nr_files;
> char *str;
> int i, j, k;
>
> - initrd_addr = 0;
> - initrd_total = 0;
> + file_addr = 0;
> + file_size_total = 0;
>
> str = cmd_line;
>
> @@ -300,7 +300,7 @@ static efi_status_t
> handle_cmdline_files(efi_system_table_t *sys_table_arg,
> if (!str || !*str)
> return EFI_SUCCESS;
>
> - for (nr_initrds = 0; *str; nr_initrds++) {
> + for (nr_files = 0; *str; nr_files++) {
> str = strstr(str, option_string);
> if (!str)
> break;
> @@ -315,21 +315,21 @@ static efi_status_t
> handle_cmdline_files(efi_system_table_t *sys_table_arg,
> str++;
> }
>
> - if (!nr_initrds)
> + if (!nr_files)
> return EFI_SUCCESS;
>
> status = efi_call_phys3(sys_table_arg->boottime->allocate_pool,
> EFI_LOADER_DATA,
> - nr_initrds * sizeof(*initrds),
> - &initrds);
> + nr_files * sizeof(*files),
> + &files);
> if (status != EFI_SUCCESS) {
> - efi_printk(sys_table_arg, "Failed to alloc mem for file
> load\n");
> + efi_printk(sys_table_arg, "Failed to alloc mem for file handle
> list\n");
> goto fail;
> }
>
> str = cmd_line;
> - for (i = 0; i < nr_initrds; i++) {
> - struct initrd *initrd;
> + for (i = 0; i < nr_files; i++) {
> + struct file_info *file;
> efi_file_handle_t *h;
> efi_file_info_t *info;
> efi_char16_t filename_16[256];
> @@ -344,7 +344,7 @@ static efi_status_t
> handle_cmdline_files(efi_system_table_t *sys_table_arg,
>
> str += strlen(option_string);
>
> - initrd = &initrds[i];
> + file = &files[i];
> p = filename_16;
>
> /* Skip any leading slashes */
> @@ -375,13 +375,13 @@ static efi_status_t
> handle_cmdline_files(efi_system_table_t *sys_table_arg,
> image->device_handle, &fs_proto, &io);
> if (status != EFI_SUCCESS) {
> efi_printk(sys_table_arg, "Failed to handle
> fs_proto\n");
> - goto free_initrds;
> + goto free_files;
> }
>
> status = efi_call_phys2(io->open_volume, io, &fh);
> if (status != EFI_SUCCESS) {
> efi_printk
Re: [PATCH 06/16] Enforce minimum alignment of 1 page on allocations. The efi_high_alloc() and efi_low_alloc() functions use the EFI_ALLOCATE_ADDRESS option to the EFI function allocate_pages(), which
On Fri, 9 Aug 2013 16:26:07 -0700, Roy Franz wrote:
> The existing code could fail to allocate depending
> on allocation size, as although repeated allocation
> attempts were made, none were guaranteed to be page
> aligned.
Commit message. Otherwise looks reasonable.
Reviewed-by: Grant Likely
> Signed-off-by: Roy Franz
> ---
> drivers/firmware/efi/efi-stub-helper.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/firmware/efi/efi-stub-helper.c
> b/drivers/firmware/efi/efi-stub-helper.c
> index 1d0a079..647b3ba 100644
> --- a/drivers/firmware/efi/efi-stub-helper.c
> +++ b/drivers/firmware/efi/efi-stub-helper.c
> @@ -105,6 +105,13 @@ static efi_status_t efi_high_alloc(efi_system_table_t
> *sys_table_arg,
> if (status != EFI_SUCCESS)
> goto fail;
>
> + /* Enforce minimum alignment that EFI requires when requesting
> + * a specific address. We are doing page-based allocations,
> + * so we must be aligned to a page.
> + */
> + if (align < EFI_PAGE_SIZE)
> + align = EFI_PAGE_SIZE;
> +
> nr_pages = round_up(size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE;
> again:
> for (i = 0; i < map_size / desc_size; i++) {
> @@ -184,6 +191,13 @@ static efi_status_t efi_low_alloc(efi_system_table_t
> *sys_table_arg,
> if (status != EFI_SUCCESS)
> goto fail;
>
> + /* Enforce minimum alignment that EFI requires when requesting
> + * a specific address. We are doing page-based allocations,
> + * so we must be aligned to a page.
> + */
> + if (align < EFI_PAGE_SIZE)
> + align = EFI_PAGE_SIZE;
> +
> nr_pages = round_up(size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE;
> for (i = 0; i < map_size / desc_size; i++) {
> efi_memory_desc_t *desc;
> --
> 1.7.10.4
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/16] rename __get_map() to efi_get_memory_map(), add parameter to optionally return mmap key. The mmap key is required to exit EFI boot services, and allows efi_get_memory_map() to be use
On Fri, 9 Aug 2013 16:26:06 -0700, Roy Franz wrote:
> Signed-off-by: Roy Franz
Same issue with the commit message here as for patch 4. Please include a
comment stating what is intended to use this new feature.
> ---
> drivers/firmware/efi/efi-stub-helper.c | 17 +++--
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi-stub-helper.c
> b/drivers/firmware/efi/efi-stub-helper.c
> index 40cd16e..1d0a079 100644
> --- a/drivers/firmware/efi/efi-stub-helper.c
> +++ b/drivers/firmware/efi/efi-stub-helper.c
> @@ -46,10 +46,11 @@ static void efi_printk(efi_system_table_t *sys_table_arg,
> char *str)
> }
>
>
> -static efi_status_t __get_map(efi_system_table_t *sys_table_arg,
> - efi_memory_desc_t **map,
> - unsigned long *map_size,
> - unsigned long *desc_size)
> +static efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg,
> +efi_memory_desc_t **map,
> +unsigned long *map_size,
> +unsigned long *desc_size,
> +unsigned long *key_ptr)
> {
> efi_memory_desc_t *m = NULL;
> efi_status_t status;
> @@ -77,6 +78,8 @@ again:
>
> if (status != EFI_SUCCESS)
> efi_call_phys1(sys_table_arg->boottime->free_pool, m);
> + if (key_ptr && status == EFI_SUCCESS)
> + *key_ptr = key;
>
> fail:
> *map = m;
> @@ -97,7 +100,8 @@ static efi_status_t efi_high_alloc(efi_system_table_t
> *sys_table_arg,
> u64 max_addr = 0;
> int i;
>
> - status = __get_map(sys_table_arg, &map, &map_size, &desc_size);
> + status = efi_get_memory_map(sys_table_arg, &map, &map_size, &desc_size,
> + NULL);
> if (status != EFI_SUCCESS)
> goto fail;
>
> @@ -175,7 +179,8 @@ static efi_status_t efi_low_alloc(efi_system_table_t
> *sys_table_arg,
> unsigned long nr_pages;
> int i;
>
> - status = __get_map(sys_table_arg, &map, &map_size, &desc_size);
> + status = efi_get_memory_map(sys_table_arg, &map, &map_size, &desc_size,
> + NULL);
> if (status != EFI_SUCCESS)
> goto fail;
>
> --
> 1.7.10.4
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/16] Add minimum address parameter to efi_low_alloc()
On Fri, 9 Aug 2013 16:26:05 -0700, Roy Franz wrote:
> This allows allocations to be made low in memory while
> avoiding allocations at the base of memory.
Your commit message should include /why/ the change is needed. From the
above I understand what the patch does, but I don't understand why it is
necessary.
The patch looks fine to me, but it would be worth investigating merging
alloc_low and alloc_high. It looks like they both do pretty close to the
same calculations. A single core function could do both, could have both
minimum and maximum constraints, and could have a flag to determine if
low or high addresses should be preferred.
g.
Reviewed-by: Grant Likely
>
> Signed-off-by: Roy Franz
> ---
> arch/x86/boot/compressed/eboot.c | 11 ++-
> drivers/firmware/efi/efi-stub-helper.c |7 +--
> 2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/eboot.c
> b/arch/x86/boot/compressed/eboot.c
> index 2a4430a..f44ef2f 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -458,7 +458,7 @@ struct boot_params *make_boot_params(void *handle,
> efi_system_table_t *_table)
> }
>
> status = efi_low_alloc(sys_table, 0x4000, 1,
> -(unsigned long *)&boot_params);
> +(unsigned long *)&boot_params, 0);
> if (status != EFI_SUCCESS) {
> efi_printk(sys_table, "Failed to alloc lowmem for boot
> params\n");
> return NULL;
> @@ -505,7 +505,7 @@ struct boot_params *make_boot_params(void *handle,
> efi_system_table_t *_table)
> options_size++; /* NUL termination */
>
> status = efi_low_alloc(sys_table, options_size, 1,
> -&cmdline);
> +&cmdline, 0);
> if (status != EFI_SUCCESS) {
> efi_printk(sys_table, "Failed to alloc mem for
> cmdline\n");
> goto fail;
> @@ -563,7 +563,8 @@ static efi_status_t exit_boot(struct boot_params
> *boot_params,
> again:
> size += sizeof(*mem_map) * 2;
> _size = size;
> - status = efi_low_alloc(sys_table, size, 1, (unsigned long *)&mem_map);
> + status = efi_low_alloc(sys_table, size, 1,
> +(unsigned long *)&mem_map, 0);
> if (status != EFI_SUCCESS)
> return status;
>
> @@ -697,7 +698,7 @@ static efi_status_t relocate_kernel(struct setup_header
> *hdr)
> nr_pages, &start);
> if (status != EFI_SUCCESS) {
> status = efi_low_alloc(sys_table, hdr->init_size,
> -hdr->kernel_alignment, &start);
> +hdr->kernel_alignment, &start, 0);
> if (status != EFI_SUCCESS)
> efi_printk(sys_table, "Failed to alloc mem for
> kernel\n");
> }
> @@ -745,7 +746,7 @@ struct boot_params *efi_main(void *handle,
> efi_system_table_t *_table,
>
> gdt->size = 0x800;
> status = efi_low_alloc(sys_table, gdt->size, 8,
> -(unsigned long *)&gdt->address);
> +(unsigned long *)&gdt->address, 0);
> if (status != EFI_SUCCESS) {
> efi_printk(sys_table, "Failed to alloc mem for gdt\n");
> goto fail;
> diff --git a/drivers/firmware/efi/efi-stub-helper.c
> b/drivers/firmware/efi/efi-stub-helper.c
> index 0218d535..40cd16e 100644
> --- a/drivers/firmware/efi/efi-stub-helper.c
> +++ b/drivers/firmware/efi/efi-stub-helper.c
> @@ -163,11 +163,11 @@ fail:
> }
>
> /*
> - * Allocate at the lowest possible address.
> + * Allocate at the lowest possible address, that is not below min.
> */
> static efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
> unsigned long size, unsigned long align,
> - unsigned long *addr)
> + unsigned long *addr, unsigned long min)
> {
> unsigned long map_size, desc_size;
> efi_memory_desc_t *map;
> @@ -204,6 +204,9 @@ static efi_status_t efi_low_alloc(efi_system_table_t
> *sys_table_arg,
> if (start == 0x0)
> start += 8;
>
> + if (start < min)
> + start = min;
> +
> start = round_up(start, align);
> if ((start + size) > end)
> continue;
> --
> 1.7.10.4
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/16] Rename memory allocation/free functions
On Fri, 9 Aug 2013 16:26:04 -0700, Roy Franz wrote:
> Rename them to be more similar, as low_free() could be used to free
> memory allocated by both high_alloc() and low_alloc().
> high_alloc() -> efi_high_alloc()
> low_alloc() -> efi_low_alloc()
> low_free() -> efi_free()
>
> Signed-off-by: Roy Franz
> ---
Looks reasonable.
Reviewed-by: Grant Likely
> arch/x86/boot/compressed/eboot.c | 19 ++-
> drivers/firmware/efi/efi-stub-helper.c | 14 +++---
> 2 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/eboot.c
> b/arch/x86/boot/compressed/eboot.c
> index 65b6a34..2a4430a 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -457,7 +457,8 @@ struct boot_params *make_boot_params(void *handle,
> efi_system_table_t *_table)
> return NULL;
> }
>
> - status = low_alloc(sys_table, 0x4000, 1, (unsigned long *)&boot_params);
> + status = efi_low_alloc(sys_table, 0x4000, 1,
> +(unsigned long *)&boot_params);
> if (status != EFI_SUCCESS) {
> efi_printk(sys_table, "Failed to alloc lowmem for boot
> params\n");
> return NULL;
> @@ -503,7 +504,7 @@ struct boot_params *make_boot_params(void *handle,
> efi_system_table_t *_table)
>
> options_size++; /* NUL termination */
>
> - status = low_alloc(sys_table, options_size, 1,
> + status = efi_low_alloc(sys_table, options_size, 1,
> &cmdline);
> if (status != EFI_SUCCESS) {
> efi_printk(sys_table, "Failed to alloc mem for
> cmdline\n");
> @@ -537,9 +538,9 @@ struct boot_params *make_boot_params(void *handle,
> efi_system_table_t *_table)
> return boot_params;
> fail2:
> if (options_size)
> - low_free(sys_table, options_size, hdr->cmd_line_ptr);
> + efi_free(sys_table, options_size, hdr->cmd_line_ptr);
> fail:
> - low_free(sys_table, 0x4000, (unsigned long)boot_params);
> + efi_free(sys_table, 0x4000, (unsigned long)boot_params);
> return NULL;
> }
>
> @@ -562,7 +563,7 @@ static efi_status_t exit_boot(struct boot_params
> *boot_params,
> again:
> size += sizeof(*mem_map) * 2;
> _size = size;
> - status = low_alloc(sys_table, size, 1, (unsigned long *)&mem_map);
> + status = efi_low_alloc(sys_table, size, 1, (unsigned long *)&mem_map);
> if (status != EFI_SUCCESS)
> return status;
>
> @@ -570,7 +571,7 @@ get_map:
> status = efi_call_phys5(sys_table->boottime->get_memory_map, &size,
> mem_map, &key, &desc_size, &desc_version);
> if (status == EFI_BUFFER_TOO_SMALL) {
> - low_free(sys_table, _size, (unsigned long)mem_map);
> + efi_free(sys_table, _size, (unsigned long)mem_map);
> goto again;
> }
>
> @@ -672,7 +673,7 @@ get_map:
> return EFI_SUCCESS;
>
> free_mem_map:
> - low_free(sys_table, _size, (unsigned long)mem_map);
> + efi_free(sys_table, _size, (unsigned long)mem_map);
> return status;
> }
>
> @@ -695,7 +696,7 @@ static efi_status_t relocate_kernel(struct setup_header
> *hdr)
> EFI_ALLOCATE_ADDRESS, EFI_LOADER_DATA,
> nr_pages, &start);
> if (status != EFI_SUCCESS) {
> - status = low_alloc(sys_table, hdr->init_size,
> + status = efi_low_alloc(sys_table, hdr->init_size,
> hdr->kernel_alignment, &start);
> if (status != EFI_SUCCESS)
> efi_printk(sys_table, "Failed to alloc mem for
> kernel\n");
> @@ -743,7 +744,7 @@ struct boot_params *efi_main(void *handle,
> efi_system_table_t *_table,
> }
>
> gdt->size = 0x800;
> - status = low_alloc(sys_table, gdt->size, 8,
> + status = efi_low_alloc(sys_table, gdt->size, 8,
> (unsigned long *)&gdt->address);
> if (status != EFI_SUCCESS) {
> efi_printk(sys_table, "Failed to alloc mem for gdt\n");
> diff --git a/drivers/firmware/efi/efi-stub-helper.c
> b/drivers/firmware/efi/efi-stub-helper.c
> index bd6c1a2..0218d535 100644
> --- a/drivers/firmware/efi/efi-stub-helper.c
> +++ b/drivers/firmware/efi/efi-stub-helper.c
> @@ -86,7 +86,7 @@ fail:
> /*
> * Allocate at t
Re: [PATCH 02/16] Add system pointer argument to shared EFI stub related functions so they no longer use global system table pointer as they did when part of eboot.c. This code is now shared, so using
On Fri, 9 Aug 2013 16:26:03 -0700, Roy Franz wrote:
> Signed-off-by: Roy Franz
The entire commit message is contained in the subject line! The first
line of a commit message needs to be a short summary, followed by a
blank line, followed by the full description. Commit text body should be
wrapped at column 72 so it looks nice in 'git log'
>From your commit message:
> Subject: Re: [PATCH 02/16] Add system pointer argument to shared EFI
> stub related functions so they no longer use global system table
> pointer as they did when part of eboot.c. This code is now shared, so
> using a global variable as part of the interface is not that nice.
> Also, by avoiding any global variables in the ARM EFI stub, this
> allows the code to be position independent without requiring GOT
> fixups.
The argument about avoiding GOT fixups is stronger than the global
variables comment. I don't see a problem at all with using a global
context pointer since this code will only ever run within the context of
the EFI environment where there must always be a system table pointer.
It isn't as if there will ever be multiple system tables.
How much of a concern are GOT fixups at this stage? I thought GOT fixups
were a solved problem for the EFI stub.
g.
> ---
> arch/x86/boot/compressed/eboot.c | 38 +++--
> drivers/firmware/efi/efi-stub-helper.c | 96
> +---
> 2 files changed, 72 insertions(+), 62 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/eboot.c
> b/arch/x86/boot/compressed/eboot.c
> index ab0eefc..65b6a34 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -453,13 +453,13 @@ struct boot_params *make_boot_params(void *handle,
> efi_system_table_t *_table)
> status = efi_call_phys3(sys_table->boottime->handle_protocol,
> handle, &proto, (void *)&image);
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to get handle for LOADED_IMAGE_PROTOCOL\n");
> + efi_printk(sys_table, "Failed to get handle for
> LOADED_IMAGE_PROTOCOL\n");
> return NULL;
> }
>
> - status = low_alloc(0x4000, 1, (unsigned long *)&boot_params);
> + status = low_alloc(sys_table, 0x4000, 1, (unsigned long *)&boot_params);
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to alloc lowmem for boot params\n");
> + efi_printk(sys_table, "Failed to alloc lowmem for boot
> params\n");
> return NULL;
> }
>
> @@ -503,9 +503,10 @@ struct boot_params *make_boot_params(void *handle,
> efi_system_table_t *_table)
>
> options_size++; /* NUL termination */
>
> - status = low_alloc(options_size, 1, &cmdline);
> + status = low_alloc(sys_table, options_size, 1,
> +&cmdline);
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to alloc mem for cmdline\n");
> + efi_printk(sys_table, "Failed to alloc mem for
> cmdline\n");
> goto fail;
> }
>
> @@ -529,16 +530,16 @@ struct boot_params *make_boot_params(void *handle,
> efi_system_table_t *_table)
>
> memset(sdt, 0, sizeof(*sdt));
>
> - status = handle_ramdisks(image, hdr);
> + status = handle_ramdisks(sys_table, image, hdr);
> if (status != EFI_SUCCESS)
> goto fail2;
>
> return boot_params;
> fail2:
> if (options_size)
> - low_free(options_size, hdr->cmd_line_ptr);
> + low_free(sys_table, options_size, hdr->cmd_line_ptr);
> fail:
> - low_free(0x4000, (unsigned long)boot_params);
> + low_free(sys_table, 0x4000, (unsigned long)boot_params);
> return NULL;
> }
>
> @@ -561,7 +562,7 @@ static efi_status_t exit_boot(struct boot_params
> *boot_params,
> again:
> size += sizeof(*mem_map) * 2;
> _size = size;
> - status = low_alloc(size, 1, (unsigned long *)&mem_map);
> + status = low_alloc(sys_table, size, 1, (unsigned long *)&mem_map);
> if (status != EFI_SUCCESS)
> return status;
>
> @@ -569,7 +570,7 @@ get_map:
> status = efi_call_phys5(sys_table->boottime->get_memory_map, &size,
> mem_map, &key, &desc_size, &desc_version);
> if (status == EFI_BUFFER_TOO_SMALL) {
> - low_free(_size, (unsigned long)mem_map);
> + low_free(sys_table, _size, (unsigned long)mem_map);
> goto again;
> }
>
> @@ -671,7 +672,7 @@ get_map:
> return EFI_SUCCESS;
>
> free_mem_map:
> - low_free(_size, (unsigned long)mem_map);
> + low_free(sys_table, _size, (unsigned long)mem_map);
> return status;
> }
>
> @@ -694,10 +695,10 @@ static efi_status_t relocate_kernel(struct setup_header
> *hdr)
> E
Re: [PATCH 01/16] Move common EFI stub code from x86 arch code to common location
On Tue, 13 Aug 2013 09:56:20 -0400, Mark Salter wrote: > On Fri, 2013-08-09 at 16:26 -0700, Roy Franz wrote: > > No code changes made, just moving functions from x86 arch directory > > to common location. > > Code is shared using #include, similar to how decompression code > > is shared among architectures. > > > > Signed-off-by: Roy Franz > > --- > > Tested on arm64. > > Acked-by: Mark Salter Looks okay to me. I'm not a big fan of #including .c files, but I'm okay with it as-is for the time being. It can be revisited later to investigate a clean way to compile these functions as a separate .o file. Reviewed-by: Grant Likely g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/16] Add config EFI_STUB for ARM to Kconfig
On Tue, 13 Aug 2013 10:37:13 -0700, Roy Franz wrote: > On Tue, Aug 13, 2013 at 6:18 AM, Dave Martin wrote: > > On Fri, Aug 09, 2013 at 04:26:17PM -0700, Roy Franz wrote: > >> Signed-off-by: Roy Franz > >> --- > >> arch/arm/Kconfig | 11 +++ > >> 1 file changed, 11 insertions(+) > >> > >> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > >> index 43594d5..8607d03 100644 > >> --- a/arch/arm/Kconfig > >> +++ b/arch/arm/Kconfig > >> @@ -1805,6 +1805,17 @@ config UACCESS_WITH_MEMCPY > >> However, if the CPU data cache is using a write-allocate mode, > >> this option is unlikely to provide any performance gain. > >> > >> +config EFI_STUB > >> + bool "EFI stub support" > >> + depends on EFI > > > > && !CPU_BIG_ENDIAN, in case you didn't get around to that. > > > > Sooner or later, someone may try to fix that, so there should be a > > comment somewhere explaining what is broken for BE. > > > > Either in efi-stub.c or in the commit message accompanying this patch, I > > guess. > > > EFI will be !CPU_BIG_ENDIAN, so we will get the dependency > transitively through EFI. > That said, I think it may be better to have both be explicitly > !CPU_BIG_ENDIAN, as even > though the underlying reasons are the same (EFI services calls being > little endian), both > the stub and runtime services would need to be fixed to enable the > stub to run in BE mode. > I'll make this explicit in the stub Kconfig, and add a comment by the > PE/COFF magic number > regarding the LE requirement. Actually, not true! Steve McIntyre is currently doing some of the BE porting work on TC2 running UEFI. Eventually he will need this. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services
On Thu, Jun 27, 2013 at 7:04 PM, Stephen Warren wrote: > On 06/26/2013 01:31 PM, Leif Lindholm wrote: >> On Wed, Jun 26, 2013 at 12:32:30PM -0600, Stephen Warren wrote: > What about ARMv8? Is the intention to have a separate definition for the > UEFI bindings on ARMv8, so that compatibility isn't an issue? What if a > future version of UEFI allows LPAE usage? It is unlikely that will happen on v7 since newer versions of UEFI are expected to remain backwards compatible with the current spec. >>> >>> The expectation of backwards-compatibility sounds nice, but it seems a >>> little dangerous to outright rely on it. >>> >>> Even if not a regular compatible property, can we define a property that >>> indicates the UEFI revision or revision of this DT binding, so that if >>> we ever have to change it, there is some way of explicitly indicating >>> which exact schema the DT corresponds to, rather than having to >>> reverse-engineer it from the set of properties that "just happen" to be >>> present in DT? >>> >>> This is rather like the firmware node discussion that happened recently, >>> where we were expecting to represent a firmware (secure mode) interface >>> by a DT node, which would have a compatible value, which in turn would >>> convey information about which "OS" the secure firmware was running, and >>> well as any potential SoC-/OEM-/board-specific interface provided by it. >>> >>> And who knows, what if UEFI gets replaced someday; presumably we'd want >>> some way of explicitly stating "running under UEFI" vs. "running under >>> something else"? >> >> To me, these concerns are all covered by the existence of the >> efi-system-table node, and the version number that you can extract >> from the table (mandatory in any UEFI implementation) located at that >> address. There is no reverse-engineering involved. > > So, what you're saying is that the existence (or lack thereof) of the > efi-system-table property is the indicator whether EFI is present? I > guess if we assume that EFI will always evolve at least compatibly > enough that the system table will always exist and be formatted > identically at least to the extent of allowing the EFI version number to > be parsed then that's workable. If that guarantee is broken, then we can > always define a different property that points at the new format of the > table. Yes, that is what it means, and there is *immense* pressure from the OEM market to not break that contract in a non-backwards-compatible way. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services
On Thu, Jun 27, 2013 at 4:09 PM, James Bottomley wrote: > On Thu, 2013-06-27 at 15:37 +0100, Matthew Garrett wrote: >> On Wed, Jun 26, 2013 at 11:33:41PM -0700, James Bottomley wrote: >> > On Thu, 2013-06-27 at 07:23 +0100, Grant Likely wrote: >> > > What is the problem trying to be avoided by not using the virtual map? >> > > Is it passing the virtual mapping data from one kernel to the next >> > > when kexecing? Or something else? >> > >> > Where to begin ... SetVirtualAddressMap() is one massive hack job ... >> > just look at the tiano core implementation. Basically it has a fixed >> > idea of where all the pointers are and it tries to convert them all to >> > the new address space. The problem we see in x86 is that this >> > conversion process isn't exhaustive due to implementation cockups, so >> > the post virtual address map image occasionally tries to access >> > unconverted pointers via the old physical address and oopses the kernel. >> >> And yet it's the only mode in which the firmrware is actually tested >> against an OS, so we don't have any real choice in the matter. > > Agree for x86 ... we just have to cope with the implementations we see > in the field. However, ARM has much more scope to have the UEFI > implementation developed collaboratively with Linux as the reference > platform. If we can convince the ARM implementors that > SetVirtualAddressMap is an accident waiting to happen, they might be > more flexible. We may not have any success convincing them of that; but given the larger scope for Linux on ARM UEFI implementations, it will actually get tested. If Linux chooses to use a 1:1 mapping, then the hardware vendors will make sure a 1:1 mapping will actually work. I must say that I'm a whole lot more comfortable with this approach. I've never been comfortable with calling out to UEFI functions while leaving the entirety of kernel space accessable. Sure UEFI can still do nasty things if it really wants to and important devices get mapped, but at least it will protect against accidental accesses. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services
On Thu, Jun 27, 2013 at 7:33 AM, James Bottomley wrote: > On Thu, 2013-06-27 at 07:23 +0100, Grant Likely wrote: >> On Thu, Jun 27, 2013 at 2:32 AM, Matthew Garrett wrote: >> > On Wed, Jun 26, 2013 at 07:38:19AM -0700, James Bottomley wrote: >> >> The fixed virtual address scheme currently being looked at for x86_64 to >> >> make SetVirtualAddressMap() kexec invariant doesn't work on 32 bit >> >> because the address space isn't big enough. For ARM, given that we've >> >> much more opportunity to work with the vendors, can we just avoid >> >> transitioning to a virtual address map and always just install a >> >> physical mapping before doing efi calls? >> > >> > We can probably get away with that now, but it does risk us ending up >> > with some firmware that expects to run in physical mode (boards designed >> > for Linux) and some firmware that expects to run in virtual mode (boards >> > designed for Windows). The degree of lockdown in the Windows ecosystem >> > at present means it's not a real problem at the moment, but if that ever >> > changes we're going to risk incompatibility. >> >> What is the problem trying to be avoided by not using the virtual map? >> Is it passing the virtual mapping data from one kernel to the next >> when kexecing? Or something else? > > Where to begin ... SetVirtualAddressMap() is one massive hack job ... > just look at the tiano core implementation. Basically it has a fixed > idea of where all the pointers are and it tries to convert them all to > the new address space. The problem we see in x86 is that this > conversion process isn't exhaustive due to implementation cockups, so > the post virtual address map image occasionally tries to access > unconverted pointers via the old physical address and oopses the kernel. Would it be possible to run the UEFI hooks in some form of pseudo userspace thread that protects against dereferencing addresses that are no longer UEFI addresses? > The problem for kexec is that SetVirtualAddressMap isn't idempotent. In > fact by API fiat it can only ever be called once for the entire lifetime > of the UEFI bios, which could be many kernels in a kexec situation. So, > somehow the subsequent kernels have to know not to call it, plus, > obviously, the virtual address map of the previous kernel has to work in > the next because it can't set up a new one. For this problem at least I think we've got a solution on ARM because the virtual map can be passed across the kexec boundary via the device tree. It will still (probably) need to be located in the ioremap region and the size of the map will push down the maximum address for ioremapping. The value of VMALLOC_END on arm 32bit is 0xff00 and that is a pretty stable number. As long as both the new and old kernels have the same VMALLOC_END (very likely) then it should be okay to pass the map over. Let me know if I'm missing something important. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services
On Thu, Jun 27, 2013 at 2:32 AM, Matthew Garrett wrote: > On Wed, Jun 26, 2013 at 07:38:19AM -0700, James Bottomley wrote: >> The fixed virtual address scheme currently being looked at for x86_64 to >> make SetVirtualAddressMap() kexec invariant doesn't work on 32 bit >> because the address space isn't big enough. For ARM, given that we've >> much more opportunity to work with the vendors, can we just avoid >> transitioning to a virtual address map and always just install a >> physical mapping before doing efi calls? > > We can probably get away with that now, but it does risk us ending up > with some firmware that expects to run in physical mode (boards designed > for Linux) and some firmware that expects to run in virtual mode (boards > designed for Windows). The degree of lockdown in the Windows ecosystem > at present means it's not a real problem at the moment, but if that ever > changes we're going to risk incompatibility. What is the problem trying to be avoided by not using the virtual map? Is it passing the virtual mapping data from one kernel to the next when kexecing? Or something else? g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] arm: Add [U]EFI runtime services support
On Wed, Jun 26, 2013 at 3:15 PM, Borislav Petkov wrote: > On Wed, Jun 26, 2013 at 02:54:17PM +0100, Matt Fleming wrote: >> On Wed, 26 Jun, at 02:46:09PM, Grant Likely wrote: >> > Eventually we'll need to look at how this interacts with kexec. A >> > kexec'd kernel will need to use the mapping already chosen by a >> > previous kernel, but that's an issue for another patch series. >> >> FYI, this is exactly what Borislav has been tackling on x86 recently. It >> would be nice if we could find one scheme that suits everyone. > > Is this arm 32 or 64-bit? Because we haven't talked about 32-bit on x86 > either. From skimming over the code, I'm not sure the same top-down > allocation and 1:1 mapping would work there. But I haven't looked hard > yet so I dunno. This is arm 32. We'll be looking at arm 64 next. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services
On Wed, Jun 26, 2013 at 3:04 PM, Leif Lindholm wrote: > On Wed, Jun 26, 2013 at 02:13:39PM +0100, Grant Likely wrote: >> > +- 'efi-runtime-mmap': >> > + Physical address of an EFI memory map, containing at least >> > + the regions to be preserved. (required) >> > +- 'efi-runtime-mmap-size': >> > + Size in bytes of the provided memory map. (required) >> >> I would collapse the above two properties into a single property that >> actually contains the memory map instead of pointing to it. You will >> also need to specify the exact format of the data in this property. > > Ok, that makes sense. > > Hmm. The data is an array of struct EFI_MEMORY_DESCRIPTOR entries, > known in Linux as efi_memory_desc_t. Is that a good enough description? Yes, it is perfectly valid to point at another spec and state "it is in that format". You'll also want to be specific that the data is using the UEFI byte ordering, and not the ordering normally used by FDT. One could argue that it should be 'translated' into a native DT data format, but I think it is better to view it as a BLOB that DT doesn't have anything to say about. >> > +- 'efi-mmap-desc-size': >> > + Size of each descriptor in the memory map. (override default) >> > +- 'efi-mmap-desc-ver': >> > + Memory descriptor format version. (override default) >> >> I don't understand how these properties will actually work. What >> changes in the parsing if these properties are set? > > I guess the intended use is that these options would permit you to > append new fields to the struct and have old code correctly parse the > array anyway. Let's leave them out as part of the binding until it is actually needed. g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] arm: Add [U]EFI runtime services support
On Tue, Jun 25, 2013 at 7:20 PM, Matthew Garrett wrote: > On Tue, Jun 25, 2013 at 07:11:02PM +0100, Leif Lindholm 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 also locates any presented SMBIOS configuration table and stores it >> for potential later use by DMI. > > This appears to duplicate code that's already duplicated between x86 and > ia64. We made a mistake there originally - let's not do it again. Having > this code in three places is inevitably going to lead to skew and missed > bugfixes. +1 > > -- > Matthew Garrett | mj...@srcf.ucam.org > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] arm: Add [U]EFI runtime services support
On Tue, Jun 25, 2013 at 7:11 PM, Leif Lindholm 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 also locates any presented SMBIOS configuration table and stores it > for potential later use by DMI. > > Signed-off-by: Leif Lindholm > --- > arch/arm/Kconfig | 15 ++ > arch/arm/include/asm/efi.h | 22 +++ > arch/arm/kernel/Makefile |2 + > arch/arm/kernel/efi.c | 456 > > arch/arm/kernel/efi_phys.S | 59 ++ > arch/arm/kernel/setup.c|5 + > 6 files changed, 559 insertions(+) > create mode 100644 arch/arm/include/asm/efi.h > create mode 100644 arch/arm/kernel/efi.c > create mode 100644 arch/arm/kernel/efi_phys.S > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index bf8e55d..022d2eb 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -1763,6 +1763,19 @@ config EARLY_IOREMAP > Provides a mechanism for kernel initialisation code to temporarily > map, in a highmem-agnostic way, memory pages in before > paging_init(). > > +config EFI > +bool "UEFI runtime service support" > + depends on OF > + select UCS2_STRING > + select EARLY_IOREMAP > + ---help--- > + This enables the kernel to use UEFI runtime services that are > + available (such as the UEFI variable services). > + > + This option is only useful on systems that have UEFI firmware. > + However, even with this option, the resultant kernel should Be confident! s/should/will/ :-) > + continue to boot on existing non-UEFI platforms. s/existing// > + > config SECCOMP > bool > prompt "Enable seccomp to safely compute untrusted bytecode" > @@ -2217,6 +2230,8 @@ source "net/Kconfig" > > source "drivers/Kconfig" > > +source "drivers/firmware/Kconfig" > + > source "fs/Kconfig" > > source "arch/arm/Kconfig.debug" > diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h > new file mode 100644 > index 000..aead94c > --- /dev/null > +++ b/arch/arm/include/asm/efi.h > @@ -0,0 +1,22 @@ > +#ifndef _ASM_ARM_EFI_H > +#define _ASM_ARM_EFI_H > + > +#include > + > +extern int efi_memblock_arm_reserve_range(void); > + > +typedef efi_status_t efi_phys_call_t(u32 memory_map_size, > +u32 descriptor_size, > +u32 descriptor_version, > +efi_memory_desc_t *dsc, > +efi_set_virtual_address_map_t *f); > + > +extern efi_status_t efi_phys_call(u32, u32, u32, efi_memory_desc_t *, > + efi_set_virtual_address_map_t *); > + > +#define efi_remap(cookie, size) __arm_ioremap((cookie), (size), MT_MEMORY) > +#define efi_ioremap(cookie, size) __arm_ioremap((cookie), (size), MT_DEVICE) > +#define efi_unmap(cookie) __arm_iounmap((cookie)) > +#define efi_iounmap(cookie) __arm_iounmap((cookie)) > + > +#endif /* _ASM_ARM_EFI_H */ > diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile > index 5f3338e..12b9c30 100644 > --- a/arch/arm/kernel/Makefile > +++ b/arch/arm/kernel/Makefile > @@ -84,4 +84,6 @@ obj-$(CONFIG_EARLY_PRINTK)+= early_printk.o > obj-$(CONFIG_ARM_VIRT_EXT) += hyp-stub.o > obj-$(CONFIG_ARM_PSCI) += psci.o > > +obj-$(CONFIG_EFI) += efi.o efi_phys.o > + > extra-y := $(head-y) vmlinux.lds > diff --git a/arch/arm/kernel/efi.c b/arch/arm/kernel/efi.c > new file mode 100644 > index 000..43ecf0b > --- /dev/null > +++ b/arch/arm/kernel/efi.c > @@ -0,0 +1,456 @@ > +/* > + * Extensible Firmware Interface > + * > + * Based on Extensible Firmware Interface Specification version 2.3.1 > + * > + * Copyright (C) 2013 Linaro Ltd. Was any of the above code extracted from the x86/ia64 efi code base? If so then make sure the copyright header and license text gets retained. > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > + > +struct efi efi; > +EXPORT_SYMBOL(efi); > +struct efi_memory_map memmap; The above symbols should be pulled out of x86 and ia64 and made part of drivers/efi/efi.c. Although in my quick look I don't see memmap defined for ia64. You'll need to make sure that it actually exists before moving it. That will also affect your earlier patch which moves the memmap lookup function. I suspect that function won't build on ia64. > + > +static efi_runtime_services_t *runtime; > + > +static phys_addr_t efi_system_table; > +static phys_addr_t efi_boot_mmap; > +static u32 efi_boot_mmap_size; > +static u32 efi_mmap_desc_size; > +static u32 efi_mmap_desc_ver; > + > +/* Default memory map descriptor information */ > +#define DESC_SIZE 48 > +#define DESC_VER 1 > + > +/* If you're planning to wire up
Re: [PATCH 2/4] x86: efi: break efi_lookup_mapped_addr out to generic code
On Tue, Jun 25, 2013 at 7:11 PM, Leif Lindholm wrote: > efi_lookup_mapped_addr is a handy helper function for translating > a physical address to the corresponding virtual one by scanning > through memmap.map. > > This patch breaks it out into a new file for use elsewhere. > > Signed-off-by: Leif Lindholm > --- > arch/x86/platform/efi/efi.c | 28 > drivers/firmware/efi/Makefile |2 +- > drivers/firmware/efi/efi-helper.c | 33 + > 3 files changed, 34 insertions(+), 29 deletions(-) > create mode 100644 drivers/firmware/efi/efi-helper.c I /think/ you should be able to put this directly into drivers/firmware/efi/efi.c > diff --git a/drivers/firmware/efi/efi-helper.c > b/drivers/firmware/efi/efi-helper.c > new file mode 100644 > index 000..c5c2c72 > --- /dev/null > +++ b/drivers/firmware/efi/efi-helper.c > @@ -0,0 +1,33 @@ > +/* > + * Common [U]EFI support helper functions across architectures. > + */ > + > +#include > + > +/* > + * We can't ioremap data in EFI boot services RAM, because we've already > mapped > + * it as RAM. So, look it up in the existing EFI memory map instead. Only > + * callable after efi_enter_virtual_mode and before efi_free_boot_services. > + */ > +void __iomem *efi_lookup_mapped_addr(u64 phys_addr) Can be __init annotated. Although it's a good idea to do that in a separate patch. Otherwise looks good to me. Reviewed-by: Grant Likely -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] init: efi: arm: enable (U)EFI runtime services on arm
On Tue, Jun 25, 2013 at 7:11 PM, Leif Lindholm wrote: > Since the efi_set_virtual_address_map call has strict init ordering > requirements, add an explicit hook in the required place. > > Signed-off-by: Leif Lindholm > --- > init/main.c |6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/init/main.c b/init/main.c > index 9484f4b..c61706e 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -872,6 +872,12 @@ static noinline void __init kernel_init_freeable(void) > smp_prepare_cpus(setup_max_cpus); > > do_pre_smp_initcalls(); > + > +#ifdef CONFIG_ARM > + if (efi_enabled(EFI_RUNTIME_SERVICES)) > + efi_enter_virtual_mode(); > +#endif Nitpick: Alternately you could use: if (IS_ENABLED(CONFIG_ARM) && efi_enabled(EFI_RUNTIME_SERVICES)) efi_enter_virtual_mode(); That would get away from the #ifdef g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services
On Wed, Jun 26, 2013 at 12:42 AM, Stephen Warren wrote: > On 06/25/2013 12:11 PM, Leif Lindholm wrote: >> This patch provides documentation of the [U]EFI runtime services and >> configuration features. > > >> diff --git a/Documentation/arm/uefi.txt b/Documentation/arm/uefi.txt > >> +The implementation depends on receiving pointers to the UEFI memory map >> +and System Table in a Flattened Device Tree - so is only available with >> +CONFIG_OF. >> + >> +It (early) parses the FDT for the following parameters: > > Part of this document (the raw requirements for DT content, rather than > the discussion of OS implementation/behaviour in parsing/interpreting > the properties) should be part of a file in > Documentation/devicetree/bindings/ (arm/uefi.txt?). > > What node are these properties expected to be contained within? > > Shouldn't that node be required to contain a compatible value, which > would define the schema for the other properties? Typically, a compatible property isn't only used for nodes that represent a device or a so-called 'virtual' device (ie. such as to describe how all the sound complex is wired together) since that should be the clue to an OS that a device driver will bind against the node. I think these properties can be dropped into /chosen without defining a new compatible value. >> +- 'efi-system-table': >> + Physical address of the system table. (required) >> +- 'efi-runtime-mmap': >> + Physical address of an EFI memory map, containing at least >> + the regions to be preserved. (required) >> +- 'efi-runtime-mmap-size': >> + Size in bytes of the provided memory map. (required) >> +- 'efi-mmap-desc-size': >> + Size of each descriptor in the memory map. (override default) >> +- 'efi-mmap-desc-ver': >> + Memory descriptor format version. (override default) >> + >> +Since UEFI firmware on ARM systems are required to use a 1:1 memory map >> +even on LPAE-capable systems, the above fields are 32-bit regardless. > > What about ARMv8? Is the intention to have a separate definition for the > UEFI bindings on ARMv8, so that compatibility isn't an issue? What if a > future version of UEFI allows LPAE usage? It is unlikely that will happen on v7 since newer versions of UEFI are expected to remain backwards compatible with the current spec. > It may be better to explicitly state that the size of those properties > is either #address-cells from the parent node (presumably the top-level > of the DT), and/or introduce some property to explicitly state the size > of the properties. Those mechanisms would allow forward-compatibility to > LPAE usage or ARMv8 without requiring the text of the binding definition > to change. > > Also, it seems legal to state the physical addresses using 64-bits even > if the actual values themselves are restricted to 32-bit range by the > UEFI spec. Illegal values would presumably cause SW that interprets them > to fail error-checks, and be rejected. > >> +After the kernel has mapped the required regions into its address space, >> +a SetVirtualAddressMap() call is made into UEFI in order to update >> +relocations. This call must be performed with all the code in a 1:1 > > Presumably "all the code" also includes "all .data and .bss", or > whatever the UEFI-equivalent may be? I'm not familiar with UEFI at all; > does the "EFI memory map" mentioned above describe all the memory > regions that must be mapped to use UEFI? yes. Actually, there is an API for retrieving the memory map from UEFI at runtime, but it is difficult to call from within the kernel. Actually, my preference would be to not require GRUB or the Linux Loader to add the above properties at all and instead have the kernel proper retrieve the memory map directly. That would reduce the dependency on GRUB/LinuxLoader doing things correctly, but Leif knows better how feasible that would be. > >> +mapping. This implementation achieves this by temporarily disabling the >> +MMU for the duration of this call. This can only be done safely: >> +- before secondary CPUs are brought online. >> +- after early_initcalls have completed, sinze it uses setup_mm_for_reboot(). > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] Documentation: arm: [U]EFI runtime services
On Tue, Jun 25, 2013 at 7:11 PM, Leif Lindholm wrote: > This patch provides documentation of the [U]EFI runtime services and > configuration features. > > Signed-off-by: Leif Lindholm > --- > Documentation/arm/00-INDEX |3 +++ > Documentation/arm/uefi.txt | 39 +++ > 2 files changed, 42 insertions(+) > create mode 100644 Documentation/arm/uefi.txt > > diff --git a/Documentation/arm/00-INDEX b/Documentation/arm/00-INDEX > index 4978456..87e01d1 100644 > --- a/Documentation/arm/00-INDEX > +++ b/Documentation/arm/00-INDEX > @@ -36,3 +36,6 @@ nwfpe/ > - NWFPE floating point emulator documentation > swp_emulation > - SWP/SWPB emulation handler/logging description > + > +uefi.txt > + - [U]EFI configuration and runtime services documentation > diff --git a/Documentation/arm/uefi.txt b/Documentation/arm/uefi.txt > new file mode 100644 > index 000..5c48271 > --- /dev/null > +++ b/Documentation/arm/uefi.txt > @@ -0,0 +1,39 @@ > +The nomenclature EFI and UEFI are used interchangeably in this document. > + > +The implementation depends on receiving pointers to the UEFI memory map > +and System Table in a Flattened Device Tree - so is only available with > +CONFIG_OF. > + > +It (early) parses the FDT for the following parameters: Need to state which node these properties can be found in. I recommend /chosen I would also prefix all of the following properties with "linux," since they are very much about what the Linux kernel needs for booting. EFI stub will be creating these properties specifically for Linux, and the LinuxLoader should do the same (until we eventually kill it). > +- 'efi-system-table': > + Physical address of the system table. (required) Need to mention the size of this address. Is it 64 bit or 32 bit? I would follow the lead of 'linux,initrd-start' here and make the size of property the size of the address. ie. If it is 8 bytes long, then it is a 64 bit address. > +- 'efi-runtime-mmap': > + Physical address of an EFI memory map, containing at least > + the regions to be preserved. (required) > +- 'efi-runtime-mmap-size': > + Size in bytes of the provided memory map. (required) I would collapse the above two properties into a single property that actually contains the memory map instead of pointing to it. You will also need to specify the exact format of the data in this property. > +- 'efi-mmap-desc-size': > + Size of each descriptor in the memory map. (override default) > +- 'efi-mmap-desc-ver': > + Memory descriptor format version. (override default) I don't understand how these properties will actually work. What changes in the parsing if these properties are set? > + > +Since UEFI firmware on ARM systems are required to use a 1:1 memory map > +even on LPAE-capable systems, the above fields are 32-bit regardless. > + > +It also depends on early_ioremap to parse the memory map and preserve > +the regions required for runtime services. > + > +For actually enabling [U]EFI support, enable: > +- CONFIG_EFI=y > +- CONFIG_EFI_VARS=y or m > + > +After the kernel has mapped the required regions into its address space, > +a SetVirtualAddressMap() call is made into UEFI in order to update > +relocations. This call must be performed with all the code in a 1:1 > +mapping. This implementation achieves this by temporarily disabling the > +MMU for the duration of this call. This can only be done safely: > +- before secondary CPUs are brought online. > +- after early_initcalls have completed, sinze it uses setup_mm_for_reboot(). > + > +For verbose debug messages, specify 'uefi_debug' on the kernel command > +line. Looks good otherwise, Thanks! g. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
