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 CAE962117AE77 for ; Tue, 23 Oct 2018 03:19:03 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 01ADB3001922; Tue, 23 Oct 2018 10:19:03 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-214.rdu2.redhat.com [10.10.120.214]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8DBBA608F2; Tue, 23 Oct 2018 10:18:54 +0000 (UTC) To: yuchenlin@synology.com Cc: edk2-devel@lists.01.org, jordan.l.justen@intel.com, ard.biesheuvel@linaro.org, anthony.perard@citrix.com, julien.grall@linaro.org, Gerd Hoffmann , Phil Dennis-Jordan References: <20181023024057.21942-1-yuchenlin@synology.com> From: Laszlo Ersek Message-ID: Date: Tue, 23 Oct 2018 12:18:53 +0200 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: <20181023024057.21942-1-yuchenlin@synology.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.47]); Tue, 23 Oct 2018 10:19:03 +0000 (UTC) Subject: Re: [PATCH] OvmfPkg: initialize bochs when initializing 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, 23 Oct 2018 10:19:04 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit (1) Adding Gerd (because he maintains video in QEMU), and Phil Dennis-Jordan (for authoring commit c137d9508169, "OvmfPkg/QemuVideoDxe: VMWare SVGA device support", 2017-04-07). On 10/23/18 04:40, yuchenlin@synology.com wrote: > From: yuchenlin > > When driver doesn't set fifo config, the vmsvga will fall back > to std vga. However, we don't initialize vbe related port. It > causes blank screen in qemu console. (2) The words "when driver doesn't set fifo config" tell me nothing. The QemuVideoDxe directory has zero instances of the word "fifo". The same applies to "OvmfPkg/Include/IndustryStandard/VmwareSvga.h". In addition, the "vmsvga will fall back to std vga" statement is also unclear. Is that a statement about the QEMU device model's behavior? I vaguely suspect that your intent might be to say, "QemuVideoDxe does not perform a necessary configuration step, and therefore it cannot drive QEMU's VMW SVGA device". However, if that is indeed your intent, then I believe something must have changed recently in QEMU, because QemuVideoDxe *definitely* worked when Phil contributed the VMW SVGA driver logic, in commit c137d9508169. Are we talking about a QEMU regression, or a driver-side configuration step that QemuVideoDxe has always missed (and we're being punished for it only now)? > This patch will fix "Guest has not initialized the display (yet)" > when using qemu -device vmware-svga with ovmf. Right, as I write above, this definitely worked earlier. I suggest bisecting QEMU (and/or testing older QEMU machine types) to identify the QEMU side change. Once we know that, we can decide whether this is a QEMU regression, or just exposing a long-standing OVMF bug. Comments about the code below: > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: yuchenlin > --- > OvmfPkg/QemuVideoDxe/Driver.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c > index 0dce80e59..255c01881 100644 > --- a/OvmfPkg/QemuVideoDxe/Driver.c > +++ b/OvmfPkg/QemuVideoDxe/Driver.c > @@ -1067,8 +1067,7 @@ InitializeVmwareSvgaGraphicsMode ( > > VmwareSvgaWrite (Private, VmwareSvgaRegEnable, 1); > > - SetDefaultPalette (Private); > - ClearScreen (Private); > + InitializeBochsGraphicsMode (Private, ModeData); > } > > EFI_STATUS > (3) Calling InitializeBochsGraphicsMode() from within InitializeVmwareSvgaGraphicsMode() seems wrong, considering the current structure of the driver. We only have InitializeXxxGraphicsMode() calls in QemuVideoGraphicsOutputSetMode(). In order to determine which variant to call, we check the "Private->Variant" field, in a "switch" statement. Therefore: (3a) If a general fallback from "VmwareSvga" to "Bochs" is necessary, then the fallback logic should be added to earlier code that sets the Variant field. You can see an example for that in the QemuVideoControllerDriverStart() function, near the debug message "QemuVideo: No mmio bar, fallback to port io". There the Variant field is degraded from the originally detected QEMU_VIDEO_BOCHS_MMIO value, to QEMU_VIDEO_BOCHS. (3b) Or else, if calling InitializeVmwareSvgaGraphicsMode() is fine as a basis, but we also need the actions of InitializeBochsGraphicsMode() *in addition*, then please: - extract the common actions from InitializeBochsGraphicsMode() to a new helper function, and call the helper from both InitializeBochsGraphicsMode() and InitializeVmwareSvgaGraphicsMode(), - explain, in InitializeVmwareSvgaGraphicsMode(), *why* the Bochs config actions are necessary, in addition to the VmwareSvga actions. Thanks, Laszlo