Hi Jean-Philippe, Thanks for reviewing. On 2017/9/19 19:43, Jean-Philippe Brucker wrote: > On 14/09/17 06:08, Yisheng Xie wrote: >> According to Spec, it is ILLEGAL to set STE.S1STALLD if STALL_MODEL >> is not 0b00, which means we should not disable stall mode if stall >> or terminate mode is not configuable. >> >> As Jean-Philippe's suggestion, this patch introduce a feature bit >> ARM_SMMU_FEAT_STALL_FORCE, which means smmu only supports stall force. >> Therefore, we can avoid the ILLEGAL setting of STE.S1STALLD.by checking >> ARM_SMMU_FEAT_STALL_FORCE. >> >> This patch keeps the ARM_SMMU_FEAT_STALLS as the meaning of stall supported >> (force or configuable) to easy to expand the future function, i.e. we can >> only use ARM_SMMU_FEAT_STALLS to check whether we should register fault >> handle or enable master can_stall, etc to supporte platform SVM. >> >> After apply this patch, the feature bit and S1STALLD setting will be like: >> STALL_MODEL FEATURE S1STALLD >> 0b00 ARM_SMMU_FEAT_STALLS 0b1 >> 0b01 !ARM_SMMU_FEAT_STALLS && !ARM_SMMU_FEAT_STALL_FORCE 0b0 >> 0b10 ARM_SMMU_FEAT_STALLS && ARM_SMMU_FEAT_STALL_FORCE 0b0 > > Thanks for the patch. Since it's the same problem, could you also fix the > context descriptor value? The spec says, in 5.5 Fault configuration: > > "A CD (Stage 1 translation enabled) is considered ILLEGAL if one of the > following applies: > * SMMU_(S_)IDR0.STALL_MODEL == 0b10 and CD.S == 0." Sure, I will fix this it in next version.
> > So I think we should always set CD.S if the SMMU has STALL_FORCE. > > As Will pointed out, more work is needed for STALL_FORCE. We can't enable > translation at all for devices that don't support stalling (e.g. PCI). We > should force them into bypass or abort mode depending on the config. Maybe > we can fix that later, after the devicetree property is added. It is fare. > >> Signed-off-by: Yisheng Xie <xieyishe...@huawei.com> >> --- >> drivers/iommu/arm-smmu-v3.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >> index e67ba6c..d2a3627 100644 >> --- a/drivers/iommu/arm-smmu-v3.c >> +++ b/drivers/iommu/arm-smmu-v3.c >> @@ -603,7 +603,8 @@ struct arm_smmu_device { >> #define ARM_SMMU_FEAT_TRANS_S1 (1 << 9) >> #define ARM_SMMU_FEAT_TRANS_S2 (1 << 10) >> #define ARM_SMMU_FEAT_STALLS (1 << 11) >> -#define ARM_SMMU_FEAT_HYP (1 << 12) >> +#define ARM_SMMU_FEAT_STALL_FORCE (1 << 12) >> +#define ARM_SMMU_FEAT_HYP (1 << 13) > > We probably should keep the feature bits backward compatible and only add > new ones at the end. It's not ABI, but it's printed at boot time and I > sometimes use them when inspecting the kernel output to see what an SMMU > supports. Take it, also will fixed it in next version. Thanks Yisheng Xie > > Thanks, > Jean > >> u32 features; >> >> #define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0) >> @@ -1112,7 +1113,8 @@ static void arm_smmu_write_strtab_ent(struct >> arm_smmu_device *smmu, u32 sid, >> #endif >> STRTAB_STE_1_STRW_NSEL1 << STRTAB_STE_1_STRW_SHIFT); >> >> - if (smmu->features & ARM_SMMU_FEAT_STALLS) >> + if (smmu->features & ARM_SMMU_FEAT_STALLS && >> + !(smmu->features & ARM_SMMU_FEAT_STALL_FORCE)) >> dst[1] |= cpu_to_le64(STRTAB_STE_1_S1STALLD); >> >> val |= (ste->s1_cfg->cdptr_dma & STRTAB_STE_0_S1CTXPTR_MASK >> @@ -2536,9 +2538,10 @@ static int arm_smmu_device_hw_probe(struct >> arm_smmu_device *smmu) >> coherent ? "true" : "false"); >> >> switch (reg & IDR0_STALL_MODEL_MASK << IDR0_STALL_MODEL_SHIFT) { >> - case IDR0_STALL_MODEL_STALL: >> - /* Fallthrough */ >> case IDR0_STALL_MODEL_FORCE: >> + smmu->features |= ARM_SMMU_FEAT_STALL_FORCE; >> + /* Fallthrough */ >> + case IDR0_STALL_MODEL_STALL: >> smmu->features |= ARM_SMMU_FEAT_STALLS; >> } >> >> > > > . >