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.web10.5656.1621500207333389004 for ; Thu, 20 May 2021 01:43:27 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=X8j8z6aI; 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=1621500206; 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=WGcon+pGYwh7WQXC4Z+R9FBHeqFRwclUw5YvqW1rBTM=; b=X8j8z6aI8BiT9x6GZM/RFaYTy2V4+lUJuJxcmIfBzzMoxTIbcZx23vgtUqug5LYsvznnxf RvCJ828jOLdyF4wkBqKoddvFyaYrihFldtLVQl2A+d4Kyf5T4dH+IZ3PBhT/EOUNT8NE8/ KpJaaqCdtYDZNxTSFQgdJ7lHhtsuzno= 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-29-abISZmXzNpWByvUxY5nXvQ-1; Thu, 20 May 2021 04:43:22 -0400 X-MC-Unique: abISZmXzNpWByvUxY5nXvQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7381E1009446; Thu, 20 May 2021 08:43:21 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-217.ams2.redhat.com [10.36.112.217]) by smtp.corp.redhat.com (Postfix) with ESMTPS id BFEBE5D720; Thu, 20 May 2021 08:43:19 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Brijesh Singh , Eric Dong , Ray Ni , Rahul Kumar , =?UTF-8?Q?Marvin_H=c3=a4user?= References: <3cae2ac836884b131725866264e0a0e1897052de.1621024125.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 20 May 2021 10:43:18 +0200 MIME-Version: 1.0 In-Reply-To: <3cae2ac836884b131725866264e0a0e1897052de.1621024125.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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 05/14/21 22:28, Lendacky, Thomas wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3324 > > The SEV-ES stacks currently share a page with the reset code and data. > Separate the SEV-ES stacks from the reset vector code and data to avoid > possible stack overflows from overwriting the code and/or data. > > When SEV-ES is enabled, invoke the GetWakeupBuffer() routine a second time > to allocate a new area, below the reset vector and data. > > Both the PEI and DXE versions of GetWakeupBuffer() are changed so that > when PcdSevEsIsEnabled is true, they will track the previous reset buffer > allocation in order to ensure that the new buffer allocation is below the > previous allocation. When PcdSevEsIsEnabled is false, the original logic > is followed. > > Fixes: 7b7508ad784d16a5208c8d12dff43aef6df0835b > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Rahul Kumar > Cc: Marvin Häuser > Signed-off-by: Tom Lendacky > > --- > > Changes since v1: > - Renamed the wakeup buffer variables to be unique in the PEI and DXE > libraries and to make it obvious they are SEV-ES specific. > - Use PcdGetBool (PcdSevEsIsEnabled) to make the changes regression-free > so that the new support is only use for SEV-ES guests. > --- > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 +++++++- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 49 +++++++++++++------- > UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 19 +++++++- > 3 files changed, 69 insertions(+), 18 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > index 7839c249760e..93fc63bf93e3 100644 > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > @@ -29,6 +29,11 @@ VOID *mReservedApLoopFunc = NULL; > UINTN mReservedTopOfApStack; > volatile UINT32 mNumberToFinish = 0; > > +// > +// Begin wakeup buffer allocation below 0x88000 > +// > +STATIC EFI_PHYSICAL_ADDRESS mSevEsDxeWakeupBuffer = 0x88000; > + > /** > Enable Debug Agent to support source debugging on AP function. > > @@ -102,7 +107,14 @@ GetWakeupBuffer ( > // LagacyBios driver depends on CPU Arch protocol which guarantees below > // allocation runs earlier than LegacyBios driver. > // > - StartAddress = 0x88000; > + if (PcdGetBool (PcdSevEsIsEnabled)) { > + // > + // SEV-ES Wakeup buffer should be under 0x88000 and under any previous one > + // > + StartAddress = mSevEsDxeWakeupBuffer; > + } else { > + StartAddress = 0x88000; > + } > Status = gBS->AllocatePages ( > AllocateMaxAddress, > MemoryType, > @@ -112,6 +124,11 @@ GetWakeupBuffer ( > ASSERT_EFI_ERROR (Status); > if (EFI_ERROR (Status)) { > StartAddress = (EFI_PHYSICAL_ADDRESS) -1; > + } else if (PcdGetBool (PcdSevEsIsEnabled)) { > + // > + // Next SEV-ES wakeup buffer allocation must be below this allocation > + // > + mSevEsDxeWakeupBuffer = StartAddress; > } > > DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n", > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index dc2a54aa31e8..b9a06747edbf 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -1164,20 +1164,6 @@ GetApResetVectorSize ( > 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; > } > > @@ -1192,6 +1178,7 @@ AllocateResetVector ( > ) > { > UINTN ApResetVectorSize; > + UINTN ApResetStackSize; > > if (CpuMpData->WakeupBuffer == (UINTN) -1) { > ApResetVectorSize = GetApResetVectorSize (&CpuMpData->AddressMap); > @@ -1207,9 +1194,39 @@ AllocateResetVector ( > CpuMpData->AddressMap.ModeTransitionOffset > ); > // > - // The reset stack starts at the end of the buffer. > + // The AP reset stack is only used by SEV-ES guests. Do not allocate it > + // if SEV-ES is not enabled. > // > - CpuMpData->SevEsAPResetStackStart = CpuMpData->WakeupBuffer + ApResetVectorSize; > + if (PcdGetBool (PcdSevEsIsEnabled)) { > + // > + // Stack location is based on ProcessorNumber, so use the total number > + // of processors for calculating the total stack area. > + // > + ApResetStackSize = (AP_RESET_STACK_SIZE * > + PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); > + > + // > + // Invoke GetWakeupBuffer a second time to allocate the stack area > + // below 1MB. The returned buffer will be page aligned and sized and > + // below the previously allocated buffer. > + // > + CpuMpData->SevEsAPResetStackStart = GetWakeupBuffer (ApResetStackSize); > + > + // > + // Check to be sure that the "allocate below" behavior hasn't changed. > + // This will also catch a failed allocation, as "-1" is returned on > + // failure. > + // > + if (CpuMpData->SevEsAPResetStackStart >= CpuMpData->WakeupBuffer) { > + DEBUG (( > + DEBUG_ERROR, > + "SEV-ES AP reset stack is not below wakeup buffer\n" > + )); > + > + ASSERT (FALSE); > + CpuDeadLoop (); > + } > + } > } > BackupAndPrepareWakeupBuffer (CpuMpData); > } > diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c > index 3989bd6a7a9f..90015c650c68 100644 > --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c > @@ -11,6 +11,8 @@ > #include > #include > > +STATIC UINT64 mSevEsPeiWakeupBuffer = BASE_1MB; > + > /** > S3 SMM Init Done notification function. > > @@ -220,7 +222,13 @@ GetWakeupBuffer ( > // Need memory under 1MB to be collected here > // > WakeupBufferEnd = Hob.ResourceDescriptor->PhysicalStart + Hob.ResourceDescriptor->ResourceLength; > - if (WakeupBufferEnd > BASE_1MB) { > + if (PcdGetBool (PcdSevEsIsEnabled) && > + WakeupBufferEnd > mSevEsPeiWakeupBuffer) { > + // > + // SEV-ES Wakeup buffer should be under 1MB and under any previous one > + // > + WakeupBufferEnd = mSevEsPeiWakeupBuffer; > + } else if (WakeupBufferEnd > BASE_1MB) { > // > // Wakeup buffer should be under 1MB > // > @@ -244,6 +252,15 @@ GetWakeupBuffer ( > } > DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n", > WakeupBufferStart, WakeupBufferSize)); > + > + if (PcdGetBool (PcdSevEsIsEnabled)) { > + // > + // Next SEV-ES wakeup buffer allocation must be below this > + // allocation > + // > + mSevEsPeiWakeupBuffer = WakeupBufferStart; > + } > + > return (UINTN)WakeupBufferStart; > } > } > Acked-by: Laszlo Ersek Should I forget to merge this after the stable tag, please remind me. Thanks! Laszlo