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: NMichael S. Tsirkin <mst@redhat.com>
Signed-off-by: NIsaku Yamahata <yamahata@valinux.co.jp>

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 */

hw/pcie.c

@@ -140,37 +140,55 @@ void pcie_cap_deverr_reset(PCIDevice *dev)
                                  PCI_EXP_DEVCTL_FERE | PCI_EXP_DEVCTL_URRE);
 }
 
-/*
- * A PCI Express Hot-Plug Event has occured, so update slot status register
- * and notify OS of the event if necessary.
- *
- * 6.7.3 PCI Express Hot-Plug Events
- * 6.7.3.4 Software Notification of Hot-Plug Events
- */
-static void pcie_cap_slot_event(PCIDevice *dev, PCIExpressHotPlugEvent event)
+static void hotplug_event_update_event_status(PCIDevice *dev)
 {
-    uint8_t *exp_cap = dev->config + dev->exp.exp_cap;
+    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);
 
-    PCIE_DEV_PRINTF(dev,
-                    "sltctl: 0x%02"PRIx16" sltsta: 0x%02"PRIx16" event: %x\n",
-                    sltctl, sltsta, event);
+    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 (pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, event)) {
+    if (prev == dev->exp.hpev_notified) {
         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)) {
+    /* 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));
-        } else {
-            qemu_set_irq(dev->irq[dev->exp.hpev_intx], 1);
     }
+}
+
+/*
+ * A PCI Express Hot-Plug Event has occured, so update slot status register
+ * and notify OS of the event if necessary.
+ *
+ * 6.7.3 PCI Express Hot-Plug Events
+ * 6.7.3.4 Software Notification of Hot-Plug Events
+ */
+static void pcie_cap_slot_event(PCIDevice *dev, PCIExpressHotPlugEvent 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;
     }
+    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);

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);

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 */