[PATCH v3 3/3] sysreset: provide SBI based sysreset driver
Heinrich Schuchardt
xypron.glpk at gmx.de
Mon Sep 6 07:58:27 CEST 2021
Am 6. September 2021 07:22:21 MESZ schrieb Bin Meng <bmeng.cn at gmail.com>:
>On Mon, Sep 6, 2021 at 12:45 PM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>> Am 6. September 2021 03:47:39 MESZ schrieb Bin Meng <bmeng.cn at gmail.com>:
>> >On Mon, Sep 6, 2021 at 1:21 AM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>> >>
>> >> On 9/5/21 7:00 PM, Bin Meng wrote:
>> >> > Hi Heinrich,
>> >> >
>> >> > On Mon, Sep 6, 2021 at 12:50 AM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>> >> >>
>> >> >> On 9/5/21 1:59 PM, Bin Meng wrote:
>> >> >>> On Sun, Sep 5, 2021 at 4:38 PM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>> >> >>>>
>> >> >>>> Provide sysreset driver using the SBI system reset extension.
>> >> >>>>
>> >> >>>
>> >> >>> This patch should be split into 2 patches, one for adding the sysreset
>> >> >>> DM driver, and the other one for EFI support.
>> >> >>>
>> >> >>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>> >> >>>> ---
>> >> >>>> v3:
>> >> >>>> no change
>> >> >>>> ---
>> >> >>>> MAINTAINERS | 1 +
>> >> >>>> arch/riscv/cpu/cpu.c | 13 ++++-
>> >> >>>> arch/riscv/include/asm/sbi.h | 1 +
>> >> >>>> arch/riscv/lib/sbi.c | 21 ++++++--
>> >> >>>> drivers/sysreset/Kconfig | 11 ++++
>> >> >>>> drivers/sysreset/Makefile | 1 +
>> >> >>>> drivers/sysreset/sysreset_sbi.c | 96 +++++++++++++++++++++++++++++++++
>> >> >>>> lib/efi_loader/Kconfig | 2 +-
>> >> >>>> 8 files changed, 140 insertions(+), 6 deletions(-)
>> >> >>>> create mode 100644 drivers/sysreset/sysreset_sbi.c
>> >> >>>>
>> >> >>>> diff --git a/MAINTAINERS b/MAINTAINERS
>> >> >>>> index 4cf0c33c5d..88d7aa2bc7 100644
>> >> >>>> --- a/MAINTAINERS
>> >> >>>> +++ b/MAINTAINERS
>> >> >>>> @@ -1017,6 +1017,7 @@ T: git https://source.denx.de/u-boot/custodians/u-boot-riscv.git
>> >> >>>> F: arch/riscv/
>> >> >>>> F: cmd/riscv/
>> >> >>>> F: doc/usage/sbi.rst
>> >> >>>> +F: drivers/sysreset/sysreset_sbi.c
>> >> >>>> F: drivers/timer/andes_plmt_timer.c
>> >> >>>> F: drivers/timer/sifive_clint_timer.c
>> >> >>>> F: tools/prelink-riscv.c
>> >> >>>> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
>> >> >>>> index c894ac10b5..8e49b6d736 100644
>> >> >>>> --- a/arch/riscv/cpu/cpu.c
>> >> >>>> +++ b/arch/riscv/cpu/cpu.c
>> >> >>>> @@ -6,6 +6,7 @@
>> >> >>>> #include <common.h>
>> >> >>>> #include <cpu.h>
>> >> >>>> #include <dm.h>
>> >> >>>> +#include <dm/lists.h>
>> >> >>>> #include <init.h>
>> >> >>>> #include <log.h>
>> >> >>>> #include <asm/encoding.h>
>> >> >>>> @@ -138,7 +139,17 @@ int arch_cpu_init_dm(void)
>> >> >>>>
>> >> >>>> int arch_early_init_r(void)
>> >> >>>> {
>> >> >>>> - return riscv_cpu_probe();
>> >> >>>> + int ret;
>> >> >>>> +
>> >> >>>> + ret = riscv_cpu_probe();
>> >> >>>> + if (ret)
>> >> >>>> + return ret;
>> >> >>>> +
>> >> >>>> + if (IS_ENABLED(CONFIG_SYSRESET_SBI))
>> >> >>>> + device_bind_driver(gd->dm_root, "sbi-sysreset",
>> >> >>>> + "sbi-sysreset", NULL);
>> >> >>>> +
>> >> >>>> + return 0;
>> >> >>>> }
>> >> >>>>
>> >> >>>> /**
>> >> >>>> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
>> >> >>>> index e9caa78d17..69cddda245 100644
>> >> >>>> --- a/arch/riscv/include/asm/sbi.h
>> >> >>>> +++ b/arch/riscv/include/asm/sbi.h
>> >> >>>> @@ -154,5 +154,6 @@ void sbi_set_timer(uint64_t stime_value);
>> >> >>>> long sbi_get_spec_version(void);
>> >> >>>> int sbi_get_impl_id(void);
>> >> >>>> int sbi_probe_extension(int ext);
>> >> >>>> +void sbi_srst_reset(unsigned long type, unsigned long reason);
>> >> >>>>
>> >> >>>> #endif
>> >> >>>> diff --git a/arch/riscv/lib/sbi.c b/arch/riscv/lib/sbi.c
>> >> >>>> index 77845a73ca..8508041f2a 100644
>> >> >>>> --- a/arch/riscv/lib/sbi.c
>> >> >>>> +++ b/arch/riscv/lib/sbi.c
>> >> >>>> @@ -8,13 +8,14 @@
>> >> >>>> */
>> >> >>>>
>> >> >>>> #include <common.h>
>> >> >>>> +#include <efi_loader.h>
>> >> >>>> #include <asm/encoding.h>
>> >> >>>> #include <asm/sbi.h>
>> >> >>>>
>> >> >>>> -struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
>> >> >>>> - unsigned long arg1, unsigned long arg2,
>> >> >>>> - unsigned long arg3, unsigned long arg4,
>> >> >>>> - unsigned long arg5)
>> >> >>>> +struct sbiret __efi_runtime sbi_ecall(int ext, int fid, unsigned long arg0,
>> >> >>>> + unsigned long arg1, unsigned long arg2,
>> >> >>>> + unsigned long arg3, unsigned long arg4,
>> >> >>>> + unsigned long arg5)
>> >> >>>> {
>> >> >>>> struct sbiret ret;
>> >> >>>>
>> >> >>>> @@ -108,6 +109,18 @@ int sbi_probe_extension(int extid)
>> >> >>>> return -ENOTSUPP;
>> >> >>>> }
>> >> >>>>
>> >> >>>> +/**
>> >> >>>> + * sbi_srst_reset() - invoke system reset extension
>> >> >>>> + *
>> >> >>>> + * @type: type of reset
>> >> >>>> + * @reason: reason for reset
>> >> >>>> + */
>> >> >>>> +void __efi_runtime sbi_srst_reset(unsigned long type, unsigned long reason)
>> >> >>>> +{
>> >> >>>> + sbi_ecall(SBI_EXT_SRST, SBI_EXT_SRST_RESET, type, reason,
>> >> >>>> + 0, 0, 0, 0);
>> >> >>>> +}
>> >> >>>> +
>> >> >>>> #ifdef CONFIG_SBI_V01
>> >> >>>>
>> >> >>>> /**
>> >> >>>> diff --git a/drivers/sysreset/Kconfig b/drivers/sysreset/Kconfig
>> >> >>>> index ac77ffbc8b..6782331181 100644
>> >> >>>> --- a/drivers/sysreset/Kconfig
>> >> >>>> +++ b/drivers/sysreset/Kconfig
>> >> >>>> @@ -85,6 +85,17 @@ config SYSRESET_PSCI
>> >> >>>> Enable PSCI SYSTEM_RESET function call. To use this, PSCI firmware
>> >> >>>> must be running on your system.
>> >> >>>>
>> >> >>>> +config SYSRESET_SBI
>> >> >>>> + bool "Enable support for SBI System Reset"
>> >> >>>> + depends on RISCV_SMODE && SBI_V02
>> >> >>>> + select SYSRESET_CMD_POWEROFF if CMD_POWEROFF
>> >> >>>> + help
>> >> >>>> + Enable system reset and poweroff via the SBI system reset extension.
>> >> >>>> + If the SBI implementation provides the extension, is board specific.
>> >> >>>> + The extension was introduced in version 0.3 of the SBI specification.
>> >> >>>> + The SBI system reset driver supports the UEFI ResetSystem() service
>> >> >>>> + at runtime.
>> >> >>>> +
>> >> >>>> config SYSRESET_SOCFPGA
>> >> >>>> bool "Enable support for Intel SOCFPGA family"
>> >> >>>> depends on ARCH_SOCFPGA && (TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_ARRIA10)
>> >> >>>> diff --git a/drivers/sysreset/Makefile b/drivers/sysreset/Makefile
>> >> >>>> index de81c399d7..8e00be0779 100644
>> >> >>>> --- a/drivers/sysreset/Makefile
>> >> >>>> +++ b/drivers/sysreset/Makefile
>> >> >>>> @@ -13,6 +13,7 @@ obj-$(CONFIG_SYSRESET_MPC83XX) += sysreset_mpc83xx.o
>> >> >>>> obj-$(CONFIG_SYSRESET_MICROBLAZE) += sysreset_microblaze.o
>> >> >>>> obj-$(CONFIG_SYSRESET_OCTEON) += sysreset_octeon.o
>> >> >>>> obj-$(CONFIG_SYSRESET_PSCI) += sysreset_psci.o
>> >> >>>> +obj-$(CONFIG_SYSRESET_SBI) += sysreset_sbi.o
>> >> >>>> obj-$(CONFIG_SYSRESET_SOCFPGA) += sysreset_socfpga.o
>> >> >>>> obj-$(CONFIG_SYSRESET_SOCFPGA_SOC64) += sysreset_socfpga_soc64.o
>> >> >>>> obj-$(CONFIG_SYSRESET_TI_SCI) += sysreset-ti-sci.o
>> >> >>>> diff --git a/drivers/sysreset/sysreset_sbi.c b/drivers/sysreset/sysreset_sbi.c
>> >> >>>> new file mode 100644
>> >> >>>> index 0000000000..fec5a66515
>> >> >>>> --- /dev/null
>> >> >>>> +++ b/drivers/sysreset/sysreset_sbi.c
>> >> >>>> @@ -0,0 +1,96 @@
>> >> >>>> +// SPDX-License-Identifier: GPL-2.0+
>> >> >>>> +/*
>> >> >>>> + * Copyright 2021, Heinrich Schuchardt <xypron.glpk at gmx.de>
>> >> >>>> + */
>> >> >>>> +
>> >> >>>> +#include <common.h>
>> >> >>>> +#include <dm.h>
>> >> >>>> +#include <errno.h>
>> >> >>>> +#include <efi_loader.h>
>> >> >>>> +#include <log.h>
>> >> >>>> +#include <sysreset.h>
>> >> >>>> +#include <asm/sbi.h>
>> >> >>>> +
>> >> >>>> +static long __efi_runtime_data have_reset;
>> >> >>>> +
>> >> >>>> +static int sbi_sysreset_request(struct udevice *dev, enum sysreset_t type)
>> >> >>>> +{
>> >> >>>> + enum sbi_srst_reset_type reset_type;
>> >> >>>> +
>> >> >>>> + switch (type) {
>> >> >>>> + case SYSRESET_WARM:
>> >> >>>> + reset_type = SBI_SRST_RESET_TYPE_WARM_REBOOT;
>> >> >>>> + break;
>> >> >>>> + case SYSRESET_COLD:
>> >> >>>> + reset_type = SBI_SRST_RESET_TYPE_COLD_REBOOT;
>> >> >>>> + break;
>> >> >>>> + case SYSRESET_POWER_OFF:
>> >> >>>> + reset_type = SBI_SRST_RESET_TYPE_SHUTDOWN;
>> >> >>>> + break;
>> >> >>>> + default:
>> >> >>>> + log_err("SBI has no system reset extension\n");
>> >> >>>> + return -ENOSYS;
>> >> >>>> + }
>> >> >>>> +
>> >> >>>> + sbi_srst_reset(reset_type, SBI_SRST_RESET_REASON_NONE);
>> >> >>>> +
>> >> >>>> + return -EINPROGRESS;
>> >> >>>> +}
>> >> >>>> +
>> >> >>>> +efi_status_t efi_reset_system_init(void)
>> >> >>>> +{
>> >> >>>> + return EFI_SUCCESS;
>> >> >>>> +}
>> >> >>>
>> >> >>> Is there a better place for the EFI stuff?
>> >> >>
>> >> >> Bin, thanks for reviewing the series.
>> >> >>
>> >> >> This seems to be related to you comment above about splitting into two
>> >> >> patches.
>> >> >
>> >> > Yep.
>> >> >
>> >> >> We put have the same set of EFI runtime functions for system
>> >> >> reset in:
>> >> >>
>> >> >> drivers/sysreset/sysreset_x86.c
>> >> >> drivers/firmware/psci.c
>> >> >> arch/arm/mach-bcm283x/reset.c
>> >> >> arch/arm/cpu/armv8/fsl-layerscape/cpu.c
>> >> >
>> >> > I didn't realize that. But this does not look correct to me.
>> >> >
>> >> > EFI reset should be independent of the system reset drivers. For
>> >> > example, on RISC-V SRST is optional so not every SBI implements this.
>> >> > On SiFive Unleashed, the "gpio-restart" driver is used for reset (see
>> >> > sysreset_gpio.c).
>> >>
>> >> The EFI functions that you see in this patch are for the runtime, i.e.
>> >> after ExitBootServices().
>> >>
>> >> When you issue the reboot or poweroff command in Linux it will call the
>> >> UEFI runtime. At this point all driver model code is gone. So we cannot
>> >> call any DM U-Boot driver. This is why these two functions are marked as
>> >> __efi_runtime.
>>
>> When using a system with multiple GiB of memory one could argue that freeing a MiB of boottime U-Boot memory at ExitBootServices is not worth the effort.
>>
>> On the other side having a very small runtime to consider for security analysis also has its merrits.
>
>Agree, but my point is that current U-Boot EFI loader approach of
>marking runtime functions with __efi_runtime is not scalable. You have
>to trace down all callees to make sure they are marked with
>__efi_runtime by an __efi_runtime caller, like you did in this patch.
>But my test on EFI loader on x86 Linux in the past suggested that we
>missed something in the calling path.
>
>> Using platform independent reset methods like PSCI and SBI allows to limit the runtime code base. We should avoid board specific code.
>
>There are platforms without PSCI and SBI but want EFI support.
>
>> But could you, please, answer my original question: Where do you want the SBI runtime code placed?
>>
>
>I still think we should implement the EFI reset via DM APIs, so as I
>mentioned the runtime codes can be put in the common efi_loader
>directory.
After ExitBootServices Linux calls SetVirtualAdressMap. The DM code would have to update all pointers it uses afterwards according to Linux' memory mapping.
So instead of spreading __efi_runtime you will be spreading pointer update code. This will lead to an increase of the U-Boot binary size for all boards and add complexity.
I am in doubt here.
Best regards
Heinrich
More information about the U-Boot
mailing list