Re: [PATCH 4/4] EFI: free unused boot mem in at least some cases

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.08.2020 19:09, Andrew Cooper wrote:
> On 06/08/2020 10:06, Jan Beulich wrote:
>> Address at least the primary reason why 52bba67f8b87 ("efi/boot: Don't
>> free ebmalloc area at all") was put in place: Make xen_in_range() aware
>> of the freed range. This is in particular relevant for EFI-enabled
>> builds not actually running on EFI, as the entire range will be unused
>> in this case.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> The remaining issue could be addressed too, by making the area 2M in
>> size and 2M-aligned.
> 
> This memory range is only used for relocating the (synthesized?)
> multiboot strings, is it not?
> 
> I'm not actually convinced that this is a sensible tradeoff.
> 
> For one, you've broken setup.c's:
> 
>     /* This needs to remain in sync with xen_in_range(). */
>     reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end));
> 
> which covers the runtime aspect of what xen_in_range() covers during boot.

I'm afraid this wasn't a good suggestion here (it was still helpful
to notice that tboot.c also needs adjustment): By not reserving the
range here, it'll get freed by end_boot_allocator(), and hence may
not (again) be freed by free_ebmalloc_unused_mem() (kind of putting
its name under question). Immediately up from the quoted place we
also reserve the space where the modules live, which also gets
freed later. I'm having difficulty to see why this particular
aspect needs to remain in sync between the reservation done here
and xen_in_range().

v2 definitely is broken because of me not having noticed this in
time. I'll first try to fix it without reverting to the v1 model,
but I'd prefer to go back to the earlier approach (keeping merely
the other v2 adjustments). Unless of course you see some breakage
from this that I don't see.

Jan