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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

> > 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.

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?

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

With that:

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

Thanks.