On Sat, 14 Sept 2024 at 20:01, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 9/5/24 10:27, Sughosh Ganu wrote:

...

The LMB module is to be used as a backend for allocating and freeing up memory requested from other modules like EFI. These memory requests are different from the typical LMB reservations in that memory required by the EFI module cannot be overwritten, or re-requested. Add versions of the LMB API functions with flags for allocating and freeing up memory. The caller can then use these API's for specifying the type of memory that is required. For now, these functions will be used by the EFI memory module.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

include/lmb.h | 6 ++++++ lib/lmb.c | 39 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/include/lmb.h b/include/lmb.h index fc2daaa7bf..45a06c3b99 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -86,8 +86,13 @@ long lmb_reserve(phys_addr_t base, phys_size_t size); long lmb_reserve_flags(phys_addr_t base, phys_size_t size, enum lmb_flags flags); phys_addr_t lmb_alloc(phys_size_t size, ulong align); +phys_addr_t lmb_alloc_flags(phys_size_t size, ulong align, uint flags);

Please, provide Sphinx style function descriptions for the functions that you are adding or changing.

Will do. Thanks.

-sughosh

Cf. https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-do...

Best regards

Heinrich

...

phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr); +phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align,

•                          phys_addr_t max_addr, uint flags);
  

  phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size);

+phys_addr_t lmb_alloc_addr_flags(phys_addr_t base, phys_size_t size,

•                          uint flags);
  

  phys_size_t lmb_get_free_size(phys_addr_t addr);

  /**

@@ -103,6 +108,7 @@ phys_size_t lmb_get_free_size(phys_addr_t addr); int lmb_is_reserved_flags(phys_addr_t addr, int flags);

long lmb_free(phys_addr_t base, phys_size_t size); +long lmb_free_flags(phys_addr_t base, phys_size_t size, uint flags);

void lmb_dump_all(void); void lmb_dump_all_force(void); diff --git a/lib/lmb.c b/lib/lmb.c index 3ed570fb29..da6a1595cc 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -479,7 +479,7 @@ long lmb_add(phys_addr_t base, phys_size_t size) return lmb_add_region(lmb_rgn_lst, base, size); }

-long lmb_free(phys_addr_t base, phys_size_t size) +static long __lmb_free(phys_addr_t base, phys_size_t size) { struct lmb_region *rgn; struct alist *lmb_rgn_lst = &lmb.used_mem; @@ -530,6 +530,17 @@ long lmb_free(phys_addr_t base, phys_size_t size) rgn[i].flags); }

+long lmb_free(phys_addr_t base, phys_size_t size) +{

• return __lmb_free(base, size);
  

+}

+long lmb_free_flags(phys_addr_t base, phys_size_t size,

•             __always_unused uint flags)
  

+{

• return __lmb_free(base, size);
  

+}

• long lmb_reserve_flags(phys_addr_t base, phys_size_t size, enum lmb_flags flags) { struct alist *lmb_rgn_lst = &lmb.used_mem;

@@ -613,6 +624,12 @@ phys_addr_t lmb_alloc(phys_size_t size, ulong align) return lmb_alloc_base(size, align, LMB_ALLOC_ANYWHERE); }

+phys_addr_t lmb_alloc_flags(phys_size_t size, ulong align, uint flags) +{

• return __lmb_alloc_base(size, align, LMB_ALLOC_ANYWHERE,
  

•                         flags);
  

+}

• phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr) { phys_addr_t alloc;

@@ -626,6 +643,20 @@ phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr) return alloc; }

+phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align,

•                          phys_addr_t max_addr, uint flags)
  

+{

• phys_addr_t alloc;
  

• alloc = __lmb_alloc_base(size, align, max_addr, flags);
  

• if (alloc == 0)
  

•         printf("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n",
  

•                (ulong)size, (ulong)max_addr);
  

• return alloc;
  

+}

• static phys_addr_t __lmb_alloc_addr(phys_addr_t base, phys_size_t size, enum lmb_flags flags) {

@@ -660,6 +691,12 @@ phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size) return __lmb_alloc_addr(base, size, LMB_NONE); }

+phys_addr_t lmb_alloc_addr_flags(phys_addr_t base, phys_size_t size,

•                          uint flags)
  

+{

• return __lmb_alloc_addr(base, size, flags);
  

+}

• /* Return number of bytes from a given address that are free */ phys_size_t lmb_get_free_size(phys_addr_t addr) {

On 9/5/24 10:27, Sughosh Ganu wrote:

Add a flag LMB_NONOTIFY that can be passed to the LMB API's for reserving memory. This will then result in no notification being sent from the LMB module for the changes to the LMB's memory map.

You seem to be using this in patch 3 and 7.

Please, describe in this patch why you want to be able to suppress notification.

In the EFI context we should use LMB notification to notify the EFI_EVENT_GROUP_MEMORY_MAP_CHANGE event.

See chapter 7.1.2 EFI_BOOT_SERVICES.CreateEventEx() in the UEFI specification.

Best regards

Heinrich

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

include/lmb.h | 1 + lib/lmb.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/lmb.h b/include/lmb.h index 45a06c3b99..ffba7e2889 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -23,6 +23,7 @@ enum lmb_flags { LMB_NONE = 0, LMB_NOMAP = BIT(1), LMB_NOOVERWRITE = BIT(2),

• LMB_NONOTIFY = BIT(3), };

  /**

diff --git a/lib/lmb.c b/lib/lmb.c index da6a1595cc..419b31a651 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -30,7 +30,7 @@ static struct lmb lmb; static void lmb_print_region_flags(enum lmb_flags flags) { u64 bitpos;

• const char *flag_str[] = { "none", "no-map", "no-overwrite" };

• const char *flag_str[] = { "none", "no-map", "no-overwrite", "no-notify" };

  do { bitpos = flags ? fls(flags) - 1 : 0;

On 9/5/24 10:27, Sughosh Ganu wrote:

Use the LMB API's for allocating and freeing up memory. With this, the LMB module becomes the common backend for managing non U-Boot image memory that might be requested by other modules.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

lib/efi_loader/Kconfig | 1 + lib/efi_loader/efi_memory.c | 74 ++++++++++--------------------------- 2 files changed, 21 insertions(+), 54 deletions(-)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 6ffefa9103..911d4c6bbe 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -18,6 +18,7 @@ config EFI_LOADER select DM_EVENT select EVENT_DYNAMIC select LIB_UUID

• select LMB imply PARTITION_UUIDS select REGEX imply FAT

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index c6f1dd0945..90e07ed6a2 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -9,6 +9,7 @@

#include <efi_loader.h> #include <init.h> +#include <lmb.h> #include <log.h> #include <malloc.h> #include <mapmem.h> @@ -432,53 +433,6 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated) return EFI_NOT_FOUND; }

-/**

• • efi_find_free_memory() - find free memory pages
• • @len: size of memory area needed
• • @max_addr: highest address to allocate
• • Return: pointer to free memory area or 0
• */

-static uint64_t efi_find_free_memory(uint64_t len, uint64_t max_addr) -{

• struct efi_mem_list *lmem;
• /*

• * Prealign input max address, so we simplify our matching
  

• * logic below and can just reuse it as return pointer.
  

• */
  

• max_addr &= ~EFI_PAGE_MASK;
• list_for_each_entry(lmem, &efi_mem, link) {

• struct efi_mem_desc *desc = &lmem->desc;
  

• uint64_t desc_len = desc->num_pages << EFI_PAGE_SHIFT;
  

• uint64_t desc_end = desc->physical_start + desc_len;
  

• uint64_t curmax = min(max_addr, desc_end);
  

• uint64_t ret = curmax - len;
  

• /* We only take memory from free RAM */
  

• if (desc->type != EFI_CONVENTIONAL_MEMORY)
  

• 	continue;
  

• /* Out of bounds for max_addr */
  

• if ((ret + len) > max_addr)
  

• 	continue;
  

• /* Out of bounds for upper map limit */
  

• if ((ret + len) > desc_end)
  

• 	continue;
  

• /* Out of bounds for lower map limit */
  

• if (ret < desc->physical_start)
  

• 	continue;
  

• /* Return the highest address in this map within bounds */
  

• return ret;
  

• }
• return 0;

-}

• /**
  • efi_allocate_pages - allocate memory pages

@@ -493,6 +447,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, efi_uintn_t pages, uint64_t *memory) { u64 len;

• uint flags; efi_status_t ret; uint64_t addr;

@@ -508,33 +463,35 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, (len >> EFI_PAGE_SHIFT) != (u64)pages) return EFI_OUT_OF_RESOURCES;

• flags = LMB_NOOVERWRITE | LMB_NONOTIFY;

In the EFI context we should use LMB notification to notify the EFI_EVENT_GROUP_MEMORY_MAP_CHANGE event and update the memory map.

See chapter 7.1.2 EFI_BOOT_SERVICES.CreateEventEx() in the UEFI specification.

switch (type) { case EFI_ALLOCATE_ANY_PAGES: /* Any page */

• addr = efi_find_free_memory(len, -1ULL);
  

• addr = (u64)lmb_alloc_flags(len, EFI_PAGE_SIZE, flags);
  

On ARM64 we must ensure that "all 4KiB pages in the 64KiB page ... use identical ARM Memory Page Attributes". I cannot see how LMB implements this.

See chapter 2.3.6 AArch64 Platforms in the UEFI specification.

If you pass the EFI memory type to LMB and store it there, we can avoid duplication of the memory map and we can implement the 64 KiB pages logic in LMB.

Best regards

Heinrich

if (!addr)
	return EFI_OUT_OF_RESOURCES;
break;

case EFI_ALLOCATE_MAX_ADDRESS: /* Max address */

• addr = efi_find_free_memory(len, *memory);
  

• addr = (u64)lmb_alloc_base_flags(len, EFI_PAGE_SIZE, *memory,
  

• 				 flags);
  

  if (!addr) return EFI_OUT_OF_RESOURCES; break; case EFI_ALLOCATE_ADDRESS: if (*memory & EFI_PAGE_MASK) return EFI_NOT_FOUND;

• /* Exact address, reserve it. The addr is already in *memory. */
  

• ret = efi_check_allocated(*memory, false);
  

• if (ret != EFI_SUCCESS)
  

• 	return EFI_NOT_FOUND;
  

• addr = *memory;
  

• addr = (u64)lmb_alloc_addr_flags(*memory, len, flags);
  

• if (!addr)
  

• 	return EFI_OUT_OF_RESOURCES;
  

  break; default: /* UEFI doesn't specify other allocation types */ return EFI_INVALID_PARAMETER; }

• addr = (u64)(uintptr_t)map_sysmem(addr, 0); /* Reserve that map in our memory maps */ ret = efi_add_memory_map_pg(addr, pages, memory_type, true); if (ret != EFI_SUCCESS)

@@ -555,6 +512,9 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, */ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) {

• u64 len;

• uint flags;

• long status; efi_status_t ret;

  ret = efi_check_allocated(memory, true);

@@ -568,6 +528,12 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) return EFI_INVALID_PARAMETER; }

• flags = LMB_NOOVERWRITE | LMB_NONOTIFY;
• len = (u64)pages << EFI_PAGE_SHIFT;
• status = lmb_free_flags(memory, len, flags);
• if (status)

• return EFI_NOT_FOUND;
  

• ret = efi_add_memory_map_pg(memory, pages, EFI_CONVENTIONAL_MEMORY, false); if (ret != EFI_SUCCESS)

Hi Sughosh,

On Tue, 17 Sept 2024 at 15:09, Sughosh Ganu sughosh.ganu@linaro.org wrote:

On Sat, 14 Sept 2024 at 20:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 9/5/24 10:27, Sughosh Ganu wrote:

...

Use the LMB API's for allocating and freeing up memory. With this, the LMB module becomes the common backend for managing non U-Boot image memory that might be requested by other modules.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

lib/efi_loader/Kconfig | 1 + lib/efi_loader/efi_memory.c | 74 ++++++++++--------------------------- 2 files changed, 21 insertions(+), 54 deletions(-)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 6ffefa9103..911d4c6bbe 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -18,6 +18,7 @@ config EFI_LOADER select DM_EVENT select EVENT_DYNAMIC select LIB_UUID

• select LMB
  imply PARTITION_UUIDS
  select REGEX
  imply FAT
  

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index c6f1dd0945..90e07ed6a2 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -9,6 +9,7 @@

#include <efi_loader.h> #include <init.h> +#include <lmb.h> #include <log.h> #include <malloc.h> #include <mapmem.h> @@ -432,53 +433,6 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated) return EFI_NOT_FOUND; }

-/**

• • efi_find_free_memory() - find free memory pages
• • @len: size of memory area needed
• • @max_addr: highest address to allocate
• • Return: pointer to free memory area or 0
• */

-static uint64_t efi_find_free_memory(uint64_t len, uint64_t max_addr) -{

• struct efi_mem_list *lmem;
  

• /*
  

•  * Prealign input max address, so we simplify our matching
  

•  * logic below and can just reuse it as return pointer.
  

•  */
  

• max_addr &= ~EFI_PAGE_MASK;
  

• list_for_each_entry(lmem, &efi_mem, link) {
  

•         struct efi_mem_desc *desc = &lmem->desc;
  

•         uint64_t desc_len = desc->num_pages << EFI_PAGE_SHIFT;
  

•         uint64_t desc_end = desc->physical_start + desc_len;
  

•         uint64_t curmax = min(max_addr, desc_end);
  

•         uint64_t ret = curmax - len;
  

•         /* We only take memory from free RAM */
  

•         if (desc->type != EFI_CONVENTIONAL_MEMORY)
  

•                 continue;
  

•         /* Out of bounds for max_addr */
  

•         if ((ret + len) > max_addr)
  

•                 continue;
  

•         /* Out of bounds for upper map limit */
  

•         if ((ret + len) > desc_end)
  

•                 continue;
  

•         /* Out of bounds for lower map limit */
  

•         if (ret < desc->physical_start)
  

•                 continue;
  

•         /* Return the highest address in this map within bounds */
  

•         return ret;
  

• }
  

• return 0;
  

-}

• /**
  • efi_allocate_pages - allocate memory pages

@@ -493,6 +447,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, efi_uintn_t pages, uint64_t *memory) { u64 len;

• uint flags;
  efi_status_t ret;
  uint64_t addr;
  
  

@@ -508,33 +463,35 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, (len >> EFI_PAGE_SHIFT) != (u64)pages) return EFI_OUT_OF_RESOURCES;

• flags = LMB_NOOVERWRITE | LMB_NONOTIFY;
  

In the EFI context we should use LMB notification to notify the EFI_EVENT_GROUP_MEMORY_MAP_CHANGE event and update the memory map.

Not sure if I am understanding your comment right, but what is happening here is that the EFI subsystem is requesting LMB for memory. And the reason why we have a flag LMB_NONOTIFY, is to prevent a circular chain of LMB then notifying EFI of a change to it's memory map.

...

See chapter 7.1.2 EFI_BOOT_SERVICES.CreateEventEx() in the UEFI specification.

...

  switch (type) {
  case EFI_ALLOCATE_ANY_PAGES:
          /* Any page */

•         addr = efi_find_free_memory(len, -1ULL);
  

•         addr = (u64)lmb_alloc_flags(len, EFI_PAGE_SIZE, flags);
  

On ARM64 we must ensure that "all 4KiB pages in the 64KiB page ... use identical ARM Memory Page Attributes". I cannot see how LMB implements this.

If you are referring to arm memory attributes, this is not being handled by the current EFI memory code as well. If this is to be handled, that needs to be taken up as a separate task -- I am not sure if U-Boot even has support in the form of any API's where memory regions can be configured for different memory attributes.

So, we *do* handle some EFI memory attributes currently, in efi_add_memory_map_pg(), but we don't translate those to Arm memory attributes. The latter should indeed be a task, but I think what Henrich is saying here is orthogonal. Regardless of what happens to the mmu in the future the spec says "a 64KiB physical page contains any 4KiB page with any of the following types listed below, then all 4KiB pages in the 64KiB page must use identical ARM Memory Page Attribute". So for runtime services/code, EfiACPIMemoryNVS and reserved memory we should group similar attributes in 64k boundaries. I think this is something worth planning ahead, instead of plugging holes afterwards

Cheers /Ilias

...

See chapter 2.3.6 AArch64 Platforms in the UEFI specification.

If you pass the EFI memory type to LMB and store it there, we can avoid duplication of the memory map and we can implement the 64 KiB pages logic in LMB.

Okay, that would mean mixing up EFI code in the LMB module. Again, not sure if this comes under the purview of the goal of this series, which is to ensure that EFI memory does not trample over that allocated by LMB. Also, thinking a bit more on this, if we are to address aspects like memory attributes etc, I feel that the earlier design that I had posted of notification between LMB and EFI to have a synchronous view of each other's memory would be a better approach. That way, the EFI memory module can then implement things that it needs to, from the EFI spec point of view, and the LMB module can then just update the EFI module of the memory that it is using.

-sughosh

...

Best regards

Heinrich

...

          if (!addr)
                  return EFI_OUT_OF_RESOURCES;
          break;
  case EFI_ALLOCATE_MAX_ADDRESS:
          /* Max address */

•         addr = efi_find_free_memory(len, *memory);
  

•         addr = (u64)lmb_alloc_base_flags(len, EFI_PAGE_SIZE, *memory,
  

•                                          flags);
          if (!addr)
                  return EFI_OUT_OF_RESOURCES;
          break;
  case EFI_ALLOCATE_ADDRESS:
          if (*memory & EFI_PAGE_MASK)
                  return EFI_NOT_FOUND;
  

•         /* Exact address, reserve it. The addr is already in *memory. */
  

•         ret = efi_check_allocated(*memory, false);
  

•         if (ret != EFI_SUCCESS)
  

•                 return EFI_NOT_FOUND;
  

•         addr = *memory;
  

•         addr = (u64)lmb_alloc_addr_flags(*memory, len, flags);
  

•         if (!addr)
  

•                 return EFI_OUT_OF_RESOURCES;
          break;
  default:
          /* UEFI doesn't specify other allocation types */
          return EFI_INVALID_PARAMETER;
  }
  
  

• addr = (u64)(uintptr_t)map_sysmem(addr, 0);
  /* Reserve that map in our memory maps */
  ret = efi_add_memory_map_pg(addr, pages, memory_type, true);
  if (ret != EFI_SUCCESS)
  

@@ -555,6 +512,9 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, */ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) {

• u64 len;
  

• uint flags;
  

• long status;
  efi_status_t ret;
  
  ret = efi_check_allocated(memory, true);
  

@@ -568,6 +528,12 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) return EFI_INVALID_PARAMETER; }

• flags = LMB_NOOVERWRITE | LMB_NONOTIFY;
  

• len = (u64)pages << EFI_PAGE_SHIFT;
  

• status = lmb_free_flags(memory, len, flags);
  

• if (status)
  

•         return EFI_NOT_FOUND;
  

• ret = efi_add_memory_map_pg(memory, pages, EFI_CONVENTIONAL_MEMORY,
                              false);
  if (ret != EFI_SUCCESS)
  

On 9/5/24 10:27, Sughosh Ganu wrote:

Add an event which would be used for notifying changes in the LMB modules' memory map. This is to be used for having a synchronous view of the memory that is currently in use, and that is available for allocations.

The synchronous view problem only exists because we are duplicating data. Store the EFI memory type in LMB and the problem vanishes.

The event is only needed to notify EFI_EVENT_GROUP_MEMORY_MAP_CHANGE.

Best regards

Heinrich

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

common/event.c | 2 ++ include/event.h | 14 ++++++++++++++ 2 files changed, 16 insertions(+)

diff --git a/common/event.c b/common/event.c index dda569d447..fc8002603c 100644 --- a/common/event.c +++ b/common/event.c @@ -48,6 +48,8 @@ const char *const type_name[] = {

/* main loop events */ "main_loop",

• "lmb_map_update", };

  _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");

diff --git a/include/event.h b/include/event.h index fb353ad623..fce7e96170 100644 --- a/include/event.h +++ b/include/event.h @@ -153,6 +153,14 @@ enum event_t { */ EVT_MAIN_LOOP,

• /**

• * @EVT_LMB_MAP_UPDATE:
  

• * This event is triggered on an update to the LMB reserved memory
  

• * region. This can be used to notify about any LMB memory allocation
  

• * or freeing of memory having occurred.
  

• */
  

• EVT_LMB_MAP_UPDATE,
• /**
  • @EVT_COUNT:
  • This constants holds the maximum event number + 1 and is used when

@@ -203,6 +211,12 @@ union event_data { oftree tree; struct bootm_headers *images; } ft_fixup;

• struct event_lmb_map_update {

• u64 base;
  

• u64 size;
  

• u8 op;
  

• } lmb_map; };

  /**

Hi Sughosh,

On Tue, 17 Sept 2024 at 15:33, Sughosh Ganu sughosh.ganu@linaro.org wrote:

On Sat, 14 Sept 2024 at 20:38, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 9/5/24 10:27, Sughosh Ganu wrote:

...

Add an event which would be used for notifying changes in the LMB modules' memory map. This is to be used for having a synchronous view of the memory that is currently in use, and that is available for allocations.

The synchronous view problem only exists because we are duplicating data. Store the EFI memory type in LMB and the problem vanishes.

The LMB module only concerns itself with RAM memory. If I understand correctly, you are proposing maintaining the EFI memory map within the LMB module ? That would mean handling memory types other than conventional memory in LMB.

I am pretty sure I've asked this before, but do these *always* need to be in sync?

The efi allocators will call LMB now. So when we allocate something gtom EFI, even if any potential changes from LMB haven't been published to EFI, we won't have any memory corruptions. Can't we just opportunistically update the memory map once someone requests it?

Thanks /Ilias

-sughosh

...

The event is only needed to notify EFI_EVENT_GROUP_MEMORY_MAP_CHANGE.

Best regards

Heinrich

...

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

common/event.c | 2 ++ include/event.h | 14 ++++++++++++++ 2 files changed, 16 insertions(+)

diff --git a/common/event.c b/common/event.c index dda569d447..fc8002603c 100644 --- a/common/event.c +++ b/common/event.c @@ -48,6 +48,8 @@ const char *const type_name[] = {

  /* main loop events */
  "main_loop",

• "lmb_map_update",
  

  };

  _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");

diff --git a/include/event.h b/include/event.h index fb353ad623..fce7e96170 100644 --- a/include/event.h +++ b/include/event.h @@ -153,6 +153,14 @@ enum event_t { */ EVT_MAIN_LOOP,

• /**
  

•  * @EVT_LMB_MAP_UPDATE:
  

•  * This event is triggered on an update to the LMB reserved memory
  

•  * region. This can be used to notify about any LMB memory allocation
  

•  * or freeing of memory having occurred.
  

•  */
  

• EVT_LMB_MAP_UPDATE,
  

• /**
   * @EVT_COUNT:
   * This constants holds the maximum event number + 1 and is used when
  

@@ -203,6 +211,12 @@ union event_data { oftree tree; struct bootm_headers *images; } ft_fixup;

• struct event_lmb_map_update {
  

•         u64 base;
  

•         u64 size;
  

•         u8 op;
  

• } lmb_map;
  

  };

  /**

Hi Sughosh,

On Fri, 20 Sept 2024 at 13:38, Sughosh Ganu sughosh.ganu@linaro.org wrote:

On Fri, 20 Sept 2024 at 14:51, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

...

Hi Sughosh,

On Tue, 17 Sept 2024 at 15:33, Sughosh Ganu sughosh.ganu@linaro.org wrote:

...

On Sat, 14 Sept 2024 at 20:38, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 9/5/24 10:27, Sughosh Ganu wrote:

...

Add an event which would be used for notifying changes in the LMB modules' memory map. This is to be used for having a synchronous view of the memory that is currently in use, and that is available for allocations.

The synchronous view problem only exists because we are duplicating data. Store the EFI memory type in LMB and the problem vanishes.

The LMB module only concerns itself with RAM memory. If I understand correctly, you are proposing maintaining the EFI memory map within the LMB module ? That would mean handling memory types other than conventional memory in LMB.

I am pretty sure I've asked this before, but do these *always* need to be in sync?

The efi allocators will call LMB now. So when we allocate something gtom EFI, even if any potential changes from LMB haven't been published to EFI, we won't have any memory corruptions. Can't we just opportunistically update the memory map once someone requests it?

I have given this a thought. Because what you mention, Simon has a similar comment. But to achieve this, it would require generating a new efi memory map afresh every time such a requirement comes up. This would mean, create a new memory map, put in the conventional memory, and add other memory types that were part of the existing memory map. And then remove the older memory map. This would need to be done every time a memory map is needed to be generated. And that would also include instances when a user enters a command to get the current memory map. I think notifying any changes to the lmb memory map to the efi memory module is easier, and less error prone.

We need to get some agreement on my memory-allocation patches first. I don't believe we are on the same page on those, despite some weeks of discussion. We need to resolve that issue first. I did try right from the start to first agree on the problem to be solved. We skipped that, so now we are having to do it now...

Regards, Simon

-sughosh

...

Thanks /Ilias

...

-sughosh

...

The event is only needed to notify EFI_EVENT_GROUP_MEMORY_MAP_CHANGE.

Best regards

Heinrich

...

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

common/event.c | 2 ++ include/event.h | 14 ++++++++++++++ 2 files changed, 16 insertions(+)

diff --git a/common/event.c b/common/event.c index dda569d447..fc8002603c 100644 --- a/common/event.c +++ b/common/event.c @@ -48,6 +48,8 @@ const char *const type_name[] = {

  /* main loop events */
  "main_loop",

• "lmb_map_update",
  

  };

  _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");

diff --git a/include/event.h b/include/event.h index fb353ad623..fce7e96170 100644 --- a/include/event.h +++ b/include/event.h @@ -153,6 +153,14 @@ enum event_t { */ EVT_MAIN_LOOP,

• /**
  

•  * @EVT_LMB_MAP_UPDATE:
  

•  * This event is triggered on an update to the LMB reserved memory
  

•  * region. This can be used to notify about any LMB memory allocation
  

•  * or freeing of memory having occurred.
  

•  */
  

• EVT_LMB_MAP_UPDATE,
  

• /**
   * @EVT_COUNT:
   * This constants holds the maximum event number + 1 and is used when
  

@@ -203,6 +211,12 @@ union event_data { oftree tree; struct bootm_headers *images; } ft_fixup;

• struct event_lmb_map_update {
  

•         u64 base;
  

•         u64 size;
  

•         u8 op;
  

• } lmb_map;
  

  };

  /**

On 26.09.24 14:58, Ilias Apalodimas wrote:

Hi Heinrich,

On Sat, 14 Sept 2024 at 18:08, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 9/5/24 10:27, Sughosh Ganu wrote:

...

Add an event which would be used for notifying changes in the LMB modules' memory map. This is to be used for having a synchronous view of the memory that is currently in use, and that is available for allocations.

The synchronous view problem only exists because we are duplicating data. Store the EFI memory type in LMB and the problem vanishes.

We could, but I wonder if that would be enough. Apart from the EFI memory types, we plan on fixing the EFI memory attributes. I'll go read the spec again and take look at EDK2 but IIRC, you can't correlate memory types to memory attributes (at least not for all of them).

If that's true we'll also need the attributes in LMB + perhaps logic for 64kb page size OSes and 4KB page firmware, which might be a bit too much. In that case, I think the sync is fine, but I really don't see the point of making an event out of it. Perhaps we could just update the efi memory map directly from LMB.

Yes, memory attributes should be stored in LMB too if UEFI is enabled.

I would suggest:

* GetMemoryMap() calls into LMB to generate the map on the fly. * The EFI_MEMORY_ATTRIBUTES_TABLE is generated from LMB information. * The EFI_MEMORY_ATTRIBUTE_PROTOCOL calls into LMB.

Best regards

Heinrich

Thanks /Ilias

...

The event is only needed to notify EFI_EVENT_GROUP_MEMORY_MAP_CHANGE.

Best regards

Heinrich

...

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

common/event.c | 2 ++ include/event.h | 14 ++++++++++++++ 2 files changed, 16 insertions(+)

diff --git a/common/event.c b/common/event.c index dda569d447..fc8002603c 100644 --- a/common/event.c +++ b/common/event.c @@ -48,6 +48,8 @@ const char *const type_name[] = {

   /* main loop events */
   "main_loop",

• "lmb_map_update",
  

  };

  _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");

diff --git a/include/event.h b/include/event.h index fb353ad623..fce7e96170 100644 --- a/include/event.h +++ b/include/event.h @@ -153,6 +153,14 @@ enum event_t { */ EVT_MAIN_LOOP,

• /**
  

•  * @EVT_LMB_MAP_UPDATE:
  

•  * This event is triggered on an update to the LMB reserved memory
  

•  * region. This can be used to notify about any LMB memory allocation
  

•  * or freeing of memory having occurred.
  

•  */
  

• EVT_LMB_MAP_UPDATE,
  

•  /**
    * @EVT_COUNT:
    * This constants holds the maximum event number + 1 and is used when
  

@@ -203,6 +211,12 @@ union event_data { oftree tree; struct bootm_headers *images; } ft_fixup;

• struct event_lmb_map_update {
  

•         u64 base;
  

•         u64 size;
  

•         u8 op;
  

• } lmb_map;
  

  };

  /**

On 26.09.24 15:54, Ilias Apalodimas wrote:

Hi Heinrich,

On Thu, 26 Sept 2024 at 16:19, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 26.09.24 14:58, Ilias Apalodimas wrote:

...

Hi Heinrich,

On Sat, 14 Sept 2024 at 18:08, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 9/5/24 10:27, Sughosh Ganu wrote:

...

Add an event which would be used for notifying changes in the LMB modules' memory map. This is to be used for having a synchronous view of the memory that is currently in use, and that is available for allocations.

The synchronous view problem only exists because we are duplicating data. Store the EFI memory type in LMB and the problem vanishes.

We could, but I wonder if that would be enough. Apart from the EFI memory types, we plan on fixing the EFI memory attributes. I'll go read the spec again and take look at EDK2 but IIRC, you can't correlate memory types to memory attributes (at least not for all of them).

If that's true we'll also need the attributes in LMB + perhaps logic for 64kb page size OSes and 4KB page firmware, which might be a bit too much. In that case, I think the sync is fine, but I really don't see the point of making an event out of it. Perhaps we could just update the efi memory map directly from LMB.

Yes, memory attributes should be stored in LMB too if UEFI is enabled.

I would suggest:

• GetMemoryMap() calls into LMB to generate the map on the fly.
• The EFI_MEMORY_ATTRIBUTES_TABLE is generated from LMB information.
• The EFI_MEMORY_ATTRIBUTE_PROTOCOL calls into LMB.

Best regards

Heinrich

That would work. TBH I do have a few doubts on how clean this is going to be. It seems a bit cleaner to me atm to keep the efi memory map as is. Especially if we get rid of the event. Would it make sense to clean this up from the event and merge it close to its current form? Then we could prototype what you suggest and send an RFC to decide it it looks cleaner or not.

Sure, we can split the work into two series.

Best regards

Heinrich

...

...

...

The event is only needed to notify EFI_EVENT_GROUP_MEMORY_MAP_CHANGE.

Best regards

Heinrich

...

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

common/event.c  |  2 ++
include/event.h | 14 ++++++++++++++
2 files changed, 16 insertions(+)

diff --git a/common/event.c b/common/event.c index dda569d447..fc8002603c 100644 --- a/common/event.c +++ b/common/event.c @@ -48,6 +48,8 @@ const char *const type_name[] = {

    /* main loop events */
    "main_loop",

• "lmb_map_update",
  

  };

  _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");

diff --git a/include/event.h b/include/event.h index fb353ad623..fce7e96170 100644 --- a/include/event.h +++ b/include/event.h @@ -153,6 +153,14 @@ enum event_t { */ EVT_MAIN_LOOP,

• /**
  

•  * @EVT_LMB_MAP_UPDATE:
  

•  * This event is triggered on an update to the LMB reserved memory
  

•  * region. This can be used to notify about any LMB memory allocation
  

•  * or freeing of memory having occurred.
  

•  */
  

• EVT_LMB_MAP_UPDATE,
  

•   /**
     * @EVT_COUNT:
     * This constants holds the maximum event number + 1 and is used when
  

@@ -203,6 +211,12 @@ union event_data { oftree tree; struct bootm_headers *images; } ft_fixup;

• struct event_lmb_map_update {
  

•         u64 base;
  

•         u64 size;
  

•         u8 op;
  

• } lmb_map;
  

  };

  /**

On 9/5/24 10:28, Sughosh Ganu wrote:

Add a Kconfig symbol to enable getting updates on any memory map changes that might be done by the LMB module. This notification mechanism can then be used to have a synchronous view of allocated and free memory.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

lib/Kconfig | 11 +++++++++++ 1 file changed, 11 insertions(+)

diff --git a/lib/Kconfig b/lib/Kconfig index 5f282ecb54..2e73cda580 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -74,6 +74,17 @@ config HAVE_PRIVATE_LIBGCC config LIB_UUID bool

+config MEM_MAP_UPDATE_NOTIFY

• bool "Get notified of any changes to the LMB memory map"
• depends on LMB && EFI_LOADER
• select EVENT
• default y
• help

•  Enable this option to get notification on any changes to the
  

•  memory that is allocated or freed by the LMB module. This will
  

•  allow different modules that allocate memory or maintain a memory
  

•  map to have a synchronous view of available and allocated memory.
  

We should have only one store for all LMB allocations. They should not be duplicated in the EFI sub-system.

The event is only needed to notify EFI_EVENT_GROUP_MEMORY_MAP_CHANGE.

Best regards

Heinrich

• config RANDOM_UUID bool "GPT Random UUID generation" select LIB_UUID