diff mbox series

[3/6] x86/boot: Remove the preconstructed low 16M superpage mappings

Message ID 20200106155423.9508-4-andrew.cooper3@citrix.com (mailing list archive)
State Superseded
Headers show
Series x86/boot: Remove mappings at 0 | expand

Commit Message

Andrew Cooper Jan. 6, 2020, 3:54 p.m. UTC
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.

Furthermore, nothing actually uses the mappings on boot.  Build these entries
in the directmap when walking the E820 table along with everything else.

As a consequence, there are now no _PAGE_PRESENT entries between
__page_tables_{start,end} which need to skip relocation.  This simplifies the
MB1/2 entry path logic to remove the l2_identmap[] special case.

The low 2M (using 4k pages) is retained for now.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/boot/head.S          | 10 ++--------
 xen/arch/x86/boot/x86_64.S        | 17 ++++++-----------
 xen/arch/x86/setup.c              | 12 ++++++------
 xen/arch/x86/x86_64/asm-offsets.c |  3 ---
 4 files changed, 14 insertions(+), 28 deletions(-)

Comments

Jan Beulich Jan. 7, 2020, 3:43 p.m. UTC | #1
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
Andrew Cooper Jan. 7, 2020, 5:24 p.m. UTC | #2
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
Jan Beulich Jan. 8, 2020, 11:23 a.m. UTC | #3
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
Andrew Cooper Jan. 8, 2020, 7:41 p.m. UTC | #4
On 08/01/2020 11:23, Jan Beulich wrote:
>>>  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.

I'm hoping to get rid of even this, (although it is complicated by
CONFIG_VIDEO's blind use of the legacy VGA range).

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

Is there?  I don't see a real connection between the two, even in this
patch.

~Andrew
Jan Beulich Jan. 9, 2020, 9:35 a.m. UTC | #5
On 08.01.2020 20:41, Andrew Cooper wrote:
> On 08/01/2020 11:23, Jan Beulich wrote:
>>>>  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.
> 
> I'm hoping to get rid of even this, (although it is complicated by
> CONFIG_VIDEO's blind use of the legacy VGA range).
> 
>> 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.
> 
> Is there?  I don't see a real connection between the two, even in this
> patch.

Perhaps not a "real" one (in the sense that I think you mean), but
at the very least when PREBUILT_MAP_LIMIT > BOOTSTRAP_MAP_BASE
adding a first bootstrap mapping would _require_ a TLB flush, whereas
things would be fine without otherwise (albeit this going through
map_pages_to_xen() means appropriate flushing will be done anyway).
Anything else depends on the potentially overlapping range actually
being used for something (which your series proves it's not).

Anyway, as said before, considering your subsequent changes, there's
no real need to add any further checking here.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 8d0ffbd1b0..7ee4511e26 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -661,15 +661,9 @@  trampoline_setup:
         mov     %eax,sym_fs(boot_tsc_stamp)
         mov     %edx,sym_fs(boot_tsc_stamp)+4
 
-        /*
-         * Update frame addresses in page tables excluding l2_identmap
-         * without its first entry which points to l1_identmap.
-         */
+        /* Relocate pagetables to point at Xen's current location in memory. */
         mov     $((__page_tables_end-__page_tables_start)/8),%ecx
-        mov     $(((l2_identmap-__page_tables_start)/8)+1),%edx
-1:      cmp     $((l2_identmap+l2_identmap_sizeof-__page_tables_start)/8),%ecx
-        cmove   %edx,%ecx
-        testl   $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8)
+1:      testl   $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8)
         jz      2f
         add     %esi,sym_fs(__page_tables_start)-8(,%ecx,8)
 2:      loop    1b
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index b54d3aceea..30c82f9d5c 100644
--- 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.
  */
 GLOBAL(l2_identmap)
         .quad sym_offs(l1_identmap) + __PAGE_HYPERVISOR
-        idx = 1
-        .rept 7
-        .quad (idx << L2_PAGETABLE_SHIFT) | PAGE_HYPERVISOR | _PAGE_PSE
-        idx = idx + 1
-        .endr
-        .fill 4 * L2_PAGETABLE_ENTRIES - 8, 8, 0
+        .fill 4 * L2_PAGETABLE_ENTRIES - 1, 8, 0
         .size l2_identmap, . - l2_identmap
 
 /*
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index ed54f79fea..452f5bdd37 100644
--- 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));
         map_e = min_t(uint64_t, e,
                       ARRAY_SIZE(l2_identmap) << L2_PAGETABLE_SHIFT);
 
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index f9cb78cfdb..07d2155bf5 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -165,8 +165,5 @@  void __dummy__(void)
     OFFSET(MB2_efi64_ih, multiboot2_tag_efi64_ih_t, pointer);
     BLANK();
 
-    DEFINE(l2_identmap_sizeof, sizeof(l2_identmap));
-    BLANK();
-
     OFFSET(DOMAIN_vm_assist, struct domain, vm_assist);
 }