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

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

[Igor Mammedov]

Reuses new pcihp code for handling legacy PCI hotplug in
acpi/piix4_pm, which significantly reduces code duplication
between piix4_pm and pcihp amd makes pcihp less dependend
on piix4 specific details.

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_v1

Igor Mammedov (4):
  hw:piix4:acpi: replace enable|disable_device() with oneliners
  hw:piix4:acpi: make PCI hotplug mmio handlers indifferent to
    PCI_HOTPLUG_ADDR
  hw:acpi:pcihp: assume root PCI bus if bus has no ACPI_PCIHP_PROP_BSEL
    property
  hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug

 hw/acpi/pcihp.c         |   58 +++++---------
 hw/acpi/piix4.c         |  196 +++++------------------------------------------
 include/hw/acpi/pcihp.h |    8 ++-
 3 files changed, 48 insertions(+), 214 deletions(-)

[Qemu-devel] [PATCH 1/4] hw:piix4:acpi: 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 3fa3d7c..b86ebfa 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -185,16 +185,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].device_present |= (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)
 {
@@ -213,9 +203,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].device_present |= (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 2/4] hw:piix4:acpi: 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 b86ebfa..6d34fe9 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;
@@ -222,26 +222,26 @@ 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:
         /* 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;
         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:
@@ -256,7 +256,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;
         }
@@ -264,7 +264,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 3/4] hw:acpi:pcihp: assume root PCI bus if bus has no ACPI_PCIHP_PROP_BSEL property

[Igor Mammedov]

when running with machine types older than 1.7 (i.e. without ACPI
builtin tables), PCI bus won't have ACPI_PCIHP_PROP_BSEL property
set.
Taking in account that acpi hotplug handler in 1.7 and older
machines is called only for root PCI bus, to make pcihp code
compatible with legacy machine types assume that bus without
ACPI_PCIHP_PROP_BSEL property has it equal to 0 and bail out
if it's not root bus.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/pcihp.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 6d34fe9..76dce8d 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -63,14 +63,10 @@ static int acpi_pcihp_get_bsel(PCIBus *bus)
 {
     QObject *o = object_property_get_qobject(OBJECT(bus),
                                              ACPI_PCIHP_PROP_BSEL, NULL);
-    int64_t bsel = -1;
     if (o) {
-        bsel = qint_get_int(qobject_to_qint(o));
+        return qint_get_int(qobject_to_qint(o));
     }
-    if (bsel < 0) {
-        return -1;
-    }
-    return bsel;
+    return 0;
 }
 
 static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
@@ -190,7 +186,7 @@ int acpi_pcihp_device_hotplug(AcpiPciHpState *s, PCIDevice *dev,
 {
     int slot = PCI_SLOT(dev->devfn);
     int bsel = acpi_pcihp_get_bsel(dev->bus);
-    if (bsel < 0) {
+    if ((bsel == 0) && (dev->bus != s->root)) {
         return -1;
     }
 
-- 
1.7.1

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

[Igor Mammedov]

reduces acpi PCI hotplug code duplication by 150LOC,
and makes pcihp less dependend on piix specific code.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/pcihp.c         |   10 +--
 hw/acpi/piix4.c         |  196 +++++------------------------------------------
 include/hw/acpi/pcihp.h |    8 ++-
 3 files changed, 31 insertions(+), 183 deletions(-)

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 76dce8d..25cc0fe 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -46,8 +46,6 @@
 # define ACPI_PCIHP_DPRINTF(format, ...)     do { } while (0)
 #endif
 
-#define PCI_HOTPLUG_ADDR 0xae00
-#define PCI_HOTPLUG_SIZE 0x0014
 #define PCI_UP_BASE 0x0000
 #define PCI_DOWN_BASE 0x0004
 #define PCI_EJ_BASE 0x0008
@@ -279,14 +277,14 @@ static const MemoryRegionOps acpi_pcihp_io_ops = {
     },
 };
 
-void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus,
-                     MemoryRegion *address_space_io)
+void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus, uint16_t io_base,
+                     uint16_t io_size, MemoryRegion *address_space_io)
 {
     s->root= root_bus;
     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);
+                          io_size);
+    memory_region_add_subregion(address_space_io, io_base, &s->io);
 }
 
 const VMStateDescription vmstate_acpi_pcihp_pci_status = {
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 5d55a3c..f818d76 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,8 +73,8 @@ typedef struct PIIX4PMState {
     Notifier machine_ready;
     Notifier powerdown_notifier;
 
-    /* for legacy pci hotplug (compatible with qemu 1.6 and older) */
-    MemoryRegion io_pci;
+    /* for legacy pci hotplug (compatible with qemu 1.6 and older)
+     * used only for migration and updated in pre_save() */
     struct pci_status pci0_status;
     uint32_t pci0_hotplug_enable;
     uint32_t pci0_slot_device_present;
@@ -175,16 +168,24 @@ static void vmstate_pci_status_pre_save(void *opaque)
     struct pci_status *pci0_status = opaque;
     PIIX4PMState *s = container_of(pci0_status, PIIX4PMState, pci0_status);
 
+    pci0_status->down = s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].down;
     /* 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;
+    pci0_status->up =
+        s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].device_present &
+        s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].hotplug_enable;
 }
 
 static int vmstate_acpi_post_load(void *opaque, int version_id)
 {
     PIIX4PMState *s = opaque;
 
+    if (!s->use_acpi_pci_hotplug) {
+        s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].down =
+            s->pci0_status.down;
+    }
+
     pm_io_space_update(s);
     return 0;
 }
@@ -304,60 +305,6 @@ static const VMStateDescription vmstate_acpi = {
     }
 };
 
-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 +324,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)
@@ -409,6 +352,10 @@ static int piix4_acpi_pci_hotplug(DeviceState *qdev, PCIDevice *dev,
 static void piix4_update_bus_hotplug(PCIBus *bus, void *opaque)
 {
     PIIX4PMState *s = opaque;
+
+    if (!s->use_acpi_pci_hotplug && (bus != s->acpi_pci_hotplug.root)) {
+        return;
+    }
     pci_bus_hotplug(bus, piix4_acpi_pci_hotplug, DEVICE(s));
 }
 
@@ -426,9 +373,7 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
     pci_conf[0x67] = (memory_region_present(io_as, 0x3f8) ? 0x08 : 0) |
         (memory_region_present(io_as, 0x2f8) ? 0x90 : 0);
 
-    if (s->use_acpi_pci_hotplug) {
-        pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
-    }
+    pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
 }
 
 static void piix4_pm_add_propeties(PIIX4PMState *s)
@@ -621,60 +566,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 +575,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,55 +582,13 @@ 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);
-    } 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, PIIX_PCI_HOTPLUG_ADDR,
+                    s->use_acpi_pci_hotplug ? PIIX_PCI_HOTPLUG_SIZE :
+                                              PIIX_PCI_HOTPLUG_LEGACY_SIZE,
+                    parent);
 
     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 6230e60..6bd41bb 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -31,6 +31,10 @@
 #include <qemu/typedefs.h>
 #include "hw/pci/pci.h" /* for PCIHotplugState */
 
+#define PIIX_PCI_HOTPLUG_ADDR 0xae00
+#define PIIX_PCI_HOTPLUG_SIZE 0x0014
+#define PIIX_PCI_HOTPLUG_LEGACY_SIZE 0x000f
+
 typedef struct AcpiPciHpPciStatus {
     uint32_t up; /* deprecated, maintained for migration compatibility */
     uint32_t down;
@@ -48,8 +52,8 @@ typedef struct AcpiPciHpState {
     MemoryRegion io;
 } AcpiPciHpState;
 
-void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root,
-                     MemoryRegion *address_space_io);
+void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root, uint16_t io_base,
+                     uint16_t io_size, MemoryRegion *address_space_io);
 
 /* Invoke on device hotplug */
 int acpi_pcihp_device_hotplug(AcpiPciHpState *, PCIDevice *,
-- 
1.7.1

Re: [Qemu-devel] [PATCH 3/4] hw:acpi:pcihp: assume root PCI bus if bus has no ACPI_PCIHP_PROP_BSEL property

[Michael S. Tsirkin]

On Tue, Jan 21, 2014 at 03:40:05PM +0100, Igor Mammedov wrote:
> when running with machine types older than 1.7 (i.e. without ACPI
> builtin tables), PCI bus won't have ACPI_PCIHP_PROP_BSEL property
> set.
> Taking in account that acpi hotplug handler in 1.7 and older
> machines is called only for root PCI bus, to make pcihp code
> compatible with legacy machine types assume that bus without
> ACPI_PCIHP_PROP_BSEL property has it equal to 0 and bail out
> if it's not root bus.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

I think that's not the best way to do this.
If bsel 0 *is* set on some bus, it should select it.
Fallback to bus 0 only if bsel value 0 is not set anywhere.
See e.g. how acpi_pcihp_find_hotplug_bus does it:

    if (!bsel && !find.bus) {
        find.bus = s->root;
    }

otherwise we introduce dependency on the logic that sets
bsel, this makes code fragile.

> ---
>  hw/acpi/pcihp.c |   10 +++-------
>  1 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 6d34fe9..76dce8d 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -63,14 +63,10 @@ static int acpi_pcihp_get_bsel(PCIBus *bus)
>  {
>      QObject *o = object_property_get_qobject(OBJECT(bus),
>                                               ACPI_PCIHP_PROP_BSEL, NULL);
> -    int64_t bsel = -1;
>      if (o) {
> -        bsel = qint_get_int(qobject_to_qint(o));
> +        return qint_get_int(qobject_to_qint(o));
>      }
> -    if (bsel < 0) {
> -        return -1;
> -    }
> -    return bsel;
> +    return 0;
>  }
>  
>  static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
> @@ -190,7 +186,7 @@ int acpi_pcihp_device_hotplug(AcpiPciHpState *s, PCIDevice *dev,
>  {
>      int slot = PCI_SLOT(dev->devfn);
>      int bsel = acpi_pcihp_get_bsel(dev->bus);
> -    if (bsel < 0) {
> +    if ((bsel == 0) && (dev->bus != s->root)) {
>          return -1;
>      }
>  
> -- 
> 1.7.1

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

[Michael S. Tsirkin]

On Tue, Jan 21, 2014 at 03:40:06PM +0100, Igor Mammedov wrote:
> reduces acpi PCI hotplug code duplication by 150LOC,
> and makes pcihp less dependend on piix specific code.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Actually pcihp was made separate intentionally so we
can cleanup the host/guest interface without
worrying about compatibility.

I posted that cleanup patch - I'm not against unifying both chunks of
code but could you put your patch on top?

> ---
>  hw/acpi/pcihp.c         |   10 +--
>  hw/acpi/piix4.c         |  196 +++++------------------------------------------
>  include/hw/acpi/pcihp.h |    8 ++-
>  3 files changed, 31 insertions(+), 183 deletions(-)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 76dce8d..25cc0fe 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -46,8 +46,6 @@
>  # define ACPI_PCIHP_DPRINTF(format, ...)     do { } while (0)
>  #endif
>  
> -#define PCI_HOTPLUG_ADDR 0xae00
> -#define PCI_HOTPLUG_SIZE 0x0014
>  #define PCI_UP_BASE 0x0000
>  #define PCI_DOWN_BASE 0x0004
>  #define PCI_EJ_BASE 0x0008
> @@ -279,14 +277,14 @@ static const MemoryRegionOps acpi_pcihp_io_ops = {
>      },
>  };
>  
> -void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus,
> -                     MemoryRegion *address_space_io)
> +void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus, uint16_t io_base,
> +                     uint16_t io_size, MemoryRegion *address_space_io)
>  {
>      s->root= root_bus;
>      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);
> +                          io_size);
> +    memory_region_add_subregion(address_space_io, io_base, &s->io);
>  }
>  
>  const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 5d55a3c..f818d76 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,8 +73,8 @@ typedef struct PIIX4PMState {
>      Notifier machine_ready;
>      Notifier powerdown_notifier;
>  
> -    /* for legacy pci hotplug (compatible with qemu 1.6 and older) */
> -    MemoryRegion io_pci;
> +    /* for legacy pci hotplug (compatible with qemu 1.6 and older)
> +     * used only for migration and updated in pre_save() */
>      struct pci_status pci0_status;
>      uint32_t pci0_hotplug_enable;
>      uint32_t pci0_slot_device_present;
> @@ -175,16 +168,24 @@ static void vmstate_pci_status_pre_save(void *opaque)
>      struct pci_status *pci0_status = opaque;
>      PIIX4PMState *s = container_of(pci0_status, PIIX4PMState, pci0_status);
>  
> +    pci0_status->down = s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].down;
>      /* 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;
> +    pci0_status->up =
> +        s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].device_present &
> +        s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].hotplug_enable;
>  }
>  
>  static int vmstate_acpi_post_load(void *opaque, int version_id)
>  {
>      PIIX4PMState *s = opaque;
>  
> +    if (!s->use_acpi_pci_hotplug) {
> +        s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].down =
> +            s->pci0_status.down;
> +    }
> +
>      pm_io_space_update(s);
>      return 0;
>  }
> @@ -304,60 +305,6 @@ static const VMStateDescription vmstate_acpi = {
>      }
>  };
>  
> -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 +324,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)
> @@ -409,6 +352,10 @@ static int piix4_acpi_pci_hotplug(DeviceState *qdev, PCIDevice *dev,
>  static void piix4_update_bus_hotplug(PCIBus *bus, void *opaque)
>  {
>      PIIX4PMState *s = opaque;
> +
> +    if (!s->use_acpi_pci_hotplug && (bus != s->acpi_pci_hotplug.root)) {
> +        return;
> +    }
>      pci_bus_hotplug(bus, piix4_acpi_pci_hotplug, DEVICE(s));
>  }
>  
> @@ -426,9 +373,7 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
>      pci_conf[0x67] = (memory_region_present(io_as, 0x3f8) ? 0x08 : 0) |
>          (memory_region_present(io_as, 0x2f8) ? 0x90 : 0);
>  
> -    if (s->use_acpi_pci_hotplug) {
> -        pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
> -    }
> +    pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
>  }
>  
>  static void piix4_pm_add_propeties(PIIX4PMState *s)
> @@ -621,60 +566,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 +575,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,55 +582,13 @@ 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);
> -    } 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, PIIX_PCI_HOTPLUG_ADDR,
> +                    s->use_acpi_pci_hotplug ? PIIX_PCI_HOTPLUG_SIZE :
> +                                              PIIX_PCI_HOTPLUG_LEGACY_SIZE,
> +                    parent);
>  
>      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 6230e60..6bd41bb 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -31,6 +31,10 @@
>  #include <qemu/typedefs.h>
>  #include "hw/pci/pci.h" /* for PCIHotplugState */
>  
> +#define PIIX_PCI_HOTPLUG_ADDR 0xae00
> +#define PIIX_PCI_HOTPLUG_SIZE 0x0014
> +#define PIIX_PCI_HOTPLUG_LEGACY_SIZE 0x000f
> +
>  typedef struct AcpiPciHpPciStatus {
>      uint32_t up; /* deprecated, maintained for migration compatibility */
>      uint32_t down;
> @@ -48,8 +52,8 @@ typedef struct AcpiPciHpState {
>      MemoryRegion io;
>  } AcpiPciHpState;
>  
> -void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root,
> -                     MemoryRegion *address_space_io);
> +void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root, uint16_t io_base,
> +                     uint16_t io_size, MemoryRegion *address_space_io);
>  
>  /* Invoke on device hotplug */
>  int acpi_pcihp_device_hotplug(AcpiPciHpState *, PCIDevice *,
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH 3/4] hw:acpi:pcihp: assume root PCI bus if bus has no ACPI_PCIHP_PROP_BSEL property

