Re: [PATCH v3] hw/block/nvme: add device self test command support

2021-04-12 Thread Gollu Appalanaidu

On Mon, Apr 12, 2021 at 01:57:49PM +0530, Gollu Appalanaidu wrote:

On Sat, Apr 10, 2021 at 12:35:20AM +0900, Keith Busch wrote:

On Wed, Mar 31, 2021 at 02:54:27PM +0530, Gollu Appalanaidu wrote:

This is to add support for Device Self Test Command (DST) and
DST Log Page. Refer NVM Express specification 1.4b section 5.8
("Device Self-test command")


Please don't write change logs that just say what you did. I can read
the code to see that. Explain why this is useful because this frankly
looks like another useless feature. We don't need to implement every
optional spec feature here. There should be a real value proposition.


Hi Keith,
It was useful to us to be able to test the feature against qemu - and
we wanted to contribute the code, but we understand that features should
be more "complete" for upstreaming.

New features for SPDK (and nvme-cli) are use-cases for optional features
like this, where one might not have physical device available and also users
who is going to develop their in house host test tool this would be useful,
since we are providing the functional behaviour as per the NVMe protocol.


Hi Keith,

It was useful to us to be able to test the feature against qemu - and
we wanted to contribute the code, but we understand that features should
be more "complete" for upstreaming.

New features for SPDK (and nvme-cli) are use-cases for optional features
like this, where one might not have physical device available and also users
who is going to develop their in house host test tool this would be useful,
since we are providing the functional behaviour as per the NVMe protocol.


Re: [PATCH v3] hw/block/nvme: add device self test command support

2021-04-09 Thread Keith Busch
On Wed, Mar 31, 2021 at 02:54:27PM +0530, Gollu Appalanaidu wrote:
> This is to add support for Device Self Test Command (DST) and
> DST Log Page. Refer NVM Express specification 1.4b section 5.8
> ("Device Self-test command")

Please don't write change logs that just say what you did. I can read
the code to see that. Explain why this is useful because this frankly
looks like another useless feature. We don't need to implement every
optional spec feature here. There should be a real value proposition.



[PATCH v3] hw/block/nvme: add device self test command support

2021-03-31 Thread Gollu Appalanaidu
This is to add support for Device Self Test Command (DST) and
DST Log Page. Refer NVM Express specification 1.4b section 5.8
("Device Self-test command")

Signed-off-by: Gollu Appalanaidu 
---
changes:
 -v3: removed unwanted patch file added

 -v2: addressed style fixes in hw/block/nvme.h

 hw/block/nvme.c   | 118 +-
 hw/block/nvme.h   |  13 +
 hw/block/trace-events |   1 +
 include/block/nvme.h  |  49 ++
 4 files changed, 180 insertions(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 6842b01ab5..3c2186b170 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -214,6 +214,7 @@ static const uint32_t nvme_cse_acs[256] = {
 [NVME_ADM_CMD_ASYNC_EV_REQ] = NVME_CMD_EFF_CSUPP,
 [NVME_ADM_CMD_NS_ATTACHMENT]= NVME_CMD_EFF_CSUPP | NVME_CMD_EFF_NIC,
 [NVME_ADM_CMD_FORMAT_NVM]   = NVME_CMD_EFF_CSUPP | NVME_CMD_EFF_LBCC,
+[NVME_ADM_CMD_DST]  = NVME_CMD_EFF_CSUPP,
 };
 
 static const uint32_t nvme_cse_iocs_none[256];
@@ -3980,6 +3981,34 @@ static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint8_t 
csi, uint32_t buf_len,
 return nvme_c2h(n, ((uint8_t *)) + off, trans_len, req);
 }
 
+static uint16_t nvme_dst_info(NvmeCtrl *n,  uint32_t buf_len, uint64_t off,
+  NvmeRequest *req)
+{
+NvmeDstLogPage dst_log = {};
+NvmeDst *dst;
+NvmeDstEntry *traverser;
+uint32_t trans_len;
+uint8_t entry_index = 0;
+dst = >dst;
+
+if (off >= sizeof(dst_log)) {
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+
+dst_log.current_dsto = dst->current_dsto;
+dst_log.current_dstc = dst->current_dstc;
+
+QTAILQ_FOREACH(traverser, >dst_list, entry) {
+memcpy(_log.dst_result[entry_index],
+>dst_entry, sizeof(NvmeSelfTestResult));
+entry_index++;
+}
+
+trans_len = MIN(sizeof(dst_log) - off, buf_len);
+
+return nvme_c2h(n, ((uint8_t *)_log) + off, trans_len, req);
+}
+
 static uint16_t nvme_get_log(NvmeCtrl *n, NvmeRequest *req)
 {
 NvmeCmd *cmd = >cmd;
@@ -4027,6 +4056,8 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeRequest 
*req)
 return nvme_changed_nslist(n, rae, len, off, req);
 case NVME_LOG_CMD_EFFECTS:
 return nvme_cmd_effects(n, csi, len, off, req);
+case NVME_LOG_DEV_SELF_TEST:
+return nvme_dst_info(n, len, off, req);
 default:
 trace_pci_nvme_err_invalid_log_page(nvme_cid(req), lid);
 return NVME_INVALID_FIELD | NVME_DNR;
@@ -5069,6 +5100,73 @@ static uint16_t nvme_format(NvmeCtrl *n, NvmeRequest 
*req)
 return req->status;
 }
 
+static void nvme_dst_create_entry(NvmeCtrl *n, uint32_t nsid,
+uint8_t stc)
+{
+NvmeDstEntry *cur_entry;
+time_t current_ms;
+
+cur_entry = QTAILQ_LAST(>dst.dst_list);
+QTAILQ_REMOVE(>dst.dst_list, cur_entry, entry);
+memset(cur_entry, 0x0, sizeof(NvmeDstEntry));
+
+cur_entry->dst_entry.dst_status = stc << 4;
+
+if ((n->temperature >= n->features.temp_thresh_hi) ||
+(n->temperature <= n->features.temp_thresh_low)) {
+cur_entry->dst_entry.dst_status |= NVME_DST_WITH_FAILED_SEG;
+cur_entry->dst_entry.segment_number = NVME_SMART_CHECK;
+}
+
+current_ms = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
+cur_entry->dst_entry.poh = cpu_to_le64current_ms -
+n->starttime_ms) / 1000) / 60) / 60);
+cur_entry->dst_entry.nsid = nsid;
+
+QTAILQ_INSERT_HEAD(>dst.dst_list, cur_entry, entry);
+}
+
+static uint16_t nvme_dst_processing(NvmeCtrl *n, uint32_t nsid,
+uint8_t stc)
+{
+/*
+ * n->dst.current_dsto will be always 0x0 or NO DST OPERATION,
+ * since no background device self test operation takes place.
+ */
+assert(n->dst.current_dsto == NVME_DST_NO_OPERATION);
+
+if (stc == NVME_ABORT_DSTO) {
+goto out;
+}
+if (stc == NVME_SHORT_DSTO || stc == NVME_EXTENDED_DSTO) {
+nvme_dst_create_entry(n, nsid, stc);
+}
+
+out:
+n->dst.current_dstc = NVME_DST_OPERATION_COMPLETED;
+return NVME_SUCCESS;
+}
+
+static uint16_t nvme_dst(NvmeCtrl *n, NvmeRequest *req)
+{
+uint32_t dw10 = le32_to_cpu(req->cmd.cdw10);
+uint32_t nsid = le32_to_cpu(req->cmd.nsid);
+uint8_t stc = dw10 & 0xf;
+
+trace_pci_nvme_dst(nvme_cid(req), nsid, stc);
+
+if (!nvme_nsid_valid(n, nsid) && nsid != 0) {
+return NVME_INVALID_NSID | NVME_DNR;
+}
+
+if (nsid != NVME_NSID_BROADCAST && nsid != 0 &&
+!nvme_ns(n, nsid)) {
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+
+return nvme_dst_processing(n, nsid, stc);
+}
+
 static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest *req)
 {
 trace_pci_nvme_admin_cmd(nvme_cid(req), nvme_sqid(req), req->cmd.opcode,
@@ -5109,6 +5207,8 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest 
*req)
 return nvme_ns_attachment(n, req);
 case