[PATCH 0/7] Implement NVMe Namespace Descriptor Identification

[Johannes Thumshirn]

This patchset implemets NVMe Namespace Descriptor Identification as of
NVMe 1.3. The Namespace Descriptor Identification allows a NVMe host
to query several Namespace Identification mechanisms, such as EUI-64,
NGUID and UUID from the target. If more than one value is set by the
target, it can transmit all set values to the host.

The Namespace Identification Descriptor list is the only way a target
can identify itself via the newly introduced UUID to the host (instead
of the EUI-64 or NGUID).

Both the Host and Target side are implemented. In order to get the
Linux Host to send the Linux target implementation a Namespace
Descriptor Identification command, you have to change the target's
announced version code to at least 1.3.

Unfortunately the host side already did have a sysfs attribute called
'uuid' which represented the NGUID, so precautions have been taken to
not break any existing userspace.

While I was already touching the relevant code paths, I decided to
also include the EUI-64 in the 'Identify Namespace' command response.

The code is tested using the nvme-loop loopback target and cut against
the nvme tree's nvme-4.12 branch.

A patch for nvmetcli will follow shortly.

Johannes Thumshirn (7):
nvme: rename uuid to nguid in nvme_ns
nvmet: add uuid field to nvme_ns and populate via configfs
nvmet: add eui64 field to nvme_ns and populate via configfs
nvme: also report include the EUI-64 in identify NS report
nvmet: implement namespace identify descriptor list
nvme: get list of namespace descriptors
nvme: provide UUID value to userspace

drivers/nvme/host/core.c | 116 ++++++++++++++++++++++++++++++++++++++--
drivers/nvme/host/nvme.h | 1 +
drivers/nvme/target/admin-cmd.c | 76 ++++++++++++++++++++++++++
drivers/nvme/target/configfs.c | 96 +++++++++++++++++++++++++++++++++
drivers/nvme/target/nvmet.h | 2 +
include/linux/nvme.h | 14 +++++
6 files changed, 301 insertions(+), 4 deletions(-)

--
2.12.0

[Johannes Thumshirn]

Add the EUI-64 field from the NVMe Namespace Identification Descriptor
to the nvmet_ns structure and allow it's population via configfs.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---
drivers/nvme/target/configfs.c | 48 ++++++++++++++++++++++++++++++++++++++++++
drivers/nvme/target/nvmet.h | 1 +
2 files changed, 49 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 0529a36501f4..0c6351253c3f 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -305,6 +305,53 @@ static ssize_t nvmet_ns_device_path_store(struct config_item *item,

CONFIGFS_ATTR(nvmet_ns_, device_path);

+static ssize_t nvmet_ns_device_eui64_show(struct config_item *item, char *page)
+{
+ return sprintf(page, "%8phN\n", &to_nvmet_ns(item)->eui64);
+}
+
+static ssize_t nvmet_ns_device_eui64_store(struct config_item *item,
+ const char *page, size_t count)
+{
+ struct nvmet_ns *ns = to_nvmet_ns(item);
+ struct nvmet_subsys *subsys = ns->subsys;
+ u8 eui64[8];
+ const char *p = page;
+ int i;
+ int ret = 0;
+
+
+ mutex_lock(&subsys->lock);
+ if (ns->enabled) {
+ ret = -EBUSY;
+ goto out_unlock;
+ }
+
+ for (i = 0; i < 8; i++) {
+ if (p + 2 > page + count) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+ if (!isxdigit(p[0]) || !isxdigit(p[1])) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
+ eui64[i] = (hex_to_bin(p[0]) << 4) | hex_to_bin(p[1]);
+ p += 2;
+
+ if (*p == '-')
+ p++;
+ }
+
+ memcpy(&ns->eui64, eui64, sizeof(eui64));
+out_unlock:
+ mutex_unlock(&subsys->lock);
+ return ret ? ret : count;
+}
+
+CONFIGFS_ATTR(nvmet_ns_, device_eui64);
+
static ssize_t nvmet_ns_device_uuid_show(struct config_item *item, char *page)
{
return sprintf(page, "%pUb\n", &to_nvmet_ns(item)->uuid);
@@ -427,6 +474,7 @@ static struct configfs_attribute *nvmet_ns_attrs[] = {
&nvmet_ns_attr_device_path,
&nvmet_ns_attr_device_nguid,
&nvmet_ns_attr_device_uuid,
+ &nvmet_ns_attr_device_eui64,
&nvmet_ns_attr_enable,
NULL,
};
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 6ef7db521716..ee936c2f394a 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -47,6 +47,7 @@ struct nvmet_ns {
loff_t size;
u8 nguid[16];
u8 uuid[16];
+ u8 eui64[8];

bool enabled;
struct nvmet_subsys *subsys;
--
2.12.0

[Johannes Thumshirn]

If a target identifies itself as NVMe 1.3 compliant, try to get the
list of Namespace Identification Descriptors and populate the UUID,
NGUID and EUI64 fileds in the NVMe namespace structure with these
values.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/host/core.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
drivers/nvme/host/nvme.h | 1 +
2 files changed, 89 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 7b254be16887..d2365f7ea612 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -643,6 +643,71 @@ int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
return error;
}

+static void nvme_parse_ns_descs(struct nvme_ns *ns, void *ns_nid)
+{
+ struct nvme_ns_nid *cur;
+ const u8 *p;
+ int pos = 0;
+ int len;
+
+ p = (u8 *)ns_nid;
+
+ for (;;) {
+ cur = (struct nvme_ns_nid *)p;
+
+ switch (cur->nidl) {
+ case 0:
+ return;
+ case 8:
+ case 16:
+ break;
+ default:
+ dev_warn(ns->ctrl->dev,
+ "Target returned bogus Namespace Identification Descriptor length: %d\n",
+ cur->nidl);
+ return;
+
+ }
+
+ switch (cur->nidt) {
+ case NVME_NIDT_EUI64:
+ len = 8;
+ memcpy(ns->eui, cur->nid, len);
+ break;
+ case NVME_NIDT_NGUID:
+ len = 16;
+ memcpy(ns->nguid, cur->nid, len);
+ break;
+ case NVME_NIDT_UUID:
+ len = 16;
+ memcpy(ns->uuid, cur->nid, len);
+ break;
+ default:
+ dev_warn(ns->ctrl->dev,
+ "Invalid Namespace Identification Descriptor Type: %d\n",
+ cur->nidt);
+ return;
+ }
+
+ pos += sizeof(struct nvme_ns_nid) + len;
+ if (pos >= 4096)
+ return;
+ p += pos;
+ }
+}
+
+static int nvme_identify_ns_descs(struct nvme_ctrl *dev, unsigned nsid,
+ void *ns_nid)
+{
+ struct nvme_command c = { };
+
+ c.identify.opcode = nvme_admin_identify;
+ c.identify.nsid = cpu_to_le32(nsid);
+ c.identify.cns = NVME_ID_CNS_NS_DESC_LIST;
+
+ return nvme_submit_sync_cmd(dev->admin_q, &c, ns_nid, 4096);
+}
+
static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *ns_list)
{
struct nvme_command c = { };
@@ -1017,6 +1082,29 @@ static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id)
memcpy(ns->eui, (*id)->eui64, sizeof(ns->eui));
if (ns->ctrl->vs >= NVME_VS(1, 2, 0))
memcpy(ns->nguid, (*id)->nguid, sizeof(ns->nguid));
+ if (ns->ctrl->vs >= NVME_VS(1, 3, 0)) {
+ void *ns_nid;
+ int ret;
+
+
+ ns_nid = kzalloc(4096, GFP_KERNEL);
+ if (!ns_nid) {
+ dev_warn(ns->ctrl->dev,
+ "%s: Identify Descriptors failed\n", __func__);
+ return 0;
+ }
+
+ ret = nvme_identify_ns_descs(ns->ctrl, ns->ns_id, ns_nid);
+ if (ret) {
+ dev_warn(ns->ctrl->dev,
+ "%s: Identify Descriptors failed\n", __func__);
+ /* Don't treat error as fatal we potentially
+ * already have a NGUID or EUI-64 */
+ return 0;
+ }
+ nvme_parse_ns_descs(ns, ns_nid);
+ kfree(ns_nid);
+ }

return 0;
}
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 5004f0c41397..7007521e8194 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -190,6 +190,7 @@ struct nvme_ns {

u8 eui[8];
u8 nguid[16];
+ u8 uuid[16];

unsigned ns_id;
int lba_shift;
--
2.12.0

[Johannes Thumshirn]

Now that we have a way for getting the UUID from a target, provide it
to userspace as well.

Unfortunately there is already a sysfs attribute called UUID which is
a misnomer as it holds the NGUID value. So instead of creating yet
another wrong name, create a new 'nguid' sysfs attribute for the
NGUID. For the UUID attribute add a check wheter the namespace has a
UUID assigned to it and return this or return the NGUID to maintain
backwards compatibility. This should give userspace a chance to catch
up.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/host/core.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d2365f7ea612..96c063bad48e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1888,11 +1888,25 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
}
static DEVICE_ATTR(wwid, S_IRUGO, wwid_show, NULL);

+static ssize_t nguid_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
+ return sprintf(buf, "%pU\n", ns->nguid);
+}
+static DEVICE_ATTR(nguid, S_IRUGO, nguid_show, NULL);
+
static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
- return sprintf(buf, "%pU\n", ns->nguid);
+
+ /* For backward compatibility expose the NGUID to userspace if
+ * we have no UUID set
+ */
+ if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid)))
+ return sprintf(buf, "%pU\n", ns->nguid);
+ return sprintf(buf, "%pU\n", ns->uuid);
}
static DEVICE_ATTR(uuid, S_IRUGO, uuid_show, NULL);

@@ -1915,6 +1929,7 @@ static DEVICE_ATTR(nsid, S_IRUGO, nsid_show, NULL);
static struct attribute *nvme_ns_attrs[] = {
&dev_attr_wwid.attr,
&dev_attr_uuid.attr,
+ &dev_attr_nguid.attr,
&dev_attr_eui.attr,
&dev_attr_nsid.attr,
NULL,
@@ -1927,6 +1942,11 @@ static umode_t nvme_ns_attrs_are_visible(struct kobject *kobj,
struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

if (a == &dev_attr_uuid.attr) {
+ if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid)) ||
+ !memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
+ return 0;
+ }
+ if (a == &dev_attr_nguid.attr) {
if (!memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
return 0;
}
--
2.12.0

[Johannes Thumshirn]

A NVMe Identify NS command with a CNS value of '3' is expecting a list
of Namespace Identification Descriptor structures to be returned to
the host for the namespace requested in the namespace identify
command.

This Namespace Identification Descriptor structure consists of the
type of the namespace identifier, the length of the identifier and the
actual identifier.

Valid types are EUI-64, NGUID and UUID which we have saved in our
nvme_ns structure if they have been configured via configfs. If no
value has been assigened to one of these we return an "invalid opcode"
back to the host to maintain backward compatibiliy with older
implementations without Namespace Identify Descriptor list support.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/target/admin-cmd.c | 75 +++++++++++++++++++++++++++++++++++++++++
include/linux/nvme.h | 14 ++++++++

2 files changed, 89 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 60c9eec57986..2290301a6172 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -368,6 +368,78 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req)
nvmet_req_complete(req, status);
}

+static void nvmet_execute_identify_desclist(struct nvmet_req *req)
+{
+ static const int buf_size = 4069;
+ struct nvmet_ns *ns;
+ struct nvme_ns_nid *ns_nid;
+ u8 *nid_list;
+ u16 status = 0;

+ int pos = 0;

+ const u8 *p;
+
+ ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid);
+ if (!ns) {
+ status = NVME_SC_INVALID_NS | NVME_SC_DNR;
+ goto out;
+ }
+
+ nid_list = kzalloc(buf_size, GFP_KERNEL);
+ if (!nid_list) {
+ status = NVME_SC_INTERNAL;
+ goto out_put_ns;
+ }
+
+ p = nid_list;
+
+ if (memchr_inv(ns->uuid, 0, sizeof(ns->uuid))) {
+ ns_nid = (struct nvme_ns_nid *)p;
+ ns_nid->nidt = NVME_NIDT_UUID;
+ ns_nid->nidl = 16;
+ memcpy(&ns_nid->nid, &ns->uuid, sizeof(ns->uuid));
+ pos += sizeof(struct nvme_ns_nid) + sizeof(ns->uuid);
+ if (pos > buf_size) {
+ status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+ goto out_free_nid_list;
+ }

+ p += pos;
+ }

+ if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid))) {
+ ns_nid = (struct nvme_ns_nid *)p;
+ ns_nid->nidt = NVME_NIDT_NGUID;
+ ns_nid->nidl = 16;
+ memcpy(&ns_nid->nid, &ns->nguid, sizeof(ns->nguid));
+ pos += sizeof(struct nvme_ns_nid) + sizeof(ns->nguid);
+ if (pos > buf_size) {
+ status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+ goto out_free_nid_list;
+ }

+ p += pos;
+ }

+ if (memchr_inv(ns->eui64, 0, sizeof(ns->eui64))) {
+ ns_nid = (struct nvme_ns_nid *)p;
+ ns_nid->nidt = NVME_NIDT_EUI64;
+ ns_nid->nidl = 8;
+ memcpy(&ns_nid->nid, &ns->eui64, sizeof(ns->eui64));
+ pos += sizeof(struct nvme_ns_nid) + sizeof(ns->eui64);
+ if (pos > buf_size) {
+ status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+ goto out_free_nid_list;
+ }

+ p += pos;
+ }
+

+ status = nvmet_copy_to_sgl(req, 0, nid_list, buf_size);
+
+out_free_nid_list:
+ kfree(nid_list);
+
+out_put_ns:
+ nvmet_put_namespace(ns);
+out:
+ nvmet_req_complete(req, status);
+}
+
/*
* A "mimimum viable" abort implementation: the command is mandatory in the
* spec, but we are not required to do any useful work. We couldn't really
@@ -516,6 +588,9 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
case NVME_ID_CNS_NS_ACTIVE_LIST:
req->execute = nvmet_execute_identify_nslist;
return 0;
+ case NVME_ID_CNS_NS_DESC_LIST:
+ req->execute = nvmet_execute_identify_desclist;
+ return 0;
}
break;
case nvme_admin_abort_cmd:
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index b625bacf37ef..cad0e19f0bba 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -288,6 +288,7 @@ enum {
NVME_ID_CNS_NS = 0x00,
NVME_ID_CNS_CTRL = 0x01,
NVME_ID_CNS_NS_ACTIVE_LIST = 0x02,
+ NVME_ID_CNS_NS_DESC_LIST = 0x03,
NVME_ID_CNS_NS_PRESENT_LIST = 0x10,
NVME_ID_CNS_NS_PRESENT = 0x11,
NVME_ID_CNS_CTRL_NS_LIST = 0x12,
@@ -314,6 +315,19 @@ enum {
NVME_NS_DPS_PI_TYPE3 = 3,
};

+struct nvme_ns_nid {
+ __u8 nidt;
+ __u8 nidl;
+ __le16 reserved;
+ __u8 nid[0];
+};
+
+enum {
+ NVME_NIDT_EUI64 = 0x01,
+ NVME_NIDT_NGUID = 0x02,
+ NVME_NIDT_UUID = 0x03,
+};
+
struct nvme_smart_log {
__u8 critical_warning;
__u8 temperature[2];
--
2.12.0

[Hannes Reinecke]

On 05/30/2017 10:08 AM, Johannes Thumshirn wrote:
> Add the EUI-64 field from the NVMe Namespace Identification Descriptor
> to the nvmet_ns structure and allow it's population via configfs.
>
> Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
> ---
> drivers/nvme/target/configfs.c | 48 ++++++++++++++++++++++++++++++++++++++++++
> drivers/nvme/target/nvmet.h | 1 +
> 2 files changed, 49 insertions(+)
>

Reviewed-by: Hannes Reinecke <ha...@suse.com>

Cheers,

Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
ha...@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[Hannes Reinecke]

On 05/30/2017 10:08 AM, Johannes Thumshirn wrote:

> A NVMe Identify NS command with a CNS value of '3' is expecting a list
> of Namespace Identification Descriptor structures to be returned to
> the host for the namespace requested in the namespace identify
> command.
>
> This Namespace Identification Descriptor structure consists of the
> type of the namespace identifier, the length of the identifier and the
> actual identifier.
>
> Valid types are EUI-64, NGUID and UUID which we have saved in our
> nvme_ns structure if they have been configured via configfs. If no
> value has been assigened to one of these we return an "invalid opcode"
> back to the host to maintain backward compatibiliy with older
> implementations without Namespace Identify Descriptor list support.
>
> Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
> ---
> drivers/nvme/target/admin-cmd.c | 75 +++++++++++++++++++++++++++++++++++++++++
> include/linux/nvme.h | 14 ++++++++
> 2 files changed, 89 insertions(+)
>

[Hannes Reinecke]

On 05/30/2017 10:08 AM, Johannes Thumshirn wrote:

> Now that we have a way for getting the UUID from a target, provide it
> to userspace as well.
>
> Unfortunately there is already a sysfs attribute called UUID which is
> a misnomer as it holds the NGUID value. So instead of creating yet
> another wrong name, create a new 'nguid' sysfs attribute for the
> NGUID. For the UUID attribute add a check wheter the namespace has a
> UUID assigned to it and return this or return the NGUID to maintain
> backwards compatibility. This should give userspace a chance to catch
> up.
>
> Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
> ---
> drivers/nvme/host/core.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>

[Hannes Reinecke]

On 05/30/2017 10:08 AM, Johannes Thumshirn wrote:

Personally, I don't like the dependency on version 1.3; especially
seeing that we're not supporting all 1.3 features (yet).

Meaning to test this we'd need to keep an additional out-of-tree patch,
which is not very appealing to me.

Maybe it's an idea to add another sysfs attribute to nvmet allowing us
to specify the version number? That would make life _so_ much easier.

But the patch itself is okay.

[Christoph Hellwig]

On Tue, May 30, 2017 at 10:08:18AM +0200, Johannes Thumshirn wrote:
> Add the EUI-64 field from the NVMe Namespace Identification Descriptor
> to the nvmet_ns structure and allow it's population via configfs.

Is there any good use case for bothering with this identifier that's
too short to actually be useful?

[Christoph Hellwig]

On Tue, May 30, 2017 at 10:08:22AM +0200, Johannes Thumshirn wrote:
> Now that we have a way for getting the UUID from a target, provide it
> to userspace as well.
>
> Unfortunately there is already a sysfs attribute called UUID which is
> a misnomer as it holds the NGUID value. So instead of creating yet
> another wrong name, create a new 'nguid' sysfs attribute for the
> NGUID. For the UUID attribute add a check wheter the namespace has a
> UUID assigned to it and return this or return the NGUID to maintain
> backwards compatibility. This should give userspace a chance to catch
> up.

Eww. This looks pretty sketchy and at least needs a good comment in
the source. Maybe even a printk_ratelimited warning.

[Johannes Thumshirn]

Mostly consistency. The current nvme host code has the EUI sprinkled all
around. Sure I can drop it, but then what's the point in evaluating it
on the host side? Other targets may send it, so we need in on the host
and do we care about potentially awkward host implementations with Linux
as a target? Also it's rather handy for testing as well, after all it's
not too much and complex code.
--
Johannes Thumshirn Storage
jthum...@suse.de +49 911 74053 689

SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg

GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[Johannes Thumshirn]

On 05/30/2017 11:30 AM, Christoph Hellwig wrote:
> Eww. This looks pretty sketchy and at least needs a good comment in
> the source. Maybe even a printk_ratelimited warning.

Comment sure, but do we really need a warning?

[Christoph Hellwig]

On Tue, May 30, 2017 at 11:45:25AM +0200, Johannes Thumshirn wrote:
> Mostly consistency. The current nvme host code has the EUI sprinkled all
> around. Sure I can drop it, but then what's the point in evaluating it
> on the host side? Other targets may send it, so we need in on the host
> and do we care about potentially awkward host implementations with Linux
> as a target? Also it's rather handy for testing as well, after all it's
> not too much and complex code.

There's only one place in the host code proper (discounting the
scsi translation mess), and that's because the field is still better
than not having any uniqueue identifier. But I don't think we should
spread it any further.

[Christoph Hellwig]

On Tue, May 30, 2017 at 10:08:15AM +0200, Johannes Thumshirn wrote:
> A patch for nvmetcli will follow shortly.

Thanks. No really required but nice would be a nvme-cli subcommand
to read the values as well.

[Johannes Thumshirn]

OK I'll drop it in my next re-send (which should hopefully be somewhen
today)

[Johannes Thumshirn]

Yes, together with other fabric specifics. I have a customer request for
it as well, just didn't have the time to do so yet.

[Johannes Thumshirn]

If a target identifies itself as NVMe 1.3 compliant, try to get the
list of Namespace Identification Descriptors and populate the UUID,
NGUID and EUI64 fileds in the NVMe namespace structure with these
values.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---
drivers/nvme/host/core.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
drivers/nvme/host/nvme.h | 1 +
2 files changed, 89 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c

index 7b254be16887..9acbf56c8796 100644

+ len = NVME_NIDT_EUI64_LEN;

+ memcpy(ns->eui, cur->nid, len);
+ break;
+ case NVME_NIDT_NGUID:

+ len = NVME_NIDT_NGUID_LEN;

+ memcpy(ns->nguid, cur->nid, len);
+ break;
+ case NVME_NIDT_UUID:

+ len = NVME_NIDT_UUID_LEN;

+ memcpy(ns->uuid, cur->nid, len);
+ break;
+ default:
+ dev_warn(ns->ctrl->dev,
+ "Invalid Namespace Identification Descriptor Type: %d\n",
+ cur->nidt);
+ return;
+ }
+
+ pos += sizeof(struct nvme_ns_nid) + len;

+ if (pos >= SZ_4K)

+ return;
+ p += pos;
+ }
+}
+
+static int nvme_identify_ns_descs(struct nvme_ctrl *dev, unsigned nsid,
+ void *ns_nid)
+{
+ struct nvme_command c = { };
+
+ c.identify.opcode = nvme_admin_identify;
+ c.identify.nsid = cpu_to_le32(nsid);
+ c.identify.cns = NVME_ID_CNS_NS_DESC_LIST;
+

+ return nvme_submit_sync_cmd(dev->admin_q, &c, ns_nid, SZ_4K);

+}
+
static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *ns_list)
{
struct nvme_command c = { };
@@ -1017,6 +1082,29 @@ static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id)
memcpy(ns->eui, (*id)->eui64, sizeof(ns->eui));
if (ns->ctrl->vs >= NVME_VS(1, 2, 0))
memcpy(ns->nguid, (*id)->nguid, sizeof(ns->nguid));
+ if (ns->ctrl->vs >= NVME_VS(1, 3, 0)) {
+ void *ns_nid;
+ int ret;
+
+

+ ns_nid = kzalloc(SZ_4K, GFP_KERNEL);

--
2.12.0

[Johannes Thumshirn]

This patchset implemets NVMe Namespace Descriptor Identification as of
NVMe 1.3. The Namespace Descriptor Identification allows a NVMe host
to query several Namespace Identification mechanisms, such as EUI-64,
NGUID and UUID from the target. If more than one value is set by the
target, it can transmit all set values to the host.

The Namespace Identification Descriptor list is the only way a target
can identify itself via the newly introduced UUID to the host (instead
of the EUI-64 or NGUID).

Both the Host and Target side are implemented. In order to get the
Linux Host to send the Linux target implementation a Namespace
Descriptor Identification command, you have to change the target's
announced version code to at least 1.3.

Unfortunately the host side already did have a sysfs attribute called
'uuid' which represented the NGUID, so precautions have been taken to
not break any existing userspace.

The code is tested using the nvme-loop loopback target and cut against
the nvme tree's nvme-4.12 branch.

A patch for nvmetcli will follow shortly.

Changes to v0:
* Fixed wrong size of 4069 and replaced it with SZ_4K (Max)
* Add constants for UUID, NGUID and EUI-64 length (Max)
* Drop EUI-64 Support on target side (Christoph)
* Use uuid_be instead of u8[] (Christoph)
* Add ability to override target's version (Hannes)
* Change hard coded magic 4096 and 0x1000 to SZ_4k in drivers/nvme/*

Johannes Thumshirn (7):
nvme: rename uuid to nguid in nvme_ns
nvmet: add uuid field to nvme_ns and populate via configfs

nvmet: implement namespace identify descriptor list
nvme: get list of namespace descriptors
nvme: provide UUID value to userspace

nvme: change magic 4096 to SZ_4K
nvmet: allow overriding the NVMe VS via configfs

drivers/nvme/host/core.c | 123 ++++++++++++++++++++++++++++++++++++++--
drivers/nvme/host/nvme.h | 1 +
drivers/nvme/host/pci.c | 6 +-
drivers/nvme/target/admin-cmd.c | 67 +++++++++++++++++++++-
drivers/nvme/target/configfs.c | 65 +++++++++++++++++++++
drivers/nvme/target/discovery.c | 2 +-
drivers/nvme/target/nvmet.h | 1 +
include/linux/nvme.h | 22 +++++++
8 files changed, 275 insertions(+), 12 deletions(-)

--
2.12.0

[Johannes Thumshirn]

Add the UUID field from the NVMe Namespace Identification Descriptor

to the nvmet_ns structure and allow it's population via configfs.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/target/configfs.c | 31 +++++++++++++++++++++++++++++++
drivers/nvme/target/nvmet.h | 1 +
2 files changed, 32 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index be8c800078e2..16f9f6e3a084 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -305,11 +305,41 @@ static ssize_t nvmet_ns_device_path_store(struct config_item *item,

CONFIGFS_ATTR(nvmet_ns_, device_path);

+static ssize_t nvmet_ns_device_uuid_show(struct config_item *item, char *page)
+{
+ return sprintf(page, "%pUb\n", &to_nvmet_ns(item)->uuid);
+}
+
+static ssize_t nvmet_ns_device_uuid_store(struct config_item *item,

+ const char *page, size_t count)
+{
+ struct nvmet_ns *ns = to_nvmet_ns(item);
+ struct nvmet_subsys *subsys = ns->subsys;

+ int ret = 0;
+
+
+ mutex_lock(&subsys->lock);
+ if (ns->enabled) {
+ ret = -EBUSY;
+ goto out_unlock;
+ }
+
+

+ if (uuid_be_to_bin(page, &ns->uuid))

+ ret = -EINVAL;
+

+out_unlock:
+ mutex_unlock(&subsys->lock);
+ return ret ? ret : count;
+}
+

static ssize_t nvmet_ns_device_nguid_show(struct config_item *item, char *page)
{
return sprintf(page, "%pUb\n", &to_nvmet_ns(item)->nguid);
}

+CONFIGFS_ATTR(nvmet_ns_, device_uuid);
+
static ssize_t nvmet_ns_device_nguid_store(struct config_item *item,

const char *page, size_t count)

{
@@ -379,6 +409,7 @@ CONFIGFS_ATTR(nvmet_ns_, enable);

static struct configfs_attribute *nvmet_ns_attrs[] = {
&nvmet_ns_attr_device_path,
&nvmet_ns_attr_device_nguid,

+ &nvmet_ns_attr_device_uuid,

&nvmet_ns_attr_enable,
NULL,
};
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h

index cfc5c7fb0ab7..4c6cb5ea1186 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -46,6 +46,7 @@ struct nvmet_ns {
u32 blksize_shift;

loff_t size;
u8 nguid[16];

+ uuid_be uuid;

[Johannes Thumshirn]

A NVMe Identify NS command with a CNS value of '3' is expecting a list
of Namespace Identification Descriptor structures to be returned to
the host for the namespace requested in the namespace identify
command.

This Namespace Identification Descriptor structure consists of the
type of the namespace identifier, the length of the identifier and the
actual identifier.

Valid types are EUI-64, NGUID and UUID which we have saved in our
nvme_ns structure if they have been configured via configfs. If no
value has been assigened to one of these we return an "invalid opcode"
back to the host to maintain backward compatibiliy with older
implementations without Namespace Identify Descriptor list support.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/target/admin-cmd.c | 63 +++++++++++++++++++++++++++++++++++++++++
include/linux/nvme.h | 18 ++++++++++++
2 files changed, 81 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index ff1f97006322..f53cab51b090 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -367,6 +367,66 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req)

nvmet_req_complete(req, status);
}

+static void nvmet_execute_identify_desclist(struct nvmet_req *req)
+{

+ static const int buf_size = SZ_4K;

+ struct nvmet_ns *ns;
+ struct nvme_ns_nid *ns_nid;
+ u8 *nid_list;
+ u16 status = 0;

+ int pos = 0;

+ const u8 *p;
+
+ ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid);
+ if (!ns) {
+ status = NVME_SC_INVALID_NS | NVME_SC_DNR;
+ goto out;
+ }
+
+ nid_list = kzalloc(buf_size, GFP_KERNEL);
+ if (!nid_list) {
+ status = NVME_SC_INTERNAL;
+ goto out_put_ns;
+ }
+
+ p = nid_list;
+

+ if (memchr_inv(&ns->uuid, 0, sizeof(ns->uuid))) {

+ ns_nid = (struct nvme_ns_nid *)p;
+ ns_nid->nidt = NVME_NIDT_UUID;

+ ns_nid->nidl = NVME_NIDT_UUID_LEN;

+ memcpy(&ns_nid->nid, &ns->uuid, sizeof(ns->uuid));
+ pos += sizeof(struct nvme_ns_nid) + sizeof(ns->uuid);
+ if (pos > buf_size) {
+ status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+ goto out_free_nid_list;
+ }

+ p += pos;
+ }

+ if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid))) {
+ ns_nid = (struct nvme_ns_nid *)p;
+ ns_nid->nidt = NVME_NIDT_NGUID;

+ ns_nid->nidl = NVME_NIDT_NGUID_LEN;

+ memcpy(&ns_nid->nid, &ns->nguid, sizeof(ns->nguid));
+ pos += sizeof(struct nvme_ns_nid) + sizeof(ns->nguid);
+ if (pos > buf_size) {
+ status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+ goto out_free_nid_list;
+ }

+ p += pos;
+ }
+

+ status = nvmet_copy_to_sgl(req, 0, nid_list, buf_size);
+
+out_free_nid_list:
+ kfree(nid_list);
+
+out_put_ns:
+ nvmet_put_namespace(ns);
+out:
+ nvmet_req_complete(req, status);
+}
+
/*
* A "mimimum viable" abort implementation: the command is mandatory in the
* spec, but we are not required to do any useful work. We couldn't really

@@ -515,6 +575,9 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)

case NVME_ID_CNS_NS_ACTIVE_LIST:
req->execute = nvmet_execute_identify_nslist;
return 0;
+ case NVME_ID_CNS_NS_DESC_LIST:
+ req->execute = nvmet_execute_identify_desclist;
+ return 0;
}
break;
case nvme_admin_abort_cmd:
diff --git a/include/linux/nvme.h b/include/linux/nvme.h

index b625bacf37ef..0a62e843ecb0 100644

--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -288,6 +288,7 @@ enum {
NVME_ID_CNS_NS = 0x00,
NVME_ID_CNS_CTRL = 0x01,
NVME_ID_CNS_NS_ACTIVE_LIST = 0x02,
+ NVME_ID_CNS_NS_DESC_LIST = 0x03,
NVME_ID_CNS_NS_PRESENT_LIST = 0x10,
NVME_ID_CNS_NS_PRESENT = 0x11,
NVME_ID_CNS_CTRL_NS_LIST = 0x12,

@@ -314,6 +315,23 @@ enum {

NVME_NS_DPS_PI_TYPE3 = 3,
};

+struct nvme_ns_nid {
+ __u8 nidt;
+ __u8 nidl;
+ __le16 reserved;
+ __u8 nid[0];
+};
+

+#define NVME_NIDT_EUI64_LEN 8
+#define NVME_NIDT_NGUID_LEN 16
+#define NVME_NIDT_UUID_LEN 16

[Johannes Thumshirn]

The uuid field in the nvme_ns structure represents the nguid field
from the identify namespace command. And as NVMe 1.3 introduced an
UUID in the NVMe Namespace Identification Descriptor this will
collide.

So rename the uuid to nguid to prevent any further
confusion. Unfortunately we export the nguid to sysfs in the uuid
sysfs attribute, but this can't be changed anymore without possibly
breaking existing userspace.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/host/core.c | 10 +++++-----
drivers/nvme/host/nvme.h | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a60926410438..7b254be16887 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1016,7 +1016,7 @@ static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id)
if (ns->ctrl->vs >= NVME_VS(1, 1, 0))

memcpy(ns->eui, (*id)->eui64, sizeof(ns->eui));
if (ns->ctrl->vs >= NVME_VS(1, 2, 0))

- memcpy(ns->uuid, (*id)->nguid, sizeof(ns->uuid));
+ memcpy(ns->nguid, (*id)->nguid, sizeof(ns->nguid));

return 0;
}
@@ -1784,8 +1784,8 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
int serial_len = sizeof(ctrl->serial);
int model_len = sizeof(ctrl->model);

- if (memchr_inv(ns->uuid, 0, sizeof(ns->uuid)))
- return sprintf(buf, "eui.%16phN\n", ns->uuid);

+ if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))

+ return sprintf(buf, "eui.%16phN\n", ns->nguid);

if (memchr_inv(ns->eui, 0, sizeof(ns->eui)))
return sprintf(buf, "eui.%8phN\n", ns->eui);
@@ -1804,7 +1804,7 @@ static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,

char *buf)
{
struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

- return sprintf(buf, "%pU\n", ns->uuid);

+ return sprintf(buf, "%pU\n", ns->nguid);
}

static DEVICE_ATTR(uuid, S_IRUGO, uuid_show, NULL);

@@ -1839,7 +1839,7 @@ static umode_t nvme_ns_attrs_are_visible(struct kobject *kobj,

struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

if (a == &dev_attr_uuid.attr) {

- if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid)))
+ if (!memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
return 0;
}
if (a == &dev_attr_eui.attr) {
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 9d6a070d4391..5004f0c41397 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -189,7 +189,7 @@ struct nvme_ns {
int instance;

u8 eui[8];
- u8 uuid[16];
+ u8 nguid[16];

[Johannes Thumshirn]

Now that we have a way for getting the UUID from a target, provide it
to userspace as well.

Unfortunately there is already a sysfs attribute called UUID which is
a misnomer as it holds the NGUID value. So instead of creating yet
another wrong name, create a new 'nguid' sysfs attribute for the
NGUID. For the UUID attribute add a check wheter the namespace has a
UUID assigned to it and return this or return the NGUID to maintain
backwards compatibility. This should give userspace a chance to catch
up.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/host/core.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9acbf56c8796..4de162e4e13e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1888,11 +1888,28 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,

}
static DEVICE_ATTR(wwid, S_IRUGO, wwid_show, NULL);

+static ssize_t nguid_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{

+ struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

+ return sprintf(buf, "%pU\n", ns->nguid);

+}
+static DEVICE_ATTR(nguid, S_IRUGO, nguid_show, NULL);
+

static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

- return sprintf(buf, "%pU\n", ns->nguid);
+
+ /* For backward compatibility expose the NGUID to userspace if
+ * we have no UUID set
+ */

+ if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid))) {
+ printk_ratelimited(KERN_WARNING
+ "No UUID available providing old NGUID\n");

+ return sprintf(buf, "%pU\n", ns->nguid);

+ }
+ return sprintf(buf, "%pU\n", ns->uuid);

}
static DEVICE_ATTR(uuid, S_IRUGO, uuid_show, NULL);

@@ -1915,6 +1932,7 @@ static DEVICE_ATTR(nsid, S_IRUGO, nsid_show, NULL);

static struct attribute *nvme_ns_attrs[] = {
&dev_attr_wwid.attr,
&dev_attr_uuid.attr,
+ &dev_attr_nguid.attr,
&dev_attr_eui.attr,
&dev_attr_nsid.attr,
NULL,

@@ -1927,6 +1945,11 @@ static umode_t nvme_ns_attrs_are_visible(struct kobject *kobj,

struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

if (a == &dev_attr_uuid.attr) {

+ if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid)) ||
+ !memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
+ return 0;
+ }
+ if (a == &dev_attr_nguid.attr) {

if (!memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
return 0;
}

--
2.12.0

[Johannes Thumshirn]

Allow overriding the announced NVMe Version of a via configfs.

This is particularly helpful when debugging new features for the host
or target side without bumping the hard coded version (as the target
might not be fully compliant to the announced version yet).

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/target/configfs.c | 34 ++++++++++++++++++++++++++++++++++
include/linux/nvme.h | 4 ++++
2 files changed, 38 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 16f9f6e3a084..45421d4308a4 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -650,8 +650,42 @@ static ssize_t nvmet_subsys_attr_allow_any_host_store(struct config_item *item,

CONFIGFS_ATTR(nvmet_subsys_, attr_allow_any_host);

+static ssize_t nvmet_subsys_version_show(struct config_item *item,
+ char *page)
+{
+ struct nvmet_subsys *subsys = to_subsys(item);
+ int major, minor, tertiary;
+ u32 ver;
+
+ ver = subsys->ver;
+ major = NVME_MAJOR(ver);
+ minor = NVME_MINOR(ver);
+ tertiary = NVME_TERRIARY(ver);
+
+ return snprintf(page, PAGE_SIZE, "%d %d %d\n", major, minor, tertiary);
+}
+
+static ssize_t nvmet_subsys_version_store(struct config_item *item,

+ const char *page, size_t count)
+{

+ struct nvmet_subsys *subsys = to_subsys(item);
+ int major, minor, tertiary;

+ int ret;
+
+

+ ret = sscanf(page, "%d %d %d\n", &major, &minor, &tertiary);
+ if (ret != 3)
+ return -EINVAL;
+
+ subsys->ver = NVME_VS(major, minor, tertiary);
+
+ return count;
+}
+CONFIGFS_ATTR(nvmet_subsys_, version);
+
static struct configfs_attribute *nvmet_subsys_attrs[] = {
&nvmet_subsys_attr_attr_allow_any_host,
+ &nvmet_subsys_attr_version,
NULL,
};

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 0a62e843ecb0..dda098c2281b 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1068,4 +1068,8 @@ struct nvme_completion {
#define NVME_VS(major, minor, tertiary) \
(((major) << 16) | ((minor) << 8) | (tertiary))

+#define NVME_MAJOR(ver) ((ver) >> 16)
+#define NVME_MINOR(ver) (((ver) >> 8) & 0xff)
+#define NVME_TERRIARY(ver) ((ver) & 0xff)
+
#endif /* _LINUX_NVME_H */
--
2.12.0

[Christoph Hellwig]

This code looks fine, but I'd rather move it past adding CNS 3h support
so that it can add all the uuid-related bits in one go.

[Christoph Hellwig]

On Wed, May 31, 2017 at 03:32:11PM +0200, Johannes Thumshirn wrote:
> The uuid field in the nvme_ns structure represents the nguid field
> from the identify namespace command. And as NVMe 1.3 introduced an
> UUID in the NVMe Namespace Identification Descriptor this will
> collide.
>
> So rename the uuid to nguid to prevent any further
> confusion. Unfortunately we export the nguid to sysfs in the uuid
> sysfs attribute, but this can't be changed anymore without possibly
> breaking existing userspace.
>
> Signed-off-by: Johannes Thumshirn <jthum...@suse.de>

Looks fine,

Reviewed-by: Christoph Hellwig <h...@lst.de>

[Christoph Hellwig]

Looks fine:

Reviewed-by: Christoph Hellwig <h...@lst.de>

[Christoph Hellwig]

> +static void nvme_parse_ns_descs(struct nvme_ns *ns, void *ns_nid)
> +{
> + struct nvme_ns_nid *cur;
> + const u8 *p;
> + int pos = 0;
> + int len;
> +
> + p = (u8 *)ns_nid;

No need for the cast. But we can do void pointer arithmetics in the
kernel anyway, so possible we could just declare this void.

And maybe just call the argument data and operate on that.

> +
> + for (;;) {
> + cur = (struct nvme_ns_nid *)p;

for (pos = 0; pos < NVME_ID_DATA_SIZE; pos += len) {
struct nvme_ns_nid *cur = data + pos;

> +
> + switch (cur->nidl) {
> + case 0:
> + return;
> + case 8:
> + case 16:
> + break;
> + default:
> + dev_warn(ns->ctrl->dev,
> + "Target returned bogus Namespace Identification Descriptor length: %d\n",
> + cur->nidl);
> + return;
> +
> + }

This needs to be verified based on the type.

> + if (ns->ctrl->vs >= NVME_VS(1, 3, 0)) {
> + void *ns_nid;
> + int ret;
> +
> +
> + ns_nid = kzalloc(SZ_4K, GFP_KERNEL);
> + if (!ns_nid) {
> + dev_warn(ns->ctrl->dev,
> + "%s: Identify Descriptors failed\n", __func__);
> + return 0;
> + }
> +
> + ret = nvme_identify_ns_descs(ns->ctrl, ns->ns_id, ns_nid);
> + if (ret) {
> + dev_warn(ns->ctrl->dev,
> + "%s: Identify Descriptors failed\n", __func__);
> + /* Don't treat error as fatal we potentially
> + * already have a NGUID or EUI-64 */
> + return 0;
> + }
> + nvme_parse_ns_descs(ns, ns_nid);
> + kfree(ns_nid);

Please move all this code into nvme_identify_ns_descs().

[Christoph Hellwig]

On Wed, May 31, 2017 at 03:32:13PM +0200, Johannes Thumshirn wrote:
> A NVMe Identify NS command with a CNS value of '3' is expecting a list
> of Namespace Identification Descriptor structures to be returned to
> the host for the namespace requested in the namespace identify
> command.
>
> This Namespace Identification Descriptor structure consists of the
> type of the namespace identifier, the length of the identifier and the
> actual identifier.
>
> Valid types are EUI-64, NGUID and UUID which we have saved in our
> nvme_ns structure if they have been configured via configfs. If no
> value has been assigened to one of these we return an "invalid opcode"
> back to the host to maintain backward compatibiliy with older
> implementations without Namespace Identify Descriptor list support.
>
> Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
> ---
> drivers/nvme/target/admin-cmd.c | 63 +++++++++++++++++++++++++++++++++++++++++
> include/linux/nvme.h | 18 ++++++++++++

Can you split all the new structures in nvme.h into a separate patch
at the beginning of the series?

> + static const int buf_size = SZ_4K;

Please add a NVME_IDENTIFY_DATA_SIZE define to nvme.h (and I'd slighty
prefer to define it to 4096 instead of the odd SZ_4K). And also replace
all the places where we use a magic 4096 for the identify payload with
it.

> + nid_list = kzalloc(buf_size, GFP_KERNEL);
> + if (!nid_list) {
> + status = NVME_SC_INTERNAL;
> + goto out_put_ns;
> + }
> +
> + p = nid_list;

No need for the dynamic allocation. Just do a straight nvmet_copy_to_sgl
from the stack for each element.

> +
> + if (memchr_inv(&ns->uuid, 0, sizeof(ns->uuid))) {
> + ns_nid = (struct nvme_ns_nid *)p;
> + ns_nid->nidt = NVME_NIDT_UUID;
> + ns_nid->nidl = NVME_NIDT_UUID_LEN;
> + memcpy(&ns_nid->nid, &ns->uuid, sizeof(ns->uuid));
> + pos += sizeof(struct nvme_ns_nid) + sizeof(ns->uuid);
> + if (pos > buf_size) {
> + status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
> + goto out_free_nid_list;
> + }
> + p += pos;
> + }

E.g. something like

struct nvme_ns_identifier_hdr nih;

...

memset(&nih, 0, sizeof(nih));
nih.nidt = NVME_NIDT_UUID;
nih.nidl = NVME_NIDT_UUID_LEN;
status = nvmet_copy_to_sgl(req, off, &nih, sizeof(nih));
if (status)
goto out_put_ns;
off += sizeof(nih);

status = nvmet_copy_to_sgl(req, off, &ns->uuid,
sizeof(ns->uuid));
if (status)
goto out_put_ns;
off += sizeof(ns->uuid);

[Christoph Hellwig]

On Wed, May 31, 2017 at 03:32:17PM +0200, Johannes Thumshirn wrote:
> Allow overriding the announced NVMe Version of a via configfs.
>
> This is particularly helpful when debugging new features for the host
> or target side without bumping the hard coded version (as the target
> might not be fully compliant to the announced version yet).

It is. Just bump to 1.3, please as Identify 3h is the only mandatory
change: http://nvmexpress.org/changes/

[Hannes Reinecke]

Actually, I still do like this.
With it it'll be very easy to test version compliance; and validate our
initiator behaviour against older versions.

Cheers,

Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
ha...@suse.de +49 911 74053 688

SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg

GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[Johannes Thumshirn]

Add the UUID field from the NVMe Namespace Identification Descriptor
to the nvmet_ns structure and allow it's population via configfs.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/target/configfs.c | 31 +++++++++++++++++++++++++++++++
drivers/nvme/target/nvmet.h | 1 +

2 files changed, 32 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index be8c800078e2..16f9f6e3a084 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c

@@ -305,11 +305,41 @@ static ssize_t nvmet_ns_device_path_store(struct config_item *item,

CONFIGFS_ATTR(nvmet_ns_, device_path);

+static ssize_t nvmet_ns_device_uuid_show(struct config_item *item, char *page)
+{
+ return sprintf(page, "%pUb\n", &to_nvmet_ns(item)->uuid);
+}
+

+static ssize_t nvmet_ns_device_uuid_store(struct config_item *item,

+ const char *page, size_t count)
+{

const char *page, size_t count)

[Johannes Thumshirn]

This patchset implemets NVMe Namespace Descriptor Identification as of
NVMe 1.3. The Namespace Descriptor Identification allows a NVMe host
to query several Namespace Identification mechanisms, such as EUI-64,
NGUID and UUID from the target. If more than one value is set by the
target, it can transmit all set values to the host.

The Namespace Identification Descriptor list is the only way a target
can identify itself via the newly introduced UUID to the host (instead
of the EUI-64 or NGUID).

Both the Host and Target side are implemented. In order to get the
Linux Host to send the Linux target implementation a Namespace
Descriptor Identification command, you have to change the target's
announced version code to at least 1.3.

Unfortunately the host side already did have a sysfs attribute called
'uuid' which represented the NGUID, so precautions have been taken to
not break any existing userspace.

While I was already touching the relevant code paths, I decided to
also include the EUI-64 in the 'Identify Namespace' command response.

The code is tested using the nvme-loop loopback target and cut against
the nvme tree's nvme-4.12 branch.

Patches for nvmetcli and nvme-cli will follow shortly.

Changes to v1:
* Added Reviewed-by tags from Christoph and Hannes for unchanged patches
* Added patch introducing new structs at the beginning (Christoph)
* Dropped SZ_4K patch
* Got rid of dynamic memory allocation on the target side (Christoph)
* Reworked host side parser (Christoph)
* Check length inside type check in the host parser (Max)

Changes to v0:
* Fixed wrong size of 4069 and replaced it with SZ_4K (Max)
* Add constants for UUID, NGUID and EUI-64 length (Max)
* Drop EUI-64 Support on target side (Christoph)
* Use uuid_be instead of u8[] (Christoph)
* Add ability to override target's version (Hannes)
* Change hard coded magic 4096 and 0x1000 to SZ_4k in drivers/nvme/*

Johannes Thumshirn (8):
nvme: introduce NVMe Namespace Identification Descriptor structures

nvme: rename uuid to nguid in nvme_ns

nvmet: implement namespace identify descriptor list
nvmet: add uuid field to nvme_ns and populate via configfs
nvme: get list of namespace descriptors
nvme: provide UUID value to userspace

nvmet: allow overriding the NVMe VS via configfs

nvmet: use NVME_IDENTIFY_DATA_SIZE

drivers/nvme/host/core.c | 118 ++++++++++++++++++++++++++++++++++++++--
drivers/nvme/host/nvme.h | 1 +
drivers/nvme/target/admin-cmd.c | 57 ++++++++++++++++++-
drivers/nvme/target/configfs.c | 65 ++++++++++++++++++++++
drivers/nvme/target/core.c | 2 +-

drivers/nvme/target/discovery.c | 2 +-
drivers/nvme/target/nvmet.h | 1 +

include/linux/nvme.h | 23 ++++++++
8 files changed, 261 insertions(+), 8 deletions(-)

--
2.12.0

[Johannes Thumshirn]

Now that we have a way for getting the UUID from a target, provide it
to userspace as well.

Unfortunately there is already a sysfs attribute called UUID which is
a misnomer as it holds the NGUID value. So instead of creating yet
another wrong name, create a new 'nguid' sysfs attribute for the
NGUID. For the UUID attribute add a check wheter the namespace has a
UUID assigned to it and return this or return the NGUID to maintain
backwards compatibility. This should give userspace a chance to catch
up.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>

Reviewed-by: Christoph Hellwig <h...@lst.de>
Reviewed-by: Hannes Reinecke <ha...@suse.com>

---
drivers/nvme/host/core.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c

index 37047841da0e..3aa5b12680e5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1887,11 +1887,28 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,

}
static DEVICE_ATTR(wwid, S_IRUGO, wwid_show, NULL);

+static ssize_t nguid_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
+ return sprintf(buf, "%pU\n", ns->nguid);
+}
+static DEVICE_ATTR(nguid, S_IRUGO, nguid_show, NULL);
+
static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
- return sprintf(buf, "%pU\n", ns->nguid);
+
+ /* For backward compatibility expose the NGUID to userspace if
+ * we have no UUID set
+ */
+ if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid))) {
+ printk_ratelimited(KERN_WARNING
+ "No UUID available providing old NGUID\n");
+ return sprintf(buf, "%pU\n", ns->nguid);
+ }
+ return sprintf(buf, "%pU\n", ns->uuid);
}
static DEVICE_ATTR(uuid, S_IRUGO, uuid_show, NULL);

@@ -1914,6 +1931,7 @@ static DEVICE_ATTR(nsid, S_IRUGO, nsid_show, NULL);

static struct attribute *nvme_ns_attrs[] = {
&dev_attr_wwid.attr,
&dev_attr_uuid.attr,
+ &dev_attr_nguid.attr,
&dev_attr_eui.attr,
&dev_attr_nsid.attr,
NULL,

@@ -1926,6 +1944,11 @@ static umode_t nvme_ns_attrs_are_visible(struct kobject *kobj,

struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

if (a == &dev_attr_uuid.attr) {
+ if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid)) ||
+ !memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))

+ return 0;
+ }

[Johannes Thumshirn]

A NVMe Identify NS command with a CNS value of '3' is expecting a list
of Namespace Identification Descriptor structures to be returned to
the host for the namespace requested in the namespace identify
command.

This Namespace Identification Descriptor structure consists of the
type of the namespace identifier, the length of the identifier and the
actual identifier.

Valid types are NGUID and UUID which we have saved in our nvme_ns

structure if they have been configured via configfs. If no value has
been assigened to one of these we return an "invalid opcode" back to
the host to maintain backward compatibiliy with older implementations
without Namespace Identify Descriptor list support.

Also as the Namespace Identify Descriptor list is the only mandatory
feature change between 1.2.1 and 1.3.0 we can bump the advertised
version as well.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/target/admin-cmd.c | 53 +++++++++++++++++++++++++++++++++++++++++
drivers/nvme/target/core.c | 2 +-
2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index ff1f97006322..833b5ef935c3 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -367,6 +367,56 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req)

nvmet_req_complete(req, status);
}

+static void nvmet_execute_identify_desclist(struct nvmet_req *req)

+{
+ struct nvmet_ns *ns;
+ struct nvme_ns_identifier_hdr hdr;

+ u16 status = 0;

+ off_t off = 0;

+
+ ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid);
+ if (!ns) {
+ status = NVME_SC_INVALID_NS | NVME_SC_DNR;
+ goto out;
+ }
+

+ if (memchr_inv(&ns->uuid, 0, sizeof(ns->uuid))) {
+ memset(&hdr, 0, sizeof(hdr));
+ hdr.nidt = NVME_NIDT_UUID;
+ hdr.nidl = NVME_NIDT_UUID_LEN;
+ status = nvmet_copy_to_sgl(req, off, &hdr, sizeof(hdr));
+ if (status)
+ goto out_put_ns;
+ off += sizeof(hdr);
+
+ status = nvmet_copy_to_sgl(req, off, &ns->uuid,
+ sizeof(ns->uuid));
+ if (status)
+ goto out_put_ns;
+ off += sizeof(ns->uuid);
+ }
+ if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid))) {
+ memset(&hdr, 0, sizeof(hdr));
+ hdr.nidt = NVME_NIDT_NGUID;
+ hdr.nidl = NVME_NIDT_NGUID_LEN;
+ status = nvmet_copy_to_sgl(req, off, &hdr, sizeof(hdr));
+ if (status)
+ goto out_put_ns;
+ off += sizeof(hdr);
+
+ status = nvmet_copy_to_sgl(req, off, &ns->nguid,
+ sizeof(ns->nguid));
+ if (status)
+ goto out_put_ns;
+ off += sizeof(ns->nguid);
+ }

+
+out_put_ns:
+ nvmet_put_namespace(ns);
+out:
+ nvmet_req_complete(req, status);
+}
+
/*
* A "mimimum viable" abort implementation: the command is mandatory in the
* spec, but we are not required to do any useful work. We couldn't really

@@ -515,6 +565,9 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)

case NVME_ID_CNS_NS_ACTIVE_LIST:
req->execute = nvmet_execute_identify_nslist;
return 0;
+ case NVME_ID_CNS_NS_DESC_LIST:
+ req->execute = nvmet_execute_identify_desclist;
+ return 0;
}
break;
case nvme_admin_abort_cmd:

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index eb9399ac97cf..d4eeee4f2fab 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -926,7 +926,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
if (!subsys)
return NULL;

- subsys->ver = NVME_VS(1, 2, 1); /* NVMe 1.2.1 */
+ subsys->ver = NVME_VS(1, 3, 0); /* NVMe 1.3.0 */

switch (type) {
case NVME_NQN_NVME:
--
2.12.0

[Johannes Thumshirn]

Use NVME_IDENTIFY_DATA_SIZE define instead of hard coding the magic
4096 value.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/target/admin-cmd.c | 4 ++--
drivers/nvme/target/discovery.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 833b5ef935c3..def6cde1e714 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -336,7 +336,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)

static void nvmet_execute_identify_nslist(struct nvmet_req *req)

{
- static const int buf_size = 4096;
+ static const int buf_size = NVME_IDENTIFY_DATA_SIZE;
struct nvmet_ctrl *ctrl = req->sq->ctrl;
struct nvmet_ns *ns;
u32 min_nsid = le32_to_cpu(req->cmd->identify.nsid);
@@ -554,7 +554,7 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
}
break;
case nvme_admin_identify:
- req->data_len = 4096;
+ req->data_len = NVME_IDENTIFY_DATA_SIZE;
switch (cmd->identify.cns) {
case NVME_ID_CNS_NS:
req->execute = nvmet_execute_identify_ns;
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index 1aaf597e81fc..c7a90384dd75 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -185,7 +185,7 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
}
case nvme_admin_identify:
- req->data_len = 4096;
+ req->data_len = NVME_IDENTIFY_DATA_SIZE;
switch (cmd->identify.cns) {
case NVME_ID_CNS_CTRL:
req->execute =
--
2.12.0

[Johannes Thumshirn]

The uuid field in the nvme_ns structure represents the nguid field
from the identify namespace command. And as NVMe 1.3 introduced an
UUID in the NVMe Namespace Identification Descriptor this will
collide.

So rename the uuid to nguid to prevent any further
confusion. Unfortunately we export the nguid to sysfs in the uuid
sysfs attribute, but this can't be changed anymore without possibly
breaking existing userspace.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
Reviewed-by: Christoph Hellwig <h...@lst.de>
Reviewed-by: Hannes Reinecke <ha...@suse.com>
---

drivers/nvme/host/core.c | 10 +++++-----
drivers/nvme/host/nvme.h | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a60926410438..7b254be16887 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c

@@ -1016,7 +1016,7 @@ static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id)
if (ns->ctrl->vs >= NVME_VS(1, 1, 0))
memcpy(ns->eui, (*id)->eui64, sizeof(ns->eui));
if (ns->ctrl->vs >= NVME_VS(1, 2, 0))
- memcpy(ns->uuid, (*id)->nguid, sizeof(ns->uuid));
+ memcpy(ns->nguid, (*id)->nguid, sizeof(ns->nguid));

return 0;
}
@@ -1784,8 +1784,8 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
int serial_len = sizeof(ctrl->serial);
int model_len = sizeof(ctrl->model);

- if (memchr_inv(ns->uuid, 0, sizeof(ns->uuid)))
- return sprintf(buf, "eui.%16phN\n", ns->uuid);

+ if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))

+ return sprintf(buf, "eui.%16phN\n", ns->nguid);

if (memchr_inv(ns->eui, 0, sizeof(ns->eui)))
return sprintf(buf, "eui.%8phN\n", ns->eui);

@@ -1804,7 +1804,7 @@ static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,

char *buf)
{
struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

- return sprintf(buf, "%pU\n", ns->uuid);

+ return sprintf(buf, "%pU\n", ns->nguid);
}

static DEVICE_ATTR(uuid, S_IRUGO, uuid_show, NULL);

@@ -1839,7 +1839,7 @@ static umode_t nvme_ns_attrs_are_visible(struct kobject *kobj,

struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

if (a == &dev_attr_uuid.attr) {

- if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid)))
+ if (!memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
return 0;
}

[Johannes Thumshirn]

Allow overriding the announced NVMe Version of a via configfs.

This is particularly helpful when debugging new features for the host
or target side without bumping the hard coded version (as the target
might not be fully compliant to the announced version yet).

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/target/configfs.c | 34 ++++++++++++++++++++++++++++++++++
include/linux/nvme.h | 4 ++++

2 files changed, 38 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 16f9f6e3a084..45421d4308a4 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c

@@ -650,8 +650,42 @@ static ssize_t nvmet_subsys_attr_allow_any_host_store(struct config_item *item,

CONFIGFS_ATTR(nvmet_subsys_, attr_allow_any_host);

+static ssize_t nvmet_subsys_version_show(struct config_item *item,
+ char *page)
+{
+ struct nvmet_subsys *subsys = to_subsys(item);
+ int major, minor, tertiary;
+ u32 ver;
+
+ ver = subsys->ver;
+ major = NVME_MAJOR(ver);
+ minor = NVME_MINOR(ver);
+ tertiary = NVME_TERRIARY(ver);
+
+ return snprintf(page, PAGE_SIZE, "%d %d %d\n", major, minor, tertiary);
+}
+

+static ssize_t nvmet_subsys_version_store(struct config_item *item,

+ const char *page, size_t count)
+{

+ struct nvmet_subsys *subsys = to_subsys(item);
+ int major, minor, tertiary;

+ int ret;
+
+

+ ret = sscanf(page, "%d %d %d\n", &major, &minor, &tertiary);
+ if (ret != 3)
+ return -EINVAL;
+
+ subsys->ver = NVME_VS(major, minor, tertiary);
+
+ return count;
+}
+CONFIGFS_ATTR(nvmet_subsys_, version);
+
static struct configfs_attribute *nvmet_subsys_attrs[] = {
&nvmet_subsys_attr_attr_allow_any_host,
+ &nvmet_subsys_attr_version,
NULL,
};

diff --git a/include/linux/nvme.h b/include/linux/nvme.h

index afa6ef484e50..0d6e307a7aa4 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1069,4 +1069,8 @@ struct nvme_completion {

[Johannes Thumshirn]

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

include/linux/nvme.h | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index b625bacf37ef..afa6ef484e50 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h

@@ -288,6 +288,7 @@ enum {
NVME_ID_CNS_NS = 0x00,
NVME_ID_CNS_CTRL = 0x01,
NVME_ID_CNS_NS_ACTIVE_LIST = 0x02,
+ NVME_ID_CNS_NS_DESC_LIST = 0x03,
NVME_ID_CNS_NS_PRESENT_LIST = 0x10,
NVME_ID_CNS_NS_PRESENT = 0x11,
NVME_ID_CNS_CTRL_NS_LIST = 0x12,

@@ -314,6 +315,22 @@ enum {
NVME_NS_DPS_PI_TYPE3 = 3,
};

+struct nvme_ns_identifier_hdr {

+ __u8 nidt;
+ __u8 nidl;
+ __le16 reserved;

+};
+
+#define NVME_NIDT_EUI64_LEN 8
+#define NVME_NIDT_NGUID_LEN 16
+#define NVME_NIDT_UUID_LEN 16
+
+enum {
+ NVME_NIDT_EUI64 = 0x01,
+ NVME_NIDT_NGUID = 0x02,
+ NVME_NIDT_UUID = 0x03,
+};
+
struct nvme_smart_log {
__u8 critical_warning;
__u8 temperature[2];

@@ -658,6 +675,8 @@ struct nvme_identify {
__u32 rsvd11[5];
};

+#define NVME_IDENTIFY_DATA_SIZE 4096
+
struct nvme_features {
__u8 opcode;
__u8 flags;
--
2.12.0

[Johannes Thumshirn]

If a target identifies itself as NVMe 1.3 compliant, try to get the
list of Namespace Identification Descriptors and populate the UUID,
NGUID and EUI64 fileds in the NVMe namespace structure with these
values.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/host/core.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++
drivers/nvme/host/nvme.h | 1 +
2 files changed, 88 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 7b254be16887..37047841da0e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -643,6 +643,85 @@ int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
return error;
}

+static void nvme_parse_ns_descs(struct nvme_ns *ns, void *data)
+{
+ int pos;
+ int len;
+
+ for (pos = 0; pos < NVME_IDENTIFY_DATA_SIZE; pos += len) {
+ struct nvme_ns_identifier_hdr *cur = data + pos;
+
+ if (cur->nidl == 0)
+ break;

+
+ switch (cur->nidt) {
+ case NVME_NIDT_EUI64:

+ if (cur->nidl != NVME_NIDT_EUI64_LEN) {
+ dev_warn(ns->ctrl->dev,
+ "Target returned bogus length: %d for NVME_NIDT_EUI64\n",

+ cur->nidl);
+ return;
+ }

+ len = NVME_NIDT_EUI64_LEN;
+ memcpy(ns->eui, data + pos + sizeof(*cur), len);

+ break;
+ case NVME_NIDT_NGUID:

+ if (cur->nidl != NVME_NIDT_NGUID_LEN) {
+ dev_warn(ns->ctrl->dev,
+ "Target returned bogus length: %d for NVME_NIDT_NGUID\n",

+ cur->nidl);
+ return;
+ }

+ len = NVME_NIDT_NGUID_LEN;
+ memcpy(ns->nguid, data + pos + sizeof(*cur), len);

+ break;
+ case NVME_NIDT_UUID:

+ if (cur->nidl != NVME_NIDT_UUID_LEN) {
+ dev_warn(ns->ctrl->dev,
+ "Target returned bogus length: %d for NVME_NIDT_UUID\n",

+ cur->nidl);
+ return;
+ }

+ len = NVME_NIDT_UUID_LEN;
+ memcpy(ns->uuid, data + pos + sizeof(*cur), len);

+ break;
+ default:
+ dev_warn(ns->ctrl->dev,

+ "Invalid Namespace Identification Descriptor Type: %d\n",
+ cur->nidt);
+ return;
+ }
+

+ len += sizeof(*cur);
+ }
+}
+
+static int nvme_identify_ns_descs(struct nvme_ns *ns, unsigned nsid)

+{
+ struct nvme_command c = { };

+ int status;
+ void *data;

+
+ c.identify.opcode = nvme_admin_identify;
+ c.identify.nsid = cpu_to_le32(nsid);
+ c.identify.cns = NVME_ID_CNS_NS_DESC_LIST;
+

+ data = kzalloc(NVME_IDENTIFY_DATA_SIZE, GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ status = nvme_submit_sync_cmd(ns->ctrl->admin_q, &c, data,
+ NVME_IDENTIFY_DATA_SIZE);
+ if (status)
+ goto free_data;
+
+ nvme_parse_ns_descs(ns, data);
+
+free_data:
+ kfree(data);
+ return status;

+}
+
static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *ns_list)
{
struct nvme_command c = { };

@@ -1017,6 +1096,14 @@ static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id)

memcpy(ns->eui, (*id)->eui64, sizeof(ns->eui));
if (ns->ctrl->vs >= NVME_VS(1, 2, 0))

memcpy(ns->nguid, (*id)->nguid, sizeof(ns->nguid));

+ if (ns->ctrl->vs >= NVME_VS(1, 3, 0)) {

+ /* Don't treat error as fatal we potentially
+ * already have a NGUID or EUI-64

+ */
+ if (nvme_identify_ns_descs(ns, ns->ns_id))

+ dev_warn(ns->ctrl->dev,
+ "%s: Identify Descriptors failed\n", __func__);
+ }

return 0;
}

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 5004f0c41397..7007521e8194 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h

@@ -190,6 +190,7 @@ struct nvme_ns {

u8 eui[8];
u8 nguid[16];

+ u8 uuid[16];

[Keith Busch]

On Thu, Jun 01, 2017 at 01:17:48PM +0200, Johannes Thumshirn wrote:
> Now that we have a way for getting the UUID from a target, provide it
> to userspace as well.
>
> Unfortunately there is already a sysfs attribute called UUID which is
> a misnomer as it holds the NGUID value. So instead of creating yet
> another wrong name, create a new 'nguid' sysfs attribute for the
> NGUID. For the UUID attribute add a check wheter the namespace has a
> UUID assigned to it and return this or return the NGUID to maintain
> backwards compatibility. This should give userspace a chance to catch
> up.

Sorry for the naming clash. Not sure why I didn't use the obvious name
for this file in the first place.

FWIW, tools should have been using the 'wwid' attribute, which returns
either EUI64 or NGUID so a unique identifier can be gotten from a
single file without checking for the existence of either. That should
help not break backward compatibility, but I've no idea if anything
actually relies on 'uuid' returning the NGUID.

[Johannes Thumshirn]

On 06/01/2017 02:47 PM, Keith Busch wrote:
> FWIW, tools should have been using the 'wwid' attribute, which returns
> either EUI64 or NGUID so a unique identifier can be gotten from a
> single file without checking for the existence of either. That should
> help not break backward compatibility, but I've no idea if anything
> actually relies on 'uuid' returning the NGUID.

Yep, that's why I though I'd be clever and place this hack in there. I
think we can remove it in 1-2 years or so when most/all userspace has
adopted to the change (there shouldn't be too much apart from some hand
crafted udev rules anyways).

--
Johannes Thumshirn Storage
jthum...@suse.de +49 911 74053 689

SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg

GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[Johannes Thumshirn]

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
Reviewed-by: Max Gurtovoy <ma...@mellanox.com>

[Johannes Thumshirn]

Allow overriding the announced NVMe Version of a via configfs.

This is particularly helpful when debugging new features for the host
or target side without bumping the hard coded version (as the target
might not be fully compliant to the announced version yet).

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index afa6ef484e50..0d6e307a7aa4 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h

[Johannes Thumshirn]

A NVMe Identify NS command with a CNS value of '3' is expecting a list
of Namespace Identification Descriptor structures to be returned to
the host for the namespace requested in the namespace identify
command.

This Namespace Identification Descriptor structure consists of the
type of the namespace identifier, the length of the identifier and the
actual identifier.

Valid types are NGUID and UUID which we have saved in our nvme_ns
structure if they have been configured via configfs. If no value has
been assigened to one of these we return an "invalid opcode" back to
the host to maintain backward compatibiliy with older implementations
without Namespace Identify Descriptor list support.

Also as the Namespace Identify Descriptor list is the only mandatory
feature change between 1.2.1 and 1.3.0 we can bump the advertised
version as well.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/target/admin-cmd.c | 53 +++++++++++++++++++++++++++++++++++++++++
drivers/nvme/target/core.c | 2 +-

drivers/nvme/target/nvmet.h | 1 +
3 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index ff1f97006322..833b5ef935c3 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c

@@ -367,6 +367,56 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req)
nvmet_req_complete(req, status);
}

+static void nvmet_execute_identify_desclist(struct nvmet_req *req)
+{
+ struct nvmet_ns *ns;
+ struct nvme_ns_identifier_hdr hdr;
+ u16 status = 0;
+ off_t off = 0;
+
+ ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid);
+ if (!ns) {
+ status = NVME_SC_INVALID_NS | NVME_SC_DNR;
+ goto out;
+ }
+
+ if (memchr_inv(&ns->uuid, 0, sizeof(ns->uuid))) {
+ memset(&hdr, 0, sizeof(hdr));
+ hdr.nidt = NVME_NIDT_UUID;
+ hdr.nidl = NVME_NIDT_UUID_LEN;
+ status = nvmet_copy_to_sgl(req, off, &hdr, sizeof(hdr));

+ if (status)

+ goto out_put_ns;
+ off += sizeof(hdr);
+
+ status = nvmet_copy_to_sgl(req, off, &ns->uuid,
+ sizeof(ns->uuid));

+ if (status)

+ goto out_put_ns;
+ off += sizeof(ns->uuid);
+ }

+ if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid))) {

+ memset(&hdr, 0, sizeof(hdr));
+ hdr.nidt = NVME_NIDT_NGUID;
+ hdr.nidl = NVME_NIDT_NGUID_LEN;
+ status = nvmet_copy_to_sgl(req, off, &hdr, sizeof(hdr));

+ if (status)

+ goto out_put_ns;
+ off += sizeof(hdr);
+
+ status = nvmet_copy_to_sgl(req, off, &ns->nguid,
+ sizeof(ns->nguid));

+ if (status)

[Johannes Thumshirn]

Now that we have a way for getting the UUID from a target, provide it
to userspace as well.

Unfortunately there is already a sysfs attribute called UUID which is
a misnomer as it holds the NGUID value. So instead of creating yet
another wrong name, create a new 'nguid' sysfs attribute for the
NGUID. For the UUID attribute add a check wheter the namespace has a
UUID assigned to it and return this or return the NGUID to maintain
backwards compatibility. This should give userspace a chance to catch
up.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>

Reviewed-by: Christoph Hellwig <h...@lst.de>
Reviewed-by: Hannes Reinecke <ha...@suse.com>
---

drivers/nvme/host/core.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 37047841da0e..3aa5b12680e5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c

@@ -1887,11 +1887,28 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
}
static DEVICE_ATTR(wwid, S_IRUGO, wwid_show, NULL);

+static ssize_t nguid_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{

+ struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

+ return sprintf(buf, "%pU\n", ns->nguid);

+}
+static DEVICE_ATTR(nguid, S_IRUGO, nguid_show, NULL);
+

static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

- return sprintf(buf, "%pU\n", ns->nguid);
+
+ /* For backward compatibility expose the NGUID to userspace if
+ * we have no UUID set
+ */
+ if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid))) {
+ printk_ratelimited(KERN_WARNING
+ "No UUID available providing old NGUID\n");

+ return sprintf(buf, "%pU\n", ns->nguid);

+ }
+ return sprintf(buf, "%pU\n", ns->uuid);

}
static DEVICE_ATTR(uuid, S_IRUGO, uuid_show, NULL);

@@ -1914,6 +1931,7 @@ static DEVICE_ATTR(nsid, S_IRUGO, nsid_show, NULL);
static struct attribute *nvme_ns_attrs[] = {
&dev_attr_wwid.attr,
&dev_attr_uuid.attr,
+ &dev_attr_nguid.attr,
&dev_attr_eui.attr,
&dev_attr_nsid.attr,
NULL,

@@ -1926,6 +1944,11 @@ static umode_t nvme_ns_attrs_are_visible(struct kobject *kobj,

struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

if (a == &dev_attr_uuid.attr) {

+ if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid)) ||
+ !memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
+ return 0;
+ }
+ if (a == &dev_attr_nguid.attr) {

if (!memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
return 0;
}

--
2.12.0

[Johannes Thumshirn]

Add the UUID field from the NVMe Namespace Identification Descriptor
to the nvmet_ns structure and allow it's population via configfs.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/target/configfs.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index be8c800078e2..16f9f6e3a084 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c

@@ -305,11 +305,41 @@ static ssize_t nvmet_ns_device_path_store(struct config_item *item,

CONFIGFS_ATTR(nvmet_ns_, device_path);

+static ssize_t nvmet_ns_device_uuid_show(struct config_item *item, char *page)
+{
+ return sprintf(page, "%pUb\n", &to_nvmet_ns(item)->uuid);
+}
+

+static ssize_t nvmet_ns_device_uuid_store(struct config_item *item,

+ const char *page, size_t count)

+{

const char *page, size_t count)

{
@@ -379,6 +409,7 @@ CONFIGFS_ATTR(nvmet_ns_, enable);
static struct configfs_attribute *nvmet_ns_attrs[] = {
&nvmet_ns_attr_device_path,
&nvmet_ns_attr_device_nguid,
+ &nvmet_ns_attr_device_uuid,
&nvmet_ns_attr_enable,
NULL,
};

--
2.12.0

[Johannes Thumshirn]

The uuid field in the nvme_ns structure represents the nguid field
from the identify namespace command. And as NVMe 1.3 introduced an
UUID in the NVMe Namespace Identification Descriptor this will
collide.

So rename the uuid to nguid to prevent any further
confusion. Unfortunately we export the nguid to sysfs in the uuid
sysfs attribute, but this can't be changed anymore without possibly
breaking existing userspace.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
Reviewed-by: Christoph Hellwig <h...@lst.de>
Reviewed-by: Hannes Reinecke <ha...@suse.com>
---

drivers/nvme/host/core.c | 10 +++++-----
drivers/nvme/host/nvme.h | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a60926410438..7b254be16887 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1016,7 +1016,7 @@ static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id)
if (ns->ctrl->vs >= NVME_VS(1, 1, 0))

memcpy(ns->eui, (*id)->eui64, sizeof(ns->eui));
if (ns->ctrl->vs >= NVME_VS(1, 2, 0))

- memcpy(ns->uuid, (*id)->nguid, sizeof(ns->uuid));
+ memcpy(ns->nguid, (*id)->nguid, sizeof(ns->nguid));

return 0;
}
@@ -1784,8 +1784,8 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
int serial_len = sizeof(ctrl->serial);
int model_len = sizeof(ctrl->model);

- if (memchr_inv(ns->uuid, 0, sizeof(ns->uuid)))
- return sprintf(buf, "eui.%16phN\n", ns->uuid);
+ if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
+ return sprintf(buf, "eui.%16phN\n", ns->nguid);

if (memchr_inv(ns->eui, 0, sizeof(ns->eui)))
return sprintf(buf, "eui.%8phN\n", ns->eui);

@@ -1804,7 +1804,7 @@ static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,

char *buf)
{
struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

- return sprintf(buf, "%pU\n", ns->uuid);

+ return sprintf(buf, "%pU\n", ns->nguid);
}

static DEVICE_ATTR(uuid, S_IRUGO, uuid_show, NULL);

@@ -1839,7 +1839,7 @@ static umode_t nvme_ns_attrs_are_visible(struct kobject *kobj,

struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

if (a == &dev_attr_uuid.attr) {

- if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid)))
+ if (!memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
return 0;
}
if (a == &dev_attr_eui.attr) {
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 9d6a070d4391..5004f0c41397 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h

@@ -189,7 +189,7 @@ struct nvme_ns {
int instance;

u8 eui[8];
- u8 uuid[16];

+ u8 nguid[16];

[Johannes Thumshirn]

If a target identifies itself as NVMe 1.3 compliant, try to get the
list of Namespace Identification Descriptors and populate the UUID,
NGUID and EUI64 fileds in the NVMe namespace structure with these
values.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/host/core.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++
drivers/nvme/host/nvme.h | 1 +
2 files changed, 88 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 7b254be16887..37047841da0e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c

+ if (status)

+ goto free_data;
+
+ nvme_parse_ns_descs(ns, data);
+
+free_data:
+ kfree(data);
+ return status;
+}
+
static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *ns_list)
{
struct nvme_command c = { };

@@ -1017,6 +1096,14 @@ static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id)

memcpy(ns->eui, (*id)->eui64, sizeof(ns->eui));
if (ns->ctrl->vs >= NVME_VS(1, 2, 0))

memcpy(ns->nguid, (*id)->nguid, sizeof(ns->nguid));

+ if (ns->ctrl->vs >= NVME_VS(1, 3, 0)) {
+ /* Don't treat error as fatal we potentially
+ * already have a NGUID or EUI-64
+ */
+ if (nvme_identify_ns_descs(ns, ns->ns_id))
+ dev_warn(ns->ctrl->dev,
+ "%s: Identify Descriptors failed\n", __func__);
+ }

return 0;
}

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 5004f0c41397..7007521e8194 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h

@@ -190,6 +190,7 @@ struct nvme_ns {

u8 eui[8];
u8 nguid[16];

+ u8 uuid[16];

[Johannes Thumshirn]

This patchset implemets NVMe Namespace Descriptor Identification as of
NVMe 1.3. The Namespace Descriptor Identification allows a NVMe host
to query several Namespace Identification mechanisms, such as EUI-64,
NGUID and UUID from the target. If more than one value is set by the
target, it can transmit all set values to the host.

The Namespace Identification Descriptor list is the only way a target
can identify itself via the newly introduced UUID to the host (instead
of the EUI-64 or NGUID).

Both the Host and Target side are implemented. In order to get the
Linux Host to send the Linux target implementation a Namespace
Descriptor Identification command, you have to change the target's
announced version code to at least 1.3.

Unfortunately the host side already did have a sysfs attribute called
'uuid' which represented the NGUID, so precautions have been taken to
not break any existing userspace.

While I was already touching the relevant code paths, I decided to
also include the EUI-64 in the 'Identify Namespace' command response.

The code is tested using the nvme-loop loopback target and cut against
the nvme tree's nvme-4.12 branch.

A patch for nvmetcli will follow shortly.

Changes to v2:
* Added Max's Reviewed-by
* Make series bisectable

drivers/nvme/target/discovery.c | 2 +-
drivers/nvme/target/nvmet.h | 1 +

[Johannes Thumshirn]

Use NVME_IDENTIFY_DATA_SIZE define instead of hard coding the magic
4096 value.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/target/admin-cmd.c | 4 ++--
drivers/nvme/target/discovery.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 833b5ef935c3..def6cde1e714 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c

@@ -336,7 +336,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)

static void nvmet_execute_identify_nslist(struct nvmet_req *req)

[Sagi Grimberg]

> switch (type) {
> case NVME_NQN_NVME:
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index cfc5c7fb0ab7..4c6cb5ea1186 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -46,6 +46,7 @@ struct nvmet_ns {
> u32 blksize_shift;
> loff_t size;
> u8 nguid[16];
> + uuid_be uuid;

This should be uuid_le to match the rest of NVMe.

Also, it would be useful to generate a random default with uuid_le_gen()
for a better out-of-the-box experience.

[Johannes Thumshirn]

On 06/02/2017 03:46 PM, Sagi Grimberg wrote:
>> + uuid_be uuid;
>
> This should be uuid_le to match the rest of NVMe.

Ahm, are you sure?
$ PAGER= git grep -E uuid_\(l\|b\)e drivers/nvme
drivers/nvme/host/fabrics.c:61: uuid_be_gen(&host->id);
drivers/nvme/host/fabrics.c:78: uuid_be_gen(&host->id);
drivers/nvme/host/fabrics.c:398: memcpy(&data->hostid,
&ctrl->opts->host->id, sizeof(uuid_be));
drivers/nvme/host/fabrics.c:457: memcpy(&data->hostid,
&ctrl->opts->host->id, sizeof(uuid_be));
drivers/nvme/host/fabrics.h:39: uuid_be id;
drivers/nvme/host/fc.c:882: min_t(size_t, FCNVME_ASSOC_HOSTID_LEN,
sizeof(uuid_be)));
drivers/nvme/target/admin-cmd.c:324: memcpy(&id->nguid, &ns->nguid,
sizeof(uuid_le));
drivers/nvme/target/configfs.c:328: if (uuid_be_to_bin(page, &ns->uuid))
drivers/nvme/target/nvmet.h:49: uuid_be uuid;

And RFC4122 4.1.2 says MSB first as well.

>
> Also, it would be useful to generate a random default with uuid_le_gen()
> for a better out-of-the-box experience.

Yep, sounds reasonable.

[Christoph Hellwig]

On Fri, Jun 02, 2017 at 04:46:40PM +0300, Sagi Grimberg wrote:
>
>> switch (type) {
>> case NVME_NQN_NVME:
>> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
>> index cfc5c7fb0ab7..4c6cb5ea1186 100644
>> --- a/drivers/nvme/target/nvmet.h
>> +++ b/drivers/nvme/target/nvmet.h
>> @@ -46,6 +46,7 @@ struct nvmet_ns {
>> u32 blksize_shift;
>> loff_t size;
>> u8 nguid[16];
>> + uuid_be uuid;
>
> This should be uuid_le to match the rest of NVMe.

It shouldn't. A RFC4122 uuid is always in "big endian" notation.
A little endiane uuid doesn't exist, it's called a GUID in Wintel
speak and isn't used in NVMe. My new uuid tree tries to fix up
the confusion by renaming our uuid_be type to uuid_t and uuid_le
to guid_t.

> Also, it would be useful to generate a random default with uuid_le_gen()
> for a better out-of-the-box experience.

That's actually a good point. Just needs to be uuid_be_gen/uuid_gen :)

[Sagi Grimberg]

>>> switch (type) {
>>> case NVME_NQN_NVME:
>>> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
>>> index cfc5c7fb0ab7..4c6cb5ea1186 100644
>>> --- a/drivers/nvme/target/nvmet.h
>>> +++ b/drivers/nvme/target/nvmet.h
>>> @@ -46,6 +46,7 @@ struct nvmet_ns {
>>> u32 blksize_shift;
>>> loff_t size;
>>> u8 nguid[16];
>>> + uuid_be uuid;
>>
>> This should be uuid_le to match the rest of NVMe.
>
> It shouldn't. A RFC4122 uuid is always in "big endian" notation.
> A little endiane uuid doesn't exist, it's called a GUID in Wintel
> speak and isn't used in NVMe. My new uuid tree tries to fix up
> the confusion by renaming our uuid_be type to uuid_t and uuid_le
> to guid_t.

Thanks for clarifying.

At one point we had a uuid before we allowed user-space to pass a nguid
and it was little endian, but it was a hack anyway.

[Johannes Thumshirn]

The uuid field in the nvme_ns structure represents the nguid field
from the identify namespace command. And as NVMe 1.3 introduced an
UUID in the NVMe Namespace Identification Descriptor this will
collide.

So rename the uuid to nguid to prevent any further
confusion. Unfortunately we export the nguid to sysfs in the uuid
sysfs attribute, but this can't be changed anymore without possibly
breaking existing userspace.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
Reviewed-by: Christoph Hellwig <h...@lst.de>
Reviewed-by: Hannes Reinecke <ha...@suse.com>
---
drivers/nvme/host/core.c | 10 +++++-----

drivers/nvme/host/nvme.h | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a60926410438..7b254be16887 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1016,7 +1016,7 @@ static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id)
if (ns->ctrl->vs >= NVME_VS(1, 1, 0))

memcpy(ns->eui, (*id)->eui64, sizeof(ns->eui));
if (ns->ctrl->vs >= NVME_VS(1, 2, 0))

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 9d6a070d4391..5004f0c41397 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h

@@ -189,7 +189,7 @@ struct nvme_ns {
int instance;

u8 eui[8];
- u8 uuid[16];

+ u8 nguid[16];

[Johannes Thumshirn]

Allow overriding the announced NVMe Version of a via configfs.

This is particularly helpful when debugging new features for the host
or target side without bumping the hard coded version (as the target
might not be fully compliant to the announced version yet).

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/target/configfs.c | 34 ++++++++++++++++++++++++++++++++++
include/linux/nvme.h | 4 ++++

2 files changed, 38 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 16f9f6e3a084..45421d4308a4 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c

@@ -650,8 +650,42 @@ static ssize_t nvmet_subsys_attr_allow_any_host_store(struct config_item *item,

CONFIGFS_ATTR(nvmet_subsys_, attr_allow_any_host);

+static ssize_t nvmet_subsys_version_show(struct config_item *item,
+ char *page)
+{
+ struct nvmet_subsys *subsys = to_subsys(item);
+ int major, minor, tertiary;
+ u32 ver;
+
+ ver = subsys->ver;
+ major = NVME_MAJOR(ver);
+ minor = NVME_MINOR(ver);
+ tertiary = NVME_TERRIARY(ver);
+
+ return snprintf(page, PAGE_SIZE, "%d %d %d\n", major, minor, tertiary);
+}
+

+static ssize_t nvmet_subsys_version_store(struct config_item *item,

+ const char *page, size_t count)
+{

[Johannes Thumshirn]

A NVMe Identify NS command with a CNS value of '3' is expecting a list
of Namespace Identification Descriptor structures to be returned to
the host for the namespace requested in the namespace identify
command.

This Namespace Identification Descriptor structure consists of the
type of the namespace identifier, the length of the identifier and the
actual identifier.

Valid types are NGUID and UUID which we have saved in our nvme_ns
structure if they have been configured via configfs. If no value has
been assigened to one of these we return an "invalid opcode" back to
the host to maintain backward compatibiliy with older implementations
without Namespace Identify Descriptor list support.

Also as the Namespace Identify Descriptor list is the only mandatory
feature change between 1.2.1 and 1.3.0 we can bump the advertised
version as well.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/target/admin-cmd.c | 53 +++++++++++++++++++++++++++++++++++++++++
drivers/nvme/target/core.c | 3 ++-
drivers/nvme/target/nvmet.h | 1 +
3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index ff1f97006322..833b5ef935c3 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c

@@ -367,6 +367,56 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req)
nvmet_req_complete(req, status);
}

+static void nvmet_execute_identify_desclist(struct nvmet_req *req)

+{

+ struct nvmet_ns *ns;
+ struct nvme_ns_identifier_hdr hdr;
+ u16 status = 0;
+ off_t off = 0;
+
+ ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid);
+ if (!ns) {
+ status = NVME_SC_INVALID_NS | NVME_SC_DNR;
+ goto out;
+ }
+
+ if (memchr_inv(&ns->uuid, 0, sizeof(ns->uuid))) {
+ memset(&hdr, 0, sizeof(hdr));
+ hdr.nidt = NVME_NIDT_UUID;
+ hdr.nidl = NVME_NIDT_UUID_LEN;
+ status = nvmet_copy_to_sgl(req, off, &hdr, sizeof(hdr));

+ if (status)

+ goto out_put_ns;
+ off += sizeof(hdr);
+
+ status = nvmet_copy_to_sgl(req, off, &ns->uuid,
+ sizeof(ns->uuid));

+ if (status)

+ goto out_put_ns;
+ off += sizeof(ns->uuid);
+ }

+ if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid))) {

+ memset(&hdr, 0, sizeof(hdr));
+ hdr.nidt = NVME_NIDT_NGUID;
+ hdr.nidl = NVME_NIDT_NGUID_LEN;
+ status = nvmet_copy_to_sgl(req, off, &hdr, sizeof(hdr));

+ if (status)

+ goto out_put_ns;
+ off += sizeof(hdr);
+
+ status = nvmet_copy_to_sgl(req, off, &ns->nguid,
+ sizeof(ns->nguid));

+ if (status)

+ goto out_put_ns;
+ off += sizeof(ns->nguid);
+ }
+
+out_put_ns:
+ nvmet_put_namespace(ns);
+out:
+ nvmet_req_complete(req, status);
+}
+
/*
* A "mimimum viable" abort implementation: the command is mandatory in the
* spec, but we are not required to do any useful work. We couldn't really
@@ -515,6 +565,9 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
case NVME_ID_CNS_NS_ACTIVE_LIST:
req->execute = nvmet_execute_identify_nslist;
return 0;
+ case NVME_ID_CNS_NS_DESC_LIST:
+ req->execute = nvmet_execute_identify_desclist;
+ return 0;
}
break;
case nvme_admin_abort_cmd:
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c

index eb9399ac97cf..1a9c17c7dc59 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -380,6 +380,7 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)

ns->nsid = nsid;
ns->subsys = subsys;
+ uuid_be_gen(&ns->uuid);

return ns;
}
@@ -926,7 +927,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,

if (!subsys)
return NULL;

- subsys->ver = NVME_VS(1, 2, 1); /* NVMe 1.2.1 */
+ subsys->ver = NVME_VS(1, 3, 0); /* NVMe 1.3.0 */

switch (type) {
case NVME_NQN_NVME:
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index cfc5c7fb0ab7..4c6cb5ea1186 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -46,6 +46,7 @@ struct nvmet_ns {
u32 blksize_shift;
loff_t size;
u8 nguid[16];
+ uuid_be uuid;

[Johannes Thumshirn]

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>

Reviewed-by: Max Gurtovoy <ma...@mellanox.com>
---
include/linux/nvme.h | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index b625bacf37ef..afa6ef484e50 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h

[Johannes Thumshirn]

If a target identifies itself as NVMe 1.3 compliant, try to get the
list of Namespace Identification Descriptors and populate the UUID,
NGUID and EUI64 fileds in the NVMe namespace structure with these
values.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/host/core.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++
drivers/nvme/host/nvme.h | 1 +
2 files changed, 88 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 7b254be16887..37047841da0e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c

+ if (status)

+ goto free_data;
+
+ nvme_parse_ns_descs(ns, data);
+
+free_data:
+ kfree(data);
+ return status;
+}
+
static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *ns_list)
{
struct nvme_command c = { };

@@ -1017,6 +1096,14 @@ static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id)

memcpy(ns->eui, (*id)->eui64, sizeof(ns->eui));
if (ns->ctrl->vs >= NVME_VS(1, 2, 0))

memcpy(ns->nguid, (*id)->nguid, sizeof(ns->nguid));

+ if (ns->ctrl->vs >= NVME_VS(1, 3, 0)) {
+ /* Don't treat error as fatal we potentially
+ * already have a NGUID or EUI-64
+ */
+ if (nvme_identify_ns_descs(ns, ns->ns_id))
+ dev_warn(ns->ctrl->dev,
+ "%s: Identify Descriptors failed\n", __func__);
+ }

return 0;
}

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 5004f0c41397..7007521e8194 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h

@@ -190,6 +190,7 @@ struct nvme_ns {

u8 eui[8];
u8 nguid[16];

+ u8 uuid[16];

[Johannes Thumshirn]

This patchset implemets NVMe Namespace Descriptor Identification as of
NVMe 1.3. The Namespace Descriptor Identification allows a NVMe host
to query several Namespace Identification mechanisms, such as EUI-64,
NGUID and UUID from the target. If more than one value is set by the
target, it can transmit all set values to the host.

The Namespace Identification Descriptor list is the only way a target
can identify itself via the newly introduced UUID to the host (instead
of the EUI-64 or NGUID).

Both the Host and Target side are implemented. In order to get the
Linux Host to send the Linux target implementation a Namespace
Descriptor Identification command, you have to change the target's
announced version code to at least 1.3.

Unfortunately the host side already did have a sysfs attribute called
'uuid' which represented the NGUID, so precautions have been taken to
not break any existing userspace.

While I was already touching the relevant code paths, I decided to
also include the EUI-64 in the 'Identify Namespace' command response.

The code is tested using the nvme-loop loopback target and cut against
the nvme tree's nvme-4.12 branch.

A patch for nvmetcli will follow shortly.

Changes to v3:
* Autogenerate UUID on target NS allocation (Sagi)

drivers/nvme/target/core.c | 3 +-

drivers/nvme/target/discovery.c | 2 +-
drivers/nvme/target/nvmet.h | 1 +
include/linux/nvme.h | 23 ++++++++

8 files changed, 262 insertions(+), 8 deletions(-)

--
2.12.0

[Johannes Thumshirn]

Add the UUID field from the NVMe Namespace Identification Descriptor
to the nvmet_ns structure and allow it's population via configfs.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/target/configfs.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index be8c800078e2..16f9f6e3a084 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c

@@ -305,11 +305,41 @@ static ssize_t nvmet_ns_device_path_store(struct config_item *item,

CONFIGFS_ATTR(nvmet_ns_, device_path);

+static ssize_t nvmet_ns_device_uuid_show(struct config_item *item, char *page)
+{
+ return sprintf(page, "%pUb\n", &to_nvmet_ns(item)->uuid);
+}
+

+static ssize_t nvmet_ns_device_uuid_store(struct config_item *item,

+ const char *page, size_t count)

+{

const char *page, size_t count)

[Johannes Thumshirn]

Now that we have a way for getting the UUID from a target, provide it
to userspace as well.

Unfortunately there is already a sysfs attribute called UUID which is
a misnomer as it holds the NGUID value. So instead of creating yet
another wrong name, create a new 'nguid' sysfs attribute for the
NGUID. For the UUID attribute add a check wheter the namespace has a
UUID assigned to it and return this or return the NGUID to maintain
backwards compatibility. This should give userspace a chance to catch
up.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>

Reviewed-by: Christoph Hellwig <h...@lst.de>
Reviewed-by: Hannes Reinecke <ha...@suse.com>
---

drivers/nvme/host/core.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 37047841da0e..3aa5b12680e5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c

@@ -1887,11 +1887,28 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
}
static DEVICE_ATTR(wwid, S_IRUGO, wwid_show, NULL);

+static ssize_t nguid_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{

+ struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

+ return sprintf(buf, "%pU\n", ns->nguid);

+}
+static DEVICE_ATTR(nguid, S_IRUGO, nguid_show, NULL);
+

static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

- return sprintf(buf, "%pU\n", ns->nguid);
+
+ /* For backward compatibility expose the NGUID to userspace if
+ * we have no UUID set
+ */
+ if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid))) {
+ printk_ratelimited(KERN_WARNING
+ "No UUID available providing old NGUID\n");

+ return sprintf(buf, "%pU\n", ns->nguid);

+ }
+ return sprintf(buf, "%pU\n", ns->uuid);

}
static DEVICE_ATTR(uuid, S_IRUGO, uuid_show, NULL);

@@ -1914,6 +1931,7 @@ static DEVICE_ATTR(nsid, S_IRUGO, nsid_show, NULL);
static struct attribute *nvme_ns_attrs[] = {
&dev_attr_wwid.attr,
&dev_attr_uuid.attr,
+ &dev_attr_nguid.attr,
&dev_attr_eui.attr,
&dev_attr_nsid.attr,
NULL,

@@ -1926,6 +1944,11 @@ static umode_t nvme_ns_attrs_are_visible(struct kobject *kobj,

struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

if (a == &dev_attr_uuid.attr) {

+ if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid)) ||
+ !memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
+ return 0;
+ }
+ if (a == &dev_attr_nguid.attr) {

if (!memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
return 0;
}

--
2.12.0

[Johannes Thumshirn]

Use NVME_IDENTIFY_DATA_SIZE define instead of hard coding the magic
4096 value.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
---

drivers/nvme/target/admin-cmd.c | 4 ++--
drivers/nvme/target/discovery.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 833b5ef935c3..def6cde1e714 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c

@@ -336,7 +336,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)

static void nvmet_execute_identify_nslist(struct nvmet_req *req)

[Sagi Grimberg]

Looks good Johannes,

Reviewed-by: Sagi Grimberg <sa...@grimberg.me>

[Sagi Grimberg]

[Sagi Grimberg]

Nit: _hdr is usually associated with a message or alike.

Maybe nvme_ns_id_desc is more consistent with the spec language.

[Sagi Grimberg]

On 04/06/17 13:36, Johannes Thumshirn wrote:

Hmm, "target" is not a spec'd entity in NVMe AFAIR, and we try to avoid
using this language in the host too, lets call it "ctrl".

[Sagi Grimberg]

[Sagi Grimberg]

On 04/06/17 13:36, Johannes Thumshirn wrote:

Nit: maybe a dot separator would be better? e.g. "1.3.0" rather than
"1 3 0"

Otherwise looks good,

Reviewed-by: Sagi Grimberg <sa...@grimberg.me>

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sa...@rimberg.me>

[Sagi Grimberg]

Plain override in case a namespace is fed with both uuid and nguid?

Perhaps we should try to help the user a bit here and either fail
the user trying to populate both in configfs or pick one here.

> + memset(&hdr, 0, sizeof(hdr));
> + hdr.nidt = NVME_NIDT_NGUID;
> + hdr.nidl = NVME_NIDT_NGUID_LEN;
> + status = nvmet_copy_to_sgl(req, off, &hdr, sizeof(hdr));
> + if (status)
> + goto out_put_ns;
> + off += sizeof(hdr);
> +
> + status = nvmet_copy_to_sgl(req, off, &ns->nguid,
> + sizeof(ns->nguid));
> + if (status)
> + goto out_put_ns;
> + off += sizeof(ns->nguid);
> + }

If none exist we return whatever the the stack contains in hdr...
Lets initialize hdr with {} at the declaration.

[Sagi Grimberg]

Looks fine,

Reviewed-by: Sagi Grimberg <sa...@grimberg.me>

[Christoph Hellwig]

What about a little helper like this: ?

static u16 nvmet_copy_ns_identifier(struct nvmet_req *req, u8 type, u8 len,
void *id, off_t *off)
{
struct nvme_ns_identifier_hdr hdr = {
.nidt = type,
.nidl = len,
};
u16 status;

status = nvmet_copy_to_sgl(req, *off, &hdr, sizeof(hdr));
if (status)
return status;
*off += sizeof(hdr);

status = nvmet_copy_to_sgl(req, off, id, len);
if (status)
return status;
*off += len;

return 0;
}

[Christoph Hellwig]

On Sun, Jun 04, 2017 at 12:36:41PM +0200, Johannes Thumshirn wrote:
> The Namespace Identification Descriptor list is the only way a target
> can identify itself via the newly introduced UUID to the host (instead
> of the EUI-64 or NGUID).

s/instead/in addition/

> While I was already touching the relevant code paths, I decided to
> also include the EUI-64 in the 'Identify Namespace' command response.

Not anymore :)

[Christoph Hellwig]

> + }
> + len = NVME_NIDT_UUID_LEN;
> + memcpy(ns->uuid, data + pos + sizeof(*cur), len);
> + break;
> + default:
> + dev_warn(ns->ctrl->dev,
> + "Invalid Namespace Identification Descriptor Type: %d\n",
> + cur->nidt);
> + return;

Please drop the warning and return, the spec says hosts should ignore
unknown types. This is important to future proof for new types that
could be added.

> +static int nvme_identify_ns_descs(struct nvme_ns *ns, unsigned nsid)
> +{
> + struct nvme_command c = { };
> + int status;
> + void *data;
> +
> + c.identify.opcode = nvme_admin_identify;
> + c.identify.nsid = cpu_to_le32(nsid);
> + c.identify.cns = NVME_ID_CNS_NS_DESC_LIST;
> +
> + data = kzalloc(NVME_IDENTIFY_DATA_SIZE, GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + status = nvme_submit_sync_cmd(ns->ctrl->admin_q, &c, data,
> + NVME_IDENTIFY_DATA_SIZE);
> + if (status)
> + goto free_data;
> +
> + nvme_parse_ns_descs(ns, data);

Just merge nvme_parse_ns_descs into the caller?

[Christoph Hellwig]

I think this needs to zero the remainder of the 4k
buffer, quoting the spec:

"The controller may return any number of variable length Namespace
Identification Descriptor structures that fit into the 4096 byte Identify
payload. All remaining bytes after the namespace identification descriptor
structures should be cleared to 0h, and the host shall interpret a
Namespace Identifier Descriptor Length (NIDL) value of 0h as the end of
the list. If the hosts sees an unknown descriptor type it should
continue parsing the structure."

It could be easily done by copying the zero page to userspace.
Eventually I want to add a helper to zero parts of a sglist to
lib/scatterlist.c, but I don't want to burden that on you.

[Christoph Hellwig]

On Sun, Jun 04, 2017 at 05:59:06PM +0300, Sagi Grimberg wrote:
>> +struct nvme_ns_identifier_hdr {
>> + __u8 nidt;
>> + __u8 nidl;
>> + __le16 reserved;
>> +};
>
> Nit: _hdr is usually associated with a message or alike.
>
> Maybe nvme_ns_id_desc is more consistent with the spec language.

The descriptor itself includes the actual value, that's why
I suggested adding the _hdr prefix. But I'm fine going either way.

[Christoph Hellwig]

Nit Dop we really need all these variables? Why not:

return snprintf(page, PAGE_SIZE, "%d %d %d\n", NVME_MAJOR(subsys->ver),
NVME_MINOR(subsys->ver), NVME_TERRIARY(subsys->ver));

?

Also except for the 1.2.1 oddbackk NVMe versions usually have two
components, and should be printed as such if the tertiary version is
0.

> +static ssize_t nvmet_subsys_version_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct nvmet_subsys *subsys = to_subsys(item);
> + int major, minor, tertiary;
> + int ret;
> +
> +
> + ret = sscanf(page, "%d %d %d\n", &major, &minor, &tertiary);
> + if (ret != 3)
> + return -EINVAL;

Same issue here. I also have to say I'm a bit sceptical about
just being able to set versions as there are various dependencies
on it. E.g. for 1.0 the version field in identify namespace doesn't
even exists and should be cleared to 0.

> +
> + subsys->ver = NVME_VS(major, minor, tertiary);

locking?

> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index afa6ef484e50..0d6e307a7aa4 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -1069,4 +1069,8 @@ struct nvme_completion {
> #define NVME_VS(major, minor, tertiary) \
> (((major) << 16) | ((minor) << 8) | (tertiary))
>
> +#define NVME_MAJOR(ver) ((ver) >> 16)
> +#define NVME_MINOR(ver) (((ver) >> 8) & 0xff)
> +#define NVME_TERRIARY(ver) ((ver) & 0xff)

I think TERRIARY should be TERTIARY.

[Christoph Hellwig]

On Sun, Jun 04, 2017 at 12:36:49PM +0200, Johannes Thumshirn wrote:
> Use NVME_IDENTIFY_DATA_SIZE define instead of hard coding the magic
> 4096 value.

Can you bump this to the front of the series?

[Hannes Reinecke]

On 06/04/2017 12:36 PM, Johannes Thumshirn wrote:
> A NVMe Identify NS command with a CNS value of '3' is expecting a list
> of Namespace Identification Descriptor structures to be returned to
> the host for the namespace requested in the namespace identify
> command.
>
> This Namespace Identification Descriptor structure consists of the
> type of the namespace identifier, the length of the identifier and the
> actual identifier.
>
> Valid types are NGUID and UUID which we have saved in our nvme_ns
> structure if they have been configured via configfs. If no value has
> been assigened to one of these we return an "invalid opcode" back to
> the host to maintain backward compatibiliy with older implementations
> without Namespace Identify Descriptor list support.
>
> Also as the Namespace Identify Descriptor list is the only mandatory
> feature change between 1.2.1 and 1.3.0 we can bump the advertised
> version as well.
>
> Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
> ---
> drivers/nvme/target/admin-cmd.c | 53 +++++++++++++++++++++++++++++++++++++++++
> drivers/nvme/target/core.c | 3 ++-
> drivers/nvme/target/nvmet.h | 1 +
> 3 files changed, 56 insertions(+), 1 deletion(-)
>

Reviewed-by: Hannes Reinecke <ha...@suse.com>

Cheers,

Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
ha...@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[Hannes Reinecke]

On 06/04/2017 12:36 PM, Johannes Thumshirn wrote:

> Use NVME_IDENTIFY_DATA_SIZE define instead of hard coding the magic
> 4096 value.
>
> Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
> ---
> drivers/nvme/target/admin-cmd.c | 4 ++--
> drivers/nvme/target/discovery.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>

[Hannes Reinecke]

On 06/04/2017 12:36 PM, Johannes Thumshirn wrote:

> If a target identifies itself as NVMe 1.3 compliant, try to get the
> list of Namespace Identification Descriptors and populate the UUID,
> NGUID and EUI64 fileds in the NVMe namespace structure with these
> values.
>
> Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
> ---
> drivers/nvme/host/core.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/nvme/host/nvme.h | 1 +
> 2 files changed, 88 insertions(+)
>

[Hannes Reinecke]

On 06/04/2017 12:36 PM, Johannes Thumshirn wrote:

> Allow overriding the announced NVMe Version of a via configfs.
>
> This is particularly helpful when debugging new features for the host
> or target side without bumping the hard coded version (as the target
> might not be fully compliant to the announced version yet).
>

> Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
> ---

> drivers/nvme/target/configfs.c | 34 ++++++++++++++++++++++++++++++++++
> include/linux/nvme.h | 4 ++++

> 2 files changed, 38 insertions(+)

[Hannes Reinecke]

On 06/04/2017 12:36 PM, Johannes Thumshirn wrote:

> Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
> Reviewed-by: Max Gurtovoy <ma...@mellanox.com>
> ---
> include/linux/nvme.h | 19 +++++++++++++++++++

> 1 file changed, 19 insertions(+)

[Hannes Reinecke]

On 06/04/2017 12:36 PM, Johannes Thumshirn wrote:

> Add the UUID field from the NVMe Namespace Identification Descriptor
> to the nvmet_ns structure and allow it's population via configfs.
>
> Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
> ---
> drivers/nvme/target/configfs.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>

[Sagi Grimberg]

On 05/06/17 08:38, Christoph Hellwig wrote:
>> + }
>> + len = NVME_NIDT_UUID_LEN;
>> + memcpy(ns->uuid, data + pos + sizeof(*cur), len);
>> + break;
>> + default:
>> + dev_warn(ns->ctrl->dev,
>> + "Invalid Namespace Identification Descriptor Type: %d\n",
>> + cur->nidt);
>> + return;
>
> Please drop the warning and return, the spec says hosts should ignore
> unknown types. This is important to future proof for new types that
> could be added.

Also, please stay consistent with the rest of the driver by logging the
correct device prefix using ctrl->device and not ctrl->dev.

(if we have some ctrl->dev left-overs, can you also send a patch to fix?)

[Johannes Thumshirn]

Yep sounds good. Sorry I didn't see this for the v4 submission, I'll
include in (the hopefully final) v5.

--
Johannes Thumshirn Storage
jthum...@suse.de +49 911 74053 689

SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg

GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[Christoph Hellwig]

Can you rebase on top of the new uuid_t type and the plain uuid_*
prefixed helpers here:

http://git.infradead.org/users/hch/uuid.git/shortlog/refs/heads/for-next

I'll declare that branch stable soon and don't want to introduce
more users of the old types. Thanks!

[Johannes Thumshirn]

Sure

[Johannes Thumshirn]

On 06/05/2017 07:35 AM, Christoph Hellwig wrote:
> I think this needs to zero the remainder of the 4k
> buffer, quoting the spec:

*gah* yeah, my initial version with the kzalloc() took care of it.

> It could be easily done by copying the zero page to userspace.
> Eventually I want to add a helper to zero parts of a sglist to
> lib/scatterlist.c, but I don't want to burden that on you.

I'll have a look.

[Johannes Thumshirn]

The uuid field in the nvme_ns structure represents the nguid field
from the identify namespace command. And as NVMe 1.3 introduced an
UUID in the NVMe Namespace Identification Descriptor this will
collide.

So rename the uuid to nguid to prevent any further
confusion. Unfortunately we export the nguid to sysfs in the uuid
sysfs attribute, but this can't be changed anymore without possibly
breaking existing userspace.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
Reviewed-by: Christoph Hellwig <h...@lst.de>
Reviewed-by: Hannes Reinecke <ha...@suse.com>
Reviewed-by: Sagi Grimberg <sa...@grimberg.me>
---
drivers/nvme/host/core.c | 10 +++++-----
drivers/nvme/host/nvme.h | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a60926410438..7b254be16887 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1016,7 +1016,7 @@ static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id)
if (ns->ctrl->vs >= NVME_VS(1, 1, 0))
memcpy(ns->eui, (*id)->eui64, sizeof(ns->eui));
if (ns->ctrl->vs >= NVME_VS(1, 2, 0))
- memcpy(ns->uuid, (*id)->nguid, sizeof(ns->uuid));
+ memcpy(ns->nguid, (*id)->nguid, sizeof(ns->nguid));

return 0;
}
@@ -1784,8 +1784,8 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
int serial_len = sizeof(ctrl->serial);
int model_len = sizeof(ctrl->model);

- if (memchr_inv(ns->uuid, 0, sizeof(ns->uuid)))
- return sprintf(buf, "eui.%16phN\n", ns->uuid);

+ if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))

+ return sprintf(buf, "eui.%16phN\n", ns->nguid);

if (memchr_inv(ns->eui, 0, sizeof(ns->eui)))
return sprintf(buf, "eui.%8phN\n", ns->eui);
@@ -1804,7 +1804,7 @@ static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
- return sprintf(buf, "%pU\n", ns->uuid);
+ return sprintf(buf, "%pU\n", ns->nguid);
}
static DEVICE_ATTR(uuid, S_IRUGO, uuid_show, NULL);

@@ -1839,7 +1839,7 @@ static umode_t nvme_ns_attrs_are_visible(struct kobject *kobj,
struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

if (a == &dev_attr_uuid.attr) {
- if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid)))
+ if (!memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
return 0;
}
if (a == &dev_attr_eui.attr) {
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 9d6a070d4391..5004f0c41397 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -189,7 +189,7 @@ struct nvme_ns {
int instance;

u8 eui[8];
- u8 uuid[16];
+ u8 nguid[16];

unsigned ns_id;
int lba_shift;
--
2.12.3

[Johannes Thumshirn]

Add the UUID field from the NVMe Namespace Identification Descriptor
to the nvmet_ns structure and allow it's population via configfs.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
Reviewed-by: Max Gurtovoy <ma...@mellanox.com>
Reviewed-by: Sagi Grimberg <sa...@grimberg.me>
Reviewed-by: Hannes Reinecke <ha...@suse.com>

---
drivers/nvme/target/configfs.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index be8c800078e2..83bfe28fe7da 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -305,11 +305,41 @@ static ssize_t nvmet_ns_device_path_store(struct config_item *item,

CONFIGFS_ATTR(nvmet_ns_, device_path);

+static ssize_t nvmet_ns_device_uuid_show(struct config_item *item, char *page)
+{
+ return sprintf(page, "%pUb\n", &to_nvmet_ns(item)->uuid);
+}
+
+static ssize_t nvmet_ns_device_uuid_store(struct config_item *item,

+ const char *page, size_t count)
+{

+ struct nvmet_ns *ns = to_nvmet_ns(item);
+ struct nvmet_subsys *subsys = ns->subsys;
+ int ret = 0;
+
+
+ mutex_lock(&subsys->lock);
+ if (ns->enabled) {
+ ret = -EBUSY;
+ goto out_unlock;
+ }
+
+
+ if (uuid_parse(page, &ns->uuid))
+ ret = -EINVAL;
+
+out_unlock:
+ mutex_unlock(&subsys->lock);
+ return ret ? ret : count;
+}
+
static ssize_t nvmet_ns_device_nguid_show(struct config_item *item, char *page)
{
return sprintf(page, "%pUb\n", &to_nvmet_ns(item)->nguid);
}

+CONFIGFS_ATTR(nvmet_ns_, device_uuid);
+
static ssize_t nvmet_ns_device_nguid_store(struct config_item *item,

const char *page, size_t count)

{
@@ -379,6 +409,7 @@ CONFIGFS_ATTR(nvmet_ns_, enable);
static struct configfs_attribute *nvmet_ns_attrs[] = {
&nvmet_ns_attr_device_path,
&nvmet_ns_attr_device_nguid,
+ &nvmet_ns_attr_device_uuid,
&nvmet_ns_attr_enable,
NULL,
};
--
2.12.3

[Johannes Thumshirn]

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
Reviewed-by: Max Gurtovoy <ma...@mellanox.com>
Reviewed-by: Sagi Grimberg <sa...@grimberg.me>
Reviewed-by: Hannes Reinecke <ha...@suse.com>
---

include/linux/nvme.h | 17 +++++++++++++++++
1 file changed, 17 insertions(+)

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index c004af5faa48..22da030a28c6 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -289,6 +289,7 @@ enum {
NVME_ID_CNS_NS = 0x00,
NVME_ID_CNS_CTRL = 0x01,
NVME_ID_CNS_NS_ACTIVE_LIST = 0x02,
+ NVME_ID_CNS_NS_DESC_LIST = 0x03,
NVME_ID_CNS_NS_PRESENT_LIST = 0x10,
NVME_ID_CNS_NS_PRESENT = 0x11,
NVME_ID_CNS_CTRL_NS_LIST = 0x12,
@@ -315,6 +316,22 @@ enum {
NVME_NS_DPS_PI_TYPE3 = 3,
};

+struct nvme_ns_identifier_hdr {
+ __u8 nidt;
+ __u8 nidl;
+ __le16 reserved;
+};

+
+#define NVME_NIDT_EUI64_LEN 8
+#define NVME_NIDT_NGUID_LEN 16
+#define NVME_NIDT_UUID_LEN 16
+
+enum {
+ NVME_NIDT_EUI64 = 0x01,
+ NVME_NIDT_NGUID = 0x02,
+ NVME_NIDT_UUID = 0x03,
+};
+
struct nvme_smart_log {
__u8 critical_warning;
__u8 temperature[2];
--
2.12.3

[Johannes Thumshirn]

Allow overriding the announced NVMe Version of a via configfs.

This is particularly helpful when debugging new features for the host
or target side without bumping the hard coded version (as the target
might not be fully compliant to the announced version yet).

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>

Reviewed-by: Hannes Reinecke <ha...@suse.com>
---

drivers/nvme/target/configfs.c | 37 +++++++++++++++++++++++++++++++++++++
include/linux/nvme.h | 4 ++++
2 files changed, 41 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 83bfe28fe7da..61995e0cd0c1 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -650,8 +650,45 @@ static ssize_t nvmet_subsys_attr_allow_any_host_store(struct config_item *item,

CONFIGFS_ATTR(nvmet_subsys_, attr_allow_any_host);

+static ssize_t nvmet_subsys_version_show(struct config_item *item,
+ char *page)
+{

+ struct nvmet_subsys *subsys = to_subsys(item);
+
+ if (NVME_TERTIARY(subsys->ver))
+ return snprintf(page, PAGE_SIZE, "%d.%d.%d\n",
+ (int)NVME_MAJOR(subsys->ver),
+ (int)NVME_MINOR(subsys->ver),
+ (int)NVME_TERTIARY(subsys->ver));
+ else
+ return snprintf(page, PAGE_SIZE, "%d.%d\n",
+ (int)NVME_MAJOR(subsys->ver),
+ (int)NVME_MINOR(subsys->ver));
+}
+
+static ssize_t nvmet_subsys_version_store(struct config_item *item,

+ const char *page, size_t count)
+{

+ struct nvmet_subsys *subsys = to_subsys(item);
+ int major, minor, tertiary;
+ int ret;
+
+

+ ret = sscanf(page, "%d.%d.%d\n", &major, &minor, &tertiary);
+ if (ret != 2 && ret != 3)
+ return -EINVAL;
+
+ down_write(&nvmet_config_sem);

+ subsys->ver = NVME_VS(major, minor, tertiary);

+ up_write(&nvmet_config_sem);
+
+ return count;
+}
+CONFIGFS_ATTR(nvmet_subsys_, version);
+
static struct configfs_attribute *nvmet_subsys_attrs[] = {
&nvmet_subsys_attr_attr_allow_any_host,
+ &nvmet_subsys_attr_version,
NULL,
};

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 22da030a28c6..667077ded7eb 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1070,4 +1070,8 @@ struct nvme_completion {

#define NVME_VS(major, minor, tertiary) \
(((major) << 16) | ((minor) << 8) | (tertiary))

+#define NVME_MAJOR(ver) ((ver) >> 16)
+#define NVME_MINOR(ver) (((ver) >> 8) & 0xff)

+#define NVME_TERTIARY(ver) ((ver) & 0xff)
+
#endif /* _LINUX_NVME_H */
--
2.12.3

[Johannes Thumshirn]

If a target identifies itself as NVMe 1.3 compliant, try to get the
list of Namespace Identification Descriptors and populate the UUID,
NGUID and EUI64 fileds in the NVMe namespace structure with these
values.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>
Reviewed-by: Hannes Reinecke <ha...@suse.com>
---

drivers/nvme/host/core.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++
drivers/nvme/host/nvme.h | 1 +
2 files changed, 80 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 7b254be16887..b0c7689427da 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -643,6 +643,77 @@ int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
return error;

}

+static int nvme_identify_ns_descs(struct nvme_ns *ns, unsigned nsid)
+{
+ struct nvme_command c = { };
+ int status;
+ void *data;

+ int pos;
+ int len;

+
+ c.identify.opcode = nvme_admin_identify;
+ c.identify.nsid = cpu_to_le32(nsid);
+ c.identify.cns = NVME_ID_CNS_NS_DESC_LIST;
+
+ data = kzalloc(NVME_IDENTIFY_DATA_SIZE, GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ status = nvme_submit_sync_cmd(ns->ctrl->admin_q, &c, data,
+ NVME_IDENTIFY_DATA_SIZE);
+ if (status)
+ goto free_data;
+

+ for (pos = 0; pos < NVME_IDENTIFY_DATA_SIZE; pos += len) {
+ struct nvme_ns_identifier_hdr *cur = data + pos;
+
+ if (cur->nidl == 0)
+ break;
+
+ switch (cur->nidt) {
+ case NVME_NIDT_EUI64:
+ if (cur->nidl != NVME_NIDT_EUI64_LEN) {
+ dev_warn(ns->ctrl->device,
+ "ctrl returned bogus length: %d for NVME_NIDT_EUI64\n",
+ cur->nidl);
+ goto free_data;
+ }
+ len = NVME_NIDT_EUI64_LEN;
+ memcpy(ns->eui, data + pos + sizeof(*cur), len);
+ break;
+ case NVME_NIDT_NGUID:
+ if (cur->nidl != NVME_NIDT_NGUID_LEN) {
+ dev_warn(ns->ctrl->device,
+ "ctrl returned bogus length: %d for NVME_NIDT_NGUID\n",
+ cur->nidl);
+ goto free_data;
+ }
+ len = NVME_NIDT_NGUID_LEN;
+ memcpy(ns->nguid, data + pos + sizeof(*cur), len);
+ break;
+ case NVME_NIDT_UUID:
+ if (cur->nidl != NVME_NIDT_UUID_LEN) {
+ dev_warn(ns->ctrl->device,
+ "ctrl returned bogus length: %d for NVME_NIDT_UUID\n",
+ cur->nidl);
+ goto free_data;

+ }
+ len = NVME_NIDT_UUID_LEN;
+ memcpy(ns->uuid, data + pos + sizeof(*cur), len);
+ break;
+ default:

+ /* Skip unnkown types */
+ len = cur->nidl;
+ break;
+ }
+
+ len += sizeof(*cur);
+ }
+free_data:
+ kfree(data);
+ return status;
+}
+
static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *ns_list)
{
struct nvme_command c = { };
@@ -1017,6 +1088,14 @@ static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id)

memcpy(ns->eui, (*id)->eui64, sizeof(ns->eui));
if (ns->ctrl->vs >= NVME_VS(1, 2, 0))

memcpy(ns->nguid, (*id)->nguid, sizeof(ns->nguid));

+ if (ns->ctrl->vs >= NVME_VS(1, 3, 0)) {
+ /* Don't treat error as fatal we potentially
+ * already have a NGUID or EUI-64
+ */
+ if (nvme_identify_ns_descs(ns, ns->ns_id))
+ dev_warn(ns->ctrl->dev,
+ "%s: Identify Descriptors failed\n", __func__);
+ }

return 0;
}
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 5004f0c41397..7007521e8194 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -190,6 +190,7 @@ struct nvme_ns {

u8 eui[8];
u8 nguid[16];
+ u8 uuid[16];

[Johannes Thumshirn]

Use NVME_IDENTIFY_DATA_SIZE define instead of hard coding the magic
4096 value.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>

Reviewed-by: Max Gurtovoy <ma...@mellanox.com>
Reviewed-by: Sagi Grimberg <sa...@grimberg.me>

Reviewed-by: Hannes Reinecke <ha...@suse.com>
---

drivers/nvme/target/admin-cmd.c | 4 ++--
drivers/nvme/target/discovery.c | 2 +-

include/linux/nvme.h | 2 ++
3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index ff1f97006322..96c144325443 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -336,7 +336,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)

static void nvmet_execute_identify_nslist(struct nvmet_req *req)
{
- static const int buf_size = 4096;
+ static const int buf_size = NVME_IDENTIFY_DATA_SIZE;
struct nvmet_ctrl *ctrl = req->sq->ctrl;
struct nvmet_ns *ns;
u32 min_nsid = le32_to_cpu(req->cmd->identify.nsid);
@@ -504,7 +504,7 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
}
break;
case nvme_admin_identify:
- req->data_len = 4096;
+ req->data_len = NVME_IDENTIFY_DATA_SIZE;
switch (cmd->identify.cns) {
case NVME_ID_CNS_NS:
req->execute = nvmet_execute_identify_ns;
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index 1aaf597e81fc..c7a90384dd75 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -185,7 +185,7 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
}
case nvme_admin_identify:
- req->data_len = 4096;
+ req->data_len = NVME_IDENTIFY_DATA_SIZE;
switch (cmd->identify.cns) {
case NVME_ID_CNS_CTRL:
req->execute =
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index e400a69fa1d3..c004af5faa48 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -659,6 +659,8 @@ struct nvme_identify {
__u32 rsvd11[5];
};

+#define NVME_IDENTIFY_DATA_SIZE 4096
+
struct nvme_features {
__u8 opcode;
__u8 flags;
--
2.12.3

[Johannes Thumshirn]

This patchset implemets NVMe Namespace Descriptor Identification as of
NVMe 1.3. The Namespace Descriptor Identification allows a NVMe host
to query several Namespace Identification mechanisms, such as EUI-64,
NGUID and UUID from the target. If more than one value is set by the
target, it can transmit all set values to the host.

The Namespace Identification Descriptor list is the only way a target
can identify itself via the newly introduced UUID to the host (in

addition to the EUI-64 or NGUID).

Both the Host and Target side are implemented. In order to get the
Linux Host to send the Linux target implementation a Namespace
Descriptor Identification command, you have to change the target's
announced version code to at least 1.3.

Unfortunately the host side already did have a sysfs attribute called
'uuid' which represented the NGUID, so precautions have been taken to
not break any existing userspace.

The code is tested using the nvme-loop loopback target and cut against
Christoph's uuid/for-next branch.

A patch for nvmetcli and nvme-cli will follow shortly.

Changes to v4:
* Add nvmet_copy_ns_identifier() helper (Christoph)
* Move 'nvmet: use NVME_IDENTIFY_DATA_SIZE' to beginning of series (Christoph)
* Protect subsystem version with nvmet_config_sem (Christoph)
* Get rid of local variables in nvmet_subsys_version_show (Christoph)
* Included dots in subsystem version (Sagi)
* Merge nvme_parse_ns_descs() into nvme_identify_ns_descs() (Christoph)
* Skip unknown NS descriptors in nvme_identify_ns_descs() (Christoph)
* Add sg_zeroout_area() helper to lib/scatterlist.c (Christoph)
* I'm not sure if I shall use nvme_ns_identifier_hdr or nvme_ns_id_desc
* Rebased on Christoph's uuid/for-next branch
* Collected Acks

Changes to v3:
* Autogenerate UUID on target NS allocation (Sagi)

Changes to v2:
* Added Max's Reviewed-by
* Make series bisectable

Changes to v1:
* Added Reviewed-by tags from Christoph and Hannes for unchanged patches
* Added patch introducing new structs at the beginning (Christoph)
* Dropped SZ_4K patch
* Got rid of dynamic memory allocation on the target side (Christoph)
* Reworked host side parser (Christoph)
* Check length inside type check in the host parser (Max)

Changes to v0:
* Fixed wrong size of 4069 and replaced it with SZ_4K (Max)
* Add constants for UUID, NGUID and EUI-64 length (Max)
* Drop EUI-64 Support on target side (Christoph)
* Use uuid_be instead of u8[] (Christoph)
* Add ability to override target's version (Hannes)
* Change hard coded magic 4096 and 0x1000 to SZ_4k in drivers/nvme/*

Johannes Thumshirn (9):
scatterlist: add sg_zeroout_area() helper
nvmet: use NVME_IDENTIFY_DATA_SIZE
nvme: introduce NVMe Namespace Identification Descriptor structures
nvme: rename uuid to nguid in nvme_ns
nvmet: implement namespace identify descriptor list
nvmet: add uuid field to nvme_ns and populate via configfs
nvme: get list of namespace descriptors
nvme: provide UUID value to userspace
nvmet: allow overriding the NVMe VS via configfs

drivers/nvme/host/core.c | 110 ++++++++++++++++++++++++++++++++++++++--
drivers/nvme/host/nvme.h | 1 +
drivers/nvme/target/admin-cmd.c | 65 +++++++++++++++++++++++-
drivers/nvme/target/configfs.c | 68 +++++++++++++++++++++++++
drivers/nvme/target/core.c | 3 +-
drivers/nvme/target/discovery.c | 2 +-
drivers/nvme/target/nvmet.h | 1 +
include/linux/nvme.h | 23 +++++++++
include/linux/scatterlist.h | 3 ++
lib/scatterlist.c | 37 ++++++++++++++
10 files changed, 305 insertions(+), 8 deletions(-)

--
2.12.3

[Johannes Thumshirn]

Now that we have a way for getting the UUID from a target, provide it
to userspace as well.

Unfortunately there is already a sysfs attribute called UUID which is
a misnomer as it holds the NGUID value. So instead of creating yet
another wrong name, create a new 'nguid' sysfs attribute for the
NGUID. For the UUID attribute add a check wheter the namespace has a
UUID assigned to it and return this or return the NGUID to maintain
backwards compatibility. This should give userspace a chance to catch
up.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>

Reviewed-by: Christoph Hellwig <h...@lst.de>
Reviewed-by: Hannes Reinecke <ha...@suse.com>

Reviewed-by: Sagi Grimberg <sa...@rimberg.me>
---
drivers/nvme/host/core.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b0c7689427da..c221c86b78f1 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1879,11 +1879,28 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
}
static DEVICE_ATTR(wwid, S_IRUGO, wwid_show, NULL);

+static ssize_t nguid_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

+ return sprintf(buf, "%pU\n", ns->nguid);

+}
+static DEVICE_ATTR(nguid, S_IRUGO, nguid_show, NULL);
+

static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

- return sprintf(buf, "%pU\n", ns->nguid);
+
+ /* For backward compatibility expose the NGUID to userspace if
+ * we have no UUID set
+ */
+ if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid))) {
+ printk_ratelimited(KERN_WARNING
+ "No UUID available providing old NGUID\n");

+ return sprintf(buf, "%pU\n", ns->nguid);

+ }
+ return sprintf(buf, "%pU\n", ns->uuid);

}
static DEVICE_ATTR(uuid, S_IRUGO, uuid_show, NULL);

@@ -1906,6 +1923,7 @@ static DEVICE_ATTR(nsid, S_IRUGO, nsid_show, NULL);
static struct attribute *nvme_ns_attrs[] = {
&dev_attr_wwid.attr,
&dev_attr_uuid.attr,
+ &dev_attr_nguid.attr,
&dev_attr_eui.attr,
&dev_attr_nsid.attr,
NULL,
@@ -1918,6 +1936,11 @@ static umode_t nvme_ns_attrs_are_visible(struct kobject *kobj,

struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

if (a == &dev_attr_uuid.attr) {

+ if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid)) ||
+ !memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
+ return 0;
+ }
+ if (a == &dev_attr_nguid.attr) {

if (!memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
return 0;
}

--
2.12.3

[Johannes Thumshirn]

A NVMe Identify NS command with a CNS value of '3' is expecting a list
of Namespace Identification Descriptor structures to be returned to
the host for the namespace requested in the namespace identify
command.

This Namespace Identification Descriptor structure consists of the
type of the namespace identifier, the length of the identifier and the
actual identifier.

Valid types are NGUID and UUID which we have saved in our nvme_ns
structure if they have been configured via configfs. If no value has
been assigened to one of these we return an "invalid opcode" back to
the host to maintain backward compatibiliy with older implementations
without Namespace Identify Descriptor list support.

Also as the Namespace Identify Descriptor list is the only mandatory
feature change between 1.2.1 and 1.3.0 we can bump the advertised
version as well.

Signed-off-by: Johannes Thumshirn <jthum...@suse.de>

Reviewed-by: Hannes Reinecke <ha...@suse.com>
---

drivers/nvme/target/admin-cmd.c | 61 +++++++++++++++++++++++++++++++++++++++++
drivers/nvme/target/core.c | 3 +-
drivers/nvme/target/nvmet.h | 1 +
3 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 96c144325443..f15aa72e28b4 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -367,6 +367,64 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req)
nvmet_req_complete(req, status);
}

+static u16 nvmet_copy_ns_identifier(struct nvmet_req *req, u8 type, u8 len,
+ void *id, off_t *off)
+{
+ struct nvme_ns_identifier_hdr hdr = {
+ .nidt = type,
+ .nidl = len,
+ };
+ u16 status;
+
+ status = nvmet_copy_to_sgl(req, *off, &hdr, sizeof(hdr));
+ if (status)
+ return status;
+ *off += sizeof(hdr);
+
+ status = nvmet_copy_to_sgl(req, *off, id, len);
+ if (status)
+ return status;
+ *off += len;
+

+ return 0;
+}
+

+static void nvmet_execute_identify_desclist(struct nvmet_req *req)
+{
+ struct nvmet_ns *ns;
+ u16 status = 0;
+ off_t off = 0;
+
+ ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid);
+ if (!ns) {
+ status = NVME_SC_INVALID_NS | NVME_SC_DNR;
+ goto out;
+ }
+
+ if (memchr_inv(&ns->uuid, 0, sizeof(ns->uuid))) {
+ status = nvmet_copy_ns_identifier(req, NVME_NIDT_UUID,
+ NVME_NIDT_UUID_LEN,
+ &ns->uuid, &off);
+ if (status)
+ goto out_put_ns;
+ }
+ if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid))) {
+ status = nvmet_copy_ns_identifier(req, NVME_NIDT_NGUID,
+ NVME_NIDT_NGUID_LEN,
+ &ns->nguid, &off);
+ if (status)
+ goto out_put_ns;
+ }
+
+ if (sg_zeroout_area(req->sg, req->sg_cnt, NVME_IDENTIFY_DATA_SIZE, off)
+ != NVME_IDENTIFY_DATA_SIZE - off)
+ status = NVME_SC_INTERNAL | NVME_SC_DNR;
+out_put_ns:
+ nvmet_put_namespace(ns);
+out:
+ nvmet_req_complete(req, status);
+}
+
/*
* A "mimimum viable" abort implementation: the command is mandatory in the
* spec, but we are not required to do any useful work. We couldn't really
@@ -515,6 +573,9 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
case NVME_ID_CNS_NS_ACTIVE_LIST:
req->execute = nvmet_execute_identify_nslist;
return 0;
+ case NVME_ID_CNS_NS_DESC_LIST:
+ req->execute = nvmet_execute_identify_desclist;
+ return 0;
}
break;
case nvme_admin_abort_cmd:
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index eb9399ac97cf..b5b4ac103748 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -380,6 +380,7 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)

ns->nsid = nsid;
ns->subsys = subsys;
+ uuid_gen(&ns->uuid);

return ns;
}
@@ -926,7 +927,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
if (!subsys)
return NULL;

- subsys->ver = NVME_VS(1, 2, 1); /* NVMe 1.2.1 */
+ subsys->ver = NVME_VS(1, 3, 0); /* NVMe 1.3.0 */

switch (type) {
case NVME_NQN_NVME:
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 8ff6e430b30a..747bbdb4f9c6 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -47,6 +47,7 @@ struct nvmet_ns {
u32 blksize_shift;
loff_t size;
u8 nguid[16];
+ uuid_t uuid;

bool enabled;
struct nvmet_subsys *subsys;
--
2.12.3