linux-kernel - RE: [PATCH v2 RESEND] EDAC/bluefield: Use Arm SMC for EMI access on BlueField-2

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <PH7PR12MB59022E4D59E610807572CCD7C7432@PH7PR12MB5902.namprd12.prod.outlook.com>
Date: Mon, 21 Oct 2024 22:07:20 +0000
From: David Thompson <davthompson@...dia.com>
To: Borislav Petkov <bp@...en8.de>
CC: "tony.luck@...el.com" <tony.luck@...el.com>, "james.morse@....com"
	<james.morse@....com>, "mchehab@...nel.org" <mchehab@...nel.org>,
	"rric@...nel.org" <rric@...nel.org>, "linux-edac@...r.kernel.org"
	<linux-edac@...r.kernel.org>, Shravan Ramani <shravankr@...dia.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 RESEND] EDAC/bluefield: Use Arm SMC for EMI access on
 BlueField-2

> -----Original Message-----
> From: Borislav Petkov <bp@...en8.de>
> Sent: Thursday, October 17, 2024 8:28 AM
> 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; Shravan Ramani
> <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.
> 

Hello Borislav,

Thank you for all your valuable feedback.

I have shortened the commit message to include the reason for the patch,
and less about what the patch is doing .  Please take a look at v3.

> >  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.
> 

v3 patch will include some comments

> >  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_//
> 

The v3 of this patch renamed static functions "bluefield_edac_secure_[readl|writel]"
to "secure_[readl|writel]". Note: the static functions "bluefield_edac_[readl|writel]" 
are not renamed to "readl|writel" because that would conflict with IO primitives in kernel.

The v3 of this patch will also include fixes for all other issues you've raised.

Regards, Dave