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

[Philippe Mathieu-Daudé]

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 <address@hidden>
> ---
>   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 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.


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

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.

---

Back to your patch, you add asserts() calls because you expect 
SysBusDeviceClass::explicit_ofw_unit_address() to be called before the 
device is plugged on a bus.

This handler is only used by sysbus_get_fw_dev_path(), so 
BusClass::get_dev_path(), similar to the scsi/usb following cases.

BusClass::get_dev_path() is only called in qdev_get_dev_path() were we 
know that dev->parent_bus is not NULL, because checked there.

So the assert is pointless.

> +
>       if (main_host_sbd->num_mmio > 0) {
>           return g_strdup_printf(TARGET_FMT_plx ",%x",
>                                  main_host_sbd->mmio[0].addr, position + 1);
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index ad0e7f6d88..3d9497882b 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -1558,10 +1558,12 @@ void scsi_device_purge_requests(SCSIDevice *sdev, 
> SCSISense sense)
>   static char *scsibus_get_dev_path(DeviceState *dev)
>   {
>       SCSIDevice *d = SCSI_DEVICE(dev);
> -    DeviceState *hba = dev->parent_bus->parent;
> +    DeviceState *hba = qdev_get_bus_device(dev);
>       char *id;
>       char *path;
>   
> +    g_assert(hba);

Similarly, we checked in qdev_get_dev_path().

> +
>       id = qdev_get_dev_path(hba);
>       if (id) {
>           path = g_strdup_printf("%s/%d:%d:%d", id, d->channel, d->id, d->lun);
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index a6522f5429..26bf794315 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -587,9 +587,11 @@ static void usb_bus_dev_print(Monitor *mon, DeviceState 
> *qdev, int indent)
>   static char *usb_get_dev_path(DeviceState *qdev)
>   {
>       USBDevice *dev = USB_DEVICE(qdev);
> -    DeviceState *hcd = qdev->parent_bus->parent;
> +    DeviceState *hcd = qdev_get_bus_device(qdev);
>       char *id = NULL;
>   
> +    g_assert(hcd);

Similarly, we checked in qdev_get_dev_path().

> +
>       if (dev->flags & (1 << USB_DEV_FLAG_FULL_PATH)) {
>           id = qdev_get_dev_path(hcd);
>       }
> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> index 4568db2568..fbb3599ddd 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -1185,10 +1185,12 @@ void ccid_card_send_apdu_to_guest(CCIDCardState *card,
>                                     uint8_t *apdu, uint32_t len)
>   {
>       DeviceState *qdev = DEVICE(card);
> -    USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
> +    USBDevice *dev = USB_DEVICE(qdev_get_bus_device(qdev));
>       USBCCIDState *s = USB_CCID_DEV(dev);
>       Answer *answer;
>   
> +    g_assert(dev);

Skipping this one for now.

> +
>       if (!ccid_has_pending_answers(s)) {
>           DPRINTF(s, 1, "CCID ERROR: got an APDU without pending answers\n");
>           return;
> @@ -1208,9 +1210,11 @@ void ccid_card_send_apdu_to_guest(CCIDCardState *card,
>   void ccid_card_card_removed(CCIDCardState *card)
>   {
>       DeviceState *qdev = DEVICE(card);
> -    USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
> +    USBDevice *dev = USB_DEVICE(qdev_get_bus_device(qdev));
>       USBCCIDState *s = USB_CCID_DEV(dev);
>   
> +    g_assert(dev);

At removal time we assume it was previously inserted.

> +
>       ccid_on_slot_change(s, false);
>       ccid_flush_pending_answers(s);
>       ccid_reset(s);
> @@ -1219,9 +1223,11 @@ void ccid_card_card_removed(CCIDCardState *card)
>   int ccid_card_ccid_attach(CCIDCardState *card)
>   {
>       DeviceState *qdev = DEVICE(card);
> -    USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
> +    USBDevice *dev = USB_DEVICE(qdev_get_bus_device(qdev));
>       USBCCIDState *s = USB_CCID_DEV(dev);
>   
> +    g_assert(dev);

Skipping this one for now.

> +
>       DPRINTF(s, 1, "CCID Attach\n");
>       return 0;
>   }
> @@ -1229,9 +1235,11 @@ int ccid_card_ccid_attach(CCIDCardState *card)
>   void ccid_card_ccid_detach(CCIDCardState *card)
>   {
>       DeviceState *qdev = DEVICE(card);
> -    USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
> +    USBDevice *dev = USB_DEVICE(qdev_get_bus_device(qdev));
>       USBCCIDState *s = USB_CCID_DEV(dev);
>   
> +    g_assert(dev);

At detach time we assume it was attached.

> +
>       DPRINTF(s, 1, "CCID Detach\n");
>       if (ccid_card_inserted(s)) {
>           ccid_on_slot_change(s, false);
> @@ -1242,9 +1250,11 @@ void ccid_card_ccid_detach(CCIDCardState *card)
>   void ccid_card_card_error(CCIDCardState *card, uint64_t error)
>   {
>       DeviceState *qdev = DEVICE(card);
> -    USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
> +    USBDevice *dev = USB_DEVICE(qdev_get_bus_device(qdev));
>       USBCCIDState *s = USB_CCID_DEV(dev);
>   
> +    g_assert(dev);

Skipping this one for now.

> +
>       s->bmCommandStatus = COMMAND_STATUS_FAILED;
>       s->last_answer_error = error;
>       DPRINTF(s, 1, "VSC_Error: %" PRIX64 "\n", s->last_answer_error);
> @@ -1261,9 +1271,11 @@ void ccid_card_card_error(CCIDCardState *card, uint64_t 
> error)
>   void ccid_card_card_inserted(CCIDCardState *card)
>   {
>       DeviceState *qdev = DEVICE(card);
> -    USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
> +    USBDevice *dev = USB_DEVICE(qdev_get_bus_device(qdev));
>       USBCCIDState *s = USB_CCID_DEV(dev);
>   
> +    g_assert(dev);

Skipping this one for now.

> +
>       s->bmCommandStatus = COMMAND_STATUS_NO_ERROR;
>       ccid_flush_pending_answers(s);
>       ccid_on_slot_change(s, true);
> @@ -1273,10 +1285,12 @@ static void ccid_card_unrealize(DeviceState *qdev, 
> Error **errp)
>   {
>       CCIDCardState *card = CCID_CARD(qdev);
>       CCIDCardClass *cc = CCID_CARD_GET_CLASS(card);
> -    USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
> +    USBDevice *dev = USB_DEVICE(qdev_get_bus_device(qdev));
>       USBCCIDState *s = USB_CCID_DEV(dev);
>       Error *local_err = NULL;
>   
> +    g_assert(dev);

Here we assume it was realized.

> +
>       if (ccid_card_inserted(s)) {
>           ccid_card_card_removed(card);
>       }
> @@ -1294,10 +1308,12 @@ static void ccid_card_realize(DeviceState *qdev, Error 
> **errp)
>   {
>       CCIDCardState *card = CCID_CARD(qdev);
>       CCIDCardClass *cc = CCID_CARD_GET_CLASS(card);
> -    USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
> +    USBDevice *dev = USB_DEVICE(qdev_get_bus_device(qdev));
>       USBCCIDState *s = USB_CCID_DEV(dev);
>       Error *local_err = NULL;
>   
> +    g_assert(dev);
> +
>       if (card->slot != 0) {
>           error_setg(errp, "usb-ccid supports one slot, can't add %d",
>                      card->slot);
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f723b9f631..8ce9269aab 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1317,9 +1317,21 @@ static uint64_t virtio_pci_notify_read(void *opaque, 
> hwaddr addr,
>   static void virtio_pci_notify_write(void *opaque, hwaddr addr,
>                                       uint64_t val, unsigned size)
>   {
> +    DeviceState *dev = DEVICE(opaque);
>       VirtIODevice *vdev = opaque;
> -    VirtIOPCIProxy *proxy = VIRTIO_PCI(DEVICE(vdev)->parent_bus->parent);
> -    unsigned queue = addr / virtio_pci_queue_mem_mult(proxy);
> +    VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev_get_bus_device(dev));
> +    unsigned queue;
> +
> +    /*
> +     * During unplug virtio device may have
> +     * already been disconnected from the bus
> +     */
> +    if (!proxy) {

So VIRTIO_PCI() calls OBJECT_CHECK().

/*
 * OBJECT_CHECK:
 * If an invalid object is passed to this function, a run time assert 
will be
 * generated.
 */
#define OBJECT_CHECK(type, obj, name) \
    ((type *)object_dynamic_cast_assert(OBJECT(obj), (name), \
                                        __FILE__, __LINE__, __func__))

Looking at object_dynamic_cast_assert(), even building with 
CONFIG_QOM_CAST_DEBUG, it looks passing obj=NULL will return NULL...

OK, so this check makes sense. I wonder how you got there. No bugreport 
or crash mentioned in this patch or the cover. Googling for 
'virtio_pci_notify_write' I found 
https://www.mail-archive.com/address@hidden/msg667664.html which 
is not filled as a Launchpad QEMU bug. The reporter wrote "but parent 
BusState was already freed & set to NULL." If you are trying to fix a 
bug, it would help if you give the context to the reviewers, at least 
the backtrace would have saved me some minutes of the 2 hours+ I'm 
looking at this.

> +        warn_report("Device %s doesn't have parent bus", vdev->name);
> +        return;
> +    }
> +
> +    queue = addr / virtio_pci_queue_mem_mult(proxy);
>   
>       if (queue < VIRTIO_QUEUE_MAX) {
>           virtio_queue_notify(vdev, queue);
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 1518495b1e..05d68f0f1a 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -452,6 +452,8 @@ Object *qdev_get_machine(void);
>   /* FIXME: make this a link<> */
>   void qdev_set_parent_bus(DeviceState *dev, BusState *bus);
>   
> +DeviceState *qdev_get_bus_device(const DeviceState *dev);
> +
>   extern bool qdev_hotplug;
>   extern bool qdev_hot_removed;
>   

I suggest you split this patch in:

1/ introduce qdev_get_bus_device()
-  hw/core/qdev.c
-  include/hw/qdev-core.h

2/ obvious use of qdev_get_bus_device(), no assert
- hw/pci-bridge/pci_expander_bridge.c
- hw/scsi/scsi-bus.c
- hw/usb/bus.c

3/ use of qdev_get_bus_device(), assertions
- hw/usb/dev-smartcard-reader.c

4a/ use qdev_get_bus_device()
- hw/virtio/virtio-pci.c

4b/ !proxy bugfix
- hw/virtio/virtio-pci.c

In 4b/ please mention the bug report and backtrace.

You might squash 1 + 4a or 4a + 4b.

Regards,

Phil.