[PATCH 0/3] gnttab: Further XSA-226 cleanup

[PATCH 0/3] gnttab: Further XSA-226 cleanup

[Andrew Cooper]

Andrew Cooper (3):
  gnttab: Drop the frame parameter from acquire_grant_for_copy()
  gnttab: Drop the frame parameter from get_paged_frame()
  gnttab: Drop the frame field from struct gnttab_copy_buf

 xen/common/grant_table.c | 80 ++++++++++++++++++++++--------------------------
 1 file changed, 37 insertions(+), 43 deletions(-)

-- 
2.1.4


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

[PATCH 1/3] gnttab: Drop the frame parameter from acquire_grant_for_copy()

[Andrew Cooper]

It is redundant with the *page parameter.  Rename the grant_frame parameter to
indicate that it is local, and highlight the correctness of the change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/common/grant_table.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 36895aa..188c477 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2142,15 +2142,17 @@ static void fixup_status_for_copy_pin(const struct active_grant_entry *act,
         gnttab_clear_flag(_GTF_reading, status);
 }
 
-/* Grab a frame number from a grant entry and update the flags and pin
-   count as appropriate. If rc == GNTST_okay, note that this *does*
-   take one ref count on the target page, stored in *page.
-   If there is any error, *page = NULL, no ref taken. */
+/*
+ * Grab a frame number from a grant entry and update the flags and pin
+ * count as appropriate. If rc == GNTST_okay, note that this *does*
+ * take one ref count on the target page, stored in *page.
+ * If there is any error, *page = NULL, no ref taken.
+ */
 static int
 acquire_grant_for_copy(
     struct domain *rd, grant_ref_t gref, domid_t ldom, bool readonly,
-    unsigned long *frame, struct page_info **page,
-    uint16_t *page_off, uint16_t *length, bool allow_transitive)
+    struct page_info **page, uint16_t *page_off, uint16_t *length,
+    bool allow_transitive)
 {
     struct grant_table *rgt = rd->grant_table;
     grant_entry_v2_t *sha2;
@@ -2161,7 +2163,7 @@ acquire_grant_for_copy(
     domid_t trans_domid;
     grant_ref_t trans_gref;
     struct domain *td;
-    unsigned long grant_frame;
+    unsigned long frame;
     uint16_t trans_page_off;
     uint16_t trans_length;
     bool is_sub_page;
@@ -2238,10 +2240,9 @@ acquire_grant_for_copy(
         active_entry_release(act);
         grant_read_unlock(rgt);
 
-        rc = acquire_grant_for_copy(td, trans_gref, rd->domain_id,
-                                    readonly, &grant_frame, page,
-                                    &trans_page_off, &trans_length,
-                                    false);
+        rc = acquire_grant_for_copy(
+            td, trans_gref, rd->domain_id, readonly, page, &trans_page_off,
+            &trans_length, false);
 
         grant_read_lock(rgt);
         act = active_entry_acquire(rgt, gref);
@@ -2255,6 +2256,8 @@ acquire_grant_for_copy(
             return rc;
         }
 
+        frame = page_to_mfn(*page);
+
         /*
          * We dropped the lock, so we have to check that the grant didn't
          * change, and that nobody else tried to pin/unpin it. If anything
@@ -2262,7 +2265,7 @@ acquire_grant_for_copy(
          */
         if ( rgt->gt_version != 2 ||
              act->pin != old_pin ||
-             (old_pin && (act->domid != ldom || act->frame != grant_frame ||
+             (old_pin && (act->domid != ldom || act->frame != frame ||
                           act->start != trans_page_off ||
                           act->length != trans_length ||
                           act->trans_domain != td ||
@@ -2286,7 +2289,7 @@ acquire_grant_for_copy(
             act->length = trans_length;
             act->trans_domain = td;
             act->trans_gref = trans_gref;
-            act->frame = grant_frame;
+            act->frame = frame;
             act_set_gfn(act, INVALID_GFN);
             /*
              * The actual remote remote grant may or may not be a sub-page,
@@ -2310,7 +2313,7 @@ acquire_grant_for_copy(
         {
             unsigned long gfn = shared_entry_v1(rgt, gref).frame;
 
-            rc = get_paged_frame(gfn, &grant_frame, page, readonly, rd);
+            rc = get_paged_frame(gfn, &frame, page, readonly, rd);
             if ( rc != GNTST_okay )
                 goto unlock_out_clear;
             act_set_gfn(act, _gfn(gfn));
@@ -2320,7 +2323,7 @@ acquire_grant_for_copy(
         }
         else if ( !(sha2->hdr.flags & GTF_sub_page) )
         {
-            rc = get_paged_frame(sha2->full_page.frame, &grant_frame, page,
+            rc = get_paged_frame(sha2->full_page.frame, &frame, page,
                                  readonly, rd);
             if ( rc != GNTST_okay )
                 goto unlock_out_clear;
@@ -2331,7 +2334,7 @@ acquire_grant_for_copy(
         }
         else
         {
-            rc = get_paged_frame(sha2->sub_page.frame, &grant_frame, page,
+            rc = get_paged_frame(sha2->sub_page.frame, &frame, page,
                                  readonly, rd);
             if ( rc != GNTST_okay )
                 goto unlock_out_clear;
@@ -2349,7 +2352,7 @@ acquire_grant_for_copy(
             act->length = trans_length;
             act->trans_domain = td;
             act->trans_gref = trans_gref;
-            act->frame = grant_frame;
+            act->frame = frame;
         }
     }
     else
@@ -2368,7 +2371,6 @@ acquire_grant_for_copy(
 
     *page_off = act->start;
     *length = act->length;
-    *frame = act->frame;
 
     active_entry_release(act);
     grant_read_unlock(rgt);
@@ -2503,11 +2505,11 @@ static int gnttab_copy_claim_buf(const struct gnttab_copy *op,
     {
         rc = acquire_grant_for_copy(buf->domain, ptr->u.ref,
                                     current->domain->domain_id,
-                                    buf->read_only,
-                                    &buf->frame, &buf->page,
+                                    buf->read_only, &buf->page,
                                     &buf->ptr.offset, &buf->len, true);
         if ( rc != GNTST_okay )
             goto out;
+        buf->frame = page_to_mfn(buf->page);
         buf->ptr.u.ref = ptr->u.ref;
         buf->have_grant = 1;
     }
-- 
2.1.4


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

[PATCH 2/3] gnttab: Drop the frame parameter from get_paged_frame()

[Andrew Cooper]

It is redundant with the *page parameter.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/common/grant_table.c | 50 +++++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 188c477..d8307b7 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -257,13 +257,13 @@ static inline void active_entry_release(struct active_grant_entry *act)
     spin_unlock(&act->lock);
 }
 
-/* Check if the page has been paged out, or needs unsharing.
-   If rc == GNTST_okay, *page contains the page struct with a ref taken.
-   Caller must do put_page(*page).
-   If any error, *page = NULL, *frame = INVALID_MFN, no ref taken. */
-static int get_paged_frame(unsigned long gfn, unsigned long *frame,
-                           struct page_info **page, bool readonly,
-                           struct domain *rd)
+/*
+ * Check if the page has been paged out, or needs unsharing.
+ * If rc == GNTST_okay, *page contains the page struct with a ref taken.
+ * Caller must do put_page(*page). If any error, *page = NULL, no ref taken.
+ */
+static int get_paged_frame(unsigned long gfn, struct page_info **page,
+                           bool readonly, struct domain *rd)
 {
     int rc = GNTST_okay;
 #if defined(P2M_PAGED_TYPES) || defined(P2M_SHARED_TYPES)
@@ -273,7 +273,6 @@ static int get_paged_frame(unsigned long gfn, unsigned long *frame,
                               (readonly) ? P2M_ALLOC : P2M_UNSHARE);
     if ( !(*page) )
     {
-        *frame = mfn_x(INVALID_MFN);
         if ( p2m_is_shared(p2mt) )
             return GNTST_eagain;
         if ( p2m_is_paging(p2mt) )
@@ -283,13 +282,12 @@ static int get_paged_frame(unsigned long gfn, unsigned long *frame,
         }
         return GNTST_bad_page;
     }
-    *frame = page_to_mfn(*page);
 #else
-    *frame = mfn_x(gfn_to_mfn(rd, _gfn(gfn)));
-    *page = mfn_valid(_mfn(*frame)) ? mfn_to_page(*frame) : NULL;
+    mfn_t mfn = gfn_to_mfn(rd, _gfn(gfn));
+
+    *page = mfn_valid(mfn) ? mfn_to_page(mfn_x(mfn)) : NULL;
     if ( (!(*page)) || (!get_page(*page, rd)) )
     {
-        *frame = mfn_x(INVALID_MFN);
         *page = NULL;
         rc = GNTST_bad_page;
     }
@@ -804,7 +802,7 @@ map_grant_ref(
     struct grant_table *lgt, *rgt;
     struct vcpu   *led;
     grant_handle_t handle;
-    unsigned long  frame = 0;
+    unsigned long  frame;
     struct page_info *pg = NULL;
     int            rc = GNTST_okay;
     u32            old_pin;
@@ -896,13 +894,12 @@ map_grant_ref(
                                 shared_entry_v1(rgt, op->ref).frame :
                                 shared_entry_v2(rgt, op->ref).full_page.frame;
 
-            rc = get_paged_frame(gfn, &frame, &pg,
-                                 op->flags & GNTMAP_readonly, rd);
+            rc = get_paged_frame(gfn, &pg, op->flags & GNTMAP_readonly, rd);
             if ( rc != GNTST_okay )
                 goto unlock_out_clear;
             act_set_gfn(act, _gfn(gfn));
             act->domid = ld->domain_id;
-            act->frame = frame;
+            act->frame = page_to_mfn(pg);
             act->start = 0;
             act->length = PAGE_SIZE;
             act->is_sub_page = false;
@@ -2163,7 +2160,6 @@ acquire_grant_for_copy(
     domid_t trans_domid;
     grant_ref_t trans_gref;
     struct domain *td;
-    unsigned long frame;
     uint16_t trans_page_off;
     uint16_t trans_length;
     bool is_sub_page;
@@ -2256,8 +2252,6 @@ acquire_grant_for_copy(
             return rc;
         }
 
-        frame = page_to_mfn(*page);
-
         /*
          * We dropped the lock, so we have to check that the grant didn't
          * change, and that nobody else tried to pin/unpin it. If anything
@@ -2265,7 +2259,8 @@ acquire_grant_for_copy(
          */
         if ( rgt->gt_version != 2 ||
              act->pin != old_pin ||
-             (old_pin && (act->domid != ldom || act->frame != frame ||
+             (old_pin && (act->domid != ldom ||
+                          act->frame != page_to_mfn(*page) ||
                           act->start != trans_page_off ||
                           act->length != trans_length ||
                           act->trans_domain != td ||
@@ -2289,7 +2284,7 @@ acquire_grant_for_copy(
             act->length = trans_length;
             act->trans_domain = td;
             act->trans_gref = trans_gref;
-            act->frame = frame;
+            act->frame = page_to_mfn(*page);
             act_set_gfn(act, INVALID_GFN);
             /*
              * The actual remote remote grant may or may not be a sub-page,
@@ -2313,7 +2308,7 @@ acquire_grant_for_copy(
         {
             unsigned long gfn = shared_entry_v1(rgt, gref).frame;
 
-            rc = get_paged_frame(gfn, &frame, page, readonly, rd);
+            rc = get_paged_frame(gfn, page, readonly, rd);
             if ( rc != GNTST_okay )
                 goto unlock_out_clear;
             act_set_gfn(act, _gfn(gfn));
@@ -2323,8 +2318,7 @@ acquire_grant_for_copy(
         }
         else if ( !(sha2->hdr.flags & GTF_sub_page) )
         {
-            rc = get_paged_frame(sha2->full_page.frame, &frame, page,
-                                 readonly, rd);
+            rc = get_paged_frame(sha2->full_page.frame, page, readonly, rd);
             if ( rc != GNTST_okay )
                 goto unlock_out_clear;
             act_set_gfn(act, _gfn(sha2->full_page.frame));
@@ -2334,8 +2328,7 @@ acquire_grant_for_copy(
         }
         else
         {
-            rc = get_paged_frame(sha2->sub_page.frame, &frame, page,
-                                 readonly, rd);
+            rc = get_paged_frame(sha2->sub_page.frame, page, readonly, rd);
             if ( rc != GNTST_okay )
                 goto unlock_out_clear;
             act_set_gfn(act, _gfn(sha2->sub_page.frame));
@@ -2352,7 +2345,7 @@ acquire_grant_for_copy(
             act->length = trans_length;
             act->trans_domain = td;
             act->trans_gref = trans_gref;
-            act->frame = frame;
+            act->frame = page_to_mfn(*page);
         }
     }
     else
@@ -2515,12 +2508,13 @@ static int gnttab_copy_claim_buf(const struct gnttab_copy *op,
     }
     else
     {
-        rc = get_paged_frame(ptr->u.gmfn, &buf->frame, &buf->page,
+        rc = get_paged_frame(ptr->u.gmfn, &buf->page,
                              buf->read_only, buf->domain);
         if ( rc != GNTST_okay )
             PIN_FAIL(out, rc,
                      "source frame %"PRI_xen_pfn" invalid.\n", ptr->u.gmfn);
 
+        buf->frame = page_to_mfn(buf->page);
         buf->ptr.u.gmfn = ptr->u.gmfn;
         buf->ptr.offset = 0;
         buf->len = PAGE_SIZE;
-- 
2.1.4


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

[PATCH 3/3] gnttab: Drop the frame field from struct gnttab_copy_buf

[Andrew Cooper]

It is redundant with *page.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/common/grant_table.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index d8307b7..bef3b26 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2393,7 +2393,6 @@ struct gnttab_copy_buf {
 
     /* Mapped etc. */
     struct domain *domain;
-    unsigned long frame;
     struct page_info *page;
     void *virt;
     bool_t read_only;
@@ -2502,7 +2501,6 @@ static int gnttab_copy_claim_buf(const struct gnttab_copy *op,
                                     &buf->ptr.offset, &buf->len, true);
         if ( rc != GNTST_okay )
             goto out;
-        buf->frame = page_to_mfn(buf->page);
         buf->ptr.u.ref = ptr->u.ref;
         buf->have_grant = 1;
     }
@@ -2514,7 +2512,6 @@ static int gnttab_copy_claim_buf(const struct gnttab_copy *op,
             PIN_FAIL(out, rc,
                      "source frame %"PRI_xen_pfn" invalid.\n", ptr->u.gmfn);
 
-        buf->frame = page_to_mfn(buf->page);
         buf->ptr.u.gmfn = ptr->u.gmfn;
         buf->ptr.offset = 0;
         buf->len = PAGE_SIZE;
@@ -2525,14 +2522,15 @@ static int gnttab_copy_claim_buf(const struct gnttab_copy *op,
         if ( !get_page_type(buf->page, PGT_writable_page) )
         {
             if ( !buf->domain->is_dying )
-                gdprintk(XENLOG_WARNING, "Could not get writable frame %lx\n", buf->frame);
+                gdprintk(XENLOG_WARNING, "Could not get writable mfn %"PRI_mfn"\n",
+                         page_to_mfn(buf->page));
             rc = GNTST_general_error;
             goto out;
         }
         buf->have_type = 1;
     }
 
-    buf->virt = map_domain_page(_mfn(buf->frame));
+    buf->virt = __map_domain_page(buf->page);
     rc = GNTST_okay;
 
  out:
@@ -2576,7 +2574,7 @@ static int gnttab_copy_buf(const struct gnttab_copy *op,
 
     memcpy(dest->virt + op->dest.offset, src->virt + op->source.offset,
            op->len);
-    gnttab_mark_dirty(dest->domain, dest->frame);
+    gnttab_mark_dirty(dest->domain, page_to_mfn(dest->page));
     rc = GNTST_okay;
  out:
     return rc;
-- 
2.1.4


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

Re: [PATCH 1/3] gnttab: Drop the frame parameter from acquire_grant_for_copy()

[Wei Liu]

On Thu, Aug 24, 2017 at 06:55:53PM +0100, Andrew Cooper wrote:
> It is redundant with the *page parameter.  Rename the grant_frame parameter to
> indicate that it is local, and highlight the correctness of the change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

>  
> -        rc = acquire_grant_for_copy(td, trans_gref, rd->domain_id,
> -                                    readonly, &grant_frame, page,
> -                                    &trans_page_off, &trans_length,
> -                                    false);
> +        rc = acquire_grant_for_copy(
> +            td, trans_gref, rd->domain_id, readonly, page, &trans_page_off,
> +            &trans_length, false);

The change of style seems unnecessary, but I don't think I care that
much.

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

Re: [PATCH 2/3] gnttab: Drop the frame parameter from get_paged_frame()

[Wei Liu]

On Thu, Aug 24, 2017 at 06:55:54PM +0100, Andrew Cooper wrote:
> It is redundant with the *page parameter.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Tim Deegan <tim@xen.org>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/common/grant_table.c | 50 +++++++++++++++++++++---------------------------
>  1 file changed, 22 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 188c477..d8307b7 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -257,13 +257,13 @@ static inline void active_entry_release(struct active_grant_entry *act)
>      spin_unlock(&act->lock);
>  }
>  
> -/* Check if the page has been paged out, or needs unsharing.
> -   If rc == GNTST_okay, *page contains the page struct with a ref taken.
> -   Caller must do put_page(*page).
> -   If any error, *page = NULL, *frame = INVALID_MFN, no ref taken. */
> -static int get_paged_frame(unsigned long gfn, unsigned long *frame,
> -                           struct page_info **page, bool readonly,
> -                           struct domain *rd)
> +/*
> + * Check if the page has been paged out, or needs unsharing.
> + * If rc == GNTST_okay, *page contains the page struct with a ref taken.
> + * Caller must do put_page(*page). If any error, *page = NULL, no ref taken.
> + */
> +static int get_paged_frame(unsigned long gfn, struct page_info **page,
> +                           bool readonly, struct domain *rd)
>  {
>      int rc = GNTST_okay;
>  #if defined(P2M_PAGED_TYPES) || defined(P2M_SHARED_TYPES)
> @@ -273,7 +273,6 @@ static int get_paged_frame(unsigned long gfn, unsigned long *frame,
>                                (readonly) ? P2M_ALLOC : P2M_UNSHARE);
>      if ( !(*page) )
>      {
> -        *frame = mfn_x(INVALID_MFN);
>          if ( p2m_is_shared(p2mt) )
>              return GNTST_eagain;
>          if ( p2m_is_paging(p2mt) )
> @@ -283,13 +282,12 @@ static int get_paged_frame(unsigned long gfn, unsigned long *frame,
>          }
>          return GNTST_bad_page;
>      }
> -    *frame = page_to_mfn(*page);
>  #else
> -    *frame = mfn_x(gfn_to_mfn(rd, _gfn(gfn)));
> -    *page = mfn_valid(_mfn(*frame)) ? mfn_to_page(*frame) : NULL;
> +    mfn_t mfn = gfn_to_mfn(rd, _gfn(gfn));
> +
> +    *page = mfn_valid(mfn) ? mfn_to_page(mfn_x(mfn)) : NULL;
>      if ( (!(*page)) || (!get_page(*page, rd)) )

Mind dropping those unneeded parentheses?

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

Re: [PATCH 3/3] gnttab: Drop the frame field from struct gnttab_copy_buf

[Wei Liu]

On Thu, Aug 24, 2017 at 06:55:55PM +0100, Andrew Cooper wrote:
> It is redundant with *page.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 2/3] gnttab: Drop the frame parameter from get_paged_frame()

[Andrew Cooper]

On 25/08/17 10:02, Wei Liu wrote:
> On Thu, Aug 24, 2017 at 06:55:54PM +0100, Andrew Cooper wrote:
>> It is redundant with the *page parameter.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Thanks.

>
>> ---
>> CC: George Dunlap <George.Dunlap@eu.citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Tim Deegan <tim@xen.org>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  xen/common/grant_table.c | 50 +++++++++++++++++++++---------------------------
>>  1 file changed, 22 insertions(+), 28 deletions(-)
>>
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index 188c477..d8307b7 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -257,13 +257,13 @@ static inline void active_entry_release(struct active_grant_entry *act)
>>      spin_unlock(&act->lock);
>>  }
>>  
>> -/* Check if the page has been paged out, or needs unsharing.
>> -   If rc == GNTST_okay, *page contains the page struct with a ref taken.
>> -   Caller must do put_page(*page).
>> -   If any error, *page = NULL, *frame = INVALID_MFN, no ref taken. */
>> -static int get_paged_frame(unsigned long gfn, unsigned long *frame,
>> -                           struct page_info **page, bool readonly,
>> -                           struct domain *rd)
>> +/*
>> + * Check if the page has been paged out, or needs unsharing.
>> + * If rc == GNTST_okay, *page contains the page struct with a ref taken.
>> + * Caller must do put_page(*page). If any error, *page = NULL, no ref taken.
>> + */
>> +static int get_paged_frame(unsigned long gfn, struct page_info **page,
>> +                           bool readonly, struct domain *rd)
>>  {
>>      int rc = GNTST_okay;
>>  #if defined(P2M_PAGED_TYPES) || defined(P2M_SHARED_TYPES)
>> @@ -273,7 +273,6 @@ static int get_paged_frame(unsigned long gfn, unsigned long *frame,
>>                                (readonly) ? P2M_ALLOC : P2M_UNSHARE);
>>      if ( !(*page) )
>>      {
>> -        *frame = mfn_x(INVALID_MFN);
>>          if ( p2m_is_shared(p2mt) )
>>              return GNTST_eagain;
>>          if ( p2m_is_paging(p2mt) )
>> @@ -283,13 +282,12 @@ static int get_paged_frame(unsigned long gfn, unsigned long *frame,
>>          }
>>          return GNTST_bad_page;
>>      }
>> -    *frame = page_to_mfn(*page);
>>  #else
>> -    *frame = mfn_x(gfn_to_mfn(rd, _gfn(gfn)));
>> -    *page = mfn_valid(_mfn(*frame)) ? mfn_to_page(*frame) : NULL;
>> +    mfn_t mfn = gfn_to_mfn(rd, _gfn(gfn));
>> +
>> +    *page = mfn_valid(mfn) ? mfn_to_page(mfn_x(mfn)) : NULL;
>>      if ( (!(*page)) || (!get_page(*page, rd)) )
> Mind dropping those unneeded parentheses?

I'm planning separate cleanup to this function (including renaming it). 
I'd prefer to defer changes like this to there.

~Andrew

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

Re: [PATCH 2/3] gnttab: Drop the frame parameter from get_paged_frame()

[Wei Liu]

On Fri, Aug 25, 2017 at 10:38:19AM +0100, Andrew Cooper wrote:
> >> +    *page = mfn_valid(mfn) ? mfn_to_page(mfn_x(mfn)) : NULL;
> >>      if ( (!(*page)) || (!get_page(*page, rd)) )
> > Mind dropping those unneeded parentheses?
> 
> I'm planning separate cleanup to this function (including renaming it). 
> I'd prefer to defer changes like this to there.
> 

Sure.

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

Re: [PATCH 1/3] gnttab: Drop the frame parameter from acquire_grant_for_copy()

[Jan Beulich]

>>> On 24.08.17 at 19:55, <andrew.cooper3@citrix.com> wrote:
> It is redundant with the *page parameter.  Rename the grant_frame parameter to
> indicate that it is local, and highlight the correctness of the change.

I don't understand this second sentence: For one, grant_frame isn't
and wasn't a parameter (but a local variable), and then I also don't
see what is being highlighted to validate the correctness. Or am I
being mislead by it not reading "and to highlight ..."? So far I was
believing that such an omission is okay in German, but not in English.

> @@ -2238,10 +2240,9 @@ acquire_grant_for_copy(
>          active_entry_release(act);
>          grant_read_unlock(rgt);
>  
> -        rc = acquire_grant_for_copy(td, trans_gref, rd->domain_id,
> -                                    readonly, &grant_frame, page,
> -                                    &trans_page_off, &trans_length,
> -                                    false);
> +        rc = acquire_grant_for_copy(
> +            td, trans_gref, rd->domain_id, readonly, page, &trans_page_off,
> +            &trans_length, false);

As hinted at by Wei, I'd prefer if you didn't alter the style to the
uglier variant - we really should use that only if the commonly
used one really gets unwieldy, which imo is not the case here.

> @@ -2255,6 +2256,8 @@ acquire_grant_for_copy(
>              return rc;
>          }
>  
> +        frame = page_to_mfn(*page);
> +
>          /*
>           * We dropped the lock, so we have to check that the grant didn't
>           * change, and that nobody else tried to pin/unpin it. If anything
> @@ -2262,7 +2265,7 @@ acquire_grant_for_copy(
>           */
>          if ( rgt->gt_version != 2 ||
>               act->pin != old_pin ||
> -             (old_pin && (act->domid != ldom || act->frame != grant_frame ||
> +             (old_pin && (act->domid != ldom || act->frame != frame ||
>                            act->start != trans_page_off ||
>                            act->length != trans_length ||
>                            act->trans_domain != td ||
> @@ -2286,7 +2289,7 @@ acquire_grant_for_copy(
>              act->length = trans_length;
>              act->trans_domain = td;
>              act->trans_gref = trans_gref;
> -            act->frame = grant_frame;
> +            act->frame = frame;

These three get re-done in patch 2 - I think you could easily bring
them into their final shape right away, proving the correctness of
the change by reducing the scope of frame to ...

> @@ -2310,7 +2313,7 @@ acquire_grant_for_copy(
>          {
>              unsigned long gfn = shared_entry_v1(rgt, gref).frame;
>  
> -            rc = get_paged_frame(gfn, &grant_frame, page, readonly, rd);
> +            rc = get_paged_frame(gfn, &frame, page, readonly, rd);

... the outer if() this and the following hunks are contained in.

Jan


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

Re: [PATCH 1/3] gnttab: Drop the frame parameter from acquire_grant_for_copy()

[Andrew Cooper]

On 25/08/17 11:13, Jan Beulich wrote:
>>>> On 24.08.17 at 19:55, <andrew.cooper3@citrix.com> wrote:
>> It is redundant with the *page parameter.  Rename the grant_frame parameter to
>> indicate that it is local, and highlight the correctness of the change.
> I don't understand this second sentence: For one, grant_frame isn't
> and wasn't a parameter (but a local variable), and then I also don't
> see what is being highlighted to validate the correctness. Or am I
> being mislead by it not reading "and to highlight ..."? So far I was
> believing that such an omission is okay in German, but not in English.

Taking grant_frame out entirely at this point (is what I did originally,
but) does not make the patch read as obviously-safe.  Changing its name
causes the compiler to help spot all uses.

>
>> @@ -2238,10 +2240,9 @@ acquire_grant_for_copy(
>>          active_entry_release(act);
>>          grant_read_unlock(rgt);
>>  
>> -        rc = acquire_grant_for_copy(td, trans_gref, rd->domain_id,
>> -                                    readonly, &grant_frame, page,
>> -                                    &trans_page_off, &trans_length,
>> -                                    false);
>> +        rc = acquire_grant_for_copy(
>> +            td, trans_gref, rd->domain_id, readonly, page, &trans_page_off,
>> +            &trans_length, false);
> As hinted at by Wei, I'd prefer if you didn't alter the style to the
> uglier variant - we really should use that only if the commonly
> used one really gets unwieldy, which imo is not the case here.

Ugly? What is ugly (in general) is squashing all parameters together on
many lines on the RHS.

In this case, the false on the end also interacts with my command line
parameter patch, where it becomes opt_transitive_grants, and gets in the
way of the previous alignment strategy.

~Andrew

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

Re: [PATCH 1/3] gnttab: Drop the frame parameter from acquire_grant_for_copy()

[Jan Beulich]

>>> On 25.08.17 at 12:50, <andrew.cooper3@citrix.com> wrote:
> On 25/08/17 11:13, Jan Beulich wrote:
>>>>> On 24.08.17 at 19:55, <andrew.cooper3@citrix.com> wrote:
>>> It is redundant with the *page parameter.  Rename the grant_frame parameter to
>>> indicate that it is local, and highlight the correctness of the change.
>> I don't understand this second sentence: For one, grant_frame isn't
>> and wasn't a parameter (but a local variable), and then I also don't
>> see what is being highlighted to validate the correctness. Or am I
>> being mislead by it not reading "and to highlight ..."? So far I was
>> believing that such an omission is okay in German, but not in English.
> 
> Taking grant_frame out entirely at this point (is what I did originally,
> but) does not make the patch read as obviously-safe.  Changing its name
> causes the compiler to help spot all uses.

With the light ambiguity resulting from there having been a
function parameter of this very name.

>>> @@ -2238,10 +2240,9 @@ acquire_grant_for_copy(
>>>          active_entry_release(act);
>>>          grant_read_unlock(rgt);
>>>  
>>> -        rc = acquire_grant_for_copy(td, trans_gref, rd->domain_id,
>>> -                                    readonly, &grant_frame, page,
>>> -                                    &trans_page_off, &trans_length,
>>> -                                    false);
>>> +        rc = acquire_grant_for_copy(
>>> +            td, trans_gref, rd->domain_id, readonly, page, &trans_page_off,
>>> +            &trans_length, false);
>> As hinted at by Wei, I'd prefer if you didn't alter the style to the
>> uglier variant - we really should use that only if the commonly
>> used one really gets unwieldy, which imo is not the case here.
> 
> Ugly? What is ugly (in general) is squashing all parameters together on
> many lines on the RHS.

As always with style it's a matter of taste: To me, the indentation
of the style you switch to looks always odd, as does the line break
right after the opening parenthesis (and this would extend to
other expressions too). Of course, ./CODING_STYLE has nothing
on this at all - "Long lines should be split at sensible places and the
trailing portions indented" leaves open what "sensible" is, or how
deep indentation ought to be.

Anyway, I certainly won't block the patch over this, I just find it
odd for style to be changed here without real need.

Jan


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