From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2AA0620336AD5 for ; Tue, 6 Nov 2018 03:48:01 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D0A88308FF0C; Tue, 6 Nov 2018 11:48:00 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-95.rdu2.redhat.com [10.10.120.95]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3AF336014D; Tue, 6 Nov 2018 11:47:47 +0000 (UTC) To: yuchenlin@synology.com Cc: edk2-devel@lists.01.org, phil@philjordan.eu, jordan.l.justen@intel.com, anthony.perard@citrix.com, =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , Gerd Hoffmann References: <20181102032402.19686-1-yuchenlin@synology.com> <20181102032402.19686-6-yuchenlin@synology.com> From: Laszlo Ersek Message-ID: <4d3da28e-42ca-c54e-1efa-c6fc8c9b0fb2@redhat.com> Date: Tue, 6 Nov 2018 12:47:46 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181102032402.19686-6-yuchenlin@synology.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Tue, 06 Nov 2018 11:48:01 +0000 (UTC) Subject: Re: [PATCH v2 5/5] OvmfPkg: simply use the Bochs interface for vmsvga X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Nov 2018 11:48:03 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit I suggest the following: On 11/02/18 04:24, yuchenlin via edk2-devel wrote: > From: yuchenlin > > BAR | std vga | vmsvga > --------------------------------- > 0 | Framebuffer | I/O space > 1 | Reserved | Framebuffer > 2 | MMIO | FIFO > > Because of the PCI BARs difference between std vga and > vmsvga, we can not simply recognize the "QEMU VMWare SVGA" > as the QEMU_VIDEO_BOCHS_MMIO variant. > > Instead, we remain variant QEMU_VIDEO_VMWARE_SVGA, and use > it for: > > (1) Get framebuffer from correct PCI BAR > (2) Prevent using BAR2 for MMIO [a] The commit message should be udpated as follows: - 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: both of the currently listed reasons, plus "prevent mis-recognizing VMW SVGA as QXL" as the third one. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: yuchenlin > --- > OvmfPkg/QemuVideoDxe/Driver.c | 18 ++++++++++++++++-- > OvmfPkg/QemuVideoDxe/Gop.c | 3 ++- > OvmfPkg/QemuVideoDxe/Qemu.h | 2 ++ > 3 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c > index 2304afd1e6..76d4a2d27e 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 */ > } > @@ -256,6 +262,12 @@ QemuVideoControllerDriverStart ( > goto ClosePciIo; > } > Private->Variant = Card->Variant; > + if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) { > + Private->FrameBufferVramBarIndex = PCI_BAR_IDX1; > + } else { > + Private->FrameBufferVramBarIndex = PCI_BAR_IDX0; > + } > + > > // > // IsQxl is based on the detected Card->Variant, which at a later point might > @@ -320,7 +332,8 @@ QemuVideoControllerDriverStart ( > // Check if accessing the bochs interface works. > // > if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || > - Private->Variant == QEMU_VIDEO_BOCHS) { > + Private->Variant == QEMU_VIDEO_BOCHS || > + Private->Variant == QEMU_VIDEO_VMWARE_SVGA) { > UINT16 BochsId; > BochsId = BochsRead(Private, VBE_DISPI_INDEX_ID); > if ((BochsId & 0xFFF0) != VBE_DISPI_ID0) { > @@ -383,6 +396,7 @@ QemuVideoControllerDriverStart ( > break; > case QEMU_VIDEO_BOCHS_MMIO: > case QEMU_VIDEO_BOCHS: > + case QEMU_VIDEO_VMWARE_SVGA: > Status = QemuVideoBochsModeSetup (Private, IsQxl); > break; > default: > @@ -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..3abc5eeb36 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 > ); > @@ -177,6 +177,7 @@ Routine Description: > break; > case QEMU_VIDEO_BOCHS_MMIO: > case QEMU_VIDEO_BOCHS: > + case QEMU_VIDEO_VMWARE_SVGA: > InitializeBochsGraphicsMode (Private, &QemuVideoBochsModes[ModeData->InternalModeIndex]); > break; > default: > 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; > > /// > [b] How about the following -- incremental, to be squashed -- patch: > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c > index 76d4a2d27e7e..8e02700d3960 100644 > --- a/OvmfPkg/QemuVideoDxe/Driver.c > +++ b/OvmfPkg/QemuVideoDxe/Driver.c > @@ -262,12 +262,6 @@ QemuVideoControllerDriverStart ( > goto ClosePciIo; > } > Private->Variant = Card->Variant; > - if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) { > - Private->FrameBufferVramBarIndex = PCI_BAR_IDX1; > - } else { > - Private->FrameBufferVramBarIndex = PCI_BAR_IDX0; > - } > - > > // > // IsQxl is based on the detected Card->Variant, which at a later point > might > @@ -328,12 +322,19 @@ 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. > // > if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || > - Private->Variant == QEMU_VIDEO_BOCHS || > - Private->Variant == QEMU_VIDEO_VMWARE_SVGA) { > + Private->Variant == QEMU_VIDEO_BOCHS) { > UINT16 BochsId; > BochsId = BochsRead(Private, VBE_DISPI_INDEX_ID); > if ((BochsId & 0xFFF0) != VBE_DISPI_ID0) { > @@ -396,7 +397,6 @@ QemuVideoControllerDriverStart ( > break; > case QEMU_VIDEO_BOCHS_MMIO: > case QEMU_VIDEO_BOCHS: > - case QEMU_VIDEO_VMWARE_SVGA: > Status = QemuVideoBochsModeSetup (Private, IsQxl); > break; > default: > diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c > index 3abc5eeb36a6..6f542d9eac05 100644 > --- a/OvmfPkg/QemuVideoDxe/Gop.c > +++ b/OvmfPkg/QemuVideoDxe/Gop.c > @@ -177,7 +177,6 @@ Routine Description: > break; > case QEMU_VIDEO_BOCHS_MMIO: > case QEMU_VIDEO_BOCHS: > - case QEMU_VIDEO_VMWARE_SVGA: > InitializeBochsGraphicsMode (Private, > &QemuVideoBochsModes[ModeData->InternalModeIndex]); > break; > default: This would produce the following -- squashed -- patch, for v3: > diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h > index d7da761705a1..3aac9eeca687 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; > > /// > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c > index 2304afd1e686..8e02700d3960 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 d490fa7a2e6f..6f542d9eac05 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 > ); For me this is much easier to understand. ... While we discuss this, I'll go ahead and push the first four patches. The code being reverted is dead anyway. I'll report back about the commit hashes. Thanks, Laszlo