[RFC PATCH v2 13/48] lmb: make LMB memory map persistent and global

[Sughosh Ganu]

hi Simon,

On Sat, 13 Jul 2024 at 20:46, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Sughosh,
>
> On Thu, 4 Jul 2024 at 08:36, Sughosh Ganu <sughosh.ganu at linaro.org> 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 using the alloced list data structure.
> >
> > Two alloced lists are declared -- one for the available(free) memory,
> > and one for the used memory. Once full, the list can then be extended
> > at runtime.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > ---
> > Changes since V1:
> > * Use alloced list structure for the available and reserved memory
> >   lists instead of static arrays.
> > * Corresponding changes in the code made as a result of the above
> >   change.
> > * Rename the reserved memory list as 'used'.
> >
> >  include/lmb.h |  77 +++--------
> >  lib/lmb.c     | 346 ++++++++++++++++++++++++++++++--------------------
> >  2 files changed, 224 insertions(+), 199 deletions(-)
> >
> > diff --git a/include/lmb.h b/include/lmb.h
> > index 99fcf5781f..27cdb18c37 100644
> > --- a/include/lmb.h
> > +++ b/include/lmb.h
> > @@ -24,78 +24,18 @@ enum lmb_flags {
> >  };
> >
> >  /**
> > - * struct lmb_property - Description of one region.
> > + * struct lmb_region - Description of one region.
> >   *
> >   * @base:      Base address of the region.
> >   * @size:      Size of the region
> >   * @flags:     memory region attributes
> >   */
> > -struct lmb_property {
> > +struct lmb_region {
> >         phys_addr_t base;
> >         phys_size_t size;
> >         enum lmb_flags flags;
> >  };
> >
> > -/*
> > - * For regions size management, see LMB configuration in KConfig
> > - * all the #if test are done with CONFIG_LMB_USE_MAX_REGIONS (boolean)
> > - *
> > - * case 1. CONFIG_LMB_USE_MAX_REGIONS is defined (legacy mode)
> > - *         => CONFIG_LMB_MAX_REGIONS is used to configure the region size,
> > - *         directly in the array lmb_region.region[], with the same
> > - *         configuration for memory and reserved regions.
> > - *
> > - * case 2. CONFIG_LMB_USE_MAX_REGIONS is not defined, the size of each
> > - *         region is configurated *independently* with
> > - *         => 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.
> > - */
> > -
> > -/**
> > - * struct lmb_region - Description of a set of region.
> > - *
> > - * @cnt: Number of regions.
> > - * @max: Size of the region array, max value of cnt.
> > - * @region: Array of the region properties
> > - */
> > -struct lmb_region {
> > -       unsigned long cnt;
> > -       unsigned long max;
> > -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
> > -       struct lmb_property region[CONFIG_LMB_MAX_REGIONS];
> > -#else
> > -       struct lmb_property *region;
> > -#endif
> > -};
> > -
> > -/**
> > - * 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.
> > - *
> > - * @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 reserved_regions[CONFIG_LMB_RESERVED_REGIONS];
> > -#endif
> > -};
> > -
> > -void lmb_init_and_reserve(struct bd_info *bd, void *fdt_blob);
> > -void lmb_init_and_reserve_range(phys_addr_t base, phys_size_t size,
> > -                               void *fdt_blob);
> >  long lmb_add(phys_addr_t base, phys_size_t size);
> >  long lmb_reserve(phys_addr_t base, phys_size_t size);
> >  /**
> > @@ -134,6 +74,19 @@ void board_lmb_reserve(void);
> >  void arch_lmb_reserve(void);
> >  void arch_lmb_reserve_generic(ulong sp, ulong end, ulong align);
> >
> > +/**
> > + * lmb_mem_regions_init() - Initialise the LMB memory
> > + *
> > + * Initialise the LMB subsystem related data structures. There are two
> > + * alloced lists that are initialised, one for the free memory, and one
> > + * for the used memory.
> > + *
> > + * Initialise the two lists as part of board init.
> > + *
> > + * Return: 0 if OK, -ve on failure.
> > + */
> > +int lmb_mem_regions_init(void);
> > +
> >  #endif /* __KERNEL__ */
> >
> >  #endif /* _LINUX_LMB_H */
> > diff --git a/lib/lmb.c b/lib/lmb.c
> > index 80945e3cae..a46bc8a7a3 100644
> > --- a/lib/lmb.c
> > +++ b/lib/lmb.c
> > @@ -6,6 +6,7 @@
> >   * Copyright (C) 2001 Peter Bergner.
> >   */
> >
> > +#include <alist.h>
> >  #include <efi_loader.h>
> >  #include <image.h>
> >  #include <mapmem.h>
> > @@ -15,24 +16,30 @@
> >
> >  #include <asm/global_data.h>
> >  #include <asm/sections.h>
> > +#include <linux/kernel.h>
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> >
> >  #define LMB_ALLOC_ANYWHERE     0
> > +#define LMB_ALIST_INITIAL_SIZE 4
> >
> > -static void lmb_dump_region(struct lmb_region *rgn, char *name)
> > +struct alist lmb_free_mem;
> > +struct alist lmb_used_mem;
>
> I think these should be in a struct, e.g. struct lmb, allocated with
> malloc() and pointed to by gd->lmb so we can avoid making the tests
> destructive, and allow use of lmb in SPL.

Can you elaborate on the point of allowing use of lmb in SPL. Why
would the current design not work in SPL ? I tested this on the
sandbox SPL variant, and the two lists do get initialised as part of
the SPL initialisation routine. Is there some corner-case that I am
not considering ?

Also, regarding putting the lmb structure pointer as part of gd, iirc
Tom had a different opinion on this. Tom, can you please chime in here
?

 > There is a
> arch_reset_for_test() which can reset the lmb back to its original
> state, or perhaps just swap the pointer for tests.
>
> I know this series is already long, but this patch seems to do quite a
> bit. Perhaps it could be split into two?

Sorry, do you mean the patch, or the series. If the later, I am going
to split the non-rfc version which comes next into two parts. The
first part being the lmb changes.

-sughosh

>
> Regards,
> Simon