[PATCH v2 01/18] abuf: Add a helper for initing and allocating a buffer

[Tom Rini]

On Sat, May 10, 2025 at 01:27:03PM +0200, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 6 May 2025 at 21:48, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Tue, May 06, 2025 at 03:23:39PM +0200, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 5 May 2025 at 22:38, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Thu, May 01, 2025 at 07:37:01AM -0600, Simon Glass wrote:
> > > >
> > > > > This construct appears in various places. Reduce code size by adding a
> > > > > function for it.
> > > > >
> > > > > It inits the abuf, then allocates it to the requested size.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > ---
> > > > >
> > > > > Changes in v2:
> > > > > - Add new patch with a helper for initing and allocating a buffer
> > > > >
> > > > >  boot/cedit.c          |  3 +--
> > > > >  boot/scene.c          |  3 +--
> > > > >  boot/scene_textline.c |  3 +--
> > > > >  include/abuf.h        | 11 +++++++++++
> > > > >  lib/abuf.c            |  9 +++++++++
> > > > >  lib/of_live.c         |  3 +--
> > > > >  test/lib/abuf.c       | 22 ++++++++++++++++++++++
> > > > >  7 files changed, 46 insertions(+), 8 deletions(-)
> > > >
> > > > This just made me look again at the abuf implementation itself and
> > > > become filled with regret I didn't reject it back in 2021. We're
> > > > introducing wrappers around standard functions and calling conventions /
> > > > patterns with something homegrown (and so not intuitive to others) that
> > > > mainly hides the "sysmem" challenge we also have  and I wish you were
> > > > interested in revisiting how that part of sandbox works instead. And
> > > > even if this is a better design, for the sake of argument, it's not
> > > > something everyone else is used to. And that's important.
> > > >
> > > > If we *really* need something different / new here, I'd rather see us go
> > > > and wrap common/dlmalloc.c with kmalloc/kfree/etc and so give people
> > > > something even more familiar-looking.
> > >
> > > The purpose of abuf is actually not about hiding sysmem - you can see
> > > that if you look at lib/abuf.c and the tests. It provides an easy way
> > > to manage buffers, which we do a lot in U-Boot, particularly with
> > > standard boot. It deals with whether the buffer is allocated or not,
> > > so freeing is easy. With expo I want to be able to manage a buffer
> > > containing an autoboot string in high-level code, say, without
> > > worrying whether it has to be realloced.
> >
> > It's a wrapper around free/malloc/et al, which then also includes the
> > sandbox-specific requirement of *sysmap* stuff. This is best shown by
> > the next two patches which convert from standard malloc/free patterns
> > (which both humans and analysis checkers are fond of) to a home grown
> > invention.
> 
> Yes, although I should point out that most of my code is a home-grown
> invention, if by that you mean it wasn't invented by someone else and
> then posted by me as if it were my own invention.

Well, I'm referring to the cases where solutions to the problems exist,
but you're inventing a new one instead.

> > > For sysmem, I don't see a viable alternative, which keeps can reliably
> > > keep addresses consistent across multiple phases (e.g. sandbox_vpl).
> > > Also, I don't believe addresses like 0x7ffff7fbc000 are
> > > human-friendly, yet that is what we would see in tests if we dropped
> > > the mapping. The point of sandbox is to test U-Boot, not expose the
> > > system internals. But we could discuss it if you like?
> >
> > I'm not sure how this is relevant to the general high level point of
> > "sandbox should be able to, in 2025, be designed so that special macros
> > do not need to be everywhere for the sanity checking it can do to
> > happen". If my web browser is going to suck up 11GiB of memory all the
> > time I can live with sandbox using 1/2/4/whatever GiB of memory when it
> > runs. The "this is not a pointer" problem sandbox can solve could be
> > done differently these days.
> 
> The special macros have several purposes:
> 1. Allow sandbox to have a contiguous region of memory at address 0,
> while the pointers are wherever the RAM buffer happens to end up
> 2. Provide an indication of whether code has been converted to sandbox
> yet, or not
> 3. Provide a way to mark all casts to/from address/pointers so it is
> clear that the cast is intentional
> 4. Ensure that sandbox seg-faults if a bad pointer is used
> 
> You seemed quite happy about it until my EFI test was rejected.

No, I've never really been happy with the macros and never understood
why we couldn't do the useful sanity checking wholly under arch/sandbox/

> > > In general I am sensing that there are a lot of things which bug you
> > > about the direction of U-Boot. and that you feel very differently
> > > about some things than you did in 2021. I have quite a few concerns
> > > too. So how about we make time to discuss these and see if we can get
> > > on the same page, at least somewhat?
> >
> > Yes, we've had numerous long threads, and a few calls. You are then
> > unhappy with what I say I want to see done, want me to just take your
> > changes instead and "Say Yes" more. I wonder if I had said No, or gather
> > more feedback from potential users, or just slow down, more back in 2021
> > if we would not be in a different position here.
> 
> I wonder if you had said yes more, if we might have got standard boot
> done a few years earlier and would not be stuck now on EFI bootmgr.

No, we probably would have just had more developers leave as I overrode
their objections.

> > > Coming back to this patch though, this is really about a code-side
> > > win, rather than anything else.
> >
> > No, it's about how similar to the "log a message and return" convention
> > change you wanted to make and then dropped, how I do not see good value
> > in changing from standard convention of malloc/free to abuf being a win.
> > Maybe this is a case where borrowing the kernel's mempool logic would be
> > a win instead? Because another part of this is that we want to avoid
> > making up our own APIs for things if we can instead borrow something
> > from the kernel, where most of the rest of the community will already be
> > familiar.
> 
> mempool is actually lmb, as I understand it. We haven't done that
> rename. It allocates from a region of pre-allocated memory. It doesn't
> use malloc(). So to me, that is quite different.

Then we should probably just drop the abuf thing and use standard
conventions that developers are used to.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20250512/66a5ea06/attachment.sig>