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

2017-08-25 Thread 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: [Xen-devel] [PATCH 2/3] gnttab: Drop the frame parameter from get_paged_frame()

2017-08-25 Thread 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 
> Reviewed-by: Wei Liu 

Thanks.

>
>> ---
>> CC: George Dunlap 
>> CC: Jan Beulich 
>> CC: Konrad Rzeszutek Wilk 
>> CC: Stefano Stabellini 
>> CC: Tim Deegan 
>> CC: Wei Liu 
>> ---
>>  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(>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: [Xen-devel] [PATCH 2/3] gnttab: Drop the frame parameter from get_paged_frame()

2017-08-25 Thread 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 

Reviewed-by: Wei Liu 

> ---
> CC: George Dunlap 
> CC: Jan Beulich 
> CC: Konrad Rzeszutek Wilk 
> CC: Stefano Stabellini 
> CC: Tim Deegan 
> CC: Wei Liu 
> ---
>  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(>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

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

2017-08-24 Thread Andrew Cooper 
It is redundant with the *page parameter.

Signed-off-by: Andrew Cooper 
---
CC: George Dunlap 
CC: Jan Beulich 
CC: Konrad Rzeszutek Wilk 
CC: Stefano Stabellini 
CC: Tim Deegan 
CC: Wei Liu 
---
 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(>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;
 intrc = GNTST_okay;
 u32old_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, , ,
- op->flags & GNTMAP_readonly, rd);
+rc = get_paged_frame(gfn, , 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