From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web09.12653.1582887016587057086 for ; Fri, 28 Feb 2020 02:50:16 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Y4lus1bC; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1582887015; 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=ghYsYDBQeYCWSgp/Z/Qwb/Q7Y9lTk6hvzbgMnkH/p5s=; b=Y4lus1bCXUVBNbhCb6bi5ijuzHbbgxFJcDNQc1a2tnH3rvlkVt0tLoB/CawnAk6EHKe1i0 rdKoNcCBquJA/7qUPDNAJWmKhTBA/1+WvLEJWRHCwdWqD3gyJpG04FAroPGO2BJItg7ifs sT336JlbsfOQPdjbcWx84P826UqLp7Y= 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-193-mIQnIACDPYKNwUyTGNnGXg-1; Fri, 28 Feb 2020 05:50:08 -0500 X-MC-Unique: mIQnIACDPYKNwUyTGNnGXg-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 1E740108838A; Fri, 28 Feb 2020 10:50:07 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-243.ams2.redhat.com [10.36.116.243]) by smtp.corp.redhat.com (Postfix) with ESMTP id E145392D01; Fri, 28 Feb 2020 10:50:04 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 02/16] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3 Resume for CPU hotplug To: "Dong, Eric" , "devel@edk2.groups.io" Cc: Ard Biesheuvel , Igor Mammedov , "Yao, Jiewen" , "Justen, Jordan L" , "Kinney, Michael D" , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , "Ni, Ray" References: <20200226221156.29589-1-lersek@redhat.com> <20200226221156.29589-3-lersek@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 28 Feb 2020 11:50:03 +0100 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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 02/28/20 04:05, Dong, Eric wrote: > Hi Laszlo, >=20 > Thanks for your patch. The change make sense base on the comments in the= data structure header file. >=20 > I also checked all the code related to this data structure. The inputs f= or this data structure are CpuS3DataDxe and RegisterCpuFeaturesLib. Both th= ese two drivers not support CPU hot plug feature, so the real inputs for mA= cpiCpuData.NumberOfCpus is the enabled CPU number in this system. So before= and after your code change, the CPU values are same. But the data structur= e comments said it can support CPU hot plug, so I agree your code change. >=20 > Reviewed-by: Eric Dong Thank you! Please allow me one additional comment on CpuS3DataDxe however: According to the comments in UefiCpuPkg/CpuS3DataDxe: [...] this module does not support hot plug CPUs. This module can be copied into a CPU specific package and customized if these additional features are required [...] in the last three patches of the series, I clone CpuS3DataDxe for OvmfPkg, and extend it for CPU hotplug: [edk2-devel] [PATCH v2 14/16] OvmfPkg: clone CpuS3DataDxe from UefiCpuPkg [edk2-devel] [PATCH v2 15/16] OvmfPkg/CpuS3DataDxe: superficial cleanups [edk2-devel] [PATCH v2 16/16] OvmfPkg/CpuS3DataDxe: enable S3 resume after CPU hotplug (This extension occurs in a QEMU-specific way -- in other words, OvmfPkg/CpuS3DataDxe is really a platform driver.) What I'm trying to say is: the PiSmmCpuDxeSmm changes from the present patch *are* utilized in a CPU hotplug situation too. In other words, PiSmmCpuDxeSmm is really exposed to a situation where the following expression is TRUE: (FeaturePcdGet (PcdCpuHotPlugSupport) && mNumberOfCpus < mAcpiCpuData.NumberOfCpus) It is testable with OVMF, after this series is applied. Thanks Laszlo >=20 > Thanks, > Eric >=20 > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Laszlo Er= sek > Sent: Thursday, February 27, 2020 6:12 AM > To: edk2-devel-groups-io > Cc: Ard Biesheuvel ; Dong, Eric ; Igor Mammedov ; Yao, Jiewen ; Justen, Jordan L ; Kinney, Michael D ; Philippe Mathieu-Daud=C3=A9 ;= Ni, Ray > Subject: [edk2-devel] [PATCH v2 02/16] UefiCpuPkg/PiSmmCpuDxeSmm: fix S3= Resume for CPU hotplug >=20 > The "ACPI_CPU_DATA.NumberOfCpus" field is specified as follows, in "Uefi= CpuPkg/Include/AcpiCpuData.h" (rewrapped for this commit message): >=20 > // > // The number of CPUs. If a platform does not support hot plug CPUs, > // then this is the number of CPUs detected when the platform is boote= d, > // regardless of being enabled or disabled. If a platform does suppor= t > // hot plug CPUs, then this is the maximum number of CPUs that the > // platform supports. > // >=20 > The InitializeCpuBeforeRebase() and InitializeCpuAfterRebase() functions= in "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c" try to restore CPU configuration on= the S3 Resume path for *all* CPUs accounted for in "ACPI_CPU_DATA.NumberOf= Cpus". This is wrong, as with CPU hotplug, not all of the possible CPUs may= be present at the time of S3 Suspend / Resume. > The symptom is an infinite wait. >=20 > Instead, the "mNumberOfCpus" variable should be used, which is properly = maintained through the EFI_SMM_CPU_SERVICE_PROTOCOL implementation (see Smm= AddProcessor(), SmmRemoveProcessor(), SmmCpuUpdate() in "UefiCpuPkg/PiSmmCp= uDxeSmm/CpuService.c"). >=20 > When CPU hotplug is disabled, "mNumberOfCpus" is constant, and equals "A= CPI_CPU_DATA.NumberOfCpus" at all times. >=20 > Cc: Ard Biesheuvel > Cc: Eric Dong > Cc: Igor Mammedov > Cc: Jiewen Yao > Cc: Jordan Justen > Cc: Michael Kinney > Cc: Philippe Mathieu-Daud=C3=A9 > Cc: Ray Ni > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1512 > Signed-off-by: Laszlo Ersek > Acked-by: Ard Biesheuvel > --- >=20 > Notes: > v2: > =20 > - Pick up Ard's Acked-by, which is conditional on approval from Inte= l > reviewers on Cc. (I'd like to save Ard the churn of re-acking > unmodified patches.) >=20 > UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeS= mm/CpuS3.c > index ba5cc0194c2d..1e0840119724 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > @@ -597,75 +597,85 @@ PrepareApStartupVector ( } > > /** > The function is invoked before SMBASE relocation in S3 path to restor= es CPU status. > > The function is invoked before SMBASE relocation in S3 path. It does = first time microcode load > and restores MTRRs for both BSP and APs. > > **/ > VOID > InitializeCpuBeforeRebase ( > VOID > ) > { > LoadMtrrData (mAcpiCpuData.MtrrTable); > > SetRegister (TRUE); > > ProgramVirtualWireMode (); > > PrepareApStartupVector (mAcpiCpuData.StartupVector); > > - mNumberToFinish =3D mAcpiCpuData.NumberOfCpus - 1; > + if (FeaturePcdGet (PcdCpuHotPlugSupport)) { > + ASSERT (mNumberOfCpus <=3D mAcpiCpuData.NumberOfCpus); } else { > + ASSERT (mNumberOfCpus =3D=3D mAcpiCpuData.NumberOfCpus); } > + mNumberToFinish =3D mNumberOfCpus - 1; > mExchangeInfo->ApFunction =3D (VOID *) (UINTN) InitializeAp; > > // > // Execute code for before SmmBaseReloc. Note: This flag is maintaine= d across S3 boots. > // > mInitApsAfterSmmBaseReloc =3D FALSE; > > // > // Send INIT IPI - SIPI to all APs > // > SendInitSipiSipiAllExcludingSelf ((UINT32)mAcpiCpuData.StartupVector)= ; > > while (mNumberToFinish > 0) { > CpuPause (); > } > } > > /** > The function is invoked after SMBASE relocation in S3 path to restore= s CPU status. > > The function is invoked after SMBASE relocation in S3 path. It restor= es configuration according to > data saved by normal boot path for both BSP and APs. > > **/ > VOID > InitializeCpuAfterRebase ( > VOID > ) > { > - mNumberToFinish =3D mAcpiCpuData.NumberOfCpus - 1; > + if (FeaturePcdGet (PcdCpuHotPlugSupport)) { > + ASSERT (mNumberOfCpus <=3D mAcpiCpuData.NumberOfCpus); } else { > + ASSERT (mNumberOfCpus =3D=3D mAcpiCpuData.NumberOfCpus); } > + mNumberToFinish =3D mNumberOfCpus - 1; > > // > // Signal that SMM base relocation is complete and to continue initia= lization for all APs. > // > mInitApsAfterSmmBaseReloc =3D TRUE; > > // > // Must begin set register after all APs have continue their initiali= zation. > // This is a requirement to support semaphore mechanism in register t= able. > // Because if semaphore's dependence type is package type, semaphore = will wait > // for all Aps in one package finishing their tasks before set next r= egister > // for all APs. If the Aps not begin its task during BSP doing its ta= sk, the > // BSP thread will hang because it is waiting for other Aps in the sa= me > // package finishing their task. > // > SetRegister (FALSE); > > while (mNumberToFinish > 0) { > CpuPause (); > } > } > > -- > 2.19.1.3.g30247aa5d201 >=20 >=20 >=20 >=20 >=20