Re: [Xen-devel] [PATCH v14 5/9] memory: add check_get_page_from_gfn() as a wrapper...

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 10/04/2018 11:45 AM, Paul Durrant wrote:
> ...for some uses of get_page_from_gfn().
> 
> There are many occurrences of the following pattern in the code:
> 
>     q = <readonly look-up> ? P2M_ALLOC : P2M_UNSHARE;
>     page = get_page_from_gfn(d, gfn, &p2mt, q);
> 
>     if ( p2m_is_paging(p2mt) )
>     {
>         if ( page )
>             put_page(page);
> 
>         p2m_mem_paging_populate(d, gfn);
>         return <-EAGAIN or equivalent>;
>     }
> 
>     if ( (q & P2M_UNSHARE) && p2m_is_shared(p2mt) )
>     {
>         if ( page )
>             put_page(page);
> 
>         return <-EAGAIN or equivalent>;
>     }
> 
>     if ( !page )
>         return <-EINVAL or equivalent>;
> 
> There are some small differences between the exact way the occurrences
> are coded but the desired semantic is the same.
> 
> This patch introduces a new common implementation of this code in
> check_get_page_from_gfn() and then converts the various open-coded patterns
> into calls to this new function.
> 
> NOTE: A forward declaration of p2m_type_t enum has been introduced in
>       p2m-common.h so that it is possible to declare
>       check_get_page_from_gfn() there rather than having to add
>       duplicate declarations in the per-architecture p2m headers.
> 
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> Reviewed-by: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> Acked-by: Julien Grall <julien.grall@xxxxxxx>

Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx>

> ---
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> v13:
>  - Expanded commit comment.
> 
> v11:
>  - Forward declare p2m_type_t in p2m-common.h to allow the duplicate
>    declarations of check_get_page_from_gfn() to be removed, and hence add
>    Jan's R-b.
> 
> v10:
>  - Expand commit comment to point out the reason for the duplicate
>    declarations of check_get_page_from_gfn().
> 
> v9:
>  - Defer P2M type checks (beyond shared or paging) to the caller.
> 
> v7:
>  - Fix ARM build by introducing p2m_is_readonly() predicate.
>  - Re-name get_paged_frame() -> check_get_page_from_gfn().
>  - Adjust default cases of callers switch-ing on return value.
> 
> v3:
>  - Addressed comments from George.
> 
> v2:
>  - New in v2.
> ---
>  xen/arch/x86/hvm/emulate.c   | 25 +++++++++++-----------
>  xen/arch/x86/hvm/hvm.c       | 14 +------------
>  xen/common/grant_table.c     | 32 ++++++++++++++---------------
>  xen/common/memory.c          | 49 
> +++++++++++++++++++++++++++++++++++---------
>  xen/include/asm-arm/p2m.h    |  4 ++--
>  xen/include/asm-x86/p2m.h    |  5 ++---
>  xen/include/xen/p2m-common.h |  6 ++++++
>  7 files changed, 77 insertions(+), 58 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index eab66eab77..cd1d9a7c57 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -356,22 +356,21 @@ static int hvmemul_acquire_page(unsigned long gmfn, 
> struct page_info **page)
>      struct domain *curr_d = current->domain;
>      p2m_type_t p2mt;
>  
> -    *page = get_page_from_gfn(curr_d, gmfn, &p2mt, P2M_UNSHARE);
> -
> -    if ( *page == NULL )
> -        return X86EMUL_UNHANDLEABLE;
> -
> -    if ( p2m_is_paging(p2mt) )
> +    switch ( check_get_page_from_gfn(curr_d, _gfn(gmfn), false, &p2mt,
> +                                     page) )
>      {
> -        put_page(*page);
> -        p2m_mem_paging_populate(curr_d, gmfn);
> -        return X86EMUL_RETRY;
> -    }
> +    case 0:
> +        break;
>  
> -    if ( p2m_is_shared(p2mt) )
> -    {
> -        put_page(*page);
> +    case -EAGAIN:
>          return X86EMUL_RETRY;
> +
> +    default:
> +        ASSERT_UNREACHABLE();
> +        /* Fallthrough */
> +
> +    case -EINVAL:
> +        return X86EMUL_UNHANDLEABLE;
>      }
>  
>      /* This code should not be reached if the gmfn is not RAM */
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 51fc3ec07f..fa994a36a4 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2536,20 +2536,8 @@ static void *_hvm_map_guest_frame(unsigned long gfn, 
> bool_t permanent,
>      struct page_info *page;
>      struct domain *d = current->domain;
>  
> -    page = get_page_from_gfn(d, gfn, &p2mt,
> -                             writable ? P2M_UNSHARE : P2M_ALLOC);
> -    if ( (p2m_is_shared(p2mt) && writable) || !page )
> -    {
> -        if ( page )
> -            put_page(page);
> -        return NULL;
> -    }
> -    if ( p2m_is_paging(p2mt) )
> -    {
> -        put_page(page);
> -        p2m_mem_paging_populate(d, gfn);
> +    if ( check_get_page_from_gfn(d, _gfn(gfn), !writable, &p2mt, &page) )
>          return NULL;
> -    }
>  
>      if ( writable )
>      {
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 0f0b7b1a49..3604a8812c 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -374,25 +374,23 @@ static int get_paged_frame(unsigned long gfn, mfn_t 
> *mfn,
>                             struct page_info **page, bool readonly,
>                             struct domain *rd)
>  {
> -    int rc = GNTST_okay;
>      p2m_type_t p2mt;
> +    int rc;
>  
> -    *mfn = INVALID_MFN;
> -    *page = get_page_from_gfn(rd, gfn, &p2mt,
> -                              readonly ? P2M_ALLOC : P2M_UNSHARE);
> -    if ( !*page )
> +    rc = check_get_page_from_gfn(rd, _gfn(gfn), readonly, &p2mt, page);
> +    switch ( rc )
>      {
> -#ifdef P2M_SHARED_TYPES
> -        if ( p2m_is_shared(p2mt) )
> -            return GNTST_eagain;
> -#endif
> -#ifdef P2M_PAGES_TYPES
> -        if ( p2m_is_paging(p2mt) )
> -        {
> -            p2m_mem_paging_populate(rd, gfn);
> -            return GNTST_eagain;
> -        }
> -#endif
> +    case 0:
> +        break;
> +
> +    case -EAGAIN:
> +        return GNTST_eagain;
> +
> +    default:
> +        ASSERT_UNREACHABLE();
> +        /* Fallthrough */
> +
> +    case -EINVAL:
>          return GNTST_bad_page;
>      }
>  
> @@ -406,7 +404,7 @@ static int get_paged_frame(unsigned long gfn, mfn_t *mfn,
>  
>      *mfn = page_to_mfn(*page);
>  
> -    return rc;
> +    return GNTST_okay;
>  }
>  
>  static inline void
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index b567bd46bb..52d55dfed7 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1626,37 +1626,66 @@ void destroy_ring_for_helper(
>      }
>  }
>  
> -int prepare_ring_for_helper(
> -    struct domain *d, unsigned long gmfn, struct page_info **_page,
> -    void **_va)
> +/*
> + * Acquire a pointer to struct page_info for a specified doman and GFN,
> + * checking whether the page has been paged out, or needs unsharing.
> + * If the function succeeds then zero is returned, page_p is written
> + * with a pointer to the struct page_info with a reference taken, and
> + * p2mt_p it is written with the P2M type of the page. The caller is
> + * responsible for dropping the reference.
> + * If the function fails then an appropriate errno is returned and the
> + * values referenced by page_p and p2mt_p are undefined.
> + */
> +int check_get_page_from_gfn(struct domain *d, gfn_t gfn, bool readonly,
> +                            p2m_type_t *p2mt_p, struct page_info **page_p)
>  {
> -    struct page_info *page;
> +    p2m_query_t q = readonly ? P2M_ALLOC : P2M_UNSHARE;
>      p2m_type_t p2mt;
> -    void *va;
> +    struct page_info *page;
>  
> -    page = get_page_from_gfn(d, gmfn, &p2mt, P2M_UNSHARE);
> +    page = get_page_from_gfn(d, gfn_x(gfn), &p2mt, q);
>  
>  #ifdef CONFIG_HAS_MEM_PAGING
>      if ( p2m_is_paging(p2mt) )
>      {
>          if ( page )
>              put_page(page);
> -        p2m_mem_paging_populate(d, gmfn);
> -        return -ENOENT;
> +
> +        p2m_mem_paging_populate(d, gfn_x(gfn));
> +        return -EAGAIN;
>      }
>  #endif
>  #ifdef CONFIG_HAS_MEM_SHARING
> -    if ( p2m_is_shared(p2mt) )
> +    if ( (q & P2M_UNSHARE) && p2m_is_shared(p2mt) )
>      {
>          if ( page )
>              put_page(page);
> -        return -ENOENT;
> +
> +        return -EAGAIN;
>      }
>  #endif
>  
>      if ( !page )
>          return -EINVAL;
>  
> +    *p2mt_p = p2mt;
> +    *page_p = page;
> +    return 0;
> +}
> +
> +int prepare_ring_for_helper(
> +    struct domain *d, unsigned long gmfn, struct page_info **_page,
> +    void **_va)
> +{
> +    p2m_type_t p2mt;
> +    struct page_info *page;
> +    void *va;
> +    int rc;
> +
> +    rc = check_get_page_from_gfn(d, _gfn(gmfn), false, &p2mt, &page);
> +    if ( rc )
> +        return (rc == -EAGAIN) ? -ENOENT : rc;
> +
>      if ( !get_page_type(page, PGT_writable_page) )
>      {
>          put_page(page);
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 8823707c17..c03557544a 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -110,7 +110,7 @@ struct p2m_domain {
>   * future, it's possible to use higher value for pseudo-type and don't store
>   * them in the p2m entry.
>   */
> -typedef enum {
> +enum p2m_type {
>      p2m_invalid = 0,    /* Nothing mapped here */
>      p2m_ram_rw,         /* Normal read/write guest RAM */
>      p2m_ram_ro,         /* Read-only; writes are silently dropped */
> @@ -124,7 +124,7 @@ typedef enum {
>      p2m_iommu_map_rw,   /* Read/write iommu mapping */
>      p2m_iommu_map_ro,   /* Read-only iommu mapping */
>      p2m_max_real_type,  /* Types after this won't be store in the p2m */
> -} p2m_type_t;
> +};
>  
>  /* We use bitmaps and mask to handle groups of types */
>  #define p2m_to_mask(_t) (1UL << (_t))
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index be3b6fcaf0..d08c595887 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -52,7 +52,7 @@ extern bool_t opt_hap_1gb, opt_hap_2mb;
>   * cannot be non-zero, otherwise, hardware generates io page faults when 
>   * device access those pages. Therefore, p2m_ram_rw has to be defined as 0.
>   */
> -typedef enum {
> +enum p2m_type {
>      p2m_ram_rw = 0,             /* Normal read/write guest RAM */
>      p2m_invalid = 1,            /* Nothing mapped here */
>      p2m_ram_logdirty = 2,       /* Temporarily read-only for log-dirty */
> @@ -72,7 +72,7 @@ typedef enum {
>      p2m_ram_broken = 13,          /* Broken page, access cause domain crash 
> */
>      p2m_map_foreign  = 14,        /* ram pages from foreign domain */
>      p2m_ioreq_server = 15,
> -} p2m_type_t;
> +};
>  
>  /* Modifiers to the query */
>  typedef unsigned int p2m_query_t;
> @@ -503,7 +503,6 @@ static inline struct page_info *get_page_from_gfn(
>      return mfn_valid(_mfn(gfn)) && get_page(page, d) ? page : NULL;
>  }
>  
> -
>  /* General conversion function from mfn to gfn */
>  static inline unsigned long mfn_to_gfn(struct domain *d, mfn_t mfn)
>  {
> diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
> index 74311950ad..f4d30efe5f 100644
> --- a/xen/include/xen/p2m-common.h
> +++ b/xen/include/xen/p2m-common.h
> @@ -32,5 +32,11 @@ unsigned long
>  p2m_pod_decrease_reservation(struct domain *d, gfn_t gfn,
>                               unsigned int order);
>  
> +typedef enum p2m_type p2m_type_t;
> +
> +int __must_check check_get_page_from_gfn(struct domain *d, gfn_t gfn,
> +                                         bool readonly, p2m_type_t *p2mt_p,
> +                                         struct page_info **page_p);
> +
>  
>  #endif /* _XEN_P2M_COMMON_H */
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel