Implement NVMe Namespace Descriptor Identification
Subsystems (2)
cover Implement NVMe Namespace Descriptor Identification
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(-)
Signature · 1 line
2.12.0
PATCH 1/7 nvme: rename uuid to nguid in nvme_ns
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.
--- drivers/nvme/host/core.c | 10 +++++----- drivers/nvme/host/nvme.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-)
drivers/nvme/host/core.c +5 -5
@@ -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) {
drivers/nvme/host/nvme.h +1 -1
@@ -189,7 +189,7 @@ struct nvme_ns {int instance;u8 eui[8 ]; -u8 uuid[16 ]; +u8 nguid[16 ];unsigned ns_id;int lba_shift;
Signature · 1 line
2.12.0
Re: [PATCH v1 1/7] nvme: rename uuid to nguid in nvme_ns
Quoted 11 lines · Johannes Thumshirn
The uuid field in the
nvme_nsstructure 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 <jthumshirn at suse.de>
Looks fine,
PATCH 2/7 nvmet: add uuid field to nvme_ns and populate via configfs
In reply to Johannes ThumshirnAdd the UUID field from the NVMe Namespace Identification Descriptor to the nvmet_ns structure and allow it's population via configfs.
--- drivers/nvme/target/configfs.c | 31 +++++++++++++++++++++++++++++++ drivers/nvme/target/nvmet.h | 1 + 2 files changed, 32 insertions(+)
drivers/nvme/target/configfs.c +31 -0
@@ -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 , };
drivers/nvme/target/nvmet.h +1 -0
@@ -46,6 +46,7 @@ struct nvmet_ns {u32 blksize_shift;loff_t size;u8 nguid[16 ]; +uuid_be uuid;bool enabled;struct nvmet_subsys *subsys;
Signature · 1 line
2.12.0
Christoph Hellwig
maintainerRe: [PATCH v1 2/7] nvmet: add uuid field to nvme_ns and populate via configfs
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.
PATCH 3/7 nvmet: implement namespace identify descriptor list
In reply to Johannes ThumshirnA 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.
--- drivers/nvme/target/admin-cmd.c | 63 +++++++++++++++++++++++++++++++++++++++++ include/linux/nvme.h | 18 ++++++++++++ 2 files changed, 81 insertions(+)
drivers/nvme/target/admin-cmd.c +63 -0
@@ -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:
include/linux/nvme.h +18 -0
@@ -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 + +enum { +NVME_NIDT_EUI64 =0x01 , +NVME_NIDT_NGUID =0x02 , +NVME_NIDT_UUID =0x03 , +}; +struct nvme_smart_log {__u8 critical_warning;__u8 temperature[2 ];
Signature · 1 line
2.12.0
Christoph Hellwig
maintainerRe: [PATCH v1 3/7] nvmet: implement namespace identify descriptor list
Quoted 15 lines · 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_nsstructure 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.
Quoted 4 lines
Signed-off-by Johannes Thumshirn <jthumshirn at 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?
drivers/nvme/target/admin-cmd.c static void nvmet_execute_identify_nslist(struct nvmet_req *req) · line 372
Quoted 1 line
drivers/nvme/target/admin-cmd.c +1
-0+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.
drivers/nvme/target/admin-cmd.c static void nvmet_execute_identify_nslist(struct nvmet_req *req) · line 392
Quoted 7 lines
drivers/nvme/target/admin-cmd.c +7
-0+ 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.
drivers/nvme/target/admin-cmd.c static void nvmet_execute_identify_nslist(struct nvmet_req *req) · line 405
Quoted 13 lines
drivers/nvme/target/admin-cmd.c +13
-0+ +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);
PATCH 4/7 nvme: get list of namespace descriptors
In reply to Johannes ThumshirnIf 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.
--- drivers/nvme/host/core.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/nvme/host/nvme.h | 1 + 2 files changed, 89 insertions(+)
drivers/nvme/host/core.c +88 -0
@@ -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 =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 ); +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 ; }
drivers/nvme/host/nvme.h +1 -0
@@ -190,6 +190,7 @@ struct nvme_ns {u8 eui[8 ];u8 nguid[16 ]; +u8 uuid[16 ];unsigned ns_id;int lba_shift;
Signature · 1 line
2.12.0
Re: [PATCH v1 4/7] nvme: get list of namespace descriptors
Quoted 26 lines · 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 <jthumshirn at suse.de>--- drivers/nvme/host/core.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/nvme/host/nvme.h | 1 + 2 files changed, 89 insertions(+)drivers/nvme/host/core.c +7
-0@@ -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; +
drivers/nvme/host/core.c int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id) · line 662
Quoted 10 lines
drivers/nvme/host/core.c +10
-0+ p = (u8 *)ns_nid; + +for (;;) { + cur = (struct nvme_ns_nid *)p; + +switch (cur->nidl) { +case 0 : +return ; +case 8 : +case 16 :
consider using the length defines from <linux/nvme.h> instead of 8/16, it's more intuitive code this way.
Actually, to be safer, you can check the length inside the second switch/case per case.
Quoted 36 lines
drivers/nvme/host/core.c +36
-0+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 =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; + } +} +
Christoph Hellwig
maintainerRe: [PATCH v1 4/7] nvme: get list of namespace descriptors
In reply to Johannes Thumshirndrivers/nvme/host/core.c int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id) · line 653
Quoted 8 lines
drivers/nvme/host/core.c +8
-0+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.
drivers/nvme/host/core.c int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id) · line 656
Quoted 3 lines
drivers/nvme/host/core.c +3
-0+ +for (;;) { + cur = (struct nvme_ns_nid *)p;
for (pos = 0; pos < NVME_ID_DATA_SIZE; pos += len) {
struct nvme_ns_nid *cur = data + pos;Quoted 4 lines
drivers/nvme/host/core.c +4
-0+ +switch (cur->nidl) { +case 0 : +return ;
drivers/nvme/host/core.c int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id) · line 670
Quoted 10 lines
drivers/nvme/host/core.c +10
-0+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.
Quoted 12 lines
drivers/nvme/host/core.c +12
-0+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 ; + } +
drivers/nvme/host/core.c static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id) · line 1106
Quoted 10 lines
drivers/nvme/host/core.c +10
-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().
PATCH 5/7 nvme: provide UUID value to userspace
In reply to Johannes ThumshirnNow 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.
--- drivers/nvme/host/core.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)
drivers/nvme/host/core.c +24 -1
@@ -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 ; }
Signature · 1 line
2.12.0
Re: [PATCH v1 5/7] nvme: provide UUID value to userspace
Looks fine:
PATCH 6/7 nvme: change magic 4096 to SZ_4K
In reply to Johannes ThumshirnChange the magic 4096 or 0x1000 to SZ_4K so it's easier to spot typos.
The magic numbers in the BUILD_BUG_ON() calls are left in place as there are no defines used for the other sizes as well.
--- drivers/nvme/host/core.c | 4 ++-- drivers/nvme/host/pci.c | 6 +++--- drivers/nvme/target/admin-cmd.c | 4 ++-- drivers/nvme/target/discovery.c | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-)
drivers/nvme/host/core.c +2 -2
@@ -715,7 +715,7 @@ static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *n c.identify.opcode = nvme_admin_identify; c.identify.cns =NVME_ID_CNS_NS_ACTIVE_LIST ; c.identify.nsid =cpu_to_le32 (nsid); -return nvme_submit_sync_cmd (dev->admin_q, &c, ns_list,0x1000 ); +return nvme_submit_sync_cmd (dev->admin_q, &c, ns_list,SZ_4K ); } int nvme_identify_ns(struct nvme_ctrl *dev, unsigned nsid,
@@ -2262,7 +2262,7 @@ static int nvme_scan_ns_list(struct nvme_ctrl *ctrl, unsigned nn)unsigned i, j, nsid, prev =0 , num_lists =DIV_ROUND_UP (nn,1024 );int ret =0 ; - ns_list =kzalloc (0x1000 ,GFP_KERNEL ); + ns_list =kzalloc (SZ_4K ,GFP_KERNEL );if (!ns_list)return -ENOMEM ;
drivers/nvme/host/pci.c +3 -3
@@ -1516,7 +1516,7 @@ static inline void nvme_release_cmb(struct nvme_dev *dev)static size_t db_bar_size (struct nvme_dev *dev,unsigned nr_io_queues) { -return 4096 + ((nr_io_queues +1 ) *8 * dev->db_stride); +return SZ_4K + ((nr_io_queues +1 ) *8 * dev->db_stride); }static int nvme_setup_io_queues (struct nvme_dev *dev)
@@ -1553,7 +1553,7 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)return -ENOMEM ; size =db_bar_size (dev, nr_io_queues); }while (1 ); - dev->dbs = dev->bar +4096 ; + dev->dbs = dev->bar +SZ_4K ; adminq->q_db = dev->dbs; }
@@ -1732,7 +1732,7 @@ static int nvme_pci_enable(struct nvme_dev *dev) dev->q_depth =min_t (int,NVME_CAP_MQES (cap) +1 ,NVME_Q_DEPTH ); dev->db_stride =1 <<NVME_CAP_STRIDE (cap); - dev->dbs = dev->bar +4096 ; + dev->dbs = dev->bar +SZ_4K ;/* * Temporary fix for the Apple controller found in the MacBook8,1 and
drivers/nvme/target/admin-cmd.c +2 -2
@@ -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 =SZ_4K ;struct nvmet_ctrl *ctrl = req->sq->ctrl;struct nvmet_ns *ns;u32 min_nsid =le32_to_cpu (req->cmd->identify.nsid);
@@ -564,7 +564,7 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req) }break ;case nvme_admin_identify: - req->data_len =4096 ; + req->data_len =SZ_4K ;switch (cmd->identify.cns) {case NVME_ID_CNS_NS : req->execute = nvmet_execute_identify_ns;
drivers/nvme/target/discovery.c +1 -1
@@ -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 =SZ_4K ;switch (cmd->identify.cns) {case NVME_ID_CNS_CTRL : req->execute =
Signature · 1 line
2.12.0
Christoph Hellwig
maintainerRe: [PATCH v1 6/7] nvme: change magic 4096 to SZ_4K
As mentioned before I'd prefer a menaingful name that indicates the purpose of the constant. In fact 4096 is a lot more obvious than SZ_4K, and shortet as well :)
PATCH 7/7 nvmet: allow overriding the NVMe VS via configfs
In reply to Johannes ThumshirnAllow 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).
--- drivers/nvme/target/configfs.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/nvme.h | 4 ++++ 2 files changed, 38 insertions(+)
drivers/nvme/target/configfs.c +34 -0
@@ -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 , };
include/linux/nvme.h +4 -0
@@ -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 */
Signature · 1 line
2.12.0
Christoph Hellwig
maintainerRe: [PATCH v1 7/7] nvmet: allow overriding the NVMe VS via configfs
Quoted 5 lines · 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).
It is. Just bump to 1.3, please as Identify 3h is the only mandatory
change: http://nvmexpress.org/changes/
Re: [PATCH v1 7/7] nvmet: allow overriding the NVMe VS via configfs
Quoted 10 lines · Christoph Hellwig
Quoted 5 lines · 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).
It is. Just bump to 1.3, please as Identify 3h is the only mandatory
change: http://nvmexpress.org/changes/
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
Signature · 5 lines
Dr. Hannes Reinecke Teamlead Storage & Networking hare at 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)