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

["Andreas Färber" <afaerber@suse.de>]

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]

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg