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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 13 September 2018 09:30
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Konrad Rzeszutek
> Wilk <konrad.wilk@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>; Julien Grall <julien.grall@xxxxxxx>; Jan Beulich
> <jbeulich@xxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v7 5/6] memory: add
> check_get_page_from_gfn() as a wrapper...
> 
> 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>
> 

Thanks.

> 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.

Ok.

> 
> > 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.
> 

Yes, that sounds like a good thing to do.

  Paul

> Thanks, Roger.

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