Dmitry Osipenko <dmitry.osipe...@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.ben...@linaro.org>
>> Cc: Dmitry Osipenko <dmitry.osipe...@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
