On Mon, Nov 16, 2015 at 05:29:42PM +0100, Ard Biesheuvel wrote:
> Even though mapping normal memory (inner) shareable is usually the
> correct choice on coherent systems, it may be desirable in some cases
> to use non-shareable mappings for normal memory, e.g., when hardware
> managed coherency is not required and the memory system is not fully
> configured yet. So introduce a PCD PcdNormalMemoryNonshareableOverride
> that makes cacheable mappings of normal memory non-shareable.
I totally aprove of the flag, but...
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> ---
> ArmPkg/ArmPkg.dec | 6 ++++++
> ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.inf | 3 +++
> ArmPkg/Library/ArmLib/ArmV7/ArmV7LibPrePi.inf | 3 +++
> ArmPkg/Library/ArmLib/ArmV7/ArmV7Mmu.c | 19 +++++++++++++++++++
> 4 files changed, 31 insertions(+)
>
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index 46e9894d3f56..ff4531e44106 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -73,6 +73,12 @@ [PcdsFeatureFlag.common]
> # Define if the GICv3 controller should use the GICv2 legacy
> gArmTokenSpaceGuid.PcdArmGicV3WithV2Legacy|FALSE|BOOLEAN|0x00000042
>
> +[PcdsFeatureFlag.ARM]
> + # Whether to map normal memory as non-shareable. FALSE is the safe choice,
> but
> + # TRUE may be appropriate to fix performance problems if you don't care
> about
> + # hardware coherency (i.e., no virtualization or cache coherent DMA)
> +
> gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride|FALSE|BOOLEAN|0x00000043
> +
> [PcdsFixedAtBuild.common]
> gArmTokenSpaceGuid.PcdTrustzoneSupport|FALSE|BOOLEAN|0x00000006
>
> diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.inf
> b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.inf
> index 01bdfb699656..d56851a1409b 100644
> --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.inf
> +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.inf
> @@ -48,3 +48,6 @@ [LibraryClasses]
>
> [Protocols]
> gEfiCpuArchProtocolGuid
> +
> +[FeaturePcd.ARM]
> + gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride
> diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7LibPrePi.inf
> b/ArmPkg/Library/ArmLib/ArmV7/ArmV7LibPrePi.inf
> index ac081068db28..6eaf350c7b9d 100644
> --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7LibPrePi.inf
> +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7LibPrePi.inf
> @@ -48,3 +48,6 @@ [LibraryClasses]
>
> [Protocols]
> gEfiCpuArchProtocolGuid
> +
> +[FeaturePcd.ARM]
> + gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride
> diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Mmu.c
> b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Mmu.c
> index e05a51e0d901..15f355012b98 100644
> --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Mmu.c
> +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Mmu.c
> @@ -62,10 +62,16 @@ PopulateLevel2PageTable (
> case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK:
> case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK:
> PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_BACK;
> + if (FeaturePcdGet(PcdNormalMemoryNonshareableOverride)) {
> + PageAttributes = (PageAttributes & ~TT_DESCRIPTOR_PAGE_S_MASK) |
> TT_DESCRIPTOR_PAGE_S_NOT_SHARED;
> + }
> break;
> case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
> case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH:
> PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_THROUGH;
> + if (FeaturePcdGet(PcdNormalMemoryNonshareableOverride)) {
> + PageAttributes = (PageAttributes & ~TT_DESCRIPTOR_PAGE_S_MASK) |
> TT_DESCRIPTOR_PAGE_S_NOT_SHARED;
> + }
> break;
> case ARM_MEMORY_REGION_ATTRIBUTE_DEVICE:
> case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_DEVICE:
> @@ -178,6 +184,19 @@ FillTranslationTable (
> break;
> }
Could this patch not be simplified by just explicitly clearing the
bit, once here?
if (FeaturePcdGet(PcdNormalMemoryNonshareableOverride)) {
PageAttributes = (PageAttributes & ~TT_DESCRIPTOR_PAGE_S_MASK) | \
TT_DESCRIPTOR_PAGE_S_NOT_SHARED;
}
>
> + if (FeaturePcdGet(PcdNormalMemoryNonshareableOverride)) {
> + switch (MemoryRegion->Attributes) {
> + case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK:
> + case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
> + case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK:
> + case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH:
> + Attributes = (Attributes & ~TT_DESCRIPTOR_SECTION_S_MASK) |
> TT_DESCRIPTOR_SECTION_S_NOT_SHARED;
> + break;
> + default:
> + ;
> + }
> + }
> +
... and once here?
if (FeaturePcdGet(PcdNormalMemoryNonshareableOverride)) {
Attributes = (Attributes & ~TT_DESCRIPTOR_SECTION_S_MASK) | \
TT_DESCRIPTOR_SECTION_S_NOT_SHARED;
}
> // Get the first section entry for this mapping
> SectionEntry =
> TRANSLATION_TABLE_ENTRY_FOR_VIRTUAL_ADDRESS(TranslationTable,
> MemoryRegion->VirtualBase);
>
> --
> 1.9.1
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
