From 909abd7104b87986ad22e92b4cb07d30a8cca11b Mon Sep 17 00:00:00 2001
From: Oliver Smith-Denny <osde@linux.microsoft.com>
Date: Mon, 29 Jul 2024 09:29:14 -0700
Subject: EmbeddedPkg: NonCoherentDmaLib: Set EFI_MEMORY_XP Capability on DMA
 Buffer

Commit 8984fba2f22a2cd44e1189403e3553f447b82852 added setting the
EFI_MEMORY_XP attribute on DMA buffers. However, it did not ensure
that the XP capability was set on that region.

This patch adds setting the XP capability before attempting to set
the attribute. If setting the capability fails, it defaults to the
old behavior of not setting the XP bit.

Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>
---
 .../Library/NonCoherentDmaLib/NonCoherentDmaLib.c  | 36 +++++++++++++++++++---
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
index 0a21d72290..403cc8e2fd 100644
--- a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
+++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
@@ -500,10 +500,12 @@ DmaAllocateAlignedBuffer (
 {
   EFI_GCD_MEMORY_SPACE_DESCRIPTOR  GcdDescriptor;
   VOID                             *Allocation;
-  UINT64                           MemType;
+  UINT64                           Attributes;
   UNCACHED_ALLOCATION              *Alloc;
   EFI_STATUS                       Status;
 
+  Attributes = EFI_MEMORY_XP;
+
   if (Alignment == 0) {
     Alignment = EFI_PAGE_SIZE;
   }
@@ -534,9 +536,9 @@ DmaAllocateAlignedBuffer (
 
   // Choose a suitable uncached memory type that is supported by the region
   if (GcdDescriptor.Capabilities & EFI_MEMORY_WC) {
-    MemType = EFI_MEMORY_WC;
+    Attributes |= EFI_MEMORY_WC;
   } else if (GcdDescriptor.Capabilities & EFI_MEMORY_UC) {
-    MemType = EFI_MEMORY_UC;
+    Attributes |= EFI_MEMORY_UC;
   } else {
     Status = EFI_UNSUPPORTED;
     goto FreeBuffer;
@@ -553,11 +555,37 @@ DmaAllocateAlignedBuffer (
 
   InsertHeadList (&UncachedAllocationList, &Alloc->Link);
 
+  // Ensure that EFI_MEMORY_XP is in the capability set
+  if ((GcdDescriptor.Capabilities & EFI_MEMORY_XP) != EFI_MEMORY_XP) {
+    Status = gDS->SetMemorySpaceCapabilities (
+                    (PHYSICAL_ADDRESS)(UINTN)Allocation,
+                    EFI_PAGES_TO_SIZE (Pages),
+                    GcdDescriptor.Capabilities | EFI_MEMORY_XP
+                    );
+
+    // if we were to fail setting the capability, this would indicate an internal failure of the GCD code. We should
+    // assert here to let a platform know something went crazy, but for a release build we can let the allocation occur
+    // without the EFI_MEMORY_XP bit set, as that was the existing behavior
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a failed to set EFI_MEMORY_XP capability on 0x%llx for length 0x%llx. Attempting to allocate without XP set.\n",
+        __func__,
+        Allocation,
+        EFI_PAGES_TO_SIZE (Pages)
+        ));
+
+      ASSERT_EFI_ERROR (Status);
+
+      Attributes &= ~EFI_MEMORY_XP;
+    }
+  }
+
   // Remap the region with the new attributes and mark it non-executable
   Status = gDS->SetMemorySpaceAttributes (
                   (PHYSICAL_ADDRESS)(UINTN)Allocation,
                   EFI_PAGES_TO_SIZE (Pages),
-                  MemType | EFI_MEMORY_XP
+                  Attributes
                   );
   if (EFI_ERROR (Status)) {
     goto FreeAlloc;
-- 
cgit v1.2.3