* [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support
@ 2020-02-24 17:17 Laszlo Ersek
2020-02-24 17:21 ` Ard Biesheuvel
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Laszlo Ersek @ 2020-02-24 17:17 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Gerd Hoffmann, Jordan Justen, Marc W Chen,
Philippe Mathieu-Daudé
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support
2020-02-24 17:17 [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support Laszlo Ersek
@ 2020-02-24 17:21 ` Ard Biesheuvel
2020-02-25 13:20 ` [edk2-devel] " Liming Gao
2020-02-25 6:26 ` Gerd Hoffmann
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Ard Biesheuvel @ 2020-02-24 17:21 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel-groups-io, Gerd Hoffmann, Jordan Justen, Marc W Chen,
Philippe Mathieu-Daudé
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support
2020-02-24 17:17 [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support Laszlo Ersek
2020-02-24 17:21 ` Ard Biesheuvel
@ 2020-02-25 6:26 ` Gerd Hoffmann
2020-02-25 20:51 ` Philippe Mathieu-Daudé
2020-02-26 16:51 ` [edk2-devel] " Laszlo Ersek
3 siblings, 0 replies; 9+ messages in thread
From: Gerd Hoffmann @ 2020-02-25 6:26 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel-groups-io, Ard Biesheuvel, Jordan Justen, Marc W Chen,
Philippe Mathieu-Daudé
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support
2020-02-24 17:21 ` Ard Biesheuvel
@ 2020-02-25 13:20 ` Liming Gao
2020-02-25 22:12 ` Laszlo Ersek
0 siblings, 1 reply; 9+ messages in thread
From: Liming Gao @ 2020-02-25 13:20 UTC (permalink / raw)
To: devel@edk2.groups.io, ard.biesheuvel@linaro.org, Laszlo Ersek
Cc: Gerd Hoffmann, Justen, Jordan L, Chen, Marc W,
Philippe Mathieu-Daudé
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
> >
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support
2020-02-24 17:17 [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support Laszlo Ersek
2020-02-24 17:21 ` Ard Biesheuvel
2020-02-25 6:26 ` Gerd Hoffmann
@ 2020-02-25 20:51 ` Philippe Mathieu-Daudé
2020-02-25 22:02 ` Laszlo Ersek
2020-02-26 16:51 ` [edk2-devel] " Laszlo Ersek
3 siblings, 1 reply; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-25 20:51 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io
Cc: Ard Biesheuvel, Gerd Hoffmann, Jordan Justen, Marc W Chen
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support
2020-02-25 20:51 ` Philippe Mathieu-Daudé
@ 2020-02-25 22:02 ` Laszlo Ersek
2020-02-26 14:23 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2020-02-25 22:02 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, edk2-devel-groups-io
Cc: Ard Biesheuvel, Gerd Hoffmann, Jordan Justen, Marc W Chen
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
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support
2020-02-25 13:20 ` [edk2-devel] " Liming Gao
@ 2020-02-25 22:12 ` Laszlo Ersek
0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2020-02-25 22:12 UTC (permalink / raw)
To: devel, liming.gao, ard.biesheuvel@linaro.org
Cc: Gerd Hoffmann, Justen, Jordan L, Chen, Marc W,
Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support
2020-02-25 22:02 ` Laszlo Ersek
@ 2020-02-26 14:23 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-26 14:23 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io
Cc: Ard Biesheuvel, Gerd Hoffmann, Jordan Justen, Marc W Chen
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
>>>
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support
2020-02-24 17:17 [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support Laszlo Ersek
` (2 preceding siblings ...)
2020-02-25 20:51 ` Philippe Mathieu-Daudé
@ 2020-02-26 16:51 ` Laszlo Ersek
3 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2020-02-26 16:51 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Ard Biesheuvel, Gerd Hoffmann, Jordan Justen, Marc W Chen,
Philippe Mathieu-Daudé, Liming Gao
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-02-26 16:51 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-24 17:17 [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support Laszlo Ersek
2020-02-24 17:21 ` Ard Biesheuvel
2020-02-25 13:20 ` [edk2-devel] " Liming Gao
2020-02-25 22:12 ` Laszlo Ersek
2020-02-25 6:26 ` Gerd Hoffmann
2020-02-25 20:51 ` Philippe Mathieu-Daudé
2020-02-25 22:02 ` Laszlo Ersek
2020-02-26 14:23 ` Philippe Mathieu-Daudé
2020-02-26 16:51 ` [edk2-devel] " Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox