On Tue, 21 Feb 2017 07:48:27 +0200
"Michael S. Tsirkin" <mst@xxxxxxxxxx> wrote:
> On Sun, Feb 19, 2017 at 10:47:07PM +0800, Lan Tianyu wrote:
> > This patch is to receive IOMMU fault eventfd and IOMMU fault event flag
> > from userspace. VFIO should register IOMMU fault event handler according
> > fault event flag which designates what kind of fault events need to be
> > reported. When VFIO get IOMMU fault event from IOMMU driver, it should
> > notify userspace via received fd.
> >
> > Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> > ---
> > drivers/vfio/vfio_iommu_type1.c | 45 +++++++++++++++++++++++++++++++++++++++++
> > include/uapi/linux/vfio.h | 15 ++++++++++++++
> > 2 files changed, 60 insertions(+)
> >
> > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> > index b3cc33f..46674ea 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -38,6 +38,7 @@
> > #include <linux/workqueue.h>
> > #include <linux/mdev.h>
> > #include <linux/notifier.h>
> > +#include <linux/eventfd.h>
> >
> > #define DRIVER_VERSION "0.2"
> > #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@xxxxxxxxxx>"
> > @@ -61,6 +62,8 @@ struct vfio_iommu {
> > struct mutex lock;
> > struct rb_root dma_list;
> > struct blocking_notifier_head notifier;
> > + struct eventfd_ctx *iommu_fault_fd;
> > + struct mutex fault_lock;
> > bool v2;
> > bool nesting;
> > };
> > @@ -1452,6 +1455,7 @@ static void *vfio_iommu_type1_open(unsigned long arg)
> > INIT_LIST_HEAD(&iommu->domain_list);
> > iommu->dma_list = RB_ROOT;
> > mutex_init(&iommu->lock);
> > + mutex_init(&iommu->fault_lock);
> > BLOCKING_INIT_NOTIFIER_HEAD(&iommu->notifier);
> >
> > return iommu;
> > @@ -1582,6 +1586,47 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> >
> > return copy_to_user((void __user *)arg, &unmap, minsz) ?
> > -EFAULT : 0;
> > + } else if (cmd == VFIO_IOMMU_SET_FAULT_EVENTFD) {
> > + struct vfio_iommu_type1_set_fault_eventfd eventfd;
> > + int fd;
> > + int ret;
> > +
> > + minsz = offsetofend(struct vfio_iommu_type1_set_fault_eventfd,
> > + fd);
> > +
> > + if (copy_from_user(&eventfd, (void __user *)arg, minsz))
> > + return -EFAULT;
> > +
> > + if (eventfd.argsz < minsz)
> > + return -EINVAL;
> > +
> > +
> > + mutex_lock(&iommu->fault_lock);
> > +
> > + fd = eventfd.fd;
> > + if (fd == -1) {
> > + if (iommu->iommu_fault_fd)
> > + eventfd_ctx_put(iommu->iommu_fault_fd);
> > + iommu->iommu_fault_fd = NULL;
> > + ret = 0;
> > + } else if (fd >= 0) {
> > + struct eventfd_ctx *ctx;
> > +
> > + ctx = eventfd_ctx_fdget(fd);
> > + if (IS_ERR(ctx))
> > + return PTR_ERR(ctx);
> > +
> > + if (ctx)
> > + eventfd_ctx_put(ctx);
> > +
> > + iommu->iommu_fault_fd = ctx;
> > + ret = 0;
> > + } else
> > + ret = -EINVAL;
> > +
> > + mutex_unlock(&iommu->fault_lock);
> > +
> > + return ret;
> > }
> >
> > return -ENOTTY;
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 519eff3..8616334 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -547,6 +547,21 @@ struct vfio_iommu_type1_dma_unmap {
> > #define VFIO_IOMMU_ENABLE _IO(VFIO_TYPE, VFIO_BASE + 15)
> > #define VFIO_IOMMU_DISABLE _IO(VFIO_TYPE, VFIO_BASE + 16)
> >
> > +/*
> > + * VFIO_IOMMU_SET_FAULT_EVENT_FD _IO(VFIO_TYPE, VFIO_BASE + 17)
> > + *
> > + * Receive eventfd from userspace to notify fault event from IOMMU.
> > + */
> > +struct vfio_iommu_type1_set_fault_eventfd {
> > + __u32 argsz;
> > + __u32 flags;
> > +/* What IOMMU Fault events should be reported. */
> > +#define VFIO_IOMMU_UR_FAULT_WITHOUT_PASID (1 << 0)
> > + __s32 fd; /* Eventfd from user space */
> > +};
> > +
> > +#define VFIO_IOMMU_SET_FAULT_EVENTFD _IO(VFIO_TYPE, VFIO_BASE + 17)
> > +
> > /* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */
>
> How about simply doing
>
> VFIO_PCI_MSIX_IRQ_INDEX,
> VFIO_PCI_ERR_IRQ_INDEX,
> VFIO_PCI_REQ_IRQ_INDEX,
> + VFIO_PCI_IOMMU_UR_FAULT_WITHOUT_PASID_IRQ_INDEX,
> VFIO_PCI_NUM_IRQS
>
>
> instead?
>
> This way you don't need to write a whole new kernel/userspace
> interface.
It seems there are pros and cons either way, but this is fundamentally
different, making the iommu fault exposed on the device rather than the
container. The container represents the iommu state, so it makes some
sense to report the fault for the container. On the other hand if
it's reported per device there's no matching of source ID to a
device. It certainly affects how the iommu fault reporting is designed,
whether it's a callback added to the device driver or a component of
the IOMMU API. Thanks,
Alex
