[PATCH v3 0/6] Add virtio-iommu built-in topology

[PATCH v3 0/6] Add virtio-iommu built-in topology

[Jean-Philippe Brucker]

Add a topology description to the virtio-iommu driver and enable x86
platforms.

Since [v2] we have made some progress on adding ACPI support for
virtio-iommu, which is the preferred boot method on x86. It will be a
new vendor-agnostic table describing para-virtual topologies in a
minimal format. However some platforms don't use either ACPI or DT for
booting (for example microvm), and will need the alternative topology
description method proposed here. In addition, since the process to get
a new ACPI table will take a long time, this provides a boot method even
to ACPI-based platforms, if only temporarily for testing and
development.

v3:
* Add patch 1 that moves virtio-iommu to a subfolder.
* Split the rest:
  * Patch 2 adds topology-helper.c, which will be shared with the ACPI
    support.
  * Patch 4 adds definitions.
  * Patch 5 adds parser in topology.c.
* Address other comments.

Linux and QEMU patches available at:
https://jpbrucker.net/git/linux virtio-iommu/devel
https://jpbrucker.net/git/qemu virtio-iommu/devel

[spec] https://lists.oasis-open.org/archives/virtio-dev/202008/msg00067.html
[v2] https://lore.kernel.org/linux-iommu/20200228172537.377327-1-jean-philippe@linaro.org/
[v1] https://lore.kernel.org/linux-iommu/20200214160413.1475396-1-jean-philippe@linaro.org/
[rfc] https://lore.kernel.org/linux-iommu/20191122105000.800410-1-jean-philippe@linaro.org/

Jean-Philippe Brucker (6):
  iommu/virtio: Move to drivers/iommu/virtio/
  iommu/virtio: Add topology helpers
  PCI: Add DMA configuration for virtual platforms
  iommu/virtio: Add topology definitions
  iommu/virtio: Support topology description in config space
  iommu/virtio: Enable x86 support

 drivers/iommu/Kconfig                     |  18 +-
 drivers/iommu/Makefile                    |   3 +-
 drivers/iommu/virtio/Makefile             |   4 +
 drivers/iommu/virtio/topology-helpers.h   |  50 +++++
 include/linux/virt_iommu.h                |  15 ++
 include/uapi/linux/virtio_iommu.h         |  44 ++++
 drivers/iommu/virtio/topology-helpers.c   | 196 ++++++++++++++++
 drivers/iommu/virtio/topology.c           | 259 ++++++++++++++++++++++
 drivers/iommu/{ => virtio}/virtio-iommu.c |   4 +
 drivers/pci/pci-driver.c                  |   5 +
 MAINTAINERS                               |   3 +-
 11 files changed, 597 insertions(+), 4 deletions(-)
 create mode 100644 drivers/iommu/virtio/Makefile
 create mode 100644 drivers/iommu/virtio/topology-helpers.h
 create mode 100644 include/linux/virt_iommu.h
 create mode 100644 drivers/iommu/virtio/topology-helpers.c
 create mode 100644 drivers/iommu/virtio/topology.c
 rename drivers/iommu/{ => virtio}/virtio-iommu.c (99%)

-- 
2.28.0

[PATCH v3 1/6] iommu/virtio: Move to drivers/iommu/virtio/

[Jean-Philippe Brucker]

Before adding new files to the virtio-iommu driver, move it to its own
subfolder, similarly to other IOMMU drivers.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/iommu/Makefile                    | 3 +--
 drivers/iommu/virtio/Makefile             | 2 ++
 drivers/iommu/{ => virtio}/virtio-iommu.c | 0
 MAINTAINERS                               | 2 +-
 4 files changed, 4 insertions(+), 3 deletions(-)
 create mode 100644 drivers/iommu/virtio/Makefile
 rename drivers/iommu/{ => virtio}/virtio-iommu.c (100%)

diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 11f1771104f3..fc7523042512 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-y += amd/ intel/ arm/
+obj-y += amd/ intel/ arm/ virtio/
 obj-$(CONFIG_IOMMU_API) += iommu.o
 obj-$(CONFIG_IOMMU_API) += iommu-traces.o
 obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
@@ -26,4 +26,3 @@ obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o
 obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o
 obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
 obj-$(CONFIG_HYPERV_IOMMU) += hyperv-iommu.o
-obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
diff --git a/drivers/iommu/virtio/Makefile b/drivers/iommu/virtio/Makefile
new file mode 100644
index 000000000000..279368fcc074
--- /dev/null
+++ b/drivers/iommu/virtio/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio/virtio-iommu.c
similarity index 100%
rename from drivers/iommu/virtio-iommu.c
rename to drivers/iommu/virtio/virtio-iommu.c
diff --git a/MAINTAINERS b/MAINTAINERS
index deaafb617361..3602b223c9b2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18451,7 +18451,7 @@ VIRTIO IOMMU DRIVER
 M:	Jean-Philippe Brucker <jean-philippe@linaro.org>
 L:	virtualization@lists.linux-foundation.org
 S:	Maintained
-F:	drivers/iommu/virtio-iommu.c
+F:	drivers/iommu/virtio/
 F:	include/uapi/linux/virtio_iommu.h
 
 VIRTIO MEM DRIVER
-- 
2.28.0

Re: [PATCH v3 1/6] iommu/virtio: Move to drivers/iommu/virtio/

[Auger Eric]

Hi Jean,

On 8/21/20 3:15 PM, Jean-Philippe Brucker wrote:
> Before adding new files to the virtio-iommu driver, move it to its own
> subfolder, similarly to other IOMMU drivers.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  drivers/iommu/Makefile                    | 3 +--
>  drivers/iommu/virtio/Makefile             | 2 ++
>  drivers/iommu/{ => virtio}/virtio-iommu.c | 0
>  MAINTAINERS                               | 2 +-
>  4 files changed, 4 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/iommu/virtio/Makefile
>  rename drivers/iommu/{ => virtio}/virtio-iommu.c (100%)
> 
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 11f1771104f3..fc7523042512 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -1,5 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0
> -obj-y += amd/ intel/ arm/
> +obj-y += amd/ intel/ arm/ virtio/
>  obj-$(CONFIG_IOMMU_API) += iommu.o
>  obj-$(CONFIG_IOMMU_API) += iommu-traces.o
>  obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
> @@ -26,4 +26,3 @@ obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o
>  obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o
>  obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
>  obj-$(CONFIG_HYPERV_IOMMU) += hyperv-iommu.o
> -obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
> diff --git a/drivers/iommu/virtio/Makefile b/drivers/iommu/virtio/Makefile
> new file mode 100644
> index 000000000000..279368fcc074
> --- /dev/null
> +++ b/drivers/iommu/virtio/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio/virtio-iommu.c
> similarity index 100%
> rename from drivers/iommu/virtio-iommu.c
> rename to drivers/iommu/virtio/virtio-iommu.c
> diff --git a/MAINTAINERS b/MAINTAINERS
> index deaafb617361..3602b223c9b2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18451,7 +18451,7 @@ VIRTIO IOMMU DRIVER
>  M:	Jean-Philippe Brucker <jean-philippe@linaro.org>
>  L:	virtualization@lists.linux-foundation.org
>  S:	Maintained
> -F:	drivers/iommu/virtio-iommu.c
> +F:	drivers/iommu/virtio/
>  F:	include/uapi/linux/virtio_iommu.h
not related to this patch but you may add an entry for
Documentation/devicetree/bindings/virtio/iommu.txt

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
>  
>  VIRTIO MEM DRIVER
> 

[PATCH v3 2/6] iommu/virtio: Add topology helpers

[Jean-Philippe Brucker]

To support topology description from ACPI and from the builtin
description, add helpers to keep track of I/O topology descriptors.

To ease re-use of the helpers by other drivers and future ACPI
extensions, use the "virt_" prefix rather than "virtio_" when naming
structs and functions.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/iommu/Kconfig                   |   3 +
 drivers/iommu/virtio/Makefile           |   1 +
 drivers/iommu/virtio/topology-helpers.h |  50 ++++++
 include/linux/virt_iommu.h              |  15 ++
 drivers/iommu/virtio/topology-helpers.c | 196 ++++++++++++++++++++++++
 drivers/iommu/virtio/virtio-iommu.c     |   4 +
 MAINTAINERS                             |   1 +
 7 files changed, 270 insertions(+)
 create mode 100644 drivers/iommu/virtio/topology-helpers.h
 create mode 100644 include/linux/virt_iommu.h
 create mode 100644 drivers/iommu/virtio/topology-helpers.c

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index bef5d75e306b..e29ae50f7100 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -391,4 +391,7 @@ config VIRTIO_IOMMU
 
 	  Say Y here if you intend to run this kernel as a guest.
 
+config VIRTIO_IOMMU_TOPOLOGY_HELPERS
+	bool
+
 endif # IOMMU_SUPPORT
diff --git a/drivers/iommu/virtio/Makefile b/drivers/iommu/virtio/Makefile
index 279368fcc074..b42ad47eac7e 100644
--- a/drivers/iommu/virtio/Makefile
+++ b/drivers/iommu/virtio/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
+obj-$(CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS) += topology-helpers.o
diff --git a/drivers/iommu/virtio/topology-helpers.h b/drivers/iommu/virtio/topology-helpers.h
new file mode 100644
index 000000000000..436ca6a900c5
--- /dev/null
+++ b/drivers/iommu/virtio/topology-helpers.h
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef TOPOLOGY_HELPERS_H_
+#define TOPOLOGY_HELPERS_H_
+
+#ifdef CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS
+
+/* Identify a device node in the topology */
+struct virt_topo_dev_id {
+	unsigned int			type;
+#define VIRT_TOPO_DEV_TYPE_PCI		1
+#define VIRT_TOPO_DEV_TYPE_MMIO		2
+	union {
+		/* PCI endpoint or range */
+		struct {
+			u16		segment;
+			u16		bdf_start;
+			u16		bdf_end;
+		};
+		/* MMIO region */
+		u64			base;
+	};
+};
+
+/* Specification of an IOMMU */
+struct virt_topo_iommu {
+	struct virt_topo_dev_id		dev_id;
+	struct device			*dev; /* transport device */
+	struct fwnode_handle		*fwnode;
+	struct iommu_ops		*ops;
+	struct list_head		list;
+};
+
+/* Specification of an endpoint */
+struct virt_topo_endpoint {
+	struct virt_topo_dev_id		dev_id;
+	u32				endpoint_id;
+	struct virt_topo_iommu		*viommu;
+	struct list_head		list;
+};
+
+void virt_topo_add_endpoint(struct virt_topo_endpoint *ep);
+void virt_topo_add_iommu(struct virt_topo_iommu *viommu);
+
+void virt_topo_set_iommu_ops(struct device *dev, struct iommu_ops *ops);
+
+#else /* !CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS */
+static inline void virt_topo_set_iommu_ops(struct device *dev, struct iommu_ops *ops)
+{ }
+#endif /* !CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS */
+#endif /* TOPOLOGY_HELPERS_H_ */
diff --git a/include/linux/virt_iommu.h b/include/linux/virt_iommu.h
new file mode 100644
index 000000000000..17d2bd4732e0
--- /dev/null
+++ b/include/linux/virt_iommu.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef VIRT_IOMMU_H_
+#define VIRT_IOMMU_H_
+
+#ifdef CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS
+int virt_dma_configure(struct device *dev);
+
+#else /* !CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS */
+static inline int virt_dma_configure(struct device *dev)
+{
+	/* Don't disturb the normal DMA configuration methods */
+	return 0;
+}
+#endif /* !CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS */
+#endif /* VIRT_IOMMU_H_ */
diff --git a/drivers/iommu/virtio/topology-helpers.c b/drivers/iommu/virtio/topology-helpers.c
new file mode 100644
index 000000000000..8815e3a5d431
--- /dev/null
+++ b/drivers/iommu/virtio/topology-helpers.c
@@ -0,0 +1,196 @@
+// SPDX-License-Identifier: GPL-2.0
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/dma-iommu.h>
+#include <linux/list.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/virt_iommu.h>
+
+#include "topology-helpers.h"
+
+static LIST_HEAD(viommus);
+static LIST_HEAD(pci_endpoints);
+static LIST_HEAD(mmio_endpoints);
+static DEFINE_MUTEX(viommus_lock);
+
+static bool virt_topo_device_match(struct device *dev,
+				   struct virt_topo_dev_id *id)
+{
+	if (id->type == VIRT_TOPO_DEV_TYPE_PCI && dev_is_pci(dev)) {
+		struct pci_dev *pdev = to_pci_dev(dev);
+		u16 dev_id = pci_dev_id(pdev);
+
+		return pci_domain_nr(pdev->bus) == id->segment &&
+			dev_id >= id->bdf_start &&
+			dev_id <= id->bdf_end;
+	} else if (id->type == VIRT_TOPO_DEV_TYPE_MMIO &&
+		   dev_is_platform(dev)) {
+		struct platform_device *plat_dev = to_platform_device(dev);
+		struct resource *mem;
+
+		mem = platform_get_resource(plat_dev, IORESOURCE_MEM, 0);
+		if (!mem)
+			return false;
+		return mem->start == id->base;
+	}
+	return false;
+}
+
+static const struct iommu_ops *virt_iommu_setup(struct device *dev)
+{
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct virt_topo_iommu *viommu = NULL;
+	struct virt_topo_endpoint *ep;
+	struct pci_dev *pci_dev = NULL;
+	u32 epid;
+	int ret;
+
+	/* Already translated? */
+	if (fwspec && fwspec->ops)
+		return NULL;
+
+	mutex_lock(&viommus_lock);
+	if (dev_is_pci(dev)) {
+		pci_dev = to_pci_dev(dev);
+		list_for_each_entry(ep, &pci_endpoints, list) {
+			if (virt_topo_device_match(dev, &ep->dev_id)) {
+				epid = pci_dev_id(pci_dev) -
+					ep->dev_id.bdf_start +
+					ep->endpoint_id;
+				viommu = ep->viommu;
+				break;
+			}
+		}
+	} else if (dev_is_platform(dev)) {
+		list_for_each_entry(ep, &mmio_endpoints, list) {
+			if (virt_topo_device_match(dev, &ep->dev_id)) {
+				epid = ep->endpoint_id;
+				viommu = ep->viommu;
+				break;
+			}
+		}
+	}
+	mutex_unlock(&viommus_lock);
+	if (!viommu)
+		return NULL;
+
+	/* We're not translating ourselves. */
+	if (virt_topo_device_match(dev, &viommu->dev_id) ||
+	    dev == viommu->dev)
+		return NULL;
+
+	/*
+	 * If we found a PCI range managed by the viommu, we're the one that has
+	 * to request ACS.
+	 */
+	if (pci_dev)
+		pci_request_acs();
+
+	if (!viommu->ops)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	ret = iommu_fwspec_init(dev, viommu->fwnode, viommu->ops);
+	if (ret)
+		return ERR_PTR(ret);
+
+	iommu_fwspec_add_ids(dev, &epid, 1);
+
+	return viommu->ops;
+}
+
+/**
+ * virt_topo_add_endpoint - Register endpoint specification
+ * @ep: the endpoint specification
+ */
+void virt_topo_add_endpoint(struct virt_topo_endpoint *ep)
+{
+	mutex_lock(&viommus_lock);
+	list_add(&ep->list,
+		 ep->dev_id.type == VIRT_TOPO_DEV_TYPE_MMIO ?
+		 &mmio_endpoints : &pci_endpoints);
+	mutex_unlock(&viommus_lock);
+}
+
+/**
+ * virt_topo_add_iommu - Register IOMMU specification
+ * @viommu: the IOMMU specification
+ */
+void virt_topo_add_iommu(struct virt_topo_iommu *viommu)
+{
+	mutex_lock(&viommus_lock);
+	list_add(&viommu->list, &viommus);
+	mutex_unlock(&viommus_lock);
+}
+
+/**
+ * virt_dma_configure - Configure DMA of virtualized devices
+ * @dev: the endpoint
+ *
+ * Setup the DMA and IOMMU ops of a virtual device, for platforms without DT or
+ * ACPI.
+ *
+ * Return: -EPROBE_DEFER if the device is managed by an IOMMU that hasn't been
+ *   probed yet, 0 otherwise
+ */
+int virt_dma_configure(struct device *dev)
+{
+	const struct iommu_ops *iommu_ops;
+
+	iommu_ops = virt_iommu_setup(dev);
+	if (IS_ERR_OR_NULL(iommu_ops)) {
+		int ret = PTR_ERR(iommu_ops);
+
+		if (ret == -EPROBE_DEFER || ret == 0)
+			return ret;
+		dev_err(dev, "error %d while setting up virt IOMMU\n", ret);
+		return 0;
+	}
+
+	/*
+	 * If we have reason to believe the IOMMU driver missed the initial
+	 * add_device callback for dev, replay it to get things in order.
+	 */
+	if (dev->bus && !device_iommu_mapped(dev))
+		iommu_probe_device(dev);
+
+	/* Assume coherent, as well as full 64-bit addresses. */
+#ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS
+	arch_setup_dma_ops(dev, 0, ~0ULL, iommu_ops, true);
+#else
+	iommu_setup_dma_ops(dev, 0, ~0ULL);
+#endif
+	return 0;
+}
+
+/**
+ * virt_topo_set_iommu_ops - Set the IOMMU ops of a virtual IOMMU device
+ * @dev: the IOMMU device (transport)
+ * @ops: the new IOMMU ops or NULL
+ *
+ * Setup the iommu_ops associated to an IOMMU, once the driver is loaded
+ * and the device probed.
+ */
+void virt_topo_set_iommu_ops(struct device *dev, struct iommu_ops *ops)
+{
+	struct virt_topo_iommu *viommu;
+
+	mutex_lock(&viommus_lock);
+	list_for_each_entry(viommu, &viommus, list) {
+		/*
+		 * In case the topology driver didn't have a dev handle when
+		 * registering the topology, add it now.
+		 */
+		if (!viommu->dev &&
+		    virt_topo_device_match(dev, &viommu->dev_id))
+			viommu->dev = dev;
+
+		if (viommu->dev == dev) {
+			viommu->ops = ops;
+			viommu->fwnode = ops ? dev->fwnode : NULL;
+			break;
+		}
+	}
+	mutex_unlock(&viommus_lock);
+}
+EXPORT_SYMBOL_GPL(virt_topo_set_iommu_ops);
diff --git a/drivers/iommu/virtio/virtio-iommu.c b/drivers/iommu/virtio/virtio-iommu.c
index b4da396cce60..b371d15f837f 100644
--- a/drivers/iommu/virtio/virtio-iommu.c
+++ b/drivers/iommu/virtio/virtio-iommu.c
@@ -25,6 +25,8 @@
 
 #include <uapi/linux/virtio_iommu.h>
 
+#include "topology-helpers.h"
+
 #define MSI_IOVA_BASE			0x8000000
 #define MSI_IOVA_LENGTH			0x100000
 
@@ -1065,6 +1067,7 @@ static int viommu_probe(struct virtio_device *vdev)
 	if (ret)
 		goto err_free_vqs;
 
+	virt_topo_set_iommu_ops(dev->parent, &viommu_ops);
 	iommu_device_set_ops(&viommu->iommu, &viommu_ops);
 	iommu_device_set_fwnode(&viommu->iommu, parent_dev->fwnode);
 
@@ -1111,6 +1114,7 @@ static void viommu_remove(struct virtio_device *vdev)
 {
 	struct viommu_dev *viommu = vdev->priv;
 
+	virt_topo_set_iommu_ops(vdev->dev.parent, NULL);
 	iommu_device_sysfs_remove(&viommu->iommu);
 	iommu_device_unregister(&viommu->iommu);
 
diff --git a/MAINTAINERS b/MAINTAINERS
index 3602b223c9b2..8fd53c22a0ab 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18452,6 +18452,7 @@ M:	Jean-Philippe Brucker <jean-philippe@linaro.org>
 L:	virtualization@lists.linux-foundation.org
 S:	Maintained
 F:	drivers/iommu/virtio/
+F:	include/linux/virt_iommu.h
 F:	include/uapi/linux/virtio_iommu.h
 
 VIRTIO MEM DRIVER
-- 
2.28.0

Re: [PATCH v3 2/6] iommu/virtio: Add topology helpers

[Auger Eric]

Hi Jean,

On 8/21/20 3:15 PM, Jean-Philippe Brucker wrote:
> To support topology description from ACPI and from the builtin
> description, add helpers to keep track of I/O topology descriptors.
> 
> To ease re-use of the helpers by other drivers and future ACPI
> extensions, use the "virt_" prefix rather than "virtio_" when naming
> structs and functions.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  drivers/iommu/Kconfig                   |   3 +
>  drivers/iommu/virtio/Makefile           |   1 +
>  drivers/iommu/virtio/topology-helpers.h |  50 ++++++
>  include/linux/virt_iommu.h              |  15 ++
>  drivers/iommu/virtio/topology-helpers.c | 196 ++++++++++++++++++++++++
>  drivers/iommu/virtio/virtio-iommu.c     |   4 +
>  MAINTAINERS                             |   1 +
>  7 files changed, 270 insertions(+)
>  create mode 100644 drivers/iommu/virtio/topology-helpers.h
>  create mode 100644 include/linux/virt_iommu.h
>  create mode 100644 drivers/iommu/virtio/topology-helpers.c
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index bef5d75e306b..e29ae50f7100 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -391,4 +391,7 @@ config VIRTIO_IOMMU
>  
>  	  Say Y here if you intend to run this kernel as a guest.
>  
> +config VIRTIO_IOMMU_TOPOLOGY_HELPERS
> +	bool
> +
>  endif # IOMMU_SUPPORT
> diff --git a/drivers/iommu/virtio/Makefile b/drivers/iommu/virtio/Makefile
> index 279368fcc074..b42ad47eac7e 100644
> --- a/drivers/iommu/virtio/Makefile
> +++ b/drivers/iommu/virtio/Makefile
> @@ -1,2 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
> +obj-$(CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS) += topology-helpers.o
> diff --git a/drivers/iommu/virtio/topology-helpers.h b/drivers/iommu/virtio/topology-helpers.h
> new file mode 100644
> index 000000000000..436ca6a900c5
> --- /dev/null
> +++ b/drivers/iommu/virtio/topology-helpers.h
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef TOPOLOGY_HELPERS_H_
> +#define TOPOLOGY_HELPERS_H_
> +
> +#ifdef CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS
> +
> +/* Identify a device node in the topology */
> +struct virt_topo_dev_id {
> +	unsigned int			type;
> +#define VIRT_TOPO_DEV_TYPE_PCI		1
> +#define VIRT_TOPO_DEV_TYPE_MMIO		2
> +	union {
> +		/* PCI endpoint or range */
> +		struct {
> +			u16		segment;
> +			u16		bdf_start;
> +			u16		bdf_end;
> +		};
> +		/* MMIO region */
> +		u64			base;
> +	};
> +};
> +
> +/* Specification of an IOMMU */
> +struct virt_topo_iommu {
> +	struct virt_topo_dev_id		dev_id;
> +	struct device			*dev; /* transport device */
> +	struct fwnode_handle		*fwnode;
> +	struct iommu_ops		*ops;
> +	struct list_head		list;
> +};
> +
> +/* Specification of an endpoint */
> +struct virt_topo_endpoint {
> +	struct virt_topo_dev_id		dev_id;
> +	u32				endpoint_id;
> +	struct virt_topo_iommu		*viommu;
> +	struct list_head		list;
> +};
> +
> +void virt_topo_add_endpoint(struct virt_topo_endpoint *ep);
> +void virt_topo_add_iommu(struct virt_topo_iommu *viommu);
> +
> +void virt_topo_set_iommu_ops(struct device *dev, struct iommu_ops *ops);
> +
> +#else /* !CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS */
> +static inline void virt_topo_set_iommu_ops(struct device *dev, struct iommu_ops *ops)
> +{ }
> +#endif /* !CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS */
> +#endif /* TOPOLOGY_HELPERS_H_ */
> diff --git a/include/linux/virt_iommu.h b/include/linux/virt_iommu.h
> new file mode 100644
> index 000000000000..17d2bd4732e0
> --- /dev/null
> +++ b/include/linux/virt_iommu.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef VIRT_IOMMU_H_
> +#define VIRT_IOMMU_H_
> +
> +#ifdef CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS
> +int virt_dma_configure(struct device *dev);
> +
> +#else /* !CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS */
> +static inline int virt_dma_configure(struct device *dev)
> +{
> +	/* Don't disturb the normal DMA configuration methods */
> +	return 0;
> +}
> +#endif /* !CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS */
> +#endif /* VIRT_IOMMU_H_ */
> diff --git a/drivers/iommu/virtio/topology-helpers.c b/drivers/iommu/virtio/topology-helpers.c
> new file mode 100644
> index 000000000000..8815e3a5d431
> --- /dev/null
> +++ b/drivers/iommu/virtio/topology-helpers.c
> @@ -0,0 +1,196 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/dma-iommu.h>
> +#include <linux/list.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/virt_iommu.h>
> +
> +#include "topology-helpers.h"
> +
> +static LIST_HEAD(viommus);
> +static LIST_HEAD(pci_endpoints);
> +static LIST_HEAD(mmio_endpoints);
> +static DEFINE_MUTEX(viommus_lock);
> +
> +static bool virt_topo_device_match(struct device *dev,
> +				   struct virt_topo_dev_id *id)
> +{
> +	if (id->type == VIRT_TOPO_DEV_TYPE_PCI && dev_is_pci(dev)) {
> +		struct pci_dev *pdev = to_pci_dev(dev);
> +		u16 dev_id = pci_dev_id(pdev);
> +
> +		return pci_domain_nr(pdev->bus) == id->segment &&
> +			dev_id >= id->bdf_start &&
> +			dev_id <= id->bdf_end;
> +	} else if (id->type == VIRT_TOPO_DEV_TYPE_MMIO &&
> +		   dev_is_platform(dev)) {
> +		struct platform_device *plat_dev = to_platform_device(dev);
> +		struct resource *mem;
> +
> +		mem = platform_get_resource(plat_dev, IORESOURCE_MEM, 0);
> +		if (!mem)
> +			return false;
> +		return mem->start == id->base;
> +	}
> +	return false;
> +}
> +
> +static const struct iommu_ops *virt_iommu_setup(struct device *dev)
> +{
> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> +	struct virt_topo_iommu *viommu = NULL;
> +	struct virt_topo_endpoint *ep;
> +	struct pci_dev *pci_dev = NULL;
> +	u32 epid;
> +	int ret;
> +
> +	/* Already translated? */
> +	if (fwspec && fwspec->ops)
> +		return NULL;
> +
> +	mutex_lock(&viommus_lock);
> +	if (dev_is_pci(dev)) {
> +		pci_dev = to_pci_dev(dev);
> +		list_for_each_entry(ep, &pci_endpoints, list) {
> +			if (virt_topo_device_match(dev, &ep->dev_id)) {
> +				epid = pci_dev_id(pci_dev) -
> +					ep->dev_id.bdf_start +
> +					ep->endpoint_id;
> +				viommu = ep->viommu;
> +				break;
> +			}
> +		}
> +	} else if (dev_is_platform(dev)) {
> +		list_for_each_entry(ep, &mmio_endpoints, list) {
> +			if (virt_topo_device_match(dev, &ep->dev_id)) {
> +				epid = ep->endpoint_id;
> +				viommu = ep->viommu;
> +				break;
> +			}
> +		}
> +	}
> +	mutex_unlock(&viommus_lock);
> +	if (!viommu)
> +		return NULL;
> +
> +	/* We're not translating ourselves. */
> +	if (virt_topo_device_match(dev, &viommu->dev_id) ||
> +	    dev == viommu->dev)
> +		return NULL;
> +
> +	/*
> +	 * If we found a PCI range managed by the viommu, we're the one that has
> +	 * to request ACS.
> +	 */
> +	if (pci_dev)
> +		pci_request_acs();
> +
> +	if (!viommu->ops)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	ret = iommu_fwspec_init(dev, viommu->fwnode, viommu->ops);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	iommu_fwspec_add_ids(dev, &epid, 1);
> +
> +	return viommu->ops;
> +}
> +
> +/**
> + * virt_topo_add_endpoint - Register endpoint specification
> + * @ep: the endpoint specification
> + */
> +void virt_topo_add_endpoint(struct virt_topo_endpoint *ep)
> +{
> +	mutex_lock(&viommus_lock);
> +	list_add(&ep->list,
> +		 ep->dev_id.type == VIRT_TOPO_DEV_TYPE_MMIO ?
> +		 &mmio_endpoints : &pci_endpoints);
> +	mutex_unlock(&viommus_lock);
> +}
> +
> +/**
> + * virt_topo_add_iommu - Register IOMMU specification
> + * @viommu: the IOMMU specification
> + */
> +void virt_topo_add_iommu(struct virt_topo_iommu *viommu)
> +{
> +	mutex_lock(&viommus_lock);
> +	list_add(&viommu->list, &viommus);
> +	mutex_unlock(&viommus_lock);
> +}
> +
> +/**
> + * virt_dma_configure - Configure DMA of virtualized devices
> + * @dev: the endpoint
> + *
> + * Setup the DMA and IOMMU ops of a virtual device, for platforms without DT or
> + * ACPI.
> + *
> + * Return: -EPROBE_DEFER if the device is managed by an IOMMU that hasn't been
> + *   probed yet, 0 otherwise
> + */
> +int virt_dma_configure(struct device *dev)
> +{
> +	const struct iommu_ops *iommu_ops;
> +
> +	iommu_ops = virt_iommu_setup(dev);
> +	if (IS_ERR_OR_NULL(iommu_ops)) {
> +		int ret = PTR_ERR(iommu_ops);
> +
> +		if (ret == -EPROBE_DEFER || ret == 0)
> +			return ret;
> +		dev_err(dev, "error %d while setting up virt IOMMU\n", ret);
> +		return 0;
why do we return 0 here?
> +	}
> +
> +	/*
> +	 * If we have reason to believe the IOMMU driver missed the initial
> +	 * add_device callback for dev, replay it to get things in order.
> +	 */
> +	if (dev->bus && !device_iommu_mapped(dev))
> +		iommu_probe_device(dev);
> +
> +	/* Assume coherent, as well as full 64-bit addresses. */
> +#ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS
> +	arch_setup_dma_ops(dev, 0, ~0ULL, iommu_ops, true);
> +#else
> +	iommu_setup_dma_ops(dev, 0, ~0ULL);
> +#endif
> +	return 0;
> +}
> +
> +/**
> + * virt_topo_set_iommu_ops - Set the IOMMU ops of a virtual IOMMU device
> + * @dev: the IOMMU device (transport)
> + * @ops: the new IOMMU ops or NULL
> + *
> + * Setup the iommu_ops associated to an IOMMU, once the driver is loaded
> + * and the device probed.
> + */
> +void virt_topo_set_iommu_ops(struct device *dev, struct iommu_ops *ops)
> +{
> +	struct virt_topo_iommu *viommu;
> +
> +	mutex_lock(&viommus_lock);
> +	list_for_each_entry(viommu, &viommus, list) {
> +		/*
> +		 * In case the topology driver didn't have a dev handle when
> +		 * registering the topology, add it now.
> +		 */
> +		if (!viommu->dev &&
> +		    virt_topo_device_match(dev, &viommu->dev_id))
> +			viommu->dev = dev;
> +
> +		if (viommu->dev == dev) {
> +			viommu->ops = ops;
> +			viommu->fwnode = ops ? dev->fwnode : NULL;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&viommus_lock);
> +}
> +EXPORT_SYMBOL_GPL(virt_topo_set_iommu_ops);
> diff --git a/drivers/iommu/virtio/virtio-iommu.c b/drivers/iommu/virtio/virtio-iommu.c
> index b4da396cce60..b371d15f837f 100644
> --- a/drivers/iommu/virtio/virtio-iommu.c
> +++ b/drivers/iommu/virtio/virtio-iommu.c
> @@ -25,6 +25,8 @@
>  
>  #include <uapi/linux/virtio_iommu.h>
>  
> +#include "topology-helpers.h"
> +
>  #define MSI_IOVA_BASE			0x8000000
>  #define MSI_IOVA_LENGTH			0x100000
>  
> @@ -1065,6 +1067,7 @@ static int viommu_probe(struct virtio_device *vdev)
>  	if (ret)
>  		goto err_free_vqs;
>  
> +	virt_topo_set_iommu_ops(dev->parent, &viommu_ops);
>  	iommu_device_set_ops(&viommu->iommu, &viommu_ops);
>  	iommu_device_set_fwnode(&viommu->iommu, parent_dev->fwnode);
>  
> @@ -1111,6 +1114,7 @@ static void viommu_remove(struct virtio_device *vdev)
>  {
>  	struct viommu_dev *viommu = vdev->priv;
>  
> +	virt_topo_set_iommu_ops(vdev->dev.parent, NULL);
>  	iommu_device_sysfs_remove(&viommu->iommu);
>  	iommu_device_unregister(&viommu->iommu);
>  
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3602b223c9b2..8fd53c22a0ab 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18452,6 +18452,7 @@ M:	Jean-Philippe Brucker <jean-philippe@linaro.org>
>  L:	virtualization@lists.linux-foundation.org
>  S:	Maintained
>  F:	drivers/iommu/virtio/
> +F:	include/linux/virt_iommu.h
>  F:	include/uapi/linux/virtio_iommu.h
>  
>  VIRTIO MEM DRIVER
> 
Besides

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

Re: [PATCH v3 2/6] iommu/virtio: Add topology helpers

[Jean-Philippe Brucker]

On Fri, Sep 04, 2020 at 06:22:12PM +0200, Auger Eric wrote:
> > +/**
> > + * virt_dma_configure - Configure DMA of virtualized devices
> > + * @dev: the endpoint
> > + *
> > + * Setup the DMA and IOMMU ops of a virtual device, for platforms without DT or
> > + * ACPI.
> > + *
> > + * Return: -EPROBE_DEFER if the device is managed by an IOMMU that hasn't been
> > + *   probed yet, 0 otherwise
> > + */
> > +int virt_dma_configure(struct device *dev)
> > +{
> > +	const struct iommu_ops *iommu_ops;
> > +
> > +	iommu_ops = virt_iommu_setup(dev);
> > +	if (IS_ERR_OR_NULL(iommu_ops)) {
> > +		int ret = PTR_ERR(iommu_ops);
> > +
> > +		if (ret == -EPROBE_DEFER || ret == 0)
> > +			return ret;
> > +		dev_err(dev, "error %d while setting up virt IOMMU\n", ret);
> > +		return 0;
> why do we return 0 here?

So we can fall back to another method (ACPI or DT) if available

> Besides
> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks!
Jean

[PATCH v3 3/6] PCI: Add DMA configuration for virtual platforms

[Jean-Philippe Brucker]

Hardware platforms usually describe the IOMMU topology using either
device-tree pointers or vendor-specific ACPI tables.  For virtual
platforms that don't provide a device-tree, the virtio-iommu device
contains a description of the endpoints it manages.  That information
allows us to probe endpoints after the IOMMU is probed (possibly as late
as userspace modprobe), provided it is discovered early enough.

Add a hook to pci_dma_configure(), which returns -EPROBE_DEFER if the
endpoint is managed by a vIOMMU that will be loaded later, or 0 in any
other case to avoid disturbing the normal DMA configuration methods.
When CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS isn't selected, the call to
virt_dma_configure() is compiled out.

As long as the information is consistent, platforms can provide both a
device-tree and a built-in topology, and the IOMMU infrastructure is
able to deal with multiple DMA configuration methods.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/pci/pci-driver.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 449466f71040..dbe9d33606b0 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -19,6 +19,7 @@
 #include <linux/kexec.h>
 #include <linux/of_device.h>
 #include <linux/acpi.h>
+#include <linux/virt_iommu.h>
 #include "pci.h"
 #include "pcie/portdrv.h"
 
@@ -1605,6 +1606,10 @@ static int pci_dma_configure(struct device *dev)
 	struct device *bridge;
 	int ret = 0;
 
+	ret = virt_dma_configure(dev);
+	if (ret)
+		return ret;
+
 	bridge = pci_get_host_bridge_device(to_pci_dev(dev));
 
 	if (IS_ENABLED(CONFIG_OF) && bridge->parent &&
-- 
2.28.0

[PATCH v3 4/6] iommu/virtio: Add topology definitions

[Jean-Philippe Brucker]

Add struct definitions for describing endpoints managed by the
virtio-iommu. When VIRTIO_IOMMU_F_TOPOLOGY is offered, an array of
virtio_iommu_topo_* structures in config space describes the endpoints,
identified either by their PCI BDF or their physical MMIO address.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 include/uapi/linux/virtio_iommu.h | 44 +++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
index 237e36a280cb..70cba30644d5 100644
--- a/include/uapi/linux/virtio_iommu.h
+++ b/include/uapi/linux/virtio_iommu.h
@@ -16,6 +16,7 @@
 #define VIRTIO_IOMMU_F_BYPASS			3
 #define VIRTIO_IOMMU_F_PROBE			4
 #define VIRTIO_IOMMU_F_MMIO			5
+#define VIRTIO_IOMMU_F_TOPOLOGY			6
 
 struct virtio_iommu_range_64 {
 	__le64					start;
@@ -27,6 +28,17 @@ struct virtio_iommu_range_32 {
 	__le32					end;
 };
 
+struct virtio_iommu_topo_config {
+	/* Number of topology description structures */
+	__le16					count;
+	/*
+	 * Offset to the first topology description structure
+	 * (virtio_iommu_topo_*) from the start of the virtio_iommu config
+	 * space. Aligned on 8 bytes.
+	 */
+	__le16					offset;
+};
+
 struct virtio_iommu_config {
 	/* Supported page sizes */
 	__le64					page_size_mask;
@@ -36,6 +48,38 @@ struct virtio_iommu_config {
 	struct virtio_iommu_range_32		domain_range;
 	/* Probe buffer size */
 	__le32					probe_size;
+	struct virtio_iommu_topo_config		topo_config;
+};
+
+#define VIRTIO_IOMMU_TOPO_PCI_RANGE		0x1
+#define VIRTIO_IOMMU_TOPO_MMIO			0x2
+
+struct virtio_iommu_topo_pci_range {
+	/* VIRTIO_IOMMU_TOPO_PCI_RANGE */
+	__u8					type;
+	__u8					reserved;
+	/* Length of this structure */
+	__le16					length;
+	/* First endpoint ID in the range */
+	__le32					endpoint_start;
+	/* PCI domain number */
+	__le16					segment;
+	/* PCI Bus:Device.Function range */
+	__le16					bdf_start;
+	__le16					bdf_end;
+	__le16					padding;
+};
+
+struct virtio_iommu_topo_mmio {
+	/* VIRTIO_IOMMU_TOPO_MMIO */
+	__u8					type;
+	__u8					reserved;
+	/* Length of this structure */
+	__le16					length;
+	/* Endpoint ID */
+	__le32					endpoint;
+	/* Address of the first MMIO region */
+	__le64					address;
 };
 
 /* Request types */
-- 
2.28.0

Re: [PATCH v3 4/6] iommu/virtio: Add topology definitions

[Auger Eric]

Hi Jean,

On 8/21/20 3:15 PM, Jean-Philippe Brucker wrote:
> Add struct definitions for describing endpoints managed by the
> virtio-iommu. When VIRTIO_IOMMU_F_TOPOLOGY is offered, an array of
> virtio_iommu_topo_* structures in config space describes the endpoints,
> identified either by their PCI BDF or their physical MMIO address.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> ---
>  include/uapi/linux/virtio_iommu.h | 44 +++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
> index 237e36a280cb..70cba30644d5 100644
> --- a/include/uapi/linux/virtio_iommu.h
> +++ b/include/uapi/linux/virtio_iommu.h
> @@ -16,6 +16,7 @@
>  #define VIRTIO_IOMMU_F_BYPASS			3
>  #define VIRTIO_IOMMU_F_PROBE			4
>  #define VIRTIO_IOMMU_F_MMIO			5
> +#define VIRTIO_IOMMU_F_TOPOLOGY			6
>  
>  struct virtio_iommu_range_64 {
>  	__le64					start;
> @@ -27,6 +28,17 @@ struct virtio_iommu_range_32 {
>  	__le32					end;
>  };
>  
> +struct virtio_iommu_topo_config {
> +	/* Number of topology description structures */
> +	__le16					count;
> +	/*
> +	 * Offset to the first topology description structure
> +	 * (virtio_iommu_topo_*) from the start of the virtio_iommu config
> +	 * space. Aligned on 8 bytes.
> +	 */
> +	__le16					offset;
> +};
> +
>  struct virtio_iommu_config {
>  	/* Supported page sizes */
>  	__le64					page_size_mask;
> @@ -36,6 +48,38 @@ struct virtio_iommu_config {
>  	struct virtio_iommu_range_32		domain_range;
>  	/* Probe buffer size */
>  	__le32					probe_size;
> +	struct virtio_iommu_topo_config		topo_config;
> +};
> +
> +#define VIRTIO_IOMMU_TOPO_PCI_RANGE		0x1
> +#define VIRTIO_IOMMU_TOPO_MMIO			0x2
> +
> +struct virtio_iommu_topo_pci_range {
> +	/* VIRTIO_IOMMU_TOPO_PCI_RANGE */
> +	__u8					type;
> +	__u8					reserved;
> +	/* Length of this structure */
> +	__le16					length;
> +	/* First endpoint ID in the range */
> +	__le32					endpoint_start;
> +	/* PCI domain number */
> +	__le16					segment;
> +	/* PCI Bus:Device.Function range */
> +	__le16					bdf_start;
> +	__le16					bdf_end;
> +	__le16					padding;
> +};
> +
> +struct virtio_iommu_topo_mmio {
> +	/* VIRTIO_IOMMU_TOPO_MMIO */
> +	__u8					type;
> +	__u8					reserved;
> +	/* Length of this structure */
> +	__le16					length;
> +	/* Endpoint ID */
> +	__le32					endpoint;
> +	/* Address of the first MMIO region */
> +	__le64					address;
>  };
>  
>  /* Request types */
> 

[PATCH v3 5/6] iommu/virtio: Support topology description in config space

[Jean-Philippe Brucker]

Platforms without device-tree nor ACPI can provide a topology
description embedded into the virtio config space. Parse it.

Use PCI FIXUP to probe the config space early, because we need to
discover the topology before any DMA configuration takes place, and the
virtio driver may be loaded much later. Since we discover the topology
description when probing the PCI hierarchy, the virtual IOMMU cannot
manage other platform devices discovered earlier.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/iommu/Kconfig           |  12 ++
 drivers/iommu/virtio/Makefile   |   1 +
 drivers/iommu/virtio/topology.c | 259 ++++++++++++++++++++++++++++++++
 3 files changed, 272 insertions(+)
 create mode 100644 drivers/iommu/virtio/topology.c

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index e29ae50f7100..98d28fdbc19a 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -394,4 +394,16 @@ config VIRTIO_IOMMU
 config VIRTIO_IOMMU_TOPOLOGY_HELPERS
 	bool
 
+config VIRTIO_IOMMU_TOPOLOGY
+	bool "Handle topology properties from the virtio-iommu"
+	depends on VIRTIO_IOMMU
+	depends on PCI
+	default y
+	select VIRTIO_IOMMU_TOPOLOGY_HELPERS
+	help
+	  Enable early probing of virtio-iommu devices to detect the built-in
+	  topology description.
+
+	  Say Y here if you intend to run this kernel as a guest.
+
 endif # IOMMU_SUPPORT
diff --git a/drivers/iommu/virtio/Makefile b/drivers/iommu/virtio/Makefile
index b42ad47eac7e..1eda8ca1cbbf 100644
--- a/drivers/iommu/virtio/Makefile
+++ b/drivers/iommu/virtio/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
+obj-$(CONFIG_VIRTIO_IOMMU_TOPOLOGY) += topology.o
 obj-$(CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS) += topology-helpers.o
diff --git a/drivers/iommu/virtio/topology.c b/drivers/iommu/virtio/topology.c
new file mode 100644
index 000000000000..4923eec618b9
--- /dev/null
+++ b/drivers/iommu/virtio/topology.c
@@ -0,0 +1,259 @@
+// SPDX-License-Identifier: GPL-2.0
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/io-64-nonatomic-hi-lo.h>
+#include <linux/iopoll.h>
+#include <linux/list.h>
+#include <linux/pci.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_pci.h>
+#include <uapi/linux/virtio_config.h>
+#include <uapi/linux/virtio_iommu.h>
+
+#include "topology-helpers.h"
+
+struct viommu_cap_config {
+	u8 bar;
+	u32 length; /* structure size */
+	u32 offset; /* structure offset within the bar */
+};
+
+struct viommu_topo_header {
+	u8 type;
+	u8 reserved;
+	u16 length;
+};
+
+static struct virt_topo_endpoint *
+viommu_parse_node(void __iomem *buf, size_t len)
+{
+	int ret = -EINVAL;
+	union {
+		struct viommu_topo_header hdr;
+		struct virtio_iommu_topo_pci_range pci;
+		struct virtio_iommu_topo_mmio mmio;
+	} __iomem *cfg = buf;
+	struct virt_topo_endpoint *spec;
+
+	spec = kzalloc(sizeof(*spec), GFP_KERNEL);
+	if (!spec)
+		return ERR_PTR(-ENOMEM);
+
+	switch (ioread8(&cfg->hdr.type)) {
+	case VIRTIO_IOMMU_TOPO_PCI_RANGE:
+		if (len < sizeof(cfg->pci))
+			goto err_free;
+
+		spec->dev_id.type = VIRT_TOPO_DEV_TYPE_PCI;
+		spec->dev_id.segment = ioread16(&cfg->pci.segment);
+		spec->dev_id.bdf_start = ioread16(&cfg->pci.bdf_start);
+		spec->dev_id.bdf_end = ioread16(&cfg->pci.bdf_end);
+		spec->endpoint_id = ioread32(&cfg->pci.endpoint_start);
+		break;
+	case VIRTIO_IOMMU_TOPO_MMIO:
+		if (len < sizeof(cfg->mmio))
+			goto err_free;
+
+		spec->dev_id.type = VIRT_TOPO_DEV_TYPE_MMIO;
+		spec->dev_id.base = ioread64(&cfg->mmio.address);
+		spec->endpoint_id = ioread32(&cfg->mmio.endpoint);
+		break;
+	default:
+		pr_warn("unhandled format 0x%x\n", ioread8(&cfg->hdr.type));
+		ret = 0;
+		goto err_free;
+	}
+	return spec;
+
+err_free:
+	kfree(spec);
+	return ERR_PTR(ret);
+}
+
+static int viommu_parse_topology(struct device *dev,
+				 struct virtio_iommu_config __iomem *cfg,
+				 size_t max_len)
+{
+	int ret;
+	u16 len;
+	size_t i;
+	LIST_HEAD(endpoints);
+	size_t offset, count;
+	struct virt_topo_iommu *viommu;
+	struct virt_topo_endpoint *ep, *next;
+	struct viommu_topo_header __iomem *cur;
+
+	offset = ioread16(&cfg->topo_config.offset);
+	count = ioread16(&cfg->topo_config.count);
+	if (!offset || !count)
+		return 0;
+
+	viommu = kzalloc(sizeof(*viommu), GFP_KERNEL);
+	if (!viommu)
+		return -ENOMEM;
+
+	viommu->dev = dev;
+
+	for (i = 0; i < count; i++, offset += len) {
+		if (offset + sizeof(*cur) > max_len) {
+			ret = -EOVERFLOW;
+			goto err_free;
+		}
+
+		cur = (void __iomem *)cfg + offset;
+		len = ioread16(&cur->length);
+		if (offset + len > max_len) {
+			ret = -EOVERFLOW;
+			goto err_free;
+		}
+
+		ep = viommu_parse_node((void __iomem *)cur, len);
+		if (!ep) {
+			continue;
+		} else if (IS_ERR(ep)) {
+			ret = PTR_ERR(ep);
+			goto err_free;
+		}
+
+		ep->viommu = viommu;
+		list_add(&ep->list, &endpoints);
+	}
+
+	list_for_each_entry_safe(ep, next, &endpoints, list)
+		/* Moves ep to the helpers list */
+		virt_topo_add_endpoint(ep);
+	virt_topo_add_iommu(viommu);
+
+	return 0;
+err_free:
+	list_for_each_entry_safe(ep, next, &endpoints, list)
+		kfree(ep);
+	kfree(viommu);
+	return ret;
+}
+
+#define VPCI_FIELD(field) offsetof(struct virtio_pci_cap, field)
+
+static inline int viommu_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
+					     struct viommu_cap_config *cap)
+{
+	int pos;
+	u8 bar;
+
+	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 + VPCI_FIELD(cfg_type), &type);
+		if (type != cfg_type)
+			continue;
+
+		pci_read_config_byte(dev, pos + VPCI_FIELD(bar), &bar);
+
+		/* Ignore structures with reserved BAR values */
+		if (type != VIRTIO_PCI_CAP_PCI_CFG && bar > 0x5)
+			continue;
+
+		cap->bar = bar;
+		pci_read_config_dword(dev, pos + VPCI_FIELD(length),
+				      &cap->length);
+		pci_read_config_dword(dev, pos + VPCI_FIELD(offset),
+				      &cap->offset);
+
+		return pos;
+	}
+	return 0;
+}
+
+static int viommu_pci_reset(struct virtio_pci_common_cfg __iomem *cfg)
+{
+	u8 status;
+	ktime_t timeout = ktime_add_ms(ktime_get(), 100);
+
+	iowrite8(0, &cfg->device_status);
+	while ((status = ioread8(&cfg->device_status)) != 0 &&
+	       ktime_before(ktime_get(), timeout))
+		msleep(1);
+
+	return status ? -ETIMEDOUT : 0;
+}
+
+static void viommu_pci_parse_topology(struct pci_dev *dev)
+{
+	int ret;
+	u32 features;
+	void __iomem *regs, *common_regs;
+	struct viommu_cap_config cap = {0};
+	struct virtio_pci_common_cfg __iomem *common_cfg;
+
+	/*
+	 * The virtio infrastructure might not be loaded at this point. We need
+	 * to access the BARs ourselves.
+	 */
+	ret = viommu_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG, &cap);
+	if (!ret) {
+		pci_warn(dev, "common capability not found\n");
+		return;
+	}
+
+	if (pci_enable_device_mem(dev))
+		return;
+
+	common_regs = pci_iomap(dev, cap.bar, 0);
+	if (!common_regs)
+		return;
+
+	common_cfg = common_regs + cap.offset;
+
+	/* Perform the init sequence before we can read the config */
+	ret = viommu_pci_reset(common_cfg);
+	if (ret < 0) {
+		pci_warn(dev, "unable to reset device\n");
+		goto out_unmap_common;
+	}
+
+	iowrite8(VIRTIO_CONFIG_S_ACKNOWLEDGE, &common_cfg->device_status);
+	iowrite8(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER,
+		 &common_cfg->device_status);
+
+	/* Find out if the device supports topology description */
+	iowrite32(0, &common_cfg->device_feature_select);
+	features = ioread32(&common_cfg->device_feature);
+
+	if (!(features & BIT(VIRTIO_IOMMU_F_TOPOLOGY))) {
+		pci_dbg(dev, "device doesn't have topology description");
+		goto out_reset;
+	}
+
+	ret = viommu_pci_find_capability(dev, VIRTIO_PCI_CAP_DEVICE_CFG, &cap);
+	if (!ret) {
+		pci_warn(dev, "device config capability not found\n");
+		goto out_reset;
+	}
+
+	regs = pci_iomap(dev, cap.bar, 0);
+	if (!regs)
+		goto out_reset;
+
+	pci_info(dev, "parsing virtio-iommu topology\n");
+	ret = viommu_parse_topology(&dev->dev, regs + cap.offset,
+				    pci_resource_len(dev, 0) - cap.offset);
+	if (ret)
+		pci_warn(dev, "failed to parse topology: %d\n", ret);
+
+	pci_iounmap(dev, regs);
+out_reset:
+	ret = viommu_pci_reset(common_cfg);
+	if (ret)
+		pci_warn(dev, "unable to reset device\n");
+out_unmap_common:
+	pci_iounmap(dev, common_regs);
+}
+
+/*
+ * Catch a PCI virtio-iommu implementation early to get the topology description
+ * before we start probing other endpoints.
+ */
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT_QUMRANET, 0x1040 + VIRTIO_ID_IOMMU,
+			viommu_pci_parse_topology);
-- 
2.28.0

Re: [PATCH v3 5/6] iommu/virtio: Support topology description in config space

[Auger Eric]

Hi Jean,

On 8/21/20 3:15 PM, Jean-Philippe Brucker wrote:
> Platforms without device-tree nor ACPI can provide a topology
> description embedded into the virtio config space. Parse it.
> 
> Use PCI FIXUP to probe the config space early, because we need to
> discover the topology before any DMA configuration takes place, and the
> virtio driver may be loaded much later. Since we discover the topology
> description when probing the PCI hierarchy, the virtual IOMMU cannot
> manage other platform devices discovered earlier.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  drivers/iommu/Kconfig           |  12 ++
>  drivers/iommu/virtio/Makefile   |   1 +
>  drivers/iommu/virtio/topology.c | 259 ++++++++++++++++++++++++++++++++
>  3 files changed, 272 insertions(+)
>  create mode 100644 drivers/iommu/virtio/topology.c
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index e29ae50f7100..98d28fdbc19a 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -394,4 +394,16 @@ config VIRTIO_IOMMU
>  config VIRTIO_IOMMU_TOPOLOGY_HELPERS
>  	bool
>  
> +config VIRTIO_IOMMU_TOPOLOGY
> +	bool "Handle topology properties from the virtio-iommu"
> +	depends on VIRTIO_IOMMU
> +	depends on PCI
> +	default y
> +	select VIRTIO_IOMMU_TOPOLOGY_HELPERS
> +	help
> +	  Enable early probing of virtio-iommu devices to detect the built-in
> +	  topology description.
> +
> +	  Say Y here if you intend to run this kernel as a guest.
> +
>  endif # IOMMU_SUPPORT
> diff --git a/drivers/iommu/virtio/Makefile b/drivers/iommu/virtio/Makefile
> index b42ad47eac7e..1eda8ca1cbbf 100644
> --- a/drivers/iommu/virtio/Makefile
> +++ b/drivers/iommu/virtio/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
> +obj-$(CONFIG_VIRTIO_IOMMU_TOPOLOGY) += topology.o
>  obj-$(CONFIG_VIRTIO_IOMMU_TOPOLOGY_HELPERS) += topology-helpers.o
> diff --git a/drivers/iommu/virtio/topology.c b/drivers/iommu/virtio/topology.c
> new file mode 100644
> index 000000000000..4923eec618b9
> --- /dev/null
> +++ b/drivers/iommu/virtio/topology.c
> @@ -0,0 +1,259 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/io-64-nonatomic-hi-lo.h>
> +#include <linux/iopoll.h>
> +#include <linux/list.h>
> +#include <linux/pci.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_pci.h>
> +#include <uapi/linux/virtio_config.h>
> +#include <uapi/linux/virtio_iommu.h>
> +
> +#include "topology-helpers.h"
> +
> +struct viommu_cap_config {
> +	u8 bar;
> +	u32 length; /* structure size */
> +	u32 offset; /* structure offset within the bar */
> +};
> +
> +struct viommu_topo_header {
> +	u8 type;
> +	u8 reserved;
> +	u16 length;
> +};
> +
> +static struct virt_topo_endpoint *
> +viommu_parse_node(void __iomem *buf, size_t len)
> +{
> +	int ret = -EINVAL;
> +	union {
> +		struct viommu_topo_header hdr;
> +		struct virtio_iommu_topo_pci_range pci;
> +		struct virtio_iommu_topo_mmio mmio;
> +	} __iomem *cfg = buf;
> +	struct virt_topo_endpoint *spec;
> +
> +	spec = kzalloc(sizeof(*spec), GFP_KERNEL);
> +	if (!spec)
> +		return ERR_PTR(-ENOMEM);
> +
> +	switch (ioread8(&cfg->hdr.type)) {
> +	case VIRTIO_IOMMU_TOPO_PCI_RANGE:
> +		if (len < sizeof(cfg->pci))
> +			goto err_free;
> +
> +		spec->dev_id.type = VIRT_TOPO_DEV_TYPE_PCI;
> +		spec->dev_id.segment = ioread16(&cfg->pci.segment);
> +		spec->dev_id.bdf_start = ioread16(&cfg->pci.bdf_start);
> +		spec->dev_id.bdf_end = ioread16(&cfg->pci.bdf_end);
> +		spec->endpoint_id = ioread32(&cfg->pci.endpoint_start);
> +		break;
> +	case VIRTIO_IOMMU_TOPO_MMIO:
> +		if (len < sizeof(cfg->mmio))
> +			goto err_free;
> +
> +		spec->dev_id.type = VIRT_TOPO_DEV_TYPE_MMIO;
> +		spec->dev_id.base = ioread64(&cfg->mmio.address);
> +		spec->endpoint_id = ioread32(&cfg->mmio.endpoint);
> +		break;
> +	default:
> +		pr_warn("unhandled format 0x%x\n", ioread8(&cfg->hdr.type));
> +		ret = 0;
> +		goto err_free;
> +	}
> +	return spec;
> +
> +err_free:
> +	kfree(spec);
> +	return ERR_PTR(ret);
> +}
> +
> +static int viommu_parse_topology(struct device *dev,
> +				 struct virtio_iommu_config __iomem *cfg,
> +				 size_t max_len)
> +{
> +	int ret;
> +	u16 len;
> +	size_t i;
> +	LIST_HEAD(endpoints);
> +	size_t offset, count;
> +	struct virt_topo_iommu *viommu;
> +	struct virt_topo_endpoint *ep, *next;
> +	struct viommu_topo_header __iomem *cur;
> +
> +	offset = ioread16(&cfg->topo_config.offset);
> +	count = ioread16(&cfg->topo_config.count);
> +	if (!offset || !count)
> +		return 0;
> +
> +	viommu = kzalloc(sizeof(*viommu), GFP_KERNEL);
> +	if (!viommu)
> +		return -ENOMEM;
> +
> +	viommu->dev = dev;
> +
> +	for (i = 0; i < count; i++, offset += len) {
> +		if (offset + sizeof(*cur) > max_len) {
> +			ret = -EOVERFLOW;
> +			goto err_free;
> +		}
> +
> +		cur = (void __iomem *)cfg + offset;
> +		len = ioread16(&cur->length);
> +		if (offset + len > max_len) {
> +			ret = -EOVERFLOW;
> +			goto err_free;
> +		}
> +
> +		ep = viommu_parse_node((void __iomem *)cur, len);
> +		if (!ep) {
> +			continue;
> +		} else if (IS_ERR(ep)) {
> +			ret = PTR_ERR(ep);
> +			goto err_free;
> +		}
> +
> +		ep->viommu = viommu;
> +		list_add(&ep->list, &endpoints);
> +	}
> +
> +	list_for_each_entry_safe(ep, next, &endpoints, list)
> +		/* Moves ep to the helpers list */
> +		virt_topo_add_endpoint(ep);
> +	virt_topo_add_iommu(viommu);
> +
> +	return 0;
> +err_free:
> +	list_for_each_entry_safe(ep, next, &endpoints, list)
> +		kfree(ep);
> +	kfree(viommu);
> +	return ret;
> +}
> +
> +#define VPCI_FIELD(field) offsetof(struct virtio_pci_cap, field)
> +
> +static inline int viommu_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
> +					     struct viommu_cap_config *cap)
not sure the inline is useful here
> +{
> +	int pos;
> +	u8 bar;
> +
> +	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 + VPCI_FIELD(cfg_type), &type);
> +		if (type != cfg_type)
> +			continue;
> +
> +		pci_read_config_byte(dev, pos + VPCI_FIELD(bar), &bar);
> +
> +		/* Ignore structures with reserved BAR values */
> +		if (type != VIRTIO_PCI_CAP_PCI_CFG && bar > 0x5)
> +			continue;
> +
> +		cap->bar = bar;
> +		pci_read_config_dword(dev, pos + VPCI_FIELD(length),
> +				      &cap->length);
> +		pci_read_config_dword(dev, pos + VPCI_FIELD(offset),
> +				      &cap->offset);
> +
> +		return pos;
> +	}
> +	return 0;
> +}
> +
> +static int viommu_pci_reset(struct virtio_pci_common_cfg __iomem *cfg)
> +{
> +	u8 status;
> +	ktime_t timeout = ktime_add_ms(ktime_get(), 100);
> +
> +	iowrite8(0, &cfg->device_status);
> +	while ((status = ioread8(&cfg->device_status)) != 0 &&
> +	       ktime_before(ktime_get(), timeout))
> +		msleep(1);
> +
> +	return status ? -ETIMEDOUT : 0;
> +}
> +
> +static void viommu_pci_parse_topology(struct pci_dev *dev)
> +{
> +	int ret;
> +	u32 features;
> +	void __iomem *regs, *common_regs;
> +	struct viommu_cap_config cap = {0};
> +	struct virtio_pci_common_cfg __iomem *common_cfg;
> +
> +	/*
> +	 * The virtio infrastructure might not be loaded at this point. We need
> +	 * to access the BARs ourselves.
> +	 */
> +	ret = viommu_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG, &cap);
> +	if (!ret) {
> +		pci_warn(dev, "common capability not found\n");
> +		return;
> +	}
> +
> +	if (pci_enable_device_mem(dev))
> +		return;
> +
> +	common_regs = pci_iomap(dev, cap.bar, 0);
> +	if (!common_regs)
> +		return;
> +
> +	common_cfg = common_regs + cap.offset;
> +
> +	/* Perform the init sequence before we can read the config */
> +	ret = viommu_pci_reset(common_cfg);
> +	if (ret < 0) {
> +		pci_warn(dev, "unable to reset device\n");
> +		goto out_unmap_common;
> +	}
> +
> +	iowrite8(VIRTIO_CONFIG_S_ACKNOWLEDGE, &common_cfg->device_status);
> +	iowrite8(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER,
> +		 &common_cfg->device_status);
> +
> +	/* Find out if the device supports topology description */
> +	iowrite32(0, &common_cfg->device_feature_select);
> +	features = ioread32(&common_cfg->device_feature);
> +
> +	if (!(features & BIT(VIRTIO_IOMMU_F_TOPOLOGY))) {
> +		pci_dbg(dev, "device doesn't have topology description");
> +		goto out_reset;
> +	}
> +
> +	ret = viommu_pci_find_capability(dev, VIRTIO_PCI_CAP_DEVICE_CFG, &cap);
> +	if (!ret) {
> +		pci_warn(dev, "device config capability not found\n");
> +		goto out_reset;
> +	}
> +
> +	regs = pci_iomap(dev, cap.bar, 0);
> +	if (!regs)
> +		goto out_reset;
> +
> +	pci_info(dev, "parsing virtio-iommu topology\n");
> +	ret = viommu_parse_topology(&dev->dev, regs + cap.offset,
> +				    pci_resource_len(dev, 0) - cap.offset);
> +	if (ret)
> +		pci_warn(dev, "failed to parse topology: %d\n", ret);
> +
> +	pci_iounmap(dev, regs);
> +out_reset:
> +	ret = viommu_pci_reset(common_cfg);
> +	if (ret)
> +		pci_warn(dev, "unable to reset device\n");
> +out_unmap_common:
> +	pci_iounmap(dev, common_regs);
> +}
> +
> +/*
> + * Catch a PCI virtio-iommu implementation early to get the topology description
> + * before we start probing other endpoints.
> + */
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT_QUMRANET, 0x1040 + VIRTIO_ID_IOMMU,
> +			viommu_pci_parse_topology);
> 
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

Re: [PATCH v3 5/6] iommu/virtio: Support topology description in config space

[Jean-Philippe Brucker]

On Fri, Sep 04, 2020 at 06:05:35PM +0200, Auger Eric wrote:
> > +static inline int viommu_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
> > +					     struct viommu_cap_config *cap)
> not sure the inline is useful here

No, I'll drop it

Thanks,
Jean

Re: [PATCH v3 5/6] iommu/virtio: Support topology description in config space

[Bjorn Helgaas]

On Fri, Aug 21, 2020 at 03:15:39PM +0200, Jean-Philippe Brucker wrote:
> Platforms without device-tree nor ACPI can provide a topology
> description embedded into the virtio config space. Parse it.
> 
> Use PCI FIXUP to probe the config space early, because we need to
> discover the topology before any DMA configuration takes place, and the
> virtio driver may be loaded much later. Since we discover the topology
> description when probing the PCI hierarchy, the virtual IOMMU cannot
> manage other platform devices discovered earlier.

> +struct viommu_cap_config {
> +	u8 bar;
> +	u32 length; /* structure size */
> +	u32 offset; /* structure offset within the bar */

s/the bar/the BAR/ (to match comment below).

> +static void viommu_pci_parse_topology(struct pci_dev *dev)
> +{
> +	int ret;
> +	u32 features;
> +	void __iomem *regs, *common_regs;
> +	struct viommu_cap_config cap = {0};
> +	struct virtio_pci_common_cfg __iomem *common_cfg;
> +
> +	/*
> +	 * The virtio infrastructure might not be loaded at this point. We need
> +	 * to access the BARs ourselves.
> +	 */
> +	ret = viommu_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG, &cap);
> +	if (!ret) {
> +		pci_warn(dev, "common capability not found\n");

Is the lack of this capability really an error, i.e., is this
pci_warn() or pci_info()?  The "device doesn't have topology
description" below is only pci_dbg(), which suggests that we can live
without this.

Maybe a hint about what "common capability" means?

> +		return;
> +	}
> +
> +	if (pci_enable_device_mem(dev))
> +		return;
> +
> +	common_regs = pci_iomap(dev, cap.bar, 0);
> +	if (!common_regs)
> +		return;
> +
> +	common_cfg = common_regs + cap.offset;
> +
> +	/* Perform the init sequence before we can read the config */
> +	ret = viommu_pci_reset(common_cfg);

I guess this is some special device-specific reset, not any kind of
standard PCI reset?

> +	if (ret < 0) {
> +		pci_warn(dev, "unable to reset device\n");
> +		goto out_unmap_common;
> +	}
> +
> +	iowrite8(VIRTIO_CONFIG_S_ACKNOWLEDGE, &common_cfg->device_status);
> +	iowrite8(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER,
> +		 &common_cfg->device_status);
> +
> +	/* Find out if the device supports topology description */
> +	iowrite32(0, &common_cfg->device_feature_select);
> +	features = ioread32(&common_cfg->device_feature);
> +
> +	if (!(features & BIT(VIRTIO_IOMMU_F_TOPOLOGY))) {
> +		pci_dbg(dev, "device doesn't have topology description");
> +		goto out_reset;
> +	}
> +
> +	ret = viommu_pci_find_capability(dev, VIRTIO_PCI_CAP_DEVICE_CFG, &cap);
> +	if (!ret) {
> +		pci_warn(dev, "device config capability not found\n");
> +		goto out_reset;
> +	}
> +
> +	regs = pci_iomap(dev, cap.bar, 0);
> +	if (!regs)
> +		goto out_reset;
> +
> +	pci_info(dev, "parsing virtio-iommu topology\n");
> +	ret = viommu_parse_topology(&dev->dev, regs + cap.offset,
> +				    pci_resource_len(dev, 0) - cap.offset);
> +	if (ret)
> +		pci_warn(dev, "failed to parse topology: %d\n", ret);
> +
> +	pci_iounmap(dev, regs);
> +out_reset:
> +	ret = viommu_pci_reset(common_cfg);
> +	if (ret)
> +		pci_warn(dev, "unable to reset device\n");
> +out_unmap_common:
> +	pci_iounmap(dev, common_regs);
> +}
> +
> +/*
> + * Catch a PCI virtio-iommu implementation early to get the topology description
> + * before we start probing other endpoints.
> + */
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT_QUMRANET, 0x1040 + VIRTIO_ID_IOMMU,
> +			viommu_pci_parse_topology);
> -- 
> 2.28.0
> 

Re: [PATCH v3 5/6] iommu/virtio: Support topology description in config space

[Jean-Philippe Brucker]

On Thu, Sep 24, 2020 at 10:22:03AM -0500, Bjorn Helgaas wrote:
> On Fri, Aug 21, 2020 at 03:15:39PM +0200, Jean-Philippe Brucker wrote:
> > Platforms without device-tree nor ACPI can provide a topology
> > description embedded into the virtio config space. Parse it.
> > 
> > Use PCI FIXUP to probe the config space early, because we need to
> > discover the topology before any DMA configuration takes place, and the
> > virtio driver may be loaded much later. Since we discover the topology
> > description when probing the PCI hierarchy, the virtual IOMMU cannot
> > manage other platform devices discovered earlier.
> 
> > +struct viommu_cap_config {
> > +	u8 bar;
> > +	u32 length; /* structure size */
> > +	u32 offset; /* structure offset within the bar */
> 
> s/the bar/the BAR/ (to match comment below).
> 
> > +static void viommu_pci_parse_topology(struct pci_dev *dev)
> > +{
> > +	int ret;
> > +	u32 features;
> > +	void __iomem *regs, *common_regs;
> > +	struct viommu_cap_config cap = {0};
> > +	struct virtio_pci_common_cfg __iomem *common_cfg;
> > +
> > +	/*
> > +	 * The virtio infrastructure might not be loaded at this point. We need
> > +	 * to access the BARs ourselves.
> > +	 */
> > +	ret = viommu_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG, &cap);
> > +	if (!ret) {
> > +		pci_warn(dev, "common capability not found\n");
> 
> Is the lack of this capability really an error, i.e., is this
> pci_warn() or pci_info()?  The "device doesn't have topology
> description" below is only pci_dbg(), which suggests that we can live
> without this.

At this point we know that this is a (modern) virtio-pci device which,
according to the virtio 1.0 specification, must have this capability. So
this is definitely an error, but the topology description is an optional
feature.

> 
> Maybe a hint about what "common capability" means?

Yes, "virtio-pci common configuration capability" would be more
appropriate

> 
> > +		return;
> > +	}
> > +
> > +	if (pci_enable_device_mem(dev))
> > +		return;
> > +
> > +	common_regs = pci_iomap(dev, cap.bar, 0);
> > +	if (!common_regs)
> > +		return;
> > +
> > +	common_cfg = common_regs + cap.offset;
> > +
> > +	/* Perform the init sequence before we can read the config */
> > +	ret = viommu_pci_reset(common_cfg);
> 
> I guess this is some special device-specific reset, not any kind of
> standard PCI reset?

Yes it's the virtio reset - writing 0 to the status register in the BAR.

Thanks,
Jean

Re: [PATCH v3 5/6] iommu/virtio: Support topology description in config space

[Bjorn Helgaas]

On Fri, Sep 25, 2020 at 10:12:43AM +0200, Jean-Philippe Brucker wrote:
> On Thu, Sep 24, 2020 at 10:22:03AM -0500, Bjorn Helgaas wrote:
> > On Fri, Aug 21, 2020 at 03:15:39PM +0200, Jean-Philippe Brucker wrote:

> > > +	/* Perform the init sequence before we can read the config */
> > > +	ret = viommu_pci_reset(common_cfg);
> > 
> > I guess this is some special device-specific reset, not any kind of
> > standard PCI reset?
> 
> Yes it's the virtio reset - writing 0 to the status register in the BAR.

I wonder if this should be named something like viommu_virtio_reset(),
so there's no confusion with PCI resets and all the timing
restrictions, config space restoration, etc. associated with them.

Bjorn

[PATCH v3 6/6] iommu/virtio: Enable x86 support

[Jean-Philippe Brucker]

With the built-in topology description in place, x86 platforms can now
use the virtio-iommu.

Architectures that use the generic iommu_dma_ops should normally select
CONFIG_IOMMU_DMA themselves (from arch/*/Kconfig). Since not all x86
drivers have been converted yet, it's currently up to the IOMMU Kconfig
to select it.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/iommu/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 98d28fdbc19a..d7cf158745eb 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -383,8 +383,9 @@ config HYPERV_IOMMU
 config VIRTIO_IOMMU
 	tristate "Virtio IOMMU driver"
 	depends on VIRTIO
-	depends on ARM64
+	depends on (ARM64 || X86)
 	select IOMMU_API
+	select IOMMU_DMA if X86
 	select INTERVAL_TREE
 	help
 	  Para-virtualised IOMMU driver with virtio.
-- 
2.28.0

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Michael S. Tsirkin]

On Fri, Aug 21, 2020 at 03:15:34PM +0200, Jean-Philippe Brucker wrote:
> Add a topology description to the virtio-iommu driver and enable x86
> platforms.
> 
> Since [v2] we have made some progress on adding ACPI support for
> virtio-iommu, which is the preferred boot method on x86. It will be a
> new vendor-agnostic table describing para-virtual topologies in a
> minimal format. However some platforms don't use either ACPI or DT for
> booting (for example microvm), and will need the alternative topology
> description method proposed here. In addition, since the process to get
> a new ACPI table will take a long time, this provides a boot method even
> to ACPI-based platforms, if only temporarily for testing and
> development.

OK should I park this in next now? Seems appropriate ...

> v3:
> * Add patch 1 that moves virtio-iommu to a subfolder.
> * Split the rest:
>   * Patch 2 adds topology-helper.c, which will be shared with the ACPI
>     support.
>   * Patch 4 adds definitions.
>   * Patch 5 adds parser in topology.c.
> * Address other comments.
> 
> Linux and QEMU patches available at:
> https://jpbrucker.net/git/linux virtio-iommu/devel
> https://jpbrucker.net/git/qemu virtio-iommu/devel
> 
> [spec] https://lists.oasis-open.org/archives/virtio-dev/202008/msg00067.html
> [v2] https://lore.kernel.org/linux-iommu/20200228172537.377327-1-jean-philippe@linaro.org/
> [v1] https://lore.kernel.org/linux-iommu/20200214160413.1475396-1-jean-philippe@linaro.org/
> [rfc] https://lore.kernel.org/linux-iommu/20191122105000.800410-1-jean-philippe@linaro.org/
> 
> Jean-Philippe Brucker (6):
>   iommu/virtio: Move to drivers/iommu/virtio/
>   iommu/virtio: Add topology helpers
>   PCI: Add DMA configuration for virtual platforms
>   iommu/virtio: Add topology definitions
>   iommu/virtio: Support topology description in config space
>   iommu/virtio: Enable x86 support
> 
>  drivers/iommu/Kconfig                     |  18 +-
>  drivers/iommu/Makefile                    |   3 +-
>  drivers/iommu/virtio/Makefile             |   4 +
>  drivers/iommu/virtio/topology-helpers.h   |  50 +++++
>  include/linux/virt_iommu.h                |  15 ++
>  include/uapi/linux/virtio_iommu.h         |  44 ++++
>  drivers/iommu/virtio/topology-helpers.c   | 196 ++++++++++++++++
>  drivers/iommu/virtio/topology.c           | 259 ++++++++++++++++++++++
>  drivers/iommu/{ => virtio}/virtio-iommu.c |   4 +
>  drivers/pci/pci-driver.c                  |   5 +
>  MAINTAINERS                               |   3 +-
>  11 files changed, 597 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/iommu/virtio/Makefile
>  create mode 100644 drivers/iommu/virtio/topology-helpers.h
>  create mode 100644 include/linux/virt_iommu.h
>  create mode 100644 drivers/iommu/virtio/topology-helpers.c
>  create mode 100644 drivers/iommu/virtio/topology.c
>  rename drivers/iommu/{ => virtio}/virtio-iommu.c (99%)
> 
> -- 
> 2.28.0
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Jean-Philippe Brucker]

On Wed, Aug 26, 2020 at 09:26:02AM -0400, Michael S. Tsirkin wrote:
> On Fri, Aug 21, 2020 at 03:15:34PM +0200, Jean-Philippe Brucker wrote:
> > Add a topology description to the virtio-iommu driver and enable x86
> > platforms.
> > 
> > Since [v2] we have made some progress on adding ACPI support for
> > virtio-iommu, which is the preferred boot method on x86. It will be a
> > new vendor-agnostic table describing para-virtual topologies in a
> > minimal format. However some platforms don't use either ACPI or DT for
> > booting (for example microvm), and will need the alternative topology
> > description method proposed here. In addition, since the process to get
> > a new ACPI table will take a long time, this provides a boot method even
> > to ACPI-based platforms, if only temporarily for testing and
> > development.
> 
> OK should I park this in next now? Seems appropriate ...

Yes that sounds like a good idea. It could uncover new bugs since there is
more automated testing happening for x86.

Thanks,
Jean

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Auger Eric]

Hi,

On 8/21/20 3:15 PM, Jean-Philippe Brucker wrote:
> Add a topology description to the virtio-iommu driver and enable x86
> platforms.
> 
> Since [v2] we have made some progress on adding ACPI support for
> virtio-iommu, which is the preferred boot method on x86. It will be a
> new vendor-agnostic table describing para-virtual topologies in a
> minimal format. However some platforms don't use either ACPI or DT for
> booting (for example microvm), and will need the alternative topology
> description method proposed here. In addition, since the process to get
> a new ACPI table will take a long time, this provides a boot method even
> to ACPI-based platforms, if only temporarily for testing and
> development.
> 
> v3:
> * Add patch 1 that moves virtio-iommu to a subfolder.
> * Split the rest:
>   * Patch 2 adds topology-helper.c, which will be shared with the ACPI
>     support.
>   * Patch 4 adds definitions.
>   * Patch 5 adds parser in topology.c.
> * Address other comments.
> 
> Linux and QEMU patches available at:
> https://jpbrucker.net/git/linux virtio-iommu/devel
> https://jpbrucker.net/git/qemu virtio-iommu/devel
I have tested that series with above QEMU branch on ARM with virtio-net
and virtio-blk translated devices in non DT mode.

It works for me:
Tested-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> 
> [spec] https://lists.oasis-open.org/archives/virtio-dev/202008/msg00067.html
> [v2] https://lore.kernel.org/linux-iommu/20200228172537.377327-1-jean-philippe@linaro.org/
> [v1] https://lore.kernel.org/linux-iommu/20200214160413.1475396-1-jean-philippe@linaro.org/
> [rfc] https://lore.kernel.org/linux-iommu/20191122105000.800410-1-jean-philippe@linaro.org/
> 
> Jean-Philippe Brucker (6):
>   iommu/virtio: Move to drivers/iommu/virtio/
>   iommu/virtio: Add topology helpers
>   PCI: Add DMA configuration for virtual platforms
>   iommu/virtio: Add topology definitions
>   iommu/virtio: Support topology description in config space
>   iommu/virtio: Enable x86 support
> 
>  drivers/iommu/Kconfig                     |  18 +-
>  drivers/iommu/Makefile                    |   3 +-
>  drivers/iommu/virtio/Makefile             |   4 +
>  drivers/iommu/virtio/topology-helpers.h   |  50 +++++
>  include/linux/virt_iommu.h                |  15 ++
>  include/uapi/linux/virtio_iommu.h         |  44 ++++
>  drivers/iommu/virtio/topology-helpers.c   | 196 ++++++++++++++++
>  drivers/iommu/virtio/topology.c           | 259 ++++++++++++++++++++++
>  drivers/iommu/{ => virtio}/virtio-iommu.c |   4 +
>  drivers/pci/pci-driver.c                  |   5 +
>  MAINTAINERS                               |   3 +-
>  11 files changed, 597 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/iommu/virtio/Makefile
>  create mode 100644 drivers/iommu/virtio/topology-helpers.h
>  create mode 100644 include/linux/virt_iommu.h
>  create mode 100644 drivers/iommu/virtio/topology-helpers.c
>  create mode 100644 drivers/iommu/virtio/topology.c
>  rename drivers/iommu/{ => virtio}/virtio-iommu.c (99%)
> 

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Michael S. Tsirkin]

On Fri, Sep 04, 2020 at 06:24:12PM +0200, Auger Eric wrote:
> Hi,
> 
> On 8/21/20 3:15 PM, Jean-Philippe Brucker wrote:
> > Add a topology description to the virtio-iommu driver and enable x86
> > platforms.
> > 
> > Since [v2] we have made some progress on adding ACPI support for
> > virtio-iommu, which is the preferred boot method on x86. It will be a
> > new vendor-agnostic table describing para-virtual topologies in a
> > minimal format. However some platforms don't use either ACPI or DT for
> > booting (for example microvm), and will need the alternative topology
> > description method proposed here. In addition, since the process to get
> > a new ACPI table will take a long time, this provides a boot method even
> > to ACPI-based platforms, if only temporarily for testing and
> > development.
> > 
> > v3:
> > * Add patch 1 that moves virtio-iommu to a subfolder.
> > * Split the rest:
> >   * Patch 2 adds topology-helper.c, which will be shared with the ACPI
> >     support.
> >   * Patch 4 adds definitions.
> >   * Patch 5 adds parser in topology.c.
> > * Address other comments.
> > 
> > Linux and QEMU patches available at:
> > https://jpbrucker.net/git/linux virtio-iommu/devel
> > https://jpbrucker.net/git/qemu virtio-iommu/devel
> I have tested that series with above QEMU branch on ARM with virtio-net
> and virtio-blk translated devices in non DT mode.
> 
> It works for me:
> Tested-by: Eric Auger <eric.auger@redhat.com>
> 
> Thanks
> 
> Eric

OK so this looks good. Can you pls repost with the minor tweak
suggested and all acks included, and I will queue this?

Thanks!

> > 
> > [spec] https://lists.oasis-open.org/archives/virtio-dev/202008/msg00067.html
> > [v2] https://lore.kernel.org/linux-iommu/20200228172537.377327-1-jean-philippe@linaro.org/
> > [v1] https://lore.kernel.org/linux-iommu/20200214160413.1475396-1-jean-philippe@linaro.org/
> > [rfc] https://lore.kernel.org/linux-iommu/20191122105000.800410-1-jean-philippe@linaro.org/
> > 
> > Jean-Philippe Brucker (6):
> >   iommu/virtio: Move to drivers/iommu/virtio/
> >   iommu/virtio: Add topology helpers
> >   PCI: Add DMA configuration for virtual platforms
> >   iommu/virtio: Add topology definitions
> >   iommu/virtio: Support topology description in config space
> >   iommu/virtio: Enable x86 support
> > 
> >  drivers/iommu/Kconfig                     |  18 +-
> >  drivers/iommu/Makefile                    |   3 +-
> >  drivers/iommu/virtio/Makefile             |   4 +
> >  drivers/iommu/virtio/topology-helpers.h   |  50 +++++
> >  include/linux/virt_iommu.h                |  15 ++
> >  include/uapi/linux/virtio_iommu.h         |  44 ++++
> >  drivers/iommu/virtio/topology-helpers.c   | 196 ++++++++++++++++
> >  drivers/iommu/virtio/topology.c           | 259 ++++++++++++++++++++++
> >  drivers/iommu/{ => virtio}/virtio-iommu.c |   4 +
> >  drivers/pci/pci-driver.c                  |   5 +
> >  MAINTAINERS                               |   3 +-
> >  11 files changed, 597 insertions(+), 4 deletions(-)
> >  create mode 100644 drivers/iommu/virtio/Makefile
> >  create mode 100644 drivers/iommu/virtio/topology-helpers.h
> >  create mode 100644 include/linux/virt_iommu.h
> >  create mode 100644 drivers/iommu/virtio/topology-helpers.c
> >  create mode 100644 drivers/iommu/virtio/topology.c
> >  rename drivers/iommu/{ => virtio}/virtio-iommu.c (99%)
> > 

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Joerg Roedel]

On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote:
> OK so this looks good. Can you pls repost with the minor tweak
> suggested and all acks included, and I will queue this?

My NACK still stands, as long as a few questions are open:

	1) The format used here will be the same as in the ACPI table? I
	   think the answer to this questions must be Yes, so this leads
	   to the real question:
	   
	2) Has the ACPI table format stabalized already? If and only if
	   the answer is Yes I will Ack these patches. We don't need to
	   wait until the ACPI table format is published in a
	   specification update, but at least some certainty that it
	   will not change in incompatible ways anymore is needed.

So what progress has been made with the ACPI table specification, is it
just a matter of time to get it approved or are there concerns?

Regards,

	Joerg

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Michael S. Tsirkin]

On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote:
> On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote:
> > OK so this looks good. Can you pls repost with the minor tweak
> > suggested and all acks included, and I will queue this?
> 
> My NACK still stands, as long as a few questions are open:
> 
> 	1) The format used here will be the same as in the ACPI table? I
> 	   think the answer to this questions must be Yes, so this leads
> 	   to the real question:

I am not sure it's a must.
We can always tweak the parser if there are slight differences
between ACPI and virtio formats.

But we do want the virtio format used here to be approved by the virtio
TC, so it won't change.

Eric, Jean-Philippe, does one of you intend to create a github issue
and request a ballot for the TC? It's been posted end of August with no
changes ...

> 	2) Has the ACPI table format stabalized already? If and only if
> 	   the answer is Yes I will Ack these patches. We don't need to
> 	   wait until the ACPI table format is published in a
> 	   specification update, but at least some certainty that it
> 	   will not change in incompatible ways anymore is needed.
> 

Not that I know, but I don't see why it's a must.

> So what progress has been made with the ACPI table specification, is it
> just a matter of time to get it approved or are there concerns?
> 
> Regards,
> 
> 	Joerg

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Auger Eric]

Hi,

Adding Al in the loop

On 9/24/20 11:38 AM, Michael S. Tsirkin wrote:
> On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote:
>> On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote:
>>> OK so this looks good. Can you pls repost with the minor tweak
>>> suggested and all acks included, and I will queue this?
>>
>> My NACK still stands, as long as a few questions are open:
>>
>> 	1) The format used here will be the same as in the ACPI table? I
>> 	   think the answer to this questions must be Yes, so this leads
>> 	   to the real question:
> 
> I am not sure it's a must.
> We can always tweak the parser if there are slight differences
> between ACPI and virtio formats.
> 
> But we do want the virtio format used here to be approved by the virtio
> TC, so it won't change.
> 
> Eric, Jean-Philippe, does one of you intend to create a github issue
> and request a ballot for the TC? It's been posted end of August with no
> changes ...
Jean-Philippe, would you?
> 
>> 	2) Has the ACPI table format stabalized already? If and only if
>> 	   the answer is Yes I will Ack these patches. We don't need to
>> 	   wait until the ACPI table format is published in a
>> 	   specification update, but at least some certainty that it
>> 	   will not change in incompatible ways anymore is needed.
>>

Al, do you have any news about the the VIOT definition submission to
the UEFI ASWG?

Thank you in advance

Best Regards

Eric


> 
> Not that I know, but I don't see why it's a must.
> 
>> So what progress has been made with the ACPI table specification, is it
>> just a matter of time to get it approved or are there concerns?
>>
>> Regards,
>>
>> 	Joerg
> 

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Al Stone]

On 24 Sep 2020 11:54, Auger Eric wrote:
> Hi,
> 
> Adding Al in the loop
> 
> On 9/24/20 11:38 AM, Michael S. Tsirkin wrote:
> > On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote:
> >> On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote:
> >>> OK so this looks good. Can you pls repost with the minor tweak
> >>> suggested and all acks included, and I will queue this?
> >>
> >> My NACK still stands, as long as a few questions are open:
> >>
> >> 	1) The format used here will be the same as in the ACPI table? I
> >> 	   think the answer to this questions must be Yes, so this leads
> >> 	   to the real question:
> > 
> > I am not sure it's a must.
> > We can always tweak the parser if there are slight differences
> > between ACPI and virtio formats.
> > 
> > But we do want the virtio format used here to be approved by the virtio
> > TC, so it won't change.

As long as we can convey the same content to the UEFI ASWG, we're
fine.  Format/syntax of the submittal is not absolutely critical
though it does need translating to what the ASWG expects (see
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-First-Process
for details -- basically a bugzilla with markdown text.

> > Eric, Jean-Philippe, does one of you intend to create a github issue
> > and request a ballot for the TC? It's been posted end of August with no
> > changes ...
> Jean-Philippe, would you?
> > 
> >> 	2) Has the ACPI table format stabalized already? If and only if
> >> 	   the answer is Yes I will Ack these patches. We don't need to
> >> 	   wait until the ACPI table format is published in a
> >> 	   specification update, but at least some certainty that it
> >> 	   will not change in incompatible ways anymore is needed.
> >>
> 
> Al, do you have any news about the the VIOT definition submission to
> the UEFI ASWG?
> 
> Thank you in advance
> 
> Best Regards
> 
> Eric
> 
> 
> > 
> > Not that I know, but I don't see why it's a must.
> > 
> >> So what progress has been made with the ACPI table specification, is it
> >> just a matter of time to get it approved or are there concerns?
> >>
> >> Regards,
> >>
> >> 	Joerg

My apologies for the delay.  No excuses, just not enough hours in the
day.

I will make the proper submission to the ASWG later today to have it
considered later this week -- I had quite a bit of confusion around
how the process is supposed to work but I think we've got that cleared
up (see the link noted above).

The content of the table appears to be in really good shape.  Will it
change?  Possibly, but my expectation is that it will be minor details,
nothing wholesale; having the table in use in code tends to act as a
pretty fierce restraint on making changes (there's a lot of precedent
for that in ASWG).

The biggest question is: are there any objections to having this
table description licensed under Creative Commons Attribution
International 4.0 (see https://spdx.org/licenses/CC-BY-4.0.html)?
This is just for the table description, not the code.  If there
are, that needs to be cleared up first.  If not, then the submittal
this week should happen.

Once submitted to ASWG, there is a very slim chance it will end up
in ACPI 6.4 which is mostly done now -- very, very slim, but stranger
things have happened.  Most likely, once approved it would be in
ACPI 6.5, sometime in 2021.  I'll post the link to the submittal
as soon as I can.

Again, my apologies for the delays; approval in the spec can proceed
pretty much independent of the implementation, and vice versa.  That's
really the whole point of this new process with the UEFI Forum.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Al Stone]

On 24 Sep 2020 11:54, Auger Eric wrote:
> Hi,
> 
> Adding Al in the loop
> 
> On 9/24/20 11:38 AM, Michael S. Tsirkin wrote:
> > On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote:
> >> On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote:
> >>> OK so this looks good. Can you pls repost with the minor tweak
> >>> suggested and all acks included, and I will queue this?
> >>
> >> My NACK still stands, as long as a few questions are open:
> >>
> >> 	1) The format used here will be the same as in the ACPI table? I
> >> 	   think the answer to this questions must be Yes, so this leads
> >> 	   to the real question:
> > 
> > I am not sure it's a must.
> > We can always tweak the parser if there are slight differences
> > between ACPI and virtio formats.
> > 
> > But we do want the virtio format used here to be approved by the virtio
> > TC, so it won't change.
> > 
> > Eric, Jean-Philippe, does one of you intend to create a github issue
> > and request a ballot for the TC? It's been posted end of August with no
> > changes ...
> Jean-Philippe, would you?
> > 
> >> 	2) Has the ACPI table format stabalized already? If and only if
> >> 	   the answer is Yes I will Ack these patches. We don't need to
> >> 	   wait until the ACPI table format is published in a
> >> 	   specification update, but at least some certainty that it
> >> 	   will not change in incompatible ways anymore is needed.
> >>
> 
> Al, do you have any news about the the VIOT definition submission to
> the UEFI ASWG?
> 
> Thank you in advance
> 
> Best Regards
> 
> Eric

A follow-up to my earlier post ....

Hearing no objection, I've submitted the VIOT table description to
the ASWG for consideration under what they call the "code first"
process.  The "first reading" -- a brief discussion on what the
table is and why we would like to add it -- was held yesterday.
No concerns have been raised as yet.  Given the discussions that
have already occurred, I don't expect any, either.  I have been
wrong at least once before, however.

At this point, ASWG will revisit the request to add VIOT each
week.  If there have been no comments in the prior week, and no
further discussion during the meeting, then a vote will be taken.
Otherwise, there will be discussion and we try again the next
week.

The ASWG was also told that the likelihood of this definition of
the table changing is pretty low, and that it has been thought out
pretty well already.  ASWG's consideration will therefore start
from the assumption that it would be best _not_ to make changes.

So, I'll let you know what happens next week.

> 
> > 
> > Not that I know, but I don't see why it's a must.
> > 
> >> So what progress has been made with the ACPI table specification, is it
> >> just a matter of time to get it approved or are there concerns?
> >>
> >> Regards,
> >>
> >> 	Joerg
> > 
> 

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Auger Eric]

Hello Al,

On 10/2/20 8:23 PM, Al Stone wrote:
> On 24 Sep 2020 11:54, Auger Eric wrote:
>> Hi,
>>
>> Adding Al in the loop
>>
>> On 9/24/20 11:38 AM, Michael S. Tsirkin wrote:
>>> On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote:
>>>> On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote:
>>>>> OK so this looks good. Can you pls repost with the minor tweak
>>>>> suggested and all acks included, and I will queue this?
>>>>
>>>> My NACK still stands, as long as a few questions are open:
>>>>
>>>> 	1) The format used here will be the same as in the ACPI table? I
>>>> 	   think the answer to this questions must be Yes, so this leads
>>>> 	   to the real question:
>>>
>>> I am not sure it's a must.
>>> We can always tweak the parser if there are slight differences
>>> between ACPI and virtio formats.
>>>
>>> But we do want the virtio format used here to be approved by the virtio
>>> TC, so it won't change.
>>>
>>> Eric, Jean-Philippe, does one of you intend to create a github issue
>>> and request a ballot for the TC? It's been posted end of August with no
>>> changes ...
>> Jean-Philippe, would you?
>>>
>>>> 	2) Has the ACPI table format stabalized already? If and only if
>>>> 	   the answer is Yes I will Ack these patches. We don't need to
>>>> 	   wait until the ACPI table format is published in a
>>>> 	   specification update, but at least some certainty that it
>>>> 	   will not change in incompatible ways anymore is needed.
>>>>
>>
>> Al, do you have any news about the the VIOT definition submission to
>> the UEFI ASWG?
>>
>> Thank you in advance
>>
>> Best Regards
>>
>> Eric
> 
> A follow-up to my earlier post ....
> 
> Hearing no objection, I've submitted the VIOT table description to
> the ASWG for consideration under what they call the "code first"
> process.  The "first reading" -- a brief discussion on what the
> table is and why we would like to add it -- was held yesterday.
> No concerns have been raised as yet.  Given the discussions that
> have already occurred, I don't expect any, either.  I have been
> wrong at least once before, however.
> 
> At this point, ASWG will revisit the request to add VIOT each
> week.  If there have been no comments in the prior week, and no
> further discussion during the meeting, then a vote will be taken.
> Otherwise, there will be discussion and we try again the next
> week.
> 
> The ASWG was also told that the likelihood of this definition of
> the table changing is pretty low, and that it has been thought out
> pretty well already.  ASWG's consideration will therefore start
> from the assumption that it would be best _not_ to make changes.
> 
> So, I'll let you know what happens next week.

Thank you very much for the updates and for your support backing the
proposal in the best delays.

Best Regards

Eric
> 
>>
>>>
>>> Not that I know, but I don't see why it's a must.
>>>
>>>> So what progress has been made with the ACPI table specification, is it
>>>> just a matter of time to get it approved or are there concerns?
>>>>
>>>> Regards,
>>>>
>>>> 	Joerg
>>>
>>
> 

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Al Stone]

On 06 Oct 2020 17:23, Auger Eric wrote:
> Hello Al,
> 
> On 10/2/20 8:23 PM, Al Stone wrote:
> > On 24 Sep 2020 11:54, Auger Eric wrote:
> >> Hi,
> >>
> >> Adding Al in the loop
> >>
> >> On 9/24/20 11:38 AM, Michael S. Tsirkin wrote:
> >>> On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote:
> >>>> On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote:
> >>>>> OK so this looks good. Can you pls repost with the minor tweak
> >>>>> suggested and all acks included, and I will queue this?
> >>>>
> >>>> My NACK still stands, as long as a few questions are open:
> >>>>
> >>>> 	1) The format used here will be the same as in the ACPI table? I
> >>>> 	   think the answer to this questions must be Yes, so this leads
> >>>> 	   to the real question:
> >>>
> >>> I am not sure it's a must.
> >>> We can always tweak the parser if there are slight differences
> >>> between ACPI and virtio formats.
> >>>
> >>> But we do want the virtio format used here to be approved by the virtio
> >>> TC, so it won't change.
> >>>
> >>> Eric, Jean-Philippe, does one of you intend to create a github issue
> >>> and request a ballot for the TC? It's been posted end of August with no
> >>> changes ...
> >> Jean-Philippe, would you?
> >>>
> >>>> 	2) Has the ACPI table format stabalized already? If and only if
> >>>> 	   the answer is Yes I will Ack these patches. We don't need to
> >>>> 	   wait until the ACPI table format is published in a
> >>>> 	   specification update, but at least some certainty that it
> >>>> 	   will not change in incompatible ways anymore is needed.
> >>>>
> >>
> >> Al, do you have any news about the the VIOT definition submission to
> >> the UEFI ASWG?
> >>
> >> Thank you in advance
> >>
> >> Best Regards
> >>
> >> Eric
> > 
> > A follow-up to my earlier post ....
> > 
> > Hearing no objection, I've submitted the VIOT table description to
> > the ASWG for consideration under what they call the "code first"
> > process.  The "first reading" -- a brief discussion on what the
> > table is and why we would like to add it -- was held yesterday.
> > No concerns have been raised as yet.  Given the discussions that
> > have already occurred, I don't expect any, either.  I have been
> > wrong at least once before, however.
> > 
> > At this point, ASWG will revisit the request to add VIOT each
> > week.  If there have been no comments in the prior week, and no
> > further discussion during the meeting, then a vote will be taken.
> > Otherwise, there will be discussion and we try again the next
> > week.
> > 
> > The ASWG was also told that the likelihood of this definition of
> > the table changing is pretty low, and that it has been thought out
> > pretty well already.  ASWG's consideration will therefore start
> > from the assumption that it would be best _not_ to make changes.
> > 
> > So, I'll let you know what happens next week.
> 
> Thank you very much for the updates and for your support backing the
> proposal in the best delays.
> 
> Best Regards
> 
> Eric

So, there are some questions about the VIOT definition and I just
don't know enough to be able to answer them.  One of the ASWG members
is trying to understand the semantics behind the subtables.

Is there a particular set of people, or mailing lists, that I can
point to to get the questions answered?  Ideally it would be one
of the public lists where it has already been discussed, but an
individual would be fine, too.  No changes have been proposed, just
some questions asked.

Thanks.

> > 
> >>
> >>>
> >>> Not that I know, but I don't see why it's a must.
> >>>
> >>>> So what progress has been made with the ACPI table specification, is it
> >>>> just a matter of time to get it approved or are there concerns?
> >>>>
> >>>> Regards,
> >>>>
> >>>> 	Joerg
> >>>
> >>
> > 
> 

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Jean-Philippe Brucker]

Hi Al,

On Tue, Nov 03, 2020 at 01:09:04PM -0700, Al Stone wrote:
> So, there are some questions about the VIOT definition and I just
> don't know enough to be able to answer them.  One of the ASWG members
> is trying to understand the semantics behind the subtables.

Thanks for the update. We dropped subtables a few versions ago, though, do
you have the latest v8?
https://jpbrucker.net/virtio-iommu/viot/viot-v8.pdf

> Is there a particular set of people, or mailing lists, that I can
> point to to get the questions answered?  Ideally it would be one
> of the public lists where it has already been discussed, but an
> individual would be fine, too.  No changes have been proposed, just
> some questions asked.

For a public list, I suggest iommu@lists.linux-foundation.org if we should
pick only one (otherwise add virtualization@lists.linux-foundation.org and
virtio-dev@lists.oasis-open.org). I'm happy to answer any question, and
the folks on here are a good set to Cc:

eric.auger@redhat.com
jean-philippe@linaro.org
joro@8bytes.org
kevin.tian@intel.com
lorenzo.pieralisi@arm.com
mst@redhat.com
sebastien.boeuf@intel.com

Thanks,
Jean

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Al Stone]

On 04 Nov 2020 10:33, Jean-Philippe Brucker wrote:
> Hi Al,
> 
> On Tue, Nov 03, 2020 at 01:09:04PM -0700, Al Stone wrote:
> > So, there are some questions about the VIOT definition and I just
> > don't know enough to be able to answer them.  One of the ASWG members
> > is trying to understand the semantics behind the subtables.
> 
> Thanks for the update. We dropped subtables a few versions ago, though, do
> you have the latest v8?
> https://jpbrucker.net/virtio-iommu/viot/viot-v8.pdf

Sorry, I confused some terminology: what are called the Node structures
are implemented as "subtables" in the ACPI reference implementation
(ACPICA).  But yes, I've proposed the v8 version.

> > Is there a particular set of people, or mailing lists, that I can
> > point to to get the questions answered?  Ideally it would be one
> > of the public lists where it has already been discussed, but an
> > individual would be fine, too.  No changes have been proposed, just
> > some questions asked.
> 
> For a public list, I suggest iommu@lists.linux-foundation.org if we should
> pick only one (otherwise add virtualization@lists.linux-foundation.org and
> virtio-dev@lists.oasis-open.org). I'm happy to answer any question, and
> the folks on here are a good set to Cc:
> 
> eric.auger@redhat.com
> jean-philippe@linaro.org
> joro@8bytes.org
> kevin.tian@intel.com
> lorenzo.pieralisi@arm.com
> mst@redhat.com
> sebastien.boeuf@intel.com
> 
> Thanks,
> Jean
> 

Merci, Jean-Philippe :).  I'll point the individual at you and the
iommu mailing list, and the CCs.  Sadly, I did not write down the
full question, nor the person's name (from Microsoft, possibly?)
and now seem to have completely forgotten both (it's been a long
few months...).  If I can find something in the meeting minutes,
I'll pass that on.

Thanks again for everyone's patience.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Joerg Roedel]

On Thu, Sep 24, 2020 at 05:38:13AM -0400, Michael S. Tsirkin wrote:
> On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote:
> > On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote:
> > > OK so this looks good. Can you pls repost with the minor tweak
> > > suggested and all acks included, and I will queue this?
> > 
> > My NACK still stands, as long as a few questions are open:
> > 
> > 	1) The format used here will be the same as in the ACPI table? I
> > 	   think the answer to this questions must be Yes, so this leads
> > 	   to the real question:
> 
> I am not sure it's a must.

It is, having only one parser for the ACPI and MMIO descriptions was one
of the selling points for MMIO in past discussions and I think it makes
sense to keep them in sync.

> We can always tweak the parser if there are slight differences
> between ACPI and virtio formats.

There is no guarantee that there only need to be "tweaks" until the
ACPI table format is stablized.

Regards,

	Joerg

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Gerd Hoffmann]

On Thu, Sep 24, 2020 at 12:02:55PM +0200, Joerg Roedel wrote:
> On Thu, Sep 24, 2020 at 05:38:13AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote:
> > > On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote:
> > > > OK so this looks good. Can you pls repost with the minor tweak
> > > > suggested and all acks included, and I will queue this?
> > > 
> > > My NACK still stands, as long as a few questions are open:
> > > 
> > > 	1) The format used here will be the same as in the ACPI table? I
> > > 	   think the answer to this questions must be Yes, so this leads
> > > 	   to the real question:
> > 
> > I am not sure it's a must.
> 
> It is, having only one parser for the ACPI and MMIO descriptions was one
> of the selling points for MMIO in past discussions and I think it makes
> sense to keep them in sync.

So that requirement basically kills the "we have something to play with
while the acpi table spec is in progress" argument.  Also note that qemu
microvm got acpi support meanwhile.

Are there other cases where neither ACPI nor DT are available?

take care,
  Gerd

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Jean-Philippe Brucker]

On Thu, Sep 24, 2020 at 12:02:55PM +0200, Joerg Roedel wrote:
> On Thu, Sep 24, 2020 at 05:38:13AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote:
> > > On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote:
> > > > OK so this looks good. Can you pls repost with the minor tweak
> > > > suggested and all acks included, and I will queue this?
> > > 
> > > My NACK still stands, as long as a few questions are open:
> > > 
> > > 	1) The format used here will be the same as in the ACPI table? I
> > > 	   think the answer to this questions must be Yes, so this leads
> > > 	   to the real question:
> > 
> > I am not sure it's a must.
> 
> It is, having only one parser for the ACPI and MMIO descriptions was one
> of the selling points for MMIO in past discussions and I think it makes
> sense to keep them in sync.

It's not possible to use exactly the same code for parsing. The access
methods are different (need to deal with port-IO for built-in description
on PCI, for example) and more importantly, the structure is different as
well. The ACPI table needs nodes for virtio-iommu while the built-in
description is contained in the virtio-iommu itself. So the endpoint nodes
point to virtio-iommu node on ACPI, while they don't need a pointer on the
built-in desc. I kept as much as possible common in structures and
implementation, but in the end we still need about 200 unique lines on
each side.

Thanks,
Jean

> 
> > We can always tweak the parser if there are slight differences
> > between ACPI and virtio formats.
> 
> There is no guarantee that there only need to be "tweaks" until the
> ACPI table format is stablized.
> 
> Regards,
> 
> 	Joerg
> 

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Joerg Roedel]

On Thu, Sep 24, 2020 at 12:29:53PM +0200, Jean-Philippe Brucker wrote:
> It's not possible to use exactly the same code for parsing.

The access methods can be separate and don't affect the parsing logic.

> The access methods are different (need to deal with port-IO for
> built-in description on PCI, for example) and more importantly, the
> structure is different as well. The ACPI table needs nodes for
> virtio-iommu while the built-in description is contained in the
> virtio-iommu itself. So the endpoint nodes point to virtio-iommu node
> on ACPI, while they don't need a pointer on the built-in desc. I kept
> as much as possible common in structures and implementation, but in
> the end we still need about 200 unique lines on each side.

Will it hurt the non-ACPI version to have the not-needed pointers anyway
to keep the parsers the same?


	Joerg

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Michael S. Tsirkin]

On Thu, Sep 24, 2020 at 12:02:55PM +0200, Joerg Roedel wrote:
> On Thu, Sep 24, 2020 at 05:38:13AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote:
> > > On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote:
> > > > OK so this looks good. Can you pls repost with the minor tweak
> > > > suggested and all acks included, and I will queue this?
> > > 
> > > My NACK still stands, as long as a few questions are open:
> > > 
> > > 	1) The format used here will be the same as in the ACPI table? I
> > > 	   think the answer to this questions must be Yes, so this leads
> > > 	   to the real question:
> > 
> > I am not sure it's a must.
> 
> It is, having only one parser for the ACPI and MMIO descriptions was one
> of the selling points for MMIO in past discussions and I think it makes
> sense to keep them in sync.
> 
> > We can always tweak the parser if there are slight differences
> > between ACPI and virtio formats.
> 
> There is no guarantee that there only need to be "tweaks" until the
> ACPI table format is stablized.
> 
> Regards,
> 
> 	Joerg

But this has nothing to do with Linux.  There is also no guarantee that
the two committees will decide to use exactly the same format. Once one
of them sets the format in stone, we can add support for that format to
linux. If another one is playing nice and uses the same format, we can
use the same parsers. If it doesn't linux will have to follow suit.

-- 
MST

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Joerg Roedel]

On Thu, Sep 24, 2020 at 08:41:21AM -0400, Michael S. Tsirkin wrote:
> But this has nothing to do with Linux.  There is also no guarantee that
> the two committees will decide to use exactly the same format. Once one
> of them sets the format in stone, we can add support for that format to
> linux. If another one is playing nice and uses the same format, we can
> use the same parsers. If it doesn't linux will have to follow suit.

Or Linux decides to support only one of the formats, which would then be
ACPI.

Regards,

	Joerg

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Michael S. Tsirkin]

On Thu, Sep 24, 2020 at 02:50:46PM +0200, Joerg Roedel wrote:
> On Thu, Sep 24, 2020 at 08:41:21AM -0400, Michael S. Tsirkin wrote:
> > But this has nothing to do with Linux.  There is also no guarantee that
> > the two committees will decide to use exactly the same format. Once one
> > of them sets the format in stone, we can add support for that format to
> > linux. If another one is playing nice and uses the same format, we can
> > use the same parsers. If it doesn't linux will have to follow suit.
> 
> Or Linux decides to support only one of the formats, which would then be
> ACPI.
> 
> Regards,
> 
> 	Joerg

It's really up to hypervisors not guests, linux as a guest can for sure
refuse to work somewhere, but that's normally not very attractive.

-- 
MST

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Jean-Philippe Brucker]

On Fri, Aug 21, 2020 at 03:15:34PM +0200, Jean-Philippe Brucker wrote:
> Add a topology description to the virtio-iommu driver and enable x86
> platforms.
> 
> Since [v2] we have made some progress on adding ACPI support for
> virtio-iommu, which is the preferred boot method on x86. It will be a
> new vendor-agnostic table describing para-virtual topologies in a
> minimal format. However some platforms don't use either ACPI or DT for
> booting (for example microvm), and will need the alternative topology
> description method proposed here. In addition, since the process to get
> a new ACPI table will take a long time, this provides a boot method even
> to ACPI-based platforms, if only temporarily for testing and
> development.
> 
> v3:
> * Add patch 1 that moves virtio-iommu to a subfolder.
> * Split the rest:
>   * Patch 2 adds topology-helper.c, which will be shared with the ACPI
>     support.
>   * Patch 4 adds definitions.
>   * Patch 5 adds parser in topology.c.
> * Address other comments.
> 
> Linux and QEMU patches available at:
> https://jpbrucker.net/git/linux virtio-iommu/devel
> https://jpbrucker.net/git/qemu virtio-iommu/devel

I'm parking this work again, until we make progress on the ACPI table, or
until a platform without ACPI and DT needs it. Until then, I've pushed v4
to my virtio-iommu/topo branch and will keep it rebased on master.

Thanks,
Jean

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Michael S. Tsirkin]

On Fri, Sep 25, 2020 at 10:48:06AM +0200, Jean-Philippe Brucker wrote:
> On Fri, Aug 21, 2020 at 03:15:34PM +0200, Jean-Philippe Brucker wrote:
> > Add a topology description to the virtio-iommu driver and enable x86
> > platforms.
> > 
> > Since [v2] we have made some progress on adding ACPI support for
> > virtio-iommu, which is the preferred boot method on x86. It will be a
> > new vendor-agnostic table describing para-virtual topologies in a
> > minimal format. However some platforms don't use either ACPI or DT for
> > booting (for example microvm), and will need the alternative topology
> > description method proposed here. In addition, since the process to get
> > a new ACPI table will take a long time, this provides a boot method even
> > to ACPI-based platforms, if only temporarily for testing and
> > development.
> > 
> > v3:
> > * Add patch 1 that moves virtio-iommu to a subfolder.
> > * Split the rest:
> >   * Patch 2 adds topology-helper.c, which will be shared with the ACPI
> >     support.
> >   * Patch 4 adds definitions.
> >   * Patch 5 adds parser in topology.c.
> > * Address other comments.
> > 
> > Linux and QEMU patches available at:
> > https://jpbrucker.net/git/linux virtio-iommu/devel
> > https://jpbrucker.net/git/qemu virtio-iommu/devel
> 
> I'm parking this work again, until we make progress on the ACPI table, or
> until a platform without ACPI and DT needs it. Until then, I've pushed v4
> to my virtio-iommu/topo branch and will keep it rebased on master.
> 
> Thanks,
> Jean

I think you guys need to work on virtio spec too, not too much left to
do there ...

-- 
MST

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Jean-Philippe Brucker]

On Fri, Sep 25, 2020 at 06:22:57AM -0400, Michael S. Tsirkin wrote:
> On Fri, Sep 25, 2020 at 10:48:06AM +0200, Jean-Philippe Brucker wrote:
> > On Fri, Aug 21, 2020 at 03:15:34PM +0200, Jean-Philippe Brucker wrote:
> > > Add a topology description to the virtio-iommu driver and enable x86
> > > platforms.
> > > 
> > > Since [v2] we have made some progress on adding ACPI support for
> > > virtio-iommu, which is the preferred boot method on x86. It will be a
> > > new vendor-agnostic table describing para-virtual topologies in a
> > > minimal format. However some platforms don't use either ACPI or DT for
> > > booting (for example microvm), and will need the alternative topology
> > > description method proposed here. In addition, since the process to get
> > > a new ACPI table will take a long time, this provides a boot method even
> > > to ACPI-based platforms, if only temporarily for testing and
> > > development.
> > > 
> > > v3:
> > > * Add patch 1 that moves virtio-iommu to a subfolder.
> > > * Split the rest:
> > >   * Patch 2 adds topology-helper.c, which will be shared with the ACPI
> > >     support.
> > >   * Patch 4 adds definitions.
> > >   * Patch 5 adds parser in topology.c.
> > > * Address other comments.
> > > 
> > > Linux and QEMU patches available at:
> > > https://jpbrucker.net/git/linux virtio-iommu/devel
> > > https://jpbrucker.net/git/qemu virtio-iommu/devel
> > 
> > I'm parking this work again, until we make progress on the ACPI table, or
> > until a platform without ACPI and DT needs it. Until then, I've pushed v4
> > to my virtio-iommu/topo branch and will keep it rebased on master.
> > 
> > Thanks,
> > Jean
> 
> I think you guys need to work on virtio spec too, not too much left to
> do there ...

I know it's ready and I'd really like to move on with this, but I'd rather
not commit it to the spec until we know it's going to be used at all. As
Gerd pointed out the one example we had, microvm, now supports ACPI. Since
we've kicked off the ACPI work anyway it isn't clear that the built-in
topology will be useful.

Thanks,
Jean

Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Michael S. Tsirkin]

On Fri, Sep 25, 2020 at 01:26:29PM +0200, Jean-Philippe Brucker wrote:
> On Fri, Sep 25, 2020 at 06:22:57AM -0400, Michael S. Tsirkin wrote:
> > On Fri, Sep 25, 2020 at 10:48:06AM +0200, Jean-Philippe Brucker wrote:
> > > On Fri, Aug 21, 2020 at 03:15:34PM +0200, Jean-Philippe Brucker wrote:
> > > > Add a topology description to the virtio-iommu driver and enable x86
> > > > platforms.
> > > > 
> > > > Since [v2] we have made some progress on adding ACPI support for
> > > > virtio-iommu, which is the preferred boot method on x86. It will be a
> > > > new vendor-agnostic table describing para-virtual topologies in a
> > > > minimal format. However some platforms don't use either ACPI or DT for
> > > > booting (for example microvm), and will need the alternative topology
> > > > description method proposed here. In addition, since the process to get
> > > > a new ACPI table will take a long time, this provides a boot method even
> > > > to ACPI-based platforms, if only temporarily for testing and
> > > > development.
> > > > 
> > > > v3:
> > > > * Add patch 1 that moves virtio-iommu to a subfolder.
> > > > * Split the rest:
> > > >   * Patch 2 adds topology-helper.c, which will be shared with the ACPI
> > > >     support.
> > > >   * Patch 4 adds definitions.
> > > >   * Patch 5 adds parser in topology.c.
> > > > * Address other comments.
> > > > 
> > > > Linux and QEMU patches available at:
> > > > https://jpbrucker.net/git/linux virtio-iommu/devel
> > > > https://jpbrucker.net/git/qemu virtio-iommu/devel
> > > 
> > > I'm parking this work again, until we make progress on the ACPI table, or
> > > until a platform without ACPI and DT needs it. Until then, I've pushed v4
> > > to my virtio-iommu/topo branch and will keep it rebased on master.
> > > 
> > > Thanks,
> > > Jean
> > 
> > I think you guys need to work on virtio spec too, not too much left to
> > do there ...
> 
> I know it's ready and I'd really like to move on with this, but I'd rather
> not commit it to the spec until we know it's going to be used at all. As
> Gerd pointed out the one example we had, microvm, now supports ACPI. Since
> we've kicked off the ACPI work anyway it isn't clear that the built-in
> topology will be useful.
> 
> Thanks,
> Jean

Many power platforms are OF based, thus without ACPI or DT support.

-- 
MST

Re: [virtio-dev] Re: [PATCH v3 0/6] Add virtio-iommu built-in topology

[Gerd Hoffmann]

  Hi,

> Many power platforms are OF based, thus without ACPI or DT support.

pseries has lots of stuff below /proc/device-tree.  Dunno whenever that
is the same kind of device tree we have on arm ...

take care,
  Gerd