* [PATCH] iommu/arm-smmu-v3: Avoid ILLEGAL setting of STE.S1STALLD
@ 2017-09-14 5:08 Yisheng Xie
2017-09-19 11:43 ` Jean-Philippe Brucker
0 siblings, 1 reply; 3+ messages in thread
From: Yisheng Xie @ 2017-09-14 5:08 UTC (permalink / raw)
To: will.deacon, jean-philippe.brucker, robin.murphy, joro
Cc: linux-arm-kernel, iommu, linux-kernel, liubo95, chenjiankang1,
xieyisheng1
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
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)
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;
}
--
1.7.12.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] iommu/arm-smmu-v3: Avoid ILLEGAL setting of STE.S1STALLD
2017-09-14 5:08 [PATCH] iommu/arm-smmu-v3: Avoid ILLEGAL setting of STE.S1STALLD Yisheng Xie
@ 2017-09-19 11:43 ` Jean-Philippe Brucker
2017-09-20 10:18 ` Yisheng Xie
0 siblings, 1 reply; 3+ messages in thread
From: Jean-Philippe Brucker @ 2017-09-19 11:43 UTC (permalink / raw)
To: Yisheng Xie, Will Deacon, Robin Murphy, joro
Cc: linux-arm-kernel, iommu, linux-kernel, liubo95, chenjiankang1
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."
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.
> 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.
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;
> }
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] iommu/arm-smmu-v3: Avoid ILLEGAL setting of STE.S1STALLD
2017-09-19 11:43 ` Jean-Philippe Brucker
@ 2017-09-20 10:18 ` Yisheng Xie
0 siblings, 0 replies; 3+ messages in thread
From: Yisheng Xie @ 2017-09-20 10:18 UTC (permalink / raw)
To: Jean-Philippe Brucker, Will Deacon, Robin Murphy, joro
Cc: linux-arm-kernel, iommu, linux-kernel, liubo95, chenjiankang1
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;
>> }
>>
>>
>
>
> .
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-09-20 10:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-14 5:08 [PATCH] iommu/arm-smmu-v3: Avoid ILLEGAL setting of STE.S1STALLD Yisheng Xie
2017-09-19 11:43 ` Jean-Philippe Brucker
2017-09-20 10:18 ` Yisheng Xie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).