[Qemu-devel] [PATCH v2 0/9] msi: Small cleanups and fixes

[Qemu-devel] [PATCH v2 0/9] msi: Small cleanups and fixes

[Jan Kiszka]

A few patches to the MSI and MSI-X layer that clean up the interfaces
and fix reset issues. They are from my MSI rework to prepare it for
KVM's requirements (in-kernel irqchip).

In contrast to the previous version, this one moves msi[x]_reset,
msi[x]_write_config and msi[x]_uninit into the PCI core, avoiding
related bugs and reducing boilerplate code on device side.

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

Jan Kiszka (9):
  msi: Fix copy&paste mistake in msi_uninit
  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: Invoke msi/msix_uninit from PCI core
  msix: Align MSI-X constants to libpci definitions and extend them
  msi: Move PCI_MSI_PENDING_32/64 into pci_regs.h

 hw/ide/ich.c            |    9 ---------
 hw/intel-hda.c          |   13 -------------
 hw/ioh3420.c            |    8 ++------
 hw/msi.c                |   17 +++++++++--------
 hw/msix.c               |   39 ++++++++++++++++++++-------------------
 hw/pci.c                |   10 ++++++++++
 hw/pci_bridge.c         |    4 ++++
 hw/pci_regs.h           |   16 ++++++++++------
 hw/virtio-pci.c         |   14 +++-----------
 hw/xio3130_downstream.c |    8 ++------
 hw/xio3130_upstream.c   |    7 +------
 11 files changed, 61 insertions(+), 84 deletions(-)

[Qemu-devel] [PATCH v2 1/9] msi: Fix copy&paste mistake in msi_uninit

[Jan Kiszka]

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

diff --git a/hw/msi.c b/hw/msi.c
index b087fe5..e8c5607 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -172,7 +172,7 @@ void msi_uninit(struct PCIDevice *dev)
     }
     flags = pci_get_word(dev->config + msi_flags_off(dev));
     cap_size = msi_cap_sizeof(flags);
-    pci_del_capability(dev, PCI_CAP_ID_MSIX, cap_size);
+    pci_del_capability(dev, PCI_CAP_ID_MSI, cap_size);
     dev->cap_present &= ~QEMU_PCI_CAP_MSI;
 
     MSI_DEV_PRINTF(dev, "uninit\n");
-- 
1.7.1

[Qemu-devel] [PATCH v2 2/9] 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 e8c5607..b7a92c9 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 af40e26..703f082 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -160,7 +160,7 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
     unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
     int vector;
 
-    if (!range_covers_byte(addr, len, enable_pos)) {
+    if (!msix_present(dev) || !range_covers_byte(addr, len, enable_pos)) {
         return;
     }
 
-- 
1.7.1

[Qemu-devel] [PATCH v2 3/9] 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 b7a92c9..b039893 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.1

[Qemu-devel] [PATCH v2 4/9] msi: Use msi/msix_present more consistently

[Jan Kiszka]

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 b039893..09bcdd1 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 703f082..600f5fb 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -290,8 +290,9 @@ static void msix_free_irq_entries(PCIDevice *dev)
 /* Clean up resources for the device. */
 int msix_uninit(PCIDevice *dev)
 {
-    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);
@@ -309,7 +310,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;
     }
 
@@ -322,7 +323,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;
     }
 
@@ -374,8 +375,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];
@@ -414,7 +416,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.1

[Qemu-devel] [PATCH v2 5/9] 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 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 95adf09..73bc55f 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 1d297d6..967f812 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -31,6 +31,8 @@
 #include "loader.h"
 #include "qemu-objects.h"
 #include "range.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 464d897..3951252 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
@@ -224,6 +226,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 c19629d..075d657 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -310,7 +310,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 5aa6a6b..a587c3e 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 a7640f5..9d75449 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.1

[Qemu-devel] [PATCH v2 6/9] 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         |    1 -
 hw/xio3130_downstream.c |    1 -
 hw/xio3130_upstream.c   |    1 -
 8 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 6150ce3..2aaef10 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -116,13 +116,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",
@@ -130,7 +123,6 @@ static PCIDeviceInfo ich_ahci_info[] = {
         .qdev.size    = sizeof(AHCIPCIState),
         .init         = pci_ich9_ahci_init,
         .exit         = pci_ich9_uninit,
-        .config_write = pci_ich9_write_config,
     },{
         /* end of list */
     }
diff --git a/hw/intel-hda.c b/hw/intel-hda.c
index 5485745..99d9b98 100644
--- a/hw/intel-hda.c
+++ b/hw/intel-hda.c
@@ -1170,17 +1170,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;
@@ -1264,7 +1253,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,
     .qdev.props   = (Property[]) {
         DEFINE_PROP_UINT32("debug", IntelHDAState, debug, 0),
         DEFINE_PROP_UINT32("msi", IntelHDAState, msi, 1),
diff --git a/hw/ioh3420.c b/hw/ioh3420.c
index 73bc55f..3c28648 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 09bcdd1..e23f5df 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -256,7 +256,7 @@ void msi_notify(PCIDevice *dev, unsigned int vector)
     stl_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 967f812..fc2b555 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1119,6 +1119,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 075d657..a181291 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -547,7 +547,6 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
     }
 
     pci_default_write_config(pci_dev, address, val, len);
-    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 a587c3e..933a1ee 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 9d75449..584ffa2 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.1

[Qemu-devel] [PATCH v2 7/9] msi: Invoke msi/msix_uninit from PCI core

[Jan Kiszka]

Again less boilerplate: Clean up MSI/MSI-X unconditionally from the PCI
PCI core. Both services do nothing if there was no support registered.
Eliminates virtio_exit_pci among other things.

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            |    1 -
 hw/intel-hda.c          |    1 -
 hw/ioh3420.c            |    5 +----
 hw/pci.c                |    2 ++
 hw/virtio-pci.c         |   12 +++---------
 hw/xio3130_downstream.c |    5 +----
 hw/xio3130_upstream.c   |    5 +----
 7 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 2aaef10..016bca0 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -109,7 +109,6 @@ static int pci_ich9_uninit(PCIDevice *dev)
     struct AHCIPCIState *d;
     d = DO_UPCAST(struct AHCIPCIState, card, dev);
 
-    msi_uninit(dev);
     qemu_unregister_reset(ahci_reset, d);
     ahci_uninit(&d->ahci);
 
diff --git a/hw/intel-hda.c b/hw/intel-hda.c
index 99d9b98..174f201 100644
--- a/hw/intel-hda.c
+++ b/hw/intel-hda.c
@@ -1165,7 +1165,6 @@ static int intel_hda_exit(PCIDevice *pci)
 {
     IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
 
-    msi_uninit(&d->pci);
     cpu_unregister_io_memory(d->mmio_addr);
     return 0;
 }
diff --git a/hw/ioh3420.c b/hw/ioh3420.c
index 3c28648..1667e66 100644
--- a/hw/ioh3420.c
+++ b/hw/ioh3420.c
@@ -122,7 +122,7 @@ static int ioh3420_initfn(PCIDevice *d)
     }
     rc = pcie_cap_init(d, IOH_EP_EXP_OFFSET, PCI_EXP_TYPE_ROOT_PORT, p->port);
     if (rc < 0) {
-        goto err_msi;
+        goto err_bridge;
     }
     pcie_cap_deverr_init(d);
     pcie_cap_slot_init(d, s->slot);
@@ -145,8 +145,6 @@ err:
     pcie_chassis_del_slot(s);
 err_pcie_cap:
     pcie_cap_exit(d);
-err_msi:
-    msi_uninit(d);
 err_bridge:
     tmp = pci_bridge_exitfn(d);
     assert(!tmp);
@@ -162,7 +160,6 @@ static int ioh3420_exitfn(PCIDevice *d)
     pcie_aer_exit(d);
     pcie_chassis_del_slot(s);
     pcie_cap_exit(d);
-    msi_uninit(d);
     return pci_bridge_exitfn(d);
 }
 
diff --git a/hw/pci.c b/hw/pci.c
index fc2b555..f77ae26 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -783,6 +783,8 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
 
 static void do_pci_unregister_device(PCIDevice *pci_dev)
 {
+    msi_uninit(pci_dev);
+    msix_uninit(pci_dev);
     qemu_free_irqs(pci_dev->irq);
     pci_dev->bus->devices[pci_dev->devfn] = NULL;
     pci_config_free(pci_dev);
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index a181291..1044fce 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -742,11 +742,6 @@ static int virtio_blk_init_pci(PCIDevice *pci_dev)
     return 0;
 }
 
-static int virtio_exit_pci(PCIDevice *pci_dev)
-{
-    return msix_uninit(pci_dev);
-}
-
 static int virtio_blk_exit_pci(PCIDevice *pci_dev)
 {
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
@@ -754,7 +749,7 @@ static int virtio_blk_exit_pci(PCIDevice *pci_dev)
     virtio_pci_stop_ioeventfd(proxy);
     virtio_blk_exit(proxy->vdev);
     blockdev_mark_auto_del(proxy->block.bs);
-    return virtio_exit_pci(pci_dev);
+    return 0;
 }
 
 static int virtio_serial_init_pci(PCIDevice *pci_dev)
@@ -788,7 +783,7 @@ static int virtio_serial_exit_pci(PCIDevice *pci_dev)
 
     virtio_pci_stop_ioeventfd(proxy);
     virtio_serial_exit(proxy->vdev);
-    return virtio_exit_pci(pci_dev);
+    return 0;
 }
 
 static int virtio_net_init_pci(PCIDevice *pci_dev)
@@ -816,7 +811,7 @@ static int virtio_net_exit_pci(PCIDevice *pci_dev)
 
     virtio_pci_stop_ioeventfd(proxy);
     virtio_net_exit(proxy->vdev);
-    return virtio_exit_pci(pci_dev);
+    return 0;
 }
 
 static int virtio_balloon_init_pci(PCIDevice *pci_dev)
@@ -913,7 +908,6 @@ static PCIDeviceInfo virtio_info[] = {
         .qdev.alias = "virtio-balloon",
         .qdev.size = sizeof(VirtIOPCIProxy),
         .init      = virtio_balloon_init_pci,
-        .exit      = virtio_exit_pci,
         .qdev.props = (Property[]) {
             DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
             DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/xio3130_downstream.c b/hw/xio3130_downstream.c
index 933a1ee..62bc7db 100644
--- a/hw/xio3130_downstream.c
+++ b/hw/xio3130_downstream.c
@@ -86,7 +86,7 @@ static int xio3130_downstream_initfn(PCIDevice *d)
     rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_DOWNSTREAM,
                        p->port);
     if (rc < 0) {
-        goto err_msi;
+        goto err_bridge;
     }
     pcie_cap_flr_init(d);
     pcie_cap_deverr_init(d);
@@ -108,8 +108,6 @@ err:
     pcie_chassis_del_slot(s);
 err_pcie_cap:
     pcie_cap_exit(d);
-err_msi:
-    msi_uninit(d);
 err_bridge:
     tmp = pci_bridge_exitfn(d);
     assert(!tmp);
@@ -125,7 +123,6 @@ static int xio3130_downstream_exitfn(PCIDevice *d)
     pcie_aer_exit(d);
     pcie_chassis_del_slot(s);
     pcie_cap_exit(d);
-    msi_uninit(d);
     return pci_bridge_exitfn(d);
 }
 
diff --git a/hw/xio3130_upstream.c b/hw/xio3130_upstream.c
index 584ffa2..3587aa6 100644
--- a/hw/xio3130_upstream.c
+++ b/hw/xio3130_upstream.c
@@ -81,7 +81,7 @@ static int xio3130_upstream_initfn(PCIDevice *d)
     rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_UPSTREAM,
                        p->port);
     if (rc < 0) {
-        goto err_msi;
+        goto err_bridge;
     }
     pcie_cap_flr_init(d);
     pcie_cap_deverr_init(d);
@@ -94,8 +94,6 @@ static int xio3130_upstream_initfn(PCIDevice *d)
 
 err:
     pcie_cap_exit(d);
-err_msi:
-    msi_uninit(d);
 err_bridge:
     tmp =  pci_bridge_exitfn(d);
     assert(!tmp);
@@ -106,7 +104,6 @@ static int xio3130_upstream_exitfn(PCIDevice *d)
 {
     pcie_aer_exit(d);
     pcie_cap_exit(d);
-    msi_uninit(d);
     return pci_bridge_exitfn(d);
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH v2 8/9] msix: Align MSI-X constants to libpci definitions and extend them

[Jan Kiszka]

Add PCI_MSIX_TABLE and PCI_MSIX_PBA, align other MSIX related constant
names to libpci style. Will be used for device assignment code in
qemu-kvm.

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

diff --git a/hw/msix.c b/hw/msix.c
index 600f5fb..b20cf7c 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -16,15 +16,12 @@
 #include "pci.h"
 #include "range.h"
 
-/* MSI-X capability structure */
-#define MSIX_TABLE_OFFSET 4
-#define MSIX_PBA_OFFSET 8
 #define MSIX_CAP_LENGTH 12
 
-/* MSI enable bit and maskall bit are in byte 1 in FLAGS register */
-#define MSIX_CONTROL_OFFSET (PCI_MSIX_FLAGS + 1)
-#define MSIX_ENABLE_MASK (PCI_MSIX_FLAGS_ENABLE >> 8)
-#define MSIX_MASKALL_MASK (PCI_MSIX_FLAGS_MASKALL >> 8)
+/* MSI enable bit and maskall bit are in byte 1 in control register */
+#define MSIX_CONTROL_OFFSET (PCI_MSIX_CTRL + 1)
+#define MSIX_ENABLE_MASK (PCI_MSIX_ENABLE >> 8)
+#define MSIX_MASKALL_MASK (PCI_MSIX_MASK >> 8)
 
 /* MSI-X table format */
 #define MSIX_MSG_ADDR 0
@@ -58,8 +55,9 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
     uint8_t *config;
     uint32_t new_size;
 
-    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
+    if (nentries < 1 || nentries > PCI_MSIX_TABSIZE + 1) {
         return -EINVAL;
+    }
     if (bar_size > 0x80000000)
         return -ENOSPC;
 
@@ -80,11 +78,11 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
         return config_offset;
     config = pdev->config + config_offset;
 
-    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
+    pci_set_word(config + PCI_MSIX_CTRL, nentries - 1);
     /* Table on top of BAR */
-    pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
+    pci_set_long(config + PCI_MSIX_TABLE, bar_size | bar_nr);
     /* Pending bits on top of that */
-    pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
+    pci_set_long(config + PCI_MSIX_PBA, (bar_size + MSIX_PAGE_PENDING) |
                  bar_nr);
     pdev->msix_cap = config_offset;
     /* Make flags bit writable. */
@@ -208,11 +206,11 @@ void msix_mmio_map(PCIDevice *d, int region_num,
                    pcibus_t addr, pcibus_t size, int type)
 {
     uint8_t *config = d->config + d->msix_cap;
-    uint32_t table = pci_get_long(config + MSIX_TABLE_OFFSET);
+    uint32_t table = pci_get_long(config + PCI_MSIX_TABLE);
     uint32_t offset = table & ~(MSIX_PAGE_SIZE - 1);
     /* TODO: for assigned devices, we'll want to make it possible to map
      * pending bits separately in case they are in a separate bar. */
-    int table_bir = table & PCI_MSIX_FLAGS_BIRMASK;
+    int table_bir = table & PCI_MSIX_BIR;
 
     if (table_bir != region_num)
         return;
diff --git a/hw/pci_regs.h b/hw/pci_regs.h
index 5a5ab89..c17c22f 100644
--- a/hw/pci_regs.h
+++ b/hw/pci_regs.h
@@ -300,12 +300,14 @@
 #define PCI_MSI_DATA_64		12	/* 16 bits of data for 64-bit devices */
 #define PCI_MSI_MASK_64		16	/* Mask bits register for 64-bit devices */
 
-/* MSI-X registers (these are at offset PCI_MSIX_FLAGS) */
-#define PCI_MSIX_FLAGS		2
-#define  PCI_MSIX_FLAGS_QSIZE	0x7FF
-#define  PCI_MSIX_FLAGS_ENABLE	(1 << 15)
-#define  PCI_MSIX_FLAGS_MASKALL	(1 << 14)
-#define PCI_MSIX_FLAGS_BIRMASK	(7 << 0)
+/* MSI-X registers */
+#define PCI_MSIX_CTRL           2       /* Message control */
+#define  PCI_MSIX_TABSIZE       0x7FF   /* Table size - 1 */
+#define  PCI_MSIX_MASK          0x4000  /* Mask all vectors */
+#define  PCI_MSIX_ENABLE        0x8000  /* Enable MSI-X */
+#define PCI_MSIX_TABLE          4       /* MSI-X table */
+#define PCI_MSIX_PBA            8       /* Pending bit array */
+#define  PCI_MSIX_BIR           0x7     /* BAR indication register */
 
 /* CompactPCI Hotswap Register */
 
-- 
1.7.1

[Qemu-devel] [PATCH v2 9/9] msi: Move PCI_MSI_PENDING_32/64 into pci_regs.h

[Jan Kiszka]

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

diff --git a/hw/msi.c b/hw/msi.c
index e23f5df..d548939 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -21,10 +21,6 @@
 #include "msi.h"
 #include "range.h"
 
-/* Eventually those constants should go to Linux pci_regs.h */
-#define PCI_MSI_PENDING_32      0x10
-#define PCI_MSI_PENDING_64      0x14
-
 /* PCI_MSI_ADDRESS_LO */
 #define PCI_MSI_ADDRESS_LO_MASK         (~0x3)
 
diff --git a/hw/pci_regs.h b/hw/pci_regs.h
index c17c22f..002ed2e 100644
--- a/hw/pci_regs.h
+++ b/hw/pci_regs.h
@@ -297,8 +297,10 @@
 #define PCI_MSI_ADDRESS_HI	8	/* Upper 32 bits (if PCI_MSI_FLAGS_64BIT set) */
 #define PCI_MSI_DATA_32		8	/* 16 bits of data for 32-bit devices */
 #define PCI_MSI_MASK_32		12	/* Mask bits register for 32-bit devices */
+#define PCI_MSI_PENDING_32      16      /* Pending bits register for 32-bit devices */
 #define PCI_MSI_DATA_64		12	/* 16 bits of data for 64-bit devices */
 #define PCI_MSI_MASK_64		16	/* Mask bits register for 64-bit devices */
+#define PCI_MSI_PENDING_64      20      /* Pending bits register for 64-bit devices */
 
 /* MSI-X registers */
 #define PCI_MSIX_CTRL           2       /* Message control */
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2 8/9] msix: Align MSI-X constants to libpci definitions and extend them

[Michael S. Tsirkin]

On Wed, Jun 08, 2011 at 06:21:51PM +0200, Jan Kiszka wrote:
> Add PCI_MSIX_TABLE and PCI_MSIX_PBA, align other MSIX related constant
> names to libpci style. Will be used for device assignment code in
> qemu-kvm.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/msix.c     |   24 +++++++++++-------------
>  hw/pci_regs.h |   14 ++++++++------
>  2 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 600f5fb..b20cf7c 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -16,15 +16,12 @@
>  #include "pci.h"
>  #include "range.h"
>  
> -/* MSI-X capability structure */
> -#define MSIX_TABLE_OFFSET 4
> -#define MSIX_PBA_OFFSET 8
>  #define MSIX_CAP_LENGTH 12
>  
> -/* MSI enable bit and maskall bit are in byte 1 in FLAGS register */
> -#define MSIX_CONTROL_OFFSET (PCI_MSIX_FLAGS + 1)
> -#define MSIX_ENABLE_MASK (PCI_MSIX_FLAGS_ENABLE >> 8)
> -#define MSIX_MASKALL_MASK (PCI_MSIX_FLAGS_MASKALL >> 8)
> +/* MSI enable bit and maskall bit are in byte 1 in control register */
> +#define MSIX_CONTROL_OFFSET (PCI_MSIX_CTRL + 1)
> +#define MSIX_ENABLE_MASK (PCI_MSIX_ENABLE >> 8)
> +#define MSIX_MASKALL_MASK (PCI_MSIX_MASK >> 8)
>  
>  /* MSI-X table format */
>  #define MSIX_MSG_ADDR 0
> @@ -58,8 +55,9 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
>      uint8_t *config;
>      uint32_t new_size;
>  
> -    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> +    if (nentries < 1 || nentries > PCI_MSIX_TABSIZE + 1) {
>          return -EINVAL;
> +    }
>      if (bar_size > 0x80000000)
>          return -ENOSPC;
>  
> @@ -80,11 +78,11 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
>          return config_offset;
>      config = pdev->config + config_offset;
>  
> -    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> +    pci_set_word(config + PCI_MSIX_CTRL, nentries - 1);
>      /* Table on top of BAR */
> -    pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> +    pci_set_long(config + PCI_MSIX_TABLE, bar_size | bar_nr);
>      /* Pending bits on top of that */
> -    pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> +    pci_set_long(config + PCI_MSIX_PBA, (bar_size + MSIX_PAGE_PENDING) |
>                   bar_nr);
>      pdev->msix_cap = config_offset;
>      /* Make flags bit writable. */
> @@ -208,11 +206,11 @@ void msix_mmio_map(PCIDevice *d, int region_num,
>                     pcibus_t addr, pcibus_t size, int type)
>  {
>      uint8_t *config = d->config + d->msix_cap;
> -    uint32_t table = pci_get_long(config + MSIX_TABLE_OFFSET);
> +    uint32_t table = pci_get_long(config + PCI_MSIX_TABLE);
>      uint32_t offset = table & ~(MSIX_PAGE_SIZE - 1);
>      /* TODO: for assigned devices, we'll want to make it possible to map
>       * pending bits separately in case they are in a separate bar. */
> -    int table_bir = table & PCI_MSIX_FLAGS_BIRMASK;
> +    int table_bir = table & PCI_MSIX_BIR;
>  
>      if (table_bir != region_num)
>          return;
> diff --git a/hw/pci_regs.h b/hw/pci_regs.h
> index 5a5ab89..c17c22f 100644
> --- a/hw/pci_regs.h
> +++ b/hw/pci_regs.h
> @@ -300,12 +300,14 @@
>  #define PCI_MSI_DATA_64		12	/* 16 bits of data for 64-bit devices */
>  #define PCI_MSI_MASK_64		16	/* Mask bits register for 64-bit devices */
>  
> -/* MSI-X registers (these are at offset PCI_MSIX_FLAGS) */
> -#define PCI_MSIX_FLAGS		2
> -#define  PCI_MSIX_FLAGS_QSIZE	0x7FF
> -#define  PCI_MSIX_FLAGS_ENABLE	(1 << 15)
> -#define  PCI_MSIX_FLAGS_MASKALL	(1 << 14)
> -#define PCI_MSIX_FLAGS_BIRMASK	(7 << 0)
> +/* MSI-X registers */
> +#define PCI_MSIX_CTRL           2       /* Message control */
> +#define  PCI_MSIX_TABSIZE       0x7FF   /* Table size - 1 */
> +#define  PCI_MSIX_MASK          0x4000  /* Mask all vectors */
> +#define  PCI_MSIX_ENABLE        0x8000  /* Enable MSI-X */
> +#define PCI_MSIX_TABLE          4       /* MSI-X table */
> +#define PCI_MSIX_PBA            8       /* Pending bit array */
> +#define  PCI_MSIX_BIR           0x7     /* BAR indication register */
>  
>  /* CompactPCI Hotswap Register */

We are using pci_regs.h from Linux, not libpci, as the base.
What I see there is:

#define PCI_MSIX_FLAGS          2
#define  PCI_MSIX_FLAGS_QSIZE   0x7FF
#define  PCI_MSIX_FLAGS_ENABLE  (1 << 15)
#define  PCI_MSIX_FLAGS_MASKALL (1 << 14)
#define PCI_MSIX_TABLE          4
#define PCI_MSIX_PBA            8
#define  PCI_MSIX_FLAGS_BIRMASK (7 << 0)
#define PCI_MSIX_ENTRY_SIZE             16
#define  PCI_MSIX_ENTRY_LOWER_ADDR      0
#define  PCI_MSIX_ENTRY_UPPER_ADDR      4
#define  PCI_MSIX_ENTRY_DATA            8
#define  PCI_MSIX_ENTRY_VECTOR_CTRL     12
#define   PCI_MSIX_ENTRY_CTRL_MASKBIT   1


Let's stick to that please.

>  
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH v2 9/9] msi: Move PCI_MSI_PENDING_32/64 into pci_regs.h

[Michael S. Tsirkin]

On Wed, Jun 08, 2011 at 06:21:52PM +0200, Jan Kiszka wrote:
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

pci_regs.h from linux does not have these
this is why we keep them in msi.c

[mst@tuck linux-2.6]$ grep PCI_MSI_ include/linux/pci_regs.h 
#define PCI_MSI_FLAGS           2       /* Various flags */
#define  PCI_MSI_FLAGS_64BIT    0x80    /* 64-bit addresses allowed */
#define  PCI_MSI_FLAGS_QSIZE    0x70    /* Message queue size configured
*/
#define  PCI_MSI_FLAGS_QMASK    0x0e    /* Maximum queue size available
*/
#define  PCI_MSI_FLAGS_ENABLE   0x01    /* MSI feature enabled */
#define  PCI_MSI_FLAGS_MASKBIT  0x100   /* 64-bit mask bits allowed */
#define PCI_MSI_RFU             3       /* Rest of capability flags */
#define PCI_MSI_ADDRESS_LO      4       /* Lower 32 bits */
#define PCI_MSI_ADDRESS_HI      8       /* Upper 32 bits (if
PCI_MSI_FLAGS_64BIT set) */
#define PCI_MSI_DATA_32         8       /* 16 bits of data for 32-bit
devices */
#define PCI_MSI_MASK_32         12      /* Mask bits register for 32-bit
devices */
#define PCI_MSI_DATA_64         12      /* 16 bits of data for 64-bit
devices */
#define PCI_MSI_MASK_64         16      /* Mask bits register for 64-bit
devices */


If you want to move them, please send them upstream we'll merge when
they are there.

> ---
>  hw/msi.c      |    4 ----
>  hw/pci_regs.h |    2 ++
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/msi.c b/hw/msi.c
> index e23f5df..d548939 100644
> --- a/hw/msi.c
> +++ b/hw/msi.c
> @@ -21,10 +21,6 @@
>  #include "msi.h"
>  #include "range.h"
>  
> -/* Eventually those constants should go to Linux pci_regs.h */
> -#define PCI_MSI_PENDING_32      0x10
> -#define PCI_MSI_PENDING_64      0x14
> -
>  /* PCI_MSI_ADDRESS_LO */
>  #define PCI_MSI_ADDRESS_LO_MASK         (~0x3)
>  
> diff --git a/hw/pci_regs.h b/hw/pci_regs.h
> index c17c22f..002ed2e 100644
> --- a/hw/pci_regs.h
> +++ b/hw/pci_regs.h
> @@ -297,8 +297,10 @@
>  #define PCI_MSI_ADDRESS_HI	8	/* Upper 32 bits (if PCI_MSI_FLAGS_64BIT set) */
>  #define PCI_MSI_DATA_32		8	/* 16 bits of data for 32-bit devices */
>  #define PCI_MSI_MASK_32		12	/* Mask bits register for 32-bit devices */
> +#define PCI_MSI_PENDING_32      16      /* Pending bits register for 32-bit devices */
>  #define PCI_MSI_DATA_64		12	/* 16 bits of data for 64-bit devices */
>  #define PCI_MSI_MASK_64		16	/* Mask bits register for 64-bit devices */
> +#define PCI_MSI_PENDING_64      20      /* Pending bits register for 64-bit devices */
>  
>  /* MSI-X registers */
>  #define PCI_MSIX_CTRL           2       /* Message control */
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH v2 8/9] msix: Align MSI-X constants to libpci definitions and extend them

[Michael S. Tsirkin]

On Wed, Jun 08, 2011 at 06:21:51PM +0200, Jan Kiszka wrote:
> Add PCI_MSIX_TABLE and PCI_MSIX_PBA, align other MSIX related constant
> names to libpci style. Will be used for device assignment code in
> qemu-kvm.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Besides keeping pci_regs.h aligned with the original,
I also think ideally pci register banging should stay
within the pci subsystem.

Could we add high-level APIs to help with that,
instead of having kvm look at config space directly?


> ---
>  hw/msix.c     |   24 +++++++++++-------------
>  hw/pci_regs.h |   14 ++++++++------
>  2 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 600f5fb..b20cf7c 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -16,15 +16,12 @@
>  #include "pci.h"
>  #include "range.h"
>  
> -/* MSI-X capability structure */
> -#define MSIX_TABLE_OFFSET 4
> -#define MSIX_PBA_OFFSET 8
>  #define MSIX_CAP_LENGTH 12
>  
> -/* MSI enable bit and maskall bit are in byte 1 in FLAGS register */
> -#define MSIX_CONTROL_OFFSET (PCI_MSIX_FLAGS + 1)
> -#define MSIX_ENABLE_MASK (PCI_MSIX_FLAGS_ENABLE >> 8)
> -#define MSIX_MASKALL_MASK (PCI_MSIX_FLAGS_MASKALL >> 8)
> +/* MSI enable bit and maskall bit are in byte 1 in control register */
> +#define MSIX_CONTROL_OFFSET (PCI_MSIX_CTRL + 1)
> +#define MSIX_ENABLE_MASK (PCI_MSIX_ENABLE >> 8)
> +#define MSIX_MASKALL_MASK (PCI_MSIX_MASK >> 8)
>  
>  /* MSI-X table format */
>  #define MSIX_MSG_ADDR 0
> @@ -58,8 +55,9 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
>      uint8_t *config;
>      uint32_t new_size;
>  
> -    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> +    if (nentries < 1 || nentries > PCI_MSIX_TABSIZE + 1) {
>          return -EINVAL;
> +    }
>      if (bar_size > 0x80000000)
>          return -ENOSPC;
>  
> @@ -80,11 +78,11 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
>          return config_offset;
>      config = pdev->config + config_offset;
>  
> -    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> +    pci_set_word(config + PCI_MSIX_CTRL, nentries - 1);
>      /* Table on top of BAR */
> -    pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> +    pci_set_long(config + PCI_MSIX_TABLE, bar_size | bar_nr);
>      /* Pending bits on top of that */
> -    pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> +    pci_set_long(config + PCI_MSIX_PBA, (bar_size + MSIX_PAGE_PENDING) |
>                   bar_nr);
>      pdev->msix_cap = config_offset;
>      /* Make flags bit writable. */
> @@ -208,11 +206,11 @@ void msix_mmio_map(PCIDevice *d, int region_num,
>                     pcibus_t addr, pcibus_t size, int type)
>  {
>      uint8_t *config = d->config + d->msix_cap;
> -    uint32_t table = pci_get_long(config + MSIX_TABLE_OFFSET);
> +    uint32_t table = pci_get_long(config + PCI_MSIX_TABLE);
>      uint32_t offset = table & ~(MSIX_PAGE_SIZE - 1);
>      /* TODO: for assigned devices, we'll want to make it possible to map
>       * pending bits separately in case they are in a separate bar. */
> -    int table_bir = table & PCI_MSIX_FLAGS_BIRMASK;
> +    int table_bir = table & PCI_MSIX_BIR;
>  
>      if (table_bir != region_num)
>          return;
> diff --git a/hw/pci_regs.h b/hw/pci_regs.h
> index 5a5ab89..c17c22f 100644
> --- a/hw/pci_regs.h
> +++ b/hw/pci_regs.h
> @@ -300,12 +300,14 @@
>  #define PCI_MSI_DATA_64		12	/* 16 bits of data for 64-bit devices */
>  #define PCI_MSI_MASK_64		16	/* Mask bits register for 64-bit devices */
>  
> -/* MSI-X registers (these are at offset PCI_MSIX_FLAGS) */
> -#define PCI_MSIX_FLAGS		2
> -#define  PCI_MSIX_FLAGS_QSIZE	0x7FF
> -#define  PCI_MSIX_FLAGS_ENABLE	(1 << 15)
> -#define  PCI_MSIX_FLAGS_MASKALL	(1 << 14)
> -#define PCI_MSIX_FLAGS_BIRMASK	(7 << 0)
> +/* MSI-X registers */
> +#define PCI_MSIX_CTRL           2       /* Message control */
> +#define  PCI_MSIX_TABSIZE       0x7FF   /* Table size - 1 */
> +#define  PCI_MSIX_MASK          0x4000  /* Mask all vectors */
> +#define  PCI_MSIX_ENABLE        0x8000  /* Enable MSI-X */
> +#define PCI_MSIX_TABLE          4       /* MSI-X table */
> +#define PCI_MSIX_PBA            8       /* Pending bit array */
> +#define  PCI_MSIX_BIR           0x7     /* BAR indication register */
>  
>  /* CompactPCI Hotswap Register */
>  
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH v2 5/9] msi: Invoke msi/msix_reset from PCI core

[Michael S. Tsirkin]

On Wed, Jun 08, 2011 at 06:21:48PM +0200, Jan Kiszka wrote:
> 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 95adf09..73bc55f 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 1d297d6..967f812 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -31,6 +31,8 @@
>  #include "loader.h"
>  #include "qemu-objects.h"
>  #include "range.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);
>  }
>  
>  /*

I am reluctant to add this dependency in pci.c:
this goes against the efforts to modularise the
pci subsystem. As I see it, pci.c should really only handle the common
header.  Instead I would like to add pci_caps.c and have
that call out to all the capabilities (not just reset, same
applies to other caps really).

I started doing that, if no one beats me to it,
I'll try to dust off that patch and post it next week.

> diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> index 464d897..3951252 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
> @@ -224,6 +226,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 c19629d..075d657 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -310,7 +310,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 5aa6a6b..a587c3e 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 a7640f5..9d75449 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.1

Re: [Qemu-devel] [PATCH v2 9/9] msi: Move PCI_MSI_PENDING_32/64 into pci_regs.h

[Jan Kiszka]

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

On 2011-06-08 21:48, Michael S. Tsirkin wrote:
> On Wed, Jun 08, 2011 at 06:21:52PM +0200, Jan Kiszka wrote:
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> pci_regs.h from linux does not have these
> this is why we keep them in msi.c
> 
> [mst@tuck linux-2.6]$ grep PCI_MSI_ include/linux/pci_regs.h 
> #define PCI_MSI_FLAGS           2       /* Various flags */
> #define  PCI_MSI_FLAGS_64BIT    0x80    /* 64-bit addresses allowed */
> #define  PCI_MSI_FLAGS_QSIZE    0x70    /* Message queue size configured
> */
> #define  PCI_MSI_FLAGS_QMASK    0x0e    /* Maximum queue size available
> */
> #define  PCI_MSI_FLAGS_ENABLE   0x01    /* MSI feature enabled */
> #define  PCI_MSI_FLAGS_MASKBIT  0x100   /* 64-bit mask bits allowed */
> #define PCI_MSI_RFU             3       /* Rest of capability flags */
> #define PCI_MSI_ADDRESS_LO      4       /* Lower 32 bits */
> #define PCI_MSI_ADDRESS_HI      8       /* Upper 32 bits (if
> PCI_MSI_FLAGS_64BIT set) */
> #define PCI_MSI_DATA_32         8       /* 16 bits of data for 32-bit
> devices */
> #define PCI_MSI_MASK_32         12      /* Mask bits register for 32-bit
> devices */
> #define PCI_MSI_DATA_64         12      /* 16 bits of data for 64-bit
> devices */
> #define PCI_MSI_MASK_64         16      /* Mask bits register for 64-bit
> devices */
> 
> 
> If you want to move them, please send them upstream we'll merge when
> they are there.

In fact, both defines are already in libpci. Since 3.0.0. Released 5
years ago. OK, I'll send a header resync patch against a more recent
release.

Then we should just lack something like PCI_MSIX_CTRL. I will have a look.

Jan


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

Re: [Qemu-devel] [PATCH v2 5/9] msi: Invoke msi/msix_reset from PCI core

[Jan Kiszka]

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

On 2011-06-08 21:59, Michael S. Tsirkin wrote:
> On Wed, Jun 08, 2011 at 06:21:48PM +0200, Jan Kiszka wrote:
>> 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 95adf09..73bc55f 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 1d297d6..967f812 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -31,6 +31,8 @@
>>  #include "loader.h"
>>  #include "qemu-objects.h"
>>  #include "range.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);
>>  }
>>  
>>  /*
> 
> I am reluctant to add this dependency in pci.c:
> this goes against the efforts to modularise the
> pci subsystem. As I see it, pci.c should really only handle the common
> header.  Instead I would like to add pci_caps.c and have
> that call out to all the capabilities (not just reset, same
> applies to other caps really).

Yeah, true. The result should just remain that devices no longer need to
worry about this.

> 
> I started doing that, if no one beats me to it,
> I'll try to dust off that patch and post it next week.

Would be welcome.

Jan


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

Re: [Qemu-devel] [PATCH v2 8/9] msix: Align MSI-X constants to libpci definitions and extend them

[Jan Kiszka]

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

On 2011-06-08 21:53, Michael S. Tsirkin wrote:
> On Wed, Jun 08, 2011 at 06:21:51PM +0200, Jan Kiszka wrote:
>> Add PCI_MSIX_TABLE and PCI_MSIX_PBA, align other MSIX related constant
>> names to libpci style. Will be used for device assignment code in
>> qemu-kvm.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Besides keeping pci_regs.h aligned with the original,
> I also think ideally pci register banging should stay
> within the pci subsystem.
> 
> Could we add high-level APIs to help with that,
> instead of having kvm look at config space directly?

We could move the related static inlines from msi/msix.c to the headers
in order to test for bits etc. Still, kvm needs to interpret the config
space of the assigned device, so the abstraction will remain rather low.

Jan


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

Re: [Qemu-devel] [PATCH v2 9/9] msi: Move PCI_MSI_PENDING_32/64 into pci_regs.h

[Michael S. Tsirkin]

On Wed, Jun 08, 2011 at 10:44:58PM +0200, Jan Kiszka wrote:
> On 2011-06-08 21:48, Michael S. Tsirkin wrote:
> > On Wed, Jun 08, 2011 at 06:21:52PM +0200, Jan Kiszka wrote:
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > pci_regs.h from linux does not have these
> > this is why we keep them in msi.c
> > 
> > [mst@tuck linux-2.6]$ grep PCI_MSI_ include/linux/pci_regs.h 
> > #define PCI_MSI_FLAGS           2       /* Various flags */
> > #define  PCI_MSI_FLAGS_64BIT    0x80    /* 64-bit addresses allowed */
> > #define  PCI_MSI_FLAGS_QSIZE    0x70    /* Message queue size configured
> > */
> > #define  PCI_MSI_FLAGS_QMASK    0x0e    /* Maximum queue size available
> > */
> > #define  PCI_MSI_FLAGS_ENABLE   0x01    /* MSI feature enabled */
> > #define  PCI_MSI_FLAGS_MASKBIT  0x100   /* 64-bit mask bits allowed */
> > #define PCI_MSI_RFU             3       /* Rest of capability flags */
> > #define PCI_MSI_ADDRESS_LO      4       /* Lower 32 bits */
> > #define PCI_MSI_ADDRESS_HI      8       /* Upper 32 bits (if
> > PCI_MSI_FLAGS_64BIT set) */
> > #define PCI_MSI_DATA_32         8       /* 16 bits of data for 32-bit
> > devices */
> > #define PCI_MSI_MASK_32         12      /* Mask bits register for 32-bit
> > devices */
> > #define PCI_MSI_DATA_64         12      /* 16 bits of data for 64-bit
> > devices */
> > #define PCI_MSI_MASK_64         16      /* Mask bits register for 64-bit
> > devices */
> > 
> > 
> > If you want to move them, please send them upstream we'll merge when
> > they are there.
> 
> In fact, both defines are already in libpci. Since 3.0.0. Released 5
> years ago. OK, I'll send a header resync patch against a more recent
> release.
> 
> Then we should just lack something like PCI_MSIX_CTRL. I will have a look.

PCI_MSIX_FLAGS is the same.

> 
> Jan
> 

Re: [Qemu-devel] [PATCH v2 9/9] msi: Move PCI_MSI_PENDING_32/64 into pci_regs.h

[Jan Kiszka]

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

On 2011-06-08 22:56, Michael S. Tsirkin wrote:
> On Wed, Jun 08, 2011 at 10:44:58PM +0200, Jan Kiszka wrote:
>> On 2011-06-08 21:48, Michael S. Tsirkin wrote:
>>> On Wed, Jun 08, 2011 at 06:21:52PM +0200, Jan Kiszka wrote:
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> pci_regs.h from linux does not have these
>>> this is why we keep them in msi.c
>>>
>>> [mst@tuck linux-2.6]$ grep PCI_MSI_ include/linux/pci_regs.h 
>>> #define PCI_MSI_FLAGS           2       /* Various flags */
>>> #define  PCI_MSI_FLAGS_64BIT    0x80    /* 64-bit addresses allowed */
>>> #define  PCI_MSI_FLAGS_QSIZE    0x70    /* Message queue size configured
>>> */
>>> #define  PCI_MSI_FLAGS_QMASK    0x0e    /* Maximum queue size available
>>> */
>>> #define  PCI_MSI_FLAGS_ENABLE   0x01    /* MSI feature enabled */
>>> #define  PCI_MSI_FLAGS_MASKBIT  0x100   /* 64-bit mask bits allowed */
>>> #define PCI_MSI_RFU             3       /* Rest of capability flags */
>>> #define PCI_MSI_ADDRESS_LO      4       /* Lower 32 bits */
>>> #define PCI_MSI_ADDRESS_HI      8       /* Upper 32 bits (if
>>> PCI_MSI_FLAGS_64BIT set) */
>>> #define PCI_MSI_DATA_32         8       /* 16 bits of data for 32-bit
>>> devices */
>>> #define PCI_MSI_MASK_32         12      /* Mask bits register for 32-bit
>>> devices */
>>> #define PCI_MSI_DATA_64         12      /* 16 bits of data for 64-bit
>>> devices */
>>> #define PCI_MSI_MASK_64         16      /* Mask bits register for 64-bit
>>> devices */
>>>
>>>
>>> If you want to move them, please send them upstream we'll merge when
>>> they are there.
>>
>> In fact, both defines are already in libpci. Since 3.0.0. Released 5
>> years ago. OK, I'll send a header resync patch against a more recent
>> release.
>>
>> Then we should just lack something like PCI_MSIX_CTRL. I will have a look.
> 
> PCI_MSIX_FLAGS is the same.

That does not exist in libpci's header.

Jan


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

Re: [Qemu-devel] [PATCH v2 8/9] msix: Align MSI-X constants to libpci definitions and extend them

[Michael S. Tsirkin]

On Wed, Jun 08, 2011 at 10:48:10PM +0200, Jan Kiszka wrote:
> On 2011-06-08 21:53, Michael S. Tsirkin wrote:
> > On Wed, Jun 08, 2011 at 06:21:51PM +0200, Jan Kiszka wrote:
> >> Add PCI_MSIX_TABLE and PCI_MSIX_PBA, align other MSIX related constant
> >> names to libpci style. Will be used for device assignment code in
> >> qemu-kvm.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > Besides keeping pci_regs.h aligned with the original,
> > I also think ideally pci register banging should stay
> > within the pci subsystem.
> > 
> > Could we add high-level APIs to help with that,
> > instead of having kvm look at config space directly?
> 
> We could move the related static inlines from msi/msix.c to the headers
> in order to test for bits etc. Still, kvm needs to interpret the config
> space of the assigned device, so the abstraction will remain rather low.
> 
> Jan
> 

Hmm, at least for MSI/MSIX I thought this is done by kvm in kernel?

-- 
MST

Re: [Qemu-devel] [PATCH v2 9/9] msi: Move PCI_MSI_PENDING_32/64 into pci_regs.h

[Michael S. Tsirkin]

On Wed, Jun 08, 2011 at 10:57:13PM +0200, Jan Kiszka wrote:
> On 2011-06-08 22:56, Michael S. Tsirkin wrote:
> > On Wed, Jun 08, 2011 at 10:44:58PM +0200, Jan Kiszka wrote:
> >> On 2011-06-08 21:48, Michael S. Tsirkin wrote:
> >>> On Wed, Jun 08, 2011 at 06:21:52PM +0200, Jan Kiszka wrote:
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>
> >>> pci_regs.h from linux does not have these
> >>> this is why we keep them in msi.c
> >>>
> >>> [mst@tuck linux-2.6]$ grep PCI_MSI_ include/linux/pci_regs.h 
> >>> #define PCI_MSI_FLAGS           2       /* Various flags */
> >>> #define  PCI_MSI_FLAGS_64BIT    0x80    /* 64-bit addresses allowed */
> >>> #define  PCI_MSI_FLAGS_QSIZE    0x70    /* Message queue size configured
> >>> */
> >>> #define  PCI_MSI_FLAGS_QMASK    0x0e    /* Maximum queue size available
> >>> */
> >>> #define  PCI_MSI_FLAGS_ENABLE   0x01    /* MSI feature enabled */
> >>> #define  PCI_MSI_FLAGS_MASKBIT  0x100   /* 64-bit mask bits allowed */
> >>> #define PCI_MSI_RFU             3       /* Rest of capability flags */
> >>> #define PCI_MSI_ADDRESS_LO      4       /* Lower 32 bits */
> >>> #define PCI_MSI_ADDRESS_HI      8       /* Upper 32 bits (if
> >>> PCI_MSI_FLAGS_64BIT set) */
> >>> #define PCI_MSI_DATA_32         8       /* 16 bits of data for 32-bit
> >>> devices */
> >>> #define PCI_MSI_MASK_32         12      /* Mask bits register for 32-bit
> >>> devices */
> >>> #define PCI_MSI_DATA_64         12      /* 16 bits of data for 64-bit
> >>> devices */
> >>> #define PCI_MSI_MASK_64         16      /* Mask bits register for 64-bit
> >>> devices */
> >>>
> >>>
> >>> If you want to move them, please send them upstream we'll merge when
> >>> they are there.
> >>
> >> In fact, both defines are already in libpci. Since 3.0.0. Released 5
> >> years ago. OK, I'll send a header resync patch against a more recent
> >> release.
> >>
> >> Then we should just lack something like PCI_MSIX_CTRL. I will have a look.
> > 
> > PCI_MSIX_FLAGS is the same.
> 
> That does not exist in libpci's header.
> 
> Jan
> 

The upstream I meant is pci_Regs.h in Linux.

-- 
MST

Re: [Qemu-devel] [PATCH v2 8/9] msix: Align MSI-X constants to libpci definitions and extend them

[Jan Kiszka]

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

On 2011-06-08 23:00, Michael S. Tsirkin wrote:
> On Wed, Jun 08, 2011 at 10:48:10PM +0200, Jan Kiszka wrote:
>> On 2011-06-08 21:53, Michael S. Tsirkin wrote:
>>> On Wed, Jun 08, 2011 at 06:21:51PM +0200, Jan Kiszka wrote:
>>>> Add PCI_MSIX_TABLE and PCI_MSIX_PBA, align other MSIX related constant
>>>> names to libpci style. Will be used for device assignment code in
>>>> qemu-kvm.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> Besides keeping pci_regs.h aligned with the original,
>>> I also think ideally pci register banging should stay
>>> within the pci subsystem.
>>>
>>> Could we add high-level APIs to help with that,
>>> instead of having kvm look at config space directly?
>>
>> We could move the related static inlines from msi/msix.c to the headers
>> in order to test for bits etc. Still, kvm needs to interpret the config
>> space of the assigned device, so the abstraction will remain rather low.
>>
>> Jan
>>
> 
> Hmm, at least for MSI/MSIX I thought this is done by kvm in kernel?
> 

At least for the "traditional" assignment interface (VFIO may offload
something), no. User space does the cap analysis, filtering, and in the
MSI/MSI-X case the translation to QEMU msi/msix services. The latter is
even WIP in my tree. Surrent assignment open-codes this, missing many
corner cases.

Jan


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

Re: [Qemu-devel] [PATCH v2 9/9] msi: Move PCI_MSI_PENDING_32/64 into pci_regs.h

[Jan Kiszka]

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

On 2011-06-08 23:01, Michael S. Tsirkin wrote:
> On Wed, Jun 08, 2011 at 10:57:13PM +0200, Jan Kiszka wrote:
>> On 2011-06-08 22:56, Michael S. Tsirkin wrote:
>>> On Wed, Jun 08, 2011 at 10:44:58PM +0200, Jan Kiszka wrote:
>>>> On 2011-06-08 21:48, Michael S. Tsirkin wrote:
>>>>> On Wed, Jun 08, 2011 at 06:21:52PM +0200, Jan Kiszka wrote:
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> pci_regs.h from linux does not have these
>>>>> this is why we keep them in msi.c
>>>>>
>>>>> [mst@tuck linux-2.6]$ grep PCI_MSI_ include/linux/pci_regs.h 
>>>>> #define PCI_MSI_FLAGS           2       /* Various flags */
>>>>> #define  PCI_MSI_FLAGS_64BIT    0x80    /* 64-bit addresses allowed */
>>>>> #define  PCI_MSI_FLAGS_QSIZE    0x70    /* Message queue size configured
>>>>> */
>>>>> #define  PCI_MSI_FLAGS_QMASK    0x0e    /* Maximum queue size available
>>>>> */
>>>>> #define  PCI_MSI_FLAGS_ENABLE   0x01    /* MSI feature enabled */
>>>>> #define  PCI_MSI_FLAGS_MASKBIT  0x100   /* 64-bit mask bits allowed */
>>>>> #define PCI_MSI_RFU             3       /* Rest of capability flags */
>>>>> #define PCI_MSI_ADDRESS_LO      4       /* Lower 32 bits */
>>>>> #define PCI_MSI_ADDRESS_HI      8       /* Upper 32 bits (if
>>>>> PCI_MSI_FLAGS_64BIT set) */
>>>>> #define PCI_MSI_DATA_32         8       /* 16 bits of data for 32-bit
>>>>> devices */
>>>>> #define PCI_MSI_MASK_32         12      /* Mask bits register for 32-bit
>>>>> devices */
>>>>> #define PCI_MSI_DATA_64         12      /* 16 bits of data for 64-bit
>>>>> devices */
>>>>> #define PCI_MSI_MASK_64         16      /* Mask bits register for 64-bit
>>>>> devices */
>>>>>
>>>>>
>>>>> If you want to move them, please send them upstream we'll merge when
>>>>> they are there.
>>>>
>>>> In fact, both defines are already in libpci. Since 3.0.0. Released 5
>>>> years ago. OK, I'll send a header resync patch against a more recent
>>>> release.
>>>>
>>>> Then we should just lack something like PCI_MSIX_CTRL. I will have a look.
>>>
>>> PCI_MSIX_FLAGS is the same.
>>
>> That does not exist in libpci's header.
>>
>> Jan
>>
> 
> The upstream I meant is pci_Regs.h in Linux.
> 

Why not sync against
git://git.kernel.org/pub/scm/utils/pciutils/pciutils.git? Sounds more
appropriate.

Jan


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

Re: [Qemu-devel] [PATCH v2 8/9] msix: Align MSI-X constants to libpci definitions and extend them

[Michael S. Tsirkin]

On Wed, Jun 08, 2011 at 11:02:44PM +0200, Jan Kiszka wrote:
> On 2011-06-08 23:00, Michael S. Tsirkin wrote:
> > On Wed, Jun 08, 2011 at 10:48:10PM +0200, Jan Kiszka wrote:
> >> On 2011-06-08 21:53, Michael S. Tsirkin wrote:
> >>> On Wed, Jun 08, 2011 at 06:21:51PM +0200, Jan Kiszka wrote:
> >>>> Add PCI_MSIX_TABLE and PCI_MSIX_PBA, align other MSIX related constant
> >>>> names to libpci style. Will be used for device assignment code in
> >>>> qemu-kvm.
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>
> >>> Besides keeping pci_regs.h aligned with the original,
> >>> I also think ideally pci register banging should stay
> >>> within the pci subsystem.
> >>>
> >>> Could we add high-level APIs to help with that,
> >>> instead of having kvm look at config space directly?
> >>
> >> We could move the related static inlines from msi/msix.c to the headers
> >> in order to test for bits etc. Still, kvm needs to interpret the config
> >> space of the assigned device, so the abstraction will remain rather low.
> >>
> >> Jan
> >>
> > 
> > Hmm, at least for MSI/MSIX I thought this is done by kvm in kernel?
> > 
> 
> At least for the "traditional" assignment interface (VFIO may offload
> something), no. User space does the cap analysis, filtering, and in the
> MSI/MSI-X case the translation to QEMU msi/msix services. The latter is
> even WIP in my tree. Surrent assignment open-codes this, missing many
> corner cases.
> 
> Jan
> 

Anyway, if some defines need to be in a header, and aren't upstream
yet, let's create pci_ext_regs.h and add a comment there that we
should work on upstreaming them.


-- 
MST

Re: [Qemu-devel] [PATCH v2 8/9] msix: Align MSI-X constants to libpci definitions and extend them

[Jan Kiszka]

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

On 2011-06-08 23:09, Michael S. Tsirkin wrote:
> On Wed, Jun 08, 2011 at 11:02:44PM +0200, Jan Kiszka wrote:
>> On 2011-06-08 23:00, Michael S. Tsirkin wrote:
>>> On Wed, Jun 08, 2011 at 10:48:10PM +0200, Jan Kiszka wrote:
>>>> On 2011-06-08 21:53, Michael S. Tsirkin wrote:
>>>>> On Wed, Jun 08, 2011 at 06:21:51PM +0200, Jan Kiszka wrote:
>>>>>> Add PCI_MSIX_TABLE and PCI_MSIX_PBA, align other MSIX related constant
>>>>>> names to libpci style. Will be used for device assignment code in
>>>>>> qemu-kvm.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> Besides keeping pci_regs.h aligned with the original,
>>>>> I also think ideally pci register banging should stay
>>>>> within the pci subsystem.
>>>>>
>>>>> Could we add high-level APIs to help with that,
>>>>> instead of having kvm look at config space directly?
>>>>
>>>> We could move the related static inlines from msi/msix.c to the headers
>>>> in order to test for bits etc. Still, kvm needs to interpret the config
>>>> space of the assigned device, so the abstraction will remain rather low.
>>>>
>>>> Jan
>>>>
>>>
>>> Hmm, at least for MSI/MSIX I thought this is done by kvm in kernel?
>>>
>>
>> At least for the "traditional" assignment interface (VFIO may offload
>> something), no. User space does the cap analysis, filtering, and in the
>> MSI/MSI-X case the translation to QEMU msi/msix services. The latter is
>> even WIP in my tree. Surrent assignment open-codes this, missing many
>> corner cases.
>>
>> Jan
>>
> 
> Anyway, if some defines need to be in a header, and aren't upstream
> yet, let's create pci_ext_regs.h and add a comment there that we
> should work on upstreaming them.

Sounds good. But what is supposed to be upstream for us, the kernel or
pci-utils/libpci?

Jan


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

Re: [Qemu-devel] [PATCH v2 9/9] msi: Move PCI_MSI_PENDING_32/64 into pci_regs.h

[Michael S. Tsirkin]

On Wed, Jun 08, 2011 at 11:03:43PM +0200, Jan Kiszka wrote:
> On 2011-06-08 23:01, Michael S. Tsirkin wrote:
> > On Wed, Jun 08, 2011 at 10:57:13PM +0200, Jan Kiszka wrote:
> >> On 2011-06-08 22:56, Michael S. Tsirkin wrote:
> >>> On Wed, Jun 08, 2011 at 10:44:58PM +0200, Jan Kiszka wrote:
> >>>> On 2011-06-08 21:48, Michael S. Tsirkin wrote:
> >>>>> On Wed, Jun 08, 2011 at 06:21:52PM +0200, Jan Kiszka wrote:
> >>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>
> >>>>> pci_regs.h from linux does not have these
> >>>>> this is why we keep them in msi.c
> >>>>>
> >>>>> [mst@tuck linux-2.6]$ grep PCI_MSI_ include/linux/pci_regs.h 
> >>>>> #define PCI_MSI_FLAGS           2       /* Various flags */
> >>>>> #define  PCI_MSI_FLAGS_64BIT    0x80    /* 64-bit addresses allowed */
> >>>>> #define  PCI_MSI_FLAGS_QSIZE    0x70    /* Message queue size configured
> >>>>> */
> >>>>> #define  PCI_MSI_FLAGS_QMASK    0x0e    /* Maximum queue size available
> >>>>> */
> >>>>> #define  PCI_MSI_FLAGS_ENABLE   0x01    /* MSI feature enabled */
> >>>>> #define  PCI_MSI_FLAGS_MASKBIT  0x100   /* 64-bit mask bits allowed */
> >>>>> #define PCI_MSI_RFU             3       /* Rest of capability flags */
> >>>>> #define PCI_MSI_ADDRESS_LO      4       /* Lower 32 bits */
> >>>>> #define PCI_MSI_ADDRESS_HI      8       /* Upper 32 bits (if
> >>>>> PCI_MSI_FLAGS_64BIT set) */
> >>>>> #define PCI_MSI_DATA_32         8       /* 16 bits of data for 32-bit
> >>>>> devices */
> >>>>> #define PCI_MSI_MASK_32         12      /* Mask bits register for 32-bit
> >>>>> devices */
> >>>>> #define PCI_MSI_DATA_64         12      /* 16 bits of data for 64-bit
> >>>>> devices */
> >>>>> #define PCI_MSI_MASK_64         16      /* Mask bits register for 64-bit
> >>>>> devices */
> >>>>>
> >>>>>
> >>>>> If you want to move them, please send them upstream we'll merge when
> >>>>> they are there.
> >>>>
> >>>> In fact, both defines are already in libpci. Since 3.0.0. Released 5
> >>>> years ago. OK, I'll send a header resync patch against a more recent
> >>>> release.
> >>>>
> >>>> Then we should just lack something like PCI_MSIX_CTRL. I will have a look.
> >>>
> >>> PCI_MSIX_FLAGS is the same.
> >>
> >> That does not exist in libpci's header.
> >>
> >> Jan
> >>
> > 
> > The upstream I meant is pci_Regs.h in Linux.
> > 
> 
> Why not sync against
> git://git.kernel.org/pub/scm/utils/pciutils/pciutils.git? Sounds more
> appropriate.
> 
> Jan
> 

We are 100% aligned with Linux changing that seems pointless.
I think Linux gets new hardware features faster and has a better
chance to be correct.
We already pull headers from Linux so that's one dependency less.

-- 
MST

Re: [Qemu-devel] [PATCH v2 8/9] msix: Align MSI-X constants to libpci definitions and extend them

[Michael S. Tsirkin]

On Wed, Jun 08, 2011 at 11:11:37PM +0200, Jan Kiszka wrote:
> On 2011-06-08 23:09, Michael S. Tsirkin wrote:
> > On Wed, Jun 08, 2011 at 11:02:44PM +0200, Jan Kiszka wrote:
> >> On 2011-06-08 23:00, Michael S. Tsirkin wrote:
> >>> On Wed, Jun 08, 2011 at 10:48:10PM +0200, Jan Kiszka wrote:
> >>>> On 2011-06-08 21:53, Michael S. Tsirkin wrote:
> >>>>> On Wed, Jun 08, 2011 at 06:21:51PM +0200, Jan Kiszka wrote:
> >>>>>> Add PCI_MSIX_TABLE and PCI_MSIX_PBA, align other MSIX related constant
> >>>>>> names to libpci style. Will be used for device assignment code in
> >>>>>> qemu-kvm.
> >>>>>>
> >>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>
> >>>>> Besides keeping pci_regs.h aligned with the original,
> >>>>> I also think ideally pci register banging should stay
> >>>>> within the pci subsystem.
> >>>>>
> >>>>> Could we add high-level APIs to help with that,
> >>>>> instead of having kvm look at config space directly?
> >>>>
> >>>> We could move the related static inlines from msi/msix.c to the headers
> >>>> in order to test for bits etc. Still, kvm needs to interpret the config
> >>>> space of the assigned device, so the abstraction will remain rather low.
> >>>>
> >>>> Jan
> >>>>
> >>>
> >>> Hmm, at least for MSI/MSIX I thought this is done by kvm in kernel?
> >>>
> >>
> >> At least for the "traditional" assignment interface (VFIO may offload
> >> something), no. User space does the cap analysis, filtering, and in the
> >> MSI/MSI-X case the translation to QEMU msi/msix services. The latter is
> >> even WIP in my tree. Surrent assignment open-codes this, missing many
> >> corner cases.
> >>
> >> Jan
> >>
> > 
> > Anyway, if some defines need to be in a header, and aren't upstream
> > yet, let's create pci_ext_regs.h and add a comment there that we
> > should work on upstreaming them.
> 
> Sounds good. But what is supposed to be upstream for us, the kernel or
> pci-utils/libpci?
> 
> Jan
> 

It's currently the kernel, I don't really see a reason to change that.

-- 
MST

Re: [Qemu-devel] [PATCH v2 9/9] msi: Move PCI_MSI_PENDING_32/64 into pci_regs.h

[Jan Kiszka]

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

On 2011-06-08 23:14, Michael S. Tsirkin wrote:
> On Wed, Jun 08, 2011 at 11:03:43PM +0200, Jan Kiszka wrote:
>> On 2011-06-08 23:01, Michael S. Tsirkin wrote:
>>> On Wed, Jun 08, 2011 at 10:57:13PM +0200, Jan Kiszka wrote:
>>>> On 2011-06-08 22:56, Michael S. Tsirkin wrote:
>>>>> On Wed, Jun 08, 2011 at 10:44:58PM +0200, Jan Kiszka wrote:
>>>>>> On 2011-06-08 21:48, Michael S. Tsirkin wrote:
>>>>>>> On Wed, Jun 08, 2011 at 06:21:52PM +0200, Jan Kiszka wrote:
>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>
>>>>>>> pci_regs.h from linux does not have these
>>>>>>> this is why we keep them in msi.c
>>>>>>>
>>>>>>> [mst@tuck linux-2.6]$ grep PCI_MSI_ include/linux/pci_regs.h 
>>>>>>> #define PCI_MSI_FLAGS           2       /* Various flags */
>>>>>>> #define  PCI_MSI_FLAGS_64BIT    0x80    /* 64-bit addresses allowed */
>>>>>>> #define  PCI_MSI_FLAGS_QSIZE    0x70    /* Message queue size configured
>>>>>>> */
>>>>>>> #define  PCI_MSI_FLAGS_QMASK    0x0e    /* Maximum queue size available
>>>>>>> */
>>>>>>> #define  PCI_MSI_FLAGS_ENABLE   0x01    /* MSI feature enabled */
>>>>>>> #define  PCI_MSI_FLAGS_MASKBIT  0x100   /* 64-bit mask bits allowed */
>>>>>>> #define PCI_MSI_RFU             3       /* Rest of capability flags */
>>>>>>> #define PCI_MSI_ADDRESS_LO      4       /* Lower 32 bits */
>>>>>>> #define PCI_MSI_ADDRESS_HI      8       /* Upper 32 bits (if
>>>>>>> PCI_MSI_FLAGS_64BIT set) */
>>>>>>> #define PCI_MSI_DATA_32         8       /* 16 bits of data for 32-bit
>>>>>>> devices */
>>>>>>> #define PCI_MSI_MASK_32         12      /* Mask bits register for 32-bit
>>>>>>> devices */
>>>>>>> #define PCI_MSI_DATA_64         12      /* 16 bits of data for 64-bit
>>>>>>> devices */
>>>>>>> #define PCI_MSI_MASK_64         16      /* Mask bits register for 64-bit
>>>>>>> devices */
>>>>>>>
>>>>>>>
>>>>>>> If you want to move them, please send them upstream we'll merge when
>>>>>>> they are there.
>>>>>>
>>>>>> In fact, both defines are already in libpci. Since 3.0.0. Released 5
>>>>>> years ago. OK, I'll send a header resync patch against a more recent
>>>>>> release.
>>>>>>
>>>>>> Then we should just lack something like PCI_MSIX_CTRL. I will have a look.
>>>>>
>>>>> PCI_MSIX_FLAGS is the same.
>>>>
>>>> That does not exist in libpci's header.
>>>>
>>>> Jan
>>>>
>>>
>>> The upstream I meant is pci_Regs.h in Linux.
>>>
>>
>> Why not sync against
>> git://git.kernel.org/pub/scm/utils/pciutils/pciutils.git? Sounds more
>> appropriate.
>>
>> Jan
>>
> 
> We are 100% aligned with Linux changing that seems pointless.
> I think Linux gets new hardware features faster and has a better
> chance to be correct.
> We already pull headers from Linux so that's one dependency less.
> 

The kernel appears to gain defines based on what it uses. The libpci
headers seem to serve more use cases, at least it had the missing fields
for several years and is a few hundred lines longer.

Jan


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

Re: [Qemu-devel] [PATCH v2 9/9] msi: Move PCI_MSI_PENDING_32/64 into pci_regs.h

[Jan Kiszka]

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

On 2011-06-08 23:18, Jan Kiszka wrote:
> On 2011-06-08 23:14, Michael S. Tsirkin wrote:
>> On Wed, Jun 08, 2011 at 11:03:43PM +0200, Jan Kiszka wrote:
>>> On 2011-06-08 23:01, Michael S. Tsirkin wrote:
>>>> On Wed, Jun 08, 2011 at 10:57:13PM +0200, Jan Kiszka wrote:
>>>>> On 2011-06-08 22:56, Michael S. Tsirkin wrote:
>>>>>> On Wed, Jun 08, 2011 at 10:44:58PM +0200, Jan Kiszka wrote:
>>>>>>> On 2011-06-08 21:48, Michael S. Tsirkin wrote:
>>>>>>>> On Wed, Jun 08, 2011 at 06:21:52PM +0200, Jan Kiszka wrote:
>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>
>>>>>>>> pci_regs.h from linux does not have these
>>>>>>>> this is why we keep them in msi.c
>>>>>>>>
>>>>>>>> [mst@tuck linux-2.6]$ grep PCI_MSI_ include/linux/pci_regs.h 
>>>>>>>> #define PCI_MSI_FLAGS           2       /* Various flags */
>>>>>>>> #define  PCI_MSI_FLAGS_64BIT    0x80    /* 64-bit addresses allowed */
>>>>>>>> #define  PCI_MSI_FLAGS_QSIZE    0x70    /* Message queue size configured
>>>>>>>> */
>>>>>>>> #define  PCI_MSI_FLAGS_QMASK    0x0e    /* Maximum queue size available
>>>>>>>> */
>>>>>>>> #define  PCI_MSI_FLAGS_ENABLE   0x01    /* MSI feature enabled */
>>>>>>>> #define  PCI_MSI_FLAGS_MASKBIT  0x100   /* 64-bit mask bits allowed */
>>>>>>>> #define PCI_MSI_RFU             3       /* Rest of capability flags */
>>>>>>>> #define PCI_MSI_ADDRESS_LO      4       /* Lower 32 bits */
>>>>>>>> #define PCI_MSI_ADDRESS_HI      8       /* Upper 32 bits (if
>>>>>>>> PCI_MSI_FLAGS_64BIT set) */
>>>>>>>> #define PCI_MSI_DATA_32         8       /* 16 bits of data for 32-bit
>>>>>>>> devices */
>>>>>>>> #define PCI_MSI_MASK_32         12      /* Mask bits register for 32-bit
>>>>>>>> devices */
>>>>>>>> #define PCI_MSI_DATA_64         12      /* 16 bits of data for 64-bit
>>>>>>>> devices */
>>>>>>>> #define PCI_MSI_MASK_64         16      /* Mask bits register for 64-bit
>>>>>>>> devices */
>>>>>>>>
>>>>>>>>
>>>>>>>> If you want to move them, please send them upstream we'll merge when
>>>>>>>> they are there.
>>>>>>>
>>>>>>> In fact, both defines are already in libpci. Since 3.0.0. Released 5
>>>>>>> years ago. OK, I'll send a header resync patch against a more recent
>>>>>>> release.
>>>>>>>
>>>>>>> Then we should just lack something like PCI_MSIX_CTRL. I will have a look.
>>>>>>
>>>>>> PCI_MSIX_FLAGS is the same.
>>>>>
>>>>> That does not exist in libpci's header.
>>>>>
>>>>> Jan
>>>>>
>>>>
>>>> The upstream I meant is pci_Regs.h in Linux.
>>>>
>>>
>>> Why not sync against
>>> git://git.kernel.org/pub/scm/utils/pciutils/pciutils.git? Sounds more
>>> appropriate.
>>>
>>> Jan
>>>
>>
>> We are 100% aligned with Linux changing that seems pointless.
>> I think Linux gets new hardware features faster and has a better
>> chance to be correct.
>> We already pull headers from Linux so that's one dependency less.
>>
> 
> The kernel appears to gain defines based on what it uses. The libpci
> headers seem to serve more use cases, at least it had the missing fields
> for several years and is a few hundred lines longer.

OK, looks like that "added value" comes from merging pci_regs.h with
pci_ids.h. Never mind, will work against the kernel.

Jan


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

Re: [Qemu-devel] [PATCH v2 9/9] msi: Move PCI_MSI_PENDING_32/64 into pci_regs.h

[Michael S. Tsirkin]

On Wed, Jun 08, 2011 at 11:18:05PM +0200, Jan Kiszka wrote:
> On 2011-06-08 23:14, Michael S. Tsirkin wrote:
> > On Wed, Jun 08, 2011 at 11:03:43PM +0200, Jan Kiszka wrote:
> >> On 2011-06-08 23:01, Michael S. Tsirkin wrote:
> >>> On Wed, Jun 08, 2011 at 10:57:13PM +0200, Jan Kiszka wrote:
> >>>> On 2011-06-08 22:56, Michael S. Tsirkin wrote:
> >>>>> On Wed, Jun 08, 2011 at 10:44:58PM +0200, Jan Kiszka wrote:
> >>>>>> On 2011-06-08 21:48, Michael S. Tsirkin wrote:
> >>>>>>> On Wed, Jun 08, 2011 at 06:21:52PM +0200, Jan Kiszka wrote:
> >>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>>
> >>>>>>> pci_regs.h from linux does not have these
> >>>>>>> this is why we keep them in msi.c
> >>>>>>>
> >>>>>>> [mst@tuck linux-2.6]$ grep PCI_MSI_ include/linux/pci_regs.h 
> >>>>>>> #define PCI_MSI_FLAGS           2       /* Various flags */
> >>>>>>> #define  PCI_MSI_FLAGS_64BIT    0x80    /* 64-bit addresses allowed */
> >>>>>>> #define  PCI_MSI_FLAGS_QSIZE    0x70    /* Message queue size configured
> >>>>>>> */
> >>>>>>> #define  PCI_MSI_FLAGS_QMASK    0x0e    /* Maximum queue size available
> >>>>>>> */
> >>>>>>> #define  PCI_MSI_FLAGS_ENABLE   0x01    /* MSI feature enabled */
> >>>>>>> #define  PCI_MSI_FLAGS_MASKBIT  0x100   /* 64-bit mask bits allowed */
> >>>>>>> #define PCI_MSI_RFU             3       /* Rest of capability flags */
> >>>>>>> #define PCI_MSI_ADDRESS_LO      4       /* Lower 32 bits */
> >>>>>>> #define PCI_MSI_ADDRESS_HI      8       /* Upper 32 bits (if
> >>>>>>> PCI_MSI_FLAGS_64BIT set) */
> >>>>>>> #define PCI_MSI_DATA_32         8       /* 16 bits of data for 32-bit
> >>>>>>> devices */
> >>>>>>> #define PCI_MSI_MASK_32         12      /* Mask bits register for 32-bit
> >>>>>>> devices */
> >>>>>>> #define PCI_MSI_DATA_64         12      /* 16 bits of data for 64-bit
> >>>>>>> devices */
> >>>>>>> #define PCI_MSI_MASK_64         16      /* Mask bits register for 64-bit
> >>>>>>> devices */
> >>>>>>>
> >>>>>>>
> >>>>>>> If you want to move them, please send them upstream we'll merge when
> >>>>>>> they are there.
> >>>>>>
> >>>>>> In fact, both defines are already in libpci. Since 3.0.0. Released 5
> >>>>>> years ago. OK, I'll send a header resync patch against a more recent
> >>>>>> release.
> >>>>>>
> >>>>>> Then we should just lack something like PCI_MSIX_CTRL. I will have a look.
> >>>>>
> >>>>> PCI_MSIX_FLAGS is the same.
> >>>>
> >>>> That does not exist in libpci's header.
> >>>>
> >>>> Jan
> >>>>
> >>>
> >>> The upstream I meant is pci_Regs.h in Linux.
> >>>
> >>
> >> Why not sync against
> >> git://git.kernel.org/pub/scm/utils/pciutils/pciutils.git? Sounds more
> >> appropriate.
> >>
> >> Jan
> >>
> > 
> > We are 100% aligned with Linux changing that seems pointless.
> > I think Linux gets new hardware features faster and has a better
> > chance to be correct.
> > We already pull headers from Linux so that's one dependency less.
> > 
> 
> The kernel appears to gain defines based on what it uses.

I don't expect trouble adding stuff we might need though.

> The libpci
> headers seem to serve more use cases, at least it had the missing fields
> for several years and is a few hundred lines longer.
> 
> Jan
> 

Some defines got into Linux already, specifically
MSIX_TABLE_OFFSET, MSIX_PBA_OFFSET
(with different names)


-- 
MST