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 <jbeul...@suse.com>
> >
> > Acked-by: Roger Pau Monné <roger....@citrix.com>
>
> 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.
