From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 452AB940F6B for ; Thu, 22 Feb 2024 01:12:05 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=8yHoXpkZwqfcriDz1Il4e1HBXFxjkGVry3eoF18YCow=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1708564323; v=1; b=alHJ38PpdpmEQnVCWiPnttn1z2kYh9Xsg9dbRbjjhP5ctnm2ZG2k4FW0XZCBV7ExUC+8qHA2 K2vv3VvRvt/nzoIgjfPgk4GF0zAdvdOaE4fBK2S50lR1H2HDQ71OkJxeqJO2nzWTJ8XyKAUbZW4 zxIOUvNdCNSXc8gCuBksEgXg= X-Received: by 127.0.0.2 with SMTP id E2QjYY7687511xRhmMFma0ym; Wed, 21 Feb 2024 17:12:03 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.3145.1708564323221788482 for ; Wed, 21 Feb 2024 17:12:03 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-660-KIaEkTXrNDa0Ei0V5KBWhQ-1; Wed, 21 Feb 2024 20:12:00 -0500 X-MC-Unique: KIaEkTXrNDa0Ei0V5KBWhQ-1 X-Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 27D44811E79; Thu, 22 Feb 2024 01:12:00 +0000 (UTC) X-Received: from [10.39.192.46] (unknown [10.39.192.46]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E8854112132A; Thu, 22 Feb 2024 01:11:58 +0000 (UTC) Message-ID: Date: Thu, 22 Feb 2024 02:11:57 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2 4/5] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to MpInitLibInitialize To: devel@edk2.groups.io, kraxel@redhat.com Cc: Oliver Steffen , Rahul Kumar , Ray Ni References: <20240220174939.1288689-1-kraxel@redhat.com> <20240220174939.1288689-5-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20240220174939.1288689-5-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 06oRwKSLaghbywgZx2mU44LTx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=alHJ38Pp; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 2/20/24 18:49, Gerd Hoffmann wrote: > Loop over all MP_HAND_OFF HOBs instead of expecting a single HOB > covering all CPUs in the system. >=20 > Add a new HaveMpHandOff variable to track whenever MP_HAND_OFF HOBs are > present, using the MpHandOff pointer for that does not work because the > variable will be NULL after looping over all HOBs. >=20 > Signed-off-by: Gerd Hoffmann > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 68 +++++++++++++++++++--------- > 1 file changed, 47 insertions(+), 21 deletions(-) as Ray says, the commit message is a bit stale >=20 > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/Mp= InitLib/MpLib.c > index c13de34e5769..80585f676b1d 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -2025,6 +2025,7 @@ MpInitLibInitialize ( > VOID > ) > { > + MP_HAND_OFF *FirstMpHandOff; > MP_HAND_OFF *MpHandOff; > CPU_INFO_IN_HOB *CpuInfoInHob; > UINT32 MaxLogicalProcessorNumber; > @@ -2038,17 +2039,31 @@ MpInitLibInitialize ( > CPU_MP_DATA *CpuMpData; > UINT8 ApLoopMode; > UINT8 *MonitorBuffer; > - UINTN Index; > + UINT32 Index, HobIndex; > UINTN ApResetVectorSizeBelow1Mb; > UINTN ApResetVectorSizeAbove1Mb; > UINTN BackupBufferAddr; > UINTN ApIdtBase; > =20 > - MpHandOff =3D GetNextMpHandOffHob (NULL); > - if (MpHandOff =3D=3D NULL) { > + FirstMpHandOff =3D GetNextMpHandOffHob (NULL); > + if (FirstMpHandOff !=3D NULL) { > + MaxLogicalProcessorNumber =3D 0; > + for (MpHandOff =3D FirstMpHandOff; > + MpHandOff !=3D NULL; > + MpHandOff =3D GetNextMpHandOffHob (MpHandOff)) > + { > + DEBUG (( > + DEBUG_INFO, > + "%a: ProcessorIndex=3D%u CpuCount=3D%u\n", > + __func__, > + MpHandOff->ProcessorIndex, > + MpHandOff->CpuCount > + )); > + ASSERT (MaxLogicalProcessorNumber =3D=3D MpHandOff->ProcessorIndex= ); > + MaxLogicalProcessorNumber +=3D MpHandOff->CpuCount; > + } > + } else { > MaxLogicalProcessorNumber =3D PcdGet32 (PcdCpuMaxLogicalProcessorNum= ber); > - } else { > - MaxLogicalProcessorNumber =3D MpHandOff->CpuCount; > } > =20 > ASSERT (MaxLogicalProcessorNumber !=3D 0); > @@ -2192,7 +2207,7 @@ MpInitLibInitialize ( > // > ProgramVirtualWireMode (); > =20 > - if (MpHandOff =3D=3D NULL) { > + if (FirstMpHandOff =3D=3D NULL) { > if (MaxLogicalProcessorNumber > 1) { > // > // Wakeup all APs and calculate the processor count in system > @@ -2208,21 +2223,32 @@ MpInitLibInitialize ( > AmdSevUpdateCpuMpData (CpuMpData); > } > =20 > - CpuMpData->CpuCount =3D MpHandOff->CpuCount; > - CpuMpData->BspNumber =3D GetBspNumber (MpHandOff); > + CpuMpData->CpuCount =3D MaxLogicalProcessorNumber; > + CpuMpData->BspNumber =3D GetBspNumber (FirstMpHandOff); > CpuInfoInHob =3D (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInf= oInHob; > - for (Index =3D 0; Index < CpuMpData->CpuCount; Index++) { > - InitializeSpinLock (&CpuMpData->CpuData[Index].ApLock); > - CpuMpData->CpuData[Index].CpuHealthy =3D (MpHandOff->Info[Index].H= ealth =3D=3D 0) ? TRUE : FALSE; > - CpuMpData->CpuData[Index].ApFunction =3D 0; > - CpuInfoInHob[Index].InitialApicId =3D MpHandOff->Info[Index].Ap= icId; > - CpuInfoInHob[Index].ApTopOfStack =3D CpuMpData->Buffer + (Inde= x + 1) * CpuMpData->CpuApStackSize; > - CpuInfoInHob[Index].ApicId =3D MpHandOff->Info[Index].Ap= icId; > - CpuInfoInHob[Index].Health =3D MpHandOff->Info[Index].He= alth; > + for (MpHandOff =3D FirstMpHandOff; > + MpHandOff !=3D NULL; > + MpHandOff =3D GetNextMpHandOffHob (MpHandOff)) > + { > + for (HobIndex =3D 0; HobIndex < MpHandOff->CpuCount; HobIndex++) { > + Index =3D MpHandOff->ProcessorIndex + HobIndex; > + InitializeSpinLock (&CpuMpData->CpuData[Index].ApLock); > + CpuMpData->CpuData[Index].CpuHealthy =3D (MpHandOff->Info[HobInd= ex].Health =3D=3D 0) ? TRUE : FALSE; > + CpuMpData->CpuData[Index].ApFunction =3D 0; > + CpuInfoInHob[Index].InitialApicId =3D MpHandOff->Info[HobInde= x].ApicId; > + CpuInfoInHob[Index].ApTopOfStack =3D CpuMpData->Buffer + (In= dex + 1) * CpuMpData->CpuApStackSize; > + CpuInfoInHob[Index].ApicId =3D MpHandOff->Info[HobInde= x].ApicId; > + CpuInfoInHob[Index].Health =3D MpHandOff->Info[HobInde= x].Health; > + } > } > =20 > - DEBUG ((DEBUG_INFO, "MpHandOff->WaitLoopExecutionMode: %04d, sizeof = (VOID *): %04d\n", MpHandOff->WaitLoopExecutionMode, sizeof (VOID *))); > - if (MpHandOff->WaitLoopExecutionMode =3D=3D sizeof (VOID *)) { > + DEBUG (( > + DEBUG_INFO, > + "FirstMpHandOff->WaitLoopExecutionMode: %04d, sizeof (VOID *): %04= d\n", > + FirstMpHandOff->WaitLoopExecutionMode, > + sizeof (VOID *) > + )); > + if (FirstMpHandOff->WaitLoopExecutionMode =3D=3D sizeof (VOID *)) { > ASSERT (CpuMpData->ApLoopMode !=3D ApInHltLoop); > =20 > CpuMpData->FinishedCount =3D 0; The code looks otherwise OK, but I'm not happy that WaitLoopExecutionMode (and StartupSignalValue) are replicated over all the HOBs, just like in v1. IMO, that will only make it harder for others to understand the code / data structures, and therefore it increases technical debt. I understand that Ray is OK with that, so I won't try to block the patch, but I'm not comfortable giving it an R-b myself, due to the increase in technical debt. Laszlo > @@ -2238,7 +2264,7 @@ MpInitLibInitialize ( > // enables the APs to switch to a different memory section and con= tinue their > // looping process there. > // > - SwitchApContext (MpHandOff); > + SwitchApContext (FirstMpHandOff); > // > // Wait for all APs finished initialization > // > @@ -2287,7 +2313,7 @@ MpInitLibInitialize ( > // Wakeup APs to do some AP initialize sync (Microcode & MTRR) > // > if (CpuMpData->CpuCount > 1) { > - if (MpHandOff !=3D NULL) { > + if (FirstMpHandOff !=3D NULL) { > // > // Only needs to use this flag for DXE phase to update the wake up > // buffer. Wakeup buffer allocated in PEI phase is no longer valid > @@ -2304,7 +2330,7 @@ MpInitLibInitialize ( > CpuPause (); > } > =20 > - if (MpHandOff !=3D NULL) { > + if (FirstMpHandOff !=3D NULL) { > CpuMpData->InitFlag =3D ApInitDone; > } > =20 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115765): https://edk2.groups.io/g/devel/message/115765 Mute This Topic: https://groups.io/mt/104472311/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-