From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=63.147.10.42; helo=atlmailgw2.ami.com; envelope-from=felixp@ami.com; receiver=edk2-devel@lists.01.org Received: from atlmailgw2.ami.com (atlmailgw2.ami.com [63.147.10.42]) (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 26DDF2118F79B for ; Wed, 5 Dec 2018 16:07:31 -0800 (PST) X-AuditID: ac10606f-741ff70000000b12-bb-5c086842713a Received: from atlms2.us.megatrends.com (atlms2.us.megatrends.com [172.16.96.152]) (using TLS with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (Client did not present a certificate) by atlmailgw2.ami.com (Symantec Messaging Gateway) with SMTP id 09.C0.02834.248680C5; Wed, 5 Dec 2018 19:07:30 -0500 (EST) Received: from ATLMS1.us.megatrends.com ([fe80::8c55:daf0:ef05:5605]) by atlms2.us.megatrends.com ([fe80::29dc:a91e:ea0c:cdeb%12]) with mapi id 14.03.0415.000; Wed, 5 Dec 2018 19:07:30 -0500 From: Felix Polyudov To: "'afish@apple.com'" CC: "Ni, Ruiyu" , "edk2-devel@lists.01.org" , "lersek@redhat.com" , "Dong, Eric" Thread-Topic: [edk2] [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP Thread-Index: AQHUiESqi1nXDN7s7uchhSAVeu0YhqVnrGJggACg0uCAAM8JgIAHuGNA Date: Thu, 6 Dec 2018 00:07:28 +0000 Message-ID: <9333E191E0D52B4999CE63A99BA663A00302C41807@atlms1.us.megatrends.com> References: <20181130003546.28252-1-felixp@ami.com> <734D49CCEBEEF84792F5B80ED585239D5BF35F8E@SHSMSX104.ccr.corp.intel.com> <9333E191E0D52B4999CE63A99BA663A00302C3FB5A@atlms1.us.megatrends.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [172.16.99.93] MIME-Version: 1.0 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrJKsWRmVeSWpSXmKPExsWyRiBhhq5TBkeMwewn1hbTbz9msdhz6Ciz xeYXwRbLju1gsXjZs5rdgdVj68kfbB6L97xk8uie/Y/F4/2+q2wBLFENjDaJeXn5JYklqQop qcXJtkoBRZllicmVSgqZKbZKhkoKBTmJyam5qXkltkqJBQWpeSlKdlwKGMAGqCwzTyE1Lzk/ JTMv3VbJM9hf18LC1FLXUMkuJCNVITMvLb8oN7EkMz9PITk/rwSoOjUFKKqQ0MWZcer1buaC p9uYK368X83awLislbmLkYNDQsBEYtHF+i5GTg4hgV1MEq2bTboYuYDsQ4wSD+YeZgNJsAmo Shxf3cwCYosIaErsWv2PFaSIWWAVo8T5C8uZQRLCAiES+/fshyoKlehaf5QJZIGIgJvE6YX8 IGEWARWJHfvegpXwCgRKHNgNMV9I4AejxMfXdiA2p4CtxNzZ35hAbEYBMYnvp9aA2cwC4hK3 nswHsyUEBCSW7DnPDGGLSrx8DHIPiK0gseV9JztEfb7E8+0fmSF2CUqcnPmEZQKjyCwko2Yh KZuFpAwiriOxYPcnNghbW2LZwtfMMPaZA4+ZkMUXMLKvYhRKLMnJTczMSS830kvMzdRLzs/d xAhJOfk7GD9+ND/EKMDBqMTDKxHNESPEmlhWXJkLDGIOZiURXgY9oBBvSmJlVWpRfnxRaU5q 8SFGJ2BwTWSW4gbFIzBhxBsbGEiJwjiGJmYm5kbmhpYm5sbGSuK8+WqfooQE0oEJLDs1tSC1 CGYIEwenVANjxbKT/+9WPs/ZfJFxojqP7o1/b5XONPL1pasZMBdobNo3a1XK0v9Hmw6YdCVU Zegunx/fdlR06ouriw2+NK01dDyt/D9a9k5XoW3Wqxk50TkJ6nv8Dx+K/NSwqOlfxet5l7xn rrsgvL/nYNjEGUfsXDbeDc+SmWl/bcryAwenhdzd9uaNeeK25UosxRmJhlrMRcWJAAc13Otc AwAA X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP 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: Thu, 06 Dec 2018 00:07:32 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Andrew, I think there are two aspects here: 1) What's the final solution we would like to see? I think DebugLibMpSafe and a fake PEI Services Table that you are proposing= are good ideas. Ideally, it should still be possible to use the real PEI Services table by o= verriding a library class. Please correct me if I'm wrong, but I believe it is legal to call PEI Servic= e from AP if all other CPUs (including BSP) are not-running (halted or sitti= ng in a spin-loop). 2) Should we do something short-term? In my opinion yes, because commit c563077a380437c1 breaks perhaps questionab= le but working production code. Additionally, as I've mentioned the library itself is broken because it uses= DebugLib, which is likely to use PEI Services. The patch I've submitted certainly does not provide a complete solution, but= it provides a workaround for the immediate issue. So I think it makes sense to apply the patch. Thanks Felix From: afish@apple.com [mailto:afish@apple.com] Sent: Friday, November 30, 2018 3:37 PM To: Felix Polyudov Cc: Ni, Ruiyu; edk2-devel@lists.01.org; lersek@redhat.com; Dong, Eric Subject: Re: [edk2] [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer= on AP Felix, I agree the obfuscation may not really be helpful. I've got a couple of thou= ghts. 1) I agree that I don't think the DebugLib is inherently safe on APs. I wond= er if we could make a library class that was DebugLibMpSafe and have the sam= e API as DebugLib. That way the library class matches how the module was cod= ed. 2) If we want to enforce the rules we should add code to the PEI or DXE Core= to CpuBreakpoint() etc. if an AP calls a core service. Adding detection code is possible, but it is not trivial. For example if you= have remembered the BSP and if the WhoAmI returns something different you n= eed to check to see if some one changed the BSP. I guess for PEI the other option could be to have a fake PEI Services Table= that stubs out all the functions with CpuBreakpoint() or some such? Thanks, Andrew Fish On Nov 30, 2018, at 6:33 AM, Felix Polyudov > wrote: Ray, I agree with the premise that calling PEI services from AP should generally= be avoided. However, the PEI services can be used on AP under certain special circumstan= ces. A couple of examples: 1. For debugging purposes. The MpInitLib contains 12 DEBUG calls and 19 ASSE= RT calls. Depending on the DebugLib instance used in the project, these call= s may lead to PEI services invocation. 2. MpInitLib provides ability to call AP in a serialized manner (only one AP= is running, other APs and BSP are waiting), when it is safe to call PEI ser= vices. Additionally, I think even if PEI services should not be used on AP, there i= s still a reason to keep PEI services table pointer initialized. On one hand, given the complexity of modern firmware projects comprised of m= odules coming from multiple vendors, making sure PEI services are not used o= n AP can be a challenge. For example, in my case the call was coming from the chipset reference code. On the other hand, with the current implementation, when somebody does try t= o use PEI services on AP the behavior is unpredictable. Depending on the content of the uninitialized PEI service table pointer, the= system may either crash with one of the handful of different exceptions, or it may start executing code from a random location. It's very difficult t= o debug such issues. One can spend weeks chasing a problem like this. -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ni, R= uiyu Sent: Thursday, November 29, 2018 10:43 PM To: Felix Polyudov; edk2-devel@lists.01.org Cc: lersek@redhat.com; Dong, Eric Subject: Re: [edk2] [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer= on AP Felix, I disagree:) Sorry about that. :) The commit you mentioned might be made by me (didn't checked). Because I aimed to avoid calling PEI services from AP. That's a violation of= PI spec and not safe by design. The AP calling standard services concern was raised by Andrew initially. Thanks, Ray -----Original Message----- From: Felix Polyudov [mailto:felixp@ami.com] Sent: Friday, November 30, 2018 8:36 AM To: edk2-devel@lists.01.org Cc: Dong, Eric >; Ni, Ruiyu= >; lersek@redhat.com Subject: [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP According to PI specification PEI Services table pointer is stored right bef= ore ITD base. Starting from commit c563077a380437c1 BSP and AP have different IDT instances. PEI Services table pointer was not initialized in the AP IDT instance. As a result, any attempt to use functions from PeiServicesTablePointerLib or PeiServicesLib on AP caused CPU exception. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Felix Polyudov --- UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c index 7f4d6e6..0e3e362 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c @@ -1567,6 +1567,7 @@ MpInitLibInitialize ( BufferSize =3D ApStackSize * MaxLogicalProcessorNumber; BufferSize +=3D MonitorFilterSize * MaxLogicalProcessorNumber; BufferSize +=3D ApResetVectorSize; + BufferSize +=3D sizeof(UINTN); BufferSize =3D ALIGN_VALUE (BufferSize, 8); BufferSize +=3D VolatileRegisters.Idtr.Limit + 1; BufferSize +=3D sizeof (CPU_MP_DATA); @@ -1587,6 +1588,8 @@ MpInitLibInitialize ( // Backup Buffer // +--------------------+ // Padding + // +--------------------+ + // PEI Services Table Pointer // +--------------------+ <-- ApIdtBase (8-byte boundary) // AP IDT All APs share one separate IDT. So AP can get= address of CPU_MP_DATA from IDT Base. // +--------------------+ <-- CpuMpData @@ -1599,7 +1602,7 @@ MpInitLibInitialize ( // MonitorBuffer =3D (UINT8 *) (Buffer + ApStackSize * MaxLogicalProcessorNumber); BackupBufferAddr =3D (UINTN) MonitorBuffer + MonitorFilterSize * MaxLogicalProcessorNumber; - ApIdtBase =3D ALIGN_VALUE (BackupBufferAddr + ApResetVectorSize, 8= ); + ApIdtBase =3D ALIGN_VALUE (BackupBufferAddr + ApResetVectorSize + sizeof(UINTN), 8); CpuMpData =3D (CPU_MP_DATA *) (ApIdtBase + VolatileRegisters.Idtr.L= imit + 1); CpuMpData->Buffer =3D Buffer; CpuMpData->CpuApStackSize =3D ApStackSize; @@ -1653,6 +1656,11 @@ MpInitLibInitialize ( Buffer + BufferSize); // + // Initialize PEI Services table pointer. Copy the address from BSP. + // + *(UINTN*)(ApIdtBase - sizeof(UINTN)) =3D + *(UINTN*)(VolatileRegisters.Idtr.Base - sizeof (UINTN)); + + // // Duplicate BSP's IDT to APs. // All APs share one separate IDT. So AP can get the address of CpuMpData= by using IDTR.BASE + IDTR.LIMIT + 1 // -- 2.10.0.windows.1 Please consider the environment before printing this email. The information contained in this message may be confidential and proprietar= y to American Megatrends, Inc. This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If= the reader of this message is not the intended recipient, you are on notice that= any distribution of this message, in any form, is strictly prohibited. Please p= romptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel Please consider the environment before printing this email. The information contained in this message may be confidential and proprietar= y to American Megatrends, Inc. This communication is intended to be read on= ly by the individual or entity to whom it is addressed or by their designee.= If the reader of this message is not the intended recipient, you are on not= ice that any distribution of this message, in any form, is strictly prohibit= ed. Please promptly notify the sender by reply e-mail or by telephone at 77= 0-246-8600, and then delete or destroy all copies of the transmission. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel Please consider the environment before printing this email. The information contained in this message may be confidential and proprietar= y to American Megatrends, Inc. This communication is intended to be read on= ly by the individual or entity to whom it is addressed or by their designee.= If the reader of this message is not the intended recipient, you are on not= ice that any distribution of this message, in any form, is strictly prohibit= ed. Please promptly notify the sender by reply e-mail or by telephone at 77= 0-246-8600, and then delete or destroy all copies of the transmission.