Re: [PATCH v9 5/7] memory: add check_get_page_from_gfn() as a wrapper...

From: Paul Durrant <Paul.Durrant@citrix.com>
To: 'Jan Beulich' <JBeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Andrew Cooper <Andrew.Cooper3@citrix.com>,
	"Tim (Xen.org)" <tim@xen.org>,
	George Dunlap <George.Dunlap@citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Ian Jackson <Ian.Jackson@citrix.com>
Subject: Re: [PATCH v9 5/7] memory: add check_get_page_from_gfn() as a wrapper...
Date: Wed, 19 Sep 2018 07:56:02 +0000	[thread overview]
Message-ID: <f6f35c7b1ccf4abbaf0352f99b89e4fe@AMSPEX02CL03.citrite.net> (raw)
In-Reply-To: <5BA1E69F02000078001E99DF@prv1-mh.provo.novell.com>

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 19 September 2018 07:03
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian
> Jackson <Ian.Jackson@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Stefano
> Stabellini <sstabellini@kernel.org>; xen-devel <xen-
> devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> Subject: RE: [Xen-devel] [PATCH v9 5/7] memory: add
> check_get_page_from_gfn() as a wrapper...
> 
> >>> On 18.09.18 at 18:37, <Paul.Durrant@citrix.com> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 18 September 2018 15:51
> >> To: Paul Durrant <Paul.Durrant@citrix.com>
> >> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> >> <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>;
> Ian
> >> Jackson <Ian.Jackson@citrix.com>; Wei Liu <wei.liu2@citrix.com>;
> Stefano
> >> Stabellini <sstabellini@kernel.org>; xen-devel <xen-
> >> devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> >> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> >> Subject: RE: [Xen-devel] [PATCH v9 5/7] memory: add
> >> check_get_page_from_gfn() as a wrapper...
> >>
> >> >>> On 18.09.18 at 16:03, <Paul.Durrant@citrix.com> wrote:
> >> >>  -----Original Message-----
> >> >> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On
> >> Behalf
> >> >> Of Jan Beulich
> >> >> Sent: 18 September 2018 14:17
> >> >> To: Paul Durrant <Paul.Durrant@citrix.com>
> >> >> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> >> >> <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>;
> >> >> George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
> >> >> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
> Tim
> >> >> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; xen-
> devel
> >> >> <xen-devel@lists.xenproject.org>
> >> >> Subject: Re: [Xen-devel] [PATCH v9 5/7] memory: add
> >> >> check_get_page_from_gfn() as a wrapper...
> >> >>
> >> >> >>> On 13.09.18 at 17:21, <paul.durrant@citrix.com> wrote:
> >> >> > --- a/xen/include/asm-arm/p2m.h
> >> >> > +++ b/xen/include/asm-arm/p2m.h
> >> >> > @@ -303,6 +303,10 @@ static inline struct page_info
> >> *get_page_from_gfn(
> >> >> >      return page;
> >> >> >  }
> >> >> >
> >> >> > +int __must_check check_get_page_from_gfn(struct domain *d, gfn_t
> >> gfn,
> >> >> > +                                         bool readonly,
> p2m_type_t
> >> >> *p2mt_p,
> >> >> > +                                         struct page_info
> **page_p);
> >> >> > +
> >> >> >  int get_page_type(struct page_info *page, unsigned long type);
> >> >> >  bool is_iomem_page(mfn_t mfn);
> >> >> >  static inline int get_page_and_type(struct page_info *page,
> >> >> > --- a/xen/include/asm-x86/p2m.h
> >> >> > +++ b/xen/include/asm-x86/p2m.h
> >> >> > @@ -492,6 +492,9 @@ static inline struct page_info
> >> *get_page_from_gfn(
> >> >> >      return mfn_valid(_mfn(gfn)) && get_page(page, d) ? page :
> NULL;
> >> >> >  }
> >> >> >
> >> >> > +int __must_check check_get_page_from_gfn(struct domain *d, gfn_t
> >> gfn,
> >> >> > +                                         bool readonly,
> p2m_type_t
> >> >> *p2mt_p,
> >> >> > +                                         struct page_info
> **page_p);
> >> >> >
> >> >> >  /* General conversion function from mfn to gfn */
> >> >> >  static inline unsigned long mfn_to_gfn(struct domain *d, mfn_t
> mfn)
> >> >>
> >> >> Why twice the same declaration? Can't this be put in xen/p2m-
> common.h?
> >> >> Or some other suitable common header? With that
> >> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >> >>
> >> >
> >> > I was not aware of the common header so I placed it near the
> >> declarations of
> >> > get_page_from_gfn(). Do you want be to submit a v10 with this fixed
> or
> >> are
> >> > you happy to move it to wherever you think is appropriate during
> commit?
> >>
> >> I'd prefer a v10, to be sure things compile fine ahead of trying to
> commit
> >> this series.
> >>
> >
> > Moving to p2m-common won't work. The function declaration involves a
> > p2m_type_t argument and that enum is defined in the arm and x86 specific
> > headers. I propose therefore to leave this patch as-is.
> 
> Leaving the duplication in place is just the last resort imo. Does
> xen/mm.h
> not work either?

No, it won't. It has to be something *after* the definition of the p2m_type_t enum. I could, as Julien suggested, move the inclusion of p2m-common after that point, but it would mean the header guards would cease to DTRT of course. I'm not sure whether C will allow me to forward declare the enum (not something I've tried) but I could give that a go. Any other suggestions?

  Paul

> 
> Jan
> 

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