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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 8732 bytes --]

On Mon, Jul 15, 2024 at 12:39:35PM +0100, Simon Glass wrote:
> Hi Sughosh,
> 
> On Mon, 15 Jul 2024 at 10:48, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Sat, 13 Jul 2024 at 20:46, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Thu, 4 Jul 2024 at 08:36, Sughosh Ganu <sughosh.ganu@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@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 ?
> 
> Just that some boards don't have their BSS available until later on in
> SPL. In general we try to avoid local variables with driver model...I
> think lmb should be the same, particularly as there it is fairly cheap
> to allocate a struct with malloc().

We also have limited malloc space, in those cases. But, yes, we likely
need LMB available to use earlier in the SPL case now than we did
before, so malloc is likely better, as Simon suggests.

> > 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
> > ?
> 
> or you could point to the discussion?

It was in reply to the last posting of this series.

While there's a strong implication that tests in post/ (as it is short
for 'power on self test') need to be non-destructive, that's not the
case for test/ code.

So my thoughts are:
- Since there's only one list for real use, calls are cleaner if we
  aren't passing a pointer to the one and only thing everyone could use.
- I don't think we have a test case that's hindered by not doing this,
  only "now boot normally" may be harder/hindered.
- Taking things out of gd is usually good?

At the end of the day, if this is a big sticking point with the new
scheme, no, OK, we can go back to gd->lmb. I don't think we need it, but
I can't convince myself Everyone Else Is Wrong about it either.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]