On Tue, Nov 15, 2022 at 12:26:17PM +0100, Klaus Jensen wrote: > On Nov 14 14:50, Joel Granados wrote: > > 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 accomodate 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.grana...@samsung.com> > > > > squash with main > > > > Signed-off-by: Joel Granados <j.grana...@samsung.com> > > Looks like you slightly messed up the squash ;) oops. that is my bad
> > Also, squash the previous patch (adding the ocp parameter) into this. Here I wanted to keep the introduction of the argument separate. In any case, I'll squash it with the other one. > Please add a note in the documentation (docs/system/devices/nvme.rst) > about this parameter. Of course. I always forget documentation. I'll add it under the "Controller Emulation" section and I'll call it ``ocp`` > > > --- > > hw/nvme/ctrl.c | 56 ++++++++++++++++++++++++++++++++++++++++++++ > > include/block/nvme.h | 36 ++++++++++++++++++++++++++++ > > 2 files changed, 92 insertions(+) > > > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c > > index 220683201a..5e6a8150a2 100644 > > --- a/hw/nvme/ctrl.c > > +++ b/hw/nvme/ctrl.c > > @@ -4455,6 +4455,42 @@ 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_ext = { 0 }; > > + struct nvme_stats stats = { 0 }; > > + uint32_t trans_len; > > + > > + if (off >= sizeof(smart_ext)) { > > + return NVME_INVALID_FIELD | NVME_DNR; > > + } > > + > > + // Accumulate all stats from all namespaces > > Use /* lower-case and no period */ for one sentence, one line comments. > > I think scripts/checkpatch.pl picks this up. There is a checkpatch like in the kernel. Fantastic! I'll make a note to use it from now on. > > > + for (int i = 1; i <= NVME_MAX_NAMESPACES; i++) { > > + ns = nvme_ns(n, i); > > + if (ns) > > + { > > Paranthesis go on the same line as the `if`. of course > > > + nvme_set_blk_stats(ns, &stats); > > + } > > + } > > + > > + smart_ext.physical_media_units_written[0] = > > cpu_to_le32(stats.units_written); > > + smart_ext.physical_media_units_read[0] = cpu_to_le32(stats.units_read); > > + smart_ext.log_page_version = 0x0003; > > + smart_ext.log_page_uuid[0] = 0xA4F2BFEA2810AFC5; > > + smart_ext.log_page_uuid[1] = 0xAFD514C97C6F4F9C; > > + > > + if (!rae) { > > + nvme_clear_events(n, NVME_AER_TYPE_SMART); > > + } > > + > > + trans_len = MIN(sizeof(smart_ext) - off, buf_len); > > + return nvme_c2h(n, (uint8_t *) &smart_ext + 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 +4678,24 @@ 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(uint8_t lid, NvmeCtrl *n, uint8_t > > rae, > > + uint32_t buf_len, uint64_t off, > > + NvmeRequest *req) > > `NvmeCtrl *n` must be first parameter. Any reason why this is the case? I'll change it in my code, but would be nice to understand the reason. > > > +{ > > + NvmeSubsystem *subsys = n->subsys; > > + switch (lid) { > > + case NVME_LOG_VENDOR_START: > > In this particular case, I think it is more clear if you simply use the > hex value directly. The "meaning" of the log page id depends on if or > not this is an controller implementing the OCP spec. Agreed > > > + if (subsys->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 */ > > Lower-case the comment. ok > > > + } > > + > > + 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 +4737,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(lid, n, rae, len, off, req); > > case NVME_LOG_FW_SLOT_INFO: > > return nvme_fw_log_info(n, len, off, req); > > case NVME_LOG_CHANGED_NSLIST: > > diff --git a/include/block/nvme.h b/include/block/nvme.h > > index 8027b7126b..2ab0dca529 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 reserved0; > > I know that it is not totally consistent across the code, but please use > `rsvd<BYTEOFFSET>` for the reserved field names. The above would be > `rsvd116` if I am not mistaken. ok > > > + uint8_t percent_free_blocks; > > + uint8_t reserved1[7]; > > + uint16_t capacity_health; > > + uint8_t nvme_errata_ver; > > + uint8_t reserved2[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 reserved3[286]; > > + uint16_t log_page_version; > > + uint64_t log_page_uuid[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, > > Existing style is generally lower-case hex. No problem Thx for the review. Will post V3 after running checkpatch > > > }; > > > > typedef struct QEMU_PACKED NvmePSD { > > -- > > 2.30.2 > > > >
signature.asc
Description: PGP signature