| Message ID | 20200625184838.28172-5-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/nvme: Various cleanups required to use multiple queues | expand |
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.
On 6/26/20 1:19 PM, Stefan Hajnoczi wrote: > 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. Clever idea, thanks!
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); if (r) { error_setg(errp, "Cannot map buffer for DMA"); goto out;
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(-)