Re: [Xen-devel] [PATCH XEN v5 07/23] tools: Refactor /dev/xen/gnt{dev, shr} wrappers into libxengnttab.

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Wed, 2015-11-11 at 15:03 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH XEN v5 07/23] tools: Refactor
> /dev/xen/gnt{dev,shr} wrappers into libxengnttab."):
> > libxengnttab will provide a stable API and ABI for accessing the
> > grant table devices.
> > 
> > The functions are moved into the xengnt{tab,shr} namespace to make a
> > clean break from libxc and avoid ambiguity regarding which interfaces
> > are stable.
> 
> I have reviewed the API.ÂÂMostly this produced questions...

Yes, thanks this is the sort of feedback I was looking for before we call
this a stable interface.

Some of my answers are of the "yes, we should decide that" variety.

Daniel, I've added you because some of the questions below relate to the
notification mechanism and to gntshr which I don't really understand (not
that I really understand gnttab either).

> > XXX consider combining into a single namespace (i.e. with
> > xengnttab_handle having two open fd's in it on Linux)
> 
> Arguments in favour of combining:
> Â* Some kernel might provide a single thing in /dev, and might even
> ÂÂÂhave trouble coping with a process which opens it more than once.
> Â* This is less fiddly for callers.
> 
> Arguments against combining:
> Â* Perhaps some OSes will exist which can do only one and not the
> ÂÂÂother.ÂÂ(Tenuous, and a workaround is possible.)
> Â* Slightly lower performance for a client which uses only one of the
> ÂÂÂinterfaces.ÂÂ(Barrel-scraping, here.)
> Â* It is more work.
> 
> I conclude it would be better to unify them.

OK, I'm convinced.

> > +/*
> > + * Note:
> > + * After fork a child process must not use any opened xc gnttab
> > + * handle inherited from their parent. They must open a new handle if
> > + * they want to interact with xc.
> 
> This should document
> Â(a) that it is safe to close the handle
> Â(b) that it is not safe to access grant mapped areas in the child

These two are easy.

> Â(c) how to reclaim the address space which is otherwise lost

The "obviously going to work, but probably not very useful" answer is you
should reclaim them before forking. That's clearly not practical in any
number of situations.

I really not sure what would happen if the child did a gnttab_close(). I
_think_ the kernel side ought to do the right thing (in that my opinion is
that really it should, and looking at the code it seems like it might).

Making a statement here that gnttab_close() is OK (and the right thing) to
do in the child might make the kernel driver insta-buggy. That might be
tolerable though, no one seems too be bothered by such bugs today and if
someone trips over them we can fix them.

> > +/**
> > + * Memory maps one or more grant references from one or more domains
> > to a
> > + * contiguous local address range. Mappings should be unmapped with
> > + * xengnttab_munmap.ÂÂLogs errors.
> > + *
> > + * @parm xgt a handle on an open grant table interface
> > + * @parm count the number of grant references to be mapped
> > + * @parm domids an array of @count domain IDs by which the
> > corresponding @refs
> > + *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂwere granted
> > + * @parm refs an array of @count grant references to be mapped
> > + * @parm prot same flag as in mmap()
> > + */
> > +void *xengnttab_map_grant_refs(xengnttab_handle *xgt,
> 
> Should explain what happens on partial failure.
> 
> > +/**
> > + * Memory maps one or more grant references from one domain to a
> > + * contiguous local address range. Mappings should be unmapped with
> > + * xengnttab_munmap.ÂÂLogs errors.
> ...
> > +void *xengnttab_map_domain_grant_refs(xengnttab_handle *xgt,
> 
> Should say that this is equivalent to a suitable call to
> xengnttab_map_grant_refs.ÂÂ(Assuming that it is.)

I believe so.

Which might argue for not having both, but I think avoiding the common case
of needing to provide an array full of the same domid makes it worthwhile
to have this.

> > +/**
> > + * Memory maps a grant reference from one domain to a local address
> > range.
> > + * Mappings should be unmapped with xengnttab_munmap. If notify_offset
> > or
> > + * notify_port are not -1, this version will attempt to set up an
> > unmap
> > + * notification at the given offset and event channel. When the page
> > is
> > + * unmapped, the byte at the given offset will be zeroed and a wakeup
> > will be
> > + * sent to the given event channel.ÂÂLogs errors.
> 
> What happens if the unmap notification cannot be set up ?
> 
> Also "when the page is unmapped" makes it sound like you mean
> xengnttab_munmap but actually I think this is when the grant is
> withdrawn by the grantor ?
> 
> If the grant is withdrawn by the grantor, does the page become
> unuseable immediately ?ÂÂIf so, how can anyone ever use this safely ?

Daniel, could you answer these ones please.

> 
> > +/*
> > + * Sets the maximum number of grants that may be mapped by the given
> > instance
> > + * to @count.ÂÂNever logs.
> 
> Line wrap.
> 
> 
> > +int xengnttab_set_max_grants(xengnttab_handle *xgt,
> > +                   ÂÂÂÂÂuint32_t count);
> 
> Is count in pages or grants or what ?

It was hard to find this out, because the underlying ioctl doesn't exist in
the Linux side. In the end I found it is in grants as the name suggests.

AFAICT this is implemented only by linux-2.6.18-xen.hg and mini-os.git. We
could consider omitting it from the initial API. QEMU calls it but doesn't
care if it didn't work.

> > +/*
> > + * Return an fd onto the grant sharing driver.ÂÂLogs errors.
> 
> It does not return an fd.

Oh yes.

> ÂÂ(See also my comments on gntmap.)

I'm not sure which ones you mean here. If you meant gnttab then I'm still
not sure since you didn't comment on _open() there, but that's the only
commonality in the API.

> 
> > +/*
> > + * Creates and shares pages with another domain.
> > + *
> ...
> > +void *xengntshr_share_pages(xengntshr_handle *xgs, uint32_t domid,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂint count, uint32_t *refs, int writable);
> 
> Can this fail ?ÂÂCan it partially succeed ?

Daniel?

> 
> > +/*
> > + * Creates and shares a page with another domain, with unmap
> > notification.
> > + *
> > + * @parm xgs a handle to an open grant sharing instance
> > + * @parm domid the domain to share memory with
> > + * @parm refs the grant reference of the pages (output)
> > + * @parm writable true if the other domain can write to the page
> > + * @parm notify_offset The byte offset in the page to use for unmap
> > + *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂnotification; -1 for none.
> > + * @parm notify_port The event channel port to use for unmap notify,
> > or -1
> > + * @return local mapping of the page
> > + */
> > +void *xengntshr_share_page_notify(xengntshr_handle *xgs, uint32_t
> > domid,
> 
> What is this `unmap notification' ?

Daniel?

> 
> > +/*
> > + * Unmaps the @count pages starting at @start_address, which were
> > mapped by a
> > + * call to xengntshr_share_*. Never logs.
> 
> Linewrap in the comment.
> 
> > + */
> > +int xengntshr_munmap(xengntshr_handle *xgs, void *start_address,
> > uint32_t count);
> 
> What effect does this have on the peer ?

Daniel?

> 
> Ian.

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