From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Tue, 27 Oct 2020 13:25:33 -0400 Subject: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property In-Reply-To: References: <20201006163602.21687-1-patrick.delaunay@st.com> <190d019a-7e18-b4bc-9276-e14bbe4c2855@pengutronix.de> <258ba4fa-8d1e-56be-e0de-2d6c09812c13@pengutronix.de> Message-ID: <20201027172533.GD14816@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Fri, Oct 09, 2020 at 05:00:44PM +0000, Patrick DELAUNAY wrote: > Hi Ard, > > > From: Ard Biesheuvel > > Sent: mercredi 7 octobre 2020 15:16 > > > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum wrote: > > > > > > Hello, > > > > > > On 10/7/20 1:23 PM, Ahmad Fatoum wrote: > > > > My findings[1] back then were that U-Boot did set the eXecute Never > > > > bit only on OMAP, but not for other platforms. So I could imagine > > > > this being the root cause of Patrick's issues as well: > > > > > > Rereading my own link, my memory is a little less fuzzy: eXecute Never > > > was being set, but was without effect due Manager mode being set in the > > DACR: > > > > > > > The ARM Architecture Reference Manual notes[1]: > > > > > When using the Short-descriptor translation table format, the XN > > > > > attribute is not checked for domains marked as Manager. > > > > > Therefore, the system must not include read-sensitive memory in > > > > > domains marked as Manager, because the XN bit does not prevent > > > > > speculative fetches from a Manager domain. > > > > > > > To avoid speculative access to read-sensitive memory-mapped > > > > peripherals on ARMv7, we'll need U-Boot to use client domain > > > > permissions, so the XN bit can function. > > > > > > > This issue has come up before and was fixed in de63ac278 > > > > ("ARM: mmu: Set domain permissions to client access") for OMAP2 only. > > > > It's equally applicable to all ARMv7-A platforms where caches are > > > > enabled. > > > > [1]: B3.7.2 - Execute-never restrictions on instruction fetching > > > > > > Hope this helps, > > > Ahmad > > > > > > > It most definitely does, thanks a lot. > > > > U-boot's mmu_setup() currently sets DACR to manager for all domains, so this is > > broken for all non-LPAE configurations running on v7 CPUs (except OMAP and > > perhaps others that fixed it individually). This affects all device mappings: not just > > secure DRAM for OP-TEE, but any MMIO register for any peripheral that is > > mapped into the CPU's address space. > > > > Patrick, could you please check whether this fixes the issue as well? > > > > --- a/arch/arm/lib/cache-cp15.c > > +++ b/arch/arm/lib/cache-cp15.c > > @@ -202,9 +202,9 @@ static inline void mmu_setup(void) > > asm volatile("mcr p15, 0, %0, c2, c0, 0" > > : : "r" (gd->arch.tlb_addr) : "memory"); #endif > > - /* Set the access control to all-supervisor */ > > + /* Set the access control to client (0b01) for each of the 16 > > + domains */ > > asm volatile("mcr p15, 0, %0, c3, c0, 0" > > - : : "r" (~0)); > > + : : "r" (0x55555555)); > > > > arm_init_domains(); > > The test will take some time to be sure that solve my remaining issue because issue is not always reproductible. > > At fist chek, I wasn't sure of DACR bahavior, but I found in [1] the line : > > The XN attribute is not checked for domains marked as Manager. Read-sensitive memory must > not be included in domains marked as Manager, because the XN bit does not prevent prefetches > in these cases. > > So, I need to test your patch + DCACHE_OFF instead of INVALID > (to map with XN the OP-TEE region) in my patchset. > > FYI: I found the same DACR configuration is done in: > arch/arm/cpu/armv7/ls102xa/cpu.c:199 > > [1] https://developer.arm.com/documentation/ddi0406/b/System-Level-Architecture/Virtual-Memory-System-Architecture--VMSA-/Memory-access-control/The-Execute-Never--XN--attribute-and-instruction-prefetching?lang=en > > Patrick > > For information: > > At the beginning I wasn't sure that the current DACR configuration is an issue because in found > in pseudo code of DDI0406B_arm_architecture_reference_manual_errata_markup_8_0.pdf > > B3.13.3 Address translation > if CheckDomain(tlbrecord.domain, mva, tlbrecord.sectionnotpage, iswrite) then > CheckPermission(tlbrecord.perms, mva, tlbrecord.sectionnotpage, iswrite, ispriv); > > B3.13.4 Domain checking > boolean CheckDomain(bits(4) domain, bits(32) mva, boolean sectionnotpage, boolean iswrite) > bitpos = 2*UInt(domain); > case DACR of > when ?00? DataAbort(mva, domain, sectionnotpage, iswrite, DAbort_Domain); > when ?01? permissioncheck = TRUE; > when ?10? UNPREDICTABLE; > when ?11? permissioncheck = FALSE; > return permissioncheck; > > B2.4.8 Access permission checking > // CheckPermission() > // ================= > CheckPermission(Permissions perms, bits(32) mva, > boolean sectionnotpage, bits(4) domain, boolean iswrite, boolean ispriv) > > if SCTLR.AFE == ?0? then > perms.ap<0> = ?1?; > case perms.ap of > when ?000? abort = TRUE; > when ?001? abort = !ispriv; > when ?010? abort = !ispriv && iswrite; > when ?011? abort = FALSE; > when ?100? UNPREDICTABLE; > when ?101? abort = !ispriv || iswrite; > when ?110? abort = iswrite; > when ?111? > if MemorySystemArchitecture() == MemArch_VMSA then > abort = iswrite > else > UNPREDICTABLE; > if abort then > DataAbort(mva, domain, sectionnotpage, iswrite, DAbort_Permission); > return; > > => it seens only the read/write permission is checked here (perms.ap) > => perms.xn is not used here > > access_control = DRACR[r]; > perms.ap = access_control<10:8>; > perms.xn = access_control<12>; > > with AP[2:0], bits [10:8] > Access Permissions field. Indicates the read and write access permissions for unprivileged > and privileged accesses to the memory region. > > But now it is clear with [1] So, where did everything end up here? I specifically didn't grab this series as it sounded like there was concern the problem should be solved via another patch. Or would that be an in-addition-to? Thanks! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: