* [PATCH 0/2] ArmPkg/ArmMmuLib AARCH64: correctness fix
@ 2020-03-07 13:34 Ard Biesheuvel
2020-03-07 13:34 ` [PATCH 1/2] ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types Ard Biesheuvel
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2020-03-07 13:34 UTC (permalink / raw)
To: devel; +Cc: leif, Ard Biesheuvel
Last one, I promise :-)
The new ArmMmuLib code is easier to reason about, so that is what I did:
currently, when we create mappings that cover existing table entries, we
may end up overwriting those with block entries without taking the mapping
attributes of the original table entries into account. So let's fix this.
I honestly don't know whether the original code was better at dealing with
this: I do remember some changes from Heyi that may have been related, but
the old code is not easy to follow. In any case, I didn't manage to hit this
case in practice, given that we typically start out with large mappings, and
break them down later (to set permissions), rather than the other way around.
Patch #1 adds some helpers to hide the insane way the type bits change
meaning when you change to level 3.
Patch #2 ensures that we only replace (and free) table entries with block
entries if it is guaranteed that doing so will not lose any attribute
information.
Ard Biesheuvel (2):
ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types
ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table
entry
.../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 61 ++++++++++++++++---
1 file changed, 54 insertions(+), 7 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types 2020-03-07 13:34 [PATCH 0/2] ArmPkg/ArmMmuLib AARCH64: correctness fix Ard Biesheuvel @ 2020-03-07 13:34 ` Ard Biesheuvel 2020-03-07 13:34 ` [PATCH 2/2] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry Ard Biesheuvel 2020-03-09 13:16 ` [PATCH 0/2] ArmPkg/ArmMmuLib AARCH64: correctness fix Leif Lindholm 2 siblings, 0 replies; 6+ messages in thread From: Ard Biesheuvel @ 2020-03-07 13:34 UTC (permalink / raw) To: devel; +Cc: leif, Ard Biesheuvel Given how the meaning of the attribute bits for page table entry types is slightly awkward, and changes between levels, add some helpers to abstract from this. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 40 +++++++++++++++++--- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c index a43d468b73ca..6f6ef5b05fbc 100644 --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c @@ -156,6 +156,36 @@ FreePageTablesRecursive ( FreePages (TranslationTable, 1); } +STATIC +BOOLEAN +IsBlockEntry ( + IN UINT64 Entry, + IN UINTN Level + ) +{ + if (Level == 3) { + return (Entry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY_LEVEL3; + } + return (Entry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY; +} + +STATIC +BOOLEAN +IsTableEntry ( + IN UINT64 Entry, + IN UINTN Level + ) +{ + if (Level == 3) { + // + // TT_TYPE_TABLE_ENTRY aliases TT_TYPE_BLOCK_ENTRY_LEVEL3 + // so we need to take the level into account as well. + // + return FALSE; + } + return (Entry & TT_TYPE_MASK) == TT_TYPE_TABLE_ENTRY; +} + STATIC EFI_STATUS UpdateRegionMappingRecursive ( @@ -197,7 +227,7 @@ UpdateRegionMappingRecursive ( if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0) { ASSERT (Level < 3); - if ((*Entry & TT_TYPE_MASK) != TT_TYPE_TABLE_ENTRY) { + if (!IsTableEntry (*Entry, Level)) { // // No table entry exists yet, so we need to allocate a page table // for the next level. @@ -215,7 +245,7 @@ UpdateRegionMappingRecursive ( InvalidateDataCacheRange (TranslationTable, EFI_PAGE_SIZE); } - if ((*Entry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) { + if (IsBlockEntry (*Entry, Level)) { // // We are splitting an existing block entry, so we have to populate // the new table with the attributes of the block entry it replaces. @@ -246,7 +276,7 @@ UpdateRegionMappingRecursive ( AttributeSetMask, AttributeClearMask, TranslationTable, Level + 1); if (EFI_ERROR (Status)) { - if ((*Entry & TT_TYPE_MASK) != TT_TYPE_TABLE_ENTRY) { + if (!IsTableEntry (*Entry, Level)) { // // We are creating a new table entry, so on failure, we can free all // allocations we made recursively, given that the whole subhierarchy @@ -259,10 +289,10 @@ UpdateRegionMappingRecursive ( return Status; } - if ((*Entry & TT_TYPE_MASK) != TT_TYPE_TABLE_ENTRY) { + if (!IsTableEntry (*Entry, Level)) { EntryValue = (UINTN)TranslationTable | TT_TYPE_TABLE_ENTRY; ReplaceTableEntry (Entry, EntryValue, RegionStart, - (*Entry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY); + IsBlockEntry (*Entry, Level)); } } else { EntryValue = (*Entry & AttributeClearMask) | AttributeSetMask; -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry 2020-03-07 13:34 [PATCH 0/2] ArmPkg/ArmMmuLib AARCH64: correctness fix Ard Biesheuvel 2020-03-07 13:34 ` [PATCH 1/2] ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types Ard Biesheuvel @ 2020-03-07 13:34 ` Ard Biesheuvel 2020-03-10 0:29 ` [edk2-devel] " Laszlo Ersek 2020-03-09 13:16 ` [PATCH 0/2] ArmPkg/ArmMmuLib AARCH64: correctness fix Leif Lindholm 2 siblings, 1 reply; 6+ messages in thread From: Ard Biesheuvel @ 2020-03-07 13:34 UTC (permalink / raw) To: devel; +Cc: leif, Ard Biesheuvel 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 | 21 ++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c index 6f6ef5b05fbc..7b2c36a7a538 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)) { @@ -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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry 2020-03-07 13:34 ` [PATCH 2/2] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry Ard Biesheuvel @ 2020-03-10 0:29 ` Laszlo Ersek 2020-03-25 9:40 ` Ard Biesheuvel 0 siblings, 1 reply; 6+ messages in thread From: Laszlo Ersek @ 2020-03-10 0:29 UTC (permalink / raw) To: ard.biesheuvel; +Cc: devel, leif Hi Ard, On 03/07/20 14:34, 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> > --- > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 21 ++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > index 6f6ef5b05fbc..7b2c36a7a538 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)) { > @@ -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; > This patch crashes an existent (RHEL-ALT-7) guest for me, when it tries to launch "grubaa64.efi". (1) I closed your PR#430 as explained here: <https://edk2.groups.io/g/devel/message/55695>, and collected *six* of your reviewed patches, for a new PR: - [edk2-devel] [PATCH v4 0/2] ArmPkg/ArmMmuLib: rewrite and improve cache handling with MMU off - [edk2-devel] [PATCH 0/2] ArmPkg/ArmMmuLib AARCH64: final cleanups - the present set (2) Before submitting the PR, I figured I'd boot one of my permanent QEMU (not KVM) guests, with all six patches applied (locally, for the time). The boot crashed; here's the end of the log: > [Bds]Booting Red Hat Enterprise Linux > FSOpen: Open '\EFI\redhat\shimaa64.efi' Success > [Bds] Expand HD(1,GPT,1F51423F-5044-4C9C-BBF7-666D366B249E,0x800,0x64000)/\EFI\redhat\shimaa64.efi -> PciRoot(0x0)/Pci(0x1,0x6)/Pci(0x0,0x0)/Scsi(0x0,0x0)/HD(1,GPT,1F51423F-5044-4C9C-BBF7-666D366B249E,0x800,0x64000)/\EFI\redhat\shimaa64.efi > BdsDxe: loading Boot0005 "Red Hat Enterprise Linux" from HD(1,GPT,1F51423F-5044-4C9C-BBF7-666D366B249E,0x800,0x64000)/\EFI\redhat\shimaa64.efi > [Security] 3rd party image[0] can be loaded after EndOfDxe: PciRoot(0x0)/Pci(0x1,0x6)/Pci(0x0,0x0)/Scsi(0x0,0x0)/HD(1,GPT,1F51423F-5044-4C9C-BBF7-666D366B249E,0x800,0x64000)/\EFI\redhat\shimaa64.efi. > InstallProtocolInterface: 5B1B31A1-9562-11D2-8E3F-00A0C969723B 13A2196C0 > UpdateRegionMappingRecursive(0): 137EE0000 - 137FA0000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(1): 137EE0000 - 137FA0000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(2): 137EE0000 - 137FA0000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(3): 137E00000 - 138000000 set 6000000000070C clr 0 > UpdateRegionMappingRecursive(3): 137EE0000 - 137FA0000 set 0 clr FF9F000000000F3F > Loading driver at 0x00137EE0000 EntryPoint=0x00137EE1000 > Loading driver at 0x00137EE0000 EntryPoint=0x00137EE1000 > InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 13A124498 > ProtectUefiImageCommon - 0x3A2196C0 > - 0x0000000137EE0000 - 0x00000000000BF500 > Section - '.text ' > VirtualSize - 0x0008C000 > VirtualAddress - 0x00001000 > SizeOfRawData - 0x0008C000 > PointerToRawData - 0x00001000 > PointerToRelocations - 0x00000000 > PointerToLinenumbers - 0x00000000 > NumberOfRelocations - 0x00000000 > NumberOfLinenumbers - 0x00000000 > Characteristics - 0x60000020 > ImageCode: 0x0000000137EE1000 - 0x000000000008C000 > Section - '.data ' > ImageCode SegmentCount - 0x1 > SetUefiImageMemoryAttributes - 0x0000000137EE0000 - 0x0000000000001000 (0x0000000000004008) > UpdateRegionMappingRecursive(0): 137EE0000 - 137EE1000 set 6000000000070C clr 0 > UpdateRegionMappingRecursive(1): 137EE0000 - 137EE1000 set 6000000000070C clr 0 > UpdateRegionMappingRecursive(2): 137EE0000 - 137EE1000 set 6000000000070C clr 0 > UpdateRegionMappingRecursive(3): 137EE0000 - 137EE1000 set 6000000000070C clr 0 > SetUefiImageMemoryAttributes - 0x0000000137EE1000 - 0x000000000008C000 (0x0000000000020008) > UpdateRegionMappingRecursive(0): 137EE1000 - 137F6D000 set 7CC clr 0 > UpdateRegionMappingRecursive(1): 137EE1000 - 137F6D000 set 7CC clr 0 > UpdateRegionMappingRecursive(2): 137EE1000 - 137F6D000 set 7CC clr 0 > UpdateRegionMappingRecursive(3): 137EE1000 - 137F6D000 set 7CC clr 0 > SetUefiImageMemoryAttributes - 0x0000000137F6D000 - 0x0000000000033000 (0x0000000000004008) > UpdateRegionMappingRecursive(0): 137F6D000 - 137FA0000 set 6000000000070C clr 0 > UpdateRegionMappingRecursive(1): 137F6D000 - 137FA0000 set 6000000000070C clr 0 > UpdateRegionMappingRecursive(2): 137F6D000 - 137FA0000 set 6000000000070C clr 0 > UpdateRegionMappingRecursive(3): 137F6D000 - 137FA0000 set 6000000000070C clr 0 > BdsDxe: starting Boot0005 "Red Hat Enterprise Linux" from HD(1,GPT,1F51423F-5044-4C9C-BBF7-666D366B249E,0x800,0x64000)/\EFI\redhat\shimaa64.efi > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > InstallProtocolInterface: 605DAB50-E046-4300-ABB6-3DD810DD8B23 137F806D8 > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 0 clr FF9F000000000F3F > FSOpen: Open '\EFI\redhat\grubaa64.efi' Success > UpdateRegionMappingRecursive(0): 137DD2000 - 137EE0000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(1): 137DD2000 - 137EE0000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(2): 137DD2000 - 137EE0000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(3): 137C00000 - 137E00000 set 6000000000070C clr 0 > UpdateRegionMappingRecursive(3): 137DD2000 - 137E00000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(3): 137E00000 - 137EE0000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(0): 137CC5000 - 137DD2000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(1): 137CC5000 - 137DD2000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(2): 137CC5000 - 137DD2000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(3): 137CC5000 - 137DD2000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(0): 138020000 - 138026000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(1): 138020000 - 138026000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(2): 138020000 - 138026000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(3): 138020000 - 138026000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(0): F8CB0000 - 137CC5000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(1): F8CB0000 - 137CC5000 set 0 clr FF9F000000000F3F > UpdateRegionMappingRecursive(2): C0000000 - 100000000 set 6000000000070C clr 0 > > > Synchronous Exception at 0x000000013BA184F0 > > > Synchronous Exception at 0x000000013BA184F0 > PC 0x00013BA184F0 (0x00013BA14000+0x000044F0) [ 0] ArmCpuDxe.dll > PC 0x00013BA18A30 (0x00013BA14000+0x00004A30) [ 0] ArmCpuDxe.dll > PC 0x00013BA18868 (0x00013BA14000+0x00004868) [ 0] ArmCpuDxe.dll > PC 0x00013BA188D0 (0x00013BA14000+0x000048D0) [ 0] ArmCpuDxe.dll > PC 0x00013BA18B00 (0x00013BA14000+0x00004B00) [ 0] ArmCpuDxe.dll > PC 0x00013BA18C60 (0x00013BA14000+0x00004C60) [ 0] ArmCpuDxe.dll > PC 0x00013BA161E0 (0x00013BA14000+0x000021E0) [ 0] ArmCpuDxe.dll > PC 0x00013F11AEE8 (0x00013F10D000+0x0000DEE8) [ 1] DxeCore.dll > PC 0x00013F1284B8 (0x00013F10D000+0x0001B4B8) [ 1] DxeCore.dll > PC 0x000137CC8380 > PC 0x000137CC8970 > PC 0x000137CC8094 > PC 0x000137CC5478 > PC 0x000137CCCFC8 > PC 0x000137EE6C64 > PC 0x00013F11420C (0x00013F10D000+0x0000720C) [ 1] DxeCore.dll > PC 0x00013B8D5A20 (0x00013B8C5000+0x00010A20) [ 2] BdsDxe.dll > PC 0x00013B8C6FE4 (0x00013B8C5000+0x00001FE4) [ 2] BdsDxe.dll > PC 0x00013B8C8680 (0x00013B8C5000+0x00003680) [ 2] BdsDxe.dll > PC 0x00013F10F88C (0x00013F10D000+0x0000288C) [ 3] DxeCore.dll > PC 0x00013F10E8C0 (0x00013F10D000+0x000018C0) [ 3] DxeCore.dll > PC 0x00013F10E024 (0x00013F10D000+0x00001024) [ 3] DxeCore.dll > > [ 0] Build/ArmVirtQemu-AARCH64/NOOPT_GCC5/AARCH64/ArmPkg/Drivers/CpuDxe/CpuDxe/DEBUG/ArmCpuDxe.dll > [ 1] Build/ArmVirtQemu-AARCH64/NOOPT_GCC5/AARCH64/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/DxeCore.dll > [ 2] Build/ArmVirtQemu-AARCH64/NOOPT_GCC5/AARCH64/MdeModulePkg/Universal/BdsDxe/BdsDxe/DEBUG/BdsDxe.dll > [ 3] Build/ArmVirtQemu-AARCH64/NOOPT_GCC5/AARCH64/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/DxeCore.dll > > X0 0x0000AFAFAFAFA000 X1 0x0000AFAFAFAFA000 X2 0x00000000000C0000 X3 0x0000000000000004 > X4 0x0000000000000200 X5 0x0000000000000000 X6 0x006000000000070C X7 0x0000000000000000 > X8 0x0000000000C5183D X9 0x0000000000C5183C X10 0x000000013A216000 X11 0x000000013B643FFF > X12 0x0000000000000000 X13 0x0000000000000008 X14 0x0000000000000000 X15 0x0000000000000000 > X16 0x00000000220A3B01 X17 0x000000013F10C660 X18 0x00000000CB9AE5A3 X19 0x0000000137CC5000 > X20 0x0000000000000002 X21 0x0000000138020000 X22 0x000000013F158008 X23 0x000000000003F015 > X24 0x00000000F8CB0000 X25 0x0000000000000030 X26 0x00000000000FFFFF X27 0x0000000000100000 > X28 0x0000000000000000 FP 0x000000013F10C530 LR 0x000000013BA18A30 > > V0 0x0000000000000000 0000000000000000 V1 0x0000000000000000 000000009E446499 > V2 0x0000000000000000 00000000E35EF5EF V3 0x0000000000000000 000000005A378E55 > V4 0x0000000000000000 00000000A394033C V5 0x0000000000000000 0000000137F566D8 > V6 0x0000000000000000 00000000430E721A V7 0x0000000000000000 00000000E531AE60 > V8 0x0000000000000000 0000000000000000 V9 0x0000000000000000 0000000000000000 > V10 0x0000000000000000 0000000000000000 V11 0x0000000000000000 0000000000000000 > V12 0x0000000000000000 0000000000000000 V13 0x0000000000000000 0000000000000000 > V14 0x0000000000000000 0000000000000000 V15 0x0000000000000000 0000000000000000 > V16 0x0000000000000000 0000000000000000 V17 0x0000000000000000 0000000000000000 > V18 0x0000000000000000 0000000000000000 V19 0x0000000000000000 0000000000000000 > V20 0x0000000000000000 0000000000000000 V21 0x0000000000000000 0000000000000000 > V22 0x0000000000000000 0000000000000000 V23 0x0000000000000000 0000000000000000 > V24 0x0000000000000000 0000000000000000 V25 0x0000000000000000 0000000000000000 > V26 0x0000000000000000 0000000000000000 V27 0x0000000000000000 0000000000000000 > V28 0x0000000000000000 0000000000000000 V29 0x0000000000000000 0000000000000000 > V30 0x0000000000000000 0000000000000000 V31 0x0000000000000000 0000000000000000 > > SP 0x000000013F10C530 ELR 0x000000013BA184F0 SPSR 0x80000205 FPSR 0x00000000 > ESR 0x96000004 FAR 0x0000AFAFAFAFA000 > > ESR : EC 0x25 IL 0x1 ISS 0x00000004 > > Data abort: Translation fault, zeroth level > > Stack dump: > 000013F10C430: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > 000013F10C450: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > 000013F10C470: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > 000013F10C490: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > 000013F10C4B0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > 000013F10C4D0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > 000013F10C4F0: 0000000000000000 0000000000000000 000000013BA1E140 0000000060000304 > 000013F10C510: 0000000000000000 0000000096000004 0000AFAFAFAFA000 00000000FFFFFFD0 > > 000013F10C530: 000000013F10C560 000000013BA18A30 01000000C0000000 0000AFAFAFAFA000 > 000013F10C550: 00600000C000070D 0000000000000000 000000013F10C5E0 000000013BA18868 > 000013F10C570: 0000000000000002 000000013A216000 0000000000000000 006000000000070C > 000013F10C590: 0000000100000000 00000000C0000000 0000000000000001 00000004FFFFFFD0 > 000013F10C5B0: 00600000C000070D 000000013A216000 00000000C0200000 00000000001FFFFF > 000013F10C5D0: 000000000000002B 0000AFAFAFAFA000 000000013F10C660 000000013BA188D0 > 000013F10C5F0: 0000000000000001 000000013FFFE000 FF9F000000000F3F 0000000000000000 > 000013F10C610: 0000000137CC5000 00000000F8CB0000 000000013F10C630 00000000FFFFFFD0 > ASSERT [ArmCpuDxe] ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c(273): ((BOOLEAN)(0==1)) (3) I bisected the 6-patch range, to find the culprit. Here's the log: > git bisect start > # bad: [31c189a32c370d4abf7a596f7f60a0578e8c7672] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry > git bisect bad 31c189a32c370d4abf7a596f7f60a0578e8c7672 > # good: [a3e25cc8a1dd3d1ea24ed02f90c44221e015e965] OvmfPkg/X86QemuLoadImageLib: fix "unused variable" error in X64 DXE builds > git bisect good a3e25cc8a1dd3d1ea24ed02f90c44221e015e965 > # good: [d4b4f49e4d43276cf410778f172eecab7fc472dc] ArmPkg/ArmMmuLib AARCH64: drop pointless page table memory type check > git bisect good d4b4f49e4d43276cf410778f172eecab7fc472dc > # good: [8952f2498dcdc2dfbba8216007bb0a4745d944b2] ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types > git bisect good 8952f2498dcdc2dfbba8216007bb0a4745d944b2 > # first bad commit: [31c189a32c370d4abf7a596f7f60a0578e8c7672] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry Therefore, I decided to push the first four patches (first two sets), via <https://github.com/tianocore/edk2/pull/431>, and to report a problem with this one. Thanks Laszlo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry 2020-03-10 0:29 ` [edk2-devel] " Laszlo Ersek @ 2020-03-25 9:40 ` Ard Biesheuvel 0 siblings, 0 replies; 6+ messages in thread From: Ard Biesheuvel @ 2020-03-25 9:40 UTC (permalink / raw) To: edk2-devel-groups-io, Laszlo Ersek; +Cc: Leif Lindholm On Tue, 10 Mar 2020 at 01:29, Laszlo Ersek <lersek@redhat.com> wrote: > > Hi Ard, > > On 03/07/20 14:34, 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> > > --- > > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 21 ++++++++++++++++++-- > > 1 file changed, 19 insertions(+), 2 deletions(-) > > > > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > > index 6f6ef5b05fbc..7b2c36a7a538 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)) { > > @@ -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; > > > > This patch crashes an existent (RHEL-ALT-7) guest for me, when it tries > to launch "grubaa64.efi". > > (1) I closed your PR#430 as explained here: > <https://edk2.groups.io/g/devel/message/55695>, and collected *six* of > your reviewed patches, for a new PR: > > - [edk2-devel] [PATCH v4 0/2] ArmPkg/ArmMmuLib: rewrite and improve > cache handling with MMU off > > - [edk2-devel] [PATCH 0/2] ArmPkg/ArmMmuLib AARCH64: final cleanups > > - the present set > > > (2) Before submitting the PR, I figured I'd boot one of my permanent > QEMU (not KVM) guests, with all six patches applied (locally, for the > time). The boot crashed; here's the end of the log: > > > [Bds]Booting Red Hat Enterprise Linux > > FSOpen: Open '\EFI\redhat\shimaa64.efi' Success > > [Bds] Expand HD(1,GPT,1F51423F-5044-4C9C-BBF7-666D366B249E,0x800,0x64000)/\EFI\redhat\shimaa64.efi -> PciRoot(0x0)/Pci(0x1,0x6)/Pci(0x0,0x0)/Scsi(0x0,0x0)/HD(1,GPT,1F51423F-5044-4C9C-BBF7-666D366B249E,0x800,0x64000)/\EFI\redhat\shimaa64.efi > > BdsDxe: loading Boot0005 "Red Hat Enterprise Linux" from HD(1,GPT,1F51423F-5044-4C9C-BBF7-666D366B249E,0x800,0x64000)/\EFI\redhat\shimaa64.efi > > [Security] 3rd party image[0] can be loaded after EndOfDxe: PciRoot(0x0)/Pci(0x1,0x6)/Pci(0x0,0x0)/Scsi(0x0,0x0)/HD(1,GPT,1F51423F-5044-4C9C-BBF7-666D366B249E,0x800,0x64000)/\EFI\redhat\shimaa64.efi. > > InstallProtocolInterface: 5B1B31A1-9562-11D2-8E3F-00A0C969723B 13A2196C0 > > UpdateRegionMappingRecursive(0): 137EE0000 - 137FA0000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(1): 137EE0000 - 137FA0000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(2): 137EE0000 - 137FA0000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(3): 137E00000 - 138000000 set 6000000000070C clr 0 > > UpdateRegionMappingRecursive(3): 137EE0000 - 137FA0000 set 0 clr FF9F000000000F3F > > Loading driver at 0x00137EE0000 EntryPoint=0x00137EE1000 > > Loading driver at 0x00137EE0000 EntryPoint=0x00137EE1000 > > InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 13A124498 > > ProtectUefiImageCommon - 0x3A2196C0 > > - 0x0000000137EE0000 - 0x00000000000BF500 > > Section - '.text ' > > VirtualSize - 0x0008C000 > > VirtualAddress - 0x00001000 > > SizeOfRawData - 0x0008C000 > > PointerToRawData - 0x00001000 > > PointerToRelocations - 0x00000000 > > PointerToLinenumbers - 0x00000000 > > NumberOfRelocations - 0x00000000 > > NumberOfLinenumbers - 0x00000000 > > Characteristics - 0x60000020 > > ImageCode: 0x0000000137EE1000 - 0x000000000008C000 > > Section - '.data ' > > ImageCode SegmentCount - 0x1 > > SetUefiImageMemoryAttributes - 0x0000000137EE0000 - 0x0000000000001000 (0x0000000000004008) > > UpdateRegionMappingRecursive(0): 137EE0000 - 137EE1000 set 6000000000070C clr 0 > > UpdateRegionMappingRecursive(1): 137EE0000 - 137EE1000 set 6000000000070C clr 0 > > UpdateRegionMappingRecursive(2): 137EE0000 - 137EE1000 set 6000000000070C clr 0 > > UpdateRegionMappingRecursive(3): 137EE0000 - 137EE1000 set 6000000000070C clr 0 > > SetUefiImageMemoryAttributes - 0x0000000137EE1000 - 0x000000000008C000 (0x0000000000020008) > > UpdateRegionMappingRecursive(0): 137EE1000 - 137F6D000 set 7CC clr 0 > > UpdateRegionMappingRecursive(1): 137EE1000 - 137F6D000 set 7CC clr 0 > > UpdateRegionMappingRecursive(2): 137EE1000 - 137F6D000 set 7CC clr 0 > > UpdateRegionMappingRecursive(3): 137EE1000 - 137F6D000 set 7CC clr 0 > > SetUefiImageMemoryAttributes - 0x0000000137F6D000 - 0x0000000000033000 (0x0000000000004008) > > UpdateRegionMappingRecursive(0): 137F6D000 - 137FA0000 set 6000000000070C clr 0 > > UpdateRegionMappingRecursive(1): 137F6D000 - 137FA0000 set 6000000000070C clr 0 > > UpdateRegionMappingRecursive(2): 137F6D000 - 137FA0000 set 6000000000070C clr 0 > > UpdateRegionMappingRecursive(3): 137F6D000 - 137FA0000 set 6000000000070C clr 0 > > BdsDxe: starting Boot0005 "Red Hat Enterprise Linux" from HD(1,GPT,1F51423F-5044-4C9C-BBF7-666D366B249E,0x800,0x64000)/\EFI\redhat\shimaa64.efi > > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > > InstallProtocolInterface: 605DAB50-E046-4300-ABB6-3DD810DD8B23 137F806D8 > > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 60000000000000 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(0): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(1): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(2): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(3): 138112000 - 138113000 set 0 clr FF9F000000000F3F > > FSOpen: Open '\EFI\redhat\grubaa64.efi' Success > > UpdateRegionMappingRecursive(0): 137DD2000 - 137EE0000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(1): 137DD2000 - 137EE0000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(2): 137DD2000 - 137EE0000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(3): 137C00000 - 137E00000 set 6000000000070C clr 0 > > UpdateRegionMappingRecursive(3): 137DD2000 - 137E00000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(3): 137E00000 - 137EE0000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(0): 137CC5000 - 137DD2000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(1): 137CC5000 - 137DD2000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(2): 137CC5000 - 137DD2000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(3): 137CC5000 - 137DD2000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(0): 138020000 - 138026000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(1): 138020000 - 138026000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(2): 138020000 - 138026000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(3): 138020000 - 138026000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(0): F8CB0000 - 137CC5000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(1): F8CB0000 - 137CC5000 set 0 clr FF9F000000000F3F > > UpdateRegionMappingRecursive(2): C0000000 - 100000000 set 6000000000070C clr 0 > > > > > > Synchronous Exception at 0x000000013BA184F0 > > > > > > Synchronous Exception at 0x000000013BA184F0 > > PC 0x00013BA184F0 (0x00013BA14000+0x000044F0) [ 0] ArmCpuDxe.dll > > PC 0x00013BA18A30 (0x00013BA14000+0x00004A30) [ 0] ArmCpuDxe.dll > > PC 0x00013BA18868 (0x00013BA14000+0x00004868) [ 0] ArmCpuDxe.dll > > PC 0x00013BA188D0 (0x00013BA14000+0x000048D0) [ 0] ArmCpuDxe.dll > > PC 0x00013BA18B00 (0x00013BA14000+0x00004B00) [ 0] ArmCpuDxe.dll > > PC 0x00013BA18C60 (0x00013BA14000+0x00004C60) [ 0] ArmCpuDxe.dll > > PC 0x00013BA161E0 (0x00013BA14000+0x000021E0) [ 0] ArmCpuDxe.dll > > PC 0x00013F11AEE8 (0x00013F10D000+0x0000DEE8) [ 1] DxeCore.dll > > PC 0x00013F1284B8 (0x00013F10D000+0x0001B4B8) [ 1] DxeCore.dll > > PC 0x000137CC8380 > > PC 0x000137CC8970 > > PC 0x000137CC8094 > > PC 0x000137CC5478 > > PC 0x000137CCCFC8 > > PC 0x000137EE6C64 > > PC 0x00013F11420C (0x00013F10D000+0x0000720C) [ 1] DxeCore.dll > > PC 0x00013B8D5A20 (0x00013B8C5000+0x00010A20) [ 2] BdsDxe.dll > > PC 0x00013B8C6FE4 (0x00013B8C5000+0x00001FE4) [ 2] BdsDxe.dll > > PC 0x00013B8C8680 (0x00013B8C5000+0x00003680) [ 2] BdsDxe.dll > > PC 0x00013F10F88C (0x00013F10D000+0x0000288C) [ 3] DxeCore.dll > > PC 0x00013F10E8C0 (0x00013F10D000+0x000018C0) [ 3] DxeCore.dll > > PC 0x00013F10E024 (0x00013F10D000+0x00001024) [ 3] DxeCore.dll > > > > [ 0] Build/ArmVirtQemu-AARCH64/NOOPT_GCC5/AARCH64/ArmPkg/Drivers/CpuDxe/CpuDxe/DEBUG/ArmCpuDxe.dll > > [ 1] Build/ArmVirtQemu-AARCH64/NOOPT_GCC5/AARCH64/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/DxeCore.dll > > [ 2] Build/ArmVirtQemu-AARCH64/NOOPT_GCC5/AARCH64/MdeModulePkg/Universal/BdsDxe/BdsDxe/DEBUG/BdsDxe.dll > > [ 3] Build/ArmVirtQemu-AARCH64/NOOPT_GCC5/AARCH64/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/DxeCore.dll > > > > X0 0x0000AFAFAFAFA000 X1 0x0000AFAFAFAFA000 X2 0x00000000000C0000 X3 0x0000000000000004 > > X4 0x0000000000000200 X5 0x0000000000000000 X6 0x006000000000070C X7 0x0000000000000000 > > X8 0x0000000000C5183D X9 0x0000000000C5183C X10 0x000000013A216000 X11 0x000000013B643FFF > > X12 0x0000000000000000 X13 0x0000000000000008 X14 0x0000000000000000 X15 0x0000000000000000 > > X16 0x00000000220A3B01 X17 0x000000013F10C660 X18 0x00000000CB9AE5A3 X19 0x0000000137CC5000 > > X20 0x0000000000000002 X21 0x0000000138020000 X22 0x000000013F158008 X23 0x000000000003F015 > > X24 0x00000000F8CB0000 X25 0x0000000000000030 X26 0x00000000000FFFFF X27 0x0000000000100000 > > X28 0x0000000000000000 FP 0x000000013F10C530 LR 0x000000013BA18A30 > > > > V0 0x0000000000000000 0000000000000000 V1 0x0000000000000000 000000009E446499 > > V2 0x0000000000000000 00000000E35EF5EF V3 0x0000000000000000 000000005A378E55 > > V4 0x0000000000000000 00000000A394033C V5 0x0000000000000000 0000000137F566D8 > > V6 0x0000000000000000 00000000430E721A V7 0x0000000000000000 00000000E531AE60 > > V8 0x0000000000000000 0000000000000000 V9 0x0000000000000000 0000000000000000 > > V10 0x0000000000000000 0000000000000000 V11 0x0000000000000000 0000000000000000 > > V12 0x0000000000000000 0000000000000000 V13 0x0000000000000000 0000000000000000 > > V14 0x0000000000000000 0000000000000000 V15 0x0000000000000000 0000000000000000 > > V16 0x0000000000000000 0000000000000000 V17 0x0000000000000000 0000000000000000 > > V18 0x0000000000000000 0000000000000000 V19 0x0000000000000000 0000000000000000 > > V20 0x0000000000000000 0000000000000000 V21 0x0000000000000000 0000000000000000 > > V22 0x0000000000000000 0000000000000000 V23 0x0000000000000000 0000000000000000 > > V24 0x0000000000000000 0000000000000000 V25 0x0000000000000000 0000000000000000 > > V26 0x0000000000000000 0000000000000000 V27 0x0000000000000000 0000000000000000 > > V28 0x0000000000000000 0000000000000000 V29 0x0000000000000000 0000000000000000 > > V30 0x0000000000000000 0000000000000000 V31 0x0000000000000000 0000000000000000 > > > > SP 0x000000013F10C530 ELR 0x000000013BA184F0 SPSR 0x80000205 FPSR 0x00000000 > > ESR 0x96000004 FAR 0x0000AFAFAFAFA000 > > > > ESR : EC 0x25 IL 0x1 ISS 0x00000004 > > > > Data abort: Translation fault, zeroth level > > > > Stack dump: > > 000013F10C430: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > > 000013F10C450: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > > 000013F10C470: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > > 000013F10C490: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > > 000013F10C4B0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > > 000013F10C4D0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > > 000013F10C4F0: 0000000000000000 0000000000000000 000000013BA1E140 0000000060000304 > > 000013F10C510: 0000000000000000 0000000096000004 0000AFAFAFAFA000 00000000FFFFFFD0 > > > 000013F10C530: 000000013F10C560 000000013BA18A30 01000000C0000000 0000AFAFAFAFA000 > > 000013F10C550: 00600000C000070D 0000000000000000 000000013F10C5E0 000000013BA18868 > > 000013F10C570: 0000000000000002 000000013A216000 0000000000000000 006000000000070C > > 000013F10C590: 0000000100000000 00000000C0000000 0000000000000001 00000004FFFFFFD0 > > 000013F10C5B0: 00600000C000070D 000000013A216000 00000000C0200000 00000000001FFFFF > > 000013F10C5D0: 000000000000002B 0000AFAFAFAFA000 000000013F10C660 000000013BA188D0 > > 000013F10C5F0: 0000000000000001 000000013FFFE000 FF9F000000000F3F 0000000000000000 > > 000013F10C610: 0000000137CC5000 00000000F8CB0000 000000013F10C630 00000000FFFFFFD0 > > ASSERT [ArmCpuDxe] ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c(273): ((BOOLEAN)(0==1)) > > > (3) I bisected the 6-patch range, to find the culprit. Here's the log: > > > git bisect start > > # bad: [31c189a32c370d4abf7a596f7f60a0578e8c7672] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry > > git bisect bad 31c189a32c370d4abf7a596f7f60a0578e8c7672 > > # good: [a3e25cc8a1dd3d1ea24ed02f90c44221e015e965] OvmfPkg/X86QemuLoadImageLib: fix "unused variable" error in X64 DXE builds > > git bisect good a3e25cc8a1dd3d1ea24ed02f90c44221e015e965 > > # good: [d4b4f49e4d43276cf410778f172eecab7fc472dc] ArmPkg/ArmMmuLib AARCH64: drop pointless page table memory type check > > git bisect good d4b4f49e4d43276cf410778f172eecab7fc472dc > > # good: [8952f2498dcdc2dfbba8216007bb0a4745d944b2] ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types > > git bisect good 8952f2498dcdc2dfbba8216007bb0a4745d944b2 > > # first bad commit: [31c189a32c370d4abf7a596f7f60a0578e8c7672] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry > > Therefore, I decided to push the first four patches (first two sets), > via <https://github.com/tianocore/edk2/pull/431>, and to report a > problem with this one. > Thanks, Laszlo, for taking care of this. I had another report of some issues with the new MMU code, and I'll take a look today. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] ArmPkg/ArmMmuLib AARCH64: correctness fix 2020-03-07 13:34 [PATCH 0/2] ArmPkg/ArmMmuLib AARCH64: correctness fix Ard Biesheuvel 2020-03-07 13:34 ` [PATCH 1/2] ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types Ard Biesheuvel 2020-03-07 13:34 ` [PATCH 2/2] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry Ard Biesheuvel @ 2020-03-09 13:16 ` Leif Lindholm 2 siblings, 0 replies; 6+ messages in thread From: Leif Lindholm @ 2020-03-09 13:16 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: devel On Sat, Mar 07, 2020 at 14:34:13 +0100, Ard Biesheuvel wrote: > Last one, I promise :-) > > The new ArmMmuLib code is easier to reason about, so that is what I did: > currently, when we create mappings that cover existing table entries, we > may end up overwriting those with block entries without taking the mapping > attributes of the original table entries into account. So let's fix this. > > I honestly don't know whether the original code was better at dealing with > this: I do remember some changes from Heyi that may have been related, but > the old code is not easy to follow. In any case, I didn't manage to hit this > case in practice, given that we typically start out with large mappings, and > break them down later (to set permissions), rather than the other way around. > > Patch #1 adds some helpers to hide the insane way the type bits change > meaning when you change to level 3. > > Patch #2 ensures that we only replace (and free) table entries with block > entries if it is guaranteed that doing so will not lose any attribute > information. For the series: Reviewed-by: Leif Lindholm <leif@nuviainc.com> Thanks! > Ard Biesheuvel (2): > ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types > ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table > entry > > .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 61 ++++++++++++++++--- > 1 file changed, 54 insertions(+), 7 deletions(-) > > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-25 9:40 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-03-07 13:34 [PATCH 0/2] ArmPkg/ArmMmuLib AARCH64: correctness fix Ard Biesheuvel 2020-03-07 13:34 ` [PATCH 1/2] ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types Ard Biesheuvel 2020-03-07 13:34 ` [PATCH 2/2] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry Ard Biesheuvel 2020-03-10 0:29 ` [edk2-devel] " Laszlo Ersek 2020-03-25 9:40 ` Ard Biesheuvel 2020-03-09 13:16 ` [PATCH 0/2] ArmPkg/ArmMmuLib AARCH64: correctness fix Leif Lindholm
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox