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

From: Klaus Jensen <its@irrelevant.dk>
To: Dmitry Tikhov <d.tihov@yadro.com>
Cc: kbusch@kernel.org, ddtikhov@gmail.com, qemu-devel@nongnu.org,
	qemu-block@nongnu.org, linux@yadro.com
Subject: Re: [PATCH] hw/nvme: add new command abort case
Date: Wed, 20 Apr 2022 12:13:05 +0200	[thread overview]
Message-ID: <Yl/csehng+W0gfQD@apples> (raw)
In-Reply-To: <20220420082044.n6orslk2aukj2jai@localhost.localdomain>

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

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.

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