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

[Klaus Jensen <its@irrelevant.dk>]

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

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.

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