Hi Tom, On Sat, 24 May 2025 at 15:20, Tom Rini <tr...@konsulko.com> wrote: > > On Sat, May 24, 2025 at 08:51:43AM +0100, Simon Glass wrote: > > Hi Tom, > > > > On Wed, 14 May 2025 at 20:44, Simon Glass <s...@chromium.org> wrote: > > > > > > Hi Tom, > > > > > > On Tue, 13 May 2025 at 00:35, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > 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 <tr...@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 <tr...@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 <s...@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. > > > > > > No, abuf is a valuable abstraction which makes it easier to avoid > > > alloc/free errors, size errors, etc. Python has bytes, C++ has > > > std::byte. Linux has sk_buff which is a bit like membuf. Perhaps Linux > > > would benefit from abuf but its memory model and usage is quite a bit > > > different, so I'm not sure. > > > > This series seems to be blocked on a desire to remove abuf from > > U-Boot. Other than that I have a minor comment to resolve on the new > > 'bootdev order' command. So I sent a patch for that. > > Yes, I disagree with spreading the use of abuf. Whatever benefits it may > or may not have are outweighed by not following any normal conventions > the rest of the developers use.
Are you completely set on this? Regards, SImon
