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

[Michael Zimmermann <sigmaepsilon92@gmail.com>]

mh after thinking about it, wouldn't it be better to swap the '<='
operands instead of changing the operator? this way the PCD memory
would be on the left side for both comparisons which seems more clear.

On Mon, Jan 16, 2017 at 7:43 PM, Michael Zimmermann
<sigmaepsilon92@gmail.com> wrote:
> 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