Messages in this thread | | | Date | Mon, 22 Apr 2024 18:21:11 +0530 | Subject | Re: [PATCH v5 2/2] vfio/cdx: add interrupt support | From | "Gupta, Nipun" <> |
| |
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
| |