Re: [PATCH v2 08/20] nvme: add support for the get log page command
On Tue, Nov 12, 2019 at 03:04:52PM +, Beata Michalska wrote:
> Hi Klaus,
>
>
> On Tue, 15 Oct 2019 at 11:45, Klaus Jensen wrote:
> > +if (!nsid || (nsid != 0x && 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 0x 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 0x).
> > +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(>out_req_list, req, entry);
> > memset(>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.
Re: [PATCH v2 08/20] nvme: add support for the get log page command
Hi Klaus,
On Tue, 15 Oct 2019 at 11:45, Klaus Jensen wrote:
>
> Add support for the Get Log Page command and basic implementations
> of the mandatory Error Information, SMART/Health Information and
> Firmware Slot Information log pages.
>
> In violation of the specification, the SMART/Health Information log page
> does not persist information over the lifetime of the controller because
> the device has no place to store such persistent state.
>
> Required for compliance with NVMe revision 1.2.1. See NVM Express 1.2.1,
> Section 5.10 ("Get Log Page command").
>
> Signed-off-by: Klaus Jensen
> ---
> hw/block/nvme.c | 150 +-
> hw/block/nvme.h | 9 ++-
> hw/block/trace-events | 2 +
> include/block/nvme.h | 2 +-
> 4 files changed, 160 insertions(+), 3 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 1fdb3b8655ed..4412a3bea3bc 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -44,6 +44,7 @@
> #include "nvme.h"
>
> #define NVME_MAX_QS PCI_MSIX_FLAGS_QSIZE
> +#define NVME_TEMPERATURE 0x143
>
> #define NVME_GUEST_ERR(trace, fmt, ...) \
> do { \
> @@ -577,6 +578,137 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeCmd
> *cmd)
> return NVME_SUCCESS;
> }
>
> +static uint16_t nvme_error_info(NvmeCtrl *n, NvmeCmd *cmd,
> +uint32_t buf_len, uint64_t off, NvmeRequest *req)
> +{
> +uint32_t trans_len;
> +uint64_t prp1 = le64_to_cpu(cmd->prp1);
> +uint64_t prp2 = le64_to_cpu(cmd->prp2);
> +
> +if (off > sizeof(*n->elpes) * (n->params.elpe + 1)) {
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
> +trans_len = MIN(sizeof(*n->elpes) * (n->params.elpe + 1) - off, buf_len);
> +
> +return nvme_dma_read_prp(n, (uint8_t *) n->elpes + off, trans_len, prp1,
> +prp2);
> +}
> +
> +static uint16_t nvme_smart_info(NvmeCtrl *n, NvmeCmd *cmd, uint32_t buf_len,
> +uint64_t off, NvmeRequest *req)
> +{
> +uint64_t prp1 = le64_to_cpu(cmd->prp1);
> +uint64_t prp2 = le64_to_cpu(cmd->prp2);
> +uint32_t nsid = le32_to_cpu(cmd->nsid);
> +
> +uint32_t trans_len;
> +time_t current_ms;
> +uint64_t units_read = 0, units_written = 0, read_commands = 0,
> +write_commands = 0;
> +NvmeSmartLog smart;
> +BlockAcctStats *s;
> +
> +if (!nsid || (nsid != 0x && 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 0x and either
abort the command or treat that as request for controller specific data.
> +s = blk_get_stats(n->conf.blk);
> +
> +units_read = s->nr_bytes[BLOCK_ACCT_READ] >> BDRV_SECTOR_BITS;
> +units_written = s->nr_bytes[BLOCK_ACCT_WRITE] >> BDRV_SECTOR_BITS;
> +read_commands = s->nr_ops[BLOCK_ACCT_READ];
> +write_commands = s->nr_ops[BLOCK_ACCT_WRITE];
> +
> +if (off > sizeof(smart)) {
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
> +trans_len = MIN(sizeof(smart) - off, buf_len);
> +
> +memset(, 0x0, sizeof(smart));
> +
> +smart.data_units_read[0] = cpu_to_le64(units_read / 1000);
> +smart.data_units_written[0] = cpu_to_le64(units_written / 1000);
> +smart.host_read_commands[0] = cpu_to_le64(read_commands);
> +smart.host_write_commands[0] = cpu_to_le64(write_commands);
> +
> +smart.number_of_error_log_entries[0] = cpu_to_le64(0);
> +smart.temperature[0] = n->temperature & 0xff;
> +smart.temperature[1] = (n->temperature >> 8) & 0xff;
> +
> +if (n->features.temp_thresh <= n->temperature) {
> +smart.critical_warning |= NVME_SMART_TEMPERATURE;
> +}
> +
> +current_ms = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
> +smart.power_on_hours[0] = cpu_to_le64(
> +(((current_ms - n->starttime_ms) / 1000) / 60) / 60);
> +
> +return nvme_dma_read_prp(n, (uint8_t *) + off, trans_len, prp1,
> +prp2);
> +}
> +
> +static uint16_t nvme_fw_log_info(NvmeCtrl *n, NvmeCmd *cmd, uint32_t buf_len,
> +uint64_t off, NvmeRequest *req)
> +{
> +uint32_t trans_len;
> +uint64_t prp1 = le64_to_cpu(cmd->prp1);
> +uint64_t prp2 = le64_to_cpu(cmd->prp2);
> +NvmeFwSlotInfoLog fw_log;
> +
> +if (off > sizeof(fw_log)) {
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
> +memset(_log, 0, sizeof(NvmeFwSlotInfoLog));
> +
> +trans_len = MIN(sizeof(fw_log) - off, buf_len);
> +
> +return nvme_dma_read_prp(n, (uint8_t *) _log + off, trans_len, prp1,
> +prp2);
> +}
> +
> +static uint16_t nvme_get_log(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> +{
> +uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> +uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> +uint32_t dw12 = le32_to_cpu(cmd->cdw12);
> +uint32_t dw13 =
[PATCH v2 08/20] nvme: add support for the get log page command
Add support for the Get Log Page command and basic implementations
of the mandatory Error Information, SMART/Health Information and
Firmware Slot Information log pages.
In violation of the specification, the SMART/Health Information log page
does not persist information over the lifetime of the controller because
the device has no place to store such persistent state.
Required for compliance with NVMe revision 1.2.1. See NVM Express 1.2.1,
Section 5.10 ("Get Log Page command").
Signed-off-by: Klaus Jensen
---
hw/block/nvme.c | 150 +-
hw/block/nvme.h | 9 ++-
hw/block/trace-events | 2 +
include/block/nvme.h | 2 +-
4 files changed, 160 insertions(+), 3 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 1fdb3b8655ed..4412a3bea3bc 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -44,6 +44,7 @@
#include "nvme.h"
#define NVME_MAX_QS PCI_MSIX_FLAGS_QSIZE
+#define NVME_TEMPERATURE 0x143
#define NVME_GUEST_ERR(trace, fmt, ...) \
do { \
@@ -577,6 +578,137 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeCmd *cmd)
return NVME_SUCCESS;
}
+static uint16_t nvme_error_info(NvmeCtrl *n, NvmeCmd *cmd,
+uint32_t buf_len, uint64_t off, NvmeRequest *req)
+{
+uint32_t trans_len;
+uint64_t prp1 = le64_to_cpu(cmd->prp1);
+uint64_t prp2 = le64_to_cpu(cmd->prp2);
+
+if (off > sizeof(*n->elpes) * (n->params.elpe + 1)) {
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+
+trans_len = MIN(sizeof(*n->elpes) * (n->params.elpe + 1) - off, buf_len);
+
+return nvme_dma_read_prp(n, (uint8_t *) n->elpes + off, trans_len, prp1,
+prp2);
+}
+
+static uint16_t nvme_smart_info(NvmeCtrl *n, NvmeCmd *cmd, uint32_t buf_len,
+uint64_t off, NvmeRequest *req)
+{
+uint64_t prp1 = le64_to_cpu(cmd->prp1);
+uint64_t prp2 = le64_to_cpu(cmd->prp2);
+uint32_t nsid = le32_to_cpu(cmd->nsid);
+
+uint32_t trans_len;
+time_t current_ms;
+uint64_t units_read = 0, units_written = 0, read_commands = 0,
+write_commands = 0;
+NvmeSmartLog smart;
+BlockAcctStats *s;
+
+if (!nsid || (nsid != 0x && nsid > n->num_namespaces)) {
+trace_nvme_err_invalid_ns(nsid, n->num_namespaces);
+return NVME_INVALID_NSID | NVME_DNR;
+}
+
+s = blk_get_stats(n->conf.blk);
+
+units_read = s->nr_bytes[BLOCK_ACCT_READ] >> BDRV_SECTOR_BITS;
+units_written = s->nr_bytes[BLOCK_ACCT_WRITE] >> BDRV_SECTOR_BITS;
+read_commands = s->nr_ops[BLOCK_ACCT_READ];
+write_commands = s->nr_ops[BLOCK_ACCT_WRITE];
+
+if (off > sizeof(smart)) {
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+
+trans_len = MIN(sizeof(smart) - off, buf_len);
+
+memset(, 0x0, sizeof(smart));
+
+smart.data_units_read[0] = cpu_to_le64(units_read / 1000);
+smart.data_units_written[0] = cpu_to_le64(units_written / 1000);
+smart.host_read_commands[0] = cpu_to_le64(read_commands);
+smart.host_write_commands[0] = cpu_to_le64(write_commands);
+
+smart.number_of_error_log_entries[0] = cpu_to_le64(0);
+smart.temperature[0] = n->temperature & 0xff;
+smart.temperature[1] = (n->temperature >> 8) & 0xff;
+
+if (n->features.temp_thresh <= n->temperature) {
+smart.critical_warning |= NVME_SMART_TEMPERATURE;
+}
+
+current_ms = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
+smart.power_on_hours[0] = cpu_to_le64(
+(((current_ms - n->starttime_ms) / 1000) / 60) / 60);
+
+return nvme_dma_read_prp(n, (uint8_t *) + off, trans_len, prp1,
+prp2);
+}
+
+static uint16_t nvme_fw_log_info(NvmeCtrl *n, NvmeCmd *cmd, uint32_t buf_len,
+uint64_t off, NvmeRequest *req)
+{
+uint32_t trans_len;
+uint64_t prp1 = le64_to_cpu(cmd->prp1);
+uint64_t prp2 = le64_to_cpu(cmd->prp2);
+NvmeFwSlotInfoLog fw_log;
+
+if (off > sizeof(fw_log)) {
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+
+memset(_log, 0, sizeof(NvmeFwSlotInfoLog));
+
+trans_len = MIN(sizeof(fw_log) - off, buf_len);
+
+return nvme_dma_read_prp(n, (uint8_t *) _log + off, trans_len, prp1,
+prp2);
+}
+
+static uint16_t nvme_get_log(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
+{
+uint32_t dw10 = le32_to_cpu(cmd->cdw10);
+uint32_t dw11 = le32_to_cpu(cmd->cdw11);
+uint32_t dw12 = le32_to_cpu(cmd->cdw12);
+uint32_t dw13 = le32_to_cpu(cmd->cdw13);
+uint16_t lid = dw10 & 0xff;
+uint8_t rae = (dw10 >> 15) & 0x1;
+uint32_t numdl, numdu;
+uint64_t off, lpol, lpou;
+size_t len;
+
+numdl = (dw10 >> 16);
+numdu = (dw11 & 0x);
+lpol = dw12;
+lpou = dw13;
+
+len = (((numdu << 16) | numdl) + 1) << 2;
+off = (lpou << 32ULL) | lpol;
+
+if (off & 0x3) {
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+
+trace_nvme_get_log(req->cid, lid, rae, len, off);
+
+switch (lid) {
+case NVME_LOG_ERROR_INFO:
+return nvme_error_info(n, cmd, len, off, req);
+case
