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

[Laszlo Ersek <lersek@redhat.com>]

On 11/07/18 18:06, Laszlo Ersek wrote:
> 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.

I haven't forgotten. :)

* First, I've cherry-picked the first four patches (from the v2 series)
from the git history. See
<http://mid.mail-archive.com/3167b5fd-5514-5a79-e400-39d7c64e50ea@redhat.com>.

* Second, I've added PhilMD's R-b to those four patches. See
<http://mid.mail-archive.com/a2bb9800-6e82-b552-a950-fa11377d24e1@redhat.com>.

* Third, PhilMD got busy with other stuff meanwhile, so I've tested the
patch I'm replying to, myself. I have confirmed the following device
models continue to work:
- Cirrus 5446
- QEMU Standard VGA
- QEMU QXL VGA
- QEMU VirtIO VGA

Therefore, for patch #5 (the present patch),

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

* Fourth, I've also determined that the patch enables OVMF to drive:
- QEMU VMWare SVGA

therefore, for this patch,

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

* Fifth, the full series has been pushed as commit range
7f3b0bad4bbb..d021868ccf49.


Thank you for the contribution, and also for your patience!
Laszlo