Re: [RFC PATCH 03/31] lmb: make the lmb reservations persistent
HI Sughosh,
On Fri, 7 Jun 2024 at 12:53, Sughosh Ganu wrote:
>
> The current LMB API's for allocating and reserving memory use a
> per-caller based memory view. Memory allocated by a caller can then be
> overwritten by another caller. Make these allocations and reservations
> persistent. With this, memory allocated or reserved will not be
> overwritten.
>
> Signed-off-by: Sughosh Ganu
> ---
>
> Note: @Mark Kettenis, please review the changes made in the apple dart
> driver. I have removed the driver-local LMB instance, but I am not
> sure if the current logic makes sense. I would think that it would be
> possible to simply use memory region allocated by the LMB module(maybe
> using the max address argument), instead of adding specific memory
> region with lmb_add().
>
> arch/arm/mach-stm32mp/dram_init.c | 1 -
> board/xilinx/common/board.c | 1 -
> drivers/iommu/apple_dart.c| 1 -
> drivers/iommu/sandbox_iommu.c | 1 -
> include/lmb.h | 17 ++
> lib/lmb.c | 91 ---
> test/lib/lmb.c| 18 --
> 7 files changed, 50 insertions(+), 80 deletions(-)
>
> diff --git a/arch/arm/mach-stm32mp/dram_init.c
> b/arch/arm/mach-stm32mp/dram_init.c
> index fb1208fc5d..86d6577b35 100644
> --- a/arch/arm/mach-stm32mp/dram_init.c
> +++ b/arch/arm/mach-stm32mp/dram_init.c
> @@ -59,7 +59,6 @@ phys_addr_t board_get_usable_ram_top(phys_size_t total_size)
> gd->ram_top = clamp_val(gd->ram_top, 0, SZ_4G - 1);
>
> /* found enough not-reserved memory to relocated U-Boot */
> - lmb_init();
> lmb_add(, gd->ram_base, gd->ram_top - gd->ram_base);
> boot_fdt_add_mem_rsv_regions(, (void *)gd->fdt_blob);
> /* add 8M for reserved memory for display, fdt, gd,... */
> diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
> index b47d2d23f9..ebe57da7f8 100644
> --- a/board/xilinx/common/board.c
> +++ b/board/xilinx/common/board.c
> @@ -685,7 +685,6 @@ phys_addr_t board_get_usable_ram_top(phys_size_t
> total_size)
> panic("Not 64bit aligned DT location: %p\n", gd->fdt_blob);
>
> /* found enough not-reserved memory to relocated U-Boot */
> - lmb_init();
> lmb_add(, gd->ram_base, gd->ram_size);
> boot_fdt_add_mem_rsv_regions(, (void *)gd->fdt_blob);
> size = ALIGN(CONFIG_SYS_MALLOC_LEN + total_size, MMU_SECTION_SIZE);
> diff --git a/drivers/iommu/apple_dart.c b/drivers/iommu/apple_dart.c
> index 6ecd84303b..ef385d9c9f 100644
> --- a/drivers/iommu/apple_dart.c
> +++ b/drivers/iommu/apple_dart.c
> @@ -214,7 +214,6 @@ static int apple_dart_probe(struct udevice *dev)
> priv->dvabase = DART_PAGE_SIZE;
> priv->dvaend = SZ_4G - DART_PAGE_SIZE;
>
> - lmb_init(>lmb);
> lmb_add(>lmb, priv->dvabase, priv->dvaend - priv->dvabase);
>
> /* Disable translations. */
> diff --git a/drivers/iommu/sandbox_iommu.c b/drivers/iommu/sandbox_iommu.c
> index 6ceb7fd5ec..31ce91345c 100644
> --- a/drivers/iommu/sandbox_iommu.c
> +++ b/drivers/iommu/sandbox_iommu.c
> @@ -55,7 +55,6 @@ static int sandbox_iommu_probe(struct udevice *dev)
> {
> struct sandbox_iommu_priv *priv = dev_get_priv(dev);
>
> - lmb_init(>lmb);
> lmb_add(>lmb, 0x89abc000, SZ_16K);
>
> return 0;
> diff --git a/include/lmb.h b/include/lmb.h
> index 7b87181b9e..bbe1c5299c 100644
> --- a/include/lmb.h
> +++ b/include/lmb.h
> @@ -49,8 +49,7 @@ struct lmb_property {
> * => CONFIG_LMB_MEMORY_REGIONS: struct lmb.memory_regions
> * => CONFIG_LMB_RESERVED_REGIONS: struct lmb.reserved_regions
> * lmb_region.region is only a pointer to the correct buffer,
> - * initialized in lmb_init(). This configuration is useful to manage
> - * more reserved memory regions with CONFIG_LMB_RESERVED_REGIONS.
> + * initialized with these values.
> */
>
> /**
> @@ -73,26 +72,18 @@ struct lmb_region {
> /**
> * struct lmb - Logical memory block handle.
> *
> - * Clients provide storage for Logical memory block (lmb) handles.
> - * The content of the structure is managed by the lmb library.
> - * A lmb struct is initialized by lmb_init() functions.
> - * The lmb struct is passed to all other lmb APIs.
> + * The Logical Memory Block structure which is used to keep track
> + * of available memory which can be used for stuff like loading
> + * images(kernel, initrd, fdt).
> *
> * @memory: Description of memory regions.
> * @reserved: Description of reserved regions.
> - * @memory_regions: Array of the memory regions (statically allocated)
> - * @reserved_regions: Array of the reserved regions (statically allocated)
> */
> struct lmb {
> struct lmb_region memory;
> struct lmb_region reserved;
> -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
> - struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS];
> - struct lmb_property
Re: [RFC PATCH 03/31] lmb: make the lmb reservations persistent
On Mon, Jun 10, 2024 at 01:23:49PM +0200, Heinrich Schuchardt wrote:
> On 1/1/70 01:00, Ilias Apalodimas wrote:
> > Hi Sughosh
> >
> > [...]
> >
> > > #define LMB_ALLOC_ANYWHERE 0
> > >
> > > +#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
> > > +struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS];
> > > +struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS];
> > > +#endif
> > > +
> > > +struct lmb lmb = {
> > > +#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
> > > + .memory.max = CONFIG_LMB_MAX_REGIONS,
> > > + .reserved.max = CONFIG_LMB_MAX_REGIONS,
> > > +#else
> > > + .memory.max = CONFIG_LMB_MEMORY_REGIONS,
> > > + .reserved.max = CONFIG_LMB_RESERVED_REGIONS,
> > > + .memory.region = memory_regions,
> > > + .reserved.region = reserved_regions,
> >
> > This is probably a good opportunity to look into why
> > CONFIG_LMB_MEMORY_REGIONS was introduced. Since we are moving towards
> > static allocations, do we still need it? Or allocating the size dynamically
> > covers all our cases.
>
> Up to now we used static arrays for saving memory allocations in LMB:
>
> include/lmb.h:67:
> struct lmb_property region[CONFIG_LMB_MAX_REGIONS];
>
> As the EFI sub-system can produce any number of non-coalescable memory
> regions we should use a linked list instead.
I think it's some historic flexibility that's I believe no longer really
relevant to how we use LMB today, let alone once this patch series is
complete. We should probably (I'm doing my size check thing now..) move
to just following Heinrich's suggestion.
--
Tom
signature.asc
Description: PGP signature
Re: [RFC PATCH 03/31] lmb: make the lmb reservations persistent
On 1/1/70 01:00, Ilias Apalodimas wrote:
Hi Sughosh
[...]
#define LMB_ALLOC_ANYWHERE0
+#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
+struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS];
+struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS];
+#endif
+
+struct lmb lmb = {
+#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
+ .memory.max = CONFIG_LMB_MAX_REGIONS,
+ .reserved.max = CONFIG_LMB_MAX_REGIONS,
+#else
+ .memory.max = CONFIG_LMB_MEMORY_REGIONS,
+ .reserved.max = CONFIG_LMB_RESERVED_REGIONS,
+ .memory.region = memory_regions,
+ .reserved.region = reserved_regions,
This is probably a good opportunity to look into why
CONFIG_LMB_MEMORY_REGIONS was introduced. Since we are moving towards
static allocations, do we still need it? Or allocating the size dynamically
covers all our cases.
Up to now we used static arrays for saving memory allocations in LMB:
include/lmb.h:67:
struct lmb_property region[CONFIG_LMB_MAX_REGIONS];
As the EFI sub-system can produce any number of non-coalescable memory
regions we should use a linked list instead.
The fields memory.max and reserved.max seem to be unused except for
test/lib/lmb.c.
lib/lmb.c:136: lmb->memory.max = CONFIG_LMB_MAX_REGIONS;
lib/lmb.c:139: lmb->memory.max = CONFIG_LMB_MEMORY_REGIONS;
test/lib/lmb.c:689: ut_asserteq(lmb.memory.max, CONFIG_LMB_MAX_REGIONS);
lib/lmb.c:137: lmb->reserved.max = CONFIG_LMB_MAX_REGIONS;
lib/lmb.c:140: lmb->reserved.max = CONFIG_LMB_RESERVED_REGIONS;
test/lib/lmb.c:691: ut_asserteq(lmb.reserved.max,
CONFIG_LMB_MAX_REGIONS);
Best regards
Heinrich
+#endif
+ .memory.cnt = 0,
+ .reserved.cnt = 0,
+};
+
static void lmb_dump_region(struct lmb_region *rgn, char *name)
{
unsigned long long base, size, end;
@@ -42,8 +61,8 @@ static void lmb_dump_region(struct lmb_region *rgn, char
*name)
void lmb_dump_all_force(struct lmb *lmb)
{
printf("lmb_dump_all:\n");
- lmb_dump_region(>memory, "memory");
- lmb_dump_region(>reserved, "reserved");
+ lmb_dump_region(, "memory");
+ lmb_dump_region(, "reserved");
}
[...]
Thanks
/Ilias
Re: [RFC PATCH 03/31] lmb: make the lmb reservations persistent
Hi Sughosh
[...]
> #define LMB_ALLOC_ANYWHERE 0
>
> +#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
> +struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS];
> +struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS];
> +#endif
> +
> +struct lmb lmb = {
> +#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
> + .memory.max = CONFIG_LMB_MAX_REGIONS,
> + .reserved.max = CONFIG_LMB_MAX_REGIONS,
> +#else
> + .memory.max = CONFIG_LMB_MEMORY_REGIONS,
> + .reserved.max = CONFIG_LMB_RESERVED_REGIONS,
> + .memory.region = memory_regions,
> + .reserved.region = reserved_regions,
This is probably a good opportunity to look into why
CONFIG_LMB_MEMORY_REGIONS was introduced. Since we are moving towards
static allocations, do we still need it? Or allocating the size dynamically
covers all our cases.
> +#endif
> + .memory.cnt = 0,
> + .reserved.cnt = 0,
> +};
> +
> static void lmb_dump_region(struct lmb_region *rgn, char *name)
> {
> unsigned long long base, size, end;
> @@ -42,8 +61,8 @@ static void lmb_dump_region(struct lmb_region *rgn, char
> *name)
> void lmb_dump_all_force(struct lmb *lmb)
> {
> printf("lmb_dump_all:\n");
> - lmb_dump_region(>memory, "memory");
> - lmb_dump_region(>reserved, "reserved");
> + lmb_dump_region(, "memory");
> + lmb_dump_region(, "reserved");
> }
>
[...]
Thanks
/Ilias
