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

[Michael S. Tsirkin]

On Mon, Jan 27, 2014 at 01:48:44PM +0100, Igor Mammedov wrote:
> 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?

I agree, will do in a follow-up patch unless there's need to respin this
one.