* [PATCH] ArmPlatformPkg: MemoryInitPei: fix wrong check for system memory @ 2017-01-12 15:46 Michael Zimmermann 2017-01-13 4:47 ` Michael Zimmermann 2017-01-16 18:09 ` Leif Lindholm 0 siblings, 2 replies; 5+ messages in thread From: Michael Zimmermann @ 2017-01-12 15:46 UTC (permalink / raw) To: edk2-devel@lists.01.org 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))) + (NextHob.ResourceDescriptor->PhysicalStart + NextHob.ResourceDescriptor->ResourceLength >= PcdGet64 (PcdSystemMemoryBase) + PcdGet64 (PcdSystemMemorySize))) { Found = TRUE; break; -- 2.11.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ArmPlatformPkg: MemoryInitPei: fix wrong check for system memory 2017-01-12 15:46 [PATCH] ArmPlatformPkg: MemoryInitPei: fix wrong check for system memory Michael Zimmermann @ 2017-01-13 4:47 ` Michael Zimmermann 2017-01-16 18:09 ` Leif Lindholm 1 sibling, 0 replies; 5+ messages in thread From: Michael Zimmermann @ 2017-01-13 4:47 UTC (permalink / raw) To: edk2-devel@lists.01.org, Leif Lindholm, Ard Biesheuvel sry, but I forgot to CC the maintainers again so doing this with this self-reply. On Thu, Jan 12, 2017 at 4:46 PM, Michael Zimmermann < sigmaepsilon92@gmail.com> 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))) > + (NextHob.ResourceDescriptor->PhysicalStart + > NextHob.ResourceDescriptor->ResourceLength >= PcdGet64 > (PcdSystemMemoryBase) + PcdGet64 (PcdSystemMemorySize))) > { > Found = TRUE; > break; > -- > 2.11.0 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ArmPlatformPkg: MemoryInitPei: fix wrong check for system memory 2017-01-12 15:46 [PATCH] ArmPlatformPkg: MemoryInitPei: fix wrong check for system memory Michael Zimmermann 2017-01-13 4:47 ` Michael Zimmermann @ 2017-01-16 18:09 ` Leif Lindholm 2017-01-16 18:43 ` Michael Zimmermann 1 sibling, 1 reply; 5+ messages in thread From: Leif Lindholm @ 2017-01-16 18:09 UTC (permalink / raw) To: Michael Zimmermann; +Cc: edk2-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ArmPlatformPkg: MemoryInitPei: fix wrong check for system memory 2017-01-16 18:09 ` Leif Lindholm @ 2017-01-16 18:43 ` Michael Zimmermann 2017-01-16 18:44 ` Michael Zimmermann 0 siblings, 1 reply; 5+ messages in thread From: Michael Zimmermann @ 2017-01-16 18:43 UTC (permalink / raw) To: Leif Lindholm; +Cc: edk2-devel@lists.01.org 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ArmPlatformPkg: MemoryInitPei: fix wrong check for system memory 2017-01-16 18:43 ` Michael Zimmermann @ 2017-01-16 18:44 ` Michael Zimmermann 0 siblings, 0 replies; 5+ messages in thread From: Michael Zimmermann @ 2017-01-16 18:44 UTC (permalink / raw) To: Leif Lindholm; +Cc: edk2-devel@lists.01.org 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-01-16 18:44 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-01-12 15:46 [PATCH] ArmPlatformPkg: MemoryInitPei: fix wrong check for system memory Michael Zimmermann 2017-01-13 4:47 ` Michael Zimmermann 2017-01-16 18:09 ` Leif Lindholm 2017-01-16 18:43 ` Michael Zimmermann 2017-01-16 18:44 ` Michael Zimmermann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox