Bug 1806359

Not able to reproduce this issue on 8.1.1 environment with qemu-kvm-4.1.0-23.module+el8.1.1+5748+5fcc84a8.1.x86_64, seavgabios-bin-1.12.0-5.module+el8.1.1+5309+6d656f05.noarch and edk2-ovmf-20190308git89910a39dcfd-6.el8.noarch

Per latest description by Gerd:
https://www.kraxel.org/blog/2019/09/display-devices-in-qemu/

"For UEFI guests it is safe to use the bochs display device instead of the standard VGA device. The firmware will setup a linear framebuffer as GOP anyway and never use any legacy VGA features.", I think basic framebuffer should work with any VM even without driver? Not quite sure..

Set regression base on not reproducible on 8.1.1, please feel free to clear this keyword if this is a expected behavior..

> driver attach

In uefi shell I assume?

Does downgrading ovmf to 8.1.1 help?

(In reply to Gerd Hoffmann from comment #2)
> > driver attach
> 
> In uefi shell I assume?
> 
> Does downgrading ovmf to 8.1.1 help?

Yes, downgrade ovmf to edk2-ovmf-20190308git89910a39dcfd-6.el8.noarch(shipped with 8.1.1) will solve the issue

(In reply to Guo, Zhiyi from comment #3)
> (In reply to Gerd Hoffmann from comment #2)
> > > driver attach
> > 
> > In uefi shell I assume?
> > 
> > Does downgrading ovmf to 8.1.1 help?
> 
> Yes, downgrade ovmf to
> edk2-ovmf-20190308git89910a39dcfd-6.el8.noarch(shipped with 8.1.1) will
> solve the issue

So my first guess would be that ovmf/edk2 doesn't bind to bochs-display by default
because it is pci class display/other not display/vga ...

Laszlo?

Right, the term "driver attach" is quite confusing. From comment 0, it seems that it stands for "native guest OS driver".

Anyway, from comment 3 it seems like a regression in edk2.

Zhiyi, can you please capture OVMF debug logs with both packages:

- edk2-ovmf-20190308git89910a39dcfd-6.el8.noarch (working)
- edk2-ovmf-20190829git37eef91017ad-8.el8.noarch (broken)

(using otherwise identical domain XMLs, of course), and attach them? Thanks!

(In reply to Gerd Hoffmann from comment #4)
> (In reply to Guo, Zhiyi from comment #3)
> > (In reply to Gerd Hoffmann from comment #2)
> > > > driver attach
> > > 
> > > In uefi shell I assume?
> > > 
> > > Does downgrading ovmf to 8.1.1 help?
> > 
> > Yes, downgrade ovmf to
> > edk2-ovmf-20190308git89910a39dcfd-6.el8.noarch(shipped with 8.1.1) will
> > solve the issue
> 
> So my first guess would be that ovmf/edk2 doesn't bind to bochs-display by
> default because it is pci class display/other not display/vga ...

I've never personally tested <model type='bochs'/> (my RHEL7 libvirtd is 4.5-based, and the bochs model gained support in libvirt 5.6 first). I can't tell off-hand if upstream QemuVideoDxe had anything related to PCI class filtering...

$ git log --oneline --reverse  edk2-stable201903..edk2-stable201908 -- OvmfPkg/QemuVideoDxe/ | cat -n
     1  b26f0cf9ee09 OvmfPkg: Replace BSD License with BSD+Patent License
     2  52d229238b2d OvmfPkg/QemuVideoDxe: avoid arithmetic on null pointer
     3  cd5147734cbe OvmfPkg/QemuVideoDxe: Remove dependency on OptionRomPkg
     4  662bd0da7fd7 OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA alias mode.

Sigh, I think commit 662bd0da7fd7 caused the regression. :(

The message on commit 662bd0da7fd7 is nearly undecipherable, but the associated upstream BZ <https://bugzilla.tianocore.org/show_bug.cgi?id=1880> is more helpful. Basically the idea was to replace the hard-coded EFI_PCI_IO_ATTRIBUTE_VGA_IO in the "EfiPciIoAttributeOperationEnable" operation with EFI_PCI_IO_ATTRIBUTE_VGA_IO or EFI_PCI_IO_ATTRIBUTE_VGA_IO_16, dependent on what the device supported. Hence the new query (EfiPciIoAttributeOperationSupported operation).

However, the following hunk:

+  if (SupportedVgaIo == 0) {
+    Status = EFI_UNSUPPORTED;
+    goto ClosePciIo;
+  }

is bogus -- it prevents QemuVideoDxe from binding devices that have no VGA compat IO stuff at all.

Gerd, can you submit a patch to back out this hunk? I'd feel more comfortable if you were writing the commit message. Otherwise, I can submit the patch later (and then I'll ask you to review).

Thanks.
Laszlo

> However, the following hunk:
> 
> +  if (SupportedVgaIo == 0) {
> +    Status = EFI_UNSUPPORTED;
> +    goto ClosePciIo;
> +  }
> 
> is bogus -- it prevents QemuVideoDxe from binding devices that have no VGA
> compat IO stuff at all.
> 
> Gerd, can you submit a patch to back out this hunk? I'd feel more
> comfortable if you were writing the commit message. Otherwise, I can submit
> the patch later (and then I'll ask you to review).

Something like this should work (untested, build fails for me somewhere in openssl :( ).

diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
index 6a4a860b3c25..fde94b606f27 100644
--- a/OvmfPkg/QemuVideoDxe/Driver.c
+++ b/OvmfPkg/QemuVideoDxe/Driver.c
@@ -292,7 +292,7 @@ QemuVideoControllerDriverStart (
   }
 
   SupportedVgaIo &= (UINT64)(EFI_PCI_IO_ATTRIBUTE_VGA_IO | EFI_PCI_IO_ATTRIBUTE_VGA_IO_16);
-  if (SupportedVgaIo == 0) {
+  if (SupportedVgaIo == 0 && Pci.Hdr.ClassCode[1] == PCI_CLASS_DISPLAY_VGA) {^M
     Status = EFI_UNSUPPORTED;
     goto ClosePciIo;
   }

Commit message along the lines of "only require vga io ports if the device actually is vga compatible".
Cc: me if you want a review, I'm not on edk2-devel any more.

(In reply to Gerd Hoffmann from comment #10)
> > However, the following hunk:
> > 
> > +  if (SupportedVgaIo == 0) {
> > +    Status = EFI_UNSUPPORTED;
> > +    goto ClosePciIo;
> > +  }
> > 
> > is bogus -- it prevents QemuVideoDxe from binding devices that have no VGA
> > compat IO stuff at all.
> > 
> > Gerd, can you submit a patch to back out this hunk? I'd feel more
> > comfortable if you were writing the commit message. Otherwise, I can submit
> > the patch later (and then I'll ask you to review).
> 
> Something like this should work (untested, build fails for me somewhere in
> openssl :( ).
> 
> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
> index 6a4a860b3c25..fde94b606f27 100644
> --- a/OvmfPkg/QemuVideoDxe/Driver.c
> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
> @@ -292,7 +292,7 @@ QemuVideoControllerDriverStart (
>    }
>  
>    SupportedVgaIo &= (UINT64)(EFI_PCI_IO_ATTRIBUTE_VGA_IO |
> EFI_PCI_IO_ATTRIBUTE_VGA_IO_16);
> -  if (SupportedVgaIo == 0) {
> +  if (SupportedVgaIo == 0 && Pci.Hdr.ClassCode[1] == PCI_CLASS_DISPLAY_VGA)
> {^M
>      Status = EFI_UNSUPPORTED;
>      goto ClosePciIo;
>    }
> 
> Commit message along the lines of "only require vga io ports if the device
> actually is vga compatible".

Thanks, very helpful! This reminds me of commit 4fdb585c69d6 ("OvmfPkg/PlatformBootManagerLib: relax device class requirement for ConOut", 2016-09-01). I.e., I could use the IS_PCI_VGA() macro, perhaps.

I'll send a patch, hopefully soon.

Upstream patch posted:

[edk2-devel] [PATCH edk2-stable202002]
OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support

http://mid.mail-archive.com/20200224171741.7494-1-lersek@redhat.com
https://edk2.groups.io/g/devel/message/54760

(In reply to Laszlo Ersek from comment #12)
> Upstream patch posted:
> 
> [edk2-devel] [PATCH edk2-stable202002]
> OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support
> 
> http://mid.mail-archive.com/20200224171741.7494-1-lersek@redhat.com
> https://edk2.groups.io/g/devel/message/54760

Upstream commit edfe16a6d9f8.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2020:1712