[PATCH v2 1/2] IOMMU: arm-smmu-v3: Fix broken STE.VALID check in Broadcom Vulcan

[PATCH v2 1/2] IOMMU: arm-smmu-v3: Fix broken STE.VALID check in Broadcom Vulcan

[Prem Mallappa]

Vulcan SMMUv3 looks into more bits than necessary to validate the STE
entry (including EATS, S2PS, AARCH64), which is a overkill, but without
the proper encoding; the SMMU stops processing PCIe read/write requests.
Giving the h/w what it wants.

Signed-off-by: Prem Mallappa <pmallapp-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
 drivers/iommu/arm-smmu-v3.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 359190b..9886d2d 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -260,6 +260,7 @@
 #define STRTAB_STE_2_S2VMID_MASK	0xffffUL
 #define STRTAB_STE_2_VTCR_SHIFT		32
 #define STRTAB_STE_2_VTCR_MASK		0x7ffffUL
+#define STRTAB_STE_2_S2PS_SHIFT        48
 #define STRTAB_STE_2_S2AA64		(1UL << 51)
 #define STRTAB_STE_2_S2ENDI		(1UL << 52)
 #define STRTAB_STE_2_S2PTW		(1UL << 54)
@@ -579,6 +580,7 @@ struct arm_smmu_device {
 	u32				features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
+#define ARM_SMMU_OPT_BRCM_BROKEN_STE_VALID	(1 << 1)
 	u32				options;
 
 	struct arm_smmu_cmdq		cmdq;
@@ -643,6 +645,7 @@ struct arm_smmu_option_prop {
 
 static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
+	{ ARM_SMMU_OPT_BRCM_BROKEN_STE_VALID, "broadcom,broken-ste-valid-check" },
 	{ 0, NULL},
 };
 
@@ -1053,6 +1056,13 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,
 				      : STRTAB_STE_0_CFG_BYPASS;
 		dst[0] = cpu_to_le64(val);
 		dst[2] = 0; /* Nuke the VMID */
+
+		if (smmu && (smmu->options & ARM_SMMU_OPT_BRCM_BROKEN_STE_VALID)) {
+			WARN_ON(smmu->oas != 44);
+			dst[1] = STRTAB_STE_1_EATS_TRANS << STRTAB_STE_1_EATS_SHIFT;
+			dst[2] |= cpu_to_le64(STRTAB_STE_2_S2AA64);
+		}
+
 		if (ste_live)
 			arm_smmu_sync_ste_for_sid(smmu, sid);
 		return;
-- 
2.6.0

[PATCH v2 2/2] Documentation: dt-bindings: Add option to workaround STE.VALID in Broadcom Vulcan

[Prem Mallappa]

Signed-off-by: Prem Mallappa <pmallapp-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
 Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
index 947863a..9c1218e 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
@@ -43,6 +43,11 @@ the PCIe specification.
 - hisilicon,broken-prefetch-cmd
                     : Avoid sending CMD_PREFETCH_* commands to the SMMU.
 
+- broadcom,broken-ste-valid-check
+                    : Broadcom specific, h/w peeks into more than just 
+		      STE.VALID bit in "Bypass" mode. Enabling this, 
+		      populates needed bits.
+
 ** Example
 
         smmu@2b400000 {
-- 
2.6.0

Re: [PATCH v2 1/2] IOMMU: arm-smmu-v3: Fix broken STE.VALID check in Broadcom Vulcan

[Will Deacon]

Hi Prem,

Thanks for the update. I still have a few questions though.

On Thu, Dec 17, 2015 at 03:29:35PM +0530, Prem Mallappa wrote:
> Vulcan SMMUv3 looks into more bits than necessary to validate the STE
> entry (including EATS, S2PS, AARCH64), which is a overkill, but without
> the proper encoding; the SMMU stops processing PCIe read/write requests.
> Giving the h/w what it wants.
> 
> Signed-off-by: Prem Mallappa <pmallapp-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu-v3.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 359190b..9886d2d 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -260,6 +260,7 @@
>  #define STRTAB_STE_2_S2VMID_MASK	0xffffUL
>  #define STRTAB_STE_2_VTCR_SHIFT		32
>  #define STRTAB_STE_2_VTCR_MASK		0x7ffffUL
> +#define STRTAB_STE_2_S2PS_SHIFT        48
>  #define STRTAB_STE_2_S2AA64		(1UL << 51)
>  #define STRTAB_STE_2_S2ENDI		(1UL << 52)
>  #define STRTAB_STE_2_S2PTW		(1UL << 54)
> @@ -579,6 +580,7 @@ struct arm_smmu_device {
>  	u32				features;
>  
>  #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
> +#define ARM_SMMU_OPT_BRCM_BROKEN_STE_VALID	(1 << 1)
>  	u32				options;
>  
>  	struct arm_smmu_cmdq		cmdq;
> @@ -643,6 +645,7 @@ struct arm_smmu_option_prop {
>  
>  static struct arm_smmu_option_prop arm_smmu_options[] = {
>  	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
> +	{ ARM_SMMU_OPT_BRCM_BROKEN_STE_VALID, "broadcom,broken-ste-valid-check" },
>  	{ 0, NULL},
>  };
>  
> @@ -1053,6 +1056,13 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,
>  				      : STRTAB_STE_0_CFG_BYPASS;
>  		dst[0] = cpu_to_le64(val);
>  		dst[2] = 0; /* Nuke the VMID */
> +
> +		if (smmu && (smmu->options & ARM_SMMU_OPT_BRCM_BROKEN_STE_VALID)) {
> +			WARN_ON(smmu->oas != 44);

Do we really need this WARN_ON?

> +			dst[1] = STRTAB_STE_1_EATS_TRANS << STRTAB_STE_1_EATS_SHIFT;

cpu_to_le64 here too.

> +			dst[2] |= cpu_to_le64(STRTAB_STE_2_S2AA64);
> +		}
> +

Is this workaround only needed for bypass STEs? If not, we have a problem
when we install a stage-1 entry, because we'll clear the EATS bits as
it stands.

Will

Re: [PATCH v2 2/2] Documentation: dt-bindings: Add option to workaround STE.VALID in Broadcom Vulcan

[Will Deacon]

[adding devicetree since I'd like an Ack from them if possible]

On Thu, Dec 17, 2015 at 03:29:36PM +0530, Prem Mallappa wrote:
> Signed-off-by: Prem Mallappa <pmallapp-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>

Please can you add a commit message for this?

> ---
>  Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> index 947863a..9c1218e 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> @@ -43,6 +43,11 @@ the PCIe specification.
>  - hisilicon,broken-prefetch-cmd
>                      : Avoid sending CMD_PREFETCH_* commands to the SMMU.
>  
> +- broadcom,broken-ste-valid-check
> +                    : Broadcom specific, h/w peeks into more than just 
> +		      STE.VALID bit in "Bypass" mode. Enabling this, 
> +		      populates needed bits.

This should really describe more about what happens, I think. For example,

"Enable ATS and Stage-2 AArch64 translation in bypass STEs, since some
 hardware requires this in order for an STE to be treated as valid."

Will
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] Documentation: dt-bindings: Add option to workaround STE.VALID in Broadcom Vulcan

[Mark Rutland]

On Thu, Dec 17, 2015 at 11:51:03AM +0000, Will Deacon wrote:
> [adding devicetree since I'd like an Ack from them if possible]
> 
> On Thu, Dec 17, 2015 at 03:29:36PM +0530, Prem Mallappa wrote:
> > Signed-off-by: Prem Mallappa <pmallapp-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> 
> Please can you add a commit message for this?
> 
> > ---
> >  Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> > index 947863a..9c1218e 100644
> > --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> > @@ -43,6 +43,11 @@ the PCIe specification.
> >  - hisilicon,broken-prefetch-cmd
> >                      : Avoid sending CMD_PREFETCH_* commands to the SMMU.
> >  
> > +- broadcom,broken-ste-valid-check
> > +                    : Broadcom specific, h/w peeks into more than just 
> > +		      STE.VALID bit in "Bypass" mode. Enabling this, 
> > +		      populates needed bits.
> 
> This should really describe more about what happens, I think. For example,
> 
> "Enable ATS and Stage-2 AArch64 translation in bypass STEs, since some
>  hardware requires this in order for an STE to be treated as valid."

We certainly need to define the "needed bits".

Assuming your example cover all of those, it sounds fine to me.

Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 1/2] IOMMU: arm-smmu-v3: Fix broken STE.VALID check in Broadcom Vulcan

[Prem (Premachandra) Mallappa]

Hi Will, 

> 
> Is this workaround only needed for bypass STEs? If not, we have a problem
> when we install a stage-1 entry, because we'll clear the EATS bits as it stands.
>

Yes, this _was_ needed only in Bypass STEs, AFAIK.
However, the h/w fixes that were committed recently has this worked out. 
I have tested and confirmed this patch is no longer necessary. We'll drop this series as STE.VALID is not broken anymore :).

Cheers,
/Prem

Re: [PATCH v2 1/2] IOMMU: arm-smmu-v3: Fix broken STE.VALID check in Broadcom Vulcan

[Will Deacon]

On Sat, Dec 19, 2015 at 04:47:30AM +0000, Prem (Premachandra) Mallappa wrote:
> Hi Will, 

Hello Prem,

> > 
> > Is this workaround only needed for bypass STEs? If not, we have a problem
> > when we install a stage-1 entry, because we'll clear the EATS bits as it stands.
> >
> 
> Yes, this _was_ needed only in Bypass STEs, AFAIK.
> However, the h/w fixes that were committed recently has this worked out. 
> I have tested and confirmed this patch is no longer necessary. We'll drop
> this series as STE.VALID is not broken anymore :).

Hey, that's an early Christmas present! :)

Will