Re: [PATCH] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
On 7/14/23 09:51, Eric Auger wrote: Hi Stefan, On 7/14/23 13:51, Stefan Berger wrote: On 7/14/23 02:07, Joelle van Dyne wrote: On Thu, Jul 13, 2023 at 10:20 AM Stefan Berger wrote: The ppi command line option for the TIS device on sysbus never worked and caused an immediate segfault. Remove support for it since it also needs support in the firmware and needs testing inside the VM. Reproducer with the ppi=on option passed: qemu-system-aarch64 \ -machine virt,gic-version=3 \ -m 4G \ -nographic -no-acpi \ -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \ -tpmdev emulator,id=tpm0,chardev=chrtpm \ -device tpm-tis-device,tpmdev=tpm0,ppi=on [...] Segmentation fault (core dumped) Signed-off-by: Stefan Berger Do you need to add a VMSTATE_UNUSED_TEST in case a future QEMU version introduces a new field in the same position which will cause an issue when restoring from an older version? Hm, you got a point there. We will have to error-out in case someone sets ppi=on instead since the expectation that PPI would work is simply not there. v2 coming soon. as Joelle pointed it out ppi_enabled is not part of vmstate_tpm_tis_sysbus fields. And since it has never worked I suspect we cannot have any existing VM enabling it. So I don't get the issue with this 1st version? You are right. I repeated my test with restoring state of a VM taken before the removal of this field and it restored it. So that other patch is good and I am withdrawing this patch here. Stefan Thanks Eric Stefan
Re: [PATCH] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
Hi Stefan, On 7/14/23 13:51, Stefan Berger wrote: > > > On 7/14/23 02:07, Joelle van Dyne wrote: >> On Thu, Jul 13, 2023 at 10:20 AM Stefan Berger >> wrote: >>> >>> The ppi command line option for the TIS device on sysbus never worked >>> and caused an immediate segfault. Remove support for it since it also >>> needs support in the firmware and needs testing inside the VM. >>> >>> Reproducer with the ppi=on option passed: >>> >>> qemu-system-aarch64 \ >>> -machine virt,gic-version=3 \ >>> -m 4G \ >>> -nographic -no-acpi \ >>> -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \ >>> -tpmdev emulator,id=tpm0,chardev=chrtpm \ >>> -device tpm-tis-device,tpmdev=tpm0,ppi=on >>> [...] >>> Segmentation fault (core dumped) >>> >>> Signed-off-by: Stefan Berger >> >> Do you need to add a VMSTATE_UNUSED_TEST in case a future QEMU version >> introduces a new field in the same position which will cause an issue >> when restoring from an older version? > > Hm, you got a point there. We will have to error-out in case someone > sets ppi=on instead since the expectation that PPI would work is > simply not there. v2 coming soon. as Joelle pointed it out ppi_enabled is not part of vmstate_tpm_tis_sysbus fields. And since it has never worked I suspect we cannot have any existing VM enabling it. So I don't get the issue with this 1st version? Thanks Eric > > Stefan >
Re: [PATCH] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
On 7/14/23 02:07, Joelle van Dyne wrote: On Thu, Jul 13, 2023 at 10:20 AM Stefan Berger wrote: The ppi command line option for the TIS device on sysbus never worked and caused an immediate segfault. Remove support for it since it also needs support in the firmware and needs testing inside the VM. Reproducer with the ppi=on option passed: qemu-system-aarch64 \ -machine virt,gic-version=3 \ -m 4G \ -nographic -no-acpi \ -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \ -tpmdev emulator,id=tpm0,chardev=chrtpm \ -device tpm-tis-device,tpmdev=tpm0,ppi=on [...] Segmentation fault (core dumped) Signed-off-by: Stefan Berger Do you need to add a VMSTATE_UNUSED_TEST in case a future QEMU version introduces a new field in the same position which will cause an issue when restoring from an older version? Hm, you got a point there. We will have to error-out in case someone sets ppi=on instead since the expectation that PPI would work is simply not there. v2 coming soon. Stefan
[PATCH] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
The ppi command line option for the TIS device on sysbus never worked
and caused an immediate segfault. Since it is part of the state of
a VM we cannot remove it but have to intercept ppi_enabled set to
true and display an error instead.
Reproducer with the ppi=on option passed:
qemu-system-aarch64 \
-machine virt,gic-version=3 \
-m 4G \
-nographic -no-acpi \
-chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm \
-device tpm-tis-device,tpmdev=tpm0,ppi=on
[...]
Segmentation fault (core dumped)
Signed-off-by: Stefan Berger
---
hw/tpm/tpm_tis_sysbus.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
index 45e63efd63..4319d31c88 100644
--- a/hw/tpm/tpm_tis_sysbus.c
+++ b/hw/tpm/tpm_tis_sysbus.c
@@ -124,6 +124,11 @@ static void tpm_tis_sysbus_realizefn(DeviceState *dev,
Error **errp)
error_setg(errp, "'tpmdev' property is required");
return;
}
+
+if (s->ppi_enabled) {
+error_setg(errp, "'ppi=on' is not supported by this device");
+return;
+}
}
static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data)
--
2.41.0
Re: [PATCH] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
On Thu, Jul 13, 2023 at 11:07 PM Joelle van Dyne wrote: > > On Thu, Jul 13, 2023 at 10:20 AM Stefan Berger wrote: > > > > The ppi command line option for the TIS device on sysbus never worked > > and caused an immediate segfault. Remove support for it since it also > > needs support in the firmware and needs testing inside the VM. > > > > Reproducer with the ppi=on option passed: > > > > qemu-system-aarch64 \ > >-machine virt,gic-version=3 \ > >-m 4G \ > >-nographic -no-acpi \ > >-chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \ > >-tpmdev emulator,id=tpm0,chardev=chrtpm \ > >-device tpm-tis-device,tpmdev=tpm0,ppi=on > > [...] > > Segmentation fault (core dumped) > > > > Signed-off-by: Stefan Berger > > Do you need to add a VMSTATE_UNUSED_TEST in case a future QEMU version > introduces a new field in the same position which will cause an issue > when restoring from an older version? Actually, ignore that last message. I misread the patch thinking it's on the VMState.
Re: [PATCH] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
On Thu, Jul 13, 2023 at 10:20 AM Stefan Berger wrote: > > The ppi command line option for the TIS device on sysbus never worked > and caused an immediate segfault. Remove support for it since it also > needs support in the firmware and needs testing inside the VM. > > Reproducer with the ppi=on option passed: > > qemu-system-aarch64 \ >-machine virt,gic-version=3 \ >-m 4G \ >-nographic -no-acpi \ >-chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \ >-tpmdev emulator,id=tpm0,chardev=chrtpm \ >-device tpm-tis-device,tpmdev=tpm0,ppi=on > [...] > Segmentation fault (core dumped) > > Signed-off-by: Stefan Berger Do you need to add a VMSTATE_UNUSED_TEST in case a future QEMU version introduces a new field in the same position which will cause an issue when restoring from an older version?
Re: [PATCH] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
Hi Stefan,
On 7/13/23 19:19, Stefan Berger wrote:
> The ppi command line option for the TIS device on sysbus never worked
> and caused an immediate segfault. Remove support for it since it also
> needs support in the firmware and needs testing inside the VM.
>
> Reproducer with the ppi=on option passed:
>
> qemu-system-aarch64 \
>-machine virt,gic-version=3 \
>-m 4G \
>-nographic -no-acpi \
>-chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
>-tpmdev emulator,id=tpm0,chardev=chrtpm \
>-device tpm-tis-device,tpmdev=tpm0,ppi=on
> [...]
> Segmentation fault (core dumped)
>
> Signed-off-by: Stefan Berger
Reviewed-by: Eric Auger
Thanks!
Eric
> ---
> hw/tpm/tpm_tis_sysbus.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
> index 45e63efd63..6724b3d4f6 100644
> --- a/hw/tpm/tpm_tis_sysbus.c
> +++ b/hw/tpm/tpm_tis_sysbus.c
> @@ -93,7 +93,6 @@ static void tpm_tis_sysbus_reset(DeviceState *dev)
> static Property tpm_tis_sysbus_properties[] = {
> DEFINE_PROP_UINT32("irq", TPMStateSysBus, state.irq_num, TPM_TIS_IRQ),
> DEFINE_PROP_TPMBE("tpmdev", TPMStateSysBus, state.be_driver),
> -DEFINE_PROP_BOOL("ppi", TPMStateSysBus, state.ppi_enabled, false),
> DEFINE_PROP_END_OF_LIST(),
> };
>
[PATCH] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
The ppi command line option for the TIS device on sysbus never worked
and caused an immediate segfault. Remove support for it since it also
needs support in the firmware and needs testing inside the VM.
Reproducer with the ppi=on option passed:
qemu-system-aarch64 \
-machine virt,gic-version=3 \
-m 4G \
-nographic -no-acpi \
-chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm \
-device tpm-tis-device,tpmdev=tpm0,ppi=on
[...]
Segmentation fault (core dumped)
Signed-off-by: Stefan Berger
---
hw/tpm/tpm_tis_sysbus.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
index 45e63efd63..6724b3d4f6 100644
--- a/hw/tpm/tpm_tis_sysbus.c
+++ b/hw/tpm/tpm_tis_sysbus.c
@@ -93,7 +93,6 @@ static void tpm_tis_sysbus_reset(DeviceState *dev)
static Property tpm_tis_sysbus_properties[] = {
DEFINE_PROP_UINT32("irq", TPMStateSysBus, state.irq_num, TPM_TIS_IRQ),
DEFINE_PROP_TPMBE("tpmdev", TPMStateSysBus, state.be_driver),
-DEFINE_PROP_BOOL("ppi", TPMStateSysBus, state.ppi_enabled, false),
DEFINE_PROP_END_OF_LIST(),
};
--
2.41.0
