On Thu, Aug 20, 2020 at 06:58:54PM +0200, Philippe Mathieu-Daudé wrote: > We allocate an unique chunk of memory then use it for two > different structures. By using an union, we make it clear > the data is overlapping (and we can remove the casts). > > Suggested-by: Stefan Hajnoczi <stefa...@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com> > --- > block/nvme.c | 31 +++++++++++++++---------------- > 1 file changed, 15 insertions(+), 16 deletions(-) > > diff --git a/block/nvme.c b/block/nvme.c > index 99822d9fd36..2bd1935f951 100644 > --- a/block/nvme.c > +++ b/block/nvme.c > @@ -508,9 +508,10 @@ static int nvme_cmd_sync(BlockDriverState *bs, > NVMeQueuePair *q, > static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp) > { > BDRVNVMeState *s = bs->opaque; > - NvmeIdCtrl *idctrl; > - NvmeIdNs *idns; > - uint8_t *id; > + union { > + NvmeIdCtrl ctrl; > + NvmeIdNs ns; > + } *id;
What about defining a new 'NvmeId' type with this union? > NvmeLBAF *lbaf; > uint16_t oncs; > int r; > @@ -520,14 +521,12 @@ static void nvme_identify(BlockDriverState *bs, int > namespace, Error **errp) > .cdw10 = cpu_to_le32(0x1), > }; > > - id = qemu_try_blockalign0(bs, sizeof(NvmeIdCtrl)); > + id = qemu_try_blockalign0(bs, sizeof(*id)); > if (!id) { > error_setg(errp, "Cannot allocate buffer for identify response"); > goto out; > } > - idctrl = (NvmeIdCtrl *)id; > - idns = (NvmeIdNs *)id; > - r = qemu_vfio_dma_map(s->vfio, id, sizeof(NvmeIdCtrl), true, &iova); > + r = qemu_vfio_dma_map(s->vfio, id, sizeof(*id), true, &iova); > if (r) { > error_setg(errp, "Cannot map buffer for DMA"); > goto out; > @@ -539,22 +538,22 @@ static void nvme_identify(BlockDriverState *bs, int > namespace, Error **errp) > goto out; > } > > - if (le32_to_cpu(idctrl->nn) < namespace) { > + if (le32_to_cpu(id->ctrl.nn) < namespace) { > error_setg(errp, "Invalid namespace"); > goto out; > } > - s->write_cache_supported = le32_to_cpu(idctrl->vwc) & 0x1; > - s->max_transfer = (idctrl->mdts ? 1 << idctrl->mdts : 0) * s->page_size; > + s->write_cache_supported = le32_to_cpu(id->ctrl.vwc) & 0x1; > + s->max_transfer = (id->ctrl.mdts ? 1 << id->ctrl.mdts : 0) * > s->page_size; > /* For now the page list buffer per command is one page, to hold at most > * s->page_size / sizeof(uint64_t) entries. */ > s->max_transfer = MIN_NON_ZERO(s->max_transfer, > s->page_size / sizeof(uint64_t) * s->page_size); > > - oncs = le16_to_cpu(idctrl->oncs); > + oncs = le16_to_cpu(id->ctrl.oncs); > s->supports_write_zeroes = !!(oncs & NVME_ONCS_WRITE_ZEROS); > s->supports_discard = !!(oncs & NVME_ONCS_DSM); > > - memset(id, 0, 4096); > + memset(id, 0, sizeof(*id)); > cmd.cdw10 = 0; > cmd.nsid = cpu_to_le32(namespace); > if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) { > @@ -562,11 +561,11 @@ static void nvme_identify(BlockDriverState *bs, int > namespace, Error **errp) > goto out; > } > > - s->nsze = le64_to_cpu(idns->nsze); > - lbaf = &idns->lbaf[NVME_ID_NS_FLBAS_INDEX(idns->flbas)]; > + s->nsze = le64_to_cpu(id->ns.nsze); > + lbaf = &id->ns.lbaf[NVME_ID_NS_FLBAS_INDEX(id->ns.flbas)]; > > - if (NVME_ID_NS_DLFEAT_WRITE_ZEROES(idns->dlfeat) && > - NVME_ID_NS_DLFEAT_READ_BEHAVIOR(idns->dlfeat) == > + if (NVME_ID_NS_DLFEAT_WRITE_ZEROES(id->ns.dlfeat) && > + NVME_ID_NS_DLFEAT_READ_BEHAVIOR(id->ns.dlfeat) == > NVME_ID_NS_DLFEAT_READ_BEHAVIOR_ZEROES) { > bs->supported_write_flags |= BDRV_REQ_MAY_UNMAP; > } > -- > 2.26.2 > > With or without the new tyoe, the patch looks good to me: Reviewed-by: Stefano Garzarella <sgarz...@redhat.com>