Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature
@ 2026-07-24 12:39 Peter Griffin
  2026-07-24 12:39 ` [PATCH 1/2] dt-bindings: iommu: add arm,instdata-override property documentation Peter Griffin
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Peter Griffin @ 2026-07-24 12:39 UTC (permalink / raw)
  To: Will Deacon, Robin Murphy, Joerg Roedel (AMD), Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: Pranjal Shrivastava, Daniel Mentz, Mostafa Saleh,
	linux-arm-kernel, iommu, devicetree, linux-kernel, kernel-team,
	tudor.ambarus, andre.draszik, willmcvicker, jyescas,
	Peter Griffin

Hi folks,

These two patches add support for a new "arm,instdata-override" DT property
that enables the override of the instruction/data attribute of incoming
traffic to Data by setting the INSTCFG override bits.

It is intended to be specified when the smmu can't guarantee that these
attributes are provided correctly from the client device.

One SoC that requires this for some IP instances is the Google Laguna SoC
found in Pixel 10 phones. The initial device tree for the platform is
proposed in
https://lore.kernel.org/lkml/20260722-contrib-pg-pixel10-initial-dts-v2-0-3abae9717feb@linaro.org/
but other drivers like USB glue and phy are already merged upstream.

Downstream code has used "arm,instdata-override" DT property name, which I
have continued with in this series. Existing options upstream tend to use a
vendor property e.g. hisilicon,broken-prefetch-cmd or
cavium,cn9900-broken-page1-regspace. So an alternative name could be 
google,lga-instdata-override or possibly google,lga-instcfg-data-override.

So far I kept the original downstream name as the override feature is
specified in the Arm SMMU architecture specification and could potentially
be useful for other SoCs.

Arm SMMU spec that documents these registers can be found here:
https://developer.arm.com/documentation/ihi0070/ha/

regards,

Peter

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
Peter Griffin (2):
      dt-bindings: iommu: add arm,instdata-override property documentation
      iommu/arm-smmu-v3: Override for Inst/Data attribute

 .../devicetree/bindings/iommu/arm,smmu-v3.yaml     |  9 ++++++++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c        | 26 ++++++++++++++++++----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h        |  7 ++++++
 3 files changed, 38 insertions(+), 4 deletions(-)
---
base-commit: b4515cf4156356e8f4fe6e0fdc17f59adab9772f
change-id: 20260724-arm-smmu-v3-instcfg-override-b5650fdbfe34

Best regards,
-- 
Peter Griffin <peter.griffin@linaro.org>



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

* [PATCH 1/2] dt-bindings: iommu: add arm,instdata-override property documentation
  2026-07-24 12:39 [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature Peter Griffin
@ 2026-07-24 12:39 ` Peter Griffin
  2026-07-24 12:39 ` [PATCH 2/2] iommu/arm-smmu-v3: Override for Inst/Data attribute Peter Griffin
  2026-07-26 13:16 ` [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature Will Deacon
  2 siblings, 0 replies; 16+ messages in thread
From: Peter Griffin @ 2026-07-24 12:39 UTC (permalink / raw)
  To: Will Deacon, Robin Murphy, Joerg Roedel (AMD), Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: Pranjal Shrivastava, Daniel Mentz, Mostafa Saleh,
	linux-arm-kernel, iommu, devicetree, linux-kernel, kernel-team,
	tudor.ambarus, andre.draszik, willmcvicker, jyescas,
	Peter Griffin

Add a new arm,instdata-override DT property used to enable the incoming
attributes override feature to Data.

This is used, when the smmu can't guarantee that these attributes are
provided correctly from the client device. One such example is Google
Laguna SoC due to silicon errata on some IP instances.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
---
 Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml
index 82957334bea2..23ea5e8880d4 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml
@@ -83,6 +83,15 @@ properties:
       register access with page 0 offsets. Set for Cavium ThunderX2 silicon that
       doesn't support SMMU page1 register space.
 
+  arm,instdata-override:
+    type: boolean
+    description:
+      Override the instruction/data attribute of incoming traffic to Data
+      by setting the INSTCFG override in all Stream Table Entries. Used
+      when the smmu can't guarantee that these attributes are provided
+      correctly from the client device. Google Laguna SoC is one such
+      example for some smmu instances.
+
 allOf:
   - if:
       not:

-- 
2.55.0.229.g6434b31f56-goog



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

* [PATCH 2/2] iommu/arm-smmu-v3: Override for Inst/Data attribute
  2026-07-24 12:39 [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature Peter Griffin
  2026-07-24 12:39 ` [PATCH 1/2] dt-bindings: iommu: add arm,instdata-override property documentation Peter Griffin
@ 2026-07-24 12:39 ` Peter Griffin
  2026-07-24 18:04   ` Nicolin Chen
  2026-07-26 13:16 ` [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature Will Deacon
  2 siblings, 1 reply; 16+ messages in thread
From: Peter Griffin @ 2026-07-24 12:39 UTC (permalink / raw)
  To: Will Deacon, Robin Murphy, Joerg Roedel (AMD), Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: Pranjal Shrivastava, Daniel Mentz, Mostafa Saleh,
	linux-arm-kernel, iommu, devicetree, linux-kernel, kernel-team,
	tudor.ambarus, andre.draszik, willmcvicker, jyescas,
	Peter Griffin

Add an option to override the instruction/data attribute
of incoming traffic to Data by setting the INSTCFG override
in all Stream Table Entries.

The option can be provided in the device tree entry for the SMMU
by adding the "arm,instdata-override" property.

Changes by Peter Griffin:
 - Make it apply on vanilla upstream arm-smmu-v3

Signed-off-by: Pranjal Shrivastava <praan@google.com>
Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 26 ++++++++++++++++++++++----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  7 +++++++
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 57b750ebcd3d..2b39cb5aa49d 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -82,6 +82,7 @@ DEFINE_MUTEX(arm_smmu_asid_lock);
 static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
 	{ ARM_SMMU_OPT_PAGE0_REGS_ONLY, "cavium,cn9900-broken-page1-regspace"},
+	{ ARM_SMMU_OPT_OVR_INSTCFG_DATA, "arm,instdata-override"},
 	{ 0, NULL},
 };
 
@@ -1206,7 +1207,8 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
 			cpu_to_le64(STRTAB_STE_1_S1DSS | STRTAB_STE_1_S1CIR |
 				    STRTAB_STE_1_S1COR | STRTAB_STE_1_S1CSH |
 				    STRTAB_STE_1_S1STALLD | STRTAB_STE_1_STRW |
-				    STRTAB_STE_1_EATS | STRTAB_STE_1_MEV);
+				    STRTAB_STE_1_EATS | STRTAB_STE_1_MEV |
+				    STRTAB_STE_1_INSTCFG);
 		used_bits[2] |= cpu_to_le64(STRTAB_STE_2_S2VMID);
 
 		/*
@@ -1222,7 +1224,8 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
 	if (cfg & BIT(1)) {
 		used_bits[1] |=
 			cpu_to_le64(STRTAB_STE_1_S2FWB | STRTAB_STE_1_EATS |
-				    STRTAB_STE_1_SHCFG | STRTAB_STE_1_MEV);
+				    STRTAB_STE_1_SHCFG | STRTAB_STE_1_MEV |
+				    STRTAB_STE_1_INSTCFG);
 		used_bits[2] |=
 			cpu_to_le64(STRTAB_STE_2_S2VMID | STRTAB_STE_2_VTCR |
 				    STRTAB_STE_2_S2AA64 | STRTAB_STE_2_S2ENDI |
@@ -1835,7 +1838,11 @@ void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target,
 			 STRTAB_STE_1_S1STALLD :
 			 0) |
 		FIELD_PREP(STRTAB_STE_1_EATS,
-			   ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0));
+			   ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0)) |
+		FIELD_PREP(STRTAB_STE_1_INSTCFG,
+			   smmu->options & ARM_SMMU_OPT_OVR_INSTCFG_DATA ?
+				   STRTAB_STE_1_INSTCFG_DATA :
+				   STRTAB_STE_1_INSTCFG_INCOMING);
 
 	if ((smmu->features & ARM_SMMU_FEAT_ATTR_TYPES_OVR) &&
 	    s1dss == STRTAB_STE_1_S1DSS_BYPASS)
@@ -1887,7 +1894,11 @@ void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
 
 	target->data[1] = cpu_to_le64(
 		FIELD_PREP(STRTAB_STE_1_EATS,
-			   ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0));
+			   ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0) |
+		FIELD_PREP(STRTAB_STE_1_INSTCFG,
+			   smmu->options & ARM_SMMU_OPT_OVR_INSTCFG_DATA ?
+				   STRTAB_STE_1_INSTCFG_DATA :
+				   STRTAB_STE_1_INSTCFG_INCOMING));
 
 	if (pgtbl_cfg->quirks & IO_PGTABLE_QUIRK_ARM_S2FWB)
 		target->data[1] |= cpu_to_le64(STRTAB_STE_1_S2FWB);
@@ -5087,6 +5098,13 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
 	if (smmu->sid_bits <= STRTAB_SPLIT)
 		smmu->features &= ~ARM_SMMU_FEAT_2_LVL_STRTAB;
 
+	if (reg & IDR1_ATTR_PERMS_OVR) {
+		smmu->features |= ARM_SMMU_FEAT_PERMS_OVR;
+	} else if (smmu->options & ARM_SMMU_OPT_OVR_INSTCFG_DATA) {
+		dev_err(smmu->dev, "Inst/Data attribute override not supported\n");
+		return -ENXIO;
+	}
+
 	/* IDR3 */
 	reg = readl_relaxed(smmu->base + ARM_SMMU_IDR3);
 	if (FIELD_GET(IDR3_RIL, reg))
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index c909c9a88538..e5f5c0bc6b8a 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -51,6 +51,7 @@ struct arm_vsmmu;
 #define IDR1_QUEUES_PRESET		(1 << 29)
 #define IDR1_REL			(1 << 28)
 #define IDR1_ATTR_TYPES_OVR		(1 << 27)
+#define IDR1_ATTR_PERMS_OVR		(1 << 26)
 #define IDR1_CMDQS			GENMASK(25, 21)
 #define IDR1_EVTQS			GENMASK(20, 16)
 #define IDR1_PRIQS			GENMASK(15, 11)
@@ -283,6 +284,10 @@ static inline u32 arm_smmu_strtab_l2_idx(u32 sid)
 #define STRTAB_STE_1_SHCFG		GENMASK_ULL(45, 44)
 #define STRTAB_STE_1_SHCFG_INCOMING	1UL
 
+#define STRTAB_STE_1_INSTCFG		GENMASK_ULL(51, 50)
+#define STRTAB_STE_1_INSTCFG_INCOMING	0UL
+#define STRTAB_STE_1_INSTCFG_DATA	2UL
+
 #define STRTAB_STE_2_S2VMID		GENMASK_ULL(15, 0)
 #define STRTAB_STE_2_VTCR		GENMASK_ULL(50, 32)
 #define STRTAB_STE_2_VTCR_S2T0SZ	GENMASK_ULL(5, 0)
@@ -921,6 +926,7 @@ struct arm_smmu_device {
 #define ARM_SMMU_FEAT_HD		(1 << 22)
 #define ARM_SMMU_FEAT_S2FWB		(1 << 23)
 #define ARM_SMMU_FEAT_BBML2		(1 << 24)
+#define ARM_SMMU_FEAT_PERMS_OVR		(1 << 25)
 	u32				features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
@@ -928,6 +934,7 @@ struct arm_smmu_device {
 #define ARM_SMMU_OPT_MSIPOLL		(1 << 2)
 #define ARM_SMMU_OPT_CMDQ_FORCE_SYNC	(1 << 3)
 #define ARM_SMMU_OPT_TEGRA241_CMDQV	(1 << 4)
+#define ARM_SMMU_OPT_OVR_INSTCFG_DATA	(1 << 5)
 	u32				options;
 
 	struct arm_smmu_cmdq		cmdq;

-- 
2.55.0.229.g6434b31f56-goog



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

* Re: [PATCH 2/2] iommu/arm-smmu-v3: Override for Inst/Data attribute
  2026-07-24 12:39 ` [PATCH 2/2] iommu/arm-smmu-v3: Override for Inst/Data attribute Peter Griffin
@ 2026-07-24 18:04   ` Nicolin Chen
  2026-08-07 14:37     ` Peter Griffin
  0 siblings, 1 reply; 16+ messages in thread
From: Nicolin Chen @ 2026-07-24 18:04 UTC (permalink / raw)
  To: Peter Griffin
  Cc: Will Deacon, Robin Murphy, Joerg Roedel (AMD), Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Pranjal Shrivastava,
	Daniel Mentz, Mostafa Saleh, linux-arm-kernel, iommu, devicetree,
	linux-kernel, kernel-team, tudor.ambarus, andre.draszik,
	willmcvicker, jyescas

On Fri, Jul 24, 2026 at 01:39:43PM +0100, Peter Griffin wrote:
> @@ -1206,7 +1207,8 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
>  			cpu_to_le64(STRTAB_STE_1_S1DSS | STRTAB_STE_1_S1CIR |
>  				    STRTAB_STE_1_S1COR | STRTAB_STE_1_S1CSH |
>  				    STRTAB_STE_1_S1STALLD | STRTAB_STE_1_STRW |
> -				    STRTAB_STE_1_EATS | STRTAB_STE_1_MEV);
> +				    STRTAB_STE_1_EATS | STRTAB_STE_1_MEV |
> +				    STRTAB_STE_1_INSTCFG);
>  		used_bits[2] |= cpu_to_le64(STRTAB_STE_2_S2VMID);
>  
>  		/*
> @@ -1222,7 +1224,8 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
>  	if (cfg & BIT(1)) {
>  		used_bits[1] |=
>  			cpu_to_le64(STRTAB_STE_1_S2FWB | STRTAB_STE_1_EATS |
> -				    STRTAB_STE_1_SHCFG | STRTAB_STE_1_MEV);
> +				    STRTAB_STE_1_SHCFG | STRTAB_STE_1_MEV |
> +				    STRTAB_STE_1_INSTCFG);
>  		used_bits[2] |=
>  			cpu_to_le64(STRTAB_STE_2_S2VMID | STRTAB_STE_2_VTCR |
>  				    STRTAB_STE_2_S2AA64 | STRTAB_STE_2_S2ENDI |

This adds for "stage-1" and "stage-2", while missing "bypass"?

> @@ -1835,7 +1838,11 @@ void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target,
>  			 STRTAB_STE_1_S1STALLD :
>  			 0) |
>  		FIELD_PREP(STRTAB_STE_1_EATS,
> -			   ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0));
> +			   ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0)) |
> +		FIELD_PREP(STRTAB_STE_1_INSTCFG,
> +			   smmu->options & ARM_SMMU_OPT_OVR_INSTCFG_DATA ?
> +				   STRTAB_STE_1_INSTCFG_DATA :
> +				   STRTAB_STE_1_INSTCFG_INCOMING);

Sashiko pointed out that this prematurely closes the cpu_to_le64()
macro.

> @@ -1887,7 +1894,11 @@ void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
>  
>  	target->data[1] = cpu_to_le64(
>  		FIELD_PREP(STRTAB_STE_1_EATS,
> -			   ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0));
> +			   ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0) |
> +		FIELD_PREP(STRTAB_STE_1_INSTCFG,
> +			   smmu->options & ARM_SMMU_OPT_OVR_INSTCFG_DATA ?
> +				   STRTAB_STE_1_INSTCFG_DATA :
> +				   STRTAB_STE_1_INSTCFG_INCOMING));

You might need to set in arm_smmu_make_bypass_ste() too.

> @@ -5087,6 +5098,13 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
>  	if (smmu->sid_bits <= STRTAB_SPLIT)
>  		smmu->features &= ~ARM_SMMU_FEAT_2_LVL_STRTAB;
>  
> +	if (reg & IDR1_ATTR_PERMS_OVR) {
> +		smmu->features |= ARM_SMMU_FEAT_PERMS_OVR;

Where does this new feature bit get used?

Nicolin


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

* Re: [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature
  2026-07-24 12:39 [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature Peter Griffin
  2026-07-24 12:39 ` [PATCH 1/2] dt-bindings: iommu: add arm,instdata-override property documentation Peter Griffin
  2026-07-24 12:39 ` [PATCH 2/2] iommu/arm-smmu-v3: Override for Inst/Data attribute Peter Griffin
@ 2026-07-26 13:16 ` Will Deacon
  2026-07-27 10:53   ` Robin Murphy
  2 siblings, 1 reply; 16+ messages in thread
From: Will Deacon @ 2026-07-26 13:16 UTC (permalink / raw)
  To: Peter Griffin
  Cc: Robin Murphy, Joerg Roedel (AMD), Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Pranjal Shrivastava,
	Daniel Mentz, Mostafa Saleh, linux-arm-kernel, iommu, devicetree,
	linux-kernel, kernel-team, tudor.ambarus, andre.draszik,
	willmcvicker, jyescas

On Fri, Jul 24, 2026 at 01:39:41PM +0100, Peter Griffin wrote:
> These two patches add support for a new "arm,instdata-override" DT property
> that enables the override of the instruction/data attribute of incoming
> traffic to Data by setting the INSTCFG override bits.
> 
> It is intended to be specified when the smmu can't guarantee that these
> attributes are provided correctly from the client device.

This is going to need an in-tree user and a much more detailed
description of what is being worked around before we consider this for
inclusion.

In particular, if a particular client is emitting data reads as
instructions, then a better work around would be to avoid mapping its
domains using IOMMU_NOEXEC. But I can't tell what's going on from the
limited description provided here.

Cheers,

Will


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

* Re: [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature
  2026-07-26 13:16 ` [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature Will Deacon
@ 2026-07-27 10:53   ` Robin Murphy
  2026-08-07 15:25     ` Peter Griffin
  0 siblings, 1 reply; 16+ messages in thread
From: Robin Murphy @ 2026-07-27 10:53 UTC (permalink / raw)
  To: Will Deacon, Peter Griffin
  Cc: Joerg Roedel (AMD), Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Pranjal Shrivastava, Daniel Mentz, Mostafa Saleh,
	linux-arm-kernel, iommu, devicetree, linux-kernel, kernel-team,
	tudor.ambarus, andre.draszik, willmcvicker, jyescas

On 26/07/2026 2:16 pm, Will Deacon wrote:
> On Fri, Jul 24, 2026 at 01:39:41PM +0100, Peter Griffin wrote:
>> These two patches add support for a new "arm,instdata-override" DT property
>> that enables the override of the instruction/data attribute of incoming
>> traffic to Data by setting the INSTCFG override bits.
>>
>> It is intended to be specified when the smmu can't guarantee that these
>> attributes are provided correctly from the client device.
> 
> This is going to need an in-tree user and a much more detailed
> description of what is being worked around before we consider this for
> inclusion.
> 
> In particular, if a particular client is emitting data reads as
> instructions, then a better work around would be to avoid mapping its
> domains using IOMMU_NOEXEC. But I can't tell what's going on from the
> limited description provided here.

Unless it's also emitting the privileged bit and thus falling foul of 
the implicit Unpriv-W -> Priv-XN rule, but then we also have the means 
to deal with devices which actually do that themselves (hello pl330...), 
so that would seemingly only leave the case of some innocent piece of 
AMBA-interfaced IP which doesn't expect to need special attributes, but 
the system integrator has gone out of their way to tie the AxPROT bits 
to some wacky value, which I would put in "erratum workaround" territory.

Thanks,
Robin.


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

* Re: [PATCH 2/2] iommu/arm-smmu-v3: Override for Inst/Data attribute
  2026-07-24 18:04   ` Nicolin Chen
@ 2026-08-07 14:37     ` Peter Griffin
  2026-08-07 16:43       ` Nicolin Chen
  2026-08-10 14:15       ` Jason Gunthorpe
  0 siblings, 2 replies; 16+ messages in thread
From: Peter Griffin @ 2026-08-07 14:37 UTC (permalink / raw)
  To: Nicolin Chen
  Cc: Will Deacon, Robin Murphy, Joerg Roedel (AMD), Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Pranjal Shrivastava,
	Daniel Mentz, Mostafa Saleh, linux-arm-kernel, iommu, devicetree,
	linux-kernel, kernel-team, tudor.ambarus, andre.draszik,
	willmcvicker, jyescas

Hi Nicolin,

Thank you for your review feedback.

On Fri, 24 Jul 2026 at 19:05, Nicolin Chen <nicolinc@nvidia.com> wrote:
>
> On Fri, Jul 24, 2026 at 01:39:43PM +0100, Peter Griffin wrote:
> > @@ -1206,7 +1207,8 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
> >                       cpu_to_le64(STRTAB_STE_1_S1DSS | STRTAB_STE_1_S1CIR |
> >                                   STRTAB_STE_1_S1COR | STRTAB_STE_1_S1CSH |
> >                                   STRTAB_STE_1_S1STALLD | STRTAB_STE_1_STRW |
> > -                                 STRTAB_STE_1_EATS | STRTAB_STE_1_MEV);
> > +                                 STRTAB_STE_1_EATS | STRTAB_STE_1_MEV |
> > +                                 STRTAB_STE_1_INSTCFG);
> >               used_bits[2] |= cpu_to_le64(STRTAB_STE_2_S2VMID);
> >
> >               /*
> > @@ -1222,7 +1224,8 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
> >       if (cfg & BIT(1)) {
> >               used_bits[1] |=
> >                       cpu_to_le64(STRTAB_STE_1_S2FWB | STRTAB_STE_1_EATS |
> > -                                 STRTAB_STE_1_SHCFG | STRTAB_STE_1_MEV);
> > +                                 STRTAB_STE_1_SHCFG | STRTAB_STE_1_MEV |
> > +                                 STRTAB_STE_1_INSTCFG);
> >               used_bits[2] |=
> >                       cpu_to_le64(STRTAB_STE_2_S2VMID | STRTAB_STE_2_VTCR |
> >                                   STRTAB_STE_2_S2AA64 | STRTAB_STE_2_S2ENDI |
>
> This adds for "stage-1" and "stage-2", while missing "bypass"?

I need to follow up again on this point, but I don't believe currently
we require the override for Laguna when it's in bypass.

>
> > @@ -1835,7 +1838,11 @@ void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target,
> >                        STRTAB_STE_1_S1STALLD :
> >                        0) |
> >               FIELD_PREP(STRTAB_STE_1_EATS,
> > -                        ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0));
> > +                        ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0)) |
> > +             FIELD_PREP(STRTAB_STE_1_INSTCFG,
> > +                        smmu->options & ARM_SMMU_OPT_OVR_INSTCFG_DATA ?
> > +                                STRTAB_STE_1_INSTCFG_DATA :
> > +                                STRTAB_STE_1_INSTCFG_INCOMING);
>
> Sashiko pointed out that this prematurely closes the cpu_to_le64()
> macro.

Will fix in v2

>
> > @@ -1887,7 +1894,11 @@ void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
> >
> >       target->data[1] = cpu_to_le64(
> >               FIELD_PREP(STRTAB_STE_1_EATS,
> > -                        ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0));
> > +                        ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0) |
> > +             FIELD_PREP(STRTAB_STE_1_INSTCFG,
> > +                        smmu->options & ARM_SMMU_OPT_OVR_INSTCFG_DATA ?
> > +                                STRTAB_STE_1_INSTCFG_DATA :
> > +                                STRTAB_STE_1_INSTCFG_INCOMING));
>
> You might need to set in arm_smmu_make_bypass_ste() too.
>
> > @@ -5087,6 +5098,13 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
> >       if (smmu->sid_bits <= STRTAB_SPLIT)
> >               smmu->features &= ~ARM_SMMU_FEAT_2_LVL_STRTAB;
> >
> > +     if (reg & IDR1_ATTR_PERMS_OVR) {
> > +             smmu->features |= ARM_SMMU_FEAT_PERMS_OVR;
>
> Where does this new feature bit get used?

Good point, the feature flag isn't actually used anywhere else except
here. The intention is to ensure the DT property
"arm,instdata-override" is not specified for a SMMU which doesn't
support the override feature. But we can perform that test without
adding a feature flag that is otherwise unused. Will fix in the next
version.

Thanks,

Peter


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

* Re: [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature
  2026-07-27 10:53   ` Robin Murphy
@ 2026-08-07 15:25     ` Peter Griffin
  2026-08-07 18:21       ` Daniel Mentz
                         ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Peter Griffin @ 2026-08-07 15:25 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Will Deacon, Joerg Roedel (AMD), Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Pranjal Shrivastava, Daniel Mentz, Mostafa Saleh,
	linux-arm-kernel, iommu, devicetree, linux-kernel, kernel-team,
	tudor.ambarus, andre.draszik, willmcvicker, jyescas

Hi Will & Robin,

Thank you both for your review feedback.

On Mon, 27 Jul 2026 at 11:53, Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 26/07/2026 2:16 pm, Will Deacon wrote:
> > On Fri, Jul 24, 2026 at 01:39:41PM +0100, Peter Griffin wrote:
> >> These two patches add support for a new "arm,instdata-override" DT property
> >> that enables the override of the instruction/data attribute of incoming
> >> traffic to Data by setting the INSTCFG override bits.
> >>
> >> It is intended to be specified when the smmu can't guarantee that these
> >> attributes are provided correctly from the client device.
> >
> > This is going to need an in-tree user and a much more detailed
> > description of what is being worked around before we consider this for
> > inclusion.

Regarding an in-tree user, I haven't sent the Device Tree (DT) patch
yet for Laguna SoC which adds the smmu nodes and this property because
1) I want to land the initial SoC/board DT first
2) I want agreement on the DT property name. Currently I used
"arm,instdata-override" which is what downstream used. However, since
this is intended to work around silicon errata something like
"google,lga-instcfg-data-override" might be more appropriate?

For Laguna SoC the first in-tree user of this is the amb_smmu smmu
instance which is used by the Synopsis dwc3 IP. The Laguna dwc3 glue
driver is already upstream at drivers/usb/dwc3/dwc3-google.c

> >
> > In particular, if a particular client is emitting data reads as
> > instructions, then a better work around would be to avoid mapping its
> > domains using IOMMU_NOEXEC. But I can't tell what's going on from the
> > limited description provided here.
>
> Unless it's also emitting the privileged bit and thus falling foul of
> the implicit Unpriv-W -> Priv-XN rule, but then we also have the means
> to deal with devices which actually do that themselves (hello pl330...),
> so that would seemingly only leave the case of some innocent piece of
> AMBA-interfaced IP which doesn't expect to need special attributes, but
> the system integrator has gone out of their way to tie the AxPROT bits
> to some wacky value, which I would put in "erratum workaround" territory.
>

You're correct Robin. It is an erratum workaround for the Laguna SoC
due to some custom usage of the AxPROT bits which differs from the
standard ARM SMMU handling for Privileged/Unprivileged and
Instruction/Data transaction attributes. The effect is all
transactions appear to the SMMU as "Privileged Instruction" accesses.
The software workaround in this series enables the SMMU's INSTCFG
override feature to ignore the incoming value and treat all SMMU
transactions as "Data".

One small clarification: in the cover letter I incorrectly said this
was set only for some SMMU IP instances, but that is incorrect. It is
actually set on *all* arm-smmu-v3 IP instances in the Laguna SoC.

Does the above provide the additional detail you need Will?

Thanks,

Peter


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

* Re: [PATCH 2/2] iommu/arm-smmu-v3: Override for Inst/Data attribute
  2026-08-07 14:37     ` Peter Griffin
@ 2026-08-07 16:43       ` Nicolin Chen
  2026-08-10 14:15       ` Jason Gunthorpe
  1 sibling, 0 replies; 16+ messages in thread
From: Nicolin Chen @ 2026-08-07 16:43 UTC (permalink / raw)
  To: Peter Griffin
  Cc: Will Deacon, Robin Murphy, Joerg Roedel (AMD), Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Pranjal Shrivastava,
	Daniel Mentz, Mostafa Saleh, linux-arm-kernel, iommu, devicetree,
	linux-kernel, kernel-team, tudor.ambarus, andre.draszik,
	willmcvicker, jyescas

On Fri, Aug 07, 2026 at 03:37:37PM +0100, Peter Griffin wrote:
> Hi Nicolin,
> 
> Thank you for your review feedback.
> 
> On Fri, 24 Jul 2026 at 19:05, Nicolin Chen <nicolinc@nvidia.com> wrote:
> >
> > On Fri, Jul 24, 2026 at 01:39:43PM +0100, Peter Griffin wrote:
> > > @@ -1206,7 +1207,8 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
> > >                       cpu_to_le64(STRTAB_STE_1_S1DSS | STRTAB_STE_1_S1CIR |
> > >                                   STRTAB_STE_1_S1COR | STRTAB_STE_1_S1CSH |
> > >                                   STRTAB_STE_1_S1STALLD | STRTAB_STE_1_STRW |
> > > -                                 STRTAB_STE_1_EATS | STRTAB_STE_1_MEV);
> > > +                                 STRTAB_STE_1_EATS | STRTAB_STE_1_MEV |
> > > +                                 STRTAB_STE_1_INSTCFG);
> > >               used_bits[2] |= cpu_to_le64(STRTAB_STE_2_S2VMID);
> > >
> > >               /*
> > > @@ -1222,7 +1224,8 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
> > >       if (cfg & BIT(1)) {
> > >               used_bits[1] |=
> > >                       cpu_to_le64(STRTAB_STE_1_S2FWB | STRTAB_STE_1_EATS |
> > > -                                 STRTAB_STE_1_SHCFG | STRTAB_STE_1_MEV);
> > > +                                 STRTAB_STE_1_SHCFG | STRTAB_STE_1_MEV |
> > > +                                 STRTAB_STE_1_INSTCFG);
> > >               used_bits[2] |=
> > >                       cpu_to_le64(STRTAB_STE_2_S2VMID | STRTAB_STE_2_VTCR |
> > >                                   STRTAB_STE_2_S2AA64 | STRTAB_STE_2_S2ENDI |
> >
> > This adds for "stage-1" and "stage-2", while missing "bypass"?
> 
> I need to follow up again on this point, but I don't believe currently
> we require the override for Laguna when it's in bypass.

This get_ste_used lists all the critical fields that the HW will
read when operating in the mode per STE.Cfg.

Given that GBPA also has INSTCFG, STE.Cfg=Bypass uses it as well.

Nicolin


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

* Re: [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature
  2026-08-07 15:25     ` Peter Griffin
@ 2026-08-07 18:21       ` Daniel Mentz
  2026-08-07 22:47       ` Rob Herring
  2026-08-10 11:00       ` Will Deacon
  2 siblings, 0 replies; 16+ messages in thread
From: Daniel Mentz @ 2026-08-07 18:21 UTC (permalink / raw)
  To: Peter Griffin
  Cc: Robin Murphy, Will Deacon, Joerg Roedel (AMD), Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Pranjal Shrivastava,
	Mostafa Saleh, linux-arm-kernel, iommu, devicetree, linux-kernel,
	kernel-team, tudor.ambarus, andre.draszik, willmcvicker, jyescas

On Fri, Aug 7, 2026 at 8:25 AM Peter Griffin <peter.griffin@linaro.org> wrote:
>
> Hi Will & Robin,
>
> Thank you both for your review feedback.
>
> On Mon, 27 Jul 2026 at 11:53, Robin Murphy <robin.murphy@arm.com> wrote:
> >
> > On 26/07/2026 2:16 pm, Will Deacon wrote:
> > > On Fri, Jul 24, 2026 at 01:39:41PM +0100, Peter Griffin wrote:
> > >> These two patches add support for a new "arm,instdata-override" DT property
> > >> that enables the override of the instruction/data attribute of incoming
> > >> traffic to Data by setting the INSTCFG override bits.
> > >>
> > >> It is intended to be specified when the smmu can't guarantee that these
> > >> attributes are provided correctly from the client device.
> > >
> > > This is going to need an in-tree user and a much more detailed
> > > description of what is being worked around before we consider this for
> > > inclusion.
>
> Regarding an in-tree user, I haven't sent the Device Tree (DT) patch
> yet for Laguna SoC which adds the smmu nodes and this property because
> 1) I want to land the initial SoC/board DT first
> 2) I want agreement on the DT property name. Currently I used
> "arm,instdata-override" which is what downstream used. However, since
> this is intended to work around silicon errata something like
> "google,lga-instcfg-data-override" might be more appropriate?
>
> For Laguna SoC the first in-tree user of this is the amb_smmu smmu
> instance which is used by the Synopsis dwc3 IP. The Laguna dwc3 glue
> driver is already upstream at drivers/usb/dwc3/dwc3-google.c
>
> > >
> > > In particular, if a particular client is emitting data reads as
> > > instructions, then a better work around would be to avoid mapping its
> > > domains using IOMMU_NOEXEC. But I can't tell what's going on from the
> > > limited description provided here.
> >
> > Unless it's also emitting the privileged bit and thus falling foul of
> > the implicit Unpriv-W -> Priv-XN rule, but then we also have the means
> > to deal with devices which actually do that themselves (hello pl330...),
> > so that would seemingly only leave the case of some innocent piece of
> > AMBA-interfaced IP which doesn't expect to need special attributes, but
> > the system integrator has gone out of their way to tie the AxPROT bits
> > to some wacky value, which I would put in "erratum workaround" territory.
> >
>
> You're correct Robin. It is an erratum workaround for the Laguna SoC
> due to some custom usage of the AxPROT bits which differs from the
> standard ARM SMMU handling for Privileged/Unprivileged and
> Instruction/Data transaction attributes. The effect is all
> transactions appear to the SMMU as "Privileged Instruction" accesses.
> The software workaround in this series enables the SMMU's INSTCFG
> override feature to ignore the incoming value and treat all SMMU
> transactions as "Data".

Due to the specific integration of the MMU-700 in this system,
AxPROT[2] is always 1 on all TBU subordinate interfaces, regardless of
whether the transaction is an instruction or data transaction.
Consequently, client devices have no control over AxPROT[2] on the
input side of SMMU. AxPROT[0] (privileged vs. unprivileged) remains
unaffected. As Robin suggested, the override is required to avoid
inadvertently triggering F_PERMISSION faults due to the VMSAv8-64
requirement that EL0-writable regions are treated as Privileged
Execute-Never (PXN)

Drivers are already avoiding IOMMU_NOEXEC. I prefer handling this via
the SMMU configuration rather than patching individual client drivers,
as this is an integration issue, not a client device issue.


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

* Re: [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature
  2026-08-07 15:25     ` Peter Griffin
  2026-08-07 18:21       ` Daniel Mentz
@ 2026-08-07 22:47       ` Rob Herring
  2026-08-10 18:03         ` Robin Murphy
  2026-08-10 11:00       ` Will Deacon
  2 siblings, 1 reply; 16+ messages in thread
From: Rob Herring @ 2026-08-07 22:47 UTC (permalink / raw)
  To: Peter Griffin
  Cc: Robin Murphy, Will Deacon, Joerg Roedel (AMD),
	Krzysztof Kozlowski, Conor Dooley, Pranjal Shrivastava,
	Daniel Mentz, Mostafa Saleh, linux-arm-kernel, iommu, devicetree,
	linux-kernel, kernel-team, tudor.ambarus, andre.draszik,
	willmcvicker, jyescas

On Fri, Aug 07, 2026 at 04:25:11PM +0100, Peter Griffin wrote:
> Hi Will & Robin,
> 
> Thank you both for your review feedback.
> 
> On Mon, 27 Jul 2026 at 11:53, Robin Murphy <robin.murphy@arm.com> wrote:
> >
> > On 26/07/2026 2:16 pm, Will Deacon wrote:
> > > On Fri, Jul 24, 2026 at 01:39:41PM +0100, Peter Griffin wrote:
> > >> These two patches add support for a new "arm,instdata-override" DT property
> > >> that enables the override of the instruction/data attribute of incoming
> > >> traffic to Data by setting the INSTCFG override bits.
> > >>
> > >> It is intended to be specified when the smmu can't guarantee that these
> > >> attributes are provided correctly from the client device.
> > >
> > > This is going to need an in-tree user and a much more detailed
> > > description of what is being worked around before we consider this for
> > > inclusion.
> 
> Regarding an in-tree user, I haven't sent the Device Tree (DT) patch
> yet for Laguna SoC which adds the smmu nodes and this property because
> 1) I want to land the initial SoC/board DT first
> 2) I want agreement on the DT property name. Currently I used
> "arm,instdata-override" which is what downstream used. However, since
> this is intended to work around silicon errata something like
> "google,lga-instcfg-data-override" might be more appropriate?

Shrug.

Really, this is why we want SoC specific compatibles so work-arounds can 
be added without DT updates. But we've generally avoided doing that for 
SMMU for whatever reasons. Just make the property clearly sound like a 
mistake, not a feature you might want to use. I'd keep the arm prefix 
just so the next vendor thinks they can't use 'google' and creates the 
same property with their vendor prefix.

Rob


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

* Re: [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature
  2026-08-07 15:25     ` Peter Griffin
  2026-08-07 18:21       ` Daniel Mentz
  2026-08-07 22:47       ` Rob Herring
@ 2026-08-10 11:00       ` Will Deacon
  2026-08-10 17:16         ` Robin Murphy
  2 siblings, 1 reply; 16+ messages in thread
From: Will Deacon @ 2026-08-10 11:00 UTC (permalink / raw)
  To: Peter Griffin
  Cc: Robin Murphy, Joerg Roedel (AMD), Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Pranjal Shrivastava,
	Daniel Mentz, Mostafa Saleh, linux-arm-kernel, iommu, devicetree,
	linux-kernel, kernel-team, tudor.ambarus, andre.draszik,
	willmcvicker, jyescas

On Fri, Aug 07, 2026 at 04:25:11PM +0100, Peter Griffin wrote:
> On Mon, 27 Jul 2026 at 11:53, Robin Murphy <robin.murphy@arm.com> wrote:
> >
> > On 26/07/2026 2:16 pm, Will Deacon wrote:
> > > On Fri, Jul 24, 2026 at 01:39:41PM +0100, Peter Griffin wrote:
> > >> These two patches add support for a new "arm,instdata-override" DT property
> > >> that enables the override of the instruction/data attribute of incoming
> > >> traffic to Data by setting the INSTCFG override bits.
> > >>
> > >> It is intended to be specified when the smmu can't guarantee that these
> > >> attributes are provided correctly from the client device.
> > >
> > > This is going to need an in-tree user and a much more detailed
> > > description of what is being worked around before we consider this for
> > > inclusion.
> 
> Regarding an in-tree user, I haven't sent the Device Tree (DT) patch
> yet for Laguna SoC which adds the smmu nodes and this property because
> 1) I want to land the initial SoC/board DT first
> 2) I want agreement on the DT property name. Currently I used
> "arm,instdata-override" which is what downstream used. However, since
> this is intended to work around silicon errata something like
> "google,lga-instcfg-data-override" might be more appropriate?
> 
> For Laguna SoC the first in-tree user of this is the amb_smmu smmu
> instance which is used by the Synopsis dwc3 IP. The Laguna dwc3 glue
> driver is already upstream at drivers/usb/dwc3/dwc3-google.c
> 
> > >
> > > In particular, if a particular client is emitting data reads as
> > > instructions, then a better work around would be to avoid mapping its
> > > domains using IOMMU_NOEXEC. But I can't tell what's going on from the
> > > limited description provided here.
> >
> > Unless it's also emitting the privileged bit and thus falling foul of
> > the implicit Unpriv-W -> Priv-XN rule, but then we also have the means
> > to deal with devices which actually do that themselves (hello pl330...),
> > so that would seemingly only leave the case of some innocent piece of
> > AMBA-interfaced IP which doesn't expect to need special attributes, but
> > the system integrator has gone out of their way to tie the AxPROT bits
> > to some wacky value, which I would put in "erratum workaround" territory.
> >
> 
> You're correct Robin. It is an erratum workaround for the Laguna SoC
> due to some custom usage of the AxPROT bits which differs from the
> standard ARM SMMU handling for Privileged/Unprivileged and
> Instruction/Data transaction attributes. The effect is all
> transactions appear to the SMMU as "Privileged Instruction" accesses.

Ah, so this sounds like what Robin was worried about.

> The software workaround in this series enables the SMMU's INSTCFG
> override feature to ignore the incoming value and treat all SMMU
> transactions as "Data".
> 
> One small clarification: in the cover letter I incorrectly said this
> was set only for some SMMU IP instances, but that is incorrect. It is
> actually set on *all* arm-smmu-v3 IP instances in the Laguna SoC.
> 
> Does the above provide the additional detail you need Will?

So it sounds like using the instcfg override on this hardware still breaks
IOMMU_PRIV and IOMMU_NOEXEC:

  1. If you don't pass IOMMU_PRIV, you still get privileged transactions
  2. If you don't pass IOMMU_NOEXEC, you do not get execute permission

Is that correct?

Perhaps it would be better to override PRIVCFG to force unprivileged,
then reject IOMMU_PRIV and ignore IOMMU_NOEXEC?

Will


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

* Re: [PATCH 2/2] iommu/arm-smmu-v3: Override for Inst/Data attribute
  2026-08-07 14:37     ` Peter Griffin
  2026-08-07 16:43       ` Nicolin Chen
@ 2026-08-10 14:15       ` Jason Gunthorpe
  2026-08-10 14:43         ` Robin Murphy
  1 sibling, 1 reply; 16+ messages in thread
From: Jason Gunthorpe @ 2026-08-10 14:15 UTC (permalink / raw)
  To: Peter Griffin
  Cc: Nicolin Chen, Will Deacon, Robin Murphy, Joerg Roedel (AMD),
	Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Pranjal Shrivastava, Daniel Mentz, Mostafa Saleh,
	linux-arm-kernel, iommu, devicetree, linux-kernel, kernel-team,
	tudor.ambarus, andre.draszik, willmcvicker, jyescas

On Fri, Aug 07, 2026 at 03:37:37PM +0100, Peter Griffin wrote:

> > > @@ -1222,7 +1224,8 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
> > >       if (cfg & BIT(1)) {
> > >               used_bits[1] |=
> > >                       cpu_to_le64(STRTAB_STE_1_S2FWB | STRTAB_STE_1_EATS |
> > > -                                 STRTAB_STE_1_SHCFG | STRTAB_STE_1_MEV);
> > > +                                 STRTAB_STE_1_SHCFG | STRTAB_STE_1_MEV |
> > > +                                 STRTAB_STE_1_INSTCFG);
> > >               used_bits[2] |=
> > >                       cpu_to_le64(STRTAB_STE_2_S2VMID | STRTAB_STE_2_VTCR |
> > >                                   STRTAB_STE_2_S2AA64 | STRTAB_STE_2_S2ENDI |
> >
> > This adds for "stage-1" and "stage-2", while missing "bypass"?
> 
> I need to follow up again on this point, but I don't believe currently
> we require the override for Laguna when it's in bypass.

Oh? Why not?

Is the issue the IOPTEs are set to no-exec or something like that?

Jason


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

* Re: [PATCH 2/2] iommu/arm-smmu-v3: Override for Inst/Data attribute
  2026-08-10 14:15       ` Jason Gunthorpe
@ 2026-08-10 14:43         ` Robin Murphy
  0 siblings, 0 replies; 16+ messages in thread
From: Robin Murphy @ 2026-08-10 14:43 UTC (permalink / raw)
  To: Jason Gunthorpe, Peter Griffin
  Cc: Nicolin Chen, Will Deacon, Joerg Roedel (AMD), Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Pranjal Shrivastava,
	Daniel Mentz, Mostafa Saleh, linux-arm-kernel, iommu, devicetree,
	linux-kernel, kernel-team, tudor.ambarus, andre.draszik,
	willmcvicker, jyescas

On 10/08/2026 3:15 pm, Jason Gunthorpe wrote:
> On Fri, Aug 07, 2026 at 03:37:37PM +0100, Peter Griffin wrote:
> 
>>>> @@ -1222,7 +1224,8 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
>>>>        if (cfg & BIT(1)) {
>>>>                used_bits[1] |=
>>>>                        cpu_to_le64(STRTAB_STE_1_S2FWB | STRTAB_STE_1_EATS |
>>>> -                                 STRTAB_STE_1_SHCFG | STRTAB_STE_1_MEV);
>>>> +                                 STRTAB_STE_1_SHCFG | STRTAB_STE_1_MEV |
>>>> +                                 STRTAB_STE_1_INSTCFG);
>>>>                used_bits[2] |=
>>>>                        cpu_to_le64(STRTAB_STE_2_S2VMID | STRTAB_STE_2_VTCR |
>>>>                                    STRTAB_STE_2_S2AA64 | STRTAB_STE_2_S2ENDI |
>>>
>>> This adds for "stage-1" and "stage-2", while missing "bypass"?
>>
>> I need to follow up again on this point, but I don't believe currently
>> we require the override for Laguna when it's in bypass.
> 
> Oh? Why not?
> 
> Is the issue the IOPTEs are set to no-exec or something like that?

In general, there isn't expected to be anything in the memory system 
downstream of the SMMU that makes use of the Privileged or 
Instruction/Data attributes, so I would assume that is the case on this 
SoC. FWIW, hardware folks would be quite keen on relaxing the 
architecture to say that an SMMU isn't even required to propagate those 
attributes at all.

Thanks,
Robin.


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

* Re: [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature
  2026-08-10 11:00       ` Will Deacon
@ 2026-08-10 17:16         ` Robin Murphy
  0 siblings, 0 replies; 16+ messages in thread
From: Robin Murphy @ 2026-08-10 17:16 UTC (permalink / raw)
  To: Will Deacon, Peter Griffin
  Cc: Joerg Roedel (AMD), Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Pranjal Shrivastava, Daniel Mentz, Mostafa Saleh,
	linux-arm-kernel, iommu, devicetree, linux-kernel, kernel-team,
	tudor.ambarus, andre.draszik, willmcvicker, jyescas

On 10/08/2026 12:00 pm, Will Deacon wrote:
> On Fri, Aug 07, 2026 at 04:25:11PM +0100, Peter Griffin wrote:
>> On Mon, 27 Jul 2026 at 11:53, Robin Murphy <robin.murphy@arm.com> wrote:
>>>
>>> On 26/07/2026 2:16 pm, Will Deacon wrote:
>>>> On Fri, Jul 24, 2026 at 01:39:41PM +0100, Peter Griffin wrote:
>>>>> These two patches add support for a new "arm,instdata-override" DT property
>>>>> that enables the override of the instruction/data attribute of incoming
>>>>> traffic to Data by setting the INSTCFG override bits.
>>>>>
>>>>> It is intended to be specified when the smmu can't guarantee that these
>>>>> attributes are provided correctly from the client device.
>>>>
>>>> This is going to need an in-tree user and a much more detailed
>>>> description of what is being worked around before we consider this for
>>>> inclusion.
>>
>> Regarding an in-tree user, I haven't sent the Device Tree (DT) patch
>> yet for Laguna SoC which adds the smmu nodes and this property because
>> 1) I want to land the initial SoC/board DT first
>> 2) I want agreement on the DT property name. Currently I used
>> "arm,instdata-override" which is what downstream used. However, since
>> this is intended to work around silicon errata something like
>> "google,lga-instcfg-data-override" might be more appropriate?
>>
>> For Laguna SoC the first in-tree user of this is the amb_smmu smmu
>> instance which is used by the Synopsis dwc3 IP. The Laguna dwc3 glue
>> driver is already upstream at drivers/usb/dwc3/dwc3-google.c
>>
>>>>
>>>> In particular, if a particular client is emitting data reads as
>>>> instructions, then a better work around would be to avoid mapping its
>>>> domains using IOMMU_NOEXEC. But I can't tell what's going on from the
>>>> limited description provided here.
>>>
>>> Unless it's also emitting the privileged bit and thus falling foul of
>>> the implicit Unpriv-W -> Priv-XN rule, but then we also have the means
>>> to deal with devices which actually do that themselves (hello pl330...),
>>> so that would seemingly only leave the case of some innocent piece of
>>> AMBA-interfaced IP which doesn't expect to need special attributes, but
>>> the system integrator has gone out of their way to tie the AxPROT bits
>>> to some wacky value, which I would put in "erratum workaround" territory.
>>>
>>
>> You're correct Robin. It is an erratum workaround for the Laguna SoC
>> due to some custom usage of the AxPROT bits which differs from the
>> standard ARM SMMU handling for Privileged/Unprivileged and
>> Instruction/Data transaction attributes. The effect is all
>> transactions appear to the SMMU as "Privileged Instruction" accesses.
> 
> Ah, so this sounds like what Robin was worried about.
> 
>> The software workaround in this series enables the SMMU's INSTCFG
>> override feature to ignore the incoming value and treat all SMMU
>> transactions as "Data".
>>
>> One small clarification: in the cover letter I incorrectly said this
>> was set only for some SMMU IP instances, but that is incorrect. It is
>> actually set on *all* arm-smmu-v3 IP instances in the Laguna SoC.
>>
>> Does the above provide the additional detail you need Will?
> 
> So it sounds like using the instcfg override on this hardware still breaks
> IOMMU_PRIV and IOMMU_NOEXEC:
> 
>    1. If you don't pass IOMMU_PRIV, you still get privileged transactions

However this is inherently true of VMSA stage 1 anyway - I admit I had 
to double-check, but we don't have any user-only permissions (other than 
perhaps execute as implied by explicit or implicit PXN). IOMMU_PRIV can 
only _remove_ unprivileged access.

>    2. If you don't pass IOMMU_NOEXEC, you do not get execute permission
> 
> Is that correct?
> 
> Perhaps it would be better to override PRIVCFG to force unprivileged,
> then reject IOMMU_PRIV and ignore IOMMU_NOEXEC?

 From what Daniel said, it sounds like the privileged attribute is still 
the doing of the device itself rather than the integration issue in this 
case, so while nobbling PRIVCFG might also achieve the end result of 
making IOMMU_READ | IOMMU_WRITE pages mostly not fault on reads, it 
seems less appropriate as a workaround. Particularly since IOMMU_PRIV is 
exposed via a general DMA API attribute, while IOMMU_NOEXEC is only 
accessible to dedicated IOMMU API/io-pgtable users.

Thanks,
Robin.


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

* Re: [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature
  2026-08-07 22:47       ` Rob Herring
@ 2026-08-10 18:03         ` Robin Murphy
  0 siblings, 0 replies; 16+ messages in thread
From: Robin Murphy @ 2026-08-10 18:03 UTC (permalink / raw)
  To: Rob Herring, Peter Griffin
  Cc: Will Deacon, Joerg Roedel (AMD), Krzysztof Kozlowski,
	Conor Dooley, Pranjal Shrivastava, Daniel Mentz, Mostafa Saleh,
	linux-arm-kernel, iommu, devicetree, linux-kernel, kernel-team,
	tudor.ambarus, andre.draszik, willmcvicker, jyescas

On 07/08/2026 11:47 pm, Rob Herring wrote:
> On Fri, Aug 07, 2026 at 04:25:11PM +0100, Peter Griffin wrote:
>> Hi Will & Robin,
>>
>> Thank you both for your review feedback.
>>
>> On Mon, 27 Jul 2026 at 11:53, Robin Murphy <robin.murphy@arm.com> wrote:
>>>
>>> On 26/07/2026 2:16 pm, Will Deacon wrote:
>>>> On Fri, Jul 24, 2026 at 01:39:41PM +0100, Peter Griffin wrote:
>>>>> These two patches add support for a new "arm,instdata-override" DT property
>>>>> that enables the override of the instruction/data attribute of incoming
>>>>> traffic to Data by setting the INSTCFG override bits.
>>>>>
>>>>> It is intended to be specified when the smmu can't guarantee that these
>>>>> attributes are provided correctly from the client device.
>>>>
>>>> This is going to need an in-tree user and a much more detailed
>>>> description of what is being worked around before we consider this for
>>>> inclusion.
>>
>> Regarding an in-tree user, I haven't sent the Device Tree (DT) patch
>> yet for Laguna SoC which adds the smmu nodes and this property because
>> 1) I want to land the initial SoC/board DT first
>> 2) I want agreement on the DT property name. Currently I used
>> "arm,instdata-override" which is what downstream used. However, since
>> this is intended to work around silicon errata something like
>> "google,lga-instcfg-data-override" might be more appropriate?
> 
> Shrug.
> 
> Really, this is why we want SoC specific compatibles so work-arounds can
> be added without DT updates. But we've generally avoided doing that for
> SMMU for whatever reasons. Just make the property clearly sound like a
> mistake, not a feature you might want to use. I'd keep the arm prefix
> just so the next vendor thinks they can't use 'google' and creates the
> same property with their vendor prefix.

I dunno, I'd be inclined to keep it more vendor/SoC-specific and tied to 
the problem more than one (of potentially several) specific workaround - 
if we did ever want to support using the INSTCFG/PRIVCFG/MEMCFG 
overrides per their full architectural intent, they are per-StreamID 
controls so as such would really want to be encoded in an additional 
#iommu-cell (or at worst maybe a property on the client device node).

Frankly for a workaround condition to just blanket-apply to every SMMU 
instance in a particular SoC, I'm honestly thinking that just whacking 
an of_machine_is_compatible() in the driver isn't necessarily the worst 
idea either - after all we do already have the ACPI equivalent of that 
for some of the HiSilicon stuff, it's just hidden in the IORT code (and 
OK it's used for the SMMU PMUs rather than the actual SMMU devices 
themselves, but still...)

Thanks,
Robin.


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

end of thread, other threads:[~2026-08-10 18:03 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-24 12:39 [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature Peter Griffin
2026-07-24 12:39 ` [PATCH 1/2] dt-bindings: iommu: add arm,instdata-override property documentation Peter Griffin
2026-07-24 12:39 ` [PATCH 2/2] iommu/arm-smmu-v3: Override for Inst/Data attribute Peter Griffin
2026-07-24 18:04   ` Nicolin Chen
2026-08-07 14:37     ` Peter Griffin
2026-08-07 16:43       ` Nicolin Chen
2026-08-10 14:15       ` Jason Gunthorpe
2026-08-10 14:43         ` Robin Murphy
2026-07-26 13:16 ` [PATCH 0/2] Add arm-smmu-v3 support for instcfg data override feature Will Deacon
2026-07-27 10:53   ` Robin Murphy
2026-08-07 15:25     ` Peter Griffin
2026-08-07 18:21       ` Daniel Mentz
2026-08-07 22:47       ` Rob Herring
2026-08-10 18:03         ` Robin Murphy
2026-08-10 11:00       ` Will Deacon
2026-08-10 17:16         ` Robin Murphy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox