| Message ID | 20200210131523.27540-2-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vTPM for aarch64 | expand |
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.
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 >
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
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 >
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
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,
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(-)