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, ¶m_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
