[RFC 0/2] vTPM for aarch64

[RFC 0/2] vTPM for aarch64

[Eric Auger]

This series adds the capability to instantiate an MMIO TPM TIS
in ARM virt.

The series was tested with the swtpm/libtpms emulator.
Automatic guest LUKS volume unlocking (tpm2) was successful.
EDK2 support is under development [3]. Thanks to Ard
for supporting me when setting up the test environment.

Best Regards

Eric

Testing:

mkdir /tmp/tpm
swtpm socket \
--tpm2 \
-t -d \
--tpmstate dir=/tmp/tpm \
--ctrl type=unixio,path=/tmp/swtpm-sock

qemu command line must be augmented with the following options:

-chardev socket,id=chrtpm,path=/tmp/swtpm-sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm \
-device tpm-tis,tpmdev=tpm0 \

References:
[1] libtpms: https://github.com/stefanberger/libtpms/wiki
[2] swtpm: https://github.com/stefanberger/swtpm/wiki
[3] [PATCH 0/4] ArmVirtPkg: implement measured boot for ArmVirtQemu

This series can be found at:
https://github.com/eauger/qemu/tree/v4.2.0-tpm-rfc

Eric Auger (2):
  tpm: Let the TPM TIS device be usable on ARM
  hw/arm/virt: vTPM support

 hw/arm/Kconfig      |  1 +
 hw/arm/sysbus-fdt.c | 36 ++++++++++++++++++++++++++++++++++++
 hw/arm/virt.c       |  7 +++++++
 hw/tpm/Kconfig      |  2 +-
 hw/tpm/tpm_tis.c    | 16 ++++++++++++++++
 5 files changed, 61 insertions(+), 1 deletion(-)

-- 
2.20.1

[RFC 1/2] tpm: Let the TPM TIS device be usable on ARM

[Eric Auger]

Implement support for TPM on aarch64 by using the
TPM TIS MMIO frontend. Instead of being an ISA device,
the TPM TIS device becomes a sysbus device on ARM. It is
bound to be dynamically instantiated.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

I am aware such kind of #ifde'fy is frown upon but this is just
for starting the discussion
---
 hw/tpm/Kconfig   |  2 +-
 hw/tpm/tpm_tis.c | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
index 9e67d990e8..326c89e6df 100644
--- a/hw/tpm/Kconfig
+++ b/hw/tpm/Kconfig
@@ -4,7 +4,7 @@ config TPMDEV
 
 config TPM_TIS
     bool
-    depends on TPM && ISA_BUS
+    depends on TPM
     select TPMDEV
 
 config TPM_CRB
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 31facb896d..cfc840942f 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -30,6 +30,7 @@
 
 #include "hw/acpi/tpm.h"
 #include "hw/pci/pci_ids.h"
+#include "hw/sysbus.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "sysemu/tpm_backend.h"
@@ -65,7 +66,11 @@ typedef struct TPMLocality {
 } TPMLocality;
 
 typedef struct TPMState {
+#ifdef CONFIG_ISA_BUS
     ISADevice busdev;
+#else
+    SysBusDevice busdev;
+#endif
     MemoryRegion mmio;
 
     unsigned char buffer[TPM_TIS_BUFFER_MAX];
@@ -967,6 +972,7 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
         error_setg(errp, "'tpmdev' property is required");
         return;
     }
+#ifdef CONFIG_ISA_BUS
     if (s->irq_num > 15) {
         error_setg(errp, "IRQ %d is outside valid range of 0 to 15",
                    s->irq_num);
@@ -982,6 +988,7 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
         tpm_ppi_init(&s->ppi, isa_address_space(ISA_DEVICE(dev)),
                      TPM_PPI_ADDR_BASE, OBJECT(s));
     }
+#endif
 }
 
 static void tpm_tis_initfn(Object *obj)
@@ -991,6 +998,10 @@ static void tpm_tis_initfn(Object *obj)
     memory_region_init_io(&s->mmio, OBJECT(s), &tpm_tis_memory_ops,
                           s, "tpm-tis-mmio",
                           TPM_TIS_NUM_LOCALITIES << TPM_TIS_LOCALITY_SHIFT);
+#ifndef CONFIG_ISA_BUS
+    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
+    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
+#endif
 }
 
 static void tpm_tis_class_init(ObjectClass *klass, void *data)
@@ -1002,6 +1013,7 @@ static void tpm_tis_class_init(ObjectClass *klass, void *data)
     device_class_set_props(dc, tpm_tis_properties);
     dc->reset = tpm_tis_reset;
     dc->vmsd  = &vmstate_tpm_tis;
+    dc->user_creatable = true;
     tc->model = TPM_MODEL_TPM_TIS;
     tc->get_version = tpm_tis_get_tpm_version;
     tc->request_completed = tpm_tis_request_completed;
@@ -1009,7 +1021,11 @@ static void tpm_tis_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo tpm_tis_info = {
     .name = TYPE_TPM_TIS,
+#ifdef CONFIG_ISA_BUS
     .parent = TYPE_ISA_DEVICE,
+#else
+    .parent = TYPE_SYS_BUS_DEVICE,
+#endif
     .instance_size = sizeof(TPMState),
     .instance_init = tpm_tis_initfn,
     .class_init  = tpm_tis_class_init,
-- 
2.20.1

[RFC 2/2] hw/arm/virt: vTPM support

[Eric Auger]

Let the TPM TIS device be dynamically instantiable in ARM virt.
A device tree node is dynamically created (TPM via MMIO).

The TPM Physical Presence interface (PPI) is not supported.

To run with the swtmp TPM emulator, the qemu command line must
be augmented with:

        -chardev socket,id=chrtpm,path=swtpm-sock \
        -tpmdev emulator,id=tpm0,chardev=chrtpm \
        -device tpm-tis,tpmdev=tpm0 \

swtpm/libtpms command line example:

swtpm socket --tpm2 -t -d --tpmstate dir=/tmp/tpm \
--ctrl type=unixio,path=swtpm-sock

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/Kconfig      |  1 +
 hw/arm/sysbus-fdt.c | 36 ++++++++++++++++++++++++++++++++++++
 hw/arm/virt.c       |  7 +++++++
 3 files changed, 44 insertions(+)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 3d86691ae0..ce0852f148 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -5,6 +5,7 @@ config ARM_VIRT
     imply VFIO_AMD_XGBE
     imply VFIO_PLATFORM
     imply VFIO_XGMAC
+    imply TPM_TIS
     select A15MPCORE
     select ACPI
     select ARM_SMMUV3
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 022fc97ecd..d723fad6ba 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -30,6 +30,7 @@
 #include "hw/arm/sysbus-fdt.h"
 #include "qemu/error-report.h"
 #include "sysemu/device_tree.h"
+#include "sysemu/tpm.h"
 #include "hw/platform-bus.h"
 #include "hw/vfio/vfio-platform.h"
 #include "hw/vfio/vfio-calxeda-xgmac.h"
@@ -434,6 +435,40 @@ static bool vfio_platform_match(SysBusDevice *sbdev,
 #define VFIO_PLATFORM_BINDING(compat, add_fn) \
     {TYPE_VFIO_PLATFORM, (compat), (add_fn), vfio_platform_match}
 
+/*
+ * add_tpm_tis_fdt_node: Create a DT node for TPM TIS
+ *
+ * See kernel documentation:
+ * Documentation/devicetree/bindings/security/tpm/tpm_tis_mmio.txt
+ * Optional interrupt for command completion is not exposed
+ */
+static int add_tpm_tis_fdt_node(SysBusDevice *sbdev, void *opaque)
+{
+    PlatformBusFDTData *data = opaque;
+    PlatformBusDevice *pbus = data->pbus;
+    void *fdt = data->fdt;
+    const char *parent_node = data->pbus_node_name;
+    int compat_str_len;
+    char *nodename;
+    uint32_t reg_attr[2];
+    uint64_t mmio_base;
+
+    mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
+    nodename = g_strdup_printf("%s/tpm_tis@%" PRIx64, parent_node, mmio_base);
+    qemu_fdt_add_subnode(fdt, nodename);
+
+    compat_str_len = strlen("tcg,tpm-tis-mmio") + 1;
+    qemu_fdt_setprop(fdt, nodename, "compatible", "tcg,tpm-tis-mmio",
+                     compat_str_len);
+
+    reg_attr[0] = cpu_to_be32(mmio_base);
+    reg_attr[1] = cpu_to_be32(0x5000);
+    qemu_fdt_setprop(fdt, nodename, "reg", reg_attr, 2 * sizeof(uint32_t));
+
+    g_free(nodename);
+    return 0;
+}
+
 #endif /* CONFIG_LINUX */
 
 static int no_fdt_node(SysBusDevice *sbdev, void *opaque)
@@ -455,6 +490,7 @@ static const BindingEntry bindings[] = {
     TYPE_BINDING(TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node),
     TYPE_BINDING(TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node),
     VFIO_PLATFORM_BINDING("amd,xgbe-seattle-v1a", add_amd_xgbe_fdt_node),
+    TYPE_BINDING(TYPE_TPM_TIS, add_tpm_tis_fdt_node),
 #endif
     TYPE_BINDING(TYPE_RAMFB_DEVICE, no_fdt_node),
     TYPE_BINDING("", NULL), /* last element */
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f788fe27d6..1bb34dfa0b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -47,6 +47,7 @@
 #include "sysemu/numa.h"
 #include "sysemu/runstate.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/tpm.h"
 #include "sysemu/kvm.h"
 #include "hw/loader.h"
 #include "exec/address-spaces.h"
@@ -2041,6 +2042,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE);
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_PLATFORM);
+    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS);
     mc->block_default_type = IF_VIRTIO;
     mc->no_cdrom = 1;
     mc->pci_allow_0_address = true;
@@ -2153,6 +2155,11 @@ type_init(machvirt_machine_init);
 
 static void virt_machine_5_0_options(MachineClass *mc)
 {
+    static GlobalProperty compat[] = {
+        { TYPE_TPM_TIS, "ppi", "false" },
+    };
+
+    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
 }
 DEFINE_VIRT_MACHINE_AS_LATEST(5, 0)
 
-- 
2.20.1

Re: [RFC 2/2] hw/arm/virt: vTPM support

[Philippe Mathieu-Daudé]

On 2/10/20 2:15 PM, Eric Auger wrote:
> Let the TPM TIS device be dynamically instantiable in ARM virt.
> A device tree node is dynamically created (TPM via MMIO).
> 
> The TPM Physical Presence interface (PPI) is not supported.
> 
> To run with the swtmp TPM emulator, the qemu command line must
> be augmented with:
> 
>          -chardev socket,id=chrtpm,path=swtpm-sock \
>          -tpmdev emulator,id=tpm0,chardev=chrtpm \
>          -device tpm-tis,tpmdev=tpm0 \
> 
> swtpm/libtpms command line example:
> 
> swtpm socket --tpm2 -t -d --tpmstate dir=/tmp/tpm \
> --ctrl type=unixio,path=swtpm-sock
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>   hw/arm/Kconfig      |  1 +
>   hw/arm/sysbus-fdt.c | 36 ++++++++++++++++++++++++++++++++++++
>   hw/arm/virt.c       |  7 +++++++
>   3 files changed, 44 insertions(+)
> 
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 3d86691ae0..ce0852f148 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -5,6 +5,7 @@ config ARM_VIRT
>       imply VFIO_AMD_XGBE
>       imply VFIO_PLATFORM
>       imply VFIO_XGMAC
> +    imply TPM_TIS
>       select A15MPCORE
>       select ACPI
>       select ARM_SMMUV3
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 022fc97ecd..d723fad6ba 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -30,6 +30,7 @@
>   #include "hw/arm/sysbus-fdt.h"
>   #include "qemu/error-report.h"
>   #include "sysemu/device_tree.h"
> +#include "sysemu/tpm.h"
>   #include "hw/platform-bus.h"
>   #include "hw/vfio/vfio-platform.h"
>   #include "hw/vfio/vfio-calxeda-xgmac.h"
> @@ -434,6 +435,40 @@ static bool vfio_platform_match(SysBusDevice *sbdev,
>   #define VFIO_PLATFORM_BINDING(compat, add_fn) \
>       {TYPE_VFIO_PLATFORM, (compat), (add_fn), vfio_platform_match}
>   
> +/*
> + * add_tpm_tis_fdt_node: Create a DT node for TPM TIS
> + *
> + * See kernel documentation:
> + * Documentation/devicetree/bindings/security/tpm/tpm_tis_mmio.txt
> + * Optional interrupt for command completion is not exposed
> + */
> +static int add_tpm_tis_fdt_node(SysBusDevice *sbdev, void *opaque)
> +{
> +    PlatformBusFDTData *data = opaque;
> +    PlatformBusDevice *pbus = data->pbus;
> +    void *fdt = data->fdt;
> +    const char *parent_node = data->pbus_node_name;
> +    int compat_str_len;
> +    char *nodename;
> +    uint32_t reg_attr[2];
> +    uint64_t mmio_base;
> +
> +    mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
> +    nodename = g_strdup_printf("%s/tpm_tis@%" PRIx64, parent_node, mmio_base);
> +    qemu_fdt_add_subnode(fdt, nodename);
> +
> +    compat_str_len = strlen("tcg,tpm-tis-mmio") + 1;
> +    qemu_fdt_setprop(fdt, nodename, "compatible", "tcg,tpm-tis-mmio",
> +                     compat_str_len);
> +
> +    reg_attr[0] = cpu_to_be32(mmio_base);
> +    reg_attr[1] = cpu_to_be32(0x5000);
> +    qemu_fdt_setprop(fdt, nodename, "reg", reg_attr, 2 * sizeof(uint32_t));
> +
> +    g_free(nodename);
> +    return 0;
> +}

I don't know well the policy with virt and fdt_node, but this patch LGTM.

> +
>   #endif /* CONFIG_LINUX */
>   
>   static int no_fdt_node(SysBusDevice *sbdev, void *opaque)
> @@ -455,6 +490,7 @@ static const BindingEntry bindings[] = {
>       TYPE_BINDING(TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node),
>       TYPE_BINDING(TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node),
>       VFIO_PLATFORM_BINDING("amd,xgbe-seattle-v1a", add_amd_xgbe_fdt_node),
> +    TYPE_BINDING(TYPE_TPM_TIS, add_tpm_tis_fdt_node),
>   #endif
>       TYPE_BINDING(TYPE_RAMFB_DEVICE, no_fdt_node),
>       TYPE_BINDING("", NULL), /* last element */
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f788fe27d6..1bb34dfa0b 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -47,6 +47,7 @@
>   #include "sysemu/numa.h"
>   #include "sysemu/runstate.h"
>   #include "sysemu/sysemu.h"
> +#include "sysemu/tpm.h"
>   #include "sysemu/kvm.h"
>   #include "hw/loader.h"
>   #include "exec/address-spaces.h"
> @@ -2041,6 +2042,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>       machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE);
>       machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
>       machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_PLATFORM);
> +    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS);
>       mc->block_default_type = IF_VIRTIO;
>       mc->no_cdrom = 1;
>       mc->pci_allow_0_address = true;
> @@ -2153,6 +2155,11 @@ type_init(machvirt_machine_init);
>   
>   static void virt_machine_5_0_options(MachineClass *mc)
>   {
> +    static GlobalProperty compat[] = {
> +        { TYPE_TPM_TIS, "ppi", "false" },
> +    };
> +
> +    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
>   }
>   DEFINE_VIRT_MACHINE_AS_LATEST(5, 0)
>   
> 

Re: [RFC 0/2] vTPM for aarch64

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200210131523.27540-1-eric.auger@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

qemu-system-x86_64: Option '-device tpm-tis' cannot be handled by this machine
Broken pipe
/tmp/qemu-test/src/tests/qtest/libqtest.c:140: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
ERROR - missing test plan
make: *** [check-qtest-x86_64] Error 1
make: *** Waiting for unfinished jobs....

Looking for expected file 'tests/data/acpi/virt/FACP.numamem'
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=b2939374f40a44a1a91bd0b59788e3e4', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-bqlt1z7p/src/docker-src.2020-02-10-08.32.35.5484:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=b2939374f40a44a1a91bd0b59788e3e4
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-bqlt1z7p/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    11m27.446s
user    0m8.845s


The full log is available at
http://patchew.org/logs/20200210131523.27540-1-eric.auger@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [RFC 1/2] tpm: Let the TPM TIS device be usable on ARM

[Philippe Mathieu-Daudé]

On 2/10/20 2:15 PM, Eric Auger wrote:
> Implement support for TPM on aarch64 by using the
> TPM TIS MMIO frontend. Instead of being an ISA device,
> the TPM TIS device becomes a sysbus device on ARM. It is
> bound to be dynamically instantiated.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> I am aware such kind of #ifde'fy is frown upon but this is just
> for starting the discussion

I doubt this can be accepted upstream, because a target has to choose 
between using sysbus OR isa devices, not both.

> ---
>   hw/tpm/Kconfig   |  2 +-
>   hw/tpm/tpm_tis.c | 16 ++++++++++++++++
>   2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
> index 9e67d990e8..326c89e6df 100644
> --- a/hw/tpm/Kconfig
> +++ b/hw/tpm/Kconfig
> @@ -4,7 +4,7 @@ config TPMDEV
>   
>   config TPM_TIS
>       bool
> -    depends on TPM && ISA_BUS
> +    depends on TPM
>       select TPMDEV
>   
>   config TPM_CRB
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 31facb896d..cfc840942f 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -30,6 +30,7 @@
>   
>   #include "hw/acpi/tpm.h"
>   #include "hw/pci/pci_ids.h"
> +#include "hw/sysbus.h"
>   #include "hw/qdev-properties.h"
>   #include "migration/vmstate.h"
>   #include "sysemu/tpm_backend.h"
> @@ -65,7 +66,11 @@ typedef struct TPMLocality {
>   } TPMLocality;
>   
>   typedef struct TPMState {
> +#ifdef CONFIG_ISA_BUS
>       ISADevice busdev;
> +#else
> +    SysBusDevice busdev;
> +#endif
>       MemoryRegion mmio;
>   
>       unsigned char buffer[TPM_TIS_BUFFER_MAX];
> @@ -967,6 +972,7 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
>           error_setg(errp, "'tpmdev' property is required");
>           return;
>       }
> +#ifdef CONFIG_ISA_BUS
>       if (s->irq_num > 15) {
>           error_setg(errp, "IRQ %d is outside valid range of 0 to 15",
>                      s->irq_num);
> @@ -982,6 +988,7 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
>           tpm_ppi_init(&s->ppi, isa_address_space(ISA_DEVICE(dev)),
>                        TPM_PPI_ADDR_BASE, OBJECT(s));
>       }
> +#endif
>   }
>   
>   static void tpm_tis_initfn(Object *obj)
> @@ -991,6 +998,10 @@ static void tpm_tis_initfn(Object *obj)
>       memory_region_init_io(&s->mmio, OBJECT(s), &tpm_tis_memory_ops,
>                             s, "tpm-tis-mmio",
>                             TPM_TIS_NUM_LOCALITIES << TPM_TIS_LOCALITY_SHIFT);
> +#ifndef CONFIG_ISA_BUS
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
> +#endif
>   }
>   
>   static void tpm_tis_class_init(ObjectClass *klass, void *data)
> @@ -1002,6 +1013,7 @@ static void tpm_tis_class_init(ObjectClass *klass, void *data)
>       device_class_set_props(dc, tpm_tis_properties);
>       dc->reset = tpm_tis_reset;
>       dc->vmsd  = &vmstate_tpm_tis;

With your #ifde'fy in mind, you probably need to restrict this to sysbus:

   #ifndef CONFIG_ISA_BUS

> +    dc->user_creatable = true;

   #endif

>       tc->model = TPM_MODEL_TPM_TIS;
>       tc->get_version = tpm_tis_get_tpm_version;
>       tc->request_completed = tpm_tis_request_completed;
> @@ -1009,7 +1021,11 @@ static void tpm_tis_class_init(ObjectClass *klass, void *data)
>   
>   static const TypeInfo tpm_tis_info = {
>       .name = TYPE_TPM_TIS,
> +#ifdef CONFIG_ISA_BUS
>       .parent = TYPE_ISA_DEVICE,
> +#else
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +#endif
>       .instance_size = sizeof(TPMState),
>       .instance_init = tpm_tis_initfn,
>       .class_init  = tpm_tis_class_init,
> 

 From the sysbus device PoV the patch looks OK.

You don't need much to remove the RFC tag:

- rename TYPE_TPM_TIS as TYPE_TPM_TIS_ISA
- rename TPMState as TPMCommonState, add an abstract TYPE_TPM_TIS 
parent, let TYPE_TPM_TIS_ISA be a child
- add TYPE_TPM_TIS_SYSBUS also child.

Re: [RFC 1/2] tpm: Let the TPM TIS device be usable on ARM

[Auger Eric]

Hi Philippe,

On 2/11/20 9:25 AM, Philippe Mathieu-Daudé wrote:
> On 2/10/20 2:15 PM, Eric Auger wrote:
>> Implement support for TPM on aarch64 by using the
>> TPM TIS MMIO frontend. Instead of being an ISA device,
>> the TPM TIS device becomes a sysbus device on ARM. It is
>> bound to be dynamically instantiated.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> I am aware such kind of #ifde'fy is frown upon but this is just
>> for starting the discussion
> 
> I doubt this can be accepted upstream, because a target has to choose
> between using sysbus OR isa devices, not both.
yep that was a first shot to show how this can be derived for ARM but
this deserves some additional care.

Anyway it currently breaks the x86 code because CONFIG_ISA_BUS is never
defined :-( config-devices.h should be included to fix that. Meaning
that the tpm-tis.o should be compiled with additional -I options. In
that prospect tpm-tis.o should be an obj-y and not a common-obj (Connie).
> 
>> ---
>>   hw/tpm/Kconfig   |  2 +-
>>   hw/tpm/tpm_tis.c | 16 ++++++++++++++++
>>   2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
>> index 9e67d990e8..326c89e6df 100644
>> --- a/hw/tpm/Kconfig
>> +++ b/hw/tpm/Kconfig
>> @@ -4,7 +4,7 @@ config TPMDEV
>>     config TPM_TIS
>>       bool
>> -    depends on TPM && ISA_BUS
>> +    depends on TPM
>>       select TPMDEV
>>     config TPM_CRB
>> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
>> index 31facb896d..cfc840942f 100644
>> --- a/hw/tpm/tpm_tis.c
>> +++ b/hw/tpm/tpm_tis.c
>> @@ -30,6 +30,7 @@
>>     #include "hw/acpi/tpm.h"
>>   #include "hw/pci/pci_ids.h"
>> +#include "hw/sysbus.h"
>>   #include "hw/qdev-properties.h"
>>   #include "migration/vmstate.h"
>>   #include "sysemu/tpm_backend.h"
>> @@ -65,7 +66,11 @@ typedef struct TPMLocality {
>>   } TPMLocality;
>>     typedef struct TPMState {
>> +#ifdef CONFIG_ISA_BUS
>>       ISADevice busdev;
>> +#else
>> +    SysBusDevice busdev;
>> +#endif
>>       MemoryRegion mmio;
>>         unsigned char buffer[TPM_TIS_BUFFER_MAX];
>> @@ -967,6 +972,7 @@ static void tpm_tis_realizefn(DeviceState *dev,
>> Error **errp)
>>           error_setg(errp, "'tpmdev' property is required");
>>           return;
>>       }
>> +#ifdef CONFIG_ISA_BUS
>>       if (s->irq_num > 15) {
>>           error_setg(errp, "IRQ %d is outside valid range of 0 to 15",
>>                      s->irq_num);
>> @@ -982,6 +988,7 @@ static void tpm_tis_realizefn(DeviceState *dev,
>> Error **errp)
>>           tpm_ppi_init(&s->ppi, isa_address_space(ISA_DEVICE(dev)),
>>                        TPM_PPI_ADDR_BASE, OBJECT(s));
>>       }
>> +#endif
>>   }
>>     static void tpm_tis_initfn(Object *obj)
>> @@ -991,6 +998,10 @@ static void tpm_tis_initfn(Object *obj)
>>       memory_region_init_io(&s->mmio, OBJECT(s), &tpm_tis_memory_ops,
>>                             s, "tpm-tis-mmio",
>>                             TPM_TIS_NUM_LOCALITIES <<
>> TPM_TIS_LOCALITY_SHIFT);
>> +#ifndef CONFIG_ISA_BUS
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
>> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
>> +#endif
>>   }
>>     static void tpm_tis_class_init(ObjectClass *klass, void *data)
>> @@ -1002,6 +1013,7 @@ static void tpm_tis_class_init(ObjectClass
>> *klass, void *data)
>>       device_class_set_props(dc, tpm_tis_properties);
>>       dc->reset = tpm_tis_reset;
>>       dc->vmsd  = &vmstate_tpm_tis;
> 
> With your #ifde'fy in mind, you probably need to restrict this to sysbus:
> 
>   #ifndef CONFIG_ISA_BUS
> 
>> +    dc->user_creatable = true;
> 
>   #endif
yes you're right, this only applies to sysbus
> 
>>       tc->model = TPM_MODEL_TPM_TIS;
>>       tc->get_version = tpm_tis_get_tpm_version;
>>       tc->request_completed = tpm_tis_request_completed;
>> @@ -1009,7 +1021,11 @@ static void tpm_tis_class_init(ObjectClass
>> *klass, void *data)
>>     static const TypeInfo tpm_tis_info = {
>>       .name = TYPE_TPM_TIS,
>> +#ifdef CONFIG_ISA_BUS
>>       .parent = TYPE_ISA_DEVICE,
>> +#else
>> +    .parent = TYPE_SYS_BUS_DEVICE,
>> +#endif
>>       .instance_size = sizeof(TPMState),
>>       .instance_init = tpm_tis_initfn,
>>       .class_init  = tpm_tis_class_init,
>>
> 
> From the sysbus device PoV the patch looks OK.
> 
> You don't need much to remove the RFC tag:
> 
> - rename TYPE_TPM_TIS as TYPE_TPM_TIS_ISA
> - rename TPMState as TPMCommonState, add an abstract TYPE_TPM_TIS
> parent, let TYPE_TPM_TIS_ISA be a child
> - add TYPE_TPM_TIS_SYSBUS also child.
Yes I tried my luck without too much hopes ;-)

Thanks!

Eric
> 

Re: [RFC 1/2] tpm: Let the TPM TIS device be usable on ARM

[Peter Maydell]

On Tue, 11 Feb 2020 at 08:35, Auger Eric <eric.auger@redhat.com> wrote:
>
> Hi Philippe,
>
> On 2/11/20 9:25 AM, Philippe Mathieu-Daudé wrote:
> > You don't need much to remove the RFC tag:
> >
> > - rename TYPE_TPM_TIS as TYPE_TPM_TIS_ISA
> > - rename TPMState as TPMCommonState, add an abstract TYPE_TPM_TIS
> > parent, let TYPE_TPM_TIS_ISA be a child
> > - add TYPE_TPM_TIS_SYSBUS also child.
> Yes I tried my luck without too much hopes ;-)

There should be a few existing examples in the tree
of devices that we provide in a sysbus and also
an isa or pci flavour, that you can use as templates
for how to structure the device.

-- PMM

Re: [RFC 1/2] tpm: Let the TPM TIS device be usable on ARM

[Auger Eric]

Hi Peter,

On 2/11/20 11:56 AM, Peter Maydell wrote:
> On Tue, 11 Feb 2020 at 08:35, Auger Eric <eric.auger@redhat.com> wrote:
>>
>> Hi Philippe,
>>
>> On 2/11/20 9:25 AM, Philippe Mathieu-Daudé wrote:
>>> You don't need much to remove the RFC tag:
>>>
>>> - rename TYPE_TPM_TIS as TYPE_TPM_TIS_ISA
>>> - rename TPMState as TPMCommonState, add an abstract TYPE_TPM_TIS
>>> parent, let TYPE_TPM_TIS_ISA be a child
>>> - add TYPE_TPM_TIS_SYSBUS also child.
>> Yes I tried my luck without too much hopes ;-)
> 
> There should be a few existing examples in the tree
> of devices that we provide in a sysbus and also
> an isa or pci flavour, that you can use as templates
> for how to structure the device.
Yes I found some. Thank you.

Eric

> 
> -- PMM
> 

Re: [RFC 1/2] tpm: Let the TPM TIS device be usable on ARM

[Stefan Berger]

On 2/11/20 5:56 AM, Peter Maydell wrote:
> On Tue, 11 Feb 2020 at 08:35, Auger Eric <eric.auger@redhat.com> wrote:
>> Hi Philippe,
>>
>> On 2/11/20 9:25 AM, Philippe Mathieu-Daudé wrote:
>>> You don't need much to remove the RFC tag:
>>>
>>> - rename TYPE_TPM_TIS as TYPE_TPM_TIS_ISA
>>> - rename TPMState as TPMCommonState, add an abstract TYPE_TPM_TIS
>>> parent, let TYPE_TPM_TIS_ISA be a child
>>> - add TYPE_TPM_TIS_SYSBUS also child.
>> Yes I tried my luck without too much hopes ;-)
> There should be a few existing examples in the tree
> of devices that we provide in a sysbus and also
> an isa or pci flavour, that you can use as templates
> for how to structure the device.

block/fdc.c ?


    Stefan