|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel][RFC]Dynamic modes support for PV xenfb (included)
Pat Campbell <plc@xxxxxxxxxx> writes:
> Rehash following last round of comments
>
> What changed:
> 1. Reverted back to pd[] for mappings, removed all gntref code.
> 2. Added videoram vfb parameter so that admin can limit what a frontend
> can allocate
> 3. xenfb kernel parameters are used to set the built-in width and
> height to support
> configure less X11.
> extra="xenfb.video=5,1024,768 "
>
> Unless I missed something I think this addresses all of the raised concerns.
Thanks!
> Thanks to all that have looked at this never ending RFC and commented.
PVFB took several interations to pass review into xen-unstable. Took
us roughly five months. I'm fairly confident that this won't drag on
for nearly as long.
> Pat
>
>
> diff -r e32fe4703ab6 drivers/xen/fbfront/xenfb.c
> --- a/drivers/xen/fbfront/xenfb.c Tue Jan 29 11:53:33 2008 +0000
> +++ b/drivers/xen/fbfront/xenfb.c Mon Feb 04 11:34:36 2008 -0700
> @@ -28,6 +28,7 @@
> #include <asm/hypervisor.h>
> #include <xen/evtchn.h>
> #include <xen/interface/io/fbif.h>
> +#include <xen/interface/io/kbdif.h>
> #include <xen/interface/io/protocols.h>
> #include <xen/xenbus.h>
> #include <linux/kthread.h>
> @@ -62,6 +63,13 @@ struct xenfb_info
> struct xenfb_page *page;
> unsigned long *mfns;
> int update_wanted; /* XENFB_TYPE_UPDATE wanted */
> + int feature_resize; /* Backend has resize feature */
> + int resize_dpy;
> + int xres, yres; /* current resolution */
Aren't these redundant with fb_info->var.xres and .yres? Whenever the
same thing is stored in multiple places, a little dwarf in the back of
my brain starts worrying about consistency, which I find kind of
distracting...
> + int fb_size; /* fb size in bytes */
Redundant with fb_info->fix.smem_len?
> + int fb_width; /* fb mem width in pixels */
> + int fb_height; /* fb mem height in pixels */
struct fb_var_screeninfo uses width and height for screen dimension in
mm. Hmm. Perhaps use max_xres and max_yres here?
But do we really need these? The only use outside of xenfb_probe() is
in xenfb_check_var(), which could easily use fb_stride instead.
> + int fb_stride; /* fb stride in bytes */
Redundant with fb_info->fix.line_length?
>
> struct xenbus_device *xbdev;
> };
> @@ -129,20 +137,48 @@ struct xenfb_info
> *
> * Oh well, we wont be updating the writes to this page anytime soon.
> */
> +enum {KPARAM_MEM, KPARAM_WIDTH, KPARAM_HEIGHT, KPARAM_CNT};
> +static int video[KPARAM_CNT] = {0};
> +static int video_cnt = 0;
> +module_param_array(video, int, &video_cnt, 0);
> +MODULE_PARM_DESC(video,
> + "Size of video memory in MB and width,height in pixels, default
> = (2,800,600)");
As far as I can see, mem never changes. width and height can change,
but never beyond the initial width. Correct?
Perhaps somebody might want to specify a maximum width different from
the initial width. I don't know.
> +
> +#define XENFB_WIDTH 800
> +#define XENFB_HEIGHT 600
> +#define XENFB_DEPTH 32
> +#define MB_ (1024*1024)
> +#define XENFB_PIXCLOCK 9025 /* Xorg "1280x1024" 110.80 to FB
> 1000000/110.80 */
I appreciate the attempt to explain how you arrived at the pixel
clock, but it's still confusing. And possibly wrong: the pixel clock
seems to be for 1280x1024, while the resolution is 800x600.
> +#define XENFB_DEFAULT_FB_LEN (XENFB_WIDTH * XENFB_HEIGHT * XENFB_DEPTH / 8)
>
> static int xenfb_fps = 20;
> -static unsigned long xenfb_mem_len = XENFB_WIDTH * XENFB_HEIGHT *
> XENFB_DEPTH / 8;
> +static unsigned long xenfb_mem_len = 0;
Is this variable still needed? I suspect all its uses can easily be
replaced by struct xenfb's fb_size.
>
> static int xenfb_remove(struct xenbus_device *);
> static void xenfb_init_shared_page(struct xenfb_info *);
> static int xenfb_connect_backend(struct xenbus_device *, struct xenfb_info
> *);
> static void xenfb_disconnect_backend(struct xenfb_info *);
> +static void xenfb_refresh(struct xenfb_info *info, int x1, int y1, int w,
> int h);
Style nitpick: long line. The parameter names don't buy us much in a
forward declaration.
> +
> +static void xenfb_send_event(struct xenfb_info *info,
> + union xenfb_out_event *event)
> +{
> + __u32 prod;
> +
> + prod = info->page->out_prod;
> + /* caller ensures !xenfb_queue_full() */
> + mb(); /* ensure ring space available */
> + XENFB_OUT_RING_REF(info->page, prod) = *event;
> + wmb(); /* ensure ring contents visible */
> + info->page->out_prod = prod + 1;
> +
> + notify_remote_via_irq(info->irq);
> +}
>
> static void xenfb_do_update(struct xenfb_info *info,
> int x, int y, int w, int h)
> {
> union xenfb_out_event event;
> - __u32 prod;
>
> event.type = XENFB_TYPE_UPDATE;
> event.update.x = x;
> @@ -150,14 +186,19 @@ static void xenfb_do_update(struct xenfb
> event.update.width = w;
> event.update.height = h;
>
> - prod = info->page->out_prod;
> - /* caller ensures !xenfb_queue_full() */
I'd keep this comment here, and also copy it to xenfb_do_resize().
> - mb(); /* ensure ring space available */
> - XENFB_OUT_RING_REF(info->page, prod) = event;
> - wmb(); /* ensure ring contents visible */
> - info->page->out_prod = prod + 1;
> -
> - notify_remote_via_irq(info->irq);
> + xenfb_send_event(info, &event);
> +}
> +
> +static void xenfb_do_resize(struct xenfb_info *info)
> +{
> + union xenfb_out_event event;
> +
> + event.type = XENFB_TYPE_RESIZE;
> + event.resize.width = info->xres;
> + event.resize.height = info->yres;
> + event.resize.stride = info->fb_stride;
I'm not sure the stride changes on resize (see below), but it's
probably a good idea to put it in the protocol.
I think you better set event.resize.depth = XENFB_DEPTH here.
> +
> + xenfb_send_event(info, &event);
> }
>
> static int xenfb_queue_full(struct xenfb_info *info)
> @@ -209,6 +250,16 @@ static void xenfb_update_screen(struct x
> xenfb_do_update(info, x1, y1, x2 - x1, y2 - y1);
> }
>
> +static void xenfb_resize_screen(struct xenfb_info *info)
> +{
> + if (xenfb_queue_full(info))
> + return;
> +
> + info->resize_dpy = 0;
> + xenfb_do_resize(info);
> + xenfb_refresh(info, 0, 0, info->xres, info->yres);
Hmm, ugly. See next comment.
> +}
> +
> static int xenfb_thread(void *data)
> {
> struct xenfb_info *info = data;
> @@ -217,6 +268,9 @@ static int xenfb_thread(void *data)
> if (info->dirty) {
> info->dirty = 0;
> xenfb_update_screen(info);
> + }
> + if (info->resize_dpy) {
> + xenfb_resize_screen(info);
> }
Both screen resizes and dirtying don't go to the backend immediately.
Instead, they accumulate in struct xenfb_info (dirty rectangle and
resize_dpy flag) until they get pushed by xenfb_thread().
The way things work, a resize triggers three events:
1. The update event for the dirty rectangle. In theory, the dirty
rectangle could be clean, but I expect it to be quite dirty in
practice, as user space typically redraws everything right after a
resize.
2. The resize event.
3. Another update event, because xenfb_resize_screen() dirties the
whole screen. This event is delayed until the next iteration of
xenfb_thread()'s loop.
I'd rather do it like this: decree that resize events count as whole
screen updates. Make xenfb_resize_screen() clear the dirty rectangle
(optional, saves an update event). Test resize_dpy before dirty.
Also: you access resize_dpy, xres, yres and fb_stride from multiple
threads without synchronization. I fear that's not safe. Don't you
have to protect them with a spinlock, like dirty_lock protects the
dirty rectangle? Could reuse dirty_lock, I guess.
> wait_event_interruptible(info->wq,
> kthread_should_stop() || info->dirty);
> @@ -413,6 +467,47 @@ static int xenfb_mmap(struct fb_info *fb
> return 0;
> }
>
> +static int
> +xenfb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
> +{
> + struct xenfb_info *xenfb_info;
> +
> + xenfb_info = info->par;
> +
> + if (!xenfb_info->feature_resize) {
> + if (var->xres == XENFB_WIDTH && var->yres == XENFB_HEIGHT
> + && var->bits_per_pixel ==
> xenfb_info->page->depth) {
Avoidable long line due to excessive indentation.
> + return 0;
> + }
> + return -EINVAL;
> + }
> + if (var->bits_per_pixel == xenfb_info->page->depth &&
> + xenfb_info->fb_width >= var->xres &&
> + xenfb_mem_len >= (var->xres * var->yres *
> (xenfb_info->page->depth / 8))) {
Ditto.
Please put all var-> terms on the left hand side, and all xenfb_ terms
on the right hand side. My poor little mind finds that easier to
understand than mixing left and right.
> + var->xres_virtual = var->xres;
> + var->yres_virtual = var->yres;
Stupid question: what about var->pixclock? Does one clock fit all, or
does somebody else update it, or should we recompute it?
> + return 0;
> + }
> + return -EINVAL;
> +}
> +
> +static int xenfb_set_par(struct fb_info *info)
> +{
> + struct xenfb_info *xenfb_info;
> +
> + xenfb_info = info->par;
> +
> + if (xenfb_info->xres != info->var.xres ||
> + xenfb_info->yres != info->var.yres) {
> + xenfb_info->resize_dpy = 1;
> + xenfb_info->xres = info->var.xres_virtual = info->var.xres;
> + xenfb_info->yres = info->var.yres_virtual = info->var.yres;
> + info->fix.line_length = xenfb_info->fb_stride;
I'm not sure we're supposed to touch info->fix. Is it necessary? I
can't see fb_stride change on resize.
> + xenkbd_set_ptr_geometry(xenfb_info->xres, xenfb_info->yres);
> + }
> + return 0;
> +}
> +
> static struct fb_ops xenfb_fb_ops = {
> .owner = THIS_MODULE,
> .fb_setcolreg = xenfb_setcolreg,
> @@ -420,6 +515,8 @@ static struct fb_ops xenfb_fb_ops = {
> .fb_copyarea = xenfb_copyarea,
> .fb_imageblit = xenfb_imageblit,
> .fb_mmap = xenfb_mmap,
> + .fb_check_var = xenfb_check_var,
> + .fb_set_par = xenfb_set_par,
> };
>
> static irqreturn_t xenfb_event_handler(int rq, void *dev_id,
> @@ -450,6 +547,8 @@ static int __devinit xenfb_probe(struct
> {
> struct xenfb_info *info;
> struct fb_info *fb_info;
> + int fb_size;
> + int val;
> int ret;
>
> info = kzalloc(sizeof(*info), GFP_KERNEL);
> @@ -457,6 +556,32 @@ static int __devinit xenfb_probe(struct
> xenbus_dev_fatal(dev, -ENOMEM, "allocating info structure");
> return -ENOMEM;
> }
All right, what do you do here:
> +
> + info->fb_width = info->xres = XENFB_WIDTH;
> + info->fb_height = info->yres = XENFB_HEIGHT;
> + fb_size = XENFB_DEFAULT_FB_LEN;
Start with defaults.
> +
> + if (xenbus_scanf(XBT_NIL, dev->otherend, "videoram", "%d", &val) == 1) {
If domain config limits videoram...
> + if (val * MB_ >= XENFB_DEFAULT_FB_LEN) {
... and the limit is below the default,
> + video[KPARAM_MEM] = val;
> + fb_size = val * MB_;
then store it in video[KPARAM_MEM].
Note that it will only get used when kernel parameter video supplied
all values (see right below).
> + }
> + }
> +
> + if (video_cnt == KPARAM_CNT && (video[KPARAM_MEM] * MB_) >=
> XENFB_DEFAULT_FB_LEN) {
If kernel parameter video supplied all values, and the memory value is
above the default,
> + fb_size = video[KPARAM_MEM] * MB_;
then use it instead of the default,
else silently ignore the memory value.
> + if (video[KPARAM_WIDTH] * video[KPARAM_HEIGHT] * XENFB_DEPTH/8
> < fb_size) {
If kernel parameter video supplied all values, and the width and
height values fit the memory value we actually use,
> + info->fb_width = info->xres = video[KPARAM_WIDTH];
> + info->fb_height = info->yres = video[KPARAM_HEIGHT];
> + }
then use them instead of the defaults,
else silently ignore them.
> + }
Isn't this too complicated?
Why do you ignore memory sizes below the default (both xenstore and
kernel parameters)? I'd trust the system administrator here. If he
asks for a 320x200 rope to hang himself, just give it to him.
Ignoring kernel parameters silently isn't very nice.
I suspect this would be easier if you initialized video[] with the
defaults. Then reduce memory for xenstore's videoram. Then check
width & height against memory, and reduce until they fit.
> +
> + info->fb_size = fb_size;
> + info->fb_stride = info->fb_width * (XENFB_DEPTH / 8);
> + xenfb_mem_len = info->fb_size;
> +
> + xenkbd_set_ptr_geometry(info->fb_width, info->fb_height);
> +
> dev->dev.driver_data = info;
> info->xbdev = dev;
> info->irq = -1;
> @@ -504,9 +629,10 @@ static int __devinit xenfb_probe(struct
> fb_info->screen_base = info->fb;
>
> fb_info->fbops = &xenfb_fb_ops;
> - fb_info->var.xres_virtual = fb_info->var.xres = info->page->width;
> - fb_info->var.yres_virtual = fb_info->var.yres = info->page->height;
> + fb_info->var.xres_virtual = fb_info->var.xres = info->xres;
> + fb_info->var.yres_virtual = fb_info->var.yres = info->yres;
> fb_info->var.bits_per_pixel = info->page->depth;
> + fb_info->var.pixclock = XENFB_PIXCLOCK;
>
> fb_info->var.red = (struct fb_bitfield){16, 8, 0};
> fb_info->var.green = (struct fb_bitfield){8, 8, 0};
> @@ -572,6 +698,8 @@ static int xenfb_resume(struct xenbus_de
>
> xenfb_disconnect_backend(info);
> xenfb_init_shared_page(info);
> + if (info->xres != XENFB_WIDTH || info->yres != XENFB_HEIGHT)
> + info->resize_dpy = 1;
Still needs a comment :)
> return xenfb_connect_backend(dev, info);
> }
>
> @@ -600,6 +728,7 @@ static void xenfb_init_shared_page(struc
> static void xenfb_init_shared_page(struct xenfb_info *info)
> {
> int i;
> + int epd = PAGE_SIZE/sizeof(info->mfns[0]);
>
> for (i = 0; i < info->nr_pages; i++)
> info->pages[i] = vmalloc_to_page(info->fb + i * PAGE_SIZE);
> @@ -607,12 +736,13 @@ static void xenfb_init_shared_page(struc
> for (i = 0; i < info->nr_pages; i++)
> info->mfns[i] = vmalloc_to_mfn(info->fb + i * PAGE_SIZE);
>
> - info->page->pd[0] = vmalloc_to_mfn(info->mfns);
> - info->page->pd[1] = 0;
> - info->page->width = XENFB_WIDTH;
> - info->page->height = XENFB_HEIGHT;
> + for (i = 0; i * epd < info->nr_pages; i++)
> + info->page->pd[i] = vmalloc_to_mfn(&info->mfns[i *
> epd]);
Indented one tab too much.
> +
> + info->page->width = info->xres;
> + info->page->height = info->yres;
> info->page->depth = XENFB_DEPTH;
> - info->page->line_length = (info->page->depth / 8) * info->page->width;
> + info->page->line_length = info->fb_stride;
> info->page->mem_length = xenfb_mem_len;
> info->page->in_cons = info->page->in_prod = 0;
> info->page->out_cons = info->page->out_prod = 0;
> @@ -710,6 +840,11 @@ static void xenfb_backend_changed(struct
> val = 0;
> if (val)
> info->update_wanted = 1;
> +
> + if (xenbus_scanf(XBT_NIL, dev->otherend,
> + "feature-resize", "%d", &val) < 0)
> + val = 0;
> + info->feature_resize = val;
> break;
>
> case XenbusStateClosing:
> diff -r e32fe4703ab6 drivers/xen/fbfront/xenkbd.c
> --- a/drivers/xen/fbfront/xenkbd.c Tue Jan 29 11:53:33 2008 +0000
> +++ b/drivers/xen/fbfront/xenkbd.c Mon Feb 04 08:41:35 2008 -0700
> @@ -40,6 +40,10 @@ static int xenkbd_remove(struct xenbus_d
> static int xenkbd_remove(struct xenbus_device *);
> static int xenkbd_connect_backend(struct xenbus_device *, struct xenkbd_info
> *);
> static void xenkbd_disconnect_backend(struct xenkbd_info *);
> +
> +static int max_abs_xres;
> +static int max_abs_yres;
> +static struct input_dev *mouse_ptr;
Can't have more than one device. How ugly.
> /*
> * Note: if you need to send out events, see xenfb_do_update() for how
> @@ -163,8 +167,8 @@ int __devinit xenkbd_probe(struct xenbus
> for (i = BTN_LEFT; i <= BTN_TASK; i++)
> set_bit(i, ptr->keybit);
> ptr->relbit[0] = BIT(REL_X) | BIT(REL_Y) | BIT(REL_WHEEL);
> - input_set_abs_params(ptr, ABS_X, 0, XENFB_WIDTH, 0, 0);
> - input_set_abs_params(ptr, ABS_Y, 0, XENFB_HEIGHT, 0, 0);
> + input_set_abs_params(ptr, ABS_X, 0, max_abs_xres, 0, 0);
> + input_set_abs_params(ptr, ABS_Y, 0, max_abs_yres, 0, 0);
>
> ret = input_register_device(ptr);
> if (ret) {
> @@ -172,7 +176,7 @@ int __devinit xenkbd_probe(struct xenbus
> xenbus_dev_fatal(dev, ret, "input_register_device(ptr)");
> goto error;
> }
> - info->ptr = ptr;
> + mouse_ptr = info->ptr = ptr;
>
> ret = xenkbd_connect_backend(dev, info);
> if (ret < 0)
> @@ -338,6 +342,18 @@ static void __exit xenkbd_cleanup(void)
> return xenbus_unregister_driver(&xenkbd);
> }
>
> +void xenkbd_set_ptr_geometry(int xres, int yres)
> +{
> + max_abs_xres = xres;
> + max_abs_yres = yres;
> + if (mouse_ptr) {
Race condition: if this gets executed after xenkbd_probe() read
max_abs_xres, max_abs_yres, but before it set mouse_ptr, the geometry
update gets lost.
I fear you need proper synchronization instead of a hack...
> + input_set_abs_params(mouse_ptr, ABS_X, 0, max_abs_xres, 0, 0);
> + input_set_abs_params(mouse_ptr, ABS_Y, 0, max_abs_yres, 0, 0);
> + input_event(mouse_ptr, EV_SYN, SYN_CONFIG, 0);
> + }
> +}
> +EXPORT_SYMBOL(xenkbd_set_ptr_geometry);
> +
> module_init(xenkbd_init);
> module_exit(xenkbd_cleanup);
>
> diff -r e32fe4703ab6 include/xen/interface/io/fbif.h
> --- a/include/xen/interface/io/fbif.h Tue Jan 29 11:53:33 2008 +0000
> +++ b/include/xen/interface/io/fbif.h Mon Feb 04 08:53:20 2008 -0700
> @@ -50,12 +50,28 @@ struct xenfb_update
> int32_t height; /* rect height */
> };
>
> +/*
> + * Framebuffer resize notification event
> + * Capable backend sets feature-resize in xenstore.
> + */
> +#define XENFB_TYPE_RESIZE 3
> +
> +struct xenfb_resize
> +{
> + uint8_t type; /* XENFB_TYPE_RESIZE */
> + int32_t width; /* width in pixels */
> + int32_t height; /* height in pixels */
> + int32_t stride; /* stride in bytes */
> + int32_t depth; /* future */
"future" isn't so helpful. What about "bits per pixel"? If the
backend can deal with that already. If not, I'd comment "reserved for
future use".
> +};
> +
> #define XENFB_OUT_EVENT_SIZE 40
>
> union xenfb_out_event
> {
> uint8_t type;
> struct xenfb_update update;
> + struct xenfb_resize resize;
> char pad[XENFB_OUT_EVENT_SIZE];
> };
>
> @@ -109,21 +125,13 @@ struct xenfb_page
> * Each directory page holds PAGE_SIZE / sizeof(*pd)
> * framebuffer pages, and can thus map up to PAGE_SIZE *
> * PAGE_SIZE / sizeof(*pd) bytes. With PAGE_SIZE == 4096 and
> - * sizeof(unsigned long) == 4, that's 4 Megs. Two directory
> - * pages should be enough for a while.
> + * sizeof(unsigned long) == 4/8, that's 4 Megs 32 bit and 2 Megs
> + * 64 bit. 256 directories give enough room for a 512 Meg
> + * framebuffer with a max resolution of 12,800x10,240. Should
> + * be enough for a while with room leftover for expansion.
> */
> - unsigned long pd[2];
> + unsigned long pd[256];
> };
> -
> -/*
> - * Wart: xenkbd needs to know resolution. Put it here until a better
> - * solution is found, but don't leak it to the backend.
> - */
> -#ifdef __KERNEL__
> -#define XENFB_WIDTH 800
> -#define XENFB_HEIGHT 600
> -#define XENFB_DEPTH 32
> -#endif
>
> #endif
>
> diff -r e32fe4703ab6 include/xen/interface/io/kbdif.h
> --- a/include/xen/interface/io/kbdif.h Tue Jan 29 11:53:33 2008 +0000
> +++ b/include/xen/interface/io/kbdif.h Fri Feb 01 11:54:37 2008 -0700
> @@ -119,6 +119,10 @@ struct xenkbd_page
> uint32_t out_cons, out_prod;
> };
>
> +#ifdef __KERNEL__
> +void xenkbd_set_ptr_geometry(int xres, int yres);
> +#endif
> +
> #endif
>
> /*
> diff -r 1b013d10c6d1 tools/ioemu/hw/xenfb.c
> --- a/tools/ioemu/hw/xenfb.c Mon Jan 28 10:37:08 2008 +0000
> +++ b/tools/ioemu/hw/xenfb.c Mon Feb 04 07:30:36 2008 -0700
> @@ -264,6 +264,9 @@ struct xenfb *xenfb_new(int domid, Displ
> xenfb->ds = ds;
> xenfb_device_set_domain(&xenfb->fb, domid);
> xenfb_device_set_domain(&xenfb->kbd, domid);
> +
> + /* Indicate we have the frame buffer resize feature */
> + xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "feature-resize", "1");
Did you take care of the synchronization problem I pointed out here:
Subject: Re: [Xen-devel][PATCH][IOEMU] Dynamic modes support for PV xenfb
(2 of 2)
From: Markus Armbruster <armbru@xxxxxxxxxx>
Date: Thu, 10 Jan 2008 11:18:41 +0100
Message-ID: <87ejcqhtku.fsf@xxxxxxxxxxxxxxxxx>
http://lists.xensource.com/archives/html/xen-devel/2008-01/msg00229.html
?
Discussion starts at
How's the transmission of feature-resize synchronized? The backend
writes it right when it initializes. The frontend reads it on device
probe. What ensures that the backend completes initialization before
the frontend starts probing?
>
> fprintf(stderr, "FB: Waiting for KBD backend creation\n");
> xenfb_wait_for_backend(&xenfb->kbd, xenfb_backend_created_kbd);
> @@ -510,6 +513,12 @@ static void xenfb_on_fb_event(struct xen
> }
> xenfb_guest_copy(xenfb, x, y, w, h);
> break;
> + case XENFB_TYPE_RESIZE:
> + xenfb->width = event->resize.width;
> + xenfb->height = event->resize.height;
> + xenfb->row_stride = event->resize.stride;
> + dpy_resize(xenfb->ds, xenfb->width, xenfb->height);
> + break;
> }
> }
> mb(); /* ensure we're done with ring contents */
> @@ -672,6 +681,7 @@ static int xenfb_read_frontend_fb_config
> static int xenfb_read_frontend_fb_config(struct xenfb *xenfb) {
> struct xenfb_page *fb_page;
> int val;
> + int videoram = 0;
>
> if (xenfb_xs_scanf1(xenfb->xsh, xenfb->fb.otherend, "feature-update",
> "%d", &val) < 0)
The diffs are a bit easier to read if you use tabs like the rest of
the file.
> @@ -686,14 +696,22 @@ static int xenfb_read_frontend_fb_config
> xenfb->protocol[0] = '\0';
> xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "request-update",
> "1");
>
> - /* TODO check for permitted ranges */
> + if (xenfb_xs_scanf1(xenfb->xsh, xenfb->fb.nodename, "videoram",
> "%d", &videoram) < 0)
> + videoram = 0;
> + videoram = videoram * 1024 * 1024;
> +
> fb_page = xenfb->fb.page;
> xenfb->depth = fb_page->depth;
> xenfb->width = fb_page->width;
> xenfb->height = fb_page->height;
> - /* TODO check for consistency with the above */
> xenfb->fb_len = fb_page->mem_length;
> xenfb->row_stride = fb_page->line_length;
> + /* Protect against hostile frontend, limit fb_len to configured */
> + if (videoram && xenfb->fb_len > videoram) {
> + xenfb->fb_len = videoram;
> + if (xenfb->row_stride * xenfb->height > xenfb->fb_len)
> + xenfb->height = xenfb->fb_len/xenfb->row_stride;
Indentation inconsistent with the rest of the file. Please stick to 8
per level.
> + }
> fprintf(stderr, "Framebuffer depth %d width %d height %d line %d\n",
> fb_page->depth, fb_page->width, fb_page->height,
> fb_page->line_length);
> if (xenfb_map_fb(xenfb, xenfb->fb.otherend_id) < 0)
> diff -r 1b013d10c6d1 tools/python/xen/xend/server/vfbif.py
> --- a/tools/python/xen/xend/server/vfbif.py Mon Jan 28 10:37:08 2008 +0000
> +++ b/tools/python/xen/xend/server/vfbif.py Mon Feb 04 08:32:10 2008 -0700
> @@ -6,7 +6,7 @@ import os
> import os
>
> CONFIG_ENTRIES = ['type', 'vncdisplay', 'vnclisten', 'vncpasswd',
> 'vncunused',
> - 'display', 'xauthority', 'keymap',
> + 'videoram', 'display', 'xauthority', 'keymap',
> 'uuid', 'location', 'protocol']
>
> class VfbifController(DevController):
> diff -r 1b013d10c6d1 tools/python/xen/xm/create.py
> --- a/tools/python/xen/xm/create.py Mon Jan 28 10:37:08 2008 +0000
> +++ b/tools/python/xen/xm/create.py Mon Feb 04 09:19:41 2008 -0700
> @@ -490,6 +490,10 @@ gopts.var('vncunused', val='',
> use="""Try to find an unused port for the VNC server.
> Only valid when vnc=1.""")
>
> +gopts.var('videoram', val='',
> + fn=set_value, default=None,
> + use="""Amount of videoram PV guest can allocate for frame
> buffer.""")
> +
> gopts.var('sdl', val='',
> fn=set_value, default=None,
> use="""Should the device model use SDL?""")
> @@ -620,7 +624,7 @@ def configure_vfbs(config_devs, vals):
> d['type'] = 'sdl'
> for (k,v) in d.iteritems():
> if not k in [ 'vnclisten', 'vncunused', 'vncdisplay', 'display',
> - 'xauthority', 'type', 'vncpasswd' ]:
> + 'videoram', 'xauthority', 'type', 'vncpasswd' ]:
> err("configuration option %s unknown to vfbs" % k)
> config.append([k,v])
> if not d.has_key("keymap"):
> diff -r 1b013d10c6d1 xen/include/public/io/fbif.h
[Copy of include/xen/interface/io/fbif.h we already saw, snipp]
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |