| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 26 Jun 2024 21:09:20 +0100
From: Robin Murphy <robin.murphy@....com>
To: Rob Clark <robdclark@...il.com>, iommu@...ts.linux.dev
Cc: linux-arm-msm@...r.kernel.org, Stephen Boyd <swboyd@...omium.org>,
Pranjal Shrivastava <praan@...gle.com>, Rob Clark <robdclark@...omium.org>,
Will Deacon <will@...nel.org>, Joerg Roedel <joro@...tes.org>,
Jason Gunthorpe <jgg@...pe.ca>, Jerry Snitselaar <jsnitsel@...hat.com>,
Rob Herring <robh@...nel.org>, Dmitry Baryshkov
<dmitry.baryshkov@...aro.org>, Georgi Djakov <quic_c_gdjako@...cinc.com>,
"moderated list:ARM SMMU DRIVERS" <linux-arm-kernel@...ts.infradead.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] iommu/arm-smmu: Pretty-print context fault related
regs
On 2024-06-26 5:38 pm, Rob Clark wrote:
> From: Rob Clark <robdclark@...omium.org>
>
> Parse out the bitfields for easier-to-read fault messages.
>
> Signed-off-by: Rob Clark <robdclark@...omium.org>
> ---
> I kept with the dev_err, which matches qcom_smmu_context_fault. It is
> only adding two extra lines, and it is ratelimited.
>
> I also converted qcom_smmu_context_fault() to use the helpers to do the
> parsing, rather than more or less duplicating.
>
> .../iommu/arm/arm-smmu/arm-smmu-qcom-debug.c | 21 +++---
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 70 ++++++++++++++++++-
> drivers/iommu/arm/arm-smmu/arm-smmu.h | 58 +++++++++------
> 3 files changed, 110 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c
> index 552199cbd9e2..ee7eab273e1a 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c
> @@ -429,22 +429,17 @@ irqreturn_t qcom_smmu_context_fault(int irq, void *dev)
> phys_addr_t phys_atos = qcom_smmu_verify_fault(smmu_domain, iova, fsr);
>
> if (__ratelimit(&_rs)) {
> + char buf[80];
> +
> dev_err(smmu->dev,
> "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, cbfrsynra=0x%x, cb=%d\n",
> fsr, iova, fsynr, cbfrsynra, idx);
> - dev_err(smmu->dev,
> - "FSR = %08x [%s%s%s%s%s%s%s%s%s], SID=0x%x\n",
> - fsr,
> - (fsr & 0x02) ? "TF " : "",
> - (fsr & 0x04) ? "AFF " : "",
> - (fsr & 0x08) ? "PF " : "",
> - (fsr & 0x10) ? "EF " : "",
> - (fsr & 0x20) ? "TLBMCF " : "",
> - (fsr & 0x40) ? "TLBLKF " : "",
> - (fsr & 0x80) ? "MHF " : "",
> - (fsr & 0x40000000) ? "SS " : "",
> - (fsr & 0x80000000) ? "MULTI " : "",
> - cbfrsynra);
> +
> + arm_smmu_parse_fsr(buf, fsr);
> + dev_err(smmu->dev, "FSR: %s\n", buf);
> +
> + arm_smmu_parse_fsynr0(buf, fsynr);
> + dev_err(smmu->dev, "FSYNR0: %s\n", buf);
>
> dev_err(smmu->dev,
> "soft iova-to-phys=%pa\n", &phys_soft);
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 87c81f75cf84..7f5ca75d5ebe 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -405,12 +405,67 @@ static const struct iommu_flush_ops arm_smmu_s2_tlb_ops_v1 = {
> .tlb_add_page = arm_smmu_tlb_add_page_s2_v1,
> };
>
> +void arm_smmu_parse_fsr(char buf[80], u32 fsr)
> +{
> + static const struct {
> + u32 mask;
> + const char *name;
> + } fsr_bits[] = {
> + { ARM_SMMU_CB_FSR_MULTI, "MULTI" },
> + { ARM_SMMU_CB_FSR_SS, "SS" },
> + { ARM_SMMU_CB_FSR_UUT, "UUT" },
> + { ARM_SMMU_CB_FSR_ASF, "ASF" },
> + { ARM_SMMU_CB_FSR_TLBLKF, "TLBLKF" },
> + { ARM_SMMU_CB_FSR_TLBMCF, "TLBMCF" },
> + { ARM_SMMU_CB_FSR_EF, "EF" },
> + { ARM_SMMU_CB_FSR_PF, "PF" },
> + { ARM_SMMU_CB_FSR_AFF, "AFF" },
> + { ARM_SMMU_CB_FSR_TF, "TF" },
> + };
> + char *p = buf;
> +
> + p += sprintf(p, "FORMAT=%u",
> + (u32)FIELD_GET(ARM_SMMU_CB_FSR_FORMAT, fsr));
> +
> + for (int i = 0; i < ARRAY_SIZE(fsr_bits); i++)
> + if (fsr & fsr_bits[i].mask)
I still much prefer the arm64 data_abort_decode() name=value style, or
indeed even the qcom_smmu_context_fault() full/empty substring style -
that's similarly easier to understand, doesn't need the awkward
temporary buffer, and is still entirely capable of producing the same
output as you're achieving here. The other good idea from there is
repeating the raw register value in the same line, that way the decode
is entirely unambiguous at a glance, and trivial to correlate with the
full fault data from the first line even when interleaved and without
CONFIG_PRINTK_CALLER.
> + p += sprintf(p, "|%s", fsr_bits[i].name);
Furthermore, IM|NT|ENTRLY|CNVNCD|THS|IS|SO|EZ|TO|RD
Spaces, man. Spaces are good. If the aim is to be readable, the
capital-letter-salad involved here needs all the help it can get :)
> +}
> +
> +void arm_smmu_parse_fsynr0(char buf[80], u32 fsynr)
> +{
> + static const struct {
> + u32 mask;
> + const char *name;
> + } fsynr0_bits[] = {
> + { ARM_SMMU_CB_FSYNR0_WNR, "WNR" },
> + { ARM_SMMU_CB_FSYNR0_PNU, "PNU" },
> + { ARM_SMMU_CB_FSYNR0_IND, "IND" },
> + { ARM_SMMU_CB_FSYNR0_NSATTR, "NSATTR" },
> + { ARM_SMMU_CB_FSYNR0_PTWF, "PTWF" },
> + { ARM_SMMU_CB_FSYNR0_AFR, "AFR" },
> + };
> + char *p = buf;
> +
> + p += sprintf(p, "S1CBNDX=%u",
> + (u32)FIELD_GET(ARM_SMMU_CB_FSYNR0_S1CBNDX, fsynr));
> +
> + for (int i = 0; i < ARRAY_SIZE(fsynr0_bits); i++)
> + if (fsynr & fsynr0_bits[i].mask)
> + p += sprintf(p, "|%s", fsynr0_bits[i].name);
> +
> + p += sprintf(p, "|PLVL=%u",
> + (u32)FIELD_GET(ARM_SMMU_CB_FSYNR0_PLVL, fsynr));
> +}
> +
> static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
> {
> u32 fsr, fsynr, cbfrsynra;
> unsigned long iova;
> struct arm_smmu_domain *smmu_domain = dev;
> struct arm_smmu_device *smmu = smmu_domain->smmu;
> + static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
> + DEFAULT_RATELIMIT_BURST);
> int idx = smmu_domain->cfg.cbndx;
> int ret;
>
> @@ -423,13 +478,22 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
> cbfrsynra = arm_smmu_gr1_read(smmu, ARM_SMMU_GR1_CBFRSYNRA(idx));
>
> ret = report_iommu_fault(&smmu_domain->domain, NULL, iova,
> - fsynr & ARM_SMMU_FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ);
> + fsynr & ARM_SMMU_CB_FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ);
> +
> + if (ret == -ENOSYS && __ratelimit(&rs)) {
> + char buf[80];
>
> - if (ret == -ENOSYS)
> - dev_err_ratelimited(smmu->dev,
> + dev_err(smmu->dev,
> "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, cbfrsynra=0x%x, cb=%d\n",
> fsr, iova, fsynr, cbfrsynra, idx);
>
> + arm_smmu_parse_fsr(buf, fsr);
> + dev_err(smmu->dev, "FSR: %s\n", buf);
> +
> + arm_smmu_parse_fsynr0(buf, fsynr);
> + dev_err(smmu->dev, "FSYNR0: %s\n", buf);
> + }
> +
> arm_smmu_cb_write(smmu, idx, ARM_SMMU_CB_FSR, fsr);
> return IRQ_HANDLED;
> }
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> index 4765c6945c34..763ea52fca64 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> @@ -196,34 +196,46 @@ enum arm_smmu_cbar_type {
> #define ARM_SMMU_CB_PAR_F BIT(0)
>
> #define ARM_SMMU_CB_FSR 0x58
> -#define ARM_SMMU_FSR_MULTI BIT(31)
> -#define ARM_SMMU_FSR_SS BIT(30)
> -#define ARM_SMMU_FSR_UUT BIT(8)
> -#define ARM_SMMU_FSR_ASF BIT(7)
> -#define ARM_SMMU_FSR_TLBLKF BIT(6)
> -#define ARM_SMMU_FSR_TLBMCF BIT(5)
> -#define ARM_SMMU_FSR_EF BIT(4)
> -#define ARM_SMMU_FSR_PF BIT(3)
> -#define ARM_SMMU_FSR_AFF BIT(2)
> -#define ARM_SMMU_FSR_TF BIT(1)
> -
> -#define ARM_SMMU_FSR_IGN (ARM_SMMU_FSR_AFF | \
> - ARM_SMMU_FSR_ASF | \
> - ARM_SMMU_FSR_TLBMCF | \
> - ARM_SMMU_FSR_TLBLKF)
> -
> -#define ARM_SMMU_FSR_FAULT (ARM_SMMU_FSR_MULTI | \
> - ARM_SMMU_FSR_SS | \
> - ARM_SMMU_FSR_UUT | \
> - ARM_SMMU_FSR_EF | \
> - ARM_SMMU_FSR_PF | \
> - ARM_SMMU_FSR_TF | \
> +#define ARM_SMMU_CB_FSR_MULTI BIT(31)
> +#define ARM_SMMU_CB_FSR_SS BIT(30)
> +#define ARM_SMMU_CB_FSR_FORMAT GENMASK(10, 9)
> +#define ARM_SMMU_CB_FSR_UUT BIT(8)
> +#define ARM_SMMU_CB_FSR_ASF BIT(7)
> +#define ARM_SMMU_CB_FSR_TLBLKF BIT(6)
> +#define ARM_SMMU_CB_FSR_TLBMCF BIT(5)
> +#define ARM_SMMU_CB_FSR_EF BIT(4)
> +#define ARM_SMMU_CB_FSR_PF BIT(3)
> +#define ARM_SMMU_CB_FSR_AFF BIT(2)
> +#define ARM_SMMU_CB_FSR_TF BIT(1)
> +
> +void arm_smmu_parse_fsr(char buf[80], u32 fsr);
> +
> +#define ARM_SMMU_FSR_IGN (ARM_SMMU_CB_FSR_AFF | \
> + ARM_SMMU_CB_FSR_ASF | \
> + ARM_SMMU_CB_FSR_TLBMCF | \
> + ARM_SMMU_CB_FSR_TLBLKF)
> +
> +#define ARM_SMMU_FSR_FAULT (ARM_SMMU_CB_FSR_MULTI | \
> + ARM_SMMU_CB_FSR_SS | \
> + ARM_SMMU_CB_FSR_UUT | \
> + ARM_SMMU_CB_FSR_EF | \
> + ARM_SMMU_CB_FSR_PF | \
> + ARM_SMMU_CB_FSR_TF | \
> ARM_SMMU_FSR_IGN)
>
> #define ARM_SMMU_CB_FAR 0x60
>
> #define ARM_SMMU_CB_FSYNR0 0x68
> -#define ARM_SMMU_FSYNR0_WNR BIT(4)
> +#define ARM_SMMU_CB_FSYNR0_PLVL GENMASK(1, 0)
> +#define ARM_SMMU_CB_FSYNR0_WNR BIT(4)
> +#define ARM_SMMU_CB_FSYNR0_PNU BIT(5)
> +#define ARM_SMMU_CB_FSYNR0_IND BIT(6)
> +#define ARM_SMMU_CB_FSYNR0_NSATTR BIT(8)
> +#define ARM_SMMU_CB_FSYNR0_PTWF BIT(10)
> +#define ARM_SMMU_CB_FSYNR0_AFR BIT(11)
> +#define ARM_SMMU_CB_FSYNR0_S1CBNDX GENMASK(23, 16)
Nit: MSB-to-LSB order like all the other register fields, please.
Thanks,
Robin.
> +
> +void arm_smmu_parse_fsynr0(char buf[80], u32 fsynr);
>
> #define ARM_SMMU_CB_FSYNR1 0x6c
>
Powered by blists - more mailing lists