Re: [PATCH 1/1] efi_loader: don't use EFI_LOADER_DATA internally

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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