xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [Xen-devel] [PATCH v3 0/2] Refactor super page shattering
@ 2019-12-11 10:58 Hongyan Xia
  2019-12-11 10:58 ` [Xen-devel] [PATCH v3 1/2] x86/mm: factor out the code for shattering an l3 PTE Hongyan Xia
  2019-12-11 10:58 ` [Xen-devel] [PATCH v3 2/2] x86/mm: factor out the code for shattering an l2 PTE Hongyan Xia
  0 siblings, 2 replies; 10+ messages in thread
From: Hongyan Xia @ 2019-12-11 10:58 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, jgrall, Wei Liu, Jan Beulich, Roger Pau Monné

map_pages_to_xen and modify_xen_mappings use almost exactly the same
page shattering logic, and the code is mingled with other PTE
manipulations so it is less obvious that the intention is page
shattering. Factor out the functions to make them reusable and to make
the intention more obvious.

Of course, there is not much difference between the shattering logic of
each level, so we could further turn the per-level functions into a
single macro, although this is not that simple since we have per-level
functions and macros all over the place and there are slight differences
between levels. Keep it per-level for now.

tree:
https://xenbits.xen.org/git-http/people/hx242/xen.git shatter-refactor

---
Changes in v3:
- style and indentation fixes.

Changes in v2:
- rebase.
- improve asm code.
- avoid stale values when taking the lock.
- move allocation of PTE tables inside the shatter function.

Hongyan Xia (2):
  x86/mm: factor out the code for shattering an l3 PTE
  x86/mm: factor out the code for shattering an l2 PTE

 xen/arch/x86/mm.c | 194 +++++++++++++++++++++++-----------------------
 1 file changed, 98 insertions(+), 96 deletions(-)

-- 
2.17.1


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Xen-devel] [PATCH v3 1/2] x86/mm: factor out the code for shattering an l3 PTE
  2019-12-11 10:58 [Xen-devel] [PATCH v3 0/2] Refactor super page shattering Hongyan Xia
@ 2019-12-11 10:58 ` Hongyan Xia
  2019-12-11 15:29   ` Jan Beulich
  2019-12-11 15:32   ` Jan Beulich
  2019-12-11 10:58 ` [Xen-devel] [PATCH v3 2/2] x86/mm: factor out the code for shattering an l2 PTE Hongyan Xia
  1 sibling, 2 replies; 10+ messages in thread
From: Hongyan Xia @ 2019-12-11 10:58 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, jgrall, Wei Liu, Jan Beulich, Roger Pau Monné

map_pages_to_xen and modify_xen_mappings are performing almost exactly
the same operations when shattering an l3 PTE, the only difference
being whether we want to flush.

Signed-off-by: Hongyan Xia <hongyxia@amazon.com>

---
Changes in v3:
- style and indentation changes.
- return -ENOMEM instead of -1.

Changes in v2:
- improve asm.
- re-read pl3e from memory when taking the lock.
- move the allocation of l2t inside the shatter function.
---
 xen/arch/x86/mm.c | 98 +++++++++++++++++++++++------------------------
 1 file changed, 49 insertions(+), 49 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 7d4dd80a85..97f11b6016 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5151,6 +5151,52 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v)
                          flush_area_local((const void *)v, f) : \
                          flush_area_all((const void *)v, f))
 
+/* Shatter an l3 entry and populate l2. If virt is passed in, also do flush. */
+static int shatter_l3e(l3_pgentry_t *pl3e, unsigned long virt, bool locking)
+{
+    unsigned int i;
+    l3_pgentry_t ol3e = *pl3e;
+    l2_pgentry_t l2e = l2e_from_intpte(l3e_get_intpte(ol3e));
+    l2_pgentry_t *l2t = alloc_xen_pagetable();
+
+    if ( !l2t )
+        return -ENOMEM;
+
+    for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
+    {
+        l2e_write(l2t + i, l2e);
+        l2e = l2e_from_intpte(
+                  l2e_get_intpte(l2e) + (PAGE_SIZE << PAGETABLE_ORDER));
+    }
+
+    if ( locking )
+        spin_lock(&map_pgdir_lock);
+    if ( (l3e_get_flags(*pl3e) & _PAGE_PRESENT) &&
+         (l3e_get_flags(*pl3e) & _PAGE_PSE) )
+    {
+        l3e_write_atomic(pl3e,
+            l3e_from_paddr((paddr_t)virt_to_maddr(l2t), __PAGE_HYPERVISOR));
+        l2t = NULL;
+    }
+    if ( locking )
+        spin_unlock(&map_pgdir_lock);
+
+    if ( virt )
+    {
+        unsigned int flush_flags =
+            FLUSH_TLB | FLUSH_ORDER(2 * PAGETABLE_ORDER);
+
+        if ( l3e_get_flags(ol3e) & _PAGE_GLOBAL )
+            flush_flags |= FLUSH_TLB_GLOBAL;
+        flush_area(virt, flush_flags);
+    }
+
+    if ( l2t )
+        free_xen_pagetable(l2t);
+
+    return 0;
+}
+
 int map_pages_to_xen(
     unsigned long virt,
     mfn_t mfn,
@@ -5244,9 +5290,6 @@ int map_pages_to_xen(
         if ( (l3e_get_flags(ol3e) & _PAGE_PRESENT) &&
              (l3e_get_flags(ol3e) & _PAGE_PSE) )
         {
-            unsigned int flush_flags =
-                FLUSH_TLB | FLUSH_ORDER(2 * PAGETABLE_ORDER);
-
             /* Skip this PTE if there is no change. */
             if ( ((l3e_get_pfn(ol3e) & ~(L2_PAGETABLE_ENTRIES *
                                          L1_PAGETABLE_ENTRIES - 1)) +
@@ -5267,33 +5310,9 @@ int map_pages_to_xen(
                 continue;
             }
 
-            pl2e = alloc_xen_pagetable();
-            if ( pl2e == NULL )
+            /* Pass virt to indicate we need to flush. */
+            if ( shatter_l3e(pl3e, virt, locking) )
                 return -ENOMEM;
-
-            for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
-                l2e_write(pl2e + i,
-                          l2e_from_pfn(l3e_get_pfn(ol3e) +
-                                       (i << PAGETABLE_ORDER),
-                                       l3e_get_flags(ol3e)));
-
-            if ( l3e_get_flags(ol3e) & _PAGE_GLOBAL )
-                flush_flags |= FLUSH_TLB_GLOBAL;
-
-            if ( locking )
-                spin_lock(&map_pgdir_lock);
-            if ( (l3e_get_flags(*pl3e) & _PAGE_PRESENT) &&
-                 (l3e_get_flags(*pl3e) & _PAGE_PSE) )
-            {
-                l3e_write_atomic(pl3e, l3e_from_mfn(virt_to_mfn(pl2e),
-                                                    __PAGE_HYPERVISOR));
-                pl2e = NULL;
-            }
-            if ( locking )
-                spin_unlock(&map_pgdir_lock);
-            flush_area(virt, flush_flags);
-            if ( pl2e )
-                free_xen_pagetable(pl2e);
         }
 
         pl2e = virt_to_xen_l2e(virt);
@@ -5578,27 +5597,8 @@ 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 )
+            if ( shatter_l3e(pl3e, 0, locking) )
                 return -ENOMEM;
-            for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
-                l2e_write(pl2e + i,
-                          l2e_from_pfn(l3e_get_pfn(*pl3e) +
-                                       (i << PAGETABLE_ORDER),
-                                       l3e_get_flags(*pl3e)));
-            if ( locking )
-                spin_lock(&map_pgdir_lock);
-            if ( (l3e_get_flags(*pl3e) & _PAGE_PRESENT) &&
-                 (l3e_get_flags(*pl3e) & _PAGE_PSE) )
-            {
-                l3e_write_atomic(pl3e, l3e_from_mfn(virt_to_mfn(pl2e),
-                                                    __PAGE_HYPERVISOR));
-                pl2e = NULL;
-            }
-            if ( locking )
-                spin_unlock(&map_pgdir_lock);
-            if ( pl2e )
-                free_xen_pagetable(pl2e);
         }
 
         /*
-- 
2.17.1


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [Xen-devel] [PATCH v3 2/2] x86/mm: factor out the code for shattering an l2 PTE
  2019-12-11 10:58 [Xen-devel] [PATCH v3 0/2] Refactor super page shattering Hongyan Xia
  2019-12-11 10:58 ` [Xen-devel] [PATCH v3 1/2] x86/mm: factor out the code for shattering an l3 PTE Hongyan Xia
@ 2019-12-11 10:58 ` Hongyan Xia
  2019-12-11 15:33   ` Jan Beulich
  1 sibling, 1 reply; 10+ messages in thread
From: Hongyan Xia @ 2019-12-11 10:58 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, jgrall, Wei Liu, Jan Beulich, Roger Pau Monné

map_pages_to_xen and modify_xen_mappings are performing almost exactly
the same operations when shattering an l2 PTE, the only difference
being whether we want to flush.

Signed-off-by: Hongyan Xia <hongyxia@amazon.com>

---
Changes in v3:
- style and indentation changes.
- return -ENOMEM instead of -1.

Changes in v2:
- improve asm.
- re-read pl2e from memory when taking the lock.
- move the allocation of l1t inside the shatter function.
---
 xen/arch/x86/mm.c | 96 ++++++++++++++++++++++++-----------------------
 1 file changed, 49 insertions(+), 47 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 97f11b6016..e5ba6b52fb 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5151,6 +5151,52 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v)
                          flush_area_local((const void *)v, f) : \
                          flush_area_all((const void *)v, f))
 
+/* Shatter an l2 entry and populate l1. If virt is passed in, also do flush. */
+static int shatter_l2e(l2_pgentry_t *pl2e, unsigned long virt, bool locking)
+{
+    unsigned int i;
+    l2_pgentry_t ol2e = *pl2e;
+    l1_pgentry_t l1e = l1e_from_paddr(l2e_get_paddr(ol2e),
+                                      lNf_to_l1f(l2e_get_flags(ol2e)));
+    l1_pgentry_t *l1t = alloc_xen_pagetable();
+
+    if ( !l1t )
+        return -ENOMEM;
+
+    for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
+    {
+        l1e_write(l1t + i, l1e);
+        l1e = l1e_from_intpte(l1e_get_intpte(l1e) + PAGE_SIZE);
+    }
+
+    if ( locking )
+        spin_lock(&map_pgdir_lock);
+    if ( (l2e_get_flags(*pl2e) & _PAGE_PRESENT) &&
+         (l2e_get_flags(*pl2e) & _PAGE_PSE) )
+    {
+        l2e_write_atomic(pl2e,
+            l2e_from_paddr((paddr_t)virt_to_maddr(l1t), __PAGE_HYPERVISOR));
+        l1t = NULL;
+    }
+    if ( locking )
+        spin_unlock(&map_pgdir_lock);
+
+    if ( virt )
+    {
+        unsigned int flush_flags =
+            FLUSH_TLB | FLUSH_ORDER(PAGETABLE_ORDER);
+
+        if ( l2e_get_flags(ol2e) & _PAGE_GLOBAL )
+            flush_flags |= FLUSH_TLB_GLOBAL;
+        flush_area(virt, flush_flags);
+    }
+
+    if ( l1t )
+        free_xen_pagetable(l1t);
+
+    return 0;
+}
+
 /* Shatter an l3 entry and populate l2. If virt is passed in, also do flush. */
 static int shatter_l3e(l3_pgentry_t *pl3e, unsigned long virt, bool locking)
 {
@@ -5364,9 +5410,6 @@ int map_pages_to_xen(
             }
             else if ( l2e_get_flags(*pl2e) & _PAGE_PSE )
             {
-                unsigned int flush_flags =
-                    FLUSH_TLB | FLUSH_ORDER(PAGETABLE_ORDER);
-
                 /* Skip this PTE if there is no change. */
                 if ( (((l2e_get_pfn(*pl2e) & ~(L1_PAGETABLE_ENTRIES - 1)) +
                        l1_table_offset(virt)) == mfn_x(mfn)) &&
@@ -5385,32 +5428,9 @@ int map_pages_to_xen(
                     goto check_l3;
                 }
 
-                pl1e = alloc_xen_pagetable();
-                if ( pl1e == NULL )
+                /* Pass virt to indicate we need to flush. */
+                if ( shatter_l2e(pl2e, virt, locking) )
                     return -ENOMEM;
-
-                for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
-                    l1e_write(&pl1e[i],
-                              l1e_from_pfn(l2e_get_pfn(*pl2e) + i,
-                                           lNf_to_l1f(l2e_get_flags(*pl2e))));
-
-                if ( l2e_get_flags(*pl2e) & _PAGE_GLOBAL )
-                    flush_flags |= FLUSH_TLB_GLOBAL;
-
-                if ( locking )
-                    spin_lock(&map_pgdir_lock);
-                if ( (l2e_get_flags(*pl2e) & _PAGE_PRESENT) &&
-                     (l2e_get_flags(*pl2e) & _PAGE_PSE) )
-                {
-                    l2e_write_atomic(pl2e, l2e_from_mfn(virt_to_mfn(pl1e),
-                                                        __PAGE_HYPERVISOR));
-                    pl1e = NULL;
-                }
-                if ( locking )
-                    spin_unlock(&map_pgdir_lock);
-                flush_area(virt, flush_flags);
-                if ( pl1e )
-                    free_xen_pagetable(pl1e);
             }
 
             pl1e  = l2e_to_l1e(*pl2e) + l1_table_offset(virt);
@@ -5633,26 +5653,8 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
             else
             {
                 /* PSE: shatter the superpage and try again. */
-                pl1e = alloc_xen_pagetable();
-                if ( !pl1e )
+                if ( shatter_l2e(pl2e, 0, locking) )
                     return -ENOMEM;
-                for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
-                    l1e_write(&pl1e[i],
-                              l1e_from_pfn(l2e_get_pfn(*pl2e) + i,
-                                           l2e_get_flags(*pl2e) & ~_PAGE_PSE));
-                if ( locking )
-                    spin_lock(&map_pgdir_lock);
-                if ( (l2e_get_flags(*pl2e) & _PAGE_PRESENT) &&
-                     (l2e_get_flags(*pl2e) & _PAGE_PSE) )
-                {
-                    l2e_write_atomic(pl2e, l2e_from_mfn(virt_to_mfn(pl1e),
-                                                        __PAGE_HYPERVISOR));
-                    pl1e = NULL;
-                }
-                if ( locking )
-                    spin_unlock(&map_pgdir_lock);
-                if ( pl1e )
-                    free_xen_pagetable(pl1e);
             }
         }
         else
-- 
2.17.1


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [Xen-devel] [PATCH v3 1/2] x86/mm: factor out the code for shattering an l3 PTE
  2019-12-11 10:58 ` [Xen-devel] [PATCH v3 1/2] x86/mm: factor out the code for shattering an l3 PTE Hongyan Xia
@ 2019-12-11 15:29   ` Jan Beulich
  2019-12-11 16:34     ` Xia, Hongyan
  2019-12-11 15:32   ` Jan Beulich
  1 sibling, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2019-12-11 15:29 UTC (permalink / raw)
  To: Hongyan Xia, Andrew Cooper
  Cc: xen-devel, jgrall, Wei Liu, Roger Pau Monné

On 11.12.2019 11:58, Hongyan Xia wrote:
> map_pages_to_xen and modify_xen_mappings are performing almost exactly
> the same operations when shattering an l3 PTE, the only difference
> being whether we want to flush.
> 
> Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
> 
> ---
> Changes in v3:
> - style and indentation changes.
> - return -ENOMEM instead of -1.
> 
> Changes in v2:
> - improve asm.
> - re-read pl3e from memory when taking the lock.
> - move the allocation of l2t inside the shatter function.
> ---
>  xen/arch/x86/mm.c | 98 +++++++++++++++++++++++------------------------
>  1 file changed, 49 insertions(+), 49 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 7d4dd80a85..97f11b6016 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5151,6 +5151,52 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v)
>                           flush_area_local((const void *)v, f) : \
>                           flush_area_all((const void *)v, f))
>  
> +/* Shatter an l3 entry and populate l2. If virt is passed in, also do flush. */
> +static int shatter_l3e(l3_pgentry_t *pl3e, unsigned long virt, bool locking)
> +{
> +    unsigned int i;
> +    l3_pgentry_t ol3e = *pl3e;
> +    l2_pgentry_t l2e = l2e_from_intpte(l3e_get_intpte(ol3e));
> +    l2_pgentry_t *l2t = alloc_xen_pagetable();
> +
> +    if ( !l2t )
> +        return -ENOMEM;
> +
> +    for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
> +    {
> +        l2e_write(l2t + i, l2e);
> +        l2e = l2e_from_intpte(
> +                  l2e_get_intpte(l2e) + (PAGE_SIZE << PAGETABLE_ORDER));

Andrew - iirc you had suggested this (in some different form, but
to the same effect) to improve code generation. If you're convinced
that the downside of the loss of type safety is worth the win in
generated code, I'm not going to stand in the way here, but it'll
then need to be you to ack these two patches in their eventually
final shape.

> +    }
> +
> +    if ( locking )
> +        spin_lock(&map_pgdir_lock);
> +    if ( (l3e_get_flags(*pl3e) & _PAGE_PRESENT) &&
> +         (l3e_get_flags(*pl3e) & _PAGE_PSE) )
> +    {
> +        l3e_write_atomic(pl3e,
> +            l3e_from_paddr((paddr_t)virt_to_maddr(l2t), __PAGE_HYPERVISOR));

Why the cast? (I'm sorry if this was there on v3 already and I
didn't spot it. And if this remains the only thing to adjust,
then I guess this could be taken care of while committing.)

Jan

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Xen-devel] [PATCH v3 1/2] x86/mm: factor out the code for shattering an l3 PTE
  2019-12-11 10:58 ` [Xen-devel] [PATCH v3 1/2] x86/mm: factor out the code for shattering an l3 PTE Hongyan Xia
  2019-12-11 15:29   ` Jan Beulich
@ 2019-12-11 15:32   ` Jan Beulich
  2019-12-11 16:27     ` Xia, Hongyan
  1 sibling, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2019-12-11 15:32 UTC (permalink / raw)
  To: Hongyan Xia
  Cc: xen-devel, jgrall, Roger Pau Monné, Wei Liu, Andrew Cooper

On 11.12.2019 11:58, Hongyan Xia wrote:
> @@ -5578,27 +5597,8 @@ 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 )
> +            if ( shatter_l3e(pl3e, 0, locking) )
>                  return -ENOMEM;

Hmm, I didn't expect I'd need to comment on this again: As per
my v2 reply, you should hand on the return value from the
function, not make up your own. This is so that in case the
function gains another error path with a different error code,
it wouldn't become indistinguishable to callers further up.

Jan

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Xen-devel] [PATCH v3 2/2] x86/mm: factor out the code for shattering an l2 PTE
  2019-12-11 10:58 ` [Xen-devel] [PATCH v3 2/2] x86/mm: factor out the code for shattering an l2 PTE Hongyan Xia
@ 2019-12-11 15:33   ` Jan Beulich
  0 siblings, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2019-12-11 15:33 UTC (permalink / raw)
  To: Hongyan Xia
  Cc: xen-devel, jgrall, Roger Pau Monné, Wei Liu, Andrew Cooper

On 11.12.2019 11:58, Hongyan Xia wrote:
> map_pages_to_xen and modify_xen_mappings are performing almost exactly
> the same operations when shattering an l2 PTE, the only difference
> being whether we want to flush.
> 
> Signed-off-by: Hongyan Xia <hongyxia@amazon.com>

As before comments on patch 1 apply here as well.

Jan

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Xen-devel] [PATCH v3 1/2] x86/mm: factor out the code for shattering an l3 PTE
  2019-12-11 15:32   ` Jan Beulich
@ 2019-12-11 16:27     ` Xia, Hongyan
  2019-12-11 16:58       ` Jan Beulich
  0 siblings, 1 reply; 10+ messages in thread
From: Xia, Hongyan @ 2019-12-11 16:27 UTC (permalink / raw)
  To: jbeulich; +Cc: andrew.cooper3, Grall, Julien, xen-devel, wl, roger.pau

On Wed, 2019-12-11 at 16:32 +0100, Jan Beulich wrote:
> On 11.12.2019 11:58, Hongyan Xia wrote:
> > @@ -5578,27 +5597,8 @@ 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 )
> > +            if ( shatter_l3e(pl3e, 0, locking) )
> >                  return -ENOMEM;
> 
> Hmm, I didn't expect I'd need to comment on this again: As per
> my v2 reply, you should hand on the return value from the
> function, not make up your own. This is so that in case the
> function gains another error path with a different error code,
> it wouldn't become indistinguishable to callers further up.
> 

I was basically thinking about the conversation we had that ENOMEM is
probably the only error value map_pages_to_xen would return ever, and
it is unlikely to gain another return value in the future, so initially
I just let shatter return -1 and the caller return -ENOMEM. There is no
problem for me if we want to change it to handle different error
values.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Xen-devel] [PATCH v3 1/2] x86/mm: factor out the code for shattering an l3 PTE
  2019-12-11 15:29   ` Jan Beulich
@ 2019-12-11 16:34     ` Xia, Hongyan
  2019-12-11 17:00       ` Jan Beulich
  0 siblings, 1 reply; 10+ messages in thread
From: Xia, Hongyan @ 2019-12-11 16:34 UTC (permalink / raw)
  To: jbeulich, andrew.cooper3; +Cc: xen-devel, Grall,  Julien, wl, roger.pau

On Wed, 2019-12-11 at 16:29 +0100, Jan Beulich wrote:
> > +    }
> > +
> > +    if ( locking )
> > +        spin_lock(&map_pgdir_lock);
> > +    if ( (l3e_get_flags(*pl3e) & _PAGE_PRESENT) &&
> > +         (l3e_get_flags(*pl3e) & _PAGE_PSE) )
> > +    {
> > +        l3e_write_atomic(pl3e,
> > +            l3e_from_paddr((paddr_t)virt_to_maddr(l2t),
> > __PAGE_HYPERVISOR));
> 
> Why the cast? (I'm sorry if this was there on v3 already and I
> didn't spot it. And if this remains the only thing to adjust,
> then I guess this could be taken care of while committing.)
> 
> Jan

Sadly there is no l3e_from_maddr or virt_to_paddr to call directly. Of
course, paddr_t and maddr have the same underlying type (unsigned
long), so it works without a cast. I just added the cast to make it
explicit that these two are not exactly the same.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Xen-devel] [PATCH v3 1/2] x86/mm: factor out the code for shattering an l3 PTE
  2019-12-11 16:27     ` Xia, Hongyan
@ 2019-12-11 16:58       ` Jan Beulich
  0 siblings, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2019-12-11 16:58 UTC (permalink / raw)
  To: Xia, Hongyan; +Cc: andrew.cooper3, Grall, Julien, xen-devel, wl, roger.pau

On 11.12.2019 17:27, Xia, Hongyan wrote:
> On Wed, 2019-12-11 at 16:32 +0100, Jan Beulich wrote:
>> On 11.12.2019 11:58, Hongyan Xia wrote:
>>> @@ -5578,27 +5597,8 @@ 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 )
>>> +            if ( shatter_l3e(pl3e, 0, locking) )
>>>                  return -ENOMEM;
>>
>> Hmm, I didn't expect I'd need to comment on this again: As per
>> my v2 reply, you should hand on the return value from the
>> function, not make up your own. This is so that in case the
>> function gains another error path with a different error code,
>> it wouldn't become indistinguishable to callers further up.
> 
> I was basically thinking about the conversation we had that ENOMEM is
> probably the only error value map_pages_to_xen would return ever, and
> it is unlikely to gain another return value in the future, so initially
> I just let shatter return -1 and the caller return -ENOMEM. There is no
> problem for me if we want to change it to handle different error
> values.

The alternative to your prior 0 / -1 returning would have been to
have the function return bool. In this case "inventing" an error
code here would be fine. The 0 / -1 approach would introduce
another instance of what we're trying to get rid of elsewhere.

Jan

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Xen-devel] [PATCH v3 1/2] x86/mm: factor out the code for shattering an l3 PTE
  2019-12-11 16:34     ` Xia, Hongyan
@ 2019-12-11 17:00       ` Jan Beulich
  0 siblings, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2019-12-11 17:00 UTC (permalink / raw)
  To: Xia, Hongyan; +Cc: andrew.cooper3, Grall, Julien, xen-devel, wl, roger.pau

On 11.12.2019 17:34, Xia, Hongyan wrote:
> On Wed, 2019-12-11 at 16:29 +0100, Jan Beulich wrote:
>>> +    }
>>> +
>>> +    if ( locking )
>>> +        spin_lock(&map_pgdir_lock);
>>> +    if ( (l3e_get_flags(*pl3e) & _PAGE_PRESENT) &&
>>> +         (l3e_get_flags(*pl3e) & _PAGE_PSE) )
>>> +    {
>>> +        l3e_write_atomic(pl3e,
>>> +            l3e_from_paddr((paddr_t)virt_to_maddr(l2t),
>>> __PAGE_HYPERVISOR));
>>
>> Why the cast? (I'm sorry if this was there on v3 already and I
>> didn't spot it. And if this remains the only thing to adjust,
>> then I guess this could be taken care of while committing.)
> 
> Sadly there is no l3e_from_maddr or virt_to_paddr to call directly. Of
> course, paddr_t and maddr have the same underlying type (unsigned
> long), so it works without a cast. I just added the cast to make it
> explicit that these two are not exactly the same.

Yes, there continues to be a naming disconnect. But no, this is
not a reason to add a cast. Casts should be used as sparingly
as possible, since they tend to hide problems.

Jan

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-12-11 16:59 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-11 10:58 [Xen-devel] [PATCH v3 0/2] Refactor super page shattering Hongyan Xia
2019-12-11 10:58 ` [Xen-devel] [PATCH v3 1/2] x86/mm: factor out the code for shattering an l3 PTE Hongyan Xia
2019-12-11 15:29   ` Jan Beulich
2019-12-11 16:34     ` Xia, Hongyan
2019-12-11 17:00       ` Jan Beulich
2019-12-11 15:32   ` Jan Beulich
2019-12-11 16:27     ` Xia, Hongyan
2019-12-11 16:58       ` Jan Beulich
2019-12-11 10:58 ` [Xen-devel] [PATCH v3 2/2] x86/mm: factor out the code for shattering an l2 PTE Hongyan Xia
2019-12-11 15:33   ` Jan Beulich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).