[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