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]

Date: Tue, 28 May 2024 12:23:44 +0200
From: Borislav Petkov <bp@...en8.de>
To: John Allen <john.allen@....com>
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 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.

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