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

[Qemu-devel] [PATCH 0/7] 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).

CC: Alexander Graf <agraf@suse.de>
CC: Gerd Hoffmann <kraxel@redhat.com>

Jan Kiszka (7):
  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
  ahci/intel-hda: Properly reset MSI state
  msix: Align MSI-X constants to libpci definitions and extend them
  msi: Move PCI_MSI_PENDING_32/64 into pci_regs.h

 hw/ide/ahci.c  |    2 ++
 hw/intel-hda.c |    8 +++-----
 hw/msi.c       |   15 ++++++++-------
 hw/msix.c      |   39 ++++++++++++++++++++-------------------
 hw/pci_regs.h  |   16 ++++++++++------
 5 files changed, 43 insertions(+), 37 deletions(-)

[Qemu-devel] [PATCH 1/7] 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 2/7] 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.

CC: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/intel-hda.c |    6 +-----
 hw/msi.c       |    3 ++-
 hw/msix.c      |    2 +-
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/hw/intel-hda.c b/hw/intel-hda.c
index 5485745..71843f1 100644
--- a/hw/intel-hda.c
+++ b/hw/intel-hda.c
@@ -1173,12 +1173,8 @@ static int intel_hda_exit(PCIDevice *pci)
 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);
-    }
+    msi_write_config(pci, addr, val, len);
 }
 
 static int intel_hda_post_load(void *opaque, int version)
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 3/7] 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 4/7] 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 5/7] ahci/intel-hda: Properly reset MSI state

[Jan Kiszka]

Also invoke msi_reset on device reset to unsure proper config space
state.

CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Alexander Graf <agraf@suse.de>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/ide/ahci.c  |    2 ++
 hw/intel-hda.c |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 1f008a3..feb0ea9 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1159,6 +1159,8 @@ void ahci_reset(void *opaque)
     struct AHCIPCIState *d = opaque;
     int i;
 
+    msi_reset(&d->card);
+
     d->ahci.control_regs.irqstatus = 0;
     d->ahci.control_regs.ghc = 0;
 
diff --git a/hw/intel-hda.c b/hw/intel-hda.c
index 71843f1..5dd69c7 100644
--- a/hw/intel-hda.c
+++ b/hw/intel-hda.c
@@ -1117,6 +1117,8 @@ static void intel_hda_reset(DeviceState *dev)
     DeviceState *qdev;
     HDACodecDevice *cdev;
 
+    msi_reset(&d->pci);
+
     intel_hda_regs_reset(d);
     d->wall_base_ns = qemu_get_clock_ns(vm_clock);
 
-- 
1.7.1

[Qemu-devel] [PATCH 6/7] 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 7/7] 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 09bcdd1..edd3e5b 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

[Qemu-devel] [PATCH 8/7] ivshmem: Reset MSI-X state on device reset

[Jan Kiszka]

CC: Cam Macdonell <cam@cs.ualberta.ca>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/ivshmem.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index 7b19a81..386f19f 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -545,6 +545,7 @@ static void ivshmem_reset(DeviceState *d)
 {
     IVShmemState *s = DO_UPCAST(IVShmemState, dev.qdev, d);
 
+    msix_reset(&s->dev);
     s->intrstatus = 0;
     return;
 }

Re: [Qemu-devel] [PATCH 5/7] ahci/intel-hda: Properly reset MSI state

[Alexander Graf]

On 08.06.2011, at 12:26, Jan Kiszka wrote:

> Also invoke msi_reset on device reset to unsure proper config space

ensure.

Otherwise looks sane :)


Alex

Re: [Qemu-devel] [PATCH 5/7] ahci/intel-hda: Properly reset MSI state

[Jan Kiszka]

On 2011-06-08 12:53, Alexander Graf wrote:
> 
> On 08.06.2011, at 12:26, Jan Kiszka wrote:
> 
>> Also invoke msi_reset on device reset to unsure proper config space
> 
> ensure.

Sure. :)

Michael, please fix up on commit unless I'll have to repost anyway.

> 
> Otherwise looks sane :)
> 
> 
> Alex
> 

Thanks,
Jan

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

Re: [Qemu-devel] [PATCH 2/7] msi: Guard msi/msix_write_config with msi_present

[Gerd Hoffmann]

> @@ -1173,12 +1173,8 @@ static int intel_hda_exit(PCIDevice *pci)
>   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);
> -    }
> +    msi_write_config(pci, addr, val, len);
>   }

Nothing device specific left in there now.  If msi_write_config() checks 
itself whenever msi is enabled or not we could call it from 
pci_default_write_config(), then zap this function altogether, no?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 2/7] msi: Guard msi/msix_write_config with msi_present

[Jan Kiszka]

On 2011-06-08 14:28, Gerd Hoffmann wrote:
>> @@ -1173,12 +1173,8 @@ static int intel_hda_exit(PCIDevice *pci)
>>   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);
>> -    }
>> +    msi_write_config(pci, addr, val, len);
>>   }
> 
> Nothing device specific left in there now.  If msi_write_config() checks 
> itself whenever msi is enabled or not we could call it from 
> pci_default_write_config(), then zap this function altogether, no?

Well, we could put those helpers (msi_write_config, msi_reset, maybe
more) into generic PCI code. Then device will only have to overload the
handlers if they do something in addition. Haven't checked the details
yet, though.

Jan

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

Re: [Qemu-devel] [PATCH 2/7] msi: Guard msi/msix_write_config with msi_present

[Jan Kiszka]

On 2011-06-08 14:33, Jan Kiszka wrote:
> On 2011-06-08 14:28, Gerd Hoffmann wrote:
>>> @@ -1173,12 +1173,8 @@ static int intel_hda_exit(PCIDevice *pci)
>>>   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);
>>> -    }
>>> +    msi_write_config(pci, addr, val, len);
>>>   }
>>
>> Nothing device specific left in there now.  If msi_write_config() checks 
>> itself whenever msi is enabled or not we could call it from 
>> pci_default_write_config(), then zap this function altogether, no?
> 
> Well, we could put those helpers (msi_write_config, msi_reset, maybe
> more) into generic PCI code. Then device will only have to overload the
> handlers if they do something in addition. Haven't checked the details
> yet, though.

Seems to fly.

I'll repost the series to make msi_reset, msi_write_config, and
msi_uninit implicit. Less boilerplate, less potential bugs.

Jan

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

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

[Isaku Yamahata]

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?
-- 
yamahata

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

[Jan Kiszka]

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

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

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

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH 0/7] msi: Small cleanups and fixes

[Michael S. Tsirkin]

On Wed, Jun 08, 2011 at 12:26:38PM +0200, Jan Kiszka wrote:
> A few patches to the MSI and MSI-X layer that clean up the interfaces
> and fix reset issues.

Are there any more fixes besides the typo in uninit?

> They are from my MSI rework to prepare it for
> KVM's requirements (in-kernel irqchip).
> 
> CC: Alexander Graf <agraf@suse.de>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> 
> Jan Kiszka (7):
>   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
>   ahci/intel-hda: Properly reset MSI state
>   msix: Align MSI-X constants to libpci definitions and extend them
>   msi: Move PCI_MSI_PENDING_32/64 into pci_regs.h
> 
>  hw/ide/ahci.c  |    2 ++
>  hw/intel-hda.c |    8 +++-----
>  hw/msi.c       |   15 ++++++++-------
>  hw/msix.c      |   39 ++++++++++++++++++++-------------------
>  hw/pci_regs.h  |   16 ++++++++++------
>  5 files changed, 43 insertions(+), 37 deletions(-)