Skip to contentSkip to conversation

iommu/arm-smmu-v3: Fix SVA vs. HAFT issues

PATCH Superseded Robin Murphy 3 participants 9 messages Jul 23 – Jul 27

Subsystems (5)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Robin Murphy

author reviewer
#0 lore raw

v2 cover iommu/arm-smmu-v3: Fix SVA vs. HAFT issues

v1: https://lore.kernel.org/all/878cd6bcbbe2d5677d2f63da13294c148268552c.1782927917.git.robin.murphy@arm.com/

Per the discussion on v1, the aim here is just to make things safe and functionally correct for now, and now with a command-line option for distro users to get themselves out of a corner if they really need to, I think that's reasonable enough for HAFT being a very new feature.

Thanks,
Robin.

Robin Murphy (2):
arm64: Add override for MMFR1.HAFDBS
iommu/arm-smmu-v3: Add HAFT support for SVA

 Documentation/admin-guide/kernel-parameters.txt | 3 +++
 arch/arm64/kernel/pi/idreg-override.c           | 2 ++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 5 +++++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c     | 9 ++++++++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h     | 3 +++
 5 files changed, 21 insertions(+), 1 deletion(-)
Signature · 1 line
2.54.0.dirty
author reviewer
#1 lore raw

PATCH v2 1/2 arm64: Add override for MMFR1.HAFDBS

In general it might be nice to have the ability to disable hardware access/dirty bit management for debugging or performance comparison purposes without having to rebuild the kernel. However once FEAT_HAFT comes into the picture we also start to have a real functional concern where the decision to use HAFT based on the boot CPUs can prevent SVA or late-onlining if SMMUs/CPUs are later found to lack HAFT support.

To that end, add the appropriate MMFR1 override, with an easy "nohaft" alias for the significant case, partly since the feature/field naming isn't the most obvious, but also so it could potentially be redirected if someone wanted to attempt a higher-level means of turning off just HAFT usage independently from FEAT_HDBSS in future.

---

v2: New

OK, turns out this was actually incredibly easy, thanks Marc for making the framework so idiot-proof!

 Documentation/admin-guide/kernel-parameters.txt | 3 +++
 arch/arm64/kernel/pi/idreg-override.c           | 2 ++
 2 files changed, 5 insertions(+)
Documentation/admin-guide/kernel-parameters.txt +3 -0
index b5493a7f8f22..7f23e5b8dc44 100644
@@ -565,6 +565,9 @@ Kernel parameters
 	arm64.nogcs	[ARM64] Unconditionally disable Guarded Control Stack
 			support
 
+	arm64.nohaft	[ARM64] Unconditionally disable Hardware managed Access
+			Flag for Table descriptors support
+
 	arm64.nomops	[ARM64] Unconditionally disable Memory Copy and Memory
 			Set instructions support
 
arch/arm64/kernel/pi/idreg-override.c +2 -0
index bc57b290e5e7..0e051fec5afe 100644
@@ -64,6 +64,7 @@ static const struct ftr_set_desc mmfr1 __prel64_initconst = {
 	.override	= &id_aa64mmfr1_override,
 	.fields		= {
 		FIELD("vh", ID_AA64MMFR1_EL1_VH_SHIFT, mmfr1_vh_filter),
+		FIELD("hafdbs", ID_AA64MMFR1_EL1_HAFDBS_SHIFT, NULL),
 		{}
 	},
 };
@@ -246,6 +247,7 @@ static const struct {
 	{ "arm64.nomte",		"id_aa64pfr1.mte=0" },
 	{ "nokaslr",			"arm64_sw.nokaslr=1" },
 	{ "rodata=off",			"arm64_sw.rodataoff=1" },
+	{ "arm64.nohaft",		"id_aa64mmfr1.hafdbs=2" },
 	{ "arm64.nolva",		"id_aa64mmfr2.varange=0" },
 	{ "arm64.no32bit_el0",		"id_aa64pfr0.el0=1" },
 	{ "arm64.nompam",		"id_aa64pfr0.mpam=0 id_aa64pfr1.mpam_frac=0" },
Signature · 1 line
2.54.0.dirty
author reviewer
#2 lore raw

PATCH v2 2/2 iommu/arm-smmu-v3: Add HAFT support for SVA

In reply to Robin Murphy

Since table access flags cannot be software-managed, if process pagetables are using HAFT then SVA must require the SMMU to support and enable it too, otherwise page aging is liable to get out of whack.

Fixes 62df5870ebf7 ("arm64: Enable ARCH_HAS_NONLEAF_PMD_YOUNG")

---

v2: Update IORT override warning condition correctly

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 5 +++++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c     | 9 ++++++++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h     | 3 +++
 3 files changed, 16 insertions(+), 1 deletion(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +5 -0
index 1ed8a6f29dc4..ef11e9493f93 100644
@@ -102,6 +102,8 @@ void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,
 			target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HA);
 		if (master->smmu->features & ARM_SMMU_FEAT_HD)
 			target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HD);
