> -----Original Message-----
> From: George Dunlap [mailto:[email protected]]
> Sent: 11 July 2018 12:24
> To: Paul Durrant <[email protected]>; [email protected]
> Cc: Jan Beulich <[email protected]>; Andrew Cooper
> <[email protected]>; George Dunlap
> <[email protected]>; Ian Jackson <[email protected]>; Julien
> Grall <[email protected]>; Konrad Rzeszutek Wilk
> <[email protected]>; Stefano Stabellini <[email protected]>; Tim
> (Xen.org) <[email protected]>; Wei Liu <[email protected]>
> 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 <[email protected]>
>
> 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
