On 6 September 2015 at 14:04, Leif Lindholm <[email protected]> wrote: > On 6 September 2015 at 12:52, Ard Biesheuvel <[email protected]> > wrote: >> On 6 September 2015 at 10:15, Heyi Guo <[email protected]> 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. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Heyi Guo <[email protected]> >>> Cc: Leif Lindholm <[email protected]> >>> Cc: Ard Biesheuvel <[email protected]> >>> --- >>> ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c >>> b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c >>> index e7b095c..b039ae1 100644 >>> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c >>> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c >>> @@ -295,14 +295,16 @@ GetBlockEntryListFromAddress ( >>> if (RegionStart == 0) { >>> // Identify the highest possible alignment for the Region Size >>> for (BaseAddressAlignment = 0; BaseAddressAlignment < 64; >>> BaseAddressAlignment++) { >>> - if ((1 << BaseAddressAlignment) & *BlockEntrySize) { >>> + // Type casting is needed as literal number will be treated as INT32 >>> by >>> + // compiler >>> + if (((UINT64) 1 << BaseAddressAlignment) & *BlockEntrySize) { >> >> Please use '1UL' instead of '(UINT64) 1' > > Wouldn't 1ULL be a better choice? > To still work if someone was to copy code across to a 32-bit arch. >
Perhaps yes, although I think it's the 'U' that makes the difference here, not the number of Ls :-) _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
