From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x232.google.com (mail-io0-x232.google.com [IPv6:2607:f8b0:4001:c06::232]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D4CF421E47D58 for ; Wed, 23 Aug 2017 06:15:00 -0700 (PDT) Received: by mail-io0-x232.google.com with SMTP id g135so305942iog.1 for ; Wed, 23 Aug 2017 06:17:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=4Uc8jASmvwBZgE8Vgq5Uf/E8Tk1XCYMlmnHX1tXa8fM=; b=fVKTbfMR3CTHuX2+QmLn/Z+aYlDqTtRHSXxLCdEyULSc/xuhX6NWvRI2IPEclWGvuP YwcLMJiVUXmHg97+Rc4ejVikjKFydwdVVcZuiF7QwpfPe2ERVXruMzfIRNK8ie6RvIYG 9xK1etrL8Sb25fH+tfndeRKv/wxxDdOl2wjNE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=4Uc8jASmvwBZgE8Vgq5Uf/E8Tk1XCYMlmnHX1tXa8fM=; b=NoiYoA4aOr9FI8x2l9nQAcwNiMf5mKJTnqUr89G7xwONHNi8j7TlH5pSWuQgceyu2Y M/Up5LOlN1qVLgeA6yaMVucMgrg/oCJ7KDiLfg5ciXtybXxszaq5dd7VwMPpgMx8R1Ge j5QnmB9oC4wrvFDeXRi37LLyWn1FkNowbPjLuSC64dWEOlSirb35bvw06AGvHF3tiQVy hfFEUEFjpa7/41oWwl29g6YBU69qMRqMmPON3rJNrDbM0PWX+1PtZ3itzjlkyau2fe3C mjxAE7L3Ks1Ed7VoJYuZ6CX0DR5PDGQQDkUX6V+CshjI5nt3uAB8Mum2L7keBwQssNJE 0mOw== X-Gm-Message-State: AHYfb5iNfl7UZeDtWgPsfWQoNSJffyq3bzo4osvz82leOUNVmyqLnu4y jvAmUa+b36NaTMc5OwgN88etmPIiYhIa X-Received: by 10.107.157.206 with SMTP id g197mr2186045ioe.263.1503494253690; Wed, 23 Aug 2017 06:17:33 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.162.1 with HTTP; Wed, 23 Aug 2017 06:17:32 -0700 (PDT) In-Reply-To: <20170823131516.6f4d2p6jy222ch6m@bivouac.eciton.net> References: <20170822163013.12233-1-ard.biesheuvel@linaro.org> <20170823131516.6f4d2p6jy222ch6m@bivouac.eciton.net> From: Ard Biesheuvel Date: Wed, 23 Aug 2017 14:17:32 +0100 Message-ID: To: Leif Lindholm Cc: Laszlo Ersek , "edk2-devel@lists.01.org" , Jordan Justen Subject: Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Aug 2017 13:15:01 -0000 Content-Type: text/plain; charset="UTF-8" On 23 August 2017 at 14:15, Leif Lindholm wrote: > On Tue, Aug 22, 2017 at 06:16:56PM +0100, Ard Biesheuvel wrote: >> On 22 August 2017 at 18:02, Laszlo Ersek 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 >> >> --- >> >> 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