Re: [PATCH XEN v6 25/32] tools/libs/gnttab: Extensive updates to API documentation.

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-12-03 at 13:08 -0500, Daniel De Graaf wrote:
> On 03/12/15 06:22, Ian Campbell wrote:
> > In particular around error handling, behaviour on fork and the unmap
> > notification mechanism.
> > 
> > Behaviour of xengnttab_map_*grant_refs and xengntshr_share_pages on
> > partial failure has been confirmed/inferred (by inspection) on Linux
> > and Mini-os (the only two known implementations. Likewise the
> > behaviour of the notification mechanism has been confirmed/inferred
> > (by inspection) of the Linux implementation (currently the only
> > implementation) and libvchan (primary known user).
> > 
> > These updates are not folded into "tools: Refactor
> > /dev/xen/gnt{dev,shr} wrappers into libxengnttab." to try and reduce
> > the amount of non-movement changes in that patch.
> > 
> > While I'm not convinced by javadoc/doxygen cause the existing comments
> > which appear to use that syntax to have the appropriate /** marker.
> > 
> > Also fix a typo in a code comment.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> > ---
> > Daniel, you input on the description of the unmap notification stuff
> > would be much appreciated.
> 
> The description looks complete and correct to me.  The statement that
> the interfaces operate on a single page only might be misleading - the
> interface will work on multi-page mappings, but only allows one notify
> on the mapping.  I'm not sure this distinction is important for most
> users of the interface, since the notify is almost always used on a
> one-page mapping.

I think I concluded that it was a single page because the library
interfaces which allow a mapping to have a notify attached only take/return
a single gref:

void *xengnttab_map_grant_ref_notify(xengnttab_handle *xgt,
                                     uint32_t domid,
                                     uint32_t ref,
                                     int prot,
                                     uint32_t notify_offset,
                                     evtchn_port_t notify_port);
void *xengntshr_share_page_notify(xengntshr_handle *xgs, uint32_t domid,
                                  uint32_t *ref, int writable,
                                  uint32_t notify_offset,
                                  evtchn_port_t notify_port);

The offset in the underlying ioctl in the Linux implementation is a
uint64_t, so you are right that in principal it could support a larger
offset.

I think I am inclined to therefore leave things as they are, the
alternative would be to expand those two functions now to support multi-
gref mappings.

I think given the current set of use cases I would prefer to add multipage
variants in the future should the need arise.

I suppose the argument for making the change right no would be the
prediction of a userspace multipage ring implementation.

> Reviewed-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Thanks.

> 
> > @@ -71,6 +154,10 @@ void *xengnttab_map_grant_ref(xengnttab_handle
> > *xgt,
> >    * contiguous local address range. Mappings should be unmapped with
> >    * xengnttab_munmap.  Logs errors.
> >    *
> > + * On failure sets errno and returns NULL.
> > + *
> > + * If notify_offset or notify_port a requested and cannot be set up an
> > + * error will be returned and no mapping will be made.
> 
> Typo: "a requested" => "are requested"

Yes, thanks.


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