From mboxrd@z Thu Jan 1 00:00:00 1970 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.web10.8416.1672987739559617265 for ; Thu, 05 Jan 2023 22:48:59 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=VBe6rDsW; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1672987738; 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=rzeU+0GhPfSCzP0pJ+Gt7LKMtQjMg00LhAWCaJhcJXg=; b=VBe6rDsWudu6UOFlOoHeDTXY4cqSG9fzF9yn9kOkBTfP3weRf3NgrsnHsxnqgPyWoslXaL 5rFNB01p+72N4MdgRuAs1BZGvg7EfE5//ISiKE1jP773m8/fEKhQaYD12w82hWJntkDens 1W2df6/N/l2UAzbXxeC1zk54L+S8Mr0= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-590-CO5bm6uCMT6KP5cnLVrygg-1; Fri, 06 Jan 2023 01:48:55 -0500 X-MC-Unique: CO5bm6uCMT6KP5cnLVrygg-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id A2EAA8027F8; Fri, 6 Jan 2023 06:48:54 +0000 (UTC) Received: from [10.39.192.26] (unknown [10.39.192.26]) by smtp.corp.redhat.com (Postfix) with ESMTPS id B76762026D4B; Fri, 6 Jan 2023 06:48:53 +0000 (UTC) Message-ID: Date: Fri, 6 Jan 2023 07:48:52 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [Patch V2 2/2] UefiCpuPkg: Keep 4GB limitation of memory allocation To: devel@edk2.groups.io, yuanhao.xie@intel.com Cc: Eric Dong , Ray Ni , Rahul Kumar References: <20230106031141.460-1-yuanhao.xie@intel.com> <20230106031141.460-2-yuanhao.xie@intel.com> From: "Laszlo Ersek" In-Reply-To: <20230106031141.460-2-yuanhao.xie@intel.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 1/6/23 04:11, Yuanhao Xie wrote: > Keep 4GB limitation of memory allocation if APs need transferring to > 32bit mode before handoff to the OS. > > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4234 > > Cc: Eric Dong > Cc: Ray Ni > Cc: Rahul Kumar > Signed-off-by: Yuanhao Xie > --- > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > index acbbf155c0..e4c42e34ce 100644 > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > @@ -480,6 +480,7 @@ InitMpGlobalData ( > EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; > UINTN StackBase; > CPU_INFO_IN_HOB *CpuInfoInHob; > + EFI_PHYSICAL_ADDRESS Address; > > SaveCpuMpData (CpuMpData); > > @@ -561,13 +562,25 @@ InitMpGlobalData ( > ASSERT ((AP_SAFE_STACK_SIZE & (CPU_STACK_ALIGNMENT - 1)) == 0); > > mReservedApLoopFunc = (VOID *)(mReservedTopOfApStack + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE); > - if (StandardSignatureIsAuthenticAMD ()) { > + if (StandardSignatureIsAuthenticAMD () && (sizeof (UINTN) == sizeof (UINT64))) { > + Address = BASE_4GB - 1; > + Status = gBS->AllocatePages ( > + AllocateMaxAddress, > + EfiReservedMemoryType, > + EFI_SIZE_TO_PAGES (ApSafeBufferSize), > + &Address > + ); > + ASSERT_EFI_ERROR (Status); > + mReservedApLoopFunc = (VOID *)((UINTN)Address + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE); > CopyMem ( > mReservedApLoopFunc, > CpuMpData->AddressMap.RelocateApLoopFuncAddressAmd, > CpuMpData->AddressMap.RelocateApLoopFuncSizeAmd > ); > } else { > + Address = (UINTN)AllocateReservedPages (EFI_SIZE_TO_PAGES (ApSafeBufferSize)); > + ASSERT (Address != 0); > + mReservedApLoopFunc = (VOID *)((UINTN)Address + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE); > CopyMem ( > mReservedApLoopFunc, > CpuMpData->AddressMap.RelocateApLoopFuncAddress, > @@ -575,12 +588,14 @@ InitMpGlobalData ( > ); > > mApPageTable = CreatePageTable ( > - mReservedTopOfApStack, > + (UINTN)Address, > ApSafeBufferSize > ); > } > > - mReservedTopOfApStack += CpuMpData->CpuCount * AP_SAFE_STACK_SIZE; > + mReservedTopOfApStack = (UINTN)Address + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE; > + ASSERT ((mReservedTopOfApStack & (UINTN)(CPU_STACK_ALIGNMENT - 1)) == 0); > + ASSERT ((AP_SAFE_STACK_SIZE & (CPU_STACK_ALIGNMENT - 1)) == 0); > > Status = gBS->CreateEvent ( > EVT_TIMER | EVT_NOTIFY_SIGNAL, Nacked-by: Laszlo Ersek The code remains in a bad shape. Please see my comments here (all three links point to the same message): https://listman.redhat.com/archives/edk2-devel-archive/2023-January/057365.html https://edk2.groups.io/g/devel/message/98067 http://mid.mail-archive.com/ad89a4bb-12b5-020f-8b83-b7833dfcd0d3@redhat.com Please revert the original series, reimplement it in minimal steps, and always check for opportunities to clean up the existent code before you try to add new functionality. Just to name one example, calling AllocateReservedPages() on one branch and gBS->AllocatePages() on the other branch is unjustifiable. Both can be expressed with gBS->AllocatePages(AllocateMaxAddress), you just need to set the limit properly on each branch. Avoid duplicating logic if you can express the desired behavior with shared code, by abstracting *data differences*. Laszlo