Patchwork [6/7] msix: Align MSI-X constants to libpci definitions and extend them

login
register
mail settings
Submitter Jan Kiszka
Date June 8, 2011, 10:26 a.m.
Message ID <32d12d55df0a3a98b7f05b3be5e896fe148d59f4.1307528800.git.jan.kiszka@siemens.com>
Download mbox | patch
Permalink /patch/99402/
State New
Headers show

Comments

Jan Kiszka - June 8, 2011, 10:26 a.m.
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(-)
Isaku Yamahata - June 8, 2011, 2:43 p.m.
On Wed, Jun 08, 2011 at 12:26:44PM +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.

So since libpci would be used by qemu, you are claiming that
it should be switched to libpci style from Linux pci_regs.h?
Jan Kiszka - June 8, 2011, 2:46 p.m.
On 2011-06-08 16:43, Isaku Yamahata wrote:
> On Wed, Jun 08, 2011 at 12:26:44PM +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.
> 
> So since libpci would be used by qemu, you are claiming that
> it should be switched to libpci style from Linux pci_regs.h?

No, libpci won't be used (I'm about to remove that dependency from
qemu-kvm). We are just copying its definitions, so I think we should
align to its style.

Jan
Michael S. Tsirkin - June 8, 2011, 8:14 p.m.
On Wed, Jun 08, 2011 at 04:46:13PM +0200, Jan Kiszka wrote:
> On 2011-06-08 16:43, Isaku Yamahata wrote:
> > On Wed, Jun 08, 2011 at 12:26:44PM +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.
> > 
> > So since libpci would be used by qemu, you are claiming that
> > it should be switched to libpci style from Linux pci_regs.h?
> 
> No, libpci won't be used (I'm about to remove that dependency from
> qemu-kvm). We are just copying its definitions, so I think we should
> align to its style.
> 
> Jan

But why?

We are currently aligned with pci_regs.h from linux.
What is the benefit of switching styles?
It adds support overhead e.g. as backport becomes harder.

BTW, libpci doesn't seem to have pci_regs.h - it has pci/header.h

If some register definitions are missing in linux, what
we did in the past is:
- stick them in a C file that needs them meanwhile
- long term add them to linux and backport to our header

We could also add pci_ext_regs.h or something to this end
as an alternative, although this removes the pressure
to upstream definitions ...

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

Patch

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 */