All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] iommu/arm-smmu-v3: Document SVA interaction with new pagetable features
@ 2024-12-05 13:48 Robin Murphy
  2024-12-05 13:48 ` [PATCH 2/2] arm64: cpufeature: Add GCS to cpucap_is_possible() Robin Murphy
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Robin Murphy @ 2024-12-05 13:48 UTC (permalink / raw)
  To: will, catalin.marinas
  Cc: joro, jean-philippe, iommu, linux-arm-kernel, mark.rutland,
	joey.gouly, ryan.roberts, broonie

Process pagetables may now be using new permission-indirection-based
features which an SMMU may not understand when given such a table for
SVA. Although SMMUv3.4 does add its own S1PIE feature, realistically
we're still going to have to cope with feature mismatches between CPUs
and SMMUs, so let's start simple and essentially just document the
expectations for what falls out as-is. Although it seems unlikely for
SVA applications to also depend on memory-hardening features, or
vice-versa, the relative lifecycles make it tricky to enforce mutual
exclusivity. Thankfully our PIE index allocation makes it relatively
benign for an SMMU to keep interpreting them as direct permissions, the
only real implication is that an SVA application cannot harden itself
against its own devices with these features. Thus, inform the user about
that just in case they have other expectations.

Also we don't (yet) support LPA2, so deny SVA entirely if we're going to
misunderstand the pagetable format altogether.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 1d3e71569775..9ba596430e7c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -112,6 +112,15 @@ void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,
 	 * from the current CPU register
 	 */
 	target->data[3] = cpu_to_le64(read_sysreg(mair_el1));
+
+	/*
+	 * Note that we don't bother with S1PIE on the SMMU, we just rely on
+	 * our default encoding scheme matching direct permissions anyway.
+	 * SMMU has no notion of S1POE nor GCS, so make sure that is clear if
+	 * either is enabled for CPUs, just in case anyone imagines otherwise.
+	 */
+	if (system_supports_poe() || system_supports_gcs())
+		dev_warn_once(master->smmu->dev, "SVA devices ignore permission overlays and GCS\n");
 }
 EXPORT_SYMBOL_IF_KUNIT(arm_smmu_make_sva_cd);
 
@@ -206,8 +215,12 @@ bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
 	unsigned long asid_bits;
 	u32 feat_mask = ARM_SMMU_FEAT_COHERENCY;
 
-	if (vabits_actual == 52)
+	if (vabits_actual == 52) {
+		/* We don't support LPA2 */
+		if (PAGE_SIZE != SZ_64K)
+			return false;
 		feat_mask |= ARM_SMMU_FEAT_VAX;
+	}
 
 	if ((smmu->features & feat_mask) != feat_mask)
 		return false;
-- 
2.39.2.101.g768bb238c484.dirty



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

* [PATCH 2/2] arm64: cpufeature: Add GCS to cpucap_is_possible()
  2024-12-05 13:48 [PATCH 1/2] iommu/arm-smmu-v3: Document SVA interaction with new pagetable features Robin Murphy
@ 2024-12-05 13:48 ` Robin Murphy
  2024-12-05 14:23   ` Mark Rutland
  2024-12-05 15:04   ` Mark Brown
  2024-12-05 18:14 ` [PATCH 1/2] iommu/arm-smmu-v3: Document SVA interaction with new pagetable features Jean-Philippe Brucker
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 12+ messages in thread
From: Robin Murphy @ 2024-12-05 13:48 UTC (permalink / raw)
  To: will, catalin.marinas
  Cc: joro, jean-philippe, iommu, linux-arm-kernel, mark.rutland,
	joey.gouly, ryan.roberts, broonie

Since system_supports_gcs() ends up referring to cpucap_is_possible(),
teach the latter about GCS for consistency with similar features.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 arch/arm64/include/asm/cpucaps.h    | 2 ++
 arch/arm64/include/asm/cpufeature.h | 3 +--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
index 201a46efd918..cbbf70e0f204 100644
--- a/arch/arm64/include/asm/cpucaps.h
+++ b/arch/arm64/include/asm/cpucaps.h
@@ -44,6 +44,8 @@ cpucap_is_possible(const unsigned int cap)
 		return IS_ENABLED(CONFIG_ARM64_TLB_RANGE);
 	case ARM64_HAS_S1POE:
 		return IS_ENABLED(CONFIG_ARM64_POE);
+	case ARM64_HAS_GCS:
+		return IS_ENABLED(CONFIG_ARM64_GCS);
 	case ARM64_UNMAP_KERNEL_AT_EL0:
 		return IS_ENABLED(CONFIG_UNMAP_KERNEL_AT_EL0);
 	case ARM64_WORKAROUND_843419:
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index b64e49bd9d10..8b4e5a3cd24c 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -847,8 +847,7 @@ static inline bool system_supports_poe(void)
 
 static inline bool system_supports_gcs(void)
 {
-	return IS_ENABLED(CONFIG_ARM64_GCS) &&
-		alternative_has_cap_unlikely(ARM64_HAS_GCS);
+	return alternative_has_cap_unlikely(ARM64_HAS_GCS);
 }
 
 static inline bool system_supports_haft(void)
-- 
2.39.2.101.g768bb238c484.dirty



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

* Re: [PATCH 2/2] arm64: cpufeature: Add GCS to cpucap_is_possible()
  2024-12-05 13:48 ` [PATCH 2/2] arm64: cpufeature: Add GCS to cpucap_is_possible() Robin Murphy
@ 2024-12-05 14:23   ` Mark Rutland
  2024-12-05 15:04   ` Mark Brown
  1 sibling, 0 replies; 12+ messages in thread
From: Mark Rutland @ 2024-12-05 14:23 UTC (permalink / raw)
  To: Robin Murphy
  Cc: will, catalin.marinas, joro, jean-philippe, iommu,
	linux-arm-kernel, joey.gouly, ryan.roberts, broonie

On Thu, Dec 05, 2024 at 01:48:10PM +0000, Robin Murphy wrote:
> Since system_supports_gcs() ends up referring to cpucap_is_possible(),
> teach the latter about GCS for consistency with similar features.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Looking around, we should probably do the same for HAFT.

Mark.

> ---
>  arch/arm64/include/asm/cpucaps.h    | 2 ++
>  arch/arm64/include/asm/cpufeature.h | 3 +--
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> index 201a46efd918..cbbf70e0f204 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -44,6 +44,8 @@ cpucap_is_possible(const unsigned int cap)
>  		return IS_ENABLED(CONFIG_ARM64_TLB_RANGE);
>  	case ARM64_HAS_S1POE:
>  		return IS_ENABLED(CONFIG_ARM64_POE);
> +	case ARM64_HAS_GCS:
> +		return IS_ENABLED(CONFIG_ARM64_GCS);
>  	case ARM64_UNMAP_KERNEL_AT_EL0:
>  		return IS_ENABLED(CONFIG_UNMAP_KERNEL_AT_EL0);
>  	case ARM64_WORKAROUND_843419:
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index b64e49bd9d10..8b4e5a3cd24c 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -847,8 +847,7 @@ static inline bool system_supports_poe(void)
>  
>  static inline bool system_supports_gcs(void)
>  {
> -	return IS_ENABLED(CONFIG_ARM64_GCS) &&
> -		alternative_has_cap_unlikely(ARM64_HAS_GCS);
> +	return alternative_has_cap_unlikely(ARM64_HAS_GCS);
>  }
>  
>  static inline bool system_supports_haft(void)
> -- 
> 2.39.2.101.g768bb238c484.dirty
> 


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

* Re: [PATCH 2/2] arm64: cpufeature: Add GCS to cpucap_is_possible()
  2024-12-05 13:48 ` [PATCH 2/2] arm64: cpufeature: Add GCS to cpucap_is_possible() Robin Murphy
  2024-12-05 14:23   ` Mark Rutland
@ 2024-12-05 15:04   ` Mark Brown
  2024-12-05 15:25     ` Catalin Marinas
  2024-12-05 15:40     ` Mark Rutland
  1 sibling, 2 replies; 12+ messages in thread
From: Mark Brown @ 2024-12-05 15:04 UTC (permalink / raw)
  To: Robin Murphy
  Cc: will, catalin.marinas, joro, jean-philippe, iommu,
	linux-arm-kernel, mark.rutland, joey.gouly, ryan.roberts

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

On Thu, Dec 05, 2024 at 01:48:10PM +0000, Robin Murphy wrote:
> Since system_supports_gcs() ends up referring to cpucap_is_possible(),
> teach the latter about GCS for consistency with similar features.

Not clear why this is part of a series, there's no obvious relationship
with patch 1?

>  static inline bool system_supports_gcs(void)
>  {
> -	return IS_ENABLED(CONFIG_ARM64_GCS) &&
> -		alternative_has_cap_unlikely(ARM64_HAS_GCS);
> +	return alternative_has_cap_unlikely(ARM64_HAS_GCS);
>  }

Ah, this is bitrot since the series was on the list for so long.  As
well as HAFT which Rutland mentioned it looks like _bti_kernel(),
_irq_prio_masking() and possibly others have the same thing.  Ideally
we'd have no uses of IS_ENABLED() in these functions so it's a bit more
obvious.  In any case

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH 2/2] arm64: cpufeature: Add GCS to cpucap_is_possible()
  2024-12-05 15:04   ` Mark Brown
@ 2024-12-05 15:25     ` Catalin Marinas
  2024-12-05 15:55       ` Robin Murphy
  2024-12-05 15:40     ` Mark Rutland
  1 sibling, 1 reply; 12+ messages in thread
From: Catalin Marinas @ 2024-12-05 15:25 UTC (permalink / raw)
  To: Mark Brown
  Cc: Robin Murphy, will, joro, jean-philippe, iommu, linux-arm-kernel,
	mark.rutland, joey.gouly, ryan.roberts

On Thu, Dec 05, 2024 at 03:04:11PM +0000, Mark Brown wrote:
> On Thu, Dec 05, 2024 at 01:48:10PM +0000, Robin Murphy wrote:
> > Since system_supports_gcs() ends up referring to cpucap_is_possible(),
> > teach the latter about GCS for consistency with similar features.
> 
> Not clear why this is part of a series, there's no obvious relationship
> with patch 1?

No, probably Robing forgot to pass --no-thread to git.

> >  static inline bool system_supports_gcs(void)
> >  {
> > -	return IS_ENABLED(CONFIG_ARM64_GCS) &&
> > -		alternative_has_cap_unlikely(ARM64_HAS_GCS);
> > +	return alternative_has_cap_unlikely(ARM64_HAS_GCS);
> >  }
> 
> Ah, this is bitrot since the series was on the list for so long.  As
> well as HAFT which Rutland mentioned it looks like _bti_kernel(),
> _irq_prio_masking() and possibly others have the same thing.  Ideally
> we'd have no uses of IS_ENABLED() in these functions so it's a bit more
> obvious.  In any case
> 
> Reviewed-by: Mark Brown <broonie@kernel.org>

Thanks. This patch will go in via the arm64 tree. I'll leave the other
for the smmu tree.

-- 
Catalin


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

* Re: [PATCH 2/2] arm64: cpufeature: Add GCS to cpucap_is_possible()
  2024-12-05 15:04   ` Mark Brown
  2024-12-05 15:25     ` Catalin Marinas
@ 2024-12-05 15:40     ` Mark Rutland
  2024-12-05 15:52       ` Mark Brown
  1 sibling, 1 reply; 12+ messages in thread
From: Mark Rutland @ 2024-12-05 15:40 UTC (permalink / raw)
  To: Mark Brown
  Cc: Robin Murphy, will, catalin.marinas, joro, jean-philippe, iommu,
	linux-arm-kernel, joey.gouly, ryan.roberts

On Thu, Dec 05, 2024 at 03:04:11PM +0000, Mark Brown wrote:
> On Thu, Dec 05, 2024 at 01:48:10PM +0000, Robin Murphy wrote:
> >  static inline bool system_supports_gcs(void)
> >  {
> > -	return IS_ENABLED(CONFIG_ARM64_GCS) &&
> > -		alternative_has_cap_unlikely(ARM64_HAS_GCS);
> > +	return alternative_has_cap_unlikely(ARM64_HAS_GCS);
> >  }
> 
> Ah, this is bitrot since the series was on the list for so long.  As
> well as HAFT which Rutland mentioned it looks like _bti_kernel(),
> _irq_prio_masking() and possibly others have the same thing.

In <asm/cpufeature.h>, only system_supports_gcs() and
system_supports_haft() don't use cpucap_is_possible(); all the other
IS_ENABLED() checks are for additional dependent properties, and rely on
the base case being handled in cpucap_is_possible().

In <asm/cpucaps.h> we have:

	static __always_inline bool
	cpucap_is_possible(const unsigned int cap)
	{
		...
		switch (cap) {
		...
		case ARM64_HAS_PAN:
			return IS_ENABLED(CONFIG_ARM64_PAN);
		...
		case ARM64_BTI:
			return IS_ENABLED(CONFIG_ARM64_BTI);
		...
		case ARM64_HAS_GIC_PRIO_MASKING:
			return IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI);
		...
		}
	}

In <asm/cpufeature.h> we have:

	// base case
	static inline bool system_uses_hw_pan(void)
	{
		return alternative_has_cap_unlikely(ARM64_HAS_PAN);
	}

	// additional dependent property
	static inline bool system_uses_ttbr0_pan(void)
	{
		return IS_ENABLED(CONFIG_ARM64_SW_TTBR0_PAN) &&
		       !system_uses_hw_pan();
	}

	...

	// base case
	static __always_inline bool system_uses_irq_prio_masking(void)
	{
		return alternative_has_cap_unlikely(ARM64_HAS_GIC_PRIO_MASKING);
	}

	// additional dependent property
	static inline bool system_has_prio_mask_debugging(void)
	{
		return IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
		       system_uses_irq_prio_masking();
	}

	...

	// base case
	static inline bool system_supports_bti(void)
	{
		return cpus_have_final_cap(ARM64_BTI);
	}

	// additional dependent property
	static inline bool system_supports_bti_kernel(void)
	{
		return IS_ENABLED(CONFIG_ARM64_BTI_KERNEL) &&
			cpus_have_final_boot_cap(ARM64_BTI);
	}

> Ideally we'd have no uses of IS_ENABLED() in these functions so it's a
> bit more obvious.

For the three cases above we can't do that without additional cpucaps,
and I think that'd be worse. I don't see anything we can practically do
other than add comments, and I suspect that's not going to help either.

Mark.


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

* Re: [PATCH 2/2] arm64: cpufeature: Add GCS to cpucap_is_possible()
  2024-12-05 15:40     ` Mark Rutland
@ 2024-12-05 15:52       ` Mark Brown
  0 siblings, 0 replies; 12+ messages in thread
From: Mark Brown @ 2024-12-05 15:52 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Robin Murphy, will, catalin.marinas, joro, jean-philippe, iommu,
	linux-arm-kernel, joey.gouly, ryan.roberts

[-- Attachment #1: Type: text/plain, Size: 384 bytes --]

On Thu, Dec 05, 2024 at 03:40:57PM +0000, Mark Rutland wrote:

> In <asm/cpufeature.h>, only system_supports_gcs() and
> system_supports_haft() don't use cpucap_is_possible(); all the other
> IS_ENABLED() checks are for additional dependent properties, and rely on
> the base case being handled in cpucap_is_possible().

Ah, yes - I got lost in indirections while checking.  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH 2/2] arm64: cpufeature: Add GCS to cpucap_is_possible()
  2024-12-05 15:25     ` Catalin Marinas
