Re: [Xen-devel] [PATCH 3/6] x86/boot: Remove the preconstructed low 16M superpage mappings

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.01.2020 18:24, Andrew Cooper wrote:
> On 07/01/2020 15:43, Jan Beulich wrote:
>> On 06.01.2020 16:54, Andrew Cooper wrote:
>>> First, it is undefined to have superpages and MTRRs disagree on cacheability
>>> boundaries, and nothing this early in boot has checked that it is safe to 
>>> use
>>> superpages here.
>> Stating this here gives, at least to me, the impression that you change
>> things here to obey to these restrictions. I don't see you do so, though
>> - map_pages_to_xen() doesn't query MTRRs at all afaics.
> 
> No, but it does now honour the E820 WRT holes and/or reserved regions,
> rather than blindly using 2M WB superpages, which is an improvement.

Can you then please make more explicit in this part of the description
what gets improved and what remains to be fishy?

>>> Furthermore, nothing actually uses the mappings on boot.  Build these 
>>> entries
>>> in the directmap when walking the E820 table along with everything else.
>> I'm pretty sure some of these mappings were used, perhaps long ago, and
>> possibly only by the 32-bit hypervisor. It would feel quite a bit better
>> if it was clear when the need for this disappeared. I wonder if I could
>> talk you into finding out, so you could say so here.
> 
> TBH, its hard enough figuring out how the mappings were used on staging
> alone.
> 
> At a guess, these date from the pre-MB2 days, where Xen depended on
> being loaded at 1M, and will have been the equivalent of:
> 
> +        /*
> +         * Map Xen into the directmap (needed for early-boot pagetable
> +         * handling/walking), and identity map Xen into bootmap (needed for
> +         * the transition into long mode), using 2M superpages.
> +         */
> 
> which is described now in patch 4.
> 
> In my experiments, discussed in the cover letter, I did get down to
> having a only the single 4k trampoline page mapped, and across a number
> of machines, it was the bootscrub which then hit their absence in the
> directmap.

Well, okay then without further archaeology.

>>> --- a/xen/arch/x86/boot/x86_64.S
>>> +++ b/xen/arch/x86/boot/x86_64.S
>>> @@ -66,24 +66,19 @@ l1_identmap:
>>>          .size l1_identmap, . - l1_identmap
>>>  
>>>  /*
>>> - * __page_tables_start does not cover l1_identmap because it (l1_identmap)
>>> - * contains 1-1 mappings. This means that frame addresses of these mappings
>>> - * are static and should not be updated at runtime.
>>> + * __page_tables_{start,end} cover the range of pagetables which need
>>> + * relocating as Xen moves around physical memory.  i.e. each sym_offs()
>>> + * reference to a different pagetable in the Xen image.
>>>   */
>>>  GLOBAL(__page_tables_start)
>>>  
>>>  /*
>>> - * Space for mapping the first 4GB of memory, with the first 16 megabytes
>>> - * actualy mapped (mostly using superpages).  Uses 4x 4k pages.
>>> + * Space for 4G worth of 2M mappings, first 2M actually mapped via
>>> + * l1_identmap[].  Uses 4x 4k pages.
>> Would you mind making this say "page tables" instead of "pages" in the
>> 2nd sentence?
> 
> Why?  Currently all the "Uses x pages" are consistent, and it is
> describing the size of the objects, whose units are pages, not pagetables.

Fair enough.

>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -1020,8 +1020,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>       *
>>>       * We require superpage alignment because the boot allocator is
>>>       * not yet initialised. Hence we can only map superpages in the
>>> -     * address range BOOTSTRAP_MAP_BASE to 4GB, as this is guaranteed
>>> -     * not to require dynamic allocation of pagetables.
>>> +     * address range 2MB to 4GB, as this is guaranteed not to require
>>> +     * dynamic allocation of pagetables.
>>>       *
>>>       * As well as mapping superpages in that range, in preparation for
>>>       * initialising the boot allocator, we also look for a region to which
>>> @@ -1036,10 +1036,10 @@ void __init noreturn __start_xen(unsigned long 
>>> mbi_p)
>>>          if ( boot_e820.map[i].type != E820_RAM )
>>>              continue;
>>>  
>>> -        /* Superpage-aligned chunks from BOOTSTRAP_MAP_BASE. */
>>> +        /* Superpage-aligned chunks from 2MB. */
>>>          s = (boot_e820.map[i].addr + mask) & ~mask;
>>>          e = (boot_e820.map[i].addr + boot_e820.map[i].size) & ~mask;
>>> -        s = max_t(uint64_t, s, BOOTSTRAP_MAP_BASE);
>>> +        s = max_t(uint64_t, s, MB(2));
>>>          if ( s >= e )
>>>              continue;
>>>  
>>> @@ -1346,8 +1346,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>  
>>>          set_pdx_range(s >> PAGE_SHIFT, e >> PAGE_SHIFT);
>>>  
>>> -        /* Need to create mappings above BOOTSTRAP_MAP_BASE. */
>>> -        map_s = max_t(uint64_t, s, BOOTSTRAP_MAP_BASE);
>>> +        /* Need to create mappings above 2MB. */
>>> +        map_s = max_t(uint64_t, s, MB(2));
>> Instead of hard coding 2Mb everywhere, how about simply reducing
>> BOOTSTRAP_MAP_BASE?
> 
> Because the use of BOOTSTRAP_MAP_BASE here is conceptually wrong.
> 
> Once I've figured out one other bug on the EFI side of things only, I've
> got a follow-on change which manages to undef BOOTSTRAP_MAP_BASE beside
> LIMIT because, ...
> 
>>  This would then also ease shrinking the build
>> time mappings further, e.g. to the low 1Mb (instead of touching
>> several of the places you touch now, it would again mainly be an
>> adjustment to BOOTSTRAP_MAP_BASE, alongside the assembly file
>> changes needed).
> 
> ... as you correctly identify here, it is a property of the prebuilt
> tables (in l?_identmap[]), not a property of where we chose to put the
> dynamic boot mappings (in the l?_bootmap[]).  Another change (blocked
> behind the above bug) moves BOOTSTRAP_MAP_BASE to be 1G to reduce the
> chance of an offset from a NULL pointer hitting a present mapping.

Right, BOOTSTRAP_MAP_BASE was (ab)used for a 2nd purpose. But this
would better be dealt with by introducing a new manifest constant
(e.g. PREBUILT_MAP_LIMIT) instead of open-coding 2Mb everywhere. Plus
there's (aiui) a PREBUILT_MAP_LIMIT <= BOOTSTRAP_MAP_BASE
requirement, which would better be verified (e.g. by a BUILD_BUG_ON())
then. Then again, having also seen patch 5 now, the relationship
basically goes away altogether there, so perhaps adding a check here
just to drop it there again isn't very useful (but the omission
thereof in the patch here might warrant a remark in the description
then).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel