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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

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

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

~Andrew

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