From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8FEAC222A3361 for ; Wed, 24 Jan 2018 07:24:13 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E285C7855A; Wed, 24 Jan 2018 15:29:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-13.rdu2.redhat.com [10.10.121.13]) by smtp.corp.redhat.com (Postfix) with ESMTP id C72295D724; Wed, 24 Jan 2018 15:29:40 +0000 (UTC) To: Jian J Wang , edk2-devel@lists.01.org Cc: Ruiyu Ni , Eric Dong References: <20180124020845.6096-1-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: <890f02ba-c4b3-0bd1-7cf4-8cc9b308861d@redhat.com> Date: Wed, 24 Jan 2018 16:29:39 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180124020845.6096-1-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Wed, 24 Jan 2018 15:29:41 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg/MpInitLib: fix issue in wakeup buffer initialization X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Jan 2018 15:24:15 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/24/18 03:08, Jian J Wang wrote: > To fix an issue in which enabling NX feature will mark the AP wakeup > buffer as non-executable and fail the AP init, the buffer was split > into two part: the lower part in memory within 1MB and the higher part > within allocated executable memory (EfiBootServicesCode). But the > address of higher part memory was stored in lower part memory, which > is actually shared with legacy components and will be overwritten by > LegacyBiosDxe driver if CSM is enabled. > > This patch fixes this issue by storing the address of higher part > memory in CpuMpData instead of ExchangeInfo. > > Cc: Ruiyu Ni > Cc: Eric Dong > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 20 ++++++++++---------- > UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 + > 2 files changed, 11 insertions(+), 10 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 6231968c74..42011d6231 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -823,19 +823,20 @@ FillExchangeInfoData ( > // Copy all 32-bit code and 64-bit code into memory with type of > // EfiBootServicesCode to avoid page fault if NX memory protection is enabled. > // > - if (ExchangeInfo->ModeTransitionMemory != 0) { > + if (CpuMpData->WakeupBufferHigh != 0) { > Size = CpuMpData->AddressMap.RendezvousFunnelSize - > CpuMpData->AddressMap.ModeTransitionOffset; > CopyMem ( > - (VOID *)(UINTN)ExchangeInfo->ModeTransitionMemory, > + (VOID *)CpuMpData->WakeupBufferHigh, > CpuMpData->AddressMap.RendezvousFunnelAddress + > CpuMpData->AddressMap.ModeTransitionOffset, > Size > ); > > - ExchangeInfo->ModeHighMemory = ExchangeInfo->ModeTransitionMemory; > - ExchangeInfo->ModeHighMemory += (UINT32)ExchangeInfo->ModeOffset - > - (UINT32)CpuMpData->AddressMap.ModeTransitionOffset; > + ExchangeInfo->ModeTransitionMemory = (UINT32)CpuMpData->WakeupBufferHigh; > + ExchangeInfo->ModeHighMemory = (UINT32)CpuMpData->WakeupBufferHigh + > + (UINT32)ExchangeInfo->ModeOffset - > + (UINT32)CpuMpData->AddressMap.ModeTransitionOffset; > ExchangeInfo->ModeHighSegment = (UINT16)ExchangeInfo->CodeSegment; > } else { > ExchangeInfo->ModeTransitionMemory = (UINT32) > @@ -916,11 +917,10 @@ AllocateResetVector ( > CpuMpData->WakeupBuffer = GetWakeupBuffer (ApResetVectorSize); > CpuMpData->MpCpuExchangeInfo = (MP_CPU_EXCHANGE_INFO *) (UINTN) > (CpuMpData->WakeupBuffer + CpuMpData->AddressMap.RendezvousFunnelSize); > - CpuMpData->MpCpuExchangeInfo->ModeTransitionMemory = (UINT32) > - GetModeTransitionBuffer ( > - CpuMpData->AddressMap.RendezvousFunnelSize - > - CpuMpData->AddressMap.ModeTransitionOffset > - ); > + CpuMpData->WakeupBufferHigh = GetModeTransitionBuffer ( > + CpuMpData->AddressMap.RendezvousFunnelSize - > + CpuMpData->AddressMap.ModeTransitionOffset > + ); > } > BackupAndPrepareWakeupBuffer (CpuMpData); > } > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h > index 0232fe896a..e7f9a4de0a 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > @@ -208,6 +208,7 @@ struct _CPU_MP_DATA { > UINTN CpuApStackSize; > MP_ASSEMBLY_ADDRESS_MAP AddressMap; > UINTN WakeupBuffer; > + UINTN WakeupBufferHigh; > UINTN BackupBuffer; > UINTN BackupBufferSize; > > As far as I see, all the values stored by FillExchangeInfoData() remain the same. The difference is that "ModeTransitionMemory" becomes an *output* field for FillExchangeInfoData(), from being an input field. We now explicitly store the value to it that we previously *assumed* it would hold (preserve), from AllocateResetVector(). The input now comes through WakeupBufferHigh, from the AllocateResetVector() function. Thus, I think the patch could have been written a bit more simply: we could have added the assignment ExchangeInfo->ModeTransitionMemory = (UINT32)CpuMpData->WakeupBufferHigh before the "if" in FillExchangeInfoData(), and then the rest of FillExchangeInfoData() would have needed no changes. But perhaps that would have been harder to understand. Reviewed-by: Laszlo Ersek Thanks Laszlo