On 6 September 2015 at 14:22, Leif Lindholm <leif.lindh...@linaro.org> wrote:
> On 6 September 2015 at 13:05, Ard Biesheuvel <ard.biesheu...@linaro.org>
> wrote:
>> On 6 September 2015 at 14:04, Leif Lindholm <leif.lindh...@linaro.org> wrote:
>>> On 6 September 2015 at 12:52, Ard Biesheuvel <ard.biesheu...@linaro.org>
>>> wrote:
>>>> On 6 September 2015 at 10:15, 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.
>>>>>
>>>>> 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>
>>>>> ---
>>>>> 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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
