On Fri, Dec 29, 2023 at 06:09:44PM +0100, Heinrich Schuchardt wrote: > On 12/29/23 17:47, Tom Rini wrote: > > On Fri, Dec 29, 2023 at 05:42:17PM +0100, Heinrich Schuchardt wrote: > > > On 12/20/23 20:12, Tom Rini wrote: > > > > On Tue, Dec 19, 2023 at 09:15:21PM -0700, Simon Glass wrote: > > > > > Hi, > > > > > > > > > > On Tue, 19 Dec 2023 at 05:46, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > > > > > On Tue, Dec 19, 2023 at 03:15:38AM +0100, Heinrich Schuchardt wrote: > > > > > > > > > > > > > > > > > > > > > Am 19. Dezember 2023 02:26:00 MEZ schrieb Tom Rini > > > > > > > <tr...@konsulko.com>: > > > > > > > > On Tue, Dec 19, 2023 at 01:01:51AM +0100, Heinrich Schuchardt > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > Am 19. Dezember 2023 00:31:30 MEZ schrieb Tom Rini > > > > > > > > > <tr...@konsulko.com>: > > > > > > > > > > On Tue, Dec 19, 2023 at 12:29:19AM +0100, Heinrich > > > > > > > > > > Schuchardt wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Am 19. Dezember 2023 00:16:40 MEZ schrieb Tom Rini > > > > > > > > > > > <tr...@konsulko.com>: > > > > > > > > > > > > On Tue, Dec 19, 2023 at 12:08:31AM +0100, Heinrich > > > > > > > > > > > > Schuchardt wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Am 18. Dezember 2023 23:41:08 MEZ schrieb Tom Rini > > > > > > > > > > > > > <tr...@konsulko.com>: > > > > > > > > > > > > > > On Mon, Dec 18, 2023 at 11:34:16PM +0100, Heinrich > > > > > > > > > > > > > > Schuchardt wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > Or take: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > load host 0:1 $c kernel.efi > > > > > > > > > > > > > > > load host 0:1 $d initrd.img > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > How could we ensure that initrd.img is not > > > > > > > > > > > > > > > overwriting a part of kernel.efi without memory > > > > > > > > > > > > > > > allocation? > > > > > > > > > > > > > > > > > > > > > > > > > > > > Today, invalid checksum as part of some part of the > > > > > > > > > > > > > > kernel fails. But > > > > > > > > > > > > > > how do we do this tomorrow, are you suggesting that > > > > > > > > > > > > > > "load" perform > > > > > > > > > > > > > > malloc() in some predefined size? If $c is below $d > > > > > > > > > > > > > > and $c + kernel.efi > > > > > > > > > > > > > > is now above $d we can throw an error before trying > > > > > > > > > > > > > > to load, yes. But > > > > > > > > > > > > > > what about: > > > > > > > > > > > > > > load host 0:1 $d initrd.img > > > > > > > > > > > > > > load host 0:1 $c kernel.efi > > > > > > > > > > > > > > > > > > > > > > > > > > > > In that case (which is only marginally contrived, > > > > > > > > > > > > > > the more real case is > > > > > > > > > > > > > > loading device tree in to unexpectedly large > > > > > > > > > > > > > > ramdisk because someone > > > > > > > > > > > > > > didn't understand the general advice on why device > > > > > > > > > > > > > > tree is lower than > > > > > > > > > > > > > > ramdisk address) I'm fine with an error that > > > > > > > > > > > > > > amounts to "you just > > > > > > > > > > > > > > corrupted another allocation" and then "fail, reset > > > > > > > > > > > > > > the board" or so. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Our current malloc library cannot manage the complete > > > > > > > > > > > > > memory. We need a library like lmb which should also > > > > > > > > > > > > > cover the memory management that we currently have in > > > > > > > > > > > > > lib/efi/efi_memory.c. This must include a memory type > > > > > > > > > > > > > attribute for usage in the GetMemoryMap() service. A > > > > > > > > > > > > > management on page level seems sufficient. > > > > > > > > > > > > > > > > > > > > > > > > > > The load command should permanently allocate memory > > > > > > > > > > > > > in that lmb+ library. > > > > > > > > > > > > > > > > > > > > > > > > > > We need an unload command to free the memory if we > > > > > > > > > > > > > want to reuse the memory or we might let the load > > > > > > > > > > > > > comand free the memory if exactly the same start > > > > > > > > > > > > > address is reused. > > > > > > > > > > > > > > > > > > > > > > > > Our current way of loading things in to memory does not > > > > > > > > > > > > handle the case > > > > > > > > > > > > I described, yes. How would what you're proposing > > > > > > > > > > > > handle it? > > > > > > > > > > > > > > > > > > > > > > If the load command has to allocate memory for the image > > > > > > > > > > > and that allocation is kept, any attempt to allocate > > > > > > > > > > > overlapping memory would fail. > > > > > > > > > > > > > > > > > > > > So you're saying that the load command has to pre-allocate > > > > > > > > > > memory? Or as > > > > > > > > > > it goes? If the latter, in what size chunks? This starts to > > > > > > > > > > get at what > > > > > > > > > > Simon was talking about with respect to memory > > > > > > > > > > fragmentation. Which to > > > > > > > > > > be clear is a problem we have today, we just let things > > > > > > > > > > overlap and hope > > > > > > > > > > something later catches an incorrect checksum. > > > > > > > > > > > > > > > > > > > > > > > > > > > > I don't want to replace the malloc library which handles > > > > > > > > > large numbets of allocations. > > > > > > > > > > > > > > > > I'm confused. The normal malloc library is not involved with > > > > > > > > current > > > > > > > > image loading, it's direct to memory (with some attempts at > > > > > > > > sanity > > > > > > > > checking by lmb). Are you proposing a different allocator with > > > > > > > > malloc/free like behavior? If so, please outline how it will > > > > > > > > determine > > > > > > > > pool size, and how we'll use it to load thing to memory. > > > > > > > > > > > > > > All memory below the stack needs to be managed. Malloc uses a > > > > > > > small memory area (a few MiB) above the stack. > > > > > > > > > > > > That's a rather huge change for how U-Boot works. > > > > > > > > > > > > > > > Closing the eyes when the user loads multiple files does not > > > > > > > > > solve the fragmentation problem. > > > > > > > > > > > > > > > > Yes. I'm only noting that today we just ignore the problem and > > > > > > > > sometimes > > > > > > > > catch it via checksums. > > > > > > > > > > > > > > > > > Fragmentation only happens if we have many concurrent > > > > > > > > > allocations. In EFI we are allocating top down. The number > > > > > > > > > of concurrent allocations is low. Typically a few dozen at > > > > > > > > > most. After terminating an application these should be freed > > > > > > > > > again. > > > > > > > > > > > > > > > > OK, so are you saying that we would no longer be loading _to_ a > > > > > > > > location > > > > > > > > in memory and instead just be saying "load this thing" and > > > > > > > > picking where > > > > > > > > dynamically? > > > > > > > > > > > > > > Both preassigned and allocator assigned adresses are compatible > > > > > > > with memory management. > > > > > > > > > > > > > > Architectures and binaries have different requirements. On > > > > > > > riscv64 you can load Linux kernel, initrd, fdt anywhere. We don't > > > > > > > need predefined addresses there. Other architectures have > > > > > > > restrictions. > > > > > > > > > > > > Yes, 64 bit architecture tend to only have alignment requirements > > > > > > while > > > > > > 32bit architectures have both alignment requirements and some memory > > > > > > window requirement. Whatever we implement here needs to handle both > > > > > > cases. > > > > > > > > > > > > > > > When loading a file from a file system we know the filesize > > > > > > > > > beforehand. So allocation is trivial. > > > > > > > > > > > > > > > > > > The loady command currently does not use the offered size > > > > > > > > > information but could do so. > > > > > > > > > > > > > > > > We should be using that information to make sure we don't > > > > > > > > overwrite > > > > > > > > U-Boot itself, but I don't recall how exactly we handle it today > > > > > > > > off-hand. > > > > > > > > > > > > > > If the user issues multiple load commands, he can overwrite > > > > > > > previous files. > > > > > > > > > > > > Then it sounds like we lost one benefit of all of this overhead. > > > > > > > > > > > > > During boot command execution I guess the different allocations > > > > > > > respect each other. > > > > > > > > > > > > > > > > > > > > > > > > TFTP is problematic because it does not transfer the > > > > > > > > > filesize. We would probably try to allocate a large chunk of > > > > > > > > > memory and then downsize the allocation after reading the > > > > > > > > > whole file. > > > > > > > > > > > > > > > > Reading from non-filesystem flash also has this problem, but we > > > > > > > > at least > > > > > > > > specify the amount to read too. But yes, it gets back to what I > > > > > > > > was > > > > > > > > asking about on how you're proposing to handle network load > > > > > > > > cases. > > > > > > > > > > > > > > > > > > > > > > It depends on the protocol. Http conveys the size before the > > > > > > > data. Tftp does not. > > > > > > > > > > > > > > If you don't know the size, you must preallocate a big chunk, > > > > > > > check that the download does not exceed it, and downsize the > > > > > > > allocation afterwards. This is not a new problem but exists > > > > > > > already with current lmb usage. > > > > > > > > > > > > Yes, and what I'm trying to find out is if what you're suggesting > > > > > > would > > > > > > do anything about it, since previous statements you made implied to > > > > > > me > > > > > > that we would prevent it. > > > > > > > > > > > > To me, at this point it sounds like what we need is more like > > > > > > persistent > > > > > > memory blocks and a hook that can be called in to for both "give me > > > > > > all > > > > > > known memory blocks" and "add this memory block to the list", so > > > > > > that > > > > > > EFI can do whatever it needs to do upon starting an application and > > > > > > then > > > > > > upon return to U-Boot. Both malloc/free allocations and "load this > > > > > > blob > > > > > > to memory from whatever" allocations would call the appropriate > > > > > > hook for > > > > > > tracking. > > > > > > > > > > In my mind the solution to this entire problem is fairly minor changes > > > > > to how memory is allocated and only for EFI. > > > > > > > > > > I tried to map out what that would look like and we have IMO got lost > > > > > in the weeds a bit. > > > > > > > > > > I am not trying to solve the problem of the 'load' command doing an > > > > > allocation and throwing it away. To be that is WAI, at least until we > > > > > come up with another type of command. This is one of the reasons for > > > > > standard boot, allowing a more cohesive approach to booting. > > > > > > > > > > I will think about this some more... > > > > > > > > OK, but please keep in mind that lmb not being at all persistent is a > > > > problem for everyone, not just EFI. That really needs to be addressed, > > > > maybe with some flags for dis-allowing overwrites to the area. For > > > > example, the apple-m1 code to use lmb to find locations for the > > > > kernel/etc can be written to more than once (allocate the address, then > > > > write to it to start with, even) but the range that covers U-Boot itself > > > > (malloc pool and so forth) need to be stopped. > > > > > > > > > > The range managed by the EFI sub-system extends over all of RAM > > > including the addresses used by load commands. Hence, "the range that > > > covers U-Boot itself (malloc pool and so forth)" includes all memory. > > > > > > Maybe we could pre-allocate a memory area for file loading. This just > > > requires to define a device-specific high file memory address above > > > which no file loads will be allowed and below which EFI, bootm, etc will > > > not be allowed to allocate memory for further uses. > > > > We're not just / only an EFI runtime. When we load something persistent > > to memory outside of EFI, we need to note that, so that when EFI_LOADER > > beings work it can see this and do whatever it needs to do. And if/when > > EFI_LOADER returns control back to U-Boot itself, it needs to update > > that list with any new allocations that were done and mark them > > appropriately. > > The idea of EFI loader returning control is misleading. > > You can load an EFI driver binary which provides a device to U-Boot. > Whenever U-Boot accesses that driver allocations might occur.
We cannot, today, load an EFI driver and then make use of it outside of the context of having bootefi'd something, yes? Or is this about that separate runtime area from another thread? But even then, if something is making an allocation it needs to inform the rest of the world. > I thought that we already agreed to have only one memory management and > get rid of the LMB/EFI memory management duplication. We've agreed that today, CONFIG_LMB does not fit the use cases that we have in a modern system, for anyones usage. It has been suggested and I know it's somewhere on Ilias' TODO list, to try and take what we have in the EFI_LOADER & co area and make it usable and useful for all contexts. We also have what in some ways feels to me to be the inverse of that, which is Simon's proposal. We have not agreed that the EFI model and restrictions are what everything will now be using. What's on Ilias' TODD list might bring us in that direction, and I want to see what that looks like in practice. But it's not agreed to as the end goal here. And to be clear, I also would like to see what Simon's proposed, in practice. -- Tom
signature.asc
Description: PGP signature
