[PATCH] Add VirtIO Frame Buffer Support

[PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

When we want to create a full VirtIO based machine, we're still missing
graphics output. Fortunately, Linux provides us with most of the frameworks
to render text and everything, we only need to implement a transport.

So this is a frame buffer backend written for VirtIO. Using this and my
patch to qemu, you can use paravirtualized graphics.

This is especially important on machines that can't do MMIO, as all current
graphics implementations qemu emulates I'm aware of so far fail here.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 drivers/video/Kconfig      |   15 +
 drivers/video/Makefile     |    1 +
 drivers/video/virtio-fb.c  |  799 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/virtio_ids.h |    1 +
 4 files changed, 816 insertions(+), 0 deletions(-)
 create mode 100644 drivers/video/virtio-fb.c

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 9bbb285..f9be4c2 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -2069,6 +2069,21 @@ config XEN_FBDEV_FRONTEND
 	  frame buffer driver.  It communicates with a back-end
 	  in another domain.
 
+config FB_VIRTIO
+	tristate "Virtio virtual frame buffer support"
+	depends on FB && VIRTIO
+	select FB_SYS_FILLRECT
+	select FB_SYS_COPYAREA
+	select FB_SYS_IMAGEBLIT
+	select FB_SYS_FOPS
+	select FB_DEFERRED_IO
+	help
+	  This driver implements a driver for a Virtio based
+	  frame buffer device.  It communicates to something that
+	  can talk Virtio too, most probably a hypervisor.
+
+	  If unsure, say N.
+
 config FB_METRONOME
 	tristate "E-Ink Metronome/8track controller support"
 	depends on FB
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 80232e1..40802c8 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -125,6 +125,7 @@ obj-$(CONFIG_FB_XILINX)           += xilinxfb.o
 obj-$(CONFIG_FB_SH_MOBILE_LCDC)	  += sh_mobile_lcdcfb.o
 obj-$(CONFIG_FB_OMAP)             += omap/
 obj-$(CONFIG_XEN_FBDEV_FRONTEND)  += xen-fbfront.o
+obj-$(CONFIG_FB_VIRTIO)           += virtio-fb.o
 obj-$(CONFIG_FB_CARMINE)          += carminefb.o
 obj-$(CONFIG_FB_MB862XX)	  += mb862xx/
 obj-$(CONFIG_FB_MSM)              += msm/
diff --git a/drivers/video/virtio-fb.c b/drivers/video/virtio-fb.c
new file mode 100644
index 0000000..2a73950
--- /dev/null
+++ b/drivers/video/virtio-fb.c
@@ -0,0 +1,799 @@
+/*
+ * VirtIO PV frame buffer device
+ *
+ * Copyright (C) 2009 Alexander Graf <agraf@suse.de>
+ *
+ *  Based on linux/drivers/video/virtio-fbfront.c
+ *
+ *  This file is subject to the terms and conditions of the GNU General Public
+ *  License. See the file COPYING in the main directory of this archive for
+ *  more details.
+ */
+
+#include <linux/console.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/fb.h>
+#include <linux/module.h>
+#include <linux/vmalloc.h>
+#include <linux/mm.h>
+#include <linux/virtio.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_config.h>
+#include <linux/uaccess.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+
+#define MAX_BUF			128
+
+struct virtio_fb_info {
+	struct fb_info		*fb_info;
+	unsigned char		*fb;
+	char			*inbuf;
+
+	u16			width;
+	u16			height;
+
+	int			nr_pages;
+	int			size;
+
+	struct kmem_cache	*cmd_cache;
+	struct virtqueue	*vq_in;
+	struct virtqueue	*vq_out;
+	struct virtio_device	*vdev;
+
+	void			*last_buf[MAX_BUF];
+	int			last_buf_idx;
+	spinlock_t		vfree_lock;
+	struct work_struct	vfree_work;
+	struct work_struct	refresh_work;
+};
+
+/* guest -> Host commands */
+#define VIRTIO_FB_CMD_RESIZE            0x01                                         
+#define VIRTIO_FB_CMD_FILL              0x02                                         
+#define VIRTIO_FB_CMD_BLIT              0x03                                         
+#define VIRTIO_FB_CMD_COPY              0x04                                         
+#define VIRTIO_FB_CMD_WRITE             0x05                                         
+    
+/* host -> guest commands */                                                         
+#define VIRTIO_FB_CMD_REFRESH           0x81                                         
+
+struct virtio_fb_cmd {
+	u8			cmd;
+	union {
+		struct {
+			u16	width;
+			u16	height;
+		} resize __attribute__ ((packed));
+		struct {
+			u16	x;
+			u16	y;
+			u16	width;
+			u16	height;
+		} blit __attribute__ ((packed));
+		struct {
+			u16	x1;
+			u16	y1;
+			u16	x2;
+			u16	y2;
+			u16	width;
+			u16	height;
+		} copy_area __attribute__ ((packed));
+		struct {
+			u8	rop;
+			u16	x;
+			u16	y;
+			u16	width;
+			u16	height;
+			u32	color;
+		} fill __attribute__ ((packed));
+		struct {
+			u64	offset;
+			u64	count;
+		} write __attribute__ ((packed));
+		u8		pad[23];
+	};
+
+	union {
+		/* We remember the data pointer so we we can easily free
+		   everything later by only knowing this structure */
+		char		*send_buf;
+		u64		_pad;
+	};
+} __attribute__ ((packed));
+
+enum copy_type {
+	COPY_KERNEL,
+	COPY_USER,
+	COPY_NOCOPY,
+};
+
+#define DEFAULT_WIDTH		800
+#define DEFAULT_HEIGHT		600
+#define DEFAULT_DEPTH		32
+#define DEFAULT_MEM		8
+
+#define DEFAULT_FB_LEN (DEFAULT_WIDTH * DEFAULT_HEIGHT * DEFAULT_DEPTH / 8)
+
+enum { KPARAM_MEM, KPARAM_WIDTH, KPARAM_HEIGHT, KPARAM_CNT };
+static int video[KPARAM_CNT] = { DEFAULT_MEM, DEFAULT_WIDTH, DEFAULT_HEIGHT };
+module_param_array(video, int, NULL, 0);
+MODULE_PARM_DESC(video,
+	"Video size in M,width,height in pixels (default \""
+	str(DEFAULT_MEM) ","
+	str(DEFAULT_WIDTH) ","
+	str(DEFAULT_HEIGHT) "\")");
+
+static void virtio_fb_output(struct virtqueue *vq);
+
+static void *rvmalloc(unsigned long size)
+{
+	void *mem;
+	unsigned long adr;
+
+	size = PAGE_ALIGN(size);
+	mem = vmalloc_32(size);
+	if (!mem)
+		return NULL;
+
+	memset(mem, 0, size); /* Clear the ram out, no junk to the user */
+	adr = (unsigned long) mem;
+	while (size > 0) {
+		SetPageReserved(vmalloc_to_page((void *)adr));
+		adr += PAGE_SIZE;
+		size -= PAGE_SIZE;
+	}
+
+	return mem;
+}
+
+/* This is videobuf_vmalloc_to_sg() from videobuf-dma-sg.c
+   I modified it to take an extra sg entry for the cmd and work with non
+   page-aligned pointers though */
+static struct scatterlist* vmalloc_to_sg(unsigned char *virt, int length,
+					 void *cmd, int cmd_len, int *sg_elem)
+{
+	struct scatterlist *sglist;
+	struct page *pg;
+	int nr_pages = (length+PAGE_SIZE-1)/PAGE_SIZE;
+	int sg_entries;
+	int i;
+
+	/* unaligned */
+	if ((ulong)virt & ~PAGE_MASK) {
+		int tmp_len = length - (PAGE_SIZE - ((ulong)virt & ~PAGE_MASK));
+		/* how long would it be without the first non-aligned chunk? */
+		nr_pages = (tmp_len+PAGE_SIZE-1)/PAGE_SIZE;
+		/* add the first chunk */
+		nr_pages++;
+	}
+
+	sg_entries = nr_pages + 1;
+
+	sglist = kcalloc(sg_entries, sizeof(struct scatterlist), GFP_KERNEL);
+	if (!sglist)
+		return NULL;
+	sg_init_table(sglist, sg_entries);
+
+	/* Put cmd element in */
+	sg_set_buf(&sglist[0], cmd, cmd_len);
+
+	/* Fill with elements for the data */
+	for (i = 1; i < sg_entries; i++) {
+		pg = vmalloc_to_page(virt);
+		if (!pg)
+			goto err;
+
+		if ((ulong)virt & ~PAGE_MASK) {
+			int tmp_off = ((ulong)virt & ~PAGE_MASK);
+
+			sg_set_page(&sglist[i], pg, PAGE_SIZE - tmp_off, tmp_off);
+			virt = (char*)((ulong)virt & PAGE_MASK);
+		} else {
+			sg_set_page(&sglist[i], pg, PAGE_SIZE, 0);
+		}
+		virt += PAGE_SIZE;
+	}
+
+	*sg_elem = sg_entries;
+	return sglist;
+
+ err:
+	kfree(sglist);
+	return NULL;
+}
+
+
+static void _virtio_fb_send(struct virtio_fb_info *info,
+			    struct virtio_fb_cmd *cmd,
+			    char *buf, int len, enum copy_type copy)
+{
+	char *send_buf = NULL;
+	char *sg_buf = NULL;
+	struct virtio_fb_cmd *send_cmd;
+	struct scatterlist *sg;
+	int len_cmd = sizeof(struct virtio_fb_cmd);
+	int r, sg_elem;
+
+	send_cmd = kmem_cache_alloc(info->cmd_cache, GFP_KERNEL);
+	if (!send_cmd) {
+		printk(KERN_ERR "virtio-fb: couldn't allocate cmd\n");
+		return;
+	}
+
+	memcpy(send_cmd, cmd, len_cmd);
+
+	if (len) {
+		sg_buf = send_buf = vmalloc(len);
+		if (!send_buf) {
+			printk(KERN_ERR "virtio-fb: couldn't allocate %d b\n",
+					len);
+			return;
+		}
+
+		switch (copy) {
+		case COPY_KERNEL:
+			memcpy(send_buf, buf, len);
+			break;
+		case COPY_USER:
+			r = copy_from_user(send_buf, (const __user char*)buf,
+					   len);
+			break;
+		case COPY_NOCOPY:
+			sg_buf = buf;
+			break;
+		}
+	}
+
+	send_cmd->send_buf = send_buf;
+
+	sg = vmalloc_to_sg(sg_buf, len, send_cmd, len_cmd, &sg_elem);
+	if (!sg) {
+		printk(KERN_ERR "virtio-fb: couldn't gather scatter list\n");
+		return;
+	}
+
+add_again:
+	r = info->vq_out->vq_ops->add_buf(info->vq_out, sg, sg_elem, 0, send_cmd);
+	info->vq_out->vq_ops->kick(info->vq_out);
+
+	if ( r == -ENOSPC ) {
+		/* Queue was full, so try again */
+		cpu_relax();
+		virtio_fb_output(info->vq_out);
+		goto add_again;
+	}
+
+	kfree(sg);
+}
+
+static void virtio_fb_send_user(struct virtio_fb_info *info,
+				struct virtio_fb_cmd *cmd,
+				const __user char *buf, int len)
+{
+	_virtio_fb_send(info, cmd, (char *)buf, len, COPY_USER);
+}
+
+static void virtio_fb_send_nocopy(struct virtio_fb_info *info,
+				  struct virtio_fb_cmd *cmd,
+				  char *buf, int len)
+{
+	_virtio_fb_send(info, cmd, buf, len, COPY_NOCOPY);
+}
+
+static void virtio_fb_send(struct virtio_fb_info *info, struct virtio_fb_cmd *cmd,
+			   char *buf, int len)
+{
+	_virtio_fb_send(info, cmd, buf, len, COPY_KERNEL);
+}
+
+
+static int virtio_fb_setcolreg(unsigned regno, unsigned red, unsigned green,
+			   unsigned blue, unsigned transp,
+			   struct fb_info *info)
+{
+	u32 v;
+	if (regno >= 16)
+		return 1;
+
+#define CNVT_TOHW(val,width) ((((val)<<(width))+0x7FFF-(val))>>16)
+	red = CNVT_TOHW(red, info->var.red.length);
+	green = CNVT_TOHW(green, info->var.green.length);
+	blue = CNVT_TOHW(blue, info->var.blue.length);
+	transp = CNVT_TOHW(transp, info->var.transp.length);
+#undef CNVT_TOHW
+
+
+	v = (red << info->var.red.offset) |
+	    (green << info->var.green.offset) |
+	    (blue << info->var.blue.offset) |
+	    (transp << info->var.transp.offset);
+
+	((u32 *) (info->pseudo_palette))[regno] = v;
+
+	return 0;
+}
+
+static void virtio_fb_fillrect(struct fb_info *p, const struct fb_fillrect *rect)
+{
+	struct virtio_fb_info *info = p->par;
+	struct virtio_fb_cmd cmd;
+
+	sys_fillrect(p, rect);
+
+	cmd.cmd = VIRTIO_FB_CMD_FILL;
+	cmd.fill.rop = rect->rop;
+	cmd.fill.x = rect->dx;
+	cmd.fill.y = rect->dy;
+	cmd.fill.width = rect->width;
+	cmd.fill.height = rect->height;
+	cmd.fill.color = rect->color;
+
+	virtio_fb_send(info, &cmd, NULL, 0);
+}
+
+static void virtio_fb_imageblit(struct fb_info *p, const struct fb_image *image)
+{
+	struct virtio_fb_info *info = p->par;
+	struct virtio_fb_cmd cmd;
+	char *_buf;
+	char *buf;
+	char *vfb;
+	int i, w;
+	int bpp = p->var.bits_per_pixel / 8;
+	int len = image->width * image->height * bpp;
+
+	sys_imageblit(p, image);
+
+	cmd.cmd = VIRTIO_FB_CMD_BLIT;
+	cmd.blit.x = image->dx;
+	cmd.blit.y = image->dy;
+	cmd.blit.height = image->height;
+	cmd.blit.width = image->width;
+
+	if (image->depth == 32) {
+		/* Send the image off directly */
+
+		virtio_fb_send(info, &cmd, (char*)image->data, len);
+		return;
+	}
+
+	/* Send the 32-bit translated image */
+
+	buf = _buf = vmalloc(len);
+
+	vfb = info->fb;
+	vfb += (image->dy * p->fix.line_length) + image->dx * bpp;
+
+	w = image->width * bpp;
+
+	for (i = 0; i < image->height; i++) {
+		memcpy(buf, vfb, w);
+
+		buf += w;
+		vfb += p->fix.line_length;
+	}
+
+	virtio_fb_send(info, &cmd, _buf, len);
+
+	vfree(_buf);
+}
+
+static void virtio_fb_copyarea(struct fb_info *p, const struct fb_copyarea *area)
+{
+	struct virtio_fb_info *info = p->par;
+	struct virtio_fb_cmd cmd;
+
+	sys_copyarea(p, area);
+
+	cmd.cmd = VIRTIO_FB_CMD_COPY;
+	cmd.copy_area.x1 = area->sx;
+	cmd.copy_area.y1 = area->sy;
+	cmd.copy_area.x2 = area->dx;
+	cmd.copy_area.y2 = area->dy;
+	cmd.copy_area.width = area->width;
+	cmd.copy_area.height = area->height;
+
+	virtio_fb_send(info, &cmd, NULL, 0);
+}
+
+static ssize_t virtio_fb_write(struct fb_info *p, const char __user *buf,
+			size_t count, loff_t *ppos)
+{
+	struct virtio_fb_info *info = p->par;
+	struct virtio_fb_cmd cmd;
+	loff_t pos = *ppos;
+	ssize_t res;
+
+	res = fb_sys_write(p, buf, count, ppos);
+
+	cmd.cmd = VIRTIO_FB_CMD_WRITE;
+	cmd.write.offset = pos;
+	cmd.write.count = count;
+
+	virtio_fb_send_user(info, &cmd, buf, count);
+	return res;
+}
+
+static int
+virtio_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *p)
+{
+	struct virtio_fb_info *info = p->par;
+
+	if (var->bits_per_pixel != DEFAULT_DEPTH) {
+		/* We only support 32 bpp */
+		return -EINVAL;
+	}
+
+	if ((var->xres * var->yres * DEFAULT_DEPTH / 8) > info->size) {
+		/* Doesn't fit in the frame buffer */
+		return -EINVAL;
+	}
+
+	var->xres_virtual = var->xres;
+	var->yres_virtual = var->yres;
+
+	return 0;
+}
+
+static int virtio_fb_set_par(struct fb_info *p)
+{
+	struct virtio_fb_info *info = p->par;
+	struct virtio_fb_cmd cmd;
+
+	/* Do nothing if we're on that resolution already */
+	if ((p->var.xres == info->width) &&
+	    (p->var.yres == info->height))
+		return 0;
+
+	info->width = p->var.xres;
+	info->height = p->var.yres;
+	p->fix.line_length = p->var.xres_virtual * 4;
+
+	/* Notify hypervisor */
+
+	cmd.cmd = VIRTIO_FB_CMD_RESIZE;
+	cmd.resize.width = p->var.xres;
+	cmd.resize.height = p->var.yres;
+
+	virtio_fb_send(info, &cmd, NULL, 0);
+
+	return 0;
+}
+
+static struct fb_ops virtio_fb_ops = {
+	.owner		= THIS_MODULE,
+	.fb_read	= fb_sys_read,
+	.fb_write	= virtio_fb_write,
+	.fb_setcolreg	= virtio_fb_setcolreg,
+	.fb_fillrect	= virtio_fb_fillrect,
+	.fb_copyarea	= virtio_fb_copyarea,
+	.fb_imageblit	= virtio_fb_imageblit,
+	.fb_check_var	= virtio_fb_check_var,
+	.fb_set_par     = virtio_fb_set_par,
+};
+
+static __devinit void
+virtio_fb_make_preferred_console(void)
+{
+	struct console *c;
+
+	if (console_set_on_cmdline)
+		return;
+
+	acquire_console_sem();
+	for (c = console_drivers; c; c = c->next) {
+		if (!strcmp(c->name, "tty") && c->index == 0)
+			break;
+	}
+	release_console_sem();
+	if (c) {
+		unregister_console(c);
+		c->flags |= CON_CONSDEV;
+		c->flags &= ~CON_PRINTBUFFER; /* don't print again */
+		register_console(c);
+	}
+}
+
+static void virtio_fb_deferred_io(struct fb_info *fb_info,
+				  struct list_head *pagelist)
+{
+	struct virtio_fb_info *info = fb_info->par;
+	struct page *page;
+	unsigned long beg, end;
+	int y1, y2, miny, maxy;
+	struct virtio_fb_cmd cmd;
+
+	miny = INT_MAX;
+	maxy = 0;
+	list_for_each_entry(page, pagelist, lru) {
+		beg = page->index << PAGE_SHIFT;
+		end = beg + PAGE_SIZE - 1;
+		y1 = beg / fb_info->fix.line_length;
+		y2 = end / fb_info->fix.line_length;
+		if (y2 >= fb_info->var.yres)
+			y2 = fb_info->var.yres - 1;
+		if (miny > y1)
+			miny = y1;
+		if (maxy < y2)
+			maxy = y2;
+	}
+
+	if (miny != INT_MAX) {
+		cmd.cmd = VIRTIO_FB_CMD_WRITE;
+		cmd.write.offset = miny * fb_info->fix.line_length;
+		cmd.write.count = (maxy - miny + 1) * fb_info->fix.line_length;
+
+		virtio_fb_send_nocopy(info, &cmd, info->fb + cmd.write.offset,
+				      cmd.write.count);
+	}
+}
+
+static struct fb_deferred_io virtio_fb_defio = {
+	.delay		= HZ / 20,
+	.deferred_io	= virtio_fb_deferred_io,
+};
+
+/* Callback when the host kicks our input queue.
+ *
+ * This is to enable notifications from host to guest. */
+static void virtio_fb_input(struct virtqueue *vq)
+{
+	struct virtio_fb_info *info = dev_get_drvdata(&vq->vdev->dev);
+	int len, i;
+	void *x;
+	void *reinject[3];
+	int reinject_count = 0;
+
+	while ((x = vq->vq_ops->get_buf(vq, &len)) != NULL) {
+		struct virtio_fb_cmd *cmd = x;
+
+		/* Make sure we're getting an inbuf page! */
+		BUG_ON((x != info->inbuf) &&
+		       (x != (info->inbuf + PAGE_SIZE)) &&
+		       (x != (info->inbuf + (PAGE_SIZE * 2))));
+
+		switch (cmd->cmd) {
+		case VIRTIO_FB_CMD_REFRESH:
+			schedule_work(&info->refresh_work);
+			break;
+		}
+
+		reinject[reinject_count++] = x;
+	}
+
+
+	for (i = 0; i < reinject_count; i++) {
+		struct scatterlist sg;
+		void *x = reinject[i];
+
+		sg_init_one(&sg, x, PAGE_SIZE);
+		vq->vq_ops->add_buf(vq, &sg, 0, 1, x);
+		vq->vq_ops->kick(vq);
+	}
+}
+
+/* Asynchronous snippet to send all screen contents to the host */
+static void deferred_refresh(struct work_struct *work)
+{
+	struct virtio_fb_info *info = container_of(work, struct virtio_fb_info,
+						   refresh_work);
+	struct virtio_fb_cmd cmd;
+
+	cmd.cmd = VIRTIO_FB_CMD_WRITE;
+	cmd.write.offset = 0;
+	cmd.write.count = info->width * info->height * 4;
+
+	virtio_fb_send_nocopy(info, &cmd, info->fb, cmd.write.count);
+}
+
+/* Asynchronous garbage collector :-) */
+static void deferred_vfree(struct work_struct *work)
+{
+	struct virtio_fb_info *info = container_of(work, struct virtio_fb_info,
+						   vfree_work);
+	int i;
+	unsigned long flags;
+	void *last_buf[MAX_BUF];
+	int idx;
+
+	spin_lock_irqsave(&info->vfree_lock, flags);
+
+	idx = info->last_buf_idx;
+	memcpy(last_buf, info->last_buf, sizeof(void*) * MAX_BUF);
+	info->last_buf_idx = 0;
+
+	spin_unlock_irqrestore(&info->vfree_lock, flags);
+
+	for (i = 0; i < idx; i++) {
+		vfree(last_buf[i]);
+	}
+}
+
+/* Callback when the host kicks our output queue. This can only mean it's done
+ * processing an item, so let's free up the memory occupied by the entries */
+static void virtio_fb_output(struct virtqueue *vq)
+{
+	struct virtio_fb_info *info = dev_get_drvdata(&vq->vdev->dev);
+	int len;
+	void *x;
+
+	while ((x = vq->vq_ops->get_buf(vq, &len)) != NULL) {
+		struct virtio_fb_cmd *cmd = x;
+
+		if (cmd->send_buf) {
+			spin_lock(&info->vfree_lock);
+			if (info->last_buf_idx != MAX_BUF) {
+				info->last_buf[info->last_buf_idx++] =
+					cmd->send_buf;
+			}
+			spin_unlock(&info->vfree_lock);
+
+			schedule_work(&info->vfree_work);
+		}
+
+		kmem_cache_free(info->cmd_cache, x);
+	}
+}
+
+static int __devinit virtio_fb_probe(struct virtio_device *dev)
+{
+	vq_callback_t *callbacks[] = { virtio_fb_input, virtio_fb_output };
+	const char *names[] = { "input", "output" };
+	struct virtio_fb_info *info;
+	struct virtqueue *vqs[2];
+	struct fb_info *fb_info = NULL;
+	int fb_size, res_size;
+	int ret, err, i;
+	char *inbuf;
+
+	err = dev->config->find_vqs(dev, 2, vqs, callbacks, names);
+	if (err) {
+		printk(KERN_ERR "VirtIO FB: couldn't find VQs\n");
+		return err;
+	}
+
+	info = kmalloc(sizeof(*info), GFP_KERNEL);
+	if (info == NULL)
+		return -ENOMEM;
+
+	info->vq_in = vqs[0];
+	info->vq_out = vqs[1];
+
+	res_size = video[KPARAM_WIDTH] * video[KPARAM_HEIGHT] * DEFAULT_DEPTH / 8;
+	fb_size = video[KPARAM_MEM] * 1024 * 1024;
+
+	if (res_size > fb_size) {
+		video[KPARAM_WIDTH] = DEFAULT_WIDTH;
+		video[KPARAM_HEIGHT] = DEFAULT_HEIGHT;
+	}
+
+	dev_set_drvdata(&dev->dev, info);
+	info->vdev = dev;
+
+	info->fb = rvmalloc(fb_size);
+	if (info->fb == NULL)
+		goto error_nomem;
+
+	info->nr_pages = (fb_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	info->size = fb_size;
+
+	fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL);
+	if (fb_info == NULL)
+		goto error_nomem;
+
+	inbuf = kmalloc(PAGE_SIZE * 3, GFP_KERNEL);
+	info->inbuf = inbuf;
+	for (i = 0; i < (3 * PAGE_SIZE); i += PAGE_SIZE) {
+		struct scatterlist sg;
+
+		sg_init_one(&sg, inbuf + i, PAGE_SIZE);
+		info->vq_in->vq_ops->add_buf(info->vq_in, &sg, 0, 1, inbuf + i);
+	}
+	info->vq_in->vq_ops->kick(info->vq_in);
+
+	fb_info->pseudo_palette = fb_info->par;
+	fb_info->par = info;
+
+	fb_info->screen_base = info->fb;
+
+	fb_info->fbops = &virtio_fb_ops;
+	fb_info->var.xres_virtual = fb_info->var.xres = video[KPARAM_WIDTH];
+	fb_info->var.yres_virtual = fb_info->var.yres = video[KPARAM_HEIGHT];
+	fb_info->var.bits_per_pixel = DEFAULT_DEPTH;
+
+	fb_info->var.transp = (struct fb_bitfield){24, 8, 0};
+	fb_info->var.red    = (struct fb_bitfield){16, 8, 0};
+	fb_info->var.green  = (struct fb_bitfield){8,  8, 0};
+	fb_info->var.blue   = (struct fb_bitfield){0,  8, 0};
+
+	fb_info->var.activate = FB_ACTIVATE_NOW;
+	fb_info->var.height = -1;
+	fb_info->var.width = -1;
+	fb_info->var.vmode = FB_VMODE_NONINTERLACED;
+
+	fb_info->fix.visual = FB_VISUAL_TRUECOLOR;
+	fb_info->fix.line_length = fb_info->var.xres * DEFAULT_DEPTH / 8;
+	fb_info->fix.smem_start = 0;
+	fb_info->fix.smem_len = fb_size;
+	strcpy(fb_info->fix.id, "virtio_fb");
+	fb_info->fix.type = FB_TYPE_PACKED_PIXELS;
+	fb_info->fix.accel = FB_ACCEL_NONE;
+
+	fb_info->flags = FBINFO_FLAG_DEFAULT | FBINFO_HWACCEL_COPYAREA |
+			 FBINFO_READS_FAST;
+
+	ret = fb_alloc_cmap(&fb_info->cmap, 256, 0);
+	if (ret < 0) {
+		framebuffer_release(fb_info);
+		goto error;
+	}
+
+	info->cmd_cache = kmem_cache_create("virtio_fb_cmd",
+					    sizeof(struct virtio_fb_cmd),
+					    0, 0, NULL);
+
+	if (!info->cmd_cache) {
+		framebuffer_release(fb_info);
+		goto error;
+	}
+
+	fb_info->fbdefio = &virtio_fb_defio;
+	fb_deferred_io_init(fb_info);
+
+	INIT_WORK(&info->refresh_work, deferred_refresh);
+	INIT_WORK(&info->vfree_work, deferred_vfree);
+	spin_lock_init(&info->vfree_lock);
+
+	ret = register_framebuffer(fb_info);
+	if (ret) {
+		fb_deferred_io_cleanup(fb_info);
+		fb_dealloc_cmap(&fb_info->cmap);
+		framebuffer_release(fb_info);
+		goto error;
+	}
+	info->fb_info = fb_info;
+
+	virtio_fb_make_preferred_console();
+	return 0;
+
+ error_nomem:
+	ret = -ENOMEM;
+ error:
+	framebuffer_release(fb_info);
+	return ret;
+}
+
+static void virtio_fb_apply_config(struct virtio_device *dev)
+{
+}
+
+static struct virtio_device_id id_table[] = {
+	{ VIRTIO_ID_FB, VIRTIO_DEV_ANY_ID },
+	{ 0 },
+};
+
+static unsigned int features[] = {
+};
+
+static struct virtio_driver virtio_console = {
+	.feature_table = features,
+	.feature_table_size = ARRAY_SIZE(features),
+	.driver.name =  KBUILD_MODNAME,
+	.driver.owner = THIS_MODULE,
+	.id_table =     id_table,
+	.probe =        virtio_fb_probe,
+	.config_changed = virtio_fb_apply_config,
+};
+
+static int __init init(void)
+{
+	return register_virtio_driver(&virtio_console);
+}
+module_init(init);
+
+
+MODULE_DEVICE_TABLE(virtio, id_table);
+MODULE_DESCRIPTION("Virtio framebuffer driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
index 06660c0..72e39f7 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_FB		6 /* virtio framebuffer */
 #define VIRTIO_ID_9P		9 /* 9p virtio console */
 
 #endif /* _LINUX_VIRTIO_IDS_H */
-- 
1.6.0.2

[Qemu-devel] [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

When we want to create a full VirtIO based machine, we're still missing
graphics output. Fortunately, Linux provides us with most of the frameworks
to render text and everything, we only need to implement a transport.

So this is a frame buffer backend written for VirtIO. Using this and my
patch to qemu, you can use paravirtualized graphics.

This is especially important on machines that can't do MMIO, as all current
graphics implementations qemu emulates I'm aware of so far fail here.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 drivers/video/Kconfig      |   15 +
 drivers/video/Makefile     |    1 +
 drivers/video/virtio-fb.c  |  799 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/virtio_ids.h |    1 +
 4 files changed, 816 insertions(+), 0 deletions(-)
 create mode 100644 drivers/video/virtio-fb.c

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 9bbb285..f9be4c2 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -2069,6 +2069,21 @@ config XEN_FBDEV_FRONTEND
 	  frame buffer driver.  It communicates with a back-end
 	  in another domain.
 
+config FB_VIRTIO
+	tristate "Virtio virtual frame buffer support"
+	depends on FB && VIRTIO
+	select FB_SYS_FILLRECT
+	select FB_SYS_COPYAREA
+	select FB_SYS_IMAGEBLIT
+	select FB_SYS_FOPS
+	select FB_DEFERRED_IO
+	help
+	  This driver implements a driver for a Virtio based
+	  frame buffer device.  It communicates to something that
+	  can talk Virtio too, most probably a hypervisor.
+
+	  If unsure, say N.
+
 config FB_METRONOME
 	tristate "E-Ink Metronome/8track controller support"
 	depends on FB
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 80232e1..40802c8 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -125,6 +125,7 @@ obj-$(CONFIG_FB_XILINX)           += xilinxfb.o
 obj-$(CONFIG_FB_SH_MOBILE_LCDC)	  += sh_mobile_lcdcfb.o
 obj-$(CONFIG_FB_OMAP)             += omap/
 obj-$(CONFIG_XEN_FBDEV_FRONTEND)  += xen-fbfront.o
+obj-$(CONFIG_FB_VIRTIO)           += virtio-fb.o
 obj-$(CONFIG_FB_CARMINE)          += carminefb.o
 obj-$(CONFIG_FB_MB862XX)	  += mb862xx/
 obj-$(CONFIG_FB_MSM)              += msm/
diff --git a/drivers/video/virtio-fb.c b/drivers/video/virtio-fb.c
new file mode 100644
index 0000000..2a73950
--- /dev/null
+++ b/drivers/video/virtio-fb.c
@@ -0,0 +1,799 @@
+/*
+ * VirtIO PV frame buffer device
+ *
+ * Copyright (C) 2009 Alexander Graf <agraf@suse.de>
+ *
+ *  Based on linux/drivers/video/virtio-fbfront.c
+ *
+ *  This file is subject to the terms and conditions of the GNU General Public
+ *  License. See the file COPYING in the main directory of this archive for
+ *  more details.
+ */
+
+#include <linux/console.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/fb.h>
+#include <linux/module.h>
+#include <linux/vmalloc.h>
+#include <linux/mm.h>
+#include <linux/virtio.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_config.h>
+#include <linux/uaccess.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+
+#define MAX_BUF			128
+
+struct virtio_fb_info {
+	struct fb_info		*fb_info;
+	unsigned char		*fb;
+	char			*inbuf;
+
+	u16			width;
+	u16			height;
+
+	int			nr_pages;
+	int			size;
+
+	struct kmem_cache	*cmd_cache;
+	struct virtqueue	*vq_in;
+	struct virtqueue	*vq_out;
+	struct virtio_device	*vdev;
+
+	void			*last_buf[MAX_BUF];
+	int			last_buf_idx;
+	spinlock_t		vfree_lock;
+	struct work_struct	vfree_work;
+	struct work_struct	refresh_work;
+};
+
+/* guest -> Host commands */
+#define VIRTIO_FB_CMD_RESIZE            0x01                                         
+#define VIRTIO_FB_CMD_FILL              0x02                                         
+#define VIRTIO_FB_CMD_BLIT              0x03                                         
+#define VIRTIO_FB_CMD_COPY              0x04                                         
+#define VIRTIO_FB_CMD_WRITE             0x05                                         
+    
+/* host -> guest commands */                                                         
+#define VIRTIO_FB_CMD_REFRESH           0x81                                         
+
+struct virtio_fb_cmd {
+	u8			cmd;
+	union {
+		struct {
+			u16	width;
+			u16	height;
+		} resize __attribute__ ((packed));
+		struct {
+			u16	x;
+			u16	y;
+			u16	width;
+			u16	height;
+		} blit __attribute__ ((packed));
+		struct {
+			u16	x1;
+			u16	y1;
+			u16	x2;
+			u16	y2;
+			u16	width;
+			u16	height;
+		} copy_area __attribute__ ((packed));
+		struct {
+			u8	rop;
+			u16	x;
+			u16	y;
+			u16	width;
+			u16	height;
+			u32	color;
+		} fill __attribute__ ((packed));
+		struct {
+			u64	offset;
+			u64	count;
+		} write __attribute__ ((packed));
+		u8		pad[23];
+	};
+
+	union {
+		/* We remember the data pointer so we we can easily free
+		   everything later by only knowing this structure */
+		char		*send_buf;
+		u64		_pad;
+	};
+} __attribute__ ((packed));
+
+enum copy_type {
+	COPY_KERNEL,
+	COPY_USER,
+	COPY_NOCOPY,
+};
+
+#define DEFAULT_WIDTH		800
+#define DEFAULT_HEIGHT		600
+#define DEFAULT_DEPTH		32
+#define DEFAULT_MEM		8
+
+#define DEFAULT_FB_LEN (DEFAULT_WIDTH * DEFAULT_HEIGHT * DEFAULT_DEPTH / 8)
+
+enum { KPARAM_MEM, KPARAM_WIDTH, KPARAM_HEIGHT, KPARAM_CNT };
+static int video[KPARAM_CNT] = { DEFAULT_MEM, DEFAULT_WIDTH, DEFAULT_HEIGHT };
+module_param_array(video, int, NULL, 0);
+MODULE_PARM_DESC(video,
+	"Video size in M,width,height in pixels (default \""
+	str(DEFAULT_MEM) ","
+	str(DEFAULT_WIDTH) ","
+	str(DEFAULT_HEIGHT) "\")");
+
+static void virtio_fb_output(struct virtqueue *vq);
+
+static void *rvmalloc(unsigned long size)
+{
+	void *mem;
+	unsigned long adr;
+
+	size = PAGE_ALIGN(size);
+	mem = vmalloc_32(size);
+	if (!mem)
+		return NULL;
+
+	memset(mem, 0, size); /* Clear the ram out, no junk to the user */
+	adr = (unsigned long) mem;
+	while (size > 0) {
+		SetPageReserved(vmalloc_to_page((void *)adr));
+		adr += PAGE_SIZE;
+		size -= PAGE_SIZE;
+	}
+
+	return mem;
+}
+
+/* This is videobuf_vmalloc_to_sg() from videobuf-dma-sg.c
+   I modified it to take an extra sg entry for the cmd and work with non
+   page-aligned pointers though */
+static struct scatterlist* vmalloc_to_sg(unsigned char *virt, int length,
+					 void *cmd, int cmd_len, int *sg_elem)
+{
+	struct scatterlist *sglist;
+	struct page *pg;
+	int nr_pages = (length+PAGE_SIZE-1)/PAGE_SIZE;
+	int sg_entries;
+	int i;
+
+	/* unaligned */
+	if ((ulong)virt & ~PAGE_MASK) {
+		int tmp_len = length - (PAGE_SIZE - ((ulong)virt & ~PAGE_MASK));
+		/* how long would it be without the first non-aligned chunk? */
+		nr_pages = (tmp_len+PAGE_SIZE-1)/PAGE_SIZE;
+		/* add the first chunk */
+		nr_pages++;
+	}
+
+	sg_entries = nr_pages + 1;
+
+	sglist = kcalloc(sg_entries, sizeof(struct scatterlist), GFP_KERNEL);
+	if (!sglist)
+		return NULL;
+	sg_init_table(sglist, sg_entries);
+
+	/* Put cmd element in */
+	sg_set_buf(&sglist[0], cmd, cmd_len);
+
+	/* Fill with elements for the data */
+	for (i = 1; i < sg_entries; i++) {
+		pg = vmalloc_to_page(virt);
+		if (!pg)
+			goto err;
+
+		if ((ulong)virt & ~PAGE_MASK) {
+			int tmp_off = ((ulong)virt & ~PAGE_MASK);
+
+			sg_set_page(&sglist[i], pg, PAGE_SIZE - tmp_off, tmp_off);
+			virt = (char*)((ulong)virt & PAGE_MASK);
+		} else {
+			sg_set_page(&sglist[i], pg, PAGE_SIZE, 0);
+		}
+		virt += PAGE_SIZE;
+	}
+
+	*sg_elem = sg_entries;
+	return sglist;
+
+ err:
+	kfree(sglist);
+	return NULL;
+}
+
+
+static void _virtio_fb_send(struct virtio_fb_info *info,
+			    struct virtio_fb_cmd *cmd,
+			    char *buf, int len, enum copy_type copy)
+{
+	char *send_buf = NULL;
+	char *sg_buf = NULL;
+	struct virtio_fb_cmd *send_cmd;
+	struct scatterlist *sg;
+	int len_cmd = sizeof(struct virtio_fb_cmd);
+	int r, sg_elem;
+
+	send_cmd = kmem_cache_alloc(info->cmd_cache, GFP_KERNEL);
+	if (!send_cmd) {
+		printk(KERN_ERR "virtio-fb: couldn't allocate cmd\n");
+		return;
+	}
+
+	memcpy(send_cmd, cmd, len_cmd);
+
+	if (len) {
+		sg_buf = send_buf = vmalloc(len);
+		if (!send_buf) {
+			printk(KERN_ERR "virtio-fb: couldn't allocate %d b\n",
+					len);
+			return;
+		}
+
+		switch (copy) {
+		case COPY_KERNEL:
+			memcpy(send_buf, buf, len);
+			break;
+		case COPY_USER:
+			r = copy_from_user(send_buf, (const __user char*)buf,
+					   len);
+			break;
+		case COPY_NOCOPY:
+			sg_buf = buf;
+			break;
+		}
+	}
+
+	send_cmd->send_buf = send_buf;
+
+	sg = vmalloc_to_sg(sg_buf, len, send_cmd, len_cmd, &sg_elem);
+	if (!sg) {
+		printk(KERN_ERR "virtio-fb: couldn't gather scatter list\n");
+		return;
+	}
+
+add_again:
+	r = info->vq_out->vq_ops->add_buf(info->vq_out, sg, sg_elem, 0, send_cmd);
+	info->vq_out->vq_ops->kick(info->vq_out);
+
+	if ( r == -ENOSPC ) {
+		/* Queue was full, so try again */
+		cpu_relax();
+		virtio_fb_output(info->vq_out);
+		goto add_again;
+	}
+
+	kfree(sg);
+}
+
+static void virtio_fb_send_user(struct virtio_fb_info *info,
+				struct virtio_fb_cmd *cmd,
+				const __user char *buf, int len)
+{
+	_virtio_fb_send(info, cmd, (char *)buf, len, COPY_USER);
+}
+
+static void virtio_fb_send_nocopy(struct virtio_fb_info *info,
+				  struct virtio_fb_cmd *cmd,
+				  char *buf, int len)
+{
+	_virtio_fb_send(info, cmd, buf, len, COPY_NOCOPY);
+}
+
+static void virtio_fb_send(struct virtio_fb_info *info, struct virtio_fb_cmd *cmd,
+			   char *buf, int len)
+{
+	_virtio_fb_send(info, cmd, buf, len, COPY_KERNEL);
+}
+
+
+static int virtio_fb_setcolreg(unsigned regno, unsigned red, unsigned green,
+			   unsigned blue, unsigned transp,
+			   struct fb_info *info)
+{
+	u32 v;
+	if (regno >= 16)
+		return 1;
+
+#define CNVT_TOHW(val,width) ((((val)<<(width))+0x7FFF-(val))>>16)
+	red = CNVT_TOHW(red, info->var.red.length);
+	green = CNVT_TOHW(green, info->var.green.length);
+	blue = CNVT_TOHW(blue, info->var.blue.length);
+	transp = CNVT_TOHW(transp, info->var.transp.length);
+#undef CNVT_TOHW
+
+
+	v = (red << info->var.red.offset) |
+	    (green << info->var.green.offset) |
+	    (blue << info->var.blue.offset) |
+	    (transp << info->var.transp.offset);
+
+	((u32 *) (info->pseudo_palette))[regno] = v;
+
+	return 0;
+}
+
+static void virtio_fb_fillrect(struct fb_info *p, const struct fb_fillrect *rect)
+{
+	struct virtio_fb_info *info = p->par;
+	struct virtio_fb_cmd cmd;
+
+	sys_fillrect(p, rect);
+
+	cmd.cmd = VIRTIO_FB_CMD_FILL;
+	cmd.fill.rop = rect->rop;
+	cmd.fill.x = rect->dx;
+	cmd.fill.y = rect->dy;
+	cmd.fill.width = rect->width;
+	cmd.fill.height = rect->height;
+	cmd.fill.color = rect->color;
+
+	virtio_fb_send(info, &cmd, NULL, 0);
+}
+
+static void virtio_fb_imageblit(struct fb_info *p, const struct fb_image *image)
+{
+	struct virtio_fb_info *info = p->par;
+	struct virtio_fb_cmd cmd;
+	char *_buf;
+	char *buf;
+	char *vfb;
+	int i, w;
+	int bpp = p->var.bits_per_pixel / 8;
+	int len = image->width * image->height * bpp;
+
+	sys_imageblit(p, image);
+
+	cmd.cmd = VIRTIO_FB_CMD_BLIT;
+	cmd.blit.x = image->dx;
+	cmd.blit.y = image->dy;
+	cmd.blit.height = image->height;
+	cmd.blit.width = image->width;
+
+	if (image->depth == 32) {
+		/* Send the image off directly */
+
+		virtio_fb_send(info, &cmd, (char*)image->data, len);
+		return;
+	}
+
+	/* Send the 32-bit translated image */
+
+	buf = _buf = vmalloc(len);
+
+	vfb = info->fb;
+	vfb += (image->dy * p->fix.line_length) + image->dx * bpp;
+
+	w = image->width * bpp;
+
+	for (i = 0; i < image->height; i++) {
+		memcpy(buf, vfb, w);
+
+		buf += w;
+		vfb += p->fix.line_length;
+	}
+
+	virtio_fb_send(info, &cmd, _buf, len);
+
+	vfree(_buf);
+}
+
+static void virtio_fb_copyarea(struct fb_info *p, const struct fb_copyarea *area)
+{
+	struct virtio_fb_info *info = p->par;
+	struct virtio_fb_cmd cmd;
+
+	sys_copyarea(p, area);
+
+	cmd.cmd = VIRTIO_FB_CMD_COPY;
+	cmd.copy_area.x1 = area->sx;
+	cmd.copy_area.y1 = area->sy;
+	cmd.copy_area.x2 = area->dx;
+	cmd.copy_area.y2 = area->dy;
+	cmd.copy_area.width = area->width;
+	cmd.copy_area.height = area->height;
+
+	virtio_fb_send(info, &cmd, NULL, 0);
+}
+
+static ssize_t virtio_fb_write(struct fb_info *p, const char __user *buf,
+			size_t count, loff_t *ppos)
+{
+	struct virtio_fb_info *info = p->par;
+	struct virtio_fb_cmd cmd;
+	loff_t pos = *ppos;
+	ssize_t res;
+
+	res = fb_sys_write(p, buf, count, ppos);
+
+	cmd.cmd = VIRTIO_FB_CMD_WRITE;
+	cmd.write.offset = pos;
+	cmd.write.count = count;
+
+	virtio_fb_send_user(info, &cmd, buf, count);
+	return res;
+}
+
+static int
+virtio_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *p)
+{
+	struct virtio_fb_info *info = p->par;
+
+	if (var->bits_per_pixel != DEFAULT_DEPTH) {
+		/* We only support 32 bpp */
+		return -EINVAL;
+	}
+
+	if ((var->xres * var->yres * DEFAULT_DEPTH / 8) > info->size) {
+		/* Doesn't fit in the frame buffer */
+		return -EINVAL;
+	}
+
+	var->xres_virtual = var->xres;
+	var->yres_virtual = var->yres;
+
+	return 0;
+}
+
+static int virtio_fb_set_par(struct fb_info *p)
+{
+	struct virtio_fb_info *info = p->par;
+	struct virtio_fb_cmd cmd;
+
+	/* Do nothing if we're on that resolution already */
+	if ((p->var.xres == info->width) &&
+	    (p->var.yres == info->height))
+		return 0;
+
+	info->width = p->var.xres;
+	info->height = p->var.yres;
+	p->fix.line_length = p->var.xres_virtual * 4;
+
+	/* Notify hypervisor */
+
+	cmd.cmd = VIRTIO_FB_CMD_RESIZE;
+	cmd.resize.width = p->var.xres;
+	cmd.resize.height = p->var.yres;
+
+	virtio_fb_send(info, &cmd, NULL, 0);
+
+	return 0;
+}
+
+static struct fb_ops virtio_fb_ops = {
+	.owner		= THIS_MODULE,
+	.fb_read	= fb_sys_read,
+	.fb_write	= virtio_fb_write,
+	.fb_setcolreg	= virtio_fb_setcolreg,
+	.fb_fillrect	= virtio_fb_fillrect,
+	.fb_copyarea	= virtio_fb_copyarea,
+	.fb_imageblit	= virtio_fb_imageblit,
+	.fb_check_var	= virtio_fb_check_var,
+	.fb_set_par     = virtio_fb_set_par,
+};
+
+static __devinit void
+virtio_fb_make_preferred_console(void)
+{
+	struct console *c;
+
+	if (console_set_on_cmdline)
+		return;
+
+	acquire_console_sem();
+	for (c = console_drivers; c; c = c->next) {
+		if (!strcmp(c->name, "tty") && c->index == 0)
+			break;
+	}
+	release_console_sem();
+	if (c) {
+		unregister_console(c);
+		c->flags |= CON_CONSDEV;
+		c->flags &= ~CON_PRINTBUFFER; /* don't print again */
+		register_console(c);
+	}
+}
+
+static void virtio_fb_deferred_io(struct fb_info *fb_info,
+				  struct list_head *pagelist)
+{
+	struct virtio_fb_info *info = fb_info->par;
+	struct page *page;
+	unsigned long beg, end;
+	int y1, y2, miny, maxy;
+	struct virtio_fb_cmd cmd;
+
+	miny = INT_MAX;
+	maxy = 0;
+	list_for_each_entry(page, pagelist, lru) {
+		beg = page->index << PAGE_SHIFT;
+		end = beg + PAGE_SIZE - 1;
+		y1 = beg / fb_info->fix.line_length;
+		y2 = end / fb_info->fix.line_length;
+		if (y2 >= fb_info->var.yres)
+			y2 = fb_info->var.yres - 1;
+		if (miny > y1)
+			miny = y1;
+		if (maxy < y2)
+			maxy = y2;
+	}
+
+	if (miny != INT_MAX) {
+		cmd.cmd = VIRTIO_FB_CMD_WRITE;
+		cmd.write.offset = miny * fb_info->fix.line_length;
+		cmd.write.count = (maxy - miny + 1) * fb_info->fix.line_length;
+
+		virtio_fb_send_nocopy(info, &cmd, info->fb + cmd.write.offset,
+				      cmd.write.count);
+	}
+}
+
+static struct fb_deferred_io virtio_fb_defio = {
+	.delay		= HZ / 20,
+	.deferred_io	= virtio_fb_deferred_io,
+};
+
+/* Callback when the host kicks our input queue.
+ *
+ * This is to enable notifications from host to guest. */
+static void virtio_fb_input(struct virtqueue *vq)
+{
+	struct virtio_fb_info *info = dev_get_drvdata(&vq->vdev->dev);
+	int len, i;
+	void *x;
+	void *reinject[3];
+	int reinject_count = 0;
+
+	while ((x = vq->vq_ops->get_buf(vq, &len)) != NULL) {
+		struct virtio_fb_cmd *cmd = x;
+
+		/* Make sure we're getting an inbuf page! */
+		BUG_ON((x != info->inbuf) &&
+		       (x != (info->inbuf + PAGE_SIZE)) &&
+		       (x != (info->inbuf + (PAGE_SIZE * 2))));
+
+		switch (cmd->cmd) {
+		case VIRTIO_FB_CMD_REFRESH:
+			schedule_work(&info->refresh_work);
+			break;
+		}
+
+		reinject[reinject_count++] = x;
+	}
+
+
+	for (i = 0; i < reinject_count; i++) {
+		struct scatterlist sg;
+		void *x = reinject[i];
+
+		sg_init_one(&sg, x, PAGE_SIZE);
+		vq->vq_ops->add_buf(vq, &sg, 0, 1, x);
+		vq->vq_ops->kick(vq);
+	}
+}
+
+/* Asynchronous snippet to send all screen contents to the host */
+static void deferred_refresh(struct work_struct *work)
+{
+	struct virtio_fb_info *info = container_of(work, struct virtio_fb_info,
+						   refresh_work);
+	struct virtio_fb_cmd cmd;
+
+	cmd.cmd = VIRTIO_FB_CMD_WRITE;
+	cmd.write.offset = 0;
+	cmd.write.count = info->width * info->height * 4;
+
+	virtio_fb_send_nocopy(info, &cmd, info->fb, cmd.write.count);
+}
+
+/* Asynchronous garbage collector :-) */
+static void deferred_vfree(struct work_struct *work)
+{
+	struct virtio_fb_info *info = container_of(work, struct virtio_fb_info,
+						   vfree_work);
+	int i;
+	unsigned long flags;
+	void *last_buf[MAX_BUF];
+	int idx;
+
+	spin_lock_irqsave(&info->vfree_lock, flags);
+
+	idx = info->last_buf_idx;
+	memcpy(last_buf, info->last_buf, sizeof(void*) * MAX_BUF);
+	info->last_buf_idx = 0;
+
+	spin_unlock_irqrestore(&info->vfree_lock, flags);
+
+	for (i = 0; i < idx; i++) {
+		vfree(last_buf[i]);
+	}
+}
+
+/* Callback when the host kicks our output queue. This can only mean it's done
+ * processing an item, so let's free up the memory occupied by the entries */
+static void virtio_fb_output(struct virtqueue *vq)
+{
+	struct virtio_fb_info *info = dev_get_drvdata(&vq->vdev->dev);
+	int len;
+	void *x;
+
+	while ((x = vq->vq_ops->get_buf(vq, &len)) != NULL) {
+		struct virtio_fb_cmd *cmd = x;
+
+		if (cmd->send_buf) {
+			spin_lock(&info->vfree_lock);
+			if (info->last_buf_idx != MAX_BUF) {
+				info->last_buf[info->last_buf_idx++] =
+					cmd->send_buf;
+			}
+			spin_unlock(&info->vfree_lock);
+
+			schedule_work(&info->vfree_work);
+		}
+
+		kmem_cache_free(info->cmd_cache, x);
+	}
+}
+
+static int __devinit virtio_fb_probe(struct virtio_device *dev)
+{
+	vq_callback_t *callbacks[] = { virtio_fb_input, virtio_fb_output };
+	const char *names[] = { "input", "output" };
+	struct virtio_fb_info *info;
+	struct virtqueue *vqs[2];
+	struct fb_info *fb_info = NULL;
+	int fb_size, res_size;
+	int ret, err, i;
+	char *inbuf;
+
+	err = dev->config->find_vqs(dev, 2, vqs, callbacks, names);
+	if (err) {
+		printk(KERN_ERR "VirtIO FB: couldn't find VQs\n");
+		return err;
+	}
+
+	info = kmalloc(sizeof(*info), GFP_KERNEL);
+	if (info == NULL)
+		return -ENOMEM;
+
+	info->vq_in = vqs[0];
+	info->vq_out = vqs[1];
+
+	res_size = video[KPARAM_WIDTH] * video[KPARAM_HEIGHT] * DEFAULT_DEPTH / 8;
+	fb_size = video[KPARAM_MEM] * 1024 * 1024;
+
+	if (res_size > fb_size) {
+		video[KPARAM_WIDTH] = DEFAULT_WIDTH;
+		video[KPARAM_HEIGHT] = DEFAULT_HEIGHT;
+	}
+
+	dev_set_drvdata(&dev->dev, info);
+	info->vdev = dev;
+
+	info->fb = rvmalloc(fb_size);
+	if (info->fb == NULL)
+		goto error_nomem;
+
+	info->nr_pages = (fb_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	info->size = fb_size;
+
+	fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL);
+	if (fb_info == NULL)
+		goto error_nomem;
+
+	inbuf = kmalloc(PAGE_SIZE * 3, GFP_KERNEL);
+	info->inbuf = inbuf;
+	for (i = 0; i < (3 * PAGE_SIZE); i += PAGE_SIZE) {
+		struct scatterlist sg;
+
+		sg_init_one(&sg, inbuf + i, PAGE_SIZE);
+		info->vq_in->vq_ops->add_buf(info->vq_in, &sg, 0, 1, inbuf + i);
+	}
+	info->vq_in->vq_ops->kick(info->vq_in);
+
+	fb_info->pseudo_palette = fb_info->par;
+	fb_info->par = info;
+
+	fb_info->screen_base = info->fb;
+
+	fb_info->fbops = &virtio_fb_ops;
+	fb_info->var.xres_virtual = fb_info->var.xres = video[KPARAM_WIDTH];
+	fb_info->var.yres_virtual = fb_info->var.yres = video[KPARAM_HEIGHT];
+	fb_info->var.bits_per_pixel = DEFAULT_DEPTH;
+
+	fb_info->var.transp = (struct fb_bitfield){24, 8, 0};
+	fb_info->var.red    = (struct fb_bitfield){16, 8, 0};
+	fb_info->var.green  = (struct fb_bitfield){8,  8, 0};
+	fb_info->var.blue   = (struct fb_bitfield){0,  8, 0};
+
+	fb_info->var.activate = FB_ACTIVATE_NOW;
+	fb_info->var.height = -1;
+	fb_info->var.width = -1;
+	fb_info->var.vmode = FB_VMODE_NONINTERLACED;
+
+	fb_info->fix.visual = FB_VISUAL_TRUECOLOR;
+	fb_info->fix.line_length = fb_info->var.xres * DEFAULT_DEPTH / 8;
+	fb_info->fix.smem_start = 0;
+	fb_info->fix.smem_len = fb_size;
+	strcpy(fb_info->fix.id, "virtio_fb");
+	fb_info->fix.type = FB_TYPE_PACKED_PIXELS;
+	fb_info->fix.accel = FB_ACCEL_NONE;
+
+	fb_info->flags = FBINFO_FLAG_DEFAULT | FBINFO_HWACCEL_COPYAREA |
+			 FBINFO_READS_FAST;
+
+	ret = fb_alloc_cmap(&fb_info->cmap, 256, 0);
+	if (ret < 0) {
+		framebuffer_release(fb_info);
+		goto error;
+	}
+
+	info->cmd_cache = kmem_cache_create("virtio_fb_cmd",
+					    sizeof(struct virtio_fb_cmd),
+					    0, 0, NULL);
+
+	if (!info->cmd_cache) {
+		framebuffer_release(fb_info);
+		goto error;
+	}
+
+	fb_info->fbdefio = &virtio_fb_defio;
+	fb_deferred_io_init(fb_info);
+
+	INIT_WORK(&info->refresh_work, deferred_refresh);
+	INIT_WORK(&info->vfree_work, deferred_vfree);
+	spin_lock_init(&info->vfree_lock);
+
+	ret = register_framebuffer(fb_info);
+	if (ret) {
+		fb_deferred_io_cleanup(fb_info);
+		fb_dealloc_cmap(&fb_info->cmap);
+		framebuffer_release(fb_info);
+		goto error;
+	}
+	info->fb_info = fb_info;
+
+	virtio_fb_make_preferred_console();
+	return 0;
+
+ error_nomem:
+	ret = -ENOMEM;
+ error:
+	framebuffer_release(fb_info);
+	return ret;
+}
+
+static void virtio_fb_apply_config(struct virtio_device *dev)
+{
+}
+
+static struct virtio_device_id id_table[] = {
+	{ VIRTIO_ID_FB, VIRTIO_DEV_ANY_ID },
+	{ 0 },
+};
+
+static unsigned int features[] = {
+};
+
+static struct virtio_driver virtio_console = {
+	.feature_table = features,
+	.feature_table_size = ARRAY_SIZE(features),
+	.driver.name =  KBUILD_MODNAME,
+	.driver.owner = THIS_MODULE,
+	.id_table =     id_table,
+	.probe =        virtio_fb_probe,
+	.config_changed = virtio_fb_apply_config,
+};
+
+static int __init init(void)
+{
+	return register_virtio_driver(&virtio_console);
+}
+module_init(init);
+
+
+MODULE_DEVICE_TABLE(virtio, id_table);
+MODULE_DESCRIPTION("Virtio framebuffer driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
index 06660c0..72e39f7 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_FB		6 /* virtio framebuffer */
 #define VIRTIO_ID_9P		9 /* 9p virtio console */
 
 #endif /* _LINUX_VIRTIO_IDS_H */
-- 
1.6.0.2

Re: [PATCH] Add VirtIO Frame Buffer Support

[Ondrej Zajicek]

[-- Attachment #1.1: Type: text/plain, Size: 826 bytes --]

On Mon, Nov 02, 2009 at 11:09:19PM +0100, Alexander Graf wrote:
> When we want to create a full VirtIO based machine, we're still missing
> graphics output. Fortunately, Linux provides us with most of the frameworks
> to render text and everything, we only need to implement a transport.
> 
> So this is a frame buffer backend written for VirtIO. Using this and my
> patch to qemu, you can use paravirtualized graphics.

Just a note: it might be useful to allow paravirtualized graphics to
handle text mode. In that case, it can be done in fbdev layer using
tileblit operations.

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 399 bytes --]

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference

[-- Attachment #3: Type: text/plain, Size: 182 bytes --]

_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

Am 02.11.2009 um 23:32 schrieb Ondrej Zajicek <santiago@crfreenet.org>:

> On Mon, Nov 02, 2009 at 11:09:19PM +0100, Alexander Graf wrote:
>> When we want to create a full VirtIO based machine, we're still  
>> missing
>> graphics output. Fortunately, Linux provides us with most of the  
>> frameworks
>> to render text and everything, we only need to implement a transport.
>>
>> So this is a frame buffer backend written for VirtIO. Using this  
>> and my
>> patch to qemu, you can use paravirtualized graphics.
>
> Just a note: it might be useful to allow paravirtualized graphics to
> handle text mode. In that case, it can be done in fbdev layer using
> tileblit operations.

Is there any real driver implementing this already? I'd prefer to copy  
from working code instead of writing my own :-).

Also, we still need to keep the local frame buffer copy in sync so we  
can mmap and read from it, right? So it's not really worth it  
probably...

Alex

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference

Re: [Linux-fbdev-devel] [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

On 02.11.2009, at 23:42, Alexander Graf wrote:

>
> Am 02.11.2009 um 23:32 schrieb Ondrej Zajicek  
> <santiago@crfreenet.org>:
>
>> On Mon, Nov 02, 2009 at 11:09:19PM +0100, Alexander Graf wrote:
>>> When we want to create a full VirtIO based machine, we're still
>>> missing
>>> graphics output. Fortunately, Linux provides us with most of the
>>> frameworks
>>> to render text and everything, we only need to implement a  
>>> transport.
>>>
>>> So this is a frame buffer backend written for VirtIO. Using this
>>> and my
>>> patch to qemu, you can use paravirtualized graphics.
>>
>> Just a note: it might be useful to allow paravirtualized graphics to
>> handle text mode. In that case, it can be done in fbdev layer using
>> tileblit operations.
>
> Is there any real driver implementing this already? I'd prefer to copy
> from working code instead of writing my own :-).
>
> Also, we still need to keep the local frame buffer copy in sync so we
> can mmap and read from it, right? So it's not really worth it
> probably...

But then again we could just try to be closer to a real graphics card.  
What if we'd set up a memory region on the host that is basically our  
graphics frame buffer? For S390 we could just append the graphics  
memory to the guest's memory.

We could use that as backing buffer in the qemu graphics frontend and  
as frame buffer in the Linux fbdev layer, similar to what real  
graphics cards set up.

Then we could send all those fancy commands that we have already over  
to the host, that renders them and thanks to the mapping have a  
consistent frame buffer we can mmap. It'd even simplify the deferred  
IO stuff, making it basically a notify that something changed, but the  
changes already being written to the frame buffer.

On sync we'd just have to make sure the virtio buffer was processed  
completely.


That would get rid of all sys_* calls, a lot of copying and the  
duplicate frame buffer we have right now. Wow.

Alex

[Qemu-devel] Re: [Linux-fbdev-devel] [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

On 02.11.2009, at 23:42, Alexander Graf wrote:

>
> Am 02.11.2009 um 23:32 schrieb Ondrej Zajicek  
> <santiago@crfreenet.org>:
>
>> On Mon, Nov 02, 2009 at 11:09:19PM +0100, Alexander Graf wrote:
>>> When we want to create a full VirtIO based machine, we're still
>>> missing
>>> graphics output. Fortunately, Linux provides us with most of the
>>> frameworks
>>> to render text and everything, we only need to implement a  
>>> transport.
>>>
>>> So this is a frame buffer backend written for VirtIO. Using this
>>> and my
>>> patch to qemu, you can use paravirtualized graphics.
>>
>> Just a note: it might be useful to allow paravirtualized graphics to
>> handle text mode. In that case, it can be done in fbdev layer using
>> tileblit operations.
>
> Is there any real driver implementing this already? I'd prefer to copy
> from working code instead of writing my own :-).
>
> Also, we still need to keep the local frame buffer copy in sync so we
> can mmap and read from it, right? So it's not really worth it
> probably...

But then again we could just try to be closer to a real graphics card.  
What if we'd set up a memory region on the host that is basically our  
graphics frame buffer? For S390 we could just append the graphics  
memory to the guest's memory.

We could use that as backing buffer in the qemu graphics frontend and  
as frame buffer in the Linux fbdev layer, similar to what real  
graphics cards set up.

Then we could send all those fancy commands that we have already over  
to the host, that renders them and thanks to the mapping have a  
consistent frame buffer we can mmap. It'd even simplify the deferred  
IO stuff, making it basically a notify that something changed, but the  
changes already being written to the frame buffer.

On sync we'd just have to make sure the virtio buffer was processed  
completely.


That would get rid of all sys_* calls, a lot of copying and the  
duplicate frame buffer we have right now. Wow.

Alex

Re: [PATCH] Add VirtIO Frame Buffer Support

[Ondrej Zajicek]

[-- Attachment #1.1: Type: text/plain, Size: 1184 bytes --]

On Tue, Nov 03, 2009 at 12:24:15AM +0100, Alexander Graf wrote:
>> Also, we still need to keep the local frame buffer copy in sync so we
>> can mmap and read from it, right? So it's not really worth it
>> probably...
>
> But then again we could just try to be closer to a real graphics card.  
> What if we'd set up a memory region on the host that is basically our  
> graphics frame buffer? For S390 we could just append the graphics memory 
> to the guest's memory.
>
> We could use that as backing buffer in the qemu graphics frontend and as 
> frame buffer in the Linux fbdev layer, similar to what real graphics 
> cards set up.

Using shared memory pages between host and guest seems like a natural
way to implement paravirtualized graphics card. Most things are
straightforward, only a little problematic thing is when fbdev 
is mmaped from guest to userspace - you have to detect writes
and notify host that it changed.

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 399 bytes --]

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference

[-- Attachment #3: Type: text/plain, Size: 182 bytes --]

_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

[Qemu-devel] Re: [Linux-fbdev-devel] [PATCH] Add VirtIO Frame Buffer Support

[Ondrej Zajicek]

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

On Tue, Nov 03, 2009 at 12:24:15AM +0100, Alexander Graf wrote:
>> Also, we still need to keep the local frame buffer copy in sync so we
>> can mmap and read from it, right? So it's not really worth it
>> probably...
>
> But then again we could just try to be closer to a real graphics card.  
> What if we'd set up a memory region on the host that is basically our  
> graphics frame buffer? For S390 we could just append the graphics memory 
> to the guest's memory.
>
> We could use that as backing buffer in the qemu graphics frontend and as 
> frame buffer in the Linux fbdev layer, similar to what real graphics 
> cards set up.

Using shared memory pages between host and guest seems like a natural
way to implement paravirtualized graphics card. Most things are
straightforward, only a little problematic thing is when fbdev 
is mmaped from guest to userspace - you have to detect writes
and notify host that it changed.

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Linux-fbdev-devel] [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

On 03.11.2009, at 00:57, Ondrej Zajicek wrote:

> On Tue, Nov 03, 2009 at 12:24:15AM +0100, Alexander Graf wrote:
>>> Also, we still need to keep the local frame buffer copy in sync so  
>>> we
>>> can mmap and read from it, right? So it's not really worth it
>>> probably...
>>
>> But then again we could just try to be closer to a real graphics  
>> card.
>> What if we'd set up a memory region on the host that is basically our
>> graphics frame buffer? For S390 we could just append the graphics  
>> memory
>> to the guest's memory.
>>
>> We could use that as backing buffer in the qemu graphics frontend  
>> and as
>> frame buffer in the Linux fbdev layer, similar to what real graphics
>> cards set up.
>
> Using shared memory pages between host and guest seems like a natural
> way to implement paravirtualized graphics card. Most things are
> straightforward, only a little problematic thing is when fbdev
> is mmaped from guest to userspace - you have to detect writes
> and notify host that it changed.

Yes, currently I use the deferred IO stuff and send over "write"  
commands with the full contents of the frame buffer.

But having a shared frame buffer really sounds better.
The major thing I really want to do different from xenfb though is  
that the _host_ renders. That way "copy" operations still work as  
designed, enabling vnc to pass them through directly. That might also  
allow for some sort of DRI interface to pass through 3d data to the  
host... at least in the future :-).


Alex

[Qemu-devel] Re: [Linux-fbdev-devel] [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

On 03.11.2009, at 00:57, Ondrej Zajicek wrote:

> On Tue, Nov 03, 2009 at 12:24:15AM +0100, Alexander Graf wrote:
>>> Also, we still need to keep the local frame buffer copy in sync so  
>>> we
>>> can mmap and read from it, right? So it's not really worth it
>>> probably...
>>
>> But then again we could just try to be closer to a real graphics  
>> card.
>> What if we'd set up a memory region on the host that is basically our
>> graphics frame buffer? For S390 we could just append the graphics  
>> memory
>> to the guest's memory.
>>
>> We could use that as backing buffer in the qemu graphics frontend  
>> and as
>> frame buffer in the Linux fbdev layer, similar to what real graphics
>> cards set up.
>
> Using shared memory pages between host and guest seems like a natural
> way to implement paravirtualized graphics card. Most things are
> straightforward, only a little problematic thing is when fbdev
> is mmaped from guest to userspace - you have to detect writes
> and notify host that it changed.

Yes, currently I use the deferred IO stuff and send over "write"  
commands with the full contents of the frame buffer.

But having a shared frame buffer really sounds better.
The major thing I really want to do different from xenfb though is  
that the _host_ renders. That way "copy" operations still work as  
designed, enabling vnc to pass them through directly. That might also  
allow for some sort of DRI interface to pass through 3d data to the  
host... at least in the future :-).


Alex

Re: [PATCH] Add VirtIO Frame Buffer Support

[Ondrej Zajicek]

[-- Attachment #1.1: Type: text/plain, Size: 1497 bytes --]

On Mon, Nov 02, 2009 at 11:42:11PM +0100, Alexander Graf wrote:
>
> Am 02.11.2009 um 23:32 schrieb Ondrej Zajicek <santiago@crfreenet.org>:
>
>> On Mon, Nov 02, 2009 at 11:09:19PM +0100, Alexander Graf wrote:
>>> When we want to create a full VirtIO based machine, we're still  
>>> missing
>>> graphics output. Fortunately, Linux provides us with most of the  
>>> frameworks
>>> to render text and everything, we only need to implement a transport.
>>>
>>> So this is a frame buffer backend written for VirtIO. Using this and 
>>> my
>>> patch to qemu, you can use paravirtualized graphics.
>>
>> Just a note: it might be useful to allow paravirtualized graphics to
>> handle text mode. In that case, it can be done in fbdev layer using
>> tileblit operations.
>
> Is there any real driver implementing this already? I'd prefer to copy  
> from working code instead of writing my own :-).

Yes, for example s3fb and vt8623fb (struct fb_tile_ops).

> Also, we still need to keep the local frame buffer copy in sync so we  
> can mmap and read from it, right? So it's not really worth it  
> probably...

Advantages might be on Qemu side (like ability to run in text-only
mode without need to change console on virtualized sytem).

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 399 bytes --]

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference

[-- Attachment #3: Type: text/plain, Size: 182 bytes --]

_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

On 03.11.2009, at 00:51, Ondrej Zajicek wrote:

> On Mon, Nov 02, 2009 at 11:42:11PM +0100, Alexander Graf wrote:
>>
>> Am 02.11.2009 um 23:32 schrieb Ondrej Zajicek  
>> <santiago@crfreenet.org>:
>>
>>> On Mon, Nov 02, 2009 at 11:09:19PM +0100, Alexander Graf wrote:
>>>> When we want to create a full VirtIO based machine, we're still
>>>> missing
>>>> graphics output. Fortunately, Linux provides us with most of the
>>>> frameworks
>>>> to render text and everything, we only need to implement a  
>>>> transport.
>>>>
>>>> So this is a frame buffer backend written for VirtIO. Using this  
>>>> and
>>>> my
>>>> patch to qemu, you can use paravirtualized graphics.
>>>
>>> Just a note: it might be useful to allow paravirtualized graphics to
>>> handle text mode. In that case, it can be done in fbdev layer using
>>> tileblit operations.
>>
>> Is there any real driver implementing this already? I'd prefer to  
>> copy
>> from working code instead of writing my own :-).
>
> Yes, for example s3fb and vt8623fb (struct fb_tile_ops).
>
>> Also, we still need to keep the local frame buffer copy in sync so we
>> can mmap and read from it, right? So it's not really worth it
>> probably...
>
> Advantages might be on Qemu side (like ability to run in text-only
> mode without need to change console on virtualized sytem).

Oh, so we could use the curses frontend. That's a neat idea.

But that also means we'll lose the penguin, right?


Alex

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference

Re: [PATCH] Add VirtIO Frame Buffer Support

[Ondrej Zajicek]

[-- Attachment #1.1: Type: text/plain, Size: 570 bytes --]

On Tue, Nov 03, 2009 at 12:53:08AM +0100, Alexander Graf wrote:
>> Advantages might be on Qemu side (like ability to run in text-only
>> mode without need to change console on virtualized sytem).
>
> Oh, so we could use the curses frontend. That's a neat idea.
>
> But that also means we'll lose the penguin, right?

Yes :-)

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 399 bytes --]

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference

[-- Attachment #3: Type: text/plain, Size: 182 bytes --]

_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 12:09 AM, Alexander Graf wrote:
> When we want to create a full VirtIO based machine, we're still missing
> graphics output. Fortunately, Linux provides us with most of the frameworks
> to render text and everything, we only need to implement a transport.
>
> So this is a frame buffer backend written for VirtIO. Using this and my
> patch to qemu, you can use paravirtualized graphics.
>
>    

What does this do that cirrus and/or vmware-vga don't?

> This is especially important on machines that can't do MMIO, as all current
> graphics implementations qemu emulates I'm aware of so far fail here.
>    

s390 virtual desktops?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 12:09 AM, Alexander Graf wrote:
> When we want to create a full VirtIO based machine, we're still missing
> graphics output. Fortunately, Linux provides us with most of the frameworks
> to render text and everything, we only need to implement a transport.
>
> So this is a frame buffer backend written for VirtIO. Using this and my
> patch to qemu, you can use paravirtualized graphics.
>
>    

What does this do that cirrus and/or vmware-vga don't?

> This is especially important on machines that can't do MMIO, as all current
> graphics implementations qemu emulates I'm aware of so far fail here.
>    

s390 virtual desktops?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

On 03.11.2009, at 07:21, Avi Kivity wrote:

> On 11/03/2009 12:09 AM, Alexander Graf wrote:
>> When we want to create a full VirtIO based machine, we're still  
>> missing
>> graphics output. Fortunately, Linux provides us with most of the  
>> frameworks
>> to render text and everything, we only need to implement a transport.
>>
>> So this is a frame buffer backend written for VirtIO. Using this  
>> and my
>> patch to qemu, you can use paravirtualized graphics.
>>
>>
>
> What does this do that cirrus and/or vmware-vga don't?

Work on non-MMIO machines.

>> This is especially important on machines that can't do MMIO, as all  
>> current
>> graphics implementations qemu emulates I'm aware of so far fail here.
>>
>
> s390 virtual desktops?

Well - have you ever tried installing SLES / RHEL on an S390? If not,  
give it a shot and comment again :-)

Alex

[Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

On 03.11.2009, at 07:21, Avi Kivity wrote:

> On 11/03/2009 12:09 AM, Alexander Graf wrote:
>> When we want to create a full VirtIO based machine, we're still  
>> missing
>> graphics output. Fortunately, Linux provides us with most of the  
>> frameworks
>> to render text and everything, we only need to implement a transport.
>>
>> So this is a frame buffer backend written for VirtIO. Using this  
>> and my
>> patch to qemu, you can use paravirtualized graphics.
>>
>>
>
> What does this do that cirrus and/or vmware-vga don't?

Work on non-MMIO machines.

>> This is especially important on machines that can't do MMIO, as all  
>> current
>> graphics implementations qemu emulates I'm aware of so far fail here.
>>
>
> s390 virtual desktops?

Well - have you ever tried installing SLES / RHEL on an S390? If not,  
give it a shot and comment again :-)

Alex

Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 08:22 AM, Alexander Graf wrote:
>
>>> This is especially important on machines that can't do MMIO, as all 
>>> current
>>> graphics implementations qemu emulates I'm aware of so far fail here.
>>>
>>
>> s390 virtual desktops?
>
> Well - have you ever tried installing SLES / RHEL on an S390? If not, 
> give it a shot and comment again :-)
>

I won't be able to until qemu support s390.

How does it work today?

Does installation over vnc work?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 08:22 AM, Alexander Graf wrote:
>
>>> This is especially important on machines that can't do MMIO, as all 
>>> current
>>> graphics implementations qemu emulates I'm aware of so far fail here.
>>>
>>
>> s390 virtual desktops?
>
> Well - have you ever tried installing SLES / RHEL on an S390? If not, 
> give it a shot and comment again :-)
>

I won't be able to until qemu support s390.

How does it work today?

Does installation over vnc work?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

On 03.11.2009, at 07:24, Avi Kivity wrote:

> On 11/03/2009 08:22 AM, Alexander Graf wrote:
>>
>>>> This is especially important on machines that can't do MMIO, as  
>>>> all current
>>>> graphics implementations qemu emulates I'm aware of so far fail  
>>>> here.
>>>>
>>>
>>> s390 virtual desktops?
>>
>> Well - have you ever tried installing SLES / RHEL on an S390? If  
>> not, give it a shot and comment again :-)
>>
>
> I won't be able to until qemu support s390.

I'm pretty sure RH has some S390 lying around ...

> How does it work today?

You boot into a TERM=dumb line based emulation on 3270 (worst thing  
haunting people's nightmares ever), trying to get out of that mode as  
quickly as possible and off into SSH / VNC.

> Does installation over vnc work?

Yes, but it requires a working network setup. That's a pretty  
requirement on guests IMHO.

Alex

[Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

On 03.11.2009, at 07:24, Avi Kivity wrote:

> On 11/03/2009 08:22 AM, Alexander Graf wrote:
>>
>>>> This is especially important on machines that can't do MMIO, as  
>>>> all current
>>>> graphics implementations qemu emulates I'm aware of so far fail  
>>>> here.
>>>>
>>>
>>> s390 virtual desktops?
>>
>> Well - have you ever tried installing SLES / RHEL on an S390? If  
>> not, give it a shot and comment again :-)
>>
>
> I won't be able to until qemu support s390.

I'm pretty sure RH has some S390 lying around ...

> How does it work today?

You boot into a TERM=dumb line based emulation on 3270 (worst thing  
haunting people's nightmares ever), trying to get out of that mode as  
quickly as possible and off into SSH / VNC.

> Does installation over vnc work?

Yes, but it requires a working network setup. That's a pretty  
requirement on guests IMHO.

Alex

Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 08:27 AM, Alexander Graf wrote:
>
>> How does it work today?
>
> You boot into a TERM=dumb line based emulation on 3270 (worst thing 
> haunting people's nightmares ever), trying to get out of that mode as 
> quickly as possible and off into SSH / VNC.

Despite the coolness factor, IMO a few minutes during install time do 
not justify a new hardware model and a new driver.

>> Does installation over vnc work?
>
> Yes, but it requires a working network setup. That's a pretty 
> requirement on guests IMHO.

Why?  the guest will typically have networking when it's set up, so it 
should have network access during install.  You can easily use slirp 
redirection and the built-in dhcp server to set this up with relatively 
few hassles.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 08:27 AM, Alexander Graf wrote:
>
>> How does it work today?
>
> You boot into a TERM=dumb line based emulation on 3270 (worst thing 
> haunting people's nightmares ever), trying to get out of that mode as 
> quickly as possible and off into SSH / VNC.

Despite the coolness factor, IMO a few minutes during install time do 
not justify a new hardware model and a new driver.

>> Does installation over vnc work?
>
> Yes, but it requires a working network setup. That's a pretty 
> requirement on guests IMHO.

Why?  the guest will typically have networking when it's set up, so it 
should have network access during install.  You can easily use slirp 
redirection and the built-in dhcp server to set this up with relatively 
few hassles.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

On 03.11.2009, at 07:34, Avi Kivity wrote:

> On 11/03/2009 08:27 AM, Alexander Graf wrote:
>>
>>> How does it work today?
>>
>> You boot into a TERM=dumb line based emulation on 3270 (worst thing  
>> haunting people's nightmares ever), trying to get out of that mode  
>> as quickly as possible and off into SSH / VNC.
>
> Despite the coolness factor, IMO a few minutes during install time  
> do not justify a new hardware model and a new driver.

It's more than just coolness factor. There are use cases out there (www.susestudio.com 
) that don't want to rely on the guest exporting a VNC server to the  
outside just to access graphics. You also want to see boot messages,  
have a console login screen, be able to debug things without switching  
between virtio-console and vnc, etc. etc.

The hardware model isn't exactly new either. It's just the next  
logical step to a full PV machine using virtio. If the virtio-fb stuff  
turns out to be really fast and reliable, I could even imagine it  
being the default target for kvm on ppc as well, as we can't switch  
resolutions on the fly there atm.

>>> Does installation over vnc work?
>>
>> Yes, but it requires a working network setup. That's a pretty  
>> requirement on guests IMHO.
>
> Why?  the guest will typically have networking when it's set up, so  
> it should have network access during install.  You can easily use  
> slirp redirection and the built-in dhcp server to set this up with  
> relatively few hassles.

That's how I use it right now. It's no fun.


Alex

[Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

On 03.11.2009, at 07:34, Avi Kivity wrote:

> On 11/03/2009 08:27 AM, Alexander Graf wrote:
>>
>>> How does it work today?
>>
>> You boot into a TERM=dumb line based emulation on 3270 (worst thing  
>> haunting people's nightmares ever), trying to get out of that mode  
>> as quickly as possible and off into SSH / VNC.
>
> Despite the coolness factor, IMO a few minutes during install time  
> do not justify a new hardware model and a new driver.

It's more than just coolness factor. There are use cases out there (www.susestudio.com 
) that don't want to rely on the guest exporting a VNC server to the  
outside just to access graphics. You also want to see boot messages,  
have a console login screen, be able to debug things without switching  
between virtio-console and vnc, etc. etc.

The hardware model isn't exactly new either. It's just the next  
logical step to a full PV machine using virtio. If the virtio-fb stuff  
turns out to be really fast and reliable, I could even imagine it  
being the default target for kvm on ppc as well, as we can't switch  
resolutions on the fly there atm.

>>> Does installation over vnc work?
>>
>> Yes, but it requires a working network setup. That's a pretty  
>> requirement on guests IMHO.
>
> Why?  the guest will typically have networking when it's set up, so  
> it should have network access during install.  You can easily use  
> slirp redirection and the built-in dhcp server to set this up with  
> relatively few hassles.

That's how I use it right now. It's no fun.


Alex

Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 08:39 AM, Alexander Graf wrote:
>
> On 03.11.2009, at 07:34, Avi Kivity wrote:
>
>> On 11/03/2009 08:27 AM, Alexander Graf wrote:
>>>
>>>> How does it work today?
>>>
>>> You boot into a TERM=dumb line based emulation on 3270 (worst thing 
>>> haunting people's nightmares ever), trying to get out of that mode 
>>> as quickly as possible and off into SSH / VNC.
>>
>> Despite the coolness factor, IMO a few minutes during install time do 
>> not justify a new hardware model and a new driver.
>
> It's more than just coolness factor. There are use cases out there 
> (www.susestudio.com) that don't want to rely on the guest exporting a 
> VNC server to the outside just to access graphics. 

Instead you rely on the guest using virtio-fb. Since we have to make 
guest modifications, why not go for the simpler ones?

> You also want to see boot messages, have a console login screen, 

virtio-console does that, except for the penguins.  Better, since you 
can scroll back.

> be able to debug things without switching between virtio-console and 
> vnc, etc. etc.

Render virtio-console on your vnc session.  We do that already, no? 
(well, the host's vnc session, not the guest's).

> The hardware model isn't exactly new either. It's just the next 
> logical step to a full PV machine using virtio. If the virtio-fb stuff 
> turns out to be really fast and reliable, I could even imagine it 
> being the default target for kvm on ppc as well, as we can't switch 
> resolutions on the fly there atm.
>

We could with vmware-vga.

>>
>> Why?  the guest will typically have networking when it's set up, so 
>> it should have network access during install.  You can easily use 
>> slirp redirection and the built-in dhcp server to set this up with 
>> relatively few hassles.
>
> That's how I use it right now. It's no fun.
>

The toolstack should hide the unfun parts.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 08:39 AM, Alexander Graf wrote:
>
> On 03.11.2009, at 07:34, Avi Kivity wrote:
>
>> On 11/03/2009 08:27 AM, Alexander Graf wrote:
>>>
>>>> How does it work today?
>>>
>>> You boot into a TERM=dumb line based emulation on 3270 (worst thing 
>>> haunting people's nightmares ever), trying to get out of that mode 
>>> as quickly as possible and off into SSH / VNC.
>>
>> Despite the coolness factor, IMO a few minutes during install time do 
>> not justify a new hardware model and a new driver.
>
> It's more than just coolness factor. There are use cases out there 
> (www.susestudio.com) that don't want to rely on the guest exporting a 
> VNC server to the outside just to access graphics. 

Instead you rely on the guest using virtio-fb. Since we have to make 
guest modifications, why not go for the simpler ones?

> You also want to see boot messages, have a console login screen, 

virtio-console does that, except for the penguins.  Better, since you 
can scroll back.

> be able to debug things without switching between virtio-console and 
> vnc, etc. etc.

Render virtio-console on your vnc session.  We do that already, no? 
(well, the host's vnc session, not the guest's).

> The hardware model isn't exactly new either. It's just the next 
> logical step to a full PV machine using virtio. If the virtio-fb stuff 
> turns out to be really fast and reliable, I could even imagine it 
> being the default target for kvm on ppc as well, as we can't switch 
> resolutions on the fly there atm.
>

We could with vmware-vga.

>>
>> Why?  the guest will typically have networking when it's set up, so 
>> it should have network access during install.  You can easily use 
>> slirp redirection and the built-in dhcp server to set this up with 
>> relatively few hassles.
>
> That's how I use it right now. It's no fun.
>

The toolstack should hide the unfun parts.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

On 03.11.2009, at 08:43, Avi Kivity wrote:

> On 11/03/2009 08:39 AM, Alexander Graf wrote:
>>
>> On 03.11.2009, at 07:34, Avi Kivity wrote:
>>
>>> On 11/03/2009 08:27 AM, Alexander Graf wrote:
>>>>
>>>>> How does it work today?
>>>>
>>>> You boot into a TERM=dumb line based emulation on 3270 (worst  
>>>> thing haunting people's nightmares ever), trying to get out of  
>>>> that mode as quickly as possible and off into SSH / VNC.
>>>
>>> Despite the coolness factor, IMO a few minutes during install time  
>>> do not justify a new hardware model and a new driver.
>>
>> It's more than just coolness factor. There are use cases out there (www.susestudio.com 
>> ) that don't want to rely on the guest exporting a VNC server to  
>> the outside just to access graphics.
>
> Instead you rely on the guest using virtio-fb. Since we have to make  
> guest modifications, why not go for the simpler ones?

Ok, imagine this was not this unloved S390 odd architecture but X86.  
The only output choices you have are:

1) virtio-console
2) VNC / SSH over network
3) virtio-fb

Now you want to configure a server, probably using yast and all those  
nice graphical utilities, but still enable a firewall so people  
outside don't intrude your machine. Well, you managed to configure the  
firewall by luck to allow VNC, but now you reconfigured it and  
something broke - but VNC was your only chance to access the machine.  
Oops...

>> You also want to see boot messages, have a console login screen,
>
> virtio-console does that, except for the penguins.  Better, since  
> you can scroll back.

It doesn't do graphics. Ever used yast in text mode?

>> be able to debug things without switching between virtio-console  
>> and vnc, etc. etc.
>
> Render virtio-console on your vnc session.  We do that already, no?  
> (well, the host's vnc session, not the guest's).

Yes, we do that. Still doesn't buy you graphics.

>> The hardware model isn't exactly new either. It's just the next  
>> logical step to a full PV machine using virtio. If the virtio-fb  
>> stuff turns out to be really fast and reliable, I could even  
>> imagine it being the default target for kvm on ppc as well, as we  
>> can't switch resolutions on the fly there atm.
>>
>
> We could with vmware-vga.

The vmware-port stuff is pretty much tied onto X86. I don't think  
modifying EAX is that easy on PPC ;-).

>>> Why?  the guest will typically have networking when it's set up,  
>>> so it should have network access during install.  You can easily  
>>> use slirp redirection and the built-in dhcp server to set this up  
>>> with relatively few hassles.
>>
>> That's how I use it right now. It's no fun.
>>
>
> The toolstack should hide the unfun parts.

You can't hide guest configuration. We as a distribution control the  
kernel. We don't control the user's configuration as that's by design  
the user's choice. The only thing we can do is give users meaningful  
choices to choose from - and having graphics available is definitely  
one of them.

Seriously, try to ask someone internally to get access to an S390. I  
think you'll understand my motivations a lot better after having used  
it for a bit.

Alex

[Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

On 03.11.2009, at 08:43, Avi Kivity wrote:

> On 11/03/2009 08:39 AM, Alexander Graf wrote:
>>
>> On 03.11.2009, at 07:34, Avi Kivity wrote:
>>
>>> On 11/03/2009 08:27 AM, Alexander Graf wrote:
>>>>
>>>>> How does it work today?
>>>>
>>>> You boot into a TERM=dumb line based emulation on 3270 (worst  
>>>> thing haunting people's nightmares ever), trying to get out of  
>>>> that mode as quickly as possible and off into SSH / VNC.
>>>
>>> Despite the coolness factor, IMO a few minutes during install time  
>>> do not justify a new hardware model and a new driver.
>>
>> It's more than just coolness factor. There are use cases out there (www.susestudio.com 
>> ) that don't want to rely on the guest exporting a VNC server to  
>> the outside just to access graphics.
>
> Instead you rely on the guest using virtio-fb. Since we have to make  
> guest modifications, why not go for the simpler ones?

Ok, imagine this was not this unloved S390 odd architecture but X86.  
The only output choices you have are:

1) virtio-console
2) VNC / SSH over network
3) virtio-fb

Now you want to configure a server, probably using yast and all those  
nice graphical utilities, but still enable a firewall so people  
outside don't intrude your machine. Well, you managed to configure the  
firewall by luck to allow VNC, but now you reconfigured it and  
something broke - but VNC was your only chance to access the machine.  
Oops...

>> You also want to see boot messages, have a console login screen,
>
> virtio-console does that, except for the penguins.  Better, since  
> you can scroll back.

It doesn't do graphics. Ever used yast in text mode?

>> be able to debug things without switching between virtio-console  
>> and vnc, etc. etc.
>
> Render virtio-console on your vnc session.  We do that already, no?  
> (well, the host's vnc session, not the guest's).

Yes, we do that. Still doesn't buy you graphics.

>> The hardware model isn't exactly new either. It's just the next  
>> logical step to a full PV machine using virtio. If the virtio-fb  
>> stuff turns out to be really fast and reliable, I could even  
>> imagine it being the default target for kvm on ppc as well, as we  
>> can't switch resolutions on the fly there atm.
>>
>
> We could with vmware-vga.

The vmware-port stuff is pretty much tied onto X86. I don't think  
modifying EAX is that easy on PPC ;-).

>>> Why?  the guest will typically have networking when it's set up,  
>>> so it should have network access during install.  You can easily  
>>> use slirp redirection and the built-in dhcp server to set this up  
>>> with relatively few hassles.
>>
>> That's how I use it right now. It's no fun.
>>
>
> The toolstack should hide the unfun parts.

You can't hide guest configuration. We as a distribution control the  
kernel. We don't control the user's configuration as that's by design  
the user's choice. The only thing we can do is give users meaningful  
choices to choose from - and having graphics available is definitely  
one of them.

Seriously, try to ask someone internally to get access to an S390. I  
think you'll understand my motivations a lot better after having used  
it for a bit.

Alex

Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 09:50 AM, Alexander Graf wrote:
>
> Ok, imagine this was not this unloved S390 odd architecture but X86. 
> The only output choices you have are:
>
> 1) virtio-console
> 2) VNC / SSH over network
> 3) virtio-fb
>
> Now you want to configure a server, probably using yast and all those 
> nice graphical utilities, but still enable a firewall so people 
> outside don't intrude your machine. Well, you managed to configure the 
> firewall by luck to allow VNC, but now you reconfigured it and 
> something broke - but VNC was your only chance to access the machine. 
> Oops...

x86 has real framebuffers, so software and people expect it.  s390 
doesn't.  How do people manage now?

>>> You also want to see boot messages, have a console login screen,
>>
>> virtio-console does that, except for the penguins.  Better, since you 
>> can scroll back.
>
> It doesn't do graphics. Ever used yast in text mode?

Once you're in, start ssh+X or vnc.  Again, what do people do now?

>
>>> The hardware model isn't exactly new either. It's just the next 
>>> logical step to a full PV machine using virtio. If the virtio-fb 
>>> stuff turns out to be really fast and reliable, I could even imagine 
>>> it being the default target for kvm on ppc as well, as we can't 
>>> switch resolutions on the fly there atm.
>>>
>>
>> We could with vmware-vga.
>
> The vmware-port stuff is pretty much tied onto X86. I don't think 
> modifying EAX is that easy on PPC ;-).

Yes, though we can probably make it work on ppc with minimal modifications.

>>>> Why?  the guest will typically have networking when it's set up, so 
>>>> it should have network access during install.  You can easily use 
>>>> slirp redirection and the built-in dhcp server to set this up with 
>>>> relatively few hassles.
>>>
>>> That's how I use it right now. It's no fun.
>>>
>>
>> The toolstack should hide the unfun parts.
>
> You can't hide guest configuration. We as a distribution control the 
> kernel. We don't control the user's configuration as that's by design 
> the user's choice. The only thing we can do is give users meaningful 
> choices to choose from - and having graphics available is definitely 
> one of them.

Well, if the user chooses not to have networking then vnc or ssh+x 
definitely fail.  That would be a strange choice for a server machine.

> Seriously, try to ask someone internally to get access to an S390. I 
> think you'll understand my motivations a lot better after having used 
> it for a bit.

I actually have a s390 vm (RHEL 4 IIRC).  It acts just like any other 
remote machine over ssh except that it's especially slow (probably the 
host is overloaded).  Of course I wouldn't dream of trying to install 
something like that though.

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

[Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 09:50 AM, Alexander Graf wrote:
>
> Ok, imagine this was not this unloved S390 odd architecture but X86. 
> The only output choices you have are:
>
> 1) virtio-console
> 2) VNC / SSH over network
> 3) virtio-fb
>
> Now you want to configure a server, probably using yast and all those 
> nice graphical utilities, but still enable a firewall so people 
> outside don't intrude your machine. Well, you managed to configure the 
> firewall by luck to allow VNC, but now you reconfigured it and 
> something broke - but VNC was your only chance to access the machine. 
> Oops...

x86 has real framebuffers, so software and people expect it.  s390 
doesn't.  How do people manage now?

>>> You also want to see boot messages, have a console login screen,
>>
>> virtio-console does that, except for the penguins.  Better, since you 
>> can scroll back.
>
> It doesn't do graphics. Ever used yast in text mode?

Once you're in, start ssh+X or vnc.  Again, what do people do now?

>
>>> The hardware model isn't exactly new either. It's just the next 
>>> logical step to a full PV machine using virtio. If the virtio-fb 
>>> stuff turns out to be really fast and reliable, I could even imagine 
>>> it being the default target for kvm on ppc as well, as we can't 
>>> switch resolutions on the fly there atm.
>>>
>>
>> We could with vmware-vga.
>
> The vmware-port stuff is pretty much tied onto X86. I don't think 
> modifying EAX is that easy on PPC ;-).

Yes, though we can probably make it work on ppc with minimal modifications.

>>>> Why?  the guest will typically have networking when it's set up, so 
>>>> it should have network access during install.  You can easily use 
>>>> slirp redirection and the built-in dhcp server to set this up with 
>>>> relatively few hassles.
>>>
>>> That's how I use it right now. It's no fun.
>>>
>>
>> The toolstack should hide the unfun parts.
>
> You can't hide guest configuration. We as a distribution control the 
> kernel. We don't control the user's configuration as that's by design 
> the user's choice. The only thing we can do is give users meaningful 
> choices to choose from - and having graphics available is definitely 
> one of them.

Well, if the user chooses not to have networking then vnc or ssh+x 
definitely fail.  That would be a strange choice for a server machine.

> Seriously, try to ask someone internally to get access to an S390. I 
> think you'll understand my motivations a lot better after having used 
> it for a bit.

I actually have a s390 vm (RHEL 4 IIRC).  It acts just like any other 
remote machine over ssh except that it's especially slow (probably the 
host is overloaded).  Of course I wouldn't dream of trying to install 
something like that though.

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

Re: [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

On 03.11.2009, at 09:20, Avi Kivity wrote:

> On 11/03/2009 09:50 AM, Alexander Graf wrote:
>>
>> Ok, imagine this was not this unloved S390 odd architecture but  
>> X86. The only output choices you have are:
>>
>> 1) virtio-console
>> 2) VNC / SSH over network
>> 3) virtio-fb
>>
>> Now you want to configure a server, probably using yast and all  
>> those nice graphical utilities, but still enable a firewall so  
>> people outside don't intrude your machine. Well, you managed to  
>> configure the firewall by luck to allow VNC, but now you  
>> reconfigured it and something broke - but VNC was your only chance  
>> to access the machine. Oops...
>
> x86 has real framebuffers, so software and people expect it.  s390  
> doesn't.  How do people manage now?

They cope with what's there. Fortunately we're in a position to change  
things, so we don't have to stick with the worse.

>>>> You also want to see boot messages, have a console login screen,
>>>
>>> virtio-console does that, except for the penguins.  Better, since  
>>> you can scroll back.
>>
>> It doesn't do graphics. Ever used yast in text mode?
>
> Once you're in, start ssh+X or vnc.  Again, what do people do now?

Exactly that. Again, it works but is not ideal. If we can improve user  
experience why work against it?

>>>> The hardware model isn't exactly new either. It's just the next  
>>>> logical step to a full PV machine using virtio. If the virtio-fb  
>>>> stuff turns out to be really fast and reliable, I could even  
>>>> imagine it being the default target for kvm on ppc as well, as we  
>>>> can't switch resolutions on the fly there atm.
>>>>
>>>
>>> We could with vmware-vga.
>>
>> The vmware-port stuff is pretty much tied onto X86. I don't think  
>> modifying EAX is that easy on PPC ;-).
>
> Yes, though we can probably make it work on ppc with minimal  
> modifications.

Is it worth it? We can also just implement a virtio mouse event dev  
plus fb and be good. That way we control the whole stack without  
risking to break vmware.

>>>>> Why?  the guest will typically have networking when it's set up,  
>>>>> so it should have network access during install.  You can easily  
>>>>> use slirp redirection and the built-in dhcp server to set this  
>>>>> up with relatively few hassles.
>>>>
>>>> That's how I use it right now. It's no fun.
>>>>
>>>
>>> The toolstack should hide the unfun parts.
>>
>> You can't hide guest configuration. We as a distribution control  
>> the kernel. We don't control the user's configuration as that's by  
>> design the user's choice. The only thing we can do is give users  
>> meaningful choices to choose from - and having graphics available  
>> is definitely one of them.
>
> Well, if the user chooses not to have networking then vnc or ssh+x  
> definitely fail.  That would be a strange choice for a server machine.

It's actually rather common on S390, though admittedly not that much  
on Linux+S390. There are more ways for inter node communication than  
networking. You can talk to another VM on the same machine without any  
network whatsoever. That way you can set up an isolated job (your bank  
transfer database for example) that is always protected by a proxy to  
the outside world.

>> Seriously, try to ask someone internally to get access to an S390.  
>> I think you'll understand my motivations a lot better after having  
>> used it for a bit.
>
> I actually have a s390 vm (RHEL 4 IIRC).  It acts just like any  
> other remote machine over ssh except that it's especially slow  
> (probably the host is overloaded).  Of course I wouldn't dream of  
> trying to install something like that though.

Exactly. In fact, I'm even scared to reboot mine because I might end  
up in a 3270 terminal. The whole text only crap keeps people from  
using this platform! And that's what I want to change here.

Alex

[Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Alexander Graf]

On 03.11.2009, at 09:20, Avi Kivity wrote:

> On 11/03/2009 09:50 AM, Alexander Graf wrote:
>>
>> Ok, imagine this was not this unloved S390 odd architecture but  
>> X86. The only output choices you have are:
>>
>> 1) virtio-console
>> 2) VNC / SSH over network
>> 3) virtio-fb
>>
>> Now you want to configure a server, probably using yast and all  
>> those nice graphical utilities, but still enable a firewall so  
>> people outside don't intrude your machine. Well, you managed to  
>> configure the firewall by luck to allow VNC, but now you  
>> reconfigured it and something broke - but VNC was your only chance  
>> to access the machine. Oops...
>
> x86 has real framebuffers, so software and people expect it.  s390  
> doesn't.  How do people manage now?

They cope with what's there. Fortunately we're in a position to change  
things, so we don't have to stick with the worse.

>>>> You also want to see boot messages, have a console login screen,
>>>
>>> virtio-console does that, except for the penguins.  Better, since  
>>> you can scroll back.
>>
>> It doesn't do graphics. Ever used yast in text mode?
>
> Once you're in, start ssh+X or vnc.  Again, what do people do now?

Exactly that. Again, it works but is not ideal. If we can improve user  
experience why work against it?

>>>> The hardware model isn't exactly new either. It's just the next  
>>>> logical step to a full PV machine using virtio. If the virtio-fb  
>>>> stuff turns out to be really fast and reliable, I could even  
>>>> imagine it being the default target for kvm on ppc as well, as we  
>>>> can't switch resolutions on the fly there atm.
>>>>
>>>
>>> We could with vmware-vga.
>>
>> The vmware-port stuff is pretty much tied onto X86. I don't think  
>> modifying EAX is that easy on PPC ;-).
>
> Yes, though we can probably make it work on ppc with minimal  
> modifications.

Is it worth it? We can also just implement a virtio mouse event dev  
plus fb and be good. That way we control the whole stack without  
risking to break vmware.

>>>>> Why?  the guest will typically have networking when it's set up,  
>>>>> so it should have network access during install.  You can easily  
>>>>> use slirp redirection and the built-in dhcp server to set this  
>>>>> up with relatively few hassles.
>>>>
>>>> That's how I use it right now. It's no fun.
>>>>
>>>
>>> The toolstack should hide the unfun parts.
>>
>> You can't hide guest configuration. We as a distribution control  
>> the kernel. We don't control the user's configuration as that's by  
>> design the user's choice. The only thing we can do is give users  
>> meaningful choices to choose from - and having graphics available  
>> is definitely one of them.
>
> Well, if the user chooses not to have networking then vnc or ssh+x  
> definitely fail.  That would be a strange choice for a server machine.

It's actually rather common on S390, though admittedly not that much  
on Linux+S390. There are more ways for inter node communication than  
networking. You can talk to another VM on the same machine without any  
network whatsoever. That way you can set up an isolated job (your bank  
transfer database for example) that is always protected by a proxy to  
the outside world.

>> Seriously, try to ask someone internally to get access to an S390.  
>> I think you'll understand my motivations a lot better after having  
>> used it for a bit.
>
> I actually have a s390 vm (RHEL 4 IIRC).  It acts just like any  
> other remote machine over ssh except that it's especially slow  
> (probably the host is overloaded).  Of course I wouldn't dream of  
> trying to install something like that though.

Exactly. In fact, I'm even scared to reboot mine because I might end  
up in a 3270 terminal. The whole text only crap keeps people from  
using this platform! And that's what I want to change here.

Alex

Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 10:26 AM, Alexander Graf wrote:
> Exactly. In fact, I'm even scared to reboot mine because I might end 
> up in a 3270 terminal. The whole text only crap keeps people from 
> using this platform! And that's what I want to change here.

Ok.  I oppose paravirtualization for its own sake and only support it if 
there's no other way to get performance.  In this case it buys us basic 
functionality which is surprisingly missing on native, that's arguably 
even more important.

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

[Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 10:26 AM, Alexander Graf wrote:
> Exactly. In fact, I'm even scared to reboot mine because I might end 
> up in a 3270 terminal. The whole text only crap keeps people from 
> using this platform! And that's what I want to change here.

Ok.  I oppose paravirtualization for its own sake and only support it if 
there's no other way to get performance.  In this case it buys us basic 
functionality which is surprisingly missing on native, that's arguably 
even more important.

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

Re: [PATCH] Add VirtIO Frame Buffer Support

[Anthony Liguori]

Avi Kivity wrote:
> On 11/03/2009 10:26 AM, Alexander Graf wrote:
>> Exactly. In fact, I'm even scared to reboot mine because I might end 
>> up in a 3270 terminal. The whole text only crap keeps people from 
>> using this platform! And that's what I want to change here.
>
> Ok.  I oppose paravirtualization for its own sake and only support it 
> if there's no other way to get performance.  In this case it buys us 
> basic functionality which is surprisingly missing on native, that's 
> arguably even more important.

There is no "native" on s390.  Everything is "paravirtual".

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Anthony Liguori]

Avi Kivity wrote:
> On 11/03/2009 10:26 AM, Alexander Graf wrote:
>> Exactly. In fact, I'm even scared to reboot mine because I might end 
>> up in a 3270 terminal. The whole text only crap keeps people from 
>> using this platform! And that's what I want to change here.
>
> Ok.  I oppose paravirtualization for its own sake and only support it 
> if there's no other way to get performance.  In this case it buys us 
> basic functionality which is surprisingly missing on native, that's 
> arguably even more important.

There is no "native" on s390.  Everything is "paravirtual".

Regards,

Anthony Liguori

Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 05:14 PM, Anthony Liguori wrote:
> Avi Kivity wrote:
>> On 11/03/2009 10:26 AM, Alexander Graf wrote:
>>> Exactly. In fact, I'm even scared to reboot mine because I might end 
>>> up in a 3270 terminal. The whole text only crap keeps people from 
>>> using this platform! And that's what I want to change here.
>>
>> Ok.  I oppose paravirtualization for its own sake and only support it 
>> if there's no other way to get performance.  In this case it buys us 
>> basic functionality which is surprisingly missing on native, that's 
>> arguably even more important.
>
> There is no "native" on s390.  Everything is "paravirtual".

I meant native as in "what they usually do without our stuff".

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

[Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 05:14 PM, Anthony Liguori wrote:
> Avi Kivity wrote:
>> On 11/03/2009 10:26 AM, Alexander Graf wrote:
>>> Exactly. In fact, I'm even scared to reboot mine because I might end 
>>> up in a 3270 terminal. The whole text only crap keeps people from 
>>> using this platform! And that's what I want to change here.
>>
>> Ok.  I oppose paravirtualization for its own sake and only support it 
>> if there's no other way to get performance.  In this case it buys us 
>> basic functionality which is surprisingly missing on native, that's 
>> arguably even more important.
>
> There is no "native" on s390.  Everything is "paravirtual".

I meant native as in "what they usually do without our stuff".

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

Re: [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Vincent Hanquez]

On Tue, Nov 03, 2009 at 07:39:34AM +0100, Alexander Graf wrote:
>
> On 03.11.2009, at 07:34, Avi Kivity wrote:
>
>> On 11/03/2009 08:27 AM, Alexander Graf wrote:
>>>
>>>> How does it work today?
>>>
>>> You boot into a TERM=dumb line based emulation on 3270 (worst thing  
>>> haunting people's nightmares ever), trying to get out of that mode  
>>> as quickly as possible and off into SSH / VNC.
>>
>> Despite the coolness factor, IMO a few minutes during install time do 
>> not justify a new hardware model and a new driver.
>
> It's more than just coolness factor. There are use cases out there 
> (www.susestudio.com) that don't want to rely on the guest exporting a VNC 
> server to the outside just to access graphics. You also want to see boot 
> messages, have a console login screen, be able to debug things without 
> switching between virtio-console and vnc, etc. etc.
>
> The hardware model isn't exactly new either. It's just the next logical 
> step to a full PV machine using virtio. If the virtio-fb stuff turns out 
> to be really fast and reliable, I could even imagine it being the default 
> target for kvm on ppc as well, as we can't switch resolutions on the fly 
> there atm.

not sure if i'm missing the point here, but couldn't it be hypothetically
extended to stuff 3d (or video & more 2d accel ?) commands too ? I can't
imagine the cirrus or stdvga driver be able to do that ever ;)

-- 
Vincent

Re: [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Vincent Hanquez]

On Tue, Nov 03, 2009 at 07:39:34AM +0100, Alexander Graf wrote:
>
> On 03.11.2009, at 07:34, Avi Kivity wrote:
>
>> On 11/03/2009 08:27 AM, Alexander Graf wrote:
>>>
>>>> How does it work today?
>>>
>>> You boot into a TERM=dumb line based emulation on 3270 (worst thing  
>>> haunting people's nightmares ever), trying to get out of that mode  
>>> as quickly as possible and off into SSH / VNC.
>>
>> Despite the coolness factor, IMO a few minutes during install time do 
>> not justify a new hardware model and a new driver.
>
> It's more than just coolness factor. There are use cases out there 
> (www.susestudio.com) that don't want to rely on the guest exporting a VNC 
> server to the outside just to access graphics. You also want to see boot 
> messages, have a console login screen, be able to debug things without 
> switching between virtio-console and vnc, etc. etc.
>
> The hardware model isn't exactly new either. It's just the next logical 
> step to a full PV machine using virtio. If the virtio-fb stuff turns out 
> to be really fast and reliable, I could even imagine it being the default 
> target for kvm on ppc as well, as we can't switch resolutions on the fly 
> there atm.

not sure if i'm missing the point here, but couldn't it be hypothetically
extended to stuff 3d (or video & more 2d accel ?) commands too ? I can't
imagine the cirrus or stdvga driver be able to do that ever ;)

-- 
Vincent

Re: [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 01:25 PM, Vincent Hanquez wrote:
> not sure if i'm missing the point here, but couldn't it be hypothetically
> extended to stuff 3d (or video&  more 2d accel ?) commands too ? I can't
> imagine the cirrus or stdvga driver be able to do that ever ;)
>    

cirrus has pretty good 2d acceleration.  3D is a mega-project though.

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

Re: [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 01:25 PM, Vincent Hanquez wrote:
> not sure if i'm missing the point here, but couldn't it be hypothetically
> extended to stuff 3d (or video&  more 2d accel ?) commands too ? I can't
> imagine the cirrus or stdvga driver be able to do that ever ;)
>    

cirrus has pretty good 2d acceleration.  3D is a mega-project though.

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

Re: [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Ondrej Zajicek]

[-- Attachment #1.1: Type: text/plain, Size: 796 bytes --]

On Tue, Nov 03, 2009 at 11:38:18AM +0200, Avi Kivity wrote:
> On 11/03/2009 01:25 PM, Vincent Hanquez wrote:
> > not sure if i'm missing the point here, but couldn't it be hypothetically
> > extended to stuff 3d (or video&  more 2d accel ?) commands too ? I can't
> > imagine the cirrus or stdvga driver be able to do that ever ;)
> >    
> 
> cirrus has pretty good 2d acceleration.  3D is a mega-project though.

Cirrus has no blending/compositing hardware support.
Paravirtualized graphics can easily support full XRender-style
2D acceleration.

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 399 bytes --]

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference

[-- Attachment #3: Type: text/plain, Size: 182 bytes --]

_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [Linux-fbdev-devel] [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Ondrej Zajicek]

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

On Tue, Nov 03, 2009 at 11:38:18AM +0200, Avi Kivity wrote:
> On 11/03/2009 01:25 PM, Vincent Hanquez wrote:
> > not sure if i'm missing the point here, but couldn't it be hypothetically
> > extended to stuff 3d (or video&  more 2d accel ?) commands too ? I can't
> > imagine the cirrus or stdvga driver be able to do that ever ;)
> >    
> 
> cirrus has pretty good 2d acceleration.  3D is a mega-project though.

Cirrus has no blending/compositing hardware support.
Paravirtualized graphics can easily support full XRender-style
2D acceleration.

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Linux-fbdev-devel] [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 05:29 PM, Ondrej Zajicek wrote:
> On Tue, Nov 03, 2009 at 11:38:18AM +0200, Avi Kivity wrote:
>    
>> On 11/03/2009 01:25 PM, Vincent Hanquez wrote:
>>      
>>> not sure if i'm missing the point here, but couldn't it be hypothetically
>>> extended to stuff 3d (or video&   more 2d accel ?) commands too ? I can't
>>> imagine the cirrus or stdvga driver be able to do that ever ;)
>>>
>>>        
>> cirrus has pretty good 2d acceleration.  3D is a mega-project though.
>>      
> Cirrus has no blending/compositing hardware support.
> Paravirtualized graphics can easily support full XRender-style
> 2D acceleration.
>    

What do that entail? 3/4 operand raster ops?

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

Re: [Linux-fbdev-devel] [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/03/2009 05:29 PM, Ondrej Zajicek wrote:
> On Tue, Nov 03, 2009 at 11:38:18AM +0200, Avi Kivity wrote:
>    
>> On 11/03/2009 01:25 PM, Vincent Hanquez wrote:
>>      
>>> not sure if i'm missing the point here, but couldn't it be hypothetically
>>> extended to stuff 3d (or video&   more 2d accel ?) commands too ? I can't
>>> imagine the cirrus or stdvga driver be able to do that ever ;)
>>>
>>>        
>> cirrus has pretty good 2d acceleration.  3D is a mega-project though.
>>      
> Cirrus has no blending/compositing hardware support.
> Paravirtualized graphics can easily support full XRender-style
> 2D acceleration.
>    

What do that entail? 3/4 operand raster ops?

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

Re: [Linux-fbdev-devel] [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Ondrej Zajicek]

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

On Tue, Nov 03, 2009 at 06:05:13PM +0200, Avi Kivity wrote:
>>> cirrus has pretty good 2d acceleration.  3D is a mega-project though.
>>>      
>> Cirrus has no blending/compositing hardware support.
>> Paravirtualized graphics can easily support full XRender-style
>> 2D acceleration.
>
> What do that entail? 3/4 operand raster ops?

Yes, basically three operand render/composite operation and rendering of
some 2D primitives (trapezoids).

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Linux-fbdev-devel] [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Ondrej Zajicek]

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

On Tue, Nov 03, 2009 at 06:05:13PM +0200, Avi Kivity wrote:
>>> cirrus has pretty good 2d acceleration.  3D is a mega-project though.
>>>      
>> Cirrus has no blending/compositing hardware support.
>> Paravirtualized graphics can easily support full XRender-style
>> 2D acceleration.
>
> What do that entail? 3/4 operand raster ops?

Yes, basically three operand render/composite operation and rendering of
some 2D primitives (trapezoids).

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Linux-fbdev-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Paolo Bonzini]

On 11/03/2009 05:05 PM, Avi Kivity wrote:
> On 11/03/2009 05:29 PM, Ondrej Zajicek wrote:
>> On Tue, Nov 03, 2009 at 11:38:18AM +0200, Avi Kivity wrote:
>>> On 11/03/2009 01:25 PM, Vincent Hanquez wrote:
>>>> not sure if i'm missing the point here, but couldn't it be
>>>> hypothetically
>>>> extended to stuff 3d (or video& more 2d accel ?) commands too ? I can't
>>>> imagine the cirrus or stdvga driver be able to do that ever ;)
>>>>
>>> cirrus has pretty good 2d acceleration. 3D is a mega-project though.
>> Cirrus has no blending/compositing hardware support.
>> Paravirtualized graphics can easily support full XRender-style
>> 2D acceleration.
>
> What do that entail? 3/4 operand raster ops?

With alpha compositing.

Paolo

Re: [Linux-fbdev-devel] [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Paolo Bonzini]

On 11/03/2009 05:05 PM, Avi Kivity wrote:
> On 11/03/2009 05:29 PM, Ondrej Zajicek wrote:
>> On Tue, Nov 03, 2009 at 11:38:18AM +0200, Avi Kivity wrote:
>>> On 11/03/2009 01:25 PM, Vincent Hanquez wrote:
>>>> not sure if i'm missing the point here, but couldn't it be
>>>> hypothetically
>>>> extended to stuff 3d (or video& more 2d accel ?) commands too ? I can't
>>>> imagine the cirrus or stdvga driver be able to do that ever ;)
>>>>
>>> cirrus has pretty good 2d acceleration. 3D is a mega-project though.
>> Cirrus has no blending/compositing hardware support.
>> Paravirtualized graphics can easily support full XRender-style
>> 2D acceleration.
>
> What do that entail? 3/4 operand raster ops?

With alpha compositing.

Paolo

Re: [Linux-fbdev-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Paolo Bonzini]

On 11/03/2009 05:05 PM, Avi Kivity wrote:
> On 11/03/2009 05:29 PM, Ondrej Zajicek wrote:
>> On Tue, Nov 03, 2009 at 11:38:18AM +0200, Avi Kivity wrote:
>>> On 11/03/2009 01:25 PM, Vincent Hanquez wrote:
>>>> not sure if i'm missing the point here, but couldn't it be
>>>> hypothetically
>>>> extended to stuff 3d (or video& more 2d accel ?) commands too ? I can't
>>>> imagine the cirrus or stdvga driver be able to do that ever ;)
>>>>
>>> cirrus has pretty good 2d acceleration. 3D is a mega-project though.
>> Cirrus has no blending/compositing hardware support.
>> Paravirtualized graphics can easily support full XRender-style
>> 2D acceleration.
>
> What do that entail? 3/4 operand raster ops?

With alpha compositing.

Paolo

Re: [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Vincent Hanquez]

On Tue, Nov 03, 2009 at 11:38:18AM +0200, Avi Kivity wrote:
> On 11/03/2009 01:25 PM, Vincent Hanquez wrote:
>> not sure if i'm missing the point here, but couldn't it be hypothetically
>> extended to stuff 3d (or video&  more 2d accel ?) commands too ? I can't
>> imagine the cirrus or stdvga driver be able to do that ever ;)
>>    
>
> cirrus has pretty good 2d acceleration.  3D is a mega-project though.

absolutely huge indeed, but still alexander's code is pretty much the
only way, to start such a project. with maybe added benefits on more
and easier 2d acceleration.

or otherwise wait for SR-IOV graphics cards (or similar tech)...

-- 
Vincent

Re: [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Anthony Liguori]

Vincent Hanquez wrote:
> On Tue, Nov 03, 2009 at 11:38:18AM +0200, Avi Kivity wrote:
>   
>> On 11/03/2009 01:25 PM, Vincent Hanquez wrote:
>>     
>>> not sure if i'm missing the point here, but couldn't it be hypothetically
>>> extended to stuff 3d (or video&  more 2d accel ?) commands too ? I can't
>>> imagine the cirrus or stdvga driver be able to do that ever ;)
>>>    
>>>       
>> cirrus has pretty good 2d acceleration.  3D is a mega-project though.
>>     
>
> absolutely huge indeed, but still alexander's code is pretty much the
> only way, to start such a project. with maybe added benefits on more
> and easier 2d acceleration.
>
> or otherwise wait for SR-IOV graphics cards (or similar tech)...
>   

I think the real question is do we paravirtualize a VGA device or a 
framebuffer.

Obviously, the advantage of doing a framebuffer is that it works for s390.

A VGA device has better backwards compatibility on PCs although it's 
obviously more complex.  In an ideal world, we could expose the virtio 
framebuffer device as part of PCI device that was also VGA capable 
(virtio-pci-vga transport?).

But then there's QXL on the horizon which complicates matters further.

I'd say that virtio-fb should just focus on the s390 use case for now.  
Let things evolve as needed.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Anthony Liguori]

Vincent Hanquez wrote:
> On Tue, Nov 03, 2009 at 11:38:18AM +0200, Avi Kivity wrote:
>   
>> On 11/03/2009 01:25 PM, Vincent Hanquez wrote:
>>     
>>> not sure if i'm missing the point here, but couldn't it be hypothetically
>>> extended to stuff 3d (or video&  more 2d accel ?) commands too ? I can't
>>> imagine the cirrus or stdvga driver be able to do that ever ;)
>>>    
>>>       
>> cirrus has pretty good 2d acceleration.  3D is a mega-project though.
>>     
>
> absolutely huge indeed, but still alexander's code is pretty much the
> only way, to start such a project. with maybe added benefits on more
> and easier 2d acceleration.
>
> or otherwise wait for SR-IOV graphics cards (or similar tech)...
>   

I think the real question is do we paravirtualize a VGA device or a 
framebuffer.

Obviously, the advantage of doing a framebuffer is that it works for s390.

A VGA device has better backwards compatibility on PCs although it's 
obviously more complex.  In an ideal world, we could expose the virtio 
framebuffer device as part of PCI device that was also VGA capable 
(virtio-pci-vga transport?).

But then there's QXL on the horizon which complicates matters further.

I'd say that virtio-fb should just focus on the s390 use case for now.  
Let things evolve as needed.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Avi Kivity]

On 11/04/2009 06:35 PM, Anthony Liguori wrote:
> Vincent Hanquez wrote:
>> On Tue, Nov 03, 2009 at 11:38:18AM +0200, Avi Kivity wrote:
>>> On 11/03/2009 01:25 PM, Vincent Hanquez wrote:
>>>> not sure if i'm missing the point here, but couldn't it be 
>>>> hypothetically
>>>> extended to stuff 3d (or video&  more 2d accel ?) commands too ? I 
>>>> can't
>>>> imagine the cirrus or stdvga driver be able to do that ever ;)
>>> cirrus has pretty good 2d acceleration.  3D is a mega-project though.
>>
>> absolutely huge indeed, but still alexander's code is pretty much the
>> only way, to start such a project. with maybe added benefits on more
>> and easier 2d acceleration.
>>
>> or otherwise wait for SR-IOV graphics cards (or similar tech)...
>
> I think the real question is do we paravirtualize a VGA device or a 
> framebuffer.
>
> Obviously, the advantage of doing a framebuffer is that it works for 
> s390.
>
> A VGA device has better backwards compatibility on PCs although it's 
> obviously more complex.  In an ideal world, we could expose the virtio 
> framebuffer device as part of PCI device that was also VGA capable 
> (virtio-pci-vga transport?).
>
> But then there's QXL on the horizon which complicates matters further.
>

qxl is vga compatible.

> I'd say that virtio-fb should just focus on the s390 use case for 
> now.  Let things evolve as needed.
>

Sure.

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

Re: [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Thomas Fjellstrom]

On Tue November 3 2009, Avi Kivity wrote:
> On 11/03/2009 01:25 PM, Vincent Hanquez wrote:
> > not sure if i'm missing the point here, but couldn't it be
> > hypothetically extended to stuff 3d (or video&  more 2d accel ?)
> > commands too ? I can't imagine the cirrus or stdvga driver be able to
> > do that ever ;)
> 
> cirrus has pretty good 2d acceleration.  3D is a mega-project though.
> 

You're kidding right? Why do I have to switch to vmware-vga so "dmesg" in a 
guest doesn't take a couple minutes to scroll by? Maybe a configuration 
issue?

-- 
Thomas Fjellstrom
tfjellstrom@shaw.ca

Re: [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Jamie Lokier]

Avi Kivity wrote:
> On 11/03/2009 12:09 AM, Alexander Graf wrote:
> >When we want to create a full VirtIO based machine, we're still missing
> >graphics output. Fortunately, Linux provides us with most of the frameworks
> >to render text and everything, we only need to implement a transport.
> >
> >So this is a frame buffer backend written for VirtIO. Using this and my
> >patch to qemu, you can use paravirtualized graphics.
> 
> What does this do that cirrus and/or vmware-vga don't?

*This* virtio-fb doesn't, but one feature I think a lot of users
(including me) would like is:

   Option to resize the guest desktop when the host desktop / host
   window / VNC client resizes.

   Tell the guest to provide multiple desktops when the host has
   multiple desktops, so things like twin monitors work nicely with
   guests.

   Relay EDID/Xrandr information and updates from host to guest, and
   generally handle hotplugging host monitors nicely.

Are there any real hardware standards worth emulating which do that?

-- Jamie

Re: [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Jamie Lokier]

Avi Kivity wrote:
> On 11/03/2009 12:09 AM, Alexander Graf wrote:
> >When we want to create a full VirtIO based machine, we're still missing
> >graphics output. Fortunately, Linux provides us with most of the frameworks
> >to render text and everything, we only need to implement a transport.
> >
> >So this is a frame buffer backend written for VirtIO. Using this and my
> >patch to qemu, you can use paravirtualized graphics.
> 
> What does this do that cirrus and/or vmware-vga don't?

*This* virtio-fb doesn't, but one feature I think a lot of users
(including me) would like is:

   Option to resize the guest desktop when the host desktop / host
   window / VNC client resizes.

   Tell the guest to provide multiple desktops when the host has
   multiple desktops, so things like twin monitors work nicely with
   guests.

   Relay EDID/Xrandr information and updates from host to guest, and
   generally handle hotplugging host monitors nicely.

Are there any real hardware standards worth emulating which do that?

-- Jamie

Re: [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Anthony Liguori]

Jamie Lokier wrote:
> Avi Kivity wrote:
>   
>> On 11/03/2009 12:09 AM, Alexander Graf wrote:
>>     
>>> When we want to create a full VirtIO based machine, we're still missing
>>> graphics output. Fortunately, Linux provides us with most of the frameworks
>>> to render text and everything, we only need to implement a transport.
>>>
>>> So this is a frame buffer backend written for VirtIO. Using this and my
>>> patch to qemu, you can use paravirtualized graphics.
>>>       
>> What does this do that cirrus and/or vmware-vga don't?
>>     
>
> *This* virtio-fb doesn't, but one feature I think a lot of users
> (including me) would like is:
>
>    Option to resize the guest desktop when the host desktop / host
>    window / VNC client resizes.
>
>    Tell the guest to provide multiple desktops when the host has
>    multiple desktops, so things like twin monitors work nicely with
>    guests.
>
>    Relay EDID/Xrandr information and updates from host to guest, and
>    generally handle hotplugging host monitors nicely.
>
> Are there any real hardware standards worth emulating which do that?
>   

vmware-vga.

If you have a little tool in a guest that uses the X extension that 
vmware vga implements, it actually works in qemu too.

Regards,

Anthony Liguoru

Re: [Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Anthony Liguori]

Jamie Lokier wrote:
> Avi Kivity wrote:
>   
>> On 11/03/2009 12:09 AM, Alexander Graf wrote:
>>     
>>> When we want to create a full VirtIO based machine, we're still missing
>>> graphics output. Fortunately, Linux provides us with most of the frameworks
>>> to render text and everything, we only need to implement a transport.
>>>
>>> So this is a frame buffer backend written for VirtIO. Using this and my
>>> patch to qemu, you can use paravirtualized graphics.
>>>       
>> What does this do that cirrus and/or vmware-vga don't?
>>     
>
> *This* virtio-fb doesn't, but one feature I think a lot of users
> (including me) would like is:
>
>    Option to resize the guest desktop when the host desktop / host
>    window / VNC client resizes.
>
>    Tell the guest to provide multiple desktops when the host has
>    multiple desktops, so things like twin monitors work nicely with
>    guests.
>
>    Relay EDID/Xrandr information and updates from host to guest, and
>    generally handle hotplugging host monitors nicely.
>
> Are there any real hardware standards worth emulating which do that?
>   

vmware-vga.

If you have a little tool in a guest that uses the X extension that 
vmware vga implements, it actually works in qemu too.

Regards,

Anthony Liguoru

Re: [PATCH] Add VirtIO Frame Buffer Support

[Michael S. Tsirkin]

On Mon, Nov 02, 2009 at 11:09:19PM +0100, Alexander Graf wrote:
> When we want to create a full VirtIO based machine, we're still missing
> graphics output. Fortunately, Linux provides us with most of the frameworks
> to render text and everything, we only need to implement a transport.
> 
> So this is a frame buffer backend written for VirtIO. Using this and my
> patch to qemu, you can use paravirtualized graphics.
> 
> This is especially important on machines that can't do MMIO, as all current
> graphics implementations qemu emulates I'm aware of so far fail here.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Nice work.  I think you want to Cc
virtualization@lists.linux-foundation.org and Rusty. Cc added.  Some
comments below, some of them checkpatch.pl would find.  I do not know
much about framebuffer, so comments are from virtio usage perspective.
Generally, I think locking is missing here.  As far as I know, virtio
APIs do no locking internally.  So when you e.g. schedule_work and then
call add_buf, this can run on many CPUs in parallel, which will cause
memory corruption.

> ---
>  drivers/video/Kconfig      |   15 +
>  drivers/video/Makefile     |    1 +
>  drivers/video/virtio-fb.c  |  799 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/virtio_ids.h |    1 +
>  4 files changed, 816 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/video/virtio-fb.c
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 9bbb285..f9be4c2 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -2069,6 +2069,21 @@ config XEN_FBDEV_FRONTEND
>  	  frame buffer driver.  It communicates with a back-end
>  	  in another domain.
>  
> +config FB_VIRTIO
> +	tristate "Virtio virtual frame buffer support"
> +	depends on FB && VIRTIO
> +	select FB_SYS_FILLRECT
> +	select FB_SYS_COPYAREA
> +	select FB_SYS_IMAGEBLIT
> +	select FB_SYS_FOPS
> +	select FB_DEFERRED_IO
> +	help
> +	  This driver implements a driver for a Virtio based
> +	  frame buffer device.  It communicates to something that
> +	  can talk Virtio too, most probably a hypervisor.
> +
> +	  If unsure, say N.
> +


Most of virtio is marked experimental. Maybe this should be as well.


>  config FB_METRONOME
>  	tristate "E-Ink Metronome/8track controller support"
>  	depends on FB
> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> index 80232e1..40802c8 100644
> --- a/drivers/video/Makefile
> +++ b/drivers/video/Makefile
> @@ -125,6 +125,7 @@ obj-$(CONFIG_FB_XILINX)           += xilinxfb.o
>  obj-$(CONFIG_FB_SH_MOBILE_LCDC)	  += sh_mobile_lcdcfb.o
>  obj-$(CONFIG_FB_OMAP)             += omap/
>  obj-$(CONFIG_XEN_FBDEV_FRONTEND)  += xen-fbfront.o
> +obj-$(CONFIG_FB_VIRTIO)           += virtio-fb.o
>  obj-$(CONFIG_FB_CARMINE)          += carminefb.o
>  obj-$(CONFIG_FB_MB862XX)	  += mb862xx/
>  obj-$(CONFIG_FB_MSM)              += msm/
> diff --git a/drivers/video/virtio-fb.c b/drivers/video/virtio-fb.c
> new file mode 100644
> index 0000000..2a73950
> --- /dev/null
> +++ b/drivers/video/virtio-fb.c
> @@ -0,0 +1,799 @@
> +/*
> + * VirtIO PV frame buffer device
> + *
> + * Copyright (C) 2009 Alexander Graf <agraf@suse.de>

Out of curiosity, are copyrights your own, or suse/novell's?
> + *
> + *  Based on linux/drivers/video/virtio-fbfront.c

Where is that? Maybe carry attribution from there as well.
There's also a lot of simularity with xen-fbfront.c
Is it accidental?

> + *
> + *  This file is subject to the terms and conditions of the GNU General Public
> + *  License. See the file COPYING in the main directory of this archive for
> + *  more details.
> + */
> +
> +#include <linux/console.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/fb.h>
> +#include <linux/module.h>
> +#include <linux/vmalloc.h>
> +#include <linux/mm.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_config.h>
> +#include <linux/uaccess.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>

is interrupt.h needed? virito mostly
abstracts this for you.

> +
> +#define MAX_BUF			128
> +
> +struct virtio_fb_info {
> +	struct fb_info		*fb_info;
> +	unsigned char		*fb;
> +	char			*inbuf;
> +
> +	u16			width;
> +	u16			height;
> +
> +	int			nr_pages;
> +	int			size;
> +
> +	struct kmem_cache	*cmd_cache;
> +	struct virtqueue	*vq_in;
> +	struct virtqueue	*vq_out;
> +	struct virtio_device	*vdev;
> +
> +	void			*last_buf[MAX_BUF];
> +	int			last_buf_idx;
> +	spinlock_t		vfree_lock;
> +	struct work_struct	vfree_work;
> +	struct work_struct	refresh_work;
> +};
> +
> +/* guest -> Host commands */
> +#define VIRTIO_FB_CMD_RESIZE            0x01                                         
> +#define VIRTIO_FB_CMD_FILL              0x02                                         
> +#define VIRTIO_FB_CMD_BLIT              0x03                                         
> +#define VIRTIO_FB_CMD_COPY              0x04                                         
> +#define VIRTIO_FB_CMD_WRITE             0x05                                         
> +    
> +/* host -> guest commands */                                                         
> +#define VIRTIO_FB_CMD_REFRESH           0x81                                         


These constants and structures (also below) should go into linux/virtio-fb.h
which should be exported. You then won't have to
duplicate them in userspace. Just remember to convert uXX to __uXX.

> +
> +struct virtio_fb_cmd {
> +	u8			cmd;
> +	union {
> +		struct {
> +			u16	width;
> +			u16	height;
> +		} resize __attribute__ ((packed));
> +		struct {
> +			u16	x;
> +			u16	y;
> +			u16	width;
> +			u16	height;
> +		} blit __attribute__ ((packed));
> +		struct {
> +			u16	x1;
> +			u16	y1;
> +			u16	x2;
> +			u16	y2;
> +			u16	width;
> +			u16	height;
> +		} copy_area __attribute__ ((packed));
> +		struct {
> +			u8	rop;
> +			u16	x;
> +			u16	y;
> +			u16	width;
> +			u16	height;
> +			u32	color;
> +		} fill __attribute__ ((packed));
> +		struct {
> +			u64	offset;
> +			u64	count;
> +		} write __attribute__ ((packed));
> +		u8		pad[23];
> +	};
> +
> +	union {
> +		/* We remember the data pointer so we we can easily free
> +		   everything later by only knowing this structure */
> +		char		*send_buf;

I thought this structure is passed between guest and host?
If so you do not want to stick your pointers there,
add_buf has data pointer for this reason.

> +		u64		_pad;

This might create problems with 32 bit userspace on 64 bit kernels:
which bits does the pointer get packed into depends on architecture.
Better to just pass in __u64 and have userspace cast pointer to that
type, IMO.


> +	};
> +} __attribute__ ((packed));


Packed structures generate terrible code on some architectures.  Can you
just pad structures to multiples of 64 bit, or to full union size, instead?

> +
> +enum copy_type {
> +	COPY_KERNEL,
> +	COPY_USER,
> +	COPY_NOCOPY,
> +};
> +
> +#define DEFAULT_WIDTH		800
> +#define DEFAULT_HEIGHT		600
> +#define DEFAULT_DEPTH		32
> +#define DEFAULT_MEM		8
> +
> +#define DEFAULT_FB_LEN (DEFAULT_WIDTH * DEFAULT_HEIGHT * DEFAULT_DEPTH / 8)
> +
> +enum { KPARAM_MEM, KPARAM_WIDTH, KPARAM_HEIGHT, KPARAM_CNT };
> +static int video[KPARAM_CNT] = { DEFAULT_MEM, DEFAULT_WIDTH, DEFAULT_HEIGHT };
> +module_param_array(video, int, NULL, 0);
> +MODULE_PARM_DESC(video,
> +	"Video size in M,width,height in pixels (default \""
> +	str(DEFAULT_MEM) ","
> +	str(DEFAULT_WIDTH) ","
> +	str(DEFAULT_HEIGHT) "\")");
> +
> +static void virtio_fb_output(struct virtqueue *vq);
> +
> +static void *rvmalloc(unsigned long size)

what exactly does this do?

> +{
> +	void *mem;
> +	unsigned long adr;
> +
> +	size = PAGE_ALIGN(size);
> +	mem = vmalloc_32(size);
> +	if (!mem)
> +		return NULL;
> +
> +	memset(mem, 0, size); /* Clear the ram out, no junk to the user */
> +	adr = (unsigned long) mem;
> +	while (size > 0) {
> +		SetPageReserved(vmalloc_to_page((void *)adr));

Where is the bit cleared?

> +		adr += PAGE_SIZE;
> +		size -= PAGE_SIZE;
> +	}
> +
> +	return mem;
> +}
> +
> +/* This is videobuf_vmalloc_to_sg() from videobuf-dma-sg.c
> +   I modified it to take an extra sg entry for the cmd and work with non
> +   page-aligned pointers though */
> +static struct scatterlist* vmalloc_to_sg(unsigned char *virt, int length,
> +					 void *cmd, int cmd_len, int *sg_elem)
> +{
> +	struct scatterlist *sglist;
> +	struct page *pg;
> +	int nr_pages = (length+PAGE_SIZE-1)/PAGE_SIZE;

Spaces are needed around +, -

> +	int sg_entries;
> +	int i;
> +
> +	/* unaligned */
> +	if ((ulong)virt & ~PAGE_MASK) {

Use offset_in_page here and below?

> +		int tmp_len = length - (PAGE_SIZE - ((ulong)virt & ~PAGE_MASK));
> +		/* how long would it be without the first non-aligned chunk? */
> +		nr_pages = (tmp_len+PAGE_SIZE-1)/PAGE_SIZE;

Spaces are needed around +, -
Also, is this just PAGE_ALIGN?

> +		/* add the first chunk */
> +		nr_pages++;
> +	}

Is all the above just
	nr_pages = PAGE_ALIGN(length + offset_in_page(virt)) / PAGE_SIZE
?
if so, no need for if statements etc.

> +
> +	sg_entries = nr_pages + 1;
> +
> +	sglist = kcalloc(sg_entries, sizeof(struct scatterlist), GFP_KERNEL);
> +	if (!sglist)
> +		return NULL;
> +	sg_init_table(sglist, sg_entries);
> +
> +	/* Put cmd element in */
> +	sg_set_buf(&sglist[0], cmd, cmd_len);
> +
> +	/* Fill with elements for the data */
> +	for (i = 1; i < sg_entries; i++) {
> +		pg = vmalloc_to_page(virt);
> +		if (!pg)
> +			goto err;
> +
> +		if ((ulong)virt & ~PAGE_MASK) {
> +			int tmp_off = ((ulong)virt & ~PAGE_MASK);
> +
> +			sg_set_page(&sglist[i], pg, PAGE_SIZE - tmp_off, tmp_off);
> +			virt = (char*)((ulong)virt & PAGE_MASK);
> +		} else {
> +			sg_set_page(&sglist[i], pg, PAGE_SIZE, 0);
> +		}

You don't need an if statement, do you?
For aligned addresses tmp_off is 0, so it works out.

> +		virt += PAGE_SIZE;
> +	}
> +
> +	*sg_elem = sg_entries;
> +	return sglist;
> +
> + err:
> +	kfree(sglist);
> +	return NULL;
> +}
> +
> +
> +static void _virtio_fb_send(struct virtio_fb_info *info,

void? Don't we care that this might fail e.g. on OOM?

> +			    struct virtio_fb_cmd *cmd,
> +			    char *buf, int len, enum copy_type copy)
> +{
> +	char *send_buf = NULL;
> +	char *sg_buf = NULL;
> +	struct virtio_fb_cmd *send_cmd;
> +	struct scatterlist *sg;
> +	int len_cmd = sizeof(struct virtio_fb_cmd);

sizeof *cmd would be 

> +	int r, sg_elem;
> +
> +	send_cmd = kmem_cache_alloc(info->cmd_cache, GFP_KERNEL);
> +	if (!send_cmd) {
> +		printk(KERN_ERR "virtio-fb: couldn't allocate cmd\n");
> +		return;
> +	}
> +
> +	memcpy(send_cmd, cmd, len_cmd);
> +
> +	if (len) {
> +		sg_buf = send_buf = vmalloc(len);
> +		if (!send_buf) {
> +			printk(KERN_ERR "virtio-fb: couldn't allocate %d b\n",
> +					len);
> +			return;
> +		}
> +
> +		switch (copy) {
> +		case COPY_KERNEL:
> +			memcpy(send_buf, buf, len);
> +			break;
> +		case COPY_USER:
> +			r = copy_from_user(send_buf, (const __user char*)buf,
> +					   len);
> +			break;
> +		case COPY_NOCOPY:
> +			sg_buf = buf;

vmalloc is not really needed in this case, is it?

> +			break;
> +		}
> +	}
> +
> +	send_cmd->send_buf = send_buf;
> +
> +	sg = vmalloc_to_sg(sg_buf, len, send_cmd, len_cmd, &sg_elem);
> +	if (!sg) {
> +		printk(KERN_ERR "virtio-fb: couldn't gather scatter list\n");
> +		return;
> +	}
> +
> +add_again:
> +	r = info->vq_out->vq_ops->add_buf(info->vq_out, sg, sg_elem, 0, send_cmd);


This seems to be done without any locks.
Just making sure: what guarantees that multiple CPUs do
not do this in parallel? Note that virtio do no
locking internally, it's always up to the user.

> +	info->vq_out->vq_ops->kick(info->vq_out);
> +
> +	if ( r == -ENOSPC ) {

what about other errors?

> +		/* Queue was full, so try again */
> +		cpu_relax();
> +		virtio_fb_output(info->vq_out);
> +		goto add_again;

That's pretty evil. Is it possible to block for interrupt
until vq becomes empty, use a timer instead, or
avoid posting more than VQ size in some other way?

> +	}

Can this use for or while loop?

> +
> +	kfree(sg);
> +}
> +
> +static void virtio_fb_send_user(struct virtio_fb_info *info,
> +				struct virtio_fb_cmd *cmd,
> +				const __user char *buf, int len)
> +{
> +	_virtio_fb_send(info, cmd, (char *)buf, len, COPY_USER);


Casting __user pointer away in this way gives up type
safety. Maybe it would be a good idea to split the copy part
away from _virtio_fb_send? Then _nocopy won't do any vmalloc,
this function will do vmalloc+copy_from_user, and virtio_fb_send
below will do vmalloc + memcpy.

> +}
> +
> +static void virtio_fb_send_nocopy(struct virtio_fb_info *info,
> +				  struct virtio_fb_cmd *cmd,
> +				  char *buf, int len)
> +{
> +	_virtio_fb_send(info, cmd, buf, len, COPY_NOCOPY);
> +}
> +
> +static void virtio_fb_send(struct virtio_fb_info *info, struct virtio_fb_cmd *cmd,
> +			   char *buf, int len)
> +{
> +	_virtio_fb_send(info, cmd, buf, len, COPY_KERNEL);
> +}
> +
> +

2 empty lines

> +static int virtio_fb_setcolreg(unsigned regno, unsigned red, unsigned green,
> +			   unsigned blue, unsigned transp,
> +			   struct fb_info *info)
> +{
> +	u32 v;
> +	if (regno >= 16)
> +		return 1;
> +
> +#define CNVT_TOHW(val,width) ((((val)<<(width))+0x7FFF-(val))>>16)

spaces are needed around <<, >>, -, +

> +	red = CNVT_TOHW(red, info->var.red.length);
> +	green = CNVT_TOHW(green, info->var.green.length);
> +	blue = CNVT_TOHW(blue, info->var.blue.length);
> +	transp = CNVT_TOHW(transp, info->var.transp.length);
> +#undef CNVT_TOHW

this is what xen does as well. Any chance to use symbolic
constants above?
Also, are var.XXXX.length values in fact constant?

> +
> +

extra empty line

> +	v = (red << info->var.red.offset) |
> +	    (green << info->var.green.offset) |
> +	    (blue << info->var.blue.offset) |
> +	    (transp << info->var.transp.offset);
> +
> +	((u32 *) (info->pseudo_palette))[regno] = v;

space between } and ( above is just confusing.

> +
> +	return 0;
> +}
> +
> +static void virtio_fb_fillrect(struct fb_info *p, const struct fb_fillrect *rect)
> +{
> +	struct virtio_fb_info *info = p->par;
> +	struct virtio_fb_cmd cmd;
> +
> +	sys_fillrect(p, rect);
> +
> +	cmd.cmd = VIRTIO_FB_CMD_FILL;
> +	cmd.fill.rop = rect->rop;
> +	cmd.fill.x = rect->dx;
> +	cmd.fill.y = rect->dy;
> +	cmd.fill.width = rect->width;
> +	cmd.fill.height = rect->height;
> +	cmd.fill.color = rect->color;
> +
> +	virtio_fb_send(info, &cmd, NULL, 0);
> +}
> +
> +static void virtio_fb_imageblit(struct fb_info *p, const struct fb_image *image)
> +{
> +	struct virtio_fb_info *info = p->par;
> +	struct virtio_fb_cmd cmd;

So this puts a large structure n stack, only to memcpy
it later into a kmalloced one. I think it would be
better just to allocate command from cache directly, for
kernel users. Same comment would apply to others below.

> +	char *_buf;
> +	char *buf;
> +	char *vfb;
> +	int i, w;
> +	int bpp = p->var.bits_per_pixel / 8;
> +	int len = image->width * image->height * bpp;
> +
> +	sys_imageblit(p, image);
> +
> +	cmd.cmd = VIRTIO_FB_CMD_BLIT;
> +	cmd.blit.x = image->dx;
> +	cmd.blit.y = image->dy;
> +	cmd.blit.height = image->height;
> +	cmd.blit.width = image->width;
> +
> +	if (image->depth == 32) {
> +		/* Send the image off directly */
> +
> +		virtio_fb_send(info, &cmd, (char*)image->data, len);

You cast constness away? that's usually not a good thing to do.

> +		return;
> +	}
> +
> +	/* Send the 32-bit translated image */
> +
> +	buf = _buf = vmalloc(len);
> +
> +	vfb = info->fb;
> +	vfb += (image->dy * p->fix.line_length) + image->dx * bpp;

() not needed around *.

> +
> +	w = image->width * bpp;
> +
> +	for (i = 0; i < image->height; i++) {
> +		memcpy(buf, vfb, w);
> +
> +		buf += w;
> +		vfb += p->fix.line_length;
> +	}
> +
> +	virtio_fb_send(info, &cmd, _buf, len);
> +
> +	vfree(_buf);

This would benefit from nocopy, right?
Just another arument why what I propose above is a good idea.

> +}
> +
> +static void virtio_fb_copyarea(struct fb_info *p, const struct fb_copyarea *area)
> +{
> +	struct virtio_fb_info *info = p->par;
> +	struct virtio_fb_cmd cmd;
> +
> +	sys_copyarea(p, area);
> +
> +	cmd.cmd = VIRTIO_FB_CMD_COPY;
> +	cmd.copy_area.x1 = area->sx;
> +	cmd.copy_area.y1 = area->sy;
> +	cmd.copy_area.x2 = area->dx;
> +	cmd.copy_area.y2 = area->dy;
> +	cmd.copy_area.width = area->width;
> +	cmd.copy_area.height = area->height;
> +
> +	virtio_fb_send(info, &cmd, NULL, 0);
> +}
> +
> +static ssize_t virtio_fb_write(struct fb_info *p, const char __user *buf,
> +			size_t count, loff_t *ppos)
> +{
> +	struct virtio_fb_info *info = p->par;
> +	struct virtio_fb_cmd cmd;
> +	loff_t pos = *ppos;
> +	ssize_t res;
> +
> +	res = fb_sys_write(p, buf, count, ppos);
> +
> +	cmd.cmd = VIRTIO_FB_CMD_WRITE;
> +	cmd.write.offset = pos;
> +	cmd.write.count = count;
> +
> +	virtio_fb_send_user(info, &cmd, buf, count);
> +	return res;
> +}
> +
> +static int
> +virtio_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *p)
> +{
> +	struct virtio_fb_info *info = p->par;
> +
> +	if (var->bits_per_pixel != DEFAULT_DEPTH) {
> +		/* We only support 32 bpp */
> +		return -EINVAL;
> +	}
> +
> +	if ((var->xres * var->yres * DEFAULT_DEPTH / 8) > info->size) {

don't need () around math

> +		/* Doesn't fit in the frame buffer */
> +		return -EINVAL;
> +	}
> +
> +	var->xres_virtual = var->xres;
> +	var->yres_virtual = var->yres;
> +
> +	return 0;
> +}
> +
> +static int virtio_fb_set_par(struct fb_info *p)
> +{
> +	struct virtio_fb_info *info = p->par;
> +	struct virtio_fb_cmd cmd;
> +
> +	/* Do nothing if we're on that resolution already */
> +	if ((p->var.xres == info->width) &&
> +	    (p->var.yres == info->height))

don't need () around ===

> +		return 0;
> +
> +	info->width = p->var.xres;
> +	info->height = p->var.yres;
> +	p->fix.line_length = p->var.xres_virtual * 4;
> +
> +	/* Notify hypervisor */
> +
> +	cmd.cmd = VIRTIO_FB_CMD_RESIZE;
> +	cmd.resize.width = p->var.xres;
> +	cmd.resize.height = p->var.yres;
> +
> +	virtio_fb_send(info, &cmd, NULL, 0);
> +
> +	return 0;
> +}
> +
> +static struct fb_ops virtio_fb_ops = {
> +	.owner		= THIS_MODULE,
> +	.fb_read	= fb_sys_read,
> +	.fb_write	= virtio_fb_write,
> +	.fb_setcolreg	= virtio_fb_setcolreg,
> +	.fb_fillrect	= virtio_fb_fillrect,
> +	.fb_copyarea	= virtio_fb_copyarea,
> +	.fb_imageblit	= virtio_fb_imageblit,
> +	.fb_check_var	= virtio_fb_check_var,
> +	.fb_set_par     = virtio_fb_set_par,
> +};
> +
> +static __devinit void
> +virtio_fb_make_preferred_console(void)
> +{
> +	struct console *c;
> +
> +	if (console_set_on_cmdline)
> +		return;
> +
> +	acquire_console_sem();
> +	for (c = console_drivers; c; c = c->next) {
> +		if (!strcmp(c->name, "tty") && c->index == 0)
> +			break;
> +	}

{} not needed

> +	release_console_sem();

Just a thought: can console c go away at this point?

> +	if (c) {
> +		unregister_console(c);
> +		c->flags |= CON_CONSDEV;
> +		c->flags &= ~CON_PRINTBUFFER; /* don't print again */
> +		register_console(c);
> +	}
> +}
> +
> +static void virtio_fb_deferred_io(struct fb_info *fb_info,
> +				  struct list_head *pagelist)
> +{
> +	struct virtio_fb_info *info = fb_info->par;
> +	struct page *page;
> +	unsigned long beg, end;
> +	int y1, y2, miny, maxy;
> +	struct virtio_fb_cmd cmd;
> +
> +	miny = INT_MAX;
> +	maxy = 0;
> +	list_for_each_entry(page, pagelist, lru) {
> +		beg = page->index << PAGE_SHIFT;

page_index()?

> +		end = beg + PAGE_SIZE - 1;
> +		y1 = beg / fb_info->fix.line_length;

You do all these divisions in a cycle, then end up multiplying
the result by line_length. Maybe do the math in bytes.

> +		y2 = end / fb_info->fix.line_length;

Is the result guaranteed to fit in int?

> +		if (y2 >= fb_info->var.yres)
> +			y2 = fb_info->var.yres - 1;
> +		if (miny > y1)
> +			miny = y1;
> +		if (maxy < y2)
> +			maxy = y2;

use min()/max() macros above.

> +	}
> +
> +	if (miny != INT_MAX) {

if (miny == INT_MAX)
	return;

will be neater

> +		cmd.cmd = VIRTIO_FB_CMD_WRITE;
> +		cmd.write.offset = miny * fb_info->fix.line_length;
> +		cmd.write.count = (maxy - miny + 1) * fb_info->fix.line_length;
> +
> +		virtio_fb_send_nocopy(info, &cmd, info->fb + cmd.write.offset,
> +				      cmd.write.count);
> +	}
> +}
> +
> +static struct fb_deferred_io virtio_fb_defio = {
> +	.delay		= HZ / 20,
> +	.deferred_io	= virtio_fb_deferred_io,
> +};
> +
> +/* Callback when the host kicks our input queue.
> + *
> + * This is to enable notifications from host to guest. */
> +static void virtio_fb_input(struct virtqueue *vq)
> +{
> +	struct virtio_fb_info *info = dev_get_drvdata(&vq->vdev->dev);
> +	int len, i;
> +	void *x;
> +	void *reinject[3];
> +	int reinject_count = 0;
> +
> +	while ((x = vq->vq_ops->get_buf(vq, &len)) != NULL)

&& reinject_count < 3?

> {

This looks unsafe: can this get triggered while another
routine does add_buf? If yes you must add locking to
prevent this.

> +		struct virtio_fb_cmd *cmd = x;
> +
> +		/* Make sure we're getting an inbuf page! */

What does this check, exactly? 

> +		BUG_ON((x != info->inbuf) &&
> +		       (x != (info->inbuf + PAGE_SIZE)) &&
> +		       (x != (info->inbuf + (PAGE_SIZE * 2))));
() not needed around math.

> +
> +		switch (cmd->cmd) {
> +		case VIRTIO_FB_CMD_REFRESH:
> +			schedule_work(&info->refresh_work);
> +			break;
> +		}
> +
> +		reinject[reinject_count++] = x;

This will overflow on a buggy host.  Better check and BUG(); Also, the
number of buffers is VQ size?
Make it a constant then.

> +	}
> +
> +
> +	for (i = 0; i < reinject_count; i++) {
> +		struct scatterlist sg;
> +		void *x = reinject[i];
> +
> +		sg_init_one(&sg, x, PAGE_SIZE);
> +		vq->vq_ops->add_buf(vq, &sg, 0, 1, x);
> +		vq->vq_ops->kick(vq);

won't it be easier to reinject wach buffer as you get it directly?

> +	}
> +}
> +
> +/* Asynchronous snippet to send all screen contents to the host */
> +static void deferred_refresh(struct work_struct *work)
> +{
> +	struct virtio_fb_info *info = container_of(work, struct virtio_fb_info,
> +						   refresh_work);
> +	struct virtio_fb_cmd cmd;
> +
> +	cmd.cmd = VIRTIO_FB_CMD_WRITE;
> +	cmd.write.offset = 0;
> +	cmd.write.count = info->width * info->height * 4;

why 4?

> +
> +	virtio_fb_send_nocopy(info, &cmd, info->fb, cmd.write.count);
> +}
> +
> +/* Asynchronous garbage collector :-) */
> +static void deferred_vfree(struct work_struct *work)
> +{
> +	struct virtio_fb_info *info = container_of(work, struct virtio_fb_info,
> +						   vfree_work);
> +	int i;
> +	unsigned long flags;
> +	void *last_buf[MAX_BUF];

Wow, that's a lot of stack.  Can't vfree be called on info->last_buf
directly?

> +	int idx;
> +
> +	spin_lock_irqsave(&info->vfree_lock, flags);


spin_lock_irq is enough

> +
> +	idx = info->last_buf_idx;
> +	memcpy(last_buf, info->last_buf, sizeof(void*) * MAX_BUF);

sizeof last_buf

> +	info->last_buf_idx = 0;
> +
> +	spin_unlock_irqrestore(&info->vfree_lock, flags);
> +
> +	for (i = 0; i < idx; i++) {
> +		vfree(last_buf[i]);
> +	}

{} not needed

> +}
> +
> +/* Callback when the host kicks our output queue. This can only mean it's done
> + * processing an item, so let's free up the memory occupied by the entries */


lines too long here and elsewhere

> +static void virtio_fb_output(struct virtqueue *vq)
> +{
> +	struct virtio_fb_info *info = dev_get_drvdata(&vq->vdev->dev);
> +	int len;
> +	void *x;
> +
> +	while ((x = vq->vq_ops->get_buf(vq, &len)) != NULL) {

!= NULL not needed.
Again, get_buf must be called under some
lock that prevents add_buf from being called.

> +		struct virtio_fb_cmd *cmd = x;
> +
> +		if (cmd->send_buf) {
> +			spin_lock(&info->vfree_lock);
> +			if (info->last_buf_idx != MAX_BUF) {
> +				info->last_buf[info->last_buf_idx++] =
> +					cmd->send_buf;
> +			}

{} not needed

> +			spin_unlock(&info->vfree_lock);
> +
> +			schedule_work(&info->vfree_work);

So this would be a good place to re-enable more output
instead of busy wait above? Also, can't we vfree
here directly?

> +		}
> +
> +		kmem_cache_free(info->cmd_cache, x);
> +	}
> +}
> +
> +static int __devinit virtio_fb_probe(struct virtio_device *dev)
> +{
> +	vq_callback_t *callbacks[] = { virtio_fb_input, virtio_fb_output };
> +	const char *names[] = { "input", "output" };
> +	struct virtio_fb_info *info;
> +	struct virtqueue *vqs[2];
> +	struct fb_info *fb_info = NULL;
> +	int fb_size, res_size;
> +	int ret, err, i;
> +	char *inbuf;
> +
> +	err = dev->config->find_vqs(dev, 2, vqs, callbacks, names);

2 -> ARRAY_SIZE(vqs).

> +	if (err) {
> +		printk(KERN_ERR "VirtIO FB: couldn't find VQs\n");
> +		return err;
> +	}

You probably want del_vqs on error as well?

> +
> +	info = kmalloc(sizeof(*info), GFP_KERNEL);

this seems to be leaked on errors?
() not needed around *info.

> +	if (info == NULL)

!info

> +		return -ENOMEM;
> +
> +	info->vq_in = vqs[0];
> +	info->vq_out = vqs[1];
> +
> +	res_size = video[KPARAM_WIDTH] * video[KPARAM_HEIGHT] * DEFAULT_DEPTH / 8;
> +	fb_size = video[KPARAM_MEM] * 1024 * 1024;
> +
> +	if (res_size > fb_size) {
> +		video[KPARAM_WIDTH] = DEFAULT_WIDTH;
> +		video[KPARAM_HEIGHT] = DEFAULT_HEIGHT;
> +	}
> +
> +	dev_set_drvdata(&dev->dev, info);
> +	info->vdev = dev;
> +
> +	info->fb = rvmalloc(fb_size);
> +	if (info->fb == NULL)

!info->fb

> +		goto error_nomem;
> +
> +	info->nr_pages = (fb_size + PAGE_SIZE - 1) >> PAGE_SHIFT;

PAGE_ALIGN?

> +	info->size = fb_size;
> +
> +	fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL);

make 256 symbolic constant? it's used below as well.

> +	if (fb_info == NULL)
> +		goto error_nomem;
> +
> +	inbuf = kmalloc(PAGE_SIZE * 3, GFP_KERNEL);

replace 3 * PAGE_SIZE with symbolic constant?
Since this seems to be part of guest/host interface,
maybe even put it in header?

> +	info->inbuf = inbuf;
> +	for (i = 0; i < (3 * PAGE_SIZE); i += PAGE_SIZE) {

() not needed around math

> +		struct scatterlist sg;
> +
> +		sg_init_one(&sg, inbuf + i, PAGE_SIZE);
> +		info->vq_in->vq_ops->add_buf(info->vq_in, &sg, 0, 1, inbuf + i);

add_buf can fail.

> +	}
> +	info->vq_in->vq_ops->kick(info->vq_in);
> +
> +	fb_info->pseudo_palette = fb_info->par;
> +	fb_info->par = info;
> +
> +	fb_info->screen_base = info->fb;
> +
> +	fb_info->fbops = &virtio_fb_ops;
> +	fb_info->var.xres_virtual = fb_info->var.xres = video[KPARAM_WIDTH];
> +	fb_info->var.yres_virtual = fb_info->var.yres = video[KPARAM_HEIGHT];
> +	fb_info->var.bits_per_pixel = DEFAULT_DEPTH;
> +
> +	fb_info->var.transp = (struct fb_bitfield){24, 8, 0};
> +	fb_info->var.red    = (struct fb_bitfield){16, 8, 0};
> +	fb_info->var.green  = (struct fb_bitfield){8,  8, 0};
> +	fb_info->var.blue   = (struct fb_bitfield){0,  8, 0};
> +
> +	fb_info->var.activate = FB_ACTIVATE_NOW;
> +	fb_info->var.height = -1;
> +	fb_info->var.width = -1;
> +	fb_info->var.vmode = FB_VMODE_NONINTERLACED;
> +
> +	fb_info->fix.visual = FB_VISUAL_TRUECOLOR;
> +	fb_info->fix.line_length = fb_info->var.xres * DEFAULT_DEPTH / 8;
> +	fb_info->fix.smem_start = 0;
> +	fb_info->fix.smem_len = fb_size;
> +	strcpy(fb_info->fix.id, "virtio_fb");
> +	fb_info->fix.type = FB_TYPE_PACKED_PIXELS;
> +	fb_info->fix.accel = FB_ACCEL_NONE;
> +
> +	fb_info->flags = FBINFO_FLAG_DEFAULT | FBINFO_HWACCEL_COPYAREA |
> +			 FBINFO_READS_FAST;
> +
> +	ret = fb_alloc_cmap(&fb_info->cmap, 256, 0);
> +	if (ret < 0) {
> +		framebuffer_release(fb_info);
> +		goto error;
> +	}
> +
> +	info->cmd_cache = kmem_cache_create("virtio_fb_cmd",
> +					    sizeof(struct virtio_fb_cmd),
> +					    0, 0, NULL);

this allocation leaks on error?

> +

empty line above more confusing than helpful.

> +	if (!info->cmd_cache) {
> +		framebuffer_release(fb_info);
> +		goto error;
> +	}
> +
> +	fb_info->fbdefio = &virtio_fb_defio;
> +	fb_deferred_io_init(fb_info);
> +
> +	INIT_WORK(&info->refresh_work, deferred_refresh);
> +	INIT_WORK(&info->vfree_work, deferred_vfree);
> +	spin_lock_init(&info->vfree_lock);
> +
> +	ret = register_framebuffer(fb_info);
> +	if (ret) {
> +		fb_deferred_io_cleanup(fb_info);
> +		fb_dealloc_cmap(&fb_info->cmap);
> +		framebuffer_release(fb_info);
> +		goto error;

will be less code with a couple more labels to goto on error?

> +	}
> +	info->fb_info = fb_info;
> +
> +	virtio_fb_make_preferred_console();
> +	return 0;
> +
> + error_nomem:
> +	ret = -ENOMEM;
> + error:
> +	framebuffer_release(fb_info);
> +	return ret;
> +}
> +
> +static void virtio_fb_apply_config(struct virtio_device *dev)
> +{
> +}
> +
> +static struct virtio_device_id id_table[] = {
> +	{ VIRTIO_ID_FB, VIRTIO_DEV_ANY_ID },
> +	{ 0 },

0 not needed here.

> +};
> +
> +static unsigned int features[] = {
> +};
> +
> +static struct virtio_driver virtio_console = {
> +	.feature_table = features,
> +	.feature_table_size = ARRAY_SIZE(features),

I think you should just set to NULL and 0 here, and remove the empty
features array.

> +	.driver.name =  KBUILD_MODNAME,
> +	.driver.owner = THIS_MODULE,
> +	.id_table =     id_table,
> +	.probe =        virtio_fb_probe,
> +	.config_changed = virtio_fb_apply_config,

This alignment looks strange. right-align = or just avoid code
alignment.

> +};
> +
> +static int __init init(void)
> +{
> +	return register_virtio_driver(&virtio_console);
> +}
> +module_init(init);

I don't know much about framebuffer. Why do all fb modules
lack module_exit?

> +
> +

extra empty line

> +MODULE_DEVICE_TABLE(virtio, id_table);
> +MODULE_DESCRIPTION("Virtio framebuffer driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
> index 06660c0..72e39f7 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_FB		6 /* virtio framebuffer */
>  #define VIRTIO_ID_9P		9 /* 9p virtio console */
>  
>  #endif /* _LINUX_VIRTIO_IDS_H */
> -- 
> 1.6.0.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add VirtIO Frame Buffer Support

[Michael S. Tsirkin]

On Mon, Nov 02, 2009 at 11:09:19PM +0100, Alexander Graf wrote:
> When we want to create a full VirtIO based machine, we're still missing
> graphics output. Fortunately, Linux provides us with most of the frameworks
> to render text and everything, we only need to implement a transport.
> 
> So this is a frame buffer backend written for VirtIO. Using this and my
> patch to qemu, you can use paravirtualized graphics.
> 
> This is especially important on machines that can't do MMIO, as all current
> graphics implementations qemu emulates I'm aware of so far fail here.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Nice work.  I think you want to Cc
virtualization@lists.linux-foundation.org and Rusty. Cc added.  Some
comments below, some of them checkpatch.pl would find.  I do not know
much about framebuffer, so comments are from virtio usage perspective.
Generally, I think locking is missing here.  As far as I know, virtio
APIs do no locking internally.  So when you e.g. schedule_work and then
call add_buf, this can run on many CPUs in parallel, which will cause
memory corruption.

> ---
>  drivers/video/Kconfig      |   15 +
>  drivers/video/Makefile     |    1 +
>  drivers/video/virtio-fb.c  |  799 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/virtio_ids.h |    1 +
>  4 files changed, 816 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/video/virtio-fb.c
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 9bbb285..f9be4c2 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -2069,6 +2069,21 @@ config XEN_FBDEV_FRONTEND
>  	  frame buffer driver.  It communicates with a back-end
>  	  in another domain.
>  
> +config FB_VIRTIO
> +	tristate "Virtio virtual frame buffer support"
> +	depends on FB && VIRTIO
> +	select FB_SYS_FILLRECT
> +	select FB_SYS_COPYAREA
> +	select FB_SYS_IMAGEBLIT
> +	select FB_SYS_FOPS
> +	select FB_DEFERRED_IO
> +	help
> +	  This driver implements a driver for a Virtio based
> +	  frame buffer device.  It communicates to something that
> +	  can talk Virtio too, most probably a hypervisor.
> +
> +	  If unsure, say N.
> +


Most of virtio is marked experimental. Maybe this should be as well.


>  config FB_METRONOME
>  	tristate "E-Ink Metronome/8track controller support"
>  	depends on FB
> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> index 80232e1..40802c8 100644
> --- a/drivers/video/Makefile
> +++ b/drivers/video/Makefile
> @@ -125,6 +125,7 @@ obj-$(CONFIG_FB_XILINX)           += xilinxfb.o
>  obj-$(CONFIG_FB_SH_MOBILE_LCDC)	  += sh_mobile_lcdcfb.o
>  obj-$(CONFIG_FB_OMAP)             += omap/
>  obj-$(CONFIG_XEN_FBDEV_FRONTEND)  += xen-fbfront.o
> +obj-$(CONFIG_FB_VIRTIO)           += virtio-fb.o
>  obj-$(CONFIG_FB_CARMINE)          += carminefb.o
>  obj-$(CONFIG_FB_MB862XX)	  += mb862xx/
>  obj-$(CONFIG_FB_MSM)              += msm/
> diff --git a/drivers/video/virtio-fb.c b/drivers/video/virtio-fb.c
> new file mode 100644
> index 0000000..2a73950
> --- /dev/null
> +++ b/drivers/video/virtio-fb.c
> @@ -0,0 +1,799 @@
> +/*
> + * VirtIO PV frame buffer device
> + *
> + * Copyright (C) 2009 Alexander Graf <agraf@suse.de>

Out of curiosity, are copyrights your own, or suse/novell's?
> + *
> + *  Based on linux/drivers/video/virtio-fbfront.c

Where is that? Maybe carry attribution from there as well.
There's also a lot of simularity with xen-fbfront.c
Is it accidental?

> + *
> + *  This file is subject to the terms and conditions of the GNU General Public
> + *  License. See the file COPYING in the main directory of this archive for
> + *  more details.
> + */
> +
> +#include <linux/console.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/fb.h>
> +#include <linux/module.h>
> +#include <linux/vmalloc.h>
> +#include <linux/mm.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_config.h>
> +#include <linux/uaccess.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>

is interrupt.h needed? virito mostly
abstracts this for you.

> +
> +#define MAX_BUF			128
> +
> +struct virtio_fb_info {
> +	struct fb_info		*fb_info;
> +	unsigned char		*fb;
> +	char			*inbuf;
> +
> +	u16			width;
> +	u16			height;
> +
> +	int			nr_pages;
> +	int			size;
> +
> +	struct kmem_cache	*cmd_cache;
> +	struct virtqueue	*vq_in;
> +	struct virtqueue	*vq_out;
> +	struct virtio_device	*vdev;
> +
> +	void			*last_buf[MAX_BUF];
> +	int			last_buf_idx;
> +	spinlock_t		vfree_lock;
> +	struct work_struct	vfree_work;
> +	struct work_struct	refresh_work;
> +};
> +
> +/* guest -> Host commands */
> +#define VIRTIO_FB_CMD_RESIZE            0x01                                         
> +#define VIRTIO_FB_CMD_FILL              0x02                                         
> +#define VIRTIO_FB_CMD_BLIT              0x03                                         
> +#define VIRTIO_FB_CMD_COPY              0x04                                         
> +#define VIRTIO_FB_CMD_WRITE             0x05                                         
> +    
> +/* host -> guest commands */                                                         
> +#define VIRTIO_FB_CMD_REFRESH           0x81                                         


These constants and structures (also below) should go into linux/virtio-fb.h
which should be exported. You then won't have to
duplicate them in userspace. Just remember to convert uXX to __uXX.

> +
> +struct virtio_fb_cmd {
> +	u8			cmd;
> +	union {
> +		struct {
> +			u16	width;
> +			u16	height;
> +		} resize __attribute__ ((packed));
> +		struct {
> +			u16	x;
> +			u16	y;
> +			u16	width;
> +			u16	height;
> +		} blit __attribute__ ((packed));
> +		struct {
> +			u16	x1;
> +			u16	y1;
> +			u16	x2;
> +			u16	y2;
> +			u16	width;
> +			u16	height;
> +		} copy_area __attribute__ ((packed));
> +		struct {
> +			u8	rop;
> +			u16	x;
> +			u16	y;
> +			u16	width;
> +			u16	height;
> +			u32	color;
> +		} fill __attribute__ ((packed));
> +		struct {
> +			u64	offset;
> +			u64	count;
> +		} write __attribute__ ((packed));
> +		u8		pad[23];
> +	};
> +
> +	union {
> +		/* We remember the data pointer so we we can easily free
> +		   everything later by only knowing this structure */
> +		char		*send_buf;

I thought this structure is passed between guest and host?
If so you do not want to stick your pointers there,
add_buf has data pointer for this reason.

> +		u64		_pad;

This might create problems with 32 bit userspace on 64 bit kernels:
which bits does the pointer get packed into depends on architecture.
Better to just pass in __u64 and have userspace cast pointer to that
type, IMO.


> +	};
> +} __attribute__ ((packed));


Packed structures generate terrible code on some architectures.  Can you
just pad structures to multiples of 64 bit, or to full union size, instead?

> +
> +enum copy_type {
> +	COPY_KERNEL,
> +	COPY_USER,
> +	COPY_NOCOPY,
> +};
> +
> +#define DEFAULT_WIDTH		800
> +#define DEFAULT_HEIGHT		600
> +#define DEFAULT_DEPTH		32
> +#define DEFAULT_MEM		8
> +
> +#define DEFAULT_FB_LEN (DEFAULT_WIDTH * DEFAULT_HEIGHT * DEFAULT_DEPTH / 8)
> +
> +enum { KPARAM_MEM, KPARAM_WIDTH, KPARAM_HEIGHT, KPARAM_CNT };
> +static int video[KPARAM_CNT] = { DEFAULT_MEM, DEFAULT_WIDTH, DEFAULT_HEIGHT };
> +module_param_array(video, int, NULL, 0);
> +MODULE_PARM_DESC(video,
> +	"Video size in M,width,height in pixels (default \""
> +	str(DEFAULT_MEM) ","
> +	str(DEFAULT_WIDTH) ","
> +	str(DEFAULT_HEIGHT) "\")");
> +
> +static void virtio_fb_output(struct virtqueue *vq);
> +
> +static void *rvmalloc(unsigned long size)

what exactly does this do?

> +{
> +	void *mem;
> +	unsigned long adr;
> +
> +	size = PAGE_ALIGN(size);
> +	mem = vmalloc_32(size);
> +	if (!mem)
> +		return NULL;
> +
> +	memset(mem, 0, size); /* Clear the ram out, no junk to the user */
> +	adr = (unsigned long) mem;
> +	while (size > 0) {
> +		SetPageReserved(vmalloc_to_page((void *)adr));

Where is the bit cleared?

> +		adr += PAGE_SIZE;
> +		size -= PAGE_SIZE;
> +	}
> +
> +	return mem;
> +}
> +
> +/* This is videobuf_vmalloc_to_sg() from videobuf-dma-sg.c
> +   I modified it to take an extra sg entry for the cmd and work with non
> +   page-aligned pointers though */
> +static struct scatterlist* vmalloc_to_sg(unsigned char *virt, int length,
> +					 void *cmd, int cmd_len, int *sg_elem)
> +{
> +	struct scatterlist *sglist;
> +	struct page *pg;
> +	int nr_pages = (length+PAGE_SIZE-1)/PAGE_SIZE;

Spaces are needed around +, -

> +	int sg_entries;
> +	int i;
> +
> +	/* unaligned */
> +	if ((ulong)virt & ~PAGE_MASK) {

Use offset_in_page here and below?

> +		int tmp_len = length - (PAGE_SIZE - ((ulong)virt & ~PAGE_MASK));
> +		/* how long would it be without the first non-aligned chunk? */
> +		nr_pages = (tmp_len+PAGE_SIZE-1)/PAGE_SIZE;

Spaces are needed around +, -
Also, is this just PAGE_ALIGN?

> +		/* add the first chunk */
> +		nr_pages++;
> +	}

Is all the above just
	nr_pages = PAGE_ALIGN(length + offset_in_page(virt)) / PAGE_SIZE
?
if so, no need for if statements etc.

> +
> +	sg_entries = nr_pages + 1;
> +
> +	sglist = kcalloc(sg_entries, sizeof(struct scatterlist), GFP_KERNEL);
> +	if (!sglist)
> +		return NULL;
> +	sg_init_table(sglist, sg_entries);
> +
> +	/* Put cmd element in */
> +	sg_set_buf(&sglist[0], cmd, cmd_len);
> +
> +	/* Fill with elements for the data */
> +	for (i = 1; i < sg_entries; i++) {
> +		pg = vmalloc_to_page(virt);
> +		if (!pg)
> +			goto err;
> +
> +		if ((ulong)virt & ~PAGE_MASK) {
> +			int tmp_off = ((ulong)virt & ~PAGE_MASK);
> +
> +			sg_set_page(&sglist[i], pg, PAGE_SIZE - tmp_off, tmp_off);
> +			virt = (char*)((ulong)virt & PAGE_MASK);
> +		} else {
> +			sg_set_page(&sglist[i], pg, PAGE_SIZE, 0);
> +		}

You don't need an if statement, do you?
For aligned addresses tmp_off is 0, so it works out.

> +		virt += PAGE_SIZE;
> +	}
> +
> +	*sg_elem = sg_entries;
> +	return sglist;
> +
> + err:
> +	kfree(sglist);
> +	return NULL;
> +}
> +
> +
> +static void _virtio_fb_send(struct virtio_fb_info *info,

void? Don't we care that this might fail e.g. on OOM?

> +			    struct virtio_fb_cmd *cmd,
> +			    char *buf, int len, enum copy_type copy)
> +{
> +	char *send_buf = NULL;
> +	char *sg_buf = NULL;
> +	struct virtio_fb_cmd *send_cmd;
> +	struct scatterlist *sg;
> +	int len_cmd = sizeof(struct virtio_fb_cmd);

sizeof *cmd would be 

> +	int r, sg_elem;
> +
> +	send_cmd = kmem_cache_alloc(info->cmd_cache, GFP_KERNEL);
> +	if (!send_cmd) {
> +		printk(KERN_ERR "virtio-fb: couldn't allocate cmd\n");
> +		return;
> +	}
> +
> +	memcpy(send_cmd, cmd, len_cmd);
> +
> +	if (len) {
> +		sg_buf = send_buf = vmalloc(len);
> +		if (!send_buf) {
> +			printk(KERN_ERR "virtio-fb: couldn't allocate %d b\n",
> +					len);
> +			return;
> +		}
> +
> +		switch (copy) {
> +		case COPY_KERNEL:
> +			memcpy(send_buf, buf, len);
> +			break;
> +		case COPY_USER:
> +			r = copy_from_user(send_buf, (const __user char*)buf,
> +					   len);
> +			break;
> +		case COPY_NOCOPY:
> +			sg_buf = buf;

vmalloc is not really needed in this case, is it?

> +			break;
> +		}
> +	}
> +
> +	send_cmd->send_buf = send_buf;
> +
> +	sg = vmalloc_to_sg(sg_buf, len, send_cmd, len_cmd, &sg_elem);
> +	if (!sg) {
> +		printk(KERN_ERR "virtio-fb: couldn't gather scatter list\n");
> +		return;
> +	}
> +
> +add_again:
> +	r = info->vq_out->vq_ops->add_buf(info->vq_out, sg, sg_elem, 0, send_cmd);


This seems to be done without any locks.
Just making sure: what guarantees that multiple CPUs do
not do this in parallel? Note that virtio do no
locking internally, it's always up to the user.

> +	info->vq_out->vq_ops->kick(info->vq_out);
> +
> +	if ( r == -ENOSPC ) {

what about other errors?

> +		/* Queue was full, so try again */
> +		cpu_relax();
> +		virtio_fb_output(info->vq_out);
> +		goto add_again;

That's pretty evil. Is it possible to block for interrupt
until vq becomes empty, use a timer instead, or
avoid posting more than VQ size in some other way?

> +	}

Can this use for or while loop?

> +
> +	kfree(sg);
> +}
> +
> +static void virtio_fb_send_user(struct virtio_fb_info *info,
> +				struct virtio_fb_cmd *cmd,
> +				const __user char *buf, int len)
> +{
> +	_virtio_fb_send(info, cmd, (char *)buf, len, COPY_USER);


Casting __user pointer away in this way gives up type
safety. Maybe it would be a good idea to split the copy part
away from _virtio_fb_send? Then _nocopy won't do any vmalloc,
this function will do vmalloc+copy_from_user, and virtio_fb_send
below will do vmalloc + memcpy.

> +}
> +
> +static void virtio_fb_send_nocopy(struct virtio_fb_info *info,
> +				  struct virtio_fb_cmd *cmd,
> +				  char *buf, int len)
> +{
> +	_virtio_fb_send(info, cmd, buf, len, COPY_NOCOPY);
> +}
> +
> +static void virtio_fb_send(struct virtio_fb_info *info, struct virtio_fb_cmd *cmd,
> +			   char *buf, int len)
> +{
> +	_virtio_fb_send(info, cmd, buf, len, COPY_KERNEL);
> +}
> +
> +

2 empty lines

> +static int virtio_fb_setcolreg(unsigned regno, unsigned red, unsigned green,
> +			   unsigned blue, unsigned transp,
> +			   struct fb_info *info)
> +{
> +	u32 v;
> +	if (regno >= 16)
> +		return 1;
> +
> +#define CNVT_TOHW(val,width) ((((val)<<(width))+0x7FFF-(val))>>16)

spaces are needed around <<, >>, -, +

> +	red = CNVT_TOHW(red, info->var.red.length);
> +	green = CNVT_TOHW(green, info->var.green.length);
> +	blue = CNVT_TOHW(blue, info->var.blue.length);
> +	transp = CNVT_TOHW(transp, info->var.transp.length);
> +#undef CNVT_TOHW

this is what xen does as well. Any chance to use symbolic
constants above?
Also, are var.XXXX.length values in fact constant?

> +
> +

extra empty line

> +	v = (red << info->var.red.offset) |
> +	    (green << info->var.green.offset) |
> +	    (blue << info->var.blue.offset) |
> +	    (transp << info->var.transp.offset);
> +
> +	((u32 *) (info->pseudo_palette))[regno] = v;

space between } and ( above is just confusing.

> +
> +	return 0;
> +}
> +
> +static void virtio_fb_fillrect(struct fb_info *p, const struct fb_fillrect *rect)
> +{
> +	struct virtio_fb_info *info = p->par;
> +	struct virtio_fb_cmd cmd;
> +
> +	sys_fillrect(p, rect);
> +
> +	cmd.cmd = VIRTIO_FB_CMD_FILL;
> +	cmd.fill.rop = rect->rop;
> +	cmd.fill.x = rect->dx;
> +	cmd.fill.y = rect->dy;
> +	cmd.fill.width = rect->width;
> +	cmd.fill.height = rect->height;
> +	cmd.fill.color = rect->color;
> +
> +	virtio_fb_send(info, &cmd, NULL, 0);
> +}
> +
> +static void virtio_fb_imageblit(struct fb_info *p, const struct fb_image *image)
> +{
> +	struct virtio_fb_info *info = p->par;
> +	struct virtio_fb_cmd cmd;

So this puts a large structure n stack, only to memcpy
it later into a kmalloced one. I think it would be
better just to allocate command from cache directly, for
kernel users. Same comment would apply to others below.

> +	char *_buf;
> +	char *buf;
> +	char *vfb;
> +	int i, w;
> +	int bpp = p->var.bits_per_pixel / 8;
> +	int len = image->width * image->height * bpp;
> +
> +	sys_imageblit(p, image);
> +
> +	cmd.cmd = VIRTIO_FB_CMD_BLIT;
> +	cmd.blit.x = image->dx;
> +	cmd.blit.y = image->dy;
> +	cmd.blit.height = image->height;
> +	cmd.blit.width = image->width;
> +
> +	if (image->depth == 32) {
> +		/* Send the image off directly */
> +
> +		virtio_fb_send(info, &cmd, (char*)image->data, len);

You cast constness away? that's usually not a good thing to do.

> +		return;
> +	}
> +
> +	/* Send the 32-bit translated image */
> +
> +	buf = _buf = vmalloc(len);
> +
> +	vfb = info->fb;
> +	vfb += (image->dy * p->fix.line_length) + image->dx * bpp;

() not needed around *.

> +
> +	w = image->width * bpp;
> +
> +	for (i = 0; i < image->height; i++) {
> +		memcpy(buf, vfb, w);
> +
> +		buf += w;
> +		vfb += p->fix.line_length;
> +	}
> +
> +	virtio_fb_send(info, &cmd, _buf, len);
> +
> +	vfree(_buf);

This would benefit from nocopy, right?
Just another arument why what I propose above is a good idea.

> +}
> +
> +static void virtio_fb_copyarea(struct fb_info *p, const struct fb_copyarea *area)
> +{
> +	struct virtio_fb_info *info = p->par;
> +	struct virtio_fb_cmd cmd;
> +
> +	sys_copyarea(p, area);
> +
> +	cmd.cmd = VIRTIO_FB_CMD_COPY;
> +	cmd.copy_area.x1 = area->sx;
> +	cmd.copy_area.y1 = area->sy;
> +	cmd.copy_area.x2 = area->dx;
> +	cmd.copy_area.y2 = area->dy;
> +	cmd.copy_area.width = area->width;
> +	cmd.copy_area.height = area->height;
> +
> +	virtio_fb_send(info, &cmd, NULL, 0);
> +}
> +
> +static ssize_t virtio_fb_write(struct fb_info *p, const char __user *buf,
> +			size_t count, loff_t *ppos)
> +{
> +	struct virtio_fb_info *info = p->par;
> +	struct virtio_fb_cmd cmd;
> +	loff_t pos = *ppos;
> +	ssize_t res;
> +
> +	res = fb_sys_write(p, buf, count, ppos);
> +
> +	cmd.cmd = VIRTIO_FB_CMD_WRITE;
> +	cmd.write.offset = pos;
> +	cmd.write.count = count;
> +
> +	virtio_fb_send_user(info, &cmd, buf, count);
> +	return res;
> +}
> +
> +static int
> +virtio_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *p)
> +{
> +	struct virtio_fb_info *info = p->par;
> +
> +	if (var->bits_per_pixel != DEFAULT_DEPTH) {
> +		/* We only support 32 bpp */
> +		return -EINVAL;
> +	}
> +
> +	if ((var->xres * var->yres * DEFAULT_DEPTH / 8) > info->size) {

don't need () around math

> +		/* Doesn't fit in the frame buffer */
> +		return -EINVAL;
> +	}
> +
> +	var->xres_virtual = var->xres;
> +	var->yres_virtual = var->yres;
> +
> +	return 0;
> +}
> +
> +static int virtio_fb_set_par(struct fb_info *p)
> +{
> +	struct virtio_fb_info *info = p->par;
> +	struct virtio_fb_cmd cmd;
> +
> +	/* Do nothing if we're on that resolution already */
> +	if ((p->var.xres == info->width) &&
> +	    (p->var.yres == info->height))

don't need () around ===

> +		return 0;
> +
> +	info->width = p->var.xres;
> +	info->height = p->var.yres;
> +	p->fix.line_length = p->var.xres_virtual * 4;
> +
> +	/* Notify hypervisor */
> +
> +	cmd.cmd = VIRTIO_FB_CMD_RESIZE;
> +	cmd.resize.width = p->var.xres;
> +	cmd.resize.height = p->var.yres;
> +
> +	virtio_fb_send(info, &cmd, NULL, 0);
> +
> +	return 0;
> +}
> +
> +static struct fb_ops virtio_fb_ops = {
> +	.owner		= THIS_MODULE,
> +	.fb_read	= fb_sys_read,
> +	.fb_write	= virtio_fb_write,
> +	.fb_setcolreg	= virtio_fb_setcolreg,
> +	.fb_fillrect	= virtio_fb_fillrect,
> +	.fb_copyarea	= virtio_fb_copyarea,
> +	.fb_imageblit	= virtio_fb_imageblit,
> +	.fb_check_var	= virtio_fb_check_var,
> +	.fb_set_par     = virtio_fb_set_par,
> +};
> +
> +static __devinit void
> +virtio_fb_make_preferred_console(void)
> +{
> +	struct console *c;
> +
> +	if (console_set_on_cmdline)
> +		return;
> +
> +	acquire_console_sem();
> +	for (c = console_drivers; c; c = c->next) {
> +		if (!strcmp(c->name, "tty") && c->index == 0)
> +			break;
> +	}

{} not needed

> +	release_console_sem();

Just a thought: can console c go away at this point?

> +	if (c) {
> +		unregister_console(c);
> +		c->flags |= CON_CONSDEV;
> +		c->flags &= ~CON_PRINTBUFFER; /* don't print again */
> +		register_console(c);
> +	}
> +}
> +
> +static void virtio_fb_deferred_io(struct fb_info *fb_info,
> +				  struct list_head *pagelist)
> +{
> +	struct virtio_fb_info *info = fb_info->par;
> +	struct page *page;
> +	unsigned long beg, end;
> +	int y1, y2, miny, maxy;
> +	struct virtio_fb_cmd cmd;
> +
> +	miny = INT_MAX;
> +	maxy = 0;
> +	list_for_each_entry(page, pagelist, lru) {
> +		beg = page->index << PAGE_SHIFT;

page_index()?

> +		end = beg + PAGE_SIZE - 1;
> +		y1 = beg / fb_info->fix.line_length;

You do all these divisions in a cycle, then end up multiplying
the result by line_length. Maybe do the math in bytes.

> +		y2 = end / fb_info->fix.line_length;

Is the result guaranteed to fit in int?

> +		if (y2 >= fb_info->var.yres)
> +			y2 = fb_info->var.yres - 1;
> +		if (miny > y1)
> +			miny = y1;
> +		if (maxy < y2)
> +			maxy = y2;

use min()/max() macros above.

> +	}
> +
> +	if (miny != INT_MAX) {

if (miny == INT_MAX)
	return;

will be neater

> +		cmd.cmd = VIRTIO_FB_CMD_WRITE;
> +		cmd.write.offset = miny * fb_info->fix.line_length;
> +		cmd.write.count = (maxy - miny + 1) * fb_info->fix.line_length;
> +
> +		virtio_fb_send_nocopy(info, &cmd, info->fb + cmd.write.offset,
> +				      cmd.write.count);
> +	}
> +}
> +
> +static struct fb_deferred_io virtio_fb_defio = {
> +	.delay		= HZ / 20,
> +	.deferred_io	= virtio_fb_deferred_io,
> +};
> +
> +/* Callback when the host kicks our input queue.
> + *
> + * This is to enable notifications from host to guest. */
> +static void virtio_fb_input(struct virtqueue *vq)
> +{
> +	struct virtio_fb_info *info = dev_get_drvdata(&vq->vdev->dev);
> +	int len, i;
> +	void *x;
> +	void *reinject[3];
> +	int reinject_count = 0;
> +
> +	while ((x = vq->vq_ops->get_buf(vq, &len)) != NULL)

&& reinject_count < 3?

> {

This looks unsafe: can this get triggered while another
routine does add_buf? If yes you must add locking to
prevent this.

> +		struct virtio_fb_cmd *cmd = x;
> +
> +		/* Make sure we're getting an inbuf page! */

What does this check, exactly? 

> +		BUG_ON((x != info->inbuf) &&
> +		       (x != (info->inbuf + PAGE_SIZE)) &&
> +		       (x != (info->inbuf + (PAGE_SIZE * 2))));
() not needed around math.

> +
> +		switch (cmd->cmd) {
> +		case VIRTIO_FB_CMD_REFRESH:
> +			schedule_work(&info->refresh_work);
> +			break;
> +		}
> +
> +		reinject[reinject_count++] = x;

This will overflow on a buggy host.  Better check and BUG(); Also, the
number of buffers is VQ size?
Make it a constant then.

> +	}
> +
> +
> +	for (i = 0; i < reinject_count; i++) {
> +		struct scatterlist sg;
> +		void *x = reinject[i];
> +
> +		sg_init_one(&sg, x, PAGE_SIZE);
> +		vq->vq_ops->add_buf(vq, &sg, 0, 1, x);
> +		vq->vq_ops->kick(vq);

won't it be easier to reinject wach buffer as you get it directly?

> +	}
> +}
> +
> +/* Asynchronous snippet to send all screen contents to the host */
> +static void deferred_refresh(struct work_struct *work)
> +{
> +	struct virtio_fb_info *info = container_of(work, struct virtio_fb_info,
> +						   refresh_work);
> +	struct virtio_fb_cmd cmd;
> +
> +	cmd.cmd = VIRTIO_FB_CMD_WRITE;
> +	cmd.write.offset = 0;
> +	cmd.write.count = info->width * info->height * 4;

why 4?

> +
> +	virtio_fb_send_nocopy(info, &cmd, info->fb, cmd.write.count);
> +}
> +
> +/* Asynchronous garbage collector :-) */
> +static void deferred_vfree(struct work_struct *work)
> +{
> +	struct virtio_fb_info *info = container_of(work, struct virtio_fb_info,
> +						   vfree_work);
> +	int i;
> +	unsigned long flags;
> +	void *last_buf[MAX_BUF];

Wow, that's a lot of stack.  Can't vfree be called on info->last_buf
directly?

> +	int idx;
> +
> +	spin_lock_irqsave(&info->vfree_lock, flags);


spin_lock_irq is enough

> +
> +	idx = info->last_buf_idx;
> +	memcpy(last_buf, info->last_buf, sizeof(void*) * MAX_BUF);

sizeof last_buf

> +	info->last_buf_idx = 0;
> +
> +	spin_unlock_irqrestore(&info->vfree_lock, flags);
> +
> +	for (i = 0; i < idx; i++) {
> +		vfree(last_buf[i]);
> +	}

{} not needed

> +}
> +
> +/* Callback when the host kicks our output queue. This can only mean it's done
> + * processing an item, so let's free up the memory occupied by the entries */


lines too long here and elsewhere

> +static void virtio_fb_output(struct virtqueue *vq)
> +{
> +	struct virtio_fb_info *info = dev_get_drvdata(&vq->vdev->dev);
> +	int len;
> +	void *x;
> +
> +	while ((x = vq->vq_ops->get_buf(vq, &len)) != NULL) {

!= NULL not needed.
Again, get_buf must be called under some
lock that prevents add_buf from being called.

> +		struct virtio_fb_cmd *cmd = x;
> +
> +		if (cmd->send_buf) {
> +			spin_lock(&info->vfree_lock);
> +			if (info->last_buf_idx != MAX_BUF) {
> +				info->last_buf[info->last_buf_idx++] =
> +					cmd->send_buf;
> +			}

{} not needed

> +			spin_unlock(&info->vfree_lock);
> +
> +			schedule_work(&info->vfree_work);

So this would be a good place to re-enable more output
instead of busy wait above? Also, can't we vfree
here directly?

> +		}
> +
> +		kmem_cache_free(info->cmd_cache, x);
> +	}
> +}
> +
> +static int __devinit virtio_fb_probe(struct virtio_device *dev)
> +{
> +	vq_callback_t *callbacks[] = { virtio_fb_input, virtio_fb_output };
> +	const char *names[] = { "input", "output" };
> +	struct virtio_fb_info *info;
> +	struct virtqueue *vqs[2];
> +	struct fb_info *fb_info = NULL;
> +	int fb_size, res_size;
> +	int ret, err, i;
> +	char *inbuf;
> +
> +	err = dev->config->find_vqs(dev, 2, vqs, callbacks, names);

2 -> ARRAY_SIZE(vqs).

> +	if (err) {
> +		printk(KERN_ERR "VirtIO FB: couldn't find VQs\n");
> +		return err;
> +	}

You probably want del_vqs on error as well?

> +
> +	info = kmalloc(sizeof(*info), GFP_KERNEL);

this seems to be leaked on errors?
() not needed around *info.

> +	if (info == NULL)

!info

> +		return -ENOMEM;
> +
> +	info->vq_in = vqs[0];
> +	info->vq_out = vqs[1];
> +
> +	res_size = video[KPARAM_WIDTH] * video[KPARAM_HEIGHT] * DEFAULT_DEPTH / 8;
> +	fb_size = video[KPARAM_MEM] * 1024 * 1024;
> +
> +	if (res_size > fb_size) {
> +		video[KPARAM_WIDTH] = DEFAULT_WIDTH;
> +		video[KPARAM_HEIGHT] = DEFAULT_HEIGHT;
> +	}
> +
> +	dev_set_drvdata(&dev->dev, info);
> +	info->vdev = dev;
> +
> +	info->fb = rvmalloc(fb_size);
> +	if (info->fb == NULL)

!info->fb

> +		goto error_nomem;
> +
> +	info->nr_pages = (fb_size + PAGE_SIZE - 1) >> PAGE_SHIFT;

PAGE_ALIGN?

> +	info->size = fb_size;
> +
> +	fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL);

make 256 symbolic constant? it's used below as well.

> +	if (fb_info == NULL)
> +		goto error_nomem;
> +
> +	inbuf = kmalloc(PAGE_SIZE * 3, GFP_KERNEL);

replace 3 * PAGE_SIZE with symbolic constant?
Since this seems to be part of guest/host interface,
maybe even put it in header?

> +	info->inbuf = inbuf;
> +	for (i = 0; i < (3 * PAGE_SIZE); i += PAGE_SIZE) {

() not needed around math

> +		struct scatterlist sg;
> +
> +		sg_init_one(&sg, inbuf + i, PAGE_SIZE);
> +		info->vq_in->vq_ops->add_buf(info->vq_in, &sg, 0, 1, inbuf + i);

add_buf can fail.

> +	}
> +	info->vq_in->vq_ops->kick(info->vq_in);
> +
> +	fb_info->pseudo_palette = fb_info->par;
> +	fb_info->par = info;
> +
> +	fb_info->screen_base = info->fb;
> +
> +	fb_info->fbops = &virtio_fb_ops;
> +	fb_info->var.xres_virtual = fb_info->var.xres = video[KPARAM_WIDTH];
> +	fb_info->var.yres_virtual = fb_info->var.yres = video[KPARAM_HEIGHT];
> +	fb_info->var.bits_per_pixel = DEFAULT_DEPTH;
> +
> +	fb_info->var.transp = (struct fb_bitfield){24, 8, 0};
> +	fb_info->var.red    = (struct fb_bitfield){16, 8, 0};
> +	fb_info->var.green  = (struct fb_bitfield){8,  8, 0};
> +	fb_info->var.blue   = (struct fb_bitfield){0,  8, 0};
> +
> +	fb_info->var.activate = FB_ACTIVATE_NOW;
> +	fb_info->var.height = -1;
> +	fb_info->var.width = -1;
> +	fb_info->var.vmode = FB_VMODE_NONINTERLACED;
> +
> +	fb_info->fix.visual = FB_VISUAL_TRUECOLOR;
> +	fb_info->fix.line_length = fb_info->var.xres * DEFAULT_DEPTH / 8;
> +	fb_info->fix.smem_start = 0;
> +	fb_info->fix.smem_len = fb_size;
> +	strcpy(fb_info->fix.id, "virtio_fb");
> +	fb_info->fix.type = FB_TYPE_PACKED_PIXELS;
> +	fb_info->fix.accel = FB_ACCEL_NONE;
> +
> +	fb_info->flags = FBINFO_FLAG_DEFAULT | FBINFO_HWACCEL_COPYAREA |
> +			 FBINFO_READS_FAST;
> +
> +	ret = fb_alloc_cmap(&fb_info->cmap, 256, 0);
> +	if (ret < 0) {
> +		framebuffer_release(fb_info);
> +		goto error;
> +	}
> +
> +	info->cmd_cache = kmem_cache_create("virtio_fb_cmd",
> +					    sizeof(struct virtio_fb_cmd),
> +					    0, 0, NULL);

this allocation leaks on error?

> +

empty line above more confusing than helpful.

> +	if (!info->cmd_cache) {
> +		framebuffer_release(fb_info);
> +		goto error;
> +	}
> +
> +	fb_info->fbdefio = &virtio_fb_defio;
> +	fb_deferred_io_init(fb_info);
> +
> +	INIT_WORK(&info->refresh_work, deferred_refresh);
> +	INIT_WORK(&info->vfree_work, deferred_vfree);
> +	spin_lock_init(&info->vfree_lock);
> +
> +	ret = register_framebuffer(fb_info);
> +	if (ret) {
> +		fb_deferred_io_cleanup(fb_info);
> +		fb_dealloc_cmap(&fb_info->cmap);
> +		framebuffer_release(fb_info);
> +		goto error;

will be less code with a couple more labels to goto on error?

> +	}
> +	info->fb_info = fb_info;
> +
> +	virtio_fb_make_preferred_console();
> +	return 0;
> +
> + error_nomem:
> +	ret = -ENOMEM;
> + error:
> +	framebuffer_release(fb_info);
> +	return ret;
> +}
> +
> +static void virtio_fb_apply_config(struct virtio_device *dev)
> +{
> +}
> +
> +static struct virtio_device_id id_table[] = {
> +	{ VIRTIO_ID_FB, VIRTIO_DEV_ANY_ID },
> +	{ 0 },

0 not needed here.

> +};
> +
> +static unsigned int features[] = {
> +};
> +
> +static struct virtio_driver virtio_console = {
> +	.feature_table = features,
> +	.feature_table_size = ARRAY_SIZE(features),

I think you should just set to NULL and 0 here, and remove the empty
features array.

> +	.driver.name =  KBUILD_MODNAME,
> +	.driver.owner = THIS_MODULE,
> +	.id_table =     id_table,
> +	.probe =        virtio_fb_probe,
> +	.config_changed = virtio_fb_apply_config,

This alignment looks strange. right-align = or just avoid code
alignment.

> +};
> +
> +static int __init init(void)
> +{
> +	return register_virtio_driver(&virtio_console);
> +}
> +module_init(init);

I don't know much about framebuffer. Why do all fb modules
lack module_exit?

> +
> +

extra empty line

> +MODULE_DEVICE_TABLE(virtio, id_table);
> +MODULE_DESCRIPTION("Virtio framebuffer driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
> index 06660c0..72e39f7 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_FB		6 /* virtio framebuffer */
>  #define VIRTIO_ID_9P		9 /* 9p virtio console */
>  
>  #endif /* _LINUX_VIRTIO_IDS_H */
> -- 
> 1.6.0.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Qemu-devel] Re: [PATCH] Add VirtIO Frame Buffer Support

[Michael S. Tsirkin]

On Mon, Nov 02, 2009 at 11:09:19PM +0100, Alexander Graf wrote:
> When we want to create a full VirtIO based machine, we're still missing
> graphics output. Fortunately, Linux provides us with most of the frameworks
> to render text and everything, we only need to implement a transport.
> 
> So this is a frame buffer backend written for VirtIO. Using this and my
> patch to qemu, you can use paravirtualized graphics.
> 
> This is especially important on machines that can't do MMIO, as all current
> graphics implementations qemu emulates I'm aware of so far fail here.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Nice work.  I think you want to Cc
virtualization@lists.linux-foundation.org and Rusty. Cc added.  Some
comments below, some of them checkpatch.pl would find.  I do not know
much about framebuffer, so comments are from virtio usage perspective.
Generally, I think locking is missing here.  As far as I know, virtio
APIs do no locking internally.  So when you e.g. schedule_work and then
call add_buf, this can run on many CPUs in parallel, which will cause
memory corruption.

> ---
>  drivers/video/Kconfig      |   15 +
>  drivers/video/Makefile     |    1 +
>  drivers/video/virtio-fb.c  |  799 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/virtio_ids.h |    1 +
>  4 files changed, 816 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/video/virtio-fb.c
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 9bbb285..f9be4c2 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -2069,6 +2069,21 @@ config XEN_FBDEV_FRONTEND
>  	  frame buffer driver.  It communicates with a back-end
>  	  in another domain.
>  
> +config FB_VIRTIO
> +	tristate "Virtio virtual frame buffer support"
> +	depends on FB && VIRTIO
> +	select FB_SYS_FILLRECT
> +	select FB_SYS_COPYAREA
> +	select FB_SYS_IMAGEBLIT
> +	select FB_SYS_FOPS
> +	select FB_DEFERRED_IO
> +	help
> +	  This driver implements a driver for a Virtio based
> +	  frame buffer device.  It communicates to something that
> +	  can talk Virtio too, most probably a hypervisor.
> +
> +	  If unsure, say N.
> +


Most of virtio is marked experimental. Maybe this should be as well.


>  config FB_METRONOME
>  	tristate "E-Ink Metronome/8track controller support"
>  	depends on FB
> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> index 80232e1..40802c8 100644
> --- a/drivers/video/Makefile
> +++ b/drivers/video/Makefile
> @@ -125,6 +125,7 @@ obj-$(CONFIG_FB_XILINX)           += xilinxfb.o
>  obj-$(CONFIG_FB_SH_MOBILE_LCDC)	  += sh_mobile_lcdcfb.o
>  obj-$(CONFIG_FB_OMAP)             += omap/
>  obj-$(CONFIG_XEN_FBDEV_FRONTEND)  += xen-fbfront.o
> +obj-$(CONFIG_FB_VIRTIO)           += virtio-fb.o
>  obj-$(CONFIG_FB_CARMINE)          += carminefb.o
>  obj-$(CONFIG_FB_MB862XX)	  += mb862xx/
>  obj-$(CONFIG_FB_MSM)              += msm/
> diff --git a/drivers/video/virtio-fb.c b/drivers/video/virtio-fb.c
> new file mode 100644
> index 0000000..2a73950
> --- /dev/null
> +++ b/drivers/video/virtio-fb.c
> @@ -0,0 +1,799 @@
> +/*
> + * VirtIO PV frame buffer device
> + *
> + * Copyright (C) 2009 Alexander Graf <agraf@suse.de>

Out of curiosity, are copyrights your own, or suse/novell's?
> + *
> + *  Based on linux/drivers/video/virtio-fbfront.c

Where is that? Maybe carry attribution from there as well.
There's also a lot of simularity with xen-fbfront.c
Is it accidental?

> + *
> + *  This file is subject to the terms and conditions of the GNU General Public
> + *  License. See the file COPYING in the main directory of this archive for
> + *  more details.
> + */
> +
> +#include <linux/console.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/fb.h>
> +#include <linux/module.h>
> +#include <linux/vmalloc.h>
> +#include <linux/mm.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_config.h>
> +#include <linux/uaccess.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>

is interrupt.h needed? virito mostly
abstracts this for you.

> +
> +#define MAX_BUF			128
> +
> +struct virtio_fb_info {
> +	struct fb_info		*fb_info;
> +	unsigned char		*fb;
> +	char			*inbuf;
> +
> +	u16			width;
> +	u16			height;
> +
> +	int			nr_pages;
> +	int			size;
> +
> +	struct kmem_cache	*cmd_cache;
> +	struct virtqueue	*vq_in;
> +	struct virtqueue	*vq_out;
> +	struct virtio_device	*vdev;
> +
> +	void			*last_buf[MAX_BUF];
> +	int			last_buf_idx;
> +	spinlock_t		vfree_lock;
> +	struct work_struct	vfree_work;
> +	struct work_struct	refresh_work;
> +};
> +
> +/* guest -> Host commands */
> +#define VIRTIO_FB_CMD_RESIZE            0x01                                         
> +#define VIRTIO_FB_CMD_FILL              0x02                                         
> +#define VIRTIO_FB_CMD_BLIT              0x03                                         
> +#define VIRTIO_FB_CMD_COPY              0x04                                         
> +#define VIRTIO_FB_CMD_WRITE             0x05                                         
> +    
> +/* host -> guest commands */                                                         
> +#define VIRTIO_FB_CMD_REFRESH           0x81                                         


These constants and structures (also below) should go into linux/virtio-fb.h
which should be exported. You then won't have to
duplicate them in userspace. Just remember to convert uXX to __uXX.

> +
> +struct virtio_fb_cmd {
> +	u8			cmd;
> +	union {
> +		struct {
> +			u16	width;
> +			u16	height;
> +		} resize __attribute__ ((packed));
> +		struct {
> +			u16	x;
> +			u16	y;
> +			u16	width;
> +			u16	height;
> +		} blit __attribute__ ((packed));
> +		struct {
> +			u16	x1;
> +			u16	y1;
> +			u16	x2;
> +			u16	y2;
> +			u16	width;
> +			u16	height;
> +		} copy_area __attribute__ ((packed));
> +		struct {
> +			u8	rop;
> +			u16	x;
> +			u16	y;
> +			u16	width;
> +			u16	height;
> +			u32	color;
> +		} fill __attribute__ ((packed));
> +		struct {
> +			u64	offset;
> +			u64	count;
> +		} write __attribute__ ((packed));
> +		u8		pad[23];
> +	};
> +
> +	union {
> +		/* We remember the data pointer so we we can easily free
> +		   everything later by only knowing this structure */
> +		char		*send_buf;

I thought this structure is passed between guest and host?
If so you do not want to stick your pointers there,
add_buf has data pointer for this reason.

> +		u64		_pad;

This might create problems with 32 bit userspace on 64 bit kernels:
which bits does the pointer get packed into depends on architecture.
Better to just pass in __u64 and have userspace cast pointer to that
type, IMO.


> +	};
> +} __attribute__ ((packed));


Packed structures generate terrible code on some architectures.  Can you
just pad structures to multiples of 64 bit, or to full union size, instead?

> +
> +enum copy_type {
> +	COPY_KERNEL,
> +	COPY_USER,
> +	COPY_NOCOPY,
> +};
> +
> +#define DEFAULT_WIDTH		800
> +#define DEFAULT_HEIGHT		600
> +#define DEFAULT_DEPTH		32
> +#define DEFAULT_MEM		8
> +
> +#define DEFAULT_FB_LEN (DEFAULT_WIDTH * DEFAULT_HEIGHT * DEFAULT_DEPTH / 8)
> +
> +enum { KPARAM_MEM, KPARAM_WIDTH, KPARAM_HEIGHT, KPARAM_CNT };
> +static int video[KPARAM_CNT] = { DEFAULT_MEM, DEFAULT_WIDTH, DEFAULT_HEIGHT };
> +module_param_array(video, int, NULL, 0);
> +MODULE_PARM_DESC(video,
> +	"Video size in M,width,height in pixels (default \""
> +	str(DEFAULT_MEM) ","
> +	str(DEFAULT_WIDTH) ","
> +	str(DEFAULT_HEIGHT) "\")");
> +
> +static void virtio_fb_output(struct virtqueue *vq);
> +
> +static void *rvmalloc(unsigned long size)

what exactly does this do?

> +{
> +	void *mem;
> +	unsigned long adr;
> +
> +	size = PAGE_ALIGN(size);
> +	mem = vmalloc_32(size);
> +	if (!mem)
> +		return NULL;
> +
> +	memset(mem, 0, size); /* Clear the ram out, no junk to the user */
> +	adr = (unsigned long) mem;
> +	while (size > 0) {
> +		SetPageReserved(vmalloc_to_page((void *)adr));

Where is the bit cleared?

> +		adr += PAGE_SIZE;
> +		size -= PAGE_SIZE;
> +	}
> +
> +	return mem;
> +}
> +
> +/* This is videobuf_vmalloc_to_sg() from videobuf-dma-sg.c
> +   I modified it to take an extra sg entry for the cmd and work with non
> +   page-aligned pointers though */
> +static struct scatterlist* vmalloc_to_sg(unsigned char *virt, int length,
> +					 void *cmd, int cmd_len, int *sg_elem)
> +{
> +	struct scatterlist *sglist;
> +	struct page *pg;
> +	int nr_pages = (length+PAGE_SIZE-1)/PAGE_SIZE;

Spaces are needed around +, -

> +	int sg_entries;
> +	int i;
> +
> +	/* unaligned */
> +	if ((ulong)virt & ~PAGE_MASK) {

Use offset_in_page here and below?

> +		int tmp_len = length - (PAGE_SIZE - ((ulong)virt & ~PAGE_MASK));
> +		/* how long would it be without the first non-aligned chunk? */
> +		nr_pages = (tmp_len+PAGE_SIZE-1)/PAGE_SIZE;

Spaces are needed around +, -
Also, is this just PAGE_ALIGN?

> +		/* add the first chunk */
> +		nr_pages++;
> +	}

Is all the above just
	nr_pages = PAGE_ALIGN(length + offset_in_page(virt)) / PAGE_SIZE
?
if so, no need for if statements etc.

> +
> +	sg_entries = nr_pages + 1;
> +
> +	sglist = kcalloc(sg_entries, sizeof(struct scatterlist), GFP_KERNEL);
> +	if (!sglist)
> +		return NULL;
> +	sg_init_table(sglist, sg_entries);
> +
> +	/* Put cmd element in */
> +	sg_set_buf(&sglist[0], cmd, cmd_len);
> +
> +	/* Fill with elements for the data */
> +	for (i = 1; i < sg_entries; i++) {
> +		pg = vmalloc_to_page(virt);
> +		if (!pg)
> +			goto err;
> +
> +		if ((ulong)virt & ~PAGE_MASK) {
> +			int tmp_off = ((ulong)virt & ~PAGE_MASK);
> +
> +			sg_set_page(&sglist[i], pg, PAGE_SIZE - tmp_off, tmp_off);
> +			virt = (char*)((ulong)virt & PAGE_MASK);
> +		} else {
> +			sg_set_page(&sglist[i], pg, PAGE_SIZE, 0);
> +		}

You don't need an if statement, do you?
For aligned addresses tmp_off is 0, so it works out.

> +		virt += PAGE_SIZE;
> +	}
> +
> +	*sg_elem = sg_entries;
> +	return sglist;
> +
> + err:
> +	kfree(sglist);
> +	return NULL;
> +}
> +
> +
> +static void _virtio_fb_send(struct virtio_fb_info *info,

void? Don't we care that this might fail e.g. on OOM?

> +			    struct virtio_fb_cmd *cmd,
> +			    char *buf, int len, enum copy_type copy)
> +{
> +	char *send_buf = NULL;
> +	char *sg_buf = NULL;
> +	struct virtio_fb_cmd *send_cmd;
> +	struct scatterlist *sg;
> +	int len_cmd = sizeof(struct virtio_fb_cmd);

sizeof *cmd would be 

> +	int r, sg_elem;
> +
> +	send_cmd = kmem_cache_alloc(info->cmd_cache, GFP_KERNEL);
> +	if (!send_cmd) {
> +		printk(KERN_ERR "virtio-fb: couldn't allocate cmd\n");
> +		return;
> +	}
> +
> +	memcpy(send_cmd, cmd, len_cmd);
> +
> +	if (len) {
> +		sg_buf = send_buf = vmalloc(len);
> +		if (!send_buf) {
> +			printk(KERN_ERR "virtio-fb: couldn't allocate %d b\n",
> +					len);
> +			return;
> +		}
> +
> +		switch (copy) {
> +		case COPY_KERNEL:
> +			memcpy(send_buf, buf, len);
> +			break;
> +		case COPY_USER:
> +			r = copy_from_user(send_buf, (const __user char*)buf,
> +					   len);
> +			break;
> +		case COPY_NOCOPY:
> +			sg_buf = buf;

vmalloc is not really needed in this case, is it?

> +			break;
> +		}
> +	}
> +
> +	send_cmd->send_buf = send_buf;
> +
> +	sg = vmalloc_to_sg(sg_buf, len, send_cmd, len_cmd, &sg_elem);
> +	if (!sg) {
> +		printk(KERN_ERR "virtio-fb: couldn't gather scatter list\n");
> +		return;
> +	}
> +
> +add_again:
> +	r = info->vq_out->vq_ops->add_buf(info->vq_out, sg, sg_elem, 0, send_cmd);


This seems to be done without any locks.
Just making sure: what guarantees that multiple CPUs do
not do this in parallel? Note that virtio do no
locking internally, it's always up to the user.

> +	info->vq_out->vq_ops->kick(info->vq_out);
> +
> +	if ( r == -ENOSPC ) {

what about other errors?

> +		/* Queue was full, so try again */
> +		cpu_relax();
> +		virtio_fb_output(info->vq_out);
> +		goto add_again;

That's pretty evil. Is it possible to block for interrupt
until vq becomes empty, use a timer instead, or
avoid posting more than VQ size in some other way?

> +	}

Can this use for or while loop?

> +
> +	kfree(sg);
> +}
> +
> +static void virtio_fb_send_user(struct virtio_fb_info *info,
> +				struct virtio_fb_cmd *cmd,
> +				const __user char *buf, int len)
> +{
> +	_virtio_fb_send(info, cmd, (char *)buf, len, COPY_USER);


Casting __user pointer away in this way gives up type
safety. Maybe it would be a good idea to split the copy part
away from _virtio_fb_send? Then _nocopy won't do any vmalloc,
this function will do vmalloc+copy_from_user, and virtio_fb_send
below will do vmalloc + memcpy.

> +}
> +
> +static void virtio_fb_send_nocopy(struct virtio_fb_info *info,
> +				  struct virtio_fb_cmd *cmd,
> +				  char *buf, int len)
> +{
> +	_virtio_fb_send(info, cmd, buf, len, COPY_NOCOPY);
> +}
> +
> +static void virtio_fb_send(struct virtio_fb_info *info, struct virtio_fb_cmd *cmd,
> +			   char *buf, int len)
> +{
> +	_virtio_fb_send(info, cmd, buf, len, COPY_KERNEL);
> +}
> +
> +

2 empty lines

> +static int virtio_fb_setcolreg(unsigned regno, unsigned red, unsigned green,
> +			   unsigned blue, unsigned transp,
> +			   struct fb_info *info)
> +{
> +	u32 v;
> +	if (regno >= 16)
> +		return 1;
> +
> +#define CNVT_TOHW(val,width) ((((val)<<(width))+0x7FFF-(val))>>16)

spaces are needed around <<, >>, -, +

> +	red = CNVT_TOHW(red, info->var.red.length);
> +	green = CNVT_TOHW(green, info->var.green.length);
> +	blue = CNVT_TOHW(blue, info->var.blue.length);
> +	transp = CNVT_TOHW(transp, info->var.transp.length);
> +#undef CNVT_TOHW

this is what xen does as well. Any chance to use symbolic
constants above?
Also, are var.XXXX.length values in fact constant?

> +
> +

extra empty line

> +	v = (red << info->var.red.offset) |
> +	    (green << info->var.green.offset) |
> +	    (blue << info->var.blue.offset) |
> +	    (transp << info->var.transp.offset);
> +
> +	((u32 *) (info->pseudo_palette))[regno] = v;

space between } and ( above is just confusing.

> +
> +	return 0;
> +}
> +
> +static void virtio_fb_fillrect(struct fb_info *p, const struct fb_fillrect *rect)
> +{
> +	struct virtio_fb_info *info = p->par;
> +	struct virtio_fb_cmd cmd;
> +
> +	sys_fillrect(p, rect);
> +
> +	cmd.cmd = VIRTIO_FB_CMD_FILL;
> +	cmd.fill.rop = rect->rop;
> +	cmd.fill.x = rect->dx;
> +	cmd.fill.y = rect->dy;
> +	cmd.fill.width = rect->width;
> +	cmd.fill.height = rect->height;
> +	cmd.fill.color = rect->color;
> +
> +	virtio_fb_send(info, &cmd, NULL, 0);
> +}
> +
> +static void virtio_fb_imageblit(struct fb_info *p, const struct fb_image *image)
> +{
> +	struct virtio_fb_info *info = p->par;
> +	struct virtio_fb_cmd cmd;

So this puts a large structure n stack, only to memcpy
it later into a kmalloced one. I think it would be
better just to allocate command from cache directly, for
kernel users. Same comment would apply to others below.

> +	char *_buf;
> +	char *buf;
> +	char *vfb;
> +	int i, w;
> +	int bpp = p->var.bits_per_pixel / 8;
> +	int len = image->width * image->height * bpp;
> +
> +	sys_imageblit(p, image);
> +
> +	cmd.cmd = VIRTIO_FB_CMD_BLIT;
> +	cmd.blit.x = image->dx;
> +	cmd.blit.y = image->dy;
> +	cmd.blit.height = image->height;
> +	cmd.blit.width = image->width;
> +
> +	if (image->depth == 32) {
> +		/* Send the image off directly */
> +
> +		virtio_fb_send(info, &cmd, (char*)image->data, len);

You cast constness away? that's usually not a good thing to do.

> +		return;
> +	}
> +
> +	/* Send the 32-bit translated image */
> +
> +	buf = _buf = vmalloc(len);
> +
> +	vfb = info->fb;
> +	vfb += (image->dy * p->fix.line_length) + image->dx * bpp;

() not needed around *.

> +
> +	w = image->width * bpp;
> +
> +	for (i = 0; i < image->height; i++) {
> +		memcpy(buf, vfb, w);
> +
> +		buf += w;
> +		vfb += p->fix.line_length;
> +	}
> +
> +	virtio_fb_send(info, &cmd, _buf, len);
> +
> +	vfree(_buf);

This would benefit from nocopy, right?
Just another arument why what I propose above is a good idea.

> +}
> +
> +static void virtio_fb_copyarea(struct fb_info *p, const struct fb_copyarea *area)
> +{
> +	struct virtio_fb_info *info = p->par;
> +	struct virtio_fb_cmd cmd;
> +
> +	sys_copyarea(p, area);
> +
> +	cmd.cmd = VIRTIO_FB_CMD_COPY;
> +	cmd.copy_area.x1 = area->sx;
> +	cmd.copy_area.y1 = area->sy;
> +	cmd.copy_area.x2 = area->dx;
> +	cmd.copy_area.y2 = area->dy;
> +	cmd.copy_area.width = area->width;
> +	cmd.copy_area.height = area->height;
> +
> +	virtio_fb_send(info, &cmd, NULL, 0);
> +}
> +
> +static ssize_t virtio_fb_write(struct fb_info *p, const char __user *buf,
> +			size_t count, loff_t *ppos)
> +{
> +	struct virtio_fb_info *info = p->par;
> +	struct virtio_fb_cmd cmd;
> +	loff_t pos = *ppos;
> +	ssize_t res;
> +
> +	res = fb_sys_write(p, buf, count, ppos);
> +
> +	cmd.cmd = VIRTIO_FB_CMD_WRITE;
> +	cmd.write.offset = pos;
> +	cmd.write.count = count;
> +
> +	virtio_fb_send_user(info, &cmd, buf, count);
> +	return res;
> +}
> +
> +static int
> +virtio_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *p)
> +{
> +	struct virtio_fb_info *info = p->par;
> +
> +	if (var->bits_per_pixel != DEFAULT_DEPTH) {
> +		/* We only support 32 bpp */
> +		return -EINVAL;
> +	}
> +
> +	if ((var->xres * var->yres * DEFAULT_DEPTH / 8) > info->size) {

don't need () around math

> +		/* Doesn't fit in the frame buffer */
> +		return -EINVAL;
> +	}
> +
> +	var->xres_virtual = var->xres;
> +	var->yres_virtual = var->yres;
> +
> +	return 0;
> +}
> +
> +static int virtio_fb_set_par(struct fb_info *p)
> +{
> +	struct virtio_fb_info *info = p->par;
> +	struct virtio_fb_cmd cmd;
> +
> +	/* Do nothing if we're on that resolution already */
> +	if ((p->var.xres == info->width) &&
> +	    (p->var.yres == info->height))

don't need () around ===

> +		return 0;
> +
> +	info->width = p->var.xres;
> +	info->height = p->var.yres;
> +	p->fix.line_length = p->var.xres_virtual * 4;
> +
> +	/* Notify hypervisor */
> +
> +	cmd.cmd = VIRTIO_FB_CMD_RESIZE;
> +	cmd.resize.width = p->var.xres;
> +	cmd.resize.height = p->var.yres;
> +
> +	virtio_fb_send(info, &cmd, NULL, 0);
> +
> +	return 0;
> +}
> +
> +static struct fb_ops virtio_fb_ops = {
> +	.owner		= THIS_MODULE,
> +	.fb_read	= fb_sys_read,
> +	.fb_write	= virtio_fb_write,
> +	.fb_setcolreg	= virtio_fb_setcolreg,
> +	.fb_fillrect	= virtio_fb_fillrect,
> +	.fb_copyarea	= virtio_fb_copyarea,
> +	.fb_imageblit	= virtio_fb_imageblit,
> +	.fb_check_var	= virtio_fb_check_var,
> +	.fb_set_par     = virtio_fb_set_par,
> +};
> +
> +static __devinit void
> +virtio_fb_make_preferred_console(void)
> +{
> +	struct console *c;
> +
> +	if (console_set_on_cmdline)
> +		return;
> +
> +	acquire_console_sem();
> +	for (c = console_drivers; c; c = c->next) {
> +		if (!strcmp(c->name, "tty") && c->index == 0)
> +			break;
> +	}

{} not needed

> +	release_console_sem();

Just a thought: can console c go away at this point?

> +	if (c) {
> +		unregister_console(c);
> +		c->flags |= CON_CONSDEV;
> +		c->flags &= ~CON_PRINTBUFFER; /* don't print again */
> +		register_console(c);
> +	}
> +}
> +
> +static void virtio_fb_deferred_io(struct fb_info *fb_info,
> +				  struct list_head *pagelist)
> +{
> +	struct virtio_fb_info *info = fb_info->par;
> +	struct page *page;
> +	unsigned long beg, end;
> +	int y1, y2, miny, maxy;
> +	struct virtio_fb_cmd cmd;
> +
> +	miny = INT_MAX;
> +	maxy = 0;
> +	list_for_each_entry(page, pagelist, lru) {
> +		beg = page->index << PAGE_SHIFT;

page_index()?

> +		end = beg + PAGE_SIZE - 1;
> +		y1 = beg / fb_info->fix.line_length;

You do all these divisions in a cycle, then end up multiplying
the result by line_length. Maybe do the math in bytes.

> +		y2 = end / fb_info->fix.line_length;

Is the result guaranteed to fit in int?

> +		if (y2 >= fb_info->var.yres)
> +			y2 = fb_info->var.yres - 1;
> +		if (miny > y1)
> +			miny = y1;
> +		if (maxy < y2)
> +			maxy = y2;

use min()/max() macros above.

> +	}
> +
> +	if (miny != INT_MAX) {

if (miny == INT_MAX)
	return;

will be neater

> +		cmd.cmd = VIRTIO_FB_CMD_WRITE;
> +		cmd.write.offset = miny * fb_info->fix.line_length;
> +		cmd.write.count = (maxy - miny + 1) * fb_info->fix.line_length;
> +
> +		virtio_fb_send_nocopy(info, &cmd, info->fb + cmd.write.offset,
> +				      cmd.write.count);
> +	}
> +}
> +
> +static struct fb_deferred_io virtio_fb_defio = {
> +	.delay		= HZ / 20,
> +	.deferred_io	= virtio_fb_deferred_io,
> +};
> +
> +/* Callback when the host kicks our input queue.
> + *
> + * This is to enable notifications from host to guest. */
> +static void virtio_fb_input(struct virtqueue *vq)
> +{
> +	struct virtio_fb_info *info = dev_get_drvdata(&vq->vdev->dev);
> +	int len, i;
> +	void *x;
> +	void *reinject[3];
> +	int reinject_count = 0;
> +
> +	while ((x = vq->vq_ops->get_buf(vq, &len)) != NULL)

&& reinject_count < 3?

> {

This looks unsafe: can this get triggered while another
routine does add_buf? If yes you must add locking to
prevent this.

> +		struct virtio_fb_cmd *cmd = x;
> +
> +		/* Make sure we're getting an inbuf page! */

What does this check, exactly? 

> +		BUG_ON((x != info->inbuf) &&
> +		       (x != (info->inbuf + PAGE_SIZE)) &&
> +		       (x != (info->inbuf + (PAGE_SIZE * 2))));
() not needed around math.

> +
> +		switch (cmd->cmd) {
> +		case VIRTIO_FB_CMD_REFRESH:
> +			schedule_work(&info->refresh_work);
> +			break;
> +		}
> +
> +		reinject[reinject_count++] = x;

This will overflow on a buggy host.  Better check and BUG(); Also, the
number of buffers is VQ size?
Make it a constant then.

> +	}
> +
> +
> +	for (i = 0; i < reinject_count; i++) {
> +		struct scatterlist sg;
> +		void *x = reinject[i];
> +
> +		sg_init_one(&sg, x, PAGE_SIZE);
> +		vq->vq_ops->add_buf(vq, &sg, 0, 1, x);
> +		vq->vq_ops->kick(vq);

won't it be easier to reinject wach buffer as you get it directly?

> +	}
> +}
> +
> +/* Asynchronous snippet to send all screen contents to the host */
> +static void deferred_refresh(struct work_struct *work)
> +{
> +	struct virtio_fb_info *info = container_of(work, struct virtio_fb_info,
> +						   refresh_work);
> +	struct virtio_fb_cmd cmd;
> +
> +	cmd.cmd = VIRTIO_FB_CMD_WRITE;
> +	cmd.write.offset = 0;
> +	cmd.write.count = info->width * info->height * 4;

why 4?

> +
> +	virtio_fb_send_nocopy(info, &cmd, info->fb, cmd.write.count);
> +}
> +
> +/* Asynchronous garbage collector :-) */
> +static void deferred_vfree(struct work_struct *work)
> +{
> +	struct virtio_fb_info *info = container_of(work, struct virtio_fb_info,
> +						   vfree_work);
> +	int i;
> +	unsigned long flags;
> +	void *last_buf[MAX_BUF];

Wow, that's a lot of stack.  Can't vfree be called on info->last_buf
directly?

> +	int idx;
> +
> +	spin_lock_irqsave(&info->vfree_lock, flags);


spin_lock_irq is enough

> +
> +	idx = info->last_buf_idx;
> +	memcpy(last_buf, info->last_buf, sizeof(void*) * MAX_BUF);

sizeof last_buf

> +	info->last_buf_idx = 0;
> +
> +	spin_unlock_irqrestore(&info->vfree_lock, flags);
> +
> +	for (i = 0; i < idx; i++) {
> +		vfree(last_buf[i]);
> +	}

{} not needed

> +}
> +
> +/* Callback when the host kicks our output queue. This can only mean it's done
> + * processing an item, so let's free up the memory occupied by the entries */


lines too long here and elsewhere

> +static void virtio_fb_output(struct virtqueue *vq)
> +{
> +	struct virtio_fb_info *info = dev_get_drvdata(&vq->vdev->dev);
> +	int len;
> +	void *x;
> +
> +	while ((x = vq->vq_ops->get_buf(vq, &len)) != NULL) {

!= NULL not needed.
Again, get_buf must be called under some
lock that prevents add_buf from being called.

> +		struct virtio_fb_cmd *cmd = x;
> +
> +		if (cmd->send_buf) {
> +			spin_lock(&info->vfree_lock);
> +			if (info->last_buf_idx != MAX_BUF) {
> +				info->last_buf[info->last_buf_idx++] =
> +					cmd->send_buf;
> +			}

{} not needed

> +			spin_unlock(&info->vfree_lock);
> +
> +			schedule_work(&info->vfree_work);

So this would be a good place to re-enable more output
instead of busy wait above? Also, can't we vfree
here directly?

> +		}
> +
> +		kmem_cache_free(info->cmd_cache, x);
> +	}
> +}
> +
> +static int __devinit virtio_fb_probe(struct virtio_device *dev)
> +{
> +	vq_callback_t *callbacks[] = { virtio_fb_input, virtio_fb_output };
> +	const char *names[] = { "input", "output" };
> +	struct virtio_fb_info *info;
> +	struct virtqueue *vqs[2];
> +	struct fb_info *fb_info = NULL;
> +	int fb_size, res_size;
> +	int ret, err, i;
> +	char *inbuf;
> +
> +	err = dev->config->find_vqs(dev, 2, vqs, callbacks, names);

2 -> ARRAY_SIZE(vqs).

> +	if (err) {
> +		printk(KERN_ERR "VirtIO FB: couldn't find VQs\n");
> +		return err;
> +	}

You probably want del_vqs on error as well?

> +
> +	info = kmalloc(sizeof(*info), GFP_KERNEL);

this seems to be leaked on errors?
() not needed around *info.

> +	if (info == NULL)

!info

> +		return -ENOMEM;
> +
> +	info->vq_in = vqs[0];
> +	info->vq_out = vqs[1];
> +
> +	res_size = video[KPARAM_WIDTH] * video[KPARAM_HEIGHT] * DEFAULT_DEPTH / 8;
> +	fb_size = video[KPARAM_MEM] * 1024 * 1024;
> +
> +	if (res_size > fb_size) {
> +		video[KPARAM_WIDTH] = DEFAULT_WIDTH;
> +		video[KPARAM_HEIGHT] = DEFAULT_HEIGHT;
> +	}
> +
> +	dev_set_drvdata(&dev->dev, info);
> +	info->vdev = dev;
> +
> +	info->fb = rvmalloc(fb_size);
> +	if (info->fb == NULL)

!info->fb

> +		goto error_nomem;
> +
> +	info->nr_pages = (fb_size + PAGE_SIZE - 1) >> PAGE_SHIFT;

PAGE_ALIGN?

> +	info->size = fb_size;
> +
> +	fb_info = framebuffer_alloc(sizeof(u32) * 256, NULL);

make 256 symbolic constant? it's used below as well.

> +	if (fb_info == NULL)
> +		goto error_nomem;
> +
> +	inbuf = kmalloc(PAGE_SIZE * 3, GFP_KERNEL);

replace 3 * PAGE_SIZE with symbolic constant?
Since this seems to be part of guest/host interface,
maybe even put it in header?

> +	info->inbuf = inbuf;
> +	for (i = 0; i < (3 * PAGE_SIZE); i += PAGE_SIZE) {

() not needed around math

> +		struct scatterlist sg;
> +
> +		sg_init_one(&sg, inbuf + i, PAGE_SIZE);
> +		info->vq_in->vq_ops->add_buf(info->vq_in, &sg, 0, 1, inbuf + i);

add_buf can fail.

> +	}
> +	info->vq_in->vq_ops->kick(info->vq_in);
> +
> +	fb_info->pseudo_palette = fb_info->par;
> +	fb_info->par = info;
> +
> +	fb_info->screen_base = info->fb;
> +
> +	fb_info->fbops = &virtio_fb_ops;
> +	fb_info->var.xres_virtual = fb_info->var.xres = video[KPARAM_WIDTH];
> +	fb_info->var.yres_virtual = fb_info->var.yres = video[KPARAM_HEIGHT];
> +	fb_info->var.bits_per_pixel = DEFAULT_DEPTH;
> +
> +	fb_info->var.transp = (struct fb_bitfield){24, 8, 0};
> +	fb_info->var.red    = (struct fb_bitfield){16, 8, 0};
> +	fb_info->var.green  = (struct fb_bitfield){8,  8, 0};
> +	fb_info->var.blue   = (struct fb_bitfield){0,  8, 0};
> +
> +	fb_info->var.activate = FB_ACTIVATE_NOW;
> +	fb_info->var.height = -1;
> +	fb_info->var.width = -1;
> +	fb_info->var.vmode = FB_VMODE_NONINTERLACED;
> +
> +	fb_info->fix.visual = FB_VISUAL_TRUECOLOR;
> +	fb_info->fix.line_length = fb_info->var.xres * DEFAULT_DEPTH / 8;
> +	fb_info->fix.smem_start = 0;
> +	fb_info->fix.smem_len = fb_size;
> +	strcpy(fb_info->fix.id, "virtio_fb");
> +	fb_info->fix.type = FB_TYPE_PACKED_PIXELS;
> +	fb_info->fix.accel = FB_ACCEL_NONE;
> +
> +	fb_info->flags = FBINFO_FLAG_DEFAULT | FBINFO_HWACCEL_COPYAREA |
> +			 FBINFO_READS_FAST;
> +
> +	ret = fb_alloc_cmap(&fb_info->cmap, 256, 0);
> +	if (ret < 0) {
> +		framebuffer_release(fb_info);
> +		goto error;
> +	}
> +
> +	info->cmd_cache = kmem_cache_create("virtio_fb_cmd",
> +					    sizeof(struct virtio_fb_cmd),
> +					    0, 0, NULL);

this allocation leaks on error?

> +

empty line above more confusing than helpful.

> +	if (!info->cmd_cache) {
> +		framebuffer_release(fb_info);
> +		goto error;
> +	}
> +
> +	fb_info->fbdefio = &virtio_fb_defio;
> +	fb_deferred_io_init(fb_info);
> +
> +	INIT_WORK(&info->refresh_work, deferred_refresh);
> +	INIT_WORK(&info->vfree_work, deferred_vfree);
> +	spin_lock_init(&info->vfree_lock);
> +
> +	ret = register_framebuffer(fb_info);
> +	if (ret) {
> +		fb_deferred_io_cleanup(fb_info);
> +		fb_dealloc_cmap(&fb_info->cmap);
> +		framebuffer_release(fb_info);
> +		goto error;

will be less code with a couple more labels to goto on error?

> +	}
> +	info->fb_info = fb_info;
> +
> +	virtio_fb_make_preferred_console();
> +	return 0;
> +
> + error_nomem:
> +	ret = -ENOMEM;
> + error:
> +	framebuffer_release(fb_info);
> +	return ret;
> +}
> +
> +static void virtio_fb_apply_config(struct virtio_device *dev)
> +{
> +}
> +
> +static struct virtio_device_id id_table[] = {
> +	{ VIRTIO_ID_FB, VIRTIO_DEV_ANY_ID },
> +	{ 0 },

0 not needed here.

> +};
> +
> +static unsigned int features[] = {
> +};
> +
> +static struct virtio_driver virtio_console = {
> +	.feature_table = features,
> +	.feature_table_size = ARRAY_SIZE(features),

I think you should just set to NULL and 0 here, and remove the empty
features array.

> +	.driver.name =  KBUILD_MODNAME,
> +	.driver.owner = THIS_MODULE,
> +	.id_table =     id_table,
> +	.probe =        virtio_fb_probe,
> +	.config_changed = virtio_fb_apply_config,

This alignment looks strange. right-align = or just avoid code
alignment.

> +};
> +
> +static int __init init(void)
> +{
> +	return register_virtio_driver(&virtio_console);
> +}
> +module_init(init);

I don't know much about framebuffer. Why do all fb modules
lack module_exit?

> +
> +

extra empty line

> +MODULE_DEVICE_TABLE(virtio, id_table);
> +MODULE_DESCRIPTION("Virtio framebuffer driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
> index 06660c0..72e39f7 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_FB		6 /* virtio framebuffer */
>  #define VIRTIO_ID_9P		9 /* 9p virtio console */
>  
>  #endif /* _LINUX_VIRTIO_IDS_H */
> -- 
> 1.6.0.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html