[PATCH 0/2] Allow injection of virtio-gpu EDID name

[PATCH 0/2] Allow injection of virtio-gpu EDID name

[Roque Arcudia Hernandez]

Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display
Identification Data) is propagated by QEMU such that a virtual
display presents legitimate metadata (e.g., name, serial number,
preferred resolutions, etc.) to its connected guest.

This change adds the ability to specify the EDID name for a
particular virtio-vga display. Previously, every virtual display
would have the same name: "QEMU Monitor". Now, we can inject names of
displays in order to test guest behavior that is specific to display
names. We provide the ability to inject the display name from the
display configuration as that most closely resembles how real
displays work (hardware displays contain static EDID information that
is provided to every connected host).

This new behavior must be enabled by setting the edid_name boolean
property on the display device (it is disabled by default).

It should be noted that EDID names longer than 12 bytes will be
truncated per spec (I think?).

Testing: verified that when I specified 2 outputs for a virtio-gpu
with edid_name set, the names matched those that I configured with my
vnc display.

  -display vnc=localhost:0,id=aaa,display=vga,head=0,name=AAA \
  -display vnc=localhost:1,id=bbb,display=vga,head=1,name=BBB \
  -device virtio-vga,max_outputs=2,id=vga,edid_name=true

Andrew Keesler (2):
  ui: Allow injection of vnc display name
  hw/display: Allow injection of virtio-gpu EDID name

 hw/display/virtio-gpu-base.c   |  4 ++++
 include/hw/virtio/virtio-gpu.h |  5 +++++
 include/ui/console.h           |  2 ++
 ui/console-priv.h              |  1 +
 ui/console.c                   | 16 ++++++++++++++++
 ui/vnc.c                       |  8 +++++++-
 6 files changed, 35 insertions(+), 1 deletion(-)

-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH 1/2] ui: Allow injection of vnc display name

[Roque Arcudia Hernandez]

From: Andrew Keesler <ankeesler@google.com>

Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display Identification
Data) is propagated by QEMU such that a virtual display presents legitimate
metadata (e.g., name, serial number, preferred resolutions, etc.) to its
connected guest.

This change propagates an optional user-provided display name to
QemuConsole. Future changes will update downstream devices to leverage this
display name for various uses, the primary one being providing a custom EDID
name to guests. Future changes will also update other displays (e.g., spice)
with a similar option to propagate a display name to downstream devices.

Currently, every virtio-gpu virtual display has the same name: "QEMU
Monitor". We hope to be able to inject the EDID name of virtual displays in
order to test guest behavior that is specific to display names. We provide the
ability to inject the display name from the display configuration as that most
closely resembles how real displays work (hardware displays contain static EDID
information that is provided to every connected host).

It should also be noted that EDID names longer than 12 bytes will be truncated
per spec (I think?).

Signed-off-by: Andrew Keesler <ankeesler@google.com>
Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
---
 include/ui/console.h | 1 +
 ui/console-priv.h    | 1 +
 ui/console.c         | 8 ++++++++
 ui/vnc.c             | 8 +++++++-
 4 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index 5832d52a8a..74ab03ed72 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -408,6 +408,7 @@ int qemu_console_get_index(QemuConsole *con);
 uint32_t qemu_console_get_head(QemuConsole *con);
 int qemu_console_get_width(QemuConsole *con, int fallback);
 int qemu_console_get_height(QemuConsole *con, int fallback);
+void qemu_console_set_name(QemuConsole *con, const char *name);
 /* Return the low-level window id for the console */
 int qemu_console_get_window_id(QemuConsole *con);
 /* Set the low-level window id for the console */
diff --git a/ui/console-priv.h b/ui/console-priv.h
index 43ceb8122f..9f2769843f 100644
--- a/ui/console-priv.h
+++ b/ui/console-priv.h
@@ -18,6 +18,7 @@ struct QemuConsole {
     Object parent;
 
     int index;
+    const char *name;
     DisplayState *ds;
     DisplaySurface *surface;
     DisplayScanout scanout;
diff --git a/ui/console.c b/ui/console.c
index 5165f17125..f377fd8417 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1452,6 +1452,14 @@ int qemu_console_get_height(QemuConsole *con, int fallback)
     }
 }
 
+void qemu_console_set_name(QemuConsole *con, const char *name)
+{
+    if (con == NULL) {
+        return;
+    }
+    con->name = name;
+}
+
 int qemu_invalidate_text_consoles(void)
 {
     QemuConsole *s;
diff --git a/ui/vnc.c b/ui/vnc.c
index 93a8dbd253..7d6acc5c2e 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3595,6 +3595,9 @@ static QemuOptsList qemu_vnc_opts = {
         },{
             .name = "power-control",
             .type = QEMU_OPT_BOOL,
+        },{
+            .name = "name",
+            .type = QEMU_OPT_STRING,
         },
         { /* end of list */ }
     },
@@ -4016,7 +4019,7 @@ void vnc_display_open(const char *id, Error **errp)
     QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
     g_autoptr(SocketAddressList) saddr_list = NULL;
     g_autoptr(SocketAddressList) wsaddr_list = NULL;
-    const char *share, *device_id;
+    const char *share, *device_id, *name;
     QemuConsole *con;
     bool password = false;
     bool reverse = false;
@@ -4217,6 +4220,9 @@ void vnc_display_open(const char *id, Error **errp)
     }
     qkbd_state_set_delay(vd->kbd, key_delay_ms);
 
+    name = qemu_opt_get(opts, "name");
+    qemu_console_set_name(vd->dcl.con, name);
+
     if (saddr_list == NULL) {
         return;
     }
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH 2/2] hw/display: Allow injection of virtio-gpu EDID name

[Roque Arcudia Hernandez]

From: Andrew Keesler <ankeesler@google.com>

Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display Identification
Data) is propagated by QEMU such that a virtual display presents legitimate
metadata (e.g., name, serial number, preferred resolutions, etc.) to its
connected guest.

This change adds the ability to specify the EDID name for a particular
virtio-vga display. Previously, every virtual display would have the same name:
"QEMU Monitor". Now, we can inject names of displays in order to test guest
behavior that is specific to display names. We provide the ability to inject the
display name from the display configuration as that most closely resembles how
real displays work (hardware displays contain static EDID information that is
provided to every connected host).

This new behavior must be enabled by setting the edid_name boolean property on
the display device (it is disabled by default).

It should be noted that EDID names longer than 12 bytes will be truncated per
spec (I think?).

Testing: verified that when I specified 2 outputs for a virtio-gpu with
edid_name set, the names matched those that I configured with my vnc display.

  -display vnc=localhost:0,id=aaa,display=vga,head=0,name=AAA \
  -display vnc=localhost:1,id=bbb,display=vga,head=1,name=BBB \
  -device virtio-vga,max_outputs=2,id=vga,edid_name=true

Signed-off-by: Andrew Keesler <ankeesler@google.com>
Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
---
 hw/display/virtio-gpu-base.c   | 4 ++++
 include/hw/virtio/virtio-gpu.h | 5 +++++
 include/ui/console.h           | 1 +
 ui/console.c                   | 8 ++++++++
 4 files changed, 18 insertions(+)

diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index 4fc7ef8896..778b798c45 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -64,6 +64,10 @@ virtio_gpu_base_generate_edid(VirtIOGPUBase *g, int scanout,
         .refresh_rate = g->req_state[scanout].refresh_rate,
     };
 
+    if (virtio_gpu_edid_name_enabled(g->conf)) {
+        info.name = qemu_console_get_name(g->scanout[scanout].con, NULL);
+    }
+
     edid->size = cpu_to_le32(sizeof(edid->edid));
     qemu_edid_generate(edid->edid, sizeof(edid->edid), &info);
 }
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index e343110e23..186355d0b9 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -97,6 +97,7 @@ enum virtio_gpu_base_conf_flags {
     VIRTIO_GPU_FLAG_BLOB_ENABLED,
     VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED,
     VIRTIO_GPU_FLAG_RUTABAGA_ENABLED,
+    VIRTIO_GPU_FLAG_EDID_NAME_ENABLED,
 };
 
 #define virtio_gpu_virgl_enabled(_cfg) \
@@ -115,6 +116,8 @@ enum virtio_gpu_base_conf_flags {
     (_cfg.flags & (1 << VIRTIO_GPU_FLAG_RUTABAGA_ENABLED))
 #define virtio_gpu_hostmem_enabled(_cfg) \
     (_cfg.hostmem > 0)
+#define virtio_gpu_edid_name_enabled(_cfg) \
+    (_cfg.flags & (1 << VIRTIO_GPU_FLAG_EDID_NAME_ENABLED))
 
 struct virtio_gpu_base_conf {
     uint32_t max_outputs;
@@ -163,6 +166,8 @@ struct VirtIOGPUBaseClass {
     DEFINE_PROP_UINT32("max_outputs", _state, _conf.max_outputs, 1),    \
     DEFINE_PROP_BIT("edid", _state, _conf.flags, \
                     VIRTIO_GPU_FLAG_EDID_ENABLED, true), \
+    DEFINE_PROP_BIT("edid_name", _state, _conf.flags, \
+                    VIRTIO_GPU_FLAG_EDID_NAME_ENABLED, false), \
     DEFINE_PROP_UINT32("xres", _state, _conf.xres, 1280), \
     DEFINE_PROP_UINT32("yres", _state, _conf.yres, 800)
 
diff --git a/include/ui/console.h b/include/ui/console.h
index 74ab03ed72..ce90802e0e 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -408,6 +408,7 @@ int qemu_console_get_index(QemuConsole *con);
 uint32_t qemu_console_get_head(QemuConsole *con);
 int qemu_console_get_width(QemuConsole *con, int fallback);
 int qemu_console_get_height(QemuConsole *con, int fallback);
+const char *qemu_console_get_name(QemuConsole *con, const char *fallback);
 void qemu_console_set_name(QemuConsole *con, const char *name);
 /* Return the low-level window id for the console */
 int qemu_console_get_window_id(QemuConsole *con);
diff --git a/ui/console.c b/ui/console.c
index f377fd8417..329688858e 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1452,6 +1452,14 @@ int qemu_console_get_height(QemuConsole *con, int fallback)
     }
 }
 
+const char *qemu_console_get_name(QemuConsole *con, const char *fallback)
+{
+    if (con == NULL) {
+        return fallback;
+    }
+    return con->name;
+}
+
 void qemu_console_set_name(QemuConsole *con, const char *name)
 {
     if (con == NULL) {
-- 
2.47.0.rc1.288.g06298d1525-goog

Re: [PATCH 1/2] ui: Allow injection of vnc display name

[Marc-André Lureau]

Hi Roque

On Fri, Oct 18, 2024 at 1:53 AM Roque Arcudia Hernandez
<roqueh@google.com> wrote:
>
> From: Andrew Keesler <ankeesler@google.com>
>
> Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display Identification
> Data) is propagated by QEMU such that a virtual display presents legitimate
> metadata (e.g., name, serial number, preferred resolutions, etc.) to its
> connected guest.
>
> This change propagates an optional user-provided display name to
> QemuConsole. Future changes will update downstream devices to leverage this
> display name for various uses, the primary one being providing a custom EDID
> name to guests. Future changes will also update other displays (e.g., spice)
> with a similar option to propagate a display name to downstream devices.
>
> Currently, every virtio-gpu virtual display has the same name: "QEMU
> Monitor". We hope to be able to inject the EDID name of virtual displays in
> order to test guest behavior that is specific to display names. We provide the
> ability to inject the display name from the display configuration as that most
> closely resembles how real displays work (hardware displays contain static EDID
> information that is provided to every connected host).
>
> It should also be noted that EDID names longer than 12 bytes will be truncated
> per spec (I think?).
>
> Signed-off-by: Andrew Keesler <ankeesler@google.com>
> Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
> ---
>  include/ui/console.h | 1 +
>  ui/console-priv.h    | 1 +
>  ui/console.c         | 8 ++++++++
>  ui/vnc.c             | 8 +++++++-
>  4 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 5832d52a8a..74ab03ed72 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -408,6 +408,7 @@ int qemu_console_get_index(QemuConsole *con);
>  uint32_t qemu_console_get_head(QemuConsole *con);
>  int qemu_console_get_width(QemuConsole *con, int fallback);
>  int qemu_console_get_height(QemuConsole *con, int fallback);
> +void qemu_console_set_name(QemuConsole *con, const char *name);
>  /* Return the low-level window id for the console */
>  int qemu_console_get_window_id(QemuConsole *con);
>  /* Set the low-level window id for the console */
> diff --git a/ui/console-priv.h b/ui/console-priv.h
> index 43ceb8122f..9f2769843f 100644
> --- a/ui/console-priv.h
> +++ b/ui/console-priv.h
> @@ -18,6 +18,7 @@ struct QemuConsole {
>      Object parent;
>
>      int index;
> +    const char *name;
>      DisplayState *ds;
>      DisplaySurface *surface;
>      DisplayScanout scanout;
> diff --git a/ui/console.c b/ui/console.c
> index 5165f17125..f377fd8417 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1452,6 +1452,14 @@ int qemu_console_get_height(QemuConsole *con, int fallback)
>      }
>  }
>
> +void qemu_console_set_name(QemuConsole *con, const char *name)
> +{
> +    if (con == NULL) {
> +        return;
> +    }
> +    con->name = name;
> +}
> +
>  int qemu_invalidate_text_consoles(void)
>  {
>      QemuConsole *s;
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 93a8dbd253..7d6acc5c2e 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3595,6 +3595,9 @@ static QemuOptsList qemu_vnc_opts = {
>          },{
>              .name = "power-control",
>              .type = QEMU_OPT_BOOL,
> +        },{
> +            .name = "name",
> +            .type = QEMU_OPT_STRING,
>          },
>          { /* end of list */ }
>      },
> @@ -4016,7 +4019,7 @@ void vnc_display_open(const char *id, Error **errp)
>      QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
>      g_autoptr(SocketAddressList) saddr_list = NULL;
>      g_autoptr(SocketAddressList) wsaddr_list = NULL;
> -    const char *share, *device_id;
> +    const char *share, *device_id, *name;
>      QemuConsole *con;
>      bool password = false;
>      bool reverse = false;
> @@ -4217,6 +4220,9 @@ void vnc_display_open(const char *id, Error **errp)
>      }
>      qkbd_state_set_delay(vd->kbd, key_delay_ms);
>
> +    name = qemu_opt_get(opts, "name");
> +    qemu_console_set_name(vd->dcl.con, name);

Why not expose a "head_name" property in QemuGraphicConsole?

This way it should be possible to set the name with QMP qom-set.

Re: [PATCH 1/2] ui: Allow injection of vnc display name

[Andrew Keesler]

[-- Attachment #1: Type: text/plain, Size: 5905 bytes --]

Hi Marc-André -

The ability to set the name with QMP qom-set seems like a nice behavior.

Note that the ultimate goal of this name is to propagate it downstream to
a device (see next patch[0] for a sample propagation to virtio-gpu).

In order to accomplish this, would it work to expose this new "head_name"
property via a qemu_graphic_console_get_head_name(QemuConsole *c) function
that:

1. verifies that c is indeed a QemuGraphicConsole with
   QEMU_IS_GRAPHIC_CONSOLE(), and
2. returns c->head_name (similar to qemu_console_get_name() from [0])?

We'd probably need a similar function
qemu_graphic_console_get_head_name(QemuConsole *c, const char *name) in
order to
set the head_name from a display (e.g., VNC) - correct me if you were
thinking
of going a different direction with this interface, though. My main goal is
to
provide some way for a user to inject a display EDID name from the command
line.

Also, just to verify my understanding - there would never be a QemuConsole
that
a) is NOT a QemuGraphicConsole AND b) is associated with an EDID in a guest,
correct?

[0] https://lists.gnu.org/archive/html/qemu-devel/2024-10/msg03238.html

On Mon, Oct 21, 2024 at 7:14 AM Marc-André Lureau <
marcandre.lureau@redhat.com> wrote:

> Hi Roque
>
> On Fri, Oct 18, 2024 at 1:53 AM Roque Arcudia Hernandez
> <roqueh@google.com> wrote:
> >
> > From: Andrew Keesler <ankeesler@google.com>
> >
> > Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display
> Identification
> > Data) is propagated by QEMU such that a virtual display presents
> legitimate
> > metadata (e.g., name, serial number, preferred resolutions, etc.) to its
> > connected guest.
> >
> > This change propagates an optional user-provided display name to
> > QemuConsole. Future changes will update downstream devices to leverage
> this
> > display name for various uses, the primary one being providing a custom
> EDID
> > name to guests. Future changes will also update other displays (e.g.,
> spice)
> > with a similar option to propagate a display name to downstream devices.
> >
> > Currently, every virtio-gpu virtual display has the same name: "QEMU
> > Monitor". We hope to be able to inject the EDID name of virtual displays
> in
> > order to test guest behavior that is specific to display names. We
> provide the
> > ability to inject the display name from the display configuration as
> that most
> > closely resembles how real displays work (hardware displays contain
> static EDID
> > information that is provided to every connected host).
> >
> > It should also be noted that EDID names longer than 12 bytes will be
> truncated
> > per spec (I think?).
> >
> > Signed-off-by: Andrew Keesler <ankeesler@google.com>
> > Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
> > ---
> >  include/ui/console.h | 1 +
> >  ui/console-priv.h    | 1 +
> >  ui/console.c         | 8 ++++++++
> >  ui/vnc.c             | 8 +++++++-
> >  4 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/ui/console.h b/include/ui/console.h
> > index 5832d52a8a..74ab03ed72 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -408,6 +408,7 @@ int qemu_console_get_index(QemuConsole *con);
> >  uint32_t qemu_console_get_head(QemuConsole *con);
> >  int qemu_console_get_width(QemuConsole *con, int fallback);
> >  int qemu_console_get_height(QemuConsole *con, int fallback);
> > +void qemu_console_set_name(QemuConsole *con, const char *name);
> >  /* Return the low-level window id for the console */
> >  int qemu_console_get_window_id(QemuConsole *con);
> >  /* Set the low-level window id for the console */
> > diff --git a/ui/console-priv.h b/ui/console-priv.h
> > index 43ceb8122f..9f2769843f 100644
> > --- a/ui/console-priv.h
> > +++ b/ui/console-priv.h
> > @@ -18,6 +18,7 @@ struct QemuConsole {
> >      Object parent;
> >
> >      int index;
> > +    const char *name;
> >      DisplayState *ds;
> >      DisplaySurface *surface;
> >      DisplayScanout scanout;
> > diff --git a/ui/console.c b/ui/console.c
> > index 5165f17125..f377fd8417 100644
> > --- a/ui/console.c
> > +++ b/ui/console.c
> > @@ -1452,6 +1452,14 @@ int qemu_console_get_height(QemuConsole *con, int
> fallback)
> >      }
> >  }
> >
> > +void qemu_console_set_name(QemuConsole *con, const char *name)
> > +{
> > +    if (con == NULL) {
> > +        return;
> > +    }
> > +    con->name = name;
> > +}
> > +
> >  int qemu_invalidate_text_consoles(void)
> >  {
> >      QemuConsole *s;
> > diff --git a/ui/vnc.c b/ui/vnc.c
> > index 93a8dbd253..7d6acc5c2e 100644
> > --- a/ui/vnc.c
> > +++ b/ui/vnc.c
> > @@ -3595,6 +3595,9 @@ static QemuOptsList qemu_vnc_opts = {
> >          },{
> >              .name = "power-control",
> >              .type = QEMU_OPT_BOOL,
> > +        },{
> > +            .name = "name",
> > +            .type = QEMU_OPT_STRING,
> >          },
> >          { /* end of list */ }
> >      },
> > @@ -4016,7 +4019,7 @@ void vnc_display_open(const char *id, Error **errp)
> >      QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
> >      g_autoptr(SocketAddressList) saddr_list = NULL;
> >      g_autoptr(SocketAddressList) wsaddr_list = NULL;
> > -    const char *share, *device_id;
> > +    const char *share, *device_id, *name;
> >      QemuConsole *con;
> >      bool password = false;
> >      bool reverse = false;
> > @@ -4217,6 +4220,9 @@ void vnc_display_open(const char *id, Error **errp)
> >      }
> >      qkbd_state_set_delay(vd->kbd, key_delay_ms);
> >
> > +    name = qemu_opt_get(opts, "name");
> > +    qemu_console_set_name(vd->dcl.con, name);
>
> Why not expose a "head_name" property in QemuGraphicConsole?
>
> This way it should be possible to set the name with QMP qom-set.
>
>

[-- Attachment #2: Type: text/html, Size: 7309 bytes --]

Re: [PATCH 1/2] ui: Allow injection of vnc display name

[Daniel P. Berrangé]

On Thu, Oct 17, 2024 at 09:53:03PM +0000, Roque Arcudia Hernandez wrote:
> From: Andrew Keesler <ankeesler@google.com>
> 
> Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display Identification
> Data) is propagated by QEMU such that a virtual display presents legitimate
> metadata (e.g., name, serial number, preferred resolutions, etc.) to its
> connected guest.
> 
> This change propagates an optional user-provided display name to
> QemuConsole. Future changes will update downstream devices to leverage this
> display name for various uses, the primary one being providing a custom EDID
> name to guests. Future changes will also update other displays (e.g., spice)
> with a similar option to propagate a display name to downstream devices.
> 
> Currently, every virtio-gpu virtual display has the same name: "QEMU
> Monitor". We hope to be able to inject the EDID name of virtual displays in
> order to test guest behavior that is specific to display names. We provide the
> ability to inject the display name from the display configuration as that most
> closely resembles how real displays work (hardware displays contain static EDID
> information that is provided to every connected host).
> 
> It should also be noted that EDID names longer than 12 bytes will be truncated
> per spec (I think?).
> 
> Signed-off-by: Andrew Keesler <ankeesler@google.com>
> Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
> ---
>  include/ui/console.h | 1 +
>  ui/console-priv.h    | 1 +
>  ui/console.c         | 8 ++++++++
>  ui/vnc.c             | 8 +++++++-
>  4 files changed, 17 insertions(+), 1 deletion(-)
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 93a8dbd253..7d6acc5c2e 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3595,6 +3595,9 @@ static QemuOptsList qemu_vnc_opts = {
>          },{
>              .name = "power-control",
>              .type = QEMU_OPT_BOOL,
> +        },{
> +            .name = "name",
> +            .type = QEMU_OPT_STRING,
>          },
>          { /* end of list */ }
>      },
> @@ -4016,7 +4019,7 @@ void vnc_display_open(const char *id, Error **errp)
>      QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
>      g_autoptr(SocketAddressList) saddr_list = NULL;
>      g_autoptr(SocketAddressList) wsaddr_list = NULL;
> -    const char *share, *device_id;
> +    const char *share, *device_id, *name;
>      QemuConsole *con;
>      bool password = false;
>      bool reverse = false;
> @@ -4217,6 +4220,9 @@ void vnc_display_open(const char *id, Error **errp)
>      }
>      qkbd_state_set_delay(vd->kbd, key_delay_ms);
>  
> +    name = qemu_opt_get(opts, "name");
> +    qemu_console_set_name(vd->dcl.con, name);
> +
>      if (saddr_list == NULL) {
>          return;
>      }

The VNC protocol has a display name field that is sent to the client,
and which they would typically display in the Window titlebar. As a
user, I would expect this 'name' setting to be controlling that. It
is currently set in the 'protocol_client_init' method.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/2] ui: Allow injection of vnc display name

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 6524 bytes --]

Hi

On Tue, Oct 22, 2024 at 12:23 AM Andrew Keesler <ankeesler@google.com>
wrote:

> Hi Marc-André -
>
> The ability to set the name with QMP qom-set seems like a nice behavior.
>
> Note that the ultimate goal of this name is to propagate it downstream to
> a device (see next patch[0] for a sample propagation to virtio-gpu).
>
> In order to accomplish this, would it work to expose this new "head_name"
> property via a qemu_graphic_console_get_head_name(QemuConsole *c) function
> that:
>
> 1. verifies that c is indeed a QemuGraphicConsole with
>    QEMU_IS_GRAPHIC_CONSOLE(), and
> 2. returns c->head_name (similar to qemu_console_get_name() from [0])?
>

> We'd probably need a similar function
> qemu_graphic_console_get_head_name(QemuConsole *c, const char *name) in
> order to
> set the head_name from a display (e.g., VNC) - correct me if you were
> thinking
>

Right (qemu_graphic_console_set_head_name), get/set exposed to QOM via
object_class_property_add_str()


> of going a different direction with this interface, though. My main goal
> is to
> provide some way for a user to inject a display EDID name from the command
> line.
>
> Also, just to verify my understanding - there would never be a QemuConsole
> that
> a) is NOT a QemuGraphicConsole AND b) is associated with an EDID in a
> guest,
> correct?
>
>
Seems correct.

(fwiw, I think we should have all UI info(s) as part of QemuUIInfo,
including the head name, but this would require further refactoring to
avoid some copy etc)


> [0] https://lists.gnu.org/archive/html/qemu-devel/2024-10/msg03238.html
>
> On Mon, Oct 21, 2024 at 7:14 AM Marc-André Lureau <
> marcandre.lureau@redhat.com> wrote:
>
>> Hi Roque
>>
>> On Fri, Oct 18, 2024 at 1:53 AM Roque Arcudia Hernandez
>> <roqueh@google.com> wrote:
>> >
>> > From: Andrew Keesler <ankeesler@google.com>
>> >
>> > Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display
>> Identification
>> > Data) is propagated by QEMU such that a virtual display presents
>> legitimate
>> > metadata (e.g., name, serial number, preferred resolutions, etc.) to its
>> > connected guest.
>> >
>> > This change propagates an optional user-provided display name to
>> > QemuConsole. Future changes will update downstream devices to leverage
>> this
>> > display name for various uses, the primary one being providing a custom
>> EDID
>> > name to guests. Future changes will also update other displays (e.g.,
>> spice)
>> > with a similar option to propagate a display name to downstream devices.
>> >
>> > Currently, every virtio-gpu virtual display has the same name: "QEMU
>> > Monitor". We hope to be able to inject the EDID name of virtual
>> displays in
>> > order to test guest behavior that is specific to display names. We
>> provide the
>> > ability to inject the display name from the display configuration as
>> that most
>> > closely resembles how real displays work (hardware displays contain
>> static EDID
>> > information that is provided to every connected host).
>> >
>> > It should also be noted that EDID names longer than 12 bytes will be
>> truncated
>> > per spec (I think?).
>> >
>> > Signed-off-by: Andrew Keesler <ankeesler@google.com>
>> > Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
>> > ---
>> >  include/ui/console.h | 1 +
>> >  ui/console-priv.h    | 1 +
>> >  ui/console.c         | 8 ++++++++
>> >  ui/vnc.c             | 8 +++++++-
>> >  4 files changed, 17 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/include/ui/console.h b/include/ui/console.h
>> > index 5832d52a8a..74ab03ed72 100644
>> > --- a/include/ui/console.h
>> > +++ b/include/ui/console.h
>> > @@ -408,6 +408,7 @@ int qemu_console_get_index(QemuConsole *con);
>> >  uint32_t qemu_console_get_head(QemuConsole *con);
>> >  int qemu_console_get_width(QemuConsole *con, int fallback);
>> >  int qemu_console_get_height(QemuConsole *con, int fallback);
>> > +void qemu_console_set_name(QemuConsole *con, const char *name);
>> >  /* Return the low-level window id for the console */
>> >  int qemu_console_get_window_id(QemuConsole *con);
>> >  /* Set the low-level window id for the console */
>> > diff --git a/ui/console-priv.h b/ui/console-priv.h
>> > index 43ceb8122f..9f2769843f 100644
>> > --- a/ui/console-priv.h
>> > +++ b/ui/console-priv.h
>> > @@ -18,6 +18,7 @@ struct QemuConsole {
>> >      Object parent;
>> >
>> >      int index;
>> > +    const char *name;
>> >      DisplayState *ds;
>> >      DisplaySurface *surface;
>> >      DisplayScanout scanout;
>> > diff --git a/ui/console.c b/ui/console.c
>> > index 5165f17125..f377fd8417 100644
>> > --- a/ui/console.c
>> > +++ b/ui/console.c
>> > @@ -1452,6 +1452,14 @@ int qemu_console_get_height(QemuConsole *con,
>> int fallback)
>> >      }
>> >  }
>> >
>> > +void qemu_console_set_name(QemuConsole *con, const char *name)
>> > +{
>> > +    if (con == NULL) {
>> > +        return;
>> > +    }
>> > +    con->name = name;
>> > +}
>> > +
>> >  int qemu_invalidate_text_consoles(void)
>> >  {
>> >      QemuConsole *s;
>> > diff --git a/ui/vnc.c b/ui/vnc.c
>> > index 93a8dbd253..7d6acc5c2e 100644
>> > --- a/ui/vnc.c
>> > +++ b/ui/vnc.c
>> > @@ -3595,6 +3595,9 @@ static QemuOptsList qemu_vnc_opts = {
>> >          },{
>> >              .name = "power-control",
>> >              .type = QEMU_OPT_BOOL,
>> > +        },{
>> > +            .name = "name",
>> > +            .type = QEMU_OPT_STRING,
>> >          },
>> >          { /* end of list */ }
>> >      },
>> > @@ -4016,7 +4019,7 @@ void vnc_display_open(const char *id, Error
>> **errp)
>> >      QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
>> >      g_autoptr(SocketAddressList) saddr_list = NULL;
>> >      g_autoptr(SocketAddressList) wsaddr_list = NULL;
>> > -    const char *share, *device_id;
>> > +    const char *share, *device_id, *name;
>> >      QemuConsole *con;
>> >      bool password = false;
>> >      bool reverse = false;
>> > @@ -4217,6 +4220,9 @@ void vnc_display_open(const char *id, Error
>> **errp)
>> >      }
>> >      qkbd_state_set_delay(vd->kbd, key_delay_ms);
>> >
>> > +    name = qemu_opt_get(opts, "name");
>> > +    qemu_console_set_name(vd->dcl.con, name);
>>
>> Why not expose a "head_name" property in QemuGraphicConsole?
>>
>> This way it should be possible to set the name with QMP qom-set.
>>
>>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 8757 bytes --]

Re: [PATCH 1/2] ui: Allow injection of vnc display name

[Daniel P. Berrangé]

On Mon, Oct 21, 2024 at 03:14:39PM +0400, Marc-André Lureau wrote:
> Hi Roque
> 
> On Fri, Oct 18, 2024 at 1:53 AM Roque Arcudia Hernandez
> <roqueh@google.com> wrote:
> >
> > From: Andrew Keesler <ankeesler@google.com>
> >
> > Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display Identification
> > Data) is propagated by QEMU such that a virtual display presents legitimate
> > metadata (e.g., name, serial number, preferred resolutions, etc.) to its
> > connected guest.
> >
> > This change propagates an optional user-provided display name to
> > QemuConsole. Future changes will update downstream devices to leverage this
> > display name for various uses, the primary one being providing a custom EDID
> > name to guests. Future changes will also update other displays (e.g., spice)
> > with a similar option to propagate a display name to downstream devices.
> >
> > Currently, every virtio-gpu virtual display has the same name: "QEMU
> > Monitor". We hope to be able to inject the EDID name of virtual displays in
> > order to test guest behavior that is specific to display names. We provide the
> > ability to inject the display name from the display configuration as that most
> > closely resembles how real displays work (hardware displays contain static EDID
> > information that is provided to every connected host).
> >
> > It should also be noted that EDID names longer than 12 bytes will be truncated
> > per spec (I think?).
> >
> > Signed-off-by: Andrew Keesler <ankeesler@google.com>
> > Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
> > ---
> >  include/ui/console.h | 1 +
> >  ui/console-priv.h    | 1 +
> >  ui/console.c         | 8 ++++++++
> >  ui/vnc.c             | 8 +++++++-
> >  4 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/ui/console.h b/include/ui/console.h
> > index 5832d52a8a..74ab03ed72 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -408,6 +408,7 @@ int qemu_console_get_index(QemuConsole *con);
> >  uint32_t qemu_console_get_head(QemuConsole *con);
> >  int qemu_console_get_width(QemuConsole *con, int fallback);
> >  int qemu_console_get_height(QemuConsole *con, int fallback);
> > +void qemu_console_set_name(QemuConsole *con, const char *name);
> >  /* Return the low-level window id for the console */
> >  int qemu_console_get_window_id(QemuConsole *con);
> >  /* Set the low-level window id for the console */
> > diff --git a/ui/console-priv.h b/ui/console-priv.h
> > index 43ceb8122f..9f2769843f 100644
> > --- a/ui/console-priv.h
> > +++ b/ui/console-priv.h
> > @@ -18,6 +18,7 @@ struct QemuConsole {
> >      Object parent;
> >
> >      int index;
> > +    const char *name;
> >      DisplayState *ds;
> >      DisplaySurface *surface;
> >      DisplayScanout scanout;
> > diff --git a/ui/console.c b/ui/console.c
> > index 5165f17125..f377fd8417 100644
> > --- a/ui/console.c
> > +++ b/ui/console.c
> > @@ -1452,6 +1452,14 @@ int qemu_console_get_height(QemuConsole *con, int fallback)
> >      }
> >  }
> >
> > +void qemu_console_set_name(QemuConsole *con, const char *name)
> > +{
> > +    if (con == NULL) {
> > +        return;
> > +    }
> > +    con->name = name;
> > +}
> > +
> >  int qemu_invalidate_text_consoles(void)
> >  {
> >      QemuConsole *s;
> > diff --git a/ui/vnc.c b/ui/vnc.c
> > index 93a8dbd253..7d6acc5c2e 100644
> > --- a/ui/vnc.c
> > +++ b/ui/vnc.c
> > @@ -3595,6 +3595,9 @@ static QemuOptsList qemu_vnc_opts = {
> >          },{
> >              .name = "power-control",
> >              .type = QEMU_OPT_BOOL,
> > +        },{
> > +            .name = "name",
> > +            .type = QEMU_OPT_STRING,
> >          },
> >          { /* end of list */ }
> >      },
> > @@ -4016,7 +4019,7 @@ void vnc_display_open(const char *id, Error **errp)
> >      QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
> >      g_autoptr(SocketAddressList) saddr_list = NULL;
> >      g_autoptr(SocketAddressList) wsaddr_list = NULL;
> > -    const char *share, *device_id;
> > +    const char *share, *device_id, *name;
> >      QemuConsole *con;
> >      bool password = false;
> >      bool reverse = false;
> > @@ -4217,6 +4220,9 @@ void vnc_display_open(const char *id, Error **errp)
> >      }
> >      qkbd_state_set_delay(vd->kbd, key_delay_ms);
> >
> > +    name = qemu_opt_get(opts, "name");
> > +    qemu_console_set_name(vd->dcl.con, name);
> 
> Why not expose a "head_name" property in QemuGraphicConsole?

QemuGraphicConsole isn't mapped to any CLI though, is it ?

In QAPI we have DisplayOptions union  for all the local displays,
and as a user I think I'd expect 'name' to be settable from
those.

For VNC/SPICE, we have not mapped to QAPI, but they do have their
own CLI options we can expose.

For runtime setting, we have a QMP  "display-update" command, that
currently just lets you change VNC listening address, but was intended
to allow for any runtime display changes.

> This way it should be possible to set the name with QMP qom-set.

qom-set isn't a particularly nice interface, as things you can set
from that are not introspectable and have no type information that
can be queried.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/2] ui: Allow injection of vnc display name

[Daniel P. Berrangé]

On Tue, Oct 22, 2024 at 11:49:46AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Oct 22, 2024 at 12:23 AM Andrew Keesler <ankeesler@google.com>
> wrote:
> 
> > Hi Marc-André -
> >
> > The ability to set the name with QMP qom-set seems like a nice behavior.
> >
> > Note that the ultimate goal of this name is to propagate it downstream to
> > a device (see next patch[0] for a sample propagation to virtio-gpu).
> >
> > In order to accomplish this, would it work to expose this new "head_name"
> > property via a qemu_graphic_console_get_head_name(QemuConsole *c) function
> > that:
> >
> > 1. verifies that c is indeed a QemuGraphicConsole with
> >    QEMU_IS_GRAPHIC_CONSOLE(), and
> > 2. returns c->head_name (similar to qemu_console_get_name() from [0])?
> >
> 
> > We'd probably need a similar function
> > qemu_graphic_console_get_head_name(QemuConsole *c, const char *name) in
> > order to
> > set the head_name from a display (e.g., VNC) - correct me if you were
> > thinking
> >
> 
> Right (qemu_graphic_console_set_head_name), get/set exposed to QOM via
> object_class_property_add_str()
> 
> 
> > of going a different direction with this interface, though. My main goal
> > is to
> > provide some way for a user to inject a display EDID name from the command
> > line.
> >
> > Also, just to verify my understanding - there would never be a QemuConsole
> > that
> > a) is NOT a QemuGraphicConsole AND b) is associated with an EDID in a
> > guest,
> > correct?
> >
> >
> Seems correct.
> 
> (fwiw, I think we should have all UI info(s) as part of QemuUIInfo,
> including the head name, but this would require further refactoring to
> avoid some copy etc)

QemuUIInfo is an internal struct, on the public side we have DisplayOptions
in QAPI which is where this should live. 


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/2] ui: Allow injection of vnc display name

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 6138 bytes --]

Hi

On Tue, Oct 22, 2024 at 11:54 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, Oct 21, 2024 at 03:14:39PM +0400, Marc-André Lureau wrote:
> > Hi Roque
> >
> > On Fri, Oct 18, 2024 at 1:53 AM Roque Arcudia Hernandez
> > <roqueh@google.com> wrote:
> > >
> > > From: Andrew Keesler <ankeesler@google.com>
> > >
> > > Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display
> Identification
> > > Data) is propagated by QEMU such that a virtual display presents
> legitimate
> > > metadata (e.g., name, serial number, preferred resolutions, etc.) to
> its
> > > connected guest.
> > >
> > > This change propagates an optional user-provided display name to
> > > QemuConsole. Future changes will update downstream devices to leverage
> this
> > > display name for various uses, the primary one being providing a
> custom EDID
> > > name to guests. Future changes will also update other displays (e.g.,
> spice)
> > > with a similar option to propagate a display name to downstream
> devices.
> > >
> > > Currently, every virtio-gpu virtual display has the same name: "QEMU
> > > Monitor". We hope to be able to inject the EDID name of virtual
> displays in
> > > order to test guest behavior that is specific to display names. We
> provide the
> > > ability to inject the display name from the display configuration as
> that most
> > > closely resembles how real displays work (hardware displays contain
> static EDID
> > > information that is provided to every connected host).
> > >
> > > It should also be noted that EDID names longer than 12 bytes will be
> truncated
> > > per spec (I think?).
> > >
> > > Signed-off-by: Andrew Keesler <ankeesler@google.com>
> > > Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
> > > ---
> > >  include/ui/console.h | 1 +
> > >  ui/console-priv.h    | 1 +
> > >  ui/console.c         | 8 ++++++++
> > >  ui/vnc.c             | 8 +++++++-
> > >  4 files changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/ui/console.h b/include/ui/console.h
> > > index 5832d52a8a..74ab03ed72 100644
> > > --- a/include/ui/console.h
> > > +++ b/include/ui/console.h
> > > @@ -408,6 +408,7 @@ int qemu_console_get_index(QemuConsole *con);
> > >  uint32_t qemu_console_get_head(QemuConsole *con);
> > >  int qemu_console_get_width(QemuConsole *con, int fallback);
> > >  int qemu_console_get_height(QemuConsole *con, int fallback);
> > > +void qemu_console_set_name(QemuConsole *con, const char *name);
> > >  /* Return the low-level window id for the console */
> > >  int qemu_console_get_window_id(QemuConsole *con);
> > >  /* Set the low-level window id for the console */
> > > diff --git a/ui/console-priv.h b/ui/console-priv.h
> > > index 43ceb8122f..9f2769843f 100644
> > > --- a/ui/console-priv.h
> > > +++ b/ui/console-priv.h
> > > @@ -18,6 +18,7 @@ struct QemuConsole {
> > >      Object parent;
> > >
> > >      int index;
> > > +    const char *name;
> > >      DisplayState *ds;
> > >      DisplaySurface *surface;
> > >      DisplayScanout scanout;
> > > diff --git a/ui/console.c b/ui/console.c
> > > index 5165f17125..f377fd8417 100644
> > > --- a/ui/console.c
> > > +++ b/ui/console.c
> > > @@ -1452,6 +1452,14 @@ int qemu_console_get_height(QemuConsole *con,
> int fallback)
> > >      }
> > >  }
> > >
> > > +void qemu_console_set_name(QemuConsole *con, const char *name)
> > > +{
> > > +    if (con == NULL) {
> > > +        return;
> > > +    }
> > > +    con->name = name;
> > > +}
> > > +
> > >  int qemu_invalidate_text_consoles(void)
> > >  {
> > >      QemuConsole *s;
> > > diff --git a/ui/vnc.c b/ui/vnc.c
> > > index 93a8dbd253..7d6acc5c2e 100644
> > > --- a/ui/vnc.c
> > > +++ b/ui/vnc.c
> > > @@ -3595,6 +3595,9 @@ static QemuOptsList qemu_vnc_opts = {
> > >          },{
> > >              .name = "power-control",
> > >              .type = QEMU_OPT_BOOL,
> > > +        },{
> > > +            .name = "name",
> > > +            .type = QEMU_OPT_STRING,
> > >          },
> > >          { /* end of list */ }
> > >      },
> > > @@ -4016,7 +4019,7 @@ void vnc_display_open(const char *id, Error
> **errp)
> > >      QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
> > >      g_autoptr(SocketAddressList) saddr_list = NULL;
> > >      g_autoptr(SocketAddressList) wsaddr_list = NULL;
> > > -    const char *share, *device_id;
> > > +    const char *share, *device_id, *name;
> > >      QemuConsole *con;
> > >      bool password = false;
> > >      bool reverse = false;
> > > @@ -4217,6 +4220,9 @@ void vnc_display_open(const char *id, Error
> **errp)
> > >      }
> > >      qkbd_state_set_delay(vd->kbd, key_delay_ms);
> > >
> > > +    name = qemu_opt_get(opts, "name");
> > > +    qemu_console_set_name(vd->dcl.con, name);
> >
> > Why not expose a "head_name" property in QemuGraphicConsole?
>
> QemuGraphicConsole isn't mapped to any CLI though, is it ?
>
>
No, it would be a bit tedious to do so with multi-head -devices.


> In QAPI we have DisplayOptions union  for all the local displays,
> and as a user I think I'd expect 'name' to be settable from
> those.
>
>
DisplayOptions is meant for the UI display.. Here, the intent is really to
set the HW EDID name field.

Also DisplayOptions doesn't map to a specific console.


For VNC/SPICE, we have not mapped to QAPI, but they do have their
> own CLI options we can expose.
>
> For runtime setting, we have a QMP  "display-update" command, that
> currently just lets you change VNC listening address, but was intended
> to allow for any runtime display changes.
>
> > This way it should be possible to set the name with QMP qom-set.
>
> qom-set isn't a particularly nice interface, as things you can set
> from that are not introspectable and have no type information that
> can be queried.
>

fwiw, it could be easily exposed to D-Bus, for ex:

busctl --user set-property org.qemu /org/qemu/Display1/Console_1
org.qemu.Display1.Console HeadName s "First Monitor"

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 8341 bytes --]

Re: [PATCH 1/2] ui: Allow injection of vnc display name

[Daniel P. Berrangé]

On Tue, Oct 22, 2024 at 12:04:29PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Oct 22, 2024 at 11:54 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Mon, Oct 21, 2024 at 03:14:39PM +0400, Marc-André Lureau wrote:
> > > Hi Roque
> > >
> > > On Fri, Oct 18, 2024 at 1:53 AM Roque Arcudia Hernandez
> > > <roqueh@google.com> wrote:
> > > >
> > > > From: Andrew Keesler <ankeesler@google.com>
> > > >
> > > > Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display
> > Identification
> > > > Data) is propagated by QEMU such that a virtual display presents
> > legitimate
> > > > metadata (e.g., name, serial number, preferred resolutions, etc.) to
> > its
> > > > connected guest.
> > > >
> > > > This change propagates an optional user-provided display name to
> > > > QemuConsole. Future changes will update downstream devices to leverage
> > this
> > > > display name for various uses, the primary one being providing a
> > custom EDID
> > > > name to guests. Future changes will also update other displays (e.g.,
> > spice)
> > > > with a similar option to propagate a display name to downstream
> > devices.
> > > >
> > > > Currently, every virtio-gpu virtual display has the same name: "QEMU
> > > > Monitor". We hope to be able to inject the EDID name of virtual
> > displays in
> > > > order to test guest behavior that is specific to display names. We
> > provide the
> > > > ability to inject the display name from the display configuration as
> > that most
> > > > closely resembles how real displays work (hardware displays contain
> > static EDID
> > > > information that is provided to every connected host).
> > > >
> > > > It should also be noted that EDID names longer than 12 bytes will be
> > truncated
> > > > per spec (I think?).
> > > >
> > > > Signed-off-by: Andrew Keesler <ankeesler@google.com>
> > > > Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
> > > > ---
> > > >  include/ui/console.h | 1 +
> > > >  ui/console-priv.h    | 1 +
> > > >  ui/console.c         | 8 ++++++++
> > > >  ui/vnc.c             | 8 +++++++-
> > > >  4 files changed, 17 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/ui/console.h b/include/ui/console.h
> > > > index 5832d52a8a..74ab03ed72 100644
> > > > --- a/include/ui/console.h
> > > > +++ b/include/ui/console.h
> > > > @@ -408,6 +408,7 @@ int qemu_console_get_index(QemuConsole *con);
> > > >  uint32_t qemu_console_get_head(QemuConsole *con);
> > > >  int qemu_console_get_width(QemuConsole *con, int fallback);
> > > >  int qemu_console_get_height(QemuConsole *con, int fallback);
> > > > +void qemu_console_set_name(QemuConsole *con, const char *name);
> > > >  /* Return the low-level window id for the console */
> > > >  int qemu_console_get_window_id(QemuConsole *con);
> > > >  /* Set the low-level window id for the console */
> > > > diff --git a/ui/console-priv.h b/ui/console-priv.h
> > > > index 43ceb8122f..9f2769843f 100644
> > > > --- a/ui/console-priv.h
> > > > +++ b/ui/console-priv.h
> > > > @@ -18,6 +18,7 @@ struct QemuConsole {
> > > >      Object parent;
> > > >
> > > >      int index;
> > > > +    const char *name;
> > > >      DisplayState *ds;
> > > >      DisplaySurface *surface;
> > > >      DisplayScanout scanout;
> > > > diff --git a/ui/console.c b/ui/console.c
> > > > index 5165f17125..f377fd8417 100644
> > > > --- a/ui/console.c
> > > > +++ b/ui/console.c
> > > > @@ -1452,6 +1452,14 @@ int qemu_console_get_height(QemuConsole *con,
> > int fallback)
> > > >      }
> > > >  }
> > > >
> > > > +void qemu_console_set_name(QemuConsole *con, const char *name)
> > > > +{
> > > > +    if (con == NULL) {
> > > > +        return;
> > > > +    }
> > > > +    con->name = name;
> > > > +}
> > > > +
> > > >  int qemu_invalidate_text_consoles(void)
> > > >  {
> > > >      QemuConsole *s;
> > > > diff --git a/ui/vnc.c b/ui/vnc.c
> > > > index 93a8dbd253..7d6acc5c2e 100644
> > > > --- a/ui/vnc.c
> > > > +++ b/ui/vnc.c
> > > > @@ -3595,6 +3595,9 @@ static QemuOptsList qemu_vnc_opts = {
> > > >          },{
> > > >              .name = "power-control",
> > > >              .type = QEMU_OPT_BOOL,
> > > > +        },{
> > > > +            .name = "name",
> > > > +            .type = QEMU_OPT_STRING,
> > > >          },
> > > >          { /* end of list */ }
> > > >      },
> > > > @@ -4016,7 +4019,7 @@ void vnc_display_open(const char *id, Error
> > **errp)
> > > >      QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
> > > >      g_autoptr(SocketAddressList) saddr_list = NULL;
> > > >      g_autoptr(SocketAddressList) wsaddr_list = NULL;
> > > > -    const char *share, *device_id;
> > > > +    const char *share, *device_id, *name;
> > > >      QemuConsole *con;
> > > >      bool password = false;
> > > >      bool reverse = false;
> > > > @@ -4217,6 +4220,9 @@ void vnc_display_open(const char *id, Error
> > **errp)
> > > >      }
> > > >      qkbd_state_set_delay(vd->kbd, key_delay_ms);
> > > >
> > > > +    name = qemu_opt_get(opts, "name");
> > > > +    qemu_console_set_name(vd->dcl.con, name);
> > >
> > > Why not expose a "head_name" property in QemuGraphicConsole?
> >
> > QemuGraphicConsole isn't mapped to any CLI though, is it ?
> >
> >
> No, it would be a bit tedious to do so with multi-head -devices.
> 
> 
> > In QAPI we have DisplayOptions union  for all the local displays,
> > and as a user I think I'd expect 'name' to be settable from
> > those.
> >
> >
> DisplayOptions is meant for the UI display.. Here, the intent is really to
> set the HW EDID name field.

But it is also applicable to the backend, all of which have a
name for the display set in the window titlebar. We should
be looking at both sides IMHO.

> Also DisplayOptions doesn't map to a specific console.

It could be made to contain per-head information if we desired
though, and would be more useful than just the name. There were
some patches a while ago trying to express per-console placement
of windows onto host monitor outputs, for example.

> > own CLI options we can expose.
> >
> > For runtime setting, we have a QMP  "display-update" command, that
> > currently just lets you change VNC listening address, but was intended
> > to allow for any runtime display changes.
> >
> > > This way it should be possible to set the name with QMP qom-set.
> >
> > qom-set isn't a particularly nice interface, as things you can set
> > from that are not introspectable and have no type information that
> > can be queried.
> >
> 
> fwiw, it could be easily exposed to D-Bus, for ex:
> 
> busctl --user set-property org.qemu /org/qemu/Display1/Console_1
> org.qemu.Display1.Console HeadName s "First Monitor"

That could be mapped to whatever interface we expose on the QEMU side,
it doesn't have to be qom-set.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/2] ui: Allow injection of vnc display name

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 7897 bytes --]

Hi

On Tue, Oct 22, 2024 at 12:10 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Tue, Oct 22, 2024 at 12:04:29PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Oct 22, 2024 at 11:54 AM Daniel P. Berrangé <berrange@redhat.com
> >
> > wrote:
> >
> > > On Mon, Oct 21, 2024 at 03:14:39PM +0400, Marc-André Lureau wrote:
> > > > Hi Roque
> > > >
> > > > On Fri, Oct 18, 2024 at 1:53 AM Roque Arcudia Hernandez
> > > > <roqueh@google.com> wrote:
> > > > >
> > > > > From: Andrew Keesler <ankeesler@google.com>
> > > > >
> > > > > Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display
> > > Identification
> > > > > Data) is propagated by QEMU such that a virtual display presents
> > > legitimate
> > > > > metadata (e.g., name, serial number, preferred resolutions, etc.)
> to
> > > its
> > > > > connected guest.
> > > > >
> > > > > This change propagates an optional user-provided display name to
> > > > > QemuConsole. Future changes will update downstream devices to
> leverage
> > > this
> > > > > display name for various uses, the primary one being providing a
> > > custom EDID
> > > > > name to guests. Future changes will also update other displays
> (e.g.,
> > > spice)
> > > > > with a similar option to propagate a display name to downstream
> > > devices.
> > > > >
> > > > > Currently, every virtio-gpu virtual display has the same name:
> "QEMU
> > > > > Monitor". We hope to be able to inject the EDID name of virtual
> > > displays in
> > > > > order to test guest behavior that is specific to display names. We
> > > provide the
> > > > > ability to inject the display name from the display configuration
> as
> > > that most
> > > > > closely resembles how real displays work (hardware displays contain
> > > static EDID
> > > > > information that is provided to every connected host).
> > > > >
> > > > > It should also be noted that EDID names longer than 12 bytes will
> be
> > > truncated
> > > > > per spec (I think?).
> > > > >
> > > > > Signed-off-by: Andrew Keesler <ankeesler@google.com>
> > > > > Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
> > > > > ---
> > > > >  include/ui/console.h | 1 +
> > > > >  ui/console-priv.h    | 1 +
> > > > >  ui/console.c         | 8 ++++++++
> > > > >  ui/vnc.c             | 8 +++++++-
> > > > >  4 files changed, 17 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/include/ui/console.h b/include/ui/console.h
> > > > > index 5832d52a8a..74ab03ed72 100644
> > > > > --- a/include/ui/console.h
> > > > > +++ b/include/ui/console.h
> > > > > @@ -408,6 +408,7 @@ int qemu_console_get_index(QemuConsole *con);
> > > > >  uint32_t qemu_console_get_head(QemuConsole *con);
> > > > >  int qemu_console_get_width(QemuConsole *con, int fallback);
> > > > >  int qemu_console_get_height(QemuConsole *con, int fallback);
> > > > > +void qemu_console_set_name(QemuConsole *con, const char *name);
> > > > >  /* Return the low-level window id for the console */
> > > > >  int qemu_console_get_window_id(QemuConsole *con);
> > > > >  /* Set the low-level window id for the console */
> > > > > diff --git a/ui/console-priv.h b/ui/console-priv.h
> > > > > index 43ceb8122f..9f2769843f 100644
> > > > > --- a/ui/console-priv.h
> > > > > +++ b/ui/console-priv.h
> > > > > @@ -18,6 +18,7 @@ struct QemuConsole {
> > > > >      Object parent;
> > > > >
> > > > >      int index;
> > > > > +    const char *name;
> > > > >      DisplayState *ds;
> > > > >      DisplaySurface *surface;
> > > > >      DisplayScanout scanout;
> > > > > diff --git a/ui/console.c b/ui/console.c
> > > > > index 5165f17125..f377fd8417 100644
> > > > > --- a/ui/console.c
> > > > > +++ b/ui/console.c
> > > > > @@ -1452,6 +1452,14 @@ int qemu_console_get_height(QemuConsole
> *con,
> > > int fallback)
> > > > >      }
> > > > >  }
> > > > >
> > > > > +void qemu_console_set_name(QemuConsole *con, const char *name)
> > > > > +{
> > > > > +    if (con == NULL) {
> > > > > +        return;
> > > > > +    }
> > > > > +    con->name = name;
> > > > > +}
> > > > > +
> > > > >  int qemu_invalidate_text_consoles(void)
> > > > >  {
> > > > >      QemuConsole *s;
> > > > > diff --git a/ui/vnc.c b/ui/vnc.c
> > > > > index 93a8dbd253..7d6acc5c2e 100644
> > > > > --- a/ui/vnc.c
> > > > > +++ b/ui/vnc.c
> > > > > @@ -3595,6 +3595,9 @@ static QemuOptsList qemu_vnc_opts = {
> > > > >          },{
> > > > >              .name = "power-control",
> > > > >              .type = QEMU_OPT_BOOL,
> > > > > +        },{
> > > > > +            .name = "name",
> > > > > +            .type = QEMU_OPT_STRING,
> > > > >          },
> > > > >          { /* end of list */ }
> > > > >      },
> > > > > @@ -4016,7 +4019,7 @@ void vnc_display_open(const char *id, Error
> > > **errp)
> > > > >      QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
> > > > >      g_autoptr(SocketAddressList) saddr_list = NULL;
> > > > >      g_autoptr(SocketAddressList) wsaddr_list = NULL;
> > > > > -    const char *share, *device_id;
> > > > > +    const char *share, *device_id, *name;
> > > > >      QemuConsole *con;
> > > > >      bool password = false;
> > > > >      bool reverse = false;
> > > > > @@ -4217,6 +4220,9 @@ void vnc_display_open(const char *id, Error
> > > **errp)
> > > > >      }
> > > > >      qkbd_state_set_delay(vd->kbd, key_delay_ms);
> > > > >
> > > > > +    name = qemu_opt_get(opts, "name");
> > > > > +    qemu_console_set_name(vd->dcl.con, name);
> > > >
> > > > Why not expose a "head_name" property in QemuGraphicConsole?
> > >
> > > QemuGraphicConsole isn't mapped to any CLI though, is it ?
> > >
> > >
> > No, it would be a bit tedious to do so with multi-head -devices.
> >
> >
> > > In QAPI we have DisplayOptions union  for all the local displays,
> > > and as a user I think I'd expect 'name' to be settable from
> > > those.
> > >
> > >
> > DisplayOptions is meant for the UI display.. Here, the intent is really
> to
> > set the HW EDID name field.
>
> But it is also applicable to the backend, all of which have a
> name for the display set in the window titlebar. We should
> be looking at both sides IMHO.
>

Ok, if we consider both should be treated similarly / reflect each other.


>
> > Also DisplayOptions doesn't map to a specific console.
>
> It could be made to contain per-head information if we desired
> though, and would be more useful than just the name. There were
> some patches a while ago trying to express per-console placement
> of windows onto host monitor outputs, for example.
>

[RFC PATCH v2 0/2] ui/gtk: Introduce new param - Connectors
https://patchew.org/QEMU/20240531185804.119557-1-dongwon.kim@intel.com/

>
> > > own CLI options we can expose.
> > >
> > > For runtime setting, we have a QMP  "display-update" command, that
> > > currently just lets you change VNC listening address, but was intended
> > > to allow for any runtime display changes.
> > >
> > > > This way it should be possible to set the name with QMP qom-set.
> > >
> > > qom-set isn't a particularly nice interface, as things you can set
> > > from that are not introspectable and have no type information that
> > > can be queried.
> > >
> >
> > fwiw, it could be easily exposed to D-Bus, for ex:
> >
> > busctl --user set-property org.qemu /org/qemu/Display1/Console_1
> > org.qemu.Display1.Console HeadName s "First Monitor"
>
> That could be mapped to whatever interface we expose on the QEMU side,
> it doesn't have to be qom-set.
>

It seems to me the main problem is that consoles are dynamically created by
devices, and it's hard for the ui/display to map options to a specific
console.

The other issue is handling arrays with CLI in general...

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 11294 bytes --]

Re: [PATCH 1/2] ui: Allow injection of vnc display name

[Andrew Keesler]

[-- Attachment #1: Type: text/plain, Size: 9225 bytes --]

Hi Daniel and Marc-André - please excuse my delay (I was traveling
last week).

I see 2 primary takeaways here:

1. Updating the name field from the ServerInit RFB message to be
   controlled by the 'name' VNC option

2. Updating where we store the display 'name' in memory

For 1 - are we amenable to me updating this display name field to be
controlled by 'name' in a future patch? I'm still learning what the
QEMU community prefers with respect to patch sizes, but in general I
have been trying to keep the patches small.

For 2 - have we landed on storing the per-display 'name' in
DisplayOptions? I can't quite tell if we've converged here. It also
seems like there is some more plumbing to install here before we
leverage DisplayOptions, so I am curious what the recommendation is
for this patch - is there a temporary solution we could pursue and do
refactors later to unlock the CLI functionality that has been brought
up?

Correct me if I am wrong about any of this.

On Tue, Oct 22, 2024 at 4:36 AM Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:

> Hi
>
> On Tue, Oct 22, 2024 at 12:10 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
>
>> On Tue, Oct 22, 2024 at 12:04:29PM +0400, Marc-André Lureau wrote:
>> > Hi
>> >
>> > On Tue, Oct 22, 2024 at 11:54 AM Daniel P. Berrangé <
>> berrange@redhat.com>
>> > wrote:
>> >
>> > > On Mon, Oct 21, 2024 at 03:14:39PM +0400, Marc-André Lureau wrote:
>> > > > Hi Roque
>> > > >
>> > > > On Fri, Oct 18, 2024 at 1:53 AM Roque Arcudia Hernandez
>> > > > <roqueh@google.com> wrote:
>> > > > >
>> > > > > From: Andrew Keesler <ankeesler@google.com>
>> > > > >
>> > > > > Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display
>> > > Identification
>> > > > > Data) is propagated by QEMU such that a virtual display presents
>> > > legitimate
>> > > > > metadata (e.g., name, serial number, preferred resolutions, etc.)
>> to
>> > > its
>> > > > > connected guest.
>> > > > >
>> > > > > This change propagates an optional user-provided display name to
>> > > > > QemuConsole. Future changes will update downstream devices to
>> leverage
>> > > this
>> > > > > display name for various uses, the primary one being providing a
>> > > custom EDID
>> > > > > name to guests. Future changes will also update other displays
>> (e.g.,
>> > > spice)
>> > > > > with a similar option to propagate a display name to downstream
>> > > devices.
>> > > > >
>> > > > > Currently, every virtio-gpu virtual display has the same name:
>> "QEMU
>> > > > > Monitor". We hope to be able to inject the EDID name of virtual
>> > > displays in
>> > > > > order to test guest behavior that is specific to display names. We
>> > > provide the
>> > > > > ability to inject the display name from the display configuration
>> as
>> > > that most
>> > > > > closely resembles how real displays work (hardware displays
>> contain
>> > > static EDID
>> > > > > information that is provided to every connected host).
>> > > > >
>> > > > > It should also be noted that EDID names longer than 12 bytes will
>> be
>> > > truncated
>> > > > > per spec (I think?).
>> > > > >
>> > > > > Signed-off-by: Andrew Keesler <ankeesler@google.com>
>> > > > > Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
>> > > > > ---
>> > > > >  include/ui/console.h | 1 +
>> > > > >  ui/console-priv.h    | 1 +
>> > > > >  ui/console.c         | 8 ++++++++
>> > > > >  ui/vnc.c             | 8 +++++++-
>> > > > >  4 files changed, 17 insertions(+), 1 deletion(-)
>> > > > >
>> > > > > diff --git a/include/ui/console.h b/include/ui/console.h
>> > > > > index 5832d52a8a..74ab03ed72 100644
>> > > > > --- a/include/ui/console.h
>> > > > > +++ b/include/ui/console.h
>> > > > > @@ -408,6 +408,7 @@ int qemu_console_get_index(QemuConsole *con);
>> > > > >  uint32_t qemu_console_get_head(QemuConsole *con);
>> > > > >  int qemu_console_get_width(QemuConsole *con, int fallback);
>> > > > >  int qemu_console_get_height(QemuConsole *con, int fallback);
>> > > > > +void qemu_console_set_name(QemuConsole *con, const char *name);
>> > > > >  /* Return the low-level window id for the console */
>> > > > >  int qemu_console_get_window_id(QemuConsole *con);
>> > > > >  /* Set the low-level window id for the console */
>> > > > > diff --git a/ui/console-priv.h b/ui/console-priv.h
>> > > > > index 43ceb8122f..9f2769843f 100644
>> > > > > --- a/ui/console-priv.h
>> > > > > +++ b/ui/console-priv.h
>> > > > > @@ -18,6 +18,7 @@ struct QemuConsole {
>> > > > >      Object parent;
>> > > > >
>> > > > >      int index;
>> > > > > +    const char *name;
>> > > > >      DisplayState *ds;
>> > > > >      DisplaySurface *surface;
>> > > > >      DisplayScanout scanout;
>> > > > > diff --git a/ui/console.c b/ui/console.c
>> > > > > index 5165f17125..f377fd8417 100644
>> > > > > --- a/ui/console.c
>> > > > > +++ b/ui/console.c
>> > > > > @@ -1452,6 +1452,14 @@ int qemu_console_get_height(QemuConsole
>> *con,
>> > > int fallback)
>> > > > >      }
>> > > > >  }
>> > > > >
>> > > > > +void qemu_console_set_name(QemuConsole *con, const char *name)
>> > > > > +{
>> > > > > +    if (con == NULL) {
>> > > > > +        return;
>> > > > > +    }
>> > > > > +    con->name = name;
>> > > > > +}
>> > > > > +
>> > > > >  int qemu_invalidate_text_consoles(void)
>> > > > >  {
>> > > > >      QemuConsole *s;
>> > > > > diff --git a/ui/vnc.c b/ui/vnc.c
>> > > > > index 93a8dbd253..7d6acc5c2e 100644
>> > > > > --- a/ui/vnc.c
>> > > > > +++ b/ui/vnc.c
>> > > > > @@ -3595,6 +3595,9 @@ static QemuOptsList qemu_vnc_opts = {
>> > > > >          },{
>> > > > >              .name = "power-control",
>> > > > >              .type = QEMU_OPT_BOOL,
>> > > > > +        },{
>> > > > > +            .name = "name",
>> > > > > +            .type = QEMU_OPT_STRING,
>> > > > >          },
>> > > > >          { /* end of list */ }
>> > > > >      },
>> > > > > @@ -4016,7 +4019,7 @@ void vnc_display_open(const char *id, Error
>> > > **errp)
>> > > > >      QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
>> > > > >      g_autoptr(SocketAddressList) saddr_list = NULL;
>> > > > >      g_autoptr(SocketAddressList) wsaddr_list = NULL;
>> > > > > -    const char *share, *device_id;
>> > > > > +    const char *share, *device_id, *name;
>> > > > >      QemuConsole *con;
>> > > > >      bool password = false;
>> > > > >      bool reverse = false;
>> > > > > @@ -4217,6 +4220,9 @@ void vnc_display_open(const char *id, Error
>> > > **errp)
>> > > > >      }
>> > > > >      qkbd_state_set_delay(vd->kbd, key_delay_ms);
>> > > > >
>> > > > > +    name = qemu_opt_get(opts, "name");
>> > > > > +    qemu_console_set_name(vd->dcl.con, name);
>> > > >
>> > > > Why not expose a "head_name" property in QemuGraphicConsole?
>> > >
>> > > QemuGraphicConsole isn't mapped to any CLI though, is it ?
>> > >
>> > >
>> > No, it would be a bit tedious to do so with multi-head -devices.
>> >
>> >
>> > > In QAPI we have DisplayOptions union  for all the local displays,
>> > > and as a user I think I'd expect 'name' to be settable from
>> > > those.
>> > >
>> > >
>> > DisplayOptions is meant for the UI display.. Here, the intent is really
>> to
>> > set the HW EDID name field.
>>
>> But it is also applicable to the backend, all of which have a
>> name for the display set in the window titlebar. We should
>> be looking at both sides IMHO.
>>
>
> Ok, if we consider both should be treated similarly / reflect each other.
>
>
>>
>> > Also DisplayOptions doesn't map to a specific console.
>>
>> It could be made to contain per-head information if we desired
>> though, and would be more useful than just the name. There were
>> some patches a while ago trying to express per-console placement
>> of windows onto host monitor outputs, for example.
>>
>
> [RFC PATCH v2 0/2] ui/gtk: Introduce new param - Connectors
> https://patchew.org/QEMU/20240531185804.119557-1-dongwon.kim@intel.com/
>
>>
>> > > own CLI options we can expose.
>> > >
>> > > For runtime setting, we have a QMP  "display-update" command, that
>> > > currently just lets you change VNC listening address, but was intended
>> > > to allow for any runtime display changes.
>> > >
>> > > > This way it should be possible to set the name with QMP qom-set.
>> > >
>> > > qom-set isn't a particularly nice interface, as things you can set
>> > > from that are not introspectable and have no type information that
>> > > can be queried.
>> > >
>> >
>> > fwiw, it could be easily exposed to D-Bus, for ex:
>> >
>> > busctl --user set-property org.qemu /org/qemu/Display1/Console_1
>> > org.qemu.Display1.Console HeadName s "First Monitor"
>>
>> That could be mapped to whatever interface we expose on the QEMU side,
>> it doesn't have to be qom-set.
>>
>
> It seems to me the main problem is that consoles are dynamically created
> by devices, and it's hard for the ui/display to map options to a specific
> console.
>
> The other issue is handling arrays with CLI in general...
>
> --
> Marc-André Lureau
>

[-- Attachment #2: Type: text/html, Size: 12783 bytes --]

Re: [PATCH 1/2] ui: Allow injection of vnc display name

[Andrew Keesler]

[-- Attachment #1: Type: text/plain, Size: 9723 bytes --]

Hi - gentle ping on my last email. Would love to hear any thoughts about
moving
this forward.

Thank you for your thoughts thus far.

On Mon, Oct 28, 2024 at 3:25 PM Andrew Keesler <ankeesler@google.com> wrote:

> Hi Daniel and Marc-André - please excuse my delay (I was traveling
> last week).
>
> I see 2 primary takeaways here:
>
> 1. Updating the name field from the ServerInit RFB message to be
>    controlled by the 'name' VNC option
>
> 2. Updating where we store the display 'name' in memory
>
> For 1 - are we amenable to me updating this display name field to be
> controlled by 'name' in a future patch? I'm still learning what the
> QEMU community prefers with respect to patch sizes, but in general I
> have been trying to keep the patches small.
>
> For 2 - have we landed on storing the per-display 'name' in
> DisplayOptions? I can't quite tell if we've converged here. It also
> seems like there is some more plumbing to install here before we
> leverage DisplayOptions, so I am curious what the recommendation is
> for this patch - is there a temporary solution we could pursue and do
> refactors later to unlock the CLI functionality that has been brought
> up?
>
> Correct me if I am wrong about any of this.
>
> On Tue, Oct 22, 2024 at 4:36 AM Marc-André Lureau <
> marcandre.lureau@gmail.com> wrote:
>
>> Hi
>>
>> On Tue, Oct 22, 2024 at 12:10 PM Daniel P. Berrangé <berrange@redhat.com>
>> wrote:
>>
>>> On Tue, Oct 22, 2024 at 12:04:29PM +0400, Marc-André Lureau wrote:
>>> > Hi
>>> >
>>> > On Tue, Oct 22, 2024 at 11:54 AM Daniel P. Berrangé <
>>> berrange@redhat.com>
>>> > wrote:
>>> >
>>> > > On Mon, Oct 21, 2024 at 03:14:39PM +0400, Marc-André Lureau wrote:
>>> > > > Hi Roque
>>> > > >
>>> > > > On Fri, Oct 18, 2024 at 1:53 AM Roque Arcudia Hernandez
>>> > > > <roqueh@google.com> wrote:
>>> > > > >
>>> > > > > From: Andrew Keesler <ankeesler@google.com>
>>> > > > >
>>> > > > > Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display
>>> > > Identification
>>> > > > > Data) is propagated by QEMU such that a virtual display presents
>>> > > legitimate
>>> > > > > metadata (e.g., name, serial number, preferred resolutions,
>>> etc.) to
>>> > > its
>>> > > > > connected guest.
>>> > > > >
>>> > > > > This change propagates an optional user-provided display name to
>>> > > > > QemuConsole. Future changes will update downstream devices to
>>> leverage
>>> > > this
>>> > > > > display name for various uses, the primary one being providing a
>>> > > custom EDID
>>> > > > > name to guests. Future changes will also update other displays
>>> (e.g.,
>>> > > spice)
>>> > > > > with a similar option to propagate a display name to downstream
>>> > > devices.
>>> > > > >
>>> > > > > Currently, every virtio-gpu virtual display has the same name:
>>> "QEMU
>>> > > > > Monitor". We hope to be able to inject the EDID name of virtual
>>> > > displays in
>>> > > > > order to test guest behavior that is specific to display names.
>>> We
>>> > > provide the
>>> > > > > ability to inject the display name from the display
>>> configuration as
>>> > > that most
>>> > > > > closely resembles how real displays work (hardware displays
>>> contain
>>> > > static EDID
>>> > > > > information that is provided to every connected host).
>>> > > > >
>>> > > > > It should also be noted that EDID names longer than 12 bytes
>>> will be
>>> > > truncated
>>> > > > > per spec (I think?).
>>> > > > >
>>> > > > > Signed-off-by: Andrew Keesler <ankeesler@google.com>
>>> > > > > Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
>>> > > > > ---
>>> > > > >  include/ui/console.h | 1 +
>>> > > > >  ui/console-priv.h    | 1 +
>>> > > > >  ui/console.c         | 8 ++++++++
>>> > > > >  ui/vnc.c             | 8 +++++++-
>>> > > > >  4 files changed, 17 insertions(+), 1 deletion(-)
>>> > > > >
>>> > > > > diff --git a/include/ui/console.h b/include/ui/console.h
>>> > > > > index 5832d52a8a..74ab03ed72 100644
>>> > > > > --- a/include/ui/console.h
>>> > > > > +++ b/include/ui/console.h
>>> > > > > @@ -408,6 +408,7 @@ int qemu_console_get_index(QemuConsole *con);
>>> > > > >  uint32_t qemu_console_get_head(QemuConsole *con);
>>> > > > >  int qemu_console_get_width(QemuConsole *con, int fallback);
>>> > > > >  int qemu_console_get_height(QemuConsole *con, int fallback);
>>> > > > > +void qemu_console_set_name(QemuConsole *con, const char *name);
>>> > > > >  /* Return the low-level window id for the console */
>>> > > > >  int qemu_console_get_window_id(QemuConsole *con);
>>> > > > >  /* Set the low-level window id for the console */
>>> > > > > diff --git a/ui/console-priv.h b/ui/console-priv.h
>>> > > > > index 43ceb8122f..9f2769843f 100644
>>> > > > > --- a/ui/console-priv.h
>>> > > > > +++ b/ui/console-priv.h
>>> > > > > @@ -18,6 +18,7 @@ struct QemuConsole {
>>> > > > >      Object parent;
>>> > > > >
>>> > > > >      int index;
>>> > > > > +    const char *name;
>>> > > > >      DisplayState *ds;
>>> > > > >      DisplaySurface *surface;
>>> > > > >      DisplayScanout scanout;
>>> > > > > diff --git a/ui/console.c b/ui/console.c
>>> > > > > index 5165f17125..f377fd8417 100644
>>> > > > > --- a/ui/console.c
>>> > > > > +++ b/ui/console.c
>>> > > > > @@ -1452,6 +1452,14 @@ int qemu_console_get_height(QemuConsole
>>> *con,
>>> > > int fallback)
>>> > > > >      }
>>> > > > >  }
>>> > > > >
>>> > > > > +void qemu_console_set_name(QemuConsole *con, const char *name)
>>> > > > > +{
>>> > > > > +    if (con == NULL) {
>>> > > > > +        return;
>>> > > > > +    }
>>> > > > > +    con->name = name;
>>> > > > > +}
>>> > > > > +
>>> > > > >  int qemu_invalidate_text_consoles(void)
>>> > > > >  {
>>> > > > >      QemuConsole *s;
>>> > > > > diff --git a/ui/vnc.c b/ui/vnc.c
>>> > > > > index 93a8dbd253..7d6acc5c2e 100644
>>> > > > > --- a/ui/vnc.c
>>> > > > > +++ b/ui/vnc.c
>>> > > > > @@ -3595,6 +3595,9 @@ static QemuOptsList qemu_vnc_opts = {
>>> > > > >          },{
>>> > > > >              .name = "power-control",
>>> > > > >              .type = QEMU_OPT_BOOL,
>>> > > > > +        },{
>>> > > > > +            .name = "name",
>>> > > > > +            .type = QEMU_OPT_STRING,
>>> > > > >          },
>>> > > > >          { /* end of list */ }
>>> > > > >      },
>>> > > > > @@ -4016,7 +4019,7 @@ void vnc_display_open(const char *id, Error
>>> > > **errp)
>>> > > > >      QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
>>> > > > >      g_autoptr(SocketAddressList) saddr_list = NULL;
>>> > > > >      g_autoptr(SocketAddressList) wsaddr_list = NULL;
>>> > > > > -    const char *share, *device_id;
>>> > > > > +    const char *share, *device_id, *name;
>>> > > > >      QemuConsole *con;
>>> > > > >      bool password = false;
>>> > > > >      bool reverse = false;
>>> > > > > @@ -4217,6 +4220,9 @@ void vnc_display_open(const char *id, Error
>>> > > **errp)
>>> > > > >      }
>>> > > > >      qkbd_state_set_delay(vd->kbd, key_delay_ms);
>>> > > > >
>>> > > > > +    name = qemu_opt_get(opts, "name");
>>> > > > > +    qemu_console_set_name(vd->dcl.con, name);
>>> > > >
>>> > > > Why not expose a "head_name" property in QemuGraphicConsole?
>>> > >
>>> > > QemuGraphicConsole isn't mapped to any CLI though, is it ?
>>> > >
>>> > >
>>> > No, it would be a bit tedious to do so with multi-head -devices.
>>> >
>>> >
>>> > > In QAPI we have DisplayOptions union  for all the local displays,
>>> > > and as a user I think I'd expect 'name' to be settable from
>>> > > those.
>>> > >
>>> > >
>>> > DisplayOptions is meant for the UI display.. Here, the intent is
>>> really to
>>> > set the HW EDID name field.
>>>
>>> But it is also applicable to the backend, all of which have a
>>> name for the display set in the window titlebar. We should
>>> be looking at both sides IMHO.
>>>
>>
>> Ok, if we consider both should be treated similarly / reflect each other.
>>
>>
>>>
>>> > Also DisplayOptions doesn't map to a specific console.
>>>
>>> It could be made to contain per-head information if we desired
>>> though, and would be more useful than just the name. There were
>>> some patches a while ago trying to express per-console placement
>>> of windows onto host monitor outputs, for example.
>>>
>>
>> [RFC PATCH v2 0/2] ui/gtk: Introduce new param - Connectors
>> https://patchew.org/QEMU/20240531185804.119557-1-dongwon.kim@intel.com/
>>
>>>
>>> > > own CLI options we can expose.
>>> > >
>>> > > For runtime setting, we have a QMP  "display-update" command, that
>>> > > currently just lets you change VNC listening address, but was
>>> intended
>>> > > to allow for any runtime display changes.
>>> > >
>>> > > > This way it should be possible to set the name with QMP qom-set.
>>> > >
>>> > > qom-set isn't a particularly nice interface, as things you can set
>>> > > from that are not introspectable and have no type information that
>>> > > can be queried.
>>> > >
>>> >
>>> > fwiw, it could be easily exposed to D-Bus, for ex:
>>> >
>>> > busctl --user set-property org.qemu /org/qemu/Display1/Console_1
>>> > org.qemu.Display1.Console HeadName s "First Monitor"
>>>
>>> That could be mapped to whatever interface we expose on the QEMU side,
>>> it doesn't have to be qom-set.
>>>
>>
>> It seems to me the main problem is that consoles are dynamically created
>> by devices, and it's hard for the ui/display to map options to a specific
>> console.
>>
>> The other issue is handling arrays with CLI in general...
>>
>> --
>> Marc-André Lureau
>>
>

[-- Attachment #2: Type: text/html, Size: 13368 bytes --]

Re: [PATCH 2/2] hw/display: Allow injection of virtio-gpu EDID name

[Daniel P. Berrangé]

On Thu, Oct 17, 2024 at 09:53:04PM +0000, Roque Arcudia Hernandez wrote:
> From: Andrew Keesler <ankeesler@google.com>
> 
> Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display Identification
> Data) is propagated by QEMU such that a virtual display presents legitimate
> metadata (e.g., name, serial number, preferred resolutions, etc.) to its
> connected guest.
> 
> This change adds the ability to specify the EDID name for a particular
> virtio-vga display. Previously, every virtual display would have the same name:
> "QEMU Monitor". Now, we can inject names of displays in order to test guest
> behavior that is specific to display names. We provide the ability to inject the
> display name from the display configuration as that most closely resembles how
> real displays work (hardware displays contain static EDID information that is
> provided to every connected host).
> 
> This new behavior must be enabled by setting the edid_name boolean property on
> the display device (it is disabled by default).
> 
> It should be noted that EDID names longer than 12 bytes will be truncated per
> spec (I think?).
> 
> Testing: verified that when I specified 2 outputs for a virtio-gpu with
> edid_name set, the names matched those that I configured with my vnc display.
> 
>   -display vnc=localhost:0,id=aaa,display=vga,head=0,name=AAA \
>   -display vnc=localhost:1,id=bbb,display=vga,head=1,name=BBB \
>   -device virtio-vga,max_outputs=2,id=vga,edid_name=true

Looking at this again, I'm thinking that it modelling this the wrong
way around.

On the QEMU side, we have a many<->many relationship between guest
display devices and host / remote display outputs.

If we assume every host / remote display output corresponds to a
separate "window" though, then we can reduce that down to  a
many:one relationship between host outputs and guest devices.

Consider this valid config:

  $ qemu-system-x86_64 \
     -vnc :1 \
     -spice port=5902,disable-ticketing \
     -display gtk \
     -device virtio-vga,max_outputs=2,id=vga

All three display outputs show the same guest display, so which
of VNC, SPICE & GTK would the virtio-vga EDID data take its names
from ?

IMHO, the name is a property of the virtio-vga "output" and the
various display backends should be honouring what that tells them
ie your configuration above should instead be:

   -display vnc=localhost:0,id=aaa,display=vga,head=0 \
   -display vnc=localhost:1,id=bbb,display=vga,head=1 \
   -device '{"driver":"virtio-vga",
             "max_outputs":2,
	     "id":"vga",
	     "outputs":[
	       {
	          "name":"AAA",
	       },
	       {
	          "name":"BBB",
	       },
	     ]}'

..whereupon we have to feed the EDID name from the device back to VNC,
so VNC can tell the client of the head name.

Note, I'm intentionally using JSON syntax for -device here, to illustrate
handling of non-scalar properties.

The set of active outputs can be turned on/off at runtime. We can declare
that the user should give names for every output at startup, upto whatever
they said for "max_outputs". That way a name is available even when non-
primary outputs are later turned on at runtime.

The secondary reason why I think names ought to be handled with -device
is that this is guest visible data, and as a general rule we aim for all
guest visible data to be controlled via properties on the frontend, and
not have the backend directly change what the guest sees.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] hw/display: Allow injection of virtio-gpu EDID name

[Andrew Keesler]

[-- Attachment #1: Type: text/plain, Size: 4918 bytes --]

I follow what you are saying. I misunderstood what a "display" was in the
domain of QEMU. Yes, this makes more sense now.

> the user should give names for every output at startup

I see DEFINE_PROP_ARRAY exists. I can use that to define the new "outputs"
property. Any reason that each "output" would ever need to be an object
(rather than just a string)? Nothing comes to mind, I'm just taking a second
to think about API forwards compatibility.

> upto whatever they said for "max_outputs"

Where is the best place to perform this validation? I would imagine we would
want to fast-fail if the user provided more "outputs" than "max_outputs". I
can
perform the validation in virtio_gpu_base_get_features but that seems late.

Thanks for your help.

On Mon, Nov 25, 2024 at 7:04 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Thu, Oct 17, 2024 at 09:53:04PM +0000, Roque Arcudia Hernandez wrote:
> > From: Andrew Keesler <ankeesler@google.com>
> >
> > Thanks to 72d277a7, 1ed2cb32, and others, EDID (Extended Display
> Identification
> > Data) is propagated by QEMU such that a virtual display presents
> legitimate
> > metadata (e.g., name, serial number, preferred resolutions, etc.) to its
> > connected guest.
> >
> > This change adds the ability to specify the EDID name for a particular
> > virtio-vga display. Previously, every virtual display would have the
> same name:
> > "QEMU Monitor". Now, we can inject names of displays in order to test
> guest
> > behavior that is specific to display names. We provide the ability to
> inject the
> > display name from the display configuration as that most closely
> resembles how
> > real displays work (hardware displays contain static EDID information
> that is
> > provided to every connected host).
> >
> > This new behavior must be enabled by setting the edid_name boolean
> property on
> > the display device (it is disabled by default).
> >
> > It should be noted that EDID names longer than 12 bytes will be
> truncated per
> > spec (I think?).
> >
> > Testing: verified that when I specified 2 outputs for a virtio-gpu with
> > edid_name set, the names matched those that I configured with my vnc
> display.
> >
> >   -display vnc=localhost:0,id=aaa,display=vga,head=0,name=AAA \
> >   -display vnc=localhost:1,id=bbb,display=vga,head=1,name=BBB \
> >   -device virtio-vga,max_outputs=2,id=vga,edid_name=true
>
> Looking at this again, I'm thinking that it modelling this the wrong
> way around.
>
> On the QEMU side, we have a many<->many relationship between guest
> display devices and host / remote display outputs.
>
> If we assume every host / remote display output corresponds to a
> separate "window" though, then we can reduce that down to  a
> many:one relationship between host outputs and guest devices.
>
> Consider this valid config:
>
>   $ qemu-system-x86_64 \
>      -vnc :1 \
>      -spice port=5902,disable-ticketing \
>      -display gtk \
>      -device virtio-vga,max_outputs=2,id=vga
>
> All three display outputs show the same guest display, so which
> of VNC, SPICE & GTK would the virtio-vga EDID data take its names
> from ?
>
> IMHO, the name is a property of the virtio-vga "output" and the
> various display backends should be honouring what that tells them
> ie your configuration above should instead be:
>
>    -display vnc=localhost:0,id=aaa,display=vga,head=0 \
>    -display vnc=localhost:1,id=bbb,display=vga,head=1 \
>    -device '{"driver":"virtio-vga",
>              "max_outputs":2,
>              "id":"vga",
>              "outputs":[
>                {
>                   "name":"AAA",
>                },
>                {
>                   "name":"BBB",
>                },
>              ]}'
>
> ..whereupon we have to feed the EDID name from the device back to VNC,
> so VNC can tell the client of the head name.
>
> Note, I'm intentionally using JSON syntax for -device here, to illustrate
> handling of non-scalar properties.
>
> The set of active outputs can be turned on/off at runtime. We can declare
> that the user should give names for every output at startup, upto whatever
> they said for "max_outputs". That way a name is available even when non-
> primary outputs are later turned on at runtime.
>
> The secondary reason why I think names ought to be handled with -device
> is that this is guest visible data, and as a general rule we aim for all
> guest visible data to be controlled via properties on the frontend, and
> not have the backend directly change what the guest sees.
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

[-- Attachment #2: Type: text/html, Size: 6499 bytes --]

Re: [PATCH 2/2] hw/display: Allow injection of virtio-gpu EDID name

[Daniel P. Berrangé]

On Mon, Nov 25, 2024 at 03:54:40PM -0500, Andrew Keesler wrote:
> I follow what you are saying. I misunderstood what a "display" was in the
> domain of QEMU. Yes, this makes more sense now.
> 
> > the user should give names for every output at startup
> 
> I see DEFINE_PROP_ARRAY exists. I can use that to define the new "outputs"
> property. Any reason that each "output" would ever need to be an object
> (rather than just a string)? Nothing comes to mind, I'm just taking a second
> to think about API forwards compatibility.

Currently we have 'xres' and 'yres' properties set against the device
for virtio-gpu.

If we're going to extend  it to allow the name of each "output" head
to be configured, it makes sense to allow for a data structure that
will let us also cnofigure xres & yres per output.

Hence, I thought it would make more sense to have an array of structs,
rather than the simpler array of strings, which will let us set any
amount of per-output config data we might want in future.

NB, I'm not asking you to wire up support for xres/yres per output,
just that we anticipate it as a possibility.

> > upto whatever they said for "max_outputs"
> 
> Where is the best place to perform this validation? I would imagine we would
> want to fast-fail if the user provided more "outputs" than "max_outputs". I
> can
> perform the validation in virtio_gpu_base_get_features but that seems late.

I'd suggest putting it in virtio_gpu_base_device_realize, as we already
have code there to validate 'max_outputs" is within limits.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] hw/display: Allow injection of virtio-gpu EDID name

[Andrew Keesler]

[-- Attachment #1: Type: text/plain, Size: 2208 bytes --]

Thanks, Daniel. We'll get this patch updated and send it out again.

> it makes sense to allow for a data structure

Whoops, I misread your original message - data structure SGTM.

On Tue, Nov 26, 2024 at 11:04 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, Nov 25, 2024 at 03:54:40PM -0500, Andrew Keesler wrote:
> > I follow what you are saying. I misunderstood what a "display" was in the
> > domain of QEMU. Yes, this makes more sense now.
> >
> > > the user should give names for every output at startup
> >
> > I see DEFINE_PROP_ARRAY exists. I can use that to define the new
> "outputs"
> > property. Any reason that each "output" would ever need to be an object
> > (rather than just a string)? Nothing comes to mind, I'm just taking a
> second
> > to think about API forwards compatibility.
>
> Currently we have 'xres' and 'yres' properties set against the device
> for virtio-gpu.
>
> If we're going to extend  it to allow the name of each "output" head
> to be configured, it makes sense to allow for a data structure that
> will let us also cnofigure xres & yres per output.
>
> Hence, I thought it would make more sense to have an array of structs,
> rather than the simpler array of strings, which will let us set any
> amount of per-output config data we might want in future.
>
> NB, I'm not asking you to wire up support for xres/yres per output,
> just that we anticipate it as a possibility.
>
> > > upto whatever they said for "max_outputs"
> >
> > Where is the best place to perform this validation? I would imagine we
> would
> > want to fast-fail if the user provided more "outputs" than
> "max_outputs". I
> > can
> > perform the validation in virtio_gpu_base_get_features but that seems
> late.
>
> I'd suggest putting it in virtio_gpu_base_device_realize, as we already
> have code there to validate 'max_outputs" is within limits.
>
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

[-- Attachment #2: Type: text/html, Size: 3276 bytes --]

Re: [PATCH 2/2] hw/display: Allow injection of virtio-gpu EDID name

[Andrew Keesler]

[-- Attachment #1: Type: text/plain, Size: 3075 bytes --]

Hi again Daniel. I have a follow up question. Can you help me
understand how I can declare this "outputs" property?

   -device '{"driver":"virtio-vga",
             "max_outputs":2,
             "id":"vga",
             "outputs":[
               {
                  "name":"AAA",
               },
               {
                  "name":"BBB",
               },
             ]}'

I thought DEFINE_PROP_ARRAY would do it, but I can't tell what PropertyInfo
implementation I should pass. All of the PropertyInfo implementations I can
find use scalar types, or simple text decoding. I am wondering if I am
missing
some sort of "JSON" encoding capabilities that can happen behind the scenes.

On Tue, Nov 26, 2024 at 4:07 PM Andrew Keesler <ankeesler@google.com> wrote:

> Thanks, Daniel. We'll get this patch updated and send it out again.
>
> > it makes sense to allow for a data structure
>
> Whoops, I misread your original message - data structure SGTM.
>
> On Tue, Nov 26, 2024 at 11:04 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
>
>> On Mon, Nov 25, 2024 at 03:54:40PM -0500, Andrew Keesler wrote:
>> > I follow what you are saying. I misunderstood what a "display" was in
>> the
>> > domain of QEMU. Yes, this makes more sense now.
>> >
>> > > the user should give names for every output at startup
>> >
>> > I see DEFINE_PROP_ARRAY exists. I can use that to define the new
>> "outputs"
>> > property. Any reason that each "output" would ever need to be an object
>> > (rather than just a string)? Nothing comes to mind, I'm just taking a
>> second
>> > to think about API forwards compatibility.
>>
>> Currently we have 'xres' and 'yres' properties set against the device
>> for virtio-gpu.
>>
>> If we're going to extend  it to allow the name of each "output" head
>> to be configured, it makes sense to allow for a data structure that
>> will let us also cnofigure xres & yres per output.
>>
>> Hence, I thought it would make more sense to have an array of structs,
>> rather than the simpler array of strings, which will let us set any
>> amount of per-output config data we might want in future.
>>
>> NB, I'm not asking you to wire up support for xres/yres per output,
>> just that we anticipate it as a possibility.
>>
>> > > upto whatever they said for "max_outputs"
>> >
>> > Where is the best place to perform this validation? I would imagine we
>> would
>> > want to fast-fail if the user provided more "outputs" than
>> "max_outputs". I
>> > can
>> > perform the validation in virtio_gpu_base_get_features but that seems
>> late.
>>
>> I'd suggest putting it in virtio_gpu_base_device_realize, as we already
>> have code there to validate 'max_outputs" is within limits.
>>
>>
>> With regards,
>> Daniel
>> --
>> |: https://berrange.com      -o-
>> https://www.flickr.com/photos/dberrange :|
>> |: https://libvirt.org         -o-
>> https://fstop138.berrange.com :|
>> |: https://entangle-photo.org    -o-
>> https://www.instagram.com/dberrange :|
>>
>>

[-- Attachment #2: Type: text/html, Size: 4708 bytes --]

Re: [PATCH 2/2] hw/display: Allow injection of virtio-gpu EDID name

[Daniel P. Berrangé]

CC Markus to keep me honest in my comments below

On Mon, Dec 02, 2024 at 03:31:53PM -0500, Andrew Keesler wrote:
> Hi again Daniel. I have a follow up question. Can you help me
> understand how I can declare this "outputs" property?
> 
>    -device '{"driver":"virtio-vga",
>              "max_outputs":2,
>              "id":"vga",
>              "outputs":[
>                {
>                   "name":"AAA",
>                },
>                {
>                   "name":"BBB",
>                },
>              ]}'
> 
> I thought DEFINE_PROP_ARRAY would do it, but I can't tell what PropertyInfo
> implementation I should pass. All of the PropertyInfo implementations I can
> find use scalar types, or simple text decoding. I am wondering if I am
> missing
> some sort of "JSON" encoding capabilities that can happen behind the scenes.

I could have sworn we had an example of how to handle this already,
but I'm not finding any Device class with a non-scalar property
that isn't merely an array of scalars.

We definitely have some examples elsewhere for exmaple "Machine" class
has an SmpCacheProperties array property, and the QAuthZList class
has an array of "QAuthZListRule" property.

In both cases the struct is defined in th qapi/<blah>.json, which
auto-generates code eg visit_type_QAuthZListRuleList, which can
then get called from qauthz_list_prop_get_rules and
qauthz_list_prop_set_rules, for the property.

Devices use a slightly higher level wrapper so instead of calling
object_class_property_add directly, then define the PropertyInfo
and object_class_property_add gets called indirectly for them.
I'm thinking it should still be possible to use the QAPI code
generator to help though. You could either just define the struct,
and thn use that to create  PropertyInfo to be used in combination
with DEFINE_PROP_ARRAY, of you could define a list of structs at
the QAPI level and use plain DEFINE_PROP. I guess the former is
probably better aligned with other Device code.

> 
> On Tue, Nov 26, 2024 at 4:07 PM Andrew Keesler <ankeesler@google.com> wrote:
> 
> > Thanks, Daniel. We'll get this patch updated and send it out again.
> >
> > > it makes sense to allow for a data structure
> >
> > Whoops, I misread your original message - data structure SGTM.
> >
> > On Tue, Nov 26, 2024 at 11:04 AM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> >> On Mon, Nov 25, 2024 at 03:54:40PM -0500, Andrew Keesler wrote:
> >> > I follow what you are saying. I misunderstood what a "display" was in
> >> the
> >> > domain of QEMU. Yes, this makes more sense now.
> >> >
> >> > > the user should give names for every output at startup
> >> >
> >> > I see DEFINE_PROP_ARRAY exists. I can use that to define the new
> >> "outputs"
> >> > property. Any reason that each "output" would ever need to be an object
> >> > (rather than just a string)? Nothing comes to mind, I'm just taking a
> >> second
> >> > to think about API forwards compatibility.
> >>
> >> Currently we have 'xres' and 'yres' properties set against the device
> >> for virtio-gpu.
> >>
> >> If we're going to extend  it to allow the name of each "output" head
> >> to be configured, it makes sense to allow for a data structure that
> >> will let us also cnofigure xres & yres per output.
> >>
> >> Hence, I thought it would make more sense to have an array of structs,
> >> rather than the simpler array of strings, which will let us set any
> >> amount of per-output config data we might want in future.
> >>
> >> NB, I'm not asking you to wire up support for xres/yres per output,
> >> just that we anticipate it as a possibility.
> >>
> >> > > upto whatever they said for "max_outputs"
> >> >
> >> > Where is the best place to perform this validation? I would imagine we
> >> would
> >> > want to fast-fail if the user provided more "outputs" than
> >> "max_outputs". I
> >> > can
> >> > perform the validation in virtio_gpu_base_get_features but that seems
> >> late.
> >>
> >> I'd suggest putting it in virtio_gpu_base_device_realize, as we already
> >> have code there to validate 'max_outputs" is within limits.
> >>
> >>
> >> With regards,
> >> Daniel
> >> --
> >> |: https://berrange.com      -o-
> >> https://www.flickr.com/photos/dberrange :|
> >> |: https://libvirt.org         -o-
> >> https://fstop138.berrange.com :|
> >> |: https://entangle-photo.org    -o-
> >> https://www.instagram.com/dberrange :|
> >>
> >>

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] hw/display: Allow injection of virtio-gpu EDID name

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> CC Markus to keep me honest in my comments below
>
> On Mon, Dec 02, 2024 at 03:31:53PM -0500, Andrew Keesler wrote:
>> Hi again Daniel. I have a follow up question. Can you help me
>> understand how I can declare this "outputs" property?
>> 
>>    -device '{"driver":"virtio-vga",
>>              "max_outputs":2,
>>              "id":"vga",
>>              "outputs":[
>>                {
>>                   "name":"AAA",
>>                },
>>                {
>>                   "name":"BBB",
>>                },
>>              ]}'
>> 
>> I thought DEFINE_PROP_ARRAY would do it, but I can't tell what PropertyInfo
>> implementation I should pass. All of the PropertyInfo implementations I can
>> find use scalar types, or simple text decoding. I am wondering if I am
>> missing
>> some sort of "JSON" encoding capabilities that can happen behind the scenes.
>
> I could have sworn we had an example of how to handle this already,
> but I'm not finding any Device class with a non-scalar property
> that isn't merely an array of scalars.

I found a few:

* TYPE_X86_CPU properties "feature-words" and "filtered-features" have
  struct type X86CPUFeatureWordInfo.  Defined in target/i386/cpu.c
  x86_cpu_initfn().

* TYPE_X86_CPU property "crash-information" has union type
  GuestPanicInformation.  Defined in target/i386/cpu.c
  x86_cpu_common_class_init().

* TYPE_VIRTIO_BLK property "iothread-vq-mapping" has type
  IOThreadVirtQueueMappingList, which is a list of struct
  IOThreadVirtQueueMapping.  Property defined in hw/block/virtio-blk.c
  virtio_blk_properties[] using macro
  DEFINE_PROP_IOTHREAD_VQ_MAPPING_LIST defined in
  qdev-properties-system.[ch].

In case you're curious how I fond them...  First, I collected device
help:

    $ for i in `upstream-qemu -nodefaults -S -display none -device help | sed -n 's/^name "\([^"]*\)".*/\1/p'`; do upstream-qemu -nodefaults -S -display none -device $i,help; done >dev-help

Then I extracted the type names:

    $ sed -n 's/.*=<\([^>]*\)>.*/\1/p' <dev-help | sort -u

Most of them "obviously" name scalars, QOM children or QOM links.
There's also crap like "list".  The promising ones are the ones
conforming to QAPI naming rules for types.

Look for them in the schema, drop the enums, and what's left are
non-scalar properties.

Since the type names are whatever the developer made up on the spot, my
search *can* miss non-scalar properties.

I didn't look at targets other than x86_64.

> We definitely have some examples elsewhere for exmaple "Machine" class
> has an SmpCacheProperties array property, and the QAuthZList class
> has an array of "QAuthZListRule" property.
>
> In both cases the struct is defined in th qapi/<blah>.json, which
> auto-generates code eg visit_type_QAuthZListRuleList, which can
> then get called from qauthz_list_prop_get_rules and
> qauthz_list_prop_set_rules, for the property.
>
> Devices use a slightly higher level wrapper so instead of calling
> object_class_property_add directly, then define the PropertyInfo
> and object_class_property_add gets called indirectly for them.
> I'm thinking it should still be possible to use the QAPI code
> generator to help though. You could either just define the struct,
> and thn use that to create  PropertyInfo to be used in combination
> with DEFINE_PROP_ARRAY, of you could define a list of structs at
> the QAPI level and use plain DEFINE_PROP. I guess the former is
> probably better aligned with other Device code.

Of the three instances I found, one uses such a qdev property machinery
(what you called "a slightly higher level wrapper"), and two do not.

Not sure what to recommend.