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.web11.8134.1672987349917558199 for ; Thu, 05 Jan 2023 22:42:30 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ZjfntXYL; 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=1672987349; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=6jQojeFBZXOPFpzvGdW01RW3ibq7VjMKQAB1PFtUoxc=; b=ZjfntXYLqqtg1NqhU0zxlBmz+KQa4jrshtHOarRBbOVmHresxF0MDlsSg4PaUmmb5AOwjh U+o584ONmdZ6xm2Yku6wG+Zvzx95i9NZrXMHb0B2dQ8W3GMTyMKr3vMYh6MhiUrM8sef/M cmCtFfGuE5uvU5vdXNUNfkBzaInQffE= 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-486-Hie2aYsFOTyR9FQtQrfYxQ-1; Fri, 06 Jan 2023 01:42:23 -0500 X-MC-Unique: Hie2aYsFOTyR9FQtQrfYxQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 6C4A2101A521; Fri, 6 Jan 2023 06:42:23 +0000 (UTC) Received: from [10.39.192.26] (unknown [10.39.192.26]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 514D72166B30; Fri, 6 Jan 2023 06:42:22 +0000 (UTC) Message-ID: Date: Fri, 6 Jan 2023 07:42:20 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg:Fixed AsmRelocateApLoopStart and ensure allocated memory <4GB To: devel@edk2.groups.io, ray.ni@intel.com, "ardb@kernel.org" , "Xie, Yuanhao" , Gerd Hoffmann References: <20230105062108.1796-1-yuanhao.xie@intel.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.6 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 05:12, Ni, Ray wrote: >>> @@ -555,19 +556,25 @@ InitMpGlobalData ( >>> ) >>> ); >>> >>> - mReservedTopOfApStack = (UINTN)AllocateReservedPages (EFI_SIZE_TO_PAGES (ApSafeBufferSize)); >>> - ASSERT (mReservedTopOfApStack != 0); >>> - ASSERT ((mReservedTopOfApStack & (UINTN)(CPU_STACK_ALIGNMENT - 1)) == 0); >>> - 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))) { >> >> This looks the wrong way around. > > > Ard, > > Only AMD X64 (including SEV and without SEV) runs the code that > switches to 32bit paging disabled mode. > Intel X64 runs the code that stays at 64bit paging mode. So no need > for <4G memory. > All IA32 CPUs (including intel and AMD) stays at 32bit paging disabled > mode. The AllocateReservedPages() call should not return a memory > above 4GB in 32bit env. This argument about the allocations sounds valid, thanks. The code still remains incredibly hard to read. It needs serious cleanup. (1) Wherever we have "Amd" in an identifier, let's rename it to "Amd64", to better reflect the revised check. (2) Wherever we have an _AMD in a typedef, rename it to _AMD64. For example, in the ASM_RELOCATE_AP_LOOP_AMD typedef. The leading *comment* on that typedef should be updated, too. (3) The way the mReservedApLoopFunc variable is used (populated, and then consumed) in C code is super awkward. We have casts left and right, and duplicated code, too. (4) Before commit 73ccde8f6d04, we had two separate allocations: namely, for the AP loop func, and then the stacks of the CPUs together. The size of the loop func was rounded up to a whole multiple of EFI_PAGE_SIZE, and then the pages accommodating the loop func were set to executable. (Unfortunately the name for the rounded-up size was terribly non-descriptive: "ApSafeBufferSize".) This was done in commit bc2288f59ba2 ("UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory", 2018-03-08). And, because we had two separate allocations, which could of course be discontiguous between each other, we needed two variables for storing their addresses, mReservedApLoopFunc and mReservedTopOfApStack. Commit 73ccde8f6d04 ("UefiCpuPkg: Has APs in 64 bit long-mode before booting to OS.", 2022-12-20) *removed* the executable marking. (4a) Is that not a problem? And if it's not a problem, why was it not done (or at least explained) separately? (4b) After commit 73ccde8f6d04, we have a single allocation only. The two allocations have been fused. The "mReservedTopOfApStack" variable is now effectively a duplicate of mReservedApLoopFunc, so it's only good for confusion. It should be eliminated. (5) The "ApSafeBufferSize" variable name is now totally bogus. It's OK to have a variable for expressing the allocation size, but "ApSafeBufferSize" is wrong. It should be renamed. (6) Here's yet another bug in commit 73ccde8f6d04 (which is not fixed by the currently proposed patch): the size of the fused allocation, expressed in "ApSafeBufferSize", does not take into account which variant of the AP loop func we're going to use; it only accounts for the non-AMD64 version. This bug is likely masked by the rounding-up to EFI_PAGE_SIZE, but it's still a bug. (7) We should assert that AP_SAFE_STACK_SIZE is correctly aligned with STATIC_ASSERT(), not ASSERT(). Here's a rough patch (on top of this one) to demonstrate some of the improvements, all squashed together (just to show the ideas): > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > index 1994ee44c259..e77bdc4f201d 100644 > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > @@ -25,11 +25,16 @@ EFI_EVENT mCheckAllApsEvent = NULL; > EFI_EVENT mMpInitExitBootServicesEvent = NULL; > EFI_EVENT mLegacyBootEvent = NULL; > volatile BOOLEAN mStopCheckAllApsStatus = TRUE; > -VOID *mReservedApLoopFunc = NULL; > UINTN mReservedTopOfApStack; > volatile UINT32 mNumberToFinish = 0; > UINTN mApPageTable; > > +STATIC union { > + VOID *Data; > + ASM_RELOCATE_AP_LOOP_AMD Amd64Entry; > + ASM_RELOCATE_AP_LOOP GenericEntry; > +} mReservedApLoop; > + > // > // Begin wakeup buffer allocation below 0x88000 > // > @@ -381,8 +386,6 @@ RelocateApLoop ( > { > CPU_MP_DATA *CpuMpData; > BOOLEAN MwaitSupport; > - ASM_RELOCATE_AP_LOOP AsmRelocateApLoopFunc; > - ASM_RELOCATE_AP_LOOP_AMD AsmRelocateApLoopFuncAmd; > UINTN ProcessorNumber; > UINTN StackStart; > > @@ -391,8 +394,7 @@ RelocateApLoop ( > MwaitSupport = IsMwaitSupport (); > if (StandardSignatureIsAuthenticAMD () && (sizeof (UINTN) == sizeof (UINT64))) { > StackStart = CpuMpData->UseSevEsAPMethod ? CpuMpData->SevEsAPResetStackStart : mReservedTopOfApStack; > - AsmRelocateApLoopFuncAmd = (ASM_RELOCATE_AP_LOOP_AMD)(UINTN)mReservedApLoopFunc; > - AsmRelocateApLoopFuncAmd ( > + mReservedApLoop.Amd64Entry ( > MwaitSupport, > CpuMpData->ApTargetCState, > CpuMpData->PmCodeSegment, > @@ -404,8 +406,7 @@ RelocateApLoop ( > ); > } else { > StackStart = mReservedTopOfApStack; > - AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP)(UINTN)mReservedApLoopFunc; > - AsmRelocateApLoopFunc ( > + mReservedApLoop.GenericEntry ( > MwaitSupport, > CpuMpData->ApTargetCState, > StackStart - ProcessorNumber * AP_SAFE_STACK_SIZE, > @@ -475,12 +476,15 @@ InitMpGlobalData ( > ) > { > EFI_STATUS Status; > - UINTN ApSafeBufferSize; > + MP_ASSEMBLY_ADDRESS_MAP *AddressMap; > + UINTN AllocSize; > UINTN Index; > EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; > UINTN StackBase; > CPU_INFO_IN_HOB *CpuInfoInHob; > EFI_PHYSICAL_ADDRESS Address; > + UINT8 *ApLoopFuncData; > + UINTN ApLoopFuncSize; > > SaveCpuMpData (CpuMpData); > > @@ -549,48 +553,58 @@ InitMpGlobalData ( > // | Stack * N | > // +------------+ (low address) > // > - ApSafeBufferSize = EFI_PAGES_TO_SIZE ( > - EFI_SIZE_TO_PAGES ( > - CpuMpData->CpuCount * AP_SAFE_STACK_SIZE > - + CpuMpData->AddressMap.RelocateApLoopFuncSize > - ) > - ); > + > + AddressMap = &CpuMpData->AddressMap; > > 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 > - ); > + // > + // 64-bit AMD Processor > + // > + Address = BASE_4GB - 1; > + ApLoopFuncData = AddressMap->RelocateApLoopFuncAddressAmd; > + ApLoopFuncSize = 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, > - CpuMpData->AddressMap.RelocateApLoopFuncSize > - ); > + // > + // Intel Processor (32-bit or 64-bit), or 32-bit AMD Processor > + // > + Address = MAX_ADDRESS; > + ApLoopFuncData = AddressMap->RelocateApLoopFuncAddress; > + ApLoopFuncSize = AddressMap->RelocateApLoopFuncSize; > + } > > + STATIC_ASSERT ((AP_SAFE_STACK_SIZE & (CPU_STACK_ALIGNMENT - 1)) == 0); > + AllocSize = EFI_PAGES_TO_SIZE ( > + EFI_SIZE_TO_PAGES ( > + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE + ApLoopFuncSize > + ) > + ); > + > + Status = gBS->AllocatePages ( > + AllocateMaxAddress, > + EfiReservedMemoryType, > + EFI_SIZE_TO_PAGES (AllocSize), > + &Address > + ); > + ASSERT_EFI_ERROR (Status); > + > + mReservedTopOfApStack = ((UINTN)Address + > + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE); > + ASSERT ((mReservedTopOfApStack & (UINTN)(CPU_STACK_ALIGNMENT - 1)) == 0); > + > + mReservedApLoop.Data = (VOID *)mReservedTopOfApStack; > + CopyMem (mReservedApLoop.Data, ApLoopFuncData, ApLoopFuncSize); > + > + if (!StandardSignatureIsAuthenticAMD () && > + (sizeof (UINTN) == sizeof (UINT64))) { > + // > + // 64-bit Intel Processor > + // > mApPageTable = CreatePageTable ( > (UINTN)Address, > - ApSafeBufferSize > + AllocSize > ); > } > > - 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, > TPL_NOTIFY, Honestly, at this point I'm *even more convinced* that the original series should be reverted, and redone from the ground up. There is way too much cruft for sensible incremental fixing. If you consider just the renames I'm requesting, that's going to introduce huge churn already. So let's please first undo the damage done by 73ccde8f6d04, and then build up the desired functionality in *very small*, careful steps, using the correct variable and type names, right off the bat. And please let us consider *cleaning up* the source code a primary goal while at it, removing code duplication and so on. Thanks, Laszlo