1. Patchew
  2. EDK2
  3. View series

[edk2-devel] [PATCH v2] OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA alias mode.

Marc W Chen posted 1 patch 4 years, 10 months ago
Failed in applying to current master (apply log)
OvmfPkg/QemuVideoDxe/Driver.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
[edk2-devel] [PATCH v2] OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA alias mode.
Posted by Marc W Chen 4 years, 10 months ago 
Query the supported attributes firstly, then bitwise AND (&) both VGA_IO
and VGA_IO_16. Since the supported attributes should only have one of
VGA_IO or VGA_IO_16 set, the result of bitwise AND (&) is either VGA_IO
or IO_16. Then the result can be passed to PciIo->Attributes() to set the
attributes.

Device driver should consider both since the mReserveVgaAliases in
PciBusDxe driver is default FALSE(implies that device driver can only set
VGA_IO_16 to PCI_ROOT_BRIDGE), and Platform code may not return
EFI_RESERVE_VGA_IO_ALIAS in GetPlatformPolicy of PciPlatformProtocol to
make mReserveVgaAliases become TRUE(implies that device driver can only
set VGA_IO to PCI_ROOT_BRIDGE), Currently OvmfPkg doesn't have problem
due to it has hard code value for PCI_ROOT_BRIDGE's attributes field, so
an IO access by PciIoProtocol will be successed due to
RootBridgeIoCheckParameter of PciRootBridgeIo.c will always get pass
result for legacy IO access.

Usually the attributes field of PCI_ROOT_BRIDGE should be 0, in that case
it will have issue since the VGA_IO may not be able to be enabled, then
IO access by PciIoProtocol will be failed, hence the QemuVideoDxe driver
will not work fine.

Signed-off-by: Marc Chen <marc.w.chen@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Stefan Berger <stefanb@linux.ibm.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1880
---
 OvmfPkg/QemuVideoDxe/Driver.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
index e8a613ef33..522110ef4e 100644
--- a/OvmfPkg/QemuVideoDxe/Driver.c
+++ b/OvmfPkg/QemuVideoDxe/Driver.c
@@ -201,6 +201,7 @@ QemuVideoControllerDriverStart (
   PCI_TYPE00                        Pci;
   QEMU_VIDEO_CARD                   *Card;
   EFI_PCI_IO_PROTOCOL               *ChildPciIo;
+  UINT64                            SupportedVgaIo;
 
   OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
 
@@ -277,13 +278,32 @@ QemuVideoControllerDriverStart (
     goto ClosePciIo;
   }
 
+  //
+  // Get supported PCI attributes
+  //
+  Status = Private->PciIo->Attributes (
+                             Private->PciIo,
+                             EfiPciIoAttributeOperationSupported,
+                             0,
+                             &SupportedVgaIo
+                             );
+  if (EFI_ERROR (Status)) {
+    goto ClosePciIo;
+  }
+
+  SupportedVgaIo &= (UINT64)(EFI_PCI_IO_ATTRIBUTE_VGA_IO | EFI_PCI_IO_ATTRIBUTE_VGA_IO_16);
+  if (SupportedVgaIo == 0) {
+    Status = EFI_UNSUPPORTED;
+    goto ClosePciIo;
+  }
+
   //
   // Set new PCI attributes
   //
   Status = Private->PciIo->Attributes (
                             Private->PciIo,
                             EfiPciIoAttributeOperationEnable,
-                            EFI_PCI_DEVICE_ENABLE | EFI_PCI_IO_ATTRIBUTE_VGA_MEMORY | EFI_PCI_IO_ATTRIBUTE_VGA_IO,
+                            EFI_PCI_DEVICE_ENABLE | EFI_PCI_IO_ATTRIBUTE_VGA_MEMORY | SupportedVgaIo,
                             NULL
                             );
   if (EFI_ERROR (Status)) {
-- 
2.16.2.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#41984): https://edk2.groups.io/g/devel/message/41984
Mute This Topic: https://groups.io/mt/31948697/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2] OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA alias mode.
Posted by Laszlo Ersek 4 years, 10 months ago 
On 06/06/19 09:42, Marc W Chen wrote:
> Query the supported attributes firstly, then bitwise AND (&) both VGA_IO
> and VGA_IO_16. Since the supported attributes should only have one of
> VGA_IO or VGA_IO_16 set, the result of bitwise AND (&) is either VGA_IO
> or IO_16. Then the result can be passed to PciIo->Attributes() to set the
> attributes.
> 
> Device driver should consider both since the mReserveVgaAliases in
> PciBusDxe driver is default FALSE(implies that device driver can only set
> VGA_IO_16 to PCI_ROOT_BRIDGE), and Platform code may not return
> EFI_RESERVE_VGA_IO_ALIAS in GetPlatformPolicy of PciPlatformProtocol to
> make mReserveVgaAliases become TRUE(implies that device driver can only
> set VGA_IO to PCI_ROOT_BRIDGE), Currently OvmfPkg doesn't have problem
> due to it has hard code value for PCI_ROOT_BRIDGE's attributes field, so
> an IO access by PciIoProtocol will be successed due to
> RootBridgeIoCheckParameter of PciRootBridgeIo.c will always get pass
> result for legacy IO access.
> 
> Usually the attributes field of PCI_ROOT_BRIDGE should be 0, in that case
> it will have issue since the VGA_IO may not be able to be enabled, then
> IO access by PciIoProtocol will be failed, hence the QemuVideoDxe driver
> will not work fine.
> 
> Signed-off-by: Marc Chen <marc.w.chen@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Stefan Berger <stefanb@linux.ibm.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1880
> ---
>  OvmfPkg/QemuVideoDxe/Driver.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
> index e8a613ef33..522110ef4e 100644
> --- a/OvmfPkg/QemuVideoDxe/Driver.c
> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
> @@ -201,6 +201,7 @@ QemuVideoControllerDriverStart (
>    PCI_TYPE00                        Pci;
>    QEMU_VIDEO_CARD                   *Card;
>    EFI_PCI_IO_PROTOCOL               *ChildPciIo;
> +  UINT64                            SupportedVgaIo;
>  
>    OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>  
> @@ -277,13 +278,32 @@ QemuVideoControllerDriverStart (
>      goto ClosePciIo;
>    }
>  
> +  //
> +  // Get supported PCI attributes
> +  //
> +  Status = Private->PciIo->Attributes (
> +                             Private->PciIo,
> +                             EfiPciIoAttributeOperationSupported,
> +                             0,
> +                             &SupportedVgaIo
> +                             );
> +  if (EFI_ERROR (Status)) {
> +    goto ClosePciIo;
> +  }
> +
> +  SupportedVgaIo &= (UINT64)(EFI_PCI_IO_ATTRIBUTE_VGA_IO | EFI_PCI_IO_ATTRIBUTE_VGA_IO_16);
> +  if (SupportedVgaIo == 0) {
> +    Status = EFI_UNSUPPORTED;
> +    goto ClosePciIo;
> +  }
> +
>    //
>    // Set new PCI attributes
>    //
>    Status = Private->PciIo->Attributes (
>                              Private->PciIo,
>                              EfiPciIoAttributeOperationEnable,
> -                            EFI_PCI_DEVICE_ENABLE | EFI_PCI_IO_ATTRIBUTE_VGA_MEMORY | EFI_PCI_IO_ATTRIBUTE_VGA_IO,
> +                            EFI_PCI_DEVICE_ENABLE | EFI_PCI_IO_ATTRIBUTE_VGA_MEMORY | SupportedVgaIo,
>                              NULL
>                              );
>    if (EFI_ERROR (Status)) {
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

I will keep this patch tagged on my end, so that I can push it once
edk2-stable201905 has been tagged. Should I forget, please ping me.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#41986): https://edk2.groups.io/g/devel/message/41986
Mute This Topic: https://groups.io/mt/31948697/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2] OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA alias mode.
Posted by Laszlo Ersek 4 years, 10 months ago 
On 06/06/19 09:56, Laszlo Ersek wrote:
> On 06/06/19 09:42, Marc W Chen wrote:
>> Query the supported attributes firstly, then bitwise AND (&) both VGA_IO
>> and VGA_IO_16. Since the supported attributes should only have one of
>> VGA_IO or VGA_IO_16 set, the result of bitwise AND (&) is either VGA_IO
>> or IO_16. Then the result can be passed to PciIo->Attributes() to set the
>> attributes.
>>
>> Device driver should consider both since the mReserveVgaAliases in
>> PciBusDxe driver is default FALSE(implies that device driver can only set
>> VGA_IO_16 to PCI_ROOT_BRIDGE), and Platform code may not return
>> EFI_RESERVE_VGA_IO_ALIAS in GetPlatformPolicy of PciPlatformProtocol to
>> make mReserveVgaAliases become TRUE(implies that device driver can only
>> set VGA_IO to PCI_ROOT_BRIDGE), Currently OvmfPkg doesn't have problem
>> due to it has hard code value for PCI_ROOT_BRIDGE's attributes field, so
>> an IO access by PciIoProtocol will be successed due to
>> RootBridgeIoCheckParameter of PciRootBridgeIo.c will always get pass
>> result for legacy IO access.
>>
>> Usually the attributes field of PCI_ROOT_BRIDGE should be 0, in that case
>> it will have issue since the VGA_IO may not be able to be enabled, then
>> IO access by PciIoProtocol will be failed, hence the QemuVideoDxe driver
>> will not work fine.
>>
>> Signed-off-by: Marc Chen <marc.w.chen@intel.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Cc: Stefan Berger <stefanb@linux.ibm.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1880
>> ---
>>  OvmfPkg/QemuVideoDxe/Driver.c | 22 +++++++++++++++++++++-
>>  1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
>> index e8a613ef33..522110ef4e 100644
>> --- a/OvmfPkg/QemuVideoDxe/Driver.c
>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
>> @@ -201,6 +201,7 @@ QemuVideoControllerDriverStart (
>>    PCI_TYPE00                        Pci;
>>    QEMU_VIDEO_CARD                   *Card;
>>    EFI_PCI_IO_PROTOCOL               *ChildPciIo;
>> +  UINT64                            SupportedVgaIo;
>>  
>>    OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>>  
>> @@ -277,13 +278,32 @@ QemuVideoControllerDriverStart (
>>      goto ClosePciIo;
>>    }
>>  
>> +  //
>> +  // Get supported PCI attributes
>> +  //
>> +  Status = Private->PciIo->Attributes (
>> +                             Private->PciIo,
>> +                             EfiPciIoAttributeOperationSupported,
>> +                             0,
>> +                             &SupportedVgaIo
>> +                             );
>> +  if (EFI_ERROR (Status)) {
>> +    goto ClosePciIo;
>> +  }
>> +
>> +  SupportedVgaIo &= (UINT64)(EFI_PCI_IO_ATTRIBUTE_VGA_IO | EFI_PCI_IO_ATTRIBUTE_VGA_IO_16);
>> +  if (SupportedVgaIo == 0) {
>> +    Status = EFI_UNSUPPORTED;
>> +    goto ClosePciIo;
>> +  }
>> +
>>    //
>>    // Set new PCI attributes
>>    //
>>    Status = Private->PciIo->Attributes (
>>                              Private->PciIo,
>>                              EfiPciIoAttributeOperationEnable,
>> -                            EFI_PCI_DEVICE_ENABLE | EFI_PCI_IO_ATTRIBUTE_VGA_MEMORY | EFI_PCI_IO_ATTRIBUTE_VGA_IO,
>> +                            EFI_PCI_DEVICE_ENABLE | EFI_PCI_IO_ATTRIBUTE_VGA_MEMORY | SupportedVgaIo,
>>                              NULL
>>                              );
>>    if (EFI_ERROR (Status)) {
>>
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> I will keep this patch tagged on my end, so that I can push it once
> edk2-stable201905 has been tagged. Should I forget, please ping me.

Pushed as commit 662bd0da7fd7.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#41997): https://edk2.groups.io/g/devel/message/41997
Mute This Topic: https://groups.io/mt/31948697/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.