On 9 September 2015 at 11:53, Heyi Guo <heyi....@linaro.org> wrote:
> There is a hidden bug for below code:
>
> (1 << BaseAddressAlignment) & *BlockEntrySize
>
> From disassembly code, we can the literal number 1 will be treated as
> INT32 by compiler by default, and we'll get 0xFFFFFFFF80000000 when
> BaseAddressAlignment is equal to 31. So we will always get 31 when
> alignment is larger than 31.
>
> if ((1 << BaseAddressAlignment) & *BlockEntrySize) {
> 5224: f9404be0 ldr x0, [sp,#144]
> 5228: 2a0003e1 mov w1, w0
> 522c: 52800020 mov w0, #0x1 // #1
> 5230: 1ac12000 lsl w0, w0, w1
> 5234: 93407c01 sxtw x1, w0
>
> The bug can be replayed on QEMU AARCH64; by adding some debug print,
> we can see lots of level 1 tables created (for block of 1GB) even
> when the region is large enough to use 512GB block size.
>
> Use LowBitSet64() in BaseLib instead to fix the bug.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Heyi Guo <heyi....@linaro.org>
> Cc: Leif Lindholm <leif.lindh...@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
Reviewed-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> ---
> ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 16 ++++------------
> 1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> index d5d20a2..20f1a7b 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> @@ -276,8 +276,8 @@ GetBlockEntryListFromAddress (
> return NULL;
> }
>
> - // Ensure the required size is aligned on 4KB boundary
> - if ((*BlockEntrySize & (SIZE_4KB - 1)) != 0) {
> + // Ensure the required size is aligned on 4KB boundary and not 0
> + if ((*BlockEntrySize & (SIZE_4KB - 1)) != 0 || *BlockEntrySize == 0) {
> ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER);
> return NULL;
> }
> @@ -294,18 +294,10 @@ GetBlockEntryListFromAddress (
> // If the start address is 0x0 then we use the size of the region to
> identify the alignment
> if (RegionStart == 0) {
> // Identify the highest possible alignment for the Region Size
> - for (BaseAddressAlignment = 0; BaseAddressAlignment < 64;
> BaseAddressAlignment++) {
> - if ((1 << BaseAddressAlignment) & *BlockEntrySize) {
> - break;
> - }
> - }
> + BaseAddressAlignment = LowBitSet64 (*BlockEntrySize);
> } else {
> // Identify the highest possible alignment for the Base Address
> - for (BaseAddressAlignment = 0; BaseAddressAlignment < 64;
> BaseAddressAlignment++) {
> - if ((1 << BaseAddressAlignment) & RegionStart) {
> - break;
> - }
> - }
> + BaseAddressAlignment = LowBitSet64 (RegionStart);
> }
>
> // Identify the Page Level the RegionStart must belongs to
> --
> 2.5.0
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
