Re: [PATCH v9 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 11 October 2017 09:47
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: 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@lists.xenproject.org; KonradRzeszutek Wilk
> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> Subject: RE: [Xen-devel] [PATCH v9 10/11] common: add a new mappable
> resource type: XENMEM_resource_grant_table
> 
> >>> On 10.10.17 at 18:01, <Paul.Durrant@citrix.com> wrote:
> >> > @@ -3795,6 +3807,18 @@ int gnttab_map_frame(struct domain *d,
> >> unsigned long idx, gfn_t gfn,
> >> >      return rc;
> >> >  }
> >> >
> >> > +int gnttab_get_frame(struct domain *d, unsigned long idx, mfn_t
> *mfn)
> >>
> >> const struct domain * (I realize now that the same should have
> >> been done for gnttab_map_frame() when it was introduced;
> >> perhaps you could change that at the same time).
> >
> > Again, the problem is that grow_table and functions it calls don't take a
> > const pointer. I tried cascading the const through to the underlying function
> > but the patch started to balloon so I think such work should be deferred.
> 
> Right, I had overlooked that. And it won't work, as
> share_xen_page_with_guest() can't be passed a const pointer.
> 
> >> > @@ -993,6 +1018,11 @@ static int acquire_resource(const
> >> xen_mem_acquire_resource_t *xmar)
> >> >                                           xmar->nr_frames, mfn_list);
> >> >          break;
> >> >
> >> > +    case XENMEM_resource_grant_table:
> >> > +        rc = acquire_grant_table(d, xmar->id, xmar->frame,
> >> > +                                 xmar->nr_frames, mfn_list);
> >> > +        break;
> >>
> >> Is this really generally useful with mfn_list[] having just two entries?
> >>
> >
> > Good point. I'll increase the size of the array in this patch (to the
> > default table size of 32... I think that's a reasonable value to choose).
> 
> I suppose for the only current use you have for this (seeding the
> grant table from the tool stack) even the two entries you have
> right now would suffice. If, however, a full grant table is supposed
> to be accessible this way, I can't see how a static upper limit will do.
> Or if you intend the caller to do multiple invocations in such a case,
> there ought to be a way to find out the (implementation) limit.

I'm open to ideas but there clearly needs to be some sort of upper limit, or we do away with being able to map multiple frames in a single invocation. The dm_op hypercalls currently have a similar upper limit on the size of the buffer array. I'd rather not have to introduce another hypercall just to find out such a thing. It's a tools-only hypercall so could I not just add a comment on what the limit currently is?

  Paul

> 
> Jan


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