On Fri, 11 Oct 2024 at 15:48, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

Hi Sughosh

On Tue, 8 Oct 2024 at 21:14, Sughosh Ganu sughosh.ganu@linaro.org 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.

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

Changes since V1: None

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 0e8426f4379..a76262d520d 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),
  

I haven't gone through the rest of the series yet, but if if we remove the notification event why we would we want the flag? The LMB will directly update the mappings no?

We are no longer using the event framework for notifying the EFI memory module of the changes to the LMB memory map, but the notification still ends up happening. The efi_add_memory_map_pg() does get called from lmb_map_update_notify().

-sughosh

Thanks /Ilias

...

};

/** diff --git a/lib/lmb.c b/lib/lmb.c index 0763f6174c1..a4886db4c45 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;
  

-- 2.34.1

Hi Sughosh,

On Tue, 8 Oct 2024 at 12:14, Sughosh Ganu sughosh.ganu@linaro.org 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

Changes since V1: None

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

Please check my recent series [1] which shows how to avoid needing to worry about lmb until the app starts, at which point I believe we can just add the lmb map to the EFI map so we know where the images are?

But without my series, this patch is needed. I do wonder what addresses lmb happens to allocate with this? From the top? From the bottom?

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index e58b8825605..5a576720606 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -20,6 +20,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 c6f1dd09456..90e07ed6a2d 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;
    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);
            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)
  

-- 2.34.1

Regards, SImon

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=427729

Hi Sughosh

On Tue, 8 Oct 2024 at 21:15, Sughosh Ganu sughosh.ganu@linaro.org 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.

Are there size concerns to enable or disable this? I think we should always sync and get rid of this

Thanks /Ilias

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

Changes since V1:

• Add a config for SPL stage

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

diff --git a/lib/Kconfig b/lib/Kconfig index 1dd4f271595..61452f7ac94 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -75,6 +75,23 @@ config LIB_UUID bool select SHA1

+config MEM_MAP_UPDATE_NOTIFY

•   bool "Get notified of any changes to the LMB memory map"
  

•   default y if EFI_LOADER
  

•   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.
  

+config SPL_MEM_MAP_UPDATE_NOTIFY

•   bool "Get notified of any changes to the LMB memory map in SPL"
  

•   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.
  

config RANDOM_UUID bool "GPT Random UUID generation" select LIB_UUID -- 2.34.1

On Sat, 12 Oct 2024 at 13:10, Sughosh Ganu sughosh.ganu@linaro.org wrote:

On Fri, 11 Oct 2024 at 15:56, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

...

Hi Sughosh

On Tue, 8 Oct 2024 at 21:15, Sughosh Ganu sughosh.ganu@linaro.org 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.

Are there size concerns to enable or disable this? I think we should always sync and get rid of this

This is needed only for platforms that enable EFI_LOADER. I haven't checked if not having this check would result in any kind of build failures and/or size impact on platforms that do not enable EFI_LOADER. I will check this, and keep it if any build/size impact happens.

Not using the config symbol causes build failure on boards which do not have EFI_LOADER enabled. So we need some kind of a check to call the notification function lmb_map_update_notify(). If not this symbol, there would have to be a check for EFI_LOADER, but I don't think having checks for EFI_LOADER in lmb.c is a clean solution. Thanks.

-sughosh

-sughosh

...

Thanks /Ilias

...

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

Changes since V1:

• Add a config for SPL stage

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

diff --git a/lib/Kconfig b/lib/Kconfig index 1dd4f271595..61452f7ac94 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -75,6 +75,23 @@ config LIB_UUID bool select SHA1

+config MEM_MAP_UPDATE_NOTIFY

•   bool "Get notified of any changes to the LMB memory map"
  

•   default y if EFI_LOADER
  

•   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.
  

+config SPL_MEM_MAP_UPDATE_NOTIFY

•   bool "Get notified of any changes to the LMB memory map in SPL"
  

•   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.
  

config RANDOM_UUID bool "GPT Random UUID generation" select LIB_UUID -- 2.34.1

On Tue, 8 Oct 2024 at 21:15, Sughosh Ganu sughosh.ganu@linaro.org wrote:

The efi_add_known_memory() function for the TI K3 platforms is adding the EFI_CONVENTIONAL_MEMORY type. This memory is now being handled through the LMB module -- the lmb_add_memory() adds this memory to the memory map. Remove the definition of the now superfluous efi_add_known_memory() function.

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

Changes since V1: None

arch/arm/mach-k3/common.c | 11 ----------- 1 file changed, 11 deletions(-)

diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c index df48ec8d479..f2086cbdf51 100644 --- a/arch/arm/mach-k3/common.c +++ b/arch/arm/mach-k3/common.c @@ -310,14 +310,3 @@ void setup_qos(void) writel(qos_data[i].val, (uintptr_t)qos_data[i].reg); } #endif

-void efi_add_known_memory(void) -{

•   if (IS_ENABLED(CONFIG_EFI_LOADER))
  

•           /*
  

•            * Memory over ram_top can be used by various firmware
  

•            * Declare to EFI only memory area below ram_top
  

•            */
  

•           efi_add_memory_map(gd->ram_base, gd->ram_top - gd->ram_base,
  

•                              EFI_CONVENTIONAL_MEMORY);
  

-}

2.34.1

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

On Tue, 8 Oct 2024 at 21:15, Sughosh Ganu sughosh.ganu@linaro.org wrote:

The efi_add_known_memory() function for the stm32mp platforms is adding the EFI_CONVENTIONAL_MEMORY type. This memory is now being handled through the LMB module -- the lmb_add_memory() adds this memory to the memory map. Remove the definition of the now superfluous efi_add_known_memory() function.

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

Changes since V1: None

arch/arm/mach-stm32mp/dram_init.c | 11 ----------- 1 file changed, 11 deletions(-)

diff --git a/arch/arm/mach-stm32mp/dram_init.c b/arch/arm/mach-stm32mp/dram_init.c index 198785353f1..3698fc49bf1 100644 --- a/arch/arm/mach-stm32mp/dram_init.c +++ b/arch/arm/mach-stm32mp/dram_init.c @@ -86,14 +86,3 @@ phys_addr_t board_get_usable_ram_top(phys_size_t total_size)

    return reg + size;

}

-void efi_add_known_memory(void) -{

•   if (IS_ENABLED(CONFIG_EFI_LOADER))
  

•           /*
  

•            * Memory over ram_top is reserved to OPTEE.
  

•            * Declare to EFI only memory area below ram_top
  

•            */
  

•           efi_add_memory_map(gd->ram_base, gd->ram_top - gd->ram_base,
  

•                              EFI_CONVENTIONAL_MEMORY);
  

-}

2.34.1

TBH we should make efi_add_known_memory() and remove __weak in the future, unless someone has a very good reason to keep it.

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

On Fri, 11 Oct 2024 at 16:33, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

Hi Sughosh,

On Tue, 8 Oct 2024 at 21:15, Sughosh Ganu sughosh.ganu@linaro.org wrote:

...

The EFI_CONVENTIONAL_MEMORY type, which is the usable RAM memory is now being managed by the LMB module. Remove the addition of this memory type to the EFI memory map. This memory now gets added to the EFI memory map as part of the LMB memory map update event handler.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Simon Glass sjg@chromium.org

---

Changes since V1: None

include/efi_loader.h | 13 ++++--- lib/efi_loader/efi_memory.c | 75 +++---------------------------------- 2 files changed, 12 insertions(+), 76 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h index 081cb65d3f7..0d5555c7597 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -798,10 +798,6 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, int memory_type, bool overlap_only_ram);

-/* Adds a conventional range into the EFI memory map */ -efi_status_t efi_add_conventional_memory_map(u64 ram_start, u64 ram_end,

•                                        u64 ram_top);
  

/* Called by board init to initialize the EFI drivers */ efi_status_t efi_driver_init(void); /* Called when a block device is added */ @@ -1186,9 +1182,14 @@ efi_status_t efi_console_get_u16_string efi_status_t efi_disk_get_device_name(const efi_handle_t handle, char *buf, int size);

/**

• • efi_add_known_memory() - add memory banks to EFI memory map

• • efi_add_known_memory() - add memory types to the EFI memory map
• • This function is to be used to adding different memory types other

/adding/add

Okay

...

• • than EFI_CONVENTIONAL_MEMORY to the EFI memory map. The conventional
• • memory is handled by the LMB module, and gets added to the memory

no comma after module

Okay

...

• • map through the LMB module.

• • This weak function may be overridden for specific architectures.

• • This function may be overridden for specific architectures.

Since you are rewriting this I think for "architecture specific purposes" is better

Okay

...

*/ void efi_add_known_memory(void);

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 974ecc51113..0f25094dbf8 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -780,82 +780,17 @@ efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size, }

/**

• • efi_add_conventional_memory_map() - add a RAM memory area to the map

• • efi_add_known_memory() - add memory types to the EFI memory map

• • @ram_start: start address of a RAM memory area
• • @ram_end: end address of a RAM memory area
• • @ram_top: max address to be used as conventional memory
• • Return: status code
• */

-efi_status_t efi_add_conventional_memory_map(u64 ram_start, u64 ram_end,

•                                        u64 ram_top)
  

-{

•   u64 pages;
  

•   /* Remove partial pages */
  

•   ram_end &= ~EFI_PAGE_MASK;
  

•   ram_start = (ram_start + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
  

•   if (ram_end <= ram_start) {
  

•           /* Invalid mapping */
  

•           return EFI_INVALID_PARAMETER;
  

•   }
  

•   pages = (ram_end - ram_start) >> EFI_PAGE_SHIFT;
  

•   efi_add_memory_map_pg(ram_start, pages,
  

•                         EFI_CONVENTIONAL_MEMORY, false);
  

•   /*
  

•    * Boards may indicate to the U-Boot memory core that they
  

•    * can not support memory above ram_top. Let's honor this
  

•    * in the efi_loader subsystem too by declaring any memory
  

•    * above ram_top as "already occupied by firmware".
  

•    */
  

•   if (ram_top < ram_start) {
  

•           /* ram_top is before this region, reserve all */
  

•           efi_add_memory_map_pg(ram_start, pages,
  

•                                 EFI_BOOT_SERVICES_DATA, true);
  

•   } else if (ram_top < ram_end) {
  

•           /* ram_top is inside this region, reserve parts */
  

•           pages = (ram_end - ram_top) >> EFI_PAGE_SHIFT;
  

•           efi_add_memory_map_pg(ram_top, pages,
  

•                                 EFI_BOOT_SERVICES_DATA, true);
  

•   }
  

•   return EFI_SUCCESS;
  

-}

-/**

• • efi_add_known_memory() - add memory banks to map

• • This function is to be used to adding different memory types other
• • than EFI_CONVENTIONAL_MEMORY to the EFI memory map. The conventional
• • memory is handled by the LMB module, and gets added to the memory
• • map through the LMB module.
  • This function may be overridden for specific architectures.
  */

__weak void efi_add_known_memory(void)

We can also get rid of this in the future. Any idea if we have platforms using it after the last 2 you removed?

Yes, this is being used by some x86 platform, which is adding other memory types to the EFI memory map.

-sughosh

...

{

•   u64 ram_top = gd->ram_top & ~EFI_PAGE_MASK;
  

•   int i;
  

•   /*
  

•    * ram_top is just outside mapped memory. So use an offset of one for
  

•    * mapping the sandbox address.
  

•    */
  

•   ram_top = (uintptr_t)map_sysmem(ram_top - 1, 0) + 1;
  

•   /* Fix for 32bit targets with ram_top at 4G */
  

•   if (!ram_top)
  

•           ram_top = 0x100000000ULL;
  

•   /* Add RAM */
  

•   for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
  

•           u64 ram_end, ram_start;
  

•           ram_start = (uintptr_t)map_sysmem(gd->bd->bi_dram[i].start, 0);
  

•           ram_end = ram_start + gd->bd->bi_dram[i].size;
  

•           efi_add_conventional_memory_map(ram_start, ram_end, ram_top);
  

•   }
  

}

