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.biesheu...@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55701): https://edk2.groups.io/g/devel/message/55701 Mute This Topic: https://groups.io/mt/71795104/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-