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

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

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