[PATCH 0/2] hw/block/nvme: add support for telemetry log pages

[PATCH 0/2] hw/block/nvme: add support for telemetry log pages

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

This adds support for the telemetry log pages and fixes up the
controller IEEE OUI.

Gollu Appalanaidu (2):
  hw/block/nvme: use locally assigned QEMU IEEE OUI
  hw/block/nvme: add nvme telemetry log support

 include/block/nvme.h  | 23 ++++++++++++++++++++---
 hw/block/nvme.c       | 37 ++++++++++++++++++++++++++++++++++---
 hw/block/trace-events |  1 +
 3 files changed, 55 insertions(+), 6 deletions(-)

-- 
2.30.0

[PATCH 1/2] hw/block/nvme: use locally assigned QEMU IEEE OUI

[Klaus Jensen]

From: Gollu Appalanaidu <anaidu.gollu@samsung.com>

Commit 6eb7a071292a ("hw/block/nvme: change controller pci id") changed
the controller to use a Red Hat assigned PCI Device and Vendor ID, but
did not change the IEEE OUI away from the Intel IEEE OUI.

Fix that and use the locally assigned QEMU IEEE OUI instead.

Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index c2f0c88fbf39..547a3073ef1b 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4686,8 +4686,8 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
 
     id->rab = 6;
     id->ieee[0] = 0x00;
-    id->ieee[1] = 0x02;
-    id->ieee[2] = 0xb3;
+    id->ieee[1] = 0x54;
+    id->ieee[2] = 0x52;
     id->mdts = n->params.mdts;
     id->ver = cpu_to_le32(NVME_SPEC_VER);
     id->oacs = cpu_to_le16(0);
-- 
2.30.0

[PATCH 2/2] hw/block/nvme: add nvme telemetry log support

[Klaus Jensen]

From: Gollu Appalanaidu <anaidu.gollu@samsung.com>

This adds basic support for Telemetry Host Initiated and Controller
Initiated log pages. The controller does not record any telemetry but
provides the log pages.

Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 include/block/nvme.h  | 23 ++++++++++++++++++++---
 hw/block/nvme.c       | 33 ++++++++++++++++++++++++++++++++-
 hw/block/trace-events |  1 +
 3 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/include/block/nvme.h b/include/block/nvme.h
index f82b5ffc2c1d..95ffe4412679 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -922,6 +922,19 @@ typedef struct NvmeEffectsLog {
     uint8_t     resv[2048];
 } NvmeEffectsLog;
 
+typedef struct NvmeTelemetry {
+    uint8_t     lid;
+    uint8_t     rsvd1[4];
+    uint8_t     ieee[3];
+    uint16_t    tda1lb;
+    uint16_t    tda2lb;
+    uint16_t    tda3lb;
+    uint8_t     rsvd14[368];
+    uint8_t     tda;
+    uint8_t     tdgn;
+    uint8_t     ri[128];
+} NvmeTelemetry;
+
 enum {
     NVME_CMD_EFF_CSUPP      = 1 << 0,
     NVME_CMD_EFF_LBCC       = 1 << 1,
@@ -937,6 +950,8 @@ enum NvmeLogIdentifier {
     NVME_LOG_SMART_INFO     = 0x02,
     NVME_LOG_FW_SLOT_INFO   = 0x03,
     NVME_LOG_CMD_EFFECTS    = 0x05,
+    NVME_LOG_TEL_HOST_INIT  = 0x07,
+    NVME_LOG_TEL_CTRL_INIT  = 0x08,
 };
 
 typedef struct QEMU_PACKED NvmePSD {
@@ -1067,9 +1082,10 @@ enum NvmeIdCtrlFrmw {
 };
 
 enum NvmeIdCtrlLpa {
-    NVME_LPA_NS_SMART = 1 << 0,
-    NVME_LPA_CSE      = 1 << 1,
-    NVME_LPA_EXTENDED = 1 << 2,
+    NVME_LPA_NS_SMART   = 1 << 0,
+    NVME_LPA_CSE        = 1 << 1,
+    NVME_LPA_EXTENDED   = 1 << 2,
+    NVME_LPA_TELEMETRY  = 1 << 3,
 };
 
 enum NvmeIdCtrlCmic {
@@ -1395,5 +1411,6 @@ static inline void _nvme_check_size(void)
     QEMU_BUILD_BUG_ON(sizeof(NvmeSglDescriptor) != 16);
     QEMU_BUILD_BUG_ON(sizeof(NvmeIdNsDescr) != 4);
     QEMU_BUILD_BUG_ON(sizeof(NvmeZoneDescr) != 64);
+    QEMU_BUILD_BUG_ON(sizeof(NvmeTelemetry) != 512);
 }
 #endif
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 547a3073ef1b..01b92a6f66c8 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2908,6 +2908,33 @@ static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint8_t csi, uint32_t buf_len,
                     DMA_DIRECTION_FROM_DEVICE, req);
 }
 
+static uint16_t nvme_telemetry(NvmeCtrl *n, uint8_t lid, uint32_t buf_len,
+                               uint64_t off, NvmeRequest *req)
+{
+    NvmeTelemetry telemetry = {
+        .lid     = lid,
+        .ieee[0] = 0x00,
+        .ieee[1] = 0x54,
+        .ieee[2] = 0x52,
+    };
+
+    uint32_t trans_len;
+
+    if (buf_len & 0x1ff || off & 0x1ff) {
+        trace_pci_nvme_telemetry_log_invalid(buf_len, off);
+        return NVME_INVALID_FIELD | NVME_DNR;
+    }
+
+    if (off >= sizeof(telemetry)) {
+        return NVME_INVALID_FIELD | NVME_DNR;
+    }
+
+    trans_len = MIN(sizeof(telemetry) - off, buf_len);
+
+    return nvme_dma(n, (uint8_t *)&telemetry + off, trans_len,
+                    DMA_DIRECTION_FROM_DEVICE, req);
+}
+
 static uint16_t nvme_get_log(NvmeCtrl *n, NvmeRequest *req)
 {
     NvmeCmd *cmd = &req->cmd;
@@ -2954,6 +2981,9 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeRequest *req)
         return nvme_fw_log_info(n, len, off, req);
     case NVME_LOG_CMD_EFFECTS:
         return nvme_cmd_effects(n, csi, len, off, req);
+    case NVME_LOG_TEL_HOST_INIT:
+    case NVME_LOG_TEL_CTRL_INIT:
+        return nvme_telemetry(n, lid, len, off, req);
     default:
         trace_pci_nvme_err_invalid_log_page(nvme_cid(req), lid);
         return NVME_INVALID_FIELD | NVME_DNR;
@@ -4707,7 +4737,8 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
     id->acl = 3;
     id->aerl = n->params.aerl;
     id->frmw = (NVME_NUM_FW_SLOTS << 1) | NVME_FRMW_SLOT1_RO;
-    id->lpa = NVME_LPA_NS_SMART | NVME_LPA_CSE | NVME_LPA_EXTENDED;
+    id->lpa = NVME_LPA_NS_SMART | NVME_LPA_CSE | NVME_LPA_EXTENDED |
+        NVME_LPA_TELEMETRY;
 
     /* recommended default value (~70 C) */
     id->wctemp = cpu_to_le16(NVME_TEMPERATURE_WARNING);
diff --git a/hw/block/trace-events b/hw/block/trace-events
index b6e972d733a6..500d991a0a14 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -110,6 +110,7 @@ pci_nvme_set_descriptor_extension(uint64_t slba, uint32_t zone_idx) "set zone de
 pci_nvme_zd_extension_set(uint32_t zone_idx) "set descriptor extension for zone_idx=%"PRIu32""
 pci_nvme_clear_ns_close(uint32_t state, uint64_t slba) "zone state=%"PRIu32", slba=%"PRIu64" transitioned to Closed state"
 pci_nvme_clear_ns_reset(uint32_t state, uint64_t slba) "zone state=%"PRIu32", slba=%"PRIu64" transitioned to Empty state"
+pci_nvme_telemetry_log_invalid(size_t len, uint64_t offset) "len %zu offset %"PRIu64""
 
 # nvme traces for error conditions
 pci_nvme_err_mdts(uint16_t cid, size_t len) "cid %"PRIu16" len %zu"
-- 
2.30.0

Re: [PATCH 0/2] hw/block/nvme: add support for telemetry log pages

[Keith Busch]

On Mon, Feb 08, 2021 at 03:10:10PM +0100, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> This adds support for the telemetry log pages and fixes up the
> controller IEEE OUI.

Patch 1 is fine.

I don't see the point for patch 2. We don't need an empty implementation
for every optional spec feature just because it's there. The features
we do implement ought to provide something useful, yeah?

Re: [PATCH 0/2] hw/block/nvme: add support for telemetry log pages

[Klaus Jensen]

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

On Feb  9 00:40, Keith Busch wrote:
> On Mon, Feb 08, 2021 at 03:10:10PM +0100, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > This adds support for the telemetry log pages and fixes up the
> > controller IEEE OUI.
> 
> Patch 1 is fine.
> 
> I don't see the point for patch 2. We don't need an empty implementation
> for every optional spec feature just because it's there. The features
> we do implement ought to provide something useful, yeah?

Alright, point taken :)

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

Re: [PATCH 1/2] hw/block/nvme: use locally assigned QEMU IEEE OUI

[Philippe Mathieu-Daudé]

On 2/8/21 3:10 PM, Klaus Jensen wrote:
> From: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> 
> Commit 6eb7a071292a ("hw/block/nvme: change controller pci id") changed
> the controller to use a Red Hat assigned PCI Device and Vendor ID, but
> did not change the IEEE OUI away from the Intel IEEE OUI.
> 
> Fix that and use the locally assigned QEMU IEEE OUI instead.
> 
> Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index c2f0c88fbf39..547a3073ef1b 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -4686,8 +4686,8 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
>  
>      id->rab = 6;
>      id->ieee[0] = 0x00;
> -    id->ieee[1] = 0x02;
> -    id->ieee[2] = 0xb3;
> +    id->ieee[1] = 0x54;
> +    id->ieee[2] = 0x52;

Shouldn't this be conditional on 'use-intel-id'?

>      id->mdts = n->params.mdts;
>      id->ver = cpu_to_le32(NVME_SPEC_VER);
>      id->oacs = cpu_to_le16(0);
> 

Re: [PATCH 1/2] hw/block/nvme: use locally assigned QEMU IEEE OUI

[Klaus Jensen]

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

On Feb  8 19:56, Philippe Mathieu-Daudé wrote:
> On 2/8/21 3:10 PM, Klaus Jensen wrote:
> > From: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > 
> > Commit 6eb7a071292a ("hw/block/nvme: change controller pci id") changed
> > the controller to use a Red Hat assigned PCI Device and Vendor ID, but
> > did not change the IEEE OUI away from the Intel IEEE OUI.
> > 
> > Fix that and use the locally assigned QEMU IEEE OUI instead.
> > 
> > Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/block/nvme.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index c2f0c88fbf39..547a3073ef1b 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -4686,8 +4686,8 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
> >  
> >      id->rab = 6;
> >      id->ieee[0] = 0x00;
> > -    id->ieee[1] = 0x02;
> > -    id->ieee[2] = 0xb3;
> > +    id->ieee[1] = 0x54;
> > +    id->ieee[2] = 0x52;
> 
> Shouldn't this be conditional on 'use-intel-id'?
> 

It definitely should! Thanks!

> >      id->mdts = n->params.mdts;
> >      id->ver = cpu_to_le32(NVME_SPEC_VER);
> >      id->oacs = cpu_to_le16(0);
> > 
> 

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