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

[Qemu-devel] [PATCH v3 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.

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 (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 |  119 +++++++++++++++++++++++++++++++++++++++++++++++---------
 3 files changed, 108 insertions(+), 19 deletions(-)

[Qemu-devel] [PATCH v3 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 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

[Qemu-devel] [PATCH v3 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: Juan Quintela <quintela@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
Changes v2 -> v3:
- rename member s/dummy_for_save_load_compat/pci_irq_levels_vmstate/g
---
 hw/piix_pci.c |   33 +++++++++++++++++++++++----------
 1 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 358da58..e88df43 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -39,8 +39,10 @@ typedef PCIHostState I440FXState;
 
 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[4];
 } PIIX3State;
 
 struct PCII440FXState {
@@ -162,9 +164,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 +260,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 +268,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 +312,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 +330,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(pci_irq_levels_vmstate, PIIX3State, 4, 3),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.7.1.1

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

[Isaku Yamahata]

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 v1 -> v2:
- some minor clean ups
- commit log message
---
 hw/piix_pci.c |   94 +++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 81 insertions(+), 13 deletions(-)

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index e88df43..f07e19d 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -37,8 +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 */
@@ -254,25 +273,63 @@ 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_NUM_PIRQS - 1) << (pic_irq * PIIX_NUM_PIRQS))));
+}
+
+static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level,
+                                bool propagate)
+{
+    int pic_irq;
+    uint64_t mask;
+
+    pic_irq = piix3->dev.config[PIIX_PIRQC + irq_num];
+    if (pic_irq >= PIIX_NUM_PIC_IRQS) {
+        return;
+    }
+
+    mask = 1ULL << ((pic_irq * PIIX_NUM_PIRQS) + irq_num);
+    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 irq_num, int level)
 {
-    int i, pic_irq, pic_level;
     PIIX3State *piix3 = opaque;
+    piix3_set_irq_level(piix3, irq_num, 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);
     }
 }
 
@@ -312,6 +369,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)
@@ -330,6 +396,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),
@@ -372,6 +439,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 v3 3/3] piix_pci: optimize set irq path

[Michael S. Tsirkin]

On Sat, Mar 19, 2011 at 10:24:52PM +0900, Isaku Yamahata wrote:
> 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 v1 -> v2:
> - some minor clean ups
> - commit log message
> ---
>  hw/piix_pci.c |   94 +++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 81 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index e88df43..f07e19d 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -37,8 +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 */
> @@ -254,25 +273,63 @@ 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_NUM_PIRQS - 1) << (pic_irq * PIIX_NUM_PIRQS))));
> +}
> +
> +static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level,
> +                                bool propagate)
> +{
> +    int pic_irq;
> +    uint64_t mask;
> +
> +    pic_irq = piix3->dev.config[PIIX_PIRQC + irq_num];
> +    if (pic_irq >= PIIX_NUM_PIC_IRQS) {
> +        return;
> +    }
> +
> +    mask = 1ULL << ((pic_irq * PIIX_NUM_PIRQS) + irq_num);
> +    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 irq_num, int level)
>  {
> -    int i, pic_irq, pic_level;
>      PIIX3State *piix3 = opaque;
> +    piix3_set_irq_level(piix3, irq_num, 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);
>          }

We wouldn't need this loop if piix3_update_irq_levels
called piix3_set_irq_level with propagate enabled.

> -        qemu_set_irq(piix3->pic[pic_irq], pic_level);
>      }
>  }
>  
> @@ -312,6 +369,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);

Couldn't figure out why would we not want to
propagate the interrupts here.
Could you explain please?
What happens if we do propagate them?
Nothing bad, right?

> +    return 0;
>  }
>  
>  static void piix3_pre_save(void *opaque)
> @@ -330,6 +396,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),
> @@ -372,6 +439,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 v3 3/3] piix_pci: optimize set irq path

[Isaku Yamahata]

On Mon, Mar 21, 2011 at 01:37:07PM +0200, Michael S. Tsirkin wrote:
> > +static int piix3_post_load(void *opaque, int version_id)
> > +{
> > +    PIIX3State *piix3 = opaque;
> > +    piix3_update_irq_levels(piix3);
> 
> Couldn't figure out why would we not want to
> propagate the interrupts here.
> Could you explain please?
> What happens if we do propagate them?
> Nothing bad, right?

I wanted to be just conservative.
If you are brave enough to change the behavior, I'm fine with propagating
interrupts.

If we propagate the interrupts, guest OS may see interrupts
unnecessarily/spuriously injected after load.
Probably such interrupts doesn't harm OSes, so there is nothing
bad in theory as you said.
On the other hand, I hesitated to change the existing behavior because
it would be very difficult to debug it and to test many OSes.
-- 
yamahata

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

[Isaku Yamahata]

On Mon, Mar 21, 2011 at 01:37:07PM +0200, Michael S. Tsirkin wrote:
> > +/* 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);
> >          }
> 
> We wouldn't need this loop if piix3_update_irq_levels
> called piix3_set_irq_level with propagate enabled.

No. piix3_update_irq_levels loops over PIRQs(= 4).
But we need to loop over all pic PINs(= 16) to raise/lower irqs.
-- 
yamahata

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

[Michael S. Tsirkin]

On Mon, Mar 21, 2011 at 09:10:32PM +0900, Isaku Yamahata wrote:
> On Mon, Mar 21, 2011 at 01:37:07PM +0200, Michael S. Tsirkin wrote:
> > > +static int piix3_post_load(void *opaque, int version_id)
> > > +{
> > > +    PIIX3State *piix3 = opaque;
> > > +    piix3_update_irq_levels(piix3);
> > 
> > Couldn't figure out why would we not want to
> > propagate the interrupts here.
> > Could you explain please?
> > What happens if we do propagate them?
> > Nothing bad, right?
> 
> I wanted to be just conservative.
> If you are brave enough to change the behavior, I'm fine with propagating
> interrupts.
> 
> If we propagate the interrupts, guest OS may see interrupts
> unnecessarily/spuriously injected after load.
> Probably such interrupts doesn't harm OSes, so there is nothing
> bad in theory as you said.
> On the other hand, I hesitated to change the existing behavior because
> it would be very difficult to debug it and to test many OSes.

I expect it won't change the behaviour because the interrupts
are level: at the moment e.g. pci devices already reassert
interrupts on load.

But I agree it better be a separate patch, and needs a lot of testing.

> -- 
> yamahata

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

[Isaku Yamahata]

On Mon, Mar 21, 2011 at 02:31:11PM +0200, Michael S. Tsirkin wrote:
> On Mon, Mar 21, 2011 at 09:10:32PM +0900, Isaku Yamahata wrote:
> > On Mon, Mar 21, 2011 at 01:37:07PM +0200, Michael S. Tsirkin wrote:
> > > > +static int piix3_post_load(void *opaque, int version_id)
> > > > +{
> > > > +    PIIX3State *piix3 = opaque;
> > > > +    piix3_update_irq_levels(piix3);
> > > 
> > > Couldn't figure out why would we not want to
> > > propagate the interrupts here.
> > > Could you explain please?
> > > What happens if we do propagate them?
> > > Nothing bad, right?
> > 
> > I wanted to be just conservative.
> > If you are brave enough to change the behavior, I'm fine with propagating
> > interrupts.
> > 
> > If we propagate the interrupts, guest OS may see interrupts
> > unnecessarily/spuriously injected after load.
> > Probably such interrupts doesn't harm OSes, so there is nothing
> > bad in theory as you said.
> > On the other hand, I hesitated to change the existing behavior because
> > it would be very difficult to debug it and to test many OSes.
> 
> I expect it won't change the behaviour because the interrupts
> are level: at the moment e.g. pci devices already reassert
> interrupts on load.
> 
> But I agree it better be a separate patch, and needs a lot of testing.

Like this?

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index f07e19d..8052c1e 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -280,8 +280,7 @@ static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
                     ((PIIX_NUM_PIRQS - 1) << (pic_irq * PIIX_NUM_PIRQS))));
 }
 
-static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level,
-                                bool propagate)
+static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level)
 {
     int pic_irq;
     uint64_t mask;
@@ -295,15 +294,13 @@ static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, 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 irq_num, int level)
 {
     PIIX3State *piix3 = opaque;
-    piix3_set_irq_level(piix3, irq_num, level, true);
+    piix3_set_irq_level(piix3, irq_num, level);
 }
 
 /* irq routing is changed. so rebuild bitmap */
@@ -314,8 +311,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));
     }
 }
 


-- 
yamahata

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

[Michael S. Tsirkin]

On Mon, Mar 21, 2011 at 09:56:56PM +0900, Isaku Yamahata wrote:
> On Mon, Mar 21, 2011 at 02:31:11PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Mar 21, 2011 at 09:10:32PM +0900, Isaku Yamahata wrote:
> > > On Mon, Mar 21, 2011 at 01:37:07PM +0200, Michael S. Tsirkin wrote:
> > > > > +static int piix3_post_load(void *opaque, int version_id)
> > > > > +{
> > > > > +    PIIX3State *piix3 = opaque;
> > > > > +    piix3_update_irq_levels(piix3);
> > > > 
> > > > Couldn't figure out why would we not want to
> > > > propagate the interrupts here.
> > > > Could you explain please?
> > > > What happens if we do propagate them?
> > > > Nothing bad, right?
> > > 
> > > I wanted to be just conservative.
> > > If you are brave enough to change the behavior, I'm fine with propagating
> > > interrupts.
> > > 
> > > If we propagate the interrupts, guest OS may see interrupts
> > > unnecessarily/spuriously injected after load.
> > > Probably such interrupts doesn't harm OSes, so there is nothing
> > > bad in theory as you said.
> > > On the other hand, I hesitated to change the existing behavior because
> > > it would be very difficult to debug it and to test many OSes.
> > 
> > I expect it won't change the behaviour because the interrupts
> > are level: at the moment e.g. pci devices already reassert
> > interrupts on load.
> > 
> > But I agree it better be a separate patch, and needs a lot of testing.
> 
> Like this?

Yes. But need to stress-test with migration, windows and linux guests
at least.

> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index f07e19d..8052c1e 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -280,8 +280,7 @@ static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
>                      ((PIIX_NUM_PIRQS - 1) << (pic_irq * PIIX_NUM_PIRQS))));
>  }
>  
> -static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level,
> -                                bool propagate)
> +static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level)
>  {
>      int pic_irq;
>      uint64_t mask;
> @@ -295,15 +294,13 @@ static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, 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 irq_num, int level)
>  {
>      PIIX3State *piix3 = opaque;
> -    piix3_set_irq_level(piix3, irq_num, level, true);
> +    piix3_set_irq_level(piix3, irq_num, level);
>  }
>  
>  /* irq routing is changed. so rebuild bitmap */
> @@ -314,8 +311,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));
>      }
>  }
>  
> 
> 
> -- 
> yamahata

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

[Michael S. Tsirkin]

On Sat, Mar 19, 2011 at 10:24:52PM +0900, Isaku Yamahata wrote:
> 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 v1 -> v2:
> - some minor clean ups
> - commit log message
> ---
>  hw/piix_pci.c |   94 +++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 81 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index e88df43..f07e19d 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -37,8 +37,27 @@
>  
>  typedef PCIHostState I440FXState;
>  
> +#define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
> +#define PIIX_NUM_PIRQS          4ULL    /* PIRQ[A-D] */

I've changed this to
((uint64_t)PCI_NUM_PINS)

Makes sense?

> +#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 */
> @@ -254,25 +273,63 @@ 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_NUM_PIRQS - 1) << (pic_irq * PIIX_NUM_PIRQS))));
> +}
> +
> +static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level,
> +                                bool propagate)
> +{
> +    int pic_irq;
> +    uint64_t mask;
> +
> +    pic_irq = piix3->dev.config[PIIX_PIRQC + irq_num];
> +    if (pic_irq >= PIIX_NUM_PIC_IRQS) {
> +        return;
> +    }
> +
> +    mask = 1ULL << ((pic_irq * PIIX_NUM_PIRQS) + irq_num);
> +    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 irq_num, int level)
>  {
> -    int i, pic_irq, pic_level;
>      PIIX3State *piix3 = opaque;
> +    piix3_set_irq_level(piix3, irq_num, 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);
>      }
>  }
>  
> @@ -312,6 +369,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)
> @@ -330,6 +396,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),
> @@ -372,6 +439,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 v3 3/3] piix_pci: optimize set irq path

[Isaku Yamahata]

On Mon, Mar 21, 2011 at 04:10:22PM +0200, Michael S. Tsirkin wrote:
> > @@ -37,8 +37,27 @@
> >  
> >  typedef PCIHostState I440FXState;
> >  
> > +#define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
> > +#define PIIX_NUM_PIRQS          4ULL    /* PIRQ[A-D] */
> 
> I've changed this to
> ((uint64_t)PCI_NUM_PINS)
> 
> Makes sense?

No. The number of pirq is independent of PCI_NUM_PINS.
For example, ich9 has 8 pirq (PIRQ[A-H]).
Probably same pin name (A-D) causes the confusion, they are irrelevant.
-- 
yamahata

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

[Michael S. Tsirkin]

On Tue, Mar 22, 2011 at 09:50:37AM +0900, Isaku Yamahata wrote:
> On Mon, Mar 21, 2011 at 04:10:22PM +0200, Michael S. Tsirkin wrote:
> > > @@ -37,8 +37,27 @@
> > >  
> > >  typedef PCIHostState I440FXState;
> > >  
> > > +#define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
> > > +#define PIIX_NUM_PIRQS          4ULL    /* PIRQ[A-D] */
> > 
> > I've changed this to
> > ((uint64_t)PCI_NUM_PINS)
> > 
> > Makes sense?
> 
> No. The number of pirq is independent of PCI_NUM_PINS.

OK, here's what confuses me:

> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index e88df43..f07e19d 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -37,8 +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 */
> @@ -254,25 +273,63 @@ 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_NUM_PIRQS - 1) << (pic_irq * PIIX_NUM_PIRQS))));
> +}
> +
> +static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level,
> +                                bool propagate)

This is called from piix3_set_irq_pic and gets INTX# (0 to PCI_NUM_PINS).

> +{
> +    int pic_irq;
> +    uint64_t mask;
> +
> +    pic_irq = piix3->dev.config[PIIX_PIRQC + irq_num];
> +    if (pic_irq >= PIIX_NUM_PIC_IRQS) {
> +        return;
> +    }
> +
> +    mask = 1ULL << ((pic_irq * PIIX_NUM_PIRQS) + irq_num);
> +    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 irq_num, int level)
>  {
> -    int i, pic_irq, pic_level;
>      PIIX3State *piix3 = opaque;
> +    piix3_set_irq_level(piix3, irq_num, 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);

Here it is called with PIRQ # 0 to PIIX_NUM_PIRQS.

> +    }
> +}
> +
> +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);
>      }
>  }
>  
> @@ -312,6 +369,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)
> @@ -330,6 +396,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),
> @@ -372,6 +439,7 @@ static PCIDeviceInfo i440fx_info[] = {
>          .qdev.no_user = 1,
>          .no_hotplug   = 1,
>          .init         = piix3_initfn,
> +        .config_write = piix3_write_config,
>      },{
>          /* end of list */
>      }
> 

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

[Isaku Yamahata]

On Tue, Mar 22, 2011 at 03:40:16PM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 22, 2011 at 09:50:37AM +0900, Isaku Yamahata wrote:
> > On Mon, Mar 21, 2011 at 04:10:22PM +0200, Michael S. Tsirkin wrote:
> > > > @@ -37,8 +37,27 @@
> > > >  
> > > >  typedef PCIHostState I440FXState;
> > > >  
> > > > +#define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
> > > > +#define PIIX_NUM_PIRQS          4ULL    /* PIRQ[A-D] */
> > > 
> > > I've changed this to
> > > ((uint64_t)PCI_NUM_PINS)
> > > 
> > > Makes sense?
> > 
> > No. The number of pirq is independent of PCI_NUM_PINS.
> 
> OK, here's what confuses me:
> 
> > diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> > index e88df43..f07e19d 100644
> > --- a/hw/piix_pci.c
> > +++ b/hw/piix_pci.c
> > @@ -37,8 +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 */
> > @@ -254,25 +273,63 @@ 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_NUM_PIRQS - 1) << (pic_irq * PIIX_NUM_PIRQS))));
> > +}
> > +
> > +static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level,
> > +                                bool propagate)
> 
> This is called from piix3_set_irq_pic and gets INTX# (0 to PCI_NUM_PINS).

I used irq_num some places because of the existing code and pci.c.
Here irq_num = PIRQ number. But irq_num = intx in pci_lost_get_pirq()
Hmm should I avoid irq_num totally, and use intx, pirq, pic_irq?
I hope the following clarifies the conversion.

A pci device asserts a interrupt pin of intx
      |
      V
pci_set_irq()
      |
      V
pci_slot_get_pirq() = bus->map_irq() gets (PCIDevice, intx)
  returns pirq
  This corresponds to how pci intx lines are connected to piix3 pirq pins
      |
      V
piix3_set_irq() = bus->set_irq() gets pirq
      |
      V
piix3_set_irq_levels() gets pirq
  converts pirq# into pic irq
  calls piix3_set_irq_pic() with pic_irq
      |
      V
piix3_set_irq_pic() gets pic_irq
      |
      V
   pic code


thanks,

> > +{
> > +    int pic_irq;
> > +    uint64_t mask;
> > +
> > +    pic_irq = piix3->dev.config[PIIX_PIRQC + irq_num];
> > +    if (pic_irq >= PIIX_NUM_PIC_IRQS) {
> > +        return;
> > +    }
> > +
> > +    mask = 1ULL << ((pic_irq * PIIX_NUM_PIRQS) + irq_num);
> > +    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 irq_num, int level)
> >  {
> > -    int i, pic_irq, pic_level;
> >      PIIX3State *piix3 = opaque;
> > +    piix3_set_irq_level(piix3, irq_num, 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);
> 
> Here it is called with PIRQ # 0 to PIIX_NUM_PIRQS.
> 
> > +    }
> > +}
> > +
> > +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);
> >      }
> >  }
> >  
> > @@ -312,6 +369,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)
> > @@ -330,6 +396,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),
> > @@ -372,6 +439,7 @@ static PCIDeviceInfo i440fx_info[] = {
> >          .qdev.no_user = 1,
> >          .no_hotplug   = 1,
> >          .init         = piix3_initfn,
> > +        .config_write = piix3_write_config,
> >      },{
> >          /* end of list */
> >      }
> > 
> 

-- 
yamahata

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

[Michael S. Tsirkin]

On Tue, Mar 22, 2011 at 11:07:03PM +0900, Isaku Yamahata wrote:
> On Tue, Mar 22, 2011 at 03:40:16PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Mar 22, 2011 at 09:50:37AM +0900, Isaku Yamahata wrote:
> > > On Mon, Mar 21, 2011 at 04:10:22PM +0200, Michael S. Tsirkin wrote:
> > > > > @@ -37,8 +37,27 @@
> > > > >  
> > > > >  typedef PCIHostState I440FXState;
> > > > >  
> > > > > +#define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
> > > > > +#define PIIX_NUM_PIRQS          4ULL    /* PIRQ[A-D] */
> > > > 
> > > > I've changed this to
> > > > ((uint64_t)PCI_NUM_PINS)
> > > > 
> > > > Makes sense?
> > > 
> > > No. The number of pirq is independent of PCI_NUM_PINS.
> > 
> > OK, here's what confuses me:
> > 
> > > diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> > > index e88df43..f07e19d 100644
> > > --- a/hw/piix_pci.c
> > > +++ b/hw/piix_pci.c
> > > @@ -37,8 +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 */
> > > @@ -254,25 +273,63 @@ 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_NUM_PIRQS - 1) << (pic_irq * PIIX_NUM_PIRQS))));
> > > +}
> > > +
> > > +static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level,
> > > +                                bool propagate)
> > 
> > This is called from piix3_set_irq_pic and gets INTX# (0 to PCI_NUM_PINS).
> 
> I used irq_num some places because of the existing code and pci.c.
> Here irq_num = PIRQ number. But irq_num = intx in pci_lost_get_pirq()
> Hmm should I avoid irq_num totally, and use intx, pirq, pic_irq?

Right, this might make it clearer.

> I hope the following clarifies the conversion.
> 
> A pci device asserts a interrupt pin of intx
>       |
>       V
> pci_set_irq()
>       |
>       V
> pci_slot_get_pirq() = bus->map_irq() gets (PCIDevice, intx)
>   returns pirq
>   This corresponds to how pci intx lines are connected to piix3 pirq pins
>       |
>       V
> piix3_set_irq() = bus->set_irq() gets pirq
>       |
>       V
> piix3_set_irq_levels() gets pirq
>   converts pirq# into pic irq
>   calls piix3_set_irq_pic() with pic_irq
>       |
>       V
> piix3_set_irq_pic() gets pic_irq
>       |
>       V
>    pic code
> 
> 
> thanks,

I think I get it.

> > > +{
> > > +    int pic_irq;
> > > +    uint64_t mask;
> > > +
> > > +    pic_irq = piix3->dev.config[PIIX_PIRQC + irq_num];
> > > +    if (pic_irq >= PIIX_NUM_PIC_IRQS) {
> > > +        return;
> > > +    }
> > > +
> > > +    mask = 1ULL << ((pic_irq * PIIX_NUM_PIRQS) + irq_num);
> > > +    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 irq_num, int level)
> > >  {
> > > -    int i, pic_irq, pic_level;
> > >      PIIX3State *piix3 = opaque;
> > > +    piix3_set_irq_level(piix3, irq_num, 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);
> > 
> > Here it is called with PIRQ # 0 to PIIX_NUM_PIRQS.
> > 
> > > +    }
> > > +}
> > > +
> > > +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);
> > >      }
> > >  }
> > >  
> > > @@ -312,6 +369,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)
> > > @@ -330,6 +396,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),
> > > @@ -372,6 +439,7 @@ static PCIDeviceInfo i440fx_info[] = {
> > >          .qdev.no_user = 1,
> > >          .no_hotplug   = 1,
> > >          .init         = piix3_initfn,
> > > +        .config_write = piix3_write_config,
> > >      },{
> > >          /* end of list */
> > >      }
> > > 
> > 
> 
> -- 
> yamahata