hi Simon, On Mon, 10 Jun 2024 at 20:33, Simon Glass <s...@chromium.org> wrote: > > Hi Sughosh, > > On Mon, 10 Jun 2024 at 08:40, Sughosh Ganu <sughosh.g...@linaro.org> wrote: > > > > hi Simon, > > > > On Mon, 10 Jun 2024 at 19:25, Simon Glass <s...@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Thu, 6 Jun 2024 at 13:18, Sughosh Ganu <sughosh.g...@linaro.org> wrote: > > > > > > > > hi Simon, > > > > > > > > On Wed, 29 May 2024 at 22:00, Simon Glass <s...@chromium.org> wrote: > > > > > > > > > > +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? > > > > > > > > Thanks for sharing this with me. I have been working on a solution for > > > > this issue [1], and plan to send a rfc series by the end of this week. > > > > My primary motivation behind this work is to have some kind of > > > > synchronisation between the allocations done by the LMB module and the > > > > EFI subsystem. As you are aware, the way things stand today, either of > > > > the two can make memory allocations overwriting any earlier allocated > > > > memory. The primary reasons behind this is 1) the LMB allocations are > > > > private to their respective callers, so the LMB memory map is not > > > > global and persistent, and 2) the EFI allocation routines do not have > > > > any view of the allocations that would have been made by the LMB > > > > module. > > > > > > > > To fix these issues, I am working on a solution which 1) makes LMB > > > > memory map persistent and global, and 2) have notifications between > > > > the two modules when either of the two has made any changes to it's > > > > memory map. This way, it would be possible to prohibit one module from > > > > allocating memory which is in use. > > > > > > > > I am aware that we have a whole bunch of tests and user scripts which > > > > work with an assumption that the memory can be re-used. The solution > > > > that I would be proposing takes care of this assumption. Hopefully I > > > > should be in a position to send a rfc series this week. We can discuss > > > > on the patches. Thanks. > > > > > > I actually am not sure what we are on the same page here. I described > > > the problems in my original email but your series seems to solve > > > somewhat different problems, while IMO introducing a whole new class > > > of problems. > > > > Can you please describe in a little more detail what "whole new class > > of problems" are being introduced here. If there are problems being > > introduced, it is better if they are enumerated, so that I can try to > > fix them. > > Please see my point immediately below: > > > > > > > > > Making LMB persistent and global is a solution looking for a problem, > > > IMO. Can we go up a level and figure out exactly what is broken here? > > > That is what I attempted to do in my original email. Perhaps you could > > > reply to that and suggest where you agree / disagree? > > > > I have gone through this thread multiple times, and my understanding > > was that there was no conclusive agreement that was arrived at. I saw > > a reply from Tom where he broadly mentioned what kind of a solution he > > is looking for. I am not sure if you have gone through the > > cover-letter of my RFC series [1], which clearly states the problem my > > patch series is trying to fix. Like I said above, it would help if you > > can describe in a little more detail the kind of issues you see with > > the RFC series. Thanks. > > My request was for you to reply to the thread with any thoughts so we > can agree what to do about it. I believe my original email mostly > stands as a summary of where things are and what the problems are. If > it helps, I can update it based on the discussion above. It is > important to avoid over-complicating U-Boot. > > My fundamental expectation is that LMB reservations only apply for a > particular boot. They should not be global, nor need freeing, nor > should they be similar to memory allocations. We discussed it in this > thread.
Please note that I have run the patches through CI, and they pass the CI run. There are numerous examples in the CI test scripts where the LMB memory is re-used, and these patches are not changing that behaviour in any which way. The only reason why the LMB memory needs to be persistent and global is because it needs to work cohesively with the EFI memory allocator. At this point, I guess the EFI maintainers and Tom have to provide their input on how we go about this. But if we have to have EFI allocations live alongside LMB, the *only* other solution that I see is to then break up the memory into two sections, one for LMB and one for EFI. But then again, that introduces it's own set of issues, for e.g. a platform might not be able to use some memory region simply because it is allocated to the other subsystem. But I would let the maintainers chime in at this point. Thanks. -sughosh > > Regards, > Simon
