Re: [PATCH v5 2/2] vfio/cdx: add interrupt support

["Gupta, Nipun" <nipun.gupta@amd.com>]

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