Re: [PATCH] ioeventfd: Introduce KVM_IOEVENTFD_FLAG_PIPE

[Avi Kivity <avi@redhat.com>]

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.

> >  +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.

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.

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