Received-SPF: pass (google.com: domain of linux-kernel+bounces-153392-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <4ffb4f53-a7a7-4ef7-88cc-e2042e953f87@amd.com>
Date: Mon, 22 Apr 2024 18:21:11 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 2/2] vfio/cdx: add interrupt support
To: Alex Williamson <alex.williamson@redhat.com>
Cc: tglx@linutronix.de, gregkh@linuxfoundation.org,
 linux-kernel@vger.kernel.org, kvm@vger.kernel.org, maz@kernel.org,
 git@amd.com, harpreet.anand@amd.com, pieter.jansen-van-vuuren@amd.com,
 nikhil.agarwal@amd.com, michal.simek@amd.com, abhijit.gangurde@amd.com,
 srivatsa@csail.mit.edu
References: <20240326040804.640860-1-nipun.gupta@amd.com>
 <20240326040804.640860-2-nipun.gupta@amd.com>
 <20240419163138.5284fc57.alex.williamson@redhat.com>
Content-Language: en-US
From: "Gupta, Nipun" <nipun.gupta@amd.com>
In-Reply-To: <20240419163138.5284fc57.alex.williamson@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
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?SjZMdGhZV0U0TUZaT1hhUHQxUFA0UTRFSFZjRkVueGhDODVTSXB2VENON2Yr?=
 =?utf-8?B?WVg1U0cwZzg2UkxzU0ZRQ2RQbXF0QWM4WHFVK3VIQ1BhR053bE1TRUxQVjRB?=
 =?utf-8?B?N2k2TGZHQW10bnR1bU8rVGdkYnZtellPYjY5bnBLZFpHUWI1OXluY2sxdnF2?=
 =?utf-8?B?YkFPYkpWTXhzY3pDc2VGYlBWVzhzT2R5c09FbmlFODhieWhMTEdFd2pQSkYx?=
 =?utf-8?B?Tmo5U3Y5MDNJSW5xaUw4WnVENzk5YlVnVWNqdm1Zby9CUFBpZVljSkJXTWxI?=
 =?utf-8?B?NXdqN05LUHdUQkVranFSUWRHQnZxd29OV1U0SVo5bDVQT1pnRlBSNk5ydWRt?=
 =?utf-8?B?Y0JwRVh6YnhBcFJEQzF5R2I1TnIzTDVqQ3pJSkJoK0k0akRwOUdhaTlXdWpl?=
 =?utf-8?B?bFU2T0tmS3hRcFN4Sk50ZjRzNXE5UnFURERNRGl6aU9qeW0zcFFLTzVMYWhR?=
 =?utf-8?B?NXZidDkzVTlUcXpZZlBnVllpeDhBSFRVMzQvcGpYaHpLRWc2elllY0dmNXNJ?=
 =?utf-8?B?czlkRTQveURGZ29xVTJGdklJcWJ5TTVKODBFMzRKNGRqbkRGVStYSGkwbmxF?=
 =?utf-8?B?RHROdFN3R3prbCsvdXJLTC8yZmxsZUFjRE9SMU9RY2hCMHkzRnk1MHBwcU91?=
 =?utf-8?B?em9YZ3hsZ2lFYlE3dEhjc2ZPOHNRR2lLeTNqWUdaeDV3Y3pPT3N0eUFlRDhW?=
 =?utf-8?B?UFlrQ1FKV2lscGhFYnhhOG9zdHlEMUVPelE0ZjgxbmxBb1hrd0Mra0RFcW5z?=
 =?utf-8?B?UlBCbmlHY1U3NzJPcFJBcXJpdlpKNFVnU2p2dzRwVlYybDBEcXNvVGhPMWhl?=
 =?utf-8?B?MXFSKzE5UUNQUzRyQnV2MEwzRjNLMmhZVmlKRXNkdjcwNC9CcmlWZUxIcGo2?=
 =?utf-8?B?d1l6dGhudmV2aTJZUHVIT1RpcUdrcHFVbFVZdmZQdzlCZ1VQenNHSGRCWHFH?=
 =?utf-8?B?aERWOXQzMW9mVjk4a2hNd0pqcDRYTEJBMUlOWVFYWEVBSGRieU9SNWpjWUhT?=
 =?utf-8?B?VENKNWJtY0VNUEtxQkFtaGhvT2s1eXBYQmNPb3ZFYU9BQlNQOGJHajdZNHhP?=
 =?utf-8?B?TmN1MTJrN1pkeWRwS2FtdEV4cW9KU2FoejBEek5HMFF5NWg5VlZmOEF0bEN4?=
 =?utf-8?B?UjFEeWFOVU1Ndm1JK3RELzZmRWVxM1ZzMUpKUlNDeDRuMUxEY3NTM1RMZElm?=
 =?utf-8?B?alJ6ZXBxbVR0V0puQnltempDOFBmN1pJeDByOW9ONkFFTjIrMnNDdUhmVTVj?=
 =?utf-8?B?bXlnbVVrdjQrMS9OZFkvWmVHUE1sRmZSeXp2NndYSlZnYVlBNWFBYk9RNEFQ?=
 =?utf-8?B?T2ZwaytVQ1BsNG9uTUdieEVuQVRCZG9HT01rQjVOY0RyZTB2WWg2UDl6ckRM?=
 =?utf-8?B?Y0c4VVJkOHlFYWt0ZGQvQnBOeStYcjByUloyYTZjVGtoYjJqWHZ5cEhuSDJE?=
 =?utf-8?B?RTVuY0tkSE16dnd5WlpPTUdFZUhxNExtMUFmM2JHUGZpZlkrZHBrTWljSTU1?=
 =?utf-8?B?Rmh4Rnp4aXRGZ2N6U3g5UHNDaGQ2R0FuRVgyVGxrUmdUOElQSmE2OFlhZHdV?=
 =?utf-8?B?dlIzbUtBak5selN5SGZ4VDFNNEF3WVg4UTJNZGF4ZkhBOCtNelZ6bEx3azRz?=
 =?utf-8?B?VDJ4MFNnVUlIdXJnbXhZa3RaZnZTaWtQZzFUcW9OdGI1M1hhVlhCZVZvbThH?=
 =?utf-8?B?ZHI1SllaVXJpbVRmZ3lYYmZ4b2dkRmJBWkgxMU9jR1lGUFk4YlpQK2NTRjBC?=
 =?utf-8?B?SVlXZjV3b2x0aGx5c1RlUEhMT3k2cUhXL2NxSXZIWEo1dWdrSGIrUWpXOXBo?=
 =?utf-8?B?cUd4UkM4S0NLNXNDU3NuS01iNkxwMWJlUEtOV3dETE5oNCs1dEJDK05Qdjdl?=
 =?utf-8?B?c1d6TGRFOERzTDkzbXpSNi9jZ2hyQzBPLzBMZTRGWW95cWxHWmYzT00vd2Y1?=
 =?utf-8?B?RHIwVGRiZjNMR1JOOTQrNEhuWWFpOXhUdTBKMCtMZUFrL2xuSVlXMHpsSXl4?=
 =?utf-8?B?K3JrRWl4N0FyRzR1UlBYK2FHb29OWXV3bWtOKzVrYXp5SUcrQm94WW9qQTAx?=
 =?utf-8?B?VzRPYnpQZUFicThFZ2dKNWRmbWVldXI5UVRHZlpyR1R2eDJCcjV6VmJGMU1t?=
 =?utf-8?Q?N7P2nvSBxiPmHRAVxCyG6f0Hi?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 55bb1490-2a96-4155-f024-08dc62caea9a
X-MS-Exchange-CrossTenant-AuthSource: CH3PR12MB9193.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Apr 2024 12:51:24.3448
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: vdZF9Qr6sOVYbLdKsg0vfclgoah/ka6/D1Pizyt4zUdl0e7uF8xglKSm9MIm5NKm
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ1PR12MB6025


On 4/20/2024 4:01 AM, Alex Williamson wrote:
> On Tue, 26 Mar 2024 09:38:04 +0530
> Nipun Gupta <nipun.gupta@amd.com> wrote:
> 
>> Support the following ioctls for CDX devices:
>> - VFIO_DEVICE_GET_IRQ_INFO
>> - VFIO_DEVICE_SET_IRQS
>>
>> This allows user to set an eventfd for cdx device interrupts and
>> trigger this interrupt eventfd from userspace.
>> All CDX device interrupts are MSIs. The MSIs are allocated from the
>> CDX-MSI domain.
>>
>> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
>> Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
>> ---
>>
>> Changes v4->v5:
>> - Rebased on 6.9-rc1

<snip>

>> +
>> +	for (i = start; i < start + count; i++) {
>> +		if (!vdev->cdx_irqs[i].trigger)
>> +			continue;
>> +		if (flags & VFIO_IRQ_SET_DATA_NONE)
>> +			eventfd_signal(vdev->cdx_irqs[i].trigger);
> 
> Typically DATA_BOOL support is also added here.

Sure. will add it.

> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int vfio_cdx_set_irqs_ioctl(struct vfio_cdx_device *vdev,
>> +			    u32 flags, unsigned int index,
>> +			    unsigned int start, unsigned int count,
>> +			    void *data)
>> +{
>> +	if (flags & VFIO_IRQ_SET_ACTION_TRIGGER)
>> +		return vfio_cdx_set_msi_trigger(vdev, index, start,
>> +			  count, flags, data);
>> +	else
>> +		return -EINVAL;
>> +}
>> +
>> +/* Free All IRQs for the given device */
>> +void vfio_cdx_irqs_cleanup(struct vfio_cdx_device *vdev)
>> +{
>> +	/*
>> +	 * Device does not support any interrupt or the interrupts
>> +	 * were not configured
>> +	 */
>> +	if (!vdev->cdx_irqs)
>> +		return;
>> +
>> +	vfio_cdx_set_msi_trigger(vdev, 1, 0, 0, VFIO_IRQ_SET_DATA_NONE, NULL);
> 
> @index is passed as 1 here.  AFAICT only index zero is supported.  The
> SET_IRQS ioctl path catches this in
> vfio_set_irqs_validate_and_prepare() but it might cause some strange
> behavior here if another index were ever added.

Agree, index should be passed as 0 here.

> 
>> +}
>> diff --git a/drivers/vfio/cdx/main.c b/drivers/vfio/cdx/main.c
>> index 9cff8d75789e..f0861a38ae10 100644
>> --- a/drivers/vfio/cdx/main.c
>> +++ b/drivers/vfio/cdx/main.c
>> @@ -61,6 +61,7 @@ static void vfio_cdx_close_device(struct vfio_device *core_vdev)
>>   
>>   	kfree(vdev->regions);
>>   	cdx_dev_reset(core_vdev->dev);
>> +	vfio_cdx_irqs_cleanup(vdev);
>>   }
>>   
>>   static int vfio_cdx_bm_ctrl(struct vfio_device *core_vdev, u32 flags,
>> @@ -123,7 +124,7 @@ static int vfio_cdx_ioctl_get_info(struct vfio_cdx_device *vdev,
>>   	info.flags |= VFIO_DEVICE_FLAGS_RESET;
>>   
>>   	info.num_regions = cdx_dev->res_count;
>> -	info.num_irqs = 0;
>> +	info.num_irqs = cdx_dev->num_msi ? 1 : 0;
>>   
>>   	return copy_to_user(arg, &info, minsz) ? -EFAULT : 0;
>>   }
>> @@ -152,6 +153,59 @@ static int vfio_cdx_ioctl_get_region_info(struct vfio_cdx_device *vdev,
>>   	return copy_to_user(arg, &info, minsz) ? -EFAULT : 0;
>>   }
>>   
>> +static int vfio_cdx_ioctl_get_irq_info(struct vfio_cdx_device *vdev,
>> +				       struct vfio_irq_info __user *arg)
>> +{
>> +	unsigned long minsz = offsetofend(struct vfio_irq_info, count);
>> +	struct cdx_device *cdx_dev = to_cdx_device(vdev->vdev.dev);
>> +	struct vfio_irq_info info;
>> +
>> +	if (copy_from_user(&info, arg, minsz))
>> +		return -EFAULT;
>> +
>> +	if (info.argsz < minsz)
>> +		return -EINVAL;
>> +
>> +	if (info.index >= 1)
>> +		return -EINVAL;
>> +
>> +	info.flags = VFIO_IRQ_INFO_EVENTFD | VFIO_IRQ_INFO_NORESIZE;
>> +	info.count = cdx_dev->num_msi;
> 
> This should return -EINVAL if cdx_dev->num_msi is zero.

Agree. Will add this check.

> 
>> +
>> +	return copy_to_user(arg, &info, minsz) ? -EFAULT : 0;
>> +}
>> +
>> +static int vfio_cdx_ioctl_set_irqs(struct vfio_cdx_device *vdev,
>> +				   struct vfio_irq_set __user *arg)
>> +{
>> +	unsigned long minsz = offsetofend(struct vfio_irq_set, count);
>> +	struct cdx_device *cdx_dev = to_cdx_device(vdev->vdev.dev);
>> +	struct vfio_irq_set hdr;
>> +	size_t data_size = 0;
>> +	u8 *data = NULL;
>> +	int ret = 0;
>> +
>> +	if (copy_from_user(&hdr, arg, minsz))
>> +		return -EFAULT;
>> +
>> +	ret = vfio_set_irqs_validate_and_prepare(&hdr, cdx_dev->num_msi,
>> +						 1, &data_size);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (data_size) {
>> +		data = memdup_user(arg->data, data_size);
>> +		if (IS_ERR(data))
>> +			return PTR_ERR(data);
>> +	}
>> +
>> +	ret = vfio_cdx_set_irqs_ioctl(vdev, hdr.flags, hdr.index,
>> +				      hdr.start, hdr.count, data);
>> +	kfree(data);
>> +
>> +	return ret;
>> +}
>> +
>>   static long vfio_cdx_ioctl(struct vfio_device *core_vdev,
>>   			   unsigned int cmd, unsigned long arg)
>>   {
>> @@ -164,6 +218,10 @@ static long vfio_cdx_ioctl(struct vfio_device *core_vdev,
>>   		return vfio_cdx_ioctl_get_info(vdev, uarg);
>>   	case VFIO_DEVICE_GET_REGION_INFO:
>>   		return vfio_cdx_ioctl_get_region_info(vdev, uarg);
>> +	case VFIO_DEVICE_GET_IRQ_INFO:
>> +		return vfio_cdx_ioctl_get_irq_info(vdev, uarg);
>> +	case VFIO_DEVICE_SET_IRQS:
>> +		return vfio_cdx_ioctl_set_irqs(vdev, uarg);
>>   	case VFIO_DEVICE_RESET:
>>   		return cdx_dev_reset(core_vdev->dev);
>>   	default:
>> diff --git a/drivers/vfio/cdx/private.h b/drivers/vfio/cdx/private.h
>> index 8e9d25913728..7a8477ae4652 100644
>> --- a/drivers/vfio/cdx/private.h
>> +++ b/drivers/vfio/cdx/private.h
>> @@ -13,6 +13,14 @@ static inline u64 vfio_cdx_index_to_offset(u32 index)
>>   	return ((u64)(index) << VFIO_CDX_OFFSET_SHIFT);
>>   }
>>   
>> +struct vfio_cdx_irq {
>> +	u32			flags;
>> +	u32			count;
>> +	int			irq_no;
>> +	struct eventfd_ctx	*trigger;
>> +	char			*name;
>> +};
>> +
>>   struct vfio_cdx_region {
>>   	u32			flags;
>>   	u32			type;
>> @@ -25,6 +33,16 @@ struct vfio_cdx_device {
>>   	struct vfio_cdx_region	*regions;
>>   	u32			flags;
>>   #define BME_SUPPORT BIT(0)
>> +	struct vfio_cdx_irq	*cdx_irqs;
>> +	u32			irq_count;
>> +	u32			config_msi;
>>   };
> 
> You might want to reorder these to avoid holes in the data structures.
> vfio_cdx_irq will have a 4byte hole in the middle, vfio_cdx_device will
> have a 4byte hole after flags.  config_msi is used as a bool, I'm not
> sure why it's defined as a u32.  irq_count also holds a value up to 1,
> so a u32 might be oversized.
> 
> There's clearly latent support here for devices with multiple MSI
> vectors, is this just copying vfio-pci-core or will CDX devices have
> multiple MSIs?  Thanks,

Will reorder structure fields to avoid holes. CDX support multiple MSIs 
and 'irq_count' is actually number of MSIs which can be more than 1. We 
can replace irq_count to msi_count to avoid confusion.

Thanks,
Nipun

> 
> Alex