From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web12.6872.1603144313875819220 for ; Mon, 19 Oct 2020 14:51:54 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=U5jf566q; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1603144312; 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=8WVWBvxrKcB0tAgB8hgyU1zgD+DZFz/Vn4X1ABc4cF8=; b=U5jf566qp5odip+bnncrp3G+G7CR0i8UAEGIdiH73i42OOT1ja+ZoNkKwFbwTskmrJDUJX /HwE06ajQVEghGoSwW47K+bgbr9+zfOd00wdbE4cx4a7mXhJt7KwOI9av+9abYJuEBw2/y cE/7mwW4VfTfdz3GaZ91sodud961qWM= 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-199-M5YuBnKzOZ-AcKoNnGdHxQ-1; Mon, 19 Oct 2020 17:51:48 -0400 X-MC-Unique: M5YuBnKzOZ-AcKoNnGdHxQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D46C48049C9; Mon, 19 Oct 2020 21:51:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-137.ams2.redhat.com [10.36.113.137]) by smtp.corp.redhat.com (Postfix) with ESMTP id ED4611992D; Mon, 19 Oct 2020 21:51:44 +0000 (UTC) Subject: Re: [PATCH v2 1/1] UefiCpuPkg/MpInitLib: Reduce reset vector memory pressure To: Tom Lendacky , devel@edk2.groups.io, Eric Dong , Ray Ni Cc: Rahul Kumar , Brijesh Singh , Garrett Kirkendall References: <21345cdbc906519558202b3851257ca07b9239ba.1600884239.git.thomas.lendacky@amd.com> <6f1b41f0-fa17-7cb5-8ee2-3fc66b58a25c@amd.com> <48451727-6cbc-8cd7-300a-d9e8c284dabf@redhat.com> <686df789-8a14-6730-f031-1ae6925d896c@amd.com> <0794f733-ca18-3b6e-b82f-ac62fa94a216@amd.com> From: "Laszlo Ersek" Message-ID: Date: Mon, 19 Oct 2020 23:51:43 +0200 MIME-Version: 1.0 In-Reply-To: <0794f733-ca18-3b6e-b82f-ac62fa94a216@amd.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 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 10/19/20 17:02, Tom Lendacky wrote: > On 10/2/20 11:42 AM, Tom Lendacky wrote: >> On 9/24/20 10:03 AM, Laszlo Ersek wrote: >>> 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). >> >> Any concern from the maintainers on this? Are you waiting on me for >> anything? Just want to check... > > Another ping to see what the status of this patch is... ... I guess we might as well break the process for a noble purpose, every once in a while, so I went ahead and merged this... with just my R-b. - [lersek@redhat.com: supply missing space character after "PcdGet32"] - commit 93edd1887e34 - https://github.com/tianocore/edk2/pull/1031 Thanks Laszlo >>>> 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 >>> >