[v4,07/23] cxl: Add callback to parse the SSLBIS subtable from CDAT

Provide a callback to parse the Switched Scoped Latency and Bandwidth
Information Structure (DSLBIS) in the CDAT structures. The SSLBIS
contains the bandwidth and latency information that's tied to the
CLX switch that the data table has been read from. The extracted
values are indexed by the downstream port id. It is possible
the downstream port id is 0xffff which is a wildcard value for any
port id.

Coherent Device Attribute Table 1.03 2.1 Switched Scoped Latency
and Bandwidth Information Structure (DSLBIS)

Signed-off-by: Dave Jiang <dave.jiang@intel.com>

---
v3:
- Add spec section in commit header (Alison)
- Move CDAT parse to cxl_switch_port_probe()
- Use 'struct node_hmem_attrs'
---
 drivers/cxl/core/cdat.c |   76 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/core/port.c |    5 +++
 drivers/cxl/cxl.h       |    1 +
 drivers/cxl/cxlpci.h    |   20 ++++++++++++
 drivers/cxl/port.c      |   14 ++++++++-
 5 files changed, 115 insertions(+), 1 deletion(-)

Dave Jiang wrote:
> Provide a callback to parse the Switched Scoped Latency and Bandwidth
> Information Structure (DSLBIS) in the CDAT structures. The SSLBIS
> contains the bandwidth and latency information that's tied to the
> CLX switch that the data table has been read from. The extracted

s/CLX/CXL/

> values are indexed by the downstream port id.

For other readers of this patch it might be worth mentioning that this
corresponds to 'struct cxl_dport::portid'.

> It is possible the downstream port id is 0xffff which is a wildcard
> value for any port id.
> 
> Coherent Device Attribute Table 1.03 2.1 Switched Scoped Latency
> and Bandwidth Information Structure (DSLBIS)
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> ---
> v3:
> - Add spec section in commit header (Alison)
> - Move CDAT parse to cxl_switch_port_probe()
> - Use 'struct node_hmem_attrs'
> ---
>  drivers/cxl/core/cdat.c |   76 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/core/port.c |    5 +++
>  drivers/cxl/cxl.h       |    1 +
>  drivers/cxl/cxlpci.h    |   20 ++++++++++++
>  drivers/cxl/port.c      |   14 ++++++++-
>  5 files changed, 115 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index e8b9bb99dfdf..ec3420dddf27 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -192,3 +192,79 @@ int cxl_dslbis_parse_entry(struct cdat_entry_header *header, void *arg)
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_dslbis_parse_entry, CXL);
> +
> +int cxl_sslbis_parse_entry(struct cdat_entry_header *header, void *arg)
> +{
> +	struct cdat_sslbis *sslbis = (struct cdat_sslbis *)header;
> +	struct xarray *sslbis_xa = arg;
> +	int remain, entries, i;
> +
> +	remain = sslbis->hdr.length - sizeof(*sslbis);
> +	if (!remain || remain % sizeof(struct sslbis_sslbe)) {
> +		pr_warn("Malformed SSLBIS table length: (%u)\n",
> +			sslbis->hdr.length);
> +		return -EINVAL;
> +	}
> +
> +	/* Unrecognized data type, we can skip */
> +	if (sslbis->data_type >= HMAT_SLLBIS_DATA_TYPE_MAX)
> +		return 0;
> +
> +	entries = remain / sizeof(*sslbis);
> +
> +	for (i = 0; i < entries; i++) {
> +		struct sslbis_sslbe *sslbe = &sslbis->sslbe[i];
> +		u16 x = le16_to_cpu(sslbe->port_x_id);
> +		u16 y = le16_to_cpu(sslbe->port_y_id);
> +		struct node_hmem_attrs *hmem_attrs;

The more "node_hmem_attrs" get reused the more it sticks out as no
longer a good name. There's no Linux "nodes" to consider in this code,
no hmem since this is switch path and not a memory node, and no sysfs
attributes (which are typically named with "_attrs"). This data
structure is just a container for passing a tuple of r/w-latency and
r/w-bandwidth numbers. It's a performance coordinate that just happens
to get reused by the hmem sysfs nodes and now CXL cdat. Perhaps 'struct
access_coordinate'?

That would also make this code more readable:

void node_set_perf_attrs(unsigned int nid, struct node_hmem_attrs *hmem_attrs,
                         unsigned access);

...vs:

void node_set_perf_attrs(unsigned int nid, struct access_coordinate *coord,
                         unsigned access);


...at least that seems more readable to me.


> +		u16 dsp_id;
> +		u64 val;
> +		int rc;
> +
> +		switch (x) {
> +		case SSLBIS_US_PORT:
> +			dsp_id = y;
> +			break;
> +		case SSLBIS_ANY_PORT:
> +			switch (y) {
> +			case SSLBIS_US_PORT:
> +				dsp_id = x;
> +				break;
> +			case SSLBIS_ANY_PORT:
> +				dsp_id = SSLBIS_ANY_PORT;
> +				break;
> +			default:
> +				dsp_id = y;
> +				break;
> +			}
> +			break;
> +		default:
> +			dsp_id = x;
> +			break;
> +		}
> +
> +		hmem_attrs = xa_load(sslbis_xa, dsp_id);
> +		if (xa_is_err(hmem_attrs))
> +			return xa_err(hmem_attrs);
> +		if (!hmem_attrs) {
> +			hmem_attrs = kzalloc(sizeof(*hmem_attrs), GFP_KERNEL);
> +			if (!hmem_attrs)
> +				return -ENOMEM;
> +		}
> +
> +		rc = check_mul_overflow(le64_to_cpu(sslbis->entry_base_unit),
> +					le16_to_cpu(sslbe->value), &val);
> +		if (unlikely(rc))
> +			pr_warn("SSLBIS value overflowed!\n");
> +
> +		cxl_hmem_attrs_set(hmem_attrs, sslbis->data_type, val);
> +		rc = xa_insert(sslbis_xa, dsp_id, hmem_attrs, GFP_KERNEL);

I'm confused why an xarray is needed. If the sslbis indicates the access
parameters from the upstream port to the downstream port, just record
that access_coordinate and point each downstream port to the same
coordinate. Why keep an xarray full of these around?

In other words just add 'struct access_coordinate' to 'struct cxl_dport'
rather maintaining this parallel array of per-downstream port data.

When / if p2p support comes along then we can worry about dport-to-dport
performance, but for this patchset those sslbis entries are 'don't care'.

> +		if (rc < 0 && rc != -EBUSY) {
> +			kfree(hmem_attrs);
> +			return rc;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_sslbis_parse_entry, CXL);