[U-Boot] [PATCH v3 1/1] efi_loader: PSCI reset and shutdown
Alexander Graf
agraf at suse.de
Wed Oct 17 06:42:46 UTC 2018
On 17.10.18 07:56, Heinrich Schuchardt wrote:
> On 10/16/2018 02:46 PM, Alexander Graf wrote:
>>
>>
>> On 15.10.18 18:43, Heinrich Schuchardt wrote:
>>> When an operating system started via bootefi tries to reset or power off
>>> this is done by calling the EFI runtime ResetSystem(). On most ARMv8 system
>>> the actual reset relies on PSCI. Depending on whether the PSCI firmware
>>> resides the hypervisor (EL2) or in the secure monitor (EL3) either an HVC
>>> or an SMC command has to be issued.
>>>
>>> The current implementation always uses SMC. This results in crashes on
>>> systems where the PSCI firmware is implemented in the hypervisor, e.g.
>>> qemu-arm64_defconfig.
>>>
>>> The logic to decide which call is needed based on the device tree is already
>>> implemented in the PSCI firmware driver. During the EFI runtime the device
>>> driver model is not available. But we can minimize code duplication by
>>> merging the EFI runtime reset and poweroff code with the PSCI firmware
>>> driver.
>>>
>>> As the same HVC/SMC problem is also evident for the ARMv8 poweroff command
>>> let's move it into the same code module.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>> ---
>>> Travis reporteded no errors in
>>> https://travis-ci.org/xypron2/u-boot/builds/440843498
>>> except for two boards with an image size problem.
>>
>> What exactly triggered the size problem? Where did we grow by how much
>> to trip them over?
>
> It is not this patch. It is all the EFI patches that I added.
> These are exactly the same boards for which we disabled the Unicode
> table for the same reason.
>
>>
>>>
>>> v3:
>>> update commit message
>>> v2:
>>> Use PSCI firmware driver to detect call method.
>>> Thanks to Sumit for pointing me at dtb parsing as part of
>>> "psci_probe()
>>> ---
>>> arch/arm/cpu/armv7/smccc-call.S | 2 +
>>> arch/arm/cpu/armv8/Kconfig | 1 +
>>> arch/arm/cpu/armv8/fwcall.c | 52 ++-----------------
>>> arch/arm/cpu/armv8/smccc-call.S | 2 +
>>> drivers/firmware/psci.c | 91 +++++++++++++++++++++++++++------
>>> include/linux/psci.h | 6 +--
>>> 6 files changed, 87 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/arch/arm/cpu/armv7/smccc-call.S b/arch/arm/cpu/armv7/smccc-call.S
>>> index 0d8b59eb6b..eae69e36c3 100644
>>> --- a/arch/arm/cpu/armv7/smccc-call.S
>>> +++ b/arch/arm/cpu/armv7/smccc-call.S
>>> @@ -7,6 +7,8 @@
>>> #include <asm/opcodes-sec.h>
>>> #include <asm/opcodes-virt.h>
>>>
>>> + .section .text.efi_runtime
>>> +
>>> #define UNWIND(x...)
>>> /*
>>> * Wrap c macros in asm macros to delay expansion until after the
>>> diff --git a/arch/arm/cpu/armv8/Kconfig b/arch/arm/cpu/armv8/Kconfig
>>> index c8bebabdf6..d643981b73 100644
>>> --- a/arch/arm/cpu/armv8/Kconfig
>>> +++ b/arch/arm/cpu/armv8/Kconfig
>>> @@ -96,6 +96,7 @@ endmenu
>>> config PSCI_RESET
>>> bool "Use PSCI for reset and shutdown"
>>> default y
>>> + select ARM_SMCCC if OF_CONTROL
>>> depends on !ARCH_EXYNOS7 && !ARCH_BCM283X && \
>>> !TARGET_LS2080A_SIMU && !TARGET_LS2080AQDS && \
>>> !TARGET_LS2080ARDB && !TARGET_LS2080A_EMU && \
>>> diff --git a/arch/arm/cpu/armv8/fwcall.c b/arch/arm/cpu/armv8/fwcall.c
>>> index 0ba3dad8cc..9957c2974b 100644
>>> --- a/arch/arm/cpu/armv8/fwcall.c
>>> +++ b/arch/arm/cpu/armv8/fwcall.c
>>> @@ -7,7 +7,6 @@
>>>
>>> #include <asm-offsets.h>
>>> #include <config.h>
>>> -#include <efi_loader.h>
>>> #include <version.h>
>>> #include <asm/macro.h>
>>> #include <asm/psci.h>
>>> @@ -19,7 +18,7 @@
>>> * x0~x7: input arguments
>>> * x0~x3: output arguments
>>> */
>>> -static void __efi_runtime hvc_call(struct pt_regs *args)
>>> +static void hvc_call(struct pt_regs *args)
>>> {
>>> asm volatile(
>>> "ldr x0, %0\n"
>>> @@ -53,7 +52,7 @@ static void __efi_runtime hvc_call(struct pt_regs *args)
>>> * x0~x3: output arguments
>>> */
>>>
>>> -void __efi_runtime smc_call(struct pt_regs *args)
>>> +void smc_call(struct pt_regs *args)
>>> {
>>> asm volatile(
>>> "ldr x0, %0\n"
>>> @@ -83,9 +82,9 @@ void __efi_runtime smc_call(struct pt_regs *args)
>>> * use PSCI on U-Boot running below a hypervisor, please detect
>>> * this and set the flag accordingly.
>>> */
>>> -static const __efi_runtime_data bool use_smc_for_psci = true;
>>> +static const bool use_smc_for_psci = true;
>>>
>>> -void __noreturn __efi_runtime psci_system_reset(void)
>>> +void __noreturn psci_system_reset(void)
>>> {
>>> struct pt_regs regs;
>>>
>>> @@ -100,7 +99,7 @@ void __noreturn __efi_runtime psci_system_reset(void)
>>> ;
>>> }
>>>
>>> -void __noreturn __efi_runtime psci_system_off(void)
>>> +void __noreturn psci_system_off(void)
>>> {
>>> struct pt_regs regs;
>>>
>>> @@ -114,44 +113,3 @@ void __noreturn __efi_runtime psci_system_off(void)
>>> while (1)
>>> ;
>>> }
>>> -
>>> -#ifdef CONFIG_CMD_POWEROFF
>>> -int do_poweroff(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>> -{
>>> - puts("poweroff ...\n");
>>> -
>>> - udelay(50000); /* wait 50 ms */
>>> -
>>> - disable_interrupts();
>>> -
>>> - psci_system_off();
>>> -
>>> - /*NOTREACHED*/
>>> - return 0;
>>> -}
>>> -#endif
>>> -
>>> -#ifdef CONFIG_PSCI_RESET
>>> -void reset_misc(void)
>>> -{
>>> - psci_system_reset();
>>> -}
>>> -
>>> -#ifdef CONFIG_EFI_LOADER
>>> -void __efi_runtime EFIAPI efi_reset_system(
>>> - enum efi_reset_type reset_type,
>>> - efi_status_t reset_status,
>>> - unsigned long data_size, void *reset_data)
>>> -{
>>> - if (reset_type == EFI_RESET_COLD ||
>>> - reset_type == EFI_RESET_WARM ||
>>> - reset_type == EFI_RESET_PLATFORM_SPECIFIC) {
>>> - psci_system_reset();
>>> - } else if (reset_type == EFI_RESET_SHUTDOWN) {
>>> - psci_system_off();
>>> - }
>>> -
>>> - while (1) { }
>>> -}
>>> -#endif /* CONFIG_EFI_LOADER */
>>> -#endif /* CONFIG_PSCI_RESET */
>>> diff --git a/arch/arm/cpu/armv8/smccc-call.S b/arch/arm/cpu/armv8/smccc-call.S
>>> index 16c9e298b4..86de4b4089 100644
>>> --- a/arch/arm/cpu/armv8/smccc-call.S
>>> +++ b/arch/arm/cpu/armv8/smccc-call.S
>>> @@ -6,6 +6,8 @@
>>> #include <linux/arm-smccc.h>
>>> #include <generated/asm-offsets.h>
>>>
>>> + .section .text.efi_runtime
>>> +
>>> .macro SMCCC instr
>>> .cfi_startproc
>>> \instr #0
>>> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
>>> index 2cb35f356f..2237668ea6 100644
>>> --- a/drivers/firmware/psci.c
>>> +++ b/drivers/firmware/psci.c
>>> @@ -9,31 +9,37 @@
>>> #include <common.h>
>>> #include <dm.h>
>>> #include <dm/lists.h>
>>> +#include <efi_loader.h>
>>> #include <linux/libfdt.h>
>>> #include <linux/arm-smccc.h>
>>> #include <linux/errno.h>
>>> #include <linux/printk.h>
>>> #include <linux/psci.h>
>>>
>>> -psci_fn *invoke_psci_fn;
>>> +#define DRIVER_NAME "psci"
>>>
>>> -static unsigned long __invoke_psci_fn_hvc(unsigned long function_id,
>>> - unsigned long arg0, unsigned long arg1,
>>> - unsigned long arg2)
>>> -{
>>> - struct arm_smccc_res res;
>>> +#define PSCI_METHOD_HVC 1
>>> +#define PSCI_METHOD_SMC 2
>>>
>>> - arm_smccc_hvc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
>>> - return res.a0;
>>> -}
>>> +int __efi_runtime_data psci_method;
>>>
>>> -static unsigned long __invoke_psci_fn_smc(unsigned long function_id,
>>> - unsigned long arg0, unsigned long arg1,
>>> - unsigned long arg2)
>>> +unsigned long __efi_runtime invoke_psci_fn
>>> + (unsigned long function_id, unsigned long arg0,
>>> + unsigned long arg1, unsigned long arg2)
>>> {
>>> struct arm_smccc_res res;
>>>
>>> - arm_smccc_smc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
>>> + switch (psci_method) {
>>> + case PSCI_METHOD_SMC:
>>> + arm_smccc_smc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
>>> + break;
>>> + case PSCI_METHOD_HVC:
>>> + arm_smccc_hvc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
>>> + break;
>>> + default:
>>> + res.a0 = PSCI_RET_DISABLED;
>>
>> We will never get here, because probing will fail with neither SMC nor
>> HVC set.
>>
>> Also, while I find switch cases more readable, gcc will do funky things
>> when it sees them, like generate jump tables and forgetting that it can
>> do constant propagation. I would prefer to stick with the bool based
>> approach here and just simply do either SMC or HVC.
>
> This function will be called via the EFI_RESET_SYSTEM runtime service
> irrespective of probe status. It is the value PSCI_RET_DISABLED that
> tells us that probe has failed.
Ah, so that's the case where we have PSCI configured in U-Boot, but PSCI
is not marked in DT? I didn't think of that, yeah ...
> Are you suggesting to remove all switch statements from U-Boot? If you
No, only in runtime code.
> have any evidence that gcc cannot correctly compile switch statements,
> please, address is to the GCC project. Or do you believe that the EFI
> relocation code is wrong?
We had bugs in the EFI runtime code before, where switch statements
generated implicit data which lead to the section rename patches. They
are really a pretty unloved child in the gcc world. Most optimization
passes only work on branches, not on switch cases.
But yeah, I see your argument for the no-PSCI case. We didn't have that
before. So I guess your patch works for me as is? I would personally
only push it into -next though, given its size and potential for side
effects.
Alex
More information about the U-Boot
mailing list