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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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!)

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

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

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

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

It feels hacky, and the wrong way to solve the problem.

~Andrew

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