From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by mx.groups.io with SMTP id smtpd.web10.25936.1664180731372879402 for ; Mon, 26 Sep 2022 01:25:32 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=FceOGfmV; spf=pass (domain: kernel.org, ip: 145.40.73.55, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sin.source.kernel.org (Postfix) with ESMTPS id BE6C0CE1091; Mon, 26 Sep 2022 08:25:28 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 98BCFC433C1; Mon, 26 Sep 2022 08:25:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1664180726; bh=nlS9eXs6pINOkL/yBm50zfYBAoX1P9eWiEaTphFtek4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=FceOGfmVLFRo6wMZC5NlMpcOUulGsWz70ZOUtxR7F5wMBnBSXKmqyVM4pJ9Hh4yqf 3tuIMd1MKWGQuZND0vp+cllVLVGROxrmDxmpknIKGASnRPCWic4C03RNohkOWhLv/f Bfr76uBpBkg9wTOVGn3qB5CFpPxhHJ67IYre7hPqXnrRumMzzEQDyA4wJT9iEd+CAa LSqkea6KGiUYZPRSkjjvQSG3g7lbz92WeztGvoZnWEG5Tcy37ksC0tm/vqNZDePF5o waK3vNlMmdv62YVBgTAV39NYVU93jKoc0o2Yjxu/zYwtMpmSX904EpgneMdhz+A5jp EWxzkWnEk2VlA== From: "Ard Biesheuvel" To: devel@edk2.groups.io Cc: Ard Biesheuvel , Leif Lindholm , Alexander Graf Subject: [PATCH v3 06/16] ArmPkg/ArmMmuLib: Disable and re-enable MMU only when needed Date: Mon, 26 Sep 2022 10:25:01 +0200 Message-Id: <20220926082511.2110797-7-ardb@kernel.org> X-Mailer: git-send-email 2.35.1 In-Reply-To: <20220926082511.2110797-1-ardb@kernel.org> References: <20220926082511.2110797-1-ardb@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable When updating a page table descriptor in a way that requires break before make, we temporarily disable the MMU to ensure that we don't unmap the memory region that the code itself is executing from. However, this is a condition we can check in a straight-forward manner, and if the regions are disjoint, we don't have to bother with the MMU controls, and we can just perform an ordinary break before make. Signed-off-by: Ard Biesheuvel --- ArmPkg/Include/Library/ArmMmuLib.h | 7 +- ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 102 +++++++++++= +++++---- ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S | 43 +++++++-- 3 files changed, 123 insertions(+), 29 deletions(-) diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/Ar= mMmuLib.h index 7538a8274a72..b745e2230e7e 100644 --- a/ArmPkg/Include/Library/ArmMmuLib.h +++ b/ArmPkg/Include/Library/ArmMmuLib.h @@ -52,9 +52,10 @@ ArmClearMemoryRegionReadOnly ( VOID=0D EFIAPI=0D ArmReplaceLiveTranslationEntry (=0D - IN UINT64 *Entry,=0D - IN UINT64 Value,=0D - IN UINT64 RegionStart=0D + IN UINT64 *Entry,=0D + IN UINT64 Value,=0D + IN UINT64 RegionStart,=0D + IN BOOLEAN DisableMmu=0D );=0D =0D EFI_STATUS=0D diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Libr= ary/ArmMmuLib/AArch64/ArmMmuLibCore.c index 34f1031c4de3..4d75788ed2b2 100644 --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c @@ -18,6 +18,17 @@ #include =0D #include =0D #include =0D +#include =0D +=0D +STATIC=0D +VOID (=0D + EFIAPI *mReplaceLiveEntryFunc=0D + )(=0D + IN UINT64 *Entry,=0D + IN UINT64 Value,=0D + IN UINT64 RegionStart,=0D + IN BOOLEAN DisableMmu=0D + ) =3D ArmReplaceLiveTranslationEntry;=0D =0D STATIC=0D UINT64=0D @@ -83,14 +94,40 @@ ReplaceTableEntry ( IN UINT64 *Entry,=0D IN UINT64 Value,=0D IN UINT64 RegionStart,=0D + IN UINT64 BlockMask,=0D IN BOOLEAN IsLiveBlockMapping=0D )=0D {=0D - if (!ArmMmuEnabled () || !IsLiveBlockMapping) {=0D + BOOLEAN DisableMmu;=0D +=0D + //=0D + // Replacing a live block entry with a table entry (or vice versa) requi= res a=0D + // break-before-make sequence as per the architecture. This means the ma= pping=0D + // must be made invalid and cleaned from the TLBs first, and this is a b= it of=0D + // a hassle if the mapping in question covers the code that is actually = doing=0D + // the mapping and the unmapping, and so we only bother with this if act= ually=0D + // necessary.=0D + //=0D +=0D + if (!IsLiveBlockMapping || !ArmMmuEnabled ()) {=0D + // If the mapping is not a live block mapping, or the MMU is not on ye= t, we=0D + // can simply overwrite the entry.=0D *Entry =3D Value;=0D ArmUpdateTranslationTableEntry (Entry, (VOID *)(UINTN)RegionStart);=0D } else {=0D - ArmReplaceLiveTranslationEntry (Entry, Value, RegionStart);=0D + // If the mapping in question does not cover the code that updates the= =0D + // entry in memory, or the entry that we are intending to update, we c= an=0D + // use an ordinary break before make. Otherwise, we will need to=0D + // temporarily disable the MMU.=0D + DisableMmu =3D FALSE;=0D + if ((((RegionStart ^ (UINTN)ArmReplaceLiveTranslationEntry) & ~BlockMa= sk) =3D=3D 0) ||=0D + (((RegionStart ^ (UINTN)Entry) & ~BlockMask) =3D=3D 0))=0D + {=0D + DisableMmu =3D TRUE;=0D + DEBUG ((DEBUG_WARN, "%a: splitting block entry with MMU disabled\n",= __FUNCTION__));=0D + }=0D +=0D + ArmReplaceLiveTranslationEntry (Entry, Value, RegionStart, DisableMmu)= ;=0D }=0D }=0D =0D @@ -155,12 +192,13 @@ IsTableEntry ( STATIC=0D EFI_STATUS=0D UpdateRegionMappingRecursive (=0D - IN UINT64 RegionStart,=0D - IN UINT64 RegionEnd,=0D - IN UINT64 AttributeSetMask,=0D - IN UINT64 AttributeClearMask,=0D - IN UINT64 *PageTable,=0D - IN UINTN Level=0D + IN UINT64 RegionStart,=0D + IN UINT64 RegionEnd,=0D + IN UINT64 AttributeSetMask,=0D + IN UINT64 AttributeClearMask,=0D + IN UINT64 *PageTable,=0D + IN UINTN Level,=0D + IN BOOLEAN TableIsLive=0D )=0D {=0D UINTN BlockShift;=0D @@ -170,6 +208,7 @@ UpdateRegionMappingRecursive ( UINT64 EntryValue;=0D VOID *TranslationTable;=0D EFI_STATUS Status;=0D + BOOLEAN NextTableIsLive;=0D =0D ASSERT (((RegionStart | RegionEnd) & EFI_PAGE_MASK) =3D=3D 0);=0D =0D @@ -198,7 +237,14 @@ UpdateRegionMappingRecursive ( // the next level. No block mappings are allowed at all at level 0,=0D // so in that case, we have to recurse unconditionally.=0D //=0D + // One special case to take into account is any region that covers the= page=0D + // table itself: if we'd cover such a region with block mappings, we a= re=0D + // more likely to end up in the situation later where we need to disab= le=0D + // the MMU in order to update page table entries safely, so prefer pag= e=0D + // mappings in that particular case.=0D + //=0D if ((Level =3D=3D 0) || (((RegionStart | BlockEnd) & BlockMask) !=3D 0= ) ||=0D + ((Level < 3) && (((UINT64)PageTable & ~BlockMask) =3D=3D RegionSta= rt)) ||=0D IsTableEntry (*Entry, Level))=0D {=0D ASSERT (Level < 3);=0D @@ -234,7 +280,8 @@ UpdateRegionMappingRecursive ( *Entry & TT_ATTRIBUTES_MASK,=0D 0,=0D TranslationTable,=0D - Level + 1=0D + Level + 1,=0D + FALSE=0D );=0D if (EFI_ERROR (Status)) {=0D //=0D @@ -246,8 +293,11 @@ UpdateRegionMappingRecursive ( return Status;=0D }=0D }=0D +=0D + NextTableIsLive =3D FALSE;=0D } else {=0D TranslationTable =3D (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOC= K_ENTRY);=0D + NextTableIsLive =3D TableIsLive;=0D }=0D =0D //=0D @@ -259,7 +309,8 @@ UpdateRegionMappingRecursive ( AttributeSetMask,=0D AttributeClearMask,=0D TranslationTable,=0D - Level + 1=0D + Level + 1,=0D + NextTableIsLive=0D );=0D if (EFI_ERROR (Status)) {=0D if (!IsTableEntry (*Entry, Level)) {=0D @@ -282,7 +333,8 @@ UpdateRegionMappingRecursive ( Entry,=0D EntryValue,=0D RegionStart,=0D - IsBlockEntry (*Entry, Level)=0D + BlockMask,=0D + TableIsLive && IsBlockEntry (*Entry, Level)=0D );=0D }=0D } else {=0D @@ -291,7 +343,7 @@ UpdateRegionMappingRecursive ( EntryValue |=3D (Level =3D=3D 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3=0D : TT_TYPE_BLOCK_ENTRY;=0D =0D - ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);=0D + ReplaceTableEntry (Entry, EntryValue, RegionStart, BlockMask, FALSE)= ;=0D }=0D }=0D =0D @@ -301,10 +353,11 @@ UpdateRegionMappingRecursive ( STATIC=0D EFI_STATUS=0D UpdateRegionMapping (=0D - IN UINT64 RegionStart,=0D - IN UINT64 RegionLength,=0D - IN UINT64 AttributeSetMask,=0D - IN UINT64 AttributeClearMask=0D + IN UINT64 RegionStart,=0D + IN UINT64 RegionLength,=0D + IN UINT64 AttributeSetMask,=0D + IN UINT64 AttributeClearMask,=0D + IN BOOLEAN TableIsLive=0D )=0D {=0D UINTN T0SZ;=0D @@ -321,7 +374,8 @@ UpdateRegionMapping ( AttributeSetMask,=0D AttributeClearMask,=0D ArmGetTTBR0BaseAddress (),=0D - GetRootTableLevel (T0SZ)=0D + GetRootTableLevel (T0SZ),=0D + TableIsLive=0D );=0D }=0D =0D @@ -336,7 +390,8 @@ FillTranslationTable ( MemoryRegion->VirtualBase,=0D MemoryRegion->Length,=0D ArmMemoryAttributeToPageAttribute (MemoryRegion->Attributes) | = TT_AF,=0D - 0=0D + 0,=0D + FALSE=0D );=0D }=0D =0D @@ -410,7 +465,8 @@ ArmSetMemoryAttributes ( BaseAddress,=0D Length,=0D PageAttributes,=0D - PageAttributeMask=0D + PageAttributeMask,=0D + TRUE=0D );=0D }=0D =0D @@ -423,7 +479,13 @@ SetMemoryRegionAttribute ( IN UINT64 BlockEntryMask=0D )=0D {=0D - return UpdateRegionMapping (BaseAddress, Length, Attributes, BlockEntryM= ask);=0D + return UpdateRegionMapping (=0D + BaseAddress,=0D + Length,=0D + Attributes,=0D + BlockEntryMask,=0D + TRUE=0D + );=0D }=0D =0D EFI_STATUS=0D diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S b/Arm= Pkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S index 66ebca571e63..e936a5be4e11 100644 --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S @@ -12,6 +12,14 @@ =0D .macro __replace_entry, el=0D =0D + // check whether we should disable the MMU=0D + cbz x3, .L1_\@=0D +=0D + // clean and invalidate first so that we don't clobber=0D + // adjacent entries that are dirty in the caches=0D + dc civac, x0=0D + dsb nsh=0D +=0D // disable the MMU=0D mrs x8, sctlr_el\el=0D bic x9, x8, #CTRL_M_BIT=0D @@ -38,8 +46,33 @@ // re-enable the MMU=0D msr sctlr_el\el, x8=0D isb=0D + b .L2_\@=0D +=0D +.L1_\@:=0D + // write invalid entry=0D + str xzr, [x0]=0D + dsb nshst=0D +=0D + // flush translations for the target address from the TLBs=0D + lsr x2, x2, #12=0D + .if \el =3D=3D 1=0D + tlbi vaae1, x2=0D + .else=0D + tlbi vae\el, x2=0D + .endif=0D + dsb nsh=0D +=0D + // write updated entry=0D + str x1, [x0]=0D + dsb nshst=0D +=0D +.L2_\@:=0D .endm=0D =0D + // Align this routine to a log2 upper bound of its size, so that it is=0D + // guaranteed not to cross a page or block boundary.=0D + .balign 0x200=0D +=0D //VOID=0D //ArmReplaceLiveTranslationEntry (=0D // IN UINT64 *Entry,=0D @@ -53,12 +86,7 @@ ASM_FUNC(ArmReplaceLiveTranslationEntry) msr daifset, #0xf=0D isb=0D =0D - // clean and invalidate first so that we don't clobber=0D - // adjacent entries that are dirty in the caches=0D - dc civac, x0=0D - dsb nsh=0D -=0D - EL1_OR_EL2_OR_EL3(x3)=0D + EL1_OR_EL2_OR_EL3(x5)=0D 1:__replace_entry 1=0D b 4f=0D 2:__replace_entry 2=0D @@ -72,3 +100,6 @@ ASM_GLOBAL ASM_PFX(ArmReplaceLiveTranslationEntrySize) =0D ASM_PFX(ArmReplaceLiveTranslationEntrySize):=0D .long . - ArmReplaceLiveTranslationEntry=0D +=0D + // Double check that we did not overrun the assumed maximum size=0D + .org ArmReplaceLiveTranslationEntry + 0x200=0D --=20 2.35.1