Re: [Qemu-devel] [PATCH RFC 3/3] vfio: add virtio pci quirk
On Mon, Apr 18, 2016 at 02:00:06PM -0600, Alex Williamson wrote:
> On Mon, 18 Apr 2016 12:58:28 +0300
> "Michael S. Tsirkin" 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
> > ---
> > drivers/vfio/pci/vfio_pci_private.h | 1 +
> > drivers/vfio/pci/vfio_pci.c | 11 +++
> > drivers/vfio/pci/vfio_pci_virtio.c | 135
> >
> > drivers/vfio/pci/Makefile | 1 +
> > 4 files changed, 148 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 8a7d546..604d445 100644
> > --- a/drivers/vfio/pci/vfio_pci_private.h
> > +++ b/drivers/vfio/pci/vfio_pci_private.h
> > @@ -130,4 +130,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, int
> > noiommu);
> > #endif /* VFIO_PCI_PRIVATE_H */
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index d622a41..2bb8c76 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -1125,6 +1125,17 @@ 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 &&
>
> Virtio really owns this entire vendor ID block? Apparently nobody told
> ivshmem: http://pci-ids.ucw.cz/read/PC/1af4/1110 Even the comment by
> virtio_pci_id_table[] suggests virtio is only a subset even if the code
> doesn't appear to honor that comment. I don't know the history there,
> but that seems like really inefficient use of an entire, coveted vendor
> block.
True - virtio spec also says it's up to 0x107f.
> > + ((ret = vfio_pci_virtio_quirk(vdev, ret {
>
> Please don't set variables like this unless necessary.
>
> if (vendor...) {
>ret = vfio_pci_virtio_quir...
>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 000..1a32064
> > --- /dev/null
> > +++ b/drivers/vfio/pci/vfio_pci_virtio.c
> > @@ -0,0 +1,135 @@
> > +/*
> > + * VFIO PCI Intel Graphics support
> > + *
> > + * Copyright (C) 2016 Red Hat, Inc. All rights reserved.
> > + * Author: Alex Williamson
> > + *
>
> 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.
>
> Update
>
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> I don't see where io or uaccess are needed here.
>
> > +
> > +#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)
>
> This is called from probe code, why inline? There's already a function
> with this exact same name in virtio code, can we come up with something
> unique to avoid confusion?
>
> > +{
> > + 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_
Re: [Qemu-devel] [PATCH RFC 3/3] vfio: add virtio pci quirk
On Mon, 18 Apr 2016 12:58:28 +0300
"Michael S. Tsirkin" 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
> ---
> drivers/vfio/pci/vfio_pci_private.h | 1 +
> drivers/vfio/pci/vfio_pci.c | 11 +++
> drivers/vfio/pci/vfio_pci_virtio.c | 135
>
> drivers/vfio/pci/Makefile | 1 +
> 4 files changed, 148 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 8a7d546..604d445 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -130,4 +130,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, int noiommu);
> #endif /* VFIO_PCI_PRIVATE_H */
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index d622a41..2bb8c76 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1125,6 +1125,17 @@ 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 &&
Virtio really owns this entire vendor ID block? Apparently nobody told
ivshmem: http://pci-ids.ucw.cz/read/PC/1af4/1110 Even the comment by
virtio_pci_id_table[] suggests virtio is only a subset even if the code
doesn't appear to honor that comment. I don't know the history there,
but that seems like really inefficient use of an entire, coveted vendor
block.
> + ((ret = vfio_pci_virtio_quirk(vdev, ret {
Please don't set variables like this unless necessary.
if (vendor...) {
ret = vfio_pci_virtio_quir...
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 000..1a32064
> --- /dev/null
> +++ b/drivers/vfio/pci/vfio_pci_virtio.c
> @@ -0,0 +1,135 @@
> +/*
> + * VFIO PCI Intel Graphics support
> + *
> + * Copyright (C) 2016 Red Hat, Inc. All rights reserved.
> + * Author: Alex Williamson
> + *
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.
Update
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
I don't see where io or uaccess are needed here.
> +
> +#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)
This is called from probe code, why inline? There's already a function
with this exact same name in virtio code, can we come up with something
unique to avoid confusion?
> +{
> + 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 p
[Qemu-devel] [PATCH RFC 3/3] vfio: add virtio pci quirk
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
---
drivers/vfio/pci/vfio_pci_private.h | 1 +
drivers/vfio/pci/vfio_pci.c | 11 +++
drivers/vfio/pci/vfio_pci_virtio.c | 135
drivers/vfio/pci/Makefile | 1 +
4 files changed, 148 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 8a7d546..604d445 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -130,4 +130,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, int noiommu);
#endif /* VFIO_PCI_PRIVATE_H */
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index d622a41..2bb8c76 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1125,6 +1125,17 @@ 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 &&
+ ((ret = vfio_pci_virtio_quirk(vdev, 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 000..1a32064
--- /dev/null
+++ b/drivers/vfio/pci/vfio_pci_virtio.c
@@ -0,0 +1,135 @@
+/*
+ * VFIO PCI Intel Graphics support
+ *
+ * Copyright (C) 2016 Red Hat, Inc. All rights reserved.
+ * Author: Alex Williamson
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+
+#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, int 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;
+ /* 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 (!
