Hi Tom, On Thu, 7 Sept 2023 at 13:40, Tom Rini <tr...@konsulko.com> wrote: > > On Thu, Sep 07, 2023 at 06:23:06AM -0600, Simon Glass wrote: > > Hi Tom, > > > > On Wed, 6 Sept 2023 at 11:58, Tom Rini <tr...@konsulko.com> wrote: > > > > > > On Tue, Sep 05, 2023 at 12:16:56PM -0600, Simon Glass wrote: > > > > Hi Tom, > > > > > > > > On Sun, 3 Sept 2023 at 13:25, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > > > On Sun, Sep 03, 2023 at 12:06:13PM -0600, Simon Glass wrote: > > > > > > Hi Tom, > > > > > > > > > > > > On Sun, 3 Sept 2023 at 10:42, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > > > > > > > On Thu, Aug 31, 2023 at 06:15:19PM -0600, Simon Glass wrote: > > > > > > > > Hi Tom, > > > > > > > > > > > > > > > > On Wed, 23 Aug 2023 at 09:14, Tom Rini <tr...@konsulko.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Wed, Aug 23, 2023 at 07:42:03AM -0600, Simon Glass wrote: > > > > > > > > > > > > > > > > > > > When lmb runs out of space in its internal tables, it gives > > > > errors on > > > > > > > > > > every fs load operation. This is horribly confusing, as the > > > > poor user > > > > > > > > > > tries different memory regions one at a time. > > > > > > > > > > > > > > > > > > > > Use the updated API to check the error code and print a > > > > > > > > > > helpful > > > > message. > > > > > > > > > > Also, allow the operation to proceed. > > > > > > > > > > > > > > > > > > > > Update the tests accordingly. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Simon Glass <s...@chromium.org> > > > > > > > > > [snip] > > > > > > > > > > + if (ret == -E2BIG) { > > > > > > > > > > + log_warning("Reservation tables exhausted: see > > > > CONFIG_LMB_USE_MAX_REGIONS\n"); > > > > > > > > > > + return 0; > > > > > > > > > > + } > > > > > > > > > > > > > > > > > > This isn't the right option. Everyone has > > > > CONFIG_LMB_USE_MAX_REGIONS > > > > > > > > > set. You would want to increase CONFIG_LMB_MAX_REGIONS. > > > > > > > > > > > > > > > > > > But it sounds like what you've found and fixed is an > > > > > > > > > underlying > > > > problem > > > > > > > > > as to why 16 regions isn't enough in some common cases now? > > > > > > > > > So > > > > we could > > > > > > > > > > > > > > > > I don't think I have fixed anything. But I'll send v2 and > > > > > > > > perhaps it > > > > > > > > will be clearer what is going on here. > > > > > > > > > > > > > > > > > possibly avoid the string size growth here and have a comment > > > > that also > > > > > > > > > matches up with the help under LMB_MAX_REGIONS? > > > > > > > > > > > > > > > > I don't know, sorry. The size of struct(lmb) on 64-bit sandbox > > > > > > > > is > > > > > > > > about 400 bytes. There seems to be a lot of code to save not > > > > > > > > much > > > > > > > > memory. > > > > > > > > > > > > > > What do you mean here? The alternative is not unlimited ranges > > > > > > > but > > > > > > > instead to define the limit of memory regions and limit of > > > > > > > reserved > > > > > > > ranges. > > > > > > > > > > > > A better alternative would be not to limit the number of regions, > > > > > > IMO, > > > > > > i.e. have an array of regions that can grow as needed. > > > > > > > > > > That's not something that we have in the code today, however. > > > > > > > > No, I do have an arraylist thing that I plan to upstream, which could > > > > handle that. > > > > > > > > But for this series, what do you think of the memregion idea? I am still > > > > unsure of the saming we should use - see Heinrich's comments too. > > > > > > My biggest worry here is that we're papering over a real issue, and > > > should either dig at that and see what's going on (should something be > > > coalescing adjacent allocations? Were many platforms just right at the > > > previous limit?) or just bump the default from 16 to "64 if EFI_LOADER" > > > and then see if anything else really needs tweaking / cleaning up in the > > > code itself. I know Heinrich has previously said the LMB system could > > > be better (or something to that effect, I'm going from memory, sorry if > > > I'm mis-stating things) and I don't object to replacing what we have > > > with something more robust/smarter/etc. > > > > I am not sure...my series was designed to rename the code to reduce > > confusion, and print a useful message when we run out of regions. It > > does not paper over the problem, but actually makes it more visible. > > Well, "papering over" maybe wasn't the best choice of words on my part. > But since the series of events was something like: > - We nee to use LMB to cover my U-Boot regions to address a handful of > CVEs over arbitrarily overwriting U-Boot at run-time. > - AFAICT no platforms suddenly ran out of LMB reservation space, but > maybe I'm wrong? > - Someone noted that the EFI subsystem was exposing the same kind of > issue. > - Heinrich adds logic for EFI_LOADER (iirc) to also add allocations > there to LMB > - Assorted platforms start changing the max allocation to 64 to fix the > problems that get reported sometimes by booting. > - Heinrich notes that our memory reservation system (LMB) could be > better designed than it is today. > - And, iirc, EFI_LOADER or similar has something maybe we can > leverage? > > Which brings me to what I was trying to say. I'm not sure it's worth > cleaning up the code a little, or refactoring/renaming things within it > without both: > - Understanding why EFI_LOADER being enabled causes us to run out of > allocations and so if the answer is "increase the default" OR "fix > some underlying issue such as coalescing being too strict or broken". > - Understanding if there's a better memory reservation system we can use > instead.
Perhaps Heinrich has some thoughts on that and/or the memregion question. EFI loves memory regions so perhaps it needs more? But it should report a useful error, not the silly one it shows today. Regards, Simon