Re: [PATCH] virtio-gpu-gl: Add 'serverfd' property

[Jiang XueQian <jiangxueqian@gmail.com>]

On 5/8/25 8:00 PM, Alex Bennée wrote:
> Jiang XueQian <jiangxueqian@gmail.com> writes:
> 
>> This property passes socket of a externally started virgl_render_server
>> to virglrenderer, so that it won't try to spawn new process and get
>> killed by seccomp, allowing virtio-gpu-gl venus and sandbox to enable
>> at the same time.
> 
> Do we have an example of how to start such a virgl server?
> docs/syste,/devices/virtio-gpu.rst could be expanded with some example
> invocations of the various modes.
> 

virgl_render_server can be started like `path/to/virgl_render_server 
--socket-fd <seqpacket-socketpair-fd>`.

It is spawned by virgl_render_init function without this patch, so I 
used that 
(https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/1.1.1/src/proxy/proxy_server.c?ref_type=tags#L73) 
as a reference during development.

Thank you for the advice. I'll add some examples and docs.

>>
>> Signed-off-by: Jiang XueQian <jiangxueqian@gmail.com>
>> ---
>>   hw/display/virtio-gpu-gl.c     | 15 +++++++++++++++
>>   hw/display/virtio-gpu-virgl.c  | 17 +++++++++++++++++
>>   include/hw/virtio/virtio-gpu.h |  2 ++
>>   3 files changed, 34 insertions(+)
>>
>> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
>> index 683fad3bf8..e7c89f7c29 100644
>> --- a/hw/display/virtio-gpu-gl.c
>> +++ b/hw/display/virtio-gpu-gl.c
>> @@ -22,6 +22,7 @@
>>   #include "hw/virtio/virtio-gpu-bswap.h"
>>   #include "hw/virtio/virtio-gpu-pixman.h"
>>   #include "hw/qdev-properties.h"
>> +#include "monitor/monitor.h"
>>   
>>   #include <virglrenderer.h>
>>   
>> @@ -143,6 +144,17 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>>           return;
>>       }
>>   
>> +#if VIRGL_RENDERER_CALLBACKS_VERSION >= 3
>> +    if (g->parent_obj.conf.serverfd) {
>> +        g->parent_obj.conf.serverfd_parsed =
>> +            monitor_fd_param(monitor_cur(),
>> g->parent_obj.conf.serverfd, errp);
> 
> I think the right place to validate we have a valid serverfd is in a
> property setter function.
> 

I see your point, but I prefer to keep it simple.

Using both variables is confusing and worth improving. This code is not 
only used to validate serverfd.

Here monitor_fd_param find the fd by its symbolic name or by parsing 
digits, and take ownership of it. The fd is stored and later pass to 
virglrenderer.

After this, .serverfd is only used as a boolean, indicating whether 
.serverfd_parsed is valid.

Many devices accept fd but most of them do so by accepting a chardev, 
which is not suitable in this case. Only a few devices accept fd 
directly, I found vhost-scsi and vhost-vsock, and they are using 
monitor_fd_param. I believe abstracting it further is unnecessary.

>> +        if (g->parent_obj.conf.serverfd_parsed < 0) {
>> +            error_prepend(errp, "unable to parse serverfd: ");
>> +            return;
>> +        }
>> +    }
>> +#endif
>> +
>>       g->parent_obj.conf.flags |= (1 << VIRTIO_GPU_FLAG_VIRGL_ENABLED);
>>       g->capset_ids = virtio_gpu_virgl_get_capsets(g);
>>       VIRTIO_GPU_BASE(g)->virtio_config.num_capsets = g->capset_ids->len;
>> @@ -159,6 +171,9 @@ static const Property virtio_gpu_gl_properties[] = {
>>                       VIRTIO_GPU_FLAG_STATS_ENABLED, false),
>>       DEFINE_PROP_BIT("venus", VirtIOGPU, parent_obj.conf.flags,
>>                       VIRTIO_GPU_FLAG_VENUS_ENABLED, false),
>> +#if VIRGL_RENDERER_CALLBACKS_VERSION >= 3
>> +    DEFINE_PROP_STRING("serverfd", VirtIOGPU,
>> parent_obj.conf.serverfd),
> 
> Use DEFINE_PROP with a PropertyInfo structure.
> 
>> +#endif
>>   };
>>   
>>   static void virtio_gpu_gl_device_unrealize(DeviceState *qdev)
>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>> index 145a0b3879..420aae3b05 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -1030,6 +1030,19 @@ static int virgl_make_context_current(void *opaque, int scanout_idx,
>>                                      qctx);
>>   }
>>   
>> +#if VIRGL_RENDERER_CALLBACKS_VERSION >= 3
>> +static int virgl_get_server_fd(void *opaque, uint32_t version)
>> +{
>> +    VirtIOGPU *g = opaque;
>> +
>> +    if (g->parent_obj.conf.serverfd) {
>> +        return g->parent_obj.conf.serverfd_parsed;
>> +    }
>> +
>> +    return -1;
>> +}
>> +#endif
>> +
>>   static struct virgl_renderer_callbacks virtio_gpu_3d_cbs = {
>>       .version             = 1,
>>       .write_fence         = virgl_write_fence,
>> @@ -1097,6 +1110,10 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
>>       uint32_t flags = 0;
>>       VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>>   
>> +#if VIRGL_RENDERER_CALLBACKS_VERSION >= 3
>> +    virtio_gpu_3d_cbs.version = 3;
>> +    virtio_gpu_3d_cbs.get_server_fd = virgl_get_server_fd;
>> +#endif
>>   #if VIRGL_RENDERER_CALLBACKS_VERSION >= 4
>>       if (qemu_egl_display) {
>>           virtio_gpu_3d_cbs.version = 4;
>> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
>> index a42957c4e2..40a81f500c 100644
>> --- a/include/hw/virtio/virtio-gpu.h
>> +++ b/include/hw/virtio/virtio-gpu.h
>> @@ -128,6 +128,8 @@ struct virtio_gpu_base_conf {
>>       uint32_t xres;
>>       uint32_t yres;
>>       uint64_t hostmem;
>> +    char *serverfd;
>> +    int serverfd_parsed;
> 
> And we can drop char serverfd and have an explicit place for the fd. Is
> 0 a valid fd or do we need a initialise to -1?
> 

I'll improve this by always giving .serverfd_parsed a value, so that 
later code don't have to check .serverfd.

>>   };
>>   
>>   struct virtio_gpu_ctrl_command {
>