On Jul 29 13:24, Maxim Levitsky wrote: > On Mon, 2020-07-06 at 08:12 +0200, Klaus Jensen wrote: > > From: Klaus Jensen <k.jen...@samsung.com> > > > > 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. > > > > Note that the LPA field in the Identify Controller data structure > > intentionally has bit 0 cleared because there is no namespace specific > > information in the SMART / Health information log page. > > > > Required for compliance with NVMe revision 1.3d. See NVM Express 1.3d, > > Section 5.14 ("Get Log Page command"). > > > > Signed-off-by: Klaus Jensen <klaus.jen...@cnexlabs.com> > > Signed-off-by: Klaus Jensen <k.jen...@samsung.com> > > Acked-by: Keith Busch <kbu...@kernel.org> > > --- > > hw/block/nvme.c | 140 +++++++++++++++++++++++++++++++++++++++++- > > hw/block/nvme.h | 2 + > > hw/block/trace-events | 2 + > > include/block/nvme.h | 8 ++- > > 4 files changed, 149 insertions(+), 3 deletions(-) > > > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > > index b6bc75eb61a2..7cb3787638f6 100644 > > --- a/hw/block/nvme.c > > +++ b/hw/block/nvme.c > > @@ -606,6 +606,140 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeCmd > > *cmd) > > return NVME_SUCCESS; > > } > > > > +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->dptr.prp1); > > + uint64_t prp2 = le64_to_cpu(cmd->dptr.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; > > + uint64_t read_commands = 0, write_commands = 0; > > + NvmeSmartLog smart; > > + BlockAcctStats *s; > > + > > + if (nsid && nsid != 0xffffffff) { > > + return NVME_INVALID_FIELD | NVME_DNR; > > + } > Correct. > > + > > + 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(&smart, 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); > Tiny nitpick - the spec asks the value to be rounded up >
Ouch. You are correct. I'll swap that for a DIV_ROUND_UP. > > +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); > > + uint8_t lid = dw10 & 0xff; > > + uint8_t lsp = (dw10 >> 8) & 0xf; > > + uint8_t rae = (dw10 >> 15) & 0x1; > > + uint32_t numdl, numdu; > > + uint64_t off, lpol, lpou; > > + size_t len; > > + > Nitpick: don't we want to check NSID=0 || NSID=0xFFFFFFFF here too? > The spec lists Get Log Page with "Yes" under "Namespace Identifier Used" but no log pages in v1.3 or v1.4 are namespace specific so we expect NSID to always be 0 or 0xffffffff. But, there are TPs that have namespace specific log pages (i.e. TP 4053 Zoned Namepaces). So, it is not invalid to have NSID set to something. So, I think we have to defer handling of NSID values to the individual log pages (like we do for the SMART page).