Xen 3.4.x and request-abs-pointer

Xen 3.4.x and request-abs-pointer

[John Haxby]

[-- Attachment #1: Type: text/plain, Size: 329 bytes --]

A little while ago Jeremy Fitzhardinge was having some problem with 
absolute pointers.  This turned out to be a very simple fix in qemu-xen: 
wait for the frontend to be connected before connecting the backend.  
I've attached that original patch, if there's going to be a xen-3.4.4 it 
would be nice if this was included.

jch

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1037 bytes --]

commit 805ed3b20492d2f4bb465bfda65cedd286e23209
Author: Ian Jackson <ian.jackson@eu.citrix.com>
Date:   Fri May 21 15:46:55 2010 +0100

    Wait for frontend state Connected before connecting the backend
    
    The frontend of the framebuffer set a value (request-abs-pointer) and go
    to the state Connected.  The backend must read this value only when the
    frontend has the state Connected.
    
    From: Anthony PERARD <anthony.perard@citrix.com>
    Tested-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

diff --git a/hw/xen_backend.c b/hw/xen_backend.c
index 76d07ec..31ed7b0 100644
--- a/hw/xen_backend.c
+++ b/hw/xen_backend.c
@@ -411,8 +411,7 @@ static int xen_be_try_connect(struct XenDevice *xendev)
 {
     int rc = 0;
 
-    if (xendev->fe_state != XenbusStateInitialised  &&
-	xendev->fe_state != XenbusStateConnected) {
+    if (xendev->fe_state != XenbusStateConnected) {
 	if (xendev->ops->flags & DEVOPS_FLAG_IGNORE_STATE) {
 	    xen_be_printf(xendev, 2, "frontend not ready, ignoring\n");
 	} else {

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: Xen 3.4.x and request-abs-pointer

[Stefano Stabellini]

On Mon, 5 Jul 2010, John Haxby wrote:
> A little while ago Jeremy Fitzhardinge was having some problem with 
> absolute pointers.  This turned out to be a very simple fix in qemu-xen: 
> wait for the frontend to be connected before connecting the backend.  
> I've attached that original patch, if there's going to be a xen-3.4.4 it 
> would be nice if this was included.
> 
> jch
> 

That patch was recently reverted because it was the wrong fix:


>From stefano.stabellini@eu.citrix.com Thu Jul  1 12:36:06 2010
Date: Thu, 1 Jul 2010 12:32:15 +0100
From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
To: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>, Ian
 Jackson <Ian.Jackson@eu.citrix.com>, Keir Fraser <Keir.Fraser@eu.citrix.com>,
	Anthony Perard <anthony.perard@citrix.com>, Keir, Gerd Hoffmann
	<kraxel@redhat.com>
Subject: Re: [Xen-devel] [PATCH 1 of 1] mini-os: PV fronted MUST be in XenbusStateConnected not XenbusStateInitialized during init

Thank you for the patch Konrad.

I think this fix shows us that 805ed3b20492d2f4bb465bfda65cedd286e23209
was the wrong fix:

commit 805ed3b20492d2f4bb465bfda65cedd286e23209
Author: Ian Jackson <ian.jackson@eu.citrix.com>
Date:   Fri May 21 15:46:55 2010 +0100

    Wait for frontend state Connected before connecting the backend

    The frontend of the framebuffer set a value
    (request-abs-pointer) and go
    to the state Connected.  The backend must read this value
    only when the
    frontend has the state Connected.


The problem was that the backend can be sure that the linux xenfb
frontend wrote request-abs-pointer only after the frontend state is
Connected.
In order to do that properly we need a new hook in qemu xen_backend: we
should probably rename the current connect hook to initialise and create
a new connect hook that would be implemented by xenfb to read
request-abs-pointer.



On Wed, 30 Jun 2010, Konrad Rzeszutek Wilk wrote:
> # HG changeset patch
> # User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> # Date 1277908182 14400
> # Node ID 76e7d0258f65e4ab1b105cd70a96f2a411d5ca22
> # Parent  a24dbfcbdf695f49867d5881ea20ab40f18aea98
> mini-os: PV fronted MUST be in XenbusStateConnected not XenbusStateInitialized during init.
> 
> With the QEMU change 805ed3b20492d2f4bb465bfda65cedd286e23209
> ("Wait for frontend state Connected before connecting the backend"),
> where QEMU backend (say VNC framebuffer) will ONLY call ops->connect
> (which when successfull, will move the backend state from XenbusStateInitWait
> to XenbusStateConnected) when the frontend state is in XenbusStateConnected.
> 
> Previous to that git commit it would call ops->connect also
> if the frontend was in XenbusStateInitialized state.
> 
> Without this patch, the MiniOS (in my case pvgrub) would hang
> in the fbfront and kbfront threads waiting for the backend to change
> its state from InitWait to Connected. Which the backend would not do
> as the ops->connect in QEMU was never called.
> 
> The c/s 21260 "mini-os: Revert 21106:b20f897d6010 "Fix xenbus initialisation"
> fixes this for the blkfront and netfront. Unfortunately
> it did not fix it for the fbfront, kbdfront and pcifront which
> this patch does.
> 
> The patch fixes pv-grub hanging at:
> "
> ******************* FBFRONT for device/vfb/0 **********
> 
> 
> ******************* KBDFRONT for device/vkbd/0 **********
> " and makes it now go to:
> "
> ******************* FBFRONT for device/vfb/0 **********
> 
> 
> ******************* KBDFRONT for device/vkbd/0 **********
> 
> 
> backend at /local/domain/0/backend/vkbd/11/0
> /local/domain/0/backend/vkbd/11/0 connected
> ************************** KBDFRONT
> Thread "kbdfront" exited.
> backend at /local/domain/0/backend/vfb/11/0
> /local/domain/0/backend/vfb/11/0 connected
> "
> and you use VNC to see the GRUB menu.
> 
> diff -r a24dbfcbdf69 -r 76e7d0258f65 extras/mini-os/fbfront.c
> --- a/extras/mini-os/fbfront.c	Tue Jun 22 07:19:38 2010 +0100
> +++ b/extras/mini-os/fbfront.c	Wed Jun 30 10:29:42 2010 -0400
> @@ -124,7 +124,12 @@
>      }
>  
>      snprintf(path, sizeof(path), "%s/state", nodename);
> -    err = xenbus_switch_state(xbt, path, XenbusStateInitialised);
> +    /* In the past we would have made this Initialized. But with a QEMU
> +     * update 805ed3b that requires the frontend to be Connected state
> +     * to progress the backend to move from InitWait to Connected.
> +     * The QEMU bug fix was meant _only_ for XenFB but it affects every
> +     * device. */
> +    err = xenbus_switch_state(xbt, path, XenbusStateConnected);
>      if (err) {
>          printk("error writing initialized: %s\n", err);
>          free(err);
> @@ -485,7 +490,12 @@
>      }
>  
>      snprintf(path, sizeof(path), "%s/state", nodename);
> -    err = xenbus_switch_state(xbt, path, XenbusStateInitialised);
> +    /* In the past we would have made this Initialized. But with a QEMU
> +     * update 805ed3b that requires the frontend to be Connected state
> +     * to progress the backend to move from InitWait to Connected.
> +     * The QEMU bug fix was meant _only_ for XenFB but it affects every
> +     * device. */
> +    err = xenbus_switch_state(xbt, path, XenbusStateConnected);
>      if (err) {
>          message = "switching state";
>          goto abort_transaction;
> diff -r a24dbfcbdf69 -r 76e7d0258f65 extras/mini-os/pcifront.c
> --- a/extras/mini-os/pcifront.c	Tue Jun 22 07:19:38 2010 +0100
> +++ b/extras/mini-os/pcifront.c	Wed Jun 30 10:29:42 2010 -0400
> @@ -204,7 +204,12 @@
>      }
>  
>      snprintf(path, sizeof(path), "%s/state", nodename);
> -    err = xenbus_switch_state(xbt, path, XenbusStateInitialised);
> +    /* In the past we would have made this Initialized. But with a QEMU
> +     * update 805ed3b that requires the frontend to be Connected state
> +     * to progress the backend to move from InitWait to Connected.
> +     * The QEMU bug fix was meant _only_ for XenFB but it affects every
> +     * device. */
> +    err = xenbus_switch_state(xbt, path, XenbusStateConnected);
>      if (err) {
>          message = "switching state";
>          goto abort_transaction;
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: Xen 3.4.x and request-abs-pointer

[John Haxby]

On 05/07/10 16:45, Stefano Stabellini wrote:
>
> That patch was recently reverted because it was the wrong fix:
>
>    

Rats.

Back to my previous attempt then which seemed a little less elegant: the 
idea was that I would initially register the pointer as relative but 
check in the first call to xenfb_mouse_event() and if I had guessed 
wrong, remove the existing mouse handler and add a new one with 
registered as absolute.  If the mouse turns out to be absolute, we miss 
the very first pointer event but this doesn't seem to be much of an 
issue because everything will be sorted out on the next mouse event (the 
various pieces of code that are interested in whether or not the mouse 
is enabled seem to be OK about switching from relative to absolute).

Before I commit a patch to electrons are the any obvious flaws in that 
approach?

jch

Re: Xen 3.4.x and request-abs-pointer

[Stefano Stabellini]

On Mon, 5 Jul 2010, John Haxby wrote:
> On 05/07/10 16:45, Stefano Stabellini wrote:
> >
> > That patch was recently reverted because it was the wrong fix:
> >
> >    
> 
> Rats.
> 
> Back to my previous attempt then which seemed a little less elegant: the 
> idea was that I would initially register the pointer as relative but 
> check in the first call to xenfb_mouse_event() and if I had guessed 
> wrong, remove the existing mouse handler and add a new one with 
> registered as absolute.  If the mouse turns out to be absolute, we miss 
> the very first pointer event but this doesn't seem to be much of an 
> issue because everything will be sorted out on the next mouse event (the 
> various pieces of code that are interested in whether or not the mouse 
> is enabled seem to be OK about switching from relative to absolute).
> 
> Before I commit a patch to electrons are the any obvious flaws in that 
> approach?
> 
 
I think that solution is not very elegant besides it doesn't fix the
basic issue that is on qemu side.
The proper fix  would be to add a new hook in qemu like suggested
in the previous email:

> In order to do that properly we need a new hook in qemu xen_backend: we
> should probably rename the current connect hook to initialise and create
> a new connect hook that would be implemented by xenfb to read
> request-abs-pointer.

Basically we need a new callback from xen_backend.c on
XenbusStateConnected.
The current "connect" hook can be called on both XenbusStateInitialised
and XenbusStateConnected so it doesn't suit our needs.
I suggest to rename the current "connect" hook to "initialised", and
create a new "connect" hook that is called only on XenbusStateConnected.
In the xenfb.c implementation of the new connect hook you can read
request-abs-pointer and be sure that it was previously set by the guest.

Re: Xen 3.4.x and request-abs-pointer

[John Haxby]

On 05/07/10 17:41, Stefano Stabellini wrote:
> On Mon, 5 Jul 2010, John Haxby wrote:
>    
>> On 05/07/10 16:45, Stefano Stabellini wrote:
>>      
>>> That patch was recently reverted because it was the wrong fix:
>>>
>>>
>>>        
>> Rats.
>>
>> Back to my previous attempt then which seemed a little less elegant: the
>> idea was that I would initially register the pointer as relative but
>> check in the first call to xenfb_mouse_event() and if I had guessed
>> wrong, remove the existing mouse handler and add a new one with
>> registered as absolute.  If the mouse turns out to be absolute, we miss
>> the very first pointer event but this doesn't seem to be much of an
>> issue because everything will be sorted out on the next mouse event (the
>> various pieces of code that are interested in whether or not the mouse
>> is enabled seem to be OK about switching from relative to absolute).
>>
>> Before I commit a patch to electrons are the any obvious flaws in that
>> approach?
>>
>>      
>
> I think that solution is not very elegant besides it doesn't fix the
> basic issue that is on qemu side.
>    

Yeah, you're right.  It sucks.  That's what comes of writing down 
something random just before you go home for the day.

> The proper fix  would be to add a new hook in qemu like suggested
> in the previous email:
>
>    
>> In order to do that properly we need a new hook in qemu xen_backend: we
>> should probably rename the current connect hook to initialise and create
>> a new connect hook that would be implemented by xenfb to read
>> request-abs-pointer.
>>      
> Basically we need a new callback from xen_backend.c on
> XenbusStateConnected.
> The current "connect" hook can be called on both XenbusStateInitialised
> and XenbusStateConnected so it doesn't suit our needs.
> I suggest to rename the current "connect" hook to "initialised", and
> create a new "connect" hook that is called only on XenbusStateConnected.
> In the xenfb.c implementation of the new connect hook you can read
> request-abs-pointer and be sure that it was previously set by the guest.
>
>    

Sounds reasonable.

jch