public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel
@ 2017-08-22 16:30 Ard Biesheuvel
  2017-08-22 16:47 ` Laszlo Ersek
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2017-08-22 16:30 UTC (permalink / raw)
  To: edk2-devel; +Cc: lersek, leif.lindholm, jordan.l.justen, 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



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel
  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:02 ` Laszlo Ersek
  2 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2017-08-22 16:47 UTC (permalink / raw)
  To: Ard Biesheuvel, edk2-devel; +Cc: leif.lindholm, jordan.l.justen

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel
  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 17:02 ` Laszlo Ersek
  2 siblings, 2 replies; 15+ messages in thread
From: Leif Lindholm @ 2017-08-22 16:57 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel, lersek, jordan.l.justen

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
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel
  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:02 ` Laszlo Ersek
  2017-08-22 17:16   ` Ard Biesheuvel
  2 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2017-08-22 17:02 UTC (permalink / raw)
  To: Ard Biesheuvel, edk2-devel; +Cc: leif.lindholm, jordan.l.justen

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel
  2017-08-22 16:57 ` Leif Lindholm
@ 2017-08-22 17:16   ` Ard Biesheuvel
  2017-08-22 17:38   ` Laszlo Ersek
  1 sibling, 0 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2017-08-22 17:16 UTC (permalink / raw)
  To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Laszlo Ersek, Jordan Justen

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.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel
  2017-08-22 17:02 ` Laszlo Ersek
@ 2017-08-22 17:16   ` Ard Biesheuvel
  2017-08-23 13:15     ` Leif Lindholm
  0 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2017-08-22 17:16 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: edk2-devel@lists.01.org, Leif Lindholm, Jordan Justen

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.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel
  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
  1 sibling, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2017-08-22 17:38 UTC (permalink / raw)
  To: Leif Lindholm, Ard Biesheuvel; +Cc: edk2-devel, jordan.l.justen, Alexander Graf

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel
  2017-08-22 17:38   ` Laszlo Ersek
@ 2017-08-22 19:05     ` Leif Lindholm
  2017-08-22 19:14       ` Ard Biesheuvel
  0 siblings, 1 reply; 15+ messages in thread
From: Leif Lindholm @ 2017-08-22 19:05 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: Ard Biesheuvel, edk2-devel, jordan.l.justen, Alexander Graf

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel
  2017-08-22 19:05     ` Leif Lindholm
@ 2017-08-22 19:14       ` Ard Biesheuvel
  0 siblings, 0 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2017-08-22 19:14 UTC (permalink / raw)
  To: Leif Lindholm
  Cc: Laszlo Ersek, edk2-devel@lists.01.org, Jordan Justen,
	Alexander Graf

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.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel
  2017-08-22 17:16   ` Ard Biesheuvel
@ 2017-08-23 13:15     ` Leif Lindholm
  2017-08-23 13:17       ` Ard Biesheuvel
  0 siblings, 1 reply; 15+ messages in thread
From: Leif Lindholm @ 2017-08-23 13:15 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Laszlo Ersek, edk2-devel@lists.01.org, Jordan Justen

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel
  2017-08-23 13:15     ` Leif Lindholm
@ 2017-08-23 13:17       ` Ard Biesheuvel
  2017-08-23 13:36         ` Laszlo Ersek
  0 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2017-08-23 13:17 UTC (permalink / raw)
  To: Leif Lindholm; +Cc: Laszlo Ersek, edk2-devel@lists.01.org, Jordan Justen

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel
  2017-08-23 13:17       ` Ard Biesheuvel
@ 2017-08-23 13:36         ` Laszlo Ersek
  2017-08-23 15:00           ` Leif Lindholm
  0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2017-08-23 13:36 UTC (permalink / raw)
  To: Ard Biesheuvel, Leif Lindholm; +Cc: edk2-devel@lists.01.org, Jordan Justen

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel
  2017-08-23 13:36         ` Laszlo Ersek
@ 2017-08-23 15:00           ` Leif Lindholm
  2017-08-24 12:02             ` Leif Lindholm
  0 siblings, 1 reply; 15+ messages in thread
From: Leif Lindholm @ 2017-08-23 15:00 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: Ard Biesheuvel, edk2-devel@lists.01.org, Jordan Justen

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel
  2017-08-23 15:00           ` Leif Lindholm
@ 2017-08-24 12:02             ` Leif Lindholm
  2017-08-24 12:25               ` Ard Biesheuvel
  0 siblings, 1 reply; 15+ messages in thread
From: Leif Lindholm @ 2017-08-24 12:02 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: Ard Biesheuvel, edk2-devel@lists.01.org, Jordan Justen

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>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] ArmVirtPkg: remove QemuVideoDxe from ArmVirtQemu and ArmVirtQemuKernel
  2017-08-24 12:02             ` Leif Lindholm
@ 2017-08-24 12:25               ` Ard Biesheuvel
  0 siblings, 0 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2017-08-24 12:25 UTC (permalink / raw)
  To: Leif Lindholm; +Cc: Laszlo Ersek, edk2-devel@lists.01.org, Jordan Justen

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.


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2017-08-24 12:22 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
2017-08-23 15:00           ` Leif Lindholm
2017-08-24 12:02             ` Leif Lindholm
2017-08-24 12:25               ` Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox