[Qemu-devel] [PATCH v2] xen: fix xen-bus state model to allow frontend re-connection

[Qemu-devel] [PATCH v2] xen: fix xen-bus state model to allow frontend re-connection

[Paul Durrant]

There is a flaw in the xen-bus state model. To allow a frontend to re-
connect the backend state of an online XenDevice is transitioned from
Closed to InitWait, but this is currently done unilaterally which is
incorrect. The backend state should remain Closed until the frontend state
transitions to Initialising.

This patch removes the automatic backend state transition from
xen_device_backend_state_changed() and, instead, adds an extra check in
xen_device_frontend_state_changed() to determine whether a frontend is
trying to re-connect to a previously Closed XenDevice. Only if this is
found to be the case is the backend state transitioned from Closed to
InitWait. Note that this transition will be common amongst all XenDevice
classes and hence xen_device_frontend_state_changed() returns immediately
afterwards without calling into the XenDeviceClass frontend_changed()
method.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>

v2:
 - Fix xl block-detach
---
 hw/xen/xen-bus.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 3aeccec69c..49a725e8c7 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -547,20 +547,15 @@ static void xen_device_backend_changed(void *opaque)
     }
 
     /*
-     * If a backend is still 'online' then its state should be cycled
-     * back round to InitWait in order for a new frontend instance to
-     * connect. This may happen when, for example, a frontend driver is
-     * re-installed or updated.
-     * If a backend is not 'online' then the device should be destroyed.
+     * If a backend is still 'online' then we should leave it alone but,
+     * if a backend is not 'online', then the device should be destroyed
+     * once the state is Closed.
      */
-    if (xendev->backend_online &&
-        xendev->backend_state == XenbusStateClosed) {
-        xen_device_backend_set_state(xendev, XenbusStateInitWait);
-    } else if (!xendev->backend_online &&
-               (xendev->backend_state == XenbusStateClosed ||
-                xendev->backend_state == XenbusStateInitialising ||
-                xendev->backend_state == XenbusStateInitWait ||
-                xendev->backend_state == XenbusStateUnknown)) {
+    if (!xendev->backend_online &&
+        (xendev->backend_state == XenbusStateClosed ||
+         xendev->backend_state == XenbusStateInitialising ||
+         xendev->backend_state == XenbusStateInitWait ||
+         xendev->backend_state == XenbusStateUnknown)) {
         Error *local_err = NULL;
 
         if (!xen_backend_try_device_destroy(xendev, &local_err)) {
@@ -715,6 +710,17 @@ static void xen_device_frontend_changed(void *opaque)
 
     xen_device_frontend_set_state(xendev, state);
 
+    if (state == XenbusStateInitialising &&
+        xendev->backend_state == XenbusStateClosed &&
+        xendev->backend_online) {
+        /*
+         * The frontend is re-initializing so switch back to
+         * InitWait.
+         */
+        xen_device_backend_set_state(xendev, XenbusStateInitWait);
+        return;
+    }
+
     if (xendev_class->frontend_changed) {
         Error *local_err = NULL;
 
-- 
2.20.1.2.gb21ebb6

Re: [Qemu-devel] [PATCH v2] xen: fix xen-bus state model to allow frontend re-connection

[Anthony PERARD]

On Tue, Jan 22, 2019 at 03:53:46PM +0000, Paul Durrant wrote:
> There is a flaw in the xen-bus state model. To allow a frontend to re-
> connect the backend state of an online XenDevice is transitioned from
> Closed to InitWait, but this is currently done unilaterally which is
> incorrect. The backend state should remain Closed until the frontend state
> transitions to Initialising.
> 
> This patch removes the automatic backend state transition from
> xen_device_backend_state_changed() and, instead, adds an extra check in
> xen_device_frontend_state_changed() to determine whether a frontend is
> trying to re-connect to a previously Closed XenDevice. Only if this is
> found to be the case is the backend state transitioned from Closed to
> InitWait. Note that this transition will be common amongst all XenDevice
> classes and hence xen_device_frontend_state_changed() returns immediately
> afterwards without calling into the XenDeviceClass frontend_changed()
> method.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

I've tested OVMF with that patch, and states transitions looks better
when transitionning from ovmf to linux.
(Less Closed->InitWait->Closed..., and ovmf trying to win a race at
reading the backend state at the right time).

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD