Re: [RFC v2 1/1] vfio/platform: add support for msi
From: Auger Eric
Date: Wed Dec 02 2020 - 09:46:58 EST
Hi Vikas,
On 11/24/20 5:16 PM, Vikas Gupta wrote:
> MSI support for platform devices.
>
> Signed-off-by: Vikas Gupta <vikas.gupta@xxxxxxxxxxxx>
> ---
> drivers/vfio/platform/vfio_platform_common.c | 99 ++++++-
> drivers/vfio/platform/vfio_platform_irq.c | 260 +++++++++++++++++-
> drivers/vfio/platform/vfio_platform_private.h | 16 ++
> include/uapi/linux/vfio.h | 43 +++
> 4 files changed, 401 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index c0771a9567fb..b0bfc0f4ee1f 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -16,6 +16,7 @@
> #include <linux/types.h>
> #include <linux/uaccess.h>
> #include <linux/vfio.h>
> +#include <linux/nospec.h>
>
> #include "vfio_platform_private.h"
>
> @@ -344,9 +345,16 @@ static long vfio_platform_ioctl(void *device_data,
>
> } else if (cmd == VFIO_DEVICE_GET_IRQ_INFO) {
> struct vfio_irq_info info;
> + struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
> + struct vfio_irq_info_cap_msi *msi_info = NULL;
> + unsigned long capsz;
> + int ext_irq_index = vdev->num_irqs - vdev->num_ext_irqs;
>
> minsz = offsetofend(struct vfio_irq_info, count);
>
> + /* For backward compatibility, cannot require this */
> + capsz = offsetofend(struct vfio_irq_info, cap_offset);
> +
> if (copy_from_user(&info, (void __user *)arg, minsz))
> return -EFAULT;
>
> @@ -356,9 +364,89 @@ static long vfio_platform_ioctl(void *device_data,
> if (info.index >= vdev->num_irqs)
> return -EINVAL;
>
> + if (info.argsz >= capsz)
> + minsz = capsz;
> +
> + if (info.index == ext_irq_index) {
nit: n case we add new ext indices afterwards, I would check info.index
- ext_irq_index against an VFIO_EXT_IRQ_MSI define.
> + struct vfio_irq_info_cap_type cap_type = {
> + .header.id = VFIO_IRQ_INFO_CAP_TYPE,
> + .header.version = 1 };
> + int i;
> + int ret;
> + int num_msgs;
> + size_t msi_info_size;
> + struct vfio_platform_irq *irq;
nit: I think generally the opposite order (length) is chosen. This also
would better match the existing style in this file
> +
> + info.index = array_index_nospec(info.index,
> + vdev->num_irqs);
> +
> + irq = &vdev->irqs[info.index];
> +
> + info.flags = irq->flags;
I think this can be removed given [*]
> + cap_type.type = irq->type;
> + cap_type.subtype = irq->subtype;
> +
> + ret = vfio_info_add_capability(&caps,
> + &cap_type.header,
> + sizeof(cap_type));
> + if (ret)
> + return ret;
> +
> + num_msgs = irq->num_ctx;
do would want to return the cap even if !num_ctx?
> +
> + msi_info_size = struct_size(msi_info, msgs, num_msgs);
> +
> + msi_info = kzalloc(msi_info_size, GFP_KERNEL);
> + if (!msi_info) {
> + kfree(caps.buf);
> + return -ENOMEM;
> + }
> +
> + msi_info->header.id = VFIO_IRQ_INFO_CAP_MSI_DESCS;
> + msi_info->header.version = 1;
> + msi_info->nr_msgs = num_msgs;
> +
> + for (i = 0; i < num_msgs; i++) {
> + struct vfio_irq_ctx *ctx = &irq->ctx[i];
> +
> + msi_info->msgs[i].addr_lo = ctx->msg.address_lo;
> + msi_info->msgs[i].addr_hi = ctx->msg.address_hi;
> + msi_info->msgs[i].data = ctx->msg.data;
> + }
> +
> + ret = vfio_info_add_capability(&caps, &msi_info->header,
> + msi_info_size);
> + if (ret) {
> + kfree(msi_info);
> + kfree(caps.buf);
> + return ret;
> + }
> + }
> +
> info.flags = vdev->irqs[info.index].flags;
[*]
> info.count = vdev->irqs[info.index].count;
>
> + if (caps.size) {
> + info.flags |= VFIO_IRQ_INFO_FLAG_CAPS;
> + if (info.argsz < sizeof(info) + caps.size) {
> + info.argsz = sizeof(info) + caps.size;
> + info.cap_offset = 0;
> + } else {
> + vfio_info_cap_shift(&caps, sizeof(info));
> + if (copy_to_user((void __user *)arg +
> + sizeof(info), caps.buf,
> + caps.size)) {
> + kfree(msi_info);
> + kfree(caps.buf);
> + return -EFAULT;
> + }
> + info.cap_offset = sizeof(info);
> + }
> +
> + kfree(msi_info);
> + kfree(caps.buf);
> + }
> +
> return copy_to_user((void __user *)arg, &info, minsz) ?
> -EFAULT : 0;
>
> @@ -366,6 +454,7 @@ static long vfio_platform_ioctl(void *device_data,
> struct vfio_irq_set hdr;
> u8 *data = NULL;
> int ret = 0;
> + int max;
> size_t data_size = 0;
>
> minsz = offsetofend(struct vfio_irq_set, count);
> @@ -373,8 +462,14 @@ static long vfio_platform_ioctl(void *device_data,
> if (copy_from_user(&hdr, (void __user *)arg, minsz))
> return -EFAULT;
>
> - ret = vfio_set_irqs_validate_and_prepare(&hdr, vdev->num_irqs,
> - vdev->num_irqs, &data_size);
> + if (hdr.index >= vdev->num_irqs)
> + return -EINVAL;
> +
> + max = vdev->irqs[hdr.index].count;
> +
> + ret = vfio_set_irqs_validate_and_prepare(&hdr, max,
> + vdev->num_irqs,
> + &data_size);
> if (ret)
> return ret;
>
> diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
> index c5b09ec0a3c9..4066223e5b2e 100644
> --- a/drivers/vfio/platform/vfio_platform_irq.c
> +++ b/drivers/vfio/platform/vfio_platform_irq.c
> @@ -8,10 +8,12 @@
>
> #include <linux/eventfd.h>
> #include <linux/interrupt.h>
> +#include <linux/eventfd.h>
> #include <linux/slab.h>
> #include <linux/types.h>
> #include <linux/vfio.h>
> #include <linux/irq.h>
> +#include <linux/msi.h>
>
> #include "vfio_platform_private.h"
>
> @@ -253,6 +255,195 @@ static int vfio_platform_set_irq_trigger(struct vfio_platform_device *vdev,
> return 0;
> }
>
> +/* MSI/MSIX */
> +static irqreturn_t vfio_msihandler(int irq, void *arg)
> +{
> + struct eventfd_ctx *trigger = arg;
> +
> + eventfd_signal(trigger, 1);
> + return IRQ_HANDLED;
> +}
> +
> +static void msi_write(struct msi_desc *desc, struct msi_msg *msg)
> +{
> + int i;
> + struct vfio_platform_irq *irq;
> + u16 index = desc->platform.msi_index;
> + struct device *dev = msi_desc_to_dev(desc);
> + struct vfio_device *device = dev_get_drvdata(dev);
> + struct vfio_platform_device *vdev = (struct vfio_platform_device *)
> + vfio_device_data(device);
> +
> + for (i = 0; i < vdev->num_irqs; i++)
> + if (vdev->irqs[i].type == VFIO_IRQ_TYPE_MSI)
> + irq = &vdev->irqs[i];
> +
> + irq->ctx[index].msg.address_lo = msg->address_lo;
> + irq->ctx[index].msg.address_hi = msg->address_hi;
> + irq->ctx[index].msg.data = msg->data;
> +}
> +
> +static int vfio_msi_enable(struct vfio_platform_device *vdev,
> + struct vfio_platform_irq *irq, int nvec)
> +{
> + int ret;
> + int msi_idx = 0;
> + struct msi_desc *desc;
> + struct device *dev = vdev->device;
> +
> + irq->ctx = kcalloc(nvec, sizeof(struct vfio_irq_ctx), GFP_KERNEL);
> + if (!irq->ctx)
> + return -ENOMEM;
> +
> + /* Allocate platform MSIs */
> + ret = platform_msi_domain_alloc_irqs(dev, nvec, msi_write);
> + if (ret < 0) {
> + kfree(irq->ctx);
> + return ret;
> + }
> +
> + for_each_msi_entry(desc, dev) {
> + irq->ctx[msi_idx].hwirq = desc->irq;
> + msi_idx++;
> + }
> +
> + irq->num_ctx = nvec;
> + irq->config_msi = 1;
> +
> + return 0;
> +}
> +
> +static int vfio_msi_set_vector_signal(struct vfio_platform_irq *irq,
> + int vector, int fd)
> +{
> + struct eventfd_ctx *trigger;
> + int irq_num, ret;
> +
> + if (vector < 0 || vector >= irq->num_ctx)
> + return -EINVAL;
> +
> + irq_num = irq->ctx[vector].hwirq;
> +
> + if (irq->ctx[vector].trigger) {
> + free_irq(irq_num, irq->ctx[vector].trigger);
> + kfree(irq->ctx[vector].name);
> + eventfd_ctx_put(irq->ctx[vector].trigger);
> + irq->ctx[vector].trigger = NULL;
> + }
> +
> + if (fd < 0)
> + return 0;
> +
> + irq->ctx[vector].name = kasprintf(GFP_KERNEL,
> + "vfio-msi[%d]", vector);
> + if (!irq->ctx[vector].name)
> + return -ENOMEM;
> +
> + trigger = eventfd_ctx_fdget(fd);
> + if (IS_ERR(trigger)) {
> + kfree(irq->ctx[vector].name);
> + return PTR_ERR(trigger);
> + }
> +
> + ret = request_irq(irq_num, vfio_msihandler, 0,
> + irq->ctx[vector].name, trigger);
> + if (ret) {
> + kfree(irq->ctx[vector].name);
> + eventfd_ctx_put(trigger);
> + return ret;
> + }
> +
> + irq->ctx[vector].trigger = trigger;
> +
> + return 0;
> +}
> +
> +static int vfio_msi_set_block(struct vfio_platform_irq *irq, unsigned int start,
> + unsigned int count, int32_t *fds)
> +{
> + int i, j, ret = 0;
> +
> + if (start >= irq->num_ctx || start + count > irq->num_ctx)
> + return -EINVAL;
> +
> + for (i = 0, j = start; i < count && !ret; i++, j++) {
> + int fd = fds ? fds[i] : -1;
> +
> + ret = vfio_msi_set_vector_signal(irq, j, fd);
> + }
> +
> + if (ret) {
> + for (--j; j >= (int)start; j--)
> + vfio_msi_set_vector_signal(irq, j, -1);
> + }
> +
> + return ret;
> +}
> +
> +static void vfio_msi_disable(struct vfio_platform_device *vdev,
> + struct vfio_platform_irq *irq)
> +{
> + struct device *dev = vdev->device;
> +
> + vfio_msi_set_block(irq, 0, irq->num_ctx, NULL);
> +
> + platform_msi_domain_free_irqs(dev);
> +
> + irq->config_msi = 0;
> + irq->num_ctx = 0;
> +
> + kfree(irq->ctx);
> +}
> +
> +static int vfio_set_msi_trigger(struct vfio_platform_device *vdev,
> + unsigned int index, unsigned int start,
> + unsigned int count, uint32_t flags, void *data)
> +{
> + int i;
> + struct vfio_platform_irq *irq = &vdev->irqs[index];
> +
> + if (start + count > irq->count)
> + return -EINVAL;
> +
> + if (!count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
> + vfio_msi_disable(vdev, irq);
> + return 0;
> + }
> +
> + if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> + s32 *fds = data;
> + int ret;
> +
> + if (irq->config_msi)
> + return vfio_msi_set_block(irq, start, count,
> + fds);
> + ret = vfio_msi_enable(vdev, irq, start + count);
> + if (ret)
> + return ret;
> +
> + ret = vfio_msi_set_block(irq, start, count, fds);
> + if (ret)
> + vfio_msi_disable(vdev, irq);
> +
> + return ret;
> + }
> +
> + for (i = start; i < start + count; i++) {
> + if (!irq->ctx[i].trigger)
> + continue;
> + if (flags & VFIO_IRQ_SET_DATA_NONE) {
> + eventfd_signal(irq->ctx[i].trigger, 1);
> + } else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
> + u8 *bools = data;
> +
> + if (bools[i - start])
> + eventfd_signal(irq->ctx[i].trigger, 1);
> + }
> + }
> +
> + return 0;
> +}
> +
> int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
> uint32_t flags, unsigned index, unsigned start,
> unsigned count, void *data)
> @@ -261,16 +452,29 @@ int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
> unsigned start, unsigned count, uint32_t flags,
> void *data) = NULL;
>
> - switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
> - case VFIO_IRQ_SET_ACTION_MASK:
> - func = vfio_platform_set_irq_mask;
> - break;
> - case VFIO_IRQ_SET_ACTION_UNMASK:
> - func = vfio_platform_set_irq_unmask;
> - break;
> - case VFIO_IRQ_SET_ACTION_TRIGGER:
> - func = vfio_platform_set_irq_trigger;
> - break;
> + struct vfio_platform_irq *irq = &vdev->irqs[index];
> +
> + if (irq->type == VFIO_IRQ_TYPE_MSI) {
> + switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
> + case VFIO_IRQ_SET_ACTION_MASK:
> + case VFIO_IRQ_SET_ACTION_UNMASK:
> + break;
> + case VFIO_IRQ_SET_ACTION_TRIGGER:
> + func = vfio_set_msi_trigger;
> + break;
> + }
> + } else {
> + switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
> + case VFIO_IRQ_SET_ACTION_MASK:
> + func = vfio_platform_set_irq_mask;
> + break;
> + case VFIO_IRQ_SET_ACTION_UNMASK:
> + func = vfio_platform_set_irq_unmask;
> + break;
> + case VFIO_IRQ_SET_ACTION_TRIGGER:
> + func = vfio_platform_set_irq_trigger;
> + break;
> + }
> }
>
> if (!func)
> @@ -281,12 +485,21 @@ int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
>
> int vfio_platform_irq_init(struct vfio_platform_device *vdev)
> {
> - int cnt = 0, i;
> + int i;
> + int cnt = 0;
> + int num_irqs;
> + struct device *dev = vdev->device;
>
> while (vdev->get_irq(vdev, cnt) >= 0)
> cnt++;
>
> - vdev->irqs = kcalloc(cnt, sizeof(struct vfio_platform_irq), GFP_KERNEL);
> + num_irqs = cnt;
> +
> + if (dev->msi_domain)
> + num_irqs++;
> +
> + vdev->irqs = kcalloc(num_irqs, sizeof(struct vfio_platform_irq),
> + GFP_KERNEL);
> if (!vdev->irqs)
> return -ENOMEM;
>
> @@ -309,7 +522,19 @@ int vfio_platform_irq_init(struct vfio_platform_device *vdev)
> vdev->irqs[i].masked = false;
> }
>
> - vdev->num_irqs = cnt;
> + /*
> + * MSI block is added at last index and its an ext irq
it is
> + */
> + if (dev->msi_domain) {
> + vdev->irqs[i].flags = VFIO_IRQ_INFO_EVENTFD;
> + vdev->irqs[i].count = NR_IRQS;
why NR_IRQS?
> + vdev->irqs[i].hwirq = 0;
> + vdev->irqs[i].masked = false;
> + vdev->irqs[i].type = VFIO_IRQ_TYPE_MSI;
> + vdev->num_ext_irqs = 1;
> + }
> +
> + vdev->num_irqs = num_irqs;
>
> return 0;
> err:
> @@ -321,8 +546,13 @@ void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev)
> {
> int i;
>
> - for (i = 0; i < vdev->num_irqs; i++)
> - vfio_set_trigger(vdev, i, -1, NULL);
> + for (i = 0; i < vdev->num_irqs; i++) {
> + if (vdev->irqs[i].type == VFIO_IRQ_TYPE_MSI)
> + vfio_set_msi_trigger(vdev, i, 0, 0,
> + VFIO_IRQ_SET_DATA_NONE, NULL);
> + else
> + vfio_set_trigger(vdev, i, -1, NULL);
> + }
>
> vdev->num_irqs = 0;
> kfree(vdev->irqs);
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index 289089910643..7bbb05988705 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -9,6 +9,7 @@
>
> #include <linux/types.h>
> #include <linux/interrupt.h>
> +#include <linux/msi.h>
>
> #define VFIO_PLATFORM_OFFSET_SHIFT 40
> #define VFIO_PLATFORM_OFFSET_MASK (((u64)(1) << VFIO_PLATFORM_OFFSET_SHIFT) - 1)
> @@ -19,9 +20,18 @@
> #define VFIO_PLATFORM_INDEX_TO_OFFSET(index) \
> ((u64)(index) << VFIO_PLATFORM_OFFSET_SHIFT)
>
> +struct vfio_irq_ctx {
> + int hwirq;
> + char *name;
> + struct msi_msg msg;
> + struct eventfd_ctx *trigger;
> +};
> +
> struct vfio_platform_irq {
> u32 flags;
> u32 count;
> + int num_ctx;
> + struct vfio_irq_ctx *ctx;
> int hwirq;
> char *name;
> struct eventfd_ctx *trigger;
> @@ -29,6 +39,11 @@ struct vfio_platform_irq {
> spinlock_t lock;
> struct virqfd *unmask;
> struct virqfd *mask;
> +
> + /* for extended irqs */
> + u32 type;
> + u32 subtype;
> + int config_msi;
> };
>
> struct vfio_platform_region {
> @@ -46,6 +61,7 @@ struct vfio_platform_device {
> u32 num_regions;
> struct vfio_platform_irq *irqs;
> u32 num_irqs;
> + int num_ext_irqs;
> int refcnt;
> struct mutex igate;
> struct module *parent_module;
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 2f313a238a8f..598d1c944283 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -697,11 +697,54 @@ struct vfio_irq_info {
> #define VFIO_IRQ_INFO_MASKABLE (1 << 1)
> #define VFIO_IRQ_INFO_AUTOMASKED (1 << 2)
> #define VFIO_IRQ_INFO_NORESIZE (1 << 3)
> +#define VFIO_IRQ_INFO_FLAG_CAPS (1 << 4) /* Info supports caps */
> __u32 index; /* IRQ index */
> __u32 count; /* Number of IRQs within this index */
> + __u32 cap_offset; /* Offset within info struct of first cap */
> };
> #define VFIO_DEVICE_GET_IRQ_INFO _IO(VFIO_TYPE, VFIO_BASE + 9)
>
> +/*
> + * The irq type capability allows IRQs unique to a specific device or
> + * class of devices to be exposed.
> + *
> + * The structures below define version 1 of this capability.
> + */
> +#define VFIO_IRQ_INFO_CAP_TYPE 3
> +
> +struct vfio_irq_info_cap_type {
> + struct vfio_info_cap_header header;
> + __u32 type; /* global per bus driver */
> + __u32 subtype; /* type specific */
> +};
> +
> +/*
> + * List of IRQ types, global per bus driver.
> + * If you introduce a new type, please add it here.
> + */
> +
> +/* Non PCI devices having MSI(s) support */
> +#define VFIO_IRQ_TYPE_MSI (1)
> +
> +/*
> + * The msi capability allows the user to use the msi msg to
> + * configure the iova for the msi configuration.
> + * The structures below define version 1 of this capability.
> + */
> +#define VFIO_IRQ_INFO_CAP_MSI_DESCS 4
> +
> +struct vfio_irq_msi_msg {
> + __u32 addr_lo;
> + __u32 addr_hi;
> + __u32 data;
> +};
> +
> +struct vfio_irq_info_cap_msi {
> + struct vfio_info_cap_header header;
> + __u32 nr_msgs;
I think you should align a __u32 reserved field to have a 64b alignment
> + struct vfio_irq_msi_msg msgs[];
Please can you clarify why this cap is needed versus your prior approach.
> +};
> +
> /**
> * VFIO_DEVICE_SET_IRQS - _IOW(VFIO_TYPE, VFIO_BASE + 10, struct vfio_irq_set)
> *
>
Thanks
Eric