Re: [Qemu-devel] [PATCH] pcihp: reduce number of device check events

[Igor Mammedov]

On Sun, 26 Jan 2014 12:37:09 +0200
"Michael S. Tsirkin" <address@hidden> 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 <address@hidden>
> ---
>  hw/acpi/pcihp.c         | 21 +++++----------------
>  include/hw/acpi/pcihp.h |  2 +-
>  2 files changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 3fa3d7c..4345f5d 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -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:
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index 6230e60..aa297c2 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -32,7 +32,7 @@
>  #include "hw/pci/pci.h" /* for PCIHotplugState */
>  
>  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?