Re: [PATCH v4 10/15] hw/nvme: Remove reg_size variable and update BAR0 size calculation

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 3506 bytes --]

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>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]