[U-Boot] [PATCH v4 1/1] efi_loader: PSCI reset and shutdown
Alexander Graf
agraf at suse.de
Thu Oct 18 07:13:10 UTC 2018
On 17.10.18 20:06, Heinrich Schuchardt wrote:
> On 10/17/2018 07:37 PM, Alexander Graf wrote:
>>
>>
>> On 17.10.18 19:05, 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 do_poweroff
>>> and reset_misc routines let's move them into the same code module.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>> Reviewed-by: Sumit Garg <sumit.garg at linaro.org>
>>> Tested-by: Sumit Garg <sumit.garg at linaro.org>
>>> ---
>>> Travis reporteded no errors in
>>> https://travis-ci.org/xypron2/u-boot/builds/442613876
>>>
>>> v4:
>>> fix reset_misc() too
>>> avoid switch statement
>>> 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 | 100 +++++++++++++++++++++++++++-----
>>> include/linux/psci.h | 6 +-
>>> 6 files changed, 96 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..7ec5eb094b 100644
>>> --- a/drivers/firmware/psci.c
>>> +++ b/drivers/firmware/psci.c
>>> @@ -9,31 +9,38 @@
>>> #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);
>>> + /*
>>> + * In the __efi_runtime we need to avoid the switch statement. In some
>>> + * cases the compiler creates lookup tables to implement switch. These
>>> + * tables are not correctly relocated when SetVirtualAddressMap is
>>> + * called.
>>> + */
>>> + if (psci_method == PSCI_METHOD_SMC)
>>> + arm_smccc_smc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
>>> + else if (psci_method == PSCI_METHOD_HVC)
>>> + arm_smccc_hvc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
>>> + else
>>> + res.a0 = PSCI_RET_DISABLED;
>>> return res.a0;
>>> }
>>>
>>> @@ -67,9 +74,9 @@ static int psci_probe(struct udevice *dev)
>>> }
>>>
>>> if (!strcmp("hvc", method)) {
>>> - invoke_psci_fn = __invoke_psci_fn_hvc;
>>> + psci_method = PSCI_METHOD_HVC;
>>> } else if (!strcmp("smc", method)) {
>>> - invoke_psci_fn = __invoke_psci_fn_smc;
>>> + psci_method = PSCI_METHOD_SMC;
>>> } else {
>>> pr_warn("invalid \"method\" property: %s\n", method);
>>> return -EINVAL;
>>> @@ -78,6 +85,67 @@ static int psci_probe(struct udevice *dev)
>>> return 0;
>>> }
>>>
>>> +/**
>>> + * void do_psci_probe() - probe PSCI firmware driver
>>> + *
>>> + * Ensure that psci_method is initialized.
>>> + */
>>> +static void __maybe_unused do_psci_probe(void)
>>> +{
>>> + struct udevice *dev;
>>> +
>>> + uclass_get_device_by_name(UCLASS_FIRMWARE, DRIVER_NAME, &dev);
>>> +}
>>> +
>>> +#ifdef CONFIG_EFI_LOADER
>>
>> I think this should be #if IS_ENABLED(CONFIG_EFI_LOADER) &&
>> IS_ENABLED(CONFIG_PSCI_RESET), as otherwise we may trigger unexpected
>> PSCI resets from efi land that wouldn't happen with normal cmds.
>>
>>
>> Alex
>
> U-Boot is the little servant that helps the rider on the horse and
> neither horse nor rider.
>
> Hence U-Boot should not unnecessarily interfere with the communication
> between the operating system and the PSCI firmware.
>
> I cannot imagine any use case where the authorized user in the operating
> system executes a reboot or shutdown command and expects the system
> simply not to do it.
I agree, but I don't think that we should do vastly different things in
the efi RTS than what you get with commands.
Without this patch, efi RTS will just fail when the board chooses not to
use PSCI reset, but does choose to use SMCCC.
With this patch, efi RTS will now call PSCI for reset even when the
board explicitly said it does not want PSCI for reset.
Now whether it's a smart thing to allow non-PSCI reset in the first
place is an orthogonal discussion. I personally think it's wrong :). But
it does change the RTS behavior at the same time as the refactoring.
That means that if one of the two breaks something, we have to revert
the full patch, not just the one change that broke something.
Alex
More information about the U-Boot
mailing list