| Message ID | 0dc2a0c8e25b13a3a41db75ab192f387a1548c80.1680038771.git.reinette.chatre@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/pci: Support dynamic allocation of MSI-X interrupts | expand |
On Tue, 28 Mar 2023 14:53:29 -0700 Reinette Chatre <reinette.chatre@intel.com> wrote: > User space provides the vector as an unsigned int that is checked > early for validity (vfio_set_irqs_validate_and_prepare()). > > A later negative check of the provided vector is not necessary. > > Remove the negative check and ensure the type used > for the vector is consistent as an unsigned int. > > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > --- > drivers/vfio/pci/vfio_pci_intrs.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c > index 6a9c6a143cc3..3f64ccdce69f 100644 > --- a/drivers/vfio/pci/vfio_pci_intrs.c > +++ b/drivers/vfio/pci/vfio_pci_intrs.c > @@ -317,14 +317,14 @@ static int vfio_msi_enable(struct vfio_pci_core_device *vdev, int nvec, bool msi > } > > static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev, > - int vector, int fd, bool msix) > + unsigned int vector, int fd, bool msix) > { > struct pci_dev *pdev = vdev->pdev; > struct eventfd_ctx *trigger; > int irq, ret; > u16 cmd; > > - if (vector < 0 || vector >= vdev->num_ctx) > + if (vector >= vdev->num_ctx) > return -EINVAL; > > irq = pci_irq_vector(pdev, vector); > @@ -399,7 +399,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev, > static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned start, > unsigned count, int32_t *fds, bool msix) > { > - int i, j, ret = 0; > + int i, ret = 0; > + unsigned int j; > > if (start >= vdev->num_ctx || start + count > vdev->num_ctx) > return -EINVAL; Unfortunately this turns the unwind portion of the function into an infinite loop in the common case when @start is zero: for (--j; j >= (int)start; j--) vfio_msi_set_vector_signal(vdev, j, -1, msix); Thanks, Alex > @@ -420,7 +421,7 @@ static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned start, > static void vfio_msi_disable(struct vfio_pci_core_device *vdev, bool msix) > { > struct pci_dev *pdev = vdev->pdev; > - int i; > + unsigned int i; > u16 cmd; > > for (i = 0; i < vdev->num_ctx; i++) { > @@ -542,7 +543,7 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev, > unsigned index, unsigned start, > unsigned count, uint32_t flags, void *data) > { > - int i; > + unsigned int i; > bool msix = (index == VFIO_PCI_MSIX_IRQ_INDEX) ? true : false; > > if (irq_is(vdev, index) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
Hi Alex, On 3/30/2023 1:26 PM, Alex Williamson wrote: > On Tue, 28 Mar 2023 14:53:29 -0700 > Reinette Chatre <reinette.chatre@intel.com> wrote: ... >> @@ -399,7 +399,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev, >> static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned start, >> unsigned count, int32_t *fds, bool msix) >> { >> - int i, j, ret = 0; >> + int i, ret = 0; >> + unsigned int j; >> >> if (start >= vdev->num_ctx || start + count > vdev->num_ctx) >> return -EINVAL; > > Unfortunately this turns the unwind portion of the function into an > infinite loop in the common case when @start is zero: > > for (--j; j >= (int)start; j--) > vfio_msi_set_vector_signal(vdev, j, -1, msix); > > Thank you very much for catching this. It is not clear to me how you would prefer to resolve this. Would you prefer that the vector parameter in vfio_msi_set_vector_signal() continue to be an int and this patch be dropped and the "if (vector < 0)" check remains (option A)? Or, alternatively, I see two other possible solutions where the vector parameter in vfio_msi_set_vector_signal() becomes an unsigned int and the above snippet could be one of: option B: vfio_msi_set_block() { int i, j, ret = 0; ... for (--j; j >= (int)start; j--) vfio_msi_set_vector_signal(vdev, (unsigned int)j, -1, msix); } option C: vfio_msi_set_block() { int i, ret = 0; unsigned int j; ... for (--j; j >= start && j < start + count; j--) vfio_msi_set_vector_signal(vdev, j, -1, msix); } What would you prefer? Reinette
On Thu, 30 Mar 2023 15:32:20 -0700 Reinette Chatre <reinette.chatre@intel.com> wrote: > Hi Alex, > > On 3/30/2023 1:26 PM, Alex Williamson wrote: > > On Tue, 28 Mar 2023 14:53:29 -0700 > > Reinette Chatre <reinette.chatre@intel.com> wrote: > ... > > >> @@ -399,7 +399,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev, > >> static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned start, > >> unsigned count, int32_t *fds, bool msix) > >> { > >> - int i, j, ret = 0; > >> + int i, ret = 0; > >> + unsigned int j; > >> > >> if (start >= vdev->num_ctx || start + count > vdev->num_ctx) > >> return -EINVAL; > > > > Unfortunately this turns the unwind portion of the function into an > > infinite loop in the common case when @start is zero: > > > > for (--j; j >= (int)start; j--) > > vfio_msi_set_vector_signal(vdev, j, -1, msix); > > > > > > Thank you very much for catching this. It is not clear to me how you > would prefer to resolve this. Would you prefer that the vector parameter > in vfio_msi_set_vector_signal() continue to be an int and this patch be > dropped and the "if (vector < 0)" check remains (option A)? Or, alternatively, > I see two other possible solutions where the vector parameter in > vfio_msi_set_vector_signal() becomes an unsigned int and the above snippet > could be one of: > > option B: > vfio_msi_set_block() > { > int i, j, ret = 0; > > ... > for (--j; j >= (int)start; j--) > vfio_msi_set_vector_signal(vdev, (unsigned int)j, -1, msix); > } > > option C: > vfio_msi_set_block() > { > int i, ret = 0; > unsigned int j; > > ... > for (--j; j >= start && j < start + count; j--) > vfio_msi_set_vector_signal(vdev, j, -1, msix); > } > > What would you prefer? Hmm, C is fine, it avoids casting. I think we could also do: unsigned int i, j; int ret = 0; ... for (i = start; i < j; i++) vfio_msi_set_vector_signal(vdev, i, -1, msix); Thanks, Alex
Hi Alex, On 3/30/2023 3:54 PM, Alex Williamson wrote: > On Thu, 30 Mar 2023 15:32:20 -0700 > Reinette Chatre <reinette.chatre@intel.com> wrote: >> On 3/30/2023 1:26 PM, Alex Williamson wrote: >>> On Tue, 28 Mar 2023 14:53:29 -0700 >>> Reinette Chatre <reinette.chatre@intel.com> wrote: >> ... >> >>>> @@ -399,7 +399,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev, >>>> static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned start, >>>> unsigned count, int32_t *fds, bool msix) >>>> { >>>> - int i, j, ret = 0; >>>> + int i, ret = 0; >>>> + unsigned int j; >>>> >>>> if (start >= vdev->num_ctx || start + count > vdev->num_ctx) >>>> return -EINVAL; >>> >>> Unfortunately this turns the unwind portion of the function into an >>> infinite loop in the common case when @start is zero: >>> >>> for (--j; j >= (int)start; j--) >>> vfio_msi_set_vector_signal(vdev, j, -1, msix); >>> >>> >> >> Thank you very much for catching this. It is not clear to me how you >> would prefer to resolve this. Would you prefer that the vector parameter >> in vfio_msi_set_vector_signal() continue to be an int and this patch be >> dropped and the "if (vector < 0)" check remains (option A)? Or, alternatively, >> I see two other possible solutions where the vector parameter in >> vfio_msi_set_vector_signal() becomes an unsigned int and the above snippet >> could be one of: >> >> option B: >> vfio_msi_set_block() >> { >> int i, j, ret = 0; >> >> ... >> for (--j; j >= (int)start; j--) >> vfio_msi_set_vector_signal(vdev, (unsigned int)j, -1, msix); >> } >> >> option C: >> vfio_msi_set_block() >> { >> int i, ret = 0; >> unsigned int j; >> >> ... >> for (--j; j >= start && j < start + count; j--) >> vfio_msi_set_vector_signal(vdev, j, -1, msix); >> } >> >> What would you prefer? > > > Hmm, C is fine, it avoids casting. I think we could also do: > > unsigned int i, j; > int ret = 0; > > ... > > for (i = start; i < j; i++) > vfio_msi_set_vector_signal(vdev, i, -1, msix); > Much better. Will do. Thank you very much. Reinette
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c index 6a9c6a143cc3..3f64ccdce69f 100644 --- a/drivers/vfio/pci/vfio_pci_intrs.c +++ b/drivers/vfio/pci/vfio_pci_intrs.c @@ -317,14 +317,14 @@ static int vfio_msi_enable(struct vfio_pci_core_device *vdev, int nvec, bool msi } static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev, - int vector, int fd, bool msix) + unsigned int vector, int fd, bool msix) { struct pci_dev *pdev = vdev->pdev; struct eventfd_ctx *trigger; int irq, ret; u16 cmd; - if (vector < 0 || vector >= vdev->num_ctx) + if (vector >= vdev->num_ctx) return -EINVAL; irq = pci_irq_vector(pdev, vector); @@ -399,7 +399,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev, static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned start, unsigned count, int32_t *fds, bool msix) { - int i, j, ret = 0; + int i, ret = 0; + unsigned int j; if (start >= vdev->num_ctx || start + count > vdev->num_ctx) return -EINVAL; @@ -420,7 +421,7 @@ static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned start, static void vfio_msi_disable(struct vfio_pci_core_device *vdev, bool msix) { struct pci_dev *pdev = vdev->pdev; - int i; + unsigned int i; u16 cmd; for (i = 0; i < vdev->num_ctx; i++) { @@ -542,7 +543,7 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev, unsigned index, unsigned start, unsigned count, uint32_t flags, void *data) { - int i; + unsigned int i; bool msix = (index == VFIO_PCI_MSIX_IRQ_INDEX) ? true : false; if (irq_is(vdev, index) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
User space provides the vector as an unsigned int that is checked early for validity (vfio_set_irqs_validate_and_prepare()). A later negative check of the provided vector is not necessary. Remove the negative check and ensure the type used for the vector is consistent as an unsigned int. Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> --- drivers/vfio/pci/vfio_pci_intrs.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)