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. -=-=-=-=-=-=-=-=-=-=-=- 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
