From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web10.19171.1600959862388276910 for ; Thu, 24 Sep 2020 08:04:22 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=cOUZBpiu; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1600959861; 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=SknP6GXhirDb77ndbKZzTl/QSyzLyuFCq6S5rY1lr78=; b=cOUZBpiuu49N1H0yvux9IvHdj1mj6IVfZykYK0so9JZxXnUIWQ9lx7WgY9lfU6FidLLoQY TN3EBmwYmxzAOIQrm0ZO5odDIgvUPi/kFd9bCMthkcwTF4F7F7EClZxImloz+c48XHJoXo TLr6ImIRvlJqZRcdxGKfvCAoxBGo3cs= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-458-nFkQeKG3ODKMwGMHMrOwQg-1; Thu, 24 Sep 2020 11:04:03 -0400 X-MC-Unique: nFkQeKG3ODKMwGMHMrOwQg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id A4EC0104FC82; Thu, 24 Sep 2020 15:04:00 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-117.ams2.redhat.com [10.36.113.117]) by smtp.corp.redhat.com (Postfix) with ESMTP id EAC7160C04; Thu, 24 Sep 2020 15:03:58 +0000 (UTC) Subject: Re: [PATCH v2 1/1] UefiCpuPkg/MpInitLib: Reduce reset vector memory pressure To: Tom Lendacky , devel@edk2.groups.io Cc: Eric Dong , Ray Ni , Rahul Kumar , Brijesh Singh , Garrett Kirkendall References: <21345cdbc906519558202b3851257ca07b9239ba.1600884239.git.thomas.lendacky@amd.com> <6f1b41f0-fa17-7cb5-8ee2-3fc66b58a25c@amd.com> From: "Laszlo Ersek" Message-ID: <48451727-6cbc-8cd7-300a-d9e8c284dabf@redhat.com> Date: Thu, 24 Sep 2020 17:03:57 +0200 MIME-Version: 1.0 In-Reply-To: <6f1b41f0-fa17-7cb5-8ee2-3fc66b58a25c@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 09/24/20 15:30, Tom Lendacky wrote: > On 9/24/20 1:22 AM, Laszlo Ersek wrote: >> On 09/23/20 20:04, Tom Lendacky wrote: >>> From: Tom Lendacky >>> >>> The AP reset vector stack allocation is only required if running as an >>> SEV-ES guest. Since the reset vector allocation is below 1MB in memory, >>> eliminate the requirement for bare-metal systems and non SEV-ES guests >>> to allocate the extra stack area, which can be large if the >>> PcdCpuMaxLogicalProcessorNumber value is large, and also remove the >>> CPU_STACK_ALIGNMENT alignment. >>> >>> Fixes: 7b7508ad784d ("UefiCpuPkg: Allow AP booting under SEV-ES") >>> Cc: Garrett Kirkendall >>> Signed-off-by: Tom Lendacky >>> --- >>>   UefiCpuPkg/Library/MpInitLib/MpLib.c | 36 +++++++++----------- >>>   1 file changed, 17 insertions(+), 19 deletions(-) >>> >>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c >>> b/UefiCpuPkg/Library/MpInitLib/MpLib.c >>> index 07426274f639..a9708c6479d2 100644 >>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c >>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c >>> @@ -1141,20 +1141,6 @@ RestoreWakeupBuffer( >>>       ); >>>   } >>>   -/** >>> -  Calculate the size of the reset stack. >>> - >>> -  @return                 Total amount of memory required for stacks >>> -**/ >>> -STATIC >>> -UINTN >>> -GetApResetStackSize ( >>> -  VOID >>> -  ) >>> -{ >>> -  return AP_RESET_STACK_SIZE * >>> PcdGet32(PcdCpuMaxLogicalProcessorNumber); >>> -} >>> - >>>   /** >>>     Calculate the size of the reset vector. >>>   @@ -1170,11 +1156,23 @@ GetApResetVectorSize ( >>>   { >>>     UINTN  Size; >>>   -  Size = ALIGN_VALUE (AddressMap->RendezvousFunnelSize + >>> -                        AddressMap->SwitchToRealSize + >>> -                        sizeof (MP_CPU_EXCHANGE_INFO), >>> -                      CPU_STACK_ALIGNMENT); >>> -  Size += GetApResetStackSize (); >>> +  Size = AddressMap->RendezvousFunnelSize + >>> +           AddressMap->SwitchToRealSize + >>> +           sizeof (MP_CPU_EXCHANGE_INFO); >>> + >>> +  // >>> +  // The AP reset stack is only used by SEV-ES guests. Do not add to >>> the >>> +  // allocation if SEV-ES is not enabled. >>> +  // >>> +  if (PcdGetBool (PcdSevEsIsEnabled)) { >>> +    // >>> +    // Stack location is based on APIC ID, so use the total number of >>> +    // processors for calculating the total stack area. >>> +    // >>> +    Size += AP_RESET_STACK_SIZE * >>> PcdGet32(PcdCpuMaxLogicalProcessorNumber); >>> + >>> +    Size = ALIGN_VALUE (Size, CPU_STACK_ALIGNMENT); >>> +  } >>>       return Size; >>>   } >>> >> >> - I don't remember if it's required that the APIC ID space be densely >> populated. For example, if we have a topology with 7 possible (= >> maximum) logical CPUs, I'm unsure if a spec forbids any of those CPUs >> from having APIC ID 7. That could cause a problem in >> MpInitLibSevEsAPReset(), I assume. >> >> Anyway: that's a different topic. This patch looks OK to me because it >> only spells out the existent assumption wrt. APIC IDs vs. >> PcdCpuMaxLogicalProcessorNumber, plus it does solve the problem it wants >> to solve. >> >> - I was a bit surprised at first upon seeing the reordering of alignment >> vs. addition. But AP_RESET_STACK_SIZE is a whole multiple of >> CPU_STACK_ALIGNMENT. So adding AP_RESET_STACK_SIZE to Size n times as >> first step, does not change the congruence class of Size modulo >> CPU_STACK_ALIGNMENT. Therefore ALIGN_VALUE() will increment Size by the >> same value, regardless of whether it's done before or after the >> AP_RESET_STACK_SIZE additions. > > Ah, yes, I did change that order. I could submit one more version that > puts the ALIGN_VALUE back to the original position and fix the PcdGet32 > space if that is desired (but, as you determined, it doesn't change the > resulting value). Given that the stack grows down, one could plausibly claim that the variant seen in this patch (= align at last) is *more* intuitive. I'm OK with this version merged (with the whitespace fixed up). > I'm surprised that PatchCheck.py didn't pick up on the > spacing with PcdGet32. Or maybe ECC should warn about it in CI?... Thanks Laszlo