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

[Keith Busch]

On Tue, Jun 14, 2022 at 03:24:37PM +0800, Jinhao Fan wrote:
> > On Jun 14, 2022, at 5:15 AM, Keith Busch <kbusch@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?