Re: [PATCH] ioeventfd: Introduce KVM_IOEVENTFD_FLAG_PIPE

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux