qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v6 0/4] piix_pci: optimize irq data path
@ 2011-03-28 15:05 Isaku Yamahata
  2011-03-28 15:05 ` [Qemu-devel] [PATCH v6 1/4] pci: add accessor function to get irq levels Isaku Yamahata
                   ` (5 more replies)
  0 siblings, 6 replies; 8+ messages in thread
From: Isaku Yamahata @ 2011-03-28 15:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: yamahata, mst

Here is v6 which fixed piix3_set_irq_pic().

4/4 needs more extensive tests. So please feel free to pick it up now or
drop it for now.

patch description:
This patch series optimizes irq data path of piix_pci.
So far piix3 tracks each pirq level and checks whether a given pic pins is
asserted by seeing if each pirq is mapped into the pic pin.
This is independent on irq routing, but data path is on slow path.

Given that irq routing is rarely changed and asserting pic pins is on
data path, the path that asserts pic pins should be optimized and
chainging irq routing should be on slow path.
The new behavior with this patch series is to use bitmap which is addressed
by pirq and pic pins with a given irq routing.
When pirq is asserted, the bitmap is set and see if the pic pins is
asserted by checking the bitmaps.
When irq routing is changed, rebuild the bitmap and re-assert pic pins.

Changes v5 -> v6:
- fixed piix3_set_irq_pic()

Changes v4 -> v5:
- typo

Changes v3 -> v4:
- use pirq, pci_intx instead of irq_num in piix_pci.c
- use symbolic constant PIC_NUM_PINS
- introduced new patch 4/4 which cleans up a bit.

Changes v2 -> v3:
- s/dummy_for_save_load_compat/pci_irq_levels_vmstate/g
- move down unused member of pci_irq_levels_vmstate in the structure
  for cache efficiency

Changes v1 -> v2:
- addressed review comments.

Isaku Yamahata (4):
  pci: add accessor function to get irq levels
  piix_pci: eliminate PIIX3State::pci_irq_levels
  piix_pci: optimize set irq path
  piix_pci: load path clean up

 hw/pci.c      |    7 +++
 hw/pci.h      |    1 +
 hw/piix_pci.c |  129 ++++++++++++++++++++++++++++++++++++++++++++++-----------
 3 files changed, 112 insertions(+), 25 deletions(-)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH v6 1/4] pci: add accessor function to get irq levels
  2011-03-28 15:05 [Qemu-devel] [PATCH v6 0/4] piix_pci: optimize irq data path Isaku Yamahata
@ 2011-03-28 15:05 ` Isaku Yamahata
  2011-03-28 15:05 ` [Qemu-devel] [PATCH v6 2/4] piix_pci: eliminate PIIX3State::pci_irq_levels Isaku Yamahata
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Isaku Yamahata @ 2011-03-28 15:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: yamahata, mst

Introduce accessor function to know INTx levels.
It will be used later by q35.
Although piix_pci tracks the intx line levels, it can be eliminated
by this helper function.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |    7 +++++++
 hw/pci.h |    1 +
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 8b76cea..6ad3f10 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -126,6 +126,13 @@ static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change)
     bus->set_irq(bus->irq_opaque, irq_num, bus->irq_count[irq_num] != 0);
 }
 
+int pci_bus_get_irq_level(PCIBus *bus, int irq_num)
+{
+    assert(irq_num >= 0);
+    assert(irq_num < bus->nirq);
+    return !!bus->irq_count[irq_num];
+}
+
 /* Update interrupt status bit in config space on interrupt
  * state change. */
 static void pci_update_irq_status(PCIDevice *dev)
diff --git a/hw/pci.h b/hw/pci.h
index 113e556..092a463 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -233,6 +233,7 @@ void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
 PCIBus *pci_bus_new(DeviceState *parent, const char *name, int devfn_min);
 void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                   void *irq_opaque, int nirq);
+int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
 void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
 PCIBus *pci_register_bus(DeviceState *parent, const char *name,
                          pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
-- 
1.7.1.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH v6 2/4] piix_pci: eliminate PIIX3State::pci_irq_levels
  2011-03-28 15:05 [Qemu-devel] [PATCH v6 0/4] piix_pci: optimize irq data path Isaku Yamahata
  2011-03-28 15:05 ` [Qemu-devel] [PATCH v6 1/4] pci: add accessor function to get irq levels Isaku Yamahata
@ 2011-03-28 15:05 ` Isaku Yamahata
  2011-03-28 15:05 ` [Qemu-devel] [PATCH v6 3/4] piix_pci: optimize set irq path Isaku Yamahata
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Isaku Yamahata @ 2011-03-28 15:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: yamahata, Juan Quintela, mst

PIIX3State::pci_irq_levels are redundant which is already tracked by
PCIBus layer. So eliminate them.

Cc: Juan Quintela <quintela@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
Changes v3 -> v4:
- use PCI_NUM_PINS instead of magic number 4

Changes v2 -> v3:
- rename member s/dummy_for_save_load_compat/pci_irq_levels_vmstate/g
---
 hw/piix_pci.c |   38 +++++++++++++++++++++++++++-----------
 1 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 358da58..35e420c 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -37,10 +37,14 @@
 
 typedef PCIHostState I440FXState;
 
+#define PIIX_NUM_PIRQS          4ULL    /* PIRQ[A-D] */
+
 typedef struct PIIX3State {
     PCIDevice dev;
-    int pci_irq_levels[4];
     qemu_irq *pic;
+
+    /* This member isn't used. Just for save/load compatibility */
+    int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
 } PIIX3State;
 
 struct PCII440FXState {
@@ -162,9 +166,11 @@ static int i440fx_load_old(QEMUFile* f, void *opaque, int version_id)
     i440fx_update_memory_mappings(d);
     qemu_get_8s(f, &d->smm_enabled);
 
-    if (version_id == 2)
-        for (i = 0; i < 4; i++)
-            d->piix3->pci_irq_levels[i] = qemu_get_be32(f);
+    if (version_id == 2) {
+        for (i = 0; i < PIIX_NUM_PIRQS; i++) {
+            qemu_get_be32(f); /* dummy load for compatibility */
+        }
+    }
 
     return 0;
 }
@@ -236,7 +242,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn, qemu_irq *
     piix3 = DO_UPCAST(PIIX3State, dev,
                       pci_create_simple_multifunction(b, -1, true, "PIIX3"));
     piix3->pic = pic;
-    pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3, 4);
+    pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3, PIIX_NUM_PIRQS);
     (*pi440fx_state)->piix3 = piix3;
 
     *piix3_devfn = piix3->dev.devfn;
@@ -256,8 +262,6 @@ static void piix3_set_irq(void *opaque, int irq_num, int level)
     int i, pic_irq, pic_level;
     PIIX3State *piix3 = opaque;
 
-    piix3->pci_irq_levels[irq_num] = level;
-
     /* now we change the pic irq level according to the piix irq mappings */
     /* XXX: optimize */
     pic_irq = piix3->dev.config[0x60 + irq_num];
@@ -266,8 +270,9 @@ static void piix3_set_irq(void *opaque, int irq_num, int level)
            to it */
         pic_level = 0;
         for (i = 0; i < 4; i++) {
-            if (pic_irq == piix3->dev.config[0x60 + i])
-                pic_level |= piix3->pci_irq_levels[i];
+            if (pic_irq == piix3->dev.config[0x60 + i]) {
+                pic_level |= pci_bus_get_irq_level(piix3->dev.bus, i);
+            }
         }
         qemu_set_irq(piix3->pic[pic_irq], pic_level);
     }
@@ -309,8 +314,17 @@ static void piix3_reset(void *opaque)
     pci_conf[0xab] = 0x00;
     pci_conf[0xac] = 0x00;
     pci_conf[0xae] = 0x00;
+}
 
-    memset(d->pci_irq_levels, 0, sizeof(d->pci_irq_levels));
+static void piix3_pre_save(void *opaque)
+{
+    int i;
+    PIIX3State *piix3 = opaque;
+
+    for (i = 0; i < ARRAY_SIZE(piix3->pci_irq_levels_vmstate); i++) {
+        piix3->pci_irq_levels_vmstate[i] =
+            pci_bus_get_irq_level(piix3->dev.bus, i);
+    }
 }
 
 static const VMStateDescription vmstate_piix3 = {
@@ -318,9 +332,11 @@ static const VMStateDescription vmstate_piix3 = {
     .version_id = 3,
     .minimum_version_id = 2,
     .minimum_version_id_old = 2,
+    .pre_save = piix3_pre_save,
     .fields      = (VMStateField []) {
         VMSTATE_PCI_DEVICE(dev, PIIX3State),
-        VMSTATE_INT32_ARRAY_V(pci_irq_levels, PIIX3State, 4, 3),
+        VMSTATE_INT32_ARRAY_V(pci_irq_levels_vmstate, PIIX3State,
+                              PIIX_NUM_PIRQS, 3),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.7.1.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH v6 3/4] piix_pci: optimize set irq path
  2011-03-28 15:05 [Qemu-devel] [PATCH v6 0/4] piix_pci: optimize irq data path Isaku Yamahata
  2011-03-28 15:05 ` [Qemu-devel] [PATCH v6 1/4] pci: add accessor function to get irq levels Isaku Yamahata
  2011-03-28 15:05 ` [Qemu-devel] [PATCH v6 2/4] piix_pci: eliminate PIIX3State::pci_irq_levels Isaku Yamahata
@ 2011-03-28 15:05 ` Isaku Yamahata
  2011-03-28 15:05 ` [Qemu-devel] [PATCH v6 4/4] piix_pci: load path clean up Isaku Yamahata
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Isaku Yamahata @ 2011-03-28 15:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: yamahata, mst

optimize irq routing in piix_pic.c which has been a TODO.
So far piix3 tracks each pirq level and checks whether a given pic pins is
asserted by seeing if each pirq is mapped into the pic pin.
This is independent on irq routing, but data path is on slow path.

Given that irq routing is rarely changed and asserting pic pins is on
data path, the path that asserts pic pins should be optimized and
chainging irq routing should be on slow path.
The new behavior with this patch series is to use bitmap which is addressed
by pirq and pic pins with a given irq routing.
When pirq is asserted, the bitmap is set and see if the pic pins is
asserted by checking the bitmaps.
When irq routing is changed, rebuild the bitmap and re-assert pic pins.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
Changes v4 -> v5:
- fix piix_set_irq_pic()

Changes v3 -> v4:
- replace irq_num with pirq or pci_intx

Changes v1 -> v2:
- some minor clean ups
- commit log message
---
 hw/piix_pci.c |  101 +++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 84 insertions(+), 17 deletions(-)

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 35e420c..7ffb821 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -37,10 +37,27 @@
 
 typedef PCIHostState I440FXState;
 
+#define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
 #define PIIX_NUM_PIRQS          4ULL    /* PIRQ[A-D] */
+#define PIIX_PIRQC              0x60
 
 typedef struct PIIX3State {
     PCIDevice dev;
+
+    /*
+     * bitmap to track pic levels.
+     * The pic level is the logical OR of all the PCI irqs mapped to it
+     * So one PIC level is tracked by PIIX_NUM_PIRQS bits.
+     *
+     * PIRQ is mapped to PIC pins, we track it by
+     * PIIX_NUM_PIRQS * PIIX_NUM_PIC_IRQS = 64 bits with
+     * pic_irq * PIIX_NUM_PIRQS + pirq
+     */
+#if PIIX_NUM_PIC_IRQS * PIIX_NUM_PIRQS > 64
+#error "unable to encode pic state in 64bit in pic_levels."
+#endif
+    uint64_t pic_levels;
+
     qemu_irq *pic;
 
     /* This member isn't used. Just for save/load compatibility */
@@ -59,16 +76,16 @@ struct PCII440FXState {
 #define I440FX_PAM_SIZE 7
 #define I440FX_SMRAM    0x72
 
-static void piix3_set_irq(void *opaque, int irq_num, int level);
+static void piix3_set_irq(void *opaque, int pirq, int level);
 
 /* return the global irq number corresponding to a given device irq
    pin. We could also use the bus number to have a more precise
    mapping. */
-static int pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
+static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
 {
     int slot_addend;
     slot_addend = (pci_dev->devfn >> 3) - 1;
-    return (irq_num + slot_addend) & 3;
+    return (pci_intx + slot_addend) & 3;
 }
 
 static void update_pam(PCII440FXState *d, uint32_t start, uint32_t end, int r)
@@ -256,25 +273,64 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn, qemu_irq *
 }
 
 /* PIIX3 PCI to ISA bridge */
+static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
+{
+    qemu_set_irq(piix3->pic[pic_irq],
+                 !!(piix3->pic_levels &
+                    (((1UL << PIIX_NUM_PIRQS) - 1) <<
+                     (pic_irq * PIIX_NUM_PIRQS))));
+}
 
-static void piix3_set_irq(void *opaque, int irq_num, int level)
+static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level,
+                                bool propagate)
+{
+    int pic_irq;
+    uint64_t mask;
+
+    pic_irq = piix3->dev.config[PIIX_PIRQC + pirq];
+    if (pic_irq >= PIIX_NUM_PIC_IRQS) {
+        return;
+    }
+
+    mask = 1ULL << ((pic_irq * PIIX_NUM_PIRQS) + pirq);
+    piix3->pic_levels &= ~mask;
+    piix3->pic_levels |= mask * !!level;
+
+    if (propagate) {
+        piix3_set_irq_pic(piix3, pic_irq);
+    }
+}
+
+static void piix3_set_irq(void *opaque, int pirq, int level)
 {
-    int i, pic_irq, pic_level;
     PIIX3State *piix3 = opaque;
+    piix3_set_irq_level(piix3, pirq, level, true);
+}
 
-    /* now we change the pic irq level according to the piix irq mappings */
-    /* XXX: optimize */
-    pic_irq = piix3->dev.config[0x60 + irq_num];
-    if (pic_irq < 16) {
-        /* The pic level is the logical OR of all the PCI irqs mapped
-           to it */
-        pic_level = 0;
-        for (i = 0; i < 4; i++) {
-            if (pic_irq == piix3->dev.config[0x60 + i]) {
-                pic_level |= pci_bus_get_irq_level(piix3->dev.bus, i);
-            }
+/* irq routing is changed. so rebuild bitmap */
+static void piix3_update_irq_levels(PIIX3State *piix3)
+{
+    int pirq;
+
+    piix3->pic_levels = 0;
+    for (pirq = 0; pirq < PIIX_NUM_PIRQS; pirq++) {
+        piix3_set_irq_level(piix3, pirq,
+                            pci_bus_get_irq_level(piix3->dev.bus, pirq),
+                            false);
+    }
+}
+
+static void piix3_write_config(PCIDevice *dev,
+                               uint32_t address, uint32_t val, int len)
+{
+    pci_default_write_config(dev, address, val, len);
+    if (ranges_overlap(address, len, PIIX_PIRQC, 4)) {
+        PIIX3State *piix3 = DO_UPCAST(PIIX3State, dev, dev);
+        int pic_irq;
+        piix3_update_irq_levels(piix3);
+        for (pic_irq = 0; pic_irq < PIIX_NUM_PIC_IRQS; pic_irq++) {
+            piix3_set_irq_pic(piix3, pic_irq);
         }
-        qemu_set_irq(piix3->pic[pic_irq], pic_level);
     }
 }
 
@@ -314,6 +370,15 @@ static void piix3_reset(void *opaque)
     pci_conf[0xab] = 0x00;
     pci_conf[0xac] = 0x00;
     pci_conf[0xae] = 0x00;
+
+    d->pic_levels = 0;
+}
+
+static int piix3_post_load(void *opaque, int version_id)
+{
+    PIIX3State *piix3 = opaque;
+    piix3_update_irq_levels(piix3);
+    return 0;
 }
 
 static void piix3_pre_save(void *opaque)
@@ -332,6 +397,7 @@ static const VMStateDescription vmstate_piix3 = {
     .version_id = 3,
     .minimum_version_id = 2,
     .minimum_version_id_old = 2,
+    .post_load = piix3_post_load,
     .pre_save = piix3_pre_save,
     .fields      = (VMStateField []) {
         VMSTATE_PCI_DEVICE(dev, PIIX3State),
@@ -375,6 +441,7 @@ static PCIDeviceInfo i440fx_info[] = {
         .qdev.no_user = 1,
         .no_hotplug   = 1,
         .init         = piix3_initfn,
+        .config_write = piix3_write_config,
     },{
         /* end of list */
     }
-- 
1.7.1.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH v6 4/4] piix_pci: load path clean up
  2011-03-28 15:05 [Qemu-devel] [PATCH v6 0/4] piix_pci: optimize irq data path Isaku Yamahata
                   ` (2 preceding siblings ...)
  2011-03-28 15:05 ` [Qemu-devel] [PATCH v6 3/4] piix_pci: optimize set irq path Isaku Yamahata
@ 2011-03-28 15:05 ` Isaku Yamahata
  2011-03-28 16:36   ` [Qemu-devel] " Michael S. Tsirkin
  2011-03-28 16:29 ` [Qemu-devel] Re: [PATCH v6 0/4] piix_pci: optimize irq data path Michael S. Tsirkin
  2011-03-28 16:41 ` Michael S. Tsirkin
  5 siblings, 1 reply; 8+ messages in thread
From: Isaku Yamahata @ 2011-03-28 15:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: yamahata, mst

The previous patch didn't change the behavior when load,
it resulted in ugly code. This patch cleans it up.

With this patch, pic irq lines are manipulated when loaded.
It is expected that it won't change the behaviour because
the interrupts are level: at the moment e.g. pci devices already
reassert interrupts on load.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
Changes v3 -> v4:
- newly introduced
- TODO: test more OSes, stress test with save/load, live-migration
---
 hw/piix_pci.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 7ffb821..5f0d92f 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -281,8 +281,7 @@ static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
                      (pic_irq * PIIX_NUM_PIRQS))));
 }
 
-static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level,
-                                bool propagate)
+static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level)
 {
     int pic_irq;
     uint64_t mask;
@@ -296,15 +295,13 @@ static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level,
     piix3->pic_levels &= ~mask;
     piix3->pic_levels |= mask * !!level;
 
-    if (propagate) {
-        piix3_set_irq_pic(piix3, pic_irq);
-    }
+    piix3_set_irq_pic(piix3, pic_irq);
 }
 
 static void piix3_set_irq(void *opaque, int pirq, int level)
 {
     PIIX3State *piix3 = opaque;
-    piix3_set_irq_level(piix3, pirq, level, true);
+    piix3_set_irq_level(piix3, pirq, level);
 }
 
 /* irq routing is changed. so rebuild bitmap */
@@ -315,8 +312,7 @@ static void piix3_update_irq_levels(PIIX3State *piix3)
     piix3->pic_levels = 0;
     for (pirq = 0; pirq < PIIX_NUM_PIRQS; pirq++) {
         piix3_set_irq_level(piix3, pirq,
-                            pci_bus_get_irq_level(piix3->dev.bus, pirq),
-                            false);
+                            pci_bus_get_irq_level(piix3->dev.bus, pirq));
     }
 }
 
-- 
1.7.1.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] Re: [PATCH v6 0/4] piix_pci: optimize irq data path
  2011-03-28 15:05 [Qemu-devel] [PATCH v6 0/4] piix_pci: optimize irq data path Isaku Yamahata
                   ` (3 preceding siblings ...)
  2011-03-28 15:05 ` [Qemu-devel] [PATCH v6 4/4] piix_pci: load path clean up Isaku Yamahata
@ 2011-03-28 16:29 ` Michael S. Tsirkin
  2011-03-28 16:41 ` Michael S. Tsirkin
  5 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2011-03-28 16:29 UTC (permalink / raw)
  To: Isaku Yamahata; +Cc: qemu-devel

On Tue, Mar 29, 2011 at 12:05:26AM +0900, Isaku Yamahata wrote:
> Here is v6 which fixed piix3_set_irq_pic().
> 
> 4/4 needs more extensive tests. So please feel free to pick it up now or
> drop it for now.

Could you pls clarify how did you test and which patches?

> patch description:
> This patch series optimizes irq data path of piix_pci.
> So far piix3 tracks each pirq level and checks whether a given pic pins is
> asserted by seeing if each pirq is mapped into the pic pin.
> This is independent on irq routing, but data path is on slow path.
> 
> Given that irq routing is rarely changed and asserting pic pins is on
> data path, the path that asserts pic pins should be optimized and
> chainging irq routing should be on slow path.
> The new behavior with this patch series is to use bitmap which is addressed
> by pirq and pic pins with a given irq routing.
> When pirq is asserted, the bitmap is set and see if the pic pins is
> asserted by checking the bitmaps.
> When irq routing is changed, rebuild the bitmap and re-assert pic pins.
> 
> Changes v5 -> v6:
> - fixed piix3_set_irq_pic()
> 
> Changes v4 -> v5:
> - typo
> 
> Changes v3 -> v4:
> - use pirq, pci_intx instead of irq_num in piix_pci.c
> - use symbolic constant PIC_NUM_PINS
> - introduced new patch 4/4 which cleans up a bit.
> 
> Changes v2 -> v3:
> - s/dummy_for_save_load_compat/pci_irq_levels_vmstate/g
> - move down unused member of pci_irq_levels_vmstate in the structure
>   for cache efficiency
> 
> Changes v1 -> v2:
> - addressed review comments.
> 
> Isaku Yamahata (4):
>   pci: add accessor function to get irq levels
>   piix_pci: eliminate PIIX3State::pci_irq_levels
>   piix_pci: optimize set irq path
>   piix_pci: load path clean up
> 
>  hw/pci.c      |    7 +++
>  hw/pci.h      |    1 +
>  hw/piix_pci.c |  129 ++++++++++++++++++++++++++++++++++++++++++++++-----------
>  3 files changed, 112 insertions(+), 25 deletions(-)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] Re: [PATCH v6 4/4] piix_pci: load path clean up
  2011-03-28 15:05 ` [Qemu-devel] [PATCH v6 4/4] piix_pci: load path clean up Isaku Yamahata
@ 2011-03-28 16:36   ` Michael S. Tsirkin
  0 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2011-03-28 16:36 UTC (permalink / raw)
  To: Isaku Yamahata; +Cc: qemu-devel

On Tue, Mar 29, 2011 at 12:05:30AM +0900, Isaku Yamahata wrote:
> The previous patch didn't change the behavior when load,
> it resulted in ugly code. This patch cleans it up.
> 
> With this patch, pic irq lines are manipulated when loaded.
> It is expected that it won't change the behaviour because
> the interrupts are level: at the moment e.g. pci devices already
> reassert interrupts on load.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
> Changes v3 -> v4:
> - newly introduced
> - TODO: test more OSes, stress test with save/load, live-migration

The patch looks good to me.
You plan to do some of this testing?

> ---
>  hw/piix_pci.c |   12 ++++--------
>  1 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 7ffb821..5f0d92f 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -281,8 +281,7 @@ static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
>                       (pic_irq * PIIX_NUM_PIRQS))));
>  }
>  
> -static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level,
> -                                bool propagate)
> +static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level)
>  {
>      int pic_irq;
>      uint64_t mask;
> @@ -296,15 +295,13 @@ static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level,
>      piix3->pic_levels &= ~mask;
>      piix3->pic_levels |= mask * !!level;
>  
> -    if (propagate) {
> -        piix3_set_irq_pic(piix3, pic_irq);
> -    }
> +    piix3_set_irq_pic(piix3, pic_irq);
>  }
>  
>  static void piix3_set_irq(void *opaque, int pirq, int level)
>  {
>      PIIX3State *piix3 = opaque;
> -    piix3_set_irq_level(piix3, pirq, level, true);
> +    piix3_set_irq_level(piix3, pirq, level);
>  }
>  
>  /* irq routing is changed. so rebuild bitmap */
> @@ -315,8 +312,7 @@ static void piix3_update_irq_levels(PIIX3State *piix3)
>      piix3->pic_levels = 0;
>      for (pirq = 0; pirq < PIIX_NUM_PIRQS; pirq++) {
>          piix3_set_irq_level(piix3, pirq,
> -                            pci_bus_get_irq_level(piix3->dev.bus, pirq),
> -                            false);
> +                            pci_bus_get_irq_level(piix3->dev.bus, pirq));
>      }
>  }
>  
> -- 
> 1.7.1.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] Re: [PATCH v6 0/4] piix_pci: optimize irq data path
  2011-03-28 15:05 [Qemu-devel] [PATCH v6 0/4] piix_pci: optimize irq data path Isaku Yamahata
                   ` (4 preceding siblings ...)
  2011-03-28 16:29 ` [Qemu-devel] Re: [PATCH v6 0/4] piix_pci: optimize irq data path Michael S. Tsirkin
@ 2011-03-28 16:41 ` Michael S. Tsirkin
  5 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2011-03-28 16:41 UTC (permalink / raw)
  To: Isaku Yamahata; +Cc: qemu-devel

On Tue, Mar 29, 2011 at 12:05:26AM +0900, Isaku Yamahata wrote:
> Here is v6 which fixed piix3_set_irq_pic().
> 
> 4/4 needs more extensive tests. So please feel free to pick it up now or
> drop it for now.
> 
> patch description:
> This patch series optimizes irq data path of piix_pci.
> So far piix3 tracks each pirq level and checks whether a given pic pins is
> asserted by seeing if each pirq is mapped into the pic pin.
> This is independent on irq routing, but data path is on slow path.
> 
> Given that irq routing is rarely changed and asserting pic pins is on
> data path, the path that asserts pic pins should be optimized and
> chainging irq routing should be on slow path.
> The new behavior with this patch series is to use bitmap which is addressed
> by pirq and pic pins with a given irq routing.
> When pirq is asserted, the bitmap is set and see if the pic pins is
> asserted by checking the bitmaps.
> When irq routing is changed, rebuild the bitmap and re-assert pic pins.

This doesn't apply on top of my pci branch.
I;ve pushed that out, could you rebase to that please?
Also pls let me know what was tested.

> Changes v5 -> v6:
> - fixed piix3_set_irq_pic()
> 
> Changes v4 -> v5:
> - typo
> 
> Changes v3 -> v4:
> - use pirq, pci_intx instead of irq_num in piix_pci.c
> - use symbolic constant PIC_NUM_PINS
> - introduced new patch 4/4 which cleans up a bit.
> 
> Changes v2 -> v3:
> - s/dummy_for_save_load_compat/pci_irq_levels_vmstate/g
> - move down unused member of pci_irq_levels_vmstate in the structure
>   for cache efficiency
> 
> Changes v1 -> v2:
> - addressed review comments.
> 
> Isaku Yamahata (4):
>   pci: add accessor function to get irq levels
>   piix_pci: eliminate PIIX3State::pci_irq_levels
>   piix_pci: optimize set irq path
>   piix_pci: load path clean up
> 
>  hw/pci.c      |    7 +++
>  hw/pci.h      |    1 +
>  hw/piix_pci.c |  129 ++++++++++++++++++++++++++++++++++++++++++++++-----------
>  3 files changed, 112 insertions(+), 25 deletions(-)

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2011-03-28 16:41 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-28 15:05 [Qemu-devel] [PATCH v6 0/4] piix_pci: optimize irq data path Isaku Yamahata
2011-03-28 15:05 ` [Qemu-devel] [PATCH v6 1/4] pci: add accessor function to get irq levels Isaku Yamahata
2011-03-28 15:05 ` [Qemu-devel] [PATCH v6 2/4] piix_pci: eliminate PIIX3State::pci_irq_levels Isaku Yamahata
2011-03-28 15:05 ` [Qemu-devel] [PATCH v6 3/4] piix_pci: optimize set irq path Isaku Yamahata
2011-03-28 15:05 ` [Qemu-devel] [PATCH v6 4/4] piix_pci: load path clean up Isaku Yamahata
2011-03-28 16:36   ` [Qemu-devel] " Michael S. Tsirkin
2011-03-28 16:29 ` [Qemu-devel] Re: [PATCH v6 0/4] piix_pci: optimize irq data path Michael S. Tsirkin
2011-03-28 16:41 ` 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).