[PATCH 0/3] Query and use Partition Info

[PATCH 0/3] Query and use Partition Info

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Three small patches which query and report Partition Info.  In addition,
correct ram and pmem size calculations to account for potential partitioned
space.

Ira Weiny (3):
  cxl/pci: Store memory capacity values
  cxl/mem: Report correct ram/pmem size in sysfs
  cxl/mem: Add partition information to sysfs

 drivers/cxl/mem.h |   4 ++
 drivers/cxl/pci.c | 178 +++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 174 insertions(+), 8 deletions(-)

-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH 1/3] cxl/pci: Store memory capacity values

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

The Identify Memory Device command returns information about the
volatile and persistent memory capacities.  Store those values in the
cxl_mem structure for later use.  While at it, reuse the calculation of
the volatile and persistent memory byte values to calculate the ram and
pmem ranges.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/cxl/mem.h |  4 ++++
 drivers/cxl/pci.c | 36 +++++++++++++++++++++++++++++++++---
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index 13868ff7cadf..8bd0d0506b97 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -75,5 +75,9 @@ struct cxl_mem {
 
 	struct range pmem_range;
 	struct range ram_range;
+	u64 total_cap_bytes;
+	u64 volatile_cap_bytes;
+	u64 persistent_cap_bytes;
+	u64 partition_align_bytes;
 };
 #endif /* __CXL_MEM_H__ */
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 5a1705b52278..9995f97d3b28 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -57,6 +57,15 @@ enum opcode {
 	CXL_MBOX_OP_MAX			= 0x10000
 };
 
+/*
+ * CXL 2.0 - Memory capacity multiplier
+ * See Section 8.2.9.5
+ *
+ * Volatile, Persistent, and Partition capacities are specified to be in
+ * multiples of 256MB - define a multiplier to convert to/from bytes.
+ */
+#define CXL_CAPACITY_MULTIPLIER SZ_256M
+
 /**
  * struct mbox_cmd - A command to be submitted to hardware.
  * @opcode: (input) The command set and command submitted to hardware.
@@ -1542,16 +1551,37 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
 	if (rc < 0)
 		return rc;
 
+	cxlm->total_cap_bytes = le64_to_cpu(id.total_capacity);
+	cxlm->total_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
+
+	cxlm->volatile_cap_bytes = le64_to_cpu(id.volatile_capacity);
+	cxlm->volatile_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
+
+	cxlm->persistent_cap_bytes = le64_to_cpu(id.persistent_capacity);
+	cxlm->persistent_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
+
+	cxlm->partition_align_bytes = le64_to_cpu(id.partition_align);
+	cxlm->partition_align_bytes *= CXL_CAPACITY_MULTIPLIER;
+
+	dev_dbg(&cxlm->pdev->dev, "Identify Memory Device\n"
+		"     total_cap_bytes = %#llx\n"
+		"     volatile_cap_bytes = %#llx\n"
+		"     persistent_cap_bytes = %#llx\n"
+		"     partition_align_bytes = %#llx\n",
+			cxlm->total_cap_bytes,
+			cxlm->volatile_cap_bytes,
+			cxlm->persistent_cap_bytes,
+			cxlm->partition_align_bytes);
+
 	/*
 	 * TODO: enumerate DPA map, as 'ram' and 'pmem' do not alias.
 	 * For now, only the capacity is exported in sysfs
 	 */
 	cxlm->ram_range.start = 0;
-	cxlm->ram_range.end = le64_to_cpu(id.volatile_capacity) * SZ_256M - 1;
+	cxlm->ram_range.end = cxlm->volatile_cap_bytes - 1;
 
 	cxlm->pmem_range.start = 0;
-	cxlm->pmem_range.end =
-		le64_to_cpu(id.persistent_capacity) * SZ_256M - 1;
+	cxlm->pmem_range.end = cxlm->persistent_cap_bytes - 1;
 
 	cxlm->lsa_size = le32_to_cpu(id.lsa_size);
 	memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision));
-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH 2/3] cxl/mem: Report correct ram/pmem size in sysfs

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Memory devices may specify volatile only, persistent only, and
partitionable space which when added together result in a total capacity.

The partitionable space is configurable between volatile and persistent
space.  To account for the dynamic partitionable space the correct ram
and pmem size information is reported in the Get Partition Info device
command.

Define cxl_mem_get_partition() and call it to retrieve the correct
ram and pmem ranges sizes.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/cxl/pci.c | 97 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 90 insertions(+), 7 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 9995f97d3b28..bcc2829e4475 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1455,6 +1455,62 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_mem *cxlm)
 	return ret;
 }
 
+/**
+ * cxl_mem_get_partition_info - Set partition info
+ * @cxlm: The device to act on
+ * @active_volatile_cap_bytes: returned active volatile capacity; in bytes
+ * @active_persistent_cap_bytes: returned active persistent capacity; in bytes
+ * @next_volatile_cap_bytes: return next volatile capacity; in bytes
+ * @next_persistent_cap_bytes: return next persistent capacity; in bytes
+ *
+ * Retrieve the current partition info for the device specified.  The active
+ * values are the current capacity in bytes.  If not 0, the 'next' values are
+ * the pending values, in bytes, which take affect on next cold reset.
+ *
+ * Return: 0 if no error: or the result of the mailbox command.
+ *
+ * See CXL @8.2.9.5.2.1 Get Partition Info
+ */
+int cxl_mem_get_partition_info(struct cxl_mem *cxlm,
+			       u64 *active_volatile_cap_bytes,
+			       u64 *active_persistent_cap_bytes,
+			       u64 *next_volatile_cap_bytes,
+			       u64 *next_persistent_cap_bytes)
+{
+	struct cxl_mbox_get_partition_info {
+		u64 active_volatile_cap;
+		u64 active_persistent_cap;
+		u64 next_volatile_cap;
+		u64 next_persistent_cap;
+	} __packed pi;
+	int rc;
+
+	/* On error report 0 */
+	*active_volatile_cap_bytes = 0;
+	*active_persistent_cap_bytes = 0;
+	*next_volatile_cap_bytes = 0;
+	*next_persistent_cap_bytes = 0;
+
+	rc = cxl_mem_mbox_send_cmd(cxlm, CXL_MBOX_OP_GET_PARTITION_INFO,
+				   NULL, 0, &pi, sizeof(pi));
+
+	if (rc)
+		return rc;
+
+	*active_volatile_cap_bytes = le64_to_cpu(pi.active_volatile_cap);
+	*active_persistent_cap_bytes = le64_to_cpu(pi.active_persistent_cap);
+	*next_volatile_cap_bytes = le64_to_cpu(pi.next_volatile_cap);
+	*next_persistent_cap_bytes = le64_to_cpu(pi.next_volatile_cap);
+
+	*active_volatile_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
+	*active_persistent_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
+	*next_volatile_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
+	*next_persistent_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cxl_mem_get_partition_info);
+
 /**
  * cxl_mem_enumerate_cmds() - Enumerate commands for a device.
  * @cxlm: The device.
@@ -1573,20 +1629,45 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
 			cxlm->persistent_cap_bytes,
 			cxlm->partition_align_bytes);
 
+	cxlm->lsa_size = le32_to_cpu(id.lsa_size);
+	memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision));
+
+	return 0;
+}
+
+static void cxl_mem_create_range_info(struct cxl_mem *cxlm)
+{
+	u64 active_volatile_cap_bytes;
+	u64 active_persistent_cap_bytes;
+	u64 next_volatile_cap_bytes;
+	u64 next_persistent_cap_bytes;
+
+	if (cxl_mem_get_partition_info(cxlm,
+			       &active_volatile_cap_bytes,
+			       &active_persistent_cap_bytes,
+			       &next_volatile_cap_bytes,
+			       &next_persistent_cap_bytes))
+		dev_err(&cxlm->pdev->dev, "Failed to query partition information\n");
+
+	dev_dbg(&cxlm->pdev->dev, "Get Partition Info\n"
+		"     active_volatile_cap_bytes = %#llx\n"
+		"     active_persistent_cap_bytes = %#llx\n"
+		"     next_volatile_cap_bytes = %#llx\n"
+		"     next_persistent_cap_bytes = %#llx\n",
+			active_volatile_cap_bytes,
+			active_persistent_cap_bytes,
+			next_volatile_cap_bytes,
+			next_persistent_cap_bytes);
+
 	/*
 	 * TODO: enumerate DPA map, as 'ram' and 'pmem' do not alias.
 	 * For now, only the capacity is exported in sysfs
 	 */
 	cxlm->ram_range.start = 0;
-	cxlm->ram_range.end = cxlm->volatile_cap_bytes - 1;
+	cxlm->ram_range.end = active_volatile_cap_bytes - 1;
 
 	cxlm->pmem_range.start = 0;
-	cxlm->pmem_range.end = cxlm->persistent_cap_bytes - 1;
-
-	cxlm->lsa_size = le32_to_cpu(id.lsa_size);
-	memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision));
-
-	return 0;
+	cxlm->pmem_range.end = active_persistent_cap_bytes - 1;
 }
 
 static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
@@ -1618,6 +1699,8 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
+	cxl_mem_create_range_info(cxlm);
+
 	return cxl_mem_add_memdev(cxlm);
 }
 
-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH 3/3] cxl/mem: Add partition information to sysfs

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Partitionable space is added to the ram and pmem size sysfs attributes
but this does not show the entire story.

Add full partition information about the device under <sysfs>/partition.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/cxl/pci.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index bcc2829e4475..7e4e9605e4ed 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1226,6 +1226,42 @@ static ssize_t pmem_size_show(struct device *dev, struct device_attribute *attr,
 static struct device_attribute dev_attr_pmem_size =
 	__ATTR(size, 0444, pmem_size_show, NULL);
 
+static ssize_t part_vo_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+	struct cxl_mem *cxlm = cxlmd->cxlm;
+
+	return sysfs_emit(buf, "%#llx\n", cxlm->volatile_cap_bytes);
+}
+
+static struct device_attribute dev_attr_part_vo =
+	__ATTR(volatile_only, 0444, part_vo_show, NULL);
+
+static ssize_t part_po_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+	struct cxl_mem *cxlm = cxlmd->cxlm;
+
+	return sysfs_emit(buf, "%#llx\n", cxlm->persistent_cap_bytes);
+}
+
+static struct device_attribute dev_attr_part_po =
+	__ATTR(persistent_only, 0444, part_po_show, NULL);
+
+static ssize_t part_total_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+	struct cxl_mem *cxlm = cxlmd->cxlm;
+
+	return sysfs_emit(buf, "%#llx\n", cxlm->total_cap_bytes);
+}
+
+static struct device_attribute dev_attr_part_total =
+	__ATTR(total, 0444, part_total_show, NULL);
+
 static struct attribute *cxl_memdev_attributes[] = {
 	&dev_attr_firmware_version.attr,
 	&dev_attr_payload_max.attr,
@@ -1243,6 +1279,13 @@ static struct attribute *cxl_memdev_ram_attributes[] = {
 	NULL,
 };
 
+static struct attribute *cxl_memdev_part_attributes[] = {
+	&dev_attr_part_vo.attr,
+	&dev_attr_part_po.attr,
+	&dev_attr_part_total.attr,
+	NULL,
+};
+
 static struct attribute_group cxl_memdev_attribute_group = {
 	.attrs = cxl_memdev_attributes,
 };
@@ -1257,10 +1300,16 @@ static struct attribute_group cxl_memdev_pmem_attribute_group = {
 	.attrs = cxl_memdev_pmem_attributes,
 };
 
+static struct attribute_group cxl_memdev_part_attribute_group = {
+	.name = "partition",
+	.attrs = cxl_memdev_part_attributes,
+};
+
 static const struct attribute_group *cxl_memdev_attribute_groups[] = {
 	&cxl_memdev_attribute_group,
 	&cxl_memdev_ram_attribute_group,
 	&cxl_memdev_pmem_attribute_group,
+	&cxl_memdev_part_attribute_group,
 	NULL,
 };
 
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH 2/3] cxl/mem: Report correct ram/pmem size in sysfs

[Jonathan Cameron]

On Thu, 10 Jun 2021 17:22:23 -0700
<ira.weiny@intel.com> wrote:

> From: Ira Weiny <ira.weiny@intel.com>
> 
> Memory devices may specify volatile only, persistent only, and
> partitionable space which when added together result in a total capacity.
> 
> The partitionable space is configurable between volatile and persistent
> space.  To account for the dynamic partitionable space the correct ram
> and pmem size information is reported in the Get Partition Info device
> command.
> 
> Define cxl_mem_get_partition() and call it to retrieve the correct
> ram and pmem ranges sizes.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/cxl/pci.c | 97 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 90 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 9995f97d3b28..bcc2829e4475 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1455,6 +1455,62 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_mem *cxlm)
>  	return ret;
>  }
>  
> +/**
> + * cxl_mem_get_partition_info - Set partition info

Having a function call _get_ that is documented as "Set" is somewhat unusual.

> + * @cxlm: The device to act on
> + * @active_volatile_cap_bytes: returned active volatile capacity; in bytes
> + * @active_persistent_cap_bytes: returned active persistent capacity; in bytes
> + * @next_volatile_cap_bytes: return next volatile capacity; in bytes
> + * @next_persistent_cap_bytes: return next persistent capacity; in bytes
> + *
> + * Retrieve the current partition info for the device specified.  The active
> + * values are the current capacity in bytes.  If not 0, the 'next' values are
> + * the pending values, in bytes, which take affect on next cold reset.
> + *
> + * Return: 0 if no error: or the result of the mailbox command.
> + *
> + * See CXL @8.2.9.5.2.1 Get Partition Info
> + */
> +int cxl_mem_get_partition_info(struct cxl_mem *cxlm,
> +			       u64 *active_volatile_cap_bytes,
> +			       u64 *active_persistent_cap_bytes,
> +			       u64 *next_volatile_cap_bytes,
> +			       u64 *next_persistent_cap_bytes)
> +{
> +	struct cxl_mbox_get_partition_info {
> +		u64 active_volatile_cap;
> +		u64 active_persistent_cap;
> +		u64 next_volatile_cap;
> +		u64 next_persistent_cap;
> +	} __packed pi;
> +	int rc;
> +
> +	/* On error report 0 */

This command is optional... See below.

> +	*active_volatile_cap_bytes = 0;
> +	*active_persistent_cap_bytes = 0;
> +	*next_volatile_cap_bytes = 0;
> +	*next_persistent_cap_bytes = 0;
> +
> +	rc = cxl_mem_mbox_send_cmd(cxlm, CXL_MBOX_OP_GET_PARTITION_INFO,
> +				   NULL, 0, &pi, sizeof(pi));
> +
> +	if (rc)
> +		return rc;
> +
> +	*active_volatile_cap_bytes = le64_to_cpu(pi.active_volatile_cap);
> +	*active_persistent_cap_bytes = le64_to_cpu(pi.active_persistent_cap);
> +	*next_volatile_cap_bytes = le64_to_cpu(pi.next_volatile_cap);
> +	*next_persistent_cap_bytes = le64_to_cpu(pi.next_volatile_cap);
> +
> +	*active_volatile_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
> +	*active_persistent_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
> +	*next_volatile_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
> +	*next_persistent_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cxl_mem_get_partition_info);
> +
>  /**
>   * cxl_mem_enumerate_cmds() - Enumerate commands for a device.
>   * @cxlm: The device.
> @@ -1573,20 +1629,45 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
>  			cxlm->persistent_cap_bytes,
>  			cxlm->partition_align_bytes);
>  
> +	cxlm->lsa_size = le32_to_cpu(id.lsa_size);
> +	memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision));
> +
> +	return 0;
> +}
> +
> +static void cxl_mem_create_range_info(struct cxl_mem *cxlm)
> +{
> +	u64 active_volatile_cap_bytes;
> +	u64 active_persistent_cap_bytes;
> +	u64 next_volatile_cap_bytes;
> +	u64 next_persistent_cap_bytes;
> +
> +	if (cxl_mem_get_partition_info(cxlm,
> +			       &active_volatile_cap_bytes,
> +			       &active_persistent_cap_bytes,
> +			       &next_volatile_cap_bytes,
> +			       &next_persistent_cap_bytes))
> +		dev_err(&cxlm->pdev->dev, "Failed to query partition information\n");
> +
> +	dev_dbg(&cxlm->pdev->dev, "Get Partition Info\n"
> +		"     active_volatile_cap_bytes = %#llx\n"
> +		"     active_persistent_cap_bytes = %#llx\n"
> +		"     next_volatile_cap_bytes = %#llx\n"
> +		"     next_persistent_cap_bytes = %#llx\n",
> +			active_volatile_cap_bytes,
> +			active_persistent_cap_bytes,
> +			next_volatile_cap_bytes,
> +			next_persistent_cap_bytes);
> +
>  	/*
>  	 * TODO: enumerate DPA map, as 'ram' and 'pmem' do not alias.
>  	 * For now, only the capacity is exported in sysfs
>  	 */
>  	cxlm->ram_range.start = 0;
> -	cxlm->ram_range.end = cxlm->volatile_cap_bytes - 1;
> +	cxlm->ram_range.end = active_volatile_cap_bytes - 1;

This change looks to wipe out the valid settings on a device
that only does fixed allocations to pmem vs volatile.

>  
>  	cxlm->pmem_range.start = 0;
> -	cxlm->pmem_range.end = cxlm->persistent_cap_bytes - 1;
> -
> -	cxlm->lsa_size = le32_to_cpu(id.lsa_size);
> -	memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision));
> -
> -	return 0;
> +	cxlm->pmem_range.end = active_persistent_cap_bytes - 1;
>  }
>  
>  static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> @@ -1618,6 +1699,8 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> +	cxl_mem_create_range_info(cxlm);
> +
>  	return cxl_mem_add_memdev(cxlm);
>  }
>  

Re: [PATCH 3/3] cxl/mem: Add partition information to sysfs

[Jonathan Cameron]

On Thu, 10 Jun 2021 17:22:24 -0700
<ira.weiny@intel.com> wrote:

> From: Ira Weiny <ira.weiny@intel.com>
> 
> Partitionable space is added to the ram and pmem size sysfs attributes
> but this does not show the entire story.
> 
> Add full partition information about the device under <sysfs>/partition.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/cxl/pci.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index bcc2829e4475..7e4e9605e4ed 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1226,6 +1226,42 @@ static ssize_t pmem_size_show(struct device *dev, struct device_attribute *attr,
>  static struct device_attribute dev_attr_pmem_size =
>  	__ATTR(size, 0444, pmem_size_show, NULL);
>  
> +static ssize_t part_vo_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +	struct cxl_mem *cxlm = cxlmd->cxlm;
> +
> +	return sysfs_emit(buf, "%#llx\n", cxlm->volatile_cap_bytes);
> +}
> +
> +static struct device_attribute dev_attr_part_vo =
> +	__ATTR(volatile_only, 0444, part_vo_show, NULL);

With a bit of tweaking could we use DEVICE_ATTR_RO for these and have even less
boilerplate? 


> +
> +static ssize_t part_po_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +	struct cxl_mem *cxlm = cxlmd->cxlm;
> +
> +	return sysfs_emit(buf, "%#llx\n", cxlm->persistent_cap_bytes);
> +}
> +
> +static struct device_attribute dev_attr_part_po =
> +	__ATTR(persistent_only, 0444, part_po_show, NULL);
> +
> +static ssize_t part_total_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +	struct cxl_mem *cxlm = cxlmd->cxlm;
> +
> +	return sysfs_emit(buf, "%#llx\n", cxlm->total_cap_bytes);
> +}
> +
> +static struct device_attribute dev_attr_part_total =
> +	__ATTR(total, 0444, part_total_show, NULL);
> +
>  static struct attribute *cxl_memdev_attributes[] = {
>  	&dev_attr_firmware_version.attr,
>  	&dev_attr_payload_max.attr,
> @@ -1243,6 +1279,13 @@ static struct attribute *cxl_memdev_ram_attributes[] = {
>  	NULL,
>  };
>  
> +static struct attribute *cxl_memdev_part_attributes[] = {
> +	&dev_attr_part_vo.attr,
> +	&dev_attr_part_po.attr,
> +	&dev_attr_part_total.attr,
> +	NULL,
> +};
> +
>  static struct attribute_group cxl_memdev_attribute_group = {
>  	.attrs = cxl_memdev_attributes,
>  };
> @@ -1257,10 +1300,16 @@ static struct attribute_group cxl_memdev_pmem_attribute_group = {
>  	.attrs = cxl_memdev_pmem_attributes,
>  };
>  
> +static struct attribute_group cxl_memdev_part_attribute_group = {
> +	.name = "partition",
> +	.attrs = cxl_memdev_part_attributes,
> +};
> +
>  static const struct attribute_group *cxl_memdev_attribute_groups[] = {
>  	&cxl_memdev_attribute_group,
>  	&cxl_memdev_ram_attribute_group,
>  	&cxl_memdev_pmem_attribute_group,
> +	&cxl_memdev_part_attribute_group,
>  	NULL,
>  };
>  

Re: [PATCH 2/3] cxl/mem: Report correct ram/pmem size in sysfs

[Ira Weiny]

> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index 9995f97d3b28..bcc2829e4475 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -1455,6 +1455,62 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_mem *cxlm)
> >  	return ret;
> >  }
> >  
> > +/**
> > + * cxl_mem_get_partition_info - Set partition info
> 
> Having a function call _get_ that is documented as "Set" is somewhat unusual.

Oops...  yea...

> > + * @cxlm: The device to act on
> > + * @active_volatile_cap_bytes: returned active volatile capacity; in bytes
> > + * @active_persistent_cap_bytes: returned active persistent capacity; in bytes
> > + * @next_volatile_cap_bytes: return next volatile capacity; in bytes
> > + * @next_persistent_cap_bytes: return next persistent capacity; in bytes
> > + *
> > + * Retrieve the current partition info for the device specified.  The active
> > + * values are the current capacity in bytes.  If not 0, the 'next' values are
> > + * the pending values, in bytes, which take affect on next cold reset.
> > + *
> > + * Return: 0 if no error: or the result of the mailbox command.
> > + *
> > + * See CXL @8.2.9.5.2.1 Get Partition Info
> > + */
> > +int cxl_mem_get_partition_info(struct cxl_mem *cxlm,
> > +			       u64 *active_volatile_cap_bytes,
> > +			       u64 *active_persistent_cap_bytes,
> > +			       u64 *next_volatile_cap_bytes,
> > +			       u64 *next_persistent_cap_bytes)
> > +{
> > +	struct cxl_mbox_get_partition_info {
> > +		u64 active_volatile_cap;
> > +		u64 active_persistent_cap;
> > +		u64 next_volatile_cap;
> > +		u64 next_persistent_cap;
> > +	} __packed pi;
> > +	int rc;
> > +
> > +	/* On error report 0 */
> 
> This command is optional... See below.

I did not realize that...  Ok, this will not work then.

> >  	/*
> >  	 * TODO: enumerate DPA map, as 'ram' and 'pmem' do not alias.
> >  	 * For now, only the capacity is exported in sysfs
> >  	 */
> >  	cxlm->ram_range.start = 0;
> > -	cxlm->ram_range.end = cxlm->volatile_cap_bytes - 1;
> > +	cxlm->ram_range.end = active_volatile_cap_bytes - 1;
> 
> This change looks to wipe out the valid settings on a device
> that only does fixed allocations to pmem vs volatile.

Yes it sure will...  :-(

Thanks for pointing it out...
Ira

Re: [PATCH 2/3] cxl/mem: Report correct ram/pmem size in sysfs

[Dan Williams]

On Thu, Jun 10, 2021 at 5:22 PM <ira.weiny@intel.com> wrote:
>
> From: Ira Weiny <ira.weiny@intel.com>
>
> Memory devices may specify volatile only, persistent only, and
> partitionable space which when added together result in a total capacity.
>
> The partitionable space is configurable between volatile and persistent
> space.  To account for the dynamic partitionable space the correct ram
> and pmem size information is reported in the Get Partition Info device
> command.
>
> Define cxl_mem_get_partition() and call it to retrieve the correct
> ram and pmem ranges sizes.
>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/cxl/pci.c | 97 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 90 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 9995f97d3b28..bcc2829e4475 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1455,6 +1455,62 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_mem *cxlm)
>         return ret;
>  }
>
> +/**
> + * cxl_mem_get_partition_info - Set partition info
> + * @cxlm: The device to act on
> + * @active_volatile_cap_bytes: returned active volatile capacity; in bytes
> + * @active_persistent_cap_bytes: returned active persistent capacity; in bytes
> + * @next_volatile_cap_bytes: return next volatile capacity; in bytes
> + * @next_persistent_cap_bytes: return next persistent capacity; in bytes
> + *
> + * Retrieve the current partition info for the device specified.  The active
> + * values are the current capacity in bytes.  If not 0, the 'next' values are
> + * the pending values, in bytes, which take affect on next cold reset.
> + *
> + * Return: 0 if no error: or the result of the mailbox command.
> + *
> + * See CXL @8.2.9.5.2.1 Get Partition Info
> + */
> +int cxl_mem_get_partition_info(struct cxl_mem *cxlm,
> +                              u64 *active_volatile_cap_bytes,
> +                              u64 *active_persistent_cap_bytes,
> +                              u64 *next_volatile_cap_bytes,
> +                              u64 *next_persistent_cap_bytes)

Similar to how cxl_mem_identify() populates data in cxl_mem, I would
expect this routine to do the same.

> +{
> +       struct cxl_mbox_get_partition_info {
> +               u64 active_volatile_cap;
> +               u64 active_persistent_cap;
> +               u64 next_volatile_cap;
> +               u64 next_persistent_cap;
> +       } __packed pi;
> +       int rc;
> +
> +       /* On error report 0 */
> +       *active_volatile_cap_bytes = 0;
> +       *active_persistent_cap_bytes = 0;
> +       *next_volatile_cap_bytes = 0;
> +       *next_persistent_cap_bytes = 0;
> +
> +       rc = cxl_mem_mbox_send_cmd(cxlm, CXL_MBOX_OP_GET_PARTITION_INFO,
> +                                  NULL, 0, &pi, sizeof(pi));
> +
> +       if (rc)
> +               return rc;
> +
> +       *active_volatile_cap_bytes = le64_to_cpu(pi.active_volatile_cap);
> +       *active_persistent_cap_bytes = le64_to_cpu(pi.active_persistent_cap);
> +       *next_volatile_cap_bytes = le64_to_cpu(pi.next_volatile_cap);
> +       *next_persistent_cap_bytes = le64_to_cpu(pi.next_volatile_cap);
> +
> +       *active_volatile_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
> +       *active_persistent_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
> +       *next_volatile_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
> +       *next_persistent_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(cxl_mem_get_partition_info);

Why is this exported? Only the PCI driver cares about this.

Re: [PATCH 1/3] cxl/pci: Store memory capacity values

[Ben Widawsky]

On 21-06-10 17:22:22, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> The Identify Memory Device command returns information about the
> volatile and persistent memory capacities.  Store those values in the
> cxl_mem structure for later use.  While at it, reuse the calculation of
> the volatile and persistent memory byte values to calculate the ram and
> pmem ranges.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

One comment below that can be taken or left...

Acked-by: Ben Widawsky <ben.widawsky@intel.com>

> ---
>  drivers/cxl/mem.h |  4 ++++
>  drivers/cxl/pci.c | 36 +++++++++++++++++++++++++++++++++---
>  2 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
> index 13868ff7cadf..8bd0d0506b97 100644
> --- a/drivers/cxl/mem.h
> +++ b/drivers/cxl/mem.h
> @@ -75,5 +75,9 @@ struct cxl_mem {
>  
>  	struct range pmem_range;
>  	struct range ram_range;
> +	u64 total_cap_bytes;
> +	u64 volatile_cap_bytes;
> +	u64 persistent_cap_bytes;

I'd either drop 'cap' entirely or type it out. I don't think 'cap' is a
descriptive name.

> +	u64 partition_align_bytes;
>  };
>  #endif /* __CXL_MEM_H__ */
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 5a1705b52278..9995f97d3b28 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -57,6 +57,15 @@ enum opcode {
>  	CXL_MBOX_OP_MAX			= 0x10000
>  };
>  
> +/*
> + * CXL 2.0 - Memory capacity multiplier
> + * See Section 8.2.9.5
> + *
> + * Volatile, Persistent, and Partition capacities are specified to be in
> + * multiples of 256MB - define a multiplier to convert to/from bytes.
> + */
> +#define CXL_CAPACITY_MULTIPLIER SZ_256M
> +
>  /**
>   * struct mbox_cmd - A command to be submitted to hardware.
>   * @opcode: (input) The command set and command submitted to hardware.
> @@ -1542,16 +1551,37 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
>  	if (rc < 0)
>  		return rc;
>  
> +	cxlm->total_cap_bytes = le64_to_cpu(id.total_capacity);
> +	cxlm->total_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
> +
> +	cxlm->volatile_cap_bytes = le64_to_cpu(id.volatile_capacity);
> +	cxlm->volatile_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
> +
> +	cxlm->persistent_cap_bytes = le64_to_cpu(id.persistent_capacity);
> +	cxlm->persistent_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
> +
> +	cxlm->partition_align_bytes = le64_to_cpu(id.partition_align);
> +	cxlm->partition_align_bytes *= CXL_CAPACITY_MULTIPLIER;
> +
> +	dev_dbg(&cxlm->pdev->dev, "Identify Memory Device\n"
> +		"     total_cap_bytes = %#llx\n"
> +		"     volatile_cap_bytes = %#llx\n"
> +		"     persistent_cap_bytes = %#llx\n"
> +		"     partition_align_bytes = %#llx\n",
> +			cxlm->total_cap_bytes,
> +			cxlm->volatile_cap_bytes,
> +			cxlm->persistent_cap_bytes,
> +			cxlm->partition_align_bytes);
> +
>  	/*
>  	 * TODO: enumerate DPA map, as 'ram' and 'pmem' do not alias.
>  	 * For now, only the capacity is exported in sysfs
>  	 */
>  	cxlm->ram_range.start = 0;
> -	cxlm->ram_range.end = le64_to_cpu(id.volatile_capacity) * SZ_256M - 1;
> +	cxlm->ram_range.end = cxlm->volatile_cap_bytes - 1;
>  
>  	cxlm->pmem_range.start = 0;
> -	cxlm->pmem_range.end =
> -		le64_to_cpu(id.persistent_capacity) * SZ_256M - 1;
> +	cxlm->pmem_range.end = cxlm->persistent_cap_bytes - 1;
>  
>  	cxlm->lsa_size = le32_to_cpu(id.lsa_size);
>  	memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision));
> -- 
> 2.28.0.rc0.12.gb6a658bd00c9
> 

Re: [PATCH 3/3] cxl/mem: Add partition information to sysfs

[Ben Widawsky]

On 21-06-10 17:22:24, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> Partitionable space is added to the ram and pmem size sysfs attributes
> but this does not show the entire story.
> 
> Add full partition information about the device under <sysfs>/partition.

Please add these to:
Documentation/ABI/testing/sysfs-bus-cxl

It might be time to break that file up... maybe a syfs-bus-cxl-memdev?

I'll also pose the original question here, what's the justification for exposing
this via sysfs versus having userspace executing the mailbox command itself? Are
you planning to have these attributes be writable at some point to do the
partition programming?

> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/cxl/pci.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index bcc2829e4475..7e4e9605e4ed 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1226,6 +1226,42 @@ static ssize_t pmem_size_show(struct device *dev, struct device_attribute *attr,
>  static struct device_attribute dev_attr_pmem_size =
>  	__ATTR(size, 0444, pmem_size_show, NULL);
>  
> +static ssize_t part_vo_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +	struct cxl_mem *cxlm = cxlmd->cxlm;
> +
> +	return sysfs_emit(buf, "%#llx\n", cxlm->volatile_cap_bytes);
> +}
> +
> +static struct device_attribute dev_attr_part_vo =
> +	__ATTR(volatile_only, 0444, part_vo_show, NULL);
> +
> +static ssize_t part_po_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +	struct cxl_mem *cxlm = cxlmd->cxlm;
> +
> +	return sysfs_emit(buf, "%#llx\n", cxlm->persistent_cap_bytes);
> +}
> +
> +static struct device_attribute dev_attr_part_po =
> +	__ATTR(persistent_only, 0444, part_po_show, NULL);
> +
> +static ssize_t part_total_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +	struct cxl_mem *cxlm = cxlmd->cxlm;
> +
> +	return sysfs_emit(buf, "%#llx\n", cxlm->total_cap_bytes);
> +}
> +
> +static struct device_attribute dev_attr_part_total =
> +	__ATTR(total, 0444, part_total_show, NULL);
> +
>  static struct attribute *cxl_memdev_attributes[] = {
>  	&dev_attr_firmware_version.attr,
>  	&dev_attr_payload_max.attr,
> @@ -1243,6 +1279,13 @@ static struct attribute *cxl_memdev_ram_attributes[] = {
>  	NULL,
>  };
>  
> +static struct attribute *cxl_memdev_part_attributes[] = {
> +	&dev_attr_part_vo.attr,
> +	&dev_attr_part_po.attr,
> +	&dev_attr_part_total.attr,
> +	NULL,
> +};
> +
>  static struct attribute_group cxl_memdev_attribute_group = {
>  	.attrs = cxl_memdev_attributes,
>  };
> @@ -1257,10 +1300,16 @@ static struct attribute_group cxl_memdev_pmem_attribute_group = {
>  	.attrs = cxl_memdev_pmem_attributes,
>  };
>  
> +static struct attribute_group cxl_memdev_part_attribute_group = {
> +	.name = "partition",
> +	.attrs = cxl_memdev_part_attributes,
> +};
> +
>  static const struct attribute_group *cxl_memdev_attribute_groups[] = {
>  	&cxl_memdev_attribute_group,
>  	&cxl_memdev_ram_attribute_group,
>  	&cxl_memdev_pmem_attribute_group,
> +	&cxl_memdev_part_attribute_group,
>  	NULL,
>  };
>  
> -- 
> 2.28.0.rc0.12.gb6a658bd00c9
> 

Re: [PATCH 1/3] cxl/pci: Store memory capacity values

[Dan Williams]

On Thu, Jun 10, 2021 at 5:22 PM <ira.weiny@intel.com> wrote:
>
> From: Ira Weiny <ira.weiny@intel.com>
>
> The Identify Memory Device command returns information about the
> volatile and persistent memory capacities.  Store those values in the
> cxl_mem structure for later use.  While at it, reuse the calculation of
> the volatile and persistent memory byte values to calculate the ram and
> pmem ranges.
>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/cxl/mem.h |  4 ++++
>  drivers/cxl/pci.c | 36 +++++++++++++++++++++++++++++++++---
>  2 files changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
> index 13868ff7cadf..8bd0d0506b97 100644
> --- a/drivers/cxl/mem.h
> +++ b/drivers/cxl/mem.h
> @@ -75,5 +75,9 @@ struct cxl_mem {
>
>         struct range pmem_range;
>         struct range ram_range;
> +       u64 total_cap_bytes;
> +       u64 volatile_cap_bytes;
> +       u64 persistent_cap_bytes;

Hmm, why these fields?

I would expect pmem_range and ram_range can already represent these
values and range_len(pmem_range) + range_len(ram_range) == total
capacity.

Re: [PATCH 1/3] cxl/pci: Store memory capacity values

[Ben Widawsky]

On 21-06-11 10:26:34, Dan Williams wrote:
> On Thu, Jun 10, 2021 at 5:22 PM <ira.weiny@intel.com> wrote:
> >
> > From: Ira Weiny <ira.weiny@intel.com>
> >
> > The Identify Memory Device command returns information about the
> > volatile and persistent memory capacities.  Store those values in the
> > cxl_mem structure for later use.  While at it, reuse the calculation of
> > the volatile and persistent memory byte values to calculate the ram and
> > pmem ranges.
> >
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  drivers/cxl/mem.h |  4 ++++
> >  drivers/cxl/pci.c | 36 +++++++++++++++++++++++++++++++++---
> >  2 files changed, 37 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
> > index 13868ff7cadf..8bd0d0506b97 100644
> > --- a/drivers/cxl/mem.h
> > +++ b/drivers/cxl/mem.h
> > @@ -75,5 +75,9 @@ struct cxl_mem {
> >
> >         struct range pmem_range;
> >         struct range ram_range;
> > +       u64 total_cap_bytes;
> > +       u64 volatile_cap_bytes;
> > +       u64 persistent_cap_bytes;
> 
> Hmm, why these fields?
> 
> I would expect pmem_range and ram_range can already represent these
> values and range_len(pmem_range) + range_len(ram_range) == total
> capacity.

So the way Ira described it (AIUI), pmem_range will be the total partitioned
amount, while persistent_cap_bytes is min(info.persistent capacity, pmem_range)
(same for volatile). But perhaps I misunderstood.

Re: [PATCH 1/3] cxl/pci: Store memory capacity values

[Ira Weiny]

On Fri, Jun 11, 2021 at 10:50:30AM -0700, Widawsky, Ben wrote:
> On 21-06-11 10:26:34, Dan Williams wrote:
> > On Thu, Jun 10, 2021 at 5:22 PM <ira.weiny@intel.com> wrote:
> > >
> > > From: Ira Weiny <ira.weiny@intel.com>
> > >
> > > The Identify Memory Device command returns information about the
> > > volatile and persistent memory capacities.  Store those values in the
> > > cxl_mem structure for later use.  While at it, reuse the calculation of
> > > the volatile and persistent memory byte values to calculate the ram and
> > > pmem ranges.
> > >
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > ---
> > >  drivers/cxl/mem.h |  4 ++++
> > >  drivers/cxl/pci.c | 36 +++++++++++++++++++++++++++++++++---
> > >  2 files changed, 37 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
> > > index 13868ff7cadf..8bd0d0506b97 100644
> > > --- a/drivers/cxl/mem.h
> > > +++ b/drivers/cxl/mem.h
> > > @@ -75,5 +75,9 @@ struct cxl_mem {
> > >
> > >         struct range pmem_range;
> > >         struct range ram_range;
> > > +       u64 total_cap_bytes;
> > > +       u64 volatile_cap_bytes;
> > > +       u64 persistent_cap_bytes;
> > 
> > Hmm, why these fields?
> > 
> > I would expect pmem_range and ram_range can already represent these
> > values and range_len(pmem_range) + range_len(ram_range) == total
> > capacity.
> 
> So the way Ira described it (AIUI), pmem_range will be the total partitioned
> amount, while persistent_cap_bytes is min(info.persistent capacity, pmem_range)
> (same for volatile). But perhaps I misunderstood.

Yes persistent is 'persistent only'...  so s/cap/only/ is probably better name.

pmem_range.len() may == persistent_cap_bytes.  But does not need to.

Ira

Re: [PATCH 2/3] cxl/mem: Report correct ram/pmem size in sysfs

[Ira Weiny]

On Fri, Jun 11, 2021 at 10:14:31AM -0700, Dan Williams wrote:
> On Thu, Jun 10, 2021 at 5:22 PM <ira.weiny@intel.com> wrote:
> >
> > From: Ira Weiny <ira.weiny@intel.com>
> >
> > Memory devices may specify volatile only, persistent only, and
> > partitionable space which when added together result in a total capacity.
> >
> > The partitionable space is configurable between volatile and persistent
> > space.  To account for the dynamic partitionable space the correct ram
> > and pmem size information is reported in the Get Partition Info device
> > command.
> >
> > Define cxl_mem_get_partition() and call it to retrieve the correct
> > ram and pmem ranges sizes.
> >
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  drivers/cxl/pci.c | 97 +++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 90 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index 9995f97d3b28..bcc2829e4475 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -1455,6 +1455,62 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_mem *cxlm)
> >         return ret;
> >  }
> >
> > +/**
> > + * cxl_mem_get_partition_info - Set partition info
> > + * @cxlm: The device to act on
> > + * @active_volatile_cap_bytes: returned active volatile capacity; in bytes
> > + * @active_persistent_cap_bytes: returned active persistent capacity; in bytes
> > + * @next_volatile_cap_bytes: return next volatile capacity; in bytes
> > + * @next_persistent_cap_bytes: return next persistent capacity; in bytes
> > + *
> > + * Retrieve the current partition info for the device specified.  The active
> > + * values are the current capacity in bytes.  If not 0, the 'next' values are
> > + * the pending values, in bytes, which take affect on next cold reset.
> > + *
> > + * Return: 0 if no error: or the result of the mailbox command.
> > + *
> > + * See CXL @8.2.9.5.2.1 Get Partition Info
> > + */
> > +int cxl_mem_get_partition_info(struct cxl_mem *cxlm,
> > +                              u64 *active_volatile_cap_bytes,
> > +                              u64 *active_persistent_cap_bytes,
> > +                              u64 *next_volatile_cap_bytes,
> > +                              u64 *next_persistent_cap_bytes)
> 
> Similar to how cxl_mem_identify() populates data in cxl_mem, I would
> expect this routine to do the same.
> 
> > +{
> > +       struct cxl_mbox_get_partition_info {
> > +               u64 active_volatile_cap;
> > +               u64 active_persistent_cap;
> > +               u64 next_volatile_cap;
> > +               u64 next_persistent_cap;
> > +       } __packed pi;
> > +       int rc;
> > +
> > +       /* On error report 0 */
> > +       *active_volatile_cap_bytes = 0;
> > +       *active_persistent_cap_bytes = 0;
> > +       *next_volatile_cap_bytes = 0;
> > +       *next_persistent_cap_bytes = 0;
> > +
> > +       rc = cxl_mem_mbox_send_cmd(cxlm, CXL_MBOX_OP_GET_PARTITION_INFO,
> > +                                  NULL, 0, &pi, sizeof(pi));
> > +
> > +       if (rc)
> > +               return rc;
> > +
> > +       *active_volatile_cap_bytes = le64_to_cpu(pi.active_volatile_cap);
> > +       *active_persistent_cap_bytes = le64_to_cpu(pi.active_persistent_cap);
> > +       *next_volatile_cap_bytes = le64_to_cpu(pi.next_volatile_cap);
> > +       *next_persistent_cap_bytes = le64_to_cpu(pi.next_volatile_cap);
> > +
> > +       *active_volatile_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
> > +       *active_persistent_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
> > +       *next_volatile_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
> > +       *next_persistent_cap_bytes *= CXL_CAPACITY_MULTIPLIER;
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(cxl_mem_get_partition_info);
> 
> Why is this exported? Only the PCI driver cares about this.

Once we get to dynamic partitioning I would expect it to need to be exported...
But I think you are correct...  No need to do that until we need it.

Ira

Re: [PATCH 3/3] cxl/mem: Add partition information to sysfs

[Ira Weiny]

On Fri, Jun 11, 2021 at 10:26:15AM -0700, Widawsky, Ben wrote:
> On 21-06-10 17:22:24, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > Partitionable space is added to the ram and pmem size sysfs attributes
> > but this does not show the entire story.
> > 
> > Add full partition information about the device under <sysfs>/partition.
> 
> Please add these to:
> Documentation/ABI/testing/sysfs-bus-cxl

Good point...  Thanks!

> 
> It might be time to break that file up... maybe a syfs-bus-cxl-memdev?
> 
> I'll also pose the original question here, what's the justification for exposing
> this via sysfs versus having userspace executing the mailbox command itself? Are
> you planning to have these attributes be writable at some point to do the
> partition programming?

Well...  if we have ram/size and pmem/size this is nice to have to know how it
is configured...

For me I like not having to have to execute mailbox commands for everything but
I understand having sysfs 'explosion'...  not really sure were to draw the
line.  For now this helps me to see what the emulators are returning.

Ira

> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  drivers/cxl/pci.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> > 
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index bcc2829e4475..7e4e9605e4ed 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -1226,6 +1226,42 @@ static ssize_t pmem_size_show(struct device *dev, struct device_attribute *attr,
> >  static struct device_attribute dev_attr_pmem_size =
> >  	__ATTR(size, 0444, pmem_size_show, NULL);
> >  
> > +static ssize_t part_vo_show(struct device *dev, struct device_attribute *attr,
> > +			     char *buf)
> > +{
> > +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> > +	struct cxl_mem *cxlm = cxlmd->cxlm;
> > +
> > +	return sysfs_emit(buf, "%#llx\n", cxlm->volatile_cap_bytes);
> > +}
> > +
> > +static struct device_attribute dev_attr_part_vo =
> > +	__ATTR(volatile_only, 0444, part_vo_show, NULL);
> > +
> > +static ssize_t part_po_show(struct device *dev, struct device_attribute *attr,
> > +			     char *buf)
> > +{
> > +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> > +	struct cxl_mem *cxlm = cxlmd->cxlm;
> > +
> > +	return sysfs_emit(buf, "%#llx\n", cxlm->persistent_cap_bytes);
> > +}
> > +
> > +static struct device_attribute dev_attr_part_po =
> > +	__ATTR(persistent_only, 0444, part_po_show, NULL);
> > +
> > +static ssize_t part_total_show(struct device *dev, struct device_attribute *attr,
> > +			     char *buf)
> > +{
> > +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> > +	struct cxl_mem *cxlm = cxlmd->cxlm;
> > +
> > +	return sysfs_emit(buf, "%#llx\n", cxlm->total_cap_bytes);
> > +}
> > +
> > +static struct device_attribute dev_attr_part_total =
> > +	__ATTR(total, 0444, part_total_show, NULL);
> > +
> >  static struct attribute *cxl_memdev_attributes[] = {
> >  	&dev_attr_firmware_version.attr,
> >  	&dev_attr_payload_max.attr,
> > @@ -1243,6 +1279,13 @@ static struct attribute *cxl_memdev_ram_attributes[] = {
> >  	NULL,
> >  };
> >  
> > +static struct attribute *cxl_memdev_part_attributes[] = {
> > +	&dev_attr_part_vo.attr,
> > +	&dev_attr_part_po.attr,
> > +	&dev_attr_part_total.attr,
> > +	NULL,
> > +};
> > +
> >  static struct attribute_group cxl_memdev_attribute_group = {
> >  	.attrs = cxl_memdev_attributes,
> >  };
> > @@ -1257,10 +1300,16 @@ static struct attribute_group cxl_memdev_pmem_attribute_group = {
> >  	.attrs = cxl_memdev_pmem_attributes,
> >  };
> >  
> > +static struct attribute_group cxl_memdev_part_attribute_group = {
> > +	.name = "partition",
> > +	.attrs = cxl_memdev_part_attributes,
> > +};
> > +
> >  static const struct attribute_group *cxl_memdev_attribute_groups[] = {
> >  	&cxl_memdev_attribute_group,
> >  	&cxl_memdev_ram_attribute_group,
> >  	&cxl_memdev_pmem_attribute_group,
> > +	&cxl_memdev_part_attribute_group,
> >  	NULL,
> >  };
> >  
> > -- 
> > 2.28.0.rc0.12.gb6a658bd00c9
> >