On 23 August 2017 at 14:15, Leif Lindholm <leif.lindh...@linaro.org> wrote: > On Tue, Aug 22, 2017 at 06:16:56PM +0100, Ard Biesheuvel wrote: >> On 22 August 2017 at 18:02, Laszlo Ersek <ler...@redhat.com> wrote: >> > On 08/22/17 18:30, Ard Biesheuvel wrote: >> >> One of the reasons for introducing virtio-gpu support to OvmfPkg and >> >> ArmVirtpkg was the fact that under KVM virtualization on ARM, the >> >> legacy VGA cannot be used reliably. This is due to an implementation >> >> detail of QEMU+KVM, which remaps cached host memory into the guest >> >> address space as a framebuffer behind a PCI BAR. Given that the purpose >> >> of a memory mapped framebuffer is its side effects, such BARs should >> >> never be mapped cacheable in the guest, and the mismatched attributes >> >> between host and guest result in a loss of coherency, visible as >> >> corruption in the framebuffer image. >> >> >> >> This issue does not occur under TCG emulation, nor did we expect it to >> >> actually bring down the guest under KVM, and so it was deemed harmless >> >> to keep support for the VGA device as well. However, as it turns out, >> >> the fact that the framebuffer BAR is mapped using device semantics by >> >> default may result in unalignment faults when we use the ordinary string >> >> copy routines on the contents. In theory, we could work around this by >> >> remapping the BAR as write combining, but it appears the generic PCI >> >> bus driver does not actually implement this. >> >> >> >> So let's remove the QemuVideoDxe driver altogether. This may result >> >> in loss of functionality for use cases that rely on the framebuffer >> >> to be directly addressable (such as EFIFB), but given that this never >> >> worked reliably under KVM in the first place, let's not let that stop >> >> us from dropping support for it. >> >> >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> >> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org> >> >> --- >> >> ArmVirtPkg/ArmVirtQemu.dsc | 2 -- >> >> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 1 - >> >> ArmVirtPkg/ArmVirtQemuKernel.dsc | 2 -- >> >> 3 files changed, 5 deletions(-) >> >> >> >> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc >> >> index e23a6d17bc44..2e6e76224987 100644 >> >> --- a/ArmVirtPkg/ArmVirtQemu.dsc >> >> +++ b/ArmVirtPkg/ArmVirtQemu.dsc >> >> @@ -57,7 +57,6 @@ >> >> BootLogoLib|MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf >> >> >> >> PlatformBootManagerLib|ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf >> >> >> >> CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf >> >> - >> >> FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf >> >> QemuBootOrderLib|OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf >> >> >> >> FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf >> >> >> >> PciPcdProducerLib|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf >> >> @@ -357,7 +356,6 @@ >> >> # >> >> # Video support >> >> # >> >> - OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> >> OvmfPkg/VirtioGpuDxe/VirtioGpu.inf >> >> OvmfPkg/PlatformDxe/Platform.inf >> >> >> >> diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc >> >> b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc >> >> index 237b2d03a714..3194aa3edc8e 100644 >> >> --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc >> >> +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc >> >> @@ -167,7 +167,6 @@ READ_LOCK_STATUS = TRUE >> >> # >> >> # Video support >> >> # >> >> - INF OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> >> INF OvmfPkg/VirtioGpuDxe/VirtioGpu.inf >> >> INF OvmfPkg/PlatformDxe/Platform.inf >> >> >> >> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc >> >> b/ArmVirtPkg/ArmVirtQemuKernel.dsc >> >> index aa01debfda69..69de887277cb 100644 >> >> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc >> >> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc >> >> @@ -57,7 +57,6 @@ >> >> BootLogoLib|MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf >> >> >> >> PlatformBootManagerLib|ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf >> >> >> >> CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf >> >> - >> >> FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf >> >> QemuBootOrderLib|OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf >> >> >> >> FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf >> >> >> >> PciPcdProducerLib|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf >> >> @@ -348,7 +347,6 @@ >> >> # >> >> # Video support >> >> # >> >> - OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf >> >> OvmfPkg/VirtioGpuDxe/VirtioGpu.inf >> >> OvmfPkg/PlatformDxe/Platform.inf >> >> >> >> >> > >> > (My R-b stands; these are comments for a possible followup patch.) >> > >> > Please see: >> > >> > - commit 84a75f70e903 ("OvmfPkg/QemuVideoDxe: enable ARM builds", >> > 2015-02-23), >> > >> > - commit 05a537945872 ("OvmfPkg/QemuVideoDxe: Helper functions for >> > unaligned port I/O.", 2017-04-07)? >> > >> > In my opinion, we should now revert parts of these commits, in one >> > followup patch: >> > >> > - from the first commit, we should revert only the "VALID_ARCHITECTURES" >> > comment change (the rest is built upon by the second commit, and should >> > be preserved) >> > >> > - from the second commit, we should revert the addition of [Sources.ARM, >> > Sources.AARCH64]. >> > >> > This boils down to removing ARM and AARCH64 references from the >> > QemuVideoDxe.inf file. If you agree, could you please submit such a >> > followup patch? >> >> Sure, but pending the graphical GRUB discussion. > > So, after looking at the GRUB code, I am leaning towards agreeing that > this is actually not a problem at all ... probably. The efi_gop driver > does a Blt() of the entire screen from an off-screen buffer for all > updates _unless_ it fails to allocate that off-screen buffer. > > So, basically, if you run out of memory at that point, it will try to > preserve a way to get messages out about that. I will send a question > out to grub-devel regarding this behaviour. > > However, looking at the specification, a question remains over how > software can determine whether direct FB access is possible. I mean, a > value of 0 seems like a decent hint, but the spec says nothing on the > topic. >
It will assume the FB is accessible unless the pixel format is PixelBltOnly _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel