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.biesheu...@linaro.org>
>> Cc: Gerd Hoffmann <kra...@redhat.com>
>> Cc: Jordan Justen <jordan.l.jus...@intel.com>
>> Cc: Marc W Chen <marc.w.c...@intel.com>
>> Cc: Philippe Mathieu-Daudé <phi...@redhat.com>
>> Fixes: 662bd0da7fd77e4d2cf9ef4a78015af5cad7d9db
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2555
>> Signed-off-by: Laszlo Ersek <ler...@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
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54802): https://edk2.groups.io/g/devel/message/54802
Mute This Topic: https://groups.io/mt/71515873/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
