Re: [PATCH edk2-platforms 1/3] Platform/ARM: drop unused EmbeddedPkg Pcds

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 8 February 2018 at 17:45, Leif Lindholm <leif.lindholm@linaro.org> 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.