Verma, Vishal L wrote: > On Wed, 2022-08-10 at 20:05 -0700, Dan Williams wrote: > > Vishal Verma wrote: > > > Add libcxl APIs to create a region under a given root decoder, and to > > > set different attributes for the new region. These allow setting the > > > size, interleave_ways, interleave_granularity, uuid, and the target > > > devices for the newly minted cxl_region object. > > > > > > Cc: Dan Williams <[email protected]> > > > Signed-off-by: Vishal Verma <[email protected]> > > > --- > > > Documentation/cxl/lib/libcxl.txt | 69 ++++++ > > > cxl/lib/private.h | 2 + > > > cxl/lib/libcxl.c | 377 ++++++++++++++++++++++++++++++- > > > cxl/libcxl.h | 23 +- > > > cxl/lib/libcxl.sym | 16 ++ > > > 5 files changed, 484 insertions(+), 3 deletions(-) > > > > > > diff --git a/Documentation/cxl/lib/libcxl.txt > > > b/Documentation/cxl/lib/libcxl.txt > > > index 7a38ce4..c3a8f36 100644 > > > --- a/Documentation/cxl/lib/libcxl.txt > > > +++ b/Documentation/cxl/lib/libcxl.txt > > > @@ -508,6 +508,75 @@ device to represent the root of a PCI device > > > hierarchy. The > > > cxl_target_get_physical_node() helper returns the device name of that > > > companion object in the PCI hierarchy. > > > > > > +==== REGIONS > > > +A CXL region is composed of one or more slices of CXL memdevs, with > > > configurable > > > +interleave settings - both the number of interleave ways, and the > > > interleave > > > +granularity. In terms of hierarchy, it is the child of a CXL root > > > decoder. A root > > > +decoder (recall that this corresponds to an ACPI CEDT.CFMWS 'window'), > > > may have > > > +multiple chile regions, but a region is strictly tied to one root > > > decoder. > > > > Mmm, that's a spicy region. > > > > s/chile/child/ > > Hah yep will fix. > > > > > > + > > > +A region also defines a set of mappings which are slices of capacity on > > > a memdev, > > > > Since the above already defined that a region is composed of one or more > > slices of CXL memdevs, how about: > > > > "The slices that compose a region are called mappings. A mapping is a > > tuple of 'memdev', 'endpoint decoder', and the 'position'. > > Yep sounds good. > > [snip] > > > > > > +CXL_EXPORT struct cxl_region * > > > +cxl_decoder_create_pmem_region(struct cxl_decoder *decoder) > > > +{ > > > + struct cxl_ctx *ctx = cxl_decoder_get_ctx(decoder); > > > + char *path = decoder->dev_buf; > > > + char buf[SYSFS_ATTR_SIZE]; > > > + struct cxl_region *region; > > > + int rc; > > > + > > > + sprintf(path, "%s/create_pmem_region", decoder->dev_path); > > > + rc = sysfs_read_attr(ctx, path, buf); > > > + if (rc < 0) { > > > + err(ctx, "failed to read new region name: %s\n", > > > + strerror(-rc)); > > > + return NULL; > > > + } > > > + > > > + rc = sysfs_write_attr(ctx, path, buf); > > > + if (rc < 0) { > > > + err(ctx, "failed to write new region name: %s\n", > > > + strerror(-rc)); > > > + return NULL; > > > + } > > > > I think there either needs to be a "decoder->regions_init = 0" here, or > > a direct call to "add_cxl_region(decoder...)" just in case this context > > had already listed regions before creating a new one. > > > > I like the precision of "add_cxl_region()", but that needs to open code > > some of the internals of sysfs_device_parse(), so maybe > > "decoder->regions_init = 0" is ok for now. > > Yes, I found that out - and added this - in patch 10 (I can instead > move it here - it makes sense). > > Until patch 10, during region creation, nothing had done regions_init > until this point, so this happens to work. Patch 10 where we do walk > the regions before this point to calculate the max available space, > necessitates the reset here. > > That being said, potentially all of patch 10 is squash-able into > different bits of the series - I left it at the end so the > max_available_extent stuff can be reviewed on its own. > > I'm happy to go either way on squashing it or keeping it standalone.
If this was the kernel I would say squash, since bisecting might be impacted, but as long as the fix is there later in the series I think that's ok. Note, I was less worried about the cxl-cli tool tripping over this, and more about 3rd party applications using libcxl, but those are even less likely to use a mid-release commit.
