Re: [PATCH v2 11/13] memory: add get_paged_gfn() as a wrapper...

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: George Dunlap [mailto:george.dunlap@citrix.com]
> Sent: 11 July 2018 12:24
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Julien
> Grall <julien.grall@arm.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim
> (Xen.org) <tim@xen.org>; Wei Liu <wei.liu2@citrix.com>
> Subject: Re: [PATCH v2 11/13] memory: add get_paged_gfn() as a wrapper...
> 
> On 07/07/2018 12:05 PM, 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 <-ENOENT or equivalent>;
> >     }
> >
> >     if ( (q & P2M_UNSHARE) && p2m_is_shared(p2mt) )
> >     {
> >         if ( page )
> >             put_page(page);
> >
> >         return <-ENOENT 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 occurrences
> are
> > coded but the desired semantic is the same.
> >
> > This patch introduces a new common implementation of this code in
> > get_paged_gfn() and then converts the various open-coded patterns into
> > calls to this new function.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> 
> This is a great idea.
> 
> It looks like this adds the restriction that the given gfn be ram (e.g.,
> not MMIO) in all cases as well.  It looks like that's what's wanted, but
> it would be good to point this out in the commit message (so people can
> verify that this change is warranted).
> 

Yes, that's what I meant by 'desired semantic' :-) I'll call out the restriction more explicitly.

> A few other comments...
> 
> > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> > index c6b99c4116..510f37f100 100644
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -375,39 +375,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;
> > -
> > -    *mfn = INVALID_MFN;
> > -    *page = get_page_from_gfn(rd, gfn, &p2mt,
> > -                              readonly ? P2M_ALLOC : P2M_UNSHARE);
> > -    if ( !*page )
> > -    {
> > -#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
> > -        return GNTST_bad_page;
> > -    }
> > +    int rc;
> >
> > -    if ( p2m_is_foreign(p2mt) )
> [snip]
> >      {
> [snip]
> > -        put_page(*page);
> > -        *page = NULL;
> > -
> 
> Comparing before-and-after, this seems to remove this 'p2m_is_foreign()'
> check.  Am I missing something?
> 

I may be. I thought p2m_is_ram() ruled out foreign pages (p2m_is_any_ram() being the way to include foreign maps if required). I'll check.

> > diff --git a/xen/common/memory.c b/xen/common/memory.c
> > index 35da9ca80e..419b76ac38 100644
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -1574,30 +1574,31 @@ void destroy_ring_for_helper(
> >      }
> >  }
> >
> > -int prepare_ring_for_helper(
> > -    struct domain *d, unsigned long gmfn, struct page_info **_page,
> > -    void **_va)
> > +int get_paged_gfn(struct domain *d, gfn_t gfn, bool readonly,
> > +                  p2m_type_t *p2mt_p, struct page_info **page_p)
> 
> This wants a comment somewhere describing exactly what the function does
> and what it will return -- probably here above the function definition
> itself would be the best.
> 

Ok.

> >  {
> > -    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);
> > +
> > +        p2m_mem_paging_populate(d, gfn_x(gfn));
> >          return -ENOENT;
> 
> I realize you're copying the return values of prepare_ring_for_helper(),
> but wouldn't -EAGAIN be more natural here?
> 

I may be able to swap for EAGAIN. I agree it seems more appropriate. Hopefully it won't complicate the callers too much.

  Paul

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