[v2,3/3] block/nvme: Use QEMU PCI MMIO API

Commit Message

Farhan Ali March 28, 2025, 7:06 p.m. UTC

Use the QEMU PCI MMIO functions to read/write
to NVMe registers, rather than directly accessing
them.

Signed-off-by: Farhan Ali <[e-mail]>
---
 block/nvme.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

Comments

Philippe Mathieu-Daudé March 28, 2025, 8:41 p.m. UTC | #1

On 28/3/25 20:06, Farhan Ali wrote:
> Use the QEMU PCI MMIO functions to read/write
> to NVMe registers, rather than directly accessing
> them.
> 
> Signed-off-by: Farhan Ali <[e-mail]>
> ---
>   block/nvme.c | 37 +++++++++++++++++++++----------------
>   1 file changed, 21 insertions(+), 16 deletions(-)


> @@ -805,16 +807,17 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>       bs->bl.request_alignment = s->page_size;
>       timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);
>   
> -    ver = le32_to_cpu(regs->vs);
> +    ver = qemu_pci_mmio_read_32(&regs->vs);
>       trace_nvme_controller_spec_version(extract32(ver, 16, 16),
>                                          extract32(ver, 8, 8),
>                                          extract32(ver, 0, 8));
>   
>       /* Reset device to get a clean state. */
> -    regs->cc = cpu_to_le32(le32_to_cpu(regs->cc) & 0xFE);
> +    cc = qemu_pci_mmio_read_32(&regs->cc);
> +    qemu_pci_mmio_write_32(&regs->cc, (cc & 0xFE));

Extra parenthesis not needed, otherwise:
Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>

Farhan Ali March 29, 2025, 6:04 a.m. UTC | #2

On 3/28/2025 1:41 PM, Philippe Mathieu-Daudé wrote:
> On 28/3/25 20:06, Farhan Ali wrote:
>> Use the QEMU PCI MMIO functions to read/write
>> to NVMe registers, rather than directly accessing
>> them.
>>
>> Signed-off-by: Farhan Ali <[e-mail]>
>> ---
>>   block/nvme.c | 37 +++++++++++++++++++++----------------
>>   1 file changed, 21 insertions(+), 16 deletions(-)
>
>
>> @@ -805,16 +807,17 @@ static int nvme_init(BlockDriverState *bs, 
>> const char *device, int namespace,
>>       bs->bl.request_alignment = s->page_size;
>>       timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);
>>   -    ver = le32_to_cpu(regs->vs);
>> +    ver = qemu_pci_mmio_read_32(&regs->vs);
>>       trace_nvme_controller_spec_version(extract32(ver, 16, 16),
>>                                          extract32(ver, 8, 8),
>>                                          extract32(ver, 0, 8));
>>         /* Reset device to get a clean state. */
>> -    regs->cc = cpu_to_le32(le32_to_cpu(regs->cc) & 0xFE);
>> +    cc = qemu_pci_mmio_read_32(&regs->cc);
>> +    qemu_pci_mmio_write_32(&regs->cc, (cc & 0xFE));
>
> Extra parenthesis not needed, otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
>
Sure, will fix. Thanks for reviewing!

Stefan Hajnoczi March 31, 2025, 4:58 p.m. UTC | #3

On Fri, Mar 28, 2025 at 12:06:27PM -0700, Farhan Ali wrote:
> Use the QEMU PCI MMIO functions to read/write
> to NVMe registers, rather than directly accessing
> them.
> 
> Signed-off-by: Farhan Ali <[e-mail]>
> ---
>  block/nvme.c | 37 +++++++++++++++++++++----------------
>  1 file changed, 21 insertions(+), 16 deletions(-)

Reviewed-by: Stefan Hajnoczi <[e-mail]>

Patch

diff --git a/block/nvme.c b/block/nvme.c
index bbf7c23dcd..ea932609e6 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -23,6 +23,7 @@ 
 #include "qemu/cutils.h"
 #include "qemu/option.h"
 #include "qemu/memalign.h"
+#include "qemu/pci-mmio.h"
 #include "qemu/vfio-helpers.h"
 #include "block/block-io.h"
 #include "block/block_int.h"
@@ -60,7 +61,7 @@  typedef struct {
     uint8_t  *queue;
     uint64_t iova;
     /* Hardware MMIO register */
-    volatile uint32_t *doorbell;
+    uint32_t *doorbell;
 } NVMeQueue;
 
 typedef struct {
@@ -100,7 +101,7 @@  struct BDRVNVMeState {
     QEMUVFIOState *vfio;
     void *bar0_wo_map;
     /* Memory mapped registers */
-    volatile struct {
+    struct {
         uint32_t sq_tail;
         uint32_t cq_head;
     } *doorbells;
@@ -292,7 +293,7 @@  static void nvme_kick(NVMeQueuePair *q)
     assert(!(q->sq.tail & 0xFF00));
     /* Fence the write to submission queue entry before notifying the device. */
     smp_wmb();
-    *q->sq.doorbell = cpu_to_le32(q->sq.tail);
+    qemu_pci_mmio_write_32(q->sq.doorbell, q->sq.tail);
     q->inflight += q->need_kick;
     q->need_kick = 0;
 }
@@ -441,7 +442,7 @@  static bool nvme_process_completion(NVMeQueuePair *q)
     if (progress) {
         /* Notify the device so it can post more completions. */
         smp_mb_release();
-        *q->cq.doorbell = cpu_to_le32(q->cq.head);
+        qemu_pci_mmio_write_32(q->cq.doorbell, q->cq.head);
         nvme_wake_free_req_locked(q);
     }
 
@@ -460,7 +461,7 @@  static void nvme_process_completion_bh(void *opaque)
      * so notify the device that it has space to fill in more completions now.
      */
     smp_mb_release();
-    *q->cq.doorbell = cpu_to_le32(q->cq.head);
+    qemu_pci_mmio_write_32(q->cq.doorbell, q->cq.head);
     nvme_wake_free_req_locked(q);
 
     nvme_process_completion(q);
@@ -749,9 +750,10 @@  static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     int ret;
     uint64_t cap;
     uint32_t ver;
+    uint32_t cc;
     uint64_t timeout_ms;
     uint64_t deadline, now;
-    volatile NvmeBar *regs = NULL;
+    NvmeBar *regs = NULL;
 
     qemu_co_mutex_init(&s->dma_map_lock);
     qemu_co_queue_init(&s->dma_flush_queue);
@@ -779,7 +781,7 @@  static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     /* Perform initialize sequence as described in NVMe spec "7.6.1
      * Initialization". */
 
-    cap = le64_to_cpu(regs->cap);
+    cap = qemu_pci_mmio_read_64(&regs->cap);
     trace_nvme_controller_capability_raw(cap);
     trace_nvme_controller_capability("Maximum Queue Entries Supported",
                                      1 + NVME_CAP_MQES(cap));
@@ -805,16 +807,17 @@  static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     bs->bl.request_alignment = s->page_size;
     timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);
 
-    ver = le32_to_cpu(regs->vs);
+    ver = qemu_pci_mmio_read_32(&regs->vs);
     trace_nvme_controller_spec_version(extract32(ver, 16, 16),
                                        extract32(ver, 8, 8),
                                        extract32(ver, 0, 8));
 
     /* Reset device to get a clean state. */
-    regs->cc = cpu_to_le32(le32_to_cpu(regs->cc) & 0xFE);
+    cc = qemu_pci_mmio_read_32(&regs->cc);
+    qemu_pci_mmio_write_32(&regs->cc, (cc & 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))) {
+    while (NVME_CSTS_RDY(qemu_pci_mmio_read_32(&regs->csts))) {
         if (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) > deadline) {
             error_setg(errp, "Timeout while waiting for device to reset (%"
                              PRId64 " ms)",
@@ -843,19 +846,21 @@  static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     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(q->sq.iova);
-    regs->acq = cpu_to_le64(q->cq.iova);
+    qemu_pci_mmio_write_32(&regs->aqa,
+                        ((NVME_QUEUE_SIZE - 1) << AQA_ACQS_SHIFT) |
+                        ((NVME_QUEUE_SIZE - 1) << AQA_ASQS_SHIFT));
+    qemu_pci_mmio_write_64(&regs->asq, q->sq.iova);
+    qemu_pci_mmio_write_64(&regs->acq, 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) |
+    qemu_pci_mmio_write_32(&regs->cc,
+                           (ctz32(NVME_CQ_ENTRY_BYTES) << CC_IOCQES_SHIFT) |
                            (ctz32(NVME_SQ_ENTRY_BYTES) << CC_IOSQES_SHIFT) |
                            CC_EN_MASK);
     /* Wait for CSTS.RDY = 1. */
     now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     deadline = now + timeout_ms * SCALE_MS;
-    while (!NVME_CSTS_RDY(le32_to_cpu(regs->csts))) {
+    while (!NVME_CSTS_RDY(qemu_pci_mmio_read_32(&regs->csts))) {
         if (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) > deadline) {
             error_setg(errp, "Timeout while waiting for device to start (%"
                              PRId64 " ms)",