[Qemu-devel] [PATCH v4 0/5] pc: make ACPI pcihp more reusable

[Qemu-devel] [PATCH v4 0/5] pc: make ACPI pcihp more reusable

[Igor Mammedov]

changes since v3:
 - keep "acpi-pci-hotplug-with-bridge-support" property
   and pass its value to acpi_pcihp_init() to init internal
   .legacy_piix field.
changes since v2:
 - rename 'use_acpi_pci_hotplug' field to 'legacy_piix'
   and corresponding properties 
 - drop excessive checks for legacy mode
 - rework legacy vmstate handling to use AcpiPciHpPciStatus[0] structure
   which reduced acpi/piix4.c by another 50 LOC
 - move legacy initialization to pcihp.c
changes since v1:
 - add BSEL property to root bus when in compatibility mode
     as result dropped "hw:acpi:pcihp: assume root PCI bus if bus has no ACPI_PCIHP_PROP_BSEL property"
 - rebasing on top of "pcihp: reduce number of device check events"
   added patch 3/4.

Reuses new pcihp code for handling legacy PCI hotplug in
acpi/piix4_pm, which significantly reduces code duplication
between piix4_pm and pcihp reducing codebase by ~200 LOC.

It also allows cleaner rebase of generic hotplug refactoring,
by not increasing code duplication since it will need to be
done only for pcihp.

Git tree for testing based on mst's PCI tree:
https://github.com/imammedo/qemu/commits/pcihp_cleanup_v4



Igor Mammedov (5):
  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

 hw/acpi/pcihp.c         |   68 ++++++++-------
 hw/acpi/piix4.c         |  210 ++++-------------------------------------------
 include/hw/acpi/pcihp.h |    5 +-
 3 files changed, 55 insertions(+), 228 deletions(-)

[Qemu-devel] [PATCH v4 1/5] pcihp: replace enable|disable_device() with oneliners

[Igor Mammedov]

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>
---
 hw/acpi/pcihp.c |   14 ++------------
 1 files 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;
-- 
1.7.1

[Qemu-devel] [PATCH v4 2/5] pcihp: make PCI hotplug mmio handlers indifferent to PCI_HOTPLUG_ADDR

[Igor Mammedov]

... 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>
---
 hw/acpi/pcihp.c |   24 ++++++++++++------------
 1 files 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);
-- 
1.7.1

[Qemu-devel] [PATCH v4 3/5] pcihp: make pci_read() mmio calback compatible with legacy ACPI hotplug

[Igor Mammedov]

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>
---
v2:
  - keep "acpi-pci-hotplug-with-bridge-support" property
    and tweek pcihp init API to initialize .legacy_piix field
---
 hw/acpi/pcihp.c         |    7 +++++--
 hw/acpi/piix4.c         |    3 ++-
 include/hw/acpi/pcihp.h |    3 ++-
 3 files changed, 9 insertions(+), 4 deletions(-)

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);
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 *,
-- 
1.7.1

[Qemu-devel] [PATCH v4 4/5] pcihp: remove unused AcpiPciHpPciStatus.device_present field

[Igor Mammedov]

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>
---
 include/hw/acpi/pcihp.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

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"
-- 
1.7.1

[Qemu-devel] [PATCH v4 5/5] hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug

[Igor Mammedov]

reduces acpi PCI hotplug code duplication by ~200LOC

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
 - remove exessive checks for compat mode
 - replace up/down fields of pci_status with corresponding fields
   form AcpiPciHpPciStatus in vmstate wich allow to remove extra
   50 LOC of legacy code
 - move legacy initialization logic to pcihp.c
v2:
 - replace obsolete 'device_present' with 'up' field
 - add/set ACPI_PCIHP_PROP_BSEL to 0 when running in compatibility
   mode with old machine types.
---
 hw/acpi/pcihp.c         |   23 ++++-
 hw/acpi/piix4.c         |  211 ++++-------------------------------------------
 include/hw/acpi/pcihp.h |    1 +
 3 files changed, 34 insertions(+), 201 deletions(-)

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;
-}
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];
-- 
1.7.1

Re: [Qemu-devel] [PATCH v4 0/5] pc: make ACPI pcihp more reusable

[Michael S. Tsirkin]

On Mon, Feb 03, 2014 at 11:44:56AM +0100, Igor Mammedov wrote:
> changes since v3:
>  - keep "acpi-pci-hotplug-with-bridge-support" property
>    and pass its value to acpi_pcihp_init() to init internal
>    .legacy_piix field.
> changes since v2:
>  - rename 'use_acpi_pci_hotplug' field to 'legacy_piix'
>    and corresponding properties 
>  - drop excessive checks for legacy mode
>  - rework legacy vmstate handling to use AcpiPciHpPciStatus[0] structure
>    which reduced acpi/piix4.c by another 50 LOC
>  - move legacy initialization to pcihp.c
> changes since v1:
>  - add BSEL property to root bus when in compatibility mode
>      as result dropped "hw:acpi:pcihp: assume root PCI bus if bus has no ACPI_PCIHP_PROP_BSEL property"
>  - rebasing on top of "pcihp: reduce number of device check events"
>    added patch 3/4.

Looks good to me.
Applied, thanks a lot.


> Reuses new pcihp code for handling legacy PCI hotplug in
> acpi/piix4_pm, which significantly reduces code duplication
> between piix4_pm and pcihp reducing codebase by ~200 LOC.
> 
> It also allows cleaner rebase of generic hotplug refactoring,
> by not increasing code duplication since it will need to be
> done only for pcihp.
> 
> Git tree for testing based on mst's PCI tree:
> https://github.com/imammedo/qemu/commits/pcihp_cleanup_v4
> 
> 
> 
> Igor Mammedov (5):
>   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
> 
>  hw/acpi/pcihp.c         |   68 ++++++++-------
>  hw/acpi/piix4.c         |  210 ++++-------------------------------------------
>  include/hw/acpi/pcihp.h |    5 +-
>  3 files changed, 55 insertions(+), 228 deletions(-)