| Message ID | 20230714070931.23476-8-j@getutm.app (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: introduce TPM CRB SysBus device | expand |
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,
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; + 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)) { + 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); + } +} +#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,
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(+)