2019-06-05 21:10:30

by Bjorn Andersson

Subject: [RFC 2/2] iommu: arm-smmu: Don't blindly use first SMR to calculate mask

With the SMRs inherited from the bootloader the first SMR might actually
be valid and in use. As such probing the SMR mask using the first SMR
might break a stream in use. Search for an unused stream and use this to
probe the SMR mask.

Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/iommu/arm-smmu.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index c8629a656b42..0c6f5fe6f382 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1084,23 +1084,35 @@ static void arm_smmu_test_smr_masks(struct arm_smmu_device *smmu)
{
void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
u32 smr;
+ int idx;

if (!smmu->smrs)
return;

+ for (idx = 0; idx < smmu->num_mapping_groups; idx++) {
+ smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(idx));
+ if (!(smr & SMR_VALID))
+ break;
+ }
+
+ if (idx == smmu->num_mapping_groups) {
+ dev_err(smmu->dev, "Unable to compute streamid_mask\n");
+ return;
+ }
+
/*
* SMR.ID bits may not be preserved if the corresponding MASK
* bits are set, so check each one separately. We can reject
* masters later if they try to claim IDs outside these masks.
*/
smr = smmu->streamid_mask << SMR_ID_SHIFT;
- writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(0));
- smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(0));
+ writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(idx));
+ smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(idx));
smmu->streamid_mask = smr >> SMR_ID_SHIFT;

smr = smmu->streamid_mask << SMR_MASK_SHIFT;
- writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(0));
- smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(0));
+ writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(idx));
+ smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(idx));
smmu->smr_mask_mask = smr >> SMR_MASK_SHIFT;
}

--
2.18.0

2019-06-12 18:11:32

by Jeffrey Hugo

Subject: Re: [RFC 2/2] iommu: arm-smmu: Don't blindly use first SMR to calculate mask

On Wed, Jun 5, 2019 at 3:09 PM Bjorn Andersson
<[email protected]> wrote:
>
> With the SMRs inherited from the bootloader the first SMR might actually
> be valid and in use. As such probing the SMR mask using the first SMR
> might break a stream in use. Search for an unused stream and use this to
> probe the SMR mask.
>
> Signed-off-by: Bjorn Andersson <[email protected]>

Reviewed-by: Jeffrey Hugo <[email protected]>

I don't quite like the situation where the is no SMR to compute the mask, but I
think the way you've handled it is the best option/

I'm curious, why is this not included in patch #1? Seems like patch
#1 introduces
the issue, yet doesn't also fix it.

> ---
> drivers/iommu/arm-smmu.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index c8629a656b42..0c6f5fe6f382 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1084,23 +1084,35 @@ static void arm_smmu_test_smr_masks(struct arm_smmu_device *smmu)
> {
> void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> u32 smr;
> + int idx;
>
> if (!smmu->smrs)
> return;
>
> + for (idx = 0; idx < smmu->num_mapping_groups; idx++) {
> + smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(idx));
> + if (!(smr & SMR_VALID))
> + break;
> + }
> +
> + if (idx == smmu->num_mapping_groups) {
> + dev_err(smmu->dev, "Unable to compute streamid_mask\n");
> + return;
> + }
> +
> /*
> * SMR.ID bits may not be preserved if the corresponding MASK
> * bits are set, so check each one separately. We can reject
> * masters later if they try to claim IDs outside these masks.
> */
> smr = smmu->streamid_mask << SMR_ID_SHIFT;
> - writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(0));
> - smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(0));
> + writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(idx));
> + smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(idx));
> smmu->streamid_mask = smr >> SMR_ID_SHIFT;
>
> smr = smmu->streamid_mask << SMR_MASK_SHIFT;
> - writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(0));
> - smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(0));
> + writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(idx));
> + smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(idx));
> smmu->smr_mask_mask = smr >> SMR_MASK_SHIFT;
> }
>
> --
> 2.18.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2019-06-12 18:29:53

by Bjorn Andersson

Subject: Re: [RFC 2/2] iommu: arm-smmu: Don't blindly use first SMR to calculate mask

On Wed 12 Jun 10:58 PDT 2019, Jeffrey Hugo wrote:

> On Wed, Jun 5, 2019 at 3:09 PM Bjorn Andersson
> <[email protected]> wrote:
> >
> > With the SMRs inherited from the bootloader the first SMR might actually
> > be valid and in use. As such probing the SMR mask using the first SMR
> > might break a stream in use. Search for an unused stream and use this to
> > probe the SMR mask.
> >
> > Signed-off-by: Bjorn Andersson <[email protected]>
>
> Reviewed-by: Jeffrey Hugo <[email protected]>
>
> I don't quite like the situation where the is no SMR to compute the mask, but I
> think the way you've handled it is the best option/
>

Right, if this happens we would end up using the smr_mask that was
previously calculated. We just won't update it based on the hardware.

> I'm curious, why is this not included in patch #1? Seems like patch
> #1 introduces
> the issue, yet doesn't also fix it.
>

You're right, didn't think about that. This needs to either predate that
patch or be included in it.

Thanks,
Bjorn

> > ---
> > drivers/iommu/arm-smmu.c | 20 ++++++++++++++++----
> > 1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index c8629a656b42..0c6f5fe6f382 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -1084,23 +1084,35 @@ static void arm_smmu_test_smr_masks(struct arm_smmu_device *smmu)
> > {
> > void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> > u32 smr;
> > + int idx;
> >
> > if (!smmu->smrs)
> > return;
> >
> > + for (idx = 0; idx < smmu->num_mapping_groups; idx++) {
> > + smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(idx));
> > + if (!(smr & SMR_VALID))
> > + break;
> > + }
> > +
> > + if (idx == smmu->num_mapping_groups) {
> > + dev_err(smmu->dev, "Unable to compute streamid_mask\n");
> > + return;
> > + }
> > +
> > /*
> > * SMR.ID bits may not be preserved if the corresponding MASK
> > * bits are set, so check each one separately. We can reject
> > * masters later if they try to claim IDs outside these masks.
> > */
> > smr = smmu->streamid_mask << SMR_ID_SHIFT;
> > - writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(0));
> > - smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(0));
> > + writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(idx));
> > + smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(idx));
> > smmu->streamid_mask = smr >> SMR_ID_SHIFT;
> >
> > smr = smmu->streamid_mask << SMR_MASK_SHIFT;
> > - writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(0));
> > - smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(0));
> > + writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(idx));
> > + smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(idx));
> > smmu->smr_mask_mask = smr >> SMR_MASK_SHIFT;
> > }
> >
> > --
> > 2.18.0
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel