[PATCH] efi_loader: access __efi_runtime_start/stop without &
A symbol defined in a linker script (e.g. __efi_runtime_start = .;) is only a symbol, not a variable and should not be dereferenced. The common practice is either define it as extern uint32_t __efi_runtime_start or extern char __efi_runtime_start[] and access it as &__efi_runtime_start or __efi_runtime_start respectively.
So let's access it properly since we define it as an array
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- lib/efi_loader/efi_memory.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index edfad2d95a1d..98f104390c8d 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -933,8 +933,8 @@ static void add_u_boot_and_runtime(void) * Add Runtime Services. We mark surrounding boottime code as runtime as * well to fulfill the runtime alignment constraints but avoid padding. */ - runtime_start = (ulong)&__efi_runtime_start & ~runtime_mask; - runtime_end = (ulong)&__efi_runtime_stop; + runtime_start = (ulong)__efi_runtime_start & ~runtime_mask; + runtime_end = (ulong)__efi_runtime_stop; runtime_end = (runtime_end + runtime_mask) & ~runtime_mask; runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; efi_add_memory_map_pg(runtime_start, runtime_pages,
On 4/4/24 08:35, Ilias Apalodimas wrote:
A symbol defined in a linker script (e.g. __efi_runtime_start = .;) is only a symbol, not a variable and should not be dereferenced. The common practice is either define it as extern uint32_t __efi_runtime_start or extern char __efi_runtime_start[] and access it as &__efi_runtime_start or __efi_runtime_start respectively.
So let's access it properly since we define it as an array
Thanks for investigating this.
Beyond this patch I guess we should eliminate these duplicate defintions:
include/asm-generic/sections.h:38:extern char __efi_runtime_start[], __efi_runtime_stop[]; include/efi_loader.h:348:extern char __efi_runtime_start[], __efi_runtime_stop[];
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
lib/efi_loader/efi_memory.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index edfad2d95a1d..98f104390c8d 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -933,8 +933,8 @@ static void add_u_boot_and_runtime(void) * Add Runtime Services. We mark surrounding boottime code as runtime as * well to fulfill the runtime alignment constraints but avoid padding. */
- runtime_start = (ulong)&__efi_runtime_start & ~runtime_mask;
- runtime_end = (ulong)&__efi_runtime_stop;
- runtime_start = (ulong)__efi_runtime_start & ~runtime_mask;
Using (uintptr_t) would make it clearer that we are converting from a pointer to an integer type.
Best regards
Heinrich
- runtime_end = (ulong)__efi_runtime_stop; runtime_end = (runtime_end + runtime_mask) & ~runtime_mask; runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; efi_add_memory_map_pg(runtime_start, runtime_pages,
On Fri, 5 Apr 2024 at 10:12, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 4/4/24 08:35, Ilias Apalodimas wrote:
A symbol defined in a linker script (e.g. __efi_runtime_start = .;) is only a symbol, not a variable and should not be dereferenced. The common practice is either define it as extern uint32_t __efi_runtime_start or extern char __efi_runtime_start[] and access it as &__efi_runtime_start or __efi_runtime_start respectively.
So let's access it properly since we define it as an array
Thanks for investigating this.
Beyond this patch I guess we should eliminate these duplicate defintions:
include/asm-generic/sections.h:38:extern char __efi_runtime_start[], __efi_runtime_stop[]; include/efi_loader.h:348:extern char __efi_runtime_start[], __efi_runtime_stop[];
Yes, you've already sent a patch on this, thanks
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
lib/efi_loader/efi_memory.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index edfad2d95a1d..98f104390c8d 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -933,8 +933,8 @@ static void add_u_boot_and_runtime(void) * Add Runtime Services. We mark surrounding boottime code as runtime as * well to fulfill the runtime alignment constraints but avoid padding. */
runtime_start = (ulong)&__efi_runtime_start & ~runtime_mask;runtime_end = (ulong)&__efi_runtime_stop;
runtime_start = (ulong)__efi_runtime_start & ~runtime_mask;Using (uintptr_t) would make it clearer that we are converting from a pointer to an integer type.
Sure, I would prefer this to be on a followup with a commit message of its own, but I am fine with amending it, if you merge this
Thanks /Ilias
Best regards
Heinrich
runtime_end = (ulong)__efi_runtime_stop; runtime_end = (runtime_end + runtime_mask) & ~runtime_mask; runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; efi_add_memory_map_pg(runtime_start, runtime_pages,
participants (2)
-
Heinrich Schuchardt -
Ilias Apalodimas