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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
