[PATCH 00/16] UI-related fixes & shareable 2d memory with -display dbus

[PATCH 00/16] UI-related fixes & shareable 2d memory with -display dbus

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

This series adds Listener.Unix.Map interface to -display dbus, to allow shared
memory for the display (similar to Listener.Win32.Map interface).

While working on it, I collected a few fixes. I can re-send them in a different
series or queue them once they are reviewed.

thanks

Marc-André Lureau (16):
  hw/audio/hda: free timer on exit
  hw/audio/hda: fix memory leak on audio setup
  ui/dbus: fix leak on message filtering
  ui/win32: fix potential use-after-free with dbus shared memory
  ui/dbus: fix filtering all update messages
  ui/dbus: discard display messages on disable
  ui/dbus: discard pending CursorDefine on new one
  util/memfd: report potential errors on free
  ui/pixman: generalize shared_image_destroy
  ui/dbus: do not limit to one listener per connection / bus name
  ui/dbus: add trace for can_share_map
  ui/surface: allocate shared memory on !win32
  ui/dbus: add Listener.Unix.Map interface XML
  ui/dbus: implement Unix.Map
  virtio-gpu: allocate shareable 2d resources on !win32
  tests: add basic -display dbus Map.Unix test

 include/hw/virtio/virtio-gpu.h  |   4 +-
 include/ui/qemu-pixman.h        |   2 +
 include/ui/surface.h            |   8 ++
 hw/audio/hda-codec.c            |  33 ++++---
 hw/display/virtio-gpu.c         |  49 +++++-----
 tests/qtest/dbus-display-test.c |  64 ++++++++++++--
 ui/console.c                    |  50 ++++++-----
 ui/dbus-console.c               |  23 ++---
 ui/dbus-listener.c              | 152 ++++++++++++++++++++++++++++----
 ui/qemu-pixman.c                |  19 ++++
 util/memfd.c                    |   9 +-
 roms/openbios                   |   2 +-
 ui/dbus-display1.xml            |  45 ++++++++++
 ui/trace-events                 |   1 +
 14 files changed, 361 insertions(+), 100 deletions(-)

-- 
2.45.2.827.g557ae147e6

[PATCH 01/16] hw/audio/hda: free timer on exit

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Fixes: 280c1e1cd ("audio/hda: create millisecond timers that handle IO")

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/audio/hda-codec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
index b40eec9604..74c0292284 100644
--- a/hw/audio/hda-codec.c
+++ b/hw/audio/hda-codec.c
@@ -751,7 +751,7 @@ static void hda_audio_exit(HDACodecDevice *hda)
             continue;
         }
         if (a->use_timer) {
-            timer_del(st->buft);
+            timer_free(st->buft);
         }
         if (st->output) {
             AUD_close_out(&a->card, st->voice.out);
-- 
2.45.2.827.g557ae147e6

[PATCH 02/16] hw/audio/hda: fix memory leak on audio setup

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

When SET_STREAM_FORMAT is called, we should clear the existing setup.

Factor out common function to close a stream.

Direct leak of 144 byte(s) in 3 object(s) allocated from:
    #0 0x7f91d38f7350 in calloc (/lib64/libasan.so.8+0xf7350) (BuildId: a4ad7eb954b390cf00f07fa10952988a41d9fc7a)
    #1 0x7f91d2ab7871 in g_malloc0 (/lib64/libglib-2.0.so.0+0x64871) (BuildId: 36b60dbd02e796145a982d0151ce37202ec05649)
    #2 0x562fa2f447ee in timer_new_full /home/elmarco/src/qemu/include/qemu/timer.h:538
    #3 0x562fa2f4486f in timer_new /home/elmarco/src/qemu/include/qemu/timer.h:559
    #4 0x562fa2f448a9 in timer_new_ns /home/elmarco/src/qemu/include/qemu/timer.h:577
    #5 0x562fa2f47955 in hda_audio_setup ../hw/audio/hda-codec.c:490
    #6 0x562fa2f4897e in hda_audio_command ../hw/audio/hda-codec.c:605

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/audio/hda-codec.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
index 74c0292284..bc661504cf 100644
--- a/hw/audio/hda-codec.c
+++ b/hw/audio/hda-codec.c
@@ -472,6 +472,24 @@ static void hda_audio_set_amp(HDAAudioStream *st)
     }
 }
 
+static void hda_close_stream(HDAAudioState *a, HDAAudioStream *st)
+{
+    if (st->node == NULL) {
+        return;
+    }
+    if (a->use_timer) {
+        timer_free(st->buft);
+        st->buft = NULL;
+    }
+    if (st->output) {
+        AUD_close_out(&a->card, st->voice.out);
+        st->voice.out = NULL;
+    } else {
+        AUD_close_in(&a->card, st->voice.in);
+        st->voice.in = NULL;
+    }
+}
+
 static void hda_audio_setup(HDAAudioStream *st)
 {
     bool use_timer = st->state->use_timer;
@@ -484,6 +502,7 @@ static void hda_audio_setup(HDAAudioStream *st)
     trace_hda_audio_format(st->node->name, st->as.nchannels,
                            fmt2name[st->as.fmt], st->as.freq);
 
+    hda_close_stream(st->state, st);
     if (st->output) {
         if (use_timer) {
             cb = hda_audio_output_cb;
@@ -741,23 +760,11 @@ static void hda_audio_init(HDACodecDevice *hda,
 static void hda_audio_exit(HDACodecDevice *hda)
 {
     HDAAudioState *a = HDA_AUDIO(hda);
-    HDAAudioStream *st;
     int i;
 
     dprint(a, 1, "%s\n", __func__);
     for (i = 0; i < ARRAY_SIZE(a->st); i++) {
-        st = a->st + i;
-        if (st->node == NULL) {
-            continue;
-        }
-        if (a->use_timer) {
-            timer_free(st->buft);
-        }
-        if (st->output) {
-            AUD_close_out(&a->card, st->voice.out);
-        } else {
-            AUD_close_in(&a->card, st->voice.in);
-        }
+        hda_close_stream(a, a->st + i);
     }
     AUD_remove_card(&a->card);
 }
-- 
2.45.2.827.g557ae147e6

[PATCH 03/16] ui/dbus: fix leak on message filtering

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

A filter function that wants to drop a message should return NULL, in
which case it must also unref the message itself.

Fixes: fa88b85de ("ui/dbus: filter out pending messages when scanout")

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus-listener.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index a54123acea..434bd608f2 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -1001,6 +1001,7 @@ dbus_filter(GDBusConnection *connection,
     serial = g_dbus_message_get_serial(message);
     if (serial <= ddl->out_serial_to_discard) {
         trace_dbus_filter(serial, ddl->out_serial_to_discard);
+        g_object_unref(message);
         return NULL;
     }
 
-- 
2.45.2.827.g557ae147e6

[PATCH 04/16] ui/win32: fix potential use-after-free with dbus shared memory

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

DisplaySurface may be free before the pixman image is freed, since the
image is refcounted and used by different objects, including pending
dbus messages.

Furthermore, setting the destroy function in
create_displaysurface_from() isn't appropriate, as it may not be used,
and may be overriden as in ramfb.

Set the destroy function when the shared handle is set, use the HANDLE
directly for destroy data, using a single common helper
qemu_pixman_win32_image_destroy().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/ui/qemu-pixman.h |  2 ++
 hw/display/virtio-gpu.c  | 14 ++------------
 ui/console.c             | 24 ++----------------------
 ui/qemu-pixman.c         | 15 +++++++++++++++
 4 files changed, 21 insertions(+), 34 deletions(-)

diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
index ef13a8210c..e3dd72b9e3 100644
--- a/include/ui/qemu-pixman.h
+++ b/include/ui/qemu-pixman.h
@@ -97,6 +97,8 @@ void qemu_pixman_glyph_render(pixman_image_t *glyph,
 
 void qemu_pixman_image_unref(pixman_image_t *image);
 
+void qemu_pixman_win32_image_destroy(pixman_image_t *image, void *data);
+
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(pixman_image_t, qemu_pixman_image_unref)
 
 #endif /* QEMU_PIXMAN_H */
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 3281842bfe..017a0f170c 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -238,16 +238,6 @@ static uint32_t calc_image_hostmem(pixman_format_code_t pformat,
     return height * stride;
 }
 
-#ifdef WIN32
-static void
-win32_pixman_image_destroy(pixman_image_t *image, void *data)
-{
-    HANDLE handle = data;
-
-    qemu_win32_map_free(pixman_image_get_data(image), handle, &error_warn);
-}
-#endif
-
 static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
                                           struct virtio_gpu_ctrl_command *cmd)
 {
@@ -308,7 +298,7 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
             bits, c2d.height ? res->hostmem / c2d.height : 0);
 #ifdef WIN32
         if (res->image) {
-            pixman_image_set_destroy_function(res->image, win32_pixman_image_destroy, res->handle);
+            pixman_image_set_destroy_function(res->image, qemu_pixman_win32_image_destroy, res->handle);
         }
 #endif
     }
@@ -1327,7 +1317,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
             return -EINVAL;
         }
 #ifdef WIN32
-        pixman_image_set_destroy_function(res->image, win32_pixman_image_destroy, res->handle);
+        pixman_image_set_destroy_function(res->image, qemu_pixman_win32_image_destroy, res->handle);
 #endif
 
         res->addrs = g_new(uint64_t, res->iov_cnt);
diff --git a/ui/console.c b/ui/console.c
index 105a0e2c70..8f416ff0b9 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -461,24 +461,6 @@ void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
     surface->handle = h;
     surface->handle_offset = offset;
 }
-
-static void
-win32_pixman_image_destroy(pixman_image_t *image, void *data)
-{
-    DisplaySurface *surface = data;
-
-    if (!surface->handle) {
-        return;
-    }
-
-    assert(surface->handle_offset == 0);
-
-    qemu_win32_map_free(
-        pixman_image_get_data(surface->image),
-        surface->handle,
-        &error_warn
-    );
-}
 #endif
 
 DisplaySurface *qemu_create_displaysurface(int width, int height)
@@ -504,6 +486,8 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
 
 #ifdef WIN32
     qemu_displaysurface_win32_set_handle(surface, handle, 0);
+    pixman_image_set_destroy_function(surface->image,
+                                      qemu_pixman_win32_image_destroy, handle);
 #endif
     return surface;
 }
@@ -519,10 +503,6 @@ DisplaySurface *qemu_create_displaysurface_from(int width, int height,
                                               width, height,
                                               (void *)data, linesize);
     assert(surface->image != NULL);
-#ifdef WIN32
-    pixman_image_set_destroy_function(surface->image,
-                                      win32_pixman_image_destroy, surface);
-#endif
 
     return surface;
 }
diff --git a/ui/qemu-pixman.c b/ui/qemu-pixman.c
index 6cada8b45e..3870e1a215 100644
--- a/ui/qemu-pixman.c
+++ b/ui/qemu-pixman.c
@@ -4,6 +4,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "ui/console.h"
 #include "standard-headers/drm/drm_fourcc.h"
 #include "trace.h"
@@ -267,3 +268,17 @@ void qemu_pixman_glyph_render(pixman_image_t *glyph,
     pixman_image_unref(ibg);
 }
 #endif /* CONFIG_PIXMAN */
+
+#ifdef WIN32
+void
+qemu_pixman_win32_image_destroy(pixman_image_t *image, void *data)
+{
+    HANDLE handle = data;
+
+    qemu_win32_map_free(
+        pixman_image_get_data(image),
+        handle,
+        &error_warn
+    );
+}
+#endif
-- 
2.45.2.827.g557ae147e6

[PATCH 05/16] ui/dbus: fix filtering all update messages

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Filtering pending messages when a new scanout is given shouldn't discard
pending cursor changes, for example.

Since filtering happens in a different thread, use atomic set/get.

Fixes: fa88b85dea ("ui/dbus: filter out pending messages when scanout")

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus-listener.c | 44 ++++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 434bd608f2..a70cad3a90 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -26,6 +26,7 @@
 #include "qapi/error.h"
 #include "sysemu/sysemu.h"
 #include "dbus.h"
+#include "glib.h"
 #ifdef G_OS_UNIX
 #include <gio/gunixfdlist.h>
 #endif
@@ -85,7 +86,7 @@ struct _DBusDisplayListener {
 #endif
 
     guint dbus_filter;
-    guint32 out_serial_to_discard;
+    guint32 display_serial_to_discard;
 };
 
 G_DEFINE_TYPE(DBusDisplayListener, dbus_display_listener, G_TYPE_OBJECT)
@@ -93,10 +94,12 @@ G_DEFINE_TYPE(DBusDisplayListener, dbus_display_listener, G_TYPE_OBJECT)
 static void dbus_gfx_update(DisplayChangeListener *dcl,
                             int x, int y, int w, int h);
 
-static void ddl_discard_pending_messages(DBusDisplayListener *ddl)
+static void ddl_discard_display_messages(DBusDisplayListener *ddl)
 {
-    ddl->out_serial_to_discard = g_dbus_connection_get_last_serial(
+    guint32 serial = g_dbus_connection_get_last_serial(
         g_dbus_proxy_get_connection(G_DBUS_PROXY(ddl->proxy)));
+
+    g_atomic_int_set(&ddl->display_serial_to_discard, serial);
 }
 
 #ifdef CONFIG_OPENGL
@@ -290,7 +293,7 @@ static void dbus_scanout_dmabuf(DisplayChangeListener *dcl,
         return;
     }
 
-    ddl_discard_pending_messages(ddl);
+    ddl_discard_display_messages(ddl);
 
     width = qemu_dmabuf_get_width(dmabuf);
     height = qemu_dmabuf_get_height(dmabuf);
@@ -338,7 +341,7 @@ static bool dbus_scanout_map(DBusDisplayListener *ddl)
         return false;
     }
 
-    ddl_discard_pending_messages(ddl);
+    ddl_discard_display_messages(ddl);
 
     if (!qemu_dbus_display1_listener_win32_map_call_scanout_map_sync(
             ddl->map_proxy,
@@ -401,7 +404,7 @@ dbus_scanout_share_d3d_texture(
         return false;
     }
 
-    ddl_discard_pending_messages(ddl);
+    ddl_discard_display_messages(ddl);
 
     qemu_dbus_display1_listener_win32_d3d11_call_scanout_texture2d(
         ddl->d3d11_proxy,
@@ -659,7 +662,7 @@ static void ddl_scanout(DBusDisplayListener *ddl)
         surface_stride(ddl->ds) * surface_height(ddl->ds), TRUE,
         (GDestroyNotify)pixman_image_unref, pixman_image_ref(ddl->ds->image));
 
-    ddl_discard_pending_messages(ddl);
+    ddl_discard_display_messages(ddl);
 
     qemu_dbus_display1_listener_call_scanout(
         ddl->proxy, surface_width(ddl->ds), surface_height(ddl->ds),
@@ -992,17 +995,34 @@ dbus_filter(GDBusConnection *connection,
             gpointer         user_data)
 {
     DBusDisplayListener *ddl = DBUS_DISPLAY_LISTENER(user_data);
-    guint32 serial;
+    const gchar *member = NULL;
+    guint32 serial, discard_serial;
 
     if (incoming) {
         return message;
     }
 
     serial = g_dbus_message_get_serial(message);
-    if (serial <= ddl->out_serial_to_discard) {
-        trace_dbus_filter(serial, ddl->out_serial_to_discard);
-        g_object_unref(message);
-        return NULL;
+
+    discard_serial = g_atomic_int_get(&ddl->display_serial_to_discard);
+    if (serial <= discard_serial) {
+        member = g_dbus_message_get_member(message);
+        if (g_strv_contains((const gchar *[]) {
+                "Scanout",
+                "Update",
+#ifdef CONFIG_GBM
+                "ScanoutDMABUF",
+                "UpdateDMABUF",
+#endif
+                "ScanoutMap",
+                "UpdateMap",
+                "Disable",
+                NULL,
+            }, member)) {
+            trace_dbus_filter(serial, discard_serial);
+            g_object_unref(message);
+            return NULL;
+        }
     }
 
     return message;
-- 
2.45.2.827.g557ae147e6

Re: [PATCH 05/16] ui/dbus: fix filtering all update messages

[Akihiko Odaki]

On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Filtering pending messages when a new scanout is given shouldn't discard
> pending cursor changes, for example.
> 
> Since filtering happens in a different thread, use atomic set/get.
> 
> Fixes: fa88b85dea ("ui/dbus: filter out pending messages when scanout")
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   ui/dbus-listener.c | 44 ++++++++++++++++++++++++++++++++------------
>   1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
> index 434bd608f2..a70cad3a90 100644
> --- a/ui/dbus-listener.c
> +++ b/ui/dbus-listener.c
> @@ -26,6 +26,7 @@
>   #include "qapi/error.h"
>   #include "sysemu/sysemu.h"
>   #include "dbus.h"
> +#include "glib.h"
>   #ifdef G_OS_UNIX
>   #include <gio/gunixfdlist.h>
>   #endif
> @@ -85,7 +86,7 @@ struct _DBusDisplayListener {
>   #endif
>   
>       guint dbus_filter;
> -    guint32 out_serial_to_discard;
> +    guint32 display_serial_to_discard;
>   };
>   
>   G_DEFINE_TYPE(DBusDisplayListener, dbus_display_listener, G_TYPE_OBJECT)
> @@ -93,10 +94,12 @@ G_DEFINE_TYPE(DBusDisplayListener, dbus_display_listener, G_TYPE_OBJECT)
>   static void dbus_gfx_update(DisplayChangeListener *dcl,
>                               int x, int y, int w, int h);
>   
> -static void ddl_discard_pending_messages(DBusDisplayListener *ddl)
> +static void ddl_discard_display_messages(DBusDisplayListener *ddl)
>   {
> -    ddl->out_serial_to_discard = g_dbus_connection_get_last_serial(
> +    guint32 serial = g_dbus_connection_get_last_serial(
>           g_dbus_proxy_get_connection(G_DBUS_PROXY(ddl->proxy)));
> +
> +    g_atomic_int_set(&ddl->display_serial_to_discard, serial);
>   }
>   
>   #ifdef CONFIG_OPENGL
> @@ -290,7 +293,7 @@ static void dbus_scanout_dmabuf(DisplayChangeListener *dcl,
>           return;
>       }
>   
> -    ddl_discard_pending_messages(ddl);
> +    ddl_discard_display_messages(ddl);
>   
>       width = qemu_dmabuf_get_width(dmabuf);
>       height = qemu_dmabuf_get_height(dmabuf);
> @@ -338,7 +341,7 @@ static bool dbus_scanout_map(DBusDisplayListener *ddl)
>           return false;
>       }
>   
> -    ddl_discard_pending_messages(ddl);
> +    ddl_discard_display_messages(ddl);
>   
>       if (!qemu_dbus_display1_listener_win32_map_call_scanout_map_sync(
>               ddl->map_proxy,
> @@ -401,7 +404,7 @@ dbus_scanout_share_d3d_texture(
>           return false;
>       }
>   
> -    ddl_discard_pending_messages(ddl);
> +    ddl_discard_display_messages(ddl);
>   
>       qemu_dbus_display1_listener_win32_d3d11_call_scanout_texture2d(
>           ddl->d3d11_proxy,
> @@ -659,7 +662,7 @@ static void ddl_scanout(DBusDisplayListener *ddl)
>           surface_stride(ddl->ds) * surface_height(ddl->ds), TRUE,
>           (GDestroyNotify)pixman_image_unref, pixman_image_ref(ddl->ds->image));
>   
> -    ddl_discard_pending_messages(ddl);
> +    ddl_discard_display_messages(ddl);
>   
>       qemu_dbus_display1_listener_call_scanout(
>           ddl->proxy, surface_width(ddl->ds), surface_height(ddl->ds),
> @@ -992,17 +995,34 @@ dbus_filter(GDBusConnection *connection,
>               gpointer         user_data)
>   {
>       DBusDisplayListener *ddl = DBUS_DISPLAY_LISTENER(user_data);
> -    guint32 serial;
> +    const gchar *member = NULL;

I suggest removing the initialization will NULL as it may suppress 
uninitialized variable warning.

> +    guint32 serial, discard_serial;
>   
>       if (incoming) {
>           return message;
>       }
>   
>       serial = g_dbus_message_get_serial(message);
> -    if (serial <= ddl->out_serial_to_discard) {
> -        trace_dbus_filter(serial, ddl->out_serial_to_discard);
> -        g_object_unref(message);
> -        return NULL;
> +
> +    discard_serial = g_atomic_int_get(&ddl->display_serial_to_discard);
> +    if (serial <= discard_serial) {
> +        member = g_dbus_message_get_member(message);
> +        if (g_strv_contains((const gchar *[]) {
> +                "Scanout",
> +                "Update",
> +#ifdef CONFIG_GBM
> +                "ScanoutDMABUF",
> +                "UpdateDMABUF",
> +#endif
> +                "ScanoutMap",
> +                "UpdateMap",
> +                "Disable",
> +                NULL,
> +            }, member)) {

I prefer to have a static variable for the array. It makes the object 
code simpler and also avoids to have a multi-line condition in the if 
statement.

> +            trace_dbus_filter(serial, discard_serial);
> +            g_object_unref(message);
> +            return NULL;
> +        }
>       }
>   
>       return message;

Re: [PATCH 05/16] ui/dbus: fix filtering all update messages

[Marc-André Lureau]

Hi Akihiko

On Sat, Oct 5, 2024 at 12:44 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Filtering pending messages when a new scanout is given shouldn't discard
> > pending cursor changes, for example.
> >
> > Since filtering happens in a different thread, use atomic set/get.
> >
> > Fixes: fa88b85dea ("ui/dbus: filter out pending messages when scanout")
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   ui/dbus-listener.c | 44 ++++++++++++++++++++++++++++++++------------
> >   1 file changed, 32 insertions(+), 12 deletions(-)
> >
> > diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
> > index 434bd608f2..a70cad3a90 100644
> > --- a/ui/dbus-listener.c
> > +++ b/ui/dbus-listener.c
> > @@ -26,6 +26,7 @@
> >   #include "qapi/error.h"
> >   #include "sysemu/sysemu.h"
> >   #include "dbus.h"
> > +#include "glib.h"
> >   #ifdef G_OS_UNIX
> >   #include <gio/gunixfdlist.h>
> >   #endif
> > @@ -85,7 +86,7 @@ struct _DBusDisplayListener {
> >   #endif
> >
> >       guint dbus_filter;
> > -    guint32 out_serial_to_discard;
> > +    guint32 display_serial_to_discard;
> >   };
> >
> >   G_DEFINE_TYPE(DBusDisplayListener, dbus_display_listener, G_TYPE_OBJECT)
> > @@ -93,10 +94,12 @@ G_DEFINE_TYPE(DBusDisplayListener, dbus_display_listener, G_TYPE_OBJECT)
> >   static void dbus_gfx_update(DisplayChangeListener *dcl,
> >                               int x, int y, int w, int h);
> >
> > -static void ddl_discard_pending_messages(DBusDisplayListener *ddl)
> > +static void ddl_discard_display_messages(DBusDisplayListener *ddl)
> >   {
> > -    ddl->out_serial_to_discard = g_dbus_connection_get_last_serial(
> > +    guint32 serial = g_dbus_connection_get_last_serial(
> >           g_dbus_proxy_get_connection(G_DBUS_PROXY(ddl->proxy)));
> > +
> > +    g_atomic_int_set(&ddl->display_serial_to_discard, serial);
> >   }
> >
> >   #ifdef CONFIG_OPENGL
> > @@ -290,7 +293,7 @@ static void dbus_scanout_dmabuf(DisplayChangeListener *dcl,
> >           return;
> >       }
> >
> > -    ddl_discard_pending_messages(ddl);
> > +    ddl_discard_display_messages(ddl);
> >
> >       width = qemu_dmabuf_get_width(dmabuf);
> >       height = qemu_dmabuf_get_height(dmabuf);
> > @@ -338,7 +341,7 @@ static bool dbus_scanout_map(DBusDisplayListener *ddl)
> >           return false;
> >       }
> >
> > -    ddl_discard_pending_messages(ddl);
> > +    ddl_discard_display_messages(ddl);
> >
> >       if (!qemu_dbus_display1_listener_win32_map_call_scanout_map_sync(
> >               ddl->map_proxy,
> > @@ -401,7 +404,7 @@ dbus_scanout_share_d3d_texture(
> >           return false;
> >       }
> >
> > -    ddl_discard_pending_messages(ddl);
> > +    ddl_discard_display_messages(ddl);
> >
> >       qemu_dbus_display1_listener_win32_d3d11_call_scanout_texture2d(
> >           ddl->d3d11_proxy,
> > @@ -659,7 +662,7 @@ static void ddl_scanout(DBusDisplayListener *ddl)
> >           surface_stride(ddl->ds) * surface_height(ddl->ds), TRUE,
> >           (GDestroyNotify)pixman_image_unref, pixman_image_ref(ddl->ds->image));
> >
> > -    ddl_discard_pending_messages(ddl);
> > +    ddl_discard_display_messages(ddl);
> >
> >       qemu_dbus_display1_listener_call_scanout(
> >           ddl->proxy, surface_width(ddl->ds), surface_height(ddl->ds),
> > @@ -992,17 +995,34 @@ dbus_filter(GDBusConnection *connection,
> >               gpointer         user_data)
> >   {
> >       DBusDisplayListener *ddl = DBUS_DISPLAY_LISTENER(user_data);
> > -    guint32 serial;
> > +    const gchar *member = NULL;
>
> I suggest removing the initialization will NULL as it may suppress
> uninitialized variable warning.

ok

>
> > +    guint32 serial, discard_serial;
> >
> >       if (incoming) {
> >           return message;
> >       }
> >
> >       serial = g_dbus_message_get_serial(message);
> > -    if (serial <= ddl->out_serial_to_discard) {
> > -        trace_dbus_filter(serial, ddl->out_serial_to_discard);
> > -        g_object_unref(message);
> > -        return NULL;
> > +
> > +    discard_serial = g_atomic_int_get(&ddl->display_serial_to_discard);
> > +    if (serial <= discard_serial) {
> > +        member = g_dbus_message_get_member(message);
> > +        if (g_strv_contains((const gchar *[]) {
> > +                "Scanout",
> > +                "Update",
> > +#ifdef CONFIG_GBM
> > +                "ScanoutDMABUF",
> > +                "UpdateDMABUF",
> > +#endif
> > +                "ScanoutMap",
> > +                "UpdateMap",
> > +                "Disable",
> > +                NULL,
> > +            }, member)) {
>
> I prefer to have a static variable for the array. It makes the object
> code simpler and also avoids to have a multi-line condition in the if
> statement.
>

done, thanks

> > +            trace_dbus_filter(serial, discard_serial);
> > +            g_object_unref(message);
> > +            return NULL;
> > +        }
> >       }
> >
> >       return message;
>

[PATCH 06/16] ui/dbus: discard display messages on disable

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus-listener.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index a70cad3a90..23a4633fa4 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -107,6 +107,8 @@ static void dbus_scanout_disable(DisplayChangeListener *dcl)
 {
     DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
 
+    ddl_discard_display_messages(ddl);
+
     qemu_dbus_display1_listener_call_disable(
         ddl->proxy, G_DBUS_CALL_FLAGS_NONE, -1, NULL, NULL, NULL);
 }
-- 
2.45.2.827.g557ae147e6

[PATCH 07/16] ui/dbus: discard pending CursorDefine on new one

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Similar to scanout updates, let's discard pending cursor changes.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus-listener.c | 23 +++++++++++++++++++++++
 roms/openbios      |  2 +-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 23a4633fa4..020c05198b 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -87,6 +87,7 @@ struct _DBusDisplayListener {
 
     guint dbus_filter;
     guint32 display_serial_to_discard;
+    guint32 cursor_serial_to_discard;
 };
 
 G_DEFINE_TYPE(DBusDisplayListener, dbus_display_listener, G_TYPE_OBJECT)
@@ -102,6 +103,14 @@ static void ddl_discard_display_messages(DBusDisplayListener *ddl)
     g_atomic_int_set(&ddl->display_serial_to_discard, serial);
 }
 
+static void ddl_discard_cursor_messages(DBusDisplayListener *ddl)
+{
+    guint32 serial = g_dbus_connection_get_last_serial(
+        g_dbus_proxy_get_connection(G_DBUS_PROXY(ddl->proxy)));
+
+    g_atomic_int_set(&ddl->cursor_serial_to_discard, serial);
+}
+
 #ifdef CONFIG_OPENGL
 static void dbus_scanout_disable(DisplayChangeListener *dcl)
 {
@@ -502,6 +511,8 @@ static void dbus_cursor_dmabuf(DisplayChangeListener *dcl,
         return;
     }
 
+    ddl_discard_cursor_messages(ddl);
+
     egl_dmabuf_import_texture(dmabuf);
     texture = qemu_dmabuf_get_texture(dmabuf);
     if (!texture) {
@@ -745,6 +756,8 @@ static void dbus_cursor_define(DisplayChangeListener *dcl,
     DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
     GVariant *v_data = NULL;
 
+    ddl_discard_cursor_messages(ddl);
+
     v_data = g_variant_new_from_data(
         G_VARIANT_TYPE("ay"),
         c->data,
@@ -1027,6 +1040,16 @@ dbus_filter(GDBusConnection *connection,
         }
     }
 
+    discard_serial = g_atomic_int_get(&ddl->cursor_serial_to_discard);
+    if (serial <= discard_serial) {
+        member = g_dbus_message_get_member(message);
+        if (g_strv_contains((const gchar *[]) { "CursorDefine", NULL }, member)) {
+            trace_dbus_filter(serial, discard_serial);
+            g_object_unref(message);
+            return NULL;
+        }
+    }
+
     return message;
 }
 
diff --git a/roms/openbios b/roms/openbios
index c3a19c1e54..af97fd7af5 160000
--- a/roms/openbios
+++ b/roms/openbios
@@ -1 +1 @@
-Subproject commit c3a19c1e54977a53027d6232050e1e3e39a98a1b
+Subproject commit af97fd7af5e7c18f591a7b987291d3db4ffb28b5
-- 
2.45.2.827.g557ae147e6

Re: [PATCH 07/16] ui/dbus: discard pending CursorDefine on new one

[Akihiko Odaki]

On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Similar to scanout updates, let's discard pending cursor changes.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   ui/dbus-listener.c | 23 +++++++++++++++++++++++
>   roms/openbios      |  2 +-
>   2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
> index 23a4633fa4..020c05198b 100644
> --- a/ui/dbus-listener.c
> +++ b/ui/dbus-listener.c
> @@ -87,6 +87,7 @@ struct _DBusDisplayListener {
>   
>       guint dbus_filter;
>       guint32 display_serial_to_discard;
> +    guint32 cursor_serial_to_discard;
>   };
>   
>   G_DEFINE_TYPE(DBusDisplayListener, dbus_display_listener, G_TYPE_OBJECT)
> @@ -102,6 +103,14 @@ static void ddl_discard_display_messages(DBusDisplayListener *ddl)
>       g_atomic_int_set(&ddl->display_serial_to_discard, serial);
>   }
>   
> +static void ddl_discard_cursor_messages(DBusDisplayListener *ddl)
> +{
> +    guint32 serial = g_dbus_connection_get_last_serial(
> +        g_dbus_proxy_get_connection(G_DBUS_PROXY(ddl->proxy)));
> +
> +    g_atomic_int_set(&ddl->cursor_serial_to_discard, serial);
> +}
> +
>   #ifdef CONFIG_OPENGL
>   static void dbus_scanout_disable(DisplayChangeListener *dcl)
>   {
> @@ -502,6 +511,8 @@ static void dbus_cursor_dmabuf(DisplayChangeListener *dcl,
>           return;
>       }
>   
> +    ddl_discard_cursor_messages(ddl);
> +
>       egl_dmabuf_import_texture(dmabuf);
>       texture = qemu_dmabuf_get_texture(dmabuf);
>       if (!texture) {
> @@ -745,6 +756,8 @@ static void dbus_cursor_define(DisplayChangeListener *dcl,
>       DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
>       GVariant *v_data = NULL;
>   
> +    ddl_discard_cursor_messages(ddl);
> +
>       v_data = g_variant_new_from_data(
>           G_VARIANT_TYPE("ay"),
>           c->data,
> @@ -1027,6 +1040,16 @@ dbus_filter(GDBusConnection *connection,
>           }
>       }
>   
> +    discard_serial = g_atomic_int_get(&ddl->cursor_serial_to_discard);
> +    if (serial <= discard_serial) {
> +        member = g_dbus_message_get_member(message);
> +        if (g_strv_contains((const gchar *[]) { "CursorDefine", NULL }, member)) {
> +            trace_dbus_filter(serial, discard_serial);
> +            g_object_unref(message);
> +            return NULL;
> +        }
> +    }
> +
>       return message;
>   }
>   
> diff --git a/roms/openbios b/roms/openbios
> index c3a19c1e54..af97fd7af5 160000
> --- a/roms/openbios
> +++ b/roms/openbios
> @@ -1 +1 @@
> -Subproject commit c3a19c1e54977a53027d6232050e1e3e39a98a1b
> +Subproject commit af97fd7af5e7c18f591a7b987291d3db4ffb28b5

An accidental change of roms/openbios.

Re: [PATCH 07/16] ui/dbus: discard pending CursorDefine on new one

[Marc-André Lureau]

Hi

On Sat, Oct 5, 2024 at 12:45 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Similar to scanout updates, let's discard pending cursor changes.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   ui/dbus-listener.c | 23 +++++++++++++++++++++++
> >   roms/openbios      |  2 +-
> >   2 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
> > index 23a4633fa4..020c05198b 100644
> > --- a/ui/dbus-listener.c
> > +++ b/ui/dbus-listener.c
> > @@ -87,6 +87,7 @@ struct _DBusDisplayListener {
> >
> >       guint dbus_filter;
> >       guint32 display_serial_to_discard;
> > +    guint32 cursor_serial_to_discard;
> >   };
> >
> >   G_DEFINE_TYPE(DBusDisplayListener, dbus_display_listener, G_TYPE_OBJECT)
> > @@ -102,6 +103,14 @@ static void ddl_discard_display_messages(DBusDisplayListener *ddl)
> >       g_atomic_int_set(&ddl->display_serial_to_discard, serial);
> >   }
> >
> > +static void ddl_discard_cursor_messages(DBusDisplayListener *ddl)
> > +{
> > +    guint32 serial = g_dbus_connection_get_last_serial(
> > +        g_dbus_proxy_get_connection(G_DBUS_PROXY(ddl->proxy)));
> > +
> > +    g_atomic_int_set(&ddl->cursor_serial_to_discard, serial);
> > +}
> > +
> >   #ifdef CONFIG_OPENGL
> >   static void dbus_scanout_disable(DisplayChangeListener *dcl)
> >   {
> > @@ -502,6 +511,8 @@ static void dbus_cursor_dmabuf(DisplayChangeListener *dcl,
> >           return;
> >       }
> >
> > +    ddl_discard_cursor_messages(ddl);
> > +
> >       egl_dmabuf_import_texture(dmabuf);
> >       texture = qemu_dmabuf_get_texture(dmabuf);
> >       if (!texture) {
> > @@ -745,6 +756,8 @@ static void dbus_cursor_define(DisplayChangeListener *dcl,
> >       DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
> >       GVariant *v_data = NULL;
> >
> > +    ddl_discard_cursor_messages(ddl);
> > +
> >       v_data = g_variant_new_from_data(
> >           G_VARIANT_TYPE("ay"),
> >           c->data,
> > @@ -1027,6 +1040,16 @@ dbus_filter(GDBusConnection *connection,
> >           }
> >       }
> >
> > +    discard_serial = g_atomic_int_get(&ddl->cursor_serial_to_discard);
> > +    if (serial <= discard_serial) {
> > +        member = g_dbus_message_get_member(message);
> > +        if (g_strv_contains((const gchar *[]) { "CursorDefine", NULL }, member)) {
> > +            trace_dbus_filter(serial, discard_serial);
> > +            g_object_unref(message);
> > +            return NULL;
> > +        }
> > +    }
> > +
> >       return message;
> >   }
> >
> > diff --git a/roms/openbios b/roms/openbios
> > index c3a19c1e54..af97fd7af5 160000
> > --- a/roms/openbios
> > +++ b/roms/openbios
> > @@ -1 +1 @@
> > -Subproject commit c3a19c1e54977a53027d6232050e1e3e39a98a1b
> > +Subproject commit af97fd7af5e7c18f591a7b987291d3db4ffb28b5
>
> An accidental change of roms/openbios.

good catch.. thanks

[PATCH 08/16] util/memfd: report potential errors on free

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 util/memfd.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/util/memfd.c b/util/memfd.c
index 4a3c07e0be..8a2e906962 100644
--- a/util/memfd.c
+++ b/util/memfd.c
@@ -28,6 +28,7 @@
 #include "qemu/osdep.h"
 
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 #include "qemu/memfd.h"
 #include "qemu/host-utils.h"
 
@@ -149,11 +150,15 @@ err:
 void qemu_memfd_free(void *ptr, size_t size, int fd)
 {
     if (ptr) {
-        munmap(ptr, size);
+        if (munmap(ptr, size) != 0) {
+            error_report("memfd munmap() failed: %s", strerror(errno));
+        }
     }
 
     if (fd != -1) {
-        close(fd);
+        if (close(fd) != 0) {
+            error_report("memfd close() failed: %s", strerror(errno));
+        }
     }
 }
 
-- 
2.45.2.827.g557ae147e6

[PATCH 09/16] ui/pixman: generalize shared_image_destroy

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Learn to free memfd-allocated shared memory.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/ui/qemu-pixman.h |  2 +-
 hw/display/virtio-gpu.c  |  4 ++--
 ui/console.c             |  2 +-
 ui/qemu-pixman.c         | 20 ++++++++++++--------
 4 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
index e3dd72b9e3..a97f56d09a 100644
--- a/include/ui/qemu-pixman.h
+++ b/include/ui/qemu-pixman.h
@@ -97,7 +97,7 @@ void qemu_pixman_glyph_render(pixman_image_t *glyph,
 
 void qemu_pixman_image_unref(pixman_image_t *image);
 
-void qemu_pixman_win32_image_destroy(pixman_image_t *image, void *data);
+void qemu_pixman_shared_image_destroy(pixman_image_t *image, void *data);
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(pixman_image_t, qemu_pixman_image_unref)
 
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 017a0f170c..77f6e76f23 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -298,7 +298,7 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
             bits, c2d.height ? res->hostmem / c2d.height : 0);
 #ifdef WIN32
         if (res->image) {
-            pixman_image_set_destroy_function(res->image, qemu_pixman_win32_image_destroy, res->handle);
+            pixman_image_set_destroy_function(res->image, qemu_pixman_shared_image_destroy, res->handle);
         }
 #endif
     }
@@ -1317,7 +1317,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
             return -EINVAL;
         }
 #ifdef WIN32
-        pixman_image_set_destroy_function(res->image, qemu_pixman_win32_image_destroy, res->handle);
+        pixman_image_set_destroy_function(res->image, qemu_pixman_shared_image_destroy, res->handle);
 #endif
 
         res->addrs = g_new(uint64_t, res->iov_cnt);
diff --git a/ui/console.c b/ui/console.c
index 8f416ff0b9..fdd76c2be4 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -487,7 +487,7 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
 #ifdef WIN32
     qemu_displaysurface_win32_set_handle(surface, handle, 0);
     pixman_image_set_destroy_function(surface->image,
-                                      qemu_pixman_win32_image_destroy, handle);
+                                      qemu_pixman_shared_image_destroy, handle);
 #endif
     return surface;
 }
diff --git a/ui/qemu-pixman.c b/ui/qemu-pixman.c
index 3870e1a215..43050ab7c5 100644
--- a/ui/qemu-pixman.c
+++ b/ui/qemu-pixman.c
@@ -6,6 +6,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "ui/console.h"
+#include "qemu/memfd.h"
 #include "standard-headers/drm/drm_fourcc.h"
 #include "trace.h"
 
@@ -269,16 +270,19 @@ void qemu_pixman_glyph_render(pixman_image_t *glyph,
 }
 #endif /* CONFIG_PIXMAN */
 
-#ifdef WIN32
 void
-qemu_pixman_win32_image_destroy(pixman_image_t *image, void *data)
+qemu_pixman_shared_image_destroy(pixman_image_t *image, void *data)
 {
+    void *ptr = pixman_image_get_data(image);
+
+#ifndef WIN32
+    int shmfd = GPOINTER_TO_INT(data);
+    size_t size = pixman_image_get_height(image) * pixman_image_get_stride(image);
+
+    qemu_memfd_free(ptr, size, shmfd);
+#else
     HANDLE handle = data;
 
-    qemu_win32_map_free(
-        pixman_image_get_data(image),
-        handle,
-        &error_warn
-    );
-}
+    qemu_win32_map_free(ptr, handle, &error_warn);
 #endif
+}
-- 
2.45.2.827.g557ae147e6

Re: [PATCH 09/16] ui/pixman: generalize shared_image_destroy

[Akihiko Odaki]

On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Learn to free memfd-allocated shared memory.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   include/ui/qemu-pixman.h |  2 +-
>   hw/display/virtio-gpu.c  |  4 ++--
>   ui/console.c             |  2 +-
>   ui/qemu-pixman.c         | 20 ++++++++++++--------
>   4 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
> index e3dd72b9e3..a97f56d09a 100644
> --- a/include/ui/qemu-pixman.h
> +++ b/include/ui/qemu-pixman.h
> @@ -97,7 +97,7 @@ void qemu_pixman_glyph_render(pixman_image_t *glyph,
>   
>   void qemu_pixman_image_unref(pixman_image_t *image);
>   
> -void qemu_pixman_win32_image_destroy(pixman_image_t *image, void *data);
> +void qemu_pixman_shared_image_destroy(pixman_image_t *image, void *data);
>   
>   G_DEFINE_AUTOPTR_CLEANUP_FUNC(pixman_image_t, qemu_pixman_image_unref)
>   
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 017a0f170c..77f6e76f23 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -298,7 +298,7 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
>               bits, c2d.height ? res->hostmem / c2d.height : 0);
>   #ifdef WIN32
>           if (res->image) {
> -            pixman_image_set_destroy_function(res->image, qemu_pixman_win32_image_destroy, res->handle);
> +            pixman_image_set_destroy_function(res->image, qemu_pixman_shared_image_destroy, res->handle);
>           }
>   #endif
>       }
> @@ -1317,7 +1317,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
>               return -EINVAL;
>           }
>   #ifdef WIN32
> -        pixman_image_set_destroy_function(res->image, qemu_pixman_win32_image_destroy, res->handle);
> +        pixman_image_set_destroy_function(res->image, qemu_pixman_shared_image_destroy, res->handle);
>   #endif
>   
>           res->addrs = g_new(uint64_t, res->iov_cnt);
> diff --git a/ui/console.c b/ui/console.c
> index 8f416ff0b9..fdd76c2be4 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -487,7 +487,7 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
>   #ifdef WIN32
>       qemu_displaysurface_win32_set_handle(surface, handle, 0);
>       pixman_image_set_destroy_function(surface->image,
> -                                      qemu_pixman_win32_image_destroy, handle);
> +                                      qemu_pixman_shared_image_destroy, handle);
>   #endif
>       return surface;
>   }
> diff --git a/ui/qemu-pixman.c b/ui/qemu-pixman.c
> index 3870e1a215..43050ab7c5 100644
> --- a/ui/qemu-pixman.c
> +++ b/ui/qemu-pixman.c
> @@ -6,6 +6,7 @@
>   #include "qemu/osdep.h"
>   #include "qapi/error.h"
>   #include "ui/console.h"
> +#include "qemu/memfd.h"
>   #include "standard-headers/drm/drm_fourcc.h"
>   #include "trace.h"
>   
> @@ -269,16 +270,19 @@ void qemu_pixman_glyph_render(pixman_image_t *glyph,
>   }
>   #endif /* CONFIG_PIXMAN */
>   
> -#ifdef WIN32
>   void
> -qemu_pixman_win32_image_destroy(pixman_image_t *image, void *data)
> +qemu_pixman_shared_image_destroy(pixman_image_t *image, void *data)
>   {
> +    void *ptr = pixman_image_get_data(image);
> +
> +#ifndef WIN32

I think it is better to have #ifdef instead of #ifndef. It is a common 
pattern to have #ifdef and #elif defined() for the platform-specific 
code and follow them with #else that implements the generic code. This 
allows supporting multiple platform conditions.

> +    int shmfd = GPOINTER_TO_INT(data);
> +    size_t size = pixman_image_get_height(image) * pixman_image_get_stride(image);
> +
> +    qemu_memfd_free(ptr, size, shmfd);
> +#else
>       HANDLE handle = data;
>   
> -    qemu_win32_map_free(
> -        pixman_image_get_data(image),
> -        handle,
> -        &error_warn
> -    );
> -}
> +    qemu_win32_map_free(ptr, handle, &error_warn);
>   #endif
> +}

Re: [PATCH 09/16] ui/pixman: generalize shared_image_destroy

[Marc-André Lureau]

Hi

On Sat, Oct 5, 2024 at 12:50 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Learn to free memfd-allocated shared memory.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   include/ui/qemu-pixman.h |  2 +-
> >   hw/display/virtio-gpu.c  |  4 ++--
> >   ui/console.c             |  2 +-
> >   ui/qemu-pixman.c         | 20 ++++++++++++--------
> >   4 files changed, 16 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
> > index e3dd72b9e3..a97f56d09a 100644
> > --- a/include/ui/qemu-pixman.h
> > +++ b/include/ui/qemu-pixman.h
> > @@ -97,7 +97,7 @@ void qemu_pixman_glyph_render(pixman_image_t *glyph,
> >
> >   void qemu_pixman_image_unref(pixman_image_t *image);
> >
> > -void qemu_pixman_win32_image_destroy(pixman_image_t *image, void *data);
> > +void qemu_pixman_shared_image_destroy(pixman_image_t *image, void *data);
> >
> >   G_DEFINE_AUTOPTR_CLEANUP_FUNC(pixman_image_t, qemu_pixman_image_unref)
> >
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 017a0f170c..77f6e76f23 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -298,7 +298,7 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
> >               bits, c2d.height ? res->hostmem / c2d.height : 0);
> >   #ifdef WIN32
> >           if (res->image) {
> > -            pixman_image_set_destroy_function(res->image, qemu_pixman_win32_image_destroy, res->handle);
> > +            pixman_image_set_destroy_function(res->image, qemu_pixman_shared_image_destroy, res->handle);
> >           }
> >   #endif
> >       }
> > @@ -1317,7 +1317,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
> >               return -EINVAL;
> >           }
> >   #ifdef WIN32
> > -        pixman_image_set_destroy_function(res->image, qemu_pixman_win32_image_destroy, res->handle);
> > +        pixman_image_set_destroy_function(res->image, qemu_pixman_shared_image_destroy, res->handle);
> >   #endif
> >
> >           res->addrs = g_new(uint64_t, res->iov_cnt);
> > diff --git a/ui/console.c b/ui/console.c
> > index 8f416ff0b9..fdd76c2be4 100644
> > --- a/ui/console.c
> > +++ b/ui/console.c
> > @@ -487,7 +487,7 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
> >   #ifdef WIN32
> >       qemu_displaysurface_win32_set_handle(surface, handle, 0);
> >       pixman_image_set_destroy_function(surface->image,
> > -                                      qemu_pixman_win32_image_destroy, handle);
> > +                                      qemu_pixman_shared_image_destroy, handle);
> >   #endif
> >       return surface;
> >   }
> > diff --git a/ui/qemu-pixman.c b/ui/qemu-pixman.c
> > index 3870e1a215..43050ab7c5 100644
> > --- a/ui/qemu-pixman.c
> > +++ b/ui/qemu-pixman.c
> > @@ -6,6 +6,7 @@
> >   #include "qemu/osdep.h"
> >   #include "qapi/error.h"
> >   #include "ui/console.h"
> > +#include "qemu/memfd.h"
> >   #include "standard-headers/drm/drm_fourcc.h"
> >   #include "trace.h"
> >
> > @@ -269,16 +270,19 @@ void qemu_pixman_glyph_render(pixman_image_t *glyph,
> >   }
> >   #endif /* CONFIG_PIXMAN */
> >
> > -#ifdef WIN32
> >   void
> > -qemu_pixman_win32_image_destroy(pixman_image_t *image, void *data)
> > +qemu_pixman_shared_image_destroy(pixman_image_t *image, void *data)
> >   {
> > +    void *ptr = pixman_image_get_data(image);
> > +
> > +#ifndef WIN32
>
> I think it is better to have #ifdef instead of #ifndef. It is a common
> pattern to have #ifdef and #elif defined() for the platform-specific
> code and follow them with #else that implements the generic code. This
> allows supporting multiple platform conditions.

indeed! thanks

>
> > +    int shmfd = GPOINTER_TO_INT(data);
> > +    size_t size = pixman_image_get_height(image) * pixman_image_get_stride(image);
> > +
> > +    qemu_memfd_free(ptr, size, shmfd);
> > +#else
> >       HANDLE handle = data;
> >
> > -    qemu_win32_map_free(
> > -        pixman_image_get_data(image),
> > -        handle,
> > -        &error_warn
> > -    );
> > -}
> > +    qemu_win32_map_free(ptr, handle, &error_warn);
> >   #endif
> > +}
>

[PATCH 10/16] ui/dbus: do not limit to one listener per connection / bus name

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This is an arbitrary limitation that doesn't concern QEMU directly and
may make some use cases unnecessarily more complicated.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus-console.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/ui/dbus-console.c b/ui/dbus-console.c
index 578b67f62b..5eb1d40d16 100644
--- a/ui/dbus-console.c
+++ b/ui/dbus-console.c
@@ -41,7 +41,7 @@ struct _DBusDisplayConsole {
     DisplayChangeListener dcl;
 
     DBusDisplay *display;
-    GHashTable *listeners;
+    GPtrArray *listeners;
     QemuDBusDisplay1Console *iface;
 
     QemuDBusDisplay1Keyboard *iface_kbd;
@@ -142,8 +142,7 @@ dbus_display_console_init(DBusDisplayConsole *object)
 {
     DBusDisplayConsole *ddc = DBUS_DISPLAY_CONSOLE(object);
 
-    ddc->listeners = g_hash_table_new_full(g_str_hash, g_str_equal,
-                                            NULL, g_object_unref);
+    ddc->listeners = g_ptr_array_new_with_free_func(g_object_unref);
     ddc->dcl.ops = &dbus_console_dcl_ops;
 }
 
@@ -157,7 +156,7 @@ dbus_display_console_dispose(GObject *object)
     g_clear_object(&ddc->iface_mouse);
     g_clear_object(&ddc->iface_kbd);
     g_clear_object(&ddc->iface);
-    g_clear_pointer(&ddc->listeners, g_hash_table_unref);
+    g_clear_pointer(&ddc->listeners, g_ptr_array_unref);
     g_clear_pointer(&ddc->kbd, qkbd_state_free);
 
     G_OBJECT_CLASS(dbus_display_console_parent_class)->dispose(object);
@@ -179,7 +178,7 @@ listener_vanished_cb(DBusDisplayListener *listener)
 
     trace_dbus_listener_vanished(name);
 
-    g_hash_table_remove(ddc->listeners, name);
+    g_ptr_array_remove_fast(ddc->listeners, listener);
     qkbd_state_lift_all_keys(ddc->kbd);
 }
 
@@ -267,16 +266,6 @@ dbus_console_register_listener(DBusDisplayConsole *ddc,
     DBusDisplayListener *listener;
     int fd;
 
-    if (sender && g_hash_table_contains(ddc->listeners, sender)) {
-        g_dbus_method_invocation_return_error(
-            invocation,
-            DBUS_DISPLAY_ERROR,
-            DBUS_DISPLAY_ERROR_INVALID,
-            "`%s` is already registered!",
-            sender);
-        return DBUS_METHOD_INVOCATION_HANDLED;
-    }
-
 #ifdef G_OS_WIN32
     if (!dbus_win32_import_socket(invocation, arg_listener, &fd)) {
         return DBUS_METHOD_INVOCATION_HANDLED;
@@ -331,9 +320,7 @@ dbus_console_register_listener(DBusDisplayConsole *ddc,
         return DBUS_METHOD_INVOCATION_HANDLED;
     }
 
-    g_hash_table_insert(ddc->listeners,
-                        (gpointer)dbus_display_listener_get_bus_name(listener),
-                        listener);
+    g_ptr_array_add(ddc->listeners, listener);
     g_object_connect(listener_conn,
                      "swapped-signal::closed", listener_vanished_cb, listener,
                      NULL);
-- 
2.45.2.827.g557ae147e6

[PATCH 11/16] ui/dbus: add trace for can_share_map

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus-listener.c | 1 +
 ui/trace-events    | 1 +
 2 files changed, 2 insertions(+)

diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 020c05198b..f55e197768 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -1083,6 +1083,7 @@ dbus_display_listener_new(const char *bus_name,
     ddl->console = console;
 
     dbus_display_listener_setup_shared_map(ddl);
+    trace_dbus_can_share_map(ddl->can_share_map);
     dbus_display_listener_setup_d3d11(ddl);
 
     con = qemu_console_lookup_by_index(dbus_display_console_get_index(console));
diff --git a/ui/trace-events b/ui/trace-events
index fb253c1666..3da0d5e280 100644
--- a/ui/trace-events
+++ b/ui/trace-events
@@ -166,6 +166,7 @@ dbus_clipboard_unregister(const char *bus_name) "peer %s"
 dbus_scanout_texture(uint32_t tex_id, bool backing_y_0_top, uint32_t backing_width, uint32_t backing_height, uint32_t x, uint32_t y, uint32_t w, uint32_t h) "tex_id:%u y0top:%d back:%ux%u %u+%u-%ux%u"
 dbus_gl_gfx_switch(void *p) "surf: %p"
 dbus_filter(unsigned int serial, unsigned int filter) "serial=%u (<= %u)"
+dbus_can_share_map(bool share) "can_share_map: %d"
 
 # egl-helpers.c
 egl_init_d3d11_device(void *p) "d3d device: %p"
-- 
2.45.2.827.g557ae147e6

[PATCH 12/16] ui/surface: allocate shared memory on !win32

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Use qemu_memfd_alloc() to allocate the display surface memory, which
will fallback on tmpfile/mmap() on systems without memfd, and allow to
share the display with other processes.

This is similar to how display memory is allocated on win32 since commit
09b4c198 ("console/win32: allocate shareable display surface").

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/ui/surface.h |  8 ++++++++
 ui/console.c         | 30 ++++++++++++++++++++++++++++--
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/include/ui/surface.h b/include/ui/surface.h
index 345b19169d..dacf12ffe2 100644
--- a/include/ui/surface.h
+++ b/include/ui/surface.h
@@ -23,6 +23,10 @@ typedef struct DisplaySurface {
     GLenum gltype;
     GLuint texture;
 #endif
+#ifndef WIN32
+    int shmfd;
+    uint32_t shmfd_offset;
+#endif
 #ifdef WIN32
     HANDLE handle;
     uint32_t handle_offset;
@@ -37,6 +41,10 @@ DisplaySurface *qemu_create_displaysurface_from(int width, int height,
 DisplaySurface *qemu_create_displaysurface_pixman(pixman_image_t *image);
 DisplaySurface *qemu_create_placeholder_surface(int w, int h,
                                                 const char *msg);
+#ifndef WIN32
+void qemu_displaysurface_set_shmfd(DisplaySurface *surface,
+                                   int shmfd, uint32_t offset);
+#endif
 #ifdef WIN32
 void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
                                           HANDLE h, uint32_t offset);
diff --git a/ui/console.c b/ui/console.c
index fdd76c2be4..56f2462c3d 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -37,6 +37,7 @@
 #include "trace.h"
 #include "exec/memory.h"
 #include "qom/object.h"
+#include "qemu/memfd.h"
 
 #include "console-priv.h"
 
@@ -452,6 +453,17 @@ qemu_graphic_console_init(Object *obj)
 {
 }
 
+#ifndef WIN32
+void qemu_displaysurface_set_shmfd(DisplaySurface *surface,
+                                   int shmfd, uint32_t offset)
+{
+    assert(surface->shmfd == -1);
+
+    surface->shmfd = shmfd;
+    surface->shmfd_offset = offset;
+}
+#endif
+
 #ifdef WIN32
 void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
                                           HANDLE h, uint32_t offset)
@@ -469,12 +481,16 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
     void *bits = NULL;
 #ifdef WIN32
     HANDLE handle = NULL;
+#else
+    int shmfd = -1;
 #endif
 
     trace_displaysurface_create(width, height);
 
 #ifdef WIN32
     bits = qemu_win32_map_alloc(width * height * 4, &handle, &error_abort);
+#else
+    bits = qemu_memfd_alloc("displaysurface", width * height * 4, 0, &shmfd, &error_abort);
 #endif
 
     surface = qemu_create_displaysurface_from(
@@ -486,9 +502,13 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
 
 #ifdef WIN32
     qemu_displaysurface_win32_set_handle(surface, handle, 0);
-    pixman_image_set_destroy_function(surface->image,
-                                      qemu_pixman_shared_image_destroy, handle);
+    void *data = handle;
+#else
+    qemu_displaysurface_set_shmfd(surface, shmfd, 0);
+    void *data = GINT_TO_POINTER(shmfd);
 #endif
+    pixman_image_set_destroy_function(surface->image, qemu_pixman_shared_image_destroy, data);
+
     return surface;
 }
 
@@ -499,6 +519,9 @@ DisplaySurface *qemu_create_displaysurface_from(int width, int height,
     DisplaySurface *surface = g_new0(DisplaySurface, 1);
 
     trace_displaysurface_create_from(surface, width, height, format);
+#ifndef WIN32
+    surface->shmfd = -1;
+#endif
     surface->image = pixman_image_create_bits(format,
                                               width, height,
                                               (void *)data, linesize);
@@ -512,6 +535,9 @@ DisplaySurface *qemu_create_displaysurface_pixman(pixman_image_t *image)
     DisplaySurface *surface = g_new0(DisplaySurface, 1);
 
     trace_displaysurface_create_pixman(surface);
+#ifndef WIN32
+    surface->shmfd = -1;
+#endif
     surface->image = pixman_image_ref(image);
 
     return surface;
-- 
2.45.2.827.g557ae147e6

Re: [PATCH 12/16] ui/surface: allocate shared memory on !win32

[Akihiko Odaki]

On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Use qemu_memfd_alloc() to allocate the display surface memory, which
> will fallback on tmpfile/mmap() on systems without memfd, and allow to
> share the display with other processes.
> 
> This is similar to how display memory is allocated on win32 since commit
> 09b4c198 ("console/win32: allocate shareable display surface").
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   include/ui/surface.h |  8 ++++++++
>   ui/console.c         | 30 ++++++++++++++++++++++++++++--
>   2 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/include/ui/surface.h b/include/ui/surface.h
> index 345b19169d..dacf12ffe2 100644
> --- a/include/ui/surface.h
> +++ b/include/ui/surface.h
> @@ -23,6 +23,10 @@ typedef struct DisplaySurface {
>       GLenum gltype;
>       GLuint texture;
>   #endif
> +#ifndef WIN32
> +    int shmfd;
> +    uint32_t shmfd_offset;
> +#endif
>   #ifdef WIN32
>       HANDLE handle;

What about defining a new struct that contains either of shmfd or 
handle? We can then have a unified set of functions that uses the struct 
to allocate/free a shared pixman image and to set one to DisplaySurface.

>       uint32_t handle_offset;
> @@ -37,6 +41,10 @@ DisplaySurface *qemu_create_displaysurface_from(int width, int height,
>   DisplaySurface *qemu_create_displaysurface_pixman(pixman_image_t *image);
>   DisplaySurface *qemu_create_placeholder_surface(int w, int h,
>                                                   const char *msg);
> +#ifndef WIN32
> +void qemu_displaysurface_set_shmfd(DisplaySurface *surface,
> +                                   int shmfd, uint32_t offset);
> +#endif
>   #ifdef WIN32
>   void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
>                                             HANDLE h, uint32_t offset);
> diff --git a/ui/console.c b/ui/console.c
> index fdd76c2be4..56f2462c3d 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -37,6 +37,7 @@
>   #include "trace.h"
>   #include "exec/memory.h"
>   #include "qom/object.h"
> +#include "qemu/memfd.h"
>   
>   #include "console-priv.h"
>   
> @@ -452,6 +453,17 @@ qemu_graphic_console_init(Object *obj)
>   {
>   }
>   
> +#ifndef WIN32
> +void qemu_displaysurface_set_shmfd(DisplaySurface *surface,
> +                                   int shmfd, uint32_t offset)
> +{
> +    assert(surface->shmfd == -1);
> +
> +    surface->shmfd = shmfd;
> +    surface->shmfd_offset = offset;
> +}
> +#endif
> +
>   #ifdef WIN32
>   void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
>                                             HANDLE h, uint32_t offset)
> @@ -469,12 +481,16 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
>       void *bits = NULL;
>   #ifdef WIN32
>       HANDLE handle = NULL;
> +#else
> +    int shmfd = -1;
>   #endif
>   
>       trace_displaysurface_create(width, height);
>   
>   #ifdef WIN32
>       bits = qemu_win32_map_alloc(width * height * 4, &handle, &error_abort);
> +#else
> +    bits = qemu_memfd_alloc("displaysurface", width * height * 4, 0, &shmfd, &error_abort);
>   #endif
>   
>       surface = qemu_create_displaysurface_from(
> @@ -486,9 +502,13 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
>   
>   #ifdef WIN32
>       qemu_displaysurface_win32_set_handle(surface, handle, 0);
> -    pixman_image_set_destroy_function(surface->image,
> -                                      qemu_pixman_shared_image_destroy, handle);
> +    void *data = handle;
> +#else
> +    qemu_displaysurface_set_shmfd(surface, shmfd, 0);
> +    void *data = GINT_TO_POINTER(shmfd);
>   #endif
> +    pixman_image_set_destroy_function(surface->image, qemu_pixman_shared_image_destroy, data);
> +
>       return surface;
>   }
>   
> @@ -499,6 +519,9 @@ DisplaySurface *qemu_create_displaysurface_from(int width, int height,
>       DisplaySurface *surface = g_new0(DisplaySurface, 1);
>   
>       trace_displaysurface_create_from(surface, width, height, format);
> +#ifndef WIN32
> +    surface->shmfd = -1;
> +#endif
>       surface->image = pixman_image_create_bits(format,
>                                                 width, height,
>                                                 (void *)data, linesize);
> @@ -512,6 +535,9 @@ DisplaySurface *qemu_create_displaysurface_pixman(pixman_image_t *image)
>       DisplaySurface *surface = g_new0(DisplaySurface, 1);
>   
>       trace_displaysurface_create_pixman(surface);
> +#ifndef WIN32
> +    surface->shmfd = -1;
> +#endif
>       surface->image = pixman_image_ref(image);
>   
>       return surface;

Re: [PATCH 12/16] ui/surface: allocate shared memory on !win32

[Marc-André Lureau]

On Sat, Oct 5, 2024 at 12:59 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Use qemu_memfd_alloc() to allocate the display surface memory, which
> > will fallback on tmpfile/mmap() on systems without memfd, and allow to
> > share the display with other processes.
> >
> > This is similar to how display memory is allocated on win32 since commit
> > 09b4c198 ("console/win32: allocate shareable display surface").
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   include/ui/surface.h |  8 ++++++++
> >   ui/console.c         | 30 ++++++++++++++++++++++++++++--
> >   2 files changed, 36 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/ui/surface.h b/include/ui/surface.h
> > index 345b19169d..dacf12ffe2 100644
> > --- a/include/ui/surface.h
> > +++ b/include/ui/surface.h
> > @@ -23,6 +23,10 @@ typedef struct DisplaySurface {
> >       GLenum gltype;
> >       GLuint texture;
> >   #endif
> > +#ifndef WIN32
> > +    int shmfd;
> > +    uint32_t shmfd_offset;
> > +#endif
> >   #ifdef WIN32
> >       HANDLE handle;
>
> What about defining a new struct that contains either of shmfd or
> handle? We can then have a unified set of functions that uses the struct
> to allocate/free a shared pixman image and to set one to DisplaySurface.

Well, that structure is pretty much DisplaySurface. I am not sure if
it's valuable to introduce another abstraction.

>
> >       uint32_t handle_offset;
> > @@ -37,6 +41,10 @@ DisplaySurface *qemu_create_displaysurface_from(int width, int height,
> >   DisplaySurface *qemu_create_displaysurface_pixman(pixman_image_t *image);
> >   DisplaySurface *qemu_create_placeholder_surface(int w, int h,
> >                                                   const char *msg);
> > +#ifndef WIN32
> > +void qemu_displaysurface_set_shmfd(DisplaySurface *surface,
> > +                                   int shmfd, uint32_t offset);
> > +#endif
> >   #ifdef WIN32
> >   void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
> >                                             HANDLE h, uint32_t offset);
> > diff --git a/ui/console.c b/ui/console.c
> > index fdd76c2be4..56f2462c3d 100644
> > --- a/ui/console.c
> > +++ b/ui/console.c
> > @@ -37,6 +37,7 @@
> >   #include "trace.h"
> >   #include "exec/memory.h"
> >   #include "qom/object.h"
> > +#include "qemu/memfd.h"
> >
> >   #include "console-priv.h"
> >
> > @@ -452,6 +453,17 @@ qemu_graphic_console_init(Object *obj)
> >   {
> >   }
> >
> > +#ifndef WIN32
> > +void qemu_displaysurface_set_shmfd(DisplaySurface *surface,
> > +                                   int shmfd, uint32_t offset)
> > +{
> > +    assert(surface->shmfd == -1);
> > +
> > +    surface->shmfd = shmfd;
> > +    surface->shmfd_offset = offset;
> > +}
> > +#endif
> > +
> >   #ifdef WIN32
> >   void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
> >                                             HANDLE h, uint32_t offset)
> > @@ -469,12 +481,16 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
> >       void *bits = NULL;
> >   #ifdef WIN32
> >       HANDLE handle = NULL;
> > +#else
> > +    int shmfd = -1;
> >   #endif
> >
> >       trace_displaysurface_create(width, height);
> >
> >   #ifdef WIN32
> >       bits = qemu_win32_map_alloc(width * height * 4, &handle, &error_abort);
> > +#else
> > +    bits = qemu_memfd_alloc("displaysurface", width * height * 4, 0, &shmfd, &error_abort);
> >   #endif
> >
> >       surface = qemu_create_displaysurface_from(
> > @@ -486,9 +502,13 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
> >
> >   #ifdef WIN32
> >       qemu_displaysurface_win32_set_handle(surface, handle, 0);
> > -    pixman_image_set_destroy_function(surface->image,
> > -                                      qemu_pixman_shared_image_destroy, handle);
> > +    void *data = handle;
> > +#else
> > +    qemu_displaysurface_set_shmfd(surface, shmfd, 0);
> > +    void *data = GINT_TO_POINTER(shmfd);
> >   #endif
> > +    pixman_image_set_destroy_function(surface->image, qemu_pixman_shared_image_destroy, data);
> > +
> >       return surface;
> >   }
> >
> > @@ -499,6 +519,9 @@ DisplaySurface *qemu_create_displaysurface_from(int width, int height,
> >       DisplaySurface *surface = g_new0(DisplaySurface, 1);
> >
> >       trace_displaysurface_create_from(surface, width, height, format);
> > +#ifndef WIN32
> > +    surface->shmfd = -1;
> > +#endif
> >       surface->image = pixman_image_create_bits(format,
> >                                                 width, height,
> >                                                 (void *)data, linesize);
> > @@ -512,6 +535,9 @@ DisplaySurface *qemu_create_displaysurface_pixman(pixman_image_t *image)
> >       DisplaySurface *surface = g_new0(DisplaySurface, 1);
> >
> >       trace_displaysurface_create_pixman(surface);
> > +#ifndef WIN32
> > +    surface->shmfd = -1;
> > +#endif
> >       surface->image = pixman_image_ref(image);
> >
> >       return surface;
>

Re: [PATCH 12/16] ui/surface: allocate shared memory on !win32

[Akihiko Odaki]

On 2024/10/07 20:47, Marc-André Lureau wrote:
> On Sat, Oct 5, 2024 at 12:59 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> Use qemu_memfd_alloc() to allocate the display surface memory, which
>>> will fallback on tmpfile/mmap() on systems without memfd, and allow to
>>> share the display with other processes.
>>>
>>> This is similar to how display memory is allocated on win32 since commit
>>> 09b4c198 ("console/win32: allocate shareable display surface").
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>    include/ui/surface.h |  8 ++++++++
>>>    ui/console.c         | 30 ++++++++++++++++++++++++++++--
>>>    2 files changed, 36 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/ui/surface.h b/include/ui/surface.h
>>> index 345b19169d..dacf12ffe2 100644
>>> --- a/include/ui/surface.h
>>> +++ b/include/ui/surface.h
>>> @@ -23,6 +23,10 @@ typedef struct DisplaySurface {
>>>        GLenum gltype;
>>>        GLuint texture;
>>>    #endif
>>> +#ifndef WIN32
>>> +    int shmfd;
>>> +    uint32_t shmfd_offset;
>>> +#endif
>>>    #ifdef WIN32
>>>        HANDLE handle;
>>
>> What about defining a new struct that contains either of shmfd or
>> handle? We can then have a unified set of functions that uses the struct
>> to allocate/free a shared pixman image and to set one to DisplaySurface.
> 
> Well, that structure is pretty much DisplaySurface. I am not sure if
> it's valuable to introduce another abstraction.

I was thinking using the abstraction for virtio-gpu to save ifdefs. It 
already has resource_set_image_destroy to abstract away the resource 
deallocation so ui/console.c can absorb it.

> 
>>
>>>        uint32_t handle_offset;
>>> @@ -37,6 +41,10 @@ DisplaySurface *qemu_create_displaysurface_from(int width, int height,
>>>    DisplaySurface *qemu_create_displaysurface_pixman(pixman_image_t *image);
>>>    DisplaySurface *qemu_create_placeholder_surface(int w, int h,
>>>                                                    const char *msg);
>>> +#ifndef WIN32
>>> +void qemu_displaysurface_set_shmfd(DisplaySurface *surface,
>>> +                                   int shmfd, uint32_t offset);
>>> +#endif
>>>    #ifdef WIN32
>>>    void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
>>>                                              HANDLE h, uint32_t offset);
>>> diff --git a/ui/console.c b/ui/console.c
>>> index fdd76c2be4..56f2462c3d 100644
>>> --- a/ui/console.c
>>> +++ b/ui/console.c
>>> @@ -37,6 +37,7 @@
>>>    #include "trace.h"
>>>    #include "exec/memory.h"
>>>    #include "qom/object.h"
>>> +#include "qemu/memfd.h"
>>>
>>>    #include "console-priv.h"
>>>
>>> @@ -452,6 +453,17 @@ qemu_graphic_console_init(Object *obj)
>>>    {
>>>    }
>>>
>>> +#ifndef WIN32
>>> +void qemu_displaysurface_set_shmfd(DisplaySurface *surface,
>>> +                                   int shmfd, uint32_t offset)
>>> +{
>>> +    assert(surface->shmfd == -1);
>>> +
>>> +    surface->shmfd = shmfd;
>>> +    surface->shmfd_offset = offset;
>>> +}
>>> +#endif
>>> +
>>>    #ifdef WIN32
>>>    void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
>>>                                              HANDLE h, uint32_t offset)
>>> @@ -469,12 +481,16 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
>>>        void *bits = NULL;
>>>    #ifdef WIN32
>>>        HANDLE handle = NULL;
>>> +#else
>>> +    int shmfd = -1;
>>>    #endif
>>>
>>>        trace_displaysurface_create(width, height);
>>>
>>>    #ifdef WIN32
>>>        bits = qemu_win32_map_alloc(width * height * 4, &handle, &error_abort);
>>> +#else
>>> +    bits = qemu_memfd_alloc("displaysurface", width * height * 4, 0, &shmfd, &error_abort);
>>>    #endif
>>>
>>>        surface = qemu_create_displaysurface_from(
>>> @@ -486,9 +502,13 @@ DisplaySurface *qemu_create_displaysurface(int width, int height)
>>>
>>>    #ifdef WIN32
>>>        qemu_displaysurface_win32_set_handle(surface, handle, 0);
>>> -    pixman_image_set_destroy_function(surface->image,
>>> -                                      qemu_pixman_shared_image_destroy, handle);
>>> +    void *data = handle;
>>> +#else
>>> +    qemu_displaysurface_set_shmfd(surface, shmfd, 0);
>>> +    void *data = GINT_TO_POINTER(shmfd);
>>>    #endif
>>> +    pixman_image_set_destroy_function(surface->image, qemu_pixman_shared_image_destroy, data);
>>> +
>>>        return surface;
>>>    }
>>>
>>> @@ -499,6 +519,9 @@ DisplaySurface *qemu_create_displaysurface_from(int width, int height,
>>>        DisplaySurface *surface = g_new0(DisplaySurface, 1);
>>>
>>>        trace_displaysurface_create_from(surface, width, height, format);
>>> +#ifndef WIN32
>>> +    surface->shmfd = -1;
>>> +#endif
>>>        surface->image = pixman_image_create_bits(format,
>>>                                                  width, height,
>>>                                                  (void *)data, linesize);
>>> @@ -512,6 +535,9 @@ DisplaySurface *qemu_create_displaysurface_pixman(pixman_image_t *image)
>>>        DisplaySurface *surface = g_new0(DisplaySurface, 1);
>>>
>>>        trace_displaysurface_create_pixman(surface);
>>> +#ifndef WIN32
>>> +    surface->shmfd = -1;
>>> +#endif
>>>        surface->image = pixman_image_ref(image);
>>>
>>>        return surface;
>>
> 

[PATCH 13/16] ui/dbus: add Listener.Unix.Map interface XML

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus-display1.xml | 45 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/ui/dbus-display1.xml b/ui/dbus-display1.xml
index ce35d64eea..4ae14288bc 100644
--- a/ui/dbus-display1.xml
+++ b/ui/dbus-display1.xml
@@ -469,6 +469,51 @@
     <property name="Interfaces" type="as" access="read"/>
   </interface>
 
+  <!--
+      org.qemu.Display1.Listener.Unix.Map:
+
+      This optional client-side interface can complement
+      org.qemu.Display1.Listener on ``/org/qemu/Display1/Listener`` for
+      Unix-specific shared memory scanouts.
+  -->
+  <interface name="org.qemu.Display1.Listener.Unix.Map">
+    <!--
+        ScanoutMap:
+        @handle: the shared map FD.
+        @offset: mapping offset, in bytes.
+        @width: display width, in pixels.
+        @height: display height, in pixels.
+        @stride: stride, in bytes.
+        @pixman_format: image format (ex: ``PIXMAN_X8R8G8B8``).
+
+        Resize and update the display content with a shared map.
+    -->
+    <method name="ScanoutMap">
+      <arg type="h" name="handle" direction="in"/>
+      <arg type="u" name="offset" direction="in"/>
+      <arg type="u" name="width" direction="in"/>
+      <arg type="u" name="height" direction="in"/>
+      <arg type="u" name="stride" direction="in"/>
+      <arg type="u" name="pixman_format" direction="in"/>
+    </method>
+
+    <!--
+        UpdateMap:
+        @x: the X update position, in pixels.
+        @y: the Y update position, in pixels.
+        @width: the update width, in pixels.
+        @height: the update height, in pixels.
+
+        Update the display content with the current shared map and the given region.
+    -->
+    <method name="UpdateMap">
+      <arg type="i" name="x" direction="in"/>
+      <arg type="i" name="y" direction="in"/>
+      <arg type="i" name="width" direction="in"/>
+      <arg type="i" name="height" direction="in"/>
+    </method>
+  </interface>
+
   <!--
       org.qemu.Display1.Listener.Win32.Map:
 
-- 
2.45.2.827.g557ae147e6

[PATCH 14/16] ui/dbus: implement Unix.Map

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus-listener.c | 83 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 78 insertions(+), 5 deletions(-)

diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index f55e197768..cc665f2f11 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -75,6 +75,9 @@ struct _DBusDisplayListener {
     bool ds_mapped;
     bool can_share_map;
 
+#ifndef WIN32
+    QemuDBusDisplay1ListenerUnixMap *map_proxy;
+#endif
 #ifdef WIN32
     QemuDBusDisplay1ListenerWin32Map *map_proxy;
     QemuDBusDisplay1ListenerWin32D3d11 *d3d11_proxy;
@@ -323,6 +326,53 @@ static void dbus_scanout_dmabuf(DisplayChangeListener *dcl,
 #endif /* GBM */
 #endif /* OPENGL */
 
+#ifndef WIN32
+static bool dbus_scanout_map(DBusDisplayListener *ddl)
+{
+    g_autoptr(GError) err = NULL;
+    g_autoptr(GUnixFDList) fd_list = NULL;
+
+    if (ddl->ds_share == SHARE_KIND_MAPPED) {
+        return true;
+    }
+
+    if (!ddl->can_share_map || ddl->ds->shmfd == -1) {
+        return false;
+    }
+
+    ddl_discard_display_messages(ddl);
+    fd_list = g_unix_fd_list_new();
+    if (g_unix_fd_list_append(fd_list, ddl->ds->shmfd, &err) != 0) {
+        g_debug("Failed to setup scanout map fdlist: %s", err->message);
+        ddl->can_share_map = false;
+        return false;
+    }
+
+    if (!qemu_dbus_display1_listener_unix_map_call_scanout_map_sync(
+            ddl->map_proxy,
+            g_variant_new_handle(0),
+            ddl->ds->shmfd_offset,
+            surface_width(ddl->ds),
+            surface_height(ddl->ds),
+            surface_stride(ddl->ds),
+            surface_format(ddl->ds),
+            G_DBUS_CALL_FLAGS_NONE,
+            DBUS_DEFAULT_TIMEOUT,
+            fd_list,
+            NULL,
+            NULL,
+            &err)) {
+        g_debug("Failed to call ScanoutMap: %s", err->message);
+        ddl->can_share_map = false;
+        return false;
+    }
+
+    ddl->ds_share = SHARE_KIND_MAPPED;
+
+    return true;
+}
+#endif /* !WIN32 */
+
 #ifdef WIN32
 static bool dbus_scanout_map(DBusDisplayListener *ddl)
 {
@@ -693,16 +743,22 @@ static void dbus_gfx_update(DisplayChangeListener *dcl,
 
     trace_dbus_update(x, y, w, h);
 
-#ifdef WIN32
     if (dbus_scanout_map(ddl)) {
+#ifndef WIN32
+        qemu_dbus_display1_listener_unix_map_call_update_map(
+            ddl->map_proxy,
+            x, y, w, h,
+            G_DBUS_CALL_FLAGS_NONE,
+            DBUS_DEFAULT_TIMEOUT, NULL, NULL, NULL);
+#else
         qemu_dbus_display1_listener_win32_map_call_update_map(
             ddl->map_proxy,
             x, y, w, h,
             G_DBUS_CALL_FLAGS_NONE,
             DBUS_DEFAULT_TIMEOUT, NULL, NULL, NULL);
+#endif
         return;
     }
-#endif
 
     if (x == 0 && y == 0 && w == surface_width(ddl->ds) && h == surface_height(ddl->ds)) {
         return ddl_scanout(ddl);
@@ -879,7 +935,6 @@ dbus_display_listener_get_console(DBusDisplayListener *ddl)
     return ddl->console;
 }
 
-#ifdef WIN32
 static bool
 dbus_display_listener_implements(DBusDisplayListener *ddl, const char *iface)
 {
@@ -894,6 +949,7 @@ dbus_display_listener_implements(DBusDisplayListener *ddl, const char *iface)
     return implements;
 }
 
+#ifdef WIN32
 static bool
 dbus_display_listener_setup_peer_process(DBusDisplayListener *ddl)
 {
@@ -976,10 +1032,27 @@ dbus_display_listener_setup_d3d11(DBusDisplayListener *ddl)
 static void
 dbus_display_listener_setup_shared_map(DBusDisplayListener *ddl)
 {
-#ifdef WIN32
     g_autoptr(GError) err = NULL;
 
-    if (!dbus_display_listener_implements(ddl, "org.qemu.Display1.Listener.Win32.Map")) {
+#ifndef WIN32
+    if (!dbus_display_listener_implements(
+            ddl, "org.qemu.Display1.Listener.Unix.Map")) {
+        return;
+    }
+    ddl->map_proxy = qemu_dbus_display1_listener_unix_map_proxy_new_sync(
+        ddl->conn, G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START, NULL,
+        "/org/qemu/Display1/Listener", NULL, &err);
+    if (!ddl->map_proxy) {
+        g_debug("Failed to setup Unix map proxy: %s", err->message);
+        return;
+    }
+
+    ddl->can_share_map = true;
+#endif
+
+#ifdef WIN32
+    if (!dbus_display_listener_implements(
+            ddl, "org.qemu.Display1.Listener.Win32.Map")) {
         return;
     }
 
-- 
2.45.2.827.g557ae147e6

[PATCH 15/16] virtio-gpu: allocate shareable 2d resources on !win32

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Similar to what was done in commit 9462ff46 ("virtio-gpu/win32: allocate
shareable 2d resources/images") for win32, allocate resource memory with
memfd, so the associated display surface memory can be shared with a
different process.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/hw/virtio/virtio-gpu.h |  4 ++-
 hw/display/virtio-gpu.c        | 47 ++++++++++++++++++++++------------
 2 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 7a59379f5a..a0d715198b 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -51,7 +51,9 @@ struct virtio_gpu_simple_resource {
     unsigned int iov_cnt;
     uint32_t scanout_bitmask;
     pixman_image_t *image;
-#ifdef WIN32
+#ifndef WIN32
+    int shmfd;
+#else
     HANDLE handle;
 #endif
     uint64_t hostmem;
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 77f6e76f23..295d0afca3 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -28,6 +28,7 @@
 #include "hw/virtio/virtio-bus.h"
 #include "hw/qdev-properties.h"
 #include "qemu/log.h"
+#include "qemu/memfd.h"
 #include "qemu/module.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
@@ -238,6 +239,20 @@ static uint32_t calc_image_hostmem(pixman_format_code_t pformat,
     return height * stride;
 }
 
+static void
+resource_set_image_destroy(struct virtio_gpu_simple_resource *res)
+{
+    if (!res) {
+        return;
+    }
+#ifndef WIN32
+    void *data = GINT_TO_POINTER(res->shmfd);
+#else
+    void *data = res->handle;
+#endif
+    pixman_image_set_destroy_function(res->image, qemu_pixman_shared_image_destroy, data);
+}
+
 static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
                                           struct virtio_gpu_ctrl_command *cmd)
 {
@@ -285,27 +300,23 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
     res->hostmem = calc_image_hostmem(pformat, c2d.width, c2d.height);
     if (res->hostmem + g->hostmem < g->conf_max_hostmem) {
         void *bits = NULL;
-#ifdef WIN32
+#ifndef WIN32
+        bits = qemu_memfd_alloc("virtio-gpu-res", res->hostmem, 0, &res->shmfd, &error_warn);
+#else
         bits = qemu_win32_map_alloc(res->hostmem, &res->handle, &error_warn);
+#endif
         if (!bits) {
             goto end;
         }
-#endif
         res->image = pixman_image_create_bits(
             pformat,
             c2d.width,
             c2d.height,
             bits, c2d.height ? res->hostmem / c2d.height : 0);
-#ifdef WIN32
-        if (res->image) {
-            pixman_image_set_destroy_function(res->image, qemu_pixman_shared_image_destroy, res->handle);
-        }
-#endif
+        resource_set_image_destroy(res);
     }
 
-#ifdef WIN32
 end:
-#endif
     if (!res->image) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: resource creation failed %d %d %d\n",
@@ -676,7 +687,9 @@ static bool virtio_gpu_do_set_scanout(VirtIOGPU *g,
 
         /* realloc the surface ptr */
         scanout->ds = qemu_create_displaysurface_pixman(rect);
-#ifdef WIN32
+#ifndef WIN32
+        qemu_displaysurface_set_shmfd(scanout->ds, res->shmfd, fb->offset);
+#else
         qemu_displaysurface_win32_set_handle(scanout->ds, res->handle, fb->offset);
 #endif
 
@@ -1301,13 +1314,15 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
         }
 
         res->hostmem = calc_image_hostmem(pformat, res->width, res->height);
-#ifdef WIN32
+#ifndef WIN32
+        bits = qemu_memfd_alloc("virtio-gpu-res", res->hostmem, 0, &res->shmfd, &error_warn);
+#else
         bits = qemu_win32_map_alloc(res->hostmem, &res->handle, &error_warn);
+#endif
         if (!bits) {
             g_free(res);
             return -EINVAL;
         }
-#endif
         res->image = pixman_image_create_bits(
             pformat,
             res->width, res->height,
@@ -1316,9 +1331,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
             g_free(res);
             return -EINVAL;
         }
-#ifdef WIN32
-        pixman_image_set_destroy_function(res->image, qemu_pixman_shared_image_destroy, res->handle);
-#endif
+        resource_set_image_destroy(res);
 
         res->addrs = g_new(uint64_t, res->iov_cnt);
         res->iov = g_new(struct iovec, res->iov_cnt);
@@ -1451,7 +1464,9 @@ static int virtio_gpu_post_load(void *opaque, int version_id)
                 return -EINVAL;
             }
             scanout->ds = qemu_create_displaysurface_pixman(res->image);
-#ifdef WIN32
+#ifndef WIN32
+            qemu_displaysurface_set_shmfd(scanout->ds, res->shmfd, 0);
+#else
             qemu_displaysurface_win32_set_handle(scanout->ds, res->handle, 0);
 #endif
             dpy_gfx_replace_surface(scanout->con, scanout->ds);
-- 
2.45.2.827.g557ae147e6

[PATCH 16/16] tests: add basic -display dbus Map.Unix test

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Only check we eventually get a shared memory scanout.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/qtest/dbus-display-test.c | 64 ++++++++++++++++++++++++++++++---
 1 file changed, 59 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/dbus-display-test.c b/tests/qtest/dbus-display-test.c
index 0390bdcb41..ac92cb00d4 100644
--- a/tests/qtest/dbus-display-test.c
+++ b/tests/qtest/dbus-display-test.c
@@ -2,9 +2,12 @@
 #include "qemu/sockets.h"
 #include "qemu/dbus.h"
 #include "qemu/sockets.h"
+#include "glib.h"
+#include "glibconfig.h"
 #include <gio/gio.h>
 #include <gio/gunixfdlist.h>
 #include "libqtest.h"
+#include <sys/mman.h>
 #include "ui/dbus-display1.h"
 
 static GDBusConnection*
@@ -82,6 +85,7 @@ typedef struct TestDBusConsoleRegister {
     GThread *thread;
     GDBusConnection *listener_conn;
     GDBusObjectManagerServer *server;
+    bool with_map;
 } TestDBusConsoleRegister;
 
 static gboolean listener_handle_scanout(
@@ -94,13 +98,48 @@ static gboolean listener_handle_scanout(
     GVariant *arg_data,
     TestDBusConsoleRegister *test)
 {
+    if (!test->with_map) {
+        g_main_loop_quit(test->loop);
+    }
+
+    return DBUS_METHOD_INVOCATION_HANDLED;
+}
+
+static gboolean listener_handle_scanout_map(
+    QemuDBusDisplay1ListenerUnixMap *object,
+    GDBusMethodInvocation *invocation,
+    GUnixFDList *fd_list,
+    GVariant *arg_handle,
+    guint arg_offset,
+    guint arg_width,
+    guint arg_height,
+    guint arg_stride,
+    guint arg_pixman_format,
+    TestDBusConsoleRegister *test)
+{
+    int fd = -1;
+    gint32 handle = g_variant_get_handle(arg_handle);
+    g_autoptr(GError) error = NULL;
+    void *addr = NULL;
+    size_t len = arg_height * arg_stride;
+
+    g_assert_cmpuint(g_unix_fd_list_get_length(fd_list), ==, 1);
+    fd = g_unix_fd_list_get(fd_list, handle, &error);
+    g_assert_no_error(error);
+
+    addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, arg_offset);
+    g_assert_no_errno(GPOINTER_TO_INT(addr));
+    g_assert_nonnull(addr);
+    g_assert_no_errno(munmap(addr, len));
+
     g_main_loop_quit(test->loop);
 
+    close(fd);
     return DBUS_METHOD_INVOCATION_HANDLED;
 }
 
 static void
-test_dbus_console_setup_listener(TestDBusConsoleRegister *test)
+test_dbus_console_setup_listener(TestDBusConsoleRegister *test, bool with_map)
 {
     g_autoptr(GDBusObjectSkeleton) listener = NULL;
     g_autoptr(QemuDBusDisplay1ListenerSkeleton) iface = NULL;
@@ -114,6 +153,20 @@ test_dbus_console_setup_listener(TestDBusConsoleRegister *test)
                      NULL);
     g_dbus_object_skeleton_add_interface(listener,
                                          G_DBUS_INTERFACE_SKELETON(iface));
+    if (with_map) {
+        g_autoptr(QemuDBusDisplay1ListenerUnixMapSkeleton) iface_map =
+            QEMU_DBUS_DISPLAY1_LISTENER_UNIX_MAP_SKELETON(
+                qemu_dbus_display1_listener_unix_map_skeleton_new());
+
+        g_object_connect(iface_map,
+                         "signal::handle-scanout-map", listener_handle_scanout_map, test,
+                         NULL);
+        g_dbus_object_skeleton_add_interface(listener,
+                                             G_DBUS_INTERFACE_SKELETON(iface_map));
+        g_object_set(iface, "interfaces",
+            (const gchar *[]) { "org.qemu.Display1.Listener.Unix.Map", NULL },
+            NULL);
+    }
     g_dbus_object_manager_server_export(test->server, listener);
     g_dbus_object_manager_server_set_connection(test->server,
                                                 test->listener_conn);
@@ -145,7 +198,7 @@ test_dbus_console_registered(GObject *source_object,
     g_assert_no_error(err);
 
     test->listener_conn = g_thread_join(test->thread);
-    test_dbus_console_setup_listener(test);
+    test_dbus_console_setup_listener(test, test->with_map);
 }
 
 static gpointer
@@ -155,7 +208,7 @@ test_dbus_p2p_server_setup_thread(gpointer data)
 }
 
 static void
-test_dbus_display_console(void)
+test_dbus_display_console(const void* data)
 {
     g_autoptr(GError) err = NULL;
     g_autoptr(GDBusConnection) conn = NULL;
@@ -163,7 +216,7 @@ test_dbus_display_console(void)
     g_autoptr(GMainLoop) loop = NULL;
     QTestState *qts = NULL;
     int pair[2];
-    TestDBusConsoleRegister test = { 0, };
+    TestDBusConsoleRegister test = { 0, .with_map = GPOINTER_TO_INT(data) };
 #ifdef WIN32
     WSAPROTOCOL_INFOW info;
     g_autoptr(GVariant) listener = NULL;
@@ -299,7 +352,8 @@ main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_func("/dbus-display/vm", test_dbus_display_vm);
-    qtest_add_func("/dbus-display/console", test_dbus_display_console);
+    qtest_add_data_func("/dbus-display/console", GINT_TO_POINTER(false), test_dbus_display_console);
+    qtest_add_data_func("/dbus-display/console/map", GINT_TO_POINTER(true), test_dbus_display_console);
     qtest_add_func("/dbus-display/keyboard", test_dbus_display_keyboard);
 
     return g_test_run();
-- 
2.45.2.827.g557ae147e6

Re: [PATCH 16/16] tests: add basic -display dbus Map.Unix test

[Akihiko Odaki]

On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Only check we eventually get a shared memory scanout.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   tests/qtest/dbus-display-test.c | 64 ++++++++++++++++++++++++++++++---
>   1 file changed, 59 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/qtest/dbus-display-test.c b/tests/qtest/dbus-display-test.c
> index 0390bdcb41..ac92cb00d4 100644
> --- a/tests/qtest/dbus-display-test.c
> +++ b/tests/qtest/dbus-display-test.c
> @@ -2,9 +2,12 @@
>   #include "qemu/sockets.h"
>   #include "qemu/dbus.h"
>   #include "qemu/sockets.h"
> +#include "glib.h"
> +#include "glibconfig.h"
>   #include <gio/gio.h>
>   #include <gio/gunixfdlist.h>
>   #include "libqtest.h"
> +#include <sys/mman.h>
>   #include "ui/dbus-display1.h"
>   
>   static GDBusConnection*
> @@ -82,6 +85,7 @@ typedef struct TestDBusConsoleRegister {
>       GThread *thread;
>       GDBusConnection *listener_conn;
>       GDBusObjectManagerServer *server;
> +    bool with_map;
>   } TestDBusConsoleRegister;
>   
>   static gboolean listener_handle_scanout(
> @@ -94,13 +98,48 @@ static gboolean listener_handle_scanout(
>       GVariant *arg_data,
>       TestDBusConsoleRegister *test)
>   {
> +    if (!test->with_map) {
> +        g_main_loop_quit(test->loop);
> +    }
> +
> +    return DBUS_METHOD_INVOCATION_HANDLED;
> +}
> +
> +static gboolean listener_handle_scanout_map(
> +    QemuDBusDisplay1ListenerUnixMap *object,
> +    GDBusMethodInvocation *invocation,
> +    GUnixFDList *fd_list,
> +    GVariant *arg_handle,
> +    guint arg_offset,
> +    guint arg_width,
> +    guint arg_height,
> +    guint arg_stride,
> +    guint arg_pixman_format,
> +    TestDBusConsoleRegister *test)
> +{
> +    int fd = -1;
> +    gint32 handle = g_variant_get_handle(arg_handle);
> +    g_autoptr(GError) error = NULL;
> +    void *addr = NULL;
> +    size_t len = arg_height * arg_stride;
> +
> +    g_assert_cmpuint(g_unix_fd_list_get_length(fd_list), ==, 1);
> +    fd = g_unix_fd_list_get(fd_list, handle, &error);
> +    g_assert_no_error(error);
> +
> +    addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, arg_offset);
> +    g_assert_no_errno(GPOINTER_TO_INT(addr));

Strictly speaking, this construct is not safe. When void * is 64-bit and 
int is 32-bit, this assetion will fail if the lower 32 bits of addr are 
in [0x80000000, 0xffffffff] though addr may still be a valid address. 
This is because GPOINTER_TO_INT() results in a negative value for such 
an address, and g_assert_no_errno() asserts that the given value is 
non-negative.

Using g_mapped_file_new_from_fd() and will simplify this function as whole.

> +    g_assert_nonnull(addr);
> +    g_assert_no_errno(munmap(addr, len));
> +
>       g_main_loop_quit(test->loop);
>   
> +    close(fd);
>       return DBUS_METHOD_INVOCATION_HANDLED;
>   }
>   
>   static void
> -test_dbus_console_setup_listener(TestDBusConsoleRegister *test)
> +test_dbus_console_setup_listener(TestDBusConsoleRegister *test, bool with_map)
>   {
>       g_autoptr(GDBusObjectSkeleton) listener = NULL;
>       g_autoptr(QemuDBusDisplay1ListenerSkeleton) iface = NULL;
> @@ -114,6 +153,20 @@ test_dbus_console_setup_listener(TestDBusConsoleRegister *test)
>                        NULL);
>       g_dbus_object_skeleton_add_interface(listener,
>                                            G_DBUS_INTERFACE_SKELETON(iface));
> +    if (with_map) {
> +        g_autoptr(QemuDBusDisplay1ListenerUnixMapSkeleton) iface_map =
> +            QEMU_DBUS_DISPLAY1_LISTENER_UNIX_MAP_SKELETON(
> +                qemu_dbus_display1_listener_unix_map_skeleton_new());
> +
> +        g_object_connect(iface_map,
> +                         "signal::handle-scanout-map", listener_handle_scanout_map, test,
> +                         NULL);
> +        g_dbus_object_skeleton_add_interface(listener,
> +                                             G_DBUS_INTERFACE_SKELETON(iface_map));
> +        g_object_set(iface, "interfaces",
> +            (const gchar *[]) { "org.qemu.Display1.Listener.Unix.Map", NULL },
> +            NULL);
> +    }
>       g_dbus_object_manager_server_export(test->server, listener);
>       g_dbus_object_manager_server_set_connection(test->server,
>                                                   test->listener_conn);
> @@ -145,7 +198,7 @@ test_dbus_console_registered(GObject *source_object,
>       g_assert_no_error(err);
>   
>       test->listener_conn = g_thread_join(test->thread);
> -    test_dbus_console_setup_listener(test);
> +    test_dbus_console_setup_listener(test, test->with_map);
>   }
>   
>   static gpointer
> @@ -155,7 +208,7 @@ test_dbus_p2p_server_setup_thread(gpointer data)
>   }
>   
>   static void
> -test_dbus_display_console(void)
> +test_dbus_display_console(const void* data)
>   {
>       g_autoptr(GError) err = NULL;
>       g_autoptr(GDBusConnection) conn = NULL;
> @@ -163,7 +216,7 @@ test_dbus_display_console(void)
>       g_autoptr(GMainLoop) loop = NULL;
>       QTestState *qts = NULL;
>       int pair[2];
> -    TestDBusConsoleRegister test = { 0, };
> +    TestDBusConsoleRegister test = { 0, .with_map = GPOINTER_TO_INT(data) };
>   #ifdef WIN32
>       WSAPROTOCOL_INFOW info;
>       g_autoptr(GVariant) listener = NULL;
> @@ -299,7 +352,8 @@ main(int argc, char **argv)
>       g_test_init(&argc, &argv, NULL);
>   
>       qtest_add_func("/dbus-display/vm", test_dbus_display_vm);
> -    qtest_add_func("/dbus-display/console", test_dbus_display_console);
> +    qtest_add_data_func("/dbus-display/console", GINT_TO_POINTER(false), test_dbus_display_console);
> +    qtest_add_data_func("/dbus-display/console/map", GINT_TO_POINTER(true), test_dbus_display_console);
>       qtest_add_func("/dbus-display/keyboard", test_dbus_display_keyboard);
>   
>       return g_test_run();

Re: [PATCH 16/16] tests: add basic -display dbus Map.Unix test

[Marc-André Lureau]

Hi

On Sat, Oct 5, 2024 at 12:32 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Only check we eventually get a shared memory scanout.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   tests/qtest/dbus-display-test.c | 64 ++++++++++++++++++++++++++++++---
> >   1 file changed, 59 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/qtest/dbus-display-test.c b/tests/qtest/dbus-display-test.c
> > index 0390bdcb41..ac92cb00d4 100644
> > --- a/tests/qtest/dbus-display-test.c
> > +++ b/tests/qtest/dbus-display-test.c
> > @@ -2,9 +2,12 @@
> >   #include "qemu/sockets.h"
> >   #include "qemu/dbus.h"
> >   #include "qemu/sockets.h"
> > +#include "glib.h"
> > +#include "glibconfig.h"
> >   #include <gio/gio.h>
> >   #include <gio/gunixfdlist.h>
> >   #include "libqtest.h"
> > +#include <sys/mman.h>
> >   #include "ui/dbus-display1.h"
> >
> >   static GDBusConnection*
> > @@ -82,6 +85,7 @@ typedef struct TestDBusConsoleRegister {
> >       GThread *thread;
> >       GDBusConnection *listener_conn;
> >       GDBusObjectManagerServer *server;
> > +    bool with_map;
> >   } TestDBusConsoleRegister;
> >
> >   static gboolean listener_handle_scanout(
> > @@ -94,13 +98,48 @@ static gboolean listener_handle_scanout(
> >       GVariant *arg_data,
> >       TestDBusConsoleRegister *test)
> >   {
> > +    if (!test->with_map) {
> > +        g_main_loop_quit(test->loop);
> > +    }
> > +
> > +    return DBUS_METHOD_INVOCATION_HANDLED;
> > +}
> > +
> > +static gboolean listener_handle_scanout_map(
> > +    QemuDBusDisplay1ListenerUnixMap *object,
> > +    GDBusMethodInvocation *invocation,
> > +    GUnixFDList *fd_list,
> > +    GVariant *arg_handle,
> > +    guint arg_offset,
> > +    guint arg_width,
> > +    guint arg_height,
> > +    guint arg_stride,
> > +    guint arg_pixman_format,
> > +    TestDBusConsoleRegister *test)
> > +{
> > +    int fd = -1;
> > +    gint32 handle = g_variant_get_handle(arg_handle);
> > +    g_autoptr(GError) error = NULL;
> > +    void *addr = NULL;
> > +    size_t len = arg_height * arg_stride;
> > +
> > +    g_assert_cmpuint(g_unix_fd_list_get_length(fd_list), ==, 1);
> > +    fd = g_unix_fd_list_get(fd_list, handle, &error);
> > +    g_assert_no_error(error);
> > +
> > +    addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, arg_offset);
> > +    g_assert_no_errno(GPOINTER_TO_INT(addr));
>
> Strictly speaking, this construct is not safe. When void * is 64-bit and
> int is 32-bit, this assetion will fail if the lower 32 bits of addr are
> in [0x80000000, 0xffffffff] though addr may still be a valid address.
> This is because GPOINTER_TO_INT() results in a negative value for such
> an address, and g_assert_no_errno() asserts that the given value is
> non-negative.
>
> Using g_mapped_file_new_from_fd() and will simplify this function as whole.

I thought about that, but g_mapped_file_new_from_fd() doesn't take a
len, and it fstat() the fd. This is ok for memfd apparently, and
appears to work, but it isn't really compliant with the dbus
interface.

>
> > +    g_assert_nonnull(addr);
> > +    g_assert_no_errno(munmap(addr, len));
> > +
> >       g_main_loop_quit(test->loop);
> >
> > +    close(fd);
> >       return DBUS_METHOD_INVOCATION_HANDLED;
> >   }
> >
> >   static void
> > -test_dbus_console_setup_listener(TestDBusConsoleRegister *test)
> > +test_dbus_console_setup_listener(TestDBusConsoleRegister *test, bool with_map)
> >   {
> >       g_autoptr(GDBusObjectSkeleton) listener = NULL;
> >       g_autoptr(QemuDBusDisplay1ListenerSkeleton) iface = NULL;
> > @@ -114,6 +153,20 @@ test_dbus_console_setup_listener(TestDBusConsoleRegister *test)
> >                        NULL);
> >       g_dbus_object_skeleton_add_interface(listener,
> >                                            G_DBUS_INTERFACE_SKELETON(iface));
> > +    if (with_map) {
> > +        g_autoptr(QemuDBusDisplay1ListenerUnixMapSkeleton) iface_map =
> > +            QEMU_DBUS_DISPLAY1_LISTENER_UNIX_MAP_SKELETON(
> > +                qemu_dbus_display1_listener_unix_map_skeleton_new());
> > +
> > +        g_object_connect(iface_map,
> > +                         "signal::handle-scanout-map", listener_handle_scanout_map, test,
> > +                         NULL);
> > +        g_dbus_object_skeleton_add_interface(listener,
> > +                                             G_DBUS_INTERFACE_SKELETON(iface_map));
> > +        g_object_set(iface, "interfaces",
> > +            (const gchar *[]) { "org.qemu.Display1.Listener.Unix.Map", NULL },
> > +            NULL);
> > +    }
> >       g_dbus_object_manager_server_export(test->server, listener);
> >       g_dbus_object_manager_server_set_connection(test->server,
> >                                                   test->listener_conn);
> > @@ -145,7 +198,7 @@ test_dbus_console_registered(GObject *source_object,
> >       g_assert_no_error(err);
> >
> >       test->listener_conn = g_thread_join(test->thread);
> > -    test_dbus_console_setup_listener(test);
> > +    test_dbus_console_setup_listener(test, test->with_map);
> >   }
> >
> >   static gpointer
> > @@ -155,7 +208,7 @@ test_dbus_p2p_server_setup_thread(gpointer data)
> >   }
> >
> >   static void
> > -test_dbus_display_console(void)
> > +test_dbus_display_console(const void* data)
> >   {
> >       g_autoptr(GError) err = NULL;
> >       g_autoptr(GDBusConnection) conn = NULL;
> > @@ -163,7 +216,7 @@ test_dbus_display_console(void)
> >       g_autoptr(GMainLoop) loop = NULL;
> >       QTestState *qts = NULL;
> >       int pair[2];
> > -    TestDBusConsoleRegister test = { 0, };
> > +    TestDBusConsoleRegister test = { 0, .with_map = GPOINTER_TO_INT(data) };
> >   #ifdef WIN32
> >       WSAPROTOCOL_INFOW info;
> >       g_autoptr(GVariant) listener = NULL;
> > @@ -299,7 +352,8 @@ main(int argc, char **argv)
> >       g_test_init(&argc, &argv, NULL);
> >
> >       qtest_add_func("/dbus-display/vm", test_dbus_display_vm);
> > -    qtest_add_func("/dbus-display/console", test_dbus_display_console);
> > +    qtest_add_data_func("/dbus-display/console", GINT_TO_POINTER(false), test_dbus_display_console);
> > +    qtest_add_data_func("/dbus-display/console/map", GINT_TO_POINTER(true), test_dbus_display_console);
> >       qtest_add_func("/dbus-display/keyboard", test_dbus_display_keyboard);
> >
> >       return g_test_run();
>

Re: [PATCH 16/16] tests: add basic -display dbus Map.Unix test

[Akihiko Odaki]

On 2024/10/07 21:42, Marc-André Lureau wrote:
> Hi
> 
> On Sat, Oct 5, 2024 at 12:32 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> Only check we eventually get a shared memory scanout.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>    tests/qtest/dbus-display-test.c | 64 ++++++++++++++++++++++++++++++---
>>>    1 file changed, 59 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tests/qtest/dbus-display-test.c b/tests/qtest/dbus-display-test.c
>>> index 0390bdcb41..ac92cb00d4 100644
>>> --- a/tests/qtest/dbus-display-test.c
>>> +++ b/tests/qtest/dbus-display-test.c
>>> @@ -2,9 +2,12 @@
>>>    #include "qemu/sockets.h"
>>>    #include "qemu/dbus.h"
>>>    #include "qemu/sockets.h"
>>> +#include "glib.h"
>>> +#include "glibconfig.h"
>>>    #include <gio/gio.h>
>>>    #include <gio/gunixfdlist.h>
>>>    #include "libqtest.h"
>>> +#include <sys/mman.h>
>>>    #include "ui/dbus-display1.h"
>>>
>>>    static GDBusConnection*
>>> @@ -82,6 +85,7 @@ typedef struct TestDBusConsoleRegister {
>>>        GThread *thread;
>>>        GDBusConnection *listener_conn;
>>>        GDBusObjectManagerServer *server;
>>> +    bool with_map;
>>>    } TestDBusConsoleRegister;
>>>
>>>    static gboolean listener_handle_scanout(
>>> @@ -94,13 +98,48 @@ static gboolean listener_handle_scanout(
>>>        GVariant *arg_data,
>>>        TestDBusConsoleRegister *test)
>>>    {
>>> +    if (!test->with_map) {
>>> +        g_main_loop_quit(test->loop);
>>> +    }
>>> +
>>> +    return DBUS_METHOD_INVOCATION_HANDLED;
>>> +}
>>> +
>>> +static gboolean listener_handle_scanout_map(
>>> +    QemuDBusDisplay1ListenerUnixMap *object,
>>> +    GDBusMethodInvocation *invocation,
>>> +    GUnixFDList *fd_list,
>>> +    GVariant *arg_handle,
>>> +    guint arg_offset,
>>> +    guint arg_width,
>>> +    guint arg_height,
>>> +    guint arg_stride,
>>> +    guint arg_pixman_format,
>>> +    TestDBusConsoleRegister *test)
>>> +{
>>> +    int fd = -1;
>>> +    gint32 handle = g_variant_get_handle(arg_handle);
>>> +    g_autoptr(GError) error = NULL;
>>> +    void *addr = NULL;
>>> +    size_t len = arg_height * arg_stride;
>>> +
>>> +    g_assert_cmpuint(g_unix_fd_list_get_length(fd_list), ==, 1);
>>> +    fd = g_unix_fd_list_get(fd_list, handle, &error);
>>> +    g_assert_no_error(error);
>>> +
>>> +    addr = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, arg_offset);
>>> +    g_assert_no_errno(GPOINTER_TO_INT(addr));
>>
>> Strictly speaking, this construct is not safe. When void * is 64-bit and
>> int is 32-bit, this assetion will fail if the lower 32 bits of addr are
>> in [0x80000000, 0xffffffff] though addr may still be a valid address.
>> This is because GPOINTER_TO_INT() results in a negative value for such
>> an address, and g_assert_no_errno() asserts that the given value is
>> non-negative.
>>
>> Using g_mapped_file_new_from_fd() and will simplify this function as whole.
> 
> I thought about that, but g_mapped_file_new_from_fd() doesn't take a
> len, and it fstat() the fd. This is ok for memfd apparently, and
> appears to work, but it isn't really compliant with the dbus
> interface.

That's unfortunate. It seems we need to do:
g_assert_no_errno(addr == MAP_FAILED ? -1 : 0)

g_assert_nonnull() following this should be unnecessary by the way. It 
shouldn't happen unless the underlying platform is broken, which is 
irrelevant from the correctness of our code.

> 
>>
>>> +    g_assert_nonnull(addr);
>>> +    g_assert_no_errno(munmap(addr, len));
>>> +
>>>        g_main_loop_quit(test->loop);
>>>
>>> +    close(fd);
>>>        return DBUS_METHOD_INVOCATION_HANDLED;
>>>    }
>>>
>>>    static void
>>> -test_dbus_console_setup_listener(TestDBusConsoleRegister *test)
>>> +test_dbus_console_setup_listener(TestDBusConsoleRegister *test, bool with_map)
>>>    {
>>>        g_autoptr(GDBusObjectSkeleton) listener = NULL;
>>>        g_autoptr(QemuDBusDisplay1ListenerSkeleton) iface = NULL;
>>> @@ -114,6 +153,20 @@ test_dbus_console_setup_listener(TestDBusConsoleRegister *test)
>>>                         NULL);
>>>        g_dbus_object_skeleton_add_interface(listener,
>>>                                             G_DBUS_INTERFACE_SKELETON(iface));
>>> +    if (with_map) {
>>> +        g_autoptr(QemuDBusDisplay1ListenerUnixMapSkeleton) iface_map =
>>> +            QEMU_DBUS_DISPLAY1_LISTENER_UNIX_MAP_SKELETON(
>>> +                qemu_dbus_display1_listener_unix_map_skeleton_new());
>>> +
>>> +        g_object_connect(iface_map,
>>> +                         "signal::handle-scanout-map", listener_handle_scanout_map, test,
>>> +                         NULL);
>>> +        g_dbus_object_skeleton_add_interface(listener,
>>> +                                             G_DBUS_INTERFACE_SKELETON(iface_map));
>>> +        g_object_set(iface, "interfaces",
>>> +            (const gchar *[]) { "org.qemu.Display1.Listener.Unix.Map", NULL },
>>> +            NULL);
>>> +    }
>>>        g_dbus_object_manager_server_export(test->server, listener);
>>>        g_dbus_object_manager_server_set_connection(test->server,
>>>                                                    test->listener_conn);
>>> @@ -145,7 +198,7 @@ test_dbus_console_registered(GObject *source_object,
>>>        g_assert_no_error(err);
>>>
>>>        test->listener_conn = g_thread_join(test->thread);
>>> -    test_dbus_console_setup_listener(test);
>>> +    test_dbus_console_setup_listener(test, test->with_map);
>>>    }
>>>
>>>    static gpointer
>>> @@ -155,7 +208,7 @@ test_dbus_p2p_server_setup_thread(gpointer data)
>>>    }
>>>
>>>    static void
>>> -test_dbus_display_console(void)
>>> +test_dbus_display_console(const void* data)
>>>    {
>>>        g_autoptr(GError) err = NULL;
>>>        g_autoptr(GDBusConnection) conn = NULL;
>>> @@ -163,7 +216,7 @@ test_dbus_display_console(void)
>>>        g_autoptr(GMainLoop) loop = NULL;
>>>        QTestState *qts = NULL;
>>>        int pair[2];
>>> -    TestDBusConsoleRegister test = { 0, };
>>> +    TestDBusConsoleRegister test = { 0, .with_map = GPOINTER_TO_INT(data) };
>>>    #ifdef WIN32
>>>        WSAPROTOCOL_INFOW info;
>>>        g_autoptr(GVariant) listener = NULL;
>>> @@ -299,7 +352,8 @@ main(int argc, char **argv)
>>>        g_test_init(&argc, &argv, NULL);
>>>
>>>        qtest_add_func("/dbus-display/vm", test_dbus_display_vm);
>>> -    qtest_add_func("/dbus-display/console", test_dbus_display_console);
>>> +    qtest_add_data_func("/dbus-display/console", GINT_TO_POINTER(false), test_dbus_display_console);
>>> +    qtest_add_data_func("/dbus-display/console/map", GINT_TO_POINTER(true), test_dbus_display_console);
>>>        qtest_add_func("/dbus-display/keyboard", test_dbus_display_keyboard);
>>>
>>>        return g_test_run();
>>
>