msi: Refactorings and reset fixes

PATCH Applied Jan Kiszka 3 participants 18 messages May 11, 2012 – Jun 4, 2012

lore

Share & tools

lore

Jan Kiszka

author

2012-05-11 14:43 UTC #0 lore raw

v3 cover msi: Refactorings and reset fixes

v3 is v2 - xhci changes as MSI is instable there anyway. So, only patches 1 and 2 are for 1.1/stable now.

CC Alexander Graf <[email protected]>

CC Gerd Hoffmann <[email protected]>

CC Isaku Yamahata <[email protected]>

CC [email protected]

Jan Kiszka (8):
ahci: Fix reset of MSI function
intel-hda: Fix reset of MSI function
ahci: Clean up reset functions
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/ahci.c           |   25 +++++++++++++++----------
 hw/ide/ahci.h           |    2 +-
 hw/ide/ich.c            |   19 ++++++++-----------
 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 +--
 12 files changed, 56 insertions(+), 52 deletions(-)

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2012-05-11 14:43 UTC #1 lore raw

PATCH v3 1/8 ahci: Fix reset of MSI function

Call msi_reset on device reset as still required by the core.

CC Alexander Graf <[email protected]>

CC [email protected]

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

---
 hw/ide/ich.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

hw/ide/ich.c +10 -2

index 560ae37..242254e 100644
@@ -84,6 +84,14 @@ static const VMStateDescription vmstate_ahci = {
     .unmigratable = 1,
 };
 
+static void pci_ich9_reset(void *opaque)
+{
+    struct AHCIPCIState *d = opaque;
+
+    msi_reset(&d->card);
+    ahci_reset(opaque);
+}
+
 static int pci_ich9_ahci_init(PCIDevice *dev)
 {
     struct AHCIPCIState *d;

@@ -102,7 +110,7 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
     /* XXX Software should program this register */
     d->card.config[0x90]   = 1 << 6; /* Address Map Register - AHCI mode */
 
-    qemu_register_reset(ahci_reset, d);
+    qemu_register_reset(pci_ich9_reset, d);
 
     msi_init(dev, 0x50, 1, true, false);
     d->ahci.irq = d->card.irq[0];

@@ -133,7 +141,7 @@ static int pci_ich9_uninit(PCIDevice *dev)
     d = DO_UPCAST(struct AHCIPCIState, card, dev);
 
     msi_uninit(dev);
-    qemu_unregister_reset(ahci_reset, d);
+    qemu_unregister_reset(pci_ich9_reset, d);
     ahci_uninit(&d->ahci);
 
     return 0;

Signature · 1 line

1.7.3.4

Alexander Graf

Acked-by on v3 1/8

2012-05-22 10:18 UTC #2 lore raw

Re: [Qemu-devel] [PATCH v3 1/8] ahci: Fix reset of MSI function

Quoted 5 lines · Jan Kiszka

Call msi_reset on device reset as still required by the core.
CC Alexander Graf <[email protected]>
CC [email protected]
Signed-off-by Jan Kiszka <[email protected]>

Acked-by Alexander Graf <[email protected]>

Alex

Jan Kiszka

author

2012-05-11 14:43 UTC #3 lore raw

PATCH v3 2/8 intel-hda: Fix reset of MSI function

In reply to Jan Kiszka

Call msi_reset on device reset as still required by the core.

CC Gerd Hoffmann <[email protected]>

CC [email protected]

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

---
 hw/intel-hda.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

hw/intel-hda.c +3 -0

index bb11af2..e38861e 100644
@@ -1107,6 +1107,9 @@ static void intel_hda_reset(DeviceState *dev)
     DeviceState *qdev;
     HDACodecDevice *cdev;
 
+    if (d->msi) {
+        msi_reset(&d->pci);
+    }
     intel_hda_regs_reset(d);
     d->wall_base_ns = qemu_get_clock_ns(vm_clock);

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2012-05-11 14:43 UTC #4 lore raw

PATCH v3 3/8 ahci: Clean up reset functions

In reply to Jan Kiszka

Properly register reset functions via the device class.

CC Alexander Graf <[email protected]>

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

---
 hw/ide/ahci.c |   25 +++++++++++++++----------
 hw/ide/ahci.h |    2 +-
 hw/ide/ich.c  |   10 ++++------
 3 files changed, 20 insertions(+), 17 deletions(-)

hw/ide/ahci.c +15 ~~-10~~

index a883a92..e992d76 100644
@@ -339,7 +339,7 @@ static void ahci_mem_write(void *opaque, target_phys_addr_t addr,
             case HOST_CTL: /* R/W */
                 if (val & HOST_CTL_RESET) {
                     DPRINTF(-1, "HBA Reset\n");
-                    ahci_reset(container_of(s, AHCIPCIState, ahci));
+                    ahci_reset(s);
                 } else {
                     s->control_regs.ghc = (val & 0x3) | HOST_CTL_AHCI_EN;
                     ahci_check_irq(s);

@@ -1149,21 +1149,20 @@ void ahci_uninit(AHCIState *s)
     g_free(s->dev);
 }
 
-void ahci_reset(void *opaque)
+void ahci_reset(AHCIState *s)
 {
-    struct AHCIPCIState *d = opaque;
     AHCIPortRegs *pr;
     int i;
 
-    d->ahci.control_regs.irqstatus = 0;
-    d->ahci.control_regs.ghc = 0;
+    s->control_regs.irqstatus = 0;
+    s->control_regs.ghc = 0;
 
-    for (i = 0; i < d->ahci.ports; i++) {
-        pr = &d->ahci.dev[i].port_regs;
+    for (i = 0; i < s->ports; i++) {
+        pr = &s->dev[i].port_regs;
         pr->irq_stat = 0;
         pr->irq_mask = 0;
         pr->scr_ctl = 0;
-        ahci_reset_port(&d->ahci, i);
+        ahci_reset_port(s, i);
     }
 }

@@ -1178,6 +1177,13 @@ static const VMStateDescription vmstate_sysbus_ahci = {
     .unmigratable = 1,
 };
 
+static void sysbus_ahci_reset(DeviceState *dev)
+{
+    SysbusAHCIState *s = DO_UPCAST(SysbusAHCIState, busdev.qdev, dev);
+
+    ahci_reset(&s->ahci);
+}
+
 static int sysbus_ahci_init(SysBusDevice *dev)
 {
     SysbusAHCIState *s = FROM_SYSBUS(SysbusAHCIState, dev);

@@ -1185,8 +1191,6 @@ static int sysbus_ahci_init(SysBusDevice *dev)
 
     sysbus_init_mmio(dev, &s->ahci.mem);
     sysbus_init_irq(dev, &s->ahci.irq);
-
-    qemu_register_reset(ahci_reset, &s->ahci);
     return 0;
 }

@@ -1203,6 +1207,7 @@ static void sysbus_ahci_class_init(ObjectClass *klass, void *data)
     sbc->init = sysbus_ahci_init;
     dc->vmsd = &vmstate_sysbus_ahci;
     dc->props = sysbus_ahci_properties;
+    dc->reset = sysbus_ahci_reset;
 }
 
 static TypeInfo sysbus_ahci_info = {

hw/ide/ahci.h +1 -1

index b223d2c..ec1b6a5 100644
@@ -332,6 +332,6 @@ typedef struct NCQFrame {
 void ahci_init(AHCIState *s, DeviceState *qdev, int ports);
 void ahci_uninit(AHCIState *s);
 
-void ahci_reset(void *opaque);
+void ahci_reset(AHCIState *s);
 
 #endif /* HW_IDE_AHCI_H */

hw/ide/ich.c +4 -6

index 242254e..e7026bb 100644
@@ -84,12 +84,12 @@ static const VMStateDescription vmstate_ahci = {
     .unmigratable = 1,
 };
 
-static void pci_ich9_reset(void *opaque)
+static void pci_ich9_reset(DeviceState *dev)
 {
-    struct AHCIPCIState *d = opaque;
+    struct AHCIPCIState *d = DO_UPCAST(struct AHCIPCIState, card.qdev, dev);
 
     msi_reset(&d->card);
-    ahci_reset(opaque);
+    ahci_reset(&d->ahci);
 }
 
 static int pci_ich9_ahci_init(PCIDevice *dev)

@@ -110,8 +110,6 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
     /* XXX Software should program this register */
     d->card.config[0x90]   = 1 << 6; /* Address Map Register - AHCI mode */
 
-    qemu_register_reset(pci_ich9_reset, d);
-
     msi_init(dev, 0x50, 1, true, false);
     d->ahci.irq = d->card.irq[0];

@@ -141,7 +139,6 @@ static int pci_ich9_uninit(PCIDevice *dev)
     d = DO_UPCAST(struct AHCIPCIState, card, dev);
 
     msi_uninit(dev);
-    qemu_unregister_reset(pci_ich9_reset, d);
     ahci_uninit(&d->ahci);
 
     return 0;

@@ -167,6 +164,7 @@ static void ich_ahci_class_init(ObjectClass *klass, void *data)
     k->revision = 0x02;
     k->class_id = PCI_CLASS_STORAGE_SATA;
     dc->vmsd = &vmstate_ahci;
+    dc->reset = pci_ich9_reset;
 }
 
 static TypeInfo ich_ahci_info = {

Signature · 1 line

1.7.3.4

Alexander Graf

Reviewed-by on v3 3/8

2012-05-22 10:20 UTC #5 lore raw

Re: [Qemu-devel] [PATCH v3 3/8] ahci: Clean up reset functions

Quoted 4 lines · Jan Kiszka

Properly register reset functions via the device class.
CC Alexander Graf <[email protected]>
Signed-off-by Jan Kiszka <[email protected]>

Looks good to me, but I'm no infrastructure expert :)

Reviewed-by Alexander Graf <[email protected]>

Alex

Jan Kiszka

author

2012-05-11 14:43 UTC #6 lore raw

PATCH v3 4/8 msi: Guard msi_reset with msi_present

In reply to Jan Kiszka

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

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

hw/msi.c +4 -0

index 5d6ceb6..b2903fc 100644
@@ -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;

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2012-05-11 14:43 UTC #7 lore raw

PATCH v3 5/8 msi: Invoke msi/msix_reset from PCI core

In reply to Jan Kiszka

There is no point in pushing this burden to the devices, they may rather forget to call them (like intel-hda, ahci, xhci did). 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 <[email protected]>

CC Gerd Hoffmann <[email protected]>

CC Isaku Yamahata <[email protected]>

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

---
 hw/ide/ich.c            |    1 -
 hw/intel-hda.c          |    3 ---
 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 +-
 8 files changed, 12 insertions(+), 8 deletions(-)

hw/ide/ich.c +0 -1

index e7026bb..d3bc822 100644
@@ -88,7 +88,6 @@ static void pci_ich9_reset(DeviceState *dev)
 {
     struct AHCIPCIState *d = DO_UPCAST(struct AHCIPCIState, card.qdev, dev);
 
-    msi_reset(&d->card);
     ahci_reset(&d->ahci);
 }

hw/intel-hda.c +0 -3

index e38861e..bb11af2 100644
@@ -1107,9 +1107,6 @@ static void intel_hda_reset(DeviceState *dev)
     DeviceState *qdev;
     HDACodecDevice *cdev;
 
-    if (d->msi) {
-        msi_reset(&d->pci);
-    }
     intel_hda_regs_reset(d);
     d->wall_base_ns = qemu_get_clock_ns(vm_clock);

hw/ioh3420.c +1 -1

index 1632d31..d1499da 100644
@@ -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);

hw/pci.c +5 -0

index b706e69..2148245 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

@@ -188,6 +190,9 @@ void pci_device_reset(PCIDevice *dev)
         }
     }
     pci_update_mappings(dev);
+
+    msi_reset(dev);
+    msix_reset(dev);
 }
 
 /*

hw/pci_bridge.c +4 -0

index 866f0b6..b533574 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 = PCI_DEVICE(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 4a4413d..3395a02 100644
@@ -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;
 }

hw/xio3130_downstream.c +1 -1

index 319624f..3716e45 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 = PCI_DEVICE(qdev);
-    msi_reset(d);
+
     pcie_cap_deverr_reset(d);
     pcie_cap_slot_reset(d);
     pcie_cap_ari_reset(d);

hw/xio3130_upstream.c +1 -1

index 34a99bb..962d48e 100644
@@ -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);
 }

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2012-05-15 23:10 UTC #8 lore raw

PATCH v4 5/8 msi: Invoke msi/msix_reset from PCI core

There is no point in pushing this burden to the devices, they tend to forget to call them (like intel-hda, ahci, xhci did). Instead, reset functions are now called from pci_device_reset. They do nothing if 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]>

---

Changes in v4:
- removed redundant msi_reset from bridges and msix_reset from ivshmem

 hw/ide/ich.c            |    1 -
 hw/intel-hda.c          |    3 ---
 hw/ioh3420.c            |    2 +-
 hw/ivshmem.c            |    1 -
 hw/pci.c                |    5 +++++
 hw/pci_bridge.c         |   10 ++--------
 hw/pci_bridge_dev.c     |    4 +---
 hw/virtio-pci.c         |    1 -
 hw/xio3130_downstream.c |    2 +-
 hw/xio3130_upstream.c   |    2 +-
 10 files changed, 11 insertions(+), 20 deletions(-)

hw/ide/ich.c +0 -1

index e7026bb..d3bc822 100644
@@ -88,7 +88,6 @@ static void pci_ich9_reset(DeviceState *dev)
 {
     struct AHCIPCIState *d = DO_UPCAST(struct AHCIPCIState, card.qdev, dev);
 
-    msi_reset(&d->card);
     ahci_reset(&d->ahci);
 }

hw/intel-hda.c +0 -3

index e38861e..bb11af2 100644
@@ -1107,9 +1107,6 @@ static void intel_hda_reset(DeviceState *dev)
     DeviceState *qdev;
     HDACodecDevice *cdev;
 
-    if (d->msi) {
-        msi_reset(&d->pci);
-    }
     intel_hda_regs_reset(d);
     d->wall_base_ns = qemu_get_clock_ns(vm_clock);

hw/ioh3420.c +1 -1

index 1632d31..d1499da 100644
@@ -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);

hw/ivshmem.c +0 -1

index d48e5f9..05559b6 100644
@@ -530,7 +530,6 @@ static void ivshmem_reset(DeviceState *d)
     IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
 
     s->intrstatus = 0;
-    msix_reset(&s->dev);
     ivshmem_use_msix(s);
     return;
 }

hw/pci.c +5 -0

index b706e69..2148245 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

@@ -188,6 +190,9 @@ void pci_device_reset(PCIDevice *dev)
         }
     }
     pci_update_mappings(dev);
+
+    msi_reset(dev);
+    msix_reset(dev);
 }
 
 /*

hw/pci_bridge.c +2 -8

index 866f0b6..e0832b4 100644
@@ -254,8 +254,9 @@ void pci_bridge_disable_base_limit(PCIDevice *dev)
 }
 
 /* reset bridge specific configuration registers */
-void pci_bridge_reset_reg(PCIDevice *dev)
+void pci_bridge_reset(DeviceState *qdev)
 {
+    PCIDevice *dev = PCI_DEVICE(qdev);
     uint8_t *conf = dev->config;
 
     conf[PCI_PRIMARY_BUS] = 0;

@@ -291,13 +292,6 @@ void pci_bridge_reset_reg(PCIDevice *dev)
     pci_set_word(conf + PCI_BRIDGE_CONTROL, 0);
 }
 
-/* default reset function for PCI-to-PCI bridge */
-void pci_bridge_reset(DeviceState *qdev)
-{
-    PCIDevice *dev = PCI_DEVICE(qdev);
-    pci_bridge_reset_reg(dev);
-}
-
 /* default qdev initialization function for PCI-to-PCI bridge */
 int pci_bridge_initfn(PCIDevice *dev)
 {

hw/pci_bridge_dev.c +1 -3

index eccaa58..b6d5fb7 100644
@@ -119,10 +119,8 @@ static void pci_bridge_dev_write_config(PCIDevice *d,
 static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
 {
     PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, qdev);
+
     pci_bridge_reset(qdev);
-    if (msi_present(dev)) {
-        msi_reset(dev);
-    }
     shpc_reset(dev);
 }

hw/virtio-pci.c +0 -1

index 4a4413d..3395a02 100644
@@ -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;
 }

