Re: [PATCH 1/2] qdev: Introduce qdev_get_bus_device

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Hi Julia,
>
> Cc'ing Markus for the qdev/qbus analysis.
>
> On 1/15/20 11:40 PM, Julia Suvorova wrote:
>> For bus devices, it is useful to be able to handle the parent device.
>>
>> Signed-off-by: Julia Suvorova <jusual@redhat.com>
>> ---
>>   hw/core/qdev.c                      |  5 +++++
>>   hw/pci-bridge/pci_expander_bridge.c |  4 +++-
>>   hw/scsi/scsi-bus.c                  |  4 +++-
>>   hw/usb/bus.c                        |  4 +++-
>>   hw/usb/dev-smartcard-reader.c       | 32 +++++++++++++++++++++--------
>>   hw/virtio/virtio-pci.c              | 16 +++++++++++++--
>>   include/hw/qdev-core.h              |  2 ++
>
> Please consider using the scripts/git.orderfile config.
>
>>   7 files changed, 54 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 9f1753f5cf..ad8226e240 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -114,6 +114,11 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
>>       }
>>   }
>>   +DeviceState *qdev_get_bus_device(const DeviceState *dev)
>
> We have qdev_get_bus_hotplug_handler(), this follow the naming, OK.
>
>> +{
>> +    return dev->parent_bus ? dev->parent_bus->parent : NULL;
>> +}
>> +
>>   /* Create a new device.  This only initializes the device state
>>      structure and allows properties to be set.  The device still needs
>>      to be realized.  See qdev-core.h.  */
>> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
>> index 0592818447..63a6c07406 100644
>> --- a/hw/pci-bridge/pci_expander_bridge.c
>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>> @@ -125,9 +125,11 @@ static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
>>       assert(position >= 0);
>>         pxb_dev_base = DEVICE(pxb_dev);
>> -    main_host = PCI_HOST_BRIDGE(pxb_dev_base->parent_bus->parent);
>> +    main_host = PCI_HOST_BRIDGE(qdev_get_bus_device(pxb_dev_base));
>>       main_host_sbd = SYS_BUS_DEVICE(main_host);
>>   +    g_assert(main_host);
>
> I found myself stuck reviewing this patch for 25min, I'm not sure
> what's bugging me yet, so I'll take notes a-la-Markus-style.
>
> We have the qdev API, with DeviceState.
>
>
> We have the qbus API, with BusState.
>
> A BusState is not a DeviceState but a raw Object.

It's a completely separate kind of Object.

> It keeps a pointer to the a DeviceState parent, a HotplugHandler, and
> a list of BusChild.
>
>
> BusChild are neither DeviceState nor Object, but keep a pointer the a
> DeviceState.

It's a thin wrapper around DeviceState to support collecting the
DeviceState into a list.

> TYPE_HOTPLUG_HANDLER is an interface. It can be implemented by any
> object, but its API seems expects a DeviceState as argument.

What do you mean by "interface expects an argument"?

The interface methods all take a HotplugHandler * and a DeviceState *.
The latter is the device being plugged / unplugged, the former is its
hotplug handler.  In the generic case, @dev's hotplug handler is
qdev_get_hotplug_handler(dev).

> Looking at examples implementing TYPE_HOTPLUG_HANDLER:
>
> - TYPE_USB_BUS. It inherits TYPE_BUS. Handlers will be called with
> USBDevice as argument (TYPE_USB_DEVICE -> TYPE_DEVICE).
>
> - TYPE_PCI_BRIDGE_DEV. Inherits TYPE_PCI_BRIDGE -> TYPE_PCI_DEVICE -> 
> TYPE_DEVICE. Handlers expects PCIDevice (TYPE_PCI_DEVICE).
>
> - TYPE_PC_MACHINE. It inherits TYPE_X86_MACHINE -> TYPE_MACHINE -> 
> TYPE_OBJECT. Not a TYPE_BUS. Handlers for TYPE_PC_DIMM, TYPE_CPU and
> TYPE_VIRTIO_PMEM_PCI. Complex... TYPE_PC_DIMM/TYPE_CPU are
> TYPE_DEVICE.
> For TYPE_VIRTIO_PMEM_PCI we have VirtIOPMEMPCI -> VirtIOPCIProxy -> 
> PCIDevice.
>
> - USB_CCID_DEV. Inherits TYPE_USB_DEVICE -> TYPE_DEVICE. Only one
> 'unplug' handler which likely expects USBCCIDState.
>
> - TYPE_SCSI_BUS. Inherits TYPE_BUS. Also a single 'unplug' handler
> expecting SCSIDevice.
>
> - TYPE_VIRTIO_SCSI. Inherits TYPE_VIRTIO_SCSI_COMMON -> 
> TYPE_VIRTIO_DEVICE -> TYPE_DEVICE. Handlers expect VirtIOSCSI.
>
>
> No simple pattern so far.
>
>
> Looking back at qbus. qbus_initfn() enforces a TYPE_HOTPLUG_HANDLER
> property on BusState (which is not a DeviceState). So IIUC TYPE_BUS
> also implements TYPE_HOTPLUG_HANDLER.

I think this merely creates a reference to the concrete bus's hotplug
handler.  TYPE_BUS is abstract, and doesn't implement any interfaces
(its .interfaces is empty).

Anything else you'd like me to check for you?

[...]