[Qemu-devel] [PATCH RFC] pcie: clean up hot plug notification

[Michael S. Tsirkin]

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.

Untested.

Signed-off-by: Michael S. Tsirkin <address@hidden>
---
 hw/ioh3420.c            |    5 +--
 hw/pcie.c               |   79 ++++++++++++++++++++++------------------------
 hw/pcie.h               |    8 +++-
 hw/xio3130_downstream.c |    5 +--
 4 files changed, 46 insertions(+), 51 deletions(-)

diff --git a/hw/ioh3420.c b/hw/ioh3420.c
index 1f340d3..23aecbf 100644
--- 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 */
 }
 
diff --git a/hw/pcie.c b/hw/pcie.c
index c972337..74d2c18 100644
--- a/hw/pcie.c
+++ b/hw/pcie.c
@@ -155,20 +155,11 @@ static void pcie_cap_slot_event(PCIDevice *dev, 
PCIExpressHotPlugEvent event)
                     "sltctl: 0x%02"PRIx16" sltsta: 0x%02"PRIx16" event: %x\n",
                     sltctl, sltsta, event);
 
+    /* Minor optimization: if nothing changed - no event is needed. */
     if (pci_word_test_and_set_mask(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,
@@ -212,6 +203,36 @@ static int pcie_cap_slot_hotplug(DeviceState *qdev,
     return 0;
 }
 
+static void hotplug_event_notify(PCIDevice *dev)
+{
+    bool prev = dev->exp.hpev_notified;
+    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 & PCI_EXP_HP_EV_SUPPORTED);
+
+    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));
+        return;
+    }
+
+}
+
 /* pci express slot for pci express root/downstream port
    PCI express capability slot registers */
 void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
@@ -256,6 +277,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->cap.hpev_notified = false;
+
     pci_bus_hotplug(pci_bridge_get_sec_bus(DO_UPCAST(PCIBridge, dev, dev)),
                     pcie_cap_slot_hotplug, &dev->qdev);
 }
@@ -284,11 +307,12 @@ 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;
@@ -313,34 +337,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);
 
     /* command completion.
      * Real hardware might take a while to complete
diff --git a/hw/pcie.h b/hw/pcie.h
index 2871e27..39c6e47 100644
--- 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,7 @@ 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);
 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 a44e188..d46f911 100644
--- 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 */
 }
-- 
1.7.3-rc1