Re: [PATCH] tpm_crb: Avoid backend startup just before shutdown under Xen

[Ross Lagerwall]

> From: Stefan Berger <stefanb@linux.ibm.com>
> Sent: Friday, August 26, 2022 5:27 PM
> To: Ross Lagerwall <ross.lagerwall@citrix.com>; Stefan Berger 
> <stefanb@linux.vnet.ibm.com>
> Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>
> Subject: Re: [PATCH] tpm_crb: Avoid backend startup just before shutdown 
> under Xen 
>  
> On 8/26/22 12:15, Ross Lagerwall wrote:
> >> From: Stefan Berger <stefanb@linux.ibm.com>
> >> Sent: Friday, August 26, 2022 4:20 PM
> >> To: Ross Lagerwall <ross.lagerwall@citrix.com>; Stefan Berger 
> >> <stefanb@linux.vnet.ibm.com>
> >> Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>
> >> Subject: Re: [PATCH] tpm_crb: Avoid backend startup just before shutdown 
> >> under Xen
> >>   
> >> On 8/26/22 10:38, Ross Lagerwall wrote:
> >>> When running under Xen and the guest reboots, it boots into a new domain
> >>> with a new QEMU process (and a new swtpm process if using the emulator
> >>> backend). The existing reset function is triggered just before the old
> >>> QEMU process exists which causes QEMU to startup the TPM backend and
> >>> then immediately shut it down. This is probably harmless but when using
> >>> the emulated backend, it wastes CPU and IO time reloading state, etc.
> >>>
> >>> Fix this by calling the reset function directly from realize() when
> >>> running under Xen. During a reboot, this will be called by the QEMU
> >>> process for the new domain.
> >>>
> >>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> >>> ---
> >>>
> >>> This conditional logic is ugly. Is there a cleaner way of doing this?
> >>>
> >>>     hw/tpm/tpm_crb.c | 7 ++++++-
> >>>     1 file changed, 6 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> >>> index 67db594c48..ea930da545 100644
> >>> --- a/hw/tpm/tpm_crb.c
> >>> +++ b/hw/tpm/tpm_crb.c
> >>> @@ -26,6 +26,7 @@
> >>>     #include "sysemu/tpm_backend.h"
> >>>     #include "sysemu/tpm_util.h"
> >>>     #include "sysemu/reset.h"
> >>> +#include "sysemu/xen.h"
> >>>     #include "tpm_prop.h"
> >>>     #include "tpm_ppi.h"
> >>>     #include "trace.h"
> >>> @@ -308,7 +309,11 @@ static void tpm_crb_realize(DeviceState *dev, Error 
> >>> **errp)
> >>>                          TPM_PPI_ADDR_BASE, OBJECT(s));
> >>>         }
> >>>
> >>> -    qemu_register_reset(tpm_crb_reset, dev);
> >>> +    if (xen_enabled()) {
> >>> +        tpm_crb_reset(dev);
> >>> +    } else {
> >>> +        qemu_register_reset(tpm_crb_reset, dev);
> >>> +    }
> >>>     }
> >>>
> >>>     static void tpm_crb_class_init(ObjectClass *klass, void *data)
> >>
> >> This doesn't look right also for Xen. Shouldn't it be something like this?
> >>
> >>       qemu_register_reset(tpm_crb_reset, dev);
> >>       if (xen_enabled()) {
> >>          tpm_crb_reset(dev);
> >>       }
> >>
> >>
> >> We need the reset callback for VM reset.
> > 
> > This patch doesn't change anything for the QEMU/KVM case which works
> > fine as is.
> > 
> > In the Xen architecture, the guest is rebooted into a new domain which
> > has new instances of QEMU and swtpm. The old instances are terminated.
> > So during a guest reboot it doesn't make sense to have the QEMU for the
> > old domain call tpm_crb_reset() just as it is about to exit since it
> > causes swtpm to be sent CMD_INIT which causes it to needlessly
> > reinitialize and reload the state. Instead, the new QEMU instance
> > post-reboot will call tpm_crb_reset() to start the backend directly from
> > the realize() function, just as for the initial guest boot.
> 
> 
> You should probably add this to the commit text because I wouldn't have 
> known that a VM reset in Xen causes a new domain to be created...

Hi Stefan,

This is already included at the start of the commit message:

"""
When running under Xen and the guest reboots, it boots into a new domain
with a new QEMU process (and a new swtpm process if using the emulator
backend).
"""

Ignoring the commit message, is the code change acceptable?

Thanks,
Ross