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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54760): https://edk2.groups.io/g/devel/message/54760
Mute This Topic: https://groups.io/mt/71515873/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54761): https://edk2.groups.io/g/devel/message/54761 Mute This Topic: https://groups.io/mt/71515873/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54787): https://edk2.groups.io/g/devel/message/54787 Mute This Topic: https://groups.io/mt/71515873/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54803): https://edk2.groups.io/g/devel/message/54803 Mute This Topic: https://groups.io/mt/71515873/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54790): https://edk2.groups.io/g/devel/message/54790 Mute This Topic: https://groups.io/mt/71515873/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54800): https://edk2.groups.io/g/devel/message/54800 Mute This Topic: https://groups.io/mt/71515873/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 >> > -=-=-=-=-=-=-=-=-=-=-=- 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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 >>> >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54886): https://edk2.groups.io/g/devel/message/54886 Mute This Topic: https://groups.io/mt/71515873/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54903): https://edk2.groups.io/g/devel/message/54903 Mute This Topic: https://groups.io/mt/71515873/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.