From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 7BFFED811DB for ; Fri, 2 Feb 2024 14:05:23 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=QEKDEjYXrSqX5zXLEY+9sdnAQ/1b/5BnJ2K+snnTk0A=; 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=1706882722; v=1; b=sxUjyL4j8Z3iYfYdmry8bAjZjn2arHtEcZnL7KBMAwEhJVxVmekL8ORLrFbKkkU+mKYNcsxc 9DFJ/u12iT5B/fukcz4wxmGhcymiL9PRw6yoBglZKWOiDw9D3qBZUSy42MaY5X6xAdF1n/18WB8 0W/hGsJQRIzsnxM136/HS6Xc= X-Received: by 127.0.0.2 with SMTP id gh1WYY7687511xCRYvNAWysE; Fri, 02 Feb 2024 06:05:22 -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.web10.23336.1706882721472680685 for ; Fri, 02 Feb 2024 06:05:21 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-125--FewekQ3OSukTjlsTbirvA-1; Fri, 02 Feb 2024 09:05:18 -0500 X-MC-Unique: -FewekQ3OSukTjlsTbirvA-1 X-Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (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 D577B3814EA7; Fri, 2 Feb 2024 14:05:16 +0000 (UTC) X-Received: from [10.39.192.34] (unknown [10.39.192.34]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 98AF9111FA; Fri, 2 Feb 2024 14:05:15 +0000 (UTC) Message-ID: <342b703f-0807-ceb1-78ba-443cc2833ee4@redhat.com> Date: Fri, 2 Feb 2024 15:05:09 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v1 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Execute CET and XD check only on BSP To: "Ni, Ray" , "Wu, Jiaxin" , "devel@edk2.groups.io" Cc: "Dong, Eric" , "Zeng, Star" , Gerd Hoffmann , "Kumar, Rahul R" References: <20240201112001.14416-1-jiaxin.wu@intel.com> <20240201112001.14416-2-jiaxin.wu@intel.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: 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: X-Gm-Message-State: ZTikbfrt23gcC7CZL9DTtFIcx7686176AA= 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=sxUjyL4j; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) On 2/2/24 07:03, Ni, Ray wrote: > Reviewed-by: Ray Ni >=20 > I originally thought CheckFeatureSupported() when running in parallel whe= n SMM base relocation is done in PEI > might corrupt the global variables. That was / is my concern as well. That's why I asked Jiaxin if CheckFeatureSupported() was executed concurrently by multiple processors. > But then I realized the function only perform variable modification from = TRUE to FALSE. > So even the code runs in parallel, it should be safe. I too noticed that those transitions are all TRUE->FALSE, and (IIRC) all the variables are just BOOLEANs (so 1 byte). Still, I don't like that (to be clear, I don't even like the pre-patch state): - The global variables are not even marked volatile. - We don't use any locking, like interlocked exchange. - We do other things than just "mBoolean =3D FALSE": we patch instructions in the SMI assembly code, too. All in all -- as long as this function can indeed run on multiple processors at the same time --, I think it would be much *cleaner* to: - split the BSP-only functionality off of CheckFeatureSupported() -- more importantly, off of SmmInitHandler. All that feature testing could be done in "single-threaded" code, could it not? - if we need to check some feature on each processor in separation, and then build a big conjunction of those results in the end, then I find it better if the implementation actually follows this logic. Let the processors store their results in separate BOOLEAN elements of an array, and let the BSP go over the array once the APs have settled. Minimally, at least document why the current lock-less but concurrent approach is safe! Laszlo >=20 > Thanks, > Ray >> -----Original Message----- >> From: Wu, Jiaxin >> Sent: Thursday, February 1, 2024 7:20 PM >> To: devel@edk2.groups.io >> Cc: Ni, Ray ; Laszlo Ersek ; Dong, = Eric >> ; Zeng, Star ; Gerd Hoffmann >> ; Kumar, Rahul R >> Subject: [PATCH v1 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Execute CET and XD >> check only on BSP >> >> Existing CheckFeatureSupported function will check CET & XD >> features on each processor. >> >> The CPUIDs for CET & XD features are software visible domain, >> which means a properly configured platform will have consistent >> values for these CPUID Leafs/SubLeafs/Fields on each logical >> processor. So, execute Execute CET and XD check only on BSP. >> >> As for MSR_IA32_MISC_ENABLE.BTS, it's core scope according SDM. >> So, still keep it check on each processor. >> >> Cc: Ray Ni >> Cc: Laszlo Ersek >> Cc: Eric Dong >> Cc: Zeng Star >> Cc: Gerd Hoffmann >> Cc: Rahul Kumar >> Signed-off-by: Jiaxin Wu >> --- >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 6 +-- >> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 78 +++++++++++++++++- >> ------------ >> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h | 6 ++- >> 3 files changed, 52 insertions(+), 38 deletions(-) >> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> index cd394826ff..15d26dd88f 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> @@ -1,9 +1,9 @@ >> /** @file >> Agent Module to load other modules to deploy SMM Entry Vector for X86 >> CPU. >> >> -Copyright (c) 2009 - 2023, Intel Corporation. All rights reserved.
>> +Copyright (c) 2009 - 2024, Intel Corporation. All rights reserved.
>> Copyright (c) 2017, AMD Incorporated. All rights reserved.
>> Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved. >> >> SPDX-License-Identifier: BSD-2-Clause-Patent >> >> @@ -375,13 +375,13 @@ SmmInitHandler ( >> &mCpuHotPlugData >> ); >> >> if (!mSmmS3Flag) { >> // >> - // Check XD and BTS features on each processor on normal boot >> + // Check CET & XD & BTS features on each processor on normal bo= ot >> // >> - CheckFeatureSupported (); >> + CheckFeatureSupported (IsBsp); >> } else if (IsBsp) { >> // >> // BSP rebase is already done above. >> // Initialize private data during S3 resume >> // >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c >> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c >> index 8142d3ceac..44c352ad98 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c >> @@ -1,9 +1,9 @@ >> /** @file >> Enable SMM profile. >> >> -Copyright (c) 2012 - 2023, Intel Corporation. All rights reserved.
>> +Copyright (c) 2012 - 2024, Intel Corporation. All rights reserved.
>> Copyright (c) 2017 - 2020, AMD Incorporated. All rights reserved.
>> >> SPDX-License-Identifier: BSD-2-Clause-Patent >> >> **/ >> @@ -892,62 +892,74 @@ InitSmmProfileInternal ( >> } >> >> /** >> Check if feature is supported by a processor. >> >> + @param[in] IsBsp Indicate it's called by BSP or not. >> + >> **/ >> VOID >> CheckFeatureSupported ( >> - VOID >> + IN BOOLEAN IsBsp >> ) >> { >> UINT32 RegEax; >> UINT32 RegEcx; >> UINT32 RegEdx; >> MSR_IA32_MISC_ENABLE_REGISTER MiscEnableMsr; >> >> - if ((PcdGet32 (PcdControlFlowEnforcementPropertyMask) !=3D 0) && >> mCetSupported) { >> - AsmCpuid (CPUID_SIGNATURE, &RegEax, NULL, NULL, NULL); >> - if (RegEax >=3D CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS) { >> - AsmCpuidEx (CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS, >> CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS_SUB_LEAF_INFO, NULL, >> NULL, &RegEcx, NULL); >> - if ((RegEcx & CPUID_CET_SS) =3D=3D 0) { >> + // >> + // The feature scope is software visible domain. >> + // Only need check on BSP. >> + // >> + if (IsBsp) { >> + if ((PcdGet32 (PcdControlFlowEnforcementPropertyMask) !=3D 0) && >> mCetSupported) { >> + AsmCpuid (CPUID_SIGNATURE, &RegEax, NULL, NULL, NULL); >> + if (RegEax >=3D CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS) { >> + AsmCpuidEx (CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS, >> CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS_SUB_LEAF_INFO, NULL, >> NULL, &RegEcx, NULL); >> + if ((RegEcx & CPUID_CET_SS) =3D=3D 0) { >> + mCetSupported =3D FALSE; >> + PatchInstructionX86 (mPatchCetSupported, mCetSupported, 1); >> + } >> + } else { >> mCetSupported =3D FALSE; >> PatchInstructionX86 (mPatchCetSupported, mCetSupported, 1); >> } >> - } else { >> - mCetSupported =3D FALSE; >> - PatchInstructionX86 (mPatchCetSupported, mCetSupported, 1); >> } >> - } >> >> - if (mXdSupported) { >> - AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL); >> - if (RegEax <=3D CPUID_EXTENDED_FUNCTION) { >> - // >> - // Extended CPUID functions are not supported on this processor. >> - // >> - mXdSupported =3D FALSE; >> - PatchInstructionX86 (gPatchXdSupported, mXdSupported, 1); >> - } >> + if (mXdSupported) { >> + AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL); >> + if (RegEax <=3D CPUID_EXTENDED_FUNCTION) { >> + // >> + // Extended CPUID functions are not supported on this processor= . >> + // >> + mXdSupported =3D FALSE; >> + PatchInstructionX86 (gPatchXdSupported, mXdSupported, 1); >> + } >> >> - AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, &RegEdx); >> - if ((RegEdx & CPUID1_EDX_XD_SUPPORT) =3D=3D 0) { >> - // >> - // Execute Disable Bit feature is not supported on this processor= . >> - // >> - mXdSupported =3D FALSE; >> - PatchInstructionX86 (gPatchXdSupported, mXdSupported, 1); >> - } >> + AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, &RegEdx); >> + if ((RegEdx & CPUID1_EDX_XD_SUPPORT) =3D=3D 0) { >> + // >> + // Execute Disable Bit feature is not supported on this process= or. >> + // >> + mXdSupported =3D FALSE; >> + PatchInstructionX86 (gPatchXdSupported, mXdSupported, 1); >> + } >> >> - if (StandardSignatureIsAuthenticAMD ()) { >> - // >> - // AMD processors do not support MSR_IA32_MISC_ENABLE >> - // >> - PatchInstructionX86 (gPatchMsrIa32MiscEnableSupported, FALSE, 1); >> + if (StandardSignatureIsAuthenticAMD ()) { >> + // >> + // AMD processors do not support MSR_IA32_MISC_ENABLE >> + // >> + PatchInstructionX86 (gPatchMsrIa32MiscEnableSupported, FALSE, 1= ); >> + } >> } >> } >> >> + // >> + // The feature scope is core. >> + // Need check on each processor. >> + // >> if (mBtsSupported) { >> AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx); >> if ((RegEdx & CPUID1_EDX_BTS_AVAILABLE) !=3D 0) { >> // >> // Per IA32 manuals: >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h >> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h >> index 1a82ac05ce..02554a9983 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h >> @@ -1,9 +1,9 @@ >> /** @file >> SMM profile header file. >> >> -Copyright (c) 2012 - 2019, Intel Corporation. All rights reserved.
>> +Copyright (c) 2012 - 2024, Intel Corporation. All rights reserved.
>> SPDX-License-Identifier: BSD-2-Clause-Patent >> >> **/ >> >> #ifndef _SMM_PROFILE_H_ >> @@ -81,14 +81,16 @@ PageFaultIdtHandlerSmmProfile ( >> ); >> >> /** >> Check if feature is supported by a processor. >> >> + @param[in] IsBsp Indicate it's called by BSP or not. >> + >> **/ >> VOID >> CheckFeatureSupported ( >> - VOID >> + IN BOOLEAN IsBsp >> ); >> >> /** >> Update page table according to protected memory ranges and the 4KB-pa= ge >> mapped memory ranges. >> >> -- >> 2.16.2.windows.1 >=20 -=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 (#115047): https://edk2.groups.io/g/devel/message/115047 Mute This Topic: https://groups.io/mt/104094806/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-