From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x229.google.com (mail-it0-x229.google.com [IPv6:2607:f8b0:4001:c0b::229]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 7B8C7802B3 for ; Mon, 6 Mar 2017 07:11:51 -0800 (PST) Received: by mail-it0-x229.google.com with SMTP id 203so50831653ith.0 for ; Mon, 06 Mar 2017 07:11:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=iJ3PQ8lCkSc0LNRHpvcErT/jJz1FTUGvGKpIsjti/DA=; b=LPTsh6z+ZjktgIQ/PB9NZTPdxBxB6lw40lHoLcgXEEIYrWoHjaeAPeyjE/FBZqHqLs eiPt5k4HENObYdOHeW88CYA39OOw4Cavx2dwiCKAUqdjEgHufMXdUEZeepAOzBCxQLI6 PLrB+BtM1ZmMwxm9x2nNA882VJ+QfDUgK253M= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=iJ3PQ8lCkSc0LNRHpvcErT/jJz1FTUGvGKpIsjti/DA=; b=sLRnKq7pMM1HDECjIPcTCCcpA1pXGz5dDKmlJ5hWvyko26uZSxkubww7kwjQBNmvUr QRP8KVFvMoNdA3qFnhOZXw9ggB378fHI7bPMPZ2kl2C+P0DQwSuDpKH22Ch9TzpFcJqL v8G8fQc1MuB3dmWY42nfWTgNacCgG6DzzJTm9dQo7DxD3c9wBJjbSdBh2f8xwW2G1yDo dzE4O+byDMhalG4drqrF3b45+qqhqdwHEym1JF+d6pDi2ixHC6NYlha3SDtaM/FcKhRi I7r7ItHJU8EGU9XNes4JUpc4rPNvgRlEnkfVeGWD0eFBGCieCNbBysJtcriEROLFz5GU X7aQ== X-Gm-Message-State: AMke39mCEe1WKS51xxQ6x3dojXcIWHXVL2boqu9CYBUWCVAdlBPUeVCDUHC6maOcSxD01HdDP806f74YVH/xafh0 X-Received: by 10.36.23.74 with SMTP id 71mr15215536ith.37.1488813110610; Mon, 06 Mar 2017 07:11:50 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Mon, 6 Mar 2017 07:11:50 -0800 (PST) In-Reply-To: <20170306144834.GV16034@bivouac.eciton.net> References: <1488450976-16257-1-git-send-email-ard.biesheuvel@linaro.org> <1488450976-16257-4-git-send-email-ard.biesheuvel@linaro.org> <20170306144834.GV16034@bivouac.eciton.net> From: Ard Biesheuvel Date: Mon, 6 Mar 2017 16:11:50 +0100 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" , Laszlo Ersek Subject: Re: [PATCH v2 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Mar 2017 15:11:51 -0000 Content-Type: text/plain; charset=UTF-8 On 6 March 2017 at 15:48, Leif Lindholm wrote: > On Thu, Mar 02, 2017 at 10:36:15AM +0000, Ard Biesheuvel wrote: >> Enable the use of strict memory permissions on ARM by processing the >> EFI_MEMORY_RO and EFI_MEMORY_XP rather than ignoring them. As before, >> calls to CpuArchProtocol::SetMemoryAttributes that only set RO/XP >> bits will preserve the cacheability attributes. Permissions attributes >> are not preserved when setting the memory type only: the way the memory >> permission attributes are defined does not allows for that, and so this >> situation does not deviate from other architectures. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel >> --- >> ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 151 ++++++++------------ >> 1 file changed, 62 insertions(+), 89 deletions(-) >> >> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c >> index 26b637e7658f..6dd749dadf8b 100644 >> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c >> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c >> @@ -374,50 +374,41 @@ UpdatePageEntries ( >> >> // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone) >> // EntryValue: values at bit positions specified by EntryMask >> - EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK; >> - EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE; >> + EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK; >> + if ((Attributes & EFI_MEMORY_XP) != 0) { >> + EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN; >> + } else { >> + EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE; >> + } >> + >> // Although the PI spec is unclear on this the GCD guarantees that only >> // one Attribute bit is set at a time, so we can safely use a switch statement > > But the switch statement is going away. > > The change effectively introduces a guaranteed priority of > interpretation if the spec is violated. Say something about this order > being arbitrarily decided due to PI spce guarantee instead? > Indeed. >> - switch (Attributes) { >> - case EFI_MEMORY_UC: >> - // modify cacheability attributes >> - EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK; >> - // map to strongly ordered >> - EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0 >> - break; >> - >> - case EFI_MEMORY_WC: >> - // modify cacheability attributes >> - EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK; >> - // map to normal non-cachable >> - EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0 >> - break; >> - >> - case EFI_MEMORY_WT: >> - // modify cacheability attributes >> - EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK; >> - // write through with no-allocate >> - EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0 >> - break; >> - >> - case EFI_MEMORY_WB: >> - // modify cacheability attributes >> - EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK; >> - // write back (with allocate) >> - EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1 >> - break; >> - >> - case EFI_MEMORY_WP: >> - case EFI_MEMORY_XP: >> - case EFI_MEMORY_UCE: >> - // cannot be implemented UEFI definition unclear for ARM >> - // Cause a page fault if these ranges are accessed. >> - EntryValue = TT_DESCRIPTOR_PAGE_TYPE_FAULT; >> - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting page %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes)); >> - break; >> + if ((Attributes & EFI_MEMORY_UC) != 0) { > > Or should these be "Attributes & Mask" == "ATTRIBUTE"? > I know this is more idiomatic for EDK2, but the mask *is* the attribute in this case, and so the former implies the latter. If the mask were (EFI_MEMORY_WB | EFI_MEMORY_WT | etc it would make more sense do to the latter I think >> + // modify cacheability attributes >> + EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK; >> + // map to strongly ordered >> + EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0 >> + } else if ((Attributes & EFI_MEMORY_WC) != 0) { >> + // modify cacheability attributes >> + EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK; >> + // map to normal non-cachable >> + EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0 >> + } else if ((Attributes & EFI_MEMORY_WT) != 0) { >> + // modify cacheability attributes >> + EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK; >> + // write through with no-allocate >> + EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0 >> + } else if ((Attributes & EFI_MEMORY_WB) != 0) { >> + // modify cacheability attributes >> + EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK; >> + // write back (with allocate) >> + EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1 >> + } >> >> - default: >> - return EFI_UNSUPPORTED; > > Do we not want a fallback handling for the if-form as well? > No, and that is actually the point of this patch. SetMemoryAttributes may be called without a memory type argument, in which case it only modifies permission attributes, and leaves the memory type attributes only. >> + if ((Attributes & EFI_MEMORY_RO) != 0) { >> + EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO; >> + } else { >> + EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW; >> } >> >> // Obtain page table base >> @@ -520,53 +511,42 @@ UpdateSectionEntries ( >> // EntryValue: values at bit positions specified by EntryMask >> >> // Make sure we handle a section range that is unmapped >> - EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK; >> + EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK | >> + TT_DESCRIPTOR_SECTION_AP_MASK; >> EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION; >> >> // Although the PI spec is unclear on this the GCD guarantees that only >> // one Attribute bit is set at a time, so we can safely use a switch statement > > Repeat of above. > >> - switch(Attributes) { >> - case EFI_MEMORY_UC: >> - // modify cacheability attributes >> - EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK; >> - // map to strongly ordered >> - EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0 >> - break; >> - >> - case EFI_MEMORY_WC: >> - // modify cacheability attributes >> - EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK; >> - // map to normal non-cachable >> - EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0 >> - break; >> - >> - case EFI_MEMORY_WT: >> - // modify cacheability attributes >> - EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK; >> - // write through with no-allocate >> - EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0 >> - break; >> - >> - case EFI_MEMORY_WB: >> - // modify cacheability attributes >> - EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK; >> - // write back (with allocate) >> - EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1 >> - break; >> - >> - case EFI_MEMORY_WP: >> - case EFI_MEMORY_XP: >> - case EFI_MEMORY_RP: >> - case EFI_MEMORY_UCE: >> - // cannot be implemented UEFI definition unclear for ARM >> - // Cause a page fault if these ranges are accessed. >> - EntryValue = TT_DESCRIPTOR_SECTION_TYPE_FAULT; >> - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting section %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes)); >> - break; >> + if ((Attributes & EFI_MEMORY_UC) != 0) { >> + // modify cacheability attributes >> + EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK; >> + // map to strongly ordered >> + EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0 >> + } else if ((Attributes & EFI_MEMORY_WC) != 0) { >> + // modify cacheability attributes >> + EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK; >> + // map to normal non-cachable >> + EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0 >> + } else if ((Attributes & EFI_MEMORY_WT) != 0) { >> + // modify cacheability attributes >> + EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK; >> + // write through with no-allocate >> + EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0 >> + } else if ((Attributes & EFI_MEMORY_WB) != 0) { >> + // modify cacheability attributes >> + EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK; >> + // write back (with allocate) >> + EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1 >> + } > > (Again, same questions as above.) > >> >> + if ((Attributes & EFI_MEMORY_RO) != 0) { >> + EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO; >> + } else { >> + EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW; >> + } >> >> - default: >> - return EFI_UNSUPPORTED; >> + if ((Attributes & EFI_MEMORY_XP) != 0) { >> + EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK; >> } >> >> // obtain page table base >> @@ -693,13 +673,6 @@ SetMemoryAttributes ( >> UINT64 ChunkLength; >> BOOLEAN FlushTlbs; >> >> - // >> - // Ignore invocations that only modify permission bits >> - // >> - if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) { >> - return EFI_SUCCESS; >> - } >> - >> FlushTlbs = FALSE; >> while (Length > 0) { >> if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) && >> -- >> 2.7.4 >>