From: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: edk2-devel@lists.01.org, Leif Lindholm <leif.lindholm@linaro.org>
Subject: Re: [PATCH 5/7] ArmPlatformPkg/MemoryInitPeiLib: don't reserve primary FV in memory
Date: Tue, 26 Dec 2017 17:58:48 -0800 [thread overview]
Message-ID: <a7cd5300b3b0060a4eabefd0c81e253e@mail.gmail.com> (raw)
In-Reply-To: <CAKv+Gu8vvw0RgEb+05k-57W8csEfmM7LpAht56+H5ODVVixv+w@mail.gmail.com>
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Tuesday, December 26, 2017 3:07 PM
> To: Vladimir Olovyannikov
> Cc: edk2-devel@lists.01.org; Leif Lindholm
> Subject: Re: [edk2] [PATCH 5/7] ArmPlatformPkg/MemoryInitPeiLib: don't
> reserve primary FV in memory
>
> On 26 December 2017 at 21:52, Vladimir Olovyannikov
> <vladimir.olovyannikov@broadcom.com> wrote:
> > 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(
> > 300)
> > : ((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?
> >
>
> Does this help?
>
> --- a/ArmPlatformPkg/PlatformPei/PlatformPeiLib.c
> +++ b/ArmPlatformPkg/PlatformPei/PlatformPeiLib.c
> @@ -24,7 +24,7 @@ PlatformPeim (
> VOID
> )
> {
> - BuildFvHob (PcdGet64 (PcdFvBaseAddress), PcdGet32 (PcdFvSize));
> + //BuildFvHob (PcdGet64 (PcdFvBaseAddress), PcdGet32 (PcdFvSize));
>
> return EFI_SUCCESS;
> }
>
> (I assume you are using PrePi, and have nothing except the PrePi module in
> the primary FV)
Thanks for response Ard,
No, this does not help.
In ArmPlatformGetVirtualMemoryMap () the platform reads a Pcd describing
SDRAM memory configuration (regions),
For each described region it creates a memory resource HOB like this:
if (PrepareMemoryResourceHob (
MyDDRInfo[Index].Address,
MyDDRInfo[Index].Size,
MyDDRInfo[Index].Size + 1,
Reserve ? EFI_RESOURCE_MEMORY_RESERVED : EFI_RESOURCE_SYSTEM_MEMORY
)) {
UINTN SizeGB;
TotalHighMemAdded += MyDDRInfo[Index].Size;
SizeGB = MyDDRInfo[Index].Size >> 30;
DEBUG((
EFI_D_INFO,
"HighMemMap: Added DDR %a area (%d). Start address: 0x%llx, size
%llu %a (0x%llx)\n",
Reserve ? "reserved" : "highmem",
Index + 1,
MyDDRInfo[Index].Address,
SizeGB ? SizeGB : MyDDRInfo[Index].Size >> 20,
SizeGB ? "GiB" : "MiB",
MyDDRInfo[Index].Size
));
Index++;
}
} else {
MyDDRInfo[Index].Address = 0;
MyDDRInfo[Index].Size = 0;
}
Thus it reports described and filled in areas of memory like this:
HighMemMap: Added DDR highmem area (1). Start address: 0x8F101000, size 1
GiB (0x70EFF000)
HighMemMap: Added DDR highmem area (2). Start address: 0x880000000, size 14
GiB (0x380000000)
HighMemMap: Added DDR highmem area (3). Start address: 0x9000000000, size 16
GiB (0x400000000)
HighMemMap: Added DDR highmem area (4). Start address: 0xA000000000, size 16
GiB (0x400000000)
HighMemMap: Added DDR reserved area (1). Start address: 0x8F000000, size 1
MiB (0x101000)
IS this not the right way to describe memory HOBs?
With your proposed change assertion happens very early, here:
ASSERT_EFI_ERROR (Status = Not Found)
ASSERT [ArmPlatformPrePiMPCore] /uefi/ArmPlatformPkg/PrePi/PrePi.c(157):
!EFI_ERROR (Status)
So it cannot DecompressFirstFv().
If I don't apply your suggested change and revert the commit I mentioned
earlier, it works fine...
Thank you,
Vladimir
next prev parent reply other threads:[~2017-12-27 1:53 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 [this message]
2017-12-27 2:01 ` Vladimir Olovyannikov
2017-12-27 7:37 ` Udit Kumar
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=a7cd5300b3b0060a4eabefd0c81e253e@mail.gmail.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