On Thu, Mar 16, 2023 at 10:27:48 +0100, Ard Biesheuvel wrote:
> On Thu, 16 Mar 2023 at 08:19, Ard Biesheuvel <a...@kernel.org> wrote:
> >
> > On Wed, 15 Mar 2023 at 19:31, Leif Lindholm <quic_llind...@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 <a...@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_llind...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
