linux-kernel - Re: [PATCH v4] EDAC/amd64: Fix size calculation for Non-Power-of-Two DIMMs

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: <20250528092250.GAaDbV6oEqvE3279dJ@fat_crate.local>
Date: Wed, 28 May 2025 11:22:50 +0200
From: Borislav Petkov <bp@...en8.de>
To: Avadhut Naik <avadhut.naik@....com>
Cc: linux-edac@...r.kernel.org, linux-kernel@...r.kernel.org,
	avadnaik@....com,
	Žilvinas Žaltiena <zilvinas@...rix.lt>,
	Yazen Ghannam <yazen.ghannam@....com>
Subject: Re: [PATCH v4] EDAC/amd64: Fix size calculation for Non-Power-of-Two
 DIMMs

On Tue, May 13, 2025 at 07:20:11PM +0000, Avadhut Naik wrote:
> Each Chip-Select (CS) of a Unified Memory Controller (UMC) on AMD's
> modern Zen-based SOCs has an Address Mask and a Secondary Address Mask
> register associated with it. The amd64_edac module logs DIMM sizes on a
> per-UMC per-CS granularity during init using these two registers.
> 
> Currently, the module primarily considers only the Address Mask register
> for computing DIMM sizes. The Secondary Address Mask register is only
> considered for odd CS. Additionally, if it has been considered, the
> Address Mask register is ignored altogether for that CS. For
> power-of-two DIMMs, this is not an issue since only the Address Mask

What are power-of-two DIMMs?

The number of DIMMs on the system is a 2^x?

Their ranks are a power of two?

Their combined size is not power of two?

One can only guess...

> register is used.
> 
> For non-power-of-two DIMMs, however, the Secondary Address Mask register
> is used in conjunction with the Address Mask register. However, since the
> module only considers either of the two registers for a CS, the size
> computed by the module is incorrect.

Yah, it must be something about the size...

> The Secondary Address Mask register
> is not considered for even CS, and the Address Mask register is not
> considered for odd CS.
> 
> Introduce a new helper function so that both Address Mask and Secondary
> Address Mask registers are considered, when valid, for computing DIMM
> sizes. Furthermore, also rename some variables for greater clarity.

So it is non-power-of-two sized DIMMs?

IOW, DIMMs whose size is not a power of two?

> Fixes: 81f5090db843 ("EDAC/amd64: Support asymmetric dual-rank DIMMs")
> Reported-by: Žilvinas Žaltiena <zilvinas@...rix.lt>
> Closes: https://lore.kernel.org/dbec22b6-00f2-498b-b70d-ab6f8a5ec87e@natrix.lt
> Signed-off-by: Avadhut Naik <avadhut.naik@....com>
> Tested-by: Žilvinas Žaltiena <zilvinas@...rix.lt>
> Reviewed-by: Yazen Ghannam <yazen.ghannam@....com>
> Cc: stable@...r.kernel.org

All that changelog stuff...

> ```
> Changes in v2:
> 1. Avoid unnecessary variable initialization.
> 2. Modify commit message to accurately reflect the changes.
> 3. Move check for non-zero Address Mask register into the new helper.
> 
> Changes in v3:
> 1. Add the missing Closes tag and rearrange tags per tip tree handbook.
> 3. Slightly modify commit message to properly reflect the SOCs that may
> encounter this issue.
> 4. Rebase on top of edac-for-next.
> 
> Changes in v4:
> 1. Rebase on top of edac-for-next.
> 
> Links:
> v1: https://lore.kernel.org/all/20250327210718.1640762-1-avadhut.naik@amd.com/
> v2: https://lore.kernel.org/all/20250415213150.755255-1-avadhut.naik@amd.com/
> v3: https://lore.kernel.org/all/20250416222552.1686475-1-avadhut.naik@amd.com/
> ---

<--- ... goes here, under the --- line so that patch handling tools can ignore
it.

>  drivers/edac/amd64_edac.c | 57 ++++++++++++++++++++++++---------------
>  1 file changed, 36 insertions(+), 21 deletions(-)

...

> +static int __addr_mask_to_cs_size(u32 addr_mask, u32 addr_mask_sec,
> +				  unsigned int cs_mode, int csrow_nr, int dimm)
> +{
> +	int size;
>  
>  	edac_dbg(1, "CS%d DIMM%d AddrMasks:\n", csrow_nr, dimm);
> -	edac_dbg(1, "  Original AddrMask: 0x%x\n", addr_mask_orig);
> -	edac_dbg(1, "  Deinterleaved AddrMask: 0x%x\n", addr_mask_deinterleaved);
> +	edac_dbg(1, "  Primary AddrMask: 0x%x\n", addr_mask);
>  
>  	/* Register [31:1] = Address [39:9]. Size is in kBs here. */
> -	size = (addr_mask_deinterleaved >> 2) + 1;
> +	size = calculate_cs_size(addr_mask, cs_mode);
> +
> +	edac_dbg(1, "  Secondary AddrMask: 0x%x\n", addr_mask_sec);
> +	size += calculate_cs_size(addr_mask_sec, cs_mode);
>  
>  	/* Return size in MBs. */
>  	return size >> 10;
> @@ -1270,7 +1284,7 @@ static int umc_addr_mask_to_cs_size(struct amd64_pvt *pvt, u8 umc,
>  				    unsigned int cs_mode, int csrow_nr)
>  {
>  	int cs_mask_nr = csrow_nr;
> -	u32 addr_mask_orig;
> +	u32 addr_mask = 0, addr_mask_sec = 0;
>  	int dimm, size = 0;

The EDAC tree preferred ordering of variable declarations at the
beginning of a function is reverse fir tree order::

	struct long_struct_name *descriptive_name;
	unsigned long foo, bar;
	unsigned int tmp;
	int ret;

The above is faster to parse than the reverse ordering::

	int ret;
	unsigned int tmp;
	unsigned long foo, bar;
	struct long_struct_name *descriptive_name;

And even more so than random ordering::

	unsigned long foo, bar;
	int ret;
	struct long_struct_name *descriptive_name;
	unsigned int tmp;

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette