From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.9427.1672911496068310591 for ; Thu, 05 Jan 2023 01:38:16 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=AjP6tNSM; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id DC02C6194B for ; Thu, 5 Jan 2023 09:38:14 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 6357FC43398 for ; Thu, 5 Jan 2023 09:38:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1672911494; bh=PBUaO0Yu0uQkci2NBvDkOlCk6iVNb8HCEt83piWGk+w=; h=References:In-Reply-To:From:Date:Subject:To:From; b=AjP6tNSMJxGMI9VQlXFuz03bap9iikF6nSAUsw04CSxY/r258vSs+YwUs1Z6zHMCA t7EWh7CbuI1iO28ApsoYSXVSBbtt1+DCWpGfNaw3i6HnQQZHah02hFOjiCQlkqZTF4 /jsEx7IVZcSMIJJn1RCCHftPu5Fz1/yNYPx00Byh91aOD6KkaqVsHr24pteZDWjqfx RRWCWbGnZFPc3p25SPQTcRZw72sY0xQVXNbrDHnPAEbuZ6OQux/Il0AOM8HXq0qP5t 5+xYw4egBQMlOtAq8WMP0IqTzwCghQJsRuMhtSF/yRxrUvFzhX4il7aAfQAuLoy0EF L49hdQkZHsKyg== Received: by mail-lj1-f171.google.com with SMTP id g14so38090097ljh.10 for ; Thu, 05 Jan 2023 01:38:14 -0800 (PST) X-Gm-Message-State: AFqh2kpXuON6jt1XYPOYsDvQo3Ud12U9BB3U/ABbI530XM6KVEQMzN95 1k2DZUGDUfzvrwQ5e8zYVWbv90NEbH9eR6d2sgc= X-Google-Smtp-Source: AMrXdXu//7Q3P28kfQMEgAT/qh1zcnoW7IrzSted4QsymYl4Af9eg/dNFfWJeE7ZjYINfJ58/z5aEMeAHsUnuwMyZA8= X-Received: by 2002:a2e:a804:0:b0:27f:fb12:6c20 with SMTP id l4-20020a2ea804000000b0027ffb126c20mr919358ljq.152.1672911492409; Thu, 05 Jan 2023 01:38:12 -0800 (PST) MIME-Version: 1.0 References: <20230105062108.1796-1-yuanhao.xie@intel.com> In-Reply-To: <20230105062108.1796-1-yuanhao.xie@intel.com> From: "Ard Biesheuvel" Date: Thu, 5 Jan 2023 10:38:01 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg:Fixed AsmRelocateApLoopStart and ensure allocated memory <4GB To: devel@edk2.groups.io, yuanhao.xie@intel.com Content-Type: text/plain; charset="UTF-8" On Thu, 5 Jan 2023 at 07:21, Yuanhao Xie wrote: > > Kept 4GB allocation limit for the case that APs are still transferred to > 32-bit protected mode on long mode DXE. > Fixed AsmRelocateApLoopStart stack offset in Ia32. > > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4234 > > Cc: Eric Dong eric.dong@intel.com > Cc: Ray Ni ray.ni@intel.com > Cc: Rahul Kumar rahul1.kumar@intel.com > Signed-off-by: Yuanhao Xie > --- > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 35 ++++++++++++------- > .../Library/MpInitLib/Ia32/MpFuncs.nasm | 9 ++--- > 2 files changed, 25 insertions(+), 19 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > index beab06a5b1..1994ee44c2 100644 > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > @@ -389,7 +389,7 @@ RelocateApLoop ( > MpInitLibWhoAmI (&ProcessorNumber); > CpuMpData = GetCpuMpData (); > MwaitSupport = IsMwaitSupport (); > - if (StandardSignatureIsAuthenticAMD ()) { > + if (StandardSignatureIsAuthenticAMD () && (sizeof (UINTN) == sizeof (UINT64))) { > StackStart = CpuMpData->UseSevEsAPMethod ? CpuMpData->SevEsAPResetStackStart : mReservedTopOfApStack; > AsmRelocateApLoopFuncAmd = (ASM_RELOCATE_AP_LOOP_AMD)(UINTN)mReservedApLoopFunc; > AsmRelocateApLoopFuncAmd ( > @@ -480,6 +480,7 @@ InitMpGlobalData ( > EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; > UINTN StackBase; > CPU_INFO_IN_HOB *CpuInfoInHob; > + EFI_PHYSICAL_ADDRESS Address; > > SaveCpuMpData (CpuMpData); > > @@ -536,9 +537,9 @@ InitMpGlobalData ( > > // > // Avoid APs access invalid buffer data which allocated by BootServices, > - // so we will allocate reserved data for AP loop code. We also need to > - // allocate this buffer below 4GB due to APs may be transferred to 32bit > - // protected mode on long mode DXE. > + // so we will allocate reserved data for AP loop code. We need to > + // allocate this buffer below 4GB for the case that APs are transferred > + // to 32-bit protected mode on long mode DXE. > // Allocating it in advance since memory services are not available in > // Exit Boot Services callback function. > // > @@ -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. > + 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); Isn't this code path still used for the IA32 version? > + mReservedApLoopFunc = (VOID *)((UINTN)Address + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE); > CopyMem ( > mReservedApLoopFunc, > CpuMpData->AddressMap.RelocateApLoopFuncAddress, > @@ -575,12 +582,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, > diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > index bfcdbd31c1..5cffa632ab 100644 > --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > @@ -219,20 +219,17 @@ SwitchToRealProcEnd: > RendezvousFunnelProcEnd: > > ;------------------------------------------------------------------------------------- > -; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish, Pm16CodeSegment, SevEsAPJumpTable, WakeupBuffer); > -; > -; The last three parameters (Pm16CodeSegment, SevEsAPJumpTable and WakeupBuffer) are > -; specific to SEV-ES support and are not applicable on IA32. > +; AsmRelocateApLoop (MwaitSupport, ApTargetCState, TopOfApStack, CountTofinish, Cr3); > ;------------------------------------------------------------------------------------- > AsmRelocateApLoopStart: > mov eax, esp > - mov esp, [eax + 16] ; TopOfApStack > + mov esp, [eax + 12] ; TopOfApStack > push dword [eax] ; push return address for stack trace > push ebp > mov ebp, esp > mov ebx, [eax + 8] ; ApTargetCState > mov ecx, [eax + 4] ; MwaitSupport > - mov eax, [eax + 20] ; CountTofinish > + mov eax, [eax + 16] ; CountTofinish > lock dec dword [eax] ; (*CountTofinish)-- > cmp cl, 1 ; Check mwait-monitor support > jnz HltLoop > -- > 2.36.1.windows.1 > > > > > >