* [Qemu-devel] [PATCH] pcihp: reduce number of device check events
@ 2014-01-26 10:37 Michael S. Tsirkin
2014-01-27 12:48 ` Igor Mammedov
0 siblings, 1 reply; 3+ messages in thread
From: Michael S. Tsirkin @ 2014-01-26 10:37 UTC (permalink / raw)
To: qemu-devel; +Cc: imammedo
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;
--
MST
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] pcihp: reduce number of device check events
2014-01-26 10:37 [Qemu-devel] [PATCH] pcihp: reduce number of device check events Michael S. Tsirkin
@ 2014-01-27 12:48 ` Igor Mammedov
2014-01-27 13:27 ` Michael S. Tsirkin
0 siblings, 1 reply; 3+ messages in thread
From: Igor Mammedov @ 2014-01-27 12:48 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
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?
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] pcihp: reduce number of device check events
2014-01-27 12:48 ` Igor Mammedov
@ 2014-01-27 13:27 ` Michael S. Tsirkin
0 siblings, 0 replies; 3+ messages in thread
From: Michael S. Tsirkin @ 2014-01-27 13:27 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-01-27 13:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-26 10:37 [Qemu-devel] [PATCH] pcihp: reduce number of device check events Michael S. Tsirkin
2014-01-27 12:48 ` Igor Mammedov
2014-01-27 13:27 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).