[PATCH 6/6] iommu/virtio: batch the mapping replay on domain attach

[Anlai Lu]

viommu_replay_mappings() sent every mapping of a domain to the device
with a synchronous request, holding mappings_lock - and with it
interrupts - across the whole walk, so the IRQ-off window grew with the
number of mappings: 233 ms for 8192 4K mappings, 29 s for a million.

Queue them instead and wait once. One mapping per mappings_lock section,
so that a full virtqueue - the only wait in there - bounds a single lock
hold rather than the whole walk; the walk is a single pass, because the
endpoint was published before it (a mapping inserted while it runs is
queued by map_pages() itself) and a mapping removed meanwhile has its
UNMAP queued in the section that removed it. The requests carry the
device's errno back - the first error it reported - so a rejected replay
fails the attach with the device's own status, and a device that goes
away fails it with -ENODEV rather than reporting a replay that never
happened; a duplicate MAP, which a replay can legitimately produce, is
answered S_INVAL - the spec says a duplicate MAP SHOULD be rejected and
MUST NOT change the existing mapping - and is not treated as a rejection.
A rejected MAP on the map path is still ignored, as before.

Measured, N=8192 4K mappings: max IRQ-off window 233.7 ms -> 1.4-2.5 ms,
and no longer growing with N (1.4-2.5 ms from N=64 to 32768, against
3.6/45/233 ms on the unpatched driver); attach ioctl 218.9/249.0 ->
50.3/48.8 ms (48-67 ms across runs); viommu_send_req_sync() calls 8193 ->
1; the device receives exactly the same MAPs (8244).

Signed-off-by: Anlai Lu <agicy@xxxxxx>
---
 drivers/iommu/virtio-iommu.c | 149 +++++++++++++++++++++++++++--------
 1 file changed, 117 insertions(+), 32 deletions(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 4e578d4255f6..7b883f59221f 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -73,6 +73,19 @@ struct viommu_mapping {
 	struct viommu_request		*unmap_req;
 };
 
+/* Where a queued request reports a rejection, or NULL */
+struct viommu_req_status {
+	bool				*rejected;
+	/*
+	 * Errno to treat as success: a replayed MAP can be a duplicate,
+	 * which the spec says the device SHOULD reject with S_INVAL and
+	 * MUST NOT change the existing mapping.
+	 */
+	int				ignore_errno;
+	/* The first error the device reported, for the caller to return */
+	int				err;
+};
+
 struct viommu_domain {
 	struct iommu_domain		domain;
 	struct viommu_dev		*viommu;
@@ -104,6 +117,8 @@ struct viommu_request {
 	void				*writeback;
 	unsigned int			write_offset;
 	unsigned int			len;
+	/* Where to report a failure of this request, or NULL */
+	struct viommu_req_status	*status;
 	char				buf[] __counted_by(len);
 };
 
@@ -144,6 +159,19 @@ static bool viommu_domain_has_endpoint(struct viommu_domain *vdomain)
 	return has;
 }
 
+/* Same, for callers that do not hold request_lock */
+static bool viommu_device_alive(struct viommu_dev *viommu)
+{
+	unsigned long flags;
+	bool live;
+
+	spin_lock_irqsave(&viommu->request_lock, flags);
+	live = viommu_device_live(viommu);
+	spin_unlock_irqrestore(&viommu->request_lock, flags);
+
+	return live;
+}
+
 static int viommu_get_req_errno(void *buf, size_t len)
 {
 	struct virtio_iommu_req_tail *tail = buf + len - sizeof(*tail);
@@ -216,6 +244,16 @@ static int __viommu_sync_req(struct viommu_dev *viommu)
 			viommu_set_req_status(req->buf, req->len,
 					      VIRTIO_IOMMU_S_IOERR);
 
+		if (req->status) {
+			int err = viommu_get_req_errno(req->buf, req->len);
+
+			if (err && err != req->status->ignore_errno) {
+				*req->status->rejected = true;
+				if (!req->status->err)
+					req->status->err = err;
+			}
+		}
+
 		write_len = req->len - req->write_offset;
 		if (req->writeback && len == write_len)
 			memcpy(req->writeback, req->buf + req->write_offset,
@@ -286,6 +324,7 @@ static int __viommu_queue_req(struct viommu_dev *viommu,
  * @buf: pointer to the request buffer
  * @len: length of the request buffer
  * @writeback: copy data back to the buffer when the request completes.
+ * @status: where to report a rejection of this request, or NULL
  *
  * Allocate a request object, fill it and queue it.  When @writeback is true,
  * data written by the device, including the request status, is copied into
@@ -295,7 +334,7 @@ static int __viommu_queue_req(struct viommu_dev *viommu,
  * Return 0 if the request was successfully added to the queue.
  */
 static int __viommu_add_req(struct viommu_dev *viommu, void *buf, size_t len,
-			    bool writeback)
+			    bool writeback, struct viommu_req_status *status)
 {
 	int ret;
 	off_t write_offset;
@@ -312,6 +351,7 @@ static int __viommu_add_req(struct viommu_dev *viommu, void *buf, size_t len,
 		return -ENOMEM;
 
 	req->len = len;
+	req->status = status;
 	if (writeback) {
 		req->writeback = buf + write_offset;
 		req->write_offset = write_offset;
@@ -325,13 +365,14 @@ static int __viommu_add_req(struct viommu_dev *viommu, void *buf, size_t len,
 	return ret;
 }
 
-static int viommu_add_req(struct viommu_dev *viommu, void *buf, size_t len)
+static int viommu_add_req(struct viommu_dev *viommu, void *buf, size_t len,
+			  struct viommu_req_status *status)
 {
 	int ret;
 	unsigned long flags;
 
 	spin_lock_irqsave(&viommu->request_lock, flags);
-	ret = __viommu_add_req(viommu, buf, len, false);
+	ret = __viommu_add_req(viommu, buf, len, false, status);
 	if (ret)
 		dev_dbg(viommu->dev, "could not add request: %d\n", ret);
 	spin_unlock_irqrestore(&viommu->request_lock, flags);
@@ -354,6 +395,7 @@ static int viommu_queue_prealloc_req(struct viommu_dev *viommu,
 
 	spin_lock_irqsave(&viommu->request_lock, flags);
 	req->len = len;
+	req->status = NULL;
 	req->writeback = NULL;
 	memcpy(req->buf, buf, write_offset);
 	ret = __viommu_queue_req(viommu, req, write_offset);
@@ -374,7 +416,7 @@ static int viommu_send_req_sync(struct viommu_dev *viommu, void *buf,
 
 	spin_lock_irqsave(&viommu->request_lock, flags);
 
-	ret = __viommu_add_req(viommu, buf, len, true);
+	ret = __viommu_add_req(viommu, buf, len, true, NULL);
 	if (ret) {
 		dev_dbg(viommu->dev, "could not add request (%d)\n", ret);
 		goto out_unlock;
@@ -462,7 +504,7 @@ static int viommu_add_mapping(struct viommu_domain *vdomain, u64 iova, u64 end,
 			.flags		= cpu_to_le32(flags),
 		};
 
-		ret = viommu_add_req(vdomain->viommu, &map, sizeof(map));
+		ret = viommu_add_req(vdomain->viommu, &map, sizeof(map), NULL);
 		if (ret) {
 			interval_tree_remove(&mapping->iova, &vdomain->mappings);
 			spin_unlock_irqrestore(&vdomain->mappings_lock, irqflags);
@@ -656,23 +698,57 @@ static int viommu_domain_map_identity(struct viommu_endpoint *vdev,
 }
 
 /*
- * viommu_replay_mappings - re-send MAP requests
+ * viommu_replay_mappings - send every mapping of the domain again
  *
- * When reattaching a domain that was previously detached from all endpoints,
- * mappings were deleted from the device. Re-create the mappings available in
- * the internal tree.
+ * Used when reattaching a domain that was previously detached from all
+ * endpoints: the device dropped its copy then (it frees a domain together with
+ * its last endpoint).
+ *
+ * The device is not waited for between mappings: they are all queued, one per
+ * mappings_lock section, and waited for once at the end.  A full virtqueue is
+ * the only wait in the loop, so it bounds a single lock hold rather than the
+ * whole walk.  A mapping that was sent concurrently is harmless: the spec says
+ * a duplicate MAP SHOULD be rejected with S_INVAL and MUST NOT change the
+ * existing mapping, and the replay treats that rejection as success.
+ *
+ * A rejected MAP is reported by returning an error, which fails the attach: the
+ * domain would be missing a mapping its driver believes in.  Nothing else is
+ * done about it.
  */
 static int viommu_replay_mappings(struct viommu_domain *vdomain)
 {
-	int ret = 0;
+	bool rejected = false;
+	struct viommu_req_status status = {
+		.rejected	= &rejected,
+		.ignore_errno	= -EINVAL,
+	};
 	unsigned long flags;
-	struct viommu_mapping *mapping;
-	struct interval_tree_node *node;
-	struct virtio_iommu_req_map map;
+	int ret = 0;
+	struct interval_tree_node *node = NULL;
+
+	/*
+	 * A single pass is enough: the endpoint was published before this ran,
+	 * so a mapping inserted while the walk progresses is queued by
+	 * map_pages() itself, and one inserted before it is in the tree when
+	 * the walk starts.  A mapping removed meanwhile is simply not there any
+	 * more, and its UNMAP was queued in the same section that removed it,
+	 * so it cannot end up after a MAP this walk sends.
+	 */
+	u64 next_iova = 0;
+	bool done = false;
+
+	while (!done) {
+		struct viommu_mapping *mapping;
+		struct virtio_iommu_req_map map;
+
+		spin_lock_irqsave(&vdomain->mappings_lock, flags);
+		node = interval_tree_iter_first(&vdomain->mappings, next_iova,
+						-1UL);
+		if (!node) {
+			spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
+			break;
+		}
 
-	spin_lock_irqsave(&vdomain->mappings_lock, flags);
-	node = interval_tree_iter_first(&vdomain->mappings, 0, -1UL);
-	while (node) {
 		mapping = container_of(node, struct viommu_mapping, iova);
 		map = (struct virtio_iommu_req_map) {
 			.head.type	= VIRTIO_IOMMU_T_MAP,
@@ -682,26 +758,35 @@ static int viommu_replay_mappings(struct viommu_domain *vdomain)
 			.phys_start	= cpu_to_le64(mapping->paddr),
 			.flags		= cpu_to_le32(mapping->flags),
 		};
+		ret = viommu_add_req(vdomain->viommu, &map, sizeof(map), &status);
 
-		ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
-		if (ret) {
-			/*
-			 * The endpoint was published before this walk, so a
-			 * map_pages() racing it can have sent the same MAP
-			 * already: the spec says a duplicate MAP SHOULD be
-			 * rejected with S_INVAL, and the device MUST NOT
-			 * change the existing mapping.
-			 */
-			if (ret != -EINVAL)
-				break;
-			ret = 0;
-		}
+		if (mapping->iova.last == ULONG_MAX)
+			done = true;
+		else
+			next_iova = mapping->iova.last + 1;
+		spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
 
-		node = interval_tree_iter_next(node, 0, -1UL);
+		if (ret)
+			break;
 	}
-	spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
 
-	return ret;
+	/* Wait for all of them outside the lock: this is the batched drain */
+	viommu_sync_req(vdomain->viommu);
+
+	/*
+	 * A device that went away did not take anything: the status pointers of
+	 * the requests still queued are on this frame, so report it here rather
+	 * than pretending the replay delivered the mappings.
+	 */
+	if (!viommu_device_alive(vdomain->viommu))
+		return -ENODEV;
+
+	/* The queueing failure, or what the device said */
+	if (ret)
+		return ret;
+
+	/* Report what the device said, like the synchronous replay did */
+	return rejected ? (status.err ?: -EIO) : 0;
 }
 
 static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
-- 
2.55.0