[Qemu-devel] [PATCH v2] nvme: add Get/Set Feature Timestamp support

[Qemu-devel] [PATCH v2] nvme: add Get/Set Feature Timestamp support

[Kenneth Heitke]

Signed-off-by: Kenneth Heitke <kenneth.heitke@intel.com>
---
 hw/block/nvme.c       | 106 +++++++++++++++++++++++++++++++++++++++++-
 hw/block/nvme.h       |   3 ++
 hw/block/trace-events |   2 +
 include/block/nvme.h  |   2 +
 4 files changed, 111 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 7caf92532a..67372e0cd1 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -219,6 +219,30 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t prp1,
     return NVME_INVALID_FIELD | NVME_DNR;
 }
 
+static uint16_t nvme_dma_write_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
+                                   uint64_t prp1, uint64_t prp2)
+{
+    QEMUSGList qsg;
+    QEMUIOVector iov;
+    uint16_t status = NVME_SUCCESS;
+
+    if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) {
+        return NVME_INVALID_FIELD | NVME_DNR;
+    }
+    if (qsg.nsg > 0) {
+        if (dma_buf_write(ptr, len, &qsg)) {
+            status = NVME_INVALID_FIELD | NVME_DNR;
+        }
+        qemu_sglist_destroy(&qsg);
+    } else {
+        if (qemu_iovec_to_buf(&iov, 0, ptr, len) != len) {
+            status = NVME_INVALID_FIELD | NVME_DNR;
+        }
+        qemu_iovec_destroy(&iov);
+    }
+    return status;
+}
+
 static uint16_t nvme_dma_read_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
     uint64_t prp1, uint64_t prp2)
 {
@@ -678,7 +702,6 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
     return ret;
 }
 
-
 static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
 {
     NvmeIdentify *c = (NvmeIdentify *)cmd;
@@ -696,6 +719,57 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
     }
 }
 
+static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
+{
+    trace_nvme_setfeat_timestamp(ts);
+
+    n->host_timestamp = le64_to_cpu(ts);
+    n->timestamp_set_qemu_clock_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+}
+
+static inline uint64_t nvme_get_timestamp(const NvmeCtrl *n)
+{
+    uint64_t current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+    uint64_t elapsed_time = current_time - n->timestamp_set_qemu_clock_ms;
+
+    union nvme_timestamp {
+        struct {
+            uint64_t timestamp:48;
+            uint64_t sync:1;
+            uint64_t origin:3;
+            uint64_t rsvd1:12;
+        };
+        uint64_t all;
+    };
+
+    union nvme_timestamp ts;
+    ts.all = 0;
+
+    /*
+     * If the sum of the Timestamp value set by the host and the elapsed
+     * time exceeds 2^48, the value returned should be reduced modulo 2^48.
+     */
+    ts.timestamp = (n->host_timestamp + elapsed_time) & 0xffffffffffff;
+
+    /* If the host timestamp is non-zero, set the timestamp origin */
+    ts.origin = n->host_timestamp ? 0x01 : 0x00;
+
+    trace_nvme_getfeat_timestamp(ts.all);
+
+    return cpu_to_le64(ts.all);
+}
+
+static uint16_t nvme_get_feature_timestamp(NvmeCtrl *n, NvmeCmd *cmd)
+{
+    uint64_t prp1 = le64_to_cpu(cmd->prp1);
+    uint64_t prp2 = le64_to_cpu(cmd->prp2);
+
+    uint64_t timestamp = nvme_get_timestamp(n);
+
+    return nvme_dma_read_prp(n, (uint8_t *)&timestamp,
+                                 sizeof(timestamp), prp1, prp2);
+}
+
 static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
 {
     uint32_t dw10 = le32_to_cpu(cmd->cdw10);
@@ -710,6 +784,9 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
         result = cpu_to_le32((n->num_queues - 2) | ((n->num_queues - 2) << 16));
         trace_nvme_getfeat_numq(result);
         break;
+    case NVME_TIMESTAMP:
+        return nvme_get_feature_timestamp(n, cmd);
+        break;
     default:
         trace_nvme_err_invalid_getfeat(dw10);
         return NVME_INVALID_FIELD | NVME_DNR;
@@ -719,6 +796,24 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
     return NVME_SUCCESS;
 }
 
+static uint16_t nvme_set_feature_timestamp(NvmeCtrl *n, NvmeCmd *cmd)
+{
+    uint16_t ret;
+    uint64_t timestamp;
+    uint64_t prp1 = le64_to_cpu(cmd->prp1);
+    uint64_t prp2 = le64_to_cpu(cmd->prp2);
+
+    ret = nvme_dma_write_prp(n, (uint8_t *)&timestamp,
+                                sizeof(timestamp), prp1, prp2);
+    if (ret != NVME_SUCCESS) {
+        return ret;
+    }
+
+    nvme_set_timestamp(n, timestamp);
+
+    return NVME_SUCCESS;
+}
+
 static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
 {
     uint32_t dw10 = le32_to_cpu(cmd->cdw10);
@@ -735,6 +830,11 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
         req->cqe.result =
             cpu_to_le32((n->num_queues - 2) | ((n->num_queues - 2) << 16));
         break;
+
+    case NVME_TIMESTAMP:
+        return nvme_set_feature_timestamp(n, cmd);
+        break;
+
     default:
         trace_nvme_err_invalid_setfeat(dw10);
         return NVME_INVALID_FIELD | NVME_DNR;
@@ -907,6 +1007,8 @@ static int nvme_start_ctrl(NvmeCtrl *n)
     nvme_init_sq(&n->admin_sq, n, n->bar.asq, 0, 0,
         NVME_AQA_ASQS(n->bar.aqa) + 1);
 
+    nvme_set_timestamp(n, 0ULL);
+
     return 0;
 }
 
@@ -1270,7 +1372,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
     id->sqes = (0x6 << 4) | 0x6;
     id->cqes = (0x4 << 4) | 0x4;
     id->nn = cpu_to_le32(n->num_namespaces);
-    id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS);
+    id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS | NVME_ONCS_TIMESTAMP);
     id->psd[0].mp = cpu_to_le16(0x9c4);
     id->psd[0].enlat = cpu_to_le32(0x10);
     id->psd[0].exlat = cpu_to_le32(0x4);
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 56c9d4b4b1..d7277e72b7 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -69,6 +69,7 @@ typedef struct NvmeCtrl {
     uint16_t    max_prp_ents;
     uint16_t    cqe_size;
     uint16_t    sqe_size;
+    uint16_t    oncs;
     uint32_t    reg_size;
     uint32_t    num_namespaces;
     uint32_t    num_queues;
@@ -79,6 +80,8 @@ typedef struct NvmeCtrl {
     uint32_t    cmbloc;
     uint8_t     *cmbuf;
     uint64_t    irq_status;
+    uint64_t    host_timestamp;                 /* Timestamp sent by the host */
+    uint64_t    timestamp_set_qemu_clock_ms;    /* QEMU clock time */
 
     char            *serial;
     NvmeNamespace   *namespaces;
diff --git a/hw/block/trace-events b/hw/block/trace-events
index b92039a573..97a17838ed 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -46,6 +46,8 @@ nvme_identify_nslist(uint16_t ns) "identify namespace list, nsid=%"PRIu16""
 nvme_getfeat_vwcache(const char* result) "get feature volatile write cache, result=%s"
 nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
 nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
+nvme_setfeat_timestamp(uint64_t ts) "set feature timestamp = 0x%"PRIx64""
+nvme_getfeat_timestamp(uint64_t ts) "get feature timestamp = 0x%"PRIx64""
 nvme_mmio_intm_set(uint64_t data, uint64_t new_mask) "wrote MMIO, interrupt mask set, data=0x%"PRIx64", new_mask=0x%"PRIx64""
 nvme_mmio_intm_clr(uint64_t data, uint64_t new_mask) "wrote MMIO, interrupt mask clr, data=0x%"PRIx64", new_mask=0x%"PRIx64""
 nvme_mmio_cfg(uint64_t data) "wrote MMIO, config controller config=0x%"PRIx64""
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 849a6f3fa3..3ec8efcc43 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -581,6 +581,7 @@ enum NvmeIdCtrlOncs {
     NVME_ONCS_WRITE_ZEROS   = 1 << 3,
     NVME_ONCS_FEATURES      = 1 << 4,
     NVME_ONCS_RESRVATIONS   = 1 << 5,
+    NVME_ONCS_TIMESTAMP     = 1 << 6,
 };
 
 #define NVME_CTRL_SQES_MIN(sqes) ((sqes) & 0xf)
@@ -622,6 +623,7 @@ enum NvmeFeatureIds {
     NVME_INTERRUPT_VECTOR_CONF      = 0x9,
     NVME_WRITE_ATOMICITY            = 0xa,
     NVME_ASYNCHRONOUS_EVENT_CONF    = 0xb,
+    NVME_TIMESTAMP                  = 0xe,
     NVME_SOFTWARE_PROGRESS_MARKER   = 0x80
 };
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2] nvme: add Get/Set Feature Timestamp support

[Klaus Birkelund]

On Mon, May 20, 2019 at 11:40:30AM -0600, Kenneth Heitke wrote:
> Signed-off-by: Kenneth Heitke <kenneth.heitke@intel.com>
> ---
>  hw/block/nvme.c       | 106 +++++++++++++++++++++++++++++++++++++++++-
>  hw/block/nvme.h       |   3 ++
>  hw/block/trace-events |   2 +
>  include/block/nvme.h  |   2 +
>  4 files changed, 111 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 7caf92532a..67372e0cd1 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -219,6 +219,30 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t prp1,
>      return NVME_INVALID_FIELD | NVME_DNR;
>  }
>  
> +static uint16_t nvme_dma_write_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
> +                                   uint64_t prp1, uint64_t prp2)
> +{
> +    QEMUSGList qsg;
> +    QEMUIOVector iov;
> +    uint16_t status = NVME_SUCCESS;
> +
> +    if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) {
> +        return NVME_INVALID_FIELD | NVME_DNR;
> +    }
> +    if (qsg.nsg > 0) {
> +        if (dma_buf_write(ptr, len, &qsg)) {
> +            status = NVME_INVALID_FIELD | NVME_DNR;
> +        }
> +        qemu_sglist_destroy(&qsg);
> +    } else {
> +        if (qemu_iovec_to_buf(&iov, 0, ptr, len) != len) {
> +            status = NVME_INVALID_FIELD | NVME_DNR;
> +        }
> +        qemu_iovec_destroy(&iov);
> +    }
> +    return status;
> +}
> +
>  static uint16_t nvme_dma_read_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
>      uint64_t prp1, uint64_t prp2)
>  {
> @@ -678,7 +702,6 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
>      return ret;
>  }
>  
> -
>  static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
>  {
>      NvmeIdentify *c = (NvmeIdentify *)cmd;
> @@ -696,6 +719,57 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
>      }
>  }
>  
> +static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
> +{
> +    trace_nvme_setfeat_timestamp(ts);
> +
> +    n->host_timestamp = le64_to_cpu(ts);
> +    n->timestamp_set_qemu_clock_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +}
> +
> +static inline uint64_t nvme_get_timestamp(const NvmeCtrl *n)
> +{
> +    uint64_t current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    uint64_t elapsed_time = current_time - n->timestamp_set_qemu_clock_ms;
> +
> +    union nvme_timestamp {
> +        struct {
> +            uint64_t timestamp:48;
> +            uint64_t sync:1;
> +            uint64_t origin:3;
> +            uint64_t rsvd1:12;
> +        };
> +        uint64_t all;
> +    };
> +
> +    union nvme_timestamp ts;
> +    ts.all = 0;
> +
> +    /*
> +     * If the sum of the Timestamp value set by the host and the elapsed
> +     * time exceeds 2^48, the value returned should be reduced modulo 2^48.
> +     */
> +    ts.timestamp = (n->host_timestamp + elapsed_time) & 0xffffffffffff;
> +
> +    /* If the host timestamp is non-zero, set the timestamp origin */
> +    ts.origin = n->host_timestamp ? 0x01 : 0x00;
> +
> +    trace_nvme_getfeat_timestamp(ts.all);
> +
> +    return cpu_to_le64(ts.all);
> +}
> +
> +static uint16_t nvme_get_feature_timestamp(NvmeCtrl *n, NvmeCmd *cmd)
> +{
> +    uint64_t prp1 = le64_to_cpu(cmd->prp1);
> +    uint64_t prp2 = le64_to_cpu(cmd->prp2);
> +
> +    uint64_t timestamp = nvme_get_timestamp(n);
> +
> +    return nvme_dma_read_prp(n, (uint8_t *)&timestamp,
> +                                 sizeof(timestamp), prp1, prp2);
> +}
> +
>  static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>  {
>      uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> @@ -710,6 +784,9 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>          result = cpu_to_le32((n->num_queues - 2) | ((n->num_queues - 2) << 16));
>          trace_nvme_getfeat_numq(result);
>          break;
> +    case NVME_TIMESTAMP:
> +        return nvme_get_feature_timestamp(n, cmd);
> +        break;
>      default:
>          trace_nvme_err_invalid_getfeat(dw10);
>          return NVME_INVALID_FIELD | NVME_DNR;
> @@ -719,6 +796,24 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>      return NVME_SUCCESS;
>  }
>  
> +static uint16_t nvme_set_feature_timestamp(NvmeCtrl *n, NvmeCmd *cmd)
> +{
> +    uint16_t ret;
> +    uint64_t timestamp;
> +    uint64_t prp1 = le64_to_cpu(cmd->prp1);
> +    uint64_t prp2 = le64_to_cpu(cmd->prp2);
> +
> +    ret = nvme_dma_write_prp(n, (uint8_t *)&timestamp,
> +                                sizeof(timestamp), prp1, prp2);
> +    if (ret != NVME_SUCCESS) {
> +        return ret;
> +    }
> +
> +    nvme_set_timestamp(n, timestamp);
> +
> +    return NVME_SUCCESS;
> +}
> +
>  static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>  {
>      uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> @@ -735,6 +830,11 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>          req->cqe.result =
>              cpu_to_le32((n->num_queues - 2) | ((n->num_queues - 2) << 16));
>          break;
> +
> +    case NVME_TIMESTAMP:
> +        return nvme_set_feature_timestamp(n, cmd);
> +        break;
> +
>      default:
>          trace_nvme_err_invalid_setfeat(dw10);
>          return NVME_INVALID_FIELD | NVME_DNR;
> @@ -907,6 +1007,8 @@ static int nvme_start_ctrl(NvmeCtrl *n)
>      nvme_init_sq(&n->admin_sq, n, n->bar.asq, 0, 0,
>          NVME_AQA_ASQS(n->bar.aqa) + 1);
>  
> +    nvme_set_timestamp(n, 0ULL);
> +
>      return 0;
>  }
>  
> @@ -1270,7 +1372,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>      id->sqes = (0x6 << 4) | 0x6;
>      id->cqes = (0x4 << 4) | 0x4;
>      id->nn = cpu_to_le32(n->num_namespaces);
> -    id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS);
> +    id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS | NVME_ONCS_TIMESTAMP);
>      id->psd[0].mp = cpu_to_le16(0x9c4);
>      id->psd[0].enlat = cpu_to_le32(0x10);
>      id->psd[0].exlat = cpu_to_le32(0x4);
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 56c9d4b4b1..d7277e72b7 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -69,6 +69,7 @@ typedef struct NvmeCtrl {
>      uint16_t    max_prp_ents;
>      uint16_t    cqe_size;
>      uint16_t    sqe_size;
> +    uint16_t    oncs;

Looks like this unused member snuck its way into the patch. But I see no
harm in it being there.

>      uint32_t    reg_size;
>      uint32_t    num_namespaces;
>      uint32_t    num_queues;
> @@ -79,6 +80,8 @@ typedef struct NvmeCtrl {
>      uint32_t    cmbloc;
>      uint8_t     *cmbuf;
>      uint64_t    irq_status;
> +    uint64_t    host_timestamp;                 /* Timestamp sent by the host */
> +    uint64_t    timestamp_set_qemu_clock_ms;    /* QEMU clock time */
>  
>      char            *serial;
>      NvmeNamespace   *namespaces;
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index b92039a573..97a17838ed 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -46,6 +46,8 @@ nvme_identify_nslist(uint16_t ns) "identify namespace list, nsid=%"PRIu16""
>  nvme_getfeat_vwcache(const char* result) "get feature volatile write cache, result=%s"
>  nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
>  nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
> +nvme_setfeat_timestamp(uint64_t ts) "set feature timestamp = 0x%"PRIx64""
> +nvme_getfeat_timestamp(uint64_t ts) "get feature timestamp = 0x%"PRIx64""
>  nvme_mmio_intm_set(uint64_t data, uint64_t new_mask) "wrote MMIO, interrupt mask set, data=0x%"PRIx64", new_mask=0x%"PRIx64""
>  nvme_mmio_intm_clr(uint64_t data, uint64_t new_mask) "wrote MMIO, interrupt mask clr, data=0x%"PRIx64", new_mask=0x%"PRIx64""
>  nvme_mmio_cfg(uint64_t data) "wrote MMIO, config controller config=0x%"PRIx64""
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 849a6f3fa3..3ec8efcc43 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -581,6 +581,7 @@ enum NvmeIdCtrlOncs {
>      NVME_ONCS_WRITE_ZEROS   = 1 << 3,
>      NVME_ONCS_FEATURES      = 1 << 4,
>      NVME_ONCS_RESRVATIONS   = 1 << 5,
> +    NVME_ONCS_TIMESTAMP     = 1 << 6,
>  };
>  
>  #define NVME_CTRL_SQES_MIN(sqes) ((sqes) & 0xf)
> @@ -622,6 +623,7 @@ enum NvmeFeatureIds {
>      NVME_INTERRUPT_VECTOR_CONF      = 0x9,
>      NVME_WRITE_ATOMICITY            = 0xa,
>      NVME_ASYNCHRONOUS_EVENT_CONF    = 0xb,
> +    NVME_TIMESTAMP                  = 0xe,
>      NVME_SOFTWARE_PROGRESS_MARKER   = 0x80
>  };
>  
> -- 
> 2.17.1
> 

Reviewed-by: Klaus Birkelund Jensen <klaus.jensen@cnexlabs.com>

Re: [Qemu-devel] [PATCH v2] nvme: add Get/Set Feature Timestamp support

[Kevin Wolf]

Am 20.05.2019 um 19:40 hat Kenneth Heitke geschrieben:
> Signed-off-by: Kenneth Heitke <kenneth.heitke@intel.com>

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH v2] nvme: add Get/Set Feature Timestamp support

[Kevin Wolf]

Am 28.05.2019 um 08:18 hat Klaus Birkelund geschrieben:
> On Mon, May 20, 2019 at 11:40:30AM -0600, Kenneth Heitke wrote:
> > Signed-off-by: Kenneth Heitke <kenneth.heitke@intel.com>

> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index 56c9d4b4b1..d7277e72b7 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -69,6 +69,7 @@ typedef struct NvmeCtrl {
> >      uint16_t    max_prp_ents;
> >      uint16_t    cqe_size;
> >      uint16_t    sqe_size;
> > +    uint16_t    oncs;
> 
> Looks like this unused member snuck its way into the patch. But I see no
> harm in it being there.

Good catch. I'll just remove it again from my branch.

> > +static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
> > +{
> > +    trace_nvme_setfeat_timestamp(ts);
> > +
> > +    n->host_timestamp = le64_to_cpu(ts);
> > +    n->timestamp_set_qemu_clock_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> > +}
> > +
> > +static inline uint64_t nvme_get_timestamp(const NvmeCtrl *n)
> > +{
> > +    uint64_t current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);

Here I wonder why we use QEMU_CLOCK_REALTIME in a device emulation.
Wouldn't QEMU_CLOCK_VIRTUAL make more sense?

Kevin

Re: [Qemu-devel] [PATCH v2] nvme: add Get/Set Feature Timestamp support

[Heitke, Kenneth]

On 6/3/2019 5:14 AM, Kevin Wolf wrote:
> Am 28.05.2019 um 08:18 hat Klaus Birkelund geschrieben:
>> On Mon, May 20, 2019 at 11:40:30AM -0600, Kenneth Heitke wrote:
>>> Signed-off-by: Kenneth Heitke <kenneth.heitke@intel.com>
> 
>>> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
>>> index 56c9d4b4b1..d7277e72b7 100644
>>> --- a/hw/block/nvme.h
>>> +++ b/hw/block/nvme.h
>>> @@ -69,6 +69,7 @@ typedef struct NvmeCtrl {
>>>       uint16_t    max_prp_ents;
>>>       uint16_t    cqe_size;
>>>       uint16_t    sqe_size;
>>> +    uint16_t    oncs;
>>
>> Looks like this unused member snuck its way into the patch. But I see no
>> harm in it being there.
> 
> Good catch. I'll just remove it again from my branch.
> 
>>> +static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
>>> +{
>>> +    trace_nvme_setfeat_timestamp(ts);
>>> +
>>> +    n->host_timestamp = le64_to_cpu(ts);
>>> +    n->timestamp_set_qemu_clock_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>>> +}
>>> +
>>> +static inline uint64_t nvme_get_timestamp(const NvmeCtrl *n)
>>> +{
>>> +    uint64_t current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> 
> Here I wonder why we use QEMU_CLOCK_REALTIME in a device emulation.
> Wouldn't QEMU_CLOCK_VIRTUAL make more sense?
> 

QEMU_CLOCK_VIRTUAL probably would make more sense. When I was reading 
through the differences I wasn't really sure what to pick. iven that 
this is the time within the device's context, the virtual time seems 
more correct.

> Kevin
> 

Re: [Qemu-devel] [Qemu-block] [PATCH v2] nvme: add Get/Set Feature Timestamp support

[Klaus Birkelund]

On Mon, Jun 03, 2019 at 09:30:53AM -0600, Heitke, Kenneth wrote:
> 
> 
> On 6/3/2019 5:14 AM, Kevin Wolf wrote:
> > Am 28.05.2019 um 08:18 hat Klaus Birkelund geschrieben:
> > > On Mon, May 20, 2019 at 11:40:30AM -0600, Kenneth Heitke wrote:
> > > > Signed-off-by: Kenneth Heitke <kenneth.heitke@intel.com>
> > 
> > > > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > > > index 56c9d4b4b1..d7277e72b7 100644
> > > > --- a/hw/block/nvme.h
> > > > +++ b/hw/block/nvme.h
> > > > @@ -69,6 +69,7 @@ typedef struct NvmeCtrl {
> > > >       uint16_t    max_prp_ents;
> > > >       uint16_t    cqe_size;
> > > >       uint16_t    sqe_size;
> > > > +    uint16_t    oncs;
> > > 
> > > Looks like this unused member snuck its way into the patch. But I see no
> > > harm in it being there.
> > 
> > Good catch. I'll just remove it again from my branch.
> > 
> > > > +static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
> > > > +{
> > > > +    trace_nvme_setfeat_timestamp(ts);
> > > > +
> > > > +    n->host_timestamp = le64_to_cpu(ts);
> > > > +    n->timestamp_set_qemu_clock_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> > > > +}
> > > > +
> > > > +static inline uint64_t nvme_get_timestamp(const NvmeCtrl *n)
> > > > +{
> > > > +    uint64_t current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> > 
> > Here I wonder why we use QEMU_CLOCK_REALTIME in a device emulation.
> > Wouldn't QEMU_CLOCK_VIRTUAL make more sense?
> > 
> 
> QEMU_CLOCK_VIRTUAL probably would make more sense. When I was reading
> through the differences I wasn't really sure what to pick. iven that this is
> the time within the device's context, the virtual time seems more correct.
> 
 
I thought about this too when I reviewed, but came to the conclusion
that REALTIME was correct. The timestamp is basically a value that the
host stores in the controller. When the host uses Get Features to get
the the current time it would expect it to match the progression for its
own wall clockright? If I understand REALTIME vs VIRTUAL correctly,
using VIRTUAL, it would go way out of sync.

Klaus

Re: [Qemu-devel] [Qemu-block] [PATCH v2] nvme: add Get/Set Feature Timestamp support

[Kevin Wolf]

Am 04.06.2019 um 10:28 hat Klaus Birkelund geschrieben:
> On Mon, Jun 03, 2019 at 09:30:53AM -0600, Heitke, Kenneth wrote:
> > 
> > 
> > On 6/3/2019 5:14 AM, Kevin Wolf wrote:
> > > Am 28.05.2019 um 08:18 hat Klaus Birkelund geschrieben:
> > > > On Mon, May 20, 2019 at 11:40:30AM -0600, Kenneth Heitke wrote:
> > > > > Signed-off-by: Kenneth Heitke <kenneth.heitke@intel.com>
> > > 
> > > > > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > > > > index 56c9d4b4b1..d7277e72b7 100644
> > > > > --- a/hw/block/nvme.h
> > > > > +++ b/hw/block/nvme.h
> > > > > @@ -69,6 +69,7 @@ typedef struct NvmeCtrl {
> > > > >       uint16_t    max_prp_ents;
> > > > >       uint16_t    cqe_size;
> > > > >       uint16_t    sqe_size;
> > > > > +    uint16_t    oncs;
> > > > 
> > > > Looks like this unused member snuck its way into the patch. But I see no
> > > > harm in it being there.
> > > 
> > > Good catch. I'll just remove it again from my branch.
> > > 
> > > > > +static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
> > > > > +{
> > > > > +    trace_nvme_setfeat_timestamp(ts);
> > > > > +
> > > > > +    n->host_timestamp = le64_to_cpu(ts);
> > > > > +    n->timestamp_set_qemu_clock_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> > > > > +}
> > > > > +
> > > > > +static inline uint64_t nvme_get_timestamp(const NvmeCtrl *n)
> > > > > +{
> > > > > +    uint64_t current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> > > 
> > > Here I wonder why we use QEMU_CLOCK_REALTIME in a device emulation.
> > > Wouldn't QEMU_CLOCK_VIRTUAL make more sense?
> > > 
> > 
> > QEMU_CLOCK_VIRTUAL probably would make more sense. When I was reading
> > through the differences I wasn't really sure what to pick. iven that this is
> > the time within the device's context, the virtual time seems more correct.
> > 
>  
> I thought about this too when I reviewed, but came to the conclusion
> that REALTIME was correct. The timestamp is basically a value that the
> host stores in the controller. When the host uses Get Features to get
> the the current time it would expect it to match the progression for its
> own wall clockright? If I understand REALTIME vs VIRTUAL correctly,
> using VIRTUAL, it would go way out of sync.

Which two things would go out of sync with VIRTUAL?

Not an expert on clocks myself, but I think the main question is what
happens to the clock while the VM is stopped. REALTIME continues running
where as VIRTUAL is stopped. If we expose REALTIME measurements to the
guest, the time passed may look a lot longer than what the guest's clock
actually says. So this is the thing I am worried would go out of sync
with REALTIME.

Or did I read the code wrong and this isn't actually exposed to the
guest?

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v2] nvme: add Get/Set Feature Timestamp support

[Klaus Birkelund]

On Tue, Jun 04, 2019 at 10:46:45AM +0200, Kevin Wolf wrote:
> Am 04.06.2019 um 10:28 hat Klaus Birkelund geschrieben:
> > On Mon, Jun 03, 2019 at 09:30:53AM -0600, Heitke, Kenneth wrote:
> > > 
> > > 
> > > On 6/3/2019 5:14 AM, Kevin Wolf wrote:
> > > > Am 28.05.2019 um 08:18 hat Klaus Birkelund geschrieben:
> > > > > On Mon, May 20, 2019 at 11:40:30AM -0600, Kenneth Heitke wrote:
> > > > > > Signed-off-by: Kenneth Heitke <kenneth.heitke@intel.com>
> > > > 
> > > > > > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > > > > > index 56c9d4b4b1..d7277e72b7 100644
> > > > > > --- a/hw/block/nvme.h
> > > > > > +++ b/hw/block/nvme.h
> > > > > > @@ -69,6 +69,7 @@ typedef struct NvmeCtrl {
> > > > > >       uint16_t    max_prp_ents;
> > > > > >       uint16_t    cqe_size;
> > > > > >       uint16_t    sqe_size;
> > > > > > +    uint16_t    oncs;
> > > > > 
> > > > > Looks like this unused member snuck its way into the patch. But I see no
> > > > > harm in it being there.
> > > > 
> > > > Good catch. I'll just remove it again from my branch.
> > > > 
> > > > > > +static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
> > > > > > +{
> > > > > > +    trace_nvme_setfeat_timestamp(ts);
> > > > > > +
> > > > > > +    n->host_timestamp = le64_to_cpu(ts);
> > > > > > +    n->timestamp_set_qemu_clock_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> > > > > > +}
> > > > > > +
> > > > > > +static inline uint64_t nvme_get_timestamp(const NvmeCtrl *n)
> > > > > > +{
> > > > > > +    uint64_t current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> > > > 
> > > > Here I wonder why we use QEMU_CLOCK_REALTIME in a device emulation.
> > > > Wouldn't QEMU_CLOCK_VIRTUAL make more sense?
> > > > 
> > > 
> > > QEMU_CLOCK_VIRTUAL probably would make more sense. When I was reading
> > > through the differences I wasn't really sure what to pick. iven that this is
> > > the time within the device's context, the virtual time seems more correct.
> > > 
> >  
> > I thought about this too when I reviewed, but came to the conclusion
> > that REALTIME was correct. The timestamp is basically a value that the
> > host stores in the controller. When the host uses Get Features to get
> > the the current time it would expect it to match the progression for its
> > own wall clockright? If I understand REALTIME vs VIRTUAL correctly,
> > using VIRTUAL, it would go way out of sync.
> 
> Which two things would go out of sync with VIRTUAL?
> 
> Not an expert on clocks myself, but I think the main question is what
> happens to the clock while the VM is stopped. REALTIME continues running
> where as VIRTUAL is stopped. If we expose REALTIME measurements to the
> guest, the time passed may look a lot longer than what the guest's clock
> actually says. So this is the thing I am worried would go out of sync
> with REALTIME.
> 

OK, fair point.

Thinking about this some more, I agree that VIRTUAL is more correct. An
application should never track elapsed time using real wall clock time,
but some monotonic clock that is oblivious to say NTP adjustments.

Klaus

Re: [Qemu-devel] [Qemu-block] [PATCH v2] nvme: add Get/Set Feature Timestamp support

[Kevin Wolf]

Am 04.06.2019 um 11:13 hat Klaus Birkelund geschrieben:
> On Tue, Jun 04, 2019 at 10:46:45AM +0200, Kevin Wolf wrote:
> > Am 04.06.2019 um 10:28 hat Klaus Birkelund geschrieben:
> > > On Mon, Jun 03, 2019 at 09:30:53AM -0600, Heitke, Kenneth wrote:
> > > > 
> > > > 
> > > > On 6/3/2019 5:14 AM, Kevin Wolf wrote:
> > > > > Am 28.05.2019 um 08:18 hat Klaus Birkelund geschrieben:
> > > > > > On Mon, May 20, 2019 at 11:40:30AM -0600, Kenneth Heitke wrote:
> > > > > > > Signed-off-by: Kenneth Heitke <kenneth.heitke@intel.com>
> > > > > 
> > > > > > > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > > > > > > index 56c9d4b4b1..d7277e72b7 100644
> > > > > > > --- a/hw/block/nvme.h
> > > > > > > +++ b/hw/block/nvme.h
> > > > > > > @@ -69,6 +69,7 @@ typedef struct NvmeCtrl {
> > > > > > >       uint16_t    max_prp_ents;
> > > > > > >       uint16_t    cqe_size;
> > > > > > >       uint16_t    sqe_size;
> > > > > > > +    uint16_t    oncs;
> > > > > > 
> > > > > > Looks like this unused member snuck its way into the patch. But I see no
> > > > > > harm in it being there.
> > > > > 
> > > > > Good catch. I'll just remove it again from my branch.
> > > > > 
> > > > > > > +static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
> > > > > > > +{
> > > > > > > +    trace_nvme_setfeat_timestamp(ts);
> > > > > > > +
> > > > > > > +    n->host_timestamp = le64_to_cpu(ts);
> > > > > > > +    n->timestamp_set_qemu_clock_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static inline uint64_t nvme_get_timestamp(const NvmeCtrl *n)
> > > > > > > +{
> > > > > > > +    uint64_t current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> > > > > 
> > > > > Here I wonder why we use QEMU_CLOCK_REALTIME in a device emulation.
> > > > > Wouldn't QEMU_CLOCK_VIRTUAL make more sense?
> > > > > 
> > > > 
> > > > QEMU_CLOCK_VIRTUAL probably would make more sense. When I was reading
> > > > through the differences I wasn't really sure what to pick. iven that this is
> > > > the time within the device's context, the virtual time seems more correct.
> > > > 
> > >  
> > > I thought about this too when I reviewed, but came to the conclusion
> > > that REALTIME was correct. The timestamp is basically a value that the
> > > host stores in the controller. When the host uses Get Features to get
> > > the the current time it would expect it to match the progression for its
> > > own wall clockright? If I understand REALTIME vs VIRTUAL correctly,
> > > using VIRTUAL, it would go way out of sync.
> > 
> > Which two things would go out of sync with VIRTUAL?
> > 
> > Not an expert on clocks myself, but I think the main question is what
> > happens to the clock while the VM is stopped. REALTIME continues running
> > where as VIRTUAL is stopped. If we expose REALTIME measurements to the
> > guest, the time passed may look a lot longer than what the guest's clock
> > actually says. So this is the thing I am worried would go out of sync
> > with REALTIME.
> > 
> 
> OK, fair point.
> 
> Thinking about this some more, I agree that VIRTUAL is more correct. An
> application should never track elapsed time using real wall clock time,
> but some monotonic clock that is oblivious to say NTP adjustments.

Thanks. I'll fix up the patch accordingly.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v2] nvme: add Get/Set Feature Timestamp support

[Heitke, Kenneth]

On 6/4/2019 3:13 AM, Klaus Birkelund wrote:
> On Tue, Jun 04, 2019 at 10:46:45AM +0200, Kevin Wolf wrote:
>> Am 04.06.2019 um 10:28 hat Klaus Birkelund geschrieben:
>>> On Mon, Jun 03, 2019 at 09:30:53AM -0600, Heitke, Kenneth wrote:
>>>>
>>>>
>>>> On 6/3/2019 5:14 AM, Kevin Wolf wrote:
>>>>> Am 28.05.2019 um 08:18 hat Klaus Birkelund geschrieben:
>>>>>> On Mon, May 20, 2019 at 11:40:30AM -0600, Kenneth Heitke wrote:
>>>>>>> Signed-off-by: Kenneth Heitke <kenneth.heitke@intel.com>
>>>>>
>>>>>>> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
>>>>>>> index 56c9d4b4b1..d7277e72b7 100644
>>>>>>> --- a/hw/block/nvme.h
>>>>>>> +++ b/hw/block/nvme.h
>>>>>>> @@ -69,6 +69,7 @@ typedef struct NvmeCtrl {
>>>>>>>        uint16_t    max_prp_ents;
>>>>>>>        uint16_t    cqe_size;
>>>>>>>        uint16_t    sqe_size;
>>>>>>> +    uint16_t    oncs;
>>>>>>
>>>>>> Looks like this unused member snuck its way into the patch. But I see no
>>>>>> harm in it being there.
>>>>>
>>>>> Good catch. I'll just remove it again from my branch.
>>>>>
>>>>>>> +static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
>>>>>>> +{
>>>>>>> +    trace_nvme_setfeat_timestamp(ts);
>>>>>>> +
>>>>>>> +    n->host_timestamp = le64_to_cpu(ts);
>>>>>>> +    n->timestamp_set_qemu_clock_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static inline uint64_t nvme_get_timestamp(const NvmeCtrl *n)
>>>>>>> +{
>>>>>>> +    uint64_t current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>>>>>
>>>>> Here I wonder why we use QEMU_CLOCK_REALTIME in a device emulation.
>>>>> Wouldn't QEMU_CLOCK_VIRTUAL make more sense?
>>>>>
>>>>
>>>> QEMU_CLOCK_VIRTUAL probably would make more sense. When I was reading
>>>> through the differences I wasn't really sure what to pick. iven that this is
>>>> the time within the device's context, the virtual time seems more correct.
>>>>
>>>   
>>> I thought about this too when I reviewed, but came to the conclusion
>>> that REALTIME was correct. The timestamp is basically a value that the
>>> host stores in the controller. When the host uses Get Features to get
>>> the the current time it would expect it to match the progression for its
>>> own wall clockright? If I understand REALTIME vs VIRTUAL correctly,
>>> using VIRTUAL, it would go way out of sync.
>>
>> Which two things would go out of sync with VIRTUAL?
>>
>> Not an expert on clocks myself, but I think the main question is what
>> happens to the clock while the VM is stopped. REALTIME continues running
>> where as VIRTUAL is stopped. If we expose REALTIME measurements to the
>> guest, the time passed may look a lot longer than what the guest's clock
>> actually says. So this is the thing I am worried would go out of sync
>> with REALTIME.
>>
> 
> OK, fair point.
> 
> Thinking about this some more, I agree that VIRTUAL is more correct. An
> application should never track elapsed time using real wall clock time,
> but some monotonic clock that is oblivious to say NTP adjustments.
> 
> Klaus
> 

Kevin, would you like me to update the patch to reflect this change or 
will you make the change directly?

thanks,
Ken

Re: [Qemu-devel] [Qemu-block] [PATCH v2] nvme: add Get/Set Feature Timestamp support

[Kevin Wolf]

Am 04.06.2019 um 19:06 hat Heitke, Kenneth geschrieben:
> 
> 
> On 6/4/2019 3:13 AM, Klaus Birkelund wrote:
> > On Tue, Jun 04, 2019 at 10:46:45AM +0200, Kevin Wolf wrote:
> > > Am 04.06.2019 um 10:28 hat Klaus Birkelund geschrieben:
> > > > On Mon, Jun 03, 2019 at 09:30:53AM -0600, Heitke, Kenneth wrote:
> > > > > 
> > > > > 
> > > > > On 6/3/2019 5:14 AM, Kevin Wolf wrote:
> > > > > > Am 28.05.2019 um 08:18 hat Klaus Birkelund geschrieben:
> > > > > > > On Mon, May 20, 2019 at 11:40:30AM -0600, Kenneth Heitke wrote:
> > > > > > > > Signed-off-by: Kenneth Heitke <kenneth.heitke@intel.com>
> > > > > > 
> > > > > > > > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > > > > > > > index 56c9d4b4b1..d7277e72b7 100644
> > > > > > > > --- a/hw/block/nvme.h
> > > > > > > > +++ b/hw/block/nvme.h
> > > > > > > > @@ -69,6 +69,7 @@ typedef struct NvmeCtrl {
> > > > > > > >        uint16_t    max_prp_ents;
> > > > > > > >        uint16_t    cqe_size;
> > > > > > > >        uint16_t    sqe_size;
> > > > > > > > +    uint16_t    oncs;
> > > > > > > 
> > > > > > > Looks like this unused member snuck its way into the patch. But I see no
> > > > > > > harm in it being there.
> > > > > > 
> > > > > > Good catch. I'll just remove it again from my branch.
> > > > > > 
> > > > > > > > +static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
> > > > > > > > +{
> > > > > > > > +    trace_nvme_setfeat_timestamp(ts);
> > > > > > > > +
> > > > > > > > +    n->host_timestamp = le64_to_cpu(ts);
> > > > > > > > +    n->timestamp_set_qemu_clock_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static inline uint64_t nvme_get_timestamp(const NvmeCtrl *n)
> > > > > > > > +{
> > > > > > > > +    uint64_t current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> > > > > > 
> > > > > > Here I wonder why we use QEMU_CLOCK_REALTIME in a device emulation.
> > > > > > Wouldn't QEMU_CLOCK_VIRTUAL make more sense?
> > > > > > 
> > > > > 
> > > > > QEMU_CLOCK_VIRTUAL probably would make more sense. When I was reading
> > > > > through the differences I wasn't really sure what to pick. iven that this is
> > > > > the time within the device's context, the virtual time seems more correct.
> > > > > 
> > > > I thought about this too when I reviewed, but came to the conclusion
> > > > that REALTIME was correct. The timestamp is basically a value that the
> > > > host stores in the controller. When the host uses Get Features to get
> > > > the the current time it would expect it to match the progression for its
> > > > own wall clockright? If I understand REALTIME vs VIRTUAL correctly,
> > > > using VIRTUAL, it would go way out of sync.
> > > 
> > > Which two things would go out of sync with VIRTUAL?
> > > 
> > > Not an expert on clocks myself, but I think the main question is what
> > > happens to the clock while the VM is stopped. REALTIME continues running
> > > where as VIRTUAL is stopped. If we expose REALTIME measurements to the
> > > guest, the time passed may look a lot longer than what the guest's clock
> > > actually says. So this is the thing I am worried would go out of sync
> > > with REALTIME.
> > > 
> > 
> > OK, fair point.
> > 
> > Thinking about this some more, I agree that VIRTUAL is more correct. An
> > application should never track elapsed time using real wall clock time,
> > but some monotonic clock that is oblivious to say NTP adjustments.
> > 
> > Klaus
> > 
> 
> Kevin, would you like me to update the patch to reflect this change or will
> you make the change directly?

I already made it directly.

Kevin