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.10269.1600928545189095761 for ; Wed, 23 Sep 2020 23:22:25 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=JQhSgafC; 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=1600928544; 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=hmb2OOYIuLxO4XopO95gw413kqLwV3h3FzRuyKLlQWs=; b=JQhSgafCa3lF+eKQwob5uyB+N4GxpqJOOid6+tw2jWjRL5CITM9T1jWM/74khKMoh7sB42 6qxNis2HEsVZQEkEOKfqKIR6xwlaQIQkbJV7VHpWM3VvbEosKiES8LCfZfDPN+nh3jAplO /SiHChupxrD2KkRFyrhfJyoOw95xO70= 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-520-zqm6DZByPwWDw0_aWyI7Bw-1; Thu, 24 Sep 2020 02:22:20 -0400 X-MC-Unique: zqm6DZByPwWDw0_aWyI7Bw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D61E0393B1; Thu, 24 Sep 2020 06:22:18 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-39.ams2.redhat.com [10.36.112.39]) by smtp.corp.redhat.com (Postfix) with ESMTP id E6E4A1002C06; Thu, 24 Sep 2020 06:22:16 +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> From: "Laszlo Ersek" Message-ID: Date: Thu, 24 Sep 2020 08:22:15 +0200 MIME-Version: 1.0 In-Reply-To: <21345cdbc906519558202b3851257ca07b9239ba.1600884239.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 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/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. - We should insert a space character after "PcdGet32", before merging this patch. I think Ray or Eric could do this without a repost. Reviewed-by: Laszlo Ersek Thanks Laszlo