Re: [PATCH v3 06/25] cxl/mem: Read dynamic capacity configuration from the device
On Fri, 16 Aug 2024 09:44:14 -0500 [email protected] wrote: > From: Navneet Singh > > Devices which optionally support Dynamic Capacity (DC) are configured > via mailbox commands. CXL 3.1 requires the host to issue the Get DC > Configuration command in order to properly configure DCDs. Without the > Get DC Configuration command DCD can't be supported. > > Implement the DC mailbox commands as specified in CXL 3.1 section > 8.2.9.9.9 (opcodes 48XXh) to read and store the DCD configuration > information. Disable DCD if DCD is not supported. Leverage the Get DC > Configuration command supported bit to indicate if DCD support. > > Linux has no use for the trailing fields of the Get Dynamic Capacity > Configuration Output Payload (Total number of supported extents, number > of available extents, total number of supported tags, and number of > available tags). Avoid defining those fields to use the more useful > dynamic C array. > > Cc: "Li, Ming" > Signed-off-by: Navneet Singh > Co-developed-by: Ira Weiny > Signed-off-by: Ira Weiny LGTM Reviewed-by: Jonathan Cameron If you can get rid of the thing even better.
Re: [PATCH v3 06/25] cxl/mem: Read dynamic capacity configuration from the device
Fan Ni wrote:
> On Fri, Aug 16, 2024 at 02:45:47PM -0700, Dave Jiang wrote:
[snip]
> > > +
> > > +/**
> > > + * cxl_dev_dynamic_capacity_identify() - Reads the dynamic capacity
> > > + *information from the device.
> > > + * @mds: The memory device state
> > > + *
> > > + * Read Dynamic Capacity information from the device and populate the
> > > state
> > > + * structures for later use.
> > > + *
> > > + * Return: 0 if identify was executed successfully, -ERRNO on error.
> > > + */
> > > +int cxl_dev_dynamic_capacity_identify(struct cxl_memdev_state *mds)
> > > +{
> > > + size_t dc_resp_size = mds->payload_size;
> > > + struct device *dev = mds->cxlds.dev;
> > > + u8 start_region, i;
> > > +
> > > + for (i = 0; i < CXL_MAX_DC_REGION; i++)
> > > + snprintf(mds->dc_region[i].name, CXL_DC_REGION_STRLEN, "");
> > > +
> > > + if (!cxl_dcd_supported(mds)) {
> > > + dev_dbg(dev, "DCD not supported\n");
> > > + return 0;
> > > + }
> >
> > This should happen before you pre-format the name string? I would assume
> > that if DCD is not supported then the dcd name sysfs attribs would be not
> > be visible?
> >
No this string is not used for sysfs. It is used to label the dpa
resources... That said in review I don't recall why it was necessary to
add the '' to them by default. I'm actually going to remove that and
continue testing and if I recall where this was showing up I might add it
back in.
> > > +
> > > + struct cxl_mbox_get_dc_config_out *dc_resp __free(kfree) =
> > > + kvmalloc(dc_resp_size, GFP_KERNEL);
> > > + if (!dc_resp)
> > > + return -ENOMEM;
> > > +
> > > + start_region = 0;
> > > + do {
> > > + int rc, j;
> > > +
> > > + rc = cxl_get_dc_config(mds, start_region, dc_resp,
> > > dc_resp_size);
> > > + if (rc < 0) {
> > > + dev_dbg(dev, "Failed to get DC config: %d\n", rc);
> > > + return rc;
> > > + }
> > > +
> > > + mds->nr_dc_region += rc;
> > > +
> > > + if (mds->nr_dc_region < 1 || mds->nr_dc_region >
> > > CXL_MAX_DC_REGION) {
> > > + dev_err(dev, "Invalid num of dynamic capacity regions
> > > %d\n",
> > > + mds->nr_dc_region);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + for (i = start_region, j = 0; i < mds->nr_dc_region; i++, j++) {
> >
> > This should be 'j < mds->nr_dc_region'? Otherwise if your start region say
> > is '3' and you have '2' DC regions, you never enter the loop. Or does that
> > not happen? I also wonder if you need to check if 'start_region +
> > mds->nr_dc_region > CXL_MAX_DC_REGION'.
> >
> That can not happen, start_region was updated to the number of regions
> has returned till now (not counting the current call), while
> nr_dc_region is the total number of regions returned till now (including
> the current call) as we update it above, so start_region should never be
> larger
> than nr_dc_region.
Yep.
>
> > > + rc = cxl_dc_save_region_info(mds, i,
> > > &dc_resp->region[j]);
> > > + if (rc) {
> > > + dev_dbg(dev, "Failed to save region info:
> > > %d\n", rc);
>
> I am not sure why we sometimes use dev_err and sometimes we use dev_dbg
> here, if dcd is supported, error from getting dc configuration is an
> error to me.
We are trying to reduce the dev_err() use. cxl_dc_save_region_info() has
dev_err() which is much more specific as to the error. At worse this is
just redundant as a debug.
I'll remove it because the debug output is pretty verbose too.
Ira
>
> Fan
[snip]
Re: [PATCH v3 06/25] cxl/mem: Read dynamic capacity configuration from the device
Fan Ni wrote:
> On Fri, Aug 16, 2024 at 02:45:47PM -0700, Dave Jiang wrote:
> >
> > > +
> > > +/**
> > > + * cxl_dev_dynamic_capacity_identify() - Reads the dynamic capacity
> > > + *information from the device.
> > > + * @mds: The memory device state
> > > + *
> > > + * Read Dynamic Capacity information from the device and populate the
> > > state
> > > + * structures for later use.
> > > + *
> > > + * Return: 0 if identify was executed successfully, -ERRNO on error.
> > > + */
> > > +int cxl_dev_dynamic_capacity_identify(struct cxl_memdev_state *mds)
> > > +{
> > > + size_t dc_resp_size = mds->payload_size;
> > > + struct device *dev = mds->cxlds.dev;
> > > + u8 start_region, i;
> > > +
> > > + for (i = 0; i < CXL_MAX_DC_REGION; i++)
> > > + snprintf(mds->dc_region[i].name, CXL_DC_REGION_STRLEN, "");
> > > +
> > > + if (!cxl_dcd_supported(mds)) {
> > > + dev_dbg(dev, "DCD not supported\n");
> > > + return 0;
> > > + }
> >
> > This should happen before you pre-format the name string? I would assume
> > that if DCD is not supported then the dcd name sysfs attribs would be not
> > be visible?
> >
No this string is not used for sysfs. It is used to label the dpa
resources... That said in review I don't recall why it was necessary to
add the '' to them by default. I'm actually going to remove that and
continue testing and if I recall where this was showing up I might add it
back in.
> > > +
> > > + struct cxl_mbox_get_dc_config_out *dc_resp __free(kfree) =
> > > + kvmalloc(dc_resp_size, GFP_KERNEL);
> > > + if (!dc_resp)
> > > + return -ENOMEM;
> > > +
> > > + start_region = 0;
> > > + do {
> > > + int rc, j;
> > > +
> > > + rc = cxl_get_dc_config(mds, start_region, dc_resp,
> > > dc_resp_size);
> > > + if (rc < 0) {
> > > + dev_dbg(dev, "Failed to get DC config: %d\n", rc);
> > > + return rc;
> > > + }
> > > +
> > > + mds->nr_dc_region += rc;
> > > +
> > > + if (mds->nr_dc_region < 1 || mds->nr_dc_region >
> > > CXL_MAX_DC_REGION) {
> > > + dev_err(dev, "Invalid num of dynamic capacity regions
> > > %d\n",
> > > + mds->nr_dc_region);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + for (i = start_region, j = 0; i < mds->nr_dc_region; i++, j++) {
> >
> > This should be 'j < mds->nr_dc_region'? Otherwise if your start region say
> > is '3' and you have '2' DC regions, you never enter the loop. Or does that
> > not happen? I also wonder if you need to check if 'start_region +
> > mds->nr_dc_region > CXL_MAX_DC_REGION'.
> >
> That can not happen, start_region was updated to the number of regions
> has returned till now (not counting the current call), while
> nr_dc_region is the total number of regions returned till now (including
> the current call) as we update it above, so start_region should never be
> larger
> than nr_dc_region.
Yep.
>
> > > + rc = cxl_dc_save_region_info(mds, i,
> > > &dc_resp->region[j]);
> > > + if (rc) {
> > > + dev_dbg(dev, "Failed to save region info:
> > > %d\n", rc);
>
> I am not sure why we sometimes use dev_err and sometimes we use dev_dbg
> here, if dcd is supported, error from getting dc configuration is an
> error to me.
We are trying to reduce the dev_err() use. cxl_dc_save_region_info() has
dev_err() which is much more specific as to the error. At worse this is
just redundant as a debug.
I'll remove it because the debug output is pretty verbose too.
Ira
>
> Fan
>
> > > + return rc;
> > > + }
> > > + }
> > > +
> > > + start_region = mds->nr_dc_region;
> > > +
> > > + } while (mds->nr_dc_region < dc_resp->avail_region_count);
> > > +
> > > + mds->dynamic_bytes =
> > > + mds->dc_region[mds->nr_dc_region - 1].base +
> > > + mds->dc_region[mds->nr_dc_region - 1].decode_len -
> > > + mds->dc_region[0].base;
> > > + dev_dbg(dev, "Total dynamic range: %#llx\n", mds->dynamic_bytes);
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(cxl_dev_dynamic_capacity_identify, CXL);
> > > +
> > > static int add_dpa_res(struct device *dev, struct resource *parent,
> > > struct resource *res, resource_size_t start,
> > > resource_size_t size, const char *type)
> > > @@ -1294,8 +1447,15 @@ int cxl_mem_create_range_info(struct
> > > cxl_memdev_state *mds)
> > > {
> > > struct cxl_dev_state *cxlds = &mds->cxlds;
> > > struct device *dev = cxlds->dev;
> > > + size_t untenanted_mem;
> > > int rc;
> > >
> > > + mds->total_bytes = mds->static_bytes;
> > > + if (mds->nr_dc_region) {
> > > + untenanted_mem = mds->dc_region[0].base - mds->static_bytes;
> > > + mds->total_bytes += untenanted_mem + mds->dynamic_bytes;
Re: [PATCH v3 06/25] cxl/mem: Read dynamic capacity configuration from the device
On Fri, Aug 16, 2024 at 02:45:47PM -0700, Dave Jiang wrote: > > > On 8/16/24 7:44 AM, [email protected] wrote: > > From: Navneet Singh > > > > Devices which optionally support Dynamic Capacity (DC) are configured > > via mailbox commands. CXL 3.1 requires the host to issue the Get DC > > Configuration command in order to properly configure DCDs. Without the > > Get DC Configuration command DCD can't be supported. > > > > Implement the DC mailbox commands as specified in CXL 3.1 section > > 8.2.9.9.9 (opcodes 48XXh) to read and store the DCD configuration > > information. Disable DCD if DCD is not supported. Leverage the Get DC > > Configuration command supported bit to indicate if DCD support. > > > > Linux has no use for the trailing fields of the Get Dynamic Capacity > > Configuration Output Payload (Total number of supported extents, number > > of available extents, total number of supported tags, and number of > > available tags). Avoid defining those fields to use the more useful > > dynamic C array. > > > > Cc: "Li, Ming" > > Signed-off-by: Navneet Singh > > Co-developed-by: Ira Weiny > > Signed-off-by: Ira Weiny > > > > --- > > Changes: > > [Li, Ming: Fix bug in total_bytes calculation] > > [iweiny: update commit message] > > [Jonathan: fix formatting] > > [Jonathan: Define block line size] > > [Jonathan/Fan: use regions returned field instead of macro in get config] > > [Jørgen: Rename memdev state range variables] > > [Jonathan: adjust use of rc in cxl_dev_dynamic_capacity_identify()] > > [Jonathan: white space cleanup] > > [fan: make a comment about the trailing configuration output fields] > > --- > > drivers/cxl/core/mbox.c | 171 > > +++- > > drivers/cxl/cxlmem.h| 64 +- > > drivers/cxl/pci.c | 4 ++ > > 3 files changed, 237 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > > index 8eb196858abe..68c26c4be91a 100644 > > --- a/drivers/cxl/core/mbox.c > > +++ b/drivers/cxl/core/mbox.c > > @@ -1157,7 +1157,7 @@ int cxl_dev_state_identify(struct cxl_memdev_state > > *mds) > > if (rc < 0) > > return rc; > > > > - mds->total_bytes = > > + mds->static_bytes = > > le64_to_cpu(id.total_capacity) * CXL_CAPACITY_MULTIPLIER; > > mds->volatile_only_bytes = > > le64_to_cpu(id.volatile_capacity) * CXL_CAPACITY_MULTIPLIER; > > @@ -1264,6 +1264,159 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 > > cmd) > > return rc; > > } > > > > +static int cxl_dc_save_region_info(struct cxl_memdev_state *mds, u8 index, > > + struct cxl_dc_region_config *region_config) > > +{ > > + struct cxl_dc_region_info *dcr = &mds->dc_region[index]; > > + struct device *dev = mds->cxlds.dev; > > + > > + dcr->base = le64_to_cpu(region_config->region_base); > > + dcr->decode_len = le64_to_cpu(region_config->region_decode_length); > > + dcr->decode_len *= CXL_CAPACITY_MULTIPLIER; > > + dcr->len = le64_to_cpu(region_config->region_length); > > + dcr->blk_size = le64_to_cpu(region_config->region_block_size); > > + dcr->dsmad_handle = le32_to_cpu(region_config->region_dsmad_handle); > > + dcr->flags = region_config->flags; > > + snprintf(dcr->name, CXL_DC_REGION_STRLEN, "dc%d", index); > > + > > + /* Check regions are in increasing DPA order */ > > + if (index > 0) { > > + struct cxl_dc_region_info *prev_dcr = &mds->dc_region[index - > > 1]; > > + > > + if ((prev_dcr->base + prev_dcr->decode_len) > dcr->base) { > > + dev_err(dev, > > + "DPA ordering violation for DC region %d and > > %d\n", > > + index - 1, index); > > + return -EINVAL; > > + } > > + } > > + > > + if (!IS_ALIGNED(dcr->base, SZ_256M) || > > + !IS_ALIGNED(dcr->base, dcr->blk_size)) { > > + dev_err(dev, "DC region %d invalid base %#llx blk size %#llx\n", > > + index, dcr->base, dcr->blk_size); > > + return -EINVAL; > > + } > > + > > + if (dcr->decode_len == 0 || dcr->len == 0 || dcr->decode_len < dcr->len > > || > > + !IS_ALIGNED(dcr->len, dcr->blk_size)) { > > + dev_err(dev, "DC region %d invalid length; decode %#llx len > > %#llx blk size %#llx\n", > > + index, dcr->decode_len, dcr->len, dcr->blk_size); > > + return -EINVAL; > > + } > > + > > + if (dcr->blk_size == 0 || dcr->blk_size % CXL_DCD_BLOCK_LINE_SIZE || > > + !is_power_of_2(dcr->blk_size)) { > > + dev_err(dev, "DC region %d invalid block size; %#llx\n", > > + index, dcr->blk_size); > > + return -EINVAL; > > + } > > + > > + dev_dbg(dev, > > + "DC region %s base %#llx length %#llx block size %#llx\n", > > + dcr->name, dcr->base, dcr->decode_len, dcr->blk_size); > > + > > +
Re: [PATCH v3 06/25] cxl/mem: Read dynamic capacity configuration from the device
On 8/16/24 7:44 AM, [email protected] wrote: > From: Navneet Singh > > Devices which optionally support Dynamic Capacity (DC) are configured > via mailbox commands. CXL 3.1 requires the host to issue the Get DC > Configuration command in order to properly configure DCDs. Without the > Get DC Configuration command DCD can't be supported. > > Implement the DC mailbox commands as specified in CXL 3.1 section > 8.2.9.9.9 (opcodes 48XXh) to read and store the DCD configuration > information. Disable DCD if DCD is not supported. Leverage the Get DC > Configuration command supported bit to indicate if DCD support. > > Linux has no use for the trailing fields of the Get Dynamic Capacity > Configuration Output Payload (Total number of supported extents, number > of available extents, total number of supported tags, and number of > available tags). Avoid defining those fields to use the more useful > dynamic C array. > > Cc: "Li, Ming" > Signed-off-by: Navneet Singh > Co-developed-by: Ira Weiny > Signed-off-by: Ira Weiny > > --- > Changes: > [Li, Ming: Fix bug in total_bytes calculation] > [iweiny: update commit message] > [Jonathan: fix formatting] > [Jonathan: Define block line size] > [Jonathan/Fan: use regions returned field instead of macro in get config] > [Jørgen: Rename memdev state range variables] > [Jonathan: adjust use of rc in cxl_dev_dynamic_capacity_identify()] > [Jonathan: white space cleanup] > [fan: make a comment about the trailing configuration output fields] > --- > drivers/cxl/core/mbox.c | 171 > +++- > drivers/cxl/cxlmem.h| 64 +- > drivers/cxl/pci.c | 4 ++ > 3 files changed, 237 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index 8eb196858abe..68c26c4be91a 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -1157,7 +1157,7 @@ int cxl_dev_state_identify(struct cxl_memdev_state *mds) > if (rc < 0) > return rc; > > - mds->total_bytes = > + mds->static_bytes = > le64_to_cpu(id.total_capacity) * CXL_CAPACITY_MULTIPLIER; > mds->volatile_only_bytes = > le64_to_cpu(id.volatile_capacity) * CXL_CAPACITY_MULTIPLIER; > @@ -1264,6 +1264,159 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 > cmd) > return rc; > } > > +static int cxl_dc_save_region_info(struct cxl_memdev_state *mds, u8 index, > +struct cxl_dc_region_config *region_config) > +{ > + struct cxl_dc_region_info *dcr = &mds->dc_region[index]; > + struct device *dev = mds->cxlds.dev; > + > + dcr->base = le64_to_cpu(region_config->region_base); > + dcr->decode_len = le64_to_cpu(region_config->region_decode_length); > + dcr->decode_len *= CXL_CAPACITY_MULTIPLIER; > + dcr->len = le64_to_cpu(region_config->region_length); > + dcr->blk_size = le64_to_cpu(region_config->region_block_size); > + dcr->dsmad_handle = le32_to_cpu(region_config->region_dsmad_handle); > + dcr->flags = region_config->flags; > + snprintf(dcr->name, CXL_DC_REGION_STRLEN, "dc%d", index); > + > + /* Check regions are in increasing DPA order */ > + if (index > 0) { > + struct cxl_dc_region_info *prev_dcr = &mds->dc_region[index - > 1]; > + > + if ((prev_dcr->base + prev_dcr->decode_len) > dcr->base) { > + dev_err(dev, > + "DPA ordering violation for DC region %d and > %d\n", > + index - 1, index); > + return -EINVAL; > + } > + } > + > + if (!IS_ALIGNED(dcr->base, SZ_256M) || > + !IS_ALIGNED(dcr->base, dcr->blk_size)) { > + dev_err(dev, "DC region %d invalid base %#llx blk size %#llx\n", > + index, dcr->base, dcr->blk_size); > + return -EINVAL; > + } > + > + if (dcr->decode_len == 0 || dcr->len == 0 || dcr->decode_len < dcr->len > || > + !IS_ALIGNED(dcr->len, dcr->blk_size)) { > + dev_err(dev, "DC region %d invalid length; decode %#llx len > %#llx blk size %#llx\n", > + index, dcr->decode_len, dcr->len, dcr->blk_size); > + return -EINVAL; > + } > + > + if (dcr->blk_size == 0 || dcr->blk_size % CXL_DCD_BLOCK_LINE_SIZE || > + !is_power_of_2(dcr->blk_size)) { > + dev_err(dev, "DC region %d invalid block size; %#llx\n", > + index, dcr->blk_size); > + return -EINVAL; > + } > + > + dev_dbg(dev, > + "DC region %s base %#llx length %#llx block size %#llx\n", > + dcr->name, dcr->base, dcr->decode_len, dcr->blk_size); > + > + return 0; > +} > + > +/* Returns the number of regions in dc_resp or -ERRNO */ > +static int cxl_get_dc_config(struct cxl_memdev_state *mds, u8 start_region, > + struct cxl_
