Re: [Xen-devel] [PATCH 2/2] efi/boot: Don't free ebmalloc area at all

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Tue, Feb 28, 2017 at 07:09:14PM +0000, Andrew Cooper wrote:
> On 28/02/17 19:01, Daniel Kiper wrote:
> > On Tue, Feb 28, 2017 at 05:58:26PM +0000, Andrew Cooper wrote:
> >> On 28/02/17 17:41, Daniel Kiper wrote:
> >>> On Tue, Feb 28, 2017 at 04:08:35PM +0000, Andrew Cooper wrote:
> >>>> On 28/02/17 16:03, Jan Beulich wrote:
> >>>>>>>> On 28.02.17 at 16:20, <andrew.cooper3@xxxxxxxxxx> wrote:
> >>>>>> Freeing part of the BSS back for general use proves to be problematic. 
> >>>>>>  It is
> >>>>>> not accounted for in xen_in_range(), causing errors when constructing 
> >>>>>> the
> >>>>>> IOMMU tables, resulting in a failure to boot.
> >>>>>>
> >>>>>> Other smaller issues are that tboot treats the entire BSS as 
> >>>>>> hypervisor data,
> >>>>>> creating and checking a MAC of it on S3, and that, by being 1MB in 
> >>>>>> size,
> >>>>>> freeing it guarentees to shatter the hypervisor superpage mappings.
> >>>>>>
> >>>>>> Judging by the content stored in it, 1MB is overkill on size.  Drop it 
> >>>>>> to a
> >>>>>> more-reasonable 32kB and keep the entire buffer around after boot.
> >>>>> Well, that's just because right now there's only a single user. The
> >>>>> reason I refused Daniel making it smaller than its predecessor is
> >>>>> that we can't really give a good estimate of how much data may
> >>>>> need storing there: The memory map can have hundreds of entries
> >>>>> and command lines for modules may also be almost arbitrarily long.
> >>>>>
> >>>>> What I don't recall, Daniel: Why was it that we can't use EFI boot
> >>>>> services allocations here?
> >>>> From the original commit message,
> >>>>
> >>>>     1) We could use native EFI allocation functions (e.g. AllocatePool()
> >>>>        or AllocatePages()) to get memory chunk. However, later (somewhere
> >>>>        in __start_xen()) we must copy its contents to safe place or 
> >>>> reserve
> >>>>        it in e820 memory map and map it in Xen virtual address space. 
> >>>> This
> >>>>        means that the code referring to Xen command line, loaded modules 
> >>>> and
> >>>>        EFI memory map, mostly in __start_xen(), will be further 
> >>>> complicated
> >>>>        and diverge from legacy BIOS cases. Additionally, both former 
> >>>> things
> >>>>        have to be placed below 4 GiB because their addresses are stored 
> >>>> in
> >>>>        multiboot_info_t structure which has 32-bit relevant members.
> >>>>
> >>>>
> >>>> One way or another, if we don't want to shatter superpages, we either
> >>>> must keep the entire allocation, or copy the content out later into a
> >>>> smaller location once other heap facilities are available.
> >>> Hmmm... Why BSS free "shatter superpages" and .init.* sections free does 
> >>> not?
> >> Xen is purposefully laid out like this:
> >>
> >> .text, 2M aligned, R/X
> >> .rodata, 2M aligned, R/NX
> >> .init.*, 2M aligned, R/W/X (reclaimed)
> >> .data + .bss, 2M aligned, R/W/NX
> > AIUI, the purpose is to have properly mapped (in terms of R/W/X/NX)
> > sections. Right?
>
> When I did the original work, it was both to get proper pagetable
> permissions, and to get all of the compiled bits of Xen living in 2M
> superpages (which allows the entire hypervisor to operate in 5 TLB entries!)

Thanks for explanation.

> >> (In reality there is a syslinux bug which caused me to revert the 2M
> >> alignment in non-EFI builds, so we are still using 4k alignment, but I
> >> need to find a way to work around this and move back to enforcing the 2M
> >> alignment.)
> >>
> >> When .init gets reclaimed, this leaves a (deliberate) hole which is all
> >> 2M aligned, which has no impact on the adjacent 2M superpages.
> >>
> >> When ebmalloc gets reclaimed, it must shatter one or two superpages
> >> mapping the .data/.bss section.
> > Looking at this I do not have a better idea for fix off the top of my head.
> > So, I have a feeling that we should drop free_ebmalloc_unused_mem() and 
> > leave
> > comment around ebmalloc() why we do not reclaim unused ebmalloc_mem region.
> > Should I post a patch or whether you would like to do it?
>
> The patch at the root of this thread is basically that.

Yep, I know.

> As a stopgap solution to unblock staging, I think I should drop the
> adjustment of MB(1) to KB(32) and submit the patch.

Sounds like a plan.

> This satisfies Jan's request to not make the current buffer smaller than
> it currently is, and will give us more time to discuss alternative
> solutions, at the cost of wasting 1MB of RAM.  (Not exactly breaking the
> bank these days, but definitely something which should be fixed before
> 4.9 ships.)

Granted!

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel