linux-kernel - Re: [PATCH v4 01/24] EDAC/amd64: Define Data Fabric operations

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:   Fri, 11 Feb 2022 20:10:43 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Yazen Ghannam <yazen.ghannam@....com>
Cc:     linux-edac@...r.kernel.org, linux-kernel@...r.kernel.org,
        mchehab@...nel.org, tony.luck@...el.com, james.morse@....com,
        rric@...nel.org, Smita.KoralahalliChannabasappa@....com
Subject: Re: [PATCH v4 01/24] EDAC/amd64: Define Data Fabric operations

On Thu, Jan 27, 2022 at 08:40:52PM +0000, Yazen Ghannam wrote:
> Define a stub to hold operations for different Data Fabric versions.
> This will be filled in following patches.
> 
> Set the operations at init-time as appropriate for each model/family
> group.
> 
> Also, start a glossary of acronyms used in the translation code.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@....com>
> ---
> Link:
> https://lore.kernel.org/r/20211028175728.121452-6-yazen.ghannam@amd.com
> 
> v3->v4:
> * Started glossary.
> * Included pr_debug() for failing case.
> 
> v2->v3:
> * Was patch 6 in v2.
> * "df_ops" is set at init time.
> 
> v1->v2:
> * New in v2.
> 
>  drivers/edac/amd64_edac.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index fba609ada0e6..639dfbea3348 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -988,6 +988,12 @@ static int sys_addr_to_csrow(struct mem_ctl_info *mci, u64 sys_addr)
>  	return csrow;
>  }
>  
> +/*
> + * Glossary of acronyms used in address translation for Zen-based systems
> + *
> + * DF          =       Data Fabric

Yeah, "DF: Data Fabric" is probably easier to parse, without that weird
spacing and equals sign.

> + */
> +
>  /* Protect the PCI config register pairs used for DF indirect access. */
>  static DEFINE_MUTEX(df_indirect_mutex);
>  
> @@ -1058,6 +1064,14 @@ struct addr_ctx {
>  	u8 inst_id;
>  };
>  
> +struct data_fabric_ops {
> +};

I know that this is not the only example but we have struct definitions
interspersed with functions in the .c file while former should be all in
the header. It is a lot cleaner to have definitions and inline functions
in the header and the actual functionality in the C file but I leave it
up to you to decide what you prefer.

> +
> +struct data_fabric_ops df2_ops = {
> +};
> +
> +struct data_fabric_ops *df_ops;
> +
>  static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
>  {
>  	u64 dram_base_addr, dram_limit_addr, dram_hole_base;
> @@ -1072,6 +1086,11 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
>  
>  	struct addr_ctx ctx;
>  
> +	if (!df_ops) {
> +		pr_debug("Data Fabric Operations not set");

That probably wants to be a WARN_ON_ONCE() so that it is loud and
prominent when it happens...

-- 
Regards/Gruss,
    Boris.

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