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 user gets full memory minus a bit, to do with as they need for their use case. -- Tom
signature.asc
Description: PGP signature
