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

["Laszlo Ersek" <lersek@redhat.com>]

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