"Pat Campbell" <plc@xxxxxxxxxx> 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@xxxxxxxxxx>
>
>
>
>
>
>
>
>
> 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.
> };
>
> /*
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home