Received-SPF: pass (google.com: domain of linux-kernel+bounces-55686-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <ce617344-ab6e-49f3-adbd-47be9fb87bf9@intel.com>
Date: Tue, 6 Feb 2024 14:22:04 -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>
 <5784cc9b-697a-40fa-99b0-b75530f51214@intel.com>
 <20240206150341.798bb9fe.alex.williamson@redhat.com>
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240206150341.798bb9fe.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?VHRpSDhSekt5Qk9GRVVncjBMM1NWZnJteXl3QW9aSjVXQnd2S3ozVXRjWmN5?=
 =?utf-8?B?VGRMbDNwWE9RZUhLUjhZcHl6R2ROUTRWK2xJcTAzSlRveEFacmhyQmZBVzU4?=
 =?utf-8?B?eXdlbEEwODkwWFhlQmplQUZPL0tUOEFYWmRzT2NnM1lSa05QRXgrNE5GTWta?=
 =?utf-8?B?RFV1eEdveGtQTXArd2NIdGRmSVdLeHcvaStINjNObTlSQUZXYWx4eW8yenJL?=
 =?utf-8?B?SllhKzFZN3hpdlA4dHFSVVZjMTU5Y2srYzVGaU1icFdqdjdKZlJxeUtRUGRw?=
 =?utf-8?B?bk9KTHdZSEhvNExVRnhnY0RKaG5lLzgzanhDeTR2Ly8xVDRmTHJadVFOUWVG?=
 =?utf-8?B?L3U5cHZRaUQ3QzNlU05jR1lFYzk0OFNFdXErS1ZQM1pwUzlCWkdZbURnZHpm?=
 =?utf-8?B?SlZDK1ZRcDFETkUwaVQ2V3JRaTdrQ2J5L1NPT0NlWTZNN0dVWXk4bkdOUFox?=
 =?utf-8?B?WHJjaTR4YXcvdEdRWndVZVNYdDRDRDhQY1ZlZ1M3SHpCWGhvVVRZL2VCYlln?=
 =?utf-8?B?ZXhSYzhydHRNbmJJYjFESllnL0pJejNvK1Rzdnd2NHFtZ1hkNkpNNlpPNnUr?=
 =?utf-8?B?bnNNcHFWaGxBOUlaQWU1bHFiVnhna0dVRmpDemhrZnM0WFE0ZWszVUZtUWdK?=
 =?utf-8?B?TW4zd1h1TmI1eEJ6ZmVwOURDbWw5c1ZZaW80cU1NMytFdjNpQUJldmpWOXBN?=
 =?utf-8?B?c2lMMzZadzg3SVhzZ3RFaDNLU1dLVzNOUlMwYXNFUTBwTjBuYzZwakZ2SnRP?=
 =?utf-8?B?dkU2TGIvWjVCMXVWQ0RiM3RBQnZ2UmxhdUptb2hZR2tqU212QXdWVEtrQTVN?=
 =?utf-8?B?TlNIR2tMdzNzS0l1UHQyQWZZQ1Zza1JOZXpCei80NktsSHA1WmxVUFhPVzlJ?=
 =?utf-8?B?dXUxMytrODhRd2wrL2NuM2xXUlprS0x3RE5wM1hHaFRjNDh1QjFvYlZyY0Vi?=
 =?utf-8?B?VDFkUW5MUzlaVnpnenR5YjN4dVd2cGgxZTJoLzF1cSs0R1E4MmF2OVJSZGhl?=
 =?utf-8?B?RHlTTWl4SVQ4bGNXZ3U1cWZaNFJrcUxBWnBhbjRMclFHV0xCZ0lMVnJrZUgx?=
 =?utf-8?B?WTRpd0dxZUdsYmZEN1FpRXZlUFdKakNOcmcyN0R6VFYxbGUycWQxdFF3OGhU?=
 =?utf-8?B?TWlxTjZQL2h2THp3clNraTBCQ0lwRjR5MTJtdVBzS2RJUmNmYnRpVWJJVzIw?=
 =?utf-8?B?cm1BVHQvNEFsbTdQSTM0a2o1MzRUalN5a21ZYWQ3MkFBUTkvNXpEUjJTZ3FF?=
 =?utf-8?B?blZGVDlPQmJKekhRQ2xDM3RPcTFqMVFiTlVWbmxQR3NDckNwaGN1bzFKUVBJ?=
 =?utf-8?B?ZWJWRHBsVEoxN3hhMXNWa2xhdFp5S3FFWFJwTlkyY0ljVzhaTVRyMU9qZ2dH?=
 =?utf-8?B?U2s1YXRjYzA4Nlc3cWlCU1VGbExyVkluNWZmU0tZRzZNWkE0aWNDOUpzSmVu?=
 =?utf-8?B?TFpTRzFWZTluMFpVQldsNllYTy9IRytKaVo0OFRNT0JOVFlBVFM3Z29MVkc0?=
 =?utf-8?B?Y1FnTVZFMlpsTjd5c2dXZVRUY3Fxd2tVZGV0R09Jejg2VTlxak1QMWk0RVda?=
 =?utf-8?B?MFptcCtLN3hrRSszL1BKeVJVZlczNUpqSFZGMVZyOHc4dDNYRHdkVytlWFNH?=
 =?utf-8?B?ckF4NkxsTHNKMXNmYTdzQVJkSWQwbDhmdStuRDMxZTVWRU84YUtpZUxMdG9a?=
 =?utf-8?B?YzNLRWpXSzNYSjVaRk5VaThwcE1ZclJKbVlPUkUrN0diSTVPVW81N3p4dith?=
 =?utf-8?B?elNyMFNoTkIxQlljRjhsWmV4UlBzcEE3dE82MTVCOTFqZGNheUdJbEJKd2xu?=
 =?utf-8?B?bjNWdnJ4VVFHZlBWVmhSbWNHYW5jdXc4VFBxd1VkMVdWdjUyVE9rWDlwbTcz?=
 =?utf-8?B?WGJydS82TTRib29meFRKeHl4K0N2bFpyZFc4TTMvbS80V2NldUZONlliZDhS?=
 =?utf-8?B?T1ROR3FCRGtPamFSdkdUNVVrRHFsZjhxbzhWcCt2U0VYY2ZuQ0ZUM3FzRzNQ?=
 =?utf-8?B?d3hUUTQ0S2hWN0doNCsvZTlOUnBBSzlQQkpuZTMrQmNRTEVoY1hhZmYzUEFx?=
 =?utf-8?B?Vno0THF0ZXZSTjg3U0d6RFlWWm5vUktraXc1SGpCdStJWUw0OS96aXpNaXB1?=
 =?utf-8?B?ZWRaYzkrMUpydWswdzJVdkdKYktyVDJJTHY1Z2ZoT0MwemlZUE1WNENaMm0x?=
 =?utf-8?B?ZFE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c161a053-224f-40af-de57-08dc27620c94
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Feb 2024 22:22:05.2389
 (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: g1tbtPVMz67aww0usKMPLiR7aEF6yfb5jh6NWlk+Gidfgv+HMf7l8r8SGs+AYoYY+QO+f4rjL1haTISWOLZvS8BxYlHvEjZezcXC+6gOzSE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7925
X-OriginatorOrg: intel.com

Hi Alex,

On 2/6/2024 2:03 PM, Alex Williamson wrote:
> On Tue, 6 Feb 2024 13:46:37 -0800
> Reinette Chatre <reinette.chatre@intel.com> wrote:
> 
>> 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.
> 
> Sounds reasonable.
> 
>> 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?
> 
> I think by nop, you mean the ioctl returns success.  Was the call a
> success?  Thanks,

Yes, I mean the ioctl returns success without taking any
action (nop).

It is not obvious to me how to interpret "success" because from what I
understand current INTx and MSI/MSI-x are behaving differently when
considering this flow. If I understand correctly, INTx will return
an error if user space attempts to trigger an eventfd that has not
been set up while MSI and MSI-x will return 0.

I can restore existing INTx behavior by adding more logic and a return
code to the send_eventfd() callback so that the different interrupt types
can maintain their existing behavior.

Reinette