1. Patchew
  2. QEMU
  3. [PATCH 0/2] virtio-gpu: coverity fixes
  4. View patch

[PATCH 1/2] hw/display: factor out the scanout blob to fb conversion

Alex Bennée posted 2 patches 2 weeks, 5 days ago
There is a newer version of this series
[PATCH 1/2] hw/display: factor out the scanout blob to fb conversion
Posted by Alex Bennée 2 weeks, 5 days ago 
There are two identical sequences of a code doing the same thing that
raise warnings with Coverity. Before fixing those issues lets factor
out the common code into a helper function we can share.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 include/hw/virtio/virtio-gpu.h | 15 +++++++++
 hw/display/virtio-gpu-virgl.c  | 21 +-----------
 hw/display/virtio-gpu.c        | 60 +++++++++++++++++++++-------------
 3 files changed, 53 insertions(+), 43 deletions(-)

diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 553799b8cc..90e4abe788 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -333,6 +333,21 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
                                    struct virtio_gpu_scanout *s,
                                    uint32_t resource_id);
 
+/**
+ * virtio_gpu_scanout_blob_to_fb() - fill out fb based on scanout data
+ * fb: the frame-buffer descriptor to fill out
+ * ss: the scanout blob data
+ * blob_size: the maximum size the blob can accommodate
+ *
+ * This will check we have enough space for the frame taking into
+ * account that stride for all but the last line.
+ *
+ * Returns true on success, otherwise logs guest error and returns false
+ */
+bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb,
+                                   struct virtio_gpu_set_scanout_blob *ss,
+                                   uint64_t blob_size);
+
 /* virtio-gpu-udmabuf.c */
 bool virtio_gpu_have_udmabuf(void);
 void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res);
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index eedae7357f..35599cddab 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -852,26 +852,7 @@ static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
         return;
     }
 
-    fb.format = virtio_gpu_get_pixman_format(ss.format);
-    if (!fb.format) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: pixel format not supported %d\n",
-                      __func__, ss.format);
-        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
-        return;
-    }
-
-    fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8);
-    fb.width = ss.width;
-    fb.height = ss.height;
-    fb.stride = ss.strides[0];
-    fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride;
-
-    fbend = fb.offset;
-    fbend += fb.stride * (ss.r.height - 1);
-    fbend += fb.bytes_pp * ss.r.width;
-    if (fbend > res->base.blob_size) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: fb end out of range\n",
-                      __func__);
+    if (!virtio_gpu_scanout_blob_to_fb(&fb, &ss, res->blob_size)) {
         cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
         return;
     }
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index c0570ef856..e7ca8fd1cf 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -721,13 +721,48 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
                               &fb, res, &ss.r, &cmd->error);
 }
 
+bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb,
+                                   struct virtio_gpu_set_scanout_blob *ss,
+                                   uint64_t blob_size)
+{
+    uint64_t fbend;
+
+    fb->format = virtio_gpu_get_pixman_format(ss->format);
+    if (!fb->format) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: host couldn't handle guest format %d\n",
+                      __func__, ss->format);
+        return false;
+    }
+
+    fb->bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb->format), 8);
+    fb->width = ss->width;
+    fb->height = ss->height;
+    fb->stride = ss->strides[0];
+    fb->offset = ss->offsets[0] + ss->r.x * fb->bytes_pp + ss->r.y * fb->stride;
+
+    fbend = fb->offset;
+    fbend += fb->stride * (ss->r.height - 1);
+    fbend += fb->bytes_pp * ss->r.width;
+
+    if (fbend > blob_size) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: fb end out of range\n",
+                      __func__);
+        return false;
+    }
+
+    return true;
+}
+
+
+
 static void virtio_gpu_set_scanout_blob(VirtIOGPU *g,
                                         struct virtio_gpu_ctrl_command *cmd)
 {
     struct virtio_gpu_simple_resource *res;
     struct virtio_gpu_framebuffer fb = { 0 };
     struct virtio_gpu_set_scanout_blob ss;
-    uint64_t fbend;
 
     VIRTIO_GPU_FILL_CMD(ss);
     virtio_gpu_scanout_blob_bswap(&ss);
@@ -753,28 +788,7 @@ static void virtio_gpu_set_scanout_blob(VirtIOGPU *g,
         return;
     }
 
-    fb.format = virtio_gpu_get_pixman_format(ss.format);
-    if (!fb.format) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: host couldn't handle guest format %d\n",
-                      __func__, ss.format);
-        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
-        return;
-    }
-
-    fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8);
-    fb.width = ss.width;
-    fb.height = ss.height;
-    fb.stride = ss.strides[0];
-    fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride;
-
-    fbend = fb.offset;
-    fbend += fb.stride * (ss.r.height - 1);
-    fbend += fb.bytes_pp * ss.r.width;
-    if (fbend > res->blob_size) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: fb end out of range\n",
-                      __func__);
+    if (!virtio_gpu_scanout_blob_to_fb(&fb, &ss, res->blob_size)) {
         cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
         return;
     }
-- 
2.39.5
Re: [PATCH 1/2] hw/display: factor out the scanout blob to fb conversion
Posted by Dmitry Osipenko 2 weeks, 3 days ago 
On 11/4/24 19:53, Alex Bennée wrote:
> There are two identical sequences of a code doing the same thing that
> raise warnings with Coverity. Before fixing those issues lets factor
> out the common code into a helper function we can share.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  include/hw/virtio/virtio-gpu.h | 15 +++++++++
>  hw/display/virtio-gpu-virgl.c  | 21 +-----------
>  hw/display/virtio-gpu.c        | 60 +++++++++++++++++++++-------------
>  3 files changed, 53 insertions(+), 43 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 553799b8cc..90e4abe788 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -333,6 +333,21 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
>                                     struct virtio_gpu_scanout *s,
>                                     uint32_t resource_id);
>  
> +/**
> + * virtio_gpu_scanout_blob_to_fb() - fill out fb based on scanout data
> + * fb: the frame-buffer descriptor to fill out
> + * ss: the scanout blob data
> + * blob_size: the maximum size the blob can accommodate

Nit: 'maximum size the blob can accommodate' makes it sound to me like
data will be copied into the blob. What about 'size of scanout blob data'.

> + *
> + * This will check we have enough space for the frame taking into
> + * account that stride for all but the last line.
> + *
> + * Returns true on success, otherwise logs guest error and returns false
> + */
> +bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb,
> +                                   struct virtio_gpu_set_scanout_blob *ss,
> +                                   uint64_t blob_size);
> +
>  /* virtio-gpu-udmabuf.c */
>  bool virtio_gpu_have_udmabuf(void);
>  void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res);
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index eedae7357f..35599cddab 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -852,26 +852,7 @@ static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
>          return;
>      }
>  
> -    fb.format = virtio_gpu_get_pixman_format(ss.format);
> -    if (!fb.format) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: pixel format not supported %d\n",
> -                      __func__, ss.format);
> -        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> -        return;
> -    }
> -
> -    fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8);
> -    fb.width = ss.width;
> -    fb.height = ss.height;
> -    fb.stride = ss.strides[0];
> -    fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride;
> -
> -    fbend = fb.offset;
> -    fbend += fb.stride * (ss.r.height - 1);
> -    fbend += fb.bytes_pp * ss.r.width;
> -    if (fbend > res->base.blob_size) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: fb end out of range\n",
> -                      __func__);
> +    if (!virtio_gpu_scanout_blob_to_fb(&fb, &ss, res->blob_size)) {

This fails to compile, needs s/res->blob_size/res->base.blob_size/

../hw/display/virtio-gpu-virgl.c:855:53: error: 'struct
virtio_gpu_virgl_resource' has no member named 'blob_size'
  855 |     if (!virtio_gpu_scanout_blob_to_fb(&fb, &ss, res->blob_size)) {
      |                                                     ^~
../hw/display/virtio-gpu-virgl.c:808:14: error: unused variable 'fbend'
[-Werror=unused-variable]
  808 |     uint64_t fbend;
      |              ^~~~~
cc1: all warnings being treated as errors

Please correct in v2.

>          cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>          return;
>      }
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index c0570ef856..e7ca8fd1cf 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -721,13 +721,48 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
>                                &fb, res, &ss.r, &cmd->error);
>  }
>  
> +bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb,
> +                                   struct virtio_gpu_set_scanout_blob *ss,
> +                                   uint64_t blob_size)
> +{
> +    uint64_t fbend;
> +
> +    fb->format = virtio_gpu_get_pixman_format(ss->format);
> +    if (!fb->format) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: host couldn't handle guest format %d\n",
> +                      __func__, ss->format);
> +        return false;
> +    }
> +
> +    fb->bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb->format), 8);
> +    fb->width = ss->width;
> +    fb->height = ss->height;
> +    fb->stride = ss->strides[0];
> +    fb->offset = ss->offsets[0] + ss->r.x * fb->bytes_pp + ss->r.y * fb->stride;
> +
> +    fbend = fb->offset;
> +    fbend += fb->stride * (ss->r.height - 1);
> +    fbend += fb->bytes_pp * ss->r.width;
> +
> +    if (fbend > blob_size) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: fb end out of range\n",
> +                      __func__);
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +
> +

Nit: extra newlines

-- 
Best regards,
Dmitry
Re: [PATCH 1/2] hw/display: factor out the scanout blob to fb conversion
Posted by Alex Bennée 2 weeks, 3 days ago 
Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:

> On 11/4/24 19:53, Alex Bennée wrote:
>> There are two identical sequences of a code doing the same thing that
>> raise warnings with Coverity. Before fixing those issues lets factor
>> out the common code into a helper function we can share.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> ---
>>  include/hw/virtio/virtio-gpu.h | 15 +++++++++
>>  hw/display/virtio-gpu-virgl.c  | 21 +-----------
>>  hw/display/virtio-gpu.c        | 60 +++++++++++++++++++++-------------
>>  3 files changed, 53 insertions(+), 43 deletions(-)
>> 
>> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
>> index 553799b8cc..90e4abe788 100644
>> --- a/include/hw/virtio/virtio-gpu.h
>> +++ b/include/hw/virtio/virtio-gpu.h
>> @@ -333,6 +333,21 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
>>                                     struct virtio_gpu_scanout *s,
>>                                     uint32_t resource_id);
>>  
>> +/**
>> + * virtio_gpu_scanout_blob_to_fb() - fill out fb based on scanout data
>> + * fb: the frame-buffer descriptor to fill out
>> + * ss: the scanout blob data
>> + * blob_size: the maximum size the blob can accommodate
>
> Nit: 'maximum size the blob can accommodate' makes it sound to me like
> data will be copied into the blob. What about 'size of scanout blob data'.
>
>> + *
>> + * This will check we have enough space for the frame taking into
>> + * account that stride for all but the last line.
>> + *
>> + * Returns true on success, otherwise logs guest error and returns false
>> + */
>> +bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb,
>> +                                   struct virtio_gpu_set_scanout_blob *ss,
>> +                                   uint64_t blob_size);
>> +
>>  /* virtio-gpu-udmabuf.c */
>>  bool virtio_gpu_have_udmabuf(void);
>>  void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res);
>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>> index eedae7357f..35599cddab 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -852,26 +852,7 @@ static void virgl_cmd_set_scanout_blob(VirtIOGPU *g,
>>          return;
>>      }
>>  
>> -    fb.format = virtio_gpu_get_pixman_format(ss.format);
>> -    if (!fb.format) {
>> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: pixel format not supported %d\n",
>> -                      __func__, ss.format);
>> -        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>> -        return;
>> -    }
>> -
>> -    fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8);
>> -    fb.width = ss.width;
>> -    fb.height = ss.height;
>> -    fb.stride = ss.strides[0];
>> -    fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride;
>> -
>> -    fbend = fb.offset;
>> -    fbend += fb.stride * (ss.r.height - 1);
>> -    fbend += fb.bytes_pp * ss.r.width;
>> -    if (fbend > res->base.blob_size) {
>> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: fb end out of range\n",
>> -                      __func__);
>> +    if (!virtio_gpu_scanout_blob_to_fb(&fb, &ss, res->blob_size)) {
>
> This fails to compile, needs s/res->blob_size/res->base.blob_size/
>
> ../hw/display/virtio-gpu-virgl.c:855:53: error: 'struct
> virtio_gpu_virgl_resource' has no member named 'blob_size'
>   855 |     if (!virtio_gpu_scanout_blob_to_fb(&fb, &ss, res->blob_size)) {
>       |                                                     ^~
> ../hw/display/virtio-gpu-virgl.c:808:14: error: unused variable 'fbend'
> [-Werror=unused-variable]
>   808 |     uint64_t fbend;
>       |              ^~~~~
> cc1: all warnings being treated as errors
>
> Please correct in v2.

Doh - I failed to compile that in my extra.libs config. Will fix.

>
>>          cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>>          return;
>>      }
>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>> index c0570ef856..e7ca8fd1cf 100644
>> --- a/hw/display/virtio-gpu.c
>> +++ b/hw/display/virtio-gpu.c
>> @@ -721,13 +721,48 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
>>                                &fb, res, &ss.r, &cmd->error);
>>  }
>>  
>> +bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb,
>> +                                   struct virtio_gpu_set_scanout_blob *ss,
>> +                                   uint64_t blob_size)
>> +{
>> +    uint64_t fbend;
>> +
>> +    fb->format = virtio_gpu_get_pixman_format(ss->format);
>> +    if (!fb->format) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: host couldn't handle guest format %d\n",
>> +                      __func__, ss->format);
>> +        return false;
>> +    }
>> +
>> +    fb->bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb->format), 8);
>> +    fb->width = ss->width;
>> +    fb->height = ss->height;
>> +    fb->stride = ss->strides[0];
>> +    fb->offset = ss->offsets[0] + ss->r.x * fb->bytes_pp + ss->r.y * fb->stride;
>> +
>> +    fbend = fb->offset;
>> +    fbend += fb->stride * (ss->r.height - 1);
>> +    fbend += fb->bytes_pp * ss->r.width;
>> +
>> +    if (fbend > blob_size) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: fb end out of range\n",
>> +                      __func__);
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +
>> +
>
> Nit: extra newlines

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.