linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC 0/2] iommu: arm-smmu: Inherit SMR and CB config during init
@ 2019-06-05 21:08 Bjorn Andersson
  2019-06-05 21:08 ` [RFC 1/2] iommu: arm-smmu: Handoff SMR registers and context banks Bjorn Andersson
  2019-06-05 21:08 ` [RFC 2/2] iommu: arm-smmu: Don't blindly use first SMR to calculate mask Bjorn Andersson
  0 siblings, 2 replies; 10+ messages in thread
From: Bjorn Andersson @ 2019-06-05 21:08 UTC (permalink / raw)
  To: Joerg Roedel, Will Deacon, Robin Murphy
  Cc: Vivek Gautam, Jeffrey Hugo, Patrick Daly, iommu, linux-kernel,
	linux-arm-kernel, linux-arm-msm

Qualcomm devices typically boot with some sort of splash screen on the display,
or for some devices (i.e. recent Qualcomm laptops) an EFI framebuffer. For this
the bootloader allocates a static framebuffer, configures the display hardware
to output this on the display, sets up the SMMU for the display hardware and
jumps to the kernel.

But as the arm-smmu disables all SMRs the display hardware will no longer be
able to access the framebuffer and the result is that the device resets.

The given proposal reads back the SMR state at boot and for marks these
contexts as busy. This ensures that the display hardware will have continued
access to the framebuffer. Once a device is attached we try to match it to the
predefined stream mapping, so that e.g. the display driver will inherit the
particular SMRs/CBs.


This has the positive side effect that on some Qualcomm platforms, e.g. QCS404,
writes to the SMR registers are ignored. But as we inherit the preconfigured
mapping from the bootloader we can use the arm-smmu driver on these platforms.

Bjorn Andersson (2):
  iommu: arm-smmu: Handoff SMR registers and context banks
  iommu: arm-smmu: Don't blindly use first SMR to calculate mask

 drivers/iommu/arm-smmu-regs.h |   2 +
 drivers/iommu/arm-smmu.c      | 100 +++++++++++++++++++++++++++++++---
 2 files changed, 93 insertions(+), 9 deletions(-)

-- 
2.18.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [RFC 1/2] iommu: arm-smmu: Handoff SMR registers and context banks
  2019-06-05 21:08 [RFC 0/2] iommu: arm-smmu: Inherit SMR and CB config during init Bjorn Andersson
@ 2019-06-05 21:08 ` Bjorn Andersson
  2019-06-12 18:07   ` Jeffrey Hugo
  2019-06-13 11:23   ` Robin Murphy
  2019-06-05 21:08 ` [RFC 2/2] iommu: arm-smmu: Don't blindly use first SMR to calculate mask Bjorn Andersson
  1 sibling, 2 replies; 10+ messages in thread
From: Bjorn Andersson @ 2019-06-05 21:08 UTC (permalink / raw)
  To: Joerg Roedel, Will Deacon, Robin Murphy
  Cc: Vivek Gautam, Jeffrey Hugo, Patrick Daly, iommu, linux-kernel,
	linux-arm-kernel, linux-arm-msm

Boot splash screen or EFI framebuffer requires the display hardware to
operate while the Linux iommu driver probes. Therefore, we cannot simply
wipe out the SMR register settings programmed by the bootloader.

Detect which SMR registers are in use during probe, and which context
banks they are associated with. Reserve these context banks for the
first Linux device whose stream-id matches the SMR register.

Any existing page-tables will be discarded.

Heavily based on downstream implementation by Patrick Daly
<pdaly@codeaurora.org>.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/iommu/arm-smmu-regs.h |  2 +
 drivers/iommu/arm-smmu.c      | 80 ++++++++++++++++++++++++++++++++---
 2 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/arm-smmu-regs.h b/drivers/iommu/arm-smmu-regs.h
index e9132a926761..8c1fd84032a2 100644
--- a/drivers/iommu/arm-smmu-regs.h
+++ b/drivers/iommu/arm-smmu-regs.h
@@ -105,7 +105,9 @@
 #define ARM_SMMU_GR0_SMR(n)		(0x800 + ((n) << 2))
 #define SMR_VALID			(1 << 31)
 #define SMR_MASK_SHIFT			16
+#define SMR_MASK_MASK			0x7fff
 #define SMR_ID_SHIFT			0
+#define SMR_ID_MASK			0xffff
 
 #define ARM_SMMU_GR0_S2CR(n)		(0xc00 + ((n) << 2))
 #define S2CR_CBNDX_SHIFT		0
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 5e54cc0a28b3..c8629a656b42 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -135,6 +135,7 @@ struct arm_smmu_s2cr {
 	enum arm_smmu_s2cr_type		type;
 	enum arm_smmu_s2cr_privcfg	privcfg;
 	u8				cbndx;
+	bool				handoff;
 };
 
 #define s2cr_init_val (struct arm_smmu_s2cr){				\
@@ -399,9 +400,22 @@ static int arm_smmu_register_legacy_master(struct device *dev,
 	return err;
 }
 
-static int __arm_smmu_alloc_bitmap(unsigned long *map, int start, int end)
+static int __arm_smmu_alloc_cb(struct arm_smmu_device *smmu, int start,
+			       struct device *dev)
 {
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	unsigned long *map = smmu->context_map;
+	int end = smmu->num_context_banks;
+	int cbndx;
 	int idx;
+	int i;
+
+	for_each_cfg_sme(fwspec, i, idx) {
+		if (smmu->s2crs[idx].handoff) {
+			cbndx = smmu->s2crs[idx].cbndx;
+			goto found_handoff;
+		}
+	}
 
 	do {
 		idx = find_next_zero_bit(map, end, start);
@@ -410,6 +424,17 @@ static int __arm_smmu_alloc_bitmap(unsigned long *map, int start, int end)
 	} while (test_and_set_bit(idx, map));
 
 	return idx;
+
+found_handoff:
+	for (i = 0; i < smmu->num_mapping_groups; i++) {
+		if (smmu->s2crs[i].cbndx == cbndx) {
+			smmu->s2crs[i].cbndx = 0;
+			smmu->s2crs[i].handoff = false;
+			smmu->s2crs[i].count--;
+		}
+	}
+
+	return cbndx;
 }
 
 static void __arm_smmu_free_bitmap(unsigned long *map, int idx)
@@ -759,7 +784,8 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
 }
 
 static int arm_smmu_init_domain_context(struct iommu_domain *domain,
-					struct arm_smmu_device *smmu)
+					struct arm_smmu_device *smmu,
+					struct device *dev)
 {
 	int irq, start, ret = 0;
 	unsigned long ias, oas;
@@ -873,8 +899,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 		ret = -EINVAL;
 		goto out_unlock;
 	}
-	ret = __arm_smmu_alloc_bitmap(smmu->context_map, start,
-				      smmu->num_context_banks);
+	ret = __arm_smmu_alloc_cb(smmu, start, dev);
 	if (ret < 0)
 		goto out_unlock;
 
@@ -1264,7 +1289,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		return ret;
 
 	/* Ensure that the domain is finalised */
-	ret = arm_smmu_init_domain_context(domain, smmu);
+	ret = arm_smmu_init_domain_context(domain, smmu, dev);
 	if (ret < 0)
 		goto rpm_put;
 
@@ -1798,6 +1823,49 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
 }
 
+static void arm_smmu_read_smr_state(struct arm_smmu_device *smmu)
+{
+	u32 privcfg;
+	u32 cbndx;
+	u32 mask;
+	u32 type;
+	u32 s2cr;
+	u32 smr;
+	u32 id;
+	int i;
+
+	for (i = 0; i < smmu->num_mapping_groups; i++) {
+		smr = readl_relaxed(ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_SMR(i));
+		mask = (smr >> SMR_MASK_SHIFT) & SMR_MASK_MASK;
+		id = smr & SMR_ID_MASK;
+		if (!(smr & SMR_VALID))
+			continue;
+
+		s2cr = readl_relaxed(ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_S2CR(i));
+		type = (s2cr >> S2CR_TYPE_SHIFT) & S2CR_TYPE_MASK;
+		cbndx = (s2cr >> S2CR_CBNDX_SHIFT) & S2CR_CBNDX_MASK;
+		privcfg = (s2cr >> S2CR_PRIVCFG_SHIFT) & S2CR_PRIVCFG_MASK;
+		if (type != S2CR_TYPE_TRANS)
+			continue;
+
+		/* Populate the SMR */
+		smmu->smrs[i].mask = mask;
+		smmu->smrs[i].id = id;
+		smmu->smrs[i].valid = true;
+
+		/* Populate the S2CR */
+		smmu->s2crs[i].group = NULL;
+		smmu->s2crs[i].count = 1;
+		smmu->s2crs[i].type = type;
+		smmu->s2crs[i].privcfg = privcfg;
+		smmu->s2crs[i].cbndx = cbndx;
+		smmu->s2crs[i].handoff = true;
+
+		/* Mark the context bank as busy */
+		bitmap_set(smmu->context_map, cbndx, 1);
+	}
+}
+
 static int arm_smmu_id_size_to_bits(int size)
 {
 	switch (size) {
@@ -2023,6 +2091,8 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 		dev_notice(smmu->dev, "\tStage-2: %lu-bit IPA -> %lu-bit PA\n",
 			   smmu->ipa_size, smmu->pa_size);
 
+	arm_smmu_read_smr_state(smmu);
+
 	return 0;
 }
 
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [RFC 2/2] iommu: arm-smmu: Don't blindly use first SMR to calculate mask
  2019-06-05 21:08 [RFC 0/2] iommu: arm-smmu: Inherit SMR and CB config during init Bjorn Andersson
  2019-06-05 21:08 ` [RFC 1/2] iommu: arm-smmu: Handoff SMR registers and context banks Bjorn Andersson
@ 2019-06-05 21:08 ` Bjorn Andersson
  2019-06-12 17:58   ` Jeffrey Hugo
  1 sibling, 1 reply; 10+ messages in thread
From: Bjorn Andersson @ 2019-06-05 21:08 UTC (permalink / raw)
  To: Joerg Roedel, Will Deacon, Robin Murphy
  Cc: Vivek Gautam, Jeffrey Hugo, Patrick Daly, iommu, linux-kernel,
	linux-arm-kernel, linux-arm-msm

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 <bjorn.andersson@linaro.org>
---
 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [RFC 2/2] iommu: arm-smmu: Don't blindly use first SMR to calculate mask
  2019-06-05 21:08 ` [RFC 2/2] iommu: arm-smmu: Don't blindly use first SMR to calculate mask Bjorn Andersson
@ 2019-06-12 17:58   ` Jeffrey Hugo
  2019-06-12 18:28     ` Bjorn Andersson
  0 siblings, 1 reply; 10+ messages in thread
From: Jeffrey Hugo @ 2019-06-12 17:58 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Joerg Roedel, Will Deacon, Robin Murphy, Patrick Daly,
	Jeffrey Hugo, MSM, lkml, iommu, Vivek Gautam, linux-arm-kernel

On Wed, Jun 5, 2019 at 3:09 PM Bjorn Andersson
<bjorn.andersson@linaro.org> 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 <bjorn.andersson@linaro.org>

Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>

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
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC 1/2] iommu: arm-smmu: Handoff SMR registers and context banks
  2019-06-05 21:08 ` [RFC 1/2] iommu: arm-smmu: Handoff SMR registers and context banks Bjorn Andersson
@ 2019-06-12 18:07   ` Jeffrey Hugo
  2019-06-12 18:42     ` Bjorn Andersson
  2019-06-13 11:23   ` Robin Murphy
  1 sibling, 1 reply; 10+ messages in thread
From: Jeffrey Hugo @ 2019-06-12 18:07 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Joerg Roedel, Will Deacon, Robin Murphy, Patrick Daly,
	Jeffrey Hugo, MSM, lkml, iommu, Vivek Gautam, linux-arm-kernel

On Wed, Jun 5, 2019 at 3:09 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> Boot splash screen or EFI framebuffer requires the display hardware to
> operate while the Linux iommu driver probes. Therefore, we cannot simply
> wipe out the SMR register settings programmed by the bootloader.
>
> Detect which SMR registers are in use during probe, and which context
> banks they are associated with. Reserve these context banks for the
> first Linux device whose stream-id matches the SMR register.
>
> Any existing page-tables will be discarded.
>
> Heavily based on downstream implementation by Patrick Daly
> <pdaly@codeaurora.org>.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>

This is very similar to the hacked up version I did, and I'm glad to see it
cleaned up and posted.

This is important work, and I want to see it move forward, however it doesn't
completely address everything, IMO.  Fixing the remaining issues certainly
can be follow on work, but I don't know if they would end up affecting this
implementation.

So, with this series, we've gone from a crash on msm8998/sdm845, to causing
context faults.  This is because while we are now not nuking the config, we
are preventing the bootloader installed translations from working.  Essentially
the display already has a mapping (technically passthrough) that is likely being
used by EFIFB.  The instant the SMMU inits, that mapping becomes invalid,
and the display is going to generate context faults.  While not fatal,
this provides
a bad user experience as there are nasty messages, and EFIFB stops working.

The situation does get resolved once the display driver inits and takes over the
HW and SMMU mappings, so we are not stuck with it, but it would be nice to
have that addressed as well, ie have EFIFB working up until the Linux display
driver takes over.

The only way I can see that happening is if the SMMU leaves the context bank
alone, with M == 0, and the iommu framework defines a domain attribute or
some other mechanism to allow the driver to flip the M bit in the context bank
after installing the necessary handover translations.

> ---
>  drivers/iommu/arm-smmu-regs.h |  2 +
>  drivers/iommu/arm-smmu.c      | 80 ++++++++++++++++++++++++++++++++---
>  2 files changed, 77 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-regs.h b/drivers/iommu/arm-smmu-regs.h
> index e9132a926761..8c1fd84032a2 100644
> --- a/drivers/iommu/arm-smmu-regs.h
> +++ b/drivers/iommu/arm-smmu-regs.h
> @@ -105,7 +105,9 @@
>  #define ARM_SMMU_GR0_SMR(n)            (0x800 + ((n) << 2))
>  #define SMR_VALID                      (1 << 31)
>  #define SMR_MASK_SHIFT                 16
> +#define SMR_MASK_MASK                  0x7fff
>  #define SMR_ID_SHIFT                   0
> +#define SMR_ID_MASK                    0xffff
>
>  #define ARM_SMMU_GR0_S2CR(n)           (0xc00 + ((n) << 2))
>  #define S2CR_CBNDX_SHIFT               0
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 5e54cc0a28b3..c8629a656b42 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -135,6 +135,7 @@ struct arm_smmu_s2cr {
>         enum arm_smmu_s2cr_type         type;
>         enum arm_smmu_s2cr_privcfg      privcfg;
>         u8                              cbndx;
> +       bool                            handoff;
>  };
>
>  #define s2cr_init_val (struct arm_smmu_s2cr){                          \
> @@ -399,9 +400,22 @@ static int arm_smmu_register_legacy_master(struct device *dev,
>         return err;
>  }
>
> -static int __arm_smmu_alloc_bitmap(unsigned long *map, int start, int end)
> +static int __arm_smmu_alloc_cb(struct arm_smmu_device *smmu, int start,
> +                              struct device *dev)
>  {
> +       struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> +       unsigned long *map = smmu->context_map;
> +       int end = smmu->num_context_banks;
> +       int cbndx;
>         int idx;
> +       int i;
> +
> +       for_each_cfg_sme(fwspec, i, idx) {
> +               if (smmu->s2crs[idx].handoff) {
> +                       cbndx = smmu->s2crs[idx].cbndx;
> +                       goto found_handoff;
> +               }
> +       }
>
>         do {
>                 idx = find_next_zero_bit(map, end, start);
> @@ -410,6 +424,17 @@ static int __arm_smmu_alloc_bitmap(unsigned long *map, int start, int end)
>         } while (test_and_set_bit(idx, map));
>
>         return idx;
> +
> +found_handoff:
> +       for (i = 0; i < smmu->num_mapping_groups; i++) {
> +               if (smmu->s2crs[i].cbndx == cbndx) {
> +                       smmu->s2crs[i].cbndx = 0;
> +                       smmu->s2crs[i].handoff = false;
> +                       smmu->s2crs[i].count--;
> +               }
> +       }
> +
> +       return cbndx;
>  }
>
>  static void __arm_smmu_free_bitmap(unsigned long *map, int idx)
> @@ -759,7 +784,8 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
>  }
>
>  static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> -                                       struct arm_smmu_device *smmu)
> +                                       struct arm_smmu_device *smmu,
> +                                       struct device *dev)
>  {
>         int irq, start, ret = 0;
>         unsigned long ias, oas;
> @@ -873,8 +899,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>                 ret = -EINVAL;
>                 goto out_unlock;
>         }
> -       ret = __arm_smmu_alloc_bitmap(smmu->context_map, start,
> -                                     smmu->num_context_banks);
> +       ret = __arm_smmu_alloc_cb(smmu, start, dev);
>         if (ret < 0)
>                 goto out_unlock;
>
> @@ -1264,7 +1289,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>                 return ret;
>
>         /* Ensure that the domain is finalised */
> -       ret = arm_smmu_init_domain_context(domain, smmu);
> +       ret = arm_smmu_init_domain_context(domain, smmu, dev);
>         if (ret < 0)
>                 goto rpm_put;
>
> @@ -1798,6 +1823,49 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>         writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
>  }
>
> +static void arm_smmu_read_smr_state(struct arm_smmu_device *smmu)
> +{
> +       u32 privcfg;
> +       u32 cbndx;
> +       u32 mask;
> +       u32 type;
> +       u32 s2cr;
> +       u32 smr;
> +       u32 id;
> +       int i;
> +
> +       for (i = 0; i < smmu->num_mapping_groups; i++) {
> +               smr = readl_relaxed(ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_SMR(i));
> +               mask = (smr >> SMR_MASK_SHIFT) & SMR_MASK_MASK;
> +               id = smr & SMR_ID_MASK;
> +               if (!(smr & SMR_VALID))
> +                       continue;
> +
> +               s2cr = readl_relaxed(ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_S2CR(i));
> +               type = (s2cr >> S2CR_TYPE_SHIFT) & S2CR_TYPE_MASK;
> +               cbndx = (s2cr >> S2CR_CBNDX_SHIFT) & S2CR_CBNDX_MASK;
> +               privcfg = (s2cr >> S2CR_PRIVCFG_SHIFT) & S2CR_PRIVCFG_MASK;
> +               if (type != S2CR_TYPE_TRANS)
> +                       continue;
> +
> +               /* Populate the SMR */
> +               smmu->smrs[i].mask = mask;
> +               smmu->smrs[i].id = id;
> +               smmu->smrs[i].valid = true;
> +
> +               /* Populate the S2CR */
> +               smmu->s2crs[i].group = NULL;
> +               smmu->s2crs[i].count = 1;
> +               smmu->s2crs[i].type = type;
> +               smmu->s2crs[i].privcfg = privcfg;
> +               smmu->s2crs[i].cbndx = cbndx;
> +               smmu->s2crs[i].handoff = true;
> +
> +               /* Mark the context bank as busy */
> +               bitmap_set(smmu->context_map, cbndx, 1);
> +       }
> +}
> +
>  static int arm_smmu_id_size_to_bits(int size)
>  {
>         switch (size) {
> @@ -2023,6 +2091,8 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>                 dev_notice(smmu->dev, "\tStage-2: %lu-bit IPA -> %lu-bit PA\n",
>                            smmu->ipa_size, smmu->pa_size);
>
> +       arm_smmu_read_smr_state(smmu);
> +
>         return 0;
>  }
>
> --
> 2.18.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC 2/2] iommu: arm-smmu: Don't blindly use first SMR to calculate mask
  2019-06-12 17:58   ` Jeffrey Hugo
@ 2019-06-12 18:28     ` Bjorn Andersson
  0 siblings, 0 replies; 10+ messages in thread
From: Bjorn Andersson @ 2019-06-12 18:28 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: Joerg Roedel, Will Deacon, Robin Murphy, Patrick Daly,
	Jeffrey Hugo, MSM, lkml, iommu, Vivek Gautam, linux-arm-kernel

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

> On Wed, Jun 5, 2019 at 3:09 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> 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 <bjorn.andersson@linaro.org>
> 
> Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
> 
> 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
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC 1/2] iommu: arm-smmu: Handoff SMR registers and context banks
  2019-06-12 18:07   ` Jeffrey Hugo
@ 2019-06-12 18:42     ` Bjorn Andersson
  2019-06-12 19:16       ` Jeffrey Hugo
  0 siblings, 1 reply; 10+ messages in thread
From: Bjorn Andersson @ 2019-06-12 18:42 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: Joerg Roedel, Will Deacon, Robin Murphy, Patrick Daly,
	Jeffrey Hugo, MSM, lkml, iommu, Vivek Gautam, linux-arm-kernel

On Wed 12 Jun 11:07 PDT 2019, Jeffrey Hugo wrote:

> On Wed, Jun 5, 2019 at 3:09 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > Boot splash screen or EFI framebuffer requires the display hardware to
> > operate while the Linux iommu driver probes. Therefore, we cannot simply
> > wipe out the SMR register settings programmed by the bootloader.
> >
> > Detect which SMR registers are in use during probe, and which context
> > banks they are associated with. Reserve these context banks for the
> > first Linux device whose stream-id matches the SMR register.
> >
> > Any existing page-tables will be discarded.
> >
> > Heavily based on downstream implementation by Patrick Daly
> > <pdaly@codeaurora.org>.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
> 
> This is very similar to the hacked up version I did, and I'm glad to see it
> cleaned up and posted.
> 
> This is important work, and I want to see it move forward, however it doesn't
> completely address everything, IMO.  Fixing the remaining issues certainly
> can be follow on work, but I don't know if they would end up affecting this
> implementation.
> 
> So, with this series, we've gone from a crash on msm8998/sdm845, to causing
> context faults.  This is because while we are now not nuking the config, we
> are preventing the bootloader installed translations from working.  Essentially
> the display already has a mapping (technically passthrough) that is likely being
> used by EFIFB.  The instant the SMMU inits, that mapping becomes invalid,
> and the display is going to generate context faults.  While not fatal,
> this provides
> a bad user experience as there are nasty messages, and EFIFB stops working.
> 
> The situation does get resolved once the display driver inits and takes over the
> HW and SMMU mappings, so we are not stuck with it, but it would be nice to
> have that addressed as well, ie have EFIFB working up until the Linux display
> driver takes over.
> 

But do you see this even though you don't enable the mdss driver?

> The only way I can see that happening is if the SMMU leaves the context bank
> alone, with M == 0, and the iommu framework defines a domain attribute or
> some other mechanism to allow the driver to flip the M bit in the context bank
> after installing the necessary handover translations.
> 

From what I can tell this implementation leaves the framebuffer mapping
in working condition until the attach_dev of the display driver, at
which time we do get context faults until the display driver is done
initializing things.

So we're reducing the problem to a question of how to seamlessly carry
over the mapping during the attach.

Regards,
Bjorn

> > ---
> >  drivers/iommu/arm-smmu-regs.h |  2 +
> >  drivers/iommu/arm-smmu.c      | 80 ++++++++++++++++++++++++++++++++---
> >  2 files changed, 77 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu-regs.h b/drivers/iommu/arm-smmu-regs.h
> > index e9132a926761..8c1fd84032a2 100644
> > --- a/drivers/iommu/arm-smmu-regs.h
> > +++ b/drivers/iommu/arm-smmu-regs.h
> > @@ -105,7 +105,9 @@
> >  #define ARM_SMMU_GR0_SMR(n)            (0x800 + ((n) << 2))
> >  #define SMR_VALID                      (1 << 31)
> >  #define SMR_MASK_SHIFT                 16
> > +#define SMR_MASK_MASK                  0x7fff
> >  #define SMR_ID_SHIFT                   0
> > +#define SMR_ID_MASK                    0xffff
> >
> >  #define ARM_SMMU_GR0_S2CR(n)           (0xc00 + ((n) << 2))
> >  #define S2CR_CBNDX_SHIFT               0
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 5e54cc0a28b3..c8629a656b42 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -135,6 +135,7 @@ struct arm_smmu_s2cr {
> >         enum arm_smmu_s2cr_type         type;
> >         enum arm_smmu_s2cr_privcfg      privcfg;
> >         u8                              cbndx;
> > +       bool                            handoff;
> >  };
> >
> >  #define s2cr_init_val (struct arm_smmu_s2cr){                          \
> > @@ -399,9 +400,22 @@ static int arm_smmu_register_legacy_master(struct device *dev,
> >         return err;
> >  }
> >
> > -static int __arm_smmu_alloc_bitmap(unsigned long *map, int start, int end)
> > +static int __arm_smmu_alloc_cb(struct arm_smmu_device *smmu, int start,
> > +                              struct device *dev)
> >  {
> > +       struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > +       unsigned long *map = smmu->context_map;
> > +       int end = smmu->num_context_banks;
> > +       int cbndx;
> >         int idx;
> > +       int i;
> > +
> > +       for_each_cfg_sme(fwspec, i, idx) {
> > +               if (smmu->s2crs[idx].handoff) {
> > +                       cbndx = smmu->s2crs[idx].cbndx;
> > +                       goto found_handoff;
> > +               }
> > +       }
> >
> >         do {
> >                 idx = find_next_zero_bit(map, end, start);
> > @@ -410,6 +424,17 @@ static int __arm_smmu_alloc_bitmap(unsigned long *map, int start, int end)
> >         } while (test_and_set_bit(idx, map));
> >
> >         return idx;
> > +
> > +found_handoff:
> > +       for (i = 0; i < smmu->num_mapping_groups; i++) {
> > +               if (smmu->s2crs[i].cbndx == cbndx) {
> > +                       smmu->s2crs[i].cbndx = 0;
> > +                       smmu->s2crs[i].handoff = false;
> > +                       smmu->s2crs[i].count--;
> > +               }
> > +       }
> > +
> > +       return cbndx;
> >  }
> >
> >  static void __arm_smmu_free_bitmap(unsigned long *map, int idx)
> > @@ -759,7 +784,8 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
> >  }
> >
> >  static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> > -                                       struct arm_smmu_device *smmu)
> > +                                       struct arm_smmu_device *smmu,
> > +                                       struct device *dev)
> >  {
> >         int irq, start, ret = 0;
> >         unsigned long ias, oas;
> > @@ -873,8 +899,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> >                 ret = -EINVAL;
> >                 goto out_unlock;
> >         }
> > -       ret = __arm_smmu_alloc_bitmap(smmu->context_map, start,
> > -                                     smmu->num_context_banks);
> > +       ret = __arm_smmu_alloc_cb(smmu, start, dev);
> >         if (ret < 0)
> >                 goto out_unlock;
> >
> > @@ -1264,7 +1289,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> >                 return ret;
> >
> >         /* Ensure that the domain is finalised */
> > -       ret = arm_smmu_init_domain_context(domain, smmu);
> > +       ret = arm_smmu_init_domain_context(domain, smmu, dev);
> >         if (ret < 0)
> >                 goto rpm_put;
> >
> > @@ -1798,6 +1823,49 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> >         writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> >  }
> >
> > +static void arm_smmu_read_smr_state(struct arm_smmu_device *smmu)
> > +{
> > +       u32 privcfg;
> > +       u32 cbndx;
> > +       u32 mask;
> > +       u32 type;
> > +       u32 s2cr;
> > +       u32 smr;
> > +       u32 id;
> > +       int i;
> > +
> > +       for (i = 0; i < smmu->num_mapping_groups; i++) {
> > +               smr = readl_relaxed(ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_SMR(i));
> > +               mask = (smr >> SMR_MASK_SHIFT) & SMR_MASK_MASK;
> > +               id = smr & SMR_ID_MASK;
> > +               if (!(smr & SMR_VALID))
> > +                       continue;
> > +
> > +               s2cr = readl_relaxed(ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_S2CR(i));
> > +               type = (s2cr >> S2CR_TYPE_SHIFT) & S2CR_TYPE_MASK;
> > +               cbndx = (s2cr >> S2CR_CBNDX_SHIFT) & S2CR_CBNDX_MASK;
> > +               privcfg = (s2cr >> S2CR_PRIVCFG_SHIFT) & S2CR_PRIVCFG_MASK;
> > +               if (type != S2CR_TYPE_TRANS)
> > +                       continue;
> > +
> > +               /* Populate the SMR */
> > +               smmu->smrs[i].mask = mask;
> > +               smmu->smrs[i].id = id;
> > +               smmu->smrs[i].valid = true;
> > +
> > +               /* Populate the S2CR */
> > +               smmu->s2crs[i].group = NULL;
> > +               smmu->s2crs[i].count = 1;
> > +               smmu->s2crs[i].type = type;
> > +               smmu->s2crs[i].privcfg = privcfg;
> > +               smmu->s2crs[i].cbndx = cbndx;
> > +               smmu->s2crs[i].handoff = true;
> > +
> > +               /* Mark the context bank as busy */
> > +               bitmap_set(smmu->context_map, cbndx, 1);
> > +       }
> > +}
> > +
> >  static int arm_smmu_id_size_to_bits(int size)
> >  {
> >         switch (size) {
> > @@ -2023,6 +2091,8 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
> >                 dev_notice(smmu->dev, "\tStage-2: %lu-bit IPA -> %lu-bit PA\n",
> >                            smmu->ipa_size, smmu->pa_size);
> >
> > +       arm_smmu_read_smr_state(smmu);
> > +
> >         return 0;
> >  }
> >
> > --
> > 2.18.0
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC 1/2] iommu: arm-smmu: Handoff SMR registers and context banks
  2019-06-12 18:42     ` Bjorn Andersson
