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 <address@hidden>
>
> ---
>
> 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.