[v5,7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR

Message ID 20210524110222.2212-8-shameerali.kolothum.thodi@huawei.com
State New
Headers show
Series
  • ACPI/IORT: Support for IORT RMR node
Related show

Commit Message

Shameerali Kolothum Thodi May 24, 2021, 11:02 a.m.
From: Jon Nettleton <jon@solid-run.com>


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 <jon@solid-run.com>

Signed-off-by: Steven Price <steven.price@arm.com>

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 65 +++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

-- 
2.17.1

Comments

Jon Nettleton June 3, 2021, 8:52 a.m. | #1
On Mon, May 24, 2021 at 1:04 PM Shameer Kolothum
<shameerali.kolothum.thodi@huawei.com> wrote:
>

> From: Jon Nettleton <jon@solid-run.com>

>

> 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 <jon@solid-run.com>

> Signed-off-by: Steven Price <steven.price@arm.com>

> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

> ---

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

>  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

> 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)

> +{

> +       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)) {

> +               /*

> +                * 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, &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;

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

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

>         }

>

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

>

> --

> 2.17.1

>


Shameer and Robin

I finally got around to updating edk2 and the HoneyComb IORT tables to
reflect the new standards.
Out of the box the new patchset was generating errors immediatly after
the smmu bringup.

arm-smmu arm-smmu.0.auto: Unhandled context fault: fsr=0x402, iova=0x2080000140,
fsynr=0x1d0040, cbfrsynra=0x4000, cb=0

These errors were generated even with disable_bypass=0

I tracked down the issue to

This code is skipped as Robin said would be correct

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

> +               }[    2.707729] arm-smmu: setting up rmr list on 0x4000

[    2.712598] arm-smmu: s2crs count is 0 smrs index 0x0
[    2.717638] arm-smmu: s2crs count is 0 smrs id is 0x4000
[    2.722939] arm-smmu: s2crs count is 0 smrs mask is 0x8000
[    2.728417] arm-smmu arm-smmu.0.auto:        preserved 1 boot mapping

> +       }


Then this code block was hit which is correct

> +               if (smmu->s2crs[i].count == 0) {

> +                       smmu->smrs[i].id = sid;

> +                       smmu->smrs[i].mask = ~0;

> +                       smmu->smrs[i].valid = true;

> +               }


The mask was causing the issue.  If I think ammended that segment to read
the mask as setup by the hardware everything was back to functioning both
with and without disable_bypass set.

Some debug from that section when it is working

[    2.712598] arm-smmu: s2crs count is 0 smrs index 0x0
[    2.717638] arm-smmu: s2crs count is 0 smrs id is 0x4000
[    2.722939] arm-smmu: s2crs count is 0 smrs mask is 0x8000
[    2.728417] arm-smmu arm-smmu.0.auto:        preserved 1 boot mapping

Robin if anything jumps out at you let me know, otherwise I will debug further.

-Jon
Steven Price June 3, 2021, 11:27 a.m. | #2
On 03/06/2021 09:52, Jon Nettleton wrote:
> On Mon, May 24, 2021 at 1:04 PM Shameer Kolothum

> <shameerali.kolothum.thodi@huawei.com> wrote:

>>

>> From: Jon Nettleton <jon@solid-run.com>

>>

>> 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 <jon@solid-run.com>

>> Signed-off-by: Steven Price <steven.price@arm.com>

>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

>> ---

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

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

>> 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)

>> +{

>> +       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)) {

>> +               /*

>> +                * 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, &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;


Looking at this code again, that mask looks wrong! According to the SMMU
spec MASK[i]==1 means ID[i] is ignored. I.e. this means completely
ignore the ID...

I'm not at all sure why they designed the hardware that way round.

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

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

>>         }

>>

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

>>

>> --

>> 2.17.1

>>

> 

> Shameer and Robin

> 

> I finally got around to updating edk2 and the HoneyComb IORT tables to

> reflect the new standards.

> Out of the box the new patchset was generating errors immediatly after

> the smmu bringup.

> 

> arm-smmu arm-smmu.0.auto: Unhandled context fault: fsr=0x402, iova=0x2080000140,

> fsynr=0x1d0040, cbfrsynra=0x4000, cb=0

> 

> These errors were generated even with disable_bypass=0

> 

> I tracked down the issue to

> 

> This code is skipped as Robin said would be correct


If you're skipping the first bit of code, then that (hopefully) means
that the SMMU is disabled...

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

>> +               }[    2.707729] arm-smmu: setting up rmr list on 0x4000

> [    2.712598] arm-smmu: s2crs count is 0 smrs index 0x0

> [    2.717638] arm-smmu: s2crs count is 0 smrs id is 0x4000

> [    2.722939] arm-smmu: s2crs count is 0 smrs mask is 0x8000

> [    2.728417] arm-smmu arm-smmu.0.auto:        preserved 1 boot mapping

> 

>> +       }

> 

> Then this code block was hit which is correct

> 

>> +               if (smmu->s2crs[i].count == 0) {

>> +                       smmu->smrs[i].id = sid;

>> +                       smmu->smrs[i].mask = ~0;

>> +                       smmu->smrs[i].valid = true;

>> +               }

> 

> The mask was causing the issue.  If I think ammended that segment to read

> the mask as setup by the hardware everything was back to functioning both

> with and without disable_bypass set.


...so reading a mask from it doesn't sounds quite right.

Can you have a go with a corrected mask of '0' rather than all-1s and
see if that helps? My guess is that the mask of ~0 was causing multiple
matches in the SMRs which is 'UNPREDICTABLE'.

Sadly in my test setup there's only the one device behind the SMMU so
I didn't spot this (below patch works for me, but that's not saying
much).

Steve

--->8---
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 56db3d3238fc..44831d0c78e4 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -2079,7 +2079,7 @@ static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
                        continue;
                if (smmu->s2crs[i].count == 0) {
                        smmu->smrs[i].id = sid;
-                       smmu->smrs[i].mask = ~0;
+                       smmu->smrs[i].mask = 0;
                        smmu->smrs[i].valid = true;
                }
                smmu->s2crs[i].count++;
Jon Nettleton June 3, 2021, 11:51 a.m. | #3
On Thu, Jun 3, 2021 at 1:27 PM Steven Price <steven.price@arm.com> wrote:
>

> On 03/06/2021 09:52, Jon Nettleton wrote:

> > On Mon, May 24, 2021 at 1:04 PM Shameer Kolothum

> > <shameerali.kolothum.thodi@huawei.com> wrote:

> >>

> >> From: Jon Nettleton <jon@solid-run.com>

> >>

> >> 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 <jon@solid-run.com>

> >> Signed-off-by: Steven Price <steven.price@arm.com>

> >> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

> >> ---

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

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

> >> 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)

> >> +{

> >> +       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)) {

> >> +               /*

> >> +                * 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, &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;

>

> Looking at this code again, that mask looks wrong! According to the SMMU

> spec MASK[i]==1 means ID[i] is ignored. I.e. this means completely

> ignore the ID...

>

> I'm not at all sure why they designed the hardware that way round.

>

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

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

> >>         }

> >>

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

> >>

> >> --

> >> 2.17.1

> >>

> >

> > Shameer and Robin

> >

> > I finally got around to updating edk2 and the HoneyComb IORT tables to

> > reflect the new standards.

> > Out of the box the new patchset was generating errors immediatly after

> > the smmu bringup.

> >

> > arm-smmu arm-smmu.0.auto: Unhandled context fault: fsr=0x402, iova=0x2080000140,

> > fsynr=0x1d0040, cbfrsynra=0x4000, cb=0

> >

> > These errors were generated even with disable_bypass=0

> >

> > I tracked down the issue to

> >

> > This code is skipped as Robin said would be correct

>

> If you're skipping the first bit of code, then that (hopefully) means

> that the SMMU is disabled...

>

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

> >> +               }[    2.707729] arm-smmu: setting up rmr list on 0x4000

> > [    2.712598] arm-smmu: s2crs count is 0 smrs index 0x0

> > [    2.717638] arm-smmu: s2crs count is 0 smrs id is 0x4000

> > [    2.722939] arm-smmu: s2crs count is 0 smrs mask is 0x8000

> > [    2.728417] arm-smmu arm-smmu.0.auto:        preserved 1 boot mapping

> >

> >> +       }

> >

> > Then this code block was hit which is correct

> >

> >> +               if (smmu->s2crs[i].count == 0) {

> >> +                       smmu->smrs[i].id = sid;

> >> +                       smmu->smrs[i].mask = ~0;

> >> +                       smmu->smrs[i].valid = true;

> >> +               }

> >

> > The mask was causing the issue.  If I think ammended that segment to read

> > the mask as setup by the hardware everything was back to functioning both

> > with and without disable_bypass set.

>

> ...so reading a mask from it doesn't sounds quite right.

>

> Can you have a go with a corrected mask of '0' rather than all-1s and

> see if that helps? My guess is that the mask of ~0 was causing multiple

> matches in the SMRs which is 'UNPREDICTABLE'.

>

> Sadly in my test setup there's only the one device behind the SMMU so

> I didn't spot this (below patch works for me, but that's not saying

> much).

>

> Steve

>

> --->8---

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

> index 56db3d3238fc..44831d0c78e4 100644

> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c

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

> @@ -2079,7 +2079,7 @@ static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)

>                         continue;

>                 if (smmu->s2crs[i].count == 0) {

>                         smmu->smrs[i].id = sid;

> -                       smmu->smrs[i].mask = ~0;

> +                       smmu->smrs[i].mask = 0;

>                         smmu->smrs[i].valid = true;

>                 }

>                 smmu->s2crs[i].count++;


Yes this works fine. Thanks
Jon Nettleton June 13, 2021, 7:40 a.m. | #4
On Thu, Jun 3, 2021 at 1:51 PM Jon Nettleton <jon@solid-run.com> wrote:
>

> On Thu, Jun 3, 2021 at 1:27 PM Steven Price <steven.price@arm.com> wrote:

> >

> > On 03/06/2021 09:52, Jon Nettleton wrote:

> > > On Mon, May 24, 2021 at 1:04 PM Shameer Kolothum

> > > <shameerali.kolothum.thodi@huawei.com> wrote:

> > >>

> > >> From: Jon Nettleton <jon@solid-run.com>

> > >>

> > >> 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 <jon@solid-run.com>

> > >> Signed-off-by: Steven Price <steven.price@arm.com>

> > >> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

> > >> ---

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

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

> > >> 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)

> > >> +{

> > >> +       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)) {

> > >> +               /*

> > >> +                * 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, &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;

> >

> > Looking at this code again, that mask looks wrong! According to the SMMU

> > spec MASK[i]==1 means ID[i] is ignored. I.e. this means completely

> > ignore the ID...

> >

> > I'm not at all sure why they designed the hardware that way round.

> >

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

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

> > >>         }

> > >>

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

> > >>

> > >> --

> > >> 2.17.1

> > >>

> > >

> > > Shameer and Robin

> > >

> > > I finally got around to updating edk2 and the HoneyComb IORT tables to

> > > reflect the new standards.

> > > Out of the box the new patchset was generating errors immediatly after

> > > the smmu bringup.

> > >

> > > arm-smmu arm-smmu.0.auto: Unhandled context fault: fsr=0x402, iova=0x2080000140,

> > > fsynr=0x1d0040, cbfrsynra=0x4000, cb=0

> > >

> > > These errors were generated even with disable_bypass=0

> > >

> > > I tracked down the issue to

> > >

> > > This code is skipped as Robin said would be correct

> >

> > If you're skipping the first bit of code, then that (hopefully) means

> > that the SMMU is disabled...

> >

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

> > >> +               }[    2.707729] arm-smmu: setting up rmr list on 0x4000

> > > [    2.712598] arm-smmu: s2crs count is 0 smrs index 0x0

> > > [    2.717638] arm-smmu: s2crs count is 0 smrs id is 0x4000

> > > [    2.722939] arm-smmu: s2crs count is 0 smrs mask is 0x8000

> > > [    2.728417] arm-smmu arm-smmu.0.auto:        preserved 1 boot mapping

> > >

> > >> +       }

> > >

> > > Then this code block was hit which is correct

> > >

> > >> +               if (smmu->s2crs[i].count == 0) {

> > >> +                       smmu->smrs[i].id = sid;

> > >> +                       smmu->smrs[i].mask = ~0;

> > >> +                       smmu->smrs[i].valid = true;

> > >> +               }

> > >

> > > The mask was causing the issue.  If I think ammended that segment to read

> > > the mask as setup by the hardware everything was back to functioning both

> > > with and without disable_bypass set.

> >

> > ...so reading a mask from it doesn't sounds quite right.

> >

> > Can you have a go with a corrected mask of '0' rather than all-1s and

> > see if that helps? My guess is that the mask of ~0 was causing multiple

> > matches in the SMRs which is 'UNPREDICTABLE'.

> >

> > Sadly in my test setup there's only the one device behind the SMMU so

> > I didn't spot this (below patch works for me, but that's not saying

> > much).

> >

> > Steve

> >

> > --->8---

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

> > index 56db3d3238fc..44831d0c78e4 100644

> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c

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

> > @@ -2079,7 +2079,7 @@ static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)

> >                         continue;

> >                 if (smmu->s2crs[i].count == 0) {

> >                         smmu->smrs[i].id = sid;

> > -                       smmu->smrs[i].mask = ~0;

> > +                       smmu->smrs[i].mask = 0;

> >                         smmu->smrs[i].valid = true;

> >                 }

> >                 smmu->s2crs[i].count++;

>

> Yes this works fine. Thanks


Shameer,

Can you pick up this change into your next patch set?  Also are there
any objections to adding this to the SMMUv2 code from the maintainers?

-Jon
Robin Murphy June 14, 2021, 9:23 a.m. | #5
On 2021-06-13 08:40, Jon Nettleton wrote:
> On Thu, Jun 3, 2021 at 1:51 PM Jon Nettleton <jon@solid-run.com> wrote:

>>

>> On Thu, Jun 3, 2021 at 1:27 PM Steven Price <steven.price@arm.com> wrote:

>>>

>>> On 03/06/2021 09:52, Jon Nettleton wrote:

>>>> On Mon, May 24, 2021 at 1:04 PM Shameer Kolothum

>>>> <shameerali.kolothum.thodi@huawei.com> wrote:

>>>>>

>>>>> From: Jon Nettleton <jon@solid-run.com>

>>>>>

>>>>> 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 <jon@solid-run.com>

>>>>> Signed-off-by: Steven Price <steven.price@arm.com>

>>>>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

>>>>> ---

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

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

>>>>> 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)

>>>>> +{

>>>>> +       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)) {

>>>>> +               /*

>>>>> +                * 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, &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;

>>>

>>> Looking at this code again, that mask looks wrong! According to the SMMU

>>> spec MASK[i]==1 means ID[i] is ignored. I.e. this means completely

>>> ignore the ID...

>>>

>>> I'm not at all sure why they designed the hardware that way round.

>>>

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

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

>>>>>          }

>>>>>

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

>>>>>

>>>>> --

>>>>> 2.17.1

>>>>>

>>>>

>>>> Shameer and Robin

>>>>

>>>> I finally got around to updating edk2 and the HoneyComb IORT tables to

>>>> reflect the new standards.

>>>> Out of the box the new patchset was generating errors immediatly after

>>>> the smmu bringup.

>>>>

>>>> arm-smmu arm-smmu.0.auto: Unhandled context fault: fsr=0x402, iova=0x2080000140,

>>>> fsynr=0x1d0040, cbfrsynra=0x4000, cb=0

>>>>

>>>> These errors were generated even with disable_bypass=0

>>>>

>>>> I tracked down the issue to

>>>>

>>>> This code is skipped as Robin said would be correct

>>>

>>> If you're skipping the first bit of code, then that (hopefully) means

>>> that the SMMU is disabled...

>>>

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

>>>>> +               }[    2.707729] arm-smmu: setting up rmr list on 0x4000

>>>> [    2.712598] arm-smmu: s2crs count is 0 smrs index 0x0

>>>> [    2.717638] arm-smmu: s2crs count is 0 smrs id is 0x4000

>>>> [    2.722939] arm-smmu: s2crs count is 0 smrs mask is 0x8000

>>>> [    2.728417] arm-smmu arm-smmu.0.auto:        preserved 1 boot mapping

>>>>

>>>>> +       }

>>>>

>>>> Then this code block was hit which is correct

>>>>

>>>>> +               if (smmu->s2crs[i].count == 0) {

>>>>> +                       smmu->smrs[i].id = sid;

>>>>> +                       smmu->smrs[i].mask = ~0;

>>>>> +                       smmu->smrs[i].valid = true;

>>>>> +               }

>>>>

>>>> The mask was causing the issue.  If I think ammended that segment to read

>>>> the mask as setup by the hardware everything was back to functioning both

>>>> with and without disable_bypass set.

>>>

>>> ...so reading a mask from it doesn't sounds quite right.

>>>

>>> Can you have a go with a corrected mask of '0' rather than all-1s and

>>> see if that helps? My guess is that the mask of ~0 was causing multiple

>>> matches in the SMRs which is 'UNPREDICTABLE'.

>>>

>>> Sadly in my test setup there's only the one device behind the SMMU so

>>> I didn't spot this (below patch works for me, but that's not saying

>>> much).

>>>

>>> Steve

>>>

>>> --->8---

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

>>> index 56db3d3238fc..44831d0c78e4 100644

>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c

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

>>> @@ -2079,7 +2079,7 @@ static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)

>>>                          continue;

>>>                  if (smmu->s2crs[i].count == 0) {

>>>                          smmu->smrs[i].id = sid;

>>> -                       smmu->smrs[i].mask = ~0;

>>> +                       smmu->smrs[i].mask = 0;

>>>                          smmu->smrs[i].valid = true;

>>>                  }

>>>                  smmu->s2crs[i].count++;

>>

>> Yes this works fine. Thanks

> 

> Shameer,

> 

> Can you pick up this change into your next patch set?  Also are there

> any objections to adding this to the SMMUv2 code from the maintainers?


Urgh, I was rather confused here since I knew I'd already written a 
review of an earlier version pointing this out along with a couple of 
other issues... then I found it still sat in my drafts folder :(

Let me just "rebase" those comments to v5...

Robin.
Robin Murphy June 14, 2021, 10:06 a.m. | #6
On 2021-05-24 12:02, Shameer Kolothum wrote:
> From: Jon Nettleton <jon@solid-run.com>

> 

> 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 <jon@solid-run.com>

> Signed-off-by: Steven Price <steven.price@arm.com>

> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

> ---

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

>   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

> 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)

> +{

> +	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)) {

> +		/*

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

> +			if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))

> +				continue;


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

> +			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, &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).

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

> +

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

> +		}


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.

Robin.

> +	}

> +

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

>   	}

>   

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

>   

>
Shameerali Kolothum Thodi June 14, 2021, 4:51 p.m. | #7
> -----Original Message-----

> From: Robin Murphy [mailto:robin.murphy@arm.com]

> Sent: 14 June 2021 11:06

> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;

> linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;

> iommu@lists.linux-foundation.org

> Cc: jon@solid-run.com; Linuxarm <linuxarm@huawei.com>;

> steven.price@arm.com; Guohanjun (Hanjun Guo) <guohanjun@huawei.com>;

> yangyicong <yangyicong@huawei.com>; Sami.Mujawar@arm.com;

> wanghuiqiang <wanghuiqiang@huawei.com>

> 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:

> > From: Jon Nettleton <jon@solid-run.com>

> >

> > 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 <jon@solid-run.com>

> > Signed-off-by: Steven Price <steven.price@arm.com>

> > Signed-off-by: Shameer Kolothum

> <shameerali.kolothum.thodi@huawei.com>

> > ---

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

> +++++++++++++++++++++++++++

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

> > 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)

> > +{

> > +	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)) {

> > +		/*

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

> 

> > +			if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))

> > +				continue;

> 

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

> 

> > +			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, &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).

> 

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

> 

> > +

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

> > +		}

> 

> 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. 

Thanks,
Shameer

> Robin.

> 

> > +	}

> > +

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

> >   	}

> >

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

> >

> >
Jon Nettleton June 15, 2021, 8:02 a.m. | #8
On Mon, Jun 14, 2021 at 6:51 PM Shameerali Kolothum Thodi
<shameerali.kolothum.thodi@huawei.com> wrote:
>

>

>

> > -----Original Message-----

> > From: Robin Murphy [mailto:robin.murphy@arm.com]

> > Sent: 14 June 2021 11:06

> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;

> > linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;

> > iommu@lists.linux-foundation.org

> > Cc: jon@solid-run.com; Linuxarm <linuxarm@huawei.com>;

> > steven.price@arm.com; Guohanjun (Hanjun Guo) <guohanjun@huawei.com>;

> > yangyicong <yangyicong@huawei.com>; Sami.Mujawar@arm.com;

> > wanghuiqiang <wanghuiqiang@huawei.com>

> > 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:

> > > From: Jon Nettleton <jon@solid-run.com>

> > >

> > > 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 <jon@solid-run.com>

> > > Signed-off-by: Steven Price <steven.price@arm.com>

> > > Signed-off-by: Shameer Kolothum

> > <shameerali.kolothum.thodi@huawei.com>

> > > ---

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

> > +++++++++++++++++++++++++++

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

> > > 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)

> > > +{

> > > +   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)) {

> > > +           /*

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

> >

> > > +                   if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))

> > > +                           continue;

> >

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

> >

> > > +                   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, &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).

> >

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

> >

> > > +

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

> > > +           }

> >

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

>

> > Robin.

> >

> > > +   }

> > > +

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

> > >     }

> > >

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

> > >

> > >
Jon Nettleton June 29, 2021, 7:03 a.m. | #9
On Mon, Jun 14, 2021 at 12:06 PM Robin Murphy <robin.murphy@arm.com> wrote:
>

> On 2021-05-24 12:02, Shameer Kolothum wrote:

> > From: Jon Nettleton <jon@solid-run.com>

> >

> > 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 <jon@solid-run.com>

> > Signed-off-by: Steven Price <steven.price@arm.com>

> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

> > ---

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

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

> > 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)

> > +{

> > +     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)) {

> > +             /*

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

>

> > +                     if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))

> > +                             continue;

>

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

>

> > +                     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, &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).

>

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

>

> > +

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

> > +             }

>

> 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.


Robin,

I am not sure what you mean here, and maybe Steve wants to jump in and
help clarify.

My understanding is that "dance" is required for regions that need to
continue to work
throughout the boot process.  I think the example I have seen the most
is for GOP drivers that
use system memory rather than dedicated VRAM.

-Jon

>

> Robin.

>

> > +     }

> > +

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

> >       }

> >

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

> >

> >
Robin Murphy June 29, 2021, 1:22 p.m. | #10
On 2021-06-29 08:03, Jon Nettleton wrote:
> On Mon, Jun 14, 2021 at 12:06 PM Robin Murphy <robin.murphy@arm.com> wrote:

>>

>> On 2021-05-24 12:02, Shameer Kolothum wrote:

>>> From: Jon Nettleton <jon@solid-run.com>

>>>

>>> 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 <jon@solid-run.com>

>>> Signed-off-by: Steven Price <steven.price@arm.com>

>>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

>>> ---

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

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

>>> 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)

>>> +{

>>> +     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)) {

>>> +             /*

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

>>

>>> +                     if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))

>>> +                             continue;

>>

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

>>

>>> +                     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, &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).

>>

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

>>

>>> +

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

>>> +             }

>>

>> 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.

> 

> Robin,

> 

> I am not sure what you mean here, and maybe Steve wants to jump in and

> help clarify.

> 

> My understanding is that "dance" is required for regions that need to

> continue to work

> throughout the boot process.  I think the example I have seen the most

> is for GOP drivers that

> use system memory rather than dedicated VRAM.


All that is required is to install bypass SMEs for the relevant streams 
before enabling the SMMU. That much is achieved by the 
list_for_each_entry(e, &rmr_list, list) loop setting up initial state 
followed by arm_smmu_device_reset(). The "dance" I'm referring to is the 
additional reading out of (possibly nonsense) SMR state beforehand to 
pre-bias the SMR allocator, then trying to clean up the remnants afterwards.

If we're going to pretend to be robust against finding the SMMU already 
enabled *with* live traffic ongoing, there's frankly an awful lot more 
care we'd need to take beyond here (and for some aspects there's still 
no right answer). If on the other hand we're implicitly assuming that 
any boot-time enabled SMRs exactly match the RMR configuration anyway, 
then simply disabling the SMMU until we enable it again in the reset 
routine still preserves the necessary bypass behaviour for RMR streams 
while sidestepping any issues related to reprogramming live SMMU state.

Robin.

>>> +     }

>>> +

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

>>>        }

>>>

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

>>>

>>>
Jon Nettleton June 29, 2021, 4:25 p.m. | #11
On Tue, Jun 29, 2021 at 3:23 PM Robin Murphy <robin.murphy@arm.com> wrote:
>

> On 2021-06-29 08:03, Jon Nettleton wrote:

> > On Mon, Jun 14, 2021 at 12:06 PM Robin Murphy <robin.murphy@arm.com> wrote:

> >>

> >> On 2021-05-24 12:02, Shameer Kolothum wrote:

> >>> From: Jon Nettleton <jon@solid-run.com>

> >>>

> >>> 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 <jon@solid-run.com>

> >>> Signed-off-by: Steven Price <steven.price@arm.com>

> >>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

> >>> ---

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

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

> >>> 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)

> >>> +{

> >>> +     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)) {

> >>> +             /*

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

> >>

> >>> +                     if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))

> >>> +                             continue;

> >>

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

> >>

> >>> +                     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, &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).

> >>

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

> >>

> >>> +

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

> >>> +             }

> >>

> >> 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.

> >

> > Robin,

> >

> > I am not sure what you mean here, and maybe Steve wants to jump in and

> > help clarify.

> >

> > My understanding is that "dance" is required for regions that need to

> > continue to work

> > throughout the boot process.  I think the example I have seen the most

> > is for GOP drivers that

> > use system memory rather than dedicated VRAM.

>

> All that is required is to install bypass SMEs for the relevant streams

> before enabling the SMMU. That much is achieved by the

> list_for_each_entry(e, &rmr_list, list) loop setting up initial state

> followed by arm_smmu_device_reset(). The "dance" I'm referring to is the

> additional reading out of (possibly nonsense) SMR state beforehand to

> pre-bias the SMR allocator, then trying to clean up the remnants afterwards.

>

> If we're going to pretend to be robust against finding the SMMU already

> enabled *with* live traffic ongoing, there's frankly an awful lot more

> care we'd need to take beyond here (and for some aspects there's still

> no right answer). If on the other hand we're implicitly assuming that

> any boot-time enabled SMRs exactly match the RMR configuration anyway,

> then simply disabling the SMMU until we enable it again in the reset

> routine still preserves the necessary bypass behaviour for RMR streams

> while sidestepping any issues related to reprogramming live SMMU state.

>

> Robin.

>

> >>> +     }

> >>> +

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

> >>>        }

> >>>

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

> >>>

> >>>


Shameer,

Sorry for the delays.  Here is a diff of the changes that should
address the issues pointed out by Robin,
I have tested that this works as expected on my HoneyComb LX2160A.

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index ab7b9db77625..a358bd326d0b 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -2068,29 +2068,21 @@ static void
arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
        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);
+       /* Rather than trying to look at existing mappings that
+        * are setup by the firmware and then invalidate the ones
+        * that do no have matching RMR entries, just disable the
+        * SMMU until it gets enabled again in the reset routine.
+        */

-       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;
-               }
-       }
+       reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
+       reg &= ~ARM_SMMU_sCR0_CLIENTPD;
+       arm_smmu_gr0_write(smmu, ARM_SMMU_GR0_sCR0, reg);

        list_for_each_entry(e, &rmr_list, list) {
                u32 sid = e->fw_data.rmr.sid;
@@ -2100,25 +2092,16 @@ static void
arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
                        continue;
                if (smmu->s2crs[i].count == 0) {
                        smmu->smrs[i].id = sid;
-                       smmu->smrs[i].mask = ~0;
+                       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");
        iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);

Please include that in your next patch series.  Let me know if you
want me to send you the patch direct
off the list.

Thanks,
Jon
Shameerali Kolothum Thodi June 30, 2021, 8:50 a.m. | #12
> -----Original Message-----

> From: Jon Nettleton [mailto:jon@solid-run.com]

> Sent: 29 June 2021 17:26

> To: Robin Murphy <robin.murphy@arm.com>

> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;

> linux-arm-kernel <linux-arm-kernel@lists.infradead.org>; ACPI Devel Maling

> List <linux-acpi@vger.kernel.org>; iommu@lists.linux-foundation.org; Linuxarm

> <linuxarm@huawei.com>; Steven Price <steven.price@arm.com>; Guohanjun

> (Hanjun Guo) <guohanjun@huawei.com>; yangyicong

> <yangyicong@huawei.com>; Sami.Mujawar@arm.com; wanghuiqiang

> <wanghuiqiang@huawei.com>

> Subject: Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and

> install bypass SMR

> 


[...]
 
> Shameer,

> 

> Sorry for the delays.  Here is a diff of the changes that should

> address the issues pointed out by Robin,

> I have tested that this works as expected on my HoneyComb LX2160A.


Ok. Thanks for that.

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

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

> index ab7b9db77625..a358bd326d0b 100644

> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c

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

> @@ -2068,29 +2068,21 @@ static void

> arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)

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

> +       /* Rather than trying to look at existing mappings that

> +        * are setup by the firmware and then invalidate the ones

> +        * that do no have matching RMR entries, just disable the

> +        * SMMU until it gets enabled again in the reset routine.

> +        */

> 

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

> -               }

> -       }

> +       reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);

> +       reg &= ~ARM_SMMU_sCR0_CLIENTPD;

> +       arm_smmu_gr0_write(smmu, ARM_SMMU_GR0_sCR0, reg);

> 

>         list_for_each_entry(e, &rmr_list, list) {

>                 u32 sid = e->fw_data.rmr.sid;

> @@ -2100,25 +2092,16 @@ static void

> arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)

>                         continue;

>                 if (smmu->s2crs[i].count == 0) {

>                         smmu->smrs[i].id = sid;

> -                       smmu->smrs[i].mask = ~0;

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

>         iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);

> 

> Please include that in your next patch series.  Let me know if you

> want me to send you the patch direct

> off the list.


No problem, I will take this in next.

Thanks,
Shameer

Patch

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
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)
+{
+	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)) {
+		/*
+		 * 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, &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;
+			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");
+	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)
 	}
 
 	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);