* [PATCH][IOEMU] Dynamic modes support for PV xenfb (2 of 2)
@ 2008-01-08 15:34 Pat Campbell
2008-01-10 10:18 ` Markus Armbruster
0 siblings, 1 reply; 5+ messages in thread
From: Pat Campbell @ 2008-01-08 15:34 UTC (permalink / raw)
To: xen-devel
[-- Attachment #1: Type: text/plain, Size: 428 bytes --]
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>
[-- Attachment #2: xen-fbback-resize.patch --]
[-- Type: text/plain, Size: 6001 bytes --]
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");
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)
+{
+ 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 */
+ 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
*/
- unsigned long pd[2];
+ unsigned long pd[3];
};
/*
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][IOEMU] Dynamic modes support for PV xenfb (2 of 2)
2008-01-08 15:34 [PATCH][IOEMU] Dynamic modes support for PV xenfb (2 of 2) Pat Campbell
@ 2008-01-10 10:18 ` Markus Armbruster
[not found] ` <47872348.3E48.0018.0@novell.com>
0 siblings, 1 reply; 5+ messages in thread
From: Markus Armbruster @ 2008-01-10 10:18 UTC (permalink / raw)
To: Pat Campbell; +Cc: xen-devel
"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 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.
> };
>
> /*
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][IOEMU] Dynamic modes support for PV xenfb (2 of 2)
@ 2008-01-11 15:15 Pat Campbell
0 siblings, 0 replies; 5+ messages in thread
From: Pat Campbell @ 2008-01-11 15:15 UTC (permalink / raw)
To: xen-devel
>>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][IOEMU] Dynamic modes support for PV xenfb (2 of 2)
[not found] ` <47872348.3E48.0018.0@novell.com>
@ 2008-01-12 7:46 ` Markus Armbruster
2008-01-12 15:46 ` Daniel P. Berrange
0 siblings, 1 reply; 5+ messages in thread
From: Markus Armbruster @ 2008-01-12 7:46 UTC (permalink / raw)
To: Pat Campbell; +Cc: xen-devel
"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!
[...]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][IOEMU] Dynamic modes support for PV xenfb (2 of 2)
2008-01-12 7:46 ` Markus Armbruster
@ 2008-01-12 15:46 ` Daniel P. Berrange
0 siblings, 0 replies; 5+ messages in thread
From: Daniel P. Berrange @ 2008-01-12 15:46 UTC (permalink / raw)
To: Markus Armbruster; +Cc: xen-devel, Pat Campbell
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 -=|
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2008-01-12 15:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-01-08 15:34 [PATCH][IOEMU] Dynamic modes support for PV xenfb (2 of 2) Pat Campbell
2008-01-10 10:18 ` Markus Armbruster
[not found] ` <47872348.3E48.0018.0@novell.com>
2008-01-12 7:46 ` Markus Armbruster
2008-01-12 15:46 ` Daniel P. Berrange
-- strict thread matches above, loose matches on Subject: below --
2008-01-11 15:15 Pat Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.