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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.09.2020 13:17, Roger Pau Monné wrote:
> On Thu, Sep 17, 2020 at 12:56:41PM +0200, Jan Beulich wrote:
>> 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.
> 
> Right, I certainly didn't catch that passing one as NULL would cause a
> deref there also.
> 
> I would be more comfortable with adding an ASSERT, but I'm not going
> to insist. IIRC there was a time when Xen running as a PVH guest (like
> in shim mode) would cause it to have a valid mapping at 0.

Well, apparently not anymore, or else v3 wouldn't have needed prompt
reverting. With ...

>>>> @@ -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.
> 
> Would you mind also adding a FIXME comment in efi_boot_mem_unused that
> setting ebmalloc_allocated to sizeof(ebmalloc_mem) will be removed
> once we can properly free the region regardless of whether 2M are
> being used?

... the two FIXMEs added, it is sufficiently clear that the goal
is for this to be transient only anyway. As said - I have a plan,
I just need to find the time to see if it works out.

> Seems like an abuse of that the function should be doing by passing
> NULL pointers to it, or maybe I'm just being dense.

In a way it is an abuse, I agree, with - as said - the goal of
avoiding to expose using_2M_mapping().

> With that:
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks much.

Jan