[PATCH v4 0/2] Add OCP extended log to nvme QEMU

[PATCH v4 0/2] Add OCP extended log to nvme QEMU

[Joel Granados]

The motivation and description are contained in the last patch in this set.
Will copy paste it here for convenience:

    In order to evaluate write amplification factor (WAF) within the storage
    stack it is important to know the number of bytes written to the
    controller. The existing SMART log value of Data Units Written is too
    coarse (given in units of 500 Kb) and so we add the SMART health
    information extended from the OCP specification (given in units of bytes).

    To accommodate different vendor specific specifications like OCP, we add a
    multiplexing function (nvme_vendor_specific_log) which will route to the
    different log functions based on arguments and log ids. We only return the
    OCP extended smart log when the command is 0xC0 and ocp has been turned on
    in the args.

    Though we add the whole nvme smart log extended structure, we only populate
    the physical_media_units_{read,written}, log_page_version and
    log_page_uuid.

V4 changes:
1. Fixed cpu_to_le64 instead of cpu_to_le32
2. Variable naming : uuid -> guid
3. Changed how the guid value appears in the code:
   Used to be:
        smart_l.log_page_uuid[0] = 0xA4F2BFEA2810AFC5;
        smart_l.log_page_uuid[1] = 0xAFD514C97C6F4F9C;

   Now is:
        static const uint8_t guid[16] = {
            0xC5, 0xAF, 0x10, 0x28, 0xEA, 0xBF, 0xF2, 0xA4,
            0x9C, 0x4F, 0x6F, 0x7C, 0xC9, 0x14, 0xD5, 0xAF
        };

   This is different from what @klaus suggested because I want to keep it
   consistent to what nvme-cli currently implements. I think here we can
   either change both nvme-cli and this patch or leave the order of the
   bytes as they are here. This all depends on how you interpret the Spec
   (which is ambiguous)

V3 changes:
1. Corrected a bunch of checkpatch issues. Since I changed the first patch
   I did not include the reviewed-by.
2. Included some documentation in nvme.rst for the ocp argument
3. Squashed the ocp arg changes into the main patch.
4. Fixed several comments and an open parenthesis
5. Hex values are now in lower case.
6. Change the reserved format to rsvd<BYTEOFFSET>
7. Made sure that NvmeCtrl is the first arg in all the functions.
8. Fixed comment on commit of main patch

V2 changes:
1. I moved the ocp parameter from the namespace to the subsystem as it is
   defined there in the OCP specification
2. I now accumulate statistics from all namespaces and report them back on
   the extended log as per the spec.
3. I removed the default case in the switch in nvme_vendor_specific_log as
   it does not have any special function.

Joel Granados (2):
  nvme: Move adjustment of data_units{read,written}
  nvme: Add physical writes/reads from OCP log

 docs/system/devices/nvme.rst |  7 ++++
 hw/nvme/ctrl.c               | 73 +++++++++++++++++++++++++++++++++---
 hw/nvme/nvme.h               |  1 +
 include/block/nvme.h         | 36 ++++++++++++++++++
 4 files changed, 111 insertions(+), 6 deletions(-)

-- 
2.30.2

[PATCH v4 1/2] nvme: Move adjustment of data_units{read,written}

[Joel Granados]

In order to return the units_{read/written} required by the SMART log we
need to shift the number of bytes value by BDRV_SECTORS_BITS and multiply
by 1000. This is a prep patch that moves this adjustment to where the SMART
log is calculated in order to use the stats struct for calculating OCP
extended smart log values.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 hw/nvme/ctrl.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 87aeba0564..bf291f7ffe 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4449,8 +4449,8 @@ static void nvme_set_blk_stats(NvmeNamespace *ns, struct nvme_stats *stats)
 {
     BlockAcctStats *s = blk_get_stats(ns->blkconf.blk);
 
-    stats->units_read += s->nr_bytes[BLOCK_ACCT_READ] >> BDRV_SECTOR_BITS;
-    stats->units_written += s->nr_bytes[BLOCK_ACCT_WRITE] >> BDRV_SECTOR_BITS;
+    stats->units_read += s->nr_bytes[BLOCK_ACCT_READ];
+    stats->units_written += s->nr_bytes[BLOCK_ACCT_WRITE];
     stats->read_commands += s->nr_ops[BLOCK_ACCT_READ];
     stats->write_commands += s->nr_ops[BLOCK_ACCT_WRITE];
 }
@@ -4464,6 +4464,7 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
     uint32_t trans_len;
     NvmeNamespace *ns;
     time_t current_ms;
+    uint64_t u_read, u_written;
 
     if (off >= sizeof(smart)) {
         return NVME_INVALID_FIELD | NVME_DNR;
@@ -4490,10 +4491,11 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
     trans_len = MIN(sizeof(smart) - off, buf_len);
     smart.critical_warning = n->smart_critical_warning;
 
-    smart.data_units_read[0] = cpu_to_le64(DIV_ROUND_UP(stats.units_read,
-                                                        1000));
-    smart.data_units_written[0] = cpu_to_le64(DIV_ROUND_UP(stats.units_written,
-                                                           1000));
+    u_read = DIV_ROUND_UP(stats.units_read >> BDRV_SECTOR_BITS, 1000);
+    u_written = DIV_ROUND_UP(stats.units_written >> BDRV_SECTOR_BITS, 1000);
+
+    smart.data_units_read[0] = cpu_to_le64(u_read);
+    smart.data_units_written[0] = cpu_to_le64(u_written);
     smart.host_read_commands[0] = cpu_to_le64(stats.read_commands);
     smart.host_write_commands[0] = cpu_to_le64(stats.write_commands);
 
-- 
2.30.2

[PATCH v4 2/2] nvme: Add physical writes/reads from OCP log

[Joel Granados]

In order to evaluate write amplification factor (WAF) within the storage
stack it is important to know the number of bytes written to the
controller. The existing SMART log value of Data Units Written is too
coarse (given in units of 500 Kb) and so we add the SMART health
information extended from the OCP specification (given in units of bytes)

We add a controller argument (ocp) that toggles on/off the SMART log
extended structure.  To accommodate different vendor specific specifications
like OCP, we add a multiplexing function (nvme_vendor_specific_log) which
will route to the different log functions based on arguments and log ids.
We only return the OCP extended SMART log when the command is 0xC0 and ocp
has been turned on in the args.

Though we add the whole nvme SMART log extended structure, we only populate
the physical_media_units_{read,written}, log_page_version and
log_page_uuid.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 docs/system/devices/nvme.rst |  7 +++++
 hw/nvme/ctrl.c               | 59 ++++++++++++++++++++++++++++++++++++
 hw/nvme/nvme.h               |  1 +
 include/block/nvme.h         | 36 ++++++++++++++++++++++
 4 files changed, 103 insertions(+)

diff --git a/docs/system/devices/nvme.rst b/docs/system/devices/nvme.rst
index 30f841ef62..1cc5e52c00 100644
--- a/docs/system/devices/nvme.rst
+++ b/docs/system/devices/nvme.rst
@@ -53,6 +53,13 @@ parameters.
   Vendor ID. Set this to ``on`` to revert to the unallocated Intel ID
   previously used.
 
+``ocp`` (default: ``off``)
+  The Open Compute Project defines the Datacenter NVMe SSD Specification that
+  sits on top of NVMe. It describes additional commands and NVMe behaviors
+  specific for the Datacenter. When this option is ``on`` OCP features such as
+  the SMART / Health information extended log become available in the
+  controller.
+
 Additional Namespaces
 ---------------------
 
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index bf291f7ffe..264d9cb220 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4455,6 +4455,45 @@ static void nvme_set_blk_stats(NvmeNamespace *ns, struct nvme_stats *stats)
     stats->write_commands += s->nr_ops[BLOCK_ACCT_WRITE];
 }
 
+static uint16_t nvme_ocp_extended_smart_info(NvmeCtrl *n, uint8_t rae,
+                                             uint32_t buf_len, uint64_t off,
+                                             NvmeRequest *req)
+{
+    NvmeNamespace *ns = NULL;
+    NvmeSmartLogExtended smart_l = { 0 };
+    struct nvme_stats stats = { 0 };
+    uint32_t trans_len;
+
+    if (off >= sizeof(smart_l)) {
+        return NVME_INVALID_FIELD | NVME_DNR;
+    }
+
+    /* accumulate all stats from all namespaces */
+    for (int i = 1; i <= NVME_MAX_NAMESPACES; i++) {
+        ns = nvme_ns(n, i);
+        if (ns) {
+            nvme_set_blk_stats(ns, &stats);
+        }
+    }
+
+    smart_l.physical_media_units_written[0] = cpu_to_le64(stats.units_written);
+    smart_l.physical_media_units_read[0] = cpu_to_le64(stats.units_read);
+    smart_l.log_page_version = 0x0003;
+
+    static const uint8_t guid[16] = {
+        0xC5, 0xAF, 0x10, 0x28, 0xEA, 0xBF, 0xF2, 0xA4,
+        0x9C, 0x4F, 0x6F, 0x7C, 0xC9, 0x14, 0xD5, 0xAF
+    };
+    memcpy(smart_l.log_page_guid, guid, sizeof(smart_l.log_page_guid));
+
+    if (!rae) {
+        nvme_clear_events(n, NVME_AER_TYPE_SMART);
+    }
+
+    trans_len = MIN(sizeof(smart_l) - off, buf_len);
+    return nvme_c2h(n, (uint8_t *) &smart_l + off, trans_len, req);
+}
+
 static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
                                 uint64_t off, NvmeRequest *req)
 {
@@ -4642,6 +4681,23 @@ static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint8_t csi, uint32_t buf_len,
     return nvme_c2h(n, ((uint8_t *)&log) + off, trans_len, req);
 }
 
+static uint16_t nvme_vendor_specific_log(NvmeCtrl *n, uint8_t rae,
+                                         uint32_t buf_len, uint64_t off,
+                                         NvmeRequest *req, uint8_t lid)
+{
+    switch (lid) {
+    case 0xc0:
+        if (n->params.ocp) {
+            return nvme_ocp_extended_smart_info(n, rae, buf_len, off, req);
+        }
+        break;
+        /* add a case for each additional vendor specific log id */
+    }
+
+    trace_pci_nvme_err_invalid_log_page(nvme_cid(req), lid);
+    return NVME_INVALID_FIELD | NVME_DNR;
+}
+
 static uint16_t nvme_get_log(NvmeCtrl *n, NvmeRequest *req)
 {
     NvmeCmd *cmd = &req->cmd;
@@ -4683,6 +4739,8 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeRequest *req)
         return nvme_error_info(n, rae, len, off, req);
     case NVME_LOG_SMART_INFO:
         return nvme_smart_info(n, rae, len, off, req);
+    case NVME_LOG_VENDOR_START...NVME_LOG_VENDOR_END:
+        return nvme_vendor_specific_log(n, rae, len, off, req, lid);
     case NVME_LOG_FW_SLOT_INFO:
         return nvme_fw_log_info(n, len, off, req);
     case NVME_LOG_CHANGED_NSLIST:
@@ -7685,6 +7743,7 @@ static Property nvme_props[] = {
                       params.sriov_max_vi_per_vf, 0),
     DEFINE_PROP_UINT8("sriov_max_vq_per_vf", NvmeCtrl,
                       params.sriov_max_vq_per_vf, 0),
+    DEFINE_PROP_BOOL("ocp", NvmeCtrl, params.ocp, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index 79f5c281c2..d7f486f795 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -427,6 +427,7 @@ typedef struct NvmeParams {
     uint16_t sriov_vi_flexible;
     uint8_t  sriov_max_vq_per_vf;
     uint8_t  sriov_max_vi_per_vf;
+    bool     ocp;
 } NvmeParams;
 
 typedef struct NvmeCtrl {
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 8027b7126b..fc020abced 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -978,6 +978,40 @@ typedef struct QEMU_PACKED NvmeSmartLog {
     uint8_t     reserved2[320];
 } NvmeSmartLog;
 
+typedef struct QEMU_PACKED NvmeSmartLogExtended {
+    uint64_t    physical_media_units_written[2];
+    uint64_t    physical_media_units_read[2];
+    uint64_t    bad_user_blocks;
+    uint64_t    bad_system_nand_blocks;
+    uint64_t    xor_recovery_count;
+    uint64_t    uncorrectable_read_error_count;
+    uint64_t    soft_ecc_error_count;
+    uint64_t    end2end_correction_counts;
+    uint8_t     system_data_percent_used;
+    uint8_t     refresh_counts[7];
+    uint64_t    user_data_erase_counts;
+    uint16_t    thermal_throttling_stat_and_count;
+    uint16_t    dssd_spec_version[3];
+    uint64_t    pcie_correctable_error_count;
+    uint32_t    incomplete_shutdowns;
+    uint32_t    rsvd116;
+    uint8_t     percent_free_blocks;
+    uint8_t     rsvd121[7];
+    uint16_t    capacity_health;
+    uint8_t     nvme_errata_ver;
+    uint8_t     rsvd131[5];
+    uint64_t    unaligned_io;
+    uint64_t    security_ver_num;
+    uint64_t    total_nuse;
+    uint64_t    plp_start_count[2];
+    uint64_t    endurance_estimate[2];
+    uint64_t    pcie_retraining_count;
+    uint64_t    power_state_change_count;
+    uint8_t     rsvd208[286];
+    uint16_t    log_page_version;
+    uint64_t    log_page_guid[2];
+} NvmeSmartLogExtended;
+
 #define NVME_SMART_WARN_MAX     6
 enum NvmeSmartWarn {
     NVME_SMART_SPARE                  = 1 << 0,
@@ -1010,6 +1044,8 @@ enum NvmeLogIdentifier {
     NVME_LOG_FW_SLOT_INFO   = 0x03,
     NVME_LOG_CHANGED_NSLIST = 0x04,
     NVME_LOG_CMD_EFFECTS    = 0x05,
+    NVME_LOG_VENDOR_START   = 0xc0,
+    NVME_LOG_VENDOR_END     = 0xff,
 };
 
 typedef struct QEMU_PACKED NvmePSD {
-- 
2.30.2

Re: [PATCH v4 0/2] Add OCP extended log to nvme QEMU

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 3324 bytes --]

ping.

Is the solution to the guid constant ok?

Best

On Fri, Nov 25, 2022 at 10:48:06AM +0100, Joel Granados wrote:
> The motivation and description are contained in the last patch in this set.
> Will copy paste it here for convenience:
> 
>     In order to evaluate write amplification factor (WAF) within the storage
>     stack it is important to know the number of bytes written to the
>     controller. The existing SMART log value of Data Units Written is too
>     coarse (given in units of 500 Kb) and so we add the SMART health
>     information extended from the OCP specification (given in units of bytes).
> 
>     To accommodate different vendor specific specifications like OCP, we add a
>     multiplexing function (nvme_vendor_specific_log) which will route to the
>     different log functions based on arguments and log ids. We only return the
>     OCP extended smart log when the command is 0xC0 and ocp has been turned on
>     in the args.
> 
>     Though we add the whole nvme smart log extended structure, we only populate
>     the physical_media_units_{read,written}, log_page_version and
>     log_page_uuid.
> 
> V4 changes:
> 1. Fixed cpu_to_le64 instead of cpu_to_le32
> 2. Variable naming : uuid -> guid
> 3. Changed how the guid value appears in the code:
>    Used to be:
>         smart_l.log_page_uuid[0] = 0xA4F2BFEA2810AFC5;
>         smart_l.log_page_uuid[1] = 0xAFD514C97C6F4F9C;
> 
>    Now is:
>         static const uint8_t guid[16] = {
>             0xC5, 0xAF, 0x10, 0x28, 0xEA, 0xBF, 0xF2, 0xA4,
>             0x9C, 0x4F, 0x6F, 0x7C, 0xC9, 0x14, 0xD5, 0xAF
>         };
> 
>    This is different from what @klaus suggested because I want to keep it
>    consistent to what nvme-cli currently implements. I think here we can
>    either change both nvme-cli and this patch or leave the order of the
>    bytes as they are here. This all depends on how you interpret the Spec
>    (which is ambiguous)
> 
> V3 changes:
> 1. Corrected a bunch of checkpatch issues. Since I changed the first patch
>    I did not include the reviewed-by.
> 2. Included some documentation in nvme.rst for the ocp argument
> 3. Squashed the ocp arg changes into the main patch.
> 4. Fixed several comments and an open parenthesis
> 5. Hex values are now in lower case.
> 6. Change the reserved format to rsvd<BYTEOFFSET>
> 7. Made sure that NvmeCtrl is the first arg in all the functions.
> 8. Fixed comment on commit of main patch
> 
> V2 changes:
> 1. I moved the ocp parameter from the namespace to the subsystem as it is
>    defined there in the OCP specification
> 2. I now accumulate statistics from all namespaces and report them back on
>    the extended log as per the spec.
> 3. I removed the default case in the switch in nvme_vendor_specific_log as
>    it does not have any special function.
> 
> Joel Granados (2):
>   nvme: Move adjustment of data_units{read,written}
>   nvme: Add physical writes/reads from OCP log
> 
>  docs/system/devices/nvme.rst |  7 ++++
>  hw/nvme/ctrl.c               | 73 +++++++++++++++++++++++++++++++++---
>  hw/nvme/nvme.h               |  1 +
>  include/block/nvme.h         | 36 ++++++++++++++++++
>  4 files changed, 111 insertions(+), 6 deletions(-)
> 
> -- 
> 2.30.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 0/2] Add OCP extended log to nvme QEMU

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 3299 bytes --]

On Nov 25 10:48, Joel Granados wrote:
> The motivation and description are contained in the last patch in this set.
> Will copy paste it here for convenience:
> 
>     In order to evaluate write amplification factor (WAF) within the storage
>     stack it is important to know the number of bytes written to the
>     controller. The existing SMART log value of Data Units Written is too
>     coarse (given in units of 500 Kb) and so we add the SMART health
>     information extended from the OCP specification (given in units of bytes).
> 
>     To accommodate different vendor specific specifications like OCP, we add a
>     multiplexing function (nvme_vendor_specific_log) which will route to the
>     different log functions based on arguments and log ids. We only return the
>     OCP extended smart log when the command is 0xC0 and ocp has been turned on
>     in the args.
> 
>     Though we add the whole nvme smart log extended structure, we only populate
>     the physical_media_units_{read,written}, log_page_version and
>     log_page_uuid.
> 
> V4 changes:
> 1. Fixed cpu_to_le64 instead of cpu_to_le32
> 2. Variable naming : uuid -> guid
> 3. Changed how the guid value appears in the code:
>    Used to be:
>         smart_l.log_page_uuid[0] = 0xA4F2BFEA2810AFC5;
>         smart_l.log_page_uuid[1] = 0xAFD514C97C6F4F9C;
> 
>    Now is:
>         static const uint8_t guid[16] = {
>             0xC5, 0xAF, 0x10, 0x28, 0xEA, 0xBF, 0xF2, 0xA4,
>             0x9C, 0x4F, 0x6F, 0x7C, 0xC9, 0x14, 0xD5, 0xAF
>         };
> 
>    This is different from what @klaus suggested because I want to keep it
>    consistent to what nvme-cli currently implements. I think here we can
>    either change both nvme-cli and this patch or leave the order of the
>    bytes as they are here. This all depends on how you interpret the Spec
>    (which is ambiguous)
> 
> V3 changes:
> 1. Corrected a bunch of checkpatch issues. Since I changed the first patch
>    I did not include the reviewed-by.
> 2. Included some documentation in nvme.rst for the ocp argument
> 3. Squashed the ocp arg changes into the main patch.
> 4. Fixed several comments and an open parenthesis
> 5. Hex values are now in lower case.
> 6. Change the reserved format to rsvd<BYTEOFFSET>
> 7. Made sure that NvmeCtrl is the first arg in all the functions.
> 8. Fixed comment on commit of main patch
> 
> V2 changes:
> 1. I moved the ocp parameter from the namespace to the subsystem as it is
>    defined there in the OCP specification
> 2. I now accumulate statistics from all namespaces and report them back on
>    the extended log as per the spec.
> 3. I removed the default case in the switch in nvme_vendor_specific_log as
>    it does not have any special function.
> 
> Joel Granados (2):
>   nvme: Move adjustment of data_units{read,written}
>   nvme: Add physical writes/reads from OCP log
> 
>  docs/system/devices/nvme.rst |  7 ++++
>  hw/nvme/ctrl.c               | 73 +++++++++++++++++++++++++++++++++---
>  hw/nvme/nvme.h               |  1 +
>  include/block/nvme.h         | 36 ++++++++++++++++++
>  4 files changed, 111 insertions(+), 6 deletions(-)
> 
> -- 
> 2.30.2
> 

LGTM,

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]