On Mar 25 12:57, Maxim Levitsky wrote: > On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote: > > From: Klaus Jensen <k.jen...@samsung.com> > > > > Check the validity of the PRINFO field. > > > > Signed-off-by: Klaus Jensen <k.jen...@samsung.com> > > --- > > 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?