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;
Message-ID: <65669f7f-e1fd-5b6b-f879-9166bd9a474c@intel.com>
Date:   Thu, 30 Mar 2023 16:54:59 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.9.0
Subject: Re: [PATCH V2 2/8] vfio/pci: Remove negative check on unsigned vector
Content-Language: en-US
To:     Alex Williamson <alex.williamson@redhat.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>
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>
 <20230330165445.7bf7cdd6.alex.williamson@redhat.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230330165445.7bf7cdd6.alex.williamson@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?a1ZJdThydFMreXppT0p4M0Z2YTNtUFgwQlhyanN5dUFaSm9lYnhTTVJBa1Y0?=
 =?utf-8?B?RS9DbzdMV0l2VVdSNW8vdmNpUEFpandHQzdyWWthWGp1S0RCNFpobmVsem9V?=
 =?utf-8?B?VXE2VnZOaUpBb3RTMzNObkdoSmJPYkpyTHRaOTNTdFVuSVQrZmVoK3lxQzcz?=
 =?utf-8?B?bkorSXp3MFdpbm1XV3l4WnZwcmVIRENrcGx3N21vUG9UMmpuUjJjRFlvbkxU?=
 =?utf-8?B?WGlybGkxQWZHTlpwTDl3VmQ4TFVFYTZWUklQbGpQM2NGbXNQVTdZZkRleDdk?=
 =?utf-8?B?YzNZcEtwcEFuM2ovV2Y0QU1tazdvZVJVMU5vMFpWZDEySU5rYS9Odm4rK0NY?=
 =?utf-8?B?dVcva2dpQm1rZCtkdE56N0dOZG1BbnJPa2lDZW1pZFREZ0psdHNTNjFRay9N?=
 =?utf-8?B?eVV5QVR1QmxxUDBwQk1uczlOMDlhbm5sK051djRSMDhCQzZwRWFjZWxjT2wy?=
 =?utf-8?B?Y3RkV0tmRUkrdTJqRDQvbHplVTFuMC9yNEdEZWtLVjZENFFzcURKMjFSbDRz?=
 =?utf-8?B?YWxnYmwwd2Nsdmh1dktlNzI3OUhtanNPaE0rZGV6aWNCL216Q1Q0VmVaOWc1?=
 =?utf-8?B?alFxSVpDZFZCVlB6MDBkMjRuYkwvQ1lhTWtQYzVuL1BJeHppa2JLN3FBcG1i?=
 =?utf-8?B?a0FYRWhQc1QzVlZFdittUnRmUTdFa2dacUF6c3plcXZKNW1TNXl6V2FqVVVO?=
 =?utf-8?B?UnNYUW1Oc2ZZVUhSZWgvK3JuaDl6RGJHVDNsa3g3TjVOWmNHa2lnMTQxb2dX?=
 =?utf-8?B?N0h1dHpNTlZlOHNDOER5V3F1TUJ1eDV0cEdRbWo3V2VMKzc5MFdzQXFuYks0?=
 =?utf-8?B?ZVhXam51TG5NR0dlVDBza1Zkcm13SW5JRjJMbzl6L1NBbTRWVWRIQ0drb2w2?=
 =?utf-8?B?RU1rVXJPcEZ1dnJJZXRjNWwrK01iN3BodHRDVlpCbFprenFCWVd1clVkcnQy?=
 =?utf-8?B?UUg4a2dlQy9VbmE3V3llajl6ZnJoVkgrazFFQXl5VVZ1aFQrd2NEcEplaE4w?=
 =?utf-8?B?eFptamVSU0J1M2RJUXRVY1A1SC8zV1Vxb0IzTmFiNEthU0NsN1M4aWRSQ0Y3?=
 =?utf-8?B?WHhqM3ZmR0EvRjBiMWJiWllxeFREQXVMdk85VkRhOW5CU2ZKenFONjNNd1BW?=
 =?utf-8?B?N1FVL0l4MEQyYU9ST1BKbXlaUzFkTmM5YU5FNWViV3lxeUxDcnY3eER5aFVk?=
 =?utf-8?B?Z01YRVZWM1BGWFR4dE9JZzNrWWhYSjFNZ3RsdXZuTGVkNk9GcUswckhxaC85?=
 =?utf-8?B?NDY2aEI2UFVRSUJmc0NnMFZFZW9RV0RwQkFnWFFmajJaZHlCQitvMHpYU0dD?=
 =?utf-8?B?bVVJZ3ErYVp3aHBxWThoRXp0Y28ySHhzMjd0RHVKR1owUE1mQmdPUXVFR3dE?=
 =?utf-8?B?SzBrWVhsNkRyN1dTR3FlY3luNEhta1RPZ3FMTEdWTXEwUFU5WnYwU3JvOVY3?=
 =?utf-8?B?SkxxMXBxendhUGxtUmVMRithdzJvVmNWdTBvdzVCT1M5c2dreVI5QlgwRnNx?=
 =?utf-8?B?NnVyY1BMWVJMbGhwY3k0bjFmRFlNbmpTVXVJdEdBUkxERXFRa1VrN0ZlQjc2?=
 =?utf-8?B?bXdldExRbDgwMXppTkhVbmg1RDVidFJnOExzOHYrRjE1OXVZZEoxVmM5WUNi?=
 =?utf-8?B?K2pKRXhrNUc3ajREYi9wcnVyYitoZWRlOXJtb3hDOGRxWUdNbEpsYzhlc1Mv?=
 =?utf-8?B?ZVV1TWJpNVNNQTUwSHEvbEVzbWprMnUraFZvdW5Rdnd2ZWFWNUhFc1c5bGJr?=
 =?utf-8?B?OGtwR0p3Qm9ERno4RmtJVkhIbEFpS3gzWnM5QzdIUTF6MURoelkxYURSa0g1?=
 =?utf-8?B?RmZraWxNMmFHaFhRVVN5ck9wVzh6VmpudlMxS3RiZWRNdHF3bHpvQUNOTWpl?=
 =?utf-8?B?TERNanUwTXRhWHFENUlkQTVzdDVXMWYrcFpPVENsLytXM0srVTVNZTJwL3Vs?=
 =?utf-8?B?NWJJYWVWei96eVV1aGNTNDZRWDVrZVU5REUvMzRRKzdmVzNiWlpLRWhEeW5W?=
 =?utf-8?B?SWlrMm1LVTFRUHJKdzlJd2tTN2cxWVdxOEUvdnlsV0czVVlLdHF3amNZNXZ2?=
 =?utf-8?B?MFltY2lUTGQ5YVhVdExFOHJ3R2ZPN2RyVUZ0WC9Fa3YwRDBpQzFjRFBYUWdE?=
 =?utf-8?B?bkkzU0h0QkFLMDBFNktmVHJkTjhqN3FRalBqWUo3dFYwYU9tYkNWd0x1R2ZQ?=
 =?utf-8?B?K1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ff46f854-8b3e-4ca3-ef46-08db317a2d8a
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Mar 2023 23:55:02.5799
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2kjrKk8gu3bkA2am8wODkqPXS75IVZ3auwC4h6+ObsI3pk4nigas7n0x0RIx3thUCTyiv6TDiF1sGYdxmyF7jag3/98JQ39s9ANTKnexiQA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4593
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=unavailable
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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