[Xen-devel] [PATCH v4 0/9] Add alternative API for Xen PTEs

[Xen-devel] [PATCH v4 0/9] Add alternative API for Xen PTEs

[Hongyan Xia]

NOTE: My email address has changed due to some HR management. I have
lost all my previous emails and I could only salvage some of the
comments to v3 from the mailing list archive. I will reply to the
comments from v3 in this v4 series.

This batch adds an alternative alloc-map-unmap-free Xen PTE API to the
normal alloc-free on the xenheap, in preparation of switching to domheap
for Xen page tables. Since map and unmap are basically no-ops now, and
other changes are cosmetic to ease future patches, this batch does not
introduce any functional changes.

tree:
https://xenbits.xen.org/git-http/people/hx242/xen.git xen_pte_map-v4

v3: 
https://lists.xenproject.org/archives/html/xen-devel/2019-10/msg00304.html

---
Changed since v3:
- change my email address in all patches
- address many style issues in v3
- rebase

Changed since v2:
- split into a smaller series
- drop the clear_page optimisation as Wei suggests
- rebase

Changed since v1:
- squash some commits
- merge bug fixes into this first batch
- rebase against latest master

Wei Liu (9):
  x86: move some xen mm function declarations
  x86: introduce a new set of APIs to manage Xen page tables
  x86/mm: introduce l{1,2}t local variables to map_pages_to_xen
  x86/mm: introduce l{1,2}t local variables to modify_xen_mappings
  x86/mm: map_pages_to_xen would better have one exit path
  x86/mm: add an end_of_loop label in map_pages_to_xen
  x86/mm: make sure there is one exit path for modify_xen_mappings
  x86/mm: add an end_of_loop label in modify_xen_mappings
  x86/mm: change pl*e to l*t in virt_to_xen_l*e

 xen/arch/x86/mm.c          | 280 ++++++++++++++++++++++---------------
 xen/include/asm-x86/mm.h   |  16 +++
 xen/include/asm-x86/page.h |   5 -
 3 files changed, 186 insertions(+), 115 deletions(-)

-- 
2.17.1


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

[Xen-devel] [PATCH v4 1/9] x86: move some xen mm function declarations

[Hongyan Xia]

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

They were put into page.h but mm.h is more appropriate.

The real reason is that I will be adding some new functions which
takes mfn_t. It turns out it is a bit difficult to do in page.h.

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>

---
Changed since v3:
- move Xen PTE API declarations next to do_page_walk().
---
 xen/include/asm-x86/mm.h   | 5 +++++
 xen/include/asm-x86/page.h | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 320c6cd196..9d2b833579 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -579,6 +579,11 @@ void update_cr3(struct vcpu *v);
 int vcpu_destroy_pagetables(struct vcpu *);
 void *do_page_walk(struct vcpu *v, unsigned long addr);
 
+/* Allocator functions for Xen pagetables. */
+void *alloc_xen_pagetable(void);
+void free_xen_pagetable(void *v);
+l1_pgentry_t *virt_to_xen_l1e(unsigned long v);
+
 int __sync_local_execstate(void);
 
 /* Arch-specific portion of memory_op hypercall. */
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index c1e92937c0..05a8b1efa6 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -345,11 +345,6 @@ void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t);
 
 #ifndef __ASSEMBLY__
 
-/* Allocator functions for Xen pagetables. */
-void *alloc_xen_pagetable(void);
-void free_xen_pagetable(void *v);
-l1_pgentry_t *virt_to_xen_l1e(unsigned long v);
-
 /* Convert between PAT/PCD/PWT embedded in PTE flags and 3-bit cacheattr. */
 static inline unsigned int pte_flags_to_cacheattr(unsigned int flags)
 {
-- 
2.17.1


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

[Xen-devel] [PATCH v4 2/9] x86: introduce a new set of APIs to manage Xen page tables

[Hongyan Xia]

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

We are going to switch to using domheap page for page tables.
A new set of APIs is introduced to allocate, map, unmap and free pages
for page tables.

The allocation and deallocation work on mfn_t but not page_info,
because they are required to work even before frame table is set up.

Implement the old functions with the new ones. We will rewrite, site
by site, other mm functions that manipulate page tables to use the new
APIs.

Note these new APIs still use xenheap page underneath and no actual
map and unmap is done so that we don't break xen half way. They will
be switched to use domheap and dynamic mappings when usage of old APIs
is eliminated.

No functional change intended in this patch.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Hongyan Xia <hongyxia@amazon.com>

---
Changed since v3:
- const qualify unmap_xen_pagetable_new().
- remove redundant parentheses.
---
 xen/arch/x86/mm.c        | 39 ++++++++++++++++++++++++++++++++++-----
 xen/include/asm-x86/mm.h | 11 +++++++++++
 2 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 7d4dd80a85..ca362ad638 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -119,6 +119,7 @@
 #include <xen/efi.h>
 #include <xen/grant_table.h>
 #include <xen/hypercall.h>
+#include <xen/mm.h>
 #include <asm/paging.h>
 #include <asm/shadow.h>
 #include <asm/page.h>
@@ -5020,22 +5021,50 @@ int mmcfg_intercept_write(
 }
 
 void *alloc_xen_pagetable(void)
+{
+    mfn_t mfn;
+
+    mfn = alloc_xen_pagetable_new();
+    ASSERT(!mfn_eq(mfn, INVALID_MFN));
+
+    return map_xen_pagetable_new(mfn);
+}
+
+void free_xen_pagetable(void *v)
+{
+    if ( system_state != SYS_STATE_early_boot )
+        free_xen_pagetable_new(virt_to_mfn(v));
+}
+
+mfn_t alloc_xen_pagetable_new(void)
 {
     if ( system_state != SYS_STATE_early_boot )
     {
         void *ptr = alloc_xenheap_page();
 
         BUG_ON(!hardware_domain && !ptr);
-        return ptr;
+        return virt_to_mfn(ptr);
     }
 
-    return mfn_to_virt(mfn_x(alloc_boot_pages(1, 1)));
+    return alloc_boot_pages(1, 1);
 }
 
-void free_xen_pagetable(void *v)
+void *map_xen_pagetable_new(mfn_t mfn)
 {
-    if ( system_state != SYS_STATE_early_boot )
-        free_xenheap_page(v);
+    return mfn_to_virt(mfn_x(mfn));
+}
+
+/* v can point to an entry within a table or be NULL */
+void unmap_xen_pagetable_new(const void *v)
+{
+    /* XXX still using xenheap page, no need to do anything.  */
+}
+
+/* mfn can be INVALID_MFN */
+void free_xen_pagetable_new(mfn_t mfn)
+{
+    if ( system_state != SYS_STATE_early_boot && !mfn_eq(mfn, INVALID_MFN) )
+        free_xenheap_page(mfn_to_virt(mfn_x(mfn)));
 }
 
 static DEFINE_SPINLOCK(map_pgdir_lock);
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 9d2b833579..76593fe9e7 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -582,6 +582,17 @@ void *do_page_walk(struct vcpu *v, unsigned long addr);
 /* Allocator functions for Xen pagetables. */
 void *alloc_xen_pagetable(void);
 void free_xen_pagetable(void *v);
+mfn_t alloc_xen_pagetable_new(void);
+void *map_xen_pagetable_new(mfn_t mfn);
+void unmap_xen_pagetable_new(const void *v);
+void free_xen_pagetable_new(mfn_t mfn);
+
+#define UNMAP_XEN_PAGETABLE_NEW(ptr)    \
+    do {                                \
+        unmap_xen_pagetable_new(ptr);   \
+        (ptr) = NULL;                   \
+    } while (0)
+
 l1_pgentry_t *virt_to_xen_l1e(unsigned long v);
 
 int __sync_local_execstate(void);
-- 
2.17.1


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

Re: [Xen-devel] [PATCH v4 2/9] x86: introduce a new set of APIs to manage Xen page tables

[Xia, Hongyan]

> There's no need for the map/unmap functions to have a _new
> suffix, is there?

I thought this was weird at first also, but what I find really useful
is that we can just change all call sites to the new API in small steps
without breaking. Otherwise we have to merge a huge batch of
changes (around 40 patches) at once.

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

Re: [Xen-devel] [PATCH v4 2/9] x86: introduce a new set of APIs to manage Xen page tables

[Jan Beulich]

On 04.12.2019 18:54, Xia, Hongyan wrote:
>> There's no need for the map/unmap functions to have a _new
>> suffix, is there?
> 
> I thought this was weird at first also, but what I find really useful
> is that we can just change all call sites to the new API in small steps
> without breaking. Otherwise we have to merge a huge batch of
> changes (around 40 patches) at once.

But my comment was on functions _not_ having an "old"
equivalent. Having the suffix there initially makes for more
code churn than necessary when finally dropping the suffixes.

Jan

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

Re: [Xen-devel] [PATCH v4 2/9] x86: introduce a new set of APIs to manage Xen page tables

[Julien Grall]

Hi,

On 04/12/2019 17:10, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@citrix.com>
> 
> We are going to switch to using domheap page for page tables.
> A new set of APIs is introduced to allocate, map, unmap and free pages
> for page tables.
> 
> The allocation and deallocation work on mfn_t but not page_info,
> because they are required to work even before frame table is set up.
> 
> Implement the old functions with the new ones. We will rewrite, site
> by site, other mm functions that manipulate page tables to use the new
> APIs.
> 
> Note these new APIs still use xenheap page underneath and no actual
> map and unmap is done so that we don't break xen half way. They will
> be switched to use domheap and dynamic mappings when usage of old APIs
> is eliminated.
> 
> No functional change intended in this patch.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
> 
> ---
> Changed since v3:
> - const qualify unmap_xen_pagetable_new().
> - remove redundant parentheses.
> ---
>   xen/arch/x86/mm.c        | 39 ++++++++++++++++++++++++++++++++++-----
>   xen/include/asm-x86/mm.h | 11 +++++++++++
>   2 files changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 7d4dd80a85..ca362ad638 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -119,6 +119,7 @@
>   #include <xen/efi.h>
>   #include <xen/grant_table.h>
>   #include <xen/hypercall.h>
> +#include <xen/mm.h>
>   #include <asm/paging.h>
>   #include <asm/shadow.h>
>   #include <asm/page.h>
> @@ -5020,22 +5021,50 @@ int mmcfg_intercept_write(
>   }
>   
>   void *alloc_xen_pagetable(void)
> +{
> +    mfn_t mfn;
> +
> +    mfn = alloc_xen_pagetable_new();
> +    ASSERT(!mfn_eq(mfn, INVALID_MFN));

The current version of alloc_xen_pagetable() may return NULL. So I can't 
see why this would not happen with the new implementation (see more below).

Furthermore, AFAIK, mfn_to_virt() is not able to deal with INVALID_MFN. 
While the ASSERT will catch such error in debug build, non-debug build 
will end up to undefined behavior (allocation failure is a real 
possibility).

Regardless the discussion on whether the ASSERT() is warrant, I think 
you want to have:

if ( mfn_eq(mfn, INVALID_MFN) )
    return NULL;

I am half tempted to suggest to put that in map_xen_pagetable_new() for 
hardening.

> +
> +    return map_xen_pagetable_new(mfn);
> +}
> +
> +void free_xen_pagetable(void *v)
> +{
> +    if ( system_state != SYS_STATE_early_boot )
> +        free_xen_pagetable_new(virt_to_mfn(v));
> +}
> +
> +mfn_t alloc_xen_pagetable_new(void)
>   {
>       if ( system_state != SYS_STATE_early_boot )
>       {
>           void *ptr = alloc_xenheap_page();
>   
>           BUG_ON(!hardware_domain && !ptr);

This BUG_ON() only catch memory exhaustion before the Hardware Domain 
(aka Dom0) is created. Afterwards, the pointer may be NULL. As ...

> -        return ptr;
> +        return virt_to_mfn(ptr);

virt_to_mfn() requires a valid MFN, you will end up to undefined behavior.

>       }
>   
> -    return mfn_to_virt(mfn_x(alloc_boot_pages(1, 1)));
> +    return alloc_boot_pages(1, 1);
>   }
>   
> -void free_xen_pagetable(void *v)
> +void *map_xen_pagetable_new(mfn_t mfn)
>   {
> -    if ( system_state != SYS_STATE_early_boot )
> -        free_xenheap_page(v);
> +    return mfn_to_virt(mfn_x(mfn));
> +}
> +
> +/* v can point to an entry within a table or be NULL */
> +void unmap_xen_pagetable_new(const void *v)
> +{
> +    /* XXX still using xenheap page, no need to do anything.  */
> +}
> +
> +/* mfn can be INVALID_MFN */
> +void free_xen_pagetable_new(mfn_t mfn)
> +{
> +    if ( system_state != SYS_STATE_early_boot && !mfn_eq(mfn, INVALID_MFN) )
> +        free_xenheap_page(mfn_to_virt(mfn_x(mfn)));
>   }
>   
>   static DEFINE_SPINLOCK(map_pgdir_lock);
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index 9d2b833579..76593fe9e7 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -582,6 +582,17 @@ void *do_page_walk(struct vcpu *v, unsigned long addr);
>   /* Allocator functions for Xen pagetables. */
>   void *alloc_xen_pagetable(void);
>   void free_xen_pagetable(void *v);
> +mfn_t alloc_xen_pagetable_new(void);
> +void *map_xen_pagetable_new(mfn_t mfn);
> +void unmap_xen_pagetable_new(const void *v);
> +void free_xen_pagetable_new(mfn_t mfn);
> +
> +#define UNMAP_XEN_PAGETABLE_NEW(ptr)    \
> +    do {                                \
> +        unmap_xen_pagetable_new(ptr);   \
> +        (ptr) = NULL;                   \
> +    } while (0)
> +
>   l1_pgentry_t *virt_to_xen_l1e(unsigned long v);
>   
>   int __sync_local_execstate(void);
> 

-- 
Julien Grall

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

[Xen-devel] [PATCH v4 3/9] x86/mm: introduce l{1, 2}t local variables to map_pages_to_xen

[Hongyan Xia]

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

The pl2e and pl1e variables are heavily (ab)used in that function. It
is fine at the moment because all page tables are always mapped so
there is no need to track the life time of each variable.

We will soon have the requirement to map and unmap page tables. We
need to track the life time of each variable to avoid leakage.

Introduce some l{1,2}t variables with limited scope so that we can
track life time of pointers to xen page tables more easily.

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/mm.c | 75 ++++++++++++++++++++++++++---------------------
 1 file changed, 42 insertions(+), 33 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index ca362ad638..790578d2b3 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5234,10 +5234,12 @@ int map_pages_to_xen(
                 }
                 else
                 {
-                    pl2e = l3e_to_l2e(ol3e);
+                    l2_pgentry_t *l2t;
+
+                    l2t = l3e_to_l2e(ol3e);
                     for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
                     {
-                        ol2e = pl2e[i];
+                        ol2e = l2t[i];
                         if ( !(l2e_get_flags(ol2e) & _PAGE_PRESENT) )
                             continue;
                         if ( l2e_get_flags(ol2e) & _PAGE_PSE )
@@ -5245,21 +5247,22 @@ int map_pages_to_xen(
                         else
                         {
                             unsigned int j;
+                            l1_pgentry_t *l1t;
 
-                            pl1e = l2e_to_l1e(ol2e);
+                            l1t = l2e_to_l1e(ol2e);
                             for ( j = 0; j < L1_PAGETABLE_ENTRIES; j++ )
-                                flush_flags(l1e_get_flags(pl1e[j]));
+                                flush_flags(l1e_get_flags(l1t[j]));
                         }
                     }
                     flush_area(virt, flush_flags);
                     for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
                     {
-                        ol2e = pl2e[i];
+                        ol2e = l2t[i];
                         if ( (l2e_get_flags(ol2e) & _PAGE_PRESENT) &&
                              !(l2e_get_flags(ol2e) & _PAGE_PSE) )
                             free_xen_pagetable(l2e_to_l1e(ol2e));
                     }
-                    free_xen_pagetable(pl2e);
+                    free_xen_pagetable(l2t);
                 }
             }
 
@@ -5275,6 +5278,7 @@ int map_pages_to_xen(
         {
             unsigned int flush_flags =
                 FLUSH_TLB | FLUSH_ORDER(2 * PAGETABLE_ORDER);
+            l2_pgentry_t *l2t;
 
             /* Skip this PTE if there is no change. */
             if ( ((l3e_get_pfn(ol3e) & ~(L2_PAGETABLE_ENTRIES *
@@ -5296,12 +5300,12 @@ int map_pages_to_xen(
                 continue;
             }
 
-            pl2e = alloc_xen_pagetable();
-            if ( pl2e == NULL )
+            l2t = alloc_xen_pagetable();
+            if ( l2t == NULL )
                 return -ENOMEM;
 
             for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
-                l2e_write(pl2e + i,
+                l2e_write(l2t + i,
                           l2e_from_pfn(l3e_get_pfn(ol3e) +
                                        (i << PAGETABLE_ORDER),
                                        l3e_get_flags(ol3e)));
@@ -5314,15 +5318,15 @@ int map_pages_to_xen(
             if ( (l3e_get_flags(*pl3e) & _PAGE_PRESENT) &&
                  (l3e_get_flags(*pl3e) & _PAGE_PSE) )
             {
-                l3e_write_atomic(pl3e, l3e_from_mfn(virt_to_mfn(pl2e),
+                l3e_write_atomic(pl3e, l3e_from_mfn(virt_to_mfn(l2t),
                                                     __PAGE_HYPERVISOR));
-                pl2e = NULL;
+                l2t = NULL;
             }
             if ( locking )
                 spin_unlock(&map_pgdir_lock);
             flush_area(virt, flush_flags);
-            if ( pl2e )
-                free_xen_pagetable(pl2e);
+            if ( l2t )
+                free_xen_pagetable(l2t);
         }
 
         pl2e = virt_to_xen_l2e(virt);
@@ -5350,11 +5354,13 @@ int map_pages_to_xen(
                 }
                 else
                 {
-                    pl1e = l2e_to_l1e(ol2e);
+                    l1_pgentry_t *l1t;
+
+                    l1t = l2e_to_l1e(ol2e);
                     for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
-                        flush_flags(l1e_get_flags(pl1e[i]));
+                        flush_flags(l1e_get_flags(l1t[i]));
                     flush_area(virt, flush_flags);
-                    free_xen_pagetable(pl1e);
+                    free_xen_pagetable(l1t);
                 }
             }
 
@@ -5376,6 +5382,7 @@ int map_pages_to_xen(
             {
                 unsigned int flush_flags =
                     FLUSH_TLB | FLUSH_ORDER(PAGETABLE_ORDER);
+                l1_pgentry_t *l1t;
 
                 /* Skip this PTE if there is no change. */
                 if ( (((l2e_get_pfn(*pl2e) & ~(L1_PAGETABLE_ENTRIES - 1)) +
@@ -5395,12 +5402,12 @@ int map_pages_to_xen(
                     goto check_l3;
                 }
 
-                pl1e = alloc_xen_pagetable();
-                if ( pl1e == NULL )
+                l1t = alloc_xen_pagetable();
+                if ( l1t == NULL )
                     return -ENOMEM;
 
                 for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
-                    l1e_write(&pl1e[i],
+                    l1e_write(&l1t[i],
                               l1e_from_pfn(l2e_get_pfn(*pl2e) + i,
                                            lNf_to_l1f(l2e_get_flags(*pl2e))));
 
@@ -5412,15 +5419,15 @@ int map_pages_to_xen(
                 if ( (l2e_get_flags(*pl2e) & _PAGE_PRESENT) &&
                      (l2e_get_flags(*pl2e) & _PAGE_PSE) )
                 {
-                    l2e_write_atomic(pl2e, l2e_from_mfn(virt_to_mfn(pl1e),
+                    l2e_write_atomic(pl2e, l2e_from_mfn(virt_to_mfn(l1t),
                                                         __PAGE_HYPERVISOR));
-                    pl1e = NULL;
+                    l1t = NULL;
                 }
                 if ( locking )
                     spin_unlock(&map_pgdir_lock);
                 flush_area(virt, flush_flags);
-                if ( pl1e )
-                    free_xen_pagetable(pl1e);
+                if ( l1t )
+                    free_xen_pagetable(l1t);
             }
 
             pl1e  = l2e_to_l1e(*pl2e) + l1_table_offset(virt);
@@ -5445,6 +5452,7 @@ int map_pages_to_xen(
                     ((1u << PAGETABLE_ORDER) - 1)) == 0)) )
             {
                 unsigned long base_mfn;
+                l1_pgentry_t *l1t;
 
                 if ( locking )
                     spin_lock(&map_pgdir_lock);
@@ -5468,11 +5476,11 @@ int map_pages_to_xen(
                     goto check_l3;
                 }
 
-                pl1e = l2e_to_l1e(ol2e);
-                base_mfn = l1e_get_pfn(*pl1e) & ~(L1_PAGETABLE_ENTRIES - 1);
-                for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++, pl1e++ )
-                    if ( (l1e_get_pfn(*pl1e) != (base_mfn + i)) ||
-                         (l1e_get_flags(*pl1e) != flags) )
+                l1t = l2e_to_l1e(ol2e);
+                base_mfn = l1e_get_pfn(l1t[0]) & ~(L1_PAGETABLE_ENTRIES - 1);
+                for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
+                    if ( (l1e_get_pfn(l1t[i]) != (base_mfn + i)) ||
+                         (l1e_get_flags(l1t[i]) != flags) )
                         break;
                 if ( i == L1_PAGETABLE_ENTRIES )
                 {
@@ -5498,6 +5506,7 @@ int map_pages_to_xen(
                 ((1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) - 1))) )
         {
             unsigned long base_mfn;
+            l2_pgentry_t *l2t;
 
             if ( locking )
                 spin_lock(&map_pgdir_lock);
@@ -5515,13 +5524,13 @@ int map_pages_to_xen(
                 continue;
             }
 
-            pl2e = l3e_to_l2e(ol3e);
-            base_mfn = l2e_get_pfn(*pl2e) & ~(L2_PAGETABLE_ENTRIES *
+            l2t = l3e_to_l2e(ol3e);
+            base_mfn = l2e_get_pfn(l2t[0]) & ~(L2_PAGETABLE_ENTRIES *
                                               L1_PAGETABLE_ENTRIES - 1);
-            for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
-                if ( (l2e_get_pfn(*pl2e) !=
+            for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
+                if ( (l2e_get_pfn(l2t[i]) !=
                       (base_mfn + (i << PAGETABLE_ORDER))) ||
-                     (l2e_get_flags(*pl2e) != l1f_to_lNf(flags)) )
+                     (l2e_get_flags(l2t[i]) != l1f_to_lNf(flags)) )
                     break;
             if ( i == L2_PAGETABLE_ENTRIES )
             {
-- 
2.17.1


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

Re: [Xen-devel] [PATCH v4 3/9] x86/mm: introduce l{1, 2}t local variables to map_pages_to_xen

[Xia, Hongyan]

>> @@ -5272,6 +5279,7 @@ int map_pages_to_xen(
>>                      ((1u << PAGETABLE_ORDER) - 1)) == 0)) )
>>              {
>>                  unsigned long base_mfn;
>> +                l1_pgentry_t *l1t;
>
> const, as this looks to be used for lookup only?

I cannot do this for now since variables like l1t are still using the
old API which is non-const. I can change it once they are switched to
the new const API in later patches.

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

Re: [Xen-devel] [PATCH v4 3/9] x86/mm: introduce l{1, 2}t local variables to map_pages_to_xen

[Jan Beulich]

On 04.12.2019 19:01, Xia, Hongyan wrote:
>>> @@ -5272,6 +5279,7 @@ int map_pages_to_xen(
>>>                      ((1u << PAGETABLE_ORDER) - 1)) == 0)) )
>>>              {
>>>                  unsigned long base_mfn;
>>> +                l1_pgentry_t *l1t;
>>
>> const, as this looks to be used for lookup only?
> 
> I cannot do this for now since variables like l1t are still using the
> old API which is non-const. I can change it once they are switched to
> the new const API in later patches.

Maybe I've indeed picked an example where this won't work yet,
but there look to be cases where the old interface wouldn't
get in the way. I'd appreciate if at least those cases could
have const added right away.

Jan

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

[Xen-devel] [PATCH v4 4/9] x86/mm: introduce l{1, 2}t local variables to modify_xen_mappings

[Hongyan Xia]

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

The pl2e and pl1e variables are heavily (ab)used in that function.  It
is fine at the moment because all page tables are always mapped so
there is no need to track the life time of each variable.

We will soon have the requirement to map and unmap page tables. We
need to track the life time of each variable to avoid leakage.

Introduce some l{1,2}t variables with limited scope so that we can
track life time of pointers to xen page tables more easily.

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/mm.c | 68 ++++++++++++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 790578d2b3..303bc35549 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5601,6 +5601,8 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
 
         if ( l3e_get_flags(*pl3e) & _PAGE_PSE )
         {
+            l2_pgentry_t *l2t;
+
             if ( l2_table_offset(v) == 0 &&
                  l1_table_offset(v) == 0 &&
                  ((e - v) >= (1UL << L3_PAGETABLE_SHIFT)) )
@@ -5616,11 +5618,11 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
             }
 
             /* PAGE1GB: shatter the superpage and fall through. */
-            pl2e = alloc_xen_pagetable();
-            if ( !pl2e )
+            l2t = alloc_xen_pagetable();
+            if ( !l2t )
                 return -ENOMEM;
             for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
-                l2e_write(pl2e + i,
+                l2e_write(l2t + i,
                           l2e_from_pfn(l3e_get_pfn(*pl3e) +
                                        (i << PAGETABLE_ORDER),
                                        l3e_get_flags(*pl3e)));
@@ -5629,14 +5631,14 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
             if ( (l3e_get_flags(*pl3e) & _PAGE_PRESENT) &&
                  (l3e_get_flags(*pl3e) & _PAGE_PSE) )
             {
-                l3e_write_atomic(pl3e, l3e_from_mfn(virt_to_mfn(pl2e),
+                l3e_write_atomic(pl3e, l3e_from_mfn(virt_to_mfn(l2t),
                                                     __PAGE_HYPERVISOR));
-                pl2e = NULL;
+                l2t = NULL;
             }
             if ( locking )
                 spin_unlock(&map_pgdir_lock);
-            if ( pl2e )
-                free_xen_pagetable(pl2e);
+            if ( l2t )
+                free_xen_pagetable(l2t);
         }
 
         /*
@@ -5670,12 +5672,14 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
             }
             else
             {
+                l1_pgentry_t *l1t;
+
                 /* PSE: shatter the superpage and try again. */
-                pl1e = alloc_xen_pagetable();
-                if ( !pl1e )
+                l1t = alloc_xen_pagetable();
+                if ( !l1t )
                     return -ENOMEM;
                 for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
-                    l1e_write(&pl1e[i],
+                    l1e_write(&l1t[i],
                               l1e_from_pfn(l2e_get_pfn(*pl2e) + i,
                                            l2e_get_flags(*pl2e) & ~_PAGE_PSE));
                 if ( locking )
@@ -5683,19 +5687,19 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
                 if ( (l2e_get_flags(*pl2e) & _PAGE_PRESENT) &&
                      (l2e_get_flags(*pl2e) & _PAGE_PSE) )
                 {
-                    l2e_write_atomic(pl2e, l2e_from_mfn(virt_to_mfn(pl1e),
+                    l2e_write_atomic(pl2e, l2e_from_mfn(virt_to_mfn(l1t),
                                                         __PAGE_HYPERVISOR));
-                    pl1e = NULL;
+                    l1t = NULL;
                 }
                 if ( locking )
                     spin_unlock(&map_pgdir_lock);
-                if ( pl1e )
-                    free_xen_pagetable(pl1e);
+                if ( l1t )
+                    free_xen_pagetable(l1t);
             }
         }
         else
         {
-            l1_pgentry_t nl1e;
+            l1_pgentry_t nl1e, *l1t;
 
             /*
              * Ordinary 4kB mapping: The L2 entry has been verified to be
@@ -5742,9 +5746,9 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
                 continue;
             }
 
-            pl1e = l2e_to_l1e(*pl2e);
+            l1t = l2e_to_l1e(*pl2e);
             for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
-                if ( l1e_get_intpte(pl1e[i]) != 0 )
+                if ( l1e_get_intpte(l1t[i]) != 0 )
                     break;
             if ( i == L1_PAGETABLE_ENTRIES )
             {
@@ -5753,7 +5757,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
                 if ( locking )
                     spin_unlock(&map_pgdir_lock);
                 flush_area(NULL, FLUSH_TLB_GLOBAL); /* flush before free */
-                free_xen_pagetable(pl1e);
+                free_xen_pagetable(l1t);
             }
             else if ( locking )
                 spin_unlock(&map_pgdir_lock);
@@ -5782,21 +5786,25 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
             continue;
         }
 
-        pl2e = l3e_to_l2e(*pl3e);
-        for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
-            if ( l2e_get_intpte(pl2e[i]) != 0 )
-                break;
-        if ( i == L2_PAGETABLE_ENTRIES )
         {
-            /* Empty: zap the L3E and free the L2 page. */
-            l3e_write_atomic(pl3e, l3e_empty());
-            if ( locking )
+            l2_pgentry_t *l2t;
+
+            l2t = l3e_to_l2e(*pl3e);
+            for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
+                if ( l2e_get_intpte(l2t[i]) != 0 )
+                    break;
+            if ( i == L2_PAGETABLE_ENTRIES )
+            {
+                /* Empty: zap the L3E and free the L2 page. */
+                l3e_write_atomic(pl3e, l3e_empty());
+                if ( locking )
+                    spin_unlock(&map_pgdir_lock);
+                flush_area(NULL, FLUSH_TLB_GLOBAL); /* flush before free */
+                free_xen_pagetable(l2t);
+            }
+            else if ( locking )
                 spin_unlock(&map_pgdir_lock);
-            flush_area(NULL, FLUSH_TLB_GLOBAL); /* flush before free */
-            free_xen_pagetable(pl2e);
         }
-        else if ( locking )
-            spin_unlock(&map_pgdir_lock);
     }
 
     flush_area(NULL, FLUSH_TLB_GLOBAL);
-- 
2.17.1


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

Re: [Xen-devel] [PATCH v4 4/9] x86/mm: introduce l{1, 2}t local variables to modify_xen_mappings

[Julien Grall]

Hi,

On 04/12/2019 17:10, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@citrix.com>
> 
> The pl2e and pl1e variables are heavily (ab)used in that function.  It
> is fine at the moment because all page tables are always mapped so
> there is no need to track the life time of each variable.
> 
> We will soon have the requirement to map and unmap page tables. We
> need to track the life time of each variable to avoid leakage.
> 
> Introduce some l{1,2}t variables with limited scope so that we can
> track life time of pointers to xen page tables more easily.
> 
> No functional change.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
>   xen/arch/x86/mm.c | 68 ++++++++++++++++++++++++++---------------------
>   1 file changed, 38 insertions(+), 30 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 790578d2b3..303bc35549 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5601,6 +5601,8 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
>   
>           if ( l3e_get_flags(*pl3e) & _PAGE_PSE )
>           {
> +            l2_pgentry_t *l2t;
> +
>               if ( l2_table_offset(v) == 0 &&
>                    l1_table_offset(v) == 0 &&
>                    ((e - v) >= (1UL << L3_PAGETABLE_SHIFT)) )
> @@ -5616,11 +5618,11 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
>               }
>   
>               /* PAGE1GB: shatter the superpage and fall through. */
> -            pl2e = alloc_xen_pagetable();
> -            if ( !pl2e )
> +            l2t = alloc_xen_pagetable();
> +            if ( !l2t )
>                   return -ENOMEM;

Indirectly related to this patch, it looks like TLBs will not be flushed 
even part of the mapping is not removed.

Another problem I have spotted is most of the callers of 
map_pages_to_xen() & modify_xen_mappings() will never check the return 
value.

If we plan to use destroy_xen_mappings() for unmapping xenheap page, 
then we will need to ensure that destroy_xen_mappings() will never fail. 
Otherwise we will end up to keep part of the mappings and therefore 
defeating the purpose of secret hiding.

This may mean that shattering/merging should be prevented for xenheap 
region.

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH v4 5/9] x86/mm: map_pages_to_xen would better have one exit path

[Hongyan Xia]

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

We will soon rewrite the function to handle dynamically mapping and
unmapping of page tables.

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Hongyan Xia <hongyxia@amazon.com>

---
Changed since v3:
- remove asserts on rc since rc never gets changed to anything else
- reword commit message
---
 xen/arch/x86/mm.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 303bc35549..f7464c2103 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5187,9 +5187,11 @@ int map_pages_to_xen(
     unsigned int flags)
 {
     bool locking = system_state > SYS_STATE_boot;
+    l3_pgentry_t *pl3e, ol3e;
     l2_pgentry_t *pl2e, ol2e;
     l1_pgentry_t *pl1e, ol1e;
     unsigned int  i;
+    int rc = -ENOMEM;
 
 #define flush_flags(oldf) do {                 \
     unsigned int o_ = (oldf);                  \
@@ -5207,10 +5209,11 @@ int map_pages_to_xen(
 
     while ( nr_mfns != 0 )
     {
-        l3_pgentry_t ol3e, *pl3e = virt_to_xen_l3e(virt);
+        pl3e = virt_to_xen_l3e(virt);
 
         if ( !pl3e )
-            return -ENOMEM;
+            goto out;
+
         ol3e = *pl3e;
 
         if ( cpu_has_page1gb &&
@@ -5302,7 +5305,7 @@ int map_pages_to_xen(
 
             l2t = alloc_xen_pagetable();
             if ( l2t == NULL )
-                return -ENOMEM;
+                goto out;
 
             for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
                 l2e_write(l2t + i,
@@ -5331,7 +5334,7 @@ int map_pages_to_xen(
 
         pl2e = virt_to_xen_l2e(virt);
         if ( !pl2e )
-            return -ENOMEM;
+            goto out;
 
         if ( ((((virt >> PAGE_SHIFT) | mfn_x(mfn)) &
                ((1u << PAGETABLE_ORDER) - 1)) == 0) &&
@@ -5376,7 +5379,7 @@ int map_pages_to_xen(
             {
                 pl1e = virt_to_xen_l1e(virt);
                 if ( pl1e == NULL )
-                    return -ENOMEM;
+                    goto out;
             }
             else if ( l2e_get_flags(*pl2e) & _PAGE_PSE )
             {
@@ -5404,7 +5407,7 @@ int map_pages_to_xen(
 
                 l1t = alloc_xen_pagetable();
                 if ( l1t == NULL )
-                    return -ENOMEM;
+                    goto out;
 
                 for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
                     l1e_write(&l1t[i],
@@ -5550,7 +5553,10 @@ int map_pages_to_xen(
 
 #undef flush_flags
 
-    return 0;
+    rc = 0;
+
+ out:
+    return rc;
 }
 
 int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
-- 
2.17.1


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

[Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label in map_pages_to_xen

[Hongyan Xia]

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

We will soon need to clean up mappings whenever the out most loop is
ended. Add a new label and turn relevant continue's into goto's.

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/mm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index f7464c2103..f530dd391c 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5273,7 +5273,7 @@ int map_pages_to_xen(
             if ( !mfn_eq(mfn, INVALID_MFN) )
                 mfn  = mfn_add(mfn, 1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT));
             nr_mfns -= 1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT);
-            continue;
+            goto end_of_loop;
         }
 
         if ( (l3e_get_flags(ol3e) & _PAGE_PRESENT) &&
@@ -5300,7 +5300,7 @@ int map_pages_to_xen(
                 if ( !mfn_eq(mfn, INVALID_MFN) )
                     mfn = mfn_add(mfn, i);
                 nr_mfns -= i;
-                continue;
+                goto end_of_loop;
             }
 
             l2t = alloc_xen_pagetable();
@@ -5469,7 +5469,7 @@ int map_pages_to_xen(
                 {
                     if ( locking )
                         spin_unlock(&map_pgdir_lock);
-                    continue;
+                    goto end_of_loop;
                 }
 
                 if ( l2e_get_flags(ol2e) & _PAGE_PSE )
@@ -5524,7 +5524,7 @@ int map_pages_to_xen(
             {
                 if ( locking )
                     spin_unlock(&map_pgdir_lock);
-                continue;
+                goto end_of_loop;
             }
 
             l2t = l3e_to_l2e(ol3e);
@@ -5549,6 +5549,7 @@ int map_pages_to_xen(
             else if ( locking )
                 spin_unlock(&map_pgdir_lock);
         }
+    end_of_loop:;
     }
 
 #undef flush_flags
-- 
2.17.1


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

Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label in map_pages_to_xen

[Xia, Hongyan]

>On 02.10.2019 19:16, Hongyan Xia wrote:
>> We will soon need to clean up mappings whenever the out most loop is
>> ended. Add a new label and turn relevant continue's into goto's.
>
>I think already when this still was RFC I did indicate that I'm not
>happy about the introduction of these labels (including also patch 8).
>I realize it's quite a lot to ask, but both functions would benefit
>from splitting up into per-level helper functions, which - afaict -
>would avoid the need for such labels, and which would at the same
>time likely make it quite a bit easier to extend these to the
>5-level page tables case down the road.
>
>Thoughts?
>
>Jan

A common pattern I have found when mapping PTE pages on-demand (and I
think is the exact intention of these labels from Wei, also described
in the commit message) is that we often need to do:

map some pages - process those pages - error occurs or this iteration
of loop can be skipped - _clean up the mappings_ - continue or return

As long as cleaning up is required, these labels will likely be needed
as the clean-up path before skipping or returning, so I would say we
will see such labels even if we split it into helper functions
(virt_to_xen_l[123]e() later in the patch series is an example). I see
the labels more or less as orthogonal to modularising into helper
functions.

I would also like to see some helpers because these functions are
getting too long and convoluted, but I wonder if they could be another
mini-series outside the directmap-removal series.

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

Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label in map_pages_to_xen

[Jan Beulich]

On 05.12.2019 11:21, Xia, Hongyan wrote:
>> On 02.10.2019 19:16, Hongyan Xia wrote:
>>> We will soon need to clean up mappings whenever the out most loop is
>>> ended. Add a new label and turn relevant continue's into goto's.
>>
>> I think already when this still was RFC I did indicate that I'm not
>> happy about the introduction of these labels (including also patch 8).
>> I realize it's quite a lot to ask, but both functions would benefit
>>from splitting up into per-level helper functions, which - afaict -
>> would avoid the need for such labels, and which would at the same
>> time likely make it quite a bit easier to extend these to the
>> 5-level page tables case down the road.
> 
> A common pattern I have found when mapping PTE pages on-demand (and I
> think is the exact intention of these labels from Wei, also described
> in the commit message) is that we often need to do:
> 
> map some pages - process those pages - error occurs or this iteration
> of loop can be skipped - _clean up the mappings_ - continue or return
> 
> As long as cleaning up is required, these labels will likely be needed
> as the clean-up path before skipping or returning, so I would say we
> will see such labels even if we split it into helper functions
> (virt_to_xen_l[123]e() later in the patch series is an example). I see
> the labels more or less as orthogonal to modularising into helper
> functions.

I think differently: The fact that labels are needed is because of
the complexity of the functions. Simpler functions would allow
goto-free handling of such error conditions (by instead being able
to use continue, break, or return without making the code less
readable, often even improving readability).

Jan

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

Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label in map_pages_to_xen

[Durrant, Paul]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan
> Beulich
> Sent: 05 December 2019 10:26
> To: Xia, Hongyan <hongyxia@amazon.com>
> Cc: andrew.cooper3@citrix.com; xen-devel@lists.xenproject.org; wl@xen.org;
> roger.pau@citrix.com
> Subject: Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label
> in map_pages_to_xen
> 
> On 05.12.2019 11:21, Xia, Hongyan wrote:
> >> On 02.10.2019 19:16, Hongyan Xia wrote:
> >>> We will soon need to clean up mappings whenever the out most loop is
> >>> ended. Add a new label and turn relevant continue's into goto's.
> >>
> >> I think already when this still was RFC I did indicate that I'm not
> >> happy about the introduction of these labels (including also patch 8).
> >> I realize it's quite a lot to ask, but both functions would benefit
> >>from splitting up into per-level helper functions, which - afaict -
> >> would avoid the need for such labels, and which would at the same
> >> time likely make it quite a bit easier to extend these to the
> >> 5-level page tables case down the road.
> >
> > A common pattern I have found when mapping PTE pages on-demand (and I
> > think is the exact intention of these labels from Wei, also described
> > in the commit message) is that we often need to do:
> >
> > map some pages - process those pages - error occurs or this iteration
> > of loop can be skipped - _clean up the mappings_ - continue or return
> >
> > As long as cleaning up is required, these labels will likely be needed
> > as the clean-up path before skipping or returning, so I would say we
> > will see such labels even if we split it into helper functions
> > (virt_to_xen_l[123]e() later in the patch series is an example). I see
> > the labels more or less as orthogonal to modularising into helper
> > functions.
> 
> I think differently: The fact that labels are needed is because of
> the complexity of the functions. Simpler functions would allow
> goto-free handling of such error conditions (by instead being able
> to use continue, break, or return without making the code less
> readable, often even improving readability).

And what is wrong with using goto-s? It is a *very* common style of error handling use widely in e.g. the linux kernel. IMO it often makes error paths much more obvious and easier to reason about. In fact I very much dislike returns from the middle of functions as they can easily lead to avoidance of necessary error cleanup.

  Paul

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

Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label in map_pages_to_xen

[Jan Beulich]

On 05.12.2019 12:02, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan
>> Beulich
>> Sent: 05 December 2019 10:26
>> To: Xia, Hongyan <hongyxia@amazon.com>
>> Cc: andrew.cooper3@citrix.com; xen-devel@lists.xenproject.org; wl@xen.org;
>> roger.pau@citrix.com
>> Subject: Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label
>> in map_pages_to_xen
>>
>> On 05.12.2019 11:21, Xia, Hongyan wrote:
>>>> On 02.10.2019 19:16, Hongyan Xia wrote:
>>>>> We will soon need to clean up mappings whenever the out most loop is
>>>>> ended. Add a new label and turn relevant continue's into goto's.
>>>>
>>>> I think already when this still was RFC I did indicate that I'm not
>>>> happy about the introduction of these labels (including also patch 8).
>>>> I realize it's quite a lot to ask, but both functions would benefit
>>> >from splitting up into per-level helper functions, which - afaict -
>>>> would avoid the need for such labels, and which would at the same
>>>> time likely make it quite a bit easier to extend these to the
>>>> 5-level page tables case down the road.
>>>
>>> A common pattern I have found when mapping PTE pages on-demand (and I
>>> think is the exact intention of these labels from Wei, also described
>>> in the commit message) is that we often need to do:
>>>
>>> map some pages - process those pages - error occurs or this iteration
>>> of loop can be skipped - _clean up the mappings_ - continue or return
>>>
>>> As long as cleaning up is required, these labels will likely be needed
>>> as the clean-up path before skipping or returning, so I would say we
>>> will see such labels even if we split it into helper functions
>>> (virt_to_xen_l[123]e() later in the patch series is an example). I see
>>> the labels more or less as orthogonal to modularising into helper
>>> functions.
>>
>> I think differently: The fact that labels are needed is because of
>> the complexity of the functions. Simpler functions would allow
>> goto-free handling of such error conditions (by instead being able
>> to use continue, break, or return without making the code less
>> readable, often even improving readability).
> 
> And what is wrong with using goto-s? It is a *very* common style of
> error handling use widely in e.g. the linux kernel. IMO it often
> makes error paths much more obvious and easier to reason about. In
> fact I very much dislike returns from the middle of functions as
> they can easily lead to avoidance of necessary error cleanup.

Whereas I personally dislike goto-s (and I've been taught so when
first learning programming languages). In private code I avoid them
by all means. In projects I'm the maintainer for I accept them when
the alternative is noticeably more ugly.

Jan

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

Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label in map_pages_to_xen

[Xia, Hongyan]

I mean... I was taught so as well but I was also taught an exception
which is using it for error handling and cleaning up. I am not sure if
using alternatives would result in cleaner code in this situation.

Hongyan

On Thu, 2019-12-05 at 12:12 +0100, Jan Beulich wrote:
> On 05.12.2019 12:02, Durrant, Paul wrote:
> > > -----Original Message-----
> > > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On
> > > Behalf Of Jan
> > > Beulich
> > > Sent: 05 December 2019 10:26
> > > To: Xia, Hongyan <hongyxia@amazon.com>
> > > Cc: andrew.cooper3@citrix.com; xen-devel@lists.xenproject.org; 
> > > wl@xen.org;
> > > roger.pau@citrix.com
> > > Subject: Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an
> > > end_of_loop label
> > > in map_pages_to_xen
> > > 
> > > On 05.12.2019 11:21, Xia, Hongyan wrote:
> > > > > On 02.10.2019 19:16, Hongyan Xia wrote:
> > > > > > We will soon need to clean up mappings whenever the out
> > > > > > most loop is
> > > > > > ended. Add a new label and turn relevant continue's into
> > > > > > goto's.
> > > > > 
> > > > > I think already when this still was RFC I did indicate that
> > > > > I'm not
> > > > > happy about the introduction of these labels (including also
> > > > > patch 8).
> > > > > I realize it's quite a lot to ask, but both functions would
> > > > > benefit
> > > > > from splitting up into per-level helper functions, which -
> > > > > afaict -
> > > > > would avoid the need for such labels, and which would at the
> > > > > same
> > > > > time likely make it quite a bit easier to extend these to the
> > > > > 5-level page tables case down the road.
> > > > 
> > > > A common pattern I have found when mapping PTE pages on-demand
> > > > (and I
> > > > think is the exact intention of these labels from Wei, also
> > > > described
> > > > in the commit message) is that we often need to do:
> > > > 
> > > > map some pages - process those pages - error occurs or this
> > > > iteration
> > > > of loop can be skipped - _clean up the mappings_ - continue or
> > > > return
> > > > 
> > > > As long as cleaning up is required, these labels will likely be
> > > > needed
> > > > as the clean-up path before skipping or returning, so I would
> > > > say we
> > > > will see such labels even if we split it into helper functions
> > > > (virt_to_xen_l[123]e() later in the patch series is an
> > > > example). I see
> > > > the labels more or less as orthogonal to modularising into
> > > > helper
> > > > functions.
> > > 
> > > I think differently: The fact that labels are needed is because
> > > of
> > > the complexity of the functions. Simpler functions would allow
> > > goto-free handling of such error conditions (by instead being
> > > able
> > > to use continue, break, or return without making the code less
> > > readable, often even improving readability).
> > 
> > And what is wrong with using goto-s? It is a *very* common style of
> > error handling use widely in e.g. the linux kernel. IMO it often
> > makes error paths much more obvious and easier to reason about. In
> > fact I very much dislike returns from the middle of functions as
> > they can easily lead to avoidance of necessary error cleanup.
> 
> Whereas I personally dislike goto-s (and I've been taught so when
> first learning programming languages). In private code I avoid them
> by all means. In projects I'm the maintainer for I accept them when
> the alternative is noticeably more ugly.
> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label in map_pages_to_xen

[Xia, Hongyan]

Thanks for the suggestions. I will attempt to restructure the code.

Hongyan

On Thu, 2019-12-05 at 12:12 +0100, Jan Beulich wrote:
> On 05.12.2019 12:02, Durrant, Paul wrote:
> > > -----Original Message-----
> > > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On
> > > Behalf Of Jan
> > > Beulich
> > > Sent: 05 December 2019 10:26
> > > To: Xia, Hongyan <hongyxia@amazon.com>
> > > Cc: andrew.cooper3@citrix.com; xen-devel@lists.xenproject.org; 
> > > wl@xen.org;
> > > roger.pau@citrix.com
> > > Subject: Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an
> > > end_of_loop label
> > > in map_pages_to_xen
> > > 
> > > On 05.12.2019 11:21, Xia, Hongyan wrote:
> > > > > On 02.10.2019 19:16, Hongyan Xia wrote:
> > > > > > We will soon need to clean up mappings whenever the out
> > > > > > most loop is
> > > > > > ended. Add a new label and turn relevant continue's into
> > > > > > goto's.
> > > > > 
> > > > > I think already when this still was RFC I did indicate that
> > > > > I'm not
> > > > > happy about the introduction of these labels (including also
> > > > > patch 8).
> > > > > I realize it's quite a lot to ask, but both functions would
> > > > > benefit
> > > > > from splitting up into per-level helper functions, which -
> > > > > afaict -
> > > > > would avoid the need for such labels, and which would at the
> > > > > same
> > > > > time likely make it quite a bit easier to extend these to the
> > > > > 5-level page tables case down the road.
> > > > 
> > > > A common pattern I have found when mapping PTE pages on-demand
> > > > (and I
> > > > think is the exact intention of these labels from Wei, also
> > > > described
> > > > in the commit message) is that we often need to do:
> > > > 
> > > > map some pages - process those pages - error occurs or this
> > > > iteration
> > > > of loop can be skipped - _clean up the mappings_ - continue or
> > > > return
> > > > 
> > > > As long as cleaning up is required, these labels will likely be
> > > > needed
> > > > as the clean-up path before skipping or returning, so I would
> > > > say we
> > > > will see such labels even if we split it into helper functions
> > > > (virt_to_xen_l[123]e() later in the patch series is an
> > > > example). I see
> > > > the labels more or less as orthogonal to modularising into
> > > > helper
> > > > functions.
> > > 
> > > I think differently: The fact that labels are needed is because
> > > of
> > > the complexity of the functions. Simpler functions would allow
> > > goto-free handling of such error conditions (by instead being
> > > able
> > > to use continue, break, or return without making the code less
> > > readable, often even improving readability).
> > 
> > And what is wrong with using goto-s? It is a *very* common style of
> > error handling use widely in e.g. the linux kernel. IMO it often
> > makes error paths much more obvious and easier to reason about. In
> > fact I very much dislike returns from the middle of functions as
> > they can easily lead to avoidance of necessary error cleanup.
> 
> Whereas I personally dislike goto-s (and I've been taught so when
> first learning programming languages). In private code I avoid them
> by all means. In projects I'm the maintainer for I accept them when
> the alternative is noticeably more ugly.
> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4 7/9] x86/mm: make sure there is one exit path for modify_xen_mappings

[Hongyan Xia]

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

We will soon need to handle dynamically mapping / unmapping page
tables in the said function.

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Hongyan Xia <hongyxia@amazon.com>

---
Changed since v3:
- remove asserts on rc since it never gets changed to anything else.
---
 xen/arch/x86/mm.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index f530dd391c..4c659c28d8 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5584,6 +5584,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
     l1_pgentry_t *pl1e;
     unsigned int  i;
     unsigned long v = s;
+    int rc = -ENOMEM;
 
     /* Set of valid PTE bits which may be altered. */
 #define FLAGS_MASK (_PAGE_NX|_PAGE_RW|_PAGE_PRESENT)
@@ -5627,7 +5628,8 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
             /* PAGE1GB: shatter the superpage and fall through. */
             l2t = alloc_xen_pagetable();
             if ( !l2t )
-                return -ENOMEM;
+                goto out;
+
             for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
                 l2e_write(l2t + i,
                           l2e_from_pfn(l3e_get_pfn(*pl3e) +
@@ -5684,7 +5686,8 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
                 /* PSE: shatter the superpage and try again. */
                 l1t = alloc_xen_pagetable();
                 if ( !l1t )
-                    return -ENOMEM;
+                    goto out;
+
                 for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
                     l1e_write(&l1t[i],
                               l1e_from_pfn(l2e_get_pfn(*pl2e) + i,
@@ -5817,7 +5820,10 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
     flush_area(NULL, FLUSH_TLB_GLOBAL);
 
 #undef FLAGS_MASK
-    return 0;
+    rc = 0;
+
+ out:
+    return rc;
 }
 
 #undef flush_area
-- 
2.17.1


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

[Xen-devel] [PATCH v4 8/9] x86/mm: add an end_of_loop label in modify_xen_mappings

[Hongyan Xia]

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

We will soon need to clean up mappings whenever the out most loop
is ended. Add a new label and turn relevant continue's into goto's.

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/mm.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 4c659c28d8..d3b0956a3a 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5604,7 +5604,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
 
             v += 1UL << L3_PAGETABLE_SHIFT;
             v &= ~((1UL << L3_PAGETABLE_SHIFT) - 1);
-            continue;
+            goto end_of_loop;
         }
 
         if ( l3e_get_flags(*pl3e) & _PAGE_PSE )
@@ -5622,7 +5622,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
 
                 l3e_write_atomic(pl3e, nl3e);
                 v += 1UL << L3_PAGETABLE_SHIFT;
-                continue;
+                goto end_of_loop;
             }
 
             /* PAGE1GB: shatter the superpage and fall through. */
@@ -5663,7 +5663,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
 
             v += 1UL << L2_PAGETABLE_SHIFT;
             v &= ~((1UL << L2_PAGETABLE_SHIFT) - 1);
-            continue;
+            goto end_of_loop;
         }
 
         if ( l2e_get_flags(*pl2e) & _PAGE_PSE )
@@ -5734,7 +5734,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
              * skip the empty&free check.
              */
             if ( (nf & _PAGE_PRESENT) || ((v != e) && (l1_table_offset(v) != 0)) )
-                continue;
+                goto end_of_loop;
             if ( locking )
                 spin_lock(&map_pgdir_lock);
 
@@ -5753,7 +5753,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
             {
                 if ( locking )
                     spin_unlock(&map_pgdir_lock);
-                continue;
+                goto end_of_loop;
             }
 
             l1t = l2e_to_l1e(*pl2e);
@@ -5780,7 +5780,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
          */
         if ( (nf & _PAGE_PRESENT) ||
              ((v != e) && (l2_table_offset(v) + l1_table_offset(v) != 0)) )
-            continue;
+            goto end_of_loop;
         if ( locking )
             spin_lock(&map_pgdir_lock);
 
@@ -5793,7 +5793,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
         {
             if ( locking )
                 spin_unlock(&map_pgdir_lock);
-            continue;
+            goto end_of_loop;
         }
 
         {
@@ -5815,6 +5815,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
             else if ( locking )
                 spin_unlock(&map_pgdir_lock);
         }
+    end_of_loop:;
     }
 
     flush_area(NULL, FLUSH_TLB_GLOBAL);
-- 
2.17.1


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

[Xen-devel] [PATCH v4 9/9] x86/mm: change pl*e to l*t in virt_to_xen_l*e

[Hongyan Xia]

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

We will need to have a variable named pl*e when we rewrite
virt_to_xen_l*e. Change pl*e to l*t to reflect better its purpose.
This will make reviewing later patch easier.

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Hongyan Xia <hongyax@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/mm.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d3b0956a3a..0b8941ca7c 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5077,25 +5077,25 @@ static l3_pgentry_t *virt_to_xen_l3e(unsigned long v)
     if ( !(l4e_get_flags(*pl4e) & _PAGE_PRESENT) )
     {
         bool locking = system_state > SYS_STATE_boot;
-        l3_pgentry_t *pl3e = alloc_xen_pagetable();
+        l3_pgentry_t *l3t = alloc_xen_pagetable();
 
-        if ( !pl3e )
+        if ( !l3t )
             return NULL;
-        clear_page(pl3e);
+        clear_page(l3t);
         if ( locking )
             spin_lock(&map_pgdir_lock);
         if ( !(l4e_get_flags(*pl4e) & _PAGE_PRESENT) )
         {
-            l4_pgentry_t l4e = l4e_from_paddr(__pa(pl3e), __PAGE_HYPERVISOR);
+            l4_pgentry_t l4e = l4e_from_paddr(__pa(l3t), __PAGE_HYPERVISOR);
 
             l4e_write(pl4e, l4e);
             efi_update_l4_pgtable(l4_table_offset(v), l4e);
-            pl3e = NULL;
+            l3t = NULL;
         }
         if ( locking )
             spin_unlock(&map_pgdir_lock);
-        if ( pl3e )
-            free_xen_pagetable(pl3e);
+        if ( l3t )
+            free_xen_pagetable(l3t);
     }
 
     return l4e_to_l3e(*pl4e) + l3_table_offset(v);
@@ -5112,22 +5112,22 @@ static l2_pgentry_t *virt_to_xen_l2e(unsigned long v)
     if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) )
     {
         bool locking = system_state > SYS_STATE_boot;
-        l2_pgentry_t *pl2e = alloc_xen_pagetable();
+        l2_pgentry_t *l2t = alloc_xen_pagetable();
 
-        if ( !pl2e )
+        if ( !l2t )
             return NULL;
-        clear_page(pl2e);
+        clear_page(l2t);
         if ( locking )
             spin_lock(&map_pgdir_lock);
         if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) )
         {
-            l3e_write(pl3e, l3e_from_paddr(__pa(pl2e), __PAGE_HYPERVISOR));
-            pl2e = NULL;
+            l3e_write(pl3e, l3e_from_paddr(__pa(l2t), __PAGE_HYPERVISOR));
+            l2t = NULL;
         }
         if ( locking )
             spin_unlock(&map_pgdir_lock);
-        if ( pl2e )
-            free_xen_pagetable(pl2e);
+        if ( l2t )
+            free_xen_pagetable(l2t);
     }
 
     BUG_ON(l3e_get_flags(*pl3e) & _PAGE_PSE);
@@ -5145,22 +5145,22 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v)
     if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )
     {
         bool locking = system_state > SYS_STATE_boot;
-        l1_pgentry_t *pl1e = alloc_xen_pagetable();
+        l1_pgentry_t *l1t = alloc_xen_pagetable();
 
-        if ( !pl1e )
+        if ( !l1t )
             return NULL;
-        clear_page(pl1e);
+        clear_page(l1t);
         if ( locking )
             spin_lock(&map_pgdir_lock);
         if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )
         {
-            l2e_write(pl2e, l2e_from_paddr(__pa(pl1e), __PAGE_HYPERVISOR));
-            pl1e = NULL;
+            l2e_write(pl2e, l2e_from_paddr(__pa(l1t), __PAGE_HYPERVISOR));
+            l1t = NULL;
         }
         if ( locking )
             spin_unlock(&map_pgdir_lock);
-        if ( pl1e )
-            free_xen_pagetable(pl1e);
+        if ( l1t )
+            free_xen_pagetable(l1t);
     }
 
     BUG_ON(l2e_get_flags(*pl2e) & _PAGE_PSE);
-- 
2.17.1


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

Re: [Xen-devel] [PATCH v4 0/9] Add alternative API for Xen PTEs

[Jan Beulich]

On 04.12.2019 18:10, Hongyan Xia wrote:
> NOTE: My email address has changed due to some HR management. I have
> lost all my previous emails and I could only salvage some of the
> comments to v3 from the mailing list archive. I will reply to the
> comments from v3 in this v4 series.

I'm afraid this isn't very helpful. In particular, does this mean
v4 is effectively v3, i.e. no review comments taken care of? Or
just some of them, and others left out? I'm not fancying re-
reviewing a version that doesn't have prior comments taken care
of. Please clarify.

Jan

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

Re: [Xen-devel] [PATCH v4 0/9] Add alternative API for Xen PTEs

[Xia, Hongyan]

I have addressed the comments that I can find in the archive. Some big
debates (like _new or not _new, whether to modularise map_pages_to_xen)
have not been touched. Various acked-by and reviewed-by added.

Hongyan

On Thu, 2019-12-05 at 10:14 +0100, Jan Beulich wrote:
> On 04.12.2019 18:10, Hongyan Xia wrote:
> > NOTE: My email address has changed due to some HR management. I
> > have
> > lost all my previous emails and I could only salvage some of the
> > comments to v3 from the mailing list archive. I will reply to the
> > comments from v3 in this v4 series.
> 
> I'm afraid this isn't very helpful. In particular, does this mean
> v4 is effectively v3, i.e. no review comments taken care of? Or
> just some of them, and others left out? I'm not fancying re-
> reviewing a version that doesn't have prior comments taken care
> of. Please clarify.
> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 0/9] Add alternative API for Xen PTEs

[Jan Beulich]

On 05.12.2019 10:41, Xia, Hongyan wrote:
> I have addressed the comments that I can find in the archive.

That's still pretty vague - is there reason to assume you were
not able to find some comments there?

> Some big
> debates (like _new or not _new, whether to modularise map_pages_to_xen)
> have not been touched.

The _new suffix discussion you've meanwhile responded you,
which is therefore fine. The modularization question, otoh,
I don't recall seeing any reply for, and hence for now I'd
skip re-reviewing the respective patches. Furthermore, is
your use of "like" implying there were more than the two
examples you gave? As much as I can understand difficulties
on your part resulting from your email issues, please also
understand my reservations regarding having to re-do things
where quite a bit of time had already been invested into.

Jan

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

Re: [Xen-devel] [PATCH v4 0/9] Add alternative API for Xen PTEs

[Xia, Hongyan]

Okay let me be explicit this time. Cross checked with mails from
lore.kernel.org, issues not touched from v3 to v4:

- _new or not _new
- splitting map_pages_to_xen; introduction of labels. Although I just
responded to these two issues.
- const not added to suggested variables since a lot of them are stuck
with the old API for now. I can review relevant functions and const
qualify other applicable ones.

Hongyan

On Thu, 2019-12-05 at 10:51 +0100, Jan Beulich wrote:
> On 05.12.2019 10:41, Xia, Hongyan wrote:
> > I have addressed the comments that I can find in the archive.
> 
> That's still pretty vague - is there reason to assume you were
> not able to find some comments there?
> 
> > Some big
> > debates (like _new or not _new, whether to modularise
> > map_pages_to_xen)
> > have not been touched.
> 
> The _new suffix discussion you've meanwhile responded you,
> which is therefore fine. The modularization question, otoh,
> I don't recall seeing any reply for, and hence for now I'd
> skip re-reviewing the respective patches. Furthermore, is
> your use of "like" implying there were more than the two
> examples you gave? As much as I can understand difficulties
> on your part resulting from your email issues, please also
> understand my reservations regarding having to re-do things
> where quite a bit of time had already been invested into.
> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel