[PATCH v3 00/16] cxl: Add CXL feature commands support via fwctl

[PATCH v3 00/16] cxl: Add CXL feature commands support via fwctl

[Dave Jiang]

v3:
- Rearrange code and shift code forward to reduce diffs. (Jonathan)
- Use struct_size() in appropriate locations. (Jonathan)
- Remove usage of __weak and refactor accordingly. (Jason)
- Return NULL for allocation functions. (Jonathan)
- See individual patches for detailed changes from v2.

v2:
- Drop features device and driver. Move features enabling to cxl_memdev. (Dan)
- Drop sysfs attribute of number of features. (Dan)
- Drop moving of include/uapi/linux/cxl_mem.h. (Dan)
- Set get driver info ioctl to return reserved 32bit. Set behavior of issue
  ioctl successful as indicating Feature commands supported.
- Set 'Set Feature Size' to 0 for kernel exclusive Features.
- See individual patches for detailed changes from v1.
- Hide FWCTL bits behind CONFIG_CXL_FWCTL. (Dan)

v1:
- Create a CXL features driver to handle all feature command related bits
  including FWCTL support. (Dan)
- Tested against CXL CLI unit tests for FWCTL.
- See individual patches for detailed changes from RFC v2.

RFC v2:
- Dropped 1/13 and 2/13 from previous version. Merged upstream already.
- Combined changes from Shiju for "get supported features"
- Addressed comments from Jonathan and Jason
- See specific changes in individual patch revision history
- Added hardware command info to FWCTL
- Added filtering to set feature command
- Added documentation

This series add support for CXL feature commands using the FWCTL framework [1].
The code has been tested with CXL CLI unit tests for FWCTL.

Patches 1-7 are shared with EDAC series [1] and there will be an immutable branch
once accepted post review.

CXL Features support is added behind the CXL mem driver. The 3 mailbox commands
"Get Supported Features", "Get Feature", and "Set Feature" are implemented. The
"Get" commands are under the FWCTL_RPC_CONFIGURATION policy, the "Set" command
checks the policy depending on the set effects of the feature defined by the
device via the Feature set effects field. All mailbox commands for CXL provides
an effects table that describes the effects of a command when performed on the
device.  For CXL features, there is also an effects field that describes the
effects a feature set operation has on the device per feature. The security
policy is checked against this feature specific effects field. 

The code is based off of v3 of FWCTL series [2] posted by Jason and rebased on top of
v6.13-rc5.

[1]: https://lore.kernel.org/linux-cxl/20250106121017.1620-1-shiju.jose@huawei.com/T/#t
[2]: https://lore.kernel.org/linux-cxl/0-v3-960f17f90f17+516-fwctl_jgg@nvidia.com/#r
---

Dave Jiang (14):
      cxl: Refactor user ioctl command path from mds to mailbox
      cxl: Enumerate feature commands
      cxl: Add Get Supported Features command for kernel usage
      cxl/test: Add Get Supported Features mailbox command support
      cxl: Setup exclusive CXL features that are reserved for the kernel
      cxl: Add FWCTL support to the CXL memdev driver
      cxl: Add support for FWCTL get driver information callback
      cxl: Move cxl feature command structs to user header
      cxl: Add support for fwctl RPC command to enable CXL feature commands
      cxl: Add support to handle user feature commands for get feature
      cxl: Add support to handle user feature commands for set feature
      cxl/test: Add Get Feature support to cxl_test
      cxl/test: Add Set Feature support to cxl_test
      fwctl/cxl: Add documentation to FWCTL CXL

Shiju Jose (2):
      cxl/mbox: Add GET_FEATURE mailbox command
      cxl/mbox: Add SET_FEATURE mailbox command

 Documentation/userspace-api/fwctl/fwctl-cxl.rst | 137 +++++++++++++++
 Documentation/userspace-api/fwctl/index.rst     |   1 +
 MAINTAINERS                                     |   1 +
 drivers/cxl/Kconfig                             |  15 ++
 drivers/cxl/Makefile                            |   3 +-
 drivers/cxl/core/Makefile                       |   1 +
 drivers/cxl/core/core.h                         |   6 +-
 drivers/cxl/core/features.c                     | 175 +++++++++++++++++++
 drivers/cxl/core/mbox.c                         | 128 +++++++++-----
 drivers/cxl/core/memdev.c                       |  22 ++-
 drivers/cxl/cxl.h                               |   3 +
 drivers/cxl/cxlmem.h                            |  45 +----
 drivers/cxl/features.c                          | 237 ++++++++++++++++++++++++++
 drivers/cxl/features.h                          |  41 +++++
 drivers/cxl/fwctl.c                             | 398 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/mem.c                               |   5 +
 include/cxl/features.h                          |  79 +++++++++
 include/cxl/mailbox.h                           |  44 ++++-
 include/uapi/cxl/features.h                     | 128 ++++++++++++++
 include/uapi/fwctl/cxl.h                        |  56 +++++++
 include/uapi/fwctl/fwctl.h                      |   1 +
 tools/testing/cxl/Kbuild                        |   4 +-
 tools/testing/cxl/test/mem.c                    | 177 ++++++++++++++++++++
 23 files changed, 1609 insertions(+), 98 deletions(-)

[PATCH v3 01/16] cxl: Refactor user ioctl command path from mds to mailbox

[Dave Jiang]

With 'struct cxl_mailbox' context introduced, the helper functions
cxl_query_cmd() and cxl_send_cmd() can take a cxl_mailbox directly
rather than a cxl_memdev parameter. Refactor to use cxl_mailbox
directly.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/core.h   |  6 ++-
 drivers/cxl/core/mbox.c   | 89 +++++++++++++++++++--------------------
 drivers/cxl/core/memdev.c | 22 +++++++---
 drivers/cxl/cxlmem.h      | 40 ------------------
 include/cxl/mailbox.h     | 41 +++++++++++++++++-
 5 files changed, 102 insertions(+), 96 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 800466f96a68..23761340e65c 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -4,6 +4,8 @@
 #ifndef __CXL_CORE_H__
 #define __CXL_CORE_H__
 
+#include <cxl/mailbox.h>
+
 extern const struct device_type cxl_nvdimm_bridge_type;
 extern const struct device_type cxl_nvdimm_type;
 extern const struct device_type cxl_pmu_type;
@@ -65,9 +67,9 @@ static inline void cxl_region_exit(void)
 
 struct cxl_send_command;
 struct cxl_mem_query_commands;
-int cxl_query_cmd(struct cxl_memdev *cxlmd,
+int cxl_query_cmd(struct cxl_mailbox *cxl_mbox,
 		  struct cxl_mem_query_commands __user *q);
-int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s);
+int cxl_send_cmd(struct cxl_mailbox *cxl_mailbox, struct cxl_send_command __user *s);
 void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
 				   resource_size_t length);
 
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 548564c770c0..bdb8f060f2c1 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -349,40 +349,40 @@ static bool cxl_payload_from_user_allowed(u16 opcode, void *payload_in)
 	return true;
 }
 
