From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web12.32705.1599202073346538260 for ; Thu, 03 Sep 2020 23:47:53 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=dE+pvMzr; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1599202072; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=7/0Y7GX5+jLtuY5NednWxGHU+vaKNG6USTZsg8rHZJY=; b=dE+pvMzrIFRfkgTgUzz/uknbZ28tha22gENiI4ksfyYxv/ooWZULQ0xM3LcYh1UPJ1DLkS 0uy1O8WiV/+1akMFFByty6j0kgiNDcoXXm9QPrjiwnRBVYyugd0vol9qlvi8T2HbBHYMOV BVkLNPfxNiDiR3kCLpMXm8aPRY52lm8= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-481-syKO1pccP4Kqk4ZRT0LlLg-1; Fri, 04 Sep 2020 02:47:48 -0400 X-MC-Unique: syKO1pccP4Kqk4ZRT0LlLg-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6B30EAF200; Fri, 4 Sep 2020 06:47:47 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-161.ams2.redhat.com [10.36.112.161]) by smtp.corp.redhat.com (Postfix) with ESMTP id 460D85D9CC; Fri, 4 Sep 2020 06:47:46 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT. To: "Ni, Ray" , "devel@edk2.groups.io" , "Dong, Eric" Cc: "Lou, Yun" References: <20200903151147.1196-1-eric.dong@intel.com> <9c9d8289-4f8e-75d8-2816-750195a54842@redhat.com> From: "Laszlo Ersek" Message-ID: <11c5546f-6f5c-e7c6-207e-1c442191da4e@redhat.com> Date: Fri, 4 Sep 2020 08:47:45 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0.002 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Content-Language: en-US On 09/04/20 03:34, Ni, Ray wrote: > Why do we need a new PCD to control such check? Under what circumstance the PCD is false? Under *all* circumstances, except when the platform wants to be compatible with the UEFI application that manually moves the GDT or IDT or CR3 above 4GB. To repeat my earlier point: I consider the actions of this UEFI application *invalid*. The UEFI spec does not authorize UEFI applications to mess with low level concepts such as IDT / GDT / CR3. All that stuff belongs to platform / DXE drivers. So if a UEFI application messes with them anyway, breakage is *expected*. In other words, I consider this patch to be adding compatibility with an invalid UEFI application. That's fine (I assume Eric has a good reason for asking for this compatibility -- must be an important or high profile application), but we should *not* pessimize other platforms. This is the reason for the Feature PCD -- on platforms that do not care about compatibility with this application, the compiler should be able to optimize away all this IDTR / GDTR / CR3 checking. > We may need to move such check out of MpLib.c. Yes, I agree. Minimally, it should go into the DXE instance of MpInitLib. Another option is to move it out to CpuDxe. > Because when bps runs at 32bit mode, AP doesn’t need to switch to long mode, such check is not needed and also always passes. Yes, that's a further possible refinement. Restrict the logic to FeaturePCD && DXE && X64 > We should not assume PEI runs at 32 bit mode. The PEI phase is irrelevant here -- by the time the UEFI application runs, PEI is gone. So indeed PEI components / lib instances should not be changed in any way. (Note: as I said earlier, if someone can show that edk2 *itself* has this problem, that is, GDT / IDT / CR can be set above 4GB *without* using this particular UEFI shell application, then we have a more serious problem. But in that case, it's not MpInitLib or the MP services protocol that we need to fix -- instead, we need to fix the *allocations* / placements themselves, so that they be under 4GB.) Thanks Laszlo > > ________________________________ > 发件人: devel@edk2.groups.io 代表 Laszlo Ersek > 发送时间: Friday, September 4, 2020 3:55:47 AM > 收件人: Dong, Eric ; devel@edk2.groups.io > 抄送: Ni, Ray > 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT. > > On 09/03/20 21:00, Laszlo Ersek wrote: > >> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the >> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and >> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c". >> >> Instead, the calls should be made in the DXE instance of the library >> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the >> functions: >> >> - MpInitLibStartupAllAPs >> - MpInitLibStartupThisAP >> - MpInitLibSwitchBSP >> - MpInitLibEnableDisableAP >> >> Here's why: >> >> (a) The symptom does not affect the PEI phase -- by the time the UEFI >> application is executed, the PEI phase has ended; there's no need to >> modify the PEI instance of the library. >> >> (b) The currently proposed failure exits are too late. For example, in >> the SwitchBSPWorker() function, by the time we exit, we have called >> DisableApicTimerInterrupt(), SaveAndDisableInterrupts(), and >> DisableLvtInterrupts() -- and the error path does not restore the >> original environment. >> >> (c) According to the PI spec (v1.7), the StartupAllAPs(), >> StartupThisAP(), SwitchBSP(), EnableDisableAP() member functions of >> EFI_MP_SERVICES_PROTOCOL may only be called on the (current) BSP. >> Because of this, it is OK to call ValidCR3GdtIdtCheck() as the very >> first action in the above-listed DxeMpLib functions. >> >> (Assuming the protocol members are called from an AP, and consequently >> we check CR3 / GDTR / IDTR on the AP (and not on the BSP), that's the >> *caller's* fault, per spec!) > > This means we can move the ValidCr3GdtIdtCheck() function to > "DxeMpLib.c", and it is not necessary to modify "MpLib.h". > > Thanks > Laszlo > > > >