@ 2024-12-05 15:55       ` Robin Murphy
  0 siblings, 0 replies; 12+ messages in thread
From: Robin Murphy @ 2024-12-05 15:55 UTC (permalink / raw)
  To: Catalin Marinas, Mark Brown
  Cc: will, joro, jean-philippe, iommu, linux-arm-kernel, mark.rutland,
	joey.gouly, ryan.roberts

On 05/12/2024 3:25 pm, Catalin Marinas wrote:
> On Thu, Dec 05, 2024 at 03:04:11PM +0000, Mark Brown wrote:
>> On Thu, Dec 05, 2024 at 01:48:10PM +0000, Robin Murphy wrote:
>>> Since system_supports_gcs() ends up referring to cpucap_is_possible(),
>>> teach the latter about GCS for consistency with similar features.
>>
>> Not clear why this is part of a series, there's no obvious relationship
>> with patch 1?
> 
> No, probably Robing forgot to pass --no-thread to git.

Yeah, this was just a tangent from poking around figuring out how to 
detect GCS being enabled for the main patch.

>>>   static inline bool system_supports_gcs(void)
>>>   {
>>> -	return IS_ENABLED(CONFIG_ARM64_GCS) &&
>>> -		alternative_has_cap_unlikely(ARM64_HAS_GCS);
>>> +	return alternative_has_cap_unlikely(ARM64_HAS_GCS);
>>>   }
>>
>> Ah, this is bitrot since the series was on the list for so long.  As
>> well as HAFT which Rutland mentioned it looks like _bti_kernel(),
>> _irq_prio_masking() and possibly others have the same thing.  Ideally
>> we'd have no uses of IS_ENABLED() in these functions so it's a bit more
>> obvious.  In any case
>>
>> Reviewed-by: Mark Brown <broonie@kernel.org>
> 
> Thanks. This patch will go in via the arm64 tree. I'll leave the other
> for the smmu tree.

Thanks!

Robin.


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

* Re: [PATCH 1/2] iommu/arm-smmu-v3: Document SVA interaction with new pagetable features
  2024-12-05 13:48 [PATCH 1/2] iommu/arm-smmu-v3: Document SVA interaction with new pagetable features Robin Murphy
  2024-12-05 13:48 ` [PATCH 2/2] arm64: cpufeature: Add GCS to cpucap_is_possible() Robin Murphy
@ 2024-12-05 18:14 ` Jean-Philippe Brucker
  2024-12-06 12:03   ` Robin Murphy
  2024-12-05 18:23 ` (subset) " Catalin Marinas
  2024-12-10  0:17 ` Will Deacon
  3 siblings, 1 reply; 12+ messages in thread
From: Jean-Philippe Brucker @ 2024-12-05 18:14 UTC (permalink / raw)
  To: Robin Murphy
  Cc: will, catalin.marinas, joro, iommu, linux-arm-kernel,
	mark.rutland, joey.gouly, ryan.roberts, broonie

On Thu, Dec 05, 2024 at 01:48:09PM +0000, Robin Murphy wrote:
> Process pagetables may now be using new permission-indirection-based
> features which an SMMU may not understand when given such a table for
> SVA. Although SMMUv3.4 does add its own S1PIE feature, realistically
> we're still going to have to cope with feature mismatches between CPUs
> and SMMUs, so let's start simple and essentially just document the
> expectations for what falls out as-is. Although it seems unlikely for
> SVA applications to also depend on memory-hardening features, or
> vice-versa,

Hard to predict, but SVA could be indirectly enabled by some common
library: for example a libcrypto plugin that shares the address space with
an accelerator when one is available (see uadk_engine). I'm not familiar
with the POE use-cases, but I guess a common library could also enable
those memory-hardening features, such that unsuspecting applications have
everything enabled.

> the relative lifecycles make it tricky to enforce mutual
> exclusivity. Thankfully our PIE index allocation makes it relatively
> benign for an SMMU to keep interpreting them as direct permissions, the
> only real implication is that an SVA application cannot harden itself
> against its own devices with these features. Thus, inform the user about
> that just in case they have other expectations.
> 
> Also we don't (yet) support LPA2, so deny SVA entirely if we're going to
> misunderstand the pagetable format altogether.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> index 1d3e71569775..9ba596430e7c 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -112,6 +112,15 @@ void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,
>  	 * from the current CPU register
>  	 */
>  	target->data[3] = cpu_to_le64(read_sysreg(mair_el1));
> +
> +	/*
> +	 * Note that we don't bother with S1PIE on the SMMU, we just rely on
> +	 * our default encoding scheme matching direct permissions anyway.
> +	 * SMMU has no notion of S1POE nor GCS, so make sure that is clear if
> +	 * either is enabled for CPUs, just in case anyone imagines otherwise.
> +	 */
> +	if (system_supports_poe() || system_supports_gcs())
> +		dev_warn_once(master->smmu->dev, "SVA devices ignore permission overlays and GCS\n");

If POE tightens the page permissions, enabling SVA on top lets the
application easily bypass the POE protection. Could we actually return
false in arm_smmu_sva_supported() instead of displaying a warning that
likely no one will notice?

I guess enforcing !(SVA & POE) on an address space would be too much work?

Thanks,
Jean

>  }
>  EXPORT_SYMBOL_IF_KUNIT(arm_smmu_make_sva_cd);
>  
> @@ -206,8 +215,12 @@ bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
>  	unsigned long asid_bits;
>  	u32 feat_mask = ARM_SMMU_FEAT_COHERENCY;
>  
> -	if (vabits_actual == 52)
> +	if (vabits_actual == 52) {
> +		/* We don't support LPA2 */
> +		if (PAGE_SIZE != SZ_64K)
> +			return false;
>  		feat_mask |= ARM_SMMU_FEAT_VAX;
> +	}
>  
>  	if ((smmu->features & feat_mask) != feat_mask)
>  		return false;
> -- 
> 2.39.2.101.g768bb238c484.dirty
> 


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

* Re: (subset) [PATCH 1/2] iommu/arm-smmu-v3: Document SVA interaction with new pagetable features
  2024-12-05 13:48 [PATCH 1/2] iommu/arm-smmu-v3: Document SVA interaction with new pagetable features Robin Murphy
  2024-12-05 13:48 ` [PATCH 2/2] arm64: cpufeature: Add GCS to cpucap_is_possible() Robin Murphy
  2024-12-05 18:14 ` [PATCH 1/2] iommu/arm-smmu-v3: Document SVA interaction with new pagetable features Jean-Philippe Brucker
@ 2024-12-05 18:23 ` Catalin Marinas
  2024-12-10  0:17 ` Will Deacon
  3 siblings, 0 replies; 12+ messages in thread
From: Catalin Marinas @ 2024-12-05 18:23 UTC (permalink / raw)
  To: will, Robin Murphy
  Cc: joro, jean-philippe, iommu, linux-arm-kernel, mark.rutland,
	joey.gouly, ryan.roberts, broonie

On Thu, 05 Dec 2024 13:48:09 +0000, Robin Murphy wrote:
> Process pagetables may now be using new permission-indirection-based
> features which an SMMU may not understand when given such a table for
> SVA. Although SMMUv3.4 does add its own S1PIE feature, realistically
> we're still going to have to cope with feature mismatches between CPUs
> and SMMUs, so let's start simple and essentially just document the
> expectations for what falls out as-is. Although it seems unlikely for
> SVA applications to also depend on memory-hardening features, or
> vice-versa, the relative lifecycles make it tricky to enforce mutual
> exclusivity. Thankfully our PIE index allocation makes it relatively
> benign for an SMMU to keep interpreting them as direct permissions, the
> only real implication is that an SVA application cannot harden itself
> against its own devices with these features. Thus, inform the user about
> that just in case they have other expectations.
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[2/2] arm64: cpufeature: Add GCS to cpucap_is_possible()
      https://git.kernel.org/arm64/c/f00b53f1614f

