| 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
| ||
|
Message-ID: <20241017122816.GUZxEC4B04zA6YsCuS@fat_crate.local>
Date: Thu, 17 Oct 2024 14:28:16 +0200
From: Borislav Petkov <bp@...en8.de>
To: David Thompson <davthompson@...dia.com>
Cc: tony.luck@...el.com, james.morse@....com, mchehab@...nel.org,
rric@...nel.org, linux-edac@...r.kernel.org, shravankr@...dia.com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 RESEND] EDAC/bluefield: Use Arm SMC for EMI access on
BlueField-2
On Mon, Sep 30, 2024 at 11:20:30AM -0400, David Thompson wrote:
> The BlueField EDAC driver supports the first generation BlueField-1 SoC,
> but not the second generation BlueField-2 SoC. The BlueField-2 SoC is
> different in that only secure accesses are allowed to the External Memory
> Interface (EMI) register block. On BlueField-2, all read/write accesses
> from Linux to EMI registers are routed via Arm Secure Monitor Call (SMC)
> through Arm Trusted Firmware (ATF), which runs at EL3 privileged state.
>
> On BlueField-1, EMI registers are mapped and accessed directly. In order
> to support BlueField-2, the driver's read and write access methods must
> be extended with additional logic to include secure access to the EMI
> registers via SMCs.
>
> The driver's probe routine must check the ACPI table for presence of
> the "sec_reg_block" property and ensure the minimum required SMC service
> version is present before enabling the BlueField-2 secure access methods.
> The "sec_reg_block" property is only present in BlueField-2 ACPI table,
> not the BlueField-1 ACPI table.
>
> Also, the bluefield_edac driver needs two coding style fixes: one fix
> addresses an issue raised by checkpatch, and the other fix provides
> consistency in declaration of #defines.
Do not talk about *what* the patch is doing in the commit message - that
should be obvious from the diff itself. Rather, concentrate on the *why*
it needs to be done.
Imagine one fine day you're doing git archeology, you find the place in
the code about which you want to find out why it was changed the way it
is now.
You do git annotate <filename> ... find the line, see the commit id and
you do:
git show <commit id>
You read the commit message and there's just gibberish and nothing's
explaining *why* that change was done. And you start scratching your head,
trying to figure out why. Because the damn commit message is not really
helping.
> 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;
Some comments above these members as to what they are, would be good.
> static u64 smc_call1(u64 smc_op, u64 smc_arg)
> @@ -86,6 +98,71 @@ static u64 smc_call1(u64 smc_op, u64 smc_arg)
> return res.a0;
> }
>
> +static int bluefield_edac_secure_readl(void __iomem *addr, u32 *result, u32 sreg_tbl)
For all static functions' names:
s/bluefield_edac_//
> +{
> + 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 = (u32)res.a1;
> + return 0;
> + }
This is a silly switch-case - just use a normal if-else.
> +}
> +
> +static int bluefield_edac_secure_writel(void __iomem *addr, u32 data, u32 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;
> + }
Ditto.
> @@ -109,14 +186,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 = bluefield_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 = bluefield_edac_readl(priv->emi_base + MLXBF_SYNDROM, &dram_syndrom,
> + priv->svc_sreg_support, priv->sreg_tbl_edac);
You're passing three priv members down here. Why don't you simply pass down
@priv itself?
Ditto for the write routine.
> + 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);
...
> @@ -279,13 +385,44 @@ static int bluefield_edac_mc_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> priv = mci->pvt_info;
> + priv->dev = dev;
> +
> + /*
> + * The "sec_reg_block" property in the ACPI table determines the method
> + * the driver uses to access the EMI registers:
> + * a) property is not present - directly access registers via readl/writel
> + * b) property is present - indirectly access registers via SMC calls
> + * (assuming required Silicon Provider service version found)
> + */
> + if (device_property_read_u32(dev,
> + "sec_reg_block", &priv->sreg_tbl_edac)) {
Please do not do such ugly linebreaks.
> + priv->svc_sreg_support = false;
> + } else {
> + /*
> + * Check for minimum required Arm Silicon Provider (SiP) service
> + * version, ensuring support of required SMC function IDs.
> + */
> + 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;
> + }
> + }
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Powered by blists - more mailing lists