[PATCH 1/4] nvme-fabrics: add discovery controller type

[PATCH 1/4] nvme-fabrics: add discovery controller type

[Martin Belanger]

From: Martin Belanger <martin.belanger@dell.com>

TP8010 introduces the Central Discovery Controller (CDC). To
differentiate between a CDC and a DDC (Direct DC), the identify
response now carries a DCTYPE (Discovery Controller Type) field.

Signed-off-by: Martin Belanger <martin.belanger@dell.com>
---
 drivers/nvme/host/core.c |  3 +++
 drivers/nvme/host/nvme.h |  2 ++
 include/linux/nvme.h     | 10 +++++++++-
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 4fc794d9c2f4..cd34b92e8e9d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2978,6 +2978,7 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ctrl->kas = le16_to_cpu(id->kas);
 	ctrl->max_namespaces = le32_to_cpu(id->mnan);
 	ctrl->ctratt = le32_to_cpu(id->ctratt);
+	ctrl->dctype = id->dctype;
 
 	if (id->rtd3e) {
 		/* us -> s */
@@ -3335,6 +3336,7 @@ nvme_show_int_function(numa_node);
 nvme_show_int_function(queue_count);
 nvme_show_int_function(sqsize);
 nvme_show_int_function(kato);
+nvme_show_int_function(dctype);
 
 static ssize_t nvme_sysfs_delete(struct device *dev,
 				struct device_attribute *attr, const char *buf,
@@ -3533,6 +3535,7 @@ static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_reconnect_delay.attr,
 	&dev_attr_fast_io_fail_tmo.attr,
 	&dev_attr_kato.attr,
+	&dev_attr_dctype.attr,
 	NULL
 };
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index fe224016418e..f8b60f7346fd 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -349,6 +349,8 @@ struct nvme_ctrl {
 	unsigned long discard_page_busy;
 
 	struct nvme_fault_inject fault_inject;
+
+	enum nvme_dctype dctype;
 };
 
 enum nvme_iopolicy {
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 855dd9b3e84b..82567d493c51 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -240,6 +240,12 @@ enum nvme_ctrl_attr {
 	NVME_CTRL_ATTR_TBKAS		= (1 << 6),
 };
 
+enum nvme_dctype {
+	NVME_DCTYPE_NOT_REPORTED = 0,
+	NVME_DCTYPE_DDC = 1,
+	NVME_DCTYPE_CDC = 2,
+};
+
 struct nvme_id_ctrl {
 	__le16			vid;
 	__le16			ssvid;
@@ -320,7 +326,9 @@ struct nvme_id_ctrl {
 	__le16			icdoff;
 	__u8			ctrattr;
 	__u8			msdbd;
-	__u8			rsvd1804[244];
+	__le16			ofcs;
+	__u8			dctype;
+	__u8			rsvd1807[241];
 	struct nvme_id_power_state	psd[32];
 	__u8			vs[1024];
 };
-- 
2.34.1

Re: [PATCH 1/4] nvme-fabrics: add discovery controller type

[Hannes Reinecke]

On 1/25/22 15:59, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> TP8010 introduces the Central Discovery Controller (CDC). To
> differentiate between a CDC and a DDC (Direct DC), the identify
> response now carries a DCTYPE (Discovery Controller Type) field.
> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---
>   drivers/nvme/host/core.c |  3 +++
>   drivers/nvme/host/nvme.h |  2 ++
>   include/linux/nvme.h     | 10 +++++++++-
>   3 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 4fc794d9c2f4..cd34b92e8e9d 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2978,6 +2978,7 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
>   	ctrl->kas = le16_to_cpu(id->kas);
>   	ctrl->max_namespaces = le32_to_cpu(id->mnan);
>   	ctrl->ctratt = le32_to_cpu(id->ctratt);
> +	ctrl->dctype = id->dctype;
>   
>   	if (id->rtd3e) {
>   		/* us -> s */
> @@ -3335,6 +3336,7 @@ nvme_show_int_function(numa_node);
>   nvme_show_int_function(queue_count);
>   nvme_show_int_function(sqsize);
>   nvme_show_int_function(kato);
> +nvme_show_int_function(dctype);
>   
>   static ssize_t nvme_sysfs_delete(struct device *dev,
>   				struct device_attribute *attr, const char *buf,
> @@ -3533,6 +3535,7 @@ static struct attribute *nvme_dev_attrs[] = {
>   	&dev_attr_reconnect_delay.attr,
>   	&dev_attr_fast_io_fail_tmo.attr,
>   	&dev_attr_kato.attr,
> +	&dev_attr_dctype.attr,
>   	NULL
>   };
>   
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index fe224016418e..f8b60f7346fd 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -349,6 +349,8 @@ struct nvme_ctrl {
>   	unsigned long discard_page_busy;
>   
>   	struct nvme_fault_inject fault_inject;
> +
> +	enum nvme_dctype dctype;
>   };
>   
>   enum nvme_iopolicy {
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 855dd9b3e84b..82567d493c51 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -240,6 +240,12 @@ enum nvme_ctrl_attr {
>   	NVME_CTRL_ATTR_TBKAS		= (1 << 6),
>   };
>   
> +enum nvme_dctype {
> +	NVME_DCTYPE_NOT_REPORTED = 0,
> +	NVME_DCTYPE_DDC = 1,
> +	NVME_DCTYPE_CDC = 2,
> +};
> +
>   struct nvme_id_ctrl {
>   	__le16			vid;
>   	__le16			ssvid;
> @@ -320,7 +326,9 @@ struct nvme_id_ctrl {
>   	__le16			icdoff;
>   	__u8			ctrattr;
>   	__u8			msdbd;
> -	__u8			rsvd1804[244];
> +	__le16			ofcs;
> +	__u8			dctype;
> +	__u8			rsvd1807[241];
>   	struct nvme_id_power_state	psd[32];
>   	__u8			vs[1024];
>   };

Please decode the fields for the sysfs attribute; 'cdc' is far easier to 
understand than '2'.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

RE: [PATCH 1/4] nvme-fabrics: add discovery controller type

[Belanger, Martin]

> On 1/25/22 15:59, Martin Belanger wrote:
> > From: Martin Belanger <martin.belanger@dell.com>
> >
> > TP8010 introduces the Central Discovery Controller (CDC). To
> > differentiate between a CDC and a DDC (Direct DC), the identify
> > response now carries a DCTYPE (Discovery Controller Type) field.
> >
> > Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> > ---
> >   drivers/nvme/host/core.c |  3 +++
> >   drivers/nvme/host/nvme.h |  2 ++
> >   include/linux/nvme.h     | 10 +++++++++-
> >   3 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index
> > 4fc794d9c2f4..cd34b92e8e9d 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -2978,6 +2978,7 @@ static int nvme_init_identify(struct nvme_ctrl
> *ctrl)
> >   	ctrl->kas = le16_to_cpu(id->kas);
> >   	ctrl->max_namespaces = le32_to_cpu(id->mnan);
> >   	ctrl->ctratt = le32_to_cpu(id->ctratt);
> > +	ctrl->dctype = id->dctype;
> >
> >   	if (id->rtd3e) {
> >   		/* us -> s */
> > @@ -3335,6 +3336,7 @@ nvme_show_int_function(numa_node);
> >   nvme_show_int_function(queue_count);
> >   nvme_show_int_function(sqsize);
> >   nvme_show_int_function(kato);
> > +nvme_show_int_function(dctype);
> >
> >   static ssize_t nvme_sysfs_delete(struct device *dev,
> >   				struct device_attribute *attr, const char *buf,
> @@ -3533,6
> > +3535,7 @@ static struct attribute *nvme_dev_attrs[] = {
> >   	&dev_attr_reconnect_delay.attr,
> >   	&dev_attr_fast_io_fail_tmo.attr,
> >   	&dev_attr_kato.attr,
> > +	&dev_attr_dctype.attr,
> >   	NULL
> >   };
> >
> > diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index
> > fe224016418e..f8b60f7346fd 100644
> > --- a/drivers/nvme/host/nvme.h
> > +++ b/drivers/nvme/host/nvme.h
> > @@ -349,6 +349,8 @@ struct nvme_ctrl {
> >   	unsigned long discard_page_busy;
> >
> >   	struct nvme_fault_inject fault_inject;
> > +
> > +	enum nvme_dctype dctype;
> >   };
> >
> >   enum nvme_iopolicy {
> > diff --git a/include/linux/nvme.h b/include/linux/nvme.h index
> > 855dd9b3e84b..82567d493c51 100644
> > --- a/include/linux/nvme.h
> > +++ b/include/linux/nvme.h
> > @@ -240,6 +240,12 @@ enum nvme_ctrl_attr {
> >   	NVME_CTRL_ATTR_TBKAS		= (1 << 6),
> >   };
> >
> > +enum nvme_dctype {
> > +	NVME_DCTYPE_NOT_REPORTED = 0,
> > +	NVME_DCTYPE_DDC = 1,
> > +	NVME_DCTYPE_CDC = 2,
> > +};
> > +
> >   struct nvme_id_ctrl {
> >   	__le16			vid;
> >   	__le16			ssvid;
> > @@ -320,7 +326,9 @@ struct nvme_id_ctrl {
> >   	__le16			icdoff;
> >   	__u8			ctrattr;
> >   	__u8			msdbd;
> > -	__u8			rsvd1804[244];
> > +	__le16			ofcs;
> > +	__u8			dctype;
> > +	__u8			rsvd1807[241];
> >   	struct nvme_id_power_state	psd[32];
> >   	__u8			vs[1024];
> >   };
> 
> Please decode the fields for the sysfs attribute; 'cdc' is far easier to
> understand than '2'.

Ok, I will add a decoder, but just to explain why I did it this way....

I agree that 'cdc' is more human-readable than 2. On the other hand,
decoding to a string has the drawback that when (if) the enumeration 
changes, then we need to add more decode code in the kernel.

As you know it takes a while for kernel changes to make it into a distro.
By simply printing the decimal value we never have to make changes 
to the kernel again and any value added to the enum in the future will
just show up.

> 
> Cheers,
> 
> Hannes
> --
> Dr. Hannes Reinecke		           Kernel Storage Architect
> hare@suse.de			                  +49 911 74053 688
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
> HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Internal Use - Confidential

Re: [PATCH 1/4] nvme-fabrics: add discovery controller type

[Sagi Grimberg]

> ---
>   drivers/nvme/host/core.c |  3 +++
>   drivers/nvme/host/nvme.h |  2 ++
>   include/linux/nvme.h     | 10 +++++++++-
>   3 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 4fc794d9c2f4..cd34b92e8e9d 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2978,6 +2978,7 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
>   	ctrl->kas = le16_to_cpu(id->kas);
>   	ctrl->max_namespaces = le32_to_cpu(id->mnan);
>   	ctrl->ctratt = le32_to_cpu(id->ctratt);
> +	ctrl->dctype = id->dctype;
>   
>   	if (id->rtd3e) {
>   		/* us -> s */
> @@ -3335,6 +3336,7 @@ nvme_show_int_function(numa_node);
>   nvme_show_int_function(queue_count);
>   nvme_show_int_function(sqsize);
>   nvme_show_int_function(kato);
> +nvme_show_int_function(dctype);
>   
>   static ssize_t nvme_sysfs_delete(struct device *dev,
>   				struct device_attribute *attr, const char *buf,
> @@ -3533,6 +3535,7 @@ static struct attribute *nvme_dev_attrs[] = {
>   	&dev_attr_reconnect_delay.attr,
>   	&dev_attr_fast_io_fail_tmo.attr,
>   	&dev_attr_kato.attr,
> +	&dev_attr_dctype.attr,
>   	NULL
>   };

This attribute should not be visible to I/O controllers.

>   
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index fe224016418e..f8b60f7346fd 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -349,6 +349,8 @@ struct nvme_ctrl {
>   	unsigned long discard_page_busy;
>   
>   	struct nvme_fault_inject fault_inject;
> +
> +	enum nvme_dctype dctype;
>   };
>   
>   enum nvme_iopolicy {
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 855dd9b3e84b..82567d493c51 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -240,6 +240,12 @@ enum nvme_ctrl_attr {
>   	NVME_CTRL_ATTR_TBKAS		= (1 << 6),
>   };
>   
> +enum nvme_dctype {
> +	NVME_DCTYPE_NOT_REPORTED = 0,
> +	NVME_DCTYPE_DDC = 1,
> +	NVME_DCTYPE_CDC = 2,
> +};
> +
>   struct nvme_id_ctrl {
>   	__le16			vid;
>   	__le16			ssvid;
> @@ -320,7 +326,9 @@ struct nvme_id_ctrl {
>   	__le16			icdoff;
>   	__u8			ctrattr;
>   	__u8			msdbd;
> -	__u8			rsvd1804[244];
> +	__le16			ofcs;

this is unused...

> +	__u8			dctype;
> +	__u8			rsvd1807[241];
>   	struct nvme_id_power_state	psd[32];
>   	__u8			vs[1024];
>   };

[PATCH 2/4] nvme: add host symbolic name

[Martin Belanger]

From: Martin Belanger <martin.belanger@dell.com>

TP8010 introduces the 'host symbolic name' as an optional
parameter for explicit registration with a central discovery
controller.

Signed-off-by: Martin Belanger <martin.belanger@dell.com>
---
 drivers/nvme/host/core.c    | 14 ++++++++++++++
 drivers/nvme/host/fabrics.c | 17 +++++++++++++++--
 drivers/nvme/host/fabrics.h |  2 ++
 include/linux/nvme.h        |  2 ++
 4 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cd34b92e8e9d..cf5d9984f8f6 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3404,6 +3404,16 @@ static ssize_t nvme_sysfs_show_hostnqn(struct device *dev,
 }
 static DEVICE_ATTR(hostnqn, S_IRUGO, nvme_sysfs_show_hostnqn, NULL);
 
+static ssize_t nvme_sysfs_show_hostsymname(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%s\n", ctrl->opts->host->symname);
+}
+static DEVICE_ATTR(hostsymname, S_IRUGO, nvme_sysfs_show_hostsymname, NULL);
+
 static ssize_t nvme_sysfs_show_hostid(struct device *dev,
 					struct device_attribute *attr,
 					char *buf)
@@ -3531,6 +3541,7 @@ static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_sqsize.attr,
 	&dev_attr_hostnqn.attr,
 	&dev_attr_hostid.attr,
+	&dev_attr_hostsymname.attr,
 	&dev_attr_ctrl_loss_tmo.attr,
 	&dev_attr_reconnect_delay.attr,
 	&dev_attr_fast_io_fail_tmo.attr,
@@ -3553,6 +3564,9 @@ static umode_t nvme_dev_attrs_are_visible(struct kobject *kobj,
 		return 0;
 	if (a == &dev_attr_hostid.attr && !ctrl->opts)
 		return 0;
+	if (a == &dev_attr_hostsymname.attr &&
+	    (!ctrl->opts || (ctrl->opts->host->symname[0] == '\0')))
+		return 0;
 	if (a == &dev_attr_ctrl_loss_tmo.attr && !ctrl->opts)
 		return 0;
 	if (a == &dev_attr_reconnect_delay.attr && !ctrl->opts)
diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 7ae041e2b3fb..040a6cce6afa 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -540,6 +540,7 @@ static const match_table_t opt_tokens = {
 	{ NVMF_OPT_HOST_TRADDR,		"host_traddr=%s"	},
 	{ NVMF_OPT_HOST_IFACE,		"host_iface=%s"		},
 	{ NVMF_OPT_HOST_ID,		"hostid=%s"		},
+	{ NVMF_OPT_SYMNAME,		"hostsymname=%s"	},
 	{ NVMF_OPT_DUP_CONNECT,		"duplicate_connect"	},
 	{ NVMF_OPT_DISABLE_SQFLOW,	"disable_sqflow"	},
 	{ NVMF_OPT_HDR_DIGEST,		"hdr_digest"		},
@@ -556,7 +557,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 		const char *buf)
 {
 	substring_t args[MAX_OPT_ARGS];
-	char *options, *o, *p;
+	char *options, *o, *p, *hostsymname = NULL;
 	int token, ret = 0;
 	size_t nqnlen  = 0;
 	int ctrl_loss_tmo = NVMF_DEF_CTRL_LOSS_TMO;
@@ -830,6 +831,13 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 		case NVMF_OPT_DISCOVERY:
 			opts->discovery_nqn = true;
 			break;
+		case NVMF_OPT_SYMNAME:
+			hostsymname = match_strdup(args);
+			if (!hostsymname) {
+				ret = -ENOMEM;
+				goto out;
+			}
+			break;
 		default:
 			pr_warn("unknown parameter or missing value '%s' in ctrl creation request\n",
 				p);
@@ -864,8 +872,13 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 
 	uuid_copy(&opts->host->id, &hostid);
 
+	if (hostsymname)
+		strncpy(opts->host->symname, hostsymname,
+			sizeof(opts->host->symname) - 1);
+
 out:
 	kfree(options);
+	kfree(hostsymname);
 	return ret;
 }
 
@@ -957,7 +970,7 @@ EXPORT_SYMBOL_GPL(nvmf_free_options);
 				 NVMF_OPT_KATO | NVMF_OPT_HOSTNQN | \
 				 NVMF_OPT_HOST_ID | NVMF_OPT_DUP_CONNECT |\
 				 NVMF_OPT_DISABLE_SQFLOW | NVMF_OPT_DISCOVERY |\
-				 NVMF_OPT_FAIL_FAST_TMO)
+				 NVMF_OPT_FAIL_FAST_TMO | NVMF_OPT_SYMNAME)
 
 static struct nvme_ctrl *
 nvmf_create_ctrl(struct device *dev, const char *buf)
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index c3203ff1c654..494e6dbe233a 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -38,6 +38,7 @@ struct nvmf_host {
 	struct list_head	list;
 	char			nqn[NVMF_NQN_SIZE];
 	uuid_t			id;
+	char			symname[NVMF_HOSTSYMNAME_SIZE+1];
 };
 
 /**
@@ -68,6 +69,7 @@ enum {
 	NVMF_OPT_FAIL_FAST_TMO	= 1 << 20,
 	NVMF_OPT_HOST_IFACE	= 1 << 21,
 	NVMF_OPT_DISCOVERY	= 1 << 22,
+	NVMF_OPT_SYMNAME	= 1 << 23,
 };
 
 /**
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 82567d493c51..3b47951342f4 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -16,6 +16,8 @@
 /* However the max length of a qualified name is another size */
 #define NVMF_NQN_SIZE		223
 
+#define NVMF_HOSTSYMNAME_SIZE	256
+
 #define NVMF_TRSVCID_SIZE	32
 #define NVMF_TRADDR_SIZE	256
 #define NVMF_TSAS_SIZE		256
-- 
2.34.1

Re: [PATCH 2/4] nvme: add host symbolic name

[Hannes Reinecke]

On 1/25/22 15:59, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> TP8010 introduces the 'host symbolic name' as an optional
> parameter for explicit registration with a central discovery
> controller.
> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---
>   drivers/nvme/host/core.c    | 14 ++++++++++++++
>   drivers/nvme/host/fabrics.c | 17 +++++++++++++++--
>   drivers/nvme/host/fabrics.h |  2 ++
>   include/linux/nvme.h        |  2 ++
>   4 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index cd34b92e8e9d..cf5d9984f8f6 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3404,6 +3404,16 @@ static ssize_t nvme_sysfs_show_hostnqn(struct device *dev,
>   }
>   static DEVICE_ATTR(hostnqn, S_IRUGO, nvme_sysfs_show_hostnqn, NULL);
>   
> +static ssize_t nvme_sysfs_show_hostsymname(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%s\n", ctrl->opts->host->symname);
> +}
> +static DEVICE_ATTR(hostsymname, S_IRUGO, nvme_sysfs_show_hostsymname, NULL);
> +
>   static ssize_t nvme_sysfs_show_hostid(struct device *dev,
>   					struct device_attribute *attr,
>   					char *buf)
> @@ -3531,6 +3541,7 @@ static struct attribute *nvme_dev_attrs[] = {
>   	&dev_attr_sqsize.attr,
>   	&dev_attr_hostnqn.attr,
>   	&dev_attr_hostid.attr,
> +	&dev_attr_hostsymname.attr,
>   	&dev_attr_ctrl_loss_tmo.attr,
>   	&dev_attr_reconnect_delay.attr,
>   	&dev_attr_fast_io_fail_tmo.attr,
> @@ -3553,6 +3564,9 @@ static umode_t nvme_dev_attrs_are_visible(struct kobject *kobj,
>   		return 0;
>   	if (a == &dev_attr_hostid.attr && !ctrl->opts)
>   		return 0;
> +	if (a == &dev_attr_hostsymname.attr &&
> +	    (!ctrl->opts || (ctrl->opts->host->symname[0] == '\0')))
> +		return 0;
>   	if (a == &dev_attr_ctrl_loss_tmo.attr && !ctrl->opts)
>   		return 0;
>   	if (a == &dev_attr_reconnect_delay.attr && !ctrl->opts)
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 7ae041e2b3fb..040a6cce6afa 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -540,6 +540,7 @@ static const match_table_t opt_tokens = {
>   	{ NVMF_OPT_HOST_TRADDR,		"host_traddr=%s"	},
>   	{ NVMF_OPT_HOST_IFACE,		"host_iface=%s"		},
>   	{ NVMF_OPT_HOST_ID,		"hostid=%s"		},
> +	{ NVMF_OPT_SYMNAME,		"hostsymname=%s"	},
>   	{ NVMF_OPT_DUP_CONNECT,		"duplicate_connect"	},
>   	{ NVMF_OPT_DISABLE_SQFLOW,	"disable_sqflow"	},
>   	{ NVMF_OPT_HDR_DIGEST,		"hdr_digest"		},
> @@ -556,7 +557,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
>   		const char *buf)
>   {
>   	substring_t args[MAX_OPT_ARGS];
> -	char *options, *o, *p;
> +	char *options, *o, *p, *hostsymname = NULL;
>   	int token, ret = 0;
>   	size_t nqnlen  = 0;
>   	int ctrl_loss_tmo = NVMF_DEF_CTRL_LOSS_TMO;
> @@ -830,6 +831,13 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
>   		case NVMF_OPT_DISCOVERY:
>   			opts->discovery_nqn = true;
>   			break;
> +		case NVMF_OPT_SYMNAME:
> +			hostsymname = match_strdup(args);
> +			if (!hostsymname) {
> +				ret = -ENOMEM;
> +				goto out;
> +			}
> +			break;
>   		default:
>   			pr_warn("unknown parameter or missing value '%s' in ctrl creation request\n",
>   				p);
> @@ -864,8 +872,13 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
>   
>   	uuid_copy(&opts->host->id, &hostid);
>   
> +	if (hostsymname)
> +		strncpy(opts->host->symname, hostsymname,
> +			sizeof(opts->host->symname) - 1);
> +
>   out:
>   	kfree(options);
> +	kfree(hostsymname);
>   	return ret;
>   }
>   
> @@ -957,7 +970,7 @@ EXPORT_SYMBOL_GPL(nvmf_free_options);
>   				 NVMF_OPT_KATO | NVMF_OPT_HOSTNQN | \
>   				 NVMF_OPT_HOST_ID | NVMF_OPT_DUP_CONNECT |\
>   				 NVMF_OPT_DISABLE_SQFLOW | NVMF_OPT_DISCOVERY |\
> -				 NVMF_OPT_FAIL_FAST_TMO)
> +				 NVMF_OPT_FAIL_FAST_TMO | NVMF_OPT_SYMNAME)
>   
>   static struct nvme_ctrl *
>   nvmf_create_ctrl(struct device *dev, const char *buf)
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index c3203ff1c654..494e6dbe233a 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -38,6 +38,7 @@ struct nvmf_host {
>   	struct list_head	list;
>   	char			nqn[NVMF_NQN_SIZE];
>   	uuid_t			id;
> +	char			symname[NVMF_HOSTSYMNAME_SIZE+1];
>   };
>   
>   /**
> @@ -68,6 +69,7 @@ enum {
>   	NVMF_OPT_FAIL_FAST_TMO	= 1 << 20,
>   	NVMF_OPT_HOST_IFACE	= 1 << 21,
>   	NVMF_OPT_DISCOVERY	= 1 << 22,
> +	NVMF_OPT_SYMNAME	= 1 << 23,
>   };
>   
>   /**
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 82567d493c51..3b47951342f4 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -16,6 +16,8 @@
>   /* However the max length of a qualified name is another size */
>   #define NVMF_NQN_SIZE		223
>   
> +#define NVMF_HOSTSYMNAME_SIZE	256
> +
>   #define NVMF_TRSVCID_SIZE	32
>   #define NVMF_TRADDR_SIZE	256
>   #define NVMF_TSAS_SIZE		256

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH 2/4] nvme: add host symbolic name

[Sagi Grimberg]

> From: Martin Belanger <martin.belanger@dell.com>
> 
> TP8010 introduces the 'host symbolic name' as an optional
> parameter for explicit registration with a central discovery
> controller.
> 

If the other pieces move out, I don't see why the kernel needs
this given that it is not supposed to function as a sysfs kv-store.

[PATCH 3/4] nvme-fabrics: add tp8010 support

[Martin Belanger]

From: Martin Belanger <martin.belanger@dell.com>

TP8010 introduces Central Discovery Controllers (CDC) and the
Discovery Information Management (DIM) PDU, which is used to
perform explicit registration with Discovery Controllers.

This patch defines the DIM PDU and adds logic to perform explicit
registration with DCs when registration has been requested by the
user.

Signed-off-by: Martin Belanger <martin.belanger@dell.com>
---
 drivers/nvme/host/fabrics.c | 164 +++++++++++++++++++++++
 drivers/nvme/host/fabrics.h |   9 ++
 drivers/nvme/host/tcp.c     |  51 +++++++-
 include/linux/nvme.h        | 255 ++++++++++++++++++++++++++++++++++--
 4 files changed, 467 insertions(+), 12 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 040a6cce6afa..e6fc2f85b670 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -10,6 +10,7 @@
 #include <linux/mutex.h>
 #include <linux/parser.h>
 #include <linux/seq_file.h>
+#include <linux/utsname.h>
 #include "nvme.h"
 #include "fabrics.h"
 
@@ -526,6 +527,165 @@ static struct nvmf_transport_ops *nvmf_lookup_transport(
 	return NULL;
 }
 
+/**
+ * nvmf_get_tel() - Calculate the amount of memory needed for a Discovery
+ * Information Entry. Each Entry must contain at a minimum an Extended
+ * Attribute for the HostID. The Entry may optionally contain an Extended
+ * Attribute for the Symbolic Name.
+ *
+ * @hostsymname - Symbolic name (may be NULL)
+ *
+ * Return Total Entry Length
+ */
+static u32 nvmf_get_tel(const char *hostsymname)
+{
+	u32 tel = SIZEOF_EXT_DIE_WO_EXAT;
+	u16 len;
+
+	/* Host ID is mandatory */
+	tel += exat_size(sizeof(uuid_t));
+
+	/* Symbolic name is optional */
+	len = hostsymname ? strlen(hostsymname) : 0;
+	if (len)
+		tel += exat_size(len);
+
+	return tel;
+}
+
+/**
+ * nvmf_fill_die() - Fill the Discovery Information Entry pointed to by
+ *                   @die.
+ *
+ * @die - Pointer to Discovery Information Entry to be filled
+ * @tel - Length of the DIE
+ * @trtype - Transport type
+ * @adrfam - Address family
+ * @reg_addr - Address to register. Setting this to an empty string tells
+ *       the DC to infer address from the source address of the socket.
+ * @nqn - Host NQN
+ * @hostid - Host ID
+ * @hostsymname - Host symbolic name
+ * @tsas - Transport Specific Address Subtype for the address being
+ *       registered.
+ */
+static void nvmf_fill_die(struct nvmf_ext_die  *die,
+			  u32                   tel,
+			  u8                    trtype,
+			  u8                    adrfam,
+			  const char           *reg_addr,
+			  struct nvmf_host     *host,
+			  union nvmf_tsas      *tsas)
+{
+	u16 numexat = 0;
+	size_t symname_len;
+	struct nvmf_ext_attr *exat;
+
+	die->tel = cpu_to_le32(tel);
+	die->trtype = trtype;
+	die->adrfam = adrfam;
+
+	strncpy(die->nqn, host->nqn, sizeof(die->nqn));
+	strncpy(die->traddr, reg_addr, sizeof(die->traddr));
+	memcpy(&die->tsas, tsas, sizeof(die->tsas));
+
+	/* Extended Attribute for the HostID (mandatory) */
+	numexat++;
+	exat = &die->exat;
+	exat->type = cpu_to_le16(NVME_EXATTYPE_HOSTID);
+	exat->len  = cpu_to_le16(exat_len(sizeof(host->id)));
+	uuid_copy((uuid_t *)exat->val, &host->id);
+
+	/* Extended Attribute for the Symbolic Name (optional) */
+	symname_len = strlen(host->symname);
+	if (symname_len) {
+		u16 exatlen = exat_len(symname_len);
+
+		numexat++;
+		exat = exat_ptr_next(exat);
+		exat->type = cpu_to_le16(NVME_EXATTYPE_SYMNAME);
+		exat->len  = cpu_to_le16(exatlen);
+		memcpy(exat->val, host->symname, symname_len);
+		/* Per Base specs, ASCII strings must be padded with spaces */
+		memset(&exat->val[symname_len], ' ', exatlen - symname_len);
+	}
+
+	die->numexat = cpu_to_le16(numexat);
+}
+
+/**
+ * nvmf_disc_info_mgmt() - Perform explicit registration, deregistration,
+ * or registration-update (specified by @tas) by sending a Discovery
+ * Information Management (DIM) command to the Discovery Controller (DC).
+ *
+ * @ctrl: Host NVMe controller instance maintaining the admin queue used to
+ *      submit the DIM command to the DC.
+ * @tas: Task field of the Command Dword 10 (cdw10). Indicates whether to
+ *     perform a Registration, Deregistration, or Registration-update.
+ * @trtype: Transport type (e.g. NVMF_TRTYPE_TCP)
+ * @adrfam: Address family (e.g. NVMF_ADDR_FAMILY_IP6)
+ * @reg_addr - Address to register. Setting this to an empty string tells
+ *       the DC to infer address from the source address of the socket.
+ * @tsas - Transport Specific Address Subtype for the address being
+ *       registered.
+ *
+ * Return:   0: success,
+ *         > 0: NVMe error status code,
+ *         < 0: Linux errno error code
+ */
+int nvmf_disc_info_mgmt(struct nvme_ctrl *ctrl,
+			enum nvmf_dim_tas tas,
+			u8                trtype,
+			u8                adrfam,
+			const char       *reg_addr,
+			union nvmf_tsas  *tsas)
+{
+	int                   ret;
+	u32                   tdl;  /* Total Data Length */
+	u32                   tel;  /* Total Entry Length */
+	struct nvmf_dim_data *dim;  /* Discovery Information Management */
+	struct nvmf_ext_die  *die;  /* Discovery Information Entry */
+	struct nvme_command   cmd = {};
+	union nvme_result     result;
+
+	/* Register 1 Discovery Information Entry (DIE) of size TEL */
+	tel = nvmf_get_tel(ctrl->opts->host->symname);
+	tdl = SIZEOF_DIM_WO_DIE + tel;
+
+	dim = kzalloc(tdl, GFP_KERNEL);
+	if (!dim)
+		return -ENOMEM;
+
+	cmd.dim.opcode = nvme_admin_dim;
+	cmd.dim.cdw10  = cpu_to_le32(tas);
+
+	dim->tdl    = cpu_to_le32(tdl);
+	dim->nument = cpu_to_le64(1);    /* only 1 DIE to register */
+	dim->entfmt = cpu_to_le16(NVME_ENTFMT_EXTENDED);
+	dim->etype  = cpu_to_le16(NVME_REGISTRATION_HOST);
+	dim->ektype = cpu_to_le16(0x5F); /* must be 0x5F per specs */
+
+	strncpy(dim->eid, ctrl->opts->host->nqn, sizeof(dim->eid));
+	strncpy(dim->ename, utsname()->nodename, sizeof(dim->ename));
+	strncpy(dim->ever, utsname()->release, sizeof(dim->ever));
+
+	die = &dim->die.extended;
+	nvmf_fill_die(die, tel, trtype, adrfam, reg_addr,
+		      ctrl->opts->host, tsas);
+
+	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &result,
+				     dim, tdl, 0, NVME_QID_ANY, 1, 0);
+	if (unlikely(ret)) {
+		dev_err(ctrl->device, "DIM error Result:0x%04X Status:0x%04X\n",
+		   le32_to_cpu(result.u32), ret);
+	}
+
+	kfree(dim);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(nvmf_disc_info_mgmt);
+
 static const match_table_t opt_tokens = {
 	{ NVMF_OPT_TRANSPORT,		"transport=%s"		},
 	{ NVMF_OPT_TRADDR,		"traddr=%s"		},
@@ -550,6 +710,7 @@ static const match_table_t opt_tokens = {
 	{ NVMF_OPT_TOS,			"tos=%d"		},
 	{ NVMF_OPT_FAIL_FAST_TMO,	"fast_io_fail_tmo=%d"	},
 	{ NVMF_OPT_DISCOVERY,		"discovery"		},
+	{ NVMF_OPT_REGISTER,		"register"		},
 	{ NVMF_OPT_ERR,			NULL			}
 };
 
@@ -831,6 +992,9 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 		case NVMF_OPT_DISCOVERY:
 			opts->discovery_nqn = true;
 			break;
+		case NVMF_OPT_REGISTER:
+			opts->explicit_registration = true;
+			break;
 		case NVMF_OPT_SYMNAME:
 			hostsymname = match_strdup(args);
 			if (!hostsymname) {
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index 494e6dbe233a..77f5768f8ff4 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -70,6 +70,7 @@ enum {
 	NVMF_OPT_HOST_IFACE	= 1 << 21,
 	NVMF_OPT_DISCOVERY	= 1 << 22,
 	NVMF_OPT_SYMNAME	= 1 << 23,
+	NVMF_OPT_REGISTER	= 1 << 24,
 };
 
 /**
@@ -106,6 +107,7 @@ enum {
  * @nr_poll_queues: number of queues for polling I/O
  * @tos: type of service
  * @fast_io_fail_tmo: Fast I/O fail timeout in seconds
+ * @explicit_registration: Explicit Registration with DC using the DIM PDU
  */
 struct nvmf_ctrl_options {
 	unsigned		mask;
@@ -130,6 +132,7 @@ struct nvmf_ctrl_options {
 	unsigned int		nr_poll_queues;
 	int			tos;
 	int			fast_io_fail_tmo;
+	bool			explicit_registration;
 };
 
 /*
@@ -200,5 +203,11 @@ int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size);
 bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
 bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
 		struct nvmf_ctrl_options *opts);
+int nvmf_disc_info_mgmt(struct nvme_ctrl *ctrl,
+			enum nvmf_dim_tas tas,
+			u8                trtype,
+			u8                adrfam,
+			const char       *reg_addr,
+			union nvmf_tsas  *tsas);
 
 #endif /* _NVME_FABRICS_H */
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 4ceb28675fdf..970b7df574a4 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1993,6 +1993,40 @@ static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
 	}
 }
 
+/**
+ * nvme_get_adrfam() - Get address family for the address we're registering
+ * with the DC. We retrieve this info from the socket itself. If we can't
+ * get the source address from the socket, then we'll infer the address
+ * family from the address of the DC since the DC address has the same
+ * address family.
+ *
+ * @ctrl: Host NVMe controller instance maintaining the admin queue used to
+ *      submit the DIM command to the DC.
+ *
+ * Return: The address family of the source address associated with the
+ * socket connected to the DC.
+ */
+static u8 nvme_get_adrfam(struct nvme_ctrl *ctrl)
+{
+	u8                       adrfam = NVMF_ADDR_FAMILY_IP4;
+	struct sockaddr_storage  sas;
+	struct sockaddr         *sa = (struct sockaddr *)&sas;
+	struct nvme_tcp_queue   *queue = &to_tcp_ctrl(ctrl)->queues[0];
+
+	if (kernel_getsockname(queue->sock, sa) == 0) {
+		if (sa->sa_family == AF_INET6)
+			adrfam = NVMF_ADDR_FAMILY_IP6;
+	} else {
+		unsigned char buf[sizeof(struct in6_addr)];
+
+		if (in6_pton(ctrl->opts->traddr,
+			     strlen(ctrl->opts->traddr), buf, -1, NULL) > 0)
+			adrfam = NVMF_ADDR_FAMILY_IP6;
+	}
+
+	return adrfam;
+}
+
 static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
 {
 	struct nvmf_ctrl_options *opts = ctrl->opts;
@@ -2045,6 +2079,20 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
 		goto destroy_io;
 	}
 
+	if (ctrl->opts->explicit_registration &&
++	    ((ctrl->dctype == NVME_DCTYPE_DDC) ||
+	     (ctrl->dctype == NVME_DCTYPE_CDC))) {
+		/* We're registering our source address with the DC. To do
+		 * that, we can simply send an empty string. This tells the DC
+		 * to retrieve the source address from the socket and use that
+		 * as the registration address.
+		 */
+		union nvmf_tsas tsas = {.tcp.sectype = NVMF_TCP_SECTYPE_NONE};
+
+		nvmf_disc_info_mgmt(ctrl, NVME_TAS_REGISTER, NVMF_TRTYPE_TCP,
+				    nvme_get_adrfam(ctrl), "", &tsas);
+	}
+
 	nvme_start_ctrl(ctrl);
 	return 0;
 
@@ -2613,7 +2661,8 @@ static struct nvmf_transport_ops nvme_tcp_transport = {
 			  NVMF_OPT_HOST_TRADDR | NVMF_OPT_CTRL_LOSS_TMO |
 			  NVMF_OPT_HDR_DIGEST | NVMF_OPT_DATA_DIGEST |
 			  NVMF_OPT_NR_WRITE_QUEUES | NVMF_OPT_NR_POLL_QUEUES |
-			  NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE,
+			  NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE |
+			  NVMF_OPT_REGISTER,
 	.create_ctrl	= nvme_tcp_create_ctrl,
 };
 
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 3b47951342f4..891615876cfa 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -9,6 +9,7 @@
 
 #include <linux/types.h>
 #include <linux/uuid.h>
+#include <linux/kernel.h>
 
 /* NQN names in commands fields specified one size */
 #define NVMF_NQN_FIELD_LEN	256
@@ -102,6 +103,16 @@ enum {
 	NVMF_RDMA_CMS_RDMA_CM	= 1, /* Sockets based endpoint addressing */
 };
 
+/**
+ * enum -
+ * @NVMF_TCP_SECTYPE_NONE: No Security
+ * @NVMF_TCP_SECTYPE_TLS:  Transport Layer Security
+ */
+enum {
+	NVMF_TCP_SECTYPE_NONE	= 0,
+	NVMF_TCP_SECTYPE_TLS	= 1,
+};
+
 #define NVME_AQ_DEPTH		32
 #define NVME_NR_AEN_COMMANDS	1
 #define NVME_AQ_BLK_MQ_DEPTH	(NVME_AQ_DEPTH - NVME_NR_AEN_COMMANDS)
@@ -1036,6 +1047,7 @@ enum nvme_admin_opcode {
 	nvme_admin_sanitize_nvm		= 0x84,
 	nvme_admin_get_lba_status	= 0x86,
 	nvme_admin_vendor_start		= 0xC0,
+	nvme_admin_dim			= 0x21,
 };
 
 #define nvme_admin_opcode_name(opcode)	{ opcode, #opcode }
@@ -1316,6 +1328,29 @@ struct nvmf_common_command {
 	__u8	ts[24];
 };
 
+/**
+ * union nvmf_tsas - Transport Specific Address Subtype
+ * @qptype:  Queue Pair Service Type (NVMF_RDMA_QPTYPE_CONNECTED, NVMF_RDMA_QPTYPE_DATAGRAM)
+ * @prtype:  Provider Type (see enum  nvme_rdma_prtype)
+ * @cma:     Connection Management Service (NVMF_RDMA_CMS_RDMA_CM)
+ * @pkey:    Partition Key
+ * @sectype: Security Type (NVMF_TCP_SECTYPE_NONE, NVMF_TCP_SECTYPE_TLS)
+ */
+union nvmf_tsas {
+	char		common[NVMF_TSAS_SIZE];
+	struct rdma {
+		__u8	qptype;
+		__u8	prtype;
+		__u8	cms;
+		__u8	rsvd3[5];
+		__u16	pkey;
+		__u8	rsvd10[246];
+	} rdma;
+	struct tcp {
+		__u8	sectype;
+	} tcp;
+};
+
 /*
  * The legal cntlid range a NVMe Target will provide.
  * Note that cntlid of value 0 is considered illegal in the fabrics world.
@@ -1349,17 +1384,7 @@ struct nvmf_disc_rsp_page_entry {
 	__u8		resv64[192];
 	char		subnqn[NVMF_NQN_FIELD_LEN];
 	char		traddr[NVMF_TRADDR_SIZE];
-	union tsas {
-		char		common[NVMF_TSAS_SIZE];
-		struct rdma {
-			__u8	qptype;
-			__u8	prtype;
-			__u8	cms;
-			__u8	resv3[5];
-			__u16	pkey;
-			__u8	resv10[246];
-		} rdma;
-	} tsas;
+	union nvmf_tsas tsas;
 };
 
 /* Discovery log page header */
@@ -1447,6 +1472,213 @@ struct streams_directive_params {
 	__u8	rsvd2[6];
 };
 
+/**
+ * Discovery Information Management (DIM) command. This is sent by a
+ * host to the Central Discovery Controller (CDC) to perform
+ * explicit registration.
+ */
+enum nvmf_dim_tas {
+	NVME_TAS_REGISTER   = 0x00,
+	NVME_TAS_DEREGISTER = 0x01,
+	NVME_TAS_UPDATE     = 0x02,
+};
+
+struct nvmf_dim_command {
+	__u8                  opcode;     /* nvme_admin_dim */
+	__u8                  flags;
+	__u16                 command_id;
+	__le32                nsid;
+	__u64                 rsvd2[2];
+	union nvme_data_ptr   dptr;
+	__le32                cdw10;      /* enum nvmf_dim_tas */
+	__le32                cdw11;
+	__le32                cdw12;
+	__le32                cdw13;
+	__le32                cdw14;
+	__le32                cdw15;
+};
+
+#define NVMF_ENAME_LEN    256
+#define NVMF_EVER_LEN     64
+
+enum nvmf_dim_entfmt {
+	NVME_ENTFMT_BASIC    = 0x01,
+	NVME_ENTFMT_EXTENDED = 0x02,
+};
+
+enum nvmf_dim_etype {
+	NVME_REGISTRATION_HOST = 0x01,
+	NVME_REGISTRATION_DDC  = 0x02,
+	NVME_REGISTRATION_CDC  = 0x03,
+};
+
+enum nvmf_exattype {
+	NVME_EXATTYPE_HOSTID   = 0x01, /* Host Identifier */
+	NVME_EXATTYPE_SYMNAME  = 0x02, /* Symbolic Name */
+};
+
+/**
+ * struct nvmf_ext_attr - Extended Attribute (EXAT)
+ *
+ *       Bytes       Description
+ * @type 01:00       Extended Attribute Type (EXATTYPE) - enum nvmf_exattype
+ * @len  03:02       Extended Attribute Length (EXATLEN)
+ * @val  (EXATLEN-1) Extended Attribute Value (EXATVAL) - size allocated
+ *       +4:04       for array must be a multiple of 4 bytes
+ */
+struct nvmf_ext_attr {
+	__le16			type;
+	__le16			len;
+	__u8			val[];
+};
+#define SIZEOF_EXT_EXAT_WO_VAL offsetof(struct nvmf_ext_attr, val)
+
+/**
+ * struct nvmf_ext_die - Extended Discovery Information Entry (DIE)
+ *
+ *            Bytes           Description
+ * @trtype    00              Transport Type (enum nvme_trtype)
+ * @adrfam    01              Address Family (enum nvmf_addr_familiy)
+ * @subtype   02              Subsystem Type (enum nvme_subsys_type)
+ * @treq      03              Transport Requirements (enum nvmf_treq)
+ * @portid    05:04           Port ID
+ * @cntlid    07:06           Controller ID
+ * @asqsz     09:08           Admin Max SQ Size
+ *            31:10           Reserved
+ * @trsvcid   63:32           Transport Service Identifier
+ *            255:64          Reserved
+ * @nqn       511:256         NVM Qualified Name
+ * @traddr    767:512         Transport Address
+ * @tsas      1023:768        Transport Specific Address Subtype
+ * @tel       1027:1024       Total Entry Length
+ * @numexat   1029:1028       Number of Extended Attributes
+ *            1031:1030       Reserved
+ * @exat[0]   ((EXATLEN-1)+   Extented Attributes 0 (see @numexat)
+ *            4)+1032:1032    Cannot access as an array because each EXAT
+ *                            entry has an undetermined size.
+ * @exat[N]   TEL-1:TEL-      Extented Attributes N (w/ N = NUMEXAT-1)
+ *            (EXATLEN+4)
+ */
+struct nvmf_ext_die {
+	__u8			trtype;
+	__u8			adrfam;
+	__u8			subtype;
+	__u8			treq;
+	__le16			portid;
+	__le16			cntlid;
+	__le16			asqsz;
+	__u8			resv10[22];
+	char			trsvcid[NVMF_TRSVCID_SIZE];
+	__u8			resv64[192];
+	char			nqn[NVMF_NQN_FIELD_LEN];
+	char			traddr[NVMF_TRADDR_SIZE];
+	union nvmf_tsas		tsas;
+	__le32			tel;
+	__le16			numexat;
+	__u16			resv1030;
+	struct nvmf_ext_attr	exat;
+};
+#define SIZEOF_EXT_DIE_WO_EXAT offsetof(struct nvmf_ext_die, exat)
+
+/**
+ * union nvmf_die - Discovery Information Entry (DIE)
+ *
+ * Depending on the ENTFMT specified in the DIM, DIEs can be entered with
+ * the Basic or Extended formats. For Basic format, each entry has a fixed
+ * length. Therefore, the "basic" field defined below can be accessed as a
+ * C array. For the Extended format, however, each entry is of variable
+ * length (TEL). Therefore, the "extended" field defined below cannot be
+ * accessed as a C array. Instead, the "extended" field is akin to a
+ * linked-list, where one can "walk" through the list. To move to the next
+ * entry, one simply adds the current entry's length (TEL) to the "walk"
+ * pointer. The number of entries in the list is specified by NUMENT.
+ * Although extended entries are of a variable lengths (TEL), TEL is
+ * always a multiple of 4 bytes.
+ */
+union nvmf_die {
+	struct nvmf_disc_rsp_page_entry	basic[0];
+	struct nvmf_ext_die		extended;
+};
+
+/**
+ * struct nvmf_dim_data - Discovery Information Management (DIM) - Data
+ *
+ *            Bytes       Description
+ * @tdl       03:00       Total Data Length
+ *            07:04       Reserved
+ * @nument    15:08       Number of entries
+ * @entfmt    17:16       Entry Format (enum nvmf_dim_entfmt)
+ * @etype     19:18       Entity Type (enum nvmf_dim_etype)
+ * @portlcL   20          Port Local
+ *            21          Reserved
+ * @ektype    23:22       Entry Key Type
+ * @eid       279:24      Entity Identifier (e.g. Host NQN)
+ * @ename     535:280     Entity Name (e.g. hostname)
+ * @ever      599:536     Entity Version (e.g. OS Name/Version)
+ *            1023:600    Reserved
+ * @die       TDL-1:1024  Discovery Information Entry (see @nument above)
+ */
+struct nvmf_dim_data {
+	__le32			tdl;
+	__u32			rsvd4;
+	__le64			nument;
+	__le16			entfmt;
+	__le16			etype;
+	__u8			portlcl;
+	__u8			rsvd21;
+	__le16			ektype;
+	char			eid[NVMF_NQN_FIELD_LEN];
+	char			ename[NVMF_ENAME_LEN];
+	char			ever[NVMF_EVER_LEN];
+	__u8			rsvd600[424];
+	union nvmf_die		die;
+};
+#define SIZEOF_DIM_WO_DIE offsetof(struct nvmf_dim_data, die)
+
+/**
+ * exat_len() - Return the size in bytes, rounded to a multiple of 4 ((i.e.
+ *         size of u32), of the buffer needed to hold the exat value of
+ *         size @val_len.
+ */
+static inline u16 exat_len(size_t val_len)
+{
+	return (u16)round_up(val_len, sizeof(u32));
+}
+
+/**
+ * exat_size - return the size of the "struct nvmf_ext_attr"
+ *   needed to hold a value of size @val_len.
+ *
+ * @val_len: This is the length of the data to be copied to the "val"
+ *         field of a "struct nvmf_ext_attr".
+ *
+ * @return The size in bytes, rounded to a multiple of 4 (i.e. size of
+ *         u32), of the "struct nvmf_ext_attr" required to hold a string of
+ *         length @val_len.
+ */
+static inline u16 exat_size(size_t val_len)
+{
+	return (u16)(SIZEOF_EXT_EXAT_WO_VAL + exat_len(val_len));
+}
+
+/**
+ * exat_ptr_next - Increment @p to the next element in the array. Extended
+ *   attributes are saved to an array of "struct nvmf_ext_attr" where each
+ *   element of the array is of variable size. In order to move to the next
+ *   element in the array one must increment the pointer to the current
+ *   element (@p) by the size of the current element.
+ *
+ * @p: Pointer to an element of an array of "struct nvmf_ext_attr".
+ *
+ * @return Pointer to the next element in the array.
+ */
+static inline struct nvmf_ext_attr *exat_ptr_next(struct nvmf_ext_attr *p)
+{
+	return (struct nvmf_ext_attr *)
+		((uintptr_t)p + (ptrdiff_t)exat_size(le16_to_cpu(p->len)));
+}
+
+
 struct nvme_command {
 	union {
 		struct nvme_common_command common;
@@ -1470,6 +1702,7 @@ struct nvme_command {
 		struct nvmf_property_get_command prop_get;
 		struct nvme_dbbuf dbbuf;
 		struct nvme_directive_cmd directive;
+		struct nvmf_dim_command dim;
 	};
 };
 
-- 
2.34.1

Re: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Hannes Reinecke]

On 1/25/22 15:59, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> TP8010 introduces Central Discovery Controllers (CDC) and the
> Discovery Information Management (DIM) PDU, which is used to
> perform explicit registration with Discovery Controllers.
> 
> This patch defines the DIM PDU and adds logic to perform explicit
> registration with DCs when registration has been requested by the
> user.
> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---
>   drivers/nvme/host/fabrics.c | 164 +++++++++++++++++++++++
>   drivers/nvme/host/fabrics.h |   9 ++
>   drivers/nvme/host/tcp.c     |  51 +++++++-
>   include/linux/nvme.h        | 255 ++++++++++++++++++++++++++++++++++--
>   4 files changed, 467 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 040a6cce6afa..e6fc2f85b670 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -10,6 +10,7 @@
>   #include <linux/mutex.h>
>   #include <linux/parser.h>
>   #include <linux/seq_file.h>
> +#include <linux/utsname.h>
>   #include "nvme.h"
>   #include "fabrics.h"
>   
> @@ -526,6 +527,165 @@ static struct nvmf_transport_ops *nvmf_lookup_transport(
>   	return NULL;
>   }
>   
> +/**
> + * nvmf_get_tel() - Calculate the amount of memory needed for a Discovery
> + * Information Entry. Each Entry must contain at a minimum an Extended
> + * Attribute for the HostID. The Entry may optionally contain an Extended
> + * Attribute for the Symbolic Name.
> + *
> + * @hostsymname - Symbolic name (may be NULL)
> + *
> + * Return Total Entry Length
> + */
> +static u32 nvmf_get_tel(const char *hostsymname)
> +{
> +	u32 tel = SIZEOF_EXT_DIE_WO_EXAT;
> +	u16 len;
> +
> +	/* Host ID is mandatory */
> +	tel += exat_size(sizeof(uuid_t));
> +
> +	/* Symbolic name is optional */
> +	len = hostsymname ? strlen(hostsymname) : 0;
> +	if (len)
> +		tel += exat_size(len);
> +
> +	return tel;
> +}
> +
> +/**
> + * nvmf_fill_die() - Fill the Discovery Information Entry pointed to by
> + *                   @die.
> + *
> + * @die - Pointer to Discovery Information Entry to be filled
> + * @tel - Length of the DIE
> + * @trtype - Transport type
> + * @adrfam - Address family
> + * @reg_addr - Address to register. Setting this to an empty string tells
> + *       the DC to infer address from the source address of the socket.
> + * @nqn - Host NQN
> + * @hostid - Host ID
> + * @hostsymname - Host symbolic name
> + * @tsas - Transport Specific Address Subtype for the address being
> + *       registered.
> + */
> +static void nvmf_fill_die(struct nvmf_ext_die  *die,
> +			  u32                   tel,
> +			  u8                    trtype,
> +			  u8                    adrfam,
> +			  const char           *reg_addr,
> +			  struct nvmf_host     *host,
> +			  union nvmf_tsas      *tsas)
> +{
> +	u16 numexat = 0;
> +	size_t symname_len;
> +	struct nvmf_ext_attr *exat;
> +
> +	die->tel = cpu_to_le32(tel);
> +	die->trtype = trtype;
> +	die->adrfam = adrfam;
> +
> +	strncpy(die->nqn, host->nqn, sizeof(die->nqn));
> +	strncpy(die->traddr, reg_addr, sizeof(die->traddr));
> +	memcpy(&die->tsas, tsas, sizeof(die->tsas));
> +
> +	/* Extended Attribute for the HostID (mandatory) */
> +	numexat++;
> +	exat = &die->exat;
> +	exat->type = cpu_to_le16(NVME_EXATTYPE_HOSTID);
> +	exat->len  = cpu_to_le16(exat_len(sizeof(host->id)));
> +	uuid_copy((uuid_t *)exat->val, &host->id);
> +
> +	/* Extended Attribute for the Symbolic Name (optional) */
> +	symname_len = strlen(host->symname);
> +	if (symname_len) {
> +		u16 exatlen = exat_len(symname_len);
> +
> +		numexat++;
> +		exat = exat_ptr_next(exat);
> +		exat->type = cpu_to_le16(NVME_EXATTYPE_SYMNAME);
> +		exat->len  = cpu_to_le16(exatlen);
> +		memcpy(exat->val, host->symname, symname_len);
> +		/* Per Base specs, ASCII strings must be padded with spaces */
> +		memset(&exat->val[symname_len], ' ', exatlen - symname_len);
> +	}
> +
> +	die->numexat = cpu_to_le16(numexat);
> +}
> +
> +/**
> + * nvmf_disc_info_mgmt() - Perform explicit registration, deregistration,
> + * or registration-update (specified by @tas) by sending a Discovery
> + * Information Management (DIM) command to the Discovery Controller (DC).
> + *
> + * @ctrl: Host NVMe controller instance maintaining the admin queue used to
> + *      submit the DIM command to the DC.
> + * @tas: Task field of the Command Dword 10 (cdw10). Indicates whether to
> + *     perform a Registration, Deregistration, or Registration-update.
> + * @trtype: Transport type (e.g. NVMF_TRTYPE_TCP)
> + * @adrfam: Address family (e.g. NVMF_ADDR_FAMILY_IP6)
> + * @reg_addr - Address to register. Setting this to an empty string tells
> + *       the DC to infer address from the source address of the socket.
> + * @tsas - Transport Specific Address Subtype for the address being
> + *       registered.
> + *
> + * Return:   0: success,
> + *         > 0: NVMe error status code,
> + *         < 0: Linux errno error code
> + */
> +int nvmf_disc_info_mgmt(struct nvme_ctrl *ctrl,
> +			enum nvmf_dim_tas tas,
> +			u8                trtype,
> +			u8                adrfam,
> +			const char       *reg_addr,
> +			union nvmf_tsas  *tsas)
> +{
> +	int                   ret;
> +	u32                   tdl;  /* Total Data Length */
> +	u32                   tel;  /* Total Entry Length */
> +	struct nvmf_dim_data *dim;  /* Discovery Information Management */
> +	struct nvmf_ext_die  *die;  /* Discovery Information Entry */
> +	struct nvme_command   cmd = {};
> +	union nvme_result     result;
> +
> +	/* Register 1 Discovery Information Entry (DIE) of size TEL */
> +	tel = nvmf_get_tel(ctrl->opts->host->symname);
> +	tdl = SIZEOF_DIM_WO_DIE + tel;
> +
> +	dim = kzalloc(tdl, GFP_KERNEL);
> +	if (!dim)
> +		return -ENOMEM;
> +
> +	cmd.dim.opcode = nvme_admin_dim;
> +	cmd.dim.cdw10  = cpu_to_le32(tas);
> +
> +	dim->tdl    = cpu_to_le32(tdl);
> +	dim->nument = cpu_to_le64(1);    /* only 1 DIE to register */
> +	dim->entfmt = cpu_to_le16(NVME_ENTFMT_EXTENDED);
> +	dim->etype  = cpu_to_le16(NVME_REGISTRATION_HOST);
> +	dim->ektype = cpu_to_le16(0x5F); /* must be 0x5F per specs */
> +
> +	strncpy(dim->eid, ctrl->opts->host->nqn, sizeof(dim->eid));
> +	strncpy(dim->ename, utsname()->nodename, sizeof(dim->ename));
> +	strncpy(dim->ever, utsname()->release, sizeof(dim->ever));
> +
> +	die = &dim->die.extended;
> +	nvmf_fill_die(die, tel, trtype, adrfam, reg_addr,
> +		      ctrl->opts->host, tsas);
> +
> +	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &result,
> +				     dim, tdl, 0, NVME_QID_ANY, 1, 0);
> +	if (unlikely(ret)) {
> +		dev_err(ctrl->device, "DIM error Result:0x%04X Status:0x%04X\n",
> +		   le32_to_cpu(result.u32), ret);
> +	}
> +
> +	kfree(dim);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(nvmf_disc_info_mgmt);
> +
>   static const match_table_t opt_tokens = {
>   	{ NVMF_OPT_TRANSPORT,		"transport=%s"		},
>   	{ NVMF_OPT_TRADDR,		"traddr=%s"		},
> @@ -550,6 +710,7 @@ static const match_table_t opt_tokens = {
>   	{ NVMF_OPT_TOS,			"tos=%d"		},
>   	{ NVMF_OPT_FAIL_FAST_TMO,	"fast_io_fail_tmo=%d"	},
>   	{ NVMF_OPT_DISCOVERY,		"discovery"		},
> +	{ NVMF_OPT_REGISTER,		"register"		},
>   	{ NVMF_OPT_ERR,			NULL			}
>   };
>   
> @@ -831,6 +992,9 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
>   		case NVMF_OPT_DISCOVERY:
>   			opts->discovery_nqn = true;
>   			break;
> +		case NVMF_OPT_REGISTER:
> +			opts->explicit_registration = true;
> +			break;
>   		case NVMF_OPT_SYMNAME:
>   			hostsymname = match_strdup(args);
>   			if (!hostsymname) {
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index 494e6dbe233a..77f5768f8ff4 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -70,6 +70,7 @@ enum {
>   	NVMF_OPT_HOST_IFACE	= 1 << 21,
>   	NVMF_OPT_DISCOVERY	= 1 << 22,
>   	NVMF_OPT_SYMNAME	= 1 << 23,
> +	NVMF_OPT_REGISTER	= 1 << 24,
>   };
>   
>   /**
> @@ -106,6 +107,7 @@ enum {
>    * @nr_poll_queues: number of queues for polling I/O
>    * @tos: type of service
>    * @fast_io_fail_tmo: Fast I/O fail timeout in seconds
> + * @explicit_registration: Explicit Registration with DC using the DIM PDU
>    */
>   struct nvmf_ctrl_options {
>   	unsigned		mask;
> @@ -130,6 +132,7 @@ struct nvmf_ctrl_options {
>   	unsigned int		nr_poll_queues;
>   	int			tos;
>   	int			fast_io_fail_tmo;
> +	bool			explicit_registration;
>   };
>   
>   /*
> @@ -200,5 +203,11 @@ int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size);
>   bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
>   bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
>   		struct nvmf_ctrl_options *opts);
> +int nvmf_disc_info_mgmt(struct nvme_ctrl *ctrl,
> +			enum nvmf_dim_tas tas,
> +			u8                trtype,
> +			u8                adrfam,
> +			const char       *reg_addr,
> +			union nvmf_tsas  *tsas);
>   
>   #endif /* _NVME_FABRICS_H */
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 4ceb28675fdf..970b7df574a4 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1993,6 +1993,40 @@ static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
>   	}
>   }
>   
> +/**
> + * nvme_get_adrfam() - Get address family for the address we're registering
> + * with the DC. We retrieve this info from the socket itself. If we can't
> + * get the source address from the socket, then we'll infer the address
> + * family from the address of the DC since the DC address has the same
> + * address family.
> + *
> + * @ctrl: Host NVMe controller instance maintaining the admin queue used to
> + *      submit the DIM command to the DC.
> + *
> + * Return: The address family of the source address associated with the
> + * socket connected to the DC.
> + */
> +static u8 nvme_get_adrfam(struct nvme_ctrl *ctrl)
> +{
> +	u8                       adrfam = NVMF_ADDR_FAMILY_IP4;
> +	struct sockaddr_storage  sas;
> +	struct sockaddr         *sa = (struct sockaddr *)&sas;
> +	struct nvme_tcp_queue   *queue = &to_tcp_ctrl(ctrl)->queues[0];
> +
> +	if (kernel_getsockname(queue->sock, sa) == 0) {
> +		if (sa->sa_family == AF_INET6)
> +			adrfam = NVMF_ADDR_FAMILY_IP6;
> +	} else {
> +		unsigned char buf[sizeof(struct in6_addr)];
> +
> +		if (in6_pton(ctrl->opts->traddr,
> +			     strlen(ctrl->opts->traddr), buf, -1, NULL) > 0)
> +			adrfam = NVMF_ADDR_FAMILY_IP6;
> +	}
> +
> +	return adrfam;
> +}
> +
>   static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
>   {
>   	struct nvmf_ctrl_options *opts = ctrl->opts;
> @@ -2045,6 +2079,20 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
>   		goto destroy_io;
>   	}
>   
> +	if (ctrl->opts->explicit_registration &&
> ++	    ((ctrl->dctype == NVME_DCTYPE_DDC) ||
> +	     (ctrl->dctype == NVME_DCTYPE_CDC))) {
> +		/* We're registering our source address with the DC. To do
> +		 * that, we can simply send an empty string. This tells the DC
> +		 * to retrieve the source address from the socket and use that
> +		 * as the registration address.
> +		 */
> +		union nvmf_tsas tsas = {.tcp.sectype = NVMF_TCP_SECTYPE_NONE};
> +
> +		nvmf_disc_info_mgmt(ctrl, NVME_TAS_REGISTER, NVMF_TRTYPE_TCP,
> +				    nvme_get_adrfam(ctrl), "", &tsas);
> +	}
> +
>   	nvme_start_ctrl(ctrl);
>   	return 0;
>   
> @@ -2613,7 +2661,8 @@ static struct nvmf_transport_ops nvme_tcp_transport = {
>   			  NVMF_OPT_HOST_TRADDR | NVMF_OPT_CTRL_LOSS_TMO |
>   			  NVMF_OPT_HDR_DIGEST | NVMF_OPT_DATA_DIGEST |
>   			  NVMF_OPT_NR_WRITE_QUEUES | NVMF_OPT_NR_POLL_QUEUES |
> -			  NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE,
> +			  NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE |
> +			  NVMF_OPT_REGISTER,
>   	.create_ctrl	= nvme_tcp_create_ctrl,
>   };
>   
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 3b47951342f4..891615876cfa 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -9,6 +9,7 @@
>   
>   #include <linux/types.h>
>   #include <linux/uuid.h>
> +#include <linux/kernel.h>
>   
>   /* NQN names in commands fields specified one size */
>   #define NVMF_NQN_FIELD_LEN	256
> @@ -102,6 +103,16 @@ enum {
>   	NVMF_RDMA_CMS_RDMA_CM	= 1, /* Sockets based endpoint addressing */
>   };
>   
> +/**
> + * enum -
> + * @NVMF_TCP_SECTYPE_NONE: No Security
> + * @NVMF_TCP_SECTYPE_TLS:  Transport Layer Security
> + */
> +enum {
> +	NVMF_TCP_SECTYPE_NONE	= 0,
> +	NVMF_TCP_SECTYPE_TLS	= 1,
> +};
> +
>   #define NVME_AQ_DEPTH		32
>   #define NVME_NR_AEN_COMMANDS	1
>   #define NVME_AQ_BLK_MQ_DEPTH	(NVME_AQ_DEPTH - NVME_NR_AEN_COMMANDS)
> @@ -1036,6 +1047,7 @@ enum nvme_admin_opcode {
>   	nvme_admin_sanitize_nvm		= 0x84,
>   	nvme_admin_get_lba_status	= 0x86,
>   	nvme_admin_vendor_start		= 0xC0,
> +	nvme_admin_dim			= 0x21,
>   };
>   
>   #define nvme_admin_opcode_name(opcode)	{ opcode, #opcode }
> @@ -1316,6 +1328,29 @@ struct nvmf_common_command {
>   	__u8	ts[24];
>   };
>   
> +/**
> + * union nvmf_tsas - Transport Specific Address Subtype
> + * @qptype:  Queue Pair Service Type (NVMF_RDMA_QPTYPE_CONNECTED, NVMF_RDMA_QPTYPE_DATAGRAM)
> + * @prtype:  Provider Type (see enum  nvme_rdma_prtype)
> + * @cma:     Connection Management Service (NVMF_RDMA_CMS_RDMA_CM)
> + * @pkey:    Partition Key
> + * @sectype: Security Type (NVMF_TCP_SECTYPE_NONE, NVMF_TCP_SECTYPE_TLS)
> + */
> +union nvmf_tsas {
> +	char		common[NVMF_TSAS_SIZE];
> +	struct rdma {
> +		__u8	qptype;
> +		__u8	prtype;
> +		__u8	cms;
> +		__u8	rsvd3[5];
> +		__u16	pkey;
> +		__u8	rsvd10[246];
> +	} rdma;
> +	struct tcp {
> +		__u8	sectype;
> +	} tcp;
> +};
> +
>   /*
>    * The legal cntlid range a NVMe Target will provide.
>    * Note that cntlid of value 0 is considered illegal in the fabrics world.
> @@ -1349,17 +1384,7 @@ struct nvmf_disc_rsp_page_entry {
>   	__u8		resv64[192];
>   	char		subnqn[NVMF_NQN_FIELD_LEN];
>   	char		traddr[NVMF_TRADDR_SIZE];
> -	union tsas {
> -		char		common[NVMF_TSAS_SIZE];
> -		struct rdma {
> -			__u8	qptype;
> -			__u8	prtype;
> -			__u8	cms;
> -			__u8	resv3[5];
> -			__u16	pkey;
> -			__u8	resv10[246];
> -		} rdma;
> -	} tsas;
> +	union nvmf_tsas tsas;
>   };
>   
>   /* Discovery log page header */
> @@ -1447,6 +1472,213 @@ struct streams_directive_params {
>   	__u8	rsvd2[6];
>   };
>   
> +/**
> + * Discovery Information Management (DIM) command. This is sent by a
> + * host to the Central Discovery Controller (CDC) to perform
> + * explicit registration.
> + */
> +enum nvmf_dim_tas {
> +	NVME_TAS_REGISTER   = 0x00,
> +	NVME_TAS_DEREGISTER = 0x01,
> +	NVME_TAS_UPDATE     = 0x02,
> +};
> +
> +struct nvmf_dim_command {
> +	__u8                  opcode;     /* nvme_admin_dim */
> +	__u8                  flags;
> +	__u16                 command_id;
> +	__le32                nsid;
> +	__u64                 rsvd2[2];
> +	union nvme_data_ptr   dptr;
> +	__le32                cdw10;      /* enum nvmf_dim_tas */
> +	__le32                cdw11;
> +	__le32                cdw12;
> +	__le32                cdw13;
> +	__le32                cdw14;
> +	__le32                cdw15;
> +};
> +
> +#define NVMF_ENAME_LEN    256
> +#define NVMF_EVER_LEN     64
> +
> +enum nvmf_dim_entfmt {
> +	NVME_ENTFMT_BASIC    = 0x01,
> +	NVME_ENTFMT_EXTENDED = 0x02,
> +};
> +
> +enum nvmf_dim_etype {
> +	NVME_REGISTRATION_HOST = 0x01,
> +	NVME_REGISTRATION_DDC  = 0x02,
> +	NVME_REGISTRATION_CDC  = 0x03,
> +};
> +
> +enum nvmf_exattype {
> +	NVME_EXATTYPE_HOSTID   = 0x01, /* Host Identifier */
> +	NVME_EXATTYPE_SYMNAME  = 0x02, /* Symbolic Name */
> +};
> +
> +/**
> + * struct nvmf_ext_attr - Extended Attribute (EXAT)
> + *
> + *       Bytes       Description
> + * @type 01:00       Extended Attribute Type (EXATTYPE) - enum nvmf_exattype
> + * @len  03:02       Extended Attribute Length (EXATLEN)
> + * @val  (EXATLEN-1) Extended Attribute Value (EXATVAL) - size allocated
> + *       +4:04       for array must be a multiple of 4 bytes
> + */
> +struct nvmf_ext_attr {
> +	__le16			type;
> +	__le16			len;
> +	__u8			val[];
> +};
> +#define SIZEOF_EXT_EXAT_WO_VAL offsetof(struct nvmf_ext_attr, val)
> +
> +/**
> + * struct nvmf_ext_die - Extended Discovery Information Entry (DIE)
> + *
> + *            Bytes           Description
> + * @trtype    00              Transport Type (enum nvme_trtype)
> + * @adrfam    01              Address Family (enum nvmf_addr_familiy)
> + * @subtype   02              Subsystem Type (enum nvme_subsys_type)
> + * @treq      03              Transport Requirements (enum nvmf_treq)
> + * @portid    05:04           Port ID
> + * @cntlid    07:06           Controller ID
> + * @asqsz     09:08           Admin Max SQ Size
> + *            31:10           Reserved
> + * @trsvcid   63:32           Transport Service Identifier
> + *            255:64          Reserved
> + * @nqn       511:256         NVM Qualified Name
> + * @traddr    767:512         Transport Address
> + * @tsas      1023:768        Transport Specific Address Subtype
> + * @tel       1027:1024       Total Entry Length
> + * @numexat   1029:1028       Number of Extended Attributes
> + *            1031:1030       Reserved
> + * @exat[0]   ((EXATLEN-1)+   Extented Attributes 0 (see @numexat)
> + *            4)+1032:1032    Cannot access as an array because each EXAT
> + *                            entry has an undetermined size.
> + * @exat[N]   TEL-1:TEL-      Extented Attributes N (w/ N = NUMEXAT-1)
> + *            (EXATLEN+4)
> + */
> +struct nvmf_ext_die {
> +	__u8			trtype;
> +	__u8			adrfam;
> +	__u8			subtype;
> +	__u8			treq;
> +	__le16			portid;
> +	__le16			cntlid;
> +	__le16			asqsz;
> +	__u8			resv10[22];
> +	char			trsvcid[NVMF_TRSVCID_SIZE];
> +	__u8			resv64[192];
> +	char			nqn[NVMF_NQN_FIELD_LEN];
> +	char			traddr[NVMF_TRADDR_SIZE];
> +	union nvmf_tsas		tsas;
> +	__le32			tel;
> +	__le16			numexat;
> +	__u16			resv1030;
> +	struct nvmf_ext_attr	exat;
> +};
> +#define SIZEOF_EXT_DIE_WO_EXAT offsetof(struct nvmf_ext_die, exat)
> +
> +/**
> + * union nvmf_die - Discovery Information Entry (DIE)
> + *
> + * Depending on the ENTFMT specified in the DIM, DIEs can be entered with
> + * the Basic or Extended formats. For Basic format, each entry has a fixed
> + * length. Therefore, the "basic" field defined below can be accessed as a
> + * C array. For the Extended format, however, each entry is of variable
> + * length (TEL). Therefore, the "extended" field defined below cannot be
> + * accessed as a C array. Instead, the "extended" field is akin to a
> + * linked-list, where one can "walk" through the list. To move to the next
> + * entry, one simply adds the current entry's length (TEL) to the "walk"
> + * pointer. The number of entries in the list is specified by NUMENT.
> + * Although extended entries are of a variable lengths (TEL), TEL is
> + * always a multiple of 4 bytes.
> + */
> +union nvmf_die {
> +	struct nvmf_disc_rsp_page_entry	basic[0];
> +	struct nvmf_ext_die		extended;
> +};
> +
> +/**
> + * struct nvmf_dim_data - Discovery Information Management (DIM) - Data
> + *
> + *            Bytes       Description
> + * @tdl       03:00       Total Data Length
> + *            07:04       Reserved
> + * @nument    15:08       Number of entries
> + * @entfmt    17:16       Entry Format (enum nvmf_dim_entfmt)
> + * @etype     19:18       Entity Type (enum nvmf_dim_etype)
> + * @portlcL   20          Port Local
> + *            21          Reserved
> + * @ektype    23:22       Entry Key Type
> + * @eid       279:24      Entity Identifier (e.g. Host NQN)
> + * @ename     535:280     Entity Name (e.g. hostname)
> + * @ever      599:536     Entity Version (e.g. OS Name/Version)
> + *            1023:600    Reserved
> + * @die       TDL-1:1024  Discovery Information Entry (see @nument above)
> + */
> +struct nvmf_dim_data {
> +	__le32			tdl;
> +	__u32			rsvd4;
> +	__le64			nument;
> +	__le16			entfmt;
> +	__le16			etype;
> +	__u8			portlcl;
> +	__u8			rsvd21;
> +	__le16			ektype;
> +	char			eid[NVMF_NQN_FIELD_LEN];
> +	char			ename[NVMF_ENAME_LEN];
> +	char			ever[NVMF_EVER_LEN];
> +	__u8			rsvd600[424];
> +	union nvmf_die		die;
> +};
> +#define SIZEOF_DIM_WO_DIE offsetof(struct nvmf_dim_data, die)
> +
> +/**
> + * exat_len() - Return the size in bytes, rounded to a multiple of 4 ((i.e.
> + *         size of u32), of the buffer needed to hold the exat value of
> + *         size @val_len.
> + */
> +static inline u16 exat_len(size_t val_len)
> +{
> +	return (u16)round_up(val_len, sizeof(u32));
> +}
> +
> +/**
> + * exat_size - return the size of the "struct nvmf_ext_attr"
> + *   needed to hold a value of size @val_len.
> + *
> + * @val_len: This is the length of the data to be copied to the "val"
> + *         field of a "struct nvmf_ext_attr".
> + *
> + * @return The size in bytes, rounded to a multiple of 4 (i.e. size of
> + *         u32), of the "struct nvmf_ext_attr" required to hold a string of
> + *         length @val_len.
> + */
> +static inline u16 exat_size(size_t val_len)
> +{
> +	return (u16)(SIZEOF_EXT_EXAT_WO_VAL + exat_len(val_len));
> +}
> +
> +/**
> + * exat_ptr_next - Increment @p to the next element in the array. Extended
> + *   attributes are saved to an array of "struct nvmf_ext_attr" where each
> + *   element of the array is of variable size. In order to move to the next
> + *   element in the array one must increment the pointer to the current
> + *   element (@p) by the size of the current element.
> + *
> + * @p: Pointer to an element of an array of "struct nvmf_ext_attr".
> + *
> + * @return Pointer to the next element in the array.
> + */
> +static inline struct nvmf_ext_attr *exat_ptr_next(struct nvmf_ext_attr *p)
> +{
> +	return (struct nvmf_ext_attr *)
> +		((uintptr_t)p + (ptrdiff_t)exat_size(le16_to_cpu(p->len)));
> +}
> +
> +
>   struct nvme_command {
>   	union {
>   		struct nvme_common_command common;
> @@ -1470,6 +1702,7 @@ struct nvme_command {
>   		struct nvmf_property_get_command prop_get;
>   		struct nvme_dbbuf dbbuf;
>   		struct nvme_directive_cmd directive;
> +		struct nvmf_dim_command dim;
>   	};
>   };
>   
I'd rather like to have the TLS bits in a separate patch; they are not 
strictly related to this issue.

And I have a rather general issue with this: Is it required to issue an 
explicit registration directly after sending the 'connect' command?
IE do we need to have it as part of the 'connect' routine?

Thing is, the linux 'connect' routine is doing several things in one go
(like establishing a transport association for the admin queue, send the 
'connect' command for the admin queue, create transport associations for 
I/O queues and send 'connect' commands on all I/O queues, _and_ possibly 
do authentication, too).
If we now start overloading it with yet more functionality it becomes 
extremely hard to cleanup after an error, and that doesn't even mention 
the non-existing error reporting to userspace.

Wouldn't it make more sense to delegate explicit registration to 
userspace (ie libnvme/nvme-cli), and leave the kernel out of it?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Sagi Grimberg]

On 1/27/22 11:30, Hannes Reinecke wrote:
> On 1/25/22 15:59, Martin Belanger wrote:
>> From: Martin Belanger <martin.belanger@dell.com>
>>
>> TP8010 introduces Central Discovery Controllers (CDC) and the
>> Discovery Information Management (DIM) PDU, which is used to
>> perform explicit registration with Discovery Controllers.
>>
>> This patch defines the DIM PDU and adds logic to perform explicit
>> registration with DCs when registration has been requested by the
>> user.
>>
>> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
>> ---
>>   drivers/nvme/host/fabrics.c | 164 +++++++++++++++++++++++
>>   drivers/nvme/host/fabrics.h |   9 ++
>>   drivers/nvme/host/tcp.c     |  51 +++++++-
>>   include/linux/nvme.h        | 255 ++++++++++++++++++++++++++++++++++--
>>   4 files changed, 467 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index 040a6cce6afa..e6fc2f85b670 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/mutex.h>
>>   #include <linux/parser.h>
>>   #include <linux/seq_file.h>
>> +#include <linux/utsname.h>
>>   #include "nvme.h"
>>   #include "fabrics.h"
>> @@ -526,6 +527,165 @@ static struct nvmf_transport_ops 
>> *nvmf_lookup_transport(
>>       return NULL;
>>   }
>> +/**
>> + * nvmf_get_tel() - Calculate the amount of memory needed for a 
>> Discovery
>> + * Information Entry. Each Entry must contain at a minimum an Extended
>> + * Attribute for the HostID. The Entry may optionally contain an 
>> Extended
>> + * Attribute for the Symbolic Name.
>> + *
>> + * @hostsymname - Symbolic name (may be NULL)
>> + *
>> + * Return Total Entry Length
>> + */
>> +static u32 nvmf_get_tel(const char *hostsymname)
>> +{
>> +    u32 tel = SIZEOF_EXT_DIE_WO_EXAT;
>> +    u16 len;
>> +
>> +    /* Host ID is mandatory */
>> +    tel += exat_size(sizeof(uuid_t));
>> +
>> +    /* Symbolic name is optional */
>> +    len = hostsymname ? strlen(hostsymname) : 0;
>> +    if (len)
>> +        tel += exat_size(len);
>> +
>> +    return tel;
>> +}
>> +
>> +/**
>> + * nvmf_fill_die() - Fill the Discovery Information Entry pointed to by
>> + *                   @die.
>> + *
>> + * @die - Pointer to Discovery Information Entry to be filled
>> + * @tel - Length of the DIE
>> + * @trtype - Transport type
>> + * @adrfam - Address family
>> + * @reg_addr - Address to register. Setting this to an empty string 
>> tells
>> + *       the DC to infer address from the source address of the socket.
>> + * @nqn - Host NQN
>> + * @hostid - Host ID
>> + * @hostsymname - Host symbolic name
>> + * @tsas - Transport Specific Address Subtype for the address being
>> + *       registered.
>> + */
>> +static void nvmf_fill_die(struct nvmf_ext_die  *die,
>> +              u32                   tel,
>> +              u8                    trtype,
>> +              u8                    adrfam,
>> +              const char           *reg_addr,
>> +              struct nvmf_host     *host,
>> +              union nvmf_tsas      *tsas)
>> +{
>> +    u16 numexat = 0;
>> +    size_t symname_len;
>> +    struct nvmf_ext_attr *exat;
>> +
>> +    die->tel = cpu_to_le32(tel);
>> +    die->trtype = trtype;
>> +    die->adrfam = adrfam;
>> +
>> +    strncpy(die->nqn, host->nqn, sizeof(die->nqn));
>> +    strncpy(die->traddr, reg_addr, sizeof(die->traddr));
>> +    memcpy(&die->tsas, tsas, sizeof(die->tsas));
>> +
>> +    /* Extended Attribute for the HostID (mandatory) */
>> +    numexat++;
>> +    exat = &die->exat;
>> +    exat->type = cpu_to_le16(NVME_EXATTYPE_HOSTID);
>> +    exat->len  = cpu_to_le16(exat_len(sizeof(host->id)));
>> +    uuid_copy((uuid_t *)exat->val, &host->id);
>> +
>> +    /* Extended Attribute for the Symbolic Name (optional) */
>> +    symname_len = strlen(host->symname);
>> +    if (symname_len) {
>> +        u16 exatlen = exat_len(symname_len);
>> +
>> +        numexat++;
>> +        exat = exat_ptr_next(exat);
>> +        exat->type = cpu_to_le16(NVME_EXATTYPE_SYMNAME);
>> +        exat->len  = cpu_to_le16(exatlen);
>> +        memcpy(exat->val, host->symname, symname_len);
>> +        /* Per Base specs, ASCII strings must be padded with spaces */
>> +        memset(&exat->val[symname_len], ' ', exatlen - symname_len);
>> +    }
>> +
>> +    die->numexat = cpu_to_le16(numexat);
>> +}
>> +
>> +/**
>> + * nvmf_disc_info_mgmt() - Perform explicit registration, 
>> deregistration,
>> + * or registration-update (specified by @tas) by sending a Discovery
>> + * Information Management (DIM) command to the Discovery Controller 
>> (DC).
>> + *
>> + * @ctrl: Host NVMe controller instance maintaining the admin queue 
>> used to
>> + *      submit the DIM command to the DC.
>> + * @tas: Task field of the Command Dword 10 (cdw10). Indicates 
>> whether to
>> + *     perform a Registration, Deregistration, or Registration-update.
>> + * @trtype: Transport type (e.g. NVMF_TRTYPE_TCP)
>> + * @adrfam: Address family (e.g. NVMF_ADDR_FAMILY_IP6)
>> + * @reg_addr - Address to register. Setting this to an empty string 
>> tells
>> + *       the DC to infer address from the source address of the socket.
>> + * @tsas - Transport Specific Address Subtype for the address being
>> + *       registered.
>> + *
>> + * Return:   0: success,
>> + *         > 0: NVMe error status code,
>> + *         < 0: Linux errno error code
>> + */
>> +int nvmf_disc_info_mgmt(struct nvme_ctrl *ctrl,
>> +            enum nvmf_dim_tas tas,
>> +            u8                trtype,
>> +            u8                adrfam,
>> +            const char       *reg_addr,
>> +            union nvmf_tsas  *tsas)
>> +{
>> +    int                   ret;
>> +    u32                   tdl;  /* Total Data Length */
>> +    u32                   tel;  /* Total Entry Length */
>> +    struct nvmf_dim_data *dim;  /* Discovery Information Management */
>> +    struct nvmf_ext_die  *die;  /* Discovery Information Entry */
>> +    struct nvme_command   cmd = {};
>> +    union nvme_result     result;
>> +
>> +    /* Register 1 Discovery Information Entry (DIE) of size TEL */
>> +    tel = nvmf_get_tel(ctrl->opts->host->symname);
>> +    tdl = SIZEOF_DIM_WO_DIE + tel;
>> +
>> +    dim = kzalloc(tdl, GFP_KERNEL);
>> +    if (!dim)
>> +        return -ENOMEM;
>> +
>> +    cmd.dim.opcode = nvme_admin_dim;
>> +    cmd.dim.cdw10  = cpu_to_le32(tas);
>> +
>> +    dim->tdl    = cpu_to_le32(tdl);
>> +    dim->nument = cpu_to_le64(1);    /* only 1 DIE to register */
>> +    dim->entfmt = cpu_to_le16(NVME_ENTFMT_EXTENDED);
>> +    dim->etype  = cpu_to_le16(NVME_REGISTRATION_HOST);
>> +    dim->ektype = cpu_to_le16(0x5F); /* must be 0x5F per specs */
>> +
>> +    strncpy(dim->eid, ctrl->opts->host->nqn, sizeof(dim->eid));
>> +    strncpy(dim->ename, utsname()->nodename, sizeof(dim->ename));
>> +    strncpy(dim->ever, utsname()->release, sizeof(dim->ever));
>> +
>> +    die = &dim->die.extended;
>> +    nvmf_fill_die(die, tel, trtype, adrfam, reg_addr,
>> +              ctrl->opts->host, tsas);
>> +
>> +    ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &result,
>> +                     dim, tdl, 0, NVME_QID_ANY, 1, 0);
>> +    if (unlikely(ret)) {
>> +        dev_err(ctrl->device, "DIM error Result:0x%04X Status:0x%04X\n",
>> +           le32_to_cpu(result.u32), ret);
>> +    }
>> +
>> +    kfree(dim);
>> +
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(nvmf_disc_info_mgmt);
>> +
>>   static const match_table_t opt_tokens = {
>>       { NVMF_OPT_TRANSPORT,        "transport=%s"        },
>>       { NVMF_OPT_TRADDR,        "traddr=%s"        },
>> @@ -550,6 +710,7 @@ static const match_table_t opt_tokens = {
>>       { NVMF_OPT_TOS,            "tos=%d"        },
>>       { NVMF_OPT_FAIL_FAST_TMO,    "fast_io_fail_tmo=%d"    },
>>       { NVMF_OPT_DISCOVERY,        "discovery"        },
>> +    { NVMF_OPT_REGISTER,        "register"        },
>>       { NVMF_OPT_ERR,            NULL            }
>>   };
>> @@ -831,6 +992,9 @@ static int nvmf_parse_options(struct 
>> nvmf_ctrl_options *opts,
>>           case NVMF_OPT_DISCOVERY:
>>               opts->discovery_nqn = true;
>>               break;
>> +        case NVMF_OPT_REGISTER:
>> +            opts->explicit_registration = true;
>> +            break;
>>           case NVMF_OPT_SYMNAME:
>>               hostsymname = match_strdup(args);
>>               if (!hostsymname) {
>> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
>> index 494e6dbe233a..77f5768f8ff4 100644
>> --- a/drivers/nvme/host/fabrics.h
>> +++ b/drivers/nvme/host/fabrics.h
>> @@ -70,6 +70,7 @@ enum {
>>       NVMF_OPT_HOST_IFACE    = 1 << 21,
>>       NVMF_OPT_DISCOVERY    = 1 << 22,
>>       NVMF_OPT_SYMNAME    = 1 << 23,
>> +    NVMF_OPT_REGISTER    = 1 << 24,
>>   };
>>   /**
>> @@ -106,6 +107,7 @@ enum {
>>    * @nr_poll_queues: number of queues for polling I/O
>>    * @tos: type of service
>>    * @fast_io_fail_tmo: Fast I/O fail timeout in seconds
>> + * @explicit_registration: Explicit Registration with DC using the 
>> DIM PDU
>>    */
>>   struct nvmf_ctrl_options {
>>       unsigned        mask;
>> @@ -130,6 +132,7 @@ struct nvmf_ctrl_options {
>>       unsigned int        nr_poll_queues;
>>       int            tos;
>>       int            fast_io_fail_tmo;
>> +    bool            explicit_registration;
>>   };
>>   /*
>> @@ -200,5 +203,11 @@ int nvmf_get_address(struct nvme_ctrl *ctrl, char 
>> *buf, int size);
>>   bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
>>   bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
>>           struct nvmf_ctrl_options *opts);
>> +int nvmf_disc_info_mgmt(struct nvme_ctrl *ctrl,
>> +            enum nvmf_dim_tas tas,
>> +            u8                trtype,
>> +            u8                adrfam,
>> +            const char       *reg_addr,
>> +            union nvmf_tsas  *tsas);
>>   #endif /* _NVME_FABRICS_H */
>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>> index 4ceb28675fdf..970b7df574a4 100644
>> --- a/drivers/nvme/host/tcp.c
>> +++ b/drivers/nvme/host/tcp.c
>> @@ -1993,6 +1993,40 @@ static void nvme_tcp_reconnect_or_remove(struct 
>> nvme_ctrl *ctrl)
>>       }
>>   }
>> +/**
>> + * nvme_get_adrfam() - Get address family for the address we're 
>> registering
>> + * with the DC. We retrieve this info from the socket itself. If we 
>> can't
>> + * get the source address from the socket, then we'll infer the address
>> + * family from the address of the DC since the DC address has the same
>> + * address family.
>> + *
>> + * @ctrl: Host NVMe controller instance maintaining the admin queue 
>> used to
>> + *      submit the DIM command to the DC.
>> + *
>> + * Return: The address family of the source address associated with the
>> + * socket connected to the DC.
>> + */
>> +static u8 nvme_get_adrfam(struct nvme_ctrl *ctrl)
>> +{
>> +    u8                       adrfam = NVMF_ADDR_FAMILY_IP4;
>> +    struct sockaddr_storage  sas;
>> +    struct sockaddr         *sa = (struct sockaddr *)&sas;
>> +    struct nvme_tcp_queue   *queue = &to_tcp_ctrl(ctrl)->queues[0];
>> +
>> +    if (kernel_getsockname(queue->sock, sa) == 0) {
>> +        if (sa->sa_family == AF_INET6)
>> +            adrfam = NVMF_ADDR_FAMILY_IP6;
>> +    } else {
>> +        unsigned char buf[sizeof(struct in6_addr)];
>> +
>> +        if (in6_pton(ctrl->opts->traddr,
>> +                 strlen(ctrl->opts->traddr), buf, -1, NULL) > 0)
>> +            adrfam = NVMF_ADDR_FAMILY_IP6;
>> +    }
>> +
>> +    return adrfam;
>> +}
>> +
>>   static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
>>   {
>>       struct nvmf_ctrl_options *opts = ctrl->opts;
>> @@ -2045,6 +2079,20 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl 
>> *ctrl, bool new)
>>           goto destroy_io;
>>       }
>> +    if (ctrl->opts->explicit_registration &&
>> ++        ((ctrl->dctype == NVME_DCTYPE_DDC) ||
>> +         (ctrl->dctype == NVME_DCTYPE_CDC))) {
>> +        /* We're registering our source address with the DC. To do
>> +         * that, we can simply send an empty string. This tells the DC
>> +         * to retrieve the source address from the socket and use that
>> +         * as the registration address.
>> +         */
>> +        union nvmf_tsas tsas = {.tcp.sectype = NVMF_TCP_SECTYPE_NONE};
>> +
>> +        nvmf_disc_info_mgmt(ctrl, NVME_TAS_REGISTER, NVMF_TRTYPE_TCP,
>> +                    nvme_get_adrfam(ctrl), "", &tsas);
>> +    }
>> +
>>       nvme_start_ctrl(ctrl);
>>       return 0;
>> @@ -2613,7 +2661,8 @@ static struct nvmf_transport_ops 
>> nvme_tcp_transport = {
>>                 NVMF_OPT_HOST_TRADDR | NVMF_OPT_CTRL_LOSS_TMO |
>>                 NVMF_OPT_HDR_DIGEST | NVMF_OPT_DATA_DIGEST |
>>                 NVMF_OPT_NR_WRITE_QUEUES | NVMF_OPT_NR_POLL_QUEUES |
>> -              NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE,
>> +              NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE |
>> +              NVMF_OPT_REGISTER,
>>       .create_ctrl    = nvme_tcp_create_ctrl,
>>   };
>> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
>> index 3b47951342f4..891615876cfa 100644
>> --- a/include/linux/nvme.h
>> +++ b/include/linux/nvme.h
>> @@ -9,6 +9,7 @@
>>   #include <linux/types.h>
>>   #include <linux/uuid.h>
>> +#include <linux/kernel.h>
>>   /* NQN names in commands fields specified one size */
>>   #define NVMF_NQN_FIELD_LEN    256
>> @@ -102,6 +103,16 @@ enum {
>>       NVMF_RDMA_CMS_RDMA_CM    = 1, /* Sockets based endpoint 
>> addressing */
>>   };
>> +/**
>> + * enum -
>> + * @NVMF_TCP_SECTYPE_NONE: No Security
>> + * @NVMF_TCP_SECTYPE_TLS:  Transport Layer Security
>> + */
>> +enum {
>> +    NVMF_TCP_SECTYPE_NONE    = 0,
>> +    NVMF_TCP_SECTYPE_TLS    = 1,
>> +};
>> +
>>   #define NVME_AQ_DEPTH        32
>>   #define NVME_NR_AEN_COMMANDS    1
>>   #define NVME_AQ_BLK_MQ_DEPTH    (NVME_AQ_DEPTH - NVME_NR_AEN_COMMANDS)
>> @@ -1036,6 +1047,7 @@ enum nvme_admin_opcode {
>>       nvme_admin_sanitize_nvm        = 0x84,
>>       nvme_admin_get_lba_status    = 0x86,
>>       nvme_admin_vendor_start        = 0xC0,
>> +    nvme_admin_dim            = 0x21,
>>   };
>>   #define nvme_admin_opcode_name(opcode)    { opcode, #opcode }
>> @@ -1316,6 +1328,29 @@ struct nvmf_common_command {
>>       __u8    ts[24];
>>   };
>> +/**
>> + * union nvmf_tsas - Transport Specific Address Subtype
>> + * @qptype:  Queue Pair Service Type (NVMF_RDMA_QPTYPE_CONNECTED, 
>> NVMF_RDMA_QPTYPE_DATAGRAM)
>> + * @prtype:  Provider Type (see enum  nvme_rdma_prtype)
>> + * @cma:     Connection Management Service (NVMF_RDMA_CMS_RDMA_CM)
>> + * @pkey:    Partition Key
>> + * @sectype: Security Type (NVMF_TCP_SECTYPE_NONE, NVMF_TCP_SECTYPE_TLS)
>> + */
>> +union nvmf_tsas {
>> +    char        common[NVMF_TSAS_SIZE];
>> +    struct rdma {
>> +        __u8    qptype;
>> +        __u8    prtype;
>> +        __u8    cms;
>> +        __u8    rsvd3[5];
>> +        __u16    pkey;
>> +        __u8    rsvd10[246];
>> +    } rdma;
>> +    struct tcp {
>> +        __u8    sectype;
>> +    } tcp;
>> +};
>> +
>>   /*
>>    * The legal cntlid range a NVMe Target will provide.
>>    * Note that cntlid of value 0 is considered illegal in the fabrics 
>> world.
>> @@ -1349,17 +1384,7 @@ struct nvmf_disc_rsp_page_entry {
>>       __u8        resv64[192];
>>       char        subnqn[NVMF_NQN_FIELD_LEN];
>>       char        traddr[NVMF_TRADDR_SIZE];
>> -    union tsas {
>> -        char        common[NVMF_TSAS_SIZE];
>> -        struct rdma {
>> -            __u8    qptype;
>> -            __u8    prtype;
>> -            __u8    cms;
>> -            __u8    resv3[5];
>> -            __u16    pkey;
>> -            __u8    resv10[246];
>> -        } rdma;
>> -    } tsas;
>> +    union nvmf_tsas tsas;
>>   };
>>   /* Discovery log page header */
>> @@ -1447,6 +1472,213 @@ struct streams_directive_params {
>>       __u8    rsvd2[6];
>>   };
>> +/**
>> + * Discovery Information Management (DIM) command. This is sent by a
>> + * host to the Central Discovery Controller (CDC) to perform
>> + * explicit registration.
>> + */
>> +enum nvmf_dim_tas {
>> +    NVME_TAS_REGISTER   = 0x00,
>> +    NVME_TAS_DEREGISTER = 0x01,
>> +    NVME_TAS_UPDATE     = 0x02,
>> +};
>> +
>> +struct nvmf_dim_command {
>> +    __u8                  opcode;     /* nvme_admin_dim */
>> +    __u8                  flags;
>> +    __u16                 command_id;
>> +    __le32                nsid;
>> +    __u64                 rsvd2[2];
>> +    union nvme_data_ptr   dptr;
>> +    __le32                cdw10;      /* enum nvmf_dim_tas */
>> +    __le32                cdw11;
>> +    __le32                cdw12;
>> +    __le32                cdw13;
>> +    __le32                cdw14;
>> +    __le32                cdw15;
>> +};
>> +
>> +#define NVMF_ENAME_LEN    256
>> +#define NVMF_EVER_LEN     64
>> +
>> +enum nvmf_dim_entfmt {
>> +    NVME_ENTFMT_BASIC    = 0x01,
>> +    NVME_ENTFMT_EXTENDED = 0x02,
>> +};
>> +
>> +enum nvmf_dim_etype {
>> +    NVME_REGISTRATION_HOST = 0x01,
>> +    NVME_REGISTRATION_DDC  = 0x02,
>> +    NVME_REGISTRATION_CDC  = 0x03,
>> +};
>> +
>> +enum nvmf_exattype {
>> +    NVME_EXATTYPE_HOSTID   = 0x01, /* Host Identifier */
>> +    NVME_EXATTYPE_SYMNAME  = 0x02, /* Symbolic Name */
>> +};
>> +
>> +/**
>> + * struct nvmf_ext_attr - Extended Attribute (EXAT)
>> + *
>> + *       Bytes       Description
>> + * @type 01:00       Extended Attribute Type (EXATTYPE) - enum 
>> nvmf_exattype
>> + * @len  03:02       Extended Attribute Length (EXATLEN)
>> + * @val  (EXATLEN-1) Extended Attribute Value (EXATVAL) - size allocated
>> + *       +4:04       for array must be a multiple of 4 bytes
>> + */
>> +struct nvmf_ext_attr {
>> +    __le16            type;
>> +    __le16            len;
>> +    __u8            val[];
>> +};
>> +#define SIZEOF_EXT_EXAT_WO_VAL offsetof(struct nvmf_ext_attr, val)
>> +
>> +/**
>> + * struct nvmf_ext_die - Extended Discovery Information Entry (DIE)
>> + *
>> + *            Bytes           Description
>> + * @trtype    00              Transport Type (enum nvme_trtype)
>> + * @adrfam    01              Address Family (enum nvmf_addr_familiy)
>> + * @subtype   02              Subsystem Type (enum nvme_subsys_type)
>> + * @treq      03              Transport Requirements (enum nvmf_treq)
>> + * @portid    05:04           Port ID
>> + * @cntlid    07:06           Controller ID
>> + * @asqsz     09:08           Admin Max SQ Size
>> + *            31:10           Reserved
>> + * @trsvcid   63:32           Transport Service Identifier
>> + *            255:64          Reserved
>> + * @nqn       511:256         NVM Qualified Name
>> + * @traddr    767:512         Transport Address
>> + * @tsas      1023:768        Transport Specific Address Subtype
>> + * @tel       1027:1024       Total Entry Length
>> + * @numexat   1029:1028       Number of Extended Attributes
>> + *            1031:1030       Reserved
>> + * @exat[0]   ((EXATLEN-1)+   Extented Attributes 0 (see @numexat)
>> + *            4)+1032:1032    Cannot access as an array because each 
>> EXAT
>> + *                            entry has an undetermined size.
>> + * @exat[N]   TEL-1:TEL-      Extented Attributes N (w/ N = NUMEXAT-1)
>> + *            (EXATLEN+4)
>> + */
>> +struct nvmf_ext_die {
>> +    __u8            trtype;
>> +    __u8            adrfam;
>> +    __u8            subtype;
>> +    __u8            treq;
>> +    __le16            portid;
>> +    __le16            cntlid;
>> +    __le16            asqsz;
>> +    __u8            resv10[22];
>> +    char            trsvcid[NVMF_TRSVCID_SIZE];
>> +    __u8            resv64[192];
>> +    char            nqn[NVMF_NQN_FIELD_LEN];
>> +    char            traddr[NVMF_TRADDR_SIZE];
>> +    union nvmf_tsas        tsas;
>> +    __le32            tel;
>> +    __le16            numexat;
>> +    __u16            resv1030;
>> +    struct nvmf_ext_attr    exat;
>> +};
>> +#define SIZEOF_EXT_DIE_WO_EXAT offsetof(struct nvmf_ext_die, exat)
>> +
>> +/**
>> + * union nvmf_die - Discovery Information Entry (DIE)
>> + *
>> + * Depending on the ENTFMT specified in the DIM, DIEs can be entered 
>> with
>> + * the Basic or Extended formats. For Basic format, each entry has a 
>> fixed
>> + * length. Therefore, the "basic" field defined below can be accessed 
>> as a
>> + * C array. For the Extended format, however, each entry is of variable
>> + * length (TEL). Therefore, the "extended" field defined below cannot be
>> + * accessed as a C array. Instead, the "extended" field is akin to a
>> + * linked-list, where one can "walk" through the list. To move to the 
>> next
>> + * entry, one simply adds the current entry's length (TEL) to the "walk"
>> + * pointer. The number of entries in the list is specified by NUMENT.
>> + * Although extended entries are of a variable lengths (TEL), TEL is
>> + * always a multiple of 4 bytes.
>> + */
>> +union nvmf_die {
>> +    struct nvmf_disc_rsp_page_entry    basic[0];
>> +    struct nvmf_ext_die        extended;
>> +};
>> +
>> +/**
>> + * struct nvmf_dim_data - Discovery Information Management (DIM) - Data
>> + *
>> + *            Bytes       Description
>> + * @tdl       03:00       Total Data Length
>> + *            07:04       Reserved
>> + * @nument    15:08       Number of entries
>> + * @entfmt    17:16       Entry Format (enum nvmf_dim_entfmt)
>> + * @etype     19:18       Entity Type (enum nvmf_dim_etype)
>> + * @portlcL   20          Port Local
>> + *            21          Reserved
>> + * @ektype    23:22       Entry Key Type
>> + * @eid       279:24      Entity Identifier (e.g. Host NQN)
>> + * @ename     535:280     Entity Name (e.g. hostname)
>> + * @ever      599:536     Entity Version (e.g. OS Name/Version)
>> + *            1023:600    Reserved
>> + * @die       TDL-1:1024  Discovery Information Entry (see @nument 
>> above)
>> + */
>> +struct nvmf_dim_data {
>> +    __le32            tdl;
>> +    __u32            rsvd4;
>> +    __le64            nument;
>> +    __le16            entfmt;
>> +    __le16            etype;
>> +    __u8            portlcl;
>> +    __u8            rsvd21;
>> +    __le16            ektype;
>> +    char            eid[NVMF_NQN_FIELD_LEN];
>> +    char            ename[NVMF_ENAME_LEN];
>> +    char            ever[NVMF_EVER_LEN];
>> +    __u8            rsvd600[424];
>> +    union nvmf_die        die;
>> +};
>> +#define SIZEOF_DIM_WO_DIE offsetof(struct nvmf_dim_data, die)
>> +
>> +/**
>> + * exat_len() - Return the size in bytes, rounded to a multiple of 4 
>> ((i.e.
>> + *         size of u32), of the buffer needed to hold the exat value of
>> + *         size @val_len.
>> + */
>> +static inline u16 exat_len(size_t val_len)
>> +{
>> +    return (u16)round_up(val_len, sizeof(u32));
>> +}
>> +
>> +/**
>> + * exat_size - return the size of the "struct nvmf_ext_attr"
>> + *   needed to hold a value of size @val_len.
>> + *
>> + * @val_len: This is the length of the data to be copied to the "val"
>> + *         field of a "struct nvmf_ext_attr".
>> + *
>> + * @return The size in bytes, rounded to a multiple of 4 (i.e. size of
>> + *         u32), of the "struct nvmf_ext_attr" required to hold a 
>> string of
>> + *         length @val_len.
>> + */
>> +static inline u16 exat_size(size_t val_len)
>> +{
>> +    return (u16)(SIZEOF_EXT_EXAT_WO_VAL + exat_len(val_len));
>> +}
>> +
>> +/**
>> + * exat_ptr_next - Increment @p to the next element in the array. 
>> Extended
>> + *   attributes are saved to an array of "struct nvmf_ext_attr" where 
>> each
>> + *   element of the array is of variable size. In order to move to 
>> the next
>> + *   element in the array one must increment the pointer to the current
>> + *   element (@p) by the size of the current element.
>> + *
>> + * @p: Pointer to an element of an array of "struct nvmf_ext_attr".
>> + *
>> + * @return Pointer to the next element in the array.
>> + */
>> +static inline struct nvmf_ext_attr *exat_ptr_next(struct 
>> nvmf_ext_attr *p)
>> +{
>> +    return (struct nvmf_ext_attr *)
>> +        ((uintptr_t)p + (ptrdiff_t)exat_size(le16_to_cpu(p->len)));
>> +}
>> +
>> +
>>   struct nvme_command {
>>       union {
>>           struct nvme_common_command common;
>> @@ -1470,6 +1702,7 @@ struct nvme_command {
>>           struct nvmf_property_get_command prop_get;
>>           struct nvme_dbbuf dbbuf;
>>           struct nvme_directive_cmd directive;
>> +        struct nvmf_dim_command dim;
>>       };
>>   };
> I'd rather like to have the TLS bits in a separate patch; they are not 
> strictly related to this issue.
> 
> And I have a rather general issue with this: Is it required to issue an 
> explicit registration directly after sending the 'connect' command?
> IE do we need to have it as part of the 'connect' routine?
> 
> Thing is, the linux 'connect' routine is doing several things in one go
> (like establishing a transport association for the admin queue, send the 
> 'connect' command for the admin queue, create transport associations for 
> I/O queues and send 'connect' commands on all I/O queues, _and_ possibly 
> do authentication, too).
> If we now start overloading it with yet more functionality it becomes 
> extremely hard to cleanup after an error, and that doesn't even mention 
> the non-existing error reporting to userspace.
> 
> Wouldn't it make more sense to delegate explicit registration to 
> userspace (ie libnvme/nvme-cli), and leave the kernel out of it?

It would. There is no reason what-so-ever to place all this register
stuff needs to live in the kernel. We have a way to passthru commands so
it can and should move to userspace.

RE: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Belanger, Martin]

> >> TP8010 introduces Central Discovery Controllers (CDC) and the
> >> Discovery Information Management (DIM) PDU, which is used to perform
> >> explicit registration with Discovery Controllers.
> >>
> >> This patch defines the DIM PDU and adds logic to perform explicit
> >> registration with DCs when registration has been requested by the
> >> user.
> >>
> >> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> >> ---
> >>   drivers/nvme/host/fabrics.c | 164 +++++++++++++++++++++++
> >>   drivers/nvme/host/fabrics.h |   9 ++
> >>   drivers/nvme/host/tcp.c     |  51 +++++++-
> >>   include/linux/nvme.h        | 255
> >> ++++++++++++++++++++++++++++++++++--
> >>   4 files changed, 467 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/nvme/host/fabrics.c
> >> b/drivers/nvme/host/fabrics.c index 040a6cce6afa..e6fc2f85b670 100644
> >> --- a/drivers/nvme/host/fabrics.c
> >> +++ b/drivers/nvme/host/fabrics.c
> >> @@ -10,6 +10,7 @@
> >>   #include <linux/mutex.h>
> >>   #include <linux/parser.h>
> >>   #include <linux/seq_file.h>
> >> +#include <linux/utsname.h>
> >>   #include "nvme.h"
> >>   #include "fabrics.h"
> >> @@ -526,6 +527,165 @@ static struct nvmf_transport_ops
> >> *nvmf_lookup_transport(
> >>       return NULL;
> >>   }
> >> +/**
> >> + * nvmf_get_tel() - Calculate the amount of memory needed for a
> >> Discovery
> >> + * Information Entry. Each Entry must contain at a minimum an
> >> + Extended
> >> + * Attribute for the HostID. The Entry may optionally contain an
> >> Extended
> >> + * Attribute for the Symbolic Name.
> >> + *
> >> + * @hostsymname - Symbolic name (may be NULL)
> >> + *
> >> + * Return Total Entry Length
> >> + */
> >> +static u32 nvmf_get_tel(const char *hostsymname) {
> >> +    u32 tel = SIZEOF_EXT_DIE_WO_EXAT;
> >> +    u16 len;
> >> +
> >> +    /* Host ID is mandatory */
> >> +    tel += exat_size(sizeof(uuid_t));
> >> +
> >> +    /* Symbolic name is optional */
> >> +    len = hostsymname ? strlen(hostsymname) : 0;
> >> +    if (len)
> >> +        tel += exat_size(len);
> >> +
> >> +    return tel;
> >> +}
> >> +
> >> +/**
> >> + * nvmf_fill_die() - Fill the Discovery Information Entry pointed to
> >> +by
> >> + *                   @die.
> >> + *
> >> + * @die - Pointer to Discovery Information Entry to be filled
> >> + * @tel - Length of the DIE
> >> + * @trtype - Transport type
> >> + * @adrfam - Address family
> >> + * @reg_addr - Address to register. Setting this to an empty string
> >> tells
> >> + *       the DC to infer address from the source address of the socket.
> >> + * @nqn - Host NQN
> >> + * @hostid - Host ID
> >> + * @hostsymname - Host symbolic name
> >> + * @tsas - Transport Specific Address Subtype for the address being
> >> + *       registered.
> >> + */
> >> +static void nvmf_fill_die(struct nvmf_ext_die  *die,
> >> +              u32                   tel,
> >> +              u8                    trtype,
> >> +              u8                    adrfam,
> >> +              const char           *reg_addr,
> >> +              struct nvmf_host     *host,
> >> +              union nvmf_tsas      *tsas) {
> >> +    u16 numexat = 0;
> >> +    size_t symname_len;
> >> +    struct nvmf_ext_attr *exat;
> >> +
> >> +    die->tel = cpu_to_le32(tel);
> >> +    die->trtype = trtype;
> >> +    die->adrfam = adrfam;
> >> +
> >> +    strncpy(die->nqn, host->nqn, sizeof(die->nqn));
> >> +    strncpy(die->traddr, reg_addr, sizeof(die->traddr));
> >> +    memcpy(&die->tsas, tsas, sizeof(die->tsas));
> >> +
> >> +    /* Extended Attribute for the HostID (mandatory) */
> >> +    numexat++;
> >> +    exat = &die->exat;
> >> +    exat->type = cpu_to_le16(NVME_EXATTYPE_HOSTID);
> >> +    exat->len  = cpu_to_le16(exat_len(sizeof(host->id)));
> >> +    uuid_copy((uuid_t *)exat->val, &host->id);
> >> +
> >> +    /* Extended Attribute for the Symbolic Name (optional) */
> >> +    symname_len = strlen(host->symname);
> >> +    if (symname_len) {
> >> +        u16 exatlen = exat_len(symname_len);
> >> +
> >> +        numexat++;
> >> +        exat = exat_ptr_next(exat);
> >> +        exat->type = cpu_to_le16(NVME_EXATTYPE_SYMNAME);
> >> +        exat->len  = cpu_to_le16(exatlen);
> >> +        memcpy(exat->val, host->symname, symname_len);
> >> +        /* Per Base specs, ASCII strings must be padded with spaces
> >> +*/
> >> +        memset(&exat->val[symname_len], ' ', exatlen - symname_len);
> >> +    }
> >> +
> >> +    die->numexat = cpu_to_le16(numexat); }
> >> +
> >> +/**
> >> + * nvmf_disc_info_mgmt() - Perform explicit registration,
> >> deregistration,
> >> + * or registration-update (specified by @tas) by sending a Discovery
> >> + * Information Management (DIM) command to the Discovery Controller
> >> (DC).
> >> + *
> >> + * @ctrl: Host NVMe controller instance maintaining the admin queue
> >> used to
> >> + *      submit the DIM command to the DC.
> >> + * @tas: Task field of the Command Dword 10 (cdw10). Indicates
> >> whether to
> >> + *     perform a Registration, Deregistration, or Registration-update.
> >> + * @trtype: Transport type (e.g. NVMF_TRTYPE_TCP)
> >> + * @adrfam: Address family (e.g. NVMF_ADDR_FAMILY_IP6)
> >> + * @reg_addr - Address to register. Setting this to an empty string
> >> tells
> >> + *       the DC to infer address from the source address of the socket.
> >> + * @tsas - Transport Specific Address Subtype for the address being
> >> + *       registered.
> >> + *
> >> + * Return:   0: success,
> >> + *         > 0: NVMe error status code,
> >> + *         < 0: Linux errno error code  */ int
> >> +nvmf_disc_info_mgmt(struct nvme_ctrl *ctrl,
> >> +            enum nvmf_dim_tas tas,
> >> +            u8                trtype,
> >> +            u8                adrfam,
> >> +            const char       *reg_addr,
> >> +            union nvmf_tsas  *tsas)
> >> +{
> >> +    int                   ret;
> >> +    u32                   tdl;  /* Total Data Length */
> >> +    u32                   tel;  /* Total Entry Length */
> >> +    struct nvmf_dim_data *dim;  /* Discovery Information Management
> >> +*/
> >> +    struct nvmf_ext_die  *die;  /* Discovery Information Entry */
> >> +    struct nvme_command   cmd = {};
> >> +    union nvme_result     result;
> >> +
> >> +    /* Register 1 Discovery Information Entry (DIE) of size TEL */
> >> +    tel = nvmf_get_tel(ctrl->opts->host->symname);
> >> +    tdl = SIZEOF_DIM_WO_DIE + tel;
> >> +
> >> +    dim = kzalloc(tdl, GFP_KERNEL);
> >> +    if (!dim)
> >> +        return -ENOMEM;
> >> +
> >> +    cmd.dim.opcode = nvme_admin_dim;
> >> +    cmd.dim.cdw10  = cpu_to_le32(tas);
> >> +
> >> +    dim->tdl    = cpu_to_le32(tdl);
> >> +    dim->nument = cpu_to_le64(1);    /* only 1 DIE to register */
> >> +    dim->entfmt = cpu_to_le16(NVME_ENTFMT_EXTENDED);
> >> +    dim->etype  = cpu_to_le16(NVME_REGISTRATION_HOST);
> >> +    dim->ektype = cpu_to_le16(0x5F); /* must be 0x5F per specs */
> >> +
> >> +    strncpy(dim->eid, ctrl->opts->host->nqn, sizeof(dim->eid));
> >> +    strncpy(dim->ename, utsname()->nodename, sizeof(dim->ename));
> >> +    strncpy(dim->ever, utsname()->release, sizeof(dim->ever));
> >> +
> >> +    die = &dim->die.extended;
> >> +    nvmf_fill_die(die, tel, trtype, adrfam, reg_addr,
> >> +              ctrl->opts->host, tsas);
> >> +
> >> +    ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &result,
> >> +                     dim, tdl, 0, NVME_QID_ANY, 1, 0);
> >> +    if (unlikely(ret)) {
> >> +        dev_err(ctrl->device, "DIM error Result:0x%04X
> >> +Status:0x%04X\n",
> >> +           le32_to_cpu(result.u32), ret);
> >> +    }
> >> +
> >> +    kfree(dim);
> >> +
> >> +    return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(nvmf_disc_info_mgmt);
> >> +
> >>   static const match_table_t opt_tokens = {
> >>       { NVMF_OPT_TRANSPORT,        "transport=%s"        },
> >>       { NVMF_OPT_TRADDR,        "traddr=%s"        }, @@ -550,6
> >> +710,7 @@ static const match_table_t opt_tokens = {
> >>       { NVMF_OPT_TOS,            "tos=%d"        },
> >>       { NVMF_OPT_FAIL_FAST_TMO,    "fast_io_fail_tmo=%d"    },
> >>       { NVMF_OPT_DISCOVERY,        "discovery"        },
> >> +    { NVMF_OPT_REGISTER,        "register"        },
> >>       { NVMF_OPT_ERR,            NULL            }
> >>   };
> >> @@ -831,6 +992,9 @@ static int nvmf_parse_options(struct
> >> nvmf_ctrl_options *opts,
> >>           case NVMF_OPT_DISCOVERY:
> >>               opts->discovery_nqn = true;
> >>               break;
> >> +        case NVMF_OPT_REGISTER:
> >> +            opts->explicit_registration = true;
> >> +            break;
> >>           case NVMF_OPT_SYMNAME:
> >>               hostsymname = match_strdup(args);
> >>               if (!hostsymname) {
> >> diff --git a/drivers/nvme/host/fabrics.h
> >> b/drivers/nvme/host/fabrics.h index 494e6dbe233a..77f5768f8ff4 100644
> >> --- a/drivers/nvme/host/fabrics.h
> >> +++ b/drivers/nvme/host/fabrics.h
> >> @@ -70,6 +70,7 @@ enum {
> >>       NVMF_OPT_HOST_IFACE    = 1 << 21,
> >>       NVMF_OPT_DISCOVERY    = 1 << 22,
> >>       NVMF_OPT_SYMNAME    = 1 << 23,
> >> +    NVMF_OPT_REGISTER    = 1 << 24,
> >>   };
> >>   /**
> >> @@ -106,6 +107,7 @@ enum {
> >>    * @nr_poll_queues: number of queues for polling I/O
> >>    * @tos: type of service
> >>    * @fast_io_fail_tmo: Fast I/O fail timeout in seconds
> >> + * @explicit_registration: Explicit Registration with DC using the
> >> DIM PDU
> >>    */
> >>   struct nvmf_ctrl_options {
> >>       unsigned        mask;
> >> @@ -130,6 +132,7 @@ struct nvmf_ctrl_options {
> >>       unsigned int        nr_poll_queues;
> >>       int            tos;
> >>       int            fast_io_fail_tmo;
> >> +    bool            explicit_registration;
> >>   };
> >>   /*
> >> @@ -200,5 +203,11 @@ int nvmf_get_address(struct nvme_ctrl *ctrl,
> >> char *buf, int size);
> >>   bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
> >>   bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
> >>           struct nvmf_ctrl_options *opts);
> >> +int nvmf_disc_info_mgmt(struct nvme_ctrl *ctrl,
> >> +            enum nvmf_dim_tas tas,
> >> +            u8                trtype,
> >> +            u8                adrfam,
> >> +            const char       *reg_addr,
> >> +            union nvmf_tsas  *tsas);
> >>   #endif /* _NVME_FABRICS_H */
> >> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c index
> >> 4ceb28675fdf..970b7df574a4 100644
> >> --- a/drivers/nvme/host/tcp.c
> >> +++ b/drivers/nvme/host/tcp.c
> >> @@ -1993,6 +1993,40 @@ static void
> >> nvme_tcp_reconnect_or_remove(struct
> >> nvme_ctrl *ctrl)
> >>       }
> >>   }
> >> +/**
> >> + * nvme_get_adrfam() - Get address family for the address we're
> >> registering
> >> + * with the DC. We retrieve this info from the socket itself. If we
> >> can't
> >> + * get the source address from the socket, then we'll infer the
> >> + address
> >> + * family from the address of the DC since the DC address has the
> >> + same
> >> + * address family.
> >> + *
> >> + * @ctrl: Host NVMe controller instance maintaining the admin queue
> >> used to
> >> + *      submit the DIM command to the DC.
> >> + *
> >> + * Return: The address family of the source address associated with
> >> +the
> >> + * socket connected to the DC.
> >> + */
> >> +static u8 nvme_get_adrfam(struct nvme_ctrl *ctrl) {
> >> +    u8                       adrfam = NVMF_ADDR_FAMILY_IP4;
> >> +    struct sockaddr_storage  sas;
> >> +    struct sockaddr         *sa = (struct sockaddr *)&sas;
> >> +    struct nvme_tcp_queue   *queue = &to_tcp_ctrl(ctrl)->queues[0];
> >> +
> >> +    if (kernel_getsockname(queue->sock, sa) == 0) {
> >> +        if (sa->sa_family == AF_INET6)
> >> +            adrfam = NVMF_ADDR_FAMILY_IP6;
> >> +    } else {
> >> +        unsigned char buf[sizeof(struct in6_addr)];
> >> +
> >> +        if (in6_pton(ctrl->opts->traddr,
> >> +                 strlen(ctrl->opts->traddr), buf, -1, NULL) > 0)
> >> +            adrfam = NVMF_ADDR_FAMILY_IP6;
> >> +    }
> >> +
> >> +    return adrfam;
> >> +}
> >> +
> >>   static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
> >>   {
> >>       struct nvmf_ctrl_options *opts = ctrl->opts; @@ -2045,6
> >> +2079,20 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl,
> >> bool new)
> >>           goto destroy_io;
> >>       }
> >> +    if (ctrl->opts->explicit_registration &&
> >> ++        ((ctrl->dctype == NVME_DCTYPE_DDC) ||
> >> +         (ctrl->dctype == NVME_DCTYPE_CDC))) {
> >> +        /* We're registering our source address with the DC. To do
> >> +         * that, we can simply send an empty string. This tells the
> >> +DC
> >> +         * to retrieve the source address from the socket and use
> >> +that
> >> +         * as the registration address.
> >> +         */
> >> +        union nvmf_tsas tsas = {.tcp.sectype =
> >> +NVMF_TCP_SECTYPE_NONE};
> >> +
> >> +        nvmf_disc_info_mgmt(ctrl, NVME_TAS_REGISTER,
> >> +NVMF_TRTYPE_TCP,
> >> +                    nvme_get_adrfam(ctrl), "", &tsas);
> >> +    }
> >> +
> >>       nvme_start_ctrl(ctrl);
> >>       return 0;
> >> @@ -2613,7 +2661,8 @@ static struct nvmf_transport_ops
> >> nvme_tcp_transport = {
> >>                 NVMF_OPT_HOST_TRADDR | NVMF_OPT_CTRL_LOSS_TMO |
> >>                 NVMF_OPT_HDR_DIGEST | NVMF_OPT_DATA_DIGEST |
> >>                 NVMF_OPT_NR_WRITE_QUEUES |
> NVMF_OPT_NR_POLL_QUEUES |
> >> -              NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE,
> >> +              NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE |
> >> +              NVMF_OPT_REGISTER,
> >>       .create_ctrl    = nvme_tcp_create_ctrl,
> >>   };
> >> diff --git a/include/linux/nvme.h b/include/linux/nvme.h index
> >> 3b47951342f4..891615876cfa 100644
> >> --- a/include/linux/nvme.h
> >> +++ b/include/linux/nvme.h
> >> @@ -9,6 +9,7 @@
> >>   #include <linux/types.h>
> >>   #include <linux/uuid.h>
> >> +#include <linux/kernel.h>
> >>   /* NQN names in commands fields specified one size */
> >>   #define NVMF_NQN_FIELD_LEN    256
> >> @@ -102,6 +103,16 @@ enum {
> >>       NVMF_RDMA_CMS_RDMA_CM    = 1, /* Sockets based endpoint
> >> addressing */
> >>   };
> >> +/**
> >> + * enum -
> >> + * @NVMF_TCP_SECTYPE_NONE: No Security
> >> + * @NVMF_TCP_SECTYPE_TLS:  Transport Layer Security  */ enum {
> >> +    NVMF_TCP_SECTYPE_NONE    = 0,
> >> +    NVMF_TCP_SECTYPE_TLS    = 1,
> >> +};
> >> +
> >>   #define NVME_AQ_DEPTH        32
> >>   #define NVME_NR_AEN_COMMANDS    1
> >>   #define NVME_AQ_BLK_MQ_DEPTH    (NVME_AQ_DEPTH -
> >> NVME_NR_AEN_COMMANDS) @@ -1036,6 +1047,7 @@ enum
> nvme_admin_opcode {
> >>       nvme_admin_sanitize_nvm        = 0x84,
> >>       nvme_admin_get_lba_status    = 0x86,
> >>       nvme_admin_vendor_start        = 0xC0,
> >> +    nvme_admin_dim            = 0x21,
> >>   };
> >>   #define nvme_admin_opcode_name(opcode)    { opcode, #opcode }
> @@
> >> -1316,6 +1328,29 @@ struct nvmf_common_command {
> >>       __u8    ts[24];
> >>   };
> >> +/**
> >> + * union nvmf_tsas - Transport Specific Address Subtype
> >> + * @qptype:  Queue Pair Service Type
> (NVMF_RDMA_QPTYPE_CONNECTED,
> >> NVMF_RDMA_QPTYPE_DATAGRAM)
> >> + * @prtype:  Provider Type (see enum  nvme_rdma_prtype)
> >> + * @cma:     Connection Management Service
> (NVMF_RDMA_CMS_RDMA_CM)
> >> + * @pkey:    Partition Key
> >> + * @sectype: Security Type (NVMF_TCP_SECTYPE_NONE,
> >> +NVMF_TCP_SECTYPE_TLS)  */ union nvmf_tsas {
> >> +    char        common[NVMF_TSAS_SIZE];
> >> +    struct rdma {
> >> +        __u8    qptype;
> >> +        __u8    prtype;
> >> +        __u8    cms;
> >> +        __u8    rsvd3[5];
> >> +        __u16    pkey;
> >> +        __u8    rsvd10[246];
> >> +    } rdma;
> >> +    struct tcp {
> >> +        __u8    sectype;
> >> +    } tcp;
> >> +};
> >> +
> >>   /*
> >>    * The legal cntlid range a NVMe Target will provide.
> >>    * Note that cntlid of value 0 is considered illegal in the fabrics
> >> world.
> >> @@ -1349,17 +1384,7 @@ struct nvmf_disc_rsp_page_entry {
> >>       __u8        resv64[192];
> >>       char        subnqn[NVMF_NQN_FIELD_LEN];
> >>       char        traddr[NVMF_TRADDR_SIZE];
> >> -    union tsas {
> >> -        char        common[NVMF_TSAS_SIZE];
> >> -        struct rdma {
> >> -            __u8    qptype;
> >> -            __u8    prtype;
> >> -            __u8    cms;
> >> -            __u8    resv3[5];
> >> -            __u16    pkey;
> >> -            __u8    resv10[246];
> >> -        } rdma;
> >> -    } tsas;
> >> +    union nvmf_tsas tsas;
> >>   };
> >>   /* Discovery log page header */
> >> @@ -1447,6 +1472,213 @@ struct streams_directive_params {
> >>       __u8    rsvd2[6];
> >>   };
> >> +/**
> >> + * Discovery Information Management (DIM) command. This is sent by a
> >> + * host to the Central Discovery Controller (CDC) to perform
> >> + * explicit registration.
> >> + */
> >> +enum nvmf_dim_tas {
> >> +    NVME_TAS_REGISTER   = 0x00,
> >> +    NVME_TAS_DEREGISTER = 0x01,
> >> +    NVME_TAS_UPDATE     = 0x02,
> >> +};
> >> +
> >> +struct nvmf_dim_command {
> >> +    __u8                  opcode;     /* nvme_admin_dim */
> >> +    __u8                  flags;
> >> +    __u16                 command_id;
> >> +    __le32                nsid;
> >> +    __u64                 rsvd2[2];
> >> +    union nvme_data_ptr   dptr;
> >> +    __le32                cdw10;      /* enum nvmf_dim_tas */
> >> +    __le32                cdw11;
> >> +    __le32                cdw12;
> >> +    __le32                cdw13;
> >> +    __le32                cdw14;
> >> +    __le32                cdw15;
> >> +};
> >> +
> >> +#define NVMF_ENAME_LEN    256
> >> +#define NVMF_EVER_LEN     64
> >> +
> >> +enum nvmf_dim_entfmt {
> >> +    NVME_ENTFMT_BASIC    = 0x01,
> >> +    NVME_ENTFMT_EXTENDED = 0x02,
> >> +};
> >> +
> >> +enum nvmf_dim_etype {
> >> +    NVME_REGISTRATION_HOST = 0x01,
> >> +    NVME_REGISTRATION_DDC  = 0x02,
> >> +    NVME_REGISTRATION_CDC  = 0x03,
> >> +};
> >> +
> >> +enum nvmf_exattype {
> >> +    NVME_EXATTYPE_HOSTID   = 0x01, /* Host Identifier */
> >> +    NVME_EXATTYPE_SYMNAME  = 0x02, /* Symbolic Name */ };
> >> +
> >> +/**
> >> + * struct nvmf_ext_attr - Extended Attribute (EXAT)
> >> + *
> >> + *       Bytes       Description
> >> + * @type 01:00       Extended Attribute Type (EXATTYPE) - enum
> >> nvmf_exattype
> >> + * @len  03:02       Extended Attribute Length (EXATLEN)
> >> + * @val  (EXATLEN-1) Extended Attribute Value (EXATVAL) - size
> >> +allocated
> >> + *       +4:04       for array must be a multiple of 4 bytes  */
> >> +struct nvmf_ext_attr {
> >> +    __le16            type;
> >> +    __le16            len;
> >> +    __u8            val[];
> >> +};
> >> +#define SIZEOF_EXT_EXAT_WO_VAL offsetof(struct nvmf_ext_attr, val)
> >> +
> >> +/**
> >> + * struct nvmf_ext_die - Extended Discovery Information Entry (DIE)
> >> + *
> >> + *            Bytes           Description
> >> + * @trtype    00              Transport Type (enum nvme_trtype)
> >> + * @adrfam    01              Address Family (enum
> >> +nvmf_addr_familiy)
> >> + * @subtype   02              Subsystem Type (enum nvme_subsys_type)
> >> + * @treq      03              Transport Requirements (enum
> >> +nvmf_treq)
> >> + * @portid    05:04           Port ID
> >> + * @cntlid    07:06           Controller ID
> >> + * @asqsz     09:08           Admin Max SQ Size
> >> + *            31:10           Reserved
> >> + * @trsvcid   63:32           Transport Service Identifier
> >> + *            255:64          Reserved
> >> + * @nqn       511:256         NVM Qualified Name
> >> + * @traddr    767:512         Transport Address
> >> + * @tsas      1023:768        Transport Specific Address Subtype
> >> + * @tel       1027:1024       Total Entry Length
> >> + * @numexat   1029:1028       Number of Extended Attributes
> >> + *            1031:1030       Reserved
> >> + * @exat[0]   ((EXATLEN-1)+   Extented Attributes 0 (see @numexat)
> >> + *            4)+1032:1032    Cannot access as an array because each
> >> EXAT
> >> + *                            entry has an undetermined size.
> >> + * @exat[N]   TEL-1:TEL-      Extented Attributes N (w/ N =
> >> +NUMEXAT-1)
> >> + *            (EXATLEN+4)
> >> + */
> >> +struct nvmf_ext_die {
> >> +    __u8            trtype;
> >> +    __u8            adrfam;
> >> +    __u8            subtype;
> >> +    __u8            treq;
> >> +    __le16            portid;
> >> +    __le16            cntlid;
> >> +    __le16            asqsz;
> >> +    __u8            resv10[22];
> >> +    char            trsvcid[NVMF_TRSVCID_SIZE];
> >> +    __u8            resv64[192];
> >> +    char            nqn[NVMF_NQN_FIELD_LEN];
> >> +    char            traddr[NVMF_TRADDR_SIZE];
> >> +    union nvmf_tsas        tsas;
> >> +    __le32            tel;
> >> +    __le16            numexat;
> >> +    __u16            resv1030;
> >> +    struct nvmf_ext_attr    exat;
> >> +};
> >> +#define SIZEOF_EXT_DIE_WO_EXAT offsetof(struct nvmf_ext_die, exat)
> >> +
> >> +/**
> >> + * union nvmf_die - Discovery Information Entry (DIE)
> >> + *
> >> + * Depending on the ENTFMT specified in the DIM, DIEs can be entered
> >> with
> >> + * the Basic or Extended formats. For Basic format, each entry has a
> >> fixed
> >> + * length. Therefore, the "basic" field defined below can be
> >> + accessed
> >> as a
> >> + * C array. For the Extended format, however, each entry is of
> >> + variable
> >> + * length (TEL). Therefore, the "extended" field defined below
> >> + cannot be
> >> + * accessed as a C array. Instead, the "extended" field is akin to a
> >> + * linked-list, where one can "walk" through the list. To move to
> >> + the
> >> next
> >> + * entry, one simply adds the current entry's length (TEL) to the "walk"
> >> + * pointer. The number of entries in the list is specified by NUMENT.
> >> + * Although extended entries are of a variable lengths (TEL), TEL is
> >> + * always a multiple of 4 bytes.
> >> + */
> >> +union nvmf_die {
> >> +    struct nvmf_disc_rsp_page_entry    basic[0];
> >> +    struct nvmf_ext_die        extended; };
> >> +
> >> +/**
> >> + * struct nvmf_dim_data - Discovery Information Management (DIM) -
> >> +Data
> >> + *
> >> + *            Bytes       Description
> >> + * @tdl       03:00       Total Data Length
> >> + *            07:04       Reserved
> >> + * @nument    15:08       Number of entries
> >> + * @entfmt    17:16       Entry Format (enum nvmf_dim_entfmt)
> >> + * @etype     19:18       Entity Type (enum nvmf_dim_etype)
> >> + * @portlcL   20          Port Local
> >> + *            21          Reserved
> >> + * @ektype    23:22       Entry Key Type
> >> + * @eid       279:24      Entity Identifier (e.g. Host NQN)
> >> + * @ename     535:280     Entity Name (e.g. hostname)
> >> + * @ever      599:536     Entity Version (e.g. OS Name/Version)
> >> + *            1023:600    Reserved
> >> + * @die       TDL-1:1024  Discovery Information Entry (see @nument
> >> above)
> >> + */
> >> +struct nvmf_dim_data {
> >> +    __le32            tdl;
> >> +    __u32            rsvd4;
> >> +    __le64            nument;
> >> +    __le16            entfmt;
> >> +    __le16            etype;
> >> +    __u8            portlcl;
> >> +    __u8            rsvd21;
> >> +    __le16            ektype;
> >> +    char            eid[NVMF_NQN_FIELD_LEN];
> >> +    char            ename[NVMF_ENAME_LEN];
> >> +    char            ever[NVMF_EVER_LEN];
> >> +    __u8            rsvd600[424];
> >> +    union nvmf_die        die;
> >> +};
> >> +#define SIZEOF_DIM_WO_DIE offsetof(struct nvmf_dim_data, die)
> >> +
> >> +/**
> >> + * exat_len() - Return the size in bytes, rounded to a multiple of 4
> >> ((i.e.
> >> + *         size of u32), of the buffer needed to hold the exat value
> >> +of
> >> + *         size @val_len.
> >> + */
> >> +static inline u16 exat_len(size_t val_len) {
> >> +    return (u16)round_up(val_len, sizeof(u32)); }
> >> +
> >> +/**
> >> + * exat_size - return the size of the "struct nvmf_ext_attr"
> >> + *   needed to hold a value of size @val_len.
> >> + *
> >> + * @val_len: This is the length of the data to be copied to the "val"
> >> + *         field of a "struct nvmf_ext_attr".
> >> + *
> >> + * @return The size in bytes, rounded to a multiple of 4 (i.e. size
> >> +of
> >> + *         u32), of the "struct nvmf_ext_attr" required to hold a
> >> string of
> >> + *         length @val_len.
> >> + */
> >> +static inline u16 exat_size(size_t val_len) {
> >> +    return (u16)(SIZEOF_EXT_EXAT_WO_VAL + exat_len(val_len)); }
> >> +
> >> +/**
> >> + * exat_ptr_next - Increment @p to the next element in the array.
> >> Extended
> >> + *   attributes are saved to an array of "struct nvmf_ext_attr" where
> >> each
> >> + *   element of the array is of variable size. In order to move to
> >> the next
> >> + *   element in the array one must increment the pointer to the current
> >> + *   element (@p) by the size of the current element.
> >> + *
> >> + * @p: Pointer to an element of an array of "struct nvmf_ext_attr".
> >> + *
> >> + * @return Pointer to the next element in the array.
> >> + */
> >> +static inline struct nvmf_ext_attr *exat_ptr_next(struct
> >> nvmf_ext_attr *p)
> >> +{
> >> +    return (struct nvmf_ext_attr *)
> >> +        ((uintptr_t)p + (ptrdiff_t)exat_size(le16_to_cpu(p->len)));
> >> +}
> >> +
> >> +
> >>   struct nvme_command {
> >>       union {
> >>           struct nvme_common_command common;
> >> @@ -1470,6 +1702,7 @@ struct nvme_command {
> >>           struct nvmf_property_get_command prop_get;
> >>           struct nvme_dbbuf dbbuf;
> >>           struct nvme_directive_cmd directive;
> >> +        struct nvmf_dim_command dim;
> >>       };
> >>   };
> > I'd rather like to have the TLS bits in a separate patch; they are not
> > strictly related to this issue.
> >
> > And I have a rather general issue with this: Is it required to issue an
> > explicit registration directly after sending the 'connect' command?
> > IE do we need to have it as part of the 'connect' routine?
> >
> > Thing is, the linux 'connect' routine is doing several things in one go
> > (like establishing a transport association for the admin queue, send the
> > 'connect' command for the admin queue, create transport associations for
> > I/O queues and send 'connect' commands on all I/O queues, _and_
> possibly
> > do authentication, too).
> > If we now start overloading it with yet more functionality it becomes
> > extremely hard to cleanup after an error, and that doesn't even mention
> > the non-existing error reporting to userspace.
> >
> > Wouldn't it make more sense to delegate explicit registration to
> > userspace (ie libnvme/nvme-cli), and leave the kernel out of it?
> 
> It would. There is no reason what-so-ever to place all this register
> stuff needs to live in the kernel. We have a way to passthru commands so
> it can and should move to userspace.

I wish it could be delegated to a user-space app, and in fact that was my 
original design. 

Unfortunately, while testing I realized that the kernel can autonomously 
reconnect and user-space apps are completely unaware of it.

For example, let's say the network goes down momentarily. The kernel then 
tries to reconnect. Once it successfully reconnects it doesn't tell anyone 
about it. 

But let's say the kernel does send a signal to user-space on a reconnect. 
What if there is no user-space app to receive this signal? I'm thinking of 
the case where one uses nvme-cli to set up persistent connections to 
discovery controllers.  In that case there is no app to send the explicit 
registration on a re-connect.

Internal Use - Confidential

Re: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Hannes Reinecke]

On 1/27/22 14:30, Belanger, Martin wrote:
>>>> TP8010 introduces Central Discovery Controllers (CDC) and the
>>>> Discovery Information Management (DIM) PDU, which is used to perform
>>>> explicit registration with Discovery Controllers.
>>>>
>>>> This patch defines the DIM PDU and adds logic to perform explicit
>>>> registration with DCs when registration has been requested by the
>>>> user.
>>>>
[ .. ]
>>> I'd rather like to have the TLS bits in a separate patch; they are not
>>> strictly related to this issue.
>>>
>>> And I have a rather general issue with this: Is it required to issue an
>>> explicit registration directly after sending the 'connect' command?
>>> IE do we need to have it as part of the 'connect' routine?
>>>
>>> Thing is, the linux 'connect' routine is doing several things in one go
>>> (like establishing a transport association for the admin queue, send the
>>> 'connect' command for the admin queue, create transport associations for
>>> I/O queues and send 'connect' commands on all I/O queues, _and_
>> possibly
>>> do authentication, too).
>>> If we now start overloading it with yet more functionality it becomes
>>> extremely hard to cleanup after an error, and that doesn't even mention
>>> the non-existing error reporting to userspace.
>>>
>>> Wouldn't it make more sense to delegate explicit registration to
>>> userspace (ie libnvme/nvme-cli), and leave the kernel out of it?
>>
>> It would. There is no reason what-so-ever to place all this register
>> stuff needs to live in the kernel. We have a way to passthru commands so
>> it can and should move to userspace.
> 
> I wish it could be delegated to a user-space app, and in fact that was my
> original design.
> 
> Unfortunately, while testing I realized that the kernel can autonomously
> reconnect and user-space apps are completely unaware of it.
> 
> For example, let's say the network goes down momentarily. The kernel then
> tries to reconnect. Once it successfully reconnects it doesn't tell anyone
> about it.
> 
> But let's say the kernel does send a signal to user-space on a reconnect.
> What if there is no user-space app to receive this signal? I'm thinking of
> the case where one uses nvme-cli to set up persistent connections to
> discovery controllers.  In that case there is no app to send the explicit
> registration on a re-connect.
> 
Hmm. There must be something I'm not getting.

Either we have to re-register every time if a connection drops, but then 
the patch to unregister once a connection drops is pointless.
Or we need to explicitly de-register when disconnecting, but then we 
don't need to re-register for connection failures.

Which one is it?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Sagi Grimberg]

>>> Wouldn't it make more sense to delegate explicit registration to
>>> userspace (ie libnvme/nvme-cli), and leave the kernel out of it?
>>
>> It would. There is no reason what-so-ever to place all this register
>> stuff needs to live in the kernel. We have a way to passthru commands so
>> it can and should move to userspace.
> 
> I wish it could be delegated to a user-space app, and in fact that was my
> original design.
> 
> Unfortunately, while testing I realized that the kernel can autonomously
> reconnect and user-space apps are completely unaware of it.
> 
> For example, let's say the network goes down momentarily. The kernel then
> tries to reconnect. Once it successfully reconnects it doesn't tell anyone
> about it.

Then add a uevent on the controller device-node. From there you should
trap it and do what you need (exactly like how discovery log change
events are handled).

BTW, I also don't understand why the host needs this reregistration on
reconnect, but that is besides the point.

> But let's say the kernel does send a signal to user-space on a reconnect.
> What if there is no user-space app to receive this signal? I'm thinking of
> the case where one uses nvme-cli to set up persistent connections to
> discovery controllers.  In that case there is no app to send the explicit
> registration on a re-connect.

This argument does not justify adding functionality in the kernel that
doesn't belong there. If we were to follow this argument we would be
placing everything in the kernel. If someone wants this functionality,
he/she needs to use the tools required for it to work.

RE: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Belanger, Martin]

> >>> Wouldn't it make more sense to delegate explicit registration to
> >>> userspace (ie libnvme/nvme-cli), and leave the kernel out of it?
> >>
> >> It would. There is no reason what-so-ever to place all this register
> >> stuff needs to live in the kernel. We have a way to passthru commands
> >> so it can and should move to userspace.
> >
> > I wish it could be delegated to a user-space app, and in fact that was
> > my original design.
> >
> > Unfortunately, while testing I realized that the kernel can
> > autonomously reconnect and user-space apps are completely unaware of it.
> >
> > For example, let's say the network goes down momentarily. The kernel
> > then tries to reconnect. Once it successfully reconnects it doesn't
> > tell anyone about it.
> 
> Then add a uevent on the controller device-node. From there you should
> trap it and do what you need (exactly like how discovery log change events
> are handled).
> 
> BTW, I also don't understand why the host needs this reregistration on
> reconnect, but that is besides the point.

The fact is that when connectivity is lost and restored, we don’t 
know whether changes have occurred on the host (e.g. maybe the 
symbolic name was changed while connectivity was lost, etc.)
and thus registration must be reapplied every time the host reconnects
to make sure there is no stale information at the discovery controller.

> 
> > But let's say the kernel does send a signal to user-space on a reconnect.
> > What if there is no user-space app to receive this signal? I'm
> > thinking of the case where one uses nvme-cli to set up persistent
> > connections to discovery controllers.  In that case there is no app to
> > send the explicit registration on a re-connect.
> 
> This argument does not justify adding functionality in the kernel that doesn't
> belong there. If we were to follow this argument we would be placing
> everything in the kernel. If someone wants this functionality, he/she needs
> to use the tools required for it to work.

Ok, Hannes, Sagi, Christoph, et al. I got the message loud and clear. 
Explicit Registration does not belong in the kernel. And, as you suggested,
the kernel needs to send a uevent when it loses connectivity and 
another uevent when connectivity is restored. This will allow userspace 
applications know when to reapply registration (if needed).

I'm not a uevent expert (at least how to send uevents from the kernel). 
From what I read, there's only a limited set of uevents defined (i.e. add, 
remove, move, online, offline). The nvme driver already uses the  
uevents "add"  and "remove" when nvme devices are "created" and 
"deleted" respectively . We also have  the "change" event that is used 
when there's a change in the Log Pages. May I suggest that we use the 
"offline" and "online" events when connectivity is "lost" and "restored" 
respectively? Please let me know.

Thanks,
Martin

Internal Use - Confidential

Re: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Hannes Reinecke]

On 1/28/22 18:55, Belanger, Martin wrote:
>>>>> Wouldn't it make more sense to delegate explicit registration to
>>>>> userspace (ie libnvme/nvme-cli), and leave the kernel out of it?
>>>>
>>>> It would. There is no reason what-so-ever to place all this register
>>>> stuff needs to live in the kernel. We have a way to passthru commands
>>>> so it can and should move to userspace.
>>>
>>> I wish it could be delegated to a user-space app, and in fact that was
>>> my original design.
>>>
>>> Unfortunately, while testing I realized that the kernel can
>>> autonomously reconnect and user-space apps are completely unaware of it.
>>>
>>> For example, let's say the network goes down momentarily. The kernel
>>> then tries to reconnect. Once it successfully reconnects it doesn't
>>> tell anyone about it.
>>
>> Then add a uevent on the controller device-node. From there you should
>> trap it and do what you need (exactly like how discovery log change events
>> are handled).
>>
>> BTW, I also don't understand why the host needs this reregistration on
>> reconnect, but that is besides the point.
> 
> The fact is that when connectivity is lost and restored, we don’t
> know whether changes have occurred on the host (e.g. maybe the
> symbolic name was changed while connectivity was lost, etc.)
> and thus registration must be reapplied every time the host reconnects
> to make sure there is no stale information at the discovery controller.
> 
>>
>>> But let's say the kernel does send a signal to user-space on a reconnect.
>>> What if there is no user-space app to receive this signal? I'm
>>> thinking of the case where one uses nvme-cli to set up persistent
>>> connections to discovery controllers.  In that case there is no app to
>>> send the explicit registration on a re-connect.
>>
>> This argument does not justify adding functionality in the kernel that doesn't
>> belong there. If we were to follow this argument we would be placing
>> everything in the kernel. If someone wants this functionality, he/she needs
>> to use the tools required for it to work.
> 
> Ok, Hannes, Sagi, Christoph, et al. I got the message loud and clear.
> Explicit Registration does not belong in the kernel. And, as you suggested,
> the kernel needs to send a uevent when it loses connectivity and
> another uevent when connectivity is restored. This will allow userspace
> applications know when to reapply registration (if needed).
> 
> I'm not a uevent expert (at least how to send uevents from the kernel).
>  From what I read, there's only a limited set of uevents defined (i.e. add,
> remove, move, online, offline). The nvme driver already uses the
> uevents "add"  and "remove" when nvme devices are "created" and
> "deleted" respectively . We also have  the "change" event that is used
> when there's a change in the Log Pages. May I suggest that we use the
> "offline" and "online" events when connectivity is "lost" and "restored"
> respectively? Please let me know.
> 

I guess we already have that; cf commit f6f09c15a767 ("nvme: generate 
uevent once a multipath namespace is operational again").

When multipath is activated (and I guess that will be for nvme-tcp) you 
will receive a 'change' uevent whenever a path gets reinstated.

So that will be the time when an explicit registration will need to be 
reapplied.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

RE: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Belanger, Martin]

> On 1/28/22 18:55, Belanger, Martin wrote:
> >>>>> Wouldn't it make more sense to delegate explicit registration to
> >>>>> userspace (ie libnvme/nvme-cli), and leave the kernel out of it?
> >>>>
> >>>> It would. There is no reason what-so-ever to place all this
> >>>> register stuff needs to live in the kernel. We have a way to
> >>>> passthru commands so it can and should move to userspace.
> >>>
> >>> I wish it could be delegated to a user-space app, and in fact that
> >>> was my original design.
> >>>
> >>> Unfortunately, while testing I realized that the kernel can
> >>> autonomously reconnect and user-space apps are completely unaware
> of it.
> >>>
> >>> For example, let's say the network goes down momentarily. The kernel
> >>> then tries to reconnect. Once it successfully reconnects it doesn't
> >>> tell anyone about it.
> >>
> >> Then add a uevent on the controller device-node. From there you
> >> should trap it and do what you need (exactly like how discovery log
> >> change events are handled).
> >>
> >> BTW, I also don't understand why the host needs this reregistration
> >> on reconnect, but that is besides the point.
> >
> > The fact is that when connectivity is lost and restored, we don’t know
> > whether changes have occurred on the host (e.g. maybe the symbolic
> > name was changed while connectivity was lost, etc.) and thus
> > registration must be reapplied every time the host reconnects to make
> > sure there is no stale information at the discovery controller.
> >
> >>
> >>> But let's say the kernel does send a signal to user-space on a reconnect.
> >>> What if there is no user-space app to receive this signal? I'm
> >>> thinking of the case where one uses nvme-cli to set up persistent
> >>> connections to discovery controllers.  In that case there is no app
> >>> to send the explicit registration on a re-connect.
> >>
> >> This argument does not justify adding functionality in the kernel
> >> that doesn't belong there. If we were to follow this argument we
> >> would be placing everything in the kernel. If someone wants this
> >> functionality, he/she needs to use the tools required for it to work.
> >
> > Ok, Hannes, Sagi, Christoph, et al. I got the message loud and clear.
> > Explicit Registration does not belong in the kernel. And, as you
> > suggested, the kernel needs to send a uevent when it loses
> > connectivity and another uevent when connectivity is restored. This
> > will allow userspace applications know when to reapply registration (if
> needed).
> >
> > I'm not a uevent expert (at least how to send uevents from the kernel).
> >  From what I read, there's only a limited set of uevents defined (i.e.
> > add, remove, move, online, offline). The nvme driver already uses the
> > uevents "add"  and "remove" when nvme devices are "created" and
> > "deleted" respectively . We also have  the "change" event that is used
> > when there's a change in the Log Pages. May I suggest that we use the
> > "offline" and "online" events when connectivity is "lost" and "restored"
> > respectively? Please let me know.
> >
> 
> I guess we already have that; cf commit f6f09c15a767 ("nvme: generate
> uevent once a multipath namespace is operational again").
> 
> When multipath is activated (and I guess that will be for nvme-tcp) you will
> receive a 'change' uevent whenever a path gets reinstated.
> 
> So that will be the time when an explicit registration will need to be reapplied.
> 

Just want to make sure I understand. When the connection is lost the kernel 
sends nothing. When the connection is restored the kernel sends a "change" 
uevent. And when the Log Pages have changed the kernel also sends a "change" 
uevent.

It would be nice to have different events because if we use the "change"
uevent for both "log pages have changed" and "connection has been 
restored", then the user-space app will have to send both an "explicit 
registration" and a "get log page" whenever it receives a "change" uevent.
It won’t be able to tell what that "change" uevent is for.

Is that what you want me to do?

Regards,
Martin

> Cheers,
> 
> Hannes
> --
> Dr. Hannes Reinecke                Kernel Storage Architect
> hare@suse.de                              +49 911 74053 688
> SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809
> (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Internal Use - Confidential

Re: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Hannes Reinecke]

On 1/29/22 00:02, Belanger, Martin wrote:
> 
>> On 1/28/22 18:55, Belanger, Martin wrote:
>>>>>>> Wouldn't it make more sense to delegate explicit registration to
>>>>>>> userspace (ie libnvme/nvme-cli), and leave the kernel out of it?
>>>>>>
>>>>>> It would. There is no reason what-so-ever to place all this
>>>>>> register stuff needs to live in the kernel. We have a way to
>>>>>> passthru commands so it can and should move to userspace.
>>>>>
>>>>> I wish it could be delegated to a user-space app, and in fact that
>>>>> was my original design.
>>>>>
>>>>> Unfortunately, while testing I realized that the kernel can
>>>>> autonomously reconnect and user-space apps are completely unaware
>> of it.
>>>>>
>>>>> For example, let's say the network goes down momentarily. The kernel
>>>>> then tries to reconnect. Once it successfully reconnects it doesn't
>>>>> tell anyone about it.
>>>>
>>>> Then add a uevent on the controller device-node. From there you
>>>> should trap it and do what you need (exactly like how discovery log
>>>> change events are handled).
>>>>
>>>> BTW, I also don't understand why the host needs this reregistration
>>>> on reconnect, but that is besides the point.
>>>
>>> The fact is that when connectivity is lost and restored, we don’t know
>>> whether changes have occurred on the host (e.g. maybe the symbolic
>>> name was changed while connectivity was lost, etc.) and thus
>>> registration must be reapplied every time the host reconnects to make
>>> sure there is no stale information at the discovery controller.
>>>
>>>>
>>>>> But let's say the kernel does send a signal to user-space on a reconnect.
>>>>> What if there is no user-space app to receive this signal? I'm
>>>>> thinking of the case where one uses nvme-cli to set up persistent
>>>>> connections to discovery controllers.  In that case there is no app
>>>>> to send the explicit registration on a re-connect.
>>>>
>>>> This argument does not justify adding functionality in the kernel
>>>> that doesn't belong there. If we were to follow this argument we
>>>> would be placing everything in the kernel. If someone wants this
>>>> functionality, he/she needs to use the tools required for it to work.
>>>
>>> Ok, Hannes, Sagi, Christoph, et al. I got the message loud and clear.
>>> Explicit Registration does not belong in the kernel. And, as you
>>> suggested, the kernel needs to send a uevent when it loses
>>> connectivity and another uevent when connectivity is restored. This
>>> will allow userspace applications know when to reapply registration (if
>> needed).
>>>
>>> I'm not a uevent expert (at least how to send uevents from the kernel).
>>>   From what I read, there's only a limited set of uevents defined (i.e.
>>> add, remove, move, online, offline). The nvme driver already uses the
>>> uevents "add"  and "remove" when nvme devices are "created" and
>>> "deleted" respectively . We also have  the "change" event that is used
>>> when there's a change in the Log Pages. May I suggest that we use the
>>> "offline" and "online" events when connectivity is "lost" and "restored"
>>> respectively? Please let me know.
>>>
>>
>> I guess we already have that; cf commit f6f09c15a767 ("nvme: generate
>> uevent once a multipath namespace is operational again").
>>
>> When multipath is activated (and I guess that will be for nvme-tcp) you will
>> receive a 'change' uevent whenever a path gets reinstated.
>>
>> So that will be the time when an explicit registration will need to be reapplied.
>>
> 
> Just want to make sure I understand. When the connection is lost the kernel
> sends nothing. When the connection is restored the kernel sends a "change"
> uevent. And when the Log Pages have changed the kernel also sends a "change"
> uevent.
> 

Yes.

> It would be nice to have different events because if we use the "change"
> uevent for both "log pages have changed" and "connection has been
> restored", then the user-space app will have to send both an "explicit
> registration" and a "get log page" whenever it receives a "change" uevent.
> It won’t be able to tell what that "change" uevent is for.
> 

Au contraire.
Change events come with a full payload detailing out what the event is 
about. In the case of a 'log page changed' uevent the event points to 
the controller device, and payload is a string
'NVME_AEN=<aen number>'.
In the case of the 'path reinstated' uevent the event points to the 
block device.

So it's easy to differentiate between those two.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

RE: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Belanger, Martin]

> 
> On 1/29/22 00:02, Belanger, Martin wrote:
> >
> >> On 1/28/22 18:55, Belanger, Martin wrote:
> >>>>>>> Wouldn't it make more sense to delegate explicit registration to
> >>>>>>> userspace (ie libnvme/nvme-cli), and leave the kernel out of it?
> >>>>>>
> >>>>>> It would. There is no reason what-so-ever to place all this
> >>>>>> register stuff needs to live in the kernel. We have a way to
> >>>>>> passthru commands so it can and should move to userspace.
> >>>>>
> >>>>> I wish it could be delegated to a user-space app, and in fact that
> >>>>> was my original design.
> >>>>>
> >>>>> Unfortunately, while testing I realized that the kernel can
> >>>>> autonomously reconnect and user-space apps are completely
> unaware
> >> of it.
> >>>>>
> >>>>> For example, let's say the network goes down momentarily. The
> >>>>> kernel then tries to reconnect. Once it successfully reconnects it
> >>>>> doesn't tell anyone about it.
> >>>>
> >>>> Then add a uevent on the controller device-node. From there you
> >>>> should trap it and do what you need (exactly like how discovery log
> >>>> change events are handled).
> >>>>
> >>>> BTW, I also don't understand why the host needs this reregistration
> >>>> on reconnect, but that is besides the point.
> >>>
> >>> The fact is that when connectivity is lost and restored, we don’t
> >>> know whether changes have occurred on the host (e.g. maybe the
> >>> symbolic name was changed while connectivity was lost, etc.) and
> >>> thus registration must be reapplied every time the host reconnects
> >>> to make sure there is no stale information at the discovery controller.
> >>>
> >>>>
> >>>>> But let's say the kernel does send a signal to user-space on a
> reconnect.
> >>>>> What if there is no user-space app to receive this signal? I'm
> >>>>> thinking of the case where one uses nvme-cli to set up persistent
> >>>>> connections to discovery controllers.  In that case there is no
> >>>>> app to send the explicit registration on a re-connect.
> >>>>
> >>>> This argument does not justify adding functionality in the kernel
> >>>> that doesn't belong there. If we were to follow this argument we
> >>>> would be placing everything in the kernel. If someone wants this
> >>>> functionality, he/she needs to use the tools required for it to work.
> >>>
> >>> Ok, Hannes, Sagi, Christoph, et al. I got the message loud and clear.
> >>> Explicit Registration does not belong in the kernel. And, as you
> >>> suggested, the kernel needs to send a uevent when it loses
> >>> connectivity and another uevent when connectivity is restored. This
> >>> will allow userspace applications know when to reapply registration
> >>> (if
> >> needed).
> >>>
> >>> I'm not a uevent expert (at least how to send uevents from the kernel).
> >>>   From what I read, there's only a limited set of uevents defined (i.e.
> >>> add, remove, move, online, offline). The nvme driver already uses
> >>> the uevents "add"  and "remove" when nvme devices are "created" and
> >>> "deleted" respectively . We also have  the "change" event that is
> >>> used when there's a change in the Log Pages. May I suggest that we
> >>> use the "offline" and "online" events when connectivity is "lost" and
> "restored"
> >>> respectively? Please let me know.
> >>>
> >>
> >> I guess we already have that; cf commit f6f09c15a767 ("nvme: generate
> >> uevent once a multipath namespace is operational again").
> >>
> >> When multipath is activated (and I guess that will be for nvme-tcp)
> >> you will receive a 'change' uevent whenever a path gets reinstated.
> >>
> >> So that will be the time when an explicit registration will need to be
> reapplied.
> >>
> >
> > Just want to make sure I understand. When the connection is lost the
> > kernel sends nothing. When the connection is restored the kernel sends a
> "change"
> > uevent. And when the Log Pages have changed the kernel also sends a
> "change"
> > uevent.
> >
> 
> Yes.
> 
> > It would be nice to have different events because if we use the "change"
> > uevent for both "log pages have changed" and "connection has been
> > restored", then the user-space app will have to send both an "explicit
> > registration" and a "get log page" whenever it receives a "change" uevent.
> > It won’t be able to tell what that "change" uevent is for.
> >
> 
> Au contraire.
> Change events come with a full payload detailing out what the event is about.
> In the case of a 'log page changed' uevent the event points to the controller
> device, and payload is a string 'NVME_AEN=<aen number>'.
> In the case of the 'path reinstated' uevent the event points to the block
> device.

There is no *block device*. It's a connection to a Discover Controller. 

> 
> So it's easy to differentiate between those two.
> 
> Cheers,
> 
> Hannes
> --
> Dr. Hannes Reinecke                Kernel Storage Architect
> hare@suse.de                              +49 911 74053 688
> SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809
> (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Internal Use - Confidential

Re: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Hannes Reinecke]

On 1/29/22 13:23, Belanger, Martin wrote:
>>
>> On 1/29/22 00:02, Belanger, Martin wrote:
>>>
[ .. ]
>>> It would be nice to have different events because if we use the "change"
>>> uevent for both "log pages have changed" and "connection has been
>>> restored", then the user-space app will have to send both an "explicit
>>> registration" and a "get log page" whenever it receives a "change" uevent.
>>> It won’t be able to tell what that "change" uevent is for.
>>>
>>
>> Au contraire.
>> Change events come with a full payload detailing out what the event is about.
>> In the case of a 'log page changed' uevent the event points to the controller
>> device, and payload is a string 'NVME_AEN=<aen number>'.
>> In the case of the 'path reinstated' uevent the event points to the block
>> device.
> 
> There is no *block device*. It's a connection to a Discover Controller.
> 
Ah. Correct. Guess we'll have to add an uevent here.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Sagi Grimberg]

> [ .. ]
>>>> It would be nice to have different events because if we use the 
>>>> "change"
>>>> uevent for both "log pages have changed" and "connection has been
>>>> restored", then the user-space app will have to send both an "explicit
>>>> registration" and a "get log page" whenever it receives a "change" 
>>>> uevent.
>>>> It won’t be able to tell what that "change" uevent is for.
>>>>
>>>
>>> Au contraire.
>>> Change events come with a full payload detailing out what the event 
>>> is about.
>>> In the case of a 'log page changed' uevent the event points to the 
>>> controller
>>> device, and payload is a string 'NVME_AEN=<aen number>'.
>>> In the case of the 'path reinstated' uevent the event points to the 
>>> block
>>> device.
>>
>> There is no *block device*. It's a connection to a Discover Controller.
>>
> Ah. Correct. Guess we'll have to add an uevent here.

Yes, thgere is no bdev for discovery controllers. We may add a uevent
for userspace to consume as soon as the transport reconnects or when
a controller (re)starts.

Don't see any point for the disconnect part as this is async and will
not have any ordering with the actual disconnect. userspace will need
to do it before it disconnects the discovery controller.

Re: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Sagi Grimberg]

> Yes, thgere is no bdev for discovery controllers. We may add a uevent
> for userspace to consume as soon as the transport reconnects or when
> a controller (re)starts.

Something like this should be sufficient:
--
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index dd18861f77c0..55e11ee19460 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4396,6 +4396,7 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
                 nvme_queue_scan(ctrl);
                 nvme_start_queues(ctrl);
         }
+       kobject_uevent(&ctrl->device->kobj, KOBJ_ONLINE);
  }
  EXPORT_SYMBOL_GPL(nvme_start_ctrl);
--

RE: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Belanger, Martin]

> > [ .. ]
> >>>> It would be nice to have different events because if we use the
> >>>> "change"
> >>>> uevent for both "log pages have changed" and "connection has been
> >>>> restored", then the user-space app will have to send both an
> >>>> "explicit registration" and a "get log page" whenever it receives a
> "change"
> >>>> uevent.
> >>>> It won’t be able to tell what that "change" uevent is for.
> >>>>
> >>>
> >>> Au contraire.
> >>> Change events come with a full payload detailing out what the event
> >>> is about.
> >>> In the case of a 'log page changed' uevent the event points to the
> >>> controller device, and payload is a string 'NVME_AEN=<aen number>'.
> >>> In the case of the 'path reinstated' uevent the event points to the
> >>> block device.
> >>
> >> There is no *block device*. It's a connection to a Discover Controller.
> >>
> > Ah. Correct. Guess we'll have to add an uevent here.
> 
> Yes, thgere is no bdev for discovery controllers. We may add a uevent for
> userspace to consume as soon as the transport reconnects or when a
> controller (re)starts.
> 
> Don't see any point for the disconnect part as this is async and will not have
> any ordering with the actual disconnect. userspace will need to do it before it
> disconnects the discovery controller.

Hi Sagi. Thanks for the code snippet. I just had a question about your comment.

You say that you don’t see any point to send an event on the disconnect 
because it is async.  I don’t quite understand that part. For example, I've
observed that when connectivity is lost the kernel will try to connect every
10 sec. And it will keep trying to connect 60 times (i.e. 10 min) before 
giving up. Wouldn't sending an "offline" event when connectivity is lost 
give user space apps a heads up that the nvme device may not respond.

Thanks,
Martin

Internal Use - Confidential

Re: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Sagi Grimberg]

> Hi Sagi. Thanks for the code snippet. I just had a question about your comment.
> 
> You say that you don’t see any point to send an event on the disconnect
> because it is async.  I don’t quite understand that part. For example, I've
> observed that when connectivity is lost the kernel will try to connect every
> 10 sec. And it will keep trying to connect 60 times (i.e. 10 min) before
> giving up. Wouldn't sending an "offline" event when connectivity is lost
> give user space apps a heads up that the nvme device may not respond.

Why does userspace need it?

When the host will disconnect it will send event asynchronously, so
userspace cannot reliably do anything before the host disconnects.
Other than that I don't understand what would userspace do with
"offline - I/O is going to fail"...

RE: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Belanger, Martin]

> > Hi Sagi. Thanks for the code snippet. I just had a question about your
> comment.
> >
> > You say that you don’t see any point to send an event on the
> > disconnect because it is async.  I don’t quite understand that part.
> > For example, I've observed that when connectivity is lost the kernel
> > will try to connect every
> > 10 sec. And it will keep trying to connect 60 times (i.e. 10 min)
> > before giving up. Wouldn't sending an "offline" event when
> > connectivity is lost give user space apps a heads up that the nvme device
> may not respond.
> 
> Why does userspace need it?
> 
> When the host will disconnect it will send event asynchronously, so
> userspace cannot reliably do anything before the host disconnects.
> Other than that I don't understand what would userspace do with "offline -
> I/O is going to fail"...

I was just asking for completeness. Some apps may use it.

Internal Use - Confidential

Re: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Sagi Grimberg]

>>> You say that you don’t see any point to send an event on the
>>> disconnect because it is async.  I don’t quite understand that part.
>>> For example, I've observed that when connectivity is lost the kernel
>>> will try to connect every
>>> 10 sec. And it will keep trying to connect 60 times (i.e. 10 min)
>>> before giving up. Wouldn't sending an "offline" event when
>>> connectivity is lost give user space apps a heads up that the nvme device
>> may not respond.
>>
>> Why does userspace need it?
>>
>> When the host will disconnect it will send event asynchronously, so
>> userspace cannot reliably do anything before the host disconnects.
>> Other than that I don't understand what would userspace do with "offline -
>> I/O is going to fail"...
> 
> I was just asking for completeness. Some apps may use it.

I think we'd want a real use-case before decide we expose it.

Re: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Hannes Reinecke]

On 1/31/22 17:16, Belanger, Martin wrote:
> 
>>> Hi Sagi. Thanks for the code snippet. I just had a question about your
>> comment.
>>>
>>> You say that you don’t see any point to send an event on the
>>> disconnect because it is async.  I don’t quite understand that part.
>>> For example, I've observed that when connectivity is lost the kernel
>>> will try to connect every
>>> 10 sec. And it will keep trying to connect 60 times (i.e. 10 min)
>>> before giving up. Wouldn't sending an "offline" event when
>>> connectivity is lost give user space apps a heads up that the nvme device
>> may not respond.
>>
>> Why does userspace need it?
>>
>> When the host will disconnect it will send event asynchronously, so
>> userspace cannot reliably do anything before the host disconnects.
>> Other than that I don't understand what would userspace do with "offline -
>> I/O is going to fail"...
> 
> I was just asking for completeness. Some apps may use it.
> 
We had a similar discussion during udev development; originally we had 
KOBJ_ONLINE and KOBJ_OFFLINE uevents
Turned out that the KOBJ_OFFLINE events were quite pointless (as Sagi 
indicated), and hence we settled on a single KOBJ_CHANGE event which 
typically is sent when a device/system gets back online.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 3/4] nvme-fabrics: add tp8010 support

[John Meneghini]

Hi Martin.

Properly speaking, registrations are sent to the CDC, not the DC.

Correct?

Can we please change this, everywhere, to be clear about when we are "talking" to the CDC (Centralized Discovery Controller) and 
when we are talking to the DC (Discovery Controller).

/John

On 1/25/22 09:59, Martin Belanger wrote:
> +/**
> + * nvme_get_adrfam() - Get address family for the address we're registering
> + * with the DC. We retrieve this info from the socket itself. If we can't
> + * get the source address from the socket, then we'll infer the address
> + * family from the address of the DC since the DC address has the same
> + * address family.
> + *
> + * @ctrl: Host NVMe controller instance maintaining the admin queue used to
> + *      submit the DIM command to the DC.
> + *
> + * Return: The address family of the source address associated with the
> + * socket connected to the DC.

RE: [PATCH 3/4] nvme-fabrics: add tp8010 support

[Belanger, Martin]

> Hi Martin.
> 
> Properly speaking, registrations are sent to the CDC, not the DC.
> 
> Correct?
> 
> Can we please change this, everywhere, to be clear about when we are
> "talking" to the CDC (Centralized Discovery Controller) and when we are
> talking to the DC (Discovery Controller).
> 
> /John

Hi John. 

TP8010 originally intended for registration (DIM PDU) to be sent 
to CDCs only. However, this was changed to include Direct Discovery 
Controllers (DDC)  as well. That's why I use DC and not CDC. 

Also note that TP8010 introduces a new DCTYPE field in the response to 
the Identify command, which is used to differentiate between a CDC and 
a DDC. Legacy DCs that do not comply to TP8010 will have DCTYPE=0 
(i.e. Discovery controller type is not reported). So, registration will not 
be attempted  for DCTYPE other than 1 (DDC) and 2 (CDC).

Here you will find the ratified version of TP8010: 
https://nvmexpress.org/wp-content/uploads/NVM-Express-2.0-Ratified-TPs-01242022.zip

Martin

> 
> On 1/25/22 09:59, Martin Belanger wrote:
> > +/**
> > + * nvme_get_adrfam() - Get address family for the address we're
> > +registering
> > + * with the DC. We retrieve this info from the socket itself. If we
> > +can't
> > + * get the source address from the socket, then we'll infer the
> > +address
> > + * family from the address of the DC since the DC address has the
> > +same
> > + * address family.
> > + *
> > + * @ctrl: Host NVMe controller instance maintaining the admin queue
> used to
> > + *      submit the DIM command to the DC.
> > + *
> > + * Return: The address family of the source address associated with
> > +the
> > + * socket connected to the DC.


Internal Use - Confidential

Re: [PATCH 3/4] nvme-fabrics: add tp8010 support

[John Meneghini]

OK. Thanks Martin.  That explains it.

On 1/31/22 12:17, Belanger, Martin wrote:
>> Hi Martin.
>>
>> Properly speaking, registrations are sent to the CDC, not the DC.
>>
>> Correct?
>>
>> Can we please change this, everywhere, to be clear about when we are
>> "talking" to the CDC (Centralized Discovery Controller) and when we are
>> talking to the DC (Discovery Controller).
>>
>> /John
> 
> Hi John.
> 
> TP8010 originally intended for registration (DIM PDU) to be sent
> to CDCs only. However, this was changed to include Direct Discovery
> Controllers (DDC)  as well. That's why I use DC and not CDC.
> 
> Also note that TP8010 introduces a new DCTYPE field in the response to
> the Identify command, which is used to differentiate between a CDC and
> a DDC. Legacy DCs that do not comply to TP8010 will have DCTYPE=0
> (i.e. Discovery controller type is not reported). So, registration will not
> be attempted  for DCTYPE other than 1 (DDC) and 2 (CDC).
> 
> Here you will find the ratified version of TP8010:
> https://nvmexpress.org/wp-content/uploads/NVM-Express-2.0-Ratified-TPs-01242022.zip
> 
> Martin
> 
>>
>> On 1/25/22 09:59, Martin Belanger wrote:
>>> +/**
>>> + * nvme_get_adrfam() - Get address family for the address we're
>>> +registering
>>> + * with the DC. We retrieve this info from the socket itself. If we
>>> +can't
>>> + * get the source address from the socket, then we'll infer the
>>> +address
>>> + * family from the address of the DC since the DC address has the
>>> +same
>>> + * address family.
>>> + *
>>> + * @ctrl: Host NVMe controller instance maintaining the admin queue
>> used to
>>> + *      submit the DIM command to the DC.
>>> + *
>>> + * Return: The address family of the source address associated with
>>> +the
>>> + * socket connected to the DC.
> 
> 
> Internal Use - Confidential

[PATCH 4/4] nvme-fabrics: add explicit deregistration on disconnect.

[Martin Belanger]

From: Martin Belanger <martin.belanger@dell.com>

Per TP8010, a host explicitly registered with a Discovery
Controller (DC) must deregister on disconnect. This patch
sends a DIM PDU to DCs to deregister the host on disconnect.

Signed-off-by: Martin Belanger <martin.belanger@dell.com>
---
 drivers/nvme/host/tcp.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 970b7df574a4..804d927b4b81 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2176,6 +2176,16 @@ static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
 
 static void nvme_tcp_delete_ctrl(struct nvme_ctrl *ctrl)
 {
+	if (ctrl->opts->explicit_registration &&
++	    ((ctrl->dctype == NVME_DCTYPE_DDC) ||
+	     (ctrl->dctype == NVME_DCTYPE_CDC))) {
+		/* Deregister from discovery controller */
+		union nvmf_tsas tsas = {.tcp.sectype = NVMF_TCP_SECTYPE_NONE};
+
+		nvmf_disc_info_mgmt(ctrl, NVME_TAS_DEREGISTER, NVMF_TRTYPE_TCP,
+				    nvme_get_adrfam(ctrl), "", &tsas);
+	}
+
 	nvme_tcp_teardown_ctrl(ctrl, true);
 }
 
-- 
2.34.1

Re: [PATCH 4/4] nvme-fabrics: add explicit deregistration on disconnect.

[Hannes Reinecke]

On 1/25/22 15:59, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> Per TP8010, a host explicitly registered with a Discovery
> Controller (DC) must deregister on disconnect. This patch
> sends a DIM PDU to DCs to deregister the host on disconnect.
> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---
>   drivers/nvme/host/tcp.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 970b7df574a4..804d927b4b81 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2176,6 +2176,16 @@ static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
>   
>   static void nvme_tcp_delete_ctrl(struct nvme_ctrl *ctrl)
>   {
> +	if (ctrl->opts->explicit_registration &&
> ++	    ((ctrl->dctype == NVME_DCTYPE_DDC) ||
> +	     (ctrl->dctype == NVME_DCTYPE_CDC))) {
> +		/* Deregister from discovery controller */
> +		union nvmf_tsas tsas = {.tcp.sectype = NVMF_TCP_SECTYPE_NONE};
> +
> +		nvmf_disc_info_mgmt(ctrl, NVME_TAS_DEREGISTER, NVMF_TRTYPE_TCP,
> +				    nvme_get_adrfam(ctrl), "", &tsas);
> +	}
> +
>   	nvme_tcp_teardown_ctrl(ctrl, true);
>   }
>   
Similar argument to the previous patch: Does this need to be in the 
kernel? Can't we delegate this to userspace?

And, more importantly: What happens if the host does _not_ deregister on 
disconnect?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH 4/4] nvme-fabrics: add explicit deregistration on disconnect.

[Sagi Grimberg]

>> Per TP8010, a host explicitly registered with a Discovery
>> Controller (DC) must deregister on disconnect. This patch
>> sends a DIM PDU to DCs to deregister the host on disconnect.
>>
>> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
>> ---
>>   drivers/nvme/host/tcp.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>> index 970b7df574a4..804d927b4b81 100644
>> --- a/drivers/nvme/host/tcp.c
>> +++ b/drivers/nvme/host/tcp.c
>> @@ -2176,6 +2176,16 @@ static void nvme_tcp_teardown_ctrl(struct 
>> nvme_ctrl *ctrl, bool shutdown)
>>   static void nvme_tcp_delete_ctrl(struct nvme_ctrl *ctrl)
>>   {
>> +    if (ctrl->opts->explicit_registration &&
>> ++        ((ctrl->dctype == NVME_DCTYPE_DDC) ||
>> +         (ctrl->dctype == NVME_DCTYPE_CDC))) {
>> +        /* Deregister from discovery controller */
>> +        union nvmf_tsas tsas = {.tcp.sectype = NVMF_TCP_SECTYPE_NONE};
>> +
>> +        nvmf_disc_info_mgmt(ctrl, NVME_TAS_DEREGISTER, NVMF_TRTYPE_TCP,
>> +                    nvme_get_adrfam(ctrl), "", &tsas);
>> +    }
>> +
>>       nvme_tcp_teardown_ctrl(ctrl, true);
>>   }
> Similar argument to the previous patch: Does this need to be in the 
> kernel? Can't we delegate this to userspace?

Same agreement here. we shouldn't add any of this to a transport driver.