Re: [PATCH] ArmPlatformPkg: MemoryInitPei: fix wrong check for system memory

[Michael Zimmermann <sigmaepsilon92@gmail.com>]

The code in question is supposed to check if the memory range defined
in the PCD is fully included in the memory resource descriptor of the
current iteration. The condition for that is wrong. example:
PcdSystemMemoryBase = 0x80000000
PcdSystemMemorySize  = 0x10000000
ResourceDescriptor->PhysicalStart = 0x40000000
NextHob.ResourceDescriptor->ResourceLength = 0x10000000

obviously, the resource descriptor doesn't contain the system memory.
but the old logic would have selected this Hob because:
0x80000000>=0x40000000 && 0x50000000 <= 0x90000000
The second comparison probably was supposed to have the
ResourceDescriptor values on the right side,
I fixed it by just inverting the direction of the comparison which then becomes:
0x80000000>=0x40000000 && 0x50000000 >= 0x90000000

And this comparison now successfully fails but will still be
successful for the case we're checking for.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
---
 ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
index 2feb11f..8f67812 100644
--- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
+++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
@@ -103,7 +103,7 @@ MemoryPeim (
   while ((NextHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR,
NextHob.Raw)) != NULL) {
     if ((NextHob.ResourceDescriptor->ResourceType ==
EFI_RESOURCE_SYSTEM_MEMORY) &&
         (PcdGet64 (PcdSystemMemoryBase) >=
NextHob.ResourceDescriptor->PhysicalStart) &&
-        (NextHob.ResourceDescriptor->PhysicalStart +
NextHob.ResourceDescriptor->ResourceLength <= PcdGet64
(PcdSystemMemoryBase) + PcdGet64 (PcdSystemMemorySize)))
+        (NextHob.ResourceDescriptor->PhysicalStart +
NextHob.ResourceDescriptor->ResourceLength >= PcdGet64
(PcdSystemMemoryBase) + PcdGet64 (PcdSystemMemorySize)))
     {
       Found = TRUE;
       break;

On Mon, Jan 16, 2017 at 7:09 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> Hi Michael,
>
> Could you provide a message body explaining the error that this patch
> fixes so that I don't sprain my brain trying to figure it out?
>
> On Thu, Jan 12, 2017 at 04:46:57PM +0100, Michael Zimmermann wrote:
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
>> ---
>>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
>> b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
>> index 2feb11f21d..8f6781212e 100644
>> --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
>> +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
>> @@ -103,7 +103,7 @@ MemoryPeim (
>>    while ((NextHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR,
>> NextHob.Raw)) != NULL) {
>>      if ((NextHob.ResourceDescriptor->ResourceType ==
>> EFI_RESOURCE_SYSTEM_MEMORY) &&
>>          (PcdGet64 (PcdSystemMemoryBase) >=
>> NextHob.ResourceDescriptor->PhysicalStart) &&
>> -        (NextHob.ResourceDescriptor->PhysicalStart +
>> NextHob.ResourceDescriptor->ResourceLength <= PcdGet64
>> (PcdSystemMemoryBase) + PcdGet64 (PcdSystemMemorySize)))
>
> It looks like the patch got corrupted on submission (lines wrapped).
> Could you resend it please?
>
>> +        (NextHob.ResourceDescriptor->PhysicalStart +
>> NextHob.ResourceDescriptor->ResourceLength >= PcdGet64
>> (PcdSystemMemoryBase) + PcdGet64 (PcdSystemMemorySize)))
>>      {
>>        Found = TRUE;
>>        break;
>> --
>> 2.11.0
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel