On Thu, 8 Aug 2024 at 19:58, Simon Glass <s...@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Wed, 7 Aug 2024 at 00:32, Sughosh Ganu <sughosh.g...@linaro.org> wrote:
> >
> > On Wed, 7 Aug 2024 at 03:21, Simon Glass <s...@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Mon, 5 Aug 2024 at 05:55, Sughosh Ganu <sughosh.g...@linaro.org> wrote:
> > > >
> > > > On Mon, 29 Jul 2024 at 20:56, Simon Glass <s...@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Mon, 29 Jul 2024 at 02:40, Sughosh Ganu <sughosh.g...@linaro.org>
> > > > > wrote:
> > > > > >
> > > > > > On Fri, 26 Jul 2024 at 05:02, Simon Glass <s...@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Sughosh,
> > > > > > >
> > > > > > > On Wed, 24 Jul 2024 at 00:04, Sughosh Ganu
> > > > > > > <sughosh.g...@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.g...@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:
Can you please explain the issue that you see with having a common set
of API's with the flags parameter added? The way I see it, the API's
are already undergoing a change where we are removing the struct lmb
instance as a parameter from all the API functions. With the change
that I propose, we are simply replacing the lmb instance parameter
with the flags parameter. So arguably we are not adding any additional
size here. Also, like the size tests show, we get a pretty good size
benefit when the EFI_LOADER is enabled.
So, if your argument is to keep the API's similar to their earlier
form, I think that they are undergoing a change in any case, so adding
the flags parameter is not so much of an issue. If there is any
problem that I am missing, I would like to understand that before we
go with separate API's. Thanks.
-sughosh
>
> 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
