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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Jul-05 15:45 UTC
Re: [Xen-devel] Xen 3.4.x and request-abs-pointer
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 2010Date: 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Jul-05 16:41 UTC
Re: [Xen-devel] Xen 3.4.x and request-abs-pointer
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel