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 <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.
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
>