[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 04/17] block/nvme: Be explicit we share NvmeIdCtrl / NvmeIdNs
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH 04/17] block/nvme: Be explicit we share NvmeIdCtrl / NvmeIdNs structures |
Date: |
Fri, 26 Jun 2020 12:19:05 +0100 |
On Thu, Jun 25, 2020 at 08:48:25PM +0200, Philippe Mathieu-Daudé wrote:
> We allocate an unique chunk of memory then use it for two
> different structures. Introduce the 'idsz_max' variable to
> hold the maximum size, to make it clearer the size is enough
> to hold the two structures.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> FIXME: reword with something that makes more sense...
> ---
> block/nvme.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/block/nvme.c b/block/nvme.c
> index 71f8cf27a8..ffda804a8e 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -438,6 +438,7 @@ static void nvme_identify(BlockDriverState *bs, int
> namespace, Error **errp)
> BDRVNVMeState *s = bs->opaque;
> NvmeIdCtrl *idctrl;
> NvmeIdNs *idns;
> + size_t idsz_max;
> NvmeLBAF *lbaf;
> uint8_t *resp;
> uint16_t oncs;
> @@ -448,14 +449,15 @@ static void nvme_identify(BlockDriverState *bs, int
> namespace, Error **errp)
> .cdw10 = cpu_to_le32(0x1),
> };
>
> - resp = qemu_try_blockalign0(bs, sizeof(NvmeIdCtrl));
> + idsz_max = MAX_CONST(sizeof(NvmeIdCtrl), sizeof(NvmeIdNs));
> + resp = qemu_try_blockalign0(bs, idsz_max);
> if (!resp) {
> error_setg(errp, "Cannot allocate buffer for identify response");
> goto out;
> }
> idctrl = (NvmeIdCtrl *)resp;
> idns = (NvmeIdNs *)resp;
> - r = qemu_vfio_dma_map(s->vfio, resp, sizeof(NvmeIdCtrl), true, &iova);
> + r = qemu_vfio_dma_map(s->vfio, resp, idsz_max, true, &iova);
_nvme_check_size() has compile-time asserts that check
sizeof(NvmeIdCtrl) == sizeof(NvmeIdNs) == 4096.
I suggest the following cleanup:
union {
NvmeIdCtrl ctrl;
NvmeIdNs ns;
} *id;
...
id = qemu_try_blockalign0(bs, sizeof(*id));
...
r = qemu_vfio_dma_map(s->vfio, resp, sizeof(*id), true, &iova);
and accesses to idctl are replaced with id->ctrl and idns with id->ns.
This eliminates the casts, makes it clear that this data is overlapping,
and avoids the need for idsz_max.
signature.asc
Description: PGP signature
- Re: [PATCH 01/17] block/nvme: Avoid further processing if trace event not enabled, (continued)
[PATCH 02/17] block/nvme: Let nvme_create_queue_pair() fail gracefully, Philippe Mathieu-Daudé, 2020/06/25
[PATCH 03/17] block/nvme: Define QUEUE_INDEX macros to ease code review, Philippe Mathieu-Daudé, 2020/06/25
[PATCH 05/17] block/nvme: Replace qemu_try_blockalign0 by qemu_try_blockalign/memset, Philippe Mathieu-Daudé, 2020/06/25
[PATCH 04/17] block/nvme: Be explicit we share NvmeIdCtrl / NvmeIdNs structures, Philippe Mathieu-Daudé, 2020/06/25
- Re: [PATCH 04/17] block/nvme: Be explicit we share NvmeIdCtrl / NvmeIdNs structures,
Stefan Hajnoczi <=
[PATCH 06/17] block/nvme: Replace qemu_try_blockalign(bs) by qemu_try_memalign(pg_sz), Philippe Mathieu-Daudé, 2020/06/25
[PATCH 07/17] block/nvme: Move code around, Philippe Mathieu-Daudé, 2020/06/25
[PATCH 08/17] block/nvme: Use correct type void*, Philippe Mathieu-Daudé, 2020/06/25
[PATCH 09/17] block/nvme: Remove unused argument from nvme_free_queue_pair(), Philippe Mathieu-Daudé, 2020/06/25