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

[Anthony Liguori <anthony@codemonkey.ws>]

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