| Message ID | 20240816125901.3773388-1-smostafa@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] iommu/arm-smmu-v3: Match Stall behaviour for S2 | expand |
On Fri, Aug 16, 2024 at 12:59:01PM +0000, Mostafa Saleh wrote: > According to the spec (ARM IHI 0070 F.b), in > "5.5 Fault configuration (A, R, S bits)": > A STE with stage 2 translation enabled and STE.S2S == 0 is > considered ILLEGAL if SMMU_IDR0.STALL_MODEL == 0b10. > > Also described in the pseudocode “SteIllegal()” > if STE.Config == '11x' then > [..] > if eff_idr0_stall_model == '10' && STE.S2S == '0' then > // stall_model forcing stall, but S2S == 0 > return TRUE; > > Which means, S2S must be set when stall model is > "ARM_SMMU_FEAT_STALL_FORCE", but at the moment the driver ignores that. > > Although, the driver can do the minimum and only set S2S for > “ARM_SMMU_FEAT_STALL_FORCE”, it is more consistent to match S1 > behaviour, which also sets it for “ARM_SMMU_FEAT_STALL” if the > master has requested stalls. > > Also, since S2 stalls are enabled now, report them to the IOMMU layer > and for VFIO devices it will fail anyway as VFIO doesn’t register an > iopf handler. > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > > --- > v3: > - Set S2S for s2 and not s1 domain > - Ignore ats check > > v2: > - Fix index of the STE > - Fix conflict with ATS > - Squash the 2 patches and drop enable_nesting > --- > 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, 2 insertions(+), 4 deletions(-) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Fri, Aug 16, 2024 at 10:31:35AM -0300, Jason Gunthorpe wrote: > On Fri, Aug 16, 2024 at 12:59:01PM +0000, Mostafa Saleh wrote: > > According to the spec (ARM IHI 0070 F.b), in > > "5.5 Fault configuration (A, R, S bits)": > > A STE with stage 2 translation enabled and STE.S2S == 0 is > > considered ILLEGAL if SMMU_IDR0.STALL_MODEL == 0b10. > > > > Also described in the pseudocode “SteIllegal()” > > if STE.Config == '11x' then > > [..] > > if eff_idr0_stall_model == '10' && STE.S2S == '0' then > > // stall_model forcing stall, but S2S == 0 > > return TRUE; > > > > Which means, S2S must be set when stall model is > > "ARM_SMMU_FEAT_STALL_FORCE", but at the moment the driver ignores that. > > > > Although, the driver can do the minimum and only set S2S for > > “ARM_SMMU_FEAT_STALL_FORCE”, it is more consistent to match S1 > > behaviour, which also sets it for “ARM_SMMU_FEAT_STALL” if the > > master has requested stalls. > > > > Also, since S2 stalls are enabled now, report them to the IOMMU layer > > and for VFIO devices it will fail anyway as VFIO doesn’t register an > > iopf handler. > > > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > > > > --- > > v3: > > - Set S2S for s2 and not s1 domain > > - Ignore ats check > > > > v2: > > - Fix index of the STE > > - Fix conflict with ATS > > - Squash the 2 patches and drop enable_nesting > > --- > > 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, 2 insertions(+), 4 deletions(-) > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Actually this misses adding S2S to arm_smmu_get_ste_used() as it is a new bit :/ And that is not detected by “CONFIG_ARM_SMMU_V3_KUNIT_TEST” as it doesn’t test stall in masters, I will fix that and add coverage in tests for stall for both stages and respin. Thanks, Mostafa > > 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..d8fe16901afb 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -1646,6 +1646,7 @@ void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target, STRTAB_STE_2_S2ENDI | #endif STRTAB_STE_2_S2PTW | + (master->stall_enabled ? STRTAB_STE_2_S2S : 0) | STRTAB_STE_2_S2R); target->data[3] = cpu_to_le64(pgtbl_cfg->arm_lpae_s2_cfg.vttbr & @@ -1739,10 +1740,6 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt) return -EOPNOTSUPP; } - /* Stage-2 is always pinned at the moment */ - if (evt[1] & EVTQ_1_S2) - return -EFAULT; - if (!(evt[1] & EVTQ_1_STALL)) return -EOPNOTSUPP; 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)
According to the spec (ARM IHI 0070 F.b), in "5.5 Fault configuration (A, R, S bits)": A STE with stage 2 translation enabled and STE.S2S == 0 is considered ILLEGAL if SMMU_IDR0.STALL_MODEL == 0b10. Also described in the pseudocode “SteIllegal()” if STE.Config == '11x' then [..] if eff_idr0_stall_model == '10' && STE.S2S == '0' then // stall_model forcing stall, but S2S == 0 return TRUE; Which means, S2S must be set when stall model is "ARM_SMMU_FEAT_STALL_FORCE", but at the moment the driver ignores that. Although, the driver can do the minimum and only set S2S for “ARM_SMMU_FEAT_STALL_FORCE”, it is more consistent to match S1 behaviour, which also sets it for “ARM_SMMU_FEAT_STALL” if the master has requested stalls. Also, since S2 stalls are enabled now, report them to the IOMMU layer and for VFIO devices it will fail anyway as VFIO doesn’t register an iopf handler. Signed-off-by: Mostafa Saleh <smostafa@google.com> --- v3: - Set S2S for s2 and not s1 domain - Ignore ats check v2: - Fix index of the STE - Fix conflict with ATS - Squash the 2 patches and drop enable_nesting --- 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, 2 insertions(+), 4 deletions(-)