From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=59.124.61.242; helo=synology.com; envelope-from=yuchenlin@synology.com; receiver=edk2-devel@lists.01.org Received: from synology.com (synology.com [59.124.61.242]) (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 9692E2118C509 for ; Tue, 6 Nov 2018 18:36:18 -0800 (PST) Received: from _ (localhost [127.0.0.1]) by synology.com (Postfix) with ESMTPA id 861E32380325; Wed, 7 Nov 2018 10:36:16 +0800 (CST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=synology.com; s=123; t=1541558176; bh=nr5pmXFMU7Q9Q5CkJ8FR2bHD9Idh4T/YKwySzPDrmh8=; h=Date:From:To:Cc:Subject:In-Reply-To:References; b=ZGVbuULUx68nQZdRaCEBvjapd1NXBP/vCzlvDqbgAoN8ps287T/f6hKH+O1wIYJSi 15GRpHA/GaF3TiAcA7edHPMHf8VymQH2yFLuVa1nypTkQvAX41TGJJHusMgjgcZ+PJ sArhmRl31qNiY+AQ2Wr6Is/QXXYkqcNG7ZDS3QhI= MIME-Version: 1.0 Date: Wed, 07 Nov 2018 10:36:16 +0800 From: yuchenlin To: Laszlo Ersek 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 In-Reply-To: <4d3da28e-42ca-c54e-1efa-c6fc8c9b0fb2@redhat.com> References: <20181102032402.19686-1-yuchenlin@synology.com> <20181102032402.19686-6-yuchenlin@synology.com> <4d3da28e-42ca-c54e-1efa-c6fc8c9b0fb2@redhat.com> Message-ID: <72357c4c380cfa491076c1baa8e8ce71@synology.com> X-Sender: yuchenlin@synology.com User-Agent: Roundcube Webmail/1.1.2 X-Synology-MCP-Status: no X-Synology-Spam-Flag: no X-Synology-Spam-Status: score=0, required 6, WHITELIST_FROM_ADDRESS 0 X-Synology-Virus-Status: no 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: Wed, 07 Nov 2018 02:36:19 -0000 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit On 2018-11-06 19:47, Laszlo Ersek wrote: > 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. > Will do. Thanks, yuchenlin >> >> 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. I agree. Will do Thanks, yuchenlin > > ... 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