* [PATCH] tpm_crb: Avoid backend startup just before shutdown under Xen
@ 2022-08-26 14:38 Ross Lagerwall via
2022-08-26 15:20 ` Stefan Berger
0 siblings, 1 reply; 6+ messages in thread
From: Ross Lagerwall via @ 2022-08-26 14:38 UTC (permalink / raw)
To: Stefan Berger; +Cc: qemu-devel, Ross Lagerwall
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)
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tpm_crb: Avoid backend startup just before shutdown under Xen
2022-08-26 14:38 [PATCH] tpm_crb: Avoid backend startup just before shutdown under Xen Ross Lagerwall via
@ 2022-08-26 15:20 ` Stefan Berger
2022-08-26 16:15 ` Ross Lagerwall
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Berger @ 2022-08-26 15:20 UTC (permalink / raw)
To: Ross Lagerwall, Stefan Berger; +Cc: qemu-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tpm_crb: Avoid backend startup just before shutdown under Xen
2022-08-26 15:20 ` Stefan Berger
@ 2022-08-26 16:15 ` Ross Lagerwall
2022-08-26 16:27 ` Stefan Berger
0 siblings, 1 reply; 6+ messages in thread
From: Ross Lagerwall @ 2022-08-26 16:15 UTC (permalink / raw)
To: Stefan Berger, Stefan Berger; +Cc: qemu-devel
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tpm_crb: Avoid backend startup just before shutdown under Xen
2022-08-26 16:15 ` Ross Lagerwall
@ 2022-08-26 16:27 ` Stefan Berger
2022-08-30 13:51 ` Ross Lagerwall
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Berger @ 2022-08-26 16:27 UTC (permalink / raw)
To: Ross Lagerwall, Stefan Berger; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tpm_crb: Avoid backend startup just before shutdown under Xen
2022-08-26 16:27 ` Stefan Berger
@ 2022-08-30 13:51 ` Ross Lagerwall
2022-08-30 14:15 ` Stefan Berger
0 siblings, 1 reply; 6+ messages in thread
From: Ross Lagerwall @ 2022-08-30 13:51 UTC (permalink / raw)
To: Stefan Berger, Stefan Berger; +Cc: qemu-devel
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tpm_crb: Avoid backend startup just before shutdown under Xen
2022-08-30 13:51 ` Ross Lagerwall
@ 2022-08-30 14:15 ` Stefan Berger
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Berger @ 2022-08-30 14:15 UTC (permalink / raw)
To: Ross Lagerwall, Stefan Berger; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-08-30 14:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-26 14:38 [PATCH] tpm_crb: Avoid backend startup just before shutdown under Xen Ross Lagerwall via
2022-08-26 15:20 ` Stefan Berger
2022-08-26 16:15 ` Ross Lagerwall
2022-08-26 16:27 ` Stefan Berger
2022-08-30 13:51 ` Ross Lagerwall
2022-08-30 14:15 ` Stefan Berger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.