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.g...@gmx.de> >> Reviewed-by: Sumit Garg <sumit.g...@linaro.org> >> Tested-by: Sumit Garg <sumit.g...@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. Best regards Heinrich _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
