[Qemu-devel] [PATCH] acpi_piix4: Add _STA support and use it to acknowledge hotplug

[Alex Williamson]

Our current PCI hotplug model assumes that guests are hotplug
capable.  Adding a device always works and we can ask for it
back with device_del.  This isn't always the case.  If the
guest doesn't support hotplug, adding a device renders the
device lost until the guest is shut down.  This is particularly
annoying when the device is an assigned device.

The Status method (_STA) is a standard way to evaluate a
device in ACPI.  By adding this to the slot object, we get a
nice interaction point where we can determine whether the
OSPM is responding to a PCI hotplug.  If the OSPM does not
trigger this callback in response to a hot add, we can
assume it is not hotplug capable, clear the pending add,
and allow device_del to skip the ACPI path and remove the
device directly.

In addition to non-hotplug aware guests, this also enables
regression testing where the remove is issued before the
guest is able to respond to the add request.  With a small
enough delay between add/del, the device is removed without
the guest ever touching it.

We don't currently have paths that make use of guest write
of the up/down fields or guest read of the eject field, so
we can re-use these and avoid adding new I/O port regions
for PCI hotplug.  Write to the "up" port becomes an
acknowledgment of the up bit and thus clears it.  Read
from the "eject" port becomes a bitmap of present slots
allowing _STA to determine it's return value.  Write to
the "down" port is unused.

_STA is also evaluated when ACPI namespace is initialized by
the guest.  Using this, we can determine if the guest is
running a bios with _STA support and only enable support
when it is.  This allows new qemu to work with old bios.  New
bios code is also able to probe for this support by testing
whether the "present" register reports any slots with present
devices.  If not, it can dynamically disable _STA, triggering
the above compatibility support.

Signed-off-by: Alex Williamson <address@hidden>
---

 hw/acpi_piix4.c |  177 +++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 118 insertions(+), 59 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 30d37f9..ea568b8 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -41,9 +41,10 @@
 #define GPE_BASE 0xafe0
 #define PROC_BASE 0xaf00
 #define GPE_LEN 4
-#define PCI_BASE 0xae00
-#define PCI_EJ_BASE 0xae08
-#define PCI_RMV_BASE 0xae0c
+#define PCI_UP_ACK 0xae00
+#define PCI_DOWN 0xae04
+#define PCI_PRES_EJ 0xae08
+#define PCI_HOTPLUG 0xae0c
 
 #define PIIX4_CPU_HOTPLUG_STATUS 4
 #define PIIX4_PCI_HOTPLUG_STATUS 2
@@ -80,6 +81,7 @@ typedef struct PIIX4PMState {
     struct gpe_regs gpe_cpu;
     struct pci_status pci0_status;
     uint32_t pci0_hotplug_enable;
+    bool bios_has_STA;
 } PIIX4PMState;
 
 static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s);
@@ -256,6 +258,24 @@ static const VMStateDescription vmstate_pci_status = {
     }
 };
 
+static bool bios_has_STA_needed(void *opaque)
+{
+    PIIX4PMState *s = opaque;
+
+    return s->bios_has_STA;
+}
+
+static const VMStateDescription vmstate_bios_has_STA = {
+    .name = "piix4_pm/bios_has_STA",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_BOOL(bios_has_STA, PIIX4PMState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_acpi = {
     .name = "piix4_pm",
     .version_id = 2,
@@ -274,6 +294,14 @@ static const VMStateDescription vmstate_acpi = {
         VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status,
                        struct pci_status),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection []) {
+        {
+            .vmsd = &vmstate_bios_has_STA,
+            .needed = bios_has_STA_needed,
+        }, {
+            /* empty */
+        }
     }
 };
 
@@ -472,49 +500,45 @@ static void gpe_writeb(void *opaque, uint32_t addr, 
uint32_t val)
     PIIX4_DPRINTF("gpe write %x <== %d\n", addr, val);
 }
 
-static uint32_t pcihotplug_read(void *opaque, uint32_t addr)
+static uint32_t pci_up_read(void *opaque, uint32_t addr)
 {
-    uint32_t val = 0;
-    struct pci_status *g = opaque;
-    switch (addr) {
-        case PCI_BASE:
-            val = g->up;
-            break;
-        case PCI_BASE + 4:
-            val = g->down;
-            break;
-        default:
-            break;
-    }
+    PIIX4PMState *s = opaque;
+    uint32_t val;
+
+    val = s->pci0_status.up;
 
-    PIIX4_DPRINTF("pcihotplug read %x == %x\n", addr, val);
+    PIIX4_DPRINTF("pci up read %x\n", val);
     return val;
 }
 
-static void pcihotplug_write(void *opaque, uint32_t addr, uint32_t val)
+static void pci_ack_write(void *opaque, uint32_t addr, uint32_t val)
 {
-    struct pci_status *g = opaque;
-    switch (addr) {
-        case PCI_BASE:
-            g->up = val;
-            break;
-        case PCI_BASE + 4:
-            g->down = val;
-            break;
-   }
+    PIIX4PMState *s = opaque;
 
-    PIIX4_DPRINTF("pcihotplug write %x <== %d\n", addr, val);
+    s->pci0_status.up &= ~val;
+    if (!s->bios_has_STA) {
+        s->bios_has_STA = true;
+    }
+
+    PIIX4_DPRINTF("pci ack write %x\n", val);
 }
 
-static uint32_t pciej_read(void *opaque, uint32_t addr)
+static uint32_t pci_down_read(void *opaque, uint32_t addr)
 {
-    PIIX4_DPRINTF("pciej read %x\n", addr);
-    return 0;
+    PIIX4PMState *s = opaque;
+    uint32_t val;
+
+    val = s->pci0_status.down;
+
+    PIIX4_DPRINTF("pci down read %x\n", val);
+    return val;
 }
 
-static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
+static void pci_ej_write(void *opaque, uint32_t addr, uint32_t val)
 {
-    BusState *bus = opaque;
+    PIIX4PMState *s = opaque;
+    PCIDevice *dev = &s->dev;
+    BusState *bus = qdev_get_parent_bus(&dev->qdev);
     DeviceState *qdev, *next;
     int slot = ffs(val) - 1;
 
@@ -523,23 +547,35 @@ static void pciej_write(void *opaque, uint32_t addr, 
uint32_t val)
         PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
         if (PCI_SLOT(dev->devfn) == slot && !pc->no_hotplug) {
             qdev_free(qdev);
+            s->pci0_status.down &= ~val;
         }
     }
 
-
-    PIIX4_DPRINTF("pciej write %x <== %d\n", addr, val);
+    PIIX4_DPRINTF("pci ej write %x\n", val);
 }
-
-static uint32_t pcirmv_read(void *opaque, uint32_t addr)
+static uint32_t pci_present_read(void *opaque, uint32_t addr)
 {
     PIIX4PMState *s = opaque;
+    PCIDevice *dev = &s->dev;
+    BusState *bus = qdev_get_parent_bus(&dev->qdev);
+    DeviceState *qdev, *next;
+    uint32_t val = 0;
 
-    return s->pci0_hotplug_enable;
+    QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) {
+        PCIDevice *dev = PCI_DEVICE(qdev);
+        val |= (1 << PCI_SLOT(dev->devfn));
+    }
+
+    PIIX4_DPRINTF("pci present read %x\n", val);
+
+    return val;
 }
 
-static void pcirmv_write(void *opaque, uint32_t addr, uint32_t val)
+static uint32_t pci_hotpluggable_read(void *opaque, uint32_t addr)
 {
-    return;
+    PIIX4PMState *s = opaque;
+
+    return s->pci0_hotplug_enable;
 }
 
 extern const char *global_cpu_model;
@@ -549,7 +585,6 @@ static int piix4_device_hotplug(DeviceState *qdev, 
PCIDevice *dev,
 
 static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
 {
-    struct pci_status *pci0_status = &s->pci0_status;
     int i = 0, cpus = smp_cpus;
 
     while (cpus > 0) {
@@ -564,14 +599,15 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, 
PIIX4PMState *s)
     register_ioport_write(PROC_BASE, 32, 1, gpe_writeb, s);
     register_ioport_read(PROC_BASE, 32, 1,  gpe_readb, s);
 
-    register_ioport_write(PCI_BASE, 8, 4, pcihotplug_write, pci0_status);
-    register_ioport_read(PCI_BASE, 8, 4,  pcihotplug_read, pci0_status);
+    register_ioport_read(PCI_UP_ACK, 4, 4,  pci_up_read, s);
+    register_ioport_write(PCI_UP_ACK, 4, 4,  pci_ack_write, s);
 
-    register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, bus);
-    register_ioport_read(PCI_EJ_BASE, 4, 4,  pciej_read, bus);
+    register_ioport_read(PCI_DOWN, 4, 4,  pci_down_read, s);
 
-    register_ioport_write(PCI_RMV_BASE, 4, 4, pcirmv_write, s);
-    register_ioport_read(PCI_RMV_BASE, 4, 4,  pcirmv_read, s);
+    register_ioport_read(PCI_PRES_EJ, 4, 4,  pci_present_read, s);
+    register_ioport_write(PCI_PRES_EJ, 4, 4,  pci_ej_write, s);
+
+    register_ioport_read(PCI_HOTPLUG, 4, 4,  pci_hotpluggable_read, s);
 
     pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
 }
@@ -618,16 +654,45 @@ void qemu_system_cpu_hot_add(int cpu, int state)
 }
 #endif
 
-static void enable_device(PIIX4PMState *s, int slot)
+static int enable_device(PIIX4PMState *s, int slot)
 {
+    uint32_t mask = 1U << slot;
+
+    if ((s->pci0_status.up | s->pci0_status.down) & mask) {
+        return -1;
+    }
+
     s->gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
-    s->pci0_status.up |= (1 << slot);
+    s->pci0_status.up |= mask;
+
+    pm_update_sci(s);
+    return 0;
 }
 
-static void disable_device(PIIX4PMState *s, int slot)
+static int disable_device(PIIX4PMState *s, int slot)
 {
+    uint32_t mask = 1U << slot;
+
+    if (s->pci0_status.up & mask) {
+        if (s->bios_has_STA) {
+            PIIX4_DPRINTF("slot %d never acknowledged, removing directly\n",
+                          slot);
+            s->pci0_status.up &= ~mask;
+            pci_ej_write(s, 0, mask);
+            if (!(s->pci0_status.up | s->pci0_status.down)) {
+                s->gpe.sts[0] &= ~PIIX4_PCI_HOTPLUG_STATUS;
+            }
+            return 0;
+        } else {
+            s->pci0_status.up &= ~mask;
+        }
+    }
+
     s->gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
-    s->pci0_status.down |= (1 << slot);
+    s->pci0_status.down |= mask;
+
+    pm_update_sci(s);
+    return 0;
 }
 
 static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
@@ -644,15 +709,9 @@ static int piix4_device_hotplug(DeviceState *qdev, 
PCIDevice *dev,
         return 0;
     }
 
-    s->pci0_status.up = 0;
-    s->pci0_status.down = 0;
     if (state == PCI_HOTPLUG_ENABLED) {
-        enable_device(s, slot);
+        return enable_device(s, slot);
     } else {
-        disable_device(s, slot);
+        return disable_device(s, slot);
     }
-
-    pm_update_sci(s);
-
-    return 0;
 }