Currently, depending on the size of the region being (re)mapped, the
page table manipulation code may replace a table entry with a block entry,
even if the existing table entry uses different mapping attributes to
describe different parts of the region it covers. This is undesirable, and
instead, we should avoid doing so unless we are disregarding the original
attributes anyway. And if we make such a replacement, we should free all
the page tables that have become orphaned in the process.
So let's implement this, by taking the table entry path through the code
for block sized regions if a table entry already exists, and the clear
mask is set (which means we are preserving attributes from the existing
mapping). And when we do replace a table entry with a block entry, free
all the pages that are no longer referenced.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 25 ++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 6f6ef5b05fbc..488156e69057 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -223,8 +223,12 @@ UpdateRegionMappingRecursive (
// than a block, and recurse to create the block or page entries at
// the next level. No block mappings are allowed at all at level 0,
// so in that case, we have to recurse unconditionally.
+ // If we are changing a table entry and the AttributeClearMask is non-zero,
+ // we cannot replace it with a block entry without potentially losing
+ // attribute information, so keep the table entry in that case.
//
- if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0) {
+ if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0 ||
+ (IsTableEntry (*Entry, Level) && AttributeClearMask != 0)) {
ASSERT (Level < 3);
if (!IsTableEntry (*Entry, Level)) {
@@ -245,6 +249,8 @@ UpdateRegionMappingRecursive (
InvalidateDataCacheRange (TranslationTable, EFI_PAGE_SIZE);
}
+ ZeroMem (TranslationTable, EFI_PAGE_SIZE);
+
if (IsBlockEntry (*Entry, Level)) {
//
// We are splitting an existing block entry, so we have to populate
@@ -262,8 +268,6 @@ UpdateRegionMappingRecursive (
FreePages (TranslationTable, 1);
return Status;
}
- } else {
- ZeroMem (TranslationTable, EFI_PAGE_SIZE);
}
} else {
TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
@@ -300,7 +304,20 @@ UpdateRegionMappingRecursive (
EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3
: TT_TYPE_BLOCK_ENTRY;
- ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
+ if (IsTableEntry (*Entry, Level)) {
+ //
+ // We are replacing a table entry with a block entry. This is only
+ // possible if we are keeping none of the original attributes.
+ // We can free the table entry's page table, and all the ones below
+ // it, since we are dropping the only possible reference to it.
+ //
+ ASSERT (AttributeClearMask == 0);
+ TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
+ ReplaceTableEntry (Entry, EntryValue, RegionStart, TRUE);
+ FreePageTablesRecursive (TranslationTable);
+ } else {
+ ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
+ }
}
}
return EFI_SUCCESS;
--
2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56288): https://edk2.groups.io/g/devel/message/56288
Mute This Topic: https://groups.io/mt/72538523/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Ard,
I tried to verify the patch just now and now I am seeing data abort with a translation fault at 0th level. Also, if this helps, it is caused by in FreePageTablesRecursive function when it tries to access the address pointed to by TranslationTable.
Thanks
Ashish
________________________________
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Sent: Wednesday, March 25, 2020 5:38 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Ashish Singhal <ashishsingha@nvidia.com>
Subject: [PATCH v2 2/2] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry
External email: Use caution opening links or attachments
Currently, depending on the size of the region being (re)mapped, the
page table manipulation code may replace a table entry with a block entry,
even if the existing table entry uses different mapping attributes to
describe different parts of the region it covers. This is undesirable, and
instead, we should avoid doing so unless we are disregarding the original
attributes anyway. And if we make such a replacement, we should free all
the page tables that have become orphaned in the process.
So let's implement this, by taking the table entry path through the code
for block sized regions if a table entry already exists, and the clear
mask is set (which means we are preserving attributes from the existing
mapping). And when we do replace a table entry with a block entry, free
all the pages that are no longer referenced.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 25 ++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 6f6ef5b05fbc..488156e69057 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -223,8 +223,12 @@ UpdateRegionMappingRecursive (
// than a block, and recurse to create the block or page entries at
// the next level. No block mappings are allowed at all at level 0,
// so in that case, we have to recurse unconditionally.
+ // If we are changing a table entry and the AttributeClearMask is non-zero,
+ // we cannot replace it with a block entry without potentially losing
+ // attribute information, so keep the table entry in that case.
//
- if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0) {
+ if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0 ||
+ (IsTableEntry (*Entry, Level) && AttributeClearMask != 0)) {
ASSERT (Level < 3);
if (!IsTableEntry (*Entry, Level)) {
@@ -245,6 +249,8 @@ UpdateRegionMappingRecursive (
InvalidateDataCacheRange (TranslationTable, EFI_PAGE_SIZE);
}
+ ZeroMem (TranslationTable, EFI_PAGE_SIZE);
+
if (IsBlockEntry (*Entry, Level)) {
//
// We are splitting an existing block entry, so we have to populate
@@ -262,8 +268,6 @@ UpdateRegionMappingRecursive (
FreePages (TranslationTable, 1);
return Status;
}
- } else {
- ZeroMem (TranslationTable, EFI_PAGE_SIZE);
}
} else {
TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
@@ -300,7 +304,20 @@ UpdateRegionMappingRecursive (
EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3
: TT_TYPE_BLOCK_ENTRY;
- ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
+ if (IsTableEntry (*Entry, Level)) {
+ //
+ // We are replacing a table entry with a block entry. This is only
+ // possible if we are keeping none of the original attributes.
+ // We can free the table entry's page table, and all the ones below
+ // it, since we are dropping the only possible reference to it.
+ //
+ ASSERT (AttributeClearMask == 0);
+ TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
+ ReplaceTableEntry (Entry, EntryValue, RegionStart, TRUE);
+ FreePageTablesRecursive (TranslationTable);
+ } else {
+ ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
+ }
}
}
return EFI_SUCCESS;
--
2.17.1
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56295): https://edk2.groups.io/g/devel/message/56295
Mute This Topic: https://groups.io/mt/72538523/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Wed, Mar 25, 2020 at 12:38:46 +0100, Ard Biesheuvel wrote:
> Currently, depending on the size of the region being (re)mapped, the
> page table manipulation code may replace a table entry with a block entry,
> even if the existing table entry uses different mapping attributes to
> describe different parts of the region it covers. This is undesirable, and
> instead, we should avoid doing so unless we are disregarding the original
> attributes anyway. And if we make such a replacement, we should free all
> the page tables that have become orphaned in the process.
>
> So let's implement this, by taking the table entry path through the code
> for block sized regions if a table entry already exists, and the clear
> mask is set (which means we are preserving attributes from the existing
> mapping). And when we do replace a table entry with a block entry, free
> all the pages that are no longer referenced.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
With Laszlo's Tested-by:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> ---
> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 25 ++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 6f6ef5b05fbc..488156e69057 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -223,8 +223,12 @@ UpdateRegionMappingRecursive (
> // than a block, and recurse to create the block or page entries at
> // the next level. No block mappings are allowed at all at level 0,
> // so in that case, we have to recurse unconditionally.
> + // If we are changing a table entry and the AttributeClearMask is non-zero,
> + // we cannot replace it with a block entry without potentially losing
> + // attribute information, so keep the table entry in that case.
> //
> - if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0) {
> + if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0 ||
> + (IsTableEntry (*Entry, Level) && AttributeClearMask != 0)) {
> ASSERT (Level < 3);
>
> if (!IsTableEntry (*Entry, Level)) {
> @@ -245,6 +249,8 @@ UpdateRegionMappingRecursive (
> InvalidateDataCacheRange (TranslationTable, EFI_PAGE_SIZE);
> }
>
> + ZeroMem (TranslationTable, EFI_PAGE_SIZE);
> +
> if (IsBlockEntry (*Entry, Level)) {
> //
> // We are splitting an existing block entry, so we have to populate
> @@ -262,8 +268,6 @@ UpdateRegionMappingRecursive (
> FreePages (TranslationTable, 1);
> return Status;
> }
> - } else {
> - ZeroMem (TranslationTable, EFI_PAGE_SIZE);
> }
> } else {
> TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
> @@ -300,7 +304,20 @@ UpdateRegionMappingRecursive (
> EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3
> : TT_TYPE_BLOCK_ENTRY;
>
> - ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
> + if (IsTableEntry (*Entry, Level)) {
> + //
> + // We are replacing a table entry with a block entry. This is only
> + // possible if we are keeping none of the original attributes.
> + // We can free the table entry's page table, and all the ones below
> + // it, since we are dropping the only possible reference to it.
> + //
> + ASSERT (AttributeClearMask == 0);
> + TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
> + ReplaceTableEntry (Entry, EntryValue, RegionStart, TRUE);
> + FreePageTablesRecursive (TranslationTable);
> + } else {
> + ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
> + }
> }
> }
> return EFI_SUCCESS;
> --
> 2.17.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56289): https://edk2.groups.io/g/devel/message/56289
Mute This Topic: https://groups.io/mt/72538523/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Wed, 25 Mar 2020 at 13:38, Leif Lindholm <leif@nuviainc.com> wrote:
>
> On Wed, Mar 25, 2020 at 12:38:46 +0100, Ard Biesheuvel wrote:
> > Currently, depending on the size of the region being (re)mapped, the
> > page table manipulation code may replace a table entry with a block entry,
> > even if the existing table entry uses different mapping attributes to
> > describe different parts of the region it covers. This is undesirable, and
> > instead, we should avoid doing so unless we are disregarding the original
> > attributes anyway. And if we make such a replacement, we should free all
> > the page tables that have become orphaned in the process.
> >
> > So let's implement this, by taking the table entry path through the code
> > for block sized regions if a table entry already exists, and the clear
> > mask is set (which means we are preserving attributes from the existing
> > mapping). And when we do replace a table entry with a block entry, free
> > all the pages that are no longer referenced.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> With Laszlo's Tested-by:
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
>
Thanks, but it is still not 100% correct. I'll send a v3 shortly.
> > ---
> > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 25 ++++++++++++++++----
> > 1 file changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > index 6f6ef5b05fbc..488156e69057 100644
> > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > @@ -223,8 +223,12 @@ UpdateRegionMappingRecursive (
> > // than a block, and recurse to create the block or page entries at
> > // the next level. No block mappings are allowed at all at level 0,
> > // so in that case, we have to recurse unconditionally.
> > + // If we are changing a table entry and the AttributeClearMask is non-zero,
> > + // we cannot replace it with a block entry without potentially losing
> > + // attribute information, so keep the table entry in that case.
> > //
> > - if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0) {
> > + if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0 ||
> > + (IsTableEntry (*Entry, Level) && AttributeClearMask != 0)) {
> > ASSERT (Level < 3);
> >
> > if (!IsTableEntry (*Entry, Level)) {
> > @@ -245,6 +249,8 @@ UpdateRegionMappingRecursive (
> > InvalidateDataCacheRange (TranslationTable, EFI_PAGE_SIZE);
> > }
> >
> > + ZeroMem (TranslationTable, EFI_PAGE_SIZE);
> > +
> > if (IsBlockEntry (*Entry, Level)) {
> > //
> > // We are splitting an existing block entry, so we have to populate
> > @@ -262,8 +268,6 @@ UpdateRegionMappingRecursive (
> > FreePages (TranslationTable, 1);
> > return Status;
> > }
> > - } else {
> > - ZeroMem (TranslationTable, EFI_PAGE_SIZE);
> > }
> > } else {
> > TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
> > @@ -300,7 +304,20 @@ UpdateRegionMappingRecursive (
> > EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3
> > : TT_TYPE_BLOCK_ENTRY;
> >
> > - ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
> > + if (IsTableEntry (*Entry, Level)) {
> > + //
> > + // We are replacing a table entry with a block entry. This is only
> > + // possible if we are keeping none of the original attributes.
> > + // We can free the table entry's page table, and all the ones below
> > + // it, since we are dropping the only possible reference to it.
> > + //
> > + ASSERT (AttributeClearMask == 0);
> > + TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
> > + ReplaceTableEntry (Entry, EntryValue, RegionStart, TRUE);
> > + FreePageTablesRecursive (TranslationTable);
> > + } else {
> > + ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
> > + }
> > }
> > }
> > return EFI_SUCCESS;
> > --
> > 2.17.1
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56294): https://edk2.groups.io/g/devel/message/56294
Mute This Topic: https://groups.io/mt/72538523/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.