From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::233; helo=mail-it0-x233.google.com; envelope-from=vladimir.olovyannikov@broadcom.com; receiver=edk2-devel@lists.01.org Received: from mail-it0-x233.google.com (mail-it0-x233.google.com [IPv6:2607:f8b0:4001:c0b::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id DD77B2035BA12 for ; Tue, 26 Dec 2017 17:57:04 -0800 (PST) Received: by mail-it0-x233.google.com with SMTP id x28so23818622ita.0 for ; Tue, 26 Dec 2017 18:01:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=from:references:in-reply-to:mime-version:thread-index:date :message-id:subject:to:cc; bh=RNffvvEBAaqz1DmrNMOd90h7fT+0eUi+VGkQ2puyUwY=; b=LZg6FHBh/eTSnNdvDgndr/scfEnOqUR5A2ZCP3F+yTnBr+9q8uwLvEIpBmLBXfJehg Ws5sn7OtP1/iNlXp3LPv0d59eD71n5sWFn3O8JRm+IZ5XHQMryExbSKhKcjDcPqH2rtV xL6esHzBwzbLmEJQfWW0BL2cFgi4/DXSEDyVo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:references:in-reply-to:mime-version :thread-index:date:message-id:subject:to:cc; bh=RNffvvEBAaqz1DmrNMOd90h7fT+0eUi+VGkQ2puyUwY=; b=aaYJdEbB7pfFM4UCSWzjpEgk+2c08TF3iSTW6eLFlFUyawGdbLk8x3Ekp9mwBbPzVD xonJeN2Ajvp+SkbqFkwDr8xCvHq93SesQlAvkyQnA5BiKM/0oH2wd4BXLJW3lbjNxk8c dnGwhke2y1CbF2Y8Yx+eO/iXASzoYmoc3+WJKxghuV7njsmBNHyT+9ns6Dy4Wgt8hDri 6vX1x0DGe3BugbK9Vtc4CfcRVBujDEBmxesxfyz9kdoxMbgMGGfuUTa+tpiYt/CLfqMJ ctkLzBM01HlrMVuYMaGM2lE8Qd+19weSEcv261+AIN2fJ3pfHgjDcdtPYHOqZVppkVsP EG8A== X-Gm-Message-State: AKGB3mLeQ4JJSHkHPK/Jciyl8j7kJWUWvTg0F+mF5w37x/yKH5Bg9ZPn 39vPU38zEVPZAQ3iB5LuZ/okVPL4LLm7lQmIqjU7/w== X-Google-Smtp-Source: ACJfBovbZduqLoHxKrpk3psWF+J2JJ1m1wN2JBx5622y9bBbKw9NDRDDUTPxacCvvfP+68VIKdYZeoq+x8YVCqK7wgk= X-Received: by 10.36.245.133 with SMTP id k127mr35158099ith.136.1514340119115; Tue, 26 Dec 2017 18:01:59 -0800 (PST) From: Vladimir Olovyannikov References: <20171130152453.19205-1-ard.biesheuvel@linaro.org> <20171130152453.19205-6-ard.biesheuvel@linaro.org> <7bd00d4b1fa918286d61e069fc68f6bd@mail.gmail.com> a7cd5300b3b0060a4eabefd0c81e253e@mail.gmail.com In-Reply-To: a7cd5300b3b0060a4eabefd0c81e253e@mail.gmail.com MIME-Version: 1.0 X-Mailer: Microsoft Outlook 14.0 Thread-Index: AQIJ89iUZB5DcK378Uzg5GRKSQIkagJS9IeBAjwtmj4CC6h+PqK0qxMQgAAD+aA= Date: Tue, 26 Dec 2017 18:01:58 -0800 Message-ID: To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, Leif Lindholm Subject: Re: [PATCH 5/7] ArmPlatformPkg/MemoryInitPeiLib: don't reserve primary FV in memory X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Dec 2017 01:57:05 -0000 Content-Type: text/plain; charset="UTF-8" > -----Original Message----- > From: Vladimir Olovyannikov [mailto:vladimir.olovyannikov@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.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 > > 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 > > > Signed-off-by: Meenakshi Aggarwal > > > [ardb: updated commit log] > > > Signed-off-by: Ard Biesheuvel > > > Reviewed-by: Leif Lindholm > > > > > > 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