[PATCH v3 3/3] sysreset: provide SBI based sysreset driver
Bin Meng
bmeng.cn at gmail.com
Mon Sep 6 07:22:21 CEST 2021
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.
Regards,
Bin
More information about the U-Boot
mailing list