iommu/arm-smmu-v3: Fix SVA vs. HAFT issues
Subsystems (5)
Robin Murphy
reviewerv2 cover iommu/arm-smmu-v3: Fix SVA vs. HAFT issues
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
Robin Murphy
reviewerPATCH 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
@@ -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
@@ -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
Robin Murphy
reviewerPATCH v2 2/2 iommu/arm-smmu-v3: Add HAFT support for SVA
In reply to Robin MurphySince 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.
---
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
@@ -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
@@ -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
@@ -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
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
Will Deacon
maintainerRe: [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')) thenreturn 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
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
Re: [PATCH v2 2/2] iommu/arm-smmu-v3: Add HAFT support for SVA
In reply to Will DeaconQuoted 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
Will Deacon
maintainerRe: [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
Robin Murphy
reviewerRe: [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.