|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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@xxxxxxxxxxxxxxxxx>, Markus Armbruster <armbru@xxxxxxxxxx>
wrote:
> "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.
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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |