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.40; helo=atlmailgw1.ami.com; envelope-from=felixp@ami.com; receiver=edk2-devel@lists.01.org Received: from atlmailgw1.ami.com (atlmailgw1.ami.com [63.147.10.40]) (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 596EA21197363 for ; Fri, 30 Nov 2018 06:33:35 -0800 (PST) X-AuditID: ac1060b2-0e1ff70000000d42-68-5c014a3e0e5b 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 atlmailgw1.ami.com (Symantec Messaging Gateway) with SMTP id 66.22.03394.E3A410C5; Fri, 30 Nov 2018 09:33:35 -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; Fri, 30 Nov 2018 09:33:34 -0500 From: Felix Polyudov To: "'Ni, Ruiyu'" , "edk2-devel@lists.01.org" CC: "lersek@redhat.com" , "Dong, Eric" Thread-Topic: [Patch] UefiCpuPkg/MpLib: Fix PEI Services Table pointer on AP Thread-Index: AQHUiESqi1nXDN7s7uchhSAVeu0YhqVnrGJggACg0uA= Date: Fri, 30 Nov 2018 14:33:33 +0000 Message-ID: <9333E191E0D52B4999CE63A99BA663A00302C3FB5A@atlms1.us.megatrends.com> References: <20181130003546.28252-1-felixp@ami.com> <734D49CCEBEEF84792F5B80ED585239D5BF35F8E@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5BF35F8E@SHSMSX104.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [172.16.99.93] content-transfer-encoding: quoted-printable MIME-Version: 1.0 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrGKsWRmVeSWpSXmKPExsWyRiBhhq69F2OMwYLdbBZ7Dh1lttj8Ithi 2bEdLBYve1azO7B4LN7zksmje/Y/Fo/3+66yBTBHNTDaJObl5ZcklqQqpKQWJ9sqBRRlliUm VyopZKbYKhkqKRTkJCan5qbmldgqJRYUpOalKNlxKWAAG6CyzDyF1Lzk/JTMvHRbJc9gf10L C1NLXUMlu5CMVIXMvLT8otzEksz8PIXk/LwSoOrUFKCoQkIXZ8byfa/ZCmbrV6zd8Za1gfGu ahcjJ4eEgInE2nPfWLoYuTiEBHYxSZycPIEdwjnMKPH6wlxWkCo2AVWJ46ubWUBsEYFYiadf G8HizAKBEk+bFjKB2MIC3hIn5y6HqvGRuHjmNJRtJfHq3hI2EJsFaM7B2bvYQWxeoN4dl9eC 1QgJ1Ep09H9j7mLk4OAUCJGY+oMDJMwoICbx/dQaJohV4hK3nsxngjhaQGLJnvPMELaoxMvH /1ghbAWJLe872SHqdSQW7P7EBmFrSyxb+JoZYq2gxMmZT1gmMIrOQjJ2FpKWWUhaZiFpWcDI sopRKLEkJzcxMye93FAvMTdTLzk/dxMjJFls2sHYctH8EKMAB6MSD+8kR8YYIdbEsuLK3EOM EhzMSiK859oYYoR4UxIrq1KL8uOLSnNSiw8xOgFDZSKzFDcouoDxH29sYCAlCuMYmpiZmBuZ G1qamBsbK4nz5qt9ihISSAemo+zU1ILUIpghTBycUg2MQu9m1nwRW/BSNvkWYwb7m7T+fSmf 1cxYlprPz89sdN0f5f63KPSo9f807a41zF8fvBbxsMw2sas22eQQ8Z6px9/q6ZSJFjJ2ibNY pjg+3Xi6MPXkOQ3u0KU6ye37s/K+8bc75nZt45leWbR4Ci+/om2QovXty1tfl7icfe7OH3Ju Y+nj3d5KLMUZiYZazEXFiQB5RR71OQMAAA== 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: Fri, 30 Nov 2018 14:33:36 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" 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 b= efore 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.Idt= r.Limit + > 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 CpuMpDa= ta 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 propriet= ary > 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 notice th= at any > distribution of this message, in any form, is strictly prohibited. Please= promptly > notify the sender by reply e-mail or by telephone at 770-246-8600, and the= n > 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.