Re: [PATCH 3/9] gnttab: fold recurring is_iomem_page()

[Julien Grall]

Hi Jan,

On 26/08/2021 11:12, Jan Beulich wrote:
In all cases call the function just once instead of up to four times, at

extra NIT: It is more like two because there is a else in 
gnttab_release_mappings() :)

the same time avoiding to store a dangling pointer in a local variable.

Signed-off-by: Jan Beulich <jbeul...@suse.com>

Reviewed-by: Julien Grall <jgr...@amazon.com>


--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c

[...]

Everything below looks a duplicate. Might be an issue in your tools?

In all cases call the function just once instead of up to four times, at
the same time avoiding to store a dangling pointer in a local variable.

Signed-off-by: Jan Beulich <jbeul...@suse.com>

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1587,11 +1587,11 @@ unmap_common_complete(struct gnttab_unma
      else
          status = &status_entry(rgt, op->ref);
  
-    pg = mfn_to_page(op->mfn);
+    pg = !is_iomem_page(act->mfn) ? mfn_to_page(op->mfn) : NULL;
  
      if ( op->done & GNTMAP_device_map )
      {
-        if ( !is_iomem_page(act->mfn) )
+        if ( pg )
          {
              if ( op->done & GNTMAP_readonly )
                  put_page(pg);
@@ -1608,7 +1608,7 @@ unmap_common_complete(struct gnttab_unma
  
      if ( op->done & GNTMAP_host_map )
      {
-        if ( !is_iomem_page(op->mfn) )
+        if ( pg )
          {
              if ( gnttab_host_mapping_get_page_type(op->done & GNTMAP_readonly,
                                                     ld, rd) )
@@ -3778,7 +3778,7 @@ int gnttab_release_mappings(struct domai
          else
              status = &status_entry(rgt, ref);
  
-        pg = mfn_to_page(act->mfn);
+        pg = !is_iomem_page(act->mfn) ? mfn_to_page(act->mfn) : NULL;
  
          if ( map->flags & GNTMAP_readonly )
          {
@@ -3786,7 +3786,7 @@ int gnttab_release_mappings(struct domai
              {
                  BUG_ON(!(act->pin & GNTPIN_devr_mask));
                  act->pin -= GNTPIN_devr_inc;
-                if ( !is_iomem_page(act->mfn) )
+                if ( pg )
                      put_page(pg);
              }
  
@@ -3794,8 +3794,7 @@ int gnttab_release_mappings(struct domai
              {
                  BUG_ON(!(act->pin & GNTPIN_hstr_mask));
                  act->pin -= GNTPIN_hstr_inc;
-                if ( gnttab_release_host_mappings(d) &&
-                     !is_iomem_page(act->mfn) )
+                if ( pg && gnttab_release_host_mappings(d) )
                      put_page(pg);
              }
          }
@@ -3805,7 +3804,7 @@ int gnttab_release_mappings(struct domai
              {
                  BUG_ON(!(act->pin & GNTPIN_devw_mask));
                  act->pin -= GNTPIN_devw_inc;
-                if ( !is_iomem_page(act->mfn) )
+                if ( pg )
                      put_page_and_type(pg);
              }
  
@@ -3813,8 +3812,7 @@ int gnttab_release_mappings(struct domai
              {
                  BUG_ON(!(act->pin & GNTPIN_hstw_mask));
                  act->pin -= GNTPIN_hstw_inc;
-                if ( gnttab_release_host_mappings(d) &&
-                     !is_iomem_page(act->mfn) )
+                if ( pg && gnttab_release_host_mappings(d) )
                  {
                      if ( gnttab_host_mapping_get_page_type(false, d, rd) )
                          put_page_type(pg);


--
Julien Grall