| Message ID | 20230713035232.48406-10-j@getutm.app |
|---|---|
| State | New |
| Headers | show |
| Series | tpm: introduce TPM CRB SysBus device | expand |
On 7/12/23 23:51, Joelle van Dyne wrote: > If 'ppi' property is set, then `tpm_ppi_reset` is called on reset > which SEGFAULTs because `tpmppi->buf` is not allocated. > > Signed-off-by: Joelle van Dyne <j@getutm.app> > --- > hw/tpm/tpm_tis_sysbus.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c > index 45e63efd63..1014d5d993 100644 > --- a/hw/tpm/tpm_tis_sysbus.c > +++ b/hw/tpm/tpm_tis_sysbus.c > @@ -124,6 +124,10 @@ static void tpm_tis_sysbus_realizefn(DeviceState *dev, Error **errp) > error_setg(errp, "'tpmdev' property is required"); > return; > } > + > + if (s->ppi_enabled) { > + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->ppi.ram); > + } > } > The tpm-tis-device doesn't work for x86_64 but for aarch64. We have this here in this file: DEFINE_PROP_BOOL("ppi", TPMStateSysBus, state.ppi_enabled, false), I don't know whether ppi would work on aarch64. It needs firmware support like in edk2. I think the best solution is to remove this DEFINE_PROP_BOOL() and if someone wants to enable it they would have to add firmware support and test it before re-enabling it. Stefan > static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data)
On Thu, Jul 13, 2023 at 9:49 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > The tpm-tis-device doesn't work for x86_64 but for aarch64. > > > We have this here in this file: > > DEFINE_PROP_BOOL("ppi", TPMStateSysBus, state.ppi_enabled, false), > > I don't know whether ppi would work on aarch64. It needs firmware support like in edk2. > I think the best solution is to remove this DEFINE_PROP_BOOL() and if someone wants > to enable it they would have to add firmware support and test it before re-enabling it. > > Stefan > > > static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data) Yeah, I'm not sure if PPI works with AARCH64 since I didn't bother to change it to not use hard coded addresses. However, isn't that "ppi" overridable from the command line? If so, should we add a check in "realize" to error if PPI=true? Otherwise, it will just crash.
On 7/13/23 14:15, Joelle van Dyne wrote: > On Thu, Jul 13, 2023 at 9:49 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> The tpm-tis-device doesn't work for x86_64 but for aarch64. >> >> >> We have this here in this file: >> >> DEFINE_PROP_BOOL("ppi", TPMStateSysBus, state.ppi_enabled, false), >> >> I don't know whether ppi would work on aarch64. It needs firmware support like in edk2. >> I think the best solution is to remove this DEFINE_PROP_BOOL() and if someone wants >> to enable it they would have to add firmware support and test it before re-enabling it. >> >> Stefan >> >>> static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data) > > Yeah, I'm not sure if PPI works with AARCH64 since I didn't bother to > change it to not use hard coded addresses. However, isn't that "ppi" > overridable from the command line? If so, should we add a check in > "realize" to error if PPI=true? Otherwise, it will just crash. Once the option is removed via my patch (cc'ed you), then you get this once you pass ppi=on on the command line: qemu-system-aarch64: -device tpm-tis-device,tpmdev=tpm0,ppi=on: Property 'tpm-tis-device.ppi' not found This disables it for good. Stefan
diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c index 45e63efd63..1014d5d993 100644 --- a/hw/tpm/tpm_tis_sysbus.c +++ b/hw/tpm/tpm_tis_sysbus.c @@ -124,6 +124,10 @@ static void tpm_tis_sysbus_realizefn(DeviceState *dev, Error **errp) error_setg(errp, "'tpmdev' property is required"); return; } + + if (s->ppi_enabled) { + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->ppi.ram); + } } static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data)
If 'ppi' property is set, then `tpm_ppi_reset` is called on reset which SEGFAULTs because `tpmppi->buf` is not allocated. Signed-off-by: Joelle van Dyne <j@getutm.app> --- hw/tpm/tpm_tis_sysbus.c | 4 ++++ 1 file changed, 4 insertions(+)