[PATCH 0/2] virtio-balloon: Reject qmp_balloon during hot-unplug

[PATCH 0/2] virtio-balloon: Reject qmp_balloon during hot-unplug

[Julia Suvorova]

Julia Suvorova (2):
  qdev: Introduce qdev_get_bus_device
  virtio-balloon: Reject qmp_balloon during hot-unplug

 balloon.c                           |  2 +-
 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-balloon.c          |  9 +++++++-
 hw/virtio/virtio-pci.c              | 16 +++++++++++++--
 include/hw/qdev-core.h              |  2 ++
 include/sysemu/balloon.h            |  2 +-
 10 files changed, 64 insertions(+), 16 deletions(-)

-- 
2.24.1

[PATCH 1/2] qdev: Introduce qdev_get_bus_device

[Julia Suvorova]

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

[PATCH 2/2] virtio-balloon: Reject qmp_balloon during hot-unplug

[Julia Suvorova]

Hot-unplug takes some time due to communication with the guest.
Do not change the device while freeing up resources.

Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
 balloon.c                  | 2 +-
 hw/virtio/virtio-balloon.c | 9 ++++++++-
 include/sysemu/balloon.h   | 2 +-
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/balloon.c b/balloon.c
index f104b42961..998ec53a0f 100644
--- a/balloon.c
+++ b/balloon.c
@@ -119,5 +119,5 @@ void qmp_balloon(int64_t target, Error **errp)
     }
 
     trace_balloon_event(balloon_opaque, target);
-    balloon_event_fn(balloon_opaque, target);
+    balloon_event_fn(balloon_opaque, target, errp);
 }
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 57f3b9f22d..0fa4e4454b 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -717,12 +717,19 @@ static void virtio_balloon_stat(void *opaque, BalloonInfo *info)
                                              VIRTIO_BALLOON_PFN_SHIFT);
 }
 
-static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
+static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
+                                     Error **errp)
 {
+    DeviceState *bus_dev = qdev_get_bus_device(DEVICE(opaque));
     VirtIOBalloon *dev = VIRTIO_BALLOON(opaque);
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     ram_addr_t vm_ram_size = get_current_ram_size();
 
+    if (bus_dev && bus_dev->pending_deleted_event) {
+        error_setg(errp, "Hot-unplug of %s is in progress", vdev->name);
+        return;
+    }
+
     if (target > vm_ram_size) {
         target = vm_ram_size;
     }
diff --git a/include/sysemu/balloon.h b/include/sysemu/balloon.h
index aea0c44985..b3a09ca946 100644
--- a/include/sysemu/balloon.h
+++ b/include/sysemu/balloon.h
@@ -17,7 +17,7 @@
 #include "exec/cpu-common.h"
 #include "qapi/qapi-types-misc.h"
 
-typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
+typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target, Error **errp);
 typedef void (QEMUBalloonStatus)(void *opaque, BalloonInfo *info);
 
 int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
-- 
2.24.1

Re: [PATCH 2/2] virtio-balloon: Reject qmp_balloon during hot-unplug

[David Hildenbrand]

On 15.01.20 23:40, Julia Suvorova wrote:
> Hot-unplug takes some time due to communication with the guest.
> Do not change the device while freeing up resources.
> 
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
>  balloon.c                  | 2 +-
>  hw/virtio/virtio-balloon.c | 9 ++++++++-
>  include/sysemu/balloon.h   | 2 +-
>  3 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/balloon.c b/balloon.c
> index f104b42961..998ec53a0f 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -119,5 +119,5 @@ void qmp_balloon(int64_t target, Error **errp)
>      }
>  
>      trace_balloon_event(balloon_opaque, target);
> -    balloon_event_fn(balloon_opaque, target);
> +    balloon_event_fn(balloon_opaque, target, errp);
>  }
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 57f3b9f22d..0fa4e4454b 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -717,12 +717,19 @@ static void virtio_balloon_stat(void *opaque, BalloonInfo *info)
>                                               VIRTIO_BALLOON_PFN_SHIFT);
>  }
>  
> -static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
> +static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
> +                                     Error **errp)
>  {
> +    DeviceState *bus_dev = qdev_get_bus_device(DEVICE(opaque));
>      VirtIOBalloon *dev = VIRTIO_BALLOON(opaque);
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      ram_addr_t vm_ram_size = get_current_ram_size();
>  
> +    if (bus_dev && bus_dev->pending_deleted_event) {
> +        error_setg(errp, "Hot-unplug of %s is in progress", vdev->name);
> +        return;
> +    }
> +

How exactly does this help? The guest is free to inflate/deflate
whenever it wants.


-- 
Thanks,

David / dhildenb

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 <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.

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

[Markus Armbruster]

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?

[...]

Re: [PATCH 2/2] virtio-balloon: Reject qmp_balloon during hot-unplug

[Julia Suvorova]

On Thu, Jan 16, 2020 at 1:36 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 15.01.20 23:40, Julia Suvorova wrote:
> > Hot-unplug takes some time due to communication with the guest.
> > Do not change the device while freeing up resources.
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> >  balloon.c                  | 2 +-
> >  hw/virtio/virtio-balloon.c | 9 ++++++++-
> >  include/sysemu/balloon.h   | 2 +-
> >  3 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/balloon.c b/balloon.c
> > index f104b42961..998ec53a0f 100644
> > --- a/balloon.c
> > +++ b/balloon.c
> > @@ -119,5 +119,5 @@ void qmp_balloon(int64_t target, Error **errp)
> >      }
> >
> >      trace_balloon_event(balloon_opaque, target);
> > -    balloon_event_fn(balloon_opaque, target);
> > +    balloon_event_fn(balloon_opaque, target, errp);
> >  }
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index 57f3b9f22d..0fa4e4454b 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -717,12 +717,19 @@ static void virtio_balloon_stat(void *opaque, BalloonInfo *info)
> >                                               VIRTIO_BALLOON_PFN_SHIFT);
> >  }
> >
> > -static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
> > +static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
> > +                                     Error **errp)
> >  {
> > +    DeviceState *bus_dev = qdev_get_bus_device(DEVICE(opaque));
> >      VirtIOBalloon *dev = VIRTIO_BALLOON(opaque);
> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >      ram_addr_t vm_ram_size = get_current_ram_size();
> >
> > +    if (bus_dev && bus_dev->pending_deleted_event) {
> > +        error_setg(errp, "Hot-unplug of %s is in progress", vdev->name);
> > +        return;
> > +    }
> > +
>
> How exactly does this help? The guest is free to inflate/deflate
> whenever it wants.

Guest is aware of hot-unplug start, and virtio driver should not
initiate any operations. This patch just restricts issuing commands
from qmp monitor.

Best regards, Julia Suvorova.

Re: [PATCH 2/2] virtio-balloon: Reject qmp_balloon during hot-unplug

[David Hildenbrand]

On 23.01.20 15:08, Julia Suvorova wrote:
> On Thu, Jan 16, 2020 at 1:36 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 15.01.20 23:40, Julia Suvorova wrote:
>>> Hot-unplug takes some time due to communication with the guest.
>>> Do not change the device while freeing up resources.
>>>
>>> Signed-off-by: Julia Suvorova <jusual@redhat.com>
>>> ---
>>>  balloon.c                  | 2 +-
>>>  hw/virtio/virtio-balloon.c | 9 ++++++++-
>>>  include/sysemu/balloon.h   | 2 +-
>>>  3 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/balloon.c b/balloon.c
>>> index f104b42961..998ec53a0f 100644
>>> --- a/balloon.c
>>> +++ b/balloon.c
>>> @@ -119,5 +119,5 @@ void qmp_balloon(int64_t target, Error **errp)
>>>      }
>>>
>>>      trace_balloon_event(balloon_opaque, target);
>>> -    balloon_event_fn(balloon_opaque, target);
>>> +    balloon_event_fn(balloon_opaque, target, errp);
>>>  }
>>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>>> index 57f3b9f22d..0fa4e4454b 100644
>>> --- a/hw/virtio/virtio-balloon.c
>>> +++ b/hw/virtio/virtio-balloon.c
>>> @@ -717,12 +717,19 @@ static void virtio_balloon_stat(void *opaque, BalloonInfo *info)
>>>                                               VIRTIO_BALLOON_PFN_SHIFT);
>>>  }
>>>
>>> -static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
>>> +static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
>>> +                                     Error **errp)
>>>  {
>>> +    DeviceState *bus_dev = qdev_get_bus_device(DEVICE(opaque));
>>>      VirtIOBalloon *dev = VIRTIO_BALLOON(opaque);
>>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>>      ram_addr_t vm_ram_size = get_current_ram_size();
>>>
>>> +    if (bus_dev && bus_dev->pending_deleted_event) {
>>> +        error_setg(errp, "Hot-unplug of %s is in progress", vdev->name);
>>> +        return;
>>> +    }
>>> +
>>
>> How exactly does this help? The guest is free to inflate/deflate
>> whenever it wants.
> 
> Guest is aware of hot-unplug start, and virtio driver should not
> initiate any operations. This patch just restricts issuing commands
> from qmp monitor.

Why shouldn't the guest driver inflate/deflate while memory hotplug is
going on?

Simple balloon compaction in a Linux guest -> deflate/inflate triggered
in the hypervisor.

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/2] virtio-balloon: Reject qmp_balloon during hot-unplug

[Julia Suvorova]

On Thu, Jan 23, 2020 at 3:17 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 23.01.20 15:08, Julia Suvorova wrote:
> > On Thu, Jan 16, 2020 at 1:36 PM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 15.01.20 23:40, Julia Suvorova wrote:
> >>> Hot-unplug takes some time due to communication with the guest.
> >>> Do not change the device while freeing up resources.
> >>>
> >>> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> >>> ---
> >>>  balloon.c                  | 2 +-
> >>>  hw/virtio/virtio-balloon.c | 9 ++++++++-
> >>>  include/sysemu/balloon.h   | 2 +-
> >>>  3 files changed, 10 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/balloon.c b/balloon.c
> >>> index f104b42961..998ec53a0f 100644
> >>> --- a/balloon.c
> >>> +++ b/balloon.c
> >>> @@ -119,5 +119,5 @@ void qmp_balloon(int64_t target, Error **errp)
> >>>      }
> >>>
> >>>      trace_balloon_event(balloon_opaque, target);
> >>> -    balloon_event_fn(balloon_opaque, target);
> >>> +    balloon_event_fn(balloon_opaque, target, errp);
> >>>  }
> >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> >>> index 57f3b9f22d..0fa4e4454b 100644
> >>> --- a/hw/virtio/virtio-balloon.c
> >>> +++ b/hw/virtio/virtio-balloon.c
> >>> @@ -717,12 +717,19 @@ static void virtio_balloon_stat(void *opaque, BalloonInfo *info)
> >>>                                               VIRTIO_BALLOON_PFN_SHIFT);
> >>>  }
> >>>
> >>> -static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
> >>> +static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
> >>> +                                     Error **errp)
> >>>  {
> >>> +    DeviceState *bus_dev = qdev_get_bus_device(DEVICE(opaque));
> >>>      VirtIOBalloon *dev = VIRTIO_BALLOON(opaque);
> >>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >>>      ram_addr_t vm_ram_size = get_current_ram_size();
> >>>
> >>> +    if (bus_dev && bus_dev->pending_deleted_event) {
> >>> +        error_setg(errp, "Hot-unplug of %s is in progress", vdev->name);
> >>> +        return;
> >>> +    }
> >>> +
> >>
> >> How exactly does this help? The guest is free to inflate/deflate
> >> whenever it wants.
> >
> > Guest is aware of hot-unplug start, and virtio driver should not
> > initiate any operations. This patch just restricts issuing commands
> > from qmp monitor.
>
> Why shouldn't the guest driver inflate/deflate while memory hotplug is
> going on?
>
> Simple balloon compaction in a Linux guest -> deflate/inflate triggered
> in the hypervisor.

QEMU crashes if inflate happens after powering-off PCI slot. Guest is
unable to interact with virtio-balloon device then, driver is
unloaded. But inflate can still happen if initiated from qmp.

Best regards, Julia Suvorova.

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

[Igor Mammedov]

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;
>  

Re: [PATCH 2/2] virtio-balloon: Reject qmp_balloon during hot-unplug

[David Hildenbrand]

On 23.01.20 16:46, Julia Suvorova wrote:
> On Thu, Jan 23, 2020 at 3:17 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 23.01.20 15:08, Julia Suvorova wrote:
>>> On Thu, Jan 16, 2020 at 1:36 PM David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>> On 15.01.20 23:40, Julia Suvorova wrote:
>>>>> Hot-unplug takes some time due to communication with the guest.
>>>>> Do not change the device while freeing up resources.
>>>>>
>>>>> Signed-off-by: Julia Suvorova <jusual@redhat.com>
>>>>> ---
>>>>>  balloon.c                  | 2 +-
>>>>>  hw/virtio/virtio-balloon.c | 9 ++++++++-
>>>>>  include/sysemu/balloon.h   | 2 +-
>>>>>  3 files changed, 10 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/balloon.c b/balloon.c
>>>>> index f104b42961..998ec53a0f 100644
>>>>> --- a/balloon.c
>>>>> +++ b/balloon.c
>>>>> @@ -119,5 +119,5 @@ void qmp_balloon(int64_t target, Error **errp)
>>>>>      }
>>>>>
>>>>>      trace_balloon_event(balloon_opaque, target);
>>>>> -    balloon_event_fn(balloon_opaque, target);
>>>>> +    balloon_event_fn(balloon_opaque, target, errp);
>>>>>  }
>>>>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>>>>> index 57f3b9f22d..0fa4e4454b 100644
>>>>> --- a/hw/virtio/virtio-balloon.c
>>>>> +++ b/hw/virtio/virtio-balloon.c
>>>>> @@ -717,12 +717,19 @@ static void virtio_balloon_stat(void *opaque, BalloonInfo *info)
>>>>>                                               VIRTIO_BALLOON_PFN_SHIFT);
>>>>>  }
>>>>>
>>>>> -static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
>>>>> +static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
>>>>> +                                     Error **errp)
>>>>>  {
>>>>> +    DeviceState *bus_dev = qdev_get_bus_device(DEVICE(opaque));
>>>>>      VirtIOBalloon *dev = VIRTIO_BALLOON(opaque);
>>>>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>>>>      ram_addr_t vm_ram_size = get_current_ram_size();
>>>>>
>>>>> +    if (bus_dev && bus_dev->pending_deleted_event) {
>>>>> +        error_setg(errp, "Hot-unplug of %s is in progress", vdev->name);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>
>>>> How exactly does this help? The guest is free to inflate/deflate
>>>> whenever it wants.
>>>
>>> Guest is aware of hot-unplug start, and virtio driver should not
>>> initiate any operations. This patch just restricts issuing commands
>>> from qmp monitor.
>>
>> Why shouldn't the guest driver inflate/deflate while memory hotplug is
>> going on?
>>
>> Simple balloon compaction in a Linux guest -> deflate/inflate triggered
>> in the hypervisor.
> 
> QEMU crashes if inflate happens after powering-off PCI slot. Guest is
> unable to interact with virtio-balloon device then, driver is
> unloaded. But inflate can still happen if initiated from qmp.

Ah guest can inflate/deflate whenever it wants. What am I missing?

IMHO, the right approach is to discard inflate/deflate requests on these
areas instead.


-- 
Thanks,

David / dhildenb

Re: [PATCH 2/2] virtio-balloon: Reject qmp_balloon during hot-unplug

[Michael S. Tsirkin]

On Thu, Jan 23, 2020 at 04:46:18PM +0100, Julia Suvorova wrote:
> On Thu, Jan 23, 2020 at 3:17 PM David Hildenbrand <david@redhat.com> wrote:
> >
> > On 23.01.20 15:08, Julia Suvorova wrote:
> > > On Thu, Jan 16, 2020 at 1:36 PM David Hildenbrand <david@redhat.com> wrote:
> > >>
> > >> On 15.01.20 23:40, Julia Suvorova wrote:
> > >>> Hot-unplug takes some time due to communication with the guest.
> > >>> Do not change the device while freeing up resources.
> > >>>
> > >>> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > >>> ---
> > >>>  balloon.c                  | 2 +-
> > >>>  hw/virtio/virtio-balloon.c | 9 ++++++++-
> > >>>  include/sysemu/balloon.h   | 2 +-
> > >>>  3 files changed, 10 insertions(+), 3 deletions(-)
> > >>>
> > >>> diff --git a/balloon.c b/balloon.c
> > >>> index f104b42961..998ec53a0f 100644
> > >>> --- a/balloon.c
> > >>> +++ b/balloon.c
> > >>> @@ -119,5 +119,5 @@ void qmp_balloon(int64_t target, Error **errp)
> > >>>      }
> > >>>
> > >>>      trace_balloon_event(balloon_opaque, target);
> > >>> -    balloon_event_fn(balloon_opaque, target);
> > >>> +    balloon_event_fn(balloon_opaque, target, errp);
> > >>>  }
> > >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > >>> index 57f3b9f22d..0fa4e4454b 100644
> > >>> --- a/hw/virtio/virtio-balloon.c
> > >>> +++ b/hw/virtio/virtio-balloon.c
> > >>> @@ -717,12 +717,19 @@ static void virtio_balloon_stat(void *opaque, BalloonInfo *info)
> > >>>                                               VIRTIO_BALLOON_PFN_SHIFT);
> > >>>  }
> > >>>
> > >>> -static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
> > >>> +static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
> > >>> +                                     Error **errp)
> > >>>  {
> > >>> +    DeviceState *bus_dev = qdev_get_bus_device(DEVICE(opaque));
> > >>>      VirtIOBalloon *dev = VIRTIO_BALLOON(opaque);
> > >>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > >>>      ram_addr_t vm_ram_size = get_current_ram_size();
> > >>>
> > >>> +    if (bus_dev && bus_dev->pending_deleted_event) {
> > >>> +        error_setg(errp, "Hot-unplug of %s is in progress", vdev->name);
> > >>> +        return;
> > >>> +    }
> > >>> +
> > >>
> > >> How exactly does this help? The guest is free to inflate/deflate
> > >> whenever it wants.
> > >
> > > Guest is aware of hot-unplug start, and virtio driver should not
> > > initiate any operations. This patch just restricts issuing commands
> > > from qmp monitor.
> >
> > Why shouldn't the guest driver inflate/deflate while memory hotplug is
> > going on?
> >
> > Simple balloon compaction in a Linux guest -> deflate/inflate triggered
> > in the hypervisor.
> 
> QEMU crashes if inflate happens after powering-off PCI slot. Guest is
> unable to interact with virtio-balloon device then, driver is
> unloaded. But inflate can still happen if initiated from qmp.
> 
> Best regards, Julia Suvorova.

Hot-unplug in progress is probably a hack - we should
prevent access when device is powered off.
Also, it would appear that have_balloon is the correct place for this
kind of check.

-- 
MST

Re: [PATCH 2/2] virtio-balloon: Reject qmp_balloon during hot-unplug

[Julia Suvorova]

On Mon, Jan 27, 2020 at 11:54 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Jan 23, 2020 at 04:46:18PM +0100, Julia Suvorova wrote:
> > On Thu, Jan 23, 2020 at 3:17 PM David Hildenbrand <david@redhat.com> wrote:
> > >
> > > On 23.01.20 15:08, Julia Suvorova wrote:
> > > > On Thu, Jan 16, 2020 at 1:36 PM David Hildenbrand <david@redhat.com> wrote:
> > > >>
> > > >> On 15.01.20 23:40, Julia Suvorova wrote:
> > > >>> Hot-unplug takes some time due to communication with the guest.
> > > >>> Do not change the device while freeing up resources.
> > > >>>
> > > >>> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > >>> ---
> > > >>>  balloon.c                  | 2 +-
> > > >>>  hw/virtio/virtio-balloon.c | 9 ++++++++-
> > > >>>  include/sysemu/balloon.h   | 2 +-
> > > >>>  3 files changed, 10 insertions(+), 3 deletions(-)
> > > >>>
> > > >>> diff --git a/balloon.c b/balloon.c
> > > >>> index f104b42961..998ec53a0f 100644
> > > >>> --- a/balloon.c
> > > >>> +++ b/balloon.c
> > > >>> @@ -119,5 +119,5 @@ void qmp_balloon(int64_t target, Error **errp)
> > > >>>      }
> > > >>>
> > > >>>      trace_balloon_event(balloon_opaque, target);
> > > >>> -    balloon_event_fn(balloon_opaque, target);
> > > >>> +    balloon_event_fn(balloon_opaque, target, errp);
> > > >>>  }
> > > >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > > >>> index 57f3b9f22d..0fa4e4454b 100644
> > > >>> --- a/hw/virtio/virtio-balloon.c
> > > >>> +++ b/hw/virtio/virtio-balloon.c
> > > >>> @@ -717,12 +717,19 @@ static void virtio_balloon_stat(void *opaque, BalloonInfo *info)
> > > >>>                                               VIRTIO_BALLOON_PFN_SHIFT);
> > > >>>  }
> > > >>>
> > > >>> -static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
> > > >>> +static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
> > > >>> +                                     Error **errp)
> > > >>>  {
> > > >>> +    DeviceState *bus_dev = qdev_get_bus_device(DEVICE(opaque));
> > > >>>      VirtIOBalloon *dev = VIRTIO_BALLOON(opaque);
> > > >>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > > >>>      ram_addr_t vm_ram_size = get_current_ram_size();
> > > >>>
> > > >>> +    if (bus_dev && bus_dev->pending_deleted_event) {
> > > >>> +        error_setg(errp, "Hot-unplug of %s is in progress", vdev->name);
> > > >>> +        return;
> > > >>> +    }
> > > >>> +
> > > >>
> > > >> How exactly does this help? The guest is free to inflate/deflate
> > > >> whenever it wants.
> > > >
> > > > Guest is aware of hot-unplug start, and virtio driver should not
> > > > initiate any operations. This patch just restricts issuing commands
> > > > from qmp monitor.
> > >
> > > Why shouldn't the guest driver inflate/deflate while memory hotplug is
> > > going on?
> > >
> > > Simple balloon compaction in a Linux guest -> deflate/inflate triggered
> > > in the hypervisor.
> >
> > QEMU crashes if inflate happens after powering-off PCI slot. Guest is
> > unable to interact with virtio-balloon device then, driver is
> > unloaded. But inflate can still happen if initiated from qmp.
> >
> > Best regards, Julia Suvorova.
>
> Hot-unplug in progress is probably a hack - we should
> prevent access when device is powered off.
> Also, it would appear that have_balloon is the correct place for this
> kind of check.

You're right, it is a hack. I can add another handler to check if
device is powered on, and check it in have_balloon(),
In assumption that have_balloon means than we can use balloon when
it's presented, it is fine. But it will get more complicated once
hidden devices are here.

Best regards, Julia Suvorova.

Re: [PATCH 2/2] virtio-balloon: Reject qmp_balloon during hot-unplug

[Michael S. Tsirkin]

On Mon, Jan 27, 2020 at 03:53:06PM +0100, Julia Suvorova wrote:
> On Mon, Jan 27, 2020 at 11:54 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Jan 23, 2020 at 04:46:18PM +0100, Julia Suvorova wrote:
> > > On Thu, Jan 23, 2020 at 3:17 PM David Hildenbrand <david@redhat.com> wrote:
> > > >
> > > > On 23.01.20 15:08, Julia Suvorova wrote:
> > > > > On Thu, Jan 16, 2020 at 1:36 PM David Hildenbrand <david@redhat.com> wrote:
> > > > >>
> > > > >> On 15.01.20 23:40, Julia Suvorova wrote:
> > > > >>> Hot-unplug takes some time due to communication with the guest.
> > > > >>> Do not change the device while freeing up resources.
> > > > >>>
> > > > >>> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > > >>> ---
> > > > >>>  balloon.c                  | 2 +-
> > > > >>>  hw/virtio/virtio-balloon.c | 9 ++++++++-
> > > > >>>  include/sysemu/balloon.h   | 2 +-
> > > > >>>  3 files changed, 10 insertions(+), 3 deletions(-)
> > > > >>>
> > > > >>> diff --git a/balloon.c b/balloon.c
> > > > >>> index f104b42961..998ec53a0f 100644
> > > > >>> --- a/balloon.c
> > > > >>> +++ b/balloon.c
> > > > >>> @@ -119,5 +119,5 @@ void qmp_balloon(int64_t target, Error **errp)
> > > > >>>      }
> > > > >>>
> > > > >>>      trace_balloon_event(balloon_opaque, target);
> > > > >>> -    balloon_event_fn(balloon_opaque, target);
> > > > >>> +    balloon_event_fn(balloon_opaque, target, errp);
> > > > >>>  }
> > > > >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > > > >>> index 57f3b9f22d..0fa4e4454b 100644
> > > > >>> --- a/hw/virtio/virtio-balloon.c
> > > > >>> +++ b/hw/virtio/virtio-balloon.c
> > > > >>> @@ -717,12 +717,19 @@ static void virtio_balloon_stat(void *opaque, BalloonInfo *info)
> > > > >>>                                               VIRTIO_BALLOON_PFN_SHIFT);
> > > > >>>  }
> > > > >>>
> > > > >>> -static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
> > > > >>> +static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
> > > > >>> +                                     Error **errp)
> > > > >>>  {
> > > > >>> +    DeviceState *bus_dev = qdev_get_bus_device(DEVICE(opaque));
> > > > >>>      VirtIOBalloon *dev = VIRTIO_BALLOON(opaque);
> > > > >>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > > > >>>      ram_addr_t vm_ram_size = get_current_ram_size();
> > > > >>>
> > > > >>> +    if (bus_dev && bus_dev->pending_deleted_event) {
> > > > >>> +        error_setg(errp, "Hot-unplug of %s is in progress", vdev->name);
> > > > >>> +        return;
> > > > >>> +    }
> > > > >>> +
> > > > >>
> > > > >> How exactly does this help? The guest is free to inflate/deflate
> > > > >> whenever it wants.
> > > > >
> > > > > Guest is aware of hot-unplug start, and virtio driver should not
> > > > > initiate any operations. This patch just restricts issuing commands
> > > > > from qmp monitor.
> > > >
> > > > Why shouldn't the guest driver inflate/deflate while memory hotplug is
> > > > going on?
> > > >
> > > > Simple balloon compaction in a Linux guest -> deflate/inflate triggered
> > > > in the hypervisor.
> > >
> > > QEMU crashes if inflate happens after powering-off PCI slot. Guest is
> > > unable to interact with virtio-balloon device then, driver is
> > > unloaded. But inflate can still happen if initiated from qmp.
> > >
> > > Best regards, Julia Suvorova.
> >
> > Hot-unplug in progress is probably a hack - we should
> > prevent access when device is powered off.
> > Also, it would appear that have_balloon is the correct place for this
> > kind of check.
> 
> You're right, it is a hack. I can add another handler to check if
> device is powered on, and check it in have_balloon(),
> In assumption that have_balloon means than we can use balloon when
> it's presented, it is fine. But it will get more complicated once
> hidden devices are here.
> 
> Best regards, Julia Suvorova.

So thinking about it, all we need to do is make sure
powered off devices don't send interrupts via msi or
intx. Changing config space should be fine ...
No?

-- 
MST