From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (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 0B0BD2095BB74 for ; Wed, 6 Sep 2017 07:45:02 -0700 (PDT) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Sep 2017 07:47:51 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,484,1498546800"; d="scan'208";a="146123027" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga005.jf.intel.com with ESMTP; 06 Sep 2017 07:47:51 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 6 Sep 2017 07:47:51 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.39]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.168]) with mapi id 14.03.0319.002; Wed, 6 Sep 2017 22:47:49 +0800 From: "Gao, Liming" To: Laszlo Ersek , "Yao, Jiewen" , "Wang, Jian J" , "Justen, Jordan L" CC: "Kinney, Michael D" , "edk2-devel@lists.01.org" Thread-Topic: [edk2] ASSERT in QemuVideoDxe driver during reset Thread-Index: AQHTJwGuDZngcfgupEyVp5K4o7VUXqKnUTCAgACeKyA= Date: Wed, 6 Sep 2017 14:47:49 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14D78AF1E@shsmsx102.ccr.corp.intel.com> References: <130779a4-f1c8-bf07-8b85-8125d466aed3@redhat.com> <74D8A39837DF1E4DA445A8C0B3885C503A9A8802@shsmsx102.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: ASSERT in QemuVideoDxe driver during reset 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: Wed, 06 Sep 2017 14:45:02 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo: > * Independently: if the CSM too conflicts with the null detection > feature, then we should probably report an "unsupported configuration" > error for the following case: >=20 > -D NULL_DETECT_ENABLE -D CSM_ENABLE >=20 > But, I don't know if triggering build errors, with custom error > messages, is possible in DSC files. Now, there is no support to report the error message for the invalid combin= ation. I think this is a good request to BaseTools. We can introduce !error= "error message" syntax in DSC/FDF. If the invalid combination happen, !err= or will trig build error message. Could you help submit this feature reques= t in BugZillar? Thanks Liming > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of La= szlo Ersek > Sent: Wednesday, September 6, 2017 9:19 PM > To: Yao, Jiewen ; Wang, Jian J ; Justen, Jordan L > Cc: Kinney, Michael D ; edk2-devel@lists.01.o= rg > Subject: Re: [edk2] ASSERT in QemuVideoDxe driver during reset >=20 > On 09/06/17 13:16, Yao, Jiewen wrote: > > HI > > I think the NULL detection feature should *never* break any existing pl= atform. > > No real function should be skipped for PcdNullDetection. > > > > If InstallVbeShim () is needed for CSM, we should always call InstallVb= eShim() for CSM. >=20 > Wait a second please -- the VBE shim exists so that we can boot Windows7 > on OVMF *without* the CSM. >=20 > If you build OVMF with the SeaBIOS CSM, then Windows7 will simply boot > in legacy mode, and work as usual. >=20 > But if you build OVMF without a CSM, then Windows7 will break mid-way > into booting. The reason is that Windows7 insists on some legacy VBE > services even if it is booted on a pure UEFI system. By faking a minimal > set of legacy VBE services, Windows7 boots just fine in UEFI mode. >=20 > (Windows 7 doesn't actually execute the VBE services' real mode code on > the processor; instead the code is interpreted in a software emulator.) >=20 > The dependency on legacy VBE services is arguably a bug of UEFI Windows > 7. One way to work it around is to build OVMF with the SeaBIOS CSM, and > just boot Windows7 in legacy mode. >=20 > However, in some environments, building OVMF with a CSM is not > desirable. That's why we created the VBE shim. It is a terrible hack, > but it fits the Windows7 bug just fine. >=20 > > I suggest below options: > > > > 1) In OVMF, if CSM is enabled, disable PcdNullDetection. > > > > 2) In OVMF, if any driver need access first 4K page, the code sho= uld explicit call SetAttribute(0, 4K, READ|WRITE), if > PcdNullDetection is enabled. > > > > Either #1 or #2 is OK. > > #1 is simple, and #2 can help detect more potential issue. >=20 > * Regarding #1: >=20 > As I wrote above, the VBE shim is mutually exclusive with CSM: >=20 > // > // The allocation request may fail, eg. if LegacyBiosDxe has already ru= n. > // >=20 > So, if the CSM was built into OVMF, then the VBE shim is already skipped > at boot. >=20 >=20 > * Regarding #2: >=20 > I agree that *temporarily* enabling R/W access to page zero, using the > appropriate DXE service, could make the VBE Shim installation work. >=20 > However, making page 0 unaccessible right after, could very easily break > Windows 7, if some component of Windows 7 accessed page 0, before > setting up new page tables. After all, in the InstallVbeShim() function, > we write stuff to page 0 *because* Windows 7 will read it from there. >=20 >=20 > * If it is more convenient than a "--pcd=3D..." option for "build", we ca= n > also add another build-time macro to the OVMF DSC files, called > NULL_DETECT_ENABLE. This would then turn on the PcdNullDetection bits, > for "-D NULL_DETECT_ENABLE". >=20 > NULL_DETECT_ENABLE should default to FALSE. >=20 >=20 > * Independently: if the CSM too conflicts with the null detection > feature, then we should probably report an "unsupported configuration" > error for the following case: >=20 > -D NULL_DETECT_ENABLE -D CSM_ENABLE >=20 > But, I don't know if triggering build errors, with custom error > messages, is possible in DSC files. >=20 > Thanks > Laszlo >=20 >=20 > > From: Laszlo Ersek [mailto:lersek@redhat.com] > > Sent: Wednesday, September 6, 2017 6:21 PM > > To: Wang, Jian J ; Justen, Jordan L ; Yao, Jiewen > > Cc: edk2-devel@lists.01.org; Kinney, Michael D > > Subject: Re: ASSERT in QemuVideoDxe driver during reset > > > > On 09/06/17 10:15, Wang, Jian J wrote: > >> Hi guys, > >> > >> I found an ASSERT issue in function InstallVbeShim() in QemuVideoDxe > >> driver during reset. The assert statement is like below. > >> > >> ASSERT (Int0x10->Segment =3D=3D 0x0000); > >> ASSERT (Int0x10->Offset =3D=3D 0x0000); > >> > >> This happened after I have enabled NULL pointer access detection > >> feature, in which page 0 (4K) is disabled. > > > > The NULL pointer access detection conflicts with the VBE shim. For > > installing the VBE shim, OVMF has to write to page 0, on purpose. > > > > Please see commit 90803342b1b6 ("OvmfPkg: QemuVideoDxe: Int10h stub for > > Windows 7 & 2008 (stdvga, QXL)", 2014-05-20). > > > >> And because of page 0 disabled, I have to skip the memory clearing for > >> page 0 in DXE core. > > > > By doing that, you invalidate the following comment in said OVMF commit= : > > > > + // > > + // We managed to allocate the page at zero. SVN r14218 guarantees = that it > > + // is NUL-filled. > > + // > > + ASSERT (Int0x10->Segment =3D=3D 0x0000); > > + ASSERT (Int0x10->Offset =3D=3D 0x0000); > > > > Because SVN r14218 was what added the clearing of page 0 to the DXE > > core. (Expressed as a git commit: d436d5ca0936.) > > > >> Otherwise it will cause page fault exception there. It seems that QEMU > >> may clear all its memory at startup. Skipping the action of clearing > >> page 0 in core won't cause ASSERT issue in QemuVideoDxe, for the first > >> time boot. But QemuVideoDxe will write int10 vector at memory 0x10 and > >> QEMU will not clear all its memory during warm boot. ASSERT will be > >> triggered after reset. > > > > QEMU does not clear guest RAM at reboot (or S3 resume). > > > >> It's easy to fix this issue but there're some subtle situations which > >> I'm not quite certain. I'd like your opinions for them. > >> > >> Here're my thoughts on several solutions: > >> a) Remove the ASSERT statement in InstallVbeShim(). > > > > Not a good idea. > > > >> But I'm sure if it is safe to do so because I don't quite understand > >> the purpose of the ASSERT. > > > > The ASSERT expresses our belief that, after we manage to *allocate* pag= e > > 0, we can freely write the real mode Int0x10 vector there, without > > overwriting anything else. > > > >> b) Instead of skipping clearing page 0, enable it, do clearing and > >> then disable it. The problem here is that CPU arch protocol is not > >> ready at that time. I have to "manually" do page operation, which > >> might be non-portable and a little bit odd in DXE core. > >> c) Move code clearing page 0 from DXE core to another place wherever > >> appropriate, like DxeIpl or cpu driver. But I think there's a good > >> reason to put code there before. > > > > I'm not sure how the NULL pointer access detection is enabled. Is it > > enabled by a PCD? > > > > Hm, yes, it seems, from your patch > > > > [edk2] [PATCH 1/2] Implement NULL pointer detection for EDK-II Core > > > > that the PCD is called > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection > > > > I can see in that patch that the DXE core is updated *not* to clear pag= e > > 0 if the PCD is set to TRUE. That makes sense. > > > > However, OVMF should also be updated to skip the VBE shim installation > > if the PCD is set to TRUE. Namely, if the PCD is set to TRUE, then we > > simply cannot put the legacy Int0x10 vector in place, so we shouldn't > > even try. > > > > Please extend your original patch set with a patch for OvmfPkg. In the > > file "OvmfPkg/QemuVideoDxe/Driver.c", locate the InstallVbeShim() call > > site: > > > >> #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 > >> if (Private->Variant =3D=3D QEMU_VIDEO_BOCHS_MMIO || > >> Private->Variant =3D=3D QEMU_VIDEO_BOCHS) { > >> InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBuf= ferBase); > >> } > >> #endif > > > > and please skip the call if the PCD is set to TRUE: > > > >> #if defined MDE_CPU_IA32 || defined MDE_CPU_X64 > >> if (!FeaturePcdGet (PcdNullPointerDetection) && > >> (Private->Variant =3D=3D QEMU_VIDEO_BOCHS_MMIO || > >> Private->Variant =3D=3D QEMU_VIDEO_BOCHS)) { > >> InstallVbeShim (Card->Name, Private->GraphicsOutput.Mode->FrameBuf= ferBase); > >> } > >> #endif > > > > Now, the consequence of this would be a working OVMF build, but it woul= d > > also break booting UEFI Windows 7 on OVMF. Therefore, please append > > *another* patch to your patch set, setting the Feature PCD to FALSE in > > all three OVMF DSC files: > > > > - OvmfPkg/OvmfPkgIa32.dsc > > - OvmfPkg/OvmfPkgIa32X64.dsc > > - OvmfPkg/OvmfPkgX64.dsc > > > > We cannot break Windows 7 booting in upstream OVMF. If someone really > > wants the null pointer detection in OVMF (and doesn't need Windows 7 fo= r > > sure), they can patch the DSC files, or else pass the > > > > --pcd=3DgEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection=3DTRUE > > > > option to the "build" utility. > > > > ... I'm now seeing in the original discussion that perhaps you will > > introduce a bitmap instead (one bit per firmware phase). That's OK with > > me; please reinterpret all of the above with the "DXE bit" then. > > > > When you post the next version of the patch set, please CC Jordan and > > myself on the OvmfPkg patches. > > > > Thank you! > > Laszlo > > >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel