From 7491e0e4096f426de4a47fa241442c84e63c5a64 Mon Sep 17 00:00:00 2001 From: Minwoo Im Date: Tue, 18 Apr 2023 09:20:25 +0900 Subject: [PATCH 1/7] hw/nvme: add comment for nvme-ns properties Add more comments of existing properties for nvme-ns device. Signed-off-by: Minwoo Im Reviewed-by: Klaus Jensen Signed-off-by: Klaus Jensen --- hw/nvme/ctrl.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index fd917fcda1..020f37a780 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -43,7 +43,14 @@ * subsys= * -device nvme-ns,drive=,bus=,nsid=,\ * zoned=, \ - * subsys=,detached= + * subsys=,shared=, \ + * detached=, \ + * zoned.zone_size=, \ + * zoned.zone_capacity=, \ + * zoned.descr_ext_size=, \ + * zoned.max_active=, \ + * zoned.max_open=, \ + * zoned.cross_read= * * Note cmb_size_mb denotes size of CMB in MB. CMB is assumed to be at * offset 0 in BAR2 and supports only WDS, RDS and SQS for now. By default, the From cab1da59c2ff3bf08d0a7becf9b51e43a724a85c Mon Sep 17 00:00:00 2001 From: Minwoo Im Date: Tue, 18 Apr 2023 09:26:21 +0900 Subject: [PATCH 2/7] hw/nvme: consider COPY command in nvme_aio_err If we don't have NVME_CMD_COPY consideration in the switch statement in nvme_aio_err(), it will go to have NVME_INTERNAL_DEV_ERROR and `req->status` will be ovewritten to it. During the aio context, it might set the NVMe status field like NVME_CMD_SIZE_LIMIT, but it's overwritten in the nvme_aio_err(). Add consideration for the NVME_CMD_COPY not to overwrite the status at the end of the function. Signed-off-by: Minwoo Im Reviewed-by: Klaus Jensen Signed-off-by: Klaus Jensen --- hw/nvme/ctrl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index 020f37a780..e031c2250a 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -1755,6 +1755,7 @@ static void nvme_aio_err(NvmeRequest *req, int ret) case NVME_CMD_WRITE: case NVME_CMD_WRITE_ZEROES: case NVME_CMD_ZONE_APPEND: + case NVME_CMD_COPY: status = NVME_WRITE_FAULT; break; default: From 381ab99d858709eab34b65d123a6356b8b1e87bd Mon Sep 17 00:00:00 2001 From: Minwoo Im Date: Tue, 18 Apr 2023 09:26:22 +0900 Subject: [PATCH 3/7] hw/nvme: check maximum copy length (MCL) for COPY MCL(Maximum Copy Length) in the Identify Namespace data structure limits the number of LBAs to be copied inside of the controller. We've not checked it at all, so added the check with returning the proper error status. Signed-off-by: Minwoo Im Reviewed-by: Klaus Jensen Signed-off-by: Klaus Jensen --- hw/nvme/ctrl.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index e031c2250a..355668bdf8 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -2855,6 +2855,25 @@ static void nvme_copy_source_range_parse(void *ranges, int idx, uint8_t format, } } +static inline uint16_t nvme_check_copy_mcl(NvmeNamespace *ns, + NvmeCopyAIOCB *iocb, uint16_t nr) +{ + uint32_t copy_len = 0; + + for (int idx = 0; idx < nr; idx++) { + uint32_t nlb; + nvme_copy_source_range_parse(iocb->ranges, idx, iocb->format, NULL, + &nlb, NULL, NULL, NULL); + copy_len += nlb + 1; + } + + if (copy_len > ns->id_ns.mcl) { + return NVME_CMD_SIZE_LIMIT | NVME_DNR; + } + + return NVME_SUCCESS; +} + static void nvme_copy_out_completed_cb(void *opaque, int ret) { NvmeCopyAIOCB *iocb = opaque; @@ -3167,6 +3186,11 @@ static uint16_t nvme_copy(NvmeCtrl *n, NvmeRequest *req) } } + status = nvme_check_copy_mcl(ns, iocb, nr); + if (status) { + goto invalid; + } + iocb->req = req; iocb->ret = 0; iocb->nr = nr; From 3ae8a54a087d54cfd109ab3d844ff4cba54a28d8 Mon Sep 17 00:00:00 2001 From: Klaus Jensen Date: Wed, 24 May 2023 11:28:34 +0200 Subject: [PATCH 4/7] hw/nvme: fix verification of number of ruhis Fix a off-by-one error when verifying the number of reclaim unit handle identifiers specified in fdp.ruhs. To make the fix nicer, move the verification of the fdp.nruh parameter to an earlier point. Fixes: 73064edfb864 ("hw/nvme: flexible data placement emulation") Reviewed-by: Jesper Wendel Devantier Signed-off-by: Klaus Jensen --- hw/nvme/ns.c | 4 +--- hw/nvme/subsys.c | 6 ++++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c index 547c0b1543..050fdaf50f 100644 --- a/hw/nvme/ns.c +++ b/hw/nvme/ns.c @@ -438,9 +438,7 @@ static bool nvme_ns_init_fdp(NvmeNamespace *ns, Error **errp) /* parse the placement handle identifiers */ while ((token = qemu_strsep(&p, ";")) != NULL) { - ns->fdp.nphs += 1; - if (ns->fdp.nphs > NVME_FDP_MAXPIDS || - ns->fdp.nphs == endgrp->fdp.nruh) { + if (ns->fdp.nphs++ == endgrp->fdp.nruh) { error_setg(errp, "too many placement handles"); free(r); return false; diff --git a/hw/nvme/subsys.c b/hw/nvme/subsys.c index 24ddec860e..d30bb8bfd5 100644 --- a/hw/nvme/subsys.c +++ b/hw/nvme/subsys.c @@ -158,8 +158,10 @@ static bool nvme_subsys_setup_fdp(NvmeSubsystem *subsys, Error **errp) endgrp->fdp.nrg = subsys->params.fdp.nrg; - if (!subsys->params.fdp.nruh) { - error_setg(errp, "fdp.nruh must be non-zero"); + if (!subsys->params.fdp.nruh || + subsys->params.fdp.nruh > NVME_FDP_MAXPIDS) { + error_setg(errp, "fdp.nruh must be non-zero and less than %u", + NVME_FDP_MAXPIDS); return false; } From 94fa8ca7ee9b6215a5c078f5d424377e5fa61b9c Mon Sep 17 00:00:00 2001 From: Klaus Jensen Date: Wed, 24 May 2023 11:45:04 +0200 Subject: [PATCH 5/7] hw/nvme: verify uniqueness of reclaim unit handle identifiers Verify that a reclaim unit handle identifier is only specified once in fdp.ruhs. Fixes: 73064edfb864 ("hw/nvme: flexible data placement emulation") Reviewed-by: Jesper Wendel Devantier Signed-off-by: Klaus Jensen --- hw/nvme/ns.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c index 050fdaf50f..c4ea2033bb 100644 --- a/hw/nvme/ns.c +++ b/hw/nvme/ns.c @@ -453,6 +453,17 @@ static bool nvme_ns_init_fdp(NvmeNamespace *ns, Error **errp) free(r); + /* verify that the ruhids are unique */ + for (unsigned int i = 0; i < ns->fdp.nphs; i++) { + for (unsigned int j = i + 1; j < ns->fdp.nphs; j++) { + if (ruhids[i] == ruhids[j]) { + error_setg(errp, "duplicate reclaim unit handle identifier: %u", + ruhids[i]); + return false; + } + } + } + ph = ns->fdp.phs = g_new(uint16_t, ns->fdp.nphs); ruhid = ruhids; From ce8017736cc82e0250c1bc8e383335822f995510 Mon Sep 17 00:00:00 2001 From: Klaus Jensen Date: Tue, 14 Feb 2023 10:16:56 +0100 Subject: [PATCH 6/7] hw/nvme: add placement handle list ranges Allow the placement handles to be specified as ranges, i.e. `fdp.ruhs=1:3-5` will attempt to assign ruh 1, 3, 4 and 5 to the namespace. Reviewed-by: Jesper Wendel Devantier Signed-off-by: Klaus Jensen --- hw/nvme/ns.c | 40 ++++++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c index c4ea2033bb..44aba8f4d9 100644 --- a/hw/nvme/ns.c +++ b/hw/nvme/ns.c @@ -400,8 +400,9 @@ static bool nvme_ns_init_fdp(NvmeNamespace *ns, Error **errp) NvmeRuHandle *ruh; uint8_t lbafi = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas); g_autofree unsigned int *ruhids = NULL; - unsigned int *ruhid; - char *r, *p, *token; + unsigned int n, m, *ruhid; + const char *endptr, *token; + char *r, *p; uint16_t *ph; if (!ns->params.fdp.ruhs) { @@ -438,16 +439,39 @@ static bool nvme_ns_init_fdp(NvmeNamespace *ns, Error **errp) /* parse the placement handle identifiers */ while ((token = qemu_strsep(&p, ";")) != NULL) { - if (ns->fdp.nphs++ == endgrp->fdp.nruh) { - error_setg(errp, "too many placement handles"); + if (qemu_strtoui(token, &endptr, 0, &n) < 0) { + error_setg(errp, "cannot parse reclaim unit handle identifier"); free(r); return false; } - if (qemu_strtoui(token, NULL, 0, ruhid++) < 0) { - error_setg(errp, "cannot parse reclaim unit handle identifier"); - free(r); - return false; + m = n; + + /* parse range */ + if (*endptr == '-') { + token = endptr + 1; + + if (qemu_strtoui(token, NULL, 0, &m) < 0) { + error_setg(errp, "cannot parse reclaim unit handle identifier"); + free(r); + return false; + } + + if (m < n) { + error_setg(errp, "invalid reclaim unit handle identifier range"); + free(r); + return false; + } + } + + for (; n <= m; n++) { + if (ns->fdp.nphs++ == endgrp->fdp.nruh) { + error_setg(errp, "too many placement handles"); + free(r); + return false; + } + + *ruhid++ = n; } } From e409c9057b55e890a6e5f70386a36932a5137bcf Mon Sep 17 00:00:00 2001 From: Klaus Jensen Date: Wed, 24 May 2023 13:06:37 +0200 Subject: [PATCH 7/7] docs: update hw/nvme documentation for TP4146 Update documentation for TP4146 ("Flexible Data Placement") emulation. Reviewed-by: Jesper Wendel Devantier Signed-off-by: Klaus Jensen --- docs/system/devices/nvme.rst | 37 +++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/docs/system/devices/nvme.rst b/docs/system/devices/nvme.rst index 30f841ef62..a8bb8d729c 100644 --- a/docs/system/devices/nvme.rst +++ b/docs/system/devices/nvme.rst @@ -212,6 +212,41 @@ The namespace may be configured with additional parameters the minimum memory page size (CAP.MPSMIN). The default value (``0``) has this property inherit the ``mdts`` value. +Flexible Data Placement +----------------------- + +The device may be configured to support TP4146 ("Flexible Data Placement") by +configuring it (``fdp=on``) on the subsystem:: + + -device nvme-subsys,id=nvme-subsys-0,nqn=subsys0,fdp=on,fdp.nruh=16 + +The subsystem emulates a single Endurance Group, on which Flexible Data +Placement will be supported. Also note that the device emulation deviates +slightly from the specification, by always enabling the "FDP Mode" feature on +the controller if the subsystems is configured for Flexible Data Placement. + +Enabling Flexible Data Placement on the subsyste enables the following +parameters: + +``fdp.nrg`` (default: ``1``) + Set the number of Reclaim Groups. + +``fdp.nruh`` (default: ``0``) + Set the number of Reclaim Unit Handles. This is a mandatory paramater and + must be non-zero. + +``fdp.runs`` (default: ``96M``) + Set the Reclaim Unit Nominal Size. Defaults to 96 MiB. + +Namespaces within this subsystem may requests Reclaim Unit Handles:: + + -device nvme-ns,drive=nvm-1,fdp.ruhs=RUHLIST + +The ``RUHLIST`` is a semicolon separated list (i.e. ``0;1;2;3``) and may +include ranges (i.e. ``0;8-15``). If no reclaim unit handle list is specified, +the controller will assign the controller-specified reclaim unit handle to +placement handle identifier 0. + Metadata -------- @@ -320,4 +355,4 @@ controller are: .. code-block:: console - echo 0000:01:00.1 > /sys/bus/pci/drivers/nvme/bind \ No newline at end of file + echo 0000:01:00.1 > /sys/bus/pci/drivers/nvme/bind