[PATCHv3 00/11] Rotational storage support

[PATCHv3 00/11] Rotational storage support

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The first part gets nvme-target up to 2.1 specification requirements.
There were a few identifiactions, logs and a property that nvm e2.1
requires.

The patch from Matias attempts to query the command set independent
identify, but this version has changed to do that only if the
controller's reported nvme version is 2.0 or greater.

Keith Busch (9):
  nvmet: implement id ns for nvm command set
  nvmet: implement active command set ns list
  nvmet: implement supported log pages
  nvmet: implement supported features log
  nvmet: implement crto property
  nvmet: declare 2.1 version compliance
  nvmet: implement endurance grous
  nvmet: implement rotational media information log
  nvmet: support for csi identify ns

Matias Bjørling (2):
  nvme: make independent ns identify default
  nvme: add rotational support

 drivers/nvme/host/core.c          |  14 +-
 drivers/nvme/target/admin-cmd.c   | 240 +++++++++++++++++++++++++++++-
 drivers/nvme/target/fabrics-cmd.c |   3 +
 drivers/nvme/target/nvmet.h       |   2 +-
 include/linux/nvme.h              |  48 +++++-
 5 files changed, 295 insertions(+), 12 deletions(-)

-- 
2.43.5

[PATCHv3 01/11] nvmet: implement id ns for nvm command set

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Nothing to report here, but it's a mandatory identification for nvme
2.1.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/admin-cmd.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 081f0473cd9ea..8d06dba42bcb3 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -685,6 +685,20 @@ static void nvmet_execute_identify_ctrl_nvm(struct nvmet_req *req)
 		   nvmet_zero_sgl(req, 0, sizeof(struct nvme_id_ctrl_nvm)));
 }
 
+static void nvme_execute_identify_ns_nvm(struct nvmet_req *req)
+{
+	u16 status;
+
+	status = nvmet_req_find_ns(req);
+	if (status)
+		goto out;
+
+	status = nvmet_copy_to_sgl(req, 0, ZERO_PAGE(0),
+				   NVME_IDENTIFY_DATA_SIZE);
+out:
+	nvmet_req_complete(req, status);
+}
+
 static void nvmet_execute_identify(struct nvmet_req *req)
 {
 	if (!nvmet_check_transfer_len(req, NVME_IDENTIFY_DATA_SIZE))
@@ -706,8 +720,8 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 	case NVME_ID_CNS_CS_NS:
 		switch (req->cmd->identify.csi) {
 		case NVME_CSI_NVM:
-			/* Not supported */
-			break;
+			nvme_execute_identify_ns_nvm(req);
+			return;
 		case NVME_CSI_ZNS:
 			if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
 				nvmet_execute_identify_ns_zns(req);
-- 
2.43.5

Re: [PATCHv3 01/11] nvmet: implement id ns for nvm command set

[Chaitanya Kulkarni]

On 11/5/24 09:48, Keith Busch wrote:
> From: Keith Busch<kbusch@kernel.org>
> 
> Nothing to report here, but it's a mandatory identification for nvme
> 2.1.
> 
> Signed-off-by: Keith Busch<kbusch@kernel.org>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCHv3 01/11] nvmet: implement id ns for nvm command set

[Christoph Hellwig]

On Tue, Nov 05, 2024 at 09:48:54AM -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Nothing to report here, but it's a mandatory identification for nvme
> 2.1.

Yeah, as of 2.1 the only field is LBSTM which we don't support.

> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/nvme/target/admin-cmd.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 081f0473cd9ea..8d06dba42bcb3 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -685,6 +685,20 @@ static void nvmet_execute_identify_ctrl_nvm(struct nvmet_req *req)
>  		   nvmet_zero_sgl(req, 0, sizeof(struct nvme_id_ctrl_nvm)));
>  }
>  
> +static void nvme_execute_identify_ns_nvm(struct nvmet_req *req)
> +{
> +	u16 status;
> +
> +	status = nvmet_req_find_ns(req);
> +	if (status)
> +		goto out;
> +
> +	status = nvmet_copy_to_sgl(req, 0, ZERO_PAGE(0),
> +				   NVME_IDENTIFY_DATA_SIZE);

Does this also need a check for the magic all-Fs nsid, which I think
is not supported for the I/O command set specific identify namespace?
(see the code іn the zns version)

Re: [PATCHv3 01/11] nvmet: implement id ns for nvm command set

[Keith Busch]

On Wed, Nov 06, 2024 at 06:38:00AM +0100, Christoph Hellwig wrote:
> On Tue, Nov 05, 2024 at 09:48:54AM -0800, Keith Busch wrote:
> > +static void nvme_execute_identify_ns_nvm(struct nvmet_req *req)
> > +{
> > +	u16 status;
> > +
> > +	status = nvmet_req_find_ns(req);
> > +	if (status)
> > +		goto out;
> > +
> > +	status = nvmet_copy_to_sgl(req, 0, ZERO_PAGE(0),
> > +				   NVME_IDENTIFY_DATA_SIZE);
> 
> Does this also need a check for the magic all-Fs nsid, which I think
> is not supported for the I/O command set specific identify namespace?
> (see the code іn the zns version)

If the command provides the broadcast NSID, nvmet_req_find_ns() will
fail, and target will return an error.

Since you mention it, it seems I've neglected the nvmet_req_find_ns()
BUG for the failure case used in an rcu context.

[PATCHv3 02/11] nvmet: implement active command set ns list

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This is required for nvme 2.1 for targets that support multiple command
sets. We support NVM and ZNS, so are required to support this
identification.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/admin-cmd.c | 9 +++++++--
 include/linux/nvme.h            | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 8d06dba42bcb3..a13242e791c0f 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -576,7 +576,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
-static void nvmet_execute_identify_nslist(struct nvmet_req *req)
+static void nvmet_execute_identify_nslist(struct nvmet_req *req, bool match_css)
 {
 	static const int buf_size = NVME_IDENTIFY_DATA_SIZE;
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
@@ -606,6 +606,8 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req)
 	xa_for_each(&ctrl->subsys->namespaces, idx, ns) {
 		if (ns->nsid <= min_nsid)
 			continue;
+		if (match_css && req->ns->csi != req->cmd->identify.csi)
+			continue;
 		list[i++] = cpu_to_le32(ns->nsid);
 		if (i == buf_size / sizeof(__le32))
 			break;
@@ -712,7 +714,7 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 		nvmet_execute_identify_ctrl(req);
 		return;
 	case NVME_ID_CNS_NS_ACTIVE_LIST:
-		nvmet_execute_identify_nslist(req);
+		nvmet_execute_identify_nslist(req, false);
 		return;
 	case NVME_ID_CNS_NS_DESC_LIST:
 		nvmet_execute_identify_desclist(req);
@@ -743,6 +745,9 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 			break;
 		}
 		break;
+	case NVME_ID_CNS_NS_ACTIVE_LIST_CS:
+		nvmet_execute_identify_nslist(req, true);
+		return;
 	}
 
 	pr_debug("unhandled identify cns %d on qid %d\n",
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index b58d9405d65e0..0f263c7e63192 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -522,6 +522,7 @@ enum {
 	NVME_ID_CNS_NS_DESC_LIST	= 0x03,
 	NVME_ID_CNS_CS_NS		= 0x05,
 	NVME_ID_CNS_CS_CTRL		= 0x06,
+	NVME_ID_CNS_NS_ACTIVE_LIST_CS	= 0x07,
 	NVME_ID_CNS_NS_CS_INDEP		= 0x08,
 	NVME_ID_CNS_NS_PRESENT_LIST	= 0x10,
 	NVME_ID_CNS_NS_PRESENT		= 0x11,
-- 
2.43.5

Re: [PATCHv3 02/11] nvmet: implement active command set ns list

[Chaitanya Kulkarni]

On 11/5/24 09:48, Keith Busch wrote:
> From: Keith Busch<kbusch@kernel.org>
> 
> This is required for nvme 2.1 for targets that support multiple command
> sets. We support NVM and ZNS, so are required to support this
> identification.
> 
> Signed-off-by: Keith Busch<kbusch@kernel.org>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCHv3 02/11] nvmet: implement active command set ns list

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHv3 03/11] nvmet: implement supported log pages

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This log is required for nvme 2.1.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/admin-cmd.c | 29 +++++++++++++++++++++++++++++
 include/linux/nvme.h            |  1 +
 2 files changed, 30 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index a13242e791c0f..b8229d6c9998d 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -71,6 +71,33 @@ static void nvmet_execute_get_log_page_error(struct nvmet_req *req)
 	nvmet_req_complete(req, 0);
 }
 
+static void nvmet_execute_get_supported_log_pages(struct nvmet_req *req)
+{
+	__le32 *logs;
+	u16 status;
+
+	logs = kzalloc(1024, GFP_KERNEL);
+	if (!logs) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+
+	logs[NVME_LOG_SUPPORTED] = cpu_to_le32(1);
+	logs[NVME_LOG_ERROR] = cpu_to_le32(1);
+	logs[NVME_LOG_SMART] = cpu_to_le32(1);
+	logs[NVME_LOG_FW_SLOT] = cpu_to_le32(1);
+	logs[NVME_LOG_CHANGED_NS] = cpu_to_le32(1);
+	logs[NVME_LOG_CMD_EFFECTS] = cpu_to_le32(1);
+	logs[NVME_LOG_ENDURANCE_GROUP] = cpu_to_le32(1);
+	logs[NVME_LOG_ANA] = cpu_to_le32(1);
+	logs[NVME_LOG_RMI] = cpu_to_le32(1);
+
+	status = nvmet_copy_to_sgl(req, 0, logs, 1024);
+	kfree(logs);
+out:
+	nvmet_req_complete(req, 0);
+}
+
 static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
 		struct nvme_smart_log *slog)
 {
@@ -323,6 +350,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 		return;
 
 	switch (req->cmd->get_log_page.lid) {
+	case NVME_LOG_SUPPORTED:
+		return nvmet_execute_get_supported_log_pages(req);
 	case NVME_LOG_ERROR:
 		return nvmet_execute_get_log_page_error(req);
 	case NVME_LOG_SMART:
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 0f263c7e63192..a94e2280d350e 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1245,6 +1245,7 @@ enum {
 	NVME_FEAT_WRITE_PROTECT	= 0x84,
 	NVME_FEAT_VENDOR_START	= 0xC0,
 	NVME_FEAT_VENDOR_END	= 0xFF,
+	NVME_LOG_SUPPORTED	= 0x00,
 	NVME_LOG_ERROR		= 0x01,
 	NVME_LOG_SMART		= 0x02,
 	NVME_LOG_FW_SLOT	= 0x03,
-- 
2.43.5

Re: [PATCHv3 03/11] nvmet: implement supported log pages

[Kanchan Joshi]

On 11/5/2024 11:18 PM, Keith Busch wrote:
> +	status = nvmet_copy_to_sgl(req, 0, logs, 1024);
> +	kfree(logs);
> +out:
> +	nvmet_req_complete(req, 0);

Shouldn't this be: nvmet_req_complete(req, status)?

Re: [PATCHv3 03/11] nvmet: implement supported log pages

[Chaitanya Kulkarni]

On 11/5/24 09:48, Keith Busch wrote:
> From: Keith Busch<kbusch@kernel.org>
> 
> This log is required for nvme 2.1.
> 
> Signed-off-by: Keith Busch<kbusch@kernel.org>
> ---
>   drivers/nvme/target/admin-cmd.c | 29 +++++++++++++++++++++++++++++
>   include/linux/nvme.h            |  1 +
>   2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index a13242e791c0f..b8229d6c9998d 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -71,6 +71,33 @@ static void nvmet_execute_get_log_page_error(struct nvmet_req *req)
>   	nvmet_req_complete(req, 0);
>   }
>   
> +static void nvmet_execute_get_supported_log_pages(struct nvmet_req *req)
> +{
> +	__le32 *logs;
> +	u16 status;
> +
> +	logs = kzalloc(1024, GFP_KERNEL);
> +	if (!logs) {
> +		status = NVME_SC_INTERNAL;
> +		goto out;
> +	}
> +
> +	logs[NVME_LOG_SUPPORTED] = cpu_to_le32(1);
> +	logs[NVME_LOG_ERROR] = cpu_to_le32(1);
> +	logs[NVME_LOG_SMART] = cpu_to_le32(1);
> +	logs[NVME_LOG_FW_SLOT] = cpu_to_le32(1);
> +	logs[NVME_LOG_CHANGED_NS] = cpu_to_le32(1);
> +	logs[NVME_LOG_CMD_EFFECTS] = cpu_to_le32(1);
> +	logs[NVME_LOG_ENDURANCE_GROUP] = cpu_to_le32(1);
> +	logs[NVME_LOG_ANA] = cpu_to_le32(1);
> +	logs[NVME_LOG_RMI] = cpu_to_le32(1);

based on what we have in nvmet_get_cmd_effects_nvm() how about :-

         logs[NVME_LOG_SUPPORTED] =
         logs[NVME_LOG_ERROR] =
         logs[NVME_LOG_SMART] =
         logs[NVME_LOG_FW_SLOT] =
         logs[NVME_LOG_CHANGED_NS] =
         logs[NVME_LOG_CMD_EFFECTS] =
         logs[NVME_LOG_ENDURANCE_GROUP] =
         logs[NVME_LOG_ANA] =
         logs[NVME_LOG_RMI] =
                         	cpu_to_le32(1);

if you don't like please ignore this comment.

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCHv3 03/11] nvmet: implement supported log pages

[Chaitanya Kulkarni]

On 11/5/24 21:16, Chaitanya Kulkarni wrote:
> On 11/5/24 09:48, Keith Busch wrote:
>> From: Keith Busch<kbusch@kernel.org>
>>
>> This log is required for nvme 2.1.
>>
>> Signed-off-by: Keith Busch<kbusch@kernel.org>
>> ---
>>   drivers/nvme/target/admin-cmd.c | 29 +++++++++++++++++++++++++++++
>>   include/linux/nvme.h            |  1 +
>>   2 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/nvme/target/admin-cmd.c 
>> b/drivers/nvme/target/admin-cmd.c
>> index a13242e791c0f..b8229d6c9998d 100644
>> --- a/drivers/nvme/target/admin-cmd.c
>> +++ b/drivers/nvme/target/admin-cmd.c
>> @@ -71,6 +71,33 @@ static void nvmet_execute_get_log_page_error(struct 
>> nvmet_req *req)
>>       nvmet_req_complete(req, 0);
>>   }
>> +static void nvmet_execute_get_supported_log_pages(struct nvmet_req *req)
>> +{
>> +    __le32 *logs;
>> +    u16 status;
>> +
>> +    logs = kzalloc(1024, GFP_KERNEL);
>> +    if (!logs) {
>> +        status = NVME_SC_INTERNAL;
>> +        goto out;
>> +    }
>> +
>> +    logs[NVME_LOG_SUPPORTED] = cpu_to_le32(1);
>> +    logs[NVME_LOG_ERROR] = cpu_to_le32(1);
>> +    logs[NVME_LOG_SMART] = cpu_to_le32(1);
>> +    logs[NVME_LOG_FW_SLOT] = cpu_to_le32(1);
>> +    logs[NVME_LOG_CHANGED_NS] = cpu_to_le32(1);
>> +    logs[NVME_LOG_CMD_EFFECTS] = cpu_to_le32(1);
>> +    logs[NVME_LOG_ENDURANCE_GROUP] = cpu_to_le32(1);
>> +    logs[NVME_LOG_ANA] = cpu_to_le32(1);
>> +    logs[NVME_LOG_RMI] = cpu_to_le32(1);
> 
> based on what we have in nvmet_get_cmd_effects_nvm() how about :-
> 
>          logs[NVME_LOG_SUPPORTED] =
>          logs[NVME_LOG_ERROR] =
>          logs[NVME_LOG_SMART] =
>          logs[NVME_LOG_FW_SLOT] =
>          logs[NVME_LOG_CHANGED_NS] =
>          logs[NVME_LOG_CMD_EFFECTS] =
>          logs[NVME_LOG_ENDURANCE_GROUP] =
>          logs[NVME_LOG_ANA] =
>          logs[NVME_LOG_RMI] =
>                              cpu_to_le32(1);
> 
> if you don't like please ignore this comment.
> 
> Looks good.
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> 
> -ck
> 

with the status fix for nvmet_req_compelete() ofcourse ...

-ck

Re: [PATCHv3 03/11] nvmet: implement supported log pages

[Christoph Hellwig]

Looks good modulo passing on the status as noted by Kanchan:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHv3 04/11] nvmet: implement supported features log

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This log is required for nvme 2.1.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/admin-cmd.c | 25 +++++++++++++++++++++++++
 include/linux/nvme.h            |  1 +
 2 files changed, 26 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index b8229d6c9998d..c9b38d593fda5 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -344,6 +344,29 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
+static void nvmet_execute_get_log_page_features(struct nvmet_req *req)
+{
+	__le32 *features;
+	u16 status;
+
+	features = kzalloc(1024, GFP_KERNEL);
+	if (!features) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+
+	features[NVME_FEAT_NUM_QUEUES] = cpu_to_le32(1 << 21 | 1);
+	features[NVME_FEAT_KATO] = cpu_to_le32(1 << 21 | 1);
+	features[NVME_FEAT_ASYNC_EVENT] = cpu_to_le32(1 << 21 | 1);
+	features[NVME_FEAT_HOST_ID] = cpu_to_le32(1 << 21 | 1);
+	features[NVME_FEAT_WRITE_PROTECT] = cpu_to_le32(1 << 20 | 1);
+
+	status = nvmet_copy_to_sgl(req, 0, features, 1024);
+	kfree(features);
+out:
+	nvmet_req_complete(req, status);
+}
+
 static void nvmet_execute_get_log_page(struct nvmet_req *req)
 {
 	if (!nvmet_check_transfer_len(req, nvmet_get_log_page_len(req->cmd)))
@@ -369,6 +392,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 		return nvmet_execute_get_log_cmd_effects_ns(req);
 	case NVME_LOG_ANA:
 		return nvmet_execute_get_log_page_ana(req);
+	case NVME_LOG_FEATURES:
+		return nvmet_execute_get_log_page_features(req);
 	}
 	pr_debug("unhandled lid %d on qid %d\n",
 	       req->cmd->get_log_page.lid, req->sq->qid);
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index a94e2280d350e..b1238691bd491 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1256,6 +1256,7 @@ enum {
 	NVME_LOG_TELEMETRY_CTRL = 0x08,
 	NVME_LOG_ENDURANCE_GROUP = 0x09,
 	NVME_LOG_ANA		= 0x0c,
+	NVME_LOG_FEATURES	= 0x12,
 	NVME_LOG_DISC		= 0x70,
 	NVME_LOG_RESERVATION	= 0x80,
 	NVME_FWACT_REPL		= (0 << 3),
-- 
2.43.5

Re: [PATCHv3 04/11] nvmet: implement supported features log

[Chaitanya Kulkarni]

On 11/5/24 09:48, Keith Busch wrote:
> From: Keith Busch<kbusch@kernel.org>
> 
> This log is required for nvme 2.1.
> 
> Signed-off-by: Keith Busch<kbusch@kernel.org>


Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCHv3 04/11] nvmet: implement supported features log

[Christoph Hellwig]

On Tue, Nov 05, 2024 at 09:48:57AM -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> This log is required for nvme 2.1.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/nvme/target/admin-cmd.c | 25 +++++++++++++++++++++++++
>  include/linux/nvme.h            |  1 +
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index b8229d6c9998d..c9b38d593fda5 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -344,6 +344,29 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
>  	nvmet_req_complete(req, status);
>  }
>  
> +static void nvmet_execute_get_log_page_features(struct nvmet_req *req)
> +{
> +	__le32 *features;
> +	u16 status;
> +
> +	features = kzalloc(1024, GFP_KERNEL);

The magic number here is a bit weird.  Maybe add a

#define NVME_FEAT_MAX		256

and then make this ΝVME_FEAT_MAX * sizeof(*features)?

Same below.

> +	if (!features) {
> +		status = NVME_SC_INTERNAL;
> +		goto out;
> +	}
> +
> +	features[NVME_FEAT_NUM_QUEUES] = cpu_to_le32(1 << 21 | 1);
> +	features[NVME_FEAT_KATO] = cpu_to_le32(1 << 21 | 1);
> +	features[NVME_FEAT_ASYNC_EVENT] = cpu_to_le32(1 << 21 | 1);
> +	features[NVME_FEAT_HOST_ID] = cpu_to_le32(1 << 21 | 1);
> +	features[NVME_FEAT_WRITE_PROTECT] = cpu_to_le32(1 << 20 | 1);

And having names for the bits / scopes would be kinda nice as well.
(maybe same for the logs in the last patch)

Otherwise this looks good to me.

[PATCHv3 05/11] nvmet: implement crto property

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This property is required for nvme 2.1. The target only supports ready
with media, so this is just the same value as CAP.TO.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/fabrics-cmd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index c4b2eddd5666a..bb80db6a7950b 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -64,6 +64,9 @@ static void nvmet_execute_prop_get(struct nvmet_req *req)
 		case NVME_REG_CSTS:
 			val = ctrl->csts;
 			break;
+		case NVME_REG_CRTO:
+			val = NVME_CAP_TIMEOUT(ctrl->csts);
+			break;
 		default:
 			status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
 			break;
-- 
2.43.5

Re: [PATCHv3 05/11] nvmet: implement crto property

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHv3 06/11] nvmet: declare 2.1 version compliance

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The target driver implements all the mandatory logs, identifications,
features, and properties up to nvme sepcification 2.1.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/nvmet.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 190f55e6d7532..f902a630a5cf4 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -21,7 +21,7 @@
 #include <linux/radix-tree.h>
 #include <linux/t10-pi.h>
 
-#define NVMET_DEFAULT_VS		NVME_VS(1, 3, 0)
+#define NVMET_DEFAULT_VS		NVME_VS(2, 1, 0)
 
 #define NVMET_ASYNC_EVENTS		4
 #define NVMET_ERROR_LOG_SLOTS		128
-- 
2.43.5

Re: [PATCHv3 06/11] nvmet: declare 2.1 version compliance

[Chaitanya Kulkarni]

On 11/5/24 09:48, Keith Busch wrote:
> From: Keith Busch<kbusch@kernel.org>
> 
> The target driver implements all the mandatory logs, identifications,
> features, and properties up to nvme sepcification 2.1.
> 
> Signed-off-by: Keith Busch<kbusch@kernel.org>
> ---

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCHv3 06/11] nvmet: declare 2.1 version compliance

[Christoph Hellwig]

On Tue, Nov 05, 2024 at 09:48:59AM -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The target driver implements all the mandatory logs, identifications,
> features, and properties up to nvme sepcification 2.1.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Most unrelated question: do we need a min/max supported version for
nvmet_subsys_attr_version_store?

Re: [PATCHv3 06/11] nvmet: declare 2.1 version compliance

[Chaitanya Kulkarni]

On 11/5/24 21:45, Christoph Hellwig wrote:
> On Tue, Nov 05, 2024 at 09:48:59AM -0800, Keith Busch wrote:
>> From: Keith Busch <kbusch@kernel.org>
>>
>> The target driver implements all the mandatory logs, identifications,
>> features, and properties up to nvme sepcification 2.1.
> 
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Most unrelated question: do we need a min/max supported version for
> nvmet_subsys_attr_version_store?
> 
> 

By allowing user to set the min and max version it can definitely help
configure target version that is compliant with the host version, e.g.
if host only supports 1.3.0 then by setting max version to 1.3.0 in the
target subsystem will not execute any functionality that is added in
this series that belongs to only 2.1.0 version.

Is there any other reason ?

-ck

[PATCHv3 07/11] nvmet: implement endurance groups

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Most of the returned information is just stubbed data. The target must
support these in order to report rotational media. Since this driver
doesn't know any better, each namespace is its own endurance group with
the engid value matching the nsid.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c        |  1 +
 drivers/nvme/target/admin-cmd.c | 85 +++++++++++++++++++++++++++++++++
 include/linux/nvme.h            | 29 ++++++++++-
 3 files changed, 113 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 3de7555a7de74..02e969a34f9e8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -5006,6 +5006,7 @@ static inline void _nvme_check_size(void)
 	BUILD_BUG_ON(sizeof(struct nvme_id_ctrl_nvm) != NVME_IDENTIFY_DATA_SIZE);
 	BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
+	BUILD_BUG_ON(sizeof(struct nvme_endurance_group_log) != 512);
 	BUILD_BUG_ON(sizeof(struct nvme_dbbuf) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_directive_cmd) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_feat_host_behavior) != 512);
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index c9b38d593fda5..978a298fc6377 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -299,6 +299,45 @@ static u32 nvmet_format_ana_group(struct nvmet_req *req, u32 grpid,
 	return struct_size(desc, nsids, count);
 }
 
