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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.09.2020 17:16, Roger Pau Monné wrote:
> On Mon, Aug 24, 2020 at 02:08:11PM +0200, 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>
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks much.

>> @@ -1145,7 +1146,8 @@ void __init noreturn __start_xen(unsigne
>>  
>>          /*
>>           * This needs to remain in sync with xen_in_range() and the
>> -         * respective reserve_e820_ram() invocation below.
>> +         * respective reserve_e820_ram() invocation below. No need to
>> +         * query efi_boot_mem_unused() here, though.
>>           */
>>          mod[mbi->mods_count].mod_start = virt_to_mfn(_stext);
>>          mod[mbi->mods_count].mod_end = __2M_rwdata_end - _stext;
> 
> I find this extremely confusing, we reuse mod_start/mod_end to contain
> a mfn and a size (in bytes) instead of a start and end address (not
> something that should be fixed here, but seeing this I assumed it was
> wrong).

While perhaps somewhat confusing, I still think it was a fair thing
to do in favor of introducing a completely new way of propagating
respective information, and then having the consumer of this data
look at two different places.

>> +bool efi_boot_mem_unused(unsigned long *start, unsigned long *end)
>> +{
>> +    *start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated);
>> +    *end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem);
>> +
>> +    return *start < *end;
>> +}
>> +
>>  void __init free_ebmalloc_unused_mem(void)
>>  {
>> -#if 0 /* FIXME: Putting a hole in the BSS breaks the IOMMU mappings for 
>> dom0. */
>>      unsigned long start, end;
>>  
>> -    start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated);
>> -    end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem);
>> +#ifdef CONFIG_X86
>> +    /* FIXME: Putting a hole in .bss would shatter the large page mapping. 
>> */
> 
> Could you make the ebmalloc size (EBMALLOC_SIZE) 2MB (and aligned), so
> that you would only shatter the malloc'ed pages but not the
> surrounding mappings?
> 
> That would be a good compromise IMO.

Yes, that's what I've been considering as a compromise as well. In
fact I was further thinking whether to allocate the space from the
linker script instead of having a global/static object. Maybe by
extending into the .pad section, which is already 2Mb aligned anyway.

Another option is to not further align the whole blob at all and
merely free whatever comes past the next 2Mb boundary (and is not
in use). This would avoid having an up to 2Mb block of unused, not
freed memory ahead of the ebmalloc one.

Jan