+Sughosh Ganu for reference
On Sun, 31 Dec 2023 at 09:16, Tom Rini <tr...@konsulko.com> wrote: > > On Sun, Dec 31, 2023 at 04:40:06PM +0100, Heinrich Schuchardt wrote: > > > > > > Am 31. Dezember 2023 16:11:44 MEZ schrieb Tom Rini <tr...@konsulko.com>: > > >On Sun, Dec 31, 2023 at 07:22:10AM -0700, Simon Glass wrote: > > >> Hi Tom, > > >> > > >> On Sun, Dec 31, 2023 at 6:54 AM Tom Rini <tr...@konsulko.com> wrote: > > >> > > > >> > On Sun, Dec 31, 2023 at 05:48:23AM -0700, Simon Glass wrote: > > >> > > Hi, > > >> > > > > >> > > On Fri, Dec 29, 2023 at 10:52 AM Tom Rini <tr...@konsulko.com> wrote: > > >> > > > > > >> > > > On Fri, Dec 29, 2023 at 06:44:15PM +0100, Mark Kettenis wrote: > > >> > > > > > Date: Fri, 29 Dec 2023 11:17:44 -0500 > > >> > > > > > From: Tom Rini <tr...@konsulko.com> > > >> > > > > > > > >> > > > > > On Fri, Dec 29, 2023 at 05:05:17PM +0100, Heinrich Schuchardt > > >> > > > > > wrote: > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > Am 29. Dezember 2023 16:43:07 MEZ schrieb Tom Rini > > >> > > > > > > <tr...@konsulko.com>: > > >> > > > > > > >On Fri, Dec 29, 2023 at 05:36:09AM +0000, Simon Glass wrote: > > >> > > > > > > >> Hi, > > >> > > > > > > >> > > >> > > > > > > >> On Sat, Dec 16, 2023 at 6:01 PM Simon Glass > > >> > > > > > > >> <s...@chromium.org> wrote: > > >> > > > > > > >> > > > >> > > > > > > >> > Hi, > > >> > > > > > > >> > > > >> > > > > > > >> > This records my thoughts after a discussion with Ilias > > >> > > > > > > >> > & Heinrich re > > >> > > > > > > >> > memory allocation in U-Boot. > > >> > > > > > > >> > > > >> > > > > > > >> > 1. malloc() > > >> > > > > > > >> > > > >> > > > > > > >> > malloc() is used for programmatic memory allocation. It > > >> > > > > > > >> > allows memory > > >> > > > > > > >> > to be freed. It is not designed for very large > > >> > > > > > > >> > allocations (e.g. a > > >> > > > > > > >> > 10MB kernel or 100MB ramdisk). > > >> > > > > > > >> > > > >> > > > > > > >> > 2. lmb > > >> > > > > > > >> > > > >> > > > > > > >> > lmb is used for large blocks of memory, such as those > > >> > > > > > > >> > needed for a > > >> > > > > > > >> > kernel or ramdisk. Allocation is only transitory, for > > >> > > > > > > >> > the purposes of > > >> > > > > > > >> > loading some images and booting. If the boot fails, > > >> > > > > > > >> > then all lmb > > >> > > > > > > >> > allocations go away. > > >> > > > > > > >> > > > >> > > > > > > >> > lmb is set up by getting all available memory and then > > >> > > > > > > >> > removing what > > >> > > > > > > >> > is used by U-Boot (code, data, malloc() space, etc.) > > >> > > > > > > >> > > > >> > > > > > > >> > lmb reservations have a few flags so that areas of > > >> > > > > > > >> > memory can be > > >> > > > > > > >> > provided with attributes > > >> > > > > > > >> > > > >> > > > > > > >> > There are some corner cases...e.g. loading a file does > > >> > > > > > > >> > an lmb > > >> > > > > > > >> > allocation but only for the purpose of avoiding a file > > >> > > > > > > >> > being loaded > > >> > > > > > > >> > over U-Boot code/data. The allocation is dropped > > >> > > > > > > >> > immediately after the > > >> > > > > > > >> > file is loaded. Within the bootm command, or when using > > >> > > > > > > >> > standard boot, > > >> > > > > > > >> > this would be fairly easy to solve. > > >> > > > > > > >> > > > >> > > > > > > >> > Linux has renamed lmb to memblock. We should consider > > >> > > > > > > >> > doing the same. > > >> > > > > > > >> > > > >> > > > > > > >> > 3. EFI > > >> > > > > > > >> > > > >> > > > > > > >> > EFI has its own memory-allocation tables. > > >> > > > > > > >> > > > >> > > > > > > >> > Like lmb, EFI is able to deal with large allocations. > > >> > > > > > > >> > But via a 'pool' > > >> > > > > > > >> > function it can also do smaller allocations similar to > > >> > > > > > > >> > malloc(), > > >> > > > > > > >> > although each one uses at least 4KB at present. > > >> > > > > > > >> > > > >> > > > > > > >> > EFI allocations do not go away when a boot fails. > > >> > > > > > > >> > > > >> > > > > > > >> > With EFI it is possible to add allocations post facto, > > >> > > > > > > >> > in which case > > >> > > > > > > >> > they are added to the allocation table just as if the > > >> > > > > > > >> > memory was > > >> > > > > > > >> > allocated with EFI to begin with. > > >> > > > > > > >> > > > >> > > > > > > >> > The EFI allocations and the lmb allocations use the > > >> > > > > > > >> > same memory, so in > > >> > > > > > > >> > principle could conflict. > > >> > > > > > > >> > > > >> > > > > > > >> > EFI allocations are sometimes used to allocate internal > > >> > > > > > > >> > U-Boot data as > > >> > > > > > > >> > well, if needed by the EFI app. For example, while > > >> > > > > > > >> > efi_image_parse() > > >> > > > > > > >> > uses malloc(), efi_var_mem.c uses EFI allocations since > > >> > > > > > > >> > the code runs > > >> > > > > > > >> > in the app context and may need to access the memory > > >> > > > > > > >> > after U-Boot has > > >> > > > > > > >> > exited. Also efi_smbios.c uses allocate_pages() and > > >> > > > > > > >> > then adds a new > > >> > > > > > > >> > mapping as well. > > >> > > > > > > >> > > > >> > > > > > > >> > EFI memory has attributes, including what the memory is > > >> > > > > > > >> > used for (to > > >> > > > > > > >> > some degree of granularity). See enum efi_memory_type > > >> > > > > > > >> > and struct > > >> > > > > > > >> > efi_mem_desc. In the latter there are also attribute > > >> > > > > > > >> > flags - whether > > >> > > > > > > >> > memory is cacheable, etc. > > >> > > > > > > >> > > > >> > > > > > > >> > EFI also has the x86 idea of 'conventional' memory, > > >> > > > > > > >> > meaning (I > > >> > > > > > > >> > believe) that below 4GB that isn't reserved for the > > >> > > > > > > >> > hardware/system. > > >> > > > > > > >> > This is meaningless, or at least confusing, on ARM > > >> > > > > > > >> > systems. > > >> > > > > > > >> > > > >> > > > > > > >> > 4. reservations > > >> > > > > > > >> > > > >> > > > > > > >> > It is perhaps worth mentioning a fourth method of > > >> > > > > > > >> > memory management, > > >> > > > > > > >> > where U-Boot reserves chunks of memory before > > >> > > > > > > >> > relocation (in > > >> > > > > > > >> > board_init_f.c), e.g. for the framebuffer, U-Boot code, > > >> > > > > > > >> > the malloc() > > >> > > > > > > >> > region, etc. > > >> > > > > > > >> > > > >> > > > > > > >> > > > >> > > > > > > >> > Problems > > >> > > > > > > >> > —------- > > >> > > > > > > >> > > > >> > > > > > > >> > There are no urgent problems, but here are some things > > >> > > > > > > >> > that could be improved: > > >> > > > > > > >> > > > >> > > > > > > >> > 1. EFI should attach most of its data structures to > > >> > > > > > > >> > driver model. This > > >> > > > > > > >> > work has started, with the partition support, but more > > >> > > > > > > >> > effort would > > >> > > > > > > >> > help. This would make it easier to see which memory is > > >> > > > > > > >> > related to > > >> > > > > > > >> > devices and which is separate. > > >> > > > > > > >> > > > >> > > > > > > >> > 2. Some drivers do EFI reservations today, whether EFI > > >> > > > > > > >> > is used for > > >> > > > > > > >> > booting or not (e.g. rockchip video rk_vop_probe()). > > >> > > > > > > >> > > > >> > > > > > > >> > 3. U-Boot doesn't really map arch-specific memory > > >> > > > > > > >> > attributes (e.g. > > >> > > > > > > >> > armv8's struct mm_region) to EFI ones. > > >> > > > > > > >> > > > >> > > > > > > >> > 4. EFI duplicates some code from bootm, some of which > > >> > > > > > > >> > relates to > > >> > > > > > > >> > memory allocation (e.g. FDT fixup). > > >> > > > > > > >> > > > >> > > > > > > >> > 5. EFI code is used even if EFI is never used to boot > > >> > > > > > > >> > > > >> > > > > > > >> > 6. EFI allocations can result in the same memory being > > >> > > > > > > >> > used as has > > >> > > > > > > >> > already been allocated by lmb. Users may load files > > >> > > > > > > >> > which overwrite > > >> > > > > > > >> > memory allocated by EFI. > > >> > > > > > > >> > > >> > > > > > > >> 7. We need to support doing an allocation when a file is > > >> > > > > > > >> loaded (to > > >> > > > > > > >> ensure files do not overlap), without making it too > > >> > > > > > > >> difficult to load > > >> > > > > > > >> multiple files to the same place, if desired. > > >> > > > > > > >> > > >> > > > > > > >> > > > >> > > > > > > >> > > > >> > > > > > > >> > Lifetime > > >> > > > > > > >> > -------- > > >> > > > > > > >> > > > >> > > > > > > >> > We have three different memory allocators with > > >> > > > > > > >> > different purposes. Can > > >> > > > > > > >> > we unify them a little? > > >> > > > > > > >> > > > >> > > > > > > >> > Within U-Boot: > > >> > > > > > > >> > - malloc() space lives forever > > >> > > > > > > >> > - lmb lives while setting out images for booting > > >> > > > > > > >> > - EFI (mostly) lives while booting an EFI app > > >> > > > > > > >> > > > >> > > > > > > >> > In practice, EFI is set up early in U-Boot. Some of > > >> > > > > > > >> > this is necessary, > > >> > > > > > > >> > some not. EFI allocations stay around forever. This > > >> > > > > > > >> > works OK since > > >> > > > > > > >> > large allocations are normally not done in EFI, so > > >> > > > > > > >> > memory isn't really > > >> > > > > > > >> > consumed to any great degree by the boot process. > > >> > > > > > > >> > > > >> > > > > > > >> > What happens to EFI allocations if the app returns? > > >> > > > > > > >> > They are still > > >> > > > > > > >> > present, in case another app is run. This seems fine. > > >> > > > > > > >> > > > >> > > > > > > >> > API > > >> > > > > > > >> > –-- > > >> > > > > > > >> > Can we unify some APIs? > > >> > > > > > > >> > > > >> > > > > > > >> > It should be possible to use lmb for large EFI memory > > >> > > > > > > >> > allocations, so > > >> > > > > > > >> > long as they are only needed for booting. We > > >> > > > > > > >> > effectively do this > > >> > > > > > > >> > today, since EFI does not manage the arrangement of > > >> > > > > > > >> > loaded images in > > >> > > > > > > >> > memory. for the most part. > > >> > > > > > > >> > > > >> > > > > > > >> > It would not make sense to use EFI allocation to > > >> > > > > > > >> > replace lmb and > > >> > > > > > > >> > malloc(), of course. > > >> > > > > > > >> > > > >> > > > > > > >> > Could we use a common (lower-level) API for allocation, > > >> > > > > > > >> > used by both > > >> > > > > > > >> > lmb and EFI? They do have some similarities. However > > >> > > > > > > >> > they have > > >> > > > > > > >> > different lifetime constraints (EFI allocations are > > >> > > > > > > >> > never dropped, > > >> > > > > > > >> > unlikely lmb). > > >> > > > > > > >> > > > >> > > > > > > >> > ** Overall, it seems that the existence of memory > > >> > > > > > > >> > allocation in > > >> > > > > > > >> > boot-time services has created confusion. Memory > > >> > > > > > > >> > allocation is > > >> > > > > > > >> > muddled, with both U-Boot code and boot-time services > > >> > > > > > > >> > calling the same > > >> > > > > > > >> > memory allocator. This just has not been clearly > > >> > > > > > > >> > thought out. > > >> > > > > > > >> > > > >> > > > > > > >> > > > >> > > > > > > >> > Proposal > > >> > > > > > > >> > —------- > > >> > > > > > > >> > > > >> > > > > > > >> > Here are some ideas: > > >> > > > > > > >> > > > >> > > > > > > >> > 1. For video, use the driver model API to locate the > > >> > > > > > > >> > video regions, or > > >> > > > > > > >> > block off the entire framebuffer memory, for all > > >> > > > > > > >> > devices as a whole. > > >> > > > > > > >> > Use efi_add_memory_map() > > >> > > > > > > >> > > > >> > > > > > > >> > 2. Add memory attributes to UCLASS_RAM and use them in > > >> > > > > > > >> > EFI, mapping to > > >> > > > > > > >> > the EFI_MEMORY_... attributes in struct efi_mem_desc. > > >> > > > > > > >> > > > >> > > > > > > >> > 3. Add all EFI reservations just before booting the > > >> > > > > > > >> > app, as we do with > > >> > > > > > > >> > devicetree fixup. With this model, malloc() and lmb are > > >> > > > > > > >> > used for all > > >> > > > > > > >> > allocation. Then efi_add_memory_map() is called for > > >> > > > > > > >> > each region in > > >> > > > > > > >> > turn just before booting. Memory attributes are dealt > > >> > > > > > > >> > with above. The > > >> > > > > > > >> > type (enum efi_memory_type) can be determined simply by > > >> > > > > > > >> > the data > > >> > > > > > > >> > structure stored in it, as is done today. For example, > > >> > > > > > > >> > SMBIOS tables > > >> > > > > > > >> > can use EFI_ACPI_RECLAIM_MEMORY. Very few types are > > >> > > > > > > >> > used and EFI code > > >> > > > > > > >> > understands the meaning of each. > > >> > > > > > > >> > > > >> > > > > > > >> > 4. Avoid setting up EFI memory at the start of U-Boot. > > >> > > > > > > >> > Do it only when > > >> > > > > > > >> > booting. This looks to require very little effort. > > >> > > > > > > >> > > > >> > > > > > > >> > 5. Avoid calling efi_allocate_pages() and > > >> > > > > > > >> > efi_allocate_pool() outside > > >> > > > > > > >> > boot-time services. This solves the problem 6. If > > >> > > > > > > >> > memory is needed by > > >> > > > > > > >> > an app, allocate it with malloc() and see 3. There are > > >> > > > > > > >> > only two > > >> > > > > > > >> > efi_allocate_pages() (smbios and efi_runtime). There > > >> > > > > > > >> > are more calls of > > >> > > > > > > >> > efi_allocate_pool(), but most of these seem easy to fix > > >> > > > > > > >> > up. For > > >> > > > > > > >> > example, efi_init_event_log() allocates a buffer, but > > >> > > > > > > >> > this can be > > >> > > > > > > >> > allocated in normal malloc() space or in a bloblist. > > >> > > > > > > >> > > > >> > > > > > > >> > 6. Don't worry too much about whether EFI will be used > > >> > > > > > > >> > for booting. > > >> > > > > > > >> > The cost is likely not that great: use bootstage to > > >> > > > > > > >> > measure it as is > > >> > > > > > > >> > done for driver model. Try to minmise the cost of its > > >> > > > > > > >> > tables, > > >> > > > > > > >> > particularly for execution time, but otherwise just > > >> > > > > > > >> > rely on the > > >> > > > > > > >> > ability to disable EFI_LOADER. > > >> > > > > > > >> > > >> > > > > > > >> 7. Add a flag to the 'load' command: > > >> > > > > > > >> > > >> > > > > > > >> -m <type> - make an lmb allocation for the file > > >> > > > > > > >> <type> is the image type to use (kernel, ramdisk, > > >> > > > > > > >> flat_dt) > > >> > > > > > > >> > > >> > > > > > > >> any existing allocation for that type will be > > >> > > > > > > >> automatically freed > > >> > > > > > > >> first. If <type> is "none" then no freeing is possible: > > >> > > > > > > >> any loaded > > >> > > > > > > >> images just stack up in lmb. > > >> > > > > > > >> > > >> > > > > > > >> Add an 'lmb' (or memblock) command to allow listing and > > >> > > > > > > >> clearing allocations. > > >> > > > > > > > > > >> > > > > > > >I would really not like to change the user interface and > > >> > > > > > > >instead simply > > >> > > > > > > >handle this with flags to whatever mark/allocation function > > >> > > > > > > >is called. > > >> > > > > > > >You can always overwrite things that are brought in to > > >> > > > > > > >memory, you > > >> > > > > > > >cannot overwrite U-Boot or our internals. Optionally noting > > >> > > > > > > >that some > > >> > > > > > > >previous load to memory has been at least partially > > >> > > > > > > >overwritten could be > > >> > > > > > > >helpful, if it's not too much extra logic. > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > In most use cases users load exactly one file at each > > >> > > > > > > address. An > > >> > > > > > > unload command would be the cleanest way for a user to > > >> > > > > > > indicate that > > >> > > > > > > he wants to reuse the memory. > > >> > > > > > > > >> > > > > > I very much do not want to change the API. There's untold > > >> > > > > > numbers of > > >> > > > > > scripts out there and they should continue to work. I > > >> > > > > > mentioned to Ilias > > >> > > > > > off list just now that I'm not against adding a command to add > > >> > > > > > flags to > > >> > > > > > these areas, but I don't think it's worthwhile to prevent > > >> > > > > > overwrites the > > >> > > > > > user did early. The biggest long running problem in this space > > >> > > > > > was that > > >> > > > > > for 32bit ARM we couldn't know where the kernel BSS was going > > >> > > > > > to be and > > >> > > > > > so would have ramdisk at the wrong spot and get partially > > >> > > > > > eaten, and > > >> > > > > > this was hard to figure out. The current example is "ooops, > > >> > > > > > decompression buffer for Image.gz/etc is too close to other > > >> > > > > > things" > > >> > > > > > which ends up failing nice and loudly, and in the future once > > >> > > > > > this > > >> > > > > > proposal is done we can just dynamically find and use a spot, > > >> > > > > > since > > >> > > > > > we'll have that ability finally. > > >> > > > > > > >> > > > > In order to keep the existing interfaces we need lmb to keep > > >> > > > > track of > > >> > > > > (at least ) three different states. I think of those as "free", > > >> > > > > "allocated" and "reserved". The load command would "reserve" > > >> > > > > memory > > >> > > > > insteaf "allocate". And it would pass a flag to lmb when > > >> > > > > reserving > > >> > > > > memory to indicate that reserving memory that is already > > >> > > > > reserved is > > >> > > > > ok. Both "reserved" and "allocated" memory should show up as > > >> > > > > not free > > >> > > > > in the EFI memory map (probably as EfiLoaderData). > > >> > > > > > >> > > > Yes, something like this is what I was getting at, thanks. > > >> > > > > >> > > Yes, that is a good way of putting it. There is definitely a > > >> > > distinction there. > > >> > > > > >> > > If we don't want this flag, we could make U-Boot always do a > > >> > > reservation on load, with a '-f' command to force loading over an > > >> > > existing reservation / releasing it first? > > >> > > > >> > Again, this is an API change and I don't want to change the API. > > >> > > >> The flag is only needed to drop a reservation, since we apparently > > >> want the 'load' command to create a permanent reservation. It should > > >> not affect existing boot scripts since they won't load overlapping > > >> images. > > >> > > >> Anyway, what do you suggest? > > > > > >That "load" (and sf read and nand read and tftp and wget and ...) > > >"reserve" memory but not "allocate" memory and "reserve" means something > > >is there and "allocate" means that it can't be modified again. For > > >example, running U-Boot and our malloc pool are "allocated" but just > > >loading a file to memory is "reserved". And then yes, I can see use for > > >the command where some cases might want to "reserve" memory to fiddle > > >with it and then "allocate" it so something else can't change it. > > > > > >This is one of those cases where english is terrible to discuss things > > >in as both reserve and allocate can mean similar things. > > > > > > > I have no clue what the semantics of the mentioned "reserved" state might > > be. Up to now memory reservations designated address ranges that U-Boot > > must not use at all, e.g. the memory used by OpenSBI or TF-A. > > > > Who can and who cannot write into "reserved" memory? > > > > What is wrong about allocating memory for files to forbid any other use > > until you are done with the file and free the memory? > > So, historically (lets say mid 2010s), you could use "load" to bring a > file in to memory, anywhere, and it could even overwrite part of U-Boot > (running, or malloc pool or whatever). You could also use "load" to > bring a file in to memory and then bring another file in to that same > location in memory for whatever reason (assorted development cases). > > Then later someone noted that using "load" to overwrite U-Boot should > get a CVE and instead of ignoring it we decided to use "lmb" to try and > make sure that we couldn't use "load" to overwrite U-Boot itself, and > that that's a pre-check. This still allows overwriting a previously > loaded file in memory. > > Overwriting something the user put in memory is part of the ABI and has > some use cases. > > So for whatever future system we setup, a memory location can be: > - Free. > - Readable but not Writable. > - Readable and Writable. > > Something like $loadaddr starts as Free. If someone then uses "load" to > bring in an OS image, it's now "Readable and Writable". But if someone > does an API call to ask for a new region memory, it wouldn't return > $loadaddr because it's not Free. On the other hand, $relocaddr where > U-Boot is, at least in terms of the API is that it's "Readable but not > Writable". > > Does that help? > > -- > Tom
