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

Roque Arcudia Hernandez posted 2 patches 1 year, 3 months ago
There is a newer version of this series
[PATCH 2/2] hw/display: Allow injection of virtio-gpu EDID name
Posted by Roque Arcudia Hernandez 1 year, 3 months ago
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 2/2] hw/display: Allow injection of virtio-gpu EDID name
Posted by Daniel P. Berrangé 1 year, 2 months ago
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
Posted by Andrew Keesler 1 year, 2 months ago
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 :|
>
>
Re: [PATCH 2/2] hw/display: Allow injection of virtio-gpu EDID name
Posted by Daniel P. Berrangé 1 year, 2 months ago
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
Posted by Andrew Keesler 1 year, 2 months ago
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 :|
>
>
Re: [PATCH 2/2] hw/display: Allow injection of virtio-gpu EDID name
Posted by Andrew Keesler 1 year, 2 months ago
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 :|
>>
>>
Re: [PATCH 2/2] hw/display: Allow injection of virtio-gpu EDID name
Posted by Daniel P. Berrangé 1 year, 2 months ago
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
Posted by Markus Armbruster 1 year, 2 months ago
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.