Re: [libvirt] [Qemu-devel] [PULL v3 00/54] Kconfig conversion, excluding ARM and MIPS
Hi Paolo, (+libvir-list) I'd like to report a regression introduced by this patch set: On 03/07/19 21:58, Paolo Bonzini wrote:
The following changes since commit 6cb4f6db4f4367faa33da85b15f75bbbd2bed2a6:
Merge remote-tracking branch 'remotes/cleber/tags/python-next-pull-request' into staging (2019-03-07 16:16:02 +0000)
are available in the Git repository at:
git://github.com/bonzini/qemu.git tags/for-upstream-kconfig
for you to fetch changes up to 576c3f2f16e7392e28cc9fe10d9a920d67d3645b:
kconfig: add documentation (2019-03-07 21:54:22 +0100)
---------------------------------------------------------------- Initial Kconfig work, excluding ARM and MIPS
----------------------------------------------------------------
I use "libvirt-daemon-4.5.0-10.el7.x86_64" at the moment. Here are the symptoms: (1) the output of the following command changes across this series: virsh domcapabilities qemu /opt/qemu-installed-optimized/bin/qemu-system-aarch64 aarch64 virt-4.0 as shown by the diff
@@ -92,6 +92,8 @@ <video supported='yes'> <enum name='modelType'> <value>vga</value> + <value>cirrus</value> + <value>vmvga</value> <value>virtio</value> </enum> </video> @@ -111,10 +113,7 @@ <value>scsi</value> </enum> <enum name='capsType'/> - <enum name='pciBackend'> - <value>default</value> - <value>vfio</value> - </enum> + <enum name='pciBackend'/> </hostdev> </devices> <features>
(2) given the following domain XML snippet, in a qemu (not KVM) aarch64 guest that I have,
<video> <model type='virtio' heads='1' primary='yes'/> <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x0'/> </video>
the QEMU command line generated by libvirt changes:
- -device virtio-gpu-pci,id=video0,max_outputs=1,bus=pci.4,addr=0x0 \ + -device virtio-vga,id=video0,max_outputs=1,bus=pci.4,addr=0x0 \
Perhaps this series causes (1), which in turn causes (2), or else this series causes both (1) and (2) independently of each other. I'm not sure about that. Either way, (2) is a problem, because virtio-vga is inappropriate for aarch64 guests (and the ArmVirtQemu platform firmware in edk2 won't even try to drive it -- a blank screen is the result). Now, I can't pinpoint an exact commit, because (after a *brutal* bisection) I get:
There are only 'skip'ped commits left to test. The first bad commit could be any of: a02c0edb35662de38d400f42b68845540669ca3d 03b348bdcbd1eda4ce097b2b84527dec774d80c4 d6e9c470fc91f75db1785f17a9d3567d5a27953d a7e23159074c9d774fb1e88357b778994a0c9365 bcb129b3154ba743f8e52c21c331a0dfcaee7c38 02017ee385ef574133c4a978d368640772978178 7c28b925b7e176b4e44ed05d23cf883561000546 1550b0e6bfe3ab6985e5ad75df1c528a0ca39468 e9947d18df97e6c6584f020cf9cc995404cc8061 8f01b41e1098d8cb9491fa3ea7bd59cf187a5bd7 9533dcdd416530a0d72140c122bf90517b6c81eb 32690c8bed5762518272876dcb6dd39a54f54fd1 c3a98aa54c734dcb7a36d193c6330d8f04d4bf8e ccf222a816d59af1318a7efb59c6b9c5578d1abf f349474920d80838ecea3d421531fdb0660b8740 2ac041c2c3d89691cda1657981c41fe4bc20244b e0e312f3525ad6ac18ba6633af29190dd9620cbc b42075bb77672616127c9452c0f836e757e9ce1a We cannot bisect more!
Here's how I built each step of the bisection: - "git clean -ffdx" and "git submodule deinit --all --force" on the source directory - create a new, pristine build directory - configure QEMU for out-of-tree build, in that directory - select the i386, x86_64, arm, and aarch64 targets (all softmmu) - report "skip" to git-bisect iff the build breaks, otherwise test the aarch64 emulator through libvirt, and report good/bad dependent on the test result. The bisection log is attached -- "git bisect replay" and "git bisect visualize" should tell the story better than the above dump of commit hashes. Basically: - The tree builds up to and including commit 82f5181777eb ("kconfig: introduce kconfig files", 2019-03-07), and that commit works fine as well. - Then the build breaks (starting with commit e0e312f3525a, "build: switch to Kconfig", 2019-03-07). - The tree becomes buildable again at commit b42075bb7767 ("virtio: express virtio dependencies with Kconfig", 2019-03-07), but at that point, the functionality has been regressed. Thanks, Laszlo
On 03/21/19 01:53, Laszlo Ersek wrote:
Hi Paolo,
(+libvir-list)
I'd like to report a regression introduced by this patch set:
On 03/07/19 21:58, Paolo Bonzini wrote:
The following changes since commit 6cb4f6db4f4367faa33da85b15f75bbbd2bed2a6:
Merge remote-tracking branch 'remotes/cleber/tags/python-next-pull-request' into staging (2019-03-07 16:16:02 +0000)
are available in the Git repository at:
git://github.com/bonzini/qemu.git tags/for-upstream-kconfig
for you to fetch changes up to 576c3f2f16e7392e28cc9fe10d9a920d67d3645b:
kconfig: add documentation (2019-03-07 21:54:22 +0100)
---------------------------------------------------------------- Initial Kconfig work, excluding ARM and MIPS
----------------------------------------------------------------
In brief, the regression is that the aarch64 system emulator now lists the "virtio-vga" device for the "virt" machine type: $ qemu-system-aarch64 -M virt -device \? | grep virtio-vga name "virtio-vga", bus PCI Here's where I think the issue was introduced: (1) In commit 82f5181777eb ("kconfig: introduce kconfig files", 2019-03-07), VIRTIO_VGA was introduced simply as a bool (with no other clauses) in "hw/display/Kconfig". (2) In commit 7c28b925b7e1 ("build: convert pci.mak to Kconfig", 2019-03-07), VIRTIO_VGA received the following clauses: default y if PCI_DEVICES depends on VIRTIO_PCI select VGA This is too lax, because it permits virtio-vga for aarch64, while that shouldn't happen (and it doesn't matches the previous state of the tree). (3) In commit b42075bb7767 ("virtio: express virtio dependencies with Kconfig", 2019-03-07), the determination of virtio-vga was switched to the Kconfig system. Importantly, in this patch, the line CONFIG_VIRTIO_VGA=y was removed *only* from the following file: default-configs/i386-softmmu.mak (4) Both of the remaining instances of CONFIG_VIRTIO_VGA=y were then removed in commit 9483cf27dd36 ("hppa-softmmu.mak: express dependencies with Kconfig", 2019-03-07) and commit 87f9108bad0c ("ppc64: Express dependencies of 'pseries' and 'powernv' machines with kconfig", 2019-03-07), from files default-configs/hppa-softmmu.mak default-configs/ppc64-softmmu.mak respectively. Therefore I think commit 7c28b925b7e1 has the bug. The VIRTIO_VGA restriction depends on VIRTIO_PCI should now be replaced with depends on VIRTIO_PCI && (PC || DINO || PSERIES) Thanks Laszlo
On Thu, 21 Mar 2019 at 12:40, Laszlo Ersek <lersek@redhat.com> wrote:
In brief, the regression is that the aarch64 system emulator now lists the "virtio-vga" device for the "virt" machine type:
$ qemu-system-aarch64 -M virt -device \? | grep virtio-vga name "virtio-vga", bus PCI
Here's where I think the issue was introduced:
(1) In commit 82f5181777eb ("kconfig: introduce kconfig files", 2019-03-07), VIRTIO_VGA was introduced simply as a bool (with no other clauses) in "hw/display/Kconfig".
(2) In commit 7c28b925b7e1 ("build: convert pci.mak to Kconfig", 2019-03-07), VIRTIO_VGA received the following clauses:
default y if PCI_DEVICES depends on VIRTIO_PCI select VGA
This is too lax, because it permits virtio-vga for aarch64, while that shouldn't happen (and it doesn't matches the previous state of the tree).
(3) In commit b42075bb7767 ("virtio: express virtio dependencies with Kconfig", 2019-03-07), the determination of virtio-vga was switched to the Kconfig system. Importantly, in this patch, the line
CONFIG_VIRTIO_VGA=y
was removed *only* from the following file:
default-configs/i386-softmmu.mak
(4) Both of the remaining instances of
CONFIG_VIRTIO_VGA=y
were then removed in commit 9483cf27dd36 ("hppa-softmmu.mak: express dependencies with Kconfig", 2019-03-07) and commit 87f9108bad0c ("ppc64: Express dependencies of 'pseries' and 'powernv' machines with kconfig", 2019-03-07), from files
default-configs/hppa-softmmu.mak default-configs/ppc64-softmmu.mak
respectively.
Therefore I think commit 7c28b925b7e1 has the bug. The VIRTIO_VGA restriction
depends on VIRTIO_PCI
should now be replaced with
depends on VIRTIO_PCI && (PC || DINO || PSERIES)
Should it? What makes the virtio-vga device only suitable for those three machines? thanks -- PMM
On 21/03/19 14:04, Peter Maydell wrote:
were then removed in commit 9483cf27dd36 ("hppa-softmmu.mak: express dependencies with Kconfig", 2019-03-07) and commit 87f9108bad0c ("ppc64: Express dependencies of 'pseries' and 'powernv' machines with kconfig", 2019-03-07), from files
default-configs/hppa-softmmu.mak default-configs/ppc64-softmmu.mak
respectively.
Therefore I think commit 7c28b925b7e1 has the bug. The VIRTIO_VGA restriction
depends on VIRTIO_PCI
should now be replaced with
depends on VIRTIO_PCI && (PC || DINO || PSERIES)
Should it? What makes the virtio-vga device only suitable for those three machines?
The idea was that to only support virtio-vga for those machines where the firmware doesn't know about virtio-gpu and supported VGA legacy hardware before virtio-{gpu,vga} were introduced. I think this is a bug in libvirt, but at least for 4.0 it should be fixed/worked around in QEMU. Paolo
On 03/21/19 14:04, Peter Maydell wrote:
On Thu, 21 Mar 2019 at 12:40, Laszlo Ersek <lersek@redhat.com> wrote:
In brief, the regression is that the aarch64 system emulator now lists the "virtio-vga" device for the "virt" machine type:
$ qemu-system-aarch64 -M virt -device \? | grep virtio-vga name "virtio-vga", bus PCI
Here's where I think the issue was introduced:
(1) In commit 82f5181777eb ("kconfig: introduce kconfig files", 2019-03-07), VIRTIO_VGA was introduced simply as a bool (with no other clauses) in "hw/display/Kconfig".
(2) In commit 7c28b925b7e1 ("build: convert pci.mak to Kconfig", 2019-03-07), VIRTIO_VGA received the following clauses:
default y if PCI_DEVICES depends on VIRTIO_PCI select VGA
This is too lax, because it permits virtio-vga for aarch64, while that shouldn't happen (and it doesn't matches the previous state of the tree).
(3) In commit b42075bb7767 ("virtio: express virtio dependencies with Kconfig", 2019-03-07), the determination of virtio-vga was switched to the Kconfig system. Importantly, in this patch, the line
CONFIG_VIRTIO_VGA=y
was removed *only* from the following file:
default-configs/i386-softmmu.mak
(4) Both of the remaining instances of
CONFIG_VIRTIO_VGA=y
were then removed in commit 9483cf27dd36 ("hppa-softmmu.mak: express dependencies with Kconfig", 2019-03-07) and commit 87f9108bad0c ("ppc64: Express dependencies of 'pseries' and 'powernv' machines with kconfig", 2019-03-07), from files
default-configs/hppa-softmmu.mak default-configs/ppc64-softmmu.mak
respectively.
Therefore I think commit 7c28b925b7e1 has the bug. The VIRTIO_VGA restriction
depends on VIRTIO_PCI
should now be replaced with
depends on VIRTIO_PCI && (PC || DINO || PSERIES)
Should it? What makes the virtio-vga device only suitable for those three machines?
It simply keeps the status quo from before the patchset. The specific emulation targets that virtio-vga should not be enabled for (regardless of other targets / machine types) are arm/aarch64. IOW, in the longer term, it's not that virtio-vga is suitable only for PC || DINO || PSERIES, but that it's *not* suitable for arm/aarch64. My original libvirt commit explains it: https://libvirt.org/git/?p=libvirt.git;a=commit;h=706b5b627719e95a33606c463b... Regarding the guest firmware: in edk2 there are two relevant drivers, QemuVideoDxe and VirtioGpuDxe. The former is framebuffer-based, the latter is a genuine virtio drier (no framebuffer). The former doesn't work on arm/aarch64 KVM, the latter does. The "virtio-vga" device presents a conflict for these drivers because it could be bound by both drivers, at the same time. (The device presents both interfaces.) Arbitration between UEFI drivers is very tricky. To keep things platform independent and sane in both drivers, VirtioGpuDxe never binds virtio-vga, only virtio-gpu-pci. Therefore, in OVMF, where both drivers are included, virtio-vga is deterministically bound by QemuVideoDxe, a framebuffer is exposed, and Windows is happy. You can still use virtio-gpu-pci, and that will be bound by VirtioGpuDxe (windows won't be happy, but you asked for it). Whereas in ArmVirtQemu, only VirtioGpuDxe is included, as QemuVideoDxe has no chance of working (on KVM). If you then boot with virtio-vga, VirtioGpuDxe will still yield... but there won't be another driver to bind the device instead. So, there you *must* use virtio-gpu-pci, for VirtioGpuDxe to like the device. Again, the goal is to keep VirtioGpuDxe and QemuVideoDxe platform-independent; i.e. refrain from aarch64-specific and x64-specific quirks in either. They bind or yield purely based on the device model they see. So, if you allow libvirt to see virtio-vga on aarch64, it will pick virtio-vga for the guest, and then VirtioGpuDxe will not bind it, and there's nothing else to bind it. Thanks, Laszlo
On Thu, 21 Mar 2019 at 17:35, Laszlo Ersek <lersek@redhat.com> wrote:
It simply keeps the status quo from before the patchset.
The specific emulation targets that virtio-vga should not be enabled for (regardless of other targets / machine types) are arm/aarch64. IOW, in the longer term, it's not that virtio-vga is suitable only for PC || DINO || PSERIES, but that it's *not* suitable for arm/aarch64.
Relying on the device not being present for aarch64 seems fragile to me. It will break if we add a different aarch64 machine other than "virt" and want that other machine to have virtio-vga. It will also break if we ever manage to get full heterogenous-CPU support and end up with a single qemu-system binary that can emulate both AArch64 virt and x86-64 pc machines :-) If what you want is "don't use virtio-vga when using KVM on aarch64" then why not have logic that implements that? For 4.0 we should definitely just retain what we had before the introduction of KConfig, but as a general thing we should look to not have to have this odd wrinkle. (AFAIK virtio-vga is no different in this regard to the other vga PCI devices, which also won't work in KVM setups.) thanks -- PMM
On 03/21/19 18:58, Peter Maydell wrote:
On Thu, 21 Mar 2019 at 17:35, Laszlo Ersek <lersek@redhat.com> wrote:
It simply keeps the status quo from before the patchset.
The specific emulation targets that virtio-vga should not be enabled for (regardless of other targets / machine types) are arm/aarch64. IOW, in the longer term, it's not that virtio-vga is suitable only for PC || DINO || PSERIES, but that it's *not* suitable for arm/aarch64.
Relying on the device not being present for aarch64 seems fragile to me. It will break if we add a different aarch64 machine other than "virt" and want that other machine to have virtio-vga. It will also break if we ever manage to get full heterogenous-CPU support and end up with a single qemu-system binary that can emulate both AArch64 virt and x86-64 pc machines :-)
If what you want is "don't use virtio-vga when using KVM on aarch64" then why not have logic that implements that?
That's actually the logic that I implemented originally (or something very close to it -- <https://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=706b5b627719e95a33606c463bc83c841c7b5b0e>), but since then, this code has been modified in libvirt multiple times: * [libvirt] [PATCH v2 00/17] cleanups and improvements for video device code https://www.redhat.com/archives/libvir-list/2016-October/msg00533.html https://libvirt.org/git/?p=libvirt.git;a=shortlog;hp=a62655f9c33a8f7c6257779... * [libvirt] [PATCH v3 0/6] Add support for video and input devices on S390 https://www.redhat.com/archives/libvir-list/2018-March/msg01519.html https://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=4bbf7f8cb5bf979ea2f5c2... I'm not implying that those patches must have introduced a libvirt regression, I just agree that the code deserves investigation. This is why I added libvir-list to the CC list when reporting the problem.
For 4.0 we should definitely just retain what we had before the introduction of KConfig, but as a general thing we should look to not have to have this odd wrinkle.
(AFAIK virtio-vga is no different in this regard to the other vga PCI devices, which also won't work in KVM setups.)
I agree -- please refer to the "domcapabilities" part (1) of my report: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg06060.html
@@ -92,6 +92,8 @@ <video supported='yes'> <enum name='modelType'> <value>vga</value> + <value>cirrus</value> + <value>vmvga</value> <value>virtio</value> </enum> </video>
"cirrus" and "vmvga" look wrong for aarch64. Thanks Laszlo
Le jeu. 21 mars 2019 13:39, Laszlo Ersek <lersek@redhat.com> a écrit :
On 03/21/19 01:53, Laszlo Ersek wrote:
Hi Paolo,
(+libvir-list)
I'd like to report a regression introduced by this patch set:
On 03/07/19 21:58, Paolo Bonzini wrote:
The following changes since commit 6cb4f6db4f4367faa33da85b15f75bbbd2bed2a6:
Merge remote-tracking branch 'remotes/cleber/tags/python-next-pull-request' into staging (2019-03-07 16:16:02 +0000)
are available in the Git repository at:
git://github.com/bonzini/qemu.git tags/for-upstream-kconfig
for you to fetch changes up to 576c3f2f16e7392e28cc9fe10d9a920d67d3645b:
kconfig: add documentation (2019-03-07 21:54:22 +0100)
---------------------------------------------------------------- Initial Kconfig work, excluding ARM and MIPS
----------------------------------------------------------------
In brief, the regression is that the aarch64 system emulator now lists the "virtio-vga" device for the "virt" machine type:
$ qemu-system-aarch64 -M virt -device \? | grep virtio-vga name "virtio-vga", bus PCI
Here's where I think the issue was introduced:
(1) In commit 82f5181777eb ("kconfig: introduce kconfig files", 2019-03-07), VIRTIO_VGA was introduced simply as a bool (with no other clauses) in "hw/display/Kconfig".
(2) In commit 7c28b925b7e1 ("build: convert pci.mak to Kconfig", 2019-03-07), VIRTIO_VGA received the following clauses:
default y if PCI_DEVICES depends on VIRTIO_PCI select VGA
This is too lax, because it permits virtio-vga for aarch64, while that shouldn't happen (and it doesn't matches the previous state of the tree).
Similar case I noticed in another thread. The Virt machine uses a chipset that provides PCI. Does this machine expose PCI slots? If yes, this config is correct, the machine can get any PCI daughter card. Else we shouldn't use PCI_DEVICES that way. Why Virt machine now default to virtio-vga?
(3) In commit b42075bb7767 ("virtio: express virtio dependencies with Kconfig", 2019-03-07), the determination of virtio-vga was switched to the Kconfig system. Importantly, in this patch, the line
CONFIG_VIRTIO_VGA=y
was removed *only* from the following file:
default-configs/i386-softmmu.mak
(4) Both of the remaining instances of
CONFIG_VIRTIO_VGA=y
were then removed in commit 9483cf27dd36 ("hppa-softmmu.mak: express dependencies with Kconfig", 2019-03-07) and commit 87f9108bad0c ("ppc64: Express dependencies of 'pseries' and 'powernv' machines with kconfig", 2019-03-07), from files
default-configs/hppa-softmmu.mak default-configs/ppc64-softmmu.mak
respectively.
Therefore I think commit 7c28b925b7e1 has the bug. The VIRTIO_VGA restriction
depends on VIRTIO_PCI
should now be replaced with
depends on VIRTIO_PCI && (PC || DINO || PSERIES)
Thanks Laszlo
On 03/21/19 15:21, Philippe Mathieu-Daudé wrote:
Le jeu. 21 mars 2019 13:39, Laszlo Ersek <lersek@redhat.com> a écrit :
On 03/21/19 01:53, Laszlo Ersek wrote:
Hi Paolo,
(+libvir-list)
I'd like to report a regression introduced by this patch set:
On 03/07/19 21:58, Paolo Bonzini wrote:
The following changes since commit 6cb4f6db4f4367faa33da85b15f75bbbd2bed2a6:
Merge remote-tracking branch 'remotes/cleber/tags/python-next-pull-request' into staging (2019-03-07 16:16:02 +0000)
are available in the Git repository at:
git://github.com/bonzini/qemu.git tags/for-upstream-kconfig
for you to fetch changes up to 576c3f2f16e7392e28cc9fe10d9a920d67d3645b:
kconfig: add documentation (2019-03-07 21:54:22 +0100)
---------------------------------------------------------------- Initial Kconfig work, excluding ARM and MIPS
----------------------------------------------------------------
In brief, the regression is that the aarch64 system emulator now lists the "virtio-vga" device for the "virt" machine type:
$ qemu-system-aarch64 -M virt -device \? | grep virtio-vga name "virtio-vga", bus PCI
Here's where I think the issue was introduced:
(1) In commit 82f5181777eb ("kconfig: introduce kconfig files", 2019-03-07), VIRTIO_VGA was introduced simply as a bool (with no other clauses) in "hw/display/Kconfig".
(2) In commit 7c28b925b7e1 ("build: convert pci.mak to Kconfig", 2019-03-07), VIRTIO_VGA received the following clauses:
default y if PCI_DEVICES depends on VIRTIO_PCI select VGA
This is too lax, because it permits virtio-vga for aarch64, while that shouldn't happen (and it doesn't matches the previous state of the tree).
Similar case I noticed in another thread. The Virt machine uses a chipset that provides PCI. Does this machine expose PCI slots?
It provides a PCI Express hierarchy, so "yes". (If you mean traditional PCI, that is also available through the pcie-pci bridge device model, which you can plug into the PCIE hierarchy.)
If yes, this config is correct, the machine can get any PCI daughter card.
Haha, right, in theory :) On the same note, the ARMv8 architecture should also, "in theory", have allowed the host side page mappings unconditionally override the guest side mappings, like it works on x86. Because in that case, QemuVideoDxe / the framebuffer would have just worked in aarch64 guests too, I wouldn't have had to write VirtioGpuDxe in the first place, and everybody would have been happy with just virtio-vga. But ARMv8 is not like that, so things don't work like that.
Else we shouldn't use PCI_DEVICES that way.
Why Virt machine now default to virtio-vga?
See above, plus my answer to Peter. Thanks Laszlo
(3) In commit b42075bb7767 ("virtio: express virtio dependencies with Kconfig", 2019-03-07), the determination of virtio-vga was switched to the Kconfig system. Importantly, in this patch, the line
CONFIG_VIRTIO_VGA=y
was removed *only* from the following file:
default-configs/i386-softmmu.mak
(4) Both of the remaining instances of
CONFIG_VIRTIO_VGA=y
were then removed in commit 9483cf27dd36 ("hppa-softmmu.mak: express dependencies with Kconfig", 2019-03-07) and commit 87f9108bad0c ("ppc64: Express dependencies of 'pseries' and 'powernv' machines with kconfig", 2019-03-07), from files
default-configs/hppa-softmmu.mak default-configs/ppc64-softmmu.mak
respectively.
Therefore I think commit 7c28b925b7e1 has the bug. The VIRTIO_VGA restriction
depends on VIRTIO_PCI
should now be replaced with
depends on VIRTIO_PCI && (PC || DINO || PSERIES)
Thanks Laszlo
participants (4)
-
Laszlo Ersek -
Paolo Bonzini -
Peter Maydell -
Philippe Mathieu-Daudé