[Qemu-devel] [PATCH 0/3] piix_pci: optimize irq data path

[Qemu-devel] [PATCH 0/3] piix_pci: optimize irq data path

[Isaku Yamahata]

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.

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

 hw/pci.c      |    7 +++
 hw/pci.h      |    1 +
 hw/piix_pci.c |  126 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 3 files changed, 115 insertions(+), 19 deletions(-)

[Qemu-devel] [PATCH 1/3] pci: add accessor function to get irq levels

[Isaku Yamahata]

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 d6c5e66..67cb3d7 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 46b3ad3..f523722 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -234,6 +234,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);
 /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
 int pci_swizzle_map_irq_fn(void *opaque, PCIDevice *pci_dev, int pin);
-- 
1.7.1.1

[Qemu-devel] [PATCH 2/3] piix_pci: eliminate PIIX3State::pci_irq_levels

[Isaku Yamahata]

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

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/piix_pci.c |   31 +++++++++++++++++++++----------
 1 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 892c576..2d0ad9b 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -39,7 +39,7 @@ typedef PCIHostState I440FXState;
 
 typedef struct PIIX3State {
     PCIDevice dev;
-    int pci_irq_levels[4];
+    int32_t dummy_for_save_load_compat[4];
     qemu_irq *pic;
 } PIIX3State;
 
@@ -162,9 +162,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 < 4; i++) {
+            qemu_get_be32(f); /* dummy load for compatibility */
+        }
+    }
 
     return 0;
 }
@@ -256,8 +258,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 +266,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 +310,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->dummy_for_save_load_compat); i++) {
+        piix3->dummy_for_save_load_compat[i] =
+            pci_bus_get_irq_level(piix3->dev.bus, i);
+    }
 }
 
 static const VMStateDescription vmstate_piix3 = {
@@ -318,9 +328,10 @@ 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(dummy_for_save_load_compat, PIIX3State, 4, 3),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.7.1.1

[Qemu-devel] [PATCH 3/3] piix_pci: optimize set irq path

[Isaku Yamahata]

optimize irq routing in piix_pic.c which has been a TODO.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/piix_pci.c |  103 +++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 90 insertions(+), 13 deletions(-)

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 2d0ad9b..80ce205 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -29,6 +29,7 @@
 #include "isa.h"
 #include "sysbus.h"
 #include "range.h"
+#include "bitops.h"
 
 /*
  * I440FX chipset data sheet.
@@ -37,8 +38,32 @@
 
 typedef PCIHostState I440FXState;
 
+#define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
+#define PIIX_NUM_PIRQS          4       /* 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
+     */
+#define PIIX_PIC_LEVEL_MASK(pic_irq)                    \
+    (((uint64_t)PIIX_NUM_PIRQS - 1) << ((pic_irq) * PIIX_NUM_PIRQS))
+#define PIIX_PIC_LEVEL_BIT(pic_irq, pirq)               \
+    (1ULL << (((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;
+
     int32_t dummy_for_save_load_compat[4];
     qemu_irq *pic;
 } PIIX3State;
@@ -252,25 +277,66 @@ 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 & PIIX_PIC_LEVEL_MASK(pic_irq)));
+}
+
+static int piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level)
+{
+    int pic_irq;
+    uint64_t mask;
+
+    pic_irq = piix3->dev.config[PIIX_PIRQC + irq_num];
+    if (pic_irq >= PIIX_NUM_PIC_IRQS) {
+        return -1;
+    }
+
+    mask = PIIX_PIC_LEVEL_BIT(pic_irq, irq_num);
+    piix3->pic_levels &= ~mask;
+    piix3->pic_levels |= mask * !!level;
+    return pic_irq;
+}
 
 static void piix3_set_irq(void *opaque, int irq_num, int level)
 {
-    int i, pic_irq, pic_level;
     PIIX3State *piix3 = opaque;
+    int pic_irq;
+    pic_irq = piix3_set_irq_level(piix3, irq_num, level);
+    if (pic_irq >= 0) {
+        piix3_set_irq_pic(piix3, pic_irq);
+    }
+}
 
-    /* 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);
-            }
+static void piix3_reset_irq_levels(PIIX3State *piix3)
+{
+    piix3->pic_levels = 0;
+}
+
+/* irq routing is changed. so rebuild bitmap */
+static void piix3_rebuild_irq_levels(PIIX3State *piix3)
+{
+    int pirq;
+
+    piix3_reset_irq_levels(piix3);
+    for (pirq = 0; pirq < PIIX_NUM_PIRQS; pirq++) {
+        piix3_set_irq_level(piix3, pirq,
+                            pci_bus_get_irq_level(piix3->dev.bus, pirq));
+    }
+}
+
+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_rebuild_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);
     }
 }
 
@@ -310,6 +376,15 @@ static void piix3_reset(void *opaque)
     pci_conf[0xab] = 0x00;
     pci_conf[0xac] = 0x00;
     pci_conf[0xae] = 0x00;
+
+    piix3_reset_irq_levels(d);
+}
+
+static int piix3_post_load(void *opaque, int version_id)
+{
+    PIIX3State *piix3 = opaque;
+    piix3_rebuild_irq_levels(piix3);
+    return 0;
 }
 
 static void piix3_pre_save(void *opaque)
@@ -328,6 +403,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),
@@ -370,6 +446,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

[Qemu-devel] Re: [PATCH 3/3] piix_pci: optimize set irq path

[Michael S. Tsirkin]

On Thu, Mar 17, 2011 at 10:49:53PM +0900, Isaku Yamahata wrote:
> optimize irq routing in piix_pic.c which has been a TODO.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Some minor comments, and looks like load has a minor bug -
probably an old one as we didn't use to have a post load.

> ---
>  hw/piix_pci.c |  103 +++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 90 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 2d0ad9b..80ce205 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -29,6 +29,7 @@
>  #include "isa.h"
>  #include "sysbus.h"
>  #include "range.h"
> +#include "bitops.h"

still needed?

>  
>  /*
>   * I440FX chipset data sheet.
> @@ -37,8 +38,32 @@
>  
>  typedef PCIHostState I440FXState;
>  
> +#define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
> +#define PIIX_NUM_PIRQS          4       /* 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
> +     */
> +#define PIIX_PIC_LEVEL_MASK(pic_irq)                    \
> +    (((uint64_t)PIIX_NUM_PIRQS - 1) << ((pic_irq) * PIIX_NUM_PIRQS))
> +#define PIIX_PIC_LEVEL_BIT(pic_irq, pirq)               \
> +    (1ULL << (((pic_irq) * PIIX_NUM_PIRQS) + (pirq)))

I'll be happier with these open-coded: will be clearer
without all the () that macros need.
And we can make PIIX_NUM_PIRQS ULL so won't need a cast.
But not critical.

> +
> +#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;
> +
>      int32_t dummy_for_save_load_compat[4];
>      qemu_irq *pic;
>  } PIIX3State;
> @@ -252,25 +277,66 @@ 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 & PIIX_PIC_LEVEL_MASK(pic_irq)));
> +}
> +
> +static int piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level)
> +{
> +    int pic_irq;
> +    uint64_t mask;
> +
> +    pic_irq = piix3->dev.config[PIIX_PIRQC + irq_num];
> +    if (pic_irq >= PIIX_NUM_PIC_IRQS) {
> +        return -1;
> +    }
> +
> +    mask = PIIX_PIC_LEVEL_BIT(pic_irq, irq_num);
> +    piix3->pic_levels &= ~mask;
> +    piix3->pic_levels |= mask * !!level;
> +    return pic_irq;
> +}
>  
>  static void piix3_set_irq(void *opaque, int irq_num, int level)

I find the split between piix3_set_irq_level
and piix3_set_irq_level unintuitive.
It seems that we can just always call piix3_set_irq
and then piix3_set_irq_level can be inlined:
return instead of pic_irq >= 0 below.

>  {
> -    int i, pic_irq, pic_level;
>      PIIX3State *piix3 = opaque;
> +    int pic_irq;
> +    pic_irq = piix3_set_irq_level(piix3, irq_num, level);
> +    if (pic_irq >= 0) {
> +        piix3_set_irq_pic(piix3, pic_irq);
> +    }
> +}
>  
> -    /* 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);
> -            }
> +static void piix3_reset_irq_levels(PIIX3State *piix3)
> +{
> +    piix3->pic_levels = 0;
> +}
> +

Going overboard on the abstraction front IMO.

> +/* irq routing is changed. so rebuild bitmap */
> +static void piix3_rebuild_irq_levels(PIIX3State *piix3)

I iked _update_ better than rebuild, but not critical.

> +{
> +    int pirq;
> +
> +    piix3_reset_irq_levels(piix3);
> +    for (pirq = 0; pirq < PIIX_NUM_PIRQS; pirq++) {
> +        piix3_set_irq_level(piix3, pirq,
> +                            pci_bus_get_irq_level(piix3->dev.bus, pirq));
> +    }
> +}
> +
> +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_rebuild_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);
>      }
>  }
>  
> @@ -310,6 +376,15 @@ static void piix3_reset(void *opaque)
>      pci_conf[0xab] = 0x00;
>      pci_conf[0xac] = 0x00;
>      pci_conf[0xae] = 0x00;
> +
> +    piix3_reset_irq_levels(d);
> +}
> +
> +static int piix3_post_load(void *opaque, int version_id)
> +{
> +    PIIX3State *piix3 = opaque;
> +    piix3_rebuild_irq_levels(piix3);

Don't we need to set_irq_pic here as well?
And in that case, just make the for loop
part of piix3_rebuild_irq_levels.

> +    return 0;
>  }
>  
>  static void piix3_pre_save(void *opaque)
> @@ -328,6 +403,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),
> @@ -370,6 +446,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

[Qemu-devel] Re: [PATCH 3/3] piix_pci: optimize set irq path

[Isaku Yamahata]

On Thu, Mar 17, 2011 at 04:41:08PM +0200, Michael S. Tsirkin wrote:

> > +static int piix3_post_load(void *opaque, int version_id)
> > +{
> > +    PIIX3State *piix3 = opaque;
> > +    piix3_rebuild_irq_levels(piix3);
> 
> Don't we need to set_irq_pic here as well?
> And in that case, just make the for loop
> part of piix3_rebuild_irq_levels.

No, we need not. Because pic irq state is saved/loaded by pic itself.
So todo here is to make pic_levels consistent with other part.
-- 
yamahata

[Qemu-devel] Re: [PATCH 0/3] piix_pci: optimize irq data path

[Michael S. Tsirkin]

On Thu, Mar 17, 2011 at 10:49:50PM +0900, Isaku Yamahata wrote:
> 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.
> 
> Isaku Yamahata (3):
>   pci: add accessor function to get irq levels
>   piix_pci: eliminate PIIX3State::pci_irq_levels
>   piix_pci: optimize set irq path
> 
>  hw/pci.c      |    7 +++
>  hw/pci.h      |    1 +
>  hw/piix_pci.c |  126 ++++++++++++++++++++++++++++++++++++++++++++++++---------
>  3 files changed, 115 insertions(+), 19 deletions(-)

Thanks, I'll apply 1-2, 3 can be improved a bit but nothing major so
if you are busy I can apply as is and we can add fixups on top.
Let me know.

-- 
MST