Re: [Qemu-devel] [PATCH v2 1/2] pci_host: Turn into SysBus-derived QOM type

[Michael S. Tsirkin]

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<address@hidden>
> >>>
> >>>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<address@hidden>
> >>>Signed-off-by: Wanpeng Li<address@hidden>
> >>>Signed-off-by: Andreas Färber<address@hidden>
> >>>Reviewed-by: Anthony Liguori<address@hidden>
> >>
> >>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]
> >