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

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

[Laszlo Ersek]

In edk2 commit 333f32ec23dd, QemuVideoDxe gained support for QEMU's
"secondary-vga" device model (originally introduced in QEMU commit
63e3e24db2e9).

In QEMU commit 765c94290863, the "bochs-display" device was introduced,
which would work with QemuVideoDxe out of the box, reusing the
"secondary-vga" logic.

Support for both models has been broken since edk2 commit 662bd0da7fd7.
Said patch ended up requiring VGA IO Ports -- i.e., at least one of
EFI_PCI_IO_ATTRIBUTE_VGA_IO and EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 -- even if
the device wasn't actually VGA compatible.

Restrict the IO Ports requirement to VGA compatible devices.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Marc W Chen <marc.w.chen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Fixes: 662bd0da7fd77e4d2cf9ef4a78015af5cad7d9db
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2555
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    Repo:   https://github.com/lersek/edk2.git
    Branch: vga_io_bz_2555

 OvmfPkg/QemuVideoDxe/Driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
index 6a4a860b3c25..37bbbbe843c9 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 && IS_PCI_VGA (&Pci)) {
     Status = EFI_UNSUPPORTED;
     goto ClosePciIo;
   }

base-commit: 1d3215fd24f47eaa4877542a59b4bbf5afc0cfe8
-- 
2.19.1.3.g30247aa5d201

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

[Ard Biesheuvel]

On Mon, 24 Feb 2020 at 18:17, Laszlo Ersek <lersek@redhat.com> wrote:
>
> In edk2 commit 333f32ec23dd, QemuVideoDxe gained support for QEMU's
> "secondary-vga" device model (originally introduced in QEMU commit
> 63e3e24db2e9).
>
> In QEMU commit 765c94290863, the "bochs-display" device was introduced,
> which would work with QemuVideoDxe out of the box, reusing the
> "secondary-vga" logic.
>
> Support for both models has been broken since edk2 commit 662bd0da7fd7.
> Said patch ended up requiring VGA IO Ports -- i.e., at least one of
> EFI_PCI_IO_ATTRIBUTE_VGA_IO and EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 -- even if
> the device wasn't actually VGA compatible.
>
> Restrict the IO Ports requirement to VGA compatible devices.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Marc W Chen <marc.w.chen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Fixes: 662bd0da7fd77e4d2cf9ef4a78015af5cad7d9db
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2555
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>
> Notes:
>     Repo:   https://github.com/lersek/edk2.git
>     Branch: vga_io_bz_2555
>
>  OvmfPkg/QemuVideoDxe/Driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
> index 6a4a860b3c25..37bbbbe843c9 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 && IS_PCI_VGA (&Pci)) {
>      Status = EFI_UNSUPPORTED;
>      goto ClosePciIo;
>    }
>
> base-commit: 1d3215fd24f47eaa4877542a59b4bbf5afc0cfe8
> --
> 2.19.1.3.g30247aa5d201
>

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

[Liming Gao]

Laszlo:
  Seemly, this is a regression issue. I am ok to catch it into stable tag edk2-stable202002. 

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> Sent: Tuesday, February 25, 2020 1:21 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Gerd Hoffmann <kraxel@redhat.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Chen, Marc W <marc.w.chen@intel.com>; Philippe Mathieu-Daudé <philmd@redhat.com>
> Subject: Re: [edk2-devel] [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support
> 
> On Mon, 24 Feb 2020 at 18:17, Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > In edk2 commit 333f32ec23dd, QemuVideoDxe gained support for QEMU's
> > "secondary-vga" device model (originally introduced in QEMU commit
> > 63e3e24db2e9).
> >
> > In QEMU commit 765c94290863, the "bochs-display" device was introduced,
> > which would work with QemuVideoDxe out of the box, reusing the
> > "secondary-vga" logic.
> >
> > Support for both models has been broken since edk2 commit 662bd0da7fd7.
> > Said patch ended up requiring VGA IO Ports -- i.e., at least one of
> > EFI_PCI_IO_ATTRIBUTE_VGA_IO and EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 -- even if
> > the device wasn't actually VGA compatible.
> >
> > Restrict the IO Ports requirement to VGA compatible devices.
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Marc W Chen <marc.w.chen@intel.com>
> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Fixes: 662bd0da7fd77e4d2cf9ef4a78015af5cad7d9db
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2555
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> > ---
> >
> > Notes:
> >     Repo:   https://github.com/lersek/edk2.git
> >     Branch: vga_io_bz_2555
> >
> >  OvmfPkg/QemuVideoDxe/Driver.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
> > index 6a4a860b3c25..37bbbbe843c9 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 && IS_PCI_VGA (&Pci)) {
> >      Status = EFI_UNSUPPORTED;
> >      goto ClosePciIo;
> >    }
> >
> > base-commit: 1d3215fd24f47eaa4877542a59b4bbf5afc0cfe8
> > --
> > 2.19.1.3.g30247aa5d201
> >
> 
> 

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

[Laszlo Ersek]

On 02/25/20 14:20, Liming Gao wrote:
> Laszlo:
>   Seemly, this is a regression issue. I am ok to catch it into stable tag edk2-stable202002. 

Thank you. If Phil is satisfied with my explanation, I'll push the patch
tomorrow.

Laszlo

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

[Gerd Hoffmann]

On Mon, Feb 24, 2020 at 06:17:41PM +0100, Laszlo Ersek wrote:
> In edk2 commit 333f32ec23dd, QemuVideoDxe gained support for QEMU's
> "secondary-vga" device model (originally introduced in QEMU commit
> 63e3e24db2e9).
> 
> In QEMU commit 765c94290863, the "bochs-display" device was introduced,
> which would work with QemuVideoDxe out of the box, reusing the
> "secondary-vga" logic.
> 
> Support for both models has been broken since edk2 commit 662bd0da7fd7.
> Said patch ended up requiring VGA IO Ports -- i.e., at least one of
> EFI_PCI_IO_ATTRIBUTE_VGA_IO and EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 -- even if
> the device wasn't actually VGA compatible.
> 
> Restrict the IO Ports requirement to VGA compatible devices.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Marc W Chen <marc.w.chen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Fixes: 662bd0da7fd77e4d2cf9ef4a78015af5cad7d9db
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2555
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     Repo:   https://github.com/lersek/edk2.git
>     Branch: vga_io_bz_2555
> 
>  OvmfPkg/QemuVideoDxe/Driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
> index 6a4a860b3c25..37bbbbe843c9 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 && IS_PCI_VGA (&Pci)) {
>      Status = EFI_UNSUPPORTED;
>      goto ClosePciIo;
>    }

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

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

[Philippe Mathieu-Daudé]

Hi Laszlo,

On 2/24/20 6:17 PM, Laszlo Ersek wrote:
> In edk2 commit 333f32ec23dd, QemuVideoDxe gained support for QEMU's
> "secondary-vga" device model (originally introduced in QEMU commit
> 63e3e24db2e9).
> 
> In QEMU commit 765c94290863, the "bochs-display" device was introduced,
> which would work with QemuVideoDxe out of the box, reusing the
> "secondary-vga" logic.
> 
> Support for both models has been broken since edk2 commit 662bd0da7fd7.
> Said patch ended up requiring VGA IO Ports -- i.e., at least one of
> EFI_PCI_IO_ATTRIBUTE_VGA_IO and EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 -- even if
> the device wasn't actually VGA compatible.
> 
> Restrict the IO Ports requirement to VGA compatible devices.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Marc W Chen <marc.w.chen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Fixes: 662bd0da7fd77e4d2cf9ef4a78015af5cad7d9db
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2555
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>      Repo:   https://github.com/lersek/edk2.git
>      Branch: vga_io_bz_2555
> 
>   OvmfPkg/QemuVideoDxe/Driver.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
> index 6a4a860b3c25..37bbbbe843c9 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 && IS_PCI_VGA (&Pci)) {

I'm having hard time to understand. Before we could have a video PCI 
controller which was not VGA?

What about the other IS_PCI_OLD_VGA() macro?

>       Status = EFI_UNSUPPORTED;
>       goto ClosePciIo;
>     }
> 
> base-commit: 1d3215fd24f47eaa4877542a59b4bbf5afc0cfe8
> 

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

[Laszlo Ersek]

On 02/25/20 21:51, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
> 
> On 2/24/20 6:17 PM, Laszlo Ersek wrote:
>> In edk2 commit 333f32ec23dd, QemuVideoDxe gained support for QEMU's
>> "secondary-vga" device model (originally introduced in QEMU commit
>> 63e3e24db2e9).
>>
>> In QEMU commit 765c94290863, the "bochs-display" device was introduced,
>> which would work with QemuVideoDxe out of the box, reusing the
>> "secondary-vga" logic.
>>
>> Support for both models has been broken since edk2 commit 662bd0da7fd7.
>> Said patch ended up requiring VGA IO Ports -- i.e., at least one of
>> EFI_PCI_IO_ATTRIBUTE_VGA_IO and EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 -- even if
>> the device wasn't actually VGA compatible.
>>
>> Restrict the IO Ports requirement to VGA compatible devices.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Marc W Chen <marc.w.chen@intel.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Fixes: 662bd0da7fd77e4d2cf9ef4a78015af5cad7d9db
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2555
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>      Repo:   https://github.com/lersek/edk2.git
>>      Branch: vga_io_bz_2555
>>
>>   OvmfPkg/QemuVideoDxe/Driver.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c
>> b/OvmfPkg/QemuVideoDxe/Driver.c
>> index 6a4a860b3c25..37bbbbe843c9 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 && IS_PCI_VGA (&Pci)) {
> 
> I'm having hard time to understand. Before we could have a video PCI
> controller which was not VGA?

Yes, exactly.

PCI devices have a particular field in their config spaces, which is
described as follows, in "MdePkg/Include/IndustryStandard/Pci22.h":


///
/// Common header region in PCI Configuration Space
/// Section 6.1, PCI Local Bus Specification, 2.2
///
typedef struct {
  UINT16  VendorId;
  UINT16  DeviceId;
  UINT16  Command;
  UINT16  Status;
  UINT8   RevisionID;
  UINT8   ClassCode[3];  <---------- this one
  UINT8   CacheLineSize;
  UINT8   LatencyTimer;
  UINT8   HeaderType;
  UINT8   BIST;
} PCI_DEVICE_INDEPENDENT_REGION;


In the same header file, the IS_PCI_VGA() macro compares:
- ClassCode[0] == PCI_IF_VGA_VGA        (interface) &&
- ClassCode[1] == PCI_CLASS_DISPLAY_VGA (subclass ) &&
- ClassCode[2] == PCI_CLASS_DISPLAY     (class)

While the IS_PCI_DISPLAY() macro compares:
- ClassCode[2] == PCI_CLASS_DISPLAY     (class)

So IS_PCI_DISPLAY() is more generic, IS_PCI_VGA() is more specific.

QEMU provides a number of video devices that satisfy IS_PCI_DISPLAY(),
but not IS_PCI_VGA(). Examples are:

- ramfb (driven by RamfbDxe)

- virtio-gpu-pci (driver by VirtioGpuDxe)

- secondary-vga and bochs-display (driven by QemuVideoDxe, and now broken)


It might help if you review the following commits:

[1] 4fdb585c69d6 ("OvmfPkg/PlatformBootManagerLib: relax device class
requirement for ConOut", 2016-09-01)

[2] 70dbd16361ee ("OvmfPkg/QemuVideoDxe: Add SubClass field to
QEMU_VIDEO_CARD", 2018-05-17)

[3] 333f32ec23dd ("OvmfPkg/QemuVideoDxe: Enable DISPLAY_OTHER pci class
for qemu stdvga", 2018-05-17)

[4] 662bd0da7fd7 ("OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA
alias mode.", 2019-06-06) -- i.e., the regression.

Commit [1] is not extremely relevant here, it just demonstrates the
usage of the IS_PCI_VGA() vs. IS_PCI_DISPLAY() macros. Relaxing the
condition in "OvmfPkg/PlatformBootManagerLib" from the former to the
latter meant that OVMF would automatically pick up virtio-gpu-pci
devices in ConOut. (The practical consequence is that the UEFI console,
such as the UEFI shell, grub, etc, are multiplexed to virtio-gpu-pci
devices too.)

In commits [2] and [3], Gerd extended QemuVideoDxe to handle
"secondary-vga". Because, "secondary-vga" has PCI_CLASS_DISPLAY_OTHER
(not PCI_CLASS_DISPLAY_VGA) for ClassCode[1] (i.e., subclass). So it
won't satisfy IS_PCI_VGA(). For ClassCode[2] -- i.e., class --
"secondary-vga" still has PCI_CLASS_DISPLAY, so it will satisfy
IS_PCI_DISPLAY().

Some time later, Gerd added the "bochs-display" device to QEMU, which
would exercise the exact same code path in QemuVideoDxe as "secondary-vga".


And, finally, while reviewing commit [4], I missed that it changed two
things at once, one intentionally, and another one unintentionally.

The intentional change was that rather than hardcoding
EFI_PCI_IO_ATTRIBUTE_VGA_IO in the "EfiPciIoAttributeOperationEnable"
operation, we would first ask the device about
(EFI_PCI_IO_ATTRIBUTE_VGA_IO | EFI_PCI_IO_ATTRIBUTE_VGA_IO_16), with
EfiPciIoAttributeOperationSupported, and then pass whichever was
available to EfiPciIoAttributeOperationEnable. This change was good.

However, the following hunk:

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

was wrong. Because, for such devices that satisfy IS_PCI_DISPLAY(), but
not IS_PCI_VGA() -- namely: "secondary-vga" and "bochs-display" --
*neither* of EFI_PCI_IO_ATTRIBUTE_VGA_IO and
EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 can be expected. So when we bail out with
EFI_UNSUPPORTED if both attributes are missing (as reported by the
device), we deny driving "secondary-vga" and "bochs-display", even
though they used to work just fine.

The present patch restricts the EFI_UNSUPPORTED branch to such devices
(i.e., to PCI display devices with actual VGA cruft) where at least one
of those attributes would be rightfully required.

> 
> What about the other IS_PCI_OLD_VGA() macro?

It seems irrelevant here -- the class, subclass and interface codes that
we need to check for in OVMF are dictated by the QEMU device models, in
the end. So unless a QEMU device advertizes PCI_CLASS_OLD for "class",
and PCI_CLASS_OLD_VGA for "subclass", the macro is irrelevant to OVMF.

Thanks!
Laszlo

> 
>>       Status = EFI_UNSUPPORTED;
>>       goto ClosePciIo;
>>     }
>>
>> base-commit: 1d3215fd24f47eaa4877542a59b4bbf5afc0cfe8
>>
> 

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

[Philippe Mathieu-Daudé]

On 2/25/20 11:02 PM, Laszlo Ersek wrote:
> On 02/25/20 21:51, Philippe Mathieu-Daudé wrote:
>> Hi Laszlo,
>>
>> On 2/24/20 6:17 PM, Laszlo Ersek wrote:
>>> In edk2 commit 333f32ec23dd, QemuVideoDxe gained support for QEMU's
>>> "secondary-vga" device model (originally introduced in QEMU commit
>>> 63e3e24db2e9).
>>>
>>> In QEMU commit 765c94290863, the "bochs-display" device was introduced,
>>> which would work with QemuVideoDxe out of the box, reusing the
>>> "secondary-vga" logic.
>>>
>>> Support for both models has been broken since edk2 commit 662bd0da7fd7.
>>> Said patch ended up requiring VGA IO Ports -- i.e., at least one of
>>> EFI_PCI_IO_ATTRIBUTE_VGA_IO and EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 -- even if
>>> the device wasn't actually VGA compatible.
>>>
>>> Restrict the IO Ports requirement to VGA compatible devices.
>>>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Marc W Chen <marc.w.chen@intel.com>
>>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> Fixes: 662bd0da7fd77e4d2cf9ef4a78015af5cad7d9db
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2555
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>
>>> Notes:
>>>       Repo:   https://github.com/lersek/edk2.git
>>>       Branch: vga_io_bz_2555
>>>
>>>    OvmfPkg/QemuVideoDxe/Driver.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c
>>> b/OvmfPkg/QemuVideoDxe/Driver.c
>>> index 6a4a860b3c25..37bbbbe843c9 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 && IS_PCI_VGA (&Pci)) {
>>
>> I'm having hard time to understand. Before we could have a video PCI
>> controller which was not VGA?
> 
> Yes, exactly.
> 
> PCI devices have a particular field in their config spaces, which is
> described as follows, in "MdePkg/Include/IndustryStandard/Pci22.h":
> 
> 
> ///
> /// Common header region in PCI Configuration Space
> /// Section 6.1, PCI Local Bus Specification, 2.2
> ///
> typedef struct {
>    UINT16  VendorId;
>    UINT16  DeviceId;
>    UINT16  Command;
>    UINT16  Status;
>    UINT8   RevisionID;
>    UINT8   ClassCode[3];  <---------- this one
>    UINT8   CacheLineSize;
>    UINT8   LatencyTimer;
>    UINT8   HeaderType;
>    UINT8   BIST;
> } PCI_DEVICE_INDEPENDENT_REGION;
> 
> 
> In the same header file, the IS_PCI_VGA() macro compares:
> - ClassCode[0] == PCI_IF_VGA_VGA        (interface) &&
> - ClassCode[1] == PCI_CLASS_DISPLAY_VGA (subclass ) &&
> - ClassCode[2] == PCI_CLASS_DISPLAY     (class)
> 
> While the IS_PCI_DISPLAY() macro compares:
> - ClassCode[2] == PCI_CLASS_DISPLAY     (class)
> 
> So IS_PCI_DISPLAY() is more generic, IS_PCI_VGA() is more specific.

OK.

> 
> QEMU provides a number of video devices that satisfy IS_PCI_DISPLAY(),
> but not IS_PCI_VGA(). Examples are:
> 
> - ramfb (driven by RamfbDxe)
> 
> - virtio-gpu-pci (driver by VirtioGpuDxe)
> 
> - secondary-vga and bochs-display (driven by QemuVideoDxe, and now broken)

I think I had some misunderstanding here.

> 
> 
> It might help if you review the following commits:
> 
> [1] 4fdb585c69d6 ("OvmfPkg/PlatformBootManagerLib: relax device class
> requirement for ConOut", 2016-09-01)
> 
> [2] 70dbd16361ee ("OvmfPkg/QemuVideoDxe: Add SubClass field to
> QEMU_VIDEO_CARD", 2018-05-17)
> 
> [3] 333f32ec23dd ("OvmfPkg/QemuVideoDxe: Enable DISPLAY_OTHER pci class
> for qemu stdvga", 2018-05-17)
> 
> [4] 662bd0da7fd7 ("OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA
> alias mode.", 2019-06-06) -- i.e., the regression.
> 
> Commit [1] is not extremely relevant here, it just demonstrates the
> usage of the IS_PCI_VGA() vs. IS_PCI_DISPLAY() macros. Relaxing the
> condition in "OvmfPkg/PlatformBootManagerLib" from the former to the
> latter meant that OVMF would automatically pick up virtio-gpu-pci
> devices in ConOut. (The practical consequence is that the UEFI console,
> such as the UEFI shell, grub, etc, are multiplexed to virtio-gpu-pci
> devices too.)
> 
> In commits [2] and [3], Gerd extended QemuVideoDxe to handle
> "secondary-vga". Because, "secondary-vga" has PCI_CLASS_DISPLAY_OTHER
> (not PCI_CLASS_DISPLAY_VGA) for ClassCode[1] (i.e., subclass). So it
> won't satisfy IS_PCI_VGA(). For ClassCode[2] -- i.e., class --
> "secondary-vga" still has PCI_CLASS_DISPLAY, so it will satisfy
> IS_PCI_DISPLAY().
> 
> Some time later, Gerd added the "bochs-display" device to QEMU, which
> would exercise the exact same code path in QemuVideoDxe as "secondary-vga".
> 
> 
> And, finally, while reviewing commit [4], I missed that it changed two
> things at once, one intentionally, and another one unintentionally.
> 
> The intentional change was that rather than hardcoding
> EFI_PCI_IO_ATTRIBUTE_VGA_IO in the "EfiPciIoAttributeOperationEnable"
> operation, we would first ask the device about
> (EFI_PCI_IO_ATTRIBUTE_VGA_IO | EFI_PCI_IO_ATTRIBUTE_VGA_IO_16), with
> EfiPciIoAttributeOperationSupported, and then pass whichever was
> available to EfiPciIoAttributeOperationEnable. This change was good.
> 
> However, the following hunk:
> 
> +  if (SupportedVgaIo == 0) {
> +    Status = EFI_UNSUPPORTED;
> +    goto ClosePciIo;
> +  }
> 
> was wrong. Because, for such devices that satisfy IS_PCI_DISPLAY(), but
> not IS_PCI_VGA() -- namely: "secondary-vga" and "bochs-display" --
> *neither* of EFI_PCI_IO_ATTRIBUTE_VGA_IO and
> EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 can be expected. So when we bail out with
> EFI_UNSUPPORTED if both attributes are missing (as reported by the
> device), we deny driving "secondary-vga" and "bochs-display", even
> though they used to work just fine.
> 
> The present patch restricts the EFI_UNSUPPORTED branch to such devices
> (i.e., to PCI display devices with actual VGA cruft) where at least one
> of those attributes would be rightfully required.

Thanks a lot for the full explanation.
Hopefully it will help others too :)

Now that I understand your patch:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

> 
>>
>> What about the other IS_PCI_OLD_VGA() macro?
> 
> It seems irrelevant here -- the class, subclass and interface codes that
> we need to check for in OVMF are dictated by the QEMU device models, in
> the end. So unless a QEMU device advertizes PCI_CLASS_OLD for "class",
> and PCI_CLASS_OLD_VGA for "subclass", the macro is irrelevant to OVMF.
> 
> Thanks!
> Laszlo
> 
>>
>>>        Status = EFI_UNSUPPORTED;
>>>        goto ClosePciIo;
>>>      }
>>>
>>> base-commit: 1d3215fd24f47eaa4877542a59b4bbf5afc0cfe8
>>>
>>
> 

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

[Laszlo Ersek]

On 02/24/20 18:17, Laszlo Ersek wrote:
> In edk2 commit 333f32ec23dd, QemuVideoDxe gained support for QEMU's
> "secondary-vga" device model (originally introduced in QEMU commit
> 63e3e24db2e9).
> 
> In QEMU commit 765c94290863, the "bochs-display" device was introduced,
> which would work with QemuVideoDxe out of the box, reusing the
> "secondary-vga" logic.
> 
> Support for both models has been broken since edk2 commit 662bd0da7fd7.
> Said patch ended up requiring VGA IO Ports -- i.e., at least one of
> EFI_PCI_IO_ATTRIBUTE_VGA_IO and EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 -- even if
> the device wasn't actually VGA compatible.
> 
> Restrict the IO Ports requirement to VGA compatible devices.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Marc W Chen <marc.w.chen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Fixes: 662bd0da7fd77e4d2cf9ef4a78015af5cad7d9db
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2555
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     Repo:   https://github.com/lersek/edk2.git
>     Branch: vga_io_bz_2555
> 
>  OvmfPkg/QemuVideoDxe/Driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
> index 6a4a860b3c25..37bbbbe843c9 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 && IS_PCI_VGA (&Pci)) {
>      Status = EFI_UNSUPPORTED;
>      goto ClosePciIo;
>    }
> 
> base-commit: 1d3215fd24f47eaa4877542a59b4bbf5afc0cfe8
> 

Thanks everyone for the feedback; pushed as commit edfe16a6d9f8 via
<https://github.com/tianocore/edk2/pull/404>.

Laszlo