pcihp: reduce number of device check events

PIIX created a made-up value for the UP register since it was read by guest 32 times for each interrupt.
There's no reason to do this for the new PCIHP: register is only read once for each interrupt, so clean up code by making read act as an interrupt acknowledgement: the new UP register clear on read.

In this way we cut down the number of bus rescans by a factor of 32, and drop a bunch of code that's now unused.

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

---
 hw/acpi/pcihp.c         | 21 +++++----------------
 include/hw/acpi/pcihp.h |  2 +-
 2 files changed, 6 insertions(+), 17 deletions(-)

hw/acpi/pcihp.c +5 -16

index 3fa3d7c..4345f5d 100644
@@ -116,7 +116,6 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slo
 {
     BusChild *kid, *next;
     int slot = ffs(slots) - 1;
-    bool slot_free = true;
     PCIBus *bus = acpi_pcihp_find_hotplug_bus(s, bsel);
 
     if (!bus) {

@@ -125,21 +124,17 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slo
 
     /* Mark request as complete */
     s->acpi_pcihp_pci_status[bsel].down &= ~(1U << slot);
+    s->acpi_pcihp_pci_status[bsel].up &= ~(1U << slot);
 
     QTAILQ_FOREACH_SAFE(kid, &bus->qbus.children, sibling, next) {
         DeviceState *qdev = kid->child;
         PCIDevice *dev = PCI_DEVICE(qdev);
         if (PCI_SLOT(dev->devfn) == slot) {
-            if (acpi_pcihp_pc_no_hotplug(s, dev)) {
-                slot_free = false;
-            } else {
+            if (!acpi_pcihp_pc_no_hotplug(s, dev)) {
                 object_unparent(OBJECT(qdev));
             }
         }
     }
-    if (slot_free) {
-        s->acpi_pcihp_pci_status[bsel].device_present &= ~(1U << slot);
-    }
 }
 
 static void acpi_pcihp_update_hotplug_bus(AcpiPciHpState *s, int bsel)

@@ -153,7 +148,6 @@ static void acpi_pcihp_update_hotplug_bus(AcpiPciHpState *s, int bsel)
     }
 
     s->acpi_pcihp_pci_status[bsel].hotplug_enable = ~0;
-    s->acpi_pcihp_pci_status[bsel].device_present = 0;
 
     if (!bus) {
         return;

@@ -166,8 +160,6 @@ static void acpi_pcihp_update_hotplug_bus(AcpiPciHpState *s, int bsel)
         if (acpi_pcihp_pc_no_hotplug(s, pdev)) {
             s->acpi_pcihp_pci_status[bsel].hotplug_enable &= ~(1U << slot);
         }
-
-        s->acpi_pcihp_pci_status[bsel].device_present |= (1U << slot);
     }
 }
 

@@ -187,7 +179,7 @@ void acpi_pcihp_reset(AcpiPciHpState *s)
 
 static void enable_device(AcpiPciHpState *s, unsigned bsel, int slot)
 {
-    s->acpi_pcihp_pci_status[bsel].device_present |= (1U << slot);
+    s->acpi_pcihp_pci_status[bsel].up |= (1U << slot);
 }
 
 static void disable_device(AcpiPciHpState *s, unsigned bsel, int slot)

@@ -208,7 +200,6 @@ int acpi_pcihp_device_hotplug(AcpiPciHpState *s, PCIDevice *dev,
      * 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->acpi_pcihp_pci_status[bsel].device_present |= (1U << slot);
         return 0;
     }
 

@@ -233,10 +224,8 @@ static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size)
 
     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->acpi_pcihp_pci_status[bsel].device_present &
-            s->acpi_pcihp_pci_status[bsel].hotplug_enable;
+        val = s->acpi_pcihp_pci_status[bsel].up;
+        s->acpi_pcihp_pci_status[bsel].up = 0;
         ACPI_PCIHP_DPRINTF("pci_up_read %" PRIu32 "\n", val);
         break;
     case PCI_DOWN_BASE - PCI_HOTPLUG_ADDR:

include/hw/acpi/pcihp.h +0 -0

index 6230e60..aa297c2 100644

Quoted 106 lines · On Sun, 26 Jan 2014 12:37:09 +0200 "Michael S. Tsirkin" <[email protected]> wrote:

PIIX created a made-up value for the UP register since it was read by guest 32 times for each interrupt.
There's no reason to do this for the new PCIHP: register is only read once for each interrupt, so clean up code by making read act as an interrupt acknowledgement: the new UP register clear on read.

In this way we cut down the number of bus rescans by a factor of 32, and drop a bunch of code that's now unused.

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

---
 hw/acpi/pcihp.c         | 21 +++++----------------
 include/hw/acpi/pcihp.h |  2 +-
 2 files changed, 6 insertions(+), 17 deletions(-)

hw/acpi/pcihp.c +5 -16

index 3fa3d7c..4345f5d 100644
@@ -116,7 +116,6 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slo
 {
     BusChild *kid, *next;
     int slot = ffs(slots) - 1;
-    bool slot_free = true;
     PCIBus *bus = acpi_pcihp_find_hotplug_bus(s, bsel);
 
     if (!bus) {

@@ -125,21 +124,17 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slo
 
     /* Mark request as complete */
     s->acpi_pcihp_pci_status[bsel].down &= ~(1U << slot);
+    s->acpi_pcihp_pci_status[bsel].up &= ~(1U << slot);
 
     QTAILQ_FOREACH_SAFE(kid, &bus->qbus.children, sibling, next) {
         DeviceState *qdev = kid->child;
         PCIDevice *dev = PCI_DEVICE(qdev);
         if (PCI_SLOT(dev->devfn) == slot) {
-            if (acpi_pcihp_pc_no_hotplug(s, dev)) {
-                slot_free = false;
-            } else {
+            if (!acpi_pcihp_pc_no_hotplug(s, dev)) {
                 object_unparent(OBJECT(qdev));
             }
         }
     }
-    if (slot_free) {
-        s->acpi_pcihp_pci_status[bsel].device_present &= ~(1U << slot);
-    }
 }
 
 static void acpi_pcihp_update_hotplug_bus(AcpiPciHpState *s, int bsel)

@@ -153,7 +148,6 @@ static void acpi_pcihp_update_hotplug_bus(AcpiPciHpState *s, int bsel)
     }
 
     s->acpi_pcihp_pci_status[bsel].hotplug_enable = ~0;
-    s->acpi_pcihp_pci_status[bsel].device_present = 0;
 
     if (!bus) {
         return;

@@ -166,8 +160,6 @@ static void acpi_pcihp_update_hotplug_bus(AcpiPciHpState *s, int bsel)
         if (acpi_pcihp_pc_no_hotplug(s, pdev)) {
             s->acpi_pcihp_pci_status[bsel].hotplug_enable &= ~(1U << slot);
         }
-
-        s->acpi_pcihp_pci_status[bsel].device_present |= (1U << slot);
     }
 }
 

@@ -187,7 +179,7 @@ void acpi_pcihp_reset(AcpiPciHpState *s)
 
 static void enable_device(AcpiPciHpState *s, unsigned bsel, int slot)
 {
-    s->acpi_pcihp_pci_status[bsel].device_present |= (1U << slot);
+    s->acpi_pcihp_pci_status[bsel].up |= (1U << slot);
 }
 
 static void disable_device(AcpiPciHpState *s, unsigned bsel, int slot)

@@ -208,7 +200,6 @@ int acpi_pcihp_device_hotplug(AcpiPciHpState *s, PCIDevice *dev,
      * 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->acpi_pcihp_pci_status[bsel].device_present |= (1U << slot);
         return 0;
     }
 

@@ -233,10 +224,8 @@ static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size)
 
     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->acpi_pcihp_pci_status[bsel].device_present &
-            s->acpi_pcihp_pci_status[bsel].hotplug_enable;
+        val = s->acpi_pcihp_pci_status[bsel].up;
+        s->acpi_pcihp_pci_status[bsel].up = 0;
         ACPI_PCIHP_DPRINTF("pci_up_read %" PRIu32 "\n", val);
         break;
     case PCI_DOWN_BASE - PCI_HOTPLUG_ADDR:

include/hw/acpi/pcihp.h +0 -0

index 6230e60..aa297c2 100644
@@ -32,7 +32,7 @@
 #include "hw/pci/pci.h" /* for PCIHotplugState */
 

include/hw/acpi/pcihp.h line 38

Quoted 6 lines

include/hw/acpi/pcihp.h +1 -1

 typedef struct AcpiPciHpPciStatus {
-    uint32_t up; /* deprecated, maintained for migration compatibility */
+    uint32_t up;
     uint32_t down;
     uint32_t hotplug_enable;
     uint32_t device_present;

After patch field AcpiPciHpPciStatus.device_present seem to be unused, should it be deleted here as well?