public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
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



      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