[PATCH 0/5] iommu/virtio: Add identity domains

[PATCH 0/5] iommu/virtio: Add identity domains

[Jean-Philippe Brucker]

Support identity domains, allowing to only enable IOMMU protection for a
subset of endpoints (those assigned to userspace, for example). Users
may enable identity domains at compile time
(CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time (iommu.passthrough=1) or
runtime (/sys/kernel/iommu_groups/*/type = identity).

Patches 1-2 support identity domains using the optional
VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
spec, see [1] for the latest proposal.

Patches 3-5 add a fallback to identity mappings, when the feature is not
supported.

Note that this series doesn't touch the global bypass bit added by
VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU should
be attached to a domain, so global bypass isn't in use after endpoints
are probed. Before that, the global bypass policy is decided by the
hypervisor and firmware. So I don't think Linux needs to touch the
global bypass bit, but there are some patches available on my
virtio-iommu/bypass branch [2] to test it.

QEMU patches are on my virtio-iommu/bypass branch [3] (and the list)

[1] https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg07898.html
[2] https://jpbrucker.net/git/linux/log/?h=virtio-iommu/bypass
[3] https://jpbrucker.net/git/qemu/log/?h=virtio-iommu/bypass

Jean-Philippe Brucker (5):
  iommu/virtio: Add definitions for VIRTIO_IOMMU_F_BYPASS_CONFIG
  iommu/virtio: Support bypass domains
  iommu/virtio: Sort reserved regions
  iommu/virtio: Pass end address to viommu_add_mapping()
  iommu/virtio: Support identity-mapped domains

 include/uapi/linux/virtio_iommu.h |   8 ++-
 drivers/iommu/virtio-iommu.c      | 113 +++++++++++++++++++++++++-----
 2 files changed, 101 insertions(+), 20 deletions(-)

-- 
2.33.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 0/5] iommu/virtio: Add identity domains

[Jean-Philippe Brucker]

Support identity domains, allowing to only enable IOMMU protection for a
subset of endpoints (those assigned to userspace, for example). Users
may enable identity domains at compile time
(CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time (iommu.passthrough=1) or
runtime (/sys/kernel/iommu_groups/*/type = identity).

Patches 1-2 support identity domains using the optional
VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
spec, see [1] for the latest proposal.

Patches 3-5 add a fallback to identity mappings, when the feature is not
supported.

Note that this series doesn't touch the global bypass bit added by
VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU should
be attached to a domain, so global bypass isn't in use after endpoints
are probed. Before that, the global bypass policy is decided by the
hypervisor and firmware. So I don't think Linux needs to touch the
global bypass bit, but there are some patches available on my
virtio-iommu/bypass branch [2] to test it.

QEMU patches are on my virtio-iommu/bypass branch [3] (and the list)

[1] https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg07898.html
[2] https://jpbrucker.net/git/linux/log/?h=virtio-iommu/bypass
[3] https://jpbrucker.net/git/qemu/log/?h=virtio-iommu/bypass

Jean-Philippe Brucker (5):
  iommu/virtio: Add definitions for VIRTIO_IOMMU_F_BYPASS_CONFIG
  iommu/virtio: Support bypass domains
  iommu/virtio: Sort reserved regions
  iommu/virtio: Pass end address to viommu_add_mapping()
  iommu/virtio: Support identity-mapped domains

 include/uapi/linux/virtio_iommu.h |   8 ++-
 drivers/iommu/virtio-iommu.c      | 113 +++++++++++++++++++++++++-----
 2 files changed, 101 insertions(+), 20 deletions(-)

-- 
2.33.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 1/5] iommu/virtio: Add definitions for VIRTIO_IOMMU_F_BYPASS_CONFIG

[Jean-Philippe Brucker]

Add definitions for the VIRTIO_IOMMU_F_BYPASS_CONFIG, which supersedes
VIRTIO_IOMMU_F_BYPASS.

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

diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
index 237e36a280cb..cafd8cf7febf 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_BYPASS_CONFIG		6
 
 struct virtio_iommu_range_64 {
 	__le64					start;
@@ -36,6 +37,8 @@ struct virtio_iommu_config {
 	struct virtio_iommu_range_32		domain_range;
 	/* Probe buffer size */
 	__le32					probe_size;
+	__u8					bypass;
+	__u8					reserved[7];
 };
 
 /* Request types */
@@ -66,11 +69,14 @@ struct virtio_iommu_req_tail {
 	__u8					reserved[3];
 };
 
+#define VIRTIO_IOMMU_ATTACH_F_BYPASS		(1 << 0)
+
 struct virtio_iommu_req_attach {
 	struct virtio_iommu_req_head		head;
 	__le32					domain;
 	__le32					endpoint;
-	__u8					reserved[8];
+	__le32					flags;
+	__u8					reserved[4];
 	struct virtio_iommu_req_tail		tail;
 };
 
-- 
2.33.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 1/5] iommu/virtio: Add definitions for VIRTIO_IOMMU_F_BYPASS_CONFIG

[Jean-Philippe Brucker]

Add definitions for the VIRTIO_IOMMU_F_BYPASS_CONFIG, which supersedes
VIRTIO_IOMMU_F_BYPASS.

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

diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
index 237e36a280cb..cafd8cf7febf 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_BYPASS_CONFIG		6
 
 struct virtio_iommu_range_64 {
 	__le64					start;
@@ -36,6 +37,8 @@ struct virtio_iommu_config {
 	struct virtio_iommu_range_32		domain_range;
 	/* Probe buffer size */
 	__le32					probe_size;
+	__u8					bypass;
+	__u8					reserved[7];
 };
 
 /* Request types */
@@ -66,11 +69,14 @@ struct virtio_iommu_req_tail {
 	__u8					reserved[3];
 };
 
+#define VIRTIO_IOMMU_ATTACH_F_BYPASS		(1 << 0)
+
 struct virtio_iommu_req_attach {
 	struct virtio_iommu_req_head		head;
 	__le32					domain;
 	__le32					endpoint;
-	__u8					reserved[8];
+	__le32					flags;
+	__u8					reserved[4];
 	struct virtio_iommu_req_tail		tail;
 };
 
-- 
2.33.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 2/5] iommu/virtio: Support bypass domains

[Jean-Philippe Brucker]

The VIRTIO_IOMMU_F_BYPASS_CONFIG feature adds a new flag to the ATTACH
request, that creates a bypass domain. Use it to enable identity
domains.

When VIRTIO_IOMMU_F_BYPASS_CONFIG is not supported by the device, we
currently fail attaching to an identity domain. Future patches will
instead create identity mappings in this case.

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

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 80930ce04a16..ee8a7afd667b 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -71,6 +71,7 @@ struct viommu_domain {
 	struct rb_root_cached		mappings;
 
 	unsigned long			nr_endpoints;
+	bool				bypass;
 };
 
 struct viommu_endpoint {
@@ -587,7 +588,9 @@ static struct iommu_domain *viommu_domain_alloc(unsigned type)
 {
 	struct viommu_domain *vdomain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+	if (type != IOMMU_DOMAIN_UNMANAGED &&
+	    type != IOMMU_DOMAIN_DMA &&
+	    type != IOMMU_DOMAIN_IDENTITY)
 		return NULL;
 
 	vdomain = kzalloc(sizeof(*vdomain), GFP_KERNEL);
@@ -630,6 +633,17 @@ static int viommu_domain_finalise(struct viommu_endpoint *vdev,
 	vdomain->map_flags	= viommu->map_flags;
 	vdomain->viommu		= viommu;
 
+	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+		if (!virtio_has_feature(viommu->vdev,
+					VIRTIO_IOMMU_F_BYPASS_CONFIG)) {
+			ida_free(&viommu->domain_ids, vdomain->id);
+			vdomain->viommu = 0;
+			return -EOPNOTSUPP;
+		}
+
+		vdomain->bypass = true;
+	}
+
 	return 0;
 }
 
@@ -691,6 +705,9 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		.domain		= cpu_to_le32(vdomain->id),
 	};
 
+	if (vdomain->bypass)
+		req.flags |= cpu_to_le32(VIRTIO_IOMMU_ATTACH_F_BYPASS);
+
 	for (i = 0; i < fwspec->num_ids; i++) {
 		req.endpoint = cpu_to_le32(fwspec->ids[i]);
 
@@ -1132,6 +1149,7 @@ static unsigned int features[] = {
 	VIRTIO_IOMMU_F_DOMAIN_RANGE,
 	VIRTIO_IOMMU_F_PROBE,
 	VIRTIO_IOMMU_F_MMIO,
+	VIRTIO_IOMMU_F_BYPASS_CONFIG,
 };
 
 static struct virtio_device_id id_table[] = {
-- 
2.33.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 2/5] iommu/virtio: Support bypass domains

[Jean-Philippe Brucker]

The VIRTIO_IOMMU_F_BYPASS_CONFIG feature adds a new flag to the ATTACH
request, that creates a bypass domain. Use it to enable identity
domains.

When VIRTIO_IOMMU_F_BYPASS_CONFIG is not supported by the device, we
currently fail attaching to an identity domain. Future patches will
instead create identity mappings in this case.

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

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 80930ce04a16..ee8a7afd667b 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -71,6 +71,7 @@ struct viommu_domain {
 	struct rb_root_cached		mappings;
 
 	unsigned long			nr_endpoints;
+	bool				bypass;
 };
 
 struct viommu_endpoint {
@@ -587,7 +588,9 @@ static struct iommu_domain *viommu_domain_alloc(unsigned type)
 {
 	struct viommu_domain *vdomain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+	if (type != IOMMU_DOMAIN_UNMANAGED &&
+	    type != IOMMU_DOMAIN_DMA &&
+	    type != IOMMU_DOMAIN_IDENTITY)
 		return NULL;
 
 	vdomain = kzalloc(sizeof(*vdomain), GFP_KERNEL);
@@ -630,6 +633,17 @@ static int viommu_domain_finalise(struct viommu_endpoint *vdev,
 	vdomain->map_flags	= viommu->map_flags;
 	vdomain->viommu		= viommu;
 
+	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+		if (!virtio_has_feature(viommu->vdev,
+					VIRTIO_IOMMU_F_BYPASS_CONFIG)) {
+			ida_free(&viommu->domain_ids, vdomain->id);
+			vdomain->viommu = 0;
+			return -EOPNOTSUPP;
+		}
+
+		vdomain->bypass = true;
+	}
+
 	return 0;
 }
 
@@ -691,6 +705,9 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		.domain		= cpu_to_le32(vdomain->id),
 	};
 
+	if (vdomain->bypass)
+		req.flags |= cpu_to_le32(VIRTIO_IOMMU_ATTACH_F_BYPASS);
+
 	for (i = 0; i < fwspec->num_ids; i++) {
 		req.endpoint = cpu_to_le32(fwspec->ids[i]);
 
@@ -1132,6 +1149,7 @@ static unsigned int features[] = {
 	VIRTIO_IOMMU_F_DOMAIN_RANGE,
 	VIRTIO_IOMMU_F_PROBE,
 	VIRTIO_IOMMU_F_MMIO,
+	VIRTIO_IOMMU_F_BYPASS_CONFIG,
 };
 
 static struct virtio_device_id id_table[] = {
-- 
2.33.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 3/5] iommu/virtio: Sort reserved regions

[Jean-Philippe Brucker]

To ease identity mapping support, keep the list of reserved regions
sorted.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/iommu/virtio-iommu.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index ee8a7afd667b..d63ec4d11b00 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -423,7 +423,7 @@ static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
 	size_t size;
 	u64 start64, end64;
 	phys_addr_t start, end;
-	struct iommu_resv_region *region = NULL;
+	struct iommu_resv_region *region = NULL, *next;
 	unsigned long prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
 
 	start = start64 = le64_to_cpu(mem->start);
@@ -454,7 +454,12 @@ static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
 	if (!region)
 		return -ENOMEM;
 
-	list_add(&region->list, &vdev->resv_regions);
+	/* Keep the list sorted */
+	list_for_each_entry(next, &vdev->resv_regions, list) {
+		if (next->start > region->start)
+			break;
+	}
+	list_add_tail(&region->list, &next->list);
 	return 0;
 }
 
-- 
2.33.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 3/5] iommu/virtio: Sort reserved regions

[Jean-Philippe Brucker]

To ease identity mapping support, keep the list of reserved regions
sorted.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/iommu/virtio-iommu.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index ee8a7afd667b..d63ec4d11b00 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -423,7 +423,7 @@ static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
 	size_t size;
 	u64 start64, end64;
 	phys_addr_t start, end;
-	struct iommu_resv_region *region = NULL;
+	struct iommu_resv_region *region = NULL, *next;
 	unsigned long prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
 
 	start = start64 = le64_to_cpu(mem->start);
@@ -454,7 +454,12 @@ static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
 	if (!region)
 		return -ENOMEM;
 
-	list_add(&region->list, &vdev->resv_regions);
+	/* Keep the list sorted */
+	list_for_each_entry(next, &vdev->resv_regions, list) {
+		if (next->start > region->start)
+			break;
+	}
+	list_add_tail(&region->list, &next->list);
 	return 0;
 }
 
-- 
2.33.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 4/5] iommu/virtio: Pass end address to viommu_add_mapping()

[Jean-Philippe Brucker]

To support identity mappings, the virtio-iommu driver must be able to
represent full 64-bit ranges internally. Pass (start, end) instead of
(start, size) to viommu_add/del_mapping().

Clean comments. The one about the returned size was never true: when
sweeping the whole address space the returned size will most certainly
be smaller than 2^64.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/iommu/virtio-iommu.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index d63ec4d11b00..eceb9281c8c1 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -311,8 +311,8 @@ static int viommu_send_req_sync(struct viommu_dev *viommu, void *buf,
  *
  * On success, return the new mapping. Otherwise return NULL.
  */
-static int viommu_add_mapping(struct viommu_domain *vdomain, unsigned long iova,
-			      phys_addr_t paddr, size_t size, u32 flags)
+static int viommu_add_mapping(struct viommu_domain *vdomain, u64 iova, u64 end,
+			      phys_addr_t paddr, u32 flags)
 {
 	unsigned long irqflags;
 	struct viommu_mapping *mapping;
@@ -323,7 +323,7 @@ static int viommu_add_mapping(struct viommu_domain *vdomain, unsigned long iova,
 
 	mapping->paddr		= paddr;
 	mapping->iova.start	= iova;
-	mapping->iova.last	= iova + size - 1;
+	mapping->iova.last	= end;
 	mapping->flags		= flags;
 
 	spin_lock_irqsave(&vdomain->mappings_lock, irqflags);
@@ -338,26 +338,24 @@ static int viommu_add_mapping(struct viommu_domain *vdomain, unsigned long iova,
  *
  * @vdomain: the domain
  * @iova: start of the range
- * @size: size of the range. A size of 0 corresponds to the entire address
- *	space.
+ * @end: end of the range
  *
- * On success, returns the number of unmapped bytes (>= size)
+ * On success, returns the number of unmapped bytes
  */
 static size_t viommu_del_mappings(struct viommu_domain *vdomain,
-				  unsigned long iova, size_t size)
+				  u64 iova, u64 end)
 {
 	size_t unmapped = 0;
 	unsigned long flags;
-	unsigned long last = iova + size - 1;
 	struct viommu_mapping *mapping = NULL;
 	struct interval_tree_node *node, *next;
 
 	spin_lock_irqsave(&vdomain->mappings_lock, flags);
-	next = interval_tree_iter_first(&vdomain->mappings, iova, last);
+	next = interval_tree_iter_first(&vdomain->mappings, iova, end);
 	while (next) {
 		node = next;
 		mapping = container_of(node, struct viommu_mapping, iova);
-		next = interval_tree_iter_next(node, iova, last);
+		next = interval_tree_iter_next(node, iova, end);
 
 		/* Trying to split a mapping? */
 		if (mapping->iova.start < iova)
@@ -656,8 +654,8 @@ static void viommu_domain_free(struct iommu_domain *domain)
 {
 	struct viommu_domain *vdomain = to_viommu_domain(domain);
 
-	/* Free all remaining mappings (size 2^64) */
-	viommu_del_mappings(vdomain, 0, 0);
+	/* Free all remaining mappings */
+	viommu_del_mappings(vdomain, 0, ULLONG_MAX);
 
 	if (vdomain->viommu)
 		ida_free(&vdomain->viommu->domain_ids, vdomain->id);
@@ -742,6 +740,7 @@ static int viommu_map(struct iommu_domain *domain, unsigned long iova,
 {
 	int ret;
 	u32 flags;
+	u64 end = iova + size - 1;
 	struct virtio_iommu_req_map map;
 	struct viommu_domain *vdomain = to_viommu_domain(domain);
 
@@ -752,7 +751,7 @@ static int viommu_map(struct iommu_domain *domain, unsigned long iova,
 	if (flags & ~vdomain->map_flags)
 		return -EINVAL;
 
-	ret = viommu_add_mapping(vdomain, iova, paddr, size, flags);
+	ret = viommu_add_mapping(vdomain, iova, end, paddr, flags);
 	if (ret)
 		return ret;
 
@@ -761,7 +760,7 @@ static int viommu_map(struct iommu_domain *domain, unsigned long iova,
 		.domain		= cpu_to_le32(vdomain->id),
 		.virt_start	= cpu_to_le64(iova),
 		.phys_start	= cpu_to_le64(paddr),
-		.virt_end	= cpu_to_le64(iova + size - 1),
+		.virt_end	= cpu_to_le64(end),
 		.flags		= cpu_to_le32(flags),
 	};
 
@@ -770,7 +769,7 @@ static int viommu_map(struct iommu_domain *domain, unsigned long iova,
 
 	ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
 	if (ret)
-		viommu_del_mappings(vdomain, iova, size);
+		viommu_del_mappings(vdomain, iova, end);
 
 	return ret;
 }
@@ -783,7 +782,7 @@ static size_t viommu_unmap(struct iommu_domain *domain, unsigned long iova,
 	struct virtio_iommu_req_unmap unmap;
 	struct viommu_domain *vdomain = to_viommu_domain(domain);
 
-	unmapped = viommu_del_mappings(vdomain, iova, size);
+	unmapped = viommu_del_mappings(vdomain, iova, iova + size - 1);
 	if (unmapped < size)
 		return 0;
 
-- 
2.33.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 4/5] iommu/virtio: Pass end address to viommu_add_mapping()

[Jean-Philippe Brucker]

To support identity mappings, the virtio-iommu driver must be able to
represent full 64-bit ranges internally. Pass (start, end) instead of
(start, size) to viommu_add/del_mapping().

Clean comments. The one about the returned size was never true: when
sweeping the whole address space the returned size will most certainly
be smaller than 2^64.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/iommu/virtio-iommu.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index d63ec4d11b00..eceb9281c8c1 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -311,8 +311,8 @@ static int viommu_send_req_sync(struct viommu_dev *viommu, void *buf,
  *
  * On success, return the new mapping. Otherwise return NULL.
  */
-static int viommu_add_mapping(struct viommu_domain *vdomain, unsigned long iova,
-			      phys_addr_t paddr, size_t size, u32 flags)
+static int viommu_add_mapping(struct viommu_domain *vdomain, u64 iova, u64 end,
+			      phys_addr_t paddr, u32 flags)
 {
 	unsigned long irqflags;
 	struct viommu_mapping *mapping;
@@ -323,7 +323,7 @@ static int viommu_add_mapping(struct viommu_domain *vdomain, unsigned long iova,
 
 	mapping->paddr		= paddr;
 	mapping->iova.start	= iova;
-	mapping->iova.last	= iova + size - 1;
+	mapping->iova.last	= end;
 	mapping->flags		= flags;
 
 	spin_lock_irqsave(&vdomain->mappings_lock, irqflags);
@@ -338,26 +338,24 @@ static int viommu_add_mapping(struct viommu_domain *vdomain, unsigned long iova,
  *
  * @vdomain: the domain
  * @iova: start of the range
- * @size: size of the range. A size of 0 corresponds to the entire address
- *	space.
+ * @end: end of the range
  *
- * On success, returns the number of unmapped bytes (>= size)
+ * On success, returns the number of unmapped bytes
  */
 static size_t viommu_del_mappings(struct viommu_domain *vdomain,
-				  unsigned long iova, size_t size)
+				  u64 iova, u64 end)
 {
 	size_t unmapped = 0;
 	unsigned long flags;
-	unsigned long last = iova + size - 1;
 	struct viommu_mapping *mapping = NULL;
 	struct interval_tree_node *node, *next;
 
 	spin_lock_irqsave(&vdomain->mappings_lock, flags);
-	next = interval_tree_iter_first(&vdomain->mappings, iova, last);
+	next = interval_tree_iter_first(&vdomain->mappings, iova, end);
 	while (next) {
 		node = next;
 		mapping = container_of(node, struct viommu_mapping, iova);
-		next = interval_tree_iter_next(node, iova, last);
+		next = interval_tree_iter_next(node, iova, end);
 
 		/* Trying to split a mapping? */
 		if (mapping->iova.start < iova)
@@ -656,8 +654,8 @@ static void viommu_domain_free(struct iommu_domain *domain)
 {
 	struct viommu_domain *vdomain = to_viommu_domain(domain);
 
-	/* Free all remaining mappings (size 2^64) */
-	viommu_del_mappings(vdomain, 0, 0);
+	/* Free all remaining mappings */
+	viommu_del_mappings(vdomain, 0, ULLONG_MAX);
 
 	if (vdomain->viommu)
 		ida_free(&vdomain->viommu->domain_ids, vdomain->id);
@@ -742,6 +740,7 @@ static int viommu_map(struct iommu_domain *domain, unsigned long iova,
 {
 	int ret;
 	u32 flags;
+	u64 end = iova + size - 1;
 	struct virtio_iommu_req_map map;
 	struct viommu_domain *vdomain = to_viommu_domain(domain);
 
@@ -752,7 +751,7 @@ static int viommu_map(struct iommu_domain *domain, unsigned long iova,
 	if (flags & ~vdomain->map_flags)
 		return -EINVAL;
 
-	ret = viommu_add_mapping(vdomain, iova, paddr, size, flags);
+	ret = viommu_add_mapping(vdomain, iova, end, paddr, flags);
 	if (ret)
 		return ret;
 
@@ -761,7 +760,7 @@ static int viommu_map(struct iommu_domain *domain, unsigned long iova,
 		.domain		= cpu_to_le32(vdomain->id),
 		.virt_start	= cpu_to_le64(iova),
 		.phys_start	= cpu_to_le64(paddr),
-		.virt_end	= cpu_to_le64(iova + size - 1),
+		.virt_end	= cpu_to_le64(end),
 		.flags		= cpu_to_le32(flags),
 	};
 
@@ -770,7 +769,7 @@ static int viommu_map(struct iommu_domain *domain, unsigned long iova,
 
 	ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
 	if (ret)
-		viommu_del_mappings(vdomain, iova, size);
+		viommu_del_mappings(vdomain, iova, end);
 
 	return ret;
 }
@@ -783,7 +782,7 @@ static size_t viommu_unmap(struct iommu_domain *domain, unsigned long iova,
 	struct virtio_iommu_req_unmap unmap;
 	struct viommu_domain *vdomain = to_viommu_domain(domain);
 
-	unmapped = viommu_del_mappings(vdomain, iova, size);
+	unmapped = viommu_del_mappings(vdomain, iova, iova + size - 1);
 	if (unmapped < size)
 		return 0;
 
-- 
2.33.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 5/5] iommu/virtio: Support identity-mapped domains

[Jean-Philippe Brucker]

Support identity domains for devices that do not offer the
VIRTIO_IOMMU_F_BYPASS_CONFIG feature, by creating 1:1 mappings between
the virtual and physical address space. Identity domains created this
way still perform noticeably better than DMA domains, because they don't
have the overhead of setting up and tearing down mappings at runtime.
The performance difference between this and bypass is minimal in
comparison.

It does not matter that the physical addresses in the identity mappings
do not all correspond to memory. By enabling passthrough we are trusting
the device driver and the device itself to only perform DMA to suitable
locations. In some cases it may even be desirable to perform DMA to MMIO
regions.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/iommu/virtio-iommu.c | 61 +++++++++++++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index eceb9281c8c1..c9e8367d2962 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -375,6 +375,55 @@ static size_t viommu_del_mappings(struct viommu_domain *vdomain,
 	return unmapped;
 }
 
+/*
+ * Fill the domain with identity mappings, skipping the device's reserved
+ * regions.
+ */
+static int viommu_domain_map_identity(struct viommu_endpoint *vdev,
+				      struct viommu_domain *vdomain)
+{
+	int ret;
+	struct iommu_resv_region *resv;
+	u64 iova = vdomain->domain.geometry.aperture_start;
+	u64 limit = vdomain->domain.geometry.aperture_end;
+	u32 flags = VIRTIO_IOMMU_MAP_F_READ | VIRTIO_IOMMU_MAP_F_WRITE;
+	unsigned long granule = 1UL << __ffs(vdomain->domain.pgsize_bitmap);
+
+	iova = ALIGN(iova, granule);
+	limit = ALIGN_DOWN(limit + 1, granule) - 1;
+
+	list_for_each_entry(resv, &vdev->resv_regions, list) {
+		u64 resv_start = ALIGN_DOWN(resv->start, granule);
+		u64 resv_end = ALIGN(resv->start + resv->length, granule) - 1;
+
+		if (resv_end < iova || resv_start > limit)
+			/* No overlap */
+			continue;
+
+		if (resv_start > iova) {
+			ret = viommu_add_mapping(vdomain, iova, resv_start - 1,
+						 (phys_addr_t)iova, flags);
+			if (ret)
+				goto err_unmap;
+		}
+
+		if (resv_end >= limit)
+			return 0;
+
+		iova = resv_end + 1;
+	}
+
+	ret = viommu_add_mapping(vdomain, iova, limit, (phys_addr_t)iova,
+				 flags);
+	if (ret)
+		goto err_unmap;
+	return 0;
+
+err_unmap:
+	viommu_del_mappings(vdomain, 0, iova);
+	return ret;
+}
+
 /*
  * viommu_replay_mappings - re-send MAP requests
  *
@@ -637,14 +686,18 @@ static int viommu_domain_finalise(struct viommu_endpoint *vdev,
 	vdomain->viommu		= viommu;
 
 	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
-		if (!virtio_has_feature(viommu->vdev,
-					VIRTIO_IOMMU_F_BYPASS_CONFIG)) {
+		if (virtio_has_feature(viommu->vdev,
+				       VIRTIO_IOMMU_F_BYPASS_CONFIG)) {
+			vdomain->bypass = true;
+			return 0;
+		}
+
+		ret = viommu_domain_map_identity(vdev, vdomain);
+		if (ret) {
 			ida_free(&viommu->domain_ids, vdomain->id);
 			vdomain->viommu = 0;
 			return -EOPNOTSUPP;
 		}
-
-		vdomain->bypass = true;
 	}
 
 	return 0;
-- 
2.33.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 5/5] iommu/virtio: Support identity-mapped domains

[Jean-Philippe Brucker]

Support identity domains for devices that do not offer the
VIRTIO_IOMMU_F_BYPASS_CONFIG feature, by creating 1:1 mappings between
the virtual and physical address space. Identity domains created this
way still perform noticeably better than DMA domains, because they don't
have the overhead of setting up and tearing down mappings at runtime.
The performance difference between this and bypass is minimal in
comparison.

It does not matter that the physical addresses in the identity mappings
do not all correspond to memory. By enabling passthrough we are trusting
the device driver and the device itself to only perform DMA to suitable
locations. In some cases it may even be desirable to perform DMA to MMIO
regions.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/iommu/virtio-iommu.c | 61 +++++++++++++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index eceb9281c8c1..c9e8367d2962 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -375,6 +375,55 @@ static size_t viommu_del_mappings(struct viommu_domain *vdomain,
 	return unmapped;
 }
 
+/*
+ * Fill the domain with identity mappings, skipping the device's reserved
+ * regions.
+ */
+static int viommu_domain_map_identity(struct viommu_endpoint *vdev,
+				      struct viommu_domain *vdomain)
+{
+	int ret;
+	struct iommu_resv_region *resv;
+	u64 iova = vdomain->domain.geometry.aperture_start;
+	u64 limit = vdomain->domain.geometry.aperture_end;
+	u32 flags = VIRTIO_IOMMU_MAP_F_READ | VIRTIO_IOMMU_MAP_F_WRITE;
+	unsigned long granule = 1UL << __ffs(vdomain->domain.pgsize_bitmap);
+
+	iova = ALIGN(iova, granule);
+	limit = ALIGN_DOWN(limit + 1, granule) - 1;
+
+	list_for_each_entry(resv, &vdev->resv_regions, list) {
+		u64 resv_start = ALIGN_DOWN(resv->start, granule);
+		u64 resv_end = ALIGN(resv->start + resv->length, granule) - 1;
+
+		if (resv_end < iova || resv_start > limit)
+			/* No overlap */
+			continue;
+
+		if (resv_start > iova) {
+			ret = viommu_add_mapping(vdomain, iova, resv_start - 1,
+						 (phys_addr_t)iova, flags);
+			if (ret)
+				goto err_unmap;
+		}
+
+		if (resv_end >= limit)
+			return 0;
+
+		iova = resv_end + 1;
+	}
+
+	ret = viommu_add_mapping(vdomain, iova, limit, (phys_addr_t)iova,
+				 flags);
+	if (ret)
+		goto err_unmap;
+	return 0;
+
+err_unmap:
+	viommu_del_mappings(vdomain, 0, iova);
+	return ret;
+}
+
 /*
  * viommu_replay_mappings - re-send MAP requests
  *
@@ -637,14 +686,18 @@ static int viommu_domain_finalise(struct viommu_endpoint *vdev,
 	vdomain->viommu		= viommu;
 
 	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
-		if (!virtio_has_feature(viommu->vdev,
-					VIRTIO_IOMMU_F_BYPASS_CONFIG)) {
+		if (virtio_has_feature(viommu->vdev,
+				       VIRTIO_IOMMU_F_BYPASS_CONFIG)) {
+			vdomain->bypass = true;
+			return 0;
+		}
+
+		ret = viommu_domain_map_identity(vdev, vdomain);
+		if (ret) {
 			ida_free(&viommu->domain_ids, vdomain->id);
 			vdomain->viommu = 0;
 			return -EOPNOTSUPP;
 		}
-
-		vdomain->bypass = true;
 	}
 
 	return 0;
-- 
2.33.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH 0/5] iommu/virtio: Add identity domains

[Tian, Kevin]

> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Sent: Wednesday, October 13, 2021 8:11 PM
> 
> Support identity domains, allowing to only enable IOMMU protection for a
> subset of endpoints (those assigned to userspace, for example). Users
> may enable identity domains at compile time
> (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> (iommu.passthrough=1) or
> runtime (/sys/kernel/iommu_groups/*/type = identity).

Do we want to use consistent terms between spec (bypass domain) 
and code (identity domain)? 

> 
> Patches 1-2 support identity domains using the optional
> VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
> spec, see [1] for the latest proposal.
> 
> Patches 3-5 add a fallback to identity mappings, when the feature is not
> supported.
> 
> Note that this series doesn't touch the global bypass bit added by
> VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU
> should
> be attached to a domain, so global bypass isn't in use after endpoints

I saw a concept of deferred attach in iommu core. See iommu_is_
attach_deferred(). Currently this is vendor specific and I haven't
looked into the exact reason why some vendor sets it now. Just
be curious whether the same reason might be applied to virtio-iommu.

> are probed. Before that, the global bypass policy is decided by the
> hypervisor and firmware. So I don't think Linux needs to touch the

This reminds me one thing. The spec says that the global bypass
bit is sticky and not affected by reset. This implies that in the case
of rebooting the VM into a different OS, the previous OS actually
has the right to override this setting for the next OS. Is it a right
design? Even the firmware itself is unable to identify the original
setting enforced by the hypervisor after reboot. I feel the hypervisor
setting should be recovered after reset since it reflects the 
security measure enforced by the virtual platform?

> global bypass bit, but there are some patches available on my
> virtio-iommu/bypass branch [2] to test it.
> 
> QEMU patches are on my virtio-iommu/bypass branch [3] (and the list)
> 
> [1] https://www.mail-archive.com/virtio-dev@lists.oasis-
> open.org/msg07898.html
> [2] https://jpbrucker.net/git/linux/log/?h=virtio-iommu/bypass
> [3] https://jpbrucker.net/git/qemu/log/?h=virtio-iommu/bypass
> 
> Jean-Philippe Brucker (5):
>   iommu/virtio: Add definitions for VIRTIO_IOMMU_F_BYPASS_CONFIG
>   iommu/virtio: Support bypass domains
>   iommu/virtio: Sort reserved regions
>   iommu/virtio: Pass end address to viommu_add_mapping()
>   iommu/virtio: Support identity-mapped domains
> 
>  include/uapi/linux/virtio_iommu.h |   8 ++-
>  drivers/iommu/virtio-iommu.c      | 113 +++++++++++++++++++++++++-----
>  2 files changed, 101 insertions(+), 20 deletions(-)
> 
> --
> 2.33.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH 0/5] iommu/virtio: Add identity domains

[Tian, Kevin]

> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Sent: Wednesday, October 13, 2021 8:11 PM
> 
> Support identity domains, allowing to only enable IOMMU protection for a
> subset of endpoints (those assigned to userspace, for example). Users
> may enable identity domains at compile time
> (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> (iommu.passthrough=1) or
> runtime (/sys/kernel/iommu_groups/*/type = identity).

Do we want to use consistent terms between spec (bypass domain) 
and code (identity domain)? 

> 
> Patches 1-2 support identity domains using the optional
> VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
> spec, see [1] for the latest proposal.
> 
> Patches 3-5 add a fallback to identity mappings, when the feature is not
> supported.
> 
> Note that this series doesn't touch the global bypass bit added by
> VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU
> should
> be attached to a domain, so global bypass isn't in use after endpoints

I saw a concept of deferred attach in iommu core. See iommu_is_
attach_deferred(). Currently this is vendor specific and I haven't
looked into the exact reason why some vendor sets it now. Just
be curious whether the same reason might be applied to virtio-iommu.

> are probed. Before that, the global bypass policy is decided by the
> hypervisor and firmware. So I don't think Linux needs to touch the

This reminds me one thing. The spec says that the global bypass
bit is sticky and not affected by reset. This implies that in the case
of rebooting the VM into a different OS, the previous OS actually
has the right to override this setting for the next OS. Is it a right
design? Even the firmware itself is unable to identify the original
setting enforced by the hypervisor after reboot. I feel the hypervisor
setting should be recovered after reset since it reflects the 
security measure enforced by the virtual platform?

> global bypass bit, but there are some patches available on my
> virtio-iommu/bypass branch [2] to test it.
> 
> QEMU patches are on my virtio-iommu/bypass branch [3] (and the list)
> 
> [1] https://www.mail-archive.com/virtio-dev@lists.oasis-
> open.org/msg07898.html
> [2] https://jpbrucker.net/git/linux/log/?h=virtio-iommu/bypass
> [3] https://jpbrucker.net/git/qemu/log/?h=virtio-iommu/bypass
> 
> Jean-Philippe Brucker (5):
>   iommu/virtio: Add definitions for VIRTIO_IOMMU_F_BYPASS_CONFIG
>   iommu/virtio: Support bypass domains
>   iommu/virtio: Sort reserved regions
>   iommu/virtio: Pass end address to viommu_add_mapping()
>   iommu/virtio: Support identity-mapped domains
> 
>  include/uapi/linux/virtio_iommu.h |   8 ++-
>  drivers/iommu/virtio-iommu.c      | 113 +++++++++++++++++++++++++-----
>  2 files changed, 101 insertions(+), 20 deletions(-)
> 
> --
> 2.33.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH 2/5] iommu/virtio: Support bypass domains

[Tian, Kevin]

> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Sent: Wednesday, October 13, 2021 8:11 PM
> 
> The VIRTIO_IOMMU_F_BYPASS_CONFIG feature adds a new flag to the
> ATTACH
> request, that creates a bypass domain. Use it to enable identity
> domains.
> 
> When VIRTIO_IOMMU_F_BYPASS_CONFIG is not supported by the device,
> we
> currently fail attaching to an identity domain. Future patches will
> instead create identity mappings in this case.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  drivers/iommu/virtio-iommu.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 80930ce04a16..ee8a7afd667b 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -71,6 +71,7 @@ struct viommu_domain {
>  	struct rb_root_cached		mappings;
> 
>  	unsigned long			nr_endpoints;
> +	bool				bypass;
>  };
> 
>  struct viommu_endpoint {
> @@ -587,7 +588,9 @@ static struct iommu_domain
> *viommu_domain_alloc(unsigned type)
>  {
>  	struct viommu_domain *vdomain;
> 
> -	if (type != IOMMU_DOMAIN_UNMANAGED && type !=
> IOMMU_DOMAIN_DMA)
> +	if (type != IOMMU_DOMAIN_UNMANAGED &&
> +	    type != IOMMU_DOMAIN_DMA &&
> +	    type != IOMMU_DOMAIN_IDENTITY)
>  		return NULL;
> 
>  	vdomain = kzalloc(sizeof(*vdomain), GFP_KERNEL);
> @@ -630,6 +633,17 @@ static int viommu_domain_finalise(struct
> viommu_endpoint *vdev,
>  	vdomain->map_flags	= viommu->map_flags;
>  	vdomain->viommu		= viommu;
> 
> +	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> +		if (!virtio_has_feature(viommu->vdev,
> +					VIRTIO_IOMMU_F_BYPASS_CONFIG))
> {
> +			ida_free(&viommu->domain_ids, vdomain->id);
> +			vdomain->viommu = 0;
> +			return -EOPNOTSUPP;
> +		}
> +
> +		vdomain->bypass = true;
> +	}
> +

move to the start of the function, then no need for above cleanup.

>  	return 0;
>  }
> 
> @@ -691,6 +705,9 @@ static int viommu_attach_dev(struct iommu_domain
> *domain, struct device *dev)
>  		.domain		= cpu_to_le32(vdomain->id),
>  	};
> 
> +	if (vdomain->bypass)
> +		req.flags |=
> cpu_to_le32(VIRTIO_IOMMU_ATTACH_F_BYPASS);
> +
>  	for (i = 0; i < fwspec->num_ids; i++) {
>  		req.endpoint = cpu_to_le32(fwspec->ids[i]);
> 
> @@ -1132,6 +1149,7 @@ static unsigned int features[] = {
>  	VIRTIO_IOMMU_F_DOMAIN_RANGE,
>  	VIRTIO_IOMMU_F_PROBE,
>  	VIRTIO_IOMMU_F_MMIO,
> +	VIRTIO_IOMMU_F_BYPASS_CONFIG,
>  };
> 
>  static struct virtio_device_id id_table[] = {
> --
> 2.33.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH 2/5] iommu/virtio: Support bypass domains

[Tian, Kevin]

> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Sent: Wednesday, October 13, 2021 8:11 PM
> 
> The VIRTIO_IOMMU_F_BYPASS_CONFIG feature adds a new flag to the
> ATTACH
> request, that creates a bypass domain. Use it to enable identity
> domains.
> 
> When VIRTIO_IOMMU_F_BYPASS_CONFIG is not supported by the device,
> we
> currently fail attaching to an identity domain. Future patches will
> instead create identity mappings in this case.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  drivers/iommu/virtio-iommu.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 80930ce04a16..ee8a7afd667b 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -71,6 +71,7 @@ struct viommu_domain {
>  	struct rb_root_cached		mappings;
> 
>  	unsigned long			nr_endpoints;
> +	bool				bypass;
>  };
> 
>  struct viommu_endpoint {
> @@ -587,7 +588,9 @@ static struct iommu_domain
> *viommu_domain_alloc(unsigned type)
>  {
>  	struct viommu_domain *vdomain;
> 
> -	if (type != IOMMU_DOMAIN_UNMANAGED && type !=
> IOMMU_DOMAIN_DMA)
> +	if (type != IOMMU_DOMAIN_UNMANAGED &&
> +	    type != IOMMU_DOMAIN_DMA &&
> +	    type != IOMMU_DOMAIN_IDENTITY)
>  		return NULL;
> 
>  	vdomain = kzalloc(sizeof(*vdomain), GFP_KERNEL);
> @@ -630,6 +633,17 @@ static int viommu_domain_finalise(struct
> viommu_endpoint *vdev,
>  	vdomain->map_flags	= viommu->map_flags;
>  	vdomain->viommu		= viommu;
> 
> +	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> +		if (!virtio_has_feature(viommu->vdev,
> +					VIRTIO_IOMMU_F_BYPASS_CONFIG))
> {
> +			ida_free(&viommu->domain_ids, vdomain->id);
> +			vdomain->viommu = 0;
> +			return -EOPNOTSUPP;
> +		}
> +
> +		vdomain->bypass = true;
> +	}
> +

move to the start of the function, then no need for above cleanup.

>  	return 0;
>  }
> 
> @@ -691,6 +705,9 @@ static int viommu_attach_dev(struct iommu_domain
> *domain, struct device *dev)
>  		.domain		= cpu_to_le32(vdomain->id),
>  	};
> 
> +	if (vdomain->bypass)
> +		req.flags |=
> cpu_to_le32(VIRTIO_IOMMU_ATTACH_F_BYPASS);
> +
>  	for (i = 0; i < fwspec->num_ids; i++) {
>  		req.endpoint = cpu_to_le32(fwspec->ids[i]);
> 
> @@ -1132,6 +1149,7 @@ static unsigned int features[] = {
>  	VIRTIO_IOMMU_F_DOMAIN_RANGE,
>  	VIRTIO_IOMMU_F_PROBE,
>  	VIRTIO_IOMMU_F_MMIO,
> +	VIRTIO_IOMMU_F_BYPASS_CONFIG,
>  };
> 
>  static struct virtio_device_id id_table[] = {
> --
> 2.33.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH 2/5] iommu/virtio: Support bypass domains

[Tian, Kevin]

> From: Tian, Kevin
> Sent: Thursday, October 14, 2021 11:25 AM
> 
> > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > Sent: Wednesday, October 13, 2021 8:11 PM
> >
> > The VIRTIO_IOMMU_F_BYPASS_CONFIG feature adds a new flag to the
> > ATTACH
> > request, that creates a bypass domain. Use it to enable identity
> > domains.
> >
> > When VIRTIO_IOMMU_F_BYPASS_CONFIG is not supported by the device,
> > we
> > currently fail attaching to an identity domain. Future patches will
> > instead create identity mappings in this case.
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> >  drivers/iommu/virtio-iommu.c | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> > index 80930ce04a16..ee8a7afd667b 100644
> > --- a/drivers/iommu/virtio-iommu.c
> > +++ b/drivers/iommu/virtio-iommu.c
> > @@ -71,6 +71,7 @@ struct viommu_domain {
> >  	struct rb_root_cached		mappings;
> >
> >  	unsigned long			nr_endpoints;
> > +	bool				bypass;
> >  };
> >
> >  struct viommu_endpoint {
> > @@ -587,7 +588,9 @@ static struct iommu_domain
> > *viommu_domain_alloc(unsigned type)
> >  {
> >  	struct viommu_domain *vdomain;
> >
> > -	if (type != IOMMU_DOMAIN_UNMANAGED && type !=
> > IOMMU_DOMAIN_DMA)
> > +	if (type != IOMMU_DOMAIN_UNMANAGED &&
> > +	    type != IOMMU_DOMAIN_DMA &&
> > +	    type != IOMMU_DOMAIN_IDENTITY)
> >  		return NULL;
> >
> >  	vdomain = kzalloc(sizeof(*vdomain), GFP_KERNEL);
> > @@ -630,6 +633,17 @@ static int viommu_domain_finalise(struct
> > viommu_endpoint *vdev,
> >  	vdomain->map_flags	= viommu->map_flags;
> >  	vdomain->viommu		= viommu;
> >
> > +	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> > +		if (!virtio_has_feature(viommu->vdev,
> > +					VIRTIO_IOMMU_F_BYPASS_CONFIG))
> > {
> > +			ida_free(&viommu->domain_ids, vdomain->id);
> > +			vdomain->viommu = 0;
> > +			return -EOPNOTSUPP;
> > +		}
> > +
> > +		vdomain->bypass = true;
> > +	}
> > +
> 
> move to the start of the function, then no need for above cleanup.
> 

forgot it as I see the reason now when looking at patch05
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH 2/5] iommu/virtio: Support bypass domains

[Tian, Kevin]

> From: Tian, Kevin
> Sent: Thursday, October 14, 2021 11:25 AM
> 
> > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > Sent: Wednesday, October 13, 2021 8:11 PM
> >
> > The VIRTIO_IOMMU_F_BYPASS_CONFIG feature adds a new flag to the
> > ATTACH
> > request, that creates a bypass domain. Use it to enable identity
> > domains.
> >
> > When VIRTIO_IOMMU_F_BYPASS_CONFIG is not supported by the device,
> > we
> > currently fail attaching to an identity domain. Future patches will
> > instead create identity mappings in this case.
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> >  drivers/iommu/virtio-iommu.c | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> > index 80930ce04a16..ee8a7afd667b 100644
> > --- a/drivers/iommu/virtio-iommu.c
> > +++ b/drivers/iommu/virtio-iommu.c
> > @@ -71,6 +71,7 @@ struct viommu_domain {
> >  	struct rb_root_cached		mappings;
> >
> >  	unsigned long			nr_endpoints;
> > +	bool				bypass;
> >  };
> >
> >  struct viommu_endpoint {
> > @@ -587,7 +588,9 @@ static struct iommu_domain
> > *viommu_domain_alloc(unsigned type)
> >  {
> >  	struct viommu_domain *vdomain;
> >
> > -	if (type != IOMMU_DOMAIN_UNMANAGED && type !=
> > IOMMU_DOMAIN_DMA)
> > +	if (type != IOMMU_DOMAIN_UNMANAGED &&
> > +	    type != IOMMU_DOMAIN_DMA &&
> > +	    type != IOMMU_DOMAIN_IDENTITY)
> >  		return NULL;
> >
> >  	vdomain = kzalloc(sizeof(*vdomain), GFP_KERNEL);
> > @@ -630,6 +633,17 @@ static int viommu_domain_finalise(struct
> > viommu_endpoint *vdev,
> >  	vdomain->map_flags	= viommu->map_flags;
> >  	vdomain->viommu		= viommu;
> >
> > +	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> > +		if (!virtio_has_feature(viommu->vdev,
> > +					VIRTIO_IOMMU_F_BYPASS_CONFIG))
> > {
> > +			ida_free(&viommu->domain_ids, vdomain->id);
> > +			vdomain->viommu = 0;
> > +			return -EOPNOTSUPP;
> > +		}
> > +
> > +		vdomain->bypass = true;
> > +	}
> > +
> 
> move to the start of the function, then no need for above cleanup.
> 

forgot it as I see the reason now when looking at patch05
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/5] iommu/virtio: Add identity domains

[joro]

On Thu, Oct 14, 2021 at 03:00:38AM +0000, Tian, Kevin wrote:
> I saw a concept of deferred attach in iommu core. See iommu_is_
> attach_deferred(). Currently this is vendor specific and I haven't
> looked into the exact reason why some vendor sets it now. Just
> be curious whether the same reason might be applied to virtio-iommu.

The reason for attach_deferred is kdump support, where the IOMMU driver
needs to keep the mappings from the old kernel until the device driver
of the new kernel takes over.

Regards,

	Joerg
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/5] iommu/virtio: Add identity domains

[joro]

On Thu, Oct 14, 2021 at 03:00:38AM +0000, Tian, Kevin wrote:
> I saw a concept of deferred attach in iommu core. See iommu_is_
> attach_deferred(). Currently this is vendor specific and I haven't
> looked into the exact reason why some vendor sets it now. Just
> be curious whether the same reason might be applied to virtio-iommu.

The reason for attach_deferred is kdump support, where the IOMMU driver
needs to keep the mappings from the old kernel until the device driver
of the new kernel takes over.

Regards,

	Joerg
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/5] iommu/virtio: Add identity domains

[Jean-Philippe Brucker]

On Thu, Oct 14, 2021 at 03:00:38AM +0000, Tian, Kevin wrote:
> > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > Sent: Wednesday, October 13, 2021 8:11 PM
> > 
> > Support identity domains, allowing to only enable IOMMU protection for a
> > subset of endpoints (those assigned to userspace, for example). Users
> > may enable identity domains at compile time
> > (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> > (iommu.passthrough=1) or
> > runtime (/sys/kernel/iommu_groups/*/type = identity).
> 
> Do we want to use consistent terms between spec (bypass domain) 
> and code (identity domain)? 

I don't think we have to. Linux uses "identity" domains and "passthrough"
IOMMU. The old virtio-iommu feature was "bypass" so we should keep that
for the new one, to be consistent. And then I've used "bypass" for domains
as well, in the spec, because it would look strange to use a different
term for the same concept. I find that it sort of falls into place: Linux'
identity domains can be implemented either with bypass or identity-mapped
virtio-iommu domains.

> > 
> > Patches 1-2 support identity domains using the optional
> > VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
> > spec, see [1] for the latest proposal.
> > 
> > Patches 3-5 add a fallback to identity mappings, when the feature is not
> > supported.
> > 
> > Note that this series doesn't touch the global bypass bit added by
> > VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU
> > should
> > be attached to a domain, so global bypass isn't in use after endpoints
> 
> I saw a concept of deferred attach in iommu core. See iommu_is_
> attach_deferred(). Currently this is vendor specific and I haven't
> looked into the exact reason why some vendor sets it now. Just
> be curious whether the same reason might be applied to virtio-iommu.
> 
> > are probed. Before that, the global bypass policy is decided by the
> > hypervisor and firmware. So I don't think Linux needs to touch the
> 
> This reminds me one thing. The spec says that the global bypass
> bit is sticky and not affected by reset.

The spec talks about *device* reset, triggered by software writing 0 to
the status register, but it doesn't mention system reset. Would be good to
clarify that. Something like:

    If the device offers the VIRTIO_IOMMU_F_BYPASS_CONFIG feature, it MAY
    initialize the \field{bypass} field to 1. Field \field{bypass} SHOULD
    NOT change on device reset, but SHOULD be restored to its initial
    value on system reset.

> This implies that in the case
> of rebooting the VM into a different OS, the previous OS actually
> has the right to override this setting for the next OS. Is it a right
> design? Even the firmware itself is unable to identify the original
> setting enforced by the hypervisor after reboot. I feel the hypervisor
> setting should be recovered after reset since it reflects the 
> security measure enforced by the virtual platform?

So I think clarifying system reset should address your questions.
I believe we should leave bypass sticky across device reset, so a FW->OS
transition, where the OS resets the device, does not open a vulnerability
(if bypass was enabled at boot and then left disabled by FW.)

It's still a good idea for the OS to restore on shutdown the bypass value
it was booted with. So it can kexec into an OS that doesn't support
virtio-iommu, for example.

Thanks,
Jean

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/5] iommu/virtio: Add identity domains

[Jean-Philippe Brucker]

On Thu, Oct 14, 2021 at 03:00:38AM +0000, Tian, Kevin wrote:
> > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > Sent: Wednesday, October 13, 2021 8:11 PM
> > 
> > Support identity domains, allowing to only enable IOMMU protection for a
> > subset of endpoints (those assigned to userspace, for example). Users
> > may enable identity domains at compile time
> > (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> > (iommu.passthrough=1) or
> > runtime (/sys/kernel/iommu_groups/*/type = identity).
> 
> Do we want to use consistent terms between spec (bypass domain) 
> and code (identity domain)? 

I don't think we have to. Linux uses "identity" domains and "passthrough"
IOMMU. The old virtio-iommu feature was "bypass" so we should keep that
for the new one, to be consistent. And then I've used "bypass" for domains
as well, in the spec, because it would look strange to use a different
term for the same concept. I find that it sort of falls into place: Linux'
identity domains can be implemented either with bypass or identity-mapped
virtio-iommu domains.

> > 
> > Patches 1-2 support identity domains using the optional
> > VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
> > spec, see [1] for the latest proposal.
> > 
> > Patches 3-5 add a fallback to identity mappings, when the feature is not
> > supported.
> > 
> > Note that this series doesn't touch the global bypass bit added by
> > VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU
> > should
> > be attached to a domain, so global bypass isn't in use after endpoints
> 
> I saw a concept of deferred attach in iommu core. See iommu_is_
> attach_deferred(). Currently this is vendor specific and I haven't
> looked into the exact reason why some vendor sets it now. Just
> be curious whether the same reason might be applied to virtio-iommu.
> 
> > are probed. Before that, the global bypass policy is decided by the
> > hypervisor and firmware. So I don't think Linux needs to touch the
> 
> This reminds me one thing. The spec says that the global bypass
> bit is sticky and not affected by reset.

The spec talks about *device* reset, triggered by software writing 0 to
the status register, but it doesn't mention system reset. Would be good to
clarify that. Something like:

    If the device offers the VIRTIO_IOMMU_F_BYPASS_CONFIG feature, it MAY
    initialize the \field{bypass} field to 1. Field \field{bypass} SHOULD
    NOT change on device reset, but SHOULD be restored to its initial
    value on system reset.

> This implies that in the case
> of rebooting the VM into a different OS, the previous OS actually
> has the right to override this setting for the next OS. Is it a right
> design? Even the firmware itself is unable to identify the original
> setting enforced by the hypervisor after reboot. I feel the hypervisor
> setting should be recovered after reset since it reflects the 
> security measure enforced by the virtual platform?

So I think clarifying system reset should address your questions.
I believe we should leave bypass sticky across device reset, so a FW->OS
transition, where the OS resets the device, does not open a vulnerability
(if bypass was enabled at boot and then left disabled by FW.)

It's still a good idea for the OS to restore on shutdown the bypass value
it was booted with. So it can kexec into an OS that doesn't support
virtio-iommu, for example.

Thanks,
Jean

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/5] iommu/virtio: Add identity domains

[Michael S. Tsirkin]

On Mon, Oct 18, 2021 at 04:23:41PM +0100, Jean-Philippe Brucker wrote:
> On Thu, Oct 14, 2021 at 03:00:38AM +0000, Tian, Kevin wrote:
> > > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > > Sent: Wednesday, October 13, 2021 8:11 PM
> > > 
> > > Support identity domains, allowing to only enable IOMMU protection for a
> > > subset of endpoints (those assigned to userspace, for example). Users
> > > may enable identity domains at compile time
> > > (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> > > (iommu.passthrough=1) or
> > > runtime (/sys/kernel/iommu_groups/*/type = identity).
> > 
> > Do we want to use consistent terms between spec (bypass domain) 
> > and code (identity domain)? 
> 
> I don't think we have to. Linux uses "identity" domains and "passthrough"
> IOMMU. The old virtio-iommu feature was "bypass" so we should keep that
> for the new one, to be consistent. And then I've used "bypass" for domains
> as well, in the spec, because it would look strange to use a different
> term for the same concept. I find that it sort of falls into place: Linux'
> identity domains can be implemented either with bypass or identity-mapped
> virtio-iommu domains.
> 
> > > 
> > > Patches 1-2 support identity domains using the optional
> > > VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
> > > spec, see [1] for the latest proposal.
> > > 
> > > Patches 3-5 add a fallback to identity mappings, when the feature is not
> > > supported.
> > > 
> > > Note that this series doesn't touch the global bypass bit added by
> > > VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU
> > > should
> > > be attached to a domain, so global bypass isn't in use after endpoints
> > 
> > I saw a concept of deferred attach in iommu core. See iommu_is_
> > attach_deferred(). Currently this is vendor specific and I haven't
> > looked into the exact reason why some vendor sets it now. Just
> > be curious whether the same reason might be applied to virtio-iommu.
> > 
> > > are probed. Before that, the global bypass policy is decided by the
> > > hypervisor and firmware. So I don't think Linux needs to touch the
> > 
> > This reminds me one thing. The spec says that the global bypass
> > bit is sticky and not affected by reset.
> 
> The spec talks about *device* reset, triggered by software writing 0 to
> the status register, but it doesn't mention system reset. Would be good to
> clarify that. Something like:
> 
>     If the device offers the VIRTIO_IOMMU_F_BYPASS_CONFIG feature, it MAY
>     initialize the \field{bypass} field to 1. Field \field{bypass} SHOULD
>     NOT change on device reset, but SHOULD be restored to its initial
>     value on system reset.
> 
> > This implies that in the case
> > of rebooting the VM into a different OS, the previous OS actually
> > has the right to override this setting for the next OS. Is it a right
> > design? Even the firmware itself is unable to identify the original
> > setting enforced by the hypervisor after reboot. I feel the hypervisor
> > setting should be recovered after reset since it reflects the 
> > security measure enforced by the virtual platform?
> 
> So I think clarifying system reset should address your questions.
> I believe we should leave bypass sticky across device reset, so a FW->OS
> transition, where the OS resets the device, does not open a vulnerability
> (if bypass was enabled at boot and then left disabled by FW.)
> 
> It's still a good idea for the OS to restore on shutdown the bypass value
> it was booted with. So it can kexec into an OS that doesn't support
> virtio-iommu, for example.
> 
> Thanks,
> Jean

Is this stickiness really important? Can't this be addressed just by
hypervisor disabling bypass at boot?

-- 
MST

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/5] iommu/virtio: Add identity domains

[Michael S. Tsirkin]

On Mon, Oct 18, 2021 at 04:23:41PM +0100, Jean-Philippe Brucker wrote:
> On Thu, Oct 14, 2021 at 03:00:38AM +0000, Tian, Kevin wrote:
> > > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > > Sent: Wednesday, October 13, 2021 8:11 PM
> > > 
> > > Support identity domains, allowing to only enable IOMMU protection for a
> > > subset of endpoints (those assigned to userspace, for example). Users
> > > may enable identity domains at compile time
> > > (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> > > (iommu.passthrough=1) or
> > > runtime (/sys/kernel/iommu_groups/*/type = identity).
> > 
> > Do we want to use consistent terms between spec (bypass domain) 
> > and code (identity domain)? 
> 
> I don't think we have to. Linux uses "identity" domains and "passthrough"
> IOMMU. The old virtio-iommu feature was "bypass" so we should keep that
> for the new one, to be consistent. And then I've used "bypass" for domains
> as well, in the spec, because it would look strange to use a different
> term for the same concept. I find that it sort of falls into place: Linux'
> identity domains can be implemented either with bypass or identity-mapped
> virtio-iommu domains.
> 
> > > 
> > > Patches 1-2 support identity domains using the optional
> > > VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
> > > spec, see [1] for the latest proposal.
> > > 
> > > Patches 3-5 add a fallback to identity mappings, when the feature is not
> > > supported.
> > > 
> > > Note that this series doesn't touch the global bypass bit added by
> > > VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU
> > > should
> > > be attached to a domain, so global bypass isn't in use after endpoints
> > 
> > I saw a concept of deferred attach in iommu core. See iommu_is_
> > attach_deferred(). Currently this is vendor specific and I haven't
> > looked into the exact reason why some vendor sets it now. Just
> > be curious whether the same reason might be applied to virtio-iommu.
> > 
> > > are probed. Before that, the global bypass policy is decided by the
> > > hypervisor and firmware. So I don't think Linux needs to touch the
> > 
> > This reminds me one thing. The spec says that the global bypass
> > bit is sticky and not affected by reset.
> 
> The spec talks about *device* reset, triggered by software writing 0 to
> the status register, but it doesn't mention system reset. Would be good to
> clarify that. Something like:
> 
>     If the device offers the VIRTIO_IOMMU_F_BYPASS_CONFIG feature, it MAY
>     initialize the \field{bypass} field to 1. Field \field{bypass} SHOULD
>     NOT change on device reset, but SHOULD be restored to its initial
>     value on system reset.
> 
> > This implies that in the case
> > of rebooting the VM into a different OS, the previous OS actually
> > has the right to override this setting for the next OS. Is it a right
> > design? Even the firmware itself is unable to identify the original
> > setting enforced by the hypervisor after reboot. I feel the hypervisor
> > setting should be recovered after reset since it reflects the 
> > security measure enforced by the virtual platform?
> 
> So I think clarifying system reset should address your questions.
> I believe we should leave bypass sticky across device reset, so a FW->OS
> transition, where the OS resets the device, does not open a vulnerability
> (if bypass was enabled at boot and then left disabled by FW.)
> 
> It's still a good idea for the OS to restore on shutdown the bypass value
> it was booted with. So it can kexec into an OS that doesn't support
> virtio-iommu, for example.
> 
> Thanks,
> Jean

Is this stickiness really important? Can't this be addressed just by
hypervisor disabling bypass at boot?

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/5] iommu/virtio: Add identity domains

[Jason Wang]

On Mon, Oct 18, 2021 at 11:35 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Oct 18, 2021 at 04:23:41PM +0100, Jean-Philippe Brucker wrote:
> > On Thu, Oct 14, 2021 at 03:00:38AM +0000, Tian, Kevin wrote:
> > > > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > > > Sent: Wednesday, October 13, 2021 8:11 PM
> > > >
> > > > Support identity domains, allowing to only enable IOMMU protection for a
> > > > subset of endpoints (those assigned to userspace, for example). Users
> > > > may enable identity domains at compile time
> > > > (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> > > > (iommu.passthrough=1) or
> > > > runtime (/sys/kernel/iommu_groups/*/type = identity).
> > >
> > > Do we want to use consistent terms between spec (bypass domain)
> > > and code (identity domain)?
> >
> > I don't think we have to. Linux uses "identity" domains and "passthrough"
> > IOMMU. The old virtio-iommu feature was "bypass" so we should keep that
> > for the new one, to be consistent. And then I've used "bypass" for domains
> > as well, in the spec, because it would look strange to use a different
> > term for the same concept. I find that it sort of falls into place: Linux'
> > identity domains can be implemented either with bypass or identity-mapped
> > virtio-iommu domains.
> >
> > > >
> > > > Patches 1-2 support identity domains using the optional
> > > > VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
> > > > spec, see [1] for the latest proposal.
> > > >
> > > > Patches 3-5 add a fallback to identity mappings, when the feature is not
> > > > supported.
> > > >
> > > > Note that this series doesn't touch the global bypass bit added by
> > > > VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU
> > > > should
> > > > be attached to a domain, so global bypass isn't in use after endpoints
> > >
> > > I saw a concept of deferred attach in iommu core. See iommu_is_
> > > attach_deferred(). Currently this is vendor specific and I haven't
> > > looked into the exact reason why some vendor sets it now. Just
> > > be curious whether the same reason might be applied to virtio-iommu.
> > >
> > > > are probed. Before that, the global bypass policy is decided by the
> > > > hypervisor and firmware. So I don't think Linux needs to touch the
> > >
> > > This reminds me one thing. The spec says that the global bypass
> > > bit is sticky and not affected by reset.
> >
> > The spec talks about *device* reset, triggered by software writing 0 to
> > the status register, but it doesn't mention system reset. Would be good to
> > clarify that. Something like:
> >
> >     If the device offers the VIRTIO_IOMMU_F_BYPASS_CONFIG feature, it MAY
> >     initialize the \field{bypass} field to 1. Field \field{bypass} SHOULD
> >     NOT change on device reset, but SHOULD be restored to its initial
> >     value on system reset.
> >
> > > This implies that in the case
> > > of rebooting the VM into a different OS, the previous OS actually
> > > has the right to override this setting for the next OS. Is it a right
> > > design? Even the firmware itself is unable to identify the original
> > > setting enforced by the hypervisor after reboot. I feel the hypervisor
> > > setting should be recovered after reset since it reflects the
> > > security measure enforced by the virtual platform?
> >
> > So I think clarifying system reset should address your questions.
> > I believe we should leave bypass sticky across device reset, so a FW->OS
> > transition, where the OS resets the device, does not open a vulnerability
> > (if bypass was enabled at boot and then left disabled by FW.)
> >
> > It's still a good idea for the OS to restore on shutdown the bypass value
> > it was booted with. So it can kexec into an OS that doesn't support
> > virtio-iommu, for example.
> >
> > Thanks,
> > Jean
>
> Is this stickiness really important? Can't this be addressed just by
> hypervisor disabling bypass at boot?

And I'm not sure if sticky can survive transport reset.

Thanks

>
> --
> MST
>

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/5] iommu/virtio: Add identity domains

[Jason Wang]

On Mon, Oct 18, 2021 at 11:35 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Oct 18, 2021 at 04:23:41PM +0100, Jean-Philippe Brucker wrote:
> > On Thu, Oct 14, 2021 at 03:00:38AM +0000, Tian, Kevin wrote:
> > > > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > > > Sent: Wednesday, October 13, 2021 8:11 PM
> > > >
> > > > Support identity domains, allowing to only enable IOMMU protection for a
> > > > subset of endpoints (those assigned to userspace, for example). Users
> > > > may enable identity domains at compile time
> > > > (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> > > > (iommu.passthrough=1) or
> > > > runtime (/sys/kernel/iommu_groups/*/type = identity).
> > >
> > > Do we want to use consistent terms between spec (bypass domain)
> > > and code (identity domain)?
> >
> > I don't think we have to. Linux uses "identity" domains and "passthrough"
> > IOMMU. The old virtio-iommu feature was "bypass" so we should keep that
> > for the new one, to be consistent. And then I've used "bypass" for domains
> > as well, in the spec, because it would look strange to use a different
> > term for the same concept. I find that it sort of falls into place: Linux'
> > identity domains can be implemented either with bypass or identity-mapped
> > virtio-iommu domains.
> >
> > > >
> > > > Patches 1-2 support identity domains using the optional
> > > > VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
> > > > spec, see [1] for the latest proposal.
> > > >
> > > > Patches 3-5 add a fallback to identity mappings, when the feature is not
> > > > supported.
> > > >
> > > > Note that this series doesn't touch the global bypass bit added by
> > > > VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU
> > > > should
> > > > be attached to a domain, so global bypass isn't in use after endpoints
> > >
> > > I saw a concept of deferred attach in iommu core. See iommu_is_
> > > attach_deferred(). Currently this is vendor specific and I haven't
> > > looked into the exact reason why some vendor sets it now. Just
> > > be curious whether the same reason might be applied to virtio-iommu.
> > >
> > > > are probed. Before that, the global bypass policy is decided by the
> > > > hypervisor and firmware. So I don't think Linux needs to touch the
> > >
> > > This reminds me one thing. The spec says that the global bypass
> > > bit is sticky and not affected by reset.
> >
> > The spec talks about *device* reset, triggered by software writing 0 to
> > the status register, but it doesn't mention system reset. Would be good to
> > clarify that. Something like:
> >
> >     If the device offers the VIRTIO_IOMMU_F_BYPASS_CONFIG feature, it MAY
> >     initialize the \field{bypass} field to 1. Field \field{bypass} SHOULD
> >     NOT change on device reset, but SHOULD be restored to its initial
> >     value on system reset.
> >
> > > This implies that in the case
> > > of rebooting the VM into a different OS, the previous OS actually
> > > has the right to override this setting for the next OS. Is it a right
> > > design? Even the firmware itself is unable to identify the original
> > > setting enforced by the hypervisor after reboot. I feel the hypervisor
> > > setting should be recovered after reset since it reflects the
> > > security measure enforced by the virtual platform?
> >
> > So I think clarifying system reset should address your questions.
> > I believe we should leave bypass sticky across device reset, so a FW->OS
> > transition, where the OS resets the device, does not open a vulnerability
> > (if bypass was enabled at boot and then left disabled by FW.)
> >
> > It's still a good idea for the OS to restore on shutdown the bypass value
> > it was booted with. So it can kexec into an OS that doesn't support
> > virtio-iommu, for example.
> >
> > Thanks,
> > Jean
>
> Is this stickiness really important? Can't this be addressed just by
> hypervisor disabling bypass at boot?

And I'm not sure if sticky can survive transport reset.

Thanks

>
> --
> MST
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/5] iommu/virtio: Add identity domains

[Jean-Philippe Brucker]

On Tue, Oct 19, 2021 at 09:22:13AM +0800, Jason Wang wrote:
> > > So I think clarifying system reset should address your questions.
> > > I believe we should leave bypass sticky across device reset, so a FW->OS
> > > transition, where the OS resets the device, does not open a vulnerability
> > > (if bypass was enabled at boot and then left disabled by FW.)
> > >
> > > It's still a good idea for the OS to restore on shutdown the bypass value
> > > it was booted with. So it can kexec into an OS that doesn't support
> > > virtio-iommu, for example.
> > >
> > > Thanks,
> > > Jean
> >
> > Is this stickiness really important?

It is important when FW has to hand the IOMMU over to the OS while keeping
DMA disabled for all endpoints. For example DMA was globally disabled on
boot through some external mechanism (e.g. Bus Master Enable in PCI
bridges), and FW disables IOMMU bypass before enabling Bus Master, and
there are some untrusted endpoints in the system that should never be
allowed to perform arbitrary DMA. If a side effect of resetting the IOMMU
is to enable bypass, then the OS opens a vulnerability without knowing it.
That's a real problem on hardware platforms, but maybe too far fetched on
virtual ones.

> > Can't this be addressed just by hypervisor disabling bypass at boot?

Yes I suppose we have that option. If we make bypass non-sticky, we're
preventing FW from working around vulnerable device implementations, but
fixing the implementation itself is much easier in virtualization than in
hardware.

> And I'm not sure if sticky can survive transport reset.

I thought "device reset" includes transport reset as well?  There seems to
be a precedent with virtio-mem which keeps state across device reset. And
PCI allows sticky registers across FLR (RWS registers)

Thanks,
Jean
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/5] iommu/virtio: Add identity domains

[Jean-Philippe Brucker]

On Tue, Oct 19, 2021 at 09:22:13AM +0800, Jason Wang wrote:
> > > So I think clarifying system reset should address your questions.
> > > I believe we should leave bypass sticky across device reset, so a FW->OS
> > > transition, where the OS resets the device, does not open a vulnerability
> > > (if bypass was enabled at boot and then left disabled by FW.)
> > >
> > > It's still a good idea for the OS to restore on shutdown the bypass value
> > > it was booted with. So it can kexec into an OS that doesn't support
> > > virtio-iommu, for example.
> > >
> > > Thanks,
> > > Jean
> >
> > Is this stickiness really important?

It is important when FW has to hand the IOMMU over to the OS while keeping
DMA disabled for all endpoints. For example DMA was globally disabled on
boot through some external mechanism (e.g. Bus Master Enable in PCI
bridges), and FW disables IOMMU bypass before enabling Bus Master, and
there are some untrusted endpoints in the system that should never be
allowed to perform arbitrary DMA. If a side effect of resetting the IOMMU
is to enable bypass, then the OS opens a vulnerability without knowing it.
That's a real problem on hardware platforms, but maybe too far fetched on
virtual ones.

> > Can't this be addressed just by hypervisor disabling bypass at boot?

Yes I suppose we have that option. If we make bypass non-sticky, we're
preventing FW from working around vulnerable device implementations, but
fixing the implementation itself is much easier in virtualization than in
hardware.

> And I'm not sure if sticky can survive transport reset.

I thought "device reset" includes transport reset as well?  There seems to
be a precedent with virtio-mem which keeps state across device reset. And
PCI allows sticky registers across FLR (RWS registers)

Thanks,
Jean
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH 0/5] iommu/virtio: Add identity domains

[Tian, Kevin]

> From: joro@8bytes.org <joro@8bytes.org>
> Sent: Monday, October 18, 2021 7:38 PM
> 
> On Thu, Oct 14, 2021 at 03:00:38AM +0000, Tian, Kevin wrote:
> > I saw a concept of deferred attach in iommu core. See iommu_is_
> > attach_deferred(). Currently this is vendor specific and I haven't
> > looked into the exact reason why some vendor sets it now. Just
> > be curious whether the same reason might be applied to virtio-iommu.
> 
> The reason for attach_deferred is kdump support, where the IOMMU driver
> needs to keep the mappings from the old kernel until the device driver
> of the new kernel takes over.
> 

ok. Then there is no problem with the original statement:

    All endpoints managed by the IOMMU should be attached to a 
    domain, so global bypass isn't in use after endpoints are probed.

Thanks
Kevin
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH 0/5] iommu/virtio: Add identity domains

[Tian, Kevin]

> From: joro@8bytes.org <joro@8bytes.org>
> Sent: Monday, October 18, 2021 7:38 PM
> 
> On Thu, Oct 14, 2021 at 03:00:38AM +0000, Tian, Kevin wrote:
> > I saw a concept of deferred attach in iommu core. See iommu_is_
> > attach_deferred(). Currently this is vendor specific and I haven't
> > looked into the exact reason why some vendor sets it now. Just
> > be curious whether the same reason might be applied to virtio-iommu.
> 
> The reason for attach_deferred is kdump support, where the IOMMU driver
> needs to keep the mappings from the old kernel until the device driver
> of the new kernel takes over.
> 

ok. Then there is no problem with the original statement:

    All endpoints managed by the IOMMU should be attached to a 
    domain, so global bypass isn't in use after endpoints are probed.

Thanks
Kevin
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH 0/5] iommu/virtio: Add identity domains

[Tian, Kevin]

> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Sent: Monday, October 18, 2021 11:24 PM
> 
> On Thu, Oct 14, 2021 at 03:00:38AM +0000, Tian, Kevin wrote:
> > > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > > Sent: Wednesday, October 13, 2021 8:11 PM
> > >
> > > Support identity domains, allowing to only enable IOMMU protection for
> a
> > > subset of endpoints (those assigned to userspace, for example). Users
> > > may enable identity domains at compile time
> > > (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> > > (iommu.passthrough=1) or
> > > runtime (/sys/kernel/iommu_groups/*/type = identity).
> >
> > Do we want to use consistent terms between spec (bypass domain)
> > and code (identity domain)?
> 
> I don't think we have to. Linux uses "identity" domains and "passthrough"
> IOMMU. The old virtio-iommu feature was "bypass" so we should keep that
> for the new one, to be consistent. And then I've used "bypass" for domains
> as well, in the spec, because it would look strange to use a different
> term for the same concept. I find that it sort of falls into place: Linux'
> identity domains can be implemented either with bypass or identity-mapped
> virtio-iommu domains.

make sense.

> 
> > >
> > > Patches 1-2 support identity domains using the optional
> > > VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in
> the
> > > spec, see [1] for the latest proposal.
> > >
> > > Patches 3-5 add a fallback to identity mappings, when the feature is not
> > > supported.
> > >
> > > Note that this series doesn't touch the global bypass bit added by
> > > VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the
> IOMMU
> > > should
> > > be attached to a domain, so global bypass isn't in use after endpoints
> >
> > I saw a concept of deferred attach in iommu core. See iommu_is_
> > attach_deferred(). Currently this is vendor specific and I haven't
> > looked into the exact reason why some vendor sets it now. Just
> > be curious whether the same reason might be applied to virtio-iommu.
> >
> > > are probed. Before that, the global bypass policy is decided by the
> > > hypervisor and firmware. So I don't think Linux needs to touch the
> >
> > This reminds me one thing. The spec says that the global bypass
> > bit is sticky and not affected by reset.
> 
> The spec talks about *device* reset, triggered by software writing 0 to
> the status register, but it doesn't mention system reset. Would be good to
> clarify that. Something like:
> 
>     If the device offers the VIRTIO_IOMMU_F_BYPASS_CONFIG feature, it MAY
>     initialize the \field{bypass} field to 1. Field \field{bypass} SHOULD
>     NOT change on device reset, but SHOULD be restored to its initial
>     value on system reset.

looks good to me

> 
> > This implies that in the case
> > of rebooting the VM into a different OS, the previous OS actually
> > has the right to override this setting for the next OS. Is it a right
> > design? Even the firmware itself is unable to identify the original
> > setting enforced by the hypervisor after reboot. I feel the hypervisor
> > setting should be recovered after reset since it reflects the
> > security measure enforced by the virtual platform?
> 
> So I think clarifying system reset should address your questions.
> I believe we should leave bypass sticky across device reset, so a FW->OS
> transition, where the OS resets the device, does not open a vulnerability
> (if bypass was enabled at boot and then left disabled by FW.)
> 
> It's still a good idea for the OS to restore on shutdown the bypass value
> it was booted with. So it can kexec into an OS that doesn't support
> virtio-iommu, for example.
> 

Thanks
Kevin
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH 0/5] iommu/virtio: Add identity domains

[Tian, Kevin]

> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Sent: Monday, October 18, 2021 11:24 PM
> 
> On Thu, Oct 14, 2021 at 03:00:38AM +0000, Tian, Kevin wrote:
> > > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > > Sent: Wednesday, October 13, 2021 8:11 PM
> > >
> > > Support identity domains, allowing to only enable IOMMU protection for
> a
> > > subset of endpoints (those assigned to userspace, for example). Users
> > > may enable identity domains at compile time
> > > (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> > > (iommu.passthrough=1) or
> > > runtime (/sys/kernel/iommu_groups/*/type = identity).
> >
> > Do we want to use consistent terms between spec (bypass domain)
> > and code (identity domain)?
> 
> I don't think we have to. Linux uses "identity" domains and "passthrough"
> IOMMU. The old virtio-iommu feature was "bypass" so we should keep that
> for the new one, to be consistent. And then I've used "bypass" for domains
> as well, in the spec, because it would look strange to use a different
> term for the same concept. I find that it sort of falls into place: Linux'
> identity domains can be implemented either with bypass or identity-mapped
> virtio-iommu domains.

make sense.

> 
> > >
> > > Patches 1-2 support identity domains using the optional
> > > VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in
> the
> > > spec, see [1] for the latest proposal.
> > >
> > > Patches 3-5 add a fallback to identity mappings, when the feature is not
> > > supported.
> > >
> > > Note that this series doesn't touch the global bypass bit added by
> > > VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the
> IOMMU
> > > should
> > > be attached to a domain, so global bypass isn't in use after endpoints
> >
> > I saw a concept of deferred attach in iommu core. See iommu_is_
> > attach_deferred(). Currently this is vendor specific and I haven't
> > looked into the exact reason why some vendor sets it now. Just
> > be curious whether the same reason might be applied to virtio-iommu.
> >
> > > are probed. Before that, the global bypass policy is decided by the
> > > hypervisor and firmware. So I don't think Linux needs to touch the
> >
> > This reminds me one thing. The spec says that the global bypass
> > bit is sticky and not affected by reset.
> 
> The spec talks about *device* reset, triggered by software writing 0 to
> the status register, but it doesn't mention system reset. Would be good to
> clarify that. Something like:
> 
>     If the device offers the VIRTIO_IOMMU_F_BYPASS_CONFIG feature, it MAY
>     initialize the \field{bypass} field to 1. Field \field{bypass} SHOULD
>     NOT change on device reset, but SHOULD be restored to its initial
>     value on system reset.

looks good to me

> 
> > This implies that in the case
> > of rebooting the VM into a different OS, the previous OS actually
> > has the right to override this setting for the next OS. Is it a right
> > design? Even the firmware itself is unable to identify the original
> > setting enforced by the hypervisor after reboot. I feel the hypervisor
> > setting should be recovered after reset since it reflects the
> > security measure enforced by the virtual platform?
> 
> So I think clarifying system reset should address your questions.
> I believe we should leave bypass sticky across device reset, so a FW->OS
> transition, where the OS resets the device, does not open a vulnerability
> (if bypass was enabled at boot and then left disabled by FW.)
> 
> It's still a good idea for the OS to restore on shutdown the bypass value
> it was booted with. So it can kexec into an OS that doesn't support
> virtio-iommu, for example.
> 

Thanks
Kevin
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH 0/5] iommu/virtio: Add identity domains

[Tian, Kevin]

> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Sent: Wednesday, October 13, 2021 8:11 PM
> 
> Support identity domains, allowing to only enable IOMMU protection for a
> subset of endpoints (those assigned to userspace, for example). Users
> may enable identity domains at compile time
> (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> (iommu.passthrough=1) or
> runtime (/sys/kernel/iommu_groups/*/type = identity).
> 
> Patches 1-2 support identity domains using the optional
> VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
> spec, see [1] for the latest proposal.
> 
> Patches 3-5 add a fallback to identity mappings, when the feature is not
> supported.
> 
> Note that this series doesn't touch the global bypass bit added by
> VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU
> should
> be attached to a domain, so global bypass isn't in use after endpoints
> are probed. Before that, the global bypass policy is decided by the
> hypervisor and firmware. So I don't think Linux needs to touch the
> global bypass bit, but there are some patches available on my
> virtio-iommu/bypass branch [2] to test it.
> 
> QEMU patches are on my virtio-iommu/bypass branch [3] (and the list)
> 
> [1] https://www.mail-archive.com/virtio-dev@lists.oasis-
> open.org/msg07898.html
> [2] https://jpbrucker.net/git/linux/log/?h=virtio-iommu/bypass
> [3] https://jpbrucker.net/git/qemu/log/?h=virtio-iommu/bypass
> 
> Jean-Philippe Brucker (5):
>   iommu/virtio: Add definitions for VIRTIO_IOMMU_F_BYPASS_CONFIG
>   iommu/virtio: Support bypass domains
>   iommu/virtio: Sort reserved regions
>   iommu/virtio: Pass end address to viommu_add_mapping()
>   iommu/virtio: Support identity-mapped domains
> 
>  include/uapi/linux/virtio_iommu.h |   8 ++-
>  drivers/iommu/virtio-iommu.c      | 113 +++++++++++++++++++++++++-----
>  2 files changed, 101 insertions(+), 20 deletions(-)
> 

For this series:

	Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH 0/5] iommu/virtio: Add identity domains

[Tian, Kevin]

> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Sent: Wednesday, October 13, 2021 8:11 PM
> 
> Support identity domains, allowing to only enable IOMMU protection for a
> subset of endpoints (those assigned to userspace, for example). Users
> may enable identity domains at compile time
> (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> (iommu.passthrough=1) or
> runtime (/sys/kernel/iommu_groups/*/type = identity).
> 
> Patches 1-2 support identity domains using the optional
> VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
> spec, see [1] for the latest proposal.
> 
> Patches 3-5 add a fallback to identity mappings, when the feature is not
> supported.
> 
> Note that this series doesn't touch the global bypass bit added by
> VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU
> should
> be attached to a domain, so global bypass isn't in use after endpoints
> are probed. Before that, the global bypass policy is decided by the
> hypervisor and firmware. So I don't think Linux needs to touch the
> global bypass bit, but there are some patches available on my
> virtio-iommu/bypass branch [2] to test it.
> 
> QEMU patches are on my virtio-iommu/bypass branch [3] (and the list)
> 
> [1] https://www.mail-archive.com/virtio-dev@lists.oasis-
> open.org/msg07898.html
> [2] https://jpbrucker.net/git/linux/log/?h=virtio-iommu/bypass
> [3] https://jpbrucker.net/git/qemu/log/?h=virtio-iommu/bypass
> 
> Jean-Philippe Brucker (5):
>   iommu/virtio: Add definitions for VIRTIO_IOMMU_F_BYPASS_CONFIG
>   iommu/virtio: Support bypass domains
>   iommu/virtio: Sort reserved regions
>   iommu/virtio: Pass end address to viommu_add_mapping()
>   iommu/virtio: Support identity-mapped domains
> 
>  include/uapi/linux/virtio_iommu.h |   8 ++-
>  drivers/iommu/virtio-iommu.c      | 113 +++++++++++++++++++++++++-----
>  2 files changed, 101 insertions(+), 20 deletions(-)
> 

For this series:

	Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/5] iommu/virtio: Add identity domains

[Michael S. Tsirkin]

On Wed, Oct 13, 2021 at 01:10:48PM +0100, Jean-Philippe Brucker wrote:
> Support identity domains, allowing to only enable IOMMU protection for a
> subset of endpoints (those assigned to userspace, for example). Users
> may enable identity domains at compile time
> (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time (iommu.passthrough=1) or
> runtime (/sys/kernel/iommu_groups/*/type = identity).


I put this in my branch so it can get testing under linux-next,
but pls notice if the ballot does not conclude in time
for the merge window I won't send it to Linus.

> Patches 1-2 support identity domains using the optional
> VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
> spec, see [1] for the latest proposal.
> 
> Patches 3-5 add a fallback to identity mappings, when the feature is not
> supported.
> 
> Note that this series doesn't touch the global bypass bit added by
> VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU should
> be attached to a domain, so global bypass isn't in use after endpoints
> are probed. Before that, the global bypass policy is decided by the
> hypervisor and firmware. So I don't think Linux needs to touch the
> global bypass bit, but there are some patches available on my
> virtio-iommu/bypass branch [2] to test it.
> 
> QEMU patches are on my virtio-iommu/bypass branch [3] (and the list)
> 
> [1] https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg07898.html
> [2] https://jpbrucker.net/git/linux/log/?h=virtio-iommu/bypass
> [3] https://jpbrucker.net/git/qemu/log/?h=virtio-iommu/bypass
> 
> Jean-Philippe Brucker (5):
>   iommu/virtio: Add definitions for VIRTIO_IOMMU_F_BYPASS_CONFIG
>   iommu/virtio: Support bypass domains
>   iommu/virtio: Sort reserved regions
>   iommu/virtio: Pass end address to viommu_add_mapping()
>   iommu/virtio: Support identity-mapped domains
> 
>  include/uapi/linux/virtio_iommu.h |   8 ++-
>  drivers/iommu/virtio-iommu.c      | 113 +++++++++++++++++++++++++-----
>  2 files changed, 101 insertions(+), 20 deletions(-)
> 
> -- 
> 2.33.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/5] iommu/virtio: Add identity domains

[Michael S. Tsirkin]

On Wed, Oct 13, 2021 at 01:10:48PM +0100, Jean-Philippe Brucker wrote:
> Support identity domains, allowing to only enable IOMMU protection for a
> subset of endpoints (those assigned to userspace, for example). Users
> may enable identity domains at compile time
> (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time (iommu.passthrough=1) or
> runtime (/sys/kernel/iommu_groups/*/type = identity).


I put this in my branch so it can get testing under linux-next,
but pls notice if the ballot does not conclude in time
for the merge window I won't send it to Linus.

> Patches 1-2 support identity domains using the optional
> VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in the
> spec, see [1] for the latest proposal.
> 
> Patches 3-5 add a fallback to identity mappings, when the feature is not
> supported.
> 
> Note that this series doesn't touch the global bypass bit added by
> VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU should
> be attached to a domain, so global bypass isn't in use after endpoints
> are probed. Before that, the global bypass policy is decided by the
> hypervisor and firmware. So I don't think Linux needs to touch the
> global bypass bit, but there are some patches available on my
> virtio-iommu/bypass branch [2] to test it.
> 
> QEMU patches are on my virtio-iommu/bypass branch [3] (and the list)
> 
> [1] https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg07898.html
> [2] https://jpbrucker.net/git/linux/log/?h=virtio-iommu/bypass
> [3] https://jpbrucker.net/git/qemu/log/?h=virtio-iommu/bypass
> 
> Jean-Philippe Brucker (5):
>   iommu/virtio: Add definitions for VIRTIO_IOMMU_F_BYPASS_CONFIG
>   iommu/virtio: Support bypass domains
>   iommu/virtio: Sort reserved regions
>   iommu/virtio: Pass end address to viommu_add_mapping()
>   iommu/virtio: Support identity-mapped domains
> 
>  include/uapi/linux/virtio_iommu.h |   8 ++-
>  drivers/iommu/virtio-iommu.c      | 113 +++++++++++++++++++++++++-----
>  2 files changed, 101 insertions(+), 20 deletions(-)
> 
> -- 
> 2.33.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/5] iommu/virtio: Add identity domains

[Jean-Philippe Brucker]

On Fri, Oct 22, 2021 at 06:16:27AM -0400, Michael S. Tsirkin wrote:
> On Wed, Oct 13, 2021 at 01:10:48PM +0100, Jean-Philippe Brucker wrote:
> > Support identity domains, allowing to only enable IOMMU protection for a
> > subset of endpoints (those assigned to userspace, for example). Users
> > may enable identity domains at compile time
> > (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time (iommu.passthrough=1) or
> > runtime (/sys/kernel/iommu_groups/*/type = identity).
> 
> 
> I put this in my branch so it can get testing under linux-next,
> but pls notice if the ballot does not conclude in time
> for the merge window I won't send it to Linus.

Makes sense, thank you. I sent a new version of the spec change with
clarifications
https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg07969.html

Thanks,
Jean

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/5] iommu/virtio: Add identity domains

[Jean-Philippe Brucker]

On Fri, Oct 22, 2021 at 06:16:27AM -0400, Michael S. Tsirkin wrote:
> On Wed, Oct 13, 2021 at 01:10:48PM +0100, Jean-Philippe Brucker wrote:
> > Support identity domains, allowing to only enable IOMMU protection for a
> > subset of endpoints (those assigned to userspace, for example). Users
> > may enable identity domains at compile time
> > (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time (iommu.passthrough=1) or
> > runtime (/sys/kernel/iommu_groups/*/type = identity).
> 
> 
> I put this in my branch so it can get testing under linux-next,
> but pls notice if the ballot does not conclude in time
> for the merge window I won't send it to Linus.

Makes sense, thank you. I sent a new version of the spec change with
clarifications
https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg07969.html

Thanks,
Jean

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization