From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by mx.groups.io with SMTP id smtpd.web10.10270.1583565352094665375 for ; Fri, 06 Mar 2020 23:15:52 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=NmL4P8pO; spf=pass (domain: linaro.org, ip: 209.85.128.67, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wm1-f67.google.com with SMTP id e26so4615428wme.5 for ; Fri, 06 Mar 2020 23:15:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Js6J1xQ61LzsTcrPN16875rd4SmgehZ3QLL9q8ZRTb4=; b=NmL4P8pO1Ai3kS699mfADrN9aI41WjwrjGDhC3JEKDWxec6SwpP81GSMI3dz4tkyF5 yCdcZzdp1JS4wy/CtjzqleupWzetLX1QOyUiPfOldN6O9Ww5cteExHdo1iEtEqcAo/du CQg4h/vJ8yU30E/HPMSuF77A3nEH65oodF3bvVWKJs6LxPB8rPqoIEmrodMMI/ohaB5E YRLPo3SQzqwtibTRXItKIZB1VEUyPKb7Gt+qR4dNHLAP83Uo8IG7LSNRvby0j33S5QK4 2a8YvcZbKzI4umFwnUha5nn/Hf0jimfCszTyR1LoyXxPg/T+KTjqeRToQu6eJ6XELtNh dSmA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Js6J1xQ61LzsTcrPN16875rd4SmgehZ3QLL9q8ZRTb4=; b=DOByLLtN+e1qPdRpovvGwVK0sc278IUWBs4LdwOV8S1iUEFSMEtES5wYsoCCak6btB PcjwAgfq9r3zBlbIfoUhaKWocjx5GnXkwGj5LVdfFBQfg/ZjL675aXuAUqETgOPnMQG5 yNJ7DSIfiDSedNnGGf64P9Msr5z9zP1950qWl2swA/8KmDlkf4u5jUxeOU02pE0DPSMH 9lp2SzVtuiXixDO8vyvln6O4KTVuYllD/0UX/5Y1r5NM5kspSvZmW8VSHLJSz25IVEQk 5eTagaL2uQTHkNm2If2obstP6pAZehC6f7cYSoVdh9ndBp5xOCd2t8+ytIPzS2MjJDVx DdqA== X-Gm-Message-State: ANhLgQ3PN/XX75Kdqjaj0SRP8sDc6pE/7vc0aSvn4ynmemA7R5FALyjN wQx5deiFh6oLg9bJiK+DUVY5dTr2/RcS/JjSwJjuqQ== X-Google-Smtp-Source: ADFU+vtDfXV+mm46oCI3MgN/3QyOjfc+Ns1kAhJdleMQmMBYzVbp2xpWnNztgOCfx9CO7PgJJdfV6ZfpHluMh+YtWzk= X-Received: by 2002:a05:600c:24b:: with SMTP id 11mr8278368wmj.1.1583565350354; Fri, 06 Mar 2020 23:15:50 -0800 (PST) MIME-Version: 1.0 References: <20200306161246.6392-1-ard.biesheuvel@linaro.org> <20200306161246.6392-2-ard.biesheuvel@linaro.org> <20200306185034.GJ23627@bivouac.eciton.net> In-Reply-To: <20200306185034.GJ23627@bivouac.eciton.net> From: "Ard Biesheuvel" Date: Sat, 7 Mar 2020 08:15:39 +0100 Message-ID: Subject: Re: [PATCH v3 1/2] ArmPkg/ArmMmuLib AARCH64: rewrite page table code To: Leif Lindholm Cc: edk2-devel-groups-io Content-Type: text/plain; charset="UTF-8" On Fri, 6 Mar 2020 at 19:50, Leif Lindholm wrote: > > On Fri, Mar 06, 2020 at 17:12:45 +0100, Ard Biesheuvel wrote: > > Replace the slightly overcomplicated page table management code with > > a simplified, recursive implementation that should be far easier to > > reason about. > > > > Signed-off-by: Ard Biesheuvel > > --- > > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 332 +++++++------------- > > 1 file changed, 108 insertions(+), 224 deletions(-) > > > > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > > index 204e33c75f95..e36594fea3ad 100644 > > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > > @@ -121,14 +121,16 @@ GetRootTranslationTableInfo ( > > > > STATIC > > VOID > > -ReplaceLiveEntry ( > > +ReplaceTableEntry ( > > IN UINT64 *Entry, > > IN UINT64 Value, > > - IN UINT64 RegionStart > > + IN UINT64 RegionStart, > > + IN BOOLEAN IsLiveBlockMapping > > ) > > { > > - if (!ArmMmuEnabled ()) { > > + if (!ArmMmuEnabled () || !IsLiveBlockMapping) { > > *Entry = Value; > > + ArmUpdateTranslationTableEntry (Entry, (VOID *)(UINTN)RegionStart); > > } else { > > ArmReplaceLiveTranslationEntry (Entry, Value, RegionStart); > > } > > @@ -165,229 +167,132 @@ LookupAddresstoRootTable ( > > } > > > > STATIC > > -UINT64* > > -GetBlockEntryListFromAddress ( > > - IN UINT64 *RootTable, > > - IN UINT64 RegionStart, > > - OUT UINTN *TableLevel, > > - IN OUT UINT64 *BlockEntrySize, > > - OUT UINT64 **LastBlockEntry > > +EFI_STATUS > > +UpdateRegionMappingRec ( > > What does Rec stand for? Record? Recursively? > Could it be written out? > > > + IN UINT64 RegionStart, > > + IN UINT64 RegionEnd, > > + IN UINT64 AttributeSetMask, > > + IN UINT64 AttributeClearMask, > > + IN UINT64 *PageTable, > > + IN UINTN Level > > This sets the scene for a spectacularly unhelpful diff. > Would you consider adding UpdateRegionMappingRec() *before* > LookupAddresstoRootTable() for the only purpose of working against the > shortcomings of the line diff algorithm? That is what I ended up doing > locally for the review. > Yeah works for me > > ) > > { > > - UINTN RootTableLevel; > > - UINTN RootTableEntryCount; > > - UINT64 *TranslationTable; > > - UINT64 *BlockEntry; > > - UINT64 *SubTableBlockEntry; > > - UINT64 BlockEntryAddress; > > - UINTN BaseAddressAlignment; > > - UINTN PageLevel; > > - UINTN Index; > > - UINTN IndexLevel; > > - UINTN T0SZ; > > - UINT64 Attributes; > > - UINT64 TableAttributes; > > - > > - // Initialize variable > > - BlockEntry = NULL; > > - > > - // Ensure the parameters are valid > > - if (!(TableLevel && BlockEntrySize && LastBlockEntry)) { > > - ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER); > > - return NULL; > > - } > > - > > - // Ensure the Region is aligned on 4KB boundary > > - if ((RegionStart & (SIZE_4KB - 1)) != 0) { > > - ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER); > > - return NULL; > > - } > > - > > - // Ensure the required size is aligned on 4KB boundary and not 0 > > - if ((*BlockEntrySize & (SIZE_4KB - 1)) != 0 || *BlockEntrySize == 0) { > > - ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER); > > - return NULL; > > - } > > - > > - T0SZ = ArmGetTCR () & TCR_T0SZ_MASK; > > - // Get the Table info from T0SZ > > - GetRootTranslationTableInfo (T0SZ, &RootTableLevel, &RootTableEntryCount); > > - > > - // If the start address is 0x0 then we use the size of the region to identify the alignment > > - if (RegionStart == 0) { > > - // Identify the highest possible alignment for the Region Size > > - BaseAddressAlignment = LowBitSet64 (*BlockEntrySize); > > - } else { > > - // Identify the highest possible alignment for the Base Address > > - BaseAddressAlignment = LowBitSet64 (RegionStart); > > - } > > - > > - // Identify the Page Level the RegionStart must belong to. Note that PageLevel > > - // should be at least 1 since block translations are not supported at level 0 > > - PageLevel = MAX (3 - ((BaseAddressAlignment - 12) / 9), 1); > > - > > - // If the required size is smaller than the current block size then we need to go to the page below. > > - // The PageLevel was calculated on the Base Address alignment but did not take in account the alignment > > - // of the allocation size > > - while (*BlockEntrySize < TT_BLOCK_ENTRY_SIZE_AT_LEVEL (PageLevel)) { > > - // It does not fit so we need to go a page level above > > - PageLevel++; > > - } > > - > > - // > > - // Get the Table Descriptor for the corresponding PageLevel. We need to decompose RegionStart to get appropriate entries > > - // > > - > > - TranslationTable = RootTable; > > - for (IndexLevel = RootTableLevel; IndexLevel <= PageLevel; IndexLevel++) { > > - BlockEntry = (UINT64*)TT_GET_ENTRY_FOR_ADDRESS (TranslationTable, IndexLevel, RegionStart); > > - > > - if ((IndexLevel != 3) && ((*BlockEntry & TT_TYPE_MASK) == TT_TYPE_TABLE_ENTRY)) { > > - // Go to the next table > > - TranslationTable = (UINT64*)(*BlockEntry & TT_ADDRESS_MASK_DESCRIPTION_TABLE); > > - > > - // If we are at the last level then update the last level to next level > > - if (IndexLevel == PageLevel) { > > - // Enter the next level > > - PageLevel++; > > - } > > - } else if ((*BlockEntry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) { > > - // If we are not at the last level then we need to split this BlockEntry > > - if (IndexLevel != PageLevel) { > > - // Retrieve the attributes from the block entry > > - Attributes = *BlockEntry & TT_ATTRIBUTES_MASK; > > - > > - // Convert the block entry attributes into Table descriptor attributes > > - TableAttributes = TT_TABLE_AP_NO_PERMISSION; > > - if (Attributes & TT_NS) { > > - TableAttributes = TT_TABLE_NS; > > + UINTN BlockShift; > > + UINT64 BlockMask; > > + UINT64 BlockEnd; > > + UINT64 *Entry; > > + UINT64 EntryValue; > > + VOID *TranslationTable; > > + EFI_STATUS Status; > > + > > + ASSERT (((RegionStart | RegionEnd) & EFI_PAGE_MASK) == 0); > > + > > + BlockShift = (Level + 1) * BITS_PER_LEVEL + MIN_T0SZ; > > + BlockMask = MAX_UINT64 >> BlockShift; > > + > > + DEBUG ((DEBUG_VERBOSE, "%a(%d): %llx - %llx set %lx clr %lx\n", __FUNCTION__, > > + Level, RegionStart, RegionEnd, AttributeSetMask, AttributeClearMask)); > > + > > + for (; RegionStart < RegionEnd; RegionStart = BlockEnd) { > > + BlockEnd = MIN (RegionEnd, (RegionStart | BlockMask) + 1); > > + Entry = &PageTable[(RegionStart >> (64 - BlockShift)) & (TT_ENTRY_COUNT - 1)]; > > + > > + // > > + // If RegionStart or BlockEnd is not aligned to the block size at this > > + // level, we will have to create a table mapping in order to map less > > + // 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 (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0) { > > + ASSERT (Level < 3); > > + > > + if ((*Entry & TT_TYPE_MASK) != TT_TYPE_TABLE_ENTRY) { > > + // > > + // No table entry exists yet, so we need to allocate a page table > > + // for the next level. > > + // > > + TranslationTable = AllocatePages (1); > > + if (TranslationTable == NULL) { > > + return EFI_OUT_OF_RESOURCES; > > } > > > > - // Get the address corresponding at this entry > > - BlockEntryAddress = RegionStart; > > - BlockEntryAddress = BlockEntryAddress >> TT_ADDRESS_OFFSET_AT_LEVEL(IndexLevel); > > - // Shift back to right to set zero before the effective address > > - BlockEntryAddress = BlockEntryAddress << TT_ADDRESS_OFFSET_AT_LEVEL(IndexLevel); > > - > > - // Set the correct entry type for the next page level > > - if ((IndexLevel + 1) == 3) { > > - Attributes |= TT_TYPE_BLOCK_ENTRY_LEVEL3; > > + if ((*Entry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) { > > + // > > + // We are splitting an existing block entry, so we have to populate > > + // the new table with the attributes of the block entry it replaces. > > + // > > + Status = UpdateRegionMappingRec ( > > + RegionStart & ~BlockMask, > > + (RegionStart | BlockMask) + 1, > > + *Entry & TT_ATTRIBUTES_MASK, > > + 0, > > + TranslationTable, > > + Level + 1); > > + if (EFI_ERROR (Status)) { > > I realise a EFI_OUT_OF_RESOURCES return value here means we would be > in dire straits already, but it would be nice if we could free the > local TranslationTable instead of leaking all allocations through the > tree. > > If not worth the effort (or impossible condition), would still be > worth an explicit comment here. > In PEI, it wouldn't help since FreePages() is a NOP there. But this library is also used by CpuDxe, so it does make sense. And the nice thing about recursion is that it is much simpler to reason about: we only need to free the page if it was allocated in the context of the same call, and everything else comes out in the wash. > > + return Status; > > + } > > } else { > > - Attributes |= TT_TYPE_BLOCK_ENTRY; > > + ZeroMem (TranslationTable, EFI_PAGE_SIZE); > > } > > + } else { > > + TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY); > > + } > > > > - // Create a new translation table > > - TranslationTable = AllocatePages (1); > > - if (TranslationTable == NULL) { > > - return NULL; > > - } > > - > > - // Populate the newly created lower level table > > - SubTableBlockEntry = TranslationTable; > > - for (Index = 0; Index < TT_ENTRY_COUNT; Index++) { > > - *SubTableBlockEntry = Attributes | (BlockEntryAddress + (Index << TT_ADDRESS_OFFSET_AT_LEVEL(IndexLevel + 1))); > > - SubTableBlockEntry++; > > - } > > + // > > + // Recurse to the next level > > + // > > + Status = UpdateRegionMappingRec ( > > + RegionStart, > > + BlockEnd, > > + AttributeSetMask, > > + AttributeClearMask, > > + TranslationTable, > > + Level + 1); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > > > - // Fill the BlockEntry with the new TranslationTable > > - ReplaceLiveEntry (BlockEntry, > > - (UINTN)TranslationTable | TableAttributes | TT_TYPE_TABLE_ENTRY, > > - RegionStart); > > + if ((*Entry & TT_TYPE_MASK) != TT_TYPE_TABLE_ENTRY) { > > + ReplaceTableEntry (Entry, > > + (UINT64)TranslationTable | TT_TYPE_TABLE_ENTRY, > > + RegionStart, > > + (*Entry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY); > > } > > } else { > > - if (IndexLevel != PageLevel) { > > - // > > - // Case when we have an Invalid Entry and we are at a page level above of the one targetted. > > - // > > + EntryValue = (*Entry & AttributeClearMask) | AttributeSetMask; > > + EntryValue |= RegionStart; > > + EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3 > > + : TT_TYPE_BLOCK_ENTRY; > > > > - // Create a new translation table > > - TranslationTable = AllocatePages (1); > > - if (TranslationTable == NULL) { > > - return NULL; > > - } > > - > > - ZeroMem (TranslationTable, TT_ENTRY_COUNT * sizeof(UINT64)); > > - > > - // Fill the new BlockEntry with the TranslationTable > > - *BlockEntry = ((UINTN)TranslationTable & TT_ADDRESS_MASK_DESCRIPTION_TABLE) | TT_TYPE_TABLE_ENTRY; > > - } > > + ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE); > > } > > } > > - > > - // Expose the found PageLevel to the caller > > - *TableLevel = PageLevel; > > - > > - // Now, we have the Table Level we can get the Block Size associated to this table > > - *BlockEntrySize = TT_BLOCK_ENTRY_SIZE_AT_LEVEL (PageLevel); > > - > > - // The last block of the root table depends on the number of entry in this table, > > - // otherwise it is always the (TT_ENTRY_COUNT - 1)th entry in the table. > > - *LastBlockEntry = TT_LAST_BLOCK_ADDRESS(TranslationTable, > > - (PageLevel == RootTableLevel) ? RootTableEntryCount : TT_ENTRY_COUNT); > > - > > - return BlockEntry; > > + return EFI_SUCCESS; > > } > > > > STATIC > > EFI_STATUS > > UpdateRegionMapping ( > > - IN UINT64 *RootTable, > > - IN UINT64 RegionStart, > > - IN UINT64 RegionLength, > > - IN UINT64 Attributes, > > - IN UINT64 BlockEntryMask > > + IN UINT64 RegionStart, > > + IN UINT64 RegionSize, > > + IN UINT64 AttributeSetMask, > > + IN UINT64 AttributeClearMask > > The new indentation here (which arguably is an improvement) sort of > masks the unchanged input RegionStart and the rename of > RegionLength->RegionSize. Since it doesn't help anything in this > series, could it be deferred (or broken out)? > I'll revert this change for now (include the name change since it is pointless) > > ) > > { > > - UINT32 Type; > > - UINT64 *BlockEntry; > > - UINT64 *LastBlockEntry; > > - UINT64 BlockEntrySize; > > - UINTN TableLevel; > > + UINTN RootTableLevel; > > + UINTN T0SZ; > > > > - // Ensure the Length is aligned on 4KB boundary > > - if ((RegionLength == 0) || ((RegionLength & (SIZE_4KB - 1)) != 0)) { > > - ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER); > > + if ((RegionStart & EFI_PAGE_MASK) != 0 || (RegionSize & EFI_PAGE_MASK) != 0) { > > return EFI_INVALID_PARAMETER; > > } > > > > - do { > > - // Get the first Block Entry that matches the Virtual Address and also the information on the Table Descriptor > > - // such as the size of the Block Entry and the address of the last BlockEntry of the Table Descriptor > > - BlockEntrySize = RegionLength; > > - BlockEntry = GetBlockEntryListFromAddress (RootTable, RegionStart, &TableLevel, &BlockEntrySize, &LastBlockEntry); > > - if (BlockEntry == NULL) { > > - // GetBlockEntryListFromAddress() return NULL when it fails to allocate new pages from the Translation Tables > > - return EFI_OUT_OF_RESOURCES; > > - } > > + T0SZ = ArmGetTCR () & TCR_T0SZ_MASK; > > + GetRootTranslationTableInfo (T0SZ, &RootTableLevel, NULL); > > > > - if (TableLevel != 3) { > > - Type = TT_TYPE_BLOCK_ENTRY; > > - } else { > > - Type = TT_TYPE_BLOCK_ENTRY_LEVEL3; > > - } > > - > > - do { > > - // Fill the Block Entry with attribute and output block address > > - *BlockEntry &= BlockEntryMask; > > - *BlockEntry |= (RegionStart & TT_ADDRESS_MASK_BLOCK_ENTRY) | Attributes | Type; > > - > > - ArmUpdateTranslationTableEntry (BlockEntry, (VOID *)RegionStart); > > - > > - // Go to the next BlockEntry > > - RegionStart += BlockEntrySize; > > - RegionLength -= BlockEntrySize; > > - BlockEntry++; > > - > > - // Break the inner loop when next block is a table > > - // Rerun GetBlockEntryListFromAddress to avoid page table memory leak > > - if (TableLevel != 3 && BlockEntry <= LastBlockEntry && > > - (*BlockEntry & TT_TYPE_MASK) == TT_TYPE_TABLE_ENTRY) { > > - break; > > - } > > - } while ((RegionLength >= BlockEntrySize) && (BlockEntry <= LastBlockEntry)); > > - } while (RegionLength != 0); > > - > > - return EFI_SUCCESS; > > + return UpdateRegionMappingRec (RegionStart, RegionStart + RegionSize, > > + AttributeSetMask, AttributeClearMask, ArmGetTTBR0BaseAddress (), > > + RootTableLevel); > > } > > > > STATIC > > @@ -398,7 +303,6 @@ FillTranslationTable ( > > ) > > { > > return UpdateRegionMapping ( > > - RootTable, > > MemoryRegion->VirtualBase, > > MemoryRegion->Length, > > ArmMemoryAttributeToPageAttribute (MemoryRegion->Attributes) | TT_AF, > > @@ -455,8 +359,6 @@ ArmSetMemoryAttributes ( > > IN UINT64 Attributes > > ) > > { > > - EFI_STATUS Status; > > - UINT64 *TranslationTable; > > UINT64 PageAttributes; > > UINT64 PageAttributeMask; > > > > @@ -473,19 +375,11 @@ ArmSetMemoryAttributes ( > > TT_PXN_MASK | TT_XN_MASK); > > } > > > > - TranslationTable = ArmGetTTBR0BaseAddress (); > > - > > - Status = UpdateRegionMapping ( > > - TranslationTable, > > + return UpdateRegionMapping ( > > BaseAddress, > > Length, > > PageAttributes, > > PageAttributeMask); > > While making a neat and clear diff, this messes up coding style. > Yup. Will fix. > / > Leif > > > - if (EFI_ERROR (Status)) { > > - return Status; > > - } > > - > > - return EFI_SUCCESS; > > } > > > > STATIC > > @@ -497,17 +391,7 @@ SetMemoryRegionAttribute ( > > IN UINT64 BlockEntryMask > > ) > > { > > - EFI_STATUS Status; > > - UINT64 *RootTable; > > - > > - RootTable = ArmGetTTBR0BaseAddress (); > > - > > - Status = UpdateRegionMapping (RootTable, BaseAddress, Length, Attributes, BlockEntryMask); > > - if (EFI_ERROR (Status)) { > > - return Status; > > - } > > - > > - return EFI_SUCCESS; > > + return UpdateRegionMapping (BaseAddress, Length, Attributes, BlockEntryMask); > > } > > > > EFI_STATUS > > -- > > 2.17.1 > >