On Tue, Nov 12, 2019 at 03:04:52PM +0000, Beata Michalska wrote:
> Hi Klaus,
>
>
> On Tue, 15 Oct 2019 at 11:45, Klaus Jensen <i...@irrelevant.dk> wrote:
> > + if (!nsid || (nsid != 0xffffffff && nsid > n->num_namespaces)) {
> > + trace_nvme_err_invalid_ns(nsid, n->num_namespaces);
> > + return NVME_INVALID_NSID | NVME_DNR;
> > + }
> > +
> The LAP '0' bit is cleared now - which means there is no support
> for per-namespace data. So in theory, if that was the aim, this condition
> should check for the values different than 0x0 and 0xFFFFFFFF and either
> abort the command or treat that as request for controller specific data.
>
This is fixed in v3 (that is, it just checks for values different from
0x0 and 0xffffffff).
> > + switch (lid) {
> > + case NVME_LOG_ERROR_INFO:
> > + return nvme_error_info(n, cmd, len, off, req);
> > + case NVME_LOG_SMART_INFO:
> > + return nvme_smart_info(n, cmd, len, off, req);
> > + case NVME_LOG_FW_SLOT_INFO:
> > + return nvme_fw_log_info(n, cmd, len, off, req);
> > + default:
> > + trace_nvme_err_invalid_log_page(req->cid, lid);
> > + return NVME_INVALID_LOG_ID | NVME_DNR;
>
> The spec mentions the Invalid Field in Command case processing
> command with an unsupported log id.
>
Thanks. Fixed!
> > + }
> > +}
> > +
> > static void nvme_free_cq(NvmeCQueue *cq, NvmeCtrl *n)
> > {
> > n->cq[cq->cqid] = NULL;
> > @@ -812,6 +944,9 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
> > *cmd, NvmeRequest *req)
> > uint32_t result;
> >
> > switch (dw10) {
> > + case NVME_TEMPERATURE_THRESHOLD:
> > + result = cpu_to_le32(n->features.temp_thresh);
> > + break;
> > case NVME_VOLATILE_WRITE_CACHE:
> > result = blk_enable_write_cache(n->conf.blk);
> > trace_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> > @@ -856,6 +991,10 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd
> > *cmd, NvmeRequest *req)
> > uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> >
> > switch (dw10) {
> > + case NVME_TEMPERATURE_THRESHOLD:
> > + n->features.temp_thresh = dw11;
> > + break;
> > +
> > case NVME_VOLATILE_WRITE_CACHE:
> > blk_set_enable_write_cache(n->conf.blk, dw11 & 1);
> > break;
> > @@ -884,6 +1023,8 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeCmd
> > *cmd, NvmeRequest *req)
> > return nvme_del_sq(n, cmd);
> > case NVME_ADM_CMD_CREATE_SQ:
> > return nvme_create_sq(n, cmd);
> > + case NVME_ADM_CMD_GET_LOG_PAGE:
> > + return nvme_get_log(n, cmd, req);
> > case NVME_ADM_CMD_DELETE_CQ:
> > return nvme_del_cq(n, cmd);
> > case NVME_ADM_CMD_CREATE_CQ:
> > @@ -923,6 +1064,7 @@ static void nvme_process_sq(void *opaque)
> > QTAILQ_INSERT_TAIL(&sq->out_req_list, req, entry);
> > memset(&req->cqe, 0, sizeof(req->cqe));
> > req->cqe.cid = cmd.cid;
> > + req->cid = le16_to_cpu(cmd.cid);
>
> If I haven't missed anything this is being used only in one place
> for tracing - is it really worth to duplicate the cid here ?
>
At this point in the series, yes - it is only used once. But it will be
used extensively for tracing in the later patches.
> > - id->lpa = 1 << 0;
> > + id->lpa = 1 << 2;
>
> This sets the bit that states support for GLP command but clears the one
> that states support for per-namespace SMART/Heatld data - is that expected ?
>
Yes, clearing the bit for per-namespace SMART/Health log page
information is intentional. There is no namespace specific information
defined in the namespace so the global and per-namespace log page
contains the same information.
