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

[Klaus Jensen]

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.

signature.asc
Description: PGP signature