On Wed, Apr 13, 2022 at 11:38 AM Ben Widawsky <[email protected]> wrote:
>
> CXL HDM decoders have distinct properties at each level in the
> hierarchy. Root decoders manage host physical address space. Switch
> decoders manage demultiplexing of data to downstream targets. Endpoint
> decoders must be aware of physical media size constraints. To properly
> support these unique needs, create these unique structures.
>
> CXL HDM decoders do have similar architectural properties at all levels:
> interleave properties, flags, types and consumption of host physical
> address space. Those are retained and when possible, still utilized.
>
> Signed-off-by: Ben Widawsky <[email protected]>
> ---
> drivers/cxl/core/hdm.c | 3 +-
> drivers/cxl/core/port.c | 102 ++++++++++++++++++++++++-----------
> drivers/cxl/cxl.h | 69 +++++++++++++++++++++---
> tools/testing/cxl/test/cxl.c | 2 +-
> 4 files changed, 137 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 3055e246aab9..37c09c77e9a7 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -6,6 +6,7 @@
>
> #include "cxlmem.h"
> #include "core.h"
> +#include "cxl.h"
>
> /**
> * DOC: cxl core hdm
> @@ -242,7 +243,7 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
> struct cxl_decoder *cxld;
>
> if (is_cxl_endpoint(port))
> - cxld = cxl_endpoint_decoder_alloc(port);
> + cxld = &cxl_endpoint_decoder_alloc(port)->base;
Please split to:
cxled = cxl_endpoint_decoder_alloc(port);
cxld = &cxled->base;
> else
> cxld = cxl_switch_decoder_alloc(port, target_count);
> if (IS_ERR(cxld)) {
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 86f451ecb7ed..8dd29c97e318 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -121,18 +121,19 @@ static DEVICE_ATTR_RO(target_type);
>
> static ssize_t emit_target_list(struct cxl_decoder *cxld, char *buf)
> {
> + struct cxl_decoder_targets *t = cxl_get_decoder_targets(cxld);
> ssize_t offset = 0;
> int i, rc = 0;
>
> for (i = 0; i < cxld->interleave_ways; i++) {
> - struct cxl_dport *dport = cxld->target[i];
> + struct cxl_dport *dport = t->target[i];
> struct cxl_dport *next = NULL;
>
> if (!dport)
> break;
>
> if (i + 1 < cxld->interleave_ways)
> - next = cxld->target[i + 1];
> + next = t->target[i + 1];
> rc = sysfs_emit_at(buf, offset, "%d%s", dport->port_id,
> next ? "," : "");
> if (rc < 0)
> @@ -147,14 +148,15 @@ static ssize_t target_list_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct cxl_decoder *cxld = to_cxl_decoder(dev);
> + struct cxl_decoder_targets *t = cxl_get_decoder_targets(cxld);
> ssize_t offset;
> unsigned int seq;
> int rc;
>
> do {
> - seq = read_seqbegin(&cxld->target_lock);
> + seq = read_seqbegin(&t->target_lock);
> rc = emit_target_list(cxld, buf);
> - } while (read_seqretry(&cxld->target_lock, seq));
> + } while (read_seqretry(&t->target_lock, seq));
>
> if (rc < 0)
> return rc;
> @@ -199,23 +201,6 @@ static const struct attribute_group
> *cxl_decoder_root_attribute_groups[] = {
> NULL,
> };
>
> -static struct attribute *cxl_decoder_switch_attrs[] = {
> - &dev_attr_target_type.attr,
> - &dev_attr_target_list.attr,
> - NULL,
> -};
> -
> -static struct attribute_group cxl_decoder_switch_attribute_group = {
> - .attrs = cxl_decoder_switch_attrs,
> -};
> -
> -static const struct attribute_group *cxl_decoder_switch_attribute_groups[] =
> {
> - &cxl_decoder_switch_attribute_group,
> - &cxl_decoder_base_attribute_group,
> - &cxl_base_attribute_group,
> - NULL,
> -};
> -
> static struct attribute *cxl_decoder_endpoint_attrs[] = {
> &dev_attr_target_type.attr,
> NULL,
> @@ -232,6 +217,12 @@ static const struct attribute_group
> *cxl_decoder_endpoint_attribute_groups[] = {
> NULL,
> };
>
> +static const struct attribute_group *cxl_decoder_switch_attribute_groups[] =
> {
> + &cxl_decoder_base_attribute_group,
> + &cxl_base_attribute_group,
> + NULL,
> +};
> +
> static void cxl_decoder_release(struct device *dev)
> {
> struct cxl_decoder *cxld = to_cxl_decoder(dev);
> @@ -264,6 +255,7 @@ bool is_endpoint_decoder(struct device *dev)
> {
> return dev->type == &cxl_decoder_endpoint_type;
> }
> +EXPORT_SYMBOL_NS_GPL(is_endpoint_decoder, CXL);
>
> bool is_root_decoder(struct device *dev)
> {
> @@ -1136,6 +1128,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_find_dport_by_dev, CXL);
> static int decoder_populate_targets(struct cxl_decoder *cxld,
> struct cxl_port *port, int *target_map)
> {
> + struct cxl_decoder_targets *t = cxl_get_decoder_targets(cxld);
> int i, rc = 0;
>
> if (!target_map)
> @@ -1146,21 +1139,72 @@ static int decoder_populate_targets(struct
> cxl_decoder *cxld,
> if (list_empty(&port->dports))
> return -EINVAL;
>
> - write_seqlock(&cxld->target_lock);
> - for (i = 0; i < cxld->nr_targets; i++) {
> + write_seqlock(&t->target_lock);
> + for (i = 0; i < t->nr_targets; i++) {
> struct cxl_dport *dport = find_dport(port, target_map[i]);
>
> if (!dport) {
> rc = -ENXIO;
> break;
> }
> - cxld->target[i] = dport;
> + t->target[i] = dport;
> }
> - write_sequnlock(&cxld->target_lock);
> + write_sequnlock(&t->target_lock);
>
> return rc;
> }
>
> +static struct cxl_decoder *__cxl_decoder_alloc(struct cxl_port *port,
> + unsigned int nr_targets)
> +{
> + struct cxl_decoder *cxld;
> +
> + if (is_cxl_endpoint(port)) {
> + struct cxl_endpoint_decoder *cxled;
> +
> + cxled = kzalloc(sizeof(*cxled), GFP_KERNEL);
> + if (!cxled)
> + return NULL;
> + cxld = &cxled->base;
> + } else if (is_cxl_root(port)) {
> + struct cxl_root_decoder *cxlrd;
> +
> + cxlrd = kzalloc(sizeof(*cxlrd), GFP_KERNEL);
> + if (!cxlrd)
> + return NULL;
> +
> + cxlrd->targets =
> + kzalloc(struct_size(cxlrd->targets, target,
> nr_targets),
> + GFP_KERNEL);
> + if (!cxlrd->targets) {
> + kfree(cxlrd);
> + return NULL;
> + }
> + cxlrd->targets->nr_targets = nr_targets;
> + seqlock_init(&cxlrd->targets->target_lock);
> + cxld = &cxlrd->base;
> + } else {
> + struct cxl_switch_decoder *cxlsd;
> +
> + cxlsd = kzalloc(sizeof(*cxlsd), GFP_KERNEL);
> + if (!cxlsd)
> + return NULL;
> +
> + cxlsd->targets =
> + kzalloc(struct_size(cxlsd->targets, target,
> nr_targets),
> + GFP_KERNEL);
> + if (!cxlsd->targets) {
> + kfree(cxlsd);
> + return NULL;
> + }
> + cxlsd->targets->nr_targets = nr_targets;
> + seqlock_init(&cxlsd->targets->target_lock);
> + cxld = &cxlsd->base;
> + }
> +
> + return cxld;
> +}
> +
> /**
> * cxl_decoder_alloc - Allocate a new CXL decoder
> * @port: owning port of this decoder
> @@ -1186,7 +1230,7 @@ static struct cxl_decoder *cxl_decoder_alloc(struct
> cxl_port *port,
> if (nr_targets > CXL_DECODER_MAX_INTERLEAVE)
> return ERR_PTR(-EINVAL);
>
> - cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL);
> + cxld = __cxl_decoder_alloc(port, nr_targets);
> if (!cxld)
> return ERR_PTR(-ENOMEM);
>
> @@ -1198,8 +1242,6 @@ static struct cxl_decoder *cxl_decoder_alloc(struct
> cxl_port *port,
> get_device(&port->dev);
> cxld->id = rc;
>
> - cxld->nr_targets = nr_targets;
> - seqlock_init(&cxld->target_lock);
> dev = &cxld->dev;
> device_initialize(dev);
> device_set_pm_not_required(dev);
> @@ -1274,12 +1316,12 @@ EXPORT_SYMBOL_NS_GPL(cxl_switch_decoder_alloc, CXL);
> *
> * Return: A new cxl decoder to be registered by cxl_decoder_add()
> */
> -struct cxl_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port)
> +struct cxl_endpoint_decoder *cxl_endpoint_decoder_alloc(struct cxl_port
> *port)
> {
> if (!is_cxl_endpoint(port))
> return ERR_PTR(-EINVAL);
>
> - return cxl_decoder_alloc(port, 0);
> + return to_cxl_endpoint_decoder(cxl_decoder_alloc(port, 0));
> }
> EXPORT_SYMBOL_NS_GPL(cxl_endpoint_decoder_alloc, CXL);
>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 6517d5cdf5ee..85fd5e84f978 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -193,6 +193,18 @@ enum cxl_decoder_type {
> */
> #define CXL_DECODER_MAX_INTERLEAVE 16
>
> +/**
> + * struct cxl_decoder_targets - Target information for root and switch
> decoders.
> + * @target_lock: coordinate coherent reads of the target list
> + * @nr_targets: number of elements in @target
> + * @target: active ordered target list in current decoder configuration
> + */
> +struct cxl_decoder_targets {
> + seqlock_t target_lock;
> + int nr_targets;
> + struct cxl_dport *target[];
> +};
> +
> /**
> * struct cxl_decoder - CXL address range decode configuration
> * @dev: this decoder's device
> @@ -202,9 +214,6 @@ enum cxl_decoder_type {
> * @interleave_granularity: data stride per dport
> * @target_type: accelerator vs expander (type2 vs type3) selector
> * @flags: memory type capabilities and locking
> - * @target_lock: coordinate coherent reads of the target list
> - * @nr_targets: number of elements in @target
> - * @target: active ordered target list in current decoder configuration
> */
> struct cxl_decoder {
> struct device dev;
> @@ -214,11 +223,46 @@ struct cxl_decoder {
> int interleave_granularity;
> enum cxl_decoder_type target_type;
> unsigned long flags;
> - seqlock_t target_lock;
> - int nr_targets;
> - struct cxl_dport *target[];
> };
>
> +/**
> + * struct cxl_endpoint_decoder - An decoder residing in a CXL endpoint.
> + * @base: Base class decoder
> + */
> +struct cxl_endpoint_decoder {
> + struct cxl_decoder base;
> +};
> +
> +/**
> + * struct cxl_switch_decoder - A decoder in a switch or hostbridge.
> + * @base: Base class decoder
> + * @targets: Downstream targets for this switch.
> + */
> +struct cxl_switch_decoder {
> + struct cxl_decoder base;
> + struct cxl_decoder_targets *targets;
Please no double-allocation when not necessary. This can be
struct cxl_switch_decoder {
struct cxl_decoder base;
struct cxl_decoder_targets targets;
};
...and then allocated with a single:
cxlsd = kzalloc(struct_size(cxlsd, targets.target, nr_targets), GFP_KERNEL);
...or something like that (not compile tested).
> +};
> +};
> +
> +/**
> + * struct cxl_root_decoder - A toplevel/platform decoder
> + * @base: Base class decoder
> + * @targets: Downstream targets (ie. hostbridges).
> + */
> +struct cxl_root_decoder {
> + struct cxl_decoder base;
> + struct cxl_decoder_targets *targets;
> +};
Ditto single allocation feedback...
...although now that struct cxl_root_decoder is identical to
cxl_switch_decoder is there any benefit to making them distinct types
beyond being pedantic? Making them the same type means
cxl_get_decoder_targets() can go because all callers of that are
already in known !cxl_endpoint_decoder paths. But of course ignore
this if cxl_root_decoder is going to game some differentiating
attributes down the road, I have not looked ahead in this series.
> +
> +#define _to_cxl_decoder(x)
> \
> + static inline struct cxl_##x##_decoder *to_cxl_##x##_decoder(
> \
> + struct cxl_decoder *cxld)
> \
> + {
> \
> + return container_of(cxld, struct cxl_##x##_decoder, base);
> \
> + }
> +
> +_to_cxl_decoder(root)
> +_to_cxl_decoder(switch)
> +_to_cxl_decoder(endpoint)
>
> /**
> * enum cxl_nvdimm_brige_state - state machine for managing bus rescans
> @@ -343,11 +387,22 @@ struct cxl_decoder *cxl_root_decoder_alloc(struct
> cxl_port *port,
> struct cxl_decoder *cxl_switch_decoder_alloc(struct cxl_port *port,
> unsigned int nr_targets);
> int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map);
> -struct cxl_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port);
> +struct cxl_endpoint_decoder *cxl_endpoint_decoder_alloc(struct cxl_port
> *port);
> int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map);
> int cxl_decoder_autoremove(struct device *host, struct cxl_decoder *cxld);
> int cxl_endpoint_autoremove(struct cxl_memdev *cxlmd, struct cxl_port
> *endpoint);
>
> +static inline struct cxl_decoder_targets *
> +cxl_get_decoder_targets(struct cxl_decoder *cxld)
> +{
> + if (is_root_decoder(&cxld->dev))
> + return to_cxl_root_decoder(cxld)->targets;
> + else if (is_endpoint_decoder(&cxld->dev))
> + return NULL;
> + else
> + return to_cxl_switch_decoder(cxld)->targets;
> +}
> +
> struct cxl_hdm;
> struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port);
> int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm);
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index 431f2bddf6c8..0534d96486eb 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -454,7 +454,7 @@ static int mock_cxl_enumerate_decoders(struct cxl_hdm
> *cxlhdm)
> if (target_count)
> cxld = cxl_switch_decoder_alloc(port, target_count);
> else
> - cxld = cxl_endpoint_decoder_alloc(port);
> + cxld = &cxl_endpoint_decoder_alloc(port)->base;
Same preference to split this into two steps.
> if (IS_ERR(cxld)) {
> dev_warn(&port->dev,
> "Failed to allocate the decoder\n");
> --
> 2.35.1
>
