Re: [Qemu-devel] [PATCH V6 for-2.3 13/26] hw/pci-host: introduce TYPE_PCI_HOST_BRIDGE_SNOOPED interface

[Marcel Apfelbaum <marcel@redhat.com>]

On 04/28/2015 11:43 AM, Michael S. Tsirkin wrote:
> On Tue, Apr 28, 2015 at 11:39:42AM +0300, Marcel Apfelbaum wrote:
>> On 04/27/2015 05:45 PM, Michael S. Tsirkin wrote:
>>> On Mon, Apr 27, 2015 at 04:01:16PM +0300, Marcel Apfelbaum wrote:
>>>> On 04/27/2015 03:14 PM, Michael S. Tsirkin wrote:
>>>>> On Thu, Mar 19, 2015 at 08:52:48PM +0200, Marcel Apfelbaum wrote:
>>>>>> TYPE_PCI_HOST_BRIDGE_SNOOPED is a special case of host bridge
>>>>>> whose configuration registers are snooped by other host bridges
>>>>>> to complete their configuration cycles.
>>>>>>
>>>>>> The interface exposes a list of snooping host bridges that
>>>>>> shall be used by the hosts implementing this interface
>>>>>> in order to emulate a snooping mechanism.
>>>>>>
>>>>>> The way that the snooping hosts are registered or how
>>>>>> the snooping is implemented are out of the interface scope,
>>>>>> it only provides a way to determine if a host bridge has
>>>>>> snooping hosts and list them.
>>>>>
>>>>>
>>>>>
>>>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>>>> ---
>>>>>>   hw/pci/pci_host.c         |  8 ++++++++
>>>>>>   include/hw/pci/pci_host.h | 24 ++++++++++++++++++++++++
>>>>>>   2 files changed, 32 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
>>>>>> index 87180c8..288e74c 100644
>>>>>> --- a/hw/pci/pci_host.c
>>>>>> +++ b/hw/pci/pci_host.c
>>>>>> @@ -180,6 +180,12 @@ static const TypeInfo pci_main_host_interface_info = {
>>>>>>       .parent        = TYPE_INTERFACE,
>>>>>>   };
>>>>>>
>>>>>> +static const TypeInfo pci_host_bridge_snooped_interface_info = {
>>>>>> +    .name = TYPE_PCI_HOST_BRIDGE_SNOOPED,
>>>>>> +    .parent = TYPE_INTERFACE,
>>>>>> +    .class_size = sizeof(PCIHostBridgeSnoopedClass),
>>>>>> +};
>>>>>> +
>>>>>>   static const TypeInfo pci_host_type_info = {
>>>>>>       .name = TYPE_PCI_HOST_BRIDGE,
>>>>>>       .parent = TYPE_SYS_BUS_DEVICE,
>>>>>> @@ -191,7 +197,9 @@ static const TypeInfo pci_host_type_info = {
>>>>>>   static void pci_host_register_types(void)
>>>>>>   {
>>>>>>       type_register_static(&pci_main_host_interface_info);
>>>>>> +    type_register_static(&pci_host_bridge_snooped_interface_info);
>>>>>>       type_register_static(&pci_host_type_info);
>>>>>> +
>>>>>
>>>>> extra empty line
>>>> Will take care of it, thanks.
>>>>
>>>>>
>>>>>>   }
>>>>>>
>>>>>>   type_init(pci_host_register_types)
>>>>>> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
>>>>>> index 3c72e26..a041919 100644
>>>>>> --- a/include/hw/pci/pci_host.h
>>>>>> +++ b/include/hw/pci/pci_host.h
>>>>>> @@ -63,6 +63,30 @@ typedef struct PCIHostBridgeClass {
>>>>>>       const char *(*root_bus_path)(PCIHostState *, PCIBus *);
>>>>>>   } PCIHostBridgeClass;
>>>>>>
>>>>>> +/**
>>>>>> + * A special case of host bridge whose configuration registers
>>>>>> + * are snooped by other host bridges to complete their
>>>>>> + * configuration cycles.
>>>>>> + */
>>>>>> +#define TYPE_PCI_HOST_BRIDGE_SNOOPED "pci-host-bridge-snooped"
>>>>>> +#define TYPE_PCI_HOST_BRIDGE_SNOOPED_CLASS(klass) \
>>>>>> +     OBJECT_CLASS_CHECK(PCIHostBridgeSnoopedClass, (klass), \
>>>>>> +                        TYPE_PCI_HOST_BRIDGE_SNOOPED)
>>>>>> +#define PCI_HOST_BRIDGE_SNOOPED_GET_CLASS(obj) \
>>>>>> +     OBJECT_GET_CLASS(PCIHostBridgeSnoopedClass, (obj), \
>>>>>> +                      TYPE_PCI_HOST_BRIDGE_SNOOPED)
>>>>>> +#define PCI_HOST_BRIDGE_SNOOPED(obj) \
>>>>>> +     INTERFACE_CHECK(PCIHostState, (obj), \
>>>>>> +                     TYPE_PCI_HOST_BRIDGE_SNOOPED)
>>>>>> +
>>>>>> +typedef struct PCIHostBridgeSnoopedClass {
>>>>>> +    /* <private> */
>>>>>> +    InterfaceClass parent_class;
>>>>>> +
>>>>>> +    /* <public> */
>>>>>> +    GPtrArray *(*snooping_hosts)(PCIHostState *);
>>>>>
>>>>> Why not just add GPtrArray here directly, and add an API to
>>>>> register/deregister?
>>>> The interface concentrates on usage, letting the way to fill the snooping
>>>> hosts to the specific implementation.
>>>>
>>>> Thanks,
>>>> Marcel
>>>
>>> IMO this is overdoing abstractions.
>> It will provide us with enough elasticity when we'll try the same for Q35.
>> Also this interface is used in several places and it will serve us when we'll go for Q35.
>
> I don't see how it helps, we don't just stick a virtual function
> in front of all new code we write on the assumption it
> helps elasticity.
It helps by letting us querying the interface instead of asking:
Is this host-bridge PIIX ? look for child buses and see if there are roots
else: is this host-bridge ...

>
>>>
>>> But all this might be moot, it's probably better to just
>>> add everyone to the child bus list, and drop the
>>> concept of snooping config cycles.
>> Well, this will be a problem, since the whole concept of the PXB is to snoop
>> on the configuration cycles and this interface gives us a more easy
>> way to understand the implementation.
>>
>> We already discussed this approach and was accepted.
>> The code is small and concise and the use of QOM makes sense in my opinion.
>> I prefer not to re-open it now.
>>
>> Thanks,
>> Marcel
>
> I'm talking about the implementation here.
> We already scan the child bus list on config
> transactions. Why not add your entry there,
> then you can snoop to your heart's content.
Maybe I didn't fully understand what you want.
Do you want to add the new root buses as child buses of root bus 0?
This interface is not used in this context, is used in other scenarios:
[PATCH V6 for-2.3 21/26] hw/pci: inform bios if the system has extra pci root buses
[PATCH V6 for-2.3 15/26] hw/acpi: add support for i440fx 'snooping' root busses

I can look for other usages, can you tell me how it will look without this interface?

Other thing, in config we have pci_data_write calling (in the end) pci_find_bus_nr that does  not support
multiple root buses.
Changing it could affect a lot of functionality and also looking at the function it will not be pretty or clear.
Keeping it at host-bridge level (see  [PATCH V6 for-2.3 14/26] hw/pci-host/piix: implement TYPE_PCI_HOST_BRIDGE_SNOOPED interface)
and having piix query its pxbs for the extra root buses seems a good fit.


Thanks,
Marce
>
>
>
>>>
>>>
>>>>>
>>>>>> +} PCIHostBridgeSnoopedClass;
>>>>>> +
>>>>>>   /* common internal helpers for PCI/PCIe hosts, cut off overflows */
>>>>>>   void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
>>>>>>                                     uint32_t limit, uint32_t val, uint32_t len);
>>>>>> --
>>>>>> 2.1.0