| Message ID | 20230714070931.23476-10-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: > This reduces redundent code in different machine types with ACPI table > generation. Additionally, this will allow us to support multiple TPM > interfaces. Finally, this matches up with the TPM TIS ISA I don't know whether we would want multiple devices. tpm_find() usage is certainly not prepared for multiple devices. > implementation. > > Ideally, we would be able to call `qbus_build_aml` and avoid any TPM > specific code in the ACPI table generation. However, currently we > still have to call `build_tpm2` anyways and it does not look like > most other ACPI devices support the `ACPI_DEV_AML_IF` interface. > > Signed-off-by: Joelle van Dyne <j@getutm.app> > --- > hw/arm/virt-acpi-build.c | 38 ++------------------------------------ > hw/loongarch/acpi-build.c | 38 ++------------------------------------ > hw/tpm/tpm_tis_sysbus.c | 35 +++++++++++++++++++++++++++++++++++ > 3 files changed, 39 insertions(+), 72 deletions(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 6b674231c2..49b2f19440 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -35,6 +35,7 @@ > #include "target/arm/cpu.h" > #include "hw/acpi/acpi-defs.h" > #include "hw/acpi/acpi.h" > +#include "hw/acpi/acpi_aml_interface.h" > #include "hw/nvram/fw_cfg.h" > #include "hw/acpi/bios-linker-loader.h" > #include "hw/acpi/aml-build.h" > @@ -208,41 +209,6 @@ static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap, > aml_append(scope, dev); > } > > -#ifdef CONFIG_TPM > -static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms) > -{ > - PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev); > - hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base; > - SysBusDevice *sbdev = SYS_BUS_DEVICE(tpm_find()); > - MemoryRegion *sbdev_mr; > - hwaddr tpm_base; > - > - if (!sbdev) { > - 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); > - > - Aml *dev = aml_device("TPM0"); > - aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101"))); > - aml_append(dev, aml_name_decl("_STR", aml_string("TPM 2.0 Device"))); > - aml_append(dev, aml_name_decl("_UID", aml_int(0))); > - > - Aml *crs = aml_resource_template(); > - aml_append(crs, > - aml_memory32_fixed(tpm_base, > - (uint32_t)memory_region_size(sbdev_mr), > - AML_READ_WRITE)); > - aml_append(dev, aml_name_decl("_CRS", crs)); > - aml_append(scope, dev); > -} > -#endif > - > #define ID_MAPPING_ENTRY_SIZE 20 > #define SMMU_V3_ENTRY_SIZE 68 > #define ROOT_COMPLEX_ENTRY_SIZE 36 > @@ -891,7 +857,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > > acpi_dsdt_add_power_button(scope); > #ifdef CONFIG_TPM > - acpi_dsdt_add_tpm(scope, vms); > + call_dev_aml_func(DEVICE(tpm_find()), scope); > #endif > > aml_append(dsdt, scope); > diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c > index 0b62c3a2f7..4291e670c8 100644 > --- a/hw/loongarch/acpi-build.c > +++ b/hw/loongarch/acpi-build.c > @@ -14,6 +14,7 @@ > #include "target/loongarch/cpu.h" > #include "hw/acpi/acpi-defs.h" > #include "hw/acpi/acpi.h" > +#include "hw/acpi/acpi_aml_interface.h" > #include "hw/nvram/fw_cfg.h" > #include "hw/acpi/bios-linker-loader.h" > #include "migration/vmstate.h" > @@ -328,41 +329,6 @@ static void build_flash_aml(Aml *scope, LoongArchMachineState *lams) > aml_append(scope, dev); > } > > -#ifdef CONFIG_TPM > -static void acpi_dsdt_add_tpm(Aml *scope, LoongArchMachineState *vms) > -{ > - PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev); > - hwaddr pbus_base = VIRT_PLATFORM_BUS_BASEADDRESS; > - SysBusDevice *sbdev = SYS_BUS_DEVICE(tpm_find()); > - MemoryRegion *sbdev_mr; > - hwaddr tpm_base; > - > - if (!sbdev) { > - 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); > - > - Aml *dev = aml_device("TPM0"); > - aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101"))); > - aml_append(dev, aml_name_decl("_STR", aml_string("TPM 2.0 Device"))); > - aml_append(dev, aml_name_decl("_UID", aml_int(0))); > - > - Aml *crs = aml_resource_template(); > - aml_append(crs, > - aml_memory32_fixed(tpm_base, > - (uint32_t)memory_region_size(sbdev_mr), > - AML_READ_WRITE)); > - aml_append(dev, aml_name_decl("_CRS", crs)); > - aml_append(scope, dev); > -} > -#endif > - Good for the consolidation. > /* build DSDT */ > static void > build_dsdt(GArray *table_data, BIOSLinker *linker, MachineState *machine) > @@ -379,7 +345,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, MachineState *machine) > build_la_ged_aml(dsdt, machine); > build_flash_aml(dsdt, lams); > #ifdef CONFIG_TPM > - acpi_dsdt_add_tpm(dsdt, lams); > + call_dev_aml_func(DEVICE(tpm_find()), dsdt); > #endif > /* System State Package */ > scope = aml_scope("\\"); > diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c > index 45e63efd63..b3ea2305b5 100644 > --- a/hw/tpm/tpm_tis_sysbus.c > +++ b/hw/tpm/tpm_tis_sysbus.c > @@ -30,6 +30,7 @@ > #include "hw/sysbus.h" > #include "tpm_tis.h" > #include "qom/object.h" > +#include "hw/acpi/acpi_aml_interface.h" > > struct TPMStateSysBus { > /*< private >*/ > @@ -37,6 +38,8 @@ struct TPMStateSysBus { > > /*< public >*/ > TPMState state; /* not a QOM object */ > + uint64_t baseaddr; > + uint64_t size; Does moving the TIS to a different address help on aarch64? Can the size really be an option? I don't see it useful and if one gave the wrong size it may break things. > }; > > OBJECT_DECLARE_SIMPLE_TYPE(TPMStateSysBus, TPM_TIS_SYSBUS) > @@ -94,6 +97,8 @@ static 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, false), > + DEFINE_PROP_UINT64("baseaddr", TPMStateSysBus, baseaddr, TPM_TIS_ADDR_BASE), > + DEFINE_PROP_UINT64("size", TPMStateSysBus, size, TPM_TIS_ADDR_SIZE), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -126,10 +131,38 @@ static void tpm_tis_sysbus_realizefn(DeviceState *dev, Error **errp) > } > } > > +static void build_tpm_tis_sysbus_aml(AcpiDevAmlIf *adev, Aml *scope) > +{ > + Aml *dev, *crs; > + TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(adev); > + TPMIf *ti = TPM_IF(sbdev); > + > + dev = aml_device("TPM"); > + if (tpm_tis_sysbus_get_tpm_version(ti) == TPM_VERSION_2_0) { > + aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101"))); > + aml_append(dev, aml_name_decl("_STR", aml_string("TPM 2.0 Device"))); > + } else { > + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31"))); > + } > + aml_append(dev, aml_name_decl("_UID", aml_int(0))); > + aml_append(dev, aml_name_decl("_STA", aml_int(0xF))); > + crs = aml_resource_template(); > + aml_append(crs, aml_memory32_fixed(sbdev->baseaddr, sbdev->size, > + AML_READ_WRITE)); > + /* > + * FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs, > + * fix default TPM_TIS_IRQ value there to use some unused IRQ > + */ > + /* aml_append(crs, aml_irq_no_flags(sbdev->state.irq_num)); */ > + aml_append(dev, aml_name_decl("_CRS", crs)); > + aml_append(scope, dev); > +} > + > static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > TPMIfClass *tc = TPM_IF_CLASS(klass); > + AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass); > > device_class_set_props(dc, tpm_tis_sysbus_properties); > dc->vmsd = &vmstate_tpm_tis_sysbus; > @@ -140,6 +173,7 @@ static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data) > tc->request_completed = tpm_tis_sysbus_request_completed; > tc->get_version = tpm_tis_sysbus_get_tpm_version; > set_bit(DEVICE_CATEGORY_MISC, dc->categories); > + adevc->build_dev_aml = build_tpm_tis_sysbus_aml; > } > > static const TypeInfo tpm_tis_sysbus_info = { > @@ -150,6 +184,7 @@ static const TypeInfo tpm_tis_sysbus_info = { > .class_init = tpm_tis_sysbus_class_init, > .interfaces = (InterfaceInfo[]) { > { TYPE_TPM_IF }, > + { TYPE_ACPI_DEV_AML_IF }, > { } > } > };
On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > > I don't know whether we would want multiple devices. tpm_find() usage is certainly not prepared for multiple devices. Sorry, "multiple TPM interfaces" here does not mean "at the same time". Will clarify the description. > > > Good for the consolidation. > > > Does moving the TIS to a different address help on aarch64? That was the first thing we tried and no it doesn't help. > > Can the size really be an option? I don't see it useful and if one gave the wrong size it may break things. It was added for consistency (otherwise we have to determine the size by looking at the interface everywhere). Also, it is possible for the size to be larger than the constant. For example, Apple Silicon uses 16KiB page sizes and we may decide to force the device to be 16KiB aligned (not sure if this is needed yet while we still track down why the dual mapping was not working). In that case, we would need to inform the OS of the true region size to prevent any overlap issues. Both baseaddr and size should be provided only by the plug handler in the virt machine, otherwise things may break even if we get rid of size and have just an incorrect baseaddr. > > >
On 7/14/23 13:29, Joelle van Dyne wrote: > On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> >> >> I don't know whether we would want multiple devices. tpm_find() usage is certainly not prepared for multiple devices. > Sorry, "multiple TPM interfaces" here does not mean "at the same > time". Will clarify the description. > >> >> >> Good for the consolidation. >> >> >> Does moving the TIS to a different address help on aarch64? > That was the first thing we tried and no it doesn't help. I would remove it if we don't have a known alternative address that makes it work. If we do, I think we should document it in tpm.rst. >> >> Can the size really be an option? I don't see it useful and if one gave the wrong size it may break things. > It was added for consistency (otherwise we have to determine the size > by looking at the interface everywhere). Also, it is possible for the > size to be larger than the constant. For example, Apple Silicon uses > 16KiB page sizes and we may decide to force the device to be 16KiB > aligned (not sure if this is needed yet while we still track down why > the dual mapping was not working). In that case, we would need to > inform the OS of the true region size to prevent any overlap issues. > Both baseaddr and size should be provided only by the plug handler in > the virt machine, otherwise things may break even if we get rid of > size and have just an incorrect baseaddr. > >> >> >>
On Fri, Jul 14, 2023 at 10:37 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 7/14/23 13:29, Joelle van Dyne wrote: > > On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > >> > >> > >> > >> > >> I don't know whether we would want multiple devices. tpm_find() usage is certainly not prepared for multiple devices. > > Sorry, "multiple TPM interfaces" here does not mean "at the same > > time". Will clarify the description. > > > >> > >> > >> Good for the consolidation. > >> > >> > >> Does moving the TIS to a different address help on aarch64? > > That was the first thing we tried and no it doesn't help. > > I would remove it if we don't have a known alternative address that makes it work. If we do, I think we should document it in tpm.rst. "It" is referring to tpm-tis-device? Note that it does work fine with Linux VMs. > > > >> > >> Can the size really be an option? I don't see it useful and if one gave the wrong size it may break things. > > It was added for consistency (otherwise we have to determine the size > > by looking at the interface everywhere). Also, it is possible for the > > size to be larger than the constant. For example, Apple Silicon uses > > 16KiB page sizes and we may decide to force the device to be 16KiB > > aligned (not sure if this is needed yet while we still track down why > > the dual mapping was not working). In that case, we would need to > > inform the OS of the true region size to prevent any overlap issues. > > Both baseaddr and size should be provided only by the plug handler in > > the virt machine, otherwise things may break even if we get rid of > > size and have just an incorrect baseaddr. > > > >> > >> > >>
On 7/14/23 13:39, Joelle van Dyne wrote: > On Fri, Jul 14, 2023 at 10:37 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> >> On 7/14/23 13:29, Joelle van Dyne wrote: >>> On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >>>> >>>> >>>> >>>> >>>> I don't know whether we would want multiple devices. tpm_find() usage is certainly not prepared for multiple devices. >>> Sorry, "multiple TPM interfaces" here does not mean "at the same >>> time". Will clarify the description. >>> >>>> >>>> >>>> Good for the consolidation. >>>> >>>> >>>> Does moving the TIS to a different address help on aarch64? >>> That was the first thing we tried and no it doesn't help. >> >> I would remove it if we don't have a known alternative address that makes it work. If we do, I think we should document it in tpm.rst. > "It" is referring to tpm-tis-device? Note that it does work fine with Linux VMs. yes, tpm_tis_sysbus and I know it works with Liunux but I see this discussion here around Win 11 on aarch64. Why do we need to user another address than the standard address if for Win 11 on aarch64 it doesn't get it to work. > >> >> >>>> >>>> Can the size really be an option? I don't see it useful and if one gave the wrong size it may break things. >>> It was added for consistency (otherwise we have to determine the size >>> by looking at the interface everywhere). Also, it is possible for the >>> size to be larger than the constant. For example, Apple Silicon uses >>> 16KiB page sizes and we may decide to force the device to be 16KiB >>> aligned (not sure if this is needed yet while we still track down why >>> the dual mapping was not working). In that case, we would need to >>> inform the OS of the true region size to prevent any overlap issues. >>> Both baseaddr and size should be provided only by the plug handler in >>> the virt machine, otherwise things may break even if we get rid of >>> size and have just an incorrect baseaddr. >>> >>>> >>>> >>>>
On Fri, Jul 14, 2023 at 10:43 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 7/14/23 13:39, Joelle van Dyne wrote: > > On Fri, Jul 14, 2023 at 10:37 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > >> > >> > >> > >> On 7/14/23 13:29, Joelle van Dyne wrote: > >>> On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > >>>> > >>>> > >>>> > >>>> > >>>> I don't know whether we would want multiple devices. tpm_find() usage is certainly not prepared for multiple devices. > >>> Sorry, "multiple TPM interfaces" here does not mean "at the same > >>> time". Will clarify the description. > >>> > >>>> > >>>> > >>>> Good for the consolidation. > >>>> > >>>> > >>>> Does moving the TIS to a different address help on aarch64? > >>> That was the first thing we tried and no it doesn't help. > >> > >> I would remove it if we don't have a known alternative address that makes it work. If we do, I think we should document it in tpm.rst. > > "It" is referring to tpm-tis-device? Note that it does work fine with Linux VMs. > > yes, tpm_tis_sysbus and I know it works with Liunux but I see this discussion here around Win 11 on aarch64. Why do we need to user another address than the standard address if for Win 11 on aarch64 it doesn't get it to work. The standard address won't work for Linux either. TPM TIS on standard address on ARM64 Virt machines = collision with DRAM, will not instantiate TPM TIS on SysBus with dynamically allocated address = works on Linux, cannot start on Windows > > > > >> > >> > >>>> > >>>> Can the size really be an option? I don't see it useful and if one gave the wrong size it may break things. > >>> It was added for consistency (otherwise we have to determine the size > >>> by looking at the interface everywhere). Also, it is possible for the > >>> size to be larger than the constant. For example, Apple Silicon uses > >>> 16KiB page sizes and we may decide to force the device to be 16KiB > >>> aligned (not sure if this is needed yet while we still track down why > >>> the dual mapping was not working). In that case, we would need to > >>> inform the OS of the true region size to prevent any overlap issues. > >>> Both baseaddr and size should be provided only by the plug handler in > >>> the virt machine, otherwise things may break even if we get rid of > >>> size and have just an incorrect baseaddr. > >>> > >>>> > >>>> > >>>>
On 7/14/23 13:46, Joelle van Dyne wrote: > On Fri, Jul 14, 2023 at 10:43 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> >> On 7/14/23 13:39, Joelle van Dyne wrote: >>> On Fri, Jul 14, 2023 at 10:37 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >>>> >>>> >>>> >>>> On 7/14/23 13:29, Joelle van Dyne wrote: >>>>> On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> I don't know whether we would want multiple devices. tpm_find() usage is certainly not prepared for multiple devices. >>>>> Sorry, "multiple TPM interfaces" here does not mean "at the same >>>>> time". Will clarify the description. >>>>> >>>>>> >>>>>> >>>>>> Good for the consolidation. >>>>>> >>>>>> >>>>>> Does moving the TIS to a different address help on aarch64? >>>>> That was the first thing we tried and no it doesn't help. >>>> >>>> I would remove it if we don't have a known alternative address that makes it work. If we do, I think we should document it in tpm.rst. >>> "It" is referring to tpm-tis-device? Note that it does work fine with Linux VMs. >> >> yes, tpm_tis_sysbus and I know it works with Liunux but I see this discussion here around Win 11 on aarch64. Why do we need to user another address than the standard address if for Win 11 on aarch64 it doesn't get it to work. > The standard address won't work for Linux either. > > TPM TIS on standard address on ARM64 Virt machines = collision with > DRAM, will not instantiate I thought that this was working with Linux on the aarch64 virt board as contributed by Eric Auger. https://github.com/qemu/qemu/commit/fcaa204194e15ba24cd53087dd616aabbc29e64f Also I had tested it to some extent: https://github.com/stefanberger/swtpm/issues/493#issuecomment-885221109 > TPM TIS on SysBus with dynamically allocated address = works on Linux, > cannot start on Windows > >> >>> >>>> >>>> >>>>>> >>>>>> Can the size really be an option? I don't see it useful and if one gave the wrong size it may break things. >>>>> It was added for consistency (otherwise we have to determine the size >>>>> by looking at the interface everywhere). Also, it is possible for the >>>>> size to be larger than the constant. For example, Apple Silicon uses >>>>> 16KiB page sizes and we may decide to force the device to be 16KiB >>>>> aligned (not sure if this is needed yet while we still track down why >>>>> the dual mapping was not working). In that case, we would need to >>>>> inform the OS of the true region size to prevent any overlap issues. >>>>> Both baseaddr and size should be provided only by the plug handler in >>>>> the virt machine, otherwise things may break even if we get rid of >>>>> size and have just an incorrect baseaddr. >>>>> >>>>>> >>>>>> >>>>>>
On Fri, Jul 14, 2023 at 11:01 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 7/14/23 13:46, Joelle van Dyne wrote: > > On Fri, Jul 14, 2023 at 10:43 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > >> > >> > >> > >> On 7/14/23 13:39, Joelle van Dyne wrote: > >>> On Fri, Jul 14, 2023 at 10:37 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > >>>> > >>>> > >>>> > >>>> On 7/14/23 13:29, Joelle van Dyne wrote: > >>>>> On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>>> I don't know whether we would want multiple devices. tpm_find() usage is certainly not prepared for multiple devices. > >>>>> Sorry, "multiple TPM interfaces" here does not mean "at the same > >>>>> time". Will clarify the description. > >>>>> > >>>>>> > >>>>>> > >>>>>> Good for the consolidation. > >>>>>> > >>>>>> > >>>>>> Does moving the TIS to a different address help on aarch64? > >>>>> That was the first thing we tried and no it doesn't help. > >>>> > >>>> I would remove it if we don't have a known alternative address that makes it work. If we do, I think we should document it in tpm.rst. > >>> "It" is referring to tpm-tis-device? Note that it does work fine with Linux VMs. > >> > >> yes, tpm_tis_sysbus and I know it works with Liunux but I see this discussion here around Win 11 on aarch64. Why do we need to user another address than the standard address if for Win 11 on aarch64 it doesn't get it to work. > > The standard address won't work for Linux either. > > > > TPM TIS on standard address on ARM64 Virt machines = collision with > > DRAM, will not instantiate > > I thought that this was working with Linux on the aarch64 virt board as contributed by Eric Auger. > > https://github.com/qemu/qemu/commit/fcaa204194e15ba24cd53087dd616aabbc29e64f > > Also I had tested it to some extent: https://github.com/stefanberger/swtpm/issues/493#issuecomment-885221109 I think I know where the confusion is. Both your examples use "tpm-tis-device" which indeed uses the SysBus and gets a dynamic address. In this patch, the removed code that generates the AML gets this address by poking into the SysBus device, getting its base, then adding the offset from the TIS device to it. In the change, we move that code to get the address to earlier in the Virt init sequence (before the machine is realized) in order to tell the TIS device what its own base address is. Then, during the AML generation phase, we can just tell the TIS device to "generate your own AML" which is now possible because it knows its own base address. This is also how the TIS ISA bus device does it but that is simpler because it can just use the default address. Separately, there is a `build_tpm2` table function which also needs the device base address but only for CRB devices (TIS has the field set to 0) so it's irrelevant here. > > > > > TPM TIS on SysBus with dynamically allocated address = works on Linux, > > cannot start on Windows > > > >> > >>> > >>>> > >>>> > >>>>>> > >>>>>> Can the size really be an option? I don't see it useful and if one gave the wrong size it may break things. > >>>>> It was added for consistency (otherwise we have to determine the size > >>>>> by looking at the interface everywhere). Also, it is possible for the > >>>>> size to be larger than the constant. For example, Apple Silicon uses > >>>>> 16KiB page sizes and we may decide to force the device to be 16KiB > >>>>> aligned (not sure if this is needed yet while we still track down why > >>>>> the dual mapping was not working). In that case, we would need to > >>>>> inform the OS of the true region size to prevent any overlap issues. > >>>>> Both baseaddr and size should be provided only by the plug handler in > >>>>> the virt machine, otherwise things may break even if we get rid of > >>>>> size and have just an incorrect baseaddr. > >>>>> > >>>>>> > >>>>>> > >>>>>>
On Fri, 14 Jul 2023 10:29:31 -0700 Joelle van Dyne <j@getutm.app> wrote: > On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > > > > > > > I don't know whether we would want multiple devices. tpm_find() usage is certainly not prepared for multiple devices. > Sorry, "multiple TPM interfaces" here does not mean "at the same > time". Will clarify the description. > > > > > > > Good for the consolidation. > > > > > > Does moving the TIS to a different address help on aarch64? > That was the first thing we tried and no it doesn't help. > > > > Can the size really be an option? I don't see it useful and if one gave the wrong size it may break things. > It was added for consistency (otherwise we have to determine the size > by looking at the interface everywhere). Also, it is possible for the > size to be larger than the constant. For example, Apple Silicon uses > 16KiB page sizes and we may decide to force the device to be 16KiB > aligned (not sure if this is needed yet while we still track down why > the dual mapping was not working). In that case, we would need to > inform the OS of the true region size to prevent any overlap issues. > Both baseaddr and size should be provided only by the plug handler in > the virt machine, otherwise things may break even if we get rid of > size and have just an incorrect baseaddr. > if properties (address/size) are for internal use, it would be better to use x- prefix as not give user false promise that user can expect them working if they provided them. > > > > >
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 6b674231c2..49b2f19440 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -35,6 +35,7 @@ #include "target/arm/cpu.h" #include "hw/acpi/acpi-defs.h" #include "hw/acpi/acpi.h" +#include "hw/acpi/acpi_aml_interface.h" #include "hw/nvram/fw_cfg.h" #include "hw/acpi/bios-linker-loader.h" #include "hw/acpi/aml-build.h" @@ -208,41 +209,6 @@ static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap, aml_append(scope, dev); } -#ifdef CONFIG_TPM -static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms) -{ - PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev); - hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base; - SysBusDevice *sbdev = SYS_BUS_DEVICE(tpm_find()); - MemoryRegion *sbdev_mr; - hwaddr tpm_base; - - if (!sbdev) { - 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); - - Aml *dev = aml_device("TPM0"); - aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101"))); - aml_append(dev, aml_name_decl("_STR", aml_string("TPM 2.0 Device"))); - aml_append(dev, aml_name_decl("_UID", aml_int(0))); - - Aml *crs = aml_resource_template(); - aml_append(crs, - aml_memory32_fixed(tpm_base, - (uint32_t)memory_region_size(sbdev_mr), - AML_READ_WRITE)); - aml_append(dev, aml_name_decl("_CRS", crs)); - aml_append(scope, dev); -} -#endif - #define ID_MAPPING_ENTRY_SIZE 20 #define SMMU_V3_ENTRY_SIZE 68 #define ROOT_COMPLEX_ENTRY_SIZE 36 @@ -891,7 +857,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) acpi_dsdt_add_power_button(scope); #ifdef CONFIG_TPM - acpi_dsdt_add_tpm(scope, vms); + call_dev_aml_func(DEVICE(tpm_find()), scope); #endif aml_append(dsdt, scope); diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c index 0b62c3a2f7..4291e670c8 100644 --- a/hw/loongarch/acpi-build.c +++ b/hw/loongarch/acpi-build.c @@ -14,6 +14,7 @@ #include "target/loongarch/cpu.h" #include "hw/acpi/acpi-defs.h" #include "hw/acpi/acpi.h" +#include "hw/acpi/acpi_aml_interface.h" #include "hw/nvram/fw_cfg.h" #include "hw/acpi/bios-linker-loader.h" #include "migration/vmstate.h" @@ -328,41 +329,6 @@ static void build_flash_aml(Aml *scope, LoongArchMachineState *lams) aml_append(scope, dev); } -#ifdef CONFIG_TPM -static void acpi_dsdt_add_tpm(Aml *scope, LoongArchMachineState *vms) -{ - PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev); - hwaddr pbus_base = VIRT_PLATFORM_BUS_BASEADDRESS; - SysBusDevice *sbdev = SYS_BUS_DEVICE(tpm_find()); - MemoryRegion *sbdev_mr; - hwaddr tpm_base; - - if (!sbdev) { - 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); - - Aml *dev = aml_device("TPM0"); - aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101"))); - aml_append(dev, aml_name_decl("_STR", aml_string("TPM 2.0 Device"))); - aml_append(dev, aml_name_decl("_UID", aml_int(0))); - - Aml *crs = aml_resource_template(); - aml_append(crs, - aml_memory32_fixed(tpm_base, - (uint32_t)memory_region_size(sbdev_mr), - AML_READ_WRITE)); - aml_append(dev, aml_name_decl("_CRS", crs)); - aml_append(scope, dev); -} -#endif - /* build DSDT */ static void build_dsdt(GArray *table_data, BIOSLinker *linker, MachineState *machine) @@ -379,7 +345,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, MachineState *machine) build_la_ged_aml(dsdt, machine); build_flash_aml(dsdt, lams); #ifdef CONFIG_TPM - acpi_dsdt_add_tpm(dsdt, lams); + call_dev_aml_func(DEVICE(tpm_find()), dsdt); #endif /* System State Package */ scope = aml_scope("\\"); diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c index 45e63efd63..b3ea2305b5 100644 --- a/hw/tpm/tpm_tis_sysbus.c +++ b/hw/tpm/tpm_tis_sysbus.c @@ -30,6 +30,7 @@ #include "hw/sysbus.h" #include "tpm_tis.h" #include "qom/object.h" +#include "hw/acpi/acpi_aml_interface.h" struct TPMStateSysBus { /*< private >*/ @@ -37,6 +38,8 @@ struct TPMStateSysBus { /*< public >*/ TPMState state; /* not a QOM object */ + uint64_t baseaddr; + uint64_t size; }; OBJECT_DECLARE_SIMPLE_TYPE(TPMStateSysBus, TPM_TIS_SYSBUS) @@ -94,6 +97,8 @@ static 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, false), + DEFINE_PROP_UINT64("baseaddr", TPMStateSysBus, baseaddr, TPM_TIS_ADDR_BASE), + DEFINE_PROP_UINT64("size", TPMStateSysBus, size, TPM_TIS_ADDR_SIZE), DEFINE_PROP_END_OF_LIST(), }; @@ -126,10 +131,38 @@ static void tpm_tis_sysbus_realizefn(DeviceState *dev, Error **errp) } } +static void build_tpm_tis_sysbus_aml(AcpiDevAmlIf *adev, Aml *scope) +{ + Aml *dev, *crs; + TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(adev); + TPMIf *ti = TPM_IF(sbdev); + + dev = aml_device("TPM"); + if (tpm_tis_sysbus_get_tpm_version(ti) == TPM_VERSION_2_0) { + aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101"))); + aml_append(dev, aml_name_decl("_STR", aml_string("TPM 2.0 Device"))); + } else { + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31"))); + } + aml_append(dev, aml_name_decl("_UID", aml_int(0))); + aml_append(dev, aml_name_decl("_STA", aml_int(0xF))); + crs = aml_resource_template(); + aml_append(crs, aml_memory32_fixed(sbdev->baseaddr, sbdev->size, + AML_READ_WRITE)); + /* + * FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs, + * fix default TPM_TIS_IRQ value there to use some unused IRQ + */ + /* aml_append(crs, aml_irq_no_flags(sbdev->state.irq_num)); */ + aml_append(dev, aml_name_decl("_CRS", crs)); + aml_append(scope, dev); +} + static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); TPMIfClass *tc = TPM_IF_CLASS(klass); + AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass); device_class_set_props(dc, tpm_tis_sysbus_properties); dc->vmsd = &vmstate_tpm_tis_sysbus; @@ -140,6 +173,7 @@ static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data) tc->request_completed = tpm_tis_sysbus_request_completed; tc->get_version = tpm_tis_sysbus_get_tpm_version; set_bit(DEVICE_CATEGORY_MISC, dc->categories); + adevc->build_dev_aml = build_tpm_tis_sysbus_aml; } static const TypeInfo tpm_tis_sysbus_info = { @@ -150,6 +184,7 @@ static const TypeInfo tpm_tis_sysbus_info = { .class_init = tpm_tis_sysbus_class_init, .interfaces = (InterfaceInfo[]) { { TYPE_TPM_IF }, + { TYPE_ACPI_DEV_AML_IF }, { } } };
This reduces redundent code in different machine types with ACPI table generation. Additionally, this will allow us to support multiple TPM interfaces. Finally, this matches up with the TPM TIS ISA implementation. Ideally, we would be able to call `qbus_build_aml` and avoid any TPM specific code in the ACPI table generation. However, currently we still have to call `build_tpm2` anyways and it does not look like most other ACPI devices support the `ACPI_DEV_AML_IF` interface. Signed-off-by: Joelle van Dyne <j@getutm.app> --- hw/arm/virt-acpi-build.c | 38 ++------------------------------------ hw/loongarch/acpi-build.c | 38 ++------------------------------------ hw/tpm/tpm_tis_sysbus.c | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 72 deletions(-)