[Igor Mammedov]

On Sun, 26 Jan 2014 12:02:23 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Jan 21, 2014 at 03:40:05PM +0100, Igor Mammedov wrote:
> > when running with machine types older than 1.7 (i.e. without ACPI
> > builtin tables), PCI bus won't have ACPI_PCIHP_PROP_BSEL property
> > set.
> > Taking in account that acpi hotplug handler in 1.7 and older
> > machines is called only for root PCI bus, to make pcihp code
> > compatible with legacy machine types assume that bus without
> > ACPI_PCIHP_PROP_BSEL property has it equal to 0 and bail out
> > if it's not root bus.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> I think that's not the best way to do this.
> If bsel 0 *is* set on some bus, it should select it.
> Fallback to bus 0 only if bsel value 0 is not set anywhere.
> See e.g. how acpi_pcihp_find_hotplug_bus does it:
> 
>     if (!bsel && !find.bus) {
>         find.bus = s->root;
>     }
> 
> otherwise we introduce dependency on the logic that sets
> bsel, this makes code fragile.
Or to avoid touching PCIHP code, as an alternative
we can add BSEL property to root bus and set it to 0 when
running in compatibility mode (!use_acpi_pci_hotplug).

> 
> > ---
> >  hw/acpi/pcihp.c |   10 +++-------
> >  1 files changed, 3 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index 6d34fe9..76dce8d 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -63,14 +63,10 @@ static int acpi_pcihp_get_bsel(PCIBus *bus)
> >  {
> >      QObject *o = object_property_get_qobject(OBJECT(bus),
> >                                               ACPI_PCIHP_PROP_BSEL, NULL);
> > -    int64_t bsel = -1;
> >      if (o) {
> > -        bsel = qint_get_int(qobject_to_qint(o));
> > +        return qint_get_int(qobject_to_qint(o));
> >      }
> > -    if (bsel < 0) {
> > -        return -1;
> > -    }
> > -    return bsel;
> > +    return 0;
> >  }
> >  
> >  static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
> > @@ -190,7 +186,7 @@ int acpi_pcihp_device_hotplug(AcpiPciHpState *s, PCIDevice *dev,
> >  {
> >      int slot = PCI_SLOT(dev->devfn);
> >      int bsel = acpi_pcihp_get_bsel(dev->bus);
> > -    if (bsel < 0) {
> > +    if ((bsel == 0) && (dev->bus != s->root)) {
> >          return -1;
> >      }
> >  
> > -- 
> > 1.7.1

Re: [Qemu-devel] [PATCH 3/4] hw:acpi:pcihp: assume root PCI bus if bus has no ACPI_PCIHP_PROP_BSEL property

[Michael S. Tsirkin]

On Mon, Jan 27, 2014 at 03:01:12PM +0100, Igor Mammedov wrote:
> On Sun, 26 Jan 2014 12:02:23 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Jan 21, 2014 at 03:40:05PM +0100, Igor Mammedov wrote:
> > > when running with machine types older than 1.7 (i.e. without ACPI
> > > builtin tables), PCI bus won't have ACPI_PCIHP_PROP_BSEL property
> > > set.
> > > Taking in account that acpi hotplug handler in 1.7 and older
> > > machines is called only for root PCI bus, to make pcihp code
> > > compatible with legacy machine types assume that bus without
> > > ACPI_PCIHP_PROP_BSEL property has it equal to 0 and bail out
> > > if it's not root bus.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > I think that's not the best way to do this.
> > If bsel 0 *is* set on some bus, it should select it.
> > Fallback to bus 0 only if bsel value 0 is not set anywhere.
> > See e.g. how acpi_pcihp_find_hotplug_bus does it:
> > 
> >     if (!bsel && !find.bus) {
> >         find.bus = s->root;
> >     }
> > 
> > otherwise we introduce dependency on the logic that sets
> > bsel, this makes code fragile.
> Or to avoid touching PCIHP code, as an alternative
> we can add BSEL property to root bus and set it to 0 when
> running in compatibility mode (!use_acpi_pci_hotplug).

I didn't think too deeply about it, but on the surface
this seems fine too.

> > 
> > > ---
> > >  hw/acpi/pcihp.c |   10 +++-------
> > >  1 files changed, 3 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > index 6d34fe9..76dce8d 100644
> > > --- a/hw/acpi/pcihp.c
> > > +++ b/hw/acpi/pcihp.c
> > > @@ -63,14 +63,10 @@ static int acpi_pcihp_get_bsel(PCIBus *bus)
> > >  {
> > >      QObject *o = object_property_get_qobject(OBJECT(bus),
> > >                                               ACPI_PCIHP_PROP_BSEL, NULL);
> > > -    int64_t bsel = -1;
> > >      if (o) {
> > > -        bsel = qint_get_int(qobject_to_qint(o));
> > > +        return qint_get_int(qobject_to_qint(o));
> > >      }
> > > -    if (bsel < 0) {
> > > -        return -1;
> > > -    }
> > > -    return bsel;
> > > +    return 0;
> > >  }
> > >  
> > >  static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
> > > @@ -190,7 +186,7 @@ int acpi_pcihp_device_hotplug(AcpiPciHpState *s, PCIDevice *dev,
> > >  {
> > >      int slot = PCI_SLOT(dev->devfn);
> > >      int bsel = acpi_pcihp_get_bsel(dev->bus);
> > > -    if (bsel < 0) {
> > > +    if ((bsel == 0) && (dev->bus != s->root)) {
> > >          return -1;
> > >      }
> > >  
> > > -- 
> > > 1.7.1