[RFC PATCH v4 3/7] virtio-gpu: Improve virgl_cmd_resource_map_blob() error handling

Dmitry Osipenko posted 7 patches 2 months, 2 weeks ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>, Dmitry Osipenko <dmitry.osipenko@collabora.com>
There is a newer version of this series
[RFC PATCH v4 3/7] virtio-gpu: Improve virgl_cmd_resource_map_blob() error handling
Posted by Dmitry Osipenko 2 months, 2 weeks ago
Make virgl_cmd_resource_map_blob() return -1 for errors originated from
external virglrenderer library and respond to guest with most appropriate
error message.

Suggested-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> # guest err msg
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 2224f59cf5d7..edcdad0af232 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -138,7 +138,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
     if (ret) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map virgl resource: %s\n",
                       __func__, strerror(-ret));
-        return ret;
+        return -1;
     }
 
     vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
@@ -789,7 +789,16 @@ static void virgl_cmd_resource_map_blob(VirtIOGPU *g,
     }
 
     ret = virtio_gpu_virgl_map_resource_blob(g, res, mblob.offset);
-    if (ret) {
+
+    switch (ret) {
+    case 0:
+        break;
+
+    case -EINVAL:
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+        return;
+
+    default:
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
         return;
     }
-- 
2.51.1
Re: [RFC PATCH v4 3/7] virtio-gpu: Improve virgl_cmd_resource_map_blob() error handling
Posted by Alex Bennée 2 months, 2 weeks ago
Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:

> Make virgl_cmd_resource_map_blob() return -1 for errors originated from
> external virglrenderer library and respond to guest with most appropriate
> error message.
>
> Suggested-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> # guest err msg
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 2224f59cf5d7..edcdad0af232 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -138,7 +138,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>      if (ret) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map virgl resource: %s\n",
>                        __func__, strerror(-ret));
> -        return ret;
> +        return -1;

If we are using errno's lets use the defines rather than -1, should this
be -EPERM?

>      }
>  
>      vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
> @@ -789,7 +789,16 @@ static void virgl_cmd_resource_map_blob(VirtIOGPU *g,
>      }
>  
>      ret = virtio_gpu_virgl_map_resource_blob(g, res, mblob.offset);
> -    if (ret) {
> +
> +    switch (ret) {
> +    case 0:
> +        break;
> +
> +    case -EINVAL:
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> +        return;

which isn't what can come here. I see EOPNOTSUPP not being handled either.

> +
> +    default:
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>          return;
>      }

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro
Re: [RFC PATCH v4 3/7] virtio-gpu: Improve virgl_cmd_resource_map_blob() error handling
Posted by Dmitry Osipenko 2 months, 2 weeks ago
On 11/25/25 15:00, Alex Bennée wrote:
>>      if (ret) {
>>          qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map virgl resource: %s\n",
>>                        __func__, strerror(-ret));
>> -        return ret;
>> +        return -1;
> If we are using errno's lets use the defines rather than -1, should this
> be -EPERM?
> 
>>      }
>>  
>>      vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
>> @@ -789,7 +789,16 @@ static void virgl_cmd_resource_map_blob(VirtIOGPU *g,
>>      }
>>  
>>      ret = virtio_gpu_virgl_map_resource_blob(g, res, mblob.offset);
>> -    if (ret) {
>> +
>> +    switch (ret) {
>> +    case 0:
>> +        break;
>> +
>> +    case -EINVAL:
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>> +        return;
> which isn't what can come here. I see EOPNOTSUPP not being handled either.

When error comes from external virglrenderer code, we don't know exact
reason it fails. Using -1 allows clear distinguish of external vs
internal to QEMU errors.

EOPNOTSUPP and everything else that doesn't have a directly matching
guest code falls into the default error handling returning
VIRTIO_GPU_RESP_ERR_UNSPEC to guest.

-- 
Best regards,
Dmitry