> -----Original Message----- > From: George Dunlap [mailto:george.dun...@citrix.com] > Sent: 11 July 2018 12:24 > To: Paul Durrant <paul.durr...@citrix.com>; xen-devel@lists.xenproject.org > Cc: Jan Beulich <jbeul...@suse.com>; Andrew Cooper > <andrew.coop...@citrix.com>; George Dunlap > <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Julien > Grall <julien.gr...@arm.com>; Konrad Rzeszutek Wilk > <konrad.w...@oracle.com>; Stefano Stabellini <sstabell...@kernel.org>; Tim > (Xen.org) <t...@xen.org>; Wei Liu <wei.l...@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.durr...@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