RE: [PATCH v4 1/2] hw/nvme: Support for Namespaces Management from guest OS - create-ns
Hi Keith, Thank you for the review and I agree with you, the issues will be addressed. Regards, Michael -Original Message- From: Keith Busch Sent: Wednesday, December 28, 2022 12:10 PM To: Jonathan Derrick Cc: qemu-de...@nongnu.org; Michael Kropaczek (CW) ; qemu-block@nongnu.org; Klaus Jensen ; Kevin Wolf ; Hanna Reitz Subject: Re: [PATCH v4 1/2] hw/nvme: Support for Namespaces Management from guest OS - create-ns [You don't often get email from kbu...@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] Caution: External Email On Wed, Dec 28, 2022 at 01:41:40PM -0600, Jonathan Derrick wrote: > +static uint16_t nvme_ns_mgmt(NvmeCtrl *n, NvmeRequest *req) { > +NvmeCtrl *n_p = NULL; /* primary controller */ > +NvmeIdCtrl *id = &n->id_ctrl; > +NvmeNamespace *ns; > +NvmeIdNsMgmt id_ns = {}; > +uint8_t flags = req->cmd.flags; > +uint32_t nsid = le32_to_cpu(req->cmd.nsid); > +uint32_t dw10 = le32_to_cpu(req->cmd.cdw10); > +uint32_t dw11 = le32_to_cpu(req->cmd.cdw11); > +uint8_t sel = dw10 & 0xf; > +uint8_t csi = (dw11 >> 24) & 0xf; > +uint16_t i; > +uint16_t ret; > +Error *local_err = NULL; > + > +trace_pci_nvme_ns_mgmt(nvme_cid(req), nsid, sel, csi, > + NVME_CMD_FLAGS_PSDT(flags)); > + > +if (!(le16_to_cpu(id->oacs) & NVME_OACS_NS_MGMT)) { > +return NVME_NS_ATTACH_MGMT_NOTSPRD | NVME_DNR; > +} > + > +if (n->cntlid && !n->subsys) { > +error_setg(&local_err, "Secondary controller without subsystem"); > +return NVME_NS_ATTACH_MGMT_NOTSPRD | NVME_DNR; Leaking local_err. Any time you call error_setg(), the error needs to be reported or freed at some point. > +} > + > +n_p = n->subsys->ctrls[0]; > + > +switch (sel) { > +case NVME_NS_MANAGEMENT_CREATE: > +switch (csi) { > +case NVME_CSI_NVM: The following case is sufficiently large enough that the implementation should be its own function. > +if (nsid) { > +return NVME_INVALID_FIELD | NVME_DNR; > +} > + > +ret = nvme_h2c(n, (uint8_t *)&id_ns, sizeof(id_ns), req); > +if (ret) { > +return ret; > +} > + > +uint64_t nsze = le64_to_cpu(id_ns.nsze); > +uint64_t ncap = le64_to_cpu(id_ns.ncap); Please don't mix declarations with code; declare these local variables at the top of the scope. > + > +if (ncap > nsze) { > +return NVME_INVALID_FIELD | NVME_DNR; > +} else if (ncap != nsze) { > +return NVME_THIN_PROVISION_NOTSPRD | NVME_DNR; > +} > + > +nvme_validate_flbas(id_ns.flbas, &local_err); > +if (local_err) { > +error_report_err(local_err); > +return NVME_INVALID_FORMAT | NVME_DNR; > +} > + > +for (i = 1; i <= NVME_MAX_NAMESPACES; i++) { > +if (nvme_ns(n_p, (uint32_t)i) || nvme_subsys_ns(n_p->subsys, > (uint32_t)i)) { > +continue; > +} > +break; > +} > + > + > +if (i > le32_to_cpu(n_p->id_ctrl.nn) || i > > NVME_MAX_NAMESPACES) { > + return NVME_NS_IDNTIFIER_UNAVAIL | NVME_DNR; > +} > +nsid = i; > + > +/* create ns here */ > +ns = nvme_ns_mgmt_create(n, nsid, &id_ns, &local_err); > +if (!ns || local_err) { > +if (local_err) { > +error_report_err(local_err); > +} > +return NVME_INVALID_FIELD | NVME_DNR; > +} > + > +if (nvme_cfg_update(n, ns->size, NVME_NS_ALLOC_CHK)) { > +/* place for delete-ns */ > +error_setg(&local_err, "Insufficient capacity, an orphaned > ns[%"PRIu32"] will be left behind", nsid); > +return NVME_NS_INSUFFICIENT_CAPAC | NVME_DNR; Leaked local_err. > +} > +(void)nvme_cfg_update(n, ns->size, NVME_NS_ALLOC); > +if (nvme_cfg_save(n)) { > +(void)nvme_cfg_update(n, ns->size, NVME_NS_DEALLOC); > +/* place for delete-ns */ > +error_setg(&local_err, "Cannot save conf file, an orphaned > ns[%"PRIu32"] will be left behind", nsid); > +return NVME_INVALID_FIELD | NVME_DNR; Another leaked local_err. > +} > +req->cqe.result = cpu_to_le32(nsid); > +break; > +case NVME_CSI_ZONED: > +/* fall through for now */ > +default: > +return NVME_INVALID_FIELD | NVME_DNR; > + } > +break; > +default: > +return NVME_INVALID_FIELD | NVME_DNR; > +} > + > +return NVME_SUCCESS; > +}
Re: [PATCH v4 1/2] hw/nvme: Support for Namespaces Management from guest OS - create-ns
On Wed, Dec 28, 2022 at 01:41:40PM -0600, Jonathan Derrick wrote: > +static uint16_t nvme_ns_mgmt(NvmeCtrl *n, NvmeRequest *req) > +{ > +NvmeCtrl *n_p = NULL; /* primary controller */ > +NvmeIdCtrl *id = &n->id_ctrl; > +NvmeNamespace *ns; > +NvmeIdNsMgmt id_ns = {}; > +uint8_t flags = req->cmd.flags; > +uint32_t nsid = le32_to_cpu(req->cmd.nsid); > +uint32_t dw10 = le32_to_cpu(req->cmd.cdw10); > +uint32_t dw11 = le32_to_cpu(req->cmd.cdw11); > +uint8_t sel = dw10 & 0xf; > +uint8_t csi = (dw11 >> 24) & 0xf; > +uint16_t i; > +uint16_t ret; > +Error *local_err = NULL; > + > +trace_pci_nvme_ns_mgmt(nvme_cid(req), nsid, sel, csi, > NVME_CMD_FLAGS_PSDT(flags)); > + > +if (!(le16_to_cpu(id->oacs) & NVME_OACS_NS_MGMT)) { > +return NVME_NS_ATTACH_MGMT_NOTSPRD | NVME_DNR; > +} > + > +if (n->cntlid && !n->subsys) { > +error_setg(&local_err, "Secondary controller without subsystem"); > +return NVME_NS_ATTACH_MGMT_NOTSPRD | NVME_DNR; Leaking local_err. Any time you call error_setg(), the error needs to be reported or freed at some point. > +} > + > +n_p = n->subsys->ctrls[0]; > + > +switch (sel) { > +case NVME_NS_MANAGEMENT_CREATE: > +switch (csi) { > +case NVME_CSI_NVM: The following case is sufficiently large enough that the implementation should be its own function. > +if (nsid) { > +return NVME_INVALID_FIELD | NVME_DNR; > +} > + > +ret = nvme_h2c(n, (uint8_t *)&id_ns, sizeof(id_ns), req); > +if (ret) { > +return ret; > +} > + > +uint64_t nsze = le64_to_cpu(id_ns.nsze); > +uint64_t ncap = le64_to_cpu(id_ns.ncap); Please don't mix declarations with code; declare these local variables at the top of the scope. > + > +if (ncap > nsze) { > +return NVME_INVALID_FIELD | NVME_DNR; > +} else if (ncap != nsze) { > +return NVME_THIN_PROVISION_NOTSPRD | NVME_DNR; > +} > + > +nvme_validate_flbas(id_ns.flbas, &local_err); > +if (local_err) { > +error_report_err(local_err); > +return NVME_INVALID_FORMAT | NVME_DNR; > +} > + > +for (i = 1; i <= NVME_MAX_NAMESPACES; i++) { > +if (nvme_ns(n_p, (uint32_t)i) || nvme_subsys_ns(n_p->subsys, > (uint32_t)i)) { > +continue; > +} > +break; > +} > + > + > +if (i > le32_to_cpu(n_p->id_ctrl.nn) || i > > NVME_MAX_NAMESPACES) { > + return NVME_NS_IDNTIFIER_UNAVAIL | NVME_DNR; > +} > +nsid = i; > + > +/* create ns here */ > +ns = nvme_ns_mgmt_create(n, nsid, &id_ns, &local_err); > +if (!ns || local_err) { > +if (local_err) { > +error_report_err(local_err); > +} > +return NVME_INVALID_FIELD | NVME_DNR; > +} > + > +if (nvme_cfg_update(n, ns->size, NVME_NS_ALLOC_CHK)) { > +/* place for delete-ns */ > +error_setg(&local_err, "Insufficient capacity, an orphaned > ns[%"PRIu32"] will be left behind", nsid); > +return NVME_NS_INSUFFICIENT_CAPAC | NVME_DNR; Leaked local_err. > +} > +(void)nvme_cfg_update(n, ns->size, NVME_NS_ALLOC); > +if (nvme_cfg_save(n)) { > +(void)nvme_cfg_update(n, ns->size, NVME_NS_DEALLOC); > +/* place for delete-ns */ > +error_setg(&local_err, "Cannot save conf file, an orphaned > ns[%"PRIu32"] will be left behind", nsid); > +return NVME_INVALID_FIELD | NVME_DNR; Another leaked local_err. > +} > +req->cqe.result = cpu_to_le32(nsid); > +break; > +case NVME_CSI_ZONED: > +/* fall through for now */ > +default: > +return NVME_INVALID_FIELD | NVME_DNR; > + } > +break; > +default: > +return NVME_INVALID_FIELD | NVME_DNR; > +} > + > +return NVME_SUCCESS; > +}
[PATCH v4 1/2] hw/nvme: Support for Namespaces Management from guest OS - create-ns
From: Michael Kropaczek Added support for NVMEe NameSpaces Mangement allowing the guest OS to create namespaces by issuing nvme create-ns command. It is an extension to currently implemented Qemu nvme virtual device. Virtual devices representing namespaces will be created and/or deleted during Qemu's running session, at anytime. Signed-off-by: Michael Kropaczek --- docs/system/devices/nvme.rst | 59 ++- hw/nvme/cfg_key_checker.c| 51 ++ hw/nvme/ctrl-cfg.c | 224 ++ hw/nvme/ctrl.c | 244 - hw/nvme/meson.build | 2 +- hw/nvme/ns-backend.c | 283 + hw/nvme/ns.c | 293 ++- hw/nvme/nvme.h | 31 +++- hw/nvme/subsys.c | 11 +- hw/nvme/trace-events | 2 + include/block/nvme.h | 30 include/hw/nvme/ctrl-cfg.h | 24 +++ include/hw/nvme/ns-cfg.h | 28 include/hw/nvme/nvme-cfg.h | 188 ++ qemu-img-cmds.hx | 6 + qemu-img.c | 132 16 files changed, 1554 insertions(+), 54 deletions(-) create mode 100644 hw/nvme/cfg_key_checker.c create mode 100644 hw/nvme/ctrl-cfg.c create mode 100644 hw/nvme/ns-backend.c create mode 100644 include/hw/nvme/ctrl-cfg.h create mode 100644 include/hw/nvme/ns-cfg.h create mode 100644 include/hw/nvme/nvme-cfg.h diff --git a/docs/system/devices/nvme.rst b/docs/system/devices/nvme.rst index 30f841ef62..6b3bee5e5d 100644 --- a/docs/system/devices/nvme.rst +++ b/docs/system/devices/nvme.rst @@ -92,6 +92,63 @@ There are a number of parameters available: attach the namespace to a specific ``nvme`` device (identified by an ``id`` parameter on the controller device). +Additional Namespaces managed by guest OS Namespaces Management +- + +.. code-block:: console + + -device nvme,id=nvme-ctrl,serial=1234,subsys=nvme-subsys,auto-ns-path=path + +Parameters: + +``auto-ns-path=`` + If specified indicates a support for dynamic management of nvme namespaces + by means of nvme create-ns command. This path points + to the storage area for backend images must exist. Additionally it requires + that parameter `ns-subsys` must be specified whereas parameter `drive` + must not. The legacy namespace backend is disabled, instead, a pair of + files 'nvme__ns_.cfg' and 'nvme__ns_.img' + will refer to respective namespaces. The create-ns, attach-ns + and detach-ns commands, issued at the guest side, will make changes to + those files accordingly. + For each namespace exists an image file in raw format and a config file + containing namespace parameters and state of the attachment allowing QEMU + to configure namespaces accordingly during start up. If for instance an + image file has a size of 0 bytes this will be interpreted as non existent + namespace. Issuing create-ns command will change the status in the config + files and and will re-size the image file accordingly so the image file + will be associated with the respective namespace. The main config file + nvme__ctrl.cfg keeps the track of allocated capacity to the + namespaces within the nvme controller. + As it is the case of a typical hard drive, backend images together with + config files need to be created. For this reason the qemu-img tool has + been extended by adding createns command. + + qemu-img createns {-S -C } + [-N ] {} + + Parameters: + -S and -C and are mandatory, `-S` must match `serial` parameter + and must match `auto-ns-path` parameter of "-device nvme,..." + specification. + -N is optional, if specified it will set a limit to the number of potential + namespaces and will reduce the number of backend images and config files + accordingly. As a default, a set of images of 0 bytes size and default + config files for 256 namespaces will be created, a total of 513 files. + +Please note that ``nvme-ns`` device is not required to support of dynamic +namespaces management feature. It is not prohibited to assign a such device to +``nvme`` device specified to support dynamic namespace management if one has +an use case to do so, however, it will only coexist and be out of the scope of +Namespaces Management. NsIds will be consistently managed, creation (create-ns) +of a namespace will not allocate the NsId already being taken. If ``nvme-ns`` +device conflicts with previously created one by create-ns (the same NsId), +it will break QEMU's start up. +More than one of NVMe controllers associated with NVMe subsystem are supported. +This feature requires that parameters ``serial=`` and ``subsys=`` of additional +controllers must match those of the primary controller and ``auto-ns-path=`` +must not be specified. + NVM Subsystems -- @@ -320,4 +377,4 @@ controller are: .. code-block::