On 09/07/15 09:34, Ard Biesheuvel wrote: > On 6 September 2015 at 14:22, Leif Lindholm <[email protected]> wrote: >> On 6 September 2015 at 13:05, Ard Biesheuvel <[email protected]> >> wrote: >>> 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 :-) >> >> For this location, certainly. >> But if the same code was compiled on a 32-bit architecture, it would. >> UINT64 is the portable way to specify this, but at least ULL means the >> same on 32- and 64-bit. UL does not. >> > > Ehm, looking at the surrounding code, we should probably just be using > LowBitSet64() instead of open coding it.
Also, quite unrelatedly, if you'd like to have a UINT64 left shift that compiles for 32-bit targets with *all* supported compilers, then you should call LShiftU64(). Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
