| Message ID | 20240812205255.97781-2-smostafa@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix handling of S2 stalls | expand |
On 12/08/2024 9:52 pm, Mostafa Saleh wrote: > S2S must be set when stall model is forced "ARM_SMMU_FEAT_STALL_FORCE". > But at the moment the driver ignores that, instead of doing the minimum > and only set S2S for “ARM_SMMU_FEAT_STALL_FORCE” we can just match what This was highly confusing, until the 3rd reading when I realised that maybe "instead of..." does not in fact belong to the description of the current behaviour, and it does start making sense if you swap the previous comma and full stop with each other. > S1 does which also set it for “ARM_SMMU_FEAT_STALL” and the master > has requested stalls. > This makes the driver more consistent when running on different SMMU > instances with different supported stages. > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 5 +++++ > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + > 2 files changed, 6 insertions(+) > > 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 a31460f9f3d4..8d573d9ca93c 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -1562,6 +1562,11 @@ void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target, > (cd_table->cdtab_dma & STRTAB_STE_0_S1CTXPTR_MASK) | > FIELD_PREP(STRTAB_STE_0_S1CDMAX, cd_table->s1cdmax)); > > + /* S2S is ignored if stage-2 exists but not enabled. */ > + if (master->stall_enabled && > + smmu->features & ARM_SMMU_FEAT_TRANS_S2) > + target->data[0] |= FIELD_PREP(STRTAB_STE_2_S2S, 1); In the middle of the ASID? Thanks, Robin. > + > target->data[1] = cpu_to_le64( > FIELD_PREP(STRTAB_STE_1_S1DSS, s1dss) | > FIELD_PREP(STRTAB_STE_1_S1CIR, STRTAB_STE_1_S1C_CACHE_WBRA) | > 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 14bca41a981b..0dc7ad43c64c 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > @@ -267,6 +267,7 @@ struct arm_smmu_ste { > #define STRTAB_STE_2_S2AA64 (1UL << 51) > #define STRTAB_STE_2_S2ENDI (1UL << 52) > #define STRTAB_STE_2_S2PTW (1UL << 54) > +#define STRTAB_STE_2_S2S (1UL << 57) > #define STRTAB_STE_2_S2R (1UL << 58) > > #define STRTAB_STE_3_S2TTB_MASK GENMASK_ULL(51, 4)
Hi Robin, On Tue, Aug 13, 2024 at 12:46:26PM +0100, Robin Murphy wrote: > On 12/08/2024 9:52 pm, Mostafa Saleh wrote: > > S2S must be set when stall model is forced "ARM_SMMU_FEAT_STALL_FORCE". > > But at the moment the driver ignores that, instead of doing the minimum > > and only set S2S for “ARM_SMMU_FEAT_STALL_FORCE” we can just match what > > This was highly confusing, until the 3rd reading when I realised that maybe > "instead of..." does not in fact belong to the description of the current > behaviour, and it does start making sense if you swap the previous comma and > full stop with each other. Will do, I will also reword it to make it more clear. > > > S1 does which also set it for “ARM_SMMU_FEAT_STALL” and the master > > has requested stalls. > > This makes the driver more consistent when running on different SMMU > > instances with different supported stages. > > > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > > --- > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 5 +++++ > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + > > 2 files changed, 6 insertions(+) > > > > 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 a31460f9f3d4..8d573d9ca93c 100644 > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > @@ -1562,6 +1562,11 @@ void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target, > > (cd_table->cdtab_dma & STRTAB_STE_0_S1CTXPTR_MASK) | > > FIELD_PREP(STRTAB_STE_0_S1CDMAX, cd_table->s1cdmax)); > > + /* S2S is ignored if stage-2 exists but not enabled. */ > > + if (master->stall_enabled && > > + smmu->features & ARM_SMMU_FEAT_TRANS_S2) > > + target->data[0] |= FIELD_PREP(STRTAB_STE_2_S2S, 1); > > In the middle of the ASID? Agh, of course it should be [2], sorry about that; it was late when I wrote the patch :) I will send a v2 with the fix. Thanks, Mostafa > > Thanks, > Robin. > > > + > > target->data[1] = cpu_to_le64( > > FIELD_PREP(STRTAB_STE_1_S1DSS, s1dss) | > > FIELD_PREP(STRTAB_STE_1_S1CIR, STRTAB_STE_1_S1C_CACHE_WBRA) | > > 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 14bca41a981b..0dc7ad43c64c 100644 > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > > @@ -267,6 +267,7 @@ struct arm_smmu_ste { > > #define STRTAB_STE_2_S2AA64 (1UL << 51) > > #define STRTAB_STE_2_S2ENDI (1UL << 52) > > #define STRTAB_STE_2_S2PTW (1UL << 54) > > +#define STRTAB_STE_2_S2S (1UL << 57) > > #define STRTAB_STE_2_S2R (1UL << 58) > > #define STRTAB_STE_3_S2TTB_MASK GENMASK_ULL(51, 4)
On Mon, Aug 12, 2024 at 08:52:54PM +0000, Mostafa Saleh wrote: > S2S must be set when stall model is forced "ARM_SMMU_FEAT_STALL_FORCE". > But at the moment the driver ignores that, instead of doing the minimum > and only set S2S for “ARM_SMMU_FEAT_STALL_FORCE” we can just match what > S1 does which also set it for “ARM_SMMU_FEAT_STALL” and the master > has requested stalls. > This makes the driver more consistent when running on different SMMU > instances with different supported stages. > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 5 +++++ > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + > 2 files changed, 6 insertions(+) > > 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 a31460f9f3d4..8d573d9ca93c 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -1562,6 +1562,11 @@ void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target, > (cd_table->cdtab_dma & STRTAB_STE_0_S1CTXPTR_MASK) | > FIELD_PREP(STRTAB_STE_0_S1CDMAX, cd_table->s1cdmax)); > > + /* S2S is ignored if stage-2 exists but not enabled. */ > + if (master->stall_enabled && > + smmu->features & ARM_SMMU_FEAT_TRANS_S2) > + target->data[0] |= FIELD_PREP(STRTAB_STE_2_S2S, 1); The style semes to be to just use target->data[] |= STRTAB_xx For single bit, not FIELD_PREP, even though it does work. And as Robin noted it is data[2], the naming is supposed to make that apparent with _2_ indicating the word.. But it is quite easy to make this typo. Jason
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 a31460f9f3d4..8d573d9ca93c 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -1562,6 +1562,11 @@ void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target, (cd_table->cdtab_dma & STRTAB_STE_0_S1CTXPTR_MASK) | FIELD_PREP(STRTAB_STE_0_S1CDMAX, cd_table->s1cdmax)); + /* S2S is ignored if stage-2 exists but not enabled. */ + if (master->stall_enabled && + smmu->features & ARM_SMMU_FEAT_TRANS_S2) + target->data[0] |= FIELD_PREP(STRTAB_STE_2_S2S, 1); + target->data[1] = cpu_to_le64( FIELD_PREP(STRTAB_STE_1_S1DSS, s1dss) | FIELD_PREP(STRTAB_STE_1_S1CIR, STRTAB_STE_1_S1C_CACHE_WBRA) | 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 14bca41a981b..0dc7ad43c64c 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -267,6 +267,7 @@ struct arm_smmu_ste { #define STRTAB_STE_2_S2AA64 (1UL << 51) #define STRTAB_STE_2_S2ENDI (1UL << 52) #define STRTAB_STE_2_S2PTW (1UL << 54) +#define STRTAB_STE_2_S2S (1UL << 57) #define STRTAB_STE_2_S2R (1UL << 58) #define STRTAB_STE_3_S2TTB_MASK GENMASK_ULL(51, 4)
S2S must be set when stall model is forced "ARM_SMMU_FEAT_STALL_FORCE". But at the moment the driver ignores that, instead of doing the minimum and only set S2S for “ARM_SMMU_FEAT_STALL_FORCE” we can just match what S1 does which also set it for “ARM_SMMU_FEAT_STALL” and the master has requested stalls. This makes the driver more consistent when running on different SMMU instances with different supported stages. Signed-off-by: Mostafa Saleh <smostafa@google.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 5 +++++ drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + 2 files changed, 6 insertions(+)