diff mbox

[PATCHv2,1/3] pci: prepare irq code for interrupt state

Message ID 20091126154833.GB2694@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Nov. 26, 2009, 3:48 p.m. UTC 
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(-)

Comments

Juan Quintela Nov. 26, 2009, 6:54 p.m. UTC | #1 
"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.
Michael S. Tsirkin Nov. 29, 2009, 11:02 a.m. UTC | #2 
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.
diff mbox

Patch

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;