[PATCH 23/40] lmb: add a flags parameter to the API's
Simon Glass
sjg at chromium.org
Thu Aug 8 16:28:44 CEST 2024
Hi Sughosh,
On Wed, 7 Aug 2024 at 00:32, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> On Wed, 7 Aug 2024 at 03:21, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Mon, 5 Aug 2024 at 05:55, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > >
> > > On Mon, 29 Jul 2024 at 20:56, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Mon, 29 Jul 2024 at 02:40, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > >
> > > > > On Fri, 26 Jul 2024 at 05:02, Simon Glass <sjg at chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Wed, 24 Jul 2024 at 00:04, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > > >
> > > > > > > Add a flags parameter to the LMB API functions. The parameter can then
> > > > > > > be used to pass any other type of reservations or allocations needed
> > > > > > > by the callers. These will be used in a subsequent set of changes for
> > > > > > > allocation requests coming from the EFI subsystem.
> > > > > > >
> > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > > > ---
> > > > > > > Changes since rfc: New patch
> > > > > > >
> > > > > > > arch/arm/mach-apple/board.c | 17 ++--
> > > > > > > arch/arm/mach-snapdragon/board.c | 2 +-
> > > > > > > arch/arm/mach-stm32mp/dram_init.c | 4 +-
> > > > > > > arch/powerpc/cpu/mpc85xx/mp.c | 2 +-
> > > > > > > arch/powerpc/lib/bootm.c | 2 +-
> > > > > > > board/xilinx/common/board.c | 4 +-
> > > > > > > boot/bootm.c | 5 +-
> > > > > > > boot/image-board.c | 15 ++-
> > > > > > > boot/image-fdt.c | 15 +--
> > > > > > > cmd/booti.c | 2 +-
> > > > > > > cmd/bootz.c | 2 +-
> > > > > > > cmd/load.c | 4 +-
> > > > > > > drivers/iommu/apple_dart.c | 6 +-
> > > > > > > drivers/iommu/sandbox_iommu.c | 6 +-
> > > > > > > fs/fs.c | 2 +-
> > > > > > > include/lmb.h | 23 ++---
> > > > > > > lib/lmb.c | 48 ++++------
> > > > > > > test/lib/lmb.c | 150 +++++++++++++++---------------
> > > > > > > 18 files changed, 150 insertions(+), 159 deletions(-)
> > > > > >
> > > > > > This negates any code-size advantage of dropping the lmb parameter.
> > > > > >
> > > > > > All of these are LMB_NONE. Can we have a separate function (e.g.
> > > > > > lmb_alloc_type()) for when we actually need to specify the type?
> > > > >
> > > > > We will be passing different values when we call the LMB API's from
> > > > > the EFI allocation function. This is only adding a parameter to the
> > > > > allocation API's, which I believe is better than adding separate
> > > > > functions which take a flag parameter only to be called from the EFI
> > > > > subsystem.
> > > >
> > > > No i believe it is worse, unless there are a lot of such functions.
> > > > The flags are a special case, not the common case.
> > >
> > > I have done some size impact tests on the two scenarios, one where we
> > > have a common set of lmb allocation API functions, with an added flags
> > > parameter, and second where we have separate API's to be called from
> > > the EFI memory module. I have put out the results of the size impact
> > > [1].
> > >
> > > You will see that with common API's, we are not losing much even on
> > > boards with EFI_LOADER disabled. But otoh, on boards which have
> > > EFI_LOADER enabled, the gains are pretty significant. I believe we
> > > should reconsider using a common LMB API with the flags parameter.
> >
> > Thanks for looking at it.
> >
> > Did you do special versions of just lmb_alloc() and lmb_add() which
> > call the flags versions? It seems that there is no size advantage with
> > EFI_LOADER and only a small one with !EFI_LOADER. Can you please point
> > me to the code?
>
> For the separate API version, I introduced new versions
> lmb_alloc_flags(), lmb_alloc_base_flags(), lmb_alloc_addr_flags() and
> lmb_free_flags(), which are being called from the EFI memory module. I
> have pushed the two branches [1] [2] on my github. Please take a look.
>
> Btw, both these branches are based on your v5 of the alist patches,
> and also incorporate the stack based implementation for running the
> lmb tests. So except for either having common API's, or not, there are
> no other differences between the two. Thanks.
Thanks for the info.
The non-flags functions can call the flags functions, so that you
don't create a new code path. Something like this:
diff --git a/lib/lmb.c b/lib/lmb.c
index 726e6c38227..0a251c587fe 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -528,7 +528,7 @@ long lmb_free_flags(phys_addr_t base, phys_size_t size,
long lmb_free(phys_addr_t base, phys_size_t size)
{
- return __lmb_free(base, size);
+ return lmb_free_flags(base, size, LMB_NONE);
}
long lmb_reserve_flags(phys_addr_t base, phys_size_t size, enum
lmb_flags flags)
@@ -624,7 +624,7 @@ static phys_addr_t __lmb_alloc_base(phys_size_t
size, ulong align,
phys_addr_t lmb_alloc(phys_size_t size, ulong align)
{
- return lmb_alloc_base(size, align, LMB_ALLOC_ANYWHERE);
+ return lmb_alloc_flags(size, align, LMB_NONE);
}
phys_addr_t lmb_alloc_flags(phys_size_t size, ulong align, uint flags)
@@ -635,15 +635,7 @@ phys_addr_t lmb_alloc_flags(phys_size_t size,
ulong align, uint flags)
phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr)
{
- phys_addr_t alloc;
-
- alloc = __lmb_alloc_base(size, align, max_addr, LMB_NONE);
-
- if (alloc == 0)
- printf("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n",
- (ulong)size, (ulong)max_addr);
-
- return alloc;
+ return lmb_alloc_base_flags(size, align, max_addr, LMB_NONE);
}
phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align,
@@ -691,7 +683,7 @@ static phys_addr_t __lmb_alloc_addr(phys_addr_t
base, phys_size_t size,
*/
phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size)
{
- return __lmb_alloc_addr(base, size, LMB_NONE);
+ return lmb_alloc_addr_flags(base, size, LMB_NONE);
}
phys_addr_t lmb_alloc_addr_flags(phys_addr_t base, phys_size_t size,
But it only saves about 40 bytes on Thumb2. You can save another 16 by
using the placeholder API:
diff --git a/lib/lmb.c b/lib/lmb.c
index 0a251c587fe..f6c8f06629c 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -416,13 +416,12 @@ static long lmb_add_region_flags(struct alist
*lmb_rgn_lst, phys_addr_t base,
if (coalesced)
return coalesced;
- if (alist_full(lmb_rgn_lst) &&
- !alist_expand_by(lmb_rgn_lst, lmb_rgn_lst->alloc))
+ if (!alist_add_placeholder(lmb_rgn_lst))
return -1;
rgn = lmb_rgn_lst->data;
/* Couldn't coalesce the LMB, so add it to the sorted table. */
- for (i = lmb_rgn_lst->count; i >= 0; i--) {
+ for (i = lmb_rgn_lst->count - 1; i >= 0; i--) {
if (i && base < rgn[i - 1].base) {
rgn[i] = rgn[i - 1];
} else {
@@ -433,8 +432,6 @@ static long lmb_add_region_flags(struct alist
*lmb_rgn_lst, phys_addr_t base,
}
}
- lmb_rgn_lst->count++;
-
return 0;
}
@@ -444,7 +441,6 @@ static long lmb_add_region(struct alist
*lmb_rgn_lst, phys_addr_t base,
return lmb_add_region_flags(lmb_rgn_lst, base, size, LMB_NONE);
}
-/* This routine may be called with relocation disabled. */
long lmb_add(phys_addr_t base, phys_size_t size)
{
long ret;
--
2.34.1
(patches are a bit rough, but I didn't think it worth sending them to
the ML as real patches)
If I am correct and we don't need to publish events, then that will
save a little more space.
Regards,
Simon
>
> -sughosh
>
> [1] - https://github.com/sughoshg/u-boot/tree/lmb_efi_common_apis_nrfc_v2
> [2] - https://github.com/sughoshg/u-boot/tree/lmb_efi_separate_flags_apis_nrfc_v2
>
> >
> > Regards,
> > Simon
> >
> >
> > >
> > > [1] - https://gist.github.com/sughoshg/a20207f26e19238fef86f710134d6efd
> > >
> > > >
> > > > Regards,
> > > > SImon
More information about the U-Boot
mailing list