[PATCH] qemu: Fix libvirt hang due to early TPM device stop

[Daniel P. Berrangé]

On Fri, Feb 19, 2021 at 09:49:36AM -0500, Stefan Berger wrote:
> This patch partially reverts commit 5cde9dee where the qemuExtDevicesStop()
> was moved to a location before the QEMU process is stopped. It may be
> alright to tear down some devices before QEMU is stopped, but it doesn't work
> for the external TPM (swtpm) which assumes that QEMU sends it a signal to stop
> it before libvirt may try to clean it up. So this patch introduces a function
> qemuExtDevicesStopEarly() to accommodate those devices that can/must
> be torn down early, which seems to be the case with virtio and its virtiofsd,
> and recreates the location of the call to qemuExtDevicesStop() from before the
> regression for all other devices.

I don't think splitting the cleanup like this is a good idea.

Also I think 5cde9dee was reall wrong, because we shouldn't tear
down /any/ external processes while QEMU itself still exists.

The comit message for 5cde9dee talks aboutneeding to run before
virFileDeleteTree(priv->libDir) which makes sense. The solution
was wrong though IMHO.

We shouldn't be calling virFileDeleteTree() until all the processes
are stopped.

So we should revert 5cde9dee entirely, and then move the
virFileDeleteTree calls further to the end of qemuProcessStop

> 
> Fixes: 5cde9dee8c70b17c458d031ab6cf71dce476eea2
> Cc: Masayoshi Mizuma <m.mizuma at jp.fujitsu.com>
> Signed-off-by: Stefan Berger <stefanb at linux.ibm.com>
> ---
>  src/qemu/qemu_extdevice.c | 27 ++++++++++++++++++++-------
>  src/qemu/qemu_extdevice.h |  4 ++++
>  src/qemu/qemu_process.c   |  5 ++++-
>  3 files changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
> index 8fe7ceaa10..96fdc9a589 100644
> --- a/src/qemu/qemu_extdevice.c
> +++ b/src/qemu/qemu_extdevice.c
> @@ -205,7 +205,27 @@ qemuExtDevicesStart(virQEMUDriverPtr driver,
>      return 0;
>  }
>  
> +/* qemuExtDevicesStopEarly stops devices that may be stopped
> + * before QEMU terminates
> + */
> +void
> +qemuExtDevicesStopEarly(virQEMUDriverPtr driver,
> +                        virDomainObjPtr vm)
> +{
> +    virDomainDefPtr def = vm->def;
> +    size_t i;
>  
> +    for (i = 0; i < def->nfss; i++) {
> +        virDomainFSDefPtr fs = def->fss[i];
> +
> +        if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS)
> +            qemuVirtioFSStop(driver, vm, fs);
> +    }
> +}
> +
> +/* qemuExtDevicesStop stops devices that may only be stopped
> + * after QEMU terminated
> + */
>  void
>  qemuExtDevicesStop(virQEMUDriverPtr driver,
>                     virDomainObjPtr vm)
> @@ -236,13 +256,6 @@ qemuExtDevicesStop(virQEMUDriverPtr driver,
>          if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET && net->downscript)
>              virNetDevRunEthernetScript(net->ifname, net->downscript);
>      }
> -
> -    for (i = 0; i < def->nfss; i++) {
> -        virDomainFSDefPtr fs = def->fss[i];
> -
> -        if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS)
> -            qemuVirtioFSStop(driver, vm, fs);
> -    }
>  }
>  
>  
> diff --git a/src/qemu/qemu_extdevice.h b/src/qemu/qemu_extdevice.h
> index 49373a15a1..df0cbd6993 100644
> --- a/src/qemu/qemu_extdevice.h
> +++ b/src/qemu/qemu_extdevice.h
> @@ -51,6 +51,10 @@ int qemuExtDevicesStart(virQEMUDriverPtr driver,
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
>      G_GNUC_WARN_UNUSED_RESULT;
>  
> +void qemuExtDevicesStopEarly(virQEMUDriverPtr driver,
> +                             virDomainObjPtr vm)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +
>  void qemuExtDevicesStop(virQEMUDriverPtr driver,
>                          virDomainObjPtr vm)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 7feb35e609..b5b2ddef4c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -7659,7 +7659,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>      /* Do this before we delete the tree and remove pidfile. */
>      qemuProcessKillManagedPRDaemon(vm);
>  
> -    qemuExtDevicesStop(driver, vm);
> +    qemuExtDevicesStopEarly(driver, vm);
>  
>      virFileDeleteTree(priv->libDir);
>      virFileDeleteTree(priv->channelTargetDir);
> @@ -7677,6 +7677,9 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>  
>      qemuDomainCleanupRun(driver, vm);
>  
> +    /* tear down external devices after QEMU is gone */
> +    qemuExtDevicesStop(driver, vm);
> +
>      qemuDBusStop(driver, vm);
>  
>      vm->def->id = -1;
> -- 
> 2.26.2
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|