+static void nvmet_execute_get_log_page_endgrp(struct nvmet_req *req)
+{
+	u64 host_reads, host_writes, data_units_read, data_units_written;
+	struct nvme_endurance_group_log *log;
+	u16 status;
+
+	req->cmd->common.nsid = cpu_to_le32((u32)
+				le16_to_cpu(req->cmd->get_log_page.lsi));
+	status = nvmet_req_find_ns(req);
+	if (status)
+		goto out;
+
+	log = kzalloc(sizeof(*log), GFP_KERNEL);
+	if (!log) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+
+	if (!req->ns->bdev)
+		goto copy;
+
+	host_reads = part_stat_read(req->ns->bdev, ios[READ]);
+	data_units_read =
+		DIV_ROUND_UP(part_stat_read(req->ns->bdev, sectors[READ]), 1000);
+	host_writes = part_stat_read(req->ns->bdev, ios[WRITE]);
+	data_units_written =
+		DIV_ROUND_UP(part_stat_read(req->ns->bdev, sectors[WRITE]), 1000);
+
+	put_unaligned_le64(host_reads, &log->hrc[0]);
+	put_unaligned_le64(data_units_read, &log->dur[0]);
+	put_unaligned_le64(host_writes, &log->hwc[0]);
+	put_unaligned_le64(data_units_written, &log->duw[0]);
+copy:
+	status = nvmet_copy_to_sgl(req, 0, log, sizeof(*log));
+	kfree(log);
+out:
+	nvmet_req_complete(req, status);
+}
+
 static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
 {
 	struct nvme_ana_rsp_hdr hdr = { 0, };
@@ -390,6 +429,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 		return nvmet_execute_get_log_changed_ns(req);
 	case NVME_LOG_CMD_EFFECTS:
 		return nvmet_execute_get_log_cmd_effects_ns(req);
+	case NVME_LOG_ENDURANCE_GROUP:
+		return nvmet_execute_get_log_page_endgrp(req);
 	case NVME_LOG_ANA:
 		return nvmet_execute_get_log_page_ana(req);
 	case NVME_LOG_FEATURES:
@@ -521,6 +562,8 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 
 	id->msdbd = ctrl->ops->msdbd;
 
+	id->endgidmax = cpu_to_le16((u16)NVMET_MAX_NAMESPACES);
+
 	id->anacap = (1 << 0) | (1 << 1) | (1 << 2) | (1 << 3) | (1 << 4);
 	id->anatt = 10; /* random value */
 	id->anagrpmax = cpu_to_le32(NVMET_MAX_ANAGRPS);
@@ -605,6 +648,12 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 	id->nmic = NVME_NS_NMIC_SHARED;
 	id->anagrpid = cpu_to_le32(req->ns->anagrpid);
 
+	/*
+	 * Since we don't know any better, every namespace is its own endurance
+	 * group.
+	 */
+	id->endgid = cpu_to_le16((u16)req->ns->nsid);
+
 	memcpy(&id->nguid, &req->ns->nguid, sizeof(id->nguid));
 
 	id->lbaf[0].ds = req->ns->blksize_shift;
@@ -630,6 +679,39 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
+static void nvmet_execute_identify_endgrp_list(struct nvmet_req *req)
+{
+	u16 min_endgid = le16_to_cpu(req->cmd->identify.cnssid);
+	static const int buf_size = NVME_IDENTIFY_DATA_SIZE;
+	struct nvmet_ctrl *ctrl = req->sq->ctrl;
+	struct nvmet_ns *ns;
+	unsigned long idx;
+	__le16 *list;
+	u16 status;
+	int i = 1;
+
+	list = kzalloc(buf_size, GFP_KERNEL);
+	if (!list) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+
+	xa_for_each(&ctrl->subsys->namespaces, idx, ns) {
+		if (ns->nsid <= min_endgid)
+			continue;
+
+		list[i++] = cpu_to_le16((u16)ns->nsid);
+		if (i == buf_size / sizeof(__le16))
+			break;
+	}
+
+	list[0] = cpu_to_le16((u16)i - 1);
+	status = nvmet_copy_to_sgl(req, 0, list, buf_size);
+	kfree(list);
+out:
+	nvmet_req_complete(req, status);
+}
+
 static void nvmet_execute_identify_nslist(struct nvmet_req *req, bool match_css)
 {
 	static const int buf_size = NVME_IDENTIFY_DATA_SIZE;
@@ -802,6 +884,9 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 	case NVME_ID_CNS_NS_ACTIVE_LIST_CS:
 		nvmet_execute_identify_nslist(req, true);
 		return;
+	case NVME_ID_CNS_ENDGRP_LIST:
+		nvmet_execute_identify_endgrp_list(req);
+		return;
 	}
 
 	pr_debug("unhandled identify cns %d on qid %d\n",
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index b1238691bd491..49b32dc74c24a 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -327,7 +327,8 @@ struct nvme_id_ctrl {
 	__le32			sanicap;
 	__le32			hmminds;
 	__le16			hmmaxd;
-	__u8			rsvd338[4];
+	__le16			nvmsetidmax;
+	__le16			endgidmax;
 	__u8			anatt;
 	__u8			anacap;
 	__le32			anagrpmax;
@@ -531,6 +532,7 @@ enum {
 	NVME_ID_CNS_SCNDRY_CTRL_LIST	= 0x15,
 	NVME_ID_CNS_NS_GRANULARITY	= 0x16,
 	NVME_ID_CNS_UUID_LIST		= 0x17,
+	NVME_ID_CNS_ENDGRP_LIST		= 0x19,
 };
 
 enum {
@@ -618,6 +620,28 @@ enum {
 	NVME_NIDT_CSI		= 0x04,
 };
 
+struct nvme_endurance_group_log {
+	__u8	egcw;
+	__u8	egfeat;
+	__u8	rsvd2;
+	__u8	avsp;
+	__u8	avspt;
+	__u8	pused;
+	__le16	did;
+	__u8	rsvd8[24];
+	__u8	ee[16];
+	__u8	dur[16];
+	__u8	duw[16];
+	__u8	muw[16];
+	__u8	hrc[16];
+	__u8	hwc[16];
+	__u8	mdie[16];
+	__u8	neile[16];
+	__u8	tegcap[16];
+	__u8	uegcap[16];
+	__u8	rsvd192[320];
+};
+
 struct nvme_smart_log {
 	__u8			critical_warning;
 	__u8			temperature[2];
@@ -1284,7 +1308,8 @@ struct nvme_identify {
 	__u8			cns;
 	__u8			rsvd3;
 	__le16			ctrlid;
-	__u8			rsvd11[3];
+	__le16			cnssid;
+	__u8			rsvd11;
 	__u8			csi;
 	__u32			rsvd12[4];
 };
-- 
2.43.5

Re: [PATCHv3 07/11] nvmet: implement endurance groups

[Chaitanya Kulkarni]

On 11/5/24 09:49, Keith Busch wrote:
> From: Keith Busch<kbusch@kernel.org>
> 
> Most of the returned information is just stubbed data. The target must
> support these in order to report rotational media. Since this driver
> doesn't know any better, each namespace is its own endurance group with
> the engid value matching the nsid.
> 
> Signed-off-by: Keith Busch<kbusch@kernel.org>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCHv3 07/11] nvmet: implement endurance groups

[Christoph Hellwig]

On Tue, Nov 05, 2024 at 09:49:00AM -0800, Keith Busch wrote:
> +static void nvmet_execute_get_log_page_endgrp(struct nvmet_req *req)
> +{
> +	u64 host_reads, host_writes, data_units_read, data_units_written;
> +	struct nvme_endurance_group_log *log;
> +	u16 status;
> +
> +	req->cmd->common.nsid = cpu_to_le32((u32)
> +				le16_to_cpu(req->cmd->get_log_page.lsi));
> +	status = nvmet_req_find_ns(req);

I think this would be a good place to point out that we "emulate" one
endurance group per namespace, as the code only make sense with that
in mind.

As far as I can tell cpu_to_* does all the normal implicit integer
conversions, so the case here and the ones further down should not be
needed.

> +	host_reads = part_stat_read(req->ns->bdev, ios[READ]);
> +	data_units_read =
> +		DIV_ROUND_UP(part_stat_read(req->ns->bdev, sectors[READ]), 1000);
> +	host_writes = part_stat_read(req->ns->bdev, ios[WRITE]);
> +	data_units_written =
> +		DIV_ROUND_UP(part_stat_read(req->ns->bdev, sectors[WRITE]), 1000);

Overly long lines, but IMHO this would read nicer anyway if the
round is done later, e.g.:

	sectors_written = part_stat_read(req->ns->bdev, sectors[WRITE]);

	...

	put_unaligned_le64(DIV_ROUND_UP(sectors_written, 1000), &log->dur[0]);


> +	xa_for_each(&ctrl->subsys->namespaces, idx, ns) {
> +		if (ns->nsid <= min_endgid)
> +			continue;
> +
> +		list[i++] = cpu_to_le16((u16)ns->nsid);
> +		if (i == buf_size / sizeof(__le16))
> +			break;
> +	}

No really important, but this could use xas_for_each for avoid
doing a full lookup on every iteration.

[PATCHv3 08/11] nvmet: implement rotational media information log

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Most of the information is stubbed. Supporting these commands is a
requirement for supporting rotational media.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c        |  1 +
 drivers/nvme/target/admin-cmd.c | 42 +++++++++++++++++++++++++++++++++
 include/linux/nvme.h            | 15 +++++++++++-
 3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 02e969a34f9e8..e51021294a78b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -5007,6 +5007,7 @@ static inline void _nvme_check_size(void)
 	BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
 	BUILD_BUG_ON(sizeof(struct nvme_endurance_group_log) != 512);
+	BUILD_BUG_ON(sizeof(struct nvme_rotational_media_log) != 512);
 	BUILD_BUG_ON(sizeof(struct nvme_dbbuf) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_directive_cmd) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_feat_host_behavior) != 512);
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 978a298fc6377..dba8940a85e35 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -157,6 +157,46 @@ static u16 nvmet_get_smart_log_all(struct nvmet_req *req,
 	return NVME_SC_SUCCESS;
 }
 
+static void nvmet_execute_get_log_page_rmi(struct nvmet_req *req)
+{
+	struct nvme_rotational_media_log *log;
+	struct gendisk *disk;
+	u16 status;
+
+	req->cmd->common.nsid = cpu_to_le32((u32)
+				le16_to_cpu(req->cmd->get_log_page.lsi));
+	status = nvmet_req_find_ns(req);
+	if (status)
+		goto out;
+
+	if (!req->ns->bdev || bdev_nonrot(req->ns->bdev)) {
+		status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
+		goto out;
+	}
+
+	if (req->transfer_len != sizeof(*log)) {
+		status = NVME_SC_SGL_INVALID_DATA | NVME_STATUS_DNR;
+		goto out;
+	}
+
+	log = kzalloc(sizeof(*log), GFP_KERNEL);
+	if (!log)
+		goto out;
+
+	log->endgid = req->cmd->get_log_page.lsi;
+
+	disk = req->ns->bdev->bd_disk;
+	if (disk && disk->ia_ranges)
+		log->numa = cpu_to_le16(disk->ia_ranges->nr_ia_ranges);
+	else
+		log->numa = cpu_to_le16(1);
+
+	status = nvmet_copy_to_sgl(req, 0, log, sizeof(*log));
+	kfree(log);
+out:
+	nvmet_req_complete(req, status);
+}
+
 static void nvmet_execute_get_log_page_smart(struct nvmet_req *req)
 {
 	struct nvme_smart_log *log;
@@ -435,6 +475,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 		return nvmet_execute_get_log_page_ana(req);
 	case NVME_LOG_FEATURES:
 		return nvmet_execute_get_log_page_features(req);
+	case NVME_LOG_RMI:
+		return nvmet_execute_get_log_page_rmi(req);
 	}
 	pr_debug("unhandled lid %d on qid %d\n",
 	       req->cmd->get_log_page.lid, req->sq->qid);
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 49b32dc74c24a..9bd37ec01c072 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -642,6 +642,18 @@ struct nvme_endurance_group_log {
 	__u8	rsvd192[320];
 };
 
+struct nvme_rotational_media_log {
+	__le16	endgid;
+	__le16	numa;
+	__le16	nrs;
+	__u8	rsvd6[2];
+	__le32	spinc;
+	__le32	fspinc;
+	__le32	ldc;
+	__le32	fldc;
+	__u8	rsvd24[488];
+};
+
 struct nvme_smart_log {
 	__u8			critical_warning;
 	__u8			temperature[2];
@@ -1281,6 +1293,7 @@ enum {
 	NVME_LOG_ENDURANCE_GROUP = 0x09,
 	NVME_LOG_ANA		= 0x0c,
 	NVME_LOG_FEATURES	= 0x12,
+	NVME_LOG_RMI		= 0x16,
 	NVME_LOG_DISC		= 0x70,
 	NVME_LOG_RESERVATION	= 0x80,
 	NVME_FWACT_REPL		= (0 << 3),
@@ -1417,7 +1430,7 @@ struct nvme_get_log_page_command {
 	__u8			lsp; /* upper 4 bits reserved */
 	__le16			numdl;
 	__le16			numdu;
-	__u16			rsvd11;
+	__le16			lsi;
 	union {
 		struct {
 			__le32 lpol;
-- 
2.43.5

Re: [PATCHv3 08/11] nvmet: implement rotational media information log

[Christoph Hellwig]

Looks good modulo the spurious casts mentioned before:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHv3 09/11] nvmet: support for csi identify ns

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Implements reporting the I/O Command Set Independent Identify Namespace
command.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/admin-cmd.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index dba8940a85e35..9609bda7b5629 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -879,6 +879,35 @@ static void nvme_execute_identify_ns_nvm(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
+static void nvmet_execute_id_cs_indep(struct nvmet_req *req)
+{
+	struct nvme_id_ns_cs_indep *id;
+	u16 status;
+
+	status = nvmet_req_find_ns(req);
+	if (status)
+		goto out;
+
+	id = kzalloc(sizeof(*id), GFP_KERNEL);
+	if (!id) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+
+	id->nstat = NVME_NSTAT_NRDY;
+	id->anagrpid = req->ns->anagrpid;
+	id->nmic = NVME_NS_NMIC_SHARED;
+	if (req->ns->readonly)
+		id->nsattr |= NVME_NS_ATTR_RO;
+	if (req->ns->bdev && !bdev_nonrot(req->ns->bdev))
+		id->nsfeat |= NVME_NS_ROTATIONAL;
+
+	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
+	kfree(id);
+out:
+	nvmet_req_complete(req, status);
+}
+
 static void nvmet_execute_identify(struct nvmet_req *req)
 {
 	if (!nvmet_check_transfer_len(req, NVME_IDENTIFY_DATA_SIZE))
@@ -926,6 +955,9 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 	case NVME_ID_CNS_NS_ACTIVE_LIST_CS:
 		nvmet_execute_identify_nslist(req, true);
 		return;
+	case NVME_ID_CNS_NS_CS_INDEP:
+		nvmet_execute_id_cs_indep(req);
+		return;
 	case NVME_ID_CNS_ENDGRP_LIST:
 		nvmet_execute_identify_endgrp_list(req);
 		return;
-- 
2.43.5

Re: [PATCHv3 09/11] nvmet: support for csi identify ns

[Guixin Liu]

在 2024/11/6 01:49, Keith Busch 写道:
> From: Keith Busch <kbusch@kernel.org>
>
> Implements reporting the I/O Command Set Independent Identify Namespace
> command.
>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/target/admin-cmd.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
>
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index dba8940a85e35..9609bda7b5629 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -879,6 +879,35 @@ static void nvme_execute_identify_ns_nvm(struct nvmet_req *req)
>   	nvmet_req_complete(req, status);
>   }
>   
> +static void nvmet_execute_id_cs_indep(struct nvmet_req *req)
> +{
> +	struct nvme_id_ns_cs_indep *id;
> +	u16 status;
> +
> +	status = nvmet_req_find_ns(req);
> +	if (status)
> +		goto out;
> +
> +	id = kzalloc(sizeof(*id), GFP_KERNEL);
> +	if (!id) {
> +		status = NVME_SC_INTERNAL;
> +		goto out;
> +	}
> +
> +	id->nstat = NVME_NSTAT_NRDY;
> +	id->anagrpid = req->ns->anagrpid;
Should use cpu_to_le32(req->ns->anagrpid) here.
> +	id->nmic = NVME_NS_NMIC_SHARED;

Nit, id->nmic |= NVME_NS_NMIC_SHARED;

Best Regards,

Guixin Liu

> +	if (req->ns->readonly)
> +		id->nsattr |= NVME_NS_ATTR_RO;
> +	if (req->ns->bdev && !bdev_nonrot(req->ns->bdev))
> +		id->nsfeat |= NVME_NS_ROTATIONAL;
> +
> +	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
> +	kfree(id);
> +out:
> +	nvmet_req_complete(req, status);
> +}

[PATCHv3 10/11] nvme: make independent ns identify default

[Keith Busch]

From: Matias Bjørling <matias.bjorling@wdc.com>

The NVMe 2.0 specification adds an independent identify namespace data
structure that contains generic attributes that apply to all namespace
types. Some attributes carry over from the NVM command set identify
namespace data structure, and others are new.

Currently, the data structure only considered when CRIMS is enabled or
when the namespace type is key-value.

However, the independent namespace data structure is mandatory for
devices that implement features from the 2.0+ specification. Therefore,
we can check this data structure first. If unavailable, retrieve the
generic attributes from the NVM command set identify namespace data
structure.

Signed-off-by: Matias Bjørling <matias.bjorling@wdc.com>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e51021294a78b..4361fc7e8a225 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3988,7 +3988,7 @@ static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 {
 	struct nvme_ns_info info = { .nsid = nsid };
 	struct nvme_ns *ns;
-	int ret;
+	int ret = 1;
 
 	if (nvme_identify_ns_descs(ctrl, &info))
 		return;
@@ -4004,10 +4004,9 @@ static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	 * data structure to find all the generic information that is needed to
 	 * set up a namespace.  If not fall back to the legacy version.
 	 */
-	if ((ctrl->cap & NVME_CAP_CRMS_CRIMS) ||
-	    (info.ids.csi != NVME_CSI_NVM && info.ids.csi != NVME_CSI_ZNS))
+	if (ctrl->vs >= NVME_VS(2, 0, 0))
 		ret = nvme_ns_info_from_id_cs_indep(ctrl, &info);
-	else
+	if (ret > 0)
 		ret = nvme_ns_info_from_identify(ctrl, &info);
 
 	if (info.is_removed)
-- 
2.43.5

Re: [PATCHv3 10/11] nvme: make independent ns identify default

[Keith Busch]

On Tue, Nov 05, 2024 at 09:49:03AM -0800, Keith Busch wrote:
> @@ -4004,10 +4004,9 @@ static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
>  	 * data structure to find all the generic information that is needed to
>  	 * set up a namespace.  If not fall back to the legacy version.
>  	 */
> -	if ((ctrl->cap & NVME_CAP_CRMS_CRIMS) ||
> -	    (info.ids.csi != NVME_CSI_NVM && info.ids.csi != NVME_CSI_ZNS))
> +	if (ctrl->vs >= NVME_VS(2, 0, 0))
>  		ret = nvme_ns_info_from_id_cs_indep(ctrl, &info);

This should probably retain the previous conditions and just OR in the
2.0 check instead. I already made the change in my local branch, it
looks like this:

	if ((ctrl->cap & NVME_CAP_CRMS_CRIMS) ||
	    (info.ids.csi != NVME_CSI_NVM && info.ids.csi != NVME_CSI_ZNS) ||
	    ctrl->vs >= NVME_VS(2, 0, 0))

Re: [PATCHv3 10/11] nvme: make independent ns identify default

[Christoph Hellwig]

On Tue, Nov 05, 2024 at 09:49:03AM -0800, Keith Busch wrote:
> From: Matias Bjørling <matias.bjorling@wdc.com>
> 
> The NVMe 2.0 specification adds an independent identify namespace data
> structure that contains generic attributes that apply to all namespace
> types. Some attributes carry over from the NVM command set identify
> namespace data structure, and others are new.

The subject could probably use a little ammending, e.g.

"use command set independent identify namespace if available"

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHv3 11/11] nvme: add rotational support

[Keith Busch]

From: Matias Bjørling <matias.bjorling@wdc.com>

Rotational devices, such as hard-drives, can be detected using
the rotational bit in the namespace independent identify namespace
data structure. Make the bit visible to the block layer through the
rotational queue setting.

Note that rotational devices typically can be used to generate random
entropy, the device is therefore also added as a block device that adds
entropy.

Signed-off-by: Matias Bjørling <matias.bjorling@wdc.com>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 5 +++++
 include/linux/nvme.h     | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 4361fc7e8a225..a47149858be59 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -42,6 +42,7 @@ struct nvme_ns_info {
 	bool is_readonly;
 	bool is_ready;
 	bool is_removed;
+	bool is_rotational;
 };
 
 unsigned int admin_timeout = 60;
@@ -1615,6 +1616,7 @@ static int nvme_ns_info_from_id_cs_indep(struct nvme_ctrl *ctrl,
 		info->is_shared = id->nmic & NVME_NS_NMIC_SHARED;
 		info->is_readonly = id->nsattr & NVME_NS_ATTR_RO;
 		info->is_ready = id->nstat & NVME_NSTAT_NRDY;
+		info->is_rotational = id->nsfeat & NVME_NS_ROTATIONAL;
 	}
 	kfree(id);
 	return ret;
@@ -2162,6 +2164,9 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
 	else
 		lim.features &= ~(BLK_FEAT_WRITE_CACHE | BLK_FEAT_FUA);
 
+	if (info->is_rotational)
+		lim.features |= BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM;
+
 	/*
 	 * Register a metadata profile for PI, or the plain non-integrity NVMe
 	 * metadata masquerading as Type 0 if supported, otherwise reject block
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 9bd37ec01c072..5976285192ae4 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -563,6 +563,7 @@ enum {
 	NVME_NS_FLBAS_LBA_SHIFT	= 1,
 	NVME_NS_FLBAS_META_EXT	= 0x10,
 	NVME_NS_NMIC_SHARED	= 1 << 0,
+	NVME_NS_ROTATIONAL	= 1 << 4,
 	NVME_LBAF_RP_BEST	= 0,
 	NVME_LBAF_RP_BETTER	= 1,
 	NVME_LBAF_RP_GOOD	= 2,
-- 
2.43.5

Re: [PATCHv3 11/11] nvme: add rotational support

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv3 11/11] nvme: add rotational support

[Chaitanya Kulkarni]

On 11/5/24 09:49, Keith Busch wrote:
> From: Matias Bjørling<matias.bjorling@wdc.com>
> 
> Rotational devices, such as hard-drives, can be detected using
> the rotational bit in the namespace independent identify namespace
> data structure. Make the bit visible to the block layer through the
> rotational queue setting.
> 
> Note that rotational devices typically can be used to generate random
> entropy, the device is therefore also added as a block device that adds
> entropy.
> 
> Signed-off-by: Matias Bjørling<matias.bjorling@wdc.com>
> Reviewed-by: Martin K. Petersen<martin.petersen@oracle.com>
> Signed-off-by: Keith Busch<kbusch@kernel.org>


Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCHv3 11/11] nvme: add rotational support

[Guixin Liu]

在 2024/11/6 01:49, Keith Busch 写道:
> From: Matias Bjørling <matias.bjorling@wdc.com>
>
> Rotational devices, such as hard-drives, can be detected using
> the rotational bit in the namespace independent identify namespace
> data structure. Make the bit visible to the block layer through the
> rotational queue setting.
>
> Note that rotational devices typically can be used to generate random
> entropy, the device is therefore also added as a block device that adds
> entropy.
>
> Signed-off-by: Matias Bjørling <matias.bjorling@wdc.com>
> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/host/core.c | 5 +++++
>   include/linux/nvme.h     | 1 +
>   2 files changed, 6 insertions(+)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 4361fc7e8a225..a47149858be59 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -42,6 +42,7 @@ struct nvme_ns_info {
>   	bool is_readonly;
>   	bool is_ready;
>   	bool is_removed;
> +	bool is_rotational;
>   };
>   
>   unsigned int admin_timeout = 60;
> @@ -1615,6 +1616,7 @@ static int nvme_ns_info_from_id_cs_indep(struct nvme_ctrl *ctrl,
>   		info->is_shared = id->nmic & NVME_NS_NMIC_SHARED;
>   		info->is_readonly = id->nsattr & NVME_NS_ATTR_RO;
>   		info->is_ready = id->nstat & NVME_NSTAT_NRDY;
> +		info->is_rotational = id->nsfeat & NVME_NS_ROTATIONAL;
>   	}
>   	kfree(id);
>   	return ret;
> @@ -2162,6 +2164,9 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
>   	else
>   		lim.features &= ~(BLK_FEAT_WRITE_CACHE | BLK_FEAT_FUA);
>   
> +	if (info->is_rotational)
> +		lim.features |= BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM;
> +

May I pose a question, what does BLK_FEAT_ADD_RANDOM do?

I see the scsi call add_disk_randomness() in scsi_end_request() if 
request_queue has

BLK_FEAT_ADD_RANDOM flag, but I dont see any calls in the patchset.

Best Regards,

Guixin Liu

>   	/*
>   	 * Register a metadata profile for PI, or the plain non-integrity NVMe
>   	 * metadata masquerading as Type 0 if supported, otherwise reject block
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 9bd37ec01c072..5976285192ae4 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -563,6 +563,7 @@ enum {
>   	NVME_NS_FLBAS_LBA_SHIFT	= 1,
>   	NVME_NS_FLBAS_META_EXT	= 0x10,
>   	NVME_NS_NMIC_SHARED	= 1 << 0,
> +	NVME_NS_ROTATIONAL	= 1 << 4,
>   	NVME_LBAF_RP_BEST	= 0,
>   	NVME_LBAF_RP_BETTER	= 1,
>   	NVME_LBAF_RP_GOOD	= 2,

Re: [PATCHv3 11/11] nvme: add rotational support

[Christoph Hellwig]

On Wed, Nov 06, 2024 at 03:57:06PM +0800, Guixin Liu wrote:
> May I pose a question, what does BLK_FEAT_ADD_RANDOM do?

Nothing.

> I see the scsi call add_disk_randomness() in scsi_end_request() if 
> request_queue has

Yes.

>
> BLK_FEAT_ADD_RANDOM flag, but I dont see any calls in the patchset.

I actually pointed that out last round and thought it got fixed,
should have noticed it wasn't this time around.  The flag really
should be dropped.

Re: [PATCHv3 11/11] nvme: add rotational support

[Keith Busch]

On Wed, Nov 06, 2024 at 09:22:32AM +0100, Christoph Hellwig wrote:
> On Wed, Nov 06, 2024 at 03:57:06PM +0800, Guixin Liu wrote:
> >
> > BLK_FEAT_ADD_RANDOM flag, but I dont see any calls in the patchset.
> 
> I actually pointed that out last round and thought it got fixed,
> should have noticed it wasn't this time around.  The flag really
> should be dropped.

My bad, I applied the RFC patch here, but it was already fixed it in
subsequent version.

I'm fixing up the remaining stuff from the review comments. Will spin a
new version shortly.