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 7C4B77803D0 for ; Mon, 19 Feb 2024 11:56:36 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=bZ0pscFmVl5WO3Q/YPqz5U/WlKOE1rjmZa6erDSAFa4=; 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=1708343795; v=1; b=VnOEUS/q69TcfaPoxeQXHkTyjg3l4NftuogefFTRUwQhDI3g6FtdV5h4KOONdZp4DgZXcAfT nO/bFA1FZY5ErraiQvmk2DPWru+LSsBpt7YKn5aylU4jtSNLDecUjKpPzTrmrUv9gfbwGzKODQ4 m5dSoLIBW8a/6Jh77x8414ec= X-Received: by 127.0.0.2 with SMTP id IjnWYY7687511xgxFcqwxzVI; Mon, 19 Feb 2024 03:56:35 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web11.39789.1708343794474895408 for ; Mon, 19 Feb 2024 03:56:34 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-301-RATdOGHDO8WHRjcz_o4EzA-1; Mon, 19 Feb 2024 06:56:31 -0500 X-MC-Unique: RATdOGHDO8WHRjcz_o4EzA-1 X-Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (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 4DB6D1C41A02; Mon, 19 Feb 2024 11:56:31 +0000 (UTC) X-Received: from [10.39.194.20] (unknown [10.39.194.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 845D5400D6D2; Mon, 19 Feb 2024 11:56:29 +0000 (UTC) Message-ID: Date: Mon, 19 Feb 2024 12:56:28 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 4/5] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to MpInitLibInitialize To: devel@edk2.groups.io, kraxel@redhat.com Cc: Oliver Steffen , Ray Ni , Rahul Kumar References: <20240215093149.251319-1-kraxel@redhat.com> <20240215093149.251319-5-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20240215093149.251319-5-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.2 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: n0tgD8Qn5EjMnzfMRn6nVmYnx7686176AA= 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="VnOEUS/q"; 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/15/24 10:31, 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 | 54 +++++++++++++++++++--------- > 1 file changed, 37 insertions(+), 17 deletions(-) >=20 > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/Mp= InitLib/MpLib.c > index 35f47d3b1289..1547b7e74a1a 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -2023,6 +2023,7 @@ MpInitLibInitialize ( > ) > { > MP_HAND_OFF *MpHandOff; > + BOOLEAN HaveMpHandOff; > CPU_INFO_IN_HOB *CpuInfoInHob; > UINT32 MaxLogicalProcessorNumber; > UINT32 ApStackSize; > @@ -2035,17 +2036,29 @@ MpInitLibInitialize ( > CPU_MP_DATA *CpuMpData; > UINT8 ApLoopMode; > UINT8 *MonitorBuffer; > - UINTN Index; > + UINTN Index, HobIndex; > UINTN ApResetVectorSizeBelow1Mb; > UINTN ApResetVectorSizeAbove1Mb; > UINTN BackupBufferAddr; > UINTN ApIdtBase; > =20 > - MpHandOff =3D GetMpHandOffHob (0); > - if (MpHandOff =3D=3D NULL) { > + MaxLogicalProcessorNumber =3D 0; > + HaveMpHandOff =3D FALSE; > + > + while ((MpHandOff =3D GetMpHandOffHob (MaxLogicalProcessorNumber)) != =3D 0) { > + DEBUG (( > + DEBUG_INFO, > + "%a: ProcessorIndex=3D%u CpuCount=3D%u\n", > + __func__, > + MpHandOff->ProcessorIndex, > + MpHandOff->CpuCount > + )); > + MaxLogicalProcessorNumber +=3D MpHandOff->CpuCount; > + HaveMpHandOff =3D TRUE; > + } > + > + if (!HaveMpHandOff) { > MaxLogicalProcessorNumber =3D PcdGet32 (PcdCpuMaxLogicalProcessorNum= ber); > - } else { > - MaxLogicalProcessorNumber =3D MpHandOff->CpuCount; > } > =20 > ASSERT (MaxLogicalProcessorNumber !=3D 0); How about: - just looping over the GUID HOBs, summing their CpuCount fields, - replacing the HaveMpHandOff variable with the sum being nonzero, after the loop (One extra assertion, on top, could be: while iterating over the GUID HOB list, also perform a maximum search for (MpHandOff->ProcessorIndex + MpHandOff->CpuCount), and after the loop, check that the maximum found like this equals the sum of CpuCount fields. But this is really an extra.) In other words -- again, I don't see any need for going through the HOBs in a particular order, just for summing their CpuCount fields. > @@ -2189,7 +2202,7 @@ MpInitLibInitialize ( > // > ProgramVirtualWireMode (); > =20 > - if (MpHandOff =3D=3D NULL) { > + if (!HaveMpHandOff) { > if (MaxLogicalProcessorNumber > 1) { > // > // Wakeup all APs and calculate the processor count in system > @@ -2205,19 +2218,26 @@ MpInitLibInitialize ( > AmdSevUpdateCpuMpData (CpuMpData); > } Hm, okay, considering this code, I do agree we need the "HaveMpHandOff" variable. Because, at this point, MaxLogicalProcessorNumber will be nonzero, regardless of how we calculated it it. > =20 > - CpuMpData->CpuCount =3D MpHandOff->CpuCount; > + CpuMpData->CpuCount =3D MaxLogicalProcessorNumber; > CpuMpData->BspNumber =3D GetBspNumber (); > CpuInfoInHob =3D (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInf= oInHob; > - for (Index =3D 0; Index < CpuMpData->CpuCount; Index++) { This original code here is a bit confused. Namely, after briefly auditing MpInitLibInitialize(), it seems like "Index" should always have been UINT32; making it UINTN is a confusing and unneeded generality. (But, if you disagree with my claim, please say so.) And the new variable HobIndex should be UINT32 too. > - InitializeSpinLock (&CpuMpData->CpuData[Index].ApLock); > - CpuMpData->CpuData[Index].CpuHealthy =3D (MpHandOff->Info[Index].H= ealth =3D=3D 0) ? TRUE : FALSE; Comment on the pre-patch code: Predicate ? TRUE : FALSE is poor style. > - 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 GetMpHandOffHob (0); > + MpHandOff !=3D NULL; > + MpHandOff =3D GetMpHandOffHob (MpHandOff->ProcessorIndex + MpHa= ndOff->CpuCount)) > + { > + 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; > + } > } Same observation as before: we don't need to *dicate* the HOB order for these nested loops, we can (and should) just process the HOBs in whatever order they appear. That makes the outer loop linear, rather than quadratic. > =20 > + MpHandOff =3D GetMpHandOffHob (0); > DEBUG ((DEBUG_INFO, "MpHandOff->WaitLoopExecutionMode: %04d, sizeof = (VOID *): %04d\n", MpHandOff->WaitLoopExecutionMode, sizeof (VOID *))); > if (MpHandOff->WaitLoopExecutionMode =3D=3D sizeof (VOID *)) { > ASSERT (CpuMpData->ApLoopMode !=3D ApInHltLoop); This indicates that a refactoring -- "normalization" -- of the MP_HAND_OFF structure is necessary. Namely, each MP_HAND_OFF HOB contains a WaitLoopExecutionMode field, but their values are expected to be identical. And here you have to use one of the HOBs (doesn't matter which one) for getting that value. The WaitLoopExecutionMode field should be a singleton, regardless of how many MP_HAND_OFF HOBs exist. It could be a separate GUID HOB, or perhaps (for simplicity?) a dynamic UINT32 PCD. ... In fact, the same seems to apply to the StartupSignalValue field. That field is only ever set to MP_HAND_OFF_SIGNAL, and that setting only depends on "CpuMpData->ApLoopMode". Therefore both WaitLoopExecutionMode and StartupSignalValue are singletons, and should not be duplicated; they should be "normalized out" to dynamic PCDs or a separate (unique) GUID HOB. > @@ -2284,7 +2304,7 @@ MpInitLibInitialize ( > // Wakeup APs to do some AP initialize sync (Microcode & MTRR) > // > if (CpuMpData->CpuCount > 1) { > - if (MpHandOff !=3D NULL) { > + if (HaveMpHandOff) { > // > // 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 > @@ -2301,7 +2321,7 @@ MpInitLibInitialize ( > CpuPause (); > } > =20 > - if (MpHandOff !=3D NULL) { > + if (HaveMpHandOff) { > CpuMpData->InitFlag =3D ApInitDone; > } > =20 Laszlo -=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 (#115594): https://edk2.groups.io/g/devel/message/115594 Mute This Topic: https://groups.io/mt/104369843/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-