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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

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

Jan

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