From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.93; helo=mga11.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (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 B4224203B8CE6 for ; Sun, 1 Jul 2018 22:56:00 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 01 Jul 2018 22:56:00 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,298,1526367600"; d="scan'208";a="71539945" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.4]) ([10.239.9.4]) by orsmga002.jf.intel.com with ESMTP; 01 Jul 2018 22:55:58 -0700 To: Laszlo Ersek , "edk2-devel@lists.01.org" Cc: Jeff Fan , "Dong, Eric" , "Yao, Jiewen" , Fish Andrew References: <20180629104201.253196-1-ruiyu.ni@intel.com> <0fe484e0-2c7c-8050-1375-58099b0ef06f@redhat.com> From: "Ni, Ruiyu" Message-ID: Date: Mon, 2 Jul 2018 13:56:21 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <0fe484e0-2c7c-8050-1375-58099b0ef06f@redhat.com> Subject: Re: [PATCH v3] UefiCpuPkg/MpInitLib: Avoid calling PEI services from AP X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 02 Jul 2018 05:56:00 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 6/30/2018 3:25 AM, Laszlo Ersek wrote: > (1) I think that, right after the above line, you are missing: > > BufferSize = ALIGN_VALUE (BufferSize, 8); Yes. That's a bug in the patch. I will fix it in V4. > > It is not noticed in practice because > > AllocatePages (EFI_SIZE_TO_PAGES (BufferSize)) > > rounds up the allocation anyway. However, the above ALIGN_VALUE() would > be necessary to remain consistent with the ALIGN_VALUE() that is used > below, in the assignment to "ApIdtBase". > >> + BufferSize += VolatileRegisters.Idtr.Limit + 1; >> + BufferSize += sizeof (CPU_MP_DATA); >> BufferSize += (sizeof (CPU_AP_DATA) + sizeof (CPU_INFO_IN_HOB))* MaxLogicalProcessorNumber; >> MpBuffer = AllocatePages (EFI_SIZE_TO_PAGES (BufferSize)); >> ASSERT (MpBuffer != NULL); >> ZeroMem (MpBuffer, BufferSize); >> Buffer = (UINTN) MpBuffer; >> >> + /* >> + The layout of the Buffer is as below: >> + >> + +--------------------+ <-- Buffer >> + AP Stacks (N) >> + +--------------------+ <-- MonitorBuffer >> + AP Monitor Filters (N) >> + +--------------------+ <-- BackupBuffer >> + Backup Buffer >> + +--------------------+ <-- 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 >> + CPU_MP_DATA >> + +--------------------+ <-- CpuMpData->CpuData >> + CPU_AP_DATA (N) >> + +--------------------+ <-- CpuMpData->CpuInfoHob >> + CPU_INFO_IN_HOB (N) >> + +--------------------+ >> + >> + */ > Some remarks for this comment block: > > (2) We should likely use the "//" comment style. OK. > > (3) The "BackupBuffer" comment should be "CpuMpData->BackupBuffer". We > don't have a "BackupBuffer" local variable, only "BackupBufferAddr". OK. > > (4) "CpuMpData->CpuInfoHob" is a typo; it should be > "CpuMpData->CpuInfoInHob". OK. > >> MonitorBuffer = (UINT8 *) (Buffer + ApStackSize * MaxLogicalProcessorNumber); >> BackupBufferAddr = (UINTN) MonitorBuffer + MonitorFilterSize * MaxLogicalProcessorNumber; >> - CpuMpData = (CPU_MP_DATA *) (BackupBufferAddr + ApResetVectorSize); >> + ApIdtBase = ALIGN_VALUE (BackupBufferAddr + ApResetVectorSize, 8); >> + CpuMpData = (CPU_MP_DATA *) (ApIdtBase + VolatileRegisters.Idtr.Limit + 1); >> CpuMpData->Buffer = Buffer; >> CpuMpData->CpuApStackSize = ApStackSize; >> CpuMpData->BackupBuffer = BackupBufferAddr; >> @@ -1557,10 +1586,14 @@ MpInitLibInitialize ( >> CpuMpData->MicrocodePatchAddress = PcdGet64 (PcdCpuMicrocodePatchAddress); >> CpuMpData->MicrocodePatchRegionSize = PcdGet64 (PcdCpuMicrocodePatchRegionSize); >> InitializeSpinLock(&CpuMpData->MpLock); >> + > (5) At this point, I suggest a self-check, something like: > > ASSERT ((CpuMpData->CpuInfoInHob + > sizeof (CPU_INFO_IN_HOB) * MaxLogicalProcessorNumber) == > Buffer + BufferSize); > OK. >> // >> - // Save BSP's Control registers to APs >> + // 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 >> // >> - SaveVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters); >> + CopyMem ((VOID *)ApIdtBase, (VOID *)VolatileRegisters.Idtr.Base, VolatileRegisters.Idtr.Limit + 1); >> + VolatileRegisters.Idtr.Base = ApIdtBase; >> + CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters)); >> // >> // Set BSP basic information >> // > This is my main concern. > > The first CopyMem() correctly populates the IDT that is supposed to be > shared by all the APs, from the BSP's IDT. > > Then, we modify "VolatileRegisters" (which is never used in the function > beyond the second CopyMem()), so that the IDTR base points to the APs' > shared IDT. OK. > > Then, with the second CopyMem(), we put the*modified* VolatileRegisters > into CpuData[0]. This replaces the original SaveVolatileRegisters() > call, and as a result, the CpuData[0] IDTR base will now point to the > APs' shared IDT, and not the BSPs own IDT. > > (6) Is this not a problem for the BSP itself? > > I see three explicit uses of CpuData[0] in the code, namely: > > (6a) The one added above by the patch. > > (6b) Lower down in the same MpInitLibInitialize() function: > > CopyMem ( > &CpuMpData->CpuData[Index].VolatileRegisters, > &CpuMpData->CpuData[0].VolatileRegisters, > sizeof (CPU_VOLATILE_REGISTERS) > ); > > I think this is fine, for all (Index > 0) values. We could as well use > the local "VolatileRegisters" here, as source argument. OK. > > (6c) And in ApWakeupFunction(): > > // > // Sync BSP's Control registers to APs > // > RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE); > > Minimally, we should update the comment to: > > // > // Sync BSP's Control registers to APs. Note that IDTR.BASE is different. > // > > Which means that, from now on, we generally consider CpuData[0] to stand > for the BSP,*except* IDTR.BASE. (I guess we could redefine CpuData[0], > not as "BSP", but as "initializer for APs > > Thus, are we sure that the BSP never uses CpuData[0].IDTR.BASE for its > own purposes? > > Should we perhaps: > - keep CpuData[0] unchanged, > - introduce a new, dedicated field, "CpuMpData->ApIdtBase", and use that > *explicitly* wherever necessary? Then we will make the RestoreVolatileRegisters() a bit complex. It needs to explicitly restores the IDT. I will add more comments for CpuData[0].VolatileRegisters. > > > (7) My next question is a corollary of the above -- what about > SwitchBSP()? > > - Is SwitchBSP() compatible with this design? Yes. > - Will the new BSP start using the old BSP's IDT? Yes. > - More importantly, will the old BSP start using the IDT that is shared > by the APs? Yes. 1). AsmExchangeRole () exchanges the IDT of BSP and AP. 2). After returning from AsmExchangeRole(), old BSP runs in ApWakeupFunction(). It saves IDTR value using below call: SaveVolatileRegisters(&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters); It guarantees next time when it's waken up, IDTR is restored using below call: RestoreVolatileRegisters(&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE); ProcessorNumber is 0 after first SwitchBSP() call. Please don't worry about your (6c) restore operation. That only happens in InitConfig path. > > Without these, I think GetCpuMpData() would break. The new BSP would go > to the HOB, which is OK. However, the old BSP (= new AP) would go to the > *original* IDTR, and that's not usable for getting CpuMpData. > > (I don't know how CpuData[0] relates to the*current* -- possibly > switched -- BSP.) > > Thank you! > Laszlo > -- Thanks, Ray