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.8340.1600848886512551148 for ; Wed, 23 Sep 2020 01:14:46 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=O9dPQsJQ; 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=1600848885; 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=smWToWn9qdMXlm+xbN7UFK/eROUavIZ8hIETB4v9DZo=; b=O9dPQsJQzT1fZh1Hm/K9Ix0BXUE1/iF0l7HxyiFILRtAPsCeF42KRbT9qBLq2aLX8Gz/vp /QwISx2PELK0MvBF6I9vh6MXUjrrOZBVVMSYy6SYO1Dw1c3pxO3XACMUD8RoRgozYj2xyH TCWmVpVJZVaf5ccvtNvFdTO0duE73Ug= 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-244-5MSEdKXbOHW1HZnaz0e2jw-1; Wed, 23 Sep 2020 04:14:41 -0400 X-MC-Unique: 5MSEdKXbOHW1HZnaz0e2jw-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 704E283DBBE; Wed, 23 Sep 2020 08:14:40 +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 5BF027369C; Wed, 23 Sep 2020 08:14:38 +0000 (UTC) Subject: Re: [PATCH 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: From: "Laszlo Ersek" Message-ID: <22eb2625-6ece-6f1d-43b2-4ca6aad54b9c@redhat.com> Date: Wed, 23 Sep 2020 10:14:37 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 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-Transfer-Encoding: 7bit Content-Language: en-US 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. Thanks, Laszlo