Message ID | 20210324200907.408996-8-its@irrelevant.dk |
---|---|
State | New |
Headers | show |
Series | hw/block/nvme: misc fixes | expand |
On Wed, Mar 24, 2021 at 09:09:07PM +0100, Klaus Jensen wrote: >From: Klaus Jensen <k.jensen@samsung.com> > >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), regardsless 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=' parametr) or implicitly >attaching to. > >Fixes: e570768566b3 ("hw/block/nvme: support for shared namespace in subsystem") >Cc: Minwoo Im <minwoo.im.dev@gmail.com> >Signed-off-by: Klaus Jensen <k.jensen@samsung.com> >--- > hw/block/nvme-ns.h | 10 ++-- > hw/block/nvme-subsys.h | 7 ++- > hw/block/nvme.h | 39 +------------- > include/block/nvme.h | 1 + > hw/block/nvme-ns.c | 74 +++++++++++++++++++++++---- > hw/block/nvme-subsys.c | 28 ----------- > hw/block/nvme.c | 112 +++++++++++++---------------------------- > hw/block/trace-events | 1 - > 8 files changed, 106 insertions(+), 166 deletions(-) > >diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h >index 82340c4b2574..fb0a41f912e7 100644 >--- a/hw/block/nvme-ns.h >+++ b/hw/block/nvme-ns.h >@@ -29,6 +29,7 @@ typedef struct NvmeZone { > > typedef struct NvmeNamespaceParams { > bool detached; >+ bool shared; > uint32_t nsid; > QemuUUID uuid; > >@@ -60,8 +61,8 @@ typedef struct NvmeNamespace { > const uint32_t *iocs; > uint8_t csi; > uint16_t status; >+ int attached; > >- NvmeSubsystem *subsys; > QTAILQ_ENTRY(NvmeNamespace) entry; > > NvmeIdNsZoned *id_ns_zoned; >@@ -99,11 +100,6 @@ static inline uint32_t nvme_nsid(NvmeNamespace *ns) > return 0; > } > >-static inline bool nvme_ns_shared(NvmeNamespace *ns) >-{ >- return !!ns->subsys; >-} >- > static inline NvmeLBAF *nvme_ns_lbaf(NvmeNamespace *ns) > { > NvmeIdNs *id_ns = &ns->id_ns; >@@ -225,7 +221,7 @@ static inline void nvme_aor_dec_active(NvmeNamespace *ns) > } > > void nvme_ns_init_format(NvmeNamespace *ns); >-int nvme_ns_setup(NvmeNamespace *ns, Error **errp); >+int nvme_ns_setup(NvmeCtrl *n, NvmeNamespace *ns, Error **errp); > void nvme_ns_drain(NvmeNamespace *ns); > void nvme_ns_shutdown(NvmeNamespace *ns); > void nvme_ns_cleanup(NvmeNamespace *ns); >diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h >index aafa04b84829..24132edd005c 100644 >--- a/hw/block/nvme-subsys.h >+++ b/hw/block/nvme-subsys.h >@@ -14,7 +14,7 @@ > OBJECT_CHECK(NvmeSubsystem, (obj), TYPE_NVME_SUBSYS) > > #define NVME_SUBSYS_MAX_CTRLS 32 >-#define NVME_SUBSYS_MAX_NAMESPACES 256 >+#define NVME_MAX_NAMESPACES 256 > > typedef struct NvmeCtrl NvmeCtrl; > typedef struct NvmeNamespace NvmeNamespace; >@@ -24,7 +24,7 @@ typedef struct NvmeSubsystem { > > NvmeCtrl *ctrls[NVME_SUBSYS_MAX_CTRLS]; > /* Allocated namespaces for this subsystem */ >- NvmeNamespace *namespaces[NVME_SUBSYS_MAX_NAMESPACES + 1]; >+ NvmeNamespace *namespaces[NVME_MAX_NAMESPACES + 1]; > > struct { > char *nqn; >@@ -32,7 +32,6 @@ typedef struct NvmeSubsystem { > } NvmeSubsystem; > > int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp); >-int nvme_subsys_register_ns(NvmeNamespace *ns, Error **errp); > > static inline NvmeCtrl *nvme_subsys_ctrl(NvmeSubsystem *subsys, > uint32_t cntlid) >@@ -54,7 +53,7 @@ static inline NvmeNamespace *nvme_subsys_ns(NvmeSubsystem *subsys, > return NULL; > } > >- assert(nsid && nsid <= NVME_SUBSYS_MAX_NAMESPACES); >+ assert(nsid && nsid <= NVME_MAX_NAMESPACES); > > return subsys->namespaces[nsid]; > } >diff --git a/hw/block/nvme.h b/hw/block/nvme.h >index 1570f65989a7..644143597a0f 100644 >--- a/hw/block/nvme.h >+++ b/hw/block/nvme.h >@@ -6,17 +6,9 @@ > #include "nvme-subsys.h" > #include "nvme-ns.h" > >-#define NVME_MAX_NAMESPACES 256 >- > #define NVME_DEFAULT_ZONE_SIZE (128 * MiB) > #define NVME_DEFAULT_MAX_ZA_SIZE (128 * KiB) > >-/* >- * Subsystem namespace list for allocated namespaces should be larger than >- * attached namespace list in a controller. >- */ >-QEMU_BUILD_BUG_ON(NVME_MAX_NAMESPACES > NVME_SUBSYS_MAX_NAMESPACES); >- > typedef struct NvmeParams { > char *serial; > uint32_t num_queues; /* deprecated since 5.1 */ >@@ -234,35 +226,6 @@ static inline NvmeNamespace *nvme_ns(NvmeCtrl *n, uint32_t nsid) > return n->namespaces[nsid]; > } > >-static inline bool nvme_ns_is_attached(NvmeCtrl *n, NvmeNamespace *ns) >-{ >- int nsid; >- >- for (nsid = 1; nsid <= n->num_namespaces; nsid++) { >- if (nvme_ns(n, nsid) == ns) { >- return true; >- } >- } >- >- return false; >-} >- >-static inline void nvme_ns_attach(NvmeCtrl *n, NvmeNamespace *ns) >-{ >- uint32_t nsid = nvme_nsid(ns); >- assert(nsid && nsid <= NVME_MAX_NAMESPACES); >- >- n->namespaces[nsid] = ns; >-} >- >-static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns) >-{ >- uint32_t nsid = nvme_nsid(ns); >- assert(nsid && nsid <= NVME_MAX_NAMESPACES); >- >- n->namespaces[nsid] = NULL; >-} >- > static inline NvmeCQueue *nvme_cq(NvmeRequest *req) > { > NvmeSQueue *sq = req->sq; >@@ -291,7 +254,7 @@ typedef enum NvmeTxDirection { > NVME_TX_DIRECTION_FROM_DEVICE = 1, > } NvmeTxDirection; > >-int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp); >+void nvme_attach_ns(NvmeCtrl *n, NvmeNamespace *ns); > uint16_t nvme_bounce_data(NvmeCtrl *n, uint8_t *ptr, uint32_t len, > NvmeTxDirection dir, NvmeRequest *req); > uint16_t nvme_bounce_mdata(NvmeCtrl *n, uint8_t *ptr, uint32_t len, >diff --git a/include/block/nvme.h b/include/block/nvme.h >index b0a4e4291611..4ac926fbc687 100644 >--- a/include/block/nvme.h >+++ b/include/block/nvme.h >@@ -847,6 +847,7 @@ enum NvmeStatusCodes { > NVME_FEAT_NOT_NS_SPEC = 0x010f, > NVME_FW_REQ_SUSYSTEM_RESET = 0x0110, > NVME_NS_ALREADY_ATTACHED = 0x0118, >+ NVME_NS_PRIVATE = 0x0119, > NVME_NS_NOT_ATTACHED = 0x011A, > NVME_NS_CTRL_LIST_INVALID = 0x011C, > NVME_CONFLICTING_ATTRS = 0x0180, >diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c >index ca04ee1bacfb..aee43ba0b873 100644 >--- a/hw/block/nvme-ns.c >+++ b/hw/block/nvme-ns.c >@@ -73,7 +73,7 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp) > /* support DULBE and I/O optimization fields */ > id_ns->nsfeat |= (0x4 | 0x10); > >- if (nvme_ns_shared(ns)) { >+ if (ns->params.shared) { > id_ns->nmic |= NVME_NMIC_NS_SHARED; > } > >@@ -387,7 +387,8 @@ static void nvme_zoned_ns_shutdown(NvmeNamespace *ns) > assert(ns->nr_open_zones == 0); > } > >-static int nvme_ns_check_constraints(NvmeNamespace *ns, Error **errp) >+static int nvme_ns_check_constraints(NvmeCtrl *n, NvmeNamespace *ns, >+ Error **errp) > { > if (!ns->blkconf.blk) { > error_setg(errp, "block backend not configured"); >@@ -400,12 +401,32 @@ static int nvme_ns_check_constraints(NvmeNamespace *ns, Error **errp) > return -1; > } > >+ if (ns->params.nsid > NVME_MAX_NAMESPACES) { >+ error_setg(errp, "invalid namespace id (must be between 0 and %d)", >+ NVME_MAX_NAMESPACES); >+ return -1; >+ } >+ >+ if (!n->subsys) { >+ if (ns->params.detached) { >+ error_setg(errp, "detached requires that the nvme device is " >+ "linked to an nvme-subsys device"); >+ return -1; >+ } >+ >+ if (ns->params.shared) { >+ error_setg(errp, "shared requires that the nvme device is " >+ "linked to an nvme-subsys device"); >+ return -1; >+ } >+ } >+ > return 0; > } > >-int nvme_ns_setup(NvmeNamespace *ns, Error **errp) >+int nvme_ns_setup(NvmeCtrl *n, NvmeNamespace *ns, Error **errp) > { >- if (nvme_ns_check_constraints(ns, errp)) { >+ if (nvme_ns_check_constraints(n, ns, errp)) { > return -1; > } > >@@ -453,27 +474,60 @@ static void nvme_ns_realize(DeviceState *dev, Error **errp) > NvmeNamespace *ns = NVME_NS(dev); > BusState *s = qdev_get_parent_bus(dev); > NvmeCtrl *n = NVME(s->parent); >+ NvmeSubsystem *subsys = n->subsys; >+ uint32_t nsid = ns->params.nsid; >+ int i; > >- if (nvme_ns_setup(ns, errp)) { >+ if (nvme_ns_setup(n, ns, errp)) { > return; > } > >- if (ns->subsys) { >- if (nvme_subsys_register_ns(ns, errp)) { >+ if (!nsid) { >+ for (i = 1; i <= NVME_MAX_NAMESPACES; i++) { >+ if (!nvme_ns(n, i) || nvme_subsys_ns(subsys, i)) { >+ nsid = ns->params.nsid = i; >+ break; >+ } >+ } >+ >+ if (!nsid) { >+ error_setg(errp, "no free namespace id"); > return; > } > } else { >- if (nvme_register_namespace(n, ns, errp)) { >+ if (nvme_ns(n, nsid) || nvme_subsys_ns(subsys, nsid)) { >+ error_setg(errp, "namespace id '%d' already allocated", nsid); > return; > } > } >+ >+ if (subsys) { >+ subsys->namespaces[nsid] = ns; >+ >+ if (ns->params.detached) { >+ return; >+ } >+ >+ if (ns->params.shared) { >+ for (i = 0; i < ARRAY_SIZE(subsys->ctrls); i++) { >+ NvmeCtrl *ctrl = subsys->ctrls[i]; >+ >+ if (ctrl) { >+ nvme_attach_ns(ctrl, ns); >+ } >+ } >+ >+ return; >+ } >+ } >+ >+ nvme_attach_ns(n, ns); > } > > static Property nvme_ns_props[] = { > DEFINE_BLOCK_PROPERTIES(NvmeNamespace, blkconf), >- DEFINE_PROP_LINK("subsys", NvmeNamespace, subsys, TYPE_NVME_SUBSYS, >- NvmeSubsystem *), > DEFINE_PROP_BOOL("detached", NvmeNamespace, params.detached, false), >+ DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, false)i, Nice change point, hope we need update the usage, removing "subsys" from nvme-ns device params and adding "shared" param? > DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0), > DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid), > DEFINE_PROP_UINT16("ms", NvmeNamespace, params.ms, 0), >diff --git a/hw/block/nvme-subsys.c b/hw/block/nvme-subsys.c >index 9fadef8cec99..283a97b79d57 100644 >--- a/hw/block/nvme-subsys.c >+++ b/hw/block/nvme-subsys.c >@@ -43,34 +43,6 @@ int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp) > return cntlid; > } > >-int nvme_subsys_register_ns(NvmeNamespace *ns, Error **errp) >-{ >- NvmeSubsystem *subsys = ns->subsys; >- NvmeCtrl *n; >- uint32_t nsid = nvme_nsid(ns); >- int i; >- >- assert(nsid && nsid <= NVME_SUBSYS_MAX_NAMESPACES); >- >- if (subsys->namespaces[nsid]) { >- error_setg(errp, "namespace %d already registerd to subsy %s", >- nvme_nsid(ns), subsys->parent_obj.id); >- return -1; >- } >- >- subsys->namespaces[nsid] = ns; >- >- for (i = 0; i < ARRAY_SIZE(subsys->ctrls); i++) { >- n = subsys->ctrls[i]; >- >- if (n && nvme_register_namespace(n, ns, errp)) { >- return -1; >- } >- } >- >- return 0; >-} >- > static void nvme_subsys_setup(NvmeSubsystem *subsys) > { > const char *nqn = subsys->params.nqn ? >diff --git a/hw/block/nvme.c b/hw/block/nvme.c >index e84e43b2692d..734a80dbd540 100644 >--- a/hw/block/nvme.c >+++ b/hw/block/nvme.c >@@ -4250,7 +4250,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; > } > >@@ -4907,6 +4907,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; >@@ -4928,18 +4932,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] = NULL; >+ ns->attached--; > > __nvme_update_dmrsl(ctrl); > } >@@ -5827,6 +5836,12 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp) > if (n->namespace.blkconf.blk) { > warn_report("drive property is deprecated; " > "please use an nvme-ns device instead"); >+ >+ if (n->subsys) { >+ error_setg(errp, "subsystem support is unavailable with legacy " >+ "namespace ('drive' property)"); >+ return; >+ } > } > > if (params->max_ioqpairs < 1 || >@@ -5889,75 +5904,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]) { >- 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; >@@ -6187,6 +6133,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] = 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); >@@ -6218,13 +6176,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); > } > } > >diff --git a/hw/block/trace-events b/hw/block/trace-events >index 22da06986d72..fa12e3a67a75 100644 >--- a/hw/block/trace-events >+++ b/hw/block/trace-events >@@ -51,7 +51,6 @@ hd_geometry_guess(void *blk, uint32_t cyls, uint32_t heads, uint32_t secs, int t > > # nvme.c > # nvme traces for successful events >-pci_nvme_register_namespace(uint32_t nsid) "nsid %"PRIu32"" > pci_nvme_irq_msix(uint32_t vector) "raising MSI-X IRQ vector %u" > pci_nvme_irq_pin(void) "pulsing IRQ pin" > pci_nvme_irq_masked(void) "IRQ is masked" >-- Changes looks good to me. Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com> >2.31.0 > >
On Apr 5 18:02, Gollu Appalanaidu wrote: > On Wed, Mar 24, 2021 at 09:09:07PM +0100, Klaus Jensen wrote: > > From: Klaus Jensen <k.jensen@samsung.com> > > > > 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), regardsless 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=' parametr) or implicitly > > attaching to. > > > > Fixes: e570768566b3 ("hw/block/nvme: support for shared namespace in subsystem") > > Cc: Minwoo Im <minwoo.im.dev@gmail.com> > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > > --- > > hw/block/nvme-ns.h | 10 ++-- > > hw/block/nvme-subsys.h | 7 ++- > > hw/block/nvme.h | 39 +------------- > > include/block/nvme.h | 1 + > > hw/block/nvme-ns.c | 74 +++++++++++++++++++++++---- > > hw/block/nvme-subsys.c | 28 ----------- > > hw/block/nvme.c | 112 +++++++++++++---------------------------- > > hw/block/trace-events | 1 - > > 8 files changed, 106 insertions(+), 166 deletions(-) > > <snip> > > > > static Property nvme_ns_props[] = { > > DEFINE_BLOCK_PROPERTIES(NvmeNamespace, blkconf), > > - DEFINE_PROP_LINK("subsys", NvmeNamespace, subsys, TYPE_NVME_SUBSYS, > > - NvmeSubsystem *), > > DEFINE_PROP_BOOL("detached", NvmeNamespace, params.detached, false), > > + DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, false)i, > > Nice change point, hope we need update the usage, removing "subsys" from > nvme-ns device params and adding "shared" param? > Good catch, thanks. Fixing that up for a v2.
diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h index 82340c4b2574..fb0a41f912e7 100644 --- a/hw/block/nvme-ns.h +++ b/hw/block/nvme-ns.h @@ -29,6 +29,7 @@ typedef struct NvmeZone { typedef struct NvmeNamespaceParams { bool detached; + bool shared; uint32_t nsid; QemuUUID uuid; @@ -60,8 +61,8 @@ typedef struct NvmeNamespace { const uint32_t *iocs; uint8_t csi; uint16_t status; + int attached; - NvmeSubsystem *subsys; QTAILQ_ENTRY(NvmeNamespace) entry; NvmeIdNsZoned *id_ns_zoned; @@ -99,11 +100,6 @@ static inline uint32_t nvme_nsid(NvmeNamespace *ns) return 0; } -static inline bool nvme_ns_shared(NvmeNamespace *ns) -{ - return !!ns->subsys; -} - static inline NvmeLBAF *nvme_ns_lbaf(NvmeNamespace *ns) { NvmeIdNs *id_ns = &ns->id_ns; @@ -225,7 +221,7 @@ static inline void nvme_aor_dec_active(NvmeNamespace *ns) } void nvme_ns_init_format(NvmeNamespace *ns); -int nvme_ns_setup(NvmeNamespace *ns, Error **errp); +int nvme_ns_setup(NvmeCtrl *n, NvmeNamespace *ns, Error **errp); void nvme_ns_drain(NvmeNamespace *ns); void nvme_ns_shutdown(NvmeNamespace *ns); void nvme_ns_cleanup(NvmeNamespace *ns); diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h index aafa04b84829..24132edd005c 100644 --- a/hw/block/nvme-subsys.h +++ b/hw/block/nvme-subsys.h @@ -14,7 +14,7 @@ OBJECT_CHECK(NvmeSubsystem, (obj), TYPE_NVME_SUBSYS) #define NVME_SUBSYS_MAX_CTRLS 32 -#define NVME_SUBSYS_MAX_NAMESPACES 256 +#define NVME_MAX_NAMESPACES 256 typedef struct NvmeCtrl NvmeCtrl; typedef struct NvmeNamespace NvmeNamespace; @@ -24,7 +24,7 @@ typedef struct NvmeSubsystem { NvmeCtrl *ctrls[NVME_SUBSYS_MAX_CTRLS]; /* Allocated namespaces for this subsystem */ - NvmeNamespace *namespaces[NVME_SUBSYS_MAX_NAMESPACES + 1]; + NvmeNamespace *namespaces[NVME_MAX_NAMESPACES + 1]; struct { char *nqn; @@ -32,7 +32,6 @@ typedef struct NvmeSubsystem { } NvmeSubsystem; int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp); -int nvme_subsys_register_ns(NvmeNamespace *ns, Error **errp); static inline NvmeCtrl *nvme_subsys_ctrl(NvmeSubsystem *subsys, uint32_t cntlid) @@ -54,7 +53,7 @@ static inline NvmeNamespace *nvme_subsys_ns(NvmeSubsystem *subsys, return NULL; } - assert(nsid && nsid <= NVME_SUBSYS_MAX_NAMESPACES); + assert(nsid && nsid <= NVME_MAX_NAMESPACES); return subsys->namespaces[nsid]; } diff --git a/hw/block/nvme.h b/hw/block/nvme.h index 1570f65989a7..644143597a0f 100644 --- a/hw/block/nvme.h +++ b/hw/block/nvme.h @@ -6,17 +6,9 @@ #include "nvme-subsys.h" #include "nvme-ns.h" -#define NVME_MAX_NAMESPACES 256 - #define NVME_DEFAULT_ZONE_SIZE (128 * MiB) #define NVME_DEFAULT_MAX_ZA_SIZE (128 * KiB) -/* - * Subsystem namespace list for allocated namespaces should be larger than - * attached namespace list in a controller. - */ -QEMU_BUILD_BUG_ON(NVME_MAX_NAMESPACES > NVME_SUBSYS_MAX_NAMESPACES); - typedef struct NvmeParams { char *serial; uint32_t num_queues; /* deprecated since 5.1 */ @@ -234,35 +226,6 @@ static inline NvmeNamespace *nvme_ns(NvmeCtrl *n, uint32_t nsid) return n->namespaces[nsid]; } -static inline bool nvme_ns_is_attached(NvmeCtrl *n, NvmeNamespace *ns) -{ - int nsid; - - for (nsid = 1; nsid <= n->num_namespaces; nsid++) { - if (nvme_ns(n, nsid) == ns) { - return true; - } - } - - return false; -} - -static inline void nvme_ns_attach(NvmeCtrl *n, NvmeNamespace *ns) -{ - uint32_t nsid = nvme_nsid(ns); - assert(nsid && nsid <= NVME_MAX_NAMESPACES); - - n->namespaces[nsid] = ns; -} - -static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns) -{ - uint32_t nsid = nvme_nsid(ns); - assert(nsid && nsid <= NVME_MAX_NAMESPACES); - - n->namespaces[nsid] = NULL; -} - static inline NvmeCQueue *nvme_cq(NvmeRequest *req) { NvmeSQueue *sq = req->sq; @@ -291,7 +254,7 @@ typedef enum NvmeTxDirection { NVME_TX_DIRECTION_FROM_DEVICE = 1, } NvmeTxDirection; -int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp); +void nvme_attach_ns(NvmeCtrl *n, NvmeNamespace *ns); uint16_t nvme_bounce_data(NvmeCtrl *n, uint8_t *ptr, uint32_t len, NvmeTxDirection dir, NvmeRequest *req); uint16_t nvme_bounce_mdata(NvmeCtrl *n, uint8_t *ptr, uint32_t len, diff --git a/include/block/nvme.h b/include/block/nvme.h index b0a4e4291611..4ac926fbc687 100644 --- a/include/block/nvme.h +++ b/include/block/nvme.h @@ -847,6 +847,7 @@ enum NvmeStatusCodes { NVME_FEAT_NOT_NS_SPEC = 0x010f, NVME_FW_REQ_SUSYSTEM_RESET = 0x0110, NVME_NS_ALREADY_ATTACHED = 0x0118, + NVME_NS_PRIVATE = 0x0119, NVME_NS_NOT_ATTACHED = 0x011A, NVME_NS_CTRL_LIST_INVALID = 0x011C, NVME_CONFLICTING_ATTRS = 0x0180, diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c index ca04ee1bacfb..aee43ba0b873 100644 --- a/hw/block/nvme-ns.c +++ b/hw/block/nvme-ns.c @@ -73,7 +73,7 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp) /* support DULBE and I/O optimization fields */ id_ns->nsfeat |= (0x4 | 0x10); - if (nvme_ns_shared(ns)) { + if (ns->params.shared) { id_ns->nmic |= NVME_NMIC_NS_SHARED; } @@ -387,7 +387,8 @@ static void nvme_zoned_ns_shutdown(NvmeNamespace *ns) assert(ns->nr_open_zones == 0); } -static int nvme_ns_check_constraints(NvmeNamespace *ns, Error **errp) +static int nvme_ns_check_constraints(NvmeCtrl *n, NvmeNamespace *ns, + Error **errp) { if (!ns->blkconf.blk) { error_setg(errp, "block backend not configured"); @@ -400,12 +401,32 @@ static int nvme_ns_check_constraints(NvmeNamespace *ns, Error **errp) return -1; } + if (ns->params.nsid > NVME_MAX_NAMESPACES) { + error_setg(errp, "invalid namespace id (must be between 0 and %d)", + NVME_MAX_NAMESPACES); + return -1; + } + + if (!n->subsys) { + if (ns->params.detached) { + error_setg(errp, "detached requires that the nvme device is " + "linked to an nvme-subsys device"); + return -1; + } + + if (ns->params.shared) { + error_setg(errp, "shared requires that the nvme device is " + "linked to an nvme-subsys device"); + return -1; + } + } + return 0; } -int nvme_ns_setup(NvmeNamespace *ns, Error **errp) +int nvme_ns_setup(NvmeCtrl *n, NvmeNamespace *ns, Error **errp) { - if (nvme_ns_check_constraints(ns, errp)) { + if (nvme_ns_check_constraints(n, ns, errp)) { return -1; } @@ -453,27 +474,60 @@ static void nvme_ns_realize(DeviceState *dev, Error **errp) NvmeNamespace *ns = NVME_NS(dev); BusState *s = qdev_get_parent_bus(dev); NvmeCtrl *n = NVME(s->parent); + NvmeSubsystem *subsys = n->subsys; + uint32_t nsid = ns->params.nsid; + int i; - if (nvme_ns_setup(ns, errp)) { + if (nvme_ns_setup(n, ns, errp)) { return; } - if (ns->subsys) { - if (nvme_subsys_register_ns(ns, errp)) { + if (!nsid) { + for (i = 1; i <= NVME_MAX_NAMESPACES; i++) { + if (!nvme_ns(n, i) || nvme_subsys_ns(subsys, i)) { + nsid = ns->params.nsid = i; + break; + } + } + + if (!nsid) { + error_setg(errp, "no free namespace id"); return; } } else { - if (nvme_register_namespace(n, ns, errp)) { + if (nvme_ns(n, nsid) || nvme_subsys_ns(subsys, nsid)) { + error_setg(errp, "namespace id '%d' already allocated", nsid); return; } } + + if (subsys) { + subsys->namespaces[nsid] = ns; + + if (ns->params.detached) { + return; + } + + if (ns->params.shared) { + for (i = 0; i < ARRAY_SIZE(subsys->ctrls); i++) { + NvmeCtrl *ctrl = subsys->ctrls[i]; + + if (ctrl) { + nvme_attach_ns(ctrl, ns); + } + } + + return; + } + } + + nvme_attach_ns(n, ns); } static Property nvme_ns_props[] = { DEFINE_BLOCK_PROPERTIES(NvmeNamespace, blkconf), - DEFINE_PROP_LINK("subsys", NvmeNamespace, subsys, TYPE_NVME_SUBSYS, - NvmeSubsystem *), DEFINE_PROP_BOOL("detached", NvmeNamespace, params.detached, false), + DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, false), DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0), DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid), DEFINE_PROP_UINT16("ms", NvmeNamespace, params.ms, 0), diff --git a/hw/block/nvme-subsys.c b/hw/block/nvme-subsys.c index 9fadef8cec99..283a97b79d57 100644 --- a/hw/block/nvme-subsys.c +++ b/hw/block/nvme-subsys.c @@ -43,34 +43,6 @@ int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp) return cntlid; } -int nvme_subsys_register_ns(NvmeNamespace *ns, Error **errp) -{ - NvmeSubsystem *subsys = ns->subsys; - NvmeCtrl *n; - uint32_t nsid = nvme_nsid(ns); - int i; - - assert(nsid && nsid <= NVME_SUBSYS_MAX_NAMESPACES); - - if (subsys->namespaces[nsid]) { - error_setg(errp, "namespace %d already registerd to subsy %s", - nvme_nsid(ns), subsys->parent_obj.id); - return -1; - } - - subsys->namespaces[nsid] = ns; - - for (i = 0; i < ARRAY_SIZE(subsys->ctrls); i++) { - n = subsys->ctrls[i]; - - if (n && nvme_register_namespace(n, ns, errp)) { - return -1; - } - } - - return 0; -} - static void nvme_subsys_setup(NvmeSubsystem *subsys) { const char *nqn = subsys->params.nqn ? diff --git a/hw/block/nvme.c b/hw/block/nvme.c index e84e43b2692d..734a80dbd540 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -4250,7 +4250,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; } @@ -4907,6 +4907,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; @@ -4928,18 +4932,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] = NULL; + ns->attached--; __nvme_update_dmrsl(ctrl); } @@ -5827,6 +5836,12 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp) if (n->namespace.blkconf.blk) { warn_report("drive property is deprecated; " "please use an nvme-ns device instead"); + + if (n->subsys) { + error_setg(errp, "subsystem support is unavailable with legacy " + "namespace ('drive' property)"); + return; + } } if (params->max_ioqpairs < 1 || @@ -5889,75 +5904,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]) { - 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; @@ -6187,6 +6133,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] = 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); @@ -6218,13 +6176,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); } } diff --git a/hw/block/trace-events b/hw/block/trace-events index 22da06986d72..fa12e3a67a75 100644 --- a/hw/block/trace-events +++ b/hw/block/trace-events @@ -51,7 +51,6 @@ hd_geometry_guess(void *blk, uint32_t cyls, uint32_t heads, uint32_t secs, int t # nvme.c # nvme traces for successful events -pci_nvme_register_namespace(uint32_t nsid) "nsid %"PRIu32"" pci_nvme_irq_msix(uint32_t vector) "raising MSI-X IRQ vector %u" pci_nvme_irq_pin(void) "pulsing IRQ pin" pci_nvme_irq_masked(void) "IRQ is masked"