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 at suse.de>
Nice work. I think you want to Cc
virtualization at 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 at 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 at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
