Re: [PATCH] ioeventfd: Introduce KVM_IOEVENTFD_FLAG_PIPE

[Avi Kivity <avi@redhat.com>]

On 07/03/2011 08:44 PM, Sasha Levin wrote:
> On Sun, 2011-07-03 at 20:16 +0300, Avi Kivity wrote:
> >  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 +++++++++++++++++
>
> I couldn't find the ioeventfd docs in there and forgot that it's not in
> the mainline yet, I'll rebase on kvm git tree :)

Please do that forall kvm patches in the future.

> >
> >  Is there no generic helper for this?  Should there be?
> >
>
> I couldn't find one, I took the code above from fs/splice.c.
>
> There should probably be a generic version of it as this snippet repeats
> itself in several locations throughout the kernel.

Yes.  Suggest sending a patchset that consolidates this to akpm.

> >  >
> >  >  -	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.
> >
>
> This can't be variable length.
>
> The user defines an ioeventfd as an address+length (with length being up
> to 8 bytes). The only time an ioeventfd is signaled is when the write to
> the guest memory is exactly at the specified address with exactly the
> specified length.
>

It can be variable length if multiple ioeventfds reference the same pipe.

> ioeventfds can be extended to handle more than 8 bytes, variable address
> offset and reads now that pipe support is added, but I'd rather do it in
> follow-up patches once basic pipe support is in.

In general incremental development is great, but I don't want to 
fragment the ABI.  I'd like to be able to forward an entire PCI BAR over 
a pipe.  That means sending the address/data/length tuple, and both read 
and write support.

> >  Is the write guaranteed atomic?  We probably need serialization here.
>
> afaik vfs_write is just a wrapper to the write() function of the
> underlying fs so it should be atomic, no?

write() isn't atomic in general.  It is for pipes under certain 
circumstances, but there is no guarantee that the circumstances apply, 
or that the fd is in fact a pipe.

> >  >    	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?
>
> I didn't think that assigning 2 different events with exactly the same
> address, length and data can happen. Why would it?
>

No reason, but this is a user interface.  You can't assume anything 
about the user except that he is an evil genius intent on breaking the 
kernel in various ways.

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