| Message ID | 1339343875-6958-2-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
On 06/10/2012 10:57 AM, Andreas Färber wrote: > From: Andreas Färber<andreas.faerber@web.de> > > Allows us to access PCIHostState QOM-style with PCI_HOST() macro. > > Update PReP Raven PCI to derive from this type. > > Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> > Signed-off-by: Wanpeng Li<liwp@linux.vnet.ibm.com> > Signed-off-by: Andreas Färber<andreas.faerber@web.de> > Reviewed-by: Anthony Liguori<aliguori@us.ibm.com> > --- > hw/pci_host.c | 11 +++++++++++ > hw/pci_host.h | 3 +++ > hw/prep_pci.c | 4 ++-- > 3 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/hw/pci_host.c b/hw/pci_host.c > index 8041778..347bfa6 100644 > --- a/hw/pci_host.c > +++ b/hw/pci_host.c > @@ -165,4 +165,15 @@ const MemoryRegionOps pci_host_data_be_ops = { > .endianness = DEVICE_BIG_ENDIAN, > }; > > +static const TypeInfo pci_host_type_info = { > + .name = TYPE_PCI_HOST, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(PCIHostState), > +}; Any reason why PCIHost can't have TYPE_DEVICE as the the parent? Regards, Anthony Liguori > + > +static void pci_host_register_types(void) > +{ > + type_register_static(&pci_host_type_info); > +} > > +type_init(pci_host_register_types) > diff --git a/hw/pci_host.h b/hw/pci_host.h > index 359e38f..fba9fde 100644 > --- a/hw/pci_host.h > +++ b/hw/pci_host.h > @@ -30,6 +30,9 @@ > > #include "sysbus.h" > > +#define TYPE_PCI_HOST "pci-host" > +#define PCI_HOST(obj) OBJECT_CHECK(PCIHostState, (obj), TYPE_PCI_HOST) > + > struct PCIHostState { > SysBusDevice busdev; > MemoryRegion conf_mem; > diff --git a/hw/prep_pci.c b/hw/prep_pci.c > index 38dbff4..3e9f6b8 100644 > --- a/hw/prep_pci.c > +++ b/hw/prep_pci.c > @@ -183,9 +183,9 @@ static void raven_pcihost_class_init(ObjectClass *klass, void *data) > dc->no_user = 1; > } > > -static TypeInfo raven_pcihost_info = { > +static const TypeInfo raven_pcihost_info = { > .name = "raven-pcihost", > - .parent = TYPE_SYS_BUS_DEVICE, > + .parent = TYPE_PCI_HOST, > .instance_size = sizeof(PREPPCIState), > .class_init = raven_pcihost_class_init, > };
Am 10.06.2012 19:33, schrieb Anthony Liguori: > On 06/10/2012 10:57 AM, Andreas Färber wrote: >> From: Andreas Färber<andreas.faerber@web.de> >> >> Allows us to access PCIHostState QOM-style with PCI_HOST() macro. >> >> Update PReP Raven PCI to derive from this type. >> >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >> Signed-off-by: Wanpeng Li<liwp@linux.vnet.ibm.com> >> Signed-off-by: Andreas Färber<andreas.faerber@web.de> >> Reviewed-by: Anthony Liguori<aliguori@us.ibm.com> >> --- >> hw/pci_host.c | 11 +++++++++++ >> hw/pci_host.h | 3 +++ >> hw/prep_pci.c | 4 ++-- >> 3 files changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/hw/pci_host.c b/hw/pci_host.c >> index 8041778..347bfa6 100644 >> --- a/hw/pci_host.c >> +++ b/hw/pci_host.c >> @@ -165,4 +165,15 @@ const MemoryRegionOps pci_host_data_be_ops = { >> .endianness = DEVICE_BIG_ENDIAN, >> }; >> >> +static const TypeInfo pci_host_type_info = { >> + .name = TYPE_PCI_HOST, >> + .parent = TYPE_SYS_BUS_DEVICE, >> + .instance_size = sizeof(PCIHostState), >> +}; > > Any reason why PCIHost can't have TYPE_DEVICE as the the parent? Many current users rely on SysBus functionality like MMIO and IRQs. A conversion to TYPE_DEVICE can be done as part of your SysBus removal series. Regards, Andreas
On Sun, Jun 10, 2012 at 05:57:54PM +0200, Andreas Färber wrote: > From: Andreas Färber <andreas.faerber@web.de> > > Allows us to access PCIHostState QOM-style with PCI_HOST() macro. > > Update PReP Raven PCI to derive from this type. > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > Signed-off-by: Wanpeng Li <liwp@linux.vnet.ibm.com> > Signed-off-by: Andreas Färber <andreas.faerber@web.de> > Reviewed-by: Anthony Liguori <aliguori@us.ibm.com> Question: this is really a pci host *bridge*. We are calling this PCIHost internally for brevity but QOM hierarchy is user-visible, right? > --- > hw/pci_host.c | 11 +++++++++++ > hw/pci_host.h | 3 +++ > hw/prep_pci.c | 4 ++-- > 3 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/hw/pci_host.c b/hw/pci_host.c > index 8041778..347bfa6 100644 > --- a/hw/pci_host.c > +++ b/hw/pci_host.c > @@ -165,4 +165,15 @@ const MemoryRegionOps pci_host_data_be_ops = { > .endianness = DEVICE_BIG_ENDIAN, > }; > > +static const TypeInfo pci_host_type_info = { > + .name = TYPE_PCI_HOST, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(PCIHostState), > +}; > + > +static void pci_host_register_types(void) > +{ > + type_register_static(&pci_host_type_info); > +} > > +type_init(pci_host_register_types) > diff --git a/hw/pci_host.h b/hw/pci_host.h > index 359e38f..fba9fde 100644 > --- a/hw/pci_host.h > +++ b/hw/pci_host.h > @@ -30,6 +30,9 @@ > > #include "sysbus.h" > > +#define TYPE_PCI_HOST "pci-host" > +#define PCI_HOST(obj) OBJECT_CHECK(PCIHostState, (obj), TYPE_PCI_HOST) > + > struct PCIHostState { > SysBusDevice busdev; > MemoryRegion conf_mem; > diff --git a/hw/prep_pci.c b/hw/prep_pci.c > index 38dbff4..3e9f6b8 100644 > --- a/hw/prep_pci.c > +++ b/hw/prep_pci.c > @@ -183,9 +183,9 @@ static void raven_pcihost_class_init(ObjectClass *klass, void *data) > dc->no_user = 1; > } > > -static TypeInfo raven_pcihost_info = { > +static const TypeInfo raven_pcihost_info = { > .name = "raven-pcihost", > - .parent = TYPE_SYS_BUS_DEVICE, > + .parent = TYPE_PCI_HOST, > .instance_size = sizeof(PREPPCIState), > .class_init = raven_pcihost_class_init, > }; > -- > 1.7.7
Am 18.06.2012 20:28, schrieb Michael S. Tsirkin: > On Sun, Jun 10, 2012 at 05:57:54PM +0200, Andreas Färber wrote: >> From: Andreas Färber <andreas.faerber@web.de> >> >> Allows us to access PCIHostState QOM-style with PCI_HOST() macro. >> >> Update PReP Raven PCI to derive from this type. >> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >> Signed-off-by: Wanpeng Li <liwp@linux.vnet.ibm.com> >> Signed-off-by: Andreas Färber <andreas.faerber@web.de> >> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com> > > Question: this is really a pci host *bridge*. > We are calling this PCIHost internally for brevity > but QOM hierarchy is user-visible, right? That's a good question... I would say it's not user-visible today unless we instantiate TYPE_PCI_HOST directly, in which case its value "pci-host" would be visible through the "type" property that got introduced on qom-next. My CPU modeling for instance is based on the assumption that we can introduce intermediate types later as a user-invisible implementation detail. >> --- >> hw/pci_host.c | 11 +++++++++++ >> hw/pci_host.h | 3 +++ >> hw/prep_pci.c | 4 ++-- >> 3 files changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/hw/pci_host.c b/hw/pci_host.c >> index 8041778..347bfa6 100644 >> --- a/hw/pci_host.c >> +++ b/hw/pci_host.c >> @@ -165,4 +165,15 @@ const MemoryRegionOps pci_host_data_be_ops = { >> .endianness = DEVICE_BIG_ENDIAN, >> }; >> >> +static const TypeInfo pci_host_type_info = { >> + .name = TYPE_PCI_HOST, >> + .parent = TYPE_SYS_BUS_DEVICE, >> + .instance_size = sizeof(PCIHostState), >> +}; It would in fact be better to set .abstract = true, I guess. Anyway, I think for sPAPR I translated PHB to PCI_HOST_BRIDGE already, so TYPE_PCI_HOST_BRIDGE "pci-host-bridge" and PCI_HOST_BRIDGE() would fit in nicely with the otherwise clear and verbose QOM naming. But I'd rather not rename PCIHostState everywhere... do you agree? Or would you want to have it as PCIHostBridge or PCIHostBridgeState for consistency? If we use TYPE_PCI_HOST_BRIDGE I should also add _BRIDGE for some of the derived types, but we can't change the user-visible "-pcihost" type name there for backwards compatibility. Any further wishes? Should the second patch be split up in some way? Andreas >> + >> +static void pci_host_register_types(void) >> +{ >> + type_register_static(&pci_host_type_info); >> +} >> >> +type_init(pci_host_register_types) [snip]
On 06/18/2012 04:44 PM, Andreas Färber wrote: > Am 18.06.2012 20:28, schrieb Michael S. Tsirkin: >> On Sun, Jun 10, 2012 at 05:57:54PM +0200, Andreas Färber wrote: >>> From: Andreas Färber<andreas.faerber@web.de> >>> >>> Allows us to access PCIHostState QOM-style with PCI_HOST() macro. >>> >>> Update PReP Raven PCI to derive from this type. >>> >>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >>> Signed-off-by: Wanpeng Li<liwp@linux.vnet.ibm.com> >>> Signed-off-by: Andreas Färber<andreas.faerber@web.de> >>> Reviewed-by: Anthony Liguori<aliguori@us.ibm.com> >> >> Question: this is really a pci host *bridge*. >> We are calling this PCIHost internally for brevity >> but QOM hierarchy is user-visible, right? > > That's a good question... I would say it's not user-visible today unless > we instantiate TYPE_PCI_HOST directly, in which case its value > "pci-host" would be visible through the "type" property that got > introduced on qom-next. My CPU modeling for instance is based on the > assumption that we can introduce intermediate types later as a > user-invisible implementation detail. Yes, we need to formulate a support statement for the 1.2 release. My general thinking is: 1) Properties will remain ABI compatible. A property will not change it's type or semantics over time. An example is link/child properties. A link will always remain a link but the link subtype made be made more specific over time. Likewise with child properties. 2) A property may be removed and new properties may be added. Applications should always gracefully handle the non-existence of a property. 3) Since paths are composed of properties, they are subject to the same rules. That is, an absolute path will always have the same semantics as long as that path is still valid. 4) No guarantee is made about the stability of relative paths. Types are really just another form of properties here so changing the type of an object provided that we don't violate ABI rules is okay. I actually think it's okay for a typename to change even if it's exposed by -device. If something is using -device, it ought to probe for the existence of a type before using -device. Regards, Anthony Liguori > >>> --- >>> hw/pci_host.c | 11 +++++++++++ >>> hw/pci_host.h | 3 +++ >>> hw/prep_pci.c | 4 ++-- >>> 3 files changed, 16 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/pci_host.c b/hw/pci_host.c >>> index 8041778..347bfa6 100644 >>> --- a/hw/pci_host.c >>> +++ b/hw/pci_host.c >>> @@ -165,4 +165,15 @@ const MemoryRegionOps pci_host_data_be_ops = { >>> .endianness = DEVICE_BIG_ENDIAN, >>> }; >>> >>> +static const TypeInfo pci_host_type_info = { >>> + .name = TYPE_PCI_HOST, >>> + .parent = TYPE_SYS_BUS_DEVICE, >>> + .instance_size = sizeof(PCIHostState), >>> +}; > > It would in fact be better to set .abstract = true, I guess. > > Anyway, I think for sPAPR I translated PHB to PCI_HOST_BRIDGE already, > so TYPE_PCI_HOST_BRIDGE "pci-host-bridge" and PCI_HOST_BRIDGE() would > fit in nicely with the otherwise clear and verbose QOM naming. But I'd > rather not rename PCIHostState everywhere... do you agree? Or would you > want to have it as PCIHostBridge or PCIHostBridgeState for consistency? > > If we use TYPE_PCI_HOST_BRIDGE I should also add _BRIDGE for some of the > derived types, but we can't change the user-visible "-pcihost" type name > there for backwards compatibility. > > Any further wishes? Should the second patch be split up in some way? > > Andreas > >>> + >>> +static void pci_host_register_types(void) >>> +{ >>> + type_register_static(&pci_host_type_info); >>> +} >>> >>> +type_init(pci_host_register_types) > [snip] >
On Mon, Jun 18, 2012 at 05:23:32PM -0500, Anthony Liguori wrote: > On 06/18/2012 04:44 PM, Andreas Färber wrote: > >Am 18.06.2012 20:28, schrieb Michael S. Tsirkin: > >>On Sun, Jun 10, 2012 at 05:57:54PM +0200, Andreas Färber wrote: > >>>From: Andreas Färber<andreas.faerber@web.de> > >>> > >>>Allows us to access PCIHostState QOM-style with PCI_HOST() macro. > >>> > >>>Update PReP Raven PCI to derive from this type. > >>> > >>>Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> > >>>Signed-off-by: Wanpeng Li<liwp@linux.vnet.ibm.com> > >>>Signed-off-by: Andreas Färber<andreas.faerber@web.de> > >>>Reviewed-by: Anthony Liguori<aliguori@us.ibm.com> > >> > >>Question: this is really a pci host *bridge*. > >>We are calling this PCIHost internally for brevity > >>but QOM hierarchy is user-visible, right? > > > >That's a good question... I would say it's not user-visible today unless > >we instantiate TYPE_PCI_HOST directly, in which case its value > >"pci-host" would be visible through the "type" property that got > >introduced on qom-next. My CPU modeling for instance is based on the > >assumption that we can introduce intermediate types later as a > >user-invisible implementation detail. > > Yes, we need to formulate a support statement for the 1.2 release. > > My general thinking is: > > 1) Properties will remain ABI compatible. A property will not > change it's type or semantics over time. An example is link/child > properties. A link will always remain a link but the link subtype > made be made more specific over time. Likewise with child > properties. > > 2) A property may be removed and new properties may be added. At will? That's a very weak statement. > Applications should always gracefully handle the non-existence of a > property. They can fail gracefully but what else can they do? > 3) Since paths are composed of properties, they are subject to the > same rules. That is, an absolute path will always have the same > semantics as long as that path is still valid. > > 4) No guarantee is made about the stability of relative paths. > > Types are really just another form of properties here so changing > the type of an object provided that we don't violate ABI rules is > okay. Let's invent internal types that we don't expect user to know about. We have x- for properties, let's do the same here. > I actually think it's okay for a typename to change even if it's > exposed by -device. If something is using -device, it ought to > probe for the existence of a type before using -device. > > Regards, > > Anthony Liguori Then it'll fail gracefully but it can't work. > > > >>>--- > >>> hw/pci_host.c | 11 +++++++++++ > >>> hw/pci_host.h | 3 +++ > >>> hw/prep_pci.c | 4 ++-- > >>> 3 files changed, 16 insertions(+), 2 deletions(-) > >>> > >>>diff --git a/hw/pci_host.c b/hw/pci_host.c > >>>index 8041778..347bfa6 100644 > >>>--- a/hw/pci_host.c > >>>+++ b/hw/pci_host.c > >>>@@ -165,4 +165,15 @@ const MemoryRegionOps pci_host_data_be_ops = { > >>> .endianness = DEVICE_BIG_ENDIAN, > >>> }; > >>> > >>>+static const TypeInfo pci_host_type_info = { > >>>+ .name = TYPE_PCI_HOST, > >>>+ .parent = TYPE_SYS_BUS_DEVICE, > >>>+ .instance_size = sizeof(PCIHostState), > >>>+}; > > > >It would in fact be better to set .abstract = true, I guess. > > > >Anyway, I think for sPAPR I translated PHB to PCI_HOST_BRIDGE already, > >so TYPE_PCI_HOST_BRIDGE "pci-host-bridge" and PCI_HOST_BRIDGE() would > >fit in nicely with the otherwise clear and verbose QOM naming. But I'd > >rather not rename PCIHostState everywhere... do you agree? Or would you > >want to have it as PCIHostBridge or PCIHostBridgeState for consistency? > > > >If we use TYPE_PCI_HOST_BRIDGE I should also add _BRIDGE for some of the > >derived types, but we can't change the user-visible "-pcihost" type name > >there for backwards compatibility. > > > >Any further wishes? Should the second patch be split up in some way? > > > >Andreas > > > >>>+ > >>>+static void pci_host_register_types(void) > >>>+{ > >>>+ type_register_static(&pci_host_type_info); > >>>+} > >>> > >>>+type_init(pci_host_register_types) > >[snip] > >
diff --git a/hw/pci_host.c b/hw/pci_host.c index 8041778..347bfa6 100644 --- a/hw/pci_host.c +++ b/hw/pci_host.c @@ -165,4 +165,15 @@ const MemoryRegionOps pci_host_data_be_ops = { .endianness = DEVICE_BIG_ENDIAN, }; +static const TypeInfo pci_host_type_info = { + .name = TYPE_PCI_HOST, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(PCIHostState), +}; + +static void pci_host_register_types(void) +{ + type_register_static(&pci_host_type_info); +} +type_init(pci_host_register_types) diff --git a/hw/pci_host.h b/hw/pci_host.h index 359e38f..fba9fde 100644 --- a/hw/pci_host.h +++ b/hw/pci_host.h @@ -30,6 +30,9 @@ #include "sysbus.h" +#define TYPE_PCI_HOST "pci-host" +#define PCI_HOST(obj) OBJECT_CHECK(PCIHostState, (obj), TYPE_PCI_HOST) + struct PCIHostState { SysBusDevice busdev; MemoryRegion conf_mem; diff --git a/hw/prep_pci.c b/hw/prep_pci.c index 38dbff4..3e9f6b8 100644 --- a/hw/prep_pci.c +++ b/hw/prep_pci.c @@ -183,9 +183,9 @@ static void raven_pcihost_class_init(ObjectClass *klass, void *data) dc->no_user = 1; } -static TypeInfo raven_pcihost_info = { +static const TypeInfo raven_pcihost_info = { .name = "raven-pcihost", - .parent = TYPE_SYS_BUS_DEVICE, + .parent = TYPE_PCI_HOST, .instance_size = sizeof(PREPPCIState), .class_init = raven_pcihost_class_init, };