| Message ID | 20240917090218.77246-1-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | efi_loader: Don't allocate from Special purpose memory | expand |
On Tue, 17 Sept 2024 at 14:32, Ilias Apalodimas <ilias.apalodimas@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@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. -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 >
On Tue, 17 Sept 2024 at 13:05, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Tue, 17 Sept 2024 at 14:32, Ilias Apalodimas > <ilias.apalodimas@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@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? 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. 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 > >
On Tue, 17 Sept 2024 at 17:11, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Tue, 17 Sept 2024 at 13:05, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Tue, 17 Sept 2024 at 14:32, Ilias Apalodimas > > <ilias.apalodimas@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@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 > > >
On 17.09.24 11:02, Ilias Apalodimas 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@linaro.org> > --- > 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) Now that Sushosh's LMB patches are merged this function does not exist anymore. I think we should next move the EFI memory map to LMB to avoid double book keeping. Best regards Heinrich > 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;
Hi Heinrich, On Wed, 16 Oct 2024 at 19:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 17.09.24 11:02, Ilias Apalodimas 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@linaro.org> > > --- > > 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) > > Now that Sushosh's LMB patches are merged this function does not exist > anymore. > > I think we should next move the EFI memory map to LMB to avoid double > book keeping. > Yea, I sent that patch before LMB as a fix, but I agreed to wait till the LMB patches get merged. Now that they are in we'll rework it. Thanks /Ilias > Best regards > > Heinrich > > > 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; >
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;
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@linaro.org> --- lib/efi_loader/efi_memory.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)