linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] add VFIO mdev support for DFL devices
@ 2020-09-08  7:13 Xu Yilun
  2020-09-08  7:13 ` [PATCH 1/3] fpga: dfl: add driver_override support Xu Yilun
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Xu Yilun @ 2020-09-08  7:13 UTC (permalink / raw)
  To: mdf, alex.williamson, kwankhede, linux-fpga, kvm, linux-kernel
  Cc: trix, lgoncalv, yilun.xu

These patches depend on the patchset: "Modularization of DFL private
feature drivers" & "add dfl bus support to MODULE_DEVICE_TABLE()"

https://lore.kernel.org/linux-fpga/1599488581-16386-1-git-send-email-yilun.xu@intel.com/

This patchset provides an VFIO Mdev driver for dfl devices. It makes
possible for dfl devices be direct accessed from userspace.

Xu Yilun (3):
  fpga: dfl: add driver_override support
  fpga: dfl: VFIO mdev support for DFL devices
  Documentation: fpga: dfl: Add description for VFIO Mdev support

 Documentation/ABI/testing/sysfs-bus-dfl |  20 ++
 Documentation/fpga/dfl.rst              |  20 ++
 drivers/fpga/Kconfig                    |   9 +
 drivers/fpga/Makefile                   |   1 +
 drivers/fpga/dfl.c                      |  54 ++++-
 drivers/fpga/vfio-mdev-dfl.c            | 391 ++++++++++++++++++++++++++++++++
 include/linux/fpga/dfl-bus.h            |   2 +
 7 files changed, 496 insertions(+), 1 deletion(-)
 create mode 100644 drivers/fpga/vfio-mdev-dfl.c

-- 
2.7.4


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/3] fpga: dfl: add driver_override support
  2020-09-08  7:13 [PATCH 0/3] add VFIO mdev support for DFL devices Xu Yilun
@ 2020-09-08  7:13 ` Xu Yilun
  2020-09-08  7:13 ` [PATCH 2/3] fpga: dfl: VFIO mdev support for DFL devices Xu Yilun
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Xu Yilun @ 2020-09-08  7:13 UTC (permalink / raw)
  To: mdf, alex.williamson, kwankhede, linux-fpga, kvm, linux-kernel
  Cc: trix, lgoncalv, yilun.xu

Add support for overriding the default matching of a dfl device to a dfl
driver. It follows the same way that can be used for PCI and platform
devices. This patch adds the 'driver_override' sysfs file. It can be
used by VFIO to bind dfl devices for user accessing.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-dfl | 20 ++++++++++++
 drivers/fpga/dfl.c                      | 54 ++++++++++++++++++++++++++++++++-
 include/linux/fpga/dfl-bus.h            |  2 ++
 3 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-dfl b/Documentation/ABI/testing/sysfs-bus-dfl
index 23543be..cf1357a 100644
--- a/Documentation/ABI/testing/sysfs-bus-dfl
+++ b/Documentation/ABI/testing/sysfs-bus-dfl
@@ -13,3 +13,23 @@ Contact:	Xu Yilun <yilun.xu@intel.com>
 Description:	Read-only. It returns feature identifier local to its DFL FIU
 		type.
 		Format: 0x%x
+
+What:		/sys/bus/dfl/devices/.../driver_override
+Date:		Sep 2020
+KernelVersion:	5.10
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	This file allows the driver for a device to be specified. When
+		specified, only a driver with a name matching the value written
+		to driver_override will have an opportunity to bind to the
+		device. The override is specified by writing a string to the
+		driver_override file (echo vfio-mdev-dfl > driver_override) and
+		may be cleared with an empty string (echo > driver_override).
+		This returns the device to standard matching rules binding.
+		Writing to driver_override does not automatically unbind the
+		device from its current driver or make any attempt to
+		automatically load the specified driver.  If no driver with a
+		matching name is currently loaded in the kernel, the device
+		will not bind to any driver.  This also allows devices to
+		opt-out of driver binding using a driver_override name such as
+		"none".  Only a single driver may be specified in the override,
+		there is no support for parsing delimiters.
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 02a6780..98f88ee 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -262,6 +262,10 @@ static int dfl_bus_match(struct device *dev, struct device_driver *drv)
 	struct dfl_driver *ddrv = to_dfl_drv(drv);
 	const struct dfl_device_id *id_entry;
 
+	/* When driver_override is set, only bind to the matching driver */
+	if (ddev->driver_override)
+		return !strcmp(ddev->driver_override, drv->name);
+
 	id_entry = ddrv->id_table;
 	if (id_entry) {
 		while (id_entry->feature_id) {
@@ -304,6 +308,53 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
 			      ddev->type, ddev->feature_id);
 }
 
+static ssize_t driver_override_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct dfl_device *ddev = to_dfl_dev(dev);
+	ssize_t len;
+
+	device_lock(dev);
+	len = sprintf(buf, "%s\n", ddev->driver_override);
+	device_unlock(dev);
+	return len;
+}
+
+static ssize_t driver_override_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct dfl_device *ddev = to_dfl_dev(dev);
+	char *driver_override, *old, *cp;
+
+	/* We need to keep extra room for a newline */
+	if (count >= (PAGE_SIZE - 1))
+		return -EINVAL;
+
+	driver_override = kstrndup(buf, count, GFP_KERNEL);
+	if (!driver_override)
+		return -ENOMEM;
+
+	cp = strchr(driver_override, '\n');
+	if (cp)
+		*cp = '\0';
+
+	device_lock(dev);
+	old = ddev->driver_override;
+	if (strlen(driver_override)) {
+		ddev->driver_override = driver_override;
+	} else {
+		kfree(driver_override);
+		ddev->driver_override = NULL;
+	}
+	device_unlock(dev);
+
+	kfree(old);
+
+	return count;
+}
+static DEVICE_ATTR_RW(driver_override);
+
 static ssize_t
 type_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -325,6 +376,7 @@ static DEVICE_ATTR_RO(feature_id);
 static struct attribute *dfl_dev_attrs[] = {
 	&dev_attr_type.attr,
 	&dev_attr_feature_id.attr,
+	&dev_attr_driver_override.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(dfl_dev);
@@ -470,7 +522,7 @@ static int dfl_devs_add(struct dfl_feature_platform_data *pdata)
 
 int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
 {
-	if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
+	if (!dfl_drv || !dfl_drv->probe)
 		return -EINVAL;
 
 	dfl_drv->drv.owner = owner;
diff --git a/include/linux/fpga/dfl-bus.h b/include/linux/fpga/dfl-bus.h
index 2a2b283..09a9ee1 100644
--- a/include/linux/fpga/dfl-bus.h
+++ b/include/linux/fpga/dfl-bus.h
@@ -43,6 +43,8 @@ struct dfl_device {
 	unsigned int num_irqs;
 	struct dfl_fpga_cdev *cdev;
 	const struct dfl_device_id *id_entry;
+
+	char *driver_override;
 };
 
 /**
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/3] fpga: dfl: VFIO mdev support for DFL devices
  2020-09-08  7:13 [PATCH 0/3] add VFIO mdev support for DFL devices Xu Yilun
  2020-09-08  7:13 ` [PATCH 1/3] fpga: dfl: add driver_override support Xu Yilun
@ 2020-09-08  7:13 ` Xu Yilun
  2020-09-08  7:13 ` [PATCH 3/3] Documentation: fpga: dfl: Add description for VFIO Mdev support Xu Yilun
  2020-09-09 21:13 ` [PATCH 0/3] add VFIO mdev support for DFL devices Tom Rix
  3 siblings, 0 replies; 10+ messages in thread
From: Xu Yilun @ 2020-09-08  7:13 UTC (permalink / raw)
  To: mdf, alex.williamson, kwankhede, linux-fpga, kvm, linux-kernel
  Cc: trix, lgoncalv, yilun.xu

This patch is to provide VFIO support to DFL devices. When bound this
driver to a dfl device, it registers a set of callbacks to the mediated
device framework to enable mediated device creation. When a mediated
device is created by user via mdev interfaces, it will be exposed to
user as a VFIO platform device.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
 drivers/fpga/Kconfig         |   9 +
 drivers/fpga/Makefile        |   1 +
 drivers/fpga/vfio-mdev-dfl.c | 391 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 401 insertions(+)
 create mode 100644 drivers/fpga/vfio-mdev-dfl.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 38c7130..88f64fb 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -202,6 +202,15 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
 	  the card. It also instantiates the SPI master (spi-altera) for
 	  the card's BMC (Board Management Controller) control.
 
+config FPGA_VFIO_MDEV_DFL
+        tristate "VFIO Mdev support for DFL devices"
+        depends on FPGA_DFL && VFIO_MDEV
+        help
+	  Support for DFL devices with VFIO Mdev. This is required to make use
+	  of DFL devices present on the system using the VFIO Mdev framework.
+
+	  If you don't know what to do here, say N.
+
 config FPGA_DFL_PCI
 	tristate "FPGA DFL PCIe Device Driver"
 	depends on PCI && FPGA_DFL
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 18dc9885..c69bfc9 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
 dfl-afu-objs += dfl-afu-error.o
 
 obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
+obj-$(CONFIG_FPGA_VFIO_MDEV_DFL)	+= vfio-mdev-dfl.o
 
 # Drivers for FPGAs which implement DFL
 obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
diff --git a/drivers/fpga/vfio-mdev-dfl.c b/drivers/fpga/vfio-mdev-dfl.c
new file mode 100644
index 0000000..34fb19f
--- /dev/null
+++ b/drivers/fpga/vfio-mdev-dfl.c
@@ -0,0 +1,391 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * VFIO Mediated device driver for DFL devices
+ *
+ * Copyright (C) 2019-2020 Intel Corporation, Inc.
+ */
+#include <linux/device.h>
+#include <linux/fpga/dfl-bus.h>
+#include <linux/init.h>
+#include <linux/iommu.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/kernel.h>
+#include <linux/mdev.h>
+#include <linux/module.h>
+#include <linux/poll.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/vfio.h>
+
+struct vfio_mdev_dfl_dev {
+	struct device *dev;
+	void __iomem *ioaddr;
+	resource_size_t phys;
+	resource_size_t memsize;
+	int num_irqs;
+	u32 region_flags;
+	atomic_t avail;
+};
+
+static ssize_t
+name_show(struct kobject *kobj, struct device *dev, char *buf)
+{
+	return sprintf(buf, "%s-type1\n", dev_name(dev));
+}
+static MDEV_TYPE_ATTR_RO(name);
+
+static ssize_t
+available_instances_show(struct kobject *kobj, struct device *dev, char *buf)
+{
+	struct vfio_mdev_dfl_dev *vmdd = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", atomic_read(&vmdd->avail));
+}
+static MDEV_TYPE_ATTR_RO(available_instances);
+
+static ssize_t device_api_show(struct kobject *kobj, struct device *dev,
+			       char *buf)
+{
+	return sprintf(buf, "%s\n", VFIO_DEVICE_API_PLATFORM_STRING);
+}
+static MDEV_TYPE_ATTR_RO(device_api);
+
+static struct attribute *mdev_types_attrs[] = {
+	&mdev_type_attr_name.attr,
+	&mdev_type_attr_device_api.attr,
+	&mdev_type_attr_available_instances.attr,
+	NULL,
+};
+
+static struct attribute_group dfl_mdev_type_group1 = {
+	.name  = "1",
+	.attrs = mdev_types_attrs,
+};
+
+static struct attribute_group *dfl_mdev_type_groups[] = {
+	&dfl_mdev_type_group1,
+	NULL,
+};
+
+static int dfl_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
+{
+	struct vfio_mdev_dfl_dev *vmdd =
+		dev_get_drvdata(mdev_parent_dev(mdev));
+
+	if (atomic_dec_if_positive(&vmdd->avail) < 0)
+		return -EPERM;
+
+	return 0;
+}
+
+static int dfl_mdev_remove(struct mdev_device *mdev)
+{
+	struct vfio_mdev_dfl_dev *vmdd =
+		dev_get_drvdata(mdev_parent_dev(mdev));
+
+	atomic_inc(&vmdd->avail);
+
+	return 0;
+}
+
+static ssize_t dfl_mdev_read(struct mdev_device *mdev, char __user *buf,
+			     size_t count, loff_t *ppos)
+{
+	struct vfio_mdev_dfl_dev *vmdd =
+		dev_get_drvdata(mdev_parent_dev(mdev));
+	unsigned int done = 0;
+	loff_t off = *ppos;
+
+	if (off + count > vmdd->memsize)
+		return -EFAULT;
+
+	while (count) {
+		size_t filled;
+
+		if (count >= 8 && !(off % 8)) {
+			u64 val;
+
+			val = ioread64(vmdd->ioaddr + off);
+			if (copy_to_user(buf, &val, 8))
+				goto err;
+
+			filled = 8;
+		} else if (count >= 4 && !(off % 4)) {
+			u32 val;
+
+			val = ioread32(vmdd->ioaddr + off);
+			if (copy_to_user(buf, &val, 4))
+				goto err;
+
+			filled = 4;
+		} else if (count >= 2 && !(off % 2)) {
+			u16 val;
+
+			val = ioread16(vmdd->ioaddr + off);
+			if (copy_to_user(buf, &val, 2))
+				goto err;
+
+			filled = 2;
+		} else {
+			u8 val;
+
+			val = ioread8(vmdd->ioaddr + off);
+			if (copy_to_user(buf, &val, 1))
+				goto err;
+
+			filled = 1;
+		}
+
+		count -= filled;
+		done += filled;
+		off += filled;
+		buf += filled;
+	}
+
+	return done;
+err:
+	return -EFAULT;
+}
+
+static ssize_t dfl_mdev_write(struct mdev_device *mdev, const char __user *buf,
+			      size_t count, loff_t *ppos)
+{
+	struct vfio_mdev_dfl_dev *vmdd =
+		dev_get_drvdata(mdev_parent_dev(mdev));
+	unsigned int done = 0;
+	loff_t off = *ppos;
+
+	if (off + count > vmdd->memsize)
+		return -EFAULT;
+
+	while (count) {
+		size_t filled;
+
+		if (count >= 8 && !(off % 8)) {
+			u64 val;
+
+			if (copy_from_user(&val, buf, 8))
+				goto err;
+			iowrite64(val, vmdd->ioaddr + off);
+
+			filled = 8;
+		} else if (count >= 4 && !(off % 4)) {
+			u32 val;
+
+			if (copy_from_user(&val, buf, 4))
+				goto err;
+			iowrite32(val, vmdd->ioaddr + off);
+
+			filled = 4;
+		} else if (count >= 2 && !(off % 2)) {
+			u16 val;
+
+			if (copy_from_user(&val, buf, 2))
+				goto err;
+			iowrite16(val, vmdd->ioaddr + off);
+
+			filled = 2;
+		} else {
+			u8 val;
+
+			if (copy_from_user(&val, buf, 1))
+				goto err;
+			iowrite8(val, vmdd->ioaddr + off);
+
+			filled = 1;
+		}
+
+		count -= filled;
+		done += filled;
+		off += filled;
+		buf += filled;
+	}
+
+	return done;
+err:
+	return -EFAULT;
+}
+
+static long dfl_mdev_ioctl(struct mdev_device *mdev, unsigned int cmd,
+			   unsigned long arg)
+{
+	struct vfio_mdev_dfl_dev *vmdd =
+		dev_get_drvdata(mdev_parent_dev(mdev));
+	unsigned long minsz;
+
+	switch (cmd) {
+	case VFIO_DEVICE_GET_INFO:
+	{
+		struct vfio_device_info info;
+
+		minsz = offsetofend(struct vfio_device_info, num_irqs);
+
+		if (copy_from_user(&info, (void __user *)arg, minsz))
+			return -EFAULT;
+
+		if (info.argsz < minsz)
+			return -EINVAL;
+
+		info.flags = VFIO_DEVICE_FLAGS_PLATFORM;
+		info.num_regions = 1;
+		info.num_irqs = vmdd->num_irqs;
+
+		return copy_to_user((void __user *)arg, &info, minsz) ?
+			-EFAULT : 0;
+	}
+	case VFIO_DEVICE_GET_REGION_INFO:
+	{
+		struct vfio_region_info info;
+
+		minsz = offsetofend(struct vfio_region_info, offset);
+
+		if (copy_from_user(&info, (void __user *)arg, minsz))
+			return -EFAULT;
+
+		if (info.argsz < minsz)
+			return -EINVAL;
+
+		if (info.index >= 1)
+			return -EINVAL;
+
+		info.offset = 0;
+		info.size = vmdd->memsize;
+		info.flags = vmdd->region_flags;
+
+		return copy_to_user((void __user *)arg, &info, minsz) ?
+			-EFAULT : 0;
+	}
+	}
+
+	return -ENOTTY;
+}
+
+static int dfl_mdev_mmap_mmio(struct vfio_mdev_dfl_dev *vmdd,
+			      struct vm_area_struct *vma)
+{
+	u64 req_len, req_start;
+
+	req_len = vma->vm_end - vma->vm_start;
+	req_start = vma->vm_pgoff << PAGE_SHIFT;
+
+	if (vmdd->memsize < PAGE_SIZE || req_start + req_len > vmdd->memsize)
+		return -EINVAL;
+
+	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+
+	return remap_pfn_range(vma, vma->vm_start,
+			       (vmdd->phys + req_start) >> PAGE_SHIFT,
+			       req_len, vma->vm_page_prot);
+}
+
+static int dfl_mdev_mmap(struct mdev_device *mdev, struct vm_area_struct *vma)
+{
+	struct vfio_mdev_dfl_dev *vmdd =
+		dev_get_drvdata(mdev_parent_dev(mdev));
+
+	if (vma->vm_end < vma->vm_start)
+		return -EINVAL;
+	if (!(vma->vm_flags & VM_SHARED))
+		return -EINVAL;
+	if (vma->vm_start & ~PAGE_MASK)
+		return -EINVAL;
+	if (vma->vm_end & ~PAGE_MASK)
+		return -EINVAL;
+
+	if (!(vmdd->region_flags & VFIO_REGION_INFO_FLAG_MMAP))
+		return -EINVAL;
+
+	if (!(vmdd->region_flags & VFIO_REGION_INFO_FLAG_READ) &&
+	    (vma->vm_flags & VM_READ))
+		return -EINVAL;
+
+	if (!(vmdd->region_flags & VFIO_REGION_INFO_FLAG_WRITE) &&
+	    (vma->vm_flags & VM_WRITE))
+		return -EINVAL;
+
+	vma->vm_private_data = vmdd;
+
+	return dfl_mdev_mmap_mmio(vmdd, vma);
+}
+
+static int dfl_mdev_open(struct mdev_device *mdev)
+{
+	if (!try_module_get(THIS_MODULE))
+		return -ENODEV;
+
+	return 0;
+}
+
+static void dfl_mdev_close(struct mdev_device *mdev)
+{
+	module_put(THIS_MODULE);
+}
+
+static const struct mdev_parent_ops dfl_mdev_ops = {
+	.owner                  = THIS_MODULE,
+	.supported_type_groups  = dfl_mdev_type_groups,
+	.create                 = dfl_mdev_create,
+	.remove			= dfl_mdev_remove,
+	.open                   = dfl_mdev_open,
+	.release                = dfl_mdev_close,
+	.read                   = dfl_mdev_read,
+	.write                  = dfl_mdev_write,
+	.ioctl		        = dfl_mdev_ioctl,
+	.mmap			= dfl_mdev_mmap,
+};
+
+static int vfio_mdev_dfl_probe(struct dfl_device *dfl_dev)
+{
+	struct device *dev = &dfl_dev->dev;
+	struct vfio_mdev_dfl_dev *vmdd;
+
+	vmdd = devm_kzalloc(dev, sizeof(*vmdd), GFP_KERNEL);
+	if (!vmdd)
+		return -ENOMEM;
+
+	dev_set_drvdata(&dfl_dev->dev, vmdd);
+
+	atomic_set(&vmdd->avail, 1);
+	vmdd->dev = &dfl_dev->dev;
+	vmdd->phys = dfl_dev->mmio_res.start;
+	vmdd->memsize = resource_size(&dfl_dev->mmio_res);
+	vmdd->region_flags = VFIO_REGION_INFO_FLAG_READ |
+			    VFIO_REGION_INFO_FLAG_WRITE;
+	/*
+	 * Only regions addressed with PAGE granularity may be MMAPed
+	 * securely.
+	 */
+	if (!(vmdd->phys & ~PAGE_MASK) && !(vmdd->memsize & ~PAGE_MASK))
+		vmdd->region_flags |= VFIO_REGION_INFO_FLAG_MMAP;
+
+	vmdd->ioaddr = devm_ioremap_resource(&dfl_dev->dev, &dfl_dev->mmio_res);
+	if (IS_ERR(vmdd->ioaddr)) {
+		dev_err(dev, "get mem resource fail!\n");
+		return PTR_ERR(vmdd->ioaddr);
+	}
+
+	/* irq not supported yet */
+	vmdd->num_irqs = 0;
+
+	return mdev_register_device(&dfl_dev->dev, &dfl_mdev_ops);
+}
+
+static void vfio_mdev_dfl_remove(struct dfl_device *dfl_dev)
+{
+	mdev_unregister_device(&dfl_dev->dev);
+}
+
+static struct dfl_driver vfio_mdev_dfl_driver = {
+	.drv	= {
+		.name       = "vfio-mdev-dfl",
+	},
+	.probe   = vfio_mdev_dfl_probe,
+	.remove  = vfio_mdev_dfl_remove,
+};
+
+module_dfl_driver(vfio_mdev_dfl_driver);
+
+MODULE_DESCRIPTION("VFIO MDEV DFL driver");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/3] Documentation: fpga: dfl: Add description for VFIO Mdev support
  2020-09-08  7:13 [PATCH 0/3] add VFIO mdev support for DFL devices Xu Yilun
  2020-09-08  7:13 ` [PATCH 1/3] fpga: dfl: add driver_override support Xu Yilun
  2020-09-08  7:13 ` [PATCH 2/3] fpga: dfl: VFIO mdev support for DFL devices Xu Yilun
@ 2020-09-08  7:13 ` Xu Yilun
  2020-09-08 21:10   ` Alex Williamson
  2020-09-09 21:13 ` [PATCH 0/3] add VFIO mdev support for DFL devices Tom Rix
  3 siblings, 1 reply; 10+ messages in thread
From: Xu Yilun @ 2020-09-08  7:13 UTC (permalink / raw)
  To: mdf, alex.williamson, kwankhede, linux-fpga, kvm, linux-kernel
  Cc: trix, lgoncalv, yilun.xu, Matthew Gerlach

This patch adds description for VFIO Mdev support for dfl devices on
dfl bus.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 Documentation/fpga/dfl.rst | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 0404fe6..f077754 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -502,6 +502,26 @@ FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
 could be a reference.
 
 
+VFIO Mdev support for DFL devices
+=================================
+As we introduced a dfl bus for private features, they could be added to dfl bus
+as independent dfl devices. There is a requirement to handle these devices
+either by kernel drivers or by direct access from userspace. Usually we bind
+the kernel drivers to devices which provide board management functions, and
+gives user direct access to devices which cooperate closely with user
+controlled Accelerated Function Unit (AFU). We realize this with a VFIO Mdev
+implementation. When we bind the vfio-mdev-dfl driver to a dfl device, it
+realizes a group of callbacks and registers to the Mdev framework as a
+parent (physical) device. It could then create one (available_instances == 1)
+mdev device.
+Since dfl devices are sub devices of FPGA DFL physical devices (e.g. PCIE
+device), which provide no DMA isolation for each sub device, this may leads to
+DMA isolation problem if a private feature is designed to be capable of DMA.
+The AFU user could potentially access the whole device addressing space and
+impact the private feature. So now the general HW design rule is, no DMA
+capability for private features. It eliminates the DMA isolation problem.
+
+
 Open discussion
 ===============
 FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] Documentation: fpga: dfl: Add description for VFIO Mdev support
  2020-09-08  7:13 ` [PATCH 3/3] Documentation: fpga: dfl: Add description for VFIO Mdev support Xu Yilun
@ 2020-09-08 21:10   ` Alex Williamson
  2020-09-10  8:32     ` Xu Yilun
  0 siblings, 1 reply; 10+ messages in thread
From: Alex Williamson @ 2020-09-08 21:10 UTC (permalink / raw)
  To: Xu Yilun
  Cc: mdf, kwankhede, linux-fpga, kvm, linux-kernel, trix, lgoncalv,
	Matthew Gerlach, Raj, Ashok

On Tue,  8 Sep 2020 15:13:32 +0800
Xu Yilun <yilun.xu@intel.com> wrote:

> This patch adds description for VFIO Mdev support for dfl devices on
> dfl bus.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  Documentation/fpga/dfl.rst | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index 0404fe6..f077754 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -502,6 +502,26 @@ FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
>  could be a reference.
>  
>  
> +VFIO Mdev support for DFL devices
> +=================================
> +As we introduced a dfl bus for private features, they could be added to dfl bus
> +as independent dfl devices. There is a requirement to handle these devices
> +either by kernel drivers or by direct access from userspace. Usually we bind
> +the kernel drivers to devices which provide board management functions, and
> +gives user direct access to devices which cooperate closely with user
> +controlled Accelerated Function Unit (AFU). We realize this with a VFIO Mdev
> +implementation. When we bind the vfio-mdev-dfl driver to a dfl device, it
> +realizes a group of callbacks and registers to the Mdev framework as a
> +parent (physical) device. It could then create one (available_instances == 1)
> +mdev device.
> +Since dfl devices are sub devices of FPGA DFL physical devices (e.g. PCIE
> +device), which provide no DMA isolation for each sub device, this may leads to
> +DMA isolation problem if a private feature is designed to be capable of DMA.
> +The AFU user could potentially access the whole device addressing space and
> +impact the private feature. So now the general HW design rule is, no DMA
> +capability for private features. It eliminates the DMA isolation problem.

What's the advantage of entangling mdev/vfio in this approach versus
simply exposing the MMIO region of the device via sysfs (similar to a
resource file in pci-sysfs)?  This implementation doesn't support
interrupts, it doesn't support multiplexing of a device, it doesn't
perform any degree of mediation, it seems to simply say "please don't
do DMA".  I don't think that's acceptable for an mdev driver.  If you
want to play loose with isolation, do it somewhere else.  Thanks,

Alex


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/3] add VFIO mdev support for DFL devices
  2020-09-08  7:13 [PATCH 0/3] add VFIO mdev support for DFL devices Xu Yilun
                   ` (2 preceding siblings ...)
  2020-09-08  7:13 ` [PATCH 3/3] Documentation: fpga: dfl: Add description for VFIO Mdev support Xu Yilun
@ 2020-09-09 21:13 ` Tom Rix
  2020-09-10  9:08   ` Xu Yilun
  3 siblings, 1 reply; 10+ messages in thread
From: Tom Rix @ 2020-09-09 21:13 UTC (permalink / raw)
  To: Xu Yilun, mdf, alex.williamson, kwankhede, linux-fpga, kvm, linux-kernel
  Cc: lgoncalv

This is a new interface, the documentation needs to go

into greater detail. I am particularly interested in the user workflow.

This seems like it would work only for kernel modules. 

Please describe both in the documentation.

A sample of a user mode driver would be helpful.

Is putting driver_override using sysfs for each device scalable ? would a list sets of {feature id,files}'s the vfio driver respond to better ? 

To be consistent the mdev driver file name should be dfl-vfio-mdev.c

There should be an opt-in flag for drivers being overridden instead of blanket approval of all drivers.

Tom

On 9/8/20 12:13 AM, Xu Yilun wrote:
> These patches depend on the patchset: "Modularization of DFL private
> feature drivers" & "add dfl bus support to MODULE_DEVICE_TABLE()"
>
> https://lore.kernel.org/linux-fpga/1599488581-16386-1-git-send-email-yilun.xu@intel.com/
>
> This patchset provides an VFIO Mdev driver for dfl devices. It makes
> possible for dfl devices be direct accessed from userspace.
>
> Xu Yilun (3):
>   fpga: dfl: add driver_override support
>   fpga: dfl: VFIO mdev support for DFL devices
>   Documentation: fpga: dfl: Add description for VFIO Mdev support
>
>  Documentation/ABI/testing/sysfs-bus-dfl |  20 ++
>  Documentation/fpga/dfl.rst              |  20 ++
>  drivers/fpga/Kconfig                    |   9 +
>  drivers/fpga/Makefile                   |   1 +
>  drivers/fpga/dfl.c                      |  54 ++++-
>  drivers/fpga/vfio-mdev-dfl.c            | 391 ++++++++++++++++++++++++++++++++
>  include/linux/fpga/dfl-bus.h            |   2 +
>  7 files changed, 496 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/fpga/vfio-mdev-dfl.c
>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] Documentation: fpga: dfl: Add description for VFIO Mdev support
  2020-09-08 21:10   ` Alex Williamson
@ 2020-09-10  8:32     ` Xu Yilun
  2020-09-10 15:49       ` Alex Williamson
  0 siblings, 1 reply; 10+ messages in thread
From: Xu Yilun @ 2020-09-10  8:32 UTC (permalink / raw)
  To: Alex Williamson
  Cc: mdf, kwankhede, linux-fpga, kvm, linux-kernel, trix, lgoncalv,
	Matthew Gerlach, Raj, Ashok, yilun.xu

Hi Alex:

Thanks for your quick response and is helpful to me. I did some more
investigation and some comments inline.

On Tue, Sep 08, 2020 at 03:10:02PM -0600, Alex Williamson wrote:
> On Tue,  8 Sep 2020 15:13:32 +0800
> Xu Yilun <yilun.xu@intel.com> wrote:
> 
> > This patch adds description for VFIO Mdev support for dfl devices on
> > dfl bus.
> > 
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > ---
> >  Documentation/fpga/dfl.rst | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> > index 0404fe6..f077754 100644
> > --- a/Documentation/fpga/dfl.rst
> > +++ b/Documentation/fpga/dfl.rst
> > @@ -502,6 +502,26 @@ FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
> >  could be a reference.
> >  
> >  
> > +VFIO Mdev support for DFL devices
> > +=================================
> > +As we introduced a dfl bus for private features, they could be added to dfl bus
> > +as independent dfl devices. There is a requirement to handle these devices
> > +either by kernel drivers or by direct access from userspace. Usually we bind
> > +the kernel drivers to devices which provide board management functions, and
> > +gives user direct access to devices which cooperate closely with user
> > +controlled Accelerated Function Unit (AFU). We realize this with a VFIO Mdev
> > +implementation. When we bind the vfio-mdev-dfl driver to a dfl device, it
> > +realizes a group of callbacks and registers to the Mdev framework as a
> > +parent (physical) device. It could then create one (available_instances == 1)
> > +mdev device.
> > +Since dfl devices are sub devices of FPGA DFL physical devices (e.g. PCIE
> > +device), which provide no DMA isolation for each sub device, this may leads to
> > +DMA isolation problem if a private feature is designed to be capable of DMA.
> > +The AFU user could potentially access the whole device addressing space and
> > +impact the private feature. So now the general HW design rule is, no DMA
> > +capability for private features. It eliminates the DMA isolation problem.
> 
> What's the advantage of entangling mdev/vfio in this approach versus
> simply exposing the MMIO region of the device via sysfs (similar to a
> resource file in pci-sysfs)?  This implementation doesn't support
> interrupts, it doesn't support multiplexing of a device, it doesn't
> perform any degree of mediation, it seems to simply say "please don't
> do DMA".  I don't think that's acceptable for an mdev driver.  If you
> want to play loose with isolation, do it somewhere else.  Thanks,

The intention of the patchset is to enable the userspace drivers for dfl
devices. The dfl devices are actually different IP blocks integrated in
FPGA to support different board functionalities. They are sub devices of
the FPGA PCIe device. Their mmio blocks are in PCIE bar regions. And we
want some of the dfl devices handled by the userspace drivers.

Some dfl devices are capable of interrupt. I didn't add interrupt code
in this patch cause now the IRQ capable dfl devices are all handled by
kernel drivers. But as a generic FPGA platform, IRQ handling for userspace
drivers should be supported.

And I can see there are several ways to enable the userspace driver.

1. Some specific sysfs like pci do. But seems it is not the common way for
userspace driver. It does't support interrupt. And potentially users
operate on the same mmio region together with kernel driver at the same
time.

2. VFIO driver with NOIOMMU enabled. I think it meets our needs. Do you
think it is good we implement an VFIO driver for dfl devices?

3. VFIO mdev. I implemented it because it will not block us from lacking
of valid iommu group. And since the driver didn't perform any mediation,
I should give up.

4. UIO driver. It should work. I'm wondering if option 2 covers the
functionalities of UIO and has more enhancement. So option 2 may be
better?

Thanks again for your time, and I really appreciate you would give some
guide on it.

Yilun.

> 
> Alex

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/3] add VFIO mdev support for DFL devices
  2020-09-09 21:13 ` [PATCH 0/3] add VFIO mdev support for DFL devices Tom Rix
@ 2020-09-10  9:08   ` Xu Yilun
  0 siblings, 0 replies; 10+ messages in thread
From: Xu Yilun @ 2020-09-10  9:08 UTC (permalink / raw)
  To: Tom Rix
  Cc: mdf, alex.williamson, kwankhede, linux-fpga, kvm, linux-kernel, lgoncalv

Hi tom:

The generic workflow of mdev is described in
Documentation/driver-api/vfio-mediated-device.rst, I could also share
some code offline.

But now I got the comments from Alex that it may not suitable we
implement the mdev for dfl devices. I think we need to first of all
reconsider the design.

Thanks,
Yilun 

On Wed, Sep 09, 2020 at 02:13:59PM -0700, Tom Rix wrote:
> This is a new interface, the documentation needs to go
> 
> into greater detail. I am particularly interested in the user workflow.
> 
> This seems like it would work only for kernel modules. 
> 
> Please describe both in the documentation.
> 
> A sample of a user mode driver would be helpful.
> 
> Is putting driver_override using sysfs for each device scalable ? would a list sets of {feature id,files}'s the vfio driver respond to better ? 
> 
> To be consistent the mdev driver file name should be dfl-vfio-mdev.c
> 
> There should be an opt-in flag for drivers being overridden instead of blanket approval of all drivers.
> 
> Tom
> 
> On 9/8/20 12:13 AM, Xu Yilun wrote:
> > These patches depend on the patchset: "Modularization of DFL private
> > feature drivers" & "add dfl bus support to MODULE_DEVICE_TABLE()"
> >
> > https://lore.kernel.org/linux-fpga/1599488581-16386-1-git-send-email-yilun.xu@intel.com/
> >
> > This patchset provides an VFIO Mdev driver for dfl devices. It makes
> > possible for dfl devices be direct accessed from userspace.
> >
> > Xu Yilun (3):
> >   fpga: dfl: add driver_override support
> >   fpga: dfl: VFIO mdev support for DFL devices
> >   Documentation: fpga: dfl: Add description for VFIO Mdev support
> >
> >  Documentation/ABI/testing/sysfs-bus-dfl |  20 ++
> >  Documentation/fpga/dfl.rst              |  20 ++
> >  drivers/fpga/Kconfig                    |   9 +
> >  drivers/fpga/Makefile                   |   1 +
> >  drivers/fpga/dfl.c                      |  54 ++++-
> >  drivers/fpga/vfio-mdev-dfl.c            | 391 ++++++++++++++++++++++++++++++++
> >  include/linux/fpga/dfl-bus.h            |   2 +
> >  7 files changed, 496 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/fpga/vfio-mdev-dfl.c
> >

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] Documentation: fpga: dfl: Add description for VFIO Mdev support
  2020-09-10  8:32     ` Xu Yilun
@ 2020-09-10 15:49       ` Alex Williamson
  2020-09-11  6:32         ` Xu Yilun
  0 siblings, 1 reply; 10+ messages in thread
From: Alex Williamson @ 2020-09-10 15:49 UTC (permalink / raw)
  To: Xu Yilun
  Cc: mdf, kwankhede, linux-fpga, kvm, linux-kernel, trix, lgoncalv,
	Matthew Gerlach, Raj, Ashok

On Thu, 10 Sep 2020 16:32:30 +0800
Xu Yilun <yilun.xu@intel.com> wrote:

> Hi Alex:
> 
> Thanks for your quick response and is helpful to me. I did some more
> investigation and some comments inline.
> 
> On Tue, Sep 08, 2020 at 03:10:02PM -0600, Alex Williamson wrote:
> > On Tue,  8 Sep 2020 15:13:32 +0800
> > Xu Yilun <yilun.xu@intel.com> wrote:
> >   
> > > This patch adds description for VFIO Mdev support for dfl devices on
> > > dfl bus.
> > > 
> > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > ---
> > >  Documentation/fpga/dfl.rst | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> > > index 0404fe6..f077754 100644
> > > --- a/Documentation/fpga/dfl.rst
> > > +++ b/Documentation/fpga/dfl.rst
> > > @@ -502,6 +502,26 @@ FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
> > >  could be a reference.
> > >  
> > >  
> > > +VFIO Mdev support for DFL devices
> > > +=================================
> > > +As we introduced a dfl bus for private features, they could be added to dfl bus
> > > +as independent dfl devices. There is a requirement to handle these devices
> > > +either by kernel drivers or by direct access from userspace. Usually we bind
> > > +the kernel drivers to devices which provide board management functions, and
> > > +gives user direct access to devices which cooperate closely with user
> > > +controlled Accelerated Function Unit (AFU). We realize this with a VFIO Mdev
> > > +implementation. When we bind the vfio-mdev-dfl driver to a dfl device, it
> > > +realizes a group of callbacks and registers to the Mdev framework as a
> > > +parent (physical) device. It could then create one (available_instances == 1)
> > > +mdev device.
> > > +Since dfl devices are sub devices of FPGA DFL physical devices (e.g. PCIE
> > > +device), which provide no DMA isolation for each sub device, this may leads to
> > > +DMA isolation problem if a private feature is designed to be capable of DMA.
> > > +The AFU user could potentially access the whole device addressing space and
> > > +impact the private feature. So now the general HW design rule is, no DMA
> > > +capability for private features. It eliminates the DMA isolation problem.  
> > 
> > What's the advantage of entangling mdev/vfio in this approach versus
> > simply exposing the MMIO region of the device via sysfs (similar to a
> > resource file in pci-sysfs)?  This implementation doesn't support
> > interrupts, it doesn't support multiplexing of a device, it doesn't
> > perform any degree of mediation, it seems to simply say "please don't
> > do DMA".  I don't think that's acceptable for an mdev driver.  If you
> > want to play loose with isolation, do it somewhere else.  Thanks,  
> 
> The intention of the patchset is to enable the userspace drivers for dfl
> devices. The dfl devices are actually different IP blocks integrated in
> FPGA to support different board functionalities. They are sub devices of
> the FPGA PCIe device. Their mmio blocks are in PCIE bar regions. And we
> want some of the dfl devices handled by the userspace drivers.
> 
> Some dfl devices are capable of interrupt. I didn't add interrupt code
> in this patch cause now the IRQ capable dfl devices are all handled by
> kernel drivers. But as a generic FPGA platform, IRQ handling for userspace
> drivers should be supported.
> 
> And I can see there are several ways to enable the userspace driver.
> 
> 1. Some specific sysfs like pci do. But seems it is not the common way for
> userspace driver. It does't support interrupt. And potentially users
> operate on the same mmio region together with kernel driver at the same
> time.
> 
> 2. VFIO driver with NOIOMMU enabled. I think it meets our needs. Do you
> think it is good we implement an VFIO driver for dfl devices?
> 
> 3. VFIO mdev. I implemented it because it will not block us from lacking
> of valid iommu group. And since the driver didn't perform any mediation,
> I should give up.
> 
> 4. UIO driver. It should work. I'm wondering if option 2 covers the
> functionalities of UIO and has more enhancement. So option 2 may be
> better?
> 
> Thanks again for your time, and I really appreciate you would give some
> guide on it.


VFIO no-iommu was intended as a transition helper for platforms that do
not support an IOMMU, particularly running within a VM where we use
regular, IOMMU protected devices within the host, but allow no-iommu
within the guest such that the host is still protected from the guest
sandbox.  There should be no new use cases of no-iommu, it's unsafe, it
taints the kernel where it's used (guest in the above intended use
case).  If you intend long term distribution support of a solution,
VFIO no-iommu should not be considered an option.

VFIO mdev requires that the mdev vendor driver mediates access to the
device in order to provide isolation.  In the initial vGPU use cases,
we expect that isolation to be provided via devices specific means, ex.
GPU GART, but we've since included system level components like IOMMU
backing devices and auxiliary domains, the latter to make use of IOMMU
PASID support.

As implemented in this proposal, mdev is being used only to subvert the
IOMMU grouping requirements of VFIO in order to order to expose a
device that is potentially fully capable of DMA to userspace with no
isolation whatsoever.  If not for the IOMMU grouping, this driver could
simply be a VFIO bus driver making use of the vfio-platform interface.
Either way, without isolation, this does not belong in the realm of
VFIO.

Given your architecture, the only potentially valid mdev use case I can
see would be if the mdev vendor driver binds to the PCIe device,
allowing multiplexing of the parent device by carving out fpga
functional blocks from MMIO BAR space, and providing isolation by
enforcing that the parent device never enables bus master, assuming
that would prevent any of the fpga sub-components from performing DMA.

Are there worthwhile use cases of these fpga devices without DMA?

If you need DMA (or the device is potentially capable of DMA and
cannot be audited to prevent it) and cannot provide isolation then
please don't use VFIO or mdev, doing so would violate the notion of
secure userspace device access that we've worked to achieve in this
ecosystem.

If you choose another route, pci-sysfs already provides full BAR access
via the resource files in sysfs, but you could also expose individual
sysfs files with the same capabilities per fpga functional unit to
resolve the conflict between kernel and userspace "ownership".  UIO
might also be a solution.  This proposal to restrict userspace usage to
devices that don't perform DMA is akin to uio_pci_generic, where the
user is not expected to enable bus master, but nothing prevents them
from doing so and as a result it's a gateway for all sorts of
unsupportable drivers.  mdev should not be used to follow that example.
Thanks,

Alex


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] Documentation: fpga: dfl: Add description for VFIO Mdev support
  2020-09-10 15:49       ` Alex Williamson
@ 2020-09-11  6:32         ` Xu Yilun
  0 siblings, 0 replies; 10+ messages in thread
From: Xu Yilun @ 2020-09-11  6:32 UTC (permalink / raw)
  To: Alex Williamson
  Cc: mdf, kwankhede, linux-fpga, kvm, linux-kernel, trix, lgoncalv,
	Matthew Gerlach, Raj, Ashok, yilun.xu

On Thu, Sep 10, 2020 at 09:49:03AM -0600, Alex Williamson wrote:
> On Thu, 10 Sep 2020 16:32:30 +0800
> Xu Yilun <yilun.xu@intel.com> wrote:
> 
> > Hi Alex:
> > 
> > Thanks for your quick response and is helpful to me. I did some more
> > investigation and some comments inline.
> > 
> > On Tue, Sep 08, 2020 at 03:10:02PM -0600, Alex Williamson wrote:
> > > On Tue,  8 Sep 2020 15:13:32 +0800
> > > Xu Yilun <yilun.xu@intel.com> wrote:
> > >   
> > > > This patch adds description for VFIO Mdev support for dfl devices on
> > > > dfl bus.
> > > > 
> > > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > ---
> > > >  Documentation/fpga/dfl.rst | 20 ++++++++++++++++++++
> > > >  1 file changed, 20 insertions(+)
> > > > 
> > > > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> > > > index 0404fe6..f077754 100644
> > > > --- a/Documentation/fpga/dfl.rst
> > > > +++ b/Documentation/fpga/dfl.rst
> > > > @@ -502,6 +502,26 @@ FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
> > > >  could be a reference.
> > > >  
> > > >  
> > > > +VFIO Mdev support for DFL devices
> > > > +=================================
> > > > +As we introduced a dfl bus for private features, they could be added to dfl bus
> > > > +as independent dfl devices. There is a requirement to handle these devices
> > > > +either by kernel drivers or by direct access from userspace. Usually we bind
> > > > +the kernel drivers to devices which provide board management functions, and
> > > > +gives user direct access to devices which cooperate closely with user
> > > > +controlled Accelerated Function Unit (AFU). We realize this with a VFIO Mdev
> > > > +implementation. When we bind the vfio-mdev-dfl driver to a dfl device, it
> > > > +realizes a group of callbacks and registers to the Mdev framework as a
> > > > +parent (physical) device. It could then create one (available_instances == 1)
> > > > +mdev device.
> > > > +Since dfl devices are sub devices of FPGA DFL physical devices (e.g. PCIE
> > > > +device), which provide no DMA isolation for each sub device, this may leads to
> > > > +DMA isolation problem if a private feature is designed to be capable of DMA.
> > > > +The AFU user could potentially access the whole device addressing space and
> > > > +impact the private feature. So now the general HW design rule is, no DMA
> > > > +capability for private features. It eliminates the DMA isolation problem.  
> > > 
> > > What's the advantage of entangling mdev/vfio in this approach versus
> > > simply exposing the MMIO region of the device via sysfs (similar to a
> > > resource file in pci-sysfs)?  This implementation doesn't support
> > > interrupts, it doesn't support multiplexing of a device, it doesn't
> > > perform any degree of mediation, it seems to simply say "please don't
> > > do DMA".  I don't think that's acceptable for an mdev driver.  If you
> > > want to play loose with isolation, do it somewhere else.  Thanks,  
> > 
> > The intention of the patchset is to enable the userspace drivers for dfl
> > devices. The dfl devices are actually different IP blocks integrated in
> > FPGA to support different board functionalities. They are sub devices of
> > the FPGA PCIe device. Their mmio blocks are in PCIE bar regions. And we
> > want some of the dfl devices handled by the userspace drivers.
> > 
> > Some dfl devices are capable of interrupt. I didn't add interrupt code
> > in this patch cause now the IRQ capable dfl devices are all handled by
> > kernel drivers. But as a generic FPGA platform, IRQ handling for userspace
> > drivers should be supported.
> > 
> > And I can see there are several ways to enable the userspace driver.
> > 
> > 1. Some specific sysfs like pci do. But seems it is not the common way for
> > userspace driver. It does't support interrupt. And potentially users
> > operate on the same mmio region together with kernel driver at the same
> > time.
> > 
> > 2. VFIO driver with NOIOMMU enabled. I think it meets our needs. Do you
> > think it is good we implement an VFIO driver for dfl devices?
> > 
> > 3. VFIO mdev. I implemented it because it will not block us from lacking
> > of valid iommu group. And since the driver didn't perform any mediation,
> > I should give up.
> > 
> > 4. UIO driver. It should work. I'm wondering if option 2 covers the
> > functionalities of UIO and has more enhancement. So option 2 may be
> > better?
> > 
> > Thanks again for your time, and I really appreciate you would give some
> > guide on it.
> 
> 
> VFIO no-iommu was intended as a transition helper for platforms that do
> not support an IOMMU, particularly running within a VM where we use
> regular, IOMMU protected devices within the host, but allow no-iommu
> within the guest such that the host is still protected from the guest
> sandbox.  There should be no new use cases of no-iommu, it's unsafe, it
> taints the kernel where it's used (guest in the above intended use
> case).  If you intend long term distribution support of a solution,
> VFIO no-iommu should not be considered an option.
> 
> VFIO mdev requires that the mdev vendor driver mediates access to the
> device in order to provide isolation.  In the initial vGPU use cases,
> we expect that isolation to be provided via devices specific means, ex.
> GPU GART, but we've since included system level components like IOMMU
> backing devices and auxiliary domains, the latter to make use of IOMMU
> PASID support.
> 
> As implemented in this proposal, mdev is being used only to subvert the
> IOMMU grouping requirements of VFIO in order to order to expose a
> device that is potentially fully capable of DMA to userspace with no
> isolation whatsoever.  If not for the IOMMU grouping, this driver could
> simply be a VFIO bus driver making use of the vfio-platform interface.
> Either way, without isolation, this does not belong in the realm of
> VFIO.
> 
> Given your architecture, the only potentially valid mdev use case I can
> see would be if the mdev vendor driver binds to the PCIe device,
> allowing multiplexing of the parent device by carving out fpga
> functional blocks from MMIO BAR space, and providing isolation by
> enforcing that the parent device never enables bus master, assuming
> that would prevent any of the fpga sub-components from performing DMA.
> 
> Are there worthwhile use cases of these fpga devices without DMA?

The board (Intel PAC N3000) we want to support is a Smart NIC. The FPGA
part is responsible for the various MAC layer offloading. The software
for FPGA usually doesn't touch the network data stream (they are all
handled by FPGA logic), it does some configurations and link status
reading so no DMA is required. These configurations are sometimes very
specific to dynamic RTL logic developed by user, so this is the purpose
we handle them in userspace.

There are some other usercases, which use FPGA to do some dedicated
algorithm like for deep learning. They need to perform memory in and
memory out. For these cases, it seems possible we carving out the
related part as the mdev, leaving management part in parent device
driver.

> 
> If you need DMA (or the device is potentially capable of DMA and
> cannot be audited to prevent it) and cannot provide isolation then
> please don't use VFIO or mdev, doing so would violate the notion of
> secure userspace device access that we've worked to achieve in this
> ecosystem.
> 
> If you choose another route, pci-sysfs already provides full BAR access
> via the resource files in sysfs, but you could also expose individual
> sysfs files with the same capabilities per fpga functional unit to
> resolve the conflict between kernel and userspace "ownership".  UIO
> might also be a solution.  This proposal to restrict userspace usage to
> devices that don't perform DMA is akin to uio_pci_generic, where the
> user is not expected to enable bus master, but nothing prevents them

We are going to exposes these devices not capable of DMA, so seems UIO
is the right way to go.

> from doing so and as a result it's a gateway for all sorts of
> unsupportable drivers.  mdev should not be used to follow that example.



I should thank you again for the detail explanation.

Yilun


> Thanks,
> 
> Alex

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-09-11  6:36 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-08  7:13 [PATCH 0/3] add VFIO mdev support for DFL devices Xu Yilun
2020-09-08  7:13 ` [PATCH 1/3] fpga: dfl: add driver_override support Xu Yilun
2020-09-08  7:13 ` [PATCH 2/3] fpga: dfl: VFIO mdev support for DFL devices Xu Yilun
2020-09-08  7:13 ` [PATCH 3/3] Documentation: fpga: dfl: Add description for VFIO Mdev support Xu Yilun
2020-09-08 21:10   ` Alex Williamson
2020-09-10  8:32     ` Xu Yilun
2020-09-10 15:49       ` Alex Williamson
2020-09-11  6:32         ` Xu Yilun
2020-09-09 21:13 ` [PATCH 0/3] add VFIO mdev support for DFL devices Tom Rix
2020-09-10  9:08   ` Xu Yilun

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).