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 47326202E617B for ; Wed, 25 Oct 2017 07:39:21 -0700 (PDT) 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 mx1.redhat.com (Postfix) with ESMTPS id B7BBBD2EE1; Wed, 25 Oct 2017 14:43:05 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B7BBBD2EE1 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-213.rdu2.redhat.com [10.10.120.213]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6BC005D6A0; Wed, 25 Oct 2017 14:43:04 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Ruiyu Ni References: <1508918319-10232-1-git-send-email-eric.dong@intel.com> From: Laszlo Ersek Message-ID: <516d85f0-c24c-25dd-6417-86643d389978@redhat.com> Date: Wed, 25 Oct 2017 16:43:03 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <1508918319-10232-1-git-send-email-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 25 Oct 2017 14:43:05 +0000 (UTC) Subject: Re: [Patch] UefiCpuPkg/CpuFeatures: Export HOB if CPU initialized in PEI. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Oct 2017 14:39:21 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Eric, On 10/25/17 09:58, Eric Dong wrote: > In current implementation, CPU initialized can be done in PEI > or DXE phase. PEI uses CpuFeaturesPie and Dxe uses CpuFeaturesDxe. (1) "CpuFeaturesPie" has a typo > If CPU initialized in PEI phase, CpuFeaturesDxe driver will > not be used. This driver will install gEdkiiCpuFeaturesInitDoneGuid > protocol after it initializes the CPU. > > Some drivers depend on this protocol to dispatch themselves. If > CpuFeaturesDxe not been used, these drivers will not be dispatched. > > This patch fix the above issue. If Cpu initialized in PEI > phase, it also report a guid HOB for CpuFeaturesDxe. > CpuFeaturesDxe will check this HOB first. If it found this > HOB, it just install gEdkiiCpuFeaturesInitDoneGuid protocol, > else it will also do the CPU initialization. > > Cc: Ruiyu Ni > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.c | 19 +++++++++++++++++++ > UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.inf | 1 + > UefiCpuPkg/CpuFeatures/CpuFeaturesPei.c | 6 ++++++ > UefiCpuPkg/CpuFeatures/CpuFeaturesPei.inf | 1 + > 4 files changed, 27 insertions(+) > > diff --git a/UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.c b/UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.c > index b0b186d..6c0ae9d 100644 > --- a/UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.c > +++ b/UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.c > @@ -19,6 +19,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -101,6 +102,24 @@ CpuFeaturesDxeInitialize ( > ) > { > VOID *Registration; > + EFI_STATUS Status; > + EFI_HANDLE Handle; > + > + if (GetFirstGuidHob (&gEdkiiCpuFeaturesInitDoneGuid) != NULL) { > + // > + // Try to find HOB first. This HOB exist means CPU features have > + // been initialized by CpuFeaturesPei driver, just install gEdkiiCpuFeaturesInitDoneGuid. (2) I think this line is too long; I suggest to wrap it to 80 characters. > + // > + Handle = NULL; > + Status = gBS->InstallProtocolInterface ( > + &Handle, > + &gEdkiiCpuFeaturesInitDoneGuid, > + EFI_NATIVE_INTERFACE, > + NULL > + ); > + ASSERT_EFI_ERROR (Status); > + return EFI_SUCCESS; > + } (3) Can we remove the ASSERT_EFI_ERROR() and just return Status here? No resources would be leaked, and the driver exit status would reflect the failure. (4) Just a question (not even a suggestion): can we perhaps install the protocol on ImageHandle as well? This way we could avoid allocating a new handle. > > if (PcdGetBool (PcdCpuFeaturesInitAfterSmmRelocation)) { > // > diff --git a/UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.inf b/UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.inf > index 175e8a9..b1733be 100644 > --- a/UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.inf > +++ b/UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.inf > @@ -33,6 +33,7 @@ > UefiDriverEntryPoint > UefiBootServicesTableLib > RegisterCpuFeaturesLib > + HobLib > > [Sources] > CpuFeaturesDxe.c > diff --git a/UefiCpuPkg/CpuFeatures/CpuFeaturesPei.c b/UefiCpuPkg/CpuFeatures/CpuFeaturesPei.c > index b052d55..72ee19b 100644 > --- a/UefiCpuPkg/CpuFeatures/CpuFeaturesPei.c > +++ b/UefiCpuPkg/CpuFeatures/CpuFeaturesPei.c > @@ -18,6 +18,7 @@ > #include > #include > #include > +#include > > #include > > @@ -70,6 +71,11 @@ CpuFeaturesPeimInitialize ( > Status = PeiServicesInstallPpi(&mPeiCpuFeaturesInitDonePpiDesc); > ASSERT_EFI_ERROR (Status); > > + // > + // Build HOB to let CpuFeatureDxe driver skip the initialization process. > + // > + BuildGuidHob (&gEdkiiCpuFeaturesInitDoneGuid, 0); > + > return EFI_SUCCESS; > } > > diff --git a/UefiCpuPkg/CpuFeatures/CpuFeaturesPei.inf b/UefiCpuPkg/CpuFeatures/CpuFeaturesPei.inf > index dd4b388..e617c5b 100644 > --- a/UefiCpuPkg/CpuFeatures/CpuFeaturesPei.inf > +++ b/UefiCpuPkg/CpuFeatures/CpuFeaturesPei.inf > @@ -32,6 +32,7 @@ > PeimEntryPoint > PeiServicesLib > RegisterCpuFeaturesLib > + HobLib > > [Sources] > CpuFeaturesPei.c > I think (1) and (2) should really be fixed -- but you can fix those when you push the patch. I don't insist on (3) and (4) -- if you don't want to change those parts, I'm fine with that too. With (1) and (2) updated: Reviewed-by: Laszlo Ersek Thanks! Laszlo