Received-SPF: pass (google.com: domain of linux-kernel+bounces-26175-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 15 Jan 2024 11:31:52 -0400
From: Jason Gunthorpe <jgg@ziepe.ca>
To: Zhang Zekun <zhangzekun11@huawei.com>
Cc: will@kernel.org, robin.murphy@arm.com, joro@8bytes.org,
	nicolinc@nvidia.com, mshavit@google.com, iommu@lists.linux.dev,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] iommu/arm-smmu-v3: Add a threshold to avoid potential
 soft lockup
Message-ID: <20240115153152.GA50608@ziepe.ca>
References: <20240115114040.6279-1-zhangzekun11@huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240115114040.6279-1-zhangzekun11@huawei.com>

On Mon, Jan 15, 2024 at 07:40:40PM +0800, Zhang Zekun wrote:
> The commit d5afb4b47e13 ("iommu/arm-smmu-v3: Fix soft lockup triggered
> by arm_smmu_mm_invalidate_range") has fix a soft lockup problem when
> running SVA case, but code paths from iommu_unmap and dma APIs still
> remain unfixed which could also cause potential soft lockup.
> 
> When cmdq is quite busy and don't have much space for batch submitting
> cmds, and size passed to __arm_smmu_tlb_inv_range() is large (1G in this
> case), the following softlockup is triggered.
> 
> WARN: soft lockup - CPU#71 stuck for 12s! [qemu-kvm:1303]
> ...
> Call trace:
>  dump_backtrace+0x0/0x200
>  show_stack+0x20/0x30
>  dump_stack+0xf0/0x138
>  watchdog_print_info+0x48/0x54
>  watchdog_process_before_softlockup+0x9c/0xa0
>  watchdog_timer_fn+0x1ac/0x2f0
>  __run_hrtimer+0x98/0x2b4
>  __hrtimer_run_queues+0xc0/0x13c
>  hrtimer_interrupt+0x150/0x3e4
>  arch_timer_handler_phys+0x3c/0x50
>  handle_percpu_devid_irq+0x90/0x1f4
>  __handle_domain_irq+0x84/0xfc
>  gic_handle_irq+0x88/0x2b0
>  el1_irq+0xb8/0x140
>  arm_smmu_cmdq_issue_cmdlist+0x184/0x5f4
>  __arm_smmu_tlb_inv_range+0x114/0x22c
>  arm_smmu_tlb_inv_walk+0x88/0x120
>  __arm_lpae_unmap+0x188/0x2c0
>  __arm_lpae_unmap+0x104/0x2c0
>  arm_lpae_unmap+0x68/0x80
>  arm_smmu_unmap+0x24/0x40
>  __iommu_unmap+0xd8/0x210
>  iommu_unmap+0x44/0x9c
> ...

What is the rest of the call chain? How did you get into such a large
invalidation?

> @@ -228,7 +219,7 @@ static void arm_smmu_mm_arch_invalidate_secondary_tlbs(struct mmu_notifier *mn,
>  	 */
>  	size = end - start;
>  	if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_RANGE_INV)) {
> -		if (size >= CMDQ_MAX_TLBI_OPS * PAGE_SIZE)
> +		if (size >= CMDQ_MAX_TLBI_OPS(PAGE_SIZE) * PAGE_SIZE)
>  			size = 0;
>  	} else {
>  		if (size == ULONG_MAX)

I would like to see the SVA code rely on the common invalidation
infrastructure, this should be pushed down into the invalidation logic
not be in the SVA code. With some adjustments we can get common
functions for all this and push the decision making into the actual
function.

Something like this, on top of my "part 3" branch
https://github.com/jgunthorpe/linux/commits/smmuv3_newapi/
split into more patches:

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 5ab976f6b108dd..b30cce97b01d06 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -30,7 +30,6 @@ static int arm_smmu_realloc_s1_domain_asid(struct arm_smmu_device *smmu,
 					   struct arm_smmu_domain *smmu_domain)
 {
 	struct arm_smmu_master_domain *master_domain;
-	u32 old_asid = smmu_domain->asid;
 	struct arm_smmu_cd target_cd;
 	unsigned long flags;
 	int ret;
@@ -68,9 +67,6 @@ static int arm_smmu_realloc_s1_domain_asid(struct arm_smmu_device *smmu,
 					&target_cd);
 	}
 	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
-
-	/* Clean the ASID we are about to assign to a new translation */
-	arm_smmu_tlb_inv_asid(smmu, old_asid);
 	return 0;
 }
 
@@ -175,24 +171,12 @@ static void arm_smmu_mm_arch_invalidate_secondary_tlbs(struct mmu_notifier *mn,
 	 * range. So do a simple translation here by calculating size correctly.
 	 */
 	size = end - start;
-	if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_RANGE_INV)) {
-		if (size >= CMDQ_MAX_TLBI_OPS * PAGE_SIZE)
-			size = 0;
-	} else {
-		if (size == ULONG_MAX)
-			size = 0;
-	}
+	if (size == ULONG_MAX)
+		size = 0;
 
-	if (!smmu_domain->btm_invalidation) {
-		ioasid_t asid = READ_ONCE(smmu_domain->asid);
-
-		if (!size)
-			arm_smmu_tlb_inv_asid(smmu_domain->smmu, asid);
-		else
-			arm_smmu_tlb_inv_range_asid(start, size, asid,
-						    PAGE_SIZE, false,
-						    smmu_domain);
-	}
+	if (!smmu_domain->btm_invalidation)
+		arm_smmu_tlb_inv_range_s1(start, size, PAGE_SIZE, false,
+					  smmu_domain);
 
 	arm_smmu_atc_inv_domain(smmu_domain, start, size);
 }
@@ -228,7 +212,7 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 	}
 	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
 
-	arm_smmu_tlb_inv_asid(smmu_domain->smmu, smmu_domain->asid);
+	arm_smmu_tlb_inv_all_s1(smmu_domain);
 	arm_smmu_atc_inv_domain(smmu_domain, 0, 0);
 }
 
@@ -506,6 +490,8 @@ static int arm_smmu_share_asid(struct arm_smmu_device *smmu,
 		ret = arm_smmu_realloc_s1_domain_asid(smmu, old_s1_domain);
 		if (ret)
 			goto out_restore_s1;
+		/* Clean the ASID since it was just recovered */
+		arm_smmu_tlb_inv_all_s1(smmu_domain);
 	}
 
 	smmu_domain->btm_invalidation = true;
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index b07e0692eb78ea..79c85791b0c3d5 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -90,6 +90,7 @@ static void arm_smmu_rmr_install_bypass_ste(struct arm_smmu_device *smmu);
 static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
 				    struct arm_smmu_device *smmu);
 static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
+static void arm_smmu_tlb_inv_all_s2(struct arm_smmu_domain *smmu_domain);
 
 static void parse_driver_options(struct arm_smmu_device *smmu)
 {
@@ -965,17 +966,6 @@ static int arm_smmu_page_response(struct device *dev,
 }
 
 /* Context descriptor manipulation functions */
-void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid)
-{
-	struct arm_smmu_cmdq_ent cmd = {
-		.opcode	= smmu->features & ARM_SMMU_FEAT_E2H ?
-			CMDQ_OP_TLBI_EL2_ASID : CMDQ_OP_TLBI_NH_ASID,
-		.tlbi.asid = asid,
-	};
-
-	arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
-}
-
 #define NUM_ENTRY_QWORDS                                                \
 	(max(sizeof(struct arm_smmu_ste), sizeof(struct arm_smmu_cd)) / \
 	 sizeof(u64))
@@ -2106,8 +2096,6 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
 static void arm_smmu_tlb_inv_context(void *cookie)
 {
 	struct arm_smmu_domain *smmu_domain = cookie;
-	struct arm_smmu_device *smmu = smmu_domain->smmu;
-	struct arm_smmu_cmdq_ent cmd;
 
 	/*
 	 * NOTE: when io-pgtable is in non-strict mode, we may get here with
@@ -2116,13 +2104,10 @@ static void arm_smmu_tlb_inv_context(void *cookie)
 	 * insertion to guarantee those are observed before the TLBI. Do be
 	 * careful, 007.
 	 */
-	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
-		arm_smmu_tlb_inv_asid(smmu, READ_ONCE(smmu_domain->asid));
-	} else {
-		cmd.opcode	= CMDQ_OP_TLBI_S12_VMALL;
-		cmd.tlbi.vmid	= smmu_domain->vmid;
-		arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
-	}
+	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1)
+		arm_smmu_tlb_inv_all_s1(smmu_domain);
+	else
+		arm_smmu_tlb_inv_all_s2(smmu_domain);
 	arm_smmu_atc_inv_domain(smmu_domain, 0, 0);
 }
 
@@ -2197,47 +2182,69 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
 	arm_smmu_cmdq_batch_submit(smmu, &cmds);
 }
 
-static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
-					  size_t granule, bool leaf,
-					  struct arm_smmu_domain *smmu_domain)
+static bool arm_smmu_inv_range_too_big(struct arm_smmu_device *smmu,
+				       size_t size, size_t granule)
 {
-	struct arm_smmu_cmdq_ent cmd = {
-		.tlbi = {
-			.leaf	= leaf,
-		},
-	};
+	unsigned max_ops;
 
-	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
-		cmd.opcode	= smmu_domain->smmu->features & ARM_SMMU_FEAT_E2H ?
-				  CMDQ_OP_TLBI_EL2_VA : CMDQ_OP_TLBI_NH_VA;
-		cmd.tlbi.asid	= smmu_domain->asid;
-	} else {
-		cmd.opcode	= CMDQ_OP_TLBI_S2_IPA;
-		cmd.tlbi.vmid	= smmu_domain->vmid;
-	}
-	__arm_smmu_tlb_inv_range(&cmd, iova, size, granule, smmu_domain);
+	/* 0 size means invalidate all */
+	if (!size || size == SIZE_MAX)
+		return true;
 
-	/*
-	 * Unfortunately, this can't be leaf-only since we may have
-	 * zapped an entire table.
-	 */
-	arm_smmu_atc_inv_domain(smmu_domain, iova, size);
+	if (smmu->features & ARM_SMMU_FEAT_RANGE_INV)
+		return false;
+
+	max_ops = 1 << (ilog2(granule) - 3);
+	return size >= max_ops * granule;
 }
 
-void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid,
-				 size_t granule, bool leaf,
-				 struct arm_smmu_domain *smmu_domain)
+void arm_smmu_tlb_inv_range_s1(unsigned long iova, size_t size, size_t granule,
+			       bool leaf, struct arm_smmu_domain *smmu_domain)
 {
 	struct arm_smmu_cmdq_ent cmd = {
 		.opcode	= smmu_domain->smmu->features & ARM_SMMU_FEAT_E2H ?
 			  CMDQ_OP_TLBI_EL2_VA : CMDQ_OP_TLBI_NH_VA,
 		.tlbi = {
-			.asid	= asid,
+			.asid	= READ_ONCE(smmu_domain->asid),
 			.leaf	= leaf,
 		},
 	};
 
-	__arm_smmu_tlb_inv_range(&cmd, iova, size, granule, smmu_domain);
+	if (arm_smmu_inv_range_too_big(smmu_domain->smmu, size, granule)) {
+		cmd.opcode = smmu_domain->smmu->features & ARM_SMMU_FEAT_E2H ?
+				     CMDQ_OP_TLBI_EL2_ASID :
+				     CMDQ_OP_TLBI_NH_ASID,
+		arm_smmu_cmdq_issue_cmd_with_sync(smmu_domain->smmu, &cmd);
+	} else{
+		__arm_smmu_tlb_inv_range(&cmd, iova, size, granule,
+					 smmu_domain);
+	}
+}
+
+static void arm_smmu_tlb_inv_range_s2(unsigned long iova, size_t size,
+				      size_t granule, bool leaf,
+				      struct arm_smmu_domain *smmu_domain)
+{
+	struct arm_smmu_cmdq_ent cmd = {
+		.opcode	= CMDQ_OP_TLBI_S2_IPA,
+		.tlbi = {
+			.vmid	= smmu_domain->vmid,
+			.leaf	= leaf,
+		},
+	};
+
+	if (arm_smmu_inv_range_too_big(smmu_domain->smmu, size, granule)) {
+		cmd.opcode = CMDQ_OP_TLBI_S12_VMALL;
+		arm_smmu_cmdq_issue_cmd_with_sync(smmu_domain->smmu, &cmd);
+	} else {
+		__arm_smmu_tlb_inv_range(&cmd, iova, size, granule,
+					 smmu_domain);
+	}
+}
+
+static void arm_smmu_tlb_inv_all_s2(struct arm_smmu_domain *smmu_domain)
+{
+	arm_smmu_tlb_inv_range_s2(0, 0, PAGE_SIZE, false, smmu_domain);
 }
 
 static void arm_smmu_tlb_inv_page_nosync(struct iommu_iotlb_gather *gather,
@@ -2253,7 +2260,15 @@ static void arm_smmu_tlb_inv_page_nosync(struct iommu_iotlb_gather *gather,
 static void arm_smmu_tlb_inv_walk(unsigned long iova, size_t size,
 				  size_t granule, void *cookie)
 {
-	arm_smmu_tlb_inv_range_domain(iova, size, granule, false, cookie);
+	struct arm_smmu_domain *smmu_domain = cookie;
+
+	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1)
+		arm_smmu_tlb_inv_range_s1(iova, size, granule, false,
+					  smmu_domain);
+	else
+		arm_smmu_tlb_inv_range_s2(iova, size, granule, false,
+					  smmu_domain);
+	arm_smmu_atc_inv_domain(smmu_domain, iova, size);
 }
 
 static const struct iommu_flush_ops arm_smmu_flush_ops = {
@@ -2379,7 +2394,7 @@ void arm_smmu_domain_free_id(struct arm_smmu_domain *smmu_domain)
 	if ((smmu_domain->stage == ARM_SMMU_DOMAIN_S1 ||
 	     smmu_domain->domain.type == IOMMU_DOMAIN_SVA) &&
 	    smmu_domain->asid) {
-		arm_smmu_tlb_inv_asid(smmu, smmu_domain->asid);
+		arm_smmu_tlb_inv_all_s1(smmu_domain);
 
 		/* Prevent SVA from touching the CD while we're freeing it */
 		mutex_lock(&smmu->asid_lock);
@@ -3183,13 +3198,23 @@ static void arm_smmu_iotlb_sync(struct iommu_domain *domain,
 				struct iommu_iotlb_gather *gather)
 {
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+	size_t size = gather->end - gather->start + 1;
 
 	if (!gather->pgsize)
 		return;
 
-	arm_smmu_tlb_inv_range_domain(gather->start,
-				      gather->end - gather->start + 1,
-				      gather->pgsize, true, smmu_domain);
+	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1)
+		arm_smmu_tlb_inv_range_s1(gather->start, size, gather->pgsize,
+					  true, smmu_domain);
+	else
+		arm_smmu_tlb_inv_range_s2(gather->start, size, gather->pgsize,
+					  true, smmu_domain);
+
+	/*
+	 * Unfortunately, this can't be leaf-only since we may have
+	 * zapped an entire table.
+	 */
+	arm_smmu_atc_inv_domain(smmu_domain, gather->start, size);
 }
 
 static phys_addr_t
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index d65c716d4fc04f..9f5537ef97caff 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -834,10 +834,13 @@ void arm_smmu_remove_pasid(struct arm_smmu_master *master,
 int arm_smmu_domain_alloc_id(struct arm_smmu_device *smmu,
 			     struct arm_smmu_domain *smmu_domain);
 void arm_smmu_domain_free_id(struct arm_smmu_domain *smmu_domain);
-void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid);
-void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid,
-				 size_t granule, bool leaf,
-				 struct arm_smmu_domain *smmu_domain);
+void arm_smmu_tlb_inv_range_s1(unsigned long iova, size_t size, size_t granule,
+			       bool leaf, struct arm_smmu_domain *smmu_domain);
+static inline void arm_smmu_tlb_inv_all_s1(struct arm_smmu_domain *smmu_domain)
+{
+	arm_smmu_tlb_inv_range_s1(0, 0, PAGE_SIZE, false, smmu_domain);
+}
+
 int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
 			    unsigned long iova, size_t size);