On 13 Nov 2018, at 20:16, Kevin O'Connor kevin@koconnor.net wrote:
On Tue, Nov 13, 2018 at 05:53:41PM +0200, Liran Alon wrote:
When 32-bit code needs to allocate memory which remains permanent after completion of the initialization phase, it doesn't care if it is allocated from high-zone or low-zone.
Currently all these cases just attempt to allocate from high-zone as low-zone memory is scarce.
However, in cases we don't have enough space left in high-zone, it makes sense to use low-zone as a fallback.
Thanks, but I fear this change would have poor secondary effects. There is only a tiny amount of ram available in the permanent "low" area of ram (typically less that 64K). If the code has exhausted the permanent "high" ram (typically 256K) then it is likely to rapidly exhaust the low ram and cause a worse failure when code that requires a tiny amount of "low" space fails.
I disagree with “likely”.
As I see it, the caller which allocates from high-zone doesn’t care if it is satisfied eventually from high-zone or low-zone and if we got to the rare scenario where we exhausted all high-zone memory and low-zone memory can save us, why not try? This could save the guest of being able to boot on some scenarios (As the scenario I have encountered when attaching many PVSCSI controllers). And that seems better to me than to fail on allocation on SeaBIOS which is a hard thing to debug for the common user which is not a SeaBIOS developer.
Also, why will failures on allocation on low-zone should be worse than failures on allocation on high-zone? From the code I see that all allocations failures eventually call warn_noalloc() which should make it very easy to debug for a SeaBIOS developer. This is true both for allocations from high-zone and allocations from low-zone.
Typical work arounds are to extend the amount of high ram available (see BUILD_MAX_HIGHTABLE) or to use malloc_tmphigh() and manually reserve the space in the e820 map.
I think this patch is good regardless if modifying BUILD_MAX_HIGHTABLE can be served as a workaround. (Also if it is a typical workaround, why isn’t it configurable as a Makefile parameter?)
I find it odd to allocate from malloc_tmphigh() and manually reserve the space in the e820 map. As it basically undermines the definition of the Temporary High-Zone which should not be reserved from OS and therefore must not be accessed after SeaBIOS initialisation phase.
Also, it is kinda hard to put the finger on which code you would change to use malloc_tmphigh() like this all the time. Will you do this change for example in pvscsi.c code? It seems odd to me that pvscsi code should deal with such low-level memory handling instead of all this being transparently handled in the malloc_*() functions.
-Liran
-Kevin