[RFC v2 0/2] vfio/pci: wrap pci device as a mediated device

[RFC v2 0/2] vfio/pci: wrap pci device as a mediated device

[Liu, Yi L]

This patchset aims to add a vfio-pci-like meta driver as a demo
user of the vfio changes introduced in "vfio/mdev: IOMMU aware
mediated device" patchset from Baolu Lu.

Previous RFC v1 has given two proposals and the discussion could
be found in following link. Per the comments, this patchset adds
a separate driver named vfio-pci-mdev. It leverages some symbols
from existing vfio-pci. When device is bound to vfio-pci-mdev
driver, it would be wrapped as a mediated device. And user should
create a mdev on this device via the mdev interface exposed to
user.
https://lkml.org/lkml/2019/3/4/529

The new vfio-pci-mdev driver could be used to verify the vfio
changes in "vfio/mdev: IOMMU aware mediated device". Besides,
per Alex's comments, it could also be a good base driver for
experimenting with device specific mdev migration.

Specific interface tested in this proposal:

*) int mdev_set_iommu_device(struct device *dev,
				struct device *iommu_device)
   introduced in the patch as below:
   "[PATCH v5 6/8] vfio/mdev: Add iommu related member in mdev_device"


Links:
*) Link of "vfio/mdev: IOMMU aware mediated device"
	https://lwn.net/Articles/780522/

Please feel free give your comments.

Thanks,
Yi Liu

Change log:
  v1->v2:
  - instead of adding kernel option to existing vfio-pci
    module in v1, v2 follows Alex's suggestion to add a
    separate vfio-pci-mdev module.
  - new patchset subject: "vfio/pci: wrap pci device as a mediated device"

Liu, Yi L (2):
  vfio/pci: export common symbols in vfio-pci
  vfio/pci: add vfio-pci-mdev driver

 drivers/vfio/pci/Kconfig            |  11 ++
 drivers/vfio/pci/Makefile           |   5 +
 drivers/vfio/pci/vfio_pci.c         | 101 ++++++-----
 drivers/vfio/pci/vfio_pci_mdev.c    | 334 ++++++++++++++++++++++++++++++++++++
 drivers/vfio/pci/vfio_pci_private.h |  17 ++
 5 files changed, 428 insertions(+), 40 deletions(-)
 create mode 100644 drivers/vfio/pci/vfio_pci_mdev.c

-- 
2.7.4

[RFC v2 1/2] vfio/pci: export common symbols in vfio-pci

[Liu, Yi L]

This patch exports the following symbols from vfio-pci driver
for vfio-pci alike driver. e.g. vfio-pci-mdev driver

*) vfio_pci_set_vga_decode();
*) vfio_pci_release();
*) vfio_pci_open();
*) vfio_pci_register_dev_region();
*) vfio_pci_ioctl();
*) vfio_pci_rw();
*) vfio_pci_mmap();
*) vfio_pci_request();
*) vfio_pci_probe_misc();
*) vfio_pci_remove_misc();
*) vfio_err_handlers;
*) vfio_pci_reflck_attach();
*) vfio_pci_reflck_put();

Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Liu, Yi L <yi.l.liu@intel.com>
---
 drivers/vfio/pci/vfio_pci.c         | 101 ++++++++++++++++++++++--------------
 drivers/vfio/pci/vfio_pci_private.h |  17 ++++++
 2 files changed, 78 insertions(+), 40 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index ff60bd1..e36b922 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -73,7 +73,7 @@ static inline bool vfio_vga_disabled(void)
  * has no way to get to it and routing can be disabled externally at the
  * bridge.
  */
-static unsigned int vfio_pci_set_vga_decode(void *opaque, bool single_vga)
+unsigned int vfio_pci_set_vga_decode(void *opaque, bool single_vga)
 {
 	struct vfio_pci_device *vdev = opaque;
 	struct pci_dev *tmp = NULL, *pdev = vdev->pdev;
@@ -103,6 +103,7 @@ static unsigned int vfio_pci_set_vga_decode(void *opaque, bool single_vga)
 
 	return decodes;
 }
+EXPORT_SYMBOL_GPL(vfio_pci_set_vga_decode);
 
 static inline bool vfio_pci_is_vga(struct pci_dev *pdev)
 {
@@ -410,7 +411,7 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
 		pci_set_power_state(pdev, PCI_D3hot);
 }
 
-static void vfio_pci_release(void *device_data)
+void vfio_pci_release(void *device_data)
 {
 	struct vfio_pci_device *vdev = device_data;
 
@@ -425,8 +426,9 @@ static void vfio_pci_release(void *device_data)
 
 	module_put(THIS_MODULE);
 }
+EXPORT_SYMBOL_GPL(vfio_pci_release);
 
-static int vfio_pci_open(void *device_data)
+int vfio_pci_open(void *device_data)
 {
 	struct vfio_pci_device *vdev = device_data;
 	int ret = 0;
@@ -450,6 +452,7 @@ static int vfio_pci_open(void *device_data)
 		module_put(THIS_MODULE);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(vfio_pci_open);
 
 static int vfio_pci_get_irq_count(struct vfio_pci_device *vdev, int irq_type)
 {
@@ -634,9 +637,10 @@ int vfio_pci_register_dev_region(struct vfio_pci_device *vdev,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(vfio_pci_register_dev_region);
 
-static long vfio_pci_ioctl(void *device_data,
-			   unsigned int cmd, unsigned long arg)
+long vfio_pci_ioctl(void *device_data,
+		   unsigned int cmd, unsigned long arg)
 {
 	struct vfio_pci_device *vdev = device_data;
 	unsigned long minsz;
@@ -1079,8 +1083,9 @@ static long vfio_pci_ioctl(void *device_data,
 
 	return -ENOTTY;
 }
+EXPORT_SYMBOL_GPL(vfio_pci_ioctl);
 
-static ssize_t vfio_pci_rw(void *device_data, char __user *buf,
+ssize_t vfio_pci_rw(void *device_data, char __user *buf,
 			   size_t count, loff_t *ppos, bool iswrite)
 {
 	unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
@@ -1111,6 +1116,7 @@ static ssize_t vfio_pci_rw(void *device_data, char __user *buf,
 
 	return -EINVAL;
 }
+EXPORT_SYMBOL_GPL(vfio_pci_rw);
 
 static ssize_t vfio_pci_read(void *device_data, char __user *buf,
 			     size_t count, loff_t *ppos)
@@ -1130,7 +1136,7 @@ static ssize_t vfio_pci_write(void *device_data, const char __user *buf,
 	return vfio_pci_rw(device_data, (char __user *)buf, count, ppos, true);
 }
 
-static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
+int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
 {
 	struct vfio_pci_device *vdev = device_data;
 	struct pci_dev *pdev = vdev->pdev;
@@ -1173,7 +1179,7 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
 	 */
 	if (!vdev->barmap[index]) {
 		ret = pci_request_selected_regions(pdev,
-						   1 << index, "vfio-pci");
+						   1 << index, vdev->name);
 		if (ret)
 			return ret;
 
@@ -1191,8 +1197,9 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
 	return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
 			       req_len, vma->vm_page_prot);
 }
+EXPORT_SYMBOL_GPL(vfio_pci_mmap);
 
-static void vfio_pci_request(void *device_data, unsigned int count)
+void vfio_pci_request(void *device_data, unsigned int count)
 {
 	struct vfio_pci_device *vdev = device_data;
 
@@ -1211,6 +1218,7 @@ static void vfio_pci_request(void *device_data, unsigned int count)
 
 	mutex_unlock(&vdev->igate);
 }
+EXPORT_SYMBOL_GPL(vfio_pci_request);
 
 static const struct vfio_device_ops vfio_pci_ops = {
 	.name		= "vfio-pci",
@@ -1223,8 +1231,31 @@ static const struct vfio_device_ops vfio_pci_ops = {
 	.request	= vfio_pci_request,
 };
 
-static int vfio_pci_reflck_attach(struct vfio_pci_device *vdev);
-static void vfio_pci_reflck_put(struct vfio_pci_reflck *reflck);
+int vfio_pci_probe_misc(struct pci_dev *pdev, struct vfio_pci_device *vdev)
+{
+	if (vfio_pci_is_vga(pdev)) {
+		vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
+		vga_set_legacy_decoding(pdev,
+					vfio_pci_set_vga_decode(vdev, false));
+	}
+
+	if (!disable_idle_d3) {
+		/*
+		 * pci-core sets the device power state to an unknown value at
+		 * bootup and after being removed from a driver.  The only
+		 * transition it allows from this unknown state is to D0, which
+		 * typically happens when a driver calls pci_enable_device().
+		 * We're not ready to enable the device yet, but we do want to
+		 * be able to get to D3.  Therefore first do a D0 transition
+		 * before going to D3.
+		 */
+		pci_set_power_state(pdev, PCI_D0);
+		pci_set_power_state(pdev, PCI_D3hot);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vfio_pci_probe_misc);
 
 static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
@@ -1259,6 +1290,7 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	}
 
 	vdev->pdev = pdev;
+	vdev->name = "vfio-pci";
 	vdev->irq_type = VFIO_PCI_NUM_IRQS;
 	mutex_init(&vdev->igate);
 	spin_lock_init(&vdev->irqlock);
@@ -1280,28 +1312,23 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return ret;
 	}
 
+	ret = vfio_pci_probe_misc(pdev, vdev);
+	return ret;
+}
+
+void vfio_pci_remove_misc(struct pci_dev *pdev)
+{
 	if (vfio_pci_is_vga(pdev)) {
-		vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
+		vga_client_register(pdev, NULL, NULL, NULL);
 		vga_set_legacy_decoding(pdev,
-					vfio_pci_set_vga_decode(vdev, false));
+				VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM |
+				VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM);
 	}
 
-	if (!disable_idle_d3) {
-		/*
-		 * pci-core sets the device power state to an unknown value at
-		 * bootup and after being removed from a driver.  The only
-		 * transition it allows from this unknown state is to D0, which
-		 * typically happens when a driver calls pci_enable_device().
-		 * We're not ready to enable the device yet, but we do want to
-		 * be able to get to D3.  Therefore first do a D0 transition
-		 * before going to D3.
-		 */
+	if (!disable_idle_d3)
 		pci_set_power_state(pdev, PCI_D0);
-		pci_set_power_state(pdev, PCI_D3hot);
-	}
-
-	return ret;
 }
+EXPORT_SYMBOL_GPL(vfio_pci_remove_misc);
 
 static void vfio_pci_remove(struct pci_dev *pdev)
 {
@@ -1317,16 +1344,7 @@ static void vfio_pci_remove(struct pci_dev *pdev)
 	kfree(vdev->region);
 	mutex_destroy(&vdev->ioeventfds_lock);
 	kfree(vdev);
-
-	if (vfio_pci_is_vga(pdev)) {
-		vga_client_register(pdev, NULL, NULL, NULL);
-		vga_set_legacy_decoding(pdev,
-				VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM |
-				VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM);
-	}
-
-	if (!disable_idle_d3)
-		pci_set_power_state(pdev, PCI_D0);
+	vfio_pci_remove_misc(pdev);
 }
 
 static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
@@ -1357,9 +1375,10 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
 	return PCI_ERS_RESULT_CAN_RECOVER;
 }
 
-static const struct pci_error_handlers vfio_err_handlers = {
+const struct pci_error_handlers vfio_err_handlers = {
 	.error_detected = vfio_pci_aer_err_detected,
 };
+EXPORT_SYMBOL_GPL(vfio_err_handlers);
 
 static struct pci_driver vfio_pci_driver = {
 	.name		= "vfio-pci",
@@ -1418,7 +1437,7 @@ static int vfio_pci_reflck_find(struct pci_dev *pdev, void *data)
 	return 0;
 }
 
-static int vfio_pci_reflck_attach(struct vfio_pci_device *vdev)
+int vfio_pci_reflck_attach(struct vfio_pci_device *vdev)
 {
 	bool slot = !pci_probe_reset_slot(vdev->pdev->slot);
 
@@ -1433,6 +1452,7 @@ static int vfio_pci_reflck_attach(struct vfio_pci_device *vdev)
 
 	return PTR_ERR_OR_ZERO(vdev->reflck);
 }
+EXPORT_SYMBOL_GPL(vfio_pci_reflck_attach);
 
 static void vfio_pci_reflck_release(struct kref *kref)
 {
@@ -1444,10 +1464,11 @@ static void vfio_pci_reflck_release(struct kref *kref)
 	mutex_unlock(&reflck_lock);
 }
 
-static void vfio_pci_reflck_put(struct vfio_pci_reflck *reflck)
+void vfio_pci_reflck_put(struct vfio_pci_reflck *reflck)
 {
 	kref_put_mutex(&reflck->kref, vfio_pci_reflck_release, &reflck_lock);
 }
+EXPORT_SYMBOL_GPL(vfio_pci_reflck_put);
 
 struct vfio_devices {
 	struct vfio_device **devices;
diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
index 8c0009f..da9afe5 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -90,6 +90,7 @@ struct vfio_pci_reflck {
 struct vfio_pci_device {
 	struct pci_dev		*pdev;
 	void __iomem		*barmap[PCI_STD_RESOURCE_END + 1];
+	char			*name;
 	bool			bar_mmap_supported[PCI_STD_RESOURCE_END + 1];
 	u8			*pci_config_map;
 	u8			*vconfig;
@@ -125,6 +126,8 @@ struct vfio_pci_device {
 	struct list_head	ioeventfds_list;
 };
 
+extern const struct pci_error_handlers vfio_err_handlers;
+
 #define is_intx(vdev) (vdev->irq_type == VFIO_PCI_INTX_IRQ_INDEX)
 #define is_msi(vdev) (vdev->irq_type == VFIO_PCI_MSI_IRQ_INDEX)
 #define is_msix(vdev) (vdev->irq_type == VFIO_PCI_MSIX_IRQ_INDEX)
@@ -154,6 +157,20 @@ extern long vfio_pci_ioeventfd(struct vfio_pci_device *vdev, loff_t offset,
 extern int vfio_pci_init_perm_bits(void);
 extern void vfio_pci_uninit_perm_bits(void);
 
+int vfio_pci_open(void *device_data);
+void vfio_pci_release(void *device_data);
+long vfio_pci_ioctl(void *device_data,
+			   unsigned int cmd, unsigned long arg);
+ssize_t vfio_pci_rw(void *device_data, char __user *buf,
+			   size_t count, loff_t *ppos, bool iswrite);
+int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma);
+void vfio_pci_request(void *device_data, unsigned int count);
+int vfio_pci_reflck_attach(struct vfio_pci_device *vdev);
+void vfio_pci_reflck_put(struct vfio_pci_reflck *reflck);
+unsigned int vfio_pci_set_vga_decode(void *opaque, bool single_vga);
+int vfio_pci_probe_misc(struct pci_dev *pdev, struct vfio_pci_device *vdev);
+void vfio_pci_remove_misc(struct pci_dev *pdev);
+
 extern int vfio_config_init(struct vfio_pci_device *vdev);
 extern void vfio_config_free(struct vfio_pci_device *vdev);
 
-- 
2.7.4

[RFC v2 2/2] vfio/pci: add vfio-pci-mdev driver

[Liu, Yi L]

This patch adds new driver named vfio-pci-mdev. It is similar
with vfio-pci driver. Thus consumes some symbols from vfio-pci
driver. However, this new driver wraps a pci device as a
mediated device. For a pci device, once bound to vfio-pci-mdev
driver, user space access of this device will go through vfio
mdev framework. User should create a mdev before exposing the
device to user-space.

Benefit of this new driver would be acting as a sample driver
for recent changes from "vfio/mdev: IOMMU aware mediated device"
patchset. Also it could be a good start for future device specific
mdev migration support.

To use this driver:
a) build and load vfio-pci-mdev.ko module
   execute "make menuconfig" and config VFIO_PCI_MDEV
   then load it with following command
   > sudo modprobe vfio
   > sudo modprobe vfio-pci
   > sudo modprobe vfio-pci-mdev

b) unbind original device driver
   e.g. for device with its bdf as $dev_bdf, use following command
   to unbind its original driver
   > echo $dev_bdf > /sys/bus/pci/devices/$dev_bdf/driver/unbind

c) bind vfio-pci-mdev driver to the physical device
   > echo $vend_id $dev_id > /sys/bus/pci/drivers/vfio-pci-mdev/new_id

d) check the supported mdev instances
   > ls /sys/bus/pci/devices/$dev_bdf/mdev_supported_types/
     vfio-pci-mdev-type1
   > ls /sys/bus/pci/devices/$dev_bdf/mdev_supported_types/\
     vfio-pci-mdev-type1/
     available_instances  create  device_api  devices  name

e)  create mdev on this physical device
   > echo "83b8f4f2-509f-382f-3c1e-e6bfe0fa1003" > \
     /sys/bus/pci/devices/$dev_bdf/mdev_supported_types/\
     vfio-pci-mdev-type1/create

f) passthru the mdev to guest
   add the following line in Qemu boot command
   -device vfio-pci,\
    sysfsdev=/sys/bus/mdev/devices/83b8f4f2-509f-382f-3c1e-e6bfe0fa1003

g) destroy mdev
   > echo 1 > /sys/bus/mdev/devices/83b8f4f2-509f-382f-3c1e-e6bfe0fa1003/\
     remove

Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Lu Baolu <baolu.lu@linux.intel.com>
Suggested-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Liu, Yi L <yi.l.liu@intel.com>
---
 drivers/vfio/pci/Kconfig         |  11 ++
 drivers/vfio/pci/Makefile        |   5 +
 drivers/vfio/pci/vfio_pci_mdev.c | 334 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 350 insertions(+)
 create mode 100644 drivers/vfio/pci/vfio_pci_mdev.c

diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
index d0f8e4f..30c6b15 100644
--- a/drivers/vfio/pci/Kconfig
+++ b/drivers/vfio/pci/Kconfig
@@ -9,6 +9,17 @@ config VFIO_PCI
 
 	  If you don't know what to do here, say N.
 
+config VFIO_PCI_MDEV
+	tristate "VFIO MDEV support for PCI devices"
+	depends on VFIO && PCI && EVENTFD && VFIO_PCI && VFIO_MDEV_DEVICE
+	select VFIO_VIRQFD
+	select IRQ_BYPASS_MANAGER
+	help
+	  Support for the PCI VFIO Mdev bus driver.  This is required to
+	  make use of PCI drivers using the VFIO Mdev framework.
+
+	  If you don't know what to do here, say N.
+
 config VFIO_PCI_VGA
 	bool "VFIO PCI support for VGA devices"
 	depends on VFIO_PCI && X86 && VGA_ARB
diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
index 9662c06..e840947 100644
--- a/drivers/vfio/pci/Makefile
+++ b/drivers/vfio/pci/Makefile
@@ -3,4 +3,9 @@ vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
 vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
 vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
 
+vfio-pci-mdev-y := vfio_pci_mdev.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
+vfio-pci-mdev-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
+vfio-pci-mdev-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
+
 obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
+obj-$(CONFIG_VFIO_PCI_MDEV) += vfio-pci-mdev.o
diff --git a/drivers/vfio/pci/vfio_pci_mdev.c b/drivers/vfio/pci/vfio_pci_mdev.c
new file mode 100644
index 0000000..15c8f7e
--- /dev/null
+++ b/drivers/vfio/pci/vfio_pci_mdev.c
@@ -0,0 +1,334 @@
+/*
+ * Copyright © 2019 Intel Corporation.
+ *     Author: Liu, Yi L <yi.l.liu@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Derived from original vfio_pci.c:
+ * Copyright (C) 2012 Red Hat, Inc.  All rights reserved.
+ *     Author: Alex Williamson <alex.williamson@redhat.com>
+ *
+ * Derived from original vfio:
+ * Copyright 2010 Cisco Systems, Inc.  All rights reserved.
+ * Author: Tom Lyon, pugs@cisco.com
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/device.h>
+#include <linux/eventfd.h>
+#include <linux/file.h>
+#include <linux/interrupt.h>
+#include <linux/iommu.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/pci.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/vfio.h>
+#include <linux/vgaarb.h>
+#include <linux/nospec.h>
+#include <linux/mdev.h>
+
+#include "vfio_pci_private.h"
+
+#define DRIVER_VERSION  "0.2"
+#define DRIVER_AUTHOR   "Alex Williamson <alex.williamson@redhat.com>"
+#define DRIVER_DESC     "VFIO PCI Mdev - User Level meta-driver"
+
+#define VFIO_PCI_MDEV_NAME  "vfio-pci-mdev"
+
+static ssize_t
+name_show(struct kobject *kobj, struct device *dev, char *buf)
+{
+	return sprintf(buf, "%s-type1\n", dev_name(dev));
+}
+
+MDEV_TYPE_ATTR_RO(name);
+
+static ssize_t
+available_instances_show(struct kobject *kobj, struct device *dev, char *buf)
+{
+	return sprintf(buf, "%d\n", 1);
+}
+
+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_PCI_STRING);
+}
+
+MDEV_TYPE_ATTR_RO(device_api);
+
+static struct attribute *vfio_pci_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 vfio_pci_mdev_type_group1 = {
+	.name  = "type1",
+	.attrs = vfio_pci_mdev_types_attrs,
+};
+
+struct attribute_group *vfio_pci_mdev_type_groups[] = {
+	&vfio_pci_mdev_type_group1,
+	NULL,
+};
+
+struct vfio_pci_mdev {
+	struct vfio_pci_device *vdev;
+	struct mdev_device *mdev;
+	unsigned long handle;
+};
+
+static int vfio_pci_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
+{
+	struct device *pdev;
+	struct vfio_pci_device *vdev;
+	struct vfio_pci_mdev *pmdev;
+	int ret;
+
+	pdev = mdev_parent_dev(mdev);
+	vdev = dev_get_drvdata(pdev);
+	pmdev = kzalloc(sizeof(struct vfio_pci_mdev), GFP_KERNEL);
+	if (pmdev == NULL) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	pmdev->mdev = mdev;
+	pmdev->vdev = vdev;
+	mdev_set_drvdata(mdev, pmdev);
+	ret = mdev_set_iommu_device(mdev_dev(mdev), pdev);
+	if (ret) {
+		pr_info("%s, failed to config iommu isolation for mdev: %s on pf: %s\n",
+			__func__, dev_name(mdev_dev(mdev)), dev_name(pdev));
+		goto out;
+	}
+
+out:
+	return ret;
+}
+
+static int vfio_pci_mdev_remove(struct mdev_device *mdev)
+{
+	struct vfio_pci_mdev *pmdev = mdev_get_drvdata(mdev);
+
+	kfree(pmdev);
+	pr_info("%s, succeeded for mdev: %s\n", __func__,
+		     dev_name(mdev_dev(mdev)));
+
+	return 0;
+}
+
+static int vfio_pci_mdev_open(struct mdev_device *mdev)
+{
+	int ret = 0;
+	struct vfio_pci_mdev *pmdev = mdev_get_drvdata(mdev);
+
+	ret = vfio_pci_open(pmdev->vdev);
+	if (!ret)
+		pr_info("Succeeded to open mdev: %s on pf: %s\n",
+		dev_name(mdev_dev(mdev)), dev_name(&pmdev->vdev->pdev->dev));
+	else
+		pr_info("Failed to open mdev: %s on pf: %s\n",
+		dev_name(mdev_dev(mdev)), dev_name(&pmdev->vdev->pdev->dev));
+	return ret;
+}
+
+static void vfio_pci_mdev_release(struct mdev_device *mdev)
+{
+	struct vfio_pci_mdev *pmdev = mdev_get_drvdata(mdev);
+
+	pr_info("Release mdev: %s on pf: %s\n",
+		dev_name(mdev_dev(mdev)), dev_name(&pmdev->vdev->pdev->dev));
+	vfio_pci_release(pmdev->vdev);
+}
+
+static long vfio_pci_mdev_ioctl(struct mdev_device *mdev, unsigned int cmd,
+			     unsigned long arg)
+{
+	struct vfio_pci_mdev *pmdev = mdev_get_drvdata(mdev);
+
+	return vfio_pci_ioctl(pmdev->vdev, cmd, arg);
+}
+
+static int vfio_pci_mdev_mmap(struct mdev_device *mdev,
+				struct vm_area_struct *vma)
+{
+	struct vfio_pci_mdev *pmdev = mdev_get_drvdata(mdev);
+
+	return vfio_pci_mmap(pmdev->vdev, vma);
+}
+
+static ssize_t vfio_pci_mdev_read(struct mdev_device *mdev, char __user *buf,
+			size_t count, loff_t *ppos)
+{
+	struct vfio_pci_mdev *pmdev = mdev_get_drvdata(mdev);
+
+	if (!count)
+		return 0;
+
+	return vfio_pci_rw(pmdev->vdev, buf, count, ppos, false);
+}
+
+static ssize_t vfio_pci_mdev_write(struct mdev_device *mdev,
+				const char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	struct vfio_pci_mdev *pmdev = mdev_get_drvdata(mdev);
+
+	if (!count)
+		return 0;
+
+	return vfio_pci_rw(pmdev->vdev, (char __user *)buf, count, ppos, true);
+}
+
+static const struct mdev_parent_ops vfio_pci_mdev_ops = {
+	.supported_type_groups	= vfio_pci_mdev_type_groups,
+	.create			= vfio_pci_mdev_create,
+	.remove			= vfio_pci_mdev_remove,
+
+	.open			= vfio_pci_mdev_open,
+	.release		= vfio_pci_mdev_release,
+
+	.read			= vfio_pci_mdev_read,
+	.write			= vfio_pci_mdev_write,
+	.mmap			= vfio_pci_mdev_mmap,
+	.ioctl			= vfio_pci_mdev_ioctl,
+};
+
+static int vfio_pci_mdev_driver_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct vfio_pci_device *vdev;
+	struct iommu_group *group;
+	int ret;
+
+	if (pdev->hdr_type != PCI_HEADER_TYPE_NORMAL)
+		return -EINVAL;
+
+	/*
+	 * Prevent binding to PFs with VFs enabled, this too easily allows
+	 * userspace instance with VFs and PFs from the same device, which
+	 * cannot work.  Disabling SR-IOV here would initiate removing the
+	 * VFs, which would unbind the driver, which is prone to blocking
+	 * if that VF is also in use by vfio-pci.  Just reject these PFs
+	 * and let the user sort it out.
+	 */
+	if (pci_num_vf(pdev)) {
+		pci_warn(pdev, "Cannot bind to PF with SR-IOV enabled\n");
+		return -EBUSY;
+	}
+
+	group = vfio_iommu_group_get(&pdev->dev);
+	if (!group)
+		return -EINVAL;
+
+	vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
+	if (!vdev) {
+		vfio_iommu_group_put(group, &pdev->dev);
+		return -ENOMEM;
+	}
+
+	vdev->pdev = pdev;
+	vdev->name = VFIO_PCI_MDEV_NAME;
+	vdev->irq_type = VFIO_PCI_NUM_IRQS;
+	mutex_init(&vdev->igate);
+	spin_lock_init(&vdev->irqlock);
+	mutex_init(&vdev->ioeventfds_lock);
+	INIT_LIST_HEAD(&vdev->ioeventfds_list);
+
+	pci_set_drvdata(pdev, vdev);
+
+	ret = vfio_pci_reflck_attach(vdev);
+	if (ret) {
+		vfio_del_group_dev(&pdev->dev);
+		vfio_iommu_group_put(group, &pdev->dev);
+		kfree(vdev);
+		return ret;
+	}
+
+	ret = vfio_pci_probe_misc(pdev, vdev);
+	if (ret) {
+		pr_err("failed to probe %s\n", dev_name(&pdev->dev));
+		return ret;
+	}
+
+	ret = mdev_register_device(&pdev->dev, &vfio_pci_mdev_ops);
+	if (ret)
+		pr_err("Cannot register mdev for device %s\n",
+			dev_name(&pdev->dev));
+	else
+		pr_info("Wrap device %s as a mdev\n", dev_name(&pdev->dev));
+
+	return ret;
+}
+
+static void vfio_pci_mdev_driver_remove(struct pci_dev *pdev)
+{
+	struct vfio_pci_device *vdev;
+
+	vdev = pci_get_drvdata(pdev);
+	if (!vdev)
+		return;
+
+	vfio_pci_reflck_put(vdev->reflck);
+
+	vfio_iommu_group_put(pdev->dev.iommu_group, &pdev->dev);
+	kfree(vdev->region);
+	mutex_destroy(&vdev->ioeventfds_lock);
+	kfree(vdev);
+
+	vfio_pci_remove_misc(pdev);
+}
+
+static struct pci_driver vfio_pci_mdev_driver = {
+	.name		= VFIO_PCI_MDEV_NAME,
+	.id_table	= NULL, /* only dynamic ids */
+	.probe		= vfio_pci_mdev_driver_probe,
+	.remove		= vfio_pci_mdev_driver_remove,
+	.err_handler	= &vfio_err_handlers,
+};
+
+static void __exit vfio_pci_mdev_cleanup(void)
+{
+	pci_unregister_driver(&vfio_pci_mdev_driver);
+	vfio_pci_uninit_perm_bits();
+}
+
+static int __init vfio_pci_mdev_init(void)
+{
+	int ret;
+
+	/* Allocate shared config space permision data used by all devices */
+	ret = vfio_pci_init_perm_bits();
+	if (ret)
+		return ret;
+
+	/* Register and scan for devices */
+	ret = pci_register_driver(&vfio_pci_mdev_driver);
+	if (ret)
+		goto out_driver;
+
+	return 0;
+out_driver:
+	vfio_pci_uninit_perm_bits();
+	return ret;
+}
+
+module_init(vfio_pci_mdev_init);
+module_exit(vfio_pci_mdev_cleanup);
+
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
-- 
2.7.4

Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci

[Alex Williamson]

On Tue, 12 Mar 2019 16:18:22 +0800
"Liu, Yi L" <yi.l.liu@intel.com> wrote:

> This patch exports the following symbols from vfio-pci driver
> for vfio-pci alike driver. e.g. vfio-pci-mdev driver
> 
> *) vfio_pci_set_vga_decode();
> *) vfio_pci_release();
> *) vfio_pci_open();
> *) vfio_pci_register_dev_region();
> *) vfio_pci_ioctl();
> *) vfio_pci_rw();
> *) vfio_pci_mmap();
> *) vfio_pci_request();
> *) vfio_pci_probe_misc();
> *) vfio_pci_remove_misc();
> *) vfio_err_handlers;
> *) vfio_pci_reflck_attach();
> *) vfio_pci_reflck_put();

Exporting all these symbols scares me a bit.  They're GPL and we don't
guarantee a kernel ABI, but I don't really want to support arbitrary
use cases either.  What if we re-factored the shared bits into a common
file and just linked them together?  Thanks,

Alex

> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Liu, Yi L <yi.l.liu@intel.com>
> ---
>  drivers/vfio/pci/vfio_pci.c         | 101 ++++++++++++++++++++++--------------
>  drivers/vfio/pci/vfio_pci_private.h |  17 ++++++
>  2 files changed, 78 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index ff60bd1..e36b922 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -73,7 +73,7 @@ static inline bool vfio_vga_disabled(void)
>   * has no way to get to it and routing can be disabled externally at the
>   * bridge.
>   */
> -static unsigned int vfio_pci_set_vga_decode(void *opaque, bool single_vga)
> +unsigned int vfio_pci_set_vga_decode(void *opaque, bool single_vga)
>  {
>  	struct vfio_pci_device *vdev = opaque;
>  	struct pci_dev *tmp = NULL, *pdev = vdev->pdev;
> @@ -103,6 +103,7 @@ static unsigned int vfio_pci_set_vga_decode(void *opaque, bool single_vga)
>  
>  	return decodes;
>  }
> +EXPORT_SYMBOL_GPL(vfio_pci_set_vga_decode);
>  
>  static inline bool vfio_pci_is_vga(struct pci_dev *pdev)
>  {
> @@ -410,7 +411,7 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
>  		pci_set_power_state(pdev, PCI_D3hot);
>  }
>  
> -static void vfio_pci_release(void *device_data)
> +void vfio_pci_release(void *device_data)
>  {
>  	struct vfio_pci_device *vdev = device_data;
>  
> @@ -425,8 +426,9 @@ static void vfio_pci_release(void *device_data)
>  
>  	module_put(THIS_MODULE);
>  }
> +EXPORT_SYMBOL_GPL(vfio_pci_release);
>  
> -static int vfio_pci_open(void *device_data)
> +int vfio_pci_open(void *device_data)
>  {
>  	struct vfio_pci_device *vdev = device_data;
>  	int ret = 0;
> @@ -450,6 +452,7 @@ static int vfio_pci_open(void *device_data)
>  		module_put(THIS_MODULE);
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(vfio_pci_open);
>  
>  static int vfio_pci_get_irq_count(struct vfio_pci_device *vdev, int irq_type)
>  {
> @@ -634,9 +637,10 @@ int vfio_pci_register_dev_region(struct vfio_pci_device *vdev,
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(vfio_pci_register_dev_region);
>  
> -static long vfio_pci_ioctl(void *device_data,
> -			   unsigned int cmd, unsigned long arg)
> +long vfio_pci_ioctl(void *device_data,
> +		   unsigned int cmd, unsigned long arg)
>  {
>  	struct vfio_pci_device *vdev = device_data;
>  	unsigned long minsz;
> @@ -1079,8 +1083,9 @@ static long vfio_pci_ioctl(void *device_data,
>  
>  	return -ENOTTY;
>  }
> +EXPORT_SYMBOL_GPL(vfio_pci_ioctl);
>  
> -static ssize_t vfio_pci_rw(void *device_data, char __user *buf,
> +ssize_t vfio_pci_rw(void *device_data, char __user *buf,
>  			   size_t count, loff_t *ppos, bool iswrite)
>  {
>  	unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> @@ -1111,6 +1116,7 @@ static ssize_t vfio_pci_rw(void *device_data, char __user *buf,
>  
>  	return -EINVAL;
>  }
> +EXPORT_SYMBOL_GPL(vfio_pci_rw);
>  
>  static ssize_t vfio_pci_read(void *device_data, char __user *buf,
>  			     size_t count, loff_t *ppos)
> @@ -1130,7 +1136,7 @@ static ssize_t vfio_pci_write(void *device_data, const char __user *buf,
>  	return vfio_pci_rw(device_data, (char __user *)buf, count, ppos, true);
>  }
>  
> -static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
> +int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
>  {
>  	struct vfio_pci_device *vdev = device_data;
>  	struct pci_dev *pdev = vdev->pdev;
> @@ -1173,7 +1179,7 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
>  	 */
>  	if (!vdev->barmap[index]) {
>  		ret = pci_request_selected_regions(pdev,
> -						   1 << index, "vfio-pci");
> +						   1 << index, vdev->name);
>  		if (ret)
>  			return ret;
>  
> @@ -1191,8 +1197,9 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
>  	return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
>  			       req_len, vma->vm_page_prot);
>  }
> +EXPORT_SYMBOL_GPL(vfio_pci_mmap);
>  
> -static void vfio_pci_request(void *device_data, unsigned int count)
> +void vfio_pci_request(void *device_data, unsigned int count)
>  {
>  	struct vfio_pci_device *vdev = device_data;
>  
> @@ -1211,6 +1218,7 @@ static void vfio_pci_request(void *device_data, unsigned int count)
>  
>  	mutex_unlock(&vdev->igate);
>  }
> +EXPORT_SYMBOL_GPL(vfio_pci_request);
>  
>  static const struct vfio_device_ops vfio_pci_ops = {
>  	.name		= "vfio-pci",
> @@ -1223,8 +1231,31 @@ static const struct vfio_device_ops vfio_pci_ops = {
>  	.request	= vfio_pci_request,
>  };
>  
> -static int vfio_pci_reflck_attach(struct vfio_pci_device *vdev);
> -static void vfio_pci_reflck_put(struct vfio_pci_reflck *reflck);
> +int vfio_pci_probe_misc(struct pci_dev *pdev, struct vfio_pci_device *vdev)
> +{
> +	if (vfio_pci_is_vga(pdev)) {
> +		vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> +		vga_set_legacy_decoding(pdev,
> +					vfio_pci_set_vga_decode(vdev, false));
> +	}
> +
> +	if (!disable_idle_d3) {
> +		/*
> +		 * pci-core sets the device power state to an unknown value at
> +		 * bootup and after being removed from a driver.  The only
> +		 * transition it allows from this unknown state is to D0, which
> +		 * typically happens when a driver calls pci_enable_device().
> +		 * We're not ready to enable the device yet, but we do want to
> +		 * be able to get to D3.  Therefore first do a D0 transition
> +		 * before going to D3.
> +		 */
> +		pci_set_power_state(pdev, PCI_D0);
> +		pci_set_power_state(pdev, PCI_D3hot);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vfio_pci_probe_misc);
>  
>  static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
> @@ -1259,6 +1290,7 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	}
>  
>  	vdev->pdev = pdev;
> +	vdev->name = "vfio-pci";
>  	vdev->irq_type = VFIO_PCI_NUM_IRQS;
>  	mutex_init(&vdev->igate);
>  	spin_lock_init(&vdev->irqlock);
> @@ -1280,28 +1312,23 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		return ret;
>  	}
>  
> +	ret = vfio_pci_probe_misc(pdev, vdev);
> +	return ret;
> +}
> +
> +void vfio_pci_remove_misc(struct pci_dev *pdev)
> +{
>  	if (vfio_pci_is_vga(pdev)) {
> -		vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> +		vga_client_register(pdev, NULL, NULL, NULL);
>  		vga_set_legacy_decoding(pdev,
> -					vfio_pci_set_vga_decode(vdev, false));
> +				VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM |
> +				VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM);
>  	}
>  
> -	if (!disable_idle_d3) {
> -		/*
> -		 * pci-core sets the device power state to an unknown value at
> -		 * bootup and after being removed from a driver.  The only
> -		 * transition it allows from this unknown state is to D0, which
> -		 * typically happens when a driver calls pci_enable_device().
> -		 * We're not ready to enable the device yet, but we do want to
> -		 * be able to get to D3.  Therefore first do a D0 transition
> -		 * before going to D3.
> -		 */
> +	if (!disable_idle_d3)
>  		pci_set_power_state(pdev, PCI_D0);
> -		pci_set_power_state(pdev, PCI_D3hot);
> -	}
> -
> -	return ret;
>  }
> +EXPORT_SYMBOL_GPL(vfio_pci_remove_misc);
>  
>  static void vfio_pci_remove(struct pci_dev *pdev)
>  {
> @@ -1317,16 +1344,7 @@ static void vfio_pci_remove(struct pci_dev *pdev)
>  	kfree(vdev->region);
>  	mutex_destroy(&vdev->ioeventfds_lock);
>  	kfree(vdev);
> -
> -	if (vfio_pci_is_vga(pdev)) {
> -		vga_client_register(pdev, NULL, NULL, NULL);
> -		vga_set_legacy_decoding(pdev,
> -				VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM |
> -				VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM);
> -	}
> -
> -	if (!disable_idle_d3)
> -		pci_set_power_state(pdev, PCI_D0);
> +	vfio_pci_remove_misc(pdev);
>  }
>  
>  static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
> @@ -1357,9 +1375,10 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
>  	return PCI_ERS_RESULT_CAN_RECOVER;
>  }
>  
> -static const struct pci_error_handlers vfio_err_handlers = {
> +const struct pci_error_handlers vfio_err_handlers = {
>  	.error_detected = vfio_pci_aer_err_detected,
>  };
> +EXPORT_SYMBOL_GPL(vfio_err_handlers);
>  
>  static struct pci_driver vfio_pci_driver = {
>  	.name		= "vfio-pci",
> @@ -1418,7 +1437,7 @@ static int vfio_pci_reflck_find(struct pci_dev *pdev, void *data)
>  	return 0;
>  }
>  
> -static int vfio_pci_reflck_attach(struct vfio_pci_device *vdev)
> +int vfio_pci_reflck_attach(struct vfio_pci_device *vdev)
>  {
>  	bool slot = !pci_probe_reset_slot(vdev->pdev->slot);
>  
> @@ -1433,6 +1452,7 @@ static int vfio_pci_reflck_attach(struct vfio_pci_device *vdev)
>  
>  	return PTR_ERR_OR_ZERO(vdev->reflck);
>  }
> +EXPORT_SYMBOL_GPL(vfio_pci_reflck_attach);
>  
>  static void vfio_pci_reflck_release(struct kref *kref)
>  {
> @@ -1444,10 +1464,11 @@ static void vfio_pci_reflck_release(struct kref *kref)
>  	mutex_unlock(&reflck_lock);
>  }
>  
> -static void vfio_pci_reflck_put(struct vfio_pci_reflck *reflck)
> +void vfio_pci_reflck_put(struct vfio_pci_reflck *reflck)
>  {
>  	kref_put_mutex(&reflck->kref, vfio_pci_reflck_release, &reflck_lock);
>  }
> +EXPORT_SYMBOL_GPL(vfio_pci_reflck_put);
>  
>  struct vfio_devices {
>  	struct vfio_device **devices;
> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> index 8c0009f..da9afe5 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -90,6 +90,7 @@ struct vfio_pci_reflck {
>  struct vfio_pci_device {
>  	struct pci_dev		*pdev;
>  	void __iomem		*barmap[PCI_STD_RESOURCE_END + 1];
> +	char			*name;
>  	bool			bar_mmap_supported[PCI_STD_RESOURCE_END + 1];
>  	u8			*pci_config_map;
>  	u8			*vconfig;
> @@ -125,6 +126,8 @@ struct vfio_pci_device {
>  	struct list_head	ioeventfds_list;
>  };
>  
> +extern const struct pci_error_handlers vfio_err_handlers;
> +
>  #define is_intx(vdev) (vdev->irq_type == VFIO_PCI_INTX_IRQ_INDEX)
>  #define is_msi(vdev) (vdev->irq_type == VFIO_PCI_MSI_IRQ_INDEX)
>  #define is_msix(vdev) (vdev->irq_type == VFIO_PCI_MSIX_IRQ_INDEX)
> @@ -154,6 +157,20 @@ extern long vfio_pci_ioeventfd(struct vfio_pci_device *vdev, loff_t offset,
>  extern int vfio_pci_init_perm_bits(void);
>  extern void vfio_pci_uninit_perm_bits(void);
>  
> +int vfio_pci_open(void *device_data);
> +void vfio_pci_release(void *device_data);
> +long vfio_pci_ioctl(void *device_data,
> +			   unsigned int cmd, unsigned long arg);
> +ssize_t vfio_pci_rw(void *device_data, char __user *buf,
> +			   size_t count, loff_t *ppos, bool iswrite);
> +int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma);
> +void vfio_pci_request(void *device_data, unsigned int count);
> +int vfio_pci_reflck_attach(struct vfio_pci_device *vdev);
> +void vfio_pci_reflck_put(struct vfio_pci_reflck *reflck);
> +unsigned int vfio_pci_set_vga_decode(void *opaque, bool single_vga);
> +int vfio_pci_probe_misc(struct pci_dev *pdev, struct vfio_pci_device *vdev);
> +void vfio_pci_remove_misc(struct pci_dev *pdev);
> +
>  extern int vfio_config_init(struct vfio_pci_device *vdev);
>  extern void vfio_config_free(struct vfio_pci_device *vdev);
>  

RE: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci

[Liu, Yi L]

> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Wednesday, March 20, 2019 2:14 AM
> To: Liu, Yi L <yi.l.liu@intel.com>
> Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
> 
> On Tue, 12 Mar 2019 16:18:22 +0800
> "Liu, Yi L" <yi.l.liu@intel.com> wrote:
> 
> > This patch exports the following symbols from vfio-pci driver
> > for vfio-pci alike driver. e.g. vfio-pci-mdev driver
> >
> > *) vfio_pci_set_vga_decode();
> > *) vfio_pci_release();
> > *) vfio_pci_open();
> > *) vfio_pci_register_dev_region();
> > *) vfio_pci_ioctl();
> > *) vfio_pci_rw();
> > *) vfio_pci_mmap();
> > *) vfio_pci_request();
> > *) vfio_pci_probe_misc();
> > *) vfio_pci_remove_misc();
> > *) vfio_err_handlers;
> > *) vfio_pci_reflck_attach();
> > *) vfio_pci_reflck_put();
> 
> Exporting all these symbols scares me a bit.  They're GPL and we don't
> guarantee a kernel ABI, but I don't really want to support arbitrary
> use cases either.  What if we re-factored the shared bits into a common
> file and just linked them together?  Thanks,

Hi, Alex,

Before refactor the code, I'd like to check with you on the module
parameters for the two modules. The existing vfio-pci driver has
some module parameters. e.g. ids, nointxmask, disable_idle_d3.
For future usage and maintain, I think it is better to let the two
drivers have same parameters. However, I'm not 100% on whether
we want to allow user load vfio-pci.ko and vfio-pci-mdev.ko with
different parameter value? e.g. load vfio-pci.ko with nointxmask=false
while load vfio-pci-mdev.ko with nointxmask=true. How about your
opinion on it?

> 
> Alex

Thanks,
Yi Liu

Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci

[Alex Williamson]

On Wed, 20 Mar 2019 11:49:37 +0000
"Liu, Yi L" <yi.l.liu@intel.com> wrote:

> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Wednesday, March 20, 2019 2:14 AM
> > To: Liu, Yi L <yi.l.liu@intel.com>
> > Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
> > 
> > On Tue, 12 Mar 2019 16:18:22 +0800
> > "Liu, Yi L" <yi.l.liu@intel.com> wrote:
> >   
> > > This patch exports the following symbols from vfio-pci driver
> > > for vfio-pci alike driver. e.g. vfio-pci-mdev driver
> > >
> > > *) vfio_pci_set_vga_decode();
> > > *) vfio_pci_release();
> > > *) vfio_pci_open();
> > > *) vfio_pci_register_dev_region();
> > > *) vfio_pci_ioctl();
> > > *) vfio_pci_rw();
> > > *) vfio_pci_mmap();
> > > *) vfio_pci_request();
> > > *) vfio_pci_probe_misc();
> > > *) vfio_pci_remove_misc();
> > > *) vfio_err_handlers;
> > > *) vfio_pci_reflck_attach();
> > > *) vfio_pci_reflck_put();  
> > 
> > Exporting all these symbols scares me a bit.  They're GPL and we don't
> > guarantee a kernel ABI, but I don't really want to support arbitrary
> > use cases either.  What if we re-factored the shared bits into a common
> > file and just linked them together?  Thanks,  
> 
> Hi, Alex,
> 
> Before refactor the code, I'd like to check with you on the module
> parameters for the two modules. The existing vfio-pci driver has
> some module parameters. e.g. ids, nointxmask, disable_idle_d3.
> For future usage and maintain, I think it is better to let the two
> drivers have same parameters. However, I'm not 100% on whether
> we want to allow user load vfio-pci.ko and vfio-pci-mdev.ko with
> different parameter value? e.g. load vfio-pci.ko with nointxmask=false
> while load vfio-pci-mdev.ko with nointxmask=true. How about your
> opinion on it?

Hi Yi,

I agree that it makes sense to retain the module options for the mdev
wrapped version, but I expect we should also allow dissimilar user
settings.  If those lived in the common code that gets linked separately
with each module, that should work fine, I think.  We can worry about
refactoring for future driver that might not want those options later.
Thanks,

Alex

RE: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci

[Liu, Yi L]

> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Thursday, March 21, 2019 3:22 AM
> To: Liu, Yi L <yi.l.liu@intel.com>
> Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
> 
> On Wed, 20 Mar 2019 11:49:37 +0000
> "Liu, Yi L" <yi.l.liu@intel.com> wrote:
> 
> > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > Sent: Wednesday, March 20, 2019 2:14 AM
> > > To: Liu, Yi L <yi.l.liu@intel.com>
> > > Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
> > >
> > > On Tue, 12 Mar 2019 16:18:22 +0800
> > > "Liu, Yi L" <yi.l.liu@intel.com> wrote:
> > >
> > > > This patch exports the following symbols from vfio-pci driver
> > > > for vfio-pci alike driver. e.g. vfio-pci-mdev driver
> > > >
> > > > *) vfio_pci_set_vga_decode();
> > > > *) vfio_pci_release();
> > > > *) vfio_pci_open();
> > > > *) vfio_pci_register_dev_region();
> > > > *) vfio_pci_ioctl();
> > > > *) vfio_pci_rw();
> > > > *) vfio_pci_mmap();
> > > > *) vfio_pci_request();
> > > > *) vfio_pci_probe_misc();
> > > > *) vfio_pci_remove_misc();
> > > > *) vfio_err_handlers;
> > > > *) vfio_pci_reflck_attach();
> > > > *) vfio_pci_reflck_put();
> > >
> > > Exporting all these symbols scares me a bit.  They're GPL and we don't
> > > guarantee a kernel ABI, but I don't really want to support arbitrary
> > > use cases either.  What if we re-factored the shared bits into a common
> > > file and just linked them together?  Thanks,
> >
> > Hi, Alex,
> >
> > Before refactor the code, I'd like to check with you on the module
> > parameters for the two modules. The existing vfio-pci driver has
> > some module parameters. e.g. ids, nointxmask, disable_idle_d3.
> > For future usage and maintain, I think it is better to let the two
> > drivers have same parameters. However, I'm not 100% on whether
> > we want to allow user load vfio-pci.ko and vfio-pci-mdev.ko with
> > different parameter value? e.g. load vfio-pci.ko with nointxmask=false
> > while load vfio-pci-mdev.ko with nointxmask=true. How about your
> > opinion on it?
> 
> Hi Yi,
> 
> I agree that it makes sense to retain the module options for the mdev
> wrapped version, but I expect we should also allow dissimilar user
> settings.  If those lived in the common code that gets linked separately
> with each module, that should work fine, I think.  We can worry about
> refactoring for future driver that might not want those options later.

Hi Alex,

I tried to get a common file which includes the definitions of the module
options and the common interfaces and get it linked separately with each
module. It works well when linked separately by config the
CONFIG_VFIO_PCI=m and CONFIG_VFIO_PCI_MDEV=m in kernel
configuration file. CONFIG_VFIO_PCI_MDEV is a new Kconfig macro
for the mdev wrapped version driver. However, if building the vfio-pci
and the mdev wrapped version into kernel image (config the
CONFIG_VFIO_PCI=y and CONFIG_VFIO_PCI_MDEV=y), then the symbols
defined in the common file will be shared thus doesn't allow dissimilar
user settings.

Per my understanding, I think we expect to allow simultaneous usage of
the two drivers. So I think the way above doesn't meet our expectation.
I considered a possible proposal as below. May listen to your opinion
on it before heading to cook. Also, better idea is welcomed. :-)

- get a common file includes interfaces which are common and have
  input parameters to differentiate the calling from vfio-pci and the
  wrapped version. e.g. vfio_pci_rw(). may call it as vfio_pci_common.c.

- get another common file includes the definitions of the module options,
  and the functions which referred the options. Define all of them as static.
  may call it as common.c

- get vfio_pci.c which includes the module_init/exit interfaces and driver
  registration operations of vfio-pci.ko. This file should include the common.c
  above to have same module options with the mdev wrapped version.

- get vfio_pci_mdev.c which includes the module_init/exit interfaces and
  driver registration operations of vfio-pci-mdev.ko. It should also include
  the common.c above to have same module options with vfio-pci.ko.

- Makefile:
vfio-pci-y := vfio_pci.o vfio_pci_common.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o

vfio-pci-mdev-y := vfio_pci_mdev.o vfio_pci_common.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
vfio-pci-mdev-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
vfio-pci-mdev-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o

obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
obj-$(CONFIG_VFIO_PCI_MDEV) += vfio-pci-mdev.o

Thanks,
Yi Liu

Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci

[Alex Williamson]

On Sat, 23 Mar 2019 11:06:44 +0000
"Liu, Yi L" <yi.l.liu@intel.com> wrote:

> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Thursday, March 21, 2019 3:22 AM
> > To: Liu, Yi L <yi.l.liu@intel.com>
> > Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
> > 
> > On Wed, 20 Mar 2019 11:49:37 +0000
> > "Liu, Yi L" <yi.l.liu@intel.com> wrote:
> >   
> > > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > > Sent: Wednesday, March 20, 2019 2:14 AM
> > > > To: Liu, Yi L <yi.l.liu@intel.com>
> > > > Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
> > > >
> > > > On Tue, 12 Mar 2019 16:18:22 +0800
> > > > "Liu, Yi L" <yi.l.liu@intel.com> wrote:
> > > >  
> > > > > This patch exports the following symbols from vfio-pci driver
> > > > > for vfio-pci alike driver. e.g. vfio-pci-mdev driver
> > > > >
> > > > > *) vfio_pci_set_vga_decode();
> > > > > *) vfio_pci_release();
> > > > > *) vfio_pci_open();
> > > > > *) vfio_pci_register_dev_region();
> > > > > *) vfio_pci_ioctl();
> > > > > *) vfio_pci_rw();
> > > > > *) vfio_pci_mmap();
> > > > > *) vfio_pci_request();
> > > > > *) vfio_pci_probe_misc();
> > > > > *) vfio_pci_remove_misc();
> > > > > *) vfio_err_handlers;
> > > > > *) vfio_pci_reflck_attach();
> > > > > *) vfio_pci_reflck_put();  
> > > >
> > > > Exporting all these symbols scares me a bit.  They're GPL and we don't
> > > > guarantee a kernel ABI, but I don't really want to support arbitrary
> > > > use cases either.  What if we re-factored the shared bits into a common
> > > > file and just linked them together?  Thanks,  
> > >
> > > Hi, Alex,
> > >
> > > Before refactor the code, I'd like to check with you on the module
> > > parameters for the two modules. The existing vfio-pci driver has
> > > some module parameters. e.g. ids, nointxmask, disable_idle_d3.
> > > For future usage and maintain, I think it is better to let the two
> > > drivers have same parameters. However, I'm not 100% on whether
> > > we want to allow user load vfio-pci.ko and vfio-pci-mdev.ko with
> > > different parameter value? e.g. load vfio-pci.ko with nointxmask=false
> > > while load vfio-pci-mdev.ko with nointxmask=true. How about your
> > > opinion on it?  
> > 
> > Hi Yi,
> > 
> > I agree that it makes sense to retain the module options for the mdev
> > wrapped version, but I expect we should also allow dissimilar user
> > settings.  If those lived in the common code that gets linked separately
> > with each module, that should work fine, I think.  We can worry about
> > refactoring for future driver that might not want those options later.  
> 
> Hi Alex,
> 
> I tried to get a common file which includes the definitions of the module
> options and the common interfaces and get it linked separately with each
> module. It works well when linked separately by config the
> CONFIG_VFIO_PCI=m and CONFIG_VFIO_PCI_MDEV=m in kernel
> configuration file. CONFIG_VFIO_PCI_MDEV is a new Kconfig macro
> for the mdev wrapped version driver. However, if building the vfio-pci
> and the mdev wrapped version into kernel image (config the
> CONFIG_VFIO_PCI=y and CONFIG_VFIO_PCI_MDEV=y), then the symbols
> defined in the common file will be shared thus doesn't allow dissimilar
> user settings.
> 
> Per my understanding, I think we expect to allow simultaneous usage of
> the two drivers. So I think the way above doesn't meet our expectation.

I agree.  They should be related in implementation only, from a user
perspective they should be entirely separate.

> I considered a possible proposal as below. May listen to your opinion
> on it before heading to cook. Also, better idea is welcomed. :-)
> 
> - get a common file includes interfaces which are common and have
>   input parameters to differentiate the calling from vfio-pci and the
>   wrapped version. e.g. vfio_pci_rw(). may call it as vfio_pci_common.c.
> 
> - get another common file includes the definitions of the module options,
>   and the functions which referred the options. Define all of them as static.
>   may call it as common.c
> 
> - get vfio_pci.c which includes the module_init/exit interfaces and driver
>   registration operations of vfio-pci.ko. This file should include the common.c
>   above to have same module options with the mdev wrapped version.
> 
> - get vfio_pci_mdev.c which includes the module_init/exit interfaces and
>   driver registration operations of vfio-pci-mdev.ko. It should also include
>   the common.c above to have same module options with vfio-pci.ko.
> 
> - Makefile:
> vfio-pci-y := vfio_pci.o vfio_pci_common.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
> 
> vfio-pci-mdev-y := vfio_pci_mdev.o vfio_pci_common.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> vfio-pci-mdev-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> vfio-pci-mdev-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
> 
> obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> obj-$(CONFIG_VFIO_PCI_MDEV) += vfio-pci-mdev.o

Each module needs it's own module_init/exit and will register its own
struct pci_driver, which gives us separate control of the probe and
remove callbacks.  I think we want the drivers to have the same module
parameters initially, but we don't necessarily want to require it for
any future options, so we can duplicate the parameter declarations.
Then to support the shared code, I think we can easily push nointxmask,
disable_vga, and disable_idle_d3 into bools on the struct
vfio_pci_device, which would be allocated and set by each module's
probe function before calling the shared probe function.
vfio_fill_ids() could take a pointer to the array to keep them separate
between modules.  I think that just leaves the config permission bits,
vfio_pci_{un}init_perm_bits().  Could we use a simple atomic reference
counter on those to potentially share them so they get initialized by
the first caller and freed by the last user, at least in the case of
both drivers being compiled statically into the kernel?  Thanks,

Alex

RE: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci

[Liu, Yi L]

> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Tuesday, March 26, 2019 2:17 AM
> To: Liu, Yi L <yi.l.liu@intel.com>
> Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
> 
> On Sat, 23 Mar 2019 11:06:44 +0000
> "Liu, Yi L" <yi.l.liu@intel.com> wrote:
> > Hi Alex,

[...]

> >
> > I tried to get a common file which includes the definitions of the module
> > options and the common interfaces and get it linked separately with each
> > module. It works well when linked separately by config the
> > CONFIG_VFIO_PCI=m and CONFIG_VFIO_PCI_MDEV=m in kernel
> > configuration file. CONFIG_VFIO_PCI_MDEV is a new Kconfig macro
> > for the mdev wrapped version driver. However, if building the vfio-pci
> > and the mdev wrapped version into kernel image (config the
> > CONFIG_VFIO_PCI=y and CONFIG_VFIO_PCI_MDEV=y), then the symbols
> > defined in the common file will be shared thus doesn't allow dissimilar
> > user settings.
> >
> > Per my understanding, I think we expect to allow simultaneous usage of
> > the two drivers. So I think the way above doesn't meet our expectation.
> 
> I agree.  They should be related in implementation only, from a user
> perspective they should be entirely separate.
> 
> > I considered a possible proposal as below. May listen to your opinion
> > on it before heading to cook. Also, better idea is welcomed. :-)
> >
> > - get a common file includes interfaces which are common and have
> >   input parameters to differentiate the calling from vfio-pci and the
> >   wrapped version. e.g. vfio_pci_rw(). may call it as vfio_pci_common.c.
> >
> > - get another common file includes the definitions of the module options,
> >   and the functions which referred the options. Define all of them as static.
> >   may call it as common.c
> >
> > - get vfio_pci.c which includes the module_init/exit interfaces and driver
> >   registration operations of vfio-pci.ko. This file should include the common.c
> >   above to have same module options with the mdev wrapped version.
> >
> > - get vfio_pci_mdev.c which includes the module_init/exit interfaces and
> >   driver registration operations of vfio-pci-mdev.ko. It should also include
> >   the common.c above to have same module options with vfio-pci.ko.
> >
> > - Makefile:
> > vfio-pci-y := vfio_pci.o vfio_pci_common.o vfio_pci_intrs.o vfio_pci_rdwr.o
> vfio_pci_config.o
> > vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
> >
> > vfio-pci-mdev-y := vfio_pci_mdev.o vfio_pci_common.o vfio_pci_intrs.o
> vfio_pci_rdwr.o vfio_pci_config.o
> > vfio-pci-mdev-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > vfio-pci-mdev-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
> >
> > obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> > obj-$(CONFIG_VFIO_PCI_MDEV) += vfio-pci-mdev.o
> 
> Each module needs it's own module_init/exit and will register its own
> struct pci_driver, which gives us separate control of the probe and

Agreed.

> remove callbacks.  I think we want the drivers to have the same module
> parameters initially, but we don't necessarily want to require it for
> any future options, so we can duplicate the parameter declarations.
> Then to support the shared code, I think we can easily push nointxmask,
> disable_vga, and disable_idle_d3 into bools on the struct
> vfio_pci_device, which would be allocated and set by each module's
> probe function before calling the shared probe function.

sounds good to me. 

> vfio_fill_ids() could take a pointer to the array to keep them separate
> between modules. 

Agreed.

> I think that just leaves the config permission bits,
> vfio_pci_{un}init_perm_bits(). Could we use a simple atomic reference
> counter on those to potentially share them so they get initialized by
> the first caller and freed by the last user, at least in the case of
> both drivers being compiled statically into the kernel?  Thanks,

Sure, I can add it. The two modules will still share the cap_perms and
ecap_perms config bits when built statically in kernel. However, I think
such share is reasonable. I'll check if any other similar bits in other files.

> Alex

Thanks for the suggestions, Alex. Let me prepare another RFC.

Regards,
Yi Liu

Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci

[Alex Williamson]

On Tue, 26 Mar 2019 12:37:37 +0000
"Liu, Yi L" <yi.l.liu@intel.com> wrote:

> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Tuesday, March 26, 2019 2:17 AM
> > To: Liu, Yi L <yi.l.liu@intel.com>
> > Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
> > 
> > On Sat, 23 Mar 2019 11:06:44 +0000
> > "Liu, Yi L" <yi.l.liu@intel.com> wrote:  
> > > Hi Alex,  
> 
> [...]
> 
> > >
> > > I tried to get a common file which includes the definitions of the module
> > > options and the common interfaces and get it linked separately with each
> > > module. It works well when linked separately by config the
> > > CONFIG_VFIO_PCI=m and CONFIG_VFIO_PCI_MDEV=m in kernel
> > > configuration file. CONFIG_VFIO_PCI_MDEV is a new Kconfig macro
> > > for the mdev wrapped version driver. However, if building the vfio-pci
> > > and the mdev wrapped version into kernel image (config the
> > > CONFIG_VFIO_PCI=y and CONFIG_VFIO_PCI_MDEV=y), then the symbols
> > > defined in the common file will be shared thus doesn't allow dissimilar
> > > user settings.
> > >
> > > Per my understanding, I think we expect to allow simultaneous usage of
> > > the two drivers. So I think the way above doesn't meet our expectation.  
> > 
> > I agree.  They should be related in implementation only, from a user
> > perspective they should be entirely separate.
> >   
> > > I considered a possible proposal as below. May listen to your opinion
> > > on it before heading to cook. Also, better idea is welcomed. :-)
> > >
> > > - get a common file includes interfaces which are common and have
> > >   input parameters to differentiate the calling from vfio-pci and the
> > >   wrapped version. e.g. vfio_pci_rw(). may call it as vfio_pci_common.c.
> > >
> > > - get another common file includes the definitions of the module options,
> > >   and the functions which referred the options. Define all of them as static.
> > >   may call it as common.c
> > >
> > > - get vfio_pci.c which includes the module_init/exit interfaces and driver
> > >   registration operations of vfio-pci.ko. This file should include the common.c
> > >   above to have same module options with the mdev wrapped version.
> > >
> > > - get vfio_pci_mdev.c which includes the module_init/exit interfaces and
> > >   driver registration operations of vfio-pci-mdev.ko. It should also include
> > >   the common.c above to have same module options with vfio-pci.ko.
> > >
> > > - Makefile:
> > > vfio-pci-y := vfio_pci.o vfio_pci_common.o vfio_pci_intrs.o vfio_pci_rdwr.o  
> > vfio_pci_config.o  
> > > vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > > vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
> > >
> > > vfio-pci-mdev-y := vfio_pci_mdev.o vfio_pci_common.o vfio_pci_intrs.o  
> > vfio_pci_rdwr.o vfio_pci_config.o  
> > > vfio-pci-mdev-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > > vfio-pci-mdev-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
> > >
> > > obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> > > obj-$(CONFIG_VFIO_PCI_MDEV) += vfio-pci-mdev.o  
> > 
> > Each module needs it's own module_init/exit and will register its own
> > struct pci_driver, which gives us separate control of the probe and  
> 
> Agreed.
> 
> > remove callbacks.  I think we want the drivers to have the same module
> > parameters initially, but we don't necessarily want to require it for
> > any future options, so we can duplicate the parameter declarations.
> > Then to support the shared code, I think we can easily push nointxmask,
> > disable_vga, and disable_idle_d3 into bools on the struct
> > vfio_pci_device, which would be allocated and set by each module's
> > probe function before calling the shared probe function.  
> 
> sounds good to me. 
> 
> > vfio_fill_ids() could take a pointer to the array to keep them separate
> > between modules.   
> 
> Agreed.
> 
> > I think that just leaves the config permission bits,
> > vfio_pci_{un}init_perm_bits(). Could we use a simple atomic reference
> > counter on those to potentially share them so they get initialized by
> > the first caller and freed by the last user, at least in the case of
> > both drivers being compiled statically into the kernel?  Thanks,  
> 
> Sure, I can add it. The two modules will still share the cap_perms and
> ecap_perms config bits when built statically in kernel. However, I think
> such share is reasonable. I'll check if any other similar bits in other files.
> 
> > Alex  
> 
> Thanks for the suggestions, Alex. Let me prepare another RFC.

Thank Yi, I appreciate your work on this.  Also, I wonder if we might
want to reconsider placing this driver in samples, the Makefile might
be a little bit ugly with paths back to drivers/vfio/pci, but I don't
think we run into the same barriers as you did with previous
approaches.  Placing it in samples would at least alleviate any
confusion that this isn't a vfio-pci replacement, but more of an mdev
wrapper proof of concept.  Thanks,

Alex

RE: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci

[Liu, Yi L]

> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Tuesday, March 26, 2019 11:35 PM
> To: Liu, Yi L <yi.l.liu@intel.com>
> Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
> 
> On Tue, 26 Mar 2019 12:37:37 +0000
> "Liu, Yi L" <yi.l.liu@intel.com> wrote:
> 
> > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > Sent: Tuesday, March 26, 2019 2:17 AM
> > > To: Liu, Yi L <yi.l.liu@intel.com>
> > > Subject: Re: [RFC v2 1/2] vfio/pci: export common symbols in vfio-pci
> > >
> > > On Sat, 23 Mar 2019 11:06:44 +0000
> > > "Liu, Yi L" <yi.l.liu@intel.com> wrote:
> > > > Hi Alex,
> >
> > [...]
> >
> > > >
> > > > I tried to get a common file which includes the definitions of the module
> > > > options and the common interfaces and get it linked separately with each
> > > > module. It works well when linked separately by config the
> > > > CONFIG_VFIO_PCI=m and CONFIG_VFIO_PCI_MDEV=m in kernel
> > > > configuration file. CONFIG_VFIO_PCI_MDEV is a new Kconfig macro
> > > > for the mdev wrapped version driver. However, if building the vfio-pci
> > > > and the mdev wrapped version into kernel image (config the
> > > > CONFIG_VFIO_PCI=y and CONFIG_VFIO_PCI_MDEV=y), then the symbols
> > > > defined in the common file will be shared thus doesn't allow dissimilar
> > > > user settings.
> > > >
> > > > Per my understanding, I think we expect to allow simultaneous usage of
> > > > the two drivers. So I think the way above doesn't meet our expectation.
> > >
> > > I agree.  They should be related in implementation only, from a user
> > > perspective they should be entirely separate.
> > >
> > > > I considered a possible proposal as below. May listen to your opinion
> > > > on it before heading to cook. Also, better idea is welcomed. :-)
> > > >
> > > > - get a common file includes interfaces which are common and have
> > > >   input parameters to differentiate the calling from vfio-pci and the
> > > >   wrapped version. e.g. vfio_pci_rw(). may call it as vfio_pci_common.c.
> > > >
> > > > - get another common file includes the definitions of the module options,
> > > >   and the functions which referred the options. Define all of them as static.
> > > >   may call it as common.c
> > > >
> > > > - get vfio_pci.c which includes the module_init/exit interfaces and driver
> > > >   registration operations of vfio-pci.ko. This file should include the
> common.c
> > > >   above to have same module options with the mdev wrapped version.
> > > >
> > > > - get vfio_pci_mdev.c which includes the module_init/exit interfaces and
> > > >   driver registration operations of vfio-pci-mdev.ko. It should also include
> > > >   the common.c above to have same module options with vfio-pci.ko.
> > > >
> > > > - Makefile:
> > > > vfio-pci-y := vfio_pci.o vfio_pci_common.o vfio_pci_intrs.o
> vfio_pci_rdwr.o
> > > vfio_pci_config.o
> > > > vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > > > vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
> > > >
> > > > vfio-pci-mdev-y := vfio_pci_mdev.o vfio_pci_common.o vfio_pci_intrs.o
> > > vfio_pci_rdwr.o vfio_pci_config.o
> > > > vfio-pci-mdev-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > > > vfio-pci-mdev-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
> > > >
> > > > obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> > > > obj-$(CONFIG_VFIO_PCI_MDEV) += vfio-pci-mdev.o
> > >
> > > Each module needs it's own module_init/exit and will register its own
> > > struct pci_driver, which gives us separate control of the probe and
> >
> > Agreed.
> >
> > > remove callbacks.  I think we want the drivers to have the same module
> > > parameters initially, but we don't necessarily want to require it for
> > > any future options, so we can duplicate the parameter declarations.
> > > Then to support the shared code, I think we can easily push nointxmask,
> > > disable_vga, and disable_idle_d3 into bools on the struct
> > > vfio_pci_device, which would be allocated and set by each module's
> > > probe function before calling the shared probe function.
> >
> > sounds good to me.
> >
> > > vfio_fill_ids() could take a pointer to the array to keep them separate
> > > between modules.
> >
> > Agreed.
> >
> > > I think that just leaves the config permission bits,
> > > vfio_pci_{un}init_perm_bits(). Could we use a simple atomic reference
> > > counter on those to potentially share them so they get initialized by
> > > the first caller and freed by the last user, at least in the case of
> > > both drivers being compiled statically into the kernel?  Thanks,
> >
> > Sure, I can add it. The two modules will still share the cap_perms and
> > ecap_perms config bits when built statically in kernel. However, I think
> > such share is reasonable. I'll check if any other similar bits in other files.
> >
> > > Alex
> >
> > Thanks for the suggestions, Alex. Let me prepare another RFC.
> 
> Thank Yi, I appreciate your work on this.  Also, I wonder if we might
> want to reconsider placing this driver in samples, the Makefile might
> be a little bit ugly with paths back to drivers/vfio/pci, but I don't
> think we run into the same barriers as you did with previous
> approaches.  Placing it in samples would at least alleviate any
> confusion that this isn't a vfio-pci replacement, but more of an mdev
> wrapper proof of concept.  Thanks,

Hi Alex,

My pleasure. Honestly, placing this driver in samples is also my first
choice at the beginning. I didn't go ahead with it due to the fear of
huge code duplication with drivers/vfio/pci. I can make the changes
based on the discussions in this thread and place the wrapped driver
under samples with its Makefile path back to drivers/vfio/pci. I believe
letting Makefile paths back to drivers/vfio/pci is fine as there is existing
case of such manner in kernel. Pls feel free to let me know your latest
idea or anything I missed. Thanks.

Regards,
Yi Liu