Controllers have different capabilities and report them in the
CAP register. We are particularly interested by the page size
limits.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c       | 10 ++++++++++
 block/trace-events |  1 +
 2 files changed, 11 insertions(+)

diff --git a/block/nvme.c b/block/nvme.c
index 5abd7257cac..3b6d3972ec2 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -720,6 +720,16 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
      * Initialization". */
 
     cap = le64_to_cpu(regs->cap);
+    trace_nvme_controller_capability("Maximum Queue Entries Supported",
+                                     NVME_CAP_MQES(cap));
+    trace_nvme_controller_capability("Contiguous Queues Required",
+                                     NVME_CAP_CQR(cap));
+    trace_nvme_controller_capability("Subsystem  Reset Supported",
+                                     NVME_CAP_NSSRS(cap));
+    trace_nvme_controller_capability("Memory Page Size Minimum",
+                                     NVME_CAP_MPSMIN(cap));
+    trace_nvme_controller_capability("Memory Page Size Maximum",
+                                     NVME_CAP_MPSMAX(cap));
     if (!NVME_CAP_CSS(cap)) {
         error_setg(errp, "Device doesn't support NVMe command set");
         ret = -EINVAL;
diff --git a/block/trace-events b/block/trace-events
index 0e351c3fa3d..3f141dc6801 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -134,6 +134,7 @@ qed_aio_write_postfill(void *s, void *acb, uint64_t start, size_t len, uint64_t
 qed_aio_write_main(void *s, void *acb, int ret, uint64_t offset, size_t len) "s %p acb %p ret %d offset %"PRIu64" len %zu"
 
 # nvme.c
+nvme_controller_capability(const char *desc, uint64_t value) "%s: %"PRIu64
 nvme_kick(void *s, int queue) "s %p queue %d"
 nvme_dma_flush_queue_wait(void *s) "s %p"
 nvme_error(int cmd_specific, int sq_head, int sqid, int cid, int status) "cmd_specific %d sq_head %d sqid %d cid %d status 0x%x"
-- 
2.26.2

Hi,

On 10/26/20 11:54 AM, Philippe Mathieu-Daudé wrote:
> Controllers have different capabilities and report them in the
> CAP register. We are particularly interested by the page size
> limits.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c       | 10 ++++++++++
>  block/trace-events |  1 +
>  2 files changed, 11 insertions(+)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 5abd7257cac..3b6d3972ec2 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -720,6 +720,16 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>       * Initialization". */
>  
>      cap = le64_to_cpu(regs->cap);
> +    trace_nvme_controller_capability("Maximum Queue Entries Supported",
> +                                     NVME_CAP_MQES(cap));
> +    trace_nvme_controller_capability("Contiguous Queues Required",
> +                                     NVME_CAP_CQR(cap));
> +    trace_nvme_controller_capability("Subsystem  Reset Supported",
nit extra space
> +                                     NVME_CAP_NSSRS(cap));
> +    trace_nvme_controller_capability("Memory Page Size Minimum",
> +                                     NVME_CAP_MPSMIN(cap));
> +    trace_nvme_controller_capability("Memory Page Size Maximum",
> +                                     NVME_CAP_MPSMAX(cap));
Don't you want to trace the true MPSMIN/MAX (I mean with the shift)?

I would personally group those traces into a single one and avoid this
generic trace format but well ...

Thanks

Eric
>      if (!NVME_CAP_CSS(cap)) {
>          error_setg(errp, "Device doesn't support NVMe command set");
>          ret = -EINVAL;
> diff --git a/block/trace-events b/block/trace-events
> index 0e351c3fa3d..3f141dc6801 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -134,6 +134,7 @@ qed_aio_write_postfill(void *s, void *acb, uint64_t start, size_t len, uint64_t
>  qed_aio_write_main(void *s, void *acb, int ret, uint64_t offset, size_t len) "s %p acb %p ret %d offset %"PRIu64" len %zu"
>  
>  # nvme.c
> +nvme_controller_capability(const char *desc, uint64_t value) "%s: %"PRIu64
>  nvme_kick(void *s, int queue) "s %p queue %d"
>  nvme_dma_flush_queue_wait(void *s) "s %p"
>  nvme_error(int cmd_specific, int sq_head, int sqid, int cid, int status) "cmd_specific %d sq_head %d sqid %d cid %d status 0x%x"
> 

On Mon, Oct 26, 2020 at 11:54:49AM +0100, Philippe Mathieu-Daudé wrote:
> Controllers have different capabilities and report them in the
> CAP register. We are particularly interested by the page size
> limits.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c       | 10 ++++++++++
>  block/trace-events |  1 +
>  2 files changed, 11 insertions(+)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 5abd7257cac..3b6d3972ec2 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -720,6 +720,16 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>       * Initialization". */
>  
>      cap = le64_to_cpu(regs->cap);
> +    trace_nvme_controller_capability("Maximum Queue Entries Supported",
> +                                     NVME_CAP_MQES(cap));
> +    trace_nvme_controller_capability("Contiguous Queues Required",
> +                                     NVME_CAP_CQR(cap));
> +    trace_nvme_controller_capability("Subsystem  Reset Supported",
> +                                     NVME_CAP_NSSRS(cap));
> +    trace_nvme_controller_capability("Memory Page Size Minimum",
> +                                     NVME_CAP_MPSMIN(cap));
> +    trace_nvme_controller_capability("Memory Page Size Maximum",
> +                                     NVME_CAP_MPSMAX(cap));

This works well for printf-style tracing but it can be tedious to
strcmp() or parse strings from SystemTap and other tracing scripts.

Another way of expressing these trace events is:

  trace_nvme_controller_capabilities(NVME_CAP_MQES(cap),
                                     NVME_CAP_CQR(cap),
				     ...);

Or:

  trace_nvme_controller_capability_mqes(NVME_CAP_MQES(cap));
  trace_nvme_controller_capability_cqr(NVME_CAP_CQR(cap));

One view is that tracing is like printf (the style used in this patch).
Another view is that tracing produces records that scripts can process
(the style I showed). Trace events defined in one style may be hard to
use in the other style (i.e. less human-readable vs harder to process in
a script).

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>