hw/xio3130_downstream.c +1 -1

index 319624f..3716e45 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 = PCI_DEVICE(qdev);
-    msi_reset(d);
+
     pcie_cap_deverr_reset(d);
     pcie_cap_slot_reset(d);
     pcie_cap_ari_reset(d);

hw/xio3130_upstream.c +1 -1

index 34a99bb..962d48e 100644
@@ -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);
 }

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2012-05-11 14:43 UTC #9 lore raw

PATCH v3 6/8 msi: Guard msi/msix_write_config with msi_present

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

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

hw/msi.c +2 -1

index b2903fc..da12f33 100644
@@ -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;
     }

hw/msix.c +1 -1

index 3835eaa..84915d8 100644
@@ -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;
     }

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2012-05-11 14:43 UTC #10 lore raw

PATCH v3 7/8 msi: Invoke msi/msix_write_config from PCI core

In reply to 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 <[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 d3bc822..e3eaaea 100644
@@ -143,13 +143,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);

@@ -157,7 +150,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;

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

index bb11af2..8f3b70b 100644
@@ -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;

hw/ioh3420.c +0 -1

index d1499da..0a2601c 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 da12f33..556c7c4 100644
@@ -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));

hw/pci.c +3 -0

index 2148245..439f3ce 100644
@@ -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);
 }
 
 /***********************************************************/

