On 11/03/2025 8:00 pm, Rob Clark wrote:
On Tue, Mar 11, 2025 at 12:42 PM Connor Abbott <cwabbott0@xxxxxxxxx> wrote:
On Tue, Mar 11, 2025 at 2:08 PM Will Deacon <will@xxxxxxxxxx> wrote:
On Tue, Mar 04, 2025 at 11:56:48AM -0500, Connor Abbott wrote:
In some cases drm/msm has to resume a stalled transaction directly in
its fault handler. Experimentally this doesn't work on SMMU500 if the
fault hasn't already been acknowledged by clearing FSR. Rather than
trying to clear FSR in msm's fault handler and implementing a
tricky handshake to avoid accidentally clearing FSR twice, we want to
clear FSR before calling the fault handlers, but this means that the
contents of registers can change underneath us in the fault handler and
msm currently uses a private function to read the register contents for
its own purposes in its fault handler, such as using the
implementation-defined FSYNR1 to determine which block caused the fault.
Fix this by making msm use the register values already read by arm-smmu
itself before clearing FSR rather than messing around with reading
registers directly.
Signed-off-by: Connor Abbott <cwabbott0@xxxxxxxxx>
---
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 19 +++++++++----------
drivers/iommu/arm/arm-smmu/arm-smmu.c | 14 +++++++-------
drivers/iommu/arm/arm-smmu/arm-smmu.h | 21 +++++++++++----------
3 files changed, 27 insertions(+), 27 deletions(-)
[...]
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
index d3bc77dcd4d40f25bc70f3289616fb866649b022..411d807e0a7033833716635efb3968a0bd3ff237 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
@@ -373,6 +373,16 @@ enum arm_smmu_domain_stage {
ARM_SMMU_DOMAIN_NESTED,
};
+struct arm_smmu_context_fault_info {
+ unsigned long iova;
+ u64 ttbr0;
+ u32 fsr;
+ u32 fsynr0;
+ u32 fsynr1;
+ u32 cbfrsynra;
+ u32 contextidr;
+};
+
struct arm_smmu_domain {
struct arm_smmu_device *smmu;
struct io_pgtable_ops *pgtbl_ops;
@@ -380,6 +390,7 @@ struct arm_smmu_domain {
const struct iommu_flush_ops *flush_ops;
struct arm_smmu_cfg cfg;
enum arm_smmu_domain_stage stage;
+ struct arm_smmu_context_fault_info cfi;
Does this mean we have to serialise all faults for a given domain? That
can't be right...
Will
They are already serialized? There's only one of each register per
context bank, so you can only have one context fault at a time per
context bank, and AFAIK a context bank is 1:1 with a domain. Also this
struct is only written and then read inside the context bank's
interrupt handler, and you can only have one interrupt at a time, no?
Connor
And if it was a race condition with cfi getting overridden, it would
have already been an equivalent race condition currently when reading
the values from registers (ie. the register values could have changed
in the elapsed time)
So no additional serialization needed here.
Agreed, the only subtlety is the other side of things is true as well -
i.e. cfi is only valid and stable between the IRQ being fired and
CBn_RESUME being written, so actually it *mustn't* be touched by
anything outside the fault handling path.
Thanks,
Robin.
