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

[Stefan Berger]

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

> Ross