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 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.

Ross