-static int cxl_mbox_cmd_ctor(struct cxl_mbox_cmd *mbox,
-			     struct cxl_memdev_state *mds, u16 opcode,
+static int cxl_mbox_cmd_ctor(struct cxl_mbox_cmd *mbox_cmd,
+			     struct cxl_mailbox *cxl_mbox, u16 opcode,
 			     size_t in_size, size_t out_size, u64 in_payload)
 {
-	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
-	*mbox = (struct cxl_mbox_cmd) {
+	*mbox_cmd = (struct cxl_mbox_cmd) {
 		.opcode = opcode,
 		.size_in = in_size,
 	};
 
 	if (in_size) {
-		mbox->payload_in = vmemdup_user(u64_to_user_ptr(in_payload),
-						in_size);
-		if (IS_ERR(mbox->payload_in))
-			return PTR_ERR(mbox->payload_in);
+		mbox_cmd->payload_in = vmemdup_user(u64_to_user_ptr(in_payload),
+						    in_size);
+		if (IS_ERR(mbox_cmd->payload_in))
+			return PTR_ERR(mbox_cmd->payload_in);
 
-		if (!cxl_payload_from_user_allowed(opcode, mbox->payload_in)) {
-			dev_dbg(mds->cxlds.dev, "%s: input payload not allowed\n",
+		if (!cxl_payload_from_user_allowed(opcode,
+						   mbox_cmd->payload_in)) {
+			dev_dbg(cxl_mbox->host, "%s: input payload not allowed\n",
 				cxl_mem_opcode_to_name(opcode));
-			kvfree(mbox->payload_in);
+			kvfree(mbox_cmd->payload_in);
 			return -EBUSY;
 		}
 	}
 
 	/* Prepare to handle a full payload for variable sized output */
 	if (out_size == CXL_VARIABLE_PAYLOAD)
-		mbox->size_out = cxl_mbox->payload_size;
+		mbox_cmd->size_out = cxl_mbox->payload_size;
 	else
-		mbox->size_out = out_size;
+		mbox_cmd->size_out = out_size;
 
-	if (mbox->size_out) {
-		mbox->payload_out = kvzalloc(mbox->size_out, GFP_KERNEL);
-		if (!mbox->payload_out) {
-			kvfree(mbox->payload_in);
+	if (mbox_cmd->size_out) {
+		mbox_cmd->payload_out = kvzalloc(mbox_cmd->size_out, GFP_KERNEL);
+		if (!mbox_cmd->payload_out) {
+			kvfree(mbox_cmd->payload_in);
 			return -ENOMEM;
 		}
 	}
@@ -397,10 +397,8 @@ static void cxl_mbox_cmd_dtor(struct cxl_mbox_cmd *mbox)
 
 static int cxl_to_mem_cmd_raw(struct cxl_mem_command *mem_cmd,
 			      const struct cxl_send_command *send_cmd,
-			      struct cxl_memdev_state *mds)
+			      struct cxl_mailbox *cxl_mbox)
 {
-	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
-
 	if (send_cmd->raw.rsvd)
 		return -EINVAL;
 
@@ -415,7 +413,7 @@ static int cxl_to_mem_cmd_raw(struct cxl_mem_command *mem_cmd,
 	if (!cxl_mem_raw_command_allowed(send_cmd->raw.opcode))
 		return -EPERM;
 
-	dev_WARN_ONCE(mds->cxlds.dev, true, "raw command path used\n");
+	dev_WARN_ONCE(cxl_mbox->host, true, "raw command path used\n");
 
 	*mem_cmd = (struct cxl_mem_command) {
 		.info = {
@@ -431,7 +429,7 @@ static int cxl_to_mem_cmd_raw(struct cxl_mem_command *mem_cmd,
 
 static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd,
 			  const struct cxl_send_command *send_cmd,
-			  struct cxl_memdev_state *mds)
+			  struct cxl_mailbox *cxl_mbox)
 {
 	struct cxl_mem_command *c = &cxl_mem_commands[send_cmd->id];
 	const struct cxl_command_info *info = &c->info;
@@ -446,11 +444,11 @@ static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd,
 		return -EINVAL;
 
 	/* Check that the command is enabled for hardware */
-	if (!test_bit(info->id, mds->enabled_cmds))
+	if (!test_bit(info->id, cxl_mbox->enabled_cmds))
 		return -ENOTTY;
 
 	/* Check that the command is not claimed for exclusive kernel use */
-	if (test_bit(info->id, mds->exclusive_cmds))
+	if (test_bit(info->id, cxl_mbox->exclusive_cmds))
 		return -EBUSY;
 
 	/* Check the input buffer is the expected size */
@@ -479,7 +477,7 @@ static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd,
 /**
  * cxl_validate_cmd_from_user() - Check fields for CXL_MEM_SEND_COMMAND.
  * @mbox_cmd: Sanitized and populated &struct cxl_mbox_cmd.
- * @mds: The driver data for the operation
+ * @cxl_mbox: CXL mailbox context
  * @send_cmd: &struct cxl_send_command copied in from userspace.
  *
  * Return:
@@ -494,10 +492,9 @@ static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd,
  * safe to send to the hardware.
  */
 static int cxl_validate_cmd_from_user(struct cxl_mbox_cmd *mbox_cmd,
-				      struct cxl_memdev_state *mds,
+				      struct cxl_mailbox *cxl_mbox,
 				      const struct cxl_send_command *send_cmd)
 {
-	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
 	struct cxl_mem_command mem_cmd;
 	int rc;
 
@@ -514,24 +511,23 @@ static int cxl_validate_cmd_from_user(struct cxl_mbox_cmd *mbox_cmd,
 
 	/* Sanitize and construct a cxl_mem_command */
 	if (send_cmd->id == CXL_MEM_COMMAND_ID_RAW)
-		rc = cxl_to_mem_cmd_raw(&mem_cmd, send_cmd, mds);
+		rc = cxl_to_mem_cmd_raw(&mem_cmd, send_cmd, cxl_mbox);
 	else
-		rc = cxl_to_mem_cmd(&mem_cmd, send_cmd, mds);
+		rc = cxl_to_mem_cmd(&mem_cmd, send_cmd, cxl_mbox);
 
 	if (rc)
 		return rc;
 
 	/* Sanitize and construct a cxl_mbox_cmd */
-	return cxl_mbox_cmd_ctor(mbox_cmd, mds, mem_cmd.opcode,
+	return cxl_mbox_cmd_ctor(mbox_cmd, cxl_mbox, mem_cmd.opcode,
 				 mem_cmd.info.size_in, mem_cmd.info.size_out,
 				 send_cmd->in.payload);
 }
 
-int cxl_query_cmd(struct cxl_memdev *cxlmd,
+int cxl_query_cmd(struct cxl_mailbox *cxl_mbox,
 		  struct cxl_mem_query_commands __user *q)
 {
-	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
-	struct device *dev = &cxlmd->dev;
+	struct device *dev = cxl_mbox->host;
 	struct cxl_mem_command *cmd;
 	u32 n_commands;
 	int j = 0;
@@ -552,9 +548,9 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd,
 	cxl_for_each_cmd(cmd) {
 		struct cxl_command_info info = cmd->info;
 
-		if (test_bit(info.id, mds->enabled_cmds))
+		if (test_bit(info.id, cxl_mbox->enabled_cmds))
 			info.flags |= CXL_MEM_COMMAND_FLAG_ENABLED;
-		if (test_bit(info.id, mds->exclusive_cmds))
+		if (test_bit(info.id, cxl_mbox->exclusive_cmds))
 			info.flags |= CXL_MEM_COMMAND_FLAG_EXCLUSIVE;
 
 		if (copy_to_user(&q->commands[j++], &info, sizeof(info)))
@@ -569,7 +565,7 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd,
 
 /**
  * handle_mailbox_cmd_from_user() - Dispatch a mailbox command for userspace.
- * @mds: The driver data for the operation
+ * @cxl_mbox: The mailbox context for the operation.
  * @mbox_cmd: The validated mailbox command.
  * @out_payload: Pointer to userspace's output payload.
  * @size_out: (Input) Max payload size to copy out.
@@ -590,13 +586,12 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd,
  *
  * See cxl_send_cmd().
  */
-static int handle_mailbox_cmd_from_user(struct cxl_memdev_state *mds,
+static int handle_mailbox_cmd_from_user(struct cxl_mailbox *cxl_mbox,
 					struct cxl_mbox_cmd *mbox_cmd,
 					u64 out_payload, s32 *size_out,
 					u32 *retval)
 {
-	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
-	struct device *dev = mds->cxlds.dev;
+	struct device *dev = cxl_mbox->host;
 	int rc;
 
 	dev_dbg(dev,
@@ -633,10 +628,9 @@ static int handle_mailbox_cmd_from_user(struct cxl_memdev_state *mds,
 	return rc;
 }
 
-int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
+int cxl_send_cmd(struct cxl_mailbox *cxl_mbox, struct cxl_send_command __user *s)
 {
-	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
-	struct device *dev = &cxlmd->dev;
+	struct device *dev = cxl_mbox->host;
 	struct cxl_send_command send;
 	struct cxl_mbox_cmd mbox_cmd;
 	int rc;
@@ -646,11 +640,11 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s)
 	if (copy_from_user(&send, s, sizeof(send)))
 		return -EFAULT;
 
-	rc = cxl_validate_cmd_from_user(&mbox_cmd, mds, &send);
+	rc = cxl_validate_cmd_from_user(&mbox_cmd, cxl_mbox, &send);
 	if (rc)
 		return rc;
 
-	rc = handle_mailbox_cmd_from_user(mds, &mbox_cmd, send.out.payload,
+	rc = handle_mailbox_cmd_from_user(cxl_mbox, &mbox_cmd, send.out.payload,
 					  &send.out.size, &send.retval);
 	if (rc)
 		return rc;
@@ -724,6 +718,7 @@ static int cxl_xfer_log(struct cxl_memdev_state *mds, uuid_t *uuid,
  */
 static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
 {
+	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
 	struct cxl_cel_entry *cel_entry;
 	const int cel_entries = size / sizeof(*cel_entry);
 	struct device *dev = mds->cxlds.dev;
@@ -737,7 +732,7 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
 		int enabled = 0;
 
 		if (cmd) {
-			set_bit(cmd->info.id, mds->enabled_cmds);
+			set_bit(cmd->info.id, cxl_mbox->enabled_cmds);
 			enabled++;
 		}
 
@@ -807,6 +802,7 @@ static const uuid_t log_uuid[] = {
  */
 int cxl_enumerate_cmds(struct cxl_memdev_state *mds)
 {
+	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
 	struct cxl_mbox_get_supported_logs *gsl;
 	struct device *dev = mds->cxlds.dev;
 	struct cxl_mem_command *cmd;
@@ -845,7 +841,7 @@ int cxl_enumerate_cmds(struct cxl_memdev_state *mds)
 		/* In case CEL was bogus, enable some default commands. */
 		cxl_for_each_cmd(cmd)
 			if (cmd->flags & CXL_CMD_FLAG_FORCE_ENABLE)
-				set_bit(cmd->info.id, mds->enabled_cmds);
+				set_bit(cmd->info.id, cxl_mbox->enabled_cmds);
 
 		/* Found the required CEL */
 		rc = 0;
@@ -1448,6 +1444,7 @@ struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev)
 	mutex_init(&mds->event.log_lock);
 	mds->cxlds.dev = dev;
 	mds->cxlds.reg_map.host = dev;
+	mds->cxlds.cxl_mbox.host = dev;
 	mds->cxlds.reg_map.resource = CXL_RESOURCE_NONE;
 	mds->cxlds.type = CXL_DEVTYPE_CLASSMEM;
 	mds->ram_perf.qos_class = CXL_QOS_CLASS_INVALID;
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index ae3dfcbe8938..2e2e035abdaa 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -564,9 +564,11 @@ EXPORT_SYMBOL_NS_GPL(is_cxl_memdev, "CXL");
 void set_exclusive_cxl_commands(struct cxl_memdev_state *mds,
 				unsigned long *cmds)
 {
+	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
+
 	down_write(&cxl_memdev_rwsem);
-	bitmap_or(mds->exclusive_cmds, mds->exclusive_cmds, cmds,
-		  CXL_MEM_COMMAND_ID_MAX);
+	bitmap_or(cxl_mbox->exclusive_cmds, cxl_mbox->exclusive_cmds,
+		  cmds, CXL_MEM_COMMAND_ID_MAX);
 	up_write(&cxl_memdev_rwsem);
 }
 EXPORT_SYMBOL_NS_GPL(set_exclusive_cxl_commands, "CXL");
@@ -579,9 +581,11 @@ EXPORT_SYMBOL_NS_GPL(set_exclusive_cxl_commands, "CXL");
 void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
 				  unsigned long *cmds)
 {
+	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
+
 	down_write(&cxl_memdev_rwsem);
-	bitmap_andnot(mds->exclusive_cmds, mds->exclusive_cmds, cmds,
-		      CXL_MEM_COMMAND_ID_MAX);
+	bitmap_andnot(cxl_mbox->exclusive_cmds, cxl_mbox->exclusive_cmds,
+		      cmds, CXL_MEM_COMMAND_ID_MAX);
 	up_write(&cxl_memdev_rwsem);
 }
 EXPORT_SYMBOL_NS_GPL(clear_exclusive_cxl_commands, "CXL");
@@ -656,11 +660,14 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
 static long __cxl_memdev_ioctl(struct cxl_memdev *cxlmd, unsigned int cmd,
 			       unsigned long arg)
 {
+	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
+	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
+
 	switch (cmd) {
 	case CXL_MEM_QUERY_COMMANDS:
-		return cxl_query_cmd(cxlmd, (void __user *)arg);
+		return cxl_query_cmd(cxl_mbox, (void __user *)arg);
 	case CXL_MEM_SEND_COMMAND:
-		return cxl_send_cmd(cxlmd, (void __user *)arg);
+		return cxl_send_cmd(cxl_mbox, (void __user *)arg);
 	default:
 		return -ENOTTY;
 	}
@@ -994,10 +1001,11 @@ static void cxl_remove_fw_upload(void *fwl)
 int devm_cxl_setup_fw_upload(struct device *host, struct cxl_memdev_state *mds)
 {
 	struct cxl_dev_state *cxlds = &mds->cxlds;
+	struct cxl_mailbox *cxl_mbox = &cxlds->cxl_mbox;
 	struct device *dev = &cxlds->cxlmd->dev;
 	struct fw_upload *fwl;
 
-	if (!test_bit(CXL_MEM_COMMAND_ID_GET_FW_INFO, mds->enabled_cmds))
+	if (!test_bit(CXL_MEM_COMMAND_ID_GET_FW_INFO, cxl_mbox->enabled_cmds))
 		return 0;
 
 	fwl = firmware_upload_register(THIS_MODULE, dev, dev_name(dev),
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 2a25d1957ddb..a0a49809cd76 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -106,42 +106,6 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
 	return xa_load(&port->endpoints, (unsigned long)&cxlmd->dev);
 }
 
-/**
- * struct cxl_mbox_cmd - A command to be submitted to hardware.
- * @opcode: (input) The command set and command submitted to hardware.
- * @payload_in: (input) Pointer to the input payload.
- * @payload_out: (output) Pointer to the output payload. Must be allocated by
- *		 the caller.
- * @size_in: (input) Number of bytes to load from @payload_in.
- * @size_out: (input) Max number of bytes loaded into @payload_out.
- *            (output) Number of bytes generated by the device. For fixed size
- *            outputs commands this is always expected to be deterministic. For
- *            variable sized output commands, it tells the exact number of bytes
- *            written.
- * @min_out: (input) internal command output payload size validation
- * @poll_count: (input) Number of timeouts to attempt.
- * @poll_interval_ms: (input) Time between mailbox background command polling
- *                    interval timeouts.
- * @return_code: (output) Error code returned from hardware.
- *
- * This is the primary mechanism used to send commands to the hardware.
- * All the fields except @payload_* correspond exactly to the fields described in
- * Command Register section of the CXL 2.0 8.2.8.4.5. @payload_in and
- * @payload_out are written to, and read from the Command Payload Registers
- * defined in CXL 2.0 8.2.8.4.8.
- */
-struct cxl_mbox_cmd {
-	u16 opcode;
-	void *payload_in;
-	void *payload_out;
-	size_t size_in;
-	size_t size_out;
-	size_t min_out;
-	int poll_count;
-	int poll_interval_ms;
-	u16 return_code;
-};
-
 /*
  * Per CXL 3.0 Section 8.2.8.4.5.1
  */
@@ -461,8 +425,6 @@ static inline struct cxl_dev_state *mbox_to_cxlds(struct cxl_mailbox *cxl_mbox)
  * @lsa_size: Size of Label Storage Area
  *                (CXL 2.0 8.2.9.5.1.1 Identify Memory Device)
  * @firmware_version: Firmware version for the memory device.
- * @enabled_cmds: Hardware commands found enabled in CEL.
- * @exclusive_cmds: Commands that are kernel-internal only
  * @total_bytes: sum of all possible capacities
  * @volatile_only_bytes: hard volatile capacity
  * @persistent_only_bytes: hard persistent capacity
@@ -485,8 +447,6 @@ struct cxl_memdev_state {
 	struct cxl_dev_state cxlds;
 	size_t lsa_size;
 	char firmware_version[0x10];
-	DECLARE_BITMAP(enabled_cmds, CXL_MEM_COMMAND_ID_MAX);
-	DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX);
 	u64 total_bytes;
 	u64 volatile_only_bytes;
 	u64 persistent_only_bytes;
diff --git a/include/cxl/mailbox.h b/include/cxl/mailbox.h
index bacd111e75f1..cc894f07a435 100644
--- a/include/cxl/mailbox.h
+++ b/include/cxl/mailbox.h
@@ -3,12 +3,49 @@
 #ifndef __CXL_MBOX_H__
 #define __CXL_MBOX_H__
 #include <linux/rcuwait.h>
+#include <uapi/linux/cxl_mem.h>
 
-struct cxl_mbox_cmd;
+/**
+ * struct cxl_mbox_cmd - A command to be submitted to hardware.
+ * @opcode: (input) The command set and command submitted to hardware.
+ * @payload_in: (input) Pointer to the input payload.
+ * @payload_out: (output) Pointer to the output payload. Must be allocated by
+ *		 the caller.
+ * @size_in: (input) Number of bytes to load from @payload_in.
+ * @size_out: (input) Max number of bytes loaded into @payload_out.
+ *            (output) Number of bytes generated by the device. For fixed size
+ *            outputs commands this is always expected to be deterministic. For
+ *            variable sized output commands, it tells the exact number of bytes
+ *            written.
+ * @min_out: (input) internal command output payload size validation
+ * @poll_count: (input) Number of timeouts to attempt.
+ * @poll_interval_ms: (input) Time between mailbox background command polling
+ *                    interval timeouts.
+ * @return_code: (output) Error code returned from hardware.
+ *
+ * This is the primary mechanism used to send commands to the hardware.
+ * All the fields except @payload_* correspond exactly to the fields described in
+ * Command Register section of the CXL 2.0 8.2.8.4.5. @payload_in and
+ * @payload_out are written to, and read from the Command Payload Registers
+ * defined in CXL 2.0 8.2.8.4.8.
+ */
+struct cxl_mbox_cmd {
+	u16 opcode;
+	void *payload_in;
+	void *payload_out;
+	size_t size_in;
+	size_t size_out;
+	size_t min_out;
+	int poll_count;
+	int poll_interval_ms;
+	u16 return_code;
+};
 
 /**
  * struct cxl_mailbox - context for CXL mailbox operations
  * @host: device that hosts the mailbox
+ * @enabled_cmds: mailbox commands that are enabled by the driver
+ * @exclusive_cmds: mailbox commands that are exclusive to the kernel
  * @payload_size: Size of space for payload
  *                (CXL 3.1 8.2.8.4.3 Mailbox Capabilities Register)
  * @mbox_mutex: mutex protects device mailbox and firmware
@@ -17,6 +54,8 @@ struct cxl_mbox_cmd;
  */
 struct cxl_mailbox {
 	struct device *host;
+	DECLARE_BITMAP(enabled_cmds, CXL_MEM_COMMAND_ID_MAX);
+	DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX);
 	size_t payload_size;
 	struct mutex mbox_mutex; /* lock to protect mailbox context */
 	struct rcuwait mbox_wait;
-- 
2.48.1

[PATCH v3 02/16] cxl: Enumerate feature commands

[Dave Jiang]

Add feature commands enumeration code in order to detect and enumerate
the 3 feature related commands "get supported features", "get feature",
and "set feature". The enumeration will help determine whether the driver
can issue any of the 3 commands to the device.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- Add comma to enum entries that are not end entry. (Jonathan)
- Pull devm_cxfs_allocate() forward from patch 8. (Jonathan)
- Return NULL for failure path of allocate function. (Jonathan)
---
 drivers/cxl/Makefile     |  2 +-
 drivers/cxl/core/mbox.c  | 30 +++++++++++++
 drivers/cxl/cxl.h        |  2 +
 drivers/cxl/cxlmem.h     |  5 +++
 drivers/cxl/features.c   | 91 ++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/features.h   |  8 ++++
 drivers/cxl/mem.c        |  5 +++
 include/cxl/features.h   | 35 ++++++++++++++++
 include/cxl/mailbox.h    |  2 +
 tools/testing/cxl/Kbuild |  2 +-
 10 files changed, 180 insertions(+), 2 deletions(-)
 create mode 100644 drivers/cxl/features.c
 create mode 100644 drivers/cxl/features.h
 create mode 100644 include/cxl/features.h

diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
index 2caa90fa4bf2..12fbc35081bb 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -17,5 +17,5 @@ obj-$(CONFIG_CXL_PCI) += cxl_pci.o
 cxl_port-y := port.o
 cxl_acpi-y := acpi.o
 cxl_pmem-y := pmem.o security.o
-cxl_mem-y := mem.o
+cxl_mem-y := mem.o features.o
 cxl_pci-y := pci.o
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index bdb8f060f2c1..9fe552e3d465 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -69,6 +69,29 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
 	CXL_CMD(GET_TIMESTAMP, 0, 0x8, 0),
 };
 
+static u16 cxl_feature_commands[] = {
+	CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
+	CXL_MBOX_OP_GET_FEATURE,
+	CXL_MBOX_OP_SET_FEATURE,
+};
+
+/**
+ * cxl_get_feature_command_id() - Get the index id for a feature command
+ * @opcode: The device opcode for the feature command
+ *
+ * Return the index id on success or -errno on failure
+ */
+int cxl_get_feature_command_id(u16 opcode)
+{
+	for (int i = 0; i < ARRAY_SIZE(cxl_feature_commands); i++) {
+		if (cxl_feature_commands[i] == opcode)
+			return i;
+	}
+
+	return -ENOENT;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_get_feature_command_id, "CXL");
+
 /*
  * Commands that RAW doesn't permit. The rationale for each:
  *
@@ -734,6 +757,13 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
 		if (cmd) {
 			set_bit(cmd->info.id, cxl_mbox->enabled_cmds);
 			enabled++;
+		} else {
+			int fid = cxl_get_feature_command_id(opcode);
+
+			if (fid >= 0) {
+				set_bit(fid, cxl_mbox->feature_cmds);
+				enabled++;
+			}
 		}
 
 		if (cxl_is_poison_command(opcode)) {
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index f6015f24ad38..2d6f7c87e5e8 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -911,6 +911,8 @@ void cxl_coordinates_combine(struct access_coordinate *out,
 
 bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
 
+int cxl_get_feature_command_id(u16 opcode);
+
 /*
  * Unit test builds overrides this to __weak, find the 'strong' version
  * of these symbols in tools/testing/cxl/.
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index a0a49809cd76..4a42cdb64b5c 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -39,6 +39,7 @@
  * @dev: driver core device object
  * @cdev: char dev core object for ioctl operations
  * @cxlds: The device state backing this device
+ * @cxlfs: The features state for the device
  * @detach_work: active memdev lost a port in its ancestry
  * @cxl_nvb: coordinate removal of @cxl_nvd if present
  * @cxl_nvd: optional bridge to an nvdimm if the device supports pmem
@@ -50,6 +51,7 @@ struct cxl_memdev {
 	struct device dev;
 	struct cdev cdev;
 	struct cxl_dev_state *cxlds;
+	struct cxl_features_state *cxlfs;
 	struct work_struct detach_work;
 	struct cxl_nvdimm_bridge *cxl_nvb;
 	struct cxl_nvdimm *cxl_nvd;
@@ -490,6 +492,9 @@ enum cxl_opcode {
 	CXL_MBOX_OP_GET_LOG_CAPS	= 0x0402,
 	CXL_MBOX_OP_CLEAR_LOG           = 0x0403,
 	CXL_MBOX_OP_GET_SUP_LOG_SUBLIST = 0x0405,
+	CXL_MBOX_OP_GET_SUPPORTED_FEATURES	= 0x0500,
+	CXL_MBOX_OP_GET_FEATURE		= 0x0501,
+	CXL_MBOX_OP_SET_FEATURE		= 0x0502,
 	CXL_MBOX_OP_IDENTIFY		= 0x4000,
 	CXL_MBOX_OP_GET_PARTITION_INFO	= 0x4100,
 	CXL_MBOX_OP_SET_PARTITION_INFO	= 0x4101,
diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
new file mode 100644
index 000000000000..958e4828a58d
--- /dev/null
+++ b/drivers/cxl/features.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
+#include <linux/device.h>
+#include <cxl/mailbox.h>
+#include <cxl/features.h>
+#include "cxl.h"
+#include "cxlmem.h"
+#include "features.h"
+
+static void enumerate_feature_cmds(struct cxl_memdev *cxlmd,
+				   struct cxl_features_state *cxlfs)
+{
+	struct cxl_mailbox *cxl_mbox = &cxlmd->cxlds->cxl_mbox;
+	int fid;
+
+	fid = cxl_get_feature_command_id(CXL_MBOX_OP_GET_SUPPORTED_FEATURES);
+	if (!test_bit(fid, cxl_mbox->feature_cmds))
+		return;
+
+	fid = cxl_get_feature_command_id(CXL_MBOX_OP_GET_FEATURE);
+	if (!test_bit(fid, cxl_mbox->feature_cmds))
+		return;
+
+	cxlfs->cap = CXL_FEATURES_RO;
+
+	fid = cxl_get_feature_command_id(CXL_MBOX_OP_SET_FEATURE);
+	if (!test_bit(fid, cxl_mbox->feature_cmds))
+		return;
+
+	cxlfs->cap = CXL_FEATURES_RW;
+}
+
+static void cxlfs_free(void *_cxlfs)
+{
+	kfree(_cxlfs);
+}
+DEFINE_FREE(free_cxlfs, struct cxl_features_state *, if (_T) cxlfs_free(_T))
+
+static struct cxl_features_state *devm_cxlfs_allocate(struct cxl_memdev *cxlmd)
+{
+	int rc;
+
+	struct cxl_features_state *cxlfs __free(free_cxlfs) =
+		kzalloc(sizeof(*cxlfs), GFP_KERNEL);
+	if (!cxlfs)
+		return NULL;
+
+	cxlfs->cxlmd = cxlmd;
+
+	rc = devm_add_action_or_reset(&cxlmd->dev, cxlfs_free, cxlfs);
+	if (rc)
+		return NULL;
+
+	return no_free_ptr(cxlfs);
+}
+
+static void devm_cxlfs_free(struct cxl_memdev *cxlmd)
+{
+	kfree(cxlmd->cxlfs);
+	/* Set in devm_cxl_add_features(), make sure it's cleared */
+	cxlmd->cxlfs = NULL;
+}
+
+static void cxl_cxlfs_free(void *_cxlfs)
+{
+	struct cxl_features_state *cxlfs = _cxlfs;
+
+	devm_cxlfs_free(cxlfs->cxlmd);
+}
+DEFINE_FREE(cxl_free_cxlfs, struct cxl_features_state *, if (_T) cxl_cxlfs_free(_T))
+
+/**
+ * devm_cxl_add_features() - Allocate and initialize features context
+ * @cxlmd: CXL memory device
+ *
+ * Return 0 on success or -errno on failure.
+ */
+int devm_cxl_add_features(struct cxl_memdev *cxlmd)
+{
+	struct cxl_features_state *cxlfs __free(cxl_free_cxlfs) =
+		devm_cxlfs_allocate(cxlmd);
+	if (!cxlfs)
+		return -ENOMEM;
+
+	enumerate_feature_cmds(cxlmd, cxlfs);
+
+	cxlmd->cxlfs = no_free_ptr(cxlfs);
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(devm_cxl_add_features, "CXL");
diff --git a/drivers/cxl/features.h b/drivers/cxl/features.h
new file mode 100644
index 000000000000..0cc6d9e6c441
--- /dev/null
+++ b/drivers/cxl/features.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright(c) 2025 Intel Corporation. */
+#ifndef __CXL_FEATURES_LOCAL__
+#define __CXL_FEATURES_LOCAL__
+
+int devm_cxl_add_features(struct cxl_memdev *cxlmd);
+
+#endif
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 2f03a4d5606e..47348a52bc05 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -7,6 +7,7 @@
 
 #include "cxlmem.h"
 #include "cxlpci.h"
+#include "features.h"
 
 /**
  * DOC: cxl mem
@@ -180,6 +181,10 @@ static int cxl_mem_probe(struct device *dev)
 			return rc;
 	}
 
+	rc = devm_cxl_add_features(cxlmd);
+	if (rc)
+		dev_dbg(dev, "No CXL Features enumerated.\n");
+
 	/*
 	 * The kernel may be operating out of CXL memory on this device,
 	 * there is no spec defined way to determine whether this device
diff --git a/include/cxl/features.h b/include/cxl/features.h
new file mode 100644
index 000000000000..8e1830f0ccba
--- /dev/null
+++ b/include/cxl/features.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright(c) 2024-2025 Intel Corporation. */
+#ifndef __CXL_FEATURES_H__
+#define __CXL_FEATURES_H__
+
+struct cxl_mailbox;
+
+/* Index IDs for CXL mailbox Feature commands */
+enum feature_cmds {
+	CXL_FEATURE_ID_GET_SUPPORTED_FEATURES = 0,
+	CXL_FEATURE_ID_GET_FEATURE,
+	CXL_FEATURE_ID_SET_FEATURE,
+	CXL_FEATURE_ID_MAX
+};
+
+/* Feature commands capability supported by a device */
+enum cxl_features_capability {
+	CXL_FEATURES_NONE = 0,
+	CXL_FEATURES_RO,
+	CXL_FEATURES_RW,
+};
+
+/**
+ * struct cxl_features_state - The Features state for the device
+ * @cxlmd: Pointer to cxl mem device
+ * @cap: Feature commands capability
+ * @num_features: total Features supported by the device
+ */
+struct cxl_features_state {
+	struct cxl_memdev *cxlmd;
+	enum cxl_features_capability cap;
+	int num_features;
+};
+
+#endif
diff --git a/include/cxl/mailbox.h b/include/cxl/mailbox.h
index cc894f07a435..99d3bbe1ba2a 100644
--- a/include/cxl/mailbox.h
+++ b/include/cxl/mailbox.h
@@ -3,6 +3,7 @@
 #ifndef __CXL_MBOX_H__
 #define __CXL_MBOX_H__
 #include <linux/rcuwait.h>
+#include <cxl/features.h>
 #include <uapi/linux/cxl_mem.h>
 
 /**
@@ -56,6 +57,7 @@ struct cxl_mailbox {
 	struct device *host;
 	DECLARE_BITMAP(enabled_cmds, CXL_MEM_COMMAND_ID_MAX);
 	DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX);
+	DECLARE_BITMAP(feature_cmds, CXL_FEATURE_ID_MAX);
 	size_t payload_size;
 	struct mutex mbox_mutex; /* lock to protect mailbox context */
 	struct rcuwait mbox_wait;
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index b1256fee3567..47be82a2dd5b 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -46,7 +46,7 @@ cxl_port-y += cxl_port_test.o
 
 obj-m += cxl_mem.o
 
-cxl_mem-y := $(CXL_SRC)/mem.o
+cxl_mem-y := $(CXL_SRC)/mem.o $(CXL_SRC)/features.o
 cxl_mem-y += config_check.o
 cxl_mem-y += cxl_mem_test.o
 
-- 
2.48.1

[PATCH v3 03/16] cxl: Add Get Supported Features command for kernel usage

[Dave Jiang]

CXL spec r3.2 8.2.9.6.1 Get Supported Features (Opcode 0500h)
The command retrieve the list of supported device-specific features
(identified by UUID) and general information about each Feature.

The driver will retrieve the feature entries in order to make checks and
provide information for the Get Feature and Set Feature command. One of
the main piece of information retrieved are the effects a Set Feature
command would have for a particular feature.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- Use struct_size() instead of open coding the calculation. (Jonathan)
---
 drivers/cxl/features.c | 138 +++++++++++++++++++++++++++++++++++++++++
 include/cxl/features.h |  57 +++++++++++++++++
 2 files changed, 195 insertions(+)

diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
index 958e4828a58d..4d8c603121f0 100644
--- a/drivers/cxl/features.c
+++ b/drivers/cxl/features.c
@@ -7,6 +7,139 @@
 #include "cxlmem.h"
 #include "features.h"
 
+static void cxl_free_feature_entries(void *entries)
+{
+	kvfree(entries);
+}
+
+static int cxl_get_supported_features_count(struct cxl_mailbox *cxl_mbox)
+{
+	struct cxl_mbox_get_sup_feats_out mbox_out;
+	struct cxl_mbox_get_sup_feats_in mbox_in;
+	struct cxl_mbox_cmd mbox_cmd;
+	int rc;
+
+	memset(&mbox_in, 0, sizeof(mbox_in));
+	mbox_in.count = cpu_to_le32(sizeof(mbox_out));
+	memset(&mbox_out, 0, sizeof(mbox_out));
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
+		.size_in = sizeof(mbox_in),
+		.payload_in = &mbox_in,
+		.size_out = sizeof(mbox_out),
+		.payload_out = &mbox_out,
+		.min_out = sizeof(mbox_out),
+	};
+	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
+	if (rc < 0)
+		return rc;
+
+	return le16_to_cpu(mbox_out.supported_feats);
+}
+
+static int get_supported_features(struct cxl_memdev *cxlmd,
+				  struct cxl_features_state *cxlfs)
+{
+	int remain_feats, max_size, max_feats, start, rc, hdr_size;
+	struct cxl_mailbox *cxl_mbox = &cxlmd->cxlds->cxl_mbox;
+	int feat_size = sizeof(struct cxl_feat_entry);
+	struct cxl_mbox_get_sup_feats_in mbox_in;
+	struct cxl_feat_entry *entry;
+	struct cxl_mbox_cmd mbox_cmd;
+	int count;
+
+	if (cxlfs->cap < CXL_FEATURES_RO)
+		return -EOPNOTSUPP;
+
+	count = cxl_get_supported_features_count(cxl_mbox);
+	if (count == 0)
+		return -ENOENT;
+	if (count < 0)
+		return -ENXIO;
+
+	struct cxl_feat_entry *entries __free(kvfree) =
+		kvmalloc(count * sizeof(*entries), GFP_KERNEL);
+	if (!entries)
+		return -ENOMEM;
+
+	struct cxl_mbox_get_sup_feats_out *mbox_out __free(kvfree) =
+		kvmalloc(cxl_mbox->payload_size, GFP_KERNEL);
+	if (!mbox_out)
+		return -ENOMEM;
+
+	hdr_size = struct_size(mbox_out, ents, 0);
+	max_size = cxl_mbox->payload_size - hdr_size;
+	/* max feat entries that can fit in mailbox max payload size */
+	max_feats = max_size / feat_size;
+	entry = entries;
+
+	start = 0;
+	remain_feats = count;
+	do {
+		int retrieved, alloc_size, copy_feats;
+		int num_entries;
+
+		if (remain_feats > max_feats) {
+			alloc_size = struct_size(mbox_out, ents, max_feats);
+			remain_feats = remain_feats - max_feats;
+			copy_feats = max_feats;
+		} else {
+			alloc_size = struct_size(mbox_out, ents, remain_feats);
+			copy_feats = remain_feats;
+			remain_feats = 0;
+		}
+
+		memset(&mbox_in, 0, sizeof(mbox_in));
+		mbox_in.count = cpu_to_le32(alloc_size);
+		mbox_in.start_idx = cpu_to_le16(start);
+		memset(mbox_out, 0, alloc_size);
+		mbox_cmd = (struct cxl_mbox_cmd) {
+			.opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
+			.size_in = sizeof(mbox_in),
+			.payload_in = &mbox_in,
+			.size_out = alloc_size,
+			.payload_out = mbox_out,
+			.min_out = hdr_size,
+		};
+		rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
+		if (rc < 0)
+			return rc;
+
+		if (mbox_cmd.size_out <= hdr_size)
+			return -ENXIO;
+
+		/*
+		 * Make sure retrieved out buffer is multiple of feature
+		 * entries.
+		 */
+		retrieved = mbox_cmd.size_out - hdr_size;
+		if (retrieved % feat_size)
+			return -ENXIO;
+
+		num_entries = le16_to_cpu(mbox_out->num_entries);
+		/*
+		 * If the reported output entries * defined entry size !=
+		 * retrieved output bytes, then the output package is incorrect.
+		 */
+		if (num_entries * feat_size != retrieved)
+			return -ENXIO;
+
+		memcpy(entry, mbox_out->ents, retrieved);
+		entry++;
+		/*
+		 * If the number of output entries is less than expected, add the
+		 * remaining entries to the next batch.
+		 */
+		remain_feats += copy_feats - num_entries;
+		start += num_entries;
+	} while (remain_feats);
+
+	cxlfs->num_features = count;
+	cxlfs->entries = no_free_ptr(entries);
+	return devm_add_action_or_reset(&cxlmd->dev, cxl_free_feature_entries,
+					cxlfs->entries);
+}
+
 static void enumerate_feature_cmds(struct cxl_memdev *cxlmd,
 				   struct cxl_features_state *cxlfs)
 {
@@ -77,12 +210,17 @@ DEFINE_FREE(cxl_free_cxlfs, struct cxl_features_state *, if (_T) cxl_cxlfs_free(
  */
 int devm_cxl_add_features(struct cxl_memdev *cxlmd)
 {
+	int rc;
+
 	struct cxl_features_state *cxlfs __free(cxl_free_cxlfs) =
 		devm_cxlfs_allocate(cxlmd);
 	if (!cxlfs)
 		return -ENOMEM;
 
 	enumerate_feature_cmds(cxlmd, cxlfs);
+	rc = get_supported_features(cxlmd, cxlfs);
+	if (rc)
+		return rc;
 
 	cxlmd->cxlfs = no_free_ptr(cxlfs);
 
diff --git a/include/cxl/features.h b/include/cxl/features.h
index 8e1830f0ccba..c65760342f97 100644
--- a/include/cxl/features.h
+++ b/include/cxl/features.h
@@ -3,6 +3,8 @@
 #ifndef __CXL_FEATURES_H__
 #define __CXL_FEATURES_H__
 
+#include <linux/uuid.h>
+
 struct cxl_mailbox;
 
 /* Index IDs for CXL mailbox Feature commands */
@@ -25,11 +27,66 @@ enum cxl_features_capability {
  * @cxlmd: Pointer to cxl mem device
  * @cap: Feature commands capability
  * @num_features: total Features supported by the device
+ * @entries: Feature detail entries fetched from the device
  */
 struct cxl_features_state {
 	struct cxl_memdev *cxlmd;
 	enum cxl_features_capability cap;
 	int num_features;
+	struct cxl_feat_entry *entries;
 };
 
+/* Get Supported Features (0x500h) CXL r3.2 8.2.9.6.1 */
+struct cxl_mbox_get_sup_feats_in {
+	__le32 count;
+	__le16 start_idx;
+	u8 reserved[2];
+} __packed;
+
+/* CXL spec r3.2 Table 8-87 command effects */
+#define CXL_CMD_CONFIG_CHANGE_COLD_RESET	BIT(0)
+#define CXL_CMD_CONFIG_CHANGE_IMMEDIATE		BIT(1)
+#define CXL_CMD_DATA_CHANGE_IMMEDIATE		BIT(2)
+#define CXL_CMD_POLICY_CHANGE_IMMEDIATE		BIT(3)
+#define CXL_CMD_LOG_CHANGE_IMMEDIATE		BIT(4)
+#define CXL_CMD_SECURITY_STATE_CHANGE		BIT(5)
+#define CXL_CMD_BACKGROUND			BIT(6)
+#define CXL_CMD_BGCMD_ABORT_SUPPORTED		BIT(7)
+#define CXL_CMD_EFFECTS_VALID			BIT(9)
+#define CXL_CMD_CONFIG_CHANGE_CONV_RESET	BIT(10)
+#define CXL_CMD_CONFIG_CHANGE_CXL_RESET		BIT(11)
+
+/*
+ * CXL spec r3.2 Table 8-109
+ * Get Supported Features Supported Feature Entry
+ */
+struct cxl_feat_entry {
+	uuid_t uuid;
+	__le16 id;
+	__le16 get_feat_size;
+	__le16 set_feat_size;
+	__le32 flags;
+	u8 get_feat_ver;
+	u8 set_feat_ver;
+	__le16 effects;
+	u8 reserved[18];
+} __packed;
+
+/* @flags field for 'struct cxl_feat_entry' */
+#define CXL_FEATURE_F_CHANGEABLE		BIT(0)
+#define CXL_FEATURE_F_PERSIST_FW_UPDATE		BIT(4)
+#define CXL_FEATURE_F_DEFAULT_SEL		BIT(5)
+#define CXL_FEATURE_F_SAVED_SEL			BIT(6)
+
+/*
+ * CXL spec r3.2 Table 8-108
+ * Get supported Features Output Payload
+ */
+struct cxl_mbox_get_sup_feats_out {
+	__le16 num_entries;
+	__le16 supported_feats;
+	u8 reserved[4];
+	struct cxl_feat_entry ents[] __counted_by_le(num_entries);
+} __packed;
+
 #endif
-- 
2.48.1

[PATCH v3 04/16] cxl/test: Add Get Supported Features mailbox command support

[Dave Jiang]

Add cxl-test emulation of Get Supported Features mailbox command.
Currently only adding a test feature with feature identifier of
all f's for testing.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Acked-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- Use struct_size(). (Jonathan)
---
 tools/testing/cxl/test/mem.c | 70 ++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index 347c1e7b37bd..90069dd686b5 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -44,6 +44,10 @@ static struct cxl_cel_entry mock_cel[] = {
 		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_SUPPORTED_LOGS),
 		.effect = CXL_CMD_EFFECT_NONE,
 	},
+	{
+		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_SUPPORTED_FEATURES),
+		.effect = CXL_CMD_EFFECT_NONE,
+	},
 	{
 		.opcode = cpu_to_le16(CXL_MBOX_OP_IDENTIFY),
 		.effect = CXL_CMD_EFFECT_NONE,
@@ -1337,6 +1341,69 @@ static int mock_activate_fw(struct cxl_mockmem_data *mdata,
 	return -EINVAL;
 }
 
+#define CXL_VENDOR_FEATURE_TEST							\
+	UUID_INIT(0xffffffff, 0xffff, 0xffff, 0xff, 0xff, 0xff, 0xff, 0xff,	\
+		  0xff, 0xff, 0xff)
+
+static void fill_feature_vendor_test(struct cxl_feat_entry *feat)
+{
+	feat->uuid = CXL_VENDOR_FEATURE_TEST;
+	feat->id = 0;
+	feat->get_feat_size = cpu_to_le16(0x4);
+	feat->set_feat_size = cpu_to_le16(0x4);
+	feat->flags = cpu_to_le32(CXL_FEATURE_F_CHANGEABLE |
+				  CXL_FEATURE_F_DEFAULT_SEL |
+				  CXL_FEATURE_F_SAVED_SEL);
+	feat->get_feat_ver = 1;
+	feat->set_feat_ver = 1;
+	feat->effects = cpu_to_le16(CXL_CMD_CONFIG_CHANGE_COLD_RESET |
+				    CXL_CMD_EFFECTS_VALID);
+}
+
+#define MAX_CXL_TEST_FEATS	1
+
+static int mock_get_supported_features(struct cxl_mockmem_data *mdata,
+				       struct cxl_mbox_cmd *cmd)
+{
+	struct cxl_mbox_get_sup_feats_in *in = cmd->payload_in;
+	struct cxl_mbox_get_sup_feats_out *out = cmd->payload_out;
+	struct cxl_feat_entry *feat;
+	u16 start_idx, count;
+
+	if (cmd->size_out < sizeof(*out)) {
+		cmd->return_code = CXL_MBOX_CMD_RC_PAYLOADLEN;
+		return -EINVAL;
+	}
+
+	/*
+	 * Current emulation only supports 1 feature
+	 */
+	start_idx = le16_to_cpu(in->start_idx);
+	if (start_idx != 0) {
+		cmd->return_code = CXL_MBOX_CMD_RC_INPUT;
+		return -EINVAL;
+	}
+
+	count = le16_to_cpu(in->count);
+	if (count < struct_size(out, ents, 0)) {
+		cmd->return_code = CXL_MBOX_CMD_RC_PAYLOADLEN;
+		return -EINVAL;
+	}
+
+	out->supported_feats = cpu_to_le16(MAX_CXL_TEST_FEATS);
+	cmd->return_code = 0;
+	if (count < struct_size(out, ents, MAX_CXL_TEST_FEATS)) {
+		out->num_entries = 0;
+		return 0;
+	}
+
+	out->num_entries = cpu_to_le16(MAX_CXL_TEST_FEATS);
+	feat = out->ents;
+	fill_feature_vendor_test(feat);
+
+	return 0;
+}
+
 static int cxl_mock_mbox_send(struct cxl_mailbox *cxl_mbox,
 			      struct cxl_mbox_cmd *cmd)
 {
@@ -1422,6 +1489,9 @@ static int cxl_mock_mbox_send(struct cxl_mailbox *cxl_mbox,
 	case CXL_MBOX_OP_ACTIVATE_FW:
 		rc = mock_activate_fw(mdata, cmd);
 		break;
+	case CXL_MBOX_OP_GET_SUPPORTED_FEATURES:
+		rc = mock_get_supported_features(mdata, cmd);
+		break;
 	default:
 		break;
 	}
-- 
2.48.1

[PATCH v3 05/16] cxl/mbox: Add GET_FEATURE mailbox command

[Dave Jiang]

From: Shiju Jose <shiju.jose@huawei.com>

Add support for GET_FEATURE mailbox command.

CXL spec r3.2 section 8.2.9.6 describes optional device specific features.
The settings of a feature can be retrieved using Get Feature command.
CXL spec r3.2 section 8.2.9.6.2 describes Get Feature command.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/Makefile   |  1 +
 drivers/cxl/core/features.c | 59 +++++++++++++++++++++++++++++++++++++
 include/cxl/features.h      | 29 ++++++++++++++++++
 tools/testing/cxl/Kbuild    |  1 +
 4 files changed, 90 insertions(+)
 create mode 100644 drivers/cxl/core/features.c

diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
index 9259bcc6773c..73b6348afd67 100644
--- a/drivers/cxl/core/Makefile
+++ b/drivers/cxl/core/Makefile
@@ -14,5 +14,6 @@ cxl_core-y += pci.o
 cxl_core-y += hdm.o
 cxl_core-y += pmu.o
 cxl_core-y += cdat.o
+cxl_core-y += features.o
 cxl_core-$(CONFIG_TRACING) += trace.o
 cxl_core-$(CONFIG_CXL_REGION) += region.o
diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
new file mode 100644
index 000000000000..b01dc5ebb24d
--- /dev/null
+++ b/drivers/cxl/core/features.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
+#include <linux/device.h>
+#include <cxl/mailbox.h>
+#include <cxl/features.h>
+#include "cxl.h"
+#include "core.h"
+#include "cxlmem.h"
+#include "features.h"
+
+size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
+		       enum cxl_get_feat_selection selection,
+		       void *feat_out, size_t feat_out_size, u16 offset,
+		       u16 *return_code)
+{
+	size_t data_to_rd_size, size_out;
+	struct cxl_mbox_get_feat_in pi;
+	struct cxl_mbox_cmd mbox_cmd;
+	size_t data_rcvd_size = 0;
+	int rc;
+
+	if (return_code)
+		*return_code = CXL_MBOX_CMD_RC_INPUT;
+
+	if (!feat_out || !feat_out_size)
+		return 0;
+
+	size_out = min(feat_out_size, cxl_mbox->payload_size);
+	uuid_copy(&pi.uuid, feat_uuid);
+	pi.selection = selection;
+	do {
+		data_to_rd_size = min(feat_out_size - data_rcvd_size,
+				      cxl_mbox->payload_size);
+		pi.offset = cpu_to_le16(offset + data_rcvd_size);
+		pi.count = cpu_to_le16(data_to_rd_size);
+
+		mbox_cmd = (struct cxl_mbox_cmd) {
+			.opcode = CXL_MBOX_OP_GET_FEATURE,
+			.size_in = sizeof(pi),
+			.payload_in = &pi,
+			.size_out = size_out,
+			.payload_out = feat_out + data_rcvd_size,
+			.min_out = data_to_rd_size,
+		};
+		rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
+		if (rc < 0 || !mbox_cmd.size_out) {
+			if (return_code)
+				*return_code = mbox_cmd.return_code;
+			return 0;
+		}
+		data_rcvd_size += mbox_cmd.size_out;
+	} while (data_rcvd_size < feat_out_size);
+
+	if (return_code)
+		*return_code = CXL_MBOX_CMD_RC_SUCCESS;
+
+	return data_rcvd_size;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_get_feature, "CXL");
diff --git a/include/cxl/features.h b/include/cxl/features.h
index c65760342f97..5e70c6c3ae9d 100644
--- a/include/cxl/features.h
+++ b/include/cxl/features.h
@@ -89,4 +89,33 @@ struct cxl_mbox_get_sup_feats_out {
 	struct cxl_feat_entry ents[] __counted_by_le(num_entries);
 } __packed;
 
+/*
+ * Get Feature CXL spec r3.2 Spec 8.2.9.6.2
+ */
+
+/*
+ * Get Feature input payload
+ * CXL spec r3.2 section 8.2.9.6.2 Table 8-99
+ */
+struct cxl_mbox_get_feat_in {
+	uuid_t uuid;
+	__le16 offset;
+	__le16 count;
+	u8 selection;
+}  __packed;
+
+/* Selection field for 'struct cxl_mbox_get_feat_in' */
+enum cxl_get_feat_selection {
+	CXL_GET_FEAT_SEL_CURRENT_VALUE,
+	CXL_GET_FEAT_SEL_DEFAULT_VALUE,
+	CXL_GET_FEAT_SEL_SAVED_VALUE,
+	CXL_GET_FEAT_SEL_MAX
+};
+
+struct cxl_mailbox;
+size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
+		       enum cxl_get_feat_selection selection,
+		       void *feat_out, size_t feat_out_size, u16 offset,
+		       u16 *return_code);
+
 #endif
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 47be82a2dd5b..6b0a992af4a7 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -61,6 +61,7 @@ cxl_core-y += $(CXL_CORE_SRC)/pci.o
 cxl_core-y += $(CXL_CORE_SRC)/hdm.o
 cxl_core-y += $(CXL_CORE_SRC)/pmu.o
 cxl_core-y += $(CXL_CORE_SRC)/cdat.o
+cxl_core-y += $(CXL_CORE_SRC)/features.o
 cxl_core-$(CONFIG_TRACING) += $(CXL_CORE_SRC)/trace.o
 cxl_core-$(CONFIG_CXL_REGION) += $(CXL_CORE_SRC)/region.o
 cxl_core-y += config_check.o
-- 
2.48.1

[PATCH v3 06/16] cxl/mbox: Add SET_FEATURE mailbox command

[Dave Jiang]

From: Shiju Jose <shiju.jose@huawei.com>

Add support for SET_FEATURE mailbox command.

CXL spec r3.2 section 8.2.9.6 describes optional device specific features.
CXL devices supports features with changeable attributes.
The settings of a feature can be optionally modified using Set Feature
command.
CXL spec r3.2 section 8.2.9.6.3 describes Set Feature command.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- Remove extraneous (). (Jonathan)
---
 drivers/cxl/core/features.c | 81 +++++++++++++++++++++++++++++++++++++
 include/cxl/features.h      | 33 +++++++++++++++
 2 files changed, 114 insertions(+)

diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
index b01dc5ebb24d..ec0df5ad741c 100644
--- a/drivers/cxl/core/features.c
+++ b/drivers/cxl/core/features.c
@@ -57,3 +57,84 @@ size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
 	return data_rcvd_size;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_get_feature, "CXL");
+
+/*
+ * FEAT_DATA_MIN_PAYLOAD_SIZE - min extra number of bytes should be
+ * available in the mailbox for storing the actual feature data so that
+ * the feature data transfer would work as expected.
+ */
+#define FEAT_DATA_MIN_PAYLOAD_SIZE 10
+int cxl_set_feature(struct cxl_mailbox *cxl_mbox,
+		    const uuid_t *feat_uuid, u8 feat_version,
+		    void *feat_data, size_t feat_data_size,
+		    u32 feat_flag, u16 offset, u16 *return_code)
+{
+	struct cxl_memdev_set_feat_pi {
+		struct cxl_mbox_set_feat_hdr hdr;
+		u8 feat_data[];
+	}  __packed;
+	size_t data_in_size, data_sent_size = 0;
+	struct cxl_mbox_cmd mbox_cmd;
+	size_t hdr_size;
+	int rc = 0;
+
+	if (return_code)
+		*return_code = CXL_MBOX_CMD_RC_INPUT;
+
+	struct cxl_memdev_set_feat_pi *pi __free(kfree) =
+			kzalloc(cxl_mbox->payload_size, GFP_KERNEL);
+	uuid_copy(&pi->hdr.uuid, feat_uuid);
+	pi->hdr.version = feat_version;
+	feat_flag &= ~CXL_SET_FEAT_FLAG_DATA_TRANSFER_MASK;
+	feat_flag |= CXL_SET_FEAT_FLAG_DATA_SAVED_ACROSS_RESET;
+	hdr_size = sizeof(pi->hdr);
+	/*
+	 * Check minimum mbox payload size is available for
+	 * the feature data transfer.
+	 */
+	if (hdr_size + FEAT_DATA_MIN_PAYLOAD_SIZE > cxl_mbox->payload_size)
+		return -ENOMEM;
+
+	if (hdr_size + feat_data_size <= cxl_mbox->payload_size) {
+		pi->hdr.flags = cpu_to_le32(feat_flag |
+				       CXL_SET_FEAT_FLAG_FULL_DATA_TRANSFER);
+		data_in_size = feat_data_size;
+	} else {
+		pi->hdr.flags = cpu_to_le32(feat_flag |
+				       CXL_SET_FEAT_FLAG_INITIATE_DATA_TRANSFER);
+		data_in_size = cxl_mbox->payload_size - hdr_size;
+	}
+
+	do {
+		pi->hdr.offset = cpu_to_le16(offset + data_sent_size);
+		memcpy(pi->feat_data, feat_data + data_sent_size, data_in_size);
+		mbox_cmd = (struct cxl_mbox_cmd) {
+			.opcode = CXL_MBOX_OP_SET_FEATURE,
+			.size_in = hdr_size + data_in_size,
+			.payload_in = pi,
+		};
+		rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
+		if (rc < 0) {
+			if (return_code)
+				*return_code = mbox_cmd.return_code;
+			return rc;
+		}
+
+		data_sent_size += data_in_size;
+		if (data_sent_size >= feat_data_size) {
+			if (return_code)
+				*return_code = CXL_MBOX_CMD_RC_SUCCESS;
+			return 0;
+		}
+
+		if ((feat_data_size - data_sent_size) <= (cxl_mbox->payload_size - hdr_size)) {
+			data_in_size = feat_data_size - data_sent_size;
+			pi->hdr.flags = cpu_to_le32(feat_flag |
+					       CXL_SET_FEAT_FLAG_FINISH_DATA_TRANSFER);
+		} else {
+			pi->hdr.flags = cpu_to_le32(feat_flag |
+					       CXL_SET_FEAT_FLAG_CONTINUE_DATA_TRANSFER);
+		}
+	} while (true);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_set_feature, "CXL");
diff --git a/include/cxl/features.h b/include/cxl/features.h
index 5e70c6c3ae9d..a7811969cfdb 100644
--- a/include/cxl/features.h
+++ b/include/cxl/features.h
@@ -112,10 +112,43 @@ enum cxl_get_feat_selection {
 	CXL_GET_FEAT_SEL_MAX
 };
 
+/*
+ * Set Feature CXL spec r3.2  8.2.9.6.3
+ */
+
+/*
+ * Set Feature input payload
+ * CXL spec r3.2 section 8.2.9.6.3 Table 8-101
+ */
+struct cxl_mbox_set_feat_hdr {
+	uuid_t uuid;
+	__le32 flags;
+	__le16 offset;
+	u8 version;
+	u8 rsvd[9];
+}  __packed;
+
+/* Set Feature flags field */
+enum cxl_set_feat_flag_data_transfer {
+	CXL_SET_FEAT_FLAG_FULL_DATA_TRANSFER = 0,
+	CXL_SET_FEAT_FLAG_INITIATE_DATA_TRANSFER,
+	CXL_SET_FEAT_FLAG_CONTINUE_DATA_TRANSFER,
+	CXL_SET_FEAT_FLAG_FINISH_DATA_TRANSFER,
+	CXL_SET_FEAT_FLAG_ABORT_DATA_TRANSFER,
+	CXL_SET_FEAT_FLAG_DATA_TRANSFER_MAX
+};
+
+#define CXL_SET_FEAT_FLAG_DATA_TRANSFER_MASK	GENMASK(2, 0)
+
+#define CXL_SET_FEAT_FLAG_DATA_SAVED_ACROSS_RESET	BIT(3)
+
 struct cxl_mailbox;
 size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
 		       enum cxl_get_feat_selection selection,
 		       void *feat_out, size_t feat_out_size, u16 offset,
 		       u16 *return_code);
+int cxl_set_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
+		    u8 feat_version, void *feat_data, size_t feat_data_size,
+		    u32 feat_flag, u16 offset, u16 *return_code);
 
 #endif
-- 
2.48.1

[PATCH v3 07/16] cxl: Setup exclusive CXL features that are reserved for the kernel

[Dave Jiang]

Certain features will be exclusively used by components such as in
kernel RAS driver. Setup an exclusion list that can be later filtered
out before exposing to user space.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/features.c | 29 +++++++++++++++++++++++++++++
 drivers/cxl/features.c      |  4 ++++
 drivers/cxl/features.h      |  3 +++
 include/cxl/features.h      | 35 +++++++++++++++++++++++++++++++++++
 4 files changed, 71 insertions(+)

diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
index ec0df5ad741c..0ab02c2a94f9 100644
--- a/drivers/cxl/core/features.c
+++ b/drivers/cxl/core/features.c
@@ -8,6 +8,35 @@
 #include "cxlmem.h"
 #include "features.h"
 
+/* All the features below are exclusive to the kernel */
+static const uuid_t cxl_exclusive_feats[] = {
+	CXL_FEAT_PATROL_SCRUB_UUID,
+	CXL_FEAT_ECS_UUID,
+	CXL_FEAT_SPPR_UUID,
+	CXL_FEAT_HPPR_UUID,
+	CXL_FEAT_CACHELINE_SPARING_UUID,
+	CXL_FEAT_ROW_SPARING_UUID,
+	CXL_FEAT_BANK_SPARING_UUID,
+	CXL_FEAT_RANK_SPARING_UUID,
+};
+
+/**
+ * is_cxl_feature_exclusive() - Check if a CXL feature is exclusive to kernel
+ * @entry: cxl feature entry
+ *
+ * Return true if feature is exclusive to kernel, otherwise false
+ */
+bool is_cxl_feature_exclusive(struct cxl_feat_entry *entry)
+{
+	for (int i = 0; i < ARRAY_SIZE(cxl_exclusive_feats); i++) {
+		if (uuid_equal(&entry->uuid, &cxl_exclusive_feats[i]))
+			return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL_NS_GPL(is_cxl_feature_exclusive, "CXL");
+
 size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
 		       enum cxl_get_feat_selection selection,
 		       void *feat_out, size_t feat_out_size, u16 offset,
diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
index 4d8c603121f0..0086fdb20bfd 100644
--- a/drivers/cxl/features.c
+++ b/drivers/cxl/features.c
@@ -46,6 +46,7 @@ static int get_supported_features(struct cxl_memdev *cxlmd,
 	struct cxl_mbox_get_sup_feats_in mbox_in;
 	struct cxl_feat_entry *entry;
 	struct cxl_mbox_cmd mbox_cmd;
+	int user_feats = 0;
 	int count;
 
 	if (cxlfs->cap < CXL_FEATURES_RO)
@@ -125,6 +126,8 @@ static int get_supported_features(struct cxl_memdev *cxlmd,
 			return -ENXIO;
 
 		memcpy(entry, mbox_out->ents, retrieved);
+		if (!is_cxl_feature_exclusive(entry))
+			user_feats++;
 		entry++;
 		/*
 		 * If the number of output entries is less than expected, add the
@@ -135,6 +138,7 @@ static int get_supported_features(struct cxl_memdev *cxlmd,
 	} while (remain_feats);
 
 	cxlfs->num_features = count;
+	cxlfs->num_user_features = user_feats;
 	cxlfs->entries = no_free_ptr(entries);
 	return devm_add_action_or_reset(&cxlmd->dev, cxl_free_feature_entries,
 					cxlfs->entries);
diff --git a/drivers/cxl/features.h b/drivers/cxl/features.h
index 0cc6d9e6c441..8706f944b476 100644
--- a/drivers/cxl/features.h
+++ b/drivers/cxl/features.h
@@ -3,6 +3,9 @@
 #ifndef __CXL_FEATURES_LOCAL__
 #define __CXL_FEATURES_LOCAL__
 
+struct cxl_feat_entry;
+
 int devm_cxl_add_features(struct cxl_memdev *cxlmd);
+bool is_cxl_feature_exclusive(struct cxl_feat_entry *entry);
 
 #endif
diff --git a/include/cxl/features.h b/include/cxl/features.h
index a7811969cfdb..869a18559f2b 100644
--- a/include/cxl/features.h
+++ b/include/cxl/features.h
@@ -5,6 +5,39 @@
 
 #include <linux/uuid.h>
 
+/* Feature UUIDs used by the kernel */
+#define CXL_FEAT_PATROL_SCRUB_UUID						\
+	UUID_INIT(0x96dad7d6, 0xfde8, 0x482b, 0xa7, 0x33, 0x75, 0x77, 0x4e,	\
+		  0x06, 0xdb, 0x8a)
+
+#define CXL_FEAT_ECS_UUID							\
+	UUID_INIT(0xe5b13f22, 0x2328, 0x4a14, 0xb8, 0xba, 0xb9, 0x69, 0x1e,	\
+		  0x89, 0x33, 0x86)
+
+#define CXL_FEAT_SPPR_UUID							\
+	UUID_INIT(0x892ba475, 0xfad8, 0x474e, 0x9d, 0x3e, 0x69, 0x2c, 0x91,	\
+		  0x75, 0x68, 0xbb)
+
+#define CXL_FEAT_HPPR_UUID							\
+	UUID_INIT(0x80ea4521, 0x786f, 0x4127, 0xaf, 0xb1, 0xec, 0x74, 0x59,	\
+		  0xfb, 0x0e, 0x24)
+
+#define CXL_FEAT_CACHELINE_SPARING_UUID						\
+	UUID_INIT(0x96C33386, 0x91dd, 0x44c7, 0x9e, 0xcb, 0xfd, 0xaf, 0x65,	\
+		  0x03, 0xba, 0xc4)
+
+#define CXL_FEAT_ROW_SPARING_UUID						\
+	UUID_INIT(0x450ebf67, 0xb135, 0x4f97, 0xa4, 0x98, 0xc2, 0xd5, 0x7f,	\
+		  0x27, 0x9b, 0xed)
+
+#define CXL_FEAT_BANK_SPARING_UUID						\
+	UUID_INIT(0x78b79636, 0x90ac, 0x4b64, 0xa4, 0xef, 0xfa, 0xac, 0x5d,	\
+		  0x18, 0xa8, 0x63)
+
+#define CXL_FEAT_RANK_SPARING_UUID						\
+	UUID_INIT(0x34dbaff5, 0x0552, 0x4281, 0x8f, 0x76, 0xda, 0x0b, 0x5e,	\
+		  0x7a, 0x76, 0xa7)
+
 struct cxl_mailbox;
 
 /* Index IDs for CXL mailbox Feature commands */
@@ -27,12 +60,14 @@ enum cxl_features_capability {
  * @cxlmd: Pointer to cxl mem device
  * @cap: Feature commands capability
  * @num_features: total Features supported by the device
+ * @num_user_features: total Features not kernel exclusive
  * @entries: Feature detail entries fetched from the device
  */
 struct cxl_features_state {
 	struct cxl_memdev *cxlmd;
 	enum cxl_features_capability cap;
 	int num_features;
+	int num_user_features;
 	struct cxl_feat_entry *entries;
 };
 
-- 
2.48.1

[PATCH v3 08/16] cxl: Add FWCTL support to the CXL memdev driver

[Dave Jiang]

Add fwctl support code to allow sending of CXL feature commands from
userspace through as ioctls via FWCTL. Provide initial setup bits.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- Fix spelling error in Kconfig text
- Remove errant blank line
- Drop using of __weak. (Jason)
- Return NULL for allocate function. (Jonathan)
---
 drivers/cxl/Kconfig        | 15 +++++++
 drivers/cxl/Makefile       |  1 +
 drivers/cxl/features.c     |  8 +++-
 drivers/cxl/features.h     | 29 +++++++++++++
 drivers/cxl/fwctl.c        | 89 ++++++++++++++++++++++++++++++++++++++
 include/cxl/features.h     |  3 ++
 include/uapi/fwctl/fwctl.h |  1 +
 tools/testing/cxl/Kbuild   |  1 +
 8 files changed, 145 insertions(+), 2 deletions(-)
 create mode 100644 drivers/cxl/fwctl.c

diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
index 876469e23f7a..6bb1ffc74956 100644
--- a/drivers/cxl/Kconfig
+++ b/drivers/cxl/Kconfig
@@ -102,6 +102,21 @@ config CXL_MEM
 
 	  If unsure say 'm'.
 
+config CXL_FWCTL
+	bool "CXL: Firmware Control support"
+	depends on CXL_MEM
+	select FWCTL
+	help
+	  Enable support for firmware control of CXL devices. This allows
+	  a FWCTL char device to be created in order to issue ioctls from
+	  user space to exercise the CXL Features commands supported by
+	  a CXL mailbox. There are some Features defined in the CXL spec
+	  such as memory sparing. However a lot of those features are
+	  exclusive to the kernel. Most likely the Features manipulated
+	  by CXL_FWCTL will be vendor defined Features.
+
+	  If unsure say 'n'.
+
 config CXL_PORT
 	default CXL_BUS
 	tristate
diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
index 12fbc35081bb..cd28fb3bf855 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -18,4 +18,5 @@ cxl_port-y := port.o
 cxl_acpi-y := acpi.o
 cxl_pmem-y := pmem.o security.o
 cxl_mem-y := mem.o features.o
+cxl_mem-$(CONFIG_CXL_FWCTL) += fwctl.o
 cxl_pci-y := pci.o
diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
index 0086fdb20bfd..c14dc6d45985 100644
--- a/drivers/cxl/features.c
+++ b/drivers/cxl/features.c
@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
+#include <linux/pci.h>
 #include <linux/device.h>
+#include <linux/module.h>
 #include <cxl/mailbox.h>
 #include <cxl/features.h>
 #include "cxl.h"
@@ -173,7 +175,7 @@ static void cxlfs_free(void *_cxlfs)
 }
 DEFINE_FREE(free_cxlfs, struct cxl_features_state *, if (_T) cxlfs_free(_T))
 
-static struct cxl_features_state *devm_cxlfs_allocate(struct cxl_memdev *cxlmd)
+struct cxl_features_state *_devm_cxlfs_allocate(struct cxl_memdev *cxlmd)
 {
 	int rc;
 
@@ -191,7 +193,7 @@ static struct cxl_features_state *devm_cxlfs_allocate(struct cxl_memdev *cxlmd)
 	return no_free_ptr(cxlfs);
 }
 
-static void devm_cxlfs_free(struct cxl_memdev *cxlmd)
+void _devm_cxlfs_free(struct cxl_memdev *cxlmd)
 {
 	kfree(cxlmd->cxlfs);
 	/* Set in devm_cxl_add_features(), make sure it's cleared */
@@ -231,3 +233,5 @@ int devm_cxl_add_features(struct cxl_memdev *cxlmd)
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(devm_cxl_add_features, "CXL");
+
+MODULE_IMPORT_NS("CXL");
diff --git a/drivers/cxl/features.h b/drivers/cxl/features.h
index 8706f944b476..a51625774b58 100644
--- a/drivers/cxl/features.h
+++ b/drivers/cxl/features.h
@@ -3,8 +3,37 @@
 #ifndef __CXL_FEATURES_LOCAL__
 #define __CXL_FEATURES_LOCAL__
 
+struct cxl_features_state *_devm_cxlfs_allocate(struct cxl_memdev *cxlmd);
+void _devm_cxlfs_free(struct cxl_memdev *cxlmd);
+
+#ifdef CONFIG_CXL_FWCTL
+struct cxl_features_state *_devm_cxlfs_fwctl_allocate(struct cxl_memdev *cxlmd);
+void _devm_cxlfs_fwctl_free(struct cxl_memdev *cxlmd);
+#endif
+
+static inline struct cxl_features_state *
+devm_cxlfs_allocate(struct cxl_memdev *cxlmd)
+{
+#ifdef CONFIG_CXL_FWCTL
+	return _devm_cxlfs_fwctl_allocate(cxlmd);
+#else
+	return _devm_cxlfs_allocate(cxlmd);
+#endif
+}
+
+static inline void devm_cxlfs_free(struct cxl_memdev *cxlmd)
+{
+#ifdef CONFIG_CXL_FWCTL
+	_devm_cxlfs_fwctl_free(cxlmd);
+#else
+	_devm_cxlfs_free(cxlmd);
+#endif
+}
+
 struct cxl_feat_entry;
 
+struct cxl_features_state *devm_cxlfs_allocate(struct cxl_memdev *cxlmd);
+void devm_cxlfs_free(struct cxl_memdev *cxlmd);
 int devm_cxl_add_features(struct cxl_memdev *cxlmd);
 bool is_cxl_feature_exclusive(struct cxl_feat_entry *entry);
 
diff --git a/drivers/cxl/fwctl.c b/drivers/cxl/fwctl.c
new file mode 100644
index 000000000000..985315d5d503
--- /dev/null
+++ b/drivers/cxl/fwctl.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2025 Intel Corporation. All rights reserved. */
+#include <linux/fwctl.h>
+#include <linux/device.h>
+#include <cxl/features.h>
+#include "cxlmem.h"
+#include "features.h"
+
+static int cxlctl_open_uctx(struct fwctl_uctx *uctx)
+{
+	return 0;
+}
+
+static void cxlctl_close_uctx(struct fwctl_uctx *uctx)
+{
+}
+
+static void *cxlctl_info(struct fwctl_uctx *uctx, size_t *length)
+{
+	/* Place holder */
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+static void *cxlctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
+			   void *in, size_t in_len, size_t *out_len)
+{
+	/* Place holder */
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+static const struct fwctl_ops cxlctl_ops = {
+	.device_type = FWCTL_DEVICE_TYPE_CXL,
+	.uctx_size = sizeof(struct fwctl_uctx),
+	.open_uctx = cxlctl_open_uctx,
+	.close_uctx = cxlctl_close_uctx,
+	.info = cxlctl_info,
+	.fw_rpc = cxlctl_fw_rpc,
+};
+
+static void remove_cxlfs(void *_cxlfs)
+{
+	struct cxl_features_state *cxlfs = _cxlfs;
+	struct cxl_memdev *cxlmd = cxlfs->cxlmd;
+	struct fwctl_device *fwctl = &cxlfs->fwctl;
+
+	/* Set in devm_cxl_add_features(), make sure it's cleared */
+	cxlmd->cxlfs = NULL;
+	fwctl_unregister(fwctl);
+	fwctl_put(fwctl);
+}
+
+DEFINE_FREE(free_cxlfs, struct cxl_features_state *, if (_T) fwctl_put(&_T->fwctl))
+
+static struct cxl_features_state *
+__devm_cxlfs_fwctl_allocate(struct cxl_memdev *cxlmd, const struct fwctl_ops *ops)
+{
+	struct device *dev = &cxlmd->dev;
+	int rc;
+
+	struct cxl_features_state *cxlfs __free(free_cxlfs) =
+		fwctl_alloc_device(dev, ops, struct cxl_features_state, fwctl);
+	if (!cxlfs)
+		return NULL;
+
+	cxlfs->cxlmd = cxlmd;
+	rc = fwctl_register(&cxlfs->fwctl);
+	if (rc)
+		return NULL;
+
+	rc = devm_add_action_or_reset(dev, remove_cxlfs, cxlfs);
+	if (rc)
+		return NULL;
+
+	return no_free_ptr(cxlfs);
+}
+
+struct cxl_features_state *_devm_cxlfs_fwctl_allocate(struct cxl_memdev *cxlmd)
+{
+	return __devm_cxlfs_fwctl_allocate(cxlmd, &cxlctl_ops);
+}
+EXPORT_SYMBOL_NS_GPL(_devm_cxlfs_fwctl_allocate, "CXL");
+
+void _devm_cxlfs_fwctl_free(struct cxl_memdev *cxlmd)
+{
+	devm_release_action(&cxlmd->dev, remove_cxlfs, cxlmd);
+}
+EXPORT_SYMBOL_NS_GPL(_devm_cxlfs_fwctl_free, "CXL");
+
+MODULE_IMPORT_NS("FWCTL");
diff --git a/include/cxl/features.h b/include/cxl/features.h
index 869a18559f2b..b11b964e6384 100644
--- a/include/cxl/features.h
+++ b/include/cxl/features.h
@@ -4,6 +4,7 @@
 #define __CXL_FEATURES_H__
 
 #include <linux/uuid.h>
+#include <linux/fwctl.h>
 
 /* Feature UUIDs used by the kernel */
 #define CXL_FEAT_PATROL_SCRUB_UUID						\
@@ -57,6 +58,7 @@ enum cxl_features_capability {
 
 /**
  * struct cxl_features_state - The Features state for the device
+ * @fwctl: FWCTL device
  * @cxlmd: Pointer to cxl mem device
  * @cap: Feature commands capability
  * @num_features: total Features supported by the device
@@ -64,6 +66,7 @@ enum cxl_features_capability {
  * @entries: Feature detail entries fetched from the device
  */
 struct cxl_features_state {
+	struct fwctl_device fwctl;
 	struct cxl_memdev *cxlmd;
 	enum cxl_features_capability cap;
 	int num_features;
diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h
index f9b27fb5c161..4e4d30104667 100644
--- a/include/uapi/fwctl/fwctl.h
+++ b/include/uapi/fwctl/fwctl.h
@@ -43,6 +43,7 @@ enum {
 enum fwctl_device_type {
 	FWCTL_DEVICE_TYPE_ERROR = 0,
 	FWCTL_DEVICE_TYPE_MLX5 = 1,
+	FWCTL_DEVICE_TYPE_CXL = 2,
 };
 
 /**
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 6b0a992af4a7..6396d1296cef 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -47,6 +47,7 @@ cxl_port-y += cxl_port_test.o
 obj-m += cxl_mem.o
 
 cxl_mem-y := $(CXL_SRC)/mem.o $(CXL_SRC)/features.o
+cxl_mem-$(CONFIG_CXL_FWCTL) += $(CXL_SRC)/fwctl.o
 cxl_mem-y += config_check.o
 cxl_mem-y += cxl_mem_test.o
 
-- 
2.48.1

[PATCH v3 09/16] cxl: Add support for FWCTL get driver information callback

[Dave Jiang]

Add definition for fwctl_ops->info() to return driver information. The
function will return a data structure with a reserved u32. This is setup
for future usages.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v2:
- Set command_info to reserved. (Dan)
---
 drivers/cxl/fwctl.c      | 18 ++++++++++++++++--
 include/cxl/mailbox.h    |  1 +
 include/uapi/fwctl/cxl.h | 19 +++++++++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)
 create mode 100644 include/uapi/fwctl/cxl.h

diff --git a/drivers/cxl/fwctl.c b/drivers/cxl/fwctl.c
index 985315d5d503..85dee01d89d1 100644
--- a/drivers/cxl/fwctl.c
+++ b/drivers/cxl/fwctl.c
@@ -6,6 +6,8 @@
 #include "cxlmem.h"
 #include "features.h"
 
+#define to_cxl_features_state(fwctl) container_of(fwctl, struct cxl_features_state, fwctl)
+
 static int cxlctl_open_uctx(struct fwctl_uctx *uctx)
 {
 	return 0;
@@ -17,8 +19,20 @@ static void cxlctl_close_uctx(struct fwctl_uctx *uctx)
 
 static void *cxlctl_info(struct fwctl_uctx *uctx, size_t *length)
 {
-	/* Place holder */
-	return ERR_PTR(-EOPNOTSUPP);
+	struct fwctl_device *fwctl = uctx->fwctl;
+	struct cxl_features_state *cxlfs = to_cxl_features_state(fwctl);
+	struct fwctl_info_cxl *info;
+
+	if (!cxlfs->num_user_features)
+		return ERR_PTR(-EOPNOTSUPP);
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return ERR_PTR(-ENOMEM);
+
+	*length = sizeof(*info);
+
+	return info;
 }
 
 static void *cxlctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
diff --git a/include/cxl/mailbox.h b/include/cxl/mailbox.h
index 99d3bbe1ba2a..bbbbff3af61f 100644
--- a/include/cxl/mailbox.h
+++ b/include/cxl/mailbox.h
@@ -4,6 +4,7 @@
 #define __CXL_MBOX_H__
 #include <linux/rcuwait.h>
 #include <cxl/features.h>
+#include <uapi/fwctl/cxl.h>
 #include <uapi/linux/cxl_mem.h>
 
 /**
diff --git a/include/uapi/fwctl/cxl.h b/include/uapi/fwctl/cxl.h
new file mode 100644
index 000000000000..4e766ffb1388
--- /dev/null
+++ b/include/uapi/fwctl/cxl.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (c) 2024, Intel Corporation
+ *
+ * These are definitions for the mailbox command interface of CXL subsystem.
+ */
+#ifndef _UAPI_FWCTL_CXL_H_
+#define _UAPI_FWCTL_CXL_H_
+
+#include <linux/types.h>
+
+/**
+ * struct fwctl_info_cxl - ioctl(FWCTL_INFO) out_device_data
+ * @reserved: Place older for future usages
+ */
+struct fwctl_info_cxl {
+	__u32 reserved;
+};
+#endif
-- 
2.48.1

[PATCH v3 10/16] cxl: Move cxl feature command structs to user header

[Dave Jiang]

In preparation for cxl fwctl enabling, move data structures related to
cxl feature commands to a user header file.

Reviewed-by; Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 include/cxl/features.h      | 107 +------------------------------
 include/uapi/cxl/features.h | 122 ++++++++++++++++++++++++++++++++++++
 2 files changed, 123 insertions(+), 106 deletions(-)
 create mode 100644 include/uapi/cxl/features.h

diff --git a/include/cxl/features.h b/include/cxl/features.h
index b11b964e6384..5c7de465fb68 100644
--- a/include/cxl/features.h
+++ b/include/cxl/features.h
@@ -5,6 +5,7 @@
 
 #include <linux/uuid.h>
 #include <linux/fwctl.h>
+#include <uapi/cxl/features.h>
 
 /* Feature UUIDs used by the kernel */
 #define CXL_FEAT_PATROL_SCRUB_UUID						\
@@ -74,112 +75,6 @@ struct cxl_features_state {
 	struct cxl_feat_entry *entries;
 };
 
-/* Get Supported Features (0x500h) CXL r3.2 8.2.9.6.1 */
-struct cxl_mbox_get_sup_feats_in {
-	__le32 count;
-	__le16 start_idx;
-	u8 reserved[2];
-} __packed;
-
-/* CXL spec r3.2 Table 8-87 command effects */
-#define CXL_CMD_CONFIG_CHANGE_COLD_RESET	BIT(0)
-#define CXL_CMD_CONFIG_CHANGE_IMMEDIATE		BIT(1)
-#define CXL_CMD_DATA_CHANGE_IMMEDIATE		BIT(2)
-#define CXL_CMD_POLICY_CHANGE_IMMEDIATE		BIT(3)
-#define CXL_CMD_LOG_CHANGE_IMMEDIATE		BIT(4)
-#define CXL_CMD_SECURITY_STATE_CHANGE		BIT(5)
-#define CXL_CMD_BACKGROUND			BIT(6)
-#define CXL_CMD_BGCMD_ABORT_SUPPORTED		BIT(7)
-#define CXL_CMD_EFFECTS_VALID			BIT(9)
-#define CXL_CMD_CONFIG_CHANGE_CONV_RESET	BIT(10)
-#define CXL_CMD_CONFIG_CHANGE_CXL_RESET		BIT(11)
-
-/*
- * CXL spec r3.2 Table 8-109
- * Get Supported Features Supported Feature Entry
- */
-struct cxl_feat_entry {
-	uuid_t uuid;
-	__le16 id;
-	__le16 get_feat_size;
-	__le16 set_feat_size;
-	__le32 flags;
-	u8 get_feat_ver;
-	u8 set_feat_ver;
-	__le16 effects;
-	u8 reserved[18];
-} __packed;
-
-/* @flags field for 'struct cxl_feat_entry' */
-#define CXL_FEATURE_F_CHANGEABLE		BIT(0)
-#define CXL_FEATURE_F_PERSIST_FW_UPDATE		BIT(4)
-#define CXL_FEATURE_F_DEFAULT_SEL		BIT(5)
-#define CXL_FEATURE_F_SAVED_SEL			BIT(6)
-
-/*
- * CXL spec r3.2 Table 8-108
- * Get supported Features Output Payload
- */
-struct cxl_mbox_get_sup_feats_out {
-	__le16 num_entries;
-	__le16 supported_feats;
-	u8 reserved[4];
-	struct cxl_feat_entry ents[] __counted_by_le(num_entries);
-} __packed;
-
-/*
- * Get Feature CXL spec r3.2 Spec 8.2.9.6.2
- */
-
-/*
- * Get Feature input payload
- * CXL spec r3.2 section 8.2.9.6.2 Table 8-99
- */
-struct cxl_mbox_get_feat_in {
-	uuid_t uuid;
-	__le16 offset;
-	__le16 count;
-	u8 selection;
-}  __packed;
-
-/* Selection field for 'struct cxl_mbox_get_feat_in' */
-enum cxl_get_feat_selection {
-	CXL_GET_FEAT_SEL_CURRENT_VALUE,
-	CXL_GET_FEAT_SEL_DEFAULT_VALUE,
-	CXL_GET_FEAT_SEL_SAVED_VALUE,
-	CXL_GET_FEAT_SEL_MAX
-};
-
-/*
- * Set Feature CXL spec r3.2  8.2.9.6.3
- */
-
-/*
- * Set Feature input payload
- * CXL spec r3.2 section 8.2.9.6.3 Table 8-101
- */
-struct cxl_mbox_set_feat_hdr {
-	uuid_t uuid;
-	__le32 flags;
-	__le16 offset;
-	u8 version;
-	u8 rsvd[9];
-}  __packed;
-
-/* Set Feature flags field */
-enum cxl_set_feat_flag_data_transfer {
-	CXL_SET_FEAT_FLAG_FULL_DATA_TRANSFER = 0,
-	CXL_SET_FEAT_FLAG_INITIATE_DATA_TRANSFER,
-	CXL_SET_FEAT_FLAG_CONTINUE_DATA_TRANSFER,
-	CXL_SET_FEAT_FLAG_FINISH_DATA_TRANSFER,
-	CXL_SET_FEAT_FLAG_ABORT_DATA_TRANSFER,
-	CXL_SET_FEAT_FLAG_DATA_TRANSFER_MAX
-};
-
-#define CXL_SET_FEAT_FLAG_DATA_TRANSFER_MASK	GENMASK(2, 0)
-
-#define CXL_SET_FEAT_FLAG_DATA_SAVED_ACROSS_RESET	BIT(3)
-
 struct cxl_mailbox;
 size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
 		       enum cxl_get_feat_selection selection,
diff --git a/include/uapi/cxl/features.h b/include/uapi/cxl/features.h
new file mode 100644
index 000000000000..2e98efb9abf1
--- /dev/null
+++ b/include/uapi/cxl/features.h
@@ -0,0 +1,122 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (c) 2024,2025, Intel Corporation
+ *
+ * These are definitions for the mailbox command interface of CXL subsystem.
+ */
+#ifndef _UAPI_CXL_FEATURES_H_
+#define _UAPI_CXL_FEATURES_H_
+
+#include <linux/types.h>
+#ifndef __KERNEL__
+#include <uuid/uuid.h>
+#else
+#include <linux/uuid.h>
+#endif
+
+/* Get Supported Features (0x500h) CXL r3.2 8.2.9.6.1 */
+struct cxl_mbox_get_sup_feats_in {
+	__le32 count;
+	__le16 start_idx;
+	__u8 reserved[2];
+} __attribute__ ((__packed__));
+
+/* CXL spec r3.2 Table 8-87 command effects */
+#define CXL_CMD_CONFIG_CHANGE_COLD_RESET	BIT(0)
+#define CXL_CMD_CONFIG_CHANGE_IMMEDIATE		BIT(1)
+#define CXL_CMD_DATA_CHANGE_IMMEDIATE		BIT(2)
+#define CXL_CMD_POLICY_CHANGE_IMMEDIATE		BIT(3)
+#define CXL_CMD_LOG_CHANGE_IMMEDIATE		BIT(4)
+#define CXL_CMD_SECURITY_STATE_CHANGE		BIT(5)
+#define CXL_CMD_BACKGROUND			BIT(6)
+#define CXL_CMD_BGCMD_ABORT_SUPPORTED		BIT(7)
+#define CXL_CMD_EFFECTS_VALID			BIT(9)
+#define CXL_CMD_CONFIG_CHANGE_CONV_RESET	BIT(10)
+#define CXL_CMD_CONFIG_CHANGE_CXL_RESET		BIT(11)
+
+/*
+ * CXL spec r3.2 Table 8-109
+ * Get Supported Features Supported Feature Entry
+ */
+struct cxl_feat_entry {
+	uuid_t uuid;
+	__le16 id;
+	__le16 get_feat_size;
+	__le16 set_feat_size;
+	__le32 flags;
+	__u8 get_feat_ver;
+	__u8 set_feat_ver;
+	__le16 effects;
+	__u8 reserved[18];
+} __attribute__ ((__packed__));
+
+/* @flags field for 'struct cxl_feat_entry' */
+#define CXL_FEATURE_F_CHANGEABLE		BIT(0)
+#define CXL_FEATURE_F_PERSIST_FW_UPDATE		BIT(4)
+#define CXL_FEATURE_F_DEFAULT_SEL		BIT(5)
+#define CXL_FEATURE_F_SAVED_SEL			BIT(6)
+
+/*
+ * CXL spec r3.2 Table 8-108
+ * Get supported Features Output Payload
+ */
+struct cxl_mbox_get_sup_feats_out {
+	__le16 num_entries;
+	__le16 supported_feats;
+	__u8 reserved[4];
+	struct cxl_feat_entry ents[] __counted_by_le(num_entries);
+} __attribute__ ((__packed__));
+
+/*
+ * Get Feature CXL spec r3.2 Spec 8.2.9.6.2
+ */
+
+/*
+ * Get Feature input payload
+ * CXL spec r3.2 section 8.2.9.6.2 Table 8-99
+ */
+struct cxl_mbox_get_feat_in {
+	uuid_t uuid;
+	__le16 offset;
+	__le16 count;
+	__u8 selection;
+} __attribute__ ((__packed__));
+
+/* Selection field for 'struct cxl_mbox_get_feat_in' */
+enum cxl_get_feat_selection {
+	CXL_GET_FEAT_SEL_CURRENT_VALUE,
+	CXL_GET_FEAT_SEL_DEFAULT_VALUE,
+	CXL_GET_FEAT_SEL_SAVED_VALUE,
+	CXL_GET_FEAT_SEL_MAX
+};
+
+/*
+ * Set Feature CXL spec r3.2  8.2.9.6.3
+ */
+
+/*
+ * Set Feature input payload
+ * CXL spec r3.2 section 8.2.9.6.3 Table 8-101
+ */
+struct cxl_mbox_set_feat_hdr {
+	uuid_t uuid;
+	__le32 flags;
+	__le16 offset;
+	__u8 version;
+	__u8 rsvd[9];
+} __attribute__ ((__packed__));
+
+/* Set Feature flags field */
+enum cxl_set_feat_flag_data_transfer {
+	CXL_SET_FEAT_FLAG_FULL_DATA_TRANSFER = 0,
+	CXL_SET_FEAT_FLAG_INITIATE_DATA_TRANSFER,
+	CXL_SET_FEAT_FLAG_CONTINUE_DATA_TRANSFER,
+	CXL_SET_FEAT_FLAG_FINISH_DATA_TRANSFER,
+	CXL_SET_FEAT_FLAG_ABORT_DATA_TRANSFER,
+	CXL_SET_FEAT_FLAG_DATA_TRANSFER_MAX
+};
+
+#define CXL_SET_FEAT_FLAG_DATA_TRANSFER_MASK		GENMASK(2, 0)
+#define CXL_SET_FEAT_FLAG_DATA_SAVED_ACROSS_RESET	BIT(3)
+
+#endif
-- 
2.48.1

[PATCH v3 11/16] cxl: Add support for fwctl RPC command to enable CXL feature commands

[Dave Jiang]

fwctl provides a fwctl_ops->fw_rpc() callback in order to issue ioctls
to a device. The cxl fwctl driver will start by supporting the CXL
feature commands: Get Supported Features, Get Feature, and Set Feature.

The fw_rpc() callback provides 'enum fwctl_rpc_scope' parameter where
it indicates the security scope of the call. The Get Supported Features
and Get Feature calls can be executed with the scope of
FWCTL_RPC_CONFIGRATION. The Set Feature call is gated by the effects
of the feature reported by Get Supported Features call for the specific
feature.

Only "get supported features" is supported in this patch. Additional
commands will be added in follow on patches. "Get supported features"
will filter the features that are exclusive to the kernel and only
report out features that are not kernel only.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- Cleanup for loop initializers. (Jonathan)
- Move is_cxl_feature_exclusive() prototype. (Jonathan)
- Move cxlctl_fwc_rpc() input param name change. (Jonathan)
- Use struct_size() for calculation. (Jonathan)
---
 drivers/cxl/core/mbox.c     |   9 ++
 drivers/cxl/cxl.h           |   1 +
 drivers/cxl/fwctl.c         | 200 +++++++++++++++++++++++++++++++++++-
 include/cxl/features.h      |   8 --
 include/uapi/cxl/features.h |   1 +
 include/uapi/fwctl/cxl.h    |  37 +++++++
 6 files changed, 246 insertions(+), 10 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 9fe552e3d465..d38a5fc1384f 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -92,6 +92,15 @@ int cxl_get_feature_command_id(u16 opcode)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_get_feature_command_id, "CXL");
 
+u16 cxl_get_feature_command_opcode(int feature_id)
+{
+	if (feature_id >= ARRAY_SIZE(cxl_feature_commands))
+		return 0xffff;
+
+	return cxl_feature_commands[feature_id];
+}
+EXPORT_SYMBOL_NS_GPL(cxl_get_feature_command_opcode, "CXL");
+
 /*
  * Commands that RAW doesn't permit. The rationale for each:
  *
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 2d6f7c87e5e8..451b2c7e995b 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -912,6 +912,7 @@ void cxl_coordinates_combine(struct access_coordinate *out,
 bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
 
 int cxl_get_feature_command_id(u16 opcode);
+u16 cxl_get_feature_command_opcode(int feature_id);
 
 /*
  * Unit test builds overrides this to __weak, find the 'strong' version
diff --git a/drivers/cxl/fwctl.c b/drivers/cxl/fwctl.c
index 85dee01d89d1..39c01ef4dbf2 100644
--- a/drivers/cxl/fwctl.c
+++ b/drivers/cxl/fwctl.c
@@ -35,11 +35,207 @@ static void *cxlctl_info(struct fwctl_uctx *uctx, size_t *length)
 	return info;
 }
 
+static struct cxl_feat_entry *
+get_support_feature_info(struct cxl_features_state *cxlfs,
+			 const struct fwctl_rpc_cxl *rpc_in)
+{
+	struct cxl_feat_entry *feat;
+	uuid_t uuid;
+
+	if (rpc_in->op_size < sizeof(uuid))
+		return ERR_PTR(-EINVAL);
+
+	if (copy_from_user(&uuid, u64_to_user_ptr(rpc_in->in_payload),
+			   sizeof(uuid)))
+		return ERR_PTR(-EFAULT);
+
+	for (int i = 0; i < cxlfs->num_features; i++) {
+		feat = &cxlfs->entries[i];
+		if (uuid_equal(&uuid, &feat->uuid))
+			return feat;
+	}
+
+	return ERR_PTR(-EINVAL);
+}
+
+static void *cxlctl_get_supported_features(struct cxl_features_state *cxlfs,
+					   const struct fwctl_rpc_cxl *rpc_in,
+					   size_t *out_len)
+{
+	struct cxl_mbox_get_sup_feats_out *feat_out;
+	struct cxl_mbox_get_sup_feats_in feat_in;
+	struct cxl_feat_entry *pos;
+	size_t out_size;
+	int requested;
+	u32 count;
+	u16 start;
+	int i;
+
+	if (rpc_in->op_size != sizeof(feat_in))
+		return ERR_PTR(-EINVAL);
+
+	if (copy_from_user(&feat_in, u64_to_user_ptr(rpc_in->in_payload),
+			   rpc_in->op_size))
+		return ERR_PTR(-EFAULT);
+
+	count = le32_to_cpu(feat_in.count);
+	start = le16_to_cpu(feat_in.start_idx);
+	requested = count / sizeof(*pos);
+
+	/*
+	 * Make sure that the total requested number of entries is not greater
+	 * than the total number of supported features allowed for userspace.
+	 */
+	if (start >= cxlfs->num_user_features)
+		return ERR_PTR(-EINVAL);
+
+	requested = min_t(int, requested, cxlfs->num_user_features - start);
+
+	out_size = sizeof(struct fwctl_rpc_cxl_out) +
+		struct_size(feat_out, ents, requested);
+
+	struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) =
+		kvzalloc(out_size, GFP_KERNEL);
+	if (!rpc_out)
+		return ERR_PTR(-ENOMEM);
+
+	rpc_out->size = struct_size(feat_out, ents, requested);
+	feat_out = (struct cxl_mbox_get_sup_feats_out *)rpc_out->payload;
+	if (requested == 0) {
+		feat_out->num_entries = cpu_to_le16(requested);
+		feat_out->supported_feats =
+			cpu_to_le16(cxlfs->num_user_features);
+		rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS;
+		*out_len = out_size;
+		return no_free_ptr(rpc_out);
+	}
+
+	for (i = 0, pos = &feat_out->ents[0];
+	     i < cxlfs->num_features; i++, pos++) {
+		if (i == requested)
+			break;
+
+		memcpy(pos, &cxlfs->entries[i], sizeof(*pos));
+		/*
+		 * If the feature is exclusive, set the set_feat_size to 0 to
+		 * indicate that the feature is not changeable.
+		 */
+		if (is_cxl_feature_exclusive(pos))
+			pos->set_feat_size = 0;
+	}
+
+	feat_out->num_entries = cpu_to_le16(requested);
+	feat_out->supported_feats = cpu_to_le16(cxlfs->num_features);
+	rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS;
+	*out_len = out_size;
+
+	return no_free_ptr(rpc_out);
+}
+
+static bool cxlctl_validate_set_features(struct cxl_features_state *cxlfs,
+					 const struct fwctl_rpc_cxl *rpc_in,
+					 enum fwctl_rpc_scope scope)
+{
+	struct cxl_feat_entry *feat;
+	u16 effects, mask;
+	u32 flags;
+
+	feat = get_support_feature_info(cxlfs, rpc_in);
+	if (IS_ERR(feat))
+		return false;
+
+	/* Ensure that the attribute is changeable */
+	flags = le32_to_cpu(feat->flags);
+	if (!(flags & CXL_FEATURE_F_CHANGEABLE))
+		return false;
+
+	effects = le16_to_cpu(feat->effects);
+
+	/*
+	 * Reserved bits are set, rejecting since the effects is not
+	 * comprehended by the driver.
+	 */
+	if (effects & CXL_CMD_EFFECTS_RESERVED) {
+		dev_warn_once(&cxlfs->cxlmd->dev,
+			      "Reserved bits set in the Feature effects field!\n");
+		return false;
+	}
+
+	/* Currently no user background command support */
+	if (effects & CXL_CMD_BACKGROUND)
+		return false;
+
+	/* Effects cause immediate change, highest security scope is needed */
+	mask = CXL_CMD_CONFIG_CHANGE_IMMEDIATE |
+	       CXL_CMD_DATA_CHANGE_IMMEDIATE |
+	       CXL_CMD_POLICY_CHANGE_IMMEDIATE |
+	       CXL_CMD_LOG_CHANGE_IMMEDIATE;
+	if (effects & mask && scope >= FWCTL_RPC_DEBUG_WRITE_FULL)
+		return true;
+
+	/* These effects supported for all WRITE scope */
+	if ((effects & CXL_CMD_CONFIG_CHANGE_COLD_RESET ||
+	     effects & CXL_CMD_CONFIG_CHANGE_CONV_RESET ||
+	     effects & CXL_CMD_CONFIG_CHANGE_CXL_RESET) &&
+	    scope >= FWCTL_RPC_DEBUG_WRITE)
+		return true;
+
+	return false;
+}
+
+static bool cxlctl_validate_hw_command(struct cxl_features_state *cxlfs,
+				       const struct fwctl_rpc_cxl *rpc_in,
+				       enum fwctl_rpc_scope scope,
+				       u16 opcode)
+{
+	if (!cxlfs->num_features)
+		return false;
+
+	switch (opcode) {
+	case CXL_MBOX_OP_GET_SUPPORTED_FEATURES:
+	case CXL_MBOX_OP_GET_FEATURE:
+		if (cxlfs->cap < CXL_FEATURES_RO)
+			return false;
+		if (scope >= FWCTL_RPC_CONFIGURATION)
+			return true;
+		return false;
+	case CXL_MBOX_OP_SET_FEATURE:
+		return cxlctl_validate_set_features(cxlfs, rpc_in, scope);
+	default:
+		return false;
+	}
+}
+
+static void *cxlctl_handle_commands(struct cxl_features_state *cxlfs,
+				    const struct fwctl_rpc_cxl *rpc_in,
+				    size_t *out_len, u16 opcode)
+{
+	switch (opcode) {
+	case CXL_MBOX_OP_GET_SUPPORTED_FEATURES:
+		return cxlctl_get_supported_features(cxlfs, rpc_in, out_len);
+	case CXL_MBOX_OP_GET_FEATURE:
+	case CXL_MBOX_OP_SET_FEATURE:
+	default:
+		return ERR_PTR(-EOPNOTSUPP);
+	}
+}
+
 static void *cxlctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
 			   void *in, size_t in_len, size_t *out_len)
 {
-	/* Place holder */
-	return ERR_PTR(-EOPNOTSUPP);
+	struct fwctl_device *fwctl = uctx->fwctl;
+	struct cxl_features_state *cxlfs = to_cxl_features_state(fwctl);
+	const struct fwctl_rpc_cxl *rpc_in = in;
+	u16 opcode;
+
+	opcode = cxl_get_feature_command_opcode(rpc_in->command_id);
+	if (opcode == 0xffff)
+		return ERR_PTR(-EINVAL);
+
+	if (!cxlctl_validate_hw_command(cxlfs, rpc_in, scope, opcode))
+		return ERR_PTR(-EINVAL);
+
+	return cxlctl_handle_commands(cxlfs, rpc_in, out_len, opcode);
 }
 
 static const struct fwctl_ops cxlctl_ops = {
diff --git a/include/cxl/features.h b/include/cxl/features.h
index 5c7de465fb68..e52d0573f504 100644
--- a/include/cxl/features.h
+++ b/include/cxl/features.h
@@ -42,14 +42,6 @@
 
 struct cxl_mailbox;
 
-/* Index IDs for CXL mailbox Feature commands */
-enum feature_cmds {
-	CXL_FEATURE_ID_GET_SUPPORTED_FEATURES = 0,
-	CXL_FEATURE_ID_GET_FEATURE,
-	CXL_FEATURE_ID_SET_FEATURE,
-	CXL_FEATURE_ID_MAX
-};
-
 /* Feature commands capability supported by a device */
 enum cxl_features_capability {
 	CXL_FEATURES_NONE = 0,
diff --git a/include/uapi/cxl/features.h b/include/uapi/cxl/features.h
index 2e98efb9abf1..05c8a06a5dff 100644
--- a/include/uapi/cxl/features.h
+++ b/include/uapi/cxl/features.h
@@ -33,6 +33,7 @@ struct cxl_mbox_get_sup_feats_in {
 #define CXL_CMD_EFFECTS_VALID			BIT(9)
 #define CXL_CMD_CONFIG_CHANGE_CONV_RESET	BIT(10)
 #define CXL_CMD_CONFIG_CHANGE_CXL_RESET		BIT(11)
+#define CXL_CMD_EFFECTS_RESERVED		GENMASK(15, 12)
 
 /*
  * CXL spec r3.2 Table 8-109
diff --git a/include/uapi/fwctl/cxl.h b/include/uapi/fwctl/cxl.h
index 4e766ffb1388..3b95e0903856 100644
--- a/include/uapi/fwctl/cxl.h
+++ b/include/uapi/fwctl/cxl.h
@@ -16,4 +16,41 @@
 struct fwctl_info_cxl {
 	__u32 reserved;
 };
+
+/**
+ * struct fwctl_rpc_cxl - ioctl(FWCTL_RPC) input for CXL
+ * @command_id: the defined command id by 'enum feature_cmds'
+ * @flags: Flags for the command (input).
+ * @op_size: Size of input payload.
+ * @reserved1: Reserved. Must be 0s.
+ * @in_payload: User address of the hardware op input structure
+ */
+struct fwctl_rpc_cxl {
+	__u32 command_id;
+	__u32 flags;
+	__u32 op_size;
+	__u32 reserved1;
+	__aligned_u64 in_payload;
+};
+
+/* command_id for CXL mailbox Feature commands */
+enum feature_cmds {
+	CXL_FEATURE_ID_GET_SUPPORTED_FEATURES = 0,
+	CXL_FEATURE_ID_GET_FEATURE,
+	CXL_FEATURE_ID_SET_FEATURE,
+	CXL_FEATURE_ID_MAX
+};
+
+/**
+ * struct fwctl_rpc_cxl_out - ioctl(FWCTL_RPC) output for CXL
+ * @size: Size of the output payload
+ * @retval: Return value from device
+ * @payload: Return data from device
+ */
+struct fwctl_rpc_cxl_out {
+	__u32 size;
+	__u32 retval;
+	__u8 payload[] __counted_by(size);
+};
+
 #endif
-- 
2.48.1

[PATCH v3 12/16] cxl: Add support to handle user feature commands for get feature

[Dave Jiang]

Add helper function to parse the user data from fwctl RPC ioctl and
send the parsed input parameters to cxl_get_feature() call.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/fwctl.c | 46 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/cxl/fwctl.c b/drivers/cxl/fwctl.c
index 39c01ef4dbf2..959845a466ff 100644
--- a/drivers/cxl/fwctl.c
+++ b/drivers/cxl/fwctl.c
@@ -132,6 +132,51 @@ static void *cxlctl_get_supported_features(struct cxl_features_state *cxlfs,
 	return no_free_ptr(rpc_out);
 }
 
+static void *cxlctl_get_feature(struct cxl_features_state *cxlfs,
+				const struct fwctl_rpc_cxl *rpc_in,
+				size_t *out_len)
+{
+	struct cxl_dev_state *cxlds = cxlfs->cxlmd->cxlds;
+	struct cxl_mailbox *cxl_mbox = &cxlds->cxl_mbox;
+	struct cxl_mbox_get_feat_in feat_in;
+	u16 offset, count, return_code;
+	size_t out_size = *out_len;
+
+	if (rpc_in->op_size != sizeof(feat_in))
+		return ERR_PTR(-EINVAL);
+
+	if (copy_from_user(&feat_in, u64_to_user_ptr(rpc_in->in_payload),
+			   rpc_in->op_size))
+		return ERR_PTR(-EFAULT);
+
+	offset = le16_to_cpu(feat_in.offset);
+	count = le16_to_cpu(feat_in.count);
+
+	if (!count)
+		return ERR_PTR(-EINVAL);
+
+	struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) =
+		kvzalloc(out_size, GFP_KERNEL);
+	if (!rpc_out)
+		return ERR_PTR(-ENOMEM);
+
+	out_size = cxl_get_feature(cxl_mbox, &feat_in.uuid,
+				   feat_in.selection, rpc_out->payload,
+				   count, offset, &return_code);
+	*out_len = sizeof(struct fwctl_rpc_cxl_out);
+	if (!out_size) {
+		rpc_out->size = 0;
+		rpc_out->retval = return_code;
+		return no_free_ptr(rpc_out);
+	}
+
+	rpc_out->size = out_size;
+	rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS;
+	*out_len += out_size;
+
+	return no_free_ptr(rpc_out);
+}
+
 static bool cxlctl_validate_set_features(struct cxl_features_state *cxlfs,
 					 const struct fwctl_rpc_cxl *rpc_in,
 					 enum fwctl_rpc_scope scope)
@@ -214,6 +259,7 @@ static void *cxlctl_handle_commands(struct cxl_features_state *cxlfs,
 	case CXL_MBOX_OP_GET_SUPPORTED_FEATURES:
 		return cxlctl_get_supported_features(cxlfs, rpc_in, out_len);
 	case CXL_MBOX_OP_GET_FEATURE:
+		return cxlctl_get_feature(cxlfs, rpc_in, out_len);
 	case CXL_MBOX_OP_SET_FEATURE:
 	default:
 		return ERR_PTR(-EOPNOTSUPP);
-- 
2.48.1

[PATCH v3 13/16] cxl: Add support to handle user feature commands for set feature

[Dave Jiang]

Add helper function to parse the user data from fwctl RPC ioctl and
send the parsed input parameters to cxl_set_feature() call.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- Remove duplicate is_cxl_feature_exclusive() prototype. (Jonathan)
---
 drivers/cxl/core/features.c | 18 ++++++++-----
 drivers/cxl/features.h      |  1 +
 drivers/cxl/fwctl.c         | 53 +++++++++++++++++++++++++++++++++++++
 include/uapi/cxl/features.h |  5 ++++
 4 files changed, 71 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
index 0ab02c2a94f9..5f64185a5c7a 100644
--- a/drivers/cxl/core/features.c
+++ b/drivers/cxl/core/features.c
@@ -20,6 +20,17 @@ static const uuid_t cxl_exclusive_feats[] = {
 	CXL_FEAT_RANK_SPARING_UUID,
 };
 
+bool is_cxl_feature_exclusive_by_uuid(uuid_t *uuid)
+{
+	for (int i = 0; i < ARRAY_SIZE(cxl_exclusive_feats); i++) {
+		if (uuid_equal(uuid, &cxl_exclusive_feats[i]))
+			return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL_NS_GPL(is_cxl_feature_exclusive_by_uuid, "CXL");
+
 /**
  * is_cxl_feature_exclusive() - Check if a CXL feature is exclusive to kernel
  * @entry: cxl feature entry
@@ -28,12 +39,7 @@ static const uuid_t cxl_exclusive_feats[] = {
  */
 bool is_cxl_feature_exclusive(struct cxl_feat_entry *entry)
 {
-	for (int i = 0; i < ARRAY_SIZE(cxl_exclusive_feats); i++) {
-		if (uuid_equal(&entry->uuid, &cxl_exclusive_feats[i]))
-			return true;
-	}
-
-	return false;
+	return is_cxl_feature_exclusive_by_uuid(&entry->uuid);
 }
 EXPORT_SYMBOL_NS_GPL(is_cxl_feature_exclusive, "CXL");
 
diff --git a/drivers/cxl/features.h b/drivers/cxl/features.h
index a51625774b58..81aabdffb60b 100644
--- a/drivers/cxl/features.h
+++ b/drivers/cxl/features.h
@@ -36,5 +36,6 @@ struct cxl_features_state *devm_cxlfs_allocate(struct cxl_memdev *cxlmd);
 void devm_cxlfs_free(struct cxl_memdev *cxlmd);
 int devm_cxl_add_features(struct cxl_memdev *cxlmd);
 bool is_cxl_feature_exclusive(struct cxl_feat_entry *entry);
+bool is_cxl_feature_exclusive_by_uuid(uuid_t *uuid);
 
 #endif
diff --git a/drivers/cxl/fwctl.c b/drivers/cxl/fwctl.c
index 959845a466ff..db6faccc51fb 100644
--- a/drivers/cxl/fwctl.c
+++ b/drivers/cxl/fwctl.c
@@ -177,6 +177,58 @@ static void *cxlctl_get_feature(struct cxl_features_state *cxlfs,
 	return no_free_ptr(rpc_out);
 }
 
+static void *cxlctl_set_feature(struct cxl_features_state *cxlfs,
+				const struct fwctl_rpc_cxl *rpc_in,
+				size_t *out_len)
+{
+	struct cxl_dev_state *cxlds = cxlfs->cxlmd->cxlds;
+	struct cxl_mailbox *cxl_mbox = &cxlds->cxl_mbox;
+	size_t out_size, data_size;
+	u16 offset, return_code;
+	u32 flags;
+	int rc;
+
+	if (rpc_in->op_size <= sizeof(struct cxl_mbox_set_feat_hdr))
+		return ERR_PTR(-EINVAL);
+
+	struct cxl_mbox_set_feat_in *feat_in __free(kvfree) =
+		kvzalloc(rpc_in->op_size, GFP_KERNEL);
+	if (!feat_in)
+		return ERR_PTR(-ENOMEM);
+
+	if (copy_from_user(feat_in, u64_to_user_ptr(rpc_in->in_payload),
+			   rpc_in->op_size))
+		return ERR_PTR(-EFAULT);
+
+	if (is_cxl_feature_exclusive_by_uuid(&feat_in->hdr.uuid))
+		return ERR_PTR(-EPERM);
+
+	offset = le16_to_cpu(feat_in->hdr.offset);
+	flags = le32_to_cpu(feat_in->hdr.flags);
+	out_size = *out_len;
+
+	struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) =
+		kvzalloc(out_size, GFP_KERNEL);
+	if (!rpc_out)
+		return ERR_PTR(-ENOMEM);
+
+	rpc_out->size = 0;
+
+	data_size = rpc_in->op_size - sizeof(feat_in->hdr);
+	rc = cxl_set_feature(cxl_mbox, &feat_in->hdr.uuid,
+			     feat_in->hdr.version, feat_in->data,
+			     data_size, flags, offset, &return_code);
+	if (rc) {
+		rpc_out->retval = return_code;
+		return no_free_ptr(rpc_out);
+	}
+
+	rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS;
+	*out_len = sizeof(*rpc_out);
+
+	return no_free_ptr(rpc_out);
+}
+
 static bool cxlctl_validate_set_features(struct cxl_features_state *cxlfs,
 					 const struct fwctl_rpc_cxl *rpc_in,
 					 enum fwctl_rpc_scope scope)
@@ -261,6 +313,7 @@ static void *cxlctl_handle_commands(struct cxl_features_state *cxlfs,
 	case CXL_MBOX_OP_GET_FEATURE:
 		return cxlctl_get_feature(cxlfs, rpc_in, out_len);
 	case CXL_MBOX_OP_SET_FEATURE:
+		return cxlctl_set_feature(cxlfs, rpc_in, out_len);
 	default:
 		return ERR_PTR(-EOPNOTSUPP);
 	}
diff --git a/include/uapi/cxl/features.h b/include/uapi/cxl/features.h
index 05c8a06a5dff..30138506b286 100644
--- a/include/uapi/cxl/features.h
+++ b/include/uapi/cxl/features.h
@@ -107,6 +107,11 @@ struct cxl_mbox_set_feat_hdr {
 	__u8 rsvd[9];
 } __attribute__ ((__packed__));
 
+struct cxl_mbox_set_feat_in {
+	struct cxl_mbox_set_feat_hdr hdr;
+	__u8 data[];
+} __attribute__ ((__packed__));
+
 /* Set Feature flags field */
 enum cxl_set_feat_flag_data_transfer {
 	CXL_SET_FEAT_FLAG_FULL_DATA_TRANSFER = 0,
-- 
2.48.1

[PATCH v3 14/16] cxl/test: Add Get Feature support to cxl_test

[Dave Jiang]

Add emulation of Get Feature command to cxl_test. The feature for
device patrol scrub is returned by the emulation code. This is
the only feature currently supported by cxl_test. It returns
the information for the device patrol scrub feature.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 tools/testing/cxl/test/mem.c | 57 ++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index 90069dd686b5..aab5905b56c3 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -48,6 +48,10 @@ static struct cxl_cel_entry mock_cel[] = {
 		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_SUPPORTED_FEATURES),
 		.effect = CXL_CMD_EFFECT_NONE,
 	},
+	{
+		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_FEATURE),
+		.effect = CXL_CMD_EFFECT_NONE,
+	},
 	{
 		.opcode = cpu_to_le16(CXL_MBOX_OP_IDENTIFY),
 		.effect = CXL_CMD_EFFECT_NONE,
@@ -149,6 +153,10 @@ struct mock_event_store {
 	u32 ev_status;
 };
 
+struct vendor_test_feat {
+	__le32 data;
+} __packed;
+
 struct cxl_mockmem_data {
 	void *lsa;
 	void *fw;
@@ -165,6 +173,7 @@ struct cxl_mockmem_data {
 	u8 event_buf[SZ_4K];
 	u64 timestamp;
 	unsigned long sanitize_timeout;
+	struct vendor_test_feat test_feat;
 };
 
 static struct mock_event_log *event_find_log(struct device *dev, int log_type)
@@ -1360,8 +1369,47 @@ static void fill_feature_vendor_test(struct cxl_feat_entry *feat)
 				    CXL_CMD_EFFECTS_VALID);
 }
 
+
 #define MAX_CXL_TEST_FEATS	1
 
+static int mock_get_test_feature(struct cxl_mockmem_data *mdata,
+				 struct cxl_mbox_cmd *cmd)
+{
+	struct vendor_test_feat *output = cmd->payload_out;
+	struct cxl_mbox_get_feat_in *input = cmd->payload_in;
+	u16 offset = le16_to_cpu(input->offset);
+	u16 count = le16_to_cpu(input->count);
+	u8 *ptr;
+
+	if (offset > sizeof(*output)) {
+		cmd->return_code = CXL_MBOX_CMD_RC_INPUT;
+		return -EINVAL;
+	}
+
+	if (offset + count > sizeof(*output)) {
+		cmd->return_code = CXL_MBOX_CMD_RC_INPUT;
+		return -EINVAL;
+	}
+
+	ptr = (u8 *)&mdata->test_feat + offset;
+	memcpy((u8 *)output + offset, ptr, count);
+
+	return 0;
+}
+
+static int mock_get_feature(struct cxl_mockmem_data *mdata,
+			    struct cxl_mbox_cmd *cmd)
+{
+	struct cxl_mbox_get_feat_in *input = cmd->payload_in;
+
+	if (uuid_equal(&input->uuid, &CXL_VENDOR_FEATURE_TEST))
+		return mock_get_test_feature(mdata, cmd);
+
+	cmd->return_code = CXL_MBOX_CMD_RC_UNSUPPORTED;
+
+	return -EOPNOTSUPP;
+}
+
 static int mock_get_supported_features(struct cxl_mockmem_data *mdata,
 				       struct cxl_mbox_cmd *cmd)
 {
@@ -1492,6 +1540,9 @@ static int cxl_mock_mbox_send(struct cxl_mailbox *cxl_mbox,
 	case CXL_MBOX_OP_GET_SUPPORTED_FEATURES:
 		rc = mock_get_supported_features(mdata, cmd);
 		break;
+	case CXL_MBOX_OP_GET_FEATURE:
+		rc = mock_get_feature(mdata, cmd);
+		break;
 	default:
 		break;
 	}
@@ -1539,6 +1590,11 @@ static int cxl_mock_mailbox_create(struct cxl_dev_state *cxlds)
 	return 0;
 }
 
+static void cxl_mock_test_feat_init(struct cxl_mockmem_data *mdata)
+{
+	mdata->test_feat.data = cpu_to_le32(0xdeadbeef);
+}
+
 static int cxl_mock_mem_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1626,6 +1682,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 		return rc;
 
 	cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL);
+	cxl_mock_test_feat_init(mdata);
 
 	return 0;
 }
-- 
2.48.1

[PATCH v3 15/16] cxl/test: Add Set Feature support to cxl_test

[Dave Jiang]

Add emulation to support Set Feature mailbox command to cxl_test.
The only feature supported is the device patrol scrub feature. The
set feature allows activation of patrol scrub for the cxl_test
emulated device. The command does not support partial data transfer
even though the spec allows it. This restriction is to reduce complexity
of the emulation given the patrol scrub feature is very minimal.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 tools/testing/cxl/test/mem.c | 50 ++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index aab5905b56c3..53091ba39978 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -52,6 +52,10 @@ static struct cxl_cel_entry mock_cel[] = {
 		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_FEATURE),
 		.effect = CXL_CMD_EFFECT_NONE,
 	},
+	{
+		.opcode = cpu_to_le16(CXL_MBOX_OP_SET_FEATURE),
+		.effect = cpu_to_le16(EFFECT(CONF_CHANGE_IMMEDIATE)),
+	},
 	{
 		.opcode = cpu_to_le16(CXL_MBOX_OP_IDENTIFY),
 		.effect = CXL_CMD_EFFECT_NONE,
@@ -1410,6 +1414,49 @@ static int mock_get_feature(struct cxl_mockmem_data *mdata,
 	return -EOPNOTSUPP;
 }
 
+static int mock_set_test_feature(struct cxl_mockmem_data *mdata,
+				 struct cxl_mbox_cmd *cmd)
+{
+	struct cxl_mbox_set_feat_in *input = cmd->payload_in;
+	struct vendor_test_feat *test = (struct vendor_test_feat *)input->data;
+	u32 action;
+
+	action = FIELD_GET(CXL_SET_FEAT_FLAG_DATA_TRANSFER_MASK,
+			   le32_to_cpu(input->hdr.flags));
+	/*
+	 * While it is spec compliant to support other set actions, it is not
+	 * necessary to add the complication in the emulation currently. Reject
+	 * anything besides full xfer.
+	 */
+	if (action != CXL_SET_FEAT_FLAG_FULL_DATA_TRANSFER) {
+		cmd->return_code = CXL_MBOX_CMD_RC_INPUT;
+		return -EINVAL;
+	}
+
+	/* Offset should be reserved when doing full transfer */
+	if (input->hdr.offset) {
+		cmd->return_code = CXL_MBOX_CMD_RC_INPUT;
+		return -EINVAL;
+	}
+
+	memcpy(&mdata->test_feat.data, &test->data, sizeof(u32));
+
+	return 0;
+}
+
+static int mock_set_feature(struct cxl_mockmem_data *mdata,
+			    struct cxl_mbox_cmd *cmd)
+{
+	struct cxl_mbox_set_feat_in *input = cmd->payload_in;
+
+	if (uuid_equal(&input->hdr.uuid, &CXL_VENDOR_FEATURE_TEST))
+		return mock_set_test_feature(mdata, cmd);
+
+	cmd->return_code = CXL_MBOX_CMD_RC_UNSUPPORTED;
+
+	return -EOPNOTSUPP;
+}
+
 static int mock_get_supported_features(struct cxl_mockmem_data *mdata,
 				       struct cxl_mbox_cmd *cmd)
 {
@@ -1543,6 +1590,9 @@ static int cxl_mock_mbox_send(struct cxl_mailbox *cxl_mbox,
 	case CXL_MBOX_OP_GET_FEATURE:
 		rc = mock_get_feature(mdata, cmd);
 		break;
+	case CXL_MBOX_OP_SET_FEATURE:
+		rc = mock_set_feature(mdata, cmd);
+		break;
 	default:
 		break;
 	}
-- 
2.48.1

[PATCH v3 16/16] fwctl/cxl: Add documentation to FWCTL CXL

[Dave Jiang]

Add policy and operational documentation for FWCTL CXL.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- Add a sentence on how entries are filtered. (Jonathan)
---
 .../userspace-api/fwctl/fwctl-cxl.rst         | 137 ++++++++++++++++++
 Documentation/userspace-api/fwctl/index.rst   |   1 +
 MAINTAINERS                                   |   1 +
 3 files changed, 139 insertions(+)
 create mode 100644 Documentation/userspace-api/fwctl/fwctl-cxl.rst

diff --git a/Documentation/userspace-api/fwctl/fwctl-cxl.rst b/Documentation/userspace-api/fwctl/fwctl-cxl.rst
new file mode 100644
index 000000000000..81c694f6a44d
--- /dev/null
+++ b/Documentation/userspace-api/fwctl/fwctl-cxl.rst
@@ -0,0 +1,137 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+================
+fwctl cxl driver
+================
+
+:Author: Dave Jiang
+
+Overview
+========
+
+The CXL spec defines a set of commands that can be issued to the mailbox of a
+CXL device or switch. It also left room for vendor specific commands to be
+issued to the mailbox as well. fwctl provides a path to issue a set of allowed
+mailbox commands from user space to the device moderated by the kernel driver.
+
+The following 3 commands will be used to support CXL Features:
+CXL spec r3.1 8.2.9.6.1 Get Supported Features (Opcode 0500h)
+CXL spec r3.1 8.2.9.6.2 Get Feature (Opcode 0501h)
+CXL spec r3.1 8.2.9.6.3 Set Feature (Opcode 0502h)
+
+The "Get Supported Features" return data may be filtered by the kernel driver to
+drop any features that are forbidden by the kernel or being exclusively used by
+the kernel. The driver will set the "Set Feature Size" of the "Get Supported
+Features Supported Feature Entry" to 0 to indicate that the Feature cannot be
+modifed. The "Get Supported Features" command and the "Get Features" falls
+under the fwctl policy of FWCTL_RPC_CONFIGURATION.
+
+For "Set Feature" command, the access policy currently is broken down into two
+categories depending on the Set Feature effects reported by the device. If the
+Set Feature will cause immediate change to the device, the fwctl access policy
+must be FWCTL_RPC_DEBUG_WRITE_FULL. The effects for this level are
+"immediate config change", "immediate data change", "immediate policy change",
+or "immediate log change" for the set effects mask. If the effects are "config
+change with cold reset" or "config change with conventional reset", then the
+fwctl access policy must be FWCTL_RPC_DEBUG_WRITE or higher.
+
+fwctl cxl User API
+==================
+
+.. kernel-doc:: include/uapi/fwctl/cxl.h
+.. kernel-doc:: include/uapi/cxl/features.h
+
+1. Driver info query
+--------------------
+
+First step for the app is to issue the ioctl(FWCTL_CMD_INFO). Successful
+invocation of the ioctl implies the Features capability is operational and
+returns an all zeros 32bit payload. A ``struct fwctl_info`` needs to be filled
+out with the ``fwctl_info.out_device_type`` set to ``FWCTL_DEVICE_TYPE_CXL``.
+The return data should be ``struct fwctl_info_cxl`` that contains a reserved
+32bit field that should be all zeros.
+
+2. Send hardware commands
+-------------------------
+
+Next step is to send the 'Get Supported Features' command to the driver from
+user space via ioctl(FWCTL_RPC). A ``struct fwctl_rpc_cxl`` is pointed to
+by ``fwctl_rpc.in``. ``struct fwctl_rpc_cxl.in_payload`` points to
+the hardware input structure that is defined by the CXL spec. ``fwctl_rpc.out``
+points to the buffer that contains a ``struct fwctl_rpc_cxl_out`` that includes
+the hardware output data inlined as ``fwctl_rpc_cxl_out.payload``. This command
+is called twice. First time to retrieve the number of features supported.
+A second time to retrieve the specific feature details as the output data.
+
+After getting the specific feature details, a Get/Set Feature command can be
+appropriately programmed and sent. For a "Set Feature" command, the retrieved
+feature info contains an effects field that details the resulting
+"Set Feature" command will trigger. That will inform the user whether
+the system is configured to allowed the "Set Feature" command or not.
+
+Code example of a Get Feature
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+.. code-block:: c
+
+        static int cxl_fwctl_rpc_get_test_feature(int fd, struct test_feature *feat_ctx,
+                                                  const uint32_t expected_data)
+        {
+                struct cxl_mbox_get_feat_in feat_in __attribute__((aligned(16))) = {0};
+                struct fwctl_rpc_cxl in __attribute__((aligned(16))) = {0};
+                struct fwctl_rpc_cxl_out *out;
+                struct fwctl_rpc rpc = {0};
+                size_t out_size;
+                uint32_t val;
+                void *data;
+                int rc;
+
+                uuid_copy(feat_in.uuid, feat_ctx->uuid);
+                feat_in.count = feat_ctx->get_size;
+
+                out_size = sizeof(*out) + feat_in.count;
+                out = aligned_alloc(16, out_size);
+                if (!out)
+                        return -ENOMEM;
+
+                memset(out, 0, out_size);
+                data = out->payload;
+
+                in.command_id = CXL_FEATURE_ID_GET_FEATURE;
+                in.op_size = sizeof(feat_in);
+                in.in_payload = (uint64_t)(uint64_t *)&feat_in;
+
+                rpc.size = sizeof(rpc);
+                rpc.scope = FWCTL_RPC_CONFIGURATION;
+                rpc.in_len = sizeof(in);
+                rpc.out_len = out_size;
+                rpc.in = (uint64_t)(uint64_t *)&in;
+                rpc.out = (uint64_t)(uint64_t *)out;
+
+                rc = send_command(fd, &rpc, out);
+                if (rc)
+                        goto out;
+
+                val = le32toh(*(__le32 *)data);
+                if (memcmp(&val, &expected_data, sizeof(val)) != 0) {
+                        rc = -ENXIO;
+                        goto out;
+                }
+
+        out:
+                free(out);
+                return rc;
+        }
+
+It is recommended to take a look at CXL CLI test directory
+<https://github.com/pmem/ndctl/tree/main/test/fwctl.c> for a detailed user code
+example on how to exercise this path.
+
+
+fwctl cxl Kernel API
+====================
+
+.. kernel-doc:: drivers/cxl/core/features.c
+.. kernel-doc:: drivers/cxl/features.c
+   :export:
+.. kernel-doc:: include/cxl/features.h
diff --git a/Documentation/userspace-api/fwctl/index.rst b/Documentation/userspace-api/fwctl/index.rst
index 06959fbf1547..d9d40a468a31 100644
--- a/Documentation/userspace-api/fwctl/index.rst
+++ b/Documentation/userspace-api/fwctl/index.rst
@@ -10,3 +10,4 @@ to securely construct and execute RPCs inside device firmware.
    :maxdepth: 1
 
    fwctl
+   fwctl-cxl
diff --git a/MAINTAINERS b/MAINTAINERS
index 877eb301b1e5..5d67225a22bc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5773,6 +5773,7 @@ M:	Dan Williams <dan.j.williams@intel.com>
 L:	linux-cxl@vger.kernel.org
 S:	Maintained
 F:	Documentation/driver-api/cxl
+F:	Documentation/userspace-api/fwctl/fwctl-cxl.rst
 F:	drivers/cxl/
 F:	include/cxl/
 F:	include/uapi/linux/cxl_mem.h
-- 
2.48.1

Re: [PATCH v3 02/16] cxl: Enumerate feature commands

[Dan Williams]

Dave Jiang wrote:
> Add feature commands enumeration code in order to detect and enumerate
> the 3 feature related commands "get supported features", "get feature",
> and "set feature". The enumeration will help determine whether the driver
> can issue any of the 3 commands to the device.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v3:
> - Add comma to enum entries that are not end entry. (Jonathan)
> - Pull devm_cxfs_allocate() forward from patch 8. (Jonathan)
> - Return NULL for failure path of allocate function. (Jonathan)
> ---
>  drivers/cxl/Makefile     |  2 +-
>  drivers/cxl/core/mbox.c  | 30 +++++++++++++
>  drivers/cxl/cxl.h        |  2 +
>  drivers/cxl/cxlmem.h     |  5 +++
>  drivers/cxl/features.c   | 91 ++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/features.h   |  8 ++++
>  drivers/cxl/mem.c        |  5 +++
>  include/cxl/features.h   | 35 ++++++++++++++++
>  include/cxl/mailbox.h    |  2 +
>  tools/testing/cxl/Kbuild |  2 +-
>  10 files changed, 180 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/cxl/features.c
>  create mode 100644 drivers/cxl/features.h
>  create mode 100644 include/cxl/features.h
> 
> diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
> index 2caa90fa4bf2..12fbc35081bb 100644
> --- a/drivers/cxl/Makefile
> +++ b/drivers/cxl/Makefile
> @@ -17,5 +17,5 @@ obj-$(CONFIG_CXL_PCI) += cxl_pci.o
>  cxl_port-y := port.o
>  cxl_acpi-y := acpi.o
>  cxl_pmem-y := pmem.o security.o
> -cxl_mem-y := mem.o
> +cxl_mem-y := mem.o features.o

Wait, why is this unconditionally added to the mem driver?

I would expect that all of this functionality would be optionally built
into the cxl_core. I *think* all of this can go into a conditionally
compiled into a drivers/cxl/core/features.c, but let me keep this in
mind as I go through the rest.

>  cxl_pci-y := pci.o
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index bdb8f060f2c1..9fe552e3d465 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -69,6 +69,29 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>  	CXL_CMD(GET_TIMESTAMP, 0, 0x8, 0),
>  };
>  
> +static u16 cxl_feature_commands[] = {
> +	CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
> +	CXL_MBOX_OP_GET_FEATURE,
> +	CXL_MBOX_OP_SET_FEATURE,
> +};

My only nit would be to call this "cxl_feature_opcodes" and
s/command/opcode/ throughout the following functions. The "command" term
is burned for the Linux command numbers / ioctl numbers.

> +
> +/**
> + * cxl_get_feature_command_id() - Get the index id for a feature command
> + * @opcode: The device opcode for the feature command
> + *
> + * Return the index id on success or -errno on failure
> + */
> +int cxl_get_feature_command_id(u16 opcode)
> +{
> +	for (int i = 0; i < ARRAY_SIZE(cxl_feature_commands); i++) {
> +		if (cxl_feature_commands[i] == opcode)
> +			return i;
> +	}
> +
> +	return -ENOENT;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_get_feature_command_id, "CXL");

This export potentially goes away in a world where Feature support is
self contained to drivers/cxl/core/features.c.

> +
>  /*
>   * Commands that RAW doesn't permit. The rationale for each:
>   *
> @@ -734,6 +757,13 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>  		if (cmd) {
>  			set_bit(cmd->info.id, cxl_mbox->enabled_cmds);
>  			enabled++;
> +		} else {
> +			int fid = cxl_get_feature_command_id(opcode);
> +
> +			if (fid >= 0) {
> +				set_bit(fid, cxl_mbox->feature_cmds);
> +				enabled++;

See enumerate_feature_cmds() comment below, I think @feature_cmds could
live on the stack.

> +			}
>  		}
>  
>  		if (cxl_is_poison_command(opcode)) {
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index f6015f24ad38..2d6f7c87e5e8 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -911,6 +911,8 @@ void cxl_coordinates_combine(struct access_coordinate *out,
>  
>  bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
>  
> +int cxl_get_feature_command_id(u16 opcode);
> +
>  /*
>   * Unit test builds overrides this to __weak, find the 'strong' version
>   * of these symbols in tools/testing/cxl/.
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index a0a49809cd76..4a42cdb64b5c 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -39,6 +39,7 @@
>   * @dev: driver core device object
>   * @cdev: char dev core object for ioctl operations
>   * @cxlds: The device state backing this device
> + * @cxlfs: The features state for the device
>   * @detach_work: active memdev lost a port in its ancestry
>   * @cxl_nvb: coordinate removal of @cxl_nvd if present
>   * @cxl_nvd: optional bridge to an nvdimm if the device supports pmem
> @@ -50,6 +51,7 @@ struct cxl_memdev {
>  	struct device dev;
>  	struct cdev cdev;
>  	struct cxl_dev_state *cxlds;
> +	struct cxl_features_state *cxlfs;

I would feel more comfortable if all memdev operational state was
retrieved through @cxlds.

Something like:

struct cxl_dev_state {
...
#ifdef CONFIG_CXL_FEATURES
	struct cxl_feature_state *cxlfs;
#endif
};


#ifdef CONFIG_CXL_FEATURES
static inline struct cxl_features_state *to_cxlfs(struct cxl_dev_state *cxlds)
{
	return cxlds->cxlfs;
}
#else
static inline struct cxl_features_state *to_cxlfs(struct cxl_dev_state *cxlds)
{
	return NULL;
}
#endif



>  	struct work_struct detach_work;
>  	struct cxl_nvdimm_bridge *cxl_nvb;
>  	struct cxl_nvdimm *cxl_nvd;
> @@ -490,6 +492,9 @@ enum cxl_opcode {
>  	CXL_MBOX_OP_GET_LOG_CAPS	= 0x0402,
>  	CXL_MBOX_OP_CLEAR_LOG           = 0x0403,
>  	CXL_MBOX_OP_GET_SUP_LOG_SUBLIST = 0x0405,
> +	CXL_MBOX_OP_GET_SUPPORTED_FEATURES	= 0x0500,
> +	CXL_MBOX_OP_GET_FEATURE		= 0x0501,
> +	CXL_MBOX_OP_SET_FEATURE		= 0x0502,
>  	CXL_MBOX_OP_IDENTIFY		= 0x4000,
>  	CXL_MBOX_OP_GET_PARTITION_INFO	= 0x4100,
>  	CXL_MBOX_OP_SET_PARTITION_INFO	= 0x4101,
> diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
> new file mode 100644
> index 000000000000..958e4828a58d
> --- /dev/null
> +++ b/drivers/cxl/features.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
> +#include <linux/device.h>
> +#include <cxl/mailbox.h>
> +#include <cxl/features.h>
> +#include "cxl.h"
> +#include "cxlmem.h"
> +#include "features.h"
> +
> +static void enumerate_feature_cmds(struct cxl_memdev *cxlmd,
> +				   struct cxl_features_state *cxlfs)
> +{
> +	struct cxl_mailbox *cxl_mbox = &cxlmd->cxlds->cxl_mbox;
> +	int fid;
> +
> +	fid = cxl_get_feature_command_id(CXL_MBOX_OP_GET_SUPPORTED_FEATURES);
> +	if (!test_bit(fid, cxl_mbox->feature_cmds))
> +		return;
> +
> +	fid = cxl_get_feature_command_id(CXL_MBOX_OP_GET_FEATURE);
> +	if (!test_bit(fid, cxl_mbox->feature_cmds))
> +		return;
> +
> +	cxlfs->cap = CXL_FEATURES_RO;
> +
> +	fid = cxl_get_feature_command_id(CXL_MBOX_OP_SET_FEATURE);
> +	if (!test_bit(fid, cxl_mbox->feature_cmds))
> +		return;
> +
> +	cxlfs->cap = CXL_FEATURES_RW;

If at all possible if the mbox can just house these caps directly and
skip the ->feature_cmds indirection that would be my preference.
Otherwise this is burning space in cxl_mbox that never gets used post
init.

> +}
> +
> +static void cxlfs_free(void *_cxlfs)
> +{
> +	kfree(_cxlfs);
> +}
> +DEFINE_FREE(free_cxlfs, struct cxl_features_state *, if (_T) cxlfs_free(_T))

Does this grow more complicated in follow-on patches, because this is
identical to __free(kfree)?

> +static struct cxl_features_state *devm_cxlfs_allocate(struct cxl_memdev *cxlmd)
> +{
> +	int rc;
> +
> +	struct cxl_features_state *cxlfs __free(free_cxlfs) =
> +		kzalloc(sizeof(*cxlfs), GFP_KERNEL);
> +	if (!cxlfs)
> +		return NULL;
> +
> +	cxlfs->cxlmd = cxlmd;
> +
> +	rc = devm_add_action_or_reset(&cxlmd->dev, cxlfs_free, cxlfs);
> +	if (rc)
> +		return NULL;

No, this is a double-free bug as when devm_add_action_or_reset() fails
it has already executed cxlfs_free(), and then the return NULL triggers
another kfree(cxlfs);

This would need to be something like:

    rc = devm_add_action_or_reset(&cxlmd->dev, cxlfs_free, no_free_ptr(cxlfs));
    if (rc)
        return NULL;

...but I really do not see the point because there are no error exits in
this state of the function that need scoped-based release. I would wait
to add scope-based handling hear until the patch actually needs it.

> +
> +	return no_free_ptr(cxlfs);
> +}
> +
> +static void devm_cxlfs_free(struct cxl_memdev *cxlmd)
> +{
> +	kfree(cxlmd->cxlfs);
> +	/* Set in devm_cxl_add_features(), make sure it's cleared */
> +	cxlmd->cxlfs = NULL;
> +}
> +
> +static void cxl_cxlfs_free(void *_cxlfs)
> +{
> +	struct cxl_features_state *cxlfs = _cxlfs;
> +
> +	devm_cxlfs_free(cxlfs->cxlmd);
> +}
> +DEFINE_FREE(cxl_free_cxlfs, struct cxl_features_state *, if (_T) cxl_cxlfs_free(_T))

devm and scope-based release are difficult to mix because it makes it
exceedingly difficult to think through the state of the pointer relative
to the state of the devm action and the device.

This needs explicit hand off points between scope-based world and devm
world and would usually be via patterns like above
"devm_add_action_or_reset(..., no_free_ptr(@obj))". Like above,
scope-based is not doing anything useful in this path.

> +
> +/**
> + * devm_cxl_add_features() - Allocate and initialize features context
> + * @cxlmd: CXL memory device
> + *
> + * Return 0 on success or -errno on failure.
> + */
> +int devm_cxl_add_features(struct cxl_memdev *cxlmd)
> +{
> +	struct cxl_features_state *cxlfs __free(cxl_free_cxlfs) =
> +		devm_cxlfs_allocate(cxlmd);
> +	if (!cxlfs)
> +		return -ENOMEM;
> +
> +	enumerate_feature_cmds(cxlmd, cxlfs);
> +
> +	cxlmd->cxlfs = no_free_ptr(cxlfs);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_add_features, "CXL");
> diff --git a/drivers/cxl/features.h b/drivers/cxl/features.h
> new file mode 100644
> index 000000000000..0cc6d9e6c441
> --- /dev/null
> +++ b/drivers/cxl/features.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright(c) 2025 Intel Corporation. */
> +#ifndef __CXL_FEATURES_LOCAL__
> +#define __CXL_FEATURES_LOCAL__
> +
> +int devm_cxl_add_features(struct cxl_memdev *cxlmd);
> +
> +#endif
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 2f03a4d5606e..47348a52bc05 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -7,6 +7,7 @@
>  
>  #include "cxlmem.h"
>  #include "cxlpci.h"
> +#include "features.h"
>  
>  /**
>   * DOC: cxl mem
> @@ -180,6 +181,10 @@ static int cxl_mem_probe(struct device *dev)
>  			return rc;
>  	}
>  
> +	rc = devm_cxl_add_features(cxlmd);
> +	if (rc)
> +		dev_dbg(dev, "No CXL Features enumerated.\n");
> +

I think features want to be enabled via the cxl_pci driver not cxl_mem.
This is for the same reason that ioctls and firmware upload are
initialized from cxl_pci, device repair scenarios.

If the device is failing media_ready for example, and a Set Feature
command could repair it, or a Get Feature command could diagnose it, then
you do not want to be dependent on the device successfully connecting to
the CXL topology to get access to Features.

>  	/*
>  	 * The kernel may be operating out of CXL memory on this device,
>  	 * there is no spec defined way to determine whether this device
> diff --git a/include/cxl/features.h b/include/cxl/features.h
> new file mode 100644
> index 000000000000..8e1830f0ccba
> --- /dev/null
> +++ b/include/cxl/features.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright(c) 2024-2025 Intel Corporation. */
> +#ifndef __CXL_FEATURES_H__
> +#define __CXL_FEATURES_H__
> +
> +struct cxl_mailbox;
> +
> +/* Index IDs for CXL mailbox Feature commands */
> +enum feature_cmds {
> +	CXL_FEATURE_ID_GET_SUPPORTED_FEATURES = 0,
> +	CXL_FEATURE_ID_GET_FEATURE,
> +	CXL_FEATURE_ID_SET_FEATURE,
> +	CXL_FEATURE_ID_MAX
> +};

These are unused in this patch, I don't think we need them, right? The
code can just work with opcodes exclusively. Maybe this becomes clearer
in later patches...

Re: [PATCH v3 03/16] cxl: Add Get Supported Features command for kernel usage

[Dan Williams]

Dave Jiang wrote:
> CXL spec r3.2 8.2.9.6.1 Get Supported Features (Opcode 0500h)
> The command retrieve the list of supported device-specific features
> (identified by UUID) and general information about each Feature.
> 
> The driver will retrieve the feature entries in order to make checks and
> provide information for the Get Feature and Set Feature command. One of
> the main piece of information retrieved are the effects a Set Feature
> command would have for a particular feature.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v3:
> - Use struct_size() instead of open coding the calculation. (Jonathan)
> ---
>  drivers/cxl/features.c | 138 +++++++++++++++++++++++++++++++++++++++++
>  include/cxl/features.h |  57 +++++++++++++++++
>  2 files changed, 195 insertions(+)
> 
> diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
> index 958e4828a58d..4d8c603121f0 100644
> --- a/drivers/cxl/features.c
> +++ b/drivers/cxl/features.c
> @@ -7,6 +7,139 @@
>  #include "cxlmem.h"
>  #include "features.h"
>  
> +static void cxl_free_feature_entries(void *entries)
> +{
> +	kvfree(entries);
> +}
> +
> +static int cxl_get_supported_features_count(struct cxl_mailbox *cxl_mbox)
> +{
[..]
> +
> +	cxlfs->num_features = count;
> +	cxlfs->entries = no_free_ptr(entries);
> +	return devm_add_action_or_reset(&cxlmd->dev, cxl_free_feature_entries,
> +					cxlfs->entries);

This is a memory leak. At this point cxlfs->entries is NULL having been
marked NULL by no_free_ptr().

It is awkward to mix a return code, a return buffer, and a return
features count all at the same time near a devm handoff. I would suggest
solving it by:

1/ skip calling devm_add_action_or_reset() inside of
   get_supported_features(), make the caller responsible for devm
   wrapping any allocations.

2/ Convey the error code and the object via an ERR_PTR() from
   get_supported_features()

3/ Move "num_features" into the allocation for @entries. Something like:

struct cxl_features_state {
     struct cxl_memdev *cxlmd;
     enum cxl_features_capability cap;
     struct cxl_feat_entries {
         int num_features;
         struct cxl_feat_entry ent[] __counted_by(num_features);
     } *entries;
};

...and then have get_supported_features() return a 'struct
cxl_feat_entries *' pointer.

> +}
> +
>  static void enumerate_feature_cmds(struct cxl_memdev *cxlmd,
>  				   struct cxl_features_state *cxlfs)
>  {
> @@ -77,12 +210,17 @@ DEFINE_FREE(cxl_free_cxlfs, struct cxl_features_state *, if (_T) cxl_cxlfs_free(
>   */
>  int devm_cxl_add_features(struct cxl_memdev *cxlmd)
>  {
> +	int rc;
> +
>  	struct cxl_features_state *cxlfs __free(cxl_free_cxlfs) =
>  		devm_cxlfs_allocate(cxlmd);

devm and scope-based should never appear in the same statement.

>  	if (!cxlfs)
>  		return -ENOMEM;
>  
>  	enumerate_feature_cmds(cxlmd, cxlfs);
> +	rc = get_supported_features(cxlmd, cxlfs);
> +	if (rc)
> +		return rc;
>  
>  	cxlmd->cxlfs = no_free_ptr(cxlfs);
>  
> diff --git a/include/cxl/features.h b/include/cxl/features.h
> index 8e1830f0ccba..c65760342f97 100644
> --- a/include/cxl/features.h
> +++ b/include/cxl/features.h
> @@ -3,6 +3,8 @@
>  #ifndef __CXL_FEATURES_H__
>  #define __CXL_FEATURES_H__
>  
> +#include <linux/uuid.h>
> +
>  struct cxl_mailbox;
>  
>  /* Index IDs for CXL mailbox Feature commands */
> @@ -25,11 +27,66 @@ enum cxl_features_capability {
>   * @cxlmd: Pointer to cxl mem device
>   * @cap: Feature commands capability
>   * @num_features: total Features supported by the device
> + * @entries: Feature detail entries fetched from the device
>   */
>  struct cxl_features_state {
>  	struct cxl_memdev *cxlmd;
>  	enum cxl_features_capability cap;
>  	int num_features;
> +	struct cxl_feat_entry *entries;

Per above, this organization results in some buggy gymanistics.

Re: [PATCH v3 05/16] cxl/mbox: Add GET_FEATURE mailbox command

[Dan Williams]

Dave Jiang wrote:
> From: Shiju Jose <shiju.jose@huawei.com>
> 
> Add support for GET_FEATURE mailbox command.
> 
> CXL spec r3.2 section 8.2.9.6 describes optional device specific features.
> The settings of a feature can be retrieved using Get Feature command.
> CXL spec r3.2 section 8.2.9.6.2 describes Get Feature command.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/Makefile   |  1 +
>  drivers/cxl/core/features.c | 59 +++++++++++++++++++++++++++++++++++++
>  include/cxl/features.h      | 29 ++++++++++++++++++
>  tools/testing/cxl/Kbuild    |  1 +
>  4 files changed, 90 insertions(+)
>  create mode 100644 drivers/cxl/core/features.c
> 
> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> index 9259bcc6773c..73b6348afd67 100644
> --- a/drivers/cxl/core/Makefile
> +++ b/drivers/cxl/core/Makefile
> @@ -14,5 +14,6 @@ cxl_core-y += pci.o
>  cxl_core-y += hdm.o
>  cxl_core-y += pmu.o
>  cxl_core-y += cdat.o
> +cxl_core-y += features.o

Would have expected:

cxl_core-$(CONFIG_CXL_FEATURES) += features.o

I think it is ok to throw all of Feature and FWCTL support behind a
single config option. That is, until it becomes clear that someone has a
"yes, kernel internal CXL Features, no FWCTL CXL Features" use case.

>  cxl_core-$(CONFIG_TRACING) += trace.o
>  cxl_core-$(CONFIG_CXL_REGION) += region.o
> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> new file mode 100644
> index 000000000000..b01dc5ebb24d
> --- /dev/null
> +++ b/drivers/cxl/core/features.c
> @@ -0,0 +1,59 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
> +#include <linux/device.h>
> +#include <cxl/mailbox.h>
> +#include <cxl/features.h>
> +#include "cxl.h"
> +#include "core.h"
> +#include "cxlmem.h"
> +#include "features.h"
> +
> +size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
> +		       enum cxl_get_feat_selection selection,
> +		       void *feat_out, size_t feat_out_size, u16 offset,
> +		       u16 *return_code)
> +{
> +	size_t data_to_rd_size, size_out;
> +	struct cxl_mbox_get_feat_in pi;
> +	struct cxl_mbox_cmd mbox_cmd;
> +	size_t data_rcvd_size = 0;
> +	int rc;
> +
> +	if (return_code)
> +		*return_code = CXL_MBOX_CMD_RC_INPUT;
> +
> +	if (!feat_out || !feat_out_size)
> +		return 0;
> +
> +	size_out = min(feat_out_size, cxl_mbox->payload_size);
> +	uuid_copy(&pi.uuid, feat_uuid);
> +	pi.selection = selection;
> +	do {
> +		data_to_rd_size = min(feat_out_size - data_rcvd_size,
> +				      cxl_mbox->payload_size);
> +		pi.offset = cpu_to_le16(offset + data_rcvd_size);
> +		pi.count = cpu_to_le16(data_to_rd_size);
> +
> +		mbox_cmd = (struct cxl_mbox_cmd) {
> +			.opcode = CXL_MBOX_OP_GET_FEATURE,
> +			.size_in = sizeof(pi),
> +			.payload_in = &pi,
> +			.size_out = size_out,
> +			.payload_out = feat_out + data_rcvd_size,
> +			.min_out = data_to_rd_size,
> +		};
> +		rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
> +		if (rc < 0 || !mbox_cmd.size_out) {
> +			if (return_code)
> +				*return_code = mbox_cmd.return_code;
> +			return 0;
> +		}
> +		data_rcvd_size += mbox_cmd.size_out;
> +	} while (data_rcvd_size < feat_out_size);
> +
> +	if (return_code)
> +		*return_code = CXL_MBOX_CMD_RC_SUCCESS;
> +
> +	return data_rcvd_size;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_get_feature, "CXL");

So I see this is exported to a new cxl_fwtcl.ko module, but I think all
of this can be core built-in functionality similar to memdev ioctl and
firmware upload support. As long as distributions can opt-out of FWCTL
and Features at build time then this is no worse than
CONFIG_CXL_MEM_RAW_COMMANDS from a proprietary use case / security model
stance. With that software only needs to worry about finding a
cxl_memdev object and not manually loading a cxl_fwctl module.

Re: [PATCH v3 08/16] cxl: Add FWCTL support to the CXL memdev driver

[Dan Williams]

Dave Jiang wrote:
> Add fwctl support code to allow sending of CXL feature commands from
> userspace through as ioctls via FWCTL. Provide initial setup bits.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v3:
> - Fix spelling error in Kconfig text
> - Remove errant blank line
> - Drop using of __weak. (Jason)
> - Return NULL for allocate function. (Jonathan)
> ---
>  drivers/cxl/Kconfig        | 15 +++++++
>  drivers/cxl/Makefile       |  1 +
>  drivers/cxl/features.c     |  8 +++-
>  drivers/cxl/features.h     | 29 +++++++++++++
>  drivers/cxl/fwctl.c        | 89 ++++++++++++++++++++++++++++++++++++++
>  include/cxl/features.h     |  3 ++
>  include/uapi/fwctl/fwctl.h |  1 +
>  tools/testing/cxl/Kbuild   |  1 +
>  8 files changed, 145 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/cxl/fwctl.c
> 
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 876469e23f7a..6bb1ffc74956 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -102,6 +102,21 @@ config CXL_MEM
>  
>  	  If unsure say 'm'.
>  
> +config CXL_FWCTL
> +	bool "CXL: Firmware Control support"
> +	depends on CXL_MEM
> +	select FWCTL
> +	help
> +	  Enable support for firmware control of CXL devices. This allows
> +	  a FWCTL char device to be created in order to issue ioctls from
> +	  user space to exercise the CXL Features commands supported by
> +	  a CXL mailbox. There are some Features defined in the CXL spec
> +	  such as memory sparing. However a lot of those features are
> +	  exclusive to the kernel. Most likely the Features manipulated
> +	  by CXL_FWCTL will be vendor defined Features.
> +
> +	  If unsure say 'n'.

Per previous comments, combine this with the CONFIG_CXL_FEATURES config
option and treat it all as one for now.

> +
>  config CXL_PORT
>  	default CXL_BUS
>  	tristate
> diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
> index 12fbc35081bb..cd28fb3bf855 100644
> --- a/drivers/cxl/Makefile
> +++ b/drivers/cxl/Makefile
> @@ -18,4 +18,5 @@ cxl_port-y := port.o
>  cxl_acpi-y := acpi.o
>  cxl_pmem-y := pmem.o security.o
>  cxl_mem-y := mem.o features.o
> +cxl_mem-$(CONFIG_CXL_FWCTL) += fwctl.o
>  cxl_pci-y := pci.o
> diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
> index 0086fdb20bfd..c14dc6d45985 100644
> --- a/drivers/cxl/features.c
> +++ b/drivers/cxl/features.c
> @@ -1,6 +1,8 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
> +#include <linux/pci.h>
>  #include <linux/device.h>
> +#include <linux/module.h>
>  #include <cxl/mailbox.h>
>  #include <cxl/features.h>
>  #include "cxl.h"
> @@ -173,7 +175,7 @@ static void cxlfs_free(void *_cxlfs)
>  }
>  DEFINE_FREE(free_cxlfs, struct cxl_features_state *, if (_T) cxlfs_free(_T))
>  
> -static struct cxl_features_state *devm_cxlfs_allocate(struct cxl_memdev *cxlmd)
> +struct cxl_features_state *_devm_cxlfs_allocate(struct cxl_memdev *cxlmd)
>  {
>  	int rc;
>  
> @@ -191,7 +193,7 @@ static struct cxl_features_state *devm_cxlfs_allocate(struct cxl_memdev *cxlmd)
>  	return no_free_ptr(cxlfs);
>  }
>  
> -static void devm_cxlfs_free(struct cxl_memdev *cxlmd)
> +void _devm_cxlfs_free(struct cxl_memdev *cxlmd)
>  {
>  	kfree(cxlmd->cxlfs);
>  	/* Set in devm_cxl_add_features(), make sure it's cleared */
> @@ -231,3 +233,5 @@ int devm_cxl_add_features(struct cxl_memdev *cxlmd)
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_cxl_add_features, "CXL");
> +
> +MODULE_IMPORT_NS("CXL");

What symbols does features.c need to import? I think this all gets
simpler with the proposal to move all of this to cxl_core.ko optional
features.o object.

> diff --git a/drivers/cxl/features.h b/drivers/cxl/features.h
> index 8706f944b476..a51625774b58 100644
> --- a/drivers/cxl/features.h
> +++ b/drivers/cxl/features.h
> @@ -3,8 +3,37 @@
>  #ifndef __CXL_FEATURES_LOCAL__
>  #define __CXL_FEATURES_LOCAL__
>  
> +struct cxl_features_state *_devm_cxlfs_allocate(struct cxl_memdev *cxlmd);
> +void _devm_cxlfs_free(struct cxl_memdev *cxlmd);
> +
> +#ifdef CONFIG_CXL_FWCTL
> +struct cxl_features_state *_devm_cxlfs_fwctl_allocate(struct cxl_memdev *cxlmd);
> +void _devm_cxlfs_fwctl_free(struct cxl_memdev *cxlmd);
> +#endif
> +
> +static inline struct cxl_features_state *
> +devm_cxlfs_allocate(struct cxl_memdev *cxlmd)
> +{
> +#ifdef CONFIG_CXL_FWCTL
> +	return _devm_cxlfs_fwctl_allocate(cxlmd);
> +#else
> +	return _devm_cxlfs_allocate(cxlmd);
> +#endif
> +}
> +
> +static inline void devm_cxlfs_free(struct cxl_memdev *cxlmd)
> +{
> +#ifdef CONFIG_CXL_FWCTL
> +	_devm_cxlfs_fwctl_free(cxlmd);
> +#else
> +	_devm_cxlfs_free(cxlmd);
> +#endif
> +}

This looks odd. Lets just do an approach like this from cxl_pci:

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index b2c943a4de0a..47d432e34ccf 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1002,6 +1002,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
        if (rc)
                return rc;
 
+       rc = devm_cxl_setup_features(&pdev->dev, cxlds);
+       if (rc)
+               return rc;
+
        cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlds);
        if (IS_ERR(cxlmd))
                return PTR_ERR(cxlmd);
@@ -1010,6 +1014,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
        if (rc)
                return rc;
 
+       rc = devm_cxl_setup_fwctl(&pdev->dev, cxlmd);
+       if (rc)
+               return rc;
+
        rc = devm_cxl_sanitize_setup_notifier(&pdev->dev, cxlmd);
        if (rc)
                return rc;

...where Feature command functionality is ready to be used by kernel
internal paths prior to registering a memdev, in support of use cases
like EDAC memory repair. Then fwctl is registered after the memdev so it
can reference that object.

Then the header file looks like a more typical:

#ifdef CONFIG_CXL_FEATURES
int devm_cxl_setup_features(struct device *host, struct cxl_dev_state *cxlds);
int devm_cxl_setup_fwctl(struct device *host, struct cxl_memdev *cxlmd);
#else
static inline int devm_cxl_setup_features(struct device *host, struct cxl_dev_state *cxlds)
{
	return 0;
}
static inline int devm_cxl_setup_fwctl(struct device *host, struct cxl_memdev *cxlmd)
{
	return 0;
}
#endif

> +
>  struct cxl_feat_entry;
>  
> +struct cxl_features_state *devm_cxlfs_allocate(struct cxl_memdev *cxlmd);
> +void devm_cxlfs_free(struct cxl_memdev *cxlmd);
>  int devm_cxl_add_features(struct cxl_memdev *cxlmd);
>  bool is_cxl_feature_exclusive(struct cxl_feat_entry *entry);
>  
> diff --git a/drivers/cxl/fwctl.c b/drivers/cxl/fwctl.c
> new file mode 100644
> index 000000000000..985315d5d503
> --- /dev/null
> +++ b/drivers/cxl/fwctl.c
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2025 Intel Corporation. All rights reserved. */
> +#include <linux/fwctl.h>
> +#include <linux/device.h>
> +#include <cxl/features.h>
> +#include "cxlmem.h"
> +#include "features.h"
> +
> +static int cxlctl_open_uctx(struct fwctl_uctx *uctx)
> +{
> +	return 0;
> +}
> +
> +static void cxlctl_close_uctx(struct fwctl_uctx *uctx)
> +{
> +}
> +
> +static void *cxlctl_info(struct fwctl_uctx *uctx, size_t *length)
> +{
> +	/* Place holder */
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static void *cxlctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
> +			   void *in, size_t in_len, size_t *out_len)
> +{
> +	/* Place holder */
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static const struct fwctl_ops cxlctl_ops = {
> +	.device_type = FWCTL_DEVICE_TYPE_CXL,
> +	.uctx_size = sizeof(struct fwctl_uctx),
> +	.open_uctx = cxlctl_open_uctx,
> +	.close_uctx = cxlctl_close_uctx,
> +	.info = cxlctl_info,
> +	.fw_rpc = cxlctl_fw_rpc,
> +};
> +
> +static void remove_cxlfs(void *_cxlfs)
> +{
> +	struct cxl_features_state *cxlfs = _cxlfs;
> +	struct cxl_memdev *cxlmd = cxlfs->cxlmd;
> +	struct fwctl_device *fwctl = &cxlfs->fwctl;
> +
> +	/* Set in devm_cxl_add_features(), make sure it's cleared */
> +	cxlmd->cxlfs = NULL;
> +	fwctl_unregister(fwctl);
> +	fwctl_put(fwctl);
> +}
> +
> +DEFINE_FREE(free_cxlfs, struct cxl_features_state *, if (_T) fwctl_put(&_T->fwctl))
> +
> +static struct cxl_features_state *
> +__devm_cxlfs_fwctl_allocate(struct cxl_memdev *cxlmd, const struct fwctl_ops *ops)

This is more than "allocate" it also registers ABI.

> +{
> +	struct device *dev = &cxlmd->dev;
> +	int rc;
> +
> +	struct cxl_features_state *cxlfs __free(free_cxlfs) =
> +		fwctl_alloc_device(dev, ops, struct cxl_features_state, fwctl);
> +	if (!cxlfs)
> +		return NULL;
> +
> +	cxlfs->cxlmd = cxlmd;
> +	rc = fwctl_register(&cxlfs->fwctl);
> +	if (rc)
> +		return NULL;
> +
> +	rc = devm_add_action_or_reset(dev, remove_cxlfs, cxlfs);
> +	if (rc)
> +		return NULL;

Same double free bug here, i.e. needs to be a handoff to the devm
domain:

	rc = devm_add_action_or_reset(dev, remove_cxlfs, no_free_ptr(cxlfs));

...but this also might be another case of a function that wants to
move the devm action responsibility to the caller.

Re: [PATCH v3 09/16] cxl: Add support for FWCTL get driver information callback

[Dan Williams]

Dave Jiang wrote:
> Add definition for fwctl_ops->info() to return driver information. The
> function will return a data structure with a reserved u32. This is setup
> for future usages.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v2:
> - Set command_info to reserved. (Dan)
> ---
>  drivers/cxl/fwctl.c      | 18 ++++++++++++++++--
>  include/cxl/mailbox.h    |  1 +
>  include/uapi/fwctl/cxl.h | 19 +++++++++++++++++++
>  3 files changed, 36 insertions(+), 2 deletions(-)
>  create mode 100644 include/uapi/fwctl/cxl.h
> 
> diff --git a/drivers/cxl/fwctl.c b/drivers/cxl/fwctl.c
> index 985315d5d503..85dee01d89d1 100644
> --- a/drivers/cxl/fwctl.c
> +++ b/drivers/cxl/fwctl.c
> @@ -6,6 +6,8 @@
>  #include "cxlmem.h"
>  #include "features.h"
>  
> +#define to_cxl_features_state(fwctl) container_of(fwctl, struct cxl_features_state, fwctl)
> +
>  static int cxlctl_open_uctx(struct fwctl_uctx *uctx)
>  {
>  	return 0;
> @@ -17,8 +19,20 @@ static void cxlctl_close_uctx(struct fwctl_uctx *uctx)
>  
>  static void *cxlctl_info(struct fwctl_uctx *uctx, size_t *length)
>  {
> -	/* Place holder */
> -	return ERR_PTR(-EOPNOTSUPP);
> +	struct fwctl_device *fwctl = uctx->fwctl;
> +	struct cxl_features_state *cxlfs = to_cxl_features_state(fwctl);
> +	struct fwctl_info_cxl *info;
> +
> +	if (!cxlfs->num_user_features)
> +		return ERR_PTR(-EOPNOTSUPP);

You could save an error case by not registering the fwctl interface at
all when ->num_features == 0.

> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return ERR_PTR(-ENOMEM);
> +
> +	*length = sizeof(*info);
> +
> +	return info;
>  }
>  
>  static void *cxlctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
> diff --git a/include/cxl/mailbox.h b/include/cxl/mailbox.h
> index 99d3bbe1ba2a..bbbbff3af61f 100644
> --- a/include/cxl/mailbox.h
> +++ b/include/cxl/mailbox.h
> @@ -4,6 +4,7 @@
>  #define __CXL_MBOX_H__
>  #include <linux/rcuwait.h>
>  #include <cxl/features.h>
> +#include <uapi/fwctl/cxl.h>
>  #include <uapi/linux/cxl_mem.h>
>  
>  /**
> diff --git a/include/uapi/fwctl/cxl.h b/include/uapi/fwctl/cxl.h
> new file mode 100644
> index 000000000000..4e766ffb1388
> --- /dev/null
> +++ b/include/uapi/fwctl/cxl.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (c) 2024, Intel Corporation
> + *
> + * These are definitions for the mailbox command interface of CXL subsystem.
> + */
> +#ifndef _UAPI_FWCTL_CXL_H_
> +#define _UAPI_FWCTL_CXL_H_
> +
> +#include <linux/types.h>
> +
> +/**
> + * struct fwctl_info_cxl - ioctl(FWCTL_INFO) out_device_data
> + * @reserved: Place older for future usages

ignoring the s/older/holder/, I would just document this as:

	@reserved: zero

...and not tease a future.

Re: [PATCH v3 11/16] cxl: Add support for fwctl RPC command to enable CXL feature commands

[Dan Williams]

Dave Jiang wrote:
> fwctl provides a fwctl_ops->fw_rpc() callback in order to issue ioctls
> to a device. The cxl fwctl driver will start by supporting the CXL
> feature commands: Get Supported Features, Get Feature, and Set Feature.
> 
> The fw_rpc() callback provides 'enum fwctl_rpc_scope' parameter where
> it indicates the security scope of the call. The Get Supported Features
> and Get Feature calls can be executed with the scope of
> FWCTL_RPC_CONFIGRATION. The Set Feature call is gated by the effects
> of the feature reported by Get Supported Features call for the specific
> feature.
> 
> Only "get supported features" is supported in this patch. Additional
> commands will be added in follow on patches. "Get supported features"
> will filter the features that are exclusive to the kernel and only
> report out features that are not kernel only.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v3:
> - Cleanup for loop initializers. (Jonathan)
> - Move is_cxl_feature_exclusive() prototype. (Jonathan)
> - Move cxlctl_fwc_rpc() input param name change. (Jonathan)
> - Use struct_size() for calculation. (Jonathan)
> ---
>  drivers/cxl/core/mbox.c     |   9 ++
>  drivers/cxl/cxl.h           |   1 +
>  drivers/cxl/fwctl.c         | 200 +++++++++++++++++++++++++++++++++++-
>  include/cxl/features.h      |   8 --
>  include/uapi/cxl/features.h |   1 +
>  include/uapi/fwctl/cxl.h    |  37 +++++++
>  6 files changed, 246 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 9fe552e3d465..d38a5fc1384f 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -92,6 +92,15 @@ int cxl_get_feature_command_id(u16 opcode)
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_get_feature_command_id, "CXL");
>  
> +u16 cxl_get_feature_command_opcode(int feature_id)
> +{
> +	if (feature_id >= ARRAY_SIZE(cxl_feature_commands))
> +		return 0xffff;
> +
> +	return cxl_feature_commands[feature_id];
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_get_feature_command_opcode, "CXL");

This can move to drivers/cxl/core/features.c, and it looks like
@feature_id and @cxl_feature_commands are misnamed.

This should be named in terms of fwctl command id to CXL opcode, right?


> +
>  /*
>   * Commands that RAW doesn't permit. The rationale for each:
>   *
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 2d6f7c87e5e8..451b2c7e995b 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -912,6 +912,7 @@ void cxl_coordinates_combine(struct access_coordinate *out,
>  bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
>  
>  int cxl_get_feature_command_id(u16 opcode);
> +u16 cxl_get_feature_command_opcode(int feature_id);
>  
>  /*
>   * Unit test builds overrides this to __weak, find the 'strong' version
> diff --git a/drivers/cxl/fwctl.c b/drivers/cxl/fwctl.c
> index 85dee01d89d1..39c01ef4dbf2 100644
> --- a/drivers/cxl/fwctl.c
> +++ b/drivers/cxl/fwctl.c
> @@ -35,11 +35,207 @@ static void *cxlctl_info(struct fwctl_uctx *uctx, size_t *length)
>  	return info;
>  }
>  
> +static struct cxl_feat_entry *
> +get_support_feature_info(struct cxl_features_state *cxlfs,
> +			 const struct fwctl_rpc_cxl *rpc_in)
> +{
> +	struct cxl_feat_entry *feat;
> +	uuid_t uuid;
> +
> +	if (rpc_in->op_size < sizeof(uuid))
> +		return ERR_PTR(-EINVAL);
> +
> +	if (copy_from_user(&uuid, u64_to_user_ptr(rpc_in->in_payload),
> +			   sizeof(uuid)))
> +		return ERR_PTR(-EFAULT);
> +
> +	for (int i = 0; i < cxlfs->num_features; i++) {
> +		feat = &cxlfs->entries[i];
> +		if (uuid_equal(&uuid, &feat->uuid))
> +			return feat;
> +	}
> +
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +static void *cxlctl_get_supported_features(struct cxl_features_state *cxlfs,
> +					   const struct fwctl_rpc_cxl *rpc_in,
> +					   size_t *out_len)
> +{
> +	struct cxl_mbox_get_sup_feats_out *feat_out;
> +	struct cxl_mbox_get_sup_feats_in feat_in;
> +	struct cxl_feat_entry *pos;
> +	size_t out_size;
> +	int requested;
> +	u32 count;
> +	u16 start;
> +	int i;
> +
> +	if (rpc_in->op_size != sizeof(feat_in))
> +		return ERR_PTR(-EINVAL);
> +
> +	if (copy_from_user(&feat_in, u64_to_user_ptr(rpc_in->in_payload),
> +			   rpc_in->op_size))
> +		return ERR_PTR(-EFAULT);
> +
> +	count = le32_to_cpu(feat_in.count);
> +	start = le16_to_cpu(feat_in.start_idx);
> +	requested = count / sizeof(*pos);
> +
> +	/*
> +	 * Make sure that the total requested number of entries is not greater
> +	 * than the total number of supported features allowed for userspace.
> +	 */
> +	if (start >= cxlfs->num_user_features)
> +		return ERR_PTR(-EINVAL);
> +
> +	requested = min_t(int, requested, cxlfs->num_user_features - start);
> +
> +	out_size = sizeof(struct fwctl_rpc_cxl_out) +
> +		struct_size(feat_out, ents, requested);
> +
> +	struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) =
> +		kvzalloc(out_size, GFP_KERNEL);
> +	if (!rpc_out)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rpc_out->size = struct_size(feat_out, ents, requested);
> +	feat_out = (struct cxl_mbox_get_sup_feats_out *)rpc_out->payload;
> +	if (requested == 0) {
> +		feat_out->num_entries = cpu_to_le16(requested);
> +		feat_out->supported_feats =
> +			cpu_to_le16(cxlfs->num_user_features);
> +		rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS;
> +		*out_len = out_size;
> +		return no_free_ptr(rpc_out);
> +	}
> +
> +	for (i = 0, pos = &feat_out->ents[0];
> +	     i < cxlfs->num_features; i++, pos++) {
> +		if (i == requested)
> +			break;
> +
> +		memcpy(pos, &cxlfs->entries[i], sizeof(*pos));
> +		/*
> +		 * If the feature is exclusive, set the set_feat_size to 0 to
> +		 * indicate that the feature is not changeable.
> +		 */
> +		if (is_cxl_feature_exclusive(pos))
> +			pos->set_feat_size = 0;
> +	}
> +
> +	feat_out->num_entries = cpu_to_le16(requested);
> +	feat_out->supported_feats = cpu_to_le16(cxlfs->num_features);
> +	rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS;
> +	*out_len = out_size;
> +
> +	return no_free_ptr(rpc_out);
> +}
> +
> +static bool cxlctl_validate_set_features(struct cxl_features_state *cxlfs,
> +					 const struct fwctl_rpc_cxl *rpc_in,
> +					 enum fwctl_rpc_scope scope)
> +{
> +	struct cxl_feat_entry *feat;
> +	u16 effects, mask;
> +	u32 flags;
> +
> +	feat = get_support_feature_info(cxlfs, rpc_in);
> +	if (IS_ERR(feat))
> +		return false;
> +
> +	/* Ensure that the attribute is changeable */
> +	flags = le32_to_cpu(feat->flags);
> +	if (!(flags & CXL_FEATURE_F_CHANGEABLE))
> +		return false;
> +
> +	effects = le16_to_cpu(feat->effects);
> +
> +	/*
> +	 * Reserved bits are set, rejecting since the effects is not
> +	 * comprehended by the driver.
> +	 */
> +	if (effects & CXL_CMD_EFFECTS_RESERVED) {
> +		dev_warn_once(&cxlfs->cxlmd->dev,
> +			      "Reserved bits set in the Feature effects field!\n");
> +		return false;
> +	}
> +
> +	/* Currently no user background command support */
> +	if (effects & CXL_CMD_BACKGROUND)
> +		return false;
> +
> +	/* Effects cause immediate change, highest security scope is needed */
> +	mask = CXL_CMD_CONFIG_CHANGE_IMMEDIATE |
> +	       CXL_CMD_DATA_CHANGE_IMMEDIATE |
> +	       CXL_CMD_POLICY_CHANGE_IMMEDIATE |
> +	       CXL_CMD_LOG_CHANGE_IMMEDIATE;
> +	if (effects & mask && scope >= FWCTL_RPC_DEBUG_WRITE_FULL)
> +		return true;
> +
> +	/* These effects supported for all WRITE scope */
> +	if ((effects & CXL_CMD_CONFIG_CHANGE_COLD_RESET ||
> +	     effects & CXL_CMD_CONFIG_CHANGE_CONV_RESET ||
> +	     effects & CXL_CMD_CONFIG_CHANGE_CXL_RESET) &&
> +	    scope >= FWCTL_RPC_DEBUG_WRITE)
> +		return true;

Jonathan and I came to the realization that bit[0,9:11] do not matter
because if they are not set there is no effect, and if they are set the
write is still allowed because the effect is post-reset.

The safety check that can be added is that if a Set has no immediate nor
post-reset effect then it is probably a device bug. In that case err on
the side of caution. I.e. all Set Feature must have at least one
immediate or reset effect.

Re: [PATCH v3 16/16] fwctl/cxl: Add documentation to FWCTL CXL

[Dan Williams]

Dave Jiang wrote:
> Add policy and operational documentation for FWCTL CXL.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v3:
> - Add a sentence on how entries are filtered. (Jonathan)
> ---
>  .../userspace-api/fwctl/fwctl-cxl.rst         | 137 ++++++++++++++++++
>  Documentation/userspace-api/fwctl/index.rst   |   1 +
>  MAINTAINERS                                   |   1 +
>  3 files changed, 139 insertions(+)
>  create mode 100644 Documentation/userspace-api/fwctl/fwctl-cxl.rst
> 
> diff --git a/Documentation/userspace-api/fwctl/fwctl-cxl.rst b/Documentation/userspace-api/fwctl/fwctl-cxl.rst
> new file mode 100644
> index 000000000000..81c694f6a44d
> --- /dev/null
> +++ b/Documentation/userspace-api/fwctl/fwctl-cxl.rst
> @@ -0,0 +1,137 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +================
> +fwctl cxl driver
> +================
> +
> +:Author: Dave Jiang
[..]

Thanks for the verb tense changes!

> +
> +Code example of a Get Feature
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +.. code-block:: c

Nice, thanks for the example, it finally allows me to put a finger on a
naming nit below...

> +
> +        static int cxl_fwctl_rpc_get_test_feature(int fd, struct test_feature *feat_ctx,
> +                                                  const uint32_t expected_data)
> +        {
> +                struct cxl_mbox_get_feat_in feat_in __attribute__((aligned(16))) = {0};
> +                struct fwctl_rpc_cxl in __attribute__((aligned(16))) = {0};
> +                struct fwctl_rpc_cxl_out *out;
> +                struct fwctl_rpc rpc = {0};
> +                size_t out_size;
> +                uint32_t val;
> +                void *data;
> +                int rc;
> +
> +                uuid_copy(feat_in.uuid, feat_ctx->uuid);
> +                feat_in.count = feat_ctx->get_size;
> +
> +                out_size = sizeof(*out) + feat_in.count;
> +                out = aligned_alloc(16, out_size);
> +                if (!out)
> +                        return -ENOMEM;
> +
> +                memset(out, 0, out_size);
> +                data = out->payload;
> +
> +                in.command_id = CXL_FEATURE_ID_GET_FEATURE;

So this should probably be CXL_FWCTL_GET_FEATURE, right? There's no such
thing as a feature command id to userspace only FWCTL command ids that
translate to one of Get Suppported, Get, and Set Feature(s), and the
"id" is implied.

> +                in.op_size = sizeof(feat_in);
> +                in.in_payload = (uint64_t)(uint64_t *)&feat_in;
> +
> +                rpc.size = sizeof(rpc);
> +                rpc.scope = FWCTL_RPC_CONFIGURATION;
> +                rpc.in_len = sizeof(in);
> +                rpc.out_len = out_size;
> +                rpc.in = (uint64_t)(uint64_t *)&in;
> +                rpc.out = (uint64_t)(uint64_t *)out;
> +
> +                rc = send_command(fd, &rpc, out);
> +                if (rc)
> +                        goto out;
> +
> +                val = le32toh(*(__le32 *)data);
> +                if (memcmp(&val, &expected_data, sizeof(val)) != 0) {
> +                        rc = -ENXIO;
> +                        goto out;
> +                }
> +
> +        out:
> +                free(out);
> +                return rc;
> +        }
> +
> +It is recommended to take a look at CXL CLI test directory

Passive -> imperative:

s/It is recommended to take/Take/

Other than that, this looks good!

Re: [PATCH v3 01/16] cxl: Refactor user ioctl command path from mds to mailbox

[Jonathan Cameron]

On Tue, 4 Feb 2025 15:03:02 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> With 'struct cxl_mailbox' context introduced, the helper functions
> cxl_query_cmd() and cxl_send_cmd() can take a cxl_mailbox directly
> rather than a cxl_memdev parameter. Refactor to use cxl_mailbox
> directly.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---

Dave, maybe it's worth nibbling this one off the front of the series
and applying? 

Jonathan

Re: [PATCH v3 01/16] cxl: Refactor user ioctl command path from mds to mailbox

[Dave Jiang]

On 2/5/25 10:41 AM, Jonathan Cameron wrote:
> On Tue, 4 Feb 2025 15:03:02 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> With 'struct cxl_mailbox' context introduced, the helper functions
>> cxl_query_cmd() and cxl_send_cmd() can take a cxl_mailbox directly
>> rather than a cxl_memdev parameter. Refactor to use cxl_mailbox
>> directly.
>>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
> 
> Dave, maybe it's worth nibbling this one off the front of the series
> and applying? 

Probably not a bad idea. I'll do that. 

> 
> Jonathan

Re: [PATCH v3 05/16] cxl/mbox: Add GET_FEATURE mailbox command

[Jonathan Cameron]

On Tue, 4 Feb 2025 16:54:43 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Dave Jiang wrote:
> > From: Shiju Jose <shiju.jose@huawei.com>
> > 
> > Add support for GET_FEATURE mailbox command.
> > 
> > CXL spec r3.2 section 8.2.9.6 describes optional device specific features.
> > The settings of a feature can be retrieved using Get Feature command.
> > CXL spec r3.2 section 8.2.9.6.2 describes Get Feature command.
> > 
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > ---
> >  drivers/cxl/core/Makefile   |  1 +
> >  drivers/cxl/core/features.c | 59 +++++++++++++++++++++++++++++++++++++
> >  include/cxl/features.h      | 29 ++++++++++++++++++
> >  tools/testing/cxl/Kbuild    |  1 +
> >  4 files changed, 90 insertions(+)
> >  create mode 100644 drivers/cxl/core/features.c
> > 
> > diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> > index 9259bcc6773c..73b6348afd67 100644
> > --- a/drivers/cxl/core/Makefile
> > +++ b/drivers/cxl/core/Makefile
> > @@ -14,5 +14,6 @@ cxl_core-y += pci.o
> >  cxl_core-y += hdm.o
> >  cxl_core-y += pmu.o
> >  cxl_core-y += cdat.o
> > +cxl_core-y += features.o  
> 
> Would have expected:
> 
> cxl_core-$(CONFIG_CXL_FEATURES) += features.o
> 
> I think it is ok to throw all of Feature and FWCTL support behind a
> single config option. That is, until it becomes clear that someone has a
> "yes, kernel internal CXL Features, no FWCTL CXL Features" use case.
> 
> >  cxl_core-$(CONFIG_TRACING) += trace.o
> >  cxl_core-$(CONFIG_CXL_REGION) += region.o
> > diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> > new file mode 100644
> > index 000000000000..b01dc5ebb24d
> > --- /dev/null
> > +++ b/drivers/cxl/core/features.c
> > @@ -0,0 +1,59 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
> > +#include <linux/device.h>
> > +#include <cxl/mailbox.h>
> > +#include <cxl/features.h>
> > +#include "cxl.h"
> > +#include "core.h"
> > +#include "cxlmem.h"
> > +#include "features.h"
> > +
> > +size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
> > +		       enum cxl_get_feat_selection selection,
> > +		       void *feat_out, size_t feat_out_size, u16 offset,
> > +		       u16 *return_code)
> > +{
> > +	size_t data_to_rd_size, size_out;
> > +	struct cxl_mbox_get_feat_in pi;
> > +	struct cxl_mbox_cmd mbox_cmd;
> > +	size_t data_rcvd_size = 0;
> > +	int rc;
> > +
> > +	if (return_code)
> > +		*return_code = CXL_MBOX_CMD_RC_INPUT;
> > +
> > +	if (!feat_out || !feat_out_size)
> > +		return 0;
> > +
> > +	size_out = min(feat_out_size, cxl_mbox->payload_size);
> > +	uuid_copy(&pi.uuid, feat_uuid);
> > +	pi.selection = selection;
> > +	do {
> > +		data_to_rd_size = min(feat_out_size - data_rcvd_size,
> > +				      cxl_mbox->payload_size);
> > +		pi.offset = cpu_to_le16(offset + data_rcvd_size);
> > +		pi.count = cpu_to_le16(data_to_rd_size);
> > +
> > +		mbox_cmd = (struct cxl_mbox_cmd) {
> > +			.opcode = CXL_MBOX_OP_GET_FEATURE,
> > +			.size_in = sizeof(pi),
> > +			.payload_in = &pi,
> > +			.size_out = size_out,
> > +			.payload_out = feat_out + data_rcvd_size,
> > +			.min_out = data_to_rd_size,
> > +		};
> > +		rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
> > +		if (rc < 0 || !mbox_cmd.size_out) {
> > +			if (return_code)
> > +				*return_code = mbox_cmd.return_code;
> > +			return 0;
> > +		}
> > +		data_rcvd_size += mbox_cmd.size_out;
> > +	} while (data_rcvd_size < feat_out_size);
> > +
> > +	if (return_code)
> > +		*return_code = CXL_MBOX_CMD_RC_SUCCESS;
> > +
> > +	return data_rcvd_size;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_get_feature, "CXL");  
> 
> So I see this is exported to a new cxl_fwtcl.ko module, but I think all
> of this can be core built-in functionality similar to memdev ioctl and
> firmware upload support. As long as distributions can opt-out of FWCTL
> and Features at build time then this is no worse than
> CONFIG_CXL_MEM_RAW_COMMANDS from a proprietary use case / security model
> stance. With that software only needs to worry about finding a
> cxl_memdev object and not manually loading a cxl_fwctl module.

It'll get exported shortly anyway as it's used by the EDAC series
and that should be separate modules.  

I'm not disagreeing that it might not be needed for fwctl though
and obviously we can add the exports in that series (subject to
yet more versions as this changes...)


> 

Re: [PATCH v3 05/16] cxl/mbox: Add GET_FEATURE mailbox command

[Dan Williams]

Jonathan Cameron wrote:
[..]
> > So I see this is exported to a new cxl_fwtcl.ko module, but I think all
> > of this can be core built-in functionality similar to memdev ioctl and
> > firmware upload support. As long as distributions can opt-out of FWCTL
> > and Features at build time then this is no worse than
> > CONFIG_CXL_MEM_RAW_COMMANDS from a proprietary use case / security model
> > stance. With that software only needs to worry about finding a
> > cxl_memdev object and not manually loading a cxl_fwctl module.
> 
> It'll get exported shortly anyway as it's used by the EDAC series
> and that should be separate modules.  

What additional modules are needed for EDAC support? The registration is
done by the cxl_mem and cxl_region drivers. Are you thinking of a pure
helper library module to the CXL core?

Does it really matter if it's a separate module if cxl_core.ko is going
to demand load it always?

Re: [PATCH v3 05/16] cxl/mbox: Add GET_FEATURE mailbox command

[Jonathan Cameron]

On Wed, 5 Feb 2025 15:12:16 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> [..]
> > > So I see this is exported to a new cxl_fwtcl.ko module, but I think all
> > > of this can be core built-in functionality similar to memdev ioctl and
> > > firmware upload support. As long as distributions can opt-out of FWCTL
> > > and Features at build time then this is no worse than
> > > CONFIG_CXL_MEM_RAW_COMMANDS from a proprietary use case / security model
> > > stance. With that software only needs to worry about finding a
> > > cxl_memdev object and not manually loading a cxl_fwctl module.  
> > 
> > It'll get exported shortly anyway as it's used by the EDAC series
> > and that should be separate modules.    
> 
> What additional modules are needed for EDAC support? The registration is
> done by the cxl_mem and cxl_region drivers. Are you thinking of a pure
> helper library module to the CXL core?
> 
> Does it really matter if it's a separate module if cxl_core.ko is going
> to demand load it always?

I've lost track of all the reorganizing.  Might indeed not be needed
because it is wrapped up other calls that are in the cxl core.

I'll let Shiju figure that out when rebasing!  Note the two sets may
well race as I don't want to hold the discussion on remaining 
controversial bits of that set whilst Dave refactors this one.
The need to eventually rebase on this doesn't stop the EDAC core bits
going forwards.

Jonathan

Re: [PATCH v3 03/16] cxl: Add Get Supported Features command for kernel usage

[Li Ming]

On 2/5/2025 6:03 AM, Dave Jiang wrote:
> CXL spec r3.2 8.2.9.6.1 Get Supported Features (Opcode 0500h)
> The command retrieve the list of supported device-specific features
> (identified by UUID) and general information about each Feature.
>
> The driver will retrieve the feature entries in order to make checks and
> provide information for the Get Feature and Set Feature command. One of
> the main piece of information retrieved are the effects a Set Feature
> command would have for a particular feature.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v3:
> - Use struct_size() instead of open coding the calculation. (Jonathan)
> ---
>  drivers/cxl/features.c | 138 +++++++++++++++++++++++++++++++++++++++++
>  include/cxl/features.h |  57 +++++++++++++++++
>  2 files changed, 195 insertions(+)
>
> diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
> index 958e4828a58d..4d8c603121f0 100644
> --- a/drivers/cxl/features.c
> +++ b/drivers/cxl/features.c
> @@ -7,6 +7,139 @@
[...]
> +static int get_supported_features(struct cxl_memdev *cxlmd,
> +				  struct cxl_features_state *cxlfs)
> +{
> +	int remain_feats, max_size, max_feats, start, rc, hdr_size;
> +	struct cxl_mailbox *cxl_mbox = &cxlmd->cxlds->cxl_mbox;
> +	int feat_size = sizeof(struct cxl_feat_entry);
> +	struct cxl_mbox_get_sup_feats_in mbox_in;
> +	struct cxl_feat_entry *entry;
> +	struct cxl_mbox_cmd mbox_cmd;
> +	int count;
> +
> +	if (cxlfs->cap < CXL_FEATURES_RO)
> +		return -EOPNOTSUPP;
> +
> +	count = cxl_get_supported_features_count(cxl_mbox);
> +	if (count == 0)
> +		return -ENOENT;
> +	if (count < 0)
> +		return -ENXIO;
> +
> +	struct cxl_feat_entry *entries __free(kvfree) =
> +		kvmalloc(count * sizeof(*entries), GFP_KERNEL);
> +	if (!entries)
> +		return -ENOMEM;
> +
> +	struct cxl_mbox_get_sup_feats_out *mbox_out __free(kvfree) =
> +		kvmalloc(cxl_mbox->payload_size, GFP_KERNEL);
> +	if (!mbox_out)
> +		return -ENOMEM;
> +
> +	hdr_size = struct_size(mbox_out, ents, 0);
> +	max_size = cxl_mbox->payload_size - hdr_size;
> +	/* max feat entries that can fit in mailbox max payload size */
> +	max_feats = max_size / feat_size;
> +	entry = entries;
> +
> +	start = 0;
> +	remain_feats = count;
> +	do {
> +		int retrieved, alloc_size, copy_feats;
> +		int num_entries;
> +
> +		if (remain_feats > max_feats) {
> +			alloc_size = struct_size(mbox_out, ents, max_feats);
> +			remain_feats = remain_feats - max_feats;
> +			copy_feats = max_feats;
> +		} else {
> +			alloc_size = struct_size(mbox_out, ents, remain_feats);
> +			copy_feats = remain_feats;
> +			remain_feats = 0;
> +		}
> +
> +		memset(&mbox_in, 0, sizeof(mbox_in));
> +		mbox_in.count = cpu_to_le32(alloc_size);
> +		mbox_in.start_idx = cpu_to_le16(start);
> +		memset(mbox_out, 0, alloc_size);
> +		mbox_cmd = (struct cxl_mbox_cmd) {
> +			.opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
> +			.size_in = sizeof(mbox_in),
> +			.payload_in = &mbox_in,
> +			.size_out = alloc_size,
> +			.payload_out = mbox_out,
> +			.min_out = hdr_size,
> +		};
> +		rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
> +		if (rc < 0)
> +			return rc;
> +
> +		if (mbox_cmd.size_out <= hdr_size)
> +			return -ENXIO;
> +
> +		/*
> +		 * Make sure retrieved out buffer is multiple of feature
> +		 * entries.
> +		 */
> +		retrieved = mbox_cmd.size_out - hdr_size;
> +		if (retrieved % feat_size)
> +			return -ENXIO;
> +
> +		num_entries = le16_to_cpu(mbox_out->num_entries);
> +		/*
> +		 * If the reported output entries * defined entry size !=
> +		 * retrieved output bytes, then the output package is incorrect.
> +		 */
> +		if (num_entries * feat_size != retrieved)
> +			return -ENXIO;
> +
> +		memcpy(entry, mbox_out->ents, retrieved);
> +		entry++;

This memcpy() copies multiple entries from mbox_out->ents to entry, this ''entry++" should be "entry += num_entries"?


Ming

> +		/*
> +		 * If the number of output entries is less than expected, add the
> +		 * remaining entries to the next batch.
> +		 */
> +		remain_feats += copy_feats - num_entries;
> +		start += num_entries;
> +	} while (remain_feats);
> +
> +	cxlfs->num_features = count;
> +	cxlfs->entries = no_free_ptr(entries);
> +	return devm_add_action_or_reset(&cxlmd->dev, cxl_free_feature_entries,
> +					cxlfs->entries);
> +}
> +

Re: [PATCH v3 01/16] cxl: Refactor user ioctl command path from mds to mailbox

[Li Ming]

On 2/5/2025 6:03 AM, Dave Jiang wrote:
> With 'struct cxl_mailbox' context introduced, the helper functions
> cxl_query_cmd() and cxl_send_cmd() can take a cxl_mailbox directly
> rather than a cxl_memdev parameter. Refactor to use cxl_mailbox
> directly.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Reviewed-by: Li Ming <ming.li@zohomail.com>


[snip]

Re: [PATCH v3 04/16] cxl/test: Add Get Supported Features mailbox command support

[Li Ming]

On 2/5/2025 6:03 AM, Dave Jiang wrote:
> Add cxl-test emulation of Get Supported Features mailbox command.
> Currently only adding a test feature with feature identifier of
> all f's for testing.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Acked-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Reviewed-by: Li Ming <ming.li@zohomail.com>

Re: [PATCH v3 05/16] cxl/mbox: Add GET_FEATURE mailbox command

[Li Ming]

On 2/5/2025 6:03 AM, Dave Jiang wrote:
> From: Shiju Jose <shiju.jose@huawei.com>
>
> Add support for GET_FEATURE mailbox command.
>
> CXL spec r3.2 section 8.2.9.6 describes optional device specific features.
> The settings of a feature can be retrieved using Get Feature command.
> CXL spec r3.2 section 8.2.9.6.2 describes Get Feature command.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>

Re: [PATCH v3 05/16] cxl/mbox: Add GET_FEATURE mailbox command

[Dan Williams]

Jonathan Cameron wrote:
> On Wed, 5 Feb 2025 15:12:16 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Jonathan Cameron wrote:
> > [..]
> > > > So I see this is exported to a new cxl_fwtcl.ko module, but I think all
> > > > of this can be core built-in functionality similar to memdev ioctl and
> > > > firmware upload support. As long as distributions can opt-out of FWCTL
> > > > and Features at build time then this is no worse than
> > > > CONFIG_CXL_MEM_RAW_COMMANDS from a proprietary use case / security model
> > > > stance. With that software only needs to worry about finding a
> > > > cxl_memdev object and not manually loading a cxl_fwctl module.  
> > > 
> > > It'll get exported shortly anyway as it's used by the EDAC series
> > > and that should be separate modules.    
> > 
> > What additional modules are needed for EDAC support? The registration is
> > done by the cxl_mem and cxl_region drivers. Are you thinking of a pure
> > helper library module to the CXL core?
> > 
> > Does it really matter if it's a separate module if cxl_core.ko is going
> > to demand load it always?
> 
> I've lost track of all the reorganizing.  Might indeed not be needed
> because it is wrapped up other calls that are in the cxl core.
> 
> I'll let Shiju figure that out when rebasing!  Note the two sets may
> well race as I don't want to hold the discussion on remaining 
> controversial bits of that set whilst Dave refactors this one.
> The need to eventually rebase on this doesn't stop the EDAC core bits
> going forwards.

Given the EDAC entanglements and ongoing discussions about FWCTL, I
suggest we merge the common bits to a shared topic branch.

It is not clear to me that either consumer, EDAC / FWCTL, can clearly
claim to be ahead of the other, but I do think we land the common bits
to get that part settled while the rest plays out.

Re: [PATCH v3 03/16] cxl: Add Get Supported Features command for kernel usage

[Dave Jiang]

On 2/6/25 10:42 PM, Li Ming wrote:
> On 2/5/2025 6:03 AM, Dave Jiang wrote:
>> CXL spec r3.2 8.2.9.6.1 Get Supported Features (Opcode 0500h)
>> The command retrieve the list of supported device-specific features
>> (identified by UUID) and general information about each Feature.
>>
>> The driver will retrieve the feature entries in order to make checks and
>> provide information for the Get Feature and Set Feature command. One of
>> the main piece of information retrieved are the effects a Set Feature
>> command would have for a particular feature.
>>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v3:
>> - Use struct_size() instead of open coding the calculation. (Jonathan)
>> ---
>>  drivers/cxl/features.c | 138 +++++++++++++++++++++++++++++++++++++++++
>>  include/cxl/features.h |  57 +++++++++++++++++
>>  2 files changed, 195 insertions(+)
>>
>> diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
>> index 958e4828a58d..4d8c603121f0 100644
>> --- a/drivers/cxl/features.c
>> +++ b/drivers/cxl/features.c
>> @@ -7,6 +7,139 @@
> [...]
>> +static int get_supported_features(struct cxl_memdev *cxlmd,
>> +				  struct cxl_features_state *cxlfs)
>> +{
>> +	int remain_feats, max_size, max_feats, start, rc, hdr_size;
>> +	struct cxl_mailbox *cxl_mbox = &cxlmd->cxlds->cxl_mbox;
>> +	int feat_size = sizeof(struct cxl_feat_entry);
>> +	struct cxl_mbox_get_sup_feats_in mbox_in;
>> +	struct cxl_feat_entry *entry;
>> +	struct cxl_mbox_cmd mbox_cmd;
>> +	int count;
>> +
>> +	if (cxlfs->cap < CXL_FEATURES_RO)
>> +		return -EOPNOTSUPP;
>> +
>> +	count = cxl_get_supported_features_count(cxl_mbox);
>> +	if (count == 0)
>> +		return -ENOENT;
>> +	if (count < 0)
>> +		return -ENXIO;
>> +
>> +	struct cxl_feat_entry *entries __free(kvfree) =
>> +		kvmalloc(count * sizeof(*entries), GFP_KERNEL);
>> +	if (!entries)
>> +		return -ENOMEM;
>> +
>> +	struct cxl_mbox_get_sup_feats_out *mbox_out __free(kvfree) =
>> +		kvmalloc(cxl_mbox->payload_size, GFP_KERNEL);
>> +	if (!mbox_out)
>> +		return -ENOMEM;
>> +
>> +	hdr_size = struct_size(mbox_out, ents, 0);
>> +	max_size = cxl_mbox->payload_size - hdr_size;
>> +	/* max feat entries that can fit in mailbox max payload size */
>> +	max_feats = max_size / feat_size;
>> +	entry = entries;
>> +
>> +	start = 0;
>> +	remain_feats = count;
>> +	do {
>> +		int retrieved, alloc_size, copy_feats;
>> +		int num_entries;
>> +
>> +		if (remain_feats > max_feats) {
>> +			alloc_size = struct_size(mbox_out, ents, max_feats);
>> +			remain_feats = remain_feats - max_feats;
>> +			copy_feats = max_feats;
>> +		} else {
>> +			alloc_size = struct_size(mbox_out, ents, remain_feats);
>> +			copy_feats = remain_feats;
>> +			remain_feats = 0;
>> +		}
>> +
>> +		memset(&mbox_in, 0, sizeof(mbox_in));
>> +		mbox_in.count = cpu_to_le32(alloc_size);
>> +		mbox_in.start_idx = cpu_to_le16(start);
>> +		memset(mbox_out, 0, alloc_size);
>> +		mbox_cmd = (struct cxl_mbox_cmd) {
>> +			.opcode = CXL_MBOX_OP_GET_SUPPORTED_FEATURES,
>> +			.size_in = sizeof(mbox_in),
>> +			.payload_in = &mbox_in,
>> +			.size_out = alloc_size,
>> +			.payload_out = mbox_out,
>> +			.min_out = hdr_size,
>> +		};
>> +		rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
>> +		if (rc < 0)
>> +			return rc;
>> +
>> +		if (mbox_cmd.size_out <= hdr_size)
>> +			return -ENXIO;
>> +
>> +		/*
>> +		 * Make sure retrieved out buffer is multiple of feature
>> +		 * entries.
>> +		 */
>> +		retrieved = mbox_cmd.size_out - hdr_size;
>> +		if (retrieved % feat_size)
>> +			return -ENXIO;
>> +
>> +		num_entries = le16_to_cpu(mbox_out->num_entries);
>> +		/*
>> +		 * If the reported output entries * defined entry size !=
>> +		 * retrieved output bytes, then the output package is incorrect.
>> +		 */
>> +		if (num_entries * feat_size != retrieved)
>> +			return -ENXIO;
>> +
>> +		memcpy(entry, mbox_out->ents, retrieved);
>> +		entry++;
> 
> This memcpy() copies multiple entries from mbox_out->ents to entry, this ''entry++" should be "entry += num_entries"?

Thanks for finding this Ming. Appreciate the review. Fixed in v4.

DJ

> 
> 
> Ming
> 
>> +		/*
>> +		 * If the number of output entries is less than expected, add the
>> +		 * remaining entries to the next batch.
>> +		 */
>> +		remain_feats += copy_feats - num_entries;
>> +		start += num_entries;
>> +	} while (remain_feats);
>> +
>> +	cxlfs->num_features = count;
>> +	cxlfs->entries = no_free_ptr(entries);
>> +	return devm_add_action_or_reset(&cxlmd->dev, cxl_free_feature_entries,
>> +					cxlfs->entries);
>> +}
>> +
>