[PATCH v2 0/2] vfio: blacklist legacy virtio devices

[PATCH v2 0/2] vfio: blacklist legacy virtio devices

[Michael S. Tsirkin]

Legacy virtio devices always bypassed an IOMMU, so using them with vfio was
never safe.  This adds a quirk detecting these and disabling VFIO unless the
noiommu mode is used.  At the moment, this only applies to virtio-pci devices.

The patch might make sense on stable as well.

Michael S. Tsirkin (2):
  vfio: report group noiommu status
  vfio: add virtio pci quirk

 drivers/vfio/pci/vfio_pci_private.h |   1 +
 include/linux/vfio.h                |   2 +
 drivers/vfio/pci/vfio_pci.c         |  14 ++++
 drivers/vfio/pci/vfio_pci_virtio.c  | 140 ++++++++++++++++++++++++++++++++++++
 drivers/vfio/vfio.c                 |  12 ++++
 drivers/vfio/pci/Makefile           |   1 +
 6 files changed, 170 insertions(+)
 create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c

-- 
MST

[PATCH v2 1/2] vfio: report group noiommu status

[Michael S. Tsirkin]

When using vfio, callers might want to know whether device is added
to a regular group or an non-iommu group.

Report this status from vfio_is_noiommu_group_dev.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/vfio.h |  2 ++
 drivers/vfio/vfio.c  | 12 ++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 0ecae0b..584757b 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -51,6 +51,8 @@ extern int vfio_add_group_dev(struct device *dev,
 			      const struct vfio_device_ops *ops,
 			      void *device_data);
 
+extern bool vfio_is_noiommu_group_dev(struct device *dev);
+
 extern void *vfio_del_group_dev(struct device *dev);
 extern struct vfio_device *vfio_device_get_from_dev(struct device *dev);
 extern void vfio_device_put(struct vfio_device *device);
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index d1d70e0..22f279f 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -872,6 +872,18 @@ static bool vfio_dev_present(struct vfio_group *group, struct device *dev)
 }
 
 /*
+ * Is device part of a noiommu group?
+ * Note: must call vfio_add_group_dev first.
+ */
+bool vfio_is_noiommu_group_dev(struct device *dev)
+{
+	struct vfio_device *device = dev_get_drvdata(dev);
+	struct vfio_group *group = device->group;
+
+	return group->noiommu;
+}
+
+/*
  * Decrement the device reference count and wait for the device to be
  * removed.  Open file descriptors for the device... */
 void *vfio_del_group_dev(struct device *dev)
-- 
MST

[PATCH v2 2/2] vfio: add virtio pci quirk

[Michael S. Tsirkin]

Modern virtio pci devices can set VIRTIO_F_IOMMU_PLATFORM
to signal they are safe to use with an IOMMU.

Without this bit, exposing the device to userspace is unsafe, so probe
and fail VFIO initialization unless noiommu is enabled.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/vfio/pci/vfio_pci_private.h |   1 +
 drivers/vfio/pci/vfio_pci.c         |  14 ++++
 drivers/vfio/pci/vfio_pci_virtio.c  | 140 ++++++++++++++++++++++++++++++++++++
 drivers/vfio/pci/Makefile           |   1 +
 4 files changed, 156 insertions(+)
 create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c

diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
index 2128de8..2bd5616 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -139,4 +139,5 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
 	return -ENODEV;
 }
 #endif
+extern int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu);
 #endif /* VFIO_PCI_PRIVATE_H */
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index d624a52..e93bf0c 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1236,6 +1236,20 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return ret;
 	}
 
+	if (pdev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET) {
+		bool noiommu = vfio_is_noiommu_group_dev(&pdev->dev);
+
+		ret = vfio_pci_virtio_quirk(vdev, noiommu);
+		if (ret) {
+			dev_warn(&vdev->pdev->dev,
+				 "Failed to setup Virtio for VFIO\n");
+			vfio_del_group_dev(&pdev->dev);
+			vfio_iommu_group_put(group, &pdev->dev);
+			kfree(vdev);
+			return ret;
+		}
+	}
+
 	if (vfio_pci_is_vga(pdev)) {
 		vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
 		vga_set_legacy_decoding(pdev,
diff --git a/drivers/vfio/pci/vfio_pci_virtio.c b/drivers/vfio/pci/vfio_pci_virtio.c
new file mode 100644
index 0000000..e1ecffd
--- /dev/null
+++ b/drivers/vfio/pci/vfio_pci_virtio.c
@@ -0,0 +1,140 @@
+/*
+ * VFIO PCI Intel Graphics support
+ *
+ * Copyright (C) 2016 Red Hat, Inc.  All rights reserved.
+ *	Author: Alex Williamson <alex.williamson@redhat.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.
+ *
+ * Register a device specific region through which to provide read-only
+ * access to the Intel IGD opregion.  The register defining the opregion
+ * address is also virtualized to prevent user modification.
+ */
+
+#include <linux/io.h>
+#include <linux/pci.h>
+#include <linux/uaccess.h>
+#include <linux/vfio.h>
+#include <linux/virtio_pci.h>
+#include <linux/virtio_config.h>
+
+#include "vfio_pci_private.h"
+
+/**
+ * virtio_pci_find_capability - walk capabilities to find device info.
+ * @dev: the pci device
+ * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
+ *
+ * Returns offset of the capability, or 0.
+ */
+static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type)
+{
+	int pos;
+
+	for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
+	     pos > 0;
+	     pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
+		u8 type;
+		pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
+							 cfg_type),
+				     &type);
+
+		if (type != cfg_type)
+			continue;
+
+		/* Ignore structures with reserved BAR values */
+		if (type != VIRTIO_PCI_CAP_PCI_CFG) {
+			u8 bar;
+
+			pci_read_config_byte(dev, pos +
+					     offsetof(struct virtio_pci_cap,
+						      bar),
+					     &bar);
+			if (bar > 0x5)
+				continue;
+		}
+
+		return pos;
+	}
+	return 0;
+}
+
+
+int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu)
+{
+	struct pci_dev *dev = vdev->pdev;
+	int common, cfg;
+	u32 features;
+	u32 offset;
+	u8 bar;
+
+	/* Without an IOMMU, we don't care */
+	if (noiommu)
+		return 0;
+
+        /* Virtio only owns devices >= 0x1000 and <= 0x107f: leave the rest. */
+        if (dev->device < 0x1000 || dev->device > 0x107f)
+                return 0;
+
+	/* Check whether device enforces the IOMMU correctly */
+
+	/*
+	 * All modern devices must have common and cfg capabilities. We use cfg
+	 * capability for access so that we don't need to worry about resource
+	 * availability. Slow but sure.
+	 * Note that all vendor-specific fields we access are little-endian
+	 * which matches what pci config accessors expect, so they do byteswap
+	 * for us if appropriate.
+	 */
+	common = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG);
+	cfg = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_PCI_CFG);
+	if (!cfg || !common) {
+                dev_warn(&dev->dev,
+                         "Virtio device lacks common or pci cfg.\n");
+		return -ENODEV;
+	}
+
+	pci_read_config_byte(dev, common + offsetof(struct virtio_pci_cap,
+						    bar),
+			     &bar);
+	pci_read_config_dword(dev, common + offsetof(struct virtio_pci_cap,
+						    offset),
+			     &offset);
+
+	/* Program cfg capability for dword access into common cfg. */
+	pci_write_config_byte(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
+						  cap.bar),
+			      bar);
+	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
+						   cap.length),
+			       0x4);
+
+	/* Select features dword that has VIRTIO_F_IOMMU_PLATFORM. */
+	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
+						  cap.offset),
+			       offset + offsetof(struct virtio_pci_common_cfg,
+						 device_feature_select));
+	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
+						  pci_cfg_data),
+			       VIRTIO_F_IOMMU_PLATFORM / 32);
+
+	/* Get the features dword. */
+	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
+						  cap.offset),
+			       offset + offsetof(struct virtio_pci_common_cfg,
+						 device_feature));
+	pci_read_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
+						  pci_cfg_data),
+			      &features);
+
+	/* Does this device obey the platform's IOMMU? If not it's an error. */
+	if (!(features & (0x1 << (VIRTIO_F_IOMMU_PLATFORM % 32)))) {
+                dev_warn(&dev->dev,
+                         "Virtio device lacks VIRTIO_F_IOMMU_PLATFORM.\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
index 76d8ec0..e9b20e7 100644
--- a/drivers/vfio/pci/Makefile
+++ b/drivers/vfio/pci/Makefile
@@ -1,5 +1,6 @@
 
 vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
+vfio-pci-y += vfio_pci_virtio.o
 vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
 
 obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
-- 
MST

Re: [PATCH v2 2/2] vfio: add virtio pci quirk

[Alex Williamson]

On Tue, 30 Aug 2016 05:27:17 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Modern virtio pci devices can set VIRTIO_F_IOMMU_PLATFORM
> to signal they are safe to use with an IOMMU.
> 
> Without this bit, exposing the device to userspace is unsafe, so probe
> and fail VFIO initialization unless noiommu is enabled.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/vfio/pci/vfio_pci_private.h |   1 +
>  drivers/vfio/pci/vfio_pci.c         |  14 ++++
>  drivers/vfio/pci/vfio_pci_virtio.c  | 140 ++++++++++++++++++++++++++++++++++++
>  drivers/vfio/pci/Makefile           |   1 +
>  4 files changed, 156 insertions(+)
>  create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
> 
> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> index 2128de8..2bd5616 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -139,4 +139,5 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
>  	return -ENODEV;
>  }
>  #endif
> +extern int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu);
>  #endif /* VFIO_PCI_PRIVATE_H */
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index d624a52..e93bf0c 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1236,6 +1236,20 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		return ret;
>  	}
>  
> +	if (pdev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET) {

Perhaps a vfio_pci_is_virtio() like vga below?  Let's test the device
ID range initially as well, this test raised a big red flag for me
whether all devices within this vendor ID were virtio.

> +		bool noiommu = vfio_is_noiommu_group_dev(&pdev->dev);

I think you can use iommu_present() for this and avoid patch 1of2.
noiommu is mutually exclusive to an iommu being present.  Seems like
all of this logic should be in the quirk itself, I'm not sure what it
buys to get the value here but wait until later to use it.  Using
iommu_present() could also move this test much earlier in
vfio_pci_probe() making the exit path easier.

> +
> +		ret = vfio_pci_virtio_quirk(vdev, noiommu);
> +		if (ret) {
> +			dev_warn(&vdev->pdev->dev,
> +				 "Failed to setup Virtio for VFIO\n");
> +			vfio_del_group_dev(&pdev->dev);
> +			vfio_iommu_group_put(group, &pdev->dev);
> +			kfree(vdev);
> +			return ret;
> +		}
> +	}
> +
>  	if (vfio_pci_is_vga(pdev)) {
>  		vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
>  		vga_set_legacy_decoding(pdev,
> diff --git a/drivers/vfio/pci/vfio_pci_virtio.c b/drivers/vfio/pci/vfio_pci_virtio.c
> new file mode 100644
> index 0000000..e1ecffd
> --- /dev/null
> +++ b/drivers/vfio/pci/vfio_pci_virtio.c
> @@ -0,0 +1,140 @@
> +/*
> + * VFIO PCI Intel Graphics support
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> + *
> + * Copyright (C) 2016 Red Hat, Inc.  All rights reserved.
> + *	Author: Alex Williamson <alex.williamson@redhat.com>

Update.

> + *
> + * 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.
> + *
> + * Register a device specific region through which to provide read-only
> + * access to the Intel IGD opregion.  The register defining the opregion
> + * address is also virtualized to prevent user modification.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/pci.h>
> +#include <linux/uaccess.h>

Are io.h and uaccess.h needed?

> +#include <linux/vfio.h>
> +#include <linux/virtio_pci.h>
> +#include <linux/virtio_config.h>
> +
> +#include "vfio_pci_private.h"
> +
> +/**
> + * virtio_pci_find_capability - walk capabilities to find device info.
> + * @dev: the pci device
> + * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> + *
> + * Returns offset of the capability, or 0.
> + */
> +static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type)

Does inlining this really make sense?

> +{
> +	int pos;
> +
> +	for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> +	     pos > 0;
> +	     pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> +		u8 type;
> +		pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> +							 cfg_type),
> +				     &type);
> +
> +		if (type != cfg_type)
> +			continue;
> +
> +		/* Ignore structures with reserved BAR values */
> +		if (type != VIRTIO_PCI_CAP_PCI_CFG) {
> +			u8 bar;
> +
> +			pci_read_config_byte(dev, pos +
> +					     offsetof(struct virtio_pci_cap,
> +						      bar),
> +					     &bar);
> +			if (bar > 0x5)

s/0x5/PCI_STD_RESOURCE_END/

> +				continue;
> +		}
> +
> +		return pos;
> +	}
> +	return 0;
> +}
> +
> +
> +int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu)
> +{
> +	struct pci_dev *dev = vdev->pdev;

Please use *pdev for consistency with the rest of drivers/vfio/pci/*

Also, is there any reason to pass the vfio_pci_device?  There's nothing
vfio here otherwise and we could remove more #includes.

> +	int common, cfg;
> +	u32 features;
> +	u32 offset;
> +	u8 bar;
> +
> +	/* Without an IOMMU, we don't care */
> +	if (noiommu)
> +		return 0;
> +
> +        /* Virtio only owns devices >= 0x1000 and <= 0x107f: leave the rest. */
> +        if (dev->device < 0x1000 || dev->device > 0x107f)
> +                return 0;

Whitespace

> +
> +	/* Check whether device enforces the IOMMU correctly */
> +
> +	/*
> +	 * All modern devices must have common and cfg capabilities. We use cfg
> +	 * capability for access so that we don't need to worry about resource
> +	 * availability. Slow but sure.
> +	 * Note that all vendor-specific fields we access are little-endian
> +	 * which matches what pci config accessors expect, so they do byteswap
> +	 * for us if appropriate.
> +	 */
> +	common = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG);
> +	cfg = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_PCI_CFG);
> +	if (!cfg || !common) {
> +                dev_warn(&dev->dev,
> +                         "Virtio device lacks common or pci cfg.\n");

Whitespace

> +		return -ENODEV;
> +	}
> +
> +	pci_read_config_byte(dev, common + offsetof(struct virtio_pci_cap,
> +						    bar),
> +			     &bar);
> +	pci_read_config_dword(dev, common + offsetof(struct virtio_pci_cap,
> +						    offset),
> +			     &offset);
> +
> +	/* Program cfg capability for dword access into common cfg. */
> +	pci_write_config_byte(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> +						  cap.bar),
> +			      bar);
> +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> +						   cap.length),
> +			       0x4);
> +
> +	/* Select features dword that has VIRTIO_F_IOMMU_PLATFORM. */
> +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> +						  cap.offset),
> +			       offset + offsetof(struct virtio_pci_common_cfg,
> +						 device_feature_select));
> +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> +						  pci_cfg_data),
> +			       VIRTIO_F_IOMMU_PLATFORM / 32);
> +
> +	/* Get the features dword. */
> +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> +						  cap.offset),
> +			       offset + offsetof(struct virtio_pci_common_cfg,
> +						 device_feature));
> +	pci_read_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> +						  pci_cfg_data),
> +			      &features);
> +
> +	/* Does this device obey the platform's IOMMU? If not it's an error. */
> +	if (!(features & (0x1 << (VIRTIO_F_IOMMU_PLATFORM % 32)))) {
> +                dev_warn(&dev->dev,
> +                         "Virtio device lacks VIRTIO_F_IOMMU_PLATFORM.\n");

Whitespace

> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> index 76d8ec0..e9b20e7 100644
> --- a/drivers/vfio/pci/Makefile
> +++ b/drivers/vfio/pci/Makefile
> @@ -1,5 +1,6 @@
>  
>  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> +vfio-pci-y += vfio_pci_virtio.o
>  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
>  
>  obj-$(CONFIG_VFIO_PCI) += vfio-pci.o

Thanks,
Alex

Re: [PATCH v2 2/2] vfio: add virtio pci quirk

[Michael S. Tsirkin]

On Mon, Aug 29, 2016 at 09:23:25PM -0600, Alex Williamson wrote:
> On Tue, 30 Aug 2016 05:27:17 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > Modern virtio pci devices can set VIRTIO_F_IOMMU_PLATFORM
> > to signal they are safe to use with an IOMMU.
> > 
> > Without this bit, exposing the device to userspace is unsafe, so probe
> > and fail VFIO initialization unless noiommu is enabled.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  drivers/vfio/pci/vfio_pci_private.h |   1 +
> >  drivers/vfio/pci/vfio_pci.c         |  14 ++++
> >  drivers/vfio/pci/vfio_pci_virtio.c  | 140 ++++++++++++++++++++++++++++++++++++
> >  drivers/vfio/pci/Makefile           |   1 +
> >  4 files changed, 156 insertions(+)
> >  create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
> > 
> > diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> > index 2128de8..2bd5616 100644
> > --- a/drivers/vfio/pci/vfio_pci_private.h
> > +++ b/drivers/vfio/pci/vfio_pci_private.h
> > @@ -139,4 +139,5 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
> >  	return -ENODEV;
> >  }
> >  #endif
> > +extern int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu);
> >  #endif /* VFIO_PCI_PRIVATE_H */
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index d624a52..e93bf0c 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -1236,6 +1236,20 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  		return ret;
> >  	}
> >  
> > +	if (pdev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET) {
> 
> Perhaps a vfio_pci_is_virtio() like vga below?  Let's test the device
> ID range initially as well, this test raised a big red flag for me
> whether all devices within this vendor ID were virtio.
> 
> > +		bool noiommu = vfio_is_noiommu_group_dev(&pdev->dev);
> 
> I think you can use iommu_present() for this and avoid patch 1of2.

So in presence of an IOMMU in the system, is it impossible to bind the
noiommu device?

I did not test this yet.

If yes this is something we'll need to fix as well -
we do want to allow binding noiommu to a legacy virtio device.



> noiommu is mutually exclusive to an iommu being present.  Seems like
> all of this logic should be in the quirk itself, I'm not sure what it
> buys to get the value here but wait until later to use it.  Using
> iommu_present() could also move this test much earlier in
> vfio_pci_probe() making the exit path easier.
> 
> > +
> > +		ret = vfio_pci_virtio_quirk(vdev, noiommu);
> > +		if (ret) {
> > +			dev_warn(&vdev->pdev->dev,
> > +				 "Failed to setup Virtio for VFIO\n");
> > +			vfio_del_group_dev(&pdev->dev);
> > +			vfio_iommu_group_put(group, &pdev->dev);
> > +			kfree(vdev);
> > +			return ret;
> > +		}
> > +	}
> > +
> >  	if (vfio_pci_is_vga(pdev)) {
> >  		vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> >  		vga_set_legacy_decoding(pdev,
> > diff --git a/drivers/vfio/pci/vfio_pci_virtio.c b/drivers/vfio/pci/vfio_pci_virtio.c
> > new file mode 100644
> > index 0000000..e1ecffd
> > --- /dev/null
> > +++ b/drivers/vfio/pci/vfio_pci_virtio.c
> > @@ -0,0 +1,140 @@
> > +/*
> > + * VFIO PCI Intel Graphics support
>       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > + *
> > + * Copyright (C) 2016 Red Hat, Inc.  All rights reserved.
> > + *	Author: Alex Williamson <alex.williamson@redhat.com>
> 
> Update.
> 
> > + *
> > + * 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.
> > + *
> > + * Register a device specific region through which to provide read-only
> > + * access to the Intel IGD opregion.  The register defining the opregion
> > + * address is also virtualized to prevent user modification.
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/pci.h>
> > +#include <linux/uaccess.h>
> 
> Are io.h and uaccess.h needed?
> 
> > +#include <linux/vfio.h>
> > +#include <linux/virtio_pci.h>
> > +#include <linux/virtio_config.h>
> > +
> > +#include "vfio_pci_private.h"
> > +
> > +/**
> > + * virtio_pci_find_capability - walk capabilities to find device info.
> > + * @dev: the pci device
> > + * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> > + *
> > + * Returns offset of the capability, or 0.
> > + */
> > +static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type)
> 
> Does inlining this really make sense?
> 
> > +{
> > +	int pos;
> > +
> > +	for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> > +	     pos > 0;
> > +	     pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> > +		u8 type;
> > +		pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> > +							 cfg_type),
> > +				     &type);
> > +
> > +		if (type != cfg_type)
> > +			continue;
> > +
> > +		/* Ignore structures with reserved BAR values */
> > +		if (type != VIRTIO_PCI_CAP_PCI_CFG) {
> > +			u8 bar;
> > +
> > +			pci_read_config_byte(dev, pos +
> > +					     offsetof(struct virtio_pci_cap,
> > +						      bar),
> > +					     &bar);
> > +			if (bar > 0x5)
> 
> s/0x5/PCI_STD_RESOURCE_END/
> 
> > +				continue;
> > +		}
> > +
> > +		return pos;
> > +	}
> > +	return 0;
> > +}
> > +
> > +
> > +int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu)
> > +{
> > +	struct pci_dev *dev = vdev->pdev;
> 
> Please use *pdev for consistency with the rest of drivers/vfio/pci/*
> 
> Also, is there any reason to pass the vfio_pci_device?  There's nothing
> vfio here otherwise and we could remove more #includes.
> 
> > +	int common, cfg;
> > +	u32 features;
> > +	u32 offset;
> > +	u8 bar;
> > +
> > +	/* Without an IOMMU, we don't care */
> > +	if (noiommu)
> > +		return 0;
> > +
> > +        /* Virtio only owns devices >= 0x1000 and <= 0x107f: leave the rest. */
> > +        if (dev->device < 0x1000 || dev->device > 0x107f)
> > +                return 0;
> 
> Whitespace
> 
> > +
> > +	/* Check whether device enforces the IOMMU correctly */
> > +
> > +	/*
> > +	 * All modern devices must have common and cfg capabilities. We use cfg
> > +	 * capability for access so that we don't need to worry about resource
> > +	 * availability. Slow but sure.
> > +	 * Note that all vendor-specific fields we access are little-endian
> > +	 * which matches what pci config accessors expect, so they do byteswap
> > +	 * for us if appropriate.
> > +	 */
> > +	common = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG);
> > +	cfg = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_PCI_CFG);
> > +	if (!cfg || !common) {
> > +                dev_warn(&dev->dev,
> > +                         "Virtio device lacks common or pci cfg.\n");
> 
> Whitespace
> 
> > +		return -ENODEV;
> > +	}
> > +
> > +	pci_read_config_byte(dev, common + offsetof(struct virtio_pci_cap,
> > +						    bar),
> > +			     &bar);
> > +	pci_read_config_dword(dev, common + offsetof(struct virtio_pci_cap,
> > +						    offset),
> > +			     &offset);
> > +
> > +	/* Program cfg capability for dword access into common cfg. */
> > +	pci_write_config_byte(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > +						  cap.bar),
> > +			      bar);
> > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > +						   cap.length),
> > +			       0x4);
> > +
> > +	/* Select features dword that has VIRTIO_F_IOMMU_PLATFORM. */
> > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > +						  cap.offset),
> > +			       offset + offsetof(struct virtio_pci_common_cfg,
> > +						 device_feature_select));
> > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > +						  pci_cfg_data),
> > +			       VIRTIO_F_IOMMU_PLATFORM / 32);
> > +
> > +	/* Get the features dword. */
> > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > +						  cap.offset),
> > +			       offset + offsetof(struct virtio_pci_common_cfg,
> > +						 device_feature));
> > +	pci_read_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > +						  pci_cfg_data),
> > +			      &features);
> > +
> > +	/* Does this device obey the platform's IOMMU? If not it's an error. */
> > +	if (!(features & (0x1 << (VIRTIO_F_IOMMU_PLATFORM % 32)))) {
> > +                dev_warn(&dev->dev,
> > +                         "Virtio device lacks VIRTIO_F_IOMMU_PLATFORM.\n");
> 
> Whitespace
> 
> > +		return -ENODEV;
> > +	}
> > +
> > +	return 0;
> > +}
> > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > index 76d8ec0..e9b20e7 100644
> > --- a/drivers/vfio/pci/Makefile
> > +++ b/drivers/vfio/pci/Makefile
> > @@ -1,5 +1,6 @@
> >  
> >  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > +vfio-pci-y += vfio_pci_virtio.o
> >  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> >  
> >  obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> 
> Thanks,
> Alex

Re: [PATCH v2 2/2] vfio: add virtio pci quirk

[Alex Williamson]

On Mon, 29 Aug 2016 21:23:25 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Tue, 30 Aug 2016 05:27:17 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > Modern virtio pci devices can set VIRTIO_F_IOMMU_PLATFORM
> > to signal they are safe to use with an IOMMU.
> > 
> > Without this bit, exposing the device to userspace is unsafe, so probe
> > and fail VFIO initialization unless noiommu is enabled.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  drivers/vfio/pci/vfio_pci_private.h |   1 +
> >  drivers/vfio/pci/vfio_pci.c         |  14 ++++
> >  drivers/vfio/pci/vfio_pci_virtio.c  | 140 ++++++++++++++++++++++++++++++++++++
> >  drivers/vfio/pci/Makefile           |   1 +
> >  4 files changed, 156 insertions(+)
> >  create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
> > 
> > diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> > index 2128de8..2bd5616 100644
> > --- a/drivers/vfio/pci/vfio_pci_private.h
> > +++ b/drivers/vfio/pci/vfio_pci_private.h
> > @@ -139,4 +139,5 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
> >  	return -ENODEV;
> >  }
> >  #endif
> > +extern int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu);
> >  #endif /* VFIO_PCI_PRIVATE_H */
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index d624a52..e93bf0c 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -1236,6 +1236,20 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  		return ret;
> >  	}
> >  
> > +	if (pdev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET) {  
> 
> Perhaps a vfio_pci_is_virtio() like vga below?  Let's test the device
> ID range initially as well, this test raised a big red flag for me
> whether all devices within this vendor ID were virtio.
> 
> > +		bool noiommu = vfio_is_noiommu_group_dev(&pdev->dev);  
> 
> I think you can use iommu_present() for this and avoid patch 1of2.
> noiommu is mutually exclusive to an iommu being present.  Seems like
> all of this logic should be in the quirk itself, I'm not sure what it
> buys to get the value here but wait until later to use it.  Using
> iommu_present() could also move this test much earlier in
> vfio_pci_probe() making the exit path easier.

Except then I'm reintroducing the bug fixed by 16ab8a5cbea4 since
iommu_present() assumes an IOMMU API based device.  I'll try to think if
there's another way to avoid adding the is_noiommu function.  Thanks,

Alex

> 
> > +
> > +		ret = vfio_pci_virtio_quirk(vdev, noiommu);
> > +		if (ret) {
> > +			dev_warn(&vdev->pdev->dev,
> > +				 "Failed to setup Virtio for VFIO\n");
> > +			vfio_del_group_dev(&pdev->dev);
> > +			vfio_iommu_group_put(group, &pdev->dev);
> > +			kfree(vdev);
> > +			return ret;
> > +		}
> > +	}
> > +
> >  	if (vfio_pci_is_vga(pdev)) {
> >  		vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> >  		vga_set_legacy_decoding(pdev,
> > diff --git a/drivers/vfio/pci/vfio_pci_virtio.c b/drivers/vfio/pci/vfio_pci_virtio.c
> > new file mode 100644
> > index 0000000..e1ecffd
> > --- /dev/null
> > +++ b/drivers/vfio/pci/vfio_pci_virtio.c
> > @@ -0,0 +1,140 @@
> > +/*
> > + * VFIO PCI Intel Graphics support  
>       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > + *
> > + * Copyright (C) 2016 Red Hat, Inc.  All rights reserved.
> > + *	Author: Alex Williamson <alex.williamson@redhat.com>  
> 
> Update.
> 
> > + *
> > + * 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.
> > + *
> > + * Register a device specific region through which to provide read-only
> > + * access to the Intel IGD opregion.  The register defining the opregion
> > + * address is also virtualized to prevent user modification.
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/pci.h>
> > +#include <linux/uaccess.h>  
> 
> Are io.h and uaccess.h needed?
> 
> > +#include <linux/vfio.h>
> > +#include <linux/virtio_pci.h>
> > +#include <linux/virtio_config.h>
> > +
> > +#include "vfio_pci_private.h"
> > +
> > +/**
> > + * virtio_pci_find_capability - walk capabilities to find device info.
> > + * @dev: the pci device
> > + * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> > + *
> > + * Returns offset of the capability, or 0.
> > + */
> > +static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type)  
> 
> Does inlining this really make sense?
> 
> > +{
> > +	int pos;
> > +
> > +	for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> > +	     pos > 0;
> > +	     pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> > +		u8 type;
> > +		pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> > +							 cfg_type),
> > +				     &type);
> > +
> > +		if (type != cfg_type)
> > +			continue;
> > +
> > +		/* Ignore structures with reserved BAR values */
> > +		if (type != VIRTIO_PCI_CAP_PCI_CFG) {
> > +			u8 bar;
> > +
> > +			pci_read_config_byte(dev, pos +
> > +					     offsetof(struct virtio_pci_cap,
> > +						      bar),
> > +					     &bar);
> > +			if (bar > 0x5)  
> 
> s/0x5/PCI_STD_RESOURCE_END/
> 
> > +				continue;
> > +		}
> > +
> > +		return pos;
> > +	}
> > +	return 0;
> > +}
> > +
> > +
> > +int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu)
> > +{
> > +	struct pci_dev *dev = vdev->pdev;  
> 
> Please use *pdev for consistency with the rest of drivers/vfio/pci/*
> 
> Also, is there any reason to pass the vfio_pci_device?  There's nothing
> vfio here otherwise and we could remove more #includes.
> 
> > +	int common, cfg;
> > +	u32 features;
> > +	u32 offset;
> > +	u8 bar;
> > +
> > +	/* Without an IOMMU, we don't care */
> > +	if (noiommu)
> > +		return 0;
> > +
> > +        /* Virtio only owns devices >= 0x1000 and <= 0x107f: leave the rest. */
> > +        if (dev->device < 0x1000 || dev->device > 0x107f)
> > +                return 0;  
> 
> Whitespace
> 
> > +
> > +	/* Check whether device enforces the IOMMU correctly */
> > +
> > +	/*
> > +	 * All modern devices must have common and cfg capabilities. We use cfg
> > +	 * capability for access so that we don't need to worry about resource
> > +	 * availability. Slow but sure.
> > +	 * Note that all vendor-specific fields we access are little-endian
> > +	 * which matches what pci config accessors expect, so they do byteswap
> > +	 * for us if appropriate.
> > +	 */
> > +	common = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG);
> > +	cfg = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_PCI_CFG);
> > +	if (!cfg || !common) {
> > +                dev_warn(&dev->dev,
> > +                         "Virtio device lacks common or pci cfg.\n");  
> 
> Whitespace
> 
> > +		return -ENODEV;
> > +	}
> > +
> > +	pci_read_config_byte(dev, common + offsetof(struct virtio_pci_cap,
> > +						    bar),
> > +			     &bar);
> > +	pci_read_config_dword(dev, common + offsetof(struct virtio_pci_cap,
> > +						    offset),
> > +			     &offset);
> > +
> > +	/* Program cfg capability for dword access into common cfg. */
> > +	pci_write_config_byte(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > +						  cap.bar),
> > +			      bar);
> > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > +						   cap.length),
> > +			       0x4);
> > +
> > +	/* Select features dword that has VIRTIO_F_IOMMU_PLATFORM. */
> > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > +						  cap.offset),
> > +			       offset + offsetof(struct virtio_pci_common_cfg,
> > +						 device_feature_select));
> > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > +						  pci_cfg_data),
> > +			       VIRTIO_F_IOMMU_PLATFORM / 32);
> > +
> > +	/* Get the features dword. */
> > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > +						  cap.offset),
> > +			       offset + offsetof(struct virtio_pci_common_cfg,
> > +						 device_feature));
> > +	pci_read_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > +						  pci_cfg_data),
> > +			      &features);
> > +
> > +	/* Does this device obey the platform's IOMMU? If not it's an error. */
> > +	if (!(features & (0x1 << (VIRTIO_F_IOMMU_PLATFORM % 32)))) {
> > +                dev_warn(&dev->dev,
> > +                         "Virtio device lacks VIRTIO_F_IOMMU_PLATFORM.\n");  
> 
> Whitespace
> 
> > +		return -ENODEV;
> > +	}
> > +
> > +	return 0;
> > +}
> > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > index 76d8ec0..e9b20e7 100644
> > --- a/drivers/vfio/pci/Makefile
> > +++ b/drivers/vfio/pci/Makefile
> > @@ -1,5 +1,6 @@
> >  
> >  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > +vfio-pci-y += vfio_pci_virtio.o
> >  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> >  
> >  obj-$(CONFIG_VFIO_PCI) += vfio-pci.o  
> 
> Thanks,
> Alex

Re: [PATCH v2 2/2] vfio: add virtio pci quirk

[Michael S. Tsirkin]

On Mon, Aug 29, 2016 at 09:52:20PM -0600, Alex Williamson wrote:
> On Mon, 29 Aug 2016 21:23:25 -0600
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> > On Tue, 30 Aug 2016 05:27:17 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > Modern virtio pci devices can set VIRTIO_F_IOMMU_PLATFORM
> > > to signal they are safe to use with an IOMMU.
> > > 
> > > Without this bit, exposing the device to userspace is unsafe, so probe
> > > and fail VFIO initialization unless noiommu is enabled.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  drivers/vfio/pci/vfio_pci_private.h |   1 +
> > >  drivers/vfio/pci/vfio_pci.c         |  14 ++++
> > >  drivers/vfio/pci/vfio_pci_virtio.c  | 140 ++++++++++++++++++++++++++++++++++++
> > >  drivers/vfio/pci/Makefile           |   1 +
> > >  4 files changed, 156 insertions(+)
> > >  create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
> > > 
> > > diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> > > index 2128de8..2bd5616 100644
> > > --- a/drivers/vfio/pci/vfio_pci_private.h
> > > +++ b/drivers/vfio/pci/vfio_pci_private.h
> > > @@ -139,4 +139,5 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
> > >  	return -ENODEV;
> > >  }
> > >  #endif
> > > +extern int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu);
> > >  #endif /* VFIO_PCI_PRIVATE_H */
> > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > > index d624a52..e93bf0c 100644
> > > --- a/drivers/vfio/pci/vfio_pci.c
> > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > @@ -1236,6 +1236,20 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > >  		return ret;
> > >  	}
> > >  
> > > +	if (pdev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET) {  
> > 
> > Perhaps a vfio_pci_is_virtio() like vga below?  Let's test the device
> > ID range initially as well, this test raised a big red flag for me
> > whether all devices within this vendor ID were virtio.
> > 
> > > +		bool noiommu = vfio_is_noiommu_group_dev(&pdev->dev);  
> > 
> > I think you can use iommu_present() for this and avoid patch 1of2.
> > noiommu is mutually exclusive to an iommu being present.  Seems like
> > all of this logic should be in the quirk itself, I'm not sure what it
> > buys to get the value here but wait until later to use it.  Using
> > iommu_present() could also move this test much earlier in
> > vfio_pci_probe() making the exit path easier.
> 
> Except then I'm reintroducing the bug fixed by 16ab8a5cbea4 since
> iommu_present() assumes an IOMMU API based device.  I'll try to think if
> there's another way to avoid adding the is_noiommu function.  Thanks,
> 
> Alex

FWIW I'm only too happy if you take over this patch.
You need Jason's recent patchset to QEMU to test,
but otherwise no special hardware is required.

> > 
> > > +
> > > +		ret = vfio_pci_virtio_quirk(vdev, noiommu);
> > > +		if (ret) {
> > > +			dev_warn(&vdev->pdev->dev,
> > > +				 "Failed to setup Virtio for VFIO\n");
> > > +			vfio_del_group_dev(&pdev->dev);
> > > +			vfio_iommu_group_put(group, &pdev->dev);
> > > +			kfree(vdev);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > >  	if (vfio_pci_is_vga(pdev)) {
> > >  		vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> > >  		vga_set_legacy_decoding(pdev,
> > > diff --git a/drivers/vfio/pci/vfio_pci_virtio.c b/drivers/vfio/pci/vfio_pci_virtio.c
> > > new file mode 100644
> > > index 0000000..e1ecffd
> > > --- /dev/null
> > > +++ b/drivers/vfio/pci/vfio_pci_virtio.c
> > > @@ -0,0 +1,140 @@
> > > +/*
> > > + * VFIO PCI Intel Graphics support  
> >       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > + *
> > > + * Copyright (C) 2016 Red Hat, Inc.  All rights reserved.
> > > + *	Author: Alex Williamson <alex.williamson@redhat.com>  
> > 
> > Update.
> > 
> > > + *
> > > + * 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.
> > > + *
> > > + * Register a device specific region through which to provide read-only
> > > + * access to the Intel IGD opregion.  The register defining the opregion
> > > + * address is also virtualized to prevent user modification.
> > > + */
> > > +
> > > +#include <linux/io.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/uaccess.h>  
> > 
> > Are io.h and uaccess.h needed?
> > 
> > > +#include <linux/vfio.h>
> > > +#include <linux/virtio_pci.h>
> > > +#include <linux/virtio_config.h>
> > > +
> > > +#include "vfio_pci_private.h"
> > > +
> > > +/**
> > > + * virtio_pci_find_capability - walk capabilities to find device info.
> > > + * @dev: the pci device
> > > + * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> > > + *
> > > + * Returns offset of the capability, or 0.
> > > + */
> > > +static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type)  
> > 
> > Does inlining this really make sense?
> > 
> > > +{
> > > +	int pos;
> > > +
> > > +	for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> > > +	     pos > 0;
> > > +	     pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> > > +		u8 type;
> > > +		pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> > > +							 cfg_type),
> > > +				     &type);
> > > +
> > > +		if (type != cfg_type)
> > > +			continue;
> > > +
> > > +		/* Ignore structures with reserved BAR values */
> > > +		if (type != VIRTIO_PCI_CAP_PCI_CFG) {
> > > +			u8 bar;
> > > +
> > > +			pci_read_config_byte(dev, pos +
> > > +					     offsetof(struct virtio_pci_cap,
> > > +						      bar),
> > > +					     &bar);
> > > +			if (bar > 0x5)  
> > 
> > s/0x5/PCI_STD_RESOURCE_END/
> > 
> > > +				continue;
> > > +		}
> > > +
> > > +		return pos;
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +
> > > +int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu)
> > > +{
> > > +	struct pci_dev *dev = vdev->pdev;  
> > 
> > Please use *pdev for consistency with the rest of drivers/vfio/pci/*
> > 
> > Also, is there any reason to pass the vfio_pci_device?  There's nothing
> > vfio here otherwise and we could remove more #includes.
> > 
> > > +	int common, cfg;
> > > +	u32 features;
> > > +	u32 offset;
> > > +	u8 bar;
> > > +
> > > +	/* Without an IOMMU, we don't care */
> > > +	if (noiommu)
> > > +		return 0;
> > > +
> > > +        /* Virtio only owns devices >= 0x1000 and <= 0x107f: leave the rest. */
> > > +        if (dev->device < 0x1000 || dev->device > 0x107f)
> > > +                return 0;  
> > 
> > Whitespace
> > 
> > > +
> > > +	/* Check whether device enforces the IOMMU correctly */
> > > +
> > > +	/*
> > > +	 * All modern devices must have common and cfg capabilities. We use cfg
> > > +	 * capability for access so that we don't need to worry about resource
> > > +	 * availability. Slow but sure.
> > > +	 * Note that all vendor-specific fields we access are little-endian
> > > +	 * which matches what pci config accessors expect, so they do byteswap
> > > +	 * for us if appropriate.
> > > +	 */
> > > +	common = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG);
> > > +	cfg = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_PCI_CFG);
> > > +	if (!cfg || !common) {
> > > +                dev_warn(&dev->dev,
> > > +                         "Virtio device lacks common or pci cfg.\n");  
> > 
> > Whitespace
> > 
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	pci_read_config_byte(dev, common + offsetof(struct virtio_pci_cap,
> > > +						    bar),
> > > +			     &bar);
> > > +	pci_read_config_dword(dev, common + offsetof(struct virtio_pci_cap,
> > > +						    offset),
> > > +			     &offset);
> > > +
> > > +	/* Program cfg capability for dword access into common cfg. */
> > > +	pci_write_config_byte(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > +						  cap.bar),
> > > +			      bar);
> > > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > +						   cap.length),
> > > +			       0x4);
> > > +
> > > +	/* Select features dword that has VIRTIO_F_IOMMU_PLATFORM. */
> > > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > +						  cap.offset),
> > > +			       offset + offsetof(struct virtio_pci_common_cfg,
> > > +						 device_feature_select));
> > > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > +						  pci_cfg_data),
> > > +			       VIRTIO_F_IOMMU_PLATFORM / 32);
> > > +
> > > +	/* Get the features dword. */
> > > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > +						  cap.offset),
> > > +			       offset + offsetof(struct virtio_pci_common_cfg,
> > > +						 device_feature));
> > > +	pci_read_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > +						  pci_cfg_data),
> > > +			      &features);
> > > +
> > > +	/* Does this device obey the platform's IOMMU? If not it's an error. */
> > > +	if (!(features & (0x1 << (VIRTIO_F_IOMMU_PLATFORM % 32)))) {
> > > +                dev_warn(&dev->dev,
> > > +                         "Virtio device lacks VIRTIO_F_IOMMU_PLATFORM.\n");  
> > 
> > Whitespace
> > 
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > > index 76d8ec0..e9b20e7 100644
> > > --- a/drivers/vfio/pci/Makefile
> > > +++ b/drivers/vfio/pci/Makefile
> > > @@ -1,5 +1,6 @@
> > >  
> > >  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > > +vfio-pci-y += vfio_pci_virtio.o
> > >  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > >  
> > >  obj-$(CONFIG_VFIO_PCI) += vfio-pci.o  
> > 
> > Thanks,
> > Alex

Re: [PATCH v2 2/2] vfio: add virtio pci quirk

[Alex Williamson]

On Mon, 29 Aug 2016 21:52:20 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Mon, 29 Aug 2016 21:23:25 -0600
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> > On Tue, 30 Aug 2016 05:27:17 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > Modern virtio pci devices can set VIRTIO_F_IOMMU_PLATFORM
> > > to signal they are safe to use with an IOMMU.
> > > 
> > > Without this bit, exposing the device to userspace is unsafe, so probe
> > > and fail VFIO initialization unless noiommu is enabled.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  drivers/vfio/pci/vfio_pci_private.h |   1 +
> > >  drivers/vfio/pci/vfio_pci.c         |  14 ++++
> > >  drivers/vfio/pci/vfio_pci_virtio.c  | 140 ++++++++++++++++++++++++++++++++++++
> > >  drivers/vfio/pci/Makefile           |   1 +
> > >  4 files changed, 156 insertions(+)
> > >  create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
> > > 
> > > diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> > > index 2128de8..2bd5616 100644
> > > --- a/drivers/vfio/pci/vfio_pci_private.h
> > > +++ b/drivers/vfio/pci/vfio_pci_private.h
> > > @@ -139,4 +139,5 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
> > >  	return -ENODEV;
> > >  }
> > >  #endif
> > > +extern int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu);
> > >  #endif /* VFIO_PCI_PRIVATE_H */
> > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > > index d624a52..e93bf0c 100644
> > > --- a/drivers/vfio/pci/vfio_pci.c
> > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > @@ -1236,6 +1236,20 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > >  		return ret;
> > >  	}
> > >  
> > > +	if (pdev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET) {    
> > 
> > Perhaps a vfio_pci_is_virtio() like vga below?  Let's test the device
> > ID range initially as well, this test raised a big red flag for me
> > whether all devices within this vendor ID were virtio.
> >   
> > > +		bool noiommu = vfio_is_noiommu_group_dev(&pdev->dev);    
> > 
> > I think you can use iommu_present() for this and avoid patch 1of2.
> > noiommu is mutually exclusive to an iommu being present.  Seems like
> > all of this logic should be in the quirk itself, I'm not sure what it
> > buys to get the value here but wait until later to use it.  Using
> > iommu_present() could also move this test much earlier in
> > vfio_pci_probe() making the exit path easier.  
> 
> Except then I'm reintroducing the bug fixed by 16ab8a5cbea4 since
> iommu_present() assumes an IOMMU API based device.  I'll try to think if
> there's another way to avoid adding the is_noiommu function.  Thanks,

I think something like this would do it.

--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1214,6 +1214,22 @@ static int vfio_pci_probe(struct pci_dev *pdev, const str
        if (pdev->hdr_type != PCI_HEADER_TYPE_NORMAL)
                return -EINVAL;
 
+       /*
+        * Filter out virtio devices that do not honor the iommu,
+        * but only for real iommu groups.
+        */
+       if (vfio_pci_is_virtio(pdev)) {
+               struct iommu_group *tmp = iommu_group_get(&pdev->dev);
+
+               if (tmp) {
+                       iommu_group_put(tmp);
+
+                       ret = vfio_pci_virtio_quirk(pdev);
+                       if (ret)
+                               return ret;
+               }
+       }
+
        group = vfio_iommu_group_get(&pdev->dev);
        if (!group)
                return -EINVAL;

Thanks,
Alex

> > > +
> > > +		ret = vfio_pci_virtio_quirk(vdev, noiommu);
> > > +		if (ret) {
> > > +			dev_warn(&vdev->pdev->dev,
> > > +				 "Failed to setup Virtio for VFIO\n");
> > > +			vfio_del_group_dev(&pdev->dev);
> > > +			vfio_iommu_group_put(group, &pdev->dev);
> > > +			kfree(vdev);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > >  	if (vfio_pci_is_vga(pdev)) {
> > >  		vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> > >  		vga_set_legacy_decoding(pdev,
> > > diff --git a/drivers/vfio/pci/vfio_pci_virtio.c b/drivers/vfio/pci/vfio_pci_virtio.c
> > > new file mode 100644
> > > index 0000000..e1ecffd
> > > --- /dev/null
> > > +++ b/drivers/vfio/pci/vfio_pci_virtio.c
> > > @@ -0,0 +1,140 @@
> > > +/*
> > > + * VFIO PCI Intel Graphics support    
> >       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  
> > > + *
> > > + * Copyright (C) 2016 Red Hat, Inc.  All rights reserved.
> > > + *	Author: Alex Williamson <alex.williamson@redhat.com>    
> > 
> > Update.
> >   
> > > + *
> > > + * 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.
> > > + *
> > > + * Register a device specific region through which to provide read-only
> > > + * access to the Intel IGD opregion.  The register defining the opregion
> > > + * address is also virtualized to prevent user modification.
> > > + */
> > > +
> > > +#include <linux/io.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/uaccess.h>    
> > 
> > Are io.h and uaccess.h needed?
> >   
> > > +#include <linux/vfio.h>
> > > +#include <linux/virtio_pci.h>
> > > +#include <linux/virtio_config.h>
> > > +
> > > +#include "vfio_pci_private.h"
> > > +
> > > +/**
> > > + * virtio_pci_find_capability - walk capabilities to find device info.
> > > + * @dev: the pci device
> > > + * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> > > + *
> > > + * Returns offset of the capability, or 0.
> > > + */
> > > +static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type)    
> > 
> > Does inlining this really make sense?
> >   
> > > +{
> > > +	int pos;
> > > +
> > > +	for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> > > +	     pos > 0;
> > > +	     pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> > > +		u8 type;
> > > +		pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> > > +							 cfg_type),
> > > +				     &type);
> > > +
> > > +		if (type != cfg_type)
> > > +			continue;
> > > +
> > > +		/* Ignore structures with reserved BAR values */
> > > +		if (type != VIRTIO_PCI_CAP_PCI_CFG) {
> > > +			u8 bar;
> > > +
> > > +			pci_read_config_byte(dev, pos +
> > > +					     offsetof(struct virtio_pci_cap,
> > > +						      bar),
> > > +					     &bar);
> > > +			if (bar > 0x5)    
> > 
> > s/0x5/PCI_STD_RESOURCE_END/
> >   
> > > +				continue;
> > > +		}
> > > +
> > > +		return pos;
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +
> > > +int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu)
> > > +{
> > > +	struct pci_dev *dev = vdev->pdev;    
> > 
> > Please use *pdev for consistency with the rest of drivers/vfio/pci/*
> > 
> > Also, is there any reason to pass the vfio_pci_device?  There's nothing
> > vfio here otherwise and we could remove more #includes.
> >   
> > > +	int common, cfg;
> > > +	u32 features;
> > > +	u32 offset;
> > > +	u8 bar;
> > > +
> > > +	/* Without an IOMMU, we don't care */
> > > +	if (noiommu)
> > > +		return 0;
> > > +
> > > +        /* Virtio only owns devices >= 0x1000 and <= 0x107f: leave the rest. */
> > > +        if (dev->device < 0x1000 || dev->device > 0x107f)
> > > +                return 0;    
> > 
> > Whitespace
> >   
> > > +
> > > +	/* Check whether device enforces the IOMMU correctly */
> > > +
> > > +	/*
> > > +	 * All modern devices must have common and cfg capabilities. We use cfg
> > > +	 * capability for access so that we don't need to worry about resource
> > > +	 * availability. Slow but sure.
> > > +	 * Note that all vendor-specific fields we access are little-endian
> > > +	 * which matches what pci config accessors expect, so they do byteswap
> > > +	 * for us if appropriate.
> > > +	 */
> > > +	common = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG);
> > > +	cfg = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_PCI_CFG);
> > > +	if (!cfg || !common) {
> > > +                dev_warn(&dev->dev,
> > > +                         "Virtio device lacks common or pci cfg.\n");    
> > 
> > Whitespace
> >   
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	pci_read_config_byte(dev, common + offsetof(struct virtio_pci_cap,
> > > +						    bar),
> > > +			     &bar);
> > > +	pci_read_config_dword(dev, common + offsetof(struct virtio_pci_cap,
> > > +						    offset),
> > > +			     &offset);
> > > +
> > > +	/* Program cfg capability for dword access into common cfg. */
> > > +	pci_write_config_byte(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > +						  cap.bar),
> > > +			      bar);
> > > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > +						   cap.length),
> > > +			       0x4);
> > > +
> > > +	/* Select features dword that has VIRTIO_F_IOMMU_PLATFORM. */
> > > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > +						  cap.offset),
> > > +			       offset + offsetof(struct virtio_pci_common_cfg,
> > > +						 device_feature_select));
> > > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > +						  pci_cfg_data),
> > > +			       VIRTIO_F_IOMMU_PLATFORM / 32);
> > > +
> > > +	/* Get the features dword. */
> > > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > +						  cap.offset),
> > > +			       offset + offsetof(struct virtio_pci_common_cfg,
> > > +						 device_feature));
> > > +	pci_read_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > +						  pci_cfg_data),
> > > +			      &features);
> > > +
> > > +	/* Does this device obey the platform's IOMMU? If not it's an error. */
> > > +	if (!(features & (0x1 << (VIRTIO_F_IOMMU_PLATFORM % 32)))) {
> > > +                dev_warn(&dev->dev,
> > > +                         "Virtio device lacks VIRTIO_F_IOMMU_PLATFORM.\n");    
> > 
> > Whitespace
> >   
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > > index 76d8ec0..e9b20e7 100644
> > > --- a/drivers/vfio/pci/Makefile
> > > +++ b/drivers/vfio/pci/Makefile
> > > @@ -1,5 +1,6 @@
> > >  
> > >  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > > +vfio-pci-y += vfio_pci_virtio.o
> > >  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > >  
> > >  obj-$(CONFIG_VFIO_PCI) += vfio-pci.o    
> > 
> > Thanks,
> > Alex  

Re: [PATCH v2 2/2] vfio: add virtio pci quirk

[Michael S. Tsirkin]

On Mon, Aug 29, 2016 at 10:53:04PM -0600, Alex Williamson wrote:
> On Mon, 29 Aug 2016 21:52:20 -0600
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> > On Mon, 29 Aug 2016 21:23:25 -0600
> > Alex Williamson <alex.williamson@redhat.com> wrote:
> > 
> > > On Tue, 30 Aug 2016 05:27:17 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > Modern virtio pci devices can set VIRTIO_F_IOMMU_PLATFORM
> > > > to signal they are safe to use with an IOMMU.
> > > > 
> > > > Without this bit, exposing the device to userspace is unsafe, so probe
> > > > and fail VFIO initialization unless noiommu is enabled.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  drivers/vfio/pci/vfio_pci_private.h |   1 +
> > > >  drivers/vfio/pci/vfio_pci.c         |  14 ++++
> > > >  drivers/vfio/pci/vfio_pci_virtio.c  | 140 ++++++++++++++++++++++++++++++++++++
> > > >  drivers/vfio/pci/Makefile           |   1 +
> > > >  4 files changed, 156 insertions(+)
> > > >  create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
> > > > 
> > > > diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> > > > index 2128de8..2bd5616 100644
> > > > --- a/drivers/vfio/pci/vfio_pci_private.h
> > > > +++ b/drivers/vfio/pci/vfio_pci_private.h
> > > > @@ -139,4 +139,5 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
> > > >  	return -ENODEV;
> > > >  }
> > > >  #endif
> > > > +extern int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu);
> > > >  #endif /* VFIO_PCI_PRIVATE_H */
> > > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > > > index d624a52..e93bf0c 100644
> > > > --- a/drivers/vfio/pci/vfio_pci.c
> > > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > > @@ -1236,6 +1236,20 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > >  		return ret;
> > > >  	}
> > > >  
> > > > +	if (pdev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET) {    
> > > 
> > > Perhaps a vfio_pci_is_virtio() like vga below?  Let's test the device
> > > ID range initially as well, this test raised a big red flag for me
> > > whether all devices within this vendor ID were virtio.
> > >   
> > > > +		bool noiommu = vfio_is_noiommu_group_dev(&pdev->dev);    
> > > 
> > > I think you can use iommu_present() for this and avoid patch 1of2.
> > > noiommu is mutually exclusive to an iommu being present.  Seems like
> > > all of this logic should be in the quirk itself, I'm not sure what it
> > > buys to get the value here but wait until later to use it.  Using
> > > iommu_present() could also move this test much earlier in
> > > vfio_pci_probe() making the exit path easier.  
> > 
> > Except then I'm reintroducing the bug fixed by 16ab8a5cbea4 since
> > iommu_present() assumes an IOMMU API based device.  I'll try to think if
> > there's another way to avoid adding the is_noiommu function.  Thanks,
> 
> I think something like this would do it.
> 
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1214,6 +1214,22 @@ static int vfio_pci_probe(struct pci_dev *pdev, const str
>         if (pdev->hdr_type != PCI_HEADER_TYPE_NORMAL)
>                 return -EINVAL;
>  
> +       /*
> +        * Filter out virtio devices that do not honor the iommu,
> +        * but only for real iommu groups.
> +        */
> +       if (vfio_pci_is_virtio(pdev)) {
> +               struct iommu_group *tmp = iommu_group_get(&pdev->dev);
> +
> +               if (tmp) {
> +                       iommu_group_put(tmp);
> +
> +                       ret = vfio_pci_virtio_quirk(pdev);
> +                       if (ret)
> +                               return ret;
> +               }
> +       }
> +
>         group = vfio_iommu_group_get(&pdev->dev);
>         if (!group)
>                 return -EINVAL;
> 
> Thanks,
> Alex

Yes but I think this will also prevent binding
a vfio-noiommu to this device.

Arguably this is a separate bug as it's already impossible ...
but now that we are disabling regular vfio the noiommu
fallback becomes more important.

Any hints on how to fix?



> > > > +
> > > > +		ret = vfio_pci_virtio_quirk(vdev, noiommu);
> > > > +		if (ret) {
> > > > +			dev_warn(&vdev->pdev->dev,
> > > > +				 "Failed to setup Virtio for VFIO\n");
> > > > +			vfio_del_group_dev(&pdev->dev);
> > > > +			vfio_iommu_group_put(group, &pdev->dev);
> > > > +			kfree(vdev);
> > > > +			return ret;
> > > > +		}
> > > > +	}
> > > > +
> > > >  	if (vfio_pci_is_vga(pdev)) {
> > > >  		vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> > > >  		vga_set_legacy_decoding(pdev,
> > > > diff --git a/drivers/vfio/pci/vfio_pci_virtio.c b/drivers/vfio/pci/vfio_pci_virtio.c
> > > > new file mode 100644
> > > > index 0000000..e1ecffd
> > > > --- /dev/null
> > > > +++ b/drivers/vfio/pci/vfio_pci_virtio.c
> > > > @@ -0,0 +1,140 @@
> > > > +/*
> > > > + * VFIO PCI Intel Graphics support    
> > >       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  
> > > > + *
> > > > + * Copyright (C) 2016 Red Hat, Inc.  All rights reserved.
> > > > + *	Author: Alex Williamson <alex.williamson@redhat.com>    
> > > 
> > > Update.
> > >   
> > > > + *
> > > > + * 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.
> > > > + *
> > > > + * Register a device specific region through which to provide read-only
> > > > + * access to the Intel IGD opregion.  The register defining the opregion
> > > > + * address is also virtualized to prevent user modification.
> > > > + */
> > > > +
> > > > +#include <linux/io.h>
> > > > +#include <linux/pci.h>
> > > > +#include <linux/uaccess.h>    
> > > 
> > > Are io.h and uaccess.h needed?
> > >   
> > > > +#include <linux/vfio.h>
> > > > +#include <linux/virtio_pci.h>
> > > > +#include <linux/virtio_config.h>
> > > > +
> > > > +#include "vfio_pci_private.h"
> > > > +
> > > > +/**
> > > > + * virtio_pci_find_capability - walk capabilities to find device info.
> > > > + * @dev: the pci device
> > > > + * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> > > > + *
> > > > + * Returns offset of the capability, or 0.
> > > > + */
> > > > +static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type)    
> > > 
> > > Does inlining this really make sense?
> > >   
> > > > +{
> > > > +	int pos;
> > > > +
> > > > +	for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> > > > +	     pos > 0;
> > > > +	     pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> > > > +		u8 type;
> > > > +		pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> > > > +							 cfg_type),
> > > > +				     &type);
> > > > +
> > > > +		if (type != cfg_type)
> > > > +			continue;
> > > > +
> > > > +		/* Ignore structures with reserved BAR values */
> > > > +		if (type != VIRTIO_PCI_CAP_PCI_CFG) {
> > > > +			u8 bar;
> > > > +
> > > > +			pci_read_config_byte(dev, pos +
> > > > +					     offsetof(struct virtio_pci_cap,
> > > > +						      bar),
> > > > +					     &bar);
> > > > +			if (bar > 0x5)    
> > > 
> > > s/0x5/PCI_STD_RESOURCE_END/
> > >   
> > > > +				continue;
> > > > +		}
> > > > +
> > > > +		return pos;
> > > > +	}
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +
> > > > +int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu)
> > > > +{
> > > > +	struct pci_dev *dev = vdev->pdev;    
> > > 
> > > Please use *pdev for consistency with the rest of drivers/vfio/pci/*
> > > 
> > > Also, is there any reason to pass the vfio_pci_device?  There's nothing
> > > vfio here otherwise and we could remove more #includes.
> > >   
> > > > +	int common, cfg;
> > > > +	u32 features;
> > > > +	u32 offset;
> > > > +	u8 bar;
> > > > +
> > > > +	/* Without an IOMMU, we don't care */
> > > > +	if (noiommu)
> > > > +		return 0;
> > > > +
> > > > +        /* Virtio only owns devices >= 0x1000 and <= 0x107f: leave the rest. */
> > > > +        if (dev->device < 0x1000 || dev->device > 0x107f)
> > > > +                return 0;    
> > > 
> > > Whitespace
> > >   
> > > > +
> > > > +	/* Check whether device enforces the IOMMU correctly */
> > > > +
> > > > +	/*
> > > > +	 * All modern devices must have common and cfg capabilities. We use cfg
> > > > +	 * capability for access so that we don't need to worry about resource
> > > > +	 * availability. Slow but sure.
> > > > +	 * Note that all vendor-specific fields we access are little-endian
> > > > +	 * which matches what pci config accessors expect, so they do byteswap
> > > > +	 * for us if appropriate.
> > > > +	 */
> > > > +	common = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG);
> > > > +	cfg = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_PCI_CFG);
> > > > +	if (!cfg || !common) {
> > > > +                dev_warn(&dev->dev,
> > > > +                         "Virtio device lacks common or pci cfg.\n");    
> > > 
> > > Whitespace
> > >   
> > > > +		return -ENODEV;
> > > > +	}
> > > > +
> > > > +	pci_read_config_byte(dev, common + offsetof(struct virtio_pci_cap,
> > > > +						    bar),
> > > > +			     &bar);
> > > > +	pci_read_config_dword(dev, common + offsetof(struct virtio_pci_cap,
> > > > +						    offset),
> > > > +			     &offset);
> > > > +
> > > > +	/* Program cfg capability for dword access into common cfg. */
> > > > +	pci_write_config_byte(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > +						  cap.bar),
> > > > +			      bar);
> > > > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > +						   cap.length),
> > > > +			       0x4);
> > > > +
> > > > +	/* Select features dword that has VIRTIO_F_IOMMU_PLATFORM. */
> > > > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > +						  cap.offset),
> > > > +			       offset + offsetof(struct virtio_pci_common_cfg,
> > > > +						 device_feature_select));
> > > > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > +						  pci_cfg_data),
> > > > +			       VIRTIO_F_IOMMU_PLATFORM / 32);
> > > > +
> > > > +	/* Get the features dword. */
> > > > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > +						  cap.offset),
> > > > +			       offset + offsetof(struct virtio_pci_common_cfg,
> > > > +						 device_feature));
> > > > +	pci_read_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > +						  pci_cfg_data),
> > > > +			      &features);
> > > > +
> > > > +	/* Does this device obey the platform's IOMMU? If not it's an error. */
> > > > +	if (!(features & (0x1 << (VIRTIO_F_IOMMU_PLATFORM % 32)))) {
> > > > +                dev_warn(&dev->dev,
> > > > +                         "Virtio device lacks VIRTIO_F_IOMMU_PLATFORM.\n");    
> > > 
> > > Whitespace
> > >   
> > > > +		return -ENODEV;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > > > index 76d8ec0..e9b20e7 100644
> > > > --- a/drivers/vfio/pci/Makefile
> > > > +++ b/drivers/vfio/pci/Makefile
> > > > @@ -1,5 +1,6 @@
> > > >  
> > > >  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > > > +vfio-pci-y += vfio_pci_virtio.o
> > > >  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > > >  
> > > >  obj-$(CONFIG_VFIO_PCI) += vfio-pci.o    
> > > 
> > > Thanks,
> > > Alex  

Re: [PATCH v2 2/2] vfio: add virtio pci quirk

[Alex Williamson]

On Tue, 30 Aug 2016 08:20:38 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Aug 29, 2016 at 10:53:04PM -0600, Alex Williamson wrote:
> > On Mon, 29 Aug 2016 21:52:20 -0600
> > Alex Williamson <alex.williamson@redhat.com> wrote:
> >   
> > > On Mon, 29 Aug 2016 21:23:25 -0600
> > > Alex Williamson <alex.williamson@redhat.com> wrote:
> > >   
> > > > On Tue, 30 Aug 2016 05:27:17 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >     
> > > > > Modern virtio pci devices can set VIRTIO_F_IOMMU_PLATFORM
> > > > > to signal they are safe to use with an IOMMU.
> > > > > 
> > > > > Without this bit, exposing the device to userspace is unsafe, so probe
> > > > > and fail VFIO initialization unless noiommu is enabled.
> > > > > 
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > >  drivers/vfio/pci/vfio_pci_private.h |   1 +
> > > > >  drivers/vfio/pci/vfio_pci.c         |  14 ++++
> > > > >  drivers/vfio/pci/vfio_pci_virtio.c  | 140 ++++++++++++++++++++++++++++++++++++
> > > > >  drivers/vfio/pci/Makefile           |   1 +
> > > > >  4 files changed, 156 insertions(+)
> > > > >  create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
> > > > > 
> > > > > diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> > > > > index 2128de8..2bd5616 100644
> > > > > --- a/drivers/vfio/pci/vfio_pci_private.h
> > > > > +++ b/drivers/vfio/pci/vfio_pci_private.h
> > > > > @@ -139,4 +139,5 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
> > > > >  	return -ENODEV;
> > > > >  }
> > > > >  #endif
> > > > > +extern int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu);
> > > > >  #endif /* VFIO_PCI_PRIVATE_H */
> > > > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > > > > index d624a52..e93bf0c 100644
> > > > > --- a/drivers/vfio/pci/vfio_pci.c
> > > > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > > > @@ -1236,6 +1236,20 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > > >  		return ret;
> > > > >  	}
> > > > >  
> > > > > +	if (pdev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET) {      
> > > > 
> > > > Perhaps a vfio_pci_is_virtio() like vga below?  Let's test the device
> > > > ID range initially as well, this test raised a big red flag for me
> > > > whether all devices within this vendor ID were virtio.
> > > >     
> > > > > +		bool noiommu = vfio_is_noiommu_group_dev(&pdev->dev);      
> > > > 
> > > > I think you can use iommu_present() for this and avoid patch 1of2.
> > > > noiommu is mutually exclusive to an iommu being present.  Seems like
> > > > all of this logic should be in the quirk itself, I'm not sure what it
> > > > buys to get the value here but wait until later to use it.  Using
> > > > iommu_present() could also move this test much earlier in
> > > > vfio_pci_probe() making the exit path easier.    
> > > 
> > > Except then I'm reintroducing the bug fixed by 16ab8a5cbea4 since
> > > iommu_present() assumes an IOMMU API based device.  I'll try to think if
> > > there's another way to avoid adding the is_noiommu function.  Thanks,  
> > 
> > I think something like this would do it.
> > 
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -1214,6 +1214,22 @@ static int vfio_pci_probe(struct pci_dev *pdev, const str
> >         if (pdev->hdr_type != PCI_HEADER_TYPE_NORMAL)
> >                 return -EINVAL;
> >  
> > +       /*
> > +        * Filter out virtio devices that do not honor the iommu,
> > +        * but only for real iommu groups.
> > +        */
> > +       if (vfio_pci_is_virtio(pdev)) {
> > +               struct iommu_group *tmp = iommu_group_get(&pdev->dev);
> > +
> > +               if (tmp) {
> > +                       iommu_group_put(tmp);
> > +
> > +                       ret = vfio_pci_virtio_quirk(pdev);
> > +                       if (ret)
> > +                               return ret;
> > +               }
> > +       }
> > +
> >         group = vfio_iommu_group_get(&pdev->dev);
> >         if (!group)
> >                 return -EINVAL;
> > 
> > Thanks,
> > Alex  
> 
> Yes but I think this will also prevent binding
> a vfio-noiommu to this device.
> 
> Arguably this is a separate bug as it's already impossible ...
> but now that we are disabling regular vfio the noiommu
> fallback becomes more important.
> 
> Any hints on how to fix?

I don't follow, prior to noiommu, we'd simply call iommu_group_get() and
rely on the iommu group created by the iommu driver.  Either there is
one and we allow binding to vfio-pci or there is not one and we would
fail.  With noiommu, vfio_iommu_group_get() wraps that call in some
logic that potentially creates an iommu group if noiommu is enabled.
So if we use the real iommu_group_get() prior to that and a group
exists, we know we're not using noiommu and we can test the virtio
device.  If the group doesn't exist here, noiommu plays out the same as
it would otherwise (depending on noiommu enabling), a virtio device can
be bound regardless of its capabilities and features.  I think that's
what we want, right?  Thanks,

Alex

> > > > > +
> > > > > +		ret = vfio_pci_virtio_quirk(vdev, noiommu);
> > > > > +		if (ret) {
> > > > > +			dev_warn(&vdev->pdev->dev,
> > > > > +				 "Failed to setup Virtio for VFIO\n");
> > > > > +			vfio_del_group_dev(&pdev->dev);
> > > > > +			vfio_iommu_group_put(group, &pdev->dev);
> > > > > +			kfree(vdev);
> > > > > +			return ret;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > >  	if (vfio_pci_is_vga(pdev)) {
> > > > >  		vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> > > > >  		vga_set_legacy_decoding(pdev,
> > > > > diff --git a/drivers/vfio/pci/vfio_pci_virtio.c b/drivers/vfio/pci/vfio_pci_virtio.c
> > > > > new file mode 100644
> > > > > index 0000000..e1ecffd
> > > > > --- /dev/null
> > > > > +++ b/drivers/vfio/pci/vfio_pci_virtio.c
> > > > > @@ -0,0 +1,140 @@
> > > > > +/*
> > > > > + * VFIO PCI Intel Graphics support      
> > > >       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^    
> > > > > + *
> > > > > + * Copyright (C) 2016 Red Hat, Inc.  All rights reserved.
> > > > > + *	Author: Alex Williamson <alex.williamson@redhat.com>      
> > > > 
> > > > Update.
> > > >     
> > > > > + *
> > > > > + * 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.
> > > > > + *
> > > > > + * Register a device specific region through which to provide read-only
> > > > > + * access to the Intel IGD opregion.  The register defining the opregion
> > > > > + * address is also virtualized to prevent user modification.
> > > > > + */
> > > > > +
> > > > > +#include <linux/io.h>
> > > > > +#include <linux/pci.h>
> > > > > +#include <linux/uaccess.h>      
> > > > 
> > > > Are io.h and uaccess.h needed?
> > > >     
> > > > > +#include <linux/vfio.h>
> > > > > +#include <linux/virtio_pci.h>
> > > > > +#include <linux/virtio_config.h>
> > > > > +
> > > > > +#include "vfio_pci_private.h"
> > > > > +
> > > > > +/**
> > > > > + * virtio_pci_find_capability - walk capabilities to find device info.
> > > > > + * @dev: the pci device
> > > > > + * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> > > > > + *
> > > > > + * Returns offset of the capability, or 0.
> > > > > + */
> > > > > +static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type)      
> > > > 
> > > > Does inlining this really make sense?
> > > >     
> > > > > +{
> > > > > +	int pos;
> > > > > +
> > > > > +	for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> > > > > +	     pos > 0;
> > > > > +	     pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> > > > > +		u8 type;
> > > > > +		pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> > > > > +							 cfg_type),
> > > > > +				     &type);
> > > > > +
> > > > > +		if (type != cfg_type)
> > > > > +			continue;
> > > > > +
> > > > > +		/* Ignore structures with reserved BAR values */
> > > > > +		if (type != VIRTIO_PCI_CAP_PCI_CFG) {
> > > > > +			u8 bar;
> > > > > +
> > > > > +			pci_read_config_byte(dev, pos +
> > > > > +					     offsetof(struct virtio_pci_cap,
> > > > > +						      bar),
> > > > > +					     &bar);
> > > > > +			if (bar > 0x5)      
> > > > 
> > > > s/0x5/PCI_STD_RESOURCE_END/
> > > >     
> > > > > +				continue;
> > > > > +		}
> > > > > +
> > > > > +		return pos;
> > > > > +	}
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +
> > > > > +int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu)
> > > > > +{
> > > > > +	struct pci_dev *dev = vdev->pdev;      
> > > > 
> > > > Please use *pdev for consistency with the rest of drivers/vfio/pci/*
> > > > 
> > > > Also, is there any reason to pass the vfio_pci_device?  There's nothing
> > > > vfio here otherwise and we could remove more #includes.
> > > >     
> > > > > +	int common, cfg;
> > > > > +	u32 features;
> > > > > +	u32 offset;
> > > > > +	u8 bar;
> > > > > +
> > > > > +	/* Without an IOMMU, we don't care */
> > > > > +	if (noiommu)
> > > > > +		return 0;
> > > > > +
> > > > > +        /* Virtio only owns devices >= 0x1000 and <= 0x107f: leave the rest. */
> > > > > +        if (dev->device < 0x1000 || dev->device > 0x107f)
> > > > > +                return 0;      
> > > > 
> > > > Whitespace
> > > >     
> > > > > +
> > > > > +	/* Check whether device enforces the IOMMU correctly */
> > > > > +
> > > > > +	/*
> > > > > +	 * All modern devices must have common and cfg capabilities. We use cfg
> > > > > +	 * capability for access so that we don't need to worry about resource
> > > > > +	 * availability. Slow but sure.
> > > > > +	 * Note that all vendor-specific fields we access are little-endian
> > > > > +	 * which matches what pci config accessors expect, so they do byteswap
> > > > > +	 * for us if appropriate.
> > > > > +	 */
> > > > > +	common = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG);
> > > > > +	cfg = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_PCI_CFG);
> > > > > +	if (!cfg || !common) {
> > > > > +                dev_warn(&dev->dev,
> > > > > +                         "Virtio device lacks common or pci cfg.\n");      
> > > > 
> > > > Whitespace
> > > >     
> > > > > +		return -ENODEV;
> > > > > +	}
> > > > > +
> > > > > +	pci_read_config_byte(dev, common + offsetof(struct virtio_pci_cap,
> > > > > +						    bar),
> > > > > +			     &bar);
> > > > > +	pci_read_config_dword(dev, common + offsetof(struct virtio_pci_cap,
> > > > > +						    offset),
> > > > > +			     &offset);
> > > > > +
> > > > > +	/* Program cfg capability for dword access into common cfg. */
> > > > > +	pci_write_config_byte(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > > +						  cap.bar),
> > > > > +			      bar);
> > > > > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > > +						   cap.length),
> > > > > +			       0x4);
> > > > > +
> > > > > +	/* Select features dword that has VIRTIO_F_IOMMU_PLATFORM. */
> > > > > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > > +						  cap.offset),
> > > > > +			       offset + offsetof(struct virtio_pci_common_cfg,
> > > > > +						 device_feature_select));
> > > > > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > > +						  pci_cfg_data),
> > > > > +			       VIRTIO_F_IOMMU_PLATFORM / 32);
> > > > > +
> > > > > +	/* Get the features dword. */
> > > > > +	pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > > +						  cap.offset),
> > > > > +			       offset + offsetof(struct virtio_pci_common_cfg,
> > > > > +						 device_feature));
> > > > > +	pci_read_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > > +						  pci_cfg_data),
> > > > > +			      &features);
> > > > > +
> > > > > +	/* Does this device obey the platform's IOMMU? If not it's an error. */
> > > > > +	if (!(features & (0x1 << (VIRTIO_F_IOMMU_PLATFORM % 32)))) {
> > > > > +                dev_warn(&dev->dev,
> > > > > +                         "Virtio device lacks VIRTIO_F_IOMMU_PLATFORM.\n");      
> > > > 
> > > > Whitespace
> > > >     
> > > > > +		return -ENODEV;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > > > > index 76d8ec0..e9b20e7 100644
> > > > > --- a/drivers/vfio/pci/Makefile
> > > > > +++ b/drivers/vfio/pci/Makefile
> > > > > @@ -1,5 +1,6 @@
> > > > >  
> > > > >  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > > > > +vfio-pci-y += vfio_pci_virtio.o
> > > > >  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > > > >  
> > > > >  obj-$(CONFIG_VFIO_PCI) += vfio-pci.o      
> > > > 
> > > > Thanks,
> > > > Alex    

Re: [PATCH v2 2/2] vfio: add virtio pci quirk

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

Hi Michael,

[auto build test ERROR on vfio/next]
[also build test ERROR on v4.8-rc4 next-20160825]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Michael-S-Tsirkin/vfio-blacklist-legacy-virtio-devices/20160830-124010
base:   https://github.com/awilliam/linux-vfio.git next
config: x86_64-rhel (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "vfio_is_noiommu_group_dev" [drivers/vfio/pci/vfio-pci.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 37603 bytes --]

Re: [Qemu-devel] [PATCH v2 0/2] vfio: blacklist legacy virtio devices

[Jason Wang]

On 2016年08月30日 10:27, Michael S. Tsirkin wrote:
> Legacy virtio devices always bypassed an IOMMU, so using them with vfio was
> never safe.

And it actually won't work since GPA is assumed in the device. So I'm 
not sure this is must since we should get a IOMMU fault in this case.

>   This adds a quirk detecting these and disabling VFIO unless the
> noiommu mode is used.  At the moment, this only applies to virtio-pci devices.
>
> The patch might make sense on stable as well.
>
> Michael S. Tsirkin (2):
>    vfio: report group noiommu status
>    vfio: add virtio pci quirk
>
>   drivers/vfio/pci/vfio_pci_private.h |   1 +
>   include/linux/vfio.h                |   2 +
>   drivers/vfio/pci/vfio_pci.c         |  14 ++++
>   drivers/vfio/pci/vfio_pci_virtio.c  | 140 ++++++++++++++++++++++++++++++++++++
>   drivers/vfio/vfio.c                 |  12 ++++
>   drivers/vfio/pci/Makefile           |   1 +
>   6 files changed, 170 insertions(+)
>   create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
>

Re: [Qemu-devel] [PATCH v2 0/2] vfio: blacklist legacy virtio devices

[Michael S. Tsirkin]

On Tue, Aug 30, 2016 at 11:16:25AM +0800, Jason Wang wrote:
> 
> 
> On 2016年08月30日 10:27, Michael S. Tsirkin wrote:
> > Legacy virtio devices always bypassed an IOMMU, so using them with vfio was
> > never safe.
> 
> And it actually won't work since GPA is assumed in the device. So I'm not
> sure this is must since we should get a IOMMU fault in this case.

We won't get an IOMMU fault for legacy systems since they
bypass the IOMMU. Instead guest userspace will get full
access to all of guest memory through the device.


> >   This adds a quirk detecting these and disabling VFIO unless the
> > noiommu mode is used.  At the moment, this only applies to virtio-pci devices.
> > 
> > The patch might make sense on stable as well.
> > 
> > Michael S. Tsirkin (2):
> >    vfio: report group noiommu status
> >    vfio: add virtio pci quirk
> > 
> >   drivers/vfio/pci/vfio_pci_private.h |   1 +
> >   include/linux/vfio.h                |   2 +
> >   drivers/vfio/pci/vfio_pci.c         |  14 ++++
> >   drivers/vfio/pci/vfio_pci_virtio.c  | 140 ++++++++++++++++++++++++++++++++++++
> >   drivers/vfio/vfio.c                 |  12 ++++
> >   drivers/vfio/pci/Makefile           |   1 +
> >   6 files changed, 170 insertions(+)
> >   create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
> >