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

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

On Mon, Sep 14, 2020 at 05:26:57PM +0200, Jan Beulich wrote:
> 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.

Maybe we could add a union there so that mod_end would alias with
mod_size or something.

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

We would have to adjust the calculations then, but would be fine IMO
as it won't require poking a hole in the bss section. I assume we
would need to then move _end to cover it.

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

I would just place it at the end of the loaded image (after bss) as it
seems simpler.

Roger.