Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 30 Mar 2023 16:54:45 -0600
From:   Alex Williamson <alex.williamson@redhat.com>
To:     Reinette Chatre <reinette.chatre@intel.com>
Cc:     <jgg@nvidia.com>, <yishaih@nvidia.com>,
        <shameerali.kolothum.thodi@huawei.com>, <kevin.tian@intel.com>,
        <tglx@linutronix.de>, <darwi@linutronix.de>, <kvm@vger.kernel.org>,
        <dave.jiang@intel.com>, <jing2.liu@intel.com>,
        <ashok.raj@intel.com>, <fenghua.yu@intel.com>,
        <tom.zanussi@linux.intel.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V2 2/8] vfio/pci: Remove negative check on unsigned
 vector
Message-ID: <20230330165445.7bf7cdd6.alex.williamson@redhat.com>
In-Reply-To: <460fdc5f-7613-5164-0247-254939cedc71@intel.com>
References: <cover.1680038771.git.reinette.chatre@intel.com>
        <0dc2a0c8e25b13a3a41db75ab192f387a1548c80.1680038771.git.reinette.chatre@intel.com>
        <20230330142657.3930c68b.alex.williamson@redhat.com>
        <460fdc5f-7613-5164-0247-254939cedc71@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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