diff mbox

[v2,2/8] xen/x86: Improvements to build-time pagetable generation

Message ID 1456245085-2302-3-git-send-email-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Cooper Feb. 23, 2016, 4:31 p.m. UTC
* Additional comments, including size and runtime use.
 * Consistent use of idx, rather than a mix including pfn.
 * Consistent use of .quad, rather than a mix including .long.
 * Consistent use of PAGE_HYPERVISOR for addresses in the Xen global range,
   which include _PAGE_GLOBAL.

No change in runtime behaviour.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

New in v2
---
 xen/arch/x86/boot/x86_64.S | 45 ++++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

Comments

Jan Beulich Feb. 24, 2016, 11:24 a.m. UTC | #1
>>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
> * Additional comments, including size and runtime use.
>  * Consistent use of idx, rather than a mix including pfn.

I don't see anything wrong with this - when we have a PFN why
should we not call it a PFN? As opposed to when we have a table
index not representing the corresponding PFN.

>  GLOBAL(l1_identmap)
> -        pfn = 0
> +        idx = 0
>          .rept L1_PAGETABLE_ENTRIES
>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
> -        .if pfn >= 0xa0 && pfn < 0xc0
> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
> +        .if idx >= 0xa0 && idx < 0xc0
> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>          .else
> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR

Please don't eliminate the MAP_SMALL_PAGES here, they serve an
(at least documentation) purpose.

> -/* Mapping of first 16 megabytes of memory. */
> +/*
> + * Space for mapping the first 4GB of memory, with the first 16 megabytes
> + * actualy mapped (mostly using superpages).  Uses 4x 4k pages.
> + */
>  GLOBAL(l2_identmap)
> -        .quad sym_phys(l1_identmap) + __PAGE_HYPERVISOR
> -        pfn = 0
> +        .quad sym_phys(l1_identmap) + PAGE_HYPERVISOR

This is wrong - the G bit is defined for leaf entries only.

Jan
Andrew Cooper Feb. 24, 2016, 1:57 p.m. UTC | #2
On 24/02/16 11:24, Jan Beulich wrote:
>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>> * Additional comments, including size and runtime use.
>>  * Consistent use of idx, rather than a mix including pfn.
> I don't see anything wrong with this - when we have a PFN why
> should we not call it a PFN? As opposed to when we have a table
> index not representing the corresponding PFN.

In the case of l2_identmap, the logic is more clearly expressed in terms
of idx.  That leaves l1_identmap as the odd one out, where idx and pfn
are equally applicable.

>
>>  GLOBAL(l1_identmap)
>> -        pfn = 0
>> +        idx = 0
>>          .rept L1_PAGETABLE_ENTRIES
>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>> -        .if pfn >= 0xa0 && pfn < 0xc0
>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>> +        .if idx >= 0xa0 && idx < 0xc0
>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>          .else
>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
> (at least documentation) purpose.

How?  Its in a l1 so are necessarily small pages, and the other l1's
don't use the constant.

~Andrew
Jan Beulich Feb. 24, 2016, 2:15 p.m. UTC | #3
>>> On 24.02.16 at 14:57, <andrew.cooper3@citrix.com> wrote:
> On 24/02/16 11:24, Jan Beulich wrote:
>>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>  GLOBAL(l1_identmap)
>>> -        pfn = 0
>>> +        idx = 0
>>>          .rept L1_PAGETABLE_ENTRIES
>>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>>> +        .if idx >= 0xa0 && idx < 0xc0
>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>>          .else
>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
>> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
>> (at least documentation) purpose.
> 
> How?  Its in a l1 so are necessarily small pages, and the other l1's
> don't use the constant.

MAP_SMALL_PAGES documents (and enforces) that the mappings
shouldn't be re-combined into 2M ones, even if - after adjustments
to the other attributes - they could be.

Jan
Andrew Cooper Feb. 24, 2016, 2:58 p.m. UTC | #4
On 24/02/16 14:15, Jan Beulich wrote:
>>>> On 24.02.16 at 14:57, <andrew.cooper3@citrix.com> wrote:
>> On 24/02/16 11:24, Jan Beulich wrote:
>>>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>>  GLOBAL(l1_identmap)
>>>> -        pfn = 0
>>>> +        idx = 0
>>>>          .rept L1_PAGETABLE_ENTRIES
>>>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>>>> +        .if idx >= 0xa0 && idx < 0xc0
>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>>>          .else
>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
>>> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
>>> (at least documentation) purpose.
>> How?  Its in a l1 so are necessarily small pages, and the other l1's
>> don't use the constant.
> MAP_SMALL_PAGES documents (and enforces) that the mappings
> shouldn't be re-combined into 2M ones, even if - after adjustments
> to the other attributes - they could be.

In which case, is actively wrong.  Were the cacheabilities to change
(e.g. booting HVMLite and knowing that there was no legacy VGA hole),
the mappings should be recombined into a 2M superpage.

~Andrew
Jan Beulich Feb. 24, 2016, 3:18 p.m. UTC | #5
>>> On 24.02.16 at 15:58, <andrew.cooper3@citrix.com> wrote:
> On 24/02/16 14:15, Jan Beulich wrote:
>>>>> On 24.02.16 at 14:57, <andrew.cooper3@citrix.com> wrote:
>>> On 24/02/16 11:24, Jan Beulich wrote:
>>>>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>>>  GLOBAL(l1_identmap)
>>>>> -        pfn = 0
>>>>> +        idx = 0
>>>>>          .rept L1_PAGETABLE_ENTRIES
>>>>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>>>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>>>>> +        .if idx >= 0xa0 && idx < 0xc0
>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>>>>          .else
>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
>>>> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
>>>> (at least documentation) purpose.
>>> How?  Its in a l1 so are necessarily small pages, and the other l1's
>>> don't use the constant.
>> MAP_SMALL_PAGES documents (and enforces) that the mappings
>> shouldn't be re-combined into 2M ones, even if - after adjustments
>> to the other attributes - they could be.
> 
> In which case, is actively wrong.  Were the cacheabilities to change
> (e.g. booting HVMLite and knowing that there was no legacy VGA hole),
> the mappings should be recombined into a 2M superpage.

No, I think there are reasons (to do with fixed range MTRRs and
errata) which require us to keep these as small pages even if they
could be recombined.

Jan
Andrew Cooper Feb. 24, 2016, 3:22 p.m. UTC | #6
On 24/02/16 15:18, Jan Beulich wrote:
>>>> On 24.02.16 at 15:58, <andrew.cooper3@citrix.com> wrote:
>> On 24/02/16 14:15, Jan Beulich wrote:
>>>>>> On 24.02.16 at 14:57, <andrew.cooper3@citrix.com> wrote:
>>>> On 24/02/16 11:24, Jan Beulich wrote:
>>>>>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>>>>  GLOBAL(l1_identmap)
>>>>>> -        pfn = 0
>>>>>> +        idx = 0
>>>>>>          .rept L1_PAGETABLE_ENTRIES
>>>>>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>>>>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>>>>>> +        .if idx >= 0xa0 && idx < 0xc0
>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>>>>>          .else
>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
>>>>> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
>>>>> (at least documentation) purpose.
>>>> How?  Its in a l1 so are necessarily small pages, and the other l1's
>>>> don't use the constant.
>>> MAP_SMALL_PAGES documents (and enforces) that the mappings
>>> shouldn't be re-combined into 2M ones, even if - after adjustments
>>> to the other attributes - they could be.
>> In which case, is actively wrong.  Were the cacheabilities to change
>> (e.g. booting HVMLite and knowing that there was no legacy VGA hole),
>> the mappings should be recombined into a 2M superpage.
> No, I think there are reasons (to do with fixed range MTRRs and
> errata)

Any idea about which generation this might apply to?

>  which require us to keep these as small pages even if they
> could be recombined.

In the HVMLite case, MTRRs can be sensibly be disable in the guest which
would also allow the host EPT mappings to recombine back to a superpage.

~Andrew
Jan Beulich Feb. 24, 2016, 3:48 p.m. UTC | #7
>>> On 24.02.16 at 16:22, <andrew.cooper3@citrix.com> wrote:
> On 24/02/16 15:18, Jan Beulich wrote:
>>>>> On 24.02.16 at 15:58, <andrew.cooper3@citrix.com> wrote:
>>> On 24/02/16 14:15, Jan Beulich wrote:
>>>>>>> On 24.02.16 at 14:57, <andrew.cooper3@citrix.com> wrote:
>>>>> On 24/02/16 11:24, Jan Beulich wrote:
>>>>>>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>>>>>  GLOBAL(l1_identmap)
>>>>>>> -        pfn = 0
>>>>>>> +        idx = 0
>>>>>>>          .rept L1_PAGETABLE_ENTRIES
>>>>>>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>>>>>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>>>>>>> +        .if idx >= 0xa0 && idx < 0xc0
>>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>>>>>>          .else
>>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
>>>>>> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
>>>>>> (at least documentation) purpose.
>>>>> How?  Its in a l1 so are necessarily small pages, and the other l1's
>>>>> don't use the constant.
>>>> MAP_SMALL_PAGES documents (and enforces) that the mappings
>>>> shouldn't be re-combined into 2M ones, even if - after adjustments
>>>> to the other attributes - they could be.
>>> In which case, is actively wrong.  Were the cacheabilities to change
>>> (e.g. booting HVMLite and knowing that there was no legacy VGA hole),
>>> the mappings should be recombined into a 2M superpage.
>> No, I think there are reasons (to do with fixed range MTRRs and
>> errata)
> 
> Any idea about which generation this might apply to?

Just read the SDM sub-section "Large Page Size Considerations"
inside the section on MTRRs.

Jan
Andrew Cooper Feb. 24, 2016, 4:14 p.m. UTC | #8
On 24/02/16 15:48, Jan Beulich wrote:
>>>> On 24.02.16 at 16:22, <andrew.cooper3@citrix.com> wrote:
>> On 24/02/16 15:18, Jan Beulich wrote:
>>>>>> On 24.02.16 at 15:58, <andrew.cooper3@citrix.com> wrote:
>>>> On 24/02/16 14:15, Jan Beulich wrote:
>>>>>>>> On 24.02.16 at 14:57, <andrew.cooper3@citrix.com> wrote:
>>>>>> On 24/02/16 11:24, Jan Beulich wrote:
>>>>>>>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>>>>>>  GLOBAL(l1_identmap)
>>>>>>>> -        pfn = 0
>>>>>>>> +        idx = 0
>>>>>>>>          .rept L1_PAGETABLE_ENTRIES
>>>>>>>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>>>>>>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>>>>>>>> +        .if idx >= 0xa0 && idx < 0xc0
>>>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>>>>>>>          .else
>>>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>>>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
>>>>>>> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
>>>>>>> (at least documentation) purpose.
>>>>>> How?  Its in a l1 so are necessarily small pages, and the other l1's
>>>>>> don't use the constant.
>>>>> MAP_SMALL_PAGES documents (and enforces) that the mappings
>>>>> shouldn't be re-combined into 2M ones, even if - after adjustments
>>>>> to the other attributes - they could be.
>>>> In which case, is actively wrong.  Were the cacheabilities to change
>>>> (e.g. booting HVMLite and knowing that there was no legacy VGA hole),
>>>> the mappings should be recombined into a 2M superpage.
>>> No, I think there are reasons (to do with fixed range MTRRs and
>>> errata)
>> Any idea about which generation this might apply to?
> Just read the SDM sub-section "Large Page Size Considerations"
> inside the section on MTRRs.

Right, and all that says is "don't accidentally mix cacheabilities
between paging and MTRRs".

In the example of an HVMLite guest, It is entirely reasonable (and
indeed preferable) to avoid shattered superpages (both guest and host)
for mapping gfn 0.  The best case is that an HVMLite guest ends up as an
exact multiple of 1GB, and uses 1GB HAP superpages.

~Andrew
Jan Beulich Feb. 24, 2016, 4:59 p.m. UTC | #9
>>> On 24.02.16 at 17:14, <andrew.cooper3@citrix.com> wrote:
> On 24/02/16 15:48, Jan Beulich wrote:
>>>>> On 24.02.16 at 16:22, <andrew.cooper3@citrix.com> wrote:
>>> On 24/02/16 15:18, Jan Beulich wrote:
>>>>>>> On 24.02.16 at 15:58, <andrew.cooper3@citrix.com> wrote:
>>>>> On 24/02/16 14:15, Jan Beulich wrote:
>>>>>>>>> On 24.02.16 at 14:57, <andrew.cooper3@citrix.com> wrote:
>>>>>>> On 24/02/16 11:24, Jan Beulich wrote:
>>>>>>>>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>>>>>>>  GLOBAL(l1_identmap)
>>>>>>>>> -        pfn = 0
>>>>>>>>> +        idx = 0
>>>>>>>>>          .rept L1_PAGETABLE_ENTRIES
>>>>>>>>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>>>>>>>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>>>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>>>>>>>>> +        .if idx >= 0xa0 && idx < 0xc0
>>>>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>>>>>>>>          .else
>>>>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>>>>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
>>>>>>>> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
>>>>>>>> (at least documentation) purpose.
>>>>>>> How?  Its in a l1 so are necessarily small pages, and the other l1's
>>>>>>> don't use the constant.
>>>>>> MAP_SMALL_PAGES documents (and enforces) that the mappings
>>>>>> shouldn't be re-combined into 2M ones, even if - after adjustments
>>>>>> to the other attributes - they could be.
>>>>> In which case, is actively wrong.  Were the cacheabilities to change
>>>>> (e.g. booting HVMLite and knowing that there was no legacy VGA hole),
>>>>> the mappings should be recombined into a 2M superpage.
>>>> No, I think there are reasons (to do with fixed range MTRRs and
>>>> errata)
>>> Any idea about which generation this might apply to?
>> Just read the SDM sub-section "Large Page Size Considerations"
>> inside the section on MTRRs.
> 
> Right, and all that says is "don't accidentally mix cacheabilities
> between paging and MTRRs".

Exactly. But we don't check back with the MTRRs when deciding
whether to re-combine a large page. Hence that flag to prevent
any such attempt.

Plus it also says something about a performance impact when
nevertheless using a large page for the first 2 or 4 MB.

Jan
Andrew Cooper Feb. 24, 2016, 5:28 p.m. UTC | #10
On 24/02/16 16:59, Jan Beulich wrote:
>>>> On 24.02.16 at 17:14, <andrew.cooper3@citrix.com> wrote:
>> On 24/02/16 15:48, Jan Beulich wrote:
>>>>>> On 24.02.16 at 16:22, <andrew.cooper3@citrix.com> wrote:
>>>> On 24/02/16 15:18, Jan Beulich wrote:
>>>>>>>> On 24.02.16 at 15:58, <andrew.cooper3@citrix.com> wrote:
>>>>>> On 24/02/16 14:15, Jan Beulich wrote:
>>>>>>>>>> On 24.02.16 at 14:57, <andrew.cooper3@citrix.com> wrote:
>>>>>>>> On 24/02/16 11:24, Jan Beulich wrote:
>>>>>>>>>  >>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>>>>>>>>>  GLOBAL(l1_identmap)
>>>>>>>>>> -        pfn = 0
>>>>>>>>>> +        idx = 0
>>>>>>>>>>          .rept L1_PAGETABLE_ENTRIES
>>>>>>>>>>          /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
>>>>>>>>>> -        .if pfn >= 0xa0 && pfn < 0xc0
>>>>>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
>>>>>>>>>> +        .if idx >= 0xa0 && idx < 0xc0
>>>>>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
>>>>>>>>>>          .else
>>>>>>>>>> -        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
>>>>>>>>>> +        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
>>>>>>>>> Please don't eliminate the MAP_SMALL_PAGES here, they serve an
>>>>>>>>> (at least documentation) purpose.
>>>>>>>> How?  Its in a l1 so are necessarily small pages, and the other l1's
>>>>>>>> don't use the constant.
>>>>>>> MAP_SMALL_PAGES documents (and enforces) that the mappings
>>>>>>> shouldn't be re-combined into 2M ones, even if - after adjustments
>>>>>>> to the other attributes - they could be.
>>>>>> In which case, is actively wrong.  Were the cacheabilities to change
>>>>>> (e.g. booting HVMLite and knowing that there was no legacy VGA hole),
>>>>>> the mappings should be recombined into a 2M superpage.
>>>>> No, I think there are reasons (to do with fixed range MTRRs and
>>>>> errata)
>>>> Any idea about which generation this might apply to?
>>> Just read the SDM sub-section "Large Page Size Considerations"
>>> inside the section on MTRRs.
>> Right, and all that says is "don't accidentally mix cacheabilities
>> between paging and MTRRs".
> Exactly. But we don't check back with the MTRRs when deciding
> whether to re-combine a large page. Hence that flag to prevent
> any such attempt.
>
> Plus it also says something about a performance impact when
> nevertheless using a large page for the first 2 or 4 MB.

The performance impact is only noted in relation to using "the most
conservative caching options".

I.e. if you use a 2M superpage and set UC because of mixed MTRRs, this
is a performance impact as the tradeoff against avoiding undefined
behaviour.

I will reinstate the bits for now, but I do intend them to be removed
longterm.

~Andrew
diff mbox

Patch

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 94cf089..cc10932 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -86,32 +86,40 @@  GLOBAL(__page_tables_start)
  * Mapping of first 2 megabytes of memory. This is mapped with 4kB mappings
  * to avoid type conflicts with fixed-range MTRRs covering the lowest megabyte
  * of physical memory. In any case the VGA hole should be mapped with type UC.
+ * Uses 1x 4k page.
  */
 GLOBAL(l1_identmap)
-        pfn = 0
+        idx = 0
         .rept L1_PAGETABLE_ENTRIES
         /* VGA hole (0xa0000-0xc0000) should be mapped UC. */
-        .if pfn >= 0xa0 && pfn < 0xc0
-        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE | MAP_SMALL_PAGES
+        .if idx >= 0xa0 && idx < 0xc0
+        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR_NOCACHE
         .else
-        .long (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
+        .quad (idx << PAGE_SHIFT) | PAGE_HYPERVISOR
         .endif
-        .long 0
-        pfn = pfn + 1
+        idx = idx + 1
         .endr
         .size l1_identmap, . - l1_identmap
 
-/* Mapping of first 16 megabytes of memory. */
+/*
+ * Space for mapping the first 4GB of memory, with the first 16 megabytes
+ * actualy mapped (mostly using superpages).  Uses 4x 4k pages.
+ */
 GLOBAL(l2_identmap)
-        .quad sym_phys(l1_identmap) + __PAGE_HYPERVISOR
-        pfn = 0
+        .quad sym_phys(l1_identmap) + PAGE_HYPERVISOR
+        idx = 1
         .rept 7
-        pfn = pfn + (1 << PAGETABLE_ORDER)
-        .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | _PAGE_PSE
+        .quad (idx << L2_PAGETABLE_SHIFT) | PAGE_HYPERVISOR | _PAGE_PSE
+        idx = idx + 1
         .endr
         .fill 4 * L2_PAGETABLE_ENTRIES - 8, 8, 0
         .size l2_identmap, . - l2_identmap
 
+/*
+ * L2 mapping the 1GB Xen text/data/bss region.  At boot it maps 16MB from
+ * __image_base__, and is modified when Xen relocates itself.  Uses 1x 4k
+ * page.
+ */
 GLOBAL(l2_xenmap)
         idx = 0
         .rept 8
@@ -121,11 +129,12 @@  GLOBAL(l2_xenmap)
         .fill L2_PAGETABLE_ENTRIES - 8, 8, 0
         .size l2_xenmap, . - l2_xenmap
 
+/* L2 mapping the fixmap.  Uses 1x 4k page. */
 l2_fixmap:
         idx = 0
         .rept L2_PAGETABLE_ENTRIES
         .if idx == l2_table_offset(FIXADDR_TOP - 1)
-        .quad sym_phys(l1_fixmap) + __PAGE_HYPERVISOR
+        .quad sym_phys(l1_fixmap) + PAGE_HYPERVISOR
         .else
         .quad 0
         .endif
@@ -133,22 +142,24 @@  l2_fixmap:
         .endr
         .size l2_fixmap, . - l2_fixmap
 
+/* Identity map, covering the 4 l2_identmap tables.  Uses 1x 4k page. */
 GLOBAL(l3_identmap)
         idx = 0
         .rept 4
-        .quad sym_phys(l2_identmap) + (idx << PAGE_SHIFT) + __PAGE_HYPERVISOR
+        .quad sym_phys(l2_identmap) + (idx << PAGE_SHIFT) + PAGE_HYPERVISOR
         idx = idx + 1
         .endr
         .fill L3_PAGETABLE_ENTRIES - 4, 8, 0
         .size l3_identmap, . - l3_identmap
 
+/* L3 mapping the fixmap.  Uses 1x 4k page. */
 l3_xenmap:
         idx = 0
         .rept L3_PAGETABLE_ENTRIES
         .if idx == l3_table_offset(XEN_VIRT_START)
-        .quad sym_phys(l2_xenmap) + __PAGE_HYPERVISOR
+        .quad sym_phys(l2_xenmap) + PAGE_HYPERVISOR
         .elseif idx == l3_table_offset(FIXADDR_TOP - 1)
-        .quad sym_phys(l2_fixmap) + __PAGE_HYPERVISOR
+        .quad sym_phys(l2_fixmap) + PAGE_HYPERVISOR
         .else
         .quad 0
         .endif
@@ -162,9 +173,9 @@  GLOBAL(idle_pg_table)
         idx = 1
         .rept L4_PAGETABLE_ENTRIES - 1
         .if idx == l4_table_offset(DIRECTMAP_VIRT_START)
-        .quad sym_phys(l3_identmap) + __PAGE_HYPERVISOR
+        .quad sym_phys(l3_identmap) + PAGE_HYPERVISOR
         .elseif idx == l4_table_offset(XEN_VIRT_START)
-        .quad sym_phys(l3_xenmap) + __PAGE_HYPERVISOR
+        .quad sym_phys(l3_xenmap) + PAGE_HYPERVISOR
         .else
         .quad 0
         .endif