Re: [PATCHv2 7/7] nvme: add support for enhanced metadata

[Keith Busch]

On Wed, Feb 02, 2022 at 02:28:53PM +0100, Hannes Reinecke wrote:
> On 2/1/22 20:01, Keith Busch wrote:
> > +			integrity.flags |= BLK_INTEGRITY_DEVICE_CAPABLE;
> > +			break;
> > +		case NVME_NVM_NS_64B_GUARD:
> > +			integrity.profile = &nvme_pi_type1_crc64

I just noticed this should be type3, not type1...

> > +			integrity.tag_size = sizeof(u16) + 6;
> > +			integrity.flags |= BLK_INTEGRITY_DEVICE_CAPABLE;
> > +			break;
> > +		default:
> > +			integrity.profile = NULL;
> > +			break;
> > +		}
> >   		break;
> >   	case NVME_NS_DPS_PI_TYPE1:
> >   	case NVME_NS_DPS_PI_TYPE2:
> > -		integrity.profile = &t10_pi_type1_crc;
> > -		integrity.tag_size = sizeof(u16);
> > -		integrity.flags |= BLK_INTEGRITY_DEVICE_CAPABLE;
> > +		switch (ns->guard_type) {
> > +		case NVME_NVM_NS_16B_GUARD:
> > +			integrity.profile = &t10_pi_type1_crc;
> > +			integrity.tag_size = sizeof(u16);
> > +			integrity.flags |= BLK_INTEGRITY_DEVICE_CAPABLE;
> > +			break;
> > +		case NVME_NVM_NS_64B_GUARD:
> > +			integrity.profile = &nvme_pi_type1_crc64;
> > +			integrity.tag_size = sizeof(u16);
> 
> Is that correct? Shouldn't it be '8' like in the above case?

For type1 and 2, I believe tag_size refers to the "application" tag,
which is 2 bytes here.

The reason it is 8 bytes for type3 is because there is no ref tag, so
that portion of the metadata becomes part of the opaque application
tag_size.

> > @@ -3104,7 +3218,7 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
> >   	if (ret < 0)
> >   		return ret;
> > -	ret = nvme_configure_acre(ctrl);
> > +	ret = nvme_configure_host_options(ctrl);
> >   	if (ret < 0)
> >   		return ret;
> 
> This could be made into a separate patch, is it's not directly related to PI
> support.

Well, the driver can't read the new PI formats without enabling host
supported features for it. Enabling the feature tells the controller
we're going to check for it, so I don't think we could reasonably split
this part into a prep patch from the part that sets up the PI formats.