Re: [PATCH] ioeventfd: Introduce KVM_IOEVENTFD_FLAG_PIPE

[Avi Kivity <avi@redhat.com>]

On 07/03/2011 08:04 PM, Sasha Levin wrote:
> The new flag allows passing a write side of a pipe instead of an
> eventfd to be notified of writes to the specified memory region.
>
> Instead of signaling an event, the value written to the memory region
> is written to the pipe.
>
> Using a pipe instead of an eventfd is usefull when any value can be
> written to the memory region but we're interested in recieving the
> actual value instead of just a notification.
>
> A simple example for practical use is the serial port. we are not
> interested in an exit every time a char is written to the port, but
> we do need to know what was written so we could handle it on the guest.

> ---
>   include/linux/kvm.h |    2 +
>   virt/kvm/eventfd.c  |   65 +++++++++++++++++++++++++++++++++++---------------

Documentation/virtua/kvm/api.txt +++++++++++++++++
>
> @@ -424,6 +425,7 @@ struct _ioeventfd {
>   	struct list_head     list;
>   	u64                  addr;
>   	int                  length;
> +	struct file         *pipe;
>   	struct eventfd_ctx  *eventfd;

In a union with eventfd please.

> @@ -481,6 +487,21 @@ ioeventfd_in_range(struct _ioeventfd *p, gpa_t addr, int len, const void *val)
>   	return _val == p->datamatch ? true : false;
>   }
>
> +static ssize_t kernel_write(struct file *file, const char *buf, size_t count,
> +			    loff_t pos)
> +{
> +	mm_segment_t old_fs;
> +	ssize_t res;
> +
> +	old_fs = get_fs();
> +	set_fs(get_ds());
> +	/* The cast to a user pointer is valid due to the set_fs() */
> +	res = vfs_write(file, (const char __user *)buf, count,&pos);
> +	set_fs(old_fs);
> +
> +	return res;
> +}
> +

Is there no generic helper for this?  Should there be?

>   /* MMIO/PIO writes trigger an event if the addr/val match */
>   static int
>   ioeventfd_write(struct kvm_io_device *this, gpa_t addr, int len,
> @@ -491,7 +512,11 @@ ioeventfd_write(struct kvm_io_device *this, gpa_t addr, int len,
>   	if (!ioeventfd_in_range(p, addr, len, val))
>   		return -EOPNOTSUPP;
>
> -	eventfd_signal(p->eventfd, 1);
> +	if (p->pipe)
> +		kernel_write(p->pipe, val, len, 0);

You're writing potentially variable length data.

We need a protocol containing address, data, length, and supporting read 
accesses as well.

Is the write guaranteed atomic?  We probably need serialization here.

> +	else
> +		eventfd_signal(p->eventfd, 1);
> +
>   	return 0;
>   }
>
> @@ -555,9 +580,11 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>   	if (args->flags&  ~KVM_IOEVENTFD_VALID_FLAG_MASK)
>   		return -EINVAL;
>
> -	eventfd = eventfd_ctx_fdget(args->fd);
> -	if (IS_ERR(eventfd))
> -		return PTR_ERR(eventfd);
> +	if (!(args->flags&  KVM_IOEVENTFD_FLAG_PIPE)) {
> +		eventfd = eventfd_ctx_fdget(args->fd);
> +		if (IS_ERR(eventfd))
> +			return PTR_ERR(eventfd);
> +	}
>
>   	p = kzalloc(sizeof(*p), GFP_KERNEL);
>   	if (!p) {
> @@ -568,7 +595,11 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>   	INIT_LIST_HEAD(&p->list);
>   	p->addr    = args->addr;
>   	p->length  = args->len;
> -	p->eventfd = eventfd;
> +
> +	if (args->flags&  KVM_IOEVENTFD_FLAG_PIPE)
> +		p->pipe = fget(args->fd);
> +	else
> +		p->eventfd = eventfd;

The split logic with the previous hunk isn't nice.  Suggest moving the 
'else' there, and assigning the whole union here.

>   	list_for_each_entry_safe(p, tmp,&kvm->ioeventfds, list) {
>   		bool wildcard = !(args->flags&  KVM_IOEVENTFD_FLAG_DATAMATCH);
>
> -		if (p->eventfd != eventfd  ||
> -		    p->addr != args->addr  ||
> +		if (p->addr != args->addr  ||
>   		    p->length != args->len ||
>   		    p->wildcard != wildcard)
>   			continue;

Why?


-- 
error compiling committee.c: too many arguments to function