[Qemu-devel] [PATCH 00/30] complete conversion to hotplug-handler API

[Qemu-devel] [PATCH 00/30] complete conversion to hotplug-handler API

[Igor Mammedov]

Series unifies different hotplug mechanisms to a recent
hotplug-handler API and does shallow conversion of
devices that still use legacy qdev hotplug to it dropping
not used after that legacy hotplug path [29/30].
It also relaces SCSI's own way to do hotplug/unplug with
hotplug-handler callbacks leaving it the only method
perform hotplug tasks.
And the last patch [30/30] allows to unplug of BUS-less
devices using hotplug-handler API.

Converted devices are covered with new hotplug
unit-tests, except of:
 s390x-*: I have no idea how or means to test it, but code
          is close to virtio, so it's converted using
          virtio template
 pvscsi: is broken, so no means to test it

Git tree for testing:
  https://github.com/imammedo/qemu/commits/hp_ctrl_conversion_v1


Igor Mammedov (30):
  test: virtio-scsi: check if hot-plug/unplug works
  test: virtio-serial: check if hot-plug/unplug works
  test: virtio-rng: check if hot-plug/unplug works
  test: virtio-net: check if hot-plug/unplug works
  test: virtio-blk: check if hot-plug/unplug works
  test: usb: add port test to uhci unit test
  test: usb: generic usb device hotplug
  test: usb: usb-storage hotplug test
  access BusState.allow_hotplug using wraper qbus_is_hotpluggable()
  qdev: HotplugHandler: rename unplug callback to unplug_request
  qdev: HotplugHandler: provide unplug callback
  qdev: add simple/generic unplug callback for HotplugHandler
  qdev: hotplug: set handler only if HOTPLUG_HANDLER interface is
    supported
  target-i386: ICC bus: replace BusState.allow_hotplug with
    hotplug_handler
  virtio-pci: replace BusState.allow_hotplug with hotplug_handler
  virtio-serial: convert to hotplug-handler API
  virtio-mmio: drop useless bus->allow_hotplug = 0
  s390x: drop not used allow_hotplug in event-facility
  s390x: convert s390-virtio to hotplug handler API
  s390x: convert virtio-ccw to hotplug handler API
  scsi: make scsi_bus_new() assign hotplug controller
  scsi: convert pvscsi HBA to hotplug hander API
  scsi: convert virtio-scsi HBA to hotplug handler API
  scsi: cleanup not used anymore SCSIBusInfo{hotplug,hot_unplug} fields
  usb-bot: drop not needed "allow_hotplug = 0"
  usb-storage: make its storage SCSI bus hotpluggable explicitly
  usb-storage: drop not needed "allow_hotplug = 0"
  usb: convert to hotplug handler API
  qdev: drop legacy hotplug fields/methods
  qdev: HotplugHandler: add support for unplugging BUS-less devices

 hw/acpi/piix4.c                |  6 ++--
 hw/char/virtio-serial-bus.c    | 20 ++++++++---
 hw/core/hotplug.c              | 11 +++++++
 hw/core/qdev.c                 | 66 ++++++++++++++++++++++---------------
 hw/cpu/icc_bus.c               | 20 ++++++-----
 hw/i386/acpi-build.c           |  2 +-
 hw/isa/lpc_ich9.c              |  6 ++--
 hw/pci-bridge/pci_bridge_dev.c |  2 +-
 hw/pci/pci-hotplug-old.c       |  4 +--
 hw/pci/pcie.c                  |  4 +--
 hw/pci/pcie_port.c             |  2 +-
 hw/pci/shpc.c                  |  4 +--
 hw/s390x/event-facility.c      |  2 --
 hw/s390x/s390-virtio-bus.c     | 19 ++++++++---
 hw/s390x/virtio-ccw.c          | 24 +++++++++-----
 hw/scsi/scsi-bus.c             | 18 +---------
 hw/scsi/virtio-scsi.c          | 28 ++++++++++------
 hw/scsi/vmw_pvscsi.c           | 24 +++++++++-----
 hw/usb/bus.c                   |  3 +-
 hw/usb/dev-smartcard-reader.c  |  8 ++++-
 hw/usb/dev-storage.c           |  6 ++--
 hw/usb/hcd-ehci-pci.c          |  6 ++++
 hw/usb/hcd-ehci-sysbus.c       |  6 ++++
 hw/usb/hcd-ohci.c              | 12 +++++++
 hw/usb/hcd-uhci.c              |  6 ++++
 hw/usb/hcd-xhci.c              |  6 ++++
 hw/virtio/virtio-mmio.c        | 17 ++--------
 hw/virtio/virtio-pci.c         |  8 +++--
 include/hw/hotplug.h           | 18 ++++++++--
 include/hw/pci/pcie.h          |  4 +--
 include/hw/pci/shpc.h          |  4 +--
 include/hw/qdev-core.h         | 15 ++++++---
 include/hw/scsi/scsi.h         |  2 --
 qdev-monitor.c                 |  2 +-
 tests/Makefile                 |  9 ++---
 tests/test-usb-hcd-common.c    | 71 +++++++++++++++++++++++++++++++++++++++
 tests/test-usb-hcd-common.h    | 16 +++++++++
 tests/usb-hcd-ehci-test.c      | 50 +++++++++++-----------------
 tests/usb-hcd-ohci-test.c      | 10 ++++--
 tests/usb-hcd-uhci-test.c      | 64 +++++++++++++++++++++++++++++++++--
 tests/usb-hcd-xhci-test.c      | 11 +++++--
 tests/virtio-blk-test.c        | 75 +++++++++++++++++++++++++++++++++++-------
 tests/virtio-net-test.c        | 39 ++++++++++++++++++++++
 tests/virtio-rng-test.c        | 38 +++++++++++++++++++++
 tests/virtio-scsi-test.c       | 29 ++++++++++++++++
 tests/virtio-serial-test.c     | 27 +++++++++++++++
 46 files changed, 631 insertions(+), 193 deletions(-)
 create mode 100644 tests/test-usb-hcd-common.c
 create mode 100644 tests/test-usb-hcd-common.h

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/30] test: virtio-scsi: check if hot-plug/unplug works

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/virtio-scsi-test.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index 3230908..41f9602 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -17,14 +17,43 @@ static void pci_nop(void)
 {
 }
 
+static void hotplug(void)
+{
+    QDict *response;
+
+    response = qmp("{\"execute\": \"device_add\","
+                   " \"arguments\": {"
+                   "   \"driver\": \"scsi-hd\","
+                   "   \"id\": \"scsi-hd\","
+                   "   \"drive\": \"drv1\""
+                   "}}");
+
+    g_assert(response);
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+
+    response = qmp("{\"execute\": \"device_del\","
+                   " \"arguments\": {"
+                   "   \"id\": \"scsi-hd\""
+                   "}}");
+
+    g_assert(response);
+    g_assert(!qdict_haskey(response, "error"));
+    g_assert(qdict_haskey(response, "event"));
+    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
+    QDECREF(response);
+}
+
 int main(int argc, char **argv)
 {
     int ret;
 
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/virtio/scsi/pci/nop", pci_nop);
+    qtest_add_func("/virtio/scsi/pci/hotplug", hotplug);
 
     qtest_start("-drive id=drv0,if=none,file=/dev/null "
+                "-drive id=drv1,if=none,file=/dev/null "
                 "-device virtio-scsi-pci,id=vscsi0 "
                 "-device scsi-hd,bus=vscsi0.0,drive=drv0");
     ret = g_test_run();
-- 
1.8.3.1

[Qemu-devel] [PATCH 02/30] test: virtio-serial: check if hot-plug/unplug works

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/virtio-serial-test.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/tests/virtio-serial-test.c b/tests/virtio-serial-test.c
index e743875..bf030a6 100644
--- a/tests/virtio-serial-test.c
+++ b/tests/virtio-serial-test.c
@@ -17,12 +17,39 @@ static void pci_nop(void)
 {
 }
 
+static void hotplug(void)
+{
+    QDict *response;
+
+    response = qmp("{\"execute\": \"device_add\","
+                   " \"arguments\": {"
+                   "   \"driver\": \"virtserialport\","
+                   "   \"id\": \"hp-port\""
+                   "}}");
+
+    g_assert(response);
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+
+    response = qmp("{\"execute\": \"device_del\","
+                   " \"arguments\": {"
+                   "   \"id\": \"hp-port\""
+                   "}}");
+
+    g_assert(response);
+    g_assert(!qdict_haskey(response, "error"));
+    g_assert(qdict_haskey(response, "event"));
+    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
+    QDECREF(response);
+}
+
 int main(int argc, char **argv)
 {
     int ret;
 
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/virtio/serial/pci/nop", pci_nop);
+    qtest_add_func("/virtio/serial/pci/hotplug", hotplug);
 
     qtest_start("-device virtio-serial-pci");
     ret = g_test_run();
-- 
1.8.3.1

[Qemu-devel] [PATCH 03/30] test: virtio-rng: check if hot-plug/unplug works

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/virtio-rng-test.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tests/virtio-rng-test.c b/tests/virtio-rng-test.c
index 402c206..7f62f84 100644
--- a/tests/virtio-rng-test.c
+++ b/tests/virtio-rng-test.c
@@ -12,17 +12,55 @@
 #include "libqtest.h"
 #include "qemu/osdep.h"
 
+#define PCI_SLOT_HP             0x06
+#define ACPI_PCIHP_ADDR         0xae00
+#define PCI_EJ_BASE             0x0008
+
 /* Tests only initialization so far. TODO: Replace with functional tests */
 static void pci_nop(void)
 {
 }
 
+static void hotplug(void)
+{
+    QDict *response;
+
+    response = qmp("{\"execute\": \"device_add\","
+                   " \"arguments\": {"
+                   "   \"driver\": \"virtio-rng-pci\","
+                   "   \"addr\": \"" stringify(PCI_SLOT_HP) "\","
+                   "   \"id\": \"rng1\""
+                   "}}");
+
+    g_assert(response);
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+
+    response = qmp("{\"execute\": \"device_del\","
+                   " \"arguments\": {"
+                   "   \"id\": \"rng1\""
+                   "}}");
+
+    g_assert(response);
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+
+    outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << PCI_SLOT_HP);
+
+    response = qmp("");
+    g_assert(response);
+    g_assert(qdict_haskey(response, "event"));
+    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
+    QDECREF(response);
+}
+
 int main(int argc, char **argv)
 {
     int ret;
 
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/virtio/rng/pci/nop", pci_nop);
+    qtest_add_func("/virtio/rng/pci/hotplug", hotplug);
 
     qtest_start("-device virtio-rng-pci");
     ret = g_test_run();
-- 
1.8.3.1

[Qemu-devel] [PATCH 04/30] test: virtio-net: check if hot-plug/unplug works

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/virtio-net-test.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index df99343..b80a72f 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -12,17 +12,56 @@
 #include "libqtest.h"
 #include "qemu/osdep.h"
 
+#define PCI_SLOT_HP             0x06
+#define ACPI_PCIHP_ADDR         0xae00
+#define PCI_EJ_BASE             0x0008
+
 /* Tests only initialization so far. TODO: Replace with functional tests */
 static void pci_nop(void)
 {
 }
 
+static void hotplug(void)
+{
+    QDict *response;
+
+    response = qmp("{\"execute\": \"device_add\","
+                   " \"arguments\": {"
+                   "   \"driver\": \"virtio-net-pci\","
+                   "   \"addr\": \"" stringify(PCI_SLOT_HP) "\","
+                   "   \"id\": \"net1\""
+                   "}}");
+
+    g_assert(response);
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+
+    response = qmp("{\"execute\": \"device_del\","
+                   " \"arguments\": {"
+                   "   \"id\": \"net1\""
+                   "}}");
+
+    g_assert(response);
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+
+    outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << PCI_SLOT_HP);
+
+    response = qmp("");
+    g_assert(response);
+    g_assert(qdict_haskey(response, "event"));
+    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
+    QDECREF(response);
+}
+
+
 int main(int argc, char **argv)
 {
     int ret;
 
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/virtio/net/pci/nop", pci_nop);
+    qtest_add_func("/virtio/net/pci/hotplug", hotplug);
 
     qtest_start("-device virtio-net-pci");
     ret = g_test_run();
-- 
1.8.3.1

[Qemu-devel] [PATCH 05/30] test: virtio-blk: check if hot-plug/unplug works

[Igor Mammedov]

since virtio-blk-pci is a PCI device, its unplug is async
and handled by ACPI. So simultate device's ACPI _EJ0
method to trigger actual device removal.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/virtio-blk-test.c | 75 +++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 64 insertions(+), 11 deletions(-)

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 588666c..b42c10b 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -45,6 +45,10 @@
 #define PCI_SLOT                0x04
 #define PCI_FN                  0x00
 
+#define PCI_SLOT_HP             0x06
+#define ACPI_PCIHP_ADDR         0xae00
+#define PCI_EJ_BASE             0x0008
+
 typedef struct QVirtioBlkReq {
     uint32_t type;
     uint32_t ioprio;
@@ -55,7 +59,7 @@ typedef struct QVirtioBlkReq {
 
 static QPCIBus *test_start(void)
 {
-    char cmdline[100];
+    char *cmdline;
     char tmp_path[] = "/tmp/qtest.XXXXXX";
     int fd, ret;
 
@@ -66,11 +70,14 @@ static QPCIBus *test_start(void)
     g_assert_cmpint(ret, ==, 0);
     close(fd);
 
-    snprintf(cmdline, 100, "-drive if=none,id=drive0,file=%s "
-                            "-device virtio-blk-pci,drive=drive0,addr=%x.%x",
-                            tmp_path, PCI_SLOT, PCI_FN);
+    cmdline = g_strdup_printf("-drive if=none,id=drive0,file=%s "
+                              "-drive if=none,id=drive1,file=/dev/null "
+                              "-device virtio-blk-pci,id=drv0,drive=drive0,"
+                              "addr=%x.%x",
+                              tmp_path, PCI_SLOT, PCI_FN);
     qtest_start(cmdline);
     unlink(tmp_path);
+    g_free(cmdline);
 
     return qpci_init_pc();
 }
@@ -80,14 +87,14 @@ static void test_end(void)
     qtest_end();
 }
 
-static QVirtioPCIDevice *virtio_blk_init(QPCIBus *bus)
+static QVirtioPCIDevice *virtio_blk_init(QPCIBus *bus, int slot)
 {
     QVirtioPCIDevice *dev;
 
     dev = qvirtio_pci_device_find(bus, QVIRTIO_BLK_DEVICE_ID);
     g_assert(dev != NULL);
     g_assert_cmphex(dev->vdev.device_type, ==, QVIRTIO_BLK_DEVICE_ID);
-    g_assert_cmphex(dev->pdev->devfn, ==, ((PCI_SLOT << 3) | PCI_FN));
+    g_assert_cmphex(dev->pdev->devfn, ==, ((slot << 3) | PCI_FN));
 
     qvirtio_pci_device_enable(dev);
     qvirtio_reset(&qvirtio_pci, &dev->vdev);
@@ -147,7 +154,7 @@ static void pci_basic(void)
 
     bus = test_start();
 
-    dev = virtio_blk_init(bus);
+    dev = virtio_blk_init(bus, PCI_SLOT);
 
     /* MSI-X is not enabled */
     addr = dev->addr + QVIRTIO_DEVICE_SPECIFIC_NO_MSIX;
@@ -293,7 +300,7 @@ static void pci_indirect(void)
 
     bus = test_start();
 
-    dev = virtio_blk_init(bus);
+    dev = virtio_blk_init(bus, PCI_SLOT);
 
     /* MSI-X is not enabled */
     addr = dev->addr + QVIRTIO_DEVICE_SPECIFIC_NO_MSIX;
@@ -384,7 +391,7 @@ static void pci_config(void)
 
     bus = test_start();
 
-    dev = virtio_blk_init(bus);
+    dev = virtio_blk_init(bus, PCI_SLOT);
 
     /* MSI-X is not enabled */
     addr = dev->addr + QVIRTIO_DEVICE_SPECIFIC_NO_MSIX;
@@ -426,7 +433,7 @@ static void pci_msix(void)
     bus = test_start();
     alloc = pc_alloc_init();
 
-    dev = virtio_blk_init(bus);
+    dev = virtio_blk_init(bus, PCI_SLOT);
     qpci_msix_enable(dev->pdev);
 
     qvirtio_pci_set_msix_configuration_vector(dev, alloc, 0);
@@ -536,7 +543,7 @@ static void pci_idx(void)
     bus = test_start();
     alloc = pc_alloc_init();
 
-    dev = virtio_blk_init(bus);
+    dev = virtio_blk_init(bus, PCI_SLOT);
     qpci_msix_enable(dev->pdev);
 
     qvirtio_pci_set_msix_configuration_vector(dev, alloc, 0);
@@ -640,6 +647,51 @@ static void pci_idx(void)
     test_end();
 }
 
+static void hotplug(void)
+{
+    QDict *response;
+    QPCIBus *bus;
+    QVirtioPCIDevice *dev;
+    bus = test_start();
+
+    /* plug secondary disk */
+    response = qmp("{\"execute\": \"device_add\","
+                   " \"arguments\": {"
+                   "   \"driver\": \"virtio-blk-pci\","
+                   "   \"drive\": \"drive1\","
+                   "   \"addr\": \"" stringify(PCI_SLOT_HP) "\","
+                   "   \"id\": \"drv1\""
+                   "}}");
+
+    g_assert(response);
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+
+    dev = virtio_blk_init(bus, PCI_SLOT_HP);
+    g_assert(dev);
+    qvirtio_pci_device_disable(dev);
+    g_free(dev);
+
+    /* unplug secondary disk */
+    response = qmp("{\"execute\": \"device_del\","
+                   " \"arguments\": {"
+                   "   \"id\": \"drv1\""
+                   "}}");
+
+    g_assert(response);
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+
+    outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << PCI_SLOT_HP);
+
+    /* check DEVICE_DELETED event */
+    response = qmp("");
+    g_assert(qdict_haskey(response, "event"));
+    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
+
+    test_end();
+}
+
 int main(int argc, char **argv)
 {
     int ret;
@@ -651,6 +703,7 @@ int main(int argc, char **argv)
     g_test_add_func("/virtio/blk/pci/config", pci_config);
     g_test_add_func("/virtio/blk/pci/msix", pci_msix);
     g_test_add_func("/virtio/blk/pci/idx", pci_idx);
+    g_test_add_func("/virtio/blk/pci/hotplug", hotplug);
 
     ret = g_test_run();
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 06/30] test: usb: add port test to uhci unit test

[Igor Mammedov]

along with it move common code fro ehci test to
a separate file to avoid duplication.
Later this code will be used for hotplug test.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/Makefile              |  5 +++--
 tests/test-usb-hcd-common.c | 37 +++++++++++++++++++++++++++++++++++++
 tests/test-usb-hcd-common.h | 13 +++++++++++++
 tests/usb-hcd-ehci-test.c   | 36 +++++-------------------------------
 tests/usb-hcd-uhci-test.c   | 18 ++++++++++++++++--
 5 files changed, 74 insertions(+), 35 deletions(-)
 create mode 100644 tests/test-usb-hcd-common.c
 create mode 100644 tests/test-usb-hcd-common.h

diff --git a/tests/Makefile b/tests/Makefile
index f5de29c..06d7644 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -301,6 +301,7 @@ libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o
 libqos-pc-obj-y += tests/libqos/malloc-pc.o
 libqos-omap-obj-y = $(libqos-obj-y) tests/libqos/i2c-omap.o
 libqos-virtio-obj-y = $(libqos-obj-y) $(libqos-pc-obj-y) tests/libqos/virtio.o tests/libqos/virtio-pci.o
+libqos-usb-obj-y = $(libqos-pc-obj-y) tests/test-usb-hcd-common.o
 
 tests/rtc-test$(EXESUF): tests/rtc-test.o
 tests/m48t59-test$(EXESUF): tests/m48t59-test.o
@@ -344,8 +345,8 @@ tests/es1370-test$(EXESUF): tests/es1370-test.o
 tests/intel-hda-test$(EXESUF): tests/intel-hda-test.o
 tests/ioh3420-test$(EXESUF): tests/ioh3420-test.o
 tests/usb-hcd-ohci-test$(EXESUF): tests/usb-hcd-ohci-test.o
-tests/usb-hcd-uhci-test$(EXESUF): tests/usb-hcd-uhci-test.o
-tests/usb-hcd-ehci-test$(EXESUF): tests/usb-hcd-ehci-test.o $(libqos-pc-obj-y)
+tests/usb-hcd-uhci-test$(EXESUF): tests/usb-hcd-uhci-test.o $(libqos-usb-obj-y)
+tests/usb-hcd-ehci-test$(EXESUF): tests/usb-hcd-ehci-test.o $(libqos-usb-obj-y)
 tests/usb-hcd-xhci-test$(EXESUF): tests/usb-hcd-xhci-test.o
 tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o qemu-char.o qemu-timer.o $(qtest-obj-y)
 tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
diff --git a/tests/test-usb-hcd-common.c b/tests/test-usb-hcd-common.c
new file mode 100644
index 0000000..547c3ac
--- /dev/null
+++ b/tests/test-usb-hcd-common.c
@@ -0,0 +1,37 @@
+/*
+ * common code shared by usb tests
+ *
+ * Copyright (c) 2014 Red Hat, Inc
+ *
+ * Authors:
+ *     Gerd Hoffmann <kraxel@redhat.com>
+ *     John Snow <jsnow@redhat.com>
+ *     Igor Mammedov <imammedo@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include <glib.h>
+#include <string.h>
+#include "libqtest.h"
+#include "qemu/osdep.h"
+#include "hw/usb/uhci-regs.h"
+#include "test-usb-hcd-common.h"
+
+void usb_pci_init_one(QPCIBus *pcibus, struct qhc *hc, uint32_t devfn, int bar)
+{
+    hc->dev = qpci_device_find(pcibus, devfn);
+    g_assert(hc->dev != NULL);
+    qpci_device_enable(hc->dev);
+    hc->base = qpci_iomap(hc->dev, bar, NULL);
+    g_assert(hc->base != NULL);
+}
+
+void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
+{
+    void *addr = hc->base + 0x10 + 2 * port;
+    uint16_t value = qpci_io_readw(hc->dev, addr);
+    uint16_t mask = ~(UHCI_PORT_WRITE_CLEAR | UHCI_PORT_RSVD1);
+
+    g_assert((value & mask) == (expect & mask));
+}
diff --git a/tests/test-usb-hcd-common.h b/tests/test-usb-hcd-common.h
new file mode 100644
index 0000000..2133702
--- /dev/null
+++ b/tests/test-usb-hcd-common.h
@@ -0,0 +1,13 @@
+#ifndef TEST_USB_HCD_COMMON_H
+#define TEST_USB_HCD_COMMON_H
+
+#include "libqos/pci-pc.h"
+
+struct qhc {
+    QPCIDevice *dev;
+    void *base;
+};
+
+void usb_pci_init_one(QPCIBus *pcibus, struct qhc *hc, uint32_t devfn, int bar);
+void uhci_port_test(struct qhc *hc, int port, uint16_t expect);
+#endif
diff --git a/tests/usb-hcd-ehci-test.c b/tests/usb-hcd-ehci-test.c
index c990492..b69ced5 100644
--- a/tests/usb-hcd-ehci-test.c
+++ b/tests/usb-hcd-ehci-test.c
@@ -15,11 +15,7 @@
 #include "qemu/osdep.h"
 #include "hw/usb/uhci-regs.h"
 #include "hw/usb/ehci-regs.h"
-
-struct qhc {
-    QPCIDevice *dev;
-    void *base;
-};
+#include "test-usb-hcd-common.h"
 
 static QPCIBus *pcibus;
 static struct qhc uhci1;
@@ -29,15 +25,6 @@ static struct qhc ehci1;
 
 /* helpers */
 
-static void pci_init_one(struct qhc *hc, uint32_t devfn, int bar)
-{
-    hc->dev = qpci_device_find(pcibus, devfn);
-    g_assert(hc->dev != NULL);
-    qpci_device_enable(hc->dev);
-    hc->base = qpci_iomap(hc->dev, bar, NULL);
-    g_assert(hc->base != NULL);
-}
-
 #if 0
 static void uhci_port_update(struct qhc *hc, int port,
                              uint16_t set, uint16_t clear)
@@ -52,19 +39,6 @@ static void uhci_port_update(struct qhc *hc, int port,
 }
 #endif
 
-static void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
-{
-    void *addr = hc->base + 0x10 + 2 * port;
-    uint16_t value = qpci_io_readw(hc->dev, addr);
-    uint16_t mask = ~(UHCI_PORT_WRITE_CLEAR | UHCI_PORT_RSVD1);
-
-#if 0
-    fprintf(stderr, "%s: %d, have 0x%04x, want 0x%04x\n",
-            __func__, port, value & mask, expect & mask);
-#endif
-    g_assert((value & mask) == (expect & mask));
-}
-
 static void ehci_port_test(struct qhc *hc, int port, uint32_t expect)
 {
     void *addr = hc->base + 0x64 + 4 * port;
@@ -88,10 +62,10 @@ static void pci_init(void)
     pcibus = qpci_init_pc();
     g_assert(pcibus != NULL);
 
-    pci_init_one(&uhci1, QPCI_DEVFN(0x1d, 0), 4);
-    pci_init_one(&uhci2, QPCI_DEVFN(0x1d, 1), 4);
-    pci_init_one(&uhci3, QPCI_DEVFN(0x1d, 2), 4);
-    pci_init_one(&ehci1, QPCI_DEVFN(0x1d, 7), 0);
+    usb_pci_init_one(pcibus, &uhci1, QPCI_DEVFN(0x1d, 0), 4);
+    usb_pci_init_one(pcibus, &uhci2, QPCI_DEVFN(0x1d, 1), 4);
+    usb_pci_init_one(pcibus, &uhci3, QPCI_DEVFN(0x1d, 2), 4);
+    usb_pci_init_one(pcibus, &ehci1, QPCI_DEVFN(0x1d, 7), 0);
 }
 
 static void pci_uhci_port_1(void)
diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
index 94e858f..4a67c9f 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -11,13 +11,23 @@
 #include <string.h>
 #include "libqtest.h"
 #include "qemu/osdep.h"
+#include "test-usb-hcd-common.h"
+#include "hw/usb/uhci-regs.h"
 
 
 static void test_uhci_init(void)
 {
-    qtest_start("-device piix3-usb-uhci,id=uhci");
+}
 
-    qtest_end();
+static void test_port_1(void)
+{
+    QPCIBus *pcibus;
+    struct qhc uhci;
+
+    pcibus = qpci_init_pc();
+    g_assert(pcibus != NULL);
+    usb_pci_init_one(pcibus, &uhci, QPCI_DEVFN(0x1d, 0), 4);
+    uhci_port_test(&uhci, 0, UHCI_PORT_CCS);
 }
 
 
@@ -28,8 +38,12 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_func("/uhci/pci/init", test_uhci_init);
+    qtest_add_func("/uhci/pci/port1", test_port_1);
 
+    qtest_start("-device piix3-usb-uhci,id=uhci,addr=1d.0"
+                " -device usb-tablet,bus=uhci.0,port=1");
     ret = g_test_run();
+    qtest_end();
 
     return ret;
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH 07/30] test: usb: generic usb device hotplug

[Igor Mammedov]

use usb-tablet as a hotplugged usb device.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
Since I'm not familiar with internals of USB,
for OHCI/XHCI test doesn't verify that usb device
has been hotplugged except of checking for error
after hotplug. So port checks similar to that of
ehci/uhci, left as excercise for future.
---
 tests/Makefile              |  4 ++--
 tests/test-usb-hcd-common.c | 34 ++++++++++++++++++++++++++++++++++
 tests/test-usb-hcd-common.h |  3 +++
 tests/usb-hcd-ehci-test.c   | 14 ++++++++++++++
 tests/usb-hcd-ohci-test.c   | 10 ++++++++--
 tests/usb-hcd-uhci-test.c   | 20 ++++++++++++++++++--
 tests/usb-hcd-xhci-test.c   | 11 ++++++++---
 7 files changed, 87 insertions(+), 9 deletions(-)

diff --git a/tests/Makefile b/tests/Makefile
index 06d7644..bcc77e5 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -344,10 +344,10 @@ tests/ac97-test$(EXESUF): tests/ac97-test.o
 tests/es1370-test$(EXESUF): tests/es1370-test.o
 tests/intel-hda-test$(EXESUF): tests/intel-hda-test.o
 tests/ioh3420-test$(EXESUF): tests/ioh3420-test.o
-tests/usb-hcd-ohci-test$(EXESUF): tests/usb-hcd-ohci-test.o
+tests/usb-hcd-ohci-test$(EXESUF): tests/usb-hcd-ohci-test.o $(libqos-usb-obj-y)
 tests/usb-hcd-uhci-test$(EXESUF): tests/usb-hcd-uhci-test.o $(libqos-usb-obj-y)
 tests/usb-hcd-ehci-test$(EXESUF): tests/usb-hcd-ehci-test.o $(libqos-usb-obj-y)
-tests/usb-hcd-xhci-test$(EXESUF): tests/usb-hcd-xhci-test.o
+tests/usb-hcd-xhci-test$(EXESUF): tests/usb-hcd-xhci-test.o $(libqos-usb-obj-y)
 tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o qemu-char.o qemu-timer.o $(qtest-obj-y)
 tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
 tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o libqemuutil.a libqemustub.a
diff --git a/tests/test-usb-hcd-common.c b/tests/test-usb-hcd-common.c
index 547c3ac..74b289b 100644
--- a/tests/test-usb-hcd-common.c
+++ b/tests/test-usb-hcd-common.c
@@ -35,3 +35,37 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t expect)
 
     g_assert((value & mask) == (expect & mask));
 }
+
+void usb_test_hotplug(const char *hcd_id, const int port,
+                      void (*port_check)(void))
+{
+    QDict *response;
+    char  *cmd;
+
+    cmd = g_strdup_printf("{'execute': 'device_add',"
+                          " 'arguments': {"
+                          "   'driver': 'usb-tablet',"
+                          "   'port': '%d',"
+                          "   'bus': '%s.0',"
+                          "   'id': 'usbdev%d'"
+                          "}}", port, hcd_id, port);
+    response = qmp(cmd);
+    g_free(cmd);
+    g_assert(response);
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+
+    if (port_check) {
+        port_check();
+    }
+
+    cmd = g_strdup_printf("{'execute': 'device_del',"
+                           " 'arguments': {"
+                           "   'id': 'usbdev%d'"
+                           "}}", port);
+    response = qmp(cmd);
+    g_free(cmd);
+    g_assert(response);
+    g_assert(qdict_haskey(response, "event"));
+    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
+}
diff --git a/tests/test-usb-hcd-common.h b/tests/test-usb-hcd-common.h
index 2133702..72be110 100644
--- a/tests/test-usb-hcd-common.h
+++ b/tests/test-usb-hcd-common.h
@@ -10,4 +10,7 @@ struct qhc {
 
 void usb_pci_init_one(QPCIBus *pcibus, struct qhc *hc, uint32_t devfn, int bar);
 void uhci_port_test(struct qhc *hc, int port, uint16_t expect);
+
+void usb_test_hotplug(const char *bus_name, const int port,
+                      void (*port_check)(void));
 #endif
diff --git a/tests/usb-hcd-ehci-test.c b/tests/usb-hcd-ehci-test.c
index b69ced5..f65d1e9 100644
--- a/tests/usb-hcd-ehci-test.c
+++ b/tests/usb-hcd-ehci-test.c
@@ -128,6 +128,19 @@ static void pci_ehci_port_2(void)
     }
 }
 
+static void pci_ehci_port_3_hotplug(void)
+{
+    /* check for presence of hotplugged usb-tablet */
+    g_assert(pcibus != NULL);
+    ehci_port_test(&ehci1, 2, PORTSC_PPOWER | PORTSC_CONNECT);
+}
+
+static void pci_ehci_port_hotplug(void)
+{
+    usb_test_hotplug("ich9-ehci-1", 3, pci_ehci_port_3_hotplug);
+}
+
+
 int main(int argc, char **argv)
 {
     int ret;
@@ -139,6 +152,7 @@ int main(int argc, char **argv)
     qtest_add_func("/ehci/pci/ehci-config", pci_ehci_config);
     qtest_add_func("/ehci/pci/uhci-port-2", pci_uhci_port_2);
     qtest_add_func("/ehci/pci/ehci-port-2", pci_ehci_port_2);
+    qtest_add_func("/ehci/pci/ehci-port-3-hotplug", pci_ehci_port_hotplug);
 
     qtest_start("-machine q35 -device ich9-usb-ehci1,bus=pcie.0,addr=1d.7,"
                 "multifunction=on,id=ich9-ehci-1 "
diff --git a/tests/usb-hcd-ohci-test.c b/tests/usb-hcd-ohci-test.c
index fbc3ffe..0a13990 100644
--- a/tests/usb-hcd-ohci-test.c
+++ b/tests/usb-hcd-ohci-test.c
@@ -11,15 +11,18 @@
 #include <string.h>
 #include "libqtest.h"
 #include "qemu/osdep.h"
+#include "test-usb-hcd-common.h"
 
 
 static void test_ohci_init(void)
 {
-    qtest_start("-device pci-ohci,id=ohci");
 
-    qtest_end();
 }
 
+static void test_ohci_hotplug(void)
+{
+    usb_test_hotplug("ohci", 1, NULL);
+}
 
 int main(int argc, char **argv)
 {
@@ -28,8 +31,11 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_func("/ohci/pci/init", test_ohci_init);
+    qtest_add_func("/ohci/pci/hotplug", test_ohci_hotplug);
 
+    qtest_start("-device pci-ohci,id=ohci");
     ret = g_test_run();
+    qtest_end();
 
     return ret;
 }
diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
index 4a67c9f..ce27962 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -19,17 +19,32 @@ static void test_uhci_init(void)
 {
 }
 
-static void test_port_1(void)
+static void test_port(int port)
 {
     QPCIBus *pcibus;
     struct qhc uhci;
 
+    g_assert(port > 0);
     pcibus = qpci_init_pc();
     g_assert(pcibus != NULL);
     usb_pci_init_one(pcibus, &uhci, QPCI_DEVFN(0x1d, 0), 4);
-    uhci_port_test(&uhci, 0, UHCI_PORT_CCS);
+    uhci_port_test(&uhci, port - 1, UHCI_PORT_CCS);
+}
+
+static void test_port_1(void)
+{
+    test_port(1);
 }
 
+static void test_port_2(void)
+{
+    test_port(2);
+}
+
+static void test_uhci_hotplug(void)
+{
+    usb_test_hotplug("uhci", 2, test_port_2);
+}
 
 int main(int argc, char **argv)
 {
@@ -39,6 +54,7 @@ int main(int argc, char **argv)
 
     qtest_add_func("/uhci/pci/init", test_uhci_init);
     qtest_add_func("/uhci/pci/port1", test_port_1);
+    qtest_add_func("/uhci/pci/hotplug", test_uhci_hotplug);
 
     qtest_start("-device piix3-usb-uhci,id=uhci,addr=1d.0"
                 " -device usb-tablet,bus=uhci.0,port=1");
diff --git a/tests/usb-hcd-xhci-test.c b/tests/usb-hcd-xhci-test.c
index 743e979..fecfa63 100644
--- a/tests/usb-hcd-xhci-test.c
+++ b/tests/usb-hcd-xhci-test.c
@@ -11,15 +11,17 @@
 #include <string.h>
 #include "libqtest.h"
 #include "qemu/osdep.h"
+#include "test-usb-hcd-common.h"
 
 
 static void test_xhci_init(void)
 {
-    qtest_start("-device nec-usb-xhci,id=xhci");
-
-    qtest_end();
 }
 
+static void test_xhci_hotplug(void)
+{
+    usb_test_hotplug("xhci", 1, NULL);
+}
 
 int main(int argc, char **argv)
 {
@@ -28,8 +30,11 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_func("/xhci/pci/init", test_xhci_init);
+    qtest_add_func("/xhci/pci/hotplug", test_xhci_hotplug);
 
+    qtest_start("-device nec-usb-xhci,id=xhci");
     ret = g_test_run();
+    qtest_end();
 
     return ret;
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH 08/30] test: usb: usb-storage hotplug test

[Igor Mammedov]

usb-storage is different from usual usb devices
in that it uses child SCSI bus for underlying storage.
That patch checks that SCSI bus is hotpluggable for
hotplug operation wouldn't succeed without it.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/usb-hcd-uhci-test.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
index ce27962..bf1d52e 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -46,6 +46,34 @@ static void test_uhci_hotplug(void)
     usb_test_hotplug("uhci", 2, test_port_2);
 }
 
+static void test_usb_storage_hotplug(void)
+{
+    QDict *response;
+
+    response = qmp("{'execute': 'device_add',"
+                   " 'arguments': {"
+                   "   'driver': 'usb-storage',"
+                   "   'drive': 'drive0',"
+                   "   'id': 'usbdev0'"
+                   "}}");
+    g_assert(response);
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+
+    response = qmp("{'execute': 'device_del',"
+                           " 'arguments': {"
+                           "   'id': 'usbdev0'"
+                           "}}");
+    g_assert(response);
+    g_assert(!qdict_haskey(response, "error"));
+    QDECREF(response);
+
+    response = qmp("");
+    g_assert(response);
+    g_assert(qdict_haskey(response, "event"));
+    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
+}
+
 int main(int argc, char **argv)
 {
     int ret;
@@ -55,8 +83,10 @@ int main(int argc, char **argv)
     qtest_add_func("/uhci/pci/init", test_uhci_init);
     qtest_add_func("/uhci/pci/port1", test_port_1);
     qtest_add_func("/uhci/pci/hotplug", test_uhci_hotplug);
+    qtest_add_func("/uhci/pci/hotplug/usb-storage", test_usb_storage_hotplug);
 
     qtest_start("-device piix3-usb-uhci,id=uhci,addr=1d.0"
+                " -drive id=drive0,if=none,file=/dev/null"
                 " -device usb-tablet,bus=uhci.0,port=1");
     ret = g_test_run();
     qtest_end();
-- 
1.8.3.1

[Qemu-devel] [PATCH 09/30] access BusState.allow_hotplug using wraper qbus_is_hotpluggable()

[Igor Mammedov]

it would allow transparently switch detection if Bus
is hotpluggable from allow_hotplug field to hotplug_handler
link and drop allow_hotplug field once all users are
converted to hotplug handler API.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/core/qdev.c           | 6 +++---
 hw/i386/acpi-build.c     | 2 +-
 hw/pci/pci-hotplug-old.c | 4 ++--
 include/hw/qdev-core.h   | 5 +++++
 qdev-monitor.c           | 2 +-
 5 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index fcb1638..5e5b963 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -86,7 +86,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
     BusChild *kid = g_malloc0(sizeof(*kid));
 
     if (qdev_hotplug) {
-        assert(bus->allow_hotplug);
+        assert(qbus_is_hotpluggable(bus));
     }
 
     kid->index = bus->max_index++;
@@ -213,7 +213,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
 {
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
 
-    if (dev->parent_bus && !dev->parent_bus->allow_hotplug) {
+    if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
         error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
         return;
     }
@@ -925,7 +925,7 @@ static bool device_get_hotpluggable(Object *obj, Error **errp)
     DeviceState *dev = DEVICE(obj);
 
     return dc->hotpluggable && (dev->parent_bus == NULL ||
-                                dev->parent_bus->allow_hotplug);
+                                qbus_is_hotpluggable(dev->parent_bus));
 }
 
 static bool device_get_hotplugged(Object *obj, Error **err)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a313321..00be4bb 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -774,7 +774,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
     unsigned *bsel_alloc = opaque;
     unsigned *bus_bsel;
 
-    if (bus->qbus.allow_hotplug) {
+    if (qbus_is_hotpluggable(BUS(bus))) {
         bus_bsel = g_malloc(sizeof *bus_bsel);
 
         *bus_bsel = (*bsel_alloc)++;
diff --git a/hw/pci/pci-hotplug-old.c b/hw/pci/pci-hotplug-old.c
index cf2caeb..f9b7398 100644
--- a/hw/pci/pci-hotplug-old.c
+++ b/hw/pci/pci-hotplug-old.c
@@ -77,7 +77,7 @@ static PCIDevice *qemu_pci_hot_add_nic(Monitor *mon,
         monitor_printf(mon, "Invalid PCI device address %s\n", devaddr);
         return NULL;
     }
-    if (!((BusState*)bus)->allow_hotplug) {
+    if (!qbus_is_hotpluggable(BUS(bus))) {
         monitor_printf(mon, "PCI bus doesn't support hotplug\n");
         return NULL;
     }
@@ -224,7 +224,7 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
         monitor_printf(mon, "Invalid PCI device address %s\n", devaddr);
         return NULL;
     }
-    if (!((BusState*)bus)->allow_hotplug) {
+    if (!qbus_is_hotpluggable(BUS(bus))) {
         monitor_printf(mon, "PCI bus doesn't support hotplug\n");
         return NULL;
     }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 178fee2..48a96d2 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -368,4 +368,9 @@ static inline void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler,
                              QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
     bus->allow_hotplug = 1;
 }
+
+static inline bool qbus_is_hotpluggable(BusState *bus)
+{
+   return bus->allow_hotplug || bus->hotplug_handler;
+}
 #endif
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 5ec6606..f6db461 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -515,7 +515,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
             return NULL;
         }
     }
-    if (qdev_hotplug && bus && !bus->allow_hotplug) {
+    if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
         qerror_report(QERR_BUS_NO_HOTPLUG, bus->name);
         return NULL;
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH 10/30] qdev: HotplugHandler: rename unplug callback to unplug_request

[Igor Mammedov]

'HotplugHandler.unplug' callback is currently used as async
call to issue unplug request for device that implements it.
Renaming 'unplug' callback to 'unplug_request' should help to
avoid confusion about what callback does and would allow to
introduce 'unplug' callback that would perform actual device
removal when guest is ready for it.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
Patch is prompted by reviewing https://patchwork.ozlabs.org/patch/383372/

Dedicated 'unplug' callback could be used by bus-less pc-dimm
device. It would allow to call HotplugHandler.unplug callback
from ACPI code when guest calls _EJ0 method and execute board
specific code (PCMachine) to unmap pc-dimm from guest's address
space and perform necessary cleanup. The same applies to CPU
unplug.
---
 hw/acpi/piix4.c                |  6 +++---
 hw/core/hotplug.c              | 10 +++++-----
 hw/core/qdev.c                 |  3 ++-
 hw/isa/lpc_ich9.c              |  6 +++---
 hw/pci-bridge/pci_bridge_dev.c |  2 +-
 hw/pci/pcie.c                  |  4 ++--
 hw/pci/pcie_port.c             |  2 +-
 hw/pci/shpc.c                  |  4 ++--
 include/hw/hotplug.h           | 16 +++++++++-------
 include/hw/pci/pcie.h          |  4 ++--
 include/hw/pci/shpc.h          |  4 ++--
 11 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index b72b34e..0bfa814 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -354,8 +354,8 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
     }
 }
 
-static void piix4_device_unplug_cb(HotplugHandler *hotplug_dev,
-                                   DeviceState *dev, Error **errp)
+static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
+                                           DeviceState *dev, Error **errp)
 {
     PIIX4PMState *s = PIIX4_PM(hotplug_dev);
 
@@ -615,7 +615,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
     dc->cannot_instantiate_with_device_add_yet = true;
     dc->hotpluggable = false;
     hc->plug = piix4_device_plug_cb;
-    hc->unplug = piix4_device_unplug_cb;
+    hc->unplug_request = piix4_device_unplug_request_cb;
     adevc->ospm_status = piix4_ospm_status;
 }
 
diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
index 5573d9d..2ec4736 100644
--- a/hw/core/hotplug.c
+++ b/hw/core/hotplug.c
@@ -23,14 +23,14 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
     }
 }
 
-void hotplug_handler_unplug(HotplugHandler *plug_handler,
-                            DeviceState *plugged_dev,
-                            Error **errp)
+void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
+                                    DeviceState *plugged_dev,
+                                    Error **errp)
 {
     HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
 
-    if (hdc->unplug) {
-        hdc->unplug(plug_handler, plugged_dev, errp);
+    if (hdc->unplug_request) {
+        hdc->unplug_request(plug_handler, plugged_dev, errp);
     }
 }
 
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 5e5b963..c98e5db 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -227,7 +227,8 @@ void qdev_unplug(DeviceState *dev, Error **errp)
     qdev_hot_removed = true;
 
     if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
-        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
+        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler,
+                                       dev, errp);
     } else {
         assert(dc->unplug != NULL);
         if (dc->unplug(dev) < 0) { /* legacy handler */
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 177023b..530b074 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -607,8 +607,8 @@ static void ich9_device_plug_cb(HotplugHandler *hotplug_dev,
     ich9_pm_device_plug_cb(&lpc->pm, dev, errp);
 }
 
-static void ich9_device_unplug_cb(HotplugHandler *hotplug_dev,
-                                  DeviceState *dev, Error **errp)
+static void ich9_device_unplug_request_cb(HotplugHandler *hotplug_dev,
+                                          DeviceState *dev, Error **errp)
 {
     error_setg(errp, "acpi: device unplug request for not supported device"
                " type: %s", object_get_typename(OBJECT(dev)));
@@ -676,7 +676,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
      */
     dc->cannot_instantiate_with_device_add_yet = true;
     hc->plug = ich9_device_plug_cb;
-    hc->unplug = ich9_device_unplug_cb;
+    hc->unplug_request = ich9_device_unplug_request_cb;
     adevc->ospm_status = ich9_pm_ospm_status;
 }
 
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 92799d0..252ea5e 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -150,7 +150,7 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &pci_bridge_dev_vmstate;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     hc->plug = shpc_device_hotplug_cb;
-    hc->unplug = shpc_device_hot_unplug_cb;
+    hc->unplug_request = shpc_device_hot_unplug_request_cb;
 }
 
 static const TypeInfo pci_bridge_dev_info = {
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 1babddf..b64a004 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -262,8 +262,8 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                         PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
 }
 
-void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
-                                 Error **errp)
+void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
+                                         DeviceState *dev, Error **errp)
 {
     uint8_t *exp_cap;
 
diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c
index fa24877..40ca8d5 100644
--- a/hw/pci/pcie_port.c
+++ b/hw/pci/pcie_port.c
@@ -154,7 +154,7 @@ static void pcie_slot_class_init(ObjectClass *oc, void *data)
 
     dc->props = pcie_slot_props;
     hc->plug = pcie_cap_slot_hotplug_cb;
-    hc->unplug = pcie_cap_slot_hot_unplug_cb;
+    hc->unplug_request = pcie_cap_slot_hot_unplug_request_cb;
 }
 
 static const TypeInfo pcie_slot_type_info = {
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 1fcb8c4..65b2f51 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -549,8 +549,8 @@ void shpc_device_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
     shpc_interrupt_update(pci_hotplug_dev);
 }
 
-void shpc_device_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
-                               Error **errp)
+void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
+                                       DeviceState *dev, Error **errp)
 {
     Error *local_err = NULL;
     PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
index a6533cb..e397d08 100644
--- a/include/hw/hotplug.h
+++ b/include/hw/hotplug.h
@@ -47,7 +47,9 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
  *
  * @parent: Opaque parent interface.
  * @plug: plug callback.
- * @unplug: unplug callback.
+ * @unplug_request: unplug request callback.
+ *                  Used as a means to initiate device unplug for devices that
+ *                  require asynchronous unplug handling.
  */
 typedef struct HotplugHandlerClass {
     /* <private> */
@@ -55,7 +57,7 @@ typedef struct HotplugHandlerClass {
 
     /* <public> */
     hotplug_fn plug;
-    hotplug_fn unplug;
+    hotplug_fn unplug_request;
 } HotplugHandlerClass;
 
 /**
@@ -68,11 +70,11 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
                           Error **errp);
 
 /**
- * hotplug_handler_unplug:
+ * hotplug_handler_unplug_request:
  *
- * Call #HotplugHandlerClass.unplug callback of @plug_handler.
+ * Calls #HotplugHandlerClass.unplug_request callback of @plug_handler.
  */
-void hotplug_handler_unplug(HotplugHandler *plug_handler,
-                            DeviceState *plugged_dev,
-                            Error **errp);
+void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
+                                    DeviceState *plugged_dev,
+                                    Error **errp);
 #endif
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index d139d58..b48a7a2 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -128,6 +128,6 @@ extern const VMStateDescription vmstate_pcie_device;
 
 void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp);
-void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
-                                 Error **errp);
+void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
+                                         DeviceState *dev, Error **errp);
 #endif /* QEMU_PCIE_H */
diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index eef1a1a..025bc5b 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -46,8 +46,8 @@ void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
 
 void shpc_device_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                             Error **errp);
-void shpc_device_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
-                               Error **errp);
+void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
+                                       DeviceState *dev, Error **errp);
 
 extern VMStateInfo shpc_vmstate_info;
 #define SHPC_VMSTATE(_field, _type) \
-- 
1.8.3.1

[Qemu-devel] [PATCH 11/30] qdev: HotplugHandler: provide unplug callback

[Igor Mammedov]

it to be called for actual device removal and
will allow to separate request and removal handling
phases of x86-CPU devices and also it's a handler
to be called for synchronously removable devices.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
unplug handling for bus-less devices will be added
later in this series.
---
 hw/core/hotplug.c    | 11 +++++++++++
 hw/core/qdev.c       | 13 +++++++++++--
 include/hw/hotplug.h | 12 ++++++++++++
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
index 2ec4736..4e01074 100644
--- a/hw/core/hotplug.c
+++ b/hw/core/hotplug.c
@@ -34,6 +34,17 @@ void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
     }
 }
 
+void hotplug_handler_unplug(HotplugHandler *plug_handler,
+                            DeviceState *plugged_dev,
+                            Error **errp)
+{
+    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
+
+    if (hdc->unplug) {
+        hdc->unplug(plug_handler, plugged_dev, errp);
+    }
+}
+
 static const TypeInfo hotplug_handler_info = {
     .name          = TYPE_HOTPLUG_HANDLER,
     .parent        = TYPE_INTERFACE,
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index c98e5db..c89d781 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -227,8 +227,17 @@ void qdev_unplug(DeviceState *dev, Error **errp)
     qdev_hot_removed = true;
 
     if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
-        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler,
-                                       dev, errp);
+        HotplugHandlerClass *hdc;
+
+        /* If device supports async unplug just request it to be done,
+         * otherwise just remove it synchronously */
+        hdc = HOTPLUG_HANDLER_GET_CLASS(dev->parent_bus->hotplug_handler);
+        if (hdc->unplug_request) {
+            hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler,
+                                           dev, errp);
+        } else {
+            hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
+        }
     } else {
         assert(dc->unplug != NULL);
         if (dc->unplug(dev) < 0) { /* legacy handler */
diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
index e397d08..451d522 100644
--- a/include/hw/hotplug.h
+++ b/include/hw/hotplug.h
@@ -50,6 +50,9 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
  * @unplug_request: unplug request callback.
  *                  Used as a means to initiate device unplug for devices that
  *                  require asynchronous unplug handling.
+ * @unplug_request: unplug callback.
+ *                  Used for device removal with devices that implement
+ *                  asynchronous and synchronous (suprise) removal.
  */
 typedef struct HotplugHandlerClass {
     /* <private> */
@@ -58,6 +61,7 @@ typedef struct HotplugHandlerClass {
     /* <public> */
     hotplug_fn plug;
     hotplug_fn unplug_request;
+    hotplug_fn unplug;
 } HotplugHandlerClass;
 
 /**
@@ -77,4 +81,12 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
 void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
                                     DeviceState *plugged_dev,
                                     Error **errp);
+/**
+ * hotplug_handler_unplug:
+ *
+ * Calls #HotplugHandlerClass.unplug callback of @plug_handler.
+ */
+void hotplug_handler_unplug(HotplugHandler *plug_handler,
+                            DeviceState *plugged_dev,
+                            Error **errp);
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH 12/30] qdev: add simple/generic unplug callback for HotplugHandler

[Igor Mammedov]

it will be used in shallow conversion from legacy hotplug
mechanism and eventually replace all the uses of old mechanism
DeviceClass->unplug = qdev_simple_unplug_cb()

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/core/qdev.c         | 5 +++++
 include/hw/qdev-core.h | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index c89d781..1d1b113 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -286,6 +286,11 @@ int qdev_simple_unplug_cb(DeviceState *dev)
     return 0;
 }
 
+void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
+                                  DeviceState *dev, Error **errp)
+{
+    qdev_simple_unplug_cb(dev);
+}
 
 /* Like qdev_init(), but terminate program via error_report() instead of
    returning an error value.  This is okay during machine creation.
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 48a96d2..ba812c5 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -265,6 +265,8 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
 void qdev_unplug(DeviceState *dev, Error **errp);
 int qdev_simple_unplug_cb(DeviceState *dev);
+void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
+                                  DeviceState *dev, Error **errp);
 void qdev_machine_creation_done(void);
 bool qdev_machine_modified(void);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 13/30] qdev: hotplug: set handler only if HOTPLUG_HANDLER interface is supported

[Igor Mammedov]

Check if 'handler' implements HOTPLUG_HANDLER interface
before setting it, if it's not then do nothing and leave
bus not hotpluggable.

That would allow to reuse the same code for creating bus
for example 'scsi_bus_new()' for both hotpluggable and not
hotpluggable controllers.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
PS:
Keep allow_hotplug = 1 before check to preserve legacy
behaviour for incremental conversion without breaking
bisect-ability. I will be removed later in series.
---
 include/hw/qdev-core.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index ba812c5..653ff4a 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -366,9 +366,14 @@ char *qdev_get_dev_path(DeviceState *dev);
 static inline void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler,
                                             Error **errp)
 {
+    bus->allow_hotplug = 1;
+
+    if (!object_dynamic_cast(OBJECT(handler), TYPE_HOTPLUG_HANDLER)) {
+        return;
+    }
+
     object_property_set_link(OBJECT(bus), OBJECT(handler),
                              QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
-    bus->allow_hotplug = 1;
 }
 
 static inline bool qbus_is_hotpluggable(BusState *bus)
-- 
1.8.3.1

[Qemu-devel] [PATCH 14/30] target-i386: ICC bus: replace BusState.allow_hotplug with hotplug_handler

[Igor Mammedov]

it will allow to drop BusState.allow_hotplug field
and still keep ICC bus hotluggable for CPU/APIC devices.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/cpu/icc_bus.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
index 7f44c59..cb5ed5c 100644
--- a/hw/cpu/icc_bus.c
+++ b/hw/cpu/icc_bus.c
@@ -24,18 +24,10 @@
 
 /* icc-bridge implementation */
 
-static void icc_bus_init(Object *obj)
-{
-    BusState *b = BUS(obj);
-
-    b->allow_hotplug = true;
-}
-
 static const TypeInfo icc_bus_info = {
     .name = TYPE_ICC_BUS,
     .parent = TYPE_BUS,
     .instance_size = sizeof(ICCBus),
-    .instance_init = icc_bus_init,
 };
 
 
@@ -100,11 +92,19 @@ static void icc_bridge_init(Object *obj)
     s->icc_bus.apic_address_space = &s->apic_container;
 }
 
+static void icc_bridge_realize(DeviceState *dev, Error **errp)
+{
+    ICCBridgeState *s = ICC_BRIGDE(dev);
+
+    qbus_set_hotplug_handler(BUS(&s->icc_bus), dev, errp);
+}
+
 static void icc_bridge_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+    dc->realize = icc_bridge_realize;
 }
 
 static const TypeInfo icc_bridge_info = {
@@ -113,6 +113,10 @@ static const TypeInfo icc_bridge_info = {
     .instance_init  = icc_bridge_init,
     .instance_size  = sizeof(ICCBridgeState),
     .class_init = icc_bridge_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 15/30] virtio-pci: replace BusState.allow_hotplug with hotplug_handler

[Igor Mammedov]

Although virtio-pci-bus is internal object of composite
virtio-pci device and it doesn't participate in
-device/device_add hotplug flow, it's required by
bus_add_child() that bus must be hotpluggable to
be able to add child at runtime.
Set parent of virtio-pci-bus as NOP hotplug controller,
so that bus_add_child() would allow to add child
device during hotplug time when BusState.allow_hotplug
is dropped.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/virtio/virtio-pci.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index f560814..0486b25 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1070,6 +1070,10 @@ static const TypeInfo virtio_pci_info = {
     .class_init    = virtio_pci_class_init,
     .class_size    = sizeof(VirtioPCIClass),
     .abstract      = true,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 /* virtio-blk-pci */
@@ -1543,13 +1547,11 @@ static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
                                VirtIOPCIProxy *dev)
 {
     DeviceState *qdev = DEVICE(dev);
-    BusState *qbus;
     char virtio_bus_name[] = "virtio-bus";
 
     qbus_create_inplace(bus, bus_size, TYPE_VIRTIO_PCI_BUS, qdev,
                         virtio_bus_name);
-    qbus = BUS(bus);
-    qbus->allow_hotplug = 1;
+    qbus_set_hotplug_handler(BUS(bus), qdev, &error_abort);
 }
 
 static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
-- 
1.8.3.1

[Qemu-devel] [PATCH 16/30] virtio-serial: convert to hotplug-handler API

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/char/virtio-serial-bus.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 3931085..c6870f1 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -904,6 +904,12 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
     }
 
     port->elem.out_num = 0;
+}
+
+static void virtser_port_device_plug(HotplugHandler *hotplug_dev,
+                                     DeviceState *dev, Error **errp)
+{
+    VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(dev);
 
     QTAILQ_INSERT_TAIL(&port->vser->ports, port, next);
     port->ivq = port->vser->ivqs[port->id];
@@ -912,7 +918,7 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
     add_port(port->vser, port->id);
 
     /* Send an update to the guest about this new port added */
-    virtio_notify_config(vdev);
+    virtio_notify_config(VIRTIO_DEVICE(hotplug_dev));
 }
 
 static void virtser_port_device_unrealize(DeviceState *dev, Error **errp)
@@ -935,7 +941,6 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOSerial *vser = VIRTIO_SERIAL(dev);
-    BusState *bus;
     uint32_t i, max_supported_ports;
 
     if (!vser->serial.max_virtserial_ports) {
@@ -957,8 +962,7 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
     /* Spawn a new virtio-serial bus on which the ports will ride as devices */
     qbus_create_inplace(&vser->bus, sizeof(vser->bus), TYPE_VIRTIO_SERIAL_BUS,
                         dev, vdev->bus_name);
-    bus = BUS(&vser->bus);
-    bus->allow_hotplug = 1;
+    qbus_set_hotplug_handler(BUS(&vser->bus), DEVICE(vser), errp);
     vser->bus.vser = vser;
     QTAILQ_INIT(&vser->ports);
 
@@ -1021,7 +1025,6 @@ static void virtio_serial_port_class_init(ObjectClass *klass, void *data)
     k->bus_type = TYPE_VIRTIO_SERIAL_BUS;
     k->realize = virtser_port_device_realize;
     k->unrealize = virtser_port_device_unrealize;
-    k->unplug = qdev_simple_unplug_cb;
     k->props = virtser_props;
 }
 
@@ -1064,6 +1067,7 @@ static void virtio_serial_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     QLIST_INIT(&vserdevices.devices);
 
@@ -1077,6 +1081,8 @@ static void virtio_serial_class_init(ObjectClass *klass, void *data)
     vdc->reset = vser_reset;
     vdc->save = virtio_serial_save_device;
     vdc->load = virtio_serial_load_device;
+    hc->plug = virtser_port_device_plug;
+    hc->unplug = qdev_simple_device_unplug_cb;
 }
 
 static const TypeInfo virtio_device_info = {
@@ -1084,6 +1090,10 @@ static const TypeInfo virtio_device_info = {
     .parent = TYPE_VIRTIO_DEVICE,
     .instance_size = sizeof(VirtIOSerial),
     .class_init = virtio_serial_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void virtio_serial_register_types(void)
-- 
1.8.3.1

[Qemu-devel] [PATCH 17/30] virtio-mmio: drop useless bus->allow_hotplug = 0

[Igor Mammedov]

Bus by default is not hotpluggable.
virtio-mmio-bus and its parent types do not set allow_hotplug
anywhere explicitly, so remove not needed field access
and wrapper along with it.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/virtio/virtio-mmio.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 18c6e5b..2450c13 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -89,9 +89,6 @@ typedef struct {
     VirtioBusState bus;
 } VirtIOMMIOProxy;
 
-static void virtio_mmio_bus_new(VirtioBusState *bus, size_t bus_size,
-                                VirtIOMMIOProxy *dev);
-
 static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)
 {
     VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
@@ -362,7 +359,8 @@ static void virtio_mmio_realizefn(DeviceState *d, Error **errp)
     VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d);
     SysBusDevice *sbd = SYS_BUS_DEVICE(d);
 
-    virtio_mmio_bus_new(&proxy->bus, sizeof(proxy->bus), proxy);
+    qbus_create_inplace(&proxy->bus, sizeof(proxy->bus), TYPE_VIRTIO_MMIO_BUS,
+                        d, NULL);
     sysbus_init_irq(sbd, &proxy->irq);
     memory_region_init_io(&proxy->iomem, OBJECT(d), &virtio_mem_ops, proxy,
                           TYPE_VIRTIO_MMIO, 0x200);
@@ -393,17 +391,6 @@ static const TypeInfo virtio_mmio_info = {
 
 /* virtio-mmio-bus. */
 
-static void virtio_mmio_bus_new(VirtioBusState *bus, size_t bus_size,
-                                VirtIOMMIOProxy *dev)
-{
-    DeviceState *qdev = DEVICE(dev);
-    BusState *qbus;
-
-    qbus_create_inplace(bus, bus_size, TYPE_VIRTIO_MMIO_BUS, qdev, NULL);
-    qbus = BUS(bus);
-    qbus->allow_hotplug = 0;
-}
-
 static void virtio_mmio_bus_class_init(ObjectClass *klass, void *data)
 {
     BusClass *bus_class = BUS_CLASS(klass);
-- 
1.8.3.1

[Qemu-devel] [PATCH 18/30] s390x: drop not used allow_hotplug in event-facility

[Igor Mammedov]

s390-sclp-event-facility creates s390-sclp-events-bus
and immeadiatly sets its allow_hotplug field to 0,
which is NOP since it's already 0 by default.

Also since BUS is not hotpluggable, it's not possible
to call SCLP_EVENT{ DeviceClass.unplug } callback
from qdev_unplug() making this unreachable code,
so drop it as well.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/s390x/event-facility.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 597db34..78da718 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -333,7 +333,6 @@ static int init_event_facility(SCLPEventFacility *event_facility)
     /* Spawn a new bus for SCLP events */
     qbus_create_inplace(&event_facility->sbus, sizeof(event_facility->sbus),
                         TYPE_SCLP_EVENTS_BUS, sdev, NULL);
-    event_facility->sbus.qbus.allow_hotplug = 0;
 
     quiesce = qdev_create(&event_facility->sbus.qbus, "sclpquiesce");
     if (!quiesce) {
@@ -408,7 +407,6 @@ static void event_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->bus_type = TYPE_SCLP_EVENTS_BUS;
-    dc->unplug = qdev_simple_unplug_cb;
     dc->realize = event_realize;
     dc->unrealize = event_unrealize;
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH 19/30] s390x: convert s390-virtio to hotplug handler API

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/s390x/s390-virtio-bus.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 6b6fb61..97a907c 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -102,7 +102,7 @@ VirtIOS390Bus *s390_virtio_bus_init(ram_addr_t *ram_size)
     bus->next_ring = bus->dev_page + TARGET_PAGE_SIZE;
 
     /* Enable hotplugging */
-    _bus->allow_hotplug = 1;
+    qbus_set_hotplug_handler(_bus, dev, &error_abort);
 
     /* Allocate RAM for VirtIO device pages (descriptors, queues, rings) */
     *ram_size += S390_DEVICE_PAGES * TARGET_PAGE_SIZE;
@@ -597,11 +597,12 @@ static void s390_virtio_busdev_reset(DeviceState *dev)
 static void virtio_s390_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     dc->init = s390_virtio_busdev_init;
     dc->bus_type = TYPE_S390_VIRTIO_BUS;
-    dc->unplug = qdev_simple_unplug_cb;
     dc->reset = s390_virtio_busdev_reset;
+    hc->unplug = qdev_simple_device_unplug_cb;
 }
 
 static const TypeInfo virtio_s390_device_info = {
@@ -611,6 +612,10 @@ static const TypeInfo virtio_s390_device_info = {
     .class_init = virtio_s390_device_class_init,
     .class_size = sizeof(VirtIOS390DeviceClass),
     .abstract = true,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static Property s390_virtio_scsi_properties[] = {
@@ -674,8 +679,10 @@ static int s390_virtio_bridge_init(SysBusDevice *dev)
 static void s390_virtio_bridge_class_init(ObjectClass *klass, void *data)
 {
     SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     k->init = s390_virtio_bridge_init;
+    hc->unplug = qdev_simple_device_unplug_cb;
 }
 
 static const TypeInfo s390_virtio_bridge_info = {
@@ -683,6 +690,10 @@ static const TypeInfo s390_virtio_bridge_info = {
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(SysBusDevice),
     .class_init    = s390_virtio_bridge_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 /* virtio-s390-bus */
@@ -691,13 +702,11 @@ static void virtio_s390_bus_new(VirtioBusState *bus, size_t bus_size,
                                 VirtIOS390Device *dev)
 {
     DeviceState *qdev = DEVICE(dev);
-    BusState *qbus;
     char virtio_bus_name[] = "virtio-bus";
 
     qbus_create_inplace(bus, bus_size, TYPE_VIRTIO_S390_BUS,
                         qdev, virtio_bus_name);
-    qbus = BUS(bus);
-    qbus->allow_hotplug = 1;
+    qbus_set_hotplug_handler(BUS(bus), qdev, &error_abort);
 }
 
 static void virtio_s390_bus_class_init(ObjectClass *klass, void *data)
-- 
1.8.3.1

[Qemu-devel] [PATCH 20/30] s390x: convert virtio-ccw to hotplug handler API

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/s390x/virtio-ccw.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 33a1d86..ccd6046 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -230,7 +230,7 @@ VirtualCssBus *virtual_css_bus_init(void)
     cbus = VIRTUAL_CSS_BUS(bus);
 
     /* Enable hotplugging */
-    bus->allow_hotplug = 1;
+    qbus_set_hotplug_handler(bus, dev, &error_abort);
 
     return cbus;
 }
@@ -1590,7 +1590,8 @@ static int virtio_ccw_busdev_exit(DeviceState *dev)
     return _info->exit(_dev);
 }
 
-static int virtio_ccw_busdev_unplug(DeviceState *dev)
+static void virtio_ccw_busdev_unplug(HotplugHandler *hotplug_dev,
+                                     DeviceState *dev, Error **errp)
 {
     VirtioCcwDevice *_dev = (VirtioCcwDevice *)dev;
     SubchDev *sch = _dev->sch;
@@ -1609,7 +1610,6 @@ static int virtio_ccw_busdev_unplug(DeviceState *dev)
     css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid, 1, 0);
 
     object_unparent(OBJECT(dev));
-    return 0;
 }
 
 static Property virtio_ccw_properties[] = {
@@ -1620,13 +1620,13 @@ static Property virtio_ccw_properties[] = {
 static void virtio_ccw_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     dc->props = virtio_ccw_properties;
     dc->init = virtio_ccw_busdev_init;
     dc->exit = virtio_ccw_busdev_exit;
-    dc->unplug = virtio_ccw_busdev_unplug;
     dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
-
+    hc->unplug = virtio_ccw_busdev_unplug;
 }
 
 static const TypeInfo virtio_ccw_device_info = {
@@ -1636,6 +1636,10 @@ static const TypeInfo virtio_ccw_device_info = {
     .class_init = virtio_ccw_device_class_init,
     .class_size = sizeof(VirtIOCCWDeviceClass),
     .abstract = true,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 /***************** Virtual-css Bus Bridge Device ********************/
@@ -1650,8 +1654,10 @@ static int virtual_css_bridge_init(SysBusDevice *dev)
 static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
 {
     SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     k->init = virtual_css_bridge_init;
+    hc->unplug = qdev_simple_device_unplug_cb;
 }
 
 static const TypeInfo virtual_css_bridge_info = {
@@ -1659,6 +1665,10 @@ static const TypeInfo virtual_css_bridge_info = {
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(SysBusDevice),
     .class_init    = virtual_css_bridge_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 /* virtio-ccw-bus */
@@ -1667,13 +1677,11 @@ static void virtio_ccw_bus_new(VirtioBusState *bus, size_t bus_size,
                                VirtioCcwDevice *dev)
 {
     DeviceState *qdev = DEVICE(dev);
-    BusState *qbus;
     char virtio_bus_name[] = "virtio-bus";
 
     qbus_create_inplace(bus, bus_size, TYPE_VIRTIO_CCW_BUS,
                         qdev, virtio_bus_name);
-    qbus = BUS(bus);
-    qbus->allow_hotplug = 1;
+    qbus_set_hotplug_handler(BUS(bus), qdev, &error_abort);
 }
 
 static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data)
-- 
1.8.3.1

[Qemu-devel] [PATCH 21/30] scsi: make scsi_bus_new() assign hotplug controller

[Igor Mammedov]

it will allow incrementally convert every controller that
creates SCSI bus and supports hotplug to hotplug controller API

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/scsi/scsi-bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 954c607..33495b6 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -92,7 +92,7 @@ void scsi_bus_new(SCSIBus *bus, size_t bus_size, DeviceState *host,
     qbus_create_inplace(bus, bus_size, TYPE_SCSI_BUS, host, bus_name);
     bus->busnr = next_scsi_bus++;
     bus->info = info;
-    bus->qbus.allow_hotplug = 1;
+    qbus_set_hotplug_handler(BUS(bus), host, &error_abort);
 }
 
 static void scsi_dma_restart_bh(void *opaque)
-- 
1.8.3.1

[Qemu-devel] [PATCH 22/30] scsi: convert pvscsi HBA to hotplug hander API

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
not tested since it's broken, i.e. QEMU hangs in BIOS
with pvscsi device present on CLI
---
 hw/scsi/vmw_pvscsi.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index 5734d19..10586c4 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -524,17 +524,20 @@ pvscsi_send_msg(PVSCSIState *s, SCSIDevice *dev, uint32_t msg_type)
 }
 
 static void
-pvscsi_hotplug(SCSIBus *bus, SCSIDevice *dev)
+pvscsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp)
 {
-    PVSCSIState *s = container_of(bus, PVSCSIState, bus);
-    pvscsi_send_msg(s, dev, PVSCSI_MSG_DEV_ADDED);
+    PVSCSIState *s = PVSCSI(hotplug_dev);
+
+    pvscsi_send_msg(s, SCSI_DEVICE(dev), PVSCSI_MSG_DEV_ADDED);
 }
 
 static void
-pvscsi_hot_unplug(SCSIBus *bus, SCSIDevice *dev)
+pvscsi_hot_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp)
 {
-    PVSCSIState *s = container_of(bus, PVSCSIState, bus);
-    pvscsi_send_msg(s, dev, PVSCSI_MSG_DEV_REMOVED);
+    PVSCSIState *s = PVSCSI(hotplug_dev);
+
+    pvscsi_send_msg(s, SCSI_DEVICE(dev), PVSCSI_MSG_DEV_REMOVED);
+    qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
 }
 
 static void
@@ -1057,8 +1060,6 @@ static const struct SCSIBusInfo pvscsi_scsi_info = {
         .get_sg_list = pvscsi_get_sg_list,
         .complete = pvscsi_command_complete,
         .cancel = pvscsi_request_cancelled,
-        .hotplug = pvscsi_hotplug,
-        .hot_unplug = pvscsi_hot_unplug,
 };
 
 static int
@@ -1187,6 +1188,7 @@ static void pvscsi_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     k->init = pvscsi_init;
     k->exit = pvscsi_uninit;
@@ -1199,6 +1201,8 @@ static void pvscsi_class_init(ObjectClass *klass, void *data)
     dc->props = pvscsi_properties;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     k->config_write = pvscsi_write_config;
+    hc->unplug = pvscsi_hot_unplug;
+    hc->plug = pvscsi_hotplug;
 }
 
 static const TypeInfo pvscsi_info = {
@@ -1206,6 +1210,10 @@ static const TypeInfo pvscsi_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PVSCSIState),
     .class_init    = pvscsi_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void
-- 
1.8.3.1

[Qemu-devel] [PATCH 23/30] scsi: convert virtio-scsi HBA to hotplug handler API

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/scsi/virtio-scsi.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 86aba88..aeda1cf 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -649,26 +649,29 @@ static void virtio_scsi_change(SCSIBus *bus, SCSIDevice *dev, SCSISense sense)
     }
 }
 
-static void virtio_scsi_hotplug(SCSIBus *bus, SCSIDevice *dev)
+static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                                Error **errp)
 {
-    VirtIOSCSI *s = container_of(bus, VirtIOSCSI, bus);
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+    VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev);
 
     if ((vdev->guest_features >> VIRTIO_SCSI_F_HOTPLUG) & 1) {
-        virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_TRANSPORT_RESET,
+        virtio_scsi_push_event(VIRTIO_SCSI(hotplug_dev), SCSI_DEVICE(dev),
+                               VIRTIO_SCSI_T_TRANSPORT_RESET,
                                VIRTIO_SCSI_EVT_RESET_RESCAN);
     }
 }
 
-static void virtio_scsi_hot_unplug(SCSIBus *bus, SCSIDevice *dev)
+static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                                  Error **errp)
 {
-    VirtIOSCSI *s = container_of(bus, VirtIOSCSI, bus);
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+    VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev);
 
     if ((vdev->guest_features >> VIRTIO_SCSI_F_HOTPLUG) & 1) {
-        virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_TRANSPORT_RESET,
+        virtio_scsi_push_event(VIRTIO_SCSI(hotplug_dev), SCSI_DEVICE(dev),
+                               VIRTIO_SCSI_T_TRANSPORT_RESET,
                                VIRTIO_SCSI_EVT_RESET_REMOVED);
     }
+    qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
 }
 
 static struct SCSIBusInfo virtio_scsi_scsi_info = {
@@ -680,8 +683,6 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = {
     .complete = virtio_scsi_command_complete,
     .cancel = virtio_scsi_request_cancelled,
     .change = virtio_scsi_change,
-    .hotplug = virtio_scsi_hotplug,
-    .hot_unplug = virtio_scsi_hot_unplug,
     .parse_cdb = virtio_scsi_parse_cdb,
     .get_sg_list = virtio_scsi_get_sg_list,
     .save_request = virtio_scsi_save_request,
@@ -785,6 +786,7 @@ static void virtio_scsi_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     dc->props = virtio_scsi_properties;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
@@ -793,6 +795,8 @@ static void virtio_scsi_class_init(ObjectClass *klass, void *data)
     vdc->set_config = virtio_scsi_set_config;
     vdc->get_features = virtio_scsi_get_features;
     vdc->reset = virtio_scsi_reset;
+    hc->plug = virtio_scsi_hotplug;
+    hc->unplug = virtio_scsi_hotunplug;
 }
 
 static const TypeInfo virtio_scsi_common_info = {
@@ -808,6 +812,10 @@ static const TypeInfo virtio_scsi_info = {
     .parent = TYPE_VIRTIO_SCSI_COMMON,
     .instance_size = sizeof(VirtIOSCSI),
     .class_init = virtio_scsi_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void virtio_register_types(void)
-- 
1.8.3.1

[Qemu-devel] [PATCH 24/30] scsi: cleanup not used anymore SCSIBusInfo{hotplug, hot_unplug} fields

[Igor Mammedov]

SCSI subsytem was converted to hotplug handler API and
doesn't use SCSIBusInfo{hotplug, hot_unplug} fields and
related callbacks anymore.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/scsi/scsi-bus.c     | 16 ----------------
 include/hw/scsi/scsi.h |  2 --
 2 files changed, 18 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 33495b6..9bdc63b 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -202,10 +202,6 @@ static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
     }
     dev->vmsentry = qemu_add_vm_change_state_handler(scsi_dma_restart_cb,
                                                      dev);
-
-    if (bus->info->hotplug) {
-        bus->info->hotplug(bus, dev);
-    }
 }
 
 static void scsi_qdev_unrealize(DeviceState *qdev, Error **errp)
@@ -1918,17 +1914,6 @@ static int get_scsi_requests(QEMUFile *f, void *pv, size_t size)
     return 0;
 }
 
-static int scsi_qdev_unplug(DeviceState *qdev)
-{
-    SCSIDevice *dev = SCSI_DEVICE(qdev);
-    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
-
-    if (bus->info->hot_unplug) {
-        bus->info->hot_unplug(bus, dev);
-    }
-    return qdev_simple_unplug_cb(qdev);
-}
-
 static const VMStateInfo vmstate_info_scsi_requests = {
     .name = "scsi-requests",
     .get  = get_scsi_requests,
@@ -1992,7 +1977,6 @@ static void scsi_device_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
     k->bus_type  = TYPE_SCSI_BUS;
     k->realize   = scsi_qdev_realize;
-    k->unplug    = scsi_qdev_unplug;
     k->unrealize = scsi_qdev_unrealize;
     k->props     = scsi_props;
 }
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 2e3a8f9..8a2581e 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -138,8 +138,6 @@ struct SCSIBusInfo {
     void (*transfer_data)(SCSIRequest *req, uint32_t arg);
     void (*complete)(SCSIRequest *req, uint32_t arg, size_t resid);
     void (*cancel)(SCSIRequest *req);
-    void (*hotplug)(SCSIBus *bus, SCSIDevice *dev);
-    void (*hot_unplug)(SCSIBus *bus, SCSIDevice *dev);
     void (*change)(SCSIBus *bus, SCSIDevice *dev, SCSISense sense);
     QEMUSGList *(*get_sg_list)(SCSIRequest *req);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 25/30] usb-bot: drop not needed "allow_hotplug = 0"

[Igor Mammedov]

usb-bot creates SCSI bus and immediately makes it
non hotpluggable which makes not possible to add
anything to it after machine is created.

Taking in account that ubs-bot doesn't implement
HOTPLUG_HANDLER interface, SCSI will be created as
non hotpluggable, so there is not need to reset
allow_hotplug field anymore.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/usb/dev-storage.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index bd7cc53..bfa71c7 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -653,7 +653,6 @@ static void usb_msd_realize_bot(USBDevice *dev, Error **errp)
     usb_desc_init(dev);
     scsi_bus_new(&s->bus, sizeof(s->bus), DEVICE(dev),
                  &usb_msd_scsi_info_bot, NULL);
-    s->bus.qbus.allow_hotplug = 0;
     usb_msd_handle_reset(dev);
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 26/30] usb-storage: make its storage SCSI bus hotpluggable explicitly

[Igor Mammedov]

usb-storage uses SCSI bus to provide underling storage
(i.e. scsi-disk) and it's hotpluggable. In addition
during usb-storage hotplug bus_add_child() won't allow
to add scsi-disk to SCSI bus owned by usb-storage
if bus is not hotpluggable.

By default SCSI bus was alway hotpluggable even if
an owner didn't support hotplug. It will change
once BusState.allow_hotplug field is dropped and
SCSI bus will be only hotpluggable only if bus owner
implements HOTPLUG_HANDLER interface.

So make usb-storage implement HOTPLUG_HANDLER interface
to keep its SCSI bus hotpluggable.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/usb/dev-storage.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index bfa71c7..4ad4ec7 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -779,6 +779,10 @@ static const TypeInfo msd_info = {
     .parent        = TYPE_USB_DEVICE,
     .instance_size = sizeof(MSDState),
     .class_init    = usb_msd_class_initfn_storage,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static const TypeInfo bot_info = {
-- 
1.8.3.1

[Qemu-devel] [PATCH 27/30] usb-storage: drop not needed "allow_hotplug = 0"

[Igor Mammedov]

Drop useless hack that disables hotplug on bus, after backend
storage was added to it, by setting "allow_hotplug = 0". Even
if bus is hotpluggable, It won't be possible to add another
SCSI device to bus since its realize will fail early with
error "no free target" in scsi_qdev_realize() method.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/usb/dev-storage.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 4ad4ec7..b168d7c 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -632,7 +632,6 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
         error_propagate(errp, err);
         return;
     }
-    s->bus.qbus.allow_hotplug = 0;
     usb_msd_handle_reset(dev);
 
     if (bdrv_key_required(bs)) {
-- 
1.8.3.1

[Qemu-devel] [PATCH 28/30] usb: convert to hotplug handler API

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/usb/bus.c                  |  3 +--
 hw/usb/dev-smartcard-reader.c |  8 +++++++-
 hw/usb/hcd-ehci-pci.c         |  6 ++++++
 hw/usb/hcd-ehci-sysbus.c      |  6 ++++++
 hw/usb/hcd-ohci.c             | 12 ++++++++++++
 hw/usb/hcd-uhci.c             |  6 ++++++
 hw/usb/hcd-xhci.c             |  6 ++++++
 7 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index da1eba9..16c2bb3 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -81,7 +81,7 @@ void usb_bus_new(USBBus *bus, size_t bus_size,
     qbus_create_inplace(bus, bus_size, TYPE_USB_BUS, host, NULL);
     bus->ops = ops;
     bus->busnr = next_usb_bus++;
-    bus->qbus.allow_hotplug = 1; /* Yes, we can */
+    qbus_set_hotplug_handler(BUS(bus), host, &error_abort);
     QTAILQ_INIT(&bus->free);
     QTAILQ_INIT(&bus->used);
     QTAILQ_INSERT_TAIL(&busses, bus, next);
@@ -701,7 +701,6 @@ static void usb_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
     k->bus_type = TYPE_USB_BUS;
-    k->unplug   = qdev_simple_unplug_cb;
     k->realize  = usb_qdev_realize;
     k->unrealize = usb_qdev_unrealize;
     k->props    = usb_props;
diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index d37ed02..91f7d87 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -1313,7 +1313,7 @@ static void ccid_realize(USBDevice *dev, Error **errp)
     qbus_create_inplace(&s->bus, sizeof(s->bus), TYPE_CCID_BUS, DEVICE(dev),
                         NULL);
     s->intr = usb_ep_get(dev, USB_TOKEN_IN, CCID_INT_IN_EP);
-    s->bus.qbus.allow_hotplug = 1;
+    qbus_set_hotplug_handler(BUS(&s->bus), DEVICE(dev), &error_abort);
     s->card = NULL;
     s->migration_state = MIGRATION_NONE;
     s->migration_target_ip = 0;
@@ -1439,6 +1439,7 @@ static void ccid_class_initfn(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     USBDeviceClass *uc = USB_DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     uc->realize        = ccid_realize;
     uc->product_desc   = "QEMU USB CCID";
@@ -1451,6 +1452,7 @@ static void ccid_class_initfn(ObjectClass *klass, void *data)
     dc->vmsd = &ccid_vmstate;
     dc->props = ccid_properties;
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
+    hc->unplug = qdev_simple_device_unplug_cb;
 }
 
 static const TypeInfo ccid_info = {
@@ -1458,6 +1460,10 @@ static const TypeInfo ccid_info = {
     .parent        = TYPE_USB_DEVICE,
     .instance_size = sizeof(USBCCIDState),
     .class_init    = ccid_class_initfn,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void ccid_card_class_init(ObjectClass *klass, void *data)
diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c
index 490f2b6..b7990da 100644
--- a/hw/usb/hcd-ehci-pci.c
+++ b/hw/usb/hcd-ehci-pci.c
@@ -138,6 +138,7 @@ static void ehci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     k->init = usb_ehci_pci_initfn;
     k->exit = usb_ehci_pci_exit;
@@ -145,6 +146,7 @@ static void ehci_class_init(ObjectClass *klass, void *data)
     k->config_write = usb_ehci_pci_write_config;
     dc->vmsd = &vmstate_ehci_pci;
     dc->props = ehci_pci_properties;
+    hc->unplug = qdev_simple_device_unplug_cb;
 }
 
 static const TypeInfo ehci_pci_type_info = {
@@ -154,6 +156,10 @@ static const TypeInfo ehci_pci_type_info = {
     .instance_init = usb_ehci_pci_init,
     .abstract = true,
     .class_init = ehci_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void ehci_data_class_init(ObjectClass *klass, void *data)
diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c
index 19ed2c2..25eb885 100644
--- a/hw/usb/hcd-ehci-sysbus.c
+++ b/hw/usb/hcd-ehci-sysbus.c
@@ -63,6 +63,7 @@ static void ehci_sysbus_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     sec->portscbase = 0x44;
     sec->portnr = NB_PORTS;
@@ -71,6 +72,7 @@ static void ehci_sysbus_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_ehci_sysbus;
     dc->props = ehci_sysbus_properties;
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
+    hc->unplug = qdev_simple_device_unplug_cb;
 }
 
 static const TypeInfo ehci_type_info = {
@@ -81,6 +83,10 @@ static const TypeInfo ehci_type_info = {
     .abstract      = true,
     .class_init    = ehci_sysbus_class_init,
     .class_size    = sizeof(SysBusEHCIClass),
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void ehci_xlnx_class_init(ObjectClass *oc, void *data)
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 7ea871d..5b2552f 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -2083,6 +2083,7 @@ static void ohci_pci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     k->init = usb_ohci_initfn_pci;
     k->exit = usb_ohci_exit;
@@ -2094,6 +2095,7 @@ static void ohci_pci_class_init(ObjectClass *klass, void *data)
     dc->props = ohci_pci_properties;
     dc->hotpluggable = false;
     dc->vmsd = &vmstate_ohci;
+    hc->unplug = qdev_simple_device_unplug_cb;
 }
 
 static const TypeInfo ohci_pci_info = {
@@ -2101,6 +2103,10 @@ static const TypeInfo ohci_pci_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(OHCIPCIState),
     .class_init    = ohci_pci_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static Property ohci_sysbus_properties[] = {
@@ -2112,11 +2118,13 @@ static Property ohci_sysbus_properties[] = {
 static void ohci_sysbus_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     dc->realize = ohci_realize_pxa;
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
     dc->desc = "OHCI USB Controller";
     dc->props = ohci_sysbus_properties;
+    hc->unplug = qdev_simple_device_unplug_cb;
 }
 
 static const TypeInfo ohci_sysbus_info = {
@@ -2124,6 +2132,10 @@ static const TypeInfo ohci_sysbus_info = {
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(OHCISysBusState),
     .class_init    = ohci_sysbus_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void ohci_register_types(void)
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 5b88f30..76f6deb 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -1298,6 +1298,7 @@ static void uhci_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
     UHCIPCIDeviceClass *u = container_of(k, UHCIPCIDeviceClass, parent_class);
     UHCIInfo *info = data;
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     k->init = info->initfn ? info->initfn : usb_uhci_common_initfn;
     k->exit = info->unplug ? usb_uhci_exit : NULL;
@@ -1315,6 +1316,7 @@ static void uhci_class_init(ObjectClass *klass, void *data)
     }
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
     u->info = *info;
+    hc->unplug = qdev_simple_device_unplug_cb;
 }
 
 static UHCIInfo uhci_info[] = {
@@ -1392,6 +1394,10 @@ static void uhci_register_types(void)
         .instance_size = sizeof(UHCIState),
         .class_size    = sizeof(UHCIPCIDeviceClass),
         .class_init    = uhci_class_init,
+        .interfaces = (InterfaceInfo[]) {
+            { TYPE_HOTPLUG_HANDLER },
+            { }
+        }
     };
     int i;
 
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index c556367..5ef74ee 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3870,6 +3870,7 @@ static void xhci_class_init(ObjectClass *klass, void *data)
 {
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     dc->vmsd    = &vmstate_xhci;
     dc->props   = xhci_properties;
@@ -3882,6 +3883,7 @@ static void xhci_class_init(ObjectClass *klass, void *data)
     k->class_id     = PCI_CLASS_SERIAL_USB;
     k->revision     = 0x03;
     k->is_express   = 1;
+    hc->unplug = qdev_simple_device_unplug_cb;
 }
 
 static const TypeInfo xhci_info = {
@@ -3889,6 +3891,10 @@ static const TypeInfo xhci_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(XHCIState),
     .class_init    = xhci_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void xhci_register_types(void)
-- 
1.8.3.1

[Qemu-devel] [PATCH 29/30] qdev: drop legacy hotplug fields/methods

[Igor Mammedov]

It removes not needed anymore BusState.allow_hotplug field and
DeviceClass.unplug callback.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/core/qdev.c         | 15 +++------------
 include/hw/qdev-core.h |  7 +------
 2 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 1d1b113..4182cc3 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -239,10 +239,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
             hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
         }
     } else {
-        assert(dc->unplug != NULL);
-        if (dc->unplug(dev) < 0) { /* legacy handler */
-            error_set(errp, QERR_UNDEFINED_ERROR);
-        }
+        assert(0);
     }
 }
 
@@ -279,17 +276,11 @@ void qbus_reset_all_fn(void *opaque)
 }
 
 /* can be used as ->unplug() callback for the simple cases */
-int qdev_simple_unplug_cb(DeviceState *dev)
-{
-    /* just zap it */
-    object_unparent(OBJECT(dev));
-    return 0;
-}
-
 void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
                                   DeviceState *dev, Error **errp)
 {
-    qdev_simple_unplug_cb(dev);
+    /* just zap it */
+    object_unparent(OBJECT(dev));
 }
 
 /* Like qdev_init(), but terminate program via error_report() instead of
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 653ff4a..01443c9 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -126,7 +126,6 @@ typedef struct DeviceClass {
 
     /* Private to qdev / bus.  */
     qdev_initfn init; /* TODO remove, once users are converted to realize */
-    qdev_event unplug;
     qdev_event exit; /* TODO remove, once users are converted to unrealize */
     const char *bus_type;
 } DeviceClass;
@@ -210,7 +209,6 @@ struct BusState {
     Object obj;
     DeviceState *parent;
     const char *name;
-    int allow_hotplug;
     HotplugHandler *hotplug_handler;
     int max_index;
     bool realized;
@@ -264,7 +262,6 @@ void qdev_init_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
 void qdev_unplug(DeviceState *dev, Error **errp);
-int qdev_simple_unplug_cb(DeviceState *dev);
 void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
                                   DeviceState *dev, Error **errp);
 void qdev_machine_creation_done(void);
@@ -366,8 +363,6 @@ char *qdev_get_dev_path(DeviceState *dev);
 static inline void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler,
                                             Error **errp)
 {
-    bus->allow_hotplug = 1;
-
     if (!object_dynamic_cast(OBJECT(handler), TYPE_HOTPLUG_HANDLER)) {
         return;
     }
@@ -378,6 +373,6 @@ static inline void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler,
 
 static inline bool qbus_is_hotpluggable(BusState *bus)
 {
-   return bus->allow_hotplug || bus->hotplug_handler;
+   return bus->hotplug_handler;
 }
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH 30/30] qdev: HotplugHandler: add support for unplugging BUS-less devices

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/core/qdev.c | 46 +++++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 4182cc3..e309b0e 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -209,9 +209,27 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
     dev->alias_required_for_version = required_for_version;
 }
 
+static HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
+{
+    HotplugHandler *hotplug_ctrl = NULL;
+
+    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_ctrl = dev->parent_bus->hotplug_handler;
+    } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
+        MachineState *machine = MACHINE(qdev_get_machine());
+        MachineClass *mc = MACHINE_GET_CLASS(machine);
+
+        if (mc->get_hotplug_handler) {
+            hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
+        }
+    }
+    return hotplug_ctrl;
+}
+
 void qdev_unplug(DeviceState *dev, Error **errp)
 {
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
+    HotplugHandler *hotplug_ctrl;
 
     if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
         error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
@@ -226,17 +244,17 @@ void qdev_unplug(DeviceState *dev, Error **errp)
 
     qdev_hot_removed = true;
 
-    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+    hotplug_ctrl = qdev_get_hotplug_handler(dev);
+    if (hotplug_ctrl) {
         HotplugHandlerClass *hdc;
 
         /* If device supports async unplug just request it to be done,
          * otherwise just remove it synchronously */
-        hdc = HOTPLUG_HANDLER_GET_CLASS(dev->parent_bus->hotplug_handler);
+        hdc = HOTPLUG_HANDLER_GET_CLASS(hotplug_ctrl);
         if (hdc->unplug_request) {
-            hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler,
-                                           dev, errp);
+            hotplug_handler_unplug_request(hotplug_ctrl, dev, errp);
         } else {
-            hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
+            hotplug_handler_unplug(hotplug_ctrl, dev, errp);
         }
     } else {
         assert(0);
@@ -817,6 +835,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
+    HotplugHandler *hotplug_ctrl;
     BusState *bus;
     Error *local_err = NULL;
 
@@ -844,20 +863,9 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             goto fail;
         }
 
-        if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
-            hotplug_handler_plug(dev->parent_bus->hotplug_handler,
-                                 dev, &local_err);
-        } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
-            HotplugHandler *hotplug_ctrl;
-            MachineState *machine = MACHINE(qdev_get_machine());
-            MachineClass *mc = MACHINE_GET_CLASS(machine);
-
-            if (mc->get_hotplug_handler) {
-                hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
-                if (hotplug_ctrl) {
-                    hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
-                }
-            }
+        hotplug_ctrl = qdev_get_hotplug_handler(dev);
+        if (hotplug_ctrl) {
+            hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
         }
 
         if (local_err != NULL) {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 30/30] qdev: HotplugHandler: add support for unplugging BUS-less devices

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/core/qdev.c | 46 +++++++++++++++++++++++++++-------------------
>  1 file changed, 27 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 4182cc3..e309b0e 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -209,9 +209,27 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>      dev->alias_required_for_version = required_for_version;
>  }
>  
> +static HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
> +{
> +    HotplugHandler *hotplug_ctrl = NULL;
> +
> +    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> +        hotplug_ctrl = dev->parent_bus->hotplug_handler;
> +    } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
> +        MachineState *machine = MACHINE(qdev_get_machine());
> +        MachineClass *mc = MACHINE_GET_CLASS(machine);
> +
> +        if (mc->get_hotplug_handler) {
> +            hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
> +        }
> +    }
> +    return hotplug_ctrl;
> +}
> +
>  void qdev_unplug(DeviceState *dev, Error **errp)
>  {
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> +    HotplugHandler *hotplug_ctrl;
>  
>      if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
>          error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
> @@ -226,17 +244,17 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>  
>      qdev_hot_removed = true;
>  
> -    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> +    hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +    if (hotplug_ctrl) {
>          HotplugHandlerClass *hdc;
>  
>          /* If device supports async unplug just request it to be done,
>           * otherwise just remove it synchronously */
> -        hdc = HOTPLUG_HANDLER_GET_CLASS(dev->parent_bus->hotplug_handler);
> +        hdc = HOTPLUG_HANDLER_GET_CLASS(hotplug_ctrl);
>          if (hdc->unplug_request) {
> -            hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler,
> -                                           dev, errp);
> +            hotplug_handler_unplug_request(hotplug_ctrl, dev, errp);
>          } else {
> -            hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
> +            hotplug_handler_unplug(hotplug_ctrl, dev, errp);
>          }
>      } else {
>          assert(0);
> @@ -817,6 +835,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>  {
>      DeviceState *dev = DEVICE(obj);
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> +    HotplugHandler *hotplug_ctrl;
>      BusState *bus;
>      Error *local_err = NULL;
>  
> @@ -844,20 +863,9 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>              goto fail;
>          }
>  
> -        if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> -            hotplug_handler_plug(dev->parent_bus->hotplug_handler,
> -                                 dev, &local_err);
> -        } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
> -            HotplugHandler *hotplug_ctrl;
> -            MachineState *machine = MACHINE(qdev_get_machine());
> -            MachineClass *mc = MACHINE_GET_CLASS(machine);
> -
> -            if (mc->get_hotplug_handler) {
> -                hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
> -                if (hotplug_ctrl) {
> -                    hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
> -                }
> -            }
> +        hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +        if (hotplug_ctrl) {
> +            hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
>          }
>  
>          if (local_err != NULL) {
> 

Let's start from the end. :)

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 29/30] qdev: drop legacy hotplug fields/methods

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> @@ -239,10 +239,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>              hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
>          }
>      } else {
> -        assert(dc->unplug != NULL);
> -        if (dc->unplug(dev) < 0) { /* legacy handler */
> -            error_set(errp, QERR_UNDEFINED_ERROR);
> -        }
> +        assert(0);
>      }

This is not particularly nice, but it makes sense at this part of the
series, since an

    assert(dev->parent_bus && dev->parent_bus->hotplug_handler);

would be changed immediately in the next patch.  Also, it would change
indentation and make the patch bigger.  Hence, please consider adding a
31st patch that changes

    hotplug_ctrl = qdev_get_hotplug_handler(dev);
    if (hotplug_ctrl) {
        ...
    } else {
        assert(0);
    }

to

    hotplug_ctrl = qdev_get_hotplug_handler(dev);
    assert(hotplug_ctrl);
    ...

Still, this patch is okay.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH 21/30] scsi: make scsi_bus_new() assign hotplug controller

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> it will allow incrementally convert every controller that
> creates SCSI bus and supports hotplug to hotplug controller API
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/scsi/scsi-bus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 954c607..33495b6 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -92,7 +92,7 @@ void scsi_bus_new(SCSIBus *bus, size_t bus_size, DeviceState *host,
>      qbus_create_inplace(bus, bus_size, TYPE_SCSI_BUS, host, bus_name);
>      bus->busnr = next_scsi_bus++;
>      bus->info = info;
> -    bus->qbus.allow_hotplug = 1;
> +    qbus_set_hotplug_handler(BUS(bus), host, &error_abort);
>  }
>  
>  static void scsi_dma_restart_bh(void *opaque)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 24/30] scsi: cleanup not used anymore SCSIBusInfo{hotplug, hot_unplug} fields

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> SCSI subsytem was converted to hotplug handler API and
> doesn't use SCSIBusInfo{hotplug, hot_unplug} fields and
> related callbacks anymore.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/scsi/scsi-bus.c     | 16 ----------------
>  include/hw/scsi/scsi.h |  2 --
>  2 files changed, 18 deletions(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 33495b6..9bdc63b 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -202,10 +202,6 @@ static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
>      }
>      dev->vmsentry = qemu_add_vm_change_state_handler(scsi_dma_restart_cb,
>                                                       dev);
> -
> -    if (bus->info->hotplug) {
> -        bus->info->hotplug(bus, dev);
> -    }
>  }
>  
>  static void scsi_qdev_unrealize(DeviceState *qdev, Error **errp)
> @@ -1918,17 +1914,6 @@ static int get_scsi_requests(QEMUFile *f, void *pv, size_t size)
>      return 0;
>  }
>  
> -static int scsi_qdev_unplug(DeviceState *qdev)
> -{
> -    SCSIDevice *dev = SCSI_DEVICE(qdev);
> -    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
> -
> -    if (bus->info->hot_unplug) {
> -        bus->info->hot_unplug(bus, dev);
> -    }
> -    return qdev_simple_unplug_cb(qdev);
> -}
> -
>  static const VMStateInfo vmstate_info_scsi_requests = {
>      .name = "scsi-requests",
>      .get  = get_scsi_requests,
> @@ -1992,7 +1977,6 @@ static void scsi_device_class_init(ObjectClass *klass, void *data)
>      set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
>      k->bus_type  = TYPE_SCSI_BUS;
>      k->realize   = scsi_qdev_realize;
> -    k->unplug    = scsi_qdev_unplug;
>      k->unrealize = scsi_qdev_unrealize;
>      k->props     = scsi_props;
>  }
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 2e3a8f9..8a2581e 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -138,8 +138,6 @@ struct SCSIBusInfo {
>      void (*transfer_data)(SCSIRequest *req, uint32_t arg);
>      void (*complete)(SCSIRequest *req, uint32_t arg, size_t resid);
>      void (*cancel)(SCSIRequest *req);
> -    void (*hotplug)(SCSIBus *bus, SCSIDevice *dev);
> -    void (*hot_unplug)(SCSIBus *bus, SCSIDevice *dev);
>      void (*change)(SCSIBus *bus, SCSIDevice *dev, SCSISense sense);
>      QEMUSGList *(*get_sg_list)(SCSIRequest *req);
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 22/30] scsi: convert pvscsi HBA to hotplug hander API

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> not tested since it's broken, i.e. QEMU hangs in BIOS
> with pvscsi device present on CLI
> ---
>  hw/scsi/vmw_pvscsi.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index 5734d19..10586c4 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -524,17 +524,20 @@ pvscsi_send_msg(PVSCSIState *s, SCSIDevice *dev, uint32_t msg_type)
>  }
>  
>  static void
> -pvscsi_hotplug(SCSIBus *bus, SCSIDevice *dev)
> +pvscsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp)
>  {
> -    PVSCSIState *s = container_of(bus, PVSCSIState, bus);
> -    pvscsi_send_msg(s, dev, PVSCSI_MSG_DEV_ADDED);
> +    PVSCSIState *s = PVSCSI(hotplug_dev);
> +
> +    pvscsi_send_msg(s, SCSI_DEVICE(dev), PVSCSI_MSG_DEV_ADDED);
>  }
>  
>  static void
> -pvscsi_hot_unplug(SCSIBus *bus, SCSIDevice *dev)
> +pvscsi_hot_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp)
>  {
> -    PVSCSIState *s = container_of(bus, PVSCSIState, bus);
> -    pvscsi_send_msg(s, dev, PVSCSI_MSG_DEV_REMOVED);
> +    PVSCSIState *s = PVSCSI(hotplug_dev);
> +
> +    pvscsi_send_msg(s, SCSI_DEVICE(dev), PVSCSI_MSG_DEV_REMOVED);
> +    qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
>  }
>  
>  static void
> @@ -1057,8 +1060,6 @@ static const struct SCSIBusInfo pvscsi_scsi_info = {
>          .get_sg_list = pvscsi_get_sg_list,
>          .complete = pvscsi_command_complete,
>          .cancel = pvscsi_request_cancelled,
> -        .hotplug = pvscsi_hotplug,
> -        .hot_unplug = pvscsi_hot_unplug,
>  };
>  
>  static int
> @@ -1187,6 +1188,7 @@ static void pvscsi_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>  
>      k->init = pvscsi_init;
>      k->exit = pvscsi_uninit;
> @@ -1199,6 +1201,8 @@ static void pvscsi_class_init(ObjectClass *klass, void *data)
>      dc->props = pvscsi_properties;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>      k->config_write = pvscsi_write_config;
> +    hc->unplug = pvscsi_hot_unplug;
> +    hc->plug = pvscsi_hotplug;
>  }
>  
>  static const TypeInfo pvscsi_info = {
> @@ -1206,6 +1210,10 @@ static const TypeInfo pvscsi_info = {
>      .parent        = TYPE_PCI_DEVICE,
>      .instance_size = sizeof(PVSCSIState),
>      .class_init    = pvscsi_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HOTPLUG_HANDLER },
> +        { }
> +    }
>  };
>  
>  static void
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 21/30] scsi: make scsi_bus_new() assign hotplug controller

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> it will allow incrementally convert every controller that
> creates SCSI bus and supports hotplug to hotplug controller API
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/scsi/scsi-bus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 954c607..33495b6 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -92,7 +92,7 @@ void scsi_bus_new(SCSIBus *bus, size_t bus_size, DeviceState *host,
>      qbus_create_inplace(bus, bus_size, TYPE_SCSI_BUS, host, bus_name);
>      bus->busnr = next_scsi_bus++;
>      bus->info = info;
> -    bus->qbus.allow_hotplug = 1;
> +    qbus_set_hotplug_handler(BUS(bus), host, &error_abort);
>  }
>  
>  static void scsi_dma_restart_bh(void *opaque)
> 

Hmm, what if the host is not a HotplugHandler?  Then you should just do
nothing for plug, and object_unparent for unplug.  Is that broken by
this patch?

Paolo

Re: [Qemu-devel] [PATCH 23/30] scsi: convert virtio-scsi HBA to hotplug handler API

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/scsi/virtio-scsi.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 86aba88..aeda1cf 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -649,26 +649,29 @@ static void virtio_scsi_change(SCSIBus *bus, SCSIDevice *dev, SCSISense sense)
>      }
>  }
>  
> -static void virtio_scsi_hotplug(SCSIBus *bus, SCSIDevice *dev)
> +static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                                Error **errp)
>  {
> -    VirtIOSCSI *s = container_of(bus, VirtIOSCSI, bus);
> -    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> +    VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev);
>  
>      if ((vdev->guest_features >> VIRTIO_SCSI_F_HOTPLUG) & 1) {
> -        virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_TRANSPORT_RESET,
> +        virtio_scsi_push_event(VIRTIO_SCSI(hotplug_dev), SCSI_DEVICE(dev),
> +                               VIRTIO_SCSI_T_TRANSPORT_RESET,
>                                 VIRTIO_SCSI_EVT_RESET_RESCAN);
>      }
>  }
>  
> -static void virtio_scsi_hot_unplug(SCSIBus *bus, SCSIDevice *dev)
> +static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                                  Error **errp)
>  {
> -    VirtIOSCSI *s = container_of(bus, VirtIOSCSI, bus);
> -    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> +    VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev);
>  
>      if ((vdev->guest_features >> VIRTIO_SCSI_F_HOTPLUG) & 1) {
> -        virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_TRANSPORT_RESET,
> +        virtio_scsi_push_event(VIRTIO_SCSI(hotplug_dev), SCSI_DEVICE(dev),
> +                               VIRTIO_SCSI_T_TRANSPORT_RESET,
>                                 VIRTIO_SCSI_EVT_RESET_REMOVED);
>      }
> +    qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
>  }
>  
>  static struct SCSIBusInfo virtio_scsi_scsi_info = {
> @@ -680,8 +683,6 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = {
>      .complete = virtio_scsi_command_complete,
>      .cancel = virtio_scsi_request_cancelled,
>      .change = virtio_scsi_change,
> -    .hotplug = virtio_scsi_hotplug,
> -    .hot_unplug = virtio_scsi_hot_unplug,
>      .parse_cdb = virtio_scsi_parse_cdb,
>      .get_sg_list = virtio_scsi_get_sg_list,
>      .save_request = virtio_scsi_save_request,
> @@ -785,6 +786,7 @@ static void virtio_scsi_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>  
>      dc->props = virtio_scsi_properties;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> @@ -793,6 +795,8 @@ static void virtio_scsi_class_init(ObjectClass *klass, void *data)
>      vdc->set_config = virtio_scsi_set_config;
>      vdc->get_features = virtio_scsi_get_features;
>      vdc->reset = virtio_scsi_reset;
> +    hc->plug = virtio_scsi_hotplug;
> +    hc->unplug = virtio_scsi_hotunplug;
>  }
>  
>  static const TypeInfo virtio_scsi_common_info = {
> @@ -808,6 +812,10 @@ static const TypeInfo virtio_scsi_info = {
>      .parent = TYPE_VIRTIO_SCSI_COMMON,
>      .instance_size = sizeof(VirtIOSCSI),
>      .class_init = virtio_scsi_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HOTPLUG_HANDLER },
> +        { }
> +    }
>  };
>  
>  static void virtio_register_types(void)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 10/30] qdev: HotplugHandler: rename unplug callback to unplug_request

[Paolo Bonzini]

Il 24/09/2014 13:47, Igor Mammedov ha scritto:
> 'HotplugHandler.unplug' callback is currently used as async
> call to issue unplug request for device that implements it.
> Renaming 'unplug' callback to 'unplug_request' should help to
> avoid confusion about what callback does and would allow to
> introduce 'unplug' callback that would perform actual device
> removal when guest is ready for it.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> Patch is prompted by reviewing https://patchwork.ozlabs.org/patch/383372/
> 
> Dedicated 'unplug' callback could be used by bus-less pc-dimm
> device. It would allow to call HotplugHandler.unplug callback
> from ACPI code when guest calls _EJ0 method and execute board
> specific code (PCMachine) to unmap pc-dimm from guest's address
> space and perform necessary cleanup. The same applies to CPU
> unplug.
> ---
>  hw/acpi/piix4.c                |  6 +++---
>  hw/core/hotplug.c              | 10 +++++-----
>  hw/core/qdev.c                 |  3 ++-
>  hw/isa/lpc_ich9.c              |  6 +++---
>  hw/pci-bridge/pci_bridge_dev.c |  2 +-
>  hw/pci/pcie.c                  |  4 ++--
>  hw/pci/pcie_port.c             |  2 +-
>  hw/pci/shpc.c                  |  4 ++--
>  include/hw/hotplug.h           | 16 +++++++++-------
>  include/hw/pci/pcie.h          |  4 ++--
>  include/hw/pci/shpc.h          |  4 ++--
>  11 files changed, 32 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index b72b34e..0bfa814 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -354,8 +354,8 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
>      }
>  }
>  
> -static void piix4_device_unplug_cb(HotplugHandler *hotplug_dev,
> -                                   DeviceState *dev, Error **errp)
> +static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> +                                           DeviceState *dev, Error **errp)
>  {
>      PIIX4PMState *s = PIIX4_PM(hotplug_dev);
>  
> @@ -615,7 +615,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
>      dc->cannot_instantiate_with_device_add_yet = true;
>      dc->hotpluggable = false;
>      hc->plug = piix4_device_plug_cb;
> -    hc->unplug = piix4_device_unplug_cb;
> +    hc->unplug_request = piix4_device_unplug_request_cb;
>      adevc->ospm_status = piix4_ospm_status;
>  }
>  
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> index 5573d9d..2ec4736 100644
> --- a/hw/core/hotplug.c
> +++ b/hw/core/hotplug.c
> @@ -23,14 +23,14 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
>      }
>  }
>  
> -void hotplug_handler_unplug(HotplugHandler *plug_handler,
> -                            DeviceState *plugged_dev,
> -                            Error **errp)
> +void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
> +                                    DeviceState *plugged_dev,
> +                                    Error **errp)
>  {
>      HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
>  
> -    if (hdc->unplug) {
> -        hdc->unplug(plug_handler, plugged_dev, errp);
> +    if (hdc->unplug_request) {
> +        hdc->unplug_request(plug_handler, plugged_dev, errp);
>      }
>  }
>  
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 5e5b963..c98e5db 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -227,7 +227,8 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>      qdev_hot_removed = true;
>  
>      if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> -        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
> +        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler,
> +                                       dev, errp);
>      } else {
>          assert(dc->unplug != NULL);
>          if (dc->unplug(dev) < 0) { /* legacy handler */
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 177023b..530b074 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -607,8 +607,8 @@ static void ich9_device_plug_cb(HotplugHandler *hotplug_dev,
>      ich9_pm_device_plug_cb(&lpc->pm, dev, errp);
>  }
>  
> -static void ich9_device_unplug_cb(HotplugHandler *hotplug_dev,
> -                                  DeviceState *dev, Error **errp)
> +static void ich9_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> +                                          DeviceState *dev, Error **errp)
>  {
>      error_setg(errp, "acpi: device unplug request for not supported device"
>                 " type: %s", object_get_typename(OBJECT(dev)));
> @@ -676,7 +676,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
>       */
>      dc->cannot_instantiate_with_device_add_yet = true;
>      hc->plug = ich9_device_plug_cb;
> -    hc->unplug = ich9_device_unplug_cb;
> +    hc->unplug_request = ich9_device_unplug_request_cb;
>      adevc->ospm_status = ich9_pm_ospm_status;
>  }
>  
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 92799d0..252ea5e 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -150,7 +150,7 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>      dc->vmsd = &pci_bridge_dev_vmstate;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      hc->plug = shpc_device_hotplug_cb;
> -    hc->unplug = shpc_device_hot_unplug_cb;
> +    hc->unplug_request = shpc_device_hot_unplug_request_cb;
>  }
>  
>  static const TypeInfo pci_bridge_dev_info = {
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 1babddf..b64a004 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -262,8 +262,8 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>                          PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
>  }
>  
> -void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> -                                 Error **errp)
> +void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
> +                                         DeviceState *dev, Error **errp)
>  {
>      uint8_t *exp_cap;
>  
> diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c
> index fa24877..40ca8d5 100644
> --- a/hw/pci/pcie_port.c
> +++ b/hw/pci/pcie_port.c
> @@ -154,7 +154,7 @@ static void pcie_slot_class_init(ObjectClass *oc, void *data)
>  
>      dc->props = pcie_slot_props;
>      hc->plug = pcie_cap_slot_hotplug_cb;
> -    hc->unplug = pcie_cap_slot_hot_unplug_cb;
> +    hc->unplug_request = pcie_cap_slot_hot_unplug_request_cb;
>  }
>  
>  static const TypeInfo pcie_slot_type_info = {
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index 1fcb8c4..65b2f51 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -549,8 +549,8 @@ void shpc_device_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>      shpc_interrupt_update(pci_hotplug_dev);
>  }
>  
> -void shpc_device_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> -                               Error **errp)
> +void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
> +                                       DeviceState *dev, Error **errp)
>  {
>      Error *local_err = NULL;
>      PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> index a6533cb..e397d08 100644
> --- a/include/hw/hotplug.h
> +++ b/include/hw/hotplug.h
> @@ -47,7 +47,9 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
>   *
>   * @parent: Opaque parent interface.
>   * @plug: plug callback.
> - * @unplug: unplug callback.
> + * @unplug_request: unplug request callback.
> + *                  Used as a means to initiate device unplug for devices that
> + *                  require asynchronous unplug handling.
>   */
>  typedef struct HotplugHandlerClass {
>      /* <private> */
> @@ -55,7 +57,7 @@ typedef struct HotplugHandlerClass {
>  
>      /* <public> */
>      hotplug_fn plug;
> -    hotplug_fn unplug;
> +    hotplug_fn unplug_request;
>  } HotplugHandlerClass;
>  
>  /**
> @@ -68,11 +70,11 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
>                            Error **errp);
>  
>  /**
> - * hotplug_handler_unplug:
> + * hotplug_handler_unplug_request:
>   *
> - * Call #HotplugHandlerClass.unplug callback of @plug_handler.
> + * Calls #HotplugHandlerClass.unplug_request callback of @plug_handler.
>   */
> -void hotplug_handler_unplug(HotplugHandler *plug_handler,
> -                            DeviceState *plugged_dev,
> -                            Error **errp);
> +void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
> +                                    DeviceState *plugged_dev,
> +                                    Error **errp);
>  #endif
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index d139d58..b48a7a2 100644
> --- a/include/hw/pci/pcie.h
> +++ b/include/hw/pci/pcie.h
> @@ -128,6 +128,6 @@ extern const VMStateDescription vmstate_pcie_device;
>  
>  void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                Error **errp);
> -void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> -                                 Error **errp);
> +void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
> +                                         DeviceState *dev, Error **errp);
>  #endif /* QEMU_PCIE_H */
> diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
> index eef1a1a..025bc5b 100644
> --- a/include/hw/pci/shpc.h
> +++ b/include/hw/pci/shpc.h
> @@ -46,8 +46,8 @@ void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
>  
>  void shpc_device_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>                              Error **errp);
> -void shpc_device_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> -                               Error **errp);
> +void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
> +                                       DeviceState *dev, Error **errp);
>  
>  extern VMStateInfo shpc_vmstate_info;
>  #define SHPC_VMSTATE(_field, _type) \
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 11/30] qdev: HotplugHandler: provide unplug callback

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> it to be called for actual device removal and
> will allow to separate request and removal handling
> phases of x86-CPU devices and also it's a handler
> to be called for synchronously removable devices.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> unplug handling for bus-less devices will be added
> later in this series.
> ---
>  hw/core/hotplug.c    | 11 +++++++++++
>  hw/core/qdev.c       | 13 +++++++++++--
>  include/hw/hotplug.h | 12 ++++++++++++
>  3 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> index 2ec4736..4e01074 100644
> --- a/hw/core/hotplug.c
> +++ b/hw/core/hotplug.c
> @@ -34,6 +34,17 @@ void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
>      }
>  }
>  
> +void hotplug_handler_unplug(HotplugHandler *plug_handler,
> +                            DeviceState *plugged_dev,
> +                            Error **errp)
> +{
> +    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
> +
> +    if (hdc->unplug) {
> +        hdc->unplug(plug_handler, plugged_dev, errp);
> +    }
> +}
> +
>  static const TypeInfo hotplug_handler_info = {
>      .name          = TYPE_HOTPLUG_HANDLER,
>      .parent        = TYPE_INTERFACE,
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index c98e5db..c89d781 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -227,8 +227,17 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>      qdev_hot_removed = true;
>  
>      if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> -        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler,
> -                                       dev, errp);
> +        HotplugHandlerClass *hdc;
> +
> +        /* If device supports async unplug just request it to be done,
> +         * otherwise just remove it synchronously */
> +        hdc = HOTPLUG_HANDLER_GET_CLASS(dev->parent_bus->hotplug_handler);
> +        if (hdc->unplug_request) {
> +            hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler,
> +                                           dev, errp);
> +        } else {
> +            hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
> +        }
>      } else {
>          assert(dc->unplug != NULL);
>          if (dc->unplug(dev) < 0) { /* legacy handler */
> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> index e397d08..451d522 100644
> --- a/include/hw/hotplug.h
> +++ b/include/hw/hotplug.h
> @@ -50,6 +50,9 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
>   * @unplug_request: unplug request callback.
>   *                  Used as a means to initiate device unplug for devices that
>   *                  require asynchronous unplug handling.
> + * @unplug_request: unplug callback.
> + *                  Used for device removal with devices that implement
> + *                  asynchronous and synchronous (suprise) removal.
>   */
>  typedef struct HotplugHandlerClass {
>      /* <private> */
> @@ -58,6 +61,7 @@ typedef struct HotplugHandlerClass {
>      /* <public> */
>      hotplug_fn plug;
>      hotplug_fn unplug_request;
> +    hotplug_fn unplug;
>  } HotplugHandlerClass;
>  
>  /**
> @@ -77,4 +81,12 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
>  void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
>                                      DeviceState *plugged_dev,
>                                      Error **errp);
> +/**
> + * hotplug_handler_unplug:
> + *
> + * Calls #HotplugHandlerClass.unplug callback of @plug_handler.
> + */
> +void hotplug_handler_unplug(HotplugHandler *plug_handler,
> +                            DeviceState *plugged_dev,
> +                            Error **errp);
>  #endif
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 12/30] qdev: add simple/generic unplug callback for HotplugHandler

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> it will be used in shallow conversion from legacy hotplug
> mechanism and eventually replace all the uses of old mechanism
> DeviceClass->unplug = qdev_simple_unplug_cb()
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/core/qdev.c         | 5 +++++
>  include/hw/qdev-core.h | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index c89d781..1d1b113 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -286,6 +286,11 @@ int qdev_simple_unplug_cb(DeviceState *dev)
>      return 0;
>  }
>  
> +void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
> +                                  DeviceState *dev, Error **errp)
> +{
> +    qdev_simple_unplug_cb(dev);
> +}
>  
>  /* Like qdev_init(), but terminate program via error_report() instead of
>     returning an error value.  This is okay during machine creation.
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 48a96d2..ba812c5 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -265,6 +265,8 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>                                   int required_for_version);
>  void qdev_unplug(DeviceState *dev, Error **errp);
>  int qdev_simple_unplug_cb(DeviceState *dev);
> +void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
> +                                  DeviceState *dev, Error **errp);
>  void qdev_machine_creation_done(void);
>  bool qdev_machine_modified(void);
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 13/30] qdev: hotplug: set handler only if HOTPLUG_HANDLER interface is supported

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> Check if 'handler' implements HOTPLUG_HANDLER interface
> before setting it, if it's not then do nothing and leave
> bus not hotpluggable.
> 
> That would allow to reuse the same code for creating bus
> for example 'scsi_bus_new()' for both hotpluggable and not
> hotpluggable controllers.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> PS:
> Keep allow_hotplug = 1 before check to preserve legacy
> behaviour for incremental conversion without breaking
> bisect-ability. I will be removed later in series.
> ---
>  include/hw/qdev-core.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index ba812c5..653ff4a 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -366,9 +366,14 @@ char *qdev_get_dev_path(DeviceState *dev);
>  static inline void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler,
>                                              Error **errp)
>  {
> +    bus->allow_hotplug = 1;
> +
> +    if (!object_dynamic_cast(OBJECT(handler), TYPE_HOTPLUG_HANDLER)) {
> +        return;
> +    }
> +
>      object_property_set_link(OBJECT(bus), OBJECT(handler),
>                               QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
> -    bus->allow_hotplug = 1;
>  }
>  
>  static inline bool qbus_is_hotpluggable(BusState *bus)
> 

Ok, so scsi won't error_abort, but you are actually breaking
hotplug/unplug on HBAs other than virtio-scsi and pvscsi.  On those HBAs
you can actually do surprise removal and hotplug, but you have to
manually rescan the bus in the virtual machine.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 14/30] target-i386: ICC bus: replace BusState.allow_hotplug with hotplug_handler

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> it will allow to drop BusState.allow_hotplug field
> and still keep ICC bus hotluggable for CPU/APIC devices.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/cpu/icc_bus.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
> index 7f44c59..cb5ed5c 100644
> --- a/hw/cpu/icc_bus.c
> +++ b/hw/cpu/icc_bus.c
> @@ -24,18 +24,10 @@
>  
>  /* icc-bridge implementation */
>  
> -static void icc_bus_init(Object *obj)
> -{
> -    BusState *b = BUS(obj);
> -
> -    b->allow_hotplug = true;
> -}
> -
>  static const TypeInfo icc_bus_info = {
>      .name = TYPE_ICC_BUS,
>      .parent = TYPE_BUS,
>      .instance_size = sizeof(ICCBus),
> -    .instance_init = icc_bus_init,
>  };
>  
>  
> @@ -100,11 +92,19 @@ static void icc_bridge_init(Object *obj)
>      s->icc_bus.apic_address_space = &s->apic_container;
>  }
>  
> +static void icc_bridge_realize(DeviceState *dev, Error **errp)
> +{
> +    ICCBridgeState *s = ICC_BRIGDE(dev);
> +
> +    qbus_set_hotplug_handler(BUS(&s->icc_bus), dev, errp);
> +}
> +
>  static void icc_bridge_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
>  
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +    dc->realize = icc_bridge_realize;
>  }
>  
>  static const TypeInfo icc_bridge_info = {
> @@ -113,6 +113,10 @@ static const TypeInfo icc_bridge_info = {
>      .instance_init  = icc_bridge_init,
>      .instance_size  = sizeof(ICCBridgeState),
>      .class_init = icc_bridge_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HOTPLUG_HANDLER },
> +        { }
> +    }
>  };
>  
>  
> 

If you do this, aren't you enabling CPU hot-unplug?  Should
hotplug_handler_plug and/or hotplug_handler_unplug return an error if
there is no callback?

Paolo

Re: [Qemu-devel] [PATCH 15/30] virtio-pci: replace BusState.allow_hotplug with hotplug_handler

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> Although virtio-pci-bus is internal object of composite
> virtio-pci device and it doesn't participate in
> -device/device_add hotplug flow, it's required by
> bus_add_child() that bus must be hotpluggable to
> be able to add child at runtime.
> Set parent of virtio-pci-bus as NOP hotplug controller,
> so that bus_add_child() would allow to add child
> device during hotplug time when BusState.allow_hotplug
> is dropped.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/virtio/virtio-pci.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f560814..0486b25 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1070,6 +1070,10 @@ static const TypeInfo virtio_pci_info = {
>      .class_init    = virtio_pci_class_init,
>      .class_size    = sizeof(VirtioPCIClass),
>      .abstract      = true,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HOTPLUG_HANDLER },
> +        { }
> +    }
>  };
>  
>  /* virtio-blk-pci */
> @@ -1543,13 +1547,11 @@ static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
>                                 VirtIOPCIProxy *dev)
>  {
>      DeviceState *qdev = DEVICE(dev);
> -    BusState *qbus;
>      char virtio_bus_name[] = "virtio-bus";
>  
>      qbus_create_inplace(bus, bus_size, TYPE_VIRTIO_PCI_BUS, qdev,
>                          virtio_bus_name);
> -    qbus = BUS(bus);
> -    qbus->allow_hotplug = 1;
> +    qbus_set_hotplug_handler(BUS(bus), qdev, &error_abort);
>  }
>  
>  static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
> 

So this is the opposite of icc-bus; here you need to call the simple
hot-unplug callback in the hotplug handler, right?

Paolo

Re: [Qemu-devel] [PATCH 16/30] virtio-serial: convert to hotplug-handler API

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/char/virtio-serial-bus.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 3931085..c6870f1 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -904,6 +904,12 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
>      }
>  
>      port->elem.out_num = 0;
> +}
> +
> +static void virtser_port_device_plug(HotplugHandler *hotplug_dev,
> +                                     DeviceState *dev, Error **errp)
> +{
> +    VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(dev);
>  
>      QTAILQ_INSERT_TAIL(&port->vser->ports, port, next);
>      port->ivq = port->vser->ivqs[port->id];
> @@ -912,7 +918,7 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
>      add_port(port->vser, port->id);
>  
>      /* Send an update to the guest about this new port added */
> -    virtio_notify_config(vdev);
> +    virtio_notify_config(VIRTIO_DEVICE(hotplug_dev));
>  }
>  
>  static void virtser_port_device_unrealize(DeviceState *dev, Error **errp)
> @@ -935,7 +941,6 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VirtIOSerial *vser = VIRTIO_SERIAL(dev);
> -    BusState *bus;
>      uint32_t i, max_supported_ports;
>  
>      if (!vser->serial.max_virtserial_ports) {
> @@ -957,8 +962,7 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
>      /* Spawn a new virtio-serial bus on which the ports will ride as devices */
>      qbus_create_inplace(&vser->bus, sizeof(vser->bus), TYPE_VIRTIO_SERIAL_BUS,
>                          dev, vdev->bus_name);
> -    bus = BUS(&vser->bus);
> -    bus->allow_hotplug = 1;
> +    qbus_set_hotplug_handler(BUS(&vser->bus), DEVICE(vser), errp);
>      vser->bus.vser = vser;
>      QTAILQ_INIT(&vser->ports);
>  
> @@ -1021,7 +1025,6 @@ static void virtio_serial_port_class_init(ObjectClass *klass, void *data)
>      k->bus_type = TYPE_VIRTIO_SERIAL_BUS;
>      k->realize = virtser_port_device_realize;
>      k->unrealize = virtser_port_device_unrealize;
> -    k->unplug = qdev_simple_unplug_cb;
>      k->props = virtser_props;
>  }
>  
> @@ -1064,6 +1067,7 @@ static void virtio_serial_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>  
>      QLIST_INIT(&vserdevices.devices);
>  
> @@ -1077,6 +1081,8 @@ static void virtio_serial_class_init(ObjectClass *klass, void *data)
>      vdc->reset = vser_reset;
>      vdc->save = virtio_serial_save_device;
>      vdc->load = virtio_serial_load_device;
> +    hc->plug = virtser_port_device_plug;
> +    hc->unplug = qdev_simple_device_unplug_cb;
>  }
>  
>  static const TypeInfo virtio_device_info = {
> @@ -1084,6 +1090,10 @@ static const TypeInfo virtio_device_info = {
>      .parent = TYPE_VIRTIO_DEVICE,
>      .instance_size = sizeof(VirtIOSerial),
>      .class_init = virtio_serial_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HOTPLUG_HANDLER },
> +        { }
> +    }
>  };
>  
>  static void virtio_serial_register_types(void)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 17/30] virtio-mmio: drop useless bus->allow_hotplug = 0

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> Bus by default is not hotpluggable.
> virtio-mmio-bus and its parent types do not set allow_hotplug
> anywhere explicitly, so remove not needed field access
> and wrapper along with it.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/virtio/virtio-mmio.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> index 18c6e5b..2450c13 100644
> --- a/hw/virtio/virtio-mmio.c
> +++ b/hw/virtio/virtio-mmio.c
> @@ -89,9 +89,6 @@ typedef struct {
>      VirtioBusState bus;
>  } VirtIOMMIOProxy;
>  
> -static void virtio_mmio_bus_new(VirtioBusState *bus, size_t bus_size,
> -                                VirtIOMMIOProxy *dev);
> -
>  static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
> @@ -362,7 +359,8 @@ static void virtio_mmio_realizefn(DeviceState *d, Error **errp)
>      VirtIOMMIOProxy *proxy = VIRTIO_MMIO(d);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(d);
>  
> -    virtio_mmio_bus_new(&proxy->bus, sizeof(proxy->bus), proxy);
> +    qbus_create_inplace(&proxy->bus, sizeof(proxy->bus), TYPE_VIRTIO_MMIO_BUS,
> +                        d, NULL);
>      sysbus_init_irq(sbd, &proxy->irq);
>      memory_region_init_io(&proxy->iomem, OBJECT(d), &virtio_mem_ops, proxy,
>                            TYPE_VIRTIO_MMIO, 0x200);
> @@ -393,17 +391,6 @@ static const TypeInfo virtio_mmio_info = {
>  
>  /* virtio-mmio-bus. */
>  
> -static void virtio_mmio_bus_new(VirtioBusState *bus, size_t bus_size,
> -                                VirtIOMMIOProxy *dev)
> -{
> -    DeviceState *qdev = DEVICE(dev);
> -    BusState *qbus;
> -
> -    qbus_create_inplace(bus, bus_size, TYPE_VIRTIO_MMIO_BUS, qdev, NULL);
> -    qbus = BUS(bus);
> -    qbus->allow_hotplug = 0;
> -}
> -
>  static void virtio_mmio_bus_class_init(ObjectClass *klass, void *data)
>  {
>      BusClass *bus_class = BUS_CLASS(klass);
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 18/30] s390x: drop not used allow_hotplug in event-facility

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> s390-sclp-event-facility creates s390-sclp-events-bus
> and immeadiatly sets its allow_hotplug field to 0,
> which is NOP since it's already 0 by default.
> 
> Also since BUS is not hotpluggable, it's not possible
> to call SCLP_EVENT{ DeviceClass.unplug } callback
> from qdev_unplug() making this unreachable code,
> so drop it as well.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/s390x/event-facility.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index 597db34..78da718 100644
> --- a/hw/s390x/event-facility.c
> +++ b/hw/s390x/event-facility.c
> @@ -333,7 +333,6 @@ static int init_event_facility(SCLPEventFacility *event_facility)
>      /* Spawn a new bus for SCLP events */
>      qbus_create_inplace(&event_facility->sbus, sizeof(event_facility->sbus),
>                          TYPE_SCLP_EVENTS_BUS, sdev, NULL);
> -    event_facility->sbus.qbus.allow_hotplug = 0;
>  
>      quiesce = qdev_create(&event_facility->sbus.qbus, "sclpquiesce");
>      if (!quiesce) {
> @@ -408,7 +407,6 @@ static void event_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->bus_type = TYPE_SCLP_EVENTS_BUS;
> -    dc->unplug = qdev_simple_unplug_cb;
>      dc->realize = event_realize;
>      dc->unrealize = event_unrealize;
>  }
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 25/30] usb-bot: drop not needed "allow_hotplug = 0"

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> usb-bot creates SCSI bus and immediately makes it
> non hotpluggable which makes not possible to add
> anything to it after machine is created.
> 
> Taking in account that ubs-bot doesn't implement
> HOTPLUG_HANDLER interface, SCSI will be created as
> non hotpluggable, so there is not need to reset
> allow_hotplug field anymore.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/usb/dev-storage.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index bd7cc53..bfa71c7 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -653,7 +653,6 @@ static void usb_msd_realize_bot(USBDevice *dev, Error **errp)
>      usb_desc_init(dev);
>      scsi_bus_new(&s->bus, sizeof(s->bus), DEVICE(dev),
>                   &usb_msd_scsi_info_bot, NULL);
> -    s->bus.qbus.allow_hotplug = 0;
>      usb_msd_handle_reset(dev);
>  }
>  
> 

I guess the fate of this patch depends on the answer to the previous
questions.

The usb-bot hotplug handler should return an error on both plug and
unplug.  You will decide whether to do it explicitly, or in
hw/core/hotplug.c with NULL callbacks in usb-bot.

Paolo

Re: [Qemu-devel] [PATCH 06/30] test: usb: add port test to uhci unit test

[Gerd Hoffmann]

On Mi, 2014-09-24 at 11:47 +0000, Igor Mammedov wrote:
> along with it move common code fro ehci test to
> a separate file to avoid duplication.

Having the code mode as separate patch makes it easier to see the actual
port test added ...

> +static void test_port_1(void)
> +{
> +    QPCIBus *pcibus;
> +    struct qhc uhci;
> +
> +    pcibus = qpci_init_pc();
> +    g_assert(pcibus != NULL);
> +    usb_pci_init_one(pcibus, &uhci, QPCI_DEVFN(0x1d, 0), 4);
> +    uhci_port_test(&uhci, 0, UHCI_PORT_CCS);
>  }

... here.  Why test only one of the two ports btw?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 26/30] usb-storage: make its storage SCSI bus hotpluggable explicitly

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> usb-storage uses SCSI bus to provide underling storage
> (i.e. scsi-disk) and it's hotpluggable. In addition
> during usb-storage hotplug bus_add_child() won't allow
> to add scsi-disk to SCSI bus owned by usb-storage
> if bus is not hotpluggable.
> 
> By default SCSI bus was alway hotpluggable even if
> an owner didn't support hotplug. It will change
> once BusState.allow_hotplug field is dropped and
> SCSI bus will be only hotpluggable only if bus owner
> implements HOTPLUG_HANDLER interface.
> 
> So make usb-storage implement HOTPLUG_HANDLER interface
> to keep its SCSI bus hotpluggable.

Correct.  usb-bot unfortunately is not hot-pluggable because we would
plug it with no SCSI LUN inside.  The guest would not be able to access
it.  We would have to delay the presentation of the usb-bot device until
after the LUN is there.  Perhaps some kind of transaction mechanism?

However, usb-storage builds the outer USB device and the inner SCSI
device at the same time, so it can be hot-plugged.

What you do for usb-storage should also be done for lsi, esp, usb-uas, etc.

Paolo

Re: [Qemu-devel] [PATCH 27/30] usb-storage: drop not needed "allow_hotplug = 0"

[Paolo Bonzini]

Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> Drop useless hack that disables hotplug on bus, after backend
> storage was added to it, by setting "allow_hotplug = 0". Even
> if bus is hotpluggable, It won't be possible to add another
> SCSI device to bus since its realize will fail early with
> error "no free target" in scsi_qdev_realize() method.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/usb/dev-storage.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index 4ad4ec7..b168d7c 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -632,7 +632,6 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
>          error_propagate(errp, err);
>          return;
>      }
> -    s->bus.qbus.allow_hotplug = 0;
>      usb_msd_handle_reset(dev);
>  
>      if (bdrv_key_required(bs)) {
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 09/30] access BusState.allow_hotplug using wraper qbus_is_hotpluggable()

[Paolo Bonzini]

Il 24/09/2014 13:47, Igor Mammedov ha scritto:
> it would allow transparently switch detection if Bus
> is hotpluggable from allow_hotplug field to hotplug_handler
> link and drop allow_hotplug field once all users are
> converted to hotplug handler API.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/core/qdev.c           | 6 +++---
>  hw/i386/acpi-build.c     | 2 +-
>  hw/pci/pci-hotplug-old.c | 4 ++--
>  include/hw/qdev-core.h   | 5 +++++
>  qdev-monitor.c           | 2 +-
>  5 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index fcb1638..5e5b963 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -86,7 +86,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>      BusChild *kid = g_malloc0(sizeof(*kid));
>  
>      if (qdev_hotplug) {
> -        assert(bus->allow_hotplug);
> +        assert(qbus_is_hotpluggable(bus));
>      }
>  
>      kid->index = bus->max_index++;
> @@ -213,7 +213,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>  {
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>  
> -    if (dev->parent_bus && !dev->parent_bus->allow_hotplug) {
> +    if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
>          error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
>          return;
>      }
> @@ -925,7 +925,7 @@ static bool device_get_hotpluggable(Object *obj, Error **errp)
>      DeviceState *dev = DEVICE(obj);
>  
>      return dc->hotpluggable && (dev->parent_bus == NULL ||
> -                                dev->parent_bus->allow_hotplug);
> +                                qbus_is_hotpluggable(dev->parent_bus));
>  }
>  
>  static bool device_get_hotplugged(Object *obj, Error **err)
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index a313321..00be4bb 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -774,7 +774,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>      unsigned *bsel_alloc = opaque;
>      unsigned *bus_bsel;
>  
> -    if (bus->qbus.allow_hotplug) {
> +    if (qbus_is_hotpluggable(BUS(bus))) {
>          bus_bsel = g_malloc(sizeof *bus_bsel);
>  
>          *bus_bsel = (*bsel_alloc)++;
> diff --git a/hw/pci/pci-hotplug-old.c b/hw/pci/pci-hotplug-old.c
> index cf2caeb..f9b7398 100644
> --- a/hw/pci/pci-hotplug-old.c
> +++ b/hw/pci/pci-hotplug-old.c
> @@ -77,7 +77,7 @@ static PCIDevice *qemu_pci_hot_add_nic(Monitor *mon,
>          monitor_printf(mon, "Invalid PCI device address %s\n", devaddr);
>          return NULL;
>      }
> -    if (!((BusState*)bus)->allow_hotplug) {
> +    if (!qbus_is_hotpluggable(BUS(bus))) {
>          monitor_printf(mon, "PCI bus doesn't support hotplug\n");
>          return NULL;
>      }
> @@ -224,7 +224,7 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
>          monitor_printf(mon, "Invalid PCI device address %s\n", devaddr);
>          return NULL;
>      }
> -    if (!((BusState*)bus)->allow_hotplug) {
> +    if (!qbus_is_hotpluggable(BUS(bus))) {
>          monitor_printf(mon, "PCI bus doesn't support hotplug\n");
>          return NULL;
>      }
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 178fee2..48a96d2 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -368,4 +368,9 @@ static inline void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler,
>                               QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
>      bus->allow_hotplug = 1;
>  }
> +
> +static inline bool qbus_is_hotpluggable(BusState *bus)
> +{
> +   return bus->allow_hotplug || bus->hotplug_handler;
> +}
>  #endif
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 5ec6606..f6db461 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -515,7 +515,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>              return NULL;
>          }
>      }
> -    if (qdev_hotplug && bus && !bus->allow_hotplug) {
> +    if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
>          qerror_report(QERR_BUS_NO_HOTPLUG, bus->name);
>          return NULL;
>      }
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 01/30] test: virtio-scsi: check if hot-plug/unplug works

[Paolo Bonzini]

Il 24/09/2014 13:47, Igor Mammedov ha scritto:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/virtio-scsi-test.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
> index 3230908..41f9602 100644
> --- a/tests/virtio-scsi-test.c
> +++ b/tests/virtio-scsi-test.c
> @@ -17,14 +17,43 @@ static void pci_nop(void)
>  {
>  }
>  
> +static void hotplug(void)
> +{
> +    QDict *response;
> +
> +    response = qmp("{\"execute\": \"device_add\","
> +                   " \"arguments\": {"
> +                   "   \"driver\": \"scsi-hd\","
> +                   "   \"id\": \"scsi-hd\","
> +                   "   \"drive\": \"drv1\""
> +                   "}}");
> +
> +    g_assert(response);
> +    g_assert(!qdict_haskey(response, "error"));
> +    QDECREF(response);
> +
> +    response = qmp("{\"execute\": \"device_del\","
> +                   " \"arguments\": {"
> +                   "   \"id\": \"scsi-hd\""
> +                   "}}");
> +
> +    g_assert(response);
> +    g_assert(!qdict_haskey(response, "error"));
> +    g_assert(qdict_haskey(response, "event"));
> +    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
> +    QDECREF(response);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      int ret;
>  
>      g_test_init(&argc, &argv, NULL);
>      qtest_add_func("/virtio/scsi/pci/nop", pci_nop);
> +    qtest_add_func("/virtio/scsi/pci/hotplug", hotplug);
>  
>      qtest_start("-drive id=drv0,if=none,file=/dev/null "
> +                "-drive id=drv1,if=none,file=/dev/null "
>                  "-device virtio-scsi-pci,id=vscsi0 "
>                  "-device scsi-hd,bus=vscsi0.0,drive=drv0");
>      ret = g_test_run();
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 02/30] test: virtio-serial: check if hot-plug/unplug works

[Paolo Bonzini]

Il 24/09/2014 13:47, Igor Mammedov ha scritto:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/virtio-serial-test.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/tests/virtio-serial-test.c b/tests/virtio-serial-test.c
> index e743875..bf030a6 100644
> --- a/tests/virtio-serial-test.c
> +++ b/tests/virtio-serial-test.c
> @@ -17,12 +17,39 @@ static void pci_nop(void)
>  {
>  }
>  
> +static void hotplug(void)
> +{
> +    QDict *response;
> +
> +    response = qmp("{\"execute\": \"device_add\","
> +                   " \"arguments\": {"
> +                   "   \"driver\": \"virtserialport\","
> +                   "   \"id\": \"hp-port\""
> +                   "}}");
> +
> +    g_assert(response);
> +    g_assert(!qdict_haskey(response, "error"));
> +    QDECREF(response);
> +
> +    response = qmp("{\"execute\": \"device_del\","
> +                   " \"arguments\": {"
> +                   "   \"id\": \"hp-port\""
> +                   "}}");
> +
> +    g_assert(response);
> +    g_assert(!qdict_haskey(response, "error"));
> +    g_assert(qdict_haskey(response, "event"));
> +    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
> +    QDECREF(response);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      int ret;
>  
>      g_test_init(&argc, &argv, NULL);
>      qtest_add_func("/virtio/serial/pci/nop", pci_nop);
> +    qtest_add_func("/virtio/serial/pci/hotplug", hotplug);
>  
>      qtest_start("-device virtio-serial-pci");
>      ret = g_test_run();
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 03/30] test: virtio-rng: check if hot-plug/unplug works

[Paolo Bonzini]

Il 24/09/2014 13:47, Igor Mammedov ha scritto:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/virtio-rng-test.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/tests/virtio-rng-test.c b/tests/virtio-rng-test.c
> index 402c206..7f62f84 100644
> --- a/tests/virtio-rng-test.c
> +++ b/tests/virtio-rng-test.c
> @@ -12,17 +12,55 @@
>  #include "libqtest.h"
>  #include "qemu/osdep.h"
>  
> +#define PCI_SLOT_HP             0x06
> +#define ACPI_PCIHP_ADDR         0xae00
> +#define PCI_EJ_BASE             0x0008
> +
>  /* Tests only initialization so far. TODO: Replace with functional tests */
>  static void pci_nop(void)
>  {
>  }
>  
> +static void hotplug(void)
> +{
> +    QDict *response;
> +
> +    response = qmp("{\"execute\": \"device_add\","

Remember that, for all these tests, you can use '' strings instead of "".

> +                   " \"arguments\": {"
> +                   "   \"driver\": \"virtio-rng-pci\","
> +                   "   \"addr\": \"" stringify(PCI_SLOT_HP) "\","
> +                   "   \"id\": \"rng1\""
> +                   "}}");
> +
> +    g_assert(response);
> +    g_assert(!qdict_haskey(response, "error"));
> +    QDECREF(response);
> +
> +    response = qmp("{\"execute\": \"device_del\","
> +                   " \"arguments\": {"
> +                   "   \"id\": \"rng1\""
> +                   "}}");
> +
> +    g_assert(response);
> +    g_assert(!qdict_haskey(response, "error"));
> +    QDECREF(response);
> +
> +    outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << PCI_SLOT_HP);

Please replace this with a callback in tests/libqos/pci.h (with the
implementation in tests/libqos/pci-pc.c).

Paolo

> +    response = qmp("");
> +    g_assert(response);
> +    g_assert(qdict_haskey(response, "event"));
> +    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
> +    QDECREF(response);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      int ret;
>  
>      g_test_init(&argc, &argv, NULL);
>      qtest_add_func("/virtio/rng/pci/nop", pci_nop);
> +    qtest_add_func("/virtio/rng/pci/hotplug", hotplug);
>  
>      qtest_start("-device virtio-rng-pci");
>      ret = g_test_run();
> 

Re: [Qemu-devel] [PATCH 04/30] test: virtio-net: check if hot-plug/unplug works

[Paolo Bonzini]

Il 24/09/2014 13:47, Igor Mammedov ha scritto:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/virtio-net-test.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
> index df99343..b80a72f 100644
> --- a/tests/virtio-net-test.c
> +++ b/tests/virtio-net-test.c
> @@ -12,17 +12,56 @@
>  #include "libqtest.h"
>  #include "qemu/osdep.h"
>  
> +#define PCI_SLOT_HP             0x06
> +#define ACPI_PCIHP_ADDR         0xae00
> +#define PCI_EJ_BASE             0x0008
> +
>  /* Tests only initialization so far. TODO: Replace with functional tests */
>  static void pci_nop(void)
>  {
>  }
>  
> +static void hotplug(void)
> +{
> +    QDict *response;
> +
> +    response = qmp("{\"execute\": \"device_add\","
> +                   " \"arguments\": {"
> +                   "   \"driver\": \"virtio-net-pci\","
> +                   "   \"addr\": \"" stringify(PCI_SLOT_HP) "\","
> +                   "   \"id\": \"net1\""
> +                   "}}");
> +
> +    g_assert(response);
> +    g_assert(!qdict_haskey(response, "error"));
> +    QDECREF(response);
> +
> +    response = qmp("{\"execute\": \"device_del\","
> +                   " \"arguments\": {"
> +                   "   \"id\": \"net1\""
> +                   "}}");
> +
> +    g_assert(response);
> +    g_assert(!qdict_haskey(response, "error"));
> +    QDECREF(response);
> +
> +    outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << PCI_SLOT_HP);

Code duplication alarm! :)

Paolo

> +    response = qmp("");
> +    g_assert(response);
> +    g_assert(qdict_haskey(response, "event"));
> +    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
> +    QDECREF(response);
> +}
> +
> +
>  int main(int argc, char **argv)
>  {
>      int ret;
>  
>      g_test_init(&argc, &argv, NULL);
>      qtest_add_func("/virtio/net/pci/nop", pci_nop);
> +    qtest_add_func("/virtio/net/pci/hotplug", hotplug);
>  
>      qtest_start("-device virtio-net-pci");
>      ret = g_test_run();
> 

Re: [Qemu-devel] [PATCH 05/30] test: virtio-blk: check if hot-plug/unplug works

[Paolo Bonzini]

Il 24/09/2014 13:47, Igor Mammedov ha scritto:
> since virtio-blk-pci is a PCI device, its unplug is async
> and handled by ACPI. So simultate device's ACPI _EJ0
> method to trigger actual device removal.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/virtio-blk-test.c | 75 +++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 64 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> index 588666c..b42c10b 100644
> --- a/tests/virtio-blk-test.c
> +++ b/tests/virtio-blk-test.c
> @@ -45,6 +45,10 @@
>  #define PCI_SLOT                0x04
>  #define PCI_FN                  0x00
>  
> +#define PCI_SLOT_HP             0x06
> +#define ACPI_PCIHP_ADDR         0xae00
> +#define PCI_EJ_BASE             0x0008
> +
>  typedef struct QVirtioBlkReq {
>      uint32_t type;
>      uint32_t ioprio;
> @@ -55,7 +59,7 @@ typedef struct QVirtioBlkReq {
>  
>  static QPCIBus *test_start(void)
>  {
> -    char cmdline[100];
> +    char *cmdline;
>      char tmp_path[] = "/tmp/qtest.XXXXXX";
>      int fd, ret;
>  
> @@ -66,11 +70,14 @@ static QPCIBus *test_start(void)
>      g_assert_cmpint(ret, ==, 0);
>      close(fd);
>  
> -    snprintf(cmdline, 100, "-drive if=none,id=drive0,file=%s "
> -                            "-device virtio-blk-pci,drive=drive0,addr=%x.%x",
> -                            tmp_path, PCI_SLOT, PCI_FN);
> +    cmdline = g_strdup_printf("-drive if=none,id=drive0,file=%s "
> +                              "-drive if=none,id=drive1,file=/dev/null "
> +                              "-device virtio-blk-pci,id=drv0,drive=drive0,"
> +                              "addr=%x.%x",
> +                              tmp_path, PCI_SLOT, PCI_FN);
>      qtest_start(cmdline);
>      unlink(tmp_path);
> +    g_free(cmdline);
>  
>      return qpci_init_pc();
>  }
> @@ -80,14 +87,14 @@ static void test_end(void)
>      qtest_end();
>  }
>  
> -static QVirtioPCIDevice *virtio_blk_init(QPCIBus *bus)
> +static QVirtioPCIDevice *virtio_blk_init(QPCIBus *bus, int slot)
>  {
>      QVirtioPCIDevice *dev;
>  
>      dev = qvirtio_pci_device_find(bus, QVIRTIO_BLK_DEVICE_ID);
>      g_assert(dev != NULL);
>      g_assert_cmphex(dev->vdev.device_type, ==, QVIRTIO_BLK_DEVICE_ID);
> -    g_assert_cmphex(dev->pdev->devfn, ==, ((PCI_SLOT << 3) | PCI_FN));
> +    g_assert_cmphex(dev->pdev->devfn, ==, ((slot << 3) | PCI_FN));
>  
>      qvirtio_pci_device_enable(dev);
>      qvirtio_reset(&qvirtio_pci, &dev->vdev);
> @@ -147,7 +154,7 @@ static void pci_basic(void)
>  
>      bus = test_start();
>  
> -    dev = virtio_blk_init(bus);
> +    dev = virtio_blk_init(bus, PCI_SLOT);
>  
>      /* MSI-X is not enabled */
>      addr = dev->addr + QVIRTIO_DEVICE_SPECIFIC_NO_MSIX;
> @@ -293,7 +300,7 @@ static void pci_indirect(void)
>  
>      bus = test_start();
>  
> -    dev = virtio_blk_init(bus);
> +    dev = virtio_blk_init(bus, PCI_SLOT);
>  
>      /* MSI-X is not enabled */
>      addr = dev->addr + QVIRTIO_DEVICE_SPECIFIC_NO_MSIX;
> @@ -384,7 +391,7 @@ static void pci_config(void)
>  
>      bus = test_start();
>  
> -    dev = virtio_blk_init(bus);
> +    dev = virtio_blk_init(bus, PCI_SLOT);
>  
>      /* MSI-X is not enabled */
>      addr = dev->addr + QVIRTIO_DEVICE_SPECIFIC_NO_MSIX;
> @@ -426,7 +433,7 @@ static void pci_msix(void)
>      bus = test_start();
>      alloc = pc_alloc_init();
>  
> -    dev = virtio_blk_init(bus);
> +    dev = virtio_blk_init(bus, PCI_SLOT);
>      qpci_msix_enable(dev->pdev);
>  
>      qvirtio_pci_set_msix_configuration_vector(dev, alloc, 0);
> @@ -536,7 +543,7 @@ static void pci_idx(void)
>      bus = test_start();
>      alloc = pc_alloc_init();
>  
> -    dev = virtio_blk_init(bus);
> +    dev = virtio_blk_init(bus, PCI_SLOT);
>      qpci_msix_enable(dev->pdev);
>  
>      qvirtio_pci_set_msix_configuration_vector(dev, alloc, 0);
> @@ -640,6 +647,51 @@ static void pci_idx(void)
>      test_end();
>  }
>  
> +static void hotplug(void)
> +{
> +    QDict *response;
> +    QPCIBus *bus;
> +    QVirtioPCIDevice *dev;
> +    bus = test_start();
> +
> +    /* plug secondary disk */
> +    response = qmp("{\"execute\": \"device_add\","
> +                   " \"arguments\": {"
> +                   "   \"driver\": \"virtio-blk-pci\","
> +                   "   \"drive\": \"drive1\","
> +                   "   \"addr\": \"" stringify(PCI_SLOT_HP) "\","
> +                   "   \"id\": \"drv1\""
> +                   "}}");
> +
> +    g_assert(response);
> +    g_assert(!qdict_haskey(response, "error"));
> +    QDECREF(response);
> +
> +    dev = virtio_blk_init(bus, PCI_SLOT_HP);
> +    g_assert(dev);
> +    qvirtio_pci_device_disable(dev);
> +    g_free(dev);
> +
> +    /* unplug secondary disk */
> +    response = qmp("{\"execute\": \"device_del\","
> +                   " \"arguments\": {"
> +                   "   \"id\": \"drv1\""
> +                   "}}");
> +
> +    g_assert(response);
> +    g_assert(!qdict_haskey(response, "error"));
> +    QDECREF(response);
> +
> +    outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << PCI_SLOT_HP);

Code duplication alarm! :)

Paolo

> +    /* check DEVICE_DELETED event */
> +    response = qmp("");
> +    g_assert(qdict_haskey(response, "event"));
> +    g_assert(!strcmp(qdict_get_str(response, "event"), "DEVICE_DELETED"));
> +
> +    test_end();
> +}
> +
>  int main(int argc, char **argv)
>  {
>      int ret;
> @@ -651,6 +703,7 @@ int main(int argc, char **argv)
>      g_test_add_func("/virtio/blk/pci/config", pci_config);
>      g_test_add_func("/virtio/blk/pci/msix", pci_msix);
>      g_test_add_func("/virtio/blk/pci/idx", pci_idx);
> +    g_test_add_func("/virtio/blk/pci/hotplug", hotplug);
>  
>      ret = g_test_run();
>  
> 

Re: [Qemu-devel] [PATCH 26/30] usb-storage: make its storage SCSI bus hotpluggable explicitly

[Gerd Hoffmann]

On Mi, 2014-09-24 at 11:48 +0000, Igor Mammedov wrote:
> usb-storage uses SCSI bus to provide underling storage
> (i.e. scsi-disk) and it's hotpluggable.

No.  usb-storage itself (the scsi hba) is hotpluggable, but the scsi
devices connected are not.

On the qemu side it isn't a problem, but the protocol has constrains.
First there is no way to signal devices coming and going, so the guest
wouldn't see them until reboot (or manual driver reload).  Second for
multi-lun devices only the number of luns is reported and devices are
expected to have luns 0 .. $count-1.  So with three devices connected
and the second being hot-unplugged you end up with an illegal
configuration.

Therefore device initialization (IIRC) goes like this today:

  (1) add hba (i.e. usb-storage).
  (2) add scsi device.
  (3) turn off hotplug on the scsi bus.
  (4) make the whole thing visible to the guest (usb_attach_device).

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 26/30] usb-storage: make its storage SCSI bus hotpluggable explicitly

[Gerd Hoffmann]

  Hi,

> it.  We would have to delay the presentation of the usb-bot device until
> after the LUN is there.

usb-bot can have multiple luns, so we can't do that automagically.

>   Perhaps some kind of transaction mechanism?

Yep, that would be needed.  Add a *group* of devices.  Preferably done
in a way that it works for hot-plugging multi-function pci devices too.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 28/30] usb: convert to hotplug handler API

[Gerd Hoffmann]

> @@ -81,7 +81,7 @@ void usb_bus_new(USBBus *bus, size_t bus_size,
>      qbus_create_inplace(bus, bus_size, TYPE_USB_BUS, host, NULL);
>      bus->ops = ops;
>      bus->busnr = next_usb_bus++;
> -    bus->qbus.allow_hotplug = 1; /* Yes, we can */
> +    qbus_set_hotplug_handler(BUS(bus), host, &error_abort);
>      QTAILQ_INIT(&bus->free);
>      QTAILQ_INIT(&bus->used);
>      QTAILQ_INSERT_TAIL(&busses, bus, next);

> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> index d37ed02..91f7d87 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -1313,7 +1313,7 @@ static void ccid_realize(USBDevice *dev, Error **errp)
>      qbus_create_inplace(&s->bus, sizeof(s->bus), TYPE_CCID_BUS, DEVICE(dev),
>                          NULL);
>      s->intr = usb_ep_get(dev, USB_TOKEN_IN, CCID_INT_IN_EP);
> -    s->bus.qbus.allow_hotplug = 1;
> +    qbus_set_hotplug_handler(BUS(&s->bus), DEVICE(dev), &error_abort);

That is the ccid (smartcard) bus, please make it a separate patch.

> diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c
> index 490f2b6..b7990da 100644
> --- a/hw/usb/hcd-ehci-pci.c
> +++ b/hw/usb/hcd-ehci-pci.c
> @@ -138,6 +138,7 @@ static void ehci_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>  
>      k->init = usb_ehci_pci_initfn;
>      k->exit = usb_ehci_pci_exit;
> @@ -145,6 +146,7 @@ static void ehci_class_init(ObjectClass *klass, void *data)
>      k->config_write = usb_ehci_pci_write_config;
>      dc->vmsd = &vmstate_ehci_pci;
>      dc->props = ehci_pci_properties;
> +    hc->unplug = qdev_simple_device_unplug_cb;
>  }

Can't we do this in usb_bus_new instead of duplicating in every host
adapter?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 00/30] complete conversion to hotplug-handler API

[Cornelia Huck]

On Wed, 24 Sep 2014 11:47:49 +0000
Igor Mammedov <imammedo@redhat.com> wrote:

> Series unifies different hotplug mechanisms to a recent
> hotplug-handler API and does shallow conversion of
> devices that still use legacy qdev hotplug to it dropping
> not used after that legacy hotplug path [29/30].
> It also relaces SCSI's own way to do hotplug/unplug with
> hotplug-handler callbacks leaving it the only method
> perform hotplug tasks.
> And the last patch [30/30] allows to unplug of BUS-less
> devices using hotplug-handler API.
> 
> Converted devices are covered with new hotplug
> unit-tests, except of:
>  s390x-*: I have no idea how or means to test it, but code
>           is close to virtio, so it's converted using
>           virtio template
>  pvscsi: is broken, so no means to test it
> 
> Git tree for testing:
>   https://github.com/imammedo/qemu/commits/hp_ctrl_conversion_v1

Gave it a quick test with the s390-ccw-virtio machine.

device_add virtio-balloon-ccw,id=xxx,devno=fe.2.1111

works (device added in qemu and crws injected to guest). However,

device_del xxx

removes the device in qemu but no crws are injected to the guest. That
seems to indicate that virtio_ccw_busdev_unplug() isn't called?

<runs under gdb>

Indeed, the callback isn't invoked for hotunplug.

Re: [Qemu-devel] [PATCH 28/30] usb: convert to hotplug handler API

[Paolo Bonzini]

Il 24/09/2014 15:00, Gerd Hoffmann ha scritto:
>> @@ -81,7 +81,7 @@ void usb_bus_new(USBBus *bus, size_t bus_size,
>>      qbus_create_inplace(bus, bus_size, TYPE_USB_BUS, host, NULL);
>>      bus->ops = ops;
>>      bus->busnr = next_usb_bus++;
>> -    bus->qbus.allow_hotplug = 1; /* Yes, we can */
>> +    qbus_set_hotplug_handler(BUS(bus), host, &error_abort);
>>      QTAILQ_INIT(&bus->free);
>>      QTAILQ_INIT(&bus->used);
>>      QTAILQ_INSERT_TAIL(&busses, bus, next);
> 
>> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
>> index d37ed02..91f7d87 100644
>> --- a/hw/usb/dev-smartcard-reader.c
>> +++ b/hw/usb/dev-smartcard-reader.c
>> @@ -1313,7 +1313,7 @@ static void ccid_realize(USBDevice *dev, Error **errp)
>>      qbus_create_inplace(&s->bus, sizeof(s->bus), TYPE_CCID_BUS, DEVICE(dev),
>>                          NULL);
>>      s->intr = usb_ep_get(dev, USB_TOKEN_IN, CCID_INT_IN_EP);
>> -    s->bus.qbus.allow_hotplug = 1;
>> +    qbus_set_hotplug_handler(BUS(&s->bus), DEVICE(dev), &error_abort);
> 
> That is the ccid (smartcard) bus, please make it a separate patch.
> 
>> diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c
>> index 490f2b6..b7990da 100644
>> --- a/hw/usb/hcd-ehci-pci.c
>> +++ b/hw/usb/hcd-ehci-pci.c
>> @@ -138,6 +138,7 @@ static void ehci_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>>  
>>      k->init = usb_ehci_pci_initfn;
>>      k->exit = usb_ehci_pci_exit;
>> @@ -145,6 +146,7 @@ static void ehci_class_init(ObjectClass *klass, void *data)
>>      k->config_write = usb_ehci_pci_write_config;
>>      dc->vmsd = &vmstate_ehci_pci;
>>      dc->props = ehci_pci_properties;
>> +    hc->unplug = qdev_simple_device_unplug_cb;
>>  }
> 
> Can't we do this in usb_bus_new instead of duplicating in every host
> adapter?

So you would make TYPE_USB_BUS the hotplug handler itself, instead of
the controller?  That would make sense since TYPE_USB_BUS already
coordinates hotplug/unplug with the HCD via the USBPortOps.

Paolo

Re: [Qemu-devel] [PATCH 28/30] usb: convert to hotplug handler API

[Gerd Hoffmann]

  Hi,

> > Can't we do this in usb_bus_new instead of duplicating in every host
> > adapter?
> 
> So you would make TYPE_USB_BUS the hotplug handler itself, instead of
> the controller?

I was more thinking of just setting the callback in common code, but if
we can attach the hotplug interface to the usb bus itself not the usb
host adapters that would be even better.  And it'll probably kill some
headache for the companion controller case.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 13/30] qdev: hotplug: set handler only if HOTPLUG_HANDLER interface is supported

[Igor Mammedov]

On Wed, 24 Sep 2014 14:19:06 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> > Check if 'handler' implements HOTPLUG_HANDLER interface
> > before setting it, if it's not then do nothing and leave
> > bus not hotpluggable.
> > 
> > That would allow to reuse the same code for creating bus
> > for example 'scsi_bus_new()' for both hotpluggable and not
> > hotpluggable controllers.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > PS:
> > Keep allow_hotplug = 1 before check to preserve legacy
> > behaviour for incremental conversion without breaking
> > bisect-ability. I will be removed later in series.
> > ---
> >  include/hw/qdev-core.h | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index ba812c5..653ff4a 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -366,9 +366,14 @@ char *qdev_get_dev_path(DeviceState *dev);
> >  static inline void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler,
> >                                              Error **errp)
> >  {
> > +    bus->allow_hotplug = 1;
> > +
> > +    if (!object_dynamic_cast(OBJECT(handler), TYPE_HOTPLUG_HANDLER)) {
> > +        return;
> > +    }
> > +
> >      object_property_set_link(OBJECT(bus), OBJECT(handler),
> >                               QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
> > -    bus->allow_hotplug = 1;
> >  }
> >  
> >  static inline bool qbus_is_hotpluggable(BusState *bus)
> > 
> 
> Ok, so scsi won't error_abort, but you are actually breaking
> hotplug/unplug on HBAs other than virtio-scsi and pvscsi.  On those HBAs
> you can actually do surprise removal and hotplug, but you have to
> manually rescan the bus in the virtual machine.
I was hesitant what do with other HBAs without explicit hotplug notification.
I'll add dropped patches that convert them as well.

> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 

Re: [Qemu-devel] [PATCH 06/30] test: usb: add port test to uhci unit test

[Igor Mammedov]

On Wed, 24 Sep 2014 14:30:25 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Mi, 2014-09-24 at 11:47 +0000, Igor Mammedov wrote:
> > along with it move common code fro ehci test to
> > a separate file to avoid duplication.
> 
> Having the code mode as separate patch makes it easier to see the actual
> port test added ...
I'll do it on respin.

> 
> > +static void test_port_1(void)
> > +{
> > +    QPCIBus *pcibus;
> > +    struct qhc uhci;
> > +
> > +    pcibus = qpci_init_pc();
> > +    g_assert(pcibus != NULL);
> > +    usb_pci_init_one(pcibus, &uhci, QPCI_DEVFN(0x1d, 0), 4);
> > +    uhci_port_test(&uhci, 0, UHCI_PORT_CCS);
> >  }
> 
> ... here.  Why test only one of the two ports btw?
Because I'd like to simplify* following hotplug test
that uses port 2, no other ill intentions so far.

*(use the same qtest_start(...) for all test until)
> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH 00/30] complete conversion to hotplug-handler API

[Igor Mammedov]

On Wed, 24 Sep 2014 15:01:44 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Wed, 24 Sep 2014 11:47:49 +0000
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > Series unifies different hotplug mechanisms to a recent
> > hotplug-handler API and does shallow conversion of
> > devices that still use legacy qdev hotplug to it dropping
> > not used after that legacy hotplug path [29/30].
> > It also relaces SCSI's own way to do hotplug/unplug with
> > hotplug-handler callbacks leaving it the only method
> > perform hotplug tasks.
> > And the last patch [30/30] allows to unplug of BUS-less
> > devices using hotplug-handler API.
> > 
> > Converted devices are covered with new hotplug
> > unit-tests, except of:
> >  s390x-*: I have no idea how or means to test it, but code
> >           is close to virtio, so it's converted using
> >           virtio template
> >  pvscsi: is broken, so no means to test it
> > 
> > Git tree for testing:
> >   https://github.com/imammedo/qemu/commits/hp_ctrl_conversion_v1
> 
> Gave it a quick test with the s390-ccw-virtio machine.
> 
> device_add virtio-balloon-ccw,id=xxx,devno=fe.2.1111
> 
> works (device added in qemu and crws injected to guest). However,
> 
> device_del xxx
> 
> removes the device in qemu but no crws are injected to the guest. That
> seems to indicate that virtio_ccw_busdev_unplug() isn't called?
> 
> <runs under gdb>
> 
> Indeed, the callback isn't invoked for hotunplug.
Cornelia,
Thanks for testing, could you share your QEMU CLI and commands you use
for testing. Since it's surprise removal, I'll probably would be able
to debug it without working guest.

BTW:
Is there a way to test it without s390 host+kvm, I've googled for
howto, but haven't found a working CLI+guest combination yet on
x86 fedora host?

Re: [Qemu-devel] [PATCH 14/30] target-i386: ICC bus: replace BusState.allow_hotplug with hotplug_handler

[Igor Mammedov]

On Wed, 24 Sep 2014 14:22:13 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> > it will allow to drop BusState.allow_hotplug field
> > and still keep ICC bus hotluggable for CPU/APIC devices.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/cpu/icc_bus.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
> > index 7f44c59..cb5ed5c 100644
> > --- a/hw/cpu/icc_bus.c
> > +++ b/hw/cpu/icc_bus.c
> > @@ -24,18 +24,10 @@
> >  
> >  /* icc-bridge implementation */
> >  
> > -static void icc_bus_init(Object *obj)
> > -{
> > -    BusState *b = BUS(obj);
> > -
> > -    b->allow_hotplug = true;
> > -}
> > -
> >  static const TypeInfo icc_bus_info = {
> >      .name = TYPE_ICC_BUS,
> >      .parent = TYPE_BUS,
> >      .instance_size = sizeof(ICCBus),
> > -    .instance_init = icc_bus_init,
> >  };
> >  
> >  
> > @@ -100,11 +92,19 @@ static void icc_bridge_init(Object *obj)
> >      s->icc_bus.apic_address_space = &s->apic_container;
> >  }
> >  
> > +static void icc_bridge_realize(DeviceState *dev, Error **errp)
> > +{
> > +    ICCBridgeState *s = ICC_BRIGDE(dev);
> > +
> > +    qbus_set_hotplug_handler(BUS(&s->icc_bus), dev, errp);
> > +}
> > +
> >  static void icc_bridge_class_init(ObjectClass *oc, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(oc);
> >  
> >      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> > +    dc->realize = icc_bridge_realize;
> >  }
> >  
> >  static const TypeInfo icc_bridge_info = {
> > @@ -113,6 +113,10 @@ static const TypeInfo icc_bridge_info = {
> >      .instance_init  = icc_bridge_init,
> >      .instance_size  = sizeof(ICCBridgeState),
> >      .class_init = icc_bridge_class_init,
> > +    .interfaces = (InterfaceInfo[]) {
> > +        { TYPE_HOTPLUG_HANDLER },
> > +        { }
> > +    }
> >  };
> >  
> >  
> > 
> 
> If you do this, aren't you enabling CPU hot-unplug?  Should
> hotplug_handler_plug and/or hotplug_handler_unplug return an error if
> there is no callback?

hotplug/unplug of x86 CPU is not usable with device_add/del yet, so
it's not used for CPUs yet.
x86 CPU nor APIC don't have DeviceClass.unplug callback so for old
behavior any attempt to call device_del would cause abort.
With this path attempt would become NOP.

I've thought it would be better to add hotplug_handler_plug and/or hotplug_handler_unplug
error handling to amended CPU unplug patches that were on list.
Leaving this series purpose clean (drop legacy unplug and provide
infrastructure for async unplug that will be used for CPU and pc-dimm devices)

> 
> Paolo

Re: [Qemu-devel] [PATCH 14/30] target-i386: ICC bus: replace BusState.allow_hotplug with hotplug_handler

[Paolo Bonzini]

Il 24/09/2014 16:37, Igor Mammedov ha scritto:
>> > If you do this, aren't you enabling CPU hot-unplug?  Should
>> > hotplug_handler_plug and/or hotplug_handler_unplug return an error if
>> > there is no callback?
> hotplug/unplug of x86 CPU is not usable with device_add/del yet, so
> it's not used for CPUs yet.
> x86 CPU nor APIC don't have DeviceClass.unplug callback so for old
> behavior any attempt to call device_del would cause abort.
> With this path attempt would become NOP.

Can you make it an error instead?  I guess that's related to the other
question---whether hotplug_handler_plug and/or hotplug_handler_unplug
should return an error if there is no callback.

Paolo

Re: [Qemu-devel] [PATCH 15/30] virtio-pci: replace BusState.allow_hotplug with hotplug_handler

[Igor Mammedov]

On Wed, 24 Sep 2014 14:23:25 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> > Although virtio-pci-bus is internal object of composite
> > virtio-pci device and it doesn't participate in
> > -device/device_add hotplug flow, it's required by
> > bus_add_child() that bus must be hotpluggable to
> > be able to add child at runtime.
> > Set parent of virtio-pci-bus as NOP hotplug controller,
> > so that bus_add_child() would allow to add child
> > device during hotplug time when BusState.allow_hotplug
> > is dropped.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/virtio/virtio-pci.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index f560814..0486b25 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1070,6 +1070,10 @@ static const TypeInfo virtio_pci_info = {
> >      .class_init    = virtio_pci_class_init,
> >      .class_size    = sizeof(VirtioPCIClass),
> >      .abstract      = true,
> > +    .interfaces = (InterfaceInfo[]) {
> > +        { TYPE_HOTPLUG_HANDLER },
> > +        { }
> > +    }
> >  };
> >  
> >  /* virtio-blk-pci */
> > @@ -1543,13 +1547,11 @@ static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
> >                                 VirtIOPCIProxy *dev)
> >  {
> >      DeviceState *qdev = DEVICE(dev);
> > -    BusState *qbus;
> >      char virtio_bus_name[] = "virtio-bus";
> >  
> >      qbus_create_inplace(bus, bus_size, TYPE_VIRTIO_PCI_BUS, qdev,
> >                          virtio_bus_name);
> > -    qbus = BUS(bus);
> > -    qbus->allow_hotplug = 1;
> > +    qbus_set_hotplug_handler(BUS(bus), qdev, &error_abort);
> >  }
> >  
> >  static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
> > 
> 
> So this is the opposite of icc-bus; here you need to call the simple
> hot-unplug callback in the hotplug handler, right?
It's only to workaround bus_add_child() limitation, there is no need
to call unplug callback for child of this bus, since bus and child device
are manually managed by owner of bus without involving hotplug machinery
(via PCIDeviceClass.[init|exit] callbacks).

Perhaps instead we should just allow to add child to bus is bus_add_child()
regardless whether it's hotpluggable or not and drop this and other
similar patches.
> 
> Paolo

Re: [Qemu-devel] [PATCH 15/30] virtio-pci: replace BusState.allow_hotplug with hotplug_handler

[Paolo Bonzini]

Il 24/09/2014 16:51, Igor Mammedov ha scritto:
>> > So this is the opposite of icc-bus; here you need to call the simple
>> > hot-unplug callback in the hotplug handler, right?
> It's only to workaround bus_add_child() limitation, there is no need
> to call unplug callback for child of this bus, since bus and child device
> are manually managed by owner of bus without involving hotplug machinery
> (via PCIDeviceClass.[init|exit] callbacks).
> 
> Perhaps instead we should just allow to add child to bus is bus_add_child()
> regardless whether it's hotpluggable or not and drop this and other
> similar patches.

Yeah, that would be great and would simply usb-storage as well.
Hotpluggability really refers to adding device after initial bring-up of
the bus.  If the controller is hotplugged together with something below,
I don't think that counts.

Paolo

Re: [Qemu-devel] [PATCH 21/30] scsi: make scsi_bus_new() assign hotplug controller

[Igor Mammedov]

On Wed, 24 Sep 2014 14:14:48 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> > it will allow incrementally convert every controller that
> > creates SCSI bus and supports hotplug to hotplug controller API
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/scsi/scsi-bus.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> > index 954c607..33495b6 100644
> > --- a/hw/scsi/scsi-bus.c
> > +++ b/hw/scsi/scsi-bus.c
> > @@ -92,7 +92,7 @@ void scsi_bus_new(SCSIBus *bus, size_t bus_size, DeviceState *host,
> >      qbus_create_inplace(bus, bus_size, TYPE_SCSI_BUS, host, bus_name);
> >      bus->busnr = next_scsi_bus++;
> >      bus->info = info;
> > -    bus->qbus.allow_hotplug = 1;
> > +    qbus_set_hotplug_handler(BUS(bus), host, &error_abort);
> >  }
> >  
> >  static void scsi_dma_restart_bh(void *opaque)
> > 
> 
> Hmm, what if the host is not a HotplugHandler?  Then you should just do
> nothing for plug, and object_unparent for unplug.  Is that broken by
> this patch?
It will break with 24/30. I'll respin with all not yet converted
HBAs implementing  HotplugHandler interface with simple unplug callback.

> 
> Paolo
> 

Re: [Qemu-devel] [PATCH 00/30] complete conversion to hotplug-handler API

[Cornelia Huck]

On Wed, 24 Sep 2014 16:20:56 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> Cornelia,
> Thanks for testing, could you share your QEMU CLI and commands you use
> for testing. Since it's surprise removal, I'll probably would be able
> to debug it without working guest.
> 
> BTW:
> Is there a way to test it without s390 host+kvm, I've googled for
> howto, but haven't found a working CLI+guest combination yet on
> x86 fedora host?
> 

I'm afraid you'll need an s390 host to test this :( However, here's
what I did:

/data/git/yyy/qemu/build/s390x-softmmu/qemu-system-s390x -machine s390-ccw-virtio,accel=kvm,usb=off,kernel_irqchip=on -m 1024 -smp 4 -nographic -drive file=/dev/sda,if=none,id=drive-virtio-disk0,format=raw,serial=ccwzfcp1,cache=none -device virtio-blk-ccw,devno=fe.0.0001,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 -kernel /data/git/xxx/linux/arch/s390/boot/image -append "root=/dev/vda1"

<guest>
[root@localhost ~]# lscss
Device   Subchan.  DevType CU Type Use  PIM PAM POM  CHPIDs
----------------------------------------------------------------------
0.0.0000 0.0.0000  0000/00 3832/01 yes  80  80  ff   00000000 00000000 
0.0.0001 0.0.0001  0000/00 3832/02 yes  80  80  ff   00000000 00000000 
</guest>

In the monitor:
device_add virtio-balloon-ccw,id=xxx,devno=fe.2.1111

<guest>
[root@localhost ~]# lscss
Device   Subchan.  DevType CU Type Use  PIM PAM POM  CHPIDs
----------------------------------------------------------------------
0.0.0000 0.0.0000  0000/00 3832/01 yes  80  80  ff   00000000 00000000 
0.0.0001 0.0.0001  0000/00 3832/02 yes  80  80  ff   00000000 00000000 
0.2.1111 0.2.0000  0000/00 3832/05 yes  80  80  ff   00000000 00000000 
[root@localhost ~]# dmesg | grep CRW
[   26.098964] crw_info : CRW reports slct=0, oflw=0, chn=1, rsc=3, anc=0, erc=4, rsid=0
[   26.098981] crw_info : CRW reports slct=0, oflw=0, chn=0, rsc=3, anc=0, erc=4, rsid=20
</guest>

In the monitor:
device_del xxx

lscss in the guest now still shows the same output (the 0.2.1111 line
should be gone), and no new crws show up in dmesg (there should be two
further crws).

I hope this is helpful. Let me know if there's something I can do (like
stepping through qemu in gdb).

Re: [Qemu-devel] [PATCH 25/30] usb-bot: drop not needed "allow_hotplug = 0"

[Igor Mammedov]

On Wed, 24 Sep 2014 14:27:07 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> > usb-bot creates SCSI bus and immediately makes it
> > non hotpluggable which makes not possible to add
> > anything to it after machine is created.
> > 
> > Taking in account that ubs-bot doesn't implement
> > HOTPLUG_HANDLER interface, SCSI will be created as
> > non hotpluggable, so there is not need to reset
> > allow_hotplug field anymore.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/usb/dev-storage.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> > index bd7cc53..bfa71c7 100644
> > --- a/hw/usb/dev-storage.c
> > +++ b/hw/usb/dev-storage.c
> > @@ -653,7 +653,6 @@ static void usb_msd_realize_bot(USBDevice *dev, Error **errp)
> >      usb_desc_init(dev);
> >      scsi_bus_new(&s->bus, sizeof(s->bus), DEVICE(dev),
> >                   &usb_msd_scsi_info_bot, NULL);
> > -    s->bus.qbus.allow_hotplug = 0;
> >      usb_msd_handle_reset(dev);
> >  }
> >  
> > 
> 
> I guess the fate of this patch depends on the answer to the previous
> questions.
> 
> The usb-bot hotplug handler should return an error on both plug and
> unplug.  You will decide whether to do it explicitly, or in
> hw/core/hotplug.c with NULL callbacks in usb-bot.

BUS is not hotpluggable since usb-bot doesn't implement HotplugHandler inteface,
therefor DeviceClass.realize will error out before reaching
hotplug handler callback

device_set_realized()
    if (dev->hotplugged && !dc->hotpluggable) {
        error_set(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
        return;
    }

But there is room for improvement here,
it it's possible to error out even earlier if usb-bot would be marked
as not hotpluggable device and add to qdev_device_add() check if
device is hotpluggable even before it's created.


> 
> Paolo

Re: [Qemu-devel] [PATCH 25/30] usb-bot: drop not needed "allow_hotplug = 0"

[Paolo Bonzini]

Il 24/09/2014 17:15, Igor Mammedov ha scritto:
> But there is room for improvement here,
> it it's possible to error out even earlier if usb-bot would be marked
> as not hotpluggable device and add to qdev_device_add() check if
> device is hotpluggable even before it's created.

That would work too.  Gerd, what about usb-uas?

Paolo

Re: [Qemu-devel] [PATCH 26/30] usb-storage: make its storage SCSI bus hotpluggable explicitly

[Igor Mammedov]

On Wed, 24 Sep 2014 14:50:41 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Mi, 2014-09-24 at 11:48 +0000, Igor Mammedov wrote:
> > usb-storage uses SCSI bus to provide underling storage
> > (i.e. scsi-disk) and it's hotpluggable.
> 
> No.  usb-storage itself (the scsi hba) is hotpluggable, but the scsi
> devices connected are not.
Agree,
I'm sorry for my bad English, under "it's hotpluggable" I've meant
usb-storage.

> 
> On the qemu side it isn't a problem, but the protocol has constrains.
> First there is no way to signal devices coming and going, so the guest
> wouldn't see them until reboot (or manual driver reload).  Second for
> multi-lun devices only the number of luns is reported and devices are
> expected to have luns 0 .. $count-1.  So with three devices connected
> and the second being hot-unplugged you end up with an illegal
> configuration.
> 
> Therefore device initialization (IIRC) goes like this today:
> 
>   (1) add hba (i.e. usb-storage).
>   (2) add scsi device.
>   (3) turn off hotplug on the scsi bus.
>   (4) make the whole thing visible to the guest (usb_attach_device).
> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH 14/30] target-i386: ICC bus: replace BusState.allow_hotplug with hotplug_handler

[Igor Mammedov]

On Wed, 24 Sep 2014 16:50:17 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 24/09/2014 16:37, Igor Mammedov ha scritto:
> >> > If you do this, aren't you enabling CPU hot-unplug?  Should
> >> > hotplug_handler_plug and/or hotplug_handler_unplug return an error if
> >> > there is no callback?
> > hotplug/unplug of x86 CPU is not usable with device_add/del yet, so
> > it's not used for CPUs yet.
> > x86 CPU nor APIC don't have DeviceClass.unplug callback so for old
> > behavior any attempt to call device_del would cause abort.
> > With this path attempt would become NOP.
> 
> Can you make it an error instead?  I guess that's related to the other
> question---whether hotplug_handler_plug and/or hotplug_handler_unplug
> should return an error if there is no callback.

For example that would make us in addition to above add fake plug/unplug
handlers without errors for APIC so it could be realized without error
when it's created by CPU. (applies to all composite devices).

Anyway question is moot if we are to drop hotplug assert from bus_add_child().

> 
> Paolo

Re: [Qemu-devel] [PATCH 14/30] target-i386: ICC bus: replace BusState.allow_hotplug with hotplug_handler

[Paolo Bonzini]

Il 24/09/2014 17:30, Igor Mammedov ha scritto:
>> > Can you make it an error instead?  I guess that's related to the other
>> > question---whether hotplug_handler_plug and/or hotplug_handler_unplug
>> > should return an error if there is no callback.
> For example that would make us in addition to above add fake plug/unplug
> handlers without errors for APIC so it could be realized without error
> when it's created by CPU. (applies to all composite devices).
> 
> Anyway question is moot if we are to drop hotplug assert from bus_add_child().

Ok, I'll wait for v2 then.  Thanks!

Paolo

Re: [Qemu-devel] [PATCH 29/30] qdev: drop legacy hotplug fields/methods

[Igor Mammedov]

On Wed, 24 Sep 2014 14:04:46 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> > @@ -239,10 +239,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
> >              hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
> >          }
> >      } else {
> > -        assert(dc->unplug != NULL);
> > -        if (dc->unplug(dev) < 0) { /* legacy handler */
> > -            error_set(errp, QERR_UNDEFINED_ERROR);
> > -        }
> > +        assert(0);
> >      }
> 
> This is not particularly nice, but it makes sense at this part of the
> series, since an
> 
>     assert(dev->parent_bus && dev->parent_bus->hotplug_handler);
> 
> would be changed immediately in the next patch.  Also, it would change
> indentation and make the patch bigger.  Hence, please consider adding a
> 31st patch that changes
> 
>     hotplug_ctrl = qdev_get_hotplug_handler(dev);
>     if (hotplug_ctrl) {
>         ...
>     } else {
>         assert(0);
>     }
> 
> to
> 
>     hotplug_ctrl = qdev_get_hotplug_handler(dev);
>     assert(hotplug_ctrl);
sure, I'll add extra patch

>     ...
> 
> Still, this patch is okay.
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Paolo

Re: [Qemu-devel] [PATCH 28/30] usb: convert to hotplug handler API

[Igor Mammedov]

On Wed, 24 Sep 2014 15:23:41 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > > Can't we do this in usb_bus_new instead of duplicating in every host
> > > adapter?
> > 
> > So you would make TYPE_USB_BUS the hotplug handler itself, instead of
> > the controller?
> 
> I was more thinking of just setting the callback in common code, but if
> we can attach the hotplug interface to the usb bus itself not the usb
> host adapters that would be even better.  And it'll probably kill some
> headache for the companion controller case.
How making bus a HotplugHandler itself will help with companion controller?

> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH 28/30] usb: convert to hotplug handler API

[Igor Mammedov]

On Wed, 24 Sep 2014 15:00:41 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> > @@ -81,7 +81,7 @@ void usb_bus_new(USBBus *bus, size_t bus_size,
> >      qbus_create_inplace(bus, bus_size, TYPE_USB_BUS, host, NULL);
> >      bus->ops = ops;
> >      bus->busnr = next_usb_bus++;
> > -    bus->qbus.allow_hotplug = 1; /* Yes, we can */
> > +    qbus_set_hotplug_handler(BUS(bus), host, &error_abort);
> >      QTAILQ_INIT(&bus->free);
> >      QTAILQ_INIT(&bus->used);
> >      QTAILQ_INSERT_TAIL(&busses, bus, next);
> 
> > diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> > index d37ed02..91f7d87 100644
> > --- a/hw/usb/dev-smartcard-reader.c
> > +++ b/hw/usb/dev-smartcard-reader.c
> > @@ -1313,7 +1313,7 @@ static void ccid_realize(USBDevice *dev, Error **errp)
> >      qbus_create_inplace(&s->bus, sizeof(s->bus), TYPE_CCID_BUS, DEVICE(dev),
> >                          NULL);
> >      s->intr = usb_ep_get(dev, USB_TOKEN_IN, CCID_INT_IN_EP);
> > -    s->bus.qbus.allow_hotplug = 1;
> > +    qbus_set_hotplug_handler(BUS(&s->bus), DEVICE(dev), &error_abort);
ok

> 
> That is the ccid (smartcard) bus, please make it a separate patch.
> 
> > diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c
> > index 490f2b6..b7990da 100644
> > --- a/hw/usb/hcd-ehci-pci.c
> > +++ b/hw/usb/hcd-ehci-pci.c
> > @@ -138,6 +138,7 @@ static void ehci_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> >  
> >      k->init = usb_ehci_pci_initfn;
> >      k->exit = usb_ehci_pci_exit;
> > @@ -145,6 +146,7 @@ static void ehci_class_init(ObjectClass *klass, void *data)
> >      k->config_write = usb_ehci_pci_write_config;
> >      dc->vmsd = &vmstate_ehci_pci;
> >      dc->props = ehci_pci_properties;
> > +    hc->unplug = qdev_simple_device_unplug_cb;
> >  }
> 
> Can't we do this in usb_bus_new instead of duplicating in every host
> adapter?
sure

> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH 11/30] qdev: HotplugHandler: provide unplug callback

[Tang Chen]

On 09/24/2014 07:48 PM, Igor Mammedov wrote:
> it to be called for actual device removal and
> will allow to separate request and removal handling
> phases of x86-CPU devices and also it's a handler
> to be called for synchronously removable devices.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> unplug handling for bus-less devices will be added
> later in this series.
> ---
>   hw/core/hotplug.c    | 11 +++++++++++
>   hw/core/qdev.c       | 13 +++++++++++--
>   include/hw/hotplug.h | 12 ++++++++++++
>   3 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> index 2ec4736..4e01074 100644
> --- a/hw/core/hotplug.c
> +++ b/hw/core/hotplug.c
> @@ -34,6 +34,17 @@ void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
>       }
>   }
>   
> +void hotplug_handler_unplug(HotplugHandler *plug_handler,
> +                            DeviceState *plugged_dev,
> +                            Error **errp)
> +{
> +    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
> +
> +    if (hdc->unplug) {
> +        hdc->unplug(plug_handler, plugged_dev, errp);
> +    }
> +}
> +
>   static const TypeInfo hotplug_handler_info = {
>       .name          = TYPE_HOTPLUG_HANDLER,
>       .parent        = TYPE_INTERFACE,
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index c98e5db..c89d781 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -227,8 +227,17 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>       qdev_hot_removed = true;
>   
>       if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> -        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler,
> -                                       dev, errp);
> +        HotplugHandlerClass *hdc;
> +
> +        /* If device supports async unplug just request it to be done,
> +         * otherwise just remove it synchronously */
> +        hdc = HOTPLUG_HANDLER_GET_CLASS(dev->parent_bus->hotplug_handler);
> +        if (hdc->unplug_request) {
> +            hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler,
> +                                           dev, errp);
> +        } else {
> +            hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
> +        }
>       } else {
>           assert(dc->unplug != NULL);
>           if (dc->unplug(dev) < 0) { /* legacy handler */
> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> index e397d08..451d522 100644
> --- a/include/hw/hotplug.h
> +++ b/include/hw/hotplug.h
> @@ -50,6 +50,9 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
>    * @unplug_request: unplug request callback.
>    *                  Used as a means to initiate device unplug for devices that
>    *                  require asynchronous unplug handling.
> + * @unplug_request: unplug callback.

Typo, s/unplug_request/unplug ?

Thanks.

> + *                  Used for device removal with devices that implement
> + *                  asynchronous and synchronous (suprise) removal.
>    */
>   typedef struct HotplugHandlerClass {
>       /* <private> */
> @@ -58,6 +61,7 @@ typedef struct HotplugHandlerClass {
>       /* <public> */
>       hotplug_fn plug;
>       hotplug_fn unplug_request;
> +    hotplug_fn unplug;
>   } HotplugHandlerClass;
>   
>   /**
> @@ -77,4 +81,12 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
>   void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
>                                       DeviceState *plugged_dev,
>                                       Error **errp);
> +/**
> + * hotplug_handler_unplug:
> + *
> + * Calls #HotplugHandlerClass.unplug callback of @plug_handler.
> + */
> +void hotplug_handler_unplug(HotplugHandler *plug_handler,
> +                            DeviceState *plugged_dev,
> +                            Error **errp);
>   #endif

Re: [Qemu-devel] [PATCH 09/30] access BusState.allow_hotplug using wraper qbus_is_hotpluggable()

[Tang Chen]

On 09/24/2014 07:47 PM, Igor Mammedov wrote:
> it would allow transparently switch detection if Bus
> is hotpluggable from allow_hotplug field to hotplug_handler
> link and drop allow_hotplug field once all users are
> converted to hotplug handler API.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   hw/core/qdev.c           | 6 +++---
>   hw/i386/acpi-build.c     | 2 +-
>   hw/pci/pci-hotplug-old.c | 4 ++--
>   include/hw/qdev-core.h   | 5 +++++
>   qdev-monitor.c           | 2 +-
>   5 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index fcb1638..5e5b963 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -86,7 +86,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>       BusChild *kid = g_malloc0(sizeof(*kid));
>   
>       if (qdev_hotplug) {
> -        assert(bus->allow_hotplug);
> +        assert(qbus_is_hotpluggable(bus));
>       }
>   
>       kid->index = bus->max_index++;
> @@ -213,7 +213,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>   {
>       DeviceClass *dc = DEVICE_GET_CLASS(dev);
>   
> -    if (dev->parent_bus && !dev->parent_bus->allow_hotplug) {
> +    if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
>           error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
>           return;
>       }
> @@ -925,7 +925,7 @@ static bool device_get_hotpluggable(Object *obj, Error **errp)
>       DeviceState *dev = DEVICE(obj);
>   
>       return dc->hotpluggable && (dev->parent_bus == NULL ||
> -                                dev->parent_bus->allow_hotplug);
> +                                qbus_is_hotpluggable(dev->parent_bus));
>   }
>   
>   static bool device_get_hotplugged(Object *obj, Error **err)
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index a313321..00be4bb 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -774,7 +774,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>       unsigned *bsel_alloc = opaque;
>       unsigned *bus_bsel;
>   
> -    if (bus->qbus.allow_hotplug) {
> +    if (qbus_is_hotpluggable(BUS(bus))) {
>           bus_bsel = g_malloc(sizeof *bus_bsel);
>   
>           *bus_bsel = (*bsel_alloc)++;
> diff --git a/hw/pci/pci-hotplug-old.c b/hw/pci/pci-hotplug-old.c
> index cf2caeb..f9b7398 100644
> --- a/hw/pci/pci-hotplug-old.c
> +++ b/hw/pci/pci-hotplug-old.c
> @@ -77,7 +77,7 @@ static PCIDevice *qemu_pci_hot_add_nic(Monitor *mon,
>           monitor_printf(mon, "Invalid PCI device address %s\n", devaddr);
>           return NULL;
>       }
> -    if (!((BusState*)bus)->allow_hotplug) {
> +    if (!qbus_is_hotpluggable(BUS(bus))) {
>           monitor_printf(mon, "PCI bus doesn't support hotplug\n");
>           return NULL;
>       }
> @@ -224,7 +224,7 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
>           monitor_printf(mon, "Invalid PCI device address %s\n", devaddr);
>           return NULL;
>       }
> -    if (!((BusState*)bus)->allow_hotplug) {
> +    if (!qbus_is_hotpluggable(BUS(bus))) {
>           monitor_printf(mon, "PCI bus doesn't support hotplug\n");
>           return NULL;
>       }
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 178fee2..48a96d2 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -368,4 +368,9 @@ static inline void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler,
>                                QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
>       bus->allow_hotplug = 1;
>   }
> +
> +static inline bool qbus_is_hotpluggable(BusState *bus)
> +{
> +   return bus->allow_hotplug || bus->hotplug_handler;

Why not synchronize these two members ?  What is the case that bus has
a hotplug_handler but allow_hotplug is false ?

BTW, I think there are other places that using bus->allow_hotplug 
directly, right ?

eg:
s390_virtio_bus_init()
{
     ......
     _bus->allow_hotplug = 1;
     ......
}

Thanks.

> +}
>   #endif
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 5ec6606..f6db461 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -515,7 +515,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>               return NULL;
>           }
>       }
> -    if (qdev_hotplug && bus && !bus->allow_hotplug) {
> +    if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
>           qerror_report(QERR_BUS_NO_HOTPLUG, bus->name);
>           return NULL;
>       }

Re: [Qemu-devel] [PATCH 13/30] qdev: hotplug: set handler only if HOTPLUG_HANDLER interface is supported

[Tang Chen]

On 09/24/2014 07:48 PM, Igor Mammedov wrote:
> Check if 'handler' implements HOTPLUG_HANDLER interface
> before setting it, if it's not then do nothing and leave
> bus not hotpluggable.
>
> That would allow to reuse the same code for creating bus
> for example 'scsi_bus_new()' for both hotpluggable and not
> hotpluggable controllers.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> PS:
> Keep allow_hotplug = 1 before check to preserve legacy
> behaviour for incremental conversion without breaking
> bisect-ability. I will be removed later in series.
> ---
>   include/hw/qdev-core.h | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index ba812c5..653ff4a 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -366,9 +366,14 @@ char *qdev_get_dev_path(DeviceState *dev);
>   static inline void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler,
>                                               Error **errp)
>   {
> +    bus->allow_hotplug = 1;

BTW, why not make allow_hotplug a bool ?

Thanks.

> +
> +    if (!object_dynamic_cast(OBJECT(handler), TYPE_HOTPLUG_HANDLER)) {
> +        return;
> +    }
> +
>       object_property_set_link(OBJECT(bus), OBJECT(handler),
>                                QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
> -    bus->allow_hotplug = 1;
>   }
>   
>   static inline bool qbus_is_hotpluggable(BusState *bus)

Re: [Qemu-devel] [PATCH 28/30] usb: convert to hotplug handler API

[Gerd Hoffmann]

On Mi, 2014-09-24 at 17:39 +0200, Igor Mammedov wrote:
> On Wed, 24 Sep 2014 15:23:41 +0200
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> >   Hi,
> > 
> > > > Can't we do this in usb_bus_new instead of duplicating in every host
> > > > adapter?
> > > 
> > > So you would make TYPE_USB_BUS the hotplug handler itself, instead of
> > > the controller?
> > 
> > I was more thinking of just setting the callback in common code, but if
> > we can attach the hotplug interface to the usb bus itself not the usb
> > host adapters that would be even better.  And it'll probably kill some
> > headache for the companion controller case.
> How making bus a HotplugHandler itself will help with companion controller?

When uhci acts as ehci companion it registers the ports with ehci and
doesn't manage its own usb bus.  ehci will call uhci port ops as needed
(depending on ehci configuration).

When attaching the hotplug interface to the host controller you'll have
to explicitly handle the companion case somehow.

When attaching the hotplug interface to the usb bus everything should
just work.  Additional bonus is that you also don't have to touch the
host controller code at all then, it should be doable by changing
hw/usb/bus.c only.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 26/30] usb-storage: make its storage SCSI bus hotpluggable explicitly

[Gerd Hoffmann]

On Mi, 2014-09-24 at 17:22 +0200, Igor Mammedov wrote:
> On Wed, 24 Sep 2014 14:50:41 +0200
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> > On Mi, 2014-09-24 at 11:48 +0000, Igor Mammedov wrote:
> > > usb-storage uses SCSI bus to provide underling storage
> > > (i.e. scsi-disk) and it's hotpluggable.
> > 
> > No.  usb-storage itself (the scsi hba) is hotpluggable, but the scsi
> > devices connected are not.
> Agree,
> I'm sorry for my bad English, under "it's hotpluggable" I've meant
> usb-storage.

/me is confused.  That contradicts $subject.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 25/30] usb-bot: drop not needed "allow_hotplug = 0"

[Gerd Hoffmann]

  Hi,

> But there is room for improvement here,
> it it's possible to error out even earlier if usb-bot would be marked
> as not hotpluggable device and add to qdev_device_add() check if
> device is hotpluggable even before it's created.

usb-bot not being hot-pluggable is a consequence of the fact that we can
only hotplug single devices in qemu today.  usb-bot without scsi devices
hooked up is useless.  Plugging in usb-bot first and plugging in disks
later doesn't work (well, could sort-of work when you plug them fast,
but you are racing with the guest initializing the device then).  With
some way to first compose the whole thing (usb-bot + disks), then
plug-in as whole usb-bot hotplugging would work though.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 25/30] usb-bot: drop not needed "allow_hotplug = 0"

[Gerd Hoffmann]

On Mi, 2014-09-24 at 17:21 +0200, Paolo Bonzini wrote:
> Il 24/09/2014 17:15, Igor Mammedov ha scritto:
> > But there is room for improvement here,
> > it it's possible to error out even earlier if usb-bot would be marked
> > as not hotpluggable device and add to qdev_device_add() check if
> > device is hotpluggable even before it's created.
> 
> That would work too.  Gerd, what about usb-uas?

Basically the same camp as usb-bot.  UAS doesn't suffer the LUN
numeration issue which BOT has, but there likewise is no signaling about
scsi devices coming and going.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 09/30] access BusState.allow_hotplug using wraper qbus_is_hotpluggable()

[Igor Mammedov]

On Thu, 25 Sep 2014 10:00:05 +0800
Tang Chen <tangchen@cn.fujitsu.com> wrote:

> 
> On 09/24/2014 07:47 PM, Igor Mammedov wrote:
> > it would allow transparently switch detection if Bus
> > is hotpluggable from allow_hotplug field to hotplug_handler
> > link and drop allow_hotplug field once all users are
> > converted to hotplug handler API.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >   hw/core/qdev.c           | 6 +++---
> >   hw/i386/acpi-build.c     | 2 +-
> >   hw/pci/pci-hotplug-old.c | 4 ++--
> >   include/hw/qdev-core.h   | 5 +++++
> >   qdev-monitor.c           | 2 +-
> >   5 files changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index fcb1638..5e5b963 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -86,7 +86,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
> >       BusChild *kid = g_malloc0(sizeof(*kid));
> >   
> >       if (qdev_hotplug) {
> > -        assert(bus->allow_hotplug);
> > +        assert(qbus_is_hotpluggable(bus));
> >       }
> >   
> >       kid->index = bus->max_index++;
> > @@ -213,7 +213,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
> >   {
> >       DeviceClass *dc = DEVICE_GET_CLASS(dev);
> >   
> > -    if (dev->parent_bus && !dev->parent_bus->allow_hotplug) {
> > +    if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
> >           error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
> >           return;
> >       }
> > @@ -925,7 +925,7 @@ static bool device_get_hotpluggable(Object *obj, Error **errp)
> >       DeviceState *dev = DEVICE(obj);
> >   
> >       return dc->hotpluggable && (dev->parent_bus == NULL ||
> > -                                dev->parent_bus->allow_hotplug);
> > +                                qbus_is_hotpluggable(dev->parent_bus));
> >   }
> >   
> >   static bool device_get_hotplugged(Object *obj, Error **err)
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index a313321..00be4bb 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -774,7 +774,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
> >       unsigned *bsel_alloc = opaque;
> >       unsigned *bus_bsel;
> >   
> > -    if (bus->qbus.allow_hotplug) {
> > +    if (qbus_is_hotpluggable(BUS(bus))) {
> >           bus_bsel = g_malloc(sizeof *bus_bsel);
> >   
> >           *bus_bsel = (*bsel_alloc)++;
> > diff --git a/hw/pci/pci-hotplug-old.c b/hw/pci/pci-hotplug-old.c
> > index cf2caeb..f9b7398 100644
> > --- a/hw/pci/pci-hotplug-old.c
> > +++ b/hw/pci/pci-hotplug-old.c
> > @@ -77,7 +77,7 @@ static PCIDevice *qemu_pci_hot_add_nic(Monitor *mon,
> >           monitor_printf(mon, "Invalid PCI device address %s\n", devaddr);
> >           return NULL;
> >       }
> > -    if (!((BusState*)bus)->allow_hotplug) {
> > +    if (!qbus_is_hotpluggable(BUS(bus))) {
> >           monitor_printf(mon, "PCI bus doesn't support hotplug\n");
> >           return NULL;
> >       }
> > @@ -224,7 +224,7 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
> >           monitor_printf(mon, "Invalid PCI device address %s\n", devaddr);
> >           return NULL;
> >       }
> > -    if (!((BusState*)bus)->allow_hotplug) {
> > +    if (!qbus_is_hotpluggable(BUS(bus))) {
> >           monitor_printf(mon, "PCI bus doesn't support hotplug\n");
> >           return NULL;
> >       }
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 178fee2..48a96d2 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -368,4 +368,9 @@ static inline void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler,
> >                                QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
> >       bus->allow_hotplug = 1;
> >   }
> > +
> > +static inline bool qbus_is_hotpluggable(BusState *bus)
> > +{
> > +   return bus->allow_hotplug || bus->hotplug_handler;
> 
> Why not synchronize these two members ?  What is the case that bus has
> a hotplug_handler but allow_hotplug is false ?
> 
> BTW, I think there are other places that using bus->allow_hotplug 
> directly, right ?
There is no point in synchronizing them since by the end of this
series 'allow_hotplug' field will be removed and only 'hotplug_handler'
will be left. allow_hotplug is kept till the 29/30 patch for maintaining
series bisect-ability and for the sake of splitting patches to small clean
ones.

> 
> eg:
> s390_virtio_bus_init()
> {
>      ......
>      _bus->allow_hotplug = 1;
>      ......
> }
> 
> Thanks.
> 
> > +}
> >   #endif
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index 5ec6606..f6db461 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -515,7 +515,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >               return NULL;
> >           }
> >       }
> > -    if (qdev_hotplug && bus && !bus->allow_hotplug) {
> > +    if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
> >           qerror_report(QERR_BUS_NO_HOTPLUG, bus->name);
> >           return NULL;
> >       }
> 

Re: [Qemu-devel] [PATCH 11/30] qdev: HotplugHandler: provide unplug callback

[Igor Mammedov]

On Thu, 25 Sep 2014 09:53:20 +0800
Tang Chen <tangchen@cn.fujitsu.com> wrote:

> 
> On 09/24/2014 07:48 PM, Igor Mammedov wrote:
> > it to be called for actual device removal and
> > will allow to separate request and removal handling
> > phases of x86-CPU devices and also it's a handler
> > to be called for synchronously removable devices.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > unplug handling for bus-less devices will be added
> > later in this series.
> > ---
> >   hw/core/hotplug.c    | 11 +++++++++++
> >   hw/core/qdev.c       | 13 +++++++++++--
> >   include/hw/hotplug.h | 12 ++++++++++++
> >   3 files changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> > index 2ec4736..4e01074 100644
> > --- a/hw/core/hotplug.c
> > +++ b/hw/core/hotplug.c
> > @@ -34,6 +34,17 @@ void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
> >       }
> >   }
> >   
> > +void hotplug_handler_unplug(HotplugHandler *plug_handler,
> > +                            DeviceState *plugged_dev,
> > +                            Error **errp)
> > +{
> > +    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
> > +
> > +    if (hdc->unplug) {
> > +        hdc->unplug(plug_handler, plugged_dev, errp);
> > +    }
> > +}
> > +
> >   static const TypeInfo hotplug_handler_info = {
> >       .name          = TYPE_HOTPLUG_HANDLER,
> >       .parent        = TYPE_INTERFACE,
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index c98e5db..c89d781 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -227,8 +227,17 @@ void qdev_unplug(DeviceState *dev, Error **errp)
> >       qdev_hot_removed = true;
> >   
> >       if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> > -        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler,
> > -                                       dev, errp);
> > +        HotplugHandlerClass *hdc;
> > +
> > +        /* If device supports async unplug just request it to be done,
> > +         * otherwise just remove it synchronously */
> > +        hdc = HOTPLUG_HANDLER_GET_CLASS(dev->parent_bus->hotplug_handler);
> > +        if (hdc->unplug_request) {
> > +            hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler,
> > +                                           dev, errp);
> > +        } else {
> > +            hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
> > +        }
> >       } else {
> >           assert(dc->unplug != NULL);
> >           if (dc->unplug(dev) < 0) { /* legacy handler */
> > diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> > index e397d08..451d522 100644
> > --- a/include/hw/hotplug.h
> > +++ b/include/hw/hotplug.h
> > @@ -50,6 +50,9 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
> >    * @unplug_request: unplug request callback.
> >    *                  Used as a means to initiate device unplug for devices that
> >    *                  require asynchronous unplug handling.
> > + * @unplug_request: unplug callback.
> 
> Typo, s/unplug_request/unplug ?
Yep, I'll fix it.

> 
> Thanks.
> 
> > + *                  Used for device removal with devices that implement
> > + *                  asynchronous and synchronous (suprise) removal.
> >    */
> >   typedef struct HotplugHandlerClass {
> >       /* <private> */
> > @@ -58,6 +61,7 @@ typedef struct HotplugHandlerClass {
> >       /* <public> */
> >       hotplug_fn plug;
> >       hotplug_fn unplug_request;
> > +    hotplug_fn unplug;
> >   } HotplugHandlerClass;
> >   
> >   /**
> > @@ -77,4 +81,12 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
> >   void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
> >                                       DeviceState *plugged_dev,
> >                                       Error **errp);
> > +/**
> > + * hotplug_handler_unplug:
> > + *
> > + * Calls #HotplugHandlerClass.unplug callback of @plug_handler.
> > + */
> > +void hotplug_handler_unplug(HotplugHandler *plug_handler,
> > +                            DeviceState *plugged_dev,
> > +                            Error **errp);
> >   #endif
> 
> 

Re: [Qemu-devel] [PATCH 25/30] usb-bot: drop not needed "allow_hotplug = 0"

[Igor Mammedov]

On Thu, 25 Sep 2014 10:01:53 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Mi, 2014-09-24 at 17:21 +0200, Paolo Bonzini wrote:
> > Il 24/09/2014 17:15, Igor Mammedov ha scritto:
> > > But there is room for improvement here,
> > > it it's possible to error out even earlier if usb-bot would be marked
> > > as not hotpluggable device and add to qdev_device_add() check if
> > > device is hotpluggable even before it's created.
> > 
> > That would work too.  Gerd, what about usb-uas?
> 
> Basically the same camp as usb-bot.  UAS doesn't suffer the LUN
> numeration issue which BOT has, but there likewise is no signaling about
> scsi devices coming and going.
Thet's why a excluded UAS from this series,
Do I need to make it hotpluggable (like SCSI HBAs without hotplug
signalling)? /i.e./ it would be possible to hotplug it but guest
won't notice the new drive on UAS until reboot/

> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH 28/30] usb: convert to hotplug handler API

[Igor Mammedov]

On Thu, 25 Sep 2014 09:50:58 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Mi, 2014-09-24 at 17:39 +0200, Igor Mammedov wrote:
> > On Wed, 24 Sep 2014 15:23:41 +0200
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > 
> > >   Hi,
> > > 
> > > > > Can't we do this in usb_bus_new instead of duplicating in every host
> > > > > adapter?
> > > > 
> > > > So you would make TYPE_USB_BUS the hotplug handler itself, instead of
> > > > the controller?
> > > 
> > > I was more thinking of just setting the callback in common code, but if
> > > we can attach the hotplug interface to the usb bus itself not the usb
> > > host adapters that would be even better.  And it'll probably kill some
> > > headache for the companion controller case.
> > How making bus a HotplugHandler itself will help with companion controller?
> 
> When uhci acts as ehci companion it registers the ports with ehci and
> doesn't manage its own usb bus.  ehci will call uhci port ops as needed
> (depending on ehci configuration).
> 
> When attaching the hotplug interface to the host controller you'll have
> to explicitly handle the companion case somehow.
> 
> When attaching the hotplug interface to the usb bus everything should
> just work.  Additional bonus is that you also don't have to touch the
> host controller code at all then, it should be doable by changing
> hw/usb/bus.c only.
hotplug-handler.[plug|unplug] callbacks are class wide, so if
USB bus might ever need to have different callbacks depending on
host it might not work.

But since so far it uses the only qdev_simple_device_unplug_cb(),
having BUS as hotplug-handler should work too.


> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH 20/30] s390x: convert virtio-ccw to hotplug handler API

[Cornelia Huck]

On Wed, 24 Sep 2014 11:48:09 +0000
Igor Mammedov <imammedo@redhat.com> wrote:

> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/s390x/virtio-ccw.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)

Well, I think I now see what's going on here. More below...


> @@ -1620,13 +1620,13 @@ static Property virtio_ccw_properties[] = {
>  static void virtio_ccw_device_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> 
>      dc->props = virtio_ccw_properties;
>      dc->init = virtio_ccw_busdev_init;
>      dc->exit = virtio_ccw_busdev_exit;
> -    dc->unplug = virtio_ccw_busdev_unplug;

Before, this callback was invoked when a device of the virtio-ccw class
was unplugged.

>      dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
> -
> +    hc->unplug = virtio_ccw_busdev_unplug;

Now, this callback is supposed to be invoked instead. However, the
unplugging code invokes the callback for the _parent bus_, which
means...

>  }
> 
>  static const TypeInfo virtio_ccw_device_info = {

>  static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
>  {
>      SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> 
>      k->init = virtual_css_bridge_init;
> +    hc->unplug = qdev_simple_device_unplug_cb;

...we're invoking this one, as the parent bus for virtio-ccw devices is
the virtual-css bus.

If I change this callback to the virtio-ccw one, everything works as
expected.

>  }

So, to summarize, what happened before was

bridge device              <--- (simple unplug invoked for dev)
 -> virtual bus
  -> virtio proxy device   <--- virtio unplug invoked for dev
   -> virtio bus
    -> virtio device

which your patch changed to

bridge device
 -> virtual bus            <--- simple unplug invoked for virtio proxy dev
  -> virtio proxy device
   -> virtio bus           <--- (virtio unplug invoked for virtio dev)
    -> virtio device

Am I understanding this correctly?

Re: [Qemu-devel] [PATCH 28/30] usb: convert to hotplug handler API

[Paolo Bonzini]

Il 25/09/2014 12:55, Igor Mammedov ha scritto:
> hotplug-handler.[plug|unplug] callbacks are class wide, so if
> USB bus might ever need to have different callbacks depending on
> host it might not work.
> 
> But since so far it uses the only qdev_simple_device_unplug_cb(),
> having BUS as hotplug-handler should work too.

Yeah, in effect the USB bus is using attach/detach as the
controller-specific part of the hotplug handler.

Perhaps the same can work for SCSI as well?  I'm waiting for v2. :)

Paolo

Re: [Qemu-devel] [PATCH 20/30] s390x: convert virtio-ccw to hotplug handler API

[Igor Mammedov]

On Thu, 25 Sep 2014 13:08:38 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Wed, 24 Sep 2014 11:48:09 +0000
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/s390x/virtio-ccw.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> Well, I think I now see what's going on here. More below...
> 
> 
> > @@ -1620,13 +1620,13 @@ static Property virtio_ccw_properties[] = {
> >  static void virtio_ccw_device_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> > +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> > 
> >      dc->props = virtio_ccw_properties;
> >      dc->init = virtio_ccw_busdev_init;
> >      dc->exit = virtio_ccw_busdev_exit;
> > -    dc->unplug = virtio_ccw_busdev_unplug;
> 
> Before, this callback was invoked when a device of the virtio-ccw class
> was unplugged.
> 
> >      dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
> > -
> > +    hc->unplug = virtio_ccw_busdev_unplug;
> 
> Now, this callback is supposed to be invoked instead. However, the
> unplugging code invokes the callback for the _parent bus_, which
> means...
> 
> >  }
> > 
> >  static const TypeInfo virtio_ccw_device_info = {
> 
> >  static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
> >  {
> >      SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> > +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> > 
> >      k->init = virtual_css_bridge_init;
> > +    hc->unplug = qdev_simple_device_unplug_cb;
> 
> ...we're invoking this one, as the parent bus for virtio-ccw devices is
> the virtual-css bus.
> 
> If I change this callback to the virtio-ccw one, everything works as
> expected.
> 
> >  }
> 
> So, to summarize, what happened before was
> 
> bridge device              <--- (simple unplug invoked for dev)
simple unplug should not exits for above device

>  -> virtual bus
>   -> virtio proxy device   <--- virtio unplug invoked for dev

>    -> virtio bus
>     -> virtio device
> 
> which your patch changed to
> 
> bridge device
>  -> virtual bus            <--- simple unplug invoked for virtio proxy dev
>   -> virtio proxy device
>    -> virtio bus           <--- (virtio unplug invoked for virtio dev)
>     -> virtio device
> 
> Am I understanding this correctly?
Let's try other way around:

bridge device (virtual_css_bridge) - non hotpluggable sysbus device
  -> virtual bus (VIRTUAL_CSS_BUS)
   -> virtio proxy device |
    -> virtio bus         |- virtio_ccw_device_foo composite device managed with
     -> virtio device     |  device_add/del command

internal "virtio device" is the only child of "virtio bus" and it's not supposed
to be managed via device_add/del.

So to unplug "virtio proxy device" we should call virtio_ccw_busdev_unplug
stored in "bridge device" with "virtual bus" using bridge as hotplug-handler.

does following patch work for you?
---

From 8f249aa4686f0a7dfa5d9636d1eee68f1d264316 Mon Sep 17 00:00:00 2001
From: Igor Mammedov <imammedo@redhat.com>
Date: Fri, 19 Sep 2014 09:07:10 +0000
Subject: [PATCH] s390x: convert virtio-ccw to hotplug handler API

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/s390x/virtio-ccw.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 33a1d86..036bd20 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -230,7 +230,7 @@ VirtualCssBus *virtual_css_bus_init(void)
     cbus = VIRTUAL_CSS_BUS(bus);
 
     /* Enable hotplugging */
-    bus->allow_hotplug = 1;
+    qbus_set_hotplug_handler(bus, dev, &error_abort);
 
     return cbus;
 }
@@ -1590,7 +1590,8 @@ static int virtio_ccw_busdev_exit(DeviceState *dev)
     return _info->exit(_dev);
 }
 
-static int virtio_ccw_busdev_unplug(DeviceState *dev)
+static void virtio_ccw_busdev_unplug(HotplugHandler *hotplug_dev,
+                                     DeviceState *dev, Error **errp)
 {
     VirtioCcwDevice *_dev = (VirtioCcwDevice *)dev;
     SubchDev *sch = _dev->sch;
@@ -1609,7 +1610,6 @@ static int virtio_ccw_busdev_unplug(DeviceState *dev)
     css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid, 1, 0);
 
     object_unparent(OBJECT(dev));
-    return 0;
 }
 
 static Property virtio_ccw_properties[] = {
@@ -1624,9 +1624,7 @@ static void virtio_ccw_device_class_init(ObjectClass *klass, void *data)
     dc->props = virtio_ccw_properties;
     dc->init = virtio_ccw_busdev_init;
     dc->exit = virtio_ccw_busdev_exit;
-    dc->unplug = virtio_ccw_busdev_unplug;
     dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
-
 }
 
 static const TypeInfo virtio_ccw_device_info = {
@@ -1636,6 +1634,10 @@ static const TypeInfo virtio_ccw_device_info = {
     .class_init = virtio_ccw_device_class_init,
     .class_size = sizeof(VirtIOCCWDeviceClass),
     .abstract = true,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 /***************** Virtual-css Bus Bridge Device ********************/
@@ -1650,8 +1652,10 @@ static int virtual_css_bridge_init(SysBusDevice *dev)
 static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
 {
     SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     k->init = virtual_css_bridge_init;
+    hc->unplug = virtio_ccw_busdev_unplug;
 }
 
 static const TypeInfo virtual_css_bridge_info = {
@@ -1659,6 +1663,10 @@ static const TypeInfo virtual_css_bridge_info = {
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(SysBusDevice),
     .class_init    = virtual_css_bridge_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 /* virtio-ccw-bus */
@@ -1667,13 +1675,11 @@ static void virtio_ccw_bus_new(VirtioBusState *bus, size_t bus_size,
                                VirtioCcwDevice *dev)
 {
     DeviceState *qdev = DEVICE(dev);
-    BusState *qbus;
     char virtio_bus_name[] = "virtio-bus";
 
     qbus_create_inplace(bus, bus_size, TYPE_VIRTIO_CCW_BUS,
                         qdev, virtio_bus_name);
-    qbus = BUS(bus);
-    qbus->allow_hotplug = 1;
+    qbus_set_hotplug_handler(BUS(bus), qdev, &error_abort);
 }
 
 static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 28/30] usb: convert to hotplug handler API

[Igor Mammedov]

On Thu, 25 Sep 2014 14:47:31 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 25/09/2014 12:55, Igor Mammedov ha scritto:
> > hotplug-handler.[plug|unplug] callbacks are class wide, so if
> > USB bus might ever need to have different callbacks depending on
> > host it might not work.
> > 
> > But since so far it uses the only qdev_simple_device_unplug_cb(),
> > having BUS as hotplug-handler should work too.
> 
> Yeah, in effect the USB bus is using attach/detach as the
> controller-specific part of the hotplug handler.
> 
> Perhaps the same can work for SCSI as well?  I'm waiting for v2. :)
Hopefully it should be ready by the end of this day.

> 
> Paolo

Re: [Qemu-devel] [PATCH 25/30] usb-bot: drop not needed "allow_hotplug = 0"

[Gerd Hoffmann]

On Do, 2014-09-25 at 10:12 +0200, Igor Mammedov wrote:

> > Basically the same camp as usb-bot.  UAS doesn't suffer the LUN
> > numeration issue which BOT has, but there likewise is no signaling about
> > scsi devices coming and going.
> Thet's why a excluded UAS from this series,
> Do I need to make it hotpluggable (like SCSI HBAs without hotplug
> signalling)? /i.e./ it would be possible to hotplug it but guest
> won't notice the new drive on UAS until reboot/

Yes, in that regard uas is more simliar to lsi than to bot.  Hotplug
works, but needs manual rescan / driver reload / reboot to make the
guest see the device.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 20/30] s390x: convert virtio-ccw to hotplug handler API

[Cornelia Huck]

On Thu, 25 Sep 2014 15:11:10 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Thu, 25 Sep 2014 13:08:38 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> > So, to summarize, what happened before was
> > 
> > bridge device              <--- (simple unplug invoked for dev)
> simple unplug should not exits for above device

Yes. I'm not sure why it does.

> 
> >  -> virtual bus
> >   -> virtio proxy device   <--- virtio unplug invoked for dev
> 
> >    -> virtio bus
> >     -> virtio device
> > 
> > which your patch changed to
> > 
> > bridge device
> >  -> virtual bus            <--- simple unplug invoked for virtio proxy dev
> >   -> virtio proxy device
> >    -> virtio bus           <--- (virtio unplug invoked for virtio dev)
> >     -> virtio device
> > 
> > Am I understanding this correctly?
> Let's try other way around:
> 
> bridge device (virtual_css_bridge) - non hotpluggable sysbus device
>   -> virtual bus (VIRTUAL_CSS_BUS)
>    -> virtio proxy device |
>     -> virtio bus         |- virtio_ccw_device_foo composite device managed with
>      -> virtio device     |  device_add/del command
> 
> internal "virtio device" is the only child of "virtio bus" and it's not supposed
> to be managed via device_add/del.

This makes sense; I'm wondering why virtio-bus had allow_hotplug set
before, though.

> 
> So to unplug "virtio proxy device" we should call virtio_ccw_busdev_unplug
> stored in "bridge device" with "virtual bus" using bridge as hotplug-handler.
> 
> does following patch work for you?

It does react as expected with a simple device_add/device_del (your
patches applied up to patch 19 with this one on top).

Re: [Qemu-devel] [PATCH 20/30] s390x: convert virtio-ccw to hotplug handler API

[Igor Mammedov]

On Thu, 25 Sep 2014 16:32:37 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Thu, 25 Sep 2014 15:11:10 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Thu, 25 Sep 2014 13:08:38 +0200
> > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > > So, to summarize, what happened before was
> > > 
> > > bridge device              <--- (simple unplug invoked for dev)
> > simple unplug should not exits for above device
> 
> Yes. I'm not sure why it does.
> 
> > 
> > >  -> virtual bus
> > >   -> virtio proxy device   <--- virtio unplug invoked for dev
> > 
> > >    -> virtio bus
> > >     -> virtio device
> > > 
> > > which your patch changed to
> > > 
> > > bridge device
> > >  -> virtual bus            <--- simple unplug invoked for virtio proxy dev
> > >   -> virtio proxy device
> > >    -> virtio bus           <--- (virtio unplug invoked for virtio dev)
> > >     -> virtio device
> > > 
> > > Am I understanding this correctly?
> > Let's try other way around:
> > 
> > bridge device (virtual_css_bridge) - non hotpluggable sysbus device
> >   -> virtual bus (VIRTUAL_CSS_BUS)
> >    -> virtio proxy device |
> >     -> virtio bus         |- virtio_ccw_device_foo composite device managed with
> >      -> virtio device     |  device_add/del command
> > 
> > internal "virtio device" is the only child of "virtio bus" and it's not supposed
> > to be managed via device_add/del.
> 
> This makes sense; I'm wondering why virtio-bus had allow_hotplug set
> before, though.
it was due to behavior of bus_add_child() which required bus to be hotplugggable.

> 
> > 
> > So to unplug "virtio proxy device" we should call virtio_ccw_busdev_unplug
> > stored in "bridge device" with "virtual bus" using bridge as hotplug-handler.
> > 
> > does following patch work for you?
> 
> It does react as expected with a simple device_add/device_del (your
> patches applied up to patch 19 with this one on top).
Thanks for confirmation, I'll amend s390-virtio in similar manner for V2