Re: [PATCH v2 14/15] cxl/amd: Enable Zen5 address translation using ACPI PRMT

Jonathan Cameron
2025-03-14T13:01:56Z

On Tue, 18 Feb 2025 14:23:55 +0100
Robert Richter <rrichter@amd.com> wrote:

> Add AMD platform specific Zen5 support for address translation.
>
> Zen5 systems may be configured to use 'Normalized addresses'. Then,
> CXL endpoints use their own physical address space and are programmed
> passthrough (DPA == HPA), the number of interleaving ways for the
> endpoint is set to one. The Host Physical Addresses (HPAs) need to be
> translated from the endpoint to its CXL host bridge. The HPA of a CXL
> host bridge is equivalent to the System Physical Address (SPA).
>
> ACPI Platform Runtime Mechanism (PRM) is used to translate the CXL
> Device Physical Address (DPA) to its System Physical Address. This is
> documented in:
>
> AMD Family 1Ah Models 00h–0Fh and Models 10h–1Fh
> ACPI v6.5 Porting Guide, Publication # 58088
> https://www.amd.com/en/search/documentation/hub.html
>
> To implement AMD Zen5 address translation the following steps are
> needed:
>
> Apply platform specific changes to each port where necessary using
> platform detection and the existing architectural framework.
>
> Add a function cxl_port_setup_amd() to implement AMD platform specific
> code. Use Kbuild and Kconfig options respectively to enable the code
> depending on architecture and platform options. Handle architecture
> specifics in arch_cxl_port_platform_setup() and create a new file
> core/x86/amd.c for this.
>
> Introduce a function cxl_zen5_init() to handle Zen5 specific
> enablement. Zen5 platforms are detected using the PCIe vendor and
> device ID of the corresponding CXL root port. There is a check for
> ACPI PRMT CXL address translation support.
>
> Apply cxl_zen5_to_hpa() as cxl_port->to_hpa() callback to Zen5 CXL
> host bridges to enable platform specific address translation.
>
> Use ACPI PRM DPA-to-SPA translation to determine an endpoint's
> interleaving configuration and base address during the early
> initialization process. This is used to determine an endpoint's SPA
> range and to check the address translation setup.
>
> The configuration can be determined calling the PRM for specific HPAs
> given. The resulting SPAs are used to calculate interleaving
> parameters of the host bridge and root port. The maximum granularity
> (chunk size) is 16k, minimum is 256. Use the following calculation for
> the given HPAs:
>
> ways = hpa_len(SZ_16K) / SZ_16K
> gran = (hpa_len(SZ_16K) - hpa_len(SZ_16K - SZ_256) - SZ_256)
> / (ways - 1)
> pos = (hpa_len(SZ_16K) - ways * SZ_16K) / gran
>
> Before the endpoint is attached to a region the translation is checked
> for reasonable values.
>
> Signed-off-by: Robert Richter <rrichter@amd.com>
Some trivial comments inline. The rest I may take another look at as
not gotten my head fully around it yet.

> diff --git a/drivers/cxl/core/x86/amd.c b/drivers/cxl/core/x86/amd.c
> new file mode 100644
> index 000000000000..483c92c18054
> --- /dev/null
> +++ b/drivers/cxl/core/x86/amd.c
> @@ -0,0 +1,259 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2025 Advanced Micro Devices, Inc.
> + */
> +
> +#include <linux/prmt.h>
> +#include <linux/pci.h>
> +
> +#include <cxlmem.h>
> +#include "../core.h"
> +
> +#define PCI_DEVICE_ID_AMD_ZEN5_ROOT 0x153e

Might as well just put it inline unless you have lots of uses.

> +
> +static const struct pci_device_id zen5_root_port_ids[] = {
> + { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_ZEN5_ROOT) },
> + {},
{}

Don't want to make it easy to add stuff after.

> +};
> +
> +static int is_zen5_root_port(struct device *dev, void *unused)
> +{
> + if (!dev_is_pci(dev))
> + return 0;
> +
> + return !!pci_match_id(zen5_root_port_ids, to_pci_dev(dev));
> +}
> +
> +static bool is_zen5(struct cxl_port *port)
> +{
> + if (!IS_ENABLED(CONFIG_ACPI_PRMT))
> + return false;
> +
> + /* To get the CXL root port, find the CXL host bridge first. */
> + if (is_cxl_root(port) ||
> + !port->host_bridge ||
combine a few of these on same line perhaps?
> + !is_cxl_root(to_cxl_port(port->dev.parent)))
> + return false;

> +
> +/* Bits used for interleaving. */
> +#define SPA_INTERLEAVING_BITS GENMASK_ULL(14, 8)
> +
> +static u64 cxl_zen5_to_hpa(struct cxl_decoder *cxld, u64 hpa)
> +{

> + port = to_cxl_port(cxld->dev.parent);
> + cxlmd = port ? to_cxl_memdev(port->uport_dev) : NULL;
> + if (!port || !dev_is_pci(cxlmd->dev.parent)) {

Given you are going to dereference cxlmd, maybe more logical to do

if (!cxlmd || !dev_is_pci()

> + dev_dbg(&cxld->dev, "No endpoint found: %s, range %#llx-%#llx\n",
> + dev_name(cxld->dev.parent), cxld->hpa_range.start,
> + cxld->hpa_range.end);
> + return ULLONG_MAX;
> + }
> + pci_dev = to_pci_dev(cxlmd->dev.parent);
> +
> + /*
> + * If the decoder is already attached we are past the decoder
> + * initialization, do not determine the address mapping and
> + * just return here.
> + */
> + if (cxld->region)
> + return prm_cxl_dpa_spa(pci_dev, hpa);
> +
>
> + /*
> + * Check the mapping: Number of ways is power of 2 or a
> + * multiple of 3 ways (len == ways * SZ_16K), granularitys is

granularity is

> + * power of 2.
> + */
> + if (len & ~(3ULL << (ways_bits + 14)) ||
> + granularity != 1 << gran_bits || offset != pos << gran_bits) {
> + dev_dbg(&cxld->dev, "Error determining address mapping: base: %#llx spa: %#llx spa2: %#llx ways: %d pos: %d granularity: %llu\n",
> + base, spa, spa2, ways, pos, granularity);
> + return ULLONG_MAX;
> + }
> +
> + spa = prm_cxl_dpa_spa(pci_dev, hpa);
> +
> + /*
> + * Check SPA using a PRM call for the closest DPA calculated
> + * for the HPA. If the HPA matches a different interleaving
> + * position other than the decoder's, determine its offset to
> + * adjust the SPA.
> + */
> +
> + gran_mask = GENMASK_ULL(gran_bits, 0);
> + spa2 = base + (hpa & ~gran_mask) * ways + (hpa & gran_mask);
> + base = base - pos * granularity;
> +
> + dev_dbg(&cxld->dev,
> + "address mapping found for %s (hpa -> spa): %#llx -> %#llx (%#llx+%#llx) ways: %d pos: %d granularity: %llu\n",
> + pci_name(pci_dev), hpa, spa, base, spa - base, ways, pos,
> + granularity);
> +
> +
> + if ((spa ^ spa2) & ~SPA_INTERLEAVING_BITS) {
> + dev_dbg(&cxld->dev, "SPA calculation failed: %#llx:%#llx\n",
> + spa, spa2);
> + return ULLONG_MAX;
> + }
> +
> + return spa;
> +}
> +
> +static void cxl_zen5_init(struct cxl_port *port)
> +{
> + u64 spa;
> + struct prm_cxl_dpa_spa_data data = { .out = &spa, };
> + int rc;
> +
> + if (!is_zen5(port))
> + return;
> +
> + /* Check kernel and firmware support */
> + rc = acpi_call_prm_handler(prm_cxl_dpa_spa_guid, &data);
> +
No blank line here. Keen the error check right up against the call.
> + if (rc == -EOPNOTSUPP) {
> + pr_warn_once("ACPI PRMT: PRM address translation not supported by kernel\n");
> + return;
> + }
> +
> + if (rc == -ENODEV) {
> + pr_warn_once("ACPI PRMT: PRM address translation not supported by firmware\n");
> + return;
> + }
> +
> + port->to_hpa = cxl_zen5_to_hpa;
> +
> + dev_dbg(port->host_bridge, "PRM address translation enabled for %s.\n",
> + dev_name(&port->dev));
> +}
> +
> +void cxl_port_setup_amd(struct cxl_port *port)
> +{
> + cxl_zen5_init(port);
> +}