-----Original Message-----
From: Steven Price [mailto:steven.price@arm.com]
Sent: 06 May 2021 16:17
To: Shameerali Kolothum Thodi <redacted>;
linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
iommu@lists.linux-foundation.org
Cc: Linuxarm <redacted>; lorenzo.pieralisi@arm.com;
joro@8bytes.org; robin.murphy@arm.com; wanghuiqiang
[off-list ref]; Guohanjun (Hanjun Guo)
[off-list ref]; Sami.Mujawar@arm.com; jon@solid-run.com;
eric.auger@redhat.com
Subject: Re: [PATCH v3 09/10] iommu/arm-smmu: Get associated RMR info
and install bypass SMR

On 20/04/2021 09:27, Shameer Kolothum wrote:

quoted

From: Jon Nettleton <redacted>

Check if there is any RMR info associated with the devices behind
the SMMU and if any, install bypass SMRs for them. This is to
keep any ongoing traffic associated with these devices alive
when we enable/reset SMMU during probe().

Signed-off-by: Jon Nettleton <redacted>
Signed-off-by: Shameer Kolothum

[off-list ref]

quoted

---
  drivers/iommu/arm/arm-smmu/arm-smmu.c | 42

+++++++++++++++++++++++++++

quoted

  drivers/iommu/arm/arm-smmu/arm-smmu.h |  2 ++
  2 files changed, 44 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c

b/drivers/iommu/arm/arm-smmu/arm-smmu.c

quoted

index d8c6bfde6a61..4d2f91626d87 100644

--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c

@@ -2102,6 +2102,43 @@ err_reset_platform_ops: __maybe_unused;
  	return err;
  }

+static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device

*smmu)

quoted

+{
+	struct iommu_rmr *e;
+	int i, cnt = 0;
+	u32 smr;
+
+	for (i = 0; i < smmu->num_mapping_groups; i++) {
+		smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
+		if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
+			continue;
+
+		list_for_each_entry(e, &smmu->rmr_list, list) {
+			if (FIELD_GET(ARM_SMMU_SMR_ID, smr) != e->sid)
+				continue;
+
+			smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
+			smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK,

smr);

quoted

+			smmu->smrs[i].valid = true;
+
+			smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
+			smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
+			smmu->s2crs[i].cbndx = 0xff;
+
+			cnt++;
+		}
+	}

If I understand this correctly - this is looking at the current
(hardware) configuration of the SMMU and attempting to preserve any
bypass SMRs. However from what I can tell it suffers from the following
two problems:

  (a) Only the ID of the SMR is being checked, not the MASK. So if the
firmware has setup an SMR matching a number of streams this will break.

  (b) The SMMU might not be enabled at all (CLIENTPD==1) or bypass
enabled for unmatched streams (USFCFG==0).

Certainly in my test setup case (b) applies and so this doesn't work.
Perhaps something like the below would work better? (It works in the
case of the SMMU not enabled - I've not tested case (a)).

Thanks Steve for taking a look and testing this on SMMUv2. My knowledge
on SMMUv2 is limited an don't have a setup to verify this. After reading
the code, agree that the current implementation addresses the hardware
configuration only and breaks all the scenarios explained above.

I will include the below snippet in next respin if that works.

Hi Jon,

Could you please take a look and see the below changes works for
you too?

Thanks,
Shameer

----8<----
static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device
*smmu)
{
	struct iommu_rmr *e;
	int i, cnt = 0;
	u32 smr;
	u32 reg;

	reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);

	if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg &
ARM_SMMU_sCR0_CLIENTPD)) {
		/*
		 * SMMU is already enabled and disallowing bypass, so preserve
		 * the existing SMRs
		 */
		for (i = 0; i < smmu->num_mapping_groups; i++) {
			smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
			if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
				continue;
			smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
			smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK,
smr);
			smmu->smrs[i].valid = true;
		}
	}

	list_for_each_entry(e, &smmu->rmr_list, list) {
		u32 sid = e->sid;

		i = arm_smmu_find_sme(smmu, sid, ~0);
		if (i < 0)
			continue;
		if (smmu->s2crs[i].count == 0) {
			smmu->smrs[i].id = sid;
			smmu->smrs[i].mask = ~0;
			smmu->smrs[i].valid = true;
		}
		smmu->s2crs[i].count++;
		smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
		smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
		smmu->s2crs[i].cbndx = 0xff;

		cnt++;
	}

	if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg &
ARM_SMMU_sCR0_CLIENTPD)) {
		/* Remove the valid bit for unused SMRs */
		for (i = 0; i < smmu->num_mapping_groups; i++) {
			if (smmu->s2crs[i].count == 0)
				smmu->smrs[i].valid = false;
		}
	}

	dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
		   cnt == 1 ? "" : "s");
}

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel