On 12/03/2025 5:23 pm, Rob Clark wrote:
On Wed, Mar 12, 2025 at 9:47 AM Will Deacon <will@xxxxxxxxxx> wrote:
On Wed, Mar 12, 2025 at 07:59:52AM -0700, Rob Clark wrote:
On Wed, Mar 12, 2025 at 6:05 AM Will Deacon <will@xxxxxxxxxx> wrote:
On Tue, Mar 11, 2025 at 06:36:38PM -0400, Connor Abbott wrote:
On Tue, Mar 11, 2025 at 2:06 PM Will Deacon <will@xxxxxxxxxx> wrote:
On Tue, Mar 04, 2025 at 11:56:47AM -0500, Connor Abbott wrote:
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index ade4684c14c9b2724a71e2457288dbfaf7562c83..a9213e0f1579d1e3be0bfba75eea1d5de23117de 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -409,9 +409,12 @@ void arm_smmu_read_context_fault_info(struct arm_smmu_device *smmu, int idx,
struct arm_smmu_context_fault_info *cfi)
{
cfi->iova = arm_smmu_cb_readq(smmu, idx, ARM_SMMU_CB_FAR);
+ cfi->ttbr0 = arm_smmu_cb_readq(smmu, idx, ARM_SMMU_CB_TTBR0);
cfi->fsr = arm_smmu_cb_read(smmu, idx, ARM_SMMU_CB_FSR);
- cfi->fsynr = arm_smmu_cb_read(smmu, idx, ARM_SMMU_CB_FSYNR0);
+ cfi->fsynr0 = arm_smmu_cb_read(smmu, idx, ARM_SMMU_CB_FSYNR0);
+ cfi->fsynr1 = arm_smmu_cb_read(smmu, idx, ARM_SMMU_CB_FSYNR1);
We already have an implementation hook (->get_fault_info()) which the
qcom SMMU driver can override with qcom_adreno_smmu_get_fault_info().
That thing dumps these registers already so if we're moving that into
the core SMMU driver, let's get rid of the hook and move everybody over
rather than having it done in both places.
As you probably saw, the next commit moves over
qcom_adreno_smmu_get_fault_info() to use this. The current back door
used by drm/msm to access these functions is specific to adreno_smmu
and there isn't an equivalent interface to allow it to call a generic
SMMU function so it isn't possible to move it entirely to the core. At
least not without a bigger refactoring that isn't justified for this
series that is just trying to fix things.
Ok :(
cfi->cbfrsynra = arm_smmu_gr1_read(smmu, ARM_SMMU_GR1_CBFRSYNRA(idx));
+ cfi->contextidr = arm_smmu_cb_read(smmu, idx, ARM_SMMU_CB_CONTEXTIDR);
I think the CONTEXTIDR register is stage-1 only, so we shouldn't dump
it for stage-2 domains.
Does it matter if we read the register though, as long as users are
aware of this and don't use its value for anything?
I think the contents are "UNKNOWN", so it could be hugely confusing even
if they just got logged someplace. Why is it difficult to avoid touching
it for stage-2?
Fwiw, we are only ever using stage-1
Sure, but this is in arm-smmu.c which is used by other people and supports
both stages.
Sure, but no one else is using this field in the fault-info. So maybe
the addition of a comment in the struct would be enough if it isn't
going to cause an SError/etc to read it for S2 cb?
Any worthwhile comment isn't going to be significantly shorter or
clearer than 1 extra line of "if (smmu_domain->stage ==
ARM_SMMU_DOMAIN_S1)"...
TBH it's the Qualcomm register-middle-man firmware I'd be more worried
about than real hardware, given how touchy it can be even with register
accesses which *should* be well defined. But then I guess it also has
the habit of killing the system if anything other than the GPU dares
cause a fault in the first place, so maybe it OK?
If anyone still uses Arm Fast Models SMMUv1/2 components it'll probably
squawk an annoying warning there too - ISTR I had at least one patch
motivated by that in the past.
Thanks,
Robin.
