On Mon, Jun 14, 2021 at 6:51 PM Shameerali Kolothum Thodi
[off-list ref] wrote:

quoted

-----Original Message-----
From: Robin Murphy [mailto:robin.murphy@arm.com]
Sent: 14 June 2021 11:06
To: Shameerali Kolothum Thodi <redacted>;
linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
iommu@lists.linux-foundation.org
Cc: jon@solid-run.com; Linuxarm <redacted>;
steven.price@arm.com; Guohanjun (Hanjun Guo) [off-list ref];
yangyicong [off-list ref]; Sami.Mujawar@arm.com;
wanghuiqiang [off-list ref]
Subject: Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and
install bypass SMR

On 2021-05-24 12:02, 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: Steven Price <steven.price@arm.com>
Signed-off-by: Shameer Kolothum

[off-list ref]

quoted

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

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

quoted

  1 file changed, 65 insertions(+)

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

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

quoted

index 6f72c4d208ca..56db3d3238fc 100644

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

@@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
    return err;
  }

+static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device

*smmu)

quoted

+{
+   struct list_head rmr_list;
+   struct iommu_resv_region *e;
+   int i, cnt = 0;
+   u32 smr;
+   u32 reg;
+
+   INIT_LIST_HEAD(&rmr_list);
+   if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
+           return;
+
+   reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
+
+   if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg &

ARM_SMMU_sCR0_CLIENTPD)) {

quoted

+           /*
+            * 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));

To reiterate, just because a bootloader/crashed kernel/whatever may have
left some configuration behind doesn't mean that it matters (e.g. what
if these SMRs are pointing at translation contexts?). All we should be
doing here is setting the relevant RMR accommodations in our "clean
slate" software state before the reset routine applies it to the
hardware, just like patch #5 does for SMMUv3.

Trying to safely reset an SMMU when we discover it with CLIENTPD=0 is
really another issue entirely, and I'd say is beyond the scope of this
series

quoted

+                   if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
+                           continue;

Note that that's not even necessarily correct (thanks to EXIDS).

quoted

+                   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;
+           }
+   }
+
+   list_for_each_entry(e, &rmr_list, list) {
+           u32 sid = e->fw_data.rmr.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;

This is very wrong (as has now already been pointed out).

quoted

+                   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;

Nit: cbndx is left uninitialised for bypass/fault entries elsewhere, so
there's little point touching it here.

quoted

+
+           cnt++;
+   }
+
+   if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg &

ARM_SMMU_sCR0_CLIENTPD)) {

quoted

+           /* 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;
+           }

If this dance is only about avoiding stream match conflicts when trying
to reprogram live SMRs, simply turning the SMMU off beforehand would be
a lot simpler.

Hi Steve/Jon,

Since I don’t have access to an SMMUv2 setup, really appreciate if one of you
could please take a look at the above comments and provide me with a tested
code so that I can include it in the v6 that I am planning to send out soon.

Will do.  Thanks.
Jon

Thanks,
Shameer

quoted

Robin.

quoted

+   }
+
+   dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
+              cnt == 1 ? "" : "s");
+   iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);
+}
+
  static int arm_smmu_device_probe(struct platform_device *pdev)
  {
    struct resource *res;

@@ -2168,6 +2229,10 @@ static int arm_smmu_device_probe(struct

platform_device *pdev)

quoted

    }

    platform_set_drvdata(pdev, smmu);
+
+   /* Check for RMRs and install bypass SMRs if any */
+   arm_smmu_rmr_install_bypass_smr(smmu);
+
    arm_smmu_device_reset(smmu);
    arm_smmu_test_smr_masks(smmu);

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