Re: [PATCH v3] iommu/vt-d: Fix IQE handling to cover all descriptors in submission range

[Baolu Lu]

On 9/14/2026 10:19 PM, Guanghui Feng wrote:
> When an Invalidation Queue Error (IQE) occurs, hardware halts fetching
> and IQH points at the faulting descriptor. The previous code only
> checked whether IQH matched the first descriptor index of the current
> submission, missing faults on any other descriptor within the batch.
>
> Expand the check to cover the entire submission range [index, wait_index],
> accounting for circular wrap-around.
>
> Furthermore, after detecting IQE, the old recovery only replaced the
> single faulting slot with a copy of the wait descriptor and immediately
> returned -EINVAL. This left two problems:
>
>    a) Hardware resumed fetching and could hit another invalid descriptor
>       in the same abandoned batch, raising a second IQE that no submitter
>       would claim — permanently deadlocking the queue.
>
>    b) The caller reclaimed all batch slots (QI_FREE) while hardware might
>       still be asynchronously processing descriptors from that batch,
>       allowing concurrent overwrite and descriptor corruption.
>
> Fix both by introducing qi_drain_remaining_descs(): upon IQE detection,
> overwrite the stranded slots in [IQH, wait_index) with no-op wait
> descriptors, resubmit the wait descriptor at wait_index, then clear IQE.
> The caller's existing poll loop naturally waits for QI_DONE — which per
> VT-d spec §6.5.2.12 is only written after ALL preceding descriptors
> complete — guaranteeing hardware has fully drained the batch before
> slots are reclaimed.
>
> Signed-off-by: Guanghui Feng <guanghuifeng@xxxxxxxxxxxxxxxxx>
> Signed-off-by: bikuan.zbk <bikuan.zbk@xxxxxxxxxxxxxxx>
> ---
>   drivers/iommu/intel/dmar.c | 78 ++++++++++++++++++++++++++++++--------
>   1 file changed, 63 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
> index ba675b08cd20..db92fb247131 100644
> --- a/drivers/iommu/intel/dmar.c
> +++ b/drivers/iommu/intel/dmar.c
> @@ -1344,6 +1344,43 @@ static void qi_dump_fault(struct intel_iommu *iommu, u32 fault)
>   	       (unsigned long long)desc->qw1);
>   }
>   
> +/**
> + * qi_drain_remaining_descs - drain descriptors stranded by an IQE
> + * @iommu: the affected IOMMU
> + * @wait_index: slot index of the wait descriptor of the current submission
> + * @shift: qi_shift(iommu), converts a slot index into a byte offset
> + * @options: QI_OPT_* flags of the current submission
> + *
> + * Overwrite the slots in [IQH, @wait_index) with no-op descriptors, resubmit
> + * the wait descriptor at @wait_index, and clear IQE so hardware resumes and
> + * eventually signals QI_DONE. Must be called with qi->q_lock held.
> + */
> +static void qi_drain_remaining_descs(struct intel_iommu *iommu,
> +				     int wait_index, int shift,
> +				     unsigned long options)

This is only an internal helper, so there is no need to add a kernel-doc
comment like this.

> +{
> +	struct q_inval *qi = iommu->qi;
> +	struct qi_desc desc;
> +	int head_idx;
> +	int cur;
> +
> +	head_idx = (readl(iommu->reg + DMAR_IQH_REG)) >> shift;
> +
> +	memset(&desc, 0, sizeof(desc));
> +	desc.qw0 = QI_IWD_TYPE;
> +	for (cur = head_idx; cur != wait_index; cur = (cur + 1) % QI_LENGTH)
> +		memcpy(qi->desc + (cur << shift), &desc, 1 << shift);
> +
> +	desc.qw0 = QI_IWD_STATUS_DATA(QI_DONE) |
> +			QI_IWD_STATUS_WRITE | QI_IWD_TYPE;
> +	if (options & QI_OPT_WAIT_DRAIN)
> +		desc.qw0 |= QI_IWD_PRQ_DRAIN;
> +	desc.qw1 = virt_to_phys(&qi->desc_status[wait_index]);
> +	memcpy(qi->desc + (wait_index << shift), &desc, 1 << shift);
> +
> +	writel(DMA_FSTS_IQE, iommu->reg + DMAR_FSTS_REG);
> +}

I would like this helper to be self-contained; that is, it should
replace all abandoned descriptors with wait descriptors and then wait
until hardware fetch passes those descriptors. Something like this (not
tested yet):

+static int qi_drain_remaining_descs(struct intel_iommu *iommu, int head,
+				    int wait_index)
+{
+	struct q_inval *qi = iommu->qi;
+	int shift = qi_shift(iommu);
+	struct qi_desc wait_desc = {};
+	struct qi_desc nop_desc = {};
+	cycles_t start;
+
+	nop_desc.qw0 = QI_IWD_FENCE | QI_IWD_TYPE;
+
+	wait_desc.qw0 = QI_IWD_STATUS_DATA(QI_DONE) |
+			QI_IWD_STATUS_WRITE | QI_IWD_FENCE | QI_IWD_TYPE;
+	wait_desc.qw1 = virt_to_phys(&qi->desc_status[wait_index]);
+
+	while (head != wait_index) {
+		memcpy(qi->desc + (head << shift), &nop_desc, 1 << shift);
+		head = (head + 1) % QI_LENGTH;
+	}
+
+	WRITE_ONCE(qi->desc_status[wait_index], QI_IN_USE);
+	memcpy(qi->desc + (wait_index << shift), &wait_desc, 1 << shift);
+
+	/*
+	 * Order the descriptor rewrites before the writel() that restarts
+	 * the fetch engine.
+	 */
+	wmb();
+
+	writel(DMA_FSTS_IQE, iommu->reg + DMAR_FSTS_REG);
+
+	start = get_cycles();
+	while (READ_ONCE(qi->desc_status[wait_index]) != QI_DONE) {
+		if (DMAR_OPERATION_TIMEOUT < (get_cycles() - start)) {
+			pr_err("Timeout draining invalidation queue after IQE\n");
+			return -ETIMEDOUT;
+		}
+		cpu_relax();
+	}
+
+	return 0;
+}

> +
>   static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
>   {
>   	u32 fault;
> @@ -1366,21 +1403,22 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
>   	 * is cleared.
>   	 */
>   	if (fault & DMA_FSTS_IQE) {
> +		int head_idx;
> +
>   		head = readl(iommu->reg + DMAR_IQH_REG);
> -		if ((head >> shift) == index) {
> -			struct qi_desc *desc = qi->desc + head;
> +		head_idx = head >> shift;
>   
> -			/*
> -			 * desc->qw2 and desc->qw3 are either reserved or
> -			 * used by software as private data. We won't print
> -			 * out these two qw's for security consideration.
> -			 */
> -			memcpy(desc, qi->desc + (wait_index << shift),
> -			       1 << shift);
> -			writel(DMA_FSTS_IQE, iommu->reg + DMAR_FSTS_REG);
> -			pr_info("Invalidation Queue Error (IQE) cleared\n");
> +		/*
> +		 * The faulting descriptor can be anywhere within the current
> +		 * submission's range [index, wait_index]. Since the queue is
> +		 * circular, this submission may wrap around QI_LENGTH
> +		 * (index > wait_index in that case), so check both the
> +		 * non-wrapped and wrapped cases of the range.
> +		 */
> +		if (index <= wait_index ?
> +		    (head_idx >= index && head_idx <= wait_index) :
> +		    (head_idx >= index || head_idx <= wait_index))
>   			return -EINVAL;
> -		}
>   	}

Then call the above helper directly here.

@@ -1366,18 +1409,25 @@ static int qi_check_fault(struct intel_iommu 
*iommu, int index, int wait_index)
 	 * is cleared.
 	 */
 	if (fault & DMA_FSTS_IQE) {
+		int head_idx, ret;
+
 		head = readl(iommu->reg + DMAR_IQH_REG);
-		if ((head >> shift) == index) {
-			struct qi_desc *desc = qi->desc + head;
+		head_idx = (head >> shift) % QI_LENGTH;
+
+		/*
+		 * The faulting descriptor can be anywhere within the current
+		 * submission's range [index, wait_index]. Since the queue is
+		 * circular, this submission may wrap around QI_LENGTH
+		 * (index > wait_index in that case), so check both the
+		 * non-wrapped and wrapped cases of the range.
+		 */
+		if (index <= wait_index ?
+		    (head_idx >= index && head_idx <= wait_index) :
+		    (head_idx >= index || head_idx <= wait_index)) {
+			ret = qi_drain_remaining_descs(iommu, head_idx, wait_index);
+			if (ret)
+				return ret;

-			/*
-			 * desc->qw2 and desc->qw3 are either reserved or
-			 * used by software as private data. We won't print
-			 * out these two qw's for security consideration.
-			 */
-			memcpy(desc, qi->desc + (wait_index << shift),
-			       1 << shift);
-			writel(DMA_FSTS_IQE, iommu->reg + DMAR_FSTS_REG);
 			pr_info("Invalidation Queue Error (IQE) cleared\n");
 			return -EINVAL;
 		}

>   
>   	/*
> @@ -1452,7 +1490,7 @@ int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc,
>   	int wait_index, index;
>   	unsigned long flags;
>   	int offset, shift;
> -	int rc, i;
> +	int rc = 0, fault, i;
>   	u64 type;
>   
>   	if (!qi)
> @@ -1528,9 +1566,19 @@ int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc,
>   		 * a deadlock where the interrupt context can wait indefinitely
>   		 * for free slots in the queue.
>   		 */
> -		rc = qi_check_fault(iommu, index, wait_index);
> -		if (rc)
> +		fault = qi_check_fault(iommu, index, wait_index);
> +		if (fault == -EINVAL) {
> +			/*
> +			 * IQE in our batch: drain the remaining descriptors
> +			 * and keep polling until hardware completes.
> +			 */
> +			qi_drain_remaining_descs(iommu, wait_index, shift,
> +						  options);
> +			rc = -EINVAL;
> +		} else if (fault) {
> +			rc = fault;
>   			break;
> +		}

As a result, there is no need to change the caller of qi_check_fault().

>   
>   		raw_spin_unlock(&qi->q_lock);
>   		cpu_relax();

Thanks,
baolu