On Tue, Jun 14, 2022 at 03:24:37PM +0800, Jinhao Fan wrote: > > On Jun 14, 2022, at 5:15 AM, Keith Busch <kbu...@kernel.org> wrote: > > @@ -6538,9 +6544,25 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr > > addr, int val) > > > > trace_pci_nvme_mmio_doorbell_sq(sq->sqid, new_tail); > > > > - if (!sq->db_addr) { > > sq->tail = new_tail; > > + if (sq->db_addr) { > > + /* > > + * The spec states "the host shall also update the controller's > > + * corresponding doorbell property to match the value of that > > entry > > + * in the Shadow Doorbell buffer." > > + * > > + * Since this context is currently a VM trap, we can safely > > enforce > > + * the requirement from the device side in case the host is > > + * misbehaving. > > + * > > + * Note, we shouldn't have to do this, but various drivers > > + * including ones that run on Linux, are not updating Admin > > Queues, > > + * so we can't trust reading it for an appropriate sq tail. > > + */ > > + pci_dma_write(&n->parent_obj, sq->db_addr, &sq->tail, > > + sizeof(sq->tail)); > > } > > + > > timer_mod(sq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500); > > } > > } > > -- > > Thanks Keith, > > This is an interesting hack. I wonder how should I incorporate your changes > in my patch. I guess I can modify the code in PATCH 1/2 and add a > “Proposed-by” tag. Is this the correct way?
It's a pretty nasty hack, and definitely not in compliance with the spec: the db_addr is supposed to be read-only from the device side, though I do think it's safe for this environment. Unless Klaus or anyone finds something I'm missing, I feel this is an acceptable compromise to address this odd discrepency. I believe the recommended tag for something like this is "Suggested-by:", but no need to credit me. Just fold it into your first patch and send a v2. By the way, I noticed that the patch never updates the cq's ei_addr value. Is that on purpose?