Pat Campbell <plc@xxxxxxxxxx> writes:
> Markus Armbruster wrote:
>> Pat Campbell <plc@xxxxxxxxxx> writes:
>>
>>
>>> Patches are against xen-unstable tip
>>>
>>> What changed since last comment round. (No major structural
>>> surprises this time:-).
>>>
>>> Backend:
>>> 1. In the resize event handler
>>> Disabled shared memory (Need to fix)
>>> Invalidate the buffer
>>> 2. Added videoram config variable to limit hostile front
>>> end frame buffer memory size.
>>> 3. Sets feature-resize and width/height in xenstore
>>> before frontend connected. width/height are for xenkbd
>>> abs input config values
>>>
>>> Frontend:
>>> 1. Variable usage cleanup. Now using the fb variables
>>> in most cases. Only two fields added to main struct
>>> 2. Removed resize event send in xenfb_resume(). There is
>>> a general race condition where the vnc view will be left
>>> at 600x400 if attached to too early. (Not resize related)
>>>
>>
>> Could you elaborate on this race?
>>
>>From the command line I suspend the guest
>>From a perl script I:
> Resume the guest, xm resume <guest-name>
> Loop checking for vnc port in xenstore, when found
> immediately attach. 9 times out of 10 times the vncviewer will
> be at 600,400 instead of the default 800x600
>
> This happens in Xen 3.2 without any of my code changes and before the
> recent shared_buf changes. I intend to investigate this further.
Interesting. Please keep us posted.
>>
>>> 3. Removed refresh call in xenfb_resize_screen(), back end
>>> invalidates buffer in resize event handler now
>>> 4. xenkbd gets width and height for abs input in Connected
>>>
>>> After I get some feed back, will wait a day or two, I will
>>> prepare a proper patch set.
>>>
>>> Pat
>>>
>>
>> I like this much better. A couple of questions remain; see comments
>> inline.
>>
>>
>>> diff -r 854b0704962b tools/ioemu/hw/xenfb.c
[...]
>>> diff -r 854b0704962b xen/include/public/io/fbif.h
>>> --- a/xen/include/public/io/fbif.h Wed Mar 05 09:43:03 2008 +0000
>>> +++ b/xen/include/public/io/fbif.h Thu Mar 06 11:12:17 2008 -0600
[...]
>>> /*
>>> - * Wart: xenkbd needs to know resolution. Put it here until a better
>>> - * solution is found, but don't leak it to the backend.
>>> + * Wart: xenkbd needs to know default resolution. Put it here until a
>>> + * better solution is found, but don't leak it to the backend.
>>> */
>>>
>>
>> You still need this wart because you still set the default resolution
>> in xenkbd_probe(). You later reset it to the real maximum resolution
>> in xenkbd_backend_changed(), after frontend went to state Connected.
>> I wonder whether the first one is necessary.
>>
>>
> Yes I think so. Handles the case of the new VM running against an older
> vnc-xenfb or qemu that does not send a new value.
You're right.
>>> #ifdef __KERNEL__
>>> #define XENFB_WIDTH 800
>>> diff -r 1cf7ba68d855 drivers/xen/fbfront/xenfb.c
>>> --- a/drivers/xen/fbfront/xenfb.c Mon Mar 03 13:36:57 2008 +0000
>>> +++ b/drivers/xen/fbfront/xenfb.c Fri Mar 07 21:21:44 2008 -0600
[...]
>>> @@ -209,11 +241,23 @@ 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;
>>>
>>
>> I think you can info->dirty = 0 here. Saves an update event.
>>
> Added that and then took it back out. Somehow that caused the GUI to
> have a terrible delay, minutes, when starting up before you could enter
> your user name password. Changing screens work fast enough. Will have
> to investigate this later.
Weird. It would be good to know what exactly goes wrong there.
>>> + xenfb_do_resize(info);
>>> +}
>>> +
[...]
>>> +static int xenfb_set_par(struct fb_info *info)
>>> +{
>>> + struct xenfb_info *xenfb_info;
>>> +
>>> + xenfb_info = info->par;
>>> +
>>> + info->var.xres_virtual = info->var.xres;
>>> + info->var.yres_virtual = info->var.yres;
>>> + xenfb_info->resize_dpy = 1;
>>> + return 0;
>>> +}
>>>
>>
>> How is this synchronized with xenfb_do_resize()?
>>
>> If that runs on another processor, it could see the new value of
>> resize_dpy, and old values of var.xres and var.yres.
>>
>>
> By the time xenfb_set_par() is called the values are already in the fb
> struct. They are set sometime between the successful xenfb_check_var()
> and xenfb_set_par() calls. I can see a possible condition where if we
> are doing back to back resizes utilizing multiple procs we could get a
> new xres and an old yres.
>
> I will add into xenfb_check_var() the following test:
> if ( xenfb_info->resize_dpy)
> return -EINVAL;
> and move the resize_dpy clear to below the xenfb_do_resize() call
Further discussed in another message, will reply there.
[...]
>>> diff -r 1cf7ba68d855 drivers/xen/fbfront/xenkbd.c
>>> --- a/drivers/xen/fbfront/xenkbd.c Mon Mar 03 13:36:57 2008 +0000
>>> +++ b/drivers/xen/fbfront/xenkbd.c Fri Mar 07 16:57:34 2008 -0600
>>> @@ -295,6 +295,16 @@ static void xenkbd_backend_changed(struc
>>> */
>>> if (dev->state != XenbusStateConnected)
>>> goto InitWait; /* no InitWait seen yet, fudge it */
>>> +
>>> + /* Set input abs params to match backend screen res */
>>> + if (xenbus_scanf(XBT_NIL, info->xbdev->otherend,
>>> + "width", "%d", &val) > 0 )
>>> + input_set_abs_params(info->ptr, ABS_X, 0, val, 0, 0);
>>> +
>>> + if (xenbus_scanf(XBT_NIL, info->xbdev->otherend,
>>> + "height", "%d", &val) > 0 )
>>> + input_set_abs_params(info->ptr, ABS_Y, 0, val, 0, 0);
>>> +
>>> break;
>>>
>>> case XenbusStateClosing:
>>>
>>
>> The combined fb/kbd backend writes width and height right before
>> entering state Connected for both devices. Therefore, we can read it
>> here in the kbd frontend after observing the kbd backend transitioning
>> to Conntected. Okay.
>>
>> But what about the race work-around? If the backend goes through
>> InitWait to Connected fast enough, we don't see the InitWait, and we
>> work around the race with the goto InitWait. But are we guaranteed to
>> get called a second time for the transition to Connected? If not,
>> width and height are never read.
>>
> I debated this. Should those parameters be read before the work
> around? Qemu xenfb did reordered things, is the work around still
> necessary? Should I work around the work around? I never saw that
> case during my testing, not to say it still does not happen. I would
> like to see if this is still an issue in a larger audience before changing.
The work-around is necessary if the backend can go through InitWait to
Connected without synchronizing with the frontend. It actually
synchronizes by waiting for the frontend changing state to
Initialised. I figure the work-around can be removed.
>> Why don't we have to set the parameters on dynamic resolution change?
>>
> I debated this. Should those parameters be read before the work
> around? Qemu xenfb did reorder things, is the work around still
> necessary? Should I work around the work around? I never saw that
> case during my testing, not to say it still does not happen. I would
> like to see if this is still an issue in a larger audience before changing.
Pardon?
[...]
> I will send out a real patch incorporating your suggestions later today.
>
> Thanks
>
> Pat
Got that, working on it.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home