Re: [Xen-devel] [PATCH v2] xen: grant-table: Simplify get_paged_frame

[Julien Grall <julien.grall@xxxxxxx>]

On 19/09/17 10:47, Paul Durrant wrote:
> > -----Original Message-----
> > From: Julien Grall [mailto:julien.grall@xxxxxxx]
> > Sent: 19 September 2017 10:40
> > To: Jan Beulich <JBeulich@xxxxxxxx>; Paul Durrant
> > <Paul.Durrant@xxxxxxxxxx>
> > Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> > <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Wei Liu
> > <wei.liu2@xxxxxxxxxx>; sstabellini@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxx; Tim
> > (Xen.org) <tim@xxxxxxx>
> > Subject: Re: [Xen-devel] [PATCH v2] xen: grant-table: Simplify
> > get_paged_frame
> >
> > Hi,
> >
> > On 19/09/17 09:56, Jan Beulich wrote:
> > > > > > On 19.09.17 at 10:34, <Paul.Durrant@xxxxxxxxxx> wrote:
> > > > I do wonder whether this function belongs in the grant table code though.
> > > > Getting the page from a (d, gfn) tuple is probably something that's
> > needed in
> > > > a few places and hence putting the code in common/memory.c (with
> > suitable
> > > > adjustment to the error values) would seem more appropriate.
> > > That's been true from the very beginning of the existence of
> > > the function, I think.
> > I am not sure how this function would fit in common/memory.c code. We
> > already have get_page_from_gfn to get a page from the tuple (d, gfn).
> But that doesn't have the extra logic for populating and unsharing right? 
> Otherwise why would get_paged_frame() need to exist at all?
This code does not have the logic to unshare. It just denies it.

There are multiple places in grant-table which require similar check. 
Hence why the helper is here to avoid duplicating the code in grant_table.c.
With this patch, this will contain specific check for grant mapping, 
such as denying foreign mapping. Are we going to impose that for 
get_paged_frame in common/memory.c?
> > This function adds more check that may not fit everyone. The only place
> > I could see potential usage is prepare_ring_for_helper. But what would
> > be a suitable name given?
> I would leave the name alone and just move the code.
The name does not make sense in common/memory.c. Why would you allow 
populating but not unsharing? This looks to me a decision that fits 
grant-table and not necessarily the rest of the Xen.
So I still don't think this would be a suitable function in common/memory.c

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel