On Tue, Mar 11, 2025 at 12:42 PM Connor Abbott <cwabbo...@gmail.com> wrote:
>
> On Tue, Mar 11, 2025 at 2:08 PM Will Deacon <w...@kernel.org> 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 <cwabbo...@gmail.com>
> > > ---
> > > 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.
BR,
-R
