[v2,07/11] hw/arm/virt: add plug handler for TPM on SysBus

TPM needs to know its own base address in order to generate its DSDT
device entry.

Signed-off-by: Joelle van Dyne <j@getutm.app>
---
 hw/arm/virt.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

On 7/14/23 03:09, Joelle van Dyne wrote:
> TPM needs to know its own base address in order to generate its DSDT
> device entry.
> 
> Signed-off-by: Joelle van Dyne <j@getutm.app>
> ---
>   hw/arm/virt.c | 37 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 37 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 7d9dbc2663..432148ef47 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2732,6 +2732,37 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
>                            dev, &error_abort);
>   }
> 
> +#ifdef CONFIG_TPM
> +static void virt_tpm_plug(VirtMachineState *vms, TPMIf *tpmif)
> +{
> +    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
> +    hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base;

+static void virt_tpm_plug(LoongArchMachineState *lams, TPMIf *tpmif)
+{
+    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(lams->platform_bus_dev);
+    hwaddr pbus_base = VIRT_PLATFORM_BUS_BASEADDRESS;

These seem to be the differences between the loongarch and the arm virt implementations.
Why not have a function with this signature that both archs can call?

static void virt_tpm_plug(PlatformBusDevice *pbus, hwaddr pbus_base, TPMIf *tpmif)

Regards,
    Stefan

On Fri, Jul 14, 2023 at 5:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 7/14/23 03:09, Joelle van Dyne wrote:
> > TPM needs to know its own base address in order to generate its DSDT
> > device entry.
> >
> > Signed-off-by: Joelle van Dyne <j@getutm.app>
> > ---
> >   hw/arm/virt.c | 37 +++++++++++++++++++++++++++++++++++++
> >   1 file changed, 37 insertions(+)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 7d9dbc2663..432148ef47 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -2732,6 +2732,37 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
> >                            dev, &error_abort);
> >   }
> >
> > +#ifdef CONFIG_TPM
> > +static void virt_tpm_plug(VirtMachineState *vms, TPMIf *tpmif)
> > +{
> > +    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
> > +    hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
>
> +static void virt_tpm_plug(LoongArchMachineState *lams, TPMIf *tpmif)
> +{
> +    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(lams->platform_bus_dev);
> +    hwaddr pbus_base = VIRT_PLATFORM_BUS_BASEADDRESS;
>
> These seem to be the differences between the loongarch and the arm virt implementations.
> Why not have a function with this signature that both archs can call?
>
> static void virt_tpm_plug(PlatformBusDevice *pbus, hwaddr pbus_base, TPMIf *tpmif)
>
> Regards,
>     Stefan

That was the first intended solution, but the code that this is
replacing was copy-pasted and as I don't know anything about this arch
or know how to test it, this caused the least amount of changes to
that target architecture (no additional includes, changes to the build
scripts, etc). I don't think there is currently a common "virt" module
that all the "virt" targets use so we would have to create that. It
seemed out of scope for this TPM patchset and could be something for a
different patch set. tl;dr: the code it replaces was copy-pasted so
it's not strictly making things worse.

On Fri, 14 Jul 2023 00:09:23 -0700
Joelle van Dyne <j@getutm.app> wrote:

> TPM needs to know its own base address in order to generate its DSDT
> device entry.
> 
> Signed-off-by: Joelle van Dyne <j@getutm.app>
> ---
>  hw/arm/virt.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 7d9dbc2663..432148ef47 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2732,6 +2732,37 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
>                           dev, &error_abort);
>  }
>  
> +#ifdef CONFIG_TPM

ifdefs are not really welcome, usually we use stubs instead
see stubs/virtio-md-pci.c or hw/display/acpi-vga-stub.c as an example.

> +static void virt_tpm_plug(VirtMachineState *vms, TPMIf *tpmif)
> +{
> +    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
> +    hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
> +    SysBusDevice *sbdev = SYS_BUS_DEVICE(tpmif);
> +    MemoryRegion *sbdev_mr;
> +    hwaddr tpm_base;
> +    uint64_t tpm_size;
> +
> +    if (!sbdev || !object_dynamic_cast(OBJECT(sbdev), TYPE_SYS_BUS_DEVICE)) {
it seems object_dynamic_cast() can deal with NULL object,  perhaps drop !sbdev part

> +        return;
> +    }
> +
> +    tpm_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
> +    assert(tpm_base != -1);
> +
> +    tpm_base += pbus_base;
> +
> +    sbdev_mr = sysbus_mmio_get_region(sbdev, 0);
> +    tpm_size = memory_region_size(sbdev_mr);
> +

> +    if (object_property_find(OBJECT(sbdev), "baseaddr")) {
> +        object_property_set_uint(OBJECT(sbdev), "baseaddr", tpm_base, NULL);
> +    }
> +    if (object_property_find(OBJECT(sbdev), "size")) {
> +        object_property_set_uint(OBJECT(sbdev), "size", tpm_size, NULL);
> +    }
why both properties wrapped into conditions?
Also can addr/size be wrong and cause property setter fail?
(if yes then you'd need to s/, NULL, &error_abort/)


> +}
> +#endif
> +
>  static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>                                              DeviceState *dev, Error **errp)
>  {
> @@ -2803,6 +2834,12 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>          vms->virtio_iommu_bdf = pci_get_bdf(pdev);
>          create_virtio_iommu_dt_bindings(vms);
>      }
> +
> +#ifdef CONFIG_TPM
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_TPM_IF)) {
> +        virt_tpm_plug(vms, TPM_IF(dev));
> +    }
> +#endif
>  }
>  
>  static void virt_dimm_unplug_request(HotplugHandler *hotplug_dev,