[PATCH V2 0/3] Query and use Partition Info

[PATCH V2 0/3] Query and use Partition Info

[ira.weiny]

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

Three small patches which query and use Partition Info.  Specifically this
correctly accounts for partitionable space should a device support it.

Changes for V2:
	Feed back from Ben, Dan, and Jonathan
	Remove sysfs patch in favor of using ndctl to query values in the
	future.
	Add patch to create the correct ram/pmem range entries per the spec.

Ira Weiny (3):
  cxl/pci: Store memory capacity values
  cxl/mem: Account for partitionable space in ram/pmem ranges
  cxl/mem: Adjust ram/pmem range to represent DPA ranges

 drivers/cxl/mem.h |   9 ++++
 drivers/cxl/pci.c | 134 ++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 134 insertions(+), 9 deletions(-)

-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH V2 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 only and persistent only memory capacities.  Store those values
in the cxl_mem structure for later use.  While at it, reuse those
calculations to calculate the ram and pmem ranges.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes for V2:
	Ben
		Change names to be more clear
			total_bytes
			volatile_only_bytes
			persistent_only_bytes
---
 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..9dc34418db36 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_bytes;
+	u64 volatile_only_bytes;
+	u64 persistent_only_bytes;
+	u64 partition_align_bytes;
 };
 #endif /* __CXL_MEM_H__ */
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 5a1705b52278..94b7ee08ef67 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_bytes = le64_to_cpu(id.total_capacity);
+	cxlm->total_bytes *= CXL_CAPACITY_MULTIPLIER;
+
+	cxlm->volatile_only_bytes = le64_to_cpu(id.volatile_capacity);
+	cxlm->volatile_only_bytes *= CXL_CAPACITY_MULTIPLIER;
+
+	cxlm->persistent_only_bytes = le64_to_cpu(id.persistent_capacity);
+	cxlm->persistent_only_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_bytes = %#llx\n"
+		"     volatile_only_bytes = %#llx\n"
+		"     persistent_only_bytes = %#llx\n"
+		"     partition_align_bytes = %#llx\n",
+			cxlm->total_bytes,
+			cxlm->volatile_only_bytes,
+			cxlm->persistent_only_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_only_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_only_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 V2 1/3] cxl/pci: Store memory capacity values

[Jonathan Cameron]

On Thu, 17 Jun 2021 15:16:18 -0700
<ira.weiny@intel.com> wrote:

> From: Ira Weiny <ira.weiny@intel.com>
> 
> The Identify Memory Device command returns information about the
> volatile only and persistent only memory capacities.  Store those values
> in the cxl_mem structure for later use.  While at it, reuse those
> calculations to calculate the ram and pmem ranges.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
New naming is indeed clearer.
FWIW
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> Changes for V2:
> 	Ben
> 		Change names to be more clear
> 			total_bytes
> 			volatile_only_bytes
> 			persistent_only_bytes
> ---
>  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..9dc34418db36 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_bytes;
> +	u64 volatile_only_bytes;
> +	u64 persistent_only_bytes;
> +	u64 partition_align_bytes;
>  };
>  #endif /* __CXL_MEM_H__ */
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 5a1705b52278..94b7ee08ef67 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_bytes = le64_to_cpu(id.total_capacity);
> +	cxlm->total_bytes *= CXL_CAPACITY_MULTIPLIER;
> +
> +	cxlm->volatile_only_bytes = le64_to_cpu(id.volatile_capacity);
> +	cxlm->volatile_only_bytes *= CXL_CAPACITY_MULTIPLIER;
> +
> +	cxlm->persistent_only_bytes = le64_to_cpu(id.persistent_capacity);
> +	cxlm->persistent_only_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_bytes = %#llx\n"
> +		"     volatile_only_bytes = %#llx\n"
> +		"     persistent_only_bytes = %#llx\n"
> +		"     partition_align_bytes = %#llx\n",
> +			cxlm->total_bytes,
> +			cxlm->volatile_only_bytes,
> +			cxlm->persistent_only_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_only_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_only_bytes - 1;
>  
>  	cxlm->lsa_size = le32_to_cpu(id.lsa_size);
>  	memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision));

[PATCH V2 2/3] cxl/mem: Account for partitionable space in ram/pmem ranges

[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.

If Identify Memory Device.Partition Alignment != 0 the device supports
partitionable space.  This partitionable space can be split between
volatile and persistent space.  The total volatile and persistent sizes
are reported in Get Partition Info.  ie

	active volatile memory = volatile only + partitionable volatile
	active persistent memory = persistent only + partitionable persistent

Define cxl_mem_get_partition(), check for partitionable support, and use
cxl_mem_get_partition() if applicable.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes for V2:
	Jonathan
		Account for Get Partition Info being optional
		Fix documentation
	Dan
		Store the active capacities in struct cxl_mem
		Make cxl_mem_get_partition() static until there is a
		need to export it.
---
 drivers/cxl/mem.h |  5 +++
 drivers/cxl/pci.c | 98 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 98 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index 9dc34418db36..8d7bd966a298 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -79,5 +79,10 @@ struct cxl_mem {
 	u64 volatile_only_bytes;
 	u64 persistent_only_bytes;
 	u64 partition_align_bytes;
+
+	u64 active_volatile_bytes;
+	u64 active_persistent_bytes;
+	u64 next_volatile_bytes;
+	u64 next_persistent_bytes;
 };
 #endif /* __CXL_MEM_H__ */
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 94b7ee08ef67..341885345b53 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1455,6 +1455,55 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_mem *cxlm)
 	return ret;
 }
 
+/**
+ * cxl_mem_get_partition_info - Get partition info
+ * @cxlm: The device to act on
+ * @active_volatile_bytes: returned active volatile capacity; in bytes
+ * @active_persistent_bytes: returned active persistent capacity; in bytes
+ * @next_volatile_bytes: return next volatile capacity; in bytes
+ * @next_persistent_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
+ */
+static int cxl_mem_get_partition_info(struct cxl_mem *cxlm,
+				      u64 *active_volatile_bytes,
+				      u64 *active_persistent_bytes,
+				      u64 *next_volatile_bytes,
+				      u64 *next_persistent_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;
+
+	rc = cxl_mem_mbox_send_cmd(cxlm, CXL_MBOX_OP_GET_PARTITION_INFO,
+				   NULL, 0, &pi, sizeof(pi));
+
+	if (rc)
+		return rc;
+
+	*active_volatile_bytes = le64_to_cpu(pi.active_volatile_cap);
+	*active_persistent_bytes = le64_to_cpu(pi.active_persistent_cap);
+	*next_volatile_bytes = le64_to_cpu(pi.next_volatile_cap);
+	*next_persistent_bytes = le64_to_cpu(pi.next_volatile_cap);
+
+	*active_volatile_bytes *= CXL_CAPACITY_MULTIPLIER;
+	*active_persistent_bytes *= CXL_CAPACITY_MULTIPLIER;
+	*next_volatile_bytes *= CXL_CAPACITY_MULTIPLIER;
+	*next_persistent_bytes *= CXL_CAPACITY_MULTIPLIER;
+
+	return 0;
+}
+
 /**
  * cxl_mem_enumerate_cmds() - Enumerate commands for a device.
  * @cxlm: The device.
@@ -1573,18 +1622,53 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
 			cxlm->persistent_only_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 int cxl_mem_create_range_info(struct cxl_mem *cxlm)
+{
+	int rc;
+
+	if (cxlm->partition_align_bytes == 0) {
+		cxlm->ram_range.start = 0;
+		cxlm->ram_range.end = cxlm->volatile_only_bytes - 1;
+		cxlm->pmem_range.start = 0;
+		cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
+		return 0;
+	}
+
+	rc = cxl_mem_get_partition_info(cxlm,
+					&cxlm->active_volatile_bytes,
+					&cxlm->active_persistent_bytes,
+					&cxlm->next_volatile_bytes,
+					&cxlm->next_persistent_bytes);
+	if (rc < 0) {
+		dev_err(&cxlm->pdev->dev, "Failed to query partition information\n");
+		return rc;
+	}
+
+	dev_dbg(&cxlm->pdev->dev, "Get Partition Info\n"
+		"     active_volatile_bytes = %#llx\n"
+		"     active_persistent_bytes = %#llx\n"
+		"     next_volatile_bytes = %#llx\n"
+		"     next_persistent_bytes = %#llx\n",
+			cxlm->active_volatile_bytes,
+			cxlm->active_persistent_bytes,
+			cxlm->next_volatile_bytes,
+			cxlm->next_persistent_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_only_bytes - 1;
+	cxlm->ram_range.end = cxlm->active_volatile_bytes - 1;
 
 	cxlm->pmem_range.start = 0;
-	cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
-
-	cxlm->lsa_size = le32_to_cpu(id.lsa_size);
-	memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision));
+	cxlm->pmem_range.end = cxlm->active_persistent_bytes - 1;
 
 	return 0;
 }
@@ -1618,6 +1702,10 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
+	rc = cxl_mem_create_range_info(cxlm);
+	if (rc)
+		return rc;
+
 	return cxl_mem_add_memdev(cxlm);
 }
 
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH V2 2/3] cxl/mem: Account for partitionable space in ram/pmem ranges

[Jonathan Cameron]

On Thu, 17 Jun 2021 15:16:19 -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.
> 
> If Identify Memory Device.Partition Alignment != 0 the device supports
> partitionable space.  This partitionable space can be split between
> volatile and persistent space.  The total volatile and persistent sizes
> are reported in Get Partition Info.  ie
> 
> 	active volatile memory = volatile only + partitionable volatile
> 	active persistent memory = persistent only + partitionable persistent
> 
> Define cxl_mem_get_partition(), check for partitionable support, and use
> cxl_mem_get_partition() if applicable.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Trivial editorial stuff inline.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> ---
> Changes for V2:
> 	Jonathan
> 		Account for Get Partition Info being optional
> 		Fix documentation
> 	Dan
> 		Store the active capacities in struct cxl_mem
> 		Make cxl_mem_get_partition() static until there is a
> 		need to export it.
> ---
>  drivers/cxl/mem.h |  5 +++
>  drivers/cxl/pci.c | 98 ++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 98 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
> index 9dc34418db36..8d7bd966a298 100644
> --- a/drivers/cxl/mem.h
> +++ b/drivers/cxl/mem.h
> @@ -79,5 +79,10 @@ struct cxl_mem {
>  	u64 volatile_only_bytes;
>  	u64 persistent_only_bytes;
>  	u64 partition_align_bytes;
> +
> +	u64 active_volatile_bytes;
> +	u64 active_persistent_bytes;
> +	u64 next_volatile_bytes;
> +	u64 next_persistent_bytes;
>  };
>  #endif /* __CXL_MEM_H__ */
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 94b7ee08ef67..341885345b53 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1455,6 +1455,55 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_mem *cxlm)
>  	return ret;
>  }
>  
> +/**
> + * cxl_mem_get_partition_info - Get partition info
> + * @cxlm: The device to act on
> + * @active_volatile_bytes: returned active volatile capacity; in bytes

Perhaps over documented?  Would be very very odd if _bytes attribute wasn't
in bytes so maybe don't need to state it in the comment?

> + * @active_persistent_bytes: returned active persistent capacity; in bytes
> + * @next_volatile_bytes: return next volatile capacity; in bytes
> + * @next_persistent_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

Definitely don't need to say they are in bytes again.

> + * 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
> + */
> +static int cxl_mem_get_partition_info(struct cxl_mem *cxlm,
> +				      u64 *active_volatile_bytes,
> +				      u64 *active_persistent_bytes,
> +				      u64 *next_volatile_bytes,
> +				      u64 *next_persistent_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;
> +
> +	rc = cxl_mem_mbox_send_cmd(cxlm, CXL_MBOX_OP_GET_PARTITION_INFO,
> +				   NULL, 0, &pi, sizeof(pi));
> +

In my view, slightly better to not have a blank line between statement
and error handling.

> +	if (rc)
> +		return rc;
> +
> +	*active_volatile_bytes = le64_to_cpu(pi.active_volatile_cap);
> +	*active_persistent_bytes = le64_to_cpu(pi.active_persistent_cap);
> +	*next_volatile_bytes = le64_to_cpu(pi.next_volatile_cap);
> +	*next_persistent_bytes = le64_to_cpu(pi.next_volatile_cap);
> +
> +	*active_volatile_bytes *= CXL_CAPACITY_MULTIPLIER;
> +	*active_persistent_bytes *= CXL_CAPACITY_MULTIPLIER;
> +	*next_volatile_bytes *= CXL_CAPACITY_MULTIPLIER;
> +	*next_persistent_bytes *= CXL_CAPACITY_MULTIPLIER;
> +
> +	return 0;
> +}
> +
>  /**
>   * cxl_mem_enumerate_cmds() - Enumerate commands for a device.
>   * @cxlm: The device.
> @@ -1573,18 +1622,53 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
>  			cxlm->persistent_only_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 int cxl_mem_create_range_info(struct cxl_mem *cxlm)
> +{
> +	int rc;
> +
> +	if (cxlm->partition_align_bytes == 0) {
> +		cxlm->ram_range.start = 0;
> +		cxlm->ram_range.end = cxlm->volatile_only_bytes - 1;
> +		cxlm->pmem_range.start = 0;
> +		cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
> +		return 0;
> +	}
> +
> +	rc = cxl_mem_get_partition_info(cxlm,
> +					&cxlm->active_volatile_bytes,
> +					&cxlm->active_persistent_bytes,
> +					&cxlm->next_volatile_bytes,
> +					&cxlm->next_persistent_bytes);
> +	if (rc < 0) {
> +		dev_err(&cxlm->pdev->dev, "Failed to query partition information\n");
> +		return rc;
> +	}
> +
> +	dev_dbg(&cxlm->pdev->dev, "Get Partition Info\n"
> +		"     active_volatile_bytes = %#llx\n"
> +		"     active_persistent_bytes = %#llx\n"
> +		"     next_volatile_bytes = %#llx\n"
> +		"     next_persistent_bytes = %#llx\n",
> +			cxlm->active_volatile_bytes,
> +			cxlm->active_persistent_bytes,
> +			cxlm->next_volatile_bytes,
> +			cxlm->next_persistent_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_only_bytes - 1;
> +	cxlm->ram_range.end = cxlm->active_volatile_bytes - 1;
>  
>  	cxlm->pmem_range.start = 0;
> -	cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
> -
> -	cxlm->lsa_size = le32_to_cpu(id.lsa_size);
> -	memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision));
> +	cxlm->pmem_range.end = cxlm->active_persistent_bytes - 1;
>  
>  	return 0;
>  }
> @@ -1618,6 +1702,10 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> +	rc = cxl_mem_create_range_info(cxlm);
> +	if (rc)
> +		return rc;
> +
>  	return cxl_mem_add_memdev(cxlm);
>  }
>  

Re: [PATCH V2 2/3] cxl/mem: Account for partitionable space in ram/pmem ranges

[Ira Weiny]

On Fri, Jun 18, 2021 at 02:59:02PM +0100, Jonathan Cameron wrote:
> On Thu, 17 Jun 2021 15:16:19 -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.
> > 
> > If Identify Memory Device.Partition Alignment != 0 the device supports
> > partitionable space.  This partitionable space can be split between
> > volatile and persistent space.  The total volatile and persistent sizes
> > are reported in Get Partition Info.  ie
> > 
> > 	active volatile memory = volatile only + partitionable volatile
> > 	active persistent memory = persistent only + partitionable persistent
> > 
> > Define cxl_mem_get_partition(), check for partitionable support, and use
> > cxl_mem_get_partition() if applicable.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> Trivial editorial stuff inline.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks!

> 
> > 
> > ---
> > Changes for V2:
> > 	Jonathan
> > 		Account for Get Partition Info being optional
> > 		Fix documentation
> > 	Dan
> > 		Store the active capacities in struct cxl_mem
> > 		Make cxl_mem_get_partition() static until there is a
> > 		need to export it.
> > ---
> >  drivers/cxl/mem.h |  5 +++
> >  drivers/cxl/pci.c | 98 ++++++++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 98 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
> > index 9dc34418db36..8d7bd966a298 100644
> > --- a/drivers/cxl/mem.h
> > +++ b/drivers/cxl/mem.h
> > @@ -79,5 +79,10 @@ struct cxl_mem {
> >  	u64 volatile_only_bytes;
> >  	u64 persistent_only_bytes;
> >  	u64 partition_align_bytes;
> > +
> > +	u64 active_volatile_bytes;
> > +	u64 active_persistent_bytes;
> > +	u64 next_volatile_bytes;
> > +	u64 next_persistent_bytes;
> >  };
> >  #endif /* __CXL_MEM_H__ */
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index 94b7ee08ef67..341885345b53 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -1455,6 +1455,55 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_mem *cxlm)
> >  	return ret;
> >  }
> >  
> > +/**
> > + * cxl_mem_get_partition_info - Get partition info
> > + * @cxlm: The device to act on
> > + * @active_volatile_bytes: returned active volatile capacity; in bytes
> 
> Perhaps over documented?  Would be very very odd if _bytes attribute wasn't
> in bytes so maybe don't need to state it in the comment?

Ok...  I keep saying this to myself because all the table values are in 256MB
increments so I just kept typing it!!!  :-D

I'll remove the redundancies...

> 
> > + * @active_persistent_bytes: returned active persistent capacity; in bytes
> > + * @next_volatile_bytes: return next volatile capacity; in bytes
> > + * @next_persistent_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
> 
> Definitely don't need to say they are in bytes again.

Sure...

> 
> > + * 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
> > + */
> > +static int cxl_mem_get_partition_info(struct cxl_mem *cxlm,
> > +				      u64 *active_volatile_bytes,
> > +				      u64 *active_persistent_bytes,
> > +				      u64 *next_volatile_bytes,
> > +				      u64 *next_persistent_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;
> > +
> > +	rc = cxl_mem_mbox_send_cmd(cxlm, CXL_MBOX_OP_GET_PARTITION_INFO,
> > +				   NULL, 0, &pi, sizeof(pi));
> > +
> 
> In my view, slightly better to not have a blank line between statement
> and error handling.

Fair enough.  I go back and forth depending on how the code looks.

All changed, v3 comming,
Ira

> 
> > +	if (rc)
> > +		return rc;
> > +
> > +	*active_volatile_bytes = le64_to_cpu(pi.active_volatile_cap);
> > +	*active_persistent_bytes = le64_to_cpu(pi.active_persistent_cap);
> > +	*next_volatile_bytes = le64_to_cpu(pi.next_volatile_cap);
> > +	*next_persistent_bytes = le64_to_cpu(pi.next_volatile_cap);
> > +
> > +	*active_volatile_bytes *= CXL_CAPACITY_MULTIPLIER;
> > +	*active_persistent_bytes *= CXL_CAPACITY_MULTIPLIER;
> > +	*next_volatile_bytes *= CXL_CAPACITY_MULTIPLIER;
> > +	*next_persistent_bytes *= CXL_CAPACITY_MULTIPLIER;
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * cxl_mem_enumerate_cmds() - Enumerate commands for a device.
> >   * @cxlm: The device.
> > @@ -1573,18 +1622,53 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
> >  			cxlm->persistent_only_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 int cxl_mem_create_range_info(struct cxl_mem *cxlm)
> > +{
> > +	int rc;
> > +
> > +	if (cxlm->partition_align_bytes == 0) {
> > +		cxlm->ram_range.start = 0;
> > +		cxlm->ram_range.end = cxlm->volatile_only_bytes - 1;
> > +		cxlm->pmem_range.start = 0;
> > +		cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
> > +		return 0;
> > +	}
> > +
> > +	rc = cxl_mem_get_partition_info(cxlm,
> > +					&cxlm->active_volatile_bytes,
> > +					&cxlm->active_persistent_bytes,
> > +					&cxlm->next_volatile_bytes,
> > +					&cxlm->next_persistent_bytes);
> > +	if (rc < 0) {
> > +		dev_err(&cxlm->pdev->dev, "Failed to query partition information\n");
> > +		return rc;
> > +	}
> > +
> > +	dev_dbg(&cxlm->pdev->dev, "Get Partition Info\n"
> > +		"     active_volatile_bytes = %#llx\n"
> > +		"     active_persistent_bytes = %#llx\n"
> > +		"     next_volatile_bytes = %#llx\n"
> > +		"     next_persistent_bytes = %#llx\n",
> > +			cxlm->active_volatile_bytes,
> > +			cxlm->active_persistent_bytes,
> > +			cxlm->next_volatile_bytes,
> > +			cxlm->next_persistent_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_only_bytes - 1;
> > +	cxlm->ram_range.end = cxlm->active_volatile_bytes - 1;
> >  
> >  	cxlm->pmem_range.start = 0;
> > -	cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
> > -
> > -	cxlm->lsa_size = le32_to_cpu(id.lsa_size);
> > -	memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision));
> > +	cxlm->pmem_range.end = cxlm->active_persistent_bytes - 1;
> >  
> >  	return 0;
> >  }
> > @@ -1618,6 +1702,10 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  	if (rc)
> >  		return rc;
> >  
> > +	rc = cxl_mem_create_range_info(cxlm);
> > +	if (rc)
> > +		return rc;
> > +
> >  	return cxl_mem_add_memdev(cxlm);
> >  }
> >  
> 

[PATCH V3] cxl/mem: Account for partitionable space in ram/pmem ranges

[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.

If Identify Memory Device.Partition Alignment != 0 the device supports
partitionable space.  This partitionable space can be split between
volatile and persistent space.  The total volatile and persistent sizes
are reported in Get Partition Info.  ie

	active volatile memory = volatile only + partitionable volatile
	active persistent memory = persistent only + partitionable persistent

Define cxl_mem_get_partition(), check for partitionable support, and use
cxl_mem_get_partition() if applicable.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes for V3:
	Johnathan
		Stop over documenting 'in bytes'...  :-D

Changes for V2:
	Jonathan
		Account for Get Partition Info being optional
		Fix documentation
	Dan
		Store the active capacities in struct cxl_mem
		Make cxl_mem_get_partition() static until there is a
		need to export it.
---
 drivers/cxl/mem.h |  5 +++
 drivers/cxl/pci.c | 96 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 96 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index 9dc34418db36..8d7bd966a298 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -79,5 +79,10 @@ struct cxl_mem {
 	u64 volatile_only_bytes;
 	u64 persistent_only_bytes;
 	u64 partition_align_bytes;
+
+	u64 active_volatile_bytes;
+	u64 active_persistent_bytes;
+	u64 next_volatile_bytes;
+	u64 next_persistent_bytes;
 };
 #endif /* __CXL_MEM_H__ */
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 94b7ee08ef67..3c8b7aa8c273 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1455,6 +1455,53 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_mem *cxlm)
 	return ret;
 }
 
+/**
+ * cxl_mem_get_partition_info - Get partition info
+ * @cxlm: The device to act on
+ * @active_volatile_bytes: returned active volatile capacity
+ * @active_persistent_bytes: returned active persistent capacity
+ * @next_volatile_bytes: return next volatile capacity
+ * @next_persistent_bytes: return next persistent capacity
+ *
+ * Retrieve the current partition info for the device specified.  If not 0, the
+ * 'next' values are pending and 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
+ */
+static int cxl_mem_get_partition_info(struct cxl_mem *cxlm,
+				      u64 *active_volatile_bytes,
+				      u64 *active_persistent_bytes,
+				      u64 *next_volatile_bytes,
+				      u64 *next_persistent_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;
+
+	rc = cxl_mem_mbox_send_cmd(cxlm, CXL_MBOX_OP_GET_PARTITION_INFO,
+				   NULL, 0, &pi, sizeof(pi));
+	if (rc)
+		return rc;
+
+	*active_volatile_bytes = le64_to_cpu(pi.active_volatile_cap);
+	*active_persistent_bytes = le64_to_cpu(pi.active_persistent_cap);
+	*next_volatile_bytes = le64_to_cpu(pi.next_volatile_cap);
+	*next_persistent_bytes = le64_to_cpu(pi.next_volatile_cap);
+
+	*active_volatile_bytes *= CXL_CAPACITY_MULTIPLIER;
+	*active_persistent_bytes *= CXL_CAPACITY_MULTIPLIER;
+	*next_volatile_bytes *= CXL_CAPACITY_MULTIPLIER;
+	*next_persistent_bytes *= CXL_CAPACITY_MULTIPLIER;
+
+	return 0;
+}
+
 /**
  * cxl_mem_enumerate_cmds() - Enumerate commands for a device.
  * @cxlm: The device.
@@ -1573,18 +1620,53 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
 			cxlm->persistent_only_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 int cxl_mem_create_range_info(struct cxl_mem *cxlm)
+{
+	int rc;
+
+	if (cxlm->partition_align_bytes == 0) {
+		cxlm->ram_range.start = 0;
+		cxlm->ram_range.end = cxlm->volatile_only_bytes - 1;
+		cxlm->pmem_range.start = 0;
+		cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
+		return 0;
+	}
+
+	rc = cxl_mem_get_partition_info(cxlm,
+					&cxlm->active_volatile_bytes,
+					&cxlm->active_persistent_bytes,
+					&cxlm->next_volatile_bytes,
+					&cxlm->next_persistent_bytes);
+	if (rc < 0) {
+		dev_err(&cxlm->pdev->dev, "Failed to query partition information\n");
+		return rc;
+	}
+
+	dev_dbg(&cxlm->pdev->dev, "Get Partition Info\n"
+		"     active_volatile_bytes = %#llx\n"
+		"     active_persistent_bytes = %#llx\n"
+		"     next_volatile_bytes = %#llx\n"
+		"     next_persistent_bytes = %#llx\n",
+			cxlm->active_volatile_bytes,
+			cxlm->active_persistent_bytes,
+			cxlm->next_volatile_bytes,
+			cxlm->next_persistent_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_only_bytes - 1;
+	cxlm->ram_range.end = cxlm->active_volatile_bytes - 1;
 
 	cxlm->pmem_range.start = 0;
-	cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
-
-	cxlm->lsa_size = le32_to_cpu(id.lsa_size);
-	memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision));
+	cxlm->pmem_range.end = cxlm->active_persistent_bytes - 1;
 
 	return 0;
 }
@@ -1618,6 +1700,10 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
+	rc = cxl_mem_create_range_info(cxlm);
+	if (rc)
+		return rc;
+
 	return cxl_mem_add_memdev(cxlm);
 }
 
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH V3] cxl/mem: Account for partitionable space in ram/pmem ranges

[Ben Widawsky]

On 21-06-18 09:31:29, 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.
> 
> If Identify Memory Device.Partition Alignment != 0 the device supports
> partitionable space.  This partitionable space can be split between
> volatile and persistent space.  The total volatile and persistent sizes
> are reported in Get Partition Info.  ie
> 
> 	active volatile memory = volatile only + partitionable volatile
> 	active persistent memory = persistent only + partitionable persistent
> 
> Define cxl_mem_get_partition(), check for partitionable support, and use
> cxl_mem_get_partition() if applicable.

This doesn't look right to me, but I'll happily stand corrected.

> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> Changes for V3:
> 	Johnathan
> 		Stop over documenting 'in bytes'...  :-D
> 
> Changes for V2:
> 	Jonathan
> 		Account for Get Partition Info being optional
> 		Fix documentation
> 	Dan
> 		Store the active capacities in struct cxl_mem
> 		Make cxl_mem_get_partition() static until there is a
> 		need to export it.
> ---
>  drivers/cxl/mem.h |  5 +++
>  drivers/cxl/pci.c | 96 ++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 96 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
> index 9dc34418db36..8d7bd966a298 100644
> --- a/drivers/cxl/mem.h
> +++ b/drivers/cxl/mem.h
> @@ -79,5 +79,10 @@ struct cxl_mem {
>  	u64 volatile_only_bytes;
>  	u64 persistent_only_bytes;
>  	u64 partition_align_bytes;
> +
> +	u64 active_volatile_bytes;
> +	u64 active_persistent_bytes;
> +	u64 next_volatile_bytes;
> +	u64 next_persistent_bytes;
>  };
>  #endif /* __CXL_MEM_H__ */
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 94b7ee08ef67..3c8b7aa8c273 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1455,6 +1455,53 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_mem *cxlm)
>  	return ret;
>  }
>  
> +/**
> + * cxl_mem_get_partition_info - Get partition info
> + * @cxlm: The device to act on
> + * @active_volatile_bytes: returned active volatile capacity
> + * @active_persistent_bytes: returned active persistent capacity
> + * @next_volatile_bytes: return next volatile capacity
> + * @next_persistent_bytes: return next persistent capacity
> + *
> + * Retrieve the current partition info for the device specified.  If not 0, the
> + * 'next' values are pending and 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
> + */
> +static int cxl_mem_get_partition_info(struct cxl_mem *cxlm,
> +				      u64 *active_volatile_bytes,
> +				      u64 *active_persistent_bytes,
> +				      u64 *next_volatile_bytes,
> +				      u64 *next_persistent_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;
> +
> +	rc = cxl_mem_mbox_send_cmd(cxlm, CXL_MBOX_OP_GET_PARTITION_INFO,
> +				   NULL, 0, &pi, sizeof(pi));
> +	if (rc)
> +		return rc;
> +
> +	*active_volatile_bytes = le64_to_cpu(pi.active_volatile_cap);
> +	*active_persistent_bytes = le64_to_cpu(pi.active_persistent_cap);
> +	*next_volatile_bytes = le64_to_cpu(pi.next_volatile_cap);
> +	*next_persistent_bytes = le64_to_cpu(pi.next_volatile_cap);
> +
> +	*active_volatile_bytes *= CXL_CAPACITY_MULTIPLIER;
> +	*active_persistent_bytes *= CXL_CAPACITY_MULTIPLIER;
> +	*next_volatile_bytes *= CXL_CAPACITY_MULTIPLIER;
> +	*next_persistent_bytes *= CXL_CAPACITY_MULTIPLIER;
> +
> +	return 0;
> +}
> +
>  /**
>   * cxl_mem_enumerate_cmds() - Enumerate commands for a device.
>   * @cxlm: The device.
> @@ -1573,18 +1620,53 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
>  			cxlm->persistent_only_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 int cxl_mem_create_range_info(struct cxl_mem *cxlm)
> +{
> +	int rc;
> +
> +	if (cxlm->partition_align_bytes == 0) {
> +		cxlm->ram_range.start = 0;
> +		cxlm->ram_range.end = cxlm->volatile_only_bytes - 1;
> +		cxlm->pmem_range.start = 0;
> +		cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
> +		return 0;
> +	}
> +
> +	rc = cxl_mem_get_partition_info(cxlm,
> +					&cxlm->active_volatile_bytes,
> +					&cxlm->active_persistent_bytes,
> +					&cxlm->next_volatile_bytes,
> +					&cxlm->next_persistent_bytes);
> +	if (rc < 0) {
> +		dev_err(&cxlm->pdev->dev, "Failed to query partition information\n");
> +		return rc;
> +	}

If the command is not supported, you're going to get back -ENXIO here and fail.
I think what you want to do also is use IDENTIFY if PARTITION_INFO doesn't
exist.

> +
> +	dev_dbg(&cxlm->pdev->dev, "Get Partition Info\n"
> +		"     active_volatile_bytes = %#llx\n"
> +		"     active_persistent_bytes = %#llx\n"
> +		"     next_volatile_bytes = %#llx\n"
> +		"     next_persistent_bytes = %#llx\n",
> +			cxlm->active_volatile_bytes,
> +			cxlm->active_persistent_bytes,
> +			cxlm->next_volatile_bytes,
> +			cxlm->next_persistent_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_only_bytes - 1;
> +	cxlm->ram_range.end = cxlm->active_volatile_bytes - 1;
>  
>  	cxlm->pmem_range.start = 0;
> -	cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
> -
> -	cxlm->lsa_size = le32_to_cpu(id.lsa_size);
> -	memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision));
> +	cxlm->pmem_range.end = cxlm->active_persistent_bytes - 1;
>  
>  	return 0;
>  }
> @@ -1618,6 +1700,10 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> +	rc = cxl_mem_create_range_info(cxlm);
> +	if (rc)
> +		return rc;
> +

Assuming my assessment is correct, I think it's time to make
cxl_mem_mbox_send_cmd grow a little bit better error handling. I'd recommend
something like this and then checking for -ENOTSUPP.

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index b1a5a18dba92..3b7d8a905393 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -92,6 +92,7 @@ struct mbox_cmd {
        size_t size_out;
        u16 return_code;
 #define CXL_MBOX_SUCCESS 0
+#define CXL_MBOX_UNSUPPORTED 0x15
 };

 static int cxl_mem_major;
@@ -876,9 +877,15 @@ static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm, u16 opcode,
        if (rc)
                return rc;

-       /* TODO: Map return code to proper kernel style errno */
-       if (mbox_cmd.return_code != CXL_MBOX_SUCCESS)
+       switch (mbox_cmd.return_code) {
+       case CXL_MBOX_SUCCESS:
+               break;
+       case CXL_MBOX_UNSUPPORTED:
+               return -ENOTSUPP;
+       default:
+               /* TODO: Map return code to proper kernel style errno */
                return -ENXIO;
+       }

        /*
         * Variable sized commands can't be validated and so it's up to the


>  	return cxl_mem_add_memdev(cxlm);
>  }

>  
> -- 
> 2.28.0.rc0.12.gb6a658bd00c9
> 

Re: [PATCH V3] cxl/mem: Account for partitionable space in ram/pmem ranges

[Ira Weiny]

On Fri, Jun 18, 2021 at 09:58:33AM -0700, Widawsky, Ben wrote:
> On 21-06-18 09:31:29, 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.
> > 
> > If Identify Memory Device.Partition Alignment != 0 the device supports
> > partitionable space.  This partitionable space can be split between
> > volatile and persistent space.  The total volatile and persistent sizes
> > are reported in Get Partition Info.  ie
> > 
> > 	active volatile memory = volatile only + partitionable volatile
> > 	active persistent memory = persistent only + partitionable persistent
> > 
> > Define cxl_mem_get_partition(), check for partitionable support, and use
> > cxl_mem_get_partition() if applicable.
> 
> This doesn't look right to me, but I'll happily stand corrected.
> 

[snip]

> > +
> > +static int cxl_mem_create_range_info(struct cxl_mem *cxlm)
> > +{
> > +	int rc;
> > +
> > +	if (cxlm->partition_align_bytes == 0) {
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This...

> > +		cxlm->ram_range.start = 0;
> > +		cxlm->ram_range.end = cxlm->volatile_only_bytes - 1;
> > +		cxlm->pmem_range.start = 0;
> > +		cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
> > +		return 0;
> > +	}
> > +
> > +	rc = cxl_mem_get_partition_info(cxlm,
> > +					&cxlm->active_volatile_bytes,
> > +					&cxlm->active_persistent_bytes,
> > +					&cxlm->next_volatile_bytes,
> > +					&cxlm->next_persistent_bytes);
> > +	if (rc < 0) {
> > +		dev_err(&cxlm->pdev->dev, "Failed to query partition information\n");
> > +		return rc;
> > +	}
> 
> If the command is not supported, you're going to get back -ENXIO here and fail.
> I think what you want to do also is use IDENTIFY if PARTITION_INFO doesn't
> exist.

... should handle the situation where Get Partition Info is not supported.

I did not see it explicitly stated but I believe if Partition Align Bytes is
non-0 then partitioning is supported and Get/Set Partition Info should work.

> 
> Assuming my assessment is correct, I think it's time to make
> cxl_mem_mbox_send_cmd grow a little bit better error handling. I'd recommend
> something like this and then checking for -ENOTSUPP.
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index b1a5a18dba92..3b7d8a905393 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -92,6 +92,7 @@ struct mbox_cmd {
>         size_t size_out;
>         u16 return_code;
>  #define CXL_MBOX_SUCCESS 0
> +#define CXL_MBOX_UNSUPPORTED 0x15
>  };
> 
>  static int cxl_mem_major;
> @@ -876,9 +877,15 @@ static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm, u16 opcode,
>         if (rc)
>                 return rc;
> 
> -       /* TODO: Map return code to proper kernel style errno */
> -       if (mbox_cmd.return_code != CXL_MBOX_SUCCESS)
> +       switch (mbox_cmd.return_code) {
> +       case CXL_MBOX_SUCCESS:
> +               break;
> +       case CXL_MBOX_UNSUPPORTED:
> +               return -ENOTSUPP;
> +       default:
> +               /* TODO: Map return code to proper kernel style errno */
>                 return -ENXIO;
> +       }


I'm not opposed to adding this though.

Ira

Re: [PATCH V3] cxl/mem: Account for partitionable space in ram/pmem ranges

[Ben Widawsky]

On 21-06-18 11:48:32, Ira Weiny wrote:
> On Fri, Jun 18, 2021 at 09:58:33AM -0700, Widawsky, Ben wrote:
> > On 21-06-18 09:31:29, 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.
> > > 
> > > If Identify Memory Device.Partition Alignment != 0 the device supports
> > > partitionable space.  This partitionable space can be split between
> > > volatile and persistent space.  The total volatile and persistent sizes
> > > are reported in Get Partition Info.  ie
> > > 
> > > 	active volatile memory = volatile only + partitionable volatile
> > > 	active persistent memory = persistent only + partitionable persistent
> > > 
> > > Define cxl_mem_get_partition(), check for partitionable support, and use
> > > cxl_mem_get_partition() if applicable.
> > 
> > This doesn't look right to me, but I'll happily stand corrected.
> > 
> 
> [snip]
> 
> > > +
> > > +static int cxl_mem_create_range_info(struct cxl_mem *cxlm)
> > > +{
> > > +	int rc;
> > > +
> > > +	if (cxlm->partition_align_bytes == 0) {
>             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This...
> 
> > > +		cxlm->ram_range.start = 0;
> > > +		cxlm->ram_range.end = cxlm->volatile_only_bytes - 1;
> > > +		cxlm->pmem_range.start = 0;
> > > +		cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
> > > +		return 0;
> > > +	}
> > > +
> > > +	rc = cxl_mem_get_partition_info(cxlm,
> > > +					&cxlm->active_volatile_bytes,
> > > +					&cxlm->active_persistent_bytes,
> > > +					&cxlm->next_volatile_bytes,
> > > +					&cxlm->next_persistent_bytes);
> > > +	if (rc < 0) {
> > > +		dev_err(&cxlm->pdev->dev, "Failed to query partition information\n");
> > > +		return rc;
> > > +	}
> > 
> > If the command is not supported, you're going to get back -ENXIO here and fail.
> > I think what you want to do also is use IDENTIFY if PARTITION_INFO doesn't
> > exist.
> 
> ... should handle the situation where Get Partition Info is not supported.
> 
> I did not see it explicitly stated but I believe if Partition Align Bytes is
> non-0 then partitioning is supported and Get/Set Partition Info should work.

Ah. I didn't notice that. Perhaps a comment with a spec reference might help the
next person who comes along to look at that. My reading of it is the same
(though not explicit). I wonder why they chose to put that in IDENTIFY...

> 
> > 
> > Assuming my assessment is correct, I think it's time to make
> > cxl_mem_mbox_send_cmd grow a little bit better error handling. I'd recommend
> > something like this and then checking for -ENOTSUPP.
> > 
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index b1a5a18dba92..3b7d8a905393 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -92,6 +92,7 @@ struct mbox_cmd {
> >         size_t size_out;
> >         u16 return_code;
> >  #define CXL_MBOX_SUCCESS 0
> > +#define CXL_MBOX_UNSUPPORTED 0x15
> >  };
> > 
> >  static int cxl_mem_major;
> > @@ -876,9 +877,15 @@ static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm, u16 opcode,
> >         if (rc)
> >                 return rc;
> > 
> > -       /* TODO: Map return code to proper kernel style errno */
> > -       if (mbox_cmd.return_code != CXL_MBOX_SUCCESS)
> > +       switch (mbox_cmd.return_code) {
> > +       case CXL_MBOX_SUCCESS:
> > +               break;
> > +       case CXL_MBOX_UNSUPPORTED:
> > +               return -ENOTSUPP;
> > +       default:
> > +               /* TODO: Map return code to proper kernel style errno */
> >                 return -ENXIO;
> > +       }
> 
> 
> I'm not opposed to adding this though.

You've resolved my concern, so it's up to you. The other option would be to
check the commands if the command got enabled in cxlm->enabled_cmds.

> 
> Ira

[PATCH v4 2/3] cxl/mem: Account for partitionable space in ram/pmem ranges

[Dan Williams]

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.

If Identify Memory Device.Partition Alignment != 0 the device supports
partitionable space.  This partitionable space can be split between
volatile and persistent space.  The total volatile and persistent sizes
are reported in Get Partition Info.  ie

	active volatile memory = volatile only + partitionable volatile
	active persistent memory = persistent only + partitionable persistent

Define cxl_mem_get_partition(), check for partitionable support, and use
cxl_mem_get_partition() if applicable.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v3:
- Fix "sparse: cast to restricted __le64" warnings, mark the members of
  struct cxl_mbox_get_partition_info as __le64.

 drivers/cxl/cxlmem.h |    5 +++
 drivers/cxl/pci.c    |   96 +++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 96 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 22344fda8ca5..6c0b1e2ea97c 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -99,5 +99,10 @@ struct cxl_mem {
 	u64 volatile_only_bytes;
 	u64 persistent_only_bytes;
 	u64 partition_align_bytes;
+
+	u64 active_volatile_bytes;
+	u64 active_persistent_bytes;
+	u64 next_volatile_bytes;
+	u64 next_persistent_bytes;
 };
 #endif /* __CXL_MEM_H__ */
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index cf4f593f426e..3f5db8960098 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1263,6 +1263,53 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_mem *cxlm)
 	return ret;
 }
 
+/**
+ * cxl_mem_get_partition_info - Get partition info
+ * @cxlm: The device to act on
+ * @active_volatile_bytes: returned active volatile capacity
+ * @active_persistent_bytes: returned active persistent capacity
+ * @next_volatile_bytes: return next volatile capacity
+ * @next_persistent_bytes: return next persistent capacity
+ *
+ * Retrieve the current partition info for the device specified.  If not 0, the
+ * 'next' values are pending and 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
+ */
+static int cxl_mem_get_partition_info(struct cxl_mem *cxlm,
+				      u64 *active_volatile_bytes,
+				      u64 *active_persistent_bytes,
+				      u64 *next_volatile_bytes,
+				      u64 *next_persistent_bytes)
+{
+	struct cxl_mbox_get_partition_info {
+		__le64 active_volatile_cap;
+		__le64 active_persistent_cap;
+		__le64 next_volatile_cap;
+		__le64 next_persistent_cap;
+	} __packed pi;
+	int rc;
+
+	rc = cxl_mem_mbox_send_cmd(cxlm, CXL_MBOX_OP_GET_PARTITION_INFO,
+				   NULL, 0, &pi, sizeof(pi));
+	if (rc)
+		return rc;
+
+	*active_volatile_bytes = le64_to_cpu(pi.active_volatile_cap);
+	*active_persistent_bytes = le64_to_cpu(pi.active_persistent_cap);
+	*next_volatile_bytes = le64_to_cpu(pi.next_volatile_cap);
+	*next_persistent_bytes = le64_to_cpu(pi.next_volatile_cap);
+
+	*active_volatile_bytes *= CXL_CAPACITY_MULTIPLIER;
+	*active_persistent_bytes *= CXL_CAPACITY_MULTIPLIER;
+	*next_volatile_bytes *= CXL_CAPACITY_MULTIPLIER;
+	*next_persistent_bytes *= CXL_CAPACITY_MULTIPLIER;
+
+	return 0;
+}
+
 /**
  * cxl_mem_enumerate_cmds() - Enumerate commands for a device.
  * @cxlm: The device.
@@ -1381,18 +1428,53 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
 			cxlm->persistent_only_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 int cxl_mem_create_range_info(struct cxl_mem *cxlm)
+{
+	int rc;
+
+	if (cxlm->partition_align_bytes == 0) {
+		cxlm->ram_range.start = 0;
+		cxlm->ram_range.end = cxlm->volatile_only_bytes - 1;
+		cxlm->pmem_range.start = 0;
+		cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
+		return 0;
+	}
+
+	rc = cxl_mem_get_partition_info(cxlm,
+					&cxlm->active_volatile_bytes,
+					&cxlm->active_persistent_bytes,
+					&cxlm->next_volatile_bytes,
+					&cxlm->next_persistent_bytes);
+	if (rc < 0) {
+		dev_err(&cxlm->pdev->dev, "Failed to query partition information\n");
+		return rc;
+	}
+
+	dev_dbg(&cxlm->pdev->dev, "Get Partition Info\n"
+		"     active_volatile_bytes = %#llx\n"
+		"     active_persistent_bytes = %#llx\n"
+		"     next_volatile_bytes = %#llx\n"
+		"     next_persistent_bytes = %#llx\n",
+			cxlm->active_volatile_bytes,
+			cxlm->active_persistent_bytes,
+			cxlm->next_volatile_bytes,
+			cxlm->next_persistent_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_only_bytes - 1;
+	cxlm->ram_range.end = cxlm->active_volatile_bytes - 1;
 
 	cxlm->pmem_range.start = 0;
-	cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
-
-	cxlm->lsa_size = le32_to_cpu(id.lsa_size);
-	memcpy(cxlm->firmware_version, id.fw_revision, sizeof(id.fw_revision));
+	cxlm->pmem_range.end = cxlm->active_persistent_bytes - 1;
 
 	return 0;
 }
@@ -1427,6 +1509,10 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
+	rc = cxl_mem_create_range_info(cxlm);
+	if (rc)
+		return rc;
+
 	cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlm, &cxl_memdev_fops);
 	if (IS_ERR(cxlmd))
 		return PTR_ERR(cxlmd);

[PATCH V2 3/3] cxl/mem: Adjust ram/pmem range to represent DPA ranges

[ira.weiny]

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

CXL spec defines the volatile DPA range to be 0 to Volatile memory size.
It further defines the persistent DPA range to follow directly after the
end of the Volatile DPA through the persistent memory size.  Essentially

Volatile DPA range   = [0, Volatile size)
Persistent DPA range = [Volatile size, Volatile size + Persistent size)

Adjust the pmem_range start to reflect this and remote the TODO.

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

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 341885345b53..a1fd7923dfb9 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1635,8 +1635,9 @@ static int cxl_mem_create_range_info(struct cxl_mem *cxlm)
 	if (cxlm->partition_align_bytes == 0) {
 		cxlm->ram_range.start = 0;
 		cxlm->ram_range.end = cxlm->volatile_only_bytes - 1;
-		cxlm->pmem_range.start = 0;
-		cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
+		cxlm->pmem_range.start = cxlm->volatile_only_bytes;
+		cxlm->pmem_range.end = cxlm->volatile_only_bytes +
+					cxlm->persistent_only_bytes - 1;
 		return 0;
 	}
 
@@ -1660,15 +1661,12 @@ static int cxl_mem_create_range_info(struct cxl_mem *cxlm)
 			cxlm->next_volatile_bytes,
 			cxlm->next_persistent_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->active_volatile_bytes - 1;
 
-	cxlm->pmem_range.start = 0;
-	cxlm->pmem_range.end = cxlm->active_persistent_bytes - 1;
+	cxlm->pmem_range.start = cxlm->active_volatile_bytes;
+	cxlm->pmem_range.end = cxlm->active_volatile_bytes +
+				cxlm->active_persistent_bytes - 1;
 
 	return 0;
 }
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH V2 3/3] cxl/mem: Adjust ram/pmem range to represent DPA ranges

[Jonathan Cameron]

On Thu, 17 Jun 2021 15:16:20 -0700
<ira.weiny@intel.com> wrote:

> From: Ira Weiny <ira.weiny@intel.com>
> 
> CXL spec defines the volatile DPA range to be 0 to Volatile memory size.
> It further defines the persistent DPA range to follow directly after the
> end of the Volatile DPA through the persistent memory size.

Reference would be good for anyone sanity checking against the spec. I think it's
Table 176 that says this.

> Essentially
> 
> Volatile DPA range   = [0, Volatile size)
> Persistent DPA range = [Volatile size, Volatile size + Persistent size)
> 
> Adjust the pmem_range start to reflect this and remote the TODO.

remove

> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/pci.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 341885345b53..a1fd7923dfb9 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1635,8 +1635,9 @@ static int cxl_mem_create_range_info(struct cxl_mem *cxlm)
>  	if (cxlm->partition_align_bytes == 0) {
>  		cxlm->ram_range.start = 0;
>  		cxlm->ram_range.end = cxlm->volatile_only_bytes - 1;
> -		cxlm->pmem_range.start = 0;
> -		cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
> +		cxlm->pmem_range.start = cxlm->volatile_only_bytes;
> +		cxlm->pmem_range.end = cxlm->volatile_only_bytes +
> +					cxlm->persistent_only_bytes - 1;
>  		return 0;
>  	}
>  
> @@ -1660,15 +1661,12 @@ static int cxl_mem_create_range_info(struct cxl_mem *cxlm)
>  			cxlm->next_volatile_bytes,
>  			cxlm->next_persistent_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->active_volatile_bytes - 1;
>  
> -	cxlm->pmem_range.start = 0;
> -	cxlm->pmem_range.end = cxlm->active_persistent_bytes - 1;
> +	cxlm->pmem_range.start = cxlm->active_volatile_bytes;
> +	cxlm->pmem_range.end = cxlm->active_volatile_bytes +
> +				cxlm->active_persistent_bytes - 1;
>  
>  	return 0;
>  }

[PATCH V3] cxl/mem: Adjust ram/pmem range to represent DPA ranges

[ira.weiny]

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

Table 176 of the CXL spec defines the volatile DPA range to be 0 to
Volatile memory size.  It further defines the persistent DPA range to
follow directly after the end of the Volatile DPA through the persistent
memory size.  Essentially

Volatile DPA range   = [0, Volatile size)
Persistent DPA range = [Volatile size, Volatile size + Persistent size)

Adjust the pmem_range start to reflect this and remove the TODO.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
For some reason I fixed this but failed to send it out...

Changes for V3
	Jonathan -- Clean up commit message

---
 drivers/cxl/pci.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 3c8b7aa8c273..592374baa005 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1633,8 +1633,9 @@ static int cxl_mem_create_range_info(struct cxl_mem *cxlm)
 	if (cxlm->partition_align_bytes == 0) {
 		cxlm->ram_range.start = 0;
 		cxlm->ram_range.end = cxlm->volatile_only_bytes - 1;
-		cxlm->pmem_range.start = 0;
-		cxlm->pmem_range.end = cxlm->persistent_only_bytes - 1;
+		cxlm->pmem_range.start = cxlm->volatile_only_bytes;
+		cxlm->pmem_range.end = cxlm->volatile_only_bytes +
+					cxlm->persistent_only_bytes - 1;
 		return 0;
 	}
 
@@ -1658,15 +1659,12 @@ static int cxl_mem_create_range_info(struct cxl_mem *cxlm)
 			cxlm->next_volatile_bytes,
 			cxlm->next_persistent_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->active_volatile_bytes - 1;
 
-	cxlm->pmem_range.start = 0;
-	cxlm->pmem_range.end = cxlm->active_persistent_bytes - 1;
+	cxlm->pmem_range.start = cxlm->active_volatile_bytes;
+	cxlm->pmem_range.end = cxlm->active_volatile_bytes +
+				cxlm->active_persistent_bytes - 1;
 
 	return 0;
 }
-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH 0/9] CDAT/DSMAS reading and cleanups

[ira.weiny]

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

This really should be 2 series and a single patch for the cxl_mem change.

But for internal review I'm just going to throw it out on email to see what
others think.

Here are the 3 cover letters.

<Documentation>
The auxiliary device documentation was not wrong but it was a bit difficult to
follow.  Add clarifications to ensure that details are not missed.
</Documentation>

<CDAT/DSMAS>
Read CDAT and DSMAS data from the device.

This work was built on Jonathan's V4 series here[1].  The big change is a
conversion to an Auxiliary bus infrastructure which allows the DOE code to be
in a separate driver object which is attached to any DOE devices which are
created by the CXL device.

Other devices can then create DOE devices and have the doe driver automatically
attach to this functionality.  Furthermore, user space control of DOE mailboxes
will be easier because of the separation from the parent device control.

[1] https://lore.kernel.org/linux-cxl/20210524133938.2815206-1-Jonathan.Cameron@huawei.com
</CDAT/DSMAS>

<Single patch so no cover for the cxlmem change.>


Ira Weiny (5):
  Documentation/auxiliary_bus: Clarify auxiliary_device creation
  Documentation/auxiliary_bus: Clarify match_name
  Documentation/auxiliary_bus: Update Auxiliary device lifespan
  cxl/cdat: Parse out DSMASS data from CDAT table
  cxl/cxlmem: Change struct cxl_mem to a more descriptive name

Jonathan Cameron (4):
  PCI: Add vendor ID for the PCI SIG
  PCI/DOE: Add Data Object Exchange Aux Driver
  cxl/pci: Add DOE Auxiliary Devices
  cxl/mem: Add CDAT table reading from DOE

 Documentation/driver-api/auxiliary_bus.rst | 137 +++-
 drivers/cxl/Kconfig                        |   1 +
 drivers/cxl/cdat.h                         |  81 +++
 drivers/cxl/core/mbox.c                    | 160 ++---
 drivers/cxl/core/memdev.c                  | 205 +++++-
 drivers/cxl/cxl.h                          |  20 +
 drivers/cxl/cxlmem.h                       |  79 ++-
 drivers/cxl/pci.c                          | 355 ++++++++---
 drivers/cxl/pmem.c                         |  32 +-
 drivers/pci/Kconfig                        |  11 +
 drivers/pci/Makefile                       |   3 +
 drivers/pci/doe.c                          | 704 +++++++++++++++++++++
 include/linux/pci-doe.h                    |  63 ++
 include/linux/pci_ids.h                    |   1 +
 include/uapi/linux/pci_regs.h              |  29 +-
 tools/testing/cxl/test/mem.c               |  50 +-
 16 files changed, 1670 insertions(+), 261 deletions(-)
 create mode 100644 drivers/cxl/cdat.h
 create mode 100644 drivers/pci/doe.c
 create mode 100644 include/linux/pci-doe.h

-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH 1/9] Documentation/auxiliary_bus: Clarify auxiliary_device creation

[ira.weiny]

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

The documentation for creating an auxiliary device is a 3 step not a 2
step process.  Specifically the requirements of setting the name, id,
dev.release, and dev.parent fields was not clear as a precursor to the '2
step' process documented.

Clarify by declaring this a 3 step process starting with setting the
fields of struct auxiliary_device correctly.

Also add some sample code and tie the changed into the rest of the
documentation.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 Documentation/driver-api/auxiliary_bus.rst | 78 +++++++++++++++++-----
 1 file changed, 60 insertions(+), 18 deletions(-)

diff --git a/Documentation/driver-api/auxiliary_bus.rst b/Documentation/driver-api/auxiliary_bus.rst
index ef902daf0d68..29b1322592ef 100644
--- a/Documentation/driver-api/auxiliary_bus.rst
+++ b/Documentation/driver-api/auxiliary_bus.rst
@@ -79,18 +79,6 @@ is given a name that, combined with the registering drivers KBUILD_MODNAME,
 creates a match_name that is used for driver binding, and an id that combined
 with the match_name provide a unique name to register with the bus subsystem.
 
-Registering an auxiliary_device is a two-step process.  First call
-auxiliary_device_init(), which checks several aspects of the auxiliary_device
-struct and performs a device_initialize().  After this step completes, any
-error state must have a call to auxiliary_device_uninit() in its resolution path.
-The second step in registering an auxiliary_device is to perform a call to
-auxiliary_device_add(), which sets the name of the device and add the device to
-the bus.
-
-Unregistering an auxiliary_device is also a two-step process to mirror the
-register process.  First call auxiliary_device_delete(), then call
-auxiliary_device_uninit().
-
 .. code-block:: c
 
 	struct auxiliary_device {
@@ -99,15 +87,69 @@ auxiliary_device_uninit().
 		u32 id;
 	};
 
-If two auxiliary_devices both with a match_name "mod.foo" are registered onto
-the bus, they must have unique id values (e.g. "x" and "y") so that the
-registered devices names are "mod.foo.x" and "mod.foo.y".  If match_name + id
-are not unique, then the device_add fails and generates an error message.
+Registering an auxiliary_device is a three-step process.
+
+First, a 'struct auxiliary_device' needs to be defined or allocated for each
+sub-device desired.  The name, id, dev.release, and dev.parent fields of this
+structure must be filled in as follows.
+
+The 'name' field is to be given a name that is recognized by the auxiliary
+driver.  If two auxiliary_devices with the same match_name, eg
+"mod.MY_DEVICE_NAME", are registered onto the bus, they must have unique id
+values (e.g. "x" and "y") so that the registered devices names are "mod.foo.x"
+and "mod.foo.y".  If match_name + id are not unique, then the device_add fails
+and generates an error message.
 
 The auxiliary_device.dev.type.release or auxiliary_device.dev.release must be
-populated with a non-NULL pointer to successfully register the auxiliary_device.
+populated with a non-NULL pointer to successfully register the
+auxiliary_device.
+
+The auxiliary_device.dev.parent should be set.  Typically to the registering
+drivers device.
+
+Second, call auxiliary_device_init(), which checks several aspects of the
+auxiliary_device struct and performs a device_initialize().  After this step
+completes, any error state must have a call to auxiliary_device_uninit() in its
+resolution path.
+
+The third and final step in registering an auxiliary_device is to perform a
+call to auxiliary_device_add(), which sets the name of the device and adds the
+device to the bus.
+
+.. code-block:: c
+
+	struct axiliary_device *my_aux_dev = my_aux_dev_alloc(xxx);
+
+        /* Step 1: */
+	my_aux_dev->name = MY_DEVICE_NAME;
+	my_aux_dev->id = alloc_unique_id(xxx);
+	my_aux_dev->dev.release = my_aux_dev_release;
+	my_aux_dev->dev.parent = my_dev;
+
+        /* Step 2: */
+        if (auxiliary_device_init(my_aux_dev))
+                goto fail;
+
+        /* Step 3: */
+        if (auxiliary_device_add(my_aux_dev)) {
+                auxiliary_device_uninit(my_aux_dev);
+                goto fail;
+        }
+
+        /* manage the memory */
+        my_dev->my_aux_dev = my_aux_dev;
+
+Unregistering an auxiliary_device is a two-step process to mirror the register
+process.  First call auxiliary_device_delete(), then call
+auxiliary_device_uninit().
+
+
+.. code-block:: c
+
+        auxiliary_device_delete(my_dev->my_aux_dev);
+        auxiliary_device_uninit(my_dev->my_aux_dev);
+        my_aux_dev_free(my_dev->my_aux_dev);
 
-The auxiliary_device.dev.parent must also be populated.
 
 Auxiliary Device Memory Model and Lifespan
 ------------------------------------------
-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH 2/9] Documentation/auxiliary_bus: Clarify match_name

[ira.weiny]

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

Provide example code for how the match name is formed and where it is
supposed to be set.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 Documentation/driver-api/auxiliary_bus.rst | 33 ++++++++++++++++++++--
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/Documentation/driver-api/auxiliary_bus.rst b/Documentation/driver-api/auxiliary_bus.rst
index 29b1322592ef..498b9ad4b6ff 100644
--- a/Documentation/driver-api/auxiliary_bus.rst
+++ b/Documentation/driver-api/auxiliary_bus.rst
@@ -78,6 +78,9 @@ An auxiliary_device represents a part of its parent device's functionality. It
 is given a name that, combined with the registering drivers KBUILD_MODNAME,
 creates a match_name that is used for driver binding, and an id that combined
 with the match_name provide a unique name to register with the bus subsystem.
+For example, a driver registering an auxiliary device is named 'foo_mod.ko' and
+the subdevice is named 'foo_dev'.  The match name is therefore
+'foo_mod.foo_dev'.
 
 .. code-block:: c
 
@@ -95,9 +98,9 @@ structure must be filled in as follows.
 
 The 'name' field is to be given a name that is recognized by the auxiliary
 driver.  If two auxiliary_devices with the same match_name, eg
-"mod.MY_DEVICE_NAME", are registered onto the bus, they must have unique id
-values (e.g. "x" and "y") so that the registered devices names are "mod.foo.x"
-and "mod.foo.y".  If match_name + id are not unique, then the device_add fails
+"foo_mod.foo_dev", are registered onto the bus, they must have unique id
+values (e.g. "x" and "y") so that the registered devices names are "foo_mod.foo_dev.x"
+and "foo_mod.foo_dev.y".  If match_name + id are not unique, then the device_add fails
 and generates an error message.
 
 The auxiliary_device.dev.type.release or auxiliary_device.dev.release must be
@@ -118,6 +121,10 @@ device to the bus.
 
 .. code-block:: c
 
+        #define MY_DEVICE_NAME "foo_dev"
+
+        ...
+
 	struct axiliary_device *my_aux_dev = my_aux_dev_alloc(xxx);
 
         /* Step 1: */
@@ -139,6 +146,9 @@ device to the bus.
         /* manage the memory */
         my_dev->my_aux_dev = my_aux_dev;
 
+        ...
+
+
 Unregistering an auxiliary_device is a two-step process to mirror the register
 process.  First call auxiliary_device_delete(), then call
 auxiliary_device_uninit().
@@ -206,6 +216,23 @@ Auxiliary drivers register themselves with the bus by calling
 auxiliary_driver_register(). The id_table contains the match_names of auxiliary
 devices that a driver can bind with.
 
+.. code-block:: c
+
+        static const struct auxiliary_device_id my_auxiliary_id_table[] = {
+		{ .name = "foo_mod.foo_dev" },
+                {},
+        };
+
+        MODULE_DEVICE_TABLE(auxiliary, my_auxiliary_id_table);
+
+        struct auxiliary_driver my_drv = {
+                .name = "myauxiliarydrv",
+                .id_table = my_auxiliary_id_table,
+                .probe = my_drv_probe,
+                .remove = my_drv_remove
+        };
+
+
 Example Usage
 =============
 
-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH 3/9] Documentation/auxiliary_bus: Update Auxiliary device lifespan

[ira.weiny]

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

It was unclear when the auxiliary device objects were to be free'ed by
the parent (registering) driver.

Also there are some patterns like using devm_add_action_or_reset() which
are helpful to mention to those using the interface to ensure they don't
double free or miss freeing the auxiliary devices.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 Documentation/driver-api/auxiliary_bus.rst | 32 ++++++++++++++--------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/Documentation/driver-api/auxiliary_bus.rst b/Documentation/driver-api/auxiliary_bus.rst
index 498b9ad4b6ff..5635845313be 100644
--- a/Documentation/driver-api/auxiliary_bus.rst
+++ b/Documentation/driver-api/auxiliary_bus.rst
@@ -165,9 +165,15 @@ Auxiliary Device Memory Model and Lifespan
 ------------------------------------------
 
 The registering driver is the entity that allocates memory for the
-auxiliary_device and register it on the auxiliary bus.  It is important to note
+auxiliary_device and registers it on the auxiliary bus.  It is important to note
 that, as opposed to the platform bus, the registering driver is wholly
-responsible for the management for the memory used for the driver object.
+responsible for the management of the memory used for the device object.
+
+To be clear the memory for the auxiliary_device is freed in the release()
+callback defined by the registering driver.  The registering driver should only
+call auxiliary_device_delete() and then auxiliary_device_uninit() when it is
+done with the device.  The release() function is then automatically called if
+and when other drivers release their reference to the devices.
 
 A parent object, defined in the shared header file, contains the
 auxiliary_device.  It also contains a pointer to the shared object(s), which
@@ -178,18 +184,22 @@ from the pointer to the auxiliary_device, that is passed during the call to the
 auxiliary_driver's probe function, up to the parent object, and then have
 access to the shared object(s).
 
-The memory for the auxiliary_device is freed only in its release() callback
-flow as defined by its registering driver.
-
 The memory for the shared object(s) must have a lifespan equal to, or greater
-than, the lifespan of the memory for the auxiliary_device.  The auxiliary_driver
-should only consider that this shared object is valid as long as the
-auxiliary_device is still registered on the auxiliary bus.  It is up to the
-registering driver to manage (e.g. free or keep available) the memory for the
-shared object beyond the life of the auxiliary_device.
+than, the lifespan of the memory for the auxiliary_device.  The
+auxiliary_driver should only consider that the shared object is valid as long
+as the auxiliary_device is still registered on the auxiliary bus.  It is up to
+the registering driver to manage (e.g. free or keep available) the memory for
+the shared object beyond the life of the auxiliary_device.
 
 The registering driver must unregister all auxiliary devices before its own
-driver.remove() is completed.
+driver.remove() is completed.  An easy way to ensure this is to use the
+devm_add_action_or_reset() call to register a function against the parent device
+which unregisters the auxiliary device object(s).
+
+Finally, any operations which operate on the auxiliary devices must continue to
+function (if only to return an error) after the registering driver unregisters
+the auxiliary device.
+
 
 Auxiliary Drivers
 =================
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH 0/9] CDAT/DSMAS reading and cleanups

[Ira Weiny]

On Wed, Oct 27, 2021 at 03:40:34PM -0700, 'Ira Weiny' wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> This really should be 2 series and a single patch for the cxl_mem change.
> 
> But for internal review I'm just going to throw it out on email to see what
> others think.

Sorry for the noise.  Somehow the public email got in my to list.  These should
be coming out soon.  But there are still a couple of questions and the series
should be 3 sets not 1.

Ira