| Message ID | 1433433340-30006-1-git-send-email-heyi.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Ard, I got this error when running SCT against QEMU AARCH64 UEFI. I think it will only happen for aarch64 runtime services allocation, because only this type of memory requires more than 4 pages alignment, which will trigger the bug. If we use the same alignment (e.g. 64KB) for both boot and runtime memory, it just avoid the bug indirectly, as each descriptor would be kept naturally aligned with 64KB. Thanks. On 06/05/2015 12:08 AM, Ard Biesheuvel wrote: > On 4 June 2015 at 17:55, Heyi Guo <heyi.guo@linaro.org> wrote: >> DescEnd will be clipped for alignment in CoreFindFreePagesI, and it >> may fall below DescStart, when alignment is more than 16KB (included) >> and both DescStart and original DescEnd fall into a single range of >> such alignment. This results in a huge size (Negative number in >> unsigned type) for this descriptor, fulfilling the allocation >> requirement but failing to run ConvertPages; at last it causes > at least > >> occasional failure of AllocatePages. >> >> A simple comparison is added to ensure we would never get a negative >> number. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Heyi Guo <heyi.guo@linaro.org> > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > So is this only happening because we use different granularities for > boottime and runtime allocations on AArch64? > > >> --- >> MdeModulePkg/Core/Dxe/Mem/Page.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c >> index a92c865..f2efaf1 100644 >> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c >> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c >> @@ -1051,6 +1051,11 @@ CoreFindFreePagesI ( >> >> DescEnd = ((DescEnd + 1) & (~(Alignment - 1))) - 1; >> >> + // Skip if DescEnd is less than DescStart after alignment clipping >> + if (DescEnd < DescStart) { >> + continue; >> + } >> + >> // >> // Compute the number of bytes we can used from this >> // descriptor, and see it's enough to satisfy the request >> -- >> 2.1.4 >> ------------------------------------------------------------------------------
Hi Tian, Do you have any comments on this patch? Thanks! On 06/05/2015 09:57 AM, Heyi Guo wrote: > Hi Ard, > > I got this error when running SCT against QEMU AARCH64 UEFI. I think > it will only happen for aarch64 runtime services allocation, because > only this type of memory requires more than 4 pages alignment, which > will trigger the bug. If we use the same alignment (e.g. 64KB) for > both boot and runtime memory, it just avoid the bug indirectly, as > each descriptor would be kept naturally aligned with 64KB. > > Thanks. > > On 06/05/2015 12:08 AM, Ard Biesheuvel wrote: >> On 4 June 2015 at 17:55, Heyi Guo <heyi.guo@linaro.org> wrote: >>> DescEnd will be clipped for alignment in CoreFindFreePagesI, and it >>> may fall below DescStart, when alignment is more than 16KB (included) >>> and both DescStart and original DescEnd fall into a single range of >>> such alignment. This results in a huge size (Negative number in >>> unsigned type) for this descriptor, fulfilling the allocation >>> requirement but failing to run ConvertPages; at last it causes >> at least >> >>> occasional failure of AllocatePages. >>> >>> A simple comparison is added to ensure we would never get a negative >>> number. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org> >> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >> So is this only happening because we use different granularities for >> boottime and runtime allocations on AArch64? >> >> >>> --- >>> MdeModulePkg/Core/Dxe/Mem/Page.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c >>> b/MdeModulePkg/Core/Dxe/Mem/Page.c >>> index a92c865..f2efaf1 100644 >>> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c >>> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c >>> @@ -1051,6 +1051,11 @@ CoreFindFreePagesI ( >>> >>> DescEnd = ((DescEnd + 1) & (~(Alignment - 1))) - 1; >>> >>> + // Skip if DescEnd is less than DescStart after alignment clipping >>> + if (DescEnd < DescStart) { >>> + continue; >>> + } >>> + >>> // >>> // Compute the number of bytes we can used from this >>> // descriptor, and see it's enough to satisfy the request >>> -- >>> 2.1.4 >>> > ------------------------------------------------------------------------------
Thanks Liming. I saw Tian is the maintainer of MdeModulePkg, so I had sent the mail to him. It will be great if you could help to commit this change :) On 06/08/2015 11:18 AM, Gao, Liming wrote: > Heyi: > Thanks for your catch issue. It only happens when the default alignment is different. Now, only Arm Arch64 uses the different alignments for the different memory types. So, you meet with this issue on QEMU AARCH64 UEFI. > I agree your fix. Reviewed-by: Liming Gao <liming.gao@intel.com> > > Btw: if you need my help to commit this change, I would like to do it. > > Thanks > Liming > -----Original Message----- > From: Heyi Guo [mailto:heyi.guo@linaro.org] > Sent: Monday, June 08, 2015 9:26 AM > To: Tian, Feng > Cc: edk2-devel@lists.sourceforge.net; Ilias Biris > Subject: Re: [edk2] [PATCH] MdeModulePkg: Fix bug of AllocatePages for large alignment > > Hi Tian, > > Do you have any comments on this patch? > > Thanks! > > On 06/05/2015 09:57 AM, Heyi Guo wrote: >> Hi Ard, >> >> I got this error when running SCT against QEMU AARCH64 UEFI. I think >> it will only happen for aarch64 runtime services allocation, because >> only this type of memory requires more than 4 pages alignment, which >> will trigger the bug. If we use the same alignment (e.g. 64KB) for >> both boot and runtime memory, it just avoid the bug indirectly, as >> each descriptor would be kept naturally aligned with 64KB. >> >> Thanks. >> >> On 06/05/2015 12:08 AM, Ard Biesheuvel wrote: >>> On 4 June 2015 at 17:55, Heyi Guo <heyi.guo@linaro.org> wrote: >>>> DescEnd will be clipped for alignment in CoreFindFreePagesI, and it >>>> may fall below DescStart, when alignment is more than 16KB >>>> (included) and both DescStart and original DescEnd fall into a >>>> single range of such alignment. This results in a huge size >>>> (Negative number in unsigned type) for this descriptor, fulfilling >>>> the allocation requirement but failing to run ConvertPages; at last >>>> it causes >>> at least >>> >>>> occasional failure of AllocatePages. >>>> >>>> A simple comparison is added to ensure we would never get a negative >>>> number. >>>> >>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org> >>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> >>> So is this only happening because we use different granularities for >>> boottime and runtime allocations on AArch64? >>> >>> >>>> --- >>>> MdeModulePkg/Core/Dxe/Mem/Page.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c >>>> b/MdeModulePkg/Core/Dxe/Mem/Page.c >>>> index a92c865..f2efaf1 100644 >>>> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c >>>> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c >>>> @@ -1051,6 +1051,11 @@ CoreFindFreePagesI ( >>>> >>>> DescEnd = ((DescEnd + 1) & (~(Alignment - 1))) - 1; >>>> >>>> + // Skip if DescEnd is less than DescStart after alignment clipping >>>> + if (DescEnd < DescStart) { >>>> + continue; >>>> + } >>>> + >>>> // >>>> // Compute the number of bytes we can used from this >>>> // descriptor, and see it's enough to satisfy the request >>>> -- >>>> 2.1.4 >>>> > > ------------------------------------------------------------------------------ > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > ------------------------------------------------------------------------------ > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------
OK, thanks! On 06/08/2015 12:38 PM, Tian, Feng wrote: > Liming is the DxeCore module owner. > > I own the whole MdeModulePkg, and may dispatch community patch/review request to corresponding owner if it's not straightforward for me. > > The review process from your side is ok. Don't worry about that:-) > > Thanks > Feng > > -----Original Message----- > From: Heyi Guo [mailto:heyi.guo@linaro.org] > Sent: Monday, June 08, 2015 12:26 > To: edk2-devel@lists.sourceforge.net; Tian, Feng > Cc: Ilias Biris > Subject: Re: [edk2] [PATCH] MdeModulePkg: Fix bug of AllocatePages for large alignment > > Thanks Liming. > > I saw Tian is the maintainer of MdeModulePkg, so I had sent the mail to him. > > It will be great if you could help to commit this change :) > > > > On 06/08/2015 11:18 AM, Gao, Liming wrote: >> Heyi: >> Thanks for your catch issue. It only happens when the default alignment is different. Now, only Arm Arch64 uses the different alignments for the different memory types. So, you meet with this issue on QEMU AARCH64 UEFI. >> I agree your fix. Reviewed-by: Liming Gao <liming.gao@intel.com> >> >> Btw: if you need my help to commit this change, I would like to do it. >> >> Thanks >> Liming >> -----Original Message----- >> From: Heyi Guo [mailto:heyi.guo@linaro.org] >> Sent: Monday, June 08, 2015 9:26 AM >> To: Tian, Feng >> Cc: edk2-devel@lists.sourceforge.net; Ilias Biris >> Subject: Re: [edk2] [PATCH] MdeModulePkg: Fix bug of AllocatePages for >> large alignment >> >> Hi Tian, >> >> Do you have any comments on this patch? >> >> Thanks! >> >> On 06/05/2015 09:57 AM, Heyi Guo wrote: >>> Hi Ard, >>> >>> I got this error when running SCT against QEMU AARCH64 UEFI. I think >>> it will only happen for aarch64 runtime services allocation, because >>> only this type of memory requires more than 4 pages alignment, which >>> will trigger the bug. If we use the same alignment (e.g. 64KB) for >>> both boot and runtime memory, it just avoid the bug indirectly, as >>> each descriptor would be kept naturally aligned with 64KB. >>> >>> Thanks. >>> >>> On 06/05/2015 12:08 AM, Ard Biesheuvel wrote: >>>> On 4 June 2015 at 17:55, Heyi Guo <heyi.guo@linaro.org> wrote: >>>>> DescEnd will be clipped for alignment in CoreFindFreePagesI, and it >>>>> may fall below DescStart, when alignment is more than 16KB >>>>> (included) and both DescStart and original DescEnd fall into a >>>>> single range of such alignment. This results in a huge size >>>>> (Negative number in unsigned type) for this descriptor, fulfilling >>>>> the allocation requirement but failing to run ConvertPages; at last >>>>> it causes >>>> at least >>>> >>>>> occasional failure of AllocatePages. >>>>> >>>>> A simple comparison is added to ensure we would never get a >>>>> negative number. >>>>> >>>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org> >>>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> >>>> So is this only happening because we use different granularities for >>>> boottime and runtime allocations on AArch64? >>>> >>>> >>>>> --- >>>>> MdeModulePkg/Core/Dxe/Mem/Page.c | 5 +++++ >>>>> 1 file changed, 5 insertions(+) >>>>> >>>>> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c >>>>> b/MdeModulePkg/Core/Dxe/Mem/Page.c >>>>> index a92c865..f2efaf1 100644 >>>>> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c >>>>> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c >>>>> @@ -1051,6 +1051,11 @@ CoreFindFreePagesI ( >>>>> >>>>> DescEnd = ((DescEnd + 1) & (~(Alignment - 1))) - 1; >>>>> >>>>> + // Skip if DescEnd is less than DescStart after alignment clipping >>>>> + if (DescEnd < DescStart) { >>>>> + continue; >>>>> + } >>>>> + >>>>> // >>>>> // Compute the number of bytes we can used from this >>>>> // descriptor, and see it's enough to satisfy the request >>>>> -- >>>>> 2.1.4 >>>>> >> ---------------------------------------------------------------------- >> -------- _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/edk2-devel >> >> ---------------------------------------------------------------------- >> -------- _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c index a92c865..f2efaf1 100644 --- a/MdeModulePkg/Core/Dxe/Mem/Page.c +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c @@ -1051,6 +1051,11 @@ CoreFindFreePagesI ( DescEnd = ((DescEnd + 1) & (~(Alignment - 1))) - 1; + // Skip if DescEnd is less than DescStart after alignment clipping + if (DescEnd < DescStart) { + continue; + } + // // Compute the number of bytes we can used from this // descriptor, and see it's enough to satisfy the request
DescEnd will be clipped for alignment in CoreFindFreePagesI, and it may fall below DescStart, when alignment is more than 16KB (included) and both DescStart and original DescEnd fall into a single range of such alignment. This results in a huge size (Negative number in unsigned type) for this descriptor, fulfilling the allocation requirement but failing to run ConvertPages; at last it causes occasional failure of AllocatePages. A simple comparison is added to ensure we would never get a negative number. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Heyi Guo <heyi.guo@linaro.org> --- MdeModulePkg/Core/Dxe/Mem/Page.c | 5 +++++ 1 file changed, 5 insertions(+)