Re: [PATCH v3] OvmfPkg: simply use the Bochs interface for vmsvga

[Laszlo Ersek <lersek@redhat.com>]

On 11/07/18 04:47, yuchenlin@synology.com wrote:
> From: yuchenlin <yuchenlin@synology.com>
> 
> BAR  |    std vga     |  vmsvga
> ---------------------------------
> 0    |   Framebuffer  | I/O space
> 1    |   Reserved     | Framebuffer
> 2    |   MMIO         | FIFO
> 
> - We cannot recognize VMW SVGA as BOCHS because that would confuse the
>   IsQxl setting in QemuVideoControllerDriverStart(),
> 
> - We cannot recognize VMW SVGA as BOCHS_MMIO because BAR2 on VMW SVGA is
>   not the BOCHS MMIO BAR (we can only use port IO).
> 
> Therefore the list of reasons for which we should introduce
> QEMU_VIDEO_VMWARE_SVGA should name three reasons:
> 
>  (1) Get framebuffer from correct PCI BAR
>  (2) Prevent using BAR2 for MMIO
>  (3) Prevent mis-recognizing VMW SVGA as QXL
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: yuchenlin <yuchenlin@synology.com>
> ---
> Changelog:
> 
> v1 -> v2
> * use 'else' clause (Thanks Philippe).
> * add more comment in revert patches (Thanks Philippe).
> * reorder the revert patches, we should revert the last commit first.
> * use correct framebuffer to ClearScreen.
> * revert VMWare svga definitions.
> 
> v2 -> v3
> * Update commit message (Thanks Laszlo)
> * Treat QEMU_VIDEO_VMWARE_SVGA as QEMU_VIDEO_BOCHS (Thanks Laszlo)
> 
> ---
>  OvmfPkg/QemuVideoDxe/Driver.c | 16 +++++++++++++++-
>  OvmfPkg/QemuVideoDxe/Gop.c    |  2 +-
>  OvmfPkg/QemuVideoDxe/Qemu.h   |  2 ++
>  3 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
> index 2304afd1e6..8e02700d39 100644
> --- a/OvmfPkg/QemuVideoDxe/Driver.c
> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
> @@ -69,6 +69,12 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = {
>          0x1050,
>          QEMU_VIDEO_BOCHS_MMIO,
>          L"QEMU VirtIO VGA"
> +    },{
> +        PCI_CLASS_DISPLAY_VGA,
> +        0x15ad,
> +        0x0405,
> +        QEMU_VIDEO_VMWARE_SVGA,
> +        L"QEMU VMWare SVGA"
>      },{
>          0 /* end of list */
>      }
> @@ -316,6 +322,14 @@ QemuVideoControllerDriverStart (
>      }
>    }
>  
> +  //
> +  // VMWare SVGA is handled like Bochs (with port IO only).
> +  //
> +  if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) {
> +    Private->Variant = QEMU_VIDEO_BOCHS;
> +    Private->FrameBufferVramBarIndex = PCI_BAR_IDX1;
> +  }
> +
>    //
>    // Check if accessing the bochs interface works.
>    //
> @@ -764,7 +778,7 @@ ClearScreen (
>    Private->PciIo->Mem.Write (
>                          Private->PciIo,
>                          EfiPciIoWidthFillUint32,
> -                        0,
> +                        Private->FrameBufferVramBarIndex,
>                          0,
>                          0x400000 >> 2,
>                          &Color
> diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
> index d490fa7a2e..6f542d9eac 100644
> --- a/OvmfPkg/QemuVideoDxe/Gop.c
> +++ b/OvmfPkg/QemuVideoDxe/Gop.c
> @@ -60,7 +60,7 @@ QemuVideoCompleteModeData (
>  
>    Private->PciIo->GetBarAttributes (
>                          Private->PciIo,
> -                        0,
> +                        Private->FrameBufferVramBarIndex,
>                          NULL,
>                          (VOID**) &FrameBufDesc
>                          );
> diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h
> index d7da761705..3aac9eeca6 100644
> --- a/OvmfPkg/QemuVideoDxe/Qemu.h
> +++ b/OvmfPkg/QemuVideoDxe/Qemu.h
> @@ -92,6 +92,7 @@ typedef enum {
>    QEMU_VIDEO_CIRRUS_5446,
>    QEMU_VIDEO_BOCHS,
>    QEMU_VIDEO_BOCHS_MMIO,
> +  QEMU_VIDEO_VMWARE_SVGA,
>  } QEMU_VIDEO_VARIANT;
>  
>  typedef struct {
> @@ -120,6 +121,7 @@ typedef struct {
>    QEMU_VIDEO_VARIANT                    Variant;
>    FRAME_BUFFER_CONFIGURE                *FrameBufferBltConfigure;
>    UINTN                                 FrameBufferBltConfigureSize;
> +  UINT8                                 FrameBufferVramBarIndex;
>  } QEMU_VIDEO_PRIVATE_DATA;
>  
>  ///
> 

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

I'm not pushing this patch at once, for two reasons:

- I should leave time for other reviewers to comment,
- we are now in the soft feature freeze period
<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning>,
and this is not a bugfix, but a feature (re-)enablement that is only now
being reviewed.

I'll keep this tagged on my queue until after the edk2-stable201811 tag
is pushed, and then I'll push this patch. Please do ping me, should I
forget.

Thank you!
Laszlo