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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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...


> 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.

> +/*
> + * 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
 (c) how to reclaim the address space which is otherwise lost

> +/**
> + * 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.)

> +/**
> + * 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 ?

> +/*
> + * 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 ?

> +/*
> + * Return an fd onto the grant sharing driver.  Logs errors.

It does not return an fd.  (See also my comments on gntmap.)

> +/*
> + * 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 ?

> +/*
> + * 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' ?

> +/*
> + * 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 ?

Ian.

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