[PATCH v3 06/15] lmb: notify of any changes to the LMB memory map
Ilias Apalodimas
ilias.apalodimas at linaro.org
Mon Oct 14 12:18:13 CEST 2024
Hi Sughosh
On Sun, 13 Oct 2024 at 13:56, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> In U-Boot, LMB and EFI are two primary modules who provide memory
> allocation and reservation API's. Both these modules operate with the
> same regions of memory for allocations. Use the LMB memory map update
> event to notify other interested listeners about a change in it's
> memory map. This can then be used by the other module to keep track of
> available and used memory.
>
> There is no need to send these notifications when the LMB module is
> being unit-tested. Add a flag to the lmb structure to indicate if the
> memory map is being used for tests, and suppress sending any
> notifications when running these unit tests.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> ---
> Changes since V2: None
>
> include/efi_loader.h | 14 +++++
> include/lmb.h | 2 +
> lib/efi_loader/Kconfig | 1 +
> lib/efi_loader/efi_memory.c | 2 +-
> lib/lmb.c | 109 ++++++++++++++++++++++++++++++++----
> 5 files changed, 115 insertions(+), 13 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 511281e150..ac20395718 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -784,6 +784,20 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
> uint32_t *descriptor_version);
> /* Adds a range into the EFI memory map */
> 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
> + */
> +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);
> diff --git a/include/lmb.h b/include/lmb.h
> index a76262d520..92e9aead76 100644
> --- a/include/lmb.h
> +++ b/include/lmb.h
> @@ -44,10 +44,12 @@ struct lmb_region {
> *
> * @free_mem: List of free memory regions
> * @used_mem: List of used/reserved memory regions
> + * @test: Is structure being used for LMB tests
> */
> struct lmb {
> struct alist free_mem;
> struct alist used_mem;
> + bool test;
> };
>
> /**
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 69b2c9360d..2282466ae9 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -21,6 +21,7 @@ config EFI_LOADER
> select EVENT_DYNAMIC
> select LIB_UUID
> select LMB
> + select MEM_MAP_UPDATE_NOTIFY
> imply PARTITION_UUIDS
> select REGEX
> imply FAT
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index aa1da21f9f..41501e9d41 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -264,7 +264,7 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
> * @overlap_only_ram: region may only overlap RAM
> * Return: status code
> */
> -static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
> +efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
> int memory_type,
> bool overlap_only_ram)
> {
> diff --git a/lib/lmb.c b/lib/lmb.c
> index e1e616679f..a567c02784 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -8,6 +8,7 @@
>
> #include <alist.h>
> #include <efi_loader.h>
> +#include <event.h>
> #include <image.h>
> #include <mapmem.h>
> #include <lmb.h>
> @@ -22,10 +23,55 @@
>
> DECLARE_GLOBAL_DATA_PTR;
>
> +#define MAP_OP_RESERVE (u8)0x1
> +#define MAP_OP_FREE (u8)0x2
> +#define MAP_OP_ADD (u8)0x3
> +
> #define LMB_ALLOC_ANYWHERE 0
> #define LMB_ALIST_INITIAL_SIZE 4
>
> static struct lmb lmb;
> +extern bool is_addr_in_ram(uintptr_t addr);
> +
> +static bool lmb_notify(enum lmb_flags flags)
> +{
> + return !lmb.test && !(flags & LMB_NONOTIFY);
Can you explain where the test flag is needed? Making the code aware
its testing something is usually a bad idea
> +}
> +
> +static int __maybe_unused lmb_map_update_notify(phys_addr_t addr,
> + phys_size_t size,
> + u8 op)
> +{
> + u64 efi_addr;
> + u64 pages;
> + efi_status_t status;
> +
> + if (!is_addr_in_ram((uintptr_t)addr))
> + return 0;
This should be an error and at least print something
> +
> + if (op != MAP_OP_RESERVE && op != MAP_OP_FREE && op != MAP_OP_ADD) {
> + log_debug("Invalid map update op received (%d)\n", op);
log_err
> + return -1;
> + }
> +
> + efi_addr = (uintptr_t)map_sysmem(addr, 0);
> + pages = efi_size_in_pages(size + (efi_addr & EFI_PAGE_MASK));
> + efi_addr &= ~EFI_PAGE_MASK;
> +
> + status = efi_add_memory_map_pg(efi_addr, pages,
> + op == MAP_OP_RESERVE ?
> + EFI_BOOT_SERVICES_DATA :
> + EFI_CONVENTIONAL_MEMORY,
> + false);
> +
> + if (status != EFI_SUCCESS) {
> + log_err("%s: LMB Map notify failure %lu\n", __func__,
> + status & ~EFI_ERROR_MASK);
> + return -1;
> + } else {
> + return 0;
> + }
> +}
>
> static void lmb_print_region_flags(enum lmb_flags flags)
> {
> @@ -474,9 +520,17 @@ static long lmb_add_region(struct alist *lmb_rgn_lst, phys_addr_t base,
> /* This routine may be called with relocation disabled. */
> long lmb_add(phys_addr_t base, phys_size_t size)
> {
> + long ret;
> struct alist *lmb_rgn_lst = &lmb.free_mem;
>
> - return lmb_add_region(lmb_rgn_lst, base, size);
> + ret = lmb_add_region(lmb_rgn_lst, base, size);
> + if (ret)
> + return ret;
> +
> + if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY))
> + return lmb_map_update_notify(base, size, MAP_OP_ADD);
I didn't get a chance to answer on the previous series, but I think
this should just depend on the EFI_LOADER, Is the are problem doing
that for SPL?
I think it's cleaner to add that check to lmb_notify() and rename it
to lmb__should_notify(). Then any future subsystems could use that
function and add their conditions if they need a notification, instead
of adding Kconfig options
> +
> + return 0;
> }
>
> static long __lmb_free(phys_addr_t base, phys_size_t size)
> @@ -530,11 +584,6 @@ static 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);
> -}
> -
> /**
> * lmb_free_flags() - Free up a region of memory
> * @base: Base Address of region to be freed
> @@ -546,16 +595,38 @@ long lmb_free(phys_addr_t base, phys_size_t size)
> * Return: 0 if successful, -1 on failure
> */
> long lmb_free_flags(phys_addr_t base, phys_size_t size,
> - __always_unused uint flags)
> + uint flags)
> {
> - return __lmb_free(base, size);
> + long ret;
> +
> + ret = __lmb_free(base, size);
> + if (ret < 0)
> + return ret;
> +
> + if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY) && lmb_notify(flags))
> + return lmb_map_update_notify(base, size, MAP_OP_FREE);
> +
> + return ret;
> +}
> +
> +long lmb_free(phys_addr_t base, phys_size_t size)
> +{
> + return lmb_free_flags(base, size, LMB_NONE);
> }
>
> long lmb_reserve_flags(phys_addr_t base, phys_size_t size, enum lmb_flags flags)
> {
> + long ret = 0;
> struct alist *lmb_rgn_lst = &lmb.used_mem;
>
> - return lmb_add_region_flags(lmb_rgn_lst, base, size, flags);
> + ret = lmb_add_region_flags(lmb_rgn_lst, base, size, flags);
> + if (ret < 0)
> + return -1;
> +
> + if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY) && lmb_notify(flags))
> + return lmb_map_update_notify(base, size, MAP_OP_RESERVE);
> +
> + return ret;
> }
>
> long lmb_reserve(phys_addr_t base, phys_size_t size)
> @@ -587,6 +658,8 @@ static phys_addr_t lmb_align_down(phys_addr_t addr, phys_size_t size)
> static phys_addr_t __lmb_alloc_base(phys_size_t size, ulong align,
> phys_addr_t max_addr, enum lmb_flags flags)
> {
> + u8 op;
> + int ret;
> long i, rgn;
> phys_addr_t base = 0;
> phys_addr_t res_base;
> @@ -617,6 +690,16 @@ static phys_addr_t __lmb_alloc_base(phys_size_t size, ulong align,
> if (lmb_add_region_flags(&lmb.used_mem, base,
> size, flags) < 0)
> return 0;
> +
> + if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY) &&
> + lmb_notify(flags)) {
> + op = MAP_OP_RESERVE;
> + ret = lmb_map_update_notify(base, size,
> + op);
> + if (ret)
> + return ret;
> + }
> +
> return base;
> }
>
> @@ -786,7 +869,7 @@ int lmb_is_reserved_flags(phys_addr_t addr, int flags)
> return 0;
> }
>
> -static int lmb_setup(void)
> +static int lmb_setup(bool test)
> {
> bool ret;
>
> @@ -804,6 +887,8 @@ static int lmb_setup(void)
> return -ENOMEM;
> }
>
> + lmb.test = test;
> +
> return 0;
> }
>
> @@ -823,7 +908,7 @@ int lmb_init(void)
> {
> int ret;
>
> - ret = lmb_setup();
> + ret = lmb_setup(false);
> if (ret) {
> log_info("Unable to init LMB\n");
> return ret;
> @@ -851,7 +936,7 @@ int lmb_push(struct lmb *store)
> int ret;
>
> *store = lmb;
> - ret = lmb_setup();
> + ret = lmb_setup(true);
> if (ret)
> return ret;
>
> --
> 2.34.1
>
Thanks
/Ilias
More information about the U-Boot
mailing list