/**

2.34.1

With the spelling nits fixed Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

On Tue, 8 Oct 2024 at 21:15, Sughosh Ganu sughosh.ganu@linaro.org wrote:

The EFI memory allocations are now being done through the LMB module. With this change, there is no need to get the EFI memory map and set aside EFI allocated memory.

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

Changes since V1:

• Do not remove the inclusion of efi_loader.h as it is now needed for the lmb_map_update_notify()

lib/lmb.c | 35 ----------------------------------- 1 file changed, 35 deletions(-)

diff --git a/lib/lmb.c b/lib/lmb.c index 82dfd36364b..82fa9cd14b9 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -208,38 +208,6 @@ static void lmb_fix_over_lap_regions(struct alist *lmb_rgn_lst, lmb_remove_region(lmb_rgn_lst, r2); }

-/**

• • efi_lmb_reserve() - add reservations for EFI memory
• • Add reservations for all EFI memory areas that are not
• • EFI_CONVENTIONAL_MEMORY.
• • Return: 0 on success, 1 on failure
• */

-static __maybe_unused int efi_lmb_reserve(void) -{

•   struct efi_mem_desc *memmap = NULL, *map;
  

•   efi_uintn_t i, map_size = 0;
  

•   efi_status_t ret;
  

•   ret = efi_get_memory_map_alloc(&map_size, &memmap);
  

•   if (ret != EFI_SUCCESS)
  

•           return 1;
  

•   for (i = 0, map = memmap; i < map_size / sizeof(*map); ++map, ++i) {
  

•           if (map->type != EFI_CONVENTIONAL_MEMORY) {
  

•                   lmb_reserve_flags(map_to_sysmem((void *)(uintptr_t)
  

•                                                   map->physical_start),
  

•                                     map->num_pages * EFI_PAGE_SIZE,
  

•                                     map->type == EFI_RESERVED_MEMORY_TYPE
  

•                                         ? LMB_NOMAP : LMB_NONE);
  

•           }
  

•   }
  

•   efi_free_pool(memmap);
  

•   return 0;
  

-}

static void lmb_reserve_uboot_region(void) { int bank; @@ -286,9 +254,6 @@ static void lmb_reserve_common(void *fdt_blob)

    if (CONFIG_IS_ENABLED(OF_LIBFDT) && fdt_blob)
            boot_fdt_add_mem_rsv_regions(fdt_blob);

•   if (CONFIG_IS_ENABLED(EFI_LOADER))
  

•           efi_lmb_reserve();
  

}

static __maybe_unused void lmb_reserve_common_spl(void)

2.34.1

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

Hi Sughosh,

On Tue, 8 Oct 2024 at 21:15, Sughosh Ganu sughosh.ganu@linaro.org wrote:

The functions to add a memory region to the EFI memory map have a variable, overlap_only_ram, which is used to indicate if a new memory region needs to be carved out from the free, or conventional memory.

This is a bit confusing. Conventional memory is free memory. Isn't it this from conventional or non-conventional memory?

The name overlap_only_ram is a bit confusing, as other allocated memory regions, like boot-services code and data are also part of the RAM memory. Rename the variable to highlight the fact that it is the free/conventional memory that is being referred to in this context.

overlap_conventional should be even better?

Thanks /Ilias

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

Changes since V1: New patch

include/efi_loader.h | 16 +++++++----- lib/efi_loader/efi_memory.c | 51 ++++++++++++++++++++----------------- 2 files changed, 37 insertions(+), 30 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h index 0d5555c7597..c3512c63a50 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -788,15 +788,17 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type); /**

• efi_add_memory_map_pg() - add pages to the memory map

• • @start: start address, must be a multiple of EFI_PAGE_SIZE
• • @pages: number of pages to add
• • @memory_type: type of memory added
• • @overlap_only_ram: region may only overlap RAM
• • Return: status code

• • @start: start address, must be a multiple of

• •                         EFI_PAGE_SIZE
    

• • @pages: number of pages to add
• • @memory_type: type of memory added
• • @overlap_only_freemem: region may only overlap free(conventional)

• •                         memory
    

• • Return: status code
  */

efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,

•                                     int memory_type,
  

•                                     bool overlap_only_ram);
  

•                              int memory_type,
  

•                              bool overlap_only_freemem);
  
  

/* Called by board init to initialize the EFI drivers */ efi_status_t efi_driver_init(void); diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 0f25094dbf8..97b89a97a2e 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -173,17 +173,19 @@ static void efi_mem_sort(void) /**

• efi_mem_carve_out() - unmap memory region

• • @map: memory map
• • @carve_desc: memory region to unmap
• • @overlap_only_ram: the carved out region may only overlap RAM
• • Return: the number of overlapping pages which have been

• •                 removed from the map,
    

• •                 EFI_CARVE_NO_OVERLAP, if the regions don't overlap,
    

• •                 EFI_CARVE_OVERLAPS_NONRAM, if the carve and map overlap,
    

• •                 and the map contains anything but free ram
    

• •                 (only when overlap_only_ram is true),
    

• •                 EFI_CARVE_LOOP_AGAIN, if the mapping list should be
    

• •                 traversed again, as it has been altered.
    

• • @map: memory map
• • @carve_desc: memory region to unmap
• • @overlap_only_freemem: the carved out region may only overlap free,

• •                         or conventional memory
    

• • Return: the number of overlapping pages which have been

• •                         removed from the map,
    

• •                         EFI_CARVE_NO_OVERLAP, if the regions don't
    

• •                         overlap, EFI_CARVE_OVERLAPS_NONRAM, if the carve
    

• •                         and map overlap, and the map contains anything
    

• •                         but free ram(only when overlap_only_freemem is
    

• •                         true),
    

• •                         EFI_CARVE_LOOP_AGAIN, if the mapping list should
    

• •                         be traversed again, as it has been altered.
    

  • Unmaps all memory occupied by the carve_desc region from the list entry
  • pointed to by map.

@@ -193,7 +195,7 @@ static void efi_mem_sort(void) */ static s64 efi_mem_carve_out(struct efi_mem_list *map, struct efi_mem_desc *carve_desc,

•                        bool overlap_only_ram)
  

•                        bool overlap_only_freemem)
  

{ struct efi_mem_list *newmap; struct efi_mem_desc *map_desc = &map->desc; @@ -208,7 +210,7 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, return EFI_CARVE_NO_OVERLAP;

    /* We're overlapping with non-RAM, warn the caller if desired */

•   if (overlap_only_ram && (map_desc->type != EFI_CONVENTIONAL_MEMORY))
  

•   if (overlap_only_freemem && (map_desc->type != EFI_CONVENTIONAL_MEMORY))
            return EFI_CARVE_OVERLAPS_NONRAM;
  
    /* Sanitize carve_start and carve_end to lie within our bounds */
  

@@ -258,15 +260,17 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, /**

• efi_add_memory_map_pg() - add pages to the memory map

• • @start: start address, must be a multiple of EFI_PAGE_SIZE
• • @pages: number of pages to add
• • @memory_type: type of memory added
• • @overlap_only_ram: region may only overlap RAM
• • Return: status code

• • @start: start address, must be a multiple of

• •                         EFI_PAGE_SIZE
    

• • @pages: number of pages to add
• • @memory_type: type of memory added
• • @overlap_only_freemem: region may only overlap free(conventional)

• •                         memory
    

• • Return: status code
  */

efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,

•                                     int memory_type,
  

•                                     bool overlap_only_ram)
  

•                              int memory_type,
  

•                              bool overlap_only_freemem)
  

{ struct efi_mem_list *lmem; struct efi_mem_list *newlist; @@ -275,7 +279,8 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, struct efi_event *evt;

    EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__,

•             start, pages, memory_type, overlap_only_ram ? "yes" : "no");
  

•             start, pages, memory_type, overlap_only_freemem ?
  

•             "yes" : "no");
  
    if (memory_type >= EFI_MAX_MEMORY_TYPE)
            return EFI_INVALID_PARAMETER;
  

@@ -312,7 +317,7 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, s64 r;

                    r = efi_mem_carve_out(lmem, &newlist->desc,

•                                         overlap_only_ram);
  

•                                         overlap_only_freemem);
                    switch (r) {
                    case EFI_CARVE_OUT_OF_RESOURCES:
                            free(newlist);
  

@@ -348,7 +353,7 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, } } while (carve_again);

•   if (overlap_only_ram && (carved_pages != pages)) {
  

•   if (overlap_only_freemem && (carved_pages != pages)) {
            /*
             * The payload wanted to have RAM overlaps, but we overlapped
             * with an unallocated region. Error out.
  

-- 2.34.1