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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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