Re: [PATCH v6 10/42] nvme: refactor device realization
On Mar 25 12:40, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:28 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen
> >
> > This patch splits up nvme_realize into multiple individual functions,
> > each initializing a different subset of the device.
> >
> > Signed-off-by: Klaus Jensen
> > Acked-by: Keith Busch
> > ---
> > hw/block/nvme.c | 178 ++--
> > hw/block/nvme.h | 23 ++-
> > 2 files changed, 134 insertions(+), 67 deletions(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 7dfd8a1a392d..665485045066 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1340,57 +1337,100 @@ static void nvme_realize(PCIDevice *pci_dev, Error
> > **errp)
> > n->params.max_ioqpairs = n->params.num_queues - 1;
> > }
> >
> > -if (!n->params.max_ioqpairs) {
> > -error_setg(errp, "max_ioqpairs can't be less than 1");
> > +if (params->max_ioqpairs < 1 ||
> > +params->max_ioqpairs > PCI_MSIX_FLAGS_QSIZE) {
> > +error_setg(errp, "nvme: max_ioqpairs must be ");
> Looks like the error message is not complete now.
Fixed!
>
> Small nitpick: To be honest this not only refactoring in the device
> realization since you also (rightfully)
> removed the duplicated cmbsz/cmbloc so I would add a mention for this in the
> commit message.
> But that doesn't matter that much, so
>
You are right. I've added it as a separate patch.
> Reviewed-by: Maxim Levitsky
>
> Best regards,
> Maxim Levitsky
>
Re: [PATCH v6 10/42] nvme: refactor device realization
On Mon, 2020-03-16 at 07:28 -0700, Klaus Jensen wrote:
> From: Klaus Jensen
>
> This patch splits up nvme_realize into multiple individual functions,
> each initializing a different subset of the device.
>
> Signed-off-by: Klaus Jensen
> Acked-by: Keith Busch
> ---
> hw/block/nvme.c | 178 ++--
> hw/block/nvme.h | 23 ++-
> 2 files changed, 134 insertions(+), 67 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 7dfd8a1a392d..665485045066 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -44,6 +44,8 @@
> #include "trace.h"
> #include "nvme.h"
>
> +#define NVME_CMB_BIR 2
> +
> #define NVME_GUEST_ERR(trace, fmt, ...) \
> do { \
> (trace_##trace)(__VA_ARGS__); \
> @@ -63,7 +65,7 @@ static inline bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr
> addr)
>
> static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
> {
> -if (n->cmbsz && nvme_addr_is_cmb(n, addr)) {
> +if (n->bar.cmbsz && nvme_addr_is_cmb(n, addr)) {
> memcpy(buf, (void *)>cmbuf[addr - n->ctrl_mem.addr], size);
> return;
> }
> @@ -157,7 +159,7 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
> QEMUIOVector *iov, uint64_t prp1,
> if (unlikely(!prp1)) {
> trace_nvme_dev_err_invalid_prp();
> return NVME_INVALID_FIELD | NVME_DNR;
> -} else if (n->cmbsz && prp1 >= n->ctrl_mem.addr &&
> +} else if (n->bar.cmbsz && prp1 >= n->ctrl_mem.addr &&
> prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)) {
> qsg->nsg = 0;
> qemu_iovec_init(iov, num_prps);
> @@ -1324,14 +1326,9 @@ static const MemoryRegionOps nvme_cmb_ops = {
> },
> };
>
> -static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> +static int nvme_check_constraints(NvmeCtrl *n, Error **errp)
> {
> -NvmeCtrl *n = NVME(pci_dev);
> -NvmeIdCtrl *id = >id_ctrl;
> -
> -int i;
> -int64_t bs_size;
> -uint8_t *pci_conf;
> +NvmeParams *params = >params;
>
> if (n->params.num_queues) {
> warn_report("nvme: num_queues is deprecated; please use max_ioqpairs
> "
> @@ -1340,57 +1337,100 @@ static void nvme_realize(PCIDevice *pci_dev, Error
> **errp)
> n->params.max_ioqpairs = n->params.num_queues - 1;
> }
>
> -if (!n->params.max_ioqpairs) {
> -error_setg(errp, "max_ioqpairs can't be less than 1");
> +if (params->max_ioqpairs < 1 ||
> +params->max_ioqpairs > PCI_MSIX_FLAGS_QSIZE) {
> +error_setg(errp, "nvme: max_ioqpairs must be ");
Looks like the error message is not complete now.
> +return -1;
> }
>
> if (!n->conf.blk) {
> -error_setg(errp, "drive property not set");
> -return;
> +error_setg(errp, "nvme: block backend not configured");
> +return -1;
> }
>
> -bs_size = blk_getlength(n->conf.blk);
> -if (bs_size < 0) {
> -error_setg(errp, "could not get backing file size");
> -return;
> +if (!params->serial) {
> +error_setg(errp, "nvme: serial not configured");
> +return -1;
> }
>
> -if (!n->params.serial) {
> -error_setg(errp, "serial property not set");
> -return;
> -}
> +return 0;
> +}
> +
> +static int nvme_init_blk(NvmeCtrl *n, Error **errp)
> +{
> blkconf_blocksizes(>conf);
> if (!blkconf_apply_backend_options(>conf,
> blk_is_read_only(n->conf.blk),
> false, errp)) {
> -return;
> +return -1;
> }
>
> -pci_conf = pci_dev->config;
> -pci_conf[PCI_INTERRUPT_PIN] = 1;
> -pci_config_set_prog_interface(pci_dev->config, 0x2);
> -pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS);
> -pcie_endpoint_cap_init(pci_dev, 0x80);
> +return 0;
> +}
>
> +static void nvme_init_state(NvmeCtrl *n)
> +{
> n->num_namespaces = 1;
> n->reg_size = pow2ceil(0x1008 + 2 * (n->params.max_ioqpairs) * 4);
> -n->ns_size = bs_size / (uint64_t)n->num_namespaces;
> -
> n->namespaces = g_new0(NvmeNamespace, n->num_namespaces);
> n->sq = g_new0(NvmeSQueue *, n->params.max_ioqpairs + 1);
> n->cq = g_new0(NvmeCQueue *, n->params.max_ioqpairs + 1);
> +}
>
> -memory_region_init_io(>iomem, OBJECT(n), _mmio_ops, n,
> - "nvme", n->reg_size);
> -pci_register_bar(pci_dev, 0,
> -PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
> ->iomem);
> +static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
> +{
> +NVME_CMBLOC_SET_BIR(n->bar.cmbloc, NVME_CMB_BIR);
> +NVME_CMBLOC_SET_OFST(n->bar.cmbloc, 0);
> +
> +NVME_CMBSZ_SET_SQS(n->bar.cmbsz, 1);
> +NVME_CMBSZ_SET_CQS(n->bar.cmbsz, 0);
> +NVME_CMBSZ_SET_LISTS(n->bar.cmbsz, 0);
> +NVME_CMBSZ_SET_RDS(n->bar.cmbsz, 1);
> +NVME_CMBSZ_SET_WDS(n->bar.cmbsz, 1);
> +NVME_CMBSZ_SET_SZU(n->bar.cmbsz, 2);
> +
[PATCH v6 10/42] nvme: refactor device realization
From: Klaus Jensen
This patch splits up nvme_realize into multiple individual functions,
each initializing a different subset of the device.
Signed-off-by: Klaus Jensen
Acked-by: Keith Busch
---
hw/block/nvme.c | 178 ++--
hw/block/nvme.h | 23 ++-
2 files changed, 134 insertions(+), 67 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 7dfd8a1a392d..665485045066 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -44,6 +44,8 @@
#include "trace.h"
#include "nvme.h"
+#define NVME_CMB_BIR 2
+
#define NVME_GUEST_ERR(trace, fmt, ...) \
do { \
(trace_##trace)(__VA_ARGS__); \
@@ -63,7 +65,7 @@ static inline bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr)
static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
{
-if (n->cmbsz && nvme_addr_is_cmb(n, addr)) {
+if (n->bar.cmbsz && nvme_addr_is_cmb(n, addr)) {
memcpy(buf, (void *)>cmbuf[addr - n->ctrl_mem.addr], size);
return;
}
@@ -157,7 +159,7 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector
*iov, uint64_t prp1,
if (unlikely(!prp1)) {
trace_nvme_dev_err_invalid_prp();
return NVME_INVALID_FIELD | NVME_DNR;
-} else if (n->cmbsz && prp1 >= n->ctrl_mem.addr &&
+} else if (n->bar.cmbsz && prp1 >= n->ctrl_mem.addr &&
prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)) {
qsg->nsg = 0;
qemu_iovec_init(iov, num_prps);
@@ -1324,14 +1326,9 @@ static const MemoryRegionOps nvme_cmb_ops = {
},
};
-static void nvme_realize(PCIDevice *pci_dev, Error **errp)
+static int nvme_check_constraints(NvmeCtrl *n, Error **errp)
{
-NvmeCtrl *n = NVME(pci_dev);
-NvmeIdCtrl *id = >id_ctrl;
-
-int i;
-int64_t bs_size;
-uint8_t *pci_conf;
+NvmeParams *params = >params;
if (n->params.num_queues) {
warn_report("nvme: num_queues is deprecated; please use max_ioqpairs "
@@ -1340,57 +1337,100 @@ static void nvme_realize(PCIDevice *pci_dev, Error
**errp)
n->params.max_ioqpairs = n->params.num_queues - 1;
}
-if (!n->params.max_ioqpairs) {
-error_setg(errp, "max_ioqpairs can't be less than 1");
+if (params->max_ioqpairs < 1 ||
+params->max_ioqpairs > PCI_MSIX_FLAGS_QSIZE) {
+error_setg(errp, "nvme: max_ioqpairs must be ");
+return -1;
}
if (!n->conf.blk) {
-error_setg(errp, "drive property not set");
-return;
+error_setg(errp, "nvme: block backend not configured");
+return -1;
}
-bs_size = blk_getlength(n->conf.blk);
-if (bs_size < 0) {
-error_setg(errp, "could not get backing file size");
-return;
+if (!params->serial) {
+error_setg(errp, "nvme: serial not configured");
+return -1;
}
-if (!n->params.serial) {
-error_setg(errp, "serial property not set");
-return;
-}
+return 0;
+}
+
+static int nvme_init_blk(NvmeCtrl *n, Error **errp)
+{
blkconf_blocksizes(>conf);
if (!blkconf_apply_backend_options(>conf, blk_is_read_only(n->conf.blk),
false, errp)) {
-return;
+return -1;
}
-pci_conf = pci_dev->config;
-pci_conf[PCI_INTERRUPT_PIN] = 1;
-pci_config_set_prog_interface(pci_dev->config, 0x2);
-pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS);
-pcie_endpoint_cap_init(pci_dev, 0x80);
+return 0;
+}
+static void nvme_init_state(NvmeCtrl *n)
+{
n->num_namespaces = 1;
n->reg_size = pow2ceil(0x1008 + 2 * (n->params.max_ioqpairs) * 4);
-n->ns_size = bs_size / (uint64_t)n->num_namespaces;
-
n->namespaces = g_new0(NvmeNamespace, n->num_namespaces);
n->sq = g_new0(NvmeSQueue *, n->params.max_ioqpairs + 1);
n->cq = g_new0(NvmeCQueue *, n->params.max_ioqpairs + 1);
+}
-memory_region_init_io(>iomem, OBJECT(n), _mmio_ops, n,
- "nvme", n->reg_size);
-pci_register_bar(pci_dev, 0,
-PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
->iomem);
+static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
+{
+NVME_CMBLOC_SET_BIR(n->bar.cmbloc, NVME_CMB_BIR);
+NVME_CMBLOC_SET_OFST(n->bar.cmbloc, 0);
+
+NVME_CMBSZ_SET_SQS(n->bar.cmbsz, 1);
+NVME_CMBSZ_SET_CQS(n->bar.cmbsz, 0);
+NVME_CMBSZ_SET_LISTS(n->bar.cmbsz, 0);
+NVME_CMBSZ_SET_RDS(n->bar.cmbsz, 1);
+NVME_CMBSZ_SET_WDS(n->bar.cmbsz, 1);
+NVME_CMBSZ_SET_SZU(n->bar.cmbsz, 2);
+NVME_CMBSZ_SET_SZ(n->bar.cmbsz, n->params.cmb_size_mb);
+
+n->cmbuf = g_malloc0(NVME_CMBSZ_GETSIZE(n->bar.cmbsz));
+memory_region_init_io(>ctrl_mem, OBJECT(n), _cmb_ops, n,
+ "nvme-cmb", NVME_CMBSZ_GETSIZE(n->bar.cmbsz));
+pci_register_bar(pci_dev, NVME_CMBLOC_BIR(n->bar.cmbloc),
+ PCI_BASE_ADDRESS_SPACE_MEMORY |
+
