From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web10.662.1582668172511985960 for ; Tue, 25 Feb 2020 14:02:52 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=PAEv3uRE; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1582668171; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=+tJz7m9lmwFKwq9bsFpkz2+u89yNjHMnUUutA7Fgv2I=; b=PAEv3uRE6rGGsjhNHZE4jTjkw21h0s9Wi1oq1nvu8dNOyoMEjpYwO9NwCAIissmwEoQw7b +XQjFMoHh01osbAFr6xQEBFUNFJPGkcq3mRpwcXyByAiaMTZZJt/HVsdjtZfP3zIWfaKfZ bo/xVZS/TEG3vBR0Lz4rBqXvZ70LYWw= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-84-30rSPN2xP1qxvJZctjbyWg-1; Tue, 25 Feb 2020 17:02:45 -0500 X-MC-Unique: 30rSPN2xP1qxvJZctjbyWg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 764D3800D53; Tue, 25 Feb 2020 22:02:44 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-104.ams2.redhat.com [10.36.117.104]) by smtp.corp.redhat.com (Postfix) with ESMTP id 491911CB; Tue, 25 Feb 2020 22:02:39 +0000 (UTC) Subject: Re: [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support To: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , edk2-devel-groups-io Cc: Ard Biesheuvel , Gerd Hoffmann , Jordan Justen , Marc W Chen References: <20200224171741.7494-1-lersek@redhat.com> <8bd42e8f-20cd-64c9-eba0-deeee5d370a9@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 25 Feb 2020 23:02:39 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <8bd42e8f-20cd-64c9-eba0-deeee5d370a9@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 02/25/20 21:51, Philippe Mathieu-Daud=C3=A9 wrote: > Hi Laszlo, >=20 > 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 i= f >> the device wasn't actually VGA compatible. >> >> Restrict the IO Ports requirement to VGA compatible devices. >> >> Cc: Ard Biesheuvel >> Cc: Gerd Hoffmann >> Cc: Jordan Justen >> Cc: Marc W Chen >> Cc: Philippe Mathieu-Daud=C3=A9 >> Fixes: 662bd0da7fd77e4d2cf9ef4a78015af5cad7d9db >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2555 >> Signed-off-by: Laszlo Ersek >> --- >> >> Notes: >> =C2=A0=C2=A0=C2=A0=C2=A0 Repo:=C2=A0=C2=A0 https://github.com/lersek/edk= 2.git >> =C2=A0=C2=A0=C2=A0=C2=A0 Branch: vga_io_bz_2555 >> >> =C2=A0 OvmfPkg/QemuVideoDxe/Driver.c | 2 +- >> =C2=A0 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 ( >> =C2=A0=C2=A0=C2=A0 } >> =C2=A0 =C2=A0=C2=A0=C2=A0 SupportedVgaIo &=3D (UINT64)(EFI_PCI_IO_ATTRIB= UTE_VGA_IO | >> EFI_PCI_IO_ATTRIBUTE_VGA_IO_16); >> -=C2=A0 if (SupportedVgaIo =3D=3D 0) { >> +=C2=A0 if (SupportedVgaIo =3D=3D 0 && IS_PCI_VGA (&Pci)) { >=20 > 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] =3D=3D PCI_IF_VGA_VGA (interface) && - ClassCode[1] =3D=3D PCI_CLASS_DISPLAY_VGA (subclass ) && - ClassCode[2] =3D=3D PCI_CLASS_DISPLAY (class) While the IS_PCI_DISPLAY() macro compares: - ClassCode[2] =3D=3D 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 =3D=3D 0) { + Status =3D 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. >=20 > 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 >=20 >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Status =3D EFI_UNSUPPORTED; >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto ClosePciIo; >> =C2=A0=C2=A0=C2=A0 } >> >> base-commit: 1d3215fd24f47eaa4877542a59b4bbf5afc0cfe8 >> >=20