Uh, two weeks since your posted this already. I apologize for taking so long to review.
Cao jin <caoj.f...@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.william...@redhat.com> > CC: Markus Armbruster <arm...@redhat.com> > CC: Marcel Apfelbaum <mar...@redhat.com> > CC: Michael S. Tsirkin <m...@redhat.com> > > Signed-off-by: Cao jin <caoj.f...@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);