diff mbox series

[V2,2/8] vfio/pci: Remove negative check on unsigned vector

Message ID 0dc2a0c8e25b13a3a41db75ab192f387a1548c80.1680038771.git.reinette.chatre@intel.com
State New
Headers
Series vfio/pci: Support dynamic allocation of MSI-X interrupts |

Commit Message

Reinette Chatre March 28, 2023, 9:53 p.m. UTC 
  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(-)

Comments

Alex Williamson March 30, 2023, 8:26 p.m. UTC | #1 
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)) {
Reinette Chatre March 30, 2023, 10:32 p.m. UTC | #2 
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
Alex Williamson March 30, 2023, 10:54 p.m. UTC | #3 
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
Reinette Chatre March 30, 2023, 11:54 p.m. UTC | #4 
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 mbox series

Patch

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)) {