[Qemu-devel] [PATCH v2 0/6] msix: Support specifying offsets, BARs, and capability location

[Qemu-devel] [PATCH v2 0/6] msix: Support specifying offsets, BARs, and capability location

[Alex Williamson]

v2:
 - split patch
 - rename msix_[un]init_bar() to msix_[un]init_exclusive_bar()
 - add the cherry on top to cleaning up PCIDevice naming

Thanks,

Alex

v1:

msix_init has very little configurability as to how it lays out MSIX
for a device.  It claims to resize BARs, but doesn't actually do this
anymore.  This patch allows MSIX to be fully specified, which is
necessary both for emulated devices trying to match the physical
layout of a hardware device as well as for any kind of device
assignment.

New functions msix_init_bar & msix_uninit_bar provide wrappers around
the more detailed functions for drivers that just want a simple MSIX
setup.

---

Alex Williamson (6):
      msix: Fix last PCIDevice naming inconsitency
      msix: Allow full specification of MSIX layout
      msix: Split PBA into it's own MemoryRegion
      virtio: Convert to msix_init_exclusive_bar() interface
      ivshmem: Convert to msix_init_exclusive_bar() interface
      msix: Add simple BAR allocation MSIX setup functions


 hw/ivshmem.c    |   10 +-
 hw/msix.c       |  308 +++++++++++++++++++++++++++++++------------------------
 hw/msix.h       |   20 ++--
 hw/pci.h        |   17 ++-
 hw/virtio-pci.c |   15 +--
 hw/virtio-pci.h |    1 
 6 files changed, 209 insertions(+), 162 deletions(-)

[Qemu-devel] [PATCH v2 1/6] msix: Add simple BAR allocation MSIX setup functions

[Alex Williamson]

msi_init() takes over a BAR without really specifying or allowing
specification of how it does so.  Instead, let's split it into
two interfaces, one fully specified, and one trivially easy.  This
implements the latter.  msix_init_exclusive_bar() takes over
allocating and filling a PCI BAR _exclusively_ for the use of MSIX.
When used, the matching msi_uninit_exclusive_bar() should be used
to tear it down.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/msix.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 hw/msix.h |    3 +++
 hw/pci.h  |    2 ++
 3 files changed, 48 insertions(+)

diff --git a/hw/msix.c b/hw/msix.c
index b64f109..a4cdfb0 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -299,6 +299,41 @@ err_config:
     return ret;
 }
 
+int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
+                            uint8_t bar_nr)
+{
+    int ret;
+    char *name;
+
+    /*
+     * Migration compatibility dictates that this remains a 4k
+     * BAR with the vector table in the lower half and PBA in
+     * the upper half.
+     */
+    if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) {
+        return -EINVAL;
+    }
+
+    if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) {
+        return -ENOMEM;
+    }
+
+    memory_region_init(&dev->msix_exclusive_bar, name, 4096);
+
+    free(name);
+
+    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
+    if (ret) {
+        memory_region_destroy(&dev->msix_exclusive_bar);
+        return ret;
+    }
+
+    pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY,
+                     &dev->msix_exclusive_bar);
+
+    return 0;
+}
+
 static void msix_free_irq_entries(PCIDevice *dev)
 {
     int vector;
@@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
     return 0;
 }
 
+void msix_uninit_exclusive_bar(PCIDevice *dev)
+{
+    if (msix_present(dev)) {
+        msix_uninit(dev, &dev->msix_exclusive_bar);
+        memory_region_destroy(&dev->msix_exclusive_bar);
+    }
+}
+
 void msix_save(PCIDevice *dev, QEMUFile *f)
 {
     unsigned n = dev->msix_entries_nr;
diff --git a/hw/msix.h b/hw/msix.h
index e5a488d..bed6bfb 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -7,11 +7,14 @@
 int msix_init(PCIDevice *pdev, unsigned short nentries,
               MemoryRegion *bar,
               unsigned bar_nr, unsigned bar_size);
+int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
+                            uint8_t bar_nr);
 
 void msix_write_config(PCIDevice *pci_dev, uint32_t address,
                        uint32_t val, int len);
 
 int msix_uninit(PCIDevice *d, MemoryRegion *bar);
+void msix_uninit_exclusive_bar(PCIDevice *dev);
 
 unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
 
diff --git a/hw/pci.h b/hw/pci.h
index 4c96268..d517a54 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -226,6 +226,8 @@ struct PCIDevice {
 
     /* Space to store MSIX table */
     uint8_t *msix_table_page;
+    /* MemoryRegion container for msix exclusive BAR setup */
+    MemoryRegion msix_exclusive_bar;
     /* MMIO index used to map MSIX table and pending bit entries. */
     MemoryRegion msix_mmio;
     /* Reference-count for entries actually in use by driver. */

Re: [Qemu-devel] [PATCH v2 1/6] msix: Add simple BAR allocation MSIX setup functions

[Michael S. Tsirkin]

On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote:
> msi_init() takes over a BAR without really specifying or allowing
> specification of how it does so.  Instead, let's split it into
> two interfaces, one fully specified, and one trivially easy.  This
> implements the latter.  msix_init_exclusive_bar() takes over
> allocating and filling a PCI BAR _exclusively_ for the use of MSIX.
> When used, the matching msi_uninit_exclusive_bar() should be used
> to tear it down.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
>  hw/msix.c |   43 +++++++++++++++++++++++++++++++++++++++++++
>  hw/msix.h |    3 +++
>  hw/pci.h  |    2 ++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index b64f109..a4cdfb0 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -299,6 +299,41 @@ err_config:
>      return ret;
>  }
>  
> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> +                            uint8_t bar_nr)
> +{
> +    int ret;
> +    char *name;
> +
> +    /*
> +     * Migration compatibility dictates that this remains a 4k
> +     * BAR with the vector table in the lower half and PBA in
> +     * the upper half.
> +     */
> +    if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) {
> +        return -EINVAL;
> +    }
> +
> +    if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) {
> +        return -ENOMEM;
> +    }
> +

I think we should avoid spaces in names.
Also let's use lower case. And BAR is confusing IMO:
it's not a base address register, it's the region.
So I would prefer simply: "%s-msix".


> +    memory_region_init(&dev->msix_exclusive_bar, name, 4096);
> +
> +    free(name);
> +
> +    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
> +    if (ret) {
> +        memory_region_destroy(&dev->msix_exclusive_bar);
> +        return ret;
> +    }
> +
> +    pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY,
> +                     &dev->msix_exclusive_bar);
> +
> +    return 0;
> +}
> +
>  static void msix_free_irq_entries(PCIDevice *dev)
>  {
>      int vector;
> @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
>      return 0;
>  }
>  
> +void msix_uninit_exclusive_bar(PCIDevice *dev)
> +{
> +    if (msix_present(dev)) {
> +        msix_uninit(dev, &dev->msix_exclusive_bar);
> +        memory_region_destroy(&dev->msix_exclusive_bar);
> +    }
> +}
> +
>  void msix_save(PCIDevice *dev, QEMUFile *f)
>  {
>      unsigned n = dev->msix_entries_nr;
> diff --git a/hw/msix.h b/hw/msix.h
> index e5a488d..bed6bfb 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -7,11 +7,14 @@
>  int msix_init(PCIDevice *pdev, unsigned short nentries,
>                MemoryRegion *bar,
>                unsigned bar_nr, unsigned bar_size);
> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> +                            uint8_t bar_nr);
>  
>  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
>                         uint32_t val, int len);
>  
>  int msix_uninit(PCIDevice *d, MemoryRegion *bar);
> +void msix_uninit_exclusive_bar(PCIDevice *dev);
>  
>  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
>  
> diff --git a/hw/pci.h b/hw/pci.h
> index 4c96268..d517a54 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -226,6 +226,8 @@ struct PCIDevice {
>  
>      /* Space to store MSIX table */
>      uint8_t *msix_table_page;
> +    /* MemoryRegion container for msix exclusive BAR setup */
> +    MemoryRegion msix_exclusive_bar;
>      /* MMIO index used to map MSIX table and pending bit entries. */
>      MemoryRegion msix_mmio;
>      /* Reference-count for entries actually in use by driver. */

Re: [Qemu-devel] [PATCH v2 1/6] msix: Add simple BAR allocation MSIX setup functions

[Alex Williamson]

On Thu, 2012-06-14 at 13:29 +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote:
> > msi_init() takes over a BAR without really specifying or allowing
> > specification of how it does so.  Instead, let's split it into
> > two interfaces, one fully specified, and one trivially easy.  This
> > implements the latter.  msix_init_exclusive_bar() takes over
> > allocating and filling a PCI BAR _exclusively_ for the use of MSIX.
> > When used, the matching msi_uninit_exclusive_bar() should be used
> > to tear it down.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > 
> >  hw/msix.c |   43 +++++++++++++++++++++++++++++++++++++++++++
> >  hw/msix.h |    3 +++
> >  hw/pci.h  |    2 ++
> >  3 files changed, 48 insertions(+)
> > 
> > diff --git a/hw/msix.c b/hw/msix.c
> > index b64f109..a4cdfb0 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -299,6 +299,41 @@ err_config:
> >      return ret;
> >  }
> >  
> > +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> > +                            uint8_t bar_nr)
> > +{
> > +    int ret;
> > +    char *name;
> > +
> > +    /*
> > +     * Migration compatibility dictates that this remains a 4k
> > +     * BAR with the vector table in the lower half and PBA in
> > +     * the upper half.
> > +     */
> > +    if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) {
> > +        return -EINVAL;
> > +    }
> > +
> > +    if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) {
> > +        return -ENOMEM;
> > +    }
> > +
> 
> I think we should avoid spaces in names.
> Also let's use lower case. And BAR is confusing IMO:
> it's not a base address register, it's the region.

It's a region backing a base address register.

> So I would prefer simply: "%s-msix".

Ok, fixed.  Thanks,

Alex

> > +    memory_region_init(&dev->msix_exclusive_bar, name, 4096);
> > +
> > +    free(name);
> > +
> > +    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
> > +    if (ret) {
> > +        memory_region_destroy(&dev->msix_exclusive_bar);
> > +        return ret;
> > +    }
> > +
> > +    pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY,
> > +                     &dev->msix_exclusive_bar);
> > +
> > +    return 0;
> > +}
> > +
> >  static void msix_free_irq_entries(PCIDevice *dev)
> >  {
> >      int vector;
> > @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
> >      return 0;
> >  }
> >  
> > +void msix_uninit_exclusive_bar(PCIDevice *dev)
> > +{
> > +    if (msix_present(dev)) {
> > +        msix_uninit(dev, &dev->msix_exclusive_bar);
> > +        memory_region_destroy(&dev->msix_exclusive_bar);
> > +    }
> > +}
> > +
> >  void msix_save(PCIDevice *dev, QEMUFile *f)
> >  {
> >      unsigned n = dev->msix_entries_nr;
> > diff --git a/hw/msix.h b/hw/msix.h
> > index e5a488d..bed6bfb 100644
> > --- a/hw/msix.h
> > +++ b/hw/msix.h
> > @@ -7,11 +7,14 @@
> >  int msix_init(PCIDevice *pdev, unsigned short nentries,
> >                MemoryRegion *bar,
> >                unsigned bar_nr, unsigned bar_size);
> > +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> > +                            uint8_t bar_nr);
> >  
> >  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
> >                         uint32_t val, int len);
> >  
> >  int msix_uninit(PCIDevice *d, MemoryRegion *bar);
> > +void msix_uninit_exclusive_bar(PCIDevice *dev);
> >  
> >  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
> >  
> > diff --git a/hw/pci.h b/hw/pci.h
> > index 4c96268..d517a54 100644
> > --- a/hw/pci.h
> > +++ b/hw/pci.h
> > @@ -226,6 +226,8 @@ struct PCIDevice {
> >  
> >      /* Space to store MSIX table */
> >      uint8_t *msix_table_page;
> > +    /* MemoryRegion container for msix exclusive BAR setup */
> > +    MemoryRegion msix_exclusive_bar;
> >      /* MMIO index used to map MSIX table and pending bit entries. */
> >      MemoryRegion msix_mmio;
> >      /* Reference-count for entries actually in use by driver. */
> 

Re: [Qemu-devel] [PATCH v2 1/6] msix: Add simple BAR allocation MSIX setup functions

[Michael S. Tsirkin]

On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote:
> msi_init() takes over a BAR without really specifying or allowing
> specification of how it does so.  Instead, let's split it into
> two interfaces, one fully specified, and one trivially easy.  This
> implements the latter.  msix_init_exclusive_bar() takes over
> allocating and filling a PCI BAR _exclusively_ for the use of MSIX.
> When used, the matching msi_uninit_exclusive_bar() should be used
> to tear it down.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
>  hw/msix.c |   43 +++++++++++++++++++++++++++++++++++++++++++
>  hw/msix.h |    3 +++
>  hw/pci.h  |    2 ++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index b64f109..a4cdfb0 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -299,6 +299,41 @@ err_config:
>      return ret;
>  }
>  
> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> +                            uint8_t bar_nr)
> +{
> +    int ret;
> +    char *name;
> +
> +    /*
> +     * Migration compatibility dictates that this remains a 4k
> +     * BAR with the vector table in the lower half and PBA in
> +     * the upper half.
> +     */
> +    if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) {
> +        return -EINVAL;
> +    }
> +
> +    if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) {
> +        return -ENOMEM;
> +    }
> +
> +    memory_region_init(&dev->msix_exclusive_bar, name, 4096);
> +
> +    free(name);
> +
> +    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
> +    if (ret) {
> +        memory_region_destroy(&dev->msix_exclusive_bar);
> +        return ret;
> +    }
> +
> +    pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY,
> +                     &dev->msix_exclusive_bar);
> +
> +    return 0;
> +}
> +
>  static void msix_free_irq_entries(PCIDevice *dev)
>  {
>      int vector;
> @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
>      return 0;
>  }
>  
> +void msix_uninit_exclusive_bar(PCIDevice *dev)
> +{
> +    if (msix_present(dev)) {
> +        msix_uninit(dev, &dev->msix_exclusive_bar);
> +        memory_region_destroy(&dev->msix_exclusive_bar);
> +    }
> +}
> +
>  void msix_save(PCIDevice *dev, QEMUFile *f)
>  {
>      unsigned n = dev->msix_entries_nr;
> diff --git a/hw/msix.h b/hw/msix.h
> index e5a488d..bed6bfb 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -7,11 +7,14 @@
>  int msix_init(PCIDevice *pdev, unsigned short nentries,
>                MemoryRegion *bar,
>                unsigned bar_nr, unsigned bar_size);
> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> +                            uint8_t bar_nr);
>  
>  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
>                         uint32_t val, int len);
>  
>  int msix_uninit(PCIDevice *d, MemoryRegion *bar);
> +void msix_uninit_exclusive_bar(PCIDevice *dev);
>  
>  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
>  
> diff --git a/hw/pci.h b/hw/pci.h
> index 4c96268..d517a54 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -226,6 +226,8 @@ struct PCIDevice {
>  
>      /* Space to store MSIX table */
>      uint8_t *msix_table_page;
> +    /* MemoryRegion container for msix exclusive BAR setup */
> +    MemoryRegion msix_exclusive_bar;

Wrappers are good but this doesn't belong in PCIDevice IMO.
E.g. when I debug and look at data structure I don't want to dig into
code and go "oh this device uses an exclusive bar to it's here, that one
uses a non exlcusive so the field is invalid, the real bar is over
there.

Keep the region in devices where it was, pass to msix_init_exclusive_bar
by parameter.

This way these are just helpers.

>      /* MMIO index used to map MSIX table and pending bit entries. */
>      MemoryRegion msix_mmio;
>      /* Reference-count for entries actually in use by driver. */

Re: [Qemu-devel] [PATCH v2 1/6] msix: Add simple BAR allocation MSIX setup functions

[Jan Kiszka]

On 2012-06-14 17:49, Michael S. Tsirkin wrote:
> On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote:
>> msi_init() takes over a BAR without really specifying or allowing
>> specification of how it does so.  Instead, let's split it into
>> two interfaces, one fully specified, and one trivially easy.  This
>> implements the latter.  msix_init_exclusive_bar() takes over
>> allocating and filling a PCI BAR _exclusively_ for the use of MSIX.
>> When used, the matching msi_uninit_exclusive_bar() should be used
>> to tear it down.
>>
>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>> ---
>>
>>  hw/msix.c |   43 +++++++++++++++++++++++++++++++++++++++++++
>>  hw/msix.h |    3 +++
>>  hw/pci.h  |    2 ++
>>  3 files changed, 48 insertions(+)
>>
>> diff --git a/hw/msix.c b/hw/msix.c
>> index b64f109..a4cdfb0 100644
>> --- a/hw/msix.c
>> +++ b/hw/msix.c
>> @@ -299,6 +299,41 @@ err_config:
>>      return ret;
>>  }
>>  
>> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>> +                            uint8_t bar_nr)
>> +{
>> +    int ret;
>> +    char *name;
>> +
>> +    /*
>> +     * Migration compatibility dictates that this remains a 4k
>> +     * BAR with the vector table in the lower half and PBA in
>> +     * the upper half.
>> +     */
>> +    if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) {
>> +        return -ENOMEM;
>> +    }
>> +
>> +    memory_region_init(&dev->msix_exclusive_bar, name, 4096);
>> +
>> +    free(name);
>> +
>> +    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
>> +    if (ret) {
>> +        memory_region_destroy(&dev->msix_exclusive_bar);
>> +        return ret;
>> +    }
>> +
>> +    pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY,
>> +                     &dev->msix_exclusive_bar);
>> +
>> +    return 0;
>> +}
>> +
>>  static void msix_free_irq_entries(PCIDevice *dev)
>>  {
>>      int vector;
>> @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
>>      return 0;
>>  }
>>  
>> +void msix_uninit_exclusive_bar(PCIDevice *dev)
>> +{
>> +    if (msix_present(dev)) {
>> +        msix_uninit(dev, &dev->msix_exclusive_bar);
>> +        memory_region_destroy(&dev->msix_exclusive_bar);
>> +    }
>> +}
>> +
>>  void msix_save(PCIDevice *dev, QEMUFile *f)
>>  {
>>      unsigned n = dev->msix_entries_nr;
>> diff --git a/hw/msix.h b/hw/msix.h
>> index e5a488d..bed6bfb 100644
>> --- a/hw/msix.h
>> +++ b/hw/msix.h
>> @@ -7,11 +7,14 @@
>>  int msix_init(PCIDevice *pdev, unsigned short nentries,
>>                MemoryRegion *bar,
>>                unsigned bar_nr, unsigned bar_size);
>> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>> +                            uint8_t bar_nr);
>>  
>>  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
>>                         uint32_t val, int len);
>>  
>>  int msix_uninit(PCIDevice *d, MemoryRegion *bar);
>> +void msix_uninit_exclusive_bar(PCIDevice *dev);
>>  
>>  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
>>  
>> diff --git a/hw/pci.h b/hw/pci.h
>> index 4c96268..d517a54 100644
>> --- a/hw/pci.h
>> +++ b/hw/pci.h
>> @@ -226,6 +226,8 @@ struct PCIDevice {
>>  
>>      /* Space to store MSIX table */
>>      uint8_t *msix_table_page;
>> +    /* MemoryRegion container for msix exclusive BAR setup */
>> +    MemoryRegion msix_exclusive_bar;
> 
> Wrappers are good but this doesn't belong in PCIDevice IMO.
> E.g. when I debug and look at data structure I don't want to dig into
> code and go "oh this device uses an exclusive bar to it's here, that one
> uses a non exlcusive so the field is invalid, the real bar is over
> there.
> 
> Keep the region in devices where it was, pass to msix_init_exclusive_bar
> by parameter.

I disagree. There are plenty of fields in PCIDevice that are only used
under certain circumstances. This is just +1, and it makes the interface
usage much easier.

Jan

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

Re: [Qemu-devel] [PATCH v2 1/6] msix: Add simple BAR allocation MSIX setup functions

[Michael S. Tsirkin]

On Thu, Jun 14, 2012 at 05:55:18PM +0200, Jan Kiszka wrote:
> On 2012-06-14 17:49, Michael S. Tsirkin wrote:
> > On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote:
> >> msi_init() takes over a BAR without really specifying or allowing
> >> specification of how it does so.  Instead, let's split it into
> >> two interfaces, one fully specified, and one trivially easy.  This
> >> implements the latter.  msix_init_exclusive_bar() takes over
> >> allocating and filling a PCI BAR _exclusively_ for the use of MSIX.
> >> When used, the matching msi_uninit_exclusive_bar() should be used
> >> to tear it down.
> >>
> >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >> ---
> >>
> >>  hw/msix.c |   43 +++++++++++++++++++++++++++++++++++++++++++
> >>  hw/msix.h |    3 +++
> >>  hw/pci.h  |    2 ++
> >>  3 files changed, 48 insertions(+)
> >>
> >> diff --git a/hw/msix.c b/hw/msix.c
> >> index b64f109..a4cdfb0 100644
> >> --- a/hw/msix.c
> >> +++ b/hw/msix.c
> >> @@ -299,6 +299,41 @@ err_config:
> >>      return ret;
> >>  }
> >>  
> >> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >> +                            uint8_t bar_nr)
> >> +{
> >> +    int ret;
> >> +    char *name;
> >> +
> >> +    /*
> >> +     * Migration compatibility dictates that this remains a 4k
> >> +     * BAR with the vector table in the lower half and PBA in
> >> +     * the upper half.
> >> +     */
> >> +    if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) {
> >> +        return -EINVAL;
> >> +    }
> >> +
> >> +    if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) {
> >> +        return -ENOMEM;
> >> +    }
> >> +
> >> +    memory_region_init(&dev->msix_exclusive_bar, name, 4096);
> >> +
> >> +    free(name);
> >> +
> >> +    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
> >> +    if (ret) {
> >> +        memory_region_destroy(&dev->msix_exclusive_bar);
> >> +        return ret;
> >> +    }
> >> +
> >> +    pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY,
> >> +                     &dev->msix_exclusive_bar);
> >> +
> >> +    return 0;
> >> +}
> >> +
> >>  static void msix_free_irq_entries(PCIDevice *dev)
> >>  {
> >>      int vector;
> >> @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
> >>      return 0;
> >>  }
> >>  
> >> +void msix_uninit_exclusive_bar(PCIDevice *dev)
> >> +{
> >> +    if (msix_present(dev)) {
> >> +        msix_uninit(dev, &dev->msix_exclusive_bar);
> >> +        memory_region_destroy(&dev->msix_exclusive_bar);
> >> +    }
> >> +}
> >> +
> >>  void msix_save(PCIDevice *dev, QEMUFile *f)
> >>  {
> >>      unsigned n = dev->msix_entries_nr;
> >> diff --git a/hw/msix.h b/hw/msix.h
> >> index e5a488d..bed6bfb 100644
> >> --- a/hw/msix.h
> >> +++ b/hw/msix.h
> >> @@ -7,11 +7,14 @@
> >>  int msix_init(PCIDevice *pdev, unsigned short nentries,
> >>                MemoryRegion *bar,
> >>                unsigned bar_nr, unsigned bar_size);
> >> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >> +                            uint8_t bar_nr);
> >>  
> >>  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
> >>                         uint32_t val, int len);
> >>  
> >>  int msix_uninit(PCIDevice *d, MemoryRegion *bar);
> >> +void msix_uninit_exclusive_bar(PCIDevice *dev);
> >>  
> >>  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
> >>  
> >> diff --git a/hw/pci.h b/hw/pci.h
> >> index 4c96268..d517a54 100644
> >> --- a/hw/pci.h
> >> +++ b/hw/pci.h
> >> @@ -226,6 +226,8 @@ struct PCIDevice {
> >>  
> >>      /* Space to store MSIX table */
> >>      uint8_t *msix_table_page;
> >> +    /* MemoryRegion container for msix exclusive BAR setup */
> >> +    MemoryRegion msix_exclusive_bar;
> > 
> > Wrappers are good but this doesn't belong in PCIDevice IMO.
> > E.g. when I debug and look at data structure I don't want to dig into
> > code and go "oh this device uses an exclusive bar to it's here, that one
> > uses a non exlcusive so the field is invalid, the real bar is over
> > there.
> > 
> > Keep the region in devices where it was, pass to msix_init_exclusive_bar
> > by parameter.
> 
> I disagree. There are plenty of fields in PCIDevice that are only used
> under certain circumstances. This is just +1, and it makes the interface
> usage much easier.
> 
> Jan

*Much* easier? It's one parameter less.

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

Re: [Qemu-devel] [PATCH v2 1/6] msix: Add simple BAR allocation MSIX setup functions

[Alex Williamson]

On Thu, 2012-06-14 at 19:05 +0300, Michael S. Tsirkin wrote:
> On Thu, Jun 14, 2012 at 05:55:18PM +0200, Jan Kiszka wrote:
> > On 2012-06-14 17:49, Michael S. Tsirkin wrote:
> > > On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote:
> > >> msi_init() takes over a BAR without really specifying or allowing
> > >> specification of how it does so.  Instead, let's split it into
> > >> two interfaces, one fully specified, and one trivially easy.  This
> > >> implements the latter.  msix_init_exclusive_bar() takes over
> > >> allocating and filling a PCI BAR _exclusively_ for the use of MSIX.
> > >> When used, the matching msi_uninit_exclusive_bar() should be used
> > >> to tear it down.
> > >>
> > >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > >> ---
> > >>
> > >>  hw/msix.c |   43 +++++++++++++++++++++++++++++++++++++++++++
> > >>  hw/msix.h |    3 +++
> > >>  hw/pci.h  |    2 ++
> > >>  3 files changed, 48 insertions(+)
> > >>
> > >> diff --git a/hw/msix.c b/hw/msix.c
> > >> index b64f109..a4cdfb0 100644
> > >> --- a/hw/msix.c
> > >> +++ b/hw/msix.c
> > >> @@ -299,6 +299,41 @@ err_config:
> > >>      return ret;
> > >>  }
> > >>  
> > >> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> > >> +                            uint8_t bar_nr)
> > >> +{
> > >> +    int ret;
> > >> +    char *name;
> > >> +
> > >> +    /*
> > >> +     * Migration compatibility dictates that this remains a 4k
> > >> +     * BAR with the vector table in the lower half and PBA in
> > >> +     * the upper half.
> > >> +     */
> > >> +    if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) {
> > >> +        return -EINVAL;
> > >> +    }
> > >> +
> > >> +    if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) {
> > >> +        return -ENOMEM;
> > >> +    }
> > >> +
> > >> +    memory_region_init(&dev->msix_exclusive_bar, name, 4096);
> > >> +
> > >> +    free(name);
> > >> +
> > >> +    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
> > >> +    if (ret) {
> > >> +        memory_region_destroy(&dev->msix_exclusive_bar);
> > >> +        return ret;
> > >> +    }
> > >> +
> > >> +    pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY,
> > >> +                     &dev->msix_exclusive_bar);
> > >> +
> > >> +    return 0;
> > >> +}
> > >> +
> > >>  static void msix_free_irq_entries(PCIDevice *dev)
> > >>  {
> > >>      int vector;
> > >> @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
> > >>      return 0;
> > >>  }
> > >>  
> > >> +void msix_uninit_exclusive_bar(PCIDevice *dev)
> > >> +{
> > >> +    if (msix_present(dev)) {
> > >> +        msix_uninit(dev, &dev->msix_exclusive_bar);
> > >> +        memory_region_destroy(&dev->msix_exclusive_bar);
> > >> +    }
> > >> +}
> > >> +
> > >>  void msix_save(PCIDevice *dev, QEMUFile *f)
> > >>  {
> > >>      unsigned n = dev->msix_entries_nr;
> > >> diff --git a/hw/msix.h b/hw/msix.h
> > >> index e5a488d..bed6bfb 100644
> > >> --- a/hw/msix.h
> > >> +++ b/hw/msix.h
> > >> @@ -7,11 +7,14 @@
> > >>  int msix_init(PCIDevice *pdev, unsigned short nentries,
> > >>                MemoryRegion *bar,
> > >>                unsigned bar_nr, unsigned bar_size);
> > >> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> > >> +                            uint8_t bar_nr);
> > >>  
> > >>  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
> > >>                         uint32_t val, int len);
> > >>  
> > >>  int msix_uninit(PCIDevice *d, MemoryRegion *bar);
> > >> +void msix_uninit_exclusive_bar(PCIDevice *dev);
> > >>  
> > >>  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
> > >>  
> > >> diff --git a/hw/pci.h b/hw/pci.h
> > >> index 4c96268..d517a54 100644
> > >> --- a/hw/pci.h
> > >> +++ b/hw/pci.h
> > >> @@ -226,6 +226,8 @@ struct PCIDevice {
> > >>  
> > >>      /* Space to store MSIX table */
> > >>      uint8_t *msix_table_page;
> > >> +    /* MemoryRegion container for msix exclusive BAR setup */
> > >> +    MemoryRegion msix_exclusive_bar;
> > > 
> > > Wrappers are good but this doesn't belong in PCIDevice IMO.
> > > E.g. when I debug and look at data structure I don't want to dig into
> > > code and go "oh this device uses an exclusive bar to it's here, that one
> > > uses a non exlcusive so the field is invalid, the real bar is over
> > > there.
> > > 
> > > Keep the region in devices where it was, pass to msix_init_exclusive_bar
> > > by parameter.
> > 
> > I disagree. There are plenty of fields in PCIDevice that are only used
> > under certain circumstances. This is just +1, and it makes the interface
> > usage much easier.
> > 
> > Jan
> 
> *Much* easier? It's one parameter less.

I've been asked to implement pci_unregister_bar for two less
parameters...

Re: [Qemu-devel] [PATCH v2 1/6] msix: Add simple BAR allocation MSIX setup functions

[Michael S. Tsirkin]

On Thu, Jun 14, 2012 at 10:10:40AM -0600, Alex Williamson wrote:
> On Thu, 2012-06-14 at 19:05 +0300, Michael S. Tsirkin wrote:
> > On Thu, Jun 14, 2012 at 05:55:18PM +0200, Jan Kiszka wrote:
> > > On 2012-06-14 17:49, Michael S. Tsirkin wrote:
> > > > On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote:
> > > >> msi_init() takes over a BAR without really specifying or allowing
> > > >> specification of how it does so.  Instead, let's split it into
> > > >> two interfaces, one fully specified, and one trivially easy.  This
> > > >> implements the latter.  msix_init_exclusive_bar() takes over
> > > >> allocating and filling a PCI BAR _exclusively_ for the use of MSIX.
> > > >> When used, the matching msi_uninit_exclusive_bar() should be used
> > > >> to tear it down.
> > > >>
> > > >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > >> ---
> > > >>
> > > >>  hw/msix.c |   43 +++++++++++++++++++++++++++++++++++++++++++
> > > >>  hw/msix.h |    3 +++
> > > >>  hw/pci.h  |    2 ++
> > > >>  3 files changed, 48 insertions(+)
> > > >>
> > > >> diff --git a/hw/msix.c b/hw/msix.c
> > > >> index b64f109..a4cdfb0 100644
> > > >> --- a/hw/msix.c
> > > >> +++ b/hw/msix.c
> > > >> @@ -299,6 +299,41 @@ err_config:
> > > >>      return ret;
> > > >>  }
> > > >>  
> > > >> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> > > >> +                            uint8_t bar_nr)
> > > >> +{
> > > >> +    int ret;
> > > >> +    char *name;
> > > >> +
> > > >> +    /*
> > > >> +     * Migration compatibility dictates that this remains a 4k
> > > >> +     * BAR with the vector table in the lower half and PBA in
> > > >> +     * the upper half.
> > > >> +     */
> > > >> +    if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) {
> > > >> +        return -EINVAL;
> > > >> +    }
> > > >> +
> > > >> +    if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) {
> > > >> +        return -ENOMEM;
> > > >> +    }
> > > >> +
> > > >> +    memory_region_init(&dev->msix_exclusive_bar, name, 4096);
> > > >> +
> > > >> +    free(name);
> > > >> +
> > > >> +    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
> > > >> +    if (ret) {
> > > >> +        memory_region_destroy(&dev->msix_exclusive_bar);
> > > >> +        return ret;
> > > >> +    }
> > > >> +
> > > >> +    pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY,
> > > >> +                     &dev->msix_exclusive_bar);
> > > >> +
> > > >> +    return 0;
> > > >> +}
> > > >> +
> > > >>  static void msix_free_irq_entries(PCIDevice *dev)
> > > >>  {
> > > >>      int vector;
> > > >> @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
> > > >>      return 0;
> > > >>  }
> > > >>  
> > > >> +void msix_uninit_exclusive_bar(PCIDevice *dev)
> > > >> +{
> > > >> +    if (msix_present(dev)) {
> > > >> +        msix_uninit(dev, &dev->msix_exclusive_bar);
> > > >> +        memory_region_destroy(&dev->msix_exclusive_bar);
> > > >> +    }
> > > >> +}
> > > >> +
> > > >>  void msix_save(PCIDevice *dev, QEMUFile *f)
> > > >>  {
> > > >>      unsigned n = dev->msix_entries_nr;
> > > >> diff --git a/hw/msix.h b/hw/msix.h
> > > >> index e5a488d..bed6bfb 100644
> > > >> --- a/hw/msix.h
> > > >> +++ b/hw/msix.h
> > > >> @@ -7,11 +7,14 @@
> > > >>  int msix_init(PCIDevice *pdev, unsigned short nentries,
> > > >>                MemoryRegion *bar,
> > > >>                unsigned bar_nr, unsigned bar_size);
> > > >> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> > > >> +                            uint8_t bar_nr);
> > > >>  
> > > >>  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
> > > >>                         uint32_t val, int len);
> > > >>  
> > > >>  int msix_uninit(PCIDevice *d, MemoryRegion *bar);
> > > >> +void msix_uninit_exclusive_bar(PCIDevice *dev);
> > > >>  
> > > >>  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
> > > >>  
> > > >> diff --git a/hw/pci.h b/hw/pci.h
> > > >> index 4c96268..d517a54 100644
> > > >> --- a/hw/pci.h
> > > >> +++ b/hw/pci.h
> > > >> @@ -226,6 +226,8 @@ struct PCIDevice {
> > > >>  
> > > >>      /* Space to store MSIX table */
> > > >>      uint8_t *msix_table_page;
> > > >> +    /* MemoryRegion container for msix exclusive BAR setup */
> > > >> +    MemoryRegion msix_exclusive_bar;
> > > > 
> > > > Wrappers are good but this doesn't belong in PCIDevice IMO.
> > > > E.g. when I debug and look at data structure I don't want to dig into
> > > > code and go "oh this device uses an exclusive bar to it's here, that one
> > > > uses a non exlcusive so the field is invalid, the real bar is over
> > > > there.
> > > > 
> > > > Keep the region in devices where it was, pass to msix_init_exclusive_bar
> > > > by parameter.
> > > 
> > > I disagree. There are plenty of fields in PCIDevice that are only used
> > > under certain circumstances. This is just +1, and it makes the interface
> > > usage much easier.
> > > 
> > > Jan
> > 
> > *Much* easier? It's one parameter less.
> 
> I've been asked to implement pci_unregister_bar for two less
> parameters...

Yes but for a function that has too many already, and you were
adding more, to limit the damage I suggested this.
More than this, many parameters have same type so if
you get confused compiler won't help you.
If you find a way to not add parameters to msix_init I will not
ask you to reduce it's numer of parameters.

This one has 1 or two, not a big deal at all.

-- 
MST

Re: [Qemu-devel] [PATCH v2 1/6] msix: Add simple BAR allocation MSIX setup functions

[Jan Kiszka]

On 2012-06-14 18:05, Michael S. Tsirkin wrote:
> On Thu, Jun 14, 2012 at 05:55:18PM +0200, Jan Kiszka wrote:
>> On 2012-06-14 17:49, Michael S. Tsirkin wrote:
>>> On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote:
>>>> msi_init() takes over a BAR without really specifying or allowing
>>>> specification of how it does so.  Instead, let's split it into
>>>> two interfaces, one fully specified, and one trivially easy.  This
>>>> implements the latter.  msix_init_exclusive_bar() takes over
>>>> allocating and filling a PCI BAR _exclusively_ for the use of MSIX.
>>>> When used, the matching msi_uninit_exclusive_bar() should be used
>>>> to tear it down.
>>>>
>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>>> ---
>>>>
>>>>  hw/msix.c |   43 +++++++++++++++++++++++++++++++++++++++++++
>>>>  hw/msix.h |    3 +++
>>>>  hw/pci.h  |    2 ++
>>>>  3 files changed, 48 insertions(+)
>>>>
>>>> diff --git a/hw/msix.c b/hw/msix.c
>>>> index b64f109..a4cdfb0 100644
>>>> --- a/hw/msix.c
>>>> +++ b/hw/msix.c
>>>> @@ -299,6 +299,41 @@ err_config:
>>>>      return ret;
>>>>  }
>>>>  
>>>> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>>>> +                            uint8_t bar_nr)
>>>> +{
>>>> +    int ret;
>>>> +    char *name;
>>>> +
>>>> +    /*
>>>> +     * Migration compatibility dictates that this remains a 4k
>>>> +     * BAR with the vector table in the lower half and PBA in
>>>> +     * the upper half.
>>>> +     */
>>>> +    if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) {
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) {
>>>> +        return -ENOMEM;
>>>> +    }
>>>> +
>>>> +    memory_region_init(&dev->msix_exclusive_bar, name, 4096);
>>>> +
>>>> +    free(name);
>>>> +
>>>> +    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
>>>> +    if (ret) {
>>>> +        memory_region_destroy(&dev->msix_exclusive_bar);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY,
>>>> +                     &dev->msix_exclusive_bar);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>  static void msix_free_irq_entries(PCIDevice *dev)
>>>>  {
>>>>      int vector;
>>>> @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
>>>>      return 0;
>>>>  }
>>>>  
>>>> +void msix_uninit_exclusive_bar(PCIDevice *dev)
>>>> +{
>>>> +    if (msix_present(dev)) {
>>>> +        msix_uninit(dev, &dev->msix_exclusive_bar);
>>>> +        memory_region_destroy(&dev->msix_exclusive_bar);
>>>> +    }
>>>> +}
>>>> +
>>>>  void msix_save(PCIDevice *dev, QEMUFile *f)
>>>>  {
>>>>      unsigned n = dev->msix_entries_nr;
>>>> diff --git a/hw/msix.h b/hw/msix.h
>>>> index e5a488d..bed6bfb 100644
>>>> --- a/hw/msix.h
>>>> +++ b/hw/msix.h
>>>> @@ -7,11 +7,14 @@
>>>>  int msix_init(PCIDevice *pdev, unsigned short nentries,
>>>>                MemoryRegion *bar,
>>>>                unsigned bar_nr, unsigned bar_size);
>>>> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>>>> +                            uint8_t bar_nr);
>>>>  
>>>>  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
>>>>                         uint32_t val, int len);
>>>>  
>>>>  int msix_uninit(PCIDevice *d, MemoryRegion *bar);
>>>> +void msix_uninit_exclusive_bar(PCIDevice *dev);
>>>>  
>>>>  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
>>>>  
>>>> diff --git a/hw/pci.h b/hw/pci.h
>>>> index 4c96268..d517a54 100644
>>>> --- a/hw/pci.h
>>>> +++ b/hw/pci.h
>>>> @@ -226,6 +226,8 @@ struct PCIDevice {
>>>>  
>>>>      /* Space to store MSIX table */
>>>>      uint8_t *msix_table_page;
>>>> +    /* MemoryRegion container for msix exclusive BAR setup */
>>>> +    MemoryRegion msix_exclusive_bar;
>>>
>>> Wrappers are good but this doesn't belong in PCIDevice IMO.
>>> E.g. when I debug and look at data structure I don't want to dig into
>>> code and go "oh this device uses an exclusive bar to it's here, that one
>>> uses a non exlcusive so the field is invalid, the real bar is over
>>> there.
>>>
>>> Keep the region in devices where it was, pass to msix_init_exclusive_bar
>>> by parameter.
>>
>> I disagree. There are plenty of fields in PCIDevice that are only used
>> under certain circumstances. This is just +1, and it makes the interface
>> usage much easier.
>>
>> Jan
> 
> *Much* easier? It's one parameter less.

...and one field to add to every user of msix_init_exclusive_bar while
those users have no business with it. The container is a pure internal
field, so it belongs to the infrastructure.

Jan

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

Re: [Qemu-devel] [PATCH v2 1/6] msix: Add simple BAR allocation MSIX setup functions

[Michael S. Tsirkin]

On Thu, Jun 14, 2012 at 06:11:18PM +0200, Jan Kiszka wrote:
> On 2012-06-14 18:05, Michael S. Tsirkin wrote:
> > On Thu, Jun 14, 2012 at 05:55:18PM +0200, Jan Kiszka wrote:
> >> On 2012-06-14 17:49, Michael S. Tsirkin wrote:
> >>> On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote:
> >>>> msi_init() takes over a BAR without really specifying or allowing
> >>>> specification of how it does so.  Instead, let's split it into
> >>>> two interfaces, one fully specified, and one trivially easy.  This
> >>>> implements the latter.  msix_init_exclusive_bar() takes over
> >>>> allocating and filling a PCI BAR _exclusively_ for the use of MSIX.
> >>>> When used, the matching msi_uninit_exclusive_bar() should be used
> >>>> to tear it down.
> >>>>
> >>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >>>> ---
> >>>>
> >>>>  hw/msix.c |   43 +++++++++++++++++++++++++++++++++++++++++++
> >>>>  hw/msix.h |    3 +++
> >>>>  hw/pci.h  |    2 ++
> >>>>  3 files changed, 48 insertions(+)
> >>>>
> >>>> diff --git a/hw/msix.c b/hw/msix.c
> >>>> index b64f109..a4cdfb0 100644
> >>>> --- a/hw/msix.c
> >>>> +++ b/hw/msix.c
> >>>> @@ -299,6 +299,41 @@ err_config:
> >>>>      return ret;
> >>>>  }
> >>>>  
> >>>> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >>>> +                            uint8_t bar_nr)
> >>>> +{
> >>>> +    int ret;
> >>>> +    char *name;
> >>>> +
> >>>> +    /*
> >>>> +     * Migration compatibility dictates that this remains a 4k
> >>>> +     * BAR with the vector table in the lower half and PBA in
> >>>> +     * the upper half.
> >>>> +     */
> >>>> +    if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) {
> >>>> +        return -EINVAL;
> >>>> +    }
> >>>> +
> >>>> +    if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) {
> >>>> +        return -ENOMEM;
> >>>> +    }
> >>>> +
> >>>> +    memory_region_init(&dev->msix_exclusive_bar, name, 4096);
> >>>> +
> >>>> +    free(name);
> >>>> +
> >>>> +    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
> >>>> +    if (ret) {
> >>>> +        memory_region_destroy(&dev->msix_exclusive_bar);
> >>>> +        return ret;
> >>>> +    }
> >>>> +
> >>>> +    pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY,
> >>>> +                     &dev->msix_exclusive_bar);
> >>>> +
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>>  static void msix_free_irq_entries(PCIDevice *dev)
> >>>>  {
> >>>>      int vector;
> >>>> @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
> >>>>      return 0;
> >>>>  }
> >>>>  
> >>>> +void msix_uninit_exclusive_bar(PCIDevice *dev)
> >>>> +{
> >>>> +    if (msix_present(dev)) {
> >>>> +        msix_uninit(dev, &dev->msix_exclusive_bar);
> >>>> +        memory_region_destroy(&dev->msix_exclusive_bar);
> >>>> +    }
> >>>> +}
> >>>> +
> >>>>  void msix_save(PCIDevice *dev, QEMUFile *f)
> >>>>  {
> >>>>      unsigned n = dev->msix_entries_nr;
> >>>> diff --git a/hw/msix.h b/hw/msix.h
> >>>> index e5a488d..bed6bfb 100644
> >>>> --- a/hw/msix.h
> >>>> +++ b/hw/msix.h
> >>>> @@ -7,11 +7,14 @@
> >>>>  int msix_init(PCIDevice *pdev, unsigned short nentries,
> >>>>                MemoryRegion *bar,
> >>>>                unsigned bar_nr, unsigned bar_size);
> >>>> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >>>> +                            uint8_t bar_nr);
> >>>>  
> >>>>  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
> >>>>                         uint32_t val, int len);
> >>>>  
> >>>>  int msix_uninit(PCIDevice *d, MemoryRegion *bar);
> >>>> +void msix_uninit_exclusive_bar(PCIDevice *dev);
> >>>>  
> >>>>  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
> >>>>  
> >>>> diff --git a/hw/pci.h b/hw/pci.h
> >>>> index 4c96268..d517a54 100644
> >>>> --- a/hw/pci.h
> >>>> +++ b/hw/pci.h
> >>>> @@ -226,6 +226,8 @@ struct PCIDevice {
> >>>>  
> >>>>      /* Space to store MSIX table */
> >>>>      uint8_t *msix_table_page;
> >>>> +    /* MemoryRegion container for msix exclusive BAR setup */
> >>>> +    MemoryRegion msix_exclusive_bar;
> >>>
> >>> Wrappers are good but this doesn't belong in PCIDevice IMO.
> >>> E.g. when I debug and look at data structure I don't want to dig into
> >>> code and go "oh this device uses an exclusive bar to it's here, that one
> >>> uses a non exlcusive so the field is invalid, the real bar is over
> >>> there.
> >>>
> >>> Keep the region in devices where it was, pass to msix_init_exclusive_bar
> >>> by parameter.
> >>
> >> I disagree. There are plenty of fields in PCIDevice that are only used
> >> under certain circumstances. This is just +1, and it makes the interface
> >> usage much easier.
> >>
> >> Jan
> > 
> > *Much* easier? It's one parameter less.
> 
> ...and one field to add to every user of msix_init_exclusive_bar while
> those users have no business with it. The container is a pure internal
> field, so it belongs to the infrastructure.
> 
> Jan

True. I wish I had a better idea. Will ponder this a bit.

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

[Qemu-devel] [PATCH v2 2/6] ivshmem: Convert to msix_init_exclusive_bar() interface

[Alex Williamson]

Trivial conversion, failed to have an uninit before and after.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/ivshmem.c |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index 05559b6..8b49eee 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -70,7 +70,6 @@ typedef struct IVShmemState {
      */
     MemoryRegion bar;
     MemoryRegion ivshmem;
-    MemoryRegion msix_bar;
     uint64_t ivshmem_size; /* size of shared memory region */
     int shm_fd; /* shared memory file descriptor */
 
@@ -563,16 +562,13 @@ static uint64_t ivshmem_get_size(IVShmemState * s) {
 
 static void ivshmem_setup_msi(IVShmemState * s)
 {
-    memory_region_init(&s->msix_bar, "ivshmem-msix", 4096);
-    if (!msix_init(&s->dev, s->vectors, &s->msix_bar, 1, 0)) {
-        pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
-                         &s->msix_bar);
-        IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
-    } else {
+    if (msix_init_exclusive_bar(&s->dev, s->vectors, 1)) {
         IVSHMEM_DPRINTF("msix initialization failed\n");
         exit(1);
     }
 
+    IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
+
     /* allocate QEMU char devices for receiving interrupts */
     s->eventfd_table = g_malloc0(s->vectors * sizeof(EventfdEntry));
 

Re: [Qemu-devel] [PATCH v2 2/6] ivshmem: Convert to msix_init_exclusive_bar() interface

[Jan Kiszka]

On 2012-06-14 06:51, Alex Williamson wrote:
> Trivial conversion, failed to have an uninit before and after.

Need not be in this series, but we should fix that trivial bug nevertheless.

Jan

> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
>  hw/ivshmem.c |   10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index 05559b6..8b49eee 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -70,7 +70,6 @@ typedef struct IVShmemState {
>       */
>      MemoryRegion bar;
>      MemoryRegion ivshmem;
> -    MemoryRegion msix_bar;
>      uint64_t ivshmem_size; /* size of shared memory region */
>      int shm_fd; /* shared memory file descriptor */
>  
> @@ -563,16 +562,13 @@ static uint64_t ivshmem_get_size(IVShmemState * s) {
>  
>  static void ivshmem_setup_msi(IVShmemState * s)
>  {
> -    memory_region_init(&s->msix_bar, "ivshmem-msix", 4096);
> -    if (!msix_init(&s->dev, s->vectors, &s->msix_bar, 1, 0)) {
> -        pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
> -                         &s->msix_bar);
> -        IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
> -    } else {
> +    if (msix_init_exclusive_bar(&s->dev, s->vectors, 1)) {
>          IVSHMEM_DPRINTF("msix initialization failed\n");
>          exit(1);
>      }
>  
> +    IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
> +
>      /* allocate QEMU char devices for receiving interrupts */
>      s->eventfd_table = g_malloc0(s->vectors * sizeof(EventfdEntry));
>  
> 

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

Re: [Qemu-devel] [PATCH v2 2/6] ivshmem: Convert to msix_init_exclusive_bar() interface

[Alex Williamson]

On Thu, 2012-06-14 at 08:01 +0200, Jan Kiszka wrote:
> On 2012-06-14 06:51, Alex Williamson wrote:
> > Trivial conversion, failed to have an uninit before and after.
> 
> Need not be in this series, but we should fix that trivial bug nevertheless.

Given the exit(1) on failure to setup msix, I'm guessing ivshmem isn't
well tuned of hotplug, so I'm not sure how trivial it is.  Thanks,

Alex

> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > 
> >  hw/ivshmem.c |   10 +++-------
> >  1 file changed, 3 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> > index 05559b6..8b49eee 100644
> > --- a/hw/ivshmem.c
> > +++ b/hw/ivshmem.c
> > @@ -70,7 +70,6 @@ typedef struct IVShmemState {
> >       */
> >      MemoryRegion bar;
> >      MemoryRegion ivshmem;
> > -    MemoryRegion msix_bar;
> >      uint64_t ivshmem_size; /* size of shared memory region */
> >      int shm_fd; /* shared memory file descriptor */
> >  
> > @@ -563,16 +562,13 @@ static uint64_t ivshmem_get_size(IVShmemState * s) {
> >  
> >  static void ivshmem_setup_msi(IVShmemState * s)
> >  {
> > -    memory_region_init(&s->msix_bar, "ivshmem-msix", 4096);
> > -    if (!msix_init(&s->dev, s->vectors, &s->msix_bar, 1, 0)) {
> > -        pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
> > -                         &s->msix_bar);
> > -        IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
> > -    } else {
> > +    if (msix_init_exclusive_bar(&s->dev, s->vectors, 1)) {
> >          IVSHMEM_DPRINTF("msix initialization failed\n");
> >          exit(1);
> >      }
> >  
> > +    IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
> > +
> >      /* allocate QEMU char devices for receiving interrupts */
> >      s->eventfd_table = g_malloc0(s->vectors * sizeof(EventfdEntry));
> >  
> > 
> 

[Qemu-devel] [PATCH v2 3/6] virtio: Convert to msix_init_exclusive_bar() interface

[Alex Williamson]

Simple conversion.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/virtio-pci.c |   15 +++++----------
 hw/virtio-pci.h |    1 -
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 9342eed..3dca37f 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -782,13 +782,10 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
     pci_set_word(config + PCI_SUBSYSTEM_ID, vdev->device_id);
     config[PCI_INTERRUPT_PIN] = 1;
 
-    memory_region_init(&proxy->msix_bar, "virtio-msix", 4096);
-    if (vdev->nvectors && !msix_init(&proxy->pci_dev, vdev->nvectors,
-                                     &proxy->msix_bar, 1, 0)) {
-        pci_register_bar(&proxy->pci_dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
-                         &proxy->msix_bar);
-    } else
+    if (vdev->nvectors &&
+        msix_init_exclusive_bar(&proxy->pci_dev, vdev->nvectors, 1)) {
         vdev->nvectors = 0;
+    }
 
     proxy->pci_dev.config_write = virtio_write_config;
 
@@ -834,12 +831,10 @@ static int virtio_blk_init_pci(PCIDevice *pci_dev)
 static int virtio_exit_pci(PCIDevice *pci_dev)
 {
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
-    int r;
 
     memory_region_destroy(&proxy->bar);
-    r = msix_uninit(pci_dev, &proxy->msix_bar);
-    memory_region_destroy(&proxy->msix_bar);
-    return r;
+    msix_uninit_exclusive_bar(pci_dev);
+    return 0;
 }
 
 static int virtio_blk_exit_pci(PCIDevice *pci_dev)
diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
index 91b791b..ac9d522 100644
--- a/hw/virtio-pci.h
+++ b/hw/virtio-pci.h
@@ -34,7 +34,6 @@ typedef struct {
     PCIDevice pci_dev;
     VirtIODevice *vdev;
     MemoryRegion bar;
-    MemoryRegion msix_bar;
     uint32_t flags;
     uint32_t class_code;
     uint32_t nvectors;

[Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion

[Alex Williamson]

These don't have to be contiguous.  Size them to only what
they need and use separate MemoryRegions for the vector
table and PBA.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/msix.c |  116 ++++++++++++++++++++++++++++++++++++++-----------------------
 hw/pci.h  |   15 ++++++--
 2 files changed, 83 insertions(+), 48 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index a4cdfb0..d476d07 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -37,7 +37,7 @@
 
 static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
 {
-    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+    uint8_t *table_entry = dev->msix_table + vector * PCI_MSIX_ENTRY_SIZE;
     MSIMessage msg;
 
     msg.address = pci_get_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR);
@@ -86,16 +86,6 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
     return 0;
 }
 
-static uint64_t msix_mmio_read(void *opaque, target_phys_addr_t addr,
-                               unsigned size)
-{
-    PCIDevice *dev = opaque;
-    unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
-    void *page = dev->msix_table_page;
-
-    return pci_get_long(page + offset);
-}
-
 static uint8_t msix_pending_mask(int vector)
 {
     return 1 << (vector % 8);
@@ -103,7 +93,7 @@ static uint8_t msix_pending_mask(int vector)
 
 static uint8_t *msix_pending_byte(PCIDevice *dev, int vector)
 {
-    return dev->msix_table_page + MSIX_PAGE_PENDING + vector / 8;
+    return dev->msix_pba + vector / 8;
 }
 
 static int msix_is_pending(PCIDevice *dev, int vector)
@@ -124,7 +114,7 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
 static bool msix_vector_masked(PCIDevice *dev, int vector, bool fmask)
 {
     unsigned offset = vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
-    return fmask || dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
+    return fmask || dev->msix_table[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
 }
 
 static bool msix_is_masked(PCIDevice *dev, int vector)
@@ -203,27 +193,46 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
     }
 }
 
-static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
-                            uint64_t val, unsigned size)
+static uint64_t msix_table_mmio_read(void *opaque, target_phys_addr_t addr,
+                                     unsigned size)
 {
     PCIDevice *dev = opaque;
-    unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
-    int vector = offset / PCI_MSIX_ENTRY_SIZE;
-    bool was_masked;
 
-    /* MSI-X page includes a read-only PBA and a writeable Vector Control. */
-    if (vector >= dev->msix_entries_nr) {
-        return;
-    }
+    return pci_get_long(dev->msix_table + addr);
+}
+
+static void msix_table_mmio_write(void *opaque, target_phys_addr_t addr,
+                                  uint64_t val, unsigned size)
+{
+    PCIDevice *dev = opaque;
+    int vector = addr / PCI_MSIX_ENTRY_SIZE;
+    bool was_masked;
 
     was_masked = msix_is_masked(dev, vector);
-    pci_set_long(dev->msix_table_page + offset, val);
+    pci_set_long(dev->msix_table + addr, val);
     msix_handle_mask_update(dev, vector, was_masked);
 }
 
-static const MemoryRegionOps msix_mmio_ops = {
-    .read = msix_mmio_read,
-    .write = msix_mmio_write,
+static const MemoryRegionOps msix_table_mmio_ops = {
+    .read = msix_table_mmio_read,
+    .write = msix_table_mmio_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
+static uint64_t msix_pba_mmio_read(void *opaque, target_phys_addr_t addr,
+                                   unsigned size)
+{
+    PCIDevice *dev = opaque;
+
+    return pci_get_long(dev->msix_pba + addr);
+}
+
+static const MemoryRegionOps msix_pba_mmio_ops = {
+    .read = msix_pba_mmio_read,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .valid = {
         .min_access_size = 4,
@@ -235,11 +244,14 @@ static void msix_mmio_setup(PCIDevice *d, MemoryRegion *bar)
 {
     uint8_t *config = d->config + d->msix_cap;
     uint32_t table = pci_get_long(config + PCI_MSIX_TABLE);
-    uint32_t offset = table & ~(MSIX_PAGE_SIZE - 1);
+    uint32_t table_offset = table & ~PCI_MSIX_FLAGS_BIRMASK;
+    uint32_t pba = pci_get_long(config + PCI_MSIX_PBA);
+    uint32_t pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
     /* TODO: for assigned devices, we'll want to make it possible to map
      * pending bits separately in case they are in a separate bar. */
 
-    memory_region_add_subregion(bar, offset, &d->msix_mmio);
+    memory_region_add_subregion(bar, table_offset, &d->msix_table_mmio);
+    memory_region_add_subregion(bar, pba_offset, &d->msix_pba_mmio);
 }
 
 static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
@@ -251,7 +263,7 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
             vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
         bool was_masked = msix_is_masked(dev, vector);
 
-        dev->msix_table_page[offset] |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
+        dev->msix_table[offset] |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
         msix_handle_mask_update(dev, vector, was_masked);
     }
 }
@@ -263,6 +275,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
               unsigned bar_nr, unsigned bar_size)
 {
     int ret;
+    unsigned table_size, pba_size;
 
     /* Nothing to do if MSI is not supported by interrupt controller */
     if (!msi_supported) {
@@ -271,14 +284,20 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
     if (nentries > MSIX_MAX_ENTRIES)
         return -EINVAL;
 
+    table_size = nentries * PCI_MSIX_ENTRY_SIZE;
+    pba_size = QEMU_ALIGN_UP(nentries, 64) / 8;
+
     dev->msix_entry_used = g_malloc0(MSIX_MAX_ENTRIES *
                                         sizeof *dev->msix_entry_used);
 
-    dev->msix_table_page = g_malloc0(MSIX_PAGE_SIZE);
+    dev->msix_table = g_malloc0(table_size);
+    dev->msix_pba = g_malloc0(pba_size);
     msix_mask_all(dev, nentries);
 
-    memory_region_init_io(&dev->msix_mmio, &msix_mmio_ops, dev,
-                          "msix", MSIX_PAGE_SIZE);
+    memory_region_init_io(&dev->msix_table_mmio, &msix_table_mmio_ops, dev,
+                          "msix", table_size);
+    memory_region_init_io(&dev->msix_pba_mmio, &msix_pba_mmio_ops, dev,
+                          "msix-pba", pba_size);
 
     dev->msix_entries_nr = nentries;
     ret = msix_add_config(dev, nentries, bar_nr, bar_size);
@@ -291,9 +310,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
 
 err_config:
     dev->msix_entries_nr = 0;
-    memory_region_destroy(&dev->msix_mmio);
-    g_free(dev->msix_table_page);
-    dev->msix_table_page = NULL;
+    memory_region_destroy(&dev->msix_pba_mmio);
+    g_free(dev->msix_pba);
+    dev->msix_pba = NULL;
+    memory_region_destroy(&dev->msix_table_mmio);
+    g_free(dev->msix_table);
+    dev->msix_table = NULL;
     g_free(dev->msix_entry_used);
     dev->msix_entry_used = NULL;
     return ret;
@@ -354,10 +376,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
     dev->msix_cap = 0;
     msix_free_irq_entries(dev);
     dev->msix_entries_nr = 0;
-    memory_region_del_subregion(bar, &dev->msix_mmio);
-    memory_region_destroy(&dev->msix_mmio);
-    g_free(dev->msix_table_page);
-    dev->msix_table_page = NULL;
+    memory_region_del_subregion(bar, &dev->msix_pba_mmio);
+    memory_region_destroy(&dev->msix_pba_mmio);
+    g_free(dev->msix_pba);
+    dev->msix_pba = NULL;
+    memory_region_del_subregion(bar, &dev->msix_table_mmio);
+    memory_region_destroy(&dev->msix_table_mmio);
+    g_free(dev->msix_table);
+    dev->msix_table = NULL;
     g_free(dev->msix_entry_used);
     dev->msix_entry_used = NULL;
     dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
@@ -380,8 +406,8 @@ void msix_save(PCIDevice *dev, QEMUFile *f)
         return;
     }
 
-    qemu_put_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_SIZE);
-    qemu_put_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
+    qemu_put_buffer(f, dev->msix_table, n * PCI_MSIX_ENTRY_SIZE);
+    qemu_put_buffer(f, dev->msix_pba, (n + 7) / 8);
 }
 
 /* Should be called after restoring the config space. */
@@ -395,8 +421,8 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
     }
 
     msix_free_irq_entries(dev);
-    qemu_get_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_SIZE);
-    qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
+    qemu_get_buffer(f, dev->msix_table, n * PCI_MSIX_ENTRY_SIZE);
+    qemu_get_buffer(f, dev->msix_pba, (n + 7) / 8);
     msix_update_function_masked(dev);
 
     for (vector = 0; vector < n; vector++) {
@@ -443,7 +469,9 @@ void msix_reset(PCIDevice *dev)
     msix_free_irq_entries(dev);
     dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
 	    ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
-    memset(dev->msix_table_page, 0, MSIX_PAGE_SIZE);
+
+    memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE);
+    memset(dev->msix_pba, 0, QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8);
     msix_mask_all(dev, dev->msix_entries_nr);
 }
 
diff --git a/hw/pci.h b/hw/pci.h
index d517a54..bd64445 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -224,16 +224,23 @@ struct PCIDevice {
     /* MSI-X entries */
     int msix_entries_nr;
 
-    /* Space to store MSIX table */
-    uint8_t *msix_table_page;
+    /* Space to store MSIX table & pending bit array */
+    uint8_t *msix_table;
+    uint8_t *msix_pba;
+
     /* MemoryRegion container for msix exclusive BAR setup */
     MemoryRegion msix_exclusive_bar;
-    /* MMIO index used to map MSIX table and pending bit entries. */
-    MemoryRegion msix_mmio;
+
+    /* Memory Regions for MSIX table and pending bit entries. */
+    MemoryRegion msix_table_mmio;
+    MemoryRegion msix_pba_mmio;
+
     /* Reference-count for entries actually in use by driver. */
     unsigned *msix_entry_used;
+
     /* MSIX function mask set or MSIX disabled */
     bool msix_function_masked;
+
     /* Version id needed for VMState */
     int32_t version_id;
 

Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion

[Jan Kiszka]

On 2012-06-14 06:51, Alex Williamson wrote:
> These don't have to be contiguous.  Size them to only what
> they need and use separate MemoryRegions for the vector
> table and PBA.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
>  hw/msix.c |  116 ++++++++++++++++++++++++++++++++++++++-----------------------
>  hw/pci.h  |   15 ++++++--
>  2 files changed, 83 insertions(+), 48 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index a4cdfb0..d476d07 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -37,7 +37,7 @@
>  
>  static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
>  {
> -    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
> +    uint8_t *table_entry = dev->msix_table + vector * PCI_MSIX_ENTRY_SIZE;
>      MSIMessage msg;
>  
>      msg.address = pci_get_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR);
> @@ -86,16 +86,6 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
>      return 0;
>  }
>  
> -static uint64_t msix_mmio_read(void *opaque, target_phys_addr_t addr,
> -                               unsigned size)
> -{
> -    PCIDevice *dev = opaque;
> -    unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
> -    void *page = dev->msix_table_page;
> -
> -    return pci_get_long(page + offset);
> -}
> -
>  static uint8_t msix_pending_mask(int vector)
>  {
>      return 1 << (vector % 8);
> @@ -103,7 +93,7 @@ static uint8_t msix_pending_mask(int vector)
>  
>  static uint8_t *msix_pending_byte(PCIDevice *dev, int vector)
>  {
> -    return dev->msix_table_page + MSIX_PAGE_PENDING + vector / 8;
> +    return dev->msix_pba + vector / 8;
>  }
>  
>  static int msix_is_pending(PCIDevice *dev, int vector)
> @@ -124,7 +114,7 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
>  static bool msix_vector_masked(PCIDevice *dev, int vector, bool fmask)
>  {
>      unsigned offset = vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
> -    return fmask || dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
> +    return fmask || dev->msix_table[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
>  }
>  
>  static bool msix_is_masked(PCIDevice *dev, int vector)
> @@ -203,27 +193,46 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
>      }
>  }
>  
> -static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
> -                            uint64_t val, unsigned size)
> +static uint64_t msix_table_mmio_read(void *opaque, target_phys_addr_t addr,
> +                                     unsigned size)
>  {
>      PCIDevice *dev = opaque;
> -    unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
> -    int vector = offset / PCI_MSIX_ENTRY_SIZE;
> -    bool was_masked;
>  
> -    /* MSI-X page includes a read-only PBA and a writeable Vector Control. */
> -    if (vector >= dev->msix_entries_nr) {
> -        return;
> -    }
> +    return pci_get_long(dev->msix_table + addr);
> +}
> +
> +static void msix_table_mmio_write(void *opaque, target_phys_addr_t addr,
> +                                  uint64_t val, unsigned size)
> +{
> +    PCIDevice *dev = opaque;
> +    int vector = addr / PCI_MSIX_ENTRY_SIZE;
> +    bool was_masked;
>  
>      was_masked = msix_is_masked(dev, vector);
> -    pci_set_long(dev->msix_table_page + offset, val);
> +    pci_set_long(dev->msix_table + addr, val);
>      msix_handle_mask_update(dev, vector, was_masked);
>  }
>  
> -static const MemoryRegionOps msix_mmio_ops = {
> -    .read = msix_mmio_read,
> -    .write = msix_mmio_write,
> +static const MemoryRegionOps msix_table_mmio_ops = {
> +    .read = msix_table_mmio_read,
> +    .write = msix_table_mmio_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +};
> +
> +static uint64_t msix_pba_mmio_read(void *opaque, target_phys_addr_t addr,
> +                                   unsigned size)
> +{
> +    PCIDevice *dev = opaque;
> +
> +    return pci_get_long(dev->msix_pba + addr);
> +}
> +
> +static const MemoryRegionOps msix_pba_mmio_ops = {
> +    .read = msix_pba_mmio_read,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>      .valid = {
>          .min_access_size = 4,
> @@ -235,11 +244,14 @@ static void msix_mmio_setup(PCIDevice *d, MemoryRegion *bar)
>  {
>      uint8_t *config = d->config + d->msix_cap;
>      uint32_t table = pci_get_long(config + PCI_MSIX_TABLE);
> -    uint32_t offset = table & ~(MSIX_PAGE_SIZE - 1);
> +    uint32_t table_offset = table & ~PCI_MSIX_FLAGS_BIRMASK;
> +    uint32_t pba = pci_get_long(config + PCI_MSIX_PBA);
> +    uint32_t pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
>      /* TODO: for assigned devices, we'll want to make it possible to map
>       * pending bits separately in case they are in a separate bar. */
>  
> -    memory_region_add_subregion(bar, offset, &d->msix_mmio);
> +    memory_region_add_subregion(bar, table_offset, &d->msix_table_mmio);
> +    memory_region_add_subregion(bar, pba_offset, &d->msix_pba_mmio);
>  }
>  
>  static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
> @@ -251,7 +263,7 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>              vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
>          bool was_masked = msix_is_masked(dev, vector);
>  
> -        dev->msix_table_page[offset] |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
> +        dev->msix_table[offset] |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
>          msix_handle_mask_update(dev, vector, was_masked);
>      }
>  }
> @@ -263,6 +275,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>                unsigned bar_nr, unsigned bar_size)
>  {
>      int ret;
> +    unsigned table_size, pba_size;
>  
>      /* Nothing to do if MSI is not supported by interrupt controller */
>      if (!msi_supported) {
> @@ -271,14 +284,20 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>      if (nentries > MSIX_MAX_ENTRIES)
>          return -EINVAL;
>  
> +    table_size = nentries * PCI_MSIX_ENTRY_SIZE;
> +    pba_size = QEMU_ALIGN_UP(nentries, 64) / 8;
> +
>      dev->msix_entry_used = g_malloc0(MSIX_MAX_ENTRIES *
>                                          sizeof *dev->msix_entry_used);
>  
> -    dev->msix_table_page = g_malloc0(MSIX_PAGE_SIZE);
> +    dev->msix_table = g_malloc0(table_size);
> +    dev->msix_pba = g_malloc0(pba_size);
>      msix_mask_all(dev, nentries);
>  
> -    memory_region_init_io(&dev->msix_mmio, &msix_mmio_ops, dev,
> -                          "msix", MSIX_PAGE_SIZE);
> +    memory_region_init_io(&dev->msix_table_mmio, &msix_table_mmio_ops, dev,
> +                          "msix", table_size);

"msix-table" (vs. "msix-pba")? Only if you have to repost anyway.

> +    memory_region_init_io(&dev->msix_pba_mmio, &msix_pba_mmio_ops, dev,
> +                          "msix-pba", pba_size);
>  
>      dev->msix_entries_nr = nentries;
>      ret = msix_add_config(dev, nentries, bar_nr, bar_size);

Jan

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

Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion

[Alex Williamson]

On Thu, 2012-06-14 at 08:13 +0200, Jan Kiszka wrote:
> On 2012-06-14 06:51, Alex Williamson wrote:
> >  
> > -    memory_region_init_io(&dev->msix_mmio, &msix_mmio_ops, dev,
> > -                          "msix", MSIX_PAGE_SIZE);
> > +    memory_region_init_io(&dev->msix_table_mmio, &msix_table_mmio_ops, dev,
> > +                          "msix", table_size);
> 
> "msix-table" (vs. "msix-pba")? Only if you have to repost anyway.

Ok.  Thanks,

Alex

Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion

[Michael S. Tsirkin]

On Wed, Jun 13, 2012 at 10:51:47PM -0600, Alex Williamson wrote:
> These don't have to be contiguous.  Size them to only what
> they need and use separate MemoryRegions for the vector
> table and PBA.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Why is this still using NATIVE?

> ---
> 
>  hw/msix.c |  116 ++++++++++++++++++++++++++++++++++++++-----------------------
>  hw/pci.h  |   15 ++++++--
>  2 files changed, 83 insertions(+), 48 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index a4cdfb0..d476d07 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -37,7 +37,7 @@
>  
>  static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
>  {
> -    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
> +    uint8_t *table_entry = dev->msix_table + vector * PCI_MSIX_ENTRY_SIZE;
>      MSIMessage msg;
>  
>      msg.address = pci_get_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR);
> @@ -86,16 +86,6 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
>      return 0;
>  }
>  
> -static uint64_t msix_mmio_read(void *opaque, target_phys_addr_t addr,
> -                               unsigned size)
> -{
> -    PCIDevice *dev = opaque;
> -    unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
> -    void *page = dev->msix_table_page;
> -
> -    return pci_get_long(page + offset);
> -}
> -
>  static uint8_t msix_pending_mask(int vector)
>  {
>      return 1 << (vector % 8);
> @@ -103,7 +93,7 @@ static uint8_t msix_pending_mask(int vector)
>  
>  static uint8_t *msix_pending_byte(PCIDevice *dev, int vector)
>  {
> -    return dev->msix_table_page + MSIX_PAGE_PENDING + vector / 8;
> +    return dev->msix_pba + vector / 8;
>  }
>  
>  static int msix_is_pending(PCIDevice *dev, int vector)
> @@ -124,7 +114,7 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
>  static bool msix_vector_masked(PCIDevice *dev, int vector, bool fmask)
>  {
>      unsigned offset = vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
> -    return fmask || dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
> +    return fmask || dev->msix_table[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
>  }
>  
>  static bool msix_is_masked(PCIDevice *dev, int vector)
> @@ -203,27 +193,46 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
>      }
>  }
>  
> -static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
> -                            uint64_t val, unsigned size)
> +static uint64_t msix_table_mmio_read(void *opaque, target_phys_addr_t addr,
> +                                     unsigned size)
>  {
>      PCIDevice *dev = opaque;
> -    unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
> -    int vector = offset / PCI_MSIX_ENTRY_SIZE;
> -    bool was_masked;
>  
> -    /* MSI-X page includes a read-only PBA and a writeable Vector Control. */
> -    if (vector >= dev->msix_entries_nr) {
> -        return;
> -    }
> +    return pci_get_long(dev->msix_table + addr);
> +}
> +
> +static void msix_table_mmio_write(void *opaque, target_phys_addr_t addr,
> +                                  uint64_t val, unsigned size)
> +{
> +    PCIDevice *dev = opaque;
> +    int vector = addr / PCI_MSIX_ENTRY_SIZE;
> +    bool was_masked;
>  
>      was_masked = msix_is_masked(dev, vector);
> -    pci_set_long(dev->msix_table_page + offset, val);
> +    pci_set_long(dev->msix_table + addr, val);
>      msix_handle_mask_update(dev, vector, was_masked);
>  }
>  
> -static const MemoryRegionOps msix_mmio_ops = {
> -    .read = msix_mmio_read,
> -    .write = msix_mmio_write,
> +static const MemoryRegionOps msix_table_mmio_ops = {
> +    .read = msix_table_mmio_read,
> +    .write = msix_table_mmio_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +};
> +
> +static uint64_t msix_pba_mmio_read(void *opaque, target_phys_addr_t addr,
> +                                   unsigned size)
> +{
> +    PCIDevice *dev = opaque;
> +
> +    return pci_get_long(dev->msix_pba + addr);
> +}
> +
> +static const MemoryRegionOps msix_pba_mmio_ops = {
> +    .read = msix_pba_mmio_read,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>      .valid = {
>          .min_access_size = 4,
> @@ -235,11 +244,14 @@ static void msix_mmio_setup(PCIDevice *d, MemoryRegion *bar)
>  {
>      uint8_t *config = d->config + d->msix_cap;
>      uint32_t table = pci_get_long(config + PCI_MSIX_TABLE);
> -    uint32_t offset = table & ~(MSIX_PAGE_SIZE - 1);
> +    uint32_t table_offset = table & ~PCI_MSIX_FLAGS_BIRMASK;
> +    uint32_t pba = pci_get_long(config + PCI_MSIX_PBA);
> +    uint32_t pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
>      /* TODO: for assigned devices, we'll want to make it possible to map
>       * pending bits separately in case they are in a separate bar. */
>  
> -    memory_region_add_subregion(bar, offset, &d->msix_mmio);
> +    memory_region_add_subregion(bar, table_offset, &d->msix_table_mmio);
> +    memory_region_add_subregion(bar, pba_offset, &d->msix_pba_mmio);
>  }
>  
>  static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
> @@ -251,7 +263,7 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>              vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
>          bool was_masked = msix_is_masked(dev, vector);
>  
> -        dev->msix_table_page[offset] |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
> +        dev->msix_table[offset] |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
>          msix_handle_mask_update(dev, vector, was_masked);
>      }
>  }
> @@ -263,6 +275,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>                unsigned bar_nr, unsigned bar_size)
>  {
>      int ret;
> +    unsigned table_size, pba_size;
>  
>      /* Nothing to do if MSI is not supported by interrupt controller */
>      if (!msi_supported) {
> @@ -271,14 +284,20 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>      if (nentries > MSIX_MAX_ENTRIES)
>          return -EINVAL;
>  
> +    table_size = nentries * PCI_MSIX_ENTRY_SIZE;
> +    pba_size = QEMU_ALIGN_UP(nentries, 64) / 8;
> +
>      dev->msix_entry_used = g_malloc0(MSIX_MAX_ENTRIES *
>                                          sizeof *dev->msix_entry_used);
>  
> -    dev->msix_table_page = g_malloc0(MSIX_PAGE_SIZE);
> +    dev->msix_table = g_malloc0(table_size);
> +    dev->msix_pba = g_malloc0(pba_size);
>      msix_mask_all(dev, nentries);
>  
> -    memory_region_init_io(&dev->msix_mmio, &msix_mmio_ops, dev,
> -                          "msix", MSIX_PAGE_SIZE);
> +    memory_region_init_io(&dev->msix_table_mmio, &msix_table_mmio_ops, dev,
> +                          "msix", table_size);
> +    memory_region_init_io(&dev->msix_pba_mmio, &msix_pba_mmio_ops, dev,
> +                          "msix-pba", pba_size);
>  
>      dev->msix_entries_nr = nentries;
>      ret = msix_add_config(dev, nentries, bar_nr, bar_size);
> @@ -291,9 +310,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>  
>  err_config:
>      dev->msix_entries_nr = 0;
> -    memory_region_destroy(&dev->msix_mmio);
> -    g_free(dev->msix_table_page);
> -    dev->msix_table_page = NULL;
> +    memory_region_destroy(&dev->msix_pba_mmio);
> +    g_free(dev->msix_pba);
> +    dev->msix_pba = NULL;
> +    memory_region_destroy(&dev->msix_table_mmio);
> +    g_free(dev->msix_table);
> +    dev->msix_table = NULL;
>      g_free(dev->msix_entry_used);
>      dev->msix_entry_used = NULL;
>      return ret;
> @@ -354,10 +376,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
>      dev->msix_cap = 0;
>      msix_free_irq_entries(dev);
>      dev->msix_entries_nr = 0;
> -    memory_region_del_subregion(bar, &dev->msix_mmio);
> -    memory_region_destroy(&dev->msix_mmio);
> -    g_free(dev->msix_table_page);
> -    dev->msix_table_page = NULL;
> +    memory_region_del_subregion(bar, &dev->msix_pba_mmio);
> +    memory_region_destroy(&dev->msix_pba_mmio);
> +    g_free(dev->msix_pba);
> +    dev->msix_pba = NULL;
> +    memory_region_del_subregion(bar, &dev->msix_table_mmio);
> +    memory_region_destroy(&dev->msix_table_mmio);
> +    g_free(dev->msix_table);
> +    dev->msix_table = NULL;
>      g_free(dev->msix_entry_used);
>      dev->msix_entry_used = NULL;
>      dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> @@ -380,8 +406,8 @@ void msix_save(PCIDevice *dev, QEMUFile *f)
>          return;
>      }
>  
> -    qemu_put_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_SIZE);
> -    qemu_put_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
> +    qemu_put_buffer(f, dev->msix_table, n * PCI_MSIX_ENTRY_SIZE);
> +    qemu_put_buffer(f, dev->msix_pba, (n + 7) / 8);
>  }
>  
>  /* Should be called after restoring the config space. */
> @@ -395,8 +421,8 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
>      }
>  
>      msix_free_irq_entries(dev);
> -    qemu_get_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_SIZE);
> -    qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
> +    qemu_get_buffer(f, dev->msix_table, n * PCI_MSIX_ENTRY_SIZE);
> +    qemu_get_buffer(f, dev->msix_pba, (n + 7) / 8);
>      msix_update_function_masked(dev);
>  
>      for (vector = 0; vector < n; vector++) {
> @@ -443,7 +469,9 @@ void msix_reset(PCIDevice *dev)
>      msix_free_irq_entries(dev);
>      dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
>  	    ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
> -    memset(dev->msix_table_page, 0, MSIX_PAGE_SIZE);
> +
> +    memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE);
> +    memset(dev->msix_pba, 0, QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8);
>      msix_mask_all(dev, dev->msix_entries_nr);
>  }
>  
> diff --git a/hw/pci.h b/hw/pci.h
> index d517a54..bd64445 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -224,16 +224,23 @@ struct PCIDevice {
>      /* MSI-X entries */
>      int msix_entries_nr;
>  
> -    /* Space to store MSIX table */
> -    uint8_t *msix_table_page;
> +    /* Space to store MSIX table & pending bit array */
> +    uint8_t *msix_table;
> +    uint8_t *msix_pba;
> +
>      /* MemoryRegion container for msix exclusive BAR setup */
>      MemoryRegion msix_exclusive_bar;
> -    /* MMIO index used to map MSIX table and pending bit entries. */
> -    MemoryRegion msix_mmio;
> +
> +    /* Memory Regions for MSIX table and pending bit entries. */
> +    MemoryRegion msix_table_mmio;
> +    MemoryRegion msix_pba_mmio;
> +
>      /* Reference-count for entries actually in use by driver. */
>      unsigned *msix_entry_used;
> +
>      /* MSIX function mask set or MSIX disabled */
>      bool msix_function_masked;
> +
>      /* Version id needed for VMState */
>      int32_t version_id;
>  

Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion

[Alex Williamson]

On Thu, 2012-06-14 at 13:24 +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 13, 2012 at 10:51:47PM -0600, Alex Williamson wrote:
> > These don't have to be contiguous.  Size them to only what
> > they need and use separate MemoryRegions for the vector
> > table and PBA.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Why is this still using NATIVE?

Because the bug already exists, this patch doesn't make it worse, so at
best it's a tangentially related additional fix.  It may seem like a
s/NATIVE/LITTLE/ to you, but to me it's asking to completely scrub
msix.c for endian correctness.  Is this going to be the carrot you hold
out to accept the rest of the series?

Alex

> > ---
> > 
> >  hw/msix.c |  116 ++++++++++++++++++++++++++++++++++++++-----------------------
> >  hw/pci.h  |   15 ++++++--
> >  2 files changed, 83 insertions(+), 48 deletions(-)
> > 
> > diff --git a/hw/msix.c b/hw/msix.c
> > index a4cdfb0..d476d07 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -37,7 +37,7 @@
> >  
> >  static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
> >  {
> > -    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
> > +    uint8_t *table_entry = dev->msix_table + vector * PCI_MSIX_ENTRY_SIZE;
> >      MSIMessage msg;
> >  
> >      msg.address = pci_get_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR);
> > @@ -86,16 +86,6 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> >      return 0;
> >  }
> >  
> > -static uint64_t msix_mmio_read(void *opaque, target_phys_addr_t addr,
> > -                               unsigned size)
> > -{
> > -    PCIDevice *dev = opaque;
> > -    unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
> > -    void *page = dev->msix_table_page;
> > -
> > -    return pci_get_long(page + offset);
> > -}
> > -
> >  static uint8_t msix_pending_mask(int vector)
> >  {
> >      return 1 << (vector % 8);
> > @@ -103,7 +93,7 @@ static uint8_t msix_pending_mask(int vector)
> >  
> >  static uint8_t *msix_pending_byte(PCIDevice *dev, int vector)
> >  {
> > -    return dev->msix_table_page + MSIX_PAGE_PENDING + vector / 8;
> > +    return dev->msix_pba + vector / 8;
> >  }
> >  
> >  static int msix_is_pending(PCIDevice *dev, int vector)
> > @@ -124,7 +114,7 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
> >  static bool msix_vector_masked(PCIDevice *dev, int vector, bool fmask)
> >  {
> >      unsigned offset = vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
> > -    return fmask || dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > +    return fmask || dev->msix_table[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
> >  }
> >  
> >  static bool msix_is_masked(PCIDevice *dev, int vector)
> > @@ -203,27 +193,46 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
> >      }
> >  }
> >  
> > -static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
> > -                            uint64_t val, unsigned size)
> > +static uint64_t msix_table_mmio_read(void *opaque, target_phys_addr_t addr,
> > +                                     unsigned size)
> >  {
> >      PCIDevice *dev = opaque;
> > -    unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
> > -    int vector = offset / PCI_MSIX_ENTRY_SIZE;
> > -    bool was_masked;
> >  
> > -    /* MSI-X page includes a read-only PBA and a writeable Vector Control. */
> > -    if (vector >= dev->msix_entries_nr) {
> > -        return;
> > -    }
> > +    return pci_get_long(dev->msix_table + addr);
> > +}
> > +
> > +static void msix_table_mmio_write(void *opaque, target_phys_addr_t addr,
> > +                                  uint64_t val, unsigned size)
> > +{
> > +    PCIDevice *dev = opaque;
> > +    int vector = addr / PCI_MSIX_ENTRY_SIZE;
> > +    bool was_masked;
> >  
> >      was_masked = msix_is_masked(dev, vector);
> > -    pci_set_long(dev->msix_table_page + offset, val);
> > +    pci_set_long(dev->msix_table + addr, val);
> >      msix_handle_mask_update(dev, vector, was_masked);
> >  }
> >  
> > -static const MemoryRegionOps msix_mmio_ops = {
> > -    .read = msix_mmio_read,
> > -    .write = msix_mmio_write,
> > +static const MemoryRegionOps msix_table_mmio_ops = {
> > +    .read = msix_table_mmio_read,
> > +    .write = msix_table_mmio_write,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > +    .valid = {
> > +        .min_access_size = 4,
> > +        .max_access_size = 4,
> > +    },
> > +};
> > +
> > +static uint64_t msix_pba_mmio_read(void *opaque, target_phys_addr_t addr,
> > +                                   unsigned size)
> > +{
> > +    PCIDevice *dev = opaque;
> > +
> > +    return pci_get_long(dev->msix_pba + addr);
> > +}
> > +
> > +static const MemoryRegionOps msix_pba_mmio_ops = {
> > +    .read = msix_pba_mmio_read,
> >      .endianness = DEVICE_NATIVE_ENDIAN,
> >      .valid = {
> >          .min_access_size = 4,
> > @@ -235,11 +244,14 @@ static void msix_mmio_setup(PCIDevice *d, MemoryRegion *bar)
> >  {
> >      uint8_t *config = d->config + d->msix_cap;
> >      uint32_t table = pci_get_long(config + PCI_MSIX_TABLE);
> > -    uint32_t offset = table & ~(MSIX_PAGE_SIZE - 1);
> > +    uint32_t table_offset = table & ~PCI_MSIX_FLAGS_BIRMASK;
> > +    uint32_t pba = pci_get_long(config + PCI_MSIX_PBA);
> > +    uint32_t pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
> >      /* TODO: for assigned devices, we'll want to make it possible to map
> >       * pending bits separately in case they are in a separate bar. */
> >  
> > -    memory_region_add_subregion(bar, offset, &d->msix_mmio);
> > +    memory_region_add_subregion(bar, table_offset, &d->msix_table_mmio);
> > +    memory_region_add_subregion(bar, pba_offset, &d->msix_pba_mmio);
> >  }
> >  
> >  static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
> > @@ -251,7 +263,7 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
> >              vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
> >          bool was_masked = msix_is_masked(dev, vector);
> >  
> > -        dev->msix_table_page[offset] |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > +        dev->msix_table[offset] |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
> >          msix_handle_mask_update(dev, vector, was_masked);
> >      }
> >  }
> > @@ -263,6 +275,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> >                unsigned bar_nr, unsigned bar_size)
> >  {
> >      int ret;
> > +    unsigned table_size, pba_size;
> >  
> >      /* Nothing to do if MSI is not supported by interrupt controller */
> >      if (!msi_supported) {
> > @@ -271,14 +284,20 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> >      if (nentries > MSIX_MAX_ENTRIES)
> >          return -EINVAL;
> >  
> > +    table_size = nentries * PCI_MSIX_ENTRY_SIZE;
> > +    pba_size = QEMU_ALIGN_UP(nentries, 64) / 8;
> > +
> >      dev->msix_entry_used = g_malloc0(MSIX_MAX_ENTRIES *
> >                                          sizeof *dev->msix_entry_used);
> >  
> > -    dev->msix_table_page = g_malloc0(MSIX_PAGE_SIZE);
> > +    dev->msix_table = g_malloc0(table_size);
> > +    dev->msix_pba = g_malloc0(pba_size);
> >      msix_mask_all(dev, nentries);
> >  
> > -    memory_region_init_io(&dev->msix_mmio, &msix_mmio_ops, dev,
> > -                          "msix", MSIX_PAGE_SIZE);
> > +    memory_region_init_io(&dev->msix_table_mmio, &msix_table_mmio_ops, dev,
> > +                          "msix", table_size);
> > +    memory_region_init_io(&dev->msix_pba_mmio, &msix_pba_mmio_ops, dev,
> > +                          "msix-pba", pba_size);
> >  
> >      dev->msix_entries_nr = nentries;
> >      ret = msix_add_config(dev, nentries, bar_nr, bar_size);
> > @@ -291,9 +310,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> >  
> >  err_config:
> >      dev->msix_entries_nr = 0;
> > -    memory_region_destroy(&dev->msix_mmio);
> > -    g_free(dev->msix_table_page);
> > -    dev->msix_table_page = NULL;
> > +    memory_region_destroy(&dev->msix_pba_mmio);
> > +    g_free(dev->msix_pba);
> > +    dev->msix_pba = NULL;
> > +    memory_region_destroy(&dev->msix_table_mmio);
> > +    g_free(dev->msix_table);
> > +    dev->msix_table = NULL;
> >      g_free(dev->msix_entry_used);
> >      dev->msix_entry_used = NULL;
> >      return ret;
> > @@ -354,10 +376,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
> >      dev->msix_cap = 0;
> >      msix_free_irq_entries(dev);
> >      dev->msix_entries_nr = 0;
> > -    memory_region_del_subregion(bar, &dev->msix_mmio);
> > -    memory_region_destroy(&dev->msix_mmio);
> > -    g_free(dev->msix_table_page);
> > -    dev->msix_table_page = NULL;
> > +    memory_region_del_subregion(bar, &dev->msix_pba_mmio);
> > +    memory_region_destroy(&dev->msix_pba_mmio);
> > +    g_free(dev->msix_pba);
> > +    dev->msix_pba = NULL;
> > +    memory_region_del_subregion(bar, &dev->msix_table_mmio);
> > +    memory_region_destroy(&dev->msix_table_mmio);
> > +    g_free(dev->msix_table);
> > +    dev->msix_table = NULL;
> >      g_free(dev->msix_entry_used);
> >      dev->msix_entry_used = NULL;
> >      dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> > @@ -380,8 +406,8 @@ void msix_save(PCIDevice *dev, QEMUFile *f)
> >          return;
> >      }
> >  
> > -    qemu_put_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_SIZE);
> > -    qemu_put_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
> > +    qemu_put_buffer(f, dev->msix_table, n * PCI_MSIX_ENTRY_SIZE);
> > +    qemu_put_buffer(f, dev->msix_pba, (n + 7) / 8);
> >  }
> >  
> >  /* Should be called after restoring the config space. */
> > @@ -395,8 +421,8 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
> >      }
> >  
> >      msix_free_irq_entries(dev);
> > -    qemu_get_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_SIZE);
> > -    qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
> > +    qemu_get_buffer(f, dev->msix_table, n * PCI_MSIX_ENTRY_SIZE);
> > +    qemu_get_buffer(f, dev->msix_pba, (n + 7) / 8);
> >      msix_update_function_masked(dev);
> >  
> >      for (vector = 0; vector < n; vector++) {
> > @@ -443,7 +469,9 @@ void msix_reset(PCIDevice *dev)
> >      msix_free_irq_entries(dev);
> >      dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
> >  	    ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
> > -    memset(dev->msix_table_page, 0, MSIX_PAGE_SIZE);
> > +
> > +    memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE);
> > +    memset(dev->msix_pba, 0, QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8);
> >      msix_mask_all(dev, dev->msix_entries_nr);
> >  }
> >  
> > diff --git a/hw/pci.h b/hw/pci.h
> > index d517a54..bd64445 100644
> > --- a/hw/pci.h
> > +++ b/hw/pci.h
> > @@ -224,16 +224,23 @@ struct PCIDevice {
> >      /* MSI-X entries */
> >      int msix_entries_nr;
> >  
> > -    /* Space to store MSIX table */
> > -    uint8_t *msix_table_page;
> > +    /* Space to store MSIX table & pending bit array */
> > +    uint8_t *msix_table;
> > +    uint8_t *msix_pba;
> > +
> >      /* MemoryRegion container for msix exclusive BAR setup */
> >      MemoryRegion msix_exclusive_bar;
> > -    /* MMIO index used to map MSIX table and pending bit entries. */
> > -    MemoryRegion msix_mmio;
> > +
> > +    /* Memory Regions for MSIX table and pending bit entries. */
> > +    MemoryRegion msix_table_mmio;
> > +    MemoryRegion msix_pba_mmio;
> > +
> >      /* Reference-count for entries actually in use by driver. */
> >      unsigned *msix_entry_used;
> > +
> >      /* MSIX function mask set or MSIX disabled */
> >      bool msix_function_masked;
> > +
> >      /* Version id needed for VMState */
> >      int32_t version_id;
> >  

Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion

[Michael S. Tsirkin]

On Thu, Jun 14, 2012 at 08:21:39AM -0600, Alex Williamson wrote:
> On Thu, 2012-06-14 at 13:24 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 13, 2012 at 10:51:47PM -0600, Alex Williamson wrote:
> > > These don't have to be contiguous.  Size them to only what
> > > they need and use separate MemoryRegions for the vector
> > > table and PBA.
> > > 
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > 
> > Why is this still using NATIVE?
> 
> Because the bug already exists,

We have lots of broken code. The way progress happens here is
such code is in a kind of freeze until fixed. This way whoever needs new
features gets to fix the bugs too.

> this patch doesn't make it worse, so at best it's a tangentially related additional fix.
> It may seem like a s/NATIVE/LITTLE/ to you, but to me it's asking to completely scrub
> msix.c for endian correctness.  Is this going to be the carrot you hold
> out to accept the rest of the series?
> 
> Alex

Unfortunately no promises yet, and that is because you basically decided
to rewrite lots of code in your preferred style while also adding new functionality.
If changes were done in small steps, then I could apply things we can
agree on and defer the ones we don't.  Sometimes it's hard, but clearly
not in this case.

> > > ---
> > > 
> > >  hw/msix.c |  116 ++++++++++++++++++++++++++++++++++++++-----------------------
> > >  hw/pci.h  |   15 ++++++--
> > >  2 files changed, 83 insertions(+), 48 deletions(-)
> > > 
> > > diff --git a/hw/msix.c b/hw/msix.c
> > > index a4cdfb0..d476d07 100644
> > > --- a/hw/msix.c
> > > +++ b/hw/msix.c
> > > @@ -37,7 +37,7 @@
> > >  
> > >  static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
> > >  {
> > > -    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
> > > +    uint8_t *table_entry = dev->msix_table + vector * PCI_MSIX_ENTRY_SIZE;
> > >      MSIMessage msg;
> > >  
> > >      msg.address = pci_get_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR);
> > > @@ -86,16 +86,6 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> > >      return 0;
> > >  }
> > >  
> > > -static uint64_t msix_mmio_read(void *opaque, target_phys_addr_t addr,
> > > -                               unsigned size)
> > > -{
> > > -    PCIDevice *dev = opaque;
> > > -    unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
> > > -    void *page = dev->msix_table_page;
> > > -
> > > -    return pci_get_long(page + offset);
> > > -}
> > > -
> > >  static uint8_t msix_pending_mask(int vector)
> > >  {
> > >      return 1 << (vector % 8);
> > > @@ -103,7 +93,7 @@ static uint8_t msix_pending_mask(int vector)
> > >  
> > >  static uint8_t *msix_pending_byte(PCIDevice *dev, int vector)
> > >  {
> > > -    return dev->msix_table_page + MSIX_PAGE_PENDING + vector / 8;
> > > +    return dev->msix_pba + vector / 8;
> > >  }
> > >  
> > >  static int msix_is_pending(PCIDevice *dev, int vector)
> > > @@ -124,7 +114,7 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
> > >  static bool msix_vector_masked(PCIDevice *dev, int vector, bool fmask)
> > >  {
> > >      unsigned offset = vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
> > > -    return fmask || dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > > +    return fmask || dev->msix_table[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > >  }
> > >  
> > >  static bool msix_is_masked(PCIDevice *dev, int vector)
> > > @@ -203,27 +193,46 @@ void msix_write_config(PCIDevice *dev, uint32_t addr,
> > >      }
> > >  }
> > >  
> > > -static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
> > > -                            uint64_t val, unsigned size)
> > > +static uint64_t msix_table_mmio_read(void *opaque, target_phys_addr_t addr,
> > > +                                     unsigned size)
> > >  {
> > >      PCIDevice *dev = opaque;
> > > -    unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
> > > -    int vector = offset / PCI_MSIX_ENTRY_SIZE;
> > > -    bool was_masked;
> > >  
> > > -    /* MSI-X page includes a read-only PBA and a writeable Vector Control. */
> > > -    if (vector >= dev->msix_entries_nr) {
> > > -        return;
> > > -    }
> > > +    return pci_get_long(dev->msix_table + addr);
> > > +}
> > > +
> > > +static void msix_table_mmio_write(void *opaque, target_phys_addr_t addr,
> > > +                                  uint64_t val, unsigned size)
> > > +{
> > > +    PCIDevice *dev = opaque;
> > > +    int vector = addr / PCI_MSIX_ENTRY_SIZE;
> > > +    bool was_masked;
> > >  
> > >      was_masked = msix_is_masked(dev, vector);
> > > -    pci_set_long(dev->msix_table_page + offset, val);
> > > +    pci_set_long(dev->msix_table + addr, val);
> > >      msix_handle_mask_update(dev, vector, was_masked);
> > >  }
> > >  
> > > -static const MemoryRegionOps msix_mmio_ops = {
> > > -    .read = msix_mmio_read,
> > > -    .write = msix_mmio_write,
> > > +static const MemoryRegionOps msix_table_mmio_ops = {
> > > +    .read = msix_table_mmio_read,
> > > +    .write = msix_table_mmio_write,
> > > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > > +    .valid = {
> > > +        .min_access_size = 4,
> > > +        .max_access_size = 4,
> > > +    },
> > > +};
> > > +
> > > +static uint64_t msix_pba_mmio_read(void *opaque, target_phys_addr_t addr,
> > > +                                   unsigned size)
> > > +{
> > > +    PCIDevice *dev = opaque;
> > > +
> > > +    return pci_get_long(dev->msix_pba + addr);
> > > +}
> > > +
> > > +static const MemoryRegionOps msix_pba_mmio_ops = {
> > > +    .read = msix_pba_mmio_read,
> > >      .endianness = DEVICE_NATIVE_ENDIAN,
> > >      .valid = {
> > >          .min_access_size = 4,
> > > @@ -235,11 +244,14 @@ static void msix_mmio_setup(PCIDevice *d, MemoryRegion *bar)
> > >  {
> > >      uint8_t *config = d->config + d->msix_cap;
> > >      uint32_t table = pci_get_long(config + PCI_MSIX_TABLE);
> > > -    uint32_t offset = table & ~(MSIX_PAGE_SIZE - 1);
> > > +    uint32_t table_offset = table & ~PCI_MSIX_FLAGS_BIRMASK;
> > > +    uint32_t pba = pci_get_long(config + PCI_MSIX_PBA);
> > > +    uint32_t pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
> > >      /* TODO: for assigned devices, we'll want to make it possible to map
> > >       * pending bits separately in case they are in a separate bar. */
> > >  
> > > -    memory_region_add_subregion(bar, offset, &d->msix_mmio);
> > > +    memory_region_add_subregion(bar, table_offset, &d->msix_table_mmio);
> > > +    memory_region_add_subregion(bar, pba_offset, &d->msix_pba_mmio);
> > >  }
> > >  
> > >  static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
> > > @@ -251,7 +263,7 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
> > >              vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
> > >          bool was_masked = msix_is_masked(dev, vector);
> > >  
> > > -        dev->msix_table_page[offset] |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > > +        dev->msix_table[offset] |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
> > >          msix_handle_mask_update(dev, vector, was_masked);
> > >      }
> > >  }
> > > @@ -263,6 +275,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> > >                unsigned bar_nr, unsigned bar_size)
> > >  {
> > >      int ret;
> > > +    unsigned table_size, pba_size;
> > >  
> > >      /* Nothing to do if MSI is not supported by interrupt controller */
> > >      if (!msi_supported) {
> > > @@ -271,14 +284,20 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> > >      if (nentries > MSIX_MAX_ENTRIES)
> > >          return -EINVAL;
> > >  
> > > +    table_size = nentries * PCI_MSIX_ENTRY_SIZE;
> > > +    pba_size = QEMU_ALIGN_UP(nentries, 64) / 8;
> > > +
> > >      dev->msix_entry_used = g_malloc0(MSIX_MAX_ENTRIES *
> > >                                          sizeof *dev->msix_entry_used);
> > >  
> > > -    dev->msix_table_page = g_malloc0(MSIX_PAGE_SIZE);
> > > +    dev->msix_table = g_malloc0(table_size);
> > > +    dev->msix_pba = g_malloc0(pba_size);
> > >      msix_mask_all(dev, nentries);
> > >  
> > > -    memory_region_init_io(&dev->msix_mmio, &msix_mmio_ops, dev,
> > > -                          "msix", MSIX_PAGE_SIZE);
> > > +    memory_region_init_io(&dev->msix_table_mmio, &msix_table_mmio_ops, dev,
> > > +                          "msix", table_size);
> > > +    memory_region_init_io(&dev->msix_pba_mmio, &msix_pba_mmio_ops, dev,
> > > +                          "msix-pba", pba_size);
> > >  
> > >      dev->msix_entries_nr = nentries;
> > >      ret = msix_add_config(dev, nentries, bar_nr, bar_size);
> > > @@ -291,9 +310,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> > >  
> > >  err_config:
> > >      dev->msix_entries_nr = 0;
> > > -    memory_region_destroy(&dev->msix_mmio);
> > > -    g_free(dev->msix_table_page);
> > > -    dev->msix_table_page = NULL;
> > > +    memory_region_destroy(&dev->msix_pba_mmio);
> > > +    g_free(dev->msix_pba);
> > > +    dev->msix_pba = NULL;
> > > +    memory_region_destroy(&dev->msix_table_mmio);
> > > +    g_free(dev->msix_table);
> > > +    dev->msix_table = NULL;
> > >      g_free(dev->msix_entry_used);
> > >      dev->msix_entry_used = NULL;
> > >      return ret;
> > > @@ -354,10 +376,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
> > >      dev->msix_cap = 0;
> > >      msix_free_irq_entries(dev);
> > >      dev->msix_entries_nr = 0;
> > > -    memory_region_del_subregion(bar, &dev->msix_mmio);
> > > -    memory_region_destroy(&dev->msix_mmio);
> > > -    g_free(dev->msix_table_page);
> > > -    dev->msix_table_page = NULL;
> > > +    memory_region_del_subregion(bar, &dev->msix_pba_mmio);
> > > +    memory_region_destroy(&dev->msix_pba_mmio);
> > > +    g_free(dev->msix_pba);
> > > +    dev->msix_pba = NULL;
> > > +    memory_region_del_subregion(bar, &dev->msix_table_mmio);
> > > +    memory_region_destroy(&dev->msix_table_mmio);
> > > +    g_free(dev->msix_table);
> > > +    dev->msix_table = NULL;
> > >      g_free(dev->msix_entry_used);
> > >      dev->msix_entry_used = NULL;
> > >      dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> > > @@ -380,8 +406,8 @@ void msix_save(PCIDevice *dev, QEMUFile *f)
> > >          return;
> > >      }
> > >  
> > > -    qemu_put_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_SIZE);
> > > -    qemu_put_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
> > > +    qemu_put_buffer(f, dev->msix_table, n * PCI_MSIX_ENTRY_SIZE);
> > > +    qemu_put_buffer(f, dev->msix_pba, (n + 7) / 8);
> > >  }
> > >  
> > >  /* Should be called after restoring the config space. */
> > > @@ -395,8 +421,8 @@ void msix_load(PCIDevice *dev, QEMUFile *f)
> > >      }
> > >  
> > >      msix_free_irq_entries(dev);
> > > -    qemu_get_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_SIZE);
> > > -    qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
> > > +    qemu_get_buffer(f, dev->msix_table, n * PCI_MSIX_ENTRY_SIZE);
> > > +    qemu_get_buffer(f, dev->msix_pba, (n + 7) / 8);
> > >      msix_update_function_masked(dev);
> > >  
> > >      for (vector = 0; vector < n; vector++) {
> > > @@ -443,7 +469,9 @@ void msix_reset(PCIDevice *dev)
> > >      msix_free_irq_entries(dev);
> > >      dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
> > >  	    ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
> > > -    memset(dev->msix_table_page, 0, MSIX_PAGE_SIZE);
> > > +
> > > +    memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE);
> > > +    memset(dev->msix_pba, 0, QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8);
> > >      msix_mask_all(dev, dev->msix_entries_nr);
> > >  }
> > >  
> > > diff --git a/hw/pci.h b/hw/pci.h
> > > index d517a54..bd64445 100644
> > > --- a/hw/pci.h
> > > +++ b/hw/pci.h
> > > @@ -224,16 +224,23 @@ struct PCIDevice {
> > >      /* MSI-X entries */
> > >      int msix_entries_nr;
> > >  
> > > -    /* Space to store MSIX table */
> > > -    uint8_t *msix_table_page;
> > > +    /* Space to store MSIX table & pending bit array */
> > > +    uint8_t *msix_table;
> > > +    uint8_t *msix_pba;
> > > +
> > >      /* MemoryRegion container for msix exclusive BAR setup */
> > >      MemoryRegion msix_exclusive_bar;
> > > -    /* MMIO index used to map MSIX table and pending bit entries. */
> > > -    MemoryRegion msix_mmio;
> > > +
> > > +    /* Memory Regions for MSIX table and pending bit entries. */
> > > +    MemoryRegion msix_table_mmio;
> > > +    MemoryRegion msix_pba_mmio;
> > > +
> > >      /* Reference-count for entries actually in use by driver. */
> > >      unsigned *msix_entry_used;
> > > +
> > >      /* MSIX function mask set or MSIX disabled */
> > >      bool msix_function_masked;
> > > +
> > >      /* Version id needed for VMState */
> > >      int32_t version_id;
> > >  
> 
> 

Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion

[Alex Williamson]

On Thu, 2012-06-14 at 17:50 +0300, Michael S. Tsirkin wrote:
> On Thu, Jun 14, 2012 at 08:21:39AM -0600, Alex Williamson wrote:
> > On Thu, 2012-06-14 at 13:24 +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 13, 2012 at 10:51:47PM -0600, Alex Williamson wrote:
> > > > These don't have to be contiguous.  Size them to only what
> > > > they need and use separate MemoryRegions for the vector
> > > > table and PBA.
> > > > 
> > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > 
> > > Why is this still using NATIVE?
> > 
> > Because the bug already exists,
> 
> We have lots of broken code. The way progress happens here is
> such code is in a kind of freeze until fixed. This way whoever needs new
> features gets to fix the bugs too.

In other words, you impose a toll and inhibit forward progress until
someone fixes it?  I have no place telling you how to be a maintainer,
but I personally find that this style makes attempting to contribute
code to anything pci/msi/msix related a huge pain.  There are far too
many of these land mines in the code and simple fixes easily explode
into tangentially related changes off your todo list.

> > this patch doesn't make it worse, so at best it's a tangentially related additional fix.
> > It may seem like a s/NATIVE/LITTLE/ to you, but to me it's asking to completely scrub
> > msix.c for endian correctness.  Is this going to be the carrot you hold
> > out to accept the rest of the series?
> > 
> > Alex
> 
> Unfortunately no promises yet, and that is because you basically decided
> to rewrite lots of code in your preferred style while also adding new functionality.
> If changes were done in small steps, then I could apply things we can
> agree on and defer the ones we don't.  Sometimes it's hard, but clearly
> not in this case.

Patches can always be reduced into smaller changes, but at some point we
have to call it good enough.  I split one patch into 6 and thought that
did a pretty good job.  Should I remove everywhere that I've added a new
line to avoid imposing my style on the rest of the code?  The next
version will eliminate the add_config move thanks to Jan's constructive
suggestion, so I hope it meets your standards.  Thanks,

Alex

Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion

[Michael S. Tsirkin]

On Thu, Jun 14, 2012 at 09:09:47AM -0600, Alex Williamson wrote:
> On Thu, 2012-06-14 at 17:50 +0300, Michael S. Tsirkin wrote:
> > On Thu, Jun 14, 2012 at 08:21:39AM -0600, Alex Williamson wrote:
> > > On Thu, 2012-06-14 at 13:24 +0300, Michael S. Tsirkin wrote:
> > > > On Wed, Jun 13, 2012 at 10:51:47PM -0600, Alex Williamson wrote:
> > > > > These don't have to be contiguous.  Size them to only what
> > > > > they need and use separate MemoryRegions for the vector
> > > > > table and PBA.
> > > > > 
> > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > > 
> > > > Why is this still using NATIVE?
> > > 
> > > Because the bug already exists,
> > 
> > We have lots of broken code. The way progress happens here is
> > such code is in a kind of freeze until fixed. This way whoever needs new
> > features gets to fix the bugs too.
> 
> In other words, you impose a toll and inhibit forward progress until
> someone fixes it?  I have no place telling you how to be a maintainer,
> but I personally find that this style makes attempting to contribute
> code to anything pci/msi/msix related a huge pain.  There are far too
> many of these land mines in the code and simple fixes easily explode
> into tangentially related changes off your todo list.

I try to pick simple fixes up straight away. Pls try to keep the fixes
simpler :)

> > > this patch doesn't make it worse, so at best it's a tangentially related additional fix.
> > > It may seem like a s/NATIVE/LITTLE/ to you, but to me it's asking to completely scrub
> > > msix.c for endian correctness.  Is this going to be the carrot you hold
> > > out to accept the rest of the series?
> > > 
> > > Alex
> > 
> > Unfortunately no promises yet, and that is because you basically decided
> > to rewrite lots of code in your preferred style while also adding new functionality.
> > If changes were done in small steps, then I could apply things we can
> > agree on and defer the ones we don't.  Sometimes it's hard, but clearly
> > not in this case.
> 
> Patches can always be reduced into smaller changes, but at some point we
> have to call it good enough.  I split one patch into 6 and thought that
> did a pretty good job.

It's not the mechanical splitting of patches that is needed.
In one case you actually added a new function in place X then moved it
to place Y. And the new order does not make sense: init then uninit looks cleaner.


> Should I remove everywhere that I've added a new
> line to avoid imposing my style on the rest of the code?

Each new line? No, that would be taking it to extreme because newlines are
easy to ignore normally. Though if someone sends me a patch with 1000
newlines tweaked and functional changes in the same patch, I won't apply
it.

> The next
> version will eliminate the add_config move thanks to Jan's constructive
> suggestion, so I hope it meets your standards.  Thanks,
> 
> Alex

Please try to address other comments too, like naming
constants. I would hate to get another revision that just ignores them.

-- 
MST

Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion

[Alex Williamson]

On Thu, 2012-06-14 at 18:45 +0300, Michael S. Tsirkin wrote:
> On Thu, Jun 14, 2012 at 09:09:47AM -0600, Alex Williamson wrote:
> > On Thu, 2012-06-14 at 17:50 +0300, Michael S. Tsirkin wrote:
> > > On Thu, Jun 14, 2012 at 08:21:39AM -0600, Alex Williamson wrote:
> > > > On Thu, 2012-06-14 at 13:24 +0300, Michael S. Tsirkin wrote:
> > > > > On Wed, Jun 13, 2012 at 10:51:47PM -0600, Alex Williamson wrote:
> > > > > > These don't have to be contiguous.  Size them to only what
> > > > > > they need and use separate MemoryRegions for the vector
> > > > > > table and PBA.
> > > > > > 
> > > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > > > 
> > > > > Why is this still using NATIVE?
> > > > 
> > > > Because the bug already exists,
> > > 
> > > We have lots of broken code. The way progress happens here is
> > > such code is in a kind of freeze until fixed. This way whoever needs new
> > > features gets to fix the bugs too.
> > 
> > In other words, you impose a toll and inhibit forward progress until
> > someone fixes it?  I have no place telling you how to be a maintainer,
> > but I personally find that this style makes attempting to contribute
> > code to anything pci/msi/msix related a huge pain.  There are far too
> > many of these land mines in the code and simple fixes easily explode
> > into tangentially related changes off your todo list.
> 
> I try to pick simple fixes up straight away. Pls try to keep the fixes
> simpler :)

What does that have to do with shoving todo list items down the throats
of contributors?

> > > > this patch doesn't make it worse, so at best it's a tangentially related additional fix.
> > > > It may seem like a s/NATIVE/LITTLE/ to you, but to me it's asking to completely scrub
> > > > msix.c for endian correctness.  Is this going to be the carrot you hold
> > > > out to accept the rest of the series?
> > > > 
> > > > Alex
> > > 
> > > Unfortunately no promises yet, and that is because you basically decided
> > > to rewrite lots of code in your preferred style while also adding new functionality.
> > > If changes were done in small steps, then I could apply things we can
> > > agree on and defer the ones we don't.  Sometimes it's hard, but clearly
> > > not in this case.
> > 
> > Patches can always be reduced into smaller changes, but at some point we
> > have to call it good enough.  I split one patch into 6 and thought that
> > did a pretty good job.
> 
> It's not the mechanical splitting of patches that is needed.
> In one case you actually added a new function in place X then moved it
> to place Y. And the new order does not make sense: init then uninit looks cleaner.

uninit was moved because I was able to remove duplicate code by making
init call uninit on error.  Do you prefer a prototype to avoid code
moves in that case?  Doesn't matter now, it's fixed with Jan's
suggestion and I've already split the move of another tiny function to a
separate patch.

> > Should I remove everywhere that I've added a new
> > line to avoid imposing my style on the rest of the code?
> 
> Each new line? No, that would be taking it to extreme because newlines are
> easy to ignore normally. Though if someone sends me a patch with 1000
> newlines tweaked and functional changes in the same patch, I won't apply
> it.

Well then, I'm not sure what you mean by "you basically decided to
rewrite lots of code in your preferred style".

> > The next
> > version will eliminate the add_config move thanks to Jan's constructive
> > suggestion, so I hope it meets your standards.  Thanks,
> > 
> > Alex
> 
> Please try to address other comments too, like naming
> constants. I would hate to get another revision that just ignores them.

It will unless you counter my rebuttal to why I'm not using macros
there.  To repeat:

On Wed, 2012-06-13 at 17:05 -0600, Alex Williamson wrote:
On Wed, 2012-06-13 at 23:43 +0300, Michael S. Tsirkin wrote:
> > On Tue, Jun 12, 2012 at 02:03:26PM -0600, Alex Williamson wrote:
> > > +    /*
> > > +     * Migration compatibility dictates that this remains a 4k
> > > +     * BAR with the vector table in the lower half and PBA in
> > > +     * the upper half.
> > > +     */
> > > +    if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) {
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    memory_region_init(bar, name, 4096);
> > > +
> > > +    ret = msix_init(pdev, nentries, bar, bar_nr, 0, bar, bar_nr, 2048, 0);
> > 
> > Lots of constants.
> > Current code uses macros for these, e.g.
> > MSIX_PAGE_PENDING, MSIX_PAGE_PENDING /2.
> > 
> > Let's keep it that way.
> 
> There is absolutely no valid use for them outside of this function.  I
> explain the size in the comment immediately above where they're used.
> Macro-izing these just risks someone assuming there's a standard or
> misusing it for something else (see device assignment imposing a 4k
> MSI-X table for example...)

Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion

[Michael S. Tsirkin]

On Thu, Jun 14, 2012 at 10:02:58AM -0600, Alex Williamson wrote:
> On Thu, 2012-06-14 at 18:45 +0300, Michael S. Tsirkin wrote:
> > On Thu, Jun 14, 2012 at 09:09:47AM -0600, Alex Williamson wrote:
> > > On Thu, 2012-06-14 at 17:50 +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Jun 14, 2012 at 08:21:39AM -0600, Alex Williamson wrote:
> > > > > On Thu, 2012-06-14 at 13:24 +0300, Michael S. Tsirkin wrote:
> > > > > > On Wed, Jun 13, 2012 at 10:51:47PM -0600, Alex Williamson wrote:
> > > > > > > These don't have to be contiguous.  Size them to only what
> > > > > > > they need and use separate MemoryRegions for the vector
> > > > > > > table and PBA.
> > > > > > > 
> > > > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > > > > 
> > > > > > Why is this still using NATIVE?
> > > > > 
> > > > > Because the bug already exists,
> > > > 
> > > > We have lots of broken code. The way progress happens here is
> > > > such code is in a kind of freeze until fixed. This way whoever needs new
> > > > features gets to fix the bugs too.
> > > 
> > > In other words, you impose a toll and inhibit forward progress until
> > > someone fixes it?  I have no place telling you how to be a maintainer,
> > > but I personally find that this style makes attempting to contribute
> > > code to anything pci/msi/msix related a huge pain.  There are far too
> > > many of these land mines in the code and simple fixes easily explode
> > > into tangentially related changes off your todo list.
> > 
> > I try to pick simple fixes up straight away. Pls try to keep the fixes
> > simpler :)
> 
> What does that have to do with shoving todo list items down the throats
> of contributors?

If you write new code you do not get to use legacy interfaces.
But - if you fix bugs you are not required to fix all the bugs in one go.
If you mix bugfixes and features all is treated as new code.

> > > > > this patch doesn't make it worse, so at best it's a tangentially related additional fix.
> > > > > It may seem like a s/NATIVE/LITTLE/ to you, but to me it's asking to completely scrub
> > > > > msix.c for endian correctness.  Is this going to be the carrot you hold
> > > > > out to accept the rest of the series?
> > > > > 
> > > > > Alex
> > > > 
> > > > Unfortunately no promises yet, and that is because you basically decided
> > > > to rewrite lots of code in your preferred style while also adding new functionality.
> > > > If changes were done in small steps, then I could apply things we can
> > > > agree on and defer the ones we don't.  Sometimes it's hard, but clearly
> > > > not in this case.
> > > 
> > > Patches can always be reduced into smaller changes, but at some point we
> > > have to call it good enough.  I split one patch into 6 and thought that
> > > did a pretty good job.
> > 
> > It's not the mechanical splitting of patches that is needed.
> > In one case you actually added a new function in place X then moved it
> > to place Y. And the new order does not make sense: init then uninit looks cleaner.
> 
> uninit was moved because I was able to remove duplicate code by making
> init call uninit on error.  Do you prefer a prototype to avoid code
> moves in that case?

msix.h has a prototype already I think.

>  Doesn't matter now, it's fixed with Jan's
> suggestion and I've already split the move of another tiny function to a
> separate patch.

This does not matter. What matters is making things easy to review.
If you send me a patch moving functions around, I can put them
side by side and compare + and -. If you make a
small cosmetic change I can see it is equivalent.
If you add functionality I see how it works.

But if you mix these types of change it's very hard to review.

> > > Should I remove everywhere that I've added a new
> > > line to avoid imposing my style on the rest of the code?
> > 
> > Each new line? No, that would be taking it to extreme because newlines are
> > easy to ignore normally. Though if someone sends me a patch with 1000
> > newlines tweaked and functional changes in the same patch, I won't apply
> > it.
> 
> Well then, I'm not sure what you mean by "you basically decided to
> rewrite lots of code in your preferred style".

The diff was very large, is what I mean.

> > > The next
> > > version will eliminate the add_config move thanks to Jan's constructive
> > > suggestion, so I hope it meets your standards.  Thanks,
> > > 
> > > Alex
> > 
> > Please try to address other comments too, like naming
> > constants. I would hate to get another revision that just ignores them.
> 
> It will unless you counter my rebuttal to why I'm not using macros
> there.  To repeat:
> 
> On Wed, 2012-06-13 at 17:05 -0600, Alex Williamson wrote:
> On Wed, 2012-06-13 at 23:43 +0300, Michael S. Tsirkin wrote:
> > > On Tue, Jun 12, 2012 at 02:03:26PM -0600, Alex Williamson wrote:
> > > > +    /*
> > > > +     * Migration compatibility dictates that this remains a 4k
> > > > +     * BAR with the vector table in the lower half and PBA in
> > > > +     * the upper half.
> > > > +     */
> > > > +    if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) {
> > > > +        return -EINVAL;
> > > > +    }
> > > > +
> > > > +    memory_region_init(bar, name, 4096);
> > > > +
> > > > +    ret = msix_init(pdev, nentries, bar, bar_nr, 0, bar, bar_nr, 2048, 0);
> > > 
> > > Lots of constants.
> > > Current code uses macros for these, e.g.
> > > MSIX_PAGE_PENDING, MSIX_PAGE_PENDING /2.
> > > 
> > > Let's keep it that way.
> > 
> > There is absolutely no valid use for them outside of this function.


They still appear multiple times. And 2048 is middle of page
but PAGE/2 is clearer.

>  I
> > explain the size in the comment immediately above where they're used.
> > Macro-izing these just risks someone assuming there's a standard or
> > misusing it for something else (see device assignment imposing a 4k
> > MSI-X table for example...)


A valid concern, but won't help against people copying code :)
Since you now use it from the exclusive call only, rename it
MSIX_EXLUSIVE_BAR_SIZE, MSIX_EXLUSIVE_BAR_PENDING?
It's actually what it is.

-- 
MST

[Qemu-devel] [PATCH v2 5/6] msix: Allow full specification of MSIX layout

[Alex Williamson]

Finally, complete the fully specified interface.  msix_add_config()
gets moved to be closer to the setup functions where it's actually
used.  msix_mmio_setup() gets folded into msix_init().  And
msix_uninit() gets reworked a bit so we can call it as cleanup
from msix_init().

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/msix.c |  217 +++++++++++++++++++++++++++----------------------------------
 hw/msix.h |   10 ++-
 2 files changed, 101 insertions(+), 126 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index d476d07..047646a 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -27,14 +27,6 @@
 #define MSIX_ENABLE_MASK (PCI_MSIX_FLAGS_ENABLE >> 8)
 #define MSIX_MASKALL_MASK (PCI_MSIX_FLAGS_MASKALL >> 8)
 
-/* How much space does an MSIX table need. */
-/* The spec requires giving the table structure
- * a 4K aligned region all by itself. */
-#define MSIX_PAGE_SIZE 0x1000
-/* Reserve second half of the page for pending bits */
-#define MSIX_PAGE_PENDING (MSIX_PAGE_SIZE / 2)
-#define MSIX_MAX_ENTRIES 32
-
 static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
 {
     uint8_t *table_entry = dev->msix_table + vector * PCI_MSIX_ENTRY_SIZE;
@@ -45,47 +37,6 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
     return msg;
 }
 
-/* Add MSI-X capability to the config space for the device. */
-/* Given a bar and its size, add MSI-X table on top of it
- * and fill MSI-X capability in the config space.
- * Original bar size must be a power of 2 or 0.
- * New bar size is returned. */
-static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
-                           unsigned bar_nr, unsigned bar_size)
-{
-    int config_offset;
-    uint8_t *config;
-
-    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
-        return -EINVAL;
-    if (bar_size > 0x80000000)
-        return -ENOSPC;
-
-    /* Require aligned offset for MSI-X structures */
-    if (bar_size & ~(MSIX_PAGE_SIZE - 1)) {
-        return -EINVAL;
-    }
-
-    config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
-                                       0, MSIX_CAP_LENGTH);
-    if (config_offset < 0)
-        return config_offset;
-    config = pdev->config + config_offset;
-
-    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
-    /* Table on top of BAR */
-    pci_set_long(config + PCI_MSIX_TABLE, bar_size | bar_nr);
-    /* Pending bits on top of that */
-    pci_set_long(config + PCI_MSIX_PBA, (bar_size + MSIX_PAGE_PENDING) |
-                 bar_nr);
-    pdev->msix_cap = config_offset;
-    /* Make flags bit writable. */
-    pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
-	    MSIX_MASKALL_MASK;
-    pdev->msix_function_masked = true;
-    return 0;
-}
-
 static uint8_t msix_pending_mask(int vector)
 {
     return 1 << (vector % 8);
@@ -240,20 +191,6 @@ static const MemoryRegionOps msix_pba_mmio_ops = {
     },
 };
 
-static void msix_mmio_setup(PCIDevice *d, MemoryRegion *bar)
-{
-    uint8_t *config = d->config + d->msix_cap;
-    uint32_t table = pci_get_long(config + PCI_MSIX_TABLE);
-    uint32_t table_offset = table & ~PCI_MSIX_FLAGS_BIRMASK;
-    uint32_t pba = pci_get_long(config + PCI_MSIX_PBA);
-    uint32_t pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
-    /* TODO: for assigned devices, we'll want to make it possible to map
-     * pending bits separately in case they are in a separate bar. */
-
-    memory_region_add_subregion(bar, table_offset, &d->msix_table_mmio);
-    memory_region_add_subregion(bar, pba_offset, &d->msix_pba_mmio);
-}
-
 static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
 {
     int vector;
@@ -268,11 +205,72 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
     }
 }
 
-/* Initialize the MSI-X structures. Note: if MSI-X is supported, BAR size is
- * modified, it should be retrieved with msix_bar_size. */
+/* Add MSI-X capability to the config space for the device. */
+static int msix_add_config(struct PCIDevice *dev, unsigned short nentries,
+                           uint8_t table_bar, unsigned table_offset,
+                           uint8_t pba_bar, unsigned pba_offset, uint8_t pos)
+{
+    int config_offset;
+    uint8_t *config;
+
+    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
+        return -EINVAL;
+    }
+
+    config_offset = pci_add_capability(dev, PCI_CAP_ID_MSIX,
+                                       pos, MSIX_CAP_LENGTH);
+    if (config_offset < 0) {
+        return config_offset;
+    }
+
+    config = dev->config + config_offset;
+
+    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
+    pci_set_long(config + PCI_MSIX_TABLE, table_offset | table_bar);
+    pci_set_long(config + PCI_MSIX_PBA, pba_offset | pba_bar);
+
+    dev->msix_cap = config_offset;
+
+    /* Make flags bit writable. */
+    dev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
+                                                       MSIX_MASKALL_MASK;
+
+    dev->msix_function_masked = true;
+
+    return 0;
+}
+
+/* Clean up resources for the device. */
+void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
+{
+    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
+        return;
+    }
+
+    pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
+    dev->msix_cap = 0;
+    dev->msix_entries_nr = 0;
+
+    memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
+    memory_region_destroy(&dev->msix_pba_mmio);
+    g_free(dev->msix_pba);
+    dev->msix_pba = NULL;
+
+    memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
+    memory_region_destroy(&dev->msix_table_mmio);
+    g_free(dev->msix_table);
+    dev->msix_table = NULL;
+
+    g_free(dev->msix_entry_used);
+    dev->msix_entry_used = NULL;
+    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
+}
+
+/* Initialize the MSI-X structures */
 int msix_init(struct PCIDevice *dev, unsigned short nentries,
-              MemoryRegion *bar,
-              unsigned bar_nr, unsigned bar_size)
+              MemoryRegion *table_bar, uint8_t table_bar_nr,
+              unsigned table_offset, MemoryRegion *pba_bar,
+              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos)
 {
     int ret;
     unsigned table_size, pba_size;
@@ -281,43 +279,41 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
     if (!msi_supported) {
         return -ENOTSUP;
     }
-    if (nentries > MSIX_MAX_ENTRIES)
-        return -EINVAL;
 
     table_size = nentries * PCI_MSIX_ENTRY_SIZE;
     pba_size = QEMU_ALIGN_UP(nentries, 64) / 8;
 
-    dev->msix_entry_used = g_malloc0(MSIX_MAX_ENTRIES *
-                                        sizeof *dev->msix_entry_used);
+    /* Sanity test: table & pba don't overlap, fit within BARs, min aligned */
+    if ((table_bar_nr == pba_bar_nr &&
+         ranges_overlap(table_offset, table_size, pba_offset, pba_size)) ||
+        table_offset + table_size > memory_region_size(table_bar) ||
+        pba_offset + pba_size > memory_region_size(pba_bar) ||
+        (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
+        return -EINVAL;
+    }
 
     dev->msix_table = g_malloc0(table_size);
     dev->msix_pba = g_malloc0(pba_size);
+    dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
+    dev->msix_entries_nr = nentries;
+    dev->cap_present |= QEMU_PCI_CAP_MSIX;
+
     msix_mask_all(dev, nentries);
 
     memory_region_init_io(&dev->msix_table_mmio, &msix_table_mmio_ops, dev,
                           "msix", table_size);
+    memory_region_add_subregion(table_bar, table_offset, &dev->msix_table_mmio);
+
     memory_region_init_io(&dev->msix_pba_mmio, &msix_pba_mmio_ops, dev,
                           "msix-pba", pba_size);
+    memory_region_add_subregion(pba_bar, pba_offset, &dev->msix_pba_mmio);
 
-    dev->msix_entries_nr = nentries;
-    ret = msix_add_config(dev, nentries, bar_nr, bar_size);
-    if (ret)
-        goto err_config;
-
-    dev->cap_present |= QEMU_PCI_CAP_MSIX;
-    msix_mmio_setup(dev, bar);
-    return 0;
+    ret = msix_add_config(dev, nentries, table_bar_nr, table_offset,
+                          pba_bar_nr, pba_offset, cap_pos);
+    if (ret) {
+        msix_uninit(dev, table_bar, pba_bar);
+    }
 
-err_config:
-    dev->msix_entries_nr = 0;
-    memory_region_destroy(&dev->msix_pba_mmio);
-    g_free(dev->msix_pba);
-    dev->msix_pba = NULL;
-    memory_region_destroy(&dev->msix_table_mmio);
-    g_free(dev->msix_table);
-    dev->msix_table = NULL;
-    g_free(dev->msix_entry_used);
-    dev->msix_entry_used = NULL;
     return ret;
 }
 
@@ -344,7 +340,8 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
 
     free(name);
 
-    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
+    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 0,
+                    &dev->msix_exclusive_bar, bar_nr, 2048, 0);
     if (ret) {
         memory_region_destroy(&dev->msix_exclusive_bar);
         return ret;
@@ -356,6 +353,14 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
     return 0;
 }
 
+void msix_uninit_exclusive_bar(PCIDevice *dev)
+{
+    if (msix_present(dev)) {
+        msix_uninit(dev, &dev->msix_exclusive_bar, &dev->msix_exclusive_bar);
+        memory_region_destroy(&dev->msix_exclusive_bar);
+    }
+}
+
 static void msix_free_irq_entries(PCIDevice *dev)
 {
     int vector;
@@ -366,38 +371,6 @@ static void msix_free_irq_entries(PCIDevice *dev)
     }
 }
 
-/* Clean up resources for the device. */
-int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
-{
-    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);
-    dev->msix_entries_nr = 0;
-    memory_region_del_subregion(bar, &dev->msix_pba_mmio);
-    memory_region_destroy(&dev->msix_pba_mmio);
-    g_free(dev->msix_pba);
-    dev->msix_pba = NULL;
-    memory_region_del_subregion(bar, &dev->msix_table_mmio);
-    memory_region_destroy(&dev->msix_table_mmio);
-    g_free(dev->msix_table);
-    dev->msix_table = NULL;
-    g_free(dev->msix_entry_used);
-    dev->msix_entry_used = NULL;
-    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
-    return 0;
-}
-
-void msix_uninit_exclusive_bar(PCIDevice *dev)
-{
-    if (msix_present(dev)) {
-        msix_uninit(dev, &dev->msix_exclusive_bar);
-        memory_region_destroy(&dev->msix_exclusive_bar);
-    }
-}
-
 void msix_save(PCIDevice *dev, QEMUFile *f)
 {
     unsigned n = dev->msix_entries_nr;
diff --git a/hw/msix.h b/hw/msix.h
index bed6bfb..14b1a2e 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -4,16 +4,18 @@
 #include "qemu-common.h"
 #include "pci.h"
 
-int msix_init(PCIDevice *pdev, unsigned short nentries,
-              MemoryRegion *bar,
-              unsigned bar_nr, unsigned bar_size);
+int msix_init(PCIDevice *dev, unsigned short nentries,
+              MemoryRegion *table_bar, uint8_t table_bar_nr,
+              unsigned table_offset, MemoryRegion *pba_bar,
+              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
 int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
                             uint8_t bar_nr);
 
 void msix_write_config(PCIDevice *pci_dev, uint32_t address,
                        uint32_t val, int len);
 
-int msix_uninit(PCIDevice *d, MemoryRegion *bar);
+void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar,
+                 MemoryRegion *pba_bar);
 void msix_uninit_exclusive_bar(PCIDevice *dev);
 
 unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);

Re: [Qemu-devel] [PATCH v2 5/6] msix: Allow full specification of MSIX layout

[Jan Kiszka]

On 2012-06-14 06:51, Alex Williamson wrote:
> Finally, complete the fully specified interface.  msix_add_config()
> gets moved to be closer to the setup functions where it's actually
> used.  msix_mmio_setup() gets folded into msix_init().  And
> msix_uninit() gets reworked a bit so we can call it as cleanup
> from msix_init().
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
>  hw/msix.c |  217 +++++++++++++++++++++++++++----------------------------------
>  hw/msix.h |   10 ++-
>  2 files changed, 101 insertions(+), 126 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index d476d07..047646a 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -27,14 +27,6 @@
>  #define MSIX_ENABLE_MASK (PCI_MSIX_FLAGS_ENABLE >> 8)
>  #define MSIX_MASKALL_MASK (PCI_MSIX_FLAGS_MASKALL >> 8)
>  
> -/* How much space does an MSIX table need. */
> -/* The spec requires giving the table structure
> - * a 4K aligned region all by itself. */
> -#define MSIX_PAGE_SIZE 0x1000
> -/* Reserve second half of the page for pending bits */
> -#define MSIX_PAGE_PENDING (MSIX_PAGE_SIZE / 2)
> -#define MSIX_MAX_ENTRIES 32
> -
>  static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
>  {
>      uint8_t *table_entry = dev->msix_table + vector * PCI_MSIX_ENTRY_SIZE;
> @@ -45,47 +37,6 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
>      return msg;
>  }
>  
> -/* Add MSI-X capability to the config space for the device. */
> -/* Given a bar and its size, add MSI-X table on top of it
> - * and fill MSI-X capability in the config space.
> - * Original bar size must be a power of 2 or 0.
> - * New bar size is returned. */
> -static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> -                           unsigned bar_nr, unsigned bar_size)
> -{
> -    int config_offset;
> -    uint8_t *config;
> -
> -    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> -        return -EINVAL;
> -    if (bar_size > 0x80000000)
> -        return -ENOSPC;
> -
> -    /* Require aligned offset for MSI-X structures */
> -    if (bar_size & ~(MSIX_PAGE_SIZE - 1)) {
> -        return -EINVAL;
> -    }
> -
> -    config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
> -                                       0, MSIX_CAP_LENGTH);
> -    if (config_offset < 0)
> -        return config_offset;
> -    config = pdev->config + config_offset;
> -
> -    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> -    /* Table on top of BAR */
> -    pci_set_long(config + PCI_MSIX_TABLE, bar_size | bar_nr);
> -    /* Pending bits on top of that */
> -    pci_set_long(config + PCI_MSIX_PBA, (bar_size + MSIX_PAGE_PENDING) |
> -                 bar_nr);
> -    pdev->msix_cap = config_offset;
> -    /* Make flags bit writable. */
> -    pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> -	    MSIX_MASKALL_MASK;
> -    pdev->msix_function_masked = true;
> -    return 0;
> -}
> -
>  static uint8_t msix_pending_mask(int vector)
>  {
>      return 1 << (vector % 8);
> @@ -240,20 +191,6 @@ static const MemoryRegionOps msix_pba_mmio_ops = {
>      },
>  };
>  
> -static void msix_mmio_setup(PCIDevice *d, MemoryRegion *bar)
> -{
> -    uint8_t *config = d->config + d->msix_cap;
> -    uint32_t table = pci_get_long(config + PCI_MSIX_TABLE);
> -    uint32_t table_offset = table & ~PCI_MSIX_FLAGS_BIRMASK;
> -    uint32_t pba = pci_get_long(config + PCI_MSIX_PBA);
> -    uint32_t pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
> -    /* TODO: for assigned devices, we'll want to make it possible to map
> -     * pending bits separately in case they are in a separate bar. */
> -
> -    memory_region_add_subregion(bar, table_offset, &d->msix_table_mmio);
> -    memory_region_add_subregion(bar, pba_offset, &d->msix_pba_mmio);
> -}
> -
>  static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>  {
>      int vector;
> @@ -268,11 +205,72 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>      }
>  }
>  
> -/* Initialize the MSI-X structures. Note: if MSI-X is supported, BAR size is
> - * modified, it should be retrieved with msix_bar_size. */
> +/* Add MSI-X capability to the config space for the device. */
> +static int msix_add_config(struct PCIDevice *dev, unsigned short nentries,
> +                           uint8_t table_bar, unsigned table_offset,
> +                           uint8_t pba_bar, unsigned pba_offset, uint8_t pos)

Why not fold msix_add_config into msix_init and move sanity checks to
the beginning, i.e. before resource allocations? Then you also do not
need to call msix_uninit from msix_init. Likely a matter of taste, so
not a must-have.

> +{
> +    int config_offset;
> +    uint8_t *config;
> +
> +    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
> +        return -EINVAL;
> +    }
> +
> +    config_offset = pci_add_capability(dev, PCI_CAP_ID_MSIX,
> +                                       pos, MSIX_CAP_LENGTH);
> +    if (config_offset < 0) {
> +        return config_offset;
> +    }
> +
> +    config = dev->config + config_offset;
> +
> +    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> +    pci_set_long(config + PCI_MSIX_TABLE, table_offset | table_bar);
> +    pci_set_long(config + PCI_MSIX_PBA, pba_offset | pba_bar);
> +
> +    dev->msix_cap = config_offset;
> +
> +    /* Make flags bit writable. */
> +    dev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> +                                                       MSIX_MASKALL_MASK;
> +
> +    dev->msix_function_masked = true;
> +
> +    return 0;
> +}
> +
> +/* Clean up resources for the device. */
> +void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
> +{
> +    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {

msix_present()

> +        return;
> +    }
> +
> +    pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> +    dev->msix_cap = 0;
> +    dev->msix_entries_nr = 0;
> +
> +    memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
> +    memory_region_destroy(&dev->msix_pba_mmio);
> +    g_free(dev->msix_pba);
> +    dev->msix_pba = NULL;
> +
> +    memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
> +    memory_region_destroy(&dev->msix_table_mmio);
> +    g_free(dev->msix_table);
> +    dev->msix_table = NULL;
> +
> +    g_free(dev->msix_entry_used);
> +    dev->msix_entry_used = NULL;
> +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> +}
> +
> +/* Initialize the MSI-X structures */
>  int msix_init(struct PCIDevice *dev, unsigned short nentries,
> -              MemoryRegion *bar,
> -              unsigned bar_nr, unsigned bar_size)
> +              MemoryRegion *table_bar, uint8_t table_bar_nr,
> +              unsigned table_offset, MemoryRegion *pba_bar,
> +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos)
>  {
>      int ret;
>      unsigned table_size, pba_size;
> @@ -281,43 +279,41 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>      if (!msi_supported) {
>          return -ENOTSUP;
>      }
> -    if (nentries > MSIX_MAX_ENTRIES)
> -        return -EINVAL;
>  
>      table_size = nentries * PCI_MSIX_ENTRY_SIZE;
>      pba_size = QEMU_ALIGN_UP(nentries, 64) / 8;
>  
> -    dev->msix_entry_used = g_malloc0(MSIX_MAX_ENTRIES *
> -                                        sizeof *dev->msix_entry_used);
> +    /* Sanity test: table & pba don't overlap, fit within BARs, min aligned */
> +    if ((table_bar_nr == pba_bar_nr &&
> +         ranges_overlap(table_offset, table_size, pba_offset, pba_size)) ||
> +        table_offset + table_size > memory_region_size(table_bar) ||
> +        pba_offset + pba_size > memory_region_size(pba_bar) ||
> +        (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
> +        return -EINVAL;
> +    }
>  
>      dev->msix_table = g_malloc0(table_size);
>      dev->msix_pba = g_malloc0(pba_size);
> +    dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
> +    dev->msix_entries_nr = nentries;
> +    dev->cap_present |= QEMU_PCI_CAP_MSIX;
> +
>      msix_mask_all(dev, nentries);
>  
>      memory_region_init_io(&dev->msix_table_mmio, &msix_table_mmio_ops, dev,
>                            "msix", table_size);
> +    memory_region_add_subregion(table_bar, table_offset, &dev->msix_table_mmio);
> +
>      memory_region_init_io(&dev->msix_pba_mmio, &msix_pba_mmio_ops, dev,
>                            "msix-pba", pba_size);
> +    memory_region_add_subregion(pba_bar, pba_offset, &dev->msix_pba_mmio);
>  
> -    dev->msix_entries_nr = nentries;
> -    ret = msix_add_config(dev, nentries, bar_nr, bar_size);
> -    if (ret)
> -        goto err_config;
> -
> -    dev->cap_present |= QEMU_PCI_CAP_MSIX;
> -    msix_mmio_setup(dev, bar);
> -    return 0;
> +    ret = msix_add_config(dev, nentries, table_bar_nr, table_offset,
> +                          pba_bar_nr, pba_offset, cap_pos);
> +    if (ret) {
> +        msix_uninit(dev, table_bar, pba_bar);
> +    }
>  
> -err_config:
> -    dev->msix_entries_nr = 0;
> -    memory_region_destroy(&dev->msix_pba_mmio);
> -    g_free(dev->msix_pba);
> -    dev->msix_pba = NULL;
> -    memory_region_destroy(&dev->msix_table_mmio);
> -    g_free(dev->msix_table);
> -    dev->msix_table = NULL;
> -    g_free(dev->msix_entry_used);
> -    dev->msix_entry_used = NULL;
>      return ret;
>  }
>  
> @@ -344,7 +340,8 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>  
>      free(name);
>  
> -    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
> +    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 0,
> +                    &dev->msix_exclusive_bar, bar_nr, 2048, 0);

Doesn't this have to be msix_table_mmi and msix_pba_mmio?

>      if (ret) {
>          memory_region_destroy(&dev->msix_exclusive_bar);
>          return ret;
> @@ -356,6 +353,14 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>      return 0;
>  }
>  
> +void msix_uninit_exclusive_bar(PCIDevice *dev)
> +{
> +    if (msix_present(dev)) {
> +        msix_uninit(dev, &dev->msix_exclusive_bar, &dev->msix_exclusive_bar);

Same here.

> +        memory_region_destroy(&dev->msix_exclusive_bar);
> +    }
> +}
> +
>  static void msix_free_irq_entries(PCIDevice *dev)
>  {
>      int vector;
> @@ -366,38 +371,6 @@ static void msix_free_irq_entries(PCIDevice *dev)
>      }
>  }
>  
> -/* Clean up resources for the device. */
> -int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
> -{
> -    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);
> -    dev->msix_entries_nr = 0;
> -    memory_region_del_subregion(bar, &dev->msix_pba_mmio);
> -    memory_region_destroy(&dev->msix_pba_mmio);
> -    g_free(dev->msix_pba);
> -    dev->msix_pba = NULL;
> -    memory_region_del_subregion(bar, &dev->msix_table_mmio);
> -    memory_region_destroy(&dev->msix_table_mmio);
> -    g_free(dev->msix_table);
> -    dev->msix_table = NULL;
> -    g_free(dev->msix_entry_used);
> -    dev->msix_entry_used = NULL;
> -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> -    return 0;
> -}
> -
> -void msix_uninit_exclusive_bar(PCIDevice *dev)
> -{
> -    if (msix_present(dev)) {
> -        msix_uninit(dev, &dev->msix_exclusive_bar);
> -        memory_region_destroy(&dev->msix_exclusive_bar);
> -    }
> -}
> -
>  void msix_save(PCIDevice *dev, QEMUFile *f)
>  {
>      unsigned n = dev->msix_entries_nr;
> diff --git a/hw/msix.h b/hw/msix.h
> index bed6bfb..14b1a2e 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -4,16 +4,18 @@
>  #include "qemu-common.h"
>  #include "pci.h"
>  
> -int msix_init(PCIDevice *pdev, unsigned short nentries,
> -              MemoryRegion *bar,
> -              unsigned bar_nr, unsigned bar_size);
> +int msix_init(PCIDevice *dev, unsigned short nentries,
> +              MemoryRegion *table_bar, uint8_t table_bar_nr,
> +              unsigned table_offset, MemoryRegion *pba_bar,
> +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>                              uint8_t bar_nr);
>  
>  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
>                         uint32_t val, int len);
>  
> -int msix_uninit(PCIDevice *d, MemoryRegion *bar);
> +void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar,
> +                 MemoryRegion *pba_bar);
>  void msix_uninit_exclusive_bar(PCIDevice *dev);
>  
>  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
> 

Interfaces look good to me, the logic as well - except for the few remarks.

Jan

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

Re: [Qemu-devel] [PATCH v2 5/6] msix: Allow full specification of MSIX layout

[Michael S. Tsirkin]

On Thu, Jun 14, 2012 at 08:17:22AM +0200, Jan Kiszka wrote:
> On 2012-06-14 06:51, Alex Williamson wrote:
> > Finally, complete the fully specified interface.  msix_add_config()
> > gets moved to be closer to the setup functions where it's actually
> > used.  msix_mmio_setup() gets folded into msix_init().  And
> > msix_uninit() gets reworked a bit so we can call it as cleanup
> > from msix_init().
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > 
> >  hw/msix.c |  217 +++++++++++++++++++++++++++----------------------------------
> >  hw/msix.h |   10 ++-
> >  2 files changed, 101 insertions(+), 126 deletions(-)
> > 
> > diff --git a/hw/msix.c b/hw/msix.c
> > index d476d07..047646a 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -27,14 +27,6 @@
> >  #define MSIX_ENABLE_MASK (PCI_MSIX_FLAGS_ENABLE >> 8)
> >  #define MSIX_MASKALL_MASK (PCI_MSIX_FLAGS_MASKALL >> 8)
> >  
> > -/* How much space does an MSIX table need. */
> > -/* The spec requires giving the table structure
> > - * a 4K aligned region all by itself. */
> > -#define MSIX_PAGE_SIZE 0x1000
> > -/* Reserve second half of the page for pending bits */
> > -#define MSIX_PAGE_PENDING (MSIX_PAGE_SIZE / 2)
> > -#define MSIX_MAX_ENTRIES 32
> > -
> >  static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
> >  {
> >      uint8_t *table_entry = dev->msix_table + vector * PCI_MSIX_ENTRY_SIZE;
> > @@ -45,47 +37,6 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
> >      return msg;
> >  }
> >  
> > -/* Add MSI-X capability to the config space for the device. */
> > -/* Given a bar and its size, add MSI-X table on top of it
> > - * and fill MSI-X capability in the config space.
> > - * Original bar size must be a power of 2 or 0.
> > - * New bar size is returned. */
> > -static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> > -                           unsigned bar_nr, unsigned bar_size)
> > -{
> > -    int config_offset;
> > -    uint8_t *config;
> > -
> > -    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> > -        return -EINVAL;
> > -    if (bar_size > 0x80000000)
> > -        return -ENOSPC;
> > -
> > -    /* Require aligned offset for MSI-X structures */
> > -    if (bar_size & ~(MSIX_PAGE_SIZE - 1)) {
> > -        return -EINVAL;
> > -    }
> > -
> > -    config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
> > -                                       0, MSIX_CAP_LENGTH);
> > -    if (config_offset < 0)
> > -        return config_offset;
> > -    config = pdev->config + config_offset;
> > -
> > -    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > -    /* Table on top of BAR */
> > -    pci_set_long(config + PCI_MSIX_TABLE, bar_size | bar_nr);
> > -    /* Pending bits on top of that */
> > -    pci_set_long(config + PCI_MSIX_PBA, (bar_size + MSIX_PAGE_PENDING) |
> > -                 bar_nr);
> > -    pdev->msix_cap = config_offset;
> > -    /* Make flags bit writable. */
> > -    pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> > -	    MSIX_MASKALL_MASK;
> > -    pdev->msix_function_masked = true;
> > -    return 0;
> > -}
> > -
> >  static uint8_t msix_pending_mask(int vector)
> >  {
> >      return 1 << (vector % 8);
> > @@ -240,20 +191,6 @@ static const MemoryRegionOps msix_pba_mmio_ops = {
> >      },
> >  };
> >  
> > -static void msix_mmio_setup(PCIDevice *d, MemoryRegion *bar)
> > -{
> > -    uint8_t *config = d->config + d->msix_cap;
> > -    uint32_t table = pci_get_long(config + PCI_MSIX_TABLE);
> > -    uint32_t table_offset = table & ~PCI_MSIX_FLAGS_BIRMASK;
> > -    uint32_t pba = pci_get_long(config + PCI_MSIX_PBA);
> > -    uint32_t pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
> > -    /* TODO: for assigned devices, we'll want to make it possible to map
> > -     * pending bits separately in case they are in a separate bar. */
> > -
> > -    memory_region_add_subregion(bar, table_offset, &d->msix_table_mmio);
> > -    memory_region_add_subregion(bar, pba_offset, &d->msix_pba_mmio);
> > -}
> > -
> >  static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
> >  {
> >      int vector;
> > @@ -268,11 +205,72 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
> >      }
> >  }
> >  
> > -/* Initialize the MSI-X structures. Note: if MSI-X is supported, BAR size is
> > - * modified, it should be retrieved with msix_bar_size. */
> > +/* Add MSI-X capability to the config space for the device. */
> > +static int msix_add_config(struct PCIDevice *dev, unsigned short nentries,
> > +                           uint8_t table_bar, unsigned table_offset,
> > +                           uint8_t pba_bar, unsigned pba_offset, uint8_t pos)
> 
> Why not fold msix_add_config into msix_init and move sanity checks to
> the beginning, i.e. before resource allocations? Then you also do not
> need to call msix_uninit from msix_init. Likely a matter of taste, so
> not a must-have.

More importantly pls try to split code reorgs and adding functionality
so diff is readable.

> > +{
> > +    int config_offset;
> > +    uint8_t *config;
> > +
> > +    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
> > +        return -EINVAL;
> > +    }
> > +
> > +    config_offset = pci_add_capability(dev, PCI_CAP_ID_MSIX,
> > +                                       pos, MSIX_CAP_LENGTH);
> > +    if (config_offset < 0) {
> > +        return config_offset;
> > +    }
> > +
> > +    config = dev->config + config_offset;
> > +
> > +    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > +    pci_set_long(config + PCI_MSIX_TABLE, table_offset | table_bar);
> > +    pci_set_long(config + PCI_MSIX_PBA, pba_offset | pba_bar);
> > +
> > +    dev->msix_cap = config_offset;
> > +
> > +    /* Make flags bit writable. */
> > +    dev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> > +                                                       MSIX_MASKALL_MASK;
> > +
> > +    dev->msix_function_masked = true;
> > +
> > +    return 0;
> > +}
> > +
> > +/* Clean up resources for the device. */
> > +void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
> > +{
> > +    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
> 
> msix_present()
> 
> > +        return;
> > +    }
> > +
> > +    pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> > +    dev->msix_cap = 0;
> > +    dev->msix_entries_nr = 0;
> > +
> > +    memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
> > +    memory_region_destroy(&dev->msix_pba_mmio);
> > +    g_free(dev->msix_pba);
> > +    dev->msix_pba = NULL;
> > +
> > +    memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
> > +    memory_region_destroy(&dev->msix_table_mmio);
> > +    g_free(dev->msix_table);
> > +    dev->msix_table = NULL;
> > +
> > +    g_free(dev->msix_entry_used);
> > +    dev->msix_entry_used = NULL;
> > +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> > +}
> > +
> > +/* Initialize the MSI-X structures */
> >  int msix_init(struct PCIDevice *dev, unsigned short nentries,
> > -              MemoryRegion *bar,
> > -              unsigned bar_nr, unsigned bar_size)
> > +              MemoryRegion *table_bar, uint8_t table_bar_nr,
> > +              unsigned table_offset, MemoryRegion *pba_bar,
> > +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos)
> >  {
> >      int ret;
> >      unsigned table_size, pba_size;
> > @@ -281,43 +279,41 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> >      if (!msi_supported) {
> >          return -ENOTSUP;
> >      }
> > -    if (nentries > MSIX_MAX_ENTRIES)
> > -        return -EINVAL;
> >  
> >      table_size = nentries * PCI_MSIX_ENTRY_SIZE;
> >      pba_size = QEMU_ALIGN_UP(nentries, 64) / 8;
> >  
> > -    dev->msix_entry_used = g_malloc0(MSIX_MAX_ENTRIES *
> > -                                        sizeof *dev->msix_entry_used);
> > +    /* Sanity test: table & pba don't overlap, fit within BARs, min aligned */
> > +    if ((table_bar_nr == pba_bar_nr &&
> > +         ranges_overlap(table_offset, table_size, pba_offset, pba_size)) ||
> > +        table_offset + table_size > memory_region_size(table_bar) ||
> > +        pba_offset + pba_size > memory_region_size(pba_bar) ||
> > +        (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
> > +        return -EINVAL;
> > +    }
> >  
> >      dev->msix_table = g_malloc0(table_size);
> >      dev->msix_pba = g_malloc0(pba_size);
> > +    dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
> > +    dev->msix_entries_nr = nentries;
> > +    dev->cap_present |= QEMU_PCI_CAP_MSIX;
> > +
> >      msix_mask_all(dev, nentries);
> >  
> >      memory_region_init_io(&dev->msix_table_mmio, &msix_table_mmio_ops, dev,
> >                            "msix", table_size);
> > +    memory_region_add_subregion(table_bar, table_offset, &dev->msix_table_mmio);
> > +
> >      memory_region_init_io(&dev->msix_pba_mmio, &msix_pba_mmio_ops, dev,
> >                            "msix-pba", pba_size);
> > +    memory_region_add_subregion(pba_bar, pba_offset, &dev->msix_pba_mmio);
> >  
> > -    dev->msix_entries_nr = nentries;
> > -    ret = msix_add_config(dev, nentries, bar_nr, bar_size);
> > -    if (ret)
> > -        goto err_config;
> > -
> > -    dev->cap_present |= QEMU_PCI_CAP_MSIX;
> > -    msix_mmio_setup(dev, bar);
> > -    return 0;
> > +    ret = msix_add_config(dev, nentries, table_bar_nr, table_offset,
> > +                          pba_bar_nr, pba_offset, cap_pos);
> > +    if (ret) {
> > +        msix_uninit(dev, table_bar, pba_bar);
> > +    }
> >  
> > -err_config:
> > -    dev->msix_entries_nr = 0;
> > -    memory_region_destroy(&dev->msix_pba_mmio);
> > -    g_free(dev->msix_pba);
> > -    dev->msix_pba = NULL;
> > -    memory_region_destroy(&dev->msix_table_mmio);
> > -    g_free(dev->msix_table);
> > -    dev->msix_table = NULL;
> > -    g_free(dev->msix_entry_used);
> > -    dev->msix_entry_used = NULL;
> >      return ret;
> >  }
> >  
> > @@ -344,7 +340,8 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >  
> >      free(name);
> >  
> > -    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
> > +    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 0,
> > +                    &dev->msix_exclusive_bar, bar_nr, 2048, 0);
> 
> Doesn't this have to be msix_table_mmi and msix_pba_mmio?
> 
> >      if (ret) {
> >          memory_region_destroy(&dev->msix_exclusive_bar);
> >          return ret;
> > @@ -356,6 +353,14 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >      return 0;
> >  }
> >  
> > +void msix_uninit_exclusive_bar(PCIDevice *dev)
> > +{
> > +    if (msix_present(dev)) {
> > +        msix_uninit(dev, &dev->msix_exclusive_bar, &dev->msix_exclusive_bar);
> 
> Same here.
> 
> > +        memory_region_destroy(&dev->msix_exclusive_bar);
> > +    }
> > +}
> > +
> >  static void msix_free_irq_entries(PCIDevice *dev)
> >  {
> >      int vector;
> > @@ -366,38 +371,6 @@ static void msix_free_irq_entries(PCIDevice *dev)
> >      }
> >  }
> >  
> > -/* Clean up resources for the device. */
> > -int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
> > -{
> > -    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);
> > -    dev->msix_entries_nr = 0;
> > -    memory_region_del_subregion(bar, &dev->msix_pba_mmio);
> > -    memory_region_destroy(&dev->msix_pba_mmio);
> > -    g_free(dev->msix_pba);
> > -    dev->msix_pba = NULL;
> > -    memory_region_del_subregion(bar, &dev->msix_table_mmio);
> > -    memory_region_destroy(&dev->msix_table_mmio);
> > -    g_free(dev->msix_table);
> > -    dev->msix_table = NULL;
> > -    g_free(dev->msix_entry_used);
> > -    dev->msix_entry_used = NULL;
> > -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> > -    return 0;
> > -}
> > -
> > -void msix_uninit_exclusive_bar(PCIDevice *dev)
> > -{
> > -    if (msix_present(dev)) {
> > -        msix_uninit(dev, &dev->msix_exclusive_bar);
> > -        memory_region_destroy(&dev->msix_exclusive_bar);
> > -    }
> > -}
> > -
> >  void msix_save(PCIDevice *dev, QEMUFile *f)
> >  {
> >      unsigned n = dev->msix_entries_nr;
> > diff --git a/hw/msix.h b/hw/msix.h
> > index bed6bfb..14b1a2e 100644
> > --- a/hw/msix.h
> > +++ b/hw/msix.h
> > @@ -4,16 +4,18 @@
> >  #include "qemu-common.h"
> >  #include "pci.h"
> >  
> > -int msix_init(PCIDevice *pdev, unsigned short nentries,
> > -              MemoryRegion *bar,
> > -              unsigned bar_nr, unsigned bar_size);
> > +int msix_init(PCIDevice *dev, unsigned short nentries,
> > +              MemoryRegion *table_bar, uint8_t table_bar_nr,
> > +              unsigned table_offset, MemoryRegion *pba_bar,
> > +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
> >  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >                              uint8_t bar_nr);
> >  
> >  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
> >                         uint32_t val, int len);
> >  
> > -int msix_uninit(PCIDevice *d, MemoryRegion *bar);
> > +void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar,
> > +                 MemoryRegion *pba_bar);
> >  void msix_uninit_exclusive_bar(PCIDevice *dev);
> >  
> >  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
> > 
> 
> Interfaces look good to me, the logic as well - except for the few remarks.
> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH v2 5/6] msix: Allow full specification of MSIX layout

[Alex Williamson]

On Thu, 2012-06-14 at 08:17 +0200, Jan Kiszka wrote:
> On 2012-06-14 06:51, Alex Williamson wrote:
>   
> > -/* Initialize the MSI-X structures. Note: if MSI-X is supported, BAR size is
> > - * modified, it should be retrieved with msix_bar_size. */
> > +/* Add MSI-X capability to the config space for the device. */
> > +static int msix_add_config(struct PCIDevice *dev, unsigned short nentries,
> > +                           uint8_t table_bar, unsigned table_offset,
> > +                           uint8_t pba_bar, unsigned pba_offset, uint8_t pos)
> 
> Why not fold msix_add_config into msix_init and move sanity checks to
> the beginning, i.e. before resource allocations? Then you also do not
> need to call msix_uninit from msix_init. Likely a matter of taste, so
> not a must-have.

Yep, that works nicely.  fixed.

> > +{
> > +    int config_offset;
> > +    uint8_t *config;
> > +
> > +    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
> > +        return -EINVAL;
> > +    }
> > +
> > +    config_offset = pci_add_capability(dev, PCI_CAP_ID_MSIX,
> > +                                       pos, MSIX_CAP_LENGTH);
> > +    if (config_offset < 0) {
> > +        return config_offset;
> > +    }
> > +
> > +    config = dev->config + config_offset;
> > +
> > +    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > +    pci_set_long(config + PCI_MSIX_TABLE, table_offset | table_bar);
> > +    pci_set_long(config + PCI_MSIX_PBA, pba_offset | pba_bar);
> > +
> > +    dev->msix_cap = config_offset;
> > +
> > +    /* Make flags bit writable. */
> > +    dev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> > +                                                       MSIX_MASKALL_MASK;
> > +
> > +    dev->msix_function_masked = true;
> > +
> > +    return 0;
> > +}
> > +
> > +/* Clean up resources for the device. */
> > +void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
> > +{
> > +    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
> 
> msix_present()

Fixed.

> > +        return;
> > +    }
> > +
> > +    pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> > +    dev->msix_cap = 0;
> > +    dev->msix_entries_nr = 0;
> > +
> > +    memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
> > +    memory_region_destroy(&dev->msix_pba_mmio);
> > +    g_free(dev->msix_pba);
> > +    dev->msix_pba = NULL;
> > +
> > +    memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
> > +    memory_region_destroy(&dev->msix_table_mmio);
> > +    g_free(dev->msix_table);
> > +    dev->msix_table = NULL;
> > +
> > +    g_free(dev->msix_entry_used);
> > +    dev->msix_entry_used = NULL;
> > +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> > +}
> > +
> > +/* Initialize the MSI-X structures */
> >  int msix_init(struct PCIDevice *dev, unsigned short nentries,
> > -              MemoryRegion *bar,
> > -              unsigned bar_nr, unsigned bar_size)
> > +              MemoryRegion *table_bar, uint8_t table_bar_nr,
> > +              unsigned table_offset, MemoryRegion *pba_bar,
> > +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos)
> >  {
> >      int ret;
> >      unsigned table_size, pba_size;
> > @@ -281,43 +279,41 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> >      if (!msi_supported) {
> >          return -ENOTSUP;
> >      }
> > -    if (nentries > MSIX_MAX_ENTRIES)
> > -        return -EINVAL;
> >  
> >      table_size = nentries * PCI_MSIX_ENTRY_SIZE;
> >      pba_size = QEMU_ALIGN_UP(nentries, 64) / 8;
> >  
> > -    dev->msix_entry_used = g_malloc0(MSIX_MAX_ENTRIES *
> > -                                        sizeof *dev->msix_entry_used);
> > +    /* Sanity test: table & pba don't overlap, fit within BARs, min aligned */
> > +    if ((table_bar_nr == pba_bar_nr &&
> > +         ranges_overlap(table_offset, table_size, pba_offset, pba_size)) ||
> > +        table_offset + table_size > memory_region_size(table_bar) ||
> > +        pba_offset + pba_size > memory_region_size(pba_bar) ||
> > +        (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
> > +        return -EINVAL;
> > +    }
> >  
> >      dev->msix_table = g_malloc0(table_size);
> >      dev->msix_pba = g_malloc0(pba_size);
> > +    dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
> > +    dev->msix_entries_nr = nentries;
> > +    dev->cap_present |= QEMU_PCI_CAP_MSIX;
> > +
> >      msix_mask_all(dev, nentries);
> >  
> >      memory_region_init_io(&dev->msix_table_mmio, &msix_table_mmio_ops, dev,
> >                            "msix", table_size);
> > +    memory_region_add_subregion(table_bar, table_offset, &dev->msix_table_mmio);
> > +
> >      memory_region_init_io(&dev->msix_pba_mmio, &msix_pba_mmio_ops, dev,
> >                            "msix-pba", pba_size);
> > +    memory_region_add_subregion(pba_bar, pba_offset, &dev->msix_pba_mmio);
> >  
> > -    dev->msix_entries_nr = nentries;
> > -    ret = msix_add_config(dev, nentries, bar_nr, bar_size);
> > -    if (ret)
> > -        goto err_config;
> > -
> > -    dev->cap_present |= QEMU_PCI_CAP_MSIX;
> > -    msix_mmio_setup(dev, bar);
> > -    return 0;
> > +    ret = msix_add_config(dev, nentries, table_bar_nr, table_offset,
> > +                          pba_bar_nr, pba_offset, cap_pos);
> > +    if (ret) {
> > +        msix_uninit(dev, table_bar, pba_bar);
> > +    }
> >  
> > -err_config:
> > -    dev->msix_entries_nr = 0;
> > -    memory_region_destroy(&dev->msix_pba_mmio);
> > -    g_free(dev->msix_pba);
> > -    dev->msix_pba = NULL;
> > -    memory_region_destroy(&dev->msix_table_mmio);
> > -    g_free(dev->msix_table);
> > -    dev->msix_table = NULL;
> > -    g_free(dev->msix_entry_used);
> > -    dev->msix_entry_used = NULL;
> >      return ret;
> >  }
> >  
> > @@ -344,7 +340,8 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >  
> >      free(name);
> >  
> > -    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
> > +    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 0,
> > +                    &dev->msix_exclusive_bar, bar_nr, 2048, 0);
> 
> Doesn't this have to be msix_table_mmi and msix_pba_mmio?

Nope, msix_exclusive_bar is the container, msix_table_mmio and
msix_pba_mmio are the subregions for their respective types.

> >      if (ret) {
> >          memory_region_destroy(&dev->msix_exclusive_bar);
> >          return ret;
> > @@ -356,6 +353,14 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >      return 0;
> >  }
> >  
> > +void msix_uninit_exclusive_bar(PCIDevice *dev)
> > +{
> > +    if (msix_present(dev)) {
> > +        msix_uninit(dev, &dev->msix_exclusive_bar, &dev->msix_exclusive_bar);
> 
> Same here.

Same as above.

> > +        memory_region_destroy(&dev->msix_exclusive_bar);
> > +    }
> > +}
> > +
> >  static void msix_free_irq_entries(PCIDevice *dev)
> >  {
> >      int vector;
> > @@ -366,38 +371,6 @@ static void msix_free_irq_entries(PCIDevice *dev)
> >      }
> >  }
> >  
> > -/* Clean up resources for the device. */
> > -int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
> > -{
> > -    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);
> > -    dev->msix_entries_nr = 0;
> > -    memory_region_del_subregion(bar, &dev->msix_pba_mmio);
> > -    memory_region_destroy(&dev->msix_pba_mmio);
> > -    g_free(dev->msix_pba);
> > -    dev->msix_pba = NULL;
> > -    memory_region_del_subregion(bar, &dev->msix_table_mmio);
> > -    memory_region_destroy(&dev->msix_table_mmio);
> > -    g_free(dev->msix_table);
> > -    dev->msix_table = NULL;
> > -    g_free(dev->msix_entry_used);
> > -    dev->msix_entry_used = NULL;
> > -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> > -    return 0;
> > -}
> > -
> > -void msix_uninit_exclusive_bar(PCIDevice *dev)
> > -{
> > -    if (msix_present(dev)) {
> > -        msix_uninit(dev, &dev->msix_exclusive_bar);
> > -        memory_region_destroy(&dev->msix_exclusive_bar);
> > -    }
> > -}
> > -
> >  void msix_save(PCIDevice *dev, QEMUFile *f)
> >  {
> >      unsigned n = dev->msix_entries_nr;
> > diff --git a/hw/msix.h b/hw/msix.h
> > index bed6bfb..14b1a2e 100644
> > --- a/hw/msix.h
> > +++ b/hw/msix.h
> > @@ -4,16 +4,18 @@
> >  #include "qemu-common.h"
> >  #include "pci.h"
> >  
> > -int msix_init(PCIDevice *pdev, unsigned short nentries,
> > -              MemoryRegion *bar,
> > -              unsigned bar_nr, unsigned bar_size);
> > +int msix_init(PCIDevice *dev, unsigned short nentries,
> > +              MemoryRegion *table_bar, uint8_t table_bar_nr,
> > +              unsigned table_offset, MemoryRegion *pba_bar,
> > +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
> >  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >                              uint8_t bar_nr);
> >  
> >  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
> >                         uint32_t val, int len);
> >  
> > -int msix_uninit(PCIDevice *d, MemoryRegion *bar);
> > +void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar,
> > +                 MemoryRegion *pba_bar);
> >  void msix_uninit_exclusive_bar(PCIDevice *dev);
> >  
> >  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
> > 
> 
> Interfaces look good to me, the logic as well - except for the few remarks.

Thanks!

Alex

Re: [Qemu-devel] [PATCH v2 5/6] msix: Allow full specification of MSIX layout

[Michael S. Tsirkin]

On Wed, Jun 13, 2012 at 10:51:56PM -0600, Alex Williamson wrote:
> Finally, complete the fully specified interface.  msix_add_config()
> gets moved to be closer to the setup functions where it's actually
> used.  msix_mmio_setup() gets folded into msix_init().  And
> msix_uninit() gets reworked a bit so we can call it as cleanup
> from msix_init().
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Some review comments seem to have been ignored: this patch -
uses hardcoded constants, still has duplicated parameters,
move functions in the same patch as adding functionality.
Do you intend to send another revision?

> ---
> 
>  hw/msix.c |  217 +++++++++++++++++++++++++++----------------------------------
>  hw/msix.h |   10 ++-
>  2 files changed, 101 insertions(+), 126 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index d476d07..047646a 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -27,14 +27,6 @@
>  #define MSIX_ENABLE_MASK (PCI_MSIX_FLAGS_ENABLE >> 8)
>  #define MSIX_MASKALL_MASK (PCI_MSIX_FLAGS_MASKALL >> 8)
>  
> -/* How much space does an MSIX table need. */
> -/* The spec requires giving the table structure
> - * a 4K aligned region all by itself. */
> -#define MSIX_PAGE_SIZE 0x1000
> -/* Reserve second half of the page for pending bits */
> -#define MSIX_PAGE_PENDING (MSIX_PAGE_SIZE / 2)
> -#define MSIX_MAX_ENTRIES 32
> -
>  static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
>  {
>      uint8_t *table_entry = dev->msix_table + vector * PCI_MSIX_ENTRY_SIZE;
> @@ -45,47 +37,6 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
>      return msg;
>  }
>  
> -/* Add MSI-X capability to the config space for the device. */
> -/* Given a bar and its size, add MSI-X table on top of it
> - * and fill MSI-X capability in the config space.
> - * Original bar size must be a power of 2 or 0.
> - * New bar size is returned. */
> -static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> -                           unsigned bar_nr, unsigned bar_size)
> -{
> -    int config_offset;
> -    uint8_t *config;
> -
> -    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> -        return -EINVAL;
> -    if (bar_size > 0x80000000)
> -        return -ENOSPC;
> -
> -    /* Require aligned offset for MSI-X structures */
> -    if (bar_size & ~(MSIX_PAGE_SIZE - 1)) {
> -        return -EINVAL;
> -    }
> -
> -    config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
> -                                       0, MSIX_CAP_LENGTH);
> -    if (config_offset < 0)
> -        return config_offset;
> -    config = pdev->config + config_offset;
> -
> -    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> -    /* Table on top of BAR */
> -    pci_set_long(config + PCI_MSIX_TABLE, bar_size | bar_nr);
> -    /* Pending bits on top of that */
> -    pci_set_long(config + PCI_MSIX_PBA, (bar_size + MSIX_PAGE_PENDING) |
> -                 bar_nr);
> -    pdev->msix_cap = config_offset;
> -    /* Make flags bit writable. */
> -    pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> -	    MSIX_MASKALL_MASK;
> -    pdev->msix_function_masked = true;
> -    return 0;
> -}
> -
>  static uint8_t msix_pending_mask(int vector)
>  {
>      return 1 << (vector % 8);
> @@ -240,20 +191,6 @@ static const MemoryRegionOps msix_pba_mmio_ops = {
>      },
>  };
>  
> -static void msix_mmio_setup(PCIDevice *d, MemoryRegion *bar)
> -{
> -    uint8_t *config = d->config + d->msix_cap;
> -    uint32_t table = pci_get_long(config + PCI_MSIX_TABLE);
> -    uint32_t table_offset = table & ~PCI_MSIX_FLAGS_BIRMASK;
> -    uint32_t pba = pci_get_long(config + PCI_MSIX_PBA);
> -    uint32_t pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
> -    /* TODO: for assigned devices, we'll want to make it possible to map
> -     * pending bits separately in case they are in a separate bar. */
> -
> -    memory_region_add_subregion(bar, table_offset, &d->msix_table_mmio);
> -    memory_region_add_subregion(bar, pba_offset, &d->msix_pba_mmio);
> -}
> -
>  static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>  {
>      int vector;
> @@ -268,11 +205,72 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>      }
>  }
>  
> -/* Initialize the MSI-X structures. Note: if MSI-X is supported, BAR size is
> - * modified, it should be retrieved with msix_bar_size. */
> +/* Add MSI-X capability to the config space for the device. */
> +static int msix_add_config(struct PCIDevice *dev, unsigned short nentries,
> +                           uint8_t table_bar, unsigned table_offset,
> +                           uint8_t pba_bar, unsigned pba_offset, uint8_t pos)
> +{
> +    int config_offset;
> +    uint8_t *config;
> +
> +    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
> +        return -EINVAL;
> +    }
> +
> +    config_offset = pci_add_capability(dev, PCI_CAP_ID_MSIX,
> +                                       pos, MSIX_CAP_LENGTH);
> +    if (config_offset < 0) {
> +        return config_offset;
> +    }
> +
> +    config = dev->config + config_offset;
> +
> +    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> +    pci_set_long(config + PCI_MSIX_TABLE, table_offset | table_bar);
> +    pci_set_long(config + PCI_MSIX_PBA, pba_offset | pba_bar);
> +
> +    dev->msix_cap = config_offset;
> +
> +    /* Make flags bit writable. */
> +    dev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> +                                                       MSIX_MASKALL_MASK;
> +
> +    dev->msix_function_masked = true;
> +
> +    return 0;
> +}
> +
> +/* Clean up resources for the device. */
> +void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
> +{
> +    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
> +        return;
> +    }
> +
> +    pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> +    dev->msix_cap = 0;
> +    dev->msix_entries_nr = 0;
> +
> +    memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
> +    memory_region_destroy(&dev->msix_pba_mmio);
> +    g_free(dev->msix_pba);
> +    dev->msix_pba = NULL;
> +
> +    memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
> +    memory_region_destroy(&dev->msix_table_mmio);
> +    g_free(dev->msix_table);
> +    dev->msix_table = NULL;
> +
> +    g_free(dev->msix_entry_used);
> +    dev->msix_entry_used = NULL;
> +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> +}
> +
> +/* Initialize the MSI-X structures */
>  int msix_init(struct PCIDevice *dev, unsigned short nentries,
> -              MemoryRegion *bar,
> -              unsigned bar_nr, unsigned bar_size)
> +              MemoryRegion *table_bar, uint8_t table_bar_nr,
> +              unsigned table_offset, MemoryRegion *pba_bar,
> +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos)
>  {
>      int ret;
>      unsigned table_size, pba_size;
> @@ -281,43 +279,41 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>      if (!msi_supported) {
>          return -ENOTSUP;
>      }
> -    if (nentries > MSIX_MAX_ENTRIES)
> -        return -EINVAL;
>  
>      table_size = nentries * PCI_MSIX_ENTRY_SIZE;
>      pba_size = QEMU_ALIGN_UP(nentries, 64) / 8;
>  
> -    dev->msix_entry_used = g_malloc0(MSIX_MAX_ENTRIES *
> -                                        sizeof *dev->msix_entry_used);
> +    /* Sanity test: table & pba don't overlap, fit within BARs, min aligned */
> +    if ((table_bar_nr == pba_bar_nr &&
> +         ranges_overlap(table_offset, table_size, pba_offset, pba_size)) ||
> +        table_offset + table_size > memory_region_size(table_bar) ||
> +        pba_offset + pba_size > memory_region_size(pba_bar) ||
> +        (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
> +        return -EINVAL;
> +    }
>  
>      dev->msix_table = g_malloc0(table_size);
>      dev->msix_pba = g_malloc0(pba_size);
> +    dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
> +    dev->msix_entries_nr = nentries;
> +    dev->cap_present |= QEMU_PCI_CAP_MSIX;
> +
>      msix_mask_all(dev, nentries);
>  
>      memory_region_init_io(&dev->msix_table_mmio, &msix_table_mmio_ops, dev,
>                            "msix", table_size);
> +    memory_region_add_subregion(table_bar, table_offset, &dev->msix_table_mmio);
> +
>      memory_region_init_io(&dev->msix_pba_mmio, &msix_pba_mmio_ops, dev,
>                            "msix-pba", pba_size);
> +    memory_region_add_subregion(pba_bar, pba_offset, &dev->msix_pba_mmio);
>  
> -    dev->msix_entries_nr = nentries;
> -    ret = msix_add_config(dev, nentries, bar_nr, bar_size);
> -    if (ret)
> -        goto err_config;
> -
> -    dev->cap_present |= QEMU_PCI_CAP_MSIX;
> -    msix_mmio_setup(dev, bar);
> -    return 0;
> +    ret = msix_add_config(dev, nentries, table_bar_nr, table_offset,
> +                          pba_bar_nr, pba_offset, cap_pos);
> +    if (ret) {
> +        msix_uninit(dev, table_bar, pba_bar);
> +    }
>  
> -err_config:
> -    dev->msix_entries_nr = 0;
> -    memory_region_destroy(&dev->msix_pba_mmio);
> -    g_free(dev->msix_pba);
> -    dev->msix_pba = NULL;
> -    memory_region_destroy(&dev->msix_table_mmio);
> -    g_free(dev->msix_table);
> -    dev->msix_table = NULL;
> -    g_free(dev->msix_entry_used);
> -    dev->msix_entry_used = NULL;
>      return ret;
>  }
>  
> @@ -344,7 +340,8 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>  
>      free(name);
>  
> -    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
> +    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 0,
> +                    &dev->msix_exclusive_bar, bar_nr, 2048, 0);
>      if (ret) {
>          memory_region_destroy(&dev->msix_exclusive_bar);
>          return ret;
> @@ -356,6 +353,14 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>      return 0;
>  }
>  
> +void msix_uninit_exclusive_bar(PCIDevice *dev)
> +{
> +    if (msix_present(dev)) {
> +        msix_uninit(dev, &dev->msix_exclusive_bar, &dev->msix_exclusive_bar);
> +        memory_region_destroy(&dev->msix_exclusive_bar);
> +    }
> +}
> +
>  static void msix_free_irq_entries(PCIDevice *dev)
>  {
>      int vector;
> @@ -366,38 +371,6 @@ static void msix_free_irq_entries(PCIDevice *dev)
>      }
>  }
>  
> -/* Clean up resources for the device. */
> -int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
> -{
> -    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);
> -    dev->msix_entries_nr = 0;
> -    memory_region_del_subregion(bar, &dev->msix_pba_mmio);
> -    memory_region_destroy(&dev->msix_pba_mmio);
> -    g_free(dev->msix_pba);
> -    dev->msix_pba = NULL;
> -    memory_region_del_subregion(bar, &dev->msix_table_mmio);
> -    memory_region_destroy(&dev->msix_table_mmio);
> -    g_free(dev->msix_table);
> -    dev->msix_table = NULL;
> -    g_free(dev->msix_entry_used);
> -    dev->msix_entry_used = NULL;
> -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> -    return 0;
> -}
> -
> -void msix_uninit_exclusive_bar(PCIDevice *dev)
> -{
> -    if (msix_present(dev)) {
> -        msix_uninit(dev, &dev->msix_exclusive_bar);
> -        memory_region_destroy(&dev->msix_exclusive_bar);
> -    }
> -}
> -
>  void msix_save(PCIDevice *dev, QEMUFile *f)
>  {
>      unsigned n = dev->msix_entries_nr;
> diff --git a/hw/msix.h b/hw/msix.h
> index bed6bfb..14b1a2e 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -4,16 +4,18 @@
>  #include "qemu-common.h"
>  #include "pci.h"
>  
> -int msix_init(PCIDevice *pdev, unsigned short nentries,
> -              MemoryRegion *bar,
> -              unsigned bar_nr, unsigned bar_size);
> +int msix_init(PCIDevice *dev, unsigned short nentries,
> +              MemoryRegion *table_bar, uint8_t table_bar_nr,
> +              unsigned table_offset, MemoryRegion *pba_bar,
> +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>                              uint8_t bar_nr);
>  
>  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
>                         uint32_t val, int len);
>  
> -int msix_uninit(PCIDevice *d, MemoryRegion *bar);
> +void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar,
> +                 MemoryRegion *pba_bar);
>  void msix_uninit_exclusive_bar(PCIDevice *dev);
>  
>  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);

Re: [Qemu-devel] [PATCH v2 5/6] msix: Allow full specification of MSIX layout

[Alex Williamson]

On Thu, 2012-06-14 at 11:10 +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 13, 2012 at 10:51:56PM -0600, Alex Williamson wrote:
> > Finally, complete the fully specified interface.  msix_add_config()
> > gets moved to be closer to the setup functions where it's actually
> > used.  msix_mmio_setup() gets folded into msix_init().  And
> > msix_uninit() gets reworked a bit so we can call it as cleanup
> > from msix_init().
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Some review comments seem to have been ignored: this patch -
> uses hardcoded constants, still has duplicated parameters,
> move functions in the same patch as adding functionality.
> Do you intend to send another revision?

The comments weren't ignored, I replied to them.  Please check your
mail.  In the interim, I sent a new series.  Thanks,

Alex

> > ---
> > 
> >  hw/msix.c |  217 +++++++++++++++++++++++++++----------------------------------
> >  hw/msix.h |   10 ++-
> >  2 files changed, 101 insertions(+), 126 deletions(-)
> > 
> > diff --git a/hw/msix.c b/hw/msix.c
> > index d476d07..047646a 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -27,14 +27,6 @@
> >  #define MSIX_ENABLE_MASK (PCI_MSIX_FLAGS_ENABLE >> 8)
> >  #define MSIX_MASKALL_MASK (PCI_MSIX_FLAGS_MASKALL >> 8)
> >  
> > -/* How much space does an MSIX table need. */
> > -/* The spec requires giving the table structure
> > - * a 4K aligned region all by itself. */
> > -#define MSIX_PAGE_SIZE 0x1000
> > -/* Reserve second half of the page for pending bits */
> > -#define MSIX_PAGE_PENDING (MSIX_PAGE_SIZE / 2)
> > -#define MSIX_MAX_ENTRIES 32
> > -
> >  static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
> >  {
> >      uint8_t *table_entry = dev->msix_table + vector * PCI_MSIX_ENTRY_SIZE;
> > @@ -45,47 +37,6 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
> >      return msg;
> >  }
> >  
> > -/* Add MSI-X capability to the config space for the device. */
> > -/* Given a bar and its size, add MSI-X table on top of it
> > - * and fill MSI-X capability in the config space.
> > - * Original bar size must be a power of 2 or 0.
> > - * New bar size is returned. */
> > -static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> > -                           unsigned bar_nr, unsigned bar_size)
> > -{
> > -    int config_offset;
> > -    uint8_t *config;
> > -
> > -    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> > -        return -EINVAL;
> > -    if (bar_size > 0x80000000)
> > -        return -ENOSPC;
> > -
> > -    /* Require aligned offset for MSI-X structures */
> > -    if (bar_size & ~(MSIX_PAGE_SIZE - 1)) {
> > -        return -EINVAL;
> > -    }
> > -
> > -    config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
> > -                                       0, MSIX_CAP_LENGTH);
> > -    if (config_offset < 0)
> > -        return config_offset;
> > -    config = pdev->config + config_offset;
> > -
> > -    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > -    /* Table on top of BAR */
> > -    pci_set_long(config + PCI_MSIX_TABLE, bar_size | bar_nr);
> > -    /* Pending bits on top of that */
> > -    pci_set_long(config + PCI_MSIX_PBA, (bar_size + MSIX_PAGE_PENDING) |
> > -                 bar_nr);
> > -    pdev->msix_cap = config_offset;
> > -    /* Make flags bit writable. */
> > -    pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> > -	    MSIX_MASKALL_MASK;
> > -    pdev->msix_function_masked = true;
> > -    return 0;
> > -}
> > -
> >  static uint8_t msix_pending_mask(int vector)
> >  {
> >      return 1 << (vector % 8);
> > @@ -240,20 +191,6 @@ static const MemoryRegionOps msix_pba_mmio_ops = {
> >      },
> >  };
> >  
> > -static void msix_mmio_setup(PCIDevice *d, MemoryRegion *bar)
> > -{
> > -    uint8_t *config = d->config + d->msix_cap;
> > -    uint32_t table = pci_get_long(config + PCI_MSIX_TABLE);
> > -    uint32_t table_offset = table & ~PCI_MSIX_FLAGS_BIRMASK;
> > -    uint32_t pba = pci_get_long(config + PCI_MSIX_PBA);
> > -    uint32_t pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
> > -    /* TODO: for assigned devices, we'll want to make it possible to map
> > -     * pending bits separately in case they are in a separate bar. */
> > -
> > -    memory_region_add_subregion(bar, table_offset, &d->msix_table_mmio);
> > -    memory_region_add_subregion(bar, pba_offset, &d->msix_pba_mmio);
> > -}
> > -
> >  static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
> >  {
> >      int vector;
> > @@ -268,11 +205,72 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
> >      }
> >  }
> >  
> > -/* Initialize the MSI-X structures. Note: if MSI-X is supported, BAR size is
> > - * modified, it should be retrieved with msix_bar_size. */
> > +/* Add MSI-X capability to the config space for the device. */
> > +static int msix_add_config(struct PCIDevice *dev, unsigned short nentries,
> > +                           uint8_t table_bar, unsigned table_offset,
> > +                           uint8_t pba_bar, unsigned pba_offset, uint8_t pos)
> > +{
> > +    int config_offset;
> > +    uint8_t *config;
> > +
> > +    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
> > +        return -EINVAL;
> > +    }
> > +
> > +    config_offset = pci_add_capability(dev, PCI_CAP_ID_MSIX,
> > +                                       pos, MSIX_CAP_LENGTH);
> > +    if (config_offset < 0) {
> > +        return config_offset;
> > +    }
> > +
> > +    config = dev->config + config_offset;
> > +
> > +    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> > +    pci_set_long(config + PCI_MSIX_TABLE, table_offset | table_bar);
> > +    pci_set_long(config + PCI_MSIX_PBA, pba_offset | pba_bar);
> > +
> > +    dev->msix_cap = config_offset;
> > +
> > +    /* Make flags bit writable. */
> > +    dev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
> > +                                                       MSIX_MASKALL_MASK;
> > +
> > +    dev->msix_function_masked = true;
> > +
> > +    return 0;
> > +}
> > +
> > +/* Clean up resources for the device. */
> > +void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
> > +{
> > +    if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) {
> > +        return;
> > +    }
> > +
> > +    pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH);
> > +    dev->msix_cap = 0;
> > +    dev->msix_entries_nr = 0;
> > +
> > +    memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
> > +    memory_region_destroy(&dev->msix_pba_mmio);
> > +    g_free(dev->msix_pba);
> > +    dev->msix_pba = NULL;
> > +
> > +    memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
> > +    memory_region_destroy(&dev->msix_table_mmio);
> > +    g_free(dev->msix_table);
> > +    dev->msix_table = NULL;
> > +
> > +    g_free(dev->msix_entry_used);
> > +    dev->msix_entry_used = NULL;
> > +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> > +}
> > +
> > +/* Initialize the MSI-X structures */
> >  int msix_init(struct PCIDevice *dev, unsigned short nentries,
> > -              MemoryRegion *bar,
> > -              unsigned bar_nr, unsigned bar_size)
> > +              MemoryRegion *table_bar, uint8_t table_bar_nr,
> > +              unsigned table_offset, MemoryRegion *pba_bar,
> > +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos)
> >  {
> >      int ret;
> >      unsigned table_size, pba_size;
> > @@ -281,43 +279,41 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> >      if (!msi_supported) {
> >          return -ENOTSUP;
> >      }
> > -    if (nentries > MSIX_MAX_ENTRIES)
> > -        return -EINVAL;
> >  
> >      table_size = nentries * PCI_MSIX_ENTRY_SIZE;
> >      pba_size = QEMU_ALIGN_UP(nentries, 64) / 8;
> >  
> > -    dev->msix_entry_used = g_malloc0(MSIX_MAX_ENTRIES *
> > -                                        sizeof *dev->msix_entry_used);
> > +    /* Sanity test: table & pba don't overlap, fit within BARs, min aligned */
> > +    if ((table_bar_nr == pba_bar_nr &&
> > +         ranges_overlap(table_offset, table_size, pba_offset, pba_size)) ||
> > +        table_offset + table_size > memory_region_size(table_bar) ||
> > +        pba_offset + pba_size > memory_region_size(pba_bar) ||
> > +        (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
> > +        return -EINVAL;
> > +    }
> >  
> >      dev->msix_table = g_malloc0(table_size);
> >      dev->msix_pba = g_malloc0(pba_size);
> > +    dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
> > +    dev->msix_entries_nr = nentries;
> > +    dev->cap_present |= QEMU_PCI_CAP_MSIX;
> > +
> >      msix_mask_all(dev, nentries);
> >  
> >      memory_region_init_io(&dev->msix_table_mmio, &msix_table_mmio_ops, dev,
> >                            "msix", table_size);
> > +    memory_region_add_subregion(table_bar, table_offset, &dev->msix_table_mmio);
> > +
> >      memory_region_init_io(&dev->msix_pba_mmio, &msix_pba_mmio_ops, dev,
> >                            "msix-pba", pba_size);
> > +    memory_region_add_subregion(pba_bar, pba_offset, &dev->msix_pba_mmio);
> >  
> > -    dev->msix_entries_nr = nentries;
> > -    ret = msix_add_config(dev, nentries, bar_nr, bar_size);
> > -    if (ret)
> > -        goto err_config;
> > -
> > -    dev->cap_present |= QEMU_PCI_CAP_MSIX;
> > -    msix_mmio_setup(dev, bar);
> > -    return 0;
> > +    ret = msix_add_config(dev, nentries, table_bar_nr, table_offset,
> > +                          pba_bar_nr, pba_offset, cap_pos);
> > +    if (ret) {
> > +        msix_uninit(dev, table_bar, pba_bar);
> > +    }
> >  
> > -err_config:
> > -    dev->msix_entries_nr = 0;
> > -    memory_region_destroy(&dev->msix_pba_mmio);
> > -    g_free(dev->msix_pba);
> > -    dev->msix_pba = NULL;
> > -    memory_region_destroy(&dev->msix_table_mmio);
> > -    g_free(dev->msix_table);
> > -    dev->msix_table = NULL;
> > -    g_free(dev->msix_entry_used);
> > -    dev->msix_entry_used = NULL;
> >      return ret;
> >  }
> >  
> > @@ -344,7 +340,8 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >  
> >      free(name);
> >  
> > -    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096);
> > +    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 0,
> > +                    &dev->msix_exclusive_bar, bar_nr, 2048, 0);
> >      if (ret) {
> >          memory_region_destroy(&dev->msix_exclusive_bar);
> >          return ret;
> > @@ -356,6 +353,14 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >      return 0;
> >  }
> >  
> > +void msix_uninit_exclusive_bar(PCIDevice *dev)
> > +{
> > +    if (msix_present(dev)) {
> > +        msix_uninit(dev, &dev->msix_exclusive_bar, &dev->msix_exclusive_bar);
> > +        memory_region_destroy(&dev->msix_exclusive_bar);
> > +    }
> > +}
> > +
> >  static void msix_free_irq_entries(PCIDevice *dev)
> >  {
> >      int vector;
> > @@ -366,38 +371,6 @@ static void msix_free_irq_entries(PCIDevice *dev)
> >      }
> >  }
> >  
> > -/* Clean up resources for the device. */
> > -int msix_uninit(PCIDevice *dev, MemoryRegion *bar)
> > -{
> > -    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);
> > -    dev->msix_entries_nr = 0;
> > -    memory_region_del_subregion(bar, &dev->msix_pba_mmio);
> > -    memory_region_destroy(&dev->msix_pba_mmio);
> > -    g_free(dev->msix_pba);
> > -    dev->msix_pba = NULL;
> > -    memory_region_del_subregion(bar, &dev->msix_table_mmio);
> > -    memory_region_destroy(&dev->msix_table_mmio);
> > -    g_free(dev->msix_table);
> > -    dev->msix_table = NULL;
> > -    g_free(dev->msix_entry_used);
> > -    dev->msix_entry_used = NULL;
> > -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> > -    return 0;
> > -}
> > -
> > -void msix_uninit_exclusive_bar(PCIDevice *dev)
> > -{
> > -    if (msix_present(dev)) {
> > -        msix_uninit(dev, &dev->msix_exclusive_bar);
> > -        memory_region_destroy(&dev->msix_exclusive_bar);
> > -    }
> > -}
> > -
> >  void msix_save(PCIDevice *dev, QEMUFile *f)
> >  {
> >      unsigned n = dev->msix_entries_nr;
> > diff --git a/hw/msix.h b/hw/msix.h
> > index bed6bfb..14b1a2e 100644
> > --- a/hw/msix.h
> > +++ b/hw/msix.h
> > @@ -4,16 +4,18 @@
> >  #include "qemu-common.h"
> >  #include "pci.h"
> >  
> > -int msix_init(PCIDevice *pdev, unsigned short nentries,
> > -              MemoryRegion *bar,
> > -              unsigned bar_nr, unsigned bar_size);
> > +int msix_init(PCIDevice *dev, unsigned short nentries,
> > +              MemoryRegion *table_bar, uint8_t table_bar_nr,
> > +              unsigned table_offset, MemoryRegion *pba_bar,
> > +              uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos);
> >  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >                              uint8_t bar_nr);
> >  
> >  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
> >                         uint32_t val, int len);
> >  
> > -int msix_uninit(PCIDevice *d, MemoryRegion *bar);
> > +void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar,
> > +                 MemoryRegion *pba_bar);
> >  void msix_uninit_exclusive_bar(PCIDevice *dev);
> >  
> >  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);

[Qemu-devel] [PATCH v2 6/6] msix: Fix last PCIDevice naming inconsitency

[Alex Williamson]

The previous patches fixed almost all the inconsistent names used
for PCIDevice in msix.c, fix the one remaining transgression.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/msix.h |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/msix.h b/hw/msix.h
index 14b1a2e..1786e27 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -11,8 +11,7 @@ int msix_init(PCIDevice *dev, unsigned short nentries,
 int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
                             uint8_t bar_nr);
 
-void msix_write_config(PCIDevice *pci_dev, uint32_t address,
-                       uint32_t val, int len);
+void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
 
 void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar,
                  MemoryRegion *pba_bar);

Re: [Qemu-devel] [PATCH v2 6/6] msix: Fix last PCIDevice naming inconsitency

[Michael S. Tsirkin]

On Wed, Jun 13, 2012 at 10:52:06PM -0600, Alex Williamson wrote:
> The previous patches fixed almost all the inconsistent names used
> for PCIDevice in msix.c, fix the one remaining transgression.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Except, would be better to first rename parameters and
do other mechanical changes, and then apply your diff
on top.

As it is the patch looks like it rewrites everything while
it could be a very small one.

> ---
> 
>  hw/msix.h |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/msix.h b/hw/msix.h
> index 14b1a2e..1786e27 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -11,8 +11,7 @@ int msix_init(PCIDevice *dev, unsigned short nentries,
>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>                              uint8_t bar_nr);
>  
> -void msix_write_config(PCIDevice *pci_dev, uint32_t address,
> -                       uint32_t val, int len);
> +void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
>  
>  void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar,
>                   MemoryRegion *pba_bar);

Re: [Qemu-devel] [PATCH v2 6/6] msix: Fix last PCIDevice naming inconsitency

[Alex Williamson]

On Thu, 2012-06-14 at 11:13 +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 13, 2012 at 10:52:06PM -0600, Alex Williamson wrote:
> > The previous patches fixed almost all the inconsistent names used
> > for PCIDevice in msix.c, fix the one remaining transgression.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Except, would be better to first rename parameters and
> do other mechanical changes, and then apply your diff
> on top.
> 
> As it is the patch looks like it rewrites everything while
> it could be a very small one.

You're welcome for trying to add consistency, sorry I pointed it out.

> > ---
> > 
> >  hw/msix.h |    3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/hw/msix.h b/hw/msix.h
> > index 14b1a2e..1786e27 100644
> > --- a/hw/msix.h
> > +++ b/hw/msix.h
> > @@ -11,8 +11,7 @@ int msix_init(PCIDevice *dev, unsigned short nentries,
> >  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> >                              uint8_t bar_nr);
> >  
> > -void msix_write_config(PCIDevice *pci_dev, uint32_t address,
> > -                       uint32_t val, int len);
> > +void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
> >  
> >  void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar,
> >                   MemoryRegion *pba_bar);

Re: [Qemu-devel] [PATCH v2 6/6] msix: Fix last PCIDevice naming inconsitency

[Michael S. Tsirkin]

On Thu, Jun 14, 2012 at 08:18:10AM -0600, Alex Williamson wrote:
> On Thu, 2012-06-14 at 11:13 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 13, 2012 at 10:52:06PM -0600, Alex Williamson wrote:
> > > The previous patches fixed almost all the inconsistent names used
> > > for PCIDevice in msix.c, fix the one remaining transgression.
> > > 
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > 
> > Except, would be better to first rename parameters and
> > do other mechanical changes, and then apply your diff
> > on top.
> > 
> > As it is the patch looks like it rewrites everything while
> > it could be a very small one.
> 
> You're welcome for trying to add consistency, sorry I pointed it out.

No problem with this patch itself though.

> > > ---
> > > 
> > >  hw/msix.h |    3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/msix.h b/hw/msix.h
> > > index 14b1a2e..1786e27 100644
> > > --- a/hw/msix.h
> > > +++ b/hw/msix.h
> > > @@ -11,8 +11,7 @@ int msix_init(PCIDevice *dev, unsigned short nentries,
> > >  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> > >                              uint8_t bar_nr);
> > >  
> > > -void msix_write_config(PCIDevice *pci_dev, uint32_t address,
> > > -                       uint32_t val, int len);
> > > +void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
> > >  
> > >  void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar,
> > >                   MemoryRegion *pba_bar);
> 
> 

Re: [Qemu-devel] [PATCH v2 6/6] msix: Fix last PCIDevice naming inconsitency

[Michael S. Tsirkin]

On Thu, Jun 14, 2012 at 08:18:10AM -0600, Alex Williamson wrote:
> On Thu, 2012-06-14 at 11:13 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 13, 2012 at 10:52:06PM -0600, Alex Williamson wrote:
> > > The previous patches fixed almost all the inconsistent names used
> > > for PCIDevice in msix.c, fix the one remaining transgression.
> > > 
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > 
> > Except, would be better to first rename parameters and
> > do other mechanical changes, and then apply your diff
> > on top.
> > 
> > As it is the patch looks like it rewrites everything while
> > it could be a very small one.
> 
> You're welcome for trying to add consistency, sorry I pointed it out.

Typo: patch should have been "patch set".
I was talking about the rest of the changes, this patch looks fine
actually. Sorry about the confusion.


> > > ---
> > > 
> > >  hw/msix.h |    3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/msix.h b/hw/msix.h
> > > index 14b1a2e..1786e27 100644
> > > --- a/hw/msix.h
> > > +++ b/hw/msix.h
> > > @@ -11,8 +11,7 @@ int msix_init(PCIDevice *dev, unsigned short nentries,
> > >  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
> > >                              uint8_t bar_nr);
> > >  
> > > -void msix_write_config(PCIDevice *pci_dev, uint32_t address,
> > > -                       uint32_t val, int len);
> > > +void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
> > >  
> > >  void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar,
> > >                   MemoryRegion *pba_bar);
> 
> 

Re: [Qemu-devel] [PATCH v2 6/6] msix: Fix last PCIDevice naming inconsitency

[Michael S. Tsirkin]

On Wed, Jun 13, 2012 at 10:52:06PM -0600, Alex Williamson wrote:
> The previous patches fixed almost all the inconsistent names used
> for PCIDevice in msix.c, fix the one remaining transgression.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Tweaked the commit log as it's not the last patch in
the commit stream anymore and applied, thanks.

> ---
> 
>  hw/msix.h |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/msix.h b/hw/msix.h
> index 14b1a2e..1786e27 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -11,8 +11,7 @@ int msix_init(PCIDevice *dev, unsigned short nentries,
>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>                              uint8_t bar_nr);
>  
> -void msix_write_config(PCIDevice *pci_dev, uint32_t address,
> -                       uint32_t val, int len);
> +void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
>  
>  void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar,
>                   MemoryRegion *pba_bar);