+		if (master->smmu->features & ARM_SMMU_FEAT_HAFT && system_supports_haft())
+			target->data[1] |= cpu_to_le64(CTXDESC_CD_1_HAFT);
 	} else {
 		target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_EPD0);
 
@@ -211,6 +213,9 @@ bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
 	if (system_supports_bbml2_noabort())
 		feat_mask |= ARM_SMMU_FEAT_BBML2;
 
+	if (system_supports_haft())
+		feat_mask |= ARM_SMMU_FEAT_HAFT;
+
 	if ((smmu->features & feat_mask) != feat_mask)
 		return false;
 
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +8 -1
index a10affb483a4..14e5b3f08962 100644
@@ -4921,10 +4921,14 @@ static void arm_smmu_device_iidr_probe(struct arm_smmu_device *smmu)
 
 static void arm_smmu_get_httu(struct arm_smmu_device *smmu, u32 reg)
 {
-	u32 fw_features = smmu->features & (ARM_SMMU_FEAT_HA | ARM_SMMU_FEAT_HD);
+	u32 fw_features = smmu->features & (ARM_SMMU_FEAT_HA | ARM_SMMU_FEAT_HD |
+					    ARM_SMMU_FEAT_HAFT);
 	u32 hw_features = 0;
 
 	switch (FIELD_GET(IDR0_HTTU, reg)) {
+	case IDR0_HTTU_ACCESS_DIRTY_HAFT:
+		hw_features |= ARM_SMMU_FEAT_HAFT;
+		fallthrough;
 	case IDR0_HTTU_ACCESS_DIRTY:
 		hw_features |= ARM_SMMU_FEAT_HD;
 		fallthrough;
@@ -5256,6 +5260,9 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
 		smmu->features |= ARM_SMMU_FEAT_COHERENCY;
 
 	switch (FIELD_GET(ACPI_IORT_SMMU_V3_HTTU_OVERRIDE, iort_smmu->flags)) {
+	case IDR0_HTTU_ACCESS_DIRTY_HAFT:
+		smmu->features |= ARM_SMMU_FEAT_HAFT;
+		fallthrough;
 	case IDR0_HTTU_ACCESS_DIRTY:
 		smmu->features |= ARM_SMMU_FEAT_HD;
 		fallthrough;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +3 -0
index c909c9a88538..61a7df5afb99 100644
@@ -40,6 +40,7 @@ struct arm_vsmmu;
 #define IDR0_HTTU			GENMASK(7, 6)
 #define IDR0_HTTU_ACCESS		1
 #define IDR0_HTTU_ACCESS_DIRTY		2
+#define IDR0_HTTU_ACCESS_DIRTY_HAFT	3
 #define IDR0_COHACC			(1 << 4)
 #define IDR0_TTF			GENMASK(3, 2)
 #define IDR0_TTF_AARCH64		2
@@ -369,6 +370,7 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
 #define CTXDESC_CD_0_ASET		(1UL << 47)
 #define CTXDESC_CD_0_ASID		GENMASK_ULL(63, 48)
 
+#define CTXDESC_CD_1_HAFT		(1UL << 3)
 #define CTXDESC_CD_1_TTB0_MASK		GENMASK_ULL(51, 4)
 
 /*
@@ -921,6 +923,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_HAFT		(1 << 25)
 	u32				features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
Signature · 1 line
2.54.0.dirty
#3 lore raw

Re: [PATCH v2 2/2] iommu/arm-smmu-v3: Add HAFT support for SVA

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c void arm_smmu_make_sva_cd(struct arm_smmu_cd *target, · line 108

Quoted 10 lines · Robin Murphy
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +2 -0
@@ -102,6 +102,8 @@ void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,
 			target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HA);
 		if (master->smmu->features & ARM_SMMU_FEAT_HD)
 			target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HD);
+		if (master->smmu->features & ARM_SMMU_FEAT_HAFT && system_supports_haft())
+			target->data[1] |= cpu_to_le64(CTXDESC_CD_1_HAFT);
 	} else {
 		target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_EPD0);

Based on the Sashiko remark and your note you should lift all the HA/HD/HAFT bits outside this if (mm) block.

AFAICT it is fine for HA/HD/HAFT to be set with EPD0, the HW won't process any PTEs so they won't have any effect.

Since this avoids the BAD_CD inconsistency you pointed at and also avoids changing word 1 it should avoid the problematic V=0 transition.

Jason

maintainer
#4 lore raw

Re: [PATCH v2 2/2] iommu/arm-smmu-v3: Add HAFT support for SVA

Quoted 15 lines · Jason Gunthorpe

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c void arm_smmu_make_sva_cd(struct arm_smmu_cd *target, · line 108

Quoted 10 lines · Robin Murphy
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +2 -0
@@ -102,6 +102,8 @@ void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,
 			target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HA);
 		if (master->smmu->features & ARM_SMMU_FEAT_HD)
 			target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HD);
+		if (master->smmu->features & ARM_SMMU_FEAT_HAFT && system_supports_haft())
+			target->data[1] |= cpu_to_le64(CTXDESC_CD_1_HAFT);
 	} else {
 		target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_EPD0);

Based on the Sashiko remark and your note you should lift all the HA/HD/HAFT bits outside this if (mm) block.

Quoted 2 lines

AFAICT it is fine for HA/HD/HAFT to be set with EPD0, the HW won't process any PTEs so they won't have any effect.

I'm not sure about that. The spec says this about CD.HAFT:

| If CD.HA is 0 and not IGNORED, it is ILLEGAL to set this field to 1 | and this results in C_BAD_CD.

so the question is whether or not CD.HA is IGNORED when EPD0 is set, and I couldn't find anything to suggest that is the case (which kinda makes sense, given that there are two TTBs).

As a last resort, I tried to parse the pseudocode for C_BAD_CD (based on CdIllegal() in "5.4.2 Validity of CD")...

It looks like EPD0 gets wrapped up in 'n_transl_cfg0', but that's not used at all in the relevant check (you need to skip past the first few bits, but I've included them for completeness):

// Check CD.HA, CD.HD, CD.HAFT
if !using_vmsa32 &&

    // The following conditions are ILLEGAL when VMSAv8-64 or
    // when VMSAv9-128 are used.
    // No flag updates supported, but CD wants to update flag
    (((CD.HA == '1' || CD.HD == '1') && (SMMU_IDR0.HTTU == '00')) ||

    // Only access flag update suppported, but CD updates dirty flag
    (CD.HD == '1' && SMMU_IDR0.HTTU == '01') ||

    ** LOOK HERE! This is the relevant part: **

    // Hardware update of AF in Table descriptors is supported and enabled,
    // but update of AF in leaf descriptors is not enabled
    (SMMU_IDR0.HTTU == '11' && CD.HAFT == '1' && CD.HA == '0')) then

return TRUE; // Invalid

so I think your options are either:

1. Use the cmdline option that Robin is adding so that you can disable HAFT altogether.

- or -

2. We could disable HAFT first, and then do the hitless transition.

(2) means we can miss table AF updates during that window, but I think that should be fine?

Will

#5 lore raw

Re: [PATCH v2 2/2] iommu/arm-smmu-v3: Add HAFT support for SVA

Quoted 16 lines · Will Deacon
Quoted 15 lines · Jason Gunthorpe

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c void arm_smmu_make_sva_cd(struct arm_smmu_cd *target, · line 108

Quoted 10 lines · Robin Murphy
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +2 -0
@@ -102,6 +102,8 @@ void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,
 			target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HA);
 		if (master->smmu->features & ARM_SMMU_FEAT_HD)
 			target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HD);
+		if (master->smmu->features & ARM_SMMU_FEAT_HAFT && system_supports_haft())
+			target->data[1] |= cpu_to_le64(CTXDESC_CD_1_HAFT);
 	} else {
 		target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_EPD0);

Based on the Sashiko remark and your note you should lift all the HA/HD/HAFT bits outside this if (mm) block.

Quoted 7 lines
Quoted 2 lines

AFAICT it is fine for HA/HD/HAFT to be set with EPD0, the HW won't process any PTEs so they won't have any effect.

I'm not sure about that. The spec says this about CD.HAFT:

| If CD.HA is 0 and not IGNORED, it is ILLEGAL to set this field to 1 | and this results in C_BAD_CD.

Right, this is what Robin pointed to

Quoted 3 lines

so the question is whether or not CD.HA is IGNORED when EPD0 is set, and I couldn't find anything to suggest that is the case (which kinda makes sense, given that there are two TTBs).

Not quite, I said to lift all three bits out. So the spec question is if any HA/HD/HAFT have to be zero when EPD0 is set, and I think it does not say that (again it makes sense since there are two TTBs)?

Basically we want HA=1, HAFT=1, EPD0=1, TTBR0=0 as the "drop" configuration.

IOW I don't think there was any reason HA/HD were put in that branch in the first place.

Jason

#6 lore raw

Re: [PATCH v2 2/2] iommu/arm-smmu-v3: Add HAFT support for SVA

In reply to Will Deacon
Quoted 16 lines · Will Deacon
Quoted 15 lines · Jason Gunthorpe

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c void arm_smmu_make_sva_cd(struct arm_smmu_cd *target, · line 108

Quoted 10 lines · Robin Murphy
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +2 -0
@@ -102,6 +102,8 @@ void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,
 			target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HA);
 		if (master->smmu->features & ARM_SMMU_FEAT_HD)
 			target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HD);
+		if (master->smmu->features & ARM_SMMU_FEAT_HAFT && system_supports_haft())
+			target->data[1] |= cpu_to_le64(CTXDESC_CD_1_HAFT);
 	} else {
 		target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_EPD0);

Based on the Sashiko remark and your note you should lift all the HA/HD/HAFT bits outside this if (mm) block.

Quoted 4 lines
Quoted 2 lines

AFAICT it is fine for HA/HD/HAFT to be set with EPD0, the HW won't process any PTEs so they won't have any effect.

I'm not sure about that. The spec says this about CD.HAFT:

Sorry I see this was unclear

I ment to move all *three* bits outside the if.

There is no reason for EPD0 to also require setting any of those three bits to zero, AFAICT.

I agree with you we can't just put HAFT outside.

Jason

maintainer
#7 lore raw

Re: [PATCH v2 2/2] iommu/arm-smmu-v3: Add HAFT support for SVA

Quoted 20 lines · Jason Gunthorpe
Quoted 19 lines · Will Deacon
Quoted 17 lines · Jason Gunthorpe

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c void arm_smmu_make_sva_cd(struct arm_smmu_cd *target, · line 108

Quoted 10 lines · Robin Murphy
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +2 -0
@@ -102,6 +102,8 @@ void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,
 			target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HA);
 		if (master->smmu->features & ARM_SMMU_FEAT_HD)
 			target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HD);
+		if (master->smmu->features & ARM_SMMU_FEAT_HAFT && system_supports_haft())
+			target->data[1] |= cpu_to_le64(CTXDESC_CD_1_HAFT);
 	} else {
 		target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_EPD0);

Based on the Sashiko remark and your note you should lift all the HA/HD/HAFT bits outside this if (mm) block.

AFAICT it is fine for HA/HD/HAFT to be set with EPD0, the HW won't process any PTEs so they won't have any effect.

Quoted 8 lines
Quoted 1 line

I'm not sure about that. The spec says this about CD.HAFT:

Sorry I see this was unclear

I ment to move all *three* bits outside the if.

There is no reason for EPD0 to also require setting any of those three bits to zero, AFAICT.

Ah, gotcha! And even if we did want to get to that configuration, I guess you could set EPD0, then clear HAFT, then clear HA+HD?

Will

author reviewer
#8 lore raw

Re: [PATCH v2 2/2] iommu/arm-smmu-v3: Add HAFT support for SVA

Quoted 25 lines · Will Deacon
Quoted 24 lines · Jason Gunthorpe
Quoted 16 lines · Will Deacon
Quoted 15 lines · Jason Gunthorpe

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c void arm_smmu_make_sva_cd(struct arm_smmu_cd *target, · line 108

Quoted 10 lines · Robin Murphy
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +2 -0
@@ -102,6 +102,8 @@ void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,
  			target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HA);
  		if (master->smmu->features & ARM_SMMU_FEAT_HD)
  			target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HD);
+		if (master->smmu->features & ARM_SMMU_FEAT_HAFT && system_supports_haft())
+			target->data[1] |= cpu_to_le64(CTXDESC_CD_1_HAFT);
  	} else {
  		target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_EPD0);

Based on the Sashiko remark and your note you should lift all the HA/HD/HAFT bits outside this if (mm) block.

Quoted 4 lines
Quoted 2 lines

AFAICT it is fine for HA/HD/HAFT to be set with EPD0, the HW won't process any PTEs so they won't have any effect.

I'm not sure about that. The spec says this about CD.HAFT:

Sorry I see this was unclear

Quoted 7 lines
Quoted 4 lines

I ment to move all *three* bits outside the if.

There is no reason for EPD0 to also require setting any of those three bits to zero, AFAICT.

Ah, gotcha! And even if we did want to get to that configuration, I guess you could set EPD0, then clear HAFT, then clear HA+HD?

Indeed, it also occurred to me on the way home on Friday that in this case we probably could get away with just setting HA/HD/HAFT consistently in all SVA CDs irrespective of the mm. Strictly, EPD0 only forbids *new* walks from using TTB0, so there might potentially be some outstanding PTE updates still made in the window between the config update and the TLBI at the end of arm_smmu_mm_release(), but that shouldn't be harmful.

Lemme spin a v3...

Cheers,
Robin.