Re: [PATCH 5/5] ioeventfd: Introduce KVM_IOEVENTFD_FLAG_SOCKET

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jul 06, 2011 at 06:01:46PM +0300, Sasha Levin wrote:
> On Wed, 2011-07-06 at 14:42 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jul 06, 2011 at 07:37:58AM +0300, Sasha Levin wrote:
> > > The new flag allows passing a connected socket instead of an
> > > eventfd to be notified of writes or reads to the specified memory region.
> > > 
> > > Instead of signaling an event, On write - the value written to the memory
> > > region is written to the pipe.
> > > On read - a notification of the read is sent to the host, and a response
> > > is expected with the value to be 'read'.
> > > 
> > > Using a socket 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.
> > > 
> > > Cc: Avi Kivity <avi@redhat.com>
> > > Cc: Ingo Molnar <mingo@elte.hu>
> > > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Pekka Enberg <penberg@kernel.org>
> > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > > ---
> > >  Documentation/virtual/kvm/api.txt |   18 ++++-
> > >  include/linux/kvm.h               |    9 ++
> > >  virt/kvm/eventfd.c                |  153 ++++++++++++++++++++++++++++++++-----
> > >  3 files changed, 161 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > > index 317d86a..74f0946 100644
> > > --- a/Documentation/virtual/kvm/api.txt
> > > +++ b/Documentation/virtual/kvm/api.txt
> > > @@ -1330,7 +1330,7 @@ Returns: 0 on success, !0 on error
> > >  
> > >  This ioctl attaches or detaches an ioeventfd to a legal pio/mmio address
> > >  within the guest.  A guest write in the registered address will signal the
> > > -provided event instead of triggering an exit.
> > > +provided event or write to the provided socket instead of triggering an exit.
> > >  
> > >  struct kvm_ioeventfd {
> > >  	__u64 datamatch;
> > > @@ -1341,6 +1341,13 @@ struct kvm_ioeventfd {
> > >  	__u8  pad[36];
> > >  };
> > >  
> > > +struct kvm_ioeventfd_data {
> > > +	__u64 data;
> > > +	__u64 addr;
> > > +	__u32 len;
> > > +	__u8  is_write;
> > > +};
> > > +
> > >  The following flags are defined:
> > >  
> > >  #define KVM_IOEVENTFD_FLAG_DATAMATCH (1 << kvm_ioeventfd_flag_nr_datamatch)
> > > @@ -1348,6 +1355,7 @@ The following flags are defined:
> > >  #define KVM_IOEVENTFD_FLAG_DEASSIGN  (1 << kvm_ioeventfd_flag_nr_deassign)
> > >  #define KVM_IOEVENTFD_FLAG_READ      (1 << kvm_ioeventfd_flag_nr_read)
> > >  #define KVM_IOEVENTFD_FLAG_NOWRITE   (1 << kvm_ioeventfd_flag_nr_nowrite)
> > > +#define KVM_IOEVENTFD_FLAG_SOCKET    (1 << kvm_ioeventfd_flag_nr_socket)
> > >  
> > >  If datamatch flag is set, the event will be signaled only if the written value
> > >  to the registered address is equal to datamatch in struct kvm_ioeventfd.
> > > @@ -1359,6 +1367,14 @@ passed in datamatch.
> > >  If the nowrite flag is set, the event won't be signaled when the specified address
> > >  is being written to.
> > >  
> > > +If the socket flag is set, fd is expected to be a connected AF_UNIX
> > > +SOCK_SEQPACKET socket.
> > 
> > Let's verify that then?
> > 
> > > +
> > > +	if (p->sock)
> > > +		socket_write(p->sock, &data, sizeof(data));
> > > +	else
> > > +		eventfd_signal(p->eventfd, 1);
> > > +
> > >  	return 0;
> > >  }
> > 
> > This still loses the data if socket would block and there's a signal.
> > I think we agreed to use non blocking operations and exit to
> > userspace in that case?
> > 
> > 
> > >  
> > > @@ -534,6 +607,7 @@ ioeventfd_read(struct kvm_io_device *this, gpa_t addr, int len,
> > >  		void *val)
> > >  {
> > >  	struct _ioeventfd *p = to_ioeventfd(this);
> > > +	struct kvm_ioeventfd_data data;
> > >  
> > >  	/* Exit if signaling on reads isn't requested */
> > >  	if (!p->track_reads)
> > > @@ -542,7 +616,21 @@ ioeventfd_read(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);
> > > +	data = (struct kvm_ioeventfd_data) {
> > > +		.addr = addr,
> > > +		.len = len,
> > > +		.is_write = 0,
> > > +	};
> > > +
> > > +	if (p->sock) {
> > > +		socket_write(p->sock, &data, sizeof(data));
> > > +		socket_read(p->sock, &data, sizeof(data));
> > > +		set_val(val, len, data.data);
> > 
> > Same here.
> 
> The socket_read() here I should leave blocking, and spin on it until I
> read something - right?

I think it's best to exit to userspace.

> > > +	} else {
> > > +		set_val(val, len, p->datamatch);
> > > +		eventfd_signal(p->eventfd, 1);
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >
> > 
> > 
> 
> 
> -- 
> 
> Sasha.