[PATCH] x86/mm: Clarify comment in create_pae_xen_mappings()

[PATCH] x86/mm: Clarify comment in create_pae_xen_mappings()

[Andrew Cooper]

"3rd slot" is confusing, because it is only correct if you start counting from
the 0-th slot.  Most people would expect this to be phrased as "4th slot", but
switch to the entirely unambiguous "slot 3" which is also in line with the
adjacent code.

While fixing that, update the comment to indicate that this is leftover
behaviour from the 32bit Xen days, and exists these days only for ABI
compatibility.

Reported-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/mm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 28a0030..254ccca 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1444,7 +1444,13 @@ static int create_pae_xen_mappings(struct domain *d, l3_pgentry_t *pl3e)
 
     pl3e = (l3_pgentry_t *)((unsigned long)pl3e & PAGE_MASK);
 
-    /* 3rd L3 slot contains L2 with Xen-private mappings. It *must* exist. */
+    /*
+     * L3 slot 3 contains an L2 with Xen mappings.
+     *
+     * For 32-bit builds of Xen, it was critical that this mapping existed.
+     * Now that Xen is 64-bit only, there is no such requirement, but the
+     * behaviour is retained to keep the ABI consistent for 32-bit PV guests.
+     */
     l3e3 = pl3e[3];
     if ( !(l3e_get_flags(l3e3) & _PAGE_PRESENT) )
     {
-- 
2.1.4


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

Re: [PATCH] x86/mm: Clarify comment in create_pae_xen_mappings()

[Jan Beulich]

>>> On 05.12.18 at 20:45, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1444,7 +1444,13 @@ static int create_pae_xen_mappings(struct domain *d, l3_pgentry_t *pl3e)
>  
>      pl3e = (l3_pgentry_t *)((unsigned long)pl3e & PAGE_MASK);
>  
> -    /* 3rd L3 slot contains L2 with Xen-private mappings. It *must* exist. */
> +    /*
> +     * L3 slot 3 contains an L2 with Xen mappings.
> +     *
> +     * For 32-bit builds of Xen, it was critical that this mapping existed.
> +     * Now that Xen is 64-bit only, there is no such requirement, but the
> +     * behaviour is retained to keep the ABI consistent for 32-bit PV guests.
> +     */
>      l3e3 = pl3e[3];
>      if ( !(l3e_get_flags(l3e3) & _PAGE_PRESENT) )
>      {

Weakening the comment a little is fine by me, but I think you go
too far: 32-bit guests won't work without them putting a present
entry in slot 3. They won't have an M2P available without it. Also
note how alloc_l3_table() would fail without such an entry.

Jan



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