Received-SPF: pass (google.com: domain of linux-kernel+bounces-55652-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <5784cc9b-697a-40fa-99b0-b75530f51214@intel.com>
Date: Tue, 6 Feb 2024 13:46:37 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 15/17] vfio/pci: Let enable and disable of interrupt types
 use same signature
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>,
	<kvm@vger.kernel.org>, <dave.jiang@intel.com>, <ashok.raj@intel.com>,
	<linux-kernel@vger.kernel.org>, <patches@lists.linux.dev>
References: <cover.1706849424.git.reinette.chatre@intel.com>
 <bf87e46c249941ebbfacb20ee9ff92e8efd2a595.1706849424.git.reinette.chatre@intel.com>
 <20240205153542.0883e2ff.alex.williamson@redhat.com>
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240205153542.0883e2ff.alex.williamson@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WExTSzhtMFBxbjd5dFUydTdzaXQwc0Q3a3BXWkVuR2N1S3E5RkI1VjNxOCsz?=
 =?utf-8?B?QlA2ZGJDV2c5RGg5a1cxcWIwY2ptTURqcTdhMFJuZ2UzLzVrdjBkUGx4bXRz?=
 =?utf-8?B?TTNnMmkzZ2tTM1R3NXNtQ2hWUjVmazFQTi9SQzU5L1Z1NnJxTnZ3MmZWZFM2?=
 =?utf-8?B?UFpKMnQvb1FtYWVyWU5lQmh4RWVDN0V5TkpLQzJTRXVDR2p2MTNhSkYzUTlk?=
 =?utf-8?B?cERsb3FMc0hFdGNsOE5nQTZIcDVQODBQNGtXVHFUd2JhUnI1VGk3cmZCYWJQ?=
 =?utf-8?B?MHAvTzhNN3JPQm1JODNieC85OXNLVVFMM2x4dUhmRWphL0E0YklOcXBETkZl?=
 =?utf-8?B?cFltWUdDcjY3SGk5cnlCS2lDbC9OODJSWDVtSFRoV3pMU25UdWxnQnVKUmx2?=
 =?utf-8?B?TlZsbmJxVmxYS1VxMXA0ZlNuSlV6NTFYSmsrNHVPV2o3eUl6bGxud1VhR1Fn?=
 =?utf-8?B?dDFYMTBaVFlPQnAwWGtodUFRTXcxY2FYUHlvamdWVFZOVjNWRXlvSGgxUnNS?=
 =?utf-8?B?ZU1TcE5XQWdFRHozVFZFVU8ySXU0ZmVGSjFoRk5BSVJHRklSemZuNWJsS1k4?=
 =?utf-8?B?RGtJUDNUYUxKNWJUSWhQYzVjNmlCRzEvTlhETGd5TktDSXNuV1I5Y2VjOGF2?=
 =?utf-8?B?dUdUTTBBb1ltKy85WUhOYTYwK1BFNnFYZFcxaDY1ZmR5OCtUbGg4YVRxSTJq?=
 =?utf-8?B?UzVVSjhJbFVJWXJRYW5henlDZ3dBSWk4YklCSXBTcUEvYm40ZGhEVEszKzds?=
 =?utf-8?B?ZlNRZFNxTVlXWExBK3FhdG5YaHFGaTNjQzNGVW96WHhDRExXUlVmbXIwUE9p?=
 =?utf-8?B?VDg5NTNnc3MzQTkxZGNiVWNJMXNib2J4U1hVajFENlk2VGZqMlhTSGNFTDQ3?=
 =?utf-8?B?eFhiL09FRU5abkc1RG9TdE1SWXMxZ3h3dUwzdXNwRUZiWm9ReE5PNC82YzZO?=
 =?utf-8?B?ckpkR01HV1FHQ0VqNkh6cEUyQllhM1YxVUxjTzRqa2s4bU9ZaUI1cXhSYUZa?=
 =?utf-8?B?ckxnaHVjL0MrN2FzMjk3SHppSGROYjM0aTNXK2kxVWt1Z0lHeVREZ2xicFZj?=
 =?utf-8?B?UGp5VmNuUXRUL2JBNTdCdE9sM3pzYWNmZ0VGQjk3cFdjZWJWMkw2UjQzVUFI?=
 =?utf-8?B?SnNvNDRmNCtGR1BKUGFKNG16cnFBS2tHYmUrYkIzY2w2eDFBU0ZJbElWV1RC?=
 =?utf-8?B?Z0YwRHV6YlluOS9vZ1VncFZSRmxFaDFWTHlzQ0t2TThtc1JSWmI4UEt2d1FT?=
 =?utf-8?B?NVV0ZDBGS3dBTHB6TDZzRmg3SmhxZXRianl0ZEtBSGp2RzdmOFJFTmVNUklo?=
 =?utf-8?B?RDNkcG43cTVPcFhIWlVSZzlzYzFJbXZjVXFXVFFHMFU5RVc2Rk1ZNkxMYzI3?=
 =?utf-8?B?RTJIYVBxaXcyL0dkVTdSZlorQ1A1YkhtK3RBNnh5VHpET2dCbk54R2xEZ1N2?=
 =?utf-8?B?UVJDY2w2c0hYakNxd3dqbXlSdmJZT3VMT3RPdzFwR1EyVlhDL1p0N3NaTVBu?=
 =?utf-8?B?bXVxWDVUUm56Rm9HK2crUkFSblEzSUV6OHhqTW1uQ2hkb2ExZHp0YVBFcTZz?=
 =?utf-8?B?QVk4Rm5xTENZcXE1V1BYWUh6Zi9IUUdSWklCaFV0UGJmRnRLdXdWdTNKT0x2?=
 =?utf-8?B?Y1czUjRIR3F5WDRhcThWMEluajFvVFJzNmFGMzhmNWZZMDlIWktNdFAvQWIx?=
 =?utf-8?B?REJZQ0JPbDBZdDZrY3NVSHJRQlo2clVTZTVmVWxnR1h6bGNOQVhNNGFmMTRR?=
 =?utf-8?B?MWdwZDRrUUd3MlR4S0RLNXhQc2ZVNFVyL3dUUStaenlLK3ZvSlJEbHY1NTFN?=
 =?utf-8?B?dFN4RWFTdy9haStPS01nK3QzVXEzbWRUQmU2VlJucnF2WHY3QzVrZ3FLNVEx?=
 =?utf-8?B?Vmp5aERnQzM0S0YzYkErMGVZZTFCRXZYRzYyVW5WMEJuemp6TjI4MWE5b1Fj?=
 =?utf-8?B?VjVnQ2RYdHU3NjJhamJOYW9hTGh2OERDaHBaUHllbDZyU20ycWEyeFB6TUhj?=
 =?utf-8?B?QnVkNGZPYUxiOXVWNlNXME1lYkxYUzlHS0tleGxqejA1REhpRFIxN01uVlBF?=
 =?utf-8?B?QmFyc2dmMFRSbXRvdlpaaWplUzVBbk1zSkZ4dmtZZW5pVmR3KzZVUzhJZjFL?=
 =?utf-8?B?aVZidTl3VWF2M1MrMWhYM2lENzNTZ2RDUXY4Nm5Kb1BEelVuaXRWcm9xdXdS?=
 =?utf-8?B?aEE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 68f3fd4a-a19e-4918-febb-08dc275d18f9
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Feb 2024 21:46:38.5438
 (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: UoZ9xo5VeHWY+ULROUhfM7NlPclpbSrMIb/lR2+d1se52xlImQynEs79ekybfE0zekwdXxP04pWBc/g2/rUXcaKHdDjOhdub1kyugdhuTn0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7073
X-OriginatorOrg: intel.com

Hi Alex,

On 2/5/2024 2:35 PM, Alex Williamson wrote:
> On Thu,  1 Feb 2024 20:57:09 -0800
> Reinette Chatre <reinette.chatre@intel.com> wrote:

..

>> @@ -715,13 +724,13 @@ static int vfio_pci_set_intx_trigger(struct vfio_pci_core_device *vdev,
>>  		if (is_intx(vdev))
>>  			return vfio_irq_set_block(vdev, start, count, fds, index);
>>  
>> -		ret = vfio_intx_enable(vdev);
>> +		ret = vfio_intx_enable(vdev, start, count, index);
> 
> Please trace what happens when a user calls SET_IRQS to setup a trigger
> eventfd with start = 0, count = 1, followed by any other combination of
> start and count values once is_intx() is true.  vfio_intx_enable()
> cannot be the only place we bounds check the user, all of the INTx
> callbacks should be an error or nop if vector != 0.  Thanks,
> 

Thank you very much for catching this. I plan to add the vector
check to the device_name() and request_interrupt() callbacks. I do
not think it is necessary to add the vector check to disable() since
it does not operate on a range and from what I can tell it depends on
a successful enable() that already contains the vector check. Similar,
free_interrupt() requires a successful request_interrupt() (that will
have vector check in next version).
send_eventfd() requires a valid interrupt context that is only
possible if enable() or request_interrupt() succeeded.

If user space creates an eventfd with start = 0 and count = 1
and then attempts to trigger the eventfd using another combination then
the changes in this series will result in a nop while the current
implementation will result in -EINVAL. Is this acceptable?

Reinette