[Qemu-devel] Re: [PATCH 0/4] pci: interrupt status/interrupt disable support

[Michael S. Tsirkin]

On Thu, Nov 26, 2009 at 12:21:46PM +0900, Isaku Yamahata wrote:
> At least I think irq_disable can be removed

The following patch on top of mine removes irq_disabled field in
PCIDevice.  I am of two minds whether this makes the code better.
What is your opinion?

diff --git a/hw/pci.c b/hw/pci.c
index 3daae46..75f97df 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -128,15 +128,18 @@ 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);
 }
 
-/* Update irq disabled field after config space change,
- * assert/deassert interrupts if necessary. */
-static void pci_update_irq_disabled(PCIDevice *d)
+static inline int pci_irq_disabled(PCIDevice *d)
 {
-    int i;
-    int disabled = !!(pci_get_word(d->config + PCI_COMMAND) &
-                      PCI_COMMAND_INTX_DISABLE);
-    if (disabled == d->irq_disabled)
+    return !!(pci_get_word(d->config + PCI_COMMAND) & 
PCI_COMMAND_INTX_DISABLE);
+}
+
+/* Called after interrupt disabled field update in config space,
+ * assert/deassert interrupts if necessary.
+ * Gets original interrupt disable bit value (before update). */
+static void pci_update_irq_disabled(PCIDevice *d, int was_irq_disabled)
+{
+    int i, disabled = pci_irq_disabled(d);
+    if (disabled == was_irq_disabled)
         return;
-    d->irq_disabled = disabled;
     for (i = 0; i < ARRAY_SIZE(d->irq_state); ++i) {
         int state = d->irq_state[i];
@@ -150,7 +153,6 @@ static void pci_device_reset(PCIDevice *dev)
 
     memset(dev->irq_state, 0, sizeof dev->irq_state);
     dev->irq_status = 0;
-    dev->irq_disabled = 0;
     pci_update_irq_status(dev);
     dev->config[PCI_COMMAND] &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
                                   PCI_COMMAND_MASTER);
@@ -369,14 +371,14 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
 
 int pci_device_load(PCIDevice *s, QEMUFile *f)
 {
-    int ret, i;
+    int ret, i, was_irq_disabled = pci_irq_disabled(d);
     ret = vmstate_load_state(f, pci_get_vmstate(s), s, s->version_id);
     for (i = 0; i < ARRAY_SIZE(s->irq_state); ++i) {
         s->irq_status += s->irq_state[i];
     }
     /* Restore the interrupt status bit. */
     pci_update_irq_status(s);
-    pci_update_irq_disabled(s);
+    pci_update_irq_disabled(s, was_irq_disabled);
     return ret;
 }
 
@@ -924,7 +926,7 @@ uint32_t pci_default_read_config(PCIDevice *d,
 
 void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
 {
-    int i;
+    int i, was_irq_disabled = pci_irq_disabled(d);
     uint32_t config_size = pci_config_size(d);
 
     for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) {
@@ -938,7 +940,7 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, 
uint32_t val, int l)
         pci_update_mappings(d);
 
     if (range_covers_byte(addr, l, PCI_COMMAND))
-        pci_update_irq_disabled(d);
+        pci_update_irq_disabled(d, was_irq_disabled);
 }
 
 /***********************************************************/
@@ -957,7 +959,7 @@ static void pci_set_irq(void *opaque, int irq_num, int 
level)
     pci_dev->irq_state[irq_num] = level;
     pci_dev->irq_status += change;
     pci_update_irq_status(pci_dev);
-    if (pci_dev->irq_disabled)
+    if (pci_irq_disabled(pci_dev))
         return;
     pci_change_irq_level(pci_dev, irq_num, change);
 }
diff --git a/hw/pci.h b/hw/pci.h
index 994b8bc..25ad114 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -227,9 +227,6 @@ struct PCIDevice {
     /* Sum of all irq levels. Used to implement irq status register. */
     int irq_status;
 
-    /* Whether interrupts are disabled by command bit. */
-    int irq_disabled;
-
     /* Capability bits */
     uint32_t cap_present;