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 A27A4D8030B for ; Mon, 19 Feb 2024 12:50:16 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=zONBspuYl4L/EirMj1d448zs2sVwBeq3BI45N4A1aoM=; 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=1708347015; v=1; b=AHoA56c20IeNiFMqOEGm1Oks9nU3R8dpxDe5DVYlDsOzAyn/VeISxv3P5ydXRppIXNN9B++R QvtXwXZyFNUubIOI1wwp8ga0YWqlKyeP/+0C+AB5fe4LIHmLbchRWFV6soltSS9/ziVUzbTQXEV q0rb4cuBOQenNsPqjR9ichHo= X-Received: by 127.0.0.2 with SMTP id GPZcYY7687511x4D90v60ItG; Mon, 19 Feb 2024 04:50:15 -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.web10.40281.1708347014535525456 for ; Mon, 19 Feb 2024 04:50:14 -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-645--xnN4JtxOQaySXqhPB9vyA-1; Mon, 19 Feb 2024 07:50:07 -0500 X-MC-Unique: -xnN4JtxOQaySXqhPB9vyA-1 X-Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (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 50AC41C41A08; Mon, 19 Feb 2024 12:50:07 +0000 (UTC) X-Received: from [10.39.194.20] (unknown [10.39.194.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 282C6C01644; Mon, 19 Feb 2024 12:50:06 +0000 (UTC) Message-ID: <6a054f43-913d-93a9-02ac-3be81d443761@redhat.com> Date: Mon, 19 Feb 2024 13:50:05 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 5/5] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData() To: devel@edk2.groups.io, kraxel@redhat.com Cc: Oliver Steffen , Ray Ni , Rahul Kumar References: <20240215093149.251319-1-kraxel@redhat.com> <20240215093149.251319-6-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20240215093149.251319-6-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.8 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: vgLvhhvBb8eWHMN6DyoEfGnDx7686176AA= 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=AHoA56c2; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) On 2/15/24 10:31, Gerd Hoffmann wrote: > Add support for splitting Hand-Off data into multiple HOBs. This is > required for VMs with thousands of CPUs. The actual CPU count per HOB > is much smaller (128) for better test coverage. > > Signed-off-by: Gerd Hoffmann > --- > UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 48 ++++++++++++++----------- > 1 file changed, 27 insertions(+), 21 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library= /MpInitLib/PeiMpLib.c > index f80e00edcff3..a5710789c8c3 100644 > --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c > @@ -126,35 +126,41 @@ SaveCpuMpData ( > IN CPU_MP_DATA *CpuMpData > ) > { > - UINT64 Data64; > - UINTN Index; > - CPU_INFO_IN_HOB *CpuInfoInHob; > - MP_HAND_OFF *MpHandOff; > - UINTN MpHandOffSize; > + STATIC CONST UINT32 CpusPerHob =3D 128; This should be a fixed-at-build PCD. Easy to modify on the build command line, for test coverage, but for production builds, it should be as large as possible. In fact, the code should determine how many CPU entries fit in a single HOB [*], and the PCD should be checked against it: - PCD =3D=3D 0: use the maximum - PCD > maximum: assert - otherwise: use the PCD as chunking factor [*] See BuildGuidHob() in "MdePkg/Library/PeiHobLib/HobLib.c": | // | // Make sure that data length is not too long. | // | ASSERT (DataLength <=3D (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE))); So the max permitted payload size is 0xFFE0 bytes, if I count right. CpusPerHob =3D (0xFFE0 - sizeof (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_O= FF); > + UINT64 Data64; > + UINT32 Index, HobBase; > + CPU_INFO_IN_HOB *CpuInfoInHob; > + MP_HAND_OFF *MpHandOff; > + UINTN MpHandOffSize; > > // > // When APs are in a state that can be waken up by a store operation t= o a memory address, > // report the MP_HAND_OFF data for DXE to use. > // > - CpuInfoInHob =3D (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob; > - MpHandOffSize =3D sizeof (MP_HAND_OFF) + sizeof (PROCESSOR_HAND_OFF) *= CpuMpData->CpuCount; > - MpHandOff =3D (MP_HAND_OFF *)BuildGuidHob (&mMpHandOffGuid, MpHand= OffSize); > - ASSERT (MpHandOff !=3D NULL); > - ZeroMem (MpHandOff, MpHandOffSize); > - MpHandOff->ProcessorIndex =3D 0; > + CpuInfoInHob =3D (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob; > > - MpHandOff->CpuCount =3D CpuMpData->CpuCount; > - if (CpuMpData->ApLoopMode !=3D ApInHltLoop) { > - MpHandOff->StartupSignalValue =3D MP_HAND_OFF_SIGNAL; > - MpHandOff->WaitLoopExecutionMode =3D sizeof (VOID *); > - } > + for (Index =3D 0; Index < CpuMpData->CpuCount; Index++) { > + if (Index % CpusPerHob =3D=3D 0) { > + MpHandOffSize =3D sizeof (MP_HAND_OFF) + sizeof (PROCESSOR_HAND_OF= F) * CpusPerHob; I don't like that the last HOB will only be partially filled in, most of the time. Especially the max chunking factor -- which strives to create HOBs that are approximately 64KB in size -- might waste ~32KB on average, using a flat multiplication like the above. How about: UINT32 FreeInHob; PROCESSOR_HAND_OFF *Info; FreeInHob =3D 0; for (Index =3D 0; Index < CpuMpData->CpuCount; Index++) { if (FreeInHob =3D=3D 0) { FreeInHob =3D MIN (CpusPerHob, CpuMpData->CpuCount - Index); MpHandOffSize =3D sizeof *MpHandOff + FreeInHob * sizeof *Info; MpHandOff =3D BuildGuidHob (&mMpHandOffGuid, MpHandOffSize); ASSERT (MpHandOff !=3D NULL); ZeroMem (MpHandOff, MpHandOffSize); MpHandOff->ProcessorIndex =3D Index; MpHandOff->CpuCount =3D FreeInHob; Info =3D MpHandOff->Info; } Info->ApicId =3D CpuInfoInHob[Index].ApicId; Info->Health =3D CpuInfoInHob[Index].Health; if (CpuMpData->ApLoopMode !=3D ApInHltLoop) { Info->StartupSignalAddress =3D (UINT64)(UINTN)CpuMpData->CpuData[I= ndex].StartupApSignal; Info->StartupProcedureAddress =3D (UINT64)(UINTN)&CpuMpData->CpuData[= Index].ApFunction; } Info++; FreeInHob--; } And then "HobBase" becomes superfluous. (Well, having a HobBase that carries information between iterations of the loop, versus an Info pointer, is conceptually the same; it's just that the Info pointer allows for shorter expressions.) The UINTN -> UINT32 type change for Index looks fine, however. > + MpHandOff =3D (MP_HAND_OFF *)BuildGuidHob (&mMpHandOffGuid, Mp= HandOffSize); > + ASSERT (MpHandOff !=3D NULL); > + ZeroMem (MpHandOff, MpHandOffSize); > > - for (Index =3D 0; Index < MpHandOff->CpuCount; Index++) { > - MpHandOff->Info[Index].ApicId =3D CpuInfoInHob[Index].ApicId; > - MpHandOff->Info[Index].Health =3D CpuInfoInHob[Index].Health; > + HobBase =3D Index; > + MpHandOff->ProcessorIndex =3D HobBase; > + MpHandOff->CpuCount =3D MIN (CpuMpData->CpuCount - HobBase, = CpusPerHob); Yes, the MIN() here is my key point, but I think we should also let it control the allocation! > + > + if (CpuMpData->ApLoopMode !=3D ApInHltLoop) { > + MpHandOff->StartupSignalValue =3D MP_HAND_OFF_SIGNAL; > + MpHandOff->WaitLoopExecutionMode =3D sizeof (VOID *); > + } > + } As noted elsewhere, these fields don't belong in the loop (they don't belong to MP_HAND_OFF, going forward); the two of them together should form a new singleton structure. > + > + MpHandOff->Info[Index-HobBase].ApicId =3D CpuInfoInHob[Index].ApicId= ; > + MpHandOff->Info[Index-HobBase].Health =3D CpuInfoInHob[Index].Health= ; > if (CpuMpData->ApLoopMode !=3D ApInHltLoop) { > - MpHandOff->Info[Index].StartupSignalAddress =3D (UINT64)(UINTN)= CpuMpData->CpuData[Index].StartupApSignal; > - MpHandOff->Info[Index].StartupProcedureAddress =3D (UINT64)(UINTN)= &CpuMpData->CpuData[Index].ApFunction; > + MpHandOff->Info[Index-HobBase].StartupSignalAddress =3D (UINT64= )(UINTN)CpuMpData->CpuData[Index].StartupApSignal; > + MpHandOff->Info[Index-HobBase].StartupProcedureAddress =3D (UINT64= )(UINTN)&CpuMpData->CpuData[Index].ApFunction; > } > } > 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 (#115595): https://edk2.groups.io/g/devel/message/115595 Mute This Topic: https://groups.io/mt/104369848/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-