[Qemu-devel] [PULL 00/20] acpi,pc,pci fixes and enhancements

[Qemu-devel] [PULL 00/20] acpi,pc,pci fixes and enhancements

[Michael S. Tsirkin]

The following changes since commit 2b2449f7e467957778ca006904471b231dc0ac8e:

  Merge remote-tracking branch 'remotes/borntraeger/tags/kvm-s390-20140131' into staging (2014-02-04 18:46:33 +0000)

are available in the git repository at:


  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

for you to fetch changes up to 417c45ab2f847c0a47b1232f611aa886df6a97d5:

  ACPI: Remove commented-out code from HPET._CRS (2014-02-10 11:09:33 +0200)

----------------------------------------------------------------
acpi,pc,pci fixes and enhancements

Most changes here are hotplug related:

This merges hotplug infrastructure changes by Igor,
some acpi related fixes, and PC fixes.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

----------------------------------------------------------------
Gabriel L. Somlo (1):
      ACPI: Remove commented-out code from HPET._CRS

Igor Mammedov (14):
      pcihp: replace enable|disable_device() with oneliners
      pcihp: make PCI hotplug mmio handlers indifferent to PCI_HOTPLUG_ADDR
      pcihp: make pci_read() mmio calback compatible with legacy ACPI hotplug
      pcihp: remove unused AcpiPciHpPciStatus.device_present field
      hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug
      define hotplug interface
      qdev: add to BusState "hotplug-handler" link
      qdev: add "hotpluggable" property to Device
      hw/acpi: move typeinfo to the file end
      qdev:pci: refactor PCIDevice to use generic "hotpluggable" property
      acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-handler API
      pci/shpc: convert SHPC hotplug to use hotplug-handler API
      pci/pcie: convert PCIE hotplug to use hotplug-handler API
      hw/pci: switch to a generic hotplug handling for PCIDevice

Michael S. Tsirkin (5):
      pcihp: reduce number of device check events
      qtest: don't report signals if qtest driver enabled
      pc_piix: enable legacy hotplug for Xen
      pc.c: better error message on initrd sizing failure
      loader: document that errno is set

 include/hw/acpi/acpi.h         |   1 +
 include/hw/acpi/pcihp.h        |  17 ++-
 include/hw/hotplug.h           |  78 ++++++++++
 include/hw/loader.h            |   7 +
 include/hw/pci/pci.h           |  13 --
 include/hw/pci/pci_bus.h       |   2 -
 include/hw/pci/pcie.h          |   5 +
 include/hw/pci/shpc.h          |   8 +
 include/hw/qdev-core.h         |  15 ++
 include/sysemu/qtest.h         |   2 +
 hw/acpi/pcihp.c                | 127 +++++++++-------
 hw/acpi/piix4.c                | 322 ++++++++++-------------------------------
 hw/core/hotplug.c              |  48 ++++++
 hw/core/qdev.c                 |  50 ++++++-
 hw/display/cirrus_vga.c        |   2 +-
 hw/display/qxl.c               |   2 +-
 hw/display/vga-pci.c           |   2 +-
 hw/display/vmware_vga.c        |   2 +-
 hw/i386/acpi-build.c           |   4 +-
 hw/i386/pc.c                   |   4 +-
 hw/i386/pc_piix.c              |  11 ++
 hw/ide/piix.c                  |   4 +-
 hw/isa/piix4.c                 |   2 +-
 hw/pci-bridge/pci_bridge_dev.c |   9 ++
 hw/pci-host/piix.c             |   6 +-
 hw/pci/pci.c                   |  40 +----
 hw/pci/pcie.c                  |  65 +++++----
 hw/pci/pcie_port.c             |   8 +
 hw/pci/shpc.c                  | 124 ++++++++++------
 hw/usb/hcd-ehci-pci.c          |   2 +-
 hw/usb/hcd-ohci.c              |   2 +-
 hw/usb/hcd-uhci.c              |   2 +-
 hw/usb/hcd-xhci.c              |   2 +-
 qtest.c                        |   5 +
 vl.c                           |   2 +-
 hw/core/Makefile.objs          |   1 +
 hw/i386/acpi-dsdt-hpet.dsl     |   3 -
 tests/Makefile                 |   2 +-
 38 files changed, 540 insertions(+), 461 deletions(-)
 create mode 100644 include/hw/hotplug.h
 create mode 100644 hw/core/hotplug.c

[Qemu-devel] [PULL 01/20] pcihp: reduce number of device check events

[Michael S. Tsirkin]

PIIX created a made-up value for the UP register since it was read by
guest 32 times for each interrupt.
There's no reason to do this for the new PCIHP: register is only read
once for each interrupt, so clean up code by making read act as an
interrupt acknowledgement: the new UP register clear on read.

In this way we cut down the number of bus rescans
by a factor of 32, and drop a bunch of code that's
now unused.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/pcihp.h |  2 +-
 hw/acpi/pcihp.c         | 21 +++++----------------
 2 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index 6230e60..aa297c2 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -32,7 +32,7 @@
 #include "hw/pci/pci.h" /* for PCIHotplugState */
 
 typedef struct AcpiPciHpPciStatus {
-    uint32_t up; /* deprecated, maintained for migration compatibility */
+    uint32_t up;
     uint32_t down;
     uint32_t hotplug_enable;
     uint32_t device_present;
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 3fa3d7c..4345f5d 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -116,7 +116,6 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slo
 {
     BusChild *kid, *next;
     int slot = ffs(slots) - 1;
-    bool slot_free = true;
     PCIBus *bus = acpi_pcihp_find_hotplug_bus(s, bsel);
 
     if (!bus) {
@@ -125,21 +124,17 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slo
 
     /* Mark request as complete */
     s->acpi_pcihp_pci_status[bsel].down &= ~(1U << slot);
+    s->acpi_pcihp_pci_status[bsel].up &= ~(1U << slot);
 
     QTAILQ_FOREACH_SAFE(kid, &bus->qbus.children, sibling, next) {
         DeviceState *qdev = kid->child;
         PCIDevice *dev = PCI_DEVICE(qdev);
         if (PCI_SLOT(dev->devfn) == slot) {
-            if (acpi_pcihp_pc_no_hotplug(s, dev)) {
-                slot_free = false;
-            } else {
+            if (!acpi_pcihp_pc_no_hotplug(s, dev)) {
                 object_unparent(OBJECT(qdev));
             }
         }
     }
-    if (slot_free) {
-        s->acpi_pcihp_pci_status[bsel].device_present &= ~(1U << slot);
-    }
 }
 
 static void acpi_pcihp_update_hotplug_bus(AcpiPciHpState *s, int bsel)
@@ -153,7 +148,6 @@ static void acpi_pcihp_update_hotplug_bus(AcpiPciHpState *s, int bsel)
     }
 
     s->acpi_pcihp_pci_status[bsel].hotplug_enable = ~0;
-    s->acpi_pcihp_pci_status[bsel].device_present = 0;
 
     if (!bus) {
         return;
@@ -166,8 +160,6 @@ static void acpi_pcihp_update_hotplug_bus(AcpiPciHpState *s, int bsel)
         if (acpi_pcihp_pc_no_hotplug(s, pdev)) {
             s->acpi_pcihp_pci_status[bsel].hotplug_enable &= ~(1U << slot);
         }
-
-        s->acpi_pcihp_pci_status[bsel].device_present |= (1U << slot);
     }
 }
 
@@ -187,7 +179,7 @@ void acpi_pcihp_reset(AcpiPciHpState *s)
 
 static void enable_device(AcpiPciHpState *s, unsigned bsel, int slot)
 {
-    s->acpi_pcihp_pci_status[bsel].device_present |= (1U << slot);
+    s->acpi_pcihp_pci_status[bsel].up |= (1U << slot);
 }
 
 static void disable_device(AcpiPciHpState *s, unsigned bsel, int slot)
@@ -208,7 +200,6 @@ int acpi_pcihp_device_hotplug(AcpiPciHpState *s, PCIDevice *dev,
      * it is present on boot, no hotplug event is necessary. We do send an
      * event when the device is disabled later. */
     if (state == PCI_COLDPLUG_ENABLED) {
-        s->acpi_pcihp_pci_status[bsel].device_present |= (1U << slot);
         return 0;
     }
 
@@ -233,10 +224,8 @@ static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size)
 
     switch (addr) {
     case PCI_UP_BASE - PCI_HOTPLUG_ADDR:
-        /* Manufacture an "up" value to cause a device check on any hotplug
-         * slot with a device.  Extra device checks are harmless. */
-        val = s->acpi_pcihp_pci_status[bsel].device_present &
-            s->acpi_pcihp_pci_status[bsel].hotplug_enable;
+        val = s->acpi_pcihp_pci_status[bsel].up;
+        s->acpi_pcihp_pci_status[bsel].up = 0;
         ACPI_PCIHP_DPRINTF("pci_up_read %" PRIu32 "\n", val);
         break;
     case PCI_DOWN_BASE - PCI_HOTPLUG_ADDR:
-- 
MST

[Qemu-devel] [PULL 02/20] pcihp: replace enable|disable_device() with oneliners

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

enable_device() and disable_device() functions aren't reused anywere,
so replace them with respective oneliners at call sites.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/acpi/pcihp.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 4345f5d..464739a 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -177,16 +177,6 @@ void acpi_pcihp_reset(AcpiPciHpState *s)
     acpi_pcihp_update(s);
 }
 
-static void enable_device(AcpiPciHpState *s, unsigned bsel, int slot)
-{
-    s->acpi_pcihp_pci_status[bsel].up |= (1U << slot);
-}
-
-static void disable_device(AcpiPciHpState *s, unsigned bsel, int slot)
-{
-    s->acpi_pcihp_pci_status[bsel].down |= (1U << slot);
-}
-
 int acpi_pcihp_device_hotplug(AcpiPciHpState *s, PCIDevice *dev,
                               PCIHotplugState state)
 {
@@ -204,9 +194,9 @@ int acpi_pcihp_device_hotplug(AcpiPciHpState *s, PCIDevice *dev,
     }
 
     if (state == PCI_HOTPLUG_ENABLED) {
-        enable_device(s, bsel, slot);
+        s->acpi_pcihp_pci_status[bsel].up |= (1U << slot);
     } else {
-        disable_device(s, bsel, slot);
+        s->acpi_pcihp_pci_status[bsel].down |= (1U << slot);
     }
 
     return 0;
-- 
MST

[Qemu-devel] [PULL 03/20] pcihp: make PCI hotplug mmio handlers indifferent to PCI_HOTPLUG_ADDR

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

... removes dependency of mmio handler on PCI_HOTPLUG_ADDR.
It will be needed in case of Q35 where base could be different.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/acpi/pcihp.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 464739a..64c8cf2 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -48,11 +48,11 @@
 
 #define PCI_HOTPLUG_ADDR 0xae00
 #define PCI_HOTPLUG_SIZE 0x0014
-#define PCI_UP_BASE 0xae00
-#define PCI_DOWN_BASE 0xae04
-#define PCI_EJ_BASE 0xae08
-#define PCI_RMV_BASE 0xae0c
-#define PCI_SEL_BASE 0xae10
+#define PCI_UP_BASE 0x0000
+#define PCI_DOWN_BASE 0x0004
+#define PCI_EJ_BASE 0x0008
+#define PCI_RMV_BASE 0x000c
+#define PCI_SEL_BASE 0x0010
 
 typedef struct AcpiPciHpFind {
     int bsel;
@@ -213,24 +213,24 @@ static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size)
     }
 
     switch (addr) {
-    case PCI_UP_BASE - PCI_HOTPLUG_ADDR:
+    case PCI_UP_BASE:
         val = s->acpi_pcihp_pci_status[bsel].up;
         s->acpi_pcihp_pci_status[bsel].up = 0;
         ACPI_PCIHP_DPRINTF("pci_up_read %" PRIu32 "\n", val);
         break;
-    case PCI_DOWN_BASE - PCI_HOTPLUG_ADDR:
+    case PCI_DOWN_BASE:
         val = s->acpi_pcihp_pci_status[bsel].down;
         ACPI_PCIHP_DPRINTF("pci_down_read %" PRIu32 "\n", val);
         break;
-    case PCI_EJ_BASE - PCI_HOTPLUG_ADDR:
+    case PCI_EJ_BASE:
         /* No feature defined yet */
         ACPI_PCIHP_DPRINTF("pci_features_read %" PRIu32 "\n", val);
         break;
-    case PCI_RMV_BASE - PCI_HOTPLUG_ADDR:
+    case PCI_RMV_BASE:
         val = s->acpi_pcihp_pci_status[bsel].hotplug_enable;
         ACPI_PCIHP_DPRINTF("pci_rmv_read %" PRIu32 "\n", val);
         break;
-    case PCI_SEL_BASE - PCI_HOTPLUG_ADDR:
+    case PCI_SEL_BASE:
         val = s->hotplug_select;
         ACPI_PCIHP_DPRINTF("pci_sel_read %" PRIu32 "\n", val);
     default:
@@ -245,7 +245,7 @@ static void pci_write(void *opaque, hwaddr addr, uint64_t data,
 {
     AcpiPciHpState *s = opaque;
     switch (addr) {
-    case PCI_EJ_BASE - PCI_HOTPLUG_ADDR:
+    case PCI_EJ_BASE:
         if (s->hotplug_select >= ACPI_PCIHP_MAX_HOTPLUG_BUS) {
             break;
         }
@@ -253,7 +253,7 @@ static void pci_write(void *opaque, hwaddr addr, uint64_t data,
         ACPI_PCIHP_DPRINTF("pciej write %" HWADDR_PRIx " <== %" PRIu64 "\n",
                       addr, data);
         break;
-    case PCI_SEL_BASE - PCI_HOTPLUG_ADDR:
+    case PCI_SEL_BASE:
         s->hotplug_select = data;
         ACPI_PCIHP_DPRINTF("pcisel write %" HWADDR_PRIx " <== %" PRIu64 "\n",
                       addr, data);
-- 
MST

[Qemu-devel] [PULL 04/20] pcihp: make pci_read() mmio calback compatible with legacy ACPI hotplug

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

due to recent change introduced by:
"pcihp: reduce number of device check events"

'up' field is cleared right after it's read.
This is incompatible with legacy BIOS ACPI code
where PCNF ACPI method reads this field 32 times.

To make pci_read mmio callback compatible with legacy
'up' behavior, pcihp code will need to know in which
mode it runs add 'legacy_piix' field to AcpiPciHpState
structure and alter register behavior accordingly.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/pcihp.h | 3 ++-
 hw/acpi/pcihp.c         | 7 +++++--
 hw/acpi/piix4.c         | 3 ++-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index aa297c2..02d3ce3 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -46,10 +46,11 @@ typedef struct AcpiPciHpState {
     uint32_t hotplug_select;
     PCIBus *root;
     MemoryRegion io;
+    bool legacy_piix;
 } AcpiPciHpState;
 
 void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root,
-                     MemoryRegion *address_space_io);
+                     MemoryRegion *address_space_io, bool bridges_enabled);
 
 /* Invoke on device hotplug */
 int acpi_pcihp_device_hotplug(AcpiPciHpState *, PCIDevice *,
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 64c8cf2..974f01c 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -215,7 +215,9 @@ static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size)
     switch (addr) {
     case PCI_UP_BASE:
         val = s->acpi_pcihp_pci_status[bsel].up;
-        s->acpi_pcihp_pci_status[bsel].up = 0;
+        if (!s->legacy_piix) {
+            s->acpi_pcihp_pci_status[bsel].up = 0;
+        }
         ACPI_PCIHP_DPRINTF("pci_up_read %" PRIu32 "\n", val);
         break;
     case PCI_DOWN_BASE:
@@ -273,9 +275,10 @@ static const MemoryRegionOps acpi_pcihp_io_ops = {
 };
 
 void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus,
-                     MemoryRegion *address_space_io)
+                     MemoryRegion *address_space_io, bool bridges_enabled)
 {
     s->root= root_bus;
+    s->legacy_piix = !bridges_enabled;
     memory_region_init_io(&s->io, NULL, &acpi_pcihp_io_ops, s,
                           "acpi-pci-hotplug",
                           PCI_HOTPLUG_SIZE);
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 5d55a3c..2aedfe5 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -695,7 +695,8 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
     memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
 
     if (s->use_acpi_pci_hotplug) {
-        acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent);
+        acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent,
+                        s->use_acpi_pci_hotplug);
     } else {
         memory_region_init_io(&s->io_pci, OBJECT(s), &piix4_pci_ops, s,
                               "acpi-pci-hotplug", PCI_HOTPLUG_SIZE);
-- 
MST

[Qemu-devel] [PULL 05/20] pcihp: remove unused AcpiPciHpPciStatus.device_present field

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

Remove now unused 'device_present' field wich was obsoleted by
patch "pcihp: reduce number of device check events"

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/pcihp.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index 02d3ce3..1fd90e1 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -35,7 +35,6 @@ typedef struct AcpiPciHpPciStatus {
     uint32_t up;
     uint32_t down;
     uint32_t hotplug_enable;
-    uint32_t device_present;
 } AcpiPciHpPciStatus;
 
 #define ACPI_PCIHP_PROP_BSEL "acpi-pcihp-bsel"
-- 
MST

[Qemu-devel] [PULL 06/20] hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

reduces acpi PCI hotplug code duplication by ~200LOC

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/pcihp.h |   1 +
 hw/acpi/pcihp.c         |  23 ++++--
 hw/acpi/piix4.c         | 211 ++++--------------------------------------------
 3 files changed, 34 insertions(+), 201 deletions(-)

diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index 1fd90e1..0a90e4a 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -39,6 +39,7 @@ typedef struct AcpiPciHpPciStatus {
 
 #define ACPI_PCIHP_PROP_BSEL "acpi-pcihp-bsel"
 #define ACPI_PCIHP_MAX_HOTPLUG_BUS 256
+#define ACPI_PCIHP_BSEL_DEFAULT 0x0
 
 typedef struct AcpiPciHpState {
     AcpiPciHpPciStatus acpi_pcihp_pci_status[ACPI_PCIHP_MAX_HOTPLUG_BUS];
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 974f01c..1ce6fc2 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -46,8 +46,9 @@
 # define ACPI_PCIHP_DPRINTF(format, ...)     do { } while (0)
 #endif
 
-#define PCI_HOTPLUG_ADDR 0xae00
-#define PCI_HOTPLUG_SIZE 0x0014
+#define ACPI_PCIHP_ADDR 0xae00
+#define ACPI_PCIHP_SIZE 0x0014
+#define ACPI_PCIHP_LEGACY_SIZE 0x000f
 #define PCI_UP_BASE 0x0000
 #define PCI_DOWN_BASE 0x0004
 #define PCI_EJ_BASE 0x0008
@@ -277,12 +278,24 @@ static const MemoryRegionOps acpi_pcihp_io_ops = {
 void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus,
                      MemoryRegion *address_space_io, bool bridges_enabled)
 {
+    uint16_t io_size = ACPI_PCIHP_SIZE;
+
     s->root= root_bus;
     s->legacy_piix = !bridges_enabled;
+
+    if (s->legacy_piix) {
+        unsigned *bus_bsel = g_malloc(sizeof *bus_bsel);
+
+        io_size = ACPI_PCIHP_LEGACY_SIZE;
+
+        *bus_bsel = ACPI_PCIHP_BSEL_DEFAULT;
+        object_property_add_uint32_ptr(OBJECT(root_bus), ACPI_PCIHP_PROP_BSEL,
+                                       bus_bsel, NULL);
+    }
+
     memory_region_init_io(&s->io, NULL, &acpi_pcihp_io_ops, s,
-                          "acpi-pci-hotplug",
-                          PCI_HOTPLUG_SIZE);
-    memory_region_add_subregion(address_space_io, PCI_HOTPLUG_ADDR, &s->io);
+                          "acpi-pci-hotplug", io_size);
+    memory_region_add_subregion(address_space_io, ACPI_PCIHP_ADDR, &s->io);
 }
 
 const VMStateDescription vmstate_acpi_pcihp_pci_status = {
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 2aedfe5..7a0efcb 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -44,13 +44,6 @@
 #define GPE_BASE 0xafe0
 #define GPE_LEN 4
 
-#define PCI_HOTPLUG_ADDR 0xae00
-#define PCI_HOTPLUG_SIZE 0x000f
-#define PCI_UP_BASE 0xae00
-#define PCI_DOWN_BASE 0xae04
-#define PCI_EJ_BASE 0xae08
-#define PCI_RMV_BASE 0xae0c
-
 #define PIIX4_PCI_HOTPLUG_STATUS 2
 
 struct pci_status {
@@ -80,13 +73,6 @@ typedef struct PIIX4PMState {
     Notifier machine_ready;
     Notifier powerdown_notifier;
 
-    /* for legacy pci hotplug (compatible with qemu 1.6 and older) */
-    MemoryRegion io_pci;
-    struct pci_status pci0_status;
-    uint32_t pci0_hotplug_enable;
-    uint32_t pci0_slot_device_present;
-
-    /* for new pci hotplug (with PCI2PCI bridge support) */
     AcpiPciHpState acpi_pci_hotplug;
     bool use_acpi_pci_hotplug;
 
@@ -170,17 +156,6 @@ static void pm_write_config(PCIDevice *d,
     }
 }
 
-static void vmstate_pci_status_pre_save(void *opaque)
-{
-    struct pci_status *pci0_status = opaque;
-    PIIX4PMState *s = container_of(pci0_status, PIIX4PMState, pci0_status);
-
-    /* We no longer track up, so build a safe value for migrating
-     * to a version that still does... of course these might get lost
-     * by an old buggy implementation, but we try. */
-    pci0_status->up = s->pci0_slot_device_present & s->pci0_hotplug_enable;
-}
-
 static int vmstate_acpi_post_load(void *opaque, int version_id)
 {
     PIIX4PMState *s = opaque;
@@ -216,10 +191,9 @@ static const VMStateDescription vmstate_pci_status = {
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
-    .pre_save = vmstate_pci_status_pre_save,
     .fields      = (VMStateField []) {
-        VMSTATE_UINT32(up, struct pci_status),
-        VMSTATE_UINT32(down, struct pci_status),
+        VMSTATE_UINT32(up, struct AcpiPciHpPciStatus),
+        VMSTATE_UINT32(down, struct AcpiPciHpPciStatus),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -256,7 +230,8 @@ static int acpi_load_old(QEMUFile *f, void *opaque, int version_id)
         qemu_get_be16s(f, &temp);
     }
 
-    ret = vmstate_load_state(f, &vmstate_pci_status, &s->pci0_status, 1);
+    ret = vmstate_load_state(f, &vmstate_pci_status,
+        &s->acpi_pci_hotplug.acpi_pcihp_pci_status[ACPI_PCIHP_BSEL_DEFAULT], 1);
     return ret;
 }
 
@@ -294,70 +269,18 @@ static const VMStateDescription vmstate_acpi = {
         VMSTATE_TIMER(ar.tmr.timer, PIIX4PMState),
         VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
         VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
-        VMSTATE_STRUCT_TEST(pci0_status, PIIX4PMState,
-                            vmstate_test_no_use_acpi_pci_hotplug,
-                            2, vmstate_pci_status,
-                            struct pci_status),
+        VMSTATE_STRUCT_TEST(
+            acpi_pci_hotplug.acpi_pcihp_pci_status[ACPI_PCIHP_BSEL_DEFAULT],
+            PIIX4PMState,
+            vmstate_test_no_use_acpi_pci_hotplug,
+            2, vmstate_pci_status,
+            struct AcpiPciHpPciStatus),
         VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
                             vmstate_test_use_acpi_pci_hotplug),
         VMSTATE_END_OF_LIST()
     }
 };
 
-static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
-{
-    BusChild *kid, *next;
-    BusState *bus = qdev_get_parent_bus(DEVICE(s));
-    int slot = ffs(slots) - 1;
-    bool slot_free = true;
-
-    /* Mark request as complete */
-    s->pci0_status.down &= ~(1U << slot);
-
-    QTAILQ_FOREACH_SAFE(kid, &bus->children, sibling, next) {
-        DeviceState *qdev = kid->child;
-        PCIDevice *dev = PCI_DEVICE(qdev);
-        PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
-        if (PCI_SLOT(dev->devfn) == slot) {
-            if (pc->no_hotplug) {
-                slot_free = false;
-            } else {
-                object_unparent(OBJECT(qdev));
-            }
-        }
-    }
-    if (slot_free) {
-        s->pci0_slot_device_present &= ~(1U << slot);
-    }
-}
-
-static void piix4_update_hotplug(PIIX4PMState *s)
-{
-    BusState *bus = qdev_get_parent_bus(DEVICE(s));
-    BusChild *kid, *next;
-
-    /* Execute any pending removes during reset */
-    while (s->pci0_status.down) {
-        acpi_piix_eject_slot(s, s->pci0_status.down);
-    }
-
-    s->pci0_hotplug_enable = ~0;
-    s->pci0_slot_device_present = 0;
-
-    QTAILQ_FOREACH_SAFE(kid, &bus->children, sibling, next) {
-        DeviceState *qdev = kid->child;
-        PCIDevice *pdev = PCI_DEVICE(qdev);
-        PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pdev);
-        int slot = PCI_SLOT(pdev->devfn);
-
-        if (pc->no_hotplug) {
-            s->pci0_hotplug_enable &= ~(1U << slot);
-        }
-
-        s->pci0_slot_device_present |= (1U << slot);
-    }
-}
-
 static void piix4_reset(void *opaque)
 {
     PIIX4PMState *s = opaque;
@@ -377,11 +300,7 @@ static void piix4_reset(void *opaque)
         pci_conf[0x5B] = 0x02;
     }
     pm_io_space_update(s);
-    if (s->use_acpi_pci_hotplug) {
-        acpi_pcihp_reset(&s->acpi_pci_hotplug);
-    } else {
-        piix4_update_hotplug(s);
-    }
+    acpi_pcihp_reset(&s->acpi_pci_hotplug);
 }
 
 static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
@@ -428,6 +347,8 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
 
     if (s->use_acpi_pci_hotplug) {
         pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
+    } else {
+        piix4_update_bus_hotplug(d->bus, s);
     }
 }
 
@@ -621,60 +542,6 @@ static const MemoryRegionOps piix4_gpe_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size)
-{
-    PIIX4PMState *s = opaque;
-    uint32_t val = 0;
-
-    switch (addr) {
-    case PCI_UP_BASE - PCI_HOTPLUG_ADDR:
-        /* Manufacture an "up" value to cause a device check on any hotplug
-         * slot with a device.  Extra device checks are harmless. */
-        val = s->pci0_slot_device_present & s->pci0_hotplug_enable;
-        PIIX4_DPRINTF("pci_up_read %" PRIu32 "\n", val);
-        break;
-    case PCI_DOWN_BASE - PCI_HOTPLUG_ADDR:
-        val = s->pci0_status.down;
-        PIIX4_DPRINTF("pci_down_read %" PRIu32 "\n", val);
-        break;
-    case PCI_EJ_BASE - PCI_HOTPLUG_ADDR:
-        /* No feature defined yet */
-        PIIX4_DPRINTF("pci_features_read %" PRIu32 "\n", val);
-        break;
-    case PCI_RMV_BASE - PCI_HOTPLUG_ADDR:
-        val = s->pci0_hotplug_enable;
-        break;
-    default:
-        break;
-    }
-
-    return val;
-}
-
-static void pci_write(void *opaque, hwaddr addr, uint64_t data,
-                      unsigned int size)
-{
-    switch (addr) {
-    case PCI_EJ_BASE - PCI_HOTPLUG_ADDR:
-        acpi_piix_eject_slot(opaque, (uint32_t)data);
-        PIIX4_DPRINTF("pciej write %" HWADDR_PRIx " <== %" PRIu64 "\n",
-                      addr, data);
-        break;
-    default:
-        break;
-    }
-}
-
-static const MemoryRegionOps piix4_pci_ops = {
-    .read = pci_read,
-    .write = pci_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-    .valid = {
-        .min_access_size = 4,
-        .max_access_size = 4,
-    },
-};
-
 static void piix4_cpu_added_req(Notifier *n, void *opaque)
 {
     PIIX4PMState *s = container_of(n, PIIX4PMState, cpu_added_notifier);
@@ -684,9 +551,6 @@ static void piix4_cpu_added_req(Notifier *n, void *opaque)
     acpi_update_sci(&s->ar, s->irq);
 }
 
-static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
-                                PCIHotplugState state);
-
 static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
                                            PCIBus *bus, PIIX4PMState *s)
 {
@@ -694,56 +558,11 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
                           "acpi-gpe0", GPE_LEN);
     memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
 
-    if (s->use_acpi_pci_hotplug) {
-        acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent,
-                        s->use_acpi_pci_hotplug);
-    } else {
-        memory_region_init_io(&s->io_pci, OBJECT(s), &piix4_pci_ops, s,
-                              "acpi-pci-hotplug", PCI_HOTPLUG_SIZE);
-        memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR,
-                                    &s->io_pci);
-        pci_bus_hotplug(bus, piix4_device_hotplug, DEVICE(s));
-    }
+    acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent,
+                    s->use_acpi_pci_hotplug);
 
     AcpiCpuHotplug_init(parent, OBJECT(s), &s->gpe_cpu,
                         PIIX4_CPU_HOTPLUG_IO_BASE);
     s->cpu_added_notifier.notify = piix4_cpu_added_req;
     qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
 }
-
-static void enable_device(PIIX4PMState *s, int slot)
-{
-    s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
-    s->pci0_slot_device_present |= (1U << slot);
-}
-
-static void disable_device(PIIX4PMState *s, int slot)
-{
-    s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
-    s->pci0_status.down |= (1U << slot);
-}
-
-static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
-				PCIHotplugState state)
-{
-    int slot = PCI_SLOT(dev->devfn);
-    PIIX4PMState *s = PIIX4_PM(qdev);
-
-    /* Don't send event when device is enabled during qemu machine creation:
-     * it is present on boot, no hotplug event is necessary. We do send an
-     * event when the device is disabled later. */
-    if (state == PCI_COLDPLUG_ENABLED) {
-        s->pci0_slot_device_present |= (1U << slot);
-        return 0;
-    }
-
-    if (state == PCI_HOTPLUG_ENABLED) {
-        enable_device(s, slot);
-    } else {
-        disable_device(s, slot);
-    }
-
-    acpi_update_sci(&s->ar, s->irq);
-
-    return 0;
-}
-- 
MST

[Qemu-devel] [PULL 07/20] qtest: don't report signals if qtest driver enabled

[Michael S. Tsirkin]

qtest driver always uses signals to kill qemu
no need to report it, whatever the accelerator state.

Add API to detect qtest driver, and suppress reporting
signals in this case.

Reported-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/sysemu/qtest.h | 2 ++
 qtest.c                | 5 +++++
 vl.c                   | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/qtest.h b/include/sysemu/qtest.h
index 112a661..6aca8e4 100644
--- a/include/sysemu/qtest.h
+++ b/include/sysemu/qtest.h
@@ -23,6 +23,8 @@ static inline bool qtest_enabled(void)
     return qtest_allowed;
 }
 
+bool qtest_driver(void);
+
 int qtest_init_accel(void);
 void qtest_init(const char *qtest_chrdev, const char *qtest_log);
 
diff --git a/qtest.c b/qtest.c
index dcf1301..a738afc 100644
--- a/qtest.c
+++ b/qtest.c
@@ -528,3 +528,8 @@ void qtest_init(const char *qtest_chrdev, const char *qtest_log)
 
     qtest_chr = chr;
 }
+
+bool qtest_driver(void)
+{
+    return qtest_chr;
+}
diff --git a/vl.c b/vl.c
index 383be1b..a7b00cd 100644
--- a/vl.c
+++ b/vl.c
@@ -1750,7 +1750,7 @@ static int qemu_shutdown_requested(void)
 
 static void qemu_kill_report(void)
 {
-    if (!qtest_enabled() && shutdown_signal != -1) {
+    if (!qtest_driver() && shutdown_signal != -1) {
         fprintf(stderr, "qemu: terminating on signal %d", shutdown_signal);
         if (shutdown_pid == 0) {
             /* This happens for eg ^C at the terminal, so it's worth
-- 
MST

[Qemu-devel] [PULL 08/20] pc_piix: enable legacy hotplug for Xen

[Michael S. Tsirkin]

xenfv has no fwcfg and so does not load acpi from QEMU.
as such new acpi features don't work.

Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/pc_piix.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a327d71..1acd2b2 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -793,6 +793,17 @@ static QEMUMachine xenfv_machine = {
     .max_cpus = HVM_MAX_VCPUS,
     .default_machine_opts = "accel=xen",
     .hot_add_cpu = pc_hot_add_cpu,
+    .compat_props = (GlobalProperty[]) {
+        /* xenfv has no fwcfg and so does not load acpi from QEMU.
+         * as such new acpi features don't work.
+         */
+        {
+            .driver   = "PIIX4_PM",
+            .property = "acpi-pci-hotplug-with-bridge-support",
+            .value    = "off",
+        },
+        { /* end of list */ }
+    },
 };
 #endif
 
-- 
MST

[Qemu-devel] [PULL 09/20] pc.c: better error message on initrd sizing failure

[Michael S. Tsirkin]

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/pc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 348b15f..e715a33 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -835,8 +835,8 @@ static void load_linux(FWCfgState *fw_cfg,
 
         initrd_size = get_image_size(initrd_filename);
         if (initrd_size < 0) {
-            fprintf(stderr, "qemu: error reading initrd %s\n",
-                    initrd_filename);
+            fprintf(stderr, "qemu: error reading initrd %s: %s\n",
+                    initrd_filename, strerror(errno));
             exit(1);
         }
 
-- 
MST

[Qemu-devel] [PULL 10/20] loader: document that errno is set

[Michael S. Tsirkin]

Document that get_image_size sets errno
on failure.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/loader.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/hw/loader.h b/include/hw/loader.h
index 7a23d6b..91b0122 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -4,6 +4,13 @@
 #include "hw/nvram/fw_cfg.h"
 
 /* loader.c */
+/**
+ * get_image_size: retrieve size of an image file
+ * @filename: Path to the image file
+ *
+ * Returns the size of the image file on success, -1 otherwise.
+ * On error, errno is also set as appropriate.
+ */
 int get_image_size(const char *filename);
 int load_image(const char *filename, uint8_t *addr); /* deprecated */
 int load_image_targphys(const char *filename, hwaddr,
-- 
MST

[Qemu-devel] [PULL 11/20] define hotplug interface

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

Provide a generic hotplug interface for hotplug handlers.
Intended for replacing hotplug mechanism used by
PCI/PCIE/SHPC code and will be used for memory hotplug.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/hotplug.h  | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/core/hotplug.c     | 48 +++++++++++++++++++++++++++++++
 hw/core/Makefile.objs |  1 +
 3 files changed, 127 insertions(+)
 create mode 100644 include/hw/hotplug.h
 create mode 100644 hw/core/hotplug.c

diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
new file mode 100644
index 0000000..a6533cb
--- /dev/null
+++ b/include/hw/hotplug.h
@@ -0,0 +1,78 @@
+/*
+ * Hotplug handler interface.
+ *
+ * Copyright (c) 2014 Red Hat Inc.
+ *
+ * Authors:
+ *  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.
+ */
+#ifndef HOTPLUG_H
+#define HOTPLUG_H
+
+#include "qom/object.h"
+#include "qemu/typedefs.h"
+
+#define TYPE_HOTPLUG_HANDLER "hotplug-handler"
+
+#define HOTPLUG_HANDLER_CLASS(klass) \
+     OBJECT_CLASS_CHECK(HotplugHandlerClass, (klass), TYPE_HOTPLUG_HANDLER)
+#define HOTPLUG_HANDLER_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(HotplugHandlerClass, (obj), TYPE_HOTPLUG_HANDLER)
+#define HOTPLUG_HANDLER(obj) \
+     INTERFACE_CHECK(HotplugHandler, (obj), TYPE_HOTPLUG_HANDLER)
+
+
+typedef struct HotplugHandler {
+    /* <private> */
+    Object Parent;
+} HotplugHandler;
+
+/**
+ * hotplug_fn:
+ * @plug_handler: a device performing plug/uplug action
+ * @plugged_dev: a device that has been (un)plugged
+ * @errp: returns an error if this function fails
+ */
+typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
+                           DeviceState *plugged_dev, Error **errp);
+
+/**
+ * HotplugDeviceClass:
+ *
+ * Interface to be implemented by a device performing
+ * hardware (un)plug functions.
+ *
+ * @parent: Opaque parent interface.
+ * @plug: plug callback.
+ * @unplug: unplug callback.
+ */
+typedef struct HotplugHandlerClass {
+    /* <private> */
+    InterfaceClass parent;
+
+    /* <public> */
+    hotplug_fn plug;
+    hotplug_fn unplug;
+} HotplugHandlerClass;
+
+/**
+ * hotplug_handler_plug:
+ *
+ * Call #HotplugHandlerClass.plug callback of @plug_handler.
+ */
+void hotplug_handler_plug(HotplugHandler *plug_handler,
+                          DeviceState *plugged_dev,
+                          Error **errp);
+
+/**
+ * hotplug_handler_unplug:
+ *
+ * Call #HotplugHandlerClass.unplug callback of @plug_handler.
+ */
+void hotplug_handler_unplug(HotplugHandler *plug_handler,
+                            DeviceState *plugged_dev,
+                            Error **errp);
+#endif
diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
new file mode 100644
index 0000000..5573d9d
--- /dev/null
+++ b/hw/core/hotplug.c
@@ -0,0 +1,48 @@
+/*
+ * Hotplug handler interface.
+ *
+ * Copyright (c) 2014 Red Hat Inc.
+ *
+ * Authors:
+ *  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 "hw/hotplug.h"
+#include "qemu/module.h"
+
+void hotplug_handler_plug(HotplugHandler *plug_handler,
+                          DeviceState *plugged_dev,
+                          Error **errp)
+{
+    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
+
+    if (hdc->plug) {
+        hdc->plug(plug_handler, plugged_dev, errp);
+    }
+}
+
+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,
+    .class_size = sizeof(HotplugHandlerClass),
+};
+
+static void hotplug_handler_register_types(void)
+{
+    type_register_static(&hotplug_handler_info);
+}
+
+type_init(hotplug_handler_register_types)
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index 950146c..9e324be 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -2,6 +2,7 @@
 common-obj-y += qdev.o qdev-properties.o
 # irq.o needed for qdev GPIO handling:
 common-obj-y += irq.o
+common-obj-y += hotplug.o
 
 common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
 common-obj-$(CONFIG_XILINX_AXI) += stream.o
-- 
MST

[Qemu-devel] [PULL 12/20] qdev: add to BusState "hotplug-handler" link

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

It will allow to reuse field with different BUSes,
reducing code duplication. Field is intended for
replacing 'hotplug_qdev' field in PCIBus and also
will allow to avoid adding equivalent field to
DimmBus with possiblitity to refactor other BUSes
to use it instead of custom field.
In addition once all users of allow_hotplug field
are converted to new API, link could replace
allow_hotplug field in qdev hotplug code.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/qdev-core.h | 12 ++++++++++++
 hw/core/qdev.c         |  4 ++++
 2 files changed, 16 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 2c4f140..41ec533 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -8,6 +8,7 @@
 #include "qom/object.h"
 #include "hw/irq.h"
 #include "qapi/error.h"
+#include "hw/hotplug.h"
 
 enum {
     DEV_NVECTORS_UNSPECIFIED = -1,
@@ -180,14 +181,18 @@ typedef struct BusChild {
     QTAILQ_ENTRY(BusChild) sibling;
 } BusChild;
 
+#define QDEV_HOTPLUG_HANDLER_PROPERTY "hotplug-handler"
+
 /**
  * BusState:
+ * @hotplug_device: link to a hotplug device associated with bus.
  */
 struct BusState {
     Object obj;
     DeviceState *parent;
     const char *name;
     int allow_hotplug;
+    HotplugHandler *hotplug_handler;
     int max_index;
     QTAILQ_HEAD(ChildrenHead, BusChild) children;
     QLIST_ENTRY(BusState) sibling;
@@ -321,4 +326,11 @@ extern int qdev_hotplug;
 
 char *qdev_get_dev_path(DeviceState *dev);
 
+static inline void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler,
+                                            Error **errp)
+{
+    object_property_set_link(OBJECT(bus), OBJECT(handler),
+                             QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
+    bus->allow_hotplug = 1;
+}
 #endif
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 82a9123..c9f0c33 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -32,6 +32,7 @@
 #include "qapi/visitor.h"
 #include "qapi/qmp/qjson.h"
 #include "monitor/monitor.h"
+#include "hw/hotplug.h"
 
 int qdev_hotplug = 0;
 static bool qdev_hot_added = false;
@@ -870,6 +871,9 @@ static void qbus_initfn(Object *obj)
     BusState *bus = BUS(obj);
 
     QTAILQ_INIT(&bus->children);
+    object_property_add_link(obj, QDEV_HOTPLUG_HANDLER_PROPERTY,
+                             TYPE_HOTPLUG_HANDLER,
+                             (Object **)&bus->hotplug_handler, NULL);
 }
 
 static char *default_bus_get_fw_dev_path(DeviceState *dev)
-- 
MST

[Qemu-devel] [PULL 13/20] qdev: add "hotpluggable" property to Device

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

Currently it's possible to make PCIDevice not hotpluggable
by using no_hotplug field of PCIDeviceClass. However it
limits this only to PCI devices and prevents from
generalizing hotplug code.

So add similar field to DeviceClass so it could be reused
with other Devices and would allow to replace PCI specific
hotplug callbacks with generic implementation. Following
patches will replace PCIDeviceClass.no_hotplug with this
new property.

In addition expose field as "hotpluggable" readonly property,
to make it possible to read its value via QOM interface.

Make DeviceClass hotpluggable by default as it was assumed
before.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/qdev-core.h |  3 +++
 hw/core/qdev.c         | 29 +++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 41ec533..08d329d 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -50,6 +50,8 @@ struct VMStateDescription;
  * is changed to %true. Deprecated, new types inheriting directly from
  * TYPE_DEVICE should use @realize instead, new leaf types should consult
  * their respective parent type.
+ * @hotpluggable: indicates if #DeviceClass is hotpluggable, available
+ * as readonly "hotpluggable" property of #DeviceState instance
  *
  * # Realization #
  * Devices are constructed in two stages,
@@ -110,6 +112,7 @@ typedef struct DeviceClass {
      * TODO remove once we're there
      */
     bool cannot_instantiate_with_device_add_yet;
+    bool hotpluggable;
 
     /* callbacks */
     void (*reset)(DeviceState *dev);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index c9f0c33..5c864db 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -215,6 +215,12 @@ void qdev_unplug(DeviceState *dev, Error **errp)
     }
     assert(dc->unplug != NULL);
 
+    if (!dc->hotpluggable) {
+        error_set(errp, QERR_DEVICE_NO_HOTPLUG,
+                  object_get_typename(OBJECT(dev)));
+        return;
+    }
+
     qdev_hot_removed = true;
 
     if (dc->unplug(dev) < 0) {
@@ -694,6 +700,11 @@ static void device_set_realized(Object *obj, bool value, Error **err)
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
     Error *local_err = NULL;
 
+    if (dev->hotplugged && !dc->hotpluggable) {
+        error_set(err, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
+        return;
+    }
+
     if (value && !dev->realized) {
         if (!obj->parent && local_err == NULL) {
             static int unattached_count;
@@ -734,6 +745,14 @@ static void device_set_realized(Object *obj, bool value, Error **err)
     dev->realized = value;
 }
 
+static bool device_get_hotpluggable(Object *obj, Error **err)
+{
+    DeviceClass *dc = DEVICE_GET_CLASS(obj);
+    DeviceState *dev = DEVICE(obj);
+
+    return dc->hotpluggable && dev->parent_bus->allow_hotplug;
+}
+
 static void device_initfn(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
@@ -750,6 +769,8 @@ static void device_initfn(Object *obj)
 
     object_property_add_bool(obj, "realized",
                              device_get_realized, device_set_realized, NULL);
+    object_property_add_bool(obj, "hotpluggable",
+                             device_get_hotpluggable, NULL, NULL);
 
     class = object_get_class(OBJECT(dev));
     do {
@@ -786,6 +807,14 @@ static void device_class_base_init(ObjectClass *class, void *data)
      * so do not propagate them to the subclasses.
      */
     klass->props = NULL;
+
+    /* by default all devices were considered as hotpluggable,
+     * so with intent to check it in generic qdev_unplug() /
+     * device_set_realized() functions make every device
+     * hotpluggable. Devices that shouldn't be hotpluggable,
+     * should override it in their class_init()
+     */
+    klass->hotpluggable = true;
 }
 
 static void device_unparent(Object *obj)
-- 
MST

[Qemu-devel] [PULL 14/20] hw/acpi: move typeinfo to the file end

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

do so to avoid not necessary forward declarations and
place typeinfo registration at the file end where it's
usually expected.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/acpi/piix4.c | 92 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 46 insertions(+), 46 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 7a0efcb..0f45e11 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -466,52 +466,6 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
     return s->smb.smbus;
 }
 
-static Property piix4_pm_properties[] = {
-    DEFINE_PROP_UINT32("smb_io_base", PIIX4PMState, smb_io_base, 0),
-    DEFINE_PROP_UINT8(ACPI_PM_PROP_S3_DISABLED, PIIX4PMState, disable_s3, 0),
-    DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_DISABLED, PIIX4PMState, disable_s4, 0),
-    DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
-    DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState,
-                     use_acpi_pci_hotplug, true),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void piix4_pm_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    k->no_hotplug = 1;
-    k->init = piix4_pm_initfn;
-    k->config_write = pm_write_config;
-    k->vendor_id = PCI_VENDOR_ID_INTEL;
-    k->device_id = PCI_DEVICE_ID_INTEL_82371AB_3;
-    k->revision = 0x03;
-    k->class_id = PCI_CLASS_BRIDGE_OTHER;
-    dc->desc = "PM";
-    dc->vmsd = &vmstate_acpi;
-    dc->props = piix4_pm_properties;
-    /*
-     * Reason: part of PIIX4 southbridge, needs to be wired up,
-     * e.g. by mips_malta_init()
-     */
-    dc->cannot_instantiate_with_device_add_yet = true;
-}
-
-static const TypeInfo piix4_pm_info = {
-    .name          = TYPE_PIIX4_PM,
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(PIIX4PMState),
-    .class_init    = piix4_pm_class_init,
-};
-
-static void piix4_pm_register_types(void)
-{
-    type_register_static(&piix4_pm_info);
-}
-
-type_init(piix4_pm_register_types)
-
 static uint64_t gpe_readb(void *opaque, hwaddr addr, unsigned width)
 {
     PIIX4PMState *s = opaque;
@@ -566,3 +520,49 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
     s->cpu_added_notifier.notify = piix4_cpu_added_req;
     qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
 }
+
+static Property piix4_pm_properties[] = {
+    DEFINE_PROP_UINT32("smb_io_base", PIIX4PMState, smb_io_base, 0),
+    DEFINE_PROP_UINT8(ACPI_PM_PROP_S3_DISABLED, PIIX4PMState, disable_s3, 0),
+    DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_DISABLED, PIIX4PMState, disable_s4, 0),
+    DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
+    DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState,
+                     use_acpi_pci_hotplug, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void piix4_pm_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->no_hotplug = 1;
+    k->init = piix4_pm_initfn;
+    k->config_write = pm_write_config;
+    k->vendor_id = PCI_VENDOR_ID_INTEL;
+    k->device_id = PCI_DEVICE_ID_INTEL_82371AB_3;
+    k->revision = 0x03;
+    k->class_id = PCI_CLASS_BRIDGE_OTHER;
+    dc->desc = "PM";
+    dc->vmsd = &vmstate_acpi;
+    dc->props = piix4_pm_properties;
+    /*
+     * Reason: part of PIIX4 southbridge, needs to be wired up,
+     * e.g. by mips_malta_init()
+     */
+    dc->cannot_instantiate_with_device_add_yet = true;
+}
+
+static const TypeInfo piix4_pm_info = {
+    .name          = TYPE_PIIX4_PM,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PIIX4PMState),
+    .class_init    = piix4_pm_class_init,
+};
+
+static void piix4_pm_register_types(void)
+{
+    type_register_static(&piix4_pm_info);
+}
+
+type_init(piix4_pm_register_types)
-- 
MST

[Qemu-devel] [PULL 15/20] qdev:pci: refactor PCIDevice to use generic "hotpluggable" property

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

Get rid of PCIDevice specific PCIDeviceClass.no_hotplug and use
generic DeviceClass.hotpluggable field instead.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/pci/pci.h    |  3 ---
 hw/acpi/pcihp.c         |  3 ++-
 hw/acpi/piix4.c         |  2 +-
 hw/display/cirrus_vga.c |  2 +-
 hw/display/qxl.c        |  2 +-
 hw/display/vga-pci.c    |  2 +-
 hw/display/vmware_vga.c |  2 +-
 hw/i386/acpi-build.c    |  4 +++-
 hw/ide/piix.c           |  4 ++--
 hw/isa/piix4.c          |  2 +-
 hw/pci-host/piix.c      |  6 +++---
 hw/pci/pci.c            | 11 +----------
 hw/usb/hcd-ehci-pci.c   |  2 +-
 hw/usb/hcd-ohci.c       |  2 +-
 hw/usb/hcd-uhci.c       |  2 +-
 hw/usb/hcd-xhci.c       |  2 +-
 16 files changed, 21 insertions(+), 30 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 5252346..c173b6a 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -201,9 +201,6 @@ typedef struct PCIDeviceClass {
     /* pcie stuff */
     int is_express;   /* is this device pci express? */
 
-    /* device isn't hot-pluggable */
-    int no_hotplug;
-
     /* rom bar */
     const char *romfile;
 } PCIDeviceClass;
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 1ce6fc2..3bd5a06 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -105,12 +105,13 @@ static PCIBus *acpi_pcihp_find_hotplug_bus(AcpiPciHpState *s, int bsel)
 static bool acpi_pcihp_pc_no_hotplug(AcpiPciHpState *s, PCIDevice *dev)
 {
     PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
+    DeviceClass *dc = DEVICE_GET_CLASS(dev);
     /*
      * ACPI doesn't allow hotplug of bridge devices.  Don't allow
      * hot-unplug of bridge devices unless they were added by hotplug
      * (and so, not described by acpi).
      */
-    return (pc->is_bridge && !dev->qdev.hotplugged) || pc->no_hotplug;
+    return (pc->is_bridge && !dev->qdev.hotplugged) || !dc->hotpluggable;
 }
 
 static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slots)
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 0f45e11..077776a 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -536,7 +536,6 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = piix4_pm_initfn;
     k->config_write = pm_write_config;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
@@ -551,6 +550,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
      * e.g. by mips_malta_init()
      */
     dc->cannot_instantiate_with_device_add_yet = true;
+    dc->hotpluggable = false;
 }
 
 static const TypeInfo piix4_pm_info = {
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index e4c345f..3a8fc0b 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -2996,7 +2996,6 @@ static void cirrus_vga_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = pci_cirrus_vga_initfn;
     k->romfile = VGABIOS_CIRRUS_FILENAME;
     k->vendor_id = PCI_VENDOR_ID_CIRRUS;
@@ -3006,6 +3005,7 @@ static void cirrus_vga_class_init(ObjectClass *klass, void *data)
     dc->desc = "Cirrus CLGD 54xx VGA";
     dc->vmsd = &vmstate_pci_cirrus_vga;
     dc->props = pci_vga_cirrus_properties;
+    dc->hotpluggable = false;
 }
 
 static const TypeInfo cirrus_vga_info = {
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index e4f172e..ec82e00 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2299,7 +2299,6 @@ static void qxl_primary_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = qxl_init_primary;
     k->romfile = "vgabios-qxl.bin";
     k->vendor_id = REDHAT_PCI_VENDOR_ID;
@@ -2310,6 +2309,7 @@ static void qxl_primary_class_init(ObjectClass *klass, void *data)
     dc->reset = qxl_reset_handler;
     dc->vmsd = &qxl_vmstate;
     dc->props = qxl_properties;
+    dc->hotpluggable = false;
 }
 
 static const TypeInfo qxl_primary_info = {
diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
index b3a45c8..f74fc43 100644
--- a/hw/display/vga-pci.c
+++ b/hw/display/vga-pci.c
@@ -190,7 +190,6 @@ static void vga_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = pci_std_vga_initfn;
     k->romfile = "vgabios-stdvga.bin";
     k->vendor_id = PCI_VENDOR_ID_QEMU;
@@ -198,6 +197,7 @@ static void vga_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_DISPLAY_VGA;
     dc->vmsd = &vmstate_vga_pci;
     dc->props = vga_pci_properties;
+    dc->hotpluggable = false;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
 }
 
diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index aba292c..334e718 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1296,7 +1296,6 @@ static void vmsvga_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = pci_vmsvga_initfn;
     k->romfile = "vgabios-vmware.bin";
     k->vendor_id = PCI_VENDOR_ID_VMWARE;
@@ -1307,6 +1306,7 @@ static void vmsvga_class_init(ObjectClass *klass, void *data)
     dc->reset = vmsvga_reset;
     dc->vmsd = &vmstate_vmware_vga;
     dc->props = vga_vmware_properties;
+    dc->hotpluggable = false;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
 }
 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 50e83f3..b1a7ebb 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -768,6 +768,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
         memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
 
         for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
+            DeviceClass *dc;
             PCIDeviceClass *pc;
             PCIDevice *pdev = bus->devices[i];
 
@@ -776,8 +777,9 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
             }
 
             pc = PCI_DEVICE_GET_CLASS(pdev);
+            dc = DEVICE_GET_CLASS(pdev);
 
-            if (pc->no_hotplug || pc->is_bridge) {
+            if (!dc->hotpluggable || pc->is_bridge) {
                 int slot = PCI_SLOT(i);
 
                 clear_bit(slot, slot_hotplug_enable);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 9b5960b..0eda301 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -241,13 +241,13 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = pci_piix_ide_initfn;
     k->exit = pci_piix_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
     k->class_id = PCI_CLASS_STORAGE_IDE;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+    dc->hotpluggable = false;
 }
 
 static const TypeInfo piix3_ide_info = {
@@ -280,13 +280,13 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = pci_piix_ide_initfn;
     k->exit = pci_piix_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82371AB;
     k->class_id = PCI_CLASS_STORAGE_IDE;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+    dc->hotpluggable = false;
 }
 
 static const TypeInfo piix4_ide_info = {
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index def6fe3..492cd22 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -107,7 +107,6 @@ static void piix4_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = piix4_initfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82371AB_0;
@@ -119,6 +118,7 @@ static void piix4_class_init(ObjectClass *klass, void *data)
      * e.g. by mips_malta_init()
      */
     dc->cannot_instantiate_with_device_add_yet = true;
+    dc->hotpluggable = false;
 }
 
 static const TypeInfo piix4_info = {
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index e89d5c1..ffdc853 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -628,7 +628,7 @@ static void piix3_class_init(ObjectClass *klass, void *data)
 
     dc->desc        = "ISA bridge";
     dc->vmsd        = &vmstate_piix3;
-    k->no_hotplug   = 1;
+    dc->hotpluggable   = false;
     k->init         = piix3_initfn;
     k->config_write = piix3_write_config;
     k->vendor_id    = PCI_VENDOR_ID_INTEL;
@@ -656,7 +656,7 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
 
     dc->desc        = "ISA bridge";
     dc->vmsd        = &vmstate_piix3;
-    k->no_hotplug   = 1;
+    dc->hotpluggable   = false;
     k->init         = piix3_initfn;
     k->config_write = piix3_write_config_xen;
     k->vendor_id    = PCI_VENDOR_ID_INTEL;
@@ -682,7 +682,6 @@ static void i440fx_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = i440fx_initfn;
     k->config_write = i440fx_write_config;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
@@ -696,6 +695,7 @@ static void i440fx_class_init(ObjectClass *klass, void *data)
      * host-facing part, which can't be device_add'ed, yet.
      */
     dc->cannot_instantiate_with_device_add_yet = true;
+    dc->hotpluggable   = false;
 }
 
 static const TypeInfo i440fx_info = {
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 1221f32..d69961f 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1761,11 +1761,7 @@ static int pci_qdev_init(DeviceState *qdev)
                                      pci_dev->devfn);
     if (pci_dev == NULL)
         return -1;
-    if (qdev->hotplugged && pc->no_hotplug) {
-        qerror_report(QERR_DEVICE_NO_HOTPLUG, object_get_typename(OBJECT(pci_dev)));
-        do_pci_unregister_device(pci_dev);
-        return -1;
-    }
+
     if (pc->init) {
         rc = pc->init(pci_dev);
         if (rc != 0) {
@@ -1800,12 +1796,7 @@ static int pci_qdev_init(DeviceState *qdev)
 static int pci_unplug_device(DeviceState *qdev)
 {
     PCIDevice *dev = PCI_DEVICE(qdev);
-    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
 
-    if (pc->no_hotplug) {
-        qerror_report(QERR_DEVICE_NO_HOTPLUG, object_get_typename(OBJECT(dev)));
-        return -1;
-    }
     return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
                              PCI_HOTPLUG_DISABLED);
 }
diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c
index 0c98594..484a9bd 100644
--- a/hw/usb/hcd-ehci-pci.c
+++ b/hw/usb/hcd-ehci-pci.c
@@ -123,7 +123,7 @@ static void ehci_class_init(ObjectClass *klass, void *data)
     k->init = usb_ehci_pci_initfn;
     k->class_id = PCI_CLASS_SERIAL_USB;
     k->config_write = usb_ehci_pci_write_config;
-    k->no_hotplug = 1;
+    dc->hotpluggable = false;
     dc->vmsd = &vmstate_ehci_pci;
     dc->props = ehci_pci_properties;
 }
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index e38cdeb..3d35058 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -1993,10 +1993,10 @@ static void ohci_pci_class_init(ObjectClass *klass, void *data)
     k->vendor_id = PCI_VENDOR_ID_APPLE;
     k->device_id = PCI_DEVICE_ID_APPLE_IPID_USB;
     k->class_id = PCI_CLASS_SERIAL_USB;
-    k->no_hotplug = 1;
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
     dc->desc = "Apple USB Controller";
     dc->props = ohci_pci_properties;
+    dc->hotpluggable = false;
 }
 
 static const TypeInfo ohci_pci_info = {
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 238d1d2..ad814b5 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -1318,7 +1318,7 @@ static void uhci_class_init(ObjectClass *klass, void *data)
     k->device_id = info->device_id;
     k->revision  = info->revision;
     k->class_id  = PCI_CLASS_SERIAL_USB;
-    k->no_hotplug = 1;
+    dc->hotpluggable = false;
     dc->vmsd = &vmstate_uhci;
     dc->props = uhci_properties;
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index bafe085..0fa814e 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3798,6 +3798,7 @@ static void xhci_class_init(ObjectClass *klass, void *data)
     dc->vmsd    = &vmstate_xhci;
     dc->props   = xhci_properties;
     dc->reset   = xhci_reset;
+    dc->hotpluggable   = false;
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
     k->init         = usb_xhci_initfn;
     k->vendor_id    = PCI_VENDOR_ID_NEC;
@@ -3805,7 +3806,6 @@ static void xhci_class_init(ObjectClass *klass, void *data)
     k->class_id     = PCI_CLASS_SERIAL_USB;
     k->revision     = 0x03;
     k->is_express   = 1;
-    k->no_hotplug   = 1;
 }
 
 static const TypeInfo xhci_info = {
-- 
MST

[Qemu-devel] [PULL 16/20] acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-handler API

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

Split piix4_device_hotplug() into hotplug/unplug callbacks
and register them as "hotplug-handler" interface implementation of
PIIX4_PM device.

Replace pci_bus_hotplug() wiring with setting link on
PCI BUS "hotplug-handler" property to PIIX4_PM device.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/acpi.h  |  1 +
 include/hw/acpi/pcihp.h | 10 ++++++----
 hw/acpi/pcihp.c         | 43 +++++++++++++++++++++++++++++++------------
 hw/acpi/piix4.c         | 36 ++++++++++++++++++++++--------------
 4 files changed, 60 insertions(+), 30 deletions(-)

diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
index 3e53297..a9fae9d 100644
--- a/include/hw/acpi/acpi.h
+++ b/include/hw/acpi/acpi.h
@@ -24,6 +24,7 @@
 #include "qemu/notify.h"
 #include "qemu/option.h"
 #include "exec/memory.h"
+#include "hw/irq.h"
 
 /* from linux include/acpi/actype.h */
 /* Default ACPI register widths */
diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index 0a90e4a..9323838 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -29,7 +29,8 @@
 
 #include <inttypes.h>
 #include <qemu/typedefs.h>
-#include "hw/pci/pci.h" /* for PCIHotplugState */
+#include "hw/acpi/acpi.h"
+#include "migration/vmstate.h"
 
 typedef struct AcpiPciHpPciStatus {
     uint32_t up;
@@ -52,9 +53,10 @@ typedef struct AcpiPciHpState {
 void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root,
                      MemoryRegion *address_space_io, bool bridges_enabled);
 
-/* Invoke on device hotplug */
-int acpi_pcihp_device_hotplug(AcpiPciHpState *, PCIDevice *,
-                              PCIHotplugState state);
+void acpi_pcihp_device_plug_cb(ACPIREGS *ar, qemu_irq irq, AcpiPciHpState *s,
+                               DeviceState *dev, Error **errp);
+void acpi_pcihp_device_unplug_cb(ACPIREGS *ar, qemu_irq irq, AcpiPciHpState *s,
+                                 DeviceState *dev, Error **errp);
 
 /* Called on reset */
 void acpi_pcihp_reset(AcpiPciHpState *s);
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 3bd5a06..f80c480 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -46,6 +46,7 @@
 # define ACPI_PCIHP_DPRINTF(format, ...)     do { } while (0)
 #endif
 
+#define ACPI_PCI_HOTPLUG_STATUS 2
 #define ACPI_PCIHP_ADDR 0xae00
 #define ACPI_PCIHP_SIZE 0x0014
 #define ACPI_PCIHP_LEGACY_SIZE 0x000f
@@ -179,29 +180,47 @@ void acpi_pcihp_reset(AcpiPciHpState *s)
     acpi_pcihp_update(s);
 }
 
-int acpi_pcihp_device_hotplug(AcpiPciHpState *s, PCIDevice *dev,
-                              PCIHotplugState state)
+void acpi_pcihp_device_plug_cb(ACPIREGS *ar, qemu_irq irq, AcpiPciHpState *s,
+                               DeviceState *dev, Error **errp)
 {
-    int slot = PCI_SLOT(dev->devfn);
-    int bsel = acpi_pcihp_get_bsel(dev->bus);
+    PCIDevice *pdev = PCI_DEVICE(dev);
+    int slot = PCI_SLOT(pdev->devfn);
+    int bsel = acpi_pcihp_get_bsel(pdev->bus);
     if (bsel < 0) {
-        return -1;
+        error_setg(errp, "Unsupported bus. Bus doesn't have property '"
+                   ACPI_PCIHP_PROP_BSEL "' set");
+        return;
     }
 
     /* Don't send event when device is enabled during qemu machine creation:
      * it is present on boot, no hotplug event is necessary. We do send an
      * event when the device is disabled later. */
-    if (state == PCI_COLDPLUG_ENABLED) {
-        return 0;
+    if (!dev->hotplugged) {
+        return;
     }
 
-    if (state == PCI_HOTPLUG_ENABLED) {
-        s->acpi_pcihp_pci_status[bsel].up |= (1U << slot);
-    } else {
-        s->acpi_pcihp_pci_status[bsel].down |= (1U << slot);
+    s->acpi_pcihp_pci_status[bsel].up |= (1U << slot);
+
+    ar->gpe.sts[0] |= ACPI_PCI_HOTPLUG_STATUS;
+    acpi_update_sci(ar, irq);
+}
+
+void acpi_pcihp_device_unplug_cb(ACPIREGS *ar, qemu_irq irq, AcpiPciHpState *s,
+                                 DeviceState *dev, Error **errp)
+{
+    PCIDevice *pdev = PCI_DEVICE(dev);
+    int slot = PCI_SLOT(pdev->devfn);
+    int bsel = acpi_pcihp_get_bsel(pdev->bus);
+    if (bsel < 0) {
+        error_setg(errp, "Unsupported bus. Bus doesn't have property '"
+                   ACPI_PCIHP_PROP_BSEL "' set");
+        return;
     }
 
-    return 0;
+    s->acpi_pcihp_pci_status[bsel].down |= (1U << slot);
+
+    ar->gpe.sts[0] |= ACPI_PCI_HOTPLUG_STATUS;
+    acpi_update_sci(ar, irq);
 }
 
 static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size)
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 077776a..9f21653 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -32,6 +32,7 @@
 #include "hw/acpi/piix4.h"
 #include "hw/acpi/pcihp.h"
 #include "hw/acpi/cpu_hotplug.h"
+#include "hw/hotplug.h"
 
 //#define DEBUG
 
@@ -44,8 +45,6 @@
 #define GPE_BASE 0xafe0
 #define GPE_LEN 4
 
-#define PIIX4_PCI_HOTPLUG_STATUS 2
-
 struct pci_status {
     uint32_t up; /* deprecated, maintained for migration compatibility */
     uint32_t down;
@@ -311,24 +310,26 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
     acpi_pm1_evt_power_down(&s->ar);
 }
 
-static int piix4_acpi_pci_hotplug(DeviceState *qdev, PCIDevice *dev,
-                                  PCIHotplugState state)
+static void piix4_pci_device_plug_cb(HotplugHandler *hotplug_dev,
+                                     DeviceState *dev, Error **errp)
 {
-    PIIX4PMState *s = PIIX4_PM(qdev);
-    int ret = acpi_pcihp_device_hotplug(&s->acpi_pci_hotplug, dev, state);
-    if (ret < 0) {
-        return ret;
-    }
-    s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
+    PIIX4PMState *s = PIIX4_PM(hotplug_dev);
+    acpi_pcihp_device_plug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev, errp);
+}
 
-    acpi_update_sci(&s->ar, s->irq);
-    return 0;
+static void piix4_pci_device_unplug_cb(HotplugHandler *hotplug_dev,
+                                       DeviceState *dev, Error **errp)
+{
+    PIIX4PMState *s = PIIX4_PM(hotplug_dev);
+    acpi_pcihp_device_unplug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev,
+                                errp);
 }
 
-static void piix4_update_bus_hotplug(PCIBus *bus, void *opaque)
+static void piix4_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
 {
     PIIX4PMState *s = opaque;
-    pci_bus_hotplug(bus, piix4_acpi_pci_hotplug, DEVICE(s));
+
+    qbus_set_hotplug_handler(BUS(pci_bus), DEVICE(s), &error_abort);
 }
 
 static void piix4_pm_machine_ready(Notifier *n, void *opaque)
@@ -535,6 +536,7 @@ static void piix4_pm_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 = piix4_pm_initfn;
     k->config_write = pm_write_config;
@@ -551,6 +553,8 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
      */
     dc->cannot_instantiate_with_device_add_yet = true;
     dc->hotpluggable = false;
+    hc->plug = piix4_pci_device_plug_cb;
+    hc->unplug = piix4_pci_device_unplug_cb;
 }
 
 static const TypeInfo piix4_pm_info = {
@@ -558,6 +562,10 @@ static const TypeInfo piix4_pm_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PIIX4PMState),
     .class_init    = piix4_pm_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void piix4_pm_register_types(void)
-- 
MST

[Qemu-devel] [PULL 17/20] pci/shpc: convert SHPC hotplug to use hotplug-handler API

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

Split shpc_device_hotplug() into hotplug/unplug callbacks
and register them as "hotplug-handler" interface implementation of
PCI_BRIDGE_DEV device.

Replace pci_bus_hotplug() wiring with setting link on PCI BUS
"hotplug-handler" property to PCI_BRIDGE_DEV device.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/pci/shpc.h          |   8 +++
 hw/pci-bridge/pci_bridge_dev.c |   9 +++
 hw/pci/shpc.c                  | 124 +++++++++++++++++++++++++----------------
 3 files changed, 94 insertions(+), 47 deletions(-)

diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index 467911a..eef1a1a 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -4,6 +4,8 @@
 #include "qemu-common.h"
 #include "exec/memory.h"
 #include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "hw/hotplug.h"
 
 struct SHPCDevice {
     /* Capability offset in device's config space */
@@ -41,6 +43,12 @@ int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar, unsigned off);
 void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
 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);
+
 extern VMStateInfo shpc_vmstate_info;
 #define SHPC_VMSTATE(_field, _type) \
     VMSTATE_BUFFER_UNSAFE_INFO(_field, _type, 0, shpc_vmstate_info, 0)
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 440e187..e68145c 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -26,6 +26,7 @@
 #include "hw/pci/slotid_cap.h"
 #include "exec/memory.h"
 #include "hw/pci/pci_bus.h"
+#include "hw/hotplug.h"
 
 #define TYPE_PCI_BRIDGE_DEV "pci-bridge"
 #define PCI_BRIDGE_DEV(obj) \
@@ -136,6 +137,8 @@ static void pci_bridge_dev_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 = pci_bridge_dev_initfn;
     k->exit = pci_bridge_dev_exitfn;
     k->config_write = pci_bridge_dev_write_config;
@@ -148,6 +151,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
     dc->props = pci_bridge_dev_properties;
     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;
 }
 
 static const TypeInfo pci_bridge_dev_info = {
@@ -155,6 +160,10 @@ static const TypeInfo pci_bridge_dev_info = {
     .parent        = TYPE_PCI_BRIDGE,
     .instance_size = sizeof(PCIBridgeDev),
     .class_init = pci_bridge_dev_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void pci_bridge_dev_register(void)
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 576244b..180faa7 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -7,6 +7,7 @@
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_bus.h"
 #include "hw/pci/msi.h"
+#include "qapi/qmp/qerror.h"
 
 /* TODO: model power only and disabled slot states. */
 /* TODO: handle SERR and wakeups */
@@ -490,65 +491,93 @@ static const MemoryRegionOps shpc_mmio_ops = {
         .max_access_size = 4,
     },
 };
-
-static int shpc_device_hotplug(DeviceState *qdev, PCIDevice *affected_dev,
-                               PCIHotplugState hotplug_state)
+static void shpc_device_hotplug_common(PCIDevice *affected_dev, int *slot,
+                                       SHPCDevice *shpc, Error **errp)
 {
     int pci_slot = PCI_SLOT(affected_dev->devfn);
-    uint8_t state;
-    uint8_t led;
-    PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev);
-    SHPCDevice *shpc = d->shpc;
-    int slot = SHPC_PCI_TO_IDX(pci_slot);
-    if (pci_slot < SHPC_IDX_TO_PCI(0) || slot >= shpc->nslots) {
-        error_report("Unsupported PCI slot %d for standard hotplug "
-                     "controller. Valid slots are between %d and %d.",
-                     pci_slot, SHPC_IDX_TO_PCI(0),
-                     SHPC_IDX_TO_PCI(shpc->nslots) - 1);
-        return -1;
+    *slot = SHPC_PCI_TO_IDX(pci_slot);
+
+    if (pci_slot < SHPC_IDX_TO_PCI(0) || *slot >= shpc->nslots) {
+        error_setg(errp, "Unsupported PCI slot %d for standard hotplug "
+                   "controller. Valid slots are between %d and %d.",
+                   pci_slot, SHPC_IDX_TO_PCI(0),
+                   SHPC_IDX_TO_PCI(shpc->nslots) - 1);
+        return;
+    }
+}
+
+void shpc_device_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                            Error **errp)
+{
+    Error *local_err = NULL;
+    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
+    SHPCDevice *shpc = pci_hotplug_dev->shpc;
+    int slot;
+
+    shpc_device_hotplug_common(PCI_DEVICE(dev), &slot, shpc, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
     }
+
     /* Don't send event when device is enabled during qemu machine creation:
      * it is present on boot, no hotplug event is necessary. We do send an
      * event when the device is disabled later. */
-    if (hotplug_state == PCI_COLDPLUG_ENABLED) {
+    if (!dev->hotplugged) {
         shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_MRL_OPEN);
         shpc_set_status(shpc, slot, SHPC_SLOT_STATUS_PRSNT_7_5W,
                         SHPC_SLOT_STATUS_PRSNT_MASK);
-        return 0;
+        return;
     }
-    if (hotplug_state == PCI_HOTPLUG_DISABLED) {
-        shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |= SHPC_SLOT_EVENT_BUTTON;
-        state = shpc_get_status(shpc, slot, SHPC_SLOT_STATE_MASK);
-        led = shpc_get_status(shpc, slot, SHPC_SLOT_PWR_LED_MASK);
-        if (state == SHPC_STATE_DISABLED && led == SHPC_LED_OFF) {
-            shpc_free_devices_in_slot(shpc, slot);
-            shpc_set_status(shpc, slot, 1, SHPC_SLOT_STATUS_MRL_OPEN);
-            shpc_set_status(shpc, slot, SHPC_SLOT_STATUS_PRSNT_EMPTY,
-                            SHPC_SLOT_STATUS_PRSNT_MASK);
-            shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
-                SHPC_SLOT_EVENT_MRL |
-                SHPC_SLOT_EVENT_PRESENCE;
-        }
+
+    /* This could be a cancellation of the previous removal.
+     * We check MRL state to figure out. */
+    if (shpc_get_status(shpc, slot, SHPC_SLOT_STATUS_MRL_OPEN)) {
+        shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_MRL_OPEN);
+        shpc_set_status(shpc, slot, SHPC_SLOT_STATUS_PRSNT_7_5W,
+                        SHPC_SLOT_STATUS_PRSNT_MASK);
+        shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
+            SHPC_SLOT_EVENT_BUTTON |
+            SHPC_SLOT_EVENT_MRL |
+            SHPC_SLOT_EVENT_PRESENCE;
     } else {
-        /* This could be a cancellation of the previous removal.
-         * We check MRL state to figure out. */
-        if (shpc_get_status(shpc, slot, SHPC_SLOT_STATUS_MRL_OPEN)) {
-            shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_MRL_OPEN);
-            shpc_set_status(shpc, slot, SHPC_SLOT_STATUS_PRSNT_7_5W,
-                            SHPC_SLOT_STATUS_PRSNT_MASK);
-            shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
-                SHPC_SLOT_EVENT_BUTTON |
-                SHPC_SLOT_EVENT_MRL |
-                SHPC_SLOT_EVENT_PRESENCE;
-        } else {
-            /* Press attention button to cancel removal */
-            shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
-                SHPC_SLOT_EVENT_BUTTON;
-        }
+        /* Press attention button to cancel removal */
+        shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
+            SHPC_SLOT_EVENT_BUTTON;
     }
     shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_66);
-    shpc_interrupt_update(d);
-    return 0;
+    shpc_interrupt_update(pci_hotplug_dev);
+}
+
+void shpc_device_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                               Error **errp)
+{
+    Error *local_err = NULL;
+    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
+    SHPCDevice *shpc = pci_hotplug_dev->shpc;
+    uint8_t state;
+    uint8_t led;
+    int slot;
+
+    shpc_device_hotplug_common(PCI_DEVICE(dev), &slot, shpc, errp);
+    if (local_err) {
+        return;
+    }
+
+    shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |= SHPC_SLOT_EVENT_BUTTON;
+    state = shpc_get_status(shpc, slot, SHPC_SLOT_STATE_MASK);
+    led = shpc_get_status(shpc, slot, SHPC_SLOT_PWR_LED_MASK);
+    if (state == SHPC_STATE_DISABLED && led == SHPC_LED_OFF) {
+        shpc_free_devices_in_slot(shpc, slot);
+        shpc_set_status(shpc, slot, 1, SHPC_SLOT_STATUS_MRL_OPEN);
+        shpc_set_status(shpc, slot, SHPC_SLOT_STATUS_PRSNT_EMPTY,
+                        SHPC_SLOT_STATUS_PRSNT_MASK);
+        shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
+            SHPC_SLOT_EVENT_MRL |
+            SHPC_SLOT_EVENT_PRESENCE;
+    }
+    shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_66);
+    shpc_interrupt_update(pci_hotplug_dev);
 }
 
 /* Initialize the SHPC structure in bridge's BAR. */
@@ -616,7 +645,8 @@ int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar, unsigned offset)
                           d, "shpc-mmio", SHPC_SIZEOF(d));
     shpc_cap_update_dword(d);
     memory_region_add_subregion(bar, offset, &shpc->mmio);
-    pci_bus_hotplug(sec_bus, shpc_device_hotplug, &d->qdev);
+
+    qbus_set_hotplug_handler(BUS(sec_bus), DEVICE(d), NULL);
 
     d->cap_present |= QEMU_PCI_CAP_SHPC;
     return 0;
-- 
MST

[Qemu-devel] [PULL 18/20] pci/pcie: convert PCIE hotplug to use hotplug-handler API

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

Split pcie_cap_slot_hotplug() into hotplug/unplug callbacks
and register them as "hotplug-handler" interface implementation of
PCIE_SLOT device.

Replace pci_bus_hotplug() wiring with setting link on PCI BUS
"hotplug-handler" property to PCI_BRIDGE_DEV device.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/pci/pcie.h |  5 ++++
 hw/pci/pcie.c         | 65 +++++++++++++++++++++++++++++++--------------------
 hw/pci/pcie_port.c    |  8 +++++++
 3 files changed, 53 insertions(+), 25 deletions(-)

diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 1966169..b0bf7e3 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -25,6 +25,7 @@
 #include "hw/pci/pci_regs.h"
 #include "hw/pci/pcie_regs.h"
 #include "hw/pci/pcie_aer.h"
+#include "hw/hotplug.h"
 
 typedef enum {
     /* for attention and power indicator */
@@ -122,4 +123,8 @@ extern const VMStateDescription vmstate_pcie_device;
     .offset     = vmstate_offset_value(_state, _field, PCIDevice),   \
 }
 
+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);
 #endif /* QEMU_PCIE_H */
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index ca60cf2..8ecd11e 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -26,6 +26,7 @@
 #include "hw/pci/pci_bus.h"
 #include "hw/pci/pcie_regs.h"
 #include "qemu/range.h"
+#include "qapi/qmp/qerror.h"
 
 //#define DEBUG_PCIE
 #ifdef DEBUG_PCIE
@@ -216,28 +217,20 @@ static void pcie_cap_slot_event(PCIDevice *dev, PCIExpressHotPlugEvent event)
     hotplug_event_notify(dev);
 }
 
-static int pcie_cap_slot_hotplug(DeviceState *qdev,
-                                 PCIDevice *pci_dev, PCIHotplugState state)
+static void pcie_cap_slot_hotplug_common(PCIDevice *hotplug_dev,
+                                         DeviceState *dev,
+                                         uint8_t **exp_cap, Error **errp)
 {
-    PCIDevice *d = PCI_DEVICE(qdev);
-    uint8_t *exp_cap = d->config + d->exp.exp_cap;
-    uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
-
-    /* Don't send event when device is enabled during qemu machine creation:
-     * it is present on boot, no hotplug event is necessary. We do send an
-     * event when the device is disabled later. */
-    if (state == PCI_COLDPLUG_ENABLED) {
-        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
-                                   PCI_EXP_SLTSTA_PDS);
-        return 0;
-    }
+    PCIDevice *pci_dev = PCI_DEVICE(dev);
+    *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
+    uint16_t sltsta = pci_get_word(*exp_cap + PCI_EXP_SLTSTA);
 
     PCIE_DEV_PRINTF(pci_dev, "hotplug state: %d\n", state);
     if (sltsta & PCI_EXP_SLTSTA_EIS) {
         /* the slot is electromechanically locked.
          * This error is propagated up to qdev and then to HMP/QMP.
          */
-        return -EBUSY;
+        error_setg_errno(errp, -EBUSY, "slot is electromechanically locked");
     }
 
     /* TODO: multifunction hot-plug.
@@ -245,18 +238,40 @@ static int pcie_cap_slot_hotplug(DeviceState *qdev,
      * hot plugged/unplugged.
      */
     assert(PCI_FUNC(pci_dev->devfn) == 0);
+}
 
-    if (state == PCI_HOTPLUG_ENABLED) {
+void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                              Error **errp)
+{
+    uint8_t *exp_cap;
+
+    pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
+
+    /* Don't send event when device is enabled during qemu machine creation:
+     * it is present on boot, no hotplug event is necessary. We do send an
+     * event when the device is disabled later. */
+    if (!dev->hotplugged) {
         pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
                                    PCI_EXP_SLTSTA_PDS);
-        pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);
-    } else {
-        object_unparent(OBJECT(pci_dev));
-        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
-                                     PCI_EXP_SLTSTA_PDS);
-        pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);
+        return;
     }
-    return 0;
+
+    pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
+                               PCI_EXP_SLTSTA_PDS);
+    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC);
+}
+
+void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                                 Error **errp)
+{
+    uint8_t *exp_cap;
+
+    pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
+
+    object_unparent(OBJECT(dev));
+    pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
+                                 PCI_EXP_SLTSTA_PDS);
+    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC);
 }
 
 /* pci express slot for pci express root/downstream port
@@ -305,8 +320,8 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
 
     dev->exp.hpev_notified = false;
 
-    pci_bus_hotplug(pci_bridge_get_sec_bus(PCI_BRIDGE(dev)),
-                    pcie_cap_slot_hotplug, &dev->qdev);
+    qbus_set_hotplug_handler(BUS(pci_bridge_get_sec_bus(PCI_BRIDGE(dev))),
+                             DEVICE(dev), NULL);
 }
 
 void pcie_cap_slot_reset(PCIDevice *dev)
diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c
index 2adb030..fa24877 100644
--- a/hw/pci/pcie_port.c
+++ b/hw/pci/pcie_port.c
@@ -19,6 +19,7 @@
  */
 
 #include "hw/pci/pcie_port.h"
+#include "hw/hotplug.h"
 
 void pcie_port_init_reg(PCIDevice *d)
 {
@@ -149,8 +150,11 @@ static Property pcie_slot_props[] = {
 static void pcie_slot_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     dc->props = pcie_slot_props;
+    hc->plug = pcie_cap_slot_hotplug_cb;
+    hc->unplug = pcie_cap_slot_hot_unplug_cb;
 }
 
 static const TypeInfo pcie_slot_type_info = {
@@ -159,6 +163,10 @@ static const TypeInfo pcie_slot_type_info = {
     .instance_size = sizeof(PCIESlot),
     .abstract = true,
     .class_init = pcie_slot_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void pcie_port_register_types(void)
-- 
MST

[Qemu-devel] [PULL 19/20] hw/pci: switch to a generic hotplug handling for PCIDevice

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

make qdev_unplug()/device_set_realized() to call hotplug handler's
plug/unplug methods if available and remove not needed anymore
hot(un)plug handling from PCIDevice.

In case if hotplug handler is not available, revert to the legacy
hotplug method for compatibility with not yet converted buses.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/pci/pci.h     | 10 ----------
 include/hw/pci/pci_bus.h |  2 --
 hw/core/qdev.c           | 17 +++++++++++++----
 hw/pci/pci.c             | 29 +----------------------------
 tests/Makefile           |  2 +-
 5 files changed, 15 insertions(+), 45 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index c173b6a..693dd6b 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -327,15 +327,6 @@ typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
 typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
 typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
 
-typedef enum {
-    PCI_HOTPLUG_DISABLED,
-    PCI_HOTPLUG_ENABLED,
-    PCI_COLDPLUG_ENABLED,
-} PCIHotplugState;
-
-typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
-                              PCIHotplugState state);
-
 #define TYPE_PCI_BUS "PCI"
 #define PCI_BUS(obj) OBJECT_CHECK(PCIBus, (obj), TYPE_PCI_BUS)
 #define TYPE_PCIE_BUS "PCIE"
@@ -354,7 +345,6 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
 void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                   void *irq_opaque, int nirq);
 int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
-void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
 /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
 int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin);
 PCIBus *pci_register_bus(DeviceState *parent, const char *name,
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 9df1788..fabaeee 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -16,8 +16,6 @@ struct PCIBus {
     pci_set_irq_fn set_irq;
     pci_map_irq_fn map_irq;
     pci_route_irq_fn route_intx_to_irq;
-    pci_hotplug_fn hotplug;
-    DeviceState *hotplug_qdev;
     void *irq_opaque;
     PCIDevice *devices[PCI_SLOT_MAX * PCI_FUNC_MAX];
     PCIDevice *parent_dev;
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 5c864db..64b66e0 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -213,7 +213,6 @@ void qdev_unplug(DeviceState *dev, Error **errp)
         error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
         return;
     }
-    assert(dc->unplug != NULL);
 
     if (!dc->hotpluggable) {
         error_set(errp, QERR_DEVICE_NO_HOTPLUG,
@@ -223,9 +222,13 @@ void qdev_unplug(DeviceState *dev, Error **errp)
 
     qdev_hot_removed = true;
 
-    if (dc->unplug(dev) < 0) {
-        error_set(errp, QERR_UNDEFINED_ERROR);
-        return;
+    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        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);
+        }
     }
 }
 
@@ -720,6 +723,12 @@ static void device_set_realized(Object *obj, bool value, Error **err)
             dc->realize(dev, &local_err);
         }
 
+        if (dev->parent_bus && dev->parent_bus->hotplug_handler &&
+            local_err == NULL) {
+            hotplug_handler_plug(dev->parent_bus->hotplug_handler,
+                                 dev, &local_err);
+        }
+
         if (qdev_get_vmsd(dev) && local_err == NULL) {
             vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
                                            dev->instance_id_alias,
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index d69961f..4e0701d 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -35,6 +35,7 @@
 #include "hw/pci/msi.h"
 #include "hw/pci/msix.h"
 #include "exec/address-spaces.h"
+#include "hw/hotplug.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI
@@ -346,13 +347,6 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
     bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
 }
 
-void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev)
-{
-    bus->qbus.allow_hotplug = 1;
-    bus->hotplug = hotplug;
-    bus->hotplug_qdev = qdev;
-}
-
 PCIBus *pci_register_bus(DeviceState *parent, const char *name,
                          pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                          void *irq_opaque,
@@ -1778,29 +1772,9 @@ static int pci_qdev_init(DeviceState *qdev)
     }
     pci_add_option_rom(pci_dev, is_default_rom);
 
-    if (bus->hotplug) {
-        /* Let buses differentiate between hotplug and when device is
-         * enabled during qemu machine creation. */
-        rc = bus->hotplug(bus->hotplug_qdev, pci_dev,
-                          qdev->hotplugged ? PCI_HOTPLUG_ENABLED:
-                          PCI_COLDPLUG_ENABLED);
-        if (rc != 0) {
-            int r = pci_unregister_device(&pci_dev->qdev);
-            assert(!r);
-            return rc;
-        }
-    }
     return 0;
 }
 
-static int pci_unplug_device(DeviceState *qdev)
-{
-    PCIDevice *dev = PCI_DEVICE(qdev);
-
-    return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
-                             PCI_HOTPLUG_DISABLED);
-}
-
 PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
                                     const char *name)
 {
@@ -2271,7 +2245,6 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
     k->init = pci_qdev_init;
-    k->unplug = pci_unplug_device;
     k->exit = pci_unregister_device;
     k->bus_type = TYPE_PCI_BUS;
     k->props = pci_props;
diff --git a/tests/Makefile b/tests/Makefile
index fd36eee..9a7d2f1 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -163,7 +163,7 @@ tests/test-xbzrle$(EXESUF): tests/test-xbzrle.o xbzrle.o page_cache.o libqemuuti
 tests/test-cutils$(EXESUF): tests/test-cutils.o util/cutils.o
 tests/test-int128$(EXESUF): tests/test-int128.o
 tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
-	hw/core/qdev.o hw/core/qdev-properties.o \
+	hw/core/qdev.o hw/core/qdev-properties.o hw/core/hotplug.o\
 	hw/core/irq.o \
 	$(qom-core-obj) \
 	$(test-qapi-obj-y) \
-- 
MST

[Qemu-devel] [PULL 20/20] ACPI: Remove commented-out code from HPET._CRS

[Michael S. Tsirkin]

From: "Gabriel L. Somlo" <gsomlo@gmail.com>

IRQNoFlags on HPET._CRS crashes WinXP because it causes the HPET
to conflict with the system timer and/or the RTC. It only occurs
on Apple hardware, and even there it is exposed fully only when
OS X is detected (via _OSI). Recent OS X versions work on QEMU
without this statement, so at this time there is no need to find
a better way to conditionally include the statement. This patch
removes the commented out (and wrong, should have been {0, 8})
statement from HPET._CRS.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-By: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-dsdt-hpet.dsl | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/i386/acpi-dsdt-hpet.dsl b/hw/i386/acpi-dsdt-hpet.dsl
index dfde174..44961b8 100644
--- a/hw/i386/acpi-dsdt-hpet.dsl
+++ b/hw/i386/acpi-dsdt-hpet.dsl
@@ -39,9 +39,6 @@ Scope(\_SB) {
             Return (0x0F)
         }
         Name(_CRS, ResourceTemplate() {
-#if 0       /* This makes WinXP BSOD for not yet figured reasons. */
-            IRQNoFlags() {2, 8}
-#endif
             Memory32Fixed(ReadOnly,
                 0xFED00000,         // Address Base
                 0x00000400,         // Address Length
-- 
MST

Re: [Qemu-devel] [PULL 00/20] acpi,pc,pci fixes and enhancements

[Peter Maydell]

On 10 February 2014 16:47, Michael S. Tsirkin <mst@redhat.com> wrote:
> The following changes since commit 2b2449f7e467957778ca006904471b231dc0ac8e:
>
>   Merge remote-tracking branch 'remotes/borntraeger/tags/kvm-s390-20140131' into staging (2014-02-04 18:46:33 +0000)
>
> are available in the git repository at:
>
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to 417c45ab2f847c0a47b1232f611aa886df6a97d5:
>
>   ACPI: Remove commented-out code from HPET._CRS (2014-02-10 11:09:33 +0200)

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 00/20] acpi,pc,pci fixes and enhancements

[Stefano Stabellini]

On Thu, 13 Feb 2014, Peter Maydell wrote:
> On 10 February 2014 16:47, Michael S. Tsirkin <mst@redhat.com> wrote:
> > The following changes since commit 2b2449f7e467957778ca006904471b231dc0ac8e:
> >
> >   Merge remote-tracking branch 'remotes/borntraeger/tags/kvm-s390-20140131' into staging (2014-02-04 18:46:33 +0000)
> >
> > are available in the git repository at:
> >
> >
> >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> >
> > for you to fetch changes up to 417c45ab2f847c0a47b1232f611aa886df6a97d5:
> >
> >   ACPI: Remove commented-out code from HPET._CRS (2014-02-10 11:09:33 +0200)
> 
> Applied, thanks.

It looks like that this series breaks disk unplug
(hw/ide/piix.c:pci_piix3_xen_ide_unplug).

I bisected it and the problem is caused by:

commit 5e95494380ecf83c97d28f72134ab45e0cace8f9
Author: Igor Mammedov <imammedo@redhat.com>
Date:   Wed Feb 5 16:36:52 2014 +0100

    hw/pci: switch to a generic hotplug handling for PCIDevice
    
    make qdev_unplug()/device_set_realized() to call hotplug handler's
    plug/unplug methods if available and remove not needed anymore
    hot(un)plug handling from PCIDevice.
    
    In case if hotplug handler is not available, revert to the legacy
    hotplug method for compatibility with not yet converted buses.
    
    Signed-off-by: Igor Mammedov <imammedo@redhat.com>
    Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Re: [Qemu-devel] [PULL 00/20] acpi,pc,pci fixes and enhancements

[Paolo Bonzini]

Il 18/02/2014 13:16, Stefano Stabellini ha scritto:
> It looks like that this series breaks disk unplug
> (hw/ide/piix.c:pci_piix3_xen_ide_unplug).
>
> I bisected it and the problem is caused by:
>
> commit 5e95494380ecf83c97d28f72134ab45e0cace8f9
> Author: Igor Mammedov <imammedo@redhat.com>
> Date:   Wed Feb 5 16:36:52 2014 +0100
>
>     hw/pci: switch to a generic hotplug handling for PCIDevice
>
>     make qdev_unplug()/device_set_realized() to call hotplug handler's
>     plug/unplug methods if available and remove not needed anymore
>     hot(un)plug handling from PCIDevice.
>
>     In case if hotplug handler is not available, revert to the legacy
>     hotplug method for compatibility with not yet converted buses.
>
>     Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>     Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>

What exactly breaks?

Paolo

Re: [Qemu-devel] [PULL 00/20] acpi,pc,pci fixes and enhancements

[Stefano Stabellini]

On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> Il 18/02/2014 13:16, Stefano Stabellini ha scritto:
> > It looks like that this series breaks disk unplug
> > (hw/ide/piix.c:pci_piix3_xen_ide_unplug).
> > 
> > I bisected it and the problem is caused by:
> > 
> > commit 5e95494380ecf83c97d28f72134ab45e0cace8f9
> > Author: Igor Mammedov <imammedo@redhat.com>
> > Date:   Wed Feb 5 16:36:52 2014 +0100
> > 
> >     hw/pci: switch to a generic hotplug handling for PCIDevice
> > 
> >     make qdev_unplug()/device_set_realized() to call hotplug handler's
> >     plug/unplug methods if available and remove not needed anymore
> >     hot(un)plug handling from PCIDevice.
> > 
> >     In case if hotplug handler is not available, revert to the legacy
> >     hotplug method for compatibility with not yet converted buses.
> > 
> >     Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >     Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > 
> 
> What exactly breaks?

Disk unplug: hw/ide/piix.c:pci_piix3_xen_ide_unplug (see the beginning
of the email :-P).
It is called by hw/xen/xen_platform.c:platform_fixed_ioport_writew, in
response to the guest writing to a magic ioport specifically to unplug
the emulated disk.
With this patch after the guest boots I can still access both xvda and
sda for the same disk, leading to fs corruptions.

Re: [Qemu-devel] [PULL 00/20] acpi,pc,pci fixes and enhancements

[Igor Mammedov]

On Tue, 18 Feb 2014 12:45:29 +0000
Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:

> On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> > Il 18/02/2014 13:16, Stefano Stabellini ha scritto:
> > > It looks like that this series breaks disk unplug
> > > (hw/ide/piix.c:pci_piix3_xen_ide_unplug).
> > > 
> > > I bisected it and the problem is caused by:
> > > 
> > > commit 5e95494380ecf83c97d28f72134ab45e0cace8f9
> > > Author: Igor Mammedov <imammedo@redhat.com>
> > > Date:   Wed Feb 5 16:36:52 2014 +0100
> > > 
> > >     hw/pci: switch to a generic hotplug handling for PCIDevice
> > > 
> > >     make qdev_unplug()/device_set_realized() to call hotplug handler's
> > >     plug/unplug methods if available and remove not needed anymore
> > >     hot(un)plug handling from PCIDevice.
> > > 
> > >     In case if hotplug handler is not available, revert to the legacy
> > >     hotplug method for compatibility with not yet converted buses.
> > > 
> > >     Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > >     Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > >     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > 
> > > 
> > 
> > What exactly breaks?
> 
> Disk unplug: hw/ide/piix.c:pci_piix3_xen_ide_unplug (see the beginning
> of the email :-P).
> It is called by hw/xen/xen_platform.c:platform_fixed_ioport_writew, in
> response to the guest writing to a magic ioport specifically to unplug
> the emulated disk.
> With this patch after the guest boots I can still access both xvda and
> sda for the same disk, leading to fs corruptions.
> 
Could you try with following debug patch?

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 64b66e0..84aa8be 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -214,6 +214,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
         return;
     }
 
+    fprintf(stderr, "dc->hotpluggable %d\n", dc->hotpluggable);
     if (!dc->hotpluggable) {
         error_set(errp, QERR_DEVICE_NO_HOTPLUG,
                   object_get_typename(OBJECT(dev)));
@@ -223,8 +224,12 @@ void qdev_unplug(DeviceState *dev, Error **errp)
     qdev_hot_removed = true;
 
     if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        fprintf(stderr, "bus name: %s, hotplug_handler: %s\n",
+                dev->parent_bus->name,
+                object_get_typename(OBJECT(dev->parent_bus->hotplug_handler)));
         hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
     } else {
+        fprintf(stderr, "legacy unplug: %p\n", dc->unplug);
         assert(dc->unplug != NULL);
         if (dc->unplug(dev) < 0) { /* legacy handler */
             error_set(errp, QERR_UNDEFINED_ERROR);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 1acd2b2..74b0cac 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -238,6 +238,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
     if (pci_enabled && acpi_enabled) {
         i2c_bus *smbus;
 
+       fprintf(stderr, "create piix4_pm_init\n");
         smi_irq = qemu_allocate_irqs(pc_acpi_smi_interrupt, first_cpu, 1);
         /* TODO: Populate SPD eeprom data.  */
         smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,

Re: [Qemu-devel] [PULL 00/20] acpi,pc,pci fixes and enhancements

[Paolo Bonzini]

Il 18/02/2014 13:45, Stefano Stabellini ha scritto:
> Disk unplug: hw/ide/piix.c:pci_piix3_xen_ide_unplug (see the beginning
> of the email :-P).
> It is called by hw/xen/xen_platform.c:platform_fixed_ioport_writew, in
> response to the guest writing to a magic ioport specifically to unplug
> the emulated disk.
> With this patch after the guest boots I can still access both xvda and
> sda for the same disk, leading to fs corruptions.

Ok, the last paragraph is what I was missing.

So this is dc->unplug for the PIIX3 IDE device.  Because PCI declares a 
hotplug handler, dc->unplug is not called anymore.

But unlike other dc->unplug callbacks, pci_piix3_xen_ide_unplug doesn't 
free the device, it just drops the disks underneath.  I think the 
simplest solution is to _not_ make it a dc->unplug callback at all, and 
call pci_piix3_xen_ide_unplug from unplug_disks instead of qdev_unplug. 
  qdev_unplug means "ask guest to start unplug", which is not what Xen 
wants to do here.

Paolo

Re: [Qemu-devel] [PULL 00/20] acpi,pc,pci fixes and enhancements

[Stefano Stabellini]

On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> Il 18/02/2014 13:45, Stefano Stabellini ha scritto:
> > Disk unplug: hw/ide/piix.c:pci_piix3_xen_ide_unplug (see the beginning
> > of the email :-P).
> > It is called by hw/xen/xen_platform.c:platform_fixed_ioport_writew, in
> > response to the guest writing to a magic ioport specifically to unplug
> > the emulated disk.
> > With this patch after the guest boots I can still access both xvda and
> > sda for the same disk, leading to fs corruptions.
> 
> Ok, the last paragraph is what I was missing.
> 
> So this is dc->unplug for the PIIX3 IDE device.  Because PCI declares a
> hotplug handler, dc->unplug is not called anymore.
> 
> But unlike other dc->unplug callbacks, pci_piix3_xen_ide_unplug doesn't free
> the device, it just drops the disks underneath.  I think the simplest solution
> is to _not_ make it a dc->unplug callback at all, and call
> pci_piix3_xen_ide_unplug from unplug_disks instead of qdev_unplug.
> qdev_unplug means "ask guest to start unplug", which is not what Xen wants to
> do here.

Yes, you are right, pci_piix3_xen_ide_unplug is not called anymore.
Calling it directly from unplug_disks fixes the issue:


---

Call pci_piix3_xen_ide_unplug from unplug_disks

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 0eda301..40757eb 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -167,7 +167,7 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
     return 0;
 }
 
-static int pci_piix3_xen_ide_unplug(DeviceState *dev)
+int pci_piix3_xen_ide_unplug(DeviceState *dev)
 {
     PCIIDEState *pci_ide;
     DriveInfo *di;
@@ -266,7 +266,6 @@ static void piix3_ide_xen_class_init(ObjectClass *klass, void *data)
     k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
     k->class_id = PCI_CLASS_STORAGE_IDE;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-    dc->unplug = pci_piix3_xen_ide_unplug;
 }
 
 static const TypeInfo piix3_ide_xen_info = {
diff --git a/hw/xen/xen_platform.c b/hw/xen/xen_platform.c
index 70875e4..1d9d0e9 100644
--- a/hw/xen/xen_platform.c
+++ b/hw/xen/xen_platform.c
@@ -27,6 +27,7 @@
 
 #include "hw/hw.h"
 #include "hw/i386/pc.h"
+#include "hw/ide.h"
 #include "hw/pci/pci.h"
 #include "hw/irq.h"
 #include "hw/xen/xen_common.h"
@@ -110,7 +111,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *o)
     if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
             PCI_CLASS_STORAGE_IDE
             && strcmp(d->name, "xen-pci-passthrough") != 0) {
-        qdev_unplug(DEVICE(d), NULL);
+        pci_piix3_xen_ide_unplug(DEVICE(d));
     }
 }
 
diff --git a/include/hw/ide.h b/include/hw/ide.h
index 507e6d3..bc8bd32 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -17,6 +17,7 @@ void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
 PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
+int pci_piix3_xen_ide_unplug(DeviceState *dev);
 void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 
 /* ide-mmio.c */

Re: [Qemu-devel] [PULL 00/20] acpi,pc,pci fixes and enhancements

[Paolo Bonzini]

Il 18/02/2014 15:25, Stefano Stabellini ha scritto:
> On Tue, 18 Feb 2014, Paolo Bonzini wrote:
>> Il 18/02/2014 13:45, Stefano Stabellini ha scritto:
>>> Disk unplug: hw/ide/piix.c:pci_piix3_xen_ide_unplug (see the beginning
>>> of the email :-P).
>>> It is called by hw/xen/xen_platform.c:platform_fixed_ioport_writew, in
>>> response to the guest writing to a magic ioport specifically to unplug
>>> the emulated disk.
>>> With this patch after the guest boots I can still access both xvda and
>>> sda for the same disk, leading to fs corruptions.
>>
>> Ok, the last paragraph is what I was missing.
>>
>> So this is dc->unplug for the PIIX3 IDE device.  Because PCI declares a
>> hotplug handler, dc->unplug is not called anymore.
>>
>> But unlike other dc->unplug callbacks, pci_piix3_xen_ide_unplug doesn't free
>> the device, it just drops the disks underneath.  I think the simplest solution
>> is to _not_ make it a dc->unplug callback at all, and call
>> pci_piix3_xen_ide_unplug from unplug_disks instead of qdev_unplug.
>> qdev_unplug means "ask guest to start unplug", which is not what Xen wants to
>> do here.
>
> Yes, you are right, pci_piix3_xen_ide_unplug is not called anymore.
> Calling it directly from unplug_disks fixes the issue:
>
>
> ---
>
> Call pci_piix3_xen_ide_unplug from unplug_disks
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 0eda301..40757eb 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -167,7 +167,7 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
>      return 0;
>  }
>
> -static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> +int pci_piix3_xen_ide_unplug(DeviceState *dev)
>  {
>      PCIIDEState *pci_ide;
>      DriveInfo *di;
> @@ -266,7 +266,6 @@ static void piix3_ide_xen_class_init(ObjectClass *klass, void *data)
>      k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
>      k->class_id = PCI_CLASS_STORAGE_IDE;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> -    dc->unplug = pci_piix3_xen_ide_unplug;
>  }
>
>  static const TypeInfo piix3_ide_xen_info = {
> diff --git a/hw/xen/xen_platform.c b/hw/xen/xen_platform.c
> index 70875e4..1d9d0e9 100644
> --- a/hw/xen/xen_platform.c
> +++ b/hw/xen/xen_platform.c
> @@ -27,6 +27,7 @@
>
>  #include "hw/hw.h"
>  #include "hw/i386/pc.h"
> +#include "hw/ide.h"
>  #include "hw/pci/pci.h"
>  #include "hw/irq.h"
>  #include "hw/xen/xen_common.h"
> @@ -110,7 +111,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *o)
>      if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
>              PCI_CLASS_STORAGE_IDE
>              && strcmp(d->name, "xen-pci-passthrough") != 0) {
> -        qdev_unplug(DEVICE(d), NULL);
> +        pci_piix3_xen_ide_unplug(DEVICE(d));
>      }
>  }
>
> diff --git a/include/hw/ide.h b/include/hw/ide.h
> index 507e6d3..bc8bd32 100644
> --- a/include/hw/ide.h
> +++ b/include/hw/ide.h
> @@ -17,6 +17,7 @@ void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
>  PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>  PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>  PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
> +int pci_piix3_xen_ide_unplug(DeviceState *dev);
>  void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>
>  /* ide-mmio.c */
>

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

Paolo

Re: [Qemu-devel] [PULL 00/20] acpi,pc,pci fixes and enhancements

[Stefano Stabellini]

On Tue, 18 Feb 2014, Igor Mammedov wrote:
> On Tue, 18 Feb 2014 12:45:29 +0000
> Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> 
> > On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> > > Il 18/02/2014 13:16, Stefano Stabellini ha scritto:
> > > > It looks like that this series breaks disk unplug
> > > > (hw/ide/piix.c:pci_piix3_xen_ide_unplug).
> > > > 
> > > > I bisected it and the problem is caused by:
> > > > 
> > > > commit 5e95494380ecf83c97d28f72134ab45e0cace8f9
> > > > Author: Igor Mammedov <imammedo@redhat.com>
> > > > Date:   Wed Feb 5 16:36:52 2014 +0100
> > > > 
> > > >     hw/pci: switch to a generic hotplug handling for PCIDevice
> > > > 
> > > >     make qdev_unplug()/device_set_realized() to call hotplug handler's
> > > >     plug/unplug methods if available and remove not needed anymore
> > > >     hot(un)plug handling from PCIDevice.
> > > > 
> > > >     In case if hotplug handler is not available, revert to the legacy
> > > >     hotplug method for compatibility with not yet converted buses.
> > > > 
> > > >     Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > >     Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > >     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > 
> > > > 
> > > 
> > > What exactly breaks?
> > 
> > Disk unplug: hw/ide/piix.c:pci_piix3_xen_ide_unplug (see the beginning
> > of the email :-P).
> > It is called by hw/xen/xen_platform.c:platform_fixed_ioport_writew, in
> > response to the guest writing to a magic ioport specifically to unplug
> > the emulated disk.
> > With this patch after the guest boots I can still access both xvda and
> > sda for the same disk, leading to fs corruptions.
> > 
> Could you try with following debug patch?
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 64b66e0..84aa8be 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -214,6 +214,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    fprintf(stderr, "dc->hotpluggable %d\n", dc->hotpluggable);
>      if (!dc->hotpluggable) {
>          error_set(errp, QERR_DEVICE_NO_HOTPLUG,
>                    object_get_typename(OBJECT(dev)));
> @@ -223,8 +224,12 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>      qdev_hot_removed = true;
>  
>      if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> +        fprintf(stderr, "bus name: %s, hotplug_handler: %s\n",
> +                dev->parent_bus->name,
> +                object_get_typename(OBJECT(dev->parent_bus->hotplug_handler)));
>          hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
>      } else {
> +        fprintf(stderr, "legacy unplug: %p\n", dc->unplug);
>          assert(dc->unplug != NULL);
>          if (dc->unplug(dev) < 0) { /* legacy handler */
>              error_set(errp, QERR_UNDEFINED_ERROR);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 1acd2b2..74b0cac 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -238,6 +238,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
>      if (pci_enabled && acpi_enabled) {
>          i2c_bus *smbus;
>  
> +       fprintf(stderr, "create piix4_pm_init\n");
>          smi_irq = qemu_allocate_irqs(pc_acpi_smi_interrupt, first_cpu, 1);
>          /* TODO: Populate SPD eeprom data.  */
>          smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,

Sure, this is the output:

dc->hotpluggable 1
bus name: pci.0, hotplug_handler: PIIX4_PM

Re: [Qemu-devel] [PULL 13/20] qdev: add "hotpluggable" property to Device

[Andreas Färber]

Am 10.02.2014 17:48, schrieb Michael S. Tsirkin:
> From: Igor Mammedov <imammedo@redhat.com>
> 
> Currently it's possible to make PCIDevice not hotpluggable
> by using no_hotplug field of PCIDeviceClass. However it
> limits this only to PCI devices and prevents from
> generalizing hotplug code.
> 
> So add similar field to DeviceClass so it could be reused
> with other Devices and would allow to replace PCI specific
> hotplug callbacks with generic implementation. Following
> patches will replace PCIDeviceClass.no_hotplug with this
> new property.
> 
> In addition expose field as "hotpluggable" readonly property,
> to make it possible to read its value via QOM interface.
> 
> Make DeviceClass hotpluggable by default as it was assumed
> before.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/qdev-core.h |  3 +++
>  hw/core/qdev.c         | 29 +++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 41ec533..08d329d 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -50,6 +50,8 @@ struct VMStateDescription;
>   * is changed to %true. Deprecated, new types inheriting directly from
>   * TYPE_DEVICE should use @realize instead, new leaf types should consult
>   * their respective parent type.
> + * @hotpluggable: indicates if #DeviceClass is hotpluggable, available
> + * as readonly "hotpluggable" property of #DeviceState instance
>   *
>   * # Realization #
>   * Devices are constructed in two stages,
> @@ -110,6 +112,7 @@ typedef struct DeviceClass {
>       * TODO remove once we're there
>       */
>      bool cannot_instantiate_with_device_add_yet;
> +    bool hotpluggable;
>  
>      /* callbacks */
>      void (*reset)(DeviceState *dev);
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index c9f0c33..5c864db 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -215,6 +215,12 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>      }
>      assert(dc->unplug != NULL);
>  
> +    if (!dc->hotpluggable) {
> +        error_set(errp, QERR_DEVICE_NO_HOTPLUG,
> +                  object_get_typename(OBJECT(dev)));
> +        return;
> +    }
> +
>      qdev_hot_removed = true;
>  
>      if (dc->unplug(dev) < 0) {
> @@ -694,6 +700,11 @@ static void device_set_realized(Object *obj, bool value, Error **err)
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>      Error *local_err = NULL;
>  
> +    if (dev->hotplugged && !dc->hotpluggable) {
> +        error_set(err, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
> +        return;
> +    }
> +
>      if (value && !dev->realized) {
>          if (!obj->parent && local_err == NULL) {
>              static int unattached_count;
> @@ -734,6 +745,14 @@ static void device_set_realized(Object *obj, bool value, Error **err)
>      dev->realized = value;
>  }
>  
> +static bool device_get_hotpluggable(Object *obj, Error **err)
> +{
> +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> +    DeviceState *dev = DEVICE(obj);
> +
> +    return dc->hotpluggable && dev->parent_bus->allow_hotplug;
> +}
> +
>  static void device_initfn(Object *obj)
>  {
>      DeviceState *dev = DEVICE(obj);
> @@ -750,6 +769,8 @@ static void device_initfn(Object *obj)
>  
>      object_property_add_bool(obj, "realized",
>                               device_get_realized, device_set_realized, NULL);
> +    object_property_add_bool(obj, "hotpluggable",
> +                             device_get_hotpluggable, NULL, NULL);
>  
>      class = object_get_class(OBJECT(dev));
>      do {
> @@ -786,6 +807,14 @@ static void device_class_base_init(ObjectClass *class, void *data)
>       * so do not propagate them to the subclasses.
>       */
>      klass->props = NULL;
> +
> +    /* by default all devices were considered as hotpluggable,
> +     * so with intent to check it in generic qdev_unplug() /
> +     * device_set_realized() functions make every device
> +     * hotpluggable. Devices that shouldn't be hotpluggable,
> +     * should override it in their class_init()
> +     */
> +    klass->hotpluggable = true;

Is this intentionally being added to class_base_init or should this have
gone into class_init? As seen above, class_base_init has so far been
used to "erase" stuff set by the parent classes, not to set random defaults.

Regards,
Andreas

>  }
>  
>  static void device_unparent(Object *obj)

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PULL 13/20] qdev: add "hotpluggable" property to Device

[Igor Mammedov]

On Tue, 18 Feb 2014 17:35:16 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 10.02.2014 17:48, schrieb Michael S. Tsirkin:
> > From: Igor Mammedov <imammedo@redhat.com>
> > 
> > Currently it's possible to make PCIDevice not hotpluggable
> > by using no_hotplug field of PCIDeviceClass. However it
> > limits this only to PCI devices and prevents from
> > generalizing hotplug code.
> > 
> > So add similar field to DeviceClass so it could be reused
> > with other Devices and would allow to replace PCI specific
> > hotplug callbacks with generic implementation. Following
> > patches will replace PCIDeviceClass.no_hotplug with this
> > new property.
> > 
> > In addition expose field as "hotpluggable" readonly property,
> > to make it possible to read its value via QOM interface.
> > 
> > Make DeviceClass hotpluggable by default as it was assumed
> > before.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/hw/qdev-core.h |  3 +++
> >  hw/core/qdev.c         | 29 +++++++++++++++++++++++++++++
> >  2 files changed, 32 insertions(+)
> > 
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 41ec533..08d329d 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -50,6 +50,8 @@ struct VMStateDescription;
> >   * is changed to %true. Deprecated, new types inheriting directly from
> >   * TYPE_DEVICE should use @realize instead, new leaf types should consult
> >   * their respective parent type.
> > + * @hotpluggable: indicates if #DeviceClass is hotpluggable, available
> > + * as readonly "hotpluggable" property of #DeviceState instance
> >   *
> >   * # Realization #
> >   * Devices are constructed in two stages,
> > @@ -110,6 +112,7 @@ typedef struct DeviceClass {
> >       * TODO remove once we're there
> >       */
> >      bool cannot_instantiate_with_device_add_yet;
> > +    bool hotpluggable;
> >  
> >      /* callbacks */
> >      void (*reset)(DeviceState *dev);
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index c9f0c33..5c864db 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -215,6 +215,12 @@ void qdev_unplug(DeviceState *dev, Error **errp)
> >      }
> >      assert(dc->unplug != NULL);
> >  
> > +    if (!dc->hotpluggable) {
> > +        error_set(errp, QERR_DEVICE_NO_HOTPLUG,
> > +                  object_get_typename(OBJECT(dev)));
> > +        return;
> > +    }
> > +
> >      qdev_hot_removed = true;
> >  
> >      if (dc->unplug(dev) < 0) {
> > @@ -694,6 +700,11 @@ static void device_set_realized(Object *obj, bool value, Error **err)
> >      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> >      Error *local_err = NULL;
> >  
> > +    if (dev->hotplugged && !dc->hotpluggable) {
> > +        error_set(err, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
> > +        return;
> > +    }
> > +
> >      if (value && !dev->realized) {
> >          if (!obj->parent && local_err == NULL) {
> >              static int unattached_count;
> > @@ -734,6 +745,14 @@ static void device_set_realized(Object *obj, bool value, Error **err)
> >      dev->realized = value;
> >  }
> >  
> > +static bool device_get_hotpluggable(Object *obj, Error **err)
> > +{
> > +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> > +    DeviceState *dev = DEVICE(obj);
> > +
> > +    return dc->hotpluggable && dev->parent_bus->allow_hotplug;
> > +}
> > +
> >  static void device_initfn(Object *obj)
> >  {
> >      DeviceState *dev = DEVICE(obj);
> > @@ -750,6 +769,8 @@ static void device_initfn(Object *obj)
> >  
> >      object_property_add_bool(obj, "realized",
> >                               device_get_realized, device_set_realized, NULL);
> > +    object_property_add_bool(obj, "hotpluggable",
> > +                             device_get_hotpluggable, NULL, NULL);
> >  
> >      class = object_get_class(OBJECT(dev));
> >      do {
> > @@ -786,6 +807,14 @@ static void device_class_base_init(ObjectClass *class, void *data)
> >       * so do not propagate them to the subclasses.
> >       */
> >      klass->props = NULL;
> > +
> > +    /* by default all devices were considered as hotpluggable,
> > +     * so with intent to check it in generic qdev_unplug() /
> > +     * device_set_realized() functions make every device
> > +     * hotpluggable. Devices that shouldn't be hotpluggable,
> > +     * should override it in their class_init()
> > +     */
> > +    klass->hotpluggable = true;
> 
> Is this intentionally being added to class_base_init or should this have
> gone into class_init? As seen above, class_base_init has so far been
> used to "erase" stuff set by the parent classes, not to set random defaults.
You are right it should be in class_init(), I'll post fix shortly.

> Regards,
> Andreas
> 
> >  }
> >  
> >  static void device_unparent(Object *obj)
> 

Re: [Qemu-devel] [PULL 00/20] acpi,pc,pci fixes and enhancements

[Stefano Stabellini]

On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> Il 18/02/2014 15:25, Stefano Stabellini ha scritto:
> > On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> > > Il 18/02/2014 13:45, Stefano Stabellini ha scritto:
> > > > Disk unplug: hw/ide/piix.c:pci_piix3_xen_ide_unplug (see the beginning
> > > > of the email :-P).
> > > > It is called by hw/xen/xen_platform.c:platform_fixed_ioport_writew, in
> > > > response to the guest writing to a magic ioport specifically to unplug
> > > > the emulated disk.
> > > > With this patch after the guest boots I can still access both xvda and
> > > > sda for the same disk, leading to fs corruptions.
> > > 
> > > Ok, the last paragraph is what I was missing.
> > > 
> > > So this is dc->unplug for the PIIX3 IDE device.  Because PCI declares a
> > > hotplug handler, dc->unplug is not called anymore.
> > > 
> > > But unlike other dc->unplug callbacks, pci_piix3_xen_ide_unplug doesn't
> > > free
> > > the device, it just drops the disks underneath.  I think the simplest
> > > solution
> > > is to _not_ make it a dc->unplug callback at all, and call
> > > pci_piix3_xen_ide_unplug from unplug_disks instead of qdev_unplug.
> > > qdev_unplug means "ask guest to start unplug", which is not what Xen wants
> > > to
> > > do here.
> > 
> > Yes, you are right, pci_piix3_xen_ide_unplug is not called anymore.
> > Calling it directly from unplug_disks fixes the issue:
> > 
> > 
> > ---
> > 
> > Call pci_piix3_xen_ide_unplug from unplug_disks
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> > index 0eda301..40757eb 100644
> > --- a/hw/ide/piix.c
> > +++ b/hw/ide/piix.c
> > @@ -167,7 +167,7 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
> >      return 0;
> >  }
> > 
> > -static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> > +int pci_piix3_xen_ide_unplug(DeviceState *dev)
> >  {
> >      PCIIDEState *pci_ide;
> >      DriveInfo *di;
> > @@ -266,7 +266,6 @@ static void piix3_ide_xen_class_init(ObjectClass *klass,
> > void *data)
> >      k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
> >      k->class_id = PCI_CLASS_STORAGE_IDE;
> >      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> > -    dc->unplug = pci_piix3_xen_ide_unplug;
> >  }
> > 
> >  static const TypeInfo piix3_ide_xen_info = {
> > diff --git a/hw/xen/xen_platform.c b/hw/xen/xen_platform.c
> > index 70875e4..1d9d0e9 100644
> > --- a/hw/xen/xen_platform.c
> > +++ b/hw/xen/xen_platform.c
> > @@ -27,6 +27,7 @@
> > 
> >  #include "hw/hw.h"
> >  #include "hw/i386/pc.h"
> > +#include "hw/ide.h"
> >  #include "hw/pci/pci.h"
> >  #include "hw/irq.h"
> >  #include "hw/xen/xen_common.h"
> > @@ -110,7 +111,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void
> > *o)
> >      if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
> >              PCI_CLASS_STORAGE_IDE
> >              && strcmp(d->name, "xen-pci-passthrough") != 0) {
> > -        qdev_unplug(DEVICE(d), NULL);
> > +        pci_piix3_xen_ide_unplug(DEVICE(d));
> >      }
> >  }
> > 
> > diff --git a/include/hw/ide.h b/include/hw/ide.h
> > index 507e6d3..bc8bd32 100644
> > --- a/include/hw/ide.h
> > +++ b/include/hw/ide.h
> > @@ -17,6 +17,7 @@ void pci_cmd646_ide_init(PCIBus *bus, DriveInfo
> > **hd_table,
> >  PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > devfn);
> >  PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > devfn);
> >  PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > devfn);
> > +int pci_piix3_xen_ide_unplug(DeviceState *dev);
> >  void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
> > 
> >  /* ide-mmio.c */
> > 
> 
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks. Should I send it to Peter via the xen tree or anybody else wants
to pick this up?

Re: [Qemu-devel] [PULL 00/20] acpi,pc,pci fixes and enhancements

[Michael S. Tsirkin]

On Tue, Feb 18, 2014 at 05:10:00PM +0000, Stefano Stabellini wrote:
> On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> > Il 18/02/2014 15:25, Stefano Stabellini ha scritto:
> > > On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> > > > Il 18/02/2014 13:45, Stefano Stabellini ha scritto:
> > > > > Disk unplug: hw/ide/piix.c:pci_piix3_xen_ide_unplug (see the beginning
> > > > > of the email :-P).
> > > > > It is called by hw/xen/xen_platform.c:platform_fixed_ioport_writew, in
> > > > > response to the guest writing to a magic ioport specifically to unplug
> > > > > the emulated disk.
> > > > > With this patch after the guest boots I can still access both xvda and
> > > > > sda for the same disk, leading to fs corruptions.
> > > > 
> > > > Ok, the last paragraph is what I was missing.
> > > > 
> > > > So this is dc->unplug for the PIIX3 IDE device.  Because PCI declares a
> > > > hotplug handler, dc->unplug is not called anymore.
> > > > 
> > > > But unlike other dc->unplug callbacks, pci_piix3_xen_ide_unplug doesn't
> > > > free
> > > > the device, it just drops the disks underneath.  I think the simplest
> > > > solution
> > > > is to _not_ make it a dc->unplug callback at all, and call
> > > > pci_piix3_xen_ide_unplug from unplug_disks instead of qdev_unplug.
> > > > qdev_unplug means "ask guest to start unplug", which is not what Xen wants
> > > > to
> > > > do here.
> > > 
> > > Yes, you are right, pci_piix3_xen_ide_unplug is not called anymore.
> > > Calling it directly from unplug_disks fixes the issue:
> > > 
> > > 
> > > ---
> > > 
> > > Call pci_piix3_xen_ide_unplug from unplug_disks
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > 
> > > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> > > index 0eda301..40757eb 100644
> > > --- a/hw/ide/piix.c
> > > +++ b/hw/ide/piix.c
> > > @@ -167,7 +167,7 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
> > >      return 0;
> > >  }
> > > 
> > > -static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> > > +int pci_piix3_xen_ide_unplug(DeviceState *dev)
> > >  {
> > >      PCIIDEState *pci_ide;
> > >      DriveInfo *di;
> > > @@ -266,7 +266,6 @@ static void piix3_ide_xen_class_init(ObjectClass *klass,
> > > void *data)
> > >      k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
> > >      k->class_id = PCI_CLASS_STORAGE_IDE;
> > >      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> > > -    dc->unplug = pci_piix3_xen_ide_unplug;
> > >  }
> > > 
> > >  static const TypeInfo piix3_ide_xen_info = {
> > > diff --git a/hw/xen/xen_platform.c b/hw/xen/xen_platform.c
> > > index 70875e4..1d9d0e9 100644
> > > --- a/hw/xen/xen_platform.c
> > > +++ b/hw/xen/xen_platform.c
> > > @@ -27,6 +27,7 @@
> > > 
> > >  #include "hw/hw.h"
> > >  #include "hw/i386/pc.h"
> > > +#include "hw/ide.h"
> > >  #include "hw/pci/pci.h"
> > >  #include "hw/irq.h"
> > >  #include "hw/xen/xen_common.h"
> > > @@ -110,7 +111,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void
> > > *o)
> > >      if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
> > >              PCI_CLASS_STORAGE_IDE
> > >              && strcmp(d->name, "xen-pci-passthrough") != 0) {
> > > -        qdev_unplug(DEVICE(d), NULL);
> > > +        pci_piix3_xen_ide_unplug(DEVICE(d));
> > >      }
> > >  }
> > > 
> > > diff --git a/include/hw/ide.h b/include/hw/ide.h
> > > index 507e6d3..bc8bd32 100644
> > > --- a/include/hw/ide.h
> > > +++ b/include/hw/ide.h
> > > @@ -17,6 +17,7 @@ void pci_cmd646_ide_init(PCIBus *bus, DriveInfo
> > > **hd_table,
> > >  PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > > devfn);
> > >  PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > > devfn);
> > >  PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > > devfn);
> > > +int pci_piix3_xen_ide_unplug(DeviceState *dev);
> > >  void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
> > > 
> > >  /* ide-mmio.c */
> > > 
> > 
> > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Thanks. Should I send it to Peter via the xen tree or anybody else wants
> to pick this up?

I'll rebase my tree on top of this, to avoid bisect failures.

Re: [Qemu-devel] [PULL 00/20] acpi,pc,pci fixes and enhancements

[Michael S. Tsirkin]

On Wed, Feb 19, 2014 at 11:08:25AM +0200, Michael S. Tsirkin wrote:
> On Tue, Feb 18, 2014 at 05:10:00PM +0000, Stefano Stabellini wrote:
> > On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> > > Il 18/02/2014 15:25, Stefano Stabellini ha scritto:
> > > > On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> > > > > Il 18/02/2014 13:45, Stefano Stabellini ha scritto:
> > > > > > Disk unplug: hw/ide/piix.c:pci_piix3_xen_ide_unplug (see the beginning
> > > > > > of the email :-P).
> > > > > > It is called by hw/xen/xen_platform.c:platform_fixed_ioport_writew, in
> > > > > > response to the guest writing to a magic ioport specifically to unplug
> > > > > > the emulated disk.
> > > > > > With this patch after the guest boots I can still access both xvda and
> > > > > > sda for the same disk, leading to fs corruptions.
> > > > > 
> > > > > Ok, the last paragraph is what I was missing.
> > > > > 
> > > > > So this is dc->unplug for the PIIX3 IDE device.  Because PCI declares a
> > > > > hotplug handler, dc->unplug is not called anymore.
> > > > > 
> > > > > But unlike other dc->unplug callbacks, pci_piix3_xen_ide_unplug doesn't
> > > > > free
> > > > > the device, it just drops the disks underneath.  I think the simplest
> > > > > solution
> > > > > is to _not_ make it a dc->unplug callback at all, and call
> > > > > pci_piix3_xen_ide_unplug from unplug_disks instead of qdev_unplug.
> > > > > qdev_unplug means "ask guest to start unplug", which is not what Xen wants
> > > > > to
> > > > > do here.
> > > > 
> > > > Yes, you are right, pci_piix3_xen_ide_unplug is not called anymore.
> > > > Calling it directly from unplug_disks fixes the issue:
> > > > 
> > > > 
> > > > ---
> > > > 
> > > > Call pci_piix3_xen_ide_unplug from unplug_disks
> > > > 
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > 
> > > > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> > > > index 0eda301..40757eb 100644
> > > > --- a/hw/ide/piix.c
> > > > +++ b/hw/ide/piix.c
> > > > @@ -167,7 +167,7 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
> > > >      return 0;
> > > >  }
> > > > 
> > > > -static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> > > > +int pci_piix3_xen_ide_unplug(DeviceState *dev)
> > > >  {
> > > >      PCIIDEState *pci_ide;
> > > >      DriveInfo *di;
> > > > @@ -266,7 +266,6 @@ static void piix3_ide_xen_class_init(ObjectClass *klass,
> > > > void *data)
> > > >      k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
> > > >      k->class_id = PCI_CLASS_STORAGE_IDE;
> > > >      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> > > > -    dc->unplug = pci_piix3_xen_ide_unplug;
> > > >  }
> > > > 
> > > >  static const TypeInfo piix3_ide_xen_info = {
> > > > diff --git a/hw/xen/xen_platform.c b/hw/xen/xen_platform.c
> > > > index 70875e4..1d9d0e9 100644
> > > > --- a/hw/xen/xen_platform.c
> > > > +++ b/hw/xen/xen_platform.c
> > > > @@ -27,6 +27,7 @@
> > > > 
> > > >  #include "hw/hw.h"
> > > >  #include "hw/i386/pc.h"
> > > > +#include "hw/ide.h"
> > > >  #include "hw/pci/pci.h"
> > > >  #include "hw/irq.h"
> > > >  #include "hw/xen/xen_common.h"
> > > > @@ -110,7 +111,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void
> > > > *o)
> > > >      if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
> > > >              PCI_CLASS_STORAGE_IDE
> > > >              && strcmp(d->name, "xen-pci-passthrough") != 0) {
> > > > -        qdev_unplug(DEVICE(d), NULL);
> > > > +        pci_piix3_xen_ide_unplug(DEVICE(d));
> > > >      }
> > > >  }
> > > > 
> > > > diff --git a/include/hw/ide.h b/include/hw/ide.h
> > > > index 507e6d3..bc8bd32 100644
> > > > --- a/include/hw/ide.h
> > > > +++ b/include/hw/ide.h
> > > > @@ -17,6 +17,7 @@ void pci_cmd646_ide_init(PCIBus *bus, DriveInfo
> > > > **hd_table,
> > > >  PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > > > devfn);
> > > >  PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > > > devfn);
> > > >  PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > > > devfn);
> > > > +int pci_piix3_xen_ide_unplug(DeviceState *dev);
> > > >  void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
> > > > 
> > > >  /* ide-mmio.c */
> > > > 
> > > 
> > > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > Thanks. Should I send it to Peter via the xen tree or anybody else wants
> > to pick this up?
> 
> I'll rebase my tree on top of this, to avoid bisect failures.

Oh sorry, I noticed what broke this is - is merged already.
Pls merge fix through xen tree then, makes more sense.

Re: [Qemu-devel] [PULL 00/20] acpi,pc,pci fixes and enhancements

[Stefano Stabellini]

On Wed, 19 Feb 2014, Michael S. Tsirkin wrote:
> On Wed, Feb 19, 2014 at 11:08:25AM +0200, Michael S. Tsirkin wrote:
> > On Tue, Feb 18, 2014 at 05:10:00PM +0000, Stefano Stabellini wrote:
> > > On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> > > > Il 18/02/2014 15:25, Stefano Stabellini ha scritto:
> > > > > On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> > > > > > Il 18/02/2014 13:45, Stefano Stabellini ha scritto:
> > > > > > > Disk unplug: hw/ide/piix.c:pci_piix3_xen_ide_unplug (see the beginning
> > > > > > > of the email :-P).
> > > > > > > It is called by hw/xen/xen_platform.c:platform_fixed_ioport_writew, in
> > > > > > > response to the guest writing to a magic ioport specifically to unplug
> > > > > > > the emulated disk.
> > > > > > > With this patch after the guest boots I can still access both xvda and
> > > > > > > sda for the same disk, leading to fs corruptions.
> > > > > > 
> > > > > > Ok, the last paragraph is what I was missing.
> > > > > > 
> > > > > > So this is dc->unplug for the PIIX3 IDE device.  Because PCI declares a
> > > > > > hotplug handler, dc->unplug is not called anymore.
> > > > > > 
> > > > > > But unlike other dc->unplug callbacks, pci_piix3_xen_ide_unplug doesn't
> > > > > > free
> > > > > > the device, it just drops the disks underneath.  I think the simplest
> > > > > > solution
> > > > > > is to _not_ make it a dc->unplug callback at all, and call
> > > > > > pci_piix3_xen_ide_unplug from unplug_disks instead of qdev_unplug.
> > > > > > qdev_unplug means "ask guest to start unplug", which is not what Xen wants
> > > > > > to
> > > > > > do here.
> > > > > 
> > > > > Yes, you are right, pci_piix3_xen_ide_unplug is not called anymore.
> > > > > Calling it directly from unplug_disks fixes the issue:
> > > > > 
> > > > > 
> > > > > ---
> > > > > 
> > > > > Call pci_piix3_xen_ide_unplug from unplug_disks
> > > > > 
> > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > > 
> > > > > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> > > > > index 0eda301..40757eb 100644
> > > > > --- a/hw/ide/piix.c
> > > > > +++ b/hw/ide/piix.c
> > > > > @@ -167,7 +167,7 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
> > > > >      return 0;
> > > > >  }
> > > > > 
> > > > > -static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> > > > > +int pci_piix3_xen_ide_unplug(DeviceState *dev)
> > > > >  {
> > > > >      PCIIDEState *pci_ide;
> > > > >      DriveInfo *di;
> > > > > @@ -266,7 +266,6 @@ static void piix3_ide_xen_class_init(ObjectClass *klass,
> > > > > void *data)
> > > > >      k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
> > > > >      k->class_id = PCI_CLASS_STORAGE_IDE;
> > > > >      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> > > > > -    dc->unplug = pci_piix3_xen_ide_unplug;
> > > > >  }
> > > > > 
> > > > >  static const TypeInfo piix3_ide_xen_info = {
> > > > > diff --git a/hw/xen/xen_platform.c b/hw/xen/xen_platform.c
> > > > > index 70875e4..1d9d0e9 100644
> > > > > --- a/hw/xen/xen_platform.c
> > > > > +++ b/hw/xen/xen_platform.c
> > > > > @@ -27,6 +27,7 @@
> > > > > 
> > > > >  #include "hw/hw.h"
> > > > >  #include "hw/i386/pc.h"
> > > > > +#include "hw/ide.h"
> > > > >  #include "hw/pci/pci.h"
> > > > >  #include "hw/irq.h"
> > > > >  #include "hw/xen/xen_common.h"
> > > > > @@ -110,7 +111,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void
> > > > > *o)
> > > > >      if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
> > > > >              PCI_CLASS_STORAGE_IDE
> > > > >              && strcmp(d->name, "xen-pci-passthrough") != 0) {
> > > > > -        qdev_unplug(DEVICE(d), NULL);
> > > > > +        pci_piix3_xen_ide_unplug(DEVICE(d));
> > > > >      }
> > > > >  }
> > > > > 
> > > > > diff --git a/include/hw/ide.h b/include/hw/ide.h
> > > > > index 507e6d3..bc8bd32 100644
> > > > > --- a/include/hw/ide.h
> > > > > +++ b/include/hw/ide.h
> > > > > @@ -17,6 +17,7 @@ void pci_cmd646_ide_init(PCIBus *bus, DriveInfo
> > > > > **hd_table,
> > > > >  PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > > > > devfn);
> > > > >  PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > > > > devfn);
> > > > >  PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > > > > devfn);
> > > > > +int pci_piix3_xen_ide_unplug(DeviceState *dev);
> > > > >  void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
> > > > > 
> > > > >  /* ide-mmio.c */
> > > > > 
> > > > 
> > > > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> > > 
> > > Thanks. Should I send it to Peter via the xen tree or anybody else wants
> > > to pick this up?
> > 
> > I'll rebase my tree on top of this, to avoid bisect failures.
> 
> Oh sorry, I noticed what broke this is - is merged already.
> Pls merge fix through xen tree then, makes more sense.

All right, thanks.

Re: [Qemu-devel] [PULL 13/20] qdev: add "hotpluggable" property to Device

[Andreas Färber]

Am 10.02.2014 17:48, schrieb Michael S. Tsirkin:
> From: Igor Mammedov <imammedo@redhat.com>
> 
> Currently it's possible to make PCIDevice not hotpluggable
> by using no_hotplug field of PCIDeviceClass. However it
> limits this only to PCI devices and prevents from
> generalizing hotplug code.
> 
> So add similar field to DeviceClass so it could be reused
> with other Devices and would allow to replace PCI specific
> hotplug callbacks with generic implementation. Following
> patches will replace PCIDeviceClass.no_hotplug with this
> new property.
> 
> In addition expose field as "hotpluggable" readonly property,
> to make it possible to read its value via QOM interface.
> 
> Make DeviceClass hotpluggable by default as it was assumed
> before.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/qdev-core.h |  3 +++
>  hw/core/qdev.c         | 29 +++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 41ec533..08d329d 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -50,6 +50,8 @@ struct VMStateDescription;
>   * is changed to %true. Deprecated, new types inheriting directly from
>   * TYPE_DEVICE should use @realize instead, new leaf types should consult
>   * their respective parent type.
> + * @hotpluggable: indicates if #DeviceClass is hotpluggable, available
> + * as readonly "hotpluggable" property of #DeviceState instance
>   *
>   * # Realization #
>   * Devices are constructed in two stages,
> @@ -110,6 +112,7 @@ typedef struct DeviceClass {
>       * TODO remove once we're there
>       */
>      bool cannot_instantiate_with_device_add_yet;
> +    bool hotpluggable;
>  
>      /* callbacks */
>      void (*reset)(DeviceState *dev);
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index c9f0c33..5c864db 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -215,6 +215,12 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>      }
>      assert(dc->unplug != NULL);
>  
> +    if (!dc->hotpluggable) {
> +        error_set(errp, QERR_DEVICE_NO_HOTPLUG,
> +                  object_get_typename(OBJECT(dev)));
> +        return;
> +    }
> +
>      qdev_hot_removed = true;
>  
>      if (dc->unplug(dev) < 0) {
> @@ -694,6 +700,11 @@ static void device_set_realized(Object *obj, bool value, Error **err)
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>      Error *local_err = NULL;
>  
> +    if (dev->hotplugged && !dc->hotpluggable) {
> +        error_set(err, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
> +        return;
> +    }
> +
>      if (value && !dev->realized) {
>          if (!obj->parent && local_err == NULL) {
>              static int unattached_count;
> @@ -734,6 +745,14 @@ static void device_set_realized(Object *obj, bool value, Error **err)
>      dev->realized = value;
>  }
>  
> +static bool device_get_hotpluggable(Object *obj, Error **err)
> +{
> +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> +    DeviceState *dev = DEVICE(obj);
> +
> +    return dc->hotpluggable && dev->parent_bus->allow_hotplug;

This is breaking my extended qom-test. You add this property to each
device, irrespective of whether it has a non-NULL parent_bus, which
results in a SIGSEGV when qom-get'ting it. Will post a fix.

Next time please ping for review of such core changes.

Andreas

> +}
> +
>  static void device_initfn(Object *obj)
>  {
>      DeviceState *dev = DEVICE(obj);
> @@ -750,6 +769,8 @@ static void device_initfn(Object *obj)
>  
>      object_property_add_bool(obj, "realized",
>                               device_get_realized, device_set_realized, NULL);
> +    object_property_add_bool(obj, "hotpluggable",
> +                             device_get_hotpluggable, NULL, NULL);
>  
>      class = object_get_class(OBJECT(dev));
>      do {
> @@ -786,6 +807,14 @@ static void device_class_base_init(ObjectClass *class, void *data)
>       * so do not propagate them to the subclasses.
>       */
>      klass->props = NULL;
> +
> +    /* by default all devices were considered as hotpluggable,
> +     * so with intent to check it in generic qdev_unplug() /
> +     * device_set_realized() functions make every device
> +     * hotpluggable. Devices that shouldn't be hotpluggable,
> +     * should override it in their class_init()
> +     */
> +    klass->hotpluggable = true;
>  }
>  
>  static void device_unparent(Object *obj)
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg