From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Both state (XenbusStateClosed) and online (0) are expected by
toolstack/xl devd to completely destroy the device. But "offline"
is never being set by the backend resulting in timeout during
domain destruction, garbage in Xestore and still running Qemu
instance.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 hw/xen/xen-bus.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 75474d4b43..6e7ec3af64 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -519,6 +519,10 @@ static void xen_device_backend_changed(void *opaque, const char *path)
         xen_device_backend_set_state(xendev, XenbusStateClosed);
     }
 
+    if (xen_device_backend_get_state(xendev) == XenbusStateClosed) {
+        xen_device_backend_set_online(xendev, false);
+    }
+
     /*
      * If a backend is still 'online' then we should leave it alone but,
      * if a backend is not 'online', then the device is a candidate
-- 
2.42.0

On 10/11/2023 15:42, Volodymyr Babchuk wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Both state (XenbusStateClosed) and online (0) are expected by
> toolstack/xl devd to completely destroy the device. But "offline"
> is never being set by the backend resulting in timeout during
> domain destruction, garbage in Xestore and still running Qemu
> instance.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>   hw/xen/xen-bus.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> index 75474d4b43..6e7ec3af64 100644
> --- a/hw/xen/xen-bus.c
> +++ b/hw/xen/xen-bus.c
> @@ -519,6 +519,10 @@ static void xen_device_backend_changed(void *opaque, const char *path)
>           xen_device_backend_set_state(xendev, XenbusStateClosed);
>       }
>   
> +    if (xen_device_backend_get_state(xendev) == XenbusStateClosed) {
> +        xen_device_backend_set_online(xendev, false);
> +    }
> +
>       /*
>        * If a backend is still 'online' then we should leave it alone but,
>        * if a backend is not 'online', then the device is a candidate

I don't understand what you're trying to do here. Just a few lines up 
from this hunk there is:

  506    if (xen_device_backend_scanf(xendev, "online", "%u", &online) 
!= 1) {
  507        online = 0;
  508    }
  509
  510    xen_device_backend_set_online(xendev, !!online);

Why is this not sufficient? What happens if the frontend decides to stop 
and start (e.g. for a driver update)? I'm guessing the backend will be 
destroyed... which is not very friendly.

   Paul

(When sending a series, if you Cc someone on one message please could you Cc them on the whole thread for context? Thanks.)

> Both state (XenbusStateClosed) and online (0) are expected by
> toolstack/xl devd to completely destroy the device. But "offline"
> is never being set by the backend resulting in timeout during
> domain destruction, garbage in Xestore and still running Qemu
> instance.

I would dearly love to see a clear state diagram showing all the possible state transitions during device creation, fe/be reconnecting, and hot plug/unplug. Does anything like that exist? Any test cases for the common and the less common transitions, and even the illegal ones which need to be handled gracefully?

Fantasy aside, can you please confirm that this patch series was tested with hotplug (device_add in the monitor) *and* unplug in QEMU, both under real Xen and ideally also emulated Xen in KVM? 

Thanks.