[PATCH v2 0/2] PRM handler direct call interface

[PATCH v2 0/2] PRM handler direct call interface

[John Allen]

Platform Runtime Mechanism (PRM) introduces a means for the AML
interpreter and OS drivers to invoke runtime handlers from platform
firmware in order to remove the need for certain classes of SMIs.
Further details can be seen in the PRM specification[1].

Future AMD platforms will implement a PRM module in firmware that will
include handlers for performing various types of address translation.
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"[2].

While the kernel currently has support for calling PRM handlers from the
AML interpreter, it does not support calling PRM handlers directly from
OS drivers. This series implements the direct call interface and uses it
for translating normalized addresses to system physical addresses.

Thanks,
John

[1]:
https://uefi.org/sites/default/files/resources/Platform%20Runtime%20Mechanism%20-%20with%20legal%20notice.pdf
[2]:
https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/programmer-references/58088-0.75-pub.pdf

Tree: git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
Base commit: 4cece764965020c22cff7665b18a012006359095

John Allen (2):
  ACPI: PRM: Add PRM handler direct call support
  RAS/AMD/ATL: Translate normalized to system physical addresses using
    PRM

 drivers/acpi/prmt.c            | 24 +++++++++++++
 drivers/ras/amd/atl/Makefile   |  1 +
 drivers/ras/amd/atl/internal.h |  2 ++
 drivers/ras/amd/atl/prm.c      | 61 ++++++++++++++++++++++++++++++++++
 drivers/ras/amd/atl/umc.c      |  5 +++
 include/linux/prmt.h           |  5 +++
 6 files changed, 98 insertions(+)
 create mode 100644 drivers/ras/amd/atl/prm.c

-- 
2.34.1

[PATCH v2 1/2] ACPI: PRM: Add PRM handler direct call support

[John Allen]

Platform Runtime Mechanism (PRM) handlers can be invoked from either the
AML interpreter or directly by an OS driver. Implement the direct call
method.

Export the symbol as this will be used by modules such as the AMD
Address Translation Library and likely others in the future.

Signed-off-by: John Allen <john.allen@amd.com>
Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
v2:
  - Align statements setting fields in context buffer on '='
---
 drivers/acpi/prmt.c  | 24 ++++++++++++++++++++++++
 include/linux/prmt.h |  5 +++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c
index c78453c74ef5..1cfaa5957ac4 100644
--- a/drivers/acpi/prmt.c
+++ b/drivers/acpi/prmt.c
@@ -214,6 +214,30 @@ static struct prm_handler_info *find_prm_handler(const guid_t *guid)
 #define UPDATE_LOCK_ALREADY_HELD 	4
 #define UPDATE_UNLOCK_WITHOUT_LOCK 	5
 
+int acpi_call_prm_handler(guid_t handler_guid, void *param_buffer)
+{
+	struct prm_handler_info *handler = find_prm_handler(&handler_guid);
+	struct prm_module_info *module = find_prm_module(&handler_guid);
+	struct prm_context_buffer context;
+	efi_status_t status;
+
+	if (!module || !handler)
+		return -ENODEV;
+
+	memset(&context, 0, sizeof(context));
+	ACPI_COPY_NAMESEG(context.signature, "PRMC");
+	context.identifier         = handler->guid;
+	context.static_data_buffer = handler->static_data_buffer_addr;
+	context.mmio_ranges        = module->mmio_info;
+
+	status = efi_call_acpi_prm_handler(handler->handler_addr,
+					   (u64)param_buffer,
+					   &context);
+
+	return efi_status_to_err(status);
+}
+EXPORT_SYMBOL_GPL(acpi_call_prm_handler);
+
 /*
  * This is the PlatformRtMechanism opregion space handler.
  * @function: indicates the read/write. In fact as the PlatformRtMechanism
diff --git a/include/linux/prmt.h b/include/linux/prmt.h
index 24da8364b919..9c094294403f 100644
--- a/include/linux/prmt.h
+++ b/include/linux/prmt.h
@@ -2,6 +2,11 @@
 
 #ifdef CONFIG_ACPI_PRMT
 void init_prmt(void);
+int acpi_call_prm_handler(guid_t handler_guid, void *param_buffer);
 #else
 static inline void init_prmt(void) { }
+static inline int acpi_call_prm_handler(guid_t handler_guid, void *param_buffer)
+{
+	return -EOPNOTSUPP;
+}
 #endif
-- 
2.34.1

Re: [PATCH v2 1/2] ACPI: PRM: Add PRM handler direct call support

[Rafael J. Wysocki]

On Mon, May 6, 2024 at 7:48 PM John Allen <john.allen@amd.com> wrote:
>
> Platform Runtime Mechanism (PRM) handlers can be invoked from either the
> AML interpreter or directly by an OS driver. Implement the direct call
> method.
>
> Export the symbol as this will be used by modules such as the AMD
> Address Translation Library and likely others in the future.
>
> Signed-off-by: John Allen <john.allen@amd.com>
> Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
> v2:
>   - Align statements setting fields in context buffer on '='

I would actually prefer spaces around the "=" there, but anyway

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/acpi/prmt.c  | 24 ++++++++++++++++++++++++
>  include/linux/prmt.h |  5 +++++
>  2 files changed, 29 insertions(+)
>
> diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c
> index c78453c74ef5..1cfaa5957ac4 100644
> --- a/drivers/acpi/prmt.c
> +++ b/drivers/acpi/prmt.c
> @@ -214,6 +214,30 @@ static struct prm_handler_info *find_prm_handler(const guid_t *guid)
>  #define UPDATE_LOCK_ALREADY_HELD       4
>  #define UPDATE_UNLOCK_WITHOUT_LOCK     5
>
> +int acpi_call_prm_handler(guid_t handler_guid, void *param_buffer)
> +{
> +       struct prm_handler_info *handler = find_prm_handler(&handler_guid);
> +       struct prm_module_info *module = find_prm_module(&handler_guid);
> +       struct prm_context_buffer context;
> +       efi_status_t status;
> +
> +       if (!module || !handler)
> +               return -ENODEV;
> +
> +       memset(&context, 0, sizeof(context));
> +       ACPI_COPY_NAMESEG(context.signature, "PRMC");
> +       context.identifier         = handler->guid;
> +       context.static_data_buffer = handler->static_data_buffer_addr;
> +       context.mmio_ranges        = module->mmio_info;
> +
> +       status = efi_call_acpi_prm_handler(handler->handler_addr,
> +                                          (u64)param_buffer,
> +                                          &context);
> +
> +       return efi_status_to_err(status);
> +}
> +EXPORT_SYMBOL_GPL(acpi_call_prm_handler);
> +
>  /*
>   * This is the PlatformRtMechanism opregion space handler.
>   * @function: indicates the read/write. In fact as the PlatformRtMechanism
> diff --git a/include/linux/prmt.h b/include/linux/prmt.h
> index 24da8364b919..9c094294403f 100644
> --- a/include/linux/prmt.h
> +++ b/include/linux/prmt.h
> @@ -2,6 +2,11 @@
>
>  #ifdef CONFIG_ACPI_PRMT
>  void init_prmt(void);
> +int acpi_call_prm_handler(guid_t handler_guid, void *param_buffer);
>  #else
>  static inline void init_prmt(void) { }
> +static inline int acpi_call_prm_handler(guid_t handler_guid, void *param_buffer)
> +{
> +       return -EOPNOTSUPP;
> +}
>  #endif
> --
> 2.34.1
>

Re: [PATCH v2 1/2] ACPI: PRM: Add PRM handler direct call support

[Borislav Petkov]

On Mon, May 06, 2024 at 05:47:20PM +0000, John Allen wrote:
> Platform Runtime Mechanism (PRM) handlers can be invoked from either the
> AML interpreter or directly by an OS driver. Implement the direct call
> method.
> 
> Export the symbol as this will be used by modules such as the AMD
> Address Translation Library and likely others in the future.
> 
> Signed-off-by: John Allen <john.allen@amd.com>
> Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
> v2:
>   - Align statements setting fields in context buffer on '='
> ---
>  drivers/acpi/prmt.c  | 24 ++++++++++++++++++++++++
>  include/linux/prmt.h |  5 +++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c
> index c78453c74ef5..1cfaa5957ac4 100644
> --- a/drivers/acpi/prmt.c
> +++ b/drivers/acpi/prmt.c
> @@ -214,6 +214,30 @@ static struct prm_handler_info *find_prm_handler(const guid_t *guid)
>  #define UPDATE_LOCK_ALREADY_HELD 	4
>  #define UPDATE_UNLOCK_WITHOUT_LOCK 	5
>  
> +int acpi_call_prm_handler(guid_t handler_guid, void *param_buffer)
> +{
> +	struct prm_handler_info *handler = find_prm_handler(&handler_guid);
> +	struct prm_module_info *module = find_prm_module(&handler_guid);
> +	struct prm_context_buffer context;
> +	efi_status_t status;
> +
> +	if (!module || !handler)
> +		return -ENODEV;
> +
> +	memset(&context, 0, sizeof(context));
> +	ACPI_COPY_NAMESEG(context.signature, "PRMC");
> +	context.identifier         = handler->guid;
> +	context.static_data_buffer = handler->static_data_buffer_addr;
> +	context.mmio_ranges        = module->mmio_info;
> +
> +	status = efi_call_acpi_prm_handler(handler->handler_addr,
> +					   (u64)param_buffer,
> +					   &context);
> +
> +	return efi_status_to_err(status);
> +}

+ linux-efi as Rafael wanted to make sure the environment is created properly
for the EFI runtime services call...

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 1/2] ACPI: PRM: Add PRM handler direct call support

[Ard Biesheuvel]

On Thu, 27 Jun 2024 at 10:08, Borislav Petkov <bp@alien8.de> wrote:
>
> On Mon, May 06, 2024 at 05:47:20PM +0000, John Allen wrote:
> > Platform Runtime Mechanism (PRM) handlers can be invoked from either the
> > AML interpreter or directly by an OS driver. Implement the direct call
> > method.
> >
> > Export the symbol as this will be used by modules such as the AMD
> > Address Translation Library and likely others in the future.
> >
> > Signed-off-by: John Allen <john.allen@amd.com>
> > Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>
> > ---
> > v2:
> >   - Align statements setting fields in context buffer on '='
> > ---
> >  drivers/acpi/prmt.c  | 24 ++++++++++++++++++++++++
> >  include/linux/prmt.h |  5 +++++
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c
> > index c78453c74ef5..1cfaa5957ac4 100644
> > --- a/drivers/acpi/prmt.c
> > +++ b/drivers/acpi/prmt.c
> > @@ -214,6 +214,30 @@ static struct prm_handler_info *find_prm_handler(const guid_t *guid)
> >  #define UPDATE_LOCK_ALREADY_HELD     4
> >  #define UPDATE_UNLOCK_WITHOUT_LOCK   5
> >
> > +int acpi_call_prm_handler(guid_t handler_guid, void *param_buffer)
> > +{
> > +     struct prm_handler_info *handler = find_prm_handler(&handler_guid);
> > +     struct prm_module_info *module = find_prm_module(&handler_guid);
> > +     struct prm_context_buffer context;
> > +     efi_status_t status;
> > +
> > +     if (!module || !handler)
> > +             return -ENODEV;
> > +
> > +     memset(&context, 0, sizeof(context));
> > +     ACPI_COPY_NAMESEG(context.signature, "PRMC");
> > +     context.identifier         = handler->guid;
> > +     context.static_data_buffer = handler->static_data_buffer_addr;
> > +     context.mmio_ranges        = module->mmio_info;
> > +
> > +     status = efi_call_acpi_prm_handler(handler->handler_addr,
> > +                                        (u64)param_buffer,
> > +                                        &context);
> > +
> > +     return efi_status_to_err(status);
> > +}
>
> + linux-efi as Rafael wanted to make sure the environment is created properly
> for the EFI runtime services call...
>

This looks fine to me.

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

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

[John Allen]

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.

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

Signed-off-by: John Allen <john.allen@amd.com>
---
v2:
  - Make norm_to_sys_prm_handler_guid static.
  - Change pr_info statements to more appropriate pr_debug and
    pr_info_once statements.
---
 drivers/ras/amd/atl/Makefile   |  1 +
 drivers/ras/amd/atl/internal.h |  2 ++
 drivers/ras/amd/atl/prm.c      | 61 ++++++++++++++++++++++++++++++++++
 drivers/ras/amd/atl/umc.c      |  5 +++
 4 files changed, 69 insertions(+)
 create mode 100644 drivers/ras/amd/atl/prm.c

diff --git a/drivers/ras/amd/atl/Makefile b/drivers/ras/amd/atl/Makefile
index 4acd5f05bd9c..8f1afa793e3b 100644
--- a/drivers/ras/amd/atl/Makefile
+++ b/drivers/ras/amd/atl/Makefile
@@ -14,5 +14,6 @@ amd_atl-y		+= denormalize.o
 amd_atl-y		+= map.o
 amd_atl-y		+= system.o
 amd_atl-y		+= umc.o
+amd_atl-y		+= prm.o
 
 obj-$(CONFIG_AMD_ATL)	+= amd_atl.o
diff --git a/drivers/ras/amd/atl/internal.h b/drivers/ras/amd/atl/internal.h
index 5de69e0bb0f9..f739dcada126 100644
--- a/drivers/ras/amd/atl/internal.h
+++ b/drivers/ras/amd/atl/internal.h
@@ -234,6 +234,8 @@ int dehash_address(struct addr_ctx *ctx);
 unsigned long norm_to_sys_addr(u8 socket_id, u8 die_id, u8 coh_st_inst_id, unsigned long addr);
 unsigned long convert_umc_mca_addr_to_sys_addr(struct atl_err *err);
 
+unsigned long prm_umc_norm_to_sys_addr(u8 socket_id, u64 umc_bank_inst_id, unsigned long addr);
+
 /*
  * Make a gap in @data that is @num_bits long starting at @bit_num.
  * e.g. data		= 11111111'b
diff --git a/drivers/ras/amd/atl/prm.c b/drivers/ras/amd/atl/prm.c
new file mode 100644
index 000000000000..8e96a6370ae3
--- /dev/null
+++ b/drivers/ras/amd/atl/prm.c
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * AMD Address Translation Library
+ *
+ * prm.c : Plumbing code to UEFI Platform Runtime Mechanism (PRM)
+ *
+ * Copyright (c) 2024, Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * Author: John Allen <john.allen@amd.com>
+ */
+
+#include "internal.h"
+
+#if defined(CONFIG_ACPI_PRMT)
+
+#include <linux/prmt.h>
+
+struct prm_umc_param_buffer_norm {
+	u64 norm_addr;
+	u8 socket;
+	u64 umc_bank_inst_id;
+	void *output_buffer;
+} __packed;
+
+static const guid_t norm_to_sys_prm_handler_guid = GUID_INIT(0xE7180659, 0xA65D,
+							     0x451D, 0x92, 0xCD,
+							     0x2B, 0x56, 0xF1, 0x2B,
+							     0xEB, 0xA6);
+
+unsigned long prm_umc_norm_to_sys_addr(u8 socket_id, u64 umc_bank_inst_id, unsigned long addr)
+{
+	struct prm_umc_param_buffer_norm param_buffer;
+	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;
+}
+
+#else /* ACPI_PRMT */
+
+unsigned long prm_umc_norm_to_sys_addr(u8 socket_id, u64 umc_bank_inst_id, unsigned long addr)
+{
+	return -ENODEV;
+}
+
+#endif
diff --git a/drivers/ras/amd/atl/umc.c b/drivers/ras/amd/atl/umc.c
index 08c6dbd44c62..3c018870633c 100644
--- a/drivers/ras/amd/atl/umc.c
+++ b/drivers/ras/amd/atl/umc.c
@@ -333,9 +333,14 @@ unsigned long convert_umc_mca_addr_to_sys_addr(struct atl_err *err)
 	u8 coh_st_inst_id = get_coh_st_inst_id(err);
 	unsigned long addr = get_addr(err->addr);
 	u8 die_id = get_die_id(err);
+	unsigned long ret_addr;
 
 	pr_debug("socket_id=0x%x die_id=0x%x coh_st_inst_id=0x%x addr=0x%016lx",
 		 socket_id, die_id, coh_st_inst_id, addr);
 
+	ret_addr = prm_umc_norm_to_sys_addr(socket_id, err->ipid, addr);
+	if (!IS_ERR_VALUE(ret_addr))
+		return ret_addr;
+
 	return norm_to_sys_addr(socket_id, die_id, coh_st_inst_id, addr);
 }
-- 
2.34.1

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

[Borislav Petkov]

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

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

[John Allen]

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

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

[Borislav Petkov]

On Mon, Jul 01, 2024 at 11:23:36AM -0500, John Allen wrote:
> 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?

What's wrong with

struct param_buffer

simply?

It is obvious from the code that it is used in the normalized -> physical
address translation.

Btw, pls do me a favor and trim your replies like I just did.

Thx.

-- 
Regards/Gruss,
    Boris.

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

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

[John Allen]

On Mon, Jul 01, 2024 at 06:35:05PM +0200, Borislav Petkov wrote:
> On Mon, Jul 01, 2024 at 11:23:36AM -0500, John Allen wrote:
> > 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?
> 
> What's wrong with
> 
> struct param_buffer
> 
> simply?
> 
> It is obvious from the code that it is used in the normalized -> physical
> address translation.

This is because the spec defines different param buffer structures for
different types of translation. We can call this just param_buffer for
now, but would need to be renamed if/when we add use cases for the other
translation handlers. My preference would be to sort of future-proof
the name now, but I don't have an issue calling it param_buffer now and
changing it later if that's what you'd prefer.

> Btw, pls do me a favor and trim your replies like I just did.

Sure, sorry about that.

Thanks,
John

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

[Borislav Petkov]

On Mon, Jul 01, 2024 at 11:43:35AM -0500, John Allen wrote:
> This is because the spec defines different param buffer structures for
> different types of translation. We can call this just param_buffer for
> now, but would need to be renamed if/when we add use cases for the other
> translation handlers. My preference would be to sort of future-proof
> the name now, but I don't have an issue calling it param_buffer now and
> changing it later if that's what you'd prefer.

Sure:

/*
 * PRM parameter buffer - normalized to system physical address, as described
 * in the section "PRM Parameter Buffer" in the aforementioned spec.
 */
struct norm_to_spa_param_buf {
	...
} __packed;

and you'll have to mention the spec in the prm.c file, at the top.

This way readers can immediately map it to the place in the spec.

Thx.

-- 
Regards/Gruss,
    Boris.

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

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

[John Allen]

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:
> > +#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.

I'm not sure this works the way we need it to. If ACPI_PRMT is not
enabled, then the norm to sys translation function will be referenced by
the base AMD ATL, but will the symbol will not be found since the the
PRM file doesn't get compiled.

I added the AMD_ATL_PRM config and added the following to the ATL
Makefile:
amd_atl-$(CONFIG_AMD_ATL_PRM) += prm.o

instead of:

amd_atl-y		+= prm.o

Is there another way you had in mind to make the additional config
option work as expected?

Thanks,
John

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

[Borislav Petkov]

On Wed, Jul 03, 2024 at 03:14:53PM -0500, John Allen wrote:
> I'm not sure this works the way we need it to. If ACPI_PRMT is not
> enabled, then the norm to sys translation function will be referenced by
> the base AMD ATL, but will the symbol will not be found since the the
> PRM file doesn't get compiled.

So you don't delete the stub but you put it in the internal.h header:

#ifndef CONFIG_AMD_ATL_PRM
+unsigned long prm_umc_norm_to_sys_addr(u8 socket_id, u64 umc_bank_inst_id, unsigned long addr)
+{
+       return -ENODEV;
+}
+
+#endif

Don't be afraid to grep the tree - there are gazillion examples how stuff like
that is usually done.

-- 
Regards/Gruss,
    Boris.

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