[v3,5/9] x86/mm: map_pages_to_xen should have one exit path

Commit Message

Xia, Hongyan Oct. 2, 2019, 5:16 p.m. UTC

From: Wei Liu <[e-mail]>

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

No functional change.

Signed-off-by: Wei Liu <[e-mail]>
---
 xen/arch/x86/mm.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

Comments

Jan Beulich Nov. 20, 2019, 4:24 p.m. UTC | #1

On 02.10.2019 19:16, Hongyan Xia wrote:
> @@ -5034,10 +5036,13 @@ 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;
> +        {
> +            ASSERT(rc == -ENOMEM);
> +            goto out;
> +        }

rc never gets changed to any other error code, and I also can't
foresee this happening in the future. Are all these ASSERT()s
(and the associated brace pairs) really helpful?

Also I think I'd prefer a less strong title, e.g. "would better"
instead of "should".

Jan

Patch

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 2b8e192e26..26fcb2709b 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5014,9 +5014,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);                  \
@@ -5034,10 +5036,13 @@  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;
+        {
+            ASSERT(rc == -ENOMEM);
+            goto out;
+        }
         ol3e = *pl3e;
 
         if ( cpu_has_page1gb &&
@@ -5129,7 +5134,10 @@  int map_pages_to_xen(
 
             l2t = alloc_xen_pagetable();
             if ( l2t == NULL )
-                return -ENOMEM;
+            {
+                ASSERT(rc == -ENOMEM);
+                goto out;
+            }
 
             for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
                 l2e_write(l2t + i,
@@ -5158,7 +5166,10 @@  int map_pages_to_xen(
 
         pl2e = virt_to_xen_l2e(virt);
         if ( !pl2e )
-            return -ENOMEM;
+        {
+            ASSERT(rc == -ENOMEM);
+            goto out;
+        }
 
         if ( ((((virt >> PAGE_SHIFT) | mfn_x(mfn)) &
                ((1u << PAGETABLE_ORDER) - 1)) == 0) &&
@@ -5203,7 +5214,10 @@  int map_pages_to_xen(
             {
                 pl1e = virt_to_xen_l1e(virt);
                 if ( pl1e == NULL )
-                    return -ENOMEM;
+                {
+                    ASSERT(rc == -ENOMEM);
+                    goto out;
+                }
             }
             else if ( l2e_get_flags(*pl2e) & _PAGE_PSE )
             {
@@ -5231,7 +5245,10 @@  int map_pages_to_xen(
 
                 l1t = alloc_xen_pagetable();
                 if ( l1t == NULL )
-                    return -ENOMEM;
+                {
+                    ASSERT(rc == -ENOMEM);
+                    goto out;
+                }
 
                 for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
                     l1e_write(&l1t[i],
@@ -5377,7 +5394,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)