Re: [PATCH v2] Driver for Inter-VM shared memory device for KVM supporting interrupts.

[Anthony Liguori <anthony@codemonkey.ws>]

Christian Bornträger wrote:
> Am Montag 18 Mai 2009 16:26:15 schrieb Avi Kivity:
>   
>> Christian Borntraeger wrote:
>>     
>>> Sorry for the late question, but I missed your first version. Is there a
>>> way to change that code to use virtio instead of PCI? That would allow us
>>> to use this driver on s390 and maybe other virtio transports.
>>>       
>> Opinion differs.  See the discussion in
>> http://article.gmane.org/gmane.comp.emulators.kvm.devel/30119.
>>
>> To summarize, Anthony thinks it should use virtio, while I believe
>> virtio is useful for exporting guest memory, not for importing host memory.
>>     
>
> I think the current virtio interface is not ideal for importing host memory, 
> but we can change that. If you look at the dcssblk driver for s390, it allows 
> a guest to map shared memory segments via a diagnose (hypercall). This driver 
> uses PCI regions to map memory.
>
> My point is, that the method to map memory is completely irrelevant, we just 
> need something like mmap/shmget between the guest and the host. We could 
> define an interface in virtio, that can be used by any transport. In case of 
> pci this could be a simple pci map operation. 
>
> What do you think about something like: (CCed Rusty)
> ---
>  include/linux/virtio.h |   26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> Index: linux-2.6/include/linux/virtio.h
> ===================================================================
> --- linux-2.6.orig/include/linux/virtio.h
> +++ linux-2.6/include/linux/virtio.h
> @@ -71,6 +71,31 @@ struct virtqueue_ops {
>  };
>  
>  /**
> + * virtio_device_ops - operations for virtio devices
> + * @map_region: map host buffer at a given address
> + *	vdev: the struct virtio_device we're talking about.
> + *	addr: The address where the buffer should be mapped (hint only)
> + *	length: THe length of the mapping
> + *	identifier: the token that identifies the host buffer
> + *      Returns the mapping address or an error pointer.
> + * @unmap_region: unmap host buffer from the address
> + *	vdev: the struct virtio_device we're talking about.
> + *	addr: The address where the buffer is mapped
> + *      Returns 0 on success or an error
> + *
> + * TBD, we might need query etc.
> + */
> +struct virtio_device_ops {
> +	void * (*map_region)(struct virtio_device *vdev,
> +			     void *addr,
> +			     size_t length,
> +			     int identifier);
> +	int (*unmap_region)(struct virtio_device *vdev, void *addr);
> +/* we might need query region and other stuff */
> +};
>   

Perhaps something that maps closer to the current add_buf/get_buf API.  
Something like:

struct iovec *(*map_buf)(struct virtqueue *vq, unsigned int *out_num, 
unsigned int *in_num);
void (*unmap_buf)(struct virtqueue *vq, struct iovec *iov, unsigned int 
out_num, unsigned int in_num);

There's symmetry here which is good.  The one bad thing about it is 
forces certain memory to be read-only and other memory to be 
read-write.  I don't see that as a bad thing though.

I think we'll need an interface like this so support driver domains too 
since "backend".  To put it another way, in QEMU, map_buf == 
virtqueue_pop and unmap_buf == virtqueue_push.

Regards,

Anthony Liguori