[PATCH 0/3] efi_loader: save global data pointer on RISC-V
On the RISC-V platform register gp (x3) is used as global data pointer in U-Boot. When executing a UEFI binary we cannot assume that the payload will restore the register before calling the UEFI API. So U-Boot must take care of saving the register value before invoking the payload and restoring it whenever the API is called or the payload returns.
On some system like the Sipeed Maix we do not have a UEFI runtime reset implementation. Hence we call do_reset() in the UEFI selftest. Here too we need to restore the global data pointer before invoking the U-Boot API.
Heinrich Schuchardt (3): riscv: define function set_gd() efi_loader: save global data pointer on RISC-V efi_selftest: restore gd before do_reset()
arch/riscv/include/asm/global_data.h | 9 +++++ lib/efi_loader/efi_boottime.c | 54 ++++++++++++++-------------- lib/efi_selftest/efi_selftest.c | 6 ++-- 3 files changed, 40 insertions(+), 29 deletions(-)
-- 2.28.0
Function set_gd() is needed in the UEFI sub-system if the global data pointer is stored in a register.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- arch/riscv/include/asm/global_data.h | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h index 2eb14815bc..b711fcc44d 100644 --- a/arch/riscv/include/asm/global_data.h +++ b/arch/riscv/include/asm/global_data.h @@ -39,4 +39,13 @@ struct arch_global_data {
#define DECLARE_GLOBAL_DATA_PTR register gd_t *gd asm ("gp")
+static inline void set_gd(volatile gd_t *gd_ptr) +{ +#ifdef CONFIG_64BIT + asm volatile("ld gp, %0\n" : : "m"(gd_ptr)); +#else + asm volatile("lw gp, %0\n" : : "m"(gd_ptr)); +#endif +} + #endif /* __ASM_GBL_DATA_H */ -- 2.28.0
On 9/10/20 7:00 AM, Heinrich Schuchardt wrote:
Function set_gd() is needed in the UEFI sub-system if the global data pointer is stored in a register.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
arch/riscv/include/asm/global_data.h | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h index 2eb14815bc..b711fcc44d 100644 --- a/arch/riscv/include/asm/global_data.h +++ b/arch/riscv/include/asm/global_data.h @@ -39,4 +39,13 @@ struct arch_global_data {
#define DECLARE_GLOBAL_DATA_PTR register gd_t *gd asm ("gp")
+static inline void set_gd(volatile gd_t *gd_ptr) +{ +#ifdef CONFIG_64BIT
- asm volatile("ld gp, %0\n" : : "m"(gd_ptr));
+#else
- asm volatile("lw gp, %0\n" : : "m"(gd_ptr));
+#endif +}
#endif /* __ASM_GBL_DATA_H */
2.28.0
Can't we just use arch_setup_gd?
--Sean
On 10.09.20 13:04, Sean Anderson wrote:
On 9/10/20 7:00 AM, Heinrich Schuchardt wrote:
Function set_gd() is needed in the UEFI sub-system if the global data pointer is stored in a register.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
arch/riscv/include/asm/global_data.h | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h index 2eb14815bc..b711fcc44d 100644 --- a/arch/riscv/include/asm/global_data.h +++ b/arch/riscv/include/asm/global_data.h @@ -39,4 +39,13 @@ struct arch_global_data {
#define DECLARE_GLOBAL_DATA_PTR register gd_t *gd asm ("gp")
+static inline void set_gd(volatile gd_t *gd_ptr) +{ +#ifdef CONFIG_64BIT
- asm volatile("ld gp, %0\n" : : "m"(gd_ptr));
+#else
- asm volatile("lw gp, %0\n" : : "m"(gd_ptr));
+#endif +}
#endif /* __ASM_GBL_DATA_H */
2.28.0
Can't we just use arch_setup_gd?
The variables that we use to store gd have been defined as volatile to ensure that the compiler does not optimize them away. See lib/trace.c) and lib/efi_loader/efi_boottime.c. (A further patch for lib/trace.c will be needed if we want to trace UEFI API calls on RISC-V.)
arch_setup_gd() does not take a volatile variable.
I looked at using WRITE_ONCE() instead of volatile variables. But if gd is a register, &gd is undefined and WRITE_ONCE() is not usable.
Best regards
Heinrich
--Sean
On RISC-V the global data pointer is stored in register gp. When a UEFI binary calls the EFI API we have to restore it.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- lib/efi_loader/efi_boottime.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index dcd3eec894..bf78176217 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -42,9 +42,9 @@ LIST_HEAD(efi_register_notify_events); /* Handle of the currently executing image */ static efi_handle_t current_image;
-#ifdef CONFIG_ARM +#if defined(CONFIG_ARM) || defined(CONFIG_RISCV) /* - * The "gd" pointer lives in a register on ARM and AArch64 that we declare + * The "gd" pointer lives in a register on ARM and RISC-V that we declare * fixed when compiling U-Boot. However, the payload does not know about that * restriction so we need to manually swap its and our view of that register on * EFI callback entry/exit. @@ -86,7 +86,7 @@ static efi_status_t EFIAPI efi_disconnect_controller( int __efi_entry_check(void) { int ret = entry_count++ == 0; -#ifdef CONFIG_ARM +#if defined(CONFIG_ARM) || defined(CONFIG_RISCV) assert(efi_gd); app_gd = gd; set_gd(efi_gd); @@ -98,7 +98,7 @@ int __efi_entry_check(void) int __efi_exit_check(void) { int ret = --entry_count == 0; -#ifdef CONFIG_ARM +#if defined(CONFIG_ARM) || defined(CONFIG_RISCV) set_gd(app_gd); #endif return ret; @@ -107,7 +107,7 @@ int __efi_exit_check(void) /** * efi_save_gd() - save global data register * - * On the ARM architecture gd is mapped to a fixed register (r9 or x18). + * On the ARM and RISC-V architectures gd is mapped to a fixed register. * As this register may be overwritten by an EFI payload we save it here * and restore it on every callback entered. * @@ -115,7 +115,7 @@ int __efi_exit_check(void) */ void efi_save_gd(void) { -#ifdef CONFIG_ARM +#if defined(CONFIG_ARM) || defined(CONFIG_RISCV) efi_gd = gd; #endif } @@ -123,13 +123,13 @@ void efi_save_gd(void) /** * efi_restore_gd() - restore global data register * - * On the ARM architecture gd is mapped to a fixed register (r9 or x18). + * On the ARM and RISC-V architectures gd is mapped to a fixed register. * Restore it after returning from the UEFI world to the value saved via * efi_save_gd(). */ void efi_restore_gd(void) { -#ifdef CONFIG_ARM +#if defined(CONFIG_ARM) || defined(CONFIG_RISCV) /* Only restore if we're already in EFI context */ if (!efi_gd) return; @@ -2920,7 +2920,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, * us to the current line. This implies that the second half * of the EFI_CALL macro has not been executed. */ -#ifdef CONFIG_ARM +#if defined(CONFIG_ARM) || defined(CONFIG_RISCV) /* * efi_exit() called efi_restore_gd(). We have to undo this * otherwise __efi_entry_check() will put the wrong value into -- 2.28.0
Before calling do_reset() in the EFI selftest we must restore the global data pointer.
Fixes: fa63753f86cc ("efi_selftest: substitute ResetSystem() by do_reset()") Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- lib/efi_selftest/efi_selftest.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/lib/efi_selftest/efi_selftest.c b/lib/efi_selftest/efi_selftest.c index 6eec8ae2a7..165fa265f2 100644 --- a/lib/efi_selftest/efi_selftest.c +++ b/lib/efi_selftest/efi_selftest.c @@ -311,11 +311,13 @@ efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle, efi_st_printf("Preparing for reset. Press any key...\n"); efi_st_get_key();
- if (IS_ENABLED(CONFIG_EFI_HAVE_RUNTIME_RESET)) + if (IS_ENABLED(CONFIG_EFI_HAVE_RUNTIME_RESET)) { runtime->reset_system(EFI_RESET_WARM, EFI_NOT_READY, sizeof(reset_message), reset_message); - else + } else { + efi_restore_gd(); do_reset(NULL, 0, 0, NULL); + }
efi_st_printf("\n"); efi_st_error("Reset failed\n"); -- 2.28.0
On 9/10/20 7:00 AM, Heinrich Schuchardt wrote:
On the RISC-V platform register gp (x3) is used as global data pointer in U-Boot. When executing a UEFI binary we cannot assume that the payload will restore the register before calling the UEFI API. So U-Boot must take care of saving the register value before invoking the payload and restoring it whenever the API is called or the payload returns.
On some system like the Sipeed Maix we do not have a UEFI runtime reset implementation. Hence we call do_reset() in the UEFI selftest. Here too we need to restore the global data pointer before invoking the U-Boot API.
Heinrich Schuchardt (3): riscv: define function set_gd() efi_loader: save global data pointer on RISC-V efi_selftest: restore gd before do_reset()
arch/riscv/include/asm/global_data.h | 9 +++++ lib/efi_loader/efi_boottime.c | 54 ++++++++++++++-------------- lib/efi_selftest/efi_selftest.c | 6 ++-- 3 files changed, 40 insertions(+), 29 deletions(-)
-- 2.28.0
Do we need to save tp as well? It is used by secondary harts to save the hartid for handle_ipi.
--Sean
On 10.09.20 13:03, Sean Anderson wrote:
On 9/10/20 7:00 AM, Heinrich Schuchardt wrote:
On the RISC-V platform register gp (x3) is used as global data pointer in U-Boot. When executing a UEFI binary we cannot assume that the payload will restore the register before calling the UEFI API. So U-Boot must take care of saving the register value before invoking the payload and restoring it whenever the API is called or the payload returns.
On some system like the Sipeed Maix we do not have a UEFI runtime reset implementation. Hence we call do_reset() in the UEFI selftest. Here too we need to restore the global data pointer before invoking the U-Boot API.
Heinrich Schuchardt (3): riscv: define function set_gd() efi_loader: save global data pointer on RISC-V efi_selftest: restore gd before do_reset()
arch/riscv/include/asm/global_data.h | 9 +++++ lib/efi_loader/efi_boottime.c | 54 ++++++++++++++-------------- lib/efi_selftest/efi_selftest.c | 6 ++-- 3 files changed, 40 insertions(+), 29 deletions(-)
-- 2.28.0
Do we need to save tp as well? It is used by secondary harts to save the hartid for handle_ipi.
Only the boot hart enters the UEFI payload.
Best regards
--Sean
participants (2)
-
Heinrich Schuchardt -
Sean Anderson