On Thu, 18 Jun 2026 at 15:33, Mohammadfaiz Bawa <[email protected]> wrote: > > On Thu, Jun 18, 2026 at 6:39 PM Peter Maydell <[email protected]> > wrote: > > > > On Thu, 18 Jun 2026 at 13:55, Mohammadfaiz Bawa <[email protected]> wrote: > > > > > > Add a "ppi" boolean property (default: true) to tpm-tis-device. > > > When ppi=off the RAMBlock is never registered and the migration > > > stream omits "tpm-ppi", restoring backward compatibility. > > > > > > When disabled, realizefn initializes the region with zero size so > > > the platform bus skips it and ACPI PPI tables are omitted. > > > > > > Fixes: 46cd2c1050f0 ("hw/tpm: add PPI support to tpm-tis-device for ARM64 > > > virt") > > > Signed-off-by: Mohammadfaiz Bawa <[email protected]> > > > --- > > > hw/tpm/tpm_tis.h | 1 + > > > hw/tpm/tpm_tis_sysbus.c | 16 +++++++++++----- > > > 2 files changed, 12 insertions(+), 5 deletions(-) > > > > > > diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h > > > index 1531620bf9..6ac9804050 100644 > > > --- a/hw/tpm/tpm_tis.h > > > +++ b/hw/tpm/tpm_tis.h > > > @@ -76,6 +76,7 @@ typedef struct TPMState { > > > size_t be_buffer_size; > > > > > > TPMPPI ppi; > > > + bool ppi_enabled; > > > } TPMState; > > > > > > extern const VMStateDescription vmstate_locty; > > > diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c > > > index 16bb17874b..54691ac272 100644 > > > --- a/hw/tpm/tpm_tis_sysbus.c > > > +++ b/hw/tpm/tpm_tis_sysbus.c > > > @@ -94,6 +94,7 @@ static void tpm_tis_sysbus_reset(DeviceState *dev) > > > static const 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, true), > > > }; > > > > > > static void tpm_tis_sysbus_initfn(Object *obj) > > > @@ -122,14 +123,19 @@ static void tpm_tis_sysbus_realizefn(DeviceState > > > *dev, Error **errp) > > > return; > > > } > > > > > > - s->ppi.buf = qemu_memalign(host_page_size, > > > - ROUND_UP(TPM_PPI_ADDR_SIZE, > > > host_page_size)); > > > memory_region_init_io(&s->mmio, OBJECT(dev), &tpm_tis_memory_ops, > > > s, "tpm-tis-mmio", > > > TPM_TIS_NUM_LOCALITIES << > > > TPM_TIS_LOCALITY_SHIFT); > > > - memory_region_init_ram_device_ptr(&s->ppi.ram, OBJECT(dev), > > > "tpm-ppi", > > > - TPM_PPI_ADDR_SIZE, s->ppi.buf); > > > - vmstate_register_ram(&s->ppi.ram, dev); > > > + > > > + if (s->ppi_enabled) { > > > + s->ppi.buf = qemu_memalign(host_page_size, > > > + ROUND_UP(TPM_PPI_ADDR_SIZE, > > > host_page_size)); > > > + memory_region_init_ram_device_ptr(&s->ppi.ram, OBJECT(dev), > > > "tpm-ppi", > > > + TPM_PPI_ADDR_SIZE, s->ppi.buf); > > > + vmstate_register_ram(&s->ppi.ram, dev); > > > + } else { > > > + memory_region_init(&s->ppi.ram, OBJECT(dev), "tpm-ppi", 0); > > > + } > > > > Is it valid to create a 0-size memory region? I think the more > > usual approach here would be to not expose the 2nd MMIO region > > if the device was configured with PPI disabled. > > > > This device also very weirdly calls sysbus_init_mmio() in > > its initfn, before it has actually set up the MR, which it > > is doing here in realize. I think that also needs to be fixed > > (especially since the property will only be available in > > realize). It happens to work since all sysbus_init_mmio() > > is doing is putting the pointer into its data structure, but > > it doesn't make much logical sense to do it that way around.
> Agreed. I initially considered not exposing the 2nd MMIO region > at all when PPI is disabled, but I followed the existing > pattern in initfn. You're right that it's cleaner to just not register it. > > I'll move both sysbus_init_mmio() calls to realizefn so the MR are > properly initialized before being registered, and only expose the PPI > region when ppi=on. > > Is it fine to include this refactoring (moving sysbus_init_mmio() to > realizefn) > in this same series, or would it be preferable as a separate patch? Either is OK, I think -- it's only moving two lines of code. -- PMM
