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

Commit Message

Roque Arcudia Hernandez Oct. 17, 2024, 9:53 p.m. UTC

From: Andrew Keesler <[e-mail]>

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 <[e-mail]>
Signed-off-by: Roque Arcudia Hernandez <[e-mail]>
---
 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(-)

Comments

Marc-André Lureau Oct. 21, 2024, 11:14 a.m. UTC | #1

Hi Roque

On Fri, Oct 18, 2024 at 1:53 AM Roque Arcudia Hernandez
<[e-mail]> wrote:
>
> From: Andrew Keesler <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Roque Arcudia Hernandez <[e-mail]>
> ---
>  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.

Andrew Keesler Oct. 21, 2024, 8:03 p.m. UTC | #2

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 <
[e-mail]> wrote:

> Hi Roque
>
> On Fri, Oct 18, 2024 at 1:53 AM Roque Arcudia Hernandez
> <[e-mail]> wrote:
> >
> > From: Andrew Keesler <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Roque Arcudia Hernandez <[e-mail]>
> > ---
> >  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.
>
>

Daniel P. Berrangé Oct. 22, 2024, 7:14 a.m. UTC | #3

On Thu, Oct 17, 2024 at 09:53:03PM +0000, Roque Arcudia Hernandez wrote:
> From: Andrew Keesler <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Roque Arcudia Hernandez <[e-mail]>
> ---
>  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

Marc-André Lureau Oct. 22, 2024, 7:49 a.m. UTC | #4

Hi

On Tue, Oct 22, 2024 at 12:23 AM Andrew Keesler <[e-mail]>
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 <
> [e-mail]> wrote:
>
>> Hi Roque
>>
>> On Fri, Oct 18, 2024 at 1:53 AM Roque Arcudia Hernandez
>> <[e-mail]> wrote:
>> >
>> > From: Andrew Keesler <[e-mail]>
>> >
>> > 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 <[e-mail]>
>> > Signed-off-by: Roque Arcudia Hernandez <[e-mail]>
>> > ---
>> >  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.
>>
>>

Daniel P. Berrangé Oct. 22, 2024, 7:53 a.m. UTC | #5

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
> <[e-mail]> wrote:
> >
> > From: Andrew Keesler <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Roque Arcudia Hernandez <[e-mail]>
> > ---
> >  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

Daniel P. Berrangé Oct. 22, 2024, 7:55 a.m. UTC | #6

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 <[e-mail]>
> 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

Marc-André Lureau Oct. 22, 2024, 8:04 a.m. UTC | #7

Hi

On Tue, Oct 22, 2024 at 11:54 AM Daniel P. Berrangé <[e-mail]>
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
> > <[e-mail]> wrote:
> > >
> > > From: Andrew Keesler <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > Signed-off-by: Roque Arcudia Hernandez <[e-mail]>
> > > ---
> > >  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"

Daniel P. Berrangé Oct. 22, 2024, 8:10 a.m. UTC | #8

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é <[e-mail]>
> 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
> > > <[e-mail]> wrote:
> > > >
> > > > From: Andrew Keesler <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > Signed-off-by: Roque Arcudia Hernandez <[e-mail]>
> > > > ---
> > > >  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

Marc-André Lureau Oct. 22, 2024, 8:36 a.m. UTC | #9

Hi

On Tue, Oct 22, 2024 at 12:10 PM Daniel P. Berrangé <[e-mail]>
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é <[e-mail]
> >
> > 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
> > > > <[e-mail]> wrote:
> > > > >
> > > > > From: Andrew Keesler <[e-mail]>
> > > > >
> > > > > 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 <[e-mail]>
> > > > > Signed-off-by: Roque Arcudia Hernandez <[e-mail]>
> > > > > ---
> > > > >  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/[e-mail]/

>
> > > 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...

Andrew Keesler Oct. 28, 2024, 7:25 p.m. UTC | #10

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 <
[e-mail]> wrote:

> Hi
>
> On Tue, Oct 22, 2024 at 12:10 PM Daniel P. Berrangé <[e-mail]>
> 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é <
>> [e-mail]>
>> > 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
>> > > > <[e-mail]> wrote:
>> > > > >
>> > > > > From: Andrew Keesler <[e-mail]>
>> > > > >
>> > > > > 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 <[e-mail]>
>> > > > > Signed-off-by: Roque Arcudia Hernandez <[e-mail]>
>> > > > > ---
>> > > > >  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/[e-mail]/
>
>>
>> > > 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
>

Patch

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;
     }