| Message ID | 1488011202-32121-2-git-send-email-caoj.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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; } 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);
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(-)