[v10,1/8] msix: Rename and create a wrapper

Rename msix_init to msix_validate_and_init, and use it from vfio which
might get a reasonable -EINVAL.  New a wrapper msix_init which assert the
programming error for debug purpose and use it from other devices.

CC: Alex Williamson <alex.williamson@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Marcel Apfelbaum <marcel@redhat.com>
CC: Michael S. Tsirkin <mst@redhat.com>

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci/msix.c         | 30 +++++++++++++++++++++---------
 hw/vfio/pci.c         | 12 ++++++------
 include/hw/pci/msix.h |  5 +++++
 3 files changed, 32 insertions(+), 15 deletions(-)

Uh, two weeks since your posted this already.  I apologize for taking so
long to review.

Cao jin <caoj.fnst@cn.fujitsu.com> writes:

> Rename msix_init to msix_validate_and_init, and use it from vfio which
> might get a reasonable -EINVAL.  New a wrapper msix_init which assert the
> programming error for debug purpose and use it from other devices.
>
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/msix.c         | 30 +++++++++++++++++++++---------
>  hw/vfio/pci.c         | 12 ++++++------
>  include/hw/pci/msix.h |  5 +++++
>  3 files changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index bb54e8b0ac37..2b7541ab2c8d 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -239,6 +239,19 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>      }
>  }
>  
> +/* Just a wrapper to check the return value */
> +int msix_init(struct 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,
> +              Error **errp)
> +{
> +    int ret = msix_validate_and_init(dev, nentries, table_bar, table_bar_nr,
> +            table_offset, pba_bar, pba_bar_nr, pba_offset, cap_pos, errp);
> +
> +    assert(ret != -EINVAL);
> +    return ret;
> +}
>  /*
>   * Make PCI device @dev MSI-X capable
>   * @nentries is the max number of MSI-X vectors that the device support.
> @@ -259,11 +272,11 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>   * also means a programming error, except device assignment, which can check
>   * if a real HW is broken.
>   */
> -int msix_init(struct 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,
> -              Error **errp)
> +int msix_validate_and_init(struct 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, Error **errp)
>  {
>      int cap;
>      unsigned table_size, pba_size;
> @@ -361,10 +374,9 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>      memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev), name, bar_size);
>      g_free(name);
>  
> -    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr,
> -                    0, &dev->msix_exclusive_bar,
> -                    bar_nr, bar_pba_offset,
> -                    0, errp);
> +    ret = msix_validate_and_init(dev, nentries, &dev->msix_exclusive_bar,
> +                                 bar_nr, 0, &dev->msix_exclusive_bar,
> +                                 bar_nr, bar_pba_offset, 0, errp);
>      if (ret) {
>          return ret;
>      }

This change assumes that for the callers of msix_exclusive_bar(),
-EINVAL (capability overlap) is not a programming error.  Is that true?

> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 332f41d6627f..06828b537a75 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1436,12 +1436,12 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>  
>      vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
>                                      sizeof(unsigned long));
> -    ret = msix_init(&vdev->pdev, vdev->msix->entries,
> -                    vdev->bars[vdev->msix->table_bar].region.mem,
> -                    vdev->msix->table_bar, vdev->msix->table_offset,
> -                    vdev->bars[vdev->msix->pba_bar].region.mem,
> -                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos,
> -                    &err);
> +    ret = msix_validate_and_init(&vdev->pdev, vdev->msix->entries,
> +                             vdev->bars[vdev->msix->table_bar].region.mem,
> +                             vdev->msix->table_bar, vdev->msix->table_offset,
> +                             vdev->bars[vdev->msix->pba_bar].region.mem,
> +                             vdev->msix->pba_bar, vdev->msix->pba_offset, pos,
> +                             &err);
>      if (ret < 0) {
>          if (ret == -ENOTSUP) {
>              error_report_err(err);
> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
> index 1f27658d352f..815e59bc96f3 100644
> --- a/include/hw/pci/msix.h
> +++ b/include/hw/pci/msix.h
> @@ -11,6 +11,11 @@ int msix_init(PCIDevice *dev, unsigned short nentries,
>                unsigned table_offset, MemoryRegion *pba_bar,
>                uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
>                Error **errp);
> +int msix_validate_and_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, Error **errp);
>  int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>                              uint8_t bar_nr, Error **errp);

On 03/07/2017 03:44 PM, Markus Armbruster wrote:
> Uh, two weeks since your posted this already.  I apologize for taking so
> long to review.
> 
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> 
>> Rename msix_init to msix_validate_and_init, and use it from vfio which
>> might get a reasonable -EINVAL.  New a wrapper msix_init which assert the
>> programming error for debug purpose and use it from other devices.
>>
>> CC: Alex Williamson <alex.williamson@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Marcel Apfelbaum <marcel@redhat.com>
>> CC: Michael S. Tsirkin <mst@redhat.com>
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>  hw/pci/msix.c         | 30 +++++++++++++++++++++---------
>>  hw/vfio/pci.c         | 12 ++++++------
>>  include/hw/pci/msix.h |  5 +++++
>>  3 files changed, 32 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
>> index bb54e8b0ac37..2b7541ab2c8d 100644
>> --- a/hw/pci/msix.c
>> +++ b/hw/pci/msix.c
>> @@ -239,6 +239,19 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>>      }
>>  }
>>  
>> +/* Just a wrapper to check the return value */
>> +int msix_init(struct 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,
>> +              Error **errp)
>> +{
>> +    int ret = msix_validate_and_init(dev, nentries, table_bar, table_bar_nr,
>> +            table_offset, pba_bar, pba_bar_nr, pba_offset, cap_pos, errp);
>> +
>> +    assert(ret != -EINVAL);
>> +    return ret;
>> +}
>>  /*
>>   * Make PCI device @dev MSI-X capable
>>   * @nentries is the max number of MSI-X vectors that the device support.
>> @@ -259,11 +272,11 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>>   * also means a programming error, except device assignment, which can check
>>   * if a real HW is broken.
>>   */
>> -int msix_init(struct 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,
>> -              Error **errp)
>> +int msix_validate_and_init(struct 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, Error **errp)
>>  {
>>      int cap;
>>      unsigned table_size, pba_size;
>> @@ -361,10 +374,9 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries,
>>      memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev), name, bar_size);
>>      g_free(name);
>>  
>> -    ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr,
>> -                    0, &dev->msix_exclusive_bar,
>> -                    bar_nr, bar_pba_offset,
>> -                    0, errp);
>> +    ret = msix_validate_and_init(dev, nentries, &dev->msix_exclusive_bar,
>> +                                 bar_nr, 0, &dev->msix_exclusive_bar,
>> +                                 bar_nr, bar_pba_offset, 0, errp);
>>      if (ret) {
>>          return ret;
>>      }
> 
> This change assumes that for the callers of msix_exclusive_bar(),
> -EINVAL (capability overlap) is not a programming error.  Is that true?
> 

Oh...it looks as you said. Will revert this part and send a new one
in-reply-to this one.