Message ID | 20220126171120.2939152-11-lukasz.maniak@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | hw/nvme: SR-IOV with Virtualization Enhancements | expand |
On Jan 26 18:11, Lukasz Maniak wrote: > From: Łukasz Gieryk <lukasz.gieryk@linux.intel.com> > > The n->reg_size parameter unnecessarily splits the BAR0 size calculation > in two phases; removed to simplify the code. > > With all the calculations done in one place, it seems the pow2ceil, > applied originally to reg_size, is unnecessary. The rounding should > happen as the last step, when BAR size includes Nvme registers, queue > registers, and MSIX-related space. > > Finally, the size of the mmio memory region is extended to cover the 1st > 4KiB padding (see the map below). Access to this range is handled as > interaction with a non-existing queue and generates an error trace, so > actually nothing changes, while the reg_size variable is no longer needed. > > -------------------- > | BAR0 | > -------------------- > [Nvme Registers ] > [Queues ] > [power-of-2 padding] - removed in this patch > [4KiB padding (1) ] > [MSIX TABLE ] > [4KiB padding (2) ] > [MSIX PBA ] > [power-of-2 padding] > > Signed-off-by: Łukasz Gieryk <lukasz.gieryk@linux.intel.com> > --- > hw/nvme/ctrl.c | 10 +++++----- > hw/nvme/nvme.h | 1 - > 2 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c > index 426507ca8a..40eb6bd1a8 100644 > --- a/hw/nvme/ctrl.c > +++ b/hw/nvme/ctrl.c > @@ -6372,9 +6372,6 @@ static void nvme_init_state(NvmeCtrl *n) > n->conf_ioqpairs = n->params.max_ioqpairs; > n->conf_msix_qsize = n->params.msix_qsize; > > - /* add one to max_ioqpairs to account for the admin queue pair */ > - n->reg_size = pow2ceil(sizeof(NvmeBar) + > - 2 * (n->params.max_ioqpairs + 1) * NVME_DB_SIZE); > n->sq = g_new0(NvmeSQueue *, n->params.max_ioqpairs + 1); > n->cq = g_new0(NvmeCQueue *, n->params.max_ioqpairs + 1); > n->temperature = NVME_TEMPERATURE; > @@ -6498,7 +6495,10 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp) > pcie_ari_init(pci_dev, 0x100, 1); > } > > - bar_size = QEMU_ALIGN_UP(n->reg_size, 4 * KiB); > + /* add one to max_ioqpairs to account for the admin queue pair */ > + bar_size = sizeof(NvmeBar) + > + 2 * (n->params.max_ioqpairs + 1) * NVME_DB_SIZE; > + bar_size = QEMU_ALIGN_UP(bar_size, 4 * KiB); > msix_table_offset = bar_size; > msix_table_size = PCI_MSIX_ENTRY_SIZE * n->params.msix_qsize; > > @@ -6512,7 +6512,7 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp) > > memory_region_init(&n->bar0, OBJECT(n), "nvme-bar0", bar_size); > memory_region_init_io(&n->iomem, OBJECT(n), &nvme_mmio_ops, n, "nvme", > - n->reg_size); > + msix_table_offset); > memory_region_add_subregion(&n->bar0, 0, &n->iomem); > > if (pci_is_vf(pci_dev)) { > diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h > index 927890b490..1401ac3904 100644 > --- a/hw/nvme/nvme.h > +++ b/hw/nvme/nvme.h > @@ -414,7 +414,6 @@ typedef struct NvmeCtrl { > uint16_t max_prp_ents; > uint16_t cqe_size; > uint16_t sqe_size; > - uint32_t reg_size; > uint32_t max_q_ents; > uint8_t outstanding_aers; > uint32_t irq_status; > -- > 2.25.1 > Nice catch. Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index 426507ca8a..40eb6bd1a8 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -6372,9 +6372,6 @@ static void nvme_init_state(NvmeCtrl *n) n->conf_ioqpairs = n->params.max_ioqpairs; n->conf_msix_qsize = n->params.msix_qsize; - /* add one to max_ioqpairs to account for the admin queue pair */ - n->reg_size = pow2ceil(sizeof(NvmeBar) + - 2 * (n->params.max_ioqpairs + 1) * NVME_DB_SIZE); n->sq = g_new0(NvmeSQueue *, n->params.max_ioqpairs + 1); n->cq = g_new0(NvmeCQueue *, n->params.max_ioqpairs + 1); n->temperature = NVME_TEMPERATURE; @@ -6498,7 +6495,10 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp) pcie_ari_init(pci_dev, 0x100, 1); } - bar_size = QEMU_ALIGN_UP(n->reg_size, 4 * KiB); + /* add one to max_ioqpairs to account for the admin queue pair */ + bar_size = sizeof(NvmeBar) + + 2 * (n->params.max_ioqpairs + 1) * NVME_DB_SIZE; + bar_size = QEMU_ALIGN_UP(bar_size, 4 * KiB); msix_table_offset = bar_size; msix_table_size = PCI_MSIX_ENTRY_SIZE * n->params.msix_qsize; @@ -6512,7 +6512,7 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp) memory_region_init(&n->bar0, OBJECT(n), "nvme-bar0", bar_size); memory_region_init_io(&n->iomem, OBJECT(n), &nvme_mmio_ops, n, "nvme", - n->reg_size); + msix_table_offset); memory_region_add_subregion(&n->bar0, 0, &n->iomem); if (pci_is_vf(pci_dev)) { diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h index 927890b490..1401ac3904 100644 --- a/hw/nvme/nvme.h +++ b/hw/nvme/nvme.h @@ -414,7 +414,6 @@ typedef struct NvmeCtrl { uint16_t max_prp_ents; uint16_t cqe_size; uint16_t sqe_size; - uint32_t reg_size; uint32_t max_q_ents; uint8_t outstanding_aers; uint32_t irq_status;