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

["Laszlo Ersek" <lersek@redhat.com>]

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