From: Laszlo Ersek <lersek@redhat.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
Leif Lindholm <leif.lindholm@linaro.org>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
Jordan Justen <jordan.l.justen@intel.com>
Subject: Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel
Date: Wed, 23 Aug 2017 15:36:37 +0200 [thread overview]
Message-ID: <97d8b88a-9583-2a53-7061-33e0351d622b@redhat.com> (raw)
In-Reply-To: <CAKv+Gu93YQNMJUN_yC0Aau_+Hr-eoZbDnQLMxZwMjhkcCHj_Eg@mail.gmail.com>
On 08/23/17 15:17, Ard Biesheuvel wrote:
> On 23 August 2017 at 14:15, Leif Lindholm <leif.lindholm@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 <lersek@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.biesheuvel@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
>
Correct, the UEFI spec (v2.7) says in "12.9 Graphics Output Protocol":
PixelBltOnly This mode does not support a physical frame buffer.
and
PixelFormat Enumeration that defines the physical format of the
pixel. A value of PixelBltOnly implies that a linear
frame buffer is not available for this mode.
IIRC simply recognizing and accepting this enum constant was the point
of Alex's patch.
Thanks
Laszlo
next prev parent reply other threads:[~2017-08-23 13:34 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-22 16:30 [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel Ard Biesheuvel
2017-08-22 16:47 ` Laszlo Ersek
2017-08-22 16:57 ` Leif Lindholm
2017-08-22 17:16 ` Ard Biesheuvel
2017-08-22 17:38 ` Laszlo Ersek
2017-08-22 19:05 ` Leif Lindholm
2017-08-22 19:14 ` Ard Biesheuvel
2017-08-22 17:02 ` Laszlo Ersek
2017-08-22 17:16 ` Ard Biesheuvel
2017-08-23 13:15 ` Leif Lindholm
2017-08-23 13:17 ` Ard Biesheuvel
2017-08-23 13:36 ` Laszlo Ersek [this message]
2017-08-23 15:00 ` Leif Lindholm
2017-08-24 12:02 ` Leif Lindholm
2017-08-24 12:25 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=97d8b88a-9583-2a53-7061-33e0351d622b@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox