From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::241; helo=mail-it0-x241.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x241.google.com (mail-it0-x241.google.com [IPv6:2607:f8b0:4001:c0b::241]) (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 E0611222DE136 for ; Thu, 8 Feb 2018 09:52:12 -0800 (PST) Received: by mail-it0-x241.google.com with SMTP id b66so7580929itd.5 for ; Thu, 08 Feb 2018 09:57:57 -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=QWqp0cQ6mireV+6VUBIESQKBVuSpazKbP4R5HDBKzu4=; b=hQZ/Kq4/ild3DGedHtbVsZz92tbbTXs2oISJsaUEu03Ha+i5uQcHHh1nnFjDXe0pHR 380w0sLJR/sy2UEcCTIxpiI433YDmBqGxq2WM9EQDKbYST5ektKF2A4Zk9g5BVgW+ptk 3b/qfvvYZ0II+eKhmSt6T7rNctBllfIulD8NY= 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=QWqp0cQ6mireV+6VUBIESQKBVuSpazKbP4R5HDBKzu4=; b=YO7u0Aeu3ukC9cL3NF2pmKzOqAcKi8Md5Nn4LLAYgXyEoqzXdarpXYFTuPodWqAKlN MJTR8DZsDEq+GS3EMSMmOJB7oilG4xCPstKyO9EtnCz/eipcnn6XSWpuD0t5l2xaAWU+ ACmY78oFbQK6uq/R++LNFJSnNJSOLn/9fak1sNeInGmuHkfMf7LG64/paT7nHXdr4SLX Z0Ntc/TrJiY5oM78UxNEm/oVrtsDmvyU68UAUmnJqs5Cl39rRA+O0s5FCSnNJNDlOURs gT/DZayi8GwM6wZZRBE6h1723GKbo4KqxGA0STiPrqb+xJmNcHxt3ywusdOMzT5HfMXh Miyg== X-Gm-Message-State: APf1xPCVhydBxEhSIL9qplxGeauQ8FzgvG6PX9VH7Fzr2HM1fmBLzRdV j2itqaLpqtf/KicrrHTf6FH0ajTsR/tsuXRLVLaUzA== X-Google-Smtp-Source: AH8x225gyBFHur62atE1sGxU7GPbI03TbYcnfEXb7SOT5dRaeMOqzOh6AFWurZc9jlgSTByCBrD3eImVNmO16dBtXcw= X-Received: by 10.36.39.215 with SMTP id g206mr22214ita.17.1518112677106; Thu, 08 Feb 2018 09:57:57 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.112.13 with HTTP; Thu, 8 Feb 2018 09:57:56 -0800 (PST) In-Reply-To: <20180208174536.fz3teftgsvk7m35h@bivouac.eciton.net> References: <20180206171225.10676-1-leif.lindholm@linaro.org> <20180208174536.fz3teftgsvk7m35h@bivouac.eciton.net> From: Ard Biesheuvel Date: Thu, 8 Feb 2018 17:57:56 +0000 Message-ID: To: Leif Lindholm Cc: Alexei Fedorov , "edk2-devel@lists.01.org" , Evan Lloyd , Sami Mujawar , Girish Pathak , Mitch Ishihara , Matteo Carlini Subject: Re: [PATCH edk2-platforms 1/3] Platform/ARM: drop unused EmbeddedPkg Pcds X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Feb 2018 17:52:13 -0000 Content-Type: text/plain; charset="UTF-8" On 8 February 2018 at 17:45, Leif Lindholm wrote: > On Thu, Feb 08, 2018 at 03:08:19PM +0000, Alexei Fedorov wrote: >> This patch causes Juno and other ARM platforms to raise an assert during MMU initialisation in ArmConfigureMmu() > > Huh... > >> (edk2\ArmPkg\Library\ArmMmuLib\AArch64\ArmMmuLibCore.c): >> >> >> ASSERT (TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK || >> TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK); >> >> >> see debug output: >> >> UEFI firmware (version built at 12:01:21 on Feb 8 2018) >> add-symbol-file n:\edk2\Build\ArmJuno\DEBUG_GCC5\AARCH64\ArmPlatformPkg\PrePi\PeiUniCore\DEBUG\ArmPlatformPrePiUniCore.dll 0xE0000800 >> ASSERT [ArmPlatformPrePiUniCore] n:\edk2\ArmPkg\Library\ArmMmuLib\AArch64\ArmMmuLibCore.c(744): TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK || TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK >> >> because removal of >> - gEmbeddedTokenSpaceGuid.PcdCacheEnable|TRUE >> from ArmVExpress.dsc.inc now makes ArmPlatformGetVirtualMemoryMap () function in >> \edk2-platforms\Platform\ARM\JunoPkg\Library\ArmJunoLib\ArmJunoMem.c >> >> use default FALSE value defined in EmbeddedPkg.dec: >> >> >> [PcdsFeatureFlag.common] >> gEmbeddedTokenSpaceGuid.PcdCacheEnable|FALSE|BOOLEAN|0x00000042 >> >> >> & set CacheAttributes to DDR_ATTRIBUTES_UNCACHED: >> >> if (FeaturePcdGet(PcdCacheEnable) == TRUE) { >> CacheAttributes = DDR_ATTRIBUTES_CACHED; >> } else { >> CacheAttributes = DDR_ATTRIBUTES_UNCACHED; >> } > > So. I spent a while scratching my head here over how i didn't spot > this when I did my final test build before generating the patches. > > Turns out, somewhere amongst my rebasing, > gEmbeddedTokenSpaceGuid.PcdCacheEnable managed to reintroduce itself > into EmbeddedPkg.dec (I explicitly deleted it). > > Since I _knew_ I'd deleted it, and all the platforms still built, I > didn't bother grepping for it through all of edk2-platforms. > > Ho hum. > > Well, the fix is trivial - just excise it from all platforms, and turn > all conditional statements into the unconditional execution of the > TRUE path. There is no code in public trees that actually does > anything different initialisation-wise depending on the value of this > Pcd. > > Scream if you disagree. > My vote would be to get rid of it entirely. These days, uncached means non-coherent, which breaks all sorts of assumptions in UEFI itself but also in Trusted Firmware.