> -----Original Message-----
> From: Vladimir Olovyannikov [mailto:vladimir.olovyanni...@broadcom.com]
> Sent: Tuesday, December 26, 2017 5:59 PM
> To: 'Ard Biesheuvel'
> Cc: 'edk2-devel@lists.01.org'; 'Leif Lindholm'
> Subject: RE: [edk2] [PATCH 5/7] ArmPlatformPkg/MemoryInitPeiLib: don't
> reserve primary FV in memory
>
> > -----Original Message-----
> > From: Ard Biesheuvel [mailto:ard.biesheu...@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.olovyanni...@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.ku...@nxp.com>
> > > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>
> > > [ardb: updated commit log]
> > > Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> > > Reviewed-by: Leif Lindholm <leif.lindh...@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...
>
To clarify, PreparememoryResourceHob looks like this:
BOOLEAN
PrepareMemoryResourceHob (
IN EFI_PHYSICAL_ADDRESS Address,
IN UINT64 MemSize,
IN UINT64 MaxAllowedSize,
UINTN MemType
)
{
BOOLEAN HobCreated;
HobCreated = FALSE;
if ((MemSize > 0) && (MemSize <= MaxAllowedSize)) {
// Additional memory is available. Declare it.
EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
ResourceAttributes = SYSTEM_MEMORY_ATTRS;
if (MemType != EFI_RESOURCE_SYSTEM_MEMORY) {
ResourceAttributes = RESERVED_MEMORY_ATTRS;
}
BuildResourceDescriptorHob (
MemType,
ResourceAttributes,
Address,
MemSize);
HobCreated = TRUE;
}
return HobCreated;
}
And SYSTEM_MEMORY_ATTRS:
#define RESERVED_MEMORY_ATTRS \
EFI_RESOURCE_ATTRIBUTE_PRESENT | \
EFI_RESOURCE_ATTRIBUTE_WRITE_PROTECTED | \
EFI_RESOURCE_ATTRIBUTE_WRITE_PROTECTABLE | \
EFI_RESOURCE_ATTRIBUTE_INITIALIZED | \
EFI_RESOURCE_ATTRIBUTE_READ_PROTECTED | \
EFI_RESOURCE_ATTRIBUTE_READ_PROTECTABLE | \
EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE | \
EFI_RESOURCE_ATTRIBUTE_TESTED
> Thank you,
> Vladimir
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
