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

[Klaus Jensen <its@irrelevant.dk>]

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

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.

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