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

["Ard Biesheuvel" <ardb@kernel.org>]

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.