From: "Ni, Ray" <ray.ni@intel.com>
To: "Chiu, Chasel" <chasel.chiu@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Dong, Eric" <eric.dong@intel.com>, Laszlo Ersek <lersek@redhat.com>
Subject: Re: [PATCH] UefiCpuPkg/SecCore: Wrong Debug Information for SecCore
Date: Tue, 19 Feb 2019 09:23:14 +0000 [thread overview]
Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C02526D@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <20190219074415.11044-1-chasel.chiu@intel.com>
Chasel,
Please change the two FindAndReportEntryPoints() calls in SecStartupPhase2() so that
their line length doesn't exceed 120.
With the modification, Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Chiu, Chasel <chasel.chiu@intel.com>
> Sent: Tuesday, February 19, 2019 3:44 PM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; Chiu, Chasel <chasel.chiu@intel.com>
> Subject: [PATCH] UefiCpuPkg/SecCore: Wrong Debug Information for
> SecCore
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1533
>
> When SecCore and PeiCore in different FV, current implementation still
> assuming SecCore and PeiCore are in the same FV.
> To fix this issue 2 FVs will be input parameters for FindAndReportEntryPoints
> () and SecCore and PeiCore will be found in each FV and correct debug
> information will be reported.
>
> Test: Booted with internal platform successfully.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
> ---
> UefiCpuPkg/SecCore/FindPeiCore.c | 60
> ++++++++++++++++++++++++++++++------------------------------
> UefiCpuPkg/SecCore/SecMain.c | 10 ++++++++--
> UefiCpuPkg/SecCore/SecMain.h | 8 +++++---
> 3 files changed, 43 insertions(+), 35 deletions(-)
>
> diff --git a/UefiCpuPkg/SecCore/FindPeiCore.c
> b/UefiCpuPkg/SecCore/FindPeiCore.c
> index bb9c434d1e..6f2046ad95 100644
> --- a/UefiCpuPkg/SecCore/FindPeiCore.c
> +++ b/UefiCpuPkg/SecCore/FindPeiCore.c
> @@ -1,7 +1,7 @@
> /** @file
> Locate the entry point for the PEI Core
>
> - Copyright (c) 2008 - 2011, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2008 - 2019, Intel Corporation. All rights
> + reserved.<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
> License
> which accompanies this distribution. The full text of the license may be
> found at @@ -19,17 +19,17 @@
> /**
> Find core image base.
>
> - @param BootFirmwareVolumePtr Point to the boot firmware volume.
> - @param SecCoreImageBase The base address of the SEC core image.
> - @param PeiCoreImageBase The base address of the PEI core image.
> + @param FirmwareVolumePtr Point to the firmware volume for finding
> core image.
> + @param FileType The FileType for searching, either SecCore or
> PeiCore.
> + @param CoreImageBase The base address of the core image.
>
> **/
> EFI_STATUS
> EFIAPI
> FindImageBase (
> - IN EFI_FIRMWARE_VOLUME_HEADER *BootFirmwareVolumePtr,
> - OUT EFI_PHYSICAL_ADDRESS *SecCoreImageBase,
> - OUT EFI_PHYSICAL_ADDRESS *PeiCoreImageBase
> + IN EFI_FIRMWARE_VOLUME_HEADER *FirmwareVolumePtr,
> + IN EFI_FV_FILETYPE FileType,
> + OUT EFI_PHYSICAL_ADDRESS *CoreImageBase
> )
> {
> EFI_PHYSICAL_ADDRESS CurrentAddress;
> @@ -40,16 +40,15 @@ FindImageBase (
> EFI_COMMON_SECTION_HEADER *Section;
> EFI_PHYSICAL_ADDRESS EndOfSection;
>
> - *SecCoreImageBase = 0;
> - *PeiCoreImageBase = 0;
> + *CoreImageBase = 0;
>
> - CurrentAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)
> BootFirmwareVolumePtr;
> - EndOfFirmwareVolume = CurrentAddress + BootFirmwareVolumePtr-
> >FvLength;
> + CurrentAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) FirmwareVolumePtr;
> + EndOfFirmwareVolume = CurrentAddress + FirmwareVolumePtr-
> >FvLength;
>
> //
> // Loop through the FFS files in the Boot Firmware Volume
> //
> - for (EndOfFile = CurrentAddress + BootFirmwareVolumePtr-
> >HeaderLength; ; ) {
> + for (EndOfFile = CurrentAddress + FirmwareVolumePtr->HeaderLength; ;
> + ) {
>
> CurrentAddress = (EndOfFile + 7) & 0xfffffffffffffff8ULL;
> if (CurrentAddress > EndOfFirmwareVolume) { @@ -75,10 +74,9 @@
> FindImageBase (
> }
>
> //
> - // Look for SEC Core / PEI Core files
> + // Look for particular Core file (either SEC Core or PEI Core)
> //
> - if (File->Type != EFI_FV_FILETYPE_SECURITY_CORE &&
> - File->Type != EFI_FV_FILETYPE_PEI_CORE) {
> + if (File->Type != FileType) {
> continue;
> }
>
> @@ -115,17 +113,11 @@ FindImageBase (
> // Look for executable sections
> //
> if (Section->Type == EFI_SECTION_PE32 || Section->Type ==
> EFI_SECTION_TE) {
> - if (File->Type == EFI_FV_FILETYPE_SECURITY_CORE) {
> + if (File->Type == FileType) {
> if (IS_SECTION2 (Section)) {
> - *SecCoreImageBase = (PHYSICAL_ADDRESS) (UINTN) ((UINT8 *)
> Section + sizeof (EFI_COMMON_SECTION_HEADER2));
> + *CoreImageBase = (PHYSICAL_ADDRESS) (UINTN) ((UINT8 *)
> + Section + sizeof (EFI_COMMON_SECTION_HEADER2));
> } else {
> - *SecCoreImageBase = (PHYSICAL_ADDRESS) (UINTN) ((UINT8 *)
> Section + sizeof (EFI_COMMON_SECTION_HEADER));
> - }
> - } else {
> - if (IS_SECTION2 (Section)) {
> - *PeiCoreImageBase = (PHYSICAL_ADDRESS) (UINTN) ((UINT8 *)
> Section + sizeof (EFI_COMMON_SECTION_HEADER2));
> - } else {
> - *PeiCoreImageBase = (PHYSICAL_ADDRESS) (UINTN) ((UINT8 *)
> Section + sizeof (EFI_COMMON_SECTION_HEADER));
> + *CoreImageBase = (PHYSICAL_ADDRESS) (UINTN) ((UINT8 *)
> + Section + sizeof (EFI_COMMON_SECTION_HEADER));
> }
> }
> break;
> @@ -133,9 +125,9 @@ FindImageBase (
> }
>
> //
> - // Both SEC Core and PEI Core images found
> + // Either SEC Core or PEI Core images found
> //
> - if (*SecCoreImageBase != 0 && *PeiCoreImageBase != 0) {
> + if (*CoreImageBase != 0) {
> return EFI_SUCCESS;
> }
> }
> @@ -147,14 +139,16 @@ FindImageBase (
> It also find SEC and PEI Core file debug information. It will report them if
> remote debug is enabled.
>
> - @param BootFirmwareVolumePtr Point to the boot firmware volume.
> + @param SecCoreFirmwareVolumePtr Point to the firmware volume for
> finding SecCore.
> + @param PeiCoreFirmwareVolumePtr Point to the firmware volume for
> finding PeiCore.
> @param PeiCoreEntryPoint The entry point of the PEI core.
>
> **/
> VOID
> EFIAPI
> FindAndReportEntryPoints (
> - IN EFI_FIRMWARE_VOLUME_HEADER *BootFirmwareVolumePtr,
> + IN EFI_FIRMWARE_VOLUME_HEADER *SecCoreFirmwareVolumePtr,
> + IN EFI_FIRMWARE_VOLUME_HEADER *PeiCoreFirmwareVolumePtr,
> OUT EFI_PEI_CORE_ENTRY_POINT *PeiCoreEntryPoint
> )
> {
> @@ -164,9 +158,9 @@ FindAndReportEntryPoints (
> PE_COFF_LOADER_IMAGE_CONTEXT ImageContext;
>
> //
> - // Find SEC Core and PEI Core image base
> + // Find SEC Core image base
> //
> - Status = FindImageBase (BootFirmwareVolumePtr, &SecCoreImageBase,
> &PeiCoreImageBase);
> + Status = FindImageBase (SecCoreFirmwareVolumePtr,
> + EFI_FV_FILETYPE_SECURITY_CORE, &SecCoreImageBase);
> ASSERT_EFI_ERROR (Status);
>
> ZeroMem ((VOID *) &ImageContext, sizeof
> (PE_COFF_LOADER_IMAGE_CONTEXT)); @@ -178,6 +172,12 @@
> FindAndReportEntryPoints (
> PeCoffLoaderRelocateImageExtraAction (&ImageContext);
>
> //
> + // Find PEI Core image base
> + //
> + Status = FindImageBase (PeiCoreFirmwareVolumePtr,
> + EFI_FV_FILETYPE_PEI_CORE, &PeiCoreImageBase); ASSERT_EFI_ERROR
> + (Status);
> +
> + //
> // Report PEI Core debug information when remote debug is enabled
> //
> ImageContext.ImageAddress = PeiCoreImageBase; diff --git
> a/UefiCpuPkg/SecCore/SecMain.c b/UefiCpuPkg/SecCore/SecMain.c index
> 752156a98e..e4961fd7c6 100644
> --- a/UefiCpuPkg/SecCore/SecMain.c
> +++ b/UefiCpuPkg/SecCore/SecMain.c
> @@ -249,7 +249,10 @@ SecStartupPhase2(
> (PpiList[Index].Flags & EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST) !=
> EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST;
> Index++) {
> if (CompareGuid (PpiList[Index].Guid, &gEfiPeiCoreFvLocationPpiGuid)
> && (((EFI_PEI_CORE_FV_LOCATION_PPI *) PpiList[Index].Ppi)-
> >PeiCoreFvLocation != 0)) {
> - FindAndReportEntryPoints ((EFI_FIRMWARE_VOLUME_HEADER *)
> ((EFI_PEI_CORE_FV_LOCATION_PPI *) PpiList[Index].Ppi)-
> >PeiCoreFvLocation, &PeiCoreEntryPoint);
> + //
> + // In this case, SecCore is in BFV but PeiCore is in another FV reported
> by PPI.
> + //
> + FindAndReportEntryPoints ((EFI_FIRMWARE_VOLUME_HEADER *)
> + SecCoreData->BootFirmwareVolumeBase,
> (EFI_FIRMWARE_VOLUME_HEADER *)
> + ((EFI_PEI_CORE_FV_LOCATION_PPI *)
> + PpiList[Index].Ppi)->PeiCoreFvLocation, &PeiCoreEntryPoint);
> if (PeiCoreEntryPoint != NULL) {
> break;
> } else {
> @@ -265,7 +268,10 @@ SecStartupPhase2(
> // If EFI_PEI_CORE_FV_LOCATION_PPI not found, try to locate PeiCore
> from BFV.
> //
> if (PeiCoreEntryPoint == NULL) {
> - FindAndReportEntryPoints ((EFI_FIRMWARE_VOLUME_HEADER *)
> SecCoreData->BootFirmwareVolumeBase, &PeiCoreEntryPoint);
> + //
> + // Both SecCore and PeiCore are in BFV.
> + //
> + FindAndReportEntryPoints ((EFI_FIRMWARE_VOLUME_HEADER *)
> + SecCoreData->BootFirmwareVolumeBase,
> (EFI_FIRMWARE_VOLUME_HEADER *)
> + SecCoreData->BootFirmwareVolumeBase, &PeiCoreEntryPoint);
> if (PeiCoreEntryPoint == NULL) {
> CpuDeadLoop ();
> }
> diff --git a/UefiCpuPkg/SecCore/SecMain.h
> b/UefiCpuPkg/SecCore/SecMain.h index 80045d34f4..6f3b0c5d12 100644
> --- a/UefiCpuPkg/SecCore/SecMain.h
> +++ b/UefiCpuPkg/SecCore/SecMain.h
> @@ -89,14 +89,16 @@ SecStartup (
> It also find SEC and PEI Core file debug information. It will report them if
> remote debug is enabled.
>
> - @param BootFirmwareVolumePtr Point to the boot firmware volume.
> - @param PeiCoreEntryPoint Point to the PEI core entry point.
> + @param SecCoreFirmwareVolumePtr Point to the firmware volume for
> finding SecCore.
> + @param PeiCoreFirmwareVolumePtr Point to the firmware volume for
> finding PeiCore.
> + @param PeiCoreEntryPoint The entry point of the PEI core.
>
> **/
> VOID
> EFIAPI
> FindAndReportEntryPoints (
> - IN EFI_FIRMWARE_VOLUME_HEADER *BootFirmwareVolumePtr,
> + IN EFI_FIRMWARE_VOLUME_HEADER *SecCoreFirmwareVolumePtr,
> + IN EFI_FIRMWARE_VOLUME_HEADER *PeiCoreFirmwareVolumePtr,
> OUT EFI_PEI_CORE_ENTRY_POINT *PeiCoreEntryPoint
> );
>
> --
> 2.13.3.windows.1
prev parent reply other threads:[~2019-02-19 9:23 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-19 7:44 [PATCH] UefiCpuPkg/SecCore: Wrong Debug Information for SecCore Chasel, Chiu
2019-02-19 9:23 ` Ni, Ray [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=734D49CCEBEEF84792F5B80ED585239D5C02526D@SHSMSX104.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox