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 ++
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)
+{
+ 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);
+
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);
+
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);
+
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);
+
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);
+
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);
+
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);
+
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);
+
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);
+
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);
+
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) {
+ 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;
--
2.24.1
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 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/qemu-devel@nongnu.org/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.
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?
[...]
On Wed, 15 Jan 2020 23:40:24 +0100
Julia Suvorova <jusual@redhat.com> wrote:
I's add () at the end of SUJ so it would be obvious that's a function
> For bus devices, it is useful to be able to handle the parent device.
maybe something like that would be more clear:
Add a wrapper qdev_get_bus_device() to replace dev->parent_bus->parent,
(add why here)
>
> 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 ++
> 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)
> +{
> + return dev->parent_bus ? dev->parent_bus->parent : NULL;
Does any caller expect to get NULL?
If not I'd move asserts you introduce below to this place only
and drop condition.
> +}
> +
> /* 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);
> +
> 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);
> +
> 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);
> +
> 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);
> +
> 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);
> +
> 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);
> +
> 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);
> +
> 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);
> +
> 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);
> +
> 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);
> +
> 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) {
> + warn_report("Device %s doesn't have parent bus", vdev->name);
> + return;
> + }
> +
> + queue = addr / virtio_pci_queue_mem_mult(proxy);
modulo qdev_get_bus_device() change, the rest of this hunk looks
to unrelated to this patch.
I'd split it out into separate patch with proper commit message.
> 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;
>
© 2016 - 2026 Red Hat, Inc.