| Message ID | 20250310210340.3234884-12-alejandro.lucero-palau@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | add type2 device basic support | expand |
On 3/10/25 4:03 PM, alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.com> > > CXL region creation involves allocating capacity from device DPA > (device-physical-address space) and assigning it to decode a given HPA > (host-physical-address space). Before determining how much DPA to > allocate the amount of available HPA must be determined. Also, not all > HPA is created equal, some specifically targets RAM, some target PMEM, > some is prepared for device-memory flows like HDM-D and HDM-DB, and some > is host-only (HDM-H). > > Wrap all of those concerns into an API that retrieves a root decoder > (platform CXL window) that fits the specified constraints and the > capacity available for a new region. > > Add a complementary function for releasing the reference to such root > decoder. > > Based on https://lore.kernel.org/linux-cxl/168592159290.1948938.13522227102445462976.stgit@dwillia2-xfh.jf.intel.com/ > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/region.c | 160 ++++++++++++++++++++++++++++++++++++++ > drivers/cxl/cxl.h | 3 + > drivers/cxl/mem.c | 26 +++++-- > include/cxl/cxl.h | 11 +++ > 4 files changed, 194 insertions(+), 6 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 8537b6a9ca18..ad809721a3e4 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -695,6 +695,166 @@ static int free_hpa(struct cxl_region *cxlr) > return 0; > } > > +struct cxlrd_max_context { > + struct device * const *host_bridges; > + int interleave_ways; > + unsigned long flags; > + resource_size_t max_hpa; > + struct cxl_root_decoder *cxlrd; > +}; > + > +static int find_max_hpa(struct device *dev, void *data) > +{ > + struct cxlrd_max_context *ctx = data; > + struct cxl_switch_decoder *cxlsd; > + struct cxl_root_decoder *cxlrd; > + struct resource *res, *prev; > + struct cxl_decoder *cxld; > + resource_size_t max; > + int found = 0; > + > + if (!is_root_decoder(dev)) > + return 0; > + > + cxlrd = to_cxl_root_decoder(dev); > + cxlsd = &cxlrd->cxlsd; > + cxld = &cxlsd->cxld; > + if ((cxld->flags & ctx->flags) != ctx->flags) { > + dev_dbg(dev, "flags not matching: %08lx vs %08lx\n", > + cxld->flags, ctx->flags); > + return 0; > + } > + > + for (int i = 0; i < ctx->interleave_ways; i++) > + for (int j = 0; j < ctx->interleave_ways; j++) > + if (ctx->host_bridges[i] == cxlsd->target[j]->dport_dev) { > + found++; > + break; > + } I think kernel coding style requires braces on the above for statements, but I may be wrong here. > + > + if (found != ctx->interleave_ways) { > + dev_dbg(dev, "Not enough host bridges found(%d) for interleave ways requested (%d)\n", > + found, ctx->interleave_ways); > + return 0; > + } > + > + /* > + * Walk the root decoder resource range relying on cxl_region_rwsem to > + * preclude sibling arrival/departure and find the largest free space > + * gap. > + */ > + lockdep_assert_held_read(&cxl_region_rwsem); > + max = 0; > + res = cxlrd->res->child; > + > + /* With no resource child the whole parent resource is available */ > + if (!res) > + max = resource_size(cxlrd->res); > + else > + max = 0; > + > + for (prev = NULL; res; prev = res, res = res->sibling) { > + struct resource *next = res->sibling; > + resource_size_t free = 0; > + > + /* > + * Sanity check for preventing arithmetic problems below as a > + * resource with size 0 could imply using the end field below > + * when set to unsigned zero - 1 or all f in hex. > + */ > + if (prev && !resource_size(prev)) > + continue; > + > + if (!prev && res->start > cxlrd->res->start) { > + free = res->start - cxlrd->res->start; > + max = max(free, max); > + } > + if (prev && res->start > prev->end + 1) { > + free = res->start - prev->end + 1; > + max = max(free, max); > + } > + if (next && res->end + 1 < next->start) { > + free = next->start - res->end + 1; > + max = max(free, max); > + } > + if (!next && res->end + 1 < cxlrd->res->end + 1) { > + free = cxlrd->res->end + 1 - res->end + 1; > + max = max(free, max); > + } > + } > + > + dev_dbg(CXLRD_DEV(cxlrd), "found %pa bytes of free space\n", &max); > + if (max > ctx->max_hpa) { > + if (ctx->cxlrd) > + put_device(CXLRD_DEV(ctx->cxlrd)); > + get_device(CXLRD_DEV(cxlrd)); > + ctx->cxlrd = cxlrd; > + ctx->max_hpa = max; > + dev_dbg(CXLRD_DEV(cxlrd), "found %pa bytes of free space\n", > + &max); Duplicate debug prints here > + } > + return 0; > +} > + > +/** > + * cxl_get_hpa_freespace - find a root decoder with free capacity per constraints > + * @cxlmd: the CXL memory device with an endpoint that is mapped by the returned > + * decoder > + * @interleave_ways: number of entries in @host_bridges > + * @flags: CXL_DECODER_F flags for selecting RAM vs PMEM, and HDM-H vs HDM-D[B] Looking below, the HDM-H vs HDM-D[B] flag is called CXL_DECODER_F_TYPE2, so I think it would be good to reference that either here or in include/cxl/cxl.h. > + * @max_avail_contig: output parameter of max contiguous bytes available in the > + * returned decoder > + * > + * The return tuple of a 'struct cxl_root_decoder' and 'bytes available given > + * in (@max_avail_contig))' is a point in time snapshot. If by the time the > + * caller goes to use this root decoder's capacity the capacity is reduced then > + * caller needs to loop and retry. > + * > + * The returned root decoder has an elevated reference count that needs to be > + * put with put_device(CXLRD_DEV(cxlrd)). s/put_device(CXLRD_DEV(cxlrd))/cxl_put_root_decoder(cxlrd)/ Using put_device() isn't possible in accelerator drivers due to not struct cxl_root_decoder not being exported. > + */ > +struct cxl_root_decoder *cxl_get_hpa_freespace(struct cxl_memdev *cxlmd, > + int interleave_ways, > + unsigned long flags, > + resource_size_t *max_avail_contig) > +{ > + struct cxl_port *endpoint = cxlmd->endpoint; > + struct cxlrd_max_context ctx = { > + .host_bridges = &endpoint->host_bridge, > + .flags = flags, > + }; > + struct cxl_port *root_port; > + struct cxl_root *root __free(put_cxl_root) = find_cxl_root(endpoint); > + > + if (!is_cxl_endpoint(endpoint)) { > + dev_dbg(&endpoint->dev, "hpa requestor is not an endpoint\n"); > + return ERR_PTR(-EINVAL); > + } > + > + if (!root) { > + dev_dbg(&endpoint->dev, "endpoint can not be related to a root port\n"); > + return ERR_PTR(-ENXIO); > + } > + > + root_port = &root->port; > + down_read(&cxl_region_rwsem); > + device_for_each_child(&root_port->dev, &ctx, find_max_hpa); > + up_read(&cxl_region_rwsem); > + > + if (!ctx.cxlrd) > + return ERR_PTR(-ENOMEM); > + > + *max_avail_contig = ctx.max_hpa; > + return ctx.cxlrd; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_get_hpa_freespace, "CXL"); > + > +void cxl_put_root_decoder(struct cxl_root_decoder *cxlrd) > +{ > + put_device(CXLRD_DEV(cxlrd)); > +} > +EXPORT_SYMBOL_NS_GPL(cxl_put_root_decoder, "CXL"); > + > static ssize_t size_store(struct device *dev, struct device_attribute *attr, > const char *buf, size_t len) > { > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 4523864eebd2..c35620c24c8f 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -672,6 +672,9 @@ struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev); > struct cxl_switch_decoder *to_cxl_switch_decoder(struct device *dev); > struct cxl_endpoint_decoder *to_cxl_endpoint_decoder(struct device *dev); > bool is_root_decoder(struct device *dev); > + > +#define CXLRD_DEV(cxlrd) (&(cxlrd)->cxlsd.cxld.dev) > + > bool is_switch_decoder(struct device *dev); > bool is_endpoint_decoder(struct device *dev); > struct cxl_root_decoder *cxl_root_decoder_alloc(struct cxl_port *port, > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index 9675243bd05b..ac152f58df98 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -130,12 +130,19 @@ static int cxl_mem_probe(struct device *dev) > dentry = cxl_debugfs_create_dir(dev_name(dev)); > debugfs_create_devm_seqfile(dev, "dpamem", dentry, cxl_mem_dpa_show); > > - if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds)) > - debugfs_create_file("inject_poison", 0200, dentry, cxlmd, > - &cxl_poison_inject_fops); > - if (test_bit(CXL_POISON_ENABLED_CLEAR, mds->poison.enabled_cmds)) > - debugfs_create_file("clear_poison", 0200, dentry, cxlmd, > - &cxl_poison_clear_fops); > + /* > + * Avoid poison debugfs files for Type2 devices as they rely on > + * cxl_memdev_state. > + */ > + if (mds) { > + if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds)) > + debugfs_create_file("inject_poison", 0200, dentry, cxlmd, > + &cxl_poison_inject_fops); > + > + if (test_bit(CXL_POISON_ENABLED_CLEAR, mds->poison.enabled_cmds)) > + debugfs_create_file("clear_poison", 0200, dentry, cxlmd, > + &cxl_poison_clear_fops); > + } > > rc = devm_add_action_or_reset(dev, remove_debugfs, dentry); > if (rc) > @@ -219,6 +226,13 @@ static umode_t cxl_mem_visible(struct kobject *kobj, struct attribute *a, int n) > struct cxl_memdev *cxlmd = to_cxl_memdev(dev); > struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); > > + /* > + * Avoid poison sysfs files for Type2 devices as they rely on > + * cxl_memdev_state. > + */ > + if (!mds) > + return 0; > + Are these changes to cxl/mem.c supposed to be in patch 09/23? They aren't related to this one as far as I can tell... > if (a == &dev_attr_trigger_poison_list.attr) > if (!test_bit(CXL_POISON_ENABLED_LIST, > mds->poison.enabled_cmds)) > diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h > index 340503d7c33c..6ca6230d1fe5 100644 > --- a/include/cxl/cxl.h > +++ b/include/cxl/cxl.h > @@ -25,6 +25,9 @@ enum cxl_devtype { > > struct device; > > +#define CXL_DECODER_F_RAM BIT(0) > +#define CXL_DECODER_F_PMEM BIT(1) > +#define CXL_DECODER_F_TYPE2 BIT(2) > > /* Capabilities as defined for: > * > @@ -244,4 +247,12 @@ void cxl_mem_dpa_init(struct cxl_dpa_info *info, u64 volatile_bytes, > int cxl_dpa_setup(struct cxl_dev_state *cxlds, const struct cxl_dpa_info *info); > struct cxl_memdev *devm_cxl_add_memdev(struct device *host, > struct cxl_dev_state *cxlmds); > +struct cxl_memdev *devm_cxl_add_memdev(struct device *host, > + struct cxl_dev_state *cxlds); This declaration is duplicated > +struct cxl_port; > +struct cxl_root_decoder *cxl_get_hpa_freespace(struct cxl_memdev *cxlmd, > + int interleave_ways, > + unsigned long flags, > + resource_size_t *max); > +void cxl_put_root_decoder(struct cxl_root_decoder *cxlrd); > #endif
On 3/11/25 20:06, Ben Cheatham wrote: > On 3/10/25 4:03 PM, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alucerop@amd.com> >> >> CXL region creation involves allocating capacity from device DPA >> (device-physical-address space) and assigning it to decode a given HPA >> (host-physical-address space). Before determining how much DPA to >> allocate the amount of available HPA must be determined. Also, not all >> HPA is created equal, some specifically targets RAM, some target PMEM, >> some is prepared for device-memory flows like HDM-D and HDM-DB, and some >> is host-only (HDM-H). >> >> Wrap all of those concerns into an API that retrieves a root decoder >> (platform CXL window) that fits the specified constraints and the >> capacity available for a new region. >> >> Add a complementary function for releasing the reference to such root >> decoder. >> >> Based on https://lore.kernel.org/linux-cxl/168592159290.1948938.13522227102445462976.stgit@dwillia2-xfh.jf.intel.com/ >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> --- >> drivers/cxl/core/region.c | 160 ++++++++++++++++++++++++++++++++++++++ >> drivers/cxl/cxl.h | 3 + >> drivers/cxl/mem.c | 26 +++++-- >> include/cxl/cxl.h | 11 +++ >> 4 files changed, 194 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c >> index 8537b6a9ca18..ad809721a3e4 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -695,6 +695,166 @@ static int free_hpa(struct cxl_region *cxlr) >> return 0; >> } >> >> +struct cxlrd_max_context { >> + struct device * const *host_bridges; >> + int interleave_ways; >> + unsigned long flags; >> + resource_size_t max_hpa; >> + struct cxl_root_decoder *cxlrd; >> +}; >> + >> +static int find_max_hpa(struct device *dev, void *data) >> +{ >> + struct cxlrd_max_context *ctx = data; >> + struct cxl_switch_decoder *cxlsd; >> + struct cxl_root_decoder *cxlrd; >> + struct resource *res, *prev; >> + struct cxl_decoder *cxld; >> + resource_size_t max; >> + int found = 0; >> + >> + if (!is_root_decoder(dev)) >> + return 0; >> + >> + cxlrd = to_cxl_root_decoder(dev); >> + cxlsd = &cxlrd->cxlsd; >> + cxld = &cxlsd->cxld; >> + if ((cxld->flags & ctx->flags) != ctx->flags) { >> + dev_dbg(dev, "flags not matching: %08lx vs %08lx\n", >> + cxld->flags, ctx->flags); >> + return 0; >> + } >> + >> + for (int i = 0; i < ctx->interleave_ways; i++) >> + for (int j = 0; j < ctx->interleave_ways; j++) >> + if (ctx->host_bridges[i] == cxlsd->target[j]->dport_dev) { >> + found++; >> + break; >> + } > I think kernel coding style requires braces on the above for statements, but I may be wrong here. I can not see a specific reference to this case. But, do you mean both for clauses requiring it or just the second one? >> + >> + if (found != ctx->interleave_ways) { >> + dev_dbg(dev, "Not enough host bridges found(%d) for interleave ways requested (%d)\n", >> + found, ctx->interleave_ways); >> + return 0; >> + } >> + >> + /* >> + * Walk the root decoder resource range relying on cxl_region_rwsem to >> + * preclude sibling arrival/departure and find the largest free space >> + * gap. >> + */ >> + lockdep_assert_held_read(&cxl_region_rwsem); >> + max = 0; >> + res = cxlrd->res->child; >> + >> + /* With no resource child the whole parent resource is available */ >> + if (!res) >> + max = resource_size(cxlrd->res); >> + else >> + max = 0; >> + >> + for (prev = NULL; res; prev = res, res = res->sibling) { >> + struct resource *next = res->sibling; >> + resource_size_t free = 0; >> + >> + /* >> + * Sanity check for preventing arithmetic problems below as a >> + * resource with size 0 could imply using the end field below >> + * when set to unsigned zero - 1 or all f in hex. >> + */ >> + if (prev && !resource_size(prev)) >> + continue; >> + >> + if (!prev && res->start > cxlrd->res->start) { >> + free = res->start - cxlrd->res->start; >> + max = max(free, max); >> + } >> + if (prev && res->start > prev->end + 1) { >> + free = res->start - prev->end + 1; >> + max = max(free, max); >> + } >> + if (next && res->end + 1 < next->start) { >> + free = next->start - res->end + 1; >> + max = max(free, max); >> + } >> + if (!next && res->end + 1 < cxlrd->res->end + 1) { >> + free = cxlrd->res->end + 1 - res->end + 1; >> + max = max(free, max); >> + } >> + } >> + >> + dev_dbg(CXLRD_DEV(cxlrd), "found %pa bytes of free space\n", &max); >> + if (max > ctx->max_hpa) { >> + if (ctx->cxlrd) >> + put_device(CXLRD_DEV(ctx->cxlrd)); >> + get_device(CXLRD_DEV(cxlrd)); >> + ctx->cxlrd = cxlrd; >> + ctx->max_hpa = max; >> + dev_dbg(CXLRD_DEV(cxlrd), "found %pa bytes of free space\n", >> + &max); > Duplicate debug prints here Yes. I'll remove the second one. >> + } >> + return 0; >> +} >> + >> +/** >> + * cxl_get_hpa_freespace - find a root decoder with free capacity per constraints >> + * @cxlmd: the CXL memory device with an endpoint that is mapped by the returned >> + * decoder >> + * @interleave_ways: number of entries in @host_bridges >> + * @flags: CXL_DECODER_F flags for selecting RAM vs PMEM, and HDM-H vs HDM-D[B] > Looking below, the HDM-H vs HDM-D[B] flag is called CXL_DECODER_F_TYPE2, so I think > it would be good to reference that either here or in include/cxl/cxl.h. Well, it is not that but I agree the description requires an update like; "flags for selecting RAM vs PMEM, and Type2 device." I think because the patch does not define the HDM-* as it is not needed yet, it should not be there. >> + * @max_avail_contig: output parameter of max contiguous bytes available in the >> + * returned decoder >> + * >> + * The return tuple of a 'struct cxl_root_decoder' and 'bytes available given >> + * in (@max_avail_contig))' is a point in time snapshot. If by the time the >> + * caller goes to use this root decoder's capacity the capacity is reduced then >> + * caller needs to loop and retry. >> + * >> + * The returned root decoder has an elevated reference count that needs to be >> + * put with put_device(CXLRD_DEV(cxlrd)). > s/put_device(CXLRD_DEV(cxlrd))/cxl_put_root_decoder(cxlrd)/ > > Using put_device() isn't possible in accelerator drivers due to not struct cxl_root_decoder > not being exported. The sfc driver is using cxl_put_root_decoder, so yes, the description needs an update. >> + */ >> +struct cxl_root_decoder *cxl_get_hpa_freespace(struct cxl_memdev *cxlmd, >> + int interleave_ways, >> + unsigned long flags, >> + resource_size_t *max_avail_contig) >> +{ >> + struct cxl_port *endpoint = cxlmd->endpoint; >> + struct cxlrd_max_context ctx = { >> + .host_bridges = &endpoint->host_bridge, >> + .flags = flags, >> + }; >> + struct cxl_port *root_port; >> + struct cxl_root *root __free(put_cxl_root) = find_cxl_root(endpoint); >> + >> + if (!is_cxl_endpoint(endpoint)) { >> + dev_dbg(&endpoint->dev, "hpa requestor is not an endpoint\n"); >> + return ERR_PTR(-EINVAL); >> + } >> + >> + if (!root) { >> + dev_dbg(&endpoint->dev, "endpoint can not be related to a root port\n"); >> + return ERR_PTR(-ENXIO); >> + } >> + >> + root_port = &root->port; >> + down_read(&cxl_region_rwsem); >> + device_for_each_child(&root_port->dev, &ctx, find_max_hpa); >> + up_read(&cxl_region_rwsem); >> + >> + if (!ctx.cxlrd) >> + return ERR_PTR(-ENOMEM); >> + >> + *max_avail_contig = ctx.max_hpa; >> + return ctx.cxlrd; >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_get_hpa_freespace, "CXL"); >> + >> +void cxl_put_root_decoder(struct cxl_root_decoder *cxlrd) >> +{ >> + put_device(CXLRD_DEV(cxlrd)); >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_put_root_decoder, "CXL"); >> + >> static ssize_t size_store(struct device *dev, struct device_attribute *attr, >> const char *buf, size_t len) >> { >> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h >> index 4523864eebd2..c35620c24c8f 100644 >> --- a/drivers/cxl/cxl.h >> +++ b/drivers/cxl/cxl.h >> @@ -672,6 +672,9 @@ struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev); >> struct cxl_switch_decoder *to_cxl_switch_decoder(struct device *dev); >> struct cxl_endpoint_decoder *to_cxl_endpoint_decoder(struct device *dev); >> bool is_root_decoder(struct device *dev); >> + >> +#define CXLRD_DEV(cxlrd) (&(cxlrd)->cxlsd.cxld.dev) >> + >> bool is_switch_decoder(struct device *dev); >> bool is_endpoint_decoder(struct device *dev); >> struct cxl_root_decoder *cxl_root_decoder_alloc(struct cxl_port *port, >> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c >> index 9675243bd05b..ac152f58df98 100644 >> --- a/drivers/cxl/mem.c >> +++ b/drivers/cxl/mem.c >> @@ -130,12 +130,19 @@ static int cxl_mem_probe(struct device *dev) >> dentry = cxl_debugfs_create_dir(dev_name(dev)); >> debugfs_create_devm_seqfile(dev, "dpamem", dentry, cxl_mem_dpa_show); >> >> - if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds)) >> - debugfs_create_file("inject_poison", 0200, dentry, cxlmd, >> - &cxl_poison_inject_fops); >> - if (test_bit(CXL_POISON_ENABLED_CLEAR, mds->poison.enabled_cmds)) >> - debugfs_create_file("clear_poison", 0200, dentry, cxlmd, >> - &cxl_poison_clear_fops); >> + /* >> + * Avoid poison debugfs files for Type2 devices as they rely on >> + * cxl_memdev_state. >> + */ >> + if (mds) { >> + if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds)) >> + debugfs_create_file("inject_poison", 0200, dentry, cxlmd, >> + &cxl_poison_inject_fops); >> + >> + if (test_bit(CXL_POISON_ENABLED_CLEAR, mds->poison.enabled_cmds)) >> + debugfs_create_file("clear_poison", 0200, dentry, cxlmd, >> + &cxl_poison_clear_fops); >> + } >> >> rc = devm_add_action_or_reset(dev, remove_debugfs, dentry); >> if (rc) >> @@ -219,6 +226,13 @@ static umode_t cxl_mem_visible(struct kobject *kobj, struct attribute *a, int n) >> struct cxl_memdev *cxlmd = to_cxl_memdev(dev); >> struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); >> >> + /* >> + * Avoid poison sysfs files for Type2 devices as they rely on >> + * cxl_memdev_state. >> + */ >> + if (!mds) >> + return 0; >> + > Are these changes to cxl/mem.c supposed to be in patch 09/23? They aren't related to this one > as far as I can tell... Oh, yes. Not sure how this ended up here. probably due to the changes introduced in v10 ... and not valid anymore. I'll put the code back to the original patch containing it. >> if (a == &dev_attr_trigger_poison_list.attr) >> if (!test_bit(CXL_POISON_ENABLED_LIST, >> mds->poison.enabled_cmds)) >> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h >> index 340503d7c33c..6ca6230d1fe5 100644 >> --- a/include/cxl/cxl.h >> +++ b/include/cxl/cxl.h >> @@ -25,6 +25,9 @@ enum cxl_devtype { >> >> struct device; >> >> +#define CXL_DECODER_F_RAM BIT(0) >> +#define CXL_DECODER_F_PMEM BIT(1) >> +#define CXL_DECODER_F_TYPE2 BIT(2) >> >> /* Capabilities as defined for: >> * >> @@ -244,4 +247,12 @@ void cxl_mem_dpa_init(struct cxl_dpa_info *info, u64 volatile_bytes, >> int cxl_dpa_setup(struct cxl_dev_state *cxlds, const struct cxl_dpa_info *info); >> struct cxl_memdev *devm_cxl_add_memdev(struct device *host, >> struct cxl_dev_state *cxlmds); >> +struct cxl_memdev *devm_cxl_add_memdev(struct device *host, >> + struct cxl_dev_state *cxlds); > This declaration is duplicated > Good catch. I'll fix it. Thanks!
>>> + >>> + for (int i = 0; i < ctx->interleave_ways; i++) >>> + for (int j = 0; j < ctx->interleave_ways; j++) >>> + if (ctx->host_bridges[i] == cxlsd->target[j]->dport_dev) { >>> + found++; >>> + break; >>> + } >> I think kernel coding style requires braces on the above for statements, but I may be wrong here. > > > I can not see a specific reference to this case. But, do you mean both for clauses requiring it or just the second one? > I think it should be: for (int i = 0; i < ctx->interleave_ways; i++) { for (int j = 0 ; j < ctx->interleave_ways; j++) { ... } } But it's a style nit, if whoever is taking the patch doesn't care then I don't really either. > [snip] >>> + >>> +/** >>> + * cxl_get_hpa_freespace - find a root decoder with free capacity per constraints >>> + * @cxlmd: the CXL memory device with an endpoint that is mapped by the returned >>> + * decoder >>> + * @interleave_ways: number of entries in @host_bridges >>> + * @flags: CXL_DECODER_F flags for selecting RAM vs PMEM, and HDM-H vs HDM-D[B] >> Looking below, the HDM-H vs HDM-D[B] flag is called CXL_DECODER_F_TYPE2, so I think >> it would be good to reference that either here or in include/cxl/cxl.h. > > > Well, it is not that but I agree the description requires an update like; > > > "flags for selecting RAM vs PMEM, and Type2 device." > > > I think because the patch does not define the HDM-* as it is not needed yet, it should not be there. > Sounds good to me. Thanks, Ben
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 8537b6a9ca18..ad809721a3e4 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -695,6 +695,166 @@ static int free_hpa(struct cxl_region *cxlr) return 0; } +struct cxlrd_max_context { + struct device * const *host_bridges; + int interleave_ways; + unsigned long flags; + resource_size_t max_hpa; + struct cxl_root_decoder *cxlrd; +}; + +static int find_max_hpa(struct device *dev, void *data) +{ + struct cxlrd_max_context *ctx = data; + struct cxl_switch_decoder *cxlsd; + struct cxl_root_decoder *cxlrd; + struct resource *res, *prev; + struct cxl_decoder *cxld; + resource_size_t max; + int found = 0; + + if (!is_root_decoder(dev)) + return 0; + + cxlrd = to_cxl_root_decoder(dev); + cxlsd = &cxlrd->cxlsd; + cxld = &cxlsd->cxld; + if ((cxld->flags & ctx->flags) != ctx->flags) { + dev_dbg(dev, "flags not matching: %08lx vs %08lx\n", + cxld->flags, ctx->flags); + return 0; + } + + for (int i = 0; i < ctx->interleave_ways; i++) + for (int j = 0; j < ctx->interleave_ways; j++) + if (ctx->host_bridges[i] == cxlsd->target[j]->dport_dev) { + found++; + break; + } + + if (found != ctx->interleave_ways) { + dev_dbg(dev, "Not enough host bridges found(%d) for interleave ways requested (%d)\n", + found, ctx->interleave_ways); + return 0; + } + + /* + * Walk the root decoder resource range relying on cxl_region_rwsem to + * preclude sibling arrival/departure and find the largest free space + * gap. + */ + lockdep_assert_held_read(&cxl_region_rwsem); + max = 0; + res = cxlrd->res->child; + + /* With no resource child the whole parent resource is available */ + if (!res) + max = resource_size(cxlrd->res); + else + max = 0; + + for (prev = NULL; res; prev = res, res = res->sibling) { + struct resource *next = res->sibling; + resource_size_t free = 0; + + /* + * Sanity check for preventing arithmetic problems below as a + * resource with size 0 could imply using the end field below + * when set to unsigned zero - 1 or all f in hex. + */ + if (prev && !resource_size(prev)) + continue; + + if (!prev && res->start > cxlrd->res->start) { + free = res->start - cxlrd->res->start; + max = max(free, max); + } + if (prev && res->start > prev->end + 1) { + free = res->start - prev->end + 1; + max = max(free, max); + } + if (next && res->end + 1 < next->start) { + free = next->start - res->end + 1; + max = max(free, max); + } + if (!next && res->end + 1 < cxlrd->res->end + 1) { + free = cxlrd->res->end + 1 - res->end + 1; + max = max(free, max); + } + } + + dev_dbg(CXLRD_DEV(cxlrd), "found %pa bytes of free space\n", &max); + if (max > ctx->max_hpa) { + if (ctx->cxlrd) + put_device(CXLRD_DEV(ctx->cxlrd)); + get_device(CXLRD_DEV(cxlrd)); + ctx->cxlrd = cxlrd; + ctx->max_hpa = max; + dev_dbg(CXLRD_DEV(cxlrd), "found %pa bytes of free space\n", + &max); + } + return 0; +} + +/** + * cxl_get_hpa_freespace - find a root decoder with free capacity per constraints + * @cxlmd: the CXL memory device with an endpoint that is mapped by the returned + * decoder + * @interleave_ways: number of entries in @host_bridges + * @flags: CXL_DECODER_F flags for selecting RAM vs PMEM, and HDM-H vs HDM-D[B] + * @max_avail_contig: output parameter of max contiguous bytes available in the + * returned decoder + * + * The return tuple of a 'struct cxl_root_decoder' and 'bytes available given + * in (@max_avail_contig))' is a point in time snapshot. If by the time the + * caller goes to use this root decoder's capacity the capacity is reduced then + * caller needs to loop and retry. + * + * The returned root decoder has an elevated reference count that needs to be + * put with put_device(CXLRD_DEV(cxlrd)). + */ +struct cxl_root_decoder *cxl_get_hpa_freespace(struct cxl_memdev *cxlmd, + int interleave_ways, + unsigned long flags, + resource_size_t *max_avail_contig) +{ + struct cxl_port *endpoint = cxlmd->endpoint; + struct cxlrd_max_context ctx = { + .host_bridges = &endpoint->host_bridge, + .flags = flags, + }; + struct cxl_port *root_port; + struct cxl_root *root __free(put_cxl_root) = find_cxl_root(endpoint); + + if (!is_cxl_endpoint(endpoint)) { + dev_dbg(&endpoint->dev, "hpa requestor is not an endpoint\n"); + return ERR_PTR(-EINVAL); + } + + if (!root) { + dev_dbg(&endpoint->dev, "endpoint can not be related to a root port\n"); + return ERR_PTR(-ENXIO); + } + + root_port = &root->port; + down_read(&cxl_region_rwsem); + device_for_each_child(&root_port->dev, &ctx, find_max_hpa); + up_read(&cxl_region_rwsem); + + if (!ctx.cxlrd) + return ERR_PTR(-ENOMEM); + + *max_avail_contig = ctx.max_hpa; + return ctx.cxlrd; +} +EXPORT_SYMBOL_NS_GPL(cxl_get_hpa_freespace, "CXL"); + +void cxl_put_root_decoder(struct cxl_root_decoder *cxlrd) +{ + put_device(CXLRD_DEV(cxlrd)); +} +EXPORT_SYMBOL_NS_GPL(cxl_put_root_decoder, "CXL"); + static ssize_t size_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t len) { diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 4523864eebd2..c35620c24c8f 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -672,6 +672,9 @@ struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev); struct cxl_switch_decoder *to_cxl_switch_decoder(struct device *dev); struct cxl_endpoint_decoder *to_cxl_endpoint_decoder(struct device *dev); bool is_root_decoder(struct device *dev); + +#define CXLRD_DEV(cxlrd) (&(cxlrd)->cxlsd.cxld.dev) + bool is_switch_decoder(struct device *dev); bool is_endpoint_decoder(struct device *dev); struct cxl_root_decoder *cxl_root_decoder_alloc(struct cxl_port *port, diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index 9675243bd05b..ac152f58df98 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -130,12 +130,19 @@ static int cxl_mem_probe(struct device *dev) dentry = cxl_debugfs_create_dir(dev_name(dev)); debugfs_create_devm_seqfile(dev, "dpamem", dentry, cxl_mem_dpa_show); - if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds)) - debugfs_create_file("inject_poison", 0200, dentry, cxlmd, - &cxl_poison_inject_fops); - if (test_bit(CXL_POISON_ENABLED_CLEAR, mds->poison.enabled_cmds)) - debugfs_create_file("clear_poison", 0200, dentry, cxlmd, - &cxl_poison_clear_fops); + /* + * Avoid poison debugfs files for Type2 devices as they rely on + * cxl_memdev_state. + */ + if (mds) { + if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds)) + debugfs_create_file("inject_poison", 0200, dentry, cxlmd, + &cxl_poison_inject_fops); + + if (test_bit(CXL_POISON_ENABLED_CLEAR, mds->poison.enabled_cmds)) + debugfs_create_file("clear_poison", 0200, dentry, cxlmd, + &cxl_poison_clear_fops); + } rc = devm_add_action_or_reset(dev, remove_debugfs, dentry); if (rc) @@ -219,6 +226,13 @@ static umode_t cxl_mem_visible(struct kobject *kobj, struct attribute *a, int n) struct cxl_memdev *cxlmd = to_cxl_memdev(dev); struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); + /* + * Avoid poison sysfs files for Type2 devices as they rely on + * cxl_memdev_state. + */ + if (!mds) + return 0; + if (a == &dev_attr_trigger_poison_list.attr) if (!test_bit(CXL_POISON_ENABLED_LIST, mds->poison.enabled_cmds)) diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h index 340503d7c33c..6ca6230d1fe5 100644 --- a/include/cxl/cxl.h +++ b/include/cxl/cxl.h @@ -25,6 +25,9 @@ enum cxl_devtype { struct device; +#define CXL_DECODER_F_RAM BIT(0) +#define CXL_DECODER_F_PMEM BIT(1) +#define CXL_DECODER_F_TYPE2 BIT(2) /* Capabilities as defined for: * @@ -244,4 +247,12 @@ void cxl_mem_dpa_init(struct cxl_dpa_info *info, u64 volatile_bytes, int cxl_dpa_setup(struct cxl_dev_state *cxlds, const struct cxl_dpa_info *info); struct cxl_memdev *devm_cxl_add_memdev(struct device *host, struct cxl_dev_state *cxlmds); +struct cxl_memdev *devm_cxl_add_memdev(struct device *host, + struct cxl_dev_state *cxlds); +struct cxl_port; +struct cxl_root_decoder *cxl_get_hpa_freespace(struct cxl_memdev *cxlmd, + int interleave_ways, + unsigned long flags, + resource_size_t *max); +void cxl_put_root_decoder(struct cxl_root_decoder *cxlrd); #endif