[Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Ian Molton]

On 10/10/10 16:11, Avi Kivity wrote:
> On 10/06/2010 05:59 PM, Ian Molton wrote:
>> This patch implements a virtio-based transport for use by a
>> virtualised OpenGL passthrough implementation.
>>
>> The libGL and qemu-gl code to support this patch are available here:
>>
>> http://gitorious.org/vm-gl-accel/qemu-gl
>> http://gitorious.org/vm-gl-accel/qemu-libgl
>>
>>
>> Comments please!
>
> 1. copy qemu-devel

Ok, will do.

> an virtualization@, many virtio developers live there.

you mean virtualization@lists.osdl.org ?

> 2. should start with a patch to the virtio-pci spec to document what
> you're doing

Where can I find that spec?

>> + /* Transfer data */
>> + if (virtqueue_add_buf(vq, sg_list, o_page, i_page, (void *)1)>= 0) {
>> + virtqueue_kick(vq);
>> + /* Chill out until it's done with the buffer. */
>> + while (!virtqueue_get_buf(vq,&count))
>> + cpu_relax();
>> + }
>> +
>
> This is pretty gross, and will burn lots of cpu if the hypervisor
> processes the queue asynchronously.

It doesnt, at present... It could be changed fairly easily ithout 
breaking anything if that happens though.

-Ian

[Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Avi Kivity]

  On 10/19/2010 12:31 PM, Ian Molton wrote:
>
>> an virtualization@, many virtio developers live there.
>
> you mean virtualization@lists.osdl.org ?

Yes.

>
>> 2. should start with a patch to the virtio-pci spec to document what
>> you're doing
>
> Where can I find that spec?

http://ozlabs.org/~rusty/virtio-spec/

>
>>> + /* Transfer data */
>>> + if (virtqueue_add_buf(vq, sg_list, o_page, i_page, (void *)1)>= 0) {
>>> + virtqueue_kick(vq);
>>> + /* Chill out until it's done with the buffer. */
>>> + while (!virtqueue_get_buf(vq,&count))
>>> + cpu_relax();
>>> + }
>>> +
>>
>> This is pretty gross, and will burn lots of cpu if the hypervisor
>> processes the queue asynchronously.
>
> It doesnt, at present... It could be changed fairly easily ithout 
> breaking anything if that happens though.

The hypervisor and the guest can be changed independently.  The driver 
should be coded so that it doesn't depend on hypervisor implementation 
details.

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

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Ian Molton]

[-- Attachment #1: Type: text/plain, Size: 717 bytes --]

On 19/10/10 11:39, Avi Kivity wrote:
> On 10/19/2010 12:31 PM, Ian Molton wrote:

>>> 2. should start with a patch to the virtio-pci spec to document what
>>> you're doing
>>
>> Where can I find that spec?
>
> http://ozlabs.org/~rusty/virtio-spec/

Ok, but I'm not patching that until theres been some review.

There are links to the associated qemu and guest OS changes in my 
original email.

>> It doesnt, at present... It could be changed fairly easily ithout
>> breaking anything if that happens though.
>
> The hypervisor and the guest can be changed independently. The driver
> should be coded so that it doesn't depend on hypervisor implementation
> details.

Fixed - updated patch tested and attached.

-Ian

[-- Attachment #2: virtio_gl.patch --]
[-- Type: text/x-patch, Size: 8972 bytes --]

diff --git a/drivers/gpu/Makefile b/drivers/gpu/Makefile
index 30879df..35699a0 100644
--- a/drivers/gpu/Makefile
+++ b/drivers/gpu/Makefile
@@ -1 +1 @@
-obj-y			+= drm/ vga/
+obj-y			+= drm/ vga/ misc/
diff --git a/drivers/gpu/misc/Kconfig b/drivers/gpu/misc/Kconfig
new file mode 100644
index 0000000..50043d3
--- /dev/null
+++ b/drivers/gpu/misc/Kconfig
@@ -0,0 +1,8 @@
+config VIRTIOGL
+        tristate "Virtio userspace memory transport"
+        depends on VIRTIO_PCI
+        default n
+        help
+          A Driver to facilitate transferring data from userspace to a
+          hypervisor (eg. qemu)
+
diff --git a/drivers/gpu/misc/Makefile b/drivers/gpu/misc/Makefile
new file mode 100644
index 0000000..d9ab333
--- /dev/null
+++ b/drivers/gpu/misc/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_VIRTIOGL)          += virtio-gl.o
diff --git a/drivers/gpu/misc/virtio-gl.c b/drivers/gpu/misc/virtio-gl.c
new file mode 100644
index 0000000..ad3ba14
--- /dev/null
+++ b/drivers/gpu/misc/virtio-gl.c
@@ -0,0 +1,325 @@
+/*
+ * Copyright (C) 2010 Intel Corporation
+ *
+ * Author: Ian Molton <ian.molton@collabora.co.uk>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/dma-mapping.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/miscdevice.h>
+#include <linux/virtio.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_config.h>
+
+#define DEVICE_NAME "glmem"
+
+/* Define to use debugging checksums on transfers */
+#undef DEBUG_GLIO
+
+struct virtio_gl_data {
+	char *buffer;
+	int pages;
+	unsigned int pid;
+	struct completion done;
+};
+
+struct virtio_gl_header {
+	int pid;
+	int buf_size;
+	int r_buf_size;
+#ifdef DEBUG_GLIO
+	int sum;
+#endif
+	char buffer;
+} __packed;
+
+#define to_virtio_gl_data(a)   ((struct virtio_gl_data *)(a)->private_data)
+
+#ifdef DEBUG_GLIO
+#define SIZE_OUT_HEADER (sizeof(int)*4)
+#define SIZE_IN_HEADER (sizeof(int)*2)
+#else
+#define SIZE_OUT_HEADER (sizeof(int)*3)
+#define SIZE_IN_HEADER sizeof(int)
+#endif
+
+static struct virtqueue *vq;
+
+static void glmem_done(struct virtqueue *vq)
+{
+	struct virtio_gl_data *gldata;
+	int count;
+
+	gldata = virtqueue_get_buf(vq, &count);
+
+	if (!gldata)
+		BUG();
+
+	complete(&gldata->done);
+}
+
+/* This is videobuf_vmalloc_to_sg() from videobuf-dma-sg.c with
+ * some modifications
+ */
+static struct scatterlist *vmalloc_to_sg(struct scatterlist *sg_list,
+				unsigned char *virt, unsigned int pages)
+{
+	struct page *pg;
+
+	/* unaligned */
+	BUG_ON((ulong)virt & ~PAGE_MASK);
+
+	/* Fill with elements for the data */
+	while (pages) {
+		pg = vmalloc_to_page(virt);
+		if (!pg)
+			goto err;
+
+		sg_set_page(sg_list, pg, PAGE_SIZE, 0);
+		virt += PAGE_SIZE;
+		sg_list++;
+		pages--;
+	}
+
+	return sg_list;
+
+err:
+	kfree(sg_list);
+	return NULL;
+}
+
+static int put_data(struct virtio_gl_data *gldata)
+{
+	struct scatterlist *sg, *sg_list;
+	unsigned int ret, o_page, i_page, sg_entries;
+	struct virtio_gl_header *header =
+				(struct virtio_gl_header *)gldata->buffer;
+
+	ret = header->buf_size;
+
+	o_page = (header->buf_size + PAGE_SIZE-1) >> PAGE_SHIFT;
+	i_page = (header->r_buf_size + PAGE_SIZE-1) >> PAGE_SHIFT;
+
+	header->pid = gldata->pid;
+
+	if ((o_page && i_page) &&
+		(o_page > gldata->pages || i_page > gldata->pages)) {
+		i_page = 0;
+	}
+
+	if (o_page > gldata->pages)
+		o_page = gldata->pages;
+
+	if (i_page > gldata->pages)
+		i_page = gldata->pages;
+
+	if (!o_page)
+		o_page = 1;
+
+	sg_entries = o_page + i_page;
+
+	sg_list = kcalloc(sg_entries, sizeof(struct scatterlist), GFP_KERNEL);
+
+	if (!sg_list) {
+		ret = -EIO;
+		goto out;
+	}
+
+	sg_init_table(sg_list, sg_entries);
+
+	sg = vmalloc_to_sg(sg_list, gldata->buffer, o_page);
+	sg = vmalloc_to_sg(sg, gldata->buffer, i_page);
+
+	if (!sg) {
+		ret = -EIO;
+		goto out_free;
+	}
+
+	/* Transfer data */
+	if (virtqueue_add_buf(vq, sg_list, o_page, i_page, gldata) >= 0) {
+		virtqueue_kick(vq);
+		/* Chill out until it's done with the buffer. */
+		wait_for_completion(&gldata->done);
+	}
+
+out_free:
+	kfree(sg_list);
+out:
+	return ret;
+}
+
+static void free_buffer(struct virtio_gl_data *gldata)
+{
+	if (gldata->buffer) {
+		vfree(gldata->buffer);
+		gldata->buffer = NULL;
+	}
+}
+
+static int glmem_open(struct inode *inode, struct file *file)
+{
+	struct virtio_gl_data *gldata = kzalloc(sizeof(struct virtio_gl_data),
+						GFP_KERNEL);
+
+	if (!gldata)
+		return -ENXIO;
+
+	gldata->pid = pid_nr(task_pid(current));
+	init_completion(&gldata->done);
+
+	file->private_data = gldata;
+
+	return 0;
+}
+
+static int glmem_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+	struct virtio_gl_data *gldata = to_virtio_gl_data(filp);
+	int pages = (vma->vm_end - vma->vm_start) / PAGE_SIZE;
+
+	/* Set a reasonable limit */
+	if (pages > 16)
+		return -ENOMEM;
+
+	/* for now, just allow one buffer to be mmap()ed. */
+	if (gldata->buffer)
+		return -EIO;
+
+	gldata->buffer = vmalloc_user(pages*PAGE_SIZE);
+
+	if (!gldata->buffer)
+		return -ENOMEM;
+
+	gldata->pages = pages;
+
+	if (remap_vmalloc_range(vma, gldata->buffer, 0) < 0) {
+		vfree(gldata->buffer);
+		return -EIO;
+	}
+
+	vma->vm_flags |= VM_DONTEXPAND;
+
+	return 0;
+}
+
+static int glmem_fsync(struct file *filp, int datasync)
+{
+	struct virtio_gl_data *gldata = to_virtio_gl_data(filp);
+
+	put_data(gldata);
+
+	return 0;
+}
+
+static int glmem_release(struct inode *inode, struct file *file)
+{
+	struct virtio_gl_data *gldata = to_virtio_gl_data(file);
+
+	if (gldata && gldata->buffer) {
+		struct virtio_gl_header *header =
+				(struct virtio_gl_header *)gldata->buffer;
+
+		/* Make sure the host hears about the process ending / dying */
+		header->pid = gldata->pid;
+		header->buf_size = SIZE_OUT_HEADER + 2;
+		header->r_buf_size = SIZE_IN_HEADER;
+		*(short *)(&header->buffer) = -1;
+
+		put_data(gldata);
+		free_buffer(gldata);
+	}
+
+	kfree(gldata);
+
+	return 0;
+}
+
+static const struct file_operations glmem_fops = {
+	.owner		= THIS_MODULE,
+	.open		= glmem_open,
+	.mmap		= glmem_mmap,
+	.fsync		= glmem_fsync,
+	.release	= glmem_release,
+};
+
+static struct miscdevice glmem_dev = {
+	MISC_DYNAMIC_MINOR,
+	DEVICE_NAME,
+	&glmem_fops
+};
+
+static int glmem_probe(struct virtio_device *vdev)
+{
+	int ret;
+
+	/* We expect a single virtqueue. */
+	vq = virtio_find_single_vq(vdev, glmem_done, "output");
+	if (IS_ERR(vq))
+		return PTR_ERR(vq);
+
+	ret = misc_register(&glmem_dev);
+	if (ret) {
+		printk(KERN_ERR "glmem: cannot register glmem_dev as misc");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static void __devexit glmem_remove(struct virtio_device *vdev)
+{
+	vdev->config->reset(vdev);
+	misc_deregister(&glmem_dev);
+	vdev->config->del_vqs(vdev);
+}
+
+static struct virtio_device_id id_table[] = {
+	{ VIRTIO_ID_GL, VIRTIO_DEV_ANY_ID },
+	{ 0 },
+};
+
+static struct virtio_driver virtio_gl_driver = {
+	.driver	= {
+		.name =  KBUILD_MODNAME,
+		.owner = THIS_MODULE,
+	},
+	.id_table =     id_table,
+	.probe =        glmem_probe,
+	.remove =       __devexit_p(glmem_remove),
+};
+
+static int __init glmem_init(void)
+{
+	return register_virtio_driver(&virtio_gl_driver);
+}
+
+static void __exit glmem_exit(void)
+{
+	unregister_virtio_driver(&virtio_gl_driver);
+}
+
+module_init(glmem_init);
+module_exit(glmem_exit);
+
+MODULE_DEVICE_TABLE(virtio, id_table);
+MODULE_DESCRIPTION("Virtio gl passthrough driver");
+MODULE_LICENSE("GPL v2");
+
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 3d94a14..9a9a6cc 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -16,6 +16,7 @@ source "drivers/char/agp/Kconfig"
 source "drivers/gpu/vga/Kconfig"
 
 source "drivers/gpu/drm/Kconfig"
+source "drivers/gpu/misc/Kconfig"
 
 config VGASTATE
        tristate
diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
index 06660c0..663b496 100644
--- a/include/linux/virtio_ids.h
+++ b/include/linux/virtio_ids.h
@@ -12,6 +12,7 @@
 #define VIRTIO_ID_CONSOLE	3 /* virtio console */
 #define VIRTIO_ID_RNG		4 /* virtio ring */
 #define VIRTIO_ID_BALLOON	5 /* virtio balloon */
+#define VIRTIO_ID_GL		6 /* virtio usermem */
 #define VIRTIO_ID_9P		9 /* 9p virtio console */
 
 #endif /* _LINUX_VIRTIO_IDS_H */

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Avi Kivity]

  On 10/27/2010 03:00 PM, Ian Molton wrote:
> On 19/10/10 11:39, Avi Kivity wrote:
>> On 10/19/2010 12:31 PM, Ian Molton wrote:
>
>>>> 2. should start with a patch to the virtio-pci spec to document what
>>>> you're doing
>>>
>>> Where can I find that spec?
>>
>> http://ozlabs.org/~rusty/virtio-spec/
>
> Ok, but I'm not patching that until theres been some review.

Well, I like to review an implementation against a spec.

>
> There are links to the associated qemu and guest OS changes in my 
> original email.
>
>>> It doesnt, at present... It could be changed fairly easily ithout
>>> breaking anything if that happens though.
>>
>> The hypervisor and the guest can be changed independently. The driver
>> should be coded so that it doesn't depend on hypervisor implementation
>> details.
>
> Fixed - updated patch tested and attached.
> +
> +	/* Transfer data */
> +	if (virtqueue_add_buf(vq, sg_list, o_page, i_page, gldata)>= 0) {
> +		virtqueue_kick(vq);
> +		/* Chill out until it's done with the buffer. */
> +		wait_for_completion(&gldata->done);
> +	}
> +

Better, but still unsatisfying.  If the server is busy, the caller would 
block.  I guess it's expected since it's called from ->fsync().  I'm not 
sure whether that's the best interface, perhaps aio_writev is better.

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

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Ian Molton]

On 28/10/10 10:27, Avi Kivity wrote:
> On 10/27/2010 03:00 PM, Ian Molton wrote:
>> On 19/10/10 11:39, Avi Kivity wrote:
>>> On 10/19/2010 12:31 PM, Ian Molton wrote:
>>
>>>>> 2. should start with a patch to the virtio-pci spec to document what
>>>>> you're doing
>>>>
>>>> Where can I find that spec?
>>>
>>> http://ozlabs.org/~rusty/virtio-spec/
>>
>> Ok, but I'm not patching that until theres been some review.
>
> Well, I like to review an implementation against a spec.

True, but then all that would prove is that I can write a spec to match 
the code.

The code is proof of concept. the kernel bit is pretty simple, but I'd 
like to get some idea of whether the rest of the code will be accepted 
given that theres not much point in having any one (or two) of these 
components exist without the other.

> Better, but still unsatisfying. If the server is busy, the caller would
> block. I guess it's expected since it's called from ->fsync(). I'm not
> sure whether that's the best interface, perhaps aio_writev is better.

The caller is intended to block as the host must perform GL rendering 
before allowing the guests process to continue.

The only real bottleneck is that processes will block trying to submit 
data if another process is performing rendering, but that will only be 
solved when the renderer is made multithreaded. The same would happen on 
a real GPU if it had only one queue too.

If you look at the host code, you can see that the data is already 
buffered per-process, in a pretty sensible way. if the renderer itself 
were made a seperate thread, then this problem magically disappears (the 
queuing code on the host is pretty fast).

In testing, the overhead of this was pretty small anyway. Running a few 
dozen glxgears and a copy of ioquake3 simultaneously on an intel video 
card managed the same framerate with the same CPU utilisation, both with 
the old code and the version I just posted. Contention during rendering 
just isn't much of an issue.

-Ian

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Avi Kivity]

  On 10/28/2010 01:54 PM, Ian Molton wrote:
>> Well, I like to review an implementation against a spec.
>
>
> True, but then all that would prove is that I can write a spec to 
> match the code.

It would also allow us to check that the spec matches the requirements.  
Those two steps are easier than checking that the code matches the 
requirements.

> The code is proof of concept. the kernel bit is pretty simple, but I'd 
> like to get some idea of whether the rest of the code will be accepted 
> given that theres not much point in having any one (or two) of these 
> components exist without the other.

I guess some graphics people need to be involved.

>
>> Better, but still unsatisfying. If the server is busy, the caller would
>> block. I guess it's expected since it's called from ->fsync(). I'm not
>> sure whether that's the best interface, perhaps aio_writev is better.
>
> The caller is intended to block as the host must perform GL rendering 
> before allowing the guests process to continue.

Why is that?  Can't we pipeline the process?

>
> The only real bottleneck is that processes will block trying to submit 
> data if another process is performing rendering, but that will only be 
> solved when the renderer is made multithreaded. The same would happen 
> on a real GPU if it had only one queue too.
>
> If you look at the host code, you can see that the data is already 
> buffered per-process, in a pretty sensible way. if the renderer itself 
> were made a seperate thread, then this problem magically disappears 
> (the queuing code on the host is pretty fast).

Well, this is out of my area of expertise.  I don't like it, but if it's 
acceptable to the gpu people, okay.

>
> In testing, the overhead of this was pretty small anyway. Running a 
> few dozen glxgears and a copy of ioquake3 simultaneously on an intel 
> video card managed the same framerate with the same CPU utilisation, 
> both with the old code and the version I just posted. Contention 
> during rendering just isn't much of an issue.

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

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Anthony Liguori]

On 10/28/2010 09:24 AM, Avi Kivity wrote:
>  On 10/28/2010 01:54 PM, Ian Molton wrote:
>>> Well, I like to review an implementation against a spec.
>>
>>
>> True, but then all that would prove is that I can write a spec to 
>> match the code.
>
> It would also allow us to check that the spec matches the 
> requirements.  Those two steps are easier than checking that the code 
> matches the requirements.

I'm extremely sceptical of any GL passthrough proposal.  There have 
literally been half a dozen over the years and they never seem to leave 
proof-of-concept phase.  My (limited) understanding is that it's a 
fundamentally hard problem that no one has adequately solved yet.

A specifically matters an awful lot less than an explanation of how the 
problem is being solved in a robust fashion such that it can be reviewed 
by people with a deeper understanding of the problem space.

Regards,

Anthony Liguori

>> The code is proof of concept. the kernel bit is pretty simple, but 
>> I'd like to get some idea of whether the rest of the code will be 
>> accepted given that theres not much point in having any one (or two) 
>> of these components exist without the other.
>
> I guess some graphics people need to be involved.
>
>>
>>> Better, but still unsatisfying. If the server is busy, the caller would
>>> block. I guess it's expected since it's called from ->fsync(). I'm not
>>> sure whether that's the best interface, perhaps aio_writev is better.
>>
>> The caller is intended to block as the host must perform GL rendering 
>> before allowing the guests process to continue.
>
> Why is that?  Can't we pipeline the process?
>
>>
>> The only real bottleneck is that processes will block trying to 
>> submit data if another process is performing rendering, but that will 
>> only be solved when the renderer is made multithreaded. The same 
>> would happen on a real GPU if it had only one queue too.
>>
>> If you look at the host code, you can see that the data is already 
>> buffered per-process, in a pretty sensible way. if the renderer 
>> itself were made a seperate thread, then this problem magically 
>> disappears (the queuing code on the host is pretty fast).
>
> Well, this is out of my area of expertise.  I don't like it, but if 
> it's acceptable to the gpu people, okay.
>
>>
>> In testing, the overhead of this was pretty small anyway. Running a 
>> few dozen glxgears and a copy of ioquake3 simultaneously on an intel 
>> video card managed the same framerate with the same CPU utilisation, 
>> both with the old code and the version I just posted. Contention 
>> during rendering just isn't much of an issue.
>

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Ian Molton]

On 28/10/10 15:43, Anthony Liguori wrote:
> On 10/28/2010 09:24 AM, Avi Kivity wrote:
>> On 10/28/2010 01:54 PM, Ian Molton wrote:
>
>>> True, but then all that would prove is that I can write a spec to
>>> match the code.
>>
>> It would also allow us to check that the spec matches the
>> requirements. Those two steps are easier than checking that the code
>> matches the requirements.

There was no formal spec for this. The code was written to replace nasty 
undefined-instruction based data transport hacks in the (already 
existing) GL passthrough code.

> I'm extremely sceptical of any GL passthrough proposal. There have
> literally been half a dozen over the years and they never seem to leave
> proof-of-concept phase. My (limited) understanding is that it's a
> fundamentally hard problem that no one has adequately solved yet.

The code in this case has been presented as a patch to qemu nearly 3 
years ago. I've taken the patches and refactored them to use virtio 
rather than an undefined instruction (which fails under KVM, unlike my 
approach).

Its in use testing meego images and seems to be fairly reliable. it can 
handle compositing window managers, games, video, etc. We're currently 
supporting OpenGL 1.4 including shaders.

> A specifically matters an awful lot less than an explanation of how the
> problem is being solved in a robust fashion such that it can be reviewed
> by people with a deeper understanding of the problem space.

I'm not sure there is a way to prevent nefarious tasks upsetting the 
hosts OpenGL with carefully crafted strings of commands, short of 
inspecting every single command, which is insane.

Really this needs to be done at a lower level by presenting a virtual 
GPU to the guest OS but I am not in a position to code that right now.

The code as it is is useful, and can always be superceeded by a virtual 
GPU implementation in future.

At least this breaks the chicken / egg cycle of people wanting GL 
support on virtual machines, but not writing stuff to take advantage of 
it because the support isn't there. its also a neatly encapsulated 
solution - if you dont want people to have access to the passthrough, 
simply tell qemu not to present the virtio-gl device to the guest, via 
qemus existing commandline options.

If this code was invasive to qemus core, I'd say 'no way' but its just 
not. and as the GL device is versioned, we can keep using it even if the 
passthrough is replaced by a virtual GPU.

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Anthony Liguori]

On 10/28/2010 02:50 PM, Ian Molton wrote:
> On 28/10/10 15:43, Anthony Liguori wrote:
>> On 10/28/2010 09:24 AM, Avi Kivity wrote:
>>> On 10/28/2010 01:54 PM, Ian Molton wrote:
>>
>>>> True, but then all that would prove is that I can write a spec to
>>>> match the code.
>>>
>>> It would also allow us to check that the spec matches the
>>> requirements. Those two steps are easier than checking that the code
>>> matches the requirements.
>
> There was no formal spec for this. The code was written to replace 
> nasty undefined-instruction based data transport hacks in the (already 
> existing) GL passthrough code.
>
>> I'm extremely sceptical of any GL passthrough proposal. There have
>> literally been half a dozen over the years and they never seem to leave
>> proof-of-concept phase. My (limited) understanding is that it's a
>> fundamentally hard problem that no one has adequately solved yet.
>
> The code in this case has been presented as a patch to qemu nearly 3 
> years ago. I've taken the patches and refactored them to use virtio 
> rather than an undefined instruction (which fails under KVM, unlike my 
> approach).
>
> Its in use testing meego images and seems to be fairly reliable. it 
> can handle compositing window managers, games, video, etc. We're 
> currently supporting OpenGL 1.4 including shaders.
>
>> A specifically matters an awful lot less than an explanation of how the
>> problem is being solved in a robust fashion such that it can be reviewed
>> by people with a deeper understanding of the problem space.
>
> I'm not sure there is a way to prevent nefarious tasks upsetting the 
> hosts OpenGL with carefully crafted strings of commands, short of 
> inspecting every single command, which is insane.
>
> Really this needs to be done at a lower level by presenting a virtual 
> GPU to the guest OS but I am not in a position to code that right now.
>
> The code as it is is useful, and can always be superceeded by a 
> virtual GPU implementation in future.
>
> At least this breaks the chicken / egg cycle of people wanting GL 
> support on virtual machines, but not writing stuff to take advantage 
> of it because the support isn't there. its also a neatly encapsulated 
> solution - if you dont want people to have access to the passthrough, 
> simply tell qemu not to present the virtio-gl device to the guest, via 
> qemus existing commandline options.
>
> If this code was invasive to qemus core, I'd say 'no way' but its just 
> not. and as the GL device is versioned, we can keep using it even if 
> the passthrough is replaced by a virtual GPU.

The virtio-gl implementation is basically duplicating virtio-serial.  It 
looks like ti creates a totally separate window for the GL session.    
In the current form, is there really any advantage to having the code in 
QEMU?  It could just as easily live outside of QEMU.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Ian Molton]

On 28/10/10 21:14, Anthony Liguori wrote:

>> If this code was invasive to qemus core, I'd say 'no way' but its just
>> not. and as the GL device is versioned, we can keep using it even if
>> the passthrough is replaced by a virtual GPU.
>
> The virtio-gl implementation is basically duplicating virtio-serial. It
> looks like ti creates a totally separate window for the GL session. In
> the current form, is there really any advantage to having the code in
> QEMU? It could just as easily live outside of QEMU.

you could say much the same about any driver in qemu... you could 
serialise up the registers and ship the data off to be processed on 
another PC if you wanted to...

The code does not, however, create a seperate window for the GL session. 
the GL scene is rendered offscreen, and then piped back to the guest for 
display, so that it is fully composited into the guests graphical 
environment. From a user perspective, its as if the guest had hardware 
3D. Performance is very reasonable, around 40fps in ioquake3 on modest 
(host) hardware.

In theory, the code *could* use a serial transport and render in a 
seperate process, but then that would make it much harder to evolve it 
into a GPU-like system in future.

-Ian

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Ian Molton]

On 28/10/10 15:24, Avi Kivity wrote:
>> The caller is intended to block as the host must perform GL rendering
>> before allowing the guests process to continue.
>
> Why is that?  Can't we pipeline the process?

No, not really. the guest may call for the scene to be rendered at any 
time and we have to wait for that to happen before we can return the 
data to it.

-Ian

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Avi Kivity]

  On 10/28/2010 03:52 PM, Ian Molton wrote:
> On 28/10/10 15:24, Avi Kivity wrote:
>>> The caller is intended to block as the host must perform GL rendering
>>> before allowing the guests process to continue.
>>
>> Why is that?  Can't we pipeline the process?
>
> No, not really. the guest may call for the scene to be rendered at any 
> time and we have to wait for that to happen before we can return the 
> data to it.

Waiting for a response is fine, but can't the guest issue a second batch 
while waiting for the first?

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

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Anthony Liguori]

On 11/01/2010 05:42 AM, Avi Kivity wrote:
>  On 10/28/2010 03:52 PM, Ian Molton wrote:
>> On 28/10/10 15:24, Avi Kivity wrote:
>>>> The caller is intended to block as the host must perform GL rendering
>>>> before allowing the guests process to continue.
>>>
>>> Why is that?  Can't we pipeline the process?
>>
>> No, not really. the guest may call for the scene to be rendered at 
>> any time and we have to wait for that to happen before we can return 
>> the data to it.
>
> Waiting for a response is fine, but can't the guest issue a second 
> batch while waiting for the first?

In a threaded application I think you mean but all RPCs are dispatched 
holding a global lock so even within a threaded application, only a 
single GL call will be executed at a time.

The other scenario would be multiple applications trying to use GL but 
AFAICT, this is not supported in the current model.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Ian Molton]

On 01/11/10 13:21, Anthony Liguori wrote:
> On 11/01/2010 05:42 AM, Avi Kivity wrote:
>> On 10/28/2010 03:52 PM, Ian Molton wrote:
>>> On 28/10/10 15:24, Avi Kivity wrote:

>> Waiting for a response is fine, but can't the guest issue a second
>> batch while waiting for the first?

> The other scenario would be multiple applications trying to use GL but
> AFAICT, this is not supported in the current model.

It very much is. It supports fully visually integrated rendering (no 
overlay windows) and even compositing GL window managers work fine, even 
if running 3D apps under them.

-Ian

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Anthony Liguori]

On 11/01/2010 10:49 AM, Ian Molton wrote:
> On 01/11/10 13:21, Anthony Liguori wrote:
>> On 11/01/2010 05:42 AM, Avi Kivity wrote:
>>> On 10/28/2010 03:52 PM, Ian Molton wrote:
>>>> On 28/10/10 15:24, Avi Kivity wrote:
>
>>> Waiting for a response is fine, but can't the guest issue a second
>>> batch while waiting for the first?
>
>> The other scenario would be multiple applications trying to use GL but
>> AFAICT, this is not supported in the current model.
>
> It very much is. It supports fully visually integrated rendering (no 
> overlay windows) and even compositing GL window managers work fine, 
> even if running 3D apps under them.

Does the kernel track userspace pid and pass that information to qemu?

Regards,

Anthony Liguori

> -Ian

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Ian Molton]

On 01/11/10 15:57, Anthony Liguori wrote:

>> It very much is. It supports fully visually integrated rendering (no
>> overlay windows) and even compositing GL window managers work fine,
>> even if running 3D apps under them.
>
> Does the kernel track userspace pid and pass that information to qemu?

Yes. And the qemu code tracks the PIDs and keeps multiple queues (one 
per pid).

-Ian

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Ian Molton]

On 01/11/10 10:42, Avi Kivity wrote:
>> No, not really. the guest may call for the scene to be rendered at
>> any time and we have to wait for that to happen before we can
>> return the data to it.
>
> Waiting for a response is fine, but can't the guest issue a second
> batch while waiting for the first?

No. Userspace doesnt do or expect that.

Multiple applications work fine, however.

-Ian

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Rusty Russell]

On Wed, 27 Oct 2010 11:30:31 pm Ian Molton wrote:
> On 19/10/10 11:39, Avi Kivity wrote:
> > On 10/19/2010 12:31 PM, Ian Molton wrote:
> 
> >>> 2. should start with a patch to the virtio-pci spec to document what
> >>> you're doing
> >>
> >> Where can I find that spec?
> >
> > http://ozlabs.org/~rusty/virtio-spec/
> 
> Ok, but I'm not patching that until theres been some review.

Fair enough; it's a bit of a PITA to patch, so it makes sense to get the
details nailed down first.

> There are links to the associated qemu and guest OS changes in my 
> original email.
> 
> >> It doesnt, at present... It could be changed fairly easily ithout
> >> breaking anything if that happens though.
> >
> > The hypervisor and the guest can be changed independently. The driver
> > should be coded so that it doesn't depend on hypervisor implementation
> > details.
> 
> Fixed - updated patch tested and attached.

OK. FWIW, I think this is an awesome idea.  I understand others are skeptical,
but this seems simple and if it works and you're happy to maintain it I'm
happy to let you do it :)

Cheers,
Rusty.

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Ed Tomlinson]

On Friday 29 October 2010 07:18:00 Rusty Russell wrote:
> On Wed, 27 Oct 2010 11:30:31 pm Ian Molton wrote:
> > On 19/10/10 11:39, Avi Kivity wrote:
> > > On 10/19/2010 12:31 PM, Ian Molton wrote:
> > 
> > >>> 2. should start with a patch to the virtio-pci spec to document what
> > >>> you're doing
> > >>
> > >> Where can I find that spec?
> > >
> > > http://ozlabs.org/~rusty/virtio-spec/
> > 
> > Ok, but I'm not patching that until theres been some review.
> 
> Fair enough; it's a bit of a PITA to patch, so it makes sense to get the
> details nailed down first.
> 
> > There are links to the associated qemu and guest OS changes in my 
> > original email.
> > 
> > >> It doesnt, at present... It could be changed fairly easily ithout
> > >> breaking anything if that happens though.
> > >
> > > The hypervisor and the guest can be changed independently. The driver
> > > should be coded so that it doesn't depend on hypervisor implementation
> > > details.
> > 
> > Fixed - updated patch tested and attached.
> 
> OK. FWIW, I think this is an awesome idea.  I understand others are skeptical,
> but this seems simple and if it works and you're happy to maintain it I'm
> happy to let you do it :)

From a kvm user's perspective this is a GREAT idea.  Looks like Ian and friends have
though about better solutions down the line and have made their code easy
to superceed.  If this is really the case I vote to get this in qemu/kvm asap -
software opengl can be a real pain.

Thanks
Ed Tomlinson

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Anthony Liguori]

On 10/29/2010 06:18 AM, Rusty Russell wrote:
>> Fixed - updated patch tested and attached.
>>      
> OK. FWIW, I think this is an awesome idea.

Paravirtual OpenGL or the actual proposed implementation?  Have you 
looked at the actual code?

If I understand correctly, the implementation is an RPC of opengl 
commands across virtio that are then rendered on the host to an 
offscreen buffer.  The buffer is then sent back to the guest in rendered 
form.

But it's possible to mess around with other guests because the opengl 
commands are not scrubbed. There have been other proposals in the past 
that include a mini JIT that would translate opengl commands into a safe 
form.

Exposing just an opengl RPC transport doesn't seem that useful either.  
It ought to be part of a VGA card in order for it to be integrated with 
the rest of the graphics system without a round trip.

The alternative proposal is Spice which so far noone has mentioned.  
Right now, Spice seems to be taking the right approach to guest 3d support.

Regards,

Anthony Liguori

>    I understand others are skeptical,
> but this seems simple and if it works and you're happy to maintain it I'm
> happy to let you do it :)
>
> Cheers,
> Rusty.
>
>    

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Alon Levy]

----- "Anthony Liguori" <anthony@codemonkey.ws> wrote:

> On 10/29/2010 06:18 AM, Rusty Russell wrote:
> >> Fixed - updated patch tested and attached.
> >>      
> > OK. FWIW, I think this is an awesome idea.
> 
> Paravirtual OpenGL or the actual proposed implementation?  Have you 
> looked at the actual code?
> 
> If I understand correctly, the implementation is an RPC of opengl 
> commands across virtio that are then rendered on the host to an 
> offscreen buffer.  The buffer is then sent back to the guest in
> rendered 
> form.
> 
> But it's possible to mess around with other guests because the opengl
> 
> commands are not scrubbed. There have been other proposals in the past
> 
> that include a mini JIT that would translate opengl commands into a
> safe 
> form.
> 
> Exposing just an opengl RPC transport doesn't seem that useful either.
>  
> It ought to be part of a VGA card in order for it to be integrated
> with 
> the rest of the graphics system without a round trip.
> 
> The alternative proposal is Spice which so far noone has mentioned.  
> Right now, Spice seems to be taking the right approach to guest 3d
> support.
> 

While we (speaking as part of the SPICE developers) want to have the same
support in our virtual GPU for 3d as we have for 2d, we just don't at this
point of time.

> Regards,
> 
> Anthony Liguori
> 
> >    I understand others are skeptical,
> > but this seems simple and if it works and you're happy to maintain
> it I'm
> > happy to let you do it :)
> >
> > Cheers,
> > Rusty.
> >
> >

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Anthony Liguori]

On 11/01/2010 06:53 AM, Alon Levy wrote:
>> The alternative proposal is Spice which so far noone has mentioned.
>> Right now, Spice seems to be taking the right approach to guest 3d
>> support.
>>
>>      
> While we (speaking as part of the SPICE developers) want to have the same
> support in our virtual GPU for 3d as we have for 2d, we just don't at this
> point of time.
>    

Yes, but I think the point is that are two general approaches to 
supporting 3d that are being proposed.  One approach is to an RPC layer 
at the OpenGL level which essentially passes through the host OpenGL 
stack.  That's what virtio-gl is.  This has existed for quite a while 
and there are multiple transports for it.  It supports serial ports, TCP 
sockets, a custom ISA extension for x86, and now a custom virtio transport.

Another approach would be to have a virtual GPU and to implement 
GPU-level commands for 3d.  I have been repeated told that much of the 
complexity of Spice is absolutely needed for 3d and that that's a major 
part of the design.

GPU-level support for 3d operations has a number of advantages mainly 
that it's more reasonably portable to things like Windows since the 3d 
commands can be a superset of both OpenGL and Direct3d.

Also, Spice has an abstraction layer that doesn't simply passthrough 
graphics commands, but translates/sanitizes them first.   That's another 
advantage over OpenGL passthrough.  Without a layer to sanitize 
commands, a guest can do funky things with the host or other guests.

I think a Spice-like approach is the best thing long term.  In the short 
term, I think doing the GL marshalling over virtio-serial makes a ton of 
sense since the kernel driver is already present upstream.  It exists 
exactly for things like this.

In the very, very short term, I think an external backend to QEMU also 
makes a lot of sense because that's something that Just Works today.  I 
think we can consider integrating it into QEMU (or at least simplifying 
the execution of the backend) but integrating into QEMU is going to 
require an awful lot of the existing code to be rewritten.  Keeping it 
separate has the advantage of allowing something to Just Work as an 
interim solution as we wait for proper support in Spice.

Regards,

Anthony Liguori

>    
>> Regards,
>>
>> Anthony Liguori
>>
>>      
>>>     I understand others are skeptical,
>>> but this seems simple and if it works and you're happy to maintain
>>>        
>> it I'm
>>      
>>> happy to let you do it :)
>>>
>>> Cheers,
>>> Rusty.
>>>
>>>
>>>        

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Ian Molton]

On 01/11/10 13:28, Anthony Liguori wrote:
> On 11/01/2010 06:53 AM, Alon Levy wrote:

>> While we (speaking as part of the SPICE developers) want to have the same
>> support in our virtual GPU for 3d as we have for 2d, we just don't at
>> this point of time.

Would it be helpful to you to have /something/ that works in the 
interim? I'm happy to work with you guys so that we dont need to 
reinvent the wheel ;-)

> Yes, but I think the point is that are two general approaches to
> supporting 3d that are being proposed. One approach is to an RPC layer
> at the OpenGL level which essentially passes through the host OpenGL
> stack. That's what virtio-gl is. This has existed for quite a while and
> there are multiple transports for it.

Well, sort of. this version is heavily modified and only the virtio 
transport is supported in it. Its quite a large code cleanup.

> It supports serial ports, TCP sockets, a custom ISA extension for x86,

The custom ISA idea is cut too since it fails to play nice with KVM. 
Virtio-gl offers better security too.

> Another approach would be to have a virtual GPU and to implement
> GPU-level commands for 3d. I have been repeated told that much of the
> complexity of Spice is absolutely needed for 3d and that that's a major
> part of the design.

I've not seen any implementations of this type that are even close to 
useable.

> GPU-level support for 3d operations has a number of advantages mainly
> that it's more reasonably portable to things like Windows since the 3d
> commands can be a superset of both OpenGL and Direct3d.

Agreed.

> Also, Spice has an abstraction layer that doesn't simply passthrough
> graphics commands, but translates/sanitizes them first. That's another
> advantage over OpenGL passthrough.

I'm not sure that thats actually needed if you are careful about what 
commands you implement on the virtual GPU (and how)

> Without a layer to sanitize commands,
> a guest can do funky things with the host or other guests.

It shouldnt be possible for the guest to hurt other guests actual GL 
rendering (or the hosts) as each PID is handed a context (or contexts) 
of its own. It is possible for the guest to cause the host problems 
though. this is a design flaw in the whole idea of GL RPC. It *isnt* 
reasonable to make it secure, but it certainly is useful right now since 
theres no alternative available.

> I think a Spice-like approach is the best thing long term. In the short
> term, I think doing the GL marshalling over virtio-serial makes a ton of
> sense since the kernel driver is already present upstream. It exists
> exactly for things like this.

The virtio driver enfoces the PID field and understands the packet 
format used. Its better than using serial. Its also just one driver - 
which doesnt have any special interdependencies and can be extended or 
got rid of in future if and when better things come along.

> In the very, very short term, I think an external backend to QEMU also
> makes a lot of sense because that's something that Just Works today.

Whos written that? The 2007 patch I've been working on and updating 
simply fails to work altogether without huge alterations on current qemu.

My current patch touches a tiny part of the qemu sources. It works today.

> I
> think we can consider integrating it into QEMU (or at least simplifying
> the execution of the backend) but integrating into QEMU is going to
> require an awful lot of the existing code to be rewritten. Keeping it
> separate has the advantage of allowing something to Just Work as an
> interim solution as we wait for proper support in Spice.

I dont know why you think integrating it into qemu is hard? I've already 
done it. I added one virtio driver and a seperate offscreen renderer. it 
touches the qemu code in *one* place. There should be no need to rewrite 
anything.

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Anthony Liguori]

On 11/03/2010 01:03 PM, Ian Molton wrote:
>
> The virtio driver enfoces the PID field and understands the packet 
> format used. Its better than using serial. Its also just one driver - 
> which doesnt have any special interdependencies and can be extended or 
> got rid of in future if and when better things come along.

Why is it better than using virtio-serial?

>
>> In the very, very short term, I think an external backend to QEMU also
>> makes a lot of sense because that's something that Just Works today.
>
> Whos written that? The 2007 patch I've been working on and updating 
> simply fails to work altogether without huge alterations on current qemu.
>
> My current patch touches a tiny part of the qemu sources. It works today.

But it's not at all mergable in the current form.  If you want to do the 
work of getting it into a mergable state (cleaning up the coding style, 
moving it to hw/, etc.) than I'm willing to consider it.  But I don't 
think a custom virtio transport is the right thing to do here.

However, if you want something that Just Works with the least amount of 
code possible, just split it into a separate process and we can stick it 
in a contrib/ directory or something.

>
>> I
>> think we can consider integrating it into QEMU (or at least simplifying
>> the execution of the backend) but integrating into QEMU is going to
>> require an awful lot of the existing code to be rewritten. Keeping it
>> separate has the advantage of allowing something to Just Work as an
>> interim solution as we wait for proper support in Spice.
>
> I dont know why you think integrating it into qemu is hard? I've 
> already done it. 

Adding a file that happens to compile as part of qemu even though it 
doesn't actually integrate with qemu in any meaningful way is not 
integrating.  That's just build system manipulation.

Regards,

Anthony Liguori

> I added one virtio driver and a seperate offscreen renderer. it 
> touches the qemu code in *one* place. There should be no need to 
> rewrite anything.

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Ian Molton]

On 03/11/10 18:17, Anthony Liguori wrote:
> On 11/03/2010 01:03 PM, Ian Molton wrote:

> Why is it better than using virtio-serial?

For one thing, it enforces the PID in kernel so the guests processes 
cant screw each other over by forging the PID passed to qemu.

>> My current patch touches a tiny part of the qemu sources. It works today.
>
> But it's not at all mergable in the current form. If you want to do the
> work of getting it into a mergable state (cleaning up the coding style,
> moving it to hw/, etc.) than I'm willing to consider it. But I don't
> think a custom virtio transport is the right thing to do here.

Hm, I thought I'd indented everything in qemus odd way... Is there a 
codingstyle document or a checkpatch-like tool for qemu?

I'm happy to make the code meet qemus coding style.

> However, if you want something that Just Works with the least amount of
> code possible, just split it into a separate process and we can stick it
> in a contrib/ directory or something.

I dont see what splitting it into a seperate process buys us given we 
still need the virtio-gl driver in order to enforce the PID. The virtio 
driver is probably quite a bit more efficient at marshalling the data 
too, given that it was designed alongside the userspace code.

>>> I
>>> think we can consider integrating it into QEMU (or at least simplifying
>>> the execution of the backend) but integrating into QEMU is going to
>>> require an awful lot of the existing code to be rewritten.

Why? aside from codingstyle, whats massively wrong with it thats going 
to demand a total rewrite?

>>> Keeping it
>>> separate has the advantage of allowing something to Just Work as an
>>> interim solution as we wait for proper support in Spice.

Why does keeping it seperate make life easier? qemu is in a git repo. 
when the time comes, if it reall is a total replacement, git-rm will 
nuke it all.

>> I dont know why you think integrating it into qemu is hard? I've
>> already done it.
>
> Adding a file that happens to compile as part of qemu even though it
> doesn't actually integrate with qemu in any meaningful way is not
> integrating. That's just build system manipulation.

Uh? Either it compiles and works as part of qemu (which it does) or it 
doesnt. How is that not integrated? I've added it to the configure 
script too.

-Ian

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Ian Molton]

Ping ?

On 05/11/10 18:05, Ian Molton wrote:
> On 03/11/10 18:17, Anthony Liguori wrote:
>> On 11/03/2010 01:03 PM, Ian Molton wrote:
>
>> Why is it better than using virtio-serial?
>
> For one thing, it enforces the PID in kernel so the guests processes
> cant screw each other over by forging the PID passed to qemu.
>
>>> My current patch touches a tiny part of the qemu sources. It works
>>> today.
>>
>> But it's not at all mergable in the current form. If you want to do the
>> work of getting it into a mergable state (cleaning up the coding style,
>> moving it to hw/, etc.) than I'm willing to consider it. But I don't
>> think a custom virtio transport is the right thing to do here.
>
> Hm, I thought I'd indented everything in qemus odd way... Is there a
> codingstyle document or a checkpatch-like tool for qemu?
>
> I'm happy to make the code meet qemus coding style.
>
>> However, if you want something that Just Works with the least amount of
>> code possible, just split it into a separate process and we can stick it
>> in a contrib/ directory or something.
>
> I dont see what splitting it into a seperate process buys us given we
> still need the virtio-gl driver in order to enforce the PID. The virtio
> driver is probably quite a bit more efficient at marshalling the data
> too, given that it was designed alongside the userspace code.
>
>>>> I
>>>> think we can consider integrating it into QEMU (or at least simplifying
>>>> the execution of the backend) but integrating into QEMU is going to
>>>> require an awful lot of the existing code to be rewritten.
>
> Why? aside from codingstyle, whats massively wrong with it thats going
> to demand a total rewrite?
>
>>>> Keeping it
>>>> separate has the advantage of allowing something to Just Work as an
>>>> interim solution as we wait for proper support in Spice.
>
> Why does keeping it seperate make life easier? qemu is in a git repo.
> when the time comes, if it reall is a total replacement, git-rm will
> nuke it all.
>
>>> I dont know why you think integrating it into qemu is hard? I've
>>> already done it.
>>
>> Adding a file that happens to compile as part of qemu even though it
>> doesn't actually integrate with qemu in any meaningful way is not
>> integrating. That's just build system manipulation.
>
> Uh? Either it compiles and works as part of qemu (which it does) or it
> doesnt. How is that not integrated? I've added it to the configure
> script too.
>
> -Ian
>

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Anthony Liguori]

On 11/10/2010 11:22 AM, Ian Molton wrote:
> Ping ?

I think the best way forward is to post patches.

To summarize what I was trying to express in the thread, I think this is 
not the right long term architecture but am not opposed to it as a short 
term solution.  I think having a new virtio device is a bad design 
choice but am not totally opposed to it.

My advice is that using virtio-serial + an external tool is probably the 
least amount of work to get something working and usable with QEMU.  If 
you want to go for the path of integration, you're going to have to fix 
all of the coding style issues and make the code fit into QEMU.  
Dropping a bunch of junk into target-i386/ is not making the code fit 
into QEMU.

If you post just what you have now in patch form, I can try to provide 
more concrete advice ignoring the coding style problems.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Ian Molton]

On 10/11/10 17:47, Anthony Liguori wrote:
> On 11/10/2010 11:22 AM, Ian Molton wrote:
>> Ping ?
>
> I think the best way forward is to post patches.

I posted links to the git trees. I can post patches, but they are 
*large*. Do you really want me to post them?

> To summarize what I was trying to express in the thread, I think this is
> not the right long term architecture but am not opposed to it as a short
> term solution. I think having a new virtio device is a bad design choice
> but am not totally opposed to it.

Ok! (I agree (that this should be a short term solution) :) )

> you want to go for the path of integration, you're going to have to fix
> all of the coding style issues and make the code fit into QEMU. Dropping
> a bunch of junk into target-i386/ is not making the code fit into QEMU.

I agree. how about hw/gl for the renderer and hw/ for the virtio module?

> If you post just what you have now in patch form, I can try to provide
> more concrete advice ignoring the coding style problems.

I can post patches, although I dont think LKML would appreciate the 
volume! I can post them to the qemu list if you do.

-Ian

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Anthony Liguori]

On 11/12/2010 06:14 AM, Ian Molton wrote:
> On 10/11/10 17:47, Anthony Liguori wrote:
>> On 11/10/2010 11:22 AM, Ian Molton wrote:
>>> Ping ?
>>
>> I think the best way forward is to post patches.
>
> I posted links to the git trees. I can post patches, but they are 
> *large*. Do you really want me to post them?

Yes, and they have to be split up into something reviewable.

>
>> To summarize what I was trying to express in the thread, I think this is
>> not the right long term architecture but am not opposed to it as a short
>> term solution. I think having a new virtio device is a bad design choice
>> but am not totally opposed to it.
>
> Ok! (I agree (that this should be a short term solution) :) )
>
>> you want to go for the path of integration, you're going to have to fix
>> all of the coding style issues and make the code fit into QEMU. Dropping
>> a bunch of junk into target-i386/ is not making the code fit into QEMU.
>
> I agree. how about hw/gl for the renderer and hw/ for the virtio module?

That would be fine.

>> If you post just what you have now in patch form, I can try to provide
>> more concrete advice ignoring the coding style problems.
>
> I can post patches, although I dont think LKML would appreciate the 
> volume! I can post them to the qemu list if you do.

Yes, qemu is where I was suggesting you post them.

Regards,

Anthony Liguori

> -Ian

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Alon Levy]

On Wed, Nov 03, 2010 at 06:03:50PM +0000, Ian Molton wrote:
> On 01/11/10 13:28, Anthony Liguori wrote:
> >On 11/01/2010 06:53 AM, Alon Levy wrote:
> 
> >>While we (speaking as part of the SPICE developers) want to have the same
> >>support in our virtual GPU for 3d as we have for 2d, we just don't at
> >>this point of time.
> 
> Would it be helpful to you to have /something/ that works in the
> interim? I'm happy to work with you guys so that we dont need to
> reinvent the wheel ;-)
> 

In case it wasn't clear, I think putting virtio-gl is a good idea, exactly because it works right now.

[Snip]

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Ian Molton]

On 04/11/10 09:13, Alon Levy wrote:
> On Wed, Nov 03, 2010 at 06:03:50PM +0000, Ian Molton wrote:
>> On 01/11/10 13:28, Anthony Liguori wrote:
>>> On 11/01/2010 06:53 AM, Alon Levy wrote:
>>
>>>> While we (speaking as part of the SPICE developers) want to have the same
>>>> support in our virtual GPU for 3d as we have for 2d, we just don't at
>>>> this point of time.
>>
>> Would it be helpful to you to have /something/ that works in the
>> interim? I'm happy to work with you guys so that we dont need to
>> reinvent the wheel ;-)
>
> In case it wasn't clear, I think putting virtio-gl is a good idea, exactly because it works right now.

Thanks :)

-Ian

Re: [Qemu-devel] Re: [PATCH] Implement a virtio GPU transport

[Ian Molton]

On 29/10/10 12:18, Rusty Russell wrote:
> On Wed, 27 Oct 2010 11:30:31 pm Ian Molton wrote:
>> On 19/10/10 11:39, Avi Kivity wrote:
>>> On 10/19/2010 12:31 PM, Ian Molton wrote:
>>
>>>>> 2. should start with a patch to the virtio-pci spec to document what
>>>>> you're doing
>>>>
>>>> Where can I find that spec?
>>>
>>> http://ozlabs.org/~rusty/virtio-spec/
>>
>> Ok, but I'm not patching that until theres been some review.
>
> Fair enough; it's a bit of a PITA to patch, so it makes sense to get the
> details nailed down first.

I thought so :)

>> Fixed - updated patch tested and attached.
>
> OK. FWIW, I think this is an awesome idea.  I understand others are skeptical,
> but this seems simple and if it works and you're happy to maintain it I'm
> happy to let you do it :)

Awesoe, thanks Rusty :)