From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <bounce+27952+111172+7686176+12367111@groups.io>
Received: from mail02.groups.io (mail02.groups.io [66.175.222.108])
by spool.mail.gandi.net (Postfix) with ESMTPS id 0AA64AC1723
for <rebecca@openfw.io>; Mon, 13 Nov 2023 18:02:17 +0000 (UTC)
DKIM-Signature: a=rsa-sha256; bh=3AFhwAxMyBkV/F/PvFZ7M6AnTOnUGU+Z7JvkZi9Cr1k=;
c=relaxed/simple; d=groups.io;
h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding;
s=20140610; t=1699898536; v=1;
b=u0uNTsafiUvGJSgk+gM5Z7MBXEh49evrTmkYct+IBT6wweXdD9pAmr/RrBEPempLBKnduv48
Byl4xBhn8jH69DAEpMGII6KlHakwLz5RWJR//Bc8CxUzB5mRtdJCcbbzUwHlhk/nKlWabDtC2uj
6j6cR7HTVZSHyqIj6a2m+gIE=
X-Received: by 127.0.0.2 with SMTP id 4eCWYY7687511xSJ6zbe9VjZ; Mon, 13 Nov 2023 10:02:16 -0800
X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124])
by mx.groups.io with SMTP id smtpd.web11.2405.1699898535699148434
for <devel@edk2.groups.io>;
Mon, 13 Nov 2023 10:02:15 -0800
X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com
[66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS
(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id
us-mta-191-oyKaYgIYMHKx17Gq0VQzLw-1; Mon, 13 Nov 2023 13:02:09 -0500
X-MC-Unique: oyKaYgIYMHKx17Gq0VQzLw-1
X-Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
(No client certificate requested)
by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 0B93B84AEA0;
Mon, 13 Nov 2023 18:02:08 +0000 (UTC)
X-Received: from [10.39.192.220] (unknown [10.39.192.220])
by smtp.corp.redhat.com (Postfix) with ESMTPS id CA7E6492BFD;
Mon, 13 Nov 2023 18:02:06 +0000 (UTC)
Message-ID: <79bd9a7b-8b84-798c-c7ce-b7292dab1c7d@redhat.com>
Date: Mon, 13 Nov 2023 19:02:05 +0100
MIME-Version: 1.0
Subject: Re: [edk2-devel] [Patch V4] UefiCpuPkg/MpInitLib: Enable execute disable bit.
To: devel@edk2.groups.io, yuanhao.xie@intel.com
Cc: Eric Dong <eric.dong@intel.com>, Ray Ni <ray.ni@intel.com>,
Rahul Kumar <rahul1.kumar@intel.com>, Gerd Hoffmann <kraxel@redhat.com>
References: <20231113055948.1773-1-yuanhao.xie@intel.com>
From: "Laszlo Ersek" <lersek@redhat.com>
In-Reply-To: <20231113055948.1773-1-yuanhao.xie@intel.com>
X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.10
X-Mimecast-Spam-Score: 0
X-Mimecast-Originator: redhat.com
Precedence: Bulk
List-Subscribe: <mailto:devel+subscribe@edk2.groups.io>
List-Help: <mailto:devel+help@edk2.groups.io>
Sender: devel@edk2.groups.io
List-Id: <devel.edk2.groups.io>
Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io
Reply-To: devel@edk2.groups.io,lersek@redhat.com
List-Unsubscribe-Post: List-Unsubscribe=One-Click
List-Unsubscribe: <https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/plugh>
X-Gm-Message-State: 1UQrnDVZmRsM3ONxxRGnlsF0x7686176AA=
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
X-GND-Status: LEGIT
Authentication-Results: spool.mail.gandi.net;
dkim=pass header.d=groups.io header.s=20140610 header.b=u0uNTsaf;
dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none);
spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io
On 11/13/23 06:59, Yuanhao Xie wrote:
> From: Yuanhao Xie <yuanhao.xie@intel.com>
>
> This patch synchronizes the No-Execute bit in the IA32_EFER
> register for the APs before the RestoreVolatileRegisters operation.
>
> The commit 964a4f0, titled "Eliminate the second INIT-SIPI-SIPI
> sequence," replaces the second INIT-SIPI-SIPI sequence with the BSP
> calling the SwitchApContext function to initiate a specialized start-up
> signal, waking up APs in the DXE instead of using INIT-SIPI-SIPI.
>
> Due to this change, the logic for "Enable execute disable bit" in
> MpFuncs.nasm is no longer executed. However, to ensure the proper setup
> of the page table, it is necessary to synchronize the IA32_EFER.NXE for
> APs before executing RestoreVolatileRegisters .
>
> Based on SDM:
> If IA32_EFER.NXE is set to 1, it signifies execute-disable, meaning
> instruction fetches are not allowed from the 4-KByte page controlled by
> this entry. Conversely, if it is set to 0, it is reserved.
>
> Signed-off-by: Yuanhao Xie <yuanhao.xie@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 14 +++++++++++---
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 +
> 2 files changed, 12 insertions(+), 3 deletions(-)
Good problem description!
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/Mp=
InitLib/MpLib.c
> index 9a6ec5db5c..f29e66a14f 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -910,9 +910,16 @@ DxeApEntryPoint (
> CPU_MP_DATA *CpuMpData
> )
> {
> - UINTN ProcessorNumber;
> + UINTN ProcessorNumber;
> + MSR_IA32_EFER_REGISTER EferMsr;
>
> GetProcessorNumber (CpuMpData, &ProcessorNumber);
> + if (CpuMpData->EnableExecuteDisableForSwitchContext) {
> + EferMsr.Uint64 =3D AsmReadMsr64 (MSR_IA32_EFER);
> + EferMsr.Bits.NXE =3D 1;
> + AsmWriteMsr64 (MSR_IA32_EFER, EferMsr.Uint64);
> + }
> +
> RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FA=
LSE);
> InterlockedIncrement ((UINT32 *)&CpuMpData->FinishedCount);
> PlaceAPInMwaitLoopOrRunLoop (
> @@ -2188,8 +2195,9 @@ MpInitLibInitialize (
> if (MpHandOff->WaitLoopExecutionMode =3D=3D sizeof (VOID *)) {
> ASSERT (CpuMpData->ApLoopMode !=3D ApInHltLoop);
>
> - CpuMpData->FinishedCount =3D 0;
> - CpuMpData->InitFlag =3D ApInitDone;
> + CpuMpData->FinishedCount =3D 0;
> + CpuMpData->InitFlag =3D ApInitDone;
> + CpuMpData->EnableExecuteDisableForSwitchContext =3D IsBspExecuteDi=
sableEnabled ();
> SaveCpuMpData (CpuMpData);
> //
> // In scenarios where both the PEI and DXE phases run in the same
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/Mp=
InitLib/MpLib.h
> index 763db4963d..af296f6ac0 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -270,6 +270,7 @@ struct _CPU_MP_DATA {
> UINT64 TotalTime;
> EFI_EVENT WaitEvent;
> UINTN **FailedCpuList;
> + BOOLEAN EnableExecuteDisableForSwitchContext;
>
> AP_INIT_STATE InitFlag;
> BOOLEAN SwitchBspFlag;
Functionally this patch seems fine.
(I cannot test it very usefully, because this code path is not active in
OVMF.)
However, there's one thing I think is less than ideal: after this patch,
we'll have
MP_CPU_EXCHANGE_INFO . EnableExecuteDisable
but also
MP_CPU_EXCHANGE_INFO . CpuMpData -> EnableExecuteDisableForSwitchContext
Furthermore, we'll have two invocations of IsBspExecuteDisableEnabled():
- once for the original (HLT loop + INIT-SIPI-SIPI) method, in
WakeUpAP() -> FillExchangeInfoData(),
- and another time for the new method, in MpInitLibInitialize().
I feel that we should centralize this to one spot.
Note that in commit 629c1dacc9bd ("UefiCpuPkg: ApWakeupFunction directly
use CpuMpData.", 2023-07-11), you changed the prototype of
ApWakeupFunction(), such that it would take CpuMpData directly, rather
than MP_CPU_EXCHANGE_INFO. This was done so that in the next commit
(964a4f032dcd, "UefiCpuPkg: Eliminate the second INIT-SIPI-SIPI
sequence.", 2023-07-11), you could invoke ApWakeupFunction() on *both*
paths, old and new:
- old (INIT-SIPI-SIPI): from the assembly language startup code,
- new: from DxeApEntryPoint().
Therefore, it seems that the *old* field
"MP_CPU_EXCHANGE_INFO.EnableExecuteDisable" is now superfluous. You have
effectively pushed it down to "CpuMpData", so that it's available in
DxeApEntryPoint().
But CpuMpData is similarly available in the assembly language startup
code (that's why you could implement commit 629c1dacc9bd).
So I think this patch is good, but it should be followed with a further
patch: can you please rebase the *old* startup code to the new CpuMpData
field "EnableExecuteDisableForSwitchContext", and then eliminate
"MP_CPU_EXCHANGE_INFO.EnableExecuteDisable"?
Where we do
mov edi, esi
add edi, MP_CPU_EXCHANGE_INFO_FIELD (EnableExecuteDisable)
cmp byte [edi], 0
jz SkipEnableExecuteDisable
on IA32, and
mov esi, MP_CPU_EXCHANGE_INFO_FIELD (EnableExecuteDisable)
cmp byte [ebx + esi], 0
jz SkipEnableExecuteDisableBit
on X64, can we dereference the CpuMpData field (pointer) instead, and
grab the new field "EnableExecuteDisableForSwitchContext"?
Effectively MP_CPU_EXCHANGE_INFO seems to contain information that is
needed *only* by the INIT-SIPI-SIPI startup path, and CpuMpData holds
information that is needed by both startup paths. Given that we now have
XD status in the latter, we should drop it from the former.
(Of course, once we start using "EnableExecuteDisableForSwitchContext"
on both startup paths, then the *name* will be wrong -- it will no
longer be for "SwitchContext" only!)
... Yet further, this seems to indicate that calling
IsBspExecuteDisableEnabled() upon every invocation of WakeUpAP() (via
FillExchangeInfoData()) is superfluous, on the "old" startup path. We
should call IsBspExecuteDisableEnabled() only once, namely in
MpInitLibInitialize(), *regardless* of "WaitLoopExecutionMode", for
filling in the new field. Is that right?
Thanks,
Laszlo
-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111172): https://edk2.groups.io/g/devel/message/111172
Mute This Topic: https://groups.io/mt/102556608/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562=
12/xyzzy [rebecca@openfw.io]
-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-