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).
Since v1 [1] I rebased onto v5.16-rc and added Kevin's review tag.
The specification update for the new feature has now been accepted [2].
Patches 1-2 support identity domains using the optional
VIRTIO_IOMMU_F_BYPASS_CONFIG feature, and patches 3-5 add a fallback to
identity mappings, when the feature is not supported.
QEMU patches are on my virtio-iommu/bypass branch [3], and depend on the
UAPI update.
[1] https://lore.kernel.org/linux-iommu/[email protected]/
[2] https://github.com/oasis-tcs/virtio-spec/issues/119
[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.1
Add definitions for the VIRTIO_IOMMU_F_BYPASS_CONFIG, which supersedes
VIRTIO_IOMMU_F_BYPASS.
Reviewed-by: Kevin Tian <[email protected]>
Signed-off-by: Jean-Philippe Brucker <[email protected]>
---
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.1
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.
Reviewed-by: Kevin Tian <[email protected]>
Signed-off-by: Jean-Philippe Brucker <[email protected]>
---
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.1
To ease identity mapping support, keep the list of reserved regions
sorted.
Reviewed-by: Kevin Tian <[email protected]>
Signed-off-by: Jean-Philippe Brucker <[email protected]>
---
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(®ion->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(®ion->list, &next->list);
return 0;
}
--
2.33.1
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.
Reviewed-by: Kevin Tian <[email protected]>
Signed-off-by: Jean-Philippe Brucker <[email protected]>
---
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.1
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.
Reviewed-by: Kevin Tian <[email protected]>
Signed-off-by: Jean-Philippe Brucker <[email protected]>
---
drivers/iommu/virtio-iommu.c | 63 +++++++++++++++++++++++++++++++++---
1 file changed, 58 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index eceb9281c8c1..6a8a52b4297b 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;
+ vdomain->viommu = NULL;
return -EOPNOTSUPP;
}
-
- vdomain->bypass = true;
}
return 0;
--
2.33.1
Hi Jean,
On 11/23/21 4:52 PM, Jean-Philippe Brucker wrote:
> Add definitions for the VIRTIO_IOMMU_F_BYPASS_CONFIG, which supersedes
> VIRTIO_IOMMU_F_BYPASS.
>
> Reviewed-by: Kevin Tian <[email protected]>
> Signed-off-by: Jean-Philippe Brucker <[email protected]>
> ---
> 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];
in [PATCH v3] virtio-iommu: Rework the bypass feature I see
+ u8 bypass;
+ u8 reserved[3];
What was exactly voted?
Thanks
Eric
> };
>
> /* 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;
> };
>
Hi Jean,
On 11/23/21 4:52 PM, Jean-Philippe Brucker wrote:
> 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.
>
> Reviewed-by: Kevin Tian <[email protected]>
> Signed-off-by: Jean-Philippe Brucker <[email protected]>
> ---
> 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,
nit: couldn't the check be done before the ida_alloc_range(),
simplifying the failure cleanup?
Thanks
Eric
> + 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[] = {
On 11/23/21 4:53 PM, Jean-Philippe Brucker wrote:
> 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.
>
> Reviewed-by: Kevin Tian <[email protected]>
> Signed-off-by: Jean-Philippe Brucker <[email protected]>
Reviewed-by: Eric Auger <[email protected]>
Eric
> ---
> 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;
>
Hi Jean,
On 11/23/21 4:53 PM, Jean-Philippe Brucker wrote:
> To ease identity mapping support, keep the list of reserved regions
> sorted.
>
> Reviewed-by: Kevin Tian <[email protected]>
> Signed-off-by: Jean-Philippe Brucker <[email protected]>
Reviewed-by: Eric Auger <[email protected]>
Eric
> ---
> 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(®ion->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(®ion->list, &next->list);
> return 0;
> }
>
Hi Jean,
On 11/23/21 4:53 PM, Jean-Philippe Brucker wrote:
> 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.
>
> Reviewed-by: Kevin Tian <[email protected]>
> Signed-off-by: Jean-Philippe Brucker <[email protected]>
> ---
> drivers/iommu/virtio-iommu.c | 63 +++++++++++++++++++++++++++++++++---
> 1 file changed, 58 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index eceb9281c8c1..6a8a52b4297b 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;
> + vdomain->viommu = NULL;
nit: that change could have been done in patch 2
> return -EOPNOTSUPP;
> }
> -
> - vdomain->bypass = true;
> }
>
> return 0;
Besides
Reviewed-by: Eric Auger <[email protected]>
Eric
On Sat, Nov 27, 2021 at 06:09:40PM +0100, Eric Auger wrote:
>
>
> On 11/23/21 4:53 PM, Jean-Philippe Brucker wrote:
> > 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.
> >
> > Reviewed-by: Kevin Tian <[email protected]>
> > Signed-off-by: Jean-Philippe Brucker <[email protected]>
> Reviewed-by: Eric Auger <[email protected]>
>
> Eric
>
> > ---
> > 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;
I am worried that API changes like that will cause subtle
bugs since types of arguments change but not their
number. If we forgot to update some callers it will all be messed up.
How about passing struct Range instead?
> > @@ -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;
> >
Hi Eric,
On Sat, Nov 27, 2021 at 08:59:25AM +0100, Eric Auger wrote:
> > @@ -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];
> in [PATCH v3] virtio-iommu: Rework the bypass feature I see
>
> + u8 bypass;
> + u8 reserved[3];
>
> What was exactly voted?
Good catch, this should be 3. It brings the config struct to 40 bytes,
which is the size compilers generate when there is no reserved field.
Thanks,
Jean
On Sat, Nov 27, 2021 at 05:18:28PM +0100, Eric Auger wrote:
> Hi Jean,
>
> On 11/23/21 4:52 PM, Jean-Philippe Brucker wrote:
> > 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.
> >
> > Reviewed-by: Kevin Tian <[email protected]>
> > Signed-off-by: Jean-Philippe Brucker <[email protected]>
> > ---
> > 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,
> nit: couldn't the check be done before the ida_alloc_range(),
> simplifying the failure cleanup?
It could, but patch 5 falls back to identity mappings, which is better
left at the end of the function to keep the error path simple. I put this
at the end already here, so patch 5 doesn't need to move things around.
Thanks,
Jean
On Sat, Nov 27, 2021 at 06:09:56PM -0500, Michael S. Tsirkin wrote:
> > > -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;
>
> I am worried that API changes like that will cause subtle
> bugs since types of arguments change but not their
> number. If we forgot to update some callers it will all be messed up.
>
> How about passing struct Range instead?
I gave struct range a try but it looks messier overall since it would only
be used to pass arguments. I think the update is safe enough because there
is one caller for viommu_add_mapping() and two for viommu_del_mappings(),
at the moment.
Thanks,
Jean
On Sat, Nov 27, 2021 at 06:09:56PM +0100, Eric Auger wrote:
> > - vdomain->viommu = 0;
> > + vdomain->viommu = NULL;
> nit: that change could have been done in patch 2
Ah yes, I changed that in v2 but fixed up the wrong patch
> > return -EOPNOTSUPP;
> > }
> > -
> > - vdomain->bypass = true;
> > }
> >
> > return 0;
> Besides
> Reviewed-by: Eric Auger <[email protected]>
Thanks!
Jean