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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: 22 January 2019 14:40
> To: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>
> Subject: [PATCH] xen: fix xen-bus state model to allow frontend re-
> connection
> 
> 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>

Unfortunately this is still not quite right. Further testing has now shown that xl block-detach is no longer working properly. I'll send a v2 once that is fixed.

  Paul

> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> ---
>  hw/xen/xen-bus.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> index 3aeccec69c..1b31a1dc50 100644
> --- a/hw/xen/xen-bus.c
> +++ b/hw/xen/xen-bus.c
> @@ -547,20 +547,12 @@ 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 &&
> +    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)) {
>          Error *local_err = NULL;
> 
>          if (!xen_backend_try_device_destroy(xendev, &local_err)) {
> @@ -715,6 +707,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