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

[Klaus Jensen <its@irrelevant.dk>]

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

On May 31 13:13, Klaus Jensen wrote:
> On Apr 20 14:48, Klaus Jensen wrote:
> > On Apr 20 15:31, Dmitry Tikhov wrote:
> > > On Wed, Apr 20, 2022 at 12:54:44, Klaus Jensen wrote:
> > > > 
> > > > NVM Command Set Specification v1.0b, Section 5.2.3. It is exactly what
> > > > you quoted above.
> > > > 
> > > > I think you are interpreting
> > > > 
> > > >   "If a command is aborted as a result of the Reference Tag Check bit of
> > > >   the PRCHK field being set to '1', ..."
> > > > 
> > > > as
> > > > 
> > > >    "If a command is aborted *because* the Reference Tag Check bit of the
> > > >    PRCHK field being set to '1', ...".
> > > Yeah, i was interpreting it exactly this way.
> > > 
> > > > 
> > > > But that is not what it is saying. IMO, the only meaningful
> > > > interpretation is that "If the command is aborted *as a result of* the
> > > > check being done *because* the bit is set, *then* return an error".
> > > Ok, but return error in this context still means to return either
> > > Invalid Protection Information or Invalid Field in Command, isn't it?
> > > Or why would it specify
> > >     ...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
> > > exactly this 2 status codes?
> > > 
> > > > 
> > > > Your interpretation would break existing hosts that set the bit.
> > > 
> > > I also opened NVM Express 1.4 "8.3.1.5 Control of Protection Information
> > > Checking - PRCHK" and it says
> > >     For Type 3 protection, if bit 0 of the PRCHK field is set to ‘1’, then
> > > 	the command should be aborted with status Invalid Protection
> > > 	Information, but may be aborted with status Invalid Field in Command.
> > > 	The controller may ignore the ILBRT and EILBRT fields when Type 3
> > > 	protection is used because the computed reference tag remains
> > > 	unchanged.
> > > I think it marks clear intent to abort cmd with "Invalid Protection
> > > Information" or "Invalid Field in Command" status codes exactly in case
> > > reftag check bit is set. Also isn't "may ignore the ILBRT and EILBRT 
> > > fields" means not to compare reftag with ILBRT/EILBRT? If it is not 
> > > compared then reftag check error can't be returned.
> > 
> > What the heck. This is a pretty major difference between v1.4 and v1.4b.
> > v1.4b does not include that wording (but it *is* present in v1.3d). You
> > are absolutely right that this conveys the intent to abort the command.
> > Looks like this was lost in the changes in that section between v1.4 and
> > v1.4b. This explains the wording in v2.0 - the spec people realized they
> > screwed up and now they have to accept both behaviors.
> > 
> > > 
> > > But anyways, spec says that "should" and "may" indicates flexibility of
> > > choice and not mandatory behavior. So if you think that current behavior
> > > is right i don't insist.
> > 
> > I'm not so sure now. Another question for the spec people... I'll get
> > back to you.
> 
> I got a long an exhaustive description of this issue from the spec
> people, and it all boils down to, well, a mistake basically.
> 
> The bottom line is that both behaviors *are* acceptable as of now, but
> this may change. Not sure how ;) However, I think this might be brough
> up with the NVMe TWG, and I'll make sure to follow that discussion.
> 
> For now, I think we leave the behavior of *this* device as-is. It's not
> that I think anyone really relies on this behavior, but better not to
> break it as long as we report v1.4.

Sigh. I just got a correction on that email and the intention *is* to
remove this. So, I'll queue this up so QEMU can be a front-runner for
compliance ;)

Sorry for the noise.

Thanks, applied to nvme-next!

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