On Wed, Mar 12, 2025 at 8:49 AM Will Deacon <w...@kernel.org> wrote:
>
> On Tue, Mar 11, 2025 at 04:01:00PM -0400, Connor Abbott wrote:
> > On Tue, Mar 11, 2025 at 2:11 PM Will Deacon <w...@kernel.org> wrote:
> > >
> > > On Tue, Mar 04, 2025 at 11:56:50AM -0500, Connor Abbott wrote:
> > > > Up until now we have only called the set_stall callback during
> > > > initialization when the device is off. But we will soon start calling it
> > > > to temporarily disable stall-on-fault when the device is on, so handle
> > > > that by checking if the device is on and writing SCTLR.
> > > >
> > > > Signed-off-by: Connor Abbott <cwabbo...@gmail.com>
> > > > ---
> > > > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 30
> > > > +++++++++++++++++++++++++++---
> > > > 1 file changed, 27 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > > b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > > index
> > > > a428e53add08d451fb2152e3ab80e0fba936e214..d34a0d917013bb3d5a24b3ce72f48e3b38474da2
> > > > 100644
> > > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > > @@ -77,12 +77,36 @@ static void qcom_adreno_smmu_set_stall(const void
> > > > *cookie, bool enabled)
> > > > {
> > > > struct arm_smmu_domain *smmu_domain = (void *)cookie;
> > > > struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> > > > - struct qcom_smmu *qsmmu = to_qcom_smmu(smmu_domain->smmu);
> > > > + struct arm_smmu_device *smmu = smmu_domain->smmu;
> > > > + struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
> > > > + u32 mask = BIT(cfg->cbndx);
> > > > + bool stall_changed = !!(qsmmu->stall_enabled & mask) != enabled;
> > > > + unsigned long flags;
> > > >
> > > > if (enabled)
> > > > - qsmmu->stall_enabled |= BIT(cfg->cbndx);
> > > > + qsmmu->stall_enabled |= mask;
> > > > else
> > > > - qsmmu->stall_enabled &= ~BIT(cfg->cbndx);
> > > > + qsmmu->stall_enabled &= ~mask;
> > > > +
> > > > + /*
> > > > + * If the device is on and we changed the setting, update the
> > > > register.
> > > > + */
> > > > + if (stall_changed && pm_runtime_get_if_active(smmu->dev) > 0) {
> > > > + spin_lock_irqsave(&smmu_domain->cb_lock, flags);
> > > > +
> > > > + u32 reg = arm_smmu_cb_read(smmu, cfg->cbndx,
> > > > ARM_SMMU_CB_SCTLR);
> > > > +
> > > > + if (enabled)
> > > > + reg |= ARM_SMMU_SCTLR_CFCFG;
> > > > + else
> > > > + reg &= ~ARM_SMMU_SCTLR_CFCFG;
> > > > +
> > > > + arm_smmu_cb_write(smmu, cfg->cbndx, ARM_SMMU_CB_SCTLR,
> > > > reg);
> > >
> > > Are you sure you don't need TLB invalidation for this to take effect? I
> > > think some fields in the SCTLR can be cached in the TLB but you'll need
> > > to check whether or not that applies to CFCFG.
> > >
> >
> > I think it should be fine because CFCFG only controls behavior when
> > there's a context fault and there can't be TLB entries for entries
> > that cause a context fault: "The architecture permits the caching of
> > any translation table entry that has been returned from memory without
> > a fault and that does not, as a result of that entry, cause a
> > Translation Fault or an Access Flag fault."
>
> Ok, but what about other types of fault? For example, a permission fault
> or an address size fault?
>
> Will
I'm not sure, but the pseudocode for context faults mentions
resampling CFCFG after a fault happens ("We have a fault and must
resample FSR, CFCFG and HUPCF") so I don't think it would be legal to
cache it. Also in practice this does seem to work. Does that answer
it?
Connor
