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