[Xen-devel] [PATCH 0/4] Post-299 cleanups

[Xen-devel] [PATCH 0/4] Post-299 cleanups

[George Dunlap]

This series implements a number of cleanups to make the code simpler
and easier to follow.  No functional changes intended.

George Dunlap (4):
  x86/mm: Refactor put_page_from_l*e to reduce code duplication
  x86/mm: Implement common put_data_pages for put_page_from_l[23]e
  x86/mm: Use a more descriptive name for pagetable mfns
  x86/mm: More discriptive names for page de/validation functions

 xen/arch/x86/domain.c    |   2 +-
 xen/arch/x86/mm.c        | 243 ++++++++++++++++-----------------------
 xen/include/asm-x86/mm.h |   4 +-
 3 files changed, 102 insertions(+), 147 deletions(-)

--
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>

2.24.0

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

[Xen-devel] [PATCH 1/4] x86/mm: Refactor put_page_from_l*e to reduce code duplication

[George Dunlap]

put_page_from_l[234]e have identical functionality for devalidating an
N-1 entry which is a pagetable.  But mystifyingly, they duplicate the
code in slightly different arrangements that make it hard to tell that
it's the same.

Create a new function, put_pt_page(), which handles the common
functionality; and refactor all the functions to be symmetric,
differing only in the level of pagetable expected (and in whether they
handle superpages).

No functional change intended.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/mm.c | 89 +++++++++++++++--------------------------------
 1 file changed, 28 insertions(+), 61 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 7d4dd80a85..d8a0eb2aa5 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1297,6 +1297,28 @@ static void put_data_page(struct page_info *page, bool writeable)
         put_page(page);
 }
 
