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

[Klaus Jensen <its@irrelevant.dk>]

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

On Jun  9 16:52, John Levon wrote:
> On Thu, Jun 09, 2022 at 08:29:30AM -0600, Keith Busch wrote:
> 
> > On Wed, Jun 08, 2022 at 10:55:30PM +0200, Klaus Jensen wrote:
> > > 
> > > Keith, is this a bug in the kernel? If the code here would expect the
> > > doorbell buffer to be updated for the admin queue as well, would we
> > > break?
> > 
> > The admin queue has to be created before db buffer can be set up, so we can't
> > rely on it. And this is a performance feature. Admin commands have never been
> > considered performant, so we decided not to consider it though the spec allows
> > it.

It's not really a question of whether or not the spec *allows* it. If
our device chooses to be compliant here, then it will read invalid
doorbell values if drivers doesnt update the admin doorbell buffer.

> 
> It's not just unnecessary, but enabling shadow doorbells on admin queues will
> actively break device implementations (such as SPDK), which have had to presume
> that driver implementations don't use shadow doorbells for the admin queue.
> 

I'm ok with following the concensus here, but we all agree that this is
a blatant spec violation that ended up manifesting itself down the
stack, right?

So... if QEMU wants to be compliant here, I guess we could ask the
kernel to introduce a quirk for *compliant* controllers. Now, THAT would
be a first! Not sure if I am being serious or not here ;)

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