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);
     if (r) {
         error_setg(errp, "Cannot map buffer for DMA");
         goto out;
-- 
2.21.3

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!