[PATCH 0/3] lmb: move lmb_map_update_notify() to EFI

[PATCH 0/3] lmb: move lmb_map_update_notify() to EFI

[Heinrich Schuchardt]

When building with qemu_arm64_defconfig with CONFIG_CC_OPTIMIZE_FOR_DEBUG=y
and CONFIG_EFI_LOADER an error undefined reference to efi_add_memory_map_pg
occurs.

Move the EFI dependent part of lmb_map_update_notify() to the EFI
sub-system and clean up a little.

Thanks to Liya for reporting the issue and providing a first patch.

Heinrich Schuchardt (3):
  lmb: avoid superfluous value check in lmb_map_update_notify()
  lmb: move lmb_map_update_notify() to EFI
  efi_loader: make efi_add_memory_map_pg() static

 include/efi_loader.h        | 30 ++++++++++-----------
 include/lmb.h               | 12 +++++++++
 lib/efi_loader/efi_memory.c | 28 ++++++++++++++++++++
 lib/lmb.c                   | 52 +++++++------------------------------
 4 files changed, 64 insertions(+), 58 deletions(-)

-- 
2.47.1

[PATCH 1/3] lmb: avoid superfluous value check in lmb_map_update_notify()

[Heinrich Schuchardt]

Instead of testing the value of parameter op at runtime use an enum to
ensure that only valid values are used.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 include/lmb.h | 12 ++++++++++++
 lib/lmb.c     | 23 +++++++----------------
 2 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/include/lmb.h b/include/lmb.h
index d9d7435a431..09297a4f530 100644
--- a/include/lmb.h
+++ b/include/lmb.h
@@ -31,6 +31,18 @@
 #define LMB_NOOVERWRITE BIT(1)
 #define LMB_NONOTIFY BIT(2)
 
+/**
+ * enum lmb_map_op - memory map operation
+ */
+enum lmb_map_op {
+	/** @LMB_MAP_OP_RESERVE:	reserve memory */
+	LMB_MAP_OP_RESERVE = 1,
+	/** @LMB_MAP_OP_FREE:		free memory */
+	LMB_MAP_OP_FREE,
+	/** @LMB_MAP_OP_ADD:		add memory */
+	LMB_MAP_OP_ADD,
+};
+
 /**
  * struct lmb_region - Description of one region
  * @base: Base address of the region
diff --git a/lib/lmb.c b/lib/lmb.c
index 7ca44591e1d..7534a231c99 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -23,10 +23,6 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-#define MAP_OP_RESERVE		(u8)0x1
-#define MAP_OP_FREE		(u8)0x2
-#define MAP_OP_ADD		(u8)0x3
-
 /*
  * The following low level LMB functions must not access the global LMB memory
  * map since they are also used to manage IOVA memory maps in iommu drivers like
@@ -436,18 +432,13 @@ static bool lmb_should_notify(u32 flags)
 		CONFIG_IS_ENABLED(EFI_LOADER);
 }
 
-static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op,
-				 u32 flags)
+static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size,
+				 enum lmb_map_op op, u32 flags)
 {
 	u64 efi_addr;
 	u64 pages;
 	efi_status_t status;
 
-	if (op != MAP_OP_RESERVE && op != MAP_OP_FREE && op != MAP_OP_ADD) {
-		log_err("Invalid map update op received (%d)\n", op);
-		return -1;
-	}
-
 	if (!lmb_should_notify(flags))
 		return 0;
 
@@ -456,7 +447,7 @@ static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op,
 	efi_addr &= ~EFI_PAGE_MASK;
 
 	status = efi_add_memory_map_pg(efi_addr, pages,
-				       op == MAP_OP_RESERVE ?
+				       op == LMB_MAP_OP_RESERVE ?
 				       EFI_BOOT_SERVICES_DATA :
 				       EFI_CONVENTIONAL_MEMORY,
 				       false);
@@ -642,7 +633,7 @@ long lmb_add(phys_addr_t base, phys_size_t size)
 	if (ret)
 		return ret;
 
-	return lmb_map_update_notify(base, size, MAP_OP_ADD, LMB_NONE);
+	return lmb_map_update_notify(base, size, LMB_MAP_OP_ADD, LMB_NONE);
 }
 
 long lmb_free_flags(phys_addr_t base, phys_size_t size,
@@ -654,7 +645,7 @@ long lmb_free_flags(phys_addr_t base, phys_size_t size,
 	if (ret < 0)
 		return ret;
 
-	return lmb_map_update_notify(base, size, MAP_OP_FREE, flags);
+	return lmb_map_update_notify(base, size, LMB_MAP_OP_FREE, flags);
 }
 
 long lmb_free(phys_addr_t base, phys_size_t size)
@@ -671,7 +662,7 @@ long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags)
 	if (ret)
 		return ret;
 
-	return lmb_map_update_notify(base, size, MAP_OP_RESERVE, flags);
+	return lmb_map_update_notify(base, size, LMB_MAP_OP_RESERVE, flags);
 }
 
 static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align,
@@ -712,7 +703,7 @@ static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align,
 					return 0;
 
 				ret = lmb_map_update_notify(base, size,
-							    MAP_OP_RESERVE,
+							    LMB_MAP_OP_RESERVE,
 							    flags);
 				if (ret)
 					return ret;
-- 
2.47.1

Re: [PATCH 1/3] lmb: avoid superfluous value check in lmb_map_update_notify()

[Ilias Apalodimas]

On Sun, 16 Feb 2025 at 13:12, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Instead of testing the value of parameter op at runtime use an enum to
> ensure that only valid values are used.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  include/lmb.h | 12 ++++++++++++
>  lib/lmb.c     | 23 +++++++----------------
>  2 files changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/include/lmb.h b/include/lmb.h
> index d9d7435a431..09297a4f530 100644
> --- a/include/lmb.h
> +++ b/include/lmb.h
> @@ -31,6 +31,18 @@
>  #define LMB_NOOVERWRITE BIT(1)
>  #define LMB_NONOTIFY BIT(2)
>
> +/**
> + * enum lmb_map_op - memory map operation
> + */
> +enum lmb_map_op {
> +       /** @LMB_MAP_OP_RESERVE:        reserve memory */
> +       LMB_MAP_OP_RESERVE = 1,
> +       /** @LMB_MAP_OP_FREE:           free memory */
> +       LMB_MAP_OP_FREE,
> +       /** @LMB_MAP_OP_ADD:            add memory */
> +       LMB_MAP_OP_ADD,
> +};
> +
>  /**
>   * struct lmb_region - Description of one region
>   * @base: Base address of the region
> diff --git a/lib/lmb.c b/lib/lmb.c
> index 7ca44591e1d..7534a231c99 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -23,10 +23,6 @@
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> -#define MAP_OP_RESERVE         (u8)0x1
> -#define MAP_OP_FREE            (u8)0x2
> -#define MAP_OP_ADD             (u8)0x3
> -
>  /*
>   * The following low level LMB functions must not access the global LMB memory
>   * map since they are also used to manage IOVA memory maps in iommu drivers like
> @@ -436,18 +432,13 @@ static bool lmb_should_notify(u32 flags)
>                 CONFIG_IS_ENABLED(EFI_LOADER);
>  }
>
> -static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op,
> -                                u32 flags)
> +static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size,
> +                                enum lmb_map_op op, u32 flags)
>  {
>         u64 efi_addr;
>         u64 pages;
>         efi_status_t status;
>
> -       if (op != MAP_OP_RESERVE && op != MAP_OP_FREE && op != MAP_OP_ADD) {
> -               log_err("Invalid map update op received (%d)\n", op);
> -               return -1;
> -       }
> -
>         if (!lmb_should_notify(flags))
>                 return 0;
>
> @@ -456,7 +447,7 @@ static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op,
>         efi_addr &= ~EFI_PAGE_MASK;
>
>         status = efi_add_memory_map_pg(efi_addr, pages,
> -                                      op == MAP_OP_RESERVE ?
> +                                      op == LMB_MAP_OP_RESERVE ?
>                                        EFI_BOOT_SERVICES_DATA :
>                                        EFI_CONVENTIONAL_MEMORY,
>                                        false);
> @@ -642,7 +633,7 @@ long lmb_add(phys_addr_t base, phys_size_t size)
>         if (ret)
>                 return ret;
>
> -       return lmb_map_update_notify(base, size, MAP_OP_ADD, LMB_NONE);
> +       return lmb_map_update_notify(base, size, LMB_MAP_OP_ADD, LMB_NONE);
>  }
>
>  long lmb_free_flags(phys_addr_t base, phys_size_t size,
> @@ -654,7 +645,7 @@ long lmb_free_flags(phys_addr_t base, phys_size_t size,
>         if (ret < 0)
>                 return ret;
>
> -       return lmb_map_update_notify(base, size, MAP_OP_FREE, flags);
> +       return lmb_map_update_notify(base, size, LMB_MAP_OP_FREE, flags);
>  }
>
>  long lmb_free(phys_addr_t base, phys_size_t size)
> @@ -671,7 +662,7 @@ long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags)
>         if (ret)
>                 return ret;
>
> -       return lmb_map_update_notify(base, size, MAP_OP_RESERVE, flags);
> +       return lmb_map_update_notify(base, size, LMB_MAP_OP_RESERVE, flags);
>  }
>
>  static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align,
> @@ -712,7 +703,7 @@ static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align,
>                                         return 0;
>
>                                 ret = lmb_map_update_notify(base, size,
> -                                                           MAP_OP_RESERVE,
> +                                                           LMB_MAP_OP_RESERVE,
>                                                             flags);
>                                 if (ret)
>                                         return ret;
> --
> 2.47.1
>

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

[PATCH 2/3] lmb: move lmb_map_update_notify() to EFI

[Heinrich Schuchardt]

When building with qemu_arm64_defconfig with CONFIG_CC_OPTIMIZE_FOR_DEBUG=y
and CONFIG_EFI_LOADER an error undefined reference to efi_add_memory_map_pg
occurs.

Move the EFI dependent part of lmb_map_update_notify() to the EFI
sub-system.

Reported-by: Liya Huang <1425075683@qq.com>
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Acked-by: Liya Huang <1425075683@qq.com>
---
 include/efi_loader.h        | 15 +++++++++++++++
 lib/efi_loader/efi_memory.c | 27 +++++++++++++++++++++++++++
 lib/lmb.c                   | 31 +++----------------------------
 3 files changed, 45 insertions(+), 28 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index dcae6a731a0..db3d20fd753 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -1263,6 +1263,21 @@ efi_status_t efi_disk_get_device_name(const efi_handle_t handle, char *buf, int
  */
 void efi_add_known_memory(void);
 
+/**
+ * efi_map_update_notify() - notify EFI of memory map changes
+ *
+ * @addr:	start of memory area
+ * @size:	size of memory area
+ * @op:		type of change
+ * Return:	0 if change could be processed
+ */
+#ifdef CONFIG_EFI_LOADER
+int efi_map_update_notify(phys_addr_t addr, phys_size_t size,
+			  enum lmb_map_op op);
+#else
+#define efi_map_update_notify(addr, size, op) (0)
+#endif
+
 /**
  * efi_load_option_dp_join() - join device-paths for load option
  *
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 1212772471e..11d092dc289 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -865,3 +865,30 @@ int efi_memory_init(void)
 
 	return 0;
 }
+
+int efi_map_update_notify(phys_addr_t addr, phys_size_t size,
+			  enum lmb_map_op op)
+{
+	u64 efi_addr;
+	u64 pages;
+	efi_status_t status;
+
+	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 == LMB_MAP_OP_RESERVE ?
+				       EFI_BOOT_SERVICES_DATA :
+				       EFI_CONVENTIONAL_MEMORY,
+				       false);
+	if (status != EFI_SUCCESS) {
+		log_err("LMB Map notify failure %lu\n",
+			status & ~EFI_ERROR_MASK);
+		return -1;
+	}
+	unmap_sysmem((void *)(uintptr_t)efi_addr);
+
+	return 0;
+}
+
diff --git a/lib/lmb.c b/lib/lmb.c
index 7534a231c99..93fc1bea07c 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -426,37 +426,12 @@ long io_lmb_free(struct lmb *io_lmb, phys_addr_t base, phys_size_t size)
 
 static struct lmb lmb;
 
-static bool lmb_should_notify(u32 flags)
-{
-	return !lmb.test && !(flags & LMB_NONOTIFY) &&
-		CONFIG_IS_ENABLED(EFI_LOADER);
-}
-
 static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size,
 				 enum lmb_map_op op, u32 flags)
 {
-	u64 efi_addr;
-	u64 pages;
-	efi_status_t status;
-
-	if (!lmb_should_notify(flags))
-		return 0;
-
-	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 == LMB_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;
-	}
-	unmap_sysmem((void *)(uintptr_t)efi_addr);
+	if (CONFIG_IS_ENABLED(EFI_LOADER) &&
+	    !lmb.test && !(flags & LMB_NONOTIFY))
+		return efi_map_update_notify(addr, size, op);
 
 	return 0;
 }
-- 
2.47.1

Re: [PATCH 2/3] lmb: move lmb_map_update_notify() to EFI

[Sughosh Ganu]

On Sun, 16 Feb 2025 at 16:42, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> When building with qemu_arm64_defconfig with CONFIG_CC_OPTIMIZE_FOR_DEBUG=y
> and CONFIG_EFI_LOADER an error undefined reference to efi_add_memory_map_pg
> occurs.

"and CONFIG_EFI_LOADER disabled".

>
> Move the EFI dependent part of lmb_map_update_notify() to the EFI
> sub-system.

Can this be rebased on the pmem patch series [1]. The pmem patches
make certain changes to the name of the function being tweaked here,
so it will be better if this patch series is rebased on top of the
pmem patch series. Thanks.

-sughosh

[1] - https://lore.kernel.org/u-boot/20250203105912.196654-1-sughosh.ganu@linaro.org/T/#mf8100ad07f82b781d14102b39948b36ec8edea0a
>
> Reported-by: Liya Huang <1425075683@qq.com>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Acked-by: Liya Huang <1425075683@qq.com>
> ---
>  include/efi_loader.h        | 15 +++++++++++++++
>  lib/efi_loader/efi_memory.c | 27 +++++++++++++++++++++++++++
>  lib/lmb.c                   | 31 +++----------------------------
>  3 files changed, 45 insertions(+), 28 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index dcae6a731a0..db3d20fd753 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -1263,6 +1263,21 @@ efi_status_t efi_disk_get_device_name(const efi_handle_t handle, char *buf, int
>   */
>  void efi_add_known_memory(void);
>
> +/**
> + * efi_map_update_notify() - notify EFI of memory map changes
> + *
> + * @addr:      start of memory area
> + * @size:      size of memory area
> + * @op:                type of change
> + * Return:     0 if change could be processed
> + */
> +#ifdef CONFIG_EFI_LOADER
> +int efi_map_update_notify(phys_addr_t addr, phys_size_t size,
> +                         enum lmb_map_op op);
> +#else
> +#define efi_map_update_notify(addr, size, op) (0)
> +#endif
> +
>  /**
>   * efi_load_option_dp_join() - join device-paths for load option
>   *
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 1212772471e..11d092dc289 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -865,3 +865,30 @@ int efi_memory_init(void)
>
>         return 0;
>  }
> +
> +int efi_map_update_notify(phys_addr_t addr, phys_size_t size,
> +                         enum lmb_map_op op)
> +{
> +       u64 efi_addr;
> +       u64 pages;
> +       efi_status_t status;
> +
> +       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 == LMB_MAP_OP_RESERVE ?
> +                                      EFI_BOOT_SERVICES_DATA :
> +                                      EFI_CONVENTIONAL_MEMORY,
> +                                      false);
> +       if (status != EFI_SUCCESS) {
> +               log_err("LMB Map notify failure %lu\n",
> +                       status & ~EFI_ERROR_MASK);
> +               return -1;
> +       }
> +       unmap_sysmem((void *)(uintptr_t)efi_addr);
> +
> +       return 0;
> +}
> +
> diff --git a/lib/lmb.c b/lib/lmb.c
> index 7534a231c99..93fc1bea07c 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -426,37 +426,12 @@ long io_lmb_free(struct lmb *io_lmb, phys_addr_t base, phys_size_t size)
>
>  static struct lmb lmb;
>
> -static bool lmb_should_notify(u32 flags)
> -{
> -       return !lmb.test && !(flags & LMB_NONOTIFY) &&
> -               CONFIG_IS_ENABLED(EFI_LOADER);
> -}
> -
>  static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size,
>                                  enum lmb_map_op op, u32 flags)
>  {
> -       u64 efi_addr;
> -       u64 pages;
> -       efi_status_t status;
> -
> -       if (!lmb_should_notify(flags))
> -               return 0;
> -
> -       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 == LMB_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;
> -       }
> -       unmap_sysmem((void *)(uintptr_t)efi_addr);
> +       if (CONFIG_IS_ENABLED(EFI_LOADER) &&
> +           !lmb.test && !(flags & LMB_NONOTIFY))
> +               return efi_map_update_notify(addr, size, op);
>
>         return 0;
>  }
> --
> 2.47.1
>

Re: [PATCH 2/3] lmb: move lmb_map_update_notify() to EFI

[Heinrich Schuchardt]

On 2/17/25 10:55, Sughosh Ganu wrote:
> On Sun, 16 Feb 2025 at 16:42, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> When building with qemu_arm64_defconfig with CONFIG_CC_OPTIMIZE_FOR_DEBUG=y
>> and CONFIG_EFI_LOADER an error undefined reference to efi_add_memory_map_pg
>> occurs.
> 
> "and CONFIG_EFI_LOADER disabled".
> 
>>
>> Move the EFI dependent part of lmb_map_update_notify() to the EFI
>> sub-system.
> 
> Can this be rebased on the pmem patch series [1]. The pmem patches
> make certain changes to the name of the function being tweaked here,
> so it will be better if this patch series is rebased on top of the
> pmem patch series. Thanks.

This is a bug fix which will go into master.

Your pmem patches will go into next once Ilias and I have finished 
reviewing.

Best regards

Heinrich

> 
> -sughosh
> 
> [1] - https://lore.kernel.org/u-boot/20250203105912.196654-1-sughosh.ganu@linaro.org/T/#mf8100ad07f82b781d14102b39948b36ec8edea0a
>>
>> Reported-by: Liya Huang <1425075683@qq.com>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> Acked-by: Liya Huang <1425075683@qq.com>
>> ---
>>   include/efi_loader.h        | 15 +++++++++++++++
>>   lib/efi_loader/efi_memory.c | 27 +++++++++++++++++++++++++++
>>   lib/lmb.c                   | 31 +++----------------------------
>>   3 files changed, 45 insertions(+), 28 deletions(-)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index dcae6a731a0..db3d20fd753 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -1263,6 +1263,21 @@ efi_status_t efi_disk_get_device_name(const efi_handle_t handle, char *buf, int
>>    */
>>   void efi_add_known_memory(void);
>>
>> +/**
>> + * efi_map_update_notify() - notify EFI of memory map changes
>> + *
>> + * @addr:      start of memory area
>> + * @size:      size of memory area
>> + * @op:                type of change
>> + * Return:     0 if change could be processed
>> + */
>> +#ifdef CONFIG_EFI_LOADER
>> +int efi_map_update_notify(phys_addr_t addr, phys_size_t size,
>> +                         enum lmb_map_op op);
>> +#else
>> +#define efi_map_update_notify(addr, size, op) (0)
>> +#endif
>> +
>>   /**
>>    * efi_load_option_dp_join() - join device-paths for load option
>>    *
>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> index 1212772471e..11d092dc289 100644
>> --- a/lib/efi_loader/efi_memory.c
>> +++ b/lib/efi_loader/efi_memory.c
>> @@ -865,3 +865,30 @@ int efi_memory_init(void)
>>
>>          return 0;
>>   }
>> +
>> +int efi_map_update_notify(phys_addr_t addr, phys_size_t size,
>> +                         enum lmb_map_op op)
>> +{
>> +       u64 efi_addr;
>> +       u64 pages;
>> +       efi_status_t status;
>> +
>> +       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 == LMB_MAP_OP_RESERVE ?
>> +                                      EFI_BOOT_SERVICES_DATA :
>> +                                      EFI_CONVENTIONAL_MEMORY,
>> +                                      false);
>> +       if (status != EFI_SUCCESS) {
>> +               log_err("LMB Map notify failure %lu\n",
>> +                       status & ~EFI_ERROR_MASK);
>> +               return -1;
>> +       }
>> +       unmap_sysmem((void *)(uintptr_t)efi_addr);
>> +
>> +       return 0;
>> +}
>> +
>> diff --git a/lib/lmb.c b/lib/lmb.c
>> index 7534a231c99..93fc1bea07c 100644
>> --- a/lib/lmb.c
>> +++ b/lib/lmb.c
>> @@ -426,37 +426,12 @@ long io_lmb_free(struct lmb *io_lmb, phys_addr_t base, phys_size_t size)
>>
>>   static struct lmb lmb;
>>
>> -static bool lmb_should_notify(u32 flags)
>> -{
>> -       return !lmb.test && !(flags & LMB_NONOTIFY) &&
>> -               CONFIG_IS_ENABLED(EFI_LOADER);
>> -}
>> -
>>   static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size,
>>                                   enum lmb_map_op op, u32 flags)
>>   {
>> -       u64 efi_addr;
>> -       u64 pages;
>> -       efi_status_t status;
>> -
>> -       if (!lmb_should_notify(flags))
>> -               return 0;
>> -
>> -       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 == LMB_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;
>> -       }
>> -       unmap_sysmem((void *)(uintptr_t)efi_addr);
>> +       if (CONFIG_IS_ENABLED(EFI_LOADER) &&
>> +           !lmb.test && !(flags & LMB_NONOTIFY))
>> +               return efi_map_update_notify(addr, size, op);
>>
>>          return 0;
>>   }
>> --
>> 2.47.1
>>

Re: [PATCH 2/3] lmb: move lmb_map_update_notify() to EFI

[Ilias Apalodimas]

Hi Heinrich

On Sun, 16 Feb 2025 at 13:12, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> When building with qemu_arm64_defconfig with CONFIG_CC_OPTIMIZE_FOR_DEBUG=y
> and CONFIG_EFI_LOADER

CONFIG_EFI_LOADER is disabled

> an error undefined reference to efi_add_memory_map_pg
> occurs.
>
> Move the EFI dependent part of lmb_map_update_notify() to the EFI
> sub-system.
>
> Reported-by: Liya Huang <1425075683@qq.com>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Acked-by: Liya Huang <1425075683@qq.com>
> ---
>  include/efi_loader.h        | 15 +++++++++++++++
>  lib/efi_loader/efi_memory.c | 27 +++++++++++++++++++++++++++
>  lib/lmb.c                   | 31 +++----------------------------
>  3 files changed, 45 insertions(+), 28 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index dcae6a731a0..db3d20fd753 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -1263,6 +1263,21 @@ efi_status_t efi_disk_get_device_name(const efi_handle_t handle, char *buf, int
>   */
>  void efi_add_known_memory(void);
>
> +/**
> + * efi_map_update_notify() - notify EFI of memory map changes
> + *
> + * @addr:      start of memory area
> + * @size:      size of memory area
> + * @op:                type of change
> + * Return:     0 if change could be processed
> + */
> +#ifdef CONFIG_EFI_LOADER
> +int efi_map_update_notify(phys_addr_t addr, phys_size_t size,
> +                         enum lmb_map_op op);
> +#else
> +#define efi_map_update_notify(addr, size, op) (0)
> +#endif
> +
>  /**
>   * efi_load_option_dp_join() - join device-paths for load option
>   *
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 1212772471e..11d092dc289 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -865,3 +865,30 @@ int efi_memory_init(void)
>
>         return 0;
>  }
> +
> +int efi_map_update_notify(phys_addr_t addr, phys_size_t size,
> +                         enum lmb_map_op op)
> +{
> +       u64 efi_addr;
> +       u64 pages;
> +       efi_status_t status;
> +
> +       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 == LMB_MAP_OP_RESERVE ?
> +                                      EFI_BOOT_SERVICES_DATA :
> +                                      EFI_CONVENTIONAL_MEMORY,
> +                                      false);
> +       if (status != EFI_SUCCESS) {
> +               log_err("LMB Map notify failure %lu\n",
> +                       status & ~EFI_ERROR_MASK);
> +               return -1;
> +       }
> +       unmap_sysmem((void *)(uintptr_t)efi_addr);
> +
> +       return 0;
> +}
> +
> diff --git a/lib/lmb.c b/lib/lmb.c
> index 7534a231c99..93fc1bea07c 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -426,37 +426,12 @@ long io_lmb_free(struct lmb *io_lmb, phys_addr_t base, phys_size_t size)
>
>  static struct lmb lmb;
>
> -static bool lmb_should_notify(u32 flags)
> -{
> -       return !lmb.test && !(flags & LMB_NONOTIFY) &&
> -               CONFIG_IS_ENABLED(EFI_LOADER);
> -}
> -
>  static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size,
>                                  enum lmb_map_op op, u32 flags)
>  {
> -       u64 efi_addr;
> -       u64 pages;
> -       efi_status_t status;
> -
> -       if (!lmb_should_notify(flags))
> -               return 0;
> -
> -       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 == LMB_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;
> -       }
> -       unmap_sysmem((void *)(uintptr_t)efi_addr);
> +       if (CONFIG_IS_ENABLED(EFI_LOADER) &&
> +           !lmb.test && !(flags & LMB_NONOTIFY))
> +               return efi_map_update_notify(addr, size, op);
>

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

>         return 0;
>  }
> --
> 2.47.1
>

Re: [PATCH 2/3] lmb: move lmb_map_update_notify() to EFI

[Alexander Dahl]

Hei hei,

Am Sun, Feb 16, 2025 at 12:12:40PM +0100 schrieb Heinrich Schuchardt:
> When building with qemu_arm64_defconfig with CONFIG_CC_OPTIMIZE_FOR_DEBUG=y
> and CONFIG_EFI_LOADER an error undefined reference to efi_add_memory_map_pg
> occurs.
> 
> Move the EFI dependent part of lmb_map_update_notify() to the EFI
> sub-system.
> 
> Reported-by: Liya Huang <1425075683@qq.com>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Acked-by: Liya Huang <1425075683@qq.com>

This seems to be the same underlying problem I reported back in January:

https://lore.kernel.org/u-boot/20250117-molecular-quicksand-e80bf947313e@thorsis.com/

FWIW, while your patch hit master, and the follow-up to my report did
not yet … you might be interested that master does not fail to build
the armv5 targets with CC_OPTIMIZE_FOR_DEBUG anymore.

Besides: keeping EFI code in the EFI subsystem seems the right
approach to me, too. ;-)

Thanks and Greets
Alex

> ---
>  include/efi_loader.h        | 15 +++++++++++++++
>  lib/efi_loader/efi_memory.c | 27 +++++++++++++++++++++++++++
>  lib/lmb.c                   | 31 +++----------------------------
>  3 files changed, 45 insertions(+), 28 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index dcae6a731a0..db3d20fd753 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -1263,6 +1263,21 @@ efi_status_t efi_disk_get_device_name(const efi_handle_t handle, char *buf, int
>   */
>  void efi_add_known_memory(void);
>  
> +/**
> + * efi_map_update_notify() - notify EFI of memory map changes
> + *
> + * @addr:	start of memory area
> + * @size:	size of memory area
> + * @op:		type of change
> + * Return:	0 if change could be processed
> + */
> +#ifdef CONFIG_EFI_LOADER
> +int efi_map_update_notify(phys_addr_t addr, phys_size_t size,
> +			  enum lmb_map_op op);
> +#else
> +#define efi_map_update_notify(addr, size, op) (0)
> +#endif
> +
>  /**
>   * efi_load_option_dp_join() - join device-paths for load option
>   *
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 1212772471e..11d092dc289 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -865,3 +865,30 @@ int efi_memory_init(void)
>  
>  	return 0;
>  }
> +
> +int efi_map_update_notify(phys_addr_t addr, phys_size_t size,
> +			  enum lmb_map_op op)
> +{
> +	u64 efi_addr;
> +	u64 pages;
> +	efi_status_t status;
> +
> +	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 == LMB_MAP_OP_RESERVE ?
> +				       EFI_BOOT_SERVICES_DATA :
> +				       EFI_CONVENTIONAL_MEMORY,
> +				       false);
> +	if (status != EFI_SUCCESS) {
> +		log_err("LMB Map notify failure %lu\n",
> +			status & ~EFI_ERROR_MASK);
> +		return -1;
> +	}
> +	unmap_sysmem((void *)(uintptr_t)efi_addr);
> +
> +	return 0;
> +}
> +
> diff --git a/lib/lmb.c b/lib/lmb.c
> index 7534a231c99..93fc1bea07c 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -426,37 +426,12 @@ long io_lmb_free(struct lmb *io_lmb, phys_addr_t base, phys_size_t size)
>  
>  static struct lmb lmb;
>  
> -static bool lmb_should_notify(u32 flags)
> -{
> -	return !lmb.test && !(flags & LMB_NONOTIFY) &&
> -		CONFIG_IS_ENABLED(EFI_LOADER);
> -}
> -
>  static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size,
>  				 enum lmb_map_op op, u32 flags)
>  {
> -	u64 efi_addr;
> -	u64 pages;
> -	efi_status_t status;
> -
> -	if (!lmb_should_notify(flags))
> -		return 0;
> -
> -	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 == LMB_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;
> -	}
> -	unmap_sysmem((void *)(uintptr_t)efi_addr);
> +	if (CONFIG_IS_ENABLED(EFI_LOADER) &&
> +	    !lmb.test && !(flags & LMB_NONOTIFY))
> +		return efi_map_update_notify(addr, size, op);
>  
>  	return 0;
>  }
> -- 
> 2.47.1
> 

[PATCH 3/3] efi_loader: make efi_add_memory_map_pg() static

[Heinrich Schuchardt]

The function is only used in the efi_memory.c module.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 include/efi_loader.h        | 15 ---------------
 lib/efi_loader/efi_memory.c |  1 +
 2 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index db3d20fd753..1d75d97ebbc 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -852,21 +852,6 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 /* 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_conventional:	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_conventional);
-
 /* Called by board init to initialize the EFI drivers */
 efi_status_t efi_driver_init(void);
 /* Called when a block device is added */
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 11d092dc289..6d00b186250 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -268,6 +268,7 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
  *				memory
  * Return:			status code
  */
+static
 efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
 				   int memory_type,
 				   bool overlap_conventional)
-- 
2.47.1

Re: [PATCH 3/3] efi_loader: make efi_add_memory_map_pg() static

[Ilias Apalodimas]

On Sun, 16 Feb 2025 at 13:13, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> The function is only used in the efi_memory.c module.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  include/efi_loader.h        | 15 ---------------
>  lib/efi_loader/efi_memory.c |  1 +
>  2 files changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index db3d20fd753..1d75d97ebbc 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -852,21 +852,6 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
>  /* 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_conventional:      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_conventional);
> -
>  /* Called by board init to initialize the EFI drivers */
>  efi_status_t efi_driver_init(void);
>  /* Called when a block device is added */
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 11d092dc289..6d00b186250 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -268,6 +268,7 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
>   *                             memory
>   * Return:                     status code
>   */
> +static
>  efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
>                                    int memory_type,
>                                    bool overlap_conventional)
> --
> 2.47.1
>

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