Re: [PATCH] hw/nvme: add new command abort case

[Dmitry Tikhov]

On Wed, Apr 20, 2022 at 12:36:54, Klaus Jensen wrote:
> On Apr 20 12:13, Klaus Jensen wrote:
> > On Apr 20 11:20, Dmitry Tikhov wrote:
> > > NVMe command set specification for end-to-end data protection formatted
> > > namespace states:
> > > 
> > >     o If the Reference Tag Check bit of the PRCHK field is set to ‘1’ and
> > >       the namespace is formatted for Type 3 protection, then the
> > >       controller:
> > >           ▪ should not compare the protection Information Reference Tag
> > >             field to the computed reference tag; and
> > >           ▪ may ignore the ILBRT and EILBRT fields. If a command is
> > >             aborted as a result of the Reference Tag Check bit of the
> > >             PRCHK field being set to ‘1’, then that command should be
> > >             aborted with a status code of Invalid Protection Information,
> > >             but may be aborted with a status code of Invalid Field in
> > >             Command.
> > > 
> > > Currently qemu compares reftag in the nvme_dif_prchk function whenever
> > > Reference Tag Check bit is set in the command. For type 3 namespaces
> > > however, caller of nvme_dif_prchk - nvme_dif_check does not increment
> > > reftag for each subsequent logical block. That way commands incorporating
> > > more than one logical block for type 3 formatted namespaces with reftag
> > > check bit set, always fail with End-to-end Reference Tag Check Error.
> > > Comply with spec by handling case of set Reference Tag Check
> > > bit in the type 3 formatted namespace.
> > > 
> > 
> > Note the "should" and "may" in your quote. What QEMU does right now is
> > compliant with v1.4. That is, the reftag must NOT be incremented
> > - it is the same for the first and all subsequent logical blocks.
> > 
> > I'm a bit hesitant to follow v2.0 here, since we do not report v2.0
> > compliance yet. I'm honestly also a bit perplexed as to how the NVMe TWG
> > ended up considering this backwards compatible. As far as I can tell
> > this breaks current hosts that do set the reference tag check bit,
> > provides a valid ILBRT/EILBRT and expects it to succeed.
> 
> Ok, so reading the spec more closely...
> 
> The Invalid Protection Information should not be set just because the
> reference tag check bit is set. It should be set *if* the controller
> chooses to check it and it fails. However, in v2.0, the controller is
> allowed to ignore it and not perform the check.
> 
> So, just because the host sets the bit, that does not mean we fail the
> command. However, a difference is that a v2.0 compliant controller
> should return Invalid Protection Information or Invalid Field in Command
> instead of End-to-end Reference Tag Check Error if the check fails.

Can you please link the spec you are referring to?