[edk2-devel] [PATCH v5 11/38] ArmPkg/CpuDxe: Implement EFI memory attributes protocol

Ard Biesheuvel posted 38 patches 1 year, 3 months ago
[edk2-devel] [PATCH v5 11/38] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
Posted by Ard Biesheuvel 1 year, 3 months ago
Expose the protocol introduced in v2.10 that permits the caller to
manage mapping permissions in the page tables.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Drivers/CpuDxe/CpuDxe.c          |   2 +
 ArmPkg/Drivers/CpuDxe/CpuDxe.h          |   3 +
 ArmPkg/Drivers/CpuDxe/CpuDxe.inf        |   2 +
 ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 271 ++++++++++++++++++++
 4 files changed, 278 insertions(+)

diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
index e6742f0a25fc..d04958e79e52 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
@@ -244,6 +244,8 @@ CpuDxeInitialize (
                   &mCpuHandle,
                   &gEfiCpuArchProtocolGuid,
                   &mCpu,
+                  &gEfiMemoryAttributeProtocolGuid,
+                  &mMemoryAttribute,
                   NULL
                   );
 
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index 8cb105dcc841..ce2981361aca 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -30,9 +30,12 @@
 #include <Protocol/Cpu.h>
 #include <Protocol/DebugSupport.h>
 #include <Protocol/LoadedImage.h>
+#include <Protocol/MemoryAttribute.h>
 
 extern BOOLEAN  mIsFlushingGCD;
 
+extern EFI_MEMORY_ATTRIBUTE_PROTOCOL  mMemoryAttribute;
+
 /**
   This function registers and enables the handler specified by InterruptHandler for a processor
   interrupt or exception type specified by InterruptType. If InterruptHandler is NULL, then the
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
index 10792b393fc8..e732e21cb94a 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
@@ -23,6 +23,7 @@ [Sources.Common]
   CpuDxe.h
   CpuMmuCommon.c
   Exception.c
+  MemoryAttribute.c
 
 [Sources.ARM]
   Arm/Mmu.c
@@ -53,6 +54,7 @@ [LibraryClasses]
 
 [Protocols]
   gEfiCpuArchProtocolGuid
+  gEfiMemoryAttributeProtocolGuid
 
 [Guids]
   gEfiDebugImageInfoTableGuid
diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
new file mode 100644
index 000000000000..b47464c0269e
--- /dev/null
+++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
@@ -0,0 +1,271 @@
+/** @file
+
+  Copyright (c) 2023, Google LLC. All rights reserved.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "CpuDxe.h"
+
+/**
+  This function retrieves the attributes of the memory region specified by
+  BaseAddress and Length. If different attributes are got from different part
+  of the memory region, EFI_NO_MAPPING will be returned.
+
+  @param  This              The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+  @param  BaseAddress       The physical address that is the start address of
+                            a memory region.
+  @param  Length            The size in bytes of the memory region.
+  @param  Attributes        Pointer to attributes returned.
+
+  @retval EFI_SUCCESS           The attributes got for the memory region.
+  @retval EFI_INVALID_PARAMETER Length is zero.
+                                Attributes is NULL.
+  @retval EFI_NO_MAPPING        Attributes are not consistent cross the memory
+                                region.
+  @retval EFI_UNSUPPORTED       The processor does not support one or more
+                                bytes of the memory resource range specified
+                                by BaseAddress and Length.
+
+**/
+STATIC
+EFI_STATUS
+GetMemoryAttributes (
+  IN  EFI_MEMORY_ATTRIBUTE_PROTOCOL  *This,
+  IN  EFI_PHYSICAL_ADDRESS           BaseAddress,
+  IN  UINT64                         Length,
+  OUT UINT64                         *Attributes
+  )
+{
+  UINTN       RegionAddress;
+  UINTN       RegionLength;
+  UINTN       RegionAttributes;
+  UINTN       Union;
+  UINTN       Intersection;
+  EFI_STATUS  Status;
+
+  if ((Length == 0) || (Attributes == NULL)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  DEBUG ((
+    DEBUG_VERBOSE,
+    "%a: BaseAddress == 0x%lx, Length == 0x%lx\n",
+    __FUNCTION__,
+    BaseAddress,
+    Length
+    ));
+
+  Union        = 0;
+  Intersection = MAX_UINTN;
+
+  for (RegionAddress = (UINTN)BaseAddress;
+       RegionAddress < (UINTN)(BaseAddress + Length);
+       RegionAddress += RegionLength)
+  {
+    Status = GetMemoryRegion (
+               &RegionAddress,
+               &RegionLength,
+               &RegionAttributes
+               );
+
+    DEBUG ((
+      DEBUG_VERBOSE,
+      "%a: RegionAddress == 0x%lx, RegionLength == 0x%lx, RegionAttributes == 0x%lx\n",
+      __FUNCTION__,
+      (UINT64)RegionAddress,
+      (UINT64)RegionLength,
+      (UINT64)RegionAttributes
+      ));
+
+    if (EFI_ERROR (Status)) {
+      return EFI_NO_MAPPING;
+    }
+
+    Union        |= RegionAttributes;
+    Intersection &= RegionAttributes;
+  }
+
+  DEBUG ((
+    DEBUG_VERBOSE,
+    "%a: Union == %lx, Intersection == %lx\n",
+    __FUNCTION__,
+    (UINT64)Union,
+    (UINT64)Intersection
+    ));
+
+  if (Union != Intersection) {
+    return EFI_NO_MAPPING;
+  }
+
+  *Attributes  = RegionAttributeToGcdAttribute (Union);
+  *Attributes &= EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP;
+  return EFI_SUCCESS;
+}
+
+/**
+  This function set given attributes of the memory region specified by
+  BaseAddress and Length.
+
+  The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
+
+  @param  This              The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+  @param  BaseAddress       The physical address that is the start address of
+                            a memory region.
+  @param  Length            The size in bytes of the memory region.
+  @param  Attributes        The bit mask of attributes to set for the memory
+                            region.
+
+  @retval EFI_SUCCESS           The attributes were set for the memory region.
+  @retval EFI_INVALID_PARAMETER Length is zero.
+                                Attributes specified an illegal combination of
+                                attributes that cannot be set together.
+  @retval EFI_UNSUPPORTED       The processor does not support one or more
+                                bytes of the memory resource range specified
+                                by BaseAddress and Length.
+                                The bit mask of attributes is not supported for
+                                the memory resource range specified by
+                                BaseAddress and Length.
+  @retval EFI_OUT_OF_RESOURCES  Requested attributes cannot be applied due to
+                                lack of system resources.
+  @retval EFI_ACCESS_DENIED     Attributes for the requested memory region are
+                                controlled by system firmware and cannot be
+                                updated via the protocol.
+
+**/
+STATIC
+EFI_STATUS
+SetMemoryAttributes (
+  IN  EFI_MEMORY_ATTRIBUTE_PROTOCOL  *This,
+  IN  EFI_PHYSICAL_ADDRESS           BaseAddress,
+  IN  UINT64                         Length,
+  IN  UINT64                         Attributes
+  )
+{
+  EFI_STATUS  Status;
+
+  DEBUG ((
+    DEBUG_INFO,
+    "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
+    __FUNCTION__,
+    (UINTN)BaseAddress,
+    (UINTN)Length,
+    (UINTN)Attributes
+    ));
+
+  if ((Length == 0) ||
+      ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0))
+  {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if ((Attributes & EFI_MEMORY_RP) != 0) {
+    Status = ArmSetMemoryRegionNoAccess (BaseAddress, Length);
+    if (EFI_ERROR (Status)) {
+      return EFI_UNSUPPORTED;
+    }
+  }
+
+  if ((Attributes & EFI_MEMORY_RO) != 0) {
+    Status = ArmSetMemoryRegionReadOnly (BaseAddress, Length);
+    if (EFI_ERROR (Status)) {
+      return EFI_UNSUPPORTED;
+    }
+  }
+
+  if ((Attributes & EFI_MEMORY_XP) != 0) {
+    Status = ArmSetMemoryRegionNoExec (BaseAddress, Length);
+    if (EFI_ERROR (Status)) {
+      return EFI_UNSUPPORTED;
+    }
+  }
+
+  return EFI_SUCCESS;
+}
+
+/**
+  This function clears given attributes of the memory region specified by
+  BaseAddress and Length.
+
+  The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
+
+  @param  This              The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+  @param  BaseAddress       The physical address that is the start address of
+                            a memory region.
+  @param  Length            The size in bytes of the memory region.
+  @param  Attributes        The bit mask of attributes to clear for the memory
+                            region.
+
+  @retval EFI_SUCCESS           The attributes were cleared for the memory region.
+  @retval EFI_INVALID_PARAMETER Length is zero.
+                                Attributes specified an illegal combination of
+                                attributes that cannot be cleared together.
+  @retval EFI_UNSUPPORTED       The processor does not support one or more
+                                bytes of the memory resource range specified
+                                by BaseAddress and Length.
+                                The bit mask of attributes is not supported for
+                                the memory resource range specified by
+                                BaseAddress and Length.
+  @retval EFI_OUT_OF_RESOURCES  Requested attributes cannot be applied due to
+                                lack of system resources.
+  @retval EFI_ACCESS_DENIED     Attributes for the requested memory region are
+                                controlled by system firmware and cannot be
+                                updated via the protocol.
+
+**/
+STATIC
+EFI_STATUS
+ClearMemoryAttributes (
+  IN  EFI_MEMORY_ATTRIBUTE_PROTOCOL  *This,
+  IN  EFI_PHYSICAL_ADDRESS           BaseAddress,
+  IN  UINT64                         Length,
+  IN  UINT64                         Attributes
+  )
+{
+  EFI_STATUS  Status;
+
+  DEBUG ((
+    DEBUG_INFO,
+    "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
+    __FUNCTION__,
+    (UINTN)BaseAddress,
+    (UINTN)Length,
+    (UINTN)Attributes
+    ));
+
+  if ((Length == 0) ||
+      ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0))
+  {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if ((Attributes & EFI_MEMORY_RP) != 0) {
+    Status = ArmClearMemoryRegionNoAccess (BaseAddress, Length);
+    if (EFI_ERROR (Status)) {
+      return EFI_UNSUPPORTED;
+    }
+  }
+
+  if ((Attributes & EFI_MEMORY_RO) != 0) {
+    Status = ArmClearMemoryRegionReadOnly (BaseAddress, Length);
+    if (EFI_ERROR (Status)) {
+      return EFI_UNSUPPORTED;
+    }
+  }
+
+  if ((Attributes & EFI_MEMORY_XP) != 0) {
+    Status = ArmClearMemoryRegionNoExec (BaseAddress, Length);
+    if (EFI_ERROR (Status)) {
+      return EFI_UNSUPPORTED;
+    }
+  }
+
+  return EFI_SUCCESS;
+}
+
+EFI_MEMORY_ATTRIBUTE_PROTOCOL  mMemoryAttribute = {
+  GetMemoryAttributes,
+  SetMemoryAttributes,
+  ClearMemoryAttributes
+};
-- 
2.39.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101115): https://edk2.groups.io/g/devel/message/101115
Mute This Topic: https://groups.io/mt/97586007/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 11/38] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
Posted by Leif Lindholm 1 year, 3 months ago
On Mon, Mar 13, 2023 at 18:16:47 +0100, Ard Biesheuvel wrote:
> Expose the protocol introduced in v2.10 that permits the caller to
> manage mapping permissions in the page tables.

Nitpicks and a question:

> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  ArmPkg/Drivers/CpuDxe/CpuDxe.c          |   2 +
>  ArmPkg/Drivers/CpuDxe/CpuDxe.h          |   3 +
>  ArmPkg/Drivers/CpuDxe/CpuDxe.inf        |   2 +
>  ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 271 ++++++++++++++++++++
>  4 files changed, 278 insertions(+)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> index e6742f0a25fc..d04958e79e52 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> @@ -244,6 +244,8 @@ CpuDxeInitialize (
>                    &mCpuHandle,
>                    &gEfiCpuArchProtocolGuid,
>                    &mCpu,
> +                  &gEfiMemoryAttributeProtocolGuid,
> +                  &mMemoryAttribute,
>                    NULL
>                    );
>  
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> index 8cb105dcc841..ce2981361aca 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> @@ -30,9 +30,12 @@
>  #include <Protocol/Cpu.h>
>  #include <Protocol/DebugSupport.h>
>  #include <Protocol/LoadedImage.h>
> +#include <Protocol/MemoryAttribute.h>
>  
>  extern BOOLEAN  mIsFlushingGCD;
>  
> +extern EFI_MEMORY_ATTRIBUTE_PROTOCOL  mMemoryAttribute;
> +
>  /**
>    This function registers and enables the handler specified by InterruptHandler for a processor
>    interrupt or exception type specified by InterruptType. If InterruptHandler is NULL, then the
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> index 10792b393fc8..e732e21cb94a 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> @@ -23,6 +23,7 @@ [Sources.Common]
>    CpuDxe.h
>    CpuMmuCommon.c
>    Exception.c
> +  MemoryAttribute.c
>  
>  [Sources.ARM]
>    Arm/Mmu.c
> @@ -53,6 +54,7 @@ [LibraryClasses]
>  
>  [Protocols]
>    gEfiCpuArchProtocolGuid
> +  gEfiMemoryAttributeProtocolGuid
>  
>  [Guids]
>    gEfiDebugImageInfoTableGuid
> diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> new file mode 100644
> index 000000000000..b47464c0269e
> --- /dev/null
> +++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> @@ -0,0 +1,271 @@
> +/** @file
> +
> +  Copyright (c) 2023, Google LLC. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include "CpuDxe.h"
> +
> +/**
> +  This function retrieves the attributes of the memory region specified by
> +  BaseAddress and Length. If different attributes are got from different part

"from different part" -> "for different parts"?

> +  of the memory region, EFI_NO_MAPPING will be returned.
> +
> +  @param  This              The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
> +  @param  BaseAddress       The physical address that is the start address of
> +                            a memory region.
> +  @param  Length            The size in bytes of the memory region.
> +  @param  Attributes        Pointer to attributes returned.
> +
> +  @retval EFI_SUCCESS           The attributes got for the memory region.
> +  @retval EFI_INVALID_PARAMETER Length is zero.
> +                                Attributes is NULL.
> +  @retval EFI_NO_MAPPING        Attributes are not consistent cross the memory
> +                                region.
> +  @retval EFI_UNSUPPORTED       The processor does not support one or more
> +                                bytes of the memory resource range specified
> +                                by BaseAddress and Length.

Question: this implementation never returns EFI_UNSUPPORTED.
Is this seen as a "some architectures may have some restricted ranges
not configurable by MMU" which simply does not apply for ARM* -
i.e. the operation is considered "supported" even if on a region not
backed by anything?

> +
> +**/
> +STATIC
> +EFI_STATUS
> +GetMemoryAttributes (
> +  IN  EFI_MEMORY_ATTRIBUTE_PROTOCOL  *This,
> +  IN  EFI_PHYSICAL_ADDRESS           BaseAddress,
> +  IN  UINT64                         Length,
> +  OUT UINT64                         *Attributes
> +  )
> +{
> +  UINTN       RegionAddress;
> +  UINTN       RegionLength;
> +  UINTN       RegionAttributes;
> +  UINTN       Union;
> +  UINTN       Intersection;
> +  EFI_STATUS  Status;
> +
> +  if ((Length == 0) || (Attributes == NULL)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  DEBUG ((
> +    DEBUG_VERBOSE,
> +    "%a: BaseAddress == 0x%lx, Length == 0x%lx\n",
> +    __FUNCTION__,
> +    BaseAddress,
> +    Length
> +    ));
> +
> +  Union        = 0;
> +  Intersection = MAX_UINTN;
> +
> +  for (RegionAddress = (UINTN)BaseAddress;
> +       RegionAddress < (UINTN)(BaseAddress + Length);
> +       RegionAddress += RegionLength)
> +  {
> +    Status = GetMemoryRegion (
> +               &RegionAddress,
> +               &RegionLength,
> +               &RegionAttributes
> +               );
> +
> +    DEBUG ((
> +      DEBUG_VERBOSE,
> +      "%a: RegionAddress == 0x%lx, RegionLength == 0x%lx, RegionAttributes == 0x%lx\n",
> +      __FUNCTION__,
> +      (UINT64)RegionAddress,
> +      (UINT64)RegionLength,
> +      (UINT64)RegionAttributes
> +      ));
> +
> +    if (EFI_ERROR (Status)) {
> +      return EFI_NO_MAPPING;
> +    }
> +
> +    Union        |= RegionAttributes;
> +    Intersection &= RegionAttributes;
> +  }
> +
> +  DEBUG ((
> +    DEBUG_VERBOSE,
> +    "%a: Union == %lx, Intersection == %lx\n",
> +    __FUNCTION__,
> +    (UINT64)Union,
> +    (UINT64)Intersection
> +    ));
> +
> +  if (Union != Intersection) {
> +    return EFI_NO_MAPPING;
> +  }
> +
> +  *Attributes  = RegionAttributeToGcdAttribute (Union);
> +  *Attributes &= EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP;
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  This function set given attributes of the memory region specified by

sets

/
    Leif

> +  BaseAddress and Length.
> +
> +  The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
> +
> +  @param  This              The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
> +  @param  BaseAddress       The physical address that is the start address of
> +                            a memory region.
> +  @param  Length            The size in bytes of the memory region.
> +  @param  Attributes        The bit mask of attributes to set for the memory
> +                            region.
> +
> +  @retval EFI_SUCCESS           The attributes were set for the memory region.
> +  @retval EFI_INVALID_PARAMETER Length is zero.
> +                                Attributes specified an illegal combination of
> +                                attributes that cannot be set together.
> +  @retval EFI_UNSUPPORTED       The processor does not support one or more
> +                                bytes of the memory resource range specified
> +                                by BaseAddress and Length.
> +                                The bit mask of attributes is not supported for
> +                                the memory resource range specified by
> +                                BaseAddress and Length.
> +  @retval EFI_OUT_OF_RESOURCES  Requested attributes cannot be applied due to
> +                                lack of system resources.
> +  @retval EFI_ACCESS_DENIED     Attributes for the requested memory region are
> +                                controlled by system firmware and cannot be
> +                                updated via the protocol.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +SetMemoryAttributes (
> +  IN  EFI_MEMORY_ATTRIBUTE_PROTOCOL  *This,
> +  IN  EFI_PHYSICAL_ADDRESS           BaseAddress,
> +  IN  UINT64                         Length,
> +  IN  UINT64                         Attributes
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
> +    __FUNCTION__,
> +    (UINTN)BaseAddress,
> +    (UINTN)Length,
> +    (UINTN)Attributes
> +    ));
> +
> +  if ((Length == 0) ||
> +      ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0))
> +  {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if ((Attributes & EFI_MEMORY_RP) != 0) {
> +    Status = ArmSetMemoryRegionNoAccess (BaseAddress, Length);
> +    if (EFI_ERROR (Status)) {
> +      return EFI_UNSUPPORTED;
> +    }
> +  }
> +
> +  if ((Attributes & EFI_MEMORY_RO) != 0) {
> +    Status = ArmSetMemoryRegionReadOnly (BaseAddress, Length);
> +    if (EFI_ERROR (Status)) {
> +      return EFI_UNSUPPORTED;
> +    }
> +  }
> +
> +  if ((Attributes & EFI_MEMORY_XP) != 0) {
> +    Status = ArmSetMemoryRegionNoExec (BaseAddress, Length);
> +    if (EFI_ERROR (Status)) {
> +      return EFI_UNSUPPORTED;
> +    }
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  This function clears given attributes of the memory region specified by
> +  BaseAddress and Length.
> +
> +  The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
> +
> +  @param  This              The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
> +  @param  BaseAddress       The physical address that is the start address of
> +                            a memory region.
> +  @param  Length            The size in bytes of the memory region.
> +  @param  Attributes        The bit mask of attributes to clear for the memory
> +                            region.
> +
> +  @retval EFI_SUCCESS           The attributes were cleared for the memory region.
> +  @retval EFI_INVALID_PARAMETER Length is zero.
> +                                Attributes specified an illegal combination of
> +                                attributes that cannot be cleared together.
> +  @retval EFI_UNSUPPORTED       The processor does not support one or more
> +                                bytes of the memory resource range specified
> +                                by BaseAddress and Length.
> +                                The bit mask of attributes is not supported for
> +                                the memory resource range specified by
> +                                BaseAddress and Length.
> +  @retval EFI_OUT_OF_RESOURCES  Requested attributes cannot be applied due to
> +                                lack of system resources.
> +  @retval EFI_ACCESS_DENIED     Attributes for the requested memory region are
> +                                controlled by system firmware and cannot be
> +                                updated via the protocol.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +ClearMemoryAttributes (
> +  IN  EFI_MEMORY_ATTRIBUTE_PROTOCOL  *This,
> +  IN  EFI_PHYSICAL_ADDRESS           BaseAddress,
> +  IN  UINT64                         Length,
> +  IN  UINT64                         Attributes
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
> +    __FUNCTION__,
> +    (UINTN)BaseAddress,
> +    (UINTN)Length,
> +    (UINTN)Attributes
> +    ));
> +
> +  if ((Length == 0) ||
> +      ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0))
> +  {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if ((Attributes & EFI_MEMORY_RP) != 0) {
> +    Status = ArmClearMemoryRegionNoAccess (BaseAddress, Length);
> +    if (EFI_ERROR (Status)) {
> +      return EFI_UNSUPPORTED;
> +    }
> +  }
> +
> +  if ((Attributes & EFI_MEMORY_RO) != 0) {
> +    Status = ArmClearMemoryRegionReadOnly (BaseAddress, Length);
> +    if (EFI_ERROR (Status)) {
> +      return EFI_UNSUPPORTED;
> +    }
> +  }
> +
> +  if ((Attributes & EFI_MEMORY_XP) != 0) {
> +    Status = ArmClearMemoryRegionNoExec (BaseAddress, Length);
> +    if (EFI_ERROR (Status)) {
> +      return EFI_UNSUPPORTED;
> +    }
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_MEMORY_ATTRIBUTE_PROTOCOL  mMemoryAttribute = {
> +  GetMemoryAttributes,
> +  SetMemoryAttributes,
> +  ClearMemoryAttributes
> +};
> -- 
> 2.39.2
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101242): https://edk2.groups.io/g/devel/message/101242
Mute This Topic: https://groups.io/mt/97586007/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 11/38] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
Posted by Ard Biesheuvel 1 year, 3 months ago
On Wed, 15 Mar 2023 at 19:31, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Mon, Mar 13, 2023 at 18:16:47 +0100, Ard Biesheuvel wrote:
> > Expose the protocol introduced in v2.10 that permits the caller to
> > manage mapping permissions in the page tables.
>
> Nitpicks and a question:
>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  ArmPkg/Drivers/CpuDxe/CpuDxe.c          |   2 +
> >  ArmPkg/Drivers/CpuDxe/CpuDxe.h          |   3 +
> >  ArmPkg/Drivers/CpuDxe/CpuDxe.inf        |   2 +
> >  ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 271 ++++++++++++++++++++
> >  4 files changed, 278 insertions(+)
> >
> > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> > index e6742f0a25fc..d04958e79e52 100644
> > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> > @@ -244,6 +244,8 @@ CpuDxeInitialize (
> >                    &mCpuHandle,
> >                    &gEfiCpuArchProtocolGuid,
> >                    &mCpu,
> > +                  &gEfiMemoryAttributeProtocolGuid,
> > +                  &mMemoryAttribute,
> >                    NULL
> >                    );
> >
> > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > index 8cb105dcc841..ce2981361aca 100644
> > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > @@ -30,9 +30,12 @@
> >  #include <Protocol/Cpu.h>
> >  #include <Protocol/DebugSupport.h>
> >  #include <Protocol/LoadedImage.h>
> > +#include <Protocol/MemoryAttribute.h>
> >
> >  extern BOOLEAN  mIsFlushingGCD;
> >
> > +extern EFI_MEMORY_ATTRIBUTE_PROTOCOL  mMemoryAttribute;
> > +
> >  /**
> >    This function registers and enables the handler specified by InterruptHandler for a processor
> >    interrupt or exception type specified by InterruptType. If InterruptHandler is NULL, then the
> > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> > index 10792b393fc8..e732e21cb94a 100644
> > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> > @@ -23,6 +23,7 @@ [Sources.Common]
> >    CpuDxe.h
> >    CpuMmuCommon.c
> >    Exception.c
> > +  MemoryAttribute.c
> >
> >  [Sources.ARM]
> >    Arm/Mmu.c
> > @@ -53,6 +54,7 @@ [LibraryClasses]
> >
> >  [Protocols]
> >    gEfiCpuArchProtocolGuid
> > +  gEfiMemoryAttributeProtocolGuid
> >
> >  [Guids]
> >    gEfiDebugImageInfoTableGuid
> > diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> > new file mode 100644
> > index 000000000000..b47464c0269e
> > --- /dev/null
> > +++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> > @@ -0,0 +1,271 @@
> > +/** @file
> > +
> > +  Copyright (c) 2023, Google LLC. All rights reserved.
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include "CpuDxe.h"
> > +
> > +/**
> > +  This function retrieves the attributes of the memory region specified by
> > +  BaseAddress and Length. If different attributes are got from different part
>
> "from different part" -> "for different parts"?
>

Yeah. this is copy/paste from the protocol definition, which prose I
didn't write. I'll fix up both instances.

> > +  of the memory region, EFI_NO_MAPPING will be returned.
> > +
> > +  @param  This              The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
> > +  @param  BaseAddress       The physical address that is the start address of
> > +                            a memory region.
> > +  @param  Length            The size in bytes of the memory region.
> > +  @param  Attributes        Pointer to attributes returned.
> > +
> > +  @retval EFI_SUCCESS           The attributes got for the memory region.
> > +  @retval EFI_INVALID_PARAMETER Length is zero.
> > +                                Attributes is NULL.
> > +  @retval EFI_NO_MAPPING        Attributes are not consistent cross the memory
> > +                                region.
> > +  @retval EFI_UNSUPPORTED       The processor does not support one or more
> > +                                bytes of the memory resource range specified
> > +                                by BaseAddress and Length.
>
> Question: this implementation never returns EFI_UNSUPPORTED.
> Is this seen as a "some architectures may have some restricted ranges
> not configurable by MMU" which simply does not apply for ARM* -
> i.e. the operation is considered "supported" even if on a region not
> backed by anything?
>

Yeah, good point.

The UEFI spec does not really reason about this at all in the
specification of the protocol, but I guess it would make a lot of
sense to only allow this to be used on regions that are marked as
system memory in the GCD memory map.

I'll have a stab at implementing it like that.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101262): https://edk2.groups.io/g/devel/message/101262
Mute This Topic: https://groups.io/mt/97586007/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 11/38] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
Posted by Ard Biesheuvel 1 year, 3 months ago
On Thu, 16 Mar 2023 at 08:19, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 15 Mar 2023 at 19:31, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
> >
> > On Mon, Mar 13, 2023 at 18:16:47 +0100, Ard Biesheuvel wrote:
> > > Expose the protocol introduced in v2.10 that permits the caller to
> > > manage mapping permissions in the page tables.
> >
> > Nitpicks and a question:
> >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >  ArmPkg/Drivers/CpuDxe/CpuDxe.c          |   2 +
> > >  ArmPkg/Drivers/CpuDxe/CpuDxe.h          |   3 +
> > >  ArmPkg/Drivers/CpuDxe/CpuDxe.inf        |   2 +
> > >  ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 271 ++++++++++++++++++++
> > >  4 files changed, 278 insertions(+)
> > >
> > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> > > index e6742f0a25fc..d04958e79e52 100644
> > > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> > > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> > > @@ -244,6 +244,8 @@ CpuDxeInitialize (
> > >                    &mCpuHandle,
> > >                    &gEfiCpuArchProtocolGuid,
> > >                    &mCpu,
> > > +                  &gEfiMemoryAttributeProtocolGuid,
> > > +                  &mMemoryAttribute,
> > >                    NULL
> > >                    );
> > >
> > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > > index 8cb105dcc841..ce2981361aca 100644
> > > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > > @@ -30,9 +30,12 @@
> > >  #include <Protocol/Cpu.h>
> > >  #include <Protocol/DebugSupport.h>
> > >  #include <Protocol/LoadedImage.h>
> > > +#include <Protocol/MemoryAttribute.h>
> > >
> > >  extern BOOLEAN  mIsFlushingGCD;
> > >
> > > +extern EFI_MEMORY_ATTRIBUTE_PROTOCOL  mMemoryAttribute;
> > > +
> > >  /**
> > >    This function registers and enables the handler specified by InterruptHandler for a processor
> > >    interrupt or exception type specified by InterruptType. If InterruptHandler is NULL, then the
> > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> > > index 10792b393fc8..e732e21cb94a 100644
> > > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> > > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> > > @@ -23,6 +23,7 @@ [Sources.Common]
> > >    CpuDxe.h
> > >    CpuMmuCommon.c
> > >    Exception.c
> > > +  MemoryAttribute.c
> > >
> > >  [Sources.ARM]
> > >    Arm/Mmu.c
> > > @@ -53,6 +54,7 @@ [LibraryClasses]
> > >
> > >  [Protocols]
> > >    gEfiCpuArchProtocolGuid
> > > +  gEfiMemoryAttributeProtocolGuid
> > >
> > >  [Guids]
> > >    gEfiDebugImageInfoTableGuid
> > > diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> > > new file mode 100644
> > > index 000000000000..b47464c0269e
> > > --- /dev/null
> > > +++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> > > @@ -0,0 +1,271 @@
> > > +/** @file
> > > +
> > > +  Copyright (c) 2023, Google LLC. All rights reserved.
> > > +
> > > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +
> > > +**/
> > > +
> > > +#include "CpuDxe.h"
> > > +
> > > +/**
> > > +  This function retrieves the attributes of the memory region specified by
> > > +  BaseAddress and Length. If different attributes are got from different part
> >
> > "from different part" -> "for different parts"?
> >
>
> Yeah. this is copy/paste from the protocol definition, which prose I
> didn't write. I'll fix up both instances.
>
> > > +  of the memory region, EFI_NO_MAPPING will be returned.
> > > +
> > > +  @param  This              The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
> > > +  @param  BaseAddress       The physical address that is the start address of
> > > +                            a memory region.
> > > +  @param  Length            The size in bytes of the memory region.
> > > +  @param  Attributes        Pointer to attributes returned.
> > > +
> > > +  @retval EFI_SUCCESS           The attributes got for the memory region.
> > > +  @retval EFI_INVALID_PARAMETER Length is zero.
> > > +                                Attributes is NULL.
> > > +  @retval EFI_NO_MAPPING        Attributes are not consistent cross the memory
> > > +                                region.
> > > +  @retval EFI_UNSUPPORTED       The processor does not support one or more
> > > +                                bytes of the memory resource range specified
> > > +                                by BaseAddress and Length.
> >
> > Question: this implementation never returns EFI_UNSUPPORTED.
> > Is this seen as a "some architectures may have some restricted ranges
> > not configurable by MMU" which simply does not apply for ARM* -
> > i.e. the operation is considered "supported" even if on a region not
> > backed by anything?
> >
>
> Yeah, good point.
>
> The UEFI spec does not really reason about this at all in the
> specification of the protocol, but I guess it would make a lot of
> sense to only allow this to be used on regions that are marked as
> system memory in the GCD memory map.
>
> I'll have a stab at implementing it like that.

Something like this applied on top should work:

diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
index b47464c0269e..46f0760b8e3b 100644
--- a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
+++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
@@ -8,6 +8,41 @@

 #include "CpuDxe.h"

+/**
+  Check whether the provided memory range is covered by a single entry of type
+  EfiGcdSystemMemory in the GCD memory map.
+
+  @param  BaseAddress       The physical address that is the start address of
+                            a memory region.
+  @param  Length            The size in bytes of the memory region.
+
+  @return Whether the region is system memory or not.
+**/
+STATIC
+BOOLEAN
+RegionIsSystemMemory (
+  IN  EFI_PHYSICAL_ADDRESS           BaseAddress,
+  IN  UINT64                         Length
+  )
+{
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR  GcdDescriptor;
+  EFI_PHYSICAL_ADDRESS             GcdEndAddress;
+  EFI_STATUS                       Status;
+
+  Status = gDS->GetMemorySpaceDescriptor (BaseAddress, &GcdDescriptor);
+  if (EFI_ERROR (Status) ||
+      (GcdDescriptor.GcdMemoryType != EfiGcdMemoryTypeSystemMemory)) {
+    return FALSE;
+  }
+
+  GcdEndAddress = GcdDescriptor.BaseAddress + GcdDescriptor.Length;
+
+  //
+  // Return TRUE if the GCD descriptor covers the range entirely
+  //
+  return GcdEndAddress >= (BaseAddress + Length);
+}
+
 /**
   This function retrieves the attributes of the memory region specified by
   BaseAddress and Length. If different attributes are got from different part
@@ -49,6 +84,10 @@ GetMemoryAttributes (
     return EFI_INVALID_PARAMETER;
   }

+  if (!RegionIsSystemMemory (BaseAddress, Length)) {
+    return EFI_UNSUPPORTED;
+  }
+
   DEBUG ((
     DEBUG_VERBOSE,
     "%a: BaseAddress == 0x%lx, Length == 0x%lx\n",
@@ -160,6 +199,10 @@ SetMemoryAttributes (
     return EFI_INVALID_PARAMETER;
   }

+  if (!RegionIsSystemMemory (BaseAddress, Length)) {
+    return EFI_UNSUPPORTED;
+  }
+
   if ((Attributes & EFI_MEMORY_RP) != 0) {
     Status = ArmSetMemoryRegionNoAccess (BaseAddress, Length);
     if (EFI_ERROR (Status)) {
@@ -240,6 +283,10 @@ ClearMemoryAttributes (
     return EFI_INVALID_PARAMETER;
   }

+  if (!RegionIsSystemMemory (BaseAddress, Length)) {
+    return EFI_UNSUPPORTED;
+  }
+
   if ((Attributes & EFI_MEMORY_RP) != 0) {
     Status = ArmClearMemoryRegionNoAccess (BaseAddress, Length);
     if (EFI_ERROR (Status)) {


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101265): https://edk2.groups.io/g/devel/message/101265
Mute This Topic: https://groups.io/mt/97586007/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 11/38] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
Posted by Leif Lindholm 1 year, 3 months ago
On Thu, Mar 16, 2023 at 10:27:48 +0100, Ard Biesheuvel wrote:
> On Thu, 16 Mar 2023 at 08:19, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 15 Mar 2023 at 19:31, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
> > >
> > > On Mon, Mar 13, 2023 at 18:16:47 +0100, Ard Biesheuvel wrote:
> > > > Expose the protocol introduced in v2.10 that permits the caller to
> > > > manage mapping permissions in the page tables.
> > >
> > > Nitpicks and a question:
> > >
> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > ---
> > > >  ArmPkg/Drivers/CpuDxe/CpuDxe.c          |   2 +
> > > >  ArmPkg/Drivers/CpuDxe/CpuDxe.h          |   3 +
> > > >  ArmPkg/Drivers/CpuDxe/CpuDxe.inf        |   2 +
> > > >  ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 271 ++++++++++++++++++++
> > > >  4 files changed, 278 insertions(+)
> > > >
> > > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> > > > index e6742f0a25fc..d04958e79e52 100644
> > > > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> > > > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> > > > @@ -244,6 +244,8 @@ CpuDxeInitialize (
> > > >                    &mCpuHandle,
> > > >                    &gEfiCpuArchProtocolGuid,
> > > >                    &mCpu,
> > > > +                  &gEfiMemoryAttributeProtocolGuid,
> > > > +                  &mMemoryAttribute,
> > > >                    NULL
> > > >                    );
> > > >
> > > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > > > index 8cb105dcc841..ce2981361aca 100644
> > > > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > > > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > > > @@ -30,9 +30,12 @@
> > > >  #include <Protocol/Cpu.h>
> > > >  #include <Protocol/DebugSupport.h>
> > > >  #include <Protocol/LoadedImage.h>
> > > > +#include <Protocol/MemoryAttribute.h>
> > > >
> > > >  extern BOOLEAN  mIsFlushingGCD;
> > > >
> > > > +extern EFI_MEMORY_ATTRIBUTE_PROTOCOL  mMemoryAttribute;
> > > > +
> > > >  /**
> > > >    This function registers and enables the handler specified by InterruptHandler for a processor
> > > >    interrupt or exception type specified by InterruptType. If InterruptHandler is NULL, then the
> > > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> > > > index 10792b393fc8..e732e21cb94a 100644
> > > > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> > > > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> > > > @@ -23,6 +23,7 @@ [Sources.Common]
> > > >    CpuDxe.h
> > > >    CpuMmuCommon.c
> > > >    Exception.c
> > > > +  MemoryAttribute.c
> > > >
> > > >  [Sources.ARM]
> > > >    Arm/Mmu.c
> > > > @@ -53,6 +54,7 @@ [LibraryClasses]
> > > >
> > > >  [Protocols]
> > > >    gEfiCpuArchProtocolGuid
> > > > +  gEfiMemoryAttributeProtocolGuid
> > > >
> > > >  [Guids]
> > > >    gEfiDebugImageInfoTableGuid
> > > > diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> > > > new file mode 100644
> > > > index 000000000000..b47464c0269e
> > > > --- /dev/null
> > > > +++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> > > > @@ -0,0 +1,271 @@
> > > > +/** @file
> > > > +
> > > > +  Copyright (c) 2023, Google LLC. All rights reserved.
> > > > +
> > > > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > +
> > > > +**/
> > > > +
> > > > +#include "CpuDxe.h"
> > > > +
> > > > +/**
> > > > +  This function retrieves the attributes of the memory region specified by
> > > > +  BaseAddress and Length. If different attributes are got from different part
> > >
> > > "from different part" -> "for different parts"?
> > >
> >
> > Yeah. this is copy/paste from the protocol definition, which prose I
> > didn't write. I'll fix up both instances.
> >
> > > > +  of the memory region, EFI_NO_MAPPING will be returned.
> > > > +
> > > > +  @param  This              The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
> > > > +  @param  BaseAddress       The physical address that is the start address of
> > > > +                            a memory region.
> > > > +  @param  Length            The size in bytes of the memory region.
> > > > +  @param  Attributes        Pointer to attributes returned.
> > > > +
> > > > +  @retval EFI_SUCCESS           The attributes got for the memory region.
> > > > +  @retval EFI_INVALID_PARAMETER Length is zero.
> > > > +                                Attributes is NULL.
> > > > +  @retval EFI_NO_MAPPING        Attributes are not consistent cross the memory
> > > > +                                region.
> > > > +  @retval EFI_UNSUPPORTED       The processor does not support one or more
> > > > +                                bytes of the memory resource range specified
> > > > +                                by BaseAddress and Length.
> > >
> > > Question: this implementation never returns EFI_UNSUPPORTED.
> > > Is this seen as a "some architectures may have some restricted ranges
> > > not configurable by MMU" which simply does not apply for ARM* -
> > > i.e. the operation is considered "supported" even if on a region not
> > > backed by anything?
> > >
> >
> > Yeah, good point.
> >
> > The UEFI spec does not really reason about this at all in the
> > specification of the protocol, but I guess it would make a lot of
> > sense to only allow this to be used on regions that are marked as
> > system memory in the GCD memory map.
> >
> > I'll have a stab at implementing it like that.
> 
> Something like this applied on top should work:

Perfect, thanks.
With this folded in:
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

> diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> index b47464c0269e..46f0760b8e3b 100644
> --- a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> +++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> @@ -8,6 +8,41 @@
> 
>  #include "CpuDxe.h"
> 
> +/**
> +  Check whether the provided memory range is covered by a single entry of type
> +  EfiGcdSystemMemory in the GCD memory map.
> +
> +  @param  BaseAddress       The physical address that is the start address of
> +                            a memory region.
> +  @param  Length            The size in bytes of the memory region.
> +
> +  @return Whether the region is system memory or not.
> +**/
> +STATIC
> +BOOLEAN
> +RegionIsSystemMemory (
> +  IN  EFI_PHYSICAL_ADDRESS           BaseAddress,
> +  IN  UINT64                         Length
> +  )
> +{
> +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR  GcdDescriptor;
> +  EFI_PHYSICAL_ADDRESS             GcdEndAddress;
> +  EFI_STATUS                       Status;
> +
> +  Status = gDS->GetMemorySpaceDescriptor (BaseAddress, &GcdDescriptor);
> +  if (EFI_ERROR (Status) ||
> +      (GcdDescriptor.GcdMemoryType != EfiGcdMemoryTypeSystemMemory)) {
> +    return FALSE;
> +  }
> +
> +  GcdEndAddress = GcdDescriptor.BaseAddress + GcdDescriptor.Length;
> +
> +  //
> +  // Return TRUE if the GCD descriptor covers the range entirely
> +  //
> +  return GcdEndAddress >= (BaseAddress + Length);
> +}
> +
>  /**
>    This function retrieves the attributes of the memory region specified by
>    BaseAddress and Length. If different attributes are got from different part
> @@ -49,6 +84,10 @@ GetMemoryAttributes (
>      return EFI_INVALID_PARAMETER;
>    }
> 
> +  if (!RegionIsSystemMemory (BaseAddress, Length)) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
>    DEBUG ((
>      DEBUG_VERBOSE,
>      "%a: BaseAddress == 0x%lx, Length == 0x%lx\n",
> @@ -160,6 +199,10 @@ SetMemoryAttributes (
>      return EFI_INVALID_PARAMETER;
>    }
> 
> +  if (!RegionIsSystemMemory (BaseAddress, Length)) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
>    if ((Attributes & EFI_MEMORY_RP) != 0) {
>      Status = ArmSetMemoryRegionNoAccess (BaseAddress, Length);
>      if (EFI_ERROR (Status)) {
> @@ -240,6 +283,10 @@ ClearMemoryAttributes (
>      return EFI_INVALID_PARAMETER;
>    }
> 
> +  if (!RegionIsSystemMemory (BaseAddress, Length)) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
>    if ((Attributes & EFI_MEMORY_RP) != 0) {
>      Status = ArmClearMemoryRegionNoAccess (BaseAddress, Length);
>      if (EFI_ERROR (Status)) {


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101266): https://edk2.groups.io/g/devel/message/101266
Mute This Topic: https://groups.io/mt/97586007/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-