Pat Campbell
2008-Jan-08 15:34 UTC
[Xen-devel][PATCH][IOEMU] Dynamic modes support for PV xenfb (2 of 2)
Attached patch adds multiple frame buffer size support to the xenfb PV backend QEMU xenfb. Backend sets feature-resize and handles the resize frame buffer event. Corresponding frontend LINUX patch is required for functionality but this patch is not dependent on it, preserving backwards compatibility. Please apply to tip of xen-unstable Signed- off- by: Pat Campbell <plc@novell.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Markus Armbruster
2008-Jan-10 10:18 UTC
Re: [Xen-devel][PATCH][IOEMU] Dynamic modes support for PV xenfb (2 of 2)
"Pat Campbell" <plc@novell.com> writes:> Attached patch adds multiple frame buffer size support to the xenfb PV > backend QEMU xenfb. Backend sets feature-resize and handles the > resize frame buffer event. > > Corresponding frontend LINUX patch is required for functionality but this > patch is not dependent on it, preserving backwards compatibility. > > Please apply to tip of xen-unstable > > Signed- off- by: Pat Campbell <plc@novell.com> > > > > > > > > > diff -r 2491691e3e69 tools/ioemu/hw/xenfb.c > --- a/tools/ioemu/hw/xenfb.c Sat Dec 29 17:57:47 2007 +0000 > +++ b/tools/ioemu/hw/xenfb.c Mon Jan 07 12:38:25 2008 -0700 > @@ -53,6 +53,7 @@ struct xenfb { > int abs_pointer_wanted; /* Whether guest supports absolute pointer */ > int button_state; /* Last seen pointer button state */ > char protocol[64]; /* frontend protocol */ > + int fixevdev_abs; /* Descale abs pos so evdev scales properly */ > }; > > /* Functions for frontend/backend state machine*/ > @@ -233,6 +234,7 @@ struct xenfb *xenfb_new(int domid, Displ > struct xenfb *xenfb = qemu_malloc(sizeof(struct xenfb)); > int serrno; > int i; > + int val; > > if (xenfb == NULL) > return NULL; > @@ -264,6 +266,19 @@ struct xenfb *xenfb_new(int domid, Displ > xenfb->ds = ds; > xenfb_device_set_domain(&xenfb->fb, domid); > xenfb_device_set_domain(&xenfb->kbd, domid); > + > + /* See if we need to fix abs ptr for guests evdev */ > + if (xenfb_xs_scanf1(xenfb->xsh, xenfb->fb.nodename, "vnc-fixevdev-abs", > + "%d", &val) < 0) > + val = 0; > + xenfb->fixevdev_abs = val; > + > + /* Indicate we have the frame buffer resize feature if resizable allowed */ > + if (xenfb_xs_scanf1(xenfb->xsh, xenfb->fb.nodename, "vncresizable-pvfb", > + "%d", &val) < 0) > + val = 0; > + if (val) > + xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "xenfb-feature-resize", "1");You pass configuration parameters vnc-fixevdev-abs and vncresizable-pvfb through xenstore. The others are on the qemu command line. I''m not fond of configuring qemu through xenstore, but I guess it''s the simplest solution here. The name xenfb-feature-resize doesn''t match existing names request-update, feature-update, request-abs-pointer, feature-abs-pointer. Suggest to call it feature-resize. 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? Have a look at the device initialization event diagram at http://wiki.xensource.com/xenwiki/XenSplitDrivers: Xenbus Xenbus Hotplug Backend Frontend ------- ------- -------- Initialising Initialising | | |<---start----+ | | | | | InitWait | | | write | | ring/ write | channel physdets-------->| details | | |<---------------------Initialised | | write | physdets | | | Connected---------------------->| | | | Connected | | For xenfb, this becomes: xenfb xenfb Backend Frontend ------- -------- Initialising Initialising | | | | | | InitWait | | write | page-ref, event-channel | protocol, feature-update | | |<---------------------Initialised | | read | page-ref, event-channel | protocol, feature-update | write | request-update | | | Connected---------------------->| | | | read | request-update | | | Connected | | Your patches add: xenfb xenfb Backend Frontend ------- -------- write read feature-resize feature-resize | | Initialising Initialising | | ... ... Here''s a design that would fit more naturally into the protocol: make the frontend advertise feature-resize, and the backend request-resize, just like feature-update / request-update. The trouble with that is that the frontend knows doesn''t know whether to do resize until it goes to state Connected. Complicates framebuffer allocation. It''s allocated in xenfb_probe() for a reason: it''s easy to handle failed allocations there, just fail the probe. Relatively easy way out: allocate the full framebuffer in probe, attempt to downsize it when going to Connected. By the way, the feature negotiation only covers whether the frontend is permitted to resize, not acceptable resolutions. What if the frontend resizes to a resolution the backend can''t accept? The backend has no way to tell the frontend not to do that. Would we end up with a defunct display and no way for the user to fix it? What happens when a malicious frontend resizes to a resolution that makes pd[] extend beyond the end of the shared page? Nothing new really, the current backend has the same problem with the initial resolution, I think.> > fprintf(stderr, "FB: Waiting for KBD backend creation\n"); > xenfb_wait_for_backend(&xenfb->kbd, xenfb_backend_created_kbd); > @@ -510,6 +525,11 @@ 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; > + dpy_resize(xenfb->ds, xenfb->width, xenfb->height); > + break; > } > } > mb(); /* ensure we''re done with ring contents */ > @@ -605,11 +625,22 @@ static int xenfb_send_motion(struct xenf > return xenfb_kbd_event(xenfb, &event); > } > > +/* Descale abs pos for older evdev_drv without AbsoluteScreen option */ > +static inline int xenfb_descale_for_evdev(float fb_width, float screen_width, float pos)This is used both for width and height, despite the parameter names. Suggest something like limit and max_limit.> +{ > + return(((fb_width/screen_width) * pos) + 0.5);Style nitpick: return (fb_width/screen_width) * pos + 0.5;> +} > + > /* Send an absolute mouse movement event */ > static int xenfb_send_position(struct xenfb *xenfb, int abs_x, int abs_y, int abs_z) > { > union xenkbd_in_event event; > > + if (xenfb->fixevdev_abs) { > + struct xenfb_page *page = xenfb->fb.page; > + abs_x = xenfb_descale_for_evdev(page->width, xenfb->width, abs_x); > + abs_y = xenfb_descale_for_evdev(page->height, xenfb->height, abs_y); > + } > memset(&event, 0, XENKBD_IN_EVENT_SIZE); > event.type = XENKBD_TYPE_POS; > event.pos.abs_x = abs_x; > diff -r 2491691e3e69 tools/python/xen/xend/server/vfbif.py > --- a/tools/python/xen/xend/server/vfbif.py Sat Dec 29 17:57:47 2007 +0000 > +++ b/tools/python/xen/xend/server/vfbif.py Sun Jan 06 12:35:21 2008 -0700 > @@ -7,7 +7,8 @@ import os > > CONFIG_ENTRIES = [''type'', ''vncdisplay'', ''vnclisten'', ''vncpasswd'', ''vncunused'', > ''display'', ''xauthority'', ''keymap'', > - ''uuid'', ''location'', ''protocol''] > + ''uuid'', ''location'', ''protocol'', > + ''vncresizable-pvfb'', ''vnc-fixevdev-abs'' ] > > class VfbifController(DevController): > """Virtual frame buffer controller. Handles all vfb devices for a domain. > diff -r 2491691e3e69 tools/python/xen/xm/create.py > --- a/tools/python/xen/xm/create.py Sat Dec 29 17:57:47 2007 +0000 > +++ b/tools/python/xen/xm/create.py Sun Jan 06 12:34:38 2008 -0700 > @@ -485,6 +485,14 @@ gopts.var(''vnclisten'', val='''', > fn=set_value, default=None, > use="""Address for VNC server to listen on.""") > > +gopts.var(''vncresizable-pvfb'', val=''no|yes'', > + fn=set_bool, default=0, > + use="""Allow resizable VNC PVFB if supported.""") > + > +gopts.var(''vnc-fixevdev-abs'', val=''no|yes'', > + fn=set_bool, default=0, > + use="""Correct resizable abs pointer positioning for evdev.""") > + > gopts.var(''vncunused'', val='''', > fn=set_bool, default=1, > use="""Try to find an unused port for the VNC server. > @@ -620,7 +628,8 @@ 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'' ]: > + ''xauthority'', ''type'', ''vncpasswd'', > + ''vncresizable-pvfb'', ''vnc-fixevdev-abs'' ]: > err("configuration option %s unknown to vfbs" % k) > config.append([k,v]) > if not d.has_key("keymap"): > diff -r 2491691e3e69 xen/include/public/io/fbif.h > --- a/xen/include/public/io/fbif.h Sat Dec 29 17:57:47 2007 +0000 > +++ b/xen/include/public/io/fbif.h Sat Jan 05 11:10:07 2008 -0700 > @@ -50,12 +50,26 @@ 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; /* xres */Commenting one snappy-short identifier with another one seems rather pointless to me :) If you insist on a comment, what about: x-resolution in pixels.> + int32_t height; /* yres */ > +}; > + > #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]; > }; > > @@ -111,8 +125,12 @@ struct xenfb_page > * 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. > + * > + * Increased to 3 to support 1280x1024 resolution on a 64bit system > + * (1280*1024*4)/PAGE_SIZE = 1280 pages required > + * PAGE_SIZE/64bit long = 512 pages per page directoryPlease update the comment instead of appending to it.> */ > - unsigned long pd[2]; > + unsigned long pd[3];Extending pd[] is safe, because: * Current backends compute the length of pd[] from the size of the framebuffer. However, when protocol is not set, they rely on pd[1] == 0 to make 32-on-64 work. * Old frontends don''t set the protocol and use only pd[0]. They set pd[1] to 0. * Current frontends set the protocol and use pd[0..1]. Unused parts of the shared page are 0, including pd[2]. * Your frontend additionally uses pd[2], but only when the backend agreed to it.> }; > > /*_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Pat Campbell
2008-Jan-11 15:15 UTC
Re: [Xen-devel][PATCH][IOEMU] Dynamic modes support for PV xenfb (2 of 2)
>>> On Thu, Jan 10, 2008 at 3:18 AM, in message<87ejcqhtku.fsf@pike.pond.sub.org>, Markus Armbruster <armbru@redhat.com> wrote:> "Pat Campbell" <plc@novell.com> writes: > >> Attached patch adds multiple frame buffer size support to the xenfb PV >> backend QEMU xenfb. Backend sets feature- resize and handles the >> resize frame buffer event. >> >> Corresponding frontend LINUX patch is required for functionality but this >> patch is not dependent on it, preserving backwards compatibility. >> >> Please apply to tip of xen- unstable >> >> Signed- off- by: Pat Campbell <plc@novell.com> >> >> >> >> >> >> >> >> >> diff - r 2491691e3e69 tools/ioemu/hw/xenfb.c >> --- a/tools/ioemu/hw/xenfb.c Sat Dec 29 17:57:47 2007 +0000 >> +++ b/tools/ioemu/hw/xenfb.c Mon Jan 07 12:38:25 2008 - 0700 >> @@ - 53,6 +53,7 @@ struct xenfb { >> int abs_pointer_wanted; /* Whether guest supports absolute pointer */ >> int button_state; /* Last seen pointer button state */ >> char protocol[64]; /* frontend protocol */ >> + int fixevdev_abs; /* Descale abs pos so evdev scales properly */ >> }; >> >> /* Functions for frontend/backend state machine*/ >> @@ - 233,6 +234,7 @@ struct xenfb *xenfb_new(int domid, Displ >> struct xenfb *xenfb = qemu_malloc(sizeof(struct xenfb)); >> int serrno; >> int i; >> + int val; >> >> if (xenfb == NULL) >> return NULL; >> @@ - 264,6 +266,19 @@ struct xenfb *xenfb_new(int domid, Displ >> xenfb- >ds = ds; >> xenfb_device_set_domain(&xenfb- >fb, domid); >> xenfb_device_set_domain(&xenfb- >kbd, domid); >> + >> + /* See if we need to fix abs ptr for guests evdev */ >> + if (xenfb_xs_scanf1(xenfb- >xsh, xenfb- >fb.nodename, "vnc- fixevdev- abs", >> + "%d", &val) < 0) >> + val = 0; >> + xenfb- >fixevdev_abs = val; >> + >> + /* Indicate we have the frame buffer resize feature if resizable allowed */ >> + if (xenfb_xs_scanf1(xenfb- >xsh, xenfb- >fb.nodename, "vncresizable- pvfb", >> + "%d", &val) < 0) >> + val = 0; >> + if (val) >> + xenfb_xs_printf(xenfb- >xsh, xenfb- >fb.nodename, "xenfb- feature- resize", "1"); > > You pass configuration parameters vnc- fixevdev- abs and > vncresizable- pvfb through xenstore. The others are on the qemu > command line. I''m not fond of configuring qemu through xenstore, but > I guess it''s the simplest solution here.Dan suggested I use kernel parameters for these instead of xenstore. Going to remove these and go that route. Also solves the init/connect problem you pointed out below.> > The name xenfb- feature- resize doesn''t match existing names > request- update, feature- update, request- abs- pointer, > feature- abs- pointer. Suggest to call it feature- resize.Ok> > 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?I guess it wasn''t. feature-resize is now set in state ''Connected'' like request-update. kernel parameter ''xenfb.dynamicModes'' is tested in the probe function to determine framebuffer memory allocation size.> > Have a look at the device initialization event diagram at > http://wiki.xensource.com/xenwiki/XenSplitDrivers: > > Xenbus Xenbus > Hotplug Backend Frontend > ------- ------- -------- > > Initialising Initialising > | | > |<--- start---- + | > | | | > | InitWait | > | | write > | | ring/ > write | channel > physdets-------- >| details > | | > |<--------------------- Initialised > | | > write | > physdets | > | | > Connected---------------------- >| > | | > | Connected > | | > > For xenfb, this becomes: > > xenfb xenfb > Backend Frontend > ------- -------- > > Initialising Initialising > | | > | | > | | > InitWait | > | write > | page- ref, event- channel > | protocol, feature- update > | | > |<--------------------- Initialised > | | > read | > page- ref, event- channel | > protocol, feature- update | > write | > request- update | > | | > Connected---------------------- >| > | | > | read > | request- update > | | > | Connected > | | > > Your patches add: > > xenfb xenfb > Backend Frontend > ------- -------- > > write read > feature- resize feature- resize > | | > Initialising Initialising > | | > ... ... > > Here''s a design that would fit more naturally into the protocol: make > the frontend advertise feature- resize, and the backend request- resize, > just like feature- update / request- update. > > The trouble with that is that the frontend knows doesn''t know whether > to do resize until it goes to state Connected. Complicates > framebuffer allocation. It''s allocated in xenfb_probe() for a reason: > it''s easy to handle failed allocations there, just fail the probe. > > Relatively easy way out: allocate the full framebuffer in probe, > attempt to downsize it when going to Connected. > > By the way, the feature negotiation only covers whether the frontend > is permitted to resize, not acceptable resolutions.True. Acceptable resolutions are what fits within a 5MB framebuffer and has a width no greater than 1280.> > What if the frontend resizes to a resolution the backend can''t accept? > The backend has no way to tell the frontend not to do that. Would we > end up with a defunct display and no way for the user to fix it?I don''t think this is a possible issue. Frontend code limits the size of the frame buffer to 5MB with a max horizontal scanline length of 1280. The backend VNC server should be able to handle that without any problems.> > What happens when a malicious frontend resizes to a resolution that > makes pd[] extend beyond the end of the shared page? Nothing new > really, the current backend has the same problem with the initial > resolution, I think.Can''t do that either. Maximum frame buffer size of 5MB fits within the 3 page descriptors.> >> >> fprintf(stderr, "FB: Waiting for KBD backend creation\n"); >> xenfb_wait_for_backend(&xenfb- >kbd, xenfb_backend_created_kbd); >> @@ - 510,6 +525,11 @@ 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; >> + dpy_resize(xenfb- >ds, xenfb- >width, xenfb- >height); >> + break; >> } >> } >> mb(); /* ensure we''re done with ring contents */ >> @@ - 605,11 +625,22 @@ static int xenfb_send_motion(struct xenf >> return xenfb_kbd_event(xenfb, &event); >> } >> >> +/* Descale abs pos for older evdev_drv without AbsoluteScreen option */ >> +static inline int xenfb_descale_for_evdev(float fb_width, float > screen_width, float pos) > > This is used both for width and height, despite the parameter names. > Suggest something like limit and max_limit. > >> +{ >> + return(((fb_width/screen_width) * pos) + 0.5); > > Style nitpick: > > return (fb_width/screen_width) * pos + 0.5; > >> +} >> +I have removed the whole scale code. I have arbitarily decided that if a VM wishes to use dynamic modes and absolute mouse positioning it should have an updated X evdev driver like what is in Fedora8 or OpenSuSE 10.3 virt-manager with mouse grab works as good as it alway has.>> /* Send an absolute mouse movement event */ >> static int xenfb_send_position(struct xenfb *xenfb, int abs_x, int abs_y, > int abs_z) >> { >> union xenkbd_in_event event; >> >> + if (xenfb- >fixevdev_abs) { >> + struct xenfb_page *page = xenfb- >fb.page; >> + abs_x = xenfb_descale_for_evdev(page- >width, xenfb- >width, abs_x); >> + abs_y = xenfb_descale_for_evdev(page- >height, xenfb- >height, abs_y); >> + } >> memset(&event, 0, XENKBD_IN_EVENT_SIZE); >> event.type = XENKBD_TYPE_POS; >> event.pos.abs_x = abs_x; >> diff - r 2491691e3e69 tools/python/xen/xend/server/vfbif.py >> --- a/tools/python/xen/xend/server/vfbif.py Sat Dec 29 17:57:47 2007 +0000 >> +++ b/tools/python/xen/xend/server/vfbif.py Sun Jan 06 12:35:21 2008 - 0700 >> @@ - 7,7 +7,8 @@ import os >> >> CONFIG_ENTRIES = [''type'', ''vncdisplay'', ''vnclisten'', ''vncpasswd'', > ''vncunused'', >> ''display'', ''xauthority'', ''keymap'', >> - ''uuid'', ''location'', ''protocol''] >> + ''uuid'', ''location'', ''protocol'', >> + ''vncresizable- pvfb'', ''vnc- fixevdev- abs'' ] >> >> class VfbifController(DevController): >> """Virtual frame buffer controller. Handles all vfb devices for a > domain. >> diff - r 2491691e3e69 tools/python/xen/xm/create.py >> --- a/tools/python/xen/xm/create.py Sat Dec 29 17:57:47 2007 +0000 >> +++ b/tools/python/xen/xm/create.py Sun Jan 06 12:34:38 2008 - 0700 >> @@ - 485,6 +485,14 @@ gopts.var(''vnclisten'', val='''', >> fn=set_value, default=None, >> use="""Address for VNC server to listen on.""") >> >> +gopts.var(''vncresizable- pvfb'', val=''no|yes'', >> + fn=set_bool, default=0, >> + use="""Allow resizable VNC PVFB if supported.""") >> + >> +gopts.var(''vnc- fixevdev- abs'', val=''no|yes'', >> + fn=set_bool, default=0, >> + use="""Correct resizable abs pointer positioning for evdev.""") >> + >> gopts.var(''vncunused'', val='''', >> fn=set_bool, default=1, >> use="""Try to find an unused port for the VNC server. >> @@ - 620,7 +628,8 @@ 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'' ]: >> + ''xauthority'', ''type'', ''vncpasswd'', >> + ''vncresizable- pvfb'', ''vnc- fixevdev- abs'' ]: >> err("configuration option %s unknown to vfbs" % k) >> config.append([k,v]) >> if not d.has_key("keymap"): >> diff - r 2491691e3e69 xen/include/public/io/fbif.h >> --- a/xen/include/public/io/fbif.h Sat Dec 29 17:57:47 2007 +0000 >> +++ b/xen/include/public/io/fbif.h Sat Jan 05 11:10:07 2008 - 0700 >> @@ - 50,12 +50,26 @@ 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; /* xres */ > > Commenting one snappy- short identifier with another one seems rather > pointless to me :) > > If you insist on a comment, what about: x- resolution in pixels.Ok> >> + int32_t height; /* yres */ >> +}; >> + >> #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]; >> }; >> >> @@ - 111,8 +125,12 @@ struct xenfb_page >> * 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. >> + * >> + * Increased to 3 to support 1280x1024 resolution on a 64bit system >> + * (1280*1024*4)/PAGE_SIZE = 1280 pages required >> + * PAGE_SIZE/64bit long = 512 pages per page directory > > Please update the comment instead of appending to it. > >> */ >> - unsigned long pd[2]; >> + unsigned long pd[3]; > > Extending pd[] is safe, because: > > * Current backends compute the length of pd[] from the size of the > framebuffer. However, when protocol is not set, they rely on pd[1] > == 0 to make 32- on- 64 work. > > * Old frontends don''t set the protocol and use only pd[0]. They set > pd[1] to 0. > > * Current frontends set the protocol and use pd[0..1]. Unused parts > of the shared page are 0, including pd[2]. >> * Your frontend additionally uses pd[2], but only when the backend > agreed to it. >This has changed slightly with the use of dynamicMode module parameter. Now using pd[2] if the vm was configured for it.>> }; >> >> /*Thanks for your feedback Will send updated patch when I have incorporated your suggestions Pat _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Markus Armbruster
2008-Jan-12 07:46 UTC
Re: [Xen-devel][PATCH][IOEMU] Dynamic modes support for PV xenfb (2 of 2)
"Pat Campbell" <plc@novell.com> writes:>>>> On Thu, Jan 10, 2008 at 3:18 AM, in message > <87ejcqhtku.fsf@pike.pond.sub.org>, Markus Armbruster <armbru@redhat.com> > wrote: >> "Pat Campbell" <plc@novell.com> writes: >> >>> Attached patch adds multiple frame buffer size support to the xenfb PV >>> backend QEMU xenfb. Backend sets feature- resize and handles the >>> resize frame buffer event. >>> >>> Corresponding frontend LINUX patch is required for functionality but this >>> patch is not dependent on it, preserving backwards compatibility.[...]>> By the way, the feature negotiation only covers whether the frontend >> is permitted to resize, not acceptable resolutions. > > True. Acceptable resolutions are what fits within a 5MB framebuffer > and has a width no greater than 1280.Is this enough for all eternity? Screen resolutions continue to grow... If it''s not, then how can we ensure that we can enlarge it easily while maintaining backwards compatibility? Perhaps the backend could publish the maximum resolution it can accept, instead of just a flag that it can accept resize events.>> What if the frontend resizes to a resolution the backend can''t accept? >> The backend has no way to tell the frontend not to do that. Would we >> end up with a defunct display and no way for the user to fix it? > > I don''t think this is a possible issue. Frontend code limits the size of > the frame buffer to 5MB with a max horizontal scanline length of > 1280. The backend VNC server should be able to handle that without > any problems. > >> >> What happens when a malicious frontend resizes to a resolution that >> makes pd[] extend beyond the end of the shared page? Nothing new >> really, the current backend has the same problem with the initial >> resolution, I think. > > Can''t do that either. Maximum frame buffer size of 5MB fits within > the 3 page descriptors.What stops the frontend from putting a bogus framebuffer description in the shared page? If that can make the backend overrun the shared page, we have a security problem. I think the backend should verify that the framebuffer dimension against he limits you quoted.>>> fprintf(stderr, "FB: Waiting for KBD backend creation\n"); >>> xenfb_wait_for_backend(&xenfb- >kbd, xenfb_backend_created_kbd); >>> @@ - 510,6 +525,11 @@ 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; >>> + dpy_resize(xenfb- >ds, xenfb- >width, xenfb- >height); >>> + break; >>> } >>> } >>> mb(); /* ensure we''re done with ring contents */ >>> @@ - 605,11 +625,22 @@ static int xenfb_send_motion(struct xenf >>> return xenfb_kbd_event(xenfb, &event); >>> } >>> >>> +/* Descale abs pos for older evdev_drv without AbsoluteScreen option */ >>> +static inline int xenfb_descale_for_evdev(float fb_width, float >> screen_width, float pos) >> >> This is used both for width and height, despite the parameter names. >> Suggest something like limit and max_limit. >> >>> +{ >>> + return(((fb_width/screen_width) * pos) + 0.5); >> >> Style nitpick: >> >> return (fb_width/screen_width) * pos + 0.5; >> >>> +} >>> + > > I have removed the whole scale code. I have arbitarily decided that if > a VM wishes to use dynamic modes and absolute mouse positioning it > should have an updated X evdev driver like what is in Fedora8 or > OpenSuSE 10.3 > > virt-manager with mouse grab works as good as it alway has.Good move! [...] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel P. Berrange
2008-Jan-12 15:46 UTC
Re: [Xen-devel][PATCH][IOEMU] Dynamic modes support for PV xenfb (2 of 2)
On Sat, Jan 12, 2008 at 08:46:46AM +0100, Markus Armbruster wrote:> "Pat Campbell" <plc@novell.com> writes: > > >>>> On Thu, Jan 10, 2008 at 3:18 AM, in message > > <87ejcqhtku.fsf@pike.pond.sub.org>, Markus Armbruster <armbru@redhat.com> > > wrote: > >> "Pat Campbell" <plc@novell.com> writes: > >> > >>> Attached patch adds multiple frame buffer size support to the xenfb PV > >>> backend QEMU xenfb. Backend sets feature- resize and handles the > >>> resize frame buffer event. > >>> > >>> Corresponding frontend LINUX patch is required for functionality but this > >>> patch is not dependent on it, preserving backwards compatibility. > [...] > >> By the way, the feature negotiation only covers whether the frontend > >> is permitted to resize, not acceptable resolutions. > > > > True. Acceptable resolutions are what fits within a 5MB framebuffer > > and has a width no greater than 1280. > > Is this enough for all eternity? Screen resolutions continue to > grow... If it''s not, then how can we ensure that we can enlarge it > easily while maintaining backwards compatibility? > > Perhaps the backend could publish the maximum resolution it can > accept, instead of just a flag that it can accept resize events.Maximum resolutions are not well defined because they vary according to your aspect ratio - normal vs widescreen. I increasingly like the like of having a ''videoram'' parameter in the backend config, so the admin can set as large a value as they feel neccessary, and frontend then set whatever resolutions they like within the constraint of that value. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=| _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel