msi: Small refactoring

PATCH In review Jan Kiszka 2 participants 11 messages Dec 4, 2011

Share & tools

Jan Kiszka

author

2011-12-04 13:22 UTC #0 lore raw

cover msi: Small refactoring

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

CC Alexander Graf <[email protected]>

CC Gerd Hoffmann <[email protected]>

CC Isaku Yamahata <[email protected]>

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(-)

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2011-12-04 13:22 UTC #1 lore raw

PATCH 1/6 msi: Guard msi/msix_write_config with msi_present

From: Jan Kiszka <[email protected]>

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 <[email protected]>

---
 hw/msi.c  |    3 ++-
 hw/msix.c |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

hw/msi.c +2 -1

index f214fcf..541e4e1 100644
@@ -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;
     }

hw/msix.c +1 -1

index 149eed2..32fd9b2 100644
@@ -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;
     }

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2011-12-04 13:22 UTC #2 lore raw

PATCH 2/6 msi: Guard msi_reset with msi_present

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/msi.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

hw/msi.c +4 -0

index 541e4e1..612b168 100644
@@ -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;

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2011-12-04 13:22 UTC #3 lore raw

PATCH 3/6 msi: Use msi/msix_present more consistently

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

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

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/msi.c  |    2 +-
 hw/msix.c |   13 ++++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

hw/msi.c +1 -1

index 612b168..137dba0 100644
@@ -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));

hw/msix.c +8 -5

index 32fd9b2..876793a 100644
@@ -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);
 }

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2011-12-04 13:22 UTC #4 lore raw

PATCH 4/6 msi: Invoke msi/msix_reset from PCI core

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

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 <[email protected]>

CC Gerd Hoffmann <[email protected]>

CC Isaku Yamahata <[email protected]>

Signed-off-by Jan Kiszka <[email protected]>

---
 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(-)

hw/ioh3420.c +1 -1

index a6bfbb9..fc2fb3b 100644
@@ -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);

hw/pci.c +5 -0

index 399227f..5d5829d 100644
@@ -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);
 }
 
 /*

hw/pci_bridge.c +4 -0

index 650d165..6799978 100644
@@ -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 */

hw/virtio-pci.c +0 -1

index 64c6a94..16a5b08 100644
@@ -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;
 }

hw/xio3130_downstream.c +1 -1

index d3c387d..464eefa 100644
@@ -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);

hw/xio3130_upstream.c +0 -1

index 8283695..0d8d254 100644
@@ -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);
 }

Signature · 1 line

1.7.3.4

Michael S. Tsirkin

2011-12-04 14:22 UTC #5 lore raw

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

Quoted 11 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>
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 <[email protected]>
CC Gerd Hoffmann <[email protected]>
CC Isaku Yamahata <[email protected]>
Signed-off-by Jan Kiszka <[email protected]>

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?

Quoted 106 lines

---
 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(-)

hw/ioh3420.c +1 -1

index a6bfbb9..fc2fb3b 100644
@@ -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);

hw/pci.c +5 -0

index 399227f..5d5829d 100644
@@ -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);
 }
 
 /*

hw/pci_bridge.c +4 -0

index 650d165..6799978 100644
@@ -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 */

hw/virtio-pci.c +0 -1

index 64c6a94..16a5b08 100644
@@ -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;
 }

hw/xio3130_downstream.c +1 -1

index d3c387d..464eefa 100644
@@ -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);

hw/xio3130_upstream.c +0 -1

index 8283695..0d8d254 100644
@@ -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);
 }

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2011-12-04 14:35 UTC #6 lore raw

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

Quoted 18 lines · Michael S. Tsirkin

Quoted 11 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
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 <[email protected]>
CC Gerd Hoffmann <[email protected]>
CC Isaku Yamahata <[email protected]>
Signed-off-by Jan Kiszka <[email protected]>
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.

Quoted 5 lines

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. ;)

Quoted 11 lines

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

Michael S. Tsirkin

2011-12-04 14:47 UTC #7 lore raw

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

Quoted 25 lines · Jan Kiszka

Quoted 18 lines · Michael S. Tsirkin
Quoted 11 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
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 <[email protected]>
CC Gerd Hoffmann <[email protected]>
CC Isaku Yamahata <[email protected]>
Signed-off-by Jan Kiszka <[email protected]>
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.
Quoted 5 lines
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. */

Quoted 8 lines

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 ...

Signature · 1 line

MST

Jan Kiszka

author

2011-12-04 14:48 UTC #8 lore raw

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

Quoted 32 lines · Michael S. Tsirkin

Quoted 31 lines · Jan Kiszka
Quoted 18 lines · Michael S. Tsirkin
Quoted 11 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
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 <[email protected]>
CC Gerd Hoffmann <[email protected]>
CC Isaku Yamahata <[email protected]>
Signed-off-by Jan Kiszka <[email protected]>
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.
Quoted 5 lines
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.

Quoted 7 lines

Quoted 2 lines
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

Jan Kiszka

author

2011-12-04 13:22 UTC #9 lore raw

PATCH 5/6 msi: Invoke msi/msix_write_config from PCI core

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

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 <[email protected]>

CC Gerd Hoffmann <[email protected]>

CC Isaku Yamahata <[email protected]>

Signed-off-by Jan Kiszka <[email protected]>

---
 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(-)

hw/ide/ich.c +0 -8

index 3f7510f..a470c01 100644
@@ -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,

hw/intel-hda.c +0 ~~-12~~

index 10769e0..995d895 100644
@@ -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,

hw/ioh3420.c +0 -1

index fc2fb3b..886ede8 100644
@@ -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);

hw/msi.c +1 -1

index 137dba0..c4e8a6e 100644
@@ -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));

hw/pci.c +3 -0

index 5d5829d..8c814cd 100644
@@ -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);
 }
 
 /***********************************************************/

hw/virtio-pci.c +0 -2

index 16a5b08..d21a7ee 100644
@@ -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)

hw/xio3130_downstream.c +0 -1

index 464eefa..8e9117d 100644
@@ -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);
 }

hw/xio3130_upstream.c +0 -1

index 0d8d254..707401e 100644
@@ -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);
 }

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2011-12-04 13:22 UTC #10 lore raw

PATCH 6/6 msi: Generalize msix_supported to msi_supported

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

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 <[email protected]>

---
 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(-)

hw/msi.c +8 -0

index c4e8a6e..5233204 100644
@@ -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",

hw/msi.h +2 -0

index 5766018..3040bb0 100644
@@ -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);

hw/msix.c +4 -5

index 876793a..4897c58 100644
@@ -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;

hw/msix.h +0 -2

index 7e04336..5aba22b 100644
@@ -29,6 +29,4 @@ void msix_notify(PCIDevice *dev, unsigned vector);
 
 void msix_reset(PCIDevice *dev);
 
-extern int msix_supported;
-
 #endif

hw/pc.c +2 -2

index 33778fe..7e40031 100644
@@ -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;
 }

Signature · 1 line

1.7.3.4