lkml.org 
[lkml]   [2017]   [Sep]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] iommu/arm-smmu-v3: Avoid ILLEGAL setting of STE.S1STALLD
From
Date
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 <xieyisheng1@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;
>> }
>>
>>
>
>
> .
>

\
 
 \ /
  Last update: 2017-09-20 12:19    [W:0.090 / U:1.444 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site