Philippe Mathieu-Daudé <phi...@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 <jus...@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?
[...]
