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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 28 Jun 2024 09:45:22 +0200
From: Borislav Petkov <bp@...en8.de>
To: John Allen <john.allen@....com>
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 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?

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