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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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);
> +}