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 45C5D81F08 for ; Thu, 17 Nov 2016 02:01:03 -0800 (PST) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0A00165729; Thu, 17 Nov 2016 10:01:08 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-6.phx2.redhat.com [10.3.116.6]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAHA16Wn001228; Thu, 17 Nov 2016 05:01:07 -0500 To: "Fan, Jeff" , edk2-devel-01 References: <20161117001754.4383-1-lersek@redhat.com> <20161117001754.4383-5-lersek@redhat.com> <542CF652F8836A4AB8DBFAAD40ED192A4A2DFD98@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: Date: Thu, 17 Nov 2016 11:01:06 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <542CF652F8836A4AB8DBFAAD40ED192A4A2DFD98@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 17 Nov 2016 10:01:08 +0000 (UTC) Subject: Re: [PATCH 4/4] UefiCpuPkg/MpInitLib: support 64-bit AP stack addresses X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Nov 2016 10:01:03 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 11/17/16 02:18, Fan, Jeff wrote: > Laszlo, > > We have two solutions to fix stack > 4G issue. > 1. Allocate AP stack buffer and all CPU MP data buffer under < 4G at the beginning. > 2. Support AP stack buffer and all CPU MP data buffer > 4G as showed in your patch. > > For 1), it seems not necessary. > For 2), besides your patch. We still need to update RelocateApLoop() in DxeMpLib.c to use one separate stack under 4G when paging disabled on long mode DXE. > (Currently, we still use AP existing stack after paging disabled) > I prefer the 2), please go ahead to check-in this serial of patch. I will create another patch to fix RelocateApLoop() stack issue. > > Reviewed-by: Jeff Fan > Thank you, series committed as 97d2760429d6..dd3fa0cd72de. Cheers Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, November 17, 2016 8:18 AM > To: edk2-devel-01 > Cc: Fan, Jeff > Subject: [PATCH 4/4] UefiCpuPkg/MpInitLib: support 64-bit AP stack addresses > > The cached "CPU_INFO_IN_HOB.ApTopOfStack" field currently has type UINT32. > This is not ideal because the AP stacks are located within "CpuMpData->Buffer", which is allocated with a plain AllocatePages() call in MpInitLibInitialize(): > > platform CpuMpPei included PEI RAM > 4GB result > -------- ----------------- ------------- ------ > Ia32 * n/a good > Ia32X64 no n/a BAD > Ia32X64 yes n/a good > X64 no * BAD > X64 yes no good > X64 yes yes BAD > > - If we are on an Ia32X64 or X64 platform that does not include CpuMpPei, > then CpuDxe cannot reuse the CPU_INFO_IN_HOB structures preallocated by > CpuMpPei (through the CpuInitMpLib GUID HOB), and then AllocatePages() > -- invoked first in 64-bit DXE -- could return an address outside of > 32-bit address space. > > - If we are on an X64 platform where the permanent PEI RAM extends above > the 32-bit address space, then the same issue can surface even if > CpuMpPei is included: even the original allocation of the > CPU_INFO_IN_HOB structures, by CpuMpPei, could be satisfied from above > 4GB. > > The original "AP init" branch in "X64/MpFuncs.nasm" correctly considers a 64-bit stack start: the "MP_CPU_EXCHANGE_INFO.StackStart" field has type UINTN, and the code uses QWORD addition and movement to set RSP from it. > > Adapt the "GetApicId" branch of "X64/MpFuncs.nasm": > > - change the type of "CPU_INFO_IN_HOB.ApTopOfStack" to UINT64, > > - remove the explicit truncation to UINT32 in InitializeApData(), > > - update the "GetNextProcNumber" iteration size to the new size of > "CPU_INFO_IN_HOB", > > - set RSP with a QWORD movement from "CPU_INFO_IN_HOB.ApTopOfStack". > > Because the same CPU_INFO_IN_HOB structure is used by "Ia32/MpFuncs.nasm", we have to update the "GetNextProcNumber" iteration size there as well. > The ESP setting can be preserved as a DWORD movement from the original offset (decimal 12), since our integers are little endian. > > Cc: Jeff Fan > Fixes: 845c5be1fd9bf7edfac4a103dfab70829686978f > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek > --- > UefiCpuPkg/Library/MpInitLib/MpLib.h | 4 +++- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 8 ++++---- > UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 2 +- UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 5 ++--- > 4 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h > index 0ac777a099b1..f73a469ae84f 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > @@ -126,18 +126,20 @@ typedef struct { > // > // Basic CPU information saved in Guided HOB. > // Because the contents will be shard between PEI and DXE, // we need to make sure the each fields offset same in different // architecture. > // > +#pragma pack (1) > typedef struct { > UINT32 InitialApicId; > UINT32 ApicId; > UINT32 Health; > - UINT32 ApTopOfStack; > + UINT64 ApTopOfStack; > } CPU_INFO_IN_HOB; > +#pragma pack () > > // > // AP reset code information including code address and size, // this structure will be shared be C code and assembly code. > // It is natural aligned by design. > // > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 3c2e6d6b89d9..15dbfa1e7d6c 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -430,22 +430,22 @@ CollectProcessorCount ( **/ VOID InitializeApData ( > IN OUT CPU_MP_DATA *CpuMpData, > IN UINTN ProcessorNumber, > IN UINT32 BistData, > - IN UINTN ApTopOfStack > + IN UINT64 ApTopOfStack > ) > { > CPU_INFO_IN_HOB *CpuInfoInHob; > > CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob; > CpuInfoInHob[ProcessorNumber].InitialApicId = GetInitialApicId (); > CpuInfoInHob[ProcessorNumber].ApicId = GetApicId (); > CpuInfoInHob[ProcessorNumber].Health = BistData; > - CpuInfoInHob[ProcessorNumber].ApTopOfStack = (UINT32) ApTopOfStack; > + CpuInfoInHob[ProcessorNumber].ApTopOfStack = ApTopOfStack; > > CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE; > CpuMpData->CpuData[ProcessorNumber].CpuHealthy = (BistData == 0) ? TRUE : FALSE; > if (CpuInfoInHob[ProcessorNumber].InitialApicId >= 0xFF) { > // > // Set x2APIC mode if there are any logical processor reporting @@ -477,13 +477,13 @@ ApWakeupFunction ( > UINTN ProcessorNumber; > EFI_AP_PROCEDURE Procedure; > VOID *Parameter; > UINT32 BistData; > volatile UINT32 *ApStartupSignalBuffer; > CPU_INFO_IN_HOB *CpuInfoInHob; > - UINTN ApTopOfStack; > + UINT64 ApTopOfStack; > > // > // AP finished assembly code and begin to execute C code > // > CpuMpData = ExchangeInfo->CpuMpData; > > @@ -497,13 +497,13 @@ ApWakeupFunction ( > InterlockedIncrement ((UINT32 *) &CpuMpData->CpuCount); > ProcessorNumber = NumApsExecuting; > // > // This is first time AP wakeup, get BIST information from AP stack > // > ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize; > - BistData = *(UINT32 *) (ApTopOfStack - sizeof (UINTN)); > + BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN)); > // > // Do some AP initialize sync > // > ApInitializeSync (CpuMpData); > // > // Sync BSP's Control registers to APs diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > index 4bfa084c85a9..64e51d87ae24 100644 > --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > @@ -178,13 +178,13 @@ GetProcessorNumber: > lea eax, [esi + CpuInfoLocation] > mov edi, [eax] > > GetNextProcNumber: > cmp [edi], edx ; APIC ID match? > jz ProgramStack > - add edi, 16 > + add edi, 20 > inc ebx > jmp GetNextProcNumber > > ProgramStack: > mov esp, [edi + 12] > > diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > index 138b97312b1d..aaabb50c5468 100644 > --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > @@ -182,19 +182,18 @@ GetProcessorNumber: > lea eax, [esi + CpuInfoLocation] > mov edi, [eax] > > GetNextProcNumber: > cmp dword [edi], edx ; APIC ID match? > jz ProgramStack > - add edi, 16 > + add edi, 20 > inc ebx > jmp GetNextProcNumber > > ProgramStack: > - xor rsp, rsp > - mov esp, dword [edi + 12] > + mov rsp, qword [edi + 12] > > CProcedureInvoke: > push rbp ; Push BIST data at top of AP stack > xor rbp, rbp ; Clear ebp for call stack trace > push rbp > mov rbp, rsp > -- > 2.9.2 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >