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.web10.6142.1579163754438627575 for ; Thu, 16 Jan 2020 00:35:54 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=erHfpOzU; 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=1579163753; 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=yVfO3o3IZwTuo58vxMRwj1zLn+ogKfw0lI40530wYUc=; b=erHfpOzU3Vb8X2Z8do78yUeaOyp7T2w6BgMB9cv2zaGO14cSeUHczwFW3X8dhjtjQGQofh n8vxUSMje/k/0BfcMIzvqFj7UwldxozXfUM853jlkTwoRnIiSQy09GFNHsx+UjXV9MLWaL OBjBWdOB25uCiuXK8f8FveIvTCVZtZA= 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-179-os2fPAUfOKSXbWN-1YR3NA-1; Thu, 16 Jan 2020 03:35:50 -0500 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AE789800D41; Thu, 16 Jan 2020 08:35:48 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-120.ams2.redhat.com [10.36.116.120]) by smtp.corp.redhat.com (Postfix) with ESMTP id AE6B619481; Thu, 16 Jan 2020 08:35:47 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption. To: "Dong, Eric" , "devel@edk2.groups.io" Cc: "Ni, Ray" References: <20200115060642.1707-1-eric.dong@intel.com> <15007a75-a43e-203d-86f1-8b6a46ca30c9@redhat.com> From: "Laszlo Ersek" Message-ID: <1b97c14d-78b7-bdfe-46a2-876a8f4495a5@redhat.com> Date: Thu, 16 Jan 2020 09:35:46 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-MC-Unique: os2fPAUfOKSXbWN-1YR3NA-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 01/16/20 04:15, Dong, Eric wrote: > Hi Laszlo, > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Wednesday, January 15, 2020 6:05 PM >> To: Dong, Eric ; devel@edk2.groups.io >> Cc: Ni, Ray >> Subject: Re: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 >> Assumption. >> >> On 01/15/20 07:06, Eric Dong wrote: >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2392 >>> >>> Current code implementation assumes BSP index is 0 at the begin. >>> This code change removes this assumption. It get BSP index from the >>> saved data structure if it existed. >>> >>> Cc: Ray Ni >>> Cc: Laszlo Ersek >>> Signed-off-by: Eric Dong >>> --- >>> UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++---- >>> 1 file changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c >>> b/UefiCpuPkg/Library/MpInitLib/MpLib.c >>> index 6ec9b172b8..922c87b766 100644 >>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c >>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c >>> @@ -636,7 +636,7 @@ ApWakeupFunction ( >>> // to initialize AP in InitConfig path. >>> // NOTE: IDTR.BASE stored in CpuMpData- >>> CpuData[0].VolatileRegisters points to a different IDT shared by all APs. >>> // >>> - RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, >> FALSE); >>> + RestoreVolatileRegisters >>> + (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, >>> + FALSE); >>> InitializeApData (CpuMpData, ProcessorNumber, BistData, >> ApTopOfStack); >>> ApStartupSignalBuffer = >>> CpuMpData->CpuData[ProcessorNumber].StartupApSignal; >>> >>> @@ -1615,6 +1615,7 @@ MpInitLibInitialize ( >>> UINTN ApResetVectorSize; >>> UINTN BackupBufferAddr; >>> UINTN ApIdtBase; >>> + UINT64 BspTopOfStack; >>> >>> OldCpuMpData = GetCpuMpDataFromGuidedHob (); >>> if (OldCpuMpData == NULL) { >>> @@ -1677,7 +1678,7 @@ MpInitLibInitialize ( >>> CpuMpData->BackupBufferSize = ApResetVectorSize; >>> CpuMpData->WakeupBuffer = (UINTN) -1; >>> CpuMpData->CpuCount = 1; >>> - CpuMpData->BspNumber = 0; >>> + CpuMpData->BspNumber = OldCpuMpData != NULL ? >> OldCpuMpData->BspNumber : 0; >>> CpuMpData->WaitEvent = NULL; >>> CpuMpData->SwitchBspFlag = FALSE; >>> CpuMpData->CpuData = (CPU_AP_DATA *) (CpuMpData + 1); >>> @@ -1704,11 +1705,12 @@ MpInitLibInitialize ( >>> // Don't pass BSP's TR to APs to avoid AP init failure. >>> // >>> VolatileRegisters.Tr = 0; >>> - CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, >>> &VolatileRegisters, sizeof (VolatileRegisters)); >>> + CopyMem >>> + (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, >>> + &VolatileRegisters, sizeof (VolatileRegisters)); >>> // >>> // Set BSP basic information >>> // >>> - InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + >>> ApStackSize); >>> + BspTopOfStack = CpuMpData->Buffer + (CpuMpData->BspNumber + 1) >> * >>> + CpuMpData->CpuApStackSize; InitializeApData (CpuMpData, >>> + CpuMpData->BspNumber, 0, BspTopOfStack); >>> // >>> // Save assembly code information >>> // >>> >> >> The patch seems reasonable to me (although I have not tried verifying that >> all necessary spots are updated). >> >> However, there is one thing I certainly don't understand, and the commit >> message doesn't explain it. In the "BspTopOfStack" calculation, why do we >> change the *second* factor, when we change the multiplication from: >> >> (0 + 1) * ApStackSize >> >> (where the (0 + 1) is implied in the old code), to: >> >> (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize >> >> ? >> >> I understand why the *first* factor is changed -- we basically replace "0" with >> "CpuMpData->BspNumber" --; what I don't understand is why we replace >> "ApStackSize" with "CpuMpData->CpuApStackSize", in the second factor. >> >> ... Higher up in the code, we have: >> >> CpuMpData->CpuApStackSize = ApStackSize; >> >> so this part of the patch might actually have no effect. But, even then, I think >> it makes the patch harder to understand. So in that case, I'd suggest sticking >> with "ApStackSize", just for keeping the patch simpler. >> > [[Eric]] driver has two places to call InitializeApData (). Here is one and the other in ApWakeupFunction(). > InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack); > At that function, it calculates the ApTopOfStack like below: > ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize; > > I update new code to follow this coding style. I think after this change, the exit two code pieces are follow > the same coding style. So I think we can keep my original change. That's fine, but then please include this specific argument in the commit message. Thanks, Laszlo