From: Udit Kumar <udit.kumar@nxp.com>
To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "leif.lindholm@linaro.org" <leif.lindholm@linaro.org>
Subject: Re: [PATCH 5/7] ArmPlatformPkg/MemoryInitPeiLib: don't reserve primary FV in memory
Date: Wed, 27 Dec 2017 07:37:05 +0000 [thread overview]
Message-ID: <AM6PR0402MB3334E332244601ED8E88217591070@AM6PR0402MB3334.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <7bd00d4b1fa918286d61e069fc68f6bd@mail.gmail.com>
Hi Vladimir
How re-allocation or say drivers are dispatched on your system.
Could you check addresses, where FV is kept and where this is getting dispatched
Thx
Udit
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Vladimir Olovyannikov
> Sent: Wednesday, December 27, 2017 3:22 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org
> Cc: leif.lindholm@linaro.org
> Subject: Re: [edk2] [PATCH 5/7] ArmPlatformPkg/MemoryInitPeiLib: don't
> reserve primary FV in memory
>
> Hi Ard, Meenakshi,
>
> I am having a problem I cannot explain the reason for, with this commit on
> an ARM64 platform.
>
> ArmPlatformPkg/MemoryInitPeiLib: don't reserve primary FV in memory
>
> Now that PrePi no longer exposes its internal code via special HOBs,
> we can remove the special handling of the primary FV, which needed to
> be reserved so that DXE core could call into the PE/COFF and LZMA
> libraries in the PrePi module.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Udit Kumar <udit.kumar@nxp.com>
> Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> [ardb: updated commit log]
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
> If a Shell is built "as is" from the source tree, there are no issues.
> However, if I slightly modify Shell.c like in the following patch:
>
> diff --git a/ShellPkg/Application/Shell/Shell.c
> b/ShellPkg/Application/Shell/Shell.c
> index 577e17311bea..bbbdde8ced96 100644
> --- a/ShellPkg/Application/Shell/Shell.c
> +++ b/ShellPkg/Application/Shell/Shell.c
> @@ -339,6 +339,11 @@ UefiMain (
> EFI_HANDLE ConInHandle;
> EFI_SIMPLE_TEXT_INPUT_PROTOCOL *OldConIn;
> SPLIT_LIST *Split;
> + CHAR16 *DelayStr;
> + CHAR16 *NoMapStr;
> + UINTN DelayVarSize;
> + UINTN NoMapVarSize;
> + BOOLEAN SilentStart;
>
> if (PcdGet8(PcdShellSupportLevel) > 3) {
> return (EFI_UNSUPPORTED);
> @@ -360,6 +365,7 @@ UefiMain (
> ShellInfoObject.PageBreakEnabled =
> PcdGetBool(PcdShellPageBreakDefault);
> ShellInfoObject.ViewingSettings.InsertMode =
> PcdGetBool(PcdShellInsertModeDefault);
> ShellInfoObject.LogScreenCount = PcdGet8
> (PcdShellScreenLogCount );
> + SilentStart = FALSE;
>
> //
> // verify we dont allow for spec violation @@ -452,6 +458,21 @@ UefiMain
> (
> goto FreeResources;
> }
>
> + if (!ShellInfoObject.ShellInitSettings.BitUnion.Bits.Delay) {
> + // Command line has priority over the variable
> + Status = ShellFindEnvVarInList(L"startupdelay", &DelayStr,
> &DelayVarSize, NULL);
> + if (!EFI_ERROR (Status)) {
> + ShellInfoObject.ShellInitSettings.Delay = ShellStrToUintn
> (DelayStr);
> + }
> + }
> +
> + if (!ShellInfoObject.ShellInitSettings.BitUnion.Bits.NoMap) {
> + Status = ShellFindEnvVarInList(L"silentstart", &NoMapStr,
> &NoMapVarSize, NULL);
> + if (!EFI_ERROR (Status)) {
> + SilentStart = (BOOLEAN)ShellStrToUintn (NoMapStr);
> + }
> + }
> +
> //
> // If shell support level is >= 1 create the mappings and paths
> //
> @@ -492,7 +513,7 @@ UefiMain (
> //
> // Display the version
> //
> - if (!ShellInfoObject.ShellInitSettings.BitUnion.Bits.NoVersion) {
> + if (!ShellInfoObject.ShellInitSettings.BitUnion.Bits.NoVersion &&
> !SilentStart) {
> ShellPrintHiiEx (
> 0,
> gST->ConOut->Mode->CursorRow, @@ -529,7 +550,7 @@ UefiMain (
> //
> // Display the mapping
> //
> - if (PcdGet8(PcdShellSupportLevel) >= 2 &&
> !ShellInfoObject.ShellInitSettings.BitUnion.Bits.NoMap) {
> + if (PcdGet8(PcdShellSupportLevel) >= 2 &&
> !ShellInfoObject.ShellInitSettings.BitUnion.Bits.NoMap && !SilentStart) {
> Status = RunCommand(L"map");
> ASSERT_EFI_ERROR(Status);
> }
>
> Shell fails to load.
> Here is an excerpt from the debug log:
>
> add-symbol-file
> /uefi/Build/StingrayPkg/DEBUG_GCC5/AARCH64/ShellPkg/Application/Shell/
> Shel
> l/DEBUG/Shell.dll 0x88480000
> Loading driver at 0x0008847F000 EntryPoint=0x00088480000 Shell.efi
> InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 8D095118
> ProtectUefiImageCommon - 0x8D08ED40
> - 0x000000008847F000 - 0x0000000000152000
> SetUefiImageMemoryAttributes - 0x000000008847F000 -
> 0x0000000000001000
> (0x0000000000004008)
> SetUefiImageMemoryAttributes - 0x0000000088480000 -
> 0x00000000000E6000
> (0x0000000000020008)
> SetUefiImageMemoryAttributes - 0x0000000088566000 -
> 0x000000000006B000
> (0x0000000000004008)
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8D088920
> InstallProtocolInterface: 752F3136-4E16-4FDC-A22A-E5F46812F4CA 8C71AF98
> InstallProtocolInterface: 6302D008-7F9B-4F30-87AC-60C9FEF5DA4E 88566710
> --- Blank lines -----
> 3h
> --- Blank lines -----
>
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1B18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B
> A3ABE6B398
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1B18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B
> A3ABE6B398
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1B18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B
> A3ABE6B398
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1B18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B
> A3ABE6B398
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1B18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1B18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1E18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1B18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1E18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1B18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1E18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1B18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1E18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1B18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1E18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1B18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1E18
> InstallProtocolInterface: 387477C2-69C7-11D2-8E39-00A0C969723B 8C0A1B18
> ASSERT [DxeCore]
> /uefi/MdeModulePkg/Core/Dxe/SectionExtraction/CoreSectionExtraction.c(3
> 00)
> : ((BOOLEAN)(0==1))
>
> Here 387477C2-69C7-11D2-8E39-00A0C969723B GUID is
> gEfiSimpleTextOutProtocolGuid.
>
> And there is no way to do source-level debug because FV image cannot be
> found in memory at the given location.
> As soon as I revert this commit
> (8ae5fc182941cf9ff7a222eb0a484088a0db8e2e), everything gets back to
> normal.
> Could you please explain me what I am doing wrong?
>
> Thank you,
> Vladimir
>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
> Biesheuvel
> Sent: Thursday, November 30, 2017 7:25 AM
> To: edk2-devel@lists.01.org
> Cc: leif.lindholm@linaro.org; Ard Biesheuvel
> Subject: [edk2] [PATCH 5/7] ArmPlatformPkg/MemoryInitPeiLib: don't reserve
> primary FV in memory
>
> From: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
>
> Now that PrePi no longer exposes its internal code via special HOBs, we can
> remove the special handling of the primary FV, which needed to be reserved
> so that DXE core could call into the PE/COFF and LZMA libraries in the PrePi
> module.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Udit Kumar <udit.kumar@nxp.com>
> Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> [ardb: updated commit log]
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c | 69 --------------------
> 1 file changed, 69 deletions(-)
>
> diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> index 2feb11f21d5d..d03214b5df66 100644
> --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> @@ -70,11 +70,7 @@ MemoryPeim (
> {
> ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable;
> EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
> - UINT64 ResourceLength;
> EFI_PEI_HOB_POINTERS NextHob;
> - EFI_PHYSICAL_ADDRESS FdTop;
> - EFI_PHYSICAL_ADDRESS SystemMemoryTop;
> - EFI_PHYSICAL_ADDRESS ResourceTop;
> BOOLEAN Found;
>
> // Get Virtual Memory Map from the Platform Library @@ -121,71 +117,6
> @@ MemoryPeim (
> );
> }
>
> - //
> - // Reserved the memory space occupied by the firmware volume
> - //
> -
> - SystemMemoryTop = (EFI_PHYSICAL_ADDRESS)PcdGet64
> (PcdSystemMemoryBase)
> + (EFI_PHYSICAL_ADDRESS)PcdGet64 (PcdSystemMemorySize);
> - FdTop = (EFI_PHYSICAL_ADDRESS)PcdGet64 (PcdFdBaseAddress) +
> (EFI_PHYSICAL_ADDRESS)PcdGet32 (PcdFdSize);
> -
> - // EDK2 does not have the concept of boot firmware copied into DRAM. To
> avoid the DXE
> - // core to overwrite this area we must mark the region with the attribute
> non-present
> - if ((PcdGet64 (PcdFdBaseAddress) >= PcdGet64 (PcdSystemMemoryBase))
> && (FdTop <= SystemMemoryTop)) {
> - Found = FALSE;
> -
> - // Search for System Memory Hob that contains the firmware
> - NextHob.Raw = GetHobList ();
> - while ((NextHob.Raw = GetNextHob
> (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR,
> NextHob.Raw)) != NULL) {
> - if ((NextHob.ResourceDescriptor->ResourceType ==
> EFI_RESOURCE_SYSTEM_MEMORY) &&
> - (PcdGet64 (PcdFdBaseAddress) >=
> NextHob.ResourceDescriptor->PhysicalStart) &&
> - (FdTop <= NextHob.ResourceDescriptor->PhysicalStart +
> NextHob.ResourceDescriptor->ResourceLength))
> - {
> - ResourceAttributes =
> NextHob.ResourceDescriptor->ResourceAttribute;
> - ResourceLength = NextHob.ResourceDescriptor->ResourceLength;
> - ResourceTop = NextHob.ResourceDescriptor->PhysicalStart +
> ResourceLength;
> -
> - if (PcdGet64 (PcdFdBaseAddress) ==
> NextHob.ResourceDescriptor->PhysicalStart) {
> - if (SystemMemoryTop == FdTop) {
> - NextHob.ResourceDescriptor->ResourceAttribute =
> ResourceAttributes & ~EFI_RESOURCE_ATTRIBUTE_PRESENT;
> - } else {
> - // Create the System Memory HOB for the firmware with the
> non-present attribute
> - BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> - ResourceAttributes &
> ~EFI_RESOURCE_ATTRIBUTE_PRESENT,
> - PcdGet64 (PcdFdBaseAddress),
> - PcdGet32 (PcdFdSize));
> -
> - // Top of the FD is system memory available for UEFI
> - NextHob.ResourceDescriptor->PhysicalStart +=
> PcdGet32(PcdFdSize);
> - NextHob.ResourceDescriptor->ResourceLength -=
> PcdGet32(PcdFdSize);
> - }
> - } else {
> - // Create the System Memory HOB for the firmware with the
> non-present attribute
> - BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> - ResourceAttributes &
> ~EFI_RESOURCE_ATTRIBUTE_PRESENT,
> - PcdGet64 (PcdFdBaseAddress),
> - PcdGet32 (PcdFdSize));
> -
> - // Update the HOB
> - NextHob.ResourceDescriptor->ResourceLength = PcdGet64
> (PcdFdBaseAddress) - NextHob.ResourceDescriptor->PhysicalStart;
> -
> - // If there is some memory available on the top of the FD then
> create a HOB
> - if (FdTop < NextHob.ResourceDescriptor->PhysicalStart +
> ResourceLength) {
> - // Create the System Memory HOB for the remaining region (top
> of the FD)
> - BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> - ResourceAttributes,
> - FdTop,
> - ResourceTop - FdTop);
> - }
> - }
> - Found = TRUE;
> - break;
> - }
> - NextHob.Raw = GET_NEXT_HOB (NextHob);
> - }
> -
> - ASSERT(Found);
> - }
> -
> // Build Memory Allocation Hob
> InitMmu (MemoryTable);
>
> --
> 2.11.0
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.
> 01.org%2Fmailman%2Flistinfo%2Fedk2-
> devel&data=02%7C01%7Cudit.kumar%40nxp.com%7Ca75148c0714749bda72
> d08d54caaf73b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6364
> 99219600898902&sdata=nmhLohXfQGCJE3F%2FDan1YPZCcgbCZ6yyxcZsdZ71h
> P8%3D&reserved=0
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.
> 01.org%2Fmailman%2Flistinfo%2Fedk2-
> devel&data=02%7C01%7Cudit.kumar%40nxp.com%7Ca75148c0714749bda72
> d08d54caaf73b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6364
> 99219600898902&sdata=nmhLohXfQGCJE3F%2FDan1YPZCcgbCZ6yyxcZsdZ71h
> P8%3D&reserved=0
next prev parent reply other threads:[~2017-12-27 7:32 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-30 15:24 [PATCH 0/7] ArmPlatformPkg/PrePi: stop exposing internal code via HOBs Ard Biesheuvel
2017-11-30 15:24 ` [PATCH 1/7] EmbeddedPkg BeagleBoardPkg: move special HOB reuse libraries into platform Ard Biesheuvel
2017-11-30 15:24 ` [PATCH 2/7] BeagleBoardPkg: create private PrePi implementation Ard Biesheuvel
2017-11-30 15:24 ` [PATCH 3/7] BeagleBoardPkg: clone MemoryInitPeiLib Ard Biesheuvel
2017-11-30 15:24 ` [PATCH 4/7] ArmPlatformPkg/PrePi: don't expose PE/COFF and LZMA libraries via HOBs Ard Biesheuvel
2017-11-30 15:24 ` [PATCH 5/7] ArmPlatformPkg/MemoryInitPeiLib: don't reserve primary FV in memory Ard Biesheuvel
2017-12-26 21:52 ` Vladimir Olovyannikov
2017-12-26 23:06 ` Ard Biesheuvel
2017-12-27 1:58 ` Vladimir Olovyannikov
2017-12-27 2:01 ` Vladimir Olovyannikov
2017-12-27 7:37 ` Udit Kumar [this message]
2017-11-30 15:24 ` [PATCH 6/7] ArmPlatformPkg/PrePi; call all constructors by hand Ard Biesheuvel
2017-11-30 16:31 ` Leif Lindholm
2017-11-30 16:35 ` Ard Biesheuvel
2017-11-30 16:45 ` Leif Lindholm
2017-11-30 17:12 ` Ard Biesheuvel
2017-11-30 15:24 ` [PATCH 7/7] ArmPlatformPkg/PrePi: remove bogus IntelFrameworkModulePkg reference Ard Biesheuvel
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=AM6PR0402MB3334E332244601ED8E88217591070@AM6PR0402MB3334.eurprd04.prod.outlook.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