Hi Sughosh,
On Mon, 15 Jul 2024 at 10:48, Sughosh Ganu <sughosh.g...@linaro.org> wrote:
>
> hi Simon,
>
> On Sat, 13 Jul 2024 at 20:46, Simon Glass <s...@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Thu, 4 Jul 2024 at 08:36, Sughosh Ganu <sughosh.g...@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.g...@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().
>
> 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?
>
> > 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.
OK ta.
Regards,
Simon
