[Qemu-devel] Re: [PATCH 04/26] pci: add accessor function to get irq levels

[Isaku Yamahata]

On Thu, Mar 17, 2011 at 07:29:09AM +0200, Michael S. Tsirkin wrote:
> On Wed, Mar 16, 2011 at 06:29:15PM +0900, Isaku Yamahata wrote:
> > 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.
> 
> At least for piix, the right thing to IMO is to have bit per
> IRQ, then the for loop can be replaced with a single !!.  There's a TODO
> there which this will fix.  I think we can reuse pci device irq_state
> for this: need to check. Haven't looked at q35 yet - applies there as
> well?

Yes, such bitmap optimization is possible.
But this accessor function is still necessary,
please see the following. (I didn't do any test yet. Just to show the idea)
If you like it, I'll post it as separate patch.

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 151353c..82b7daf 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -40,6 +40,7 @@ typedef PCIHostState I440FXState;
 
 typedef struct PIIX3State {
     PCIDevice dev;
+    unsigned long irq_level[16];
     int32_t dummy_for_save_load_compat[4];
     qemu_irq *pic;
 } PIIX3State;
@@ -200,25 +201,51 @@ PCIBus *i440fx_init(int *piix3_devfn, qemu_irq *pic, 
ram_addr_t ram_size)
 }
 
 /* PIIX3 PCI to ISA bridge */
-
 static void piix3_set_irq(void *opaque, int irq_num, int level)
 {
     int i, pic_irq, pic_level;
     PIIX3State *piix3 = opaque;
 
-    /* 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);
-            }
+    if (pic_irq >= 16) {
+        return;
+    }
+
+    /* The pic level is the logical OR of all the PCI irqs mapped to it */
+    if (level) {
+        set_bit(&piix3->irq_level[pic_irq], irq_num);
+    } else {
+        clear_bit(&piix3->irq_level[pic_irq], irq_num);
+    }
+    qemu_set_irq(piix3->pic[pic_irq], !!piix3->irq_level[pic_irq]);
+}
+
+/* irq routing is changed. so rebuild bitmap */
+static void piix3_update_irq_levels(PIIX3State *piix3)
+{
+    int i;
+    for (i = 0; i < 16; i++) {
+        piix3->irq_level[i] = 0;
+    }
+    for (i = 0; i < 4; i++) {
+        int pic_irq = piix3->dev.config[0x60 + irq_num];
+        if (pic_irq >= 16) {
+            continue;
+        }
+        if (pci_bus_get_irq_level(piix3->dev.bus, i)) {
+            set_bit(&piix3->irq_level[pic_irq], i);
         }
-        qemu_set_irq(piix3->pic[pic_irq], pic_level);
+    }
+}
+
+static void piix3_write_config(PCIDevice *dev,
+                               uint32_t address, uint32_t val, int len)
+{
+    PIIX3State *piix3 = DO_UPCAST(PIIX3State, dev, dev);
+
+    pci_default_write_config(dev, address, val, len);
+    if (ranges_overlap(address, len, 0x60, 4)) {
+        piix3_update_irq_levels(piix3);
     }
 }
 
@@ -318,6 +345,7 @@ static PCIDeviceInfo i440fx_info[] = {
         .qdev.no_user = 1,
         .no_hotplug   = 1,
         .init         = piix3_initfn,
+        .config_write = piix3_write_config,
     },{
         /* end of list */
     }

-- 
yamahata