| Message ID | 20140126103709.GA8844@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Sun, 26 Jan 2014 12:37:09 +0200 "Michael S. Tsirkin" <mst@redhat.com> 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 <mst@redhat.com> > --- > 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?
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" <mst@redhat.com> 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 <mst@redhat.com> > > --- > > 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.
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;
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 <mst@redhat.com> --- hw/acpi/pcihp.c | 21 +++++---------------- include/hw/acpi/pcihp.h | 2 +- 2 files changed, 6 insertions(+), 17 deletions(-)