Re: [PATCH] ioeventfd: Introduce KVM_IOEVENTFD_FLAG_PIPE

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

On Mon, Jul 04, 2011 at 01:45:07PM +0300, Avi Kivity wrote:
> On 07/04/2011 01:32 PM, Michael S. Tsirkin wrote:
> >On Sun, Jul 03, 2011 at 08:04:49PM +0300, 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.
> >
> >Looking at this example, how would you handle a pipe full condition?
> >We can't buffer unlimited amount of data in the host.
> 
> Stall.

Right, but the guest gets no indication that the pipe is full.
Something like virtio would let the guest do something useful
instead of stalling the vcpu.

Also noting that the fd can be set not to block, or that
a signal can interrupt the write. Both cases are not errors.

> >>  +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() */
> >
> >Interesting. Is buf really always a user pointer?
> >Why don't we tag it __user then?
> 
> It's not a user pointer, this is just to get the tools happy.
> set_fs() makes it safe.
> 
> >>  +	res = vfs_write(file, (const char __user *)buf, count,&pos);
> >
> >If pipe is non-blocking, or if we get a signal,
> >this might fail or return a value<  len.
> >Data will be lost then, won't it?
> 
> Yes.  Need a loop-until-buffer-exhausted-or-error.

Signal handling becomes a problem. You don't want a
full pipe to prevent qemu from getting killed or
getting a timer alert.

> Error reporting is an interesting question.  Typically we have
> KVM_RUN return the error, but if we use this facility to run
> something in a separate process, this can cause the device process
> crash to cause the guest to crash.
> 
> >>  -	p->eventfd = eventfd;
> >>  +
> >>  +	if (args->flags&  KVM_IOEVENTFD_FLAG_PIPE)
> >>  +		p->pipe = fget(args->fd);
> >
> >This really needs to check that the fd is a pipe.
> >Otherwise you can do weird things like pass in
> >the kvm device fd itself.
> 
> Eww, reference loop.  Good catch.
> 
> We should allow unix domain sockets as well.  In fact, for
> read/write support, we need this to be a unix domain socket.

Sockets are actually better at this than pipes
as you can at least make the writes
non-blocking by passing in a message flag.
If we support sockets, do we really need to support
pipes at all?

-- 
MST