[Qemu-devel] [PATCH 0/6] msi: Small refactoring

[Qemu-devel] [PATCH 0/6] msi: Small refactoring

[Jan Kiszka]

Collection of patches to improve MSI[X] usability in device models,
clean up some minor bits, and help kvm irqchip introduction.

CC: Alexander Graf <agraf@suse.de>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Isaku Yamahata <yamahata@valinux.co.jp>

Jan Kiszka (6):
  msi: Guard msi/msix_write_config with msi_present
  msi: Guard msi_reset with msi_present
  msi: Use msi/msix_present more consistently
  msi: Invoke msi/msix_reset from PCI core
  msi: Invoke msi/msix_write_config from PCI core
  msi: Generalize msix_supported to msi_supported

 hw/ide/ich.c            |    8 --------
 hw/intel-hda.c          |   12 ------------
 hw/ioh3420.c            |    3 +--
 hw/msi.c                |   19 ++++++++++++++++---
 hw/msi.h                |    2 ++
 hw/msix.c               |   24 +++++++++++++-----------
 hw/msix.h               |    2 --
 hw/pc.c                 |    4 ++--
 hw/pci.c                |    8 ++++++++
 hw/pci_bridge.c         |    4 ++++
 hw/virtio-pci.c         |    3 ---
 hw/xio3130_downstream.c |    3 +--
 hw/xio3130_upstream.c   |    2 --
 13 files changed, 47 insertions(+), 47 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH 1/6] msi: Guard msi/msix_write_config with msi_present

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Terminate msi/msix_write_config early if support is not enabled. This
allows to remove checks at the caller site if MSI is optional.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/msi.c  |    3 ++-
 hw/msix.c |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/msi.c b/hw/msi.c
index f214fcf..541e4e1 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -264,7 +264,8 @@ void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len)
     unsigned int vector;
     uint32_t pending;
 
-    if (!ranges_overlap(addr, len, dev->msi_cap, msi_cap_sizeof(flags))) {
+    if (!msi_present(dev) ||
+        !ranges_overlap(addr, len, dev->msi_cap, msi_cap_sizeof(flags))) {
         return;
     }
 
diff --git a/hw/msix.c b/hw/msix.c
index 149eed2..32fd9b2 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -156,7 +156,7 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
     int vector;
     bool was_masked;
 
-    if (!range_covers_byte(addr, len, enable_pos)) {
+    if (!msix_present(dev) || !range_covers_byte(addr, len, enable_pos)) {
         return;
     }
 
-- 
1.7.3.4

[Qemu-devel] [PATCH 2/6] msi: Guard msi_reset with msi_present

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/msi.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/msi.c b/hw/msi.c
index 541e4e1..612b168 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -183,6 +183,10 @@ void msi_reset(PCIDevice *dev)
     uint16_t flags;
     bool msi64bit;
 
+    if (!msi_present(dev)) {
+        return;
+    }
+
     flags = pci_get_word(dev->config + msi_flags_off(dev));
     flags &= ~(PCI_MSI_FLAGS_QSIZE | PCI_MSI_FLAGS_ENABLE);
     msi64bit = flags & PCI_MSI_FLAGS_64BIT;
-- 
1.7.3.4

[Qemu-devel] [PATCH 3/6] msi: Use msi/msix_present more consistently

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Replace some open-coded msi/msix_present checks and drop redundant
msix_supported tests (present implies supported).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/msi.c  |    2 +-
 hw/msix.c |   13 ++++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/msi.c b/hw/msi.c
index 612b168..137dba0 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -167,7 +167,7 @@ void msi_uninit(struct PCIDevice *dev)
     uint16_t flags;
     uint8_t cap_size;
 
-    if (!(dev->cap_present & QEMU_PCI_CAP_MSI)) {
+    if (!msi_present(dev)) {
         return;
     }
     flags = pci_get_word(dev->config + msi_flags_off(dev));
diff --git a/hw/msix.c b/hw/msix.c
index 32fd9b2..876793a 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -283,8 +283,9 @@ static void msix_free_irq_entries(PCIDevice *dev)
 /* Clean up resources for the device. */
 int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
 {
-    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))
+    if (!msix_present(dev)) {
         return 0;
+    }
     pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
     dev->msix_cap = 0;
     msix_free_irq_entries(dev);
@@ -303,7 +304,7 @@ void msix_save(PCIDevice *dev, QEMUFile *f)
 {
     unsigned n = dev->msix_entries_nr;
 
-    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
+    if (!msix_present(dev)) {
         return;
     }
 
@@ -316,7 +317,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
 {
     unsigned n = dev->msix_entries_nr;
 
-    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
+    if (!msix_present(dev)) {
         return;
     }
 
@@ -368,8 +369,9 @@ void msix_notify(PCIDevice *dev, unsigned vector)
 
 void msix_reset(PCIDevice *dev)
 {
-    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))
+    if (!msix_present(dev)) {
         return;
+    }
     msix_free_irq_entries(dev);
     dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
 	    ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
@@ -408,7 +410,8 @@ void msix_vector_unuse(PCIDevice *dev, unsigned vector)
 
 void msix_unuse_all_vectors(PCIDevice *dev)
 {
-    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX))
+    if (!msix_present(dev)) {
         return;
+    }
     msix_free_irq_entries(dev);
 }
-- 
1.7.3.4

[Qemu-devel] [PATCH 4/6] msi: Invoke msi/msix_reset from PCI core

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

There is no point in pushing this burden to the devices, they may rather
forget to call them (like intel-hda and ahci ATM). Instead, reset
functions are now called from pci_device_reset and pci_bridge_reset.
They do nothing if the MSI/MSI-X is not in use.

CC: Alexander Graf <agraf@suse.de>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/ioh3420.c            |    2 +-
 hw/pci.c                |    5 +++++
 hw/pci_bridge.c         |    4 ++++
 hw/virtio-pci.c         |    1 -
 hw/xio3130_downstream.c |    2 +-
 hw/xio3130_upstream.c   |    1 -
 6 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/ioh3420.c b/hw/ioh3420.c
index a6bfbb9..fc2fb3b 100644
--- a/hw/ioh3420.c
+++ b/hw/ioh3420.c
@@ -81,7 +81,7 @@ static void ioh3420_write_config(PCIDevice *d,
 static void ioh3420_reset(DeviceState *qdev)
 {
     PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev);
-    msi_reset(d);
+
     ioh3420_aer_vector_update(d);
     pcie_cap_root_reset(d);
     pcie_cap_deverr_reset(d);
diff --git a/hw/pci.c b/hw/pci.c
index 399227f..5d5829d 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -31,6 +31,8 @@
 #include "loader.h"
 #include "range.h"
 #include "qmp-commands.h"
+#include "msi.h"
+#include "msix.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI
@@ -191,6 +193,9 @@ void pci_device_reset(PCIDevice *dev)
         }
     }
     pci_update_mappings(dev);
+
+    msi_reset(dev);
+    msix_reset(dev);
 }
 
 /*
diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
index 650d165..6799978 100644
--- a/hw/pci_bridge.c
+++ b/hw/pci_bridge.c
@@ -32,6 +32,8 @@
 #include "pci_bridge.h"
 #include "pci_internals.h"
 #include "range.h"
+#include "msi.h"
+#include "msix.h"
 
 /* PCI bridge subsystem vendor ID helper functions */
 #define PCI_SSVID_SIZEOF        8
@@ -296,6 +298,8 @@ void pci_bridge_reset(DeviceState *qdev)
 {
     PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, qdev);
     pci_bridge_reset_reg(dev);
+    msi_reset(dev);
+    msix_reset(dev);
 }
 
 /* default qdev initialization function for PCI-to-PCI bridge */
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 64c6a94..16a5b08 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -271,7 +271,6 @@ static void virtio_pci_reset(DeviceState *d)
     VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     virtio_pci_stop_ioeventfd(proxy);
     virtio_reset(proxy->vdev);
-    msix_reset(&proxy->pci_dev);
     proxy->flags &= ~VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
 }
 
diff --git a/hw/xio3130_downstream.c b/hw/xio3130_downstream.c
index d3c387d..464eefa 100644
--- a/hw/xio3130_downstream.c
+++ b/hw/xio3130_downstream.c
@@ -48,7 +48,7 @@ static void xio3130_downstream_write_config(PCIDevice *d, uint32_t address,
 static void xio3130_downstream_reset(DeviceState *qdev)
 {
     PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev);
-    msi_reset(d);
+
     pcie_cap_deverr_reset(d);
     pcie_cap_slot_reset(d);
     pcie_cap_ari_reset(d);
diff --git a/hw/xio3130_upstream.c b/hw/xio3130_upstream.c
index 8283695..0d8d254 100644
--- a/hw/xio3130_upstream.c
+++ b/hw/xio3130_upstream.c
@@ -47,7 +47,6 @@ static void xio3130_upstream_write_config(PCIDevice *d, uint32_t address,
 static void xio3130_upstream_reset(DeviceState *qdev)
 {
     PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev);
-    msi_reset(d);
     pci_bridge_reset(qdev);
     pcie_cap_deverr_reset(d);
 }
-- 
1.7.3.4

[Qemu-devel] [PATCH 5/6] msi: Invoke msi/msix_write_config from PCI core

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Also this functions is better invoked by the core than by each and every
device. This allows to drop the config_write callbacks from ich and
intel-hda.

CC: Alexander Graf <agraf@suse.de>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/ide/ich.c            |    8 --------
 hw/intel-hda.c          |   12 ------------
 hw/ioh3420.c            |    1 -
 hw/msi.c                |    2 +-
 hw/pci.c                |    3 +++
 hw/virtio-pci.c         |    2 --
 hw/xio3130_downstream.c |    1 -
 hw/xio3130_upstream.c   |    1 -
 8 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 3f7510f..a470c01 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -139,13 +139,6 @@ static int pci_ich9_uninit(PCIDevice *dev)
     return 0;
 }
 
-static void pci_ich9_write_config(PCIDevice *pci, uint32_t addr,
-                                  uint32_t val, int len)
-{
-    pci_default_write_config(pci, addr, val, len);
-    msi_write_config(pci, addr, val, len);
-}
-
 static PCIDeviceInfo ich_ahci_info[] = {
     {
         .qdev.name    = "ich9-ahci",
@@ -154,7 +147,6 @@ static PCIDeviceInfo ich_ahci_info[] = {
         .qdev.vmsd    = &vmstate_ahci,
         .init         = pci_ich9_ahci_init,
         .exit         = pci_ich9_uninit,
-        .config_write = pci_ich9_write_config,
         .vendor_id    = PCI_VENDOR_ID_INTEL,
         .device_id    = PCI_DEVICE_ID_INTEL_82801IR,
         .revision     = 0x02,
diff --git a/hw/intel-hda.c b/hw/intel-hda.c
index 10769e0..995d895 100644
--- a/hw/intel-hda.c
+++ b/hw/intel-hda.c
@@ -1158,17 +1158,6 @@ static int intel_hda_exit(PCIDevice *pci)
     return 0;
 }
 
-static void intel_hda_write_config(PCIDevice *pci, uint32_t addr,
-                                   uint32_t val, int len)
-{
-    IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
-
-    pci_default_write_config(pci, addr, val, len);
-    if (d->msi) {
-        msi_write_config(pci, addr, val, len);
-    }
-}
-
 static int intel_hda_post_load(void *opaque, int version)
 {
     IntelHDAState* d = opaque;
@@ -1252,7 +1241,6 @@ static PCIDeviceInfo intel_hda_info = {
     .qdev.reset   = intel_hda_reset,
     .init         = intel_hda_init,
     .exit         = intel_hda_exit,
-    .config_write = intel_hda_write_config,
     .vendor_id    = PCI_VENDOR_ID_INTEL,
     .device_id    = 0x2668,
     .revision     = 1,
diff --git a/hw/ioh3420.c b/hw/ioh3420.c
index fc2fb3b..886ede8 100644
--- a/hw/ioh3420.c
+++ b/hw/ioh3420.c
@@ -71,7 +71,6 @@ static void ioh3420_write_config(PCIDevice *d,
         pci_get_long(d->config + d->exp.aer_cap + PCI_ERR_ROOT_COMMAND);
 
     pci_bridge_write_config(d, address, val, len);
-    msi_write_config(d, address, val, len);
     ioh3420_aer_vector_update(d);
     pcie_cap_slot_write_config(d, address, val, len);
     pcie_aer_write_config(d, address, val, len);
diff --git a/hw/msi.c b/hw/msi.c
index 137dba0..c4e8a6e 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -256,7 +256,7 @@ void msi_notify(PCIDevice *dev, unsigned int vector)
     stl_le_phys(address, data);
 }
 
-/* call this function after updating configs by pci_default_write_config(). */
+/* Normally called by pci_default_write_config(). */
 void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len)
 {
     uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
diff --git a/hw/pci.c b/hw/pci.c
index 5d5829d..8c814cd 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1056,6 +1056,9 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
 
     if (range_covers_byte(addr, l, PCI_COMMAND))
         pci_update_irq_disabled(d, was_irq_disabled);
+
+    msi_write_config(d, addr, val, l);
+    msix_write_config(d, addr, val, l);
 }
 
 /***********************************************************/
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 16a5b08..d21a7ee 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -492,8 +492,6 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
         virtio_set_status(proxy->vdev,
                           proxy->vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
     }
-
-    msix_write_config(pci_dev, address, val, len);
 }
 
 static unsigned virtio_pci_get_features(void *opaque)
diff --git a/hw/xio3130_downstream.c b/hw/xio3130_downstream.c
index 464eefa..8e9117d 100644
--- a/hw/xio3130_downstream.c
+++ b/hw/xio3130_downstream.c
@@ -41,7 +41,6 @@ static void xio3130_downstream_write_config(PCIDevice *d, uint32_t address,
     pci_bridge_write_config(d, address, val, len);
     pcie_cap_flr_write_config(d, address, val, len);
     pcie_cap_slot_write_config(d, address, val, len);
-    msi_write_config(d, address, val, len);
     pcie_aer_write_config(d, address, val, len);
 }
 
diff --git a/hw/xio3130_upstream.c b/hw/xio3130_upstream.c
index 0d8d254..707401e 100644
--- a/hw/xio3130_upstream.c
+++ b/hw/xio3130_upstream.c
@@ -40,7 +40,6 @@ static void xio3130_upstream_write_config(PCIDevice *d, uint32_t address,
 {
     pci_bridge_write_config(d, address, val, len);
     pcie_cap_flr_write_config(d, address, val, len);
-    msi_write_config(d, address, val, len);
     pcie_aer_write_config(d, address, val, len);
 }
 
-- 
1.7.3.4

[Qemu-devel] [PATCH 6/6] msi: Generalize msix_supported to msi_supported

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Rename msix_supported to msi_supported and control MSI and MSI-X
activation this way. That was likely to original intention for this
flag, but MSI support came after MSI-X.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/msi.c  |    8 ++++++++
 hw/msi.h  |    2 ++
 hw/msix.c |    9 ++++-----
 hw/msix.h |    2 --
 hw/pc.c   |    4 ++--
 5 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/msi.c b/hw/msi.c
index c4e8a6e..5233204 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -36,6 +36,9 @@
 
 #define PCI_MSI_VECTORS_MAX     32
 
+/* Flag for interrupt controller to declare MSI/MSI-X support */
+bool msi_supported;
+
 /* If we get rid of cap allocator, we won't need this. */
 static inline uint8_t msi_cap_sizeof(uint16_t flags)
 {
@@ -116,6 +119,11 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
     uint16_t flags;
     uint8_t cap_size;
     int config_offset;
+
+    if (!msi_supported) {
+        return -ENOTSUP;
+    }
+
     MSI_DEV_PRINTF(dev,
                    "init offset: 0x%"PRIx8" vector: %"PRId8
                    " 64bit %d mask %d\n",
diff --git a/hw/msi.h b/hw/msi.h
index 5766018..3040bb0 100644
--- a/hw/msi.h
+++ b/hw/msi.h
@@ -24,6 +24,8 @@
 #include "qemu-common.h"
 #include "pci.h"
 
+extern bool msi_supported;
+
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
              unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);
diff --git a/hw/msix.c b/hw/msix.c
index 876793a..4897c58 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -12,6 +12,7 @@
  */
 
 #include "hw.h"
+#include "msi.h"
 #include "msix.h"
 #include "pci.h"
 #include "range.h"
@@ -32,9 +33,6 @@
 #define MSIX_MAX_ENTRIES 32
 
 
-/* Flag for interrupt controller to declare MSI-X support */
-int msix_supported;
-
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
  * and fill MSI-X capability in the config space.
@@ -235,10 +233,11 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
               unsigned bar_nr, unsigned bar_size)
 {
     int ret;
+
     /* Nothing to do if MSI is not supported by interrupt controller */
-    if (!msix_supported)
+    if (!msi_supported) {
         return -ENOTSUP;
-
+    }
     if (nentries > MSIX_MAX_ENTRIES)
         return -EINVAL;
 
diff --git a/hw/msix.h b/hw/msix.h
index 7e04336..5aba22b 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -29,6 +29,4 @@ void msix_notify(PCIDevice *dev, unsigned vector);
 
 void msix_reset(PCIDevice *dev);
 
-extern int msix_supported;
-
 #endif
diff --git a/hw/pc.c b/hw/pc.c
index 33778fe..7e40031 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -36,7 +36,7 @@
 #include "elf.h"
 #include "multiboot.h"
 #include "mc146818rtc.h"
-#include "msix.h"
+#include "msi.h"
 #include "sysbus.h"
 #include "sysemu.h"
 #include "blockdev.h"
@@ -896,7 +896,7 @@ static DeviceState *apic_init(void *env, uint8_t apic_id)
         apic_mapped = 1;
     }
 
-    msix_supported = 1;
+    msi_supported = true;
 
     return dev;
 }
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 4/6] msi: Invoke msi/msix_reset from PCI core

[Michael S. Tsirkin]

On Sun, Dec 04, 2011 at 02:22:12PM +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> There is no point in pushing this burden to the devices, they may rather
> forget to call them (like intel-hda and ahci ATM). Instead, reset
> functions are now called from pci_device_reset and pci_bridge_reset.
> They do nothing if the MSI/MSI-X is not in use.
> 
> CC: Alexander Graf <agraf@suse.de>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> CC: Isaku Yamahata <yamahata@valinux.co.jp>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

What makes me unhappy with this proposal is that msix_write_config, for
example, becomes in fact an internal interface. So devices should be
calling some functions like msix_init from msix.h, but not others like
msix_write_config.

It used to be simple: devices should call msix_.
Now, how are devices to figure it out?

E.g. the comment near msix_write_config says:
/* Handle MSI-X capability config write. */

This puts it at level 11 on Rusty's misuse scale:
Read the documentation and you will get it wrong.

So I tried writing a wapper, something like pci_capability.h, that would
hide the detail and handle all capabilities seamlessly.  Where I got
stuck was migration though, format is ordered so we can't just move the
fields around.  So I decided to wait until we switch to an unordered
format, then it'll become easy.

Thoughts?

> ---
>  hw/ioh3420.c            |    2 +-
>  hw/pci.c                |    5 +++++
>  hw/pci_bridge.c         |    4 ++++
>  hw/virtio-pci.c         |    1 -
>  hw/xio3130_downstream.c |    2 +-
>  hw/xio3130_upstream.c   |    1 -
>  6 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ioh3420.c b/hw/ioh3420.c
> index a6bfbb9..fc2fb3b 100644
> --- a/hw/ioh3420.c
> +++ b/hw/ioh3420.c
> @@ -81,7 +81,7 @@ static void ioh3420_write_config(PCIDevice *d,
>  static void ioh3420_reset(DeviceState *qdev)
>  {
>      PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev);
> -    msi_reset(d);
> +
>      ioh3420_aer_vector_update(d);
>      pcie_cap_root_reset(d);
>      pcie_cap_deverr_reset(d);
> diff --git a/hw/pci.c b/hw/pci.c
> index 399227f..5d5829d 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -31,6 +31,8 @@
>  #include "loader.h"
>  #include "range.h"
>  #include "qmp-commands.h"
> +#include "msi.h"
> +#include "msix.h"
>  
>  //#define DEBUG_PCI
>  #ifdef DEBUG_PCI
> @@ -191,6 +193,9 @@ void pci_device_reset(PCIDevice *dev)
>          }
>      }
>      pci_update_mappings(dev);
> +
> +    msi_reset(dev);
> +    msix_reset(dev);
>  }
>  
>  /*
> diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> index 650d165..6799978 100644
> --- a/hw/pci_bridge.c
> +++ b/hw/pci_bridge.c
> @@ -32,6 +32,8 @@
>  #include "pci_bridge.h"
>  #include "pci_internals.h"
>  #include "range.h"
> +#include "msi.h"
> +#include "msix.h"
>  
>  /* PCI bridge subsystem vendor ID helper functions */
>  #define PCI_SSVID_SIZEOF        8
> @@ -296,6 +298,8 @@ void pci_bridge_reset(DeviceState *qdev)
>  {
>      PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, qdev);
>      pci_bridge_reset_reg(dev);
> +    msi_reset(dev);
> +    msix_reset(dev);
>  }
>  
>  /* default qdev initialization function for PCI-to-PCI bridge */
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 64c6a94..16a5b08 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -271,7 +271,6 @@ static void virtio_pci_reset(DeviceState *d)
>      VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
>      virtio_pci_stop_ioeventfd(proxy);
>      virtio_reset(proxy->vdev);
> -    msix_reset(&proxy->pci_dev);
>      proxy->flags &= ~VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
>  }
>  
> diff --git a/hw/xio3130_downstream.c b/hw/xio3130_downstream.c
> index d3c387d..464eefa 100644
> --- a/hw/xio3130_downstream.c
> +++ b/hw/xio3130_downstream.c
> @@ -48,7 +48,7 @@ static void xio3130_downstream_write_config(PCIDevice *d, uint32_t address,
>  static void xio3130_downstream_reset(DeviceState *qdev)
>  {
>      PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev);
> -    msi_reset(d);
> +
>      pcie_cap_deverr_reset(d);
>      pcie_cap_slot_reset(d);
>      pcie_cap_ari_reset(d);
> diff --git a/hw/xio3130_upstream.c b/hw/xio3130_upstream.c
> index 8283695..0d8d254 100644
> --- a/hw/xio3130_upstream.c
> +++ b/hw/xio3130_upstream.c
> @@ -47,7 +47,6 @@ static void xio3130_upstream_write_config(PCIDevice *d, uint32_t address,
>  static void xio3130_upstream_reset(DeviceState *qdev)
>  {
>      PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev);
> -    msi_reset(d);
>      pci_bridge_reset(qdev);
>      pcie_cap_deverr_reset(d);
>  }
> -- 
> 1.7.3.4

Re: [Qemu-devel] [PATCH 4/6] msi: Invoke msi/msix_reset from PCI core

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2137 bytes --]

On 2011-12-04 15:24, Michael S. Tsirkin wrote:
> On Sun, Dec 04, 2011 at 02:22:12PM +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> There is no point in pushing this burden to the devices, they may rather
>> forget to call them (like intel-hda and ahci ATM). Instead, reset
>> functions are now called from pci_device_reset and pci_bridge_reset.
>> They do nothing if the MSI/MSI-X is not in use.
>>
>> CC: Alexander Graf <agraf@suse.de>
>> CC: Gerd Hoffmann <kraxel@redhat.com>
>> CC: Isaku Yamahata <yamahata@valinux.co.jp>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> What makes me unhappy with this proposal is that msix_write_config, for
> example, becomes in fact an internal interface. So devices should be
> calling some functions like msix_init from msix.h, but not others like
> msix_write_config.
> 
> It used to be simple: devices should call msix_.
> Now, how are devices to figure it out?
> 
> E.g. the comment near msix_write_config says:
> /* Handle MSI-X capability config write. */

That should be aligned to msi_write_config's comment.

My goal is to reduce the number of calls devices have to do in order to
use MSI. We have quite a few correct examples by now, so it should not
be too hard to figure out what to do to use standard MSI[X] services.

Maybe a PCI skeleton device model would help further. Or up-to-date
documentation, thought that may be even harder. ;)

> 
> This puts it at level 11 on Rusty's misuse scale:
> Read the documentation and you will get it wrong.
> 
> So I tried writing a wapper, something like pci_capability.h, that would
> hide the detail and handle all capabilities seamlessly.  Where I got
> stuck was migration though, format is ordered so we can't just move the
> fields around.  So I decided to wait until we switch to an unordered
> format, then it'll become easy.
> 
> Thoughts?

MSI-X save/restore is, well, unfortunate. Just like the whole PCI layer
in this regard. But I don't think that should block this particular step
as it frees device models from an unneeded burden.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH 4/6] msi: Invoke msi/msix_reset from PCI core

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1816 bytes --]

On 2011-12-04 15:48, Michael S. Tsirkin wrote:
> On Sun, Dec 04, 2011 at 03:35:38PM +0100, Jan Kiszka wrote:
>> On 2011-12-04 15:24, Michael S. Tsirkin wrote:
>>> On Sun, Dec 04, 2011 at 02:22:12PM +0100, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> There is no point in pushing this burden to the devices, they may rather
>>>> forget to call them (like intel-hda and ahci ATM). Instead, reset
>>>> functions are now called from pci_device_reset and pci_bridge_reset.
>>>> They do nothing if the MSI/MSI-X is not in use.
>>>>
>>>> CC: Alexander Graf <agraf@suse.de>
>>>> CC: Gerd Hoffmann <kraxel@redhat.com>
>>>> CC: Isaku Yamahata <yamahata@valinux.co.jp>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> What makes me unhappy with this proposal is that msix_write_config, for
>>> example, becomes in fact an internal interface. So devices should be
>>> calling some functions like msix_init from msix.h, but not others like
>>> msix_write_config.
>>>
>>> It used to be simple: devices should call msix_.
>>> Now, how are devices to figure it out?
>>>
>>> E.g. the comment near msix_write_config says:
>>> /* Handle MSI-X capability config write. */
>>
>> That should be aligned to msi_write_config's comment.
>>
>> My goal is to reduce the number of calls devices have to do in order to
>> use MSI. We have quite a few correct examples by now, so it should not
>> be too hard to figure out what to do to use standard MSI[X] services.
>>
>> Maybe a PCI skeleton device model would help further. Or up-to-date
>> documentation, thought that may be even harder. ;)
> 
> Maybe it's time to move code into hw/pci/ ?
> Then we could have private interfaces without
> kludges like pci_internals.h ...
> 

Sounds reasonable.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH 4/6] msi: Invoke msi/msix_reset from PCI core

[Michael S. Tsirkin]

On Sun, Dec 04, 2011 at 03:35:38PM +0100, Jan Kiszka wrote:
> On 2011-12-04 15:24, Michael S. Tsirkin wrote:
> > On Sun, Dec 04, 2011 at 02:22:12PM +0100, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> There is no point in pushing this burden to the devices, they may rather
> >> forget to call them (like intel-hda and ahci ATM). Instead, reset
> >> functions are now called from pci_device_reset and pci_bridge_reset.
> >> They do nothing if the MSI/MSI-X is not in use.
> >>
> >> CC: Alexander Graf <agraf@suse.de>
> >> CC: Gerd Hoffmann <kraxel@redhat.com>
> >> CC: Isaku Yamahata <yamahata@valinux.co.jp>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > What makes me unhappy with this proposal is that msix_write_config, for
> > example, becomes in fact an internal interface. So devices should be
> > calling some functions like msix_init from msix.h, but not others like
> > msix_write_config.
> > 
> > It used to be simple: devices should call msix_.
> > Now, how are devices to figure it out?
> > 
> > E.g. the comment near msix_write_config says:
> > /* Handle MSI-X capability config write. */
> 
> That should be aligned to msi_write_config's comment.
> 
> My goal is to reduce the number of calls devices have to do in order to
> use MSI. We have quite a few correct examples by now, so it should not
> be too hard to figure out what to do to use standard MSI[X] services.
> 
> Maybe a PCI skeleton device model would help further. Or up-to-date
> documentation, thought that may be even harder. ;)

Maybe it's time to move code into hw/pci/ ?
Then we could have private interfaces without
kludges like pci_internals.h ...

-- 
MST