Re: [PATCH] ArmPkg/ArmMmuLib: Revert "use a pool allocation for the root table"

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 20 January 2017 at 17:49, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Jan 20, 2017 at 05:05:46PM +0000, Ard Biesheuvel wrote:
>> This reverts commit d32702d2c2aa23e828363a7f88829b78ce36c3af.
>>
>> Using a pool allocation for the root translation table seemed like
>> a good idea at the time, but as it turns out, such allocations are
>> handled in a way that makes them unsuitable for this purpose: they
>> are backed by HOBs that don't remain in the same place during the
>> various PI phase changes, which means the address programmed into
>> the TTBR register is no longer valid, and may refer to memory that
>> is reported as available to the OS.
>>
>> So switch back to using a page based allocation.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Given the discussion on the other thread:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>

Pushed, thanks.

>> ---
>>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 29 ++++----------------
>>  1 file changed, 6 insertions(+), 23 deletions(-)
>>
>> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> index c78297084207..540069a59b2e 100644
>> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> @@ -553,12 +553,10 @@ ArmConfigureMmu (
>>    )
>>  {
>>    VOID*                         TranslationTable;
>> -  VOID*                         TranslationTableBuffer;
>>    UINT32                        TranslationTableAttribute;
>>    UINT64                        MaxAddress;
>>    UINTN                         T0SZ;
>>    UINTN                         RootTableEntryCount;
>> -  UINTN                         RootTableEntrySize;
>>    UINT64                        TCR;
>>    RETURN_STATUS                 Status;
>>
>> @@ -643,19 +641,8 @@ ArmConfigureMmu (
>>    // Set TCR
>>    ArmSetTCR (TCR);
>>
>> -  // Allocate pages for translation table. Pool allocations are 8 byte aligned,
>> -  // but we may require a higher alignment based on the size of the root table.
>> -  RootTableEntrySize = RootTableEntryCount * sizeof(UINT64);
>> -  if (RootTableEntrySize < EFI_PAGE_SIZE / 2) {
>> -    TranslationTableBuffer = AllocatePool (2 * RootTableEntrySize - 8);
>> -    //
>> -    // Naturally align the root table. Preserves possible NULL value
>> -    //
>> -    TranslationTable = (VOID *)((UINTN)(TranslationTableBuffer - 1) | (RootTableEntrySize - 1)) + 1;
>> -  } else {
>> -    TranslationTable = AllocatePages (1);
>> -    TranslationTableBuffer = NULL;
>> -  }
>> +  // Allocate pages for translation table
>> +  TranslationTable = AllocatePages (1);
>>    if (TranslationTable == NULL) {
>>      return RETURN_OUT_OF_RESOURCES;
>>    }
>> @@ -669,10 +656,10 @@ ArmConfigureMmu (
>>    }
>>
>>    if (TranslationTableSize != NULL) {
>> -    *TranslationTableSize = RootTableEntrySize;
>> +    *TranslationTableSize = RootTableEntryCount * sizeof(UINT64);
>>    }
>>
>> -  ZeroMem (TranslationTable, RootTableEntrySize);
>> +  ZeroMem (TranslationTable, RootTableEntryCount * sizeof(UINT64));
>>
>>    // Disable MMU and caches. ArmDisableMmu() also invalidates the TLBs
>>    ArmDisableMmu ();
>> @@ -689,7 +676,7 @@ ArmConfigureMmu (
>>      DEBUG_CODE_BEGIN ();
>>        // Find the memory attribute for the Translation Table
>>        if ((UINTN)TranslationTable >= MemoryTable->PhysicalBase &&
>> -          (UINTN)TranslationTable + RootTableEntrySize <= MemoryTable->PhysicalBase +
>> +          (UINTN)TranslationTable + EFI_PAGE_SIZE <= MemoryTable->PhysicalBase +
>>                                                            MemoryTable->Length) {
>>          TranslationTableAttribute = MemoryTable->Attributes;
>>        }
>> @@ -718,11 +705,7 @@ ArmConfigureMmu (
>>    return RETURN_SUCCESS;
>>
>>  FREE_TRANSLATION_TABLE:
>> -  if (TranslationTableBuffer != NULL) {
>> -    FreePool (TranslationTableBuffer);
>> -  } else {
>> -    FreePages (TranslationTable, 1);
>> -  }
>> +  FreePages (TranslationTable, 1);
>>    return Status;
>>  }
>>
>> --
>> 2.7.4
>>