Re: [PATCH v1 25/29] cxl/amd: Enable Zen5 address translation using ACPI PRMT

[Gregory Price]

On Tue, Jan 07, 2025 at 03:10:11PM +0100, Robert Richter wrote:
> Add AMD platform specific Zen5 support for address translation.

Doing some testing here and I'm seeing some odd results, also noticing
some naming inconsistencies

> 
> +static u64 cxl_zen5_to_hpa(struct cxl_decoder *cxld, u64 hpa)
> +{

Function name is _to_hpa, but hpa is an argument?

Should be dpa as argument? Confusing to convert an hpa to an hpa.

... snip ...

> +#define DPA_MAGIC	0xd20000
> +	base = prm_cxl_dpa_spa(pci_dev, DPA_MAGIC);
> +	spa  = prm_cxl_dpa_spa(pci_dev, DPA_MAGIC + SZ_16K);
> +	spa2 = prm_cxl_dpa_spa(pci_dev, DPA_MAGIC + SZ_16K - SZ_256);

For two devices interleaved, the base should be the same, correct?

example: 2 128GB devices interleaved/normalized:

dev0: base(0xc051a40000) spa(0xc051a48000) spa2(0xc051a47e00)
dev1: base(0xc051a40100) spa(0xc051a48100) spa2(0xc051a47f00)

I believe these numbers are correct.

(Note: Using PRMT emulation because I don't have a BIOS with this blob,
 but this is the same emulation i have been using for about 4 months now
 with operational hardware, so unless the translation contract changed
 and this code expects something different, it should be correct).

... snip ...
> +	len = spa - base;
> +	len2 = spa2 - base;
> +
> +	/* offset = pos * granularity */
> +	if (len == SZ_16K && len2 == SZ_16K - SZ_256) {
> +		ways = 1;
> +		offset = 0;
> +		granularity = 0;
> +		pos = 0;
> +	} else {
> +		ways = len / SZ_16K;
> +		offset = spa & (SZ_16K - 1);
> +		granularity = (len - len2 - SZ_256) / (ways - 1);
> +		pos = offset / granularity;
> +	}

the interleave ways and such calculate out correctly

dev0: ways(0x2) offset(0x0)   granularity(0x100) pos(0x0)
dev1: ways(0x2) offset(0x100) granularity(0x100) pos(0x1)

> +
> +	base = base - DPA_MAGIC * ways - pos * granularity;
> +	spa = base + hpa;

DPA(0)
dev0: base(0xc050000000) spa(0xc050000000)
dev1: base(0xc050000000) spa(0xc050000000)

DPA(0x1fffffffff)
dev0: base(0xc050000000) spa(0xe04fffffff)
dev1: base(0xc050000000) spa(0xe04fffffff)

The bases seems correct, the SPAs looks suspect.

dev1 should have a very different SPA shouldn't it?

> +
> +	/*
> +	 * 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.
> +	 */
> +
> +	dpa = (hpa & ~(granularity * ways - 1)) / ways
> +		+ (hpa & (granularity - 1));

I do not understand this chunk here, we seem to just be chopping the HPA
in half to acquire the DPA.  But the value passed in is already a DPA.

dpa = (0x1fffffffff & ~(256 * 2 - 1)) / 2 + (0x1fffffffff & (256 - 1))
    =  0xfffffffff

I don't understand why the DPA address is suddenly half (64GB boundary).

> +	offset = hpa & (granularity * ways - 1) & ~(granularity - 1);
> +	offset -= pos * granularity;
> +	spa2 = prm_cxl_dpa_spa(pci_dev, dpa) + offset;
> +
> +	dev_dbg(&cxld->dev,
> +		"address mapping found for %s (dpa -> hpa -> spa): %#llx -> %#llx -> %#llx base: %#llx ways: %d pos: %d granularity: %llu\n",
> +		pci_name(pci_dev), dpa, hpa, spa, base, ways, pos, granularity);
> +

This results in a translation that appears to be wrong:

dev0:
cxl decoder5.0: address mapping found for 0000:e1:00.0
    (dpa -> hpa -> spa): 0x0 -> 0x0 -> 0xc050000000
    base: 0xc050000000 ways: 2 pos: 0 granularity: 256
cxl decoder5.0: address mapping found for 0000:e1:00.0
    (dpa -> hpa -> spa): 0xfffffffff -> 0x1fffffffff -> 0xe04fffffff
    base: 0xc050000000 ways: 2 pos: 0 granularity: 256

dev1:
cxl decoder6.0: address mapping found for 0000:c1:00.0
	(dpa -> hpa -> spa): 0x0 -> 0x0 -> 0xc050000000 
	base: 0xc050000000 ways: 2 pos: 1 granularity: 256
cxl decoder6.0: address mapping found for 0000:c1:00.0
	(dpa -> hpa -> spa): 0xfffffffff -> 0x1fffffffff -> 0xe04fffffff
	base: 0xc050000000 ways: 2 pos: 1 granularity: 256

These do not look correct.

Is my understanding of the PRMT translation incorrect?
I expect the following: (assuming one contiguous CFMW)

dev0 (dpa -> hpa -> spa): 0x0          -> 0x0          -> 0xc050000000
dev1 (dpa -> hpa -> spa): 0x0          -> 0x100        -> 0xc050000100
dev0 (dpa -> hpa -> spa): 0x1fffffffff -> 0x3ffffffeff -> 0x1004ffffeff
dev1 (dpa -> hpa -> spa): 0x1fffffffff -> 0x3fffffffff -> 0x1004fffffff

Extra data: here are the programmed endpoint decoder values

[endpoint5/decoder5.0]# cat start size dpa_size interleave_ways interleave_granularity 
0x0
0x2000000000
0x0000002000000000
1
256

[endpoint6/decoder6.0]# cat start size dpa_size interleave_ways interleave_granularity 
0x0
0x2000000000
0x0000002000000000
1
256


Anyway, yeah I'm a bit confused how this is all supposed to actually
work given that both devices translate to the same addresses.

In theory this *should* work since the root decoder covers the whole
space - as this has been working for me previously with some hacked up
PRMT emulation code.

[decoder0.0]# cat start size interleave_ways interleave_granularity
0xc050000000
0x4000000000
2
256

[decoder1.0]# cat start size interleave_ways interleave_granularity
0xc050000000
0x4000000000
1
256

[decoder3.0]# cat start size interleave_ways interleave_granularity 
0xc050000000
0x4000000000
1
256

[decoder5.0]# cat start size interleave_ways interleave_granularity
0x0
0x2000000000
1
256

[decoder6.0]# cat start size interleave_ways interleave_granularity 
0x0
0x2000000000
1
256

~Gregory