Re: [Xen-devel] [PATCH] x86/mm: drop redundant domain parameter from get_page_from_gfn_p2m()

[George Dunlap]

On 21/06/17 11:12, Jan Beulich wrote:
> It can always be read from the passed p2m. Take the opportunity and
> also rename the function, making the "p2m" suffix a prefix, to match
> other p2m functions, and convert the "gfn" parameter's type.
> 
> Signed-off-by: Jan Beulich 

Looks good, thanks:

Reviewed-by: George Dunlap 



> 
> --- a/xen/arch/x86/mm/hap/guest_walk.c
> +++ b/xen/arch/x86/mm/hap/guest_walk.c
> @@ -55,13 +55,13 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PA
>  void *top_map;
>  p2m_type_t p2mt;
>  walk_t gw;
> -unsigned long top_gfn;
> +gfn_t top_gfn;
>  struct page_info *top_page;
>  
>  /* Get the top-level table's MFN */
> -top_gfn = cr3 >> PAGE_SHIFT;
> -top_page = get_page_from_gfn_p2m(p2m->domain, p2m, top_gfn,
> - , NULL, P2M_ALLOC | P2M_UNSHARE);
> +top_gfn = _gfn(cr3 >> PAGE_SHIFT);
> +top_page = p2m_get_page_from_gfn(p2m, top_gfn, , NULL,
> + P2M_ALLOC | P2M_UNSHARE);
>  if ( p2m_is_paging(p2mt) )
>  {
>  ASSERT(p2m_is_hostp2m(p2m));
> @@ -100,8 +100,9 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PA
>  {
>  gfn_t gfn = guest_walk_to_gfn();
>  struct page_info *page;
> -page = get_page_from_gfn_p2m(p2m->domain, p2m, gfn_x(gfn), ,
> - NULL, P2M_ALLOC | P2M_UNSHARE);
> +
> +page = p2m_get_page_from_gfn(p2m, gfn, , NULL,
> + P2M_ALLOC | P2M_UNSHARE);
>  if ( page )
>  put_page(page);
>  if ( p2m_is_paging(p2mt) )
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -472,8 +472,8 @@ void __put_gfn(struct p2m_domain *p2m, u
>  }
>  
>  /* Atomically look up a GFN and take a reference count on the backing page. 
> */
> -struct page_info *get_page_from_gfn_p2m(
> -struct domain *d, struct p2m_domain *p2m, unsigned long gfn,
> +struct page_info *p2m_get_page_from_gfn(
> +struct p2m_domain *p2m, gfn_t gfn,
>  p2m_type_t *t, p2m_access_t *a, p2m_query_t q)
>  {
>  struct page_info *page = NULL;
> @@ -489,7 +489,7 @@ struct page_info *get_page_from_gfn_p2m(
>  {
>  /* Fast path: look up and get out */
>  p2m_read_lock(p2m);
> -mfn = __get_gfn_type_access(p2m, gfn, t, a, 0, NULL, 0);
> +mfn = __get_gfn_type_access(p2m, gfn_x(gfn), t, a, 0, NULL, 0);
>  if ( p2m_is_any_ram(*t) && mfn_valid(mfn)
>   && !((q & P2M_UNSHARE) && p2m_is_shared(*t)) )
>  {
> @@ -497,11 +497,12 @@ struct page_info *get_page_from_gfn_p2m(
>  if ( unlikely(p2m_is_foreign(*t)) )
>  {
>  struct domain *fdom = page_get_owner_and_reference(page);
> -ASSERT(fdom != d);
> +
> +ASSERT(fdom != p2m->domain);
>  if ( fdom == NULL )
>  page = NULL;
>  }
> -else if ( !get_page(page, d) &&
> +else if ( !get_page(page, p2m->domain) &&
>/* Page could be shared */
>(!p2m_is_shared(*t) || !get_page(page, dom_cow)) )
>  page = NULL;
> @@ -517,14 +518,14 @@ struct page_info *get_page_from_gfn_p2m(
>  }
>  
>  /* Slow path: take the write lock and do fixups */
> -mfn = get_gfn_type_access(p2m, gfn, t, a, q, NULL);
> +mfn = get_gfn_type_access(p2m, gfn_x(gfn), t, a, q, NULL);
>  if ( p2m_is_ram(*t) && mfn_valid(mfn) )
>  {
>  page = mfn_to_page(mfn);
> -if ( !get_page(page, d) )
> +if ( !get_page(page, p2m->domain) )
>  page = NULL;
>  }
> -put_gfn(d, gfn);
> +put_gfn(p2m->domain, gfn_x(gfn));
>  
>  return page;
>  }
> @@ -1900,7 +1901,7 @@ void *map_domain_gfn(struct p2m_domain *
>  }
>  
>  /* Translate the gfn, unsharing if shared. */
> -page = get_page_from_gfn_p2m(p2m->domain, p2m, gfn_x(gfn), p2mt, NULL, 
> q);
> +page = p2m_get_page_from_gfn(p2m, gfn, p2mt, NULL, q);
>  if ( p2m_is_paging(*p2mt) )
>  {
>  ASSERT(p2m_is_hostp2m(p2m));
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -465,9 +465,7 @@ static inline mfn_t get_gfn_query_unlock
>   * and should be used by any path that intends to write to the backing page.
>   * Returns NULL if the page is not backed by RAM.
>   * The caller is responsible for calling put_page() afterwards. */
> -struct page_info *get_page_from_gfn_p2m(struct domain *d,
> -struct p2m_domain *p2m,
> -unsigned long gfn,
> +struct page_info *p2m_get_page_from_gfn(struct p2m_domain *p2m, gfn_t gfn,
>  p2m_type_t *t, p2m_access_t *a,
>  p2m_query_t q);
>  
> @@ -477,7 +475,7 @@ static inline struct page_info *get_page
>  

Re: [Xen-devel] [PATCH] x86/mm: drop redundant domain parameter from get_page_from_gfn_p2m()

[Andrew Cooper]

On 21/06/17 11:12, Jan Beulich wrote:
> It can always be read from the passed p2m. Take the opportunity and
> also rename the function, making the "p2m" suffix a prefix, to match
> other p2m functions, and convert the "gfn" parameter's type.
>
> Signed-off-by: Jan Beulich 

Reviewed-by: Andrew Cooper 

One observation though.  Given its name, I'd expect this to be common
with ARM.  As it isn't, I suspect there is some API/infrastructure
de-duplication which could be worked on in due course.

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

[Xen-devel] [PATCH] x86/mm: drop redundant domain parameter from get_page_from_gfn_p2m()

[Jan Beulich]

It can always be read from the passed p2m. Take the opportunity and
also rename the function, making the "p2m" suffix a prefix, to match
other p2m functions, and convert the "gfn" parameter's type.

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/mm/hap/guest_walk.c
+++ b/xen/arch/x86/mm/hap/guest_walk.c
@@ -55,13 +55,13 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PA
 void *top_map;
 p2m_type_t p2mt;
 walk_t gw;
-unsigned long top_gfn;
+gfn_t top_gfn;
 struct page_info *top_page;
 
 /* Get the top-level table's MFN */
-top_gfn = cr3 >> PAGE_SHIFT;
-top_page = get_page_from_gfn_p2m(p2m->domain, p2m, top_gfn,
- , NULL, P2M_ALLOC | P2M_UNSHARE);
+top_gfn = _gfn(cr3 >> PAGE_SHIFT);
+top_page = p2m_get_page_from_gfn(p2m, top_gfn, , NULL,
+ P2M_ALLOC | P2M_UNSHARE);
 if ( p2m_is_paging(p2mt) )
 {
 ASSERT(p2m_is_hostp2m(p2m));
@@ -100,8 +100,9 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PA
 {
 gfn_t gfn = guest_walk_to_gfn();
 struct page_info *page;
-page = get_page_from_gfn_p2m(p2m->domain, p2m, gfn_x(gfn), ,
- NULL, P2M_ALLOC | P2M_UNSHARE);
+
+page = p2m_get_page_from_gfn(p2m, gfn, , NULL,
+ P2M_ALLOC | P2M_UNSHARE);
 if ( page )
 put_page(page);
 if ( p2m_is_paging(p2mt) )
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -472,8 +472,8 @@ void __put_gfn(struct p2m_domain *p2m, u
 }
 
 /* Atomically look up a GFN and take a reference count on the backing page. */
-struct page_info *get_page_from_gfn_p2m(
-struct domain *d, struct p2m_domain *p2m, unsigned long gfn,
+struct page_info *p2m_get_page_from_gfn(
+struct p2m_domain *p2m, gfn_t gfn,
 p2m_type_t *t, p2m_access_t *a, p2m_query_t q)
 {
 struct page_info *page = NULL;
@@ -489,7 +489,7 @@ struct page_info *get_page_from_gfn_p2m(
 {
 /* Fast path: look up and get out */
 p2m_read_lock(p2m);
-mfn = __get_gfn_type_access(p2m, gfn, t, a, 0, NULL, 0);
+mfn = __get_gfn_type_access(p2m, gfn_x(gfn), t, a, 0, NULL, 0);
 if ( p2m_is_any_ram(*t) && mfn_valid(mfn)
  && !((q & P2M_UNSHARE) && p2m_is_shared(*t)) )
 {
@@ -497,11 +497,12 @@ struct page_info *get_page_from_gfn_p2m(
 if ( unlikely(p2m_is_foreign(*t)) )
 {
 struct domain *fdom = page_get_owner_and_reference(page);
-ASSERT(fdom != d);
+
+ASSERT(fdom != p2m->domain);
 if ( fdom == NULL )
 page = NULL;
 }
-else if ( !get_page(page, d) &&
+else if ( !get_page(page, p2m->domain) &&
   /* Page could be shared */
   (!p2m_is_shared(*t) || !get_page(page, dom_cow)) )
 page = NULL;
@@ -517,14 +518,14 @@ struct page_info *get_page_from_gfn_p2m(
 }
 
 /* Slow path: take the write lock and do fixups */
-mfn = get_gfn_type_access(p2m, gfn, t, a, q, NULL);
+mfn = get_gfn_type_access(p2m, gfn_x(gfn), t, a, q, NULL);
 if ( p2m_is_ram(*t) && mfn_valid(mfn) )
 {
 page = mfn_to_page(mfn);
-if ( !get_page(page, d) )
+if ( !get_page(page, p2m->domain) )
 page = NULL;
 }
-put_gfn(d, gfn);
+put_gfn(p2m->domain, gfn_x(gfn));
 
 return page;
 }
@@ -1900,7 +1901,7 @@ void *map_domain_gfn(struct p2m_domain *
 }
 
 /* Translate the gfn, unsharing if shared. */
-page = get_page_from_gfn_p2m(p2m->domain, p2m, gfn_x(gfn), p2mt, NULL, q);
+page = p2m_get_page_from_gfn(p2m, gfn, p2mt, NULL, q);
 if ( p2m_is_paging(*p2mt) )
 {
 ASSERT(p2m_is_hostp2m(p2m));
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -465,9 +465,7 @@ static inline mfn_t get_gfn_query_unlock
  * and should be used by any path that intends to write to the backing page.
  * Returns NULL if the page is not backed by RAM.
  * The caller is responsible for calling put_page() afterwards. */
-struct page_info *get_page_from_gfn_p2m(struct domain *d,
-struct p2m_domain *p2m,
-unsigned long gfn,
+struct page_info *p2m_get_page_from_gfn(struct p2m_domain *p2m, gfn_t gfn,
 p2m_type_t *t, p2m_access_t *a,
 p2m_query_t q);
 
@@ -477,7 +475,7 @@ static inline struct page_info *get_page
 struct page_info *page;
 
 if ( paging_mode_translate(d) )
-return get_page_from_gfn_p2m(d, p2m_get_hostp2m(d), gfn, t, NULL, q);
+return p2m_get_page_from_gfn(p2m_get_hostp2m(d), _gfn(gfn), t, NULL, 
q);
 
 /* Non-translated guests see 1-1 RAM / MMIO mappings everywhere */
 if ( t )


x86/mm: drop redundant domain parameter from