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

[Stefan Berger <stefanb@linux.ibm.com>]

On 8/30/22 09:51, Ross Lagerwall wrote:
>> 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?

Yes, I am fine with it.

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 
> Thanks,
> Ross