@ 2019-06-12 19:16       ` Jeffrey Hugo
  0 siblings, 0 replies; 10+ messages in thread
From: Jeffrey Hugo @ 2019-06-12 19:16 UTC (permalink / raw)
  To: Bjorn Andersson, Jeffrey Hugo
  Cc: Patrick Daly, MSM, Joerg Roedel, Will Deacon, lkml, iommu,
	Vivek Gautam, Robin Murphy, linux-arm-kernel

On 6/12/2019 12:42 PM, Bjorn Andersson wrote:
> On Wed 12 Jun 11:07 PDT 2019, Jeffrey Hugo wrote:
> 
>> On Wed, Jun 5, 2019 at 3:09 PM Bjorn Andersson
>> <bjorn.andersson@linaro.org> wrote:
>>>
>>> Boot splash screen or EFI framebuffer requires the display hardware to
>>> operate while the Linux iommu driver probes. Therefore, we cannot simply
>>> wipe out the SMR register settings programmed by the bootloader.
>>>
>>> Detect which SMR registers are in use during probe, and which context
>>> banks they are associated with. Reserve these context banks for the
>>> first Linux device whose stream-id matches the SMR register.
>>>
>>> Any existing page-tables will be discarded.
>>>
>>> Heavily based on downstream implementation by Patrick Daly
>>> <pdaly@codeaurora.org>.
>>>
>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>
>> Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
>>
>> This is very similar to the hacked up version I did, and I'm glad to see it
>> cleaned up and posted.
>>
>> This is important work, and I want to see it move forward, however it doesn't
>> completely address everything, IMO.  Fixing the remaining issues certainly
>> can be follow on work, but I don't know if they would end up affecting this
>> implementation.
>>
>> So, with this series, we've gone from a crash on msm8998/sdm845, to causing
>> context faults.  This is because while we are now not nuking the config, we
>> are preventing the bootloader installed translations from working.  Essentially
>> the display already has a mapping (technically passthrough) that is likely being
>> used by EFIFB.  The instant the SMMU inits, that mapping becomes invalid,
>> and the display is going to generate context faults.  While not fatal,
>> this provides
>> a bad user experience as there are nasty messages, and EFIFB stops working.
>>
>> The situation does get resolved once the display driver inits and takes over the
>> HW and SMMU mappings, so we are not stuck with it, but it would be nice to
>> have that addressed as well, ie have EFIFB working up until the Linux display
>> driver takes over.
>>
> 
> But do you see this even though you don't enable the mdss driver?
> 
>> The only way I can see that happening is if the SMMU leaves the context bank
>> alone, with M == 0, and the iommu framework defines a domain attribute or
>> some other mechanism to allow the driver to flip the M bit in the context bank
>> after installing the necessary handover translations.
>>
> 
>  From what I can tell this implementation leaves the framebuffer mapping
> in working condition until the attach_dev of the display driver, at
> which time we do get context faults until the display driver is done
> initializing things.
> 
> So we're reducing the problem to a question of how to seamlessly carry
> over the mapping during the attach.

Actually, you are correct.  Without mdss this won't occur.  The window 
is from the creation of the default domain for the mdss device, to the 
point that the display is fully init'd (and EFIFB is shut down).

> 
> Regards,
> Bjorn
> 
>>> ---
>>>   drivers/iommu/arm-smmu-regs.h |  2 +
>>>   drivers/iommu/arm-smmu.c      | 80 ++++++++++++++++++++++++++++++++---
>>>   2 files changed, 77 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iommu/arm-smmu-regs.h b/drivers/iommu/arm-smmu-regs.h
>>> index e9132a926761..8c1fd84032a2 100644
>>> --- a/drivers/iommu/arm-smmu-regs.h
>>> +++ b/drivers/iommu/arm-smmu-regs.h
>>> @@ -105,7 +105,9 @@
>>>   #define ARM_SMMU_GR0_SMR(n)            (0x800 + ((n) << 2))
>>>   #define SMR_VALID                      (1 << 31)
>>>   #define SMR_MASK_SHIFT                 16
>>> +#define SMR_MASK_MASK                  0x7fff
>>>   #define SMR_ID_SHIFT                   0
>>> +#define SMR_ID_MASK                    0xffff
>>>
>>>   #define ARM_SMMU_GR0_S2CR(n)           (0xc00 + ((n) << 2))
>>>   #define S2CR_CBNDX_SHIFT               0
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index 5e54cc0a28b3..c8629a656b42 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -135,6 +135,7 @@ struct arm_smmu_s2cr {
>>>          enum arm_smmu_s2cr_type         type;
>>>          enum arm_smmu_s2cr_privcfg      privcfg;
>>>          u8                              cbndx;
>>> +       bool                            handoff;
>>>   };
>>>
>>>   #define s2cr_init_val (struct arm_smmu_s2cr){                          \
>>> @@ -399,9 +400,22 @@ static int arm_smmu_register_legacy_master(struct device *dev,
>>>          return err;
>>>   }
>>>
>>> -static int __arm_smmu_alloc_bitmap(unsigned long *map, int start, int end)
>>> +static int __arm_smmu_alloc_cb(struct arm_smmu_device *smmu, int start,
>>> +                              struct device *dev)
>>>   {
>>> +       struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>>> +       unsigned long *map = smmu->context_map;
>>> +       int end = smmu->num_context_banks;
>>> +       int cbndx;
>>>          int idx;
>>> +       int i;
>>> +
>>> +       for_each_cfg_sme(fwspec, i, idx) {
>>> +               if (smmu->s2crs[idx].handoff) {
>>> +                       cbndx = smmu->s2crs[idx].cbndx;
>>> +                       goto found_handoff;
>>> +               }
>>> +       }
>>>
>>>          do {
>>>                  idx = find_next_zero_bit(map, end, start);
>>> @@ -410,6 +424,17 @@ static int __arm_smmu_alloc_bitmap(unsigned long *map, int start, int end)
>>>          } while (test_and_set_bit(idx, map));
>>>
>>>          return idx;
>>> +
>>> +found_handoff:
>>> +       for (i = 0; i < smmu->num_mapping_groups; i++) {
>>> +               if (smmu->s2crs[i].cbndx == cbndx) {
>>> +                       smmu->s2crs[i].cbndx = 0;
>>> +                       smmu->s2crs[i].handoff = false;
>>> +                       smmu->s2crs[i].count--;
>>> +               }
>>> +       }
>>> +
>>> +       return cbndx;
>>>   }
>>>
>>>   static void __arm_smmu_free_bitmap(unsigned long *map, int idx)
>>> @@ -759,7 +784,8 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
>>>   }
>>>
>>>   static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>>> -                                       struct arm_smmu_device *smmu)
>>> +                                       struct arm_smmu_device *smmu,
>>> +                                       struct device *dev)
>>>   {
>>>          int irq, start, ret = 0;
>>>          unsigned long ias, oas;
>>> @@ -873,8 +899,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>>>                  ret = -EINVAL;
>>>                  goto out_unlock;
>>>          }
>>> -       ret = __arm_smmu_alloc_bitmap(smmu->context_map, start,
>>> -                                     smmu->num_context_banks);
>>> +       ret = __arm_smmu_alloc_cb(smmu, start, dev);
>>>          if (ret < 0)
>>>                  goto out_unlock;
>>>
>>> @@ -1264,7 +1289,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>>>                  return ret;
>>>
>>>          /* Ensure that the domain is finalised */
>>> -       ret = arm_smmu_init_domain_context(domain, smmu);
>>> +       ret = arm_smmu_init_domain_context(domain, smmu, dev);
>>>          if (ret < 0)
>>>                  goto rpm_put;
>>>
>>> @@ -1798,6 +1823,49 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>>>          writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
>>>   }
>>>
>>> +static void arm_smmu_read_smr_state(struct arm_smmu_device *smmu)
>>> +{
>>> +       u32 privcfg;
>>> +       u32 cbndx;
>>> +       u32 mask;
>>> +       u32 type;
>>> +       u32 s2cr;
>>> +       u32 smr;
>>> +       u32 id;
>>> +       int i;
>>> +
>>> +       for (i = 0; i < smmu->num_mapping_groups; i++) {
>>> +               smr = readl_relaxed(ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_SMR(i));
>>> +               mask = (smr >> SMR_MASK_SHIFT) & SMR_MASK_MASK;
>>> +               id = smr & SMR_ID_MASK;
>>> +               if (!(smr & SMR_VALID))
>>> +                       continue;
>>> +
>>> +               s2cr = readl_relaxed(ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_S2CR(i));
>>> +               type = (s2cr >> S2CR_TYPE_SHIFT) & S2CR_TYPE_MASK;
>>> +               cbndx = (s2cr >> S2CR_CBNDX_SHIFT) & S2CR_CBNDX_MASK;
>>> +               privcfg = (s2cr >> S2CR_PRIVCFG_SHIFT) & S2CR_PRIVCFG_MASK;
>>> +               if (type != S2CR_TYPE_TRANS)
>>> +                       continue;
>>> +
>>> +               /* Populate the SMR */
>>> +               smmu->smrs[i].mask = mask;
>>> +               smmu->smrs[i].id = id;
>>> +               smmu->smrs[i].valid = true;
>>> +
>>> +               /* Populate the S2CR */
>>> +               smmu->s2crs[i].group = NULL;
>>> +               smmu->s2crs[i].count = 1;
>>> +               smmu->s2crs[i].type = type;
>>> +               smmu->s2crs[i].privcfg = privcfg;
>>> +               smmu->s2crs[i].cbndx = cbndx;
>>> +               smmu->s2crs[i].handoff = true;
>>> +
>>> +               /* Mark the context bank as busy */
>>> +               bitmap_set(smmu->context_map, cbndx, 1);
>>> +       }
>>> +}
>>> +
>>>   static int arm_smmu_id_size_to_bits(int size)
>>>   {
>>>          switch (size) {
>>> @@ -2023,6 +2091,8 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>>>                  dev_notice(smmu->dev, "\tStage-2: %lu-bit IPA -> %lu-bit PA\n",
>>>                             smmu->ipa_size, smmu->pa_size);
>>>
>>> +       arm_smmu_read_smr_state(smmu);
>>> +
>>>          return 0;
>>>   }
>>>
>>> --
>>> 2.18.0
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC 1/2] iommu: arm-smmu: Handoff SMR registers and context banks
  2019-06-05 21:08 ` [RFC 1/2] iommu: arm-smmu: Handoff SMR registers and context banks Bjorn Andersson
  2019-06-12 18:07   ` Jeffrey Hugo
@ 2019-06-13 11:23   ` Robin Murphy
  2019-06-13 22:49     ` Bjorn Andersson
  1 sibling, 1 reply; 10+ messages in thread
From: Robin Murphy @ 2019-06-13 11:23 UTC (permalink / raw)
  To: Bjorn Andersson, Joerg Roedel, Will Deacon
  Cc: Vivek Gautam, Jeffrey Hugo, Patrick Daly, iommu, linux-kernel,
	linux-arm-kernel, linux-arm-msm

On 05/06/2019 22:08, Bjorn Andersson wrote:
> Boot splash screen or EFI framebuffer requires the display hardware to
> operate while the Linux iommu driver probes. Therefore, we cannot simply
> wipe out the SMR register settings programmed by the bootloader.
> 
> Detect which SMR registers are in use during probe, and which context
> banks they are associated with. Reserve these context banks for the
> first Linux device whose stream-id matches the SMR register.
> 
> Any existing page-tables will be discarded.

That doesn't seem particularly useful :/

Either way, if firmware did set up a translation context, is there any 
guarantee that its pagetables haven't already been stomped on by Linux 
(e.g. via memtest)?

> Heavily based on downstream implementation by Patrick Daly
> <pdaly@codeaurora.org>.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>   drivers/iommu/arm-smmu-regs.h |  2 +
>   drivers/iommu/arm-smmu.c      | 80 ++++++++++++++++++++++++++++++++---
>   2 files changed, 77 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-regs.h b/drivers/iommu/arm-smmu-regs.h
> index e9132a926761..8c1fd84032a2 100644
> --- a/drivers/iommu/arm-smmu-regs.h
> +++ b/drivers/iommu/arm-smmu-regs.h
> @@ -105,7 +105,9 @@
>   #define ARM_SMMU_GR0_SMR(n)		(0x800 + ((n) << 2))
>   #define SMR_VALID			(1 << 31)
>   #define SMR_MASK_SHIFT			16
> +#define SMR_MASK_MASK			0x7fff
>   #define SMR_ID_SHIFT			0
> +#define SMR_ID_MASK			0xffff

The SMR ID and MASK fields are either both 15 bits or both 16 bits, 
depending on EXIDS. This mix-and-match is plain wrong either way.

>   #define ARM_SMMU_GR0_S2CR(n)		(0xc00 + ((n) << 2))
>   #define S2CR_CBNDX_SHIFT		0
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 5e54cc0a28b3..c8629a656b42 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -135,6 +135,7 @@ struct arm_smmu_s2cr {
>   	enum arm_smmu_s2cr_type		type;
>   	enum arm_smmu_s2cr_privcfg	privcfg;
>   	u8				cbndx;
> +	bool				handoff;
>   };
>   
>   #define s2cr_init_val (struct arm_smmu_s2cr){				\
> @@ -399,9 +400,22 @@ static int arm_smmu_register_legacy_master(struct device *dev,
>   	return err;
>   }
>   
> -static int __arm_smmu_alloc_bitmap(unsigned long *map, int start, int end)
> +static int __arm_smmu_alloc_cb(struct arm_smmu_device *smmu, int start,
> +			       struct device *dev)
>   {
> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> +	unsigned long *map = smmu->context_map;
> +	int end = smmu->num_context_banks;
> +	int cbndx;
>   	int idx;
> +	int i;
> +
> +	for_each_cfg_sme(fwspec, i, idx) {
> +		if (smmu->s2crs[idx].handoff) {
> +			cbndx = smmu->s2crs[idx].cbndx;
> +			goto found_handoff;
> +		}
> +	}
>   
>   	do {
>   		idx = find_next_zero_bit(map, end, start);
> @@ -410,6 +424,17 @@ static int __arm_smmu_alloc_bitmap(unsigned long *map, int start, int end)
>   	} while (test_and_set_bit(idx, map));
>   
>   	return idx;
> +
> +found_handoff:
> +	for (i = 0; i < smmu->num_mapping_groups; i++) {
> +		if (smmu->s2crs[i].cbndx == cbndx) {
> +			smmu->s2crs[i].cbndx = 0;
> +			smmu->s2crs[i].handoff = false;
> +			smmu->s2crs[i].count--;
> +		}
> +	}
> +
> +	return cbndx;
>   }
>   
>   static void __arm_smmu_free_bitmap(unsigned long *map, int idx)
> @@ -759,7 +784,8 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
>   }
>   
>   static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> -					struct arm_smmu_device *smmu)
> +					struct arm_smmu_device *smmu,
> +					struct device *dev)
>   {
>   	int irq, start, ret = 0;
>   	unsigned long ias, oas;
> @@ -873,8 +899,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>   		ret = -EINVAL;
>   		goto out_unlock;
>   	}
> -	ret = __arm_smmu_alloc_bitmap(smmu->context_map, start,
> -				      smmu->num_context_banks);
> +	ret = __arm_smmu_alloc_cb(smmu, start, dev);
>   	if (ret < 0)
>   		goto out_unlock;
>   
> @@ -1264,7 +1289,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>   		return ret;
>   
>   	/* Ensure that the domain is finalised */
> -	ret = arm_smmu_init_domain_context(domain, smmu);
> +	ret = arm_smmu_init_domain_context(domain, smmu, dev);
>   	if (ret < 0)
>   		goto rpm_put;
>   
> @@ -1798,6 +1823,49 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>   	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
>   }
>   
> +static void arm_smmu_read_smr_state(struct arm_smmu_device *smmu)
> +{
> +	u32 privcfg;
> +	u32 cbndx;
> +	u32 mask;
> +	u32 type;
> +	u32 s2cr;
> +	u32 smr;
> +	u32 id;
> +	int i;
> +
> +	for (i = 0; i < smmu->num_mapping_groups; i++) {
> +		smr = readl_relaxed(ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_SMR(i));

What about stream indexing?

> +		mask = (smr >> SMR_MASK_SHIFT) & SMR_MASK_MASK;
> +		id = smr & SMR_ID_MASK;
> +		if (!(smr & SMR_VALID))

EXIDs again...

> +			continue;
> +
> +		s2cr = readl_relaxed(ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_S2CR(i));
> +		type = (s2cr >> S2CR_TYPE_SHIFT) & S2CR_TYPE_MASK;
> +		cbndx = (s2cr >> S2CR_CBNDX_SHIFT) & S2CR_CBNDX_MASK;
> +		privcfg = (s2cr >> S2CR_PRIVCFG_SHIFT) & S2CR_PRIVCFG_MASK;
> +		if (type != S2CR_TYPE_TRANS)
> +			continue;

How can you tell whether an SMR or S2CR has been programmed by the 
firmware, or that its UNKNOWN reset value is junk which just happens to 
look plausible?

> +
> +		/* Populate the SMR */
> +		smmu->smrs[i].mask = mask;
> +		smmu->smrs[i].id = id;
> +		smmu->smrs[i].valid = true;
> +
> +		/* Populate the S2CR */
> +		smmu->s2crs[i].group = NULL;
> +		smmu->s2crs[i].count = 1;
> +		smmu->s2crs[i].type = type;
> +		smmu->s2crs[i].privcfg = privcfg;
> +		smmu->s2crs[i].cbndx = cbndx;
> +		smmu->s2crs[i].handoff = true;
> +
> +		/* Mark the context bank as busy */
> +		bitmap_set(smmu->context_map, cbndx, 1);

Does anything prevent the SMMU being suspended between here and whenever 
the relevant drivers(s) show up to properly establish the device links?

> +	}
> +}
> +
>   static int arm_smmu_id_size_to_bits(int size)
>   {
>   	switch (size) {
> @@ -2023,6 +2091,8 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>   		dev_notice(smmu->dev, "\tStage-2: %lu-bit IPA -> %lu-bit PA\n",
>   			   smmu->ipa_size, smmu->pa_size);
>   
> +	arm_smmu_read_smr_state(smmu);
> +
>   	return 0;
>   }

Stepping back from the implementation details, I also have concerns that 
this will interact badly with kexec/kdump, but mostly the fact that it's 
at best a partial workaround, rather than a real solution to the 
fundamental problem that initialising an IOMMU can break 
EFIFB/bootsplash/etc., *regardless* of whether the firmware is even 
aware of said IOMMU at all - I've already been living with this on my 
Juno board for months since EDK2 gained Arm HDLCD support. AFAICS this 
can only be solved by some sort of RMRR-like mechanism, i.e. firmware 
providing explicit information about what address ranges are currently 
in use by which devices. I've been pondering what a DT-based 
implementation might look like for a while now, and I guess it's 
probably time to raise it with the IORT spec owners as well.

That said, once we've perfected our WIP design for keeping invasive 
implementation details out of the way of each other and the core 
architectural driver flow, I wouldn't rule out the possibility of 
slotting something like this in as a qcom-specific feature if the "we 
can't change the firmware" argument is going to come up.

Robin.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC 1/2] iommu: arm-smmu: Handoff SMR registers and context banks
  2019-06-13 11:23   ` Robin Murphy
@ 2019-06-13 22:49     ` Bjorn Andersson
  0 siblings, 0 replies; 10+ messages in thread
From: Bjorn Andersson @ 2019-06-13 22:49 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Joerg Roedel, Will Deacon, Vivek Gautam, Jeffrey Hugo,
	Patrick Daly, iommu, linux-kernel, linux-arm-kernel,
	linux-arm-msm

On Thu 13 Jun 04:23 PDT 2019, Robin Murphy wrote:

> On 05/06/2019 22:08, Bjorn Andersson wrote:
> > Boot splash screen or EFI framebuffer requires the display hardware to
> > operate while the Linux iommu driver probes. Therefore, we cannot simply
> > wipe out the SMR register settings programmed by the bootloader.
> > 
> > Detect which SMR registers are in use during probe, and which context
> > banks they are associated with. Reserve these context banks for the
> > first Linux device whose stream-id matches the SMR register.
> > 
> > Any existing page-tables will be discarded.
> 
> That doesn't seem particularly useful :/
> 

In my case all valid CBs have TTBR0 and TTBR1 as 0, so I don't have a
page table. So all I need is a matching stream to keep the display
hardware satisfied.

Is this perhaps the thing that's mentioned as "S2CR bypass" mode?

> Either way, if firmware did set up a translation context, is there any
> guarantee that its pagetables haven't already been stomped on by Linux (e.g.
> via memtest)?
> 

The only way I can think of is if the bootloader would allocate these in
a chunk of memory which it removes from the memory ranges communicated
to the kernel.

In order to support this I believe we would need some logic that
remap the existing page tables and clone them into new ones, and then
update the CBs to match the existing configuration.

But as I said, this is beyond our current needs.

> > Heavily based on downstream implementation by Patrick Daly
> > <pdaly@codeaurora.org>.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >   drivers/iommu/arm-smmu-regs.h |  2 +
> >   drivers/iommu/arm-smmu.c      | 80 ++++++++++++++++++++++++++++++++---
> >   2 files changed, 77 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm-smmu-regs.h b/drivers/iommu/arm-smmu-regs.h
> > index e9132a926761..8c1fd84032a2 100644
> > --- a/drivers/iommu/arm-smmu-regs.h
> > +++ b/drivers/iommu/arm-smmu-regs.h
> > @@ -105,7 +105,9 @@
> >   #define ARM_SMMU_GR0_SMR(n)		(0x800 + ((n) << 2))
> >   #define SMR_VALID			(1 << 31)
> >   #define SMR_MASK_SHIFT			16
> > +#define SMR_MASK_MASK			0x7fff
> >   #define SMR_ID_SHIFT			0
> > +#define SMR_ID_MASK			0xffff
> 
> The SMR ID and MASK fields are either both 15 bits or both 16 bits,
> depending on EXIDS. This mix-and-match is plain wrong either way.
> 

Okay, I will read up some more on how these pieces works.

> >   #define ARM_SMMU_GR0_S2CR(n)		(0xc00 + ((n) << 2))
> >   #define S2CR_CBNDX_SHIFT		0
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 5e54cc0a28b3..c8629a656b42 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -135,6 +135,7 @@ struct arm_smmu_s2cr {
> >   	enum arm_smmu_s2cr_type		type;
> >   	enum arm_smmu_s2cr_privcfg	privcfg;
> >   	u8				cbndx;
> > +	bool				handoff;
> >   };
> >   #define s2cr_init_val (struct arm_smmu_s2cr){				\
> > @@ -399,9 +400,22 @@ static int arm_smmu_register_legacy_master(struct device *dev,
> >   	return err;
> >   }
> > -static int __arm_smmu_alloc_bitmap(unsigned long *map, int start, int end)
> > +static int __arm_smmu_alloc_cb(struct arm_smmu_device *smmu, int start,
> > +			       struct device *dev)
> >   {
> > +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > +	unsigned long *map = smmu->context_map;
> > +	int end = smmu->num_context_banks;
> > +	int cbndx;
> >   	int idx;
> > +	int i;
> > +
> > +	for_each_cfg_sme(fwspec, i, idx) {
> > +		if (smmu->s2crs[idx].handoff) {
> > +			cbndx = smmu->s2crs[idx].cbndx;
> > +			goto found_handoff;
> > +		}
> > +	}
> >   	do {
> >   		idx = find_next_zero_bit(map, end, start);
> > @@ -410,6 +424,17 @@ static int __arm_smmu_alloc_bitmap(unsigned long *map, int start, int end)
> >   	} while (test_and_set_bit(idx, map));
> >   	return idx;
> > +
> > +found_handoff:
> > +	for (i = 0; i < smmu->num_mapping_groups; i++) {
> > +		if (smmu->s2crs[i].cbndx == cbndx) {
> > +			smmu->s2crs[i].cbndx = 0;
> > +			smmu->s2crs[i].handoff = false;
> > +			smmu->s2crs[i].count--;
> > +		}
> > +	}
> > +
> > +	return cbndx;
> >   }
> >   static void __arm_smmu_free_bitmap(unsigned long *map, int idx)
> > @@ -759,7 +784,8 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
> >   }
> >   static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> > -					struct arm_smmu_device *smmu)
> > +					struct arm_smmu_device *smmu,
> > +					struct device *dev)
> >   {
> >   	int irq, start, ret = 0;
> >   	unsigned long ias, oas;
> > @@ -873,8 +899,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> >   		ret = -EINVAL;
> >   		goto out_unlock;
> >   	}
> > -	ret = __arm_smmu_alloc_bitmap(smmu->context_map, start,
> > -				      smmu->num_context_banks);
> > +	ret = __arm_smmu_alloc_cb(smmu, start, dev);
> >   	if (ret < 0)
> >   		goto out_unlock;
> > @@ -1264,7 +1289,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> >   		return ret;
> >   	/* Ensure that the domain is finalised */
> > -	ret = arm_smmu_init_domain_context(domain, smmu);
> > +	ret = arm_smmu_init_domain_context(domain, smmu, dev);
> >   	if (ret < 0)
> >   		goto rpm_put;
> > @@ -1798,6 +1823,49 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> >   	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> >   }
> > +static void arm_smmu_read_smr_state(struct arm_smmu_device *smmu)
> > +{
> > +	u32 privcfg;
> > +	u32 cbndx;
> > +	u32 mask;
> > +	u32 type;
> > +	u32 s2cr;
> > +	u32 smr;
> > +	u32 id;
> > +	int i;
> > +
> > +	for (i = 0; i < smmu->num_mapping_groups; i++) {
> > +		smr = readl_relaxed(ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_SMR(i));
> 
> What about stream indexing?
> 
> > +		mask = (smr >> SMR_MASK_SHIFT) & SMR_MASK_MASK;
> > +		id = smr & SMR_ID_MASK;
> > +		if (!(smr & SMR_VALID))
> 
> EXIDs again...
> 
> > +			continue;
> > +
> > +		s2cr = readl_relaxed(ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_S2CR(i));
> > +		type = (s2cr >> S2CR_TYPE_SHIFT) & S2CR_TYPE_MASK;
> > +		cbndx = (s2cr >> S2CR_CBNDX_SHIFT) & S2CR_CBNDX_MASK;
> > +		privcfg = (s2cr >> S2CR_PRIVCFG_SHIFT) & S2CR_PRIVCFG_MASK;
> > +		if (type != S2CR_TYPE_TRANS)
> > +			continue;
> 
> How can you tell whether an SMR or S2CR has been programmed by the firmware,
> or that its UNKNOWN reset value is junk which just happens to look
> plausible?
> 

My expectation was that the reset value of these registers would have
some sane value, preferably without SMR_VALID being set.

> > +
> > +		/* Populate the SMR */
> > +		smmu->smrs[i].mask = mask;
> > +		smmu->smrs[i].id = id;
> > +		smmu->smrs[i].valid = true;
> > +
> > +		/* Populate the S2CR */
> > +		smmu->s2crs[i].group = NULL;
> > +		smmu->s2crs[i].count = 1;
> > +		smmu->s2crs[i].type = type;
> > +		smmu->s2crs[i].privcfg = privcfg;
> > +		smmu->s2crs[i].cbndx = cbndx;
> > +		smmu->s2crs[i].handoff = true;
> > +
> > +		/* Mark the context bank as busy */
> > +		bitmap_set(smmu->context_map, cbndx, 1);
> 
> Does anything prevent the SMMU being suspended between here and whenever the
> relevant drivers(s) show up to properly establish the device links?
> 

There's nothing that prevents a system suspend before such drivers are
inserted and attached.

> > +	}
> > +}
> > +
> >   static int arm_smmu_id_size_to_bits(int size)
> >   {
> >   	switch (size) {
> > @@ -2023,6 +2091,8 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
> >   		dev_notice(smmu->dev, "\tStage-2: %lu-bit IPA -> %lu-bit PA\n",
> >   			   smmu->ipa_size, smmu->pa_size);
> > +	arm_smmu_read_smr_state(smmu);
> > +
> >   	return 0;
> >   }
> 
> Stepping back from the implementation details, I also have concerns that
> this will interact badly with kexec/kdump,

I didn't consider kexec, which I expect would also rule out the type of
DT properties that downstream uses to trigger some of this behavior (as
the same DT would be used by the launched system).

> but mostly the fact that it's at
> best a partial workaround, rather than a real solution to the fundamental
> problem that initialising an IOMMU can break EFIFB/bootsplash/etc.,
> *regardless* of whether the firmware is even aware of said IOMMU at all -
> I've already been living with this on my Juno board for months since EDK2
> gained Arm HDLCD support. AFAICS this can only be solved by some sort of
> RMRR-like mechanism, i.e. firmware providing explicit information about what
> address ranges are currently in use by which devices. I've been pondering
> what a DT-based implementation might look like for a while now, and I guess
> it's probably time to raise it with the IORT spec owners as well.
> 

In the downstream DT targets a static reserved-memory region is used to
ensure Linux doesn't stomp on the actual framebuffer data and iirc the
ACPI firmware has the EFIFB memory region removed as well.

So I was expecting a translation of this particular region, but that's
not what we're seeing. But regardless, Linux needs to be aware of it
somehow, it's just that there's no link between the SMMU configuration
and this region as of today.


As a concrete example, booting a SDM845 based phone with this patch set
(and some debug prints) indicates that I have SMRs for primary and
secondary display pipe and then four entries for the various storage
devices.

> That said, once we've perfected our WIP design for keeping invasive
> implementation details out of the way of each other and the core
> architectural driver flow, I wouldn't rule out the possibility of slotting
> something like this in as a qcom-specific feature if the "we can't change
> the firmware" argument is going to come up.
> 

While it's possible to alter the firmware for specific (and future)
devices, figuring out a way to do this in the kernel will be necessary
for the majority of existing devices. In many cases we can work around
this problem by configuring the bootloader to not do a splash screen but
with EFIFB on the laptops we do not have this ability (and that's why
these patches has not shown up until now).

I'm very much interested in us coming up with a sustainable, generic
solution that we can work with the firmware guys on implementing beyond
the current needs.


I will read up on the stream ids and EXIDs and make the read back of
SMRs conditioned on the qcom compatible.

Thanks Robin

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-06-13 22:49 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-05 21:08 [RFC 0/2] iommu: arm-smmu: Inherit SMR and CB config during init Bjorn Andersson
2019-06-05 21:08 ` [RFC 1/2] iommu: arm-smmu: Handoff SMR registers and context banks Bjorn Andersson
2019-06-12 18:07   ` Jeffrey Hugo
2019-06-12 18:42     ` Bjorn Andersson
2019-06-12 19:16       ` Jeffrey Hugo
2019-06-13 11:23   ` Robin Murphy
2019-06-13 22:49     ` Bjorn Andersson
2019-06-05 21:08 ` [RFC 2/2] iommu: arm-smmu: Don't blindly use first SMR to calculate mask Bjorn Andersson
2019-06-12 17:58   ` Jeffrey Hugo
2019-06-12 18:28     ` Bjorn Andersson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).