* [PATCH] virtio-gpu-gl: Add 'serverfd' property
@ 2025-04-03 15:51 Jiang XueQian
2025-05-08 11:06 ` Jiang XueQian
2025-05-08 12:00 ` Alex Bennée
0 siblings, 2 replies; 4+ messages in thread
From: Jiang XueQian @ 2025-04-03 15:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael S. Tsirkin, Jiang XueQian
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.
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);
+ 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),
+#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;
};
struct virtio_gpu_ctrl_command {
--
2.49.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] virtio-gpu-gl: Add 'serverfd' property
2025-04-03 15:51 [PATCH] virtio-gpu-gl: Add 'serverfd' property Jiang XueQian
@ 2025-05-08 11:06 ` Jiang XueQian
2025-05-08 12:00 ` Alex Bennée
1 sibling, 0 replies; 4+ messages in thread
From: Jiang XueQian @ 2025-05-08 11:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael S. Tsirkin
Ping.
On 4/3/25 11:51 PM, Jiang XueQian wrote:
> 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.
>
> 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);
> + 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),
> +#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;
> };
>
> struct virtio_gpu_ctrl_command {
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] virtio-gpu-gl: Add 'serverfd' property
2025-04-03 15:51 [PATCH] virtio-gpu-gl: Add 'serverfd' property Jiang XueQian
2025-05-08 11:06 ` Jiang XueQian
@ 2025-05-08 12:00 ` Alex Bennée
2025-05-09 16:30 ` Jiang XueQian
1 sibling, 1 reply; 4+ messages in thread
From: Alex Bennée @ 2025-05-08 12:00 UTC (permalink / raw)
To: Jiang XueQian; +Cc: qemu-devel, Michael S. Tsirkin
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.
>
> 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.
> + 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?
> };
>
> struct virtio_gpu_ctrl_command {
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] virtio-gpu-gl: Add 'serverfd' property
2025-05-08 12:00 ` Alex Bennée
@ 2025-05-09 16:30 ` Jiang XueQian
0 siblings, 0 replies; 4+ messages in thread
From: Jiang XueQian @ 2025-05-09 16:30 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, Michael S. Tsirkin
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 {
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-05-09 16:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-03 15:51 [PATCH] virtio-gpu-gl: Add 'serverfd' property Jiang XueQian
2025-05-08 11:06 ` Jiang XueQian
2025-05-08 12:00 ` Alex Bennée
2025-05-09 16:30 ` Jiang XueQian
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).