From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Fri, 26 Apr 2019 12:25:01 -0700 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 27D9E9D788; Fri, 26 Apr 2019 19:25:01 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-104.rdu2.redhat.com [10.10.121.104]) by smtp.corp.redhat.com (Postfix) with ESMTP id ECB701001DE7; Fri, 26 Apr 2019 19:24:58 +0000 (UTC) Subject: Re: [Patch 2/4] UefiCpuPkg/MpInitLib: Avoid MSR_IA32_APIC_BASE for single core To: Michael D Kinney , devel@edk2.groups.io Cc: Eric Dong , Ray Ni , Jian J Wang References: <20190425175334.5944-1-michael.d.kinney@intel.com> <20190425175334.5944-3-michael.d.kinney@intel.com> From: "Laszlo Ersek" Message-ID: <8c03f148-8b61-2477-7708-0e55f32b581b@redhat.com> Date: Fri, 26 Apr 2019 21:24:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190425175334.5944-3-michael.d.kinney@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Fri, 26 Apr 2019 19:25:01 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit (+Jian) Hi Mike, thank you for the CC. On 04/25/19 19:53, Michael D Kinney wrote: > Avoid access to MSR_IA32_APIC_BASE that may not be supported > on single core CPUs. If PcdCpuMaxLogicalProcessorNumber is 1, > then there is only one CPU that must be the BSP. > > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Signed-off-by: Michael D Kinney > --- > UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c > index 35dff91fd2..5488049c08 100644 > --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c > @@ -1,7 +1,7 @@ > /** @file > MP initialize support functions for PEI phase. > > - Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.
> + Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -101,6 +101,19 @@ GetCpuMpData ( > MSR_IA32_APIC_BASE_REGISTER ApicBaseMsr; > IA32_DESCRIPTOR Idtr; > > + // > + // If there is only 1 CPU, then it must be the BSP. This avoids an access to > + // MSR_IA32_APIC_BASE that may not be supported on single core CPUs. > + // > + if (PcdGet32 (PcdCpuMaxLogicalProcessorNumber) == 1) { > + CpuMpData = GetCpuMpDataFromGuidedHob (); > + ASSERT (CpuMpData != NULL); > + return CpuMpData; > + } > + > + // > + // Otherwise use MSR_IA32_APIC_BASE to determine if the CPU is BSP or AP. > + // > ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE); > if (ApicBaseMsr.Bits.BSP == 1) { > CpuMpData = GetCpuMpDataFromGuidedHob (); > This patch leads me down on two paths: (1) Specifically regarding the code change. I think this patch is unsafe on platforms that dynamically set the PCD to a value larger than 1. (Including OVMF.) If the value is larger than 1, then the system has at least one AP, and the AP may enter the function. In addition, because the PCD is dynamic, the PcdGet32() call will invoke the PCD PPI (if I understand correctly), which is not allowed for an AP. Is it not possible to determine the availability of MSR_IA32_APIC_BASE from CPUID, or a different MSR? (2) More generally, this patch made me review OvmfPkg/PlatformPei: (a) OvmfPkg/PlatformPei sets the PCD in MaxCpuCountInitialization(), (b) later, OvmfPkg/PlatformPei publishes the permanent PEI RAM in PublishPeiMemory() (c) which in turn leads to the installation of gEfiPeiMemoryDiscoveredPpiGuid intto the PPI database, by the PEI Core (d) CpuMpPei can now be dispatched, because it has a depex on the "memory discovered" PPI (e) PeiMpInitLib, which is linked into CpuMpPei, can consume the PCD safely. I relied on this behavior in the following OVMF commit: commit 45a70db3c3a59b64e0f517870415963fbfacf507 Author: Laszlo Ersek Date: Thu Nov 24 15:18:44 2016 +0100 OvmfPkg/PlatformPei: take VCPU count from QEMU and configure MpInitLib These settings will allow CpuMpPei and CpuDxe to wait for the initial AP check-ins exactly as long as necessary. It is safe to set PcdCpuMaxLogicalProcessorNumber and PcdCpuApInitTimeOutInMicroSeconds in OvmfPkg/PlatformPei. OvmfPkg/PlatformPei installs the permanent PEI RAM, producing gEfiPeiMemoryDiscoveredPpiGuid, and UefiCpuPkg/CpuMpPei has a depex on gEfiPeiMemoryDiscoveredPpiGuid. [...] Except... in commit 0a0d5296e448 ("UefiCpuPkg/CpuMpPei: support stack guard feature", 2018-09-10), the DEPEX mentioned in step (d) was deleted. So now I got a bit nervous, because how are then the setting and reading of the PCD serialized between OvmfPkg/PlatformPei, and PeiMpInitLib in CpuMpPei? Luckily however, I think we're safe: - CpuMpPei itself doesn't consume the PCD, - MpInitLib consumes the PCD in several functions, but all clients of MpInitLib must call MpInitLibInitialize() first, before using other library APIs - CpuMpPei calls MpInitLibInitialize() in the InitializeCpuMpWorker() function - the InitializeCpuMpWorker() function is only called from the MemoryDiscoveredPpiNotifyCallback(). So the PCD set/get order remains safe and deterministic. Even though CpuMpPei can now be dispatched before permanent memory is discovered, MpInitLibInitialize() -- and so the reading of the PCD -- is still delayed until after permanent PEI RAM is available. That's a relief. In fact, it looks like commit 0a0d5296e448 ("UefiCpuPkg/CpuMpPei: support stack guard feature", 2018-09-10) delayed the entire original entry point of CpuMpPei into the "memory discovered" callback. That appears OK to me, it's just that the patch (~1000 lines) could have been split at least in two: one patch could have implemented the "PPI DEPEX --> PPI notify" transformation, without other changes in behavior, and the second patch could have extended the (now delayed) startup logic with InitializeMpExceptionStackSwitchHandlers() & friends. Thanks Laszlo