| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4ffb4f53-a7a7-4ef7-88cc-e2042e953f87@amd.com>
Date: Mon, 22 Apr 2024 18:21:11 +0530
From: "Gupta, Nipun" <nipun.gupta@....com>
To: Alex Williamson <alex.williamson@...hat.com>
Cc: tglx@...utronix.de, gregkh@...uxfoundation.org,
linux-kernel@...r.kernel.org, kvm@...r.kernel.org, maz@...nel.org,
git@....com, harpreet.anand@....com, pieter.jansen-van-vuuren@....com,
nikhil.agarwal@....com, michal.simek@....com, abhijit.gangurde@....com,
srivatsa@...il.mit.edu
Subject: Re: [PATCH v5 2/2] vfio/cdx: add interrupt support
On 4/20/2024 4:01 AM, Alex Williamson wrote:
> On Tue, 26 Mar 2024 09:38:04 +0530
> Nipun Gupta <nipun.gupta@....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@....com>
>> Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@....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
Powered by blists - more mailing lists