docs/formatdomain.rst | 8 ++++++++ src/conf/domain_conf.c | 5 +++++ src/conf/domain_conf.h | 1 + src/conf/domain_validate.c | 11 +++++++++++ src/conf/schemas/domaincommon.rng | 5 +++++ src/qemu/qemu_command.c | 7 +++++++ 6 files changed, 37 insertions(+)
Add the ability to enable/disable exposing the EDID information to the guest.
This allows migration from legacy machine types that have EDID disabled to a
newer machine type without any change becoming visible to the guest.
The edid attribute can specified in the domain XML as below:
<video>
<model type='virtio' edid='off'/>
</video>
If the edid attribute is unspecified, it is not generated so that the
virtualisation platform will continue to use its default.
The edid attribute is only valid for the vga, boch and virtio display models
and is currently only implemented for the QEMU driver.
Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
---
docs/formatdomain.rst | 8 ++++++++
src/conf/domain_conf.c | 5 +++++
src/conf/domain_conf.h | 1 +
src/conf/domain_validate.c | 11 +++++++++++
src/conf/schemas/domaincommon.rng | 5 +++++
src/qemu/qemu_command.c | 7 +++++++
6 files changed, 37 insertions(+)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 976746e292..7fe8b03a56 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -7103,6 +7103,14 @@ A video device.
sub-element is valid for model types "vga", "qxl", "bochs", "gop",
and "virtio".
+ :since:`Since 11.7.0` (QEMU driver only), the ``model`` element may have an
+ optional ``edid`` attribute that can be set to "on" or "off". If the ``edid``
+ attribute is not specified then the device will use its default value.
+ Otherwise setting ``edid`` to "on" will expose the device EDID blob to the
+ guest, whilst setting it to "off" will hide the device EDID blob from the
+ guest. The ``edid`` attribute is only valid for model types "vga", "bochs",
+ and "virtio".
+
``acceleration``
Configure if video acceleration should be enabled.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 59958c2f08..10cc6d7432 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13535,6 +13535,9 @@ virDomainVideoModelDefParseXML(virDomainVideoDef *def,
if (virXMLPropTristateSwitch(node, "blob", VIR_XML_PROP_NONE, &def->blob) < 0)
return -1;
+ if (virXMLPropTristateSwitch(node, "edid", VIR_XML_PROP_NONE, &def->edid) < 0)
+ return -1;
+
return 0;
}
@@ -26629,6 +26632,8 @@ virDomainVideoDefFormat(virBuffer *buf,
virBufferAddLit(buf, " primary='yes'");
if (def->blob != VIR_TRISTATE_SWITCH_ABSENT)
virBufferAsprintf(buf, " blob='%s'", virTristateSwitchTypeToString(def->blob));
+ if (def->edid != VIR_TRISTATE_SWITCH_ABSENT)
+ virBufferAsprintf(buf, " edid='%s'", virTristateSwitchTypeToString(def->edid));
if (def->accel || def->res) {
virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 2);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 596d138973..425ccfa97c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1895,6 +1895,7 @@ struct _virDomainVideoDef {
virDomainDeviceInfo info;
virDomainVirtioOptions *virtio;
virDomainVideoBackendType backend;
+ virTristateSwitch edid;
};
/* graphics console modes */
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 40edecef83..60a2e46b7e 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -231,6 +231,17 @@ virDomainVideoDefValidate(const virDomainVideoDef *video,
}
}
+ if ((video->type != VIR_DOMAIN_VIDEO_TYPE_BOCHS) &&
+ (video->type != VIR_DOMAIN_VIDEO_TYPE_VGA) &&
+ (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO)) {
+ if (video->edid != VIR_TRISTATE_SWITCH_ABSENT) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("video type '%1$s' does not support edid"),
+ virDomainVideoTypeToString(video->type));
+ return -1;
+ }
+ }
+
return 0;
}
diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
index a714c3fcc5..d84f20637c 100644
--- a/src/conf/schemas/domaincommon.rng
+++ b/src/conf/schemas/domaincommon.rng
@@ -4807,6 +4807,11 @@
<ref name="virOnOff"/>
</attribute>
</optional>
+ <optional>
+ <attribute name="edid">
+ <ref name="virOnOff"/>
+ </attribute>
+ </optional>
<optional>
<element name="acceleration">
<optional>
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 457dee7029..db00f9b173 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4739,6 +4739,13 @@ qemuBuildDeviceVideoCmd(virCommand *cmd,
return -1;
}
+ if ((video->type == VIR_DOMAIN_VIDEO_TYPE_BOCHS) ||
+ (video->type == VIR_DOMAIN_VIDEO_TYPE_VGA) ||
+ (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO)) {
+ if (virJSONValueObjectAdd(&props, "T:edid", video->edid, NULL) < 0)
+ return -1;
+ }
+
if (video->res) {
if (virJSONValueObjectAdd(&props,
"p:xres", video->res->x,
--
2.43.0On Thu, Jul 31, 2025 at 10:50:40AM +0100, Mark Cave-Ayland wrote: > Add the ability to enable/disable exposing the EDID information to the guest. > This allows migration from legacy machine types that have EDID disabled to a > newer machine type without any change becoming visible to the guest. Conceptually migrating between two different machine types should be impossible regardless of the EDID setting, because libvirt's ABI stability check should have rejected the changed machine type name upfront. > > The edid attribute can specified in the domain XML as below: > > <video> > <model type='virtio' edid='off'/> > </video> Supporting EDID as a setting of course makes sense regardless though so that's fine. Just don't justify it in the commit message based on the migration point. It is justiable simply because this is a QEMU feature that makes sense to be able to control > > If the edid attribute is unspecified, it is not generated so that the > virtualisation platform will continue to use its default. > > The edid attribute is only valid for the vga, boch and virtio display models > and is currently only implemented for the QEMU driver. > > Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com> > --- > docs/formatdomain.rst | 8 ++++++++ > src/conf/domain_conf.c | 5 +++++ > src/conf/domain_conf.h | 1 + > src/conf/domain_validate.c | 11 +++++++++++ > src/conf/schemas/domaincommon.rng | 5 +++++ > src/qemu/qemu_command.c | 7 +++++++ > 6 files changed, 37 insertions(+) Lacking test XML file coverage 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 :|
On 31/07/2025 13:04, Daniel P. Berrangé wrote: > On Thu, Jul 31, 2025 at 10:50:40AM +0100, Mark Cave-Ayland wrote: >> Add the ability to enable/disable exposing the EDID information to the guest. >> This allows migration from legacy machine types that have EDID disabled to a >> newer machine type without any change becoming visible to the guest. > > Conceptually migrating between two different machine types should be > impossible regardless of the EDID setting, because libvirt's ABI > stability check should have rejected the changed machine type name > upfront. That's true: the migration example is from an internal use case here. >> The edid attribute can specified in the domain XML as below: >> >> <video> >> <model type='virtio' edid='off'/> >> </video> > > Supporting EDID as a setting of course makes sense regardless though > so that's fine. Just don't justify it in the commit message based on > the migration point. It is justiable simply because this is a QEMU > feature that makes sense to be able to control Fair enough. I'll drop the 2nd sentence from the top paragraph in v2 to avoid further confusion. >> If the edid attribute is unspecified, it is not generated so that the >> virtualisation platform will continue to use its default. >> >> The edid attribute is only valid for the vga, boch and virtio display models >> and is currently only implemented for the QEMU driver. >> >> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com> >> --- >> docs/formatdomain.rst | 8 ++++++++ >> src/conf/domain_conf.c | 5 +++++ >> src/conf/domain_conf.h | 1 + >> src/conf/domain_validate.c | 11 +++++++++++ >> src/conf/schemas/domaincommon.rng | 5 +++++ >> src/qemu/qemu_command.c | 7 +++++++ >> 6 files changed, 37 insertions(+) > > Lacking test XML file coverage Peter also pointed this out, so I'll have a look at this before submitting v2. > With regards, > Daniel ATB, Mark.
On Thu, Jul 31, 2025 at 10:50:40 +0100, Mark Cave-Ayland wrote:
> Add the ability to enable/disable exposing the EDID information to the guest.
> This allows migration from legacy machine types that have EDID disabled to a
> newer machine type without any change becoming visible to the guest.
If this option has guest OS visible effects it ought to be covered by
the ABI stability checks which it isn't.
That though prevents it to be changed during migrations though.
In addition qemu itself has mechanisms that allow different behaviour
based on machine type for the exact reason to preserve guest ABI, but
allow changes in the future.
Is this feature covered by this?
If not libvirt might need to probe the current state to behave
correctly.
>
> The edid attribute can specified in the domain XML as below:
>
> <video>
> <model type='virtio' edid='off'/>
> </video>
>
> If the edid attribute is unspecified, it is not generated so that the
> virtualisation platform will continue to use its default.
>
> The edid attribute is only valid for the vga, boch and virtio display models
> and is currently only implemented for the QEMU driver.
>
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> ---
> docs/formatdomain.rst | 8 ++++++++
> src/conf/domain_conf.c | 5 +++++
> src/conf/domain_conf.h | 1 +
> src/conf/domain_validate.c | 11 +++++++++++
> src/conf/schemas/domaincommon.rng | 5 +++++
> src/qemu/qemu_command.c | 7 +++++++
missing test case addition to qemuxmlconftest which proves both the XML
parser/formatter and the commandline generator.
> 6 files changed, 37 insertions(+)
>
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 976746e292..7fe8b03a56 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -7103,6 +7103,14 @@ A video device.
> sub-element is valid for model types "vga", "qxl", "bochs", "gop",
> and "virtio".
>
> + :since:`Since 11.7.0` (QEMU driver only), the ``model`` element may have an
> + optional ``edid`` attribute that can be set to "on" or "off". If the ``edid``
> + attribute is not specified then the device will use its default value.
> + Otherwise setting ``edid`` to "on" will expose the device EDID blob to the
> + guest, whilst setting it to "off" will hide the device EDID blob from the
> + guest. The ``edid`` attribute is only valid for model types "vga", "bochs",
> + and "virtio".
> +
> ``acceleration``
> Configure if video acceleration should be enabled.
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 59958c2f08..10cc6d7432 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -13535,6 +13535,9 @@ virDomainVideoModelDefParseXML(virDomainVideoDef *def,
> if (virXMLPropTristateSwitch(node, "blob", VIR_XML_PROP_NONE, &def->blob) < 0)
> return -1;
>
> + if (virXMLPropTristateSwitch(node, "edid", VIR_XML_PROP_NONE, &def->edid) < 0)
> + return -1;
> +
> return 0;
> }
>
> @@ -26629,6 +26632,8 @@ virDomainVideoDefFormat(virBuffer *buf,
> virBufferAddLit(buf, " primary='yes'");
> if (def->blob != VIR_TRISTATE_SWITCH_ABSENT)
> virBufferAsprintf(buf, " blob='%s'", virTristateSwitchTypeToString(def->blob));
> + if (def->edid != VIR_TRISTATE_SWITCH_ABSENT)
> + virBufferAsprintf(buf, " edid='%s'", virTristateSwitchTypeToString(def->edid));
> if (def->accel || def->res) {
> virBufferAddLit(buf, ">\n");
> virBufferAdjustIndent(buf, 2);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 596d138973..425ccfa97c 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1895,6 +1895,7 @@ struct _virDomainVideoDef {
> virDomainDeviceInfo info;
> virDomainVirtioOptions *virtio;
> virDomainVideoBackendType backend;
> + virTristateSwitch edid;
> };
>
> /* graphics console modes */
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 40edecef83..60a2e46b7e 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -231,6 +231,17 @@ virDomainVideoDefValidate(const virDomainVideoDef *video,
> }
> }
>
> + if ((video->type != VIR_DOMAIN_VIDEO_TYPE_BOCHS) &&
> + (video->type != VIR_DOMAIN_VIDEO_TYPE_VGA) &&
> + (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO)) {
> + if (video->edid != VIR_TRISTATE_SWITCH_ABSENT) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("video type '%1$s' does not support edid"),
> + virDomainVideoTypeToString(video->type));
> + return -1;
> + }
> + }
> +
> return 0;
> }
>
> diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
> index a714c3fcc5..d84f20637c 100644
> --- a/src/conf/schemas/domaincommon.rng
> +++ b/src/conf/schemas/domaincommon.rng
> @@ -4807,6 +4807,11 @@
> <ref name="virOnOff"/>
> </attribute>
> </optional>
> + <optional>
> + <attribute name="edid">
> + <ref name="virOnOff"/>
> + </attribute>
> + </optional>
> <optional>
> <element name="acceleration">
> <optional>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 457dee7029..db00f9b173 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4739,6 +4739,13 @@ qemuBuildDeviceVideoCmd(virCommand *cmd,
> return -1;
> }
>
> + if ((video->type == VIR_DOMAIN_VIDEO_TYPE_BOCHS) ||
> + (video->type == VIR_DOMAIN_VIDEO_TYPE_VGA) ||
> + (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO)) {
This check should not be needed becxause the validation code ensures
that we don't allow it elsewhere. This should make the formatter
simpler.
> + if (virJSONValueObjectAdd(&props, "T:edid", video->edid, NULL) < 0)
> + return -1;
> + }
> +
> if (video->res) {
> if (virJSONValueObjectAdd(&props,
> "p:xres", video->res->x,
> --
> 2.43.0
>
On 31/07/2025 12:52, Peter Krempa wrote:
> On Thu, Jul 31, 2025 at 10:50:40 +0100, Mark Cave-Ayland wrote:
>> Add the ability to enable/disable exposing the EDID information to the guest.
>> This allows migration from legacy machine types that have EDID disabled to a
>> newer machine type without any change becoming visible to the guest.
>
> If this option has guest OS visible effects it ought to be covered by
> the ABI stability checks which it isn't.
>
> That though prevents it to be changed during migrations though.
>
> In addition qemu itself has mechanisms that allow different behaviour
> based on machine type for the exact reason to preserve guest ABI, but
> allow changes in the future.
>
> Is this feature covered by this?
>
> If not libvirt might need to probe the current state to behave
> correctly.
I took a look at QEMU and it looks like the edid property was added back
in 2018 for QEMU 3.1.0, so I think we should be good?
>>
>> The edid attribute can specified in the domain XML as below:
>>
>> <video>
>> <model type='virtio' edid='off'/>
>> </video>
>>
>> If the edid attribute is unspecified, it is not generated so that the
>> virtualisation platform will continue to use its default.
>>
>> The edid attribute is only valid for the vga, boch and virtio display models
>> and is currently only implemented for the QEMU driver.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>> ---
>> docs/formatdomain.rst | 8 ++++++++
>> src/conf/domain_conf.c | 5 +++++
>> src/conf/domain_conf.h | 1 +
>> src/conf/domain_validate.c | 11 +++++++++++
>> src/conf/schemas/domaincommon.rng | 5 +++++
>> src/qemu/qemu_command.c | 7 +++++++
>
> missing test case addition to qemuxmlconftest which proves both the XML
> parser/formatter and the commandline generator.
Thanks for the reminder, I'll add some test cases for v2.
>> 6 files changed, 37 insertions(+)
>>
>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>> index 976746e292..7fe8b03a56 100644
>> --- a/docs/formatdomain.rst
>> +++ b/docs/formatdomain.rst
>> @@ -7103,6 +7103,14 @@ A video device.
>> sub-element is valid for model types "vga", "qxl", "bochs", "gop",
>> and "virtio".
>>
>> + :since:`Since 11.7.0` (QEMU driver only), the ``model`` element may have an
>> + optional ``edid`` attribute that can be set to "on" or "off". If the ``edid``
>> + attribute is not specified then the device will use its default value.
>> + Otherwise setting ``edid`` to "on" will expose the device EDID blob to the
>> + guest, whilst setting it to "off" will hide the device EDID blob from the
>> + guest. The ``edid`` attribute is only valid for model types "vga", "bochs",
>> + and "virtio".
>> +
>> ``acceleration``
>> Configure if video acceleration should be enabled.
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 59958c2f08..10cc6d7432 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -13535,6 +13535,9 @@ virDomainVideoModelDefParseXML(virDomainVideoDef *def,
>> if (virXMLPropTristateSwitch(node, "blob", VIR_XML_PROP_NONE, &def->blob) < 0)
>> return -1;
>>
>> + if (virXMLPropTristateSwitch(node, "edid", VIR_XML_PROP_NONE, &def->edid) < 0)
>> + return -1;
>> +
>> return 0;
>> }
>>
>> @@ -26629,6 +26632,8 @@ virDomainVideoDefFormat(virBuffer *buf,
>> virBufferAddLit(buf, " primary='yes'");
>> if (def->blob != VIR_TRISTATE_SWITCH_ABSENT)
>> virBufferAsprintf(buf, " blob='%s'", virTristateSwitchTypeToString(def->blob));
>> + if (def->edid != VIR_TRISTATE_SWITCH_ABSENT)
>> + virBufferAsprintf(buf, " edid='%s'", virTristateSwitchTypeToString(def->edid));
>> if (def->accel || def->res) {
>> virBufferAddLit(buf, ">\n");
>> virBufferAdjustIndent(buf, 2);
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 596d138973..425ccfa97c 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -1895,6 +1895,7 @@ struct _virDomainVideoDef {
>> virDomainDeviceInfo info;
>> virDomainVirtioOptions *virtio;
>> virDomainVideoBackendType backend;
>> + virTristateSwitch edid;
>> };
>>
>> /* graphics console modes */
>> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
>> index 40edecef83..60a2e46b7e 100644
>> --- a/src/conf/domain_validate.c
>> +++ b/src/conf/domain_validate.c
>> @@ -231,6 +231,17 @@ virDomainVideoDefValidate(const virDomainVideoDef *video,
>> }
>> }
>>
>> + if ((video->type != VIR_DOMAIN_VIDEO_TYPE_BOCHS) &&
>> + (video->type != VIR_DOMAIN_VIDEO_TYPE_VGA) &&
>> + (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO)) {
>> + if (video->edid != VIR_TRISTATE_SWITCH_ABSENT) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("video type '%1$s' does not support edid"),
>> + virDomainVideoTypeToString(video->type));
>> + return -1;
>> + }
>> + }
>> +
>> return 0;
>> }
>>
>> diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
>> index a714c3fcc5..d84f20637c 100644
>> --- a/src/conf/schemas/domaincommon.rng
>> +++ b/src/conf/schemas/domaincommon.rng
>> @@ -4807,6 +4807,11 @@
>> <ref name="virOnOff"/>
>> </attribute>
>> </optional>
>> + <optional>
>> + <attribute name="edid">
>> + <ref name="virOnOff"/>
>> + </attribute>
>> + </optional>
>> <optional>
>> <element name="acceleration">
>> <optional>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 457dee7029..db00f9b173 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -4739,6 +4739,13 @@ qemuBuildDeviceVideoCmd(virCommand *cmd,
>> return -1;
>> }
>>
>> + if ((video->type == VIR_DOMAIN_VIDEO_TYPE_BOCHS) ||
>> + (video->type == VIR_DOMAIN_VIDEO_TYPE_VGA) ||
>> + (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO)) {
>
> This check should not be needed becxause the validation code ensures
> that we don't allow it elsewhere. This should make the formatter
> simpler.
I see. I'll replace the if() with a check for (video->edid !=
VIR_TRISTATE_SWITCH_ABSENT) instead.
>> + if (virJSONValueObjectAdd(&props, "T:edid", video->edid, NULL) < 0)
>> + return -1;
>> + }
>> +
>> if (video->res) {
>> if (virJSONValueObjectAdd(&props,
>> "p:xres", video->res->x,
>> --
>> 2.43.0
Many thanks,
Mark.
On Thu, Jul 31, 2025 at 17:05:18 +0100, Mark Cave-Ayland wrote:
> On 31/07/2025 12:52, Peter Krempa wrote:
>
> > On Thu, Jul 31, 2025 at 10:50:40 +0100, Mark Cave-Ayland wrote:
> > > Add the ability to enable/disable exposing the EDID information to the guest.
> > > This allows migration from legacy machine types that have EDID disabled to a
> > > newer machine type without any change becoming visible to the guest.
> >
> > If this option has guest OS visible effects it ought to be covered by
> > the ABI stability checks which it isn't.
> >
> > That though prevents it to be changed during migrations though.
> >
> > In addition qemu itself has mechanisms that allow different behaviour
> > based on machine type for the exact reason to preserve guest ABI, but
> > allow changes in the future.
> >
> > Is this feature covered by this?
> >
> > If not libvirt might need to probe the current state to behave
> > correctly.
>
> I took a look at QEMU and it looks like the edid property was added back in
> 2018 for QEMU 3.1.0, so I think we should be good?
Sorry for not being clear enough. I wasn't talking about detecting
support in qemu here.
I was talking about detecting the actual state of the feature, after
qemu is started, so that it can be represented in the XML.
That would be necessary if the value isn't bound to a machine type in
qemu, but the default did change already.
That way libvirt could ensure that the state doesn't change on
migration.
If it is bound to a machine type, it will properly be handled by qemu.
Libvirt will still require an ABI stability check so that the users
don't change the explicitly provided value.
> > > The edid attribute can specified in the domain XML as below:
> > >
> > > <video>
> > > <model type='virtio' edid='off'/>
> > > </video>
> > >
> > > If the edid attribute is unspecified, it is not generated so that the
> > > virtualisation platform will continue to use its default.
> > >
> > > The edid attribute is only valid for the vga, boch and virtio display models
> > > and is currently only implemented for the QEMU driver.
> > >
> > > Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> > > ---
> > > docs/formatdomain.rst | 8 ++++++++
> > > src/conf/domain_conf.c | 5 +++++
> > > src/conf/domain_conf.h | 1 +
> > > src/conf/domain_validate.c | 11 +++++++++++
> > > src/conf/schemas/domaincommon.rng | 5 +++++
> > > src/qemu/qemu_command.c | 7 +++++++
> >
> > missing test case addition to qemuxmlconftest which proves both the XML
> > parser/formatter and the commandline generator.
>
> Thanks for the reminder, I'll add some test cases for v2.
>
> > > 6 files changed, 37 insertions(+)
> > >
> > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > > index 976746e292..7fe8b03a56 100644
> > > --- a/docs/formatdomain.rst
> > > +++ b/docs/formatdomain.rst
> > > @@ -7103,6 +7103,14 @@ A video device.
> > > sub-element is valid for model types "vga", "qxl", "bochs", "gop",
> > > and "virtio".
> > > + :since:`Since 11.7.0` (QEMU driver only), the ``model`` element may have an
> > > + optional ``edid`` attribute that can be set to "on" or "off". If the ``edid``
> > > + attribute is not specified then the device will use its default value.
> > > + Otherwise setting ``edid`` to "on" will expose the device EDID blob to the
> > > + guest, whilst setting it to "off" will hide the device EDID blob from the
> > > + guest. The ``edid`` attribute is only valid for model types "vga", "bochs",
> > > + and "virtio".
> > > +
> > > ``acceleration``
> > > Configure if video acceleration should be enabled.
> > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > > index 59958c2f08..10cc6d7432 100644
> > > --- a/src/conf/domain_conf.c
> > > +++ b/src/conf/domain_conf.c
> > > @@ -13535,6 +13535,9 @@ virDomainVideoModelDefParseXML(virDomainVideoDef *def,
> > > if (virXMLPropTristateSwitch(node, "blob", VIR_XML_PROP_NONE, &def->blob) < 0)
> > > return -1;
> > > + if (virXMLPropTristateSwitch(node, "edid", VIR_XML_PROP_NONE, &def->edid) < 0)
> > > + return -1;
> > > +
> > > return 0;
> > > }
> > > @@ -26629,6 +26632,8 @@ virDomainVideoDefFormat(virBuffer *buf,
> > > virBufferAddLit(buf, " primary='yes'");
> > > if (def->blob != VIR_TRISTATE_SWITCH_ABSENT)
> > > virBufferAsprintf(buf, " blob='%s'", virTristateSwitchTypeToString(def->blob));
> > > + if (def->edid != VIR_TRISTATE_SWITCH_ABSENT)
> > > + virBufferAsprintf(buf, " edid='%s'", virTristateSwitchTypeToString(def->edid));
> > > if (def->accel || def->res) {
> > > virBufferAddLit(buf, ">\n");
> > > virBufferAdjustIndent(buf, 2);
> > > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > > index 596d138973..425ccfa97c 100644
> > > --- a/src/conf/domain_conf.h
> > > +++ b/src/conf/domain_conf.h
> > > @@ -1895,6 +1895,7 @@ struct _virDomainVideoDef {
> > > virDomainDeviceInfo info;
> > > virDomainVirtioOptions *virtio;
> > > virDomainVideoBackendType backend;
> > > + virTristateSwitch edid;
> > > };
> > > /* graphics console modes */
> > > diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> > > index 40edecef83..60a2e46b7e 100644
> > > --- a/src/conf/domain_validate.c
> > > +++ b/src/conf/domain_validate.c
> > > @@ -231,6 +231,17 @@ virDomainVideoDefValidate(const virDomainVideoDef *video,
> > > }
> > > }
> > > + if ((video->type != VIR_DOMAIN_VIDEO_TYPE_BOCHS) &&
> > > + (video->type != VIR_DOMAIN_VIDEO_TYPE_VGA) &&
> > > + (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO)) {
> > > + if (video->edid != VIR_TRISTATE_SWITCH_ABSENT) {
> > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > > + _("video type '%1$s' does not support edid"),
> > > + virDomainVideoTypeToString(video->type));
> > > + return -1;
> > > + }
> > > + }
> > > +
> > > return 0;
> > > }
> > > diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
> > > index a714c3fcc5..d84f20637c 100644
> > > --- a/src/conf/schemas/domaincommon.rng
> > > +++ b/src/conf/schemas/domaincommon.rng
> > > @@ -4807,6 +4807,11 @@
> > > <ref name="virOnOff"/>
> > > </attribute>
> > > </optional>
> > > + <optional>
> > > + <attribute name="edid">
> > > + <ref name="virOnOff"/>
> > > + </attribute>
> > > + </optional>
> > > <optional>
> > > <element name="acceleration">
> > > <optional>
> > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > > index 457dee7029..db00f9b173 100644
> > > --- a/src/qemu/qemu_command.c
> > > +++ b/src/qemu/qemu_command.c
> > > @@ -4739,6 +4739,13 @@ qemuBuildDeviceVideoCmd(virCommand *cmd,
> > > return -1;
> > > }
> > > + if ((video->type == VIR_DOMAIN_VIDEO_TYPE_BOCHS) ||
> > > + (video->type == VIR_DOMAIN_VIDEO_TYPE_VGA) ||
> > > + (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO)) {
> >
> > This check should not be needed becxause the validation code ensures
> > that we don't allow it elsewhere. This should make the formatter
> > simpler.
>
> I see. I'll replace the if() with a check for (video->edid !=
> VIR_TRISTATE_SWITCH_ABSENT) instead.
That's what the 'T:' conversion does implicitly. It outputs nothing if
the value is _ABSENT.
>
> > > + if (virJSONValueObjectAdd(&props, "T:edid", video->edid, NULL) < 0)
> > > + return -1;
> > > + }
> > > +
> > > if (video->res) {
> > > if (virJSONValueObjectAdd(&props,
> > > "p:xres", video->res->x,
> > > --
> > > 2.43.0
>
>
> Many thanks,
>
> Mark.
>
On 31/07/2025 17:12, Peter Krempa wrote:
> On Thu, Jul 31, 2025 at 17:05:18 +0100, Mark Cave-Ayland wrote:
>> On 31/07/2025 12:52, Peter Krempa wrote:
>>
>>> On Thu, Jul 31, 2025 at 10:50:40 +0100, Mark Cave-Ayland wrote:
>>>> Add the ability to enable/disable exposing the EDID information to the guest.
>>>> This allows migration from legacy machine types that have EDID disabled to a
>>>> newer machine type without any change becoming visible to the guest.
>>>
>>> If this option has guest OS visible effects it ought to be covered by
>>> the ABI stability checks which it isn't.
>>>
>>> That though prevents it to be changed during migrations though.
>>>
>>> In addition qemu itself has mechanisms that allow different behaviour
>>> based on machine type for the exact reason to preserve guest ABI, but
>>> allow changes in the future.
>>>
>>> Is this feature covered by this?
>>>
>>> If not libvirt might need to probe the current state to behave
>>> correctly.
>>
>> I took a look at QEMU and it looks like the edid property was added back in
>> 2018 for QEMU 3.1.0, so I think we should be good?
>
> Sorry for not being clear enough. I wasn't talking about detecting
> support in qemu here.
>
> I was talking about detecting the actual state of the feature, after
> qemu is started, so that it can be represented in the XML.
>
> That would be necessary if the value isn't bound to a machine type in
> qemu, but the default did change already.
>
> That way libvirt could ensure that the state doesn't change on
> migration.
>
> If it is bound to a machine type, it will properly be handled by qemu.
I just checked QEMU and the edid properties are stored in
hw_compat_4_0[]. According to https://libvirt.org/news.html, libvirt
requires a minimum of QEMU 6.2 so I think this should be fine.
> Libvirt will still require an ABI stability check so that the users
> don't change the explicitly provided value.
Is this still required? If so, can you point me towards an example of this?
>>>> The edid attribute can specified in the domain XML as below:
>>>>
>>>> <video>
>>>> <model type='virtio' edid='off'/>
>>>> </video>
>>>>
>>>> If the edid attribute is unspecified, it is not generated so that the
>>>> virtualisation platform will continue to use its default.
>>>>
>>>> The edid attribute is only valid for the vga, boch and virtio display models
>>>> and is currently only implemented for the QEMU driver.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>>>> ---
>>>> docs/formatdomain.rst | 8 ++++++++
>>>> src/conf/domain_conf.c | 5 +++++
>>>> src/conf/domain_conf.h | 1 +
>>>> src/conf/domain_validate.c | 11 +++++++++++
>>>> src/conf/schemas/domaincommon.rng | 5 +++++
>>>> src/qemu/qemu_command.c | 7 +++++++
>>>
>>> missing test case addition to qemuxmlconftest which proves both the XML
>>> parser/formatter and the commandline generator.
>>
>> Thanks for the reminder, I'll add some test cases for v2.
>>
>>>> 6 files changed, 37 insertions(+)
>>>>
>>>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>>>> index 976746e292..7fe8b03a56 100644
>>>> --- a/docs/formatdomain.rst
>>>> +++ b/docs/formatdomain.rst
>>>> @@ -7103,6 +7103,14 @@ A video device.
>>>> sub-element is valid for model types "vga", "qxl", "bochs", "gop",
>>>> and "virtio".
>>>> + :since:`Since 11.7.0` (QEMU driver only), the ``model`` element may have an
>>>> + optional ``edid`` attribute that can be set to "on" or "off". If the ``edid``
>>>> + attribute is not specified then the device will use its default value.
>>>> + Otherwise setting ``edid`` to "on" will expose the device EDID blob to the
>>>> + guest, whilst setting it to "off" will hide the device EDID blob from the
>>>> + guest. The ``edid`` attribute is only valid for model types "vga", "bochs",
>>>> + and "virtio".
>>>> +
>>>> ``acceleration``
>>>> Configure if video acceleration should be enabled.
>>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>>> index 59958c2f08..10cc6d7432 100644
>>>> --- a/src/conf/domain_conf.c
>>>> +++ b/src/conf/domain_conf.c
>>>> @@ -13535,6 +13535,9 @@ virDomainVideoModelDefParseXML(virDomainVideoDef *def,
>>>> if (virXMLPropTristateSwitch(node, "blob", VIR_XML_PROP_NONE, &def->blob) < 0)
>>>> return -1;
>>>> + if (virXMLPropTristateSwitch(node, "edid", VIR_XML_PROP_NONE, &def->edid) < 0)
>>>> + return -1;
>>>> +
>>>> return 0;
>>>> }
>>>> @@ -26629,6 +26632,8 @@ virDomainVideoDefFormat(virBuffer *buf,
>>>> virBufferAddLit(buf, " primary='yes'");
>>>> if (def->blob != VIR_TRISTATE_SWITCH_ABSENT)
>>>> virBufferAsprintf(buf, " blob='%s'", virTristateSwitchTypeToString(def->blob));
>>>> + if (def->edid != VIR_TRISTATE_SWITCH_ABSENT)
>>>> + virBufferAsprintf(buf, " edid='%s'", virTristateSwitchTypeToString(def->edid));
>>>> if (def->accel || def->res) {
>>>> virBufferAddLit(buf, ">\n");
>>>> virBufferAdjustIndent(buf, 2);
>>>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>>>> index 596d138973..425ccfa97c 100644
>>>> --- a/src/conf/domain_conf.h
>>>> +++ b/src/conf/domain_conf.h
>>>> @@ -1895,6 +1895,7 @@ struct _virDomainVideoDef {
>>>> virDomainDeviceInfo info;
>>>> virDomainVirtioOptions *virtio;
>>>> virDomainVideoBackendType backend;
>>>> + virTristateSwitch edid;
>>>> };
>>>> /* graphics console modes */
>>>> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
>>>> index 40edecef83..60a2e46b7e 100644
>>>> --- a/src/conf/domain_validate.c
>>>> +++ b/src/conf/domain_validate.c
>>>> @@ -231,6 +231,17 @@ virDomainVideoDefValidate(const virDomainVideoDef *video,
>>>> }
>>>> }
>>>> + if ((video->type != VIR_DOMAIN_VIDEO_TYPE_BOCHS) &&
>>>> + (video->type != VIR_DOMAIN_VIDEO_TYPE_VGA) &&
>>>> + (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO)) {
>>>> + if (video->edid != VIR_TRISTATE_SWITCH_ABSENT) {
>>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>>> + _("video type '%1$s' does not support edid"),
>>>> + virDomainVideoTypeToString(video->type));
>>>> + return -1;
>>>> + }
>>>> + }
>>>> +
>>>> return 0;
>>>> }
>>>> diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
>>>> index a714c3fcc5..d84f20637c 100644
>>>> --- a/src/conf/schemas/domaincommon.rng
>>>> +++ b/src/conf/schemas/domaincommon.rng
>>>> @@ -4807,6 +4807,11 @@
>>>> <ref name="virOnOff"/>
>>>> </attribute>
>>>> </optional>
>>>> + <optional>
>>>> + <attribute name="edid">
>>>> + <ref name="virOnOff"/>
>>>> + </attribute>
>>>> + </optional>
>>>> <optional>
>>>> <element name="acceleration">
>>>> <optional>
>>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>>> index 457dee7029..db00f9b173 100644
>>>> --- a/src/qemu/qemu_command.c
>>>> +++ b/src/qemu/qemu_command.c
>>>> @@ -4739,6 +4739,13 @@ qemuBuildDeviceVideoCmd(virCommand *cmd,
>>>> return -1;
>>>> }
>>>> + if ((video->type == VIR_DOMAIN_VIDEO_TYPE_BOCHS) ||
>>>> + (video->type == VIR_DOMAIN_VIDEO_TYPE_VGA) ||
>>>> + (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO)) {
>>>
>>> This check should not be needed becxause the validation code ensures
>>> that we don't allow it elsewhere. This should make the formatter
>>> simpler.
>>
>> I see. I'll replace the if() with a check for (video->edid !=
>> VIR_TRISTATE_SWITCH_ABSENT) instead.
>
> That's what the 'T:' conversion does implicitly. It outputs nothing if
> the value is _ABSENT.
Thanks for the confirmation!
>>>> + if (virJSONValueObjectAdd(&props, "T:edid", video->edid, NULL) < 0)
>>>> + return -1;
>>>> + }
>>>> +
>>>> if (video->res) {
>>>> if (virJSONValueObjectAdd(&props,
>>>> "p:xres", video->res->x,
>>>> --
>>>> 2.43.0
ATB,
Mark.
© 2016 - 2025 Red Hat, Inc.