hw/virtio-pci.c +0 -2

index 3395a02..985acfc 100644
@@ -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)

hw/xio3130_downstream.c +0 -1

index 3716e45..56d1b35 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 962d48e..7972581 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

2012-05-11 14:43 UTC #11 lore raw

PATCH v3 8/8 msi: Use msi/msix_present more consistently

In reply to Jan Kiszka

Replace some open-coded msi/msix_present checks.

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 556c7c4..5233204 100644
@@ -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));

hw/msix.c +8 -5

index 84915d8..7c314bb 100644
@@ -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);
 }

Signature · 1 line

1.7.3.4

Michael S. Tsirkin

2012-06-04 14:48 UTC #12 lore raw

Re: [Qemu-devel] [PATCH v3 8/8] msi: Use msi/msix_present more consistently

Quoted 3 lines · Jan Kiszka

Replace some open-coded msi/msix_present checks.
Signed-off-by Jan Kiszka <[email protected]>

This last one doesn't apply but I don't expect this blocks anything. Applied 1-7 for now.

Quoted 74 lines

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

hw/msi.c +1 -1

index 556c7c4..5233204 100644
@@ -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));

hw/msix.c +8 -5

index 84915d8..7c314bb 100644
@@ -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);
 }

Signature · 1 line

1.7.3.4

Jan Kiszka

author

2012-06-04 14:54 UTC #13 lore raw

PATCH v4 8/8 msi: Use msi/msix_present more consistently

Quoted 7 lines · Michael S. Tsirkin

Quoted 3 lines · Jan Kiszka
Replace some open-coded msi/msix_present checks.
Signed-off-by Jan Kiszka <[email protected]>
This last one doesn't apply but I don't expect this blocks anything. Applied 1-7 for now.

Hmm, might have rebased here but forgot to send:

---8<----

Replace some open-coded msi/msix_present checks.

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 556c7c4..5233204 100644
@@ -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));

hw/msix.c +8 -5

index 339e652..ded3c55 100644
@@ -319,8 +319,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);

@@ -339,7 +340,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;
     }

@@ -353,7 +354,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
     unsigned n = dev->msix_entries_nr;
     unsigned int vector;
 
-    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
+    if (!msix_present(dev)) {
         return;
     }

@@ -407,8 +408,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];

@@ -447,8 +449,9 @@ 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

Michael S. Tsirkin

Applied

2012-06-04 18:57 UTC #14 lore raw

Re: [Qemu-devel] [PATCH v4 8/8] msi: Use msi/msix_present more consistently

Quoted 9 lines · Jan Kiszka

Quoted 7 lines · Michael S. Tsirkin
Quoted 3 lines · Jan Kiszka
Replace some open-coded msi/msix_present checks.
Signed-off-by Jan Kiszka <[email protected]>
This last one doesn't apply but I don't expect this blocks anything. Applied 1-7 for now.

Quoted 7 lines

Hmm, might have rebased here but forgot to send:
---8<----
Replace some open-coded msi/msix_present checks.
Signed-off-by Jan Kiszka <[email protected]>

Applied, thanks.

Quoted 75 lines

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

hw/msi.c +1 -1

index 556c7c4..5233204 100644
@@ -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));

hw/msix.c +8 -5

index 339e652..ded3c55 100644
@@ -319,8 +319,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);

@@ -339,7 +340,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;
     }

@@ -353,7 +354,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
     unsigned n = dev->msix_entries_nr;
     unsigned int vector;
 
-    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
+    if (!msix_present(dev)) {
         return;
     }

@@ -407,8 +408,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];

@@ -447,8 +449,9 @@ 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

Alexander Graf

2012-05-22 10:16 UTC #15 lore raw

In reply to Jan Kiszka

Quoted 2 lines · Jan Kiszka

v3 is v2 - xhci changes as MSI is instable there anyway. So, only patches 1 and 2 are for 1.1/stable now.

Just saw these in my inbox as unanswered, so mst: ping? :)

Alex

Jan Kiszka

author

2012-06-04 14:18 UTC #16 lore raw

In reply to Jan Kiszka

Michael,

Quoted 2 lines · Jan Kiszka

v3 is v2 - xhci changes as MSI is instable there anyway. So, only patches 1 and 2 are for 1.1/stable now.

Unless concerns remain, please push this queue (with v4 of patch 5) soon
- to get the stable stuff in and to help further work on the MSI layer for PCI device assignment.

Thanks,
Jan

Quoted 30 lines

CC Alexander Graf <[email protected]>
CC Gerd Hoffmann <[email protected]>
CC Isaku Yamahata <[email protected]>
CC [email protected]
Jan Kiszka (8):
ahci: Fix reset of MSI function
intel-hda: Fix reset of MSI function
ahci: Clean up reset functions
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/ahci.c           |   25 +++++++++++++++----------
 hw/ide/ahci.h           |    2 +-
 hw/ide/ich.c            |   19 ++++++++-----------
 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 +--
 12 files changed, 56 insertions(+), 52 deletions(-)

Signature · 2 lines

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

Michael S. Tsirkin

Applied

2012-06-04 14:28 UTC #17 lore raw

Quoted 6 lines · Jan Kiszka

Michael,
Quoted 2 lines · Jan Kiszka
v3 is v2 - xhci changes as MSI is instable there anyway. So, only patches 1 and 2 are for 1.1/stable now.

Quoted 6 lines

Unless concerns remain, please push this queue (with v4 of patch 5) soon
- to get the stable stuff in and to help further work on the MSI layer for PCI device assignment.
Thanks,
Jan

Applied, thanks.

Quoted 34 lines

Quoted 30 lines
CC Alexander Graf <[email protected]>
CC Gerd Hoffmann <[email protected]>
CC Isaku Yamahata <[email protected]>
CC [email protected]
Jan Kiszka (8):
ahci: Fix reset of MSI function
intel-hda: Fix reset of MSI function
ahci: Clean up reset functions
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/ahci.c           |   25 +++++++++++++++----------
 hw/ide/ahci.h           |    2 +-
 hw/ide/ich.c            |   19 ++++++++-----------
 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 +--
 12 files changed, 56 insertions(+), 52 deletions(-)
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux