[Qemu-devel] [PATCH 0/5] msi: Refactorings and reset

[Qemu-devel] [PATCH 0/5] msi: Refactorings and reset

[Jan Kiszka]

These patches had been sent before. I now rebased them and reduced them
to a set that has a value independent of the ongoing MSI rework for KVM
integration.

I'm not yet sure if patch 1&2 should be considered for 1.1 as they
implicitly fix the missing msi_reset for intel-hda and ahci. An
alternative could be to add those reset calls the classic way and leave
the refactoring for post-1.1. Maybe Gerd and Alex can comment on this.

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

Jan Kiszka (5):
  msi: Guard msi_reset with msi_present
  msi: Invoke msi/msix_reset from PCI core
  msi: Guard msi/msix_write_config with msi_present
  msi: Invoke msi/msix_write_config from PCI core
  msi: Use msi/msix_present more consistently

 hw/ide/ich.c            |    8 --------
 hw/intel-hda.c          |   12 ------------
 hw/ioh3420.c            |    3 +--
 hw/msi.c                |   11 ++++++++---
 hw/msix.c               |   15 +++++++++------
 hw/pci.c                |    8 ++++++++
 hw/pci_bridge.c         |    4 ++++
 hw/virtio-pci.c         |    3 ---
 hw/xio3130_downstream.c |    3 +--
 hw/xio3130_upstream.c   |    3 +--
 10 files changed, 32 insertions(+), 38 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH 1/5] msi: Guard msi_reset with msi_present

[Jan Kiszka]

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 5d6ceb6..b2903fc 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -191,6 +191,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 2/5] msi: Invoke msi/msix_reset from PCI core

[Jan Kiszka]

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 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   |    2 +-
 6 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/ioh3420.c b/hw/ioh3420.c
index 1632d31..d1499da 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 = PCI_DEVICE(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 b706e69..2148245 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
@@ -188,6 +190,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 866f0b6..b533574 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 = PCI_DEVICE(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 4a4413d..3395a02 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -277,7 +277,6 @@ 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 319624f..3716e45 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 = PCI_DEVICE(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 34a99bb..962d48e 100644
--- a/hw/xio3130_upstream.c
+++ b/hw/xio3130_upstream.c
@@ -47,7 +47,7 @@ static void xio3130_upstream_write_config(PCIDevice *d, uint32_t address,
 static void xio3130_upstream_reset(DeviceState *qdev)
 {
     PCIDevice *d = PCI_DEVICE(qdev);
-    msi_reset(d);
+
     pci_bridge_reset(qdev);
     pcie_cap_deverr_reset(d);
 }
-- 
1.7.3.4

[Qemu-devel] [PATCH 3/5] msi: Guard msi/msix_write_config with msi_present

[Jan Kiszka]

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 b2903fc..da12f33 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -276,7 +276,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 3835eaa..84915d8 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -157,7 +157,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 4/5] msi: Invoke msi/msix_write_config from PCI core

[Jan Kiszka]

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 560ae37..2022fa1 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 void ich_ahci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -153,7 +146,6 @@ static void ich_ahci_class_init(ObjectClass *klass, void *data)
 
     k->init = pci_ich9_ahci_init;
     k->exit = pci_ich9_uninit;
-    k->config_write = pci_ich9_write_config;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82801IR;
     k->revision = 0x02;
diff --git a/hw/intel-hda.c b/hw/intel-hda.c
index bb11af2..8f3b70b 100644
--- a/hw/intel-hda.c
+++ b/hw/intel-hda.c
@@ -1153,17 +1153,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 void intel_hda_class_init(ObjectClass *klass, void *data)
 
     k->init = intel_hda_init;
     k->exit = intel_hda_exit;
-    k->config_write = intel_hda_write_config;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = 0x2668;
     k->revision = 1;
diff --git a/hw/ioh3420.c b/hw/ioh3420.c
index d1499da..0a2601c 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 da12f33..556c7c4 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -264,7 +264,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 2148245..439f3ce 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1042,6 +1042,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 3395a02..985acfc 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -519,8 +519,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 3716e45..56d1b35 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 962d48e..7972581 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 5/5] msi: Use msi/msix_present more consistently

[Jan Kiszka]

Replace some open-coded msi/msix_present checks.

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 556c7c4..5233204 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -175,7 +175,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 84915d8..7c314bb 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -285,8 +285,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);
@@ -305,7 +306,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;
     }
 
@@ -318,7 +319,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;
     }
 
@@ -370,8 +371,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];
@@ -410,7 +412,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

Re: [Qemu-devel] [PATCH 0/5] msi: Refactorings and reset

[Michael S. Tsirkin]

On Thu, May 10, 2012 at 03:22:11PM -0300, Jan Kiszka wrote:
> These patches had been sent before. I now rebased them and reduced them
> to a set that has a value independent of the ongoing MSI rework for KVM
> integration.
> 
> I'm not yet sure if patch 1&2 should be considered for 1.1 as they
> implicitly fix the missing msi_reset for intel-hda and ahci. An
> alternative could be to add those reset calls the classic way and leave
> the refactoring for post-1.1. Maybe Gerd and Alex can comment on this.

I'd prefer that for 1.1, core changes are an unnecessary risk for 1.1.

> CC: Alexander Graf <agraf@suse.de>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> CC: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> Jan Kiszka (5):
>   msi: Guard msi_reset with msi_present
>   msi: Invoke msi/msix_reset from PCI core
>   msi: Guard msi/msix_write_config with msi_present
>   msi: Invoke msi/msix_write_config from PCI core
>   msi: Use msi/msix_present more consistently
> 
>  hw/ide/ich.c            |    8 --------
>  hw/intel-hda.c          |   12 ------------
>  hw/ioh3420.c            |    3 +--
>  hw/msi.c                |   11 ++++++++---
>  hw/msix.c               |   15 +++++++++------
>  hw/pci.c                |    8 ++++++++
>  hw/pci_bridge.c         |    4 ++++
>  hw/virtio-pci.c         |    3 ---
>  hw/xio3130_downstream.c |    3 +--
>  hw/xio3130_upstream.c   |    3 +--
>  10 files changed, 32 insertions(+), 38 deletions(-)
> 
> -- 
> 1.7.3.4

Re: [Qemu-devel] [PATCH 0/5] msi: Refactorings and reset

[Jan Kiszka]

On 2012-05-10 15:58, Michael S. Tsirkin wrote:
> On Thu, May 10, 2012 at 03:22:11PM -0300, Jan Kiszka wrote:
>> These patches had been sent before. I now rebased them and reduced them
>> to a set that has a value independent of the ongoing MSI rework for KVM
>> integration.
>>
>> I'm not yet sure if patch 1&2 should be considered for 1.1 as they
>> implicitly fix the missing msi_reset for intel-hda and ahci. An
>> alternative could be to add those reset calls the classic way and leave
>> the refactoring for post-1.1. Maybe Gerd and Alex can comment on this.
> 
> I'd prefer that for 1.1, core changes are an unnecessary risk for 1.1.

OK, I will fix this first and base the rest of this series on top. Then
we can pull the fixes into 1.1 and stable. XHCI is broken as well, BTW.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux