Re: [PATCH v1 1/1] hw/nvme: Add OCP SMART / Health Information Extended Log Page

[Klaus Jensen]

Hi Stephen,

On Nov 21 15:23, Stephen Bates wrote:
> The Open Compute Project [1] includes a Datacenter NVMe
> SSD Specification [2]. The most recent version of this specification
> (as of November 2024) is 2.6.1. This specification layers on top of
> the NVM Express specifications [3] to provide additional
> functionality. A key part of of this is the 512 Byte OCP SMART / Health
> Information Extended log page that is defined in Section 4.8.6 of the
> specification.
> 
> 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 nvme argumants.
> 
> 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.
> 
> This patch is based on work done by Joel but has been modified enough
> that he requested a co-developed-by tag rather than a signed-off-by.
> 
> [1]: https://www.opencompute.org/
> [2]: 
> https://www.opencompute.org/documents/datacenter-nvme-ssd-specification-v2-6-1-pdf
> [3]: https://nvmexpress.org/specifications/
> 
> Signed-off-by: Stephen Bates <sbates@raithlin.com>
> Co-developed-by: Joel Granados <j.granados@samsung.com>

LGTM, but see a few comments below.

> diff --git a/docs/system/devices/nvme.rst b/docs/system/devices/nvme.rst
> index d2b1ca9645..6509b35fcb 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. We emulate version 5 of this log page.
> +

Do you think we would need to differentiate between version of the
specification?

> @@ -5113,6 +5152,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:

Let's do an enum for these log pages.

>  typedef struct NvmeCtrl {
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 5298bc4a28..df8e45e396 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -1015,6 +1015,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;

Please add a QEMU_BUILD_BUG_ON to check the size.

signature.asc
Description: PGP signature