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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Sep 12, 2018 at 12:30:27PM +0100, Paul Durrant wrote:
> ...for some uses of get_page_from_gfn().
> 
> There are many occurences 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>;
> 
>     if ( !p2m_is_ram(p2mt) ||
>          (!<readonly look-up> && p2m_is_readonly(p2mt)) )
>     {
>         put_page(page);
>         return <-EINVAL or equivalent>;
>     }
> 
> There are some small differences between the exact way the occurences
> 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.
> 
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Just two nits below.

> ---
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> 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 | 34 ++++++++--------------------
>  xen/arch/x86/hvm/hvm.c     | 16 ++-----------
>  xen/common/grant_table.c   | 39 ++++++++++----------------------
>  xen/common/memory.c        | 56 
> +++++++++++++++++++++++++++++++++++++---------
>  xen/include/asm-arm/p2m.h  |  8 +++++++
>  xen/include/asm-x86/p2m.h  |  2 ++
>  6 files changed, 79 insertions(+), 76 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index a577685dc6..0135272bf4 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -353,33 +353,17 @@ static int hvmemul_do_io_buffer(
>  
>  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) )
> -    {
> -        put_page(*page);
> -        p2m_mem_paging_populate(curr_d, gmfn);
> -        return X86EMUL_RETRY;
> -    }
> -
> -    if ( p2m_is_shared(p2mt) )
> +    switch ( check_get_page_from_gfn(current->domain, _gfn(gmfn), false,
> +                                     NULL, page) )
>      {
> -        put_page(*page);
> +    case 0:
> +        break;
> +    case -EAGAIN:
>          return X86EMUL_RETRY;

I would add a newline between cases.

> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index d4b3cfcb6e..03897b49c3 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -492,6 +492,8 @@ static inline struct page_info *get_page_from_gfn(
>      return mfn_valid(_mfn(gfn)) && get_page(page, d) ? page : NULL;
>  }
>  
> +int check_get_page_from_gfn(struct domain *d, gfn_t gfn, bool readonly,
> +                            p2m_type_t *p2mt_p, struct page_info **page_p);

Given the usage above, I would add the must_check attribute.

Thanks, Roger.

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