3:56 p.m.
Hi,
I'm looking into fixing
https://bugzilla.redhat.com/show_bug.cgi?id=1668141
(as a short summary, if a graphics device is added to XML that has no video
device, libvirt automatically adds a video device which is always of type
'cirrus' - even if the underlying qemu doesn't support cirrus).
I'm able to affect the behaviour in question by using qemu capabilities in
qemuDomainDeviceVideoDefPostParse(), see proof-of-concept change in [1]. I
have a couple of questions though:
1) is this a proper place and approach to fix the bug?
2) what would be the full specification of expected behaviour? The bug report
only states that the video type shouldn't be cirrus but doesn't say what it
should be. [2] gives some information about the order of preference of video
device types but I was wondering if there are any opinions about this on this
list?
Cheers,
pvl
[1]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b4175a846e..0de491b79f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7821,7 +7821,8 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDefPtr net,
static int
qemuDomainDeviceVideoDefPostParse(virDomainVideoDefPtr video,
- const virDomainDef *def)
+ const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
{
if (video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) {
if (ARCH_IS_PPC64(def->os.arch))
@@ -7830,8 +7831,16 @@ qemuDomainDeviceVideoDefPostParse(virDomainVideoDefPtr video,
qemuDomainIsRISCVVirt(def) ||
ARCH_IS_S390(def->os.arch))
video->type = VIR_DOMAIN_VIDEO_TYPE_VIRTIO;
- else
- video->type = VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
+ else {
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE)
+ && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) {
+ video->type = VIR_DOMAIN_VIDEO_TYPE_QXL;
+ video->vgamem = QEMU_QXL_VGAMEM_DEFAULT;
+ } else {
+ video->type = VIR_DOMAIN_VIDEO_TYPE_VGA;
+ }
+ }
}
if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
@@ -7926,7 +7935,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
break;
case VIR_DOMAIN_DEVICE_VIDEO:
- ret = qemuDomainDeviceVideoDefPostParse(dev->data.video, def);
+ ret = qemuDomainDeviceVideoDefPostParse(dev->data.video, def, qemuCaps);
break;
case VIR_DOMAIN_DEVICE_PANIC:
[2] https://www.kraxel.org/blog/2019/09/display-devices-in-qemu/
4:22 p.m.
On Mon, Oct 07, 2019 at 02:56:11PM +0200, Pavel Mores wrote:
Hi,
I'm looking into fixing
https://bugzilla.redhat.com/show_bug.cgi?id=1668141
(as a short summary, if a graphics device is added to XML that has no video
device, libvirt automatically adds a video device which is always of type
'cirrus' - even if the underlying qemu doesn't support cirrus).
I'm able to affect the behaviour in question by using qemu capabilities in
qemuDomainDeviceVideoDefPostParse(), see proof-of-concept change in [1]. I
have a couple of questions though:
1) is this a proper place and approach to fix the bug?
I don't think so because the guest ABI could change. Imagine there is an
application that just starts a simple VM (with graphics with no model) and out
of nowhere (after a long time) they upgrade libvirt and qemu and the VM will
look differently.
2) what would be the full specification of expected behaviour? The
bug report
only states that the video type shouldn't be cirrus but doesn't say what it
should be. [2] gives some information about the order of preference of video
device types but I was wondering if there are any opinions about this on this
list?
Reading the BZ it looks to me like virt-manager should allow choosing cirrus and
the different default could be chosen in virt-manager. The BZ is not well
described, to be honest.
Martin
1:16 a.m.
On 11/13/19 8:22 AM, Martin Kletzander wrote:
On Mon, Oct 07, 2019 at 02:56:11PM +0200, Pavel Mores wrote:
>
> Hi,
>
> I'm looking into fixing
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1668141
>
> (as a short summary, if a graphics device is added to XML that has no
> video
> device, libvirt automatically adds a video device which is always of type
> 'cirrus' - even if the underlying qemu doesn't support cirrus).
>
> I'm able to affect the behaviour in question by using qemu
> capabilities in
> qemuDomainDeviceVideoDefPostParse(), see proof-of-concept change in
> [1]. I
> have a couple of questions though:
>
> 1) is this a proper place and approach to fix the bug?
I don't think so because the guest ABI could change. Imagine there is an
application that just starts a simple VM (with graphics with no model)
and out
of nowhere (after a long time) they upgrade libvirt and qemu and the VM
will
look differently.
> 2) what would be the full specification of expected behaviour? The
> bug report
> only states that the video type shouldn't be cirrus but doesn't say
> what it
> should be. [2] gives some information about the order of preference
> of video
> device types but I was wondering if there are any opinions about
> this on this
> list?
>
Reading the BZ it looks to me like virt-manager should allow choosing
cirrus and
the different default could be chosen in virt-manager. The BZ is not well
described, to be honest.
My take on the bug was: libvirt shouldn't use a default configuration
that we blatantly know will result in unbootable qemu. So, don't use
cirrus when qemuCaps tells us that the qemu binary definitely doesn't
have cirrus support.
Pavel's v2 series changes the behavior to:
* use cirrus if it's available, so current working behavior is maintained
* if cirrus is unavailable but vga _is_ available, use that. this fixes
the rhel case, but also likely fixes the default for non-x86 archs that
didn't ever have cirrus
* if cirrus and vga aren't available, explicitly error that the user
needs to specify a video model because we didn't find one for them.
Which all sounds good to me.
Thanks,
Cole
9:15 a.m.
On Wed, Nov 13, 2019 at 05:16:51PM -0500, Cole Robinson wrote:
On 11/13/19 8:22 AM, Martin Kletzander wrote:
> On Mon, Oct 07, 2019 at 02:56:11PM +0200, Pavel Mores wrote:
>>
>> Hi,
>>
>> I'm looking into fixing
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1668141
>>
>> (as a short summary, if a graphics device is added to XML that has no
>> video
>> device, libvirt automatically adds a video device which is always of type
>> 'cirrus' - even if the underlying qemu doesn't support cirrus).
>>
>> I'm able to affect the behaviour in question by using qemu
>> capabilities in
>> qemuDomainDeviceVideoDefPostParse(), see proof-of-concept change in
>> [1]. I
>> have a couple of questions though:
>>
>> 1) is this a proper place and approach to fix the bug?
>
> I don't think so because the guest ABI could change. Imagine there is an
> application that just starts a simple VM (with graphics with no model)
> and out
> of nowhere (after a long time) they upgrade libvirt and qemu and the VM
> will
> look differently.
>
>> 2) what would be the full specification of expected behaviour? The
>> bug report
>> only states that the video type shouldn't be cirrus but doesn't say
>> what it
>> should be. [2] gives some information about the order of preference
>> of video
>> device types but I was wondering if there are any opinions about
>> this on this
>> list?
>>
>
> Reading the BZ it looks to me like virt-manager should allow choosing
> cirrus and
> the different default could be chosen in virt-manager. The BZ is not well
> described, to be honest.
My take on the bug was: libvirt shouldn't use a default configuration
that we blatantly know will result in unbootable qemu. So, don't use
cirrus when qemuCaps tells us that the qemu binary definitely doesn't
have cirrus support.
Oh, I missed that. I was going through old tagged e-mails with no reply, that's
how I got here. If this resulted in unbootable qemu, that's of course something
we can and should change.
Pavel's v2 series changes the behavior to:
* use cirrus if it's available, so current working behavior is maintained
* if cirrus is unavailable but vga _is_ available, use that. this fixes
the rhel case, but also likely fixes the default for non-x86 archs that
didn't ever have cirrus
* if cirrus and vga aren't available, explicitly error that the user
needs to specify a video model because we didn't find one for them.
Which all sounds good to me.
Me too! Thanks for the update.
Thanks,
Cole
1910
days inactive
1948
days old
3 comments
3 participants
participants (3)
-
Cole Robinson -
Martin Kletzander -
Pavel Mores