shadow doorbell on admin queue

shadow doorbell on admin queue

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

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.


  [1]: https://github.com/spdk/spdk/issues/2147

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: shadow doorbell on admin queue

[Keith Busch]

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.

Re: shadow doorbell on admin queue

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1930 bytes --]

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!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]