[Qemu-devel] [PATCHv2 1/3] pci: prepare irq code for interrupt state

[Qemu-devel] [PATCHv2 1/3] pci: prepare irq code for interrupt state

[Michael S. Tsirkin]

This rearranges code in preparation for interrupt state
implementation.
Changes:
	- split up but walk away from interrupt handling
          into a subroutine
	- change irq_state from an array to bitmask
	- verify that irq_state values are 0 or 1 on load

There are no functional changes.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci.c |   89 ++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 hw/pci.h |    2 +-
 2 files changed, 74 insertions(+), 17 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 0359f30..1eb51f8 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -103,11 +103,36 @@ static int pci_bar(PCIDevice *d, int reg)
     return type == PCI_HEADER_TYPE_BRIDGE ? PCI_ROM_ADDRESS1 : PCI_ROM_ADDRESS;
 }
 
+static inline int pci_irq_state(PCIDevice *d, int irq_num)
+{
+	return (d->irq_state >> irq_num) & 0x1;
+}
+
+static inline void pci_set_irq_state(PCIDevice *d, int irq_num, int level)
+{
+	d->irq_state &= ~(0x1 << irq_num);
+	d->irq_state |= level << irq_num;
+}
+
+static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change)
+{
+    PCIBus *bus;
+    for (;;) {
+        bus = pci_dev->bus;
+        irq_num = bus->map_irq(pci_dev, irq_num);
+        if (bus->set_irq)
+            break;
+        pci_dev = bus->parent_dev;
+    }
+    bus->irq_count[irq_num] += change;
+    bus->set_irq(bus->irq_opaque, irq_num, bus->irq_count[irq_num] != 0);
+}
+
 static void pci_device_reset(PCIDevice *dev)
 {
     int r;
 
-    memset(dev->irq_state, 0, sizeof dev->irq_state);
+    dev->irq_state = 0;
     dev->config[PCI_COMMAND] &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
                                   PCI_COMMAND_MASTER);
     dev->config[PCI_CACHE_LINE_SIZE] = 0x0;
@@ -276,6 +301,43 @@ static VMStateInfo vmstate_info_pci_config = {
     .put  = put_pci_config_device,
 };
 
+static int get_pci_irq_state(QEMUFile *f, void *pv, size_t size)
+{
+    PCIDevice *s = container_of(pv, PCIDevice, config);
+    uint32_t irq_state[PCI_NUM_PINS];
+    int i;
+    for (i = 0; i < PCI_NUM_PINS; ++i) {
+        irq_state[i] = qemu_get_be32(f);
+        if (irq_state[i] != 0x1 && irq_state[i] != 0) {
+            fprintf(stderr, "irq state %d: must be 0 or 1.\n",
+                    irq_state[i]);
+            return -EINVAL;
+        }
+    }
+
+    for (i = 0; i < PCI_NUM_PINS; ++i) {
+        pci_set_irq_state(s, i, irq_state[i]);
+    }
+
+    return 0;
+}
+
+static void put_pci_irq_state(QEMUFile *f, void *pv, size_t size)
+{
+    int i;
+    PCIDevice *s = container_of(pv, PCIDevice, config);
+
+    for (i = 0; i < PCI_NUM_PINS; ++i) {
+        qemu_put_be32(f, pci_irq_state(s, i));
+    }
+}
+
+static VMStateInfo vmstate_info_pci_irq_state = {
+    .name = "pci irq state",
+    .get  = get_pci_irq_state,
+    .put  = put_pci_irq_state,
+};
+
 const VMStateDescription vmstate_pci_device = {
     .name = "PCIDevice",
     .version_id = 2,
@@ -286,7 +348,9 @@ const VMStateDescription vmstate_pci_device = {
         VMSTATE_BUFFER_UNSAFE_INFO(config, PCIDevice, 0,
                                    vmstate_info_pci_config,
                                    PCI_CONFIG_SPACE_SIZE),
-        VMSTATE_INT32_ARRAY_V(irq_state, PCIDevice, PCI_NUM_PINS, 2),
+        VMSTATE_BUFFER_UNSAFE_INFO(irq_state, PCIDevice, 2,
+				   vmstate_info_pci_irq_state,
+				   PCI_NUM_PINS * sizeof(int32_t)),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -301,7 +365,9 @@ const VMStateDescription vmstate_pcie_device = {
         VMSTATE_BUFFER_UNSAFE_INFO(config, PCIDevice, 0,
                                    vmstate_info_pci_config,
                                    PCIE_CONFIG_SPACE_SIZE),
-        VMSTATE_INT32_ARRAY_V(irq_state, PCIDevice, PCI_NUM_PINS, 2),
+        VMSTATE_BUFFER_UNSAFE_INFO(irq_state, PCIDevice, 2,
+				   vmstate_info_pci_irq_state,
+				   PCI_NUM_PINS * sizeof(int32_t)),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -501,7 +567,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
     pci_dev->bus = bus;
     pci_dev->devfn = devfn;
     pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
-    memset(pci_dev->irq_state, 0, sizeof(pci_dev->irq_state));
+    pci_dev->irq_state = 0;
     pci_config_alloc(pci_dev);
 
     header_type &= ~PCI_HEADER_TYPE_MULTI_FUNCTION;
@@ -884,23 +950,14 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
 static void pci_set_irq(void *opaque, int irq_num, int level)
 {
     PCIDevice *pci_dev = opaque;
-    PCIBus *bus;
     int change;
 
-    change = level - pci_dev->irq_state[irq_num];
+    change = level - pci_irq_state(pci_dev, irq_num);
     if (!change)
         return;
 
-    pci_dev->irq_state[irq_num] = level;
-    for (;;) {
-        bus = pci_dev->bus;
-        irq_num = bus->map_irq(pci_dev, irq_num);
-        if (bus->set_irq)
-            break;
-        pci_dev = bus->parent_dev;
-    }
-    bus->irq_count[irq_num] += change;
-    bus->set_irq(bus->irq_opaque, irq_num, bus->irq_count[irq_num] != 0);
+    pci_set_irq_state(pci_dev, irq_num, level);
+    pci_change_irq_level(pci_dev, irq_num, change);
 }
 
 /***********************************************************/
diff --git a/hw/pci.h b/hw/pci.h
index 3e8abad..ebf6c39 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -220,7 +220,7 @@ struct PCIDevice {
     qemu_irq *irq;
 
     /* Current IRQ levels.  Used internally by the generic PCI code.  */
-    int irq_state[PCI_NUM_PINS];
+    uint8_t irq_state;
 
     /* Capability bits */
     uint32_t cap_present;
-- 
1.6.5.2.143.g8cc62

[Qemu-devel] [PATCHv2 2/3] pci: interrupt status bit implementation

[Michael S. Tsirkin]

interrupt status is a mandatory feature in PCI spec,
so devices must implement it to be spec compliant.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci.c |   26 +++++++++++++++++++++++++-
 hw/pci.h |    1 +
 2 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 1eb51f8..f83ea93 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -128,11 +128,23 @@ 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 interrupt status bit in config space on interrupt
+ * state change. */
+static void pci_update_irq_status(PCIDevice *dev)
+{
+    if (dev->irq_state) {
+        dev->config[PCI_STATUS] |= PCI_STATUS_INTERRUPT;
+    } else {
+        dev->config[PCI_STATUS] &= ~PCI_STATUS_INTERRUPT;
+    }
+}
+
 static void pci_device_reset(PCIDevice *dev)
 {
     int r;
 
     dev->irq_state = 0;
+    pci_update_irq_status(dev);
     dev->config[PCI_COMMAND] &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
                                   PCI_COMMAND_MASTER);
     dev->config[PCI_CACHE_LINE_SIZE] = 0x0;
@@ -379,12 +391,23 @@ static inline const VMStateDescription *pci_get_vmstate(PCIDevice *s)
 
 void pci_device_save(PCIDevice *s, QEMUFile *f)
 {
+    /* Clear interrupt status bit: it is implicit
+     * in irq_state which we are saving.
+     * This makes us compatible with old devices
+     * which never set or clear this bit. */
+    s->config[PCI_STATUS] &= ~PCI_STATUS_INTERRUPT;
     vmstate_save_state(f, pci_get_vmstate(s), s);
+    /* Restore the interrupt status bit. */
+    pci_update_irq_status(s);
 }
 
 int pci_device_load(PCIDevice *s, QEMUFile *f)
 {
-    return vmstate_load_state(f, pci_get_vmstate(s), s, s->version_id);
+    int ret;
+    ret = vmstate_load_state(f, pci_get_vmstate(s), s, s->version_id);
+    /* Restore the interrupt status bit. */
+    pci_update_irq_status(s);
+    return ret;
 }
 
 static int pci_set_default_subsystem_id(PCIDevice *pci_dev)
@@ -957,6 +980,7 @@ static void pci_set_irq(void *opaque, int irq_num, int level)
         return;
 
     pci_set_irq_state(pci_dev, irq_num, level);
+    pci_update_irq_status(pci_dev);
     pci_change_irq_level(pci_dev, irq_num, change);
 }
 
diff --git a/hw/pci.h b/hw/pci.h
index ebf6c39..dc9b860 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -102,6 +102,7 @@ typedef struct PCIIORegion {
 #define  PCI_COMMAND_MEMORY	0x2	/* Enable response in Memory space */
 #define  PCI_COMMAND_MASTER	0x4	/* Enable bus master */
 #define PCI_STATUS              0x06    /* 16 bits */
+#define  PCI_STATUS_INTERRUPT   0x08
 #define PCI_REVISION_ID         0x08    /* 8 bits  */
 #define PCI_CLASS_PROG		0x09	/* Reg. Level Programming Interface */
 #define PCI_CLASS_DEVICE        0x0a    /* Device class */
-- 
1.6.5.2.143.g8cc62

[Qemu-devel] [PATCHv2 3/3] pci: interrupt disable bit support

[Michael S. Tsirkin]

Interrupt disable bit is mandatory in PCI spec.
Implement it to make devices spec compliant.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci.c |   29 +++++++++++++++++++++++++++--
 hw/pci.h |    1 +
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index f83ea93..6542c20 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -520,7 +520,8 @@ static void pci_init_wmask(PCIDevice *dev)
     dev->wmask[PCI_CACHE_LINE_SIZE] = 0xff;
     dev->wmask[PCI_INTERRUPT_LINE] = 0xff;
     pci_set_word(dev->wmask + PCI_COMMAND,
-                 PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
+                 PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER |
+                 PCI_COMMAND_INTX_DISABLE);
 
     memset(dev->wmask + PCI_CONFIG_HEADER_SIZE, 0xff,
            config_size - PCI_CONFIG_HEADER_SIZE);
@@ -940,6 +941,25 @@ static void pci_update_mappings(PCIDevice *d)
     }
 }
 
+static inline int pci_irq_disabled(PCIDevice *d)
+{
+    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;
+    for (i = 0; i < PCI_NUM_PINS; ++i) {
+        int state = pci_irq_state(d, i);
+        pci_change_irq_level(d, i, disabled ? -state : state);
+    }
+}
+
 uint32_t pci_default_read_config(PCIDevice *d,
                                  uint32_t address, int len)
 {
@@ -952,7 +972,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) {
@@ -964,6 +984,9 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
         ranges_overlap(addr, l, PCI_ROM_ADDRESS1, 4) ||
         range_covers_byte(addr, l, PCI_COMMAND))
         pci_update_mappings(d);
+
+    if (range_covers_byte(addr, l, PCI_COMMAND))
+        pci_update_irq_disabled(d, was_irq_disabled);
 }
 
 /***********************************************************/
@@ -981,6 +1004,8 @@ static void pci_set_irq(void *opaque, int irq_num, int level)
 
     pci_set_irq_state(pci_dev, irq_num, level);
     pci_update_irq_status(pci_dev);
+    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 dc9b860..d279e3f 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -101,6 +101,7 @@ typedef struct PCIIORegion {
 #define  PCI_COMMAND_IO		0x1	/* Enable response in I/O space */
 #define  PCI_COMMAND_MEMORY	0x2	/* Enable response in Memory space */
 #define  PCI_COMMAND_MASTER	0x4	/* Enable bus master */
+#define  PCI_COMMAND_INTX_DISABLE 0x400 /* INTx Emulation Disable */
 #define PCI_STATUS              0x06    /* 16 bits */
 #define  PCI_STATUS_INTERRUPT   0x08
 #define PCI_REVISION_ID         0x08    /* 8 bits  */
-- 
1.6.5.2.143.g8cc62

[Qemu-devel] Re: [PATCHv2 1/3] pci: prepare irq code for interrupt state

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> This rearranges code in preparation for interrupt state
> implementation.
> Changes:
> 	- split up but walk away from interrupt handling
>           into a subroutine
> 	- change irq_state from an array to bitmask
> 	- verify that irq_state values are 0 or 1 on load
>
> There are no functional changes.

I guess that there is a good reason for changing for an array of ints to
one bitmap.

Change looks ok, and if it worked for you, it should work for everybody
(if you breaks pci_device migration stops working).

I would delay changing things to bitmaps until we decide how to
implement bitmaps in qemu (there are more than one implementation), at
least:
   - use array of ints, save as array of ints (i.e. works between hosts with
     different endianess)
   - use arraf of ints but save as array of bytes (break when you change
     endianess)
   - Now, you have a bitmap that fits in an uint8_t.

An probably some other way that I have missed.

Later, Juan.

[Qemu-devel] Re: [PATCHv2 1/3] pci: prepare irq code for interrupt state

[Michael S. Tsirkin]

On Thu, Nov 26, 2009 at 07:54:43PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > This rearranges code in preparation for interrupt state
> > implementation.
> > Changes:
> > 	- split up but walk away from interrupt handling
> >           into a subroutine
> > 	- change irq_state from an array to bitmask
> > 	- verify that irq_state values are 0 or 1 on load
> >
> > There are no functional changes.
> 
> I guess that there is a good reason for changing for an array of ints to
> one bitmap.
> 
> Change looks ok, and if it worked for you, it should work for everybody
> (if you breaks pci_device migration stops working).

Works fine for me.

> I would delay changing things to bitmaps until we decide how to
> implement bitmaps in qemu (there are more than one implementation), at
> least:
>    - use array of ints, save as array of ints (i.e. works between hosts with
>      different endianess)
>    - use arraf of ints but save as array of bytes (break when you change
>      endianess)
>    - Now, you have a bitmap that fits in an uint8_t.
> 
> An probably some other way that I have missed.
> 
> Later, Juan.

As we have to preserve compatibility with old qemu, there's no choice
but to keep saving pci in the same way as we have done in the past: ints
in big endian format.  And forcing implementation to match the save
format is just silly.

So I think the best way forward is to apply this patch, then you as
vmstate expert can go and unify things.  My idea is that vmstate should
"conversion routines" which would e.g. convert bitmap to array of ints,
vmstate then saves ints.

-- 
MST