Re: [PATCH 6/6] xen/gntalloc, gntdev: Add unmap notify ioctl

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 02/14/2011 10:37 AM, Konrad Rzeszutek Wilk wrote:
>> diff --git a/include/xen/gntalloc.h b/include/xen/gntalloc.h
>> index bc3b85e..257cc8d 100644
>> --- a/include/xen/gntalloc.h
>> +++ b/include/xen/gntalloc.h
>> @@ -47,4 +47,32 @@ struct ioctl_gntalloc_dealloc_gref {
>>  	/* Number of references to unmap */
>>  	uint32_t count;
>>  };
>> +
>> +/*
>> + * Sets up an unmap notification within the page, so that the other side can do
>> + * cleanup if this side crashes. Required to implement cross-domain robust
>> + * mutexes or close notification on communication channels.
>> + *
>> + * Each mapped page only supports one notification; multiple calls referring to
>> + * the same page overwrite the previous notification. You must clear the
>> + * notification prior to the IOCTL_GNTALLOC_DEALLOC_GREF if you do not want it
>> + * to occur.
>> + */
>> +#define IOCTL_GNTALLOC_SET_UNMAP_NOTIFY \
>> +_IOC(_IOC_NONE, 'G', 7, sizeof(struct ioctl_gntalloc_unmap_notify))
>> +struct ioctl_gntalloc_unmap_notify {
>> +	/* IN parameters */
>> +	/* Index of a byte in the page */
>> +	uint64_t index;
> 
> That isn't actually the whole truth. It is the index within the page
> and also the "offset used on a subsequent call to mmap()" (ioctl_gntdev_map_grant_ref)
> 
> It might make sense to change the description to: "offset of the mmap region
> and the index within the page" perhaps?

How about:
	/* Offset in the file descriptor for a byte within the page (same as
	 * used in mmap). If using UNMAP_NOTIFY_CLEAR_BYTE, this is the byte to
	 * be cleared. Otherwise, it can be any byte in the page whose
	 * notification we are adjusting.
	 */ 

Would you prefer a patch that just changes the descriptions, or a repost of
the patch that adds the ioctl with the better descriptions?

>> +	/* Action(s) to take on unmap */
>> +	uint32_t action;
>> +	/* Event channel to notify */
>> +	uint32_t event_channel_port;
>> +};
>> +
>> +/* Clear (set to zero) the byte specified by index */
>> +#define UNMAP_NOTIFY_CLEAR_BYTE 0x1
>> +/* Send an interrupt on the indicated event channel */
>> +#define UNMAP_NOTIFY_SEND_EVENT 0x2
>> +
>>  #endif /* __LINUX_PUBLIC_GNTALLOC_H__ */

[and the same for gntdev]

-- 
Daniel De Graaf
National Security Agency