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

> (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?

> > In regards to tboot I think that we should just take into account during
> > calculation ebmalloc_mem allocated part only. Maybe move of ebmalloc_mem
> > region to the end of BSS would help somehow here.
>
> At the moment, all the ranges of Xen VA space are bounded by linker symbols.
>
> It is certainly possible to could account for the ebmalloc object in the
> middle of the BSS, but it is turning into a layering violation.

I am not sure what do you mean by "layering violation".

Daniel

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