* [PATCH] ArmPkg/ArmMmuLib: Revert "use a pool allocation for the root table"
@ 2017-01-20 17:05 Ard Biesheuvel
2017-01-20 17:43 ` Laszlo Ersek
2017-01-20 17:49 ` Leif Lindholm
0 siblings, 2 replies; 4+ messages in thread
From: Ard Biesheuvel @ 2017-01-20 17:05 UTC (permalink / raw)
To: edk2-devel; +Cc: leif.lindholm, lersek, heyi.guo, ashedel, Ard Biesheuvel
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>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] ArmPkg/ArmMmuLib: Revert "use a pool allocation for the root table"
2017-01-20 17:05 [PATCH] ArmPkg/ArmMmuLib: Revert "use a pool allocation for the root table" Ard Biesheuvel
@ 2017-01-20 17:43 ` Laszlo Ersek
2017-01-20 17:49 ` Leif Lindholm
1 sibling, 0 replies; 4+ messages in thread
From: Laszlo Ersek @ 2017-01-20 17:43 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel; +Cc: leif.lindholm, heyi.guo, ashedel
On 01/20/17 18:05, 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>
> ---
> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 29 ++++----------------
> 1 file changed, 6 insertions(+), 23 deletions(-)
Acked-by: Laszlo Ersek <lersek@redhat.com>
> 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;
> }
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ArmPkg/ArmMmuLib: Revert "use a pool allocation for the root table"
2017-01-20 17:05 [PATCH] ArmPkg/ArmMmuLib: Revert "use a pool allocation for the root table" Ard Biesheuvel
2017-01-20 17:43 ` Laszlo Ersek
@ 2017-01-20 17:49 ` Leif Lindholm
2017-01-20 17:52 ` Ard Biesheuvel
1 sibling, 1 reply; 4+ messages in thread
From: Leif Lindholm @ 2017-01-20 17:49 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, lersek, heyi.guo, ashedel
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>
> ---
> 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ArmPkg/ArmMmuLib: Revert "use a pool allocation for the root table"
2017-01-20 17:49 ` Leif Lindholm
@ 2017-01-20 17:52 ` Ard Biesheuvel
0 siblings, 0 replies; 4+ messages in thread
From: Ard Biesheuvel @ 2017-01-20 17:52 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel@lists.01.org, Laszlo Ersek, Heyi Guo, Andrey Shedel
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
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-01-20 17:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-20 17:05 [PATCH] ArmPkg/ArmMmuLib: Revert "use a pool allocation for the root table" Ard Biesheuvel
2017-01-20 17:43 ` Laszlo Ersek
2017-01-20 17:49 ` Leif Lindholm
2017-01-20 17:52 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox