From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 8139D21E43B42 for ; Fri, 22 Sep 2017 08:26:05 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 326E14E33E; Fri, 22 Sep 2017 15:29:13 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 326E14E33E Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-219.rdu2.redhat.com [10.10.120.219]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2B5DD63631; Fri, 22 Sep 2017 15:29:10 +0000 (UTC) From: Laszlo Ersek To: Jian J Wang , edk2-devel@lists.01.org Cc: Star Zeng , Eric Dong , Jiewen Yao , Michael Kinney , Jordan Justen , Ayellet Wolman References: <20170921052032.13652-1-jian.j.wang@intel.com> <20170921052032.13652-7-jian.j.wang@intel.com> <6ed8b38e-3387-e3a1-972f-5922470ed4c7@redhat.com> Message-ID: <27af5dab-0c14-1c8b-1e2e-f08772471d06@redhat.com> Date: Fri, 22 Sep 2017 17:29:10 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <6ed8b38e-3387-e3a1-972f-5922470ed4c7@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Fri, 22 Sep 2017 15:29:13 +0000 (UTC) Subject: Re: [PATCH v2 6/6] OvmfPkg/QemuVideoDxe: Bypass NULL pointer detection during VBE SHIM installing X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 22 Sep 2017 15:26:05 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/22/17 13:50, Laszlo Ersek wrote: > This patch looks great to me, I would like to request a few small > updates: > > On 09/21/17 07:20, Jian J Wang wrote: >> QemuVideoDxe driver will install VBE SHIM into page 0. If NULL pointer > > (1) please replace the word "install" with "link". > > The VBE Shim is technically installed into the "real-mode" C segment, > only the int 0x10 vector lives in page 0. > >> detection is enabled, this driver will fail to load. NULL pointer detection >> bypassing code is added to prevent such problem during boot. >> >> Please note that Windows 7 will try to access VBE SHIM during boot if it's >> installed, and then cause boot failure. This can be fixed by setting BIT7 >> of PcdNullPointerDetectionPropertyMask to disable NULL pointer detection >> after EndOfDxe. As far as we know, there's no other OSs has such issue. > > This is not a request, just a comment: I verified the default value in > the .dec, and I see it is 0. So there's no need to post an additional > patch for the OVMF DSC files, in order to set BIT7. Actually, let me take a step back, and re-think the necessity of all this work for QemuVideoDxe! The facts are: (1) The *only* purpose of the VBE Shim is to allow Windows 7 to boot in pure UEFI mode (i.e. without a CSM). (2) If I understand correctly, you guys have verified that Windows 7 cannot boot with the page0 protection enabled, *regardless* of what we do in QemuVideoDxe. Can you confirm this please? With the above in mind, let's consider the effects of the "PcdNullPointerDetectionPropertyMask" bits: * BIT0 clear: - The page0 protection is completely disabled. - This patch does nothing, in effect. - The VBE Shim works. - Windows 7 boots. * BIT0 set, BIT7 also set: - The page0 protection is disabled in the DXE core at the end of DXE. - This patch does nothing, in effect. - The VBE Shim works, because it is a UEFI driver, and it connects its devices (and installs the shim) after End-of-Dxe, at which point page0 protection is no longer in effect. - Windows 7 boots fine, again because it is loaded after End-of-Dxe. * BIT0 set, BIT7 clear: - The page0 protection is never disabled until the OS (loader) installs its own page tables. - This patch enables the VBE Shim to work, by temporarily disabling page0 protection. - However, Windows 7 will fail to boot nonetheless, because it cannot cope with page0 protection. (This is fact (2).) Now, if you consider fact (1) as well: given that Windows 7 cannot boot with page0 protection enabled *anyway*, why mess with the VBE Shim at all? How about the following patch instead: > diff --git a/OvmfPkg/QemuVideoDxe/VbeShim.c b/OvmfPkg/QemuVideoDxe/VbeShim.c > index e45a08e8873f..8ba5522cde3c 100644 > --- a/OvmfPkg/QemuVideoDxe/VbeShim.c > +++ b/OvmfPkg/QemuVideoDxe/VbeShim.c > @@ -75,6 +75,20 @@ InstallVbeShim ( > UINTN Printed; > VBE_MODE_INFO *VbeModeInfo; > > + if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) { > + DEBUG (( > + DEBUG_WARN, > + "%a: page 0 protected, not installing VBE shim\n", > + __FUNCTION__ > + )); > + DEBUG (( > + DEBUG_WARN, > + "%a: page 0 protection prevents Windows 7 from booting anyway\n", > + __FUNCTION__ > + )); > + return; > + } > + > Segment0 = 0x00000; > SegmentC = 0xC0000; > SegmentF = 0xF0000; Thanks! Laszlo