[PATCH 0/4] cxl: Add a firmware update mechanism and cxl_test emulation

[PATCH 0/4] cxl: Add a firmware update mechanism and cxl_test emulation

[Vishal Verma]

Add firmware update capability to the CXL memdev driver, and emulation
in cxl_test. Since the 'Transfer FW' mailbox command is a background
command, this series depends on the initial background command support
patches[1] from Davidlohr. These patches (patch 1 and 2) are included
in this series, unchanged, for reference.

Since the Transfer FW command can be a long-running background command,
it is desirable to retain kernel control over it, so that one command
doesn't monopolize the mailbox interface. The sysfs based firmware
loader mechanism that was developed for FPGAs is a suitable candidate to
help accomplish this. Patch 3 goes into more details on this.

The poll interval for the Transfer FW command is arbitrarily set at 1
second, and a poll count of 300, giving us a total wait time of five
minutes before which each slice of the transfer times out. This seems
like a good mix of responsiveness and a total wait - the spec doesn't
have any guidance on any upper or lower bounds for this. This likely
does not need to be user-configurable, so for now it is just hard-coded
in the driver.

Patch 4 implements the emulation of firmware update related commands in
the cxl_test environment to enable unit testing.

[1]: https://lore.kernel.org/linux-cxl/20230421092321.12741-1-dave@stgolabs.net

Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
Davidlohr Bueso (2):
      cxl/pci: Allocate irq vectors earlier in pci probe
      cxl/mbox: Add background cmd handling machinery

Vishal Verma (2):
      cxl: add a firmware update mechanism using the sysfs firmware loader
      tools/testing/cxl: add firmware update emulation to CXL memdevs

 drivers/cxl/cxl.h                       |   7 +
 drivers/cxl/cxlmem.h                    |  84 +++++++++
 drivers/cxl/core/mbox.c                 |   4 +-
 drivers/cxl/core/memdev.c               | 324 ++++++++++++++++++++++++++++++++
 drivers/cxl/pci.c                       | 112 ++++++++++-
 tools/testing/cxl/test/mem.c            | 191 +++++++++++++++++++
 Documentation/ABI/testing/sysfs-bus-cxl |  11 ++
 drivers/cxl/Kconfig                     |   1 +
 8 files changed, 728 insertions(+), 6 deletions(-)
---
base-commit: 24b18197184ac39bb8566fb82c0bf788bcd0d45b
change-id: 20230421-vv-fw_update-59e35ad0d018

Best regards,
-- 
Vishal Verma <vishal.l.verma@intel.com>

[PATCH 1/4] cxl/pci: Allocate irq vectors earlier in pci probe

[Vishal Verma]

From: Davidlohr Bueso <dave@stgolabs.net>

Move the cxl_alloc_irq_vectors() call further up in the probing
in order to allow for mailbox interrupt usage. No change in
semantics.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
Link: https://lore.kernel.org/r/20230421092321.12741-2-dave@stgolabs.net
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/cxl/pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 60b23624d167..39b829a29f6c 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -757,6 +757,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		dev_dbg(&pdev->dev, "Failed to map RAS capability.\n");
 
+	rc = cxl_alloc_irq_vectors(pdev);
+	if (rc)
+		return rc;
+
 	rc = cxl_pci_setup_mailbox(cxlds);
 	if (rc)
 		return rc;
@@ -777,10 +781,6 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
-	rc = cxl_alloc_irq_vectors(pdev);
-	if (rc)
-		return rc;
-
 	cxlmd = devm_cxl_add_memdev(cxlds);
 	if (IS_ERR(cxlmd))
 		return PTR_ERR(cxlmd);

-- 
2.40.0

Re: [PATCH 1/4] cxl/pci: Allocate irq vectors earlier in pci probe

[Jonathan Cameron]

On Fri, 21 Apr 2023 21:09:25 -0600
Vishal Verma <vishal.l.verma@intel.com> wrote:

> From: Davidlohr Bueso <dave@stgolabs.net>
> 
> Move the cxl_alloc_irq_vectors() call further up in the probing
> in order to allow for mailbox interrupt usage. No change in
> semantics.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> Link: https://lore.kernel.org/r/20230421092321.12741-2-dave@stgolabs.net
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

If it merges with this set to make life easier for picking it up
I'll repeat this here.

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

> ---
>  drivers/cxl/pci.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 60b23624d167..39b829a29f6c 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -757,6 +757,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		dev_dbg(&pdev->dev, "Failed to map RAS capability.\n");
>  
> +	rc = cxl_alloc_irq_vectors(pdev);
> +	if (rc)
> +		return rc;
> +
>  	rc = cxl_pci_setup_mailbox(cxlds);
>  	if (rc)
>  		return rc;
> @@ -777,10 +781,6 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> -	rc = cxl_alloc_irq_vectors(pdev);
> -	if (rc)
> -		return rc;
> -
>  	cxlmd = devm_cxl_add_memdev(cxlds);
>  	if (IS_ERR(cxlmd))
>  		return PTR_ERR(cxlmd);
> 

[PATCH 2/4] cxl/mbox: Add background cmd handling machinery

[Vishal Verma]

From: Davidlohr Bueso <dave@stgolabs.net>

This adds support for handling background operations, as defined in
the CXL 3.0 spec. Commands that can take too long (over ~2 seconds)
can run in the background asynchronously (to the hardware).

The driver will deal with such commands synchronously, blocking all
other incoming commands for a specified period of time, allowing
time-slicing the command such that the caller can send incremental
requests to avoid monopolizing the driver/device. This approach
makes the code simpler, where any out of sync (timeout) between the
driver and hardware is just disregarded as an invalid state until
the next successful submission.

On devices where mbox interrupts are supported, this will still use
a poller that will wakeup in the specified wait intervals. The irq
handler will simply awake a blocked cmd, which is also safe vs a
task that is either waking (timing out) or already awoken. Similarly
any irq setup error during the probing falls back to polling, thus
avoids unnecessarily erroring out.

Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
Link: https://lore.kernel.org/r/20230421092321.12741-2-dave@stgolabs.net
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/cxl/cxl.h       |   7 ++++
 drivers/cxl/cxlmem.h    |   5 +++
 drivers/cxl/core/mbox.c |   3 +-
 drivers/cxl/pci.c       | 104 +++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 044a92d9813e..72731a896f58 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -176,14 +176,21 @@ static inline int ways_to_eiw(unsigned int ways, u8 *eiw)
 /* CXL 2.0 8.2.8.4 Mailbox Registers */
 #define CXLDEV_MBOX_CAPS_OFFSET 0x00
 #define   CXLDEV_MBOX_CAP_PAYLOAD_SIZE_MASK GENMASK(4, 0)
+#define   CXLDEV_MBOX_CAP_IRQ_MSGNUM_MASK GENMASK(10, 7)
+#define   CXLDEV_MBOX_CAP_BG_CMD_IRQ BIT(6)
 #define CXLDEV_MBOX_CTRL_OFFSET 0x04
 #define   CXLDEV_MBOX_CTRL_DOORBELL BIT(0)
+#define   CXLDEV_MBOX_CTRL_BG_CMD_IRQ BIT(2)
 #define CXLDEV_MBOX_CMD_OFFSET 0x08
 #define   CXLDEV_MBOX_CMD_COMMAND_OPCODE_MASK GENMASK_ULL(15, 0)
 #define   CXLDEV_MBOX_CMD_PAYLOAD_LENGTH_MASK GENMASK_ULL(36, 16)
 #define CXLDEV_MBOX_STATUS_OFFSET 0x10
+#define   CXLDEV_MBOX_STATUS_BG_CMD BIT(0)
 #define   CXLDEV_MBOX_STATUS_RET_CODE_MASK GENMASK_ULL(47, 32)
 #define CXLDEV_MBOX_BG_CMD_STATUS_OFFSET 0x18
+#define   CXLDEV_MBOX_BG_CMD_COMMAND_OPCODE_MASK GENMASK_ULL(15, 0)
+#define   CXLDEV_MBOX_BG_CMD_COMMAND_PCT_MASK GENMASK_ULL(22, 16)
+#define   CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK GENMASK_ULL(47, 32)
 #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20
 
 /*
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 090acebba4fa..8c3302fc7738 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -108,6 +108,9 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
  *            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: (input) Number of ms between mailbox background command
+ *                 polling intervals timeouts.
  * @return_code: (output) Error code returned from hardware.
  *
  * This is the primary mechanism used to send commands to the hardware.
@@ -123,6 +126,8 @@ struct cxl_mbox_cmd {
 	size_t size_in;
 	size_t size_out;
 	size_t min_out;
+	int poll_count;
+	int poll_interval;
 	u16 return_code;
 };
 
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index f2addb457172..4b0c7564d350 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -181,7 +181,8 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
 	if (rc)
 		return rc;
 
-	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS)
+	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS &&
+	    mbox_cmd->return_code != CXL_MBOX_CMD_RC_BACKGROUND)
 		return cxl_mbox_cmd_rc2errno(mbox_cmd);
 
 	if (!out_size)
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 39b829a29f6c..aa1bb74a52a1 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -51,6 +51,7 @@
 static unsigned short mbox_ready_timeout = 60;
 module_param(mbox_ready_timeout, ushort, 0644);
 MODULE_PARM_DESC(mbox_ready_timeout, "seconds to wait for mailbox ready");
+static DECLARE_WAIT_QUEUE_HEAD(mbox_wait);
 
 static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds)
 {
@@ -85,6 +86,33 @@ static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds)
 			    status & CXLMDEV_DEV_FATAL ? " fatal" : "",        \
 			    status & CXLMDEV_FW_HALT ? " firmware-halt" : "")
 
+static bool cxl_mbox_background_complete(struct cxl_dev_state *cxlds)
+{
+	u64 reg;
+
+	reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
+	return FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_PCT_MASK, reg) == 100;
+}
+
+static irqreturn_t cxl_pci_mbox_irq(int irq, void *id)
+{
+	struct cxl_dev_state *cxlds = id;
+
+	/* spurious or raced with hw? */
+	if (!cxl_mbox_background_complete(cxlds)) {
+		struct pci_dev *pdev = to_pci_dev(cxlds->dev);
+
+		dev_warn(&pdev->dev,
+			 "Mailbox background operation IRQ but incomplete\n");
+		goto done;
+	}
+
+	/* short-circuit the wait in __cxl_pci_mbox_send_cmd() */
+	wake_up(&mbox_wait);
+done:
+	return IRQ_HANDLED;
+}
+
 /**
  * __cxl_pci_mbox_send_cmd() - Execute a mailbox command
  * @cxlds: The device state to communicate with.
@@ -178,7 +206,59 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
 	mbox_cmd->return_code =
 		FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
 
-	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS) {
+	/*
+	 * Handle the background command in a synchronous manner.
+	 *
+	 * All other mailbox commands will serialize/queue on the mbox_mutex,
+	 * which we currently hold. Furthermore this also guarantees that
+	 * cxl_mbox_background_complete() checks are safe amongst each other,
+	 * in that no new bg operation can occur in between.
+	 *
+	 * Background operations are timesliced in accordance with the nature
+	 * of the command. In the event of timeout, the mailbox state is
+	 * indeterminate until the next successful command submission and the
+	 * driver can get back in sync with the hardware state.
+	 */
+	if (mbox_cmd->return_code == CXL_MBOX_CMD_RC_BACKGROUND) {
+		u64 bg_status_reg;
+		int i;
+
+		dev_dbg(dev, "Mailbox background operation (0x%04x) started\n",
+			mbox_cmd->opcode);
+
+		for (i = 0; i < mbox_cmd->poll_count; i++) {
+			int ret = wait_event_interruptible_timeout(
+				mbox_wait, cxl_mbox_background_complete(cxlds),
+				msecs_to_jiffies(mbox_cmd->poll_interval));
+			if (ret > 0)
+				break;
+
+			/* interrupted by a signal */
+			if (ret < 0)
+				return ret;
+		}
+
+		if (!cxl_mbox_background_complete(cxlds)) {
+			u64 md_status =
+				readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
+
+			cxl_cmd_err(cxlds->dev, mbox_cmd, md_status,
+				    "background timeout");
+			return -ETIMEDOUT;
+		}
+
+		bg_status_reg = readq(cxlds->regs.mbox +
+				      CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
+		mbox_cmd->return_code =
+			FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK,
+				  bg_status_reg);
+		dev_dbg(dev,
+			"Mailbox background operation (0x%04x) completed\n",
+			mbox_cmd->opcode);
+	}
+
+	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS &&
+	    mbox_cmd->return_code != CXL_MBOX_CMD_RC_BACKGROUND) {
 		dev_dbg(dev, "Mailbox operation had an error: %s\n",
 			cxl_mbox_cmd_rc2str(mbox_cmd));
 		return 0; /* completed but caller must check return_code */
@@ -224,6 +304,7 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
 	const int cap = readl(cxlds->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
 	unsigned long timeout;
 	u64 md_status;
+	int rc, irq;
 
 	timeout = jiffies + mbox_ready_timeout * HZ;
 	do {
@@ -272,6 +353,27 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
 	dev_dbg(cxlds->dev, "Mailbox payload sized %zu",
 		cxlds->payload_size);
 
+	if (cap & CXLDEV_MBOX_CAP_BG_CMD_IRQ) {
+		struct pci_dev *pdev = to_pci_dev(cxlds->dev);
+
+		irq = pci_irq_vector(pdev,
+			     FIELD_GET(CXLDEV_MBOX_CAP_IRQ_MSGNUM_MASK, cap));
+		if (irq < 0)
+			goto mbox_poll;
+
+		rc = devm_request_irq(cxlds->dev, irq, cxl_pci_mbox_irq,
+				      IRQF_SHARED, "mailbox", cxlds);
+		if (rc)
+			goto mbox_poll;
+
+		writel(CXLDEV_MBOX_CTRL_BG_CMD_IRQ,
+		       cxlds->regs.mbox + CXLDEV_MBOX_CTRL_OFFSET);
+
+		return 0;
+	}
+
+mbox_poll:
+	dev_dbg(cxlds->dev, "Mailbox interrupts are unsupported");
 	return 0;
 }
 

-- 
2.40.0

[PATCH 3/4] cxl: add a firmware update mechanism using the sysfs firmware loader

[Vishal Verma]

The sysfs based firmware loader mechanism was created to easily allow
userspace to upload firmware images to FPGA cards. This also happens to
be pretty suitable to create a user-initiated but kernel-controlled
firmware update mechanism for CXL devices, using the CXL specified
mailbox commands.

Since firmware update commands can be long-running, and can be processed
in the background by the endpoint device, it is desirable to have the
ability to chunk the firmware transfer down to smaller pieces, so that
one operation does not monopolize the mailbox, locking out any other
long running background commands entirely - e.g. security commands like
'sanitize' or poison scanning operations.

The firmware loader mechanism allows a natural way to perform this
chunking, as after each mailbox command, that is restricted to the
maximum mailbox payload size, the cxl memdev driver relinquishes control
back to the fw_loader system and awaits the next chunk of data to
transfer. This opens opportunities for other background commands to
access the mailbox and send their own slices of background commands.

Add the necessary helpers and state tracking to be able to perform the
'Get FW Info', 'Transfer FW', and 'Activate FW' mailbox commands as
described in the CXL spec. Wire these up to the firmware loader
callbacks, and register with that system to create the memX/firmware/
sysfs ABI.

Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 drivers/cxl/cxlmem.h                    |  79 ++++++++
 drivers/cxl/core/mbox.c                 |   1 +
 drivers/cxl/core/memdev.c               | 324 ++++++++++++++++++++++++++++++++
 Documentation/ABI/testing/sysfs-bus-cxl |  11 ++
 drivers/cxl/Kconfig                     |   1 +
 5 files changed, 416 insertions(+)

diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 8c3302fc7738..0ecee5b558f4 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -48,6 +48,8 @@ struct cxl_memdev {
 	struct work_struct detach_work;
 	struct cxl_nvdimm_bridge *cxl_nvb;
 	struct cxl_nvdimm *cxl_nvd;
+	struct fw_upload *fwl;
+	const char *fw_name;
 	int id;
 	int depth;
 };
@@ -220,6 +222,80 @@ struct cxl_event_state {
 	struct mutex log_lock;
 };
 
+/*
+ * Get Firmware Info
+ * CXL rev 3.0 section 8.2.9.3.1; Table 8-56
+ */
+struct cxl_mbox_get_fw_info {
+	u8 num_slots;
+	u8 slot_info;
+	u8 activation_cap;
+	u8 reserved[13];
+	char slot_1_revision[0x10];
+	char slot_2_revision[0x10];
+	char slot_3_revision[0x10];
+	char slot_4_revision[0x10];
+} __packed;
+
+#define CXL_FW_INFO_CUR_SLOT_MASK	GENMASK(2, 0)
+#define CXL_FW_INFO_NEXT_SLOT_MASK	GENMASK(5, 3)
+#define CXL_FW_INFO_NEXT_SLOT_SHIFT	3
+#define CXL_FW_INFO_HAS_LIVE_ACTIVATE	BIT(0)
+
+/*
+ * Transfer Firmware Input Payload
+ * CXL rev 3.0 section 8.2.9.3.2; Table 8-57
+ */
+struct cxl_mbox_transfer_fw {
+	u8 action;
+	u8 slot;
+	u8 reserved[2];
+	__le32 offset;
+	u8 reserved2[0x78];
+	u8 data[];
+} __packed;
+
+#define CXL_FW_TRANSFER_ACTION_FULL	0x0
+#define CXL_FW_TRANSFER_ACTION_START	0x1
+#define CXL_FW_TRANSFER_ACTION_CONTINUE	0x2
+#define CXL_FW_TRANSFER_ACTION_END	0x3
+#define CXL_FW_TRANSFER_ACTION_ABORT	0x4
+#define CXL_FW_TRANSFER_OFFSET_ALIGN	128
+
+/*
+ * Activate Firmware Input Payload
+ * CXL rev 3.0 section 8.2.9.3.3; Table 8-58
+ */
+struct cxl_mbox_activate_fw {
+	u8 action;
+	u8 slot;
+} __packed;
+
+#define CXL_FW_ACTIVATE_ONLINE		0x0
+#define CXL_FW_ACTIVATE_OFFLINE		0x1
+
+/**
+ * struct cxl_fw_state - Firmware upload / activation state
+ *
+ * @fw_mutex: Mutex to serialize fw update requests
+ * @clear_to_send: Initial checks done, ready to start FW transfer
+ * @cancel: Cancel any in-progress FW upload
+ * @next_slot: Slot number for the new firmware
+ * @info: Get FW Info structure
+ * @activate: Activate FW structure
+ * @transfer: Transfer FW Info structure
+ */
+struct cxl_fw_state {
+	struct mutex fw_mutex;
+	bool clear_to_send;
+	bool oneshot;
+	bool cancel;
+	int next_slot;
+	struct cxl_mbox_get_fw_info info;
+	struct cxl_mbox_activate_fw activate;
+	struct cxl_mbox_transfer_fw *transfer;
+};
+
 /**
  * struct cxl_dev_state - The driver device state
  *
@@ -256,6 +332,7 @@ struct cxl_event_state {
  * @serial: PCIe Device Serial Number
  * @doe_mbs: PCI DOE mailbox array
  * @event: event log driver state
+ * @fw: firmware upload / activation state
  * @mbox_send: @dev specific transport for transmitting mailbox commands
  *
  * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
@@ -295,6 +372,7 @@ struct cxl_dev_state {
 	struct xarray doe_mbs;
 
 	struct cxl_event_state event;
+	struct cxl_fw_state fw;
 
 	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
 };
@@ -307,6 +385,7 @@ enum cxl_opcode {
 	CXL_MBOX_OP_GET_EVT_INT_POLICY	= 0x0102,
 	CXL_MBOX_OP_SET_EVT_INT_POLICY	= 0x0103,
 	CXL_MBOX_OP_GET_FW_INFO		= 0x0200,
+	CXL_MBOX_OP_TRANSFER_FW		= 0x0201,
 	CXL_MBOX_OP_ACTIVATE_FW		= 0x0202,
 	CXL_MBOX_OP_SET_TIMESTAMP	= 0x0301,
 	CXL_MBOX_OP_GET_SUPPORTED_LOGS	= 0x0400,
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 4b0c7564d350..bdeb5495d1cc 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1120,6 +1120,7 @@ struct cxl_dev_state *cxl_dev_state_create(struct device *dev)
 
 	mutex_init(&cxlds->mbox_mutex);
 	mutex_init(&cxlds->event.log_lock);
+	mutex_init(&cxlds->fw.fw_mutex);
 	cxlds->dev = dev;
 
 	return cxlds;
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 28a05f2fe32d..8f61b1e526ae 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2020 Intel Corporation. */
 
+#include <linux/firmware.h>
 #include <linux/device.h>
 #include <linux/slab.h>
 #include <linux/idr.h>
@@ -214,6 +215,8 @@ static void cxl_memdev_unregister(void *_cxlmd)
 	struct cxl_memdev *cxlmd = _cxlmd;
 	struct device *dev = &cxlmd->dev;
 
+	firmware_upload_unregister(cxlmd->fwl);
+	kfree(cxlmd->fw_name);
 	cxl_memdev_shutdown(dev);
 	cdev_device_del(&cxlmd->cdev, dev);
 	put_device(dev);
@@ -315,6 +318,320 @@ static int cxl_memdev_release_file(struct inode *inode, struct file *file)
 	return 0;
 }
 
+/**
+ * cxl_mem_get_fw_info - Get Firmware info
+ * @cxlds: The device data for the operation
+ *
+ * Retrieve firmware info for the device specified.
+ *
+ * Return: 0 if no error: or the result of the mailbox command.
+ *
+ * See CXL-3.0 8.2.9.3.1 Get FW Info
+ */
+static int cxl_mem_get_fw_info(struct cxl_dev_state *cxlds)
+{
+	struct cxl_mbox_get_fw_info *info = &cxlds->fw.info;
+	struct cxl_mbox_cmd mbox_cmd;
+
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_GET_FW_INFO,
+		.size_out = sizeof(*info),
+		.payload_out = info,
+	};
+
+	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
+}
+
+/**
+ * cxl_mem_activate_fw - Activate Firmware
+ * @cxlds: The device data for the operation
+ * @slot: slot number to activate
+ *
+ * Activate firmware in a given slot for the device specified.
+ *
+ * Return: 0 if no error: or the result of the mailbox command.
+ *
+ * See CXL-3.0 8.2.9.3.3 Activate FW
+ */
+static int cxl_mem_activate_fw(struct cxl_dev_state *cxlds, int slot)
+{
+	struct cxl_mbox_activate_fw *activate = &cxlds->fw.activate;
+	struct cxl_mbox_cmd mbox_cmd;
+
+	if (slot == 0 || slot > cxlds->fw.info.num_slots)
+		return -EINVAL;
+
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_ACTIVATE_FW,
+		.size_in = sizeof(*activate),
+		.payload_in = activate,
+	};
+
+	/* Only offline activation supported for now */
+	activate->action = CXL_FW_ACTIVATE_OFFLINE;
+	activate->slot = slot;
+
+	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
+}
+
+/**
+ * cxl_mem_abort_fw_xfer - Abort an in-progress FW transfer
+ * @cxlds: The device data for the operation
+ *
+ * Abort an in-progress firmware transfer for the device specified.
+ *
+ * Return: 0 if no error: or the result of the mailbox command.
+ *
+ * See CXL-3.0 8.2.9.3.2 Transfer FW
+ */
+static int cxl_mem_abort_fw_xfer(struct cxl_dev_state *cxlds)
+{
+	struct cxl_mbox_transfer_fw *transfer;
+	struct cxl_mbox_cmd mbox_cmd;
+	int rc;
+
+	transfer = kzalloc(sizeof(*transfer), GFP_KERNEL);
+	if (!transfer)
+		return -ENOMEM;
+
+	/* Set a 1s poll interval and a total wait time of 5 minutes */
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_TRANSFER_FW,
+		.size_in = sizeof(*transfer),
+		.payload_in = transfer,
+		.poll_interval = 1000,
+		.poll_count = 300,
+	};
+
+	transfer->action = CXL_FW_TRANSFER_ACTION_ABORT;
+
+	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
+	kfree(transfer);
+	return rc;
+}
+
+static void cxl_fw_cleanup(struct fw_upload *fwl)
+{
+	struct cxl_memdev *cxlmd = fwl->dd_handle;
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+
+	cxlds->fw.clear_to_send = false;
+	cxlds->fw.cancel = false;
+	cxlds->fw.next_slot = 0;
+}
+
+static enum fw_upload_err cxl_fw_prepare(struct fw_upload *fwl, const u8 *data,
+					 u32 size)
+{
+	struct cxl_memdev *cxlmd = fwl->dd_handle;
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+
+	if (!size)
+		return FW_UPLOAD_ERR_INVALID_SIZE;
+
+	if (size + sizeof(struct cxl_mbox_transfer_fw) < cxlds->payload_size)
+		cxlds->fw.oneshot = true;
+	else
+		cxlds->fw.oneshot = false;
+
+	if (cxl_mem_get_fw_info(cxlds))
+		return FW_UPLOAD_ERR_HW_ERROR;
+
+	if (cxlds->fw.cancel) {
+		cxl_fw_cleanup(fwl);
+		return FW_UPLOAD_ERR_CANCELED;
+	}
+
+	cxlds->fw.clear_to_send = true;
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static enum fw_upload_err cxl_fw_write(struct fw_upload *fwl, const u8 *data,
+				       u32 offset, u32 size, u32 *written)
+{
+	struct cxl_memdev *cxlmd = fwl->dd_handle;
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	struct cxl_mbox_transfer_fw *transfer;
+	struct cxl_mbox_cmd mbox_cmd;
+	int rc, num_slots, cur_slot;
+	u32 cur_size, remaining;
+	size_t size_in;
+
+	*written = 0;
+
+	/* Offset has to be aligned to 128B (CXL-3.0 8.2.9.3.2 Table 8-57) */
+	if (offset % CXL_FW_TRANSFER_OFFSET_ALIGN) {
+		dev_err(&cxlmd->dev,
+			"misaligned offset for FW transfer slice (%u)\n",
+			offset);
+		return FW_UPLOAD_ERR_RW_ERROR;
+	}
+
+	/* Pick transfer size based on cxlds->payload_size */
+	cur_size = ALIGN_DOWN(min((size_t)size,
+				  cxlds->payload_size - sizeof(*transfer)),
+			      CXL_FW_TRANSFER_OFFSET_ALIGN);
+	remaining = size - cur_size;
+	size_in = cur_size + sizeof(*transfer);
+
+	mutex_lock(&cxlds->fw.fw_mutex);
+	if (!cxlds->fw.clear_to_send) {
+		dev_err(&cxlmd->dev, "firmware_loader prep not done\n");
+		rc = FW_UPLOAD_ERR_BUSY;
+		goto out_unlock;
+	}
+
+	if (cxlds->fw.cancel) {
+		rc = cxl_mem_abort_fw_xfer(cxlds);
+		if (rc < 0)
+			dev_err(&cxlmd->dev, "Error aborting FW transfer: %d\n",
+				rc);
+		cxl_fw_cleanup(fwl);
+		rc = FW_UPLOAD_ERR_CANCELED;
+		goto out_unlock;
+	}
+
+	/* Determine next slot from fw_info */
+	num_slots = cxlds->fw.info.num_slots;
+	cur_slot = FIELD_GET(CXL_FW_INFO_CUR_SLOT_MASK,
+			     cxlds->fw.info.slot_info);
+
+	/*
+	 * Slot numbers are 1-indexed
+	 * cur_slot is the 0-indexed next_slot (i.e. 'cur_slot - 1 + 1')
+	 * Check for rollover using modulo, and 1-index it by adding 1
+	 */
+	cxlds->fw.next_slot = (cur_slot % num_slots) + 1;
+
+	/* Do the transfer via mailbox cmd */
+	transfer = kzalloc(size_in, GFP_KERNEL);
+	if (!transfer) {
+		rc = FW_UPLOAD_ERR_RW_ERROR;
+		goto out_unlock;
+	}
+
+	transfer->offset = cpu_to_le32(offset / CXL_FW_TRANSFER_OFFSET_ALIGN);
+	memcpy(transfer->data, data + offset, cur_size);
+	if (cxlds->fw.oneshot) {
+		transfer->action = CXL_FW_TRANSFER_ACTION_FULL;
+		transfer->slot = cxlds->fw.next_slot;
+	} else {
+		if (offset == 0) {
+			transfer->action = CXL_FW_TRANSFER_ACTION_START;
+		} else if (remaining == 0) {
+			transfer->action = CXL_FW_TRANSFER_ACTION_END;
+			transfer->slot = cxlds->fw.next_slot;
+		} else {
+			transfer->action = CXL_FW_TRANSFER_ACTION_CONTINUE;
+		}
+	}
+
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_TRANSFER_FW,
+		.size_in = size_in,
+		.payload_in = transfer,
+	};
+
+	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
+	if (rc < 0) {
+		kfree(transfer);
+		rc = FW_UPLOAD_ERR_RW_ERROR;
+		goto out_unlock;
+	}
+
+	*written = cur_size;
+
+	/* Activate FW if oneshot or if the last slice was written */
+	if (cxlds->fw.oneshot || remaining == 0) {
+		dev_dbg(&cxlmd->dev, "Activating firmware slot: %d\n",
+			cxlds->fw.next_slot);
+		rc = cxl_mem_activate_fw(cxlds, cxlds->fw.next_slot);
+		if (rc < 0) {
+			dev_err(&cxlmd->dev, "Error activating firmware: %d\n",
+				rc);
+			rc = FW_UPLOAD_ERR_HW_ERROR;
+			goto out_unlock;
+		}
+	}
+
+	rc = FW_UPLOAD_ERR_NONE;
+
+out_unlock:
+	mutex_unlock(&cxlds->fw.fw_mutex);
+	return rc;
+}
+
+static enum fw_upload_err cxl_fw_poll_complete(struct fw_upload *fwl)
+{
+	struct cxl_memdev *cxlmd = fwl->dd_handle;
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	int rc;
+
+	/*
+	 * cxl_internal_send_cmd() handles background operations synchronously.
+	 * No need to wait for completions here - any errors would've been
+	 * reported and handled during the ->write() call(s).
+	 * Just check if a cancel request was received, and return success.
+	 */
+	mutex_lock(&cxlds->fw.fw_mutex);
+	if (cxlds->fw.cancel) {
+		rc = cxl_mem_abort_fw_xfer(cxlds);
+		if (rc < 0)
+			dev_err(&cxlmd->dev, "Error aborting FW transfer: %d\n",
+				rc);
+		cxl_fw_cleanup(fwl);
+		rc = FW_UPLOAD_ERR_CANCELED;
+		goto out_unlock;
+	}
+
+	rc = FW_UPLOAD_ERR_NONE;
+
+out_unlock:
+	mutex_unlock(&cxlds->fw.fw_mutex);
+	return rc;
+}
+
+static void cxl_fw_cancel(struct fw_upload *fwl)
+{
+	struct cxl_memdev *cxlmd = fwl->dd_handle;
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+
+	mutex_lock(&cxlds->fw.fw_mutex);
+	cxlds->fw.cancel = true;
+	mutex_unlock(&cxlds->fw.fw_mutex);
+}
+
+static const struct fw_upload_ops cxl_memdev_fw_ops = {
+        .prepare = cxl_fw_prepare,
+        .write = cxl_fw_write,
+        .poll_complete = cxl_fw_poll_complete,
+        .cancel = cxl_fw_cancel,
+        .cleanup = cxl_fw_cleanup,
+};
+
+static int cxl_memdev_setup_fw_upload(struct cxl_memdev *cxlmd)
+{
+	const char *fw_name, *truncate;
+	struct fw_upload *fwl;
+	unsigned int len;
+
+	fw_name = dev_name(&cxlmd->dev);
+	truncate = strstr(fw_name, ".auto");
+	len = (truncate) ? truncate - fw_name : strlen(fw_name);
+	cxlmd->fw_name = kmemdup_nul(fw_name, len, GFP_KERNEL);
+
+	fwl = firmware_upload_register(THIS_MODULE, &cxlmd->dev, cxlmd->fw_name,
+				       &cxl_memdev_fw_ops, cxlmd);
+	if (IS_ERR(fwl)) {
+		dev_err(&cxlmd->dev, "Failed to register firmware loader\n");
+		kfree(cxlmd->fw_name);
+		return PTR_ERR(fwl);
+	}
+
+	cxlmd->fwl = fwl;
+	return 0;
+}
+
 static const struct file_operations cxl_memdev_fops = {
 	.owner = THIS_MODULE,
 	.unlocked_ioctl = cxl_memdev_ioctl,
@@ -352,11 +669,18 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
 	if (rc)
 		goto err;
 
+	rc = cxl_memdev_setup_fw_upload(cxlmd);
+	if (rc)
+		goto err_fw;
+
 	rc = devm_add_action_or_reset(cxlds->dev, cxl_memdev_unregister, cxlmd);
 	if (rc)
 		return ERR_PTR(rc);
 	return cxlmd;
 
+err_fw:
+	firmware_upload_unregister(cxlmd->fwl);
+	kfree(cxlmd->fw_name);
 err:
 	/*
 	 * The cdev was briefly live, shutdown any ioctl operations that
diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 3acf2f17a73f..e94237f5568d 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -58,6 +58,17 @@ Description:
 		affinity for this device.
 
 
+What:		/sys/bus/cxl/devices/memX/firmware/
+Date:		April, 2023
+KernelVersion:	v6.5
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		(RW) Firmware uploader mechanism. The different files under
+		this directory can be used to upload and activate new
+		firmware for CXL devices. The interfaces under this are
+		documented in sysfs-class-firmware.
+
+
 What:		/sys/bus/cxl/devices/*/devtype
 Date:		June, 2021
 KernelVersion:	v5.14
diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
index ff4e78117b31..80d8e35fa049 100644
--- a/drivers/cxl/Kconfig
+++ b/drivers/cxl/Kconfig
@@ -82,6 +82,7 @@ config CXL_PMEM
 config CXL_MEM
 	tristate "CXL: Memory Expansion"
 	depends on CXL_PCI
+	select FW_UPLOAD
 	default CXL_BUS
 	help
 	  The CXL.mem protocol allows a device to act as a provider of "System

-- 
2.40.0

Re: [PATCH 3/4] cxl: add a firmware update mechanism using the sysfs firmware loader

[Jonathan Cameron]

On Fri, 21 Apr 2023 21:09:27 -0600
Vishal Verma <vishal.l.verma@intel.com> wrote:

> The sysfs based firmware loader mechanism was created to easily allow
> userspace to upload firmware images to FPGA cards. This also happens to
> be pretty suitable to create a user-initiated but kernel-controlled
> firmware update mechanism for CXL devices, using the CXL specified
> mailbox commands.
> 
> Since firmware update commands can be long-running, and can be processed
> in the background by the endpoint device, it is desirable to have the
> ability to chunk the firmware transfer down to smaller pieces, so that
> one operation does not monopolize the mailbox, locking out any other
> long running background commands entirely - e.g. security commands like
> 'sanitize' or poison scanning operations.
> 
> The firmware loader mechanism allows a natural way to perform this
> chunking, as after each mailbox command, that is restricted to the
> maximum mailbox payload size, the cxl memdev driver relinquishes control
> back to the fw_loader system and awaits the next chunk of data to
> transfer. This opens opportunities for other background commands to
> access the mailbox and send their own slices of background commands.
> 
> Add the necessary helpers and state tracking to be able to perform the
> 'Get FW Info', 'Transfer FW', and 'Activate FW' mailbox commands as
> described in the CXL spec. Wire these up to the firmware loader
> callbacks, and register with that system to create the memX/firmware/
> sysfs ABI.
> 
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
Hi Vishal,

Various comments inline.

Thanks,

Jonathan

> ---
>  drivers/cxl/cxlmem.h                    |  79 ++++++++
>  drivers/cxl/core/mbox.c                 |   1 +
>  drivers/cxl/core/memdev.c               | 324 ++++++++++++++++++++++++++++++++
>  Documentation/ABI/testing/sysfs-bus-cxl |  11 ++
>  drivers/cxl/Kconfig                     |   1 +
>  5 files changed, 416 insertions(+)
> 
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 8c3302fc7738..0ecee5b558f4 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -48,6 +48,8 @@ struct cxl_memdev {
>  	struct work_struct detach_work;
>  	struct cxl_nvdimm_bridge *cxl_nvb;
>  	struct cxl_nvdimm *cxl_nvd;
> +	struct fw_upload *fwl;
> +	const char *fw_name;
>  	int id;
>  	int depth;
>  };
> @@ -220,6 +222,80 @@ struct cxl_event_state {
>  	struct mutex log_lock;
>  };
>  
> +/*
> + * Get Firmware Info

Odd though it is, Get FW Info
might be better to track with the spec abbreviation used
in the section title etc.
No idea why that got shortened.

> + * CXL rev 3.0 section 8.2.9.3.1; Table 8-56
> + */
> +struct cxl_mbox_get_fw_info {
> +	u8 num_slots;
> +	u8 slot_info;
> +	u8 activation_cap;
> +	u8 reserved[13];
> +	char slot_1_revision[0x10];
> +	char slot_2_revision[0x10];
> +	char slot_3_revision[0x10];
> +	char slot_4_revision[0x10];
> +} __packed;
> +
> +#define CXL_FW_INFO_CUR_SLOT_MASK	GENMASK(2, 0)
I'd like this associate by naming slot info field even if that's
a bit wordy.
	CXL_FW_INFO_SLOT_INFO_CUR_MASK perhaps?

> +#define CXL_FW_INFO_NEXT_SLOT_MASK	GENMASK(5, 3)
> +#define CXL_FW_INFO_NEXT_SLOT_SHIFT	3
> +#define CXL_FW_INFO_HAS_LIVE_ACTIVATE	BIT(0)
CXL_FW_INFO_ACTIVATION_CAP_HAS_LIVE_ACTIVATE

Wordy but makes it clear which field of the message this goes in.

> +
> +/*
> + * Transfer Firmware Input Payload

As above, Transfer FW Input Payload
for spec alignment?

> + * CXL rev 3.0 section 8.2.9.3.2; Table 8-57
> + */
> +struct cxl_mbox_transfer_fw {
> +	u8 action;
> +	u8 slot;
> +	u8 reserved[2];
> +	__le32 offset;
> +	u8 reserved2[0x78];
> +	u8 data[];
> +} __packed;
> +
> +#define CXL_FW_TRANSFER_ACTION_FULL	0x0
> +#define CXL_FW_TRANSFER_ACTION_START	0x1

Initiate isn't that bad and more obviously matches spec than start.

> +#define CXL_FW_TRANSFER_ACTION_CONTINUE	0x2
> +#define CXL_FW_TRANSFER_ACTION_END	0x3
> +#define CXL_FW_TRANSFER_ACTION_ABORT	0x4
> +#define CXL_FW_TRANSFER_OFFSET_ALIGN	128

This looks like a field definition but it isn't.
Can we rename it or add a comment to make that clear.

> +
> +/*
> + * Activate Firmware Input Payload
> + * CXL rev 3.0 section 8.2.9.3.3; Table 8-58
> + */
> +struct cxl_mbox_activate_fw {
> +	u8 action;
> +	u8 slot;
> +} __packed;
> +
> +#define CXL_FW_ACTIVATE_ONLINE		0x0
> +#define CXL_FW_ACTIVATE_OFFLINE		0x1
> +
> +/**
> + * struct cxl_fw_state - Firmware upload / activation state
> + *
> + * @fw_mutex: Mutex to serialize fw update requests
> + * @clear_to_send: Initial checks done, ready to start FW transfer
> + * @cancel: Cancel any in-progress FW upload
> + * @next_slot: Slot number for the new firmware
> + * @info: Get FW Info structure
> + * @activate: Activate FW structure
> + * @transfer: Transfer FW Info structure
> + */
> +struct cxl_fw_state {
> +	struct mutex fw_mutex;
> +	bool clear_to_send;
> +	bool oneshot;
> +	bool cancel;
> +	int next_slot;
> +	struct cxl_mbox_get_fw_info info;
> +	struct cxl_mbox_activate_fw activate;

Not seeing why activate or transfer should be in here.
Even info could be replaced with just the fields we care about outside
of the command itself.

> +	struct cxl_mbox_transfer_fw *transfer;
> +};
> +
>  /**
>   * struct cxl_dev_state - The driver device state
>   *
> @@ -256,6 +332,7 @@ struct cxl_event_state {
>   * @serial: PCIe Device Serial Number
>   * @doe_mbs: PCI DOE mailbox array
>   * @event: event log driver state
> + * @fw: firmware upload / activation state
>   * @mbox_send: @dev specific transport for transmitting mailbox commands
>   *
>   * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
> @@ -295,6 +372,7 @@ struct cxl_dev_state {
>  	struct xarray doe_mbs;
>  
>  	struct cxl_event_state event;
> +	struct cxl_fw_state fw;
>  
>  	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
>  };
> @@ -307,6 +385,7 @@ enum cxl_opcode {
>  	CXL_MBOX_OP_GET_EVT_INT_POLICY	= 0x0102,
>  	CXL_MBOX_OP_SET_EVT_INT_POLICY	= 0x0103,
>  	CXL_MBOX_OP_GET_FW_INFO		= 0x0200,
> +	CXL_MBOX_OP_TRANSFER_FW		= 0x0201,
>  	CXL_MBOX_OP_ACTIVATE_FW		= 0x0202,
>  	CXL_MBOX_OP_SET_TIMESTAMP	= 0x0301,
>  	CXL_MBOX_OP_GET_SUPPORTED_LOGS	= 0x0400,
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 4b0c7564d350..bdeb5495d1cc 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1120,6 +1120,7 @@ struct cxl_dev_state *cxl_dev_state_create(struct device *dev)
>  
>  	mutex_init(&cxlds->mbox_mutex);
>  	mutex_init(&cxlds->event.log_lock);
> +	mutex_init(&cxlds->fw.fw_mutex);
>  	cxlds->dev = dev;
>  
>  	return cxlds;
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 28a05f2fe32d..8f61b1e526ae 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2020 Intel Corporation. */
>  
> +#include <linux/firmware.h>
>  #include <linux/device.h>
>  #include <linux/slab.h>
>  #include <linux/idr.h>
> @@ -214,6 +215,8 @@ static void cxl_memdev_unregister(void *_cxlmd)
>  	struct cxl_memdev *cxlmd = _cxlmd;
>  	struct device *dev = &cxlmd->dev;
>  
> +	firmware_upload_unregister(cxlmd->fwl);
> +	kfree(cxlmd->fw_name);
>  	cxl_memdev_shutdown(dev);
>  	cdev_device_del(&cxlmd->cdev, dev);
>  	put_device(dev);
> @@ -315,6 +318,320 @@ static int cxl_memdev_release_file(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> +/**
> + * cxl_mem_get_fw_info - Get Firmware info
> + * @cxlds: The device data for the operation
> + *
> + * Retrieve firmware info for the device specified.
> + *
> + * Return: 0 if no error: or the result of the mailbox command.
> + *
> + * See CXL-3.0 8.2.9.3.1 Get FW Info
> + */
> +static int cxl_mem_get_fw_info(struct cxl_dev_state *cxlds)
> +{
> +	struct cxl_mbox_get_fw_info *info = &cxlds->fw.info;

> +	struct cxl_mbox_cmd mbox_cmd;
> +
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_GET_FW_INFO,
> +		.size_out = sizeof(*info),
> +		.payload_out = info,
> +	};
> +
> +	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
> +}
> +
> +/**
> + * cxl_mem_activate_fw - Activate Firmware
> + * @cxlds: The device data for the operation
> + * @slot: slot number to activate
> + *
> + * Activate firmware in a given slot for the device specified.
> + *
> + * Return: 0 if no error: or the result of the mailbox command.
> + *
> + * See CXL-3.0 8.2.9.3.3 Activate FW
> + */
> +static int cxl_mem_activate_fw(struct cxl_dev_state *cxlds, int slot)
> +{
> +	struct cxl_mbox_activate_fw *activate = &cxlds->fw.activate;

I'd put this on the stack locally.  Doesn't need to be allocated
all the time.

> +	struct cxl_mbox_cmd mbox_cmd;
> +
> +	if (slot == 0 || slot > cxlds->fw.info.num_slots)
> +		return -EINVAL;
> +
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_ACTIVATE_FW,
> +		.size_in = sizeof(*activate),
> +		.payload_in = activate,
> +	};
> +
> +	/* Only offline activation supported for now */
> +	activate->action = CXL_FW_ACTIVATE_OFFLINE;
> +	activate->slot = slot;
> +
> +	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
> +}
> +
> +/**
> + * cxl_mem_abort_fw_xfer - Abort an in-progress FW transfer
> + * @cxlds: The device data for the operation
> + *
> + * Abort an in-progress firmware transfer for the device specified.
> + *
> + * Return: 0 if no error: or the result of the mailbox command.
> + *
> + * See CXL-3.0 8.2.9.3.2 Transfer FW
> + */
> +static int cxl_mem_abort_fw_xfer(struct cxl_dev_state *cxlds)
> +{
> +	struct cxl_mbox_transfer_fw *transfer;
> +	struct cxl_mbox_cmd mbox_cmd;
> +	int rc;
> +
> +	transfer = kzalloc(sizeof(*transfer), GFP_KERNEL);

Even though the extra part is size 0 I'd use
struct_size() to make that explicit.

> +	if (!transfer)
> +		return -ENOMEM;
> +
> +	/* Set a 1s poll interval and a total wait time of 5 minutes */
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_TRANSFER_FW,
> +		.size_in = sizeof(*transfer),
> +		.payload_in = transfer,
> +		.poll_interval = 1000,
> +		.poll_count = 300,
> +	};
> +
> +	transfer->action = CXL_FW_TRANSFER_ACTION_ABORT;
> +
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
> +	kfree(transfer);
> +	return rc;
> +}
> +

> +
> +static enum fw_upload_err cxl_fw_prepare(struct fw_upload *fwl, const u8 *data,
> +					 u32 size)
> +{
> +	struct cxl_memdev *cxlmd = fwl->dd_handle;
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +
> +	if (!size)
> +		return FW_UPLOAD_ERR_INVALID_SIZE;
> +
> +	if (size + sizeof(struct cxl_mbox_transfer_fw) < cxlds->payload_size)
> +		cxlds->fw.oneshot = true;
> +	else
> +		cxlds->fw.oneshot = false;

	cxlds->fw.oneshot = struct_size(..) < cxlds->payload_size;

> +
> +	if (cxl_mem_get_fw_info(cxlds))
> +		return FW_UPLOAD_ERR_HW_ERROR;
> +
> +	if (cxlds->fw.cancel) {
> +		cxl_fw_cleanup(fwl);
> +		return FW_UPLOAD_ERR_CANCELED;
> +	}
> +
> +	cxlds->fw.clear_to_send = true;
> +	return FW_UPLOAD_ERR_NONE;
> +}
> +
> +static enum fw_upload_err cxl_fw_write(struct fw_upload *fwl, const u8 *data,
> +				       u32 offset, u32 size, u32 *written)
> +{
> +	struct cxl_memdev *cxlmd = fwl->dd_handle;
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_mbox_transfer_fw *transfer;
> +	struct cxl_mbox_cmd mbox_cmd;
> +	int rc, num_slots, cur_slot;
> +	u32 cur_size, remaining;
> +	size_t size_in;
> +
> +	*written = 0;
> +
> +	/* Offset has to be aligned to 128B (CXL-3.0 8.2.9.3.2 Table 8-57) */
> +	if (offset % CXL_FW_TRANSFER_OFFSET_ALIGN) {
> +		dev_err(&cxlmd->dev,
> +			"misaligned offset for FW transfer slice (%u)\n",
> +			offset);
> +		return FW_UPLOAD_ERR_RW_ERROR;
> +	}
> +
> +	/* Pick transfer size based on cxlds->payload_size */
> +	cur_size = ALIGN_DOWN(min((size_t)size,
> +				  cxlds->payload_size - sizeof(*transfer)),
> +			      CXL_FW_TRANSFER_OFFSET_ALIGN);
> +	remaining = size - cur_size;
> +	size_in = cur_size + sizeof(*transfer);

struct_size()?

> +
> +	mutex_lock(&cxlds->fw.fw_mutex);
> +	if (!cxlds->fw.clear_to_send) {
> +		dev_err(&cxlmd->dev, "firmware_loader prep not done\n");
> +		rc = FW_UPLOAD_ERR_BUSY;
> +		goto out_unlock;
> +	}
> +
> +	if (cxlds->fw.cancel) {
> +		rc = cxl_mem_abort_fw_xfer(cxlds);
> +		if (rc < 0)
> +			dev_err(&cxlmd->dev, "Error aborting FW transfer: %d\n",
> +				rc);
> +		cxl_fw_cleanup(fwl);
> +		rc = FW_UPLOAD_ERR_CANCELED;
> +		goto out_unlock;
> +	}
> +
> +	/* Determine next slot from fw_info */
> +	num_slots = cxlds->fw.info.num_slots;
> +	cur_slot = FIELD_GET(CXL_FW_INFO_CUR_SLOT_MASK,
> +			     cxlds->fw.info.slot_info);
> +
> +	/*
> +	 * Slot numbers are 1-indexed
> +	 * cur_slot is the 0-indexed next_slot (i.e. 'cur_slot - 1 + 1')
> +	 * Check for rollover using modulo, and 1-index it by adding 1
> +	 */
> +	cxlds->fw.next_slot = (cur_slot % num_slots) + 1;
> +
> +	/* Do the transfer via mailbox cmd */
> +	transfer = kzalloc(size_in, GFP_KERNEL);
> +	if (!transfer) {
> +		rc = FW_UPLOAD_ERR_RW_ERROR;
> +		goto out_unlock;
> +	}
> +
> +	transfer->offset = cpu_to_le32(offset / CXL_FW_TRANSFER_OFFSET_ALIGN);
> +	memcpy(transfer->data, data + offset, cur_size);
> +	if (cxlds->fw.oneshot) {
> +		transfer->action = CXL_FW_TRANSFER_ACTION_FULL;
> +		transfer->slot = cxlds->fw.next_slot;
> +	} else {
> +		if (offset == 0) {
> +			transfer->action = CXL_FW_TRANSFER_ACTION_START;
> +		} else if (remaining == 0) {
> +			transfer->action = CXL_FW_TRANSFER_ACTION_END;
> +			transfer->slot = cxlds->fw.next_slot;
> +		} else {
> +			transfer->action = CXL_FW_TRANSFER_ACTION_CONTINUE;
> +		}
> +	}
> +
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_TRANSFER_FW,
> +		.size_in = size_in,
> +		.payload_in = transfer,
> +	};
> +
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
> +	if (rc < 0) {
> +		kfree(transfer);

Where is this freed in non error paths?

> +		rc = FW_UPLOAD_ERR_RW_ERROR;
> +		goto out_unlock;
> +	}
> +
> +	*written = cur_size;
> +
> +	/* Activate FW if oneshot or if the last slice was written */
> +	if (cxlds->fw.oneshot || remaining == 0) {
> +		dev_dbg(&cxlmd->dev, "Activating firmware slot: %d\n",
> +			cxlds->fw.next_slot);
> +		rc = cxl_mem_activate_fw(cxlds, cxlds->fw.next_slot);
> +		if (rc < 0) {
> +			dev_err(&cxlmd->dev, "Error activating firmware: %d\n",
> +				rc);
> +			rc = FW_UPLOAD_ERR_HW_ERROR;
> +			goto out_unlock;
> +		}
> +	}
> +
> +	rc = FW_UPLOAD_ERR_NONE;
> +
> +out_unlock:
> +	mutex_unlock(&cxlds->fw.fw_mutex);
> +	return rc;
> +}
> +
> +static enum fw_upload_err cxl_fw_poll_complete(struct fw_upload *fwl)
> +{
> +	struct cxl_memdev *cxlmd = fwl->dd_handle;
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	int rc;
> +
> +	/*
> +	 * cxl_internal_send_cmd() handles background operations synchronously.
> +	 * No need to wait for completions here - any errors would've been
> +	 * reported and handled during the ->write() call(s).
> +	 * Just check if a cancel request was received, and return success.
> +	 */
> +	mutex_lock(&cxlds->fw.fw_mutex);
> +	if (cxlds->fw.cancel) {
> +		rc = cxl_mem_abort_fw_xfer(cxlds);
> +		if (rc < 0)
> +			dev_err(&cxlmd->dev, "Error aborting FW transfer: %d\n",
> +				rc);
> +		cxl_fw_cleanup(fwl);
> +		rc = FW_UPLOAD_ERR_CANCELED;
> +		goto out_unlock;
> +	}
> +
> +	rc = FW_UPLOAD_ERR_NONE;

maybe use an else and drop the goto?

> +
> +out_unlock:
> +	mutex_unlock(&cxlds->fw.fw_mutex);
> +	return rc;
> +}
> +
> +static void cxl_fw_cancel(struct fw_upload *fwl)
> +{
> +	struct cxl_memdev *cxlmd = fwl->dd_handle;
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +
> +	mutex_lock(&cxlds->fw.fw_mutex);

Local variable for fw perhaps? Lots of cxlds->fw otherwise.
Same in some places above as well.


> +	cxlds->fw.cancel = true;
> +	mutex_unlock(&cxlds->fw.fw_mutex);
> +}
> +
> +static const struct fw_upload_ops cxl_memdev_fw_ops = {
> +        .prepare = cxl_fw_prepare,
> +        .write = cxl_fw_write,
> +        .poll_complete = cxl_fw_poll_complete,
> +        .cancel = cxl_fw_cancel,
> +        .cleanup = cxl_fw_cleanup,
> +};
> +
> +static int cxl_memdev_setup_fw_upload(struct cxl_memdev *cxlmd)
> +{
> +	const char *fw_name, *truncate;
> +	struct fw_upload *fwl;
> +	unsigned int len;
> +
> +	fw_name = dev_name(&cxlmd->dev);
> +	truncate = strstr(fw_name, ".auto");
> +	len = (truncate) ? truncate - fw_name : strlen(fw_name);

Why first set of brackets?

> +	cxlmd->fw_name = kmemdup_nul(fw_name, len, GFP_KERNEL);

Can fail in theory, does that matter?

> +
> +	fwl = firmware_upload_register(THIS_MODULE, &cxlmd->dev, cxlmd->fw_name,
> +				       &cxl_memdev_fw_ops, cxlmd);
> +	if (IS_ERR(fwl)) {
> +		dev_err(&cxlmd->dev, "Failed to register firmware loader\n");
> +		kfree(cxlmd->fw_name);
> +		return PTR_ERR(fwl);
> +	}
> +
> +	cxlmd->fwl = fwl;
> +	return 0;
> +}
> +
>  static const struct file_operations cxl_memdev_fops = {
>  	.owner = THIS_MODULE,
>  	.unlocked_ioctl = cxl_memdev_ioctl,
> @@ -352,11 +669,18 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
>  	if (rc)
>  		goto err;
>  
> +	rc = cxl_memdev_setup_fw_upload(cxlmd);

I'd expect this to be side effect free on error.  And it seems to be...
So why the cleanup below?  Looks like fw_name freed twice...

I'd be tempted to push one or more explicit devm_add_action_or_reset() calls into
there so that we can keep the setup and cleanup local in the code.  Also don't need
to do the kfree by hand above. Pity there is no devm_kmemdup_nul()



> +	if (rc)
> +		goto err_fw;
> +
>  	rc = devm_add_action_or_reset(cxlds->dev, cxl_memdev_unregister, cxlmd);
>  	if (rc)
>  		return ERR_PTR(rc);

>  	return cxlmd;
>  
> +err_fw:
> +	firmware_upload_unregister(cxlmd->fwl);
If you need to call this on firmware_upload_register() failing I'd be somewhat surprised...

> +	kfree(cxlmd->fw_name);
>  err:
>  	/*
>  	 * The cdev was briefly live, shutdown any ioctl operations that

Re: [PATCH 3/4] cxl: add a firmware update mechanism using the sysfs firmware loader

[Alison Schofield]

On Fri, Apr 21, 2023 at 09:09:27PM -0600, Vishal Verma wrote:
> The sysfs based firmware loader mechanism was created to easily allow
> userspace to upload firmware images to FPGA cards. This also happens to
> be pretty suitable to create a user-initiated but kernel-controlled
> firmware update mechanism for CXL devices, using the CXL specified
> mailbox commands.
> 
> Since firmware update commands can be long-running, and can be processed
> in the background by the endpoint device, it is desirable to have the
> ability to chunk the firmware transfer down to smaller pieces, so that
> one operation does not monopolize the mailbox, locking out any other
> long running background commands entirely - e.g. security commands like
> 'sanitize' or poison scanning operations.
> 
> The firmware loader mechanism allows a natural way to perform this
> chunking, as after each mailbox command, that is restricted to the
> maximum mailbox payload size, the cxl memdev driver relinquishes control
> back to the fw_loader system and awaits the next chunk of data to
> transfer. This opens opportunities for other background commands to
> access the mailbox and send their own slices of background commands.
> 
> Add the necessary helpers and state tracking to be able to perform the
> 'Get FW Info', 'Transfer FW', and 'Activate FW' mailbox commands as
> described in the CXL spec. Wire these up to the firmware loader
> callbacks, and register with that system to create the memX/firmware/
> sysfs ABI.

I just have a quick drive-by comment for you....

> 
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  drivers/cxl/cxlmem.h                    |  79 ++++++++
>  drivers/cxl/core/mbox.c                 |   1 +
>  drivers/cxl/core/memdev.c               | 324 ++++++++++++++++++++++++++++++++
>  Documentation/ABI/testing/sysfs-bus-cxl |  11 ++
>  drivers/cxl/Kconfig                     |   1 +
>  5 files changed, 416 insertions(+)
> 
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 8c3302fc7738..0ecee5b558f4 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h

snip

> + * Get Firmware Info
> + * CXL rev 3.0 section 8.2.9.3.1; Table 8-56
> + */
> +struct cxl_mbox_get_fw_info {
> +	u8 num_slots;
> +	u8 slot_info;
> +	u8 activation_cap;
> +	u8 reserved[13];
> +	char slot_1_revision[0x10];
> +	char slot_2_revision[0x10];
> +	char slot_3_revision[0x10];
> +	char slot_4_revision[0x10];

The practice here is to use decimals [16]

> +} __packed;
> +

snip

> + * Transfer Firmware Input Payload
> + * CXL rev 3.0 section 8.2.9.3.2; Table 8-57
> + */
> +struct cxl_mbox_transfer_fw {
> +	u8 action;
> +	u8 slot;
> +	u8 reserved[2];
> +	__le32 offset;
> +	u8 reserved2[0x78];

Here too.

That's all for now.

Re: [PATCH 3/4] cxl: add a firmware update mechanism using the sysfs firmware loader

[Verma, Vishal L]

On Thu, 2023-05-18 at 19:58 -0700, Alison Schofield wrote:
> > 
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index 8c3302fc7738..0ecee5b558f4 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> 
> snip
> 
> > + * Get Firmware Info
> > + * CXL rev 3.0 section 8.2.9.3.1; Table 8-56
> > + */
> > +struct cxl_mbox_get_fw_info {
> > +       u8 num_slots;
> > +       u8 slot_info;
> > +       u8 activation_cap;
> > +       u8 reserved[13];
> > +       char slot_1_revision[0x10];
> > +       char slot_2_revision[0x10];
> > +       char slot_3_revision[0x10];
> > +       char slot_4_revision[0x10];
> 
> The practice here is to use decimals [16]

I looked around, and see cxl_identify, cxl_event_mem_module, and
cxl_event_dram all have a few hex ones thrown in each. So I looked to
the spec to see if there was any consistency there, and unfortunately
it isn't either. The spec does say 'Length in bytes' 16 for these fw
revision fields though, so I think it makes sense changing these to
decimal..

> 
> > +} __packed;
> > +
> 
> snip
> 
> > + * Transfer Firmware Input Payload
> > + * CXL rev 3.0 section 8.2.9.3.2; Table 8-57
> > + */
> > +struct cxl_mbox_transfer_fw {
> > +       u8 action;
> > +       u8 slot;
> > +       u8 reserved[2];
> > +       __le32 offset;
> > +       u8 reserved2[0x78];
> 
> Here too.

..however for this one the spec has '78h'. I think we should just match
the spec in these cases so anyone cross referencing the header and spec
is saved from doing a bunch of math for these.

> 
> That's all for now.
> 
Thanks for taking a look!

Re: [PATCH 3/4] cxl: add a firmware update mechanism using the sysfs firmware loader

[Dan Williams]

Verma, Vishal L wrote:
> On Thu, 2023-05-18 at 19:58 -0700, Alison Schofield wrote:
> > > 
> > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > > index 8c3302fc7738..0ecee5b558f4 100644
> > > --- a/drivers/cxl/cxlmem.h
> > > +++ b/drivers/cxl/cxlmem.h
> > 
> > snip
> > 
> > > + * Get Firmware Info
> > > + * CXL rev 3.0 section 8.2.9.3.1; Table 8-56
> > > + */
> > > +struct cxl_mbox_get_fw_info {
> > > +       u8 num_slots;
> > > +       u8 slot_info;
> > > +       u8 activation_cap;
> > > +       u8 reserved[13];
> > > +       char slot_1_revision[0x10];
> > > +       char slot_2_revision[0x10];
> > > +       char slot_3_revision[0x10];
> > > +       char slot_4_revision[0x10];
> > 
> > The practice here is to use decimals [16]
> 
> I looked around, and see cxl_identify, cxl_event_mem_module, and
> cxl_event_dram all have a few hex ones thrown in each. So I looked to
> the spec to see if there was any consistency there, and unfortunately
> it isn't either. The spec does say 'Length in bytes' 16 for these fw
> revision fields though, so I think it makes sense changing these to
> decimal..

I had asked for decimal in the event and poison bits, but yes, we are not
consistent. Those ones stand out because even the spec has them as
decimal. Not a deal breaker either way.

RE: [PATCH 3/4] cxl: add a firmware update mechanism using the sysfs firmware loader

[Dan Williams]

Vishal Verma wrote:
> The sysfs based firmware loader mechanism was created to easily allow
> userspace to upload firmware images to FPGA cards. This also happens to
> be pretty suitable to create a user-initiated but kernel-controlled
> firmware update mechanism for CXL devices, using the CXL specified
> mailbox commands.
> 
> Since firmware update commands can be long-running, and can be processed
> in the background by the endpoint device, it is desirable to have the
> ability to chunk the firmware transfer down to smaller pieces, so that
> one operation does not monopolize the mailbox, locking out any other
> long running background commands entirely - e.g. security commands like
> 'sanitize' or poison scanning operations.
> 
> The firmware loader mechanism allows a natural way to perform this
> chunking, as after each mailbox command, that is restricted to the
> maximum mailbox payload size, the cxl memdev driver relinquishes control
> back to the fw_loader system and awaits the next chunk of data to
> transfer. This opens opportunities for other background commands to
> access the mailbox and send their own slices of background commands.
> 
> Add the necessary helpers and state tracking to be able to perform the
> 'Get FW Info', 'Transfer FW', and 'Activate FW' mailbox commands as
> described in the CXL spec. Wire these up to the firmware loader
> callbacks, and register with that system to create the memX/firmware/
> sysfs ABI.
> 
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  drivers/cxl/cxlmem.h                    |  79 ++++++++
>  drivers/cxl/core/mbox.c                 |   1 +
>  drivers/cxl/core/memdev.c               | 324 ++++++++++++++++++++++++++++++++
>  Documentation/ABI/testing/sysfs-bus-cxl |  11 ++
>  drivers/cxl/Kconfig                     |   1 +
>  5 files changed, 416 insertions(+)
> 
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 8c3302fc7738..0ecee5b558f4 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -48,6 +48,8 @@ struct cxl_memdev {
>  	struct work_struct detach_work;
>  	struct cxl_nvdimm_bridge *cxl_nvb;
>  	struct cxl_nvdimm *cxl_nvd;
> +	struct fw_upload *fwl;
> +	const char *fw_name;
>  	int id;
>  	int depth;
>  };
> @@ -220,6 +222,80 @@ struct cxl_event_state {
>  	struct mutex log_lock;
>  };
>  
> +/*
> + * Get Firmware Info
> + * CXL rev 3.0 section 8.2.9.3.1; Table 8-56
> + */
> +struct cxl_mbox_get_fw_info {
> +	u8 num_slots;
> +	u8 slot_info;
> +	u8 activation_cap;
> +	u8 reserved[13];
> +	char slot_1_revision[0x10];
> +	char slot_2_revision[0x10];
> +	char slot_3_revision[0x10];
> +	char slot_4_revision[0x10];
> +} __packed;
> +
> +#define CXL_FW_INFO_CUR_SLOT_MASK	GENMASK(2, 0)
> +#define CXL_FW_INFO_NEXT_SLOT_MASK	GENMASK(5, 3)
> +#define CXL_FW_INFO_NEXT_SLOT_SHIFT	3
> +#define CXL_FW_INFO_HAS_LIVE_ACTIVATE	BIT(0)
> +
> +/*
> + * Transfer Firmware Input Payload
> + * CXL rev 3.0 section 8.2.9.3.2; Table 8-57
> + */
> +struct cxl_mbox_transfer_fw {
> +	u8 action;
> +	u8 slot;
> +	u8 reserved[2];
> +	__le32 offset;
> +	u8 reserved2[0x78];
> +	u8 data[];
> +} __packed;
> +
> +#define CXL_FW_TRANSFER_ACTION_FULL	0x0
> +#define CXL_FW_TRANSFER_ACTION_START	0x1
> +#define CXL_FW_TRANSFER_ACTION_CONTINUE	0x2
> +#define CXL_FW_TRANSFER_ACTION_END	0x3
> +#define CXL_FW_TRANSFER_ACTION_ABORT	0x4
> +#define CXL_FW_TRANSFER_OFFSET_ALIGN	128
> +
> +/*
> + * Activate Firmware Input Payload
> + * CXL rev 3.0 section 8.2.9.3.3; Table 8-58
> + */
> +struct cxl_mbox_activate_fw {
> +	u8 action;
> +	u8 slot;
> +} __packed;
> +
> +#define CXL_FW_ACTIVATE_ONLINE		0x0
> +#define CXL_FW_ACTIVATE_OFFLINE		0x1
> +
> +/**
> + * struct cxl_fw_state - Firmware upload / activation state
> + *
> + * @fw_mutex: Mutex to serialize fw update requests
> + * @clear_to_send: Initial checks done, ready to start FW transfer
> + * @cancel: Cancel any in-progress FW upload
> + * @next_slot: Slot number for the new firmware
> + * @info: Get FW Info structure
> + * @activate: Activate FW structure
> + * @transfer: Transfer FW Info structure
> + */
> +struct cxl_fw_state {
> +	struct mutex fw_mutex;
> +	bool clear_to_send;
> +	bool oneshot;
> +	bool cancel;
> +	int next_slot;
> +	struct cxl_mbox_get_fw_info info;
> +	struct cxl_mbox_activate_fw activate;
> +	struct cxl_mbox_transfer_fw *transfer;
> +};
> +
>  /**
>   * struct cxl_dev_state - The driver device state
>   *
> @@ -256,6 +332,7 @@ struct cxl_event_state {
>   * @serial: PCIe Device Serial Number
>   * @doe_mbs: PCI DOE mailbox array
>   * @event: event log driver state
> + * @fw: firmware upload / activation state
>   * @mbox_send: @dev specific transport for transmitting mailbox commands
>   *
>   * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
> @@ -295,6 +372,7 @@ struct cxl_dev_state {
>  	struct xarray doe_mbs;
>  
>  	struct cxl_event_state event;
> +	struct cxl_fw_state fw;
>  
>  	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
>  };
> @@ -307,6 +385,7 @@ enum cxl_opcode {
>  	CXL_MBOX_OP_GET_EVT_INT_POLICY	= 0x0102,
>  	CXL_MBOX_OP_SET_EVT_INT_POLICY	= 0x0103,
>  	CXL_MBOX_OP_GET_FW_INFO		= 0x0200,
> +	CXL_MBOX_OP_TRANSFER_FW		= 0x0201,
>  	CXL_MBOX_OP_ACTIVATE_FW		= 0x0202,
>  	CXL_MBOX_OP_SET_TIMESTAMP	= 0x0301,
>  	CXL_MBOX_OP_GET_SUPPORTED_LOGS	= 0x0400,
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 4b0c7564d350..bdeb5495d1cc 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1120,6 +1120,7 @@ struct cxl_dev_state *cxl_dev_state_create(struct device *dev)
>  
>  	mutex_init(&cxlds->mbox_mutex);
>  	mutex_init(&cxlds->event.log_lock);
> +	mutex_init(&cxlds->fw.fw_mutex);
>  	cxlds->dev = dev;
>  
>  	return cxlds;
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 28a05f2fe32d..8f61b1e526ae 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2020 Intel Corporation. */
>  
> +#include <linux/firmware.h>
>  #include <linux/device.h>
>  #include <linux/slab.h>
>  #include <linux/idr.h>
> @@ -214,6 +215,8 @@ static void cxl_memdev_unregister(void *_cxlmd)
>  	struct cxl_memdev *cxlmd = _cxlmd;
>  	struct device *dev = &cxlmd->dev;
>  
> +	firmware_upload_unregister(cxlmd->fwl);

I think this wants to be a separate devm_add_action_or_reset() callback
registered from the setup side.

> +	kfree(cxlmd->fw_name);

Not sure ->fw_name is needed, see below.

>  	cxl_memdev_shutdown(dev);
>  	cdev_device_del(&cxlmd->cdev, dev);
>  	put_device(dev);
> @@ -315,6 +318,320 @@ static int cxl_memdev_release_file(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> +/**
> + * cxl_mem_get_fw_info - Get Firmware info
> + * @cxlds: The device data for the operation
> + *
> + * Retrieve firmware info for the device specified.
> + *
> + * Return: 0 if no error: or the result of the mailbox command.
> + *
> + * See CXL-3.0 8.2.9.3.1 Get FW Info
> + */
> +static int cxl_mem_get_fw_info(struct cxl_dev_state *cxlds)
> +{
> +	struct cxl_mbox_get_fw_info *info = &cxlds->fw.info;
> +	struct cxl_mbox_cmd mbox_cmd;
> +
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_GET_FW_INFO,
> +		.size_out = sizeof(*info),
> +		.payload_out = info,
> +	};
> +
> +	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
> +}
> +
> +/**
> + * cxl_mem_activate_fw - Activate Firmware
> + * @cxlds: The device data for the operation
> + * @slot: slot number to activate
> + *
> + * Activate firmware in a given slot for the device specified.
> + *
> + * Return: 0 if no error: or the result of the mailbox command.
> + *
> + * See CXL-3.0 8.2.9.3.3 Activate FW
> + */
> +static int cxl_mem_activate_fw(struct cxl_dev_state *cxlds, int slot)
> +{
> +	struct cxl_mbox_activate_fw *activate = &cxlds->fw.activate;
> +	struct cxl_mbox_cmd mbox_cmd;
> +
> +	if (slot == 0 || slot > cxlds->fw.info.num_slots)
> +		return -EINVAL;
> +
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_ACTIVATE_FW,
> +		.size_in = sizeof(*activate),
> +		.payload_in = activate,
> +	};
> +
> +	/* Only offline activation supported for now */
> +	activate->action = CXL_FW_ACTIVATE_OFFLINE;
> +	activate->slot = slot;
> +
> +	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
> +}
> +
> +/**
> + * cxl_mem_abort_fw_xfer - Abort an in-progress FW transfer
> + * @cxlds: The device data for the operation
> + *
> + * Abort an in-progress firmware transfer for the device specified.
> + *
> + * Return: 0 if no error: or the result of the mailbox command.
> + *
> + * See CXL-3.0 8.2.9.3.2 Transfer FW
> + */
> +static int cxl_mem_abort_fw_xfer(struct cxl_dev_state *cxlds)
> +{
> +	struct cxl_mbox_transfer_fw *transfer;
> +	struct cxl_mbox_cmd mbox_cmd;
> +	int rc;
> +
> +	transfer = kzalloc(sizeof(*transfer), GFP_KERNEL);
> +	if (!transfer)
> +		return -ENOMEM;
> +
> +	/* Set a 1s poll interval and a total wait time of 5 minutes */
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_TRANSFER_FW,
> +		.size_in = sizeof(*transfer),
> +		.payload_in = transfer,
> +		.poll_interval = 1000,
> +		.poll_count = 300,

Hung task timeouts start firing at 120 seconds
(CONFIG_DEFAULT_HUNG_TASK_TIMEOUT). The block layer gives up on disk I/O
by default at 30 seconds. Is 300 just an arbitrary choice and can we get
by with 30 for now?


> +	};
> +
> +	transfer->action = CXL_FW_TRANSFER_ACTION_ABORT;
> +
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
> +	kfree(transfer);
> +	return rc;
> +}
> +
> +static void cxl_fw_cleanup(struct fw_upload *fwl)
> +{
> +	struct cxl_memdev *cxlmd = fwl->dd_handle;
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +
> +	cxlds->fw.clear_to_send = false;
> +	cxlds->fw.cancel = false;
> +	cxlds->fw.next_slot = 0;
> +}
> +
> +static enum fw_upload_err cxl_fw_prepare(struct fw_upload *fwl, const u8 *data,
> +					 u32 size)
> +{
> +	struct cxl_memdev *cxlmd = fwl->dd_handle;
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +
> +	if (!size)
> +		return FW_UPLOAD_ERR_INVALID_SIZE;
> +
> +	if (size + sizeof(struct cxl_mbox_transfer_fw) < cxlds->payload_size)
> +		cxlds->fw.oneshot = true;
> +	else
> +		cxlds->fw.oneshot = false;
> +
> +	if (cxl_mem_get_fw_info(cxlds))
> +		return FW_UPLOAD_ERR_HW_ERROR;
> +
> +	if (cxlds->fw.cancel) {
> +		cxl_fw_cleanup(fwl);
> +		return FW_UPLOAD_ERR_CANCELED;
> +	}
> +
> +	cxlds->fw.clear_to_send = true;
> +	return FW_UPLOAD_ERR_NONE;
> +}
> +
> +static enum fw_upload_err cxl_fw_write(struct fw_upload *fwl, const u8 *data,
> +				       u32 offset, u32 size, u32 *written)
> +{
> +	struct cxl_memdev *cxlmd = fwl->dd_handle;
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_mbox_transfer_fw *transfer;
> +	struct cxl_mbox_cmd mbox_cmd;
> +	int rc, num_slots, cur_slot;
> +	u32 cur_size, remaining;
> +	size_t size_in;
> +
> +	*written = 0;
> +
> +	/* Offset has to be aligned to 128B (CXL-3.0 8.2.9.3.2 Table 8-57) */
> +	if (offset % CXL_FW_TRANSFER_OFFSET_ALIGN) {

Could use IS_ALIGNED() here to save open coding it.

> +		dev_err(&cxlmd->dev,
> +			"misaligned offset for FW transfer slice (%u)\n",
> +			offset);
> +		return FW_UPLOAD_ERR_RW_ERROR;
> +	}
> +
> +	/* Pick transfer size based on cxlds->payload_size */
> +	cur_size = ALIGN_DOWN(min((size_t)size,
> +				  cxlds->payload_size - sizeof(*transfer)),

A few notes.

- min() in terms of given type is min_t() to save that open-coded cast

- what is the align for? I.e. @size must bw 128-byte aligned,
  ->payload_size is a power of 2 starting at 256 bytes, and
  sizeof(*transfer) is 128.

So I think this can just be:

	cur_size = min_t(size_t, size, cxlds->payload_size - sizeof(*transfer));




> +			      CXL_FW_TRANSFER_OFFSET_ALIGN);
> +	remaining = size - cur_size;
> +	size_in = cur_size + sizeof(*transfer);
> +
> +	mutex_lock(&cxlds->fw.fw_mutex);

What is this lock protecting? I.e. will the fw_loader really try to send
multiple overlapping firmware update attempts?

> +	if (!cxlds->fw.clear_to_send) {

I tend to prefer atomic bitops for state flags, especially if that lets
you get away without a lock.

> +		dev_err(&cxlmd->dev, "firmware_loader prep not done\n");
> +		rc = FW_UPLOAD_ERR_BUSY;
> +		goto out_unlock;
> +	}
> +
> +	if (cxlds->fw.cancel) {
> +		rc = cxl_mem_abort_fw_xfer(cxlds);
> +		if (rc < 0)
> +			dev_err(&cxlmd->dev, "Error aborting FW transfer: %d\n",
> +				rc);
> +		cxl_fw_cleanup(fwl);
> +		rc = FW_UPLOAD_ERR_CANCELED;
> +		goto out_unlock;
> +	}
> +
> +	/* Determine next slot from fw_info */
> +	num_slots = cxlds->fw.info.num_slots;
> +	cur_slot = FIELD_GET(CXL_FW_INFO_CUR_SLOT_MASK,
> +			     cxlds->fw.info.slot_info);
> +
> +	/*
> +	 * Slot numbers are 1-indexed
> +	 * cur_slot is the 0-indexed next_slot (i.e. 'cur_slot - 1 + 1')
> +	 * Check for rollover using modulo, and 1-index it by adding 1
> +	 */
> +	cxlds->fw.next_slot = (cur_slot % num_slots) + 1;
> +
> +	/* Do the transfer via mailbox cmd */
> +	transfer = kzalloc(size_in, GFP_KERNEL);
> +	if (!transfer) {
> +		rc = FW_UPLOAD_ERR_RW_ERROR;
> +		goto out_unlock;
> +	}
> +
> +	transfer->offset = cpu_to_le32(offset / CXL_FW_TRANSFER_OFFSET_ALIGN);
> +	memcpy(transfer->data, data + offset, cur_size);
> +	if (cxlds->fw.oneshot) {
> +		transfer->action = CXL_FW_TRANSFER_ACTION_FULL;
> +		transfer->slot = cxlds->fw.next_slot;
> +	} else {
> +		if (offset == 0) {
> +			transfer->action = CXL_FW_TRANSFER_ACTION_START;
> +		} else if (remaining == 0) {
> +			transfer->action = CXL_FW_TRANSFER_ACTION_END;
> +			transfer->slot = cxlds->fw.next_slot;
> +		} else {
> +			transfer->action = CXL_FW_TRANSFER_ACTION_CONTINUE;
> +		}
> +	}
> +
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_TRANSFER_FW,
> +		.size_in = size_in,
> +		.payload_in = transfer,
> +	};
> +
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
> +	if (rc < 0) {
> +		kfree(transfer);
> +		rc = FW_UPLOAD_ERR_RW_ERROR;
> +		goto out_unlock;
> +	}
> +
> +	*written = cur_size;
> +
> +	/* Activate FW if oneshot or if the last slice was written */
> +	if (cxlds->fw.oneshot || remaining == 0) {
> +		dev_dbg(&cxlmd->dev, "Activating firmware slot: %d\n",
> +			cxlds->fw.next_slot);
> +		rc = cxl_mem_activate_fw(cxlds, cxlds->fw.next_slot);
> +		if (rc < 0) {
> +			dev_err(&cxlmd->dev, "Error activating firmware: %d\n",
> +				rc);
> +			rc = FW_UPLOAD_ERR_HW_ERROR;
> +			goto out_unlock;
> +		}
> +	}
> +
> +	rc = FW_UPLOAD_ERR_NONE;
> +
> +out_unlock:
> +	mutex_unlock(&cxlds->fw.fw_mutex);
> +	return rc;
> +}
> +
> +static enum fw_upload_err cxl_fw_poll_complete(struct fw_upload *fwl)
> +{
> +	struct cxl_memdev *cxlmd = fwl->dd_handle;
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	int rc;
> +
> +	/*
> +	 * cxl_internal_send_cmd() handles background operations synchronously.
> +	 * No need to wait for completions here - any errors would've been
> +	 * reported and handled during the ->write() call(s).
> +	 * Just check if a cancel request was received, and return success.
> +	 */
> +	mutex_lock(&cxlds->fw.fw_mutex);
> +	if (cxlds->fw.cancel) {
> +		rc = cxl_mem_abort_fw_xfer(cxlds);
> +		if (rc < 0)
> +			dev_err(&cxlmd->dev, "Error aborting FW transfer: %d\n",
> +				rc);
> +		cxl_fw_cleanup(fwl);
> +		rc = FW_UPLOAD_ERR_CANCELED;
> +		goto out_unlock;
> +	}
> +
> +	rc = FW_UPLOAD_ERR_NONE;
> +
> +out_unlock:
> +	mutex_unlock(&cxlds->fw.fw_mutex);
> +	return rc;
> +}
> +
> +static void cxl_fw_cancel(struct fw_upload *fwl)
> +{
> +	struct cxl_memdev *cxlmd = fwl->dd_handle;
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +
> +	mutex_lock(&cxlds->fw.fw_mutex);
> +	cxlds->fw.cancel = true;
> +	mutex_unlock(&cxlds->fw.fw_mutex);
> +}
> +
> +static const struct fw_upload_ops cxl_memdev_fw_ops = {
> +        .prepare = cxl_fw_prepare,
> +        .write = cxl_fw_write,
> +        .poll_complete = cxl_fw_poll_complete,
> +        .cancel = cxl_fw_cancel,
> +        .cleanup = cxl_fw_cleanup,
> +};
> +
> +static int cxl_memdev_setup_fw_upload(struct cxl_memdev *cxlmd)
> +{
> +	const char *fw_name, *truncate;
> +	struct fw_upload *fwl;
> +	unsigned int len;

I would have expected this to skip registering a firmware loader
interface if the device does not support the firmware update commands.
See how the poison code gated setup based on command availability.

> +
> +	fw_name = dev_name(&cxlmd->dev);
> +	truncate = strstr(fw_name, ".auto");
> +	len = (truncate) ? truncate - fw_name : strlen(fw_name);

What is this doing? The device name of a cxl_memdev will never have the
string ".auto", looks like unnecessary copy/pasta.

> +	cxlmd->fw_name = kmemdup_nul(fw_name, len, GFP_KERNEL);

Not sure this is needed either. AFAICS just pass dev_name(&cxlmd->dev)
and skip a separate string.

> +
> +	fwl = firmware_upload_register(THIS_MODULE, &cxlmd->dev, cxlmd->fw_name,
> +				       &cxl_memdev_fw_ops, cxlmd);
> +	if (IS_ERR(fwl)) {
> +		dev_err(&cxlmd->dev, "Failed to register firmware loader\n");
> +		kfree(cxlmd->fw_name);
> +		return PTR_ERR(fwl);
> +	}
> +
> +	cxlmd->fwl = fwl;
> +	return 0;
> +}
> +
>  static const struct file_operations cxl_memdev_fops = {
>  	.owner = THIS_MODULE,
>  	.unlocked_ioctl = cxl_memdev_ioctl,
> @@ -352,11 +669,18 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
>  	if (rc)
>  		goto err;
>  
> +	rc = cxl_memdev_setup_fw_upload(cxlmd);
> +	if (rc)
> +		goto err_fw;
> +
>  	rc = devm_add_action_or_reset(cxlds->dev, cxl_memdev_unregister, cxlmd);
>  	if (rc)
>  		return ERR_PTR(rc);
>  	return cxlmd;
>  
> +err_fw:
> +	firmware_upload_unregister(cxlmd->fwl);
> +	kfree(cxlmd->fw_name);
>  err:
>  	/*
>  	 * The cdev was briefly live, shutdown any ioctl operations that
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 3acf2f17a73f..e94237f5568d 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -58,6 +58,17 @@ Description:
>  		affinity for this device.
>  
>  
> +What:		/sys/bus/cxl/devices/memX/firmware/
> +Date:		April, 2023
> +KernelVersion:	v6.5
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(RW) Firmware uploader mechanism. The different files under
> +		this directory can be used to upload and activate new
> +		firmware for CXL devices. The interfaces under this are
> +		documented in sysfs-class-firmware.
> +
> +
>  What:		/sys/bus/cxl/devices/*/devtype
>  Date:		June, 2021
>  KernelVersion:	v5.14
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index ff4e78117b31..80d8e35fa049 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -82,6 +82,7 @@ config CXL_PMEM
>  config CXL_MEM
>  	tristate "CXL: Memory Expansion"
>  	depends on CXL_PCI
> +	select FW_UPLOAD
>  	default CXL_BUS
>  	help
>  	  The CXL.mem protocol allows a device to act as a provider of "System
> 
> -- 
> 2.40.0
> 

Re: [PATCH 3/4] cxl: add a firmware update mechanism using the sysfs firmware loader

[Dan Williams]

Verma, Vishal L wrote:
> On Mon, 2023-05-22 at 20:21 -0700, Dan Williams wrote:
> > Vishal Verma wrote:
> 
> <snip>
> Everything else not addressed here sounds good and I've made those
> changes.
> 
> > > 
> > > +       remaining = size - cur_size;
> > > +       size_in = cur_size + sizeof(*transfer);
> > > +
> > > +       mutex_lock(&cxlds->fw.fw_mutex);
> > 
> > What is this lock protecting? I.e. will the fw_loader really try to send
> > multiple overlapping firmware update attempts?
> 
> The lock is just to provide predictable points at which a cancel
> request may be intercepted. The loader won't try overlapping firmware
> transfer requests, but the ->cancel request comes from user space, and
> could happen while there is a transfer in progress. With the lock, the
> cancel will only be 'processed' after the current chunk's transfer is
> done. 

So right now cancel is only considered at certain points within either
the ->write() or ->poll_complete() callbacks. The firmware upload core
is guaranteeing that ->prepare(), ->write() and ->poll_complete() never
overlap for a given session, and that if any of those return an error
the upload session is terminated.

While the lock does flush in flight ->write() and ->prepare() it does
nothing to enforce when the cancellation is processed. It will still be
the case that the next invocation of ->write() or ->poll_complete() will
consider the cancel state before doing the next step.

I am failing to see what the lock is protecting. The other usage is for
checking that ->prepare() has completed before ->write() is invoked, but
again that is enforced by the firmware uploader workqueue.

I think the lock and the clear_to_send bit can be eliminated. Clear to
send is implied by ->prepare() succeeding. Convert cancel to an atomic
flag where cxl_fw_cancel() does:

set_bit(CXL_FW_CANCEL, &cxlds->fw.state);

...and cxl_fw_write() and cxl_fw_poll_complete() can just do:

if (test_and_clear_bit(CXL_FW_CANCEL, &cxlds->fw.state))
	do_cancel();

[PATCH 4/4] tools/testing/cxl: add firmware update emulation to CXL memdevs

[Vishal Verma]

Add emulation for the 'Get FW Info', 'Transfer FW', and 'Activate FW'
CXL mailbox commands to the cxl_test emulated memdevs to enable
end-to-end unit testing of a firmware update flow. For now, only
advertise an 'offline activation' capability as that is all the CXL
memdev driver currently implements.

Add some canned values for the serial number fields, and create a
platform device sysfs knob to calculate the sha256sum of the firmware
image that was received, so a unit test can compare it with the original
file that was uploaded.

Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 tools/testing/cxl/test/mem.c | 191 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 191 insertions(+)

diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index 9263b04d35f7..bc99cc673550 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -7,11 +7,14 @@
 #include <linux/delay.h>
 #include <linux/sizes.h>
 #include <linux/bits.h>
+#include <crypto/hash.h>
 #include <cxlmem.h>
 
 #include "trace.h"
 
 #define LSA_SIZE SZ_128K
+#define FW_SIZE SZ_64M
+#define FW_SLOTS 3
 #define DEV_SIZE SZ_2G
 #define EFFECT(x) (1U << x)
 
@@ -40,6 +43,18 @@ static struct cxl_cel_entry mock_cel[] = {
 		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_HEALTH_INFO),
 		.effect = cpu_to_le16(0),
 	},
+	{
+		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_FW_INFO),
+		.effect = cpu_to_le16(0),
+	},
+	{
+		.opcode = cpu_to_le16(CXL_MBOX_OP_TRANSFER_FW),
+		.effect = cpu_to_le16(EFFECT(0) | EFFECT(6)),
+	},
+	{
+		.opcode = cpu_to_le16(CXL_MBOX_OP_ACTIVATE_FW),
+		.effect = cpu_to_le16(EFFECT(0) | EFFECT(1)),
+	},
 };
 
 /* See CXL 2.0 Table 181 Get Health Info Output Payload */
@@ -91,6 +106,10 @@ struct mock_event_store {
 
 struct cxl_mockmem_data {
 	void *lsa;
+	void *fw;
+	int fw_slot;
+	int fw_staged;
+	size_t fw_size;
 	u32 security_state;
 	u8 user_pass[NVDIMM_PASSPHRASE_LEN];
 	u8 master_pass[NVDIMM_PASSPHRASE_LEN];
@@ -888,6 +907,88 @@ static int mock_health_info(struct cxl_dev_state *cxlds,
 	return 0;
 }
 
+static int mock_fw_info(struct cxl_dev_state *cxlds,
+			    struct cxl_mbox_cmd *cmd)
+{
+	struct cxl_mockmem_data *mdata = dev_get_drvdata(cxlds->dev);
+	struct cxl_mbox_get_fw_info fw_info = {
+		.num_slots = FW_SLOTS,
+		.slot_info = (mdata->fw_slot & 0x7) |
+			     ((mdata->fw_staged & 0x7) << 3),
+		.activation_cap = 0,
+	};
+
+	strcpy(fw_info.slot_1_revision, "cxl_test_fw_001");
+	strcpy(fw_info.slot_2_revision, "cxl_test_fw_002");
+	strcpy(fw_info.slot_3_revision, "cxl_test_fw_003");
+	strcpy(fw_info.slot_4_revision, "");
+
+	if (cmd->size_out < sizeof(fw_info))
+		return -EINVAL;
+
+	memcpy(cmd->payload_out, &fw_info, sizeof(fw_info));
+	return 0;
+}
+
+static int mock_transfer_fw(struct cxl_dev_state *cxlds,
+			    struct cxl_mbox_cmd *cmd)
+{
+	struct cxl_mbox_transfer_fw *transfer = cmd->payload_in;
+	struct cxl_mockmem_data *mdata = dev_get_drvdata(cxlds->dev);
+	void *fw = mdata->fw;
+	size_t offset, length;
+
+	offset = le32_to_cpu(transfer->offset) * CXL_FW_TRANSFER_OFFSET_ALIGN;
+	length = cmd->size_in - sizeof(*transfer);
+	if (offset + length > FW_SIZE)
+		return -EINVAL;
+
+	switch (transfer->action) {
+	case CXL_FW_TRANSFER_ACTION_FULL:
+		if (offset != 0)
+			return -EINVAL;
+		fallthrough;
+	case CXL_FW_TRANSFER_ACTION_END:
+		if (transfer->slot == 0 || transfer->slot > FW_SLOTS)
+			return -EINVAL;
+		mdata->fw_size = offset + length;
+		break;
+	case CXL_FW_TRANSFER_ACTION_START:
+	case CXL_FW_TRANSFER_ACTION_CONTINUE:
+	case CXL_FW_TRANSFER_ACTION_ABORT:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	memcpy(fw + offset, &transfer->data[0], length);
+	return 0;
+}
+
+static int mock_activate_fw(struct cxl_dev_state *cxlds,
+			    struct cxl_mbox_cmd *cmd)
+{
+	struct cxl_mbox_activate_fw *activate = cmd->payload_in;
+	struct cxl_mockmem_data *mdata = dev_get_drvdata(cxlds->dev);
+
+	if (activate->slot == 0 || activate->slot > FW_SLOTS)
+		return -EINVAL;
+
+	switch (activate->action) {
+	case CXL_FW_ACTIVATE_ONLINE:
+		mdata->fw_slot = activate->slot;
+		mdata->fw_staged = 0;
+		break;
+	case CXL_FW_ACTIVATE_OFFLINE:
+		mdata->fw_staged = activate->slot;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int cxl_mock_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
 {
 	struct device *dev = cxlds->dev;
@@ -942,6 +1043,15 @@ static int cxl_mock_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *
 	case CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE:
 		rc = mock_passphrase_secure_erase(cxlds, cmd);
 		break;
+	case CXL_MBOX_OP_GET_FW_INFO:
+		rc = mock_fw_info(cxlds, cmd);
+		break;
+	case CXL_MBOX_OP_TRANSFER_FW:
+		rc = mock_transfer_fw(cxlds, cmd);
+		break;
+	case CXL_MBOX_OP_ACTIVATE_FW:
+		rc = mock_activate_fw(cxlds, cmd);
+		break;
 	default:
 		break;
 	}
@@ -957,6 +1067,11 @@ static void label_area_release(void *lsa)
 	vfree(lsa);
 }
 
+static void fw_buf_release(void *buf)
+{
+	vfree(buf);
+}
+
 static bool is_rcd(struct platform_device *pdev)
 {
 	const struct platform_device_id *id = platform_get_device_id(pdev);
@@ -989,10 +1104,19 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 	mdata->lsa = vmalloc(LSA_SIZE);
 	if (!mdata->lsa)
 		return -ENOMEM;
+	mdata->fw = vmalloc(FW_SIZE);
+	if (!mdata->fw)
+		return -ENOMEM;
+	mdata->fw_slot = 2;
+
 	rc = devm_add_action_or_reset(dev, label_area_release, mdata->lsa);
 	if (rc)
 		return rc;
 
+	rc = devm_add_action_or_reset(dev, fw_buf_release, mdata->fw);
+	if (rc)
+		return rc;
+
 	cxlds = cxl_dev_state_create(dev);
 	if (IS_ERR(cxlds))
 		return PTR_ERR(cxlds);
@@ -1063,9 +1187,76 @@ static ssize_t security_lock_store(struct device *dev, struct device_attribute *
 
 static DEVICE_ATTR_RW(security_lock);
 
+struct sdesc {
+	struct shash_desc shash;
+	char ctx[];
+};
+
+static int do_sha256(u8 *data, unsigned int length, u8 *hash)
+{
+	struct crypto_shash *alg;
+	struct sdesc *sdesc;
+	size_t size;
+	int rc;
+
+	alg = crypto_alloc_shash("sha256", 0, 0);
+	if (IS_ERR(alg))
+		return PTR_ERR(alg);
+
+	size = sizeof(struct shash_desc) + crypto_shash_descsize(alg);
+	sdesc = kzalloc(size, GFP_KERNEL);
+	if (!sdesc) {
+		rc = -ENOMEM;
+		goto out_shash;
+	}
+
+	sdesc->shash.tfm = alg;
+	rc = crypto_shash_digest(&sdesc->shash, data, length, hash);
+
+	kfree(sdesc);
+out_shash:
+	crypto_free_shash(alg);
+	return rc;
+}
+
+#define CHECKSUM_SIZE 32
+
+static ssize_t fw_buf_checksum_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct cxl_mockmem_data *mdata = dev_get_drvdata(dev);
+	unsigned char *hstr, *hptr;
+	u8 hash[CHECKSUM_SIZE];
+	ssize_t written = 0;
+	int i, rc;
+
+	rc = do_sha256(mdata->fw, mdata->fw_size, &hash[0]);
+	if (rc) {
+		dev_err(dev, "error calculating checksum: %d\n", rc);
+		goto out_free;
+	}
+
+	hstr = kzalloc((CHECKSUM_SIZE * 2) + 1, GFP_KERNEL);
+	if (!hstr)
+		return -ENOMEM;
+
+	hptr = hstr;
+	for (i = 0; i < CHECKSUM_SIZE; i++)
+		hptr += sprintf(hptr, "%02x", hash[i]);
+
+	written = sysfs_emit(buf, "%s\n", hstr);
+
+out_free:
+	kfree(hstr);
+	return written;
+}
+
+static DEVICE_ATTR_RO(fw_buf_checksum);
+
 static struct attribute *cxl_mock_mem_attrs[] = {
 	&dev_attr_security_lock.attr,
 	&dev_attr_event_trigger.attr,
+	&dev_attr_fw_buf_checksum.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(cxl_mock_mem);

-- 
2.40.0

Re: [PATCH 4/4] tools/testing/cxl: add firmware update emulation to CXL memdevs

[Jonathan Cameron]

On Fri, 21 Apr 2023 21:09:28 -0600
Vishal Verma <vishal.l.verma@intel.com> wrote:

> Add emulation for the 'Get FW Info', 'Transfer FW', and 'Activate FW'
> CXL mailbox commands to the cxl_test emulated memdevs to enable
> end-to-end unit testing of a firmware update flow. For now, only
> advertise an 'offline activation' capability as that is all the CXL
> memdev driver currently implements.
> 
> Add some canned values for the serial number fields, and create a
> platform device sysfs knob to calculate the sha256sum of the firmware
> image that was received, so a unit test can compare it with the original
> file that was uploaded.
> 
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
Hi Vishal,

A few trivial comments inline,

Thanks,

Jonathan

> ---
>  tools/testing/cxl/test/mem.c | 191 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 191 insertions(+)
> 
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 9263b04d35f7..bc99cc673550 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -7,11 +7,14 @@
>  #include <linux/delay.h>
>  #include <linux/sizes.h>
>  #include <linux/bits.h>
> +#include <crypto/hash.h>
>  #include <cxlmem.h>
>  
>  #include "trace.h"
>  
>  #define LSA_SIZE SZ_128K
> +#define FW_SIZE SZ_64M
> +#define FW_SLOTS 3
>  #define DEV_SIZE SZ_2G
>  #define EFFECT(x) (1U << x)
>  
> @@ -40,6 +43,18 @@ static struct cxl_cel_entry mock_cel[] = {
>  		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_HEALTH_INFO),
>  		.effect = cpu_to_le16(0),
>  	},
> +	{
> +		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_FW_INFO),
> +		.effect = cpu_to_le16(0),
> +	},
> +	{
> +		.opcode = cpu_to_le16(CXL_MBOX_OP_TRANSFER_FW),
> +		.effect = cpu_to_le16(EFFECT(0) | EFFECT(6)),

Beginning to feel like some defines for each effect might be worth
adding.

> +	},
> +	{
> +		.opcode = cpu_to_le16(CXL_MBOX_OP_ACTIVATE_FW),
> +		.effect = cpu_to_le16(EFFECT(0) | EFFECT(1)),
> +	},
>  };

...

> +static int mock_transfer_fw(struct cxl_dev_state *cxlds,
> +			    struct cxl_mbox_cmd *cmd)
> +{
> +	struct cxl_mbox_transfer_fw *transfer = cmd->payload_in;
> +	struct cxl_mockmem_data *mdata = dev_get_drvdata(cxlds->dev);
> +	void *fw = mdata->fw;
> +	size_t offset, length;
> +
> +	offset = le32_to_cpu(transfer->offset) * CXL_FW_TRANSFER_OFFSET_ALIGN;
> +	length = cmd->size_in - sizeof(*transfer);
> +	if (offset + length > FW_SIZE)
> +		return -EINVAL;
> +
> +	switch (transfer->action) {
> +	case CXL_FW_TRANSFER_ACTION_FULL:
> +		if (offset != 0)
> +			return -EINVAL;
> +		fallthrough;
> +	case CXL_FW_TRANSFER_ACTION_END:
> +		if (transfer->slot == 0 || transfer->slot > FW_SLOTS)
> +			return -EINVAL;
> +		mdata->fw_size = offset + length;
> +		break;
> +	case CXL_FW_TRANSFER_ACTION_START:
> +	case CXL_FW_TRANSFER_ACTION_CONTINUE:
> +	case CXL_FW_TRANSFER_ACTION_ABORT:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	memcpy(fw + offset, &transfer->data[0], length);

Slight preference for transfer->data

> +	return 0;
> +}
> +

...

> +static int do_sha256(u8 *data, unsigned int length, u8 *hash)

Can't use the one in include/crypto/sha2.h?  Don't think anyone really
cares about extreme performance here.

> +{
> +	struct crypto_shash *alg;
> +	struct sdesc *sdesc;
> +	size_t size;
> +	int rc;
> +
> +	alg = crypto_alloc_shash("sha256", 0, 0);
> +	if (IS_ERR(alg))
> +		return PTR_ERR(alg);
> +
> +	size = sizeof(struct shash_desc) + crypto_shash_descsize(alg);
> +	sdesc = kzalloc(size, GFP_KERNEL);
> +	if (!sdesc) {
> +		rc = -ENOMEM;
> +		goto out_shash;
> +	}
> +
> +	sdesc->shash.tfm = alg;
> +	rc = crypto_shash_digest(&sdesc->shash, data, length, hash);
> +
> +	kfree(sdesc);
> +out_shash:
> +	crypto_free_shash(alg);
> +	return rc;
> +}
> +
> +#define CHECKSUM_SIZE 32
> +
> +static ssize_t fw_buf_checksum_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct cxl_mockmem_data *mdata = dev_get_drvdata(dev);
> +	unsigned char *hstr, *hptr;
> +	u8 hash[CHECKSUM_SIZE];
> +	ssize_t written = 0;
> +	int i, rc;
> +
> +	rc = do_sha256(mdata->fw, mdata->fw_size, &hash[0]);
> +	if (rc) {
> +		dev_err(dev, "error calculating checksum: %d\n", rc);
> +		goto out_free;
> +	}
> +
> +	hstr = kzalloc((CHECKSUM_SIZE * 2) + 1, GFP_KERNEL);
> +	if (!hstr)
> +		return -ENOMEM;
> +
> +	hptr = hstr;
> +	for (i = 0; i < CHECKSUM_SIZE; i++)
> +		hptr += sprintf(hptr, "%02x", hash[i]);
> +
> +	written = sysfs_emit(buf, "%s\n", hstr);
> +
> +out_free:
> +	kfree(hstr);
> +	return written;
> +}
> +
> +static DEVICE_ATTR_RO(fw_buf_checksum);
> +
>  static struct attribute *cxl_mock_mem_attrs[] = {
>  	&dev_attr_security_lock.attr,
>  	&dev_attr_event_trigger.attr,
> +	&dev_attr_fw_buf_checksum.attr,
>  	NULL
>  };
>  ATTRIBUTE_GROUPS(cxl_mock_mem);
> 

Re: [PATCH 4/4] tools/testing/cxl: add firmware update emulation to CXL memdevs

[Alison Schofield]

On Thu, May 11, 2023 at 05:18:16PM +0100, Jonathan Cameron wrote:
> On Fri, 21 Apr 2023 21:09:28 -0600
> Vishal Verma <vishal.l.verma@intel.com> wrote:
> 
> > Add emulation for the 'Get FW Info', 'Transfer FW', and 'Activate FW'
> > CXL mailbox commands to the cxl_test emulated memdevs to enable
> > end-to-end unit testing of a firmware update flow. For now, only
> > advertise an 'offline activation' capability as that is all the CXL
> > memdev driver currently implements.
> > 
> > Add some canned values for the serial number fields, and create a
> > platform device sysfs knob to calculate the sha256sum of the firmware
> > image that was received, so a unit test can compare it with the original
> > file that was uploaded.
> > 
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> Hi Vishal,
> 
> A few trivial comments inline,
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  tools/testing/cxl/test/mem.c | 191 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 191 insertions(+)
> > 
> > diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> > index 9263b04d35f7..bc99cc673550 100644
> > --- a/tools/testing/cxl/test/mem.c
> > +++ b/tools/testing/cxl/test/mem.c
> > @@ -7,11 +7,14 @@
> >  #include <linux/delay.h>
> >  #include <linux/sizes.h>
> >  #include <linux/bits.h>
> > +#include <crypto/hash.h>
> >  #include <cxlmem.h>
> >  
> >  #include "trace.h"
> >  
> >  #define LSA_SIZE SZ_128K
> > +#define FW_SIZE SZ_64M
> > +#define FW_SLOTS 3
> >  #define DEV_SIZE SZ_2G
> >  #define EFFECT(x) (1U << x)
> >  
> > @@ -40,6 +43,18 @@ static struct cxl_cel_entry mock_cel[] = {
> >  		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_HEALTH_INFO),
> >  		.effect = cpu_to_le16(0),
> >  	},
> > +	{
> > +		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_FW_INFO),
> > +		.effect = cpu_to_le16(0),
> > +	},
> > +	{
> > +		.opcode = cpu_to_le16(CXL_MBOX_OP_TRANSFER_FW),
> > +		.effect = cpu_to_le16(EFFECT(0) | EFFECT(6)),
> 
> Beginning to feel like some defines for each effect might be worth
> adding.
> 
> > +	},
> > +	{
> > +		.opcode = cpu_to_le16(CXL_MBOX_OP_ACTIVATE_FW),
> > +		.effect = cpu_to_le16(EFFECT(0) | EFFECT(1)),
> > +	},
> >  };
> 
> ...
> 
> > +static int mock_transfer_fw(struct cxl_dev_state *cxlds,
> > +			    struct cxl_mbox_cmd *cmd)
> > +{
> > +	struct cxl_mbox_transfer_fw *transfer = cmd->payload_in;
> > +	struct cxl_mockmem_data *mdata = dev_get_drvdata(cxlds->dev);
> > +	void *fw = mdata->fw;
> > +	size_t offset, length;
> > +
> > +	offset = le32_to_cpu(transfer->offset) * CXL_FW_TRANSFER_OFFSET_ALIGN;
> > +	length = cmd->size_in - sizeof(*transfer);
> > +	if (offset + length > FW_SIZE)
> > +		return -EINVAL;
> > +
> > +	switch (transfer->action) {
> > +	case CXL_FW_TRANSFER_ACTION_FULL:
> > +		if (offset != 0)
> > +			return -EINVAL;
> > +		fallthrough;
> > +	case CXL_FW_TRANSFER_ACTION_END:
> > +		if (transfer->slot == 0 || transfer->slot > FW_SLOTS)
> > +			return -EINVAL;
> > +		mdata->fw_size = offset + length;
> > +		break;
> > +	case CXL_FW_TRANSFER_ACTION_START:
> > +	case CXL_FW_TRANSFER_ACTION_CONTINUE:
> > +	case CXL_FW_TRANSFER_ACTION_ABORT:
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	memcpy(fw + offset, &transfer->data[0], length);
> 
> Slight preference for transfer->data
> 

What's the story behind that Jonathan? 
I imagined kernel developers leaned towards the explicit.

Alison


> > +	return 0;
> > +}
> > +
> 
> ...
> 
> > +static int do_sha256(u8 *data, unsigned int length, u8 *hash)
> 
> Can't use the one in include/crypto/sha2.h?  Don't think anyone really
> cares about extreme performance here.
> 
> > +{
> > +	struct crypto_shash *alg;
> > +	struct sdesc *sdesc;
> > +	size_t size;
> > +	int rc;
> > +
> > +	alg = crypto_alloc_shash("sha256", 0, 0);
> > +	if (IS_ERR(alg))
> > +		return PTR_ERR(alg);
> > +
> > +	size = sizeof(struct shash_desc) + crypto_shash_descsize(alg);
> > +	sdesc = kzalloc(size, GFP_KERNEL);
> > +	if (!sdesc) {
> > +		rc = -ENOMEM;
> > +		goto out_shash;
> > +	}
> > +
> > +	sdesc->shash.tfm = alg;
> > +	rc = crypto_shash_digest(&sdesc->shash, data, length, hash);
> > +
> > +	kfree(sdesc);
> > +out_shash:
> > +	crypto_free_shash(alg);
> > +	return rc;
> > +}
> > +
> > +#define CHECKSUM_SIZE 32
> > +
> > +static ssize_t fw_buf_checksum_show(struct device *dev,
> > +				    struct device_attribute *attr, char *buf)
> > +{
> > +	struct cxl_mockmem_data *mdata = dev_get_drvdata(dev);
> > +	unsigned char *hstr, *hptr;
> > +	u8 hash[CHECKSUM_SIZE];
> > +	ssize_t written = 0;
> > +	int i, rc;
> > +
> > +	rc = do_sha256(mdata->fw, mdata->fw_size, &hash[0]);
> > +	if (rc) {
> > +		dev_err(dev, "error calculating checksum: %d\n", rc);
> > +		goto out_free;
> > +	}
> > +
> > +	hstr = kzalloc((CHECKSUM_SIZE * 2) + 1, GFP_KERNEL);
> > +	if (!hstr)
> > +		return -ENOMEM;
> > +
> > +	hptr = hstr;
> > +	for (i = 0; i < CHECKSUM_SIZE; i++)
> > +		hptr += sprintf(hptr, "%02x", hash[i]);
> > +
> > +	written = sysfs_emit(buf, "%s\n", hstr);
> > +
> > +out_free:
> > +	kfree(hstr);
> > +	return written;
> > +}
> > +
> > +static DEVICE_ATTR_RO(fw_buf_checksum);
> > +
> >  static struct attribute *cxl_mock_mem_attrs[] = {
> >  	&dev_attr_security_lock.attr,
> >  	&dev_attr_event_trigger.attr,
> > +	&dev_attr_fw_buf_checksum.attr,
> >  	NULL
> >  };
> >  ATTRIBUTE_GROUPS(cxl_mock_mem);
> > 
> 

Re: [PATCH 4/4] tools/testing/cxl: add firmware update emulation to CXL memdevs

[Jonathan Cameron]

On Thu, 18 May 2023 20:01:18 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> On Thu, May 11, 2023 at 05:18:16PM +0100, Jonathan Cameron wrote:
> > On Fri, 21 Apr 2023 21:09:28 -0600
> > Vishal Verma <vishal.l.verma@intel.com> wrote:
> >   
> > > Add emulation for the 'Get FW Info', 'Transfer FW', and 'Activate FW'
> > > CXL mailbox commands to the cxl_test emulated memdevs to enable
> > > end-to-end unit testing of a firmware update flow. For now, only
> > > advertise an 'offline activation' capability as that is all the CXL
> > > memdev driver currently implements.
> > > 
> > > Add some canned values for the serial number fields, and create a
> > > platform device sysfs knob to calculate the sha256sum of the firmware
> > > image that was received, so a unit test can compare it with the original
> > > file that was uploaded.
> > > 
> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>  
> > Hi Vishal,
> > 
> > A few trivial comments inline,
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> > > ---
> > >  tools/testing/cxl/test/mem.c | 191 +++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 191 insertions(+)
> > > 
> > > diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> > > index 9263b04d35f7..bc99cc673550 100644
> > > --- a/tools/testing/cxl/test/mem.c
> > > +++ b/tools/testing/cxl/test/mem.c
> > > @@ -7,11 +7,14 @@
> > >  #include <linux/delay.h>
> > >  #include <linux/sizes.h>
> > >  #include <linux/bits.h>
> > > +#include <crypto/hash.h>
> > >  #include <cxlmem.h>
> > >  
> > >  #include "trace.h"
> > >  
> > >  #define LSA_SIZE SZ_128K
> > > +#define FW_SIZE SZ_64M
> > > +#define FW_SLOTS 3
> > >  #define DEV_SIZE SZ_2G
> > >  #define EFFECT(x) (1U << x)
> > >  
> > > @@ -40,6 +43,18 @@ static struct cxl_cel_entry mock_cel[] = {
> > >  		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_HEALTH_INFO),
> > >  		.effect = cpu_to_le16(0),
> > >  	},
> > > +	{
> > > +		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_FW_INFO),
> > > +		.effect = cpu_to_le16(0),
> > > +	},
> > > +	{
> > > +		.opcode = cpu_to_le16(CXL_MBOX_OP_TRANSFER_FW),
> > > +		.effect = cpu_to_le16(EFFECT(0) | EFFECT(6)),  
> > 
> > Beginning to feel like some defines for each effect might be worth
> > adding.
> >   
> > > +	},
> > > +	{
> > > +		.opcode = cpu_to_le16(CXL_MBOX_OP_ACTIVATE_FW),
> > > +		.effect = cpu_to_le16(EFFECT(0) | EFFECT(1)),
> > > +	},
> > >  };  
> > 
> > ...
> >   
> > > +static int mock_transfer_fw(struct cxl_dev_state *cxlds,
> > > +			    struct cxl_mbox_cmd *cmd)
> > > +{
> > > +	struct cxl_mbox_transfer_fw *transfer = cmd->payload_in;
> > > +	struct cxl_mockmem_data *mdata = dev_get_drvdata(cxlds->dev);
> > > +	void *fw = mdata->fw;
> > > +	size_t offset, length;
> > > +
> > > +	offset = le32_to_cpu(transfer->offset) * CXL_FW_TRANSFER_OFFSET_ALIGN;
> > > +	length = cmd->size_in - sizeof(*transfer);
> > > +	if (offset + length > FW_SIZE)
> > > +		return -EINVAL;
> > > +
> > > +	switch (transfer->action) {
> > > +	case CXL_FW_TRANSFER_ACTION_FULL:
> > > +		if (offset != 0)
> > > +			return -EINVAL;
> > > +		fallthrough;
> > > +	case CXL_FW_TRANSFER_ACTION_END:
> > > +		if (transfer->slot == 0 || transfer->slot > FW_SLOTS)
> > > +			return -EINVAL;
> > > +		mdata->fw_size = offset + length;
> > > +		break;
> > > +	case CXL_FW_TRANSFER_ACTION_START:
> > > +	case CXL_FW_TRANSFER_ACTION_CONTINUE:
> > > +	case CXL_FW_TRANSFER_ACTION_ABORT:
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	memcpy(fw + offset, &transfer->data[0], length);  
> > 
> > Slight preference for transfer->data
> >   
> 
> What's the story behind that Jonathan? 
> I imagined kernel developers leaned towards the explicit.

Meh.  In my head it's copying beyond end of that particular
element, and transfer->data conveys that 'potential' better.  Others
may have different opinions.

I've never seen a hard and fast rule on this one...

Jonathan

> 
> Alison
> 
> 
> > > +	return 0;
> > > +}
> > > +  
> > 
> > ...
> >   
> > > +static int do_sha256(u8 *data, unsigned int length, u8 *hash)  
> > 
> > Can't use the one in include/crypto/sha2.h?  Don't think anyone really
> > cares about extreme performance here.
> >   
> > > +{
> > > +	struct crypto_shash *alg;
> > > +	struct sdesc *sdesc;
> > > +	size_t size;
> > > +	int rc;
> > > +
> > > +	alg = crypto_alloc_shash("sha256", 0, 0);
> > > +	if (IS_ERR(alg))
> > > +		return PTR_ERR(alg);
> > > +
> > > +	size = sizeof(struct shash_desc) + crypto_shash_descsize(alg);
> > > +	sdesc = kzalloc(size, GFP_KERNEL);
> > > +	if (!sdesc) {
> > > +		rc = -ENOMEM;
> > > +		goto out_shash;
> > > +	}
> > > +
> > > +	sdesc->shash.tfm = alg;
> > > +	rc = crypto_shash_digest(&sdesc->shash, data, length, hash);
> > > +
> > > +	kfree(sdesc);
> > > +out_shash:
> > > +	crypto_free_shash(alg);
> > > +	return rc;
> > > +}
> > > +
> > > +#define CHECKSUM_SIZE 32
> > > +
> > > +static ssize_t fw_buf_checksum_show(struct device *dev,
> > > +				    struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct cxl_mockmem_data *mdata = dev_get_drvdata(dev);
> > > +	unsigned char *hstr, *hptr;
> > > +	u8 hash[CHECKSUM_SIZE];
> > > +	ssize_t written = 0;
> > > +	int i, rc;
> > > +
> > > +	rc = do_sha256(mdata->fw, mdata->fw_size, &hash[0]);
> > > +	if (rc) {
> > > +		dev_err(dev, "error calculating checksum: %d\n", rc);
> > > +		goto out_free;
> > > +	}
> > > +
> > > +	hstr = kzalloc((CHECKSUM_SIZE * 2) + 1, GFP_KERNEL);
> > > +	if (!hstr)
> > > +		return -ENOMEM;
> > > +
> > > +	hptr = hstr;
> > > +	for (i = 0; i < CHECKSUM_SIZE; i++)
> > > +		hptr += sprintf(hptr, "%02x", hash[i]);
> > > +
> > > +	written = sysfs_emit(buf, "%s\n", hstr);
> > > +
> > > +out_free:
> > > +	kfree(hstr);
> > > +	return written;
> > > +}
> > > +
> > > +static DEVICE_ATTR_RO(fw_buf_checksum);
> > > +
> > >  static struct attribute *cxl_mock_mem_attrs[] = {
> > >  	&dev_attr_security_lock.attr,
> > >  	&dev_attr_event_trigger.attr,
> > > +	&dev_attr_fw_buf_checksum.attr,
> > >  	NULL
> > >  };
> > >  ATTRIBUTE_GROUPS(cxl_mock_mem);
> > >   
> >   

Re: [PATCH 4/4] tools/testing/cxl: add firmware update emulation to CXL memdevs

[Verma, Vishal L]

On Thu, 2023-05-11 at 17:18 +0100, Jonathan Cameron wrote:
> 
> ...

Addressed everything else for v2

> 
> > +static int do_sha256(u8 *data, unsigned int length, u8 *hash)
> 
> Can't use the one in include/crypto/sha2.h?  Don't think anyone really
> cares about extreme performance here.
> 
> 
Ah nice, I hadn't seen that before. I was slightly concerned by the
comment there saying this is really only for kexec, and the other
interfaces were more document/prevalent. But using the interfaces in
include/crypto/sha2.h made the code a whole lot easier to follow /
maintain, so I've gone forward with it. We can always switch back later
if need be.

RE: [PATCH 4/4] tools/testing/cxl: add firmware update emulation to CXL memdevs

[Dan Williams]

Vishal Verma wrote:
> Add emulation for the 'Get FW Info', 'Transfer FW', and 'Activate FW'
> CXL mailbox commands to the cxl_test emulated memdevs to enable
> end-to-end unit testing of a firmware update flow. For now, only
> advertise an 'offline activation' capability as that is all the CXL
> memdev driver currently implements.
> 
> Add some canned values for the serial number fields, and create a
> platform device sysfs knob to calculate the sha256sum of the firmware
> image that was received, so a unit test can compare it with the original
> file that was uploaded.
> 

Looks good.

Re: [PATCH 0/4] cxl: Add a firmware update mechanism and cxl_test emulation

[Davidlohr Bueso]

On Fri, 21 Apr 2023, Vishal Verma wrote:

>The poll interval for the Transfer FW command is arbitrarily set at 1
>second, and a poll count of 300, giving us a total wait time of five
>minutes before which each slice of the transfer times out. This seems
>like a good mix of responsiveness and a total wait - the spec doesn't
>have any guidance on any upper or lower bounds for this. This likely
>does not need to be user-configurable, so for now it is just hard-coded
>in the driver.

Nothing against this, just thinking that in general, but we should
probably limit the poll interval to CXL_MAILBOX_TIMEOUT_MS. I'm not
sure, however, what would be a proper value across all commands. Or
would having this limit be per-cmd make more sense instead?

Thanks,
Davidlohr

Re: [PATCH 0/4] cxl: Add a firmware update mechanism and cxl_test emulation

[Verma, Vishal L]

On Mon, 2023-04-24 at 10:39 -0700, Davidlohr Bueso wrote:
> On Fri, 21 Apr 2023, Vishal Verma wrote:
> 
> > The poll interval for the Transfer FW command is arbitrarily set at 1
> > second, and a poll count of 300, giving us a total wait time of five
> > minutes before which each slice of the transfer times out. This seems
> > like a good mix of responsiveness and a total wait - the spec doesn't
> > have any guidance on any upper or lower bounds for this. This likely
> > does not need to be user-configurable, so for now it is just hard-coded
> > in the driver.
> 
> Nothing against this, just thinking that in general, but we should
> probably limit the poll interval to CXL_MAILBOX_TIMEOUT_MS. I'm not
> sure, however, what would be a proper value across all commands. Or
> would having this limit be per-cmd make more sense instead?
> 
Sorry I missed this comment earlier - are these actually related? The
mailbox timeout just waits for the doorbell, which in the case of a
background command would just indicate successful submission. The poll
interval is just how frequently we want to check the status of the
background command. I assume this could be set to something longer than
the mailbox ready timeout, especially if we know it will be a long
running command.