[PATCH] efi_loader: Don't allocate from Special purpose memory

[Sughosh Ganu]

On Tue, 17 Sept 2024 at 17:11, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Tue, 17 Sept 2024 at 13:05, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > On Tue, 17 Sept 2024 at 14:32, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > The EFI spec defines special-purpose memory in §7.2. That memory
> > > serves as a hint to the OS to avoid allocating this memory for core
> > > OS data or code that can not be relocated. So let's ignore it when
> > > allocating from conventional memory.
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > > ---
> >
> > Can we rebase this patch on top of the LMB/EFI series patches that are
> > currently under review. I believe if we consider the direction that
> > the EFI allocation functionality is going to take, that of relying on
> > LMB for allocating conventional memory, we will have to take a
> > different route to this problem, whereby 1) the LMB module needs to be
> > intimated that the region of memory is reserved so that it does not
> > get allocated by LMB and 2) then adding this region of memory as
> > special-purpose in the EFI memory map.
>
> So, the problem here is LMB (outside EFI) trying to allocate a piece
> of memory right?

Yes. Since there could be a request to the LMB module to allocate
memory, and the memory region that gets allocated could be the
special-purpose memory.

>  Because the EFI subsystem should do this check an
> exit before it allocates memory.
> I don't mind waiting, since I discovered the problem while working on
> a patch series for persistent memory. I can send it then.
> The EFI memory attributes can change at any point in time. So we will
> need to augment the notification to the LMB subsystem that marks that
> memory as reserved for LMB whenever memory attributes change.

Yes, we can see if the lmb_reserve_flags() API can be a good fit for
this purpose. But it will be good to base this on top of the EFI
series that I have posted. Thanks.

-sughosh

>
> Thanks
> /Ilias
>
>
> >
> > -sughosh
> >
> >
> > >  lib/efi_loader/efi_memory.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > > index c6f1dd09456e..74732e37f8aa 100644
> > > --- a/lib/efi_loader/efi_memory.c
> > > +++ b/lib/efi_loader/efi_memory.c
> > > @@ -422,7 +422,8 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated)
> > >
> > >                 if (addr >= start && addr < end) {
> > >                         if (must_be_allocated ^
> > > -                           (item->desc.type == EFI_CONVENTIONAL_MEMORY))
> > > +                           (item->desc.type == EFI_CONVENTIONAL_MEMORY) &&
> > > +                           !(item->desc.attribute & EFI_MEMORY_SP))
> > >                                 return EFI_SUCCESS;
> > >                         else
> > >                                 return EFI_NOT_FOUND;
> > > @@ -460,6 +461,9 @@ static uint64_t efi_find_free_memory(uint64_t len, uint64_t max_addr)
> > >                 if (desc->type != EFI_CONVENTIONAL_MEMORY)
> > >                         continue;
> > >
> > > +               if (desc->attribute & EFI_MEMORY_SP)
> > > +                       continue;
> > > +
> > >                 /* Out of bounds for max_addr */
> > >                 if ((ret + len) > max_addr)
> > >                         continue;
> > > --
> > > 2.45.2
> > >