> On Jun 15, 2022, at 6:11 PM, John Levon <le...@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
