From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.5140.1579082678212931333 for ; Wed, 15 Jan 2020 02:04:38 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=fKt0622M; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1579082677; 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=aAjoEqeKSjd3B/mrbqURnarvjn3JzZ0fEaLfFvHm8A4=; b=fKt0622MvXIsi1MVLxRTyNS3vW3slqaFxQOtP0cFAKLBdznlCZbI0wuz2BrAyIZhlDxzNV doDeUBNWiH4UqVLfWdHWxkaGzNEewwZ/4Oo9Dw8v4/RBeYMD3SMyXQXhQAcFnsRt3yakAg /dM8o2yzLhc3wVfWKeyX23wwwxUwVtE= 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-390-oHWaHOV0OWypC_ifu7spXA-1; Wed, 15 Jan 2020 05:04:33 -0500 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 91BE18E4F5C; Wed, 15 Jan 2020 10:04:32 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-98.ams2.redhat.com [10.36.116.98]) by smtp.corp.redhat.com (Postfix) with ESMTP id 94E4F80617; Wed, 15 Jan 2020 10:04:31 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption. To: Eric Dong , devel@edk2.groups.io Cc: Ray Ni References: <20200115060642.1707-1-eric.dong@intel.com> From: "Laszlo Ersek" Message-ID: <15007a75-a43e-203d-86f1-8b6a46ca30c9@redhat.com> Date: Wed, 15 Jan 2020 11:04:30 +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: <20200115060642.1707-1-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-MC-Unique: oHWaHOV0OWypC_ifu7spXA-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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. Thanks Laszlo