[PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel

[PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel

[Ard Biesheuvel]

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
 
-- 
2.11.0

Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel

[Laszlo Ersek]

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
>  
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel

[Leif Lindholm]

On Tue, Aug 22, 2017 at 05:30:13PM +0100, 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.

For the record, this would most likely mean we would not be able to
test graphical installers on QEMU. GRUB certainly looks like it's
using FrameBufferBase. Maybe that isn't the most important use-case,
but it's certainly not invalid.

/
    Leif

> 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
>  
> -- 
> 2.11.0
> 

Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel

[Ard Biesheuvel]

On 22 August 2017 at 17:57, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Aug 22, 2017 at 05:30:13PM +0100, 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.
>
> For the record, this would most likely mean we would not be able to
> test graphical installers on QEMU. GRUB certainly looks like it's
> using FrameBufferBase. Maybe that isn't the most important use-case,
> but it's certainly not invalid.
>

This came up at the time, and I /though/ the conclusion was that GRUB
hooks into the stack at a higher level.

I will give Debian's GRUB a quick spin tomorrow.

Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel

[Laszlo Ersek]

On 08/22/17 18:57, Leif Lindholm wrote:
> On Tue, Aug 22, 2017 at 05:30:13PM +0100, 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.
> 
> For the record, this would most likely mean we would not be able to
> test graphical installers on QEMU.

Hmmm, RHEL-7.3+ (and Fedora 2x+, for some "x" I don't recall), for
Aarch64, definitely install in GUI mode, on virtio-gpu-pci, *including*
a perfectly working grub2. I'd been obsessed with this (i.e., a fully
graphical installation in aarch64/KVM guests).

I also have GUI-enabled openSUSE Tumbleweed and Ubuntu guests on my
Mustang. I've booted them just now, and their grub2's work flawlessly.
These guests are not new, the installer ISO names are:

- openSUSE-Tumbleweed-DVD-aarch64-Snapshot20161031-Media.iso
- ubuntu-16.04.2-server-arm64.iso

I remember that for the Ubuntu installation, I had to pick the "HWE
kernel" from the grub menu.

I've uploaded a few screenshots here:

https://people.redhat.com/lersek/aarch64-vgpu-screenshots-7c6bb412-923d-468b-8cfa-5894abd90b40/

> GRUB certainly looks like it's
> using FrameBufferBase.

Please see this (quite small) grub2 patch:
<http://mid.mail-archive.com/1485987247-16230-1-git-send-email-agraf@suse.de>.

(See also the responses from phcoder.)

> Maybe that isn't the most important use-case,
> but it's certainly not invalid.

To me personally, the use case you describe is extremely important (see
above). My impression has been that this has been sorted out for ages.
At least Alex Graf posted the grub2 patch in Feb 2017 (CC'ing him now).

... FWIW, as far as I can see, Fedora and RHEL do *not* carry Alex's
grub2 patch. Yet grub2 works just fine with virtio-gpu-pci (BLT only).

Not sure about the grub2 details, but I consider this problem (graphical
GNU/Linux installers in aarch64 guests, using virtio-gpu-pci) solved;
cross-distro.

Thanks,
Laszlo

>> 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
>>  
>> -- 
>> 2.11.0
>>

Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel

[Leif Lindholm]

On Tue, Aug 22, 2017 at 07:38:03PM +0200, Laszlo Ersek wrote:
> On 08/22/17 18:57, Leif Lindholm wrote:
> > On Tue, Aug 22, 2017 at 05:30:13PM +0100, 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.
> > 
> > For the record, this would most likely mean we would not be able to
> > test graphical installers on QEMU.
> 
> Hmmm, RHEL-7.3+ (and Fedora 2x+, for some "x" I don't recall), for
> Aarch64, definitely install in GUI mode, on virtio-gpu-pci, *including*
> a perfectly working grub2. I'd been obsessed with this (i.e., a fully
> graphical installation in aarch64/KVM guests).

OK, good, then our goals are aligned.

> I also have GUI-enabled openSUSE Tumbleweed and Ubuntu guests on my
> Mustang. I've booted them just now, and their grub2's work flawlessly.
> These guests are not new, the installer ISO names are:
> 
> - openSUSE-Tumbleweed-DVD-aarch64-Snapshot20161031-Media.iso
> - ubuntu-16.04.2-server-arm64.iso
> 
> I remember that for the Ubuntu installation, I had to pick the "HWE
> kernel" from the grub menu.
> 
> I've uploaded a few screenshots here:
> 
> https://people.redhat.com/lersek/aarch64-vgpu-screenshots-7c6bb412-923d-468b-8cfa-5894abd90b40/
> 
> > GRUB certainly looks like it's
> > using FrameBufferBase.
> 
> Please see this (quite small) grub2 patch:
> <http://mid.mail-archive.com/1485987247-16230-1-git-send-email-agraf@suse.de>.
> 
> (See also the responses from phcoder.)
> 
> > Maybe that isn't the most important use-case,
> > but it's certainly not invalid.
> 
> To me personally, the use case you describe is extremely important (see
> above). My impression has been that this has been sorted out for ages.
> At least Alex Graf posted the grub2 patch in Feb 2017 (CC'ing him now).

Yes, grub work pretty much seized for anything other than preparing
the release until that happened, and then stalled for a while
(presumably down to recovery).

> ... FWIW, as far as I can see, Fedora and RHEL do *not* carry Alex's
> grub2 patch. Yet grub2 works just fine with virtio-gpu-pci (BLT only).

"works just fine" is not the same as "are unaffected by the lack of
direct framebuffer access". I will have a skim through the code and
see what (if any) situations could cause direct accesses to the
framebuffer.

> Not sure about the grub2 details, but I consider this problem (graphical
> GNU/Linux installers in aarch64 guests, using virtio-gpu-pci) solved;
> cross-distro.

If we have a way to resolve this situation, I do not religiously
oppose a temporary breakage.

However, if we (for example) end up with "upstream GRUB won't be able
to test the graphic installer of Debian Stable for the next two
years", that will be inconvenient.

Best Regards,

Leif

> Thanks,
> Laszlo
> 
> >> 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
> >>  
> >> -- 
> >> 2.11.0
> >>
> 

Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel

[Ard Biesheuvel]

On 22 August 2017 at 20:05, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Aug 22, 2017 at 07:38:03PM +0200, Laszlo Ersek wrote:
>> On 08/22/17 18:57, Leif Lindholm wrote:
>> > On Tue, Aug 22, 2017 at 05:30:13PM +0100, 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.
>> >
>> > For the record, this would most likely mean we would not be able to
>> > test graphical installers on QEMU.
>>
>> Hmmm, RHEL-7.3+ (and Fedora 2x+, for some "x" I don't recall), for
>> Aarch64, definitely install in GUI mode, on virtio-gpu-pci, *including*
>> a perfectly working grub2. I'd been obsessed with this (i.e., a fully
>> graphical installation in aarch64/KVM guests).
>
> OK, good, then our goals are aligned.
>
>> I also have GUI-enabled openSUSE Tumbleweed and Ubuntu guests on my
>> Mustang. I've booted them just now, and their grub2's work flawlessly.
>> These guests are not new, the installer ISO names are:
>>
>> - openSUSE-Tumbleweed-DVD-aarch64-Snapshot20161031-Media.iso
>> - ubuntu-16.04.2-server-arm64.iso
>>
>> I remember that for the Ubuntu installation, I had to pick the "HWE
>> kernel" from the grub menu.
>>
>> I've uploaded a few screenshots here:
>>
>> https://people.redhat.com/lersek/aarch64-vgpu-screenshots-7c6bb412-923d-468b-8cfa-5894abd90b40/
>>
>> > GRUB certainly looks like it's
>> > using FrameBufferBase.
>>
>> Please see this (quite small) grub2 patch:
>> <http://mid.mail-archive.com/1485987247-16230-1-git-send-email-agraf@suse.de>.
>>
>> (See also the responses from phcoder.)
>>
>> > Maybe that isn't the most important use-case,
>> > but it's certainly not invalid.
>>
>> To me personally, the use case you describe is extremely important (see
>> above). My impression has been that this has been sorted out for ages.
>> At least Alex Graf posted the grub2 patch in Feb 2017 (CC'ing him now).
>
> Yes, grub work pretty much seized for anything other than preparing
> the release until that happened, and then stalled for a while
> (presumably down to recovery).
>
>> ... FWIW, as far as I can see, Fedora and RHEL do *not* carry Alex's
>> grub2 patch. Yet grub2 works just fine with virtio-gpu-pci (BLT only).
>
> "works just fine" is not the same as "are unaffected by the lack of
> direct framebuffer access". I will have a skim through the code and
> see what (if any) situations could cause direct accesses to the
> framebuffer.
>
>> Not sure about the grub2 details, but I consider this problem (graphical
>> GNU/Linux installers in aarch64 guests, using virtio-gpu-pci) solved;
>> cross-distro.
>
> If we have a way to resolve this situation, I do not religiously
> oppose a temporary breakage.
>
> However, if we (for example) end up with "upstream GRUB won't be able
> to test the graphic installer of Debian Stable for the next two
> years", that will be inconvenient.
>

I *think* there is no disagreement here, only Laszlo and I were under
the impression that this was a solved issue.

Interestingly, some GRUB versions without the patch also work,
including Ubuntu's, so I wonder what the deal is here.

I'll dig into this tomorrow.

Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel

[Laszlo Ersek]

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?

Thanks!
Laszlo

Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel

[Ard Biesheuvel]

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.

Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel

[Leif Lindholm]

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.

/
    Leif

Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel

[Ard Biesheuvel]

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

Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel

[Laszlo Ersek]

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

Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel

[Leif Lindholm]

On Wed, Aug 23, 2017 at 03:36:37PM +0200, Laszlo Ersek wrote:
> On 08/23/17 15:17, Ard Biesheuvel wrote:
> >>>> (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.

-ETOOMANYLEVELSOFINDIRECTION
Right, so that's a non-issue.

Hopefully, that means other operating systems can also deal with the
lack.

> IIRC simply recognizing and accepting this enum constant was the point
> of Alex's patch.

Ah, yes, that makes sense now.
It's a bit surprising things work without this patch, but GRUBs
fallback seems to match anyway.

/
    Leif

Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel

[Leif Lindholm]

On Wed, Aug 23, 2017 at 04:00:57PM +0100, Leif Lindholm wrote:
> On Wed, Aug 23, 2017 at 03:36:37PM +0200, Laszlo Ersek wrote:
> > On 08/23/17 15:17, Ard Biesheuvel wrote:
> > >>>> (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.
> 
> -ETOOMANYLEVELSOFINDIRECTION
> Right, so that's a non-issue.
> 
> Hopefully, that means other operating systems can also deal with the
> lack.
> 
> > IIRC simply recognizing and accepting this enum constant was the point
> > of Alex's patch.
> 
> Ah, yes, that makes sense now.
> It's a bit surprising things work without this patch, but GRUBs
> fallback seems to match anyway.

To clarify:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel

[Ard Biesheuvel]

On 24 August 2017 at 13:02, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Aug 23, 2017 at 04:00:57PM +0100, Leif Lindholm wrote:
>> On Wed, Aug 23, 2017 at 03:36:37PM +0200, Laszlo Ersek wrote:
>> > On 08/23/17 15:17, Ard Biesheuvel wrote:
>> > >>>> (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.
>>
>> -ETOOMANYLEVELSOFINDIRECTION
>> Right, so that's a non-issue.
>>
>> Hopefully, that means other operating systems can also deal with the
>> lack.
>>
>> > IIRC simply recognizing and accepting this enum constant was the point
>> > of Alex's patch.
>>
>> Ah, yes, that makes sense now.
>> It's a bit surprising things work without this patch, but GRUBs
>> fallback seems to match anyway.
>
> To clarify:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Pushed, thanks.