| Message ID | 20230713035232.48406-8-j@getutm.app (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: introduce TPM CRB SysBus device | expand |
On 7/12/23 23:51, Joelle van Dyne wrote: > TPM needs to know its own base address in order to generate its DSDT > device entry. This and the loongarch patch seem to have largely identical virt_tpm_plug functions. Could they be consolidated in hw/tpm/virt.c ? Stefan > > 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; > + 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,
On Thu, 13 Jul 2023 at 04:52, 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 > +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 I do not like the "platform bus" at all -- it is a nasty hack. If the virt board needs a memory mapped TPM device it should probably just create one, the same way we create our other memory mapped devices. But... How are TPM devices typically set up/visible to the guest on real Arm server hardware ? Should this be a sysbus device at all? thanks -- PMM
On Thu, Jul 13, 2023 at 8:31 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Thu, 13 Jul 2023 at 04:52, 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 > > +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 > > I do not like the "platform bus" at all -- it is a nasty hack. > If the virt board needs a memory mapped TPM device it should probably > just create one, the same way we create our other memory mapped > devices. But... > > How are TPM devices typically set up/visible to the guest on > real Arm server hardware ? Should this be a sysbus device at all? +Alexander Graf who may answer this better. My understanding is that we need to do this for the device to know its own address which it needs to return in a register. On ISA devices, it is always mapped to the same physical address so there's no issues but for Virt machines, device addresses are dynamically allocated by the PlatformBusDevice so only at this late stage can we tell the device what its own address is. > > thanks > -- PMM Also to Stefan's question on consolidating code: that is ideal but currently, it seems like much platform setup code is duplicated amongst the various architecture's Virt machines. There would have to be a larger effort in de-duplicating a lot of that code. Indeed, we try to do this already with some of the ACPI stuff in the other patches. For this specifically, we would need to know the platform bus' base address which is done differently in ARM64's Virt and in Loongarch's Virt. All we did was delete some existing duplicated code and replace it with a different duplicated code :)
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(+)