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

[David Thompson]

> -----Original Message-----
> From: Borislav Petkov <bp@xxxxxxxxx>
> Sent: Thursday, October 17, 2024 8:28 AM
> To: David Thompson <davthompson@xxxxxxxxxx>
> Cc: tony.luck@xxxxxxxxx; james.morse@xxxxxxx; mchehab@xxxxxxxxxx;
> rric@xxxxxxxxxx; linux-edac@xxxxxxxxxxxxxxx; Shravan Ramani
> <shravankr@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> 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