| Message ID | 20240404063535.469995-1-ilias.apalodimas@linaro.org |
|---|---|
| State | Accepted |
| Commit | c16248464f93be2254f32f67aaa24c7aa821136a |
| Headers | show |
| Series | efi_loader: access __efi_runtime_start/stop without & | expand |
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, >
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,
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(-)