-- 
Catalin



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

* Re: [PATCH 1/2] iommu/arm-smmu-v3: Document SVA interaction with new pagetable features
  2024-12-05 18:14 ` [PATCH 1/2] iommu/arm-smmu-v3: Document SVA interaction with new pagetable features Jean-Philippe Brucker
@ 2024-12-06 12:03   ` Robin Murphy
  0 siblings, 0 replies; 12+ messages in thread
From: Robin Murphy @ 2024-12-06 12:03 UTC (permalink / raw)
  To: Jean-Philippe Brucker
  Cc: will, catalin.marinas, joro, iommu, linux-arm-kernel,
	mark.rutland, joey.gouly, ryan.roberts, broonie

On 2024-12-05 6:14 pm, Jean-Philippe Brucker wrote:
> On Thu, Dec 05, 2024 at 01:48:09PM +0000, Robin Murphy wrote:
>> Process pagetables may now be using new permission-indirection-based
>> features which an SMMU may not understand when given such a table for
>> SVA. Although SMMUv3.4 does add its own S1PIE feature, realistically
>> we're still going to have to cope with feature mismatches between CPUs
>> and SMMUs, so let's start simple and essentially just document the
>> expectations for what falls out as-is. Although it seems unlikely for
>> SVA applications to also depend on memory-hardening features, or
>> vice-versa,
> 
> Hard to predict, but SVA could be indirectly enabled by some common
> library: for example a libcrypto plugin that shares the address space with
> an accelerator when one is available (see uadk_engine). I'm not familiar
> with the POE use-cases, but I guess a common library could also enable
> those memory-hardening features, such that unsuspecting applications have
> everything enabled.
> 
>> the relative lifecycles make it tricky to enforce mutual
>> exclusivity. Thankfully our PIE index allocation makes it relatively
>> benign for an SMMU to keep interpreting them as direct permissions, the
>> only real implication is that an SVA application cannot harden itself
>> against its own devices with these features. Thus, inform the user about
>> that just in case they have other expectations.
>>
>> Also we don't (yet) support LPA2, so deny SVA entirely if we're going to
>> misunderstand the pagetable format altogether.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> index 1d3e71569775..9ba596430e7c 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> @@ -112,6 +112,15 @@ void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,
>>   	 * from the current CPU register
>>   	 */
>>   	target->data[3] = cpu_to_le64(read_sysreg(mair_el1));
>> +
>> +	/*
>> +	 * Note that we don't bother with S1PIE on the SMMU, we just rely on
>> +	 * our default encoding scheme matching direct permissions anyway.
>> +	 * SMMU has no notion of S1POE nor GCS, so make sure that is clear if
>> +	 * either is enabled for CPUs, just in case anyone imagines otherwise.
>> +	 */
>> +	if (system_supports_poe() || system_supports_gcs())
>> +		dev_warn_once(master->smmu->dev, "SVA devices ignore permission overlays and GCS\n");
> 
> If POE tightens the page permissions, enabling SVA on top lets the
> application easily bypass the POE protection. Could we actually return
> false in arm_smmu_sva_supported() instead of displaying a warning that
> likely no one will notice?

The difficulty with both features is that they're always unconditionally 
*enabled* if present, but the SVA concern only arises if and when they 
are *actively used*, and that can be a transient thing. It doesn't seem 
reasonable to effectively make SVA unusable on newer CPUs (or at least 
force users to boot with MMFR overrides to lose GCS/POE if they want to 
use SVA), especially when it's far from clear how bothered anyone's 
really going to be about this anyway. I just think we should do at least 
*something* to remember and acknowledge that certain CPU features impact 
SVA too.

> I guess enforcing !(SVA & POE) on an address space would be too much work?

Yes, to attempt to do this "properly" I believe we'd need to keep track 
of how many SVA attachments, overlays, and GCS pages (if the SMMU can't 
support S1PIE) a process has at any given point in time, then check 
those in all 3 places to deny individual usage calls if the "opposing" 
feature is currently in use.

Thanks,
Robin.

> 
> Thanks,
> Jean
> 
>>   }
>>   EXPORT_SYMBOL_IF_KUNIT(arm_smmu_make_sva_cd);
>>   
>> @@ -206,8 +215,12 @@ bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
>>   	unsigned long asid_bits;
>>   	u32 feat_mask = ARM_SMMU_FEAT_COHERENCY;
>>   
>> -	if (vabits_actual == 52)
>> +	if (vabits_actual == 52) {
>> +		/* We don't support LPA2 */
>> +		if (PAGE_SIZE != SZ_64K)
>> +			return false;
>>   		feat_mask |= ARM_SMMU_FEAT_VAX;
>> +	}
>>   
>>   	if ((smmu->features & feat_mask) != feat_mask)
>>   		return false;
>> -- 
>> 2.39.2.101.g768bb238c484.dirty
>>



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

* Re: [PATCH 1/2] iommu/arm-smmu-v3: Document SVA interaction with new pagetable features
  2024-12-05 13:48 [PATCH 1/2] iommu/arm-smmu-v3: Document SVA interaction with new pagetable features Robin Murphy
                   ` (2 preceding siblings ...)
  2024-12-05 18:23 ` (subset) " Catalin Marinas
@ 2024-12-10  0:17 ` Will Deacon
  3 siblings, 0 replies; 12+ messages in thread
From: Will Deacon @ 2024-12-10  0:17 UTC (permalink / raw)
  To: catalin.marinas, Robin Murphy
  Cc: kernel-team, Will Deacon, joro, jean-philippe, iommu,
	linux-arm-kernel, mark.rutland, joey.gouly, ryan.roberts, broonie

On Thu, 05 Dec 2024 13:48:09 +0000, Robin Murphy wrote:
> Process pagetables may now be using new permission-indirection-based
> features which an SMMU may not understand when given such a table for
> SVA. Although SMMUv3.4 does add its own S1PIE feature, realistically
> we're still going to have to cope with feature mismatches between CPUs
> and SMMUs, so let's start simple and essentially just document the
> expectations for what falls out as-is. Although it seems unlikely for
> SVA applications to also depend on memory-hardening features, or
> vice-versa, the relative lifecycles make it tricky to enforce mutual
> exclusivity. Thankfully our PIE index allocation makes it relatively
> benign for an SMMU to keep interpreting them as direct permissions, the
> only real implication is that an SVA application cannot harden itself
> against its own devices with these features. Thus, inform the user about
> that just in case they have other expectations.
> 
> [...]

Applied SMMU change to will (for-joerg/arm-smmu/updates), thanks!

[1/2] iommu/arm-smmu-v3: Document SVA interaction with new pagetable features
      https://git.kernel.org/will/c/6e192214c6c8

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev


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

end of thread, other threads:[~2024-12-10  0:22 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-05 13:48 [PATCH 1/2] iommu/arm-smmu-v3: Document SVA interaction with new pagetable features Robin Murphy
2024-12-05 13:48 ` [PATCH 2/2] arm64: cpufeature: Add GCS to cpucap_is_possible() Robin Murphy
2024-12-05 14:23   ` Mark Rutland
2024-12-05 15:04   ` Mark Brown
2024-12-05 15:25     ` Catalin Marinas
2024-12-05 15:55       ` Robin Murphy
2024-12-05 15:40     ` Mark Rutland
2024-12-05 15:52       ` Mark Brown
2024-12-05 18:14 ` [PATCH 1/2] iommu/arm-smmu-v3: Document SVA interaction with new pagetable features Jean-Philippe Brucker
2024-12-06 12:03   ` Robin Murphy
2024-12-05 18:23 ` (subset) " Catalin Marinas
2024-12-10  0:17 ` Will Deacon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.