From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by mx.groups.io with SMTP id smtpd.web11.13091.1585147638655254224 for ; Wed, 25 Mar 2020 07:47:19 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=yz3sorsa; spf=pass (domain: linaro.org, ip: 209.85.221.65, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wr1-f65.google.com with SMTP id b2so3410527wrj.10 for ; Wed, 25 Mar 2020 07:47:18 -0700 (PDT) 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=vg8+WuMS4YpcWYKaoBCGUOdzvpcKtvBkFy2ewEmWBTw=; b=yz3sorsa4G8poFRs6T9txRh+gtXR8sMZy4r20lnFR56CNiesrTjyrZzrvDMwWgsK73 1sLwBFj0N7Kt+96hJPTdl3d59PfEYdRteF70DKV5fklLs32bOZX/0MfQ0j7BFsVaYBwa 0lkBEpQpVxJMCfaARF4xdHvqP8hPQBxlsDc2IJf+cKqoB/jK4rgfkIWce1XfVsjt/ayI q3uAy2HMoqmMMaBb4sI4rRAe9fY0DP5m51hg1MU1zkBAuTwU2Tgeqaj5yTTraypP+1EM 85GWKNN98/46AbTaX6YkH+ClPMz5u6b3noyLQZdbQ44VYEb8QDlr72ALa6cw76BYxhID w2eg== 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=vg8+WuMS4YpcWYKaoBCGUOdzvpcKtvBkFy2ewEmWBTw=; b=lCEVG3V9qKILCAljCeW3gUPBOUVG8Q3EbcxJhNSHlqXbhOwoocbY7WkcW6ilmqfuR6 OwgzCua8+t8yWVKfYu8cIJz8IAJ+oXgqkZWe3lwAahXzcoReokyhw7j/QKZh75g/Y3EJ S8EAEfstpMxLdQdaSmNiMvO5aYy+LJIz3w2d6V76yWX1zc75BtZXalcPGbBsLZOTFqSe AniO9ZPdgWigURBfAhyXINbmV1zR05UYfWBoZsbDILolbNFe/Lj5U9R8WviJqM9VdWU4 zSyswn+rIfiIgvnHkE9yt/rXIJ+Rkk9IMGJUjxJvDWBgs5asJ9Icin2pFQpHNqiyFbdT OY/g== X-Gm-Message-State: ANhLgQ2+gPpQsCxO+j1z5/8G/Xn/pmmqdVRG4FJAZPiL6soyjpRT2Zzr /ptIy/lx08Ya0P2OnSA0jWpQgLr+hL1jLCCfUwGHHQ== X-Google-Smtp-Source: ADFU+vvIIcelBCwbzj+QDdtq2NZAMhoQw7u7Q2xnQj9D1cuMGZrSIF59Vzo2F1bY6PU2ctkzqFv9sbOigMWo48gnczI= X-Received: by 2002:adf:e487:: with SMTP id i7mr3788373wrm.151.1585147637177; Wed, 25 Mar 2020 07:47:17 -0700 (PDT) MIME-Version: 1.0 References: <20200325113846.21700-1-ard.biesheuvel@linaro.org> <20200325113846.21700-3-ard.biesheuvel@linaro.org> <20200325123855.GN22097@bivouac.eciton.net> In-Reply-To: <20200325123855.GN22097@bivouac.eciton.net> From: "Ard Biesheuvel" Date: Wed, 25 Mar 2020 15:47:06 +0100 Message-ID: Subject: Re: [PATCH v2 2/2] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry To: Leif Lindholm Cc: edk2-devel-groups-io , Laszlo Ersek , Ashish Singhal Content-Type: text/plain; charset="UTF-8" On Wed, 25 Mar 2020 at 13:38, Leif Lindholm wrote: > > 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 > Thanks, but it is still not 100% correct. I'll send a v3 shortly. > > --- > > 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 > >