[PATCH 00/25] block/nvme: Fix Aarch64 host

[PATCH 00/25] block/nvme: Fix Aarch64 host

[Philippe Mathieu-Daudé]

Add a bit of tracing, clean around to finally fix few bugs.
In particular, restore NVMe on Aarch64 host.

Eric Auger (4):
  block/nvme: Change size and alignment of IDENTIFY response buffer
  block/nvme: Change size and alignment of queue
  block/nvme: Change size and alignment of prp_list_pages
  block/nvme: Align iov's va and size on host page size

Philippe Mathieu-Daudé (21):
  MAINTAINERS: Cover 'block/nvme.h' file
  block/nvme: Use hex format to display offset in trace events
  block/nvme: Report warning with warn_report()
  block/nvme: Trace controller capabilities
  block/nvme: Trace nvme_poll_queue() per queue
  block/nvme: Improve nvme_free_req_queue_wait() trace information
  block/nvme: Trace queue pair creation/deletion
  block/nvme: Simplify device reset
  block/nvme: Move definitions before structure declarations
  block/nvme: Use unsigned integer for queue counter/size
  block/nvme: Make nvme_identify() return boolean indicating error
  block/nvme: Make nvme_init_queue() return boolean indicating error
  block/nvme: Introduce Completion Queue definitions
  block/nvme: Use definitions instead of magic values in add_io_queue()
  block/nvme: Correctly initialize Admin Queue Attributes
  block/nvme: Simplify ADMIN queue access
  block/nvme: Simplify nvme_cmd_sync()
  block/nvme: Pass AioContext argument to nvme_add_io_queue()
  block/nvme: Set request_alignment at initialization
  block/nvme: Correct minimum device page size
  block/nvme: Fix use of write-only doorbells page on Aarch64 arch

 include/block/nvme.h |  17 ++--
 block/nvme.c         | 208 ++++++++++++++++++++++++-------------------
 MAINTAINERS          |   2 +
 block/trace-events   |  30 ++++---
 4 files changed, 148 insertions(+), 109 deletions(-)

-- 
2.26.2

[PATCH 01/25] MAINTAINERS: Cover 'block/nvme.h' file

[Philippe Mathieu-Daudé]

The "block/nvme.h" header is shared by both the NVMe block
driver and the NVMe emulated device. Add the 'F:' entry on
both sections, so all maintainers/reviewers are notified
when it is changed.

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ef6f5c73998..9366a1b2b3a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1876,6 +1876,7 @@ M: Klaus Jensen <its@irrelevant.dk>
 L: qemu-block@nongnu.org
 S: Supported
 F: hw/block/nvme*
+F: include/block/nvme.h
 F: tests/qtest/nvme-test.c
 T: git git://git.infradead.org/qemu-nvme.git nvme-next
 
@@ -2953,6 +2954,7 @@ R: Fam Zheng <fam@euphon.net>
 L: qemu-block@nongnu.org
 S: Supported
 F: block/nvme*
+F: include/block/nvme.h
 T: git https://github.com/stefanha/qemu.git block
 
 Bootdevice
-- 
2.26.2

Re: [PATCH 01/25] MAINTAINERS: Cover 'block/nvme.h' file

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:23PM +0100, Philippe Mathieu-Daudé wrote:
> The "block/nvme.h" header is shared by both the NVMe block
> driver and the NVMe emulated device. Add the 'F:' entry on
> both sections, so all maintainers/reviewers are notified
> when it is changed.
> 
> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  MAINTAINERS | 2 ++
>  1 file changed, 2 insertions(+)

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

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

[PATCH 02/25] block/nvme: Use hex format to display offset in trace events

[Philippe Mathieu-Daudé]

Use the same format used for the hw/vfio/ trace events.

Suggested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/trace-events | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/trace-events b/block/trace-events
index 0e351c3fa3d..0955c85c783 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -144,13 +144,13 @@ nvme_submit_command(void *s, int index, int cid) "s %p queue %d cid %d"
 nvme_submit_command_raw(int c0, int c1, int c2, int c3, int c4, int c5, int c6, int c7) "%02x %02x %02x %02x %02x %02x %02x %02x"
 nvme_handle_event(void *s) "s %p"
 nvme_poll_cb(void *s) "s %p"
-nvme_prw_aligned(void *s, int is_write, uint64_t offset, uint64_t bytes, int flags, int niov) "s %p is_write %d offset %"PRId64" bytes %"PRId64" flags %d niov %d"
-nvme_write_zeroes(void *s, uint64_t offset, uint64_t bytes, int flags) "s %p offset %"PRId64" bytes %"PRId64" flags %d"
+nvme_prw_aligned(void *s, int is_write, uint64_t offset, uint64_t bytes, int flags, int niov) "s %p is_write %d offset 0x%"PRIx64" bytes %"PRId64" flags %d niov %d"
+nvme_write_zeroes(void *s, uint64_t offset, uint64_t bytes, int flags) "s %p offset 0x%"PRIx64" bytes %"PRId64" flags %d"
 nvme_qiov_unaligned(const void *qiov, int n, void *base, size_t size, int align) "qiov %p n %d base %p size 0x%zx align 0x%x"
-nvme_prw_buffered(void *s, uint64_t offset, uint64_t bytes, int niov, int is_write) "s %p offset %"PRId64" bytes %"PRId64" niov %d is_write %d"
-nvme_rw_done(void *s, int is_write, uint64_t offset, uint64_t bytes, int ret) "s %p is_write %d offset %"PRId64" bytes %"PRId64" ret %d"
-nvme_dsm(void *s, uint64_t offset, uint64_t bytes) "s %p offset %"PRId64" bytes %"PRId64""
-nvme_dsm_done(void *s, uint64_t offset, uint64_t bytes, int ret) "s %p offset %"PRId64" bytes %"PRId64" ret %d"
+nvme_prw_buffered(void *s, uint64_t offset, uint64_t bytes, int niov, int is_write) "s %p offset 0x%"PRIx64" bytes %"PRId64" niov %d is_write %d"
+nvme_rw_done(void *s, int is_write, uint64_t offset, uint64_t bytes, int ret) "s %p is_write %d offset 0x%"PRIx64" bytes %"PRId64" ret %d"
+nvme_dsm(void *s, uint64_t offset, uint64_t bytes) "s %p offset 0x%"PRIx64" bytes %"PRId64""
+nvme_dsm_done(void *s, uint64_t offset, uint64_t bytes, int ret) "s %p offset 0x%"PRIx64" bytes %"PRId64" ret %d"
 nvme_dma_map_flush(void *s) "s %p"
 nvme_free_req_queue_wait(void *q) "q %p"
 nvme_cmd_map_qiov(void *s, void *cmd, void *req, void *qiov, int entries) "s %p cmd %p req %p qiov %p entries %d"
-- 
2.26.2

Re: [PATCH 02/25] block/nvme: Use hex format to display offset in trace events

[Auger Eric]

Hi Philippe,

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> Use the same format used for the hw/vfio/ trace events.
> 
> Suggested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/trace-events | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/block/trace-events b/block/trace-events
> index 0e351c3fa3d..0955c85c783 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -144,13 +144,13 @@ nvme_submit_command(void *s, int index, int cid) "s %p queue %d cid %d"
>  nvme_submit_command_raw(int c0, int c1, int c2, int c3, int c4, int c5, int c6, int c7) "%02x %02x %02x %02x %02x %02x %02x %02x"
>  nvme_handle_event(void *s) "s %p"
>  nvme_poll_cb(void *s) "s %p"
> -nvme_prw_aligned(void *s, int is_write, uint64_t offset, uint64_t bytes, int flags, int niov) "s %p is_write %d offset %"PRId64" bytes %"PRId64" flags %d niov %d"
while we are at it I would change bytes here and below too.

But this can be part of another patch
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> -nvme_write_zeroes(void *s, uint64_t offset, uint64_t bytes, int flags) "s %p offset %"PRId64" bytes %"PRId64" flags %d"
> +nvme_prw_aligned(void *s, int is_write, uint64_t offset, uint64_t bytes, int flags, int niov) "s %p is_write %d offset 0x%"PRIx64" bytes %"PRId64" flags %d niov %d"
> +nvme_write_zeroes(void *s, uint64_t offset, uint64_t bytes, int flags) "s %p offset 0x%"PRIx64" bytes %"PRId64" flags %d"
>  nvme_qiov_unaligned(const void *qiov, int n, void *base, size_t size, int align) "qiov %p n %d base %p size 0x%zx align 0x%x"
> -nvme_prw_buffered(void *s, uint64_t offset, uint64_t bytes, int niov, int is_write) "s %p offset %"PRId64" bytes %"PRId64" niov %d is_write %d"
> -nvme_rw_done(void *s, int is_write, uint64_t offset, uint64_t bytes, int ret) "s %p is_write %d offset %"PRId64" bytes %"PRId64" ret %d"
> -nvme_dsm(void *s, uint64_t offset, uint64_t bytes) "s %p offset %"PRId64" bytes %"PRId64""
> -nvme_dsm_done(void *s, uint64_t offset, uint64_t bytes, int ret) "s %p offset %"PRId64" bytes %"PRId64" ret %d"
> +nvme_prw_buffered(void *s, uint64_t offset, uint64_t bytes, int niov, int is_write) "s %p offset 0x%"PRIx64" bytes %"PRId64" niov %d is_write %d"
> +nvme_rw_done(void *s, int is_write, uint64_t offset, uint64_t bytes, int ret) "s %p is_write %d offset 0x%"PRIx64" bytes %"PRId64" ret %d"
> +nvme_dsm(void *s, uint64_t offset, uint64_t bytes) "s %p offset 0x%"PRIx64" bytes %"PRId64""
> +nvme_dsm_done(void *s, uint64_t offset, uint64_t bytes, int ret) "s %p offset 0x%"PRIx64" bytes %"PRId64" ret %d"
>  nvme_dma_map_flush(void *s) "s %p"
>  nvme_free_req_queue_wait(void *q) "q %p"
>  nvme_cmd_map_qiov(void *s, void *cmd, void *req, void *qiov, int entries) "s %p cmd %p req %p qiov %p entries %d"
> 

Re: [PATCH 02/25] block/nvme: Use hex format to display offset in trace events

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:24PM +0100, Philippe Mathieu-Daudé wrote:
> Use the same format used for the hw/vfio/ trace events.
> 
> Suggested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/trace-events | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

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

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

[PATCH 03/25] block/nvme: Report warning with warn_report()

[Philippe Mathieu-Daudé]

Instead of displaying warning on stderr, use warn_report()
which also displays it on the monitor.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 739a0a700cb..6f1d7f9b2a1 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -399,8 +399,8 @@ static bool nvme_process_completion(NVMeQueuePair *q)
         }
         cid = le16_to_cpu(c->cid);
         if (cid == 0 || cid > NVME_QUEUE_SIZE) {
-            fprintf(stderr, "Unexpected CID in completion queue: %" PRIu32 "\n",
-                    cid);
+            warn_report("NVMe: Unexpected CID in completion queue: %"PRIu32", "
+                        "queue size: %u", cid, NVME_QUEUE_SIZE);
             continue;
         }
         trace_nvme_complete_command(s, q->index, cid);
-- 
2.26.2

Re: [PATCH 03/25] block/nvme: Report warning with warn_report()

[Keith Busch]

On Tue, Oct 27, 2020 at 02:55:25PM +0100, Philippe Mathieu-Daudé wrote:
> Instead of displaying warning on stderr, use warn_report()
> which also displays it on the monitor.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 739a0a700cb..6f1d7f9b2a1 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -399,8 +399,8 @@ static bool nvme_process_completion(NVMeQueuePair *q)
>          }
>          cid = le16_to_cpu(c->cid);

Not related to your patch, but it stands out as odd that this is treated
as an endian type. The field is just an opaque cookie, so there shouldn't
be a need for byte swapping. It in fact looks like this is broken on a
big-endian host, as the swaping on submission uses a 32-bit value. Won't
that truncate the relavant bits?

>          if (cid == 0 || cid > NVME_QUEUE_SIZE) {
> -            fprintf(stderr, "Unexpected CID in completion queue: %" PRIu32 "\n",
> -                    cid);
> +            warn_report("NVMe: Unexpected CID in completion queue: %"PRIu32", "
> +                        "queue size: %u", cid, NVME_QUEUE_SIZE);
>              continue;
>          }
>          trace_nvme_complete_command(s, q->index, cid);

Re: [PATCH 03/25] block/nvme: Report warning with warn_report()

[Philippe Mathieu-Daudé]

On 10/27/20 3:45 PM, Keith Busch wrote:
> On Tue, Oct 27, 2020 at 02:55:25PM +0100, Philippe Mathieu-Daudé wrote:
>> Instead of displaying warning on stderr, use warn_report()
>> which also displays it on the monitor.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  block/nvme.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 739a0a700cb..6f1d7f9b2a1 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -399,8 +399,8 @@ static bool nvme_process_completion(NVMeQueuePair *q)
>>          }
>>          cid = le16_to_cpu(c->cid);
> 
> Not related to your patch, but it stands out as odd that this is treated
> as an endian type. The field is just an opaque cookie, so there shouldn't
> be a need for byte swapping. It in fact looks like this is broken on a
> big-endian host, as the swaping on submission uses a 32-bit value. Won't
> that truncate the relavant bits?

You are right, thanks for having a look and catching this bug :)

I suppose we never tested on big-endian host yet.

> 
>>          if (cid == 0 || cid > NVME_QUEUE_SIZE) {
>> -            fprintf(stderr, "Unexpected CID in completion queue: %" PRIu32 "\n",
>> -                    cid);
>> +            warn_report("NVMe: Unexpected CID in completion queue: %"PRIu32", "
>> +                        "queue size: %u", cid, NVME_QUEUE_SIZE);
>>              continue;
>>          }
>>          trace_nvme_complete_command(s, q->index, cid);
> 

Re: [PATCH 03/25] block/nvme: Report warning with warn_report()

[Philippe Mathieu-Daudé]

On 10/27/20 4:33 PM, Philippe Mathieu-Daudé wrote:
> On 10/27/20 3:45 PM, Keith Busch wrote:
>> On Tue, Oct 27, 2020 at 02:55:25PM +0100, Philippe Mathieu-Daudé wrote:
>>> Instead of displaying warning on stderr, use warn_report()
>>> which also displays it on the monitor.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>  block/nvme.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/nvme.c b/block/nvme.c
>>> index 739a0a700cb..6f1d7f9b2a1 100644
>>> --- a/block/nvme.c
>>> +++ b/block/nvme.c
>>> @@ -399,8 +399,8 @@ static bool nvme_process_completion(NVMeQueuePair *q)
>>>          }
>>>          cid = le16_to_cpu(c->cid);
>>
>> Not related to your patch, but it stands out as odd that this is treated
>> as an endian type. The field is just an opaque cookie, so there shouldn't
>> be a need for byte swapping. It in fact looks like this is broken on a
>> big-endian host, as the swaping on submission uses a 32-bit value. Won't
>> that truncate the relavant bits?
> 
> You are right, thanks for having a look and catching this bug :)
> 
> I suppose we never tested on big-endian host yet.

FYI we barely have 64-bit testing on x86_64 and aarch64.

Re: [PATCH 03/25] block/nvme: Report warning with warn_report()

[Auger Eric]

Hi Philippe,

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> Instead of displaying warning on stderr, use warn_report()
> which also displays it on the monitor.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> ---
>  block/nvme.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 739a0a700cb..6f1d7f9b2a1 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -399,8 +399,8 @@ static bool nvme_process_completion(NVMeQueuePair *q)
>          }
>          cid = le16_to_cpu(c->cid);
>          if (cid == 0 || cid > NVME_QUEUE_SIZE) {
> -            fprintf(stderr, "Unexpected CID in completion queue: %" PRIu32 "\n",
> -                    cid);
> +            warn_report("NVMe: Unexpected CID in completion queue: %"PRIu32", "
> +                        "queue size: %u", cid, NVME_QUEUE_SIZE);
>              continue;
>          }
>          trace_nvme_complete_command(s, q->index, cid);
> 

Re: [PATCH 03/25] block/nvme: Report warning with warn_report()

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:25PM +0100, Philippe Mathieu-Daudé wrote:
> Instead of displaying warning on stderr, use warn_report()
> which also displays it on the monitor.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

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

[PATCH 04/25] block/nvme: Trace controller capabilities

[Philippe Mathieu-Daudé]

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

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c       | 13 +++++++++++++
 block/trace-events |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/block/nvme.c b/block/nvme.c
index 6f1d7f9b2a1..361b5772b7a 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -727,6 +727,19 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
      * Initialization". */
 
     cap = le64_to_cpu(regs->cap);
+    trace_nvme_controller_capability_raw(cap);
+    trace_nvme_controller_capability("Maximum Queue Entries Supported",
+                                     1 + NVME_CAP_MQES(cap));
+    trace_nvme_controller_capability("Contiguous Queues Required",
+                                     NVME_CAP_CQR(cap));
+    trace_nvme_controller_capability("Doorbell Stride",
+                                     2 << (2 + NVME_CAP_DSTRD(cap)));
+    trace_nvme_controller_capability("Subsystem Reset Supported",
+                                     NVME_CAP_NSSRS(cap));
+    trace_nvme_controller_capability("Memory Page Size Minimum",
+                                     1 << (12 + NVME_CAP_MPSMIN(cap)));
+    trace_nvme_controller_capability("Memory Page Size Maximum",
+                                     1 << (12 + 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 0955c85c783..b90b07b15fa 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -134,6 +134,8 @@ 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_raw(uint64_t value) "0x%08"PRIx64
+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

Re: [PATCH 04/25] block/nvme: Trace controller capabilities

[Auger Eric]

Hi Philippe,

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> Controllers have different capabilities and report them in the
> CAP register. We are particularly interested by the page size
> limits.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c       | 13 +++++++++++++
>  block/trace-events |  2 ++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 6f1d7f9b2a1..361b5772b7a 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -727,6 +727,19 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>       * Initialization". */
>  
>      cap = le64_to_cpu(regs->cap);
> +    trace_nvme_controller_capability_raw(cap);
> +    trace_nvme_controller_capability("Maximum Queue Entries Supported",
> +                                     1 + NVME_CAP_MQES(cap));
> +    trace_nvme_controller_capability("Contiguous Queues Required",
> +                                     NVME_CAP_CQR(cap));
I think this should be +1 too (0's based value)
> +    trace_nvme_controller_capability("Doorbell Stride",
> +                                     2 << (2 + NVME_CAP_DSTRD(cap)));
> +    trace_nvme_controller_capability("Subsystem Reset Supported",
> +                                     NVME_CAP_NSSRS(cap));
> +    trace_nvme_controller_capability("Memory Page Size Minimum",
> +                                     1 << (12 + NVME_CAP_MPSMIN(cap)));
> +    trace_nvme_controller_capability("Memory Page Size Maximum",
> +                                     1 << (12 + 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 0955c85c783..b90b07b15fa 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -134,6 +134,8 @@ 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_raw(uint64_t value) "0x%08"PRIx64
> +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"
> 
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

Re: [PATCH 04/25] block/nvme: Trace controller capabilities

[Philippe Mathieu-Daudé]

On 10/28/20 11:20 AM, Auger Eric wrote:
> Hi Philippe,
> 
> On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
>> Controllers have different capabilities and report them in the
>> CAP register. We are particularly interested by the page size
>> limits.
>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  block/nvme.c       | 13 +++++++++++++
>>  block/trace-events |  2 ++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 6f1d7f9b2a1..361b5772b7a 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -727,6 +727,19 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>>       * Initialization". */
>>  
>>      cap = le64_to_cpu(regs->cap);
>> +    trace_nvme_controller_capability_raw(cap);
>> +    trace_nvme_controller_capability("Maximum Queue Entries Supported",
>> +                                     1 + NVME_CAP_MQES(cap));
>> +    trace_nvme_controller_capability("Contiguous Queues Required",
>> +                                     NVME_CAP_CQR(cap));
> I think this should be +1 too (0's based value)

This is a boolean:

  Contiguous Queues Required (CQR): This field is set to ‘1’ if
  the controller requires that I/O Submission Queues and I/O
  Completion Queues are required to be physically contiguous.
  This field is cleared to ‘0’ if the controller supports I/O
  Submission Queues and I/O Completion Queues that are not
  physically contiguous. If this field is set to ‘1’, then the
  Physically Contiguous bit (CDW11.PC) in the Create I/O Submission
  Queue and Create I/O Completion Queue commands shall be set to ‘1’.

>> +    trace_nvme_controller_capability("Doorbell Stride",
>> +                                     2 << (2 + NVME_CAP_DSTRD(cap)));
>> +    trace_nvme_controller_capability("Subsystem Reset Supported",
>> +                                     NVME_CAP_NSSRS(cap));
>> +    trace_nvme_controller_capability("Memory Page Size Minimum",
>> +                                     1 << (12 + NVME_CAP_MPSMIN(cap)));
>> +    trace_nvme_controller_capability("Memory Page Size Maximum",
>> +                                     1 << (12 + 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 0955c85c783..b90b07b15fa 100644
>> --- a/block/trace-events
>> +++ b/block/trace-events
>> @@ -134,6 +134,8 @@ 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_raw(uint64_t value) "0x%08"PRIx64
>> +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"
>>
> Besides
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 
> Thanks
> 
> Eric
> 

Re: [PATCH 04/25] block/nvme: Trace controller capabilities

[Auger Eric]

Hi Philippe,
On 10/28/20 11:25 AM, Philippe Mathieu-Daudé wrote:
> On 10/28/20 11:20 AM, Auger Eric wrote:
>> Hi Philippe,
>>
>> On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
>>> Controllers have different capabilities and report them in the
>>> CAP register. We are particularly interested by the page size
>>> limits.
>>>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>  block/nvme.c       | 13 +++++++++++++
>>>  block/trace-events |  2 ++
>>>  2 files changed, 15 insertions(+)
>>>
>>> diff --git a/block/nvme.c b/block/nvme.c
>>> index 6f1d7f9b2a1..361b5772b7a 100644
>>> --- a/block/nvme.c
>>> +++ b/block/nvme.c
>>> @@ -727,6 +727,19 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>>>       * Initialization". */
>>>  
>>>      cap = le64_to_cpu(regs->cap);
>>> +    trace_nvme_controller_capability_raw(cap);
>>> +    trace_nvme_controller_capability("Maximum Queue Entries Supported",
>>> +                                     1 + NVME_CAP_MQES(cap));
>>> +    trace_nvme_controller_capability("Contiguous Queues Required",
>>> +                                     NVME_CAP_CQR(cap));
>> I think this should be +1 too (0's based value)
> 
> This is a boolean:
> 
>   Contiguous Queues Required (CQR): This field is set to ‘1’ if
>   the controller requires that I/O Submission Queues and I/O
>   Completion Queues are required to be physically contiguous.
>   This field is cleared to ‘0’ if the controller supports I/O
>   Submission Queues and I/O Completion Queues that are not
>   physically contiguous. If this field is set to ‘1’, then the
>   Physically Contiguous bit (CDW11.PC) in the Create I/O Submission
>   Queue and Create I/O Completion Queue commands shall be set to ‘1’.

Oh I mixed with NCQR :-(

sorry for the noise
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

> 
>>> +    trace_nvme_controller_capability("Doorbell Stride",
>>> +                                     2 << (2 + NVME_CAP_DSTRD(cap)));
>>> +    trace_nvme_controller_capability("Subsystem Reset Supported",
>>> +                                     NVME_CAP_NSSRS(cap));
>>> +    trace_nvme_controller_capability("Memory Page Size Minimum",
>>> +                                     1 << (12 + NVME_CAP_MPSMIN(cap)));
>>> +    trace_nvme_controller_capability("Memory Page Size Maximum",
>>> +                                     1 << (12 + 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 0955c85c783..b90b07b15fa 100644
>>> --- a/block/trace-events
>>> +++ b/block/trace-events
>>> @@ -134,6 +134,8 @@ 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_raw(uint64_t value) "0x%08"PRIx64
>>> +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"
>>>
>> Besides
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>
>> Thanks
>>
>> Eric
>>
> 
> 

[PATCH 05/25] block/nvme: Trace nvme_poll_queue() per queue

[Philippe Mathieu-Daudé]

As we want to enable multiple queues, report the event
in each nvme_poll_queue() call, rather than once in
the callback calling nvme_poll_queues().

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

diff --git a/block/nvme.c b/block/nvme.c
index 361b5772b7a..8d74401ae7a 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -594,6 +594,7 @@ static bool nvme_poll_queue(NVMeQueuePair *q)
     const size_t cqe_offset = q->cq.head * NVME_CQ_ENTRY_BYTES;
     NvmeCqe *cqe = (NvmeCqe *)&q->cq.queue[cqe_offset];
 
+    trace_nvme_poll_queue(q->s, q->index);
     /*
      * Do an early check for completions. q->lock isn't needed because
      * nvme_process_completion() only runs in the event loop thread and
@@ -684,7 +685,6 @@ static bool nvme_poll_cb(void *opaque)
     BDRVNVMeState *s = container_of(e, BDRVNVMeState,
                                     irq_notifier[MSIX_SHARED_IRQ_IDX]);
 
-    trace_nvme_poll_cb(s);
     return nvme_poll_queues(s);
 }
 
diff --git a/block/trace-events b/block/trace-events
index b90b07b15fa..86292f3312b 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -145,7 +145,7 @@ nvme_complete_command(void *s, int index, int cid) "s %p queue %d cid %d"
 nvme_submit_command(void *s, int index, int cid) "s %p queue %d cid %d"
 nvme_submit_command_raw(int c0, int c1, int c2, int c3, int c4, int c5, int c6, int c7) "%02x %02x %02x %02x %02x %02x %02x %02x"
 nvme_handle_event(void *s) "s %p"
-nvme_poll_cb(void *s) "s %p"
+nvme_poll_queue(void *s, unsigned q_index) "s %p q #%u"
 nvme_prw_aligned(void *s, int is_write, uint64_t offset, uint64_t bytes, int flags, int niov) "s %p is_write %d offset 0x%"PRIx64" bytes %"PRId64" flags %d niov %d"
 nvme_write_zeroes(void *s, uint64_t offset, uint64_t bytes, int flags) "s %p offset 0x%"PRIx64" bytes %"PRId64" flags %d"
 nvme_qiov_unaligned(const void *qiov, int n, void *base, size_t size, int align) "qiov %p n %d base %p size 0x%zx align 0x%x"
-- 
2.26.2

Re: [PATCH 05/25] block/nvme: Trace nvme_poll_queue() per queue

[Auger Eric]

Hi,

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> As we want to enable multiple queues, report the event
> in each nvme_poll_queue() call, rather than once in
> the callback calling nvme_poll_queues().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> ---
>  block/nvme.c       | 2 +-
>  block/trace-events | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 361b5772b7a..8d74401ae7a 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -594,6 +594,7 @@ static bool nvme_poll_queue(NVMeQueuePair *q)
>      const size_t cqe_offset = q->cq.head * NVME_CQ_ENTRY_BYTES;
>      NvmeCqe *cqe = (NvmeCqe *)&q->cq.queue[cqe_offset];
>  
> +    trace_nvme_poll_queue(q->s, q->index);
>      /*
>       * Do an early check for completions. q->lock isn't needed because
>       * nvme_process_completion() only runs in the event loop thread and
> @@ -684,7 +685,6 @@ static bool nvme_poll_cb(void *opaque)
>      BDRVNVMeState *s = container_of(e, BDRVNVMeState,
>                                      irq_notifier[MSIX_SHARED_IRQ_IDX]);
>  
> -    trace_nvme_poll_cb(s);
>      return nvme_poll_queues(s);
>  }
>  
> diff --git a/block/trace-events b/block/trace-events
> index b90b07b15fa..86292f3312b 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -145,7 +145,7 @@ nvme_complete_command(void *s, int index, int cid) "s %p queue %d cid %d"
>  nvme_submit_command(void *s, int index, int cid) "s %p queue %d cid %d"
>  nvme_submit_command_raw(int c0, int c1, int c2, int c3, int c4, int c5, int c6, int c7) "%02x %02x %02x %02x %02x %02x %02x %02x"
>  nvme_handle_event(void *s) "s %p"
> -nvme_poll_cb(void *s) "s %p"
> +nvme_poll_queue(void *s, unsigned q_index) "s %p q #%u"
>  nvme_prw_aligned(void *s, int is_write, uint64_t offset, uint64_t bytes, int flags, int niov) "s %p is_write %d offset 0x%"PRIx64" bytes %"PRId64" flags %d niov %d"
>  nvme_write_zeroes(void *s, uint64_t offset, uint64_t bytes, int flags) "s %p offset 0x%"PRIx64" bytes %"PRId64" flags %d"
>  nvme_qiov_unaligned(const void *qiov, int n, void *base, size_t size, int align) "qiov %p n %d base %p size 0x%zx align 0x%x"
> 

Re: [PATCH 05/25] block/nvme: Trace nvme_poll_queue() per queue

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:27PM +0100, Philippe Mathieu-Daudé wrote:
> As we want to enable multiple queues, report the event
> in each nvme_poll_queue() call, rather than once in
> the callback calling nvme_poll_queues().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c       | 2 +-
>  block/trace-events | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

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

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

[PATCH 06/25] block/nvme: Improve nvme_free_req_queue_wait() trace information

[Philippe Mathieu-Daudé]

What we want to trace is the block driver state and the queue index.

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c       | 2 +-
 block/trace-events | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 8d74401ae7a..29d2541b911 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -292,7 +292,7 @@ static NVMeRequest *nvme_get_free_req(NVMeQueuePair *q)
 
     while (q->free_req_head == -1) {
         if (qemu_in_coroutine()) {
-            trace_nvme_free_req_queue_wait(q);
+            trace_nvme_free_req_queue_wait(q->s, q->index);
             qemu_co_queue_wait(&q->free_req_queue, &q->lock);
         } else {
             qemu_mutex_unlock(&q->lock);
diff --git a/block/trace-events b/block/trace-events
index 86292f3312b..cc5e2b55cb5 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -154,7 +154,7 @@ nvme_rw_done(void *s, int is_write, uint64_t offset, uint64_t bytes, int ret) "s
 nvme_dsm(void *s, uint64_t offset, uint64_t bytes) "s %p offset 0x%"PRIx64" bytes %"PRId64""
 nvme_dsm_done(void *s, uint64_t offset, uint64_t bytes, int ret) "s %p offset 0x%"PRIx64" bytes %"PRId64" ret %d"
 nvme_dma_map_flush(void *s) "s %p"
-nvme_free_req_queue_wait(void *q) "q %p"
+nvme_free_req_queue_wait(void *s, unsigned q_index) "s %p q #%u"
 nvme_cmd_map_qiov(void *s, void *cmd, void *req, void *qiov, int entries) "s %p cmd %p req %p qiov %p entries %d"
 nvme_cmd_map_qiov_pages(void *s, int i, uint64_t page) "s %p page[%d] 0x%"PRIx64
 nvme_cmd_map_qiov_iov(void *s, int i, void *page, int pages) "s %p iov[%d] %p pages %d"
-- 
2.26.2

Re: [PATCH 06/25] block/nvme: Improve nvme_free_req_queue_wait() trace information

[Auger Eric]

Hi,

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> What we want to trace is the block driver state and the queue index.
> 
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> ---
>  block/nvme.c       | 2 +-
>  block/trace-events | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 8d74401ae7a..29d2541b911 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -292,7 +292,7 @@ static NVMeRequest *nvme_get_free_req(NVMeQueuePair *q)
>  
>      while (q->free_req_head == -1) {
>          if (qemu_in_coroutine()) {
> -            trace_nvme_free_req_queue_wait(q);
> +            trace_nvme_free_req_queue_wait(q->s, q->index);
>              qemu_co_queue_wait(&q->free_req_queue, &q->lock);
>          } else {
>              qemu_mutex_unlock(&q->lock);
> diff --git a/block/trace-events b/block/trace-events
> index 86292f3312b..cc5e2b55cb5 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -154,7 +154,7 @@ nvme_rw_done(void *s, int is_write, uint64_t offset, uint64_t bytes, int ret) "s
>  nvme_dsm(void *s, uint64_t offset, uint64_t bytes) "s %p offset 0x%"PRIx64" bytes %"PRId64""
>  nvme_dsm_done(void *s, uint64_t offset, uint64_t bytes, int ret) "s %p offset 0x%"PRIx64" bytes %"PRId64" ret %d"
>  nvme_dma_map_flush(void *s) "s %p"
> -nvme_free_req_queue_wait(void *q) "q %p"
> +nvme_free_req_queue_wait(void *s, unsigned q_index) "s %p q #%u"
>  nvme_cmd_map_qiov(void *s, void *cmd, void *req, void *qiov, int entries) "s %p cmd %p req %p qiov %p entries %d"
>  nvme_cmd_map_qiov_pages(void *s, int i, uint64_t page) "s %p page[%d] 0x%"PRIx64
>  nvme_cmd_map_qiov_iov(void *s, int i, void *page, int pages) "s %p iov[%d] %p pages %d"
> 

Re: [PATCH 06/25] block/nvme: Improve nvme_free_req_queue_wait() trace information

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:28PM +0100, Philippe Mathieu-Daudé wrote:
> What we want to trace is the block driver state and the queue index.
> 
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c       | 2 +-
>  block/trace-events | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

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

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

[PATCH 07/25] block/nvme: Trace queue pair creation/deletion

[Philippe Mathieu-Daudé]

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

diff --git a/block/nvme.c b/block/nvme.c
index 29d2541b911..e95d59d3126 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -181,6 +181,7 @@ static void nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
 
 static void nvme_free_queue_pair(NVMeQueuePair *q)
 {
+    trace_nvme_free_queue_pair(q->index, q);
     if (q->completion_bh) {
         qemu_bh_delete(q->completion_bh);
     }
@@ -216,6 +217,8 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
     if (!q) {
         return NULL;
     }
+    trace_nvme_create_queue_pair(idx, q, size, aio_context,
+                                 event_notifier_get_fd(s->irq_notifier));
     q->prp_list_pages = qemu_try_memalign(s->page_size,
                                           s->page_size * NVME_NUM_REQS);
     if (!q->prp_list_pages) {
diff --git a/block/trace-events b/block/trace-events
index cc5e2b55cb5..f6a0f99df1a 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -155,6 +155,8 @@ nvme_dsm(void *s, uint64_t offset, uint64_t bytes) "s %p offset 0x%"PRIx64" byte
 nvme_dsm_done(void *s, uint64_t offset, uint64_t bytes, int ret) "s %p offset 0x%"PRIx64" bytes %"PRId64" ret %d"
 nvme_dma_map_flush(void *s) "s %p"
 nvme_free_req_queue_wait(void *s, unsigned q_index) "s %p q #%u"
+nvme_create_queue_pair(unsigned q_index, void *q, unsigned size, void *aio_context, int fd) "index %u q %p size %u aioctx %p fd %d"
+nvme_free_queue_pair(unsigned q_index, void *q) "index %u q %p"
 nvme_cmd_map_qiov(void *s, void *cmd, void *req, void *qiov, int entries) "s %p cmd %p req %p qiov %p entries %d"
 nvme_cmd_map_qiov_pages(void *s, int i, uint64_t page) "s %p page[%d] 0x%"PRIx64
 nvme_cmd_map_qiov_iov(void *s, int i, void *page, int pages) "s %p iov[%d] %p pages %d"
-- 
2.26.2

Re: [PATCH 07/25] block/nvme: Trace queue pair creation/deletion

[Auger Eric]

Hi Philippe,

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> ---
>  block/nvme.c       | 3 +++
>  block/trace-events | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 29d2541b911..e95d59d3126 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -181,6 +181,7 @@ static void nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
>  
>  static void nvme_free_queue_pair(NVMeQueuePair *q)
>  {
> +    trace_nvme_free_queue_pair(q->index, q);
>      if (q->completion_bh) {
>          qemu_bh_delete(q->completion_bh);
>      }
> @@ -216,6 +217,8 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
>      if (!q) {
>          return NULL;
>      }
> +    trace_nvme_create_queue_pair(idx, q, size, aio_context,
> +                                 event_notifier_get_fd(s->irq_notifier));
>      q->prp_list_pages = qemu_try_memalign(s->page_size,
>                                            s->page_size * NVME_NUM_REQS);
>      if (!q->prp_list_pages) {
> diff --git a/block/trace-events b/block/trace-events
> index cc5e2b55cb5..f6a0f99df1a 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -155,6 +155,8 @@ nvme_dsm(void *s, uint64_t offset, uint64_t bytes) "s %p offset 0x%"PRIx64" byte
>  nvme_dsm_done(void *s, uint64_t offset, uint64_t bytes, int ret) "s %p offset 0x%"PRIx64" bytes %"PRId64" ret %d"
>  nvme_dma_map_flush(void *s) "s %p"
>  nvme_free_req_queue_wait(void *s, unsigned q_index) "s %p q #%u"
> +nvme_create_queue_pair(unsigned q_index, void *q, unsigned size, void *aio_context, int fd) "index %u q %p size %u aioctx %p fd %d"
> +nvme_free_queue_pair(unsigned q_index, void *q) "index %u q %p"
>  nvme_cmd_map_qiov(void *s, void *cmd, void *req, void *qiov, int entries) "s %p cmd %p req %p qiov %p entries %d"
>  nvme_cmd_map_qiov_pages(void *s, int i, uint64_t page) "s %p page[%d] 0x%"PRIx64
>  nvme_cmd_map_qiov_iov(void *s, int i, void *page, int pages) "s %p iov[%d] %p pages %d"
> 

Re: [PATCH 07/25] block/nvme: Trace queue pair creation/deletion

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:29PM +0100, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c       | 3 +++
>  block/trace-events | 2 ++
>  2 files changed, 5 insertions(+)

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

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

[PATCH 08/25] block/nvme: Simplify device reset

[Philippe Mathieu-Daudé]

Avoid multiple endianess conversion by using device endianess.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c
index e95d59d3126..be14350f959 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -755,7 +755,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);
 
     /* Reset device to get a clean state. */
-    regs->cc = cpu_to_le32(le32_to_cpu(regs->cc) & 0xFE);
+    regs->cc &= const_le32(0xFE);
     /* Wait for CSTS.RDY = 0. */
     deadline = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + timeout_ms * SCALE_MS;
     while (NVME_CSTS_RDY(le32_to_cpu(regs->csts))) {
-- 
2.26.2

Re: [PATCH 08/25] block/nvme: Simplify device reset

[Keith Busch]

On Tue, Oct 27, 2020 at 02:55:30PM +0100, Philippe Mathieu-Daudé wrote:
> Avoid multiple endianess conversion by using device endianess.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index e95d59d3126..be14350f959 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -755,7 +755,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>      timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);
>  
>      /* Reset device to get a clean state. */
> -    regs->cc = cpu_to_le32(le32_to_cpu(regs->cc) & 0xFE);
> +    regs->cc &= const_le32(0xFE);

This doesn't look right. The 'regs' is an MMIO address, correct? Memory
mappings use the CPU native.

Re: [PATCH 08/25] block/nvme: Simplify device reset

[Keith Busch]

On Tue, Oct 27, 2020 at 02:55:30PM +0100, Philippe Mathieu-Daudé wrote:
> Avoid multiple endianess conversion by using device endianess.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index e95d59d3126..be14350f959 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -755,7 +755,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>      timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);
>  
>      /* Reset device to get a clean state. */
> -    regs->cc = cpu_to_le32(le32_to_cpu(regs->cc) & 0xFE);
> +    regs->cc &= const_le32(0xFE);

This doesn't look right. The 'regs' is an MMIO address, correct? Memory
mappings use the CPU native access.

Re: [PATCH 08/25] block/nvme: Simplify device reset

[Philippe Mathieu-Daudé]

On 10/27/20 3:58 PM, Keith Busch wrote:
> On Tue, Oct 27, 2020 at 02:55:30PM +0100, Philippe Mathieu-Daudé wrote:
>> Avoid multiple endianess conversion by using device endianess.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  block/nvme.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index e95d59d3126..be14350f959 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -755,7 +755,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>>      timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);
>>  
>>      /* Reset device to get a clean state. */
>> -    regs->cc = cpu_to_le32(le32_to_cpu(regs->cc) & 0xFE);
>> +    regs->cc &= const_le32(0xFE);
> 
> This doesn't look right. The 'regs' is an MMIO address, correct? Memory
> mappings use the CPU native access.

cc is little-endian uint32_t.

on big-endian: const_le32(0xFE) = 0xfe000000;
so: regs->cc &= 0xfe000000.

Anyway this is an example of unproductive patch, as it makes
things more confuse to you. Let's ignore it.

Re: [PATCH 08/25] block/nvme: Simplify device reset

[Keith Busch]

On Tue, Oct 27, 2020 at 04:53:31PM +0100, Philippe Mathieu-Daudé wrote:
> On 10/27/20 3:58 PM, Keith Busch wrote:
> > On Tue, Oct 27, 2020 at 02:55:30PM +0100, Philippe Mathieu-Daudé wrote:
> >> Avoid multiple endianess conversion by using device endianess.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>  block/nvme.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/block/nvme.c b/block/nvme.c
> >> index e95d59d3126..be14350f959 100644
> >> --- a/block/nvme.c
> >> +++ b/block/nvme.c
> >> @@ -755,7 +755,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
> >>      timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);
> >>  
> >>      /* Reset device to get a clean state. */
> >> -    regs->cc = cpu_to_le32(le32_to_cpu(regs->cc) & 0xFE);
> >> +    regs->cc &= const_le32(0xFE);
> > 
> > This doesn't look right. The 'regs' is an MMIO address, correct? Memory
> > mappings use the CPU native access.
> 
> cc is little-endian uint32_t.

Well, yes and no. PCI is defined as a little endian transport, so all
CPUs have to automatically convert from their native format when
accessing memory mapped addresses over that transport, so you always use
the arch native format from the host software.

This isn't just for CC. This includes all memory mapped registers, so
this driver's CSTS, AQA, doorbells, etc... shouldn't have any endian
swapping.

See also: every other nvme driver. :)

Re: [PATCH 08/25] block/nvme: Simplify device reset

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 09:55:34AM -0700, Keith Busch wrote:
> On Tue, Oct 27, 2020 at 04:53:31PM +0100, Philippe Mathieu-Daudé wrote:
> > On 10/27/20 3:58 PM, Keith Busch wrote:
> > > On Tue, Oct 27, 2020 at 02:55:30PM +0100, Philippe Mathieu-Daudé wrote:
> > >> Avoid multiple endianess conversion by using device endianess.
> > >>
> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > >> ---
> > >>  block/nvme.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/block/nvme.c b/block/nvme.c
> > >> index e95d59d3126..be14350f959 100644
> > >> --- a/block/nvme.c
> > >> +++ b/block/nvme.c
> > >> @@ -755,7 +755,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
> > >>      timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);
> > >>  
> > >>      /* Reset device to get a clean state. */
> > >> -    regs->cc = cpu_to_le32(le32_to_cpu(regs->cc) & 0xFE);
> > >> +    regs->cc &= const_le32(0xFE);
> > > 
> > > This doesn't look right. The 'regs' is an MMIO address, correct? Memory
> > > mappings use the CPU native access.
> > 
> > cc is little-endian uint32_t.
> 
> Well, yes and no. PCI is defined as a little endian transport, so all
> CPUs have to automatically convert from their native format when
> accessing memory mapped addresses over that transport, so you always use
> the arch native format from the host software.
> 
> This isn't just for CC. This includes all memory mapped registers, so
> this driver's CSTS, AQA, doorbells, etc... shouldn't have any endian
> swapping.
> 
> See also: every other nvme driver. :)

I don't see the opposite in Linux. The Linux NVMe drivers use byteswap
instructions because readl()/writel() and friends perform little-endian
memory accesses, not native endian memory accesses:

  static int nvme_pci_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
  {
  	writel(val, to_nvme_dev(ctrl)->bar + off);
  	return 0;
  }

arch/arm/include/asm/io.h:

  #define writel(v,c)     ({ __iowmb(); writel_relaxed(v,c); })

where the byteswap happens here:

  #define writel_relaxed(v,c) __raw_writel((__force u32) cpu_to_le32(v),c)

The CPU is using explicit byteswaps, which matches what the QEMU driver
is doing. Am I missing something?

Stefan

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

Re: [PATCH 08/25] block/nvme: Simplify device reset

[Keith Busch]

On Wed, Oct 28, 2020 at 03:02:09PM +0000, Stefan Hajnoczi wrote:
> On Tue, Oct 27, 2020 at 09:55:34AM -0700, Keith Busch wrote:
> > On Tue, Oct 27, 2020 at 04:53:31PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 10/27/20 3:58 PM, Keith Busch wrote:
> > > > On Tue, Oct 27, 2020 at 02:55:30PM +0100, Philippe Mathieu-Daudé wrote:
> > > >> Avoid multiple endianess conversion by using device endianess.
> > > >>
> > > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > >> ---
> > > >>  block/nvme.c | 2 +-
> > > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/block/nvme.c b/block/nvme.c
> > > >> index e95d59d3126..be14350f959 100644
> > > >> --- a/block/nvme.c
> > > >> +++ b/block/nvme.c
> > > >> @@ -755,7 +755,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
> > > >>      timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);
> > > >>  
> > > >>      /* Reset device to get a clean state. */
> > > >> -    regs->cc = cpu_to_le32(le32_to_cpu(regs->cc) & 0xFE);
> > > >> +    regs->cc &= const_le32(0xFE);
> > > > 
> > > > This doesn't look right. The 'regs' is an MMIO address, correct? Memory
> > > > mappings use the CPU native access.
> > > 
> > > cc is little-endian uint32_t.
> > 
> > Well, yes and no. PCI is defined as a little endian transport, so all
> > CPUs have to automatically convert from their native format when
> > accessing memory mapped addresses over that transport, so you always use
> > the arch native format from the host software.
> > 
> > This isn't just for CC. This includes all memory mapped registers, so
> > this driver's CSTS, AQA, doorbells, etc... shouldn't have any endian
> > swapping.
> > 
> > See also: every other nvme driver. :)
> 
> I don't see the opposite in Linux. The Linux NVMe drivers use byteswap
> instructions because readl()/writel() and friends perform little-endian
> memory accesses, not native endian memory accesses:
> 
>   static int nvme_pci_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
>   {
>   	writel(val, to_nvme_dev(ctrl)->bar + off);
>   	return 0;
>   }
> 
> arch/arm/include/asm/io.h:
> 
>   #define writel(v,c)     ({ __iowmb(); writel_relaxed(v,c); })
> 
> where the byteswap happens here:
> 
>   #define writel_relaxed(v,c) __raw_writel((__force u32) cpu_to_le32(v),c)
> 
> The CPU is using explicit byteswaps, which matches what the QEMU driver
> is doing. Am I missing something?

You're not missing anything. I just made a mistake. Looks like I never
followed the implementation that far along for the BE archs.

[PATCH 09/25] block/nvme: Move definitions before structure declarations

[Philippe Mathieu-Daudé]

To be able to use some definitions in structure declarations,
move them earlier. No logical change.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index be14350f959..30075e230ca 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -41,6 +41,16 @@
 
 typedef struct BDRVNVMeState BDRVNVMeState;
 
+/* Same index is used for queues and IRQs */
+#define INDEX_ADMIN     0
+#define INDEX_IO(n)     (1 + n)
+
+/* This driver shares a single MSIX IRQ for the admin and I/O queues */
+enum {
+    MSIX_SHARED_IRQ_IDX = 0,
+    MSIX_IRQ_COUNT = 1
+};
+
 typedef struct {
     int32_t  head, tail;
     uint8_t  *queue;
@@ -81,15 +91,6 @@ typedef struct {
     QEMUBH      *completion_bh;
 } NVMeQueuePair;
 
-#define INDEX_ADMIN     0
-#define INDEX_IO(n)     (1 + n)
-
-/* This driver shares a single MSIX IRQ for the admin and I/O queues */
-enum {
-    MSIX_SHARED_IRQ_IDX = 0,
-    MSIX_IRQ_COUNT = 1
-};
-
 struct BDRVNVMeState {
     AioContext *aio_context;
     QEMUVFIOState *vfio;
-- 
2.26.2

Re: [PATCH 09/25] block/nvme: Move definitions before structure declarations

[Auger Eric]

Hi,

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> To be able to use some definitions in structure declarations,
> move them earlier. No logical change.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

> ---
>  block/nvme.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index be14350f959..30075e230ca 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -41,6 +41,16 @@
>  
>  typedef struct BDRVNVMeState BDRVNVMeState;
>  
> +/* Same index is used for queues and IRQs */
> +#define INDEX_ADMIN     0
> +#define INDEX_IO(n)     (1 + n)
> +
> +/* This driver shares a single MSIX IRQ for the admin and I/O queues */
> +enum {
> +    MSIX_SHARED_IRQ_IDX = 0,
> +    MSIX_IRQ_COUNT = 1
> +};
> +
>  typedef struct {
>      int32_t  head, tail;
>      uint8_t  *queue;
> @@ -81,15 +91,6 @@ typedef struct {
>      QEMUBH      *completion_bh;
>  } NVMeQueuePair;
>  
> -#define INDEX_ADMIN     0
> -#define INDEX_IO(n)     (1 + n)
> -
> -/* This driver shares a single MSIX IRQ for the admin and I/O queues */
> -enum {
> -    MSIX_SHARED_IRQ_IDX = 0,
> -    MSIX_IRQ_COUNT = 1
> -};
> -
>  struct BDRVNVMeState {
>      AioContext *aio_context;
>      QEMUVFIOState *vfio;
> 

Re: [PATCH 09/25] block/nvme: Move definitions before structure declarations

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:31PM +0100, Philippe Mathieu-Daudé wrote:
> To be able to use some definitions in structure declarations,
> move them earlier. No logical change.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)

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

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

[PATCH 10/25] block/nvme: Use unsigned integer for queue counter/size

[Philippe Mathieu-Daudé]

We can not have negative queue count/size/index, use unsigned type.
Rename 'nr_queues' as 'queue_count' to match the spec naming.

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

diff --git a/block/nvme.c b/block/nvme.c
index 30075e230ca..8b0fd59c6ea 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -104,7 +104,7 @@ struct BDRVNVMeState {
      * [1..]: io queues.
      */
     NVMeQueuePair **queues;
-    int nr_queues;
+    unsigned queue_count;
     size_t page_size;
     /* How many uint32_t elements does each doorbell entry take. */
     size_t doorbell_scale;
@@ -161,7 +161,7 @@ static QemuOptsList runtime_opts = {
 };
 
 static void nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
-                            int nentries, int entry_bytes, Error **errp)
+                            unsigned nentries, size_t entry_bytes, Error **errp)
 {
     size_t bytes;
     int r;
@@ -206,7 +206,7 @@ static void nvme_free_req_queue_cb(void *opaque)
 
 static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
                                              AioContext *aio_context,
-                                             int idx, int size,
+                                             unsigned idx, size_t size,
                                              Error **errp)
 {
     int i, r;
@@ -623,7 +623,7 @@ static bool nvme_poll_queues(BDRVNVMeState *s)
     bool progress = false;
     int i;
 
-    for (i = 0; i < s->nr_queues; i++) {
+    for (i = 0; i < s->queue_count; i++) {
         if (nvme_poll_queue(s->queues[i])) {
             progress = true;
         }
@@ -644,10 +644,10 @@ static void nvme_handle_event(EventNotifier *n)
 static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
 {
     BDRVNVMeState *s = bs->opaque;
-    int n = s->nr_queues;
+    unsigned n = s->queue_count;
     NVMeQueuePair *q;
     NvmeCmd cmd;
-    int queue_size = NVME_QUEUE_SIZE;
+    unsigned queue_size = NVME_QUEUE_SIZE;
 
     q = nvme_create_queue_pair(s, bdrv_get_aio_context(bs),
                                n, queue_size, errp);
@@ -661,7 +661,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
         .cdw11 = cpu_to_le32(0x3),
     };
     if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
-        error_setg(errp, "Failed to create CQ io queue [%d]", n);
+        error_setg(errp, "Failed to create CQ io queue [%u]", n);
         goto out_error;
     }
     cmd = (NvmeCmd) {
@@ -671,12 +671,12 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
         .cdw11 = cpu_to_le32(0x1 | (n << 16)),
     };
     if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
-        error_setg(errp, "Failed to create SQ io queue [%d]", n);
+        error_setg(errp, "Failed to create SQ io queue [%u]", n);
         goto out_error;
     }
     s->queues = g_renew(NVMeQueuePair *, s->queues, n + 1);
     s->queues[n] = q;
-    s->nr_queues++;
+    s->queue_count++;
     return true;
 out_error:
     nvme_free_queue_pair(q);
@@ -785,7 +785,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
         ret = -EINVAL;
         goto out;
     }
-    s->nr_queues = 1;
+    s->queue_count = 1;
     QEMU_BUILD_BUG_ON(NVME_QUEUE_SIZE & 0xF000);
     regs->aqa = cpu_to_le32((NVME_QUEUE_SIZE << AQA_ACQS_SHIFT) |
                             (NVME_QUEUE_SIZE << AQA_ASQS_SHIFT));
@@ -895,10 +895,9 @@ static int nvme_enable_disable_write_cache(BlockDriverState *bs, bool enable,
 
 static void nvme_close(BlockDriverState *bs)
 {
-    int i;
     BDRVNVMeState *s = bs->opaque;
 
-    for (i = 0; i < s->nr_queues; ++i) {
+    for (unsigned i = 0; i < s->queue_count; ++i) {
         nvme_free_queue_pair(s->queues[i]);
     }
     g_free(s->queues);
@@ -1123,7 +1122,7 @@ static coroutine_fn int nvme_co_prw_aligned(BlockDriverState *bs,
     };
 
     trace_nvme_prw_aligned(s, is_write, offset, bytes, flags, qiov->niov);
-    assert(s->nr_queues > 1);
+    assert(s->queue_count > 1);
     req = nvme_get_free_req(ioq);
     assert(req);
 
@@ -1233,7 +1232,7 @@ static coroutine_fn int nvme_co_flush(BlockDriverState *bs)
         .ret = -EINPROGRESS,
     };
 
-    assert(s->nr_queues > 1);
+    assert(s->queue_count > 1);
     req = nvme_get_free_req(ioq);
     assert(req);
     nvme_submit_command(ioq, req, &cmd, nvme_rw_cb, &data);
@@ -1285,7 +1284,7 @@ static coroutine_fn int nvme_co_pwrite_zeroes(BlockDriverState *bs,
     cmd.cdw12 = cpu_to_le32(cdw12);
 
     trace_nvme_write_zeroes(s, offset, bytes, flags);
-    assert(s->nr_queues > 1);
+    assert(s->queue_count > 1);
     req = nvme_get_free_req(ioq);
     assert(req);
 
@@ -1328,7 +1327,7 @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
         return -ENOTSUP;
     }
 
-    assert(s->nr_queues > 1);
+    assert(s->queue_count > 1);
 
     buf = qemu_try_memalign(s->page_size, s->page_size);
     if (!buf) {
@@ -1408,7 +1407,7 @@ static void nvme_detach_aio_context(BlockDriverState *bs)
 {
     BDRVNVMeState *s = bs->opaque;
 
-    for (int i = 0; i < s->nr_queues; i++) {
+    for (unsigned i = 0; i < s->queue_count; i++) {
         NVMeQueuePair *q = s->queues[i];
 
         qemu_bh_delete(q->completion_bh);
@@ -1429,7 +1428,7 @@ static void nvme_attach_aio_context(BlockDriverState *bs,
     aio_set_event_notifier(new_context, &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
                            false, nvme_handle_event, nvme_poll_cb);
 
-    for (int i = 0; i < s->nr_queues; i++) {
+    for (unsigned i = 0; i < s->queue_count; i++) {
         NVMeQueuePair *q = s->queues[i];
 
         q->completion_bh =
@@ -1446,11 +1445,10 @@ static void nvme_aio_plug(BlockDriverState *bs)
 
 static void nvme_aio_unplug(BlockDriverState *bs)
 {
-    int i;
     BDRVNVMeState *s = bs->opaque;
     assert(s->plugged);
     s->plugged = false;
-    for (i = INDEX_IO(0); i < s->nr_queues; i++) {
+    for (unsigned i = INDEX_IO(0); i < s->queue_count; i++) {
         NVMeQueuePair *q = s->queues[i];
         qemu_mutex_lock(&q->lock);
         nvme_kick(q);
diff --git a/block/trace-events b/block/trace-events
index f6a0f99df1a..8368f4acb0b 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -136,13 +136,13 @@ qed_aio_write_main(void *s, void *acb, int ret, uint64_t offset, size_t len) "s
 # nvme.c
 nvme_controller_capability_raw(uint64_t value) "0x%08"PRIx64
 nvme_controller_capability(const char *desc, uint64_t value) "%s: %"PRIu64
-nvme_kick(void *s, int queue) "s %p queue %d"
+nvme_kick(void *s, unsigned q_index) "s %p q #%u"
 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"
-nvme_process_completion(void *s, int index, int inflight) "s %p queue %d inflight %d"
-nvme_process_completion_queue_plugged(void *s, int index) "s %p queue %d"
-nvme_complete_command(void *s, int index, int cid) "s %p queue %d cid %d"
-nvme_submit_command(void *s, int index, int cid) "s %p queue %d cid %d"
+nvme_process_completion(void *s, unsigned q_index, int inflight) "s %p q #%u inflight %d"
+nvme_process_completion_queue_plugged(void *s, unsigned q_index) "s %p q #%u"
+nvme_complete_command(void *s, unsigned q_index, int cid) "s %p q #%u cid %d"
+nvme_submit_command(void *s, unsigned q_index, int cid) "s %p q #%u cid %d"
 nvme_submit_command_raw(int c0, int c1, int c2, int c3, int c4, int c5, int c6, int c7) "%02x %02x %02x %02x %02x %02x %02x %02x"
 nvme_handle_event(void *s) "s %p"
 nvme_poll_queue(void *s, unsigned q_index) "s %p q #%u"
-- 
2.26.2

Re: [PATCH 10/25] block/nvme: Use unsigned integer for queue counter/size

[Auger Eric]

Hi,

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> We can not have negative queue count/size/index, use unsigned type.
> Rename 'nr_queues' as 'queue_count' to match the spec naming.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  block/nvme.c       | 38 ++++++++++++++++++--------------------
>  block/trace-events | 10 +++++-----
>  2 files changed, 23 insertions(+), 25 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 30075e230ca..8b0fd59c6ea 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -104,7 +104,7 @@ struct BDRVNVMeState {
>       * [1..]: io queues.
>       */
>      NVMeQueuePair **queues;
> -    int nr_queues;
> +    unsigned queue_count;
>      size_t page_size;
>      /* How many uint32_t elements does each doorbell entry take. */
>      size_t doorbell_scale;
> @@ -161,7 +161,7 @@ static QemuOptsList runtime_opts = {
>  };
>  
>  static void nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
> -                            int nentries, int entry_bytes, Error **errp)
> +                            unsigned nentries, size_t entry_bytes, Error **errp)
>  {
>      size_t bytes;
>      int r;
> @@ -206,7 +206,7 @@ static void nvme_free_req_queue_cb(void *opaque)
>  
>  static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
>                                               AioContext *aio_context,
> -                                             int idx, int size,
> +                                             unsigned idx, size_t size,
>                                               Error **errp)
>  {
>      int i, r;
> @@ -623,7 +623,7 @@ static bool nvme_poll_queues(BDRVNVMeState *s)
>      bool progress = false;
>      int i;
>  
> -    for (i = 0; i < s->nr_queues; i++) {
> +    for (i = 0; i < s->queue_count; i++) {
>          if (nvme_poll_queue(s->queues[i])) {
>              progress = true;
>          }
> @@ -644,10 +644,10 @@ static void nvme_handle_event(EventNotifier *n)
>  static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>  {
>      BDRVNVMeState *s = bs->opaque;
> -    int n = s->nr_queues;
> +    unsigned n = s->queue_count;
>      NVMeQueuePair *q;
>      NvmeCmd cmd;
> -    int queue_size = NVME_QUEUE_SIZE;
> +    unsigned queue_size = NVME_QUEUE_SIZE;
>  
>      q = nvme_create_queue_pair(s, bdrv_get_aio_context(bs),
>                                 n, queue_size, errp);
> @@ -661,7 +661,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>          .cdw11 = cpu_to_le32(0x3),
>      };
>      if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
> -        error_setg(errp, "Failed to create CQ io queue [%d]", n);
> +        error_setg(errp, "Failed to create CQ io queue [%u]", n);
>          goto out_error;
>      }
>      cmd = (NvmeCmd) {
> @@ -671,12 +671,12 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>          .cdw11 = cpu_to_le32(0x1 | (n << 16)),
>      };
>      if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
> -        error_setg(errp, "Failed to create SQ io queue [%d]", n);
> +        error_setg(errp, "Failed to create SQ io queue [%u]", n);
>          goto out_error;
>      }
>      s->queues = g_renew(NVMeQueuePair *, s->queues, n + 1);
>      s->queues[n] = q;
> -    s->nr_queues++;
> +    s->queue_count++;
>      return true;
>  out_error:
>      nvme_free_queue_pair(q);
> @@ -785,7 +785,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>          ret = -EINVAL;
>          goto out;
>      }
> -    s->nr_queues = 1;
> +    s->queue_count = 1;
>      QEMU_BUILD_BUG_ON(NVME_QUEUE_SIZE & 0xF000);
>      regs->aqa = cpu_to_le32((NVME_QUEUE_SIZE << AQA_ACQS_SHIFT) |
>                              (NVME_QUEUE_SIZE << AQA_ASQS_SHIFT));
> @@ -895,10 +895,9 @@ static int nvme_enable_disable_write_cache(BlockDriverState *bs, bool enable,
>  
>  static void nvme_close(BlockDriverState *bs)
>  {
> -    int i;
>      BDRVNVMeState *s = bs->opaque;
>  
> -    for (i = 0; i < s->nr_queues; ++i) {
> +    for (unsigned i = 0; i < s->queue_count; ++i) {
>          nvme_free_queue_pair(s->queues[i]);
>      }
>      g_free(s->queues);
> @@ -1123,7 +1122,7 @@ static coroutine_fn int nvme_co_prw_aligned(BlockDriverState *bs,
>      };
>  
>      trace_nvme_prw_aligned(s, is_write, offset, bytes, flags, qiov->niov);
> -    assert(s->nr_queues > 1);
> +    assert(s->queue_count > 1);
>      req = nvme_get_free_req(ioq);
>      assert(req);
>  
> @@ -1233,7 +1232,7 @@ static coroutine_fn int nvme_co_flush(BlockDriverState *bs)
>          .ret = -EINPROGRESS,
>      };
>  
> -    assert(s->nr_queues > 1);
> +    assert(s->queue_count > 1);
>      req = nvme_get_free_req(ioq);
>      assert(req);
>      nvme_submit_command(ioq, req, &cmd, nvme_rw_cb, &data);
> @@ -1285,7 +1284,7 @@ static coroutine_fn int nvme_co_pwrite_zeroes(BlockDriverState *bs,
>      cmd.cdw12 = cpu_to_le32(cdw12);
>  
>      trace_nvme_write_zeroes(s, offset, bytes, flags);
> -    assert(s->nr_queues > 1);
> +    assert(s->queue_count > 1);
>      req = nvme_get_free_req(ioq);
>      assert(req);
>  
> @@ -1328,7 +1327,7 @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
>          return -ENOTSUP;
>      }
>  
> -    assert(s->nr_queues > 1);
> +    assert(s->queue_count > 1);
>  
>      buf = qemu_try_memalign(s->page_size, s->page_size);
>      if (!buf) {
> @@ -1408,7 +1407,7 @@ static void nvme_detach_aio_context(BlockDriverState *bs)
>  {
>      BDRVNVMeState *s = bs->opaque;
>  
> -    for (int i = 0; i < s->nr_queues; i++) {
> +    for (unsigned i = 0; i < s->queue_count; i++) {
>          NVMeQueuePair *q = s->queues[i];
>  
>          qemu_bh_delete(q->completion_bh);
> @@ -1429,7 +1428,7 @@ static void nvme_attach_aio_context(BlockDriverState *bs,
>      aio_set_event_notifier(new_context, &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
>                             false, nvme_handle_event, nvme_poll_cb);
>  
> -    for (int i = 0; i < s->nr_queues; i++) {
> +    for (unsigned i = 0; i < s->queue_count; i++) {
>          NVMeQueuePair *q = s->queues[i];
>  
>          q->completion_bh =
> @@ -1446,11 +1445,10 @@ static void nvme_aio_plug(BlockDriverState *bs)
>  
>  static void nvme_aio_unplug(BlockDriverState *bs)
>  {
> -    int i;
>      BDRVNVMeState *s = bs->opaque;
>      assert(s->plugged);
>      s->plugged = false;
> -    for (i = INDEX_IO(0); i < s->nr_queues; i++) {
> +    for (unsigned i = INDEX_IO(0); i < s->queue_count; i++) {
>          NVMeQueuePair *q = s->queues[i];
>          qemu_mutex_lock(&q->lock);
>          nvme_kick(q);
> diff --git a/block/trace-events b/block/trace-events
> index f6a0f99df1a..8368f4acb0b 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -136,13 +136,13 @@ qed_aio_write_main(void *s, void *acb, int ret, uint64_t offset, size_t len) "s
>  # nvme.c
>  nvme_controller_capability_raw(uint64_t value) "0x%08"PRIx64
>  nvme_controller_capability(const char *desc, uint64_t value) "%s: %"PRIu64
> -nvme_kick(void *s, int queue) "s %p queue %d"
> +nvme_kick(void *s, unsigned q_index) "s %p q #%u"
>  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"
> -nvme_process_completion(void *s, int index, int inflight) "s %p queue %d inflight %d"
> -nvme_process_completion_queue_plugged(void *s, int index) "s %p queue %d"
> -nvme_complete_command(void *s, int index, int cid) "s %p queue %d cid %d"
> -nvme_submit_command(void *s, int index, int cid) "s %p queue %d cid %d"
> +nvme_process_completion(void *s, unsigned q_index, int inflight) "s %p q #%u inflight %d"
> +nvme_process_completion_queue_plugged(void *s, unsigned q_index) "s %p q #%u"
> +nvme_complete_command(void *s, unsigned q_index, int cid) "s %p q #%u cid %d"
> +nvme_submit_command(void *s, unsigned q_index, int cid) "s %p q #%u cid %d"
>  nvme_submit_command_raw(int c0, int c1, int c2, int c3, int c4, int c5, int c6, int c7) "%02x %02x %02x %02x %02x %02x %02x %02x"
>  nvme_handle_event(void *s) "s %p"
>  nvme_poll_queue(void *s, unsigned q_index) "s %p q #%u"
> 

Re: [PATCH 10/25] block/nvme: Use unsigned integer for queue counter/size

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:32PM +0100, Philippe Mathieu-Daudé wrote:
> We can not have negative queue count/size/index, use unsigned type.
> Rename 'nr_queues' as 'queue_count' to match the spec naming.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c       | 38 ++++++++++++++++++--------------------
>  block/trace-events | 10 +++++-----
>  2 files changed, 23 insertions(+), 25 deletions(-)

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

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

[PATCH 11/25] block/nvme: Make nvme_identify() return boolean indicating error

[Philippe Mathieu-Daudé]

Just for consistency, following the example documented since
commit e3fe3988d7 ("error: Document Error API usage rules"),
return a boolean value indicating an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c
index 8b0fd59c6ea..74994c442e5 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -506,9 +506,11 @@ static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
     return ret;
 }
 
-static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
+/* Returns true on success, false on failure. */
+static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
 {
     BDRVNVMeState *s = bs->opaque;
+    bool ret = false;
     union {
         NvmeIdCtrl ctrl;
         NvmeIdNs ns;
@@ -585,10 +587,13 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
         goto out;
     }
 
+    ret = true;
     s->blkshift = lbaf->ds;
 out:
     qemu_vfio_dma_unmap(s->vfio, id);
     qemu_vfree(id);
+
+    return ret;
 }
 
 static bool nvme_poll_queue(NVMeQueuePair *q)
-- 
2.26.2

Re: [PATCH 11/25] block/nvme: Make nvme_identify() return boolean indicating error

[Auger Eric]

Hi,

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> Just for consistency, following the example documented since
> commit e3fe3988d7 ("error: Document Error API usage rules"),
> return a boolean value indicating an error is set or not.
Then I think the returned value should be used by the caller in this patch

Thanks

Eric
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 8b0fd59c6ea..74994c442e5 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -506,9 +506,11 @@ static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
>      return ret;
>  }
>  
> -static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
> +/* Returns true on success, false on failure. */
> +static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>  {
>      BDRVNVMeState *s = bs->opaque;
> +    bool ret = false;>      union {
>          NvmeIdCtrl ctrl;
>          NvmeIdNs ns;
> @@ -585,10 +587,13 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>          goto out;
>      }
>  
> +    ret = true;
>      s->blkshift = lbaf->ds;
>  out:
>      qemu_vfio_dma_unmap(s->vfio, id);
>      qemu_vfree(id);
> +
> +    return ret;
>  }
>  
>  static bool nvme_poll_queue(NVMeQueuePair *q)
> 

Re: [PATCH 11/25] block/nvme: Make nvme_identify() return boolean indicating error

[Stefan Hajnoczi]

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

On Wed, Oct 28, 2020 at 12:03:00PM +0100, Auger Eric wrote:
> On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> > Just for consistency, following the example documented since
> > commit e3fe3988d7 ("error: Document Error API usage rules"),
> > return a boolean value indicating an error is set or not.
> Then I think the returned value should be used by the caller in this patch

Yes, please.

Stefan

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

[PATCH 12/25] block/nvme: Make nvme_init_queue() return boolean indicating error

[Philippe Mathieu-Daudé]

Just for consistency, following the example documented since
commit e3fe3988d7 ("error: Document Error API usage rules"),
return a boolean value indicating an error is set or not.
This simplifies a bit nvme_create_queue_pair().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 74994c442e5..9324f0bfdc4 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -160,7 +160,8 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static void nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
+/* Returns true on success, false on failure. */
+static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
                             unsigned nentries, size_t entry_bytes, Error **errp)
 {
     size_t bytes;
@@ -171,13 +172,14 @@ static void nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
     q->queue = qemu_try_memalign(s->page_size, bytes);
     if (!q->queue) {
         error_setg(errp, "Cannot allocate queue");
-        return;
+        return false;
     }
     memset(q->queue, 0, bytes);
     r = qemu_vfio_dma_map(s->vfio, q->queue, bytes, false, &q->iova);
     if (r) {
         error_setg(errp, "Cannot map queue");
     }
+    return r == 0;
 }
 
 static void nvme_free_queue_pair(NVMeQueuePair *q)
@@ -210,7 +212,6 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
                                              Error **errp)
 {
     int i, r;
-    Error *local_err = NULL;
     NVMeQueuePair *q;
     uint64_t prp_list_iova;
 
@@ -247,16 +248,12 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
         req->prp_list_iova = prp_list_iova + i * s->page_size;
     }
 
-    nvme_init_queue(s, &q->sq, size, NVME_SQ_ENTRY_BYTES, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!nvme_init_queue(s, &q->sq, size, NVME_SQ_ENTRY_BYTES, errp)) {
         goto fail;
     }
     q->sq.doorbell = &s->doorbells[idx * s->doorbell_scale].sq_tail;
 
-    nvme_init_queue(s, &q->cq, size, NVME_CQ_ENTRY_BYTES, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!nvme_init_queue(s, &q->cq, size, NVME_CQ_ENTRY_BYTES, errp)) {
         goto fail;
     }
     q->cq.doorbell = &s->doorbells[idx * s->doorbell_scale].cq_head;
-- 
2.26.2

Re: [PATCH 12/25] block/nvme: Make nvme_init_queue() return boolean indicating error

[Auger Eric]

Hi,
On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> Just for consistency, following the example documented since
> commit e3fe3988d7 ("error: Document Error API usage rules"),
> return a boolean value indicating an error is set or not.
> This simplifies a bit nvme_create_queue_pair().
also directly pass errp as the local_err is not requested in our case.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 74994c442e5..9324f0bfdc4 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -160,7 +160,8 @@ static QemuOptsList runtime_opts = {
>      },
>  };
>  
> -static void nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
> +/* Returns true on success, false on failure. */
> +static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
>                              unsigned nentries, size_t entry_bytes, Error **errp)
>  {
>      size_t bytes;
> @@ -171,13 +172,14 @@ static void nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
I personally prefer returning a conventional int instead of bool.
>      q->queue = qemu_try_memalign(s->page_size, bytes);
>      if (!q->queue) {
>          error_setg(errp, "Cannot allocate queue");
> -        return;
> +        return false;
>      }
>      memset(q->queue, 0, bytes);
>      r = qemu_vfio_dma_map(s->vfio, q->queue, bytes, false, &q->iova);
>      if (r) {
>          error_setg(errp, "Cannot map queue");
>      }
> +    return r == 0;
also avoids that kind of conversion and use of !() in the called
>  }
>  
>  static void nvme_free_queue_pair(NVMeQueuePair *q)
> @@ -210,7 +212,6 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
>                                               Error **errp)
>  {
>      int i, r;
> -    Error *local_err = NULL;
>      NVMeQueuePair *q;
>      uint64_t prp_list_iova;
>  
> @@ -247,16 +248,12 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
>          req->prp_list_iova = prp_list_iova + i * s->page_size;
>      }
>  
> -    nvme_init_queue(s, &q->sq, size, NVME_SQ_ENTRY_BYTES, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    if (!nvme_init_queue(s, &q->sq, size, NVME_SQ_ENTRY_BYTES, errp)) {
>          goto fail;
>      }
>      q->sq.doorbell = &s->doorbells[idx * s->doorbell_scale].sq_tail;
>  
> -    nvme_init_queue(s, &q->cq, size, NVME_CQ_ENTRY_BYTES, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    if (!nvme_init_queue(s, &q->cq, size, NVME_CQ_ENTRY_BYTES, errp)) {
>          goto fail;
>      }
>      q->cq.doorbell = &s->doorbells[idx * s->doorbell_scale].cq_head;
> 
Thanks

Eric

Re: [PATCH 12/25] block/nvme: Make nvme_init_queue() return boolean indicating error

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:34PM +0100, Philippe Mathieu-Daudé wrote:
> Just for consistency, following the example documented since
> commit e3fe3988d7 ("error: Document Error API usage rules"),
> return a boolean value indicating an error is set or not.
> This simplifies a bit nvme_create_queue_pair().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)

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

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

[PATCH 13/25] block/nvme: Introduce Completion Queue definitions

[Philippe Mathieu-Daudé]

Rename Submission Queue flags with 'Sq' and introduce
Completion Queue flag definitions.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/block/nvme.h | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 65e68a82c89..079f884a2d3 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -491,6 +491,11 @@ typedef struct QEMU_PACKED NvmeCreateCq {
 #define NVME_CQ_FLAGS_PC(cq_flags)  (cq_flags & 0x1)
 #define NVME_CQ_FLAGS_IEN(cq_flags) ((cq_flags >> 1) & 0x1)
 
+enum NvmeFlagsCq {
+    NVME_CQ_PC          = 1,
+    NVME_CQ_IEN         = 2,
+};
+
 typedef struct QEMU_PACKED NvmeCreateSq {
     uint8_t     opcode;
     uint8_t     flags;
@@ -508,12 +513,12 @@ typedef struct QEMU_PACKED NvmeCreateSq {
 #define NVME_SQ_FLAGS_PC(sq_flags)      (sq_flags & 0x1)
 #define NVME_SQ_FLAGS_QPRIO(sq_flags)   ((sq_flags >> 1) & 0x3)
 
-enum NvmeQueueFlags {
-    NVME_Q_PC           = 1,
-    NVME_Q_PRIO_URGENT  = 0,
-    NVME_Q_PRIO_HIGH    = 1,
-    NVME_Q_PRIO_NORMAL  = 2,
-    NVME_Q_PRIO_LOW     = 3,
+enum NvmeFlagsSq {
+    NVME_SQ_PC          = 1,
+    NVME_SQ_PRIO_URGENT = 0,
+    NVME_SQ_PRIO_HIGH   = 1,
+    NVME_SQ_PRIO_NORMAL = 2,
+    NVME_SQ_PRIO_LOW    = 3,
 };
 
 typedef struct QEMU_PACKED NvmeIdentify {
-- 
2.26.2

Re: [PATCH 13/25] block/nvme: Introduce Completion Queue definitions

[Auger Eric]

Hi,

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> Rename Submission Queue flags with 'Sq' 
... to differentiate submission queue flags from command queue flags.

and introduce
> Completion Queue flag definitions.

besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/block/nvme.h | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 65e68a82c89..079f884a2d3 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -491,6 +491,11 @@ typedef struct QEMU_PACKED NvmeCreateCq {
>  #define NVME_CQ_FLAGS_PC(cq_flags)  (cq_flags & 0x1)
>  #define NVME_CQ_FLAGS_IEN(cq_flags) ((cq_flags >> 1) & 0x1)
>  
> +enum NvmeFlagsCq {
> +    NVME_CQ_PC          = 1,
> +    NVME_CQ_IEN         = 2,
> +};
> +
>  typedef struct QEMU_PACKED NvmeCreateSq {
>      uint8_t     opcode;
>      uint8_t     flags;
> @@ -508,12 +513,12 @@ typedef struct QEMU_PACKED NvmeCreateSq {
>  #define NVME_SQ_FLAGS_PC(sq_flags)      (sq_flags & 0x1)
>  #define NVME_SQ_FLAGS_QPRIO(sq_flags)   ((sq_flags >> 1) & 0x3)
>  
> -enum NvmeQueueFlags {
> -    NVME_Q_PC           = 1,
> -    NVME_Q_PRIO_URGENT  = 0,
> -    NVME_Q_PRIO_HIGH    = 1,
> -    NVME_Q_PRIO_NORMAL  = 2,
> -    NVME_Q_PRIO_LOW     = 3,
> +enum NvmeFlagsSq {
> +    NVME_SQ_PC          = 1,
> +    NVME_SQ_PRIO_URGENT = 0,
> +    NVME_SQ_PRIO_HIGH   = 1,
> +    NVME_SQ_PRIO_NORMAL = 2,
> +    NVME_SQ_PRIO_LOW    = 3,
>  };
>  
>  typedef struct QEMU_PACKED NvmeIdentify {
> 

Re: [PATCH 13/25] block/nvme: Introduce Completion Queue definitions

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:35PM +0100, Philippe Mathieu-Daudé wrote:
> Rename Submission Queue flags with 'Sq' and introduce
> Completion Queue flag definitions.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/block/nvme.h | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 65e68a82c89..079f884a2d3 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -491,6 +491,11 @@ typedef struct QEMU_PACKED NvmeCreateCq {
>  #define NVME_CQ_FLAGS_PC(cq_flags)  (cq_flags & 0x1)
>  #define NVME_CQ_FLAGS_IEN(cq_flags) ((cq_flags >> 1) & 0x1)
>  
> +enum NvmeFlagsCq {
> +    NVME_CQ_PC          = 1,
> +    NVME_CQ_IEN         = 2,
> +};
> +
>  typedef struct QEMU_PACKED NvmeCreateSq {
>      uint8_t     opcode;
>      uint8_t     flags;
> @@ -508,12 +513,12 @@ typedef struct QEMU_PACKED NvmeCreateSq {
>  #define NVME_SQ_FLAGS_PC(sq_flags)      (sq_flags & 0x1)
>  #define NVME_SQ_FLAGS_QPRIO(sq_flags)   ((sq_flags >> 1) & 0x3)
>  
> -enum NvmeQueueFlags {
> -    NVME_Q_PC           = 1,
> -    NVME_Q_PRIO_URGENT  = 0,
> -    NVME_Q_PRIO_HIGH    = 1,
> -    NVME_Q_PRIO_NORMAL  = 2,
> -    NVME_Q_PRIO_LOW     = 3,
> +enum NvmeFlagsSq {
> +    NVME_SQ_PC          = 1,
> +    NVME_SQ_PRIO_URGENT = 0,
> +    NVME_SQ_PRIO_HIGH   = 1,
> +    NVME_SQ_PRIO_NORMAL = 2,
> +    NVME_SQ_PRIO_LOW    = 3,
>  };

Why have these constants at all if nothing uses them? I would rather
remove dead code than spend time modifying it.

Stefan

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

Re: [PATCH 13/25] block/nvme: Introduce Completion Queue definitions

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:35PM +0100, Philippe Mathieu-Daudé wrote:
> Rename Submission Queue flags with 'Sq' and introduce
> Completion Queue flag definitions.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/block/nvme.h | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 65e68a82c89..079f884a2d3 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -491,6 +491,11 @@ typedef struct QEMU_PACKED NvmeCreateCq {
>  #define NVME_CQ_FLAGS_PC(cq_flags)  (cq_flags & 0x1)
>  #define NVME_CQ_FLAGS_IEN(cq_flags) ((cq_flags >> 1) & 0x1)
>  
> +enum NvmeFlagsCq {
> +    NVME_CQ_PC          = 1,
> +    NVME_CQ_IEN         = 2,
> +};
> +
>  typedef struct QEMU_PACKED NvmeCreateSq {
>      uint8_t     opcode;
>      uint8_t     flags;
> @@ -508,12 +513,12 @@ typedef struct QEMU_PACKED NvmeCreateSq {
>  #define NVME_SQ_FLAGS_PC(sq_flags)      (sq_flags & 0x1)
>  #define NVME_SQ_FLAGS_QPRIO(sq_flags)   ((sq_flags >> 1) & 0x3)
>  
> -enum NvmeQueueFlags {
> -    NVME_Q_PC           = 1,
> -    NVME_Q_PRIO_URGENT  = 0,
> -    NVME_Q_PRIO_HIGH    = 1,
> -    NVME_Q_PRIO_NORMAL  = 2,
> -    NVME_Q_PRIO_LOW     = 3,
> +enum NvmeFlagsSq {
> +    NVME_SQ_PC          = 1,
> +    NVME_SQ_PRIO_URGENT = 0,
> +    NVME_SQ_PRIO_HIGH   = 1,
> +    NVME_SQ_PRIO_NORMAL = 2,
> +    NVME_SQ_PRIO_LOW    = 3,
>  };

There is also:

  #define NVME_SQ_FLAGS_PC(sq_flags)      (sq_flags & 0x1)
  #define NVME_SQ_FLAGS_QPRIO(sq_flags)   ((sq_flags >> 1) & 0x3)

These macros should use the new constants.

I didn't check if there are additional magic numbers in hw/block/nvme.c
that should be converted.

Stefan

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

Re: [PATCH 13/25] block/nvme: Introduce Completion Queue definitions

[Philippe Mathieu-Daudé]

On 10/28/20 4:16 PM, Stefan Hajnoczi wrote:
> On Tue, Oct 27, 2020 at 02:55:35PM +0100, Philippe Mathieu-Daudé wrote:
>> Rename Submission Queue flags with 'Sq' and introduce
>> Completion Queue flag definitions.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/block/nvme.h | 17 +++++++++++------
>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/block/nvme.h b/include/block/nvme.h
>> index 65e68a82c89..079f884a2d3 100644
>> --- a/include/block/nvme.h
>> +++ b/include/block/nvme.h
>> @@ -491,6 +491,11 @@ typedef struct QEMU_PACKED NvmeCreateCq {
>>  #define NVME_CQ_FLAGS_PC(cq_flags)  (cq_flags & 0x1)
>>  #define NVME_CQ_FLAGS_IEN(cq_flags) ((cq_flags >> 1) & 0x1)
>>  
>> +enum NvmeFlagsCq {
>> +    NVME_CQ_PC          = 1,
>> +    NVME_CQ_IEN         = 2,
>> +};
>> +
>>  typedef struct QEMU_PACKED NvmeCreateSq {
>>      uint8_t     opcode;
>>      uint8_t     flags;
>> @@ -508,12 +513,12 @@ typedef struct QEMU_PACKED NvmeCreateSq {
>>  #define NVME_SQ_FLAGS_PC(sq_flags)      (sq_flags & 0x1)
>>  #define NVME_SQ_FLAGS_QPRIO(sq_flags)   ((sq_flags >> 1) & 0x3)
>>  
>> -enum NvmeQueueFlags {
>> -    NVME_Q_PC           = 1,
>> -    NVME_Q_PRIO_URGENT  = 0,
>> -    NVME_Q_PRIO_HIGH    = 1,
>> -    NVME_Q_PRIO_NORMAL  = 2,
>> -    NVME_Q_PRIO_LOW     = 3,
>> +enum NvmeFlagsSq {
>> +    NVME_SQ_PC          = 1,
>> +    NVME_SQ_PRIO_URGENT = 0,
>> +    NVME_SQ_PRIO_HIGH   = 1,
>> +    NVME_SQ_PRIO_NORMAL = 2,
>> +    NVME_SQ_PRIO_LOW    = 3,
>>  };
> 
> There is also:
> 
>   #define NVME_SQ_FLAGS_PC(sq_flags)      (sq_flags & 0x1)
>   #define NVME_SQ_FLAGS_QPRIO(sq_flags)   ((sq_flags >> 1) & 0x3)
> 
> These macros should use the new constants.
> 
> I didn't check if there are additional magic numbers in hw/block/nvme.c
> that should be converted.

FYI we discussed with Klaus and might convert "block/nvme.h" to
use the registerfields API during the 6.0 dev cycle.

Re: [PATCH 13/25] block/nvme: Introduce Completion Queue definitions

[Philippe Mathieu-Daudé]

On 10/28/20 7:24 PM, Philippe Mathieu-Daudé wrote:
> On 10/28/20 4:16 PM, Stefan Hajnoczi wrote:
>> On Tue, Oct 27, 2020 at 02:55:35PM +0100, Philippe Mathieu-Daudé wrote:
>>> Rename Submission Queue flags with 'Sq' and introduce
>>> Completion Queue flag definitions.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>  include/block/nvme.h | 17 +++++++++++------
>>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/block/nvme.h b/include/block/nvme.h
>>> index 65e68a82c89..079f884a2d3 100644
>>> --- a/include/block/nvme.h
>>> +++ b/include/block/nvme.h
>>> @@ -491,6 +491,11 @@ typedef struct QEMU_PACKED NvmeCreateCq {
>>>  #define NVME_CQ_FLAGS_PC(cq_flags)  (cq_flags & 0x1)
>>>  #define NVME_CQ_FLAGS_IEN(cq_flags) ((cq_flags >> 1) & 0x1)
>>>  
>>> +enum NvmeFlagsCq {
>>> +    NVME_CQ_PC          = 1,
>>> +    NVME_CQ_IEN         = 2,
>>> +};
>>> +
>>>  typedef struct QEMU_PACKED NvmeCreateSq {
>>>      uint8_t     opcode;
>>>      uint8_t     flags;
>>> @@ -508,12 +513,12 @@ typedef struct QEMU_PACKED NvmeCreateSq {
>>>  #define NVME_SQ_FLAGS_PC(sq_flags)      (sq_flags & 0x1)
>>>  #define NVME_SQ_FLAGS_QPRIO(sq_flags)   ((sq_flags >> 1) & 0x3)
>>>  
>>> -enum NvmeQueueFlags {
>>> -    NVME_Q_PC           = 1,
>>> -    NVME_Q_PRIO_URGENT  = 0,
>>> -    NVME_Q_PRIO_HIGH    = 1,
>>> -    NVME_Q_PRIO_NORMAL  = 2,
>>> -    NVME_Q_PRIO_LOW     = 3,
>>> +enum NvmeFlagsSq {
>>> +    NVME_SQ_PC          = 1,
>>> +    NVME_SQ_PRIO_URGENT = 0,
>>> +    NVME_SQ_PRIO_HIGH   = 1,
>>> +    NVME_SQ_PRIO_NORMAL = 2,
>>> +    NVME_SQ_PRIO_LOW    = 3,
>>>  };
>>
>> There is also:
>>
>>   #define NVME_SQ_FLAGS_PC(sq_flags)      (sq_flags & 0x1)
>>   #define NVME_SQ_FLAGS_QPRIO(sq_flags)   ((sq_flags >> 1) & 0x3)
>>
>> These macros should use the new constants.

SQ_PC is bit#0, NVME_SQ_PC is "bit SQ_PC set (PC enabled)",
SQ_PRIO are bits #1-2 (shift by 1, mask 2 bits),
NVME_SQ_PRIO_xxx is the enum of these 2 bits.

The NVME_SQ_FLAGS_X() macros extract the flags.

So the macros can not use the new constants.

>>
>> I didn't check if there are additional magic numbers in hw/block/nvme.c
>> that should be converted.
> 
> FYI we discussed with Klaus and might convert "block/nvme.h" to
> use the registerfields API during the 6.0 dev cycle.

Re: [PATCH 13/25] block/nvme: Introduce Completion Queue definitions

[Stefan Hajnoczi]

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

On Thu, Oct 29, 2020 at 10:02:37AM +0100, Philippe Mathieu-Daudé wrote:
> On 10/28/20 7:24 PM, Philippe Mathieu-Daudé wrote:
> > On 10/28/20 4:16 PM, Stefan Hajnoczi wrote:
> >> On Tue, Oct 27, 2020 at 02:55:35PM +0100, Philippe Mathieu-Daudé wrote:
> >>> Rename Submission Queue flags with 'Sq' and introduce
> >>> Completion Queue flag definitions.
> >>>
> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >>> ---
> >>>  include/block/nvme.h | 17 +++++++++++------
> >>>  1 file changed, 11 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/include/block/nvme.h b/include/block/nvme.h
> >>> index 65e68a82c89..079f884a2d3 100644
> >>> --- a/include/block/nvme.h
> >>> +++ b/include/block/nvme.h
> >>> @@ -491,6 +491,11 @@ typedef struct QEMU_PACKED NvmeCreateCq {
> >>>  #define NVME_CQ_FLAGS_PC(cq_flags)  (cq_flags & 0x1)
> >>>  #define NVME_CQ_FLAGS_IEN(cq_flags) ((cq_flags >> 1) & 0x1)
> >>>  
> >>> +enum NvmeFlagsCq {
> >>> +    NVME_CQ_PC          = 1,
> >>> +    NVME_CQ_IEN         = 2,
> >>> +};
> >>> +
> >>>  typedef struct QEMU_PACKED NvmeCreateSq {
> >>>      uint8_t     opcode;
> >>>      uint8_t     flags;
> >>> @@ -508,12 +513,12 @@ typedef struct QEMU_PACKED NvmeCreateSq {
> >>>  #define NVME_SQ_FLAGS_PC(sq_flags)      (sq_flags & 0x1)
> >>>  #define NVME_SQ_FLAGS_QPRIO(sq_flags)   ((sq_flags >> 1) & 0x3)
> >>>  
> >>> -enum NvmeQueueFlags {
> >>> -    NVME_Q_PC           = 1,
> >>> -    NVME_Q_PRIO_URGENT  = 0,
> >>> -    NVME_Q_PRIO_HIGH    = 1,
> >>> -    NVME_Q_PRIO_NORMAL  = 2,
> >>> -    NVME_Q_PRIO_LOW     = 3,
> >>> +enum NvmeFlagsSq {
> >>> +    NVME_SQ_PC          = 1,
> >>> +    NVME_SQ_PRIO_URGENT = 0,
> >>> +    NVME_SQ_PRIO_HIGH   = 1,
> >>> +    NVME_SQ_PRIO_NORMAL = 2,
> >>> +    NVME_SQ_PRIO_LOW    = 3,
> >>>  };
> >>
> >> There is also:
> >>
> >>   #define NVME_SQ_FLAGS_PC(sq_flags)      (sq_flags & 0x1)
> >>   #define NVME_SQ_FLAGS_QPRIO(sq_flags)   ((sq_flags >> 1) & 0x3)
> >>
> >> These macros should use the new constants.
> 
> SQ_PC is bit#0, NVME_SQ_PC is "bit SQ_PC set (PC enabled)",
> SQ_PRIO are bits #1-2 (shift by 1, mask 2 bits),
> NVME_SQ_PRIO_xxx is the enum of these 2 bits.
> 
> The NVME_SQ_FLAGS_X() macros extract the flags.
> 
> So the macros can not use the new constants.

I'm not sure I understand. Does this mean the header only defines the
flag values but not the bit shift constants?

It seems like hw/block/nvme.c and block/nvme.c are expressing flags in
slightly different approaches. Can they be unified instead of
introducing hw/block/nvme.c- and block/nvme.c-only constants in the
shared header file?

Stefan

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

Re: [PATCH 13/25] block/nvme: Introduce Completion Queue definitions

[Philippe Mathieu-Daudé]

On 10/30/20 12:46 PM, Stefan Hajnoczi wrote:
> On Thu, Oct 29, 2020 at 10:02:37AM +0100, Philippe Mathieu-Daudé wrote:
>> On 10/28/20 7:24 PM, Philippe Mathieu-Daudé wrote:
>>> On 10/28/20 4:16 PM, Stefan Hajnoczi wrote:
>>>> On Tue, Oct 27, 2020 at 02:55:35PM +0100, Philippe Mathieu-Daudé wrote:
>>>>> Rename Submission Queue flags with 'Sq' and introduce
>>>>> Completion Queue flag definitions.
>>>>>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>> ---
>>>>>  include/block/nvme.h | 17 +++++++++++------
>>>>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/include/block/nvme.h b/include/block/nvme.h
>>>>> index 65e68a82c89..079f884a2d3 100644
>>>>> --- a/include/block/nvme.h
>>>>> +++ b/include/block/nvme.h
>>>>> @@ -491,6 +491,11 @@ typedef struct QEMU_PACKED NvmeCreateCq {
>>>>>  #define NVME_CQ_FLAGS_PC(cq_flags)  (cq_flags & 0x1)
>>>>>  #define NVME_CQ_FLAGS_IEN(cq_flags) ((cq_flags >> 1) & 0x1)
>>>>>  
>>>>> +enum NvmeFlagsCq {
>>>>> +    NVME_CQ_PC          = 1,
>>>>> +    NVME_CQ_IEN         = 2,
>>>>> +};
>>>>> +
>>>>>  typedef struct QEMU_PACKED NvmeCreateSq {
>>>>>      uint8_t     opcode;
>>>>>      uint8_t     flags;
>>>>> @@ -508,12 +513,12 @@ typedef struct QEMU_PACKED NvmeCreateSq {
>>>>>  #define NVME_SQ_FLAGS_PC(sq_flags)      (sq_flags & 0x1)
>>>>>  #define NVME_SQ_FLAGS_QPRIO(sq_flags)   ((sq_flags >> 1) & 0x3)
>>>>>  
>>>>> -enum NvmeQueueFlags {
>>>>> -    NVME_Q_PC           = 1,
>>>>> -    NVME_Q_PRIO_URGENT  = 0,
>>>>> -    NVME_Q_PRIO_HIGH    = 1,
>>>>> -    NVME_Q_PRIO_NORMAL  = 2,
>>>>> -    NVME_Q_PRIO_LOW     = 3,
>>>>> +enum NvmeFlagsSq {
>>>>> +    NVME_SQ_PC          = 1,
>>>>> +    NVME_SQ_PRIO_URGENT = 0,
>>>>> +    NVME_SQ_PRIO_HIGH   = 1,
>>>>> +    NVME_SQ_PRIO_NORMAL = 2,
>>>>> +    NVME_SQ_PRIO_LOW    = 3,
>>>>>  };
>>>>
>>>> There is also:
>>>>
>>>>   #define NVME_SQ_FLAGS_PC(sq_flags)      (sq_flags & 0x1)
>>>>   #define NVME_SQ_FLAGS_QPRIO(sq_flags)   ((sq_flags >> 1) & 0x3)
>>>>
>>>> These macros should use the new constants.
>>
>> SQ_PC is bit#0, NVME_SQ_PC is "bit SQ_PC set (PC enabled)",
>> SQ_PRIO are bits #1-2 (shift by 1, mask 2 bits),
>> NVME_SQ_PRIO_xxx is the enum of these 2 bits.
>>
>> The NVME_SQ_FLAGS_X() macros extract the flags.
>>
>> So the macros can not use the new constants.
> 
> I'm not sure I understand. Does this mean the header only defines the
> flag values but not the bit shift constants?

Yes.

> 
> It seems like hw/block/nvme.c and block/nvme.c are expressing flags in
> slightly different approaches. Can they be unified instead of
> introducing hw/block/nvme.c- and block/nvme.c-only constants in the
> shared header file?

I suggested the hw/block/nvme.c to unify the style.
Klaus agreed (at least to have a look). Any change will
be for 6.0 anyway.

[PATCH 14/25] block/nvme: Use definitions instead of magic values in add_io_queue()

[Philippe Mathieu-Daudé]

Replace magic values by definitions, and simplifiy since the
number of queues will never reach 64K.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 9324f0bfdc4..2dfcf8c41d7 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -651,6 +651,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
     NvmeCmd cmd;
     unsigned queue_size = NVME_QUEUE_SIZE;
 
+    assert(n <= UINT16_MAX);
     q = nvme_create_queue_pair(s, bdrv_get_aio_context(bs),
                                n, queue_size, errp);
     if (!q) {
@@ -659,8 +660,8 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
     cmd = (NvmeCmd) {
         .opcode = NVME_ADM_CMD_CREATE_CQ,
         .dptr.prp1 = cpu_to_le64(q->cq.iova),
-        .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | (n & 0xFFFF)),
-        .cdw11 = cpu_to_le32(0x3),
+        .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | n),
+        .cdw11 = cpu_to_le32(NVME_CQ_IEN | NVME_CQ_PC),
     };
     if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
         error_setg(errp, "Failed to create CQ io queue [%u]", n);
@@ -669,8 +670,8 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
     cmd = (NvmeCmd) {
         .opcode = NVME_ADM_CMD_CREATE_SQ,
         .dptr.prp1 = cpu_to_le64(q->sq.iova),
-        .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | (n & 0xFFFF)),
-        .cdw11 = cpu_to_le32(0x1 | (n << 16)),
+        .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | n),
+        .cdw11 = cpu_to_le32(NVME_SQ_PC | (n << 16)),
     };
     if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
         error_setg(errp, "Failed to create SQ io queue [%u]", n);
-- 
2.26.2

Re: [PATCH 14/25] block/nvme: Use definitions instead of magic values in add_io_queue()

[Auger Eric]

Hi Philippe,

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> Replace magic values by definitions, and simplifiy since the
> number of queues will never reach 64K.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

> ---
>  block/nvme.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 9324f0bfdc4..2dfcf8c41d7 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -651,6 +651,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>      NvmeCmd cmd;
>      unsigned queue_size = NVME_QUEUE_SIZE;
>  
> +    assert(n <= UINT16_MAX);
>      q = nvme_create_queue_pair(s, bdrv_get_aio_context(bs),
>                                 n, queue_size, errp);
>      if (!q) {
> @@ -659,8 +660,8 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>      cmd = (NvmeCmd) {
>          .opcode = NVME_ADM_CMD_CREATE_CQ,
>          .dptr.prp1 = cpu_to_le64(q->cq.iova),
> -        .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | (n & 0xFFFF)),
> -        .cdw11 = cpu_to_le32(0x3),
> +        .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | n),
> +        .cdw11 = cpu_to_le32(NVME_CQ_IEN | NVME_CQ_PC),
>      };
>      if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
>          error_setg(errp, "Failed to create CQ io queue [%u]", n);
> @@ -669,8 +670,8 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>      cmd = (NvmeCmd) {
>          .opcode = NVME_ADM_CMD_CREATE_SQ,
>          .dptr.prp1 = cpu_to_le64(q->sq.iova),
> -        .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | (n & 0xFFFF)),
> -        .cdw11 = cpu_to_le32(0x1 | (n << 16)),
> +        .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | n),
> +        .cdw11 = cpu_to_le32(NVME_SQ_PC | (n << 16)),
>      };
>      if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
>          error_setg(errp, "Failed to create SQ io queue [%u]", n);
> 

Re: [PATCH 14/25] block/nvme: Use definitions instead of magic values in add_io_queue()

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:36PM +0100, Philippe Mathieu-Daudé wrote:
> Replace magic values by definitions, and simplifiy since the
> number of queues will never reach 64K.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

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

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

[PATCH 15/25] block/nvme: Correctly initialize Admin Queue Attributes

[Philippe Mathieu-Daudé]

From the specification chapter 3.1.8 "AQA - Admin Queue Attributes"
the Admin Submission Queue Size field is a 0’s based value:

  Admin Submission Queue Size (ASQS):

    Defines the size of the Admin Submission Queue in entries.
    Enabling a controller while this field is cleared to 00h
    produces undefined results. The minimum size of the Admin
    Submission Queue is two entries. The maximum size of the
    Admin Submission Queue is 4096 entries.
    This is a 0’s based value.

This bug has never been hit because the device initialization
uses a single command synchronously :)

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 2dfcf8c41d7..d5df30ec074 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -789,9 +789,9 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
         goto out;
     }
     s->queue_count = 1;
-    QEMU_BUILD_BUG_ON(NVME_QUEUE_SIZE & 0xF000);
-    regs->aqa = cpu_to_le32((NVME_QUEUE_SIZE << AQA_ACQS_SHIFT) |
-                            (NVME_QUEUE_SIZE << AQA_ASQS_SHIFT));
+    QEMU_BUILD_BUG_ON((NVME_QUEUE_SIZE - 1) & 0xF000);
+    regs->aqa = cpu_to_le32(((NVME_QUEUE_SIZE - 1) << AQA_ACQS_SHIFT) |
+                            ((NVME_QUEUE_SIZE - 1) << AQA_ASQS_SHIFT));
     regs->asq = cpu_to_le64(s->queues[INDEX_ADMIN]->sq.iova);
     regs->acq = cpu_to_le64(s->queues[INDEX_ADMIN]->cq.iova);
 
-- 
2.26.2

Re: [PATCH 15/25] block/nvme: Correctly initialize Admin Queue Attributes

[Auger Eric]

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> From the specification chapter 3.1.8 "AQA - Admin Queue Attributes"
> the Admin Submission Queue Size field is a 0’s based value:
> 
>   Admin Submission Queue Size (ASQS):
> 
>     Defines the size of the Admin Submission Queue in entries.
>     Enabling a controller while this field is cleared to 00h
>     produces undefined results. The minimum size of the Admin
>     Submission Queue is two entries. The maximum size of the
>     Admin Submission Queue is 4096 entries.
>     This is a 0’s based value.
> 
> This bug has never been hit because the device initialization
> uses a single command synchronously :)
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>


Eric

> ---
>  block/nvme.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 2dfcf8c41d7..d5df30ec074 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -789,9 +789,9 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>          goto out;
>      }
>      s->queue_count = 1;
> -    QEMU_BUILD_BUG_ON(NVME_QUEUE_SIZE & 0xF000);
> -    regs->aqa = cpu_to_le32((NVME_QUEUE_SIZE << AQA_ACQS_SHIFT) |
> -                            (NVME_QUEUE_SIZE << AQA_ASQS_SHIFT));
> +    QEMU_BUILD_BUG_ON((NVME_QUEUE_SIZE - 1) & 0xF000);
> +    regs->aqa = cpu_to_le32(((NVME_QUEUE_SIZE - 1) << AQA_ACQS_SHIFT) |
> +                            ((NVME_QUEUE_SIZE - 1) << AQA_ASQS_SHIFT));
>      regs->asq = cpu_to_le64(s->queues[INDEX_ADMIN]->sq.iova);
>      regs->acq = cpu_to_le64(s->queues[INDEX_ADMIN]->cq.iova);
>  
> 

Re: [PATCH 15/25] block/nvme: Correctly initialize Admin Queue Attributes

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:37PM +0100, Philippe Mathieu-Daudé wrote:
> From the specification chapter 3.1.8 "AQA - Admin Queue Attributes"
> the Admin Submission Queue Size field is a 0’s based value:
> 
>   Admin Submission Queue Size (ASQS):
> 
>     Defines the size of the Admin Submission Queue in entries.
>     Enabling a controller while this field is cleared to 00h
>     produces undefined results. The minimum size of the Admin
>     Submission Queue is two entries. The maximum size of the
>     Admin Submission Queue is 4096 entries.
>     This is a 0’s based value.
> 
> This bug has never been hit because the device initialization
> uses a single command synchronously :)
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

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

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

[PATCH 16/25] block/nvme: Simplify ADMIN queue access

[Philippe Mathieu-Daudé]

We don't need to dereference from BDRVNVMeState each time.
Use a NVMeQueuePair pointer to the admin queue and use it.
The nvme_init() becomes easier to review, matching the style
of nvme_add_io_queue().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index d5df30ec074..2d3648694b0 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -699,6 +699,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
                      Error **errp)
 {
     BDRVNVMeState *s = bs->opaque;
+    NVMeQueuePair *q;
     AioContext *aio_context = bdrv_get_aio_context(bs);
     int ret;
     uint64_t cap;
@@ -781,19 +782,18 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
 
     /* Set up admin queue. */
     s->queues = g_new(NVMeQueuePair *, 1);
-    s->queues[INDEX_ADMIN] = nvme_create_queue_pair(s, aio_context, 0,
-                                                          NVME_QUEUE_SIZE,
-                                                          errp);
-    if (!s->queues[INDEX_ADMIN]) {
+    q = nvme_create_queue_pair(s, aio_context, 0, NVME_QUEUE_SIZE, errp);
+    if (!q) {
         ret = -EINVAL;
         goto out;
     }
+    s->queues[INDEX_ADMIN] = q;
     s->queue_count = 1;
     QEMU_BUILD_BUG_ON((NVME_QUEUE_SIZE - 1) & 0xF000);
     regs->aqa = cpu_to_le32(((NVME_QUEUE_SIZE - 1) << AQA_ACQS_SHIFT) |
                             ((NVME_QUEUE_SIZE - 1) << AQA_ASQS_SHIFT));
-    regs->asq = cpu_to_le64(s->queues[INDEX_ADMIN]->sq.iova);
-    regs->acq = cpu_to_le64(s->queues[INDEX_ADMIN]->cq.iova);
+    regs->asq = cpu_to_le64(q->sq.iova);
+    regs->acq = cpu_to_le64(q->cq.iova);
 
     /* After setting up all control registers we can enable device now. */
     regs->cc = cpu_to_le32((ctz32(NVME_CQ_ENTRY_BYTES) << CC_IOCQES_SHIFT) |
-- 
2.26.2

Re: [PATCH 16/25] block/nvme: Simplify ADMIN queue access

[Auger Eric]

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> We don't need to dereference from BDRVNVMeState each time.
> Use a NVMeQueuePair pointer to the admin queue and use it.
double "use"
> The nvme_init() becomes easier to review, matching the style
> of nvme_add_io_queue().>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

> ---
>  block/nvme.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index d5df30ec074..2d3648694b0 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -699,6 +699,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>                       Error **errp)
>  {
>      BDRVNVMeState *s = bs->opaque;
> +    NVMeQueuePair *q;
>      AioContext *aio_context = bdrv_get_aio_context(bs);
>      int ret;
>      uint64_t cap;
> @@ -781,19 +782,18 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>  
>      /* Set up admin queue. */
>      s->queues = g_new(NVMeQueuePair *, 1);
> -    s->queues[INDEX_ADMIN] = nvme_create_queue_pair(s, aio_context, 0,
> -                                                          NVME_QUEUE_SIZE,
> -                                                          errp);
> -    if (!s->queues[INDEX_ADMIN]) {
> +    q = nvme_create_queue_pair(s, aio_context, 0, NVME_QUEUE_SIZE, errp);
> +    if (!q) {
>          ret = -EINVAL;
>          goto out;
>      }
> +    s->queues[INDEX_ADMIN] = q;
>      s->queue_count = 1;
>      QEMU_BUILD_BUG_ON((NVME_QUEUE_SIZE - 1) & 0xF000);
>      regs->aqa = cpu_to_le32(((NVME_QUEUE_SIZE - 1) << AQA_ACQS_SHIFT) |
>                              ((NVME_QUEUE_SIZE - 1) << AQA_ASQS_SHIFT));
> -    regs->asq = cpu_to_le64(s->queues[INDEX_ADMIN]->sq.iova);
> -    regs->acq = cpu_to_le64(s->queues[INDEX_ADMIN]->cq.iova);
> +    regs->asq = cpu_to_le64(q->sq.iova);
> +    regs->acq = cpu_to_le64(q->cq.iova);
>  
>      /* After setting up all control registers we can enable device now. */
>      regs->cc = cpu_to_le32((ctz32(NVME_CQ_ENTRY_BYTES) << CC_IOCQES_SHIFT) |
> 

Re: [PATCH 16/25] block/nvme: Simplify ADMIN queue access

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:38PM +0100, Philippe Mathieu-Daudé wrote:
> We don't need to dereference from BDRVNVMeState each time.
> Use a NVMeQueuePair pointer to the admin queue and use it.
> The nvme_init() becomes easier to review, matching the style
> of nvme_add_io_queue().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

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

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

[PATCH 17/25] block/nvme: Simplify nvme_cmd_sync()

[Philippe Mathieu-Daudé]

As all commands use the ADMIN queue, it is pointless to pass
it as argument each time. Remove the argument.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 2d3648694b0..68f0c3f7959 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -487,9 +487,10 @@ static void nvme_cmd_sync_cb(void *opaque, int ret)
     aio_wait_kick();
 }
 
-static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
-                         NvmeCmd *cmd)
+static int nvme_cmd_sync(BlockDriverState *bs, NvmeCmd *cmd)
 {
+    BDRVNVMeState *s = bs->opaque;
+    NVMeQueuePair *q = s->queues[INDEX_ADMIN];
     AioContext *aio_context = bdrv_get_aio_context(bs);
     NVMeRequest *req;
     int ret = -EINPROGRESS;
@@ -534,7 +535,7 @@ static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
 
     memset(id, 0, sizeof(*id));
     cmd.dptr.prp1 = cpu_to_le64(iova);
-    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
+    if (nvme_cmd_sync(bs, &cmd)) {
         error_setg(errp, "Failed to identify controller");
         goto out;
     }
@@ -557,7 +558,7 @@ static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
     memset(id, 0, sizeof(*id));
     cmd.cdw10 = 0;
     cmd.nsid = cpu_to_le32(namespace);
-    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
+    if (nvme_cmd_sync(bs, &cmd)) {
         error_setg(errp, "Failed to identify namespace");
         goto out;
     }
@@ -663,7 +664,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
         .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | n),
         .cdw11 = cpu_to_le32(NVME_CQ_IEN | NVME_CQ_PC),
     };
-    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
+    if (nvme_cmd_sync(bs, &cmd)) {
         error_setg(errp, "Failed to create CQ io queue [%u]", n);
         goto out_error;
     }
@@ -673,7 +674,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
         .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | n),
         .cdw11 = cpu_to_le32(NVME_SQ_PC | (n << 16)),
     };
-    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
+    if (nvme_cmd_sync(bs, &cmd)) {
         error_setg(errp, "Failed to create SQ io queue [%u]", n);
         goto out_error;
     }
@@ -889,7 +890,7 @@ static int nvme_enable_disable_write_cache(BlockDriverState *bs, bool enable,
         .cdw11 = cpu_to_le32(enable ? 0x01 : 0x00),
     };
 
-    ret = nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd);
+    ret = nvme_cmd_sync(bs, &cmd);
     if (ret) {
         error_setg(errp, "Failed to configure NVMe write cache");
     }
-- 
2.26.2

Re: [PATCH 17/25] block/nvme: Simplify nvme_cmd_sync()

[Auger Eric]

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> As all commands use the ADMIN queue, it is pointless to pass
> it as argument each time. Remove the argument.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  block/nvme.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 2d3648694b0..68f0c3f7959 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -487,9 +487,10 @@ static void nvme_cmd_sync_cb(void *opaque, int ret)
>      aio_wait_kick();
>  }
>  
> -static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
> -                         NvmeCmd *cmd)
> +static int nvme_cmd_sync(BlockDriverState *bs, NvmeCmd *cmd)
>  {
> +    BDRVNVMeState *s = bs->opaque;
> +    NVMeQueuePair *q = s->queues[INDEX_ADMIN];
>      AioContext *aio_context = bdrv_get_aio_context(bs);
>      NVMeRequest *req;
>      int ret = -EINPROGRESS;
> @@ -534,7 +535,7 @@ static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>  
>      memset(id, 0, sizeof(*id));
>      cmd.dptr.prp1 = cpu_to_le64(iova);
> -    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
> +    if (nvme_cmd_sync(bs, &cmd)) {
>          error_setg(errp, "Failed to identify controller");
>          goto out;
>      }
> @@ -557,7 +558,7 @@ static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>      memset(id, 0, sizeof(*id));
>      cmd.cdw10 = 0;
>      cmd.nsid = cpu_to_le32(namespace);
> -    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
> +    if (nvme_cmd_sync(bs, &cmd)) {
>          error_setg(errp, "Failed to identify namespace");
>          goto out;
>      }
> @@ -663,7 +664,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>          .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | n),
>          .cdw11 = cpu_to_le32(NVME_CQ_IEN | NVME_CQ_PC),
>      };
> -    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
> +    if (nvme_cmd_sync(bs, &cmd)) {
>          error_setg(errp, "Failed to create CQ io queue [%u]", n);
>          goto out_error;
>      }
> @@ -673,7 +674,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>          .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | n),
>          .cdw11 = cpu_to_le32(NVME_SQ_PC | (n << 16)),
>      };
> -    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
> +    if (nvme_cmd_sync(bs, &cmd)) {
>          error_setg(errp, "Failed to create SQ io queue [%u]", n);
>          goto out_error;
>      }
> @@ -889,7 +890,7 @@ static int nvme_enable_disable_write_cache(BlockDriverState *bs, bool enable,
>          .cdw11 = cpu_to_le32(enable ? 0x01 : 0x00),
>      };
>  
> -    ret = nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd);
> +    ret = nvme_cmd_sync(bs, &cmd);
>      if (ret) {
>          error_setg(errp, "Failed to configure NVMe write cache");
>      }
> 

Re: [PATCH 17/25] block/nvme: Simplify nvme_cmd_sync()

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:39PM +0100, Philippe Mathieu-Daudé wrote:
> As all commands use the ADMIN queue, it is pointless to pass
> it as argument each time. Remove the argument.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 2d3648694b0..68f0c3f7959 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -487,9 +487,10 @@ static void nvme_cmd_sync_cb(void *opaque, int ret)
>      aio_wait_kick();
>  }
>  
> -static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
> -                         NvmeCmd *cmd)
> +static int nvme_cmd_sync(BlockDriverState *bs, NvmeCmd *cmd)

Please rename the function to nvme_admin_cmd_sync() so it's behavior is
clear.

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

Re: [PATCH 17/25] block/nvme: Simplify nvme_cmd_sync()

[Philippe Mathieu-Daudé]

On 10/28/20 4:21 PM, Stefan Hajnoczi wrote:
> On Tue, Oct 27, 2020 at 02:55:39PM +0100, Philippe Mathieu-Daudé wrote:
>> As all commands use the ADMIN queue, it is pointless to pass
>> it as argument each time. Remove the argument.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  block/nvme.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 2d3648694b0..68f0c3f7959 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -487,9 +487,10 @@ static void nvme_cmd_sync_cb(void *opaque, int ret)
>>      aio_wait_kick();
>>  }
>>  
>> -static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
>> -                         NvmeCmd *cmd)
>> +static int nvme_cmd_sync(BlockDriverState *bs, NvmeCmd *cmd)
> 
> Please rename the function to nvme_admin_cmd_sync() so it's behavior is
> clear.

Good idea :)

[PATCH 18/25] block/nvme: Pass AioContext argument to nvme_add_io_queue()

[Philippe Mathieu-Daudé]

We want to get ride of the BlockDriverState pointer at some point,
so pass aio_context along.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 68f0c3f7959..a0871fc2a81 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -644,7 +644,9 @@ static void nvme_handle_event(EventNotifier *n)
     nvme_poll_queues(s);
 }
 
-static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
+/* Returns true on success, false on failure. */
+static bool nvme_add_io_queue(BlockDriverState *bs,
+                              AioContext *aio_context, Error **errp)
 {
     BDRVNVMeState *s = bs->opaque;
     unsigned n = s->queue_count;
@@ -653,8 +655,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
     unsigned queue_size = NVME_QUEUE_SIZE;
 
     assert(n <= UINT16_MAX);
-    q = nvme_create_queue_pair(s, bdrv_get_aio_context(bs),
-                               n, queue_size, errp);
+    q = nvme_create_queue_pair(s, aio_context, n, queue_size, errp);
     if (!q) {
         return false;
     }
@@ -830,7 +831,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     }
 
     /* Set up command queues. */
-    if (!nvme_add_io_queue(bs, errp)) {
+    if (!nvme_add_io_queue(bs, aio_context, errp)) {
         ret = -EIO;
     }
 out:
-- 
2.26.2

Re: [PATCH 18/25] block/nvme: Pass AioContext argument to nvme_add_io_queue()

[Auger Eric]

Hi Philippe,

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> We want to get ride of the BlockDriverState pointer at some point,
s/ride/rid
> so pass aio_context along.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 68f0c3f7959..a0871fc2a81 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -644,7 +644,9 @@ static void nvme_handle_event(EventNotifier *n)
>      nvme_poll_queues(s);
>  }
>  
> -static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
> +/* Returns true on success, false on failure. */
belongs to another patch, still not a big fan of bool ;-)
> +static bool nvme_add_io_queue(BlockDriverState *bs,
> +                              AioContext *aio_context, Error **errp)
>  {
>      BDRVNVMeState *s = bs->opaque;
>      unsigned n = s->queue_count;
> @@ -653,8 +655,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>      unsigned queue_size = NVME_QUEUE_SIZE;
>  
>      assert(n <= UINT16_MAX);
> -    q = nvme_create_queue_pair(s, bdrv_get_aio_context(bs),
> -                               n, queue_size, errp);
> +    q = nvme_create_queue_pair(s, aio_context, n, queue_size, errp);
>      if (!q) {
>          return false;
>      }
> @@ -830,7 +831,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>      }
>  
>      /* Set up command queues. */
> -    if (!nvme_add_io_queue(bs, errp)) {
> +    if (!nvme_add_io_queue(bs, aio_context, errp)) {
>          ret = -EIO;
>      }
>  out:
> 
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

Re: [PATCH 18/25] block/nvme: Pass AioContext argument to nvme_add_io_queue()

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:40PM +0100, Philippe Mathieu-Daudé wrote:
> We want to get ride of the BlockDriverState pointer at some point,
> so pass aio_context along.

Potential future changes are a weak justification. Someone else might
decide that the aio_context argument is redundant and reverse this
change. Either way works, so this change is somewhat arbitrary at the
moment.

A strong justification would be if the next patch removes the BDS
pointer argument.

Until this change is necessary I would prefer not to include it. That
saves reviewer time, reduces the risk of introducing bugs, etc.

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

[PATCH 19/25] block/nvme: Set request_alignment at initialization

[Philippe Mathieu-Daudé]

Commit bdd6a90a9e5 ("block: Add VFIO based NVMe driver")
sets the request_alignment in nvme_refresh_limits().
For consistency, also set it during initialization.

Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/nvme.c b/block/nvme.c
index a0871fc2a81..682946ad840 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -759,6 +759,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     s->page_size = MAX(4096, 1 << NVME_CAP_MPSMIN(cap));
     s->doorbell_scale = (4 << NVME_CAP_DSTRD(cap)) / sizeof(uint32_t);
     bs->bl.opt_mem_alignment = s->page_size;
+    bs->bl.request_alignment = s->page_size;
     timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);
 
     /* Reset device to get a clean state. */
-- 
2.26.2

[PATCH 20/25] block/nvme: Correct minimum device page size

[Philippe Mathieu-Daudé]

While trying to simplify the code using a macro, we forgot
the 12-bit shift... Correct that.

Fixes: fad1eb68862 ("block/nvme: Use register definitions from 'block/nvme.h'")
Reported-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c
index 682946ad840..d2d57a287cc 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -756,7 +756,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
         goto out;
     }
 
-    s->page_size = MAX(4096, 1 << NVME_CAP_MPSMIN(cap));
+    s->page_size = 1u << (12 + NVME_CAP_MPSMIN(cap));
     s->doorbell_scale = (4 << NVME_CAP_DSTRD(cap)) / sizeof(uint32_t);
     bs->bl.opt_mem_alignment = s->page_size;
     bs->bl.request_alignment = s->page_size;
-- 
2.26.2

[PATCH 21/25] block/nvme: Change size and alignment of IDENTIFY response buffer

[Philippe Mathieu-Daudé]

From: Eric Auger <eric.auger@redhat.com>

In preparation of 64kB host page support, let's change the size
and alignment of the IDENTIFY command response buffer so that
the VFIO DMA MAP succeeds. We align on the host page size.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index d2d57a287cc..ad70303c5c1 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -521,19 +521,20 @@ static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
         .opcode = NVME_ADM_CMD_IDENTIFY,
         .cdw10 = cpu_to_le32(0x1),
     };
+    size_t id_size = QEMU_ALIGN_UP(sizeof(*id), qemu_real_host_page_size);
 
-    id = qemu_try_memalign(s->page_size, sizeof(*id));
+    id = qemu_try_memalign(qemu_real_host_page_size, id_size);
     if (!id) {
         error_setg(errp, "Cannot allocate buffer for identify response");
         goto out;
     }
-    r = qemu_vfio_dma_map(s->vfio, id, sizeof(*id), true, &iova);
+    r = qemu_vfio_dma_map(s->vfio, id, id_size, true, &iova);
     if (r) {
         error_setg(errp, "Cannot map buffer for DMA");
         goto out;
     }
 
-    memset(id, 0, sizeof(*id));
+    memset(id, 0, id_size);
     cmd.dptr.prp1 = cpu_to_le64(iova);
     if (nvme_cmd_sync(bs, &cmd)) {
         error_setg(errp, "Failed to identify controller");
@@ -555,7 +556,7 @@ static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
     s->supports_write_zeroes = !!(oncs & NVME_ONCS_WRITE_ZEROES);
     s->supports_discard = !!(oncs & NVME_ONCS_DSM);
 
-    memset(id, 0, sizeof(*id));
+    memset(id, 0, id_size);
     cmd.cdw10 = 0;
     cmd.nsid = cpu_to_le32(namespace);
     if (nvme_cmd_sync(bs, &cmd)) {
-- 
2.26.2

Re: [PATCH 21/25] block/nvme: Change size and alignment of IDENTIFY response buffer

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:43PM +0100, Philippe Mathieu-Daudé wrote:
> From: Eric Auger <eric.auger@redhat.com>
> 
> In preparation of 64kB host page support, let's change the size
> and alignment of the IDENTIFY command response buffer so that
> the VFIO DMA MAP succeeds. We align on the host page size.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

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

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

[PATCH 22/25] block/nvme: Change size and alignment of queue

[Philippe Mathieu-Daudé]

From: Eric Auger <eric.auger@redhat.com>

In preparation of 64kB host page support, let's change the size
and alignment of the queue so that the VFIO DMA MAP succeeds.
We align on the host page size.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index ad70303c5c1..941f96b4c92 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -167,9 +167,9 @@ static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
     size_t bytes;
     int r;
 
-    bytes = ROUND_UP(nentries * entry_bytes, s->page_size);
+    bytes = ROUND_UP(nentries * entry_bytes, qemu_real_host_page_size);
     q->head = q->tail = 0;
-    q->queue = qemu_try_memalign(s->page_size, bytes);
+    q->queue = qemu_try_memalign(qemu_real_host_page_size, bytes);
     if (!q->queue) {
         error_setg(errp, "Cannot allocate queue");
         return false;
-- 
2.26.2

Re: [PATCH 22/25] block/nvme: Change size and alignment of queue

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:44PM +0100, Philippe Mathieu-Daudé wrote:
> From: Eric Auger <eric.auger@redhat.com>
> 
> In preparation of 64kB host page support, let's change the size
> and alignment of the queue so that the VFIO DMA MAP succeeds.
> We align on the host page size.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

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

[PATCH 23/25] block/nvme: Change size and alignment of prp_list_pages

[Philippe Mathieu-Daudé]

From: Eric Auger <eric.auger@redhat.com>

In preparation of 64kB host page support, let's change the size
and alignment of the prp_list_pages so that the VFIO DMA MAP succeeds
with 64kB host page size. We align on the host page size.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 941f96b4c92..e3626045565 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -214,6 +214,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
     int i, r;
     NVMeQueuePair *q;
     uint64_t prp_list_iova;
+    size_t bytes;
 
     q = g_try_new0(NVMeQueuePair, 1);
     if (!q) {
@@ -221,19 +222,19 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
     }
     trace_nvme_create_queue_pair(idx, q, size, aio_context,
                                  event_notifier_get_fd(s->irq_notifier));
-    q->prp_list_pages = qemu_try_memalign(s->page_size,
-                                          s->page_size * NVME_NUM_REQS);
+    bytes = QEMU_ALIGN_UP(s->page_size * NVME_NUM_REQS,
+                          qemu_real_host_page_size);
+    q->prp_list_pages = qemu_try_memalign(qemu_real_host_page_size, bytes);
     if (!q->prp_list_pages) {
         goto fail;
     }
-    memset(q->prp_list_pages, 0, s->page_size * NVME_NUM_REQS);
+    memset(q->prp_list_pages, 0, bytes);
     qemu_mutex_init(&q->lock);
     q->s = s;
     q->index = idx;
     qemu_co_queue_init(&q->free_req_queue);
     q->completion_bh = aio_bh_new(aio_context, nvme_process_completion_bh, q);
-    r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
-                          s->page_size * NVME_NUM_REQS,
+    r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages, bytes,
                           false, &prp_list_iova);
     if (r) {
         goto fail;
-- 
2.26.2

Re: [PATCH 23/25] block/nvme: Change size and alignment of prp_list_pages

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:45PM +0100, Philippe Mathieu-Daudé wrote:
> From: Eric Auger <eric.auger@redhat.com>
> 
> In preparation of 64kB host page support, let's change the size
> and alignment of the prp_list_pages so that the VFIO DMA MAP succeeds
> with 64kB host page size. We align on the host page size.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

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

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

[PATCH 24/25] block/nvme: Align iov's va and size on host page size

[Philippe Mathieu-Daudé]

From: Eric Auger <eric.auger@redhat.com>

Make sure iov's va and size are properly aligned on the
host page size.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index e3626045565..c1c52bae44f 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -1018,11 +1018,12 @@ static coroutine_fn int nvme_cmd_map_qiov(BlockDriverState *bs, NvmeCmd *cmd,
     for (i = 0; i < qiov->niov; ++i) {
         bool retry = true;
         uint64_t iova;
+        size_t len = QEMU_ALIGN_UP(qiov->iov[i].iov_len,
+                                   qemu_real_host_page_size);
 try_map:
         r = qemu_vfio_dma_map(s->vfio,
                               qiov->iov[i].iov_base,
-                              qiov->iov[i].iov_len,
-                              true, &iova);
+                              len, true, &iova);
         if (r == -ENOMEM && retry) {
             retry = false;
             trace_nvme_dma_flush_queue_wait(s);
@@ -1166,8 +1167,9 @@ static inline bool nvme_qiov_aligned(BlockDriverState *bs,
     BDRVNVMeState *s = bs->opaque;
 
     for (i = 0; i < qiov->niov; ++i) {
-        if (!QEMU_PTR_IS_ALIGNED(qiov->iov[i].iov_base, s->page_size) ||
-            !QEMU_IS_ALIGNED(qiov->iov[i].iov_len, s->page_size)) {
+        if (!QEMU_PTR_IS_ALIGNED(qiov->iov[i].iov_base,
+                                 qemu_real_host_page_size) ||
+            !QEMU_IS_ALIGNED(qiov->iov[i].iov_len, qemu_real_host_page_size)) {
             trace_nvme_qiov_unaligned(qiov, i, qiov->iov[i].iov_base,
                                       qiov->iov[i].iov_len, s->page_size);
             return false;
@@ -1183,7 +1185,7 @@ static int nvme_co_prw(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     int r;
     uint8_t *buf = NULL;
     QEMUIOVector local_qiov;
-
+    size_t len = QEMU_ALIGN_UP(bytes, qemu_real_host_page_size);
     assert(QEMU_IS_ALIGNED(offset, s->page_size));
     assert(QEMU_IS_ALIGNED(bytes, s->page_size));
     assert(bytes <= s->max_transfer);
@@ -1193,7 +1195,7 @@ static int nvme_co_prw(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     }
     s->stats.unaligned_accesses++;
     trace_nvme_prw_buffered(s, offset, bytes, qiov->niov, is_write);
-    buf = qemu_try_memalign(s->page_size, bytes);
+    buf = qemu_try_memalign(qemu_real_host_page_size, len);
 
     if (!buf) {
         return -ENOMEM;
-- 
2.26.2

Re: [PATCH 24/25] block/nvme: Align iov's va and size on host page size

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:46PM +0100, Philippe Mathieu-Daudé wrote:
> From: Eric Auger <eric.auger@redhat.com>
> 
> Make sure iov's va and size are properly aligned on the
> host page size.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)

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

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

[RFC PATCH 25/25] block/nvme: Fix use of write-only doorbells page on Aarch64 arch

[Philippe Mathieu-Daudé]

qemu_vfio_pci_map_bar() calls mmap(), and mmap(2) states:

  'offset' must be a multiple of the page size as returned
   by sysconf(_SC_PAGE_SIZE).

In commit f68453237b9 we started to use an offset of 4K which
broke this contract on Aarch64 arch.

Fix by mapping at offset 0, and and accessing doorbells at offset=4K.

Fixes: f68453237b9 ("block/nvme: Map doorbells pages write-only")
Reported-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index c1c52bae44f..ff645eefe6a 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -94,6 +94,7 @@ typedef struct {
 struct BDRVNVMeState {
     AioContext *aio_context;
     QEMUVFIOState *vfio;
+    void *bar0_wo_map;
     /* Memory mapped registers */
     volatile struct {
         uint32_t sq_tail;
@@ -778,8 +779,10 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
         }
     }
 
-    s->doorbells = qemu_vfio_pci_map_bar(s->vfio, 0, sizeof(NvmeBar),
-                                         NVME_DOORBELL_SIZE, PROT_WRITE, errp);
+    s->bar0_wo_map = qemu_vfio_pci_map_bar(s->vfio, 0, 0,
+                                           sizeof(NvmeBar) + NVME_DOORBELL_SIZE,
+                                           PROT_WRITE, errp);
+    s->doorbells = (void *)((uintptr_t)s->bar0_wo_map + sizeof(NvmeBar));
     if (!s->doorbells) {
         ret = -EINVAL;
         goto out;
@@ -913,8 +916,8 @@ static void nvme_close(BlockDriverState *bs)
                            &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
                            false, NULL, NULL);
     event_notifier_cleanup(&s->irq_notifier[MSIX_SHARED_IRQ_IDX]);
-    qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->doorbells,
-                            sizeof(NvmeBar), NVME_DOORBELL_SIZE);
+    qemu_vfio_pci_unmap_bar(s->vfio, 0, s->bar0_wo_map,
+                            0, sizeof(NvmeBar) + NVME_DOORBELL_SIZE);
     qemu_vfio_close(s->vfio);
 
     g_free(s->device);
-- 
2.26.2

Re: [RFC PATCH 25/25] block/nvme: Fix use of write-only doorbells page on Aarch64 arch

[Stefan Hajnoczi]

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

On Tue, Oct 27, 2020 at 02:55:47PM +0100, Philippe Mathieu-Daudé wrote:
> qemu_vfio_pci_map_bar() calls mmap(), and mmap(2) states:
> 
>   'offset' must be a multiple of the page size as returned
>    by sysconf(_SC_PAGE_SIZE).
> 
> In commit f68453237b9 we started to use an offset of 4K which
> broke this contract on Aarch64 arch.
> 
> Fix by mapping at offset 0, and and accessing doorbells at offset=4K.
> 
> Fixes: f68453237b9 ("block/nvme: Map doorbells pages write-only")
> Reported-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

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

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

Re: [RFC PATCH 25/25] block/nvme: Fix use of write-only doorbells page on Aarch64 arch

[Auger Eric]

Hi,

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> qemu_vfio_pci_map_bar() calls mmap(), and mmap(2) states:
> 
>   'offset' must be a multiple of the page size as returned
>    by sysconf(_SC_PAGE_SIZE).
> 
> In commit f68453237b9 we started to use an offset of 4K which
> broke this contract on Aarch64 arch.
> 
> Fix by mapping at offset 0, and and accessing doorbells at offset=4K.
> 
> Fixes: f68453237b9 ("block/nvme: Map doorbells pages write-only")
> Reported-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

> ---
>  block/nvme.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index c1c52bae44f..ff645eefe6a 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -94,6 +94,7 @@ typedef struct {
>  struct BDRVNVMeState {
>      AioContext *aio_context;
>      QEMUVFIOState *vfio;
> +    void *bar0_wo_map;
>      /* Memory mapped registers */
>      volatile struct {
>          uint32_t sq_tail;
> @@ -778,8 +779,10 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>          }
>      }
>  
> -    s->doorbells = qemu_vfio_pci_map_bar(s->vfio, 0, sizeof(NvmeBar),
> -                                         NVME_DOORBELL_SIZE, PROT_WRITE, errp);
> +    s->bar0_wo_map = qemu_vfio_pci_map_bar(s->vfio, 0, 0,
> +                                           sizeof(NvmeBar) + NVME_DOORBELL_SIZE,
> +                                           PROT_WRITE, errp);
> +    s->doorbells = (void *)((uintptr_t)s->bar0_wo_map + sizeof(NvmeBar));
>      if (!s->doorbells) {
>          ret = -EINVAL;
>          goto out;
> @@ -913,8 +916,8 @@ static void nvme_close(BlockDriverState *bs)
>                             &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
>                             false, NULL, NULL);
>      event_notifier_cleanup(&s->irq_notifier[MSIX_SHARED_IRQ_IDX]);
> -    qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->doorbells,
> -                            sizeof(NvmeBar), NVME_DOORBELL_SIZE);
> +    qemu_vfio_pci_unmap_bar(s->vfio, 0, s->bar0_wo_map,
> +                            0, sizeof(NvmeBar) + NVME_DOORBELL_SIZE);
>      qemu_vfio_close(s->vfio);
>  
>      g_free(s->device);
> 

Re: [PATCH 00/25] block/nvme: Fix Aarch64 host

[Auger Eric]

Hi Philippe,

On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> Add a bit of tracing, clean around to finally fix few bugs.
> In particular, restore NVMe on Aarch64 host.
> 
> Eric Auger (4):
>   block/nvme: Change size and alignment of IDENTIFY response buffer
>   block/nvme: Change size and alignment of queue
>   block/nvme: Change size and alignment of prp_list_pages
>   block/nvme: Align iov's va and size on host page size>
> Philippe Mathieu-Daudé (21):
>   MAINTAINERS: Cover 'block/nvme.h' file
>   block/nvme: Use hex format to display offset in trace events
>   block/nvme: Report warning with warn_report()
>   block/nvme: Trace controller capabilities
>   block/nvme: Trace nvme_poll_queue() per queue
>   block/nvme: Improve nvme_free_req_queue_wait() trace information
>   block/nvme: Trace queue pair creation/deletion
>   block/nvme: Simplify device reset
>   block/nvme: Move definitions before structure declarations
>   block/nvme: Use unsigned integer for queue counter/size
>   block/nvme: Make nvme_identify() return boolean indicating error
>   block/nvme: Make nvme_init_queue() return boolean indicating error
>   block/nvme: Introduce Completion Queue definitions
>   block/nvme: Use definitions instead of magic values in add_io_queue()
>   block/nvme: Correctly initialize Admin Queue Attributes
>   block/nvme: Simplify ADMIN queue access
>   block/nvme: Simplify nvme_cmd_sync()
>   block/nvme: Pass AioContext argument to nvme_add_io_queue()
>   block/nvme: Set request_alignment at initialization
>   block/nvme: Correct minimum device page size
>   block/nvme: Fix use of write-only doorbells page on Aarch64 arch
> 
>  include/block/nvme.h |  17 ++--
>  block/nvme.c         | 208 ++++++++++++++++++++++++-------------------
>  MAINTAINERS          |   2 +
>  block/trace-events   |  30 ++++---
>  4 files changed, 148 insertions(+), 109 deletions(-)
> 

I have tested the series on ARM with both 4kB and 64kB pages and it
works for me.

Feel free to add:
Tested-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

Re: [PATCH 00/25] block/nvme: Fix Aarch64 host

[Philippe Mathieu-Daudé]

On 10/28/20 7:10 PM, Auger Eric wrote:
> Hi Philippe,
> 
> On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
>> Add a bit of tracing, clean around to finally fix few bugs.
>> In particular, restore NVMe on Aarch64 host.
>>
>> Eric Auger (4):
>>   block/nvme: Change size and alignment of IDENTIFY response buffer
>>   block/nvme: Change size and alignment of queue
>>   block/nvme: Change size and alignment of prp_list_pages
>>   block/nvme: Align iov's va and size on host page size>
>> Philippe Mathieu-Daudé (21):
>>   MAINTAINERS: Cover 'block/nvme.h' file
>>   block/nvme: Use hex format to display offset in trace events
>>   block/nvme: Report warning with warn_report()
>>   block/nvme: Trace controller capabilities
>>   block/nvme: Trace nvme_poll_queue() per queue
>>   block/nvme: Improve nvme_free_req_queue_wait() trace information
>>   block/nvme: Trace queue pair creation/deletion
>>   block/nvme: Simplify device reset
>>   block/nvme: Move definitions before structure declarations
>>   block/nvme: Use unsigned integer for queue counter/size
>>   block/nvme: Make nvme_identify() return boolean indicating error
>>   block/nvme: Make nvme_init_queue() return boolean indicating error
>>   block/nvme: Introduce Completion Queue definitions
>>   block/nvme: Use definitions instead of magic values in add_io_queue()
>>   block/nvme: Correctly initialize Admin Queue Attributes
>>   block/nvme: Simplify ADMIN queue access
>>   block/nvme: Simplify nvme_cmd_sync()
>>   block/nvme: Pass AioContext argument to nvme_add_io_queue()
>>   block/nvme: Set request_alignment at initialization
>>   block/nvme: Correct minimum device page size
>>   block/nvme: Fix use of write-only doorbells page on Aarch64 arch
>>
>>  include/block/nvme.h |  17 ++--
>>  block/nvme.c         | 208 ++++++++++++++++++++++++-------------------
>>  MAINTAINERS          |   2 +
>>  block/trace-events   |  30 ++++---
>>  4 files changed, 148 insertions(+), 109 deletions(-)
>>
> 
> I have tested the series on ARM with both 4kB and 64kB pages and it
> works for me.
> 
> Feel free to add:
> Tested-by: Eric Auger <eric.auger@redhat.com>

Thanks!

Phil.