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.web12.8502.1600849690405340061 for ; Wed, 23 Sep 2020 01:28:10 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=DfqJrOPB; 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=1600849689; 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=WdhbiEmGMn9bkYA8xLzVztflKKEGchwenOxyPzxBNzc=; b=DfqJrOPBhWtZeWVGB2z6HpCJuDyNoq2HhrS/laN1uYi1GKdo7LDXhOQk7t3Om30ylXIlIq uBI8UfkNL8gz9BByLhIswpkb04EKU5hHKETOTtP8lm3+ZzXhVolIzumxln/NaWlZNySW17 2T/L6aBiG+OiW+nPQ+dyXGcUcf1T3bA= 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-42-TAJeM5axNZe1mXVbKDbLSg-1; Wed, 23 Sep 2020 04:28:05 -0400 X-MC-Unique: TAJeM5axNZe1mXVbKDbLSg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id BDCF11882FA7; Wed, 23 Sep 2020 08:28:03 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-233.ams2.redhat.com [10.36.112.233]) by smtp.corp.redhat.com (Postfix) with ESMTP id 01660702E7; Wed, 23 Sep 2020 08:28:01 +0000 (UTC) Subject: Re: [PATCH 1/1] UefiCpuPkg/MpInitLib: Reduce reset vector memory pressure From: "Laszlo Ersek" To: Tom Lendacky , devel@edk2.groups.io Cc: Eric Dong , Ray Ni , Rahul Kumar , Brijesh Singh , Garrett Kirkendall References: <22eb2625-6ece-6f1d-43b2-4ca6aad54b9c@redhat.com> Message-ID: <53df4691-7cf7-66a8-da95-3f4dbe64545a@redhat.com> Date: Wed, 23 Sep 2020 10:28:00 +0200 MIME-Version: 1.0 In-Reply-To: <22eb2625-6ece-6f1d-43b2-4ca6aad54b9c@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 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: 7bit On 09/23/20 10:14, Laszlo Ersek wrote: > On 09/22/20 21:59, 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. >> >> Cc: Garrett Kirkendall >> Signed-off-by: Tom Lendacky >> --- >> UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c >> index 07426274f639..39af2f9fba7d 100644 >> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c >> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c >> @@ -1152,7 +1152,15 @@ GetApResetStackSize ( >> VOID >> ) >> { >> - return AP_RESET_STACK_SIZE * PcdGet32(PcdCpuMaxLogicalProcessorNumber); >> + // >> + // The AP reset stack is only used by SEV-ES guests. Don't add to the >> + // allocation if not necessary. >> + // >> + if (PcdGetBool (PcdSevEsIsEnabled) == TRUE) { >> + return AP_RESET_STACK_SIZE * PcdGet32(PcdCpuMaxLogicalProcessorNumber); >> + } else { >> + return 0; >> + } >> } >> >> /** >> > > Looks like this is a fix for commit 7b7508ad784d ("UefiCpuPkg: Allow AP > booting under SEV-ES", 2020-08-17). > > In retrospect, that commit changed "ApResetVectorSize" -- which is > passed to GetWakeupBuffer() -- from value [a] > > RendezvousFunnelSize + sizeof (MP_CPU_EXCHANGE_INFO) > > to value [b] > > ALIGN_VALUE ((RendezvousFunnelSize + > SwitchToRealSize + > sizeof (MP_CPU_EXCHANGE_INFO)), > CPU_STACK_ALIGNMENT) + > AP_RESET_STACK_SIZE * PcdGet32(PcdCpuMaxLogicalProcessorNumber) > > The currently proposed patch does not entirely restore > "ApResetVectorSize" to the value it used to carry before commit > 7b7508ad784d. Namely, while the patch eliminates the last addend, two > other changes from commit 7b7508ad784d remain in place: > > - the addition of "SwitchToRealSize", > - the alignment up to CPU_STACK_ALIGNMENT. > > As far as I understand, "SwitchToRealSize" is never zero > (AsmGetAddressMap() populates it with a difference of build-time > constants). I think it's not useful when SEV-ES is inactive. > > Furthermore, the alignment for stack purposes is useless if we won't > have AP stacks (i.e., again when SEV-ES is inactive). > > (1) Therefore I'd propose: > > - folding GetApResetStackSize() into GetApResetVectorSize(), > > - modifying GetApResetVectorSize() such that it return the original sum > [a] if SEV-ES is inactive, and the larger sum [b] if SEV-ES is active. > > Hmmm. OK, maybe "SwitchToRealSize" should remain in place. It's a small > addition, and it reflects a code portion that is permanent. However, I > do think the alignment is both useless and confusing. If we won't > allocate an array of stacks, the alignment really makes no sense. > > > (2) A style comment: PcdGetBool()'s return value should not be compared > with TRUE or FALSE; just use PcdGetBool() as the whole controlling > expression for the "if". > > > (3) Even better... can you modify GetApResetVectorSize() to take > &CpuMpData rather than &CpuMpData->AddressMap, and then check > CpuMpData->SevEsIsEnabled? > > Hmmm, wait, that's not really simple, as we call GetApResetVectorSize() > from MpInitLibInitialize() too, way before we set > CpuMpData->SevEsIsEnabled from the PCD. > > So I guess we should pass a dedicated BOOLEAN parameter to > GetApResetVectorSize(), called "SevEsIsEnabled". At the call site in > MpInitLibInitialize(), we should pass in the PCD's value. At the call > site in AllocateResetVector(), we should pass in > CpuMpData->SevEsIsEnabled. > > The reason I'm suggesting (3) is that I don't feel comfortable with > checking dynamic PCDs outside of entry point functions / initialization > functions. (4) The product AP_RESET_STACK_SIZE * PcdGet32(PcdCpuMaxLogicalProcessorNumber) includes the BSP. Is that intentional? Should we rather use: AP_RESET_STACK_SIZE * (PcdGet32(PcdCpuMaxLogicalProcessorNumber) - 1) ? (That would be a separate patch, of course.) Thanks Laszlo