pcie: clean up hot plug notification

Simplify logic for hotplug notification, by tracking state of the
logical interrupt condition.  We then simply use this variable to make
the interrupt decision, according to spec.

API is made cleaner as we no longer force users to pass in
old slot control value.

Includes fixes by Isaku Yamahata.

Signed-off-by: Michael S. Tsirkin <[email protected]>
Signed-off-by: Isaku Yamahata <[email protected]>

diff --git a/hw/ioh3420.c b/hw/ioh3420.c
index 1f340d32235c232d680b8297c53fc59d09c8db1e..3cc129f50b9bf2f1a4ff88f67d34cf6491a5afa5 100644 (file)
--- a/hw/ioh3420.c
+++ b/hw/ioh3420.c
@@ -39,12 +39,9 @@
 static void ioh3420_write_config(PCIDevice *d,
                                    uint32_t address, uint32_t val, int len)
 {
-    uint16_t sltctl =
-        pci_get_word(d->config + d->exp.exp_cap + PCI_EXP_SLTCTL);
-
     pci_bridge_write_config(d, address, val, len);
     msi_write_config(d, address, val, len);
-    pcie_cap_slot_write_config(d, address, val, len, sltctl);
+    pcie_cap_slot_write_config(d, address, val, len);
     /* TODO: AER */
 }
 
@@ -142,6 +139,7 @@ static const VMStateDescription vmstate_ioh3420 = {
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
+    .post_load = pcie_cap_slot_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_PCIE_DEVICE(port.br.dev, PCIESlot),
         /* TODO: AER */

diff --git a/hw/pcie.c b/hw/pcie.c
index bfccf5ec78b134848b8250f38a9bb7639b0a4428..373e33e7416a119d3cb298d3f68919a6d03946fe 100644 (file)
--- a/hw/pcie.c
+++ b/hw/pcie.c
@@ -140,6 +140,40 @@ void pcie_cap_deverr_reset(PCIDevice *dev)
                                  PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE);
 }
 
+static void hotplug_event_update_event_status(PCIDevice *dev)
+{
+    uint32_t pos = dev->exp.exp_cap;
+    uint8_t *exp_cap = dev->config + pos;
+    uint16_t sltctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL);
+    uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
+
+    dev->exp.hpev_notified = (sltctl & PCI_EXP_SLTCTL_HPIE) &&
+        (sltsta & sltctl & PCI_EXP_HP_EV_SUPPORTED);
+}
+
+static void hotplug_event_notify(PCIDevice *dev)
+{
+    bool prev = dev->exp.hpev_notified;
+
+    hotplug_event_update_event_status(dev);
+
+    if (prev == dev->exp.hpev_notified) {
+        return;
+    }
+
+    /* Note: the logic above does not take into account whether interrupts
+     * are masked. The result is that interrupt will be sent when it is
+     * subsequently unmasked. This appears to be legal: Section 6.7.3.4:
+     * The Port may optionally send an MSI when there are hot-plug events that
+     * occur while interrupt generation is disabled, and interrupt generation is
+     * subsequently enabled. */
+    if (!pci_msi_enabled(dev)) {
+        qemu_set_irq(dev->irq[dev->exp.hpev_intx], dev->exp.hpev_notified);
+    } else if (dev->exp.hpev_notified) {
+        pci_msi_notify(dev, pcie_cap_flags_get_vector(dev));
+    }
+}
+
 /*
  * A PCI Express Hot-Plug Event has occured, so update slot status register
  * and notify OS of the event if necessary.
@@ -149,28 +183,12 @@ void pcie_cap_deverr_reset(PCIDevice *dev)
  */
 static void pcie_cap_slot_event(PCIDevice *dev, PCIExpressHotPlugEvent event)
 {
-    uint8_t *exp_cap = dev->config + dev->exp.exp_cap;
-    uint16_t sltctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL);
-    uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
-
-    PCIE_DEV_PRINTF(dev,
-                    "sltctl: 0x%02"PRIx16" sltsta: 0x%02"PRIx16" event: %x\n",
-                    sltctl, sltsta, event);
-
-    if (pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, event)) {
+    /* Minor optimization: if nothing changed - no event is needed. */
+    if (pci_word_test_and_set_mask(dev->config + dev->exp.exp_cap +
+                                   PCI_EXP_SLTSTA, event)) {
         return;
     }
-    sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
-    PCIE_DEV_PRINTF(dev, "sltsta -> %02"PRIx16"\n", sltsta);
-
-    if ((sltctl & PCI_EXP_SLTCTL_HPIE) &&
-        (sltctl & event & PCI_EXP_HP_EV_SUPPORTED)) {
-        if (pci_msi_enabled(dev)) {
-            pci_msi_notify(dev, pcie_cap_flags_get_vector(dev));
-        } else {
-            qemu_set_irq(dev->irq[dev->exp.hpev_intx], 1);
-        }
-    }
+    hotplug_event_notify(dev);
 }
 
 static int pcie_cap_slot_hotplug(DeviceState *qdev,
@@ -258,6 +276,8 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
     pci_word_test_and_set_mask(dev->w1cmask + pos + PCI_EXP_SLTSTA,
                                PCI_EXP_HP_EV_SUPPORTED);
 
+    dev->exp.hpev_notified = false;
+
     pci_bus_hotplug(pci_bridge_get_sec_bus(DO_UPCAST(PCIBridge, dev, dev)),
                     pcie_cap_slot_hotplug, &dev->qdev);
 }
@@ -286,31 +306,21 @@ void pcie_cap_slot_reset(PCIDevice *dev)
                                  PCI_EXP_SLTSTA_CC |
                                  PCI_EXP_SLTSTA_PDC |
                                  PCI_EXP_SLTSTA_ABP);
+
+    hotplug_event_notify(dev);
 }
 
 void pcie_cap_slot_write_config(PCIDevice *dev,
-                                uint32_t addr, uint32_t val, int len,
-                                uint16_t sltctl_prev)
+                                uint32_t addr, uint32_t val, int len)
 {
     uint32_t pos = dev->exp.exp_cap;
     uint8_t *exp_cap = dev->config + pos;
-    uint16_t sltctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL);
     uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
 
     if (!ranges_overlap(addr, len, pos + PCI_EXP_SLTCTL, 2)) {
         return;
     }
 
-    PCIE_DEV_PRINTF(dev,
-                    "addr: 0x%"PRIx32" val: 0x%"PRIx32" len: %d\n"
-                    "\tsltctl_prev: 0x%02"PRIx16" sltctl: 0x%02"PRIx16
-                    " sltsta: 0x%02"PRIx16"\n",
-                    addr, val, len, sltctl_prev, sltctl, sltsta);
-
-    /* SLTCTL */
-    PCIE_DEV_PRINTF(dev, "sltctl: 0x%02"PRIx16" -> 0x%02"PRIx16"\n",
-                    sltctl_prev, sltctl);
-
     if (pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTCTL,
                                      PCI_EXP_SLTCTL_EIC)) {
         sltsta ^= PCI_EXP_SLTSTA_EIS; /* toggle PCI_EXP_SLTSTA_EIS bit */
@@ -320,34 +330,7 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
                         sltsta);
     }
 
-    /*
-     * The events control bits might be enabled or disabled,
-     * Check if the software notificastion condition is satisfied
-     * or disatisfied.
-     *
-     * 6.7.3.4 Software Notification of Hot-plug events
-     */
-    if (pci_msi_enabled(dev)) {
-        bool msi_trigger =
-            (sltctl & PCI_EXP_SLTCTL_HPIE) &&
-            ((sltctl_prev ^ sltctl) & sltctl & /* stlctl: 0 -> 1 */
-             sltsta & PCI_EXP_HP_EV_SUPPORTED);
-        if (msi_trigger) {
-            pci_msi_notify(dev, pcie_cap_flags_get_vector(dev));
-        }
-    } else {
-        int int_level =
-            (sltctl & PCI_EXP_SLTCTL_HPIE) &&
-            (sltctl & sltsta & PCI_EXP_HP_EV_SUPPORTED);
-        qemu_set_irq(dev->irq[dev->exp.hpev_intx], int_level);
-    }
-
-    if (!((sltctl_prev ^ sltctl) & PCI_EXP_SLTCTL_SUPPORTED)) {
-        PCIE_DEV_PRINTF(dev,
-                        "sprious command completion slctl "
-                        "0x%"PRIx16" -> 0x%"PRIx16"\n",
-                        sltctl_prev, sltctl);
-    }
+    hotplug_event_notify(dev);
 
     /* 
      * 6.7.3.2 Command Completed Events
@@ -368,6 +351,13 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
     pcie_cap_slot_event(dev, PCI_EXP_HP_EV_CCI);
 }
 
+int pcie_cap_slot_post_load(void *opaque, int version_id)
+{
+    PCIDevice *dev = opaque;
+    hotplug_event_update_event_status(dev);
+    return 0;
+}
+
 void pcie_cap_slot_push_attention_button(PCIDevice *dev)
 {
     pcie_cap_slot_event(dev, PCI_EXP_HP_EV_ABP);

diff --git a/hw/pcie.h b/hw/pcie.h
index 2871e270120c2c66cdb5c01bc178970f9cf1cc62..87085041f263d36ca2b54a204e964c43059a7962 100644 (file)
--- a/hw/pcie.h
+++ b/hw/pcie.h
@@ -74,6 +74,11 @@ struct PCIExpressDevice {
                                  * also initialize it when loaded as
                                  * appropreately.
                                  */
+    bool hpev_notified; /* Logical AND of conditions for hot plug event.
+                         Following 6.7.3.4:
+                         Software Notification of Hot-Plug Events, an interrupt
+                         is sent whenever the logical and of these conditions
+                         transitions from false to true. */
 };
 
 /* PCI express capability helper functions */
@@ -89,8 +94,8 @@ void pcie_cap_deverr_reset(PCIDevice *dev);
 void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot);
 void pcie_cap_slot_reset(PCIDevice *dev);
 void pcie_cap_slot_write_config(PCIDevice *dev,
-                                uint32_t addr, uint32_t val, int len,
-                                uint16_t sltctl_prev);
+                                uint32_t addr, uint32_t val, int len);
+int pcie_cap_slot_post_load(void *opaque, int version_id);
 void pcie_cap_slot_push_attention_button(PCIDevice *dev);
 
 void pcie_cap_root_init(PCIDevice *dev);

diff --git a/hw/xio3130_downstream.c b/hw/xio3130_downstream.c
index a44e188190921208afa2eb182ef55f7574594533..854eba8931723429da3ea0fd33e075b59910d978 100644 (file)
--- a/hw/xio3130_downstream.c
+++ b/hw/xio3130_downstream.c
@@ -38,12 +38,9 @@
 static void xio3130_downstream_write_config(PCIDevice *d, uint32_t address,
                                          uint32_t val, int len)
 {
-    uint16_t sltctl =
-        pci_get_word(d->config + d->exp.exp_cap + PCI_EXP_SLTCTL);
-
     pci_bridge_write_config(d, address, val, len);
     pcie_cap_flr_write_config(d, address, val, len);
-    pcie_cap_slot_write_config(d, address, val, len, sltctl);
+    pcie_cap_slot_write_config(d, address, val, len);
     msi_write_config(d, address, val, len);
     /* TODO: AER */
 }
@@ -144,6 +141,7 @@ static const VMStateDescription vmstate_xio3130_downstream = {
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
+    .post_load = pcie_cap_slot_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_PCIE_DEVICE(port.br.dev, PCIESlot),
         /* TODO: AER */