Pat Campbell
2008-Feb-04 18:49 UTC
[Xen-devel][RFC]Dynamic modes support for PV xenfb (included)
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 to all that have looked at this never ending RFC and commented.
Pat
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Markus Armbruster
2008-Feb-05 09:52 UTC
Re: [Xen-devel][RFC]Dynamic modes support for PV xenfb (included)
Pat Campbell <plc@novell.com> 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@redhat.com> Date: Thu, 10 Jan 2008 11:18:41 +0100 Message-ID: <87ejcqhtku.fsf@pike.pond.sub.org> 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@lists.xensource.com http://lists.xensource.com/xen-devel
Pat Campbell
2008-Feb-05 23:38 UTC
Re: [Xen-devel][RFC]Dynamic modes support for PV xenfb (included)
Markus Armbruster wrote:> Pat Campbell <plc@novell.com> writes: >cut out a whole bunch to address the need to protect some variables.> > >> + >> + 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. > >Don''t need to protect these three: fb_stride: Eliminated, now using fb_info->fixed.line_len which is read only after probe() xres, yres: Set in xenfb_set_par(), read only in all other threads. resize_dpy: While thinking about this variable it occurred to me that a resize would invalidate any screen updates that are in the queue. Resize from xenfb_thread() was done so that I could ensure that the resize did not get lost due to a queue full situation. So, if I clear the queue, cons=prod, I can add in the resize event packet directly from xenfb_set_par() getting rid of resizing from within the thread. Change would require a new spinlock to protect the queue but would eliminate resize_dpy. Setting cons=prod should be safe. Worst case is when a clear occurs while backend is in the event for loop, loop process events it did not need to. xenfb_resize_screen() becomes: (called directly from xenfb_set_par()) spin_lock_irqsave(&info->queue_lock, flags); info->page->out_cons = info->page->out_prod; spin_unlock_irqrestore(&info->queue_lock, flags); xenfb_do_resize(info); xenfb_refresh(info, 0, 0, info->xres, info->yres); xenfb_send_event() becomes: spin_lock_irqsave(&info->queue_lock, flags); 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; spin_unlock_irqrestore(&info->queue_lock, flags); Thoughts on this approach? Pat _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Markus Armbruster
2008-Feb-06 15:49 UTC
Re: [Xen-devel][RFC]Dynamic modes support for PV xenfb (included)
Pat Campbell <plc@novell.com> writes:> Markus Armbruster wrote: >> Pat Campbell <plc@novell.com> writes: >> > cut out a whole bunch to address the need to protect some variables. >> >> >>> + >>> + 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. >> >> > Don''t need to protect these three: > fb_stride: Eliminated, now using fb_info->fixed.line_len which is read > only after probe() > xres, yres: > Set in xenfb_set_par(), read only in all other threads.Say one thread does: xenfb_info->xres = ... xenfb_info->yres = ... xenfb_info->resize_dpy = 1; And another thread does: if (info->resize_dpy) { info->resize_dpy = 0; event.type = XENFB_TYPE_RESIZE; event.resize.width = info->xres; event.resize.height = info->yres; event.resize.stride = info->fb_stride; You''re in trouble on SMP. The assignment to resize_dpy may be visible on another processor *before* the assignments to xres, yres. In other words, the second thread can use old values, and the resize gets lost or bent out of shape. You need memory barriers to protect against this. The easiest way by far to get memory barriers right is to use some higher level synchronization primitive.> resize_dpy: > While thinking about this variable it occurred to me that a resize would > invalidate any screen updates that are in the queue. Resize from > xenfb_thread() was done so that I could ensure that the resize did not > get lost due to a queue full situation. So, if I clear the queue, > cons=prod, I can add in the resize event packet directly from > xenfb_set_par() getting rid of resizing from within the thread. ChangeIt''s tempting, isn''t it?> would require a new spinlock to protect the queue but would eliminate > resize_dpy. > > Setting cons=prod should be safe. Worst case is when a clear occurs > while backend is in the event for loop, loop process events it did not > need to. > > xenfb_resize_screen() becomes: (called directly from xenfb_set_par()) > spin_lock_irqsave(&info->queue_lock, flags); > info->page->out_cons = info->page->out_prod; > spin_unlock_irqrestore(&info->queue_lock, flags); > xenfb_do_resize(info); > xenfb_refresh(info, 0, 0, info->xres, info->yres); > > xenfb_send_event() becomes: > spin_lock_irqsave(&info->queue_lock, flags); > 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; > spin_unlock_irqrestore(&info->queue_lock, flags); > > Thoughts on this approach? > > PatThe correctness of a lockless ring buffer depends on having *one* producer put stuff in (here: frontend), and *one* consumer take stuff out (here: backend), very carefully. If I understand you correctly, you''re proposing to rely on queue_lock to synchronize within the frontend (mutex xenfb_resize_screen() and xenfb_send_event()), and argue that neglecting to synchronize frontend with backend can''t do harm. I fear you are wrong. Say the producer empties the ring buffer after the consumer determined that the ring buffer is not empty, and before it is done taking out the first element. Say that element is in slot #7. The producer then continues, and puts stuff into the now empty ring buffer normally. Eventually, it''ll reach slot#7. The consumer may or may not be done is with that element at that time. We got a race condition. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel