* [PATCH 1/1] efi_loader: don't use EFI_LOADER_DATA internally
@ 2022-11-29 15:04 Heinrich Schuchardt
2022-11-29 15:38 ` Ilias Apalodimas
2022-11-29 15:41 ` François-Frédéric Ozog
0 siblings, 2 replies; 5+ messages in thread
From: Heinrich Schuchardt @ 2022-11-29 15:04 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: François-Frédéric Ozog, u-boot,
Heinrich Schuchardt
Memory allocated by U-Boot for internal usage should be
EFI_BOOT_SERVICES_DATA or _CODE or EFI_RUNTIME_SERVICES_DATA or _CODE.
Reported-by: François-Frédéric Ozog <ff@ozog.com>
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
cmd/efidebug.c | 2 +-
lib/efi_loader/efi_memory.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index ef239bb34b..64104da130 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -600,7 +600,7 @@ static int do_efi_show_memmap(struct cmd_tbl *cmdtp, int flag,
ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
if (ret == EFI_BUFFER_TOO_SMALL) {
map_size += sizeof(struct efi_mem_desc); /* for my own */
- ret = efi_allocate_pool(EFI_LOADER_DATA, map_size,
+ ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, map_size,
(void *)&memmap);
if (ret != EFI_SUCCESS)
return CMD_RET_FAILURE;
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index a17b426d11..0c336f98d2 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -823,7 +823,7 @@ static void add_u_boot_and_runtime(void)
uboot_stack_size) & ~EFI_PAGE_MASK;
uboot_pages = ((uintptr_t)map_sysmem(gd->ram_top - 1, 0) -
uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
- efi_add_memory_map_pg(uboot_start, uboot_pages, EFI_LOADER_DATA,
+ efi_add_memory_map_pg(uboot_start, uboot_pages, EFI_BOOT_SERVICES_DATA,
false);
#if defined(__aarch64__)
@@ -857,7 +857,7 @@ int efi_memory_init(void)
/* Request a 32bit 64MB bounce buffer region */
uint64_t efi_bounce_buffer_addr = 0xffffffff;
- if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_LOADER_DATA,
+ if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_BOOT_SERVICES_DATA,
(64 * 1024 * 1024) >> EFI_PAGE_SHIFT,
&efi_bounce_buffer_addr) != EFI_SUCCESS)
return -1;
--
2.37.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] efi_loader: don't use EFI_LOADER_DATA internally
2022-11-29 15:04 [PATCH 1/1] efi_loader: don't use EFI_LOADER_DATA internally Heinrich Schuchardt
@ 2022-11-29 15:38 ` Ilias Apalodimas
2022-11-29 17:35 ` Heinrich Schuchardt
2022-11-29 15:41 ` François-Frédéric Ozog
1 sibling, 1 reply; 5+ messages in thread
From: Ilias Apalodimas @ 2022-11-29 15:38 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: François-Frédéric Ozog, u-boot
Hi Heinrich,
On Tue, 29 Nov 2022 at 17:04, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Memory allocated by U-Boot for internal usage should be
> EFI_BOOT_SERVICES_DATA or _CODE or EFI_RUNTIME_SERVICES_DATA or _CODE.
Agreed, EFI_LOADER_DATA should be for EFI apps.
>
> Reported-by: François-Frédéric Ozog <ff@ozog.com>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
[...]
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index a17b426d11..0c336f98d2 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -823,7 +823,7 @@ static void add_u_boot_and_runtime(void)
> uboot_stack_size) & ~EFI_PAGE_MASK;
> uboot_pages = ((uintptr_t)map_sysmem(gd->ram_top - 1, 0) -
> uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
> - efi_add_memory_map_pg(uboot_start, uboot_pages, EFI_LOADER_DATA,
> + efi_add_memory_map_pg(uboot_start, uboot_pages, EFI_BOOT_SERVICES_DATA,
> false);
I am not sure if we should have this as _DATA or _CODE. None of these
is an exact match of what we allocate here and both of these are
backed by EFI_MEMORY_WB. So your reasoning here is prefer _DATA since
it's not memory that holds boottime service drivers?
[...]
Thanks
/Ilias
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] efi_loader: don't use EFI_LOADER_DATA internally
2022-11-29 15:04 [PATCH 1/1] efi_loader: don't use EFI_LOADER_DATA internally Heinrich Schuchardt
2022-11-29 15:38 ` Ilias Apalodimas
@ 2022-11-29 15:41 ` François-Frédéric Ozog
1 sibling, 0 replies; 5+ messages in thread
From: François-Frédéric Ozog @ 2022-11-29 15:41 UTC (permalink / raw)
To: Heinrich Schuchardt, Ilias Apalodimas; +Cc: u-boot
Reviewed-by: François-Frédéric Ozog <ff@ozog.com>
I confirm the patch addresses the issue
Le 29/11/2022 16:04, « Heinrich Schuchardt » <heinrich.schuchardt@canonical.com> a écrit :
Memory allocated by U-Boot for internal usage should be
EFI_BOOT_SERVICES_DATA or _CODE or EFI_RUNTIME_SERVICES_DATA or _CODE.
Reported-by: François-Frédéric Ozog <ff@ozog.com>
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
cmd/efidebug.c | 2 +-
lib/efi_loader/efi_memory.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index ef239bb34b..64104da130 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -600,7 +600,7 @@ static int do_efi_show_memmap(struct cmd_tbl *cmdtp, int flag,
ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
if (ret == EFI_BUFFER_TOO_SMALL) {
map_size += sizeof(struct efi_mem_desc); /* for my own */
- ret = efi_allocate_pool(EFI_LOADER_DATA, map_size,
+ ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, map_size,
(void *)&memmap);
if (ret != EFI_SUCCESS)
return CMD_RET_FAILURE;
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index a17b426d11..0c336f98d2 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -823,7 +823,7 @@ static void add_u_boot_and_runtime(void)
uboot_stack_size) & ~EFI_PAGE_MASK;
uboot_pages = ((uintptr_t)map_sysmem(gd->ram_top - 1, 0) -
uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
- efi_add_memory_map_pg(uboot_start, uboot_pages, EFI_LOADER_DATA,
+ efi_add_memory_map_pg(uboot_start, uboot_pages, EFI_BOOT_SERVICES_DATA,
false);
#if defined(__aarch64__)
@@ -857,7 +857,7 @@ int efi_memory_init(void)
/* Request a 32bit 64MB bounce buffer region */
uint64_t efi_bounce_buffer_addr = 0xffffffff;
- if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_LOADER_DATA,
+ if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_BOOT_SERVICES_DATA,
(64 * 1024 * 1024) >> EFI_PAGE_SHIFT,
&efi_bounce_buffer_addr) != EFI_SUCCESS)
return -1;
--
2.37.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] efi_loader: don't use EFI_LOADER_DATA internally
2022-11-29 15:38 ` Ilias Apalodimas
@ 2022-11-29 17:35 ` Heinrich Schuchardt
2022-11-30 6:51 ` Ilias Apalodimas
0 siblings, 1 reply; 5+ messages in thread
From: Heinrich Schuchardt @ 2022-11-29 17:35 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: François-Frédéric Ozog, u-boot
On 11/29/22 16:38, Ilias Apalodimas wrote:
> Hi Heinrich,
>
> On Tue, 29 Nov 2022 at 17:04, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> Memory allocated by U-Boot for internal usage should be
>> EFI_BOOT_SERVICES_DATA or _CODE or EFI_RUNTIME_SERVICES_DATA or _CODE.
>
> Agreed, EFI_LOADER_DATA should be for EFI apps.
>
>>
>> Reported-by: François-Frédéric Ozog <ff@ozog.com>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>
> [...]
>
>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> index a17b426d11..0c336f98d2 100644
>> --- a/lib/efi_loader/efi_memory.c
>> +++ b/lib/efi_loader/efi_memory.c
>> @@ -823,7 +823,7 @@ static void add_u_boot_and_runtime(void)
>> uboot_stack_size) & ~EFI_PAGE_MASK;
>> uboot_pages = ((uintptr_t)map_sysmem(gd->ram_top - 1, 0) -
>> uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
>> - efi_add_memory_map_pg(uboot_start, uboot_pages, EFI_LOADER_DATA,
>> + efi_add_memory_map_pg(uboot_start, uboot_pages, EFI_BOOT_SERVICES_DATA,
>> false);
>
> I am not sure if we should have this as _DATA or _CODE. None of these
> is an exact match of what we allocate here and both of these are
> backed by EFI_MEMORY_WB. So your reasoning here is prefer _DATA since
> it's not memory that holds boottime service drivers?
We are lacking a clear separation of data and code here. We would have
to add another pointer global data and enforce that data is in separate
pages if we wanted to do so.
The same problem exists when loading applications as some sections are
data and others are code but we put all into EFI_LOADER_CODE.
Please, tell if you would prefer EFI_BOOT_SERVICES_CODE here.
Best regards
Heinrich
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] efi_loader: don't use EFI_LOADER_DATA internally
2022-11-29 17:35 ` Heinrich Schuchardt
@ 2022-11-30 6:51 ` Ilias Apalodimas
0 siblings, 0 replies; 5+ messages in thread
From: Ilias Apalodimas @ 2022-11-30 6:51 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: François-Frédéric Ozog, u-boot
On Tue, Nov 29, 2022 at 06:35:40PM +0100, Heinrich Schuchardt wrote:
> On 11/29/22 16:38, Ilias Apalodimas wrote:
> > Hi Heinrich,
> >
> > On Tue, 29 Nov 2022 at 17:04, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> > >
> > > Memory allocated by U-Boot for internal usage should be
> > > EFI_BOOT_SERVICES_DATA or _CODE or EFI_RUNTIME_SERVICES_DATA or _CODE.
> >
> > Agreed, EFI_LOADER_DATA should be for EFI apps.
> >
> > >
> > > Reported-by: François-Frédéric Ozog <ff@ozog.com>
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >
> > [...]
> >
> > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > > index a17b426d11..0c336f98d2 100644
> > > --- a/lib/efi_loader/efi_memory.c
> > > +++ b/lib/efi_loader/efi_memory.c
> > > @@ -823,7 +823,7 @@ static void add_u_boot_and_runtime(void)
> > > uboot_stack_size) & ~EFI_PAGE_MASK;
> > > uboot_pages = ((uintptr_t)map_sysmem(gd->ram_top - 1, 0) -
> > > uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
> > > - efi_add_memory_map_pg(uboot_start, uboot_pages, EFI_LOADER_DATA,
> > > + efi_add_memory_map_pg(uboot_start, uboot_pages, EFI_BOOT_SERVICES_DATA,
> > > false);
> >
> > I am not sure if we should have this as _DATA or _CODE. None of these
> > is an exact match of what we allocate here and both of these are
> > backed by EFI_MEMORY_WB. So your reasoning here is prefer _DATA since
> > it's not memory that holds boottime service drivers?
>
> We are lacking a clear separation of data and code here. We would have to
> add another pointer global data and enforce that data is in separate pages
> if we wanted to do so.
>
> The same problem exists when loading applications as some sections are data
> and others are code but we put all into EFI_LOADER_CODE.
>
> Please, tell if you would prefer EFI_BOOT_SERVICES_CODE here.
I think I prefer _CODE, but I don't really mind tbh
With or without the changes. In case you update the patch can you add a
sentence along the lines of "EFI_LOADER_DATA/CODE is reserved for EFI
applications"
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>
> Best regards
>
> Heinrich
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-11-30 6:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-29 15:04 [PATCH 1/1] efi_loader: don't use EFI_LOADER_DATA internally Heinrich Schuchardt
2022-11-29 15:38 ` Ilias Apalodimas
2022-11-29 17:35 ` Heinrich Schuchardt
2022-11-30 6:51 ` Ilias Apalodimas
2022-11-29 15:41 ` François-Frédéric Ozog
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.