Re: [PATCH 01/16] nvme: fix pci doorbell size calculation - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Klaus Jensen <its@irrelevant.dk>, qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>,
	Beata Michalska <beata.michalska@linaro.org>,
	Klaus Jensen <k.jensen@samsung.com>,
	qemu-devel@nongnu.org, Max Reitz <mreitz@redhat.com>,
	Keith Busch <kbusch@kernel.org>,
	Javier Gonzalez <javier.gonz@samsung.com>,
	Maxim Levitsky <mlevitsk@redhat.com>
Subject: Re: [PATCH 01/16] nvme: fix pci doorbell size calculation
Date: Wed, 15 Apr 2020 13:34:16 +0200	[thread overview]
Message-ID: <6c2f8cf6-efb0-89e2-8a67-4b752d85c0a1@redhat.com> (raw)
In-Reply-To: <20200415102445.564803-2-its@irrelevant.dk>

On 4/15/20 12:24 PM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> The size of the BAR is 0x1000 (main registers) + 8 bytes for each
> queue. Currently, the size of the BAR is calculated like so:
> 
>      n->reg_size = pow2ceil(0x1004 + 2 * (n->params.num_queues + 1) * 4);
> 
> Since the 'num_queues' parameter already accounts for the admin queue,
> this should in any case not need to be incremented by one. Also, the
> size should be initialized to (0x1000).
> 
>      n->reg_size = pow2ceil(0x1000 + 2 * n->params.num_queues * 4);
> 
> This, with the default value of num_queues (64), we will set aside room
> for 1 admin queue and 63 I/O queues (4 bytes per doorbell, 2 doorbells
> per queue).

It seems you are right, but I'd feel safer with a Ack-by from Keith.

Do you mind adding definitions such:

#define NVME_IO_SIZE (4 * KiB)
#define DOORBELL_SIZE 4
#define DOORBELL_COUNT 2

Looking at nvme_set_feature() I see better why you introduce 
max_ioqpairs in patch #7:

     case NVME_NUMBER_OF_QUEUES:
         trace_nvme_setfeat_numq((dw11 & 0xFFFF) + 1,
                                 ((dw11 >> 16) & 0xFFFF) + 1,
                                 n->num_queues - 1, n->num_queues - 1);
         req->cqe.result =
             cpu_to_le32((n->num_queues - 2) | ((n->num_queues - 2) << 16));
         break;

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/block/nvme.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index d28335cbf377..77f9c151a665 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1345,7 +1345,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>       pcie_endpoint_cap_init(pci_dev, 0x80);
>   
>       n->num_namespaces = 1;
> -    n->reg_size = pow2ceil(0x1004 + 2 * (n->num_queues + 1) * 4);
> +    n->reg_size = pow2ceil(0x1000 + 2 * n->num_queues * 4);
>       n->ns_size = bs_size / (uint64_t)n->num_namespaces;
>   
>       n->namespaces = g_new0(NvmeNamespace, n->num_namespaces);
> 