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.campb...@citrix.com>
> > Cc: Daniel De Graaf <dgde...@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 <dgde...@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
