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 = >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(_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 *)_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, _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, _ns, _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(_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(_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 = >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(_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 *)_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, _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, _ns, _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(_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(_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:
..
