linux-kernel - Re: [PATCH v4 4/4] RAS/AMD/ATL: Implement DF 4.5 NP2 denormalization

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zl4vIkNNzP+cKSYF@AUS-L1-JOHALLEN.amd.com>
Date: Mon, 3 Jun 2024 16:01:22 -0500
From: John Allen <john.allen@....com>
To: Borislav Petkov <bp@...en8.de>
Cc: linux-edac@...r.kernel.org, tony.luck@...el.com, yazen.ghannam@....com,
	linux-kernel@...r.kernel.org, avadhut.naik@....com,
	muralidhara.mk@....com
Subject: Re: [PATCH v4 4/4] RAS/AMD/ATL: Implement DF 4.5 NP2 denormalization

On Tue, May 28, 2024 at 12:23:44PM +0200, Borislav Petkov wrote:
> On Mon, May 06, 2024 at 03:46:05PM +0000, John Allen wrote:
> > +	switch (ctx->map.intlv_mode) {
> > +	case DF4p5_NPS0_24CHAN_1K_HASH:
> > +		cs_id = FIELD_GET(GENMASK_ULL(63, 13), denorm_ctx->current_spa) << 3;
> > +		cs_id %= denorm_ctx->mod_value;
> > +		cs_id <<= 2;
> > +		cs_id |= (hash_pa9 | (hash_pa12 << 1));
> > +		cs_id |= hash_pa8 << df_cfg.socket_id_shift;
> > +		break;
> 
> Newlines after those "break"s pls.
> 
> > +	case DF4p5_NPS0_24CHAN_2K_HASH:
> > +		cs_id = FIELD_GET(GENMASK_ULL(63, 14), denorm_ctx->current_spa) << 4;
> > +		cs_id %= denorm_ctx->mod_value;
> > +		cs_id <<= 2;
> > +		cs_id |= (hash_pa12 | (hash_pa13 << 1));
> > +		cs_id |= hash_pa8 << df_cfg.socket_id_shift;
> > +		break;
> 
> ...
> 
> > +static u64 normalize_addr_df4p5_np2(struct addr_ctx *ctx, struct df4p5_denorm_ctx *denorm_ctx,
> > +				    u64 addr)
> > +{
> > +	u64 temp_addr_a = 0, temp_addr_b = 0;
> > +
> > +	switch (ctx->map.intlv_mode) {
> > +	case DF4p5_NPS0_24CHAN_1K_HASH:
> > +	case DF4p5_NPS1_12CHAN_1K_HASH:
> > +	case DF4p5_NPS2_6CHAN_1K_HASH:
> > +	case DF4p5_NPS4_3CHAN_1K_HASH:
> > +	case DF4p5_NPS1_10CHAN_1K_HASH:
> > +	case DF4p5_NPS2_5CHAN_1K_HASH:
> > +		temp_addr_a = FIELD_GET(GENMASK_ULL(11, 10), addr) << 8;
> > +		break;
> 
> Also linebreaks pls.
> 
> Please change this in all your patches.
> 
> > diff --git a/drivers/ras/amd/atl/internal.h b/drivers/ras/amd/atl/internal.h
> > index 05b870fcb24e..946e36c053c5 100644
> > --- a/drivers/ras/amd/atl/internal.h
> > +++ b/drivers/ras/amd/atl/internal.h
> > @@ -34,6 +34,8 @@
> >  #define DF_DRAM_BASE_LIMIT_LSB		28
> >  #define MI300_DRAM_LIMIT_LSB		20
> >  
> > +#define INVALID_SPA ~0ULL
> 
> No lazy definitions pls:
> 
> drivers/ras/amd/atl/internal.h:37:#define INVALID_SPA ~0ULL
> drivers/ras/amd/fmpm.c:119:#define INVALID_SPA  ~0ULL
> 
> Unify them.

Is there a particular existing header that you suggest we pull this into
or just create a new one under drivers/ras/amd?

Thanks,
John

> 
> > +
> >  enum df_revisions {
> >  	UNKNOWN,
> >  	DF2,
> > @@ -90,6 +92,44 @@ enum intlv_modes {
> >  	DF4p5_NPS1_10CHAN_2K_HASH	= 0x49,
> >  };
> >  
> > +struct df4p5_denorm_ctx {
> > +	/* perm_shift: Indicates the number of "lost" bits. This will be 1, 2, or 3. */
> 
> No need to repeat the variable name in the comment.
> 
> ...
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette