subject:"\[edk2\] \[PATCH\] ArmPlatformPkg\/MemoryInitPeiLib\: mark primary FV region as boot services data"

Re: [edk2] [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region as boot services data

2018-02-28 Thread Ard Biesheuvel

On 28 February 2018 at 15:03, Leif Lindholm  wrote:
> On Tue, Jan 02, 2018 at 03:50:34PM +, Ard Biesheuvel wrote:
>> Commit 8ae5fc182941 ("ArmPlatformPkg/MemoryInitPeiLib: don't reserve
>> primary FV in memory") deleted the code that removes the memory covering
>> the primary firmware volume from the memory map. The assumption was that
>> this is no longer necessary now that we no longer expose compression and
>> PE/COFF loader library code from the PrePi module to DXE core.
>>
>> However, the FV is still declared, and so code may attempt to access it
>> anyway, which may cause unexpected results depending on whether the
>> memory has been reused for other purposes in the mean time.
>>
>> So reinstate the code that splits off the resource descriptor HOB that
>> describes the firmware device, but this time, don't mark the memory as
>> unusable, but create a memory allocation HOB that marks the region as
>> boot services data.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel 
>
> So, this looks like a necessary fix.
> However, there are many line-length violations below.
> And it's not apparent how fixing that would make the code more
> readable.
>
> So rather than asking for a rewrite - could you split the revert
> from the modification?
>

Sure.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region as boot services data

2018-02-28 Thread Leif Lindholm

On Tue, Jan 02, 2018 at 03:50:34PM +, Ard Biesheuvel wrote:
> Commit 8ae5fc182941 ("ArmPlatformPkg/MemoryInitPeiLib: don't reserve
> primary FV in memory") deleted the code that removes the memory covering
> the primary firmware volume from the memory map. The assumption was that
> this is no longer necessary now that we no longer expose compression and
> PE/COFF loader library code from the PrePi module to DXE core.
> 
> However, the FV is still declared, and so code may attempt to access it
> anyway, which may cause unexpected results depending on whether the
> memory has been reused for other purposes in the mean time.
> 
> So reinstate the code that splits off the resource descriptor HOB that
> describes the firmware device, but this time, don't mark the memory as
> unusable, but create a memory allocation HOB that marks the region as
> boot services data.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 

So, this looks like a necessary fix.
However, there are many line-length violations below.
And it's not apparent how fixing that would make the code more
readable.

So rather than asking for a rewrite - could you split the revert
from the modification?

/
Leif

> Vladimir, Udit, Meenakshi: please confirm whether this code works for you.
> 
>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c | 74 
>  1 file changed, 74 insertions(+)
> 
> diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c 
> b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> index d03214b5df66..d1b5c0be9497 100644
> --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> @@ -70,7 +70,11 @@ 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
> @@ -117,6 +121,76 @@ MemoryPeim (
>  );
>}
>  
> +  //
> +  // Reserve 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 create a memory allocation HOB for 
> the region,
> +  // but this only works if we split off the underlying resource descriptor 
> as well.
> +  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) {
> +// Create the System Memory HOB for the firmware with the 
> non-present attribute
> +BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> +ResourceAttributes,
> +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
> +  BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> +  ResourceAttributes,
> +  PcdGet64 (PcdFdBaseAddress),
> +  PcdGet32 (PcdFdSize));
> +
> +  // Update the HOB
> +  NextHob.ResourceDescriptor->ResourceLength = PcdGet64 
>

Re: [edk2] [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region as boot services data

2018-02-01 Thread Ard Biesheuvel

Leif,

On 3 January 2018 at 07:44, Ard Biesheuvel  wrote:
> On 3 January 2018 at 05:09, Udit Kumar  wrote:
>> Thanks Ard,
>> This works for us as well
>> Few comments inline
>>
>>
>> Regards
>> Udit
>>
>>> -Original Message-
>>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>>> Sent: Tuesday, January 02, 2018 9:21 PM
>>> To: edk2-devel@lists.01.org
>>> Cc: leif.lindh...@linaro.org; vladimir.olovyanni...@broadcom.com; Udit
>>> Kumar ; Meenakshi Aggarwal
>>> ; Ard Biesheuvel
>>> 
>>> Subject: [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region
>>> as boot services data
>>>
>>> Commit 8ae5fc182941 ("ArmPlatformPkg/MemoryInitPeiLib: don't reserve
>>> primary FV in memory") deleted the code that removes the memory covering
>>> the primary firmware volume from the memory map. The assumption was
>>> that
>>> this is no longer necessary now that we no longer expose compression and
>>> PE/COFF loader library code from the PrePi module to DXE core.
>>>
>>> However, the FV is still declared, and so code may attempt to access it
>>> anyway, which may cause unexpected results depending on whether the
>>> memory has been reused for other purposes in the mean time.
>>>
>>> So reinstate the code that splits off the resource descriptor HOB that
>>> describes the firmware device, but this time, don't mark the memory as
>>> unusable, but create a memory allocation HOB that marks the region as
>>> boot services data.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel 
>>> ---
>>> Vladimir, Udit, Meenakshi: please confirm whether this code works for you.
>>>
>>>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c | 74
>>> 
>>>  1 file changed, 74 insertions(+)
>>>
>>> diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
>>> b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
>>> index d03214b5df66..d1b5c0be9497 100644
>>> --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
>>> +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
>>> @@ -70,7 +70,11 @@ 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
>>> @@ -117,6 +121,76 @@ MemoryPeim (
>>>  );
>>>}
>>>
>>> +  //
>>> +  // Reserve 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 create a memory allocation HOB for
>>> the region,
>>> +  // but this only works if we split off the underlying resource 
>>> descriptor as
>>> well.
>>> +  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) {
>>> +// Create the System Memory HOB for the firmware with the non-
>>> present attribute
>>
>> Please correct comments, now this memory is present
>>
>
> Yes
>
>>> +BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
>>> +ResourceAttributes,
>>> +PcdGet64 (PcdFdBaseAddress),
>>> +PcdGet32 (PcdFdSize));
>>> +
>>> +// Top of the FD is system memory available

Re: [edk2] [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region as boot services data

2018-01-02 Thread Ard Biesheuvel

On 3 January 2018 at 05:09, Udit Kumar  wrote:
> Thanks Ard,
> This works for us as well
> Few comments inline
>
>
> Regards
> Udit
>
>> -Original Message-
>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> Sent: Tuesday, January 02, 2018 9:21 PM
>> To: edk2-devel@lists.01.org
>> Cc: leif.lindh...@linaro.org; vladimir.olovyanni...@broadcom.com; Udit
>> Kumar ; Meenakshi Aggarwal
>> ; Ard Biesheuvel
>> 
>> Subject: [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region
>> as boot services data
>>
>> Commit 8ae5fc182941 ("ArmPlatformPkg/MemoryInitPeiLib: don't reserve
>> primary FV in memory") deleted the code that removes the memory covering
>> the primary firmware volume from the memory map. The assumption was
>> that
>> this is no longer necessary now that we no longer expose compression and
>> PE/COFF loader library code from the PrePi module to DXE core.
>>
>> However, the FV is still declared, and so code may attempt to access it
>> anyway, which may cause unexpected results depending on whether the
>> memory has been reused for other purposes in the mean time.
>>
>> So reinstate the code that splits off the resource descriptor HOB that
>> describes the firmware device, but this time, don't mark the memory as
>> unusable, but create a memory allocation HOB that marks the region as
>> boot services data.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel 
>> ---
>> Vladimir, Udit, Meenakshi: please confirm whether this code works for you.
>>
>>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c | 74
>> 
>>  1 file changed, 74 insertions(+)
>>
>> diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
>> b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
>> index d03214b5df66..d1b5c0be9497 100644
>> --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
>> +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
>> @@ -70,7 +70,11 @@ 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
>> @@ -117,6 +121,76 @@ MemoryPeim (
>>  );
>>}
>>
>> +  //
>> +  // Reserve 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 create a memory allocation HOB for
>> the region,
>> +  // but this only works if we split off the underlying resource descriptor 
>> as
>> well.
>> +  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) {
>> +// Create the System Memory HOB for the firmware with the non-
>> present attribute
>
> Please correct comments, now this memory is present
>

Yes

>> +BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
>> +ResourceAttributes,
>> +PcdGet64 (PcdFdBaseAddress),
>> +PcdGet32 (PcdFdSize));
>> +
>> +// Top of the FD is system memory available for UEFI
>> +NextHob.ResourceDescriptor->PhysicalStart += 
>> PcdGet32(PcdFdSize);
>> +NextHob.ResourceDescriptor->ResourceLength -=
>> PcdGet32(PcdFdSize);
>> +  }
>> +

Re: [edk2] [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region as boot services data

2018-01-02 Thread Udit Kumar

Thanks Ard, 
This works for us as well 
Few comments inline 


Regards
Udit

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Tuesday, January 02, 2018 9:21 PM
> To: edk2-devel@lists.01.org
> Cc: leif.lindh...@linaro.org; vladimir.olovyanni...@broadcom.com; Udit
> Kumar ; Meenakshi Aggarwal
> ; Ard Biesheuvel
> 
> Subject: [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region
> as boot services data
> 
> Commit 8ae5fc182941 ("ArmPlatformPkg/MemoryInitPeiLib: don't reserve
> primary FV in memory") deleted the code that removes the memory covering
> the primary firmware volume from the memory map. The assumption was
> that
> this is no longer necessary now that we no longer expose compression and
> PE/COFF loader library code from the PrePi module to DXE core.
> 
> However, the FV is still declared, and so code may attempt to access it
> anyway, which may cause unexpected results depending on whether the
> memory has been reused for other purposes in the mean time.
> 
> So reinstate the code that splits off the resource descriptor HOB that
> describes the firmware device, but this time, don't mark the memory as
> unusable, but create a memory allocation HOB that marks the region as
> boot services data.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
> Vladimir, Udit, Meenakshi: please confirm whether this code works for you.
> 
>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c | 74
> 
>  1 file changed, 74 insertions(+)
> 
> diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> index d03214b5df66..d1b5c0be9497 100644
> --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> @@ -70,7 +70,11 @@ 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
> @@ -117,6 +121,76 @@ MemoryPeim (
>  );
>}
> 
> +  //
> +  // Reserve 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 create a memory allocation HOB for
> the region,
> +  // but this only works if we split off the underlying resource descriptor 
> as
> well.
> +  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) {
> +// Create the System Memory HOB for the firmware with the non-
> present attribute

Please correct comments, now this memory is present 

> +BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> +ResourceAttributes,
> +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
> +  BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> +

Re: [edk2] [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region as boot services data

2018-01-02 Thread Vladimir Olovyannikov

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Tuesday, January 2, 2018 7:51 AM
> To: edk2-devel@lists.01.org
> Cc: leif.lindh...@linaro.org; vladimir.olovyanni...@broadcom.com;
> udit.ku...@nxp.com; meenakshi.aggar...@nxp.com; Ard Biesheuvel
> Subject: [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV
> region as boot services data
>
> Commit 8ae5fc182941 ("ArmPlatformPkg/MemoryInitPeiLib: don't reserve
> primary FV in memory") deleted the code that removes the memory
> covering the primary firmware volume from the memory map. The
> assumption was that this is no longer necessary now that we no longer
> expose compression and PE/COFF loader library code from the PrePi module
> to DXE core.
>
> However, the FV is still declared, and so code may attempt to access it
> anyway, which may cause unexpected results depending on whether the
> memory has been reused for other purposes in the mean time.
>
> So reinstate the code that splits off the resource descriptor HOB that
> describes the firmware device, but this time, don't mark the memory as
> unusable, but create a memory allocation HOB that marks the region as
boot
> services data.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
> Vladimir, Udit, Meenakshi: please confirm whether this code works for
you.
Thanks Ard,
Works for me.

Vladimir
>
>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c | 74
> 
>  1 file changed, 74 insertions(+)
>
> diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> index d03214b5df66..d1b5c0be9497 100644
> --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> @@ -70,7 +70,11 @@ 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 @@ -117,6 +121,76
> @@ MemoryPeim (
>  );
>}
>
> +  //
> +  // Reserve 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 create a
> + memory allocation HOB for the region,  // but this only works if we
split off
> the underlying resource descriptor as well.
> +  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) {
> +// Create the System Memory HOB for the firmware with the
non-
> present attribute
> +BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> +ResourceAttributes,
> +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
> +  BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> +  ResourceAttributes,
> +  PcdGet64 (PcdFdBaseAddress),
> +  PcdGet32 (PcdFdSize));
> +
> +  // Update the

[edk2] [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region as boot services data

2018-01-02 Thread Ard Biesheuvel

Commit 8ae5fc182941 ("ArmPlatformPkg/MemoryInitPeiLib: don't reserve
primary FV in memory") deleted the code that removes the memory covering
the primary firmware volume from the memory map. The assumption was that
this is no longer necessary now that we no longer expose compression and
PE/COFF loader library code from the PrePi module to DXE core.

However, the FV is still declared, and so code may attempt to access it
anyway, which may cause unexpected results depending on whether the
memory has been reused for other purposes in the mean time.

So reinstate the code that splits off the resource descriptor HOB that
describes the firmware device, but this time, don't mark the memory as
unusable, but create a memory allocation HOB that marks the region as
boot services data.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel 
---
Vladimir, Udit, Meenakshi: please confirm whether this code works for you.

 ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c | 74 
 1 file changed, 74 insertions(+)

diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c 
b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
index d03214b5df66..d1b5c0be9497 100644
--- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
+++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
@@ -70,7 +70,11 @@ 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
@@ -117,6 +121,76 @@ MemoryPeim (
 );
   }
 
+  //
+  // Reserve 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 create a memory allocation HOB for 
the region,
+  // but this only works if we split off the underlying resource descriptor as 
well.
+  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) {
+// Create the System Memory HOB for the firmware with the 
non-present attribute
+BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
+ResourceAttributes,
+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
+  BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
+  ResourceAttributes,
+  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,
+

Re: [edk2] [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region as boot services data

Re: [edk2] [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region as boot services data

Re: [edk2] [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region as boot services data

Re: [edk2] [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region as boot services data

Re: [edk2] [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region as boot services data

Re: [edk2] [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region as boot services data

[edk2] [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region as boot services data

7 matches

Site Navigation

Mail list logo

Footer information