Re: [PATCH v2] PeiLib : Inform UEFI memory to Linux

[Udit Kumar <udit.kumar@nxp.com>]

Hi Ard, 
Sorry for coming back late on this 
Are you ok to apply this patch in edk2

Thx
Udit

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Sakar
> Arora
> Sent: Wednesday, September 20, 2017 1:50 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: edk2-devel@lists.01.org; leif.lindholm@linaro.org
> Subject: Re: [edk2] [PATCH v2] PeiLib : Inform UEFI memory to Linux
> 
> Thanks for the information. Seems my understanding was not correct in this
> context. I have no other doubts on this change.
> 
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Wednesday, September 20, 2017 12:02 PM
> To: Sakar Arora <Sakar.Arora@arm.com>
> Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; edk2-
> devel@lists.01.org; leif.lindholm@linaro.org
> Subject: Re: [edk2] [PATCH v2] PeiLib : Inform UEFI memory to Linux
> 
> On 19 September 2017 at 22:32, Sakar Arora <Sakar.Arora@arm.com> wrote:
> > The DXE core dispatcher relies on the available memory to allocate space for
> loading the rest of the modules from the UEFI image. If we declare the UEFI
> image memory space (from which DXE dispatcher reads the efi modules) open to
> allocation, it might lead to data corruption, depending on the location of UEFI
> image and cumulative size of uncompressed EFI modules.
> >
> > Also, since UEFI allows unloading and loading of drivers at runtime, IMO, there
> is a need to preserve the UEFI image even after all the modules have been
> decompressed and loaded in the boot sequence.
> >
> 
> None of this is relevant. The uncompressed firmware volume containing DXE
> core and everything else is preserved as before. The only thing that gets
> discarded is the outer FV, which only contains the PrePi SEC module, and the
> compressed FV, neither of which is ever touched again after DXE core has
> started executing. So we should not register the FV in the first place, and not
> reserve the memory so we don't lose it.
> 
> If you still think this may break anything, could you please elaborate?
> 
> 
> 
> > -----Original Message-----
> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > Sent: Tuesday, September 19, 2017 6:18 PM
> > To: Sakar Arora <Sakar.Arora@arm.com>
> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>;
> > edk2-devel@lists.01.org; leif.lindholm@linaro.org
> > Subject: Re: [edk2] [PATCH v2] PeiLib : Inform UEFI memory to Linux
> >
> > On 19 September 2017 at 01:07, Sakar Arora <Sakar.Arora@arm.com> wrote:
> >> This change will create the possibility for memory space holding the UEFI
> image to be over-written by the DXE core code, since this space will then be
> available for allocation.
> >
> > Yes. But why does this matter after the entire payload has been decompressed
> into memory already?
> >
> >
> >> Any such change, if required, should be done just before booting the OS.
> >>
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> >> Of Meenakshi Aggarwal
> >> Sent: Tuesday, September 19, 2017 6:02 PM
> >> To: edk2-devel@lists.01.org; leif.lindholm@linaro.org;
> >> ard.biesheuvel@linaro.org
> >> Subject: [edk2] [PATCH v2] PeiLib : Inform UEFI memory to Linux
> >>
> >> While creating Hob list, ArmPlatformPkg is hiding UEFI memory.
> >> whereas this memory can be used by OS.
> >>
> >> This patch, allows OS to use UEFI code area.
> >>
> >> 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>
> >> ---
> >>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c | 69
> >> -------------------------
> >>  1 file changed, 69 deletions(-)
> >>
> >> diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> >> b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> >> index 2feb11f..d03214b 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);
> >>
> >> --
> >> 1.9.1
> >>
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
> >> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
> > IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel