Re: [PATCH v6 31/42] nvme: add check for prinfo

[Klaus Birkelund Jensen]

On Mar 25 12:57, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen <address@hidden>
> > 
> > Check the validity of the PRINFO field.
> > 
> > Signed-off-by: Klaus Jensen <address@hidden>
> > ---
> >  hw/block/nvme.c       | 50 ++++++++++++++++++++++++++++++++++++-------
> >  hw/block/trace-events |  1 +
> >  include/block/nvme.h  |  1 +
> >  3 files changed, 44 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 7d5340c272c6..0d2b5b45b0c5 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -505,6 +505,17 @@ static inline uint16_t nvme_check_mdts(NvmeCtrl *n, 
> > size_t len,
> >      return NVME_SUCCESS;
> >  }
> >  
> > +static inline uint16_t nvme_check_prinfo(NvmeCtrl *n, NvmeNamespace *ns,
> > +                                         uint16_t ctrl, NvmeRequest *req)
> > +{
> > +    if ((ctrl & NVME_RW_PRINFO_PRACT) && !(ns->id_ns.dps & DPS_TYPE_MASK)) 
> > {
> > +        trace_nvme_dev_err_prinfo(nvme_cid(req), ctrl);
> > +        return NVME_INVALID_FIELD | NVME_DNR;
> > +    }
> 
> I refreshed my (still very limited) knowelege on the metadata
> and the protection info, and this is what I found:
> 
> I think that this is very far from complete, because we also have:
> 
> 1. PRCHECK. According to the spec it is independent of PRACT
>    And when some of it is set, 
>    together with enabled protection (the DPS field in namespace),
>    Then the 8 bytes of the protection info is checked (optionally using the
>    the EILBRT and ELBAT/ELBATM fields in the command and CRC of the data for 
> the guard field)
> 
>    So this field should also be checked to be zero when protection is disabled
>    (I don't see an explicit requirement for that in the spec, but neither I 
> see
>    such requirement for PRACT)
> 
> 2. The protection values to be written / checked ((E)ILBRT/(E)LBATM/(E)LBAT)
>    Same here, but also these should not be set when PRCHECK is not set for 
> reads,
>    plus some are protection type specific.
> 
> 
> The spec does mention the 'Invalid Protection Information' error code which
> refers to invalid values in the PRINFO field.
> So this error code I think should be returned instead of the 'Invalid field'
> 
> Another thing to optionaly check is that the metadata pointer for separate 
> metadata.
>  Is zero as long as we don't support metadata
> (again I don't see an explicit requirement for this in the spec, but it 
> mentions:
> 
> "This field is valid only if the command has metadata that is not interleaved 
> with
> the logical block data, as specified in the Format NVM command"
> 
> )
> 

I'm kinda inclined to just drop this patch. The spec actually says that
the PRACT and PRCHK fields are used only if the namespace is formatted
to use end-to-end protection information. Since we do not support that,
I don't think we even need to check it.

Any opinion on this?