+static int put_pt_page(struct page_info *pg, struct page_info *ptpg,
+                       unsigned int flags)
+{
+    int rc = 0;
+
+    if ( flags & PTF_defer )
+    {
+        ASSERT(!(flags & PTF_partial_set));
+        current->arch.old_guest_ptpg = ptpg;
+        current->arch.old_guest_table = pg;
+        current->arch.old_guest_table_partial = false;
+    }
+    else
+    {
+        rc = _put_page_type(pg, flags | PTF_preemptible, ptpg);
+        if ( likely(!rc) )
+            put_page(pg);
+    }
+
+    return rc;
+}
+
 /*
  * NB. Virtual address 'l2e' maps to a machine address within frame 'pfn'.
  * Note also that this automatically deals correctly with linear p.t.'s.
@@ -1304,8 +1326,6 @@ static void put_data_page(struct page_info *page, bool writeable)
 static int put_page_from_l2e(l2_pgentry_t l2e, unsigned long pfn,
                              unsigned int flags)
 {
-    int rc = 0;
-
     if ( !(l2e_get_flags(l2e) & _PAGE_PRESENT) || (l2e_get_pfn(l2e) == pfn) )
         return 1;
 
@@ -1319,35 +1339,16 @@ static int put_page_from_l2e(l2_pgentry_t l2e, unsigned long pfn,
                  ((1UL << (L2_PAGETABLE_SHIFT - PAGE_SHIFT)) - 1)));
         for ( i = 0; i < (1u << PAGETABLE_ORDER); i++, page++ )
             put_data_page(page, writeable);
-    }
-    else
-    {
-        struct page_info *pg = l2e_get_page(l2e);
-        struct page_info *ptpg = mfn_to_page(_mfn(pfn));
 
-        if ( flags & PTF_defer )
-        {
-            current->arch.old_guest_ptpg = ptpg;
-            current->arch.old_guest_table = pg;
-            current->arch.old_guest_table_partial = false;
-        }
-        else
-        {
-            rc = _put_page_type(pg, flags | PTF_preemptible, ptpg);
-            if ( likely(!rc) )
-                put_page(pg);
-        }
+        return 0;
     }
 
-    return rc;
+    return put_pt_page(l2e_get_page(l2e), mfn_to_page(_mfn(pfn)), flags);
 }
 
 static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
                              unsigned int flags)
 {
-    struct page_info *pg;
-    int rc;
-
     if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) || (l3e_get_pfn(l3e) == pfn) )
         return 1;
 
@@ -1365,50 +1366,16 @@ static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
         return 0;
     }
 
-    pg = l3e_get_page(l3e);
-
-    if ( flags & PTF_defer )
-    {
-        ASSERT(!(flags & PTF_partial_set));
-        current->arch.old_guest_ptpg = mfn_to_page(_mfn(pfn));
-        current->arch.old_guest_table = pg;
-        current->arch.old_guest_table_partial = false;
-        return 0;
-    }
-
-    rc = _put_page_type(pg, flags | PTF_preemptible, mfn_to_page(_mfn(pfn)));
-    if ( likely(!rc) )
-        put_page(pg);
-
-    return rc;
+    return put_pt_page(l3e_get_page(l3e), mfn_to_page(_mfn(pfn)), flags);
 }
 
 static int put_page_from_l4e(l4_pgentry_t l4e, unsigned long pfn,
                              unsigned int flags)
 {
-    int rc = 1;
-
-    if ( (l4e_get_flags(l4e) & _PAGE_PRESENT) &&
-         (l4e_get_pfn(l4e) != pfn) )
-    {
-        struct page_info *pg = l4e_get_page(l4e);
-
-        if ( flags & PTF_defer )
-        {
-            ASSERT(!(flags & PTF_partial_set));
-            current->arch.old_guest_ptpg = mfn_to_page(_mfn(pfn));
-            current->arch.old_guest_table = pg;
-            current->arch.old_guest_table_partial = false;
-            return 0;
-        }
-
-        rc = _put_page_type(pg, flags | PTF_preemptible,
-                            mfn_to_page(_mfn(pfn)));
-        if ( likely(!rc) )
-            put_page(pg);
-    }
+    if ( !(l4e_get_flags(l4e) & _PAGE_PRESENT) || (l4e_get_pfn(l4e) == pfn) )
+        return 1;
 
-    return rc;
+    return put_pt_page(l4e_get_page(l4e), mfn_to_page(_mfn(pfn)), flags);
 }
 
 static int alloc_l1_table(struct page_info *page)
-- 
2.24.0


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

[Xen-devel] [PATCH 2/4] x86/mm: Implement common put_data_pages for put_page_from_l[23]e

[George Dunlap]

Both put_page_from_l2e and put_page_from_l3e handle having superpage
entries by looping over each page and "put"-ing each one individually.
As with putting page table entries, this code is functionally
identical, but for some reason different.  Moreover, there is already
a common function, put_data_page(), to handle automatically swapping
between put_page() (for read-only pages) or put_page_and_type() (for
read-write pages).

Replace this with put_data_pages() (plural), which does the entire
loop, as well as the put_page / put_page_and_type switch.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
NB that I've used the "simple for loop" version to make it easy to see
what's going on, rather than the "do { } while()" version which uses &
and compare to zero rather than comparing to the max.

CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/mm.c | 52 ++++++++++++++++++-----------------------------
 1 file changed, 20 insertions(+), 32 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d8a0eb2aa5..c05039ab21 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1289,14 +1289,6 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner)
 }
 
 #ifdef CONFIG_PV
-static void put_data_page(struct page_info *page, bool writeable)
-{
-    if ( writeable )
-        put_page_and_type(page);
-    else
-        put_page(page);
-}
-
 static int put_pt_page(struct page_info *pg, struct page_info *ptpg,
                        unsigned int flags)
 {
@@ -1319,6 +1311,20 @@ static int put_pt_page(struct page_info *pg, struct page_info *ptpg,
     return rc;
 }
 
+static int put_data_pages(struct page_info *page, bool writeable, int pt_shift)
+{
+    int i, count = 1 << (pt_shift - PAGE_SHIFT);
+
+    ASSERT(!(mfn_x(page_to_mfn(page)) & (count - 1)));
+    for ( i = 0; i < count ; i++, page++ )
+        if ( writeable )
+            put_page_and_type(page);
+        else
+            put_page(page);
+
+    return 0;
+}
+
 /*
  * NB. Virtual address 'l2e' maps to a machine address within frame 'pfn'.
  * Note also that this automatically deals correctly with linear p.t.'s.
@@ -1330,18 +1336,9 @@ static int put_page_from_l2e(l2_pgentry_t l2e, unsigned long pfn,
         return 1;
 
     if ( l2e_get_flags(l2e) & _PAGE_PSE )
-    {
-        struct page_info *page = l2e_get_page(l2e);
-        bool writeable = l2e_get_flags(l2e) & _PAGE_RW;
-        unsigned int i;
-
-        ASSERT(!(mfn_x(page_to_mfn(page)) &
-                 ((1UL << (L2_PAGETABLE_SHIFT - PAGE_SHIFT)) - 1)));
-        for ( i = 0; i < (1u << PAGETABLE_ORDER); i++, page++ )
-            put_data_page(page, writeable);
-
-        return 0;
-    }
+        return put_data_pages(l2e_get_page(l2e),
+                              l2e_get_flags(l2e) & _PAGE_RW,
+                              L2_PAGETABLE_SHIFT);
 
     return put_pt_page(l2e_get_page(l2e), mfn_to_page(_mfn(pfn)), flags);
 }
@@ -1353,18 +1350,9 @@ static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
         return 1;
 
     if ( unlikely(l3e_get_flags(l3e) & _PAGE_PSE) )
-    {
-        unsigned long mfn = l3e_get_pfn(l3e);
-        bool writeable = l3e_get_flags(l3e) & _PAGE_RW;
-
-        ASSERT(!(flags & PTF_partial_set));
-        ASSERT(!(mfn & ((1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) - 1)));
-        do {
-            put_data_page(mfn_to_page(_mfn(mfn)), writeable);
-        } while ( ++mfn & ((1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) - 1) );
-
-        return 0;
-    }
+        return put_data_pages(l3e_get_page(l3e),
+                              l3e_get_flags(l3e) & _PAGE_RW,
+                              L3_PAGETABLE_SHIFT);
 
     return put_pt_page(l3e_get_page(l3e), mfn_to_page(_mfn(pfn)), flags);
 }
-- 
2.24.0


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

[Xen-devel] [PATCH 3/4] x86/mm: Use a more descriptive name for pagetable mfns

[George Dunlap]

In many places, a PTE being modified is accompanied by the pagetable
mfn which contains the PTE (primarily in order to be able to maintain
linear mapping counts).  In many cases, this mfn is stored in the
non-descript variable (or argement) "pfn".

Replace these names with lNmfn, to indicate that 1) this is a
pagetable mfn, and 2) that it is the same level as the PTE in
question.  This should be enough to remind readers that it's the mfn
containing the PTE.

No functional change.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/mm.c | 50 +++++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index c05039ab21..54b4100d55 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1141,7 +1141,7 @@ static int get_page_and_type_from_mfn(
 define_get_linear_pagetable(l2);
 static int
 get_page_from_l2e(
-    l2_pgentry_t l2e, unsigned long pfn, struct domain *d, unsigned int flags)
+    l2_pgentry_t l2e, unsigned long l2mfn, struct domain *d, unsigned int flags)
 {
     unsigned long mfn = l2e_get_pfn(l2e);
     int rc;
@@ -1156,7 +1156,7 @@ get_page_from_l2e(
     ASSERT(!(flags & PTF_preemptible));
 
     rc = get_page_and_type_from_mfn(_mfn(mfn), PGT_l1_page_table, d, flags);
-    if ( unlikely(rc == -EINVAL) && get_l2_linear_pagetable(l2e, pfn, d) )
+    if ( unlikely(rc == -EINVAL) && get_l2_linear_pagetable(l2e, l2mfn, d) )
         rc = 0;
 
     return rc;
@@ -1165,7 +1165,7 @@ get_page_from_l2e(
 define_get_linear_pagetable(l3);
 static int
 get_page_from_l3e(
-    l3_pgentry_t l3e, unsigned long pfn, struct domain *d, unsigned int flags)
+    l3_pgentry_t l3e, unsigned long l3mfn, struct domain *d, unsigned int flags)
 {
     int rc;
 
@@ -1180,7 +1180,7 @@ get_page_from_l3e(
         l3e_get_mfn(l3e), PGT_l2_page_table, d, flags | PTF_preemptible);
     if ( unlikely(rc == -EINVAL) &&
          !is_pv_32bit_domain(d) &&
-         get_l3_linear_pagetable(l3e, pfn, d) )
+         get_l3_linear_pagetable(l3e, l3mfn, d) )
         rc = 0;
 
     return rc;
@@ -1189,7 +1189,7 @@ get_page_from_l3e(
 define_get_linear_pagetable(l4);
 static int
 get_page_from_l4e(
-    l4_pgentry_t l4e, unsigned long pfn, struct domain *d, unsigned int flags)
+    l4_pgentry_t l4e, unsigned long l4mfn, struct domain *d, unsigned int flags)
 {
     int rc;
 
@@ -1202,7 +1202,7 @@ get_page_from_l4e(
 
     rc = get_page_and_type_from_mfn(
         l4e_get_mfn(l4e), PGT_l3_page_table, d, flags | PTF_preemptible);
-    if ( unlikely(rc == -EINVAL) && get_l4_linear_pagetable(l4e, pfn, d) )
+    if ( unlikely(rc == -EINVAL) && get_l4_linear_pagetable(l4e, l4mfn, d) )
         rc = 0;
 
     return rc;
@@ -1460,13 +1460,13 @@ static int create_pae_xen_mappings(struct domain *d, l3_pgentry_t *pl3e)
 static int alloc_l2_table(struct page_info *page, unsigned long type)
 {
     struct domain *d = page_get_owner(page);
-    unsigned long  pfn = mfn_x(page_to_mfn(page));
+    unsigned long  l2mfn = mfn_x(page_to_mfn(page));
     l2_pgentry_t  *pl2e;
     unsigned int   i;
     int            rc = 0;
     unsigned int   partial_flags = page->partial_flags;
 
-    pl2e = map_domain_page(_mfn(pfn));
+    pl2e = map_domain_page(_mfn(l2mfn));
 
     /*
      * NB that alloc_l2_table will never set partial_pte on an l2; but
@@ -1492,7 +1492,7 @@ static int alloc_l2_table(struct page_info *page, unsigned long type)
             rc = -EINTR;
         }
         else
-            rc = get_page_from_l2e(l2e, pfn, d, partial_flags);
+            rc = get_page_from_l2e(l2e, l2mfn, d, partial_flags);
 
         /*
          * It shouldn't be possible for get_page_from_l2e to return
@@ -1559,14 +1559,14 @@ static int alloc_l2_table(struct page_info *page, unsigned long type)
 static int alloc_l3_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
-    unsigned long  pfn = mfn_x(page_to_mfn(page));
+    unsigned long  l3mfn = mfn_x(page_to_mfn(page));
     l3_pgentry_t  *pl3e;
     unsigned int   i;
     int            rc = 0;
     unsigned int   partial_flags = page->partial_flags;
     l3_pgentry_t   l3e = l3e_empty();
 
-    pl3e = map_domain_page(_mfn(pfn));
+    pl3e = map_domain_page(_mfn(l3mfn));
 
     /*
      * PAE guests allocate full pages, but aren't required to initialize
@@ -1603,7 +1603,7 @@ static int alloc_l3_table(struct page_info *page)
             rc = -EINTR;
         }
         else
-            rc = get_page_from_l3e(l3e, pfn, d,
+            rc = get_page_from_l3e(l3e, l3mfn, d,
                                    partial_flags | PTF_retain_ref_on_restart);
 
         if ( rc == -ERESTART )
@@ -1786,8 +1786,8 @@ void zap_ro_mpt(mfn_t mfn)
 static int alloc_l4_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
-    unsigned long  pfn = mfn_x(page_to_mfn(page));
-    l4_pgentry_t  *pl4e = map_domain_page(_mfn(pfn));
+    unsigned long  l4mfn = mfn_x(page_to_mfn(page));
+    l4_pgentry_t  *pl4e = map_domain_page(_mfn(l4mfn));
     unsigned int   i;
     int            rc = 0;
     unsigned int   partial_flags = page->partial_flags;
@@ -1809,7 +1809,7 @@ static int alloc_l4_table(struct page_info *page)
             rc = -EINTR;
         }
         else
-            rc = get_page_from_l4e(l4e, pfn, d,
+            rc = get_page_from_l4e(l4e, l4mfn, d,
                                    partial_flags | PTF_retain_ref_on_restart);
 
         if ( rc == -ERESTART )
@@ -1869,7 +1869,7 @@ static int alloc_l4_table(struct page_info *page)
 
     if ( !rc )
     {
-        init_xen_l4_slots(pl4e, _mfn(pfn),
+        init_xen_l4_slots(pl4e, _mfn(l4mfn),
                           d, INVALID_MFN, VM_ASSIST(d, m2p_strict));
         atomic_inc(&d->arch.pv.nr_l4_pages);
     }
@@ -1896,18 +1896,18 @@ static void free_l1_table(struct page_info *page)
 static int free_l2_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
-    unsigned long pfn = mfn_x(page_to_mfn(page));
+    unsigned long l2mfn = mfn_x(page_to_mfn(page));
     l2_pgentry_t *pl2e;
     int rc = 0;
     unsigned int partial_flags = page->partial_flags,
         i = page->nr_validated_ptes - !(partial_flags & PTF_partial_set);
 
-    pl2e = map_domain_page(_mfn(pfn));
+    pl2e = map_domain_page(_mfn(l2mfn));
 
     for ( ; ; )
     {
         if ( is_guest_l2_slot(d, page->u.inuse.type_info, i) )
-            rc = put_page_from_l2e(pl2e[i], pfn, partial_flags);
+            rc = put_page_from_l2e(pl2e[i], l2mfn, partial_flags);
         if ( rc < 0 )
             break;
 
@@ -1948,17 +1948,17 @@ static int free_l2_table(struct page_info *page)
 static int free_l3_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
-    unsigned long pfn = mfn_x(page_to_mfn(page));
+    unsigned long l3mfn = mfn_x(page_to_mfn(page));
     l3_pgentry_t *pl3e;
     int rc = 0;
     unsigned int partial_flags = page->partial_flags,
         i = page->nr_validated_ptes - !(partial_flags & PTF_partial_set);
 
-    pl3e = map_domain_page(_mfn(pfn));
+    pl3e = map_domain_page(_mfn(l3mfn));
 
     for ( ; ; )
     {
-        rc = put_page_from_l3e(pl3e[i], pfn, partial_flags);
+        rc = put_page_from_l3e(pl3e[i], l3mfn, partial_flags);
         if ( rc < 0 )
             break;
 
@@ -1995,15 +1995,15 @@ static int free_l3_table(struct page_info *page)
 static int free_l4_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
-    unsigned long pfn = mfn_x(page_to_mfn(page));
-    l4_pgentry_t *pl4e = map_domain_page(_mfn(pfn));
+    unsigned long l4mfn = mfn_x(page_to_mfn(page));
+    l4_pgentry_t *pl4e = map_domain_page(_mfn(l4mfn));
     int rc = 0;
     unsigned partial_flags = page->partial_flags,
         i = page->nr_validated_ptes - !(partial_flags & PTF_partial_set);
 
     do {
         if ( is_guest_l4_slot(d, i) )
-            rc = put_page_from_l4e(pl4e[i], pfn, partial_flags);
+            rc = put_page_from_l4e(pl4e[i], l4mfn, partial_flags);
         if ( rc < 0 )
             break;
         partial_flags = 0;
-- 
2.24.0


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

[Xen-devel] [PATCH 4/4] x86/mm: More discriptive names for page de/validation functions

[George Dunlap]

The functions alloc_page_type(), alloc_lN_table(), free_page_type()
and free_lN_table() are confusingly named: nothing is being allocated
or freed.  Rather, the page being passed in is being either validated
or devalidated for use as the specific type.

Rename these functions to "validate" and "devalidate" respectively to
be more clear.

No functional change.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/domain.c    |  2 +-
 xen/arch/x86/mm.c        | 62 ++++++++++++++++++++--------------------
 xen/include/asm-x86/mm.h |  4 +--
 3 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index f5c0c378ef..ee20de6493 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2032,7 +2032,7 @@ static int relinquish_memory(
             if ( likely(y == x) )
             {
                 /* No need for atomic update of type_info here: noone else updates it. */
-                switch ( ret = free_page_type(page, x, 1) )
+                switch ( ret = devalidate_page_type(page, x, 1) )
                 {
                 case 0:
                     break;
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 54b4100d55..dc1463123c 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1366,7 +1366,7 @@ static int put_page_from_l4e(l4_pgentry_t l4e, unsigned long pfn,
     return put_pt_page(l4e_get_page(l4e), mfn_to_page(_mfn(pfn)), flags);
 }
 
-static int alloc_l1_table(struct page_info *page)
+static int validate_l1_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
     l1_pgentry_t  *pl1e;
@@ -1408,7 +1408,7 @@ static int alloc_l1_table(struct page_info *page)
 
  fail:
     gdprintk(XENLOG_WARNING,
-             "Failure %d in alloc_l1_table: slot %#x\n", ret, i);
+             "Failure %d in validate_l1_table: slot %#x\n", ret, i);
  out:
     while ( i-- > 0 )
         put_page_from_l1e(pl1e[i], d);
@@ -1441,7 +1441,7 @@ static int create_pae_xen_mappings(struct domain *d, l3_pgentry_t *pl3e)
      *  1. Cannot appear in slots != 3 because get_page_type() checks the
      *     PGT_pae_xen_l2 flag, which is asserted iff the L2 appears in slot 3
      *  2. Cannot appear in another page table's L3:
-     *     a. alloc_l3_table() calls this function and this check will fail
+     *     a. validate_l3_table() calls this function and this check will fail
      *     b. mod_l3_entry() disallows updates to slot 3 in an existing table
      */
     page = l3e_get_page(l3e3);
@@ -1457,7 +1457,7 @@ static int create_pae_xen_mappings(struct domain *d, l3_pgentry_t *pl3e)
     return 1;
 }
 
-static int alloc_l2_table(struct page_info *page, unsigned long type)
+static int validate_l2_table(struct page_info *page, unsigned long type)
 {
     struct domain *d = page_get_owner(page);
     unsigned long  l2mfn = mfn_x(page_to_mfn(page));
@@ -1469,8 +1469,8 @@ static int alloc_l2_table(struct page_info *page, unsigned long type)
     pl2e = map_domain_page(_mfn(l2mfn));
 
     /*
-     * NB that alloc_l2_table will never set partial_pte on an l2; but
-     * free_l2_table might if a linear_pagetable entry is interrupted
+     * NB that validate_l2_table will never set partial_pte on an l2; but
+     * devalidate_l2_table might if a linear_pagetable entry is interrupted
      * partway through de-validation.  In that circumstance,
      * get_page_from_l2e() will always return -EINVAL; and we must
      * retain the type ref by doing the normal partial_flags tracking.
@@ -1497,7 +1497,7 @@ static int alloc_l2_table(struct page_info *page, unsigned long type)
         /*
          * It shouldn't be possible for get_page_from_l2e to return
          * -ERESTART, since we never call this with PTF_preemptible.
-         * (alloc_l1_table may return -EINTR on an L1TF-vulnerable
+         * (validate_l1_table may return -EINTR on an L1TF-vulnerable
          * entry.)
          *
          * NB that while on a "clean" promotion, we can never get
@@ -1518,12 +1518,12 @@ static int alloc_l2_table(struct page_info *page, unsigned long type)
         else if ( rc < 0 && rc != -EINTR )
         {
             gdprintk(XENLOG_WARNING,
-                     "Failure %d in alloc_l2_table: slot %#x\n", rc, i);
+                     "Failure %d in validate_l2_table: slot %#x\n", rc, i);
             ASSERT(current->arch.old_guest_table == NULL);
             if ( i )
             {
                 /*
-                 * alloc_l1_table() doesn't set old_guest_table; it does
+                 * validate_l1_table() doesn't set old_guest_table; it does
                  * its own tear-down immediately on failure.  If it
                  * did we'd need to check it and set partial_flags as we
                  * do in alloc_l[34]_table().
@@ -1556,7 +1556,7 @@ static int alloc_l2_table(struct page_info *page, unsigned long type)
     return rc;
 }
 
-static int alloc_l3_table(struct page_info *page)
+static int validate_l3_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
     unsigned long  l3mfn = mfn_x(page_to_mfn(page));
@@ -1629,7 +1629,7 @@ static int alloc_l3_table(struct page_info *page)
     if ( rc < 0 && rc != -ERESTART && rc != -EINTR )
     {
         gdprintk(XENLOG_WARNING,
-                 "Failure %d in alloc_l3_table: slot %#x\n", rc, i);
+                 "Failure %d in validate_l3_table: slot %#x\n", rc, i);
         if ( i )
         {
             page->nr_validated_ptes = i;
@@ -1680,7 +1680,7 @@ void init_xen_pae_l2_slots(l2_pgentry_t *l2t, const struct domain *d)
  * Fill an L4 with Xen entries.
  *
  * This function must write all ROOT_PAGETABLE_PV_XEN_SLOTS, to clobber any
- * values a guest may have left there from alloc_l4_table().
+ * values a guest may have left there from validate_l4_table().
  *
  * l4t and l4mfn are mandatory, but l4mfn doesn't need to be the mfn under
  * *l4t.  All other parameters are optional and will either fill or zero the
@@ -1783,7 +1783,7 @@ void zap_ro_mpt(mfn_t mfn)
 }
 
 #ifdef CONFIG_PV
-static int alloc_l4_table(struct page_info *page)
+static int validate_l4_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
     unsigned long  l4mfn = mfn_x(page_to_mfn(page));
@@ -1822,7 +1822,7 @@ static int alloc_l4_table(struct page_info *page)
         {
             if ( rc != -EINTR )
                 gdprintk(XENLOG_WARNING,
-                         "Failure %d in alloc_l4_table: slot %#x\n", rc, i);
+                         "Failure %d in validate_l4_table: slot %#x\n", rc, i);
             if ( i )
             {
                 page->nr_validated_ptes = i;
@@ -1878,7 +1878,7 @@ static int alloc_l4_table(struct page_info *page)
     return rc;
 }
 
-static void free_l1_table(struct page_info *page)
+static void devalidate_l1_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
     l1_pgentry_t *pl1e;
@@ -1893,7 +1893,7 @@ static void free_l1_table(struct page_info *page)
 }
 
 
-static int free_l2_table(struct page_info *page)
+static int devalidate_l2_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
     unsigned long l2mfn = mfn_x(page_to_mfn(page));
@@ -1945,7 +1945,7 @@ static int free_l2_table(struct page_info *page)
     return rc;
 }
 
-static int free_l3_table(struct page_info *page)
+static int devalidate_l3_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
     unsigned long l3mfn = mfn_x(page_to_mfn(page));
@@ -1992,7 +1992,7 @@ static int free_l3_table(struct page_info *page)
     return rc > 0 ? 0 : rc;
 }
 
-static int free_l4_table(struct page_info *page)
+static int devalidate_l4_table(struct page_info *page)
 {
     struct domain *d = page_get_owner(page);
     unsigned long l4mfn = mfn_x(page_to_mfn(page));
@@ -2592,7 +2592,7 @@ static void get_page_light(struct page_info *page)
     while ( unlikely(y != x) );
 }
 
-static int alloc_page_type(struct page_info *page, unsigned long type,
+static int validate_page_type(struct page_info *page, unsigned long type,
                            int preemptible)
 {
 #ifdef CONFIG_PV
@@ -2606,25 +2606,25 @@ static int alloc_page_type(struct page_info *page, unsigned long type,
     switch ( type & PGT_type_mask )
     {
     case PGT_l1_page_table:
-        rc = alloc_l1_table(page);
+        rc = validate_l1_table(page);
         break;
     case PGT_l2_page_table:
         ASSERT(preemptible);
-        rc = alloc_l2_table(page, type);
+        rc = validate_l2_table(page, type);
         break;
     case PGT_l3_page_table:
         ASSERT(preemptible);
-        rc = alloc_l3_table(page);
+        rc = validate_l3_table(page);
         break;
     case PGT_l4_page_table:
         ASSERT(preemptible);
-        rc = alloc_l4_table(page);
+        rc = validate_l4_table(page);
         break;
     case PGT_seg_desc_page:
         rc = alloc_segdesc_page(page);
         break;
     default:
-        printk("Bad type in alloc_page_type %lx t=%" PRtype_info " c=%lx\n",
+        printk("Bad type in validate_page_type %lx t=%" PRtype_info " c=%lx\n",
                type, page->u.inuse.type_info,
                page->count_info);
         rc = -EINVAL;
@@ -2672,7 +2672,7 @@ static int alloc_page_type(struct page_info *page, unsigned long type,
 }
 
 
-int free_page_type(struct page_info *page, unsigned long type,
+int devalidate_page_type(struct page_info *page, unsigned long type,
                    int preemptible)
 {
 #ifdef CONFIG_PV
@@ -2700,20 +2700,20 @@ int free_page_type(struct page_info *page, unsigned long type,
     switch ( type & PGT_type_mask )
     {
     case PGT_l1_page_table:
-        free_l1_table(page);
+        devalidate_l1_table(page);
         rc = 0;
         break;
     case PGT_l2_page_table:
         ASSERT(preemptible);
-        rc = free_l2_table(page);
+        rc = devalidate_l2_table(page);
         break;
     case PGT_l3_page_table:
         ASSERT(preemptible);
-        rc = free_l3_table(page);
+        rc = devalidate_l3_table(page);
         break;
     case PGT_l4_page_table:
         ASSERT(preemptible);
-        rc = free_l4_table(page);
+        rc = devalidate_l4_table(page);
         break;
     default:
         gdprintk(XENLOG_WARNING, "type %" PRtype_info " mfn %" PRI_mfn "\n",
@@ -2733,7 +2733,7 @@ int free_page_type(struct page_info *page, unsigned long type,
 static int _put_final_page_type(struct page_info *page, unsigned long type,
                                 bool preemptible, struct page_info *ptpg)
 {
-    int rc = free_page_type(page, type, preemptible);
+    int rc = devalidate_page_type(page, type, preemptible);
 
     if ( ptpg && PGT_type_equal(type, ptpg->u.inuse.type_info) &&
          (type & PGT_validated) && rc != -EINTR )
@@ -3016,7 +3016,7 @@ static int _get_page_type(struct page_info *page, unsigned long type,
             page->partial_flags = 0;
         }
         page->linear_pt_count = 0;
-        rc = alloc_page_type(page, type, preemptible);
+        rc = validate_page_type(page, type, preemptible);
     }
 
  out:
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 320c6cd196..a837f69548 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -355,8 +355,8 @@ static inline void *__page_to_virt(const struct page_info *pg)
                     (PAGE_SIZE / (sizeof(*pg) & -sizeof(*pg))));
 }
 
-int free_page_type(struct page_info *page, unsigned long type,
-                   int preemptible);
+int devalidate_page_type(struct page_info *page, unsigned long type,
+                         int preemptible);
 
 void init_xen_pae_l2_slots(l2_pgentry_t *l2t, const struct domain *d);
 void init_xen_l4_slots(l4_pgentry_t *l4t, mfn_t l4mfn,
-- 
2.24.0


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

Re: [Xen-devel] [PATCH 4/4] x86/mm: More discriptive names for page de/validation functions

[Andrew Cooper]

On 12/12/2019 17:32, George Dunlap wrote:
> @@ -3016,7 +3016,7 @@ static int _get_page_type(struct page_info *page, unsigned long type,
>              page->partial_flags = 0;
>          }
>          page->linear_pt_count = 0;
> -        rc = alloc_page_type(page, type, preemptible);
> +        rc = validate_page_type(page, type, preemptible);
>      }
>  
>   out:

FYI this has a conflict vs XSA-309, and needs rebasing onto staging. 
Luckily, this one isn't hard to fix up.

~Andrew

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

Re: [Xen-devel] [PATCH 1/4] x86/mm: Refactor put_page_from_l*e to reduce code duplication

[Andrew Cooper]

On 12/12/2019 17:32, George Dunlap wrote:
> put_page_from_l[234]e have identical functionality for devalidating an
> N-1 entry which is a pagetable.  But mystifyingly, they duplicate the
> code in slightly different arrangements that make it hard to tell that
> it's the same.

ITYM "this code has grown rather more organically than most" :)

> Create a new function, put_pt_page(), which handles the common
> functionality; and refactor all the functions to be symmetric,
> differing only in the level of pagetable expected (and in whether they
> handle superpages).
>
> No functional change intended.
>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

The only thing I can see is that the l2 case gains an assertion, but
this looks to be correct.

~Andrew

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

Re: [Xen-devel] [PATCH 2/4] x86/mm: Implement common put_data_pages for put_page_from_l[23]e

[Andrew Cooper]

On 12/12/2019 17:32, George Dunlap wrote:
> Both put_page_from_l2e and put_page_from_l3e handle having superpage
> entries by looping over each page and "put"-ing each one individually.
> As with putting page table entries, this code is functionally
> identical, but for some reason different.  Moreover, there is already
> a common function, put_data_page(), to handle automatically swapping
> between put_page() (for read-only pages) or put_page_and_type() (for
> read-write pages).
>
> Replace this with put_data_pages() (plural), which does the entire
> loop, as well as the put_page / put_page_and_type switch.
>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> NB that I've used the "simple for loop" version to make it easy to see
> what's going on, rather than the "do { } while()" version which uses &
> and compare to zero rather than comparing to the max.

So while I think the change is an improvement, and are fine as
presented, I'm -1 towards it overall.

I am going to once again express my firm opinion that the remaining use
of PV superpages do far more harm than good, and should be removed
completely.

We construct dom0 with some superpages for its p2m and/or initrd.

This turned out to be the issue behind pv-l1tf breaking for dom0 (c/s
96f6ee15ad7c), and why we had to ship XSA-273 in an insecure
configuration (and I'd like to point out that Xen is still in an
insecure configuration by default.)

There is a still-outstanding issue with grant map by linear address over
a superpage where things malfunction, and the only reason this doesn't
have an XSA is that it is believed to be restricted to dom0 only.

Finally, an L3_SHIFT loop is a long running operation which we can't put
a continuation into the middle of, and I bet there are fun things which
can be done there in the general case.

Seeing as PV guests decompress and free the initrd, and repositions the
p2m, none of these superpages tend to survive post boot, so I am
currently unconvinced that a perf improvement would be anything but
marginal.

I certainly don't think it is worth the complexity and corner cases it
causes is Xen.

~Andrew

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

Re: [Xen-devel] [PATCH 3/4] x86/mm: Use a more descriptive name for pagetable mfns

[Andrew Cooper]

On 12/12/2019 17:32, George Dunlap wrote:
> In many places, a PTE being modified is accompanied by the pagetable
> mfn which contains the PTE (primarily in order to be able to maintain
> linear mapping counts).  In many cases, this mfn is stored in the
> non-descript variable (or argement) "pfn".
>
> Replace these names with lNmfn, to indicate that 1) this is a
> pagetable mfn, and 2) that it is the same level as the PTE in
> question.  This should be enough to remind readers that it's the mfn
> containing the PTE.
>
> No functional change.
>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Any chance mfn_t can find its way in?

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

Re: [Xen-devel] [PATCH 4/4] x86/mm: More discriptive names for page de/validation functions

[Andrew Cooper]

On 12/12/2019 17:32, George Dunlap wrote:
> The functions alloc_page_type(), alloc_lN_table(), free_page_type()
> and free_lN_table() are confusingly named:

There is alloc_segdesc_page() which should be adjusted for consistency.

> nothing is being allocated or freed.

Well - strictly speaking the type reference is being obtained/dropped,
and this is a kind of alloc/free, although I agree that the names are
not great.

However, I'm not entirely sure that {de,}validate are great either,
because it isn't obviously tied to obtaining/dropping a type reference.

That said, I don't have a better suggestion right now.

~Andrew

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

Re: [Xen-devel] [PATCH 2/4] x86/mm: Implement common put_data_pages for put_page_from_l[23]e

[Jan Beulich]

On 12.12.2019 18:32, George Dunlap wrote:
> Both put_page_from_l2e and put_page_from_l3e handle having superpage
> entries by looping over each page and "put"-ing each one individually.
> As with putting page table entries, this code is functionally
> identical, but for some reason different.  Moreover, there is already
> a common function, put_data_page(), to handle automatically swapping
> between put_page() (for read-only pages) or put_page_and_type() (for
> read-write pages).
> 
> Replace this with put_data_pages() (plural), which does the entire
> loop, as well as the put_page / put_page_and_type switch.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> NB that I've used the "simple for loop" version to make it easy to see
> what's going on, rather than the "do { } while()" version which uses &
> and compare to zero rather than comparing to the max.
> 
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/arch/x86/mm.c | 52 ++++++++++++++++++-----------------------------
>  1 file changed, 20 insertions(+), 32 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index d8a0eb2aa5..c05039ab21 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1289,14 +1289,6 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner)
>  }
>  
>  #ifdef CONFIG_PV
> -static void put_data_page(struct page_info *page, bool writeable)
> -{
> -    if ( writeable )
> -        put_page_and_type(page);
> -    else
> -        put_page(page);
> -}
> -
>  static int put_pt_page(struct page_info *pg, struct page_info *ptpg,
>                         unsigned int flags)
>  {
> @@ -1319,6 +1311,20 @@ static int put_pt_page(struct page_info *pg, struct page_info *ptpg,
>      return rc;
>  }
>  
> +static int put_data_pages(struct page_info *page, bool writeable, int pt_shift)
> +{
> +    int i, count = 1 << (pt_shift - PAGE_SHIFT);

With both "int" here changed to "unsigned int"
Reviewed-by: Jan Beulich <jbeulich@suse.com>
But of course Andrew's objection needs addressing one way or another.

Jan

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

Re: [Xen-devel] [PATCH 4/4] x86/mm: More discriptive names for page de/validation functions

[Jan Beulich]

On 12.12.2019 21:07, Andrew Cooper wrote:
> On 12/12/2019 17:32, George Dunlap wrote:
>> The functions alloc_page_type(), alloc_lN_table(), free_page_type()
>> and free_lN_table() are confusingly named:
> 
> There is alloc_segdesc_page() which should be adjusted for consistency.
> 
>> nothing is being allocated or freed.
> 
> Well - strictly speaking the type reference is being obtained/dropped,
> and this is a kind of alloc/free, although I agree that the names are
> not great.
> 
> However, I'm not entirely sure that {de,}validate are great either,
> because it isn't obviously tied to obtaining/dropping a type reference.
> 
> That said, I don't have a better suggestion right now.

Following the wording of yours, how about {obtain,drop}_page_type()?

Jan

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

Re: [Xen-devel] [PATCH 4/4] x86/mm: More discriptive names for page de/validation functions

[George Dunlap]

> On Dec 13, 2019, at 10:48 AM, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 12.12.2019 21:07, Andrew Cooper wrote:
>> On 12/12/2019 17:32, George Dunlap wrote:
>>> The functions alloc_page_type(), alloc_lN_table(), free_page_type()
>>> and free_lN_table() are confusingly named:
>> 
>> There is alloc_segdesc_page() which should be adjusted for consistency.
>> 
>>> nothing is being allocated or freed.
>> 
>> Well - strictly speaking the type reference is being obtained/dropped,
>> and this is a kind of alloc/free, although I agree that the names are
>> not great.

On the contrary — the type reference was obtained / will be dropped in {get,put}_page_type(); but the page has not yet been validated to actually be used as that type / still holds references to other pages as though it were that type.  

>> 
>> However, I'm not entirely sure that {de,}validate are great either,
>> because it isn't obviously tied to obtaining/dropping a type reference.
>> 
>> That said, I don't have a better suggestion right now.
> 
> Following the wording of yours, how about {obtain,drop}_page_type()?

“Obtain” is literally a synonym for “get”; and there are many places in the code where we say thing like, “drop the type count” just before calling “put”.  

I agree “devalidate” looks a bit clunky, but all through the discussions on XSA-299, the word “de-validate” was used for the work that the “free" functions are doing — namely, dropping references to other pages such that the “validate” bit is clear.

I mean, we could do something like “bless” and “unbless”, but I hardly think that’s more clear.  “promote” and “demote”?

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

Re: [Xen-devel] [PATCH 4/4] x86/mm: More discriptive names for page de/validation functions

[Jan Beulich]

On 13.12.2019 13:00, George Dunlap wrote:
> 
> 
>> On Dec 13, 2019, at 10:48 AM, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 12.12.2019 21:07, Andrew Cooper wrote:
>>> On 12/12/2019 17:32, George Dunlap wrote:
>>>> The functions alloc_page_type(), alloc_lN_table(), free_page_type()
>>>> and free_lN_table() are confusingly named:
>>>
>>> There is alloc_segdesc_page() which should be adjusted for consistency.
>>>
>>>> nothing is being allocated or freed.
>>>
>>> Well - strictly speaking the type reference is being obtained/dropped,
>>> and this is a kind of alloc/free, although I agree that the names are
>>> not great.
> 
> On the contrary — the type reference was obtained / will be dropped in {get,put}_page_type(); but the page has not yet been validated to actually be used as that type / still holds references to other pages as though it were that type.  
> 
>>>
>>> However, I'm not entirely sure that {de,}validate are great either,
>>> because it isn't obviously tied to obtaining/dropping a type reference.
>>>
>>> That said, I don't have a better suggestion right now.
>>
>> Following the wording of yours, how about {obtain,drop}_page_type()?
> 
> “Obtain” is literally a synonym for “get”; and there are many places in the code where we say thing like, “drop the type count” just before calling “put”.  
> 
> I agree “devalidate” looks a bit clunky, but all through the discussions on XSA-299, the word “de-validate” was used for the work that the “free" functions are doing — namely, dropping references to other pages such that the “validate” bit is clear.

True.

> I mean, we could do something like “bless” and “unbless”, but I hardly think that’s more clear.  “promote” and “demote”?

Why not. Perhaps with the trailing _type also dropped?

Jan

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

Re: [Xen-devel] [PATCH 2/4] x86/mm: Implement common put_data_pages for put_page_from_l[23]e

[George Dunlap]

> On Dec 12, 2019, at 7:57 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 12/12/2019 17:32, George Dunlap wrote:
>> Both put_page_from_l2e and put_page_from_l3e handle having superpage
>> entries by looping over each page and "put"-ing each one individually.
>> As with putting page table entries, this code is functionally
>> identical, but for some reason different.  Moreover, there is already
>> a common function, put_data_page(), to handle automatically swapping
>> between put_page() (for read-only pages) or put_page_and_type() (for
>> read-write pages).
>> 
>> Replace this with put_data_pages() (plural), which does the entire
>> loop, as well as the put_page / put_page_and_type switch.
>> 
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>> ---
>> NB that I've used the "simple for loop" version to make it easy to see
>> what's going on, rather than the "do { } while()" version which uses &
>> and compare to zero rather than comparing to the max.
> 
> So while I think the change is an improvement, and are fine as
> presented, I'm -1 towards it overall.
> 
> I am going to once again express my firm opinion that the remaining use
> of PV superpages do far more harm than good, and should be removed
> completely.
> 
> We construct dom0 with some superpages for its p2m and/or initrd.
> 
> This turned out to be the issue behind pv-l1tf breaking for dom0 (c/s
> 96f6ee15ad7c), and why we had to ship XSA-273 in an insecure
> configuration (and I'd like to point out that Xen is still in an
> insecure configuration by default.)
> 
> There is a still-outstanding issue with grant map by linear address over
> a superpage where things malfunction, and the only reason this doesn't
> have an XSA is that it is believed to be restricted to dom0 only.
> 
> Finally, an L3_SHIFT loop is a long running operation which we can't put
> a continuation into the middle of, and I bet there are fun things which
> can be done there in the general case.
> 
> Seeing as PV guests decompress and free the initrd, and repositions the
> p2m, none of these superpages tend to survive post boot, so I am
> currently unconvinced that a perf improvement would be anything but
> marginal.
> 
> I certainly don't think it is worth the complexity and corner cases it
> causes is Xen.

That all sounds reasonable, but I don’t really know anything about PV superpages in Xen at the moment (in fact I sort of wondered what this code was about).

I’d recommend taking this patch as-is, and “someone” can put it on their list to get rid of the PV superpages later.  The alternate is I drop this patch from the series and “someone" puts the PV superpage removal on their list to do later.

(My mind is also involuntarily churning through options to regularize superpage promotion and demotion.)

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

Re: [Xen-devel] [PATCH 2/4] x86/mm: Implement common put_data_pages for put_page_from_l[23]e

[Andrew Cooper]

On 13/12/2019 13:58, George Dunlap wrote:
>
>> On Dec 12, 2019, at 7:57 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>
>> On 12/12/2019 17:32, George Dunlap wrote:
>>> Both put_page_from_l2e and put_page_from_l3e handle having superpage
>>> entries by looping over each page and "put"-ing each one individually.
>>> As with putting page table entries, this code is functionally
>>> identical, but for some reason different.  Moreover, there is already
>>> a common function, put_data_page(), to handle automatically swapping
>>> between put_page() (for read-only pages) or put_page_and_type() (for
>>> read-write pages).
>>>
>>> Replace this with put_data_pages() (plural), which does the entire
>>> loop, as well as the put_page / put_page_and_type switch.
>>>
>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>>> ---
>>> NB that I've used the "simple for loop" version to make it easy to see
>>> what's going on, rather than the "do { } while()" version which uses &
>>> and compare to zero rather than comparing to the max.
>> So while I think the change is an improvement, and are fine as
>> presented, I'm -1 towards it overall.
>>
>> I am going to once again express my firm opinion that the remaining use
>> of PV superpages do far more harm than good, and should be removed
>> completely.
>>
>> We construct dom0 with some superpages for its p2m and/or initrd.
>>
>> This turned out to be the issue behind pv-l1tf breaking for dom0 (c/s
>> 96f6ee15ad7c), and why we had to ship XSA-273 in an insecure
>> configuration (and I'd like to point out that Xen is still in an
>> insecure configuration by default.)
>>
>> There is a still-outstanding issue with grant map by linear address over
>> a superpage where things malfunction, and the only reason this doesn't
>> have an XSA is that it is believed to be restricted to dom0 only.
>>
>> Finally, an L3_SHIFT loop is a long running operation which we can't put
>> a continuation into the middle of, and I bet there are fun things which
>> can be done there in the general case.
>>
>> Seeing as PV guests decompress and free the initrd, and repositions the
>> p2m, none of these superpages tend to survive post boot, so I am
>> currently unconvinced that a perf improvement would be anything but
>> marginal.
>>
>> I certainly don't think it is worth the complexity and corner cases it
>> causes is Xen.
> That all sounds reasonable, but I don’t really know anything about PV superpages in Xen at the moment (in fact I sort of wondered what this code was about).
>
> I’d recommend taking this patch as-is, and “someone” can put it on their list to get rid of the PV superpages later.  The alternate is I drop this patch from the series and “someone" puts the PV superpage removal on their list to do later.

As I said, I think the patch is fine cleanup (modulo the 3 int's which
Jan commented on).

If you don't want to tackle the bigger problem now, then guess it is
another thing which can sit on my todo list (and possibly never get done...)

~Andrew

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