Received-SPF: pass (google.com: domain of linux-kernel+bounces-57298-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: <63ba0079-a035-4595-a40e-8c063b4a59eb@intel.com>
Date: Wed, 7 Feb 2024 15:30:15 -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>
 <ce617344-ab6e-49f3-adbd-47be9fb87bf9@intel.com>
 <20240206161934.684237d3.alex.williamson@redhat.com>
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240206161934.684237d3.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?NFdoY1NvU2lQL3ZoWFZuNmJraGEzanNZR3U3UStKc0ljUTZyVUN4ZkFtQVJR?=
 =?utf-8?B?Vmxkc21wcCtWRzdIZXhKZUtRTmlMaCtxamxob2VUNXgxb09zblF6MmhzLzZs?=
 =?utf-8?B?NFAwTkc5K1grQW9nbVk1c0R5MjRPVHRsanI0NVh0ajQ3QkRyYzBZMkwwSHNY?=
 =?utf-8?B?U0pFVDgyeUlOMEdpYm1QcE9BWVRUMUh5MHZmTVo2NEJjN3QwcUtVdVQxbyt2?=
 =?utf-8?B?dytyeFBhZDUySHlwbDdMWEh5K3hydjB1UUxYRGc4UFQ4N1cwTFBpcmVyYzdl?=
 =?utf-8?B?U2Vxb0JMdUJUZnhVQ21neWdVZ3lMeUEyRnUza29Cb3hyYzY1OWNidlNETEpT?=
 =?utf-8?B?RFE0RStDOWFlTmNKUmp5OVYwOXd1ZWZzeDlOVTI1NnRlVE9teUJOQ2FGSDgr?=
 =?utf-8?B?aW81QlFPSkpiVlpMR0ZpNzVaOGpXVFVaKzBkYkhpcUZ6QTlVemNMamIzMm1G?=
 =?utf-8?B?S3JVTGVHRFhqZ0hGK09jaVRhUmpLMzVzZXZTZGY5VU5UN2RPaDJBSzNEdDdr?=
 =?utf-8?B?ZzlGTTkzNnk3bFNoRTk1ZjVJdVJySGZzTEtEdUNjcStoUUc1YWhValEyNEhS?=
 =?utf-8?B?aTl2Sjh1UVN6VU0xQkg4clQzR1lzZGNnQzVOUTkwMGFuNkE3QnlMU3VVRGhU?=
 =?utf-8?B?aDJtVE9ZclJDa1dBYlpzTXlYTTkzWkR2U2F2L2dQZFhDWXF5YmxkcVdQMEQw?=
 =?utf-8?B?V2JMWWhmUVBVdTJLak1QYkozTmxsdEJGb0NMcHl2OWlnOG0vQU15bWoyTGxm?=
 =?utf-8?B?U3MyMTVNMlB2MjFTWmIvN0Y2My9BMlNhNU16SWFsK0RjblVTSEM5VGJXZUNN?=
 =?utf-8?B?MkJnNkVXUkJJejF3UysrYXB0TWJraGFxaG1WMURVM1NRZmFMbm1kUDI0eENI?=
 =?utf-8?B?REZZd0NGWkphT0tMUHM5Q28rK1hzN2NEdGpSK2ExbUFYMmtrOGxDRVRMdkJq?=
 =?utf-8?B?NGh4dXZ2aUF2SmZ6S29objAzTzVOTDZiaTFiMFpBWmsvSjU5MFl0NDVDUzdI?=
 =?utf-8?B?ZmR5b3FzZ1IyWUcyTU0wcks3UHpNL3k1cXNjaEsrbWRQdzFBNGxBVnFMbGZW?=
 =?utf-8?B?SGo3c2NvUklCZThmSitXU0k1TjYrME0ySnhDeWJwL0Q4WEd2RW9IN2ppZk1G?=
 =?utf-8?B?dkVTYVZsdFFnL1lGQytJaVV1NWdzTVNFR1hhZlAzbXhtSnlPa3JwMmptWVRi?=
 =?utf-8?B?MWlxNGNWS29TL3U1WXpKNXV6ZnpPZVJVeGpiVXpDSnRST0pqSnphd3ZHYTZ6?=
 =?utf-8?B?K0dJSXhBbnlNMTZiRDF1am8wM0FTdFY1Ung1TnVPSU8zUVVZdlJnWWlyVUk1?=
 =?utf-8?B?c3JBeWlmdXU3ZlVrNTczdTV5eEJqdEpyTlkySlFoRFNINml3NjZKQ09mcStN?=
 =?utf-8?B?ZlVLelhmN1JUOFlUVHhmWnc3cXBlWmVuVjZGZjlxVGJFRXUyNk9OTTJrRm1V?=
 =?utf-8?B?Sjh4Zm9iMkcxS0U0MEdCeEFYQ0RWNE1aS3U3OEp3QWFqbHNiQ2FLcHhyVU83?=
 =?utf-8?B?OE5wSFUvVVVzM0Mvb1dUYURZOHFnTWs0eTVNTVQ1STNZcERTbytac3J0elQy?=
 =?utf-8?B?Y2lFN3luanQ4WmJWWWxTZXZYS3k0eS9LU0tEVU5teUh2SUxDZ3RqeVlvT1dy?=
 =?utf-8?B?NVpaaHlTamJGTWpsdUUrSEt0RGk3MFdLa3E1bmxxWDZDQTlkR3QwU1JnNDVT?=
 =?utf-8?B?Y1NYNzdxRUc4ODVWZ0RvamZGcEZNSHIzSXFwby9rSTQzNWpRc0g5MnFQTTFk?=
 =?utf-8?B?V3NINmMrWUY3SmJQWmFNb2dReGI1YXRWbC9XeHJTYU1lZGtPZ1h6VGJWdnBy?=
 =?utf-8?B?Z3hkaU1OYjlRaTN6L0JPVTlLWlVISElqNUhDZzhuRkZYMS9zT0Q5K1R1YTIv?=
 =?utf-8?B?dGIrdXN3K01pR29tTlBuMzc1bko5SEx2MVl3MDN4R2NVL1JXMmhoRmZaRE5T?=
 =?utf-8?B?Tkc3ZURBRGsxcnFOdTBhOFpZaUNwSXhyTGVvMlFzMjVoc24wSE1PL3BUdUhB?=
 =?utf-8?B?T2pweUN1WkVrUFFHOVdYRlRKK3UyNWZ6RitiZy9TV0ZQOW1XYUM1TEZJenp4?=
 =?utf-8?B?dVI4Z2dDdWdzTmhhbWtzT1BOWWxDU0xtTlNoY0pMbVVZSHh4blpXd3FGbzY3?=
 =?utf-8?B?ZjhyZG5QS2doMkdNUDNLRVNVR1ByYzF1a0ZoQ3RrMWV0MFZwNm9tMjljeDFF?=
 =?utf-8?B?d2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4cbf374d-9160-4a47-4e2d-08dc2834beda
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Feb 2024 23:30:18.6546
 (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: D0TzgEyuPglMm1V9uaGoZXQOFDgSWxT0ymmLLDffaFnGSI1OmY5sBbvmhz2Vmr55ovE0n7F5qkk9IHc1Sjs48yjA/5dY1+45YYulWsJPy8M=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM8PR11MB5591
X-OriginatorOrg: intel.com

Hi Alex,

On 2/6/2024 3:19 PM, Alex Williamson wrote:
> On Tue, 6 Feb 2024 14:22:04 -0800
> Reinette Chatre <reinette.chatre@intel.com> wrote:
>> 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:
>>>> 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.
> 
> Ah yes, I see the dilemma now.  INTx always checked start/count were
> valid but MSI/X plowed through regardless, and with this series we've
> standardized the loop around the MSI/X flow.
> 
> Tricky, but probably doesn't really matter.  Unless we break someone.
> 
> I can ignore that INTx can be masked and signaling a masked vector
> doesn't do anything, but signaling an unconfigured vector feels like an
> error condition and trying to create verbiage in the uAPI header to
> weasel out of that error and unconditionally return success makes me
> cringe.
> 
> What if we did this:
> 
>         uint8_t *bools = data;
> 	...
>         for (i = start; i < start + count; i++) {
>                 if ((flags & VFIO_IRQ_SET_DATA_NONE) ||
>                     ((flags & VFIO_IRQ_SET_DATA_BOOL) && bools[i - start])) {
>                         ctx = vfio_irq_ctx_get(vdev, i);
>                         if (!ctx || !ctx->trigger)
>                                 return -EINVAL;
>                         intr_ops[index].send_eventfd(vdev, ctx);
>                 }
>         }
> 

This looks good. Thank you very much. Will do.

I studied the code more and have one more observation related to this portion
of the flow:
From what I can tell this change makes the INTx code more robust. If I
understand current implementation correctly it seems possible to enable
INTx but not have interrupt allocated. In this case the interrupt context
(ctx) will exist but ctx->trigger will be NULL. Current
vfio_pci_set_intx_trigger()->vfio_send_intx_eventfd() only checks if
ctx is valid. It looks like it may call eventfd_signal(NULL) where
pointer is dereferenced.

If this is correct then I think a separate fix that can easily be
backported may be needed. Something like:

diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 237beac83809..17ec46d8ab29 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -92,7 +92,7 @@ static void vfio_send_intx_eventfd(void *opaque, void *unused)
 		struct vfio_pci_irq_ctx *ctx;
 
 		ctx = vfio_irq_ctx_get(vdev, 0);
-		if (WARN_ON_ONCE(!ctx))
+		if (WARN_ON_ONCE(!ctx || !ctx->trigger))
 			return;
 		eventfd_signal(ctx->trigger);
 	}

> And we note the behavior change for MSI/X in the commit log and if
> someone shouts that we broke them, we can make that an -errno or
> continue based on is_intx().  Sound ok?  Thanks,

I'll be sure to highlight the impact on MSI/MSI-x. Please do expect this
in the final patch "vfio/pci: Remove duplicate interrupt management flow"
though since that is where the different flows are merged.

I am not familiar with how all user space interacts with this flow and if/how
this may break things. I did look at Qemu code and I was not able to find
where it intentionally triggers MSI/MSI-x interrupts, I could only find it
for INTx.

If this does break things I would like to also consider moving the
different behavior into the interrupt type's respective send_eventfd()
callback instead of adding interrupt type specific code (like
is_intx()) into the shared flow.

Thank you.

Reinette