From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by mx.groups.io with SMTP id smtpd.web10.10186.1585139938627116074 for ; Wed, 25 Mar 2020 05:38:59 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=sAvuITuz; spf=pass (domain: nuviainc.com, ip: 209.85.128.66, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f66.google.com with SMTP id d1so2466609wmb.2 for ; Wed, 25 Mar 2020 05:38:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=OBXtAP9Qo/x7/RdElV8/qPoh9zCXEzD/APOzCRROMqQ=; b=sAvuITuzPWKyyorjGpuxEn6ENoV2BrNjJ9P0t8TnEGCcmhNvqITJKB+MO+rq6RiBVI xb/zAmXH4/Yu/D3ilU0/cjswdSl2g5RTyP7LhDmVl89NPp/0SuI0x3SShBwkN5UxCJT9 efDvwA8JLE+TEkYhUgpwaapDHim8OwNKvEEUv8qNBiAWxt0BNDAa+YZX8mBOSp0SsGlX FTIzUgzmyNGpBmyGM9xiOVp06BM4cEhW3eDM/IHHcvph5DkdmxE5f+YebX4MhUtEyMAZ yQlRFWDLjWQkyWQGd/oJSpKeJRV/z3oCNH8uYyykTJdE+h1sS5U27B5aZia1pfY0j0Im dE7g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=OBXtAP9Qo/x7/RdElV8/qPoh9zCXEzD/APOzCRROMqQ=; b=Zf8u0Dkl8vOIe++COmpgzVQE8KNr3XkXtbbwf8UlMrLdR6xWvxDkUmC5lUOpmNDUDF b3Mxf35ozPVo/tZs6kIiQzrxPiar830h1hNcCUGT/upbPkfhbjjEkJGevVtC3s0fdpJE y4+lYSfsE8fRD+FA36YKkQSzlGyrIH6olMXhFVkXdeEviXdFFEO/9NNj1guMcODXf2gE vQZE+u//EkfMm5gDdS2WulgEA2LID7LJBvUH4Se25xTAkxLYGIXDY5LAyTi7v/2B8jWw HnbnhjpsvhxueeT6oyXhyC5gNCIX0ZvzsYURWWUicPsZIcMfa0XhoPlTtFFqkO3KXc38 Gf5w== X-Gm-Message-State: ANhLgQ3Y7Uf1e/WB2iQ9zhlVhZbUYU8gCkwOSPCKlGJdbdL3yUaUj/5z +MBcD/ePNm4cr2/Rs69k8V/Okg== X-Google-Smtp-Source: ADFU+vui9LGyA7bpgo0WLmIAzzQV9MPlxuAr1VSExePQYuD8oWZoS3XlR2HaXFVjFnLJ+cm8PJXxjQ== X-Received: by 2002:a7b:cd89:: with SMTP id y9mr2092440wmj.142.1585139937151; Wed, 25 Mar 2020 05:38:57 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id b80sm9416256wme.24.2020.03.25.05.38.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 25 Mar 2020 05:38:56 -0700 (PDT) Date: Wed, 25 Mar 2020 12:38:55 +0000 From: "Leif Lindholm" To: Ard Biesheuvel Cc: devel@edk2.groups.io, Laszlo Ersek , Ashish Singhal Subject: Re: [PATCH v2 2/2] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry Message-ID: <20200325123855.GN22097@bivouac.eciton.net> References: <20200325113846.21700-1-ard.biesheuvel@linaro.org> <20200325113846.21700-3-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <20200325113846.21700-3-ard.biesheuvel@linaro.org> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Mar 25, 2020 at 12:38:46 +0100, Ard Biesheuvel wrote: > Currently, depending on the size of the region being (re)mapped, the > page table manipulation code may replace a table entry with a block entry, > even if the existing table entry uses different mapping attributes to > describe different parts of the region it covers. This is undesirable, and > instead, we should avoid doing so unless we are disregarding the original > attributes anyway. And if we make such a replacement, we should free all > the page tables that have become orphaned in the process. > > So let's implement this, by taking the table entry path through the code > for block sized regions if a table entry already exists, and the clear > mask is set (which means we are preserving attributes from the existing > mapping). And when we do replace a table entry with a block entry, free > all the pages that are no longer referenced. > > Signed-off-by: Ard Biesheuvel With Laszlo's Tested-by: Reviewed-by: Leif Lindholm > --- > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 25 ++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > index 6f6ef5b05fbc..488156e69057 100644 > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > @@ -223,8 +223,12 @@ UpdateRegionMappingRecursive ( > // than a block, and recurse to create the block or page entries at > // the next level. No block mappings are allowed at all at level 0, > // so in that case, we have to recurse unconditionally. > + // If we are changing a table entry and the AttributeClearMask is non-zero, > + // we cannot replace it with a block entry without potentially losing > + // attribute information, so keep the table entry in that case. > // > - if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0) { > + if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0 || > + (IsTableEntry (*Entry, Level) && AttributeClearMask != 0)) { > ASSERT (Level < 3); > > if (!IsTableEntry (*Entry, Level)) { > @@ -245,6 +249,8 @@ UpdateRegionMappingRecursive ( > InvalidateDataCacheRange (TranslationTable, EFI_PAGE_SIZE); > } > > + ZeroMem (TranslationTable, EFI_PAGE_SIZE); > + > if (IsBlockEntry (*Entry, Level)) { > // > // We are splitting an existing block entry, so we have to populate > @@ -262,8 +268,6 @@ UpdateRegionMappingRecursive ( > FreePages (TranslationTable, 1); > return Status; > } > - } else { > - ZeroMem (TranslationTable, EFI_PAGE_SIZE); > } > } else { > TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY); > @@ -300,7 +304,20 @@ UpdateRegionMappingRecursive ( > EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3 > : TT_TYPE_BLOCK_ENTRY; > > - ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE); > + if (IsTableEntry (*Entry, Level)) { > + // > + // We are replacing a table entry with a block entry. This is only > + // possible if we are keeping none of the original attributes. > + // We can free the table entry's page table, and all the ones below > + // it, since we are dropping the only possible reference to it. > + // > + ASSERT (AttributeClearMask == 0); > + TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY); > + ReplaceTableEntry (Entry, EntryValue, RegionStart, TRUE); > + FreePageTablesRecursive (TranslationTable); > + } else { > + ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE); > + } > } > } > return EFI_SUCCESS; > -- > 2.17.1 >