* [PATCH v1] EDAC/bluefield_edac: Use ARM SMC for EMI access
@ 2023-08-30 10:56 Shravan Kumar Ramani
2023-08-30 11:34 ` Borislav Petkov
0 siblings, 1 reply; 4+ messages in thread
From: Shravan Kumar Ramani @ 2023-08-30 10:56 UTC (permalink / raw)
To: James Morse, Mauro Carvalho Chehab, Robert Richter, Borislav Petkov
Cc: Shravan Kumar Ramani, linux-edac, linux-kernel
Add secure read/write calls (arm_smccc_smc) to bluefield_edac.
The ACPI table entry decides whether SMC is need for accessing
the registers in the External Memory Interface block. If not,
the registers may be mapped and accessed directly.
v0 -> v1
Updated commit message
Signed-off-by: Shravan Kumar Ramani <shravankr@nvidia.com>
---
drivers/edac/bluefield_edac.c | 176 ++++++++++++++++++++++++++++++----
1 file changed, 156 insertions(+), 20 deletions(-)
diff --git a/drivers/edac/bluefield_edac.c b/drivers/edac/bluefield_edac.c
index e4736eb37bfb..cb9e42ef64d1 100644
--- a/drivers/edac/bluefield_edac.c
+++ b/drivers/edac/bluefield_edac.c
@@ -47,13 +47,25 @@
#define MLXBF_EDAC_MAX_DIMM_PER_MC 2
#define MLXBF_EDAC_ERROR_GRAIN 8
+#define MLXBF_WRITE_REG_32 (0x82000009)
+#define MLXBF_READ_REG_32 (0x8200000A)
+#define MLXBF_WRITE_REG_64 (0x8200000B)
+#define MLXBF_READ_REG_64 (0x8200000C)
+#define MLXBF_SIP_SVC_UID (0x8200ff01)
+#define MLXBF_SIP_SVC_VERSION (0x8200ff03)
+
+#define MLXBF_SMCCC_ACCESS_VIOLATION (-4)
+
+#define MLXBF_SVC_REQ_MAJOR 0
+#define MLXBF_SVC_REQ_MINOR 3
+
/*
- * Request MLNX_SIP_GET_DIMM_INFO
+ * Request MLXBF_SIP_GET_DIMM_INFO
*
* Retrieve information about DIMM on a certain slot.
*
* Call register usage:
- * a0: MLNX_SIP_GET_DIMM_INFO
+ * a0: MLXBF_SIP_GET_DIMM_INFO
* a1: (Memory controller index) << 16 | (Dimm index in memory controller)
* a2-7: not used.
*
@@ -61,7 +73,7 @@
* a0: MLXBF_DIMM_INFO defined below describing the DIMM.
* a1-3: not used.
*/
-#define MLNX_SIP_GET_DIMM_INFO 0x82000008
+#define MLXBF_SIP_GET_DIMM_INFO 0x82000008
/* Format for the SMC response about the memory information */
#define MLXBF_DIMM_INFO__SIZE_GB GENMASK_ULL(15, 0)
@@ -72,9 +84,12 @@
#define MLXBF_DIMM_INFO__PACKAGE_X GENMASK_ULL(31, 24)
struct bluefield_edac_priv {
+ struct device *dev;
int dimm_ranks[MLXBF_EDAC_MAX_DIMM_PER_MC];
void __iomem *emi_base;
int dimm_per_mc;
+ bool svc_sreg_support;
+ u32 sreg_tbl_edac;
};
static u64 smc_call1(u64 smc_op, u64 smc_arg)
@@ -86,6 +101,71 @@ static u64 smc_call1(u64 smc_op, u64 smc_arg)
return res.a0;
}
+static int secure_readl(void __iomem *addr, uint32_t *result, uint32_t sreg_tbl)
+{
+ struct arm_smccc_res res;
+ int status;
+
+ arm_smccc_smc(MLXBF_READ_REG_32, sreg_tbl, (uintptr_t)addr,
+ 0, 0, 0, 0, 0, &res);
+
+ status = res.a0;
+
+ switch (status) {
+ case SMCCC_RET_NOT_SUPPORTED:
+ case MLXBF_SMCCC_ACCESS_VIOLATION:
+ return -1;
+ default:
+ *result = (uint32_t)res.a1;
+ return 0;
+ }
+}
+
+static int secure_writel(void __iomem *addr, uint32_t data, uint32_t sreg_tbl)
+{
+ struct arm_smccc_res res;
+ int status;
+
+ arm_smccc_smc(MLXBF_WRITE_REG_32, sreg_tbl, data, (uintptr_t)addr,
+ 0, 0, 0, 0, &res);
+
+ status = res.a0;
+
+ switch (status) {
+ case SMCCC_RET_NOT_SUPPORTED:
+ case MLXBF_SMCCC_ACCESS_VIOLATION:
+ return -1;
+ default:
+ return 0;
+ }
+}
+
+static int edac_readl(void __iomem *addr, uint32_t *result,
+ bool sreg_support, uint32_t sreg_tbl)
+{
+ int err = 0;
+
+ if (sreg_support)
+ err = secure_readl(addr, result, sreg_tbl);
+ else
+ *result = readl(addr);
+
+ return err;
+}
+
+static int edac_writel(void __iomem *addr, uint32_t data,
+ bool sreg_support, uint32_t sreg_tbl)
+{
+ int err = 0;
+
+ if (sreg_support)
+ err = secure_writel(addr, data, sreg_tbl);
+ else
+ writel(data, addr);
+
+ return err;
+}
+
/*
* Gather the ECC information from the External Memory Interface registers
* and report it to the edac handler.
@@ -99,7 +179,7 @@ static void bluefield_gather_report_ecc(struct mem_ctl_info *mci,
u32 ecc_latch_select, dram_syndrom, serr, derr, syndrom;
enum hw_event_mc_err_type ecc_type;
u64 ecc_dimm_addr;
- int ecc_dimm;
+ int ecc_dimm, err;
ecc_type = is_single_ecc ? HW_EVENT_ERR_CORRECTED :
HW_EVENT_ERR_UNCORRECTED;
@@ -109,14 +189,22 @@ static void bluefield_gather_report_ecc(struct mem_ctl_info *mci,
* registers with information about the last ECC error occurrence.
*/
ecc_latch_select = MLXBF_ECC_LATCH_SEL__START;
- writel(ecc_latch_select, priv->emi_base + MLXBF_ECC_LATCH_SEL);
+ err = edac_writel(priv->emi_base + MLXBF_ECC_LATCH_SEL,
+ ecc_latch_select, priv->svc_sreg_support,
+ priv->sreg_tbl_edac);
+ if (err)
+ dev_err(priv->dev, "ECC latch select write failed.\n");
/*
* Verify that the ECC reported info in the registers is of the
* same type as the one asked to report. If not, just report the
* error without the detailed information.
*/
- dram_syndrom = readl(priv->emi_base + MLXBF_SYNDROM);
+ err = edac_readl(priv->emi_base + MLXBF_SYNDROM, &dram_syndrom,
+ priv->svc_sreg_support, priv->sreg_tbl_edac);
+ if (err)
+ dev_err(priv->dev, "DRAM syndrom read failed.\n");
+
serr = FIELD_GET(MLXBF_SYNDROM__SERR, dram_syndrom);
derr = FIELD_GET(MLXBF_SYNDROM__DERR, dram_syndrom);
syndrom = FIELD_GET(MLXBF_SYNDROM__SYN, dram_syndrom);
@@ -127,13 +215,24 @@ static void bluefield_gather_report_ecc(struct mem_ctl_info *mci,
return;
}
- dram_additional_info = readl(priv->emi_base + MLXBF_ADD_INFO);
+ err = edac_readl(priv->emi_base + MLXBF_ADD_INFO, &dram_additional_info,
+ priv->svc_sreg_support, priv->sreg_tbl_edac);
+ if (err)
+ dev_err(priv->dev, "DRAM additional info read failed.\n");
+
err_prank = FIELD_GET(MLXBF_ADD_INFO__ERR_PRANK, dram_additional_info);
ecc_dimm = (err_prank >= 2 && priv->dimm_ranks[0] <= 2) ? 1 : 0;
- edea0 = readl(priv->emi_base + MLXBF_ERR_ADDR_0);
- edea1 = readl(priv->emi_base + MLXBF_ERR_ADDR_1);
+ err = edac_readl(priv->emi_base + MLXBF_ERR_ADDR_0, &edea0,
+ priv->svc_sreg_support, priv->sreg_tbl_edac);
+ if (err)
+ dev_err(priv->dev, "Error addr 0 read failed.\n");
+
+ err = edac_readl(priv->emi_base + MLXBF_ERR_ADDR_1, &edea1,
+ priv->svc_sreg_support, priv->sreg_tbl_edac);
+ if (err)
+ dev_err(priv->dev, "Error addr 1 read failed.\n");
ecc_dimm_addr = ((u64)edea1 << 32) | edea0;
@@ -147,6 +246,7 @@ static void bluefield_edac_check(struct mem_ctl_info *mci)
{
struct bluefield_edac_priv *priv = mci->pvt_info;
u32 ecc_count, single_error_count, double_error_count, ecc_error = 0;
+ int err;
/*
* The memory controller might not be initialized by the firmware
@@ -155,7 +255,11 @@ static void bluefield_edac_check(struct mem_ctl_info *mci)
if (mci->edac_cap == EDAC_FLAG_NONE)
return;
- ecc_count = readl(priv->emi_base + MLXBF_ECC_CNT);
+ err = edac_readl(priv->emi_base + MLXBF_ECC_CNT, &ecc_count,
+ priv->svc_sreg_support, priv->sreg_tbl_edac);
+ if (err)
+ dev_err(priv->dev, "ECC count read failed.\n");
+
single_error_count = FIELD_GET(MLXBF_ECC_CNT__SERR_CNT, ecc_count);
double_error_count = FIELD_GET(MLXBF_ECC_CNT__DERR_CNT, ecc_count);
@@ -172,15 +276,19 @@ static void bluefield_edac_check(struct mem_ctl_info *mci)
}
/* Write to clear reported errors. */
- if (ecc_count)
- writel(ecc_error, priv->emi_base + MLXBF_ECC_ERR);
+ if (ecc_count) {
+ err = edac_writel(priv->emi_base + MLXBF_ECC_ERR, ecc_error,
+ priv->svc_sreg_support, priv->sreg_tbl_edac);
+ if (err)
+ dev_err(priv->dev, "ECC Error write failed.\n");
+ }
}
/* Initialize the DIMMs information for the given memory controller. */
static void bluefield_edac_init_dimms(struct mem_ctl_info *mci)
{
struct bluefield_edac_priv *priv = mci->pvt_info;
- int mem_ctrl_idx = mci->mc_idx;
+ u64 mem_ctrl_idx = mci->mc_idx;
struct dimm_info *dimm;
u64 smc_info, smc_arg;
int is_empty = 1, i;
@@ -189,7 +297,7 @@ static void bluefield_edac_init_dimms(struct mem_ctl_info *mci)
dimm = mci->dimms[i];
smc_arg = mem_ctrl_idx << 16 | i;
- smc_info = smc_call1(MLNX_SIP_GET_DIMM_INFO, smc_arg);
+ smc_info = smc_call1(MLXBF_SIP_GET_DIMM_INFO, smc_arg);
if (!FIELD_GET(MLXBF_DIMM_INFO__SIZE_GB, smc_info)) {
dimm->mtype = MEM_EMPTY;
@@ -244,6 +352,7 @@ static int bluefield_edac_mc_probe(struct platform_device *pdev)
struct bluefield_edac_priv *priv;
struct device *dev = &pdev->dev;
struct edac_mc_layer layers[1];
+ struct arm_smccc_res res;
struct mem_ctl_info *mci;
struct resource *emi_res;
unsigned int mc_idx, dimm_count;
@@ -280,12 +389,40 @@ static int bluefield_edac_mc_probe(struct platform_device *pdev)
priv = mci->pvt_info;
+ /*
+ * ACPI indicates whether we use SMCs to access registers or not.
+ * If sreg_tbl_perf is not present, just assume we're not using SMCs.
+ */
+ if (device_property_read_u32(dev,
+ "sec_reg_block", &priv->sreg_tbl_edac)) {
+ priv->svc_sreg_support = false;
+ } else {
+ /*
+ * Check service version to see if we actually do support the
+ * needed SMCs. If we have the calls we need, mark support for
+ * them in the pmc struct.
+ */
+ arm_smccc_smc(MLXBF_SIP_SVC_VERSION, 0, 0, 0, 0, 0, 0, 0, &res);
+ if (res.a0 == MLXBF_SVC_REQ_MAJOR &&
+ res.a1 >= MLXBF_SVC_REQ_MINOR) {
+ priv->svc_sreg_support = true;
+ } else {
+ dev_err(dev, "Required SMCs are not supported.\n");
+ ret = -EINVAL;
+ goto err;
+ }
+ }
+
priv->dimm_per_mc = dimm_count;
- priv->emi_base = devm_ioremap_resource(dev, emi_res);
- if (IS_ERR(priv->emi_base)) {
- dev_err(dev, "failed to map EMI IO resource\n");
- ret = PTR_ERR(priv->emi_base);
- goto err;
+ if (!priv->svc_sreg_support) {
+ priv->emi_base = devm_ioremap_resource(dev, emi_res);
+ if (IS_ERR(priv->emi_base)) {
+ dev_err(dev, "failed to map EMI IO resource\n");
+ ret = PTR_ERR(priv->emi_base);
+ goto err;
+ }
+ } else {
+ priv->emi_base = (void __iomem *)emi_res->start;
}
mci->pdev = dev;
@@ -320,7 +457,6 @@ static int bluefield_edac_mc_probe(struct platform_device *pdev)
edac_mc_free(mci);
return ret;
-
}
static int bluefield_edac_mc_remove(struct platform_device *pdev)
--
2.30.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1] EDAC/bluefield_edac: Use ARM SMC for EMI access
2023-08-30 10:56 [PATCH v1] EDAC/bluefield_edac: Use ARM SMC for EMI access Shravan Kumar Ramani
@ 2023-08-30 11:34 ` Borislav Petkov
2023-08-31 4:31 ` Shravan Ramani
0 siblings, 1 reply; 4+ messages in thread
From: Borislav Petkov @ 2023-08-30 11:34 UTC (permalink / raw)
To: Shravan Kumar Ramani
Cc: James Morse, Mauro Carvalho Chehab, Robert Richter, linux-edac,
linux-kernel
On Wed, Aug 30, 2023 at 06:56:14AM -0400, Shravan Kumar Ramani wrote:
> Add secure read/write calls (arm_smccc_smc) to bluefield_edac.
> The ACPI table entry decides whether SMC is need for accessing
SMC? Self-Modifying Code?
> the registers in the External Memory Interface block. If not,
> the registers may be mapped and accessed directly.
I'm sure there's a human readable explanation for the above.
> v0 -> v1
> Updated commit message
I still have no clue what those secure calls are.
I'm sure there's some ARM documentation which you use in order to
explain why those are needed.
> +static int edac_readl(void __iomem *addr, uint32_t *result,
No "edac_" previxes to driver-local functions. They belong to the EDAC
subsystem.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] EDAC/bluefield_edac: Use ARM SMC for EMI access
2023-08-30 11:34 ` Borislav Petkov
@ 2023-08-31 4:31 ` Shravan Ramani
2023-09-01 7:38 ` Borislav Petkov
0 siblings, 1 reply; 4+ messages in thread
From: Shravan Ramani @ 2023-08-31 4:31 UTC (permalink / raw)
To: Borislav Petkov
Cc: James Morse, Mauro Carvalho Chehab, Robert Richter, linux-edac,
linux-kernel
>> Add secure read/write calls (arm_smccc_smc) to bluefield_edac.
>> The ACPI table entry decides whether SMC is need for accessing
>
> SMC? Self-Modifying Code?
SMC stands for Secure Monitor Call. ARM documentation can be found
here: https://developer.arm.com/documentation/den0028/latest/
SMC is needed by the driver to access certain HW blocks/registers which
can be accessed only from secure state. Whether a certain block requires
these secure calls or not is encoded in the ACPI table entry and passed
on to the driver.
>
>> +static int edac_readl(void __iomem *addr, uint32_t *result,
>
> No "edac_" previxes to driver-local functions. They belong to the EDAC
> subsystem.
Ack, will change the prefixes in v2.
Thanks,
Shravan
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] EDAC/bluefield_edac: Use ARM SMC for EMI access
2023-08-31 4:31 ` Shravan Ramani
@ 2023-09-01 7:38 ` Borislav Petkov
0 siblings, 0 replies; 4+ messages in thread
From: Borislav Petkov @ 2023-09-01 7:38 UTC (permalink / raw)
To: Shravan Ramani
Cc: James Morse, Mauro Carvalho Chehab, Robert Richter, linux-edac,
linux-kernel
On Thu, Aug 31, 2023 at 04:31:23AM +0000, Shravan Ramani wrote:
> SMC is needed by the driver to access certain HW blocks/registers which
> can be accessed only from secure state. Whether a certain block requires
> these secure calls or not is encoded in the ACPI table entry and passed
> on to the driver.
This sounds like a good start towards explaining why this patch is
needed. Here's an example commit message structure:
1. Prepare the context for the explanation briefly.
2. Explain the problem at hand.
3. "It happens because of <...>"
4. "Fix it by doing X"
5. "(Potentially do Y)."
For more detailed info, see
Documentation/process/submitting-patches.rst, Section "2) Describe your
changes".
Do not talk about what your patch does - that should (hopefully) be
visible from the diff itself. Rather, talk about *why* you're doing what
you're doing.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-09-01 7:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-30 10:56 [PATCH v1] EDAC/bluefield_edac: Use ARM SMC for EMI access Shravan Kumar Ramani
2023-08-30 11:34 ` Borislav Petkov
2023-08-31 4:31 ` Shravan Ramani
2023-09-01 7:38 ` Borislav Petkov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).