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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.09.2020 12:45, Roger Pau Monné wrote:
> On Wed, Sep 16, 2020 at 02:20:54PM +0200, Jan Beulich wrote:
>> --- a/xen/arch/x86/efi/stub.c
>> +++ b/xen/arch/x86/efi/stub.c
>> @@ -52,6 +52,13 @@ bool efi_enabled(unsigned int feature)
>>  
>>  void __init efi_init_memory(void) { }
>>  
>> +bool efi_boot_mem_unused(unsigned long *start, unsigned long *end)
>> +{
>> +    if ( start || end )
> 
> Shouldn't this be start && end?

This is consistent with "if ( !start && !end )" in the non-stub
function, which was there in v3 already.

> Or else you might be de-referencing a NULL pointer?

Intentionally so: I'd view it as worse if we didn't fill *start
or *end if just one gets passed as NULL. The way it's done now
it'll be a reliable crash, as the v3 issue with the shim has
shown (where the if() here was missing).

>> @@ -1417,8 +1419,18 @@ void __init noreturn __start_xen(unsigne
>>      if ( !xen_phys_start )
>>          panic("Not enough memory to relocate Xen\n");
>>  
>> +    /* FIXME: Putting a hole in .bss would shatter the large page mapping. 
>> */
>> +    if ( using_2M_mapping() )
>> +        efi_boot_mem_unused(NULL, NULL);
> 
> This seems really weird IMO...

What I didn't like about earlier versions was the exposure of
using_2M_mapping() outside of this CU. The way it works is
somewhat fragile, and hence I think limiting its exposure is a
win. This way there's also no x86-specific code in ebmalloc.c
anymore.

I'm also slightly puzzled that you ask now when you had acked
this same construct in v3. It's really just the stub function
which has changed in v4.

>> @@ -36,17 +37,37 @@ void __init *ebmalloc(size_t size)
>>      return ptr;
>>  }
>>  
>> +bool efi_boot_mem_unused(unsigned long *start, unsigned long *end)
>> +{
>> +    if ( !start && !end )
>> +    {
>> +        ebmalloc_allocated = sizeof(ebmalloc_mem);
>> +        return false;
>> +    }
> 
> ... I would instead place the using_2M_mapping check here

As per above, this would mean x86-specific code here again.

> and return start = end in that case.

I don't think I understand this part, possibly starting with me
wondering whether you mean *start == *end (and implying they'd
be set to valid values first).

Jan