Patchwork [v2,1/6] msix: Add simple BAR allocation MSIX setup functions

login
register
mail settings
Submitter Alex Williamson
Date June 14, 2012, 4:51 a.m.
Message ID <20120614045118.11034.11599.stgit@bling.home>
Download mbox | patch
Permalink /patch/164809/
State New
Headers show

Comments

Alex Williamson - June 14, 2012, 4:51 a.m.
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(+)
Michael S. Tsirkin - June 14, 2012, 10:29 a.m.
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. */
Alex Williamson - June 14, 2012, 2:24 p.m.
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. */
>
Michael S. Tsirkin - June 14, 2012, 3:49 p.m.
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. */
Jan Kiszka - June 14, 2012, 3:55 p.m.
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
Michael S. Tsirkin - June 14, 2012, 4:05 p.m.
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
Alex Williamson - June 14, 2012, 4:10 p.m.
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...
Jan Kiszka - June 14, 2012, 4:11 p.m.
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
Michael S. Tsirkin - June 14, 2012, 4:31 p.m.
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.
Michael S. Tsirkin - June 14, 2012, 4:35 p.m.
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

Patch

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