On Fri, Aug 21, 2020 at 03:27:15PM +0200, Philippe Mathieu-Daudé wrote: > On 8/21/20 12:03 PM, Stefano Garzarella wrote: > > 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? > > I'd rather not, these are different command responses, it > just happens to make this code simpler as the same response > packet is used for the 2 requests. > > See previous discussion: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg716858.html
Yeah, if it is useful only in this part of the code, never mind defining a new type. > > > > >> 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> > > Thanks! > > Phil. >