From e5489356349a400774c1bda60dbf78cff211e009 Mon Sep 17 00:00:00 2001 From: Klaus Jensen Date: Tue, 23 Mar 2021 12:43:24 +0100 Subject: hw/block/nvme: fix handling of private namespaces Prior to this patch, if a private nvme-ns device (that is, a namespace that is not linked to a subsystem) is wired up to an nvme-subsys linked nvme controller device, the device fails to verify that the namespace id is unique within the subsystem. NVM Express v1.4b, Section 6.1.6 ("NSID and Namespace Usage") states that because the device supports Namespace Management, "NSIDs *shall* be unique within the NVM subsystem". Additionally, prior to this patch, private namespaces are not known to the subsystem and the namespace is considered exclusive to the controller with which it is initially wired up to. However, this is not the definition of a private namespace; per Section 1.6.33 ("private namespace"), a private namespace is just a namespace that does not support multipath I/O or namespace sharing, which means "that it is only able to be attached to one controller at a time". Fix this by always allocating namespaces in the subsystem (if one is linked to the controller), regardless of the shared/private status of the namespace. Whether or not the namespace is shareable is controlled by a new `shared` nvme-ns parameter. Finally, this fix allows the nvme-ns `subsys` parameter to be removed, since the `shared` parameter now serves the purpose of attaching the namespace to all controllers in the subsystem upon device realization. It is invalid to have an nvme-ns namespace device with a linked subsystem without the parent nvme controller device also being linked to one and since the nvme-ns devices will unconditionally be "attached" (in QEMU terms that is) to an nvme controller device through an NvmeBus, the nvme-ns namespace device can always get a reference to the subsystem of the controller it is explicitly (using 'bus=' parameter) or implicitly attaching to. Fixes: e570768566b3 ("hw/block/nvme: support for shared namespace in subsystem") Cc: Minwoo Im Signed-off-by: Klaus Jensen Reviewed-by: Gollu Appalanaidu Reviewed-by: Keith Busch Reviewed-by: Minwoo Im --- hw/block/nvme.c | 123 +++++++++++++++++++------------------------------------- 1 file changed, 41 insertions(+), 82 deletions(-) (limited to 'hw/block/nvme.c') diff --git a/hw/block/nvme.c b/hw/block/nvme.c index 0e5ba27c46..82b3d453f5 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -93,10 +93,13 @@ * * nvme namespace device parameters * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - * - `subsys` - * If given, the namespace will be attached to all controllers in the - * subsystem. Otherwise, `bus` must be given to attach this namespace to a - * specific controller as a non-shared namespace. + * - `shared` + * When the parent nvme device (as defined explicitly by the 'bus' parameter + * or implicitly by the most recently defined NvmeBus) is linked to an + * nvme-subsys device, the namespace will be attached to all controllers in + * the subsystem. If set to 'off' (the default), the namespace will remain a + * private namespace and may only be attached to a single controller at a + * time. * * - `detached` * This parameter is only valid together with the `subsys` parameter. If left @@ -4242,7 +4245,7 @@ static uint16_t nvme_identify_ns_attached_list(NvmeCtrl *n, NvmeRequest *req) continue; } - if (!nvme_ns_is_attached(ctrl, ns)) { + if (!nvme_ns(ctrl, c->nsid)) { continue; } @@ -4899,6 +4902,10 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req) trace_pci_nvme_ns_attachment(nvme_cid(req), dw10 & 0xf); + if (!nvme_nsid_valid(n, nsid)) { + return NVME_INVALID_NSID | NVME_DNR; + } + ns = nvme_subsys_ns(n->subsys, nsid); if (!ns) { return NVME_INVALID_FIELD | NVME_DNR; @@ -4920,18 +4927,23 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req) } if (attach) { - if (nvme_ns_is_attached(ctrl, ns)) { + if (nvme_ns(ctrl, nsid)) { return NVME_NS_ALREADY_ATTACHED | NVME_DNR; } - nvme_ns_attach(ctrl, ns); + if (ns->attached && !ns->params.shared) { + return NVME_NS_PRIVATE | NVME_DNR; + } + + nvme_attach_ns(ctrl, ns); __nvme_select_ns_iocs(ctrl, ns); } else { - if (!nvme_ns_is_attached(ctrl, ns)) { + if (!nvme_ns(ctrl, nsid)) { return NVME_NS_NOT_ATTACHED | NVME_DNR; } - nvme_ns_detach(ctrl, ns); + ctrl->namespaces[nsid - 1] = NULL; + ns->attached--; nvme_update_dmrsl(ctrl); } @@ -5822,6 +5834,12 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp) params->max_ioqpairs = params->num_queues - 1; } + if (n->namespace.blkconf.blk && n->subsys) { + error_setg(errp, "subsystem support is unavailable with legacy " + "namespace ('drive' property)"); + return; + } + if (params->max_ioqpairs < 1 || params->max_ioqpairs > NVME_MAX_IOQPAIRS) { error_setg(errp, "max_ioqpairs must be between 1 and %d", @@ -5882,75 +5900,6 @@ static void nvme_init_state(NvmeCtrl *n) n->aer_reqs = g_new0(NvmeRequest *, n->params.aerl + 1); } -static int nvme_attach_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp) -{ - if (nvme_ns_is_attached(n, ns)) { - error_setg(errp, - "namespace %d is already attached to controller %d", - nvme_nsid(ns), n->cntlid); - return -1; - } - - nvme_ns_attach(n, ns); - - return 0; -} - -int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp) -{ - uint32_t nsid = nvme_nsid(ns); - - if (nsid > NVME_MAX_NAMESPACES) { - error_setg(errp, "invalid namespace id (must be between 0 and %d)", - NVME_MAX_NAMESPACES); - return -1; - } - - if (!nsid) { - for (int i = 1; i <= n->num_namespaces; i++) { - if (!nvme_ns(n, i)) { - nsid = ns->params.nsid = i; - break; - } - } - - if (!nsid) { - error_setg(errp, "no free namespace id"); - return -1; - } - } else { - if (n->namespaces[nsid - 1]) { - error_setg(errp, "namespace id '%d' is already in use", nsid); - return -1; - } - } - - trace_pci_nvme_register_namespace(nsid); - - /* - * If subsys is not given, namespae is always attached to the controller - * because there's no subsystem to manage namespace allocation. - */ - if (!n->subsys) { - if (ns->params.detached) { - error_setg(errp, - "detached needs nvme-subsys specified nvme or nvme-ns"); - return -1; - } - - return nvme_attach_namespace(n, ns, errp); - } else { - if (!ns->params.detached) { - return nvme_attach_namespace(n, ns, errp); - } - } - - n->dmrsl = MIN_NON_ZERO(n->dmrsl, - BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1)); - - return 0; -} - static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev) { uint64_t cmb_size = n->params.cmb_size_mb * MiB; @@ -6180,6 +6129,18 @@ static int nvme_init_subsys(NvmeCtrl *n, Error **errp) return 0; } +void nvme_attach_ns(NvmeCtrl *n, NvmeNamespace *ns) +{ + uint32_t nsid = ns->params.nsid; + assert(nsid && nsid <= NVME_MAX_NAMESPACES); + + n->namespaces[nsid - 1] = ns; + ns->attached++; + + n->dmrsl = MIN_NON_ZERO(n->dmrsl, + BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1)); +} + static void nvme_realize(PCIDevice *pci_dev, Error **errp) { NvmeCtrl *n = NVME(pci_dev); @@ -6211,13 +6172,11 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) ns = &n->namespace; ns->params.nsid = 1; - if (nvme_ns_setup(ns, errp)) { + if (nvme_ns_setup(n, ns, errp)) { return; } - if (nvme_register_namespace(n, ns, errp)) { - return; - } + nvme_attach_ns(n, ns); } } -- cgit v1.2.3