On Jul 14 08:07, Keith Busch wrote:
> On Thu, Jul 14, 2022 at 10:50:56AM +0200, Klaus Jensen wrote:
> > Hi,
> > 
> > Jinhao, our QEMU GSoC intern is working on nvme performance improvements
> > and while implementing shadow doorbells an interesting issue came up.
> > 
> > The spec states that when enabling shadow doorbells, it is for all
> > queues (including the admin queue). However, the kernel will currently
> > not update the shadow doorbell for the admin queue, which causes trouble
> > if the device expects it to.
> > 
> > The kernel is not the only driver doing this - SPDK doesn't update it
> > either[1]. At least one virtual target implementing shadow doorbells
> > (SPDK's vfio-user target) jumped on the band-wagon and simply expects
> > the driver to not update it. In QEMU, Keith came up with a hack to
> > update the shadow doorbell from the device side, allowing both compliant
> > and non-compliant drivers to work. Just fixing it on the driver side is
> > a problem, because it will break targets that expects the non-compliant
> > behavior (i.e. always expecting mmio on the admin queue).
> > 
> > Question is if the kernel even wants to do anything about this at all? I
> > kinda already know the answer here - "spec is screwed up", but I wanted
> > to raise the issue here for feedback prior to potentially starting a
> > process with the NVMe TWG to sort out there.
> 
> The driver has been this way forever, so either (a) no one was actually using
> this feature, or (b) every target implementing shadow doorbells has the same
> non-spec implementation. Either way, we can't very well change it now, and it
> looks like shadows can't reliably be used on a live queue anyway. I think you'd
> have to refine this feature with the TWG.

I agree - initializing the shadow values on the admin queue is a little
wonky. I'll bring this up with my reps in the TWG.

Thanks!