Re: [PATCH] virtio-gpu: fix unmap the already mapped items

[Li Zhijian]

On 8/21/20 6:07 PM, Gerd Hoffmann wrote:

On Fri, Aug 21, 2020 at 04:49:45PM +0800, Li Zhijian wrote:

we go here either (!(*iov)[i].iov_base) or (len != l), so we need to consider
to unmap the 'i'th item as well when the 'i'th item is not nil

Signed-off-by: Li Zhijian 
---
  hw/display/virtio-gpu.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 5f0dd7c150..1f777e43ff 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -656,7 +656,7 @@ int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
  qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map MMIO memory for"
" resource %d element %d\n",
__func__, ab->resource_id, i);
-virtio_gpu_cleanup_mapping_iov(g, *iov, i);
+virtio_gpu_cleanup_mapping_iov(g, *iov, i + !!(*iov)[i].iov_base);

Cute trick, but the code should be readable without having to dig out
the commit message which explains it.  Can we have something simpler
along the lines of "if (iov_base) { i++; /* cleanup partial map */ }"
please?

make sense !

Thanks
Zhijian



thanks,
   Gerd

Re: [PATCH] virtio-gpu: fix unmap the already mapped items

[Gerd Hoffmann]

On Fri, Aug 21, 2020 at 04:49:45PM +0800, Li Zhijian wrote:
> we go here either (!(*iov)[i].iov_base) or (len != l), so we need to consider
> to unmap the 'i'th item as well when the 'i'th item is not nil
> 
> Signed-off-by: Li Zhijian 
> ---
>  hw/display/virtio-gpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 5f0dd7c150..1f777e43ff 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -656,7 +656,7 @@ int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
>  qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map MMIO memory 
> for"
>" resource %d element %d\n",
>__func__, ab->resource_id, i);
> -virtio_gpu_cleanup_mapping_iov(g, *iov, i);
> +virtio_gpu_cleanup_mapping_iov(g, *iov, i + 
> !!(*iov)[i].iov_base);

Cute trick, but the code should be readable without having to dig out
the commit message which explains it.  Can we have something simpler
along the lines of "if (iov_base) { i++; /* cleanup partial map */ }"
please?

thanks,
  Gerd

[PATCH] virtio-gpu: fix unmap the already mapped items

[Li Zhijian]

we go here either (!(*iov)[i].iov_base) or (len != l), so we need to consider
to unmap the 'i'th item as well when the 'i'th item is not nil

Signed-off-by: Li Zhijian 
---
 hw/display/virtio-gpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 5f0dd7c150..1f777e43ff 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -656,7 +656,7 @@ int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
 qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map MMIO memory for"
   " resource %d element %d\n",
   __func__, ab->resource_id, i);
-virtio_gpu_cleanup_mapping_iov(g, *iov, i);
+virtio_gpu_cleanup_mapping_iov(g, *iov, i + !!(*iov)[i].iov_base);
 g_free(ents);
 *iov = NULL;
 if (addr) {
-- 
2.17.1