Re: [PATCH 1/2] hw/nvme: Implement shadow doorbell buffer support

[Jinhao Fan]

> On Jun 15, 2022, at 6:11 PM, John Levon <levon@movementarian.org> wrote:
> 
> On Wed, Jun 15, 2022 at 11:33:02AM +0200, Klaus Jensen wrote:
> 
>>> BTW I'm surprised that this patch has just this:
>>> 
>>> +static void nvme_update_sq_eventidx(const NvmeSQueue *sq)
>>> +{
>>> +    pci_dma_write(&sq->ctrl->parent_obj, sq->ei_addr, &sq->tail,
>>> +                  sizeof(sq->tail));
>>> +}
>>> 
>>> Isn't this racy against the driver? Compare
>>> https://github.com/spdk/spdk/blob/master/lib/nvmf/vfio_user.c#L1317
>>> 
>>> thanks
>>> john
>> 
>> QEMU has full memory barriers on dma read/write, so I believe this is
>> safe?
> 
> But don't you need to re-read the tail still, for example:


Hi John,

I think we also have a check for concurrent update on the tail. After writing 
eventidx, we read the tail again. It is here:

@@ -5854,6 +5943,11 @@ static void nvme_process_sq(void *opaque)
             req->status = status;
             nvme_enqueue_req_completion(cq, req);
         }
+
+        if (n->dbbuf_enabled) {
+            nvme_update_sq_eventidx(sq);
+            nvme_update_sq_tail(sq);
+        }

> 
> 
> driver                        device
> 
>                       eventidx is 3
> 
> write 4 to tail
>                       read tail of 4
> write 5 to tail
> read eventidx of 3
> nvme_dbbuf_need_event (1)
> 
>                       set eventidx to 4

Therefore, at this point, we read the tail of 5.

>                       go to sleep
> 
> at (1), our tail update of 4->5 doesn't straddle the eventidx, so we don't 
> send
> any MMIO, and the device won't wake up. This is why the above code checks the
> tail twice for any concurrent update.

Thanks,
Jinhao Fan

> 
> regards
> john