Re: [PATCH v3 3/5] hw/nvme: add basic endurance group support

[Klaus Jensen]

On Feb 24 09:51, Joel Granados wrote:
> On Mon, Feb 20, 2023 at 12:59:24PM +0100, Jesper Devantier wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Add the mandatory Endurance Group identify data structures and log
> > pages.
> > 
> > For now, all namespaces in a subsystem belongs to a single Endurance
> > Group.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/nvme/ctrl.c       | 48 +++++++++++++++++++++++++++++++++++++++++++-
> >  hw/nvme/ns.c         |  3 +++
> >  hw/nvme/nvme.h       |  4 ++++
> >  include/block/nvme.h | 42 +++++++++++++++++++++++++++++++-------
> >  4 files changed, 89 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 99b92ff20b..729ed9adc5 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -4454,6 +4454,44 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t 
> > rae, uint32_t buf_len,
> >      return nvme_c2h(n, (uint8_t *) &smart + off, trans_len, req);
> >  }
> >  
> > +static uint16_t nvme_endgrp_info(NvmeCtrl *n,  uint8_t rae, uint32_t 
> > buf_len,
> > +                                 uint64_t off, NvmeRequest *req)
> > +{
> > +    uint32_t dw11 = le32_to_cpu(req->cmd.cdw11);
> > +    uint16_t endgrpid = (dw11 >> 16) & 0xffff;
> > +    struct nvme_stats stats = {};
> > +    NvmeEndGrpLog info = {};
> > +    int i;
> > +
> > +    if (!n->subsys || endgrpid != 0x1) {
> > +        return NVME_INVALID_FIELD | NVME_DNR;
> > +    }
> > +
> > +    if (off >= sizeof(info)) {
> > +        return NVME_INVALID_FIELD | NVME_DNR;
> > +    }
> > +
> > +    for (i = 1; i <= NVME_MAX_NAMESPACES; i++) {
> > +        NvmeNamespace *ns = nvme_subsys_ns(n->subsys, i);
> > +        if (!ns) {
> > +            continue;
> > +        }
> > +
> > +        nvme_set_blk_stats(ns, &stats);
> > +    }
> > +
> > +    info.data_units_read[0] = cpu_to_le64(stats.units_read / 1000000000);
> > +    info.data_units_written[0] = cpu_to_le64(stats.units_written / 
> > 1000000000);
> > +    info.media_units_written[0] = cpu_to_le64(stats.units_written / 
> > 1000000000);
> 
> This division is a bit strange for me. Maybe I'm missing something:
> 
> NVMe spec states this about Data Units {Written,Read}: "... Contains the
> number of 512 byte data units the host has .... This value is reported
> in thousands (i.e., a value of 1 corresponds to 1,000 units of 512 bytes
> read) and is rounded up (e.g., one indicates that the number of 512 byte
> data units {read,written} is from 1 to 1,000..."
> 
> 1. The way I understand this text from the spec is that if 512 were
>    written, then the data_units_written should contain 1; but as I see
>    it now, it will contain 0. Am I missing something?
> 
> 2. And where is the 512 units represented? I was expecting a bit shift by
>    9.
> 
> 3. And where is it rounding up to thousands?
> 
> Shouldn't it be like this:
> 
>  +    info.data_units_read[0] = cpu_to_le64(DIV_ROUND_UP(stats.units_read >> 
> BDRV_SECTOR_BITS, 1000));
>  +    info.data_units_written[0] = 
> cpu_to_le64(DIV_ROUND_UP(stats.units_written >> BDRV_SECTOR_BITS, 1000));
>  +    info.media_units_written[0] = 
> cpu_to_le64(DIV_ROUND_UP(stats.units_written >> BDRV_SECTOR_BITS, 1000));
> 

Yes and no ;)

First, No, because you are quoting the spec about the SMART log page.
This is the Endurance Group Information log page, where Data Units Read
are total number of bytes, reported in billions.

Secondly, Yes, it is missing the DIV_ROUND_UP().

signature.asc
Description: PGP signature