On Jul 23 09:03, Andrzej Jakowski wrote: > So far it was not possible to have CMB and PMR emulated on the same > device, because BAR2 was used exclusively either of PMR or CMB. This > patch places CMB at BAR4 offset so it not conflicts with MSI-X vectors. > > Signed-off-by: Andrzej Jakowski <andrzej.jakow...@linux.intel.com> > --- > hw/block/nvme.c | 120 +++++++++++++++++++++++++++++-------------- > hw/block/nvme.h | 1 + > include/block/nvme.h | 4 +- > 3 files changed, 85 insertions(+), 40 deletions(-) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index 43866b744f..d55a71a346 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -22,12 +22,13 @@ > * [pmrdev=<mem_backend_file_id>,] \ > * max_ioqpairs=<N[optional]> > * > - * Note cmb_size_mb denotes size of CMB in MB. CMB is assumed to be at > - * offset 0 in BAR2 and supports only WDS, RDS and SQS for now. > + * Note cmb_size_mb denotes size of CMB in MB. CMB when configured is assumed > + * to be resident in BAR4 at offset that is 2MiB aligned. When CMB is > emulated > + * on Linux guest it is recommended to make cmb_size_mb multiple of 2. Both > + * size and alignment restrictions are imposed by Linux guest. > * > - * cmb_size_mb= and pmrdev= options are mutually exclusive due to limitation > - * in available BAR's. cmb_size_mb= will take precedence over pmrdev= when > - * both provided. > + * pmrdev is assumed to be resident in BAR2/BAR3. When configured it consumes > + * whole BAR2/BAR3 exclusively. > * Enabling pmr emulation can be achieved by pointing to memory-backend-file. > * For example: > * -object memory-backend-file,id=<mem_id>,share=on,mem-path=<file_path>, \ > @@ -57,8 +58,8 @@ > #define NVME_MAX_IOQPAIRS 0xffff > #define NVME_DB_SIZE 4 > #define NVME_SPEC_VER 0x00010300 > -#define NVME_CMB_BIR 2 > #define NVME_PMR_BIR 2 > +#define NVME_MSIX_BIR 4
I think that either we keep the CMB constant (but updated with '4' of course) or we just get rid of both NVME_{CMB,MSIX}_BIR and use a literal '4' in nvme_bar4_init. It is very clear that is only BAR 4 we use. > #define NVME_TEMPERATURE 0x143 > #define NVME_TEMPERATURE_WARNING 0x157 > #define NVME_TEMPERATURE_CRITICAL 0x175 > @@ -111,16 +112,18 @@ static uint16_t nvme_sqid(NvmeRequest *req) > > static bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr) > { > - hwaddr low = n->ctrl_mem.addr; > - hwaddr hi = n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size); > + hwaddr low = memory_region_to_absolute_addr(&n->ctrl_mem, 0); > + hwaddr hi = low + int128_get64(n->ctrl_mem.size); Are we really really sure we want to use a global helper like this? What are the chances/risk that we ever introduce another overlay? I'd say zero. We are not even using a *real* overlay, it's just an io memory region (ctrl_mem) on top of a pure container (bar4). Can't we live with an internal helper doing `n->bar4.addr + n->ctrl_mem.addr` and be done with it? It also removes a data structure walk on each invocation of nvme_addr_is_cmb (which is done for **all** addresses in PRP lists and SGLs). > > return addr >= low && addr < hi; > } > > static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size) > { > + hwaddr cmb_addr = memory_region_to_absolute_addr(&n->ctrl_mem, 0); > + > if (n->bar.cmbsz && nvme_addr_is_cmb(n, addr)) { > - memcpy(buf, (void *)&n->cmbuf[addr - n->ctrl_mem.addr], size); > + memcpy(buf, (void *)&n->cmbuf[addr - cmb_addr], size); > return; > } > > @@ -207,17 +210,18 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, > QEMUIOVector *iov, uint64_t prp1, > uint64_t prp2, uint32_t len, NvmeCtrl *n) > { > hwaddr trans_len = n->page_size - (prp1 % n->page_size); > + hwaddr cmb_addr = memory_region_to_absolute_addr(&n->ctrl_mem, 0); > trans_len = MIN(len, trans_len); > int num_prps = (len >> n->page_bits) + 1; > > if (unlikely(!prp1)) { > trace_pci_nvme_err_invalid_prp(); > return NVME_INVALID_FIELD | NVME_DNR; > - } else if (n->bar.cmbsz && prp1 >= n->ctrl_mem.addr && > - prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)) { > + } else if (n->bar.cmbsz && prp1 >= cmb_addr && > + prp1 < cmb_addr + int128_get64(n->ctrl_mem.size)) { > qsg->nsg = 0; > qemu_iovec_init(iov, num_prps); > - qemu_iovec_add(iov, (void *)&n->cmbuf[prp1 - n->ctrl_mem.addr], > trans_len); > + qemu_iovec_add(iov, (void *)&n->cmbuf[prp1 - cmb_addr], trans_len); > } else { > pci_dma_sglist_init(qsg, &n->parent_obj, num_prps); > qemu_sglist_add(qsg, prp1, trans_len); > @@ -262,7 +266,7 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, > QEMUIOVector *iov, uint64_t prp1, > if (qsg->nsg){ > qemu_sglist_add(qsg, prp_ent, trans_len); > } else { > - qemu_iovec_add(iov, (void *)&n->cmbuf[prp_ent - > n->ctrl_mem.addr], trans_len); > + qemu_iovec_add(iov, (void *)&n->cmbuf[prp_ent - > cmb_addr], trans_len); > } > len -= trans_len; > i++; > @@ -275,7 +279,7 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, > QEMUIOVector *iov, uint64_t prp1, > if (qsg->nsg) { > qemu_sglist_add(qsg, prp2, len); > } else { > - qemu_iovec_add(iov, (void *)&n->cmbuf[prp2 - > n->ctrl_mem.addr], trans_len); > + qemu_iovec_add(iov, (void *)&n->cmbuf[prp2 - cmb_addr], > trans_len); > } > } > } > @@ -1980,7 +1984,7 @@ static void nvme_check_constraints(NvmeCtrl *n, Error > **errp) > return; > } > > - if (!n->params.cmb_size_mb && n->pmrdev) { > + if (n->pmrdev) { > if (host_memory_backend_is_mapped(n->pmrdev)) { > char *path = > object_get_canonical_path_component(OBJECT(n->pmrdev)); > error_setg(errp, "can't use already busy memdev: %s", path); > @@ -2042,33 +2046,73 @@ static void nvme_init_namespace(NvmeCtrl *n, > NvmeNamespace *ns, Error **errp) > id_ns->nuse = id_ns->ncap; > } > > -static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev) > +static void nvme_bar4_init(PCIDevice *pci_dev, Error **errp) Since everything else is nvme_init_yadayada, can we swap for nvme_init_bar4? > { > - NVME_CMBLOC_SET_BIR(n->bar.cmbloc, NVME_CMB_BIR); > - NVME_CMBLOC_SET_OFST(n->bar.cmbloc, 0); > + NvmeCtrl *n = NVME(pci_dev); > + int status; > + uint64_t bar_size, cmb_offset = 0; > + uint32_t msix_vectors; > + uint32_t nvme_pba_offset; > + uint32_t cmb_size_units; > + > + msix_vectors = n->params.msix_qsize; > + nvme_pba_offset = PCI_MSIX_ENTRY_SIZE * msix_vectors; > + bar_size = nvme_pba_offset + QEMU_ALIGN_UP(msix_vectors, 64) / 8; > + > + if (n->params.cmb_size_mb) { > + 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); /* MBs */ > + NVME_CMBSZ_SET_SZ(n->bar.cmbsz, n->params.cmb_size_mb); > + > + cmb_size_units = NVME_CMBSZ_GETSIZEUNITS(n->bar.cmbsz); > + /* Linux guest requires it to be 2MiB aligned */ > + cmb_offset = QEMU_ALIGN_UP(bar_size, 2 * cmb_size_units); > + This only makes sense because cmb_size_units is fixed at 2, so instead of relying on the value, can we just use a constant of `2 * MiB` to make it completely clear? > + NVME_CMBLOC_SET_BIR(n->bar.cmbloc, NVME_MSIX_BIR); Use the NVME_CMB_BIR constant here (or a literal '4'). > + NVME_CMBLOC_SET_OFST(n->bar.cmbloc, cmb_offset / cmb_size_units); > + > + n->cmbuf = g_malloc0(NVME_CMBSZ_GETSIZE(n->bar.cmbsz)); > + > + bar_size += cmb_offset; > + bar_size += NVME_CMBSZ_GETSIZE(n->bar.cmbsz); > + } > + > + bar_size = pow2ceil(bar_size); > + > + /* > + * Create memory region for BAR4, then overlap cmb, msix and pba > + * tables on top of it. > + */ > + memory_region_init(&n->bar4, OBJECT(n), "nvme-bar4", bar_size); > + > + if (n->params.cmb_size_mb) { > + memory_region_init_io(&n->ctrl_mem, OBJECT(n), &nvme_cmb_ops, n, > + "nvme-cmb", NVME_CMBSZ_GETSIZE(n->bar.cmbsz)); > > - 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); /* MBs */ > - NVME_CMBSZ_SET_SZ(n->bar.cmbsz, n->params.cmb_size_mb); > + memory_region_add_subregion(&n->bar4, cmb_offset, &n->ctrl_mem); > + } > + > + status = msix_init(pci_dev, n->params.msix_qsize, > + &n->bar4, NVME_MSIX_BIR, 0, > + &n->bar4, NVME_MSIX_BIR, nvme_pba_offset, > + 0, errp); > + > + if (status) { > + return; > + } > > - n->cmbuf = g_malloc0(NVME_CMBSZ_GETSIZE(n->bar.cmbsz)); > - memory_region_init_io(&n->ctrl_mem, OBJECT(n), &nvme_cmb_ops, n, > - "nvme-cmb", NVME_CMBSZ_GETSIZE(n->bar.cmbsz)); > - pci_register_bar(pci_dev, NVME_CMBLOC_BIR(n->bar.cmbloc), > + pci_register_bar(pci_dev, NVME_MSIX_BIR, Since the bar has both MSIX and CMB, I'd rather use a literal '4' here (or a local constant). > PCI_BASE_ADDRESS_SPACE_MEMORY | > PCI_BASE_ADDRESS_MEM_TYPE_64 | > - PCI_BASE_ADDRESS_MEM_PREFETCH, &n->ctrl_mem); > + PCI_BASE_ADDRESS_MEM_PREFETCH, &n->bar4); > } > > static void nvme_init_pmr(NvmeCtrl *n, PCIDevice *pci_dev) > { > - /* Controller Capabilities register */ > - NVME_CAP_SET_PMRS(n->bar.cap, 1); > - > /* PMR Capabities register */ > n->bar.pmrcap = 0; > NVME_PMRCAP_SET_RDS(n->bar.pmrcap, 0); > @@ -2126,13 +2170,10 @@ static void nvme_init_pci(NvmeCtrl *n, PCIDevice > *pci_dev, Error **errp) > n->reg_size); > pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY | > PCI_BASE_ADDRESS_MEM_TYPE_64, &n->iomem); > - if (msix_init_exclusive_bar(pci_dev, n->params.msix_qsize, 4, errp)) { > - return; > - } > > - if (n->params.cmb_size_mb) { > - nvme_init_cmb(n, pci_dev); > - } else if (n->pmrdev) { > + nvme_bar4_init(pci_dev, errp); > + > + if (n->pmrdev) { > nvme_init_pmr(n, pci_dev); > } > } > @@ -2199,6 +2240,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice > *pci_dev) > NVME_CAP_SET_CSS(n->bar.cap, 1); > NVME_CAP_SET_MPSMAX(n->bar.cap, 4); > NVME_CAP_SET_CMBS(n->bar.cap, n->params.cmb_size_mb ? 1 : 0); > + NVME_CAP_SET_PMRS(n->bar.cap, n->pmrdev ? 1 : 0); > > n->bar.vs = NVME_SPEC_VER; > n->bar.intmc = n->bar.intms = 0; > diff --git a/hw/block/nvme.h b/hw/block/nvme.h > index 0b6a8ae665..f291395cd0 100644 > --- a/hw/block/nvme.h > +++ b/hw/block/nvme.h > @@ -91,6 +91,7 @@ typedef struct NvmeCtrl { > PCIDevice parent_obj; > MemoryRegion iomem; > MemoryRegion ctrl_mem; > + MemoryRegion bar4; > NvmeBar bar; > BlockConf conf; > NvmeParams params; > diff --git a/include/block/nvme.h b/include/block/nvme.h > index d641ca6649..77b59d18dd 100644 > --- a/include/block/nvme.h > +++ b/include/block/nvme.h > @@ -217,9 +217,11 @@ enum NvmeCmbszMask { > (cmbsz |= (uint64_t)(val & CMBSZ_SZU_MASK) << CMBSZ_SZU_SHIFT) > #define NVME_CMBSZ_SET_SZ(cmbsz, val) \ > (cmbsz |= (uint64_t)(val & CMBSZ_SZ_MASK) << CMBSZ_SZ_SHIFT) > +#define NVME_CMBSZ_GETSIZEUNITS(cmbsz) \ > + (1 << (12 + 4 * NVME_CMBSZ_SZU(cmbsz))) > > #define NVME_CMBSZ_GETSIZE(cmbsz) \ > - (NVME_CMBSZ_SZ(cmbsz) * (1 << (12 + 4 * NVME_CMBSZ_SZU(cmbsz)))) > + (NVME_CMBSZ_SZ(cmbsz) * NVME_CMBSZ_GETSIZEUNITS(cmbsz)) > > enum NvmePmrcapShift { > PMRCAP_RDS_SHIFT = 3, > -- > 2.21.1 > >