iommu/arm-smmu: Properly initialize CBAR MemAttr
PATCH 1/1 iommu/arm-smmu: Properly initialize CBAR MemAttr
Currently, when initializing the CBAR memattr attributes to the weakest values, it is expected that the final ones will be declared in the TTBCR register (SMMU_CBn_TCR).
This is not required when CBAR type consists of a stage 1 translation followed by a stage 2 bypass. This is the case when assigning a VFIO PCI device to a KVM guest. Overriding the default transaction attributes to writeback cacheable results in the device no longer working in the guest (the adapter requires explicit flushes on the descriptor rings memory).
Update the context init routine to initialize the CBAR MemAttr field only if there's a stage 1 followed by a stage 2 translation.
--- drivers/iommu/arm-smmu.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/iommu/arm-smmu.c +9 -4
@@ -765,13 +765,14 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, {u32 reg;u64 reg64; -bool stage1; +bool stage1, stage1_stage2;struct arm_smmu_cfg *cfg = &smmu_domain->cfg;struct arm_smmu_device *smmu = smmu_domain->smmu;void __iomem *cb_base, *gr1_base; gr1_base =ARM_SMMU_GR1 (smmu); stage1 = cfg->cbar !=CBAR_TYPE_S2_TRANS ; + stage1_stage2 = cfg->cbar ==CBAR_TYPE_S1_TRANS_S2_TRANS ; cb_base =ARM_SMMU_CB_BASE (smmu) +ARM_SMMU_CB (smmu, cfg->cbndx);if (smmu->version >ARM_SMMU_V1 ) {
@@ -793,15 +794,19 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,/* * Use the weakest shareability/memory types, so they are -* overridden by the ttbcr/pte. +* overridden by the ttbcr/pte. This happens only if the stage +* 1 is followed by a stage 2 translation. */ -if (stage1) { +if (stage1_stage2) { reg |= (CBAR_S1_BPSHCFG_NSH <<CBAR_S1_BPSHCFG_SHIFT ) | (CBAR_S1_MEMATTR_WB <<CBAR_S1_MEMATTR_SHIFT ); - }else if (!(smmu->features &ARM_SMMU_FEAT_VMID16 )) { + } + +if (!stage1 && !(smmu->features &ARM_SMMU_FEAT_VMID16 )) {/* 8-bit VMIDs live in CBAR */ reg |=ARM_SMMU_CB_VMID (smmu, cfg) <<CBAR_VMID_SHIFT ; } +writel_relaxed (reg, gr1_base +ARM_SMMU_GR1_CBAR (cfg->cbndx));/* TTBRs */
Signature · 1 line
1.9.1
Robin Murphy
reviewerPATCH 1/1 iommu/arm-smmu: Properly initialize CBAR MemAttr
Quoted 3 lines · Bogdan Purcareata
Currently, when initializing the CBAR memattr attributes to the weakest values, it is expected that the final ones will be declared in the TTBCR register (
SMMU_CBn_TCR).
You mean the stage 1 PTE, right? TTBCR only controls the attributes for table walks.
Quoted 2 lines
This is not required when CBAR type consists of a stage 1 translation followed by a stage 2 bypass.
On the contrary, this is _only_ required for stage 1 translation with stage 2 bypass - CBAR.MemAttr gives the _stage 2_ memory type (see section 2.4 "Memory type and shareability attribute determination" in the SMMU spec). For nested translation the stage 2 attributes come from the stage 2 context itself.
Quoted 4 lines
This is the case when assigning a VFIO PCI
device to a KVM guest. Overriding the default transaction attributes to writeback cacheable results in the device no longer working in the guest (the adapter requires explicit flushes on the descriptor rings memory).
From that, the real problem is almost certainly that you're erroneously
describing the device as coherent or non-coherent (whichever it actually isn't) somewhere.
Quoted 2 lines
Update the context init routine to initialize the CBAR MemAttr field only if there's a stage 1 followed by a stage 2 translation.
The CBAR.MemAttr field doesn't even exist in that format - it's a good thing that we don't actually implement nested translation (we currently just use a stage 2 context instead), because this would end up corrupting CBAR.CBNDX (i.e. the stage 2 context bank index). Now that I should probably fix...
Quoted 28 lines
Signed-off-by Bogdan Purcareata <[email protected]>--- drivers/iommu/arm-smmu.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)drivers/iommu/arm-smmu.c +2
-1@@ -765,13 +765,14 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, {u32 reg;u64 reg64; -bool stage1; +bool stage1, stage1_stage2;struct arm_smmu_cfg *cfg = &smmu_domain->cfg;struct arm_smmu_device *smmu = smmu_domain->smmu;void __iomem *cb_base, *gr1_base; gr1_base =ARM_SMMU_GR1 (smmu); stage1 = cfg->cbar !=CBAR_TYPE_S2_TRANS ; + stage1_stage2 = cfg->cbar ==CBAR_TYPE_S1_TRANS_S2_TRANS ; cb_base =ARM_SMMU_CB_BASE (smmu) +ARM_SMMU_CB (smmu, cfg->cbndx);if (smmu->version >ARM_SMMU_V1 ) {@@ -793,15 +794,19 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,/*
drivers/iommu/arm-smmu.c static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, · line 801
Quoted 10 lines
drivers/iommu/arm-smmu.c +3
-3* Use the weakest shareability/memory types, so they are -* overridden by the ttbcr/pte. +* overridden by the ttbcr/pte. This happens only if the stage +* 1 is followed by a stage 2 translation. */ -if (stage1) { +if (stage1_stage2) { reg |= (CBAR_S1_BPSHCFG_NSH <<CBAR_S1_BPSHCFG_SHIFT ) | (CBAR_S1_MEMATTR_WB <<CBAR_S1_MEMATTR_SHIFT ); - }else if (!(smmu->features &ARM_SMMU_FEAT_VMID16 )) {
Since MemAttr is initially zero, the net result of this is that *all* stage 1 transactions will now get overridden to Strongly-Ordered. That may hide your problem, but it's definitely not the correct thing to do.
Robin.
Quoted 11 lines
drivers/iommu/arm-smmu.c +4
-0+ } + +if (!stage1 && !(smmu->features &ARM_SMMU_FEAT_VMID16 )) {/* 8-bit VMIDs live in CBAR */ reg |=ARM_SMMU_CB_VMID (smmu, cfg) <<CBAR_VMID_SHIFT ; } +writel_relaxed (reg, gr1_base +ARM_SMMU_GR1_CBAR (cfg->cbndx));/* TTBRs */
PATCH 1/1 iommu/arm-smmu: Properly initialize CBAR MemAttr
Quoted 7 lines · Robin Murphy
Quoted 3 lines · Bogdan Purcareata
Currently, when initializing the CBAR memattr attributes to the weakest values, it is expected that the final ones will be declared in the TTBCR register (
SMMU_CBn_TCR).You mean the stage 1 PTE, right? TTBCR only controls the attributes for table walks.
I got lost in the SMMU spec. I couldn't find the direct correlation between CBAR and TTBCR. Thanks for the clarification.
Quoted 8 lines
Quoted 2 lines
This is not required when CBAR type consists of a stage 1 translation followed by a stage 2 bypass.
On the contrary, this is _only_ required for stage 1 translation with stage 2 bypass - CBAR.MemAttr gives the
_stage2_ memory type (see section 2.4 "Memory type and shareability attribute determination" in the SMMU spec). For nested translation the stage 2 attributes come from the stage 2 context itself.
Yes. In the previous "this", I was referring to the TTBCR configurations, not CBAR itself.
Quoted 8 lines
Quoted 4 lines
This is the case when assigning a VFIO PCI
device to a KVM guest. Overriding the default transaction attributes to writeback cacheable results in the device no longer working in the guest (the adapter requires explicit flushes on the descriptor rings memory).From that, the real problem is almost certainly that you're erroneously
describing the device as coherent or non-coherent (whichever it actually isn't) somewhere.
And that was the issue, indeed. The PCIe controller was not described as dma-coherent in the device tree, when in fact it was, thus wreaking all this havoc.
Thank you for the valuable insight and please discard the patch.
Bogdan P.
Quoted 66 lines
Quoted 2 lines
Update the context init routine to initialize the CBAR MemAttr field only if there's a stage 1 followed by a stage 2 translation.
The CBAR.MemAttr field doesn't even exist in that format - it's a good thing that we don't actually implement nested translation (we currently just use a stage 2 context instead), because this would end up corrupting CBAR.CBNDX (i.e. the stage 2 context bank index). Now that I should probably fix...
Quoted 28 lines
Signed-off-by Bogdan Purcareata <[email protected]>--- drivers/iommu/arm-smmu.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)drivers/iommu/arm-smmu.c +2
-1@@ -765,13 +765,14 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, {u32 reg;u64 reg64; -bool stage1; +bool stage1, stage1_stage2;struct arm_smmu_cfg *cfg = &smmu_domain->cfg;struct arm_smmu_device *smmu = smmu_domain->smmu;void __iomem *cb_base, *gr1_base; gr1_base =ARM_SMMU_GR1 (smmu); stage1 = cfg->cbar !=CBAR_TYPE_S2_TRANS ; + stage1_stage2 = cfg->cbar ==CBAR_TYPE_S1_TRANS_S2_TRANS ; cb_base =ARM_SMMU_CB_BASE (smmu) +ARM_SMMU_CB (smmu, cfg->cbndx);if (smmu->version >ARM_SMMU_V1 ) {@@ -793,15 +794,19 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,/* drivers/iommu/arm-smmu.c static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, · line 801
Quoted 10 lines
drivers/iommu/arm-smmu.c +3
-3* Use the weakest shareability/memory types, so they are -* overridden by the ttbcr/pte. +* overridden by the ttbcr/pte. This happens only if the stage +* 1 is followed by a stage 2 translation. */ -if (stage1) { +if (stage1_stage2) { reg |= (CBAR_S1_BPSHCFG_NSH <<CBAR_S1_BPSHCFG_SHIFT ) | (CBAR_S1_MEMATTR_WB <<CBAR_S1_MEMATTR_SHIFT ); - }else if (!(smmu->features &ARM_SMMU_FEAT_VMID16 )) {Since MemAttr is initially zero, the net result of this is that *all* stage 1 transactions will now get overridden to Strongly-Ordered. That may hide your problem, but it's definitely not the correct thing to do.
Robin.
Quoted 11 lines
drivers/iommu/arm-smmu.c +4
-0+ } + +if (!stage1 && !(smmu->features &ARM_SMMU_FEAT_VMID16 )) {/* 8-bit VMIDs live in CBAR */ reg |=ARM_SMMU_CB_VMID (smmu, cfg) <<CBAR_VMID_SHIFT ; } +writel_relaxed (reg, gr1_base +ARM_SMMU_GR1_CBAR (cfg->cbndx));/* TTBRs */