Re: [PATCH 1/5] vring: Replace mmap() interface with ioctl()

[Avi Kivity <avi@qumranet.com>]

Mark McLoughlin wrote:
> /dev/vring's mmap() interface is a strange creature. It
> serves as a way for userland to supply the address of the
> already allocated ring descriptors, but causes those pages
> to be re-maped as a natural side effect of the mmap()
>
> This is not an issue for lguest because it does the mmap()
> before even starting the guest. However, in the case of kvm,
> the guest allocates the ring and informs the host of its
> addresss. If we then mmap() it, we cause it to be remapped
> to new pages which the vring driver will then use.
>
> Now, KVM guests don't actually use the ring pages before
> informing the host of its address, so we could probably just
> invalidate the guest's shadow page table and have the new
> pfns picked up. That would be an odd requirement to impose
> on the guest ABI, though.
>
> Since the mmap() semantics are so strange, switch to using a
> single ioctl() for setting up the ring.
>
>   

Definitely an improvement; iterfaces which seem familiar but aren't are 
not user friendly.

In any case the guest can't use the ring directly since physical memory 
is discontiguous.

> -	.ioctl		= vring_ioctl,
> +	.unlocked_ioctl	= vring_ioctl,
> +	.compat_ioctl	= vring_compat_ioctl,
>   

I think you can set compat_ioctl = vring_ioctl (that's what kvm does).

> diff --git a/include/linux/vring.h b/include/linux/vring.h
> index 47c8848..de4125d 100644
> --- a/include/linux/vring.h
> +++ b/include/linux/vring.h
> @@ -21,8 +21,14 @@
>  #include <linux/types.h>
>  
>  /* Ioctl defines. */
> -#define VRINGSETBASE	_IO(0xAD, 0)
> -#define VRINGSETLIMIT	_IO(0xAD, 1)
> +#define VRINGSETINFO _IO(0xAD, 0)
> +
> +struct vring_ioctl_info {
> +	__u16 num_descs;
>   

Padding for 64-bits here, otherwise compat_ioctl breaks.

> +	__u64 descs;
> +	__u64 base;
> +	__u64 limit;
> +};


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.