linux-kernel - Re: [PATCH v2 2/2] RAS/AMD/ATL: Translate normalized to system physical addresses using PRM

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: Mon, 1 Jul 2024 11:23:36 -0500
From: John Allen <john.allen@....com>
To: Borislav Petkov <bp@...en8.de>
Cc: rafael@...nel.org, lenb@...nel.org, yazen.ghannam@....com,
	linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-edac@...r.kernel.org
Subject: Re: [PATCH v2 2/2] RAS/AMD/ATL: Translate normalized to system
 physical addresses using PRM

On Fri, Jun 28, 2024 at 09:45:22AM +0200, Borislav Petkov wrote:
> On Mon, May 06, 2024 at 05:47:21PM +0000, John Allen wrote:
> > Future AMD platforms will provide a UEFI PRM module that implements a
> > number of address translation PRM handlers. This will provide an
> > interface for the OS to call platform specific code without requiring
> > the use of SMM or other heavy firmware operations.
> > 
> > AMD Zen-based systems report memory error addresses through Machine
> > Check banks representing Unified Memory Controllers (UMCs) in the form
> > of UMC relative "normalized" addresses. A normalized address must be
> > converted to a system physical address to be usable by the OS.
> 
> This should be your first paragraph.
> 
> > Add support for the normalized to system physical address translation
> > PRM handler in the AMD Address Translation Library and prefer it over
> > native code if available. The GUID and parameter buffer structure are
> > specific to the normalized to system physical address handler provided
> > by the address translation PRM module included in future AMD systems.
> > 
> > The address translation PRM module is documented in chapter 22 of the
> > publicly available "AMD Family 1Ah Models 00h–0Fh and Models 10h–1Fh
> > ACPI v6.5 Porting Guide":
> > https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/programmer-references/58088-0.75-pub.pdf
> 
> Those URLs are flaky and become invalid over time. When you quote
> a document, quote it in such a way so that searching for it on the web,
> can find it. The name above works for me so that's good.
> 
> > +#include "internal.h"
> > +
> > +#if defined(CONFIG_ACPI_PRMT)
> 
> Instead of that ifdeffery you could do:
> 
> config AMD_ATL_PRM
> 	depends on AMD_ATL && ACPI_PRMT
> 
> and it'll get enabled automatically and then you don't need the empty
> stub either.
> 
> > +#include <linux/prmt.h>
> > +
> > +struct prm_umc_param_buffer_norm {
> 
> What's a prm_umc_param_buffer_norm?

This is the param buffer struct used for norm -> X tranlations. I can
shorten and clarify this along with the others you pointed out. Maybe
"param_buffer_norm" instead and a comment explaining the purpose?

Thanks,
John

> 
> > +	u64 norm_addr;
> > +	u8 socket;
> > +	u64 umc_bank_inst_id;
> > +	void *output_buffer;
> 
> Use the usual short versions for such standard names: "out_buf"
> 
> > +} __packed;
> > +
> > +static const guid_t norm_to_sys_prm_handler_guid = GUID_INIT(0xE7180659, 0xA65D,
> > +							     0x451D, 0x92, 0xCD,
> > +							     0x2B, 0x56, 0xF1, 0x2B,
> > +							     0xEB, 0xA6);
> 
> When you define such long variable names, your lines stick out
> unnecessarily. Shorten pls.
> 
> > +unsigned long prm_umc_norm_to_sys_addr(u8 socket_id, u64 umc_bank_inst_id, unsigned long addr)
> 
> bank_id is fine.
> 
> > +{
> > +	struct prm_umc_param_buffer_norm param_buffer;
> 
> ... p_buf;
> 
> > +	unsigned long ret_addr;
> > +	int ret;
> > +
> > +	param_buffer.norm_addr        = addr;
> > +	param_buffer.socket           = socket_id;
> > +	param_buffer.umc_bank_inst_id = umc_bank_inst_id;
> > +	param_buffer.output_buffer    = &ret_addr;
> > +
> > +	ret = acpi_call_prm_handler(norm_to_sys_prm_handler_guid, &param_buffer);
> > +	if (!ret)
> > +		return ret_addr;
> > +
> > +	if (ret == -ENODEV)
> > +		pr_debug("PRM module/handler not available\n");
> > +	else
> > +		pr_notice_once("PRM address translation failed\n");
> > +
> > +	return ret;
> > +}
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette