[Xen-devel] [PATCH 0/5] use new API for Xen page tables

[Xen-devel] [PATCH 0/5] use new API for Xen page tables

[Hongyan Xia]

From: Hongyan Xia <hongyxia@amazon.com>

This small series is basically just rewriting functions using the new
API to map and unmap PTEs. Each patch is independent.

Apart from mapping and unmapping page tables, no other functional change
intended.

Wei Liu (5):
  x86/shim: map and unmap page tables in replace_va_mapping
  x86_64/mm: map and unmap page tables in m2p_mapped
  x86_64/mm: map and unmap page tables in share_hotadd_m2p_table
  x86_64/mm: map and unmap page tables in destroy_compat_m2p_mapping
  x86_64/mm: map and unmap page tables in destroy_m2p_mapping

 xen/arch/x86/pv/shim.c     | 10 ++++---
 xen/arch/x86/x86_64/mm.c   | 55 +++++++++++++++++++++++++-------------
 xen/include/asm-x86/page.h | 18 +++++++++++++
 3 files changed, 62 insertions(+), 21 deletions(-)

-- 
2.24.1.AMZN


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

[Xen-devel] [PATCH 1/5] x86/shim: map and unmap page tables in replace_va_mapping

[Hongyan Xia]

From: Wei Liu <wei.liu2@citrix.com>

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Hongyan Xia <hongyxia@amazon.com>
---
 xen/arch/x86/pv/shim.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index ed2ece8a8a..1229d5ffb3 100644
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -169,15 +169,19 @@ static void __init replace_va_mapping(struct domain *d, l4_pgentry_t *l4start,
                                       unsigned long va, mfn_t mfn)
 {
     l4_pgentry_t *pl4e = l4start + l4_table_offset(va);
-    l3_pgentry_t *pl3e = l4e_to_l3e(*pl4e) + l3_table_offset(va);
-    l2_pgentry_t *pl2e = l3e_to_l2e(*pl3e) + l2_table_offset(va);
-    l1_pgentry_t *pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(va);
+    l3_pgentry_t *pl3e = map_l3t_from_l4e(*pl4e) + l3_table_offset(va);
+    l2_pgentry_t *pl2e = map_l2t_from_l3e(*pl3e) + l2_table_offset(va);
+    l1_pgentry_t *pl1e = map_l1t_from_l2e(*pl2e) + l1_table_offset(va);
     struct page_info *page = mfn_to_page(l1e_get_mfn(*pl1e));
 
     put_page_and_type(page);
 
     *pl1e = l1e_from_mfn(mfn, (!is_pv_32bit_domain(d) ? L1_PROT
                                                       : COMPAT_L1_PROT));
+
+    UNMAP_DOMAIN_PAGE(pl1e);
+    UNMAP_DOMAIN_PAGE(pl2e);
+    UNMAP_DOMAIN_PAGE(pl3e);
 }
 
 static void evtchn_reserve(struct domain *d, unsigned int port)
-- 
2.24.1.AMZN


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

Re: [PATCH 1/5] x86/shim: map and unmap page tables in replace_va_mapping

[Jan Beulich]

On 23.03.2020 10:41, Hongyan Xia wrote:
> --- a/xen/arch/x86/pv/shim.c
> +++ b/xen/arch/x86/pv/shim.c
> @@ -169,15 +169,19 @@ static void __init replace_va_mapping(struct domain *d, l4_pgentry_t *l4start,
>                                        unsigned long va, mfn_t mfn)
>  {
>      l4_pgentry_t *pl4e = l4start + l4_table_offset(va);
> -    l3_pgentry_t *pl3e = l4e_to_l3e(*pl4e) + l3_table_offset(va);
> -    l2_pgentry_t *pl2e = l3e_to_l2e(*pl3e) + l2_table_offset(va);
> -    l1_pgentry_t *pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(va);
> +    l3_pgentry_t *pl3e = map_l3t_from_l4e(*pl4e) + l3_table_offset(va);
> +    l2_pgentry_t *pl2e = map_l2t_from_l3e(*pl3e) + l2_table_offset(va);
> +    l1_pgentry_t *pl1e = map_l1t_from_l2e(*pl2e) + l1_table_offset(va);
>      struct page_info *page = mfn_to_page(l1e_get_mfn(*pl1e));
>  
>      put_page_and_type(page);
>  
>      *pl1e = l1e_from_mfn(mfn, (!is_pv_32bit_domain(d) ? L1_PROT
>                                                        : COMPAT_L1_PROT));
> +
> +    UNMAP_DOMAIN_PAGE(pl1e);
> +    UNMAP_DOMAIN_PAGE(pl2e);
> +    UNMAP_DOMAIN_PAGE(pl3e);
>  }

I disagree to an approach like this: Why have three pending mappings
when one at a time will do? Map-read/write-unmap three times is what
you want here, even if this is more code churn.

Jan

[Xen-devel] [PATCH 2/5] x86_64/mm: map and unmap page tables in m2p_mapped

[Hongyan Xia]

From: Wei Liu <wei.liu2@citrix.com>

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Hongyan Xia <hongyxia@amazon.com>
---
 xen/arch/x86/x86_64/mm.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index b7ce833ffc..a440dac25e 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -131,27 +131,33 @@ static int m2p_mapped(unsigned long spfn)
     unsigned long va;
     l3_pgentry_t *l3_ro_mpt;
     l2_pgentry_t *l2_ro_mpt;
+    int rc = M2P_NO_MAPPED;
 
     va = RO_MPT_VIRT_START + spfn * sizeof(*machine_to_phys_mapping);
-    l3_ro_mpt = l4e_to_l3e(idle_pg_table[l4_table_offset(va)]);
+    l3_ro_mpt = map_l3t_from_l4e(idle_pg_table[l4_table_offset(va)]);
 
     switch ( l3e_get_flags(l3_ro_mpt[l3_table_offset(va)]) &
              (_PAGE_PRESENT |_PAGE_PSE))
     {
         case _PAGE_PSE|_PAGE_PRESENT:
-            return M2P_1G_MAPPED;
+            rc = M2P_1G_MAPPED;
+            goto out;
         /* Check for next level */
         case _PAGE_PRESENT:
             break;
         default:
-            return M2P_NO_MAPPED;
+            rc = M2P_NO_MAPPED;
+            goto out;
     }
-    l2_ro_mpt = l3e_to_l2e(l3_ro_mpt[l3_table_offset(va)]);
+    l2_ro_mpt = map_l2t_from_l3e(l3_ro_mpt[l3_table_offset(va)]);
 
     if (l2e_get_flags(l2_ro_mpt[l2_table_offset(va)]) & _PAGE_PRESENT)
-        return M2P_2M_MAPPED;
+        rc = M2P_2M_MAPPED;
+    UNMAP_DOMAIN_PAGE(l2_ro_mpt);
 
-    return M2P_NO_MAPPED;
+ out:
+    UNMAP_DOMAIN_PAGE(l3_ro_mpt);
+    return rc;
 }
 
 static int share_hotadd_m2p_table(struct mem_hotadd_info *info)
-- 
2.24.1.AMZN


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

Re: [PATCH 2/5] x86_64/mm: map and unmap page tables in m2p_mapped

[Jan Beulich]

On 23.03.2020 10:41, Hongyan Xia wrote:
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -131,27 +131,33 @@ static int m2p_mapped(unsigned long spfn)
>      unsigned long va;
>      l3_pgentry_t *l3_ro_mpt;
>      l2_pgentry_t *l2_ro_mpt;
> +    int rc = M2P_NO_MAPPED;
>  
>      va = RO_MPT_VIRT_START + spfn * sizeof(*machine_to_phys_mapping);
> -    l3_ro_mpt = l4e_to_l3e(idle_pg_table[l4_table_offset(va)]);
> +    l3_ro_mpt = map_l3t_from_l4e(idle_pg_table[l4_table_offset(va)]);

Along the lines of what I've said for patch 1 - read the l3e
here and unmap again right away. No need for converting
"return" to "goto" further down. Same for the l2e then.

Jan

[Xen-devel] [PATCH 3/5] x86_64/mm: map and unmap page tables in share_hotadd_m2p_table

[Hongyan Xia]

From: Wei Liu <wei.liu2@citrix.com>

Fetch lYe by mapping and unmapping lXe instead of using the direct map,
which is now done via the new lYe_from_lXe() helpers.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
---
 xen/arch/x86/x86_64/mm.c   | 12 ++++++------
 xen/include/asm-x86/page.h | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index a440dac25e..2680173fab 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -173,14 +173,14 @@ static int share_hotadd_m2p_table(struct mem_hotadd_info *info)
           v += n << PAGE_SHIFT )
     {
         n = L2_PAGETABLE_ENTRIES * L1_PAGETABLE_ENTRIES;
-        l3e = l4e_to_l3e(idle_pg_table[l4_table_offset(v)])[
-            l3_table_offset(v)];
+        l3e = l3e_from_l4e(idle_pg_table[l4_table_offset(v)],
+                           l3_table_offset(v));
         if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) )
             continue;
         if ( !(l3e_get_flags(l3e) & _PAGE_PSE) )
         {
             n = L1_PAGETABLE_ENTRIES;
-            l2e = l3e_to_l2e(l3e)[l2_table_offset(v)];
+            l2e = l2e_from_l3e(l3e, l2_table_offset(v));
             if ( !(l2e_get_flags(l2e) & _PAGE_PRESENT) )
                 continue;
             m2p_start_mfn = l2e_get_mfn(l2e);
@@ -201,11 +201,11 @@ static int share_hotadd_m2p_table(struct mem_hotadd_info *info)
           v != RDWR_COMPAT_MPT_VIRT_END;
           v += 1 << L2_PAGETABLE_SHIFT )
     {
-        l3e = l4e_to_l3e(idle_pg_table[l4_table_offset(v)])[
-            l3_table_offset(v)];
+        l3e = l3e_from_l4e(idle_pg_table[l4_table_offset(v)],
+                           l3_table_offset(v));
         if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) )
             continue;
-        l2e = l3e_to_l2e(l3e)[l2_table_offset(v)];
+        l2e = l2e_from_l3e(l3e, l2_table_offset(v));
         if ( !(l2e_get_flags(l2e) & _PAGE_PRESENT) )
             continue;
         m2p_start_mfn = l2e_get_mfn(l2e);
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index c98d8f5ede..d4752a5925 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -196,6 +196,24 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa, unsigned int flags)
 #define map_l2t_from_l3e(x)        (l2_pgentry_t *)map_domain_page(l3e_get_mfn(x))
 #define map_l3t_from_l4e(x)        (l3_pgentry_t *)map_domain_page(l4e_get_mfn(x))
 
+#define l1e_from_l2e(l2e, off) ({                   \
+        l1_pgentry_t *l1t = map_l1t_from_l2e(l2e);  \
+        l1_pgentry_t l1e = l1t[off];                \
+        UNMAP_DOMAIN_PAGE(l1t);                     \
+        l1e; })
+
+#define l2e_from_l3e(l3e, off) ({                   \
+        l2_pgentry_t *l2t = map_l2t_from_l3e(l3e);  \
+        l2_pgentry_t l2e = l2t[off];                \
+        UNMAP_DOMAIN_PAGE(l2t);                     \
+        l2e; })
+
+#define l3e_from_l4e(l4e, off) ({                   \
+        l3_pgentry_t *l3t = map_l3t_from_l4e(l4e);  \
+        l3_pgentry_t l3e = l3t[off];                \
+        UNMAP_DOMAIN_PAGE(l3t);                     \
+        l3e; })
+
 /* Given a virtual address, get an entry offset into a page table. */
 #define l1_table_offset(a)         \
     (((a) >> L1_PAGETABLE_SHIFT) & (L1_PAGETABLE_ENTRIES - 1))
-- 
2.24.1.AMZN


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

Re: [PATCH 3/5] x86_64/mm: map and unmap page tables in share_hotadd_m2p_table

[Jan Beulich]

On 23.03.2020 10:41, Hongyan Xia wrote:
> --- a/xen/include/asm-x86/page.h
> +++ b/xen/include/asm-x86/page.h
> @@ -196,6 +196,24 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa, unsigned int flags)
>  #define map_l2t_from_l3e(x)        (l2_pgentry_t *)map_domain_page(l3e_get_mfn(x))
>  #define map_l3t_from_l4e(x)        (l3_pgentry_t *)map_domain_page(l4e_get_mfn(x))
>  
> +#define l1e_from_l2e(l2e, off) ({                   \
> +        l1_pgentry_t *l1t = map_l1t_from_l2e(l2e);  \
> +        l1_pgentry_t l1e = l1t[off];                \
> +        UNMAP_DOMAIN_PAGE(l1t);                     \
> +        l1e; })
> +
> +#define l2e_from_l3e(l3e, off) ({                   \
> +        l2_pgentry_t *l2t = map_l2t_from_l3e(l3e);  \
> +        l2_pgentry_t l2e = l2t[off];                \
> +        UNMAP_DOMAIN_PAGE(l2t);                     \
> +        l2e; })
> +
> +#define l3e_from_l4e(l4e, off) ({                   \
> +        l3_pgentry_t *l3t = map_l3t_from_l4e(l4e);  \
> +        l3_pgentry_t l3e = l3t[off];                \
> +        UNMAP_DOMAIN_PAGE(l3t);                     \
> +        l3e; })

There's a reason these are macros rather than inline functions,
I assume? (This reason would be nice to be stated in the
description.)

I don't see why you use UNMAP_DOMAIN_PAGE() rather than
unmap_domain_page() here - the variables in question go out of
scope right afterwards, so the storing of NULL into them is
pointless (and very likely to be eliminated by the compiler
anyway).

Finally the pointers should each be "to const", as you're
only reading through them.

Jan

Re: [PATCH 3/5] x86_64/mm: map and unmap page tables in share_hotadd_m2p_table

[Hongyan Xia]

On Wed, 2020-04-01 at 14:29 +0200, Jan Beulich wrote:
> On 23.03.2020 10:41, Hongyan Xia wrote:
> > --- a/xen/include/asm-x86/page.h
> > +++ b/xen/include/asm-x86/page.h
> > @@ -196,6 +196,24 @@ static inline l4_pgentry_t
> > l4e_from_paddr(paddr_t pa, unsigned int flags)
> >  #define map_l2t_from_l3e(x)        (l2_pgentry_t
> > *)map_domain_page(l3e_get_mfn(x))
> >  #define map_l3t_from_l4e(x)        (l3_pgentry_t
> > *)map_domain_page(l4e_get_mfn(x))
> >  
> > +#define l1e_from_l2e(l2e, off) ({                   \
> > +        l1_pgentry_t *l1t = map_l1t_from_l2e(l2e);  \
> > +        l1_pgentry_t l1e = l1t[off];                \
> > +        UNMAP_DOMAIN_PAGE(l1t);                     \
> > +        l1e; })
> > +
> > +#define l2e_from_l3e(l3e, off) ({                   \
> > +        l2_pgentry_t *l2t = map_l2t_from_l3e(l3e);  \
> > +        l2_pgentry_t l2e = l2t[off];                \
> > +        UNMAP_DOMAIN_PAGE(l2t);                     \
> > +        l2e; })
> > +
> > +#define l3e_from_l4e(l4e, off) ({                   \
> > +        l3_pgentry_t *l3t = map_l3t_from_l4e(l4e);  \
> > +        l3_pgentry_t l3e = l3t[off];                \
> > +        UNMAP_DOMAIN_PAGE(l3t);                     \
> > +        l3e; })
> 
> There's a reason these are macros rather than inline functions,
> I assume? (This reason would be nice to be stated in the
> description.)

Actually no specific reasons, just to keep them as macros to be
consistent with other helpers above. Fairly trivial to convert them
into inline functions if this is desired.

> I don't see why you use UNMAP_DOMAIN_PAGE() rather than
> unmap_domain_page() here - the variables in question go out of
> scope right afterwards, so the storing of NULL into them is
> pointless (and very likely to be eliminated by the compiler
> anyway).

My intention is to just avoid using the lower case one altogether, so
that one day when we need to expand any function or macros we do not
need to look back at the code and worry about whether any lower case
ones need to be converted to upper case (sometimes for large functions
this may not be trivial), and the compiler can deal with the extra
nullification anyway.

> Finally the pointers should each be "to const", as you're
> only reading through them.

Good point. Will const qualify in next revision.

Thanks,
Hongyan

Re: [PATCH 3/5] x86_64/mm: map and unmap page tables in share_hotadd_m2p_table

[Jan Beulich]

On 07.04.2020 17:11, Hongyan Xia wrote:
> On Wed, 2020-04-01 at 14:29 +0200, Jan Beulich wrote:
>> On 23.03.2020 10:41, Hongyan Xia wrote:
>>> --- a/xen/include/asm-x86/page.h
>>> +++ b/xen/include/asm-x86/page.h
>>> @@ -196,6 +196,24 @@ static inline l4_pgentry_t
>>> l4e_from_paddr(paddr_t pa, unsigned int flags)
>>>  #define map_l2t_from_l3e(x)        (l2_pgentry_t
>>> *)map_domain_page(l3e_get_mfn(x))
>>>  #define map_l3t_from_l4e(x)        (l3_pgentry_t
>>> *)map_domain_page(l4e_get_mfn(x))
>>>  
>>> +#define l1e_from_l2e(l2e, off) ({                   \
>>> +        l1_pgentry_t *l1t = map_l1t_from_l2e(l2e);  \
>>> +        l1_pgentry_t l1e = l1t[off];                \
>>> +        UNMAP_DOMAIN_PAGE(l1t);                     \
>>> +        l1e; })
>>> +
>>> +#define l2e_from_l3e(l3e, off) ({                   \
>>> +        l2_pgentry_t *l2t = map_l2t_from_l3e(l3e);  \
>>> +        l2_pgentry_t l2e = l2t[off];                \
>>> +        UNMAP_DOMAIN_PAGE(l2t);                     \
>>> +        l2e; })
>>> +
>>> +#define l3e_from_l4e(l4e, off) ({                   \
>>> +        l3_pgentry_t *l3t = map_l3t_from_l4e(l4e);  \
>>> +        l3_pgentry_t l3e = l3t[off];                \
>>> +        UNMAP_DOMAIN_PAGE(l3t);                     \
>>> +        l3e; })
>>
>> There's a reason these are macros rather than inline functions,
>> I assume? (This reason would be nice to be stated in the
>> description.)
> 
> Actually no specific reasons, just to keep them as macros to be
> consistent with other helpers above. Fairly trivial to convert them
> into inline functions if this is desired.

Where possible this would be the preferred route for new helpers.

>> I don't see why you use UNMAP_DOMAIN_PAGE() rather than
>> unmap_domain_page() here - the variables in question go out of
>> scope right afterwards, so the storing of NULL into them is
>> pointless (and very likely to be eliminated by the compiler
>> anyway).
> 
> My intention is to just avoid using the lower case one altogether, so
> that one day when we need to expand any function or macros we do not
> need to look back at the code and worry about whether any lower case
> ones need to be converted to upper case (sometimes for large functions
> this may not be trivial), and the compiler can deal with the extra
> nullification anyway.

I don't agree with this goal; perhaps others on Cc have an opinion?

Jan

Re: [PATCH 3/5] x86_64/mm: map and unmap page tables in share_hotadd_m2p_table

[Hongyan Xia]

On Wed, 2020-04-01 at 14:29 +0200, Jan Beulich wrote:
> On 23.03.2020 10:41, Hongyan Xia wrote:
> > --- a/xen/include/asm-x86/page.h
> > +++ b/xen/include/asm-x86/page.h
> > @@ -196,6 +196,24 @@ static inline l4_pgentry_t
> > l4e_from_paddr(paddr_t pa, unsigned int flags)
> >  #define map_l2t_from_l3e(x)        (l2_pgentry_t
> > *)map_domain_page(l3e_get_mfn(x))
> >  #define map_l3t_from_l4e(x)        (l3_pgentry_t
> > *)map_domain_page(l4e_get_mfn(x))
> >  
> > +#define l1e_from_l2e(l2e, off) ({                   \
> > +        l1_pgentry_t *l1t = map_l1t_from_l2e(l2e);  \
> > +        l1_pgentry_t l1e = l1t[off];                \
> > +        UNMAP_DOMAIN_PAGE(l1t);                     \
> > +        l1e; })
> > +
> > +#define l2e_from_l3e(l3e, off) ({                   \
> > +        l2_pgentry_t *l2t = map_l2t_from_l3e(l3e);  \
> > +        l2_pgentry_t l2e = l2t[off];                \
> > +        UNMAP_DOMAIN_PAGE(l2t);                     \
> > +        l2e; })
> > +
> > +#define l3e_from_l4e(l4e, off) ({                   \
> > +        l3_pgentry_t *l3t = map_l3t_from_l4e(l4e);  \
> > +        l3_pgentry_t l3e = l3t[off];                \
> > +        UNMAP_DOMAIN_PAGE(l3t);                     \
> > +        l3e; })
> 
> There's a reason these are macros rather than inline functions,
> I assume? (This reason would be nice to be stated in the
> description.)

While converting them into inline functions, I realised I cannot do
that due to the header mess. Converting into inline functions needs the
domain_page.h header, which opens a can of worms if I include it here
(page.h). Keeping them as macros works around this issue.

I will add this in the description.

Hongyan

[Xen-devel] [PATCH 4/5] x86_64/mm: map and unmap page tables in destroy_compat_m2p_mapping

[Hongyan Xia]

From: Wei Liu <wei.liu2@citrix.com>

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Hongyan Xia <hongyxia@amazon.com>
---
 xen/arch/x86/x86_64/mm.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 2680173fab..71c84ac593 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -235,11 +235,13 @@ static void destroy_compat_m2p_mapping(struct mem_hotadd_info *info)
     if ( emap > ((RDWR_COMPAT_MPT_VIRT_END - RDWR_COMPAT_MPT_VIRT_START) >> 2) )
         emap = (RDWR_COMPAT_MPT_VIRT_END - RDWR_COMPAT_MPT_VIRT_START) >> 2;
 
-    l3_ro_mpt = l4e_to_l3e(idle_pg_table[l4_table_offset(HIRO_COMPAT_MPT_VIRT_START)]);
+    l3_ro_mpt = map_l3t_from_l4e(idle_pg_table[
+                    l4_table_offset(HIRO_COMPAT_MPT_VIRT_START)]);
 
     ASSERT(l3e_get_flags(l3_ro_mpt[l3_table_offset(HIRO_COMPAT_MPT_VIRT_START)]) & _PAGE_PRESENT);
 
-    l2_ro_mpt = l3e_to_l2e(l3_ro_mpt[l3_table_offset(HIRO_COMPAT_MPT_VIRT_START)]);
+    l2_ro_mpt = map_l2t_from_l3e(
+                    l3_ro_mpt[l3_table_offset(HIRO_COMPAT_MPT_VIRT_START)]);
 
     for ( i = smap; i < emap; )
     {
@@ -261,6 +263,9 @@ static void destroy_compat_m2p_mapping(struct mem_hotadd_info *info)
         i += 1UL << (L2_PAGETABLE_SHIFT - 2);
     }
 
+    UNMAP_DOMAIN_PAGE(l2_ro_mpt);
+    UNMAP_DOMAIN_PAGE(l3_ro_mpt);
+
     return;
 }
 
-- 
2.24.1.AMZN


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

[Xen-devel] [PATCH 5/5] x86_64/mm: map and unmap page tables in destroy_m2p_mapping

[Hongyan Xia]

From: Wei Liu <wei.liu2@citrix.com>

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Hongyan Xia <hongyxia@amazon.com>
---
 xen/arch/x86/x86_64/mm.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 71c84ac593..6a0ffe088b 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -275,7 +275,8 @@ static void destroy_m2p_mapping(struct mem_hotadd_info *info)
     unsigned long i, va, rwva;
     unsigned long smap = info->spfn, emap = info->epfn;
 
-    l3_ro_mpt = l4e_to_l3e(idle_pg_table[l4_table_offset(RO_MPT_VIRT_START)]);
+    l3_ro_mpt = map_l3t_from_l4e(
+                    idle_pg_table[l4_table_offset(RO_MPT_VIRT_START)]);
 
     /*
      * No need to clean m2p structure existing before the hotplug
@@ -297,26 +298,33 @@ static void destroy_m2p_mapping(struct mem_hotadd_info *info)
             continue;
         }
 
-        l2_ro_mpt = l3e_to_l2e(l3_ro_mpt[l3_table_offset(va)]);
+        l2_ro_mpt = map_l2t_from_l3e(l3_ro_mpt[l3_table_offset(va)]);
         if (!(l2e_get_flags(l2_ro_mpt[l2_table_offset(va)]) & _PAGE_PRESENT))
         {
             i = ( i & ~((1UL << (L2_PAGETABLE_SHIFT - 3)) - 1)) +
                     (1UL << (L2_PAGETABLE_SHIFT - 3)) ;
+            UNMAP_DOMAIN_PAGE(l2_ro_mpt);
             continue;
         }
 
         pt_pfn = l2e_get_pfn(l2_ro_mpt[l2_table_offset(va)]);
         if ( hotadd_mem_valid(pt_pfn, info) )
         {
+            l2_pgentry_t *l2t;
+
             destroy_xen_mappings(rwva, rwva + (1UL << L2_PAGETABLE_SHIFT));
 
-            l2_ro_mpt = l3e_to_l2e(l3_ro_mpt[l3_table_offset(va)]);
-            l2e_write(&l2_ro_mpt[l2_table_offset(va)], l2e_empty());
+            l2t = map_l2t_from_l3e(l3_ro_mpt[l3_table_offset(va)]);
+            l2e_write(&l2t[l2_table_offset(va)], l2e_empty());
+            UNMAP_DOMAIN_PAGE(l2t);
         }
         i = ( i & ~((1UL << (L2_PAGETABLE_SHIFT - 3)) - 1)) +
               (1UL << (L2_PAGETABLE_SHIFT - 3));
+        UNMAP_DOMAIN_PAGE(l2_ro_mpt);
     }
 
+    UNMAP_DOMAIN_PAGE(l3_ro_mpt);
+
     destroy_compat_m2p_mapping(info);
 
     /* Brute-Force flush all TLB */
-- 
2.24.1.AMZN


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

Re: [PATCH 5/5] x86_64/mm: map and unmap page tables in destroy_m2p_mapping

[Jan Beulich]

On 23.03.2020 10:41, Hongyan Xia wrote:
> @@ -297,26 +298,33 @@ static void destroy_m2p_mapping(struct mem_hotadd_info *info)
>              continue;
>          }
>  
> -        l2_ro_mpt = l3e_to_l2e(l3_ro_mpt[l3_table_offset(va)]);
> +        l2_ro_mpt = map_l2t_from_l3e(l3_ro_mpt[l3_table_offset(va)]);
>          if (!(l2e_get_flags(l2_ro_mpt[l2_table_offset(va)]) & _PAGE_PRESENT))
>          {
>              i = ( i & ~((1UL << (L2_PAGETABLE_SHIFT - 3)) - 1)) +
>                      (1UL << (L2_PAGETABLE_SHIFT - 3)) ;
> +            UNMAP_DOMAIN_PAGE(l2_ro_mpt);
>              continue;
>          }
>  
>          pt_pfn = l2e_get_pfn(l2_ro_mpt[l2_table_offset(va)]);
>          if ( hotadd_mem_valid(pt_pfn, info) )
>          {
> +            l2_pgentry_t *l2t;
> +
>              destroy_xen_mappings(rwva, rwva + (1UL << L2_PAGETABLE_SHIFT));
>  
> -            l2_ro_mpt = l3e_to_l2e(l3_ro_mpt[l3_table_offset(va)]);
> -            l2e_write(&l2_ro_mpt[l2_table_offset(va)], l2e_empty());
> +            l2t = map_l2t_from_l3e(l3_ro_mpt[l3_table_offset(va)]);

Why a 2nd mapping of the same L3 entry that you've already mapped
into l2_ro_mpt?

> +            l2e_write(&l2t[l2_table_offset(va)], l2e_empty());
> +            UNMAP_DOMAIN_PAGE(l2t);

If this then weren't to go away, it should again be the lowercase
variant imo, as the variable's scope ends here.

Jan

Re: [PATCH 5/5] x86_64/mm: map and unmap page tables in destroy_m2p_mapping

[Hongyan Xia]

On Wed, 2020-04-01 at 14:40 +0200, Jan Beulich wrote:
> On 23.03.2020 10:41, Hongyan Xia wrote:
> > @@ -297,26 +298,33 @@ static void destroy_m2p_mapping(struct
> > mem_hotadd_info *info)
> >              continue;
> >          }
> >  
> > -        l2_ro_mpt = l3e_to_l2e(l3_ro_mpt[l3_table_offset(va)]);
> > +        l2_ro_mpt =
> > map_l2t_from_l3e(l3_ro_mpt[l3_table_offset(va)]);
> >          if (!(l2e_get_flags(l2_ro_mpt[l2_table_offset(va)]) &
> > _PAGE_PRESENT))
> >          {
> >              i = ( i & ~((1UL << (L2_PAGETABLE_SHIFT - 3)) - 1)) +
> >                      (1UL << (L2_PAGETABLE_SHIFT - 3)) ;
> > +            UNMAP_DOMAIN_PAGE(l2_ro_mpt);
> >              continue;
> >          }
> >  
> >          pt_pfn = l2e_get_pfn(l2_ro_mpt[l2_table_offset(va)]);
> >          if ( hotadd_mem_valid(pt_pfn, info) )
> >          {
> > +            l2_pgentry_t *l2t;
> > +
> >              destroy_xen_mappings(rwva, rwva + (1UL <<
> > L2_PAGETABLE_SHIFT));
> >  
> > -            l2_ro_mpt =
> > l3e_to_l2e(l3_ro_mpt[l3_table_offset(va)]);
> > -            l2e_write(&l2_ro_mpt[l2_table_offset(va)],
> > l2e_empty());
> > +            l2t =
> > map_l2t_from_l3e(l3_ro_mpt[l3_table_offset(va)]);
> 
> Why a 2nd mapping of the same L3 entry that you've already mapped
> into l2_ro_mpt?
> > +            l2e_write(&l2t[l2_table_offset(va)], l2e_empty());
> > +            UNMAP_DOMAIN_PAGE(l2t);
> 
> If this then weren't to go away, it should again be the lowercase
> variant imo, as the variable's scope ends here.

Hmm, I don't see a reason why l2_ro_mpt needs to be mapped again either
(and don't see why it was re-derived in the original code), so yes, I
think the map and unmap can just be dropped. Will revise.

Hongyan

Re: [Xen-devel] [PATCH 0/5] use new API for Xen page tables

[Wei Liu]

On Mon, Mar 23, 2020 at 09:41:37AM +0000, Hongyan Xia wrote:
> From: Hongyan Xia <hongyxia@amazon.com>
> 
> This small series is basically just rewriting functions using the new
> API to map and unmap PTEs. Each patch is independent.
> 
> Apart from mapping and unmapping page tables, no other functional change
> intended.
> 

The code looks correct to me.

I do wonder if you should've put your SoB instead of Rb in some of the
patches.

Wei.

Re: [Xen-devel] [PATCH 0/5] use new API for Xen page tables

[Hongyan Xia]

On Sun, 2020-03-29 at 16:06 +0100, Wei Liu wrote:
> On Mon, Mar 23, 2020 at 09:41:37AM +0000, Hongyan Xia wrote:
> > From: Hongyan Xia <hongyxia@amazon.com>
> > 
> > This small series is basically just rewriting functions using the
> > new
> > API to map and unmap PTEs. Each patch is independent.
> > 
> > Apart from mapping and unmapping page tables, no other functional
> > change
> > intended.
> > 
> 
> The code looks correct to me.
> 
> I do wonder if you should've put your SoB instead of Rb in some of
> the
> patches.

I am not exactly sure what the policy is. For a couple of the patches,
I did not touch anything but just cherry-picked from your tree, and
what I did was just looking at the code and double checking, so I put
an Rb there. Will change to SoB if this is not how we do things.

Hongyan

Re: [Xen-devel] [PATCH 0/5] use new API for Xen page tables

[Hongyan Xia]

Ping.

On Mon, 2020-03-23 at 09:41 +0000, Hongyan Xia wrote:
> From: Hongyan Xia <hongyxia@amazon.com>
> 
> This small series is basically just rewriting functions using the new
> API to map and unmap PTEs. Each patch is independent.
> 
> Apart from mapping and unmapping page tables, no other functional
> change
> intended.
> 
> Wei Liu (5):
>   x86/shim: map and unmap page tables in replace_va_mapping
>   x86_64/mm: map and unmap page tables in m2p_mapped
>   x86_64/mm: map and unmap page tables in share_hotadd_m2p_table
>   x86_64/mm: map and unmap page tables in destroy_compat_m2p_mapping
>   x86_64/mm: map and unmap page tables in destroy_m2p_mapping
> 
>  xen/arch/x86/pv/shim.c     | 10 ++++---
>  xen/arch/x86/x86_64/mm.c   | 55 +++++++++++++++++++++++++-----------
> --
>  xen/include/asm-x86/page.h | 18 +++++++++++++
>  3 files changed, 62 insertions(+), 21 deletions(-)
> 

Re: [PATCH 0/5] use new API for Xen page tables

[Jan Beulich]

On 06.04.2020 10:27, Hongyan Xia wrote:
> Ping.

Does this somehow imply you didn't get my replies sent on the 1st?

Jan

> On Mon, 2020-03-23 at 09:41 +0000, Hongyan Xia wrote:
>> From: Hongyan Xia <hongyxia@amazon.com>
>>
>> This small series is basically just rewriting functions using the new
>> API to map and unmap PTEs. Each patch is independent.
>>
>> Apart from mapping and unmapping page tables, no other functional
>> change
>> intended.
>>
>> Wei Liu (5):
>>   x86/shim: map and unmap page tables in replace_va_mapping
>>   x86_64/mm: map and unmap page tables in m2p_mapped
>>   x86_64/mm: map and unmap page tables in share_hotadd_m2p_table
>>   x86_64/mm: map and unmap page tables in destroy_compat_m2p_mapping
>>   x86_64/mm: map and unmap page tables in destroy_m2p_mapping
>>
>>  xen/arch/x86/pv/shim.c     | 10 ++++---
>>  xen/arch/x86/x86_64/mm.c   | 55 +++++++++++++++++++++++++-----------
>> --
>>  xen/include/asm-x86/page.h | 18 +++++++++++++
>>  3 files changed, 62 insertions(+), 21 deletions(-)
>>
> 

Re: [PATCH 0/5] use new API for Xen page tables

[Hongyan Xia]

On Mon, 2020-04-06 at 13:03 +0200, Jan Beulich wrote:
> On 06.04.2020 10:27, Hongyan Xia wrote:
> > Ping.
> 
> Does this somehow imply you didn't get my replies sent on the 1st?
> 
> Jan

Apologies. Somehow your replies ended up in a separate thread. There is
a problem with my email client.

Hongyan