[PATCH 0/2] nvme-cli: update Firmware Activate command

[PATCH 0/2] nvme-cli: update Firmware Activate command

[Minwoo Im]

Hi all,

Name of Firmware Activate command was changed in NVMe 1.2 to Firmware Commit.
This patch will change the command name accordingly.
It will also make it support for new feature(Boot Partition) specified in
this command which was introduced in NVMe 1.3.

Please feel free to give any comments.

Thanks,

Minwoo Im (2):
  nvme-cli: update Firmware Activate command for new feature
  nvme-cli: add a status code of Firmware Commit command

 linux/nvme.h   |  1 +
 nvme-builtin.h |  2 +-
 nvme-ioctl.c   |  4 ++--
 nvme-ioctl.h   |  2 +-
 nvme-print.c   |  1 +
 nvme.c         | 35 +++++++++++++++++++++++++----------
 6 files changed, 31 insertions(+), 14 deletions(-)

-- 
2.7.4

[PATCH 1/2] nvme-cli: update Firmware Activate command for new feature

[Minwoo Im]

Firmware Activate command were known as "Firmware Activate" in NVMe 1.0 and 1.1.
It has been changed to Firmware Commit since 1.2.
Also NVMe 1.3 spec introduced boot partition feature to Firmware Commit
command.

Update Firmware Activate command name and add a few new feature of boot
partition.

Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 nvme-builtin.h |  2 +-
 nvme-ioctl.c   |  4 ++--
 nvme-ioctl.h   |  2 +-
 nvme.c         | 35 +++++++++++++++++++++++++----------
 4 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/nvme-builtin.h b/nvme-builtin.h
index b06eb34..e62701d 100644
--- a/nvme-builtin.h
+++ b/nvme-builtin.h
@@ -28,7 +28,7 @@ COMMAND_LIST(
 	ENTRY("set-feature", "Set a feature and show the resulting value", set_feature)
 	ENTRY("set-property", "Set a property and show the resulting value", set_property)
 	ENTRY("format", "Format namespace with new block format", format)
-	ENTRY("fw-activate", "Activate new firmware slot", fw_activate)
+	ENTRY("fw-commit", "Verify and commit firmware to a specific firmware slot", fw_commit)
 	ENTRY("fw-download", "Download new firmware", fw_download)
 	ENTRY("admin-passthru", "Submit an arbitrary admin command, return results", admin_passthru)
 	ENTRY("io-passthru", "Submit an arbitrary IO command, return results", io_passthru)
diff --git a/nvme-ioctl.c b/nvme-ioctl.c
index f735ecc..65d64a7 100644
--- a/nvme-ioctl.c
+++ b/nvme-ioctl.c
@@ -650,11 +650,11 @@ int nvme_fw_download(int fd, __u32 offset, __u32 data_len, void *data)
 	return nvme_submit_admin_passthru(fd, &cmd);
 }
 
-int nvme_fw_activate(int fd, __u8 slot, __u8 action)
+int nvme_fw_commit(int fd, __u8 slot, __u8 action, __u8 bpid)
 {
 	struct nvme_admin_cmd cmd = {
 		.opcode		= nvme_admin_activate_fw,
-		.cdw10		= (action << 3) | slot,
+		.cdw10		= (bpid << 31) | (action << 3) | slot,
 	};
 
 	return nvme_submit_admin_passthru(fd, &cmd);
diff --git a/nvme-ioctl.h b/nvme-ioctl.h
index 7cc80c8..6a3b52b 100644
--- a/nvme-ioctl.h
+++ b/nvme-ioctl.h
@@ -106,7 +106,7 @@ int nvme_ns_attach_ctrls(int fd, __u32 nsid, __u16 num_ctrls, __u16 *ctrlist);
 int nvme_ns_detach_ctrls(int fd, __u32 nsid, __u16 num_ctrls, __u16 *ctrlist);
 
 int nvme_fw_download(int fd, __u32 offset, __u32 data_len, void *data);
-int nvme_fw_activate(int fd, __u8 slot, __u8 action);
+int nvme_fw_commit(int fd, __u8 slot, __u8 action, __u8 bpid);
 
 int nvme_sec_send(int fd, __u32 nsid, __u8 nssf, __u16 spsp,
 		  __u8 secp, __u32 tl, __u32 data_len, void *data, __u32 *result);
diff --git a/nvme.c b/nvme.c
index 4b0b861..d9cb5db 100644
--- a/nvme.c
+++ b/nvme.c
@@ -1780,30 +1780,34 @@ static char *nvme_fw_status_reset_type(__u32 status)
 	}
 }
 
-static int fw_activate(int argc, char **argv, struct command *cmd, struct plugin *plugin)
+static int fw_commit(int argc, char **argv, struct command *cmd, struct plugin *plugin)
 {
 	const char *desc = "Verify downloaded firmware image and "\
 		"commit to specific firmware slot. Device is not automatically "\
 		"reset following firmware activation. A reset may be issued "\
 		"with an 'echo 1 > /sys/class/nvme/nvmeX/reset_controller'. "\
 		"Ensure nvmeX is the device you just activated before reset.";
-	const char *slot = "firmware slot to activate";
-	const char *action = "[0-2]: replacement action";
+	const char *slot = "[0-7]: firmware slot for commit action";
+	const char *action = "[0-7]: commit action";
+	const char *bpid = "[0,1]: boot partition identifier, if applicable (default: 0)";
 	int err, fd;
 
 	struct config {
 		__u8 slot;
 		__u8 action;
+		__u8 bpid;
 	};
 
 	struct config cfg = {
 		.slot   = 0,
 		.action = 0,
+		.bpid   = 0,
 	};
 
 	const struct argconfig_commandline_options command_line_options[] = {
 		{"slot",   's', "NUM", CFG_BYTE, &cfg.slot,   required_argument, slot},
 		{"action", 'a', "NUM", CFG_BYTE, &cfg.action, required_argument, action},
+		{"bpid",   'b', "NUM", CFG_BYTE, &cfg.bpid,   required_argument, bpid},
 		{NULL}
 	};
 
@@ -1815,30 +1819,41 @@ static int fw_activate(int argc, char **argv, struct command *cmd, struct plugin
 		fprintf(stderr, "invalid slot:%d\n", cfg.slot);
 		return EINVAL;
 	}
-	if (cfg.action > 3) {
+	if (cfg.action > 7 || cfg.action == 4 || cfg.action == 5) {
 		fprintf(stderr, "invalid action:%d\n", cfg.action);
 		return EINVAL;
 	}
+	if (cfg.bpid > 1) {
+		fprintf(stderr, "invalid boot partition id:%d\n", cfg.bpid);
+		return EINVAL;
+	}
 
-	err = nvme_fw_activate(fd, cfg.slot, cfg.action);
+	err = nvme_fw_commit(fd, cfg.slot, cfg.action, cfg.bpid);
 	if (err < 0)
-		perror("fw-activate");
+		perror("fw-commit");
 	else if (err != 0)
 		switch (err & 0x3ff) {
 		case NVME_SC_FW_NEEDS_CONV_RESET:
 		case NVME_SC_FW_NEEDS_SUBSYS_RESET:
 		case NVME_SC_FW_NEEDS_RESET:
-			printf("Success activating firmware action:%d slot:%d, but firmware requires %s reset\n",
-			       cfg.action, cfg.slot, nvme_fw_status_reset_type(err));
+			printf("Success activating firmware action:%d slot:%d",
+			       cfg.action, cfg.slot);
+			if (cfg.action == 6 || cfg.action == 7)
+				printf(" bpid:%d", cfg.bpid);
+			printf(", but firmware requires %s reset\n", nvme_fw_status_reset_type(err));
 			break;
 		default:
 			fprintf(stderr, "NVME Admin command error:%s(%x)\n",
 						nvme_status_to_string(err), err);
 			break;
 		}
-	else
-		printf("Success activating firmware action:%d slot:%d\n",
+	else {
+		printf("Success committing firmware action:%d slot:%d",
 		       cfg.action, cfg.slot);
+		if (cfg.action == 6 || cfg.action == 7)
+			printf(" bpid:%d", cfg.bpid);
+		printf("\n");
+	}
 	return err;
 }
 
-- 
2.7.4

[PATCH 1/2] nvme-cli: update Firmware Activate command for new feature

[Keith Busch]

On Sat, Dec 16, 2017@03:02:50AM +0900, Minwoo Im wrote:
> Firmware Activate command were known as "Firmware Activate" in NVMe 1.0 and 1.1.
> It has been changed to Firmware Commit since 1.2.
> Also NVMe 1.3 spec introduced boot partition feature to Firmware Commit
> command.
> 
> Update Firmware Activate command name and add a few new feature of boot
> partition.

Hm, I can't really accept this. The tool existed when the command was
still called firmware activate in the spec. There are scripts in people's
environments that automate firmware updates, and we can't just break them
by changing a command name.

What I would accept is an alias command so we can deprecate/hide the
old command name. I've wanted to add infrastructure for creating aliased
commands, but I've not had time to get around to it.

[PATCH 1/2] nvme-cli: update Firmware Activate command for new feature

[Minwoo Im]

Hi Keith,

On Sat, Dec 16, 2017@3:23 AM, Keith Busch <keith.busch@intel.com> wrote:
> On Sat, Dec 16, 2017@03:02:50AM +0900, Minwoo Im wrote:
>> Firmware Activate command were known as "Firmware Activate" in NVMe 1.0 and 1.1.
>> It has been changed to Firmware Commit since 1.2.
>> Also NVMe 1.3 spec introduced boot partition feature to Firmware Commit
>> command.
>>
>> Update Firmware Activate command name and add a few new feature of boot
>> partition.
>
> Hm, I can't really accept this. The tool existed when the command was
> still called firmware activate in the spec. There are scripts in people's
> environments that automate firmware updates, and we can't just break them
> by changing a command name.
>
> What I would accept is an alias command so we can deprecate/hide the
> old command name. I've wanted to add infrastructure for creating aliased
> commands, but I've not had time to get around to it.

I totally agree with you. Sorry for sending a patch without any
considering users
who are using 1.0 and 1.1 NVMe devices. It would make some confusions.

If it's okay, I'll try to send a patch for an alias command infrastructure with
a real use case for Firmware Activate command.

Thanks,

[PATCH 2/2] nvme-cli: add a status code of Firmware Commit command

[Minwoo Im]

NVMe 1.3 spec introduced Boot Partition feature with Boot Partition
Write Prohibited status code for Firmware Commit command.

Add this status code and print statement when parsing error code.

Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 linux/nvme.h | 1 +
 nvme-print.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/linux/nvme.h b/linux/nvme.h
index e21610f..a15e6b8 100644
--- a/linux/nvme.h
+++ b/linux/nvme.h
@@ -1188,6 +1188,7 @@ enum {
 	NVME_SC_NS_NOT_ATTACHED		= 0x11a,
 	NVME_SC_THIN_PROV_NOT_SUPP	= 0x11b,
 	NVME_SC_CTRL_LIST_INVALID	= 0x11c,
+	NVME_SC_BP_WRITE_PROHIBITED	= 0x11e,
 
 	/*
 	 * I/O Command Set Specific - NVM commands:
diff --git a/nvme-print.c b/nvme-print.c
index 87f0766..ef00c41 100644
--- a/nvme-print.c
+++ b/nvme-print.c
@@ -1206,6 +1206,7 @@ char *nvme_status_to_string(__u32 status)
 	case NVME_SC_NS_NOT_ATTACHED:		return "NS_NOT_ATTACHED";
 	case NVME_SC_THIN_PROV_NOT_SUPP:	return "THIN_PROVISIONING_NOT_SUPPORTED";
 	case NVME_SC_CTRL_LIST_INVALID:		return "CONTROLLER_LIST_INVALID";
+	case NVME_SC_BP_WRITE_PROHIBITED:	return "BOOT PARTITION WRITE PROHIBITED";
 	case NVME_SC_BAD_ATTRIBUTES:		return "BAD_ATTRIBUTES";
 	case NVME_SC_WRITE_FAULT:		return "WRITE_FAULT";
 	case NVME_SC_READ_ERROR:		return "READ_ERROR";
-- 
2.7.4