Re: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI

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

On Tue, 30 May 2023 at 09:15, Ni, Ray <ray.ni@intel.com> wrote:
>
> 1.
> The PPI interface supports to set and clear any attributes with single invocation.
> That's much better than the existing UEFI protocol prototype which requires caller to call the interfaces
> twice to set and clear some attributes.
>

Agree, I think that was a mistake to define the UEFI memory attributes
protocol like that, as it requires two traversals of the page tables
for the most common case of converting RO -> XP or vice versa.

> So far I see two patterns for attributes setting:
> *. The patten in this patch: SetMask/ClearMask
> *. The pattern I used in PageTableLib: Attribute/Mask.
>
> I think from caller side, they provide the same capabilities.
> The difference is SetMask/ClearMask expects callers to not set the same BIT in both
> SetMask/ClearMask.
>
> (I thought there would be similar existing interfaces as pattern 2. But I didn't find any now.)
> Do you mind to align to pattern #2?
>

That is fine - I actually prefer that (and this is what ArmMmuLib
implements internally) but I did not want to deviate from the UEFI
protocol too much.

>
> 2.
> When a memory region is marked from not-present to present, PageTableLib expects
> caller to supply all memory attributes (including RW, NX, etc.) as the lib implementation doesn't
> want to carry any default attributes..
> Do you think the MemoryAttribute PPI should expect the same to caller?
>

I'm not sure I follow.

The PPI (as well as the UEFI protocol) can only operate on valid
mapping, and can only be used to manipulate RP/RO/XP. It cannot be
used to create mappings from scratch.

Do you think this capability should be added? If so, I think it is
reasonable to require the caller to provide all attributes, and on ARM
this would have to include the memory cacheability type as we should
not provide a default for that either.

Thanks,
Ard.


>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Thursday, May 25, 2023 10:31 PM
> > To: devel@edk2.groups.io
> > Cc: Ard Biesheuvel <ardb@kernel.org>; Ni, Ray <ray.ni@intel.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Taylor Beebe
> > <t@taylorbeebe.com>; Oliver Smith-Denny <osd@smith-denny.com>; Bi, Dandan
> > <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney,
> > Michael D <michael.d.kinney@intel.com>; Leif Lindholm
> > <quic_llindhol@quicinc.com>; Sunil V L <sunilvl@ventanamicro.com>; Warkentin,
> > Andrei <andrei.warkentin@intel.com>
> > Subject: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
> >
> > Define a PPI interface that may be used by the PEI core or other PEIMs
> > to manage permissions on memory ranges. This is primarily intended for
> > restricting permissions to what is actually needed for correct execution
> > by the code in question, and for limiting the use of memory mappings
> > that are both writable and executable at the same time.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdeModulePkg/Include/Ppi/MemoryAttribute.h | 78 ++++++++++++++++++++
> >  MdeModulePkg/MdeModulePkg.dec              |  3 +
> >  2 files changed, 81 insertions(+)
> >
> > diff --git a/MdeModulePkg/Include/Ppi/MemoryAttribute.h
> > b/MdeModulePkg/Include/Ppi/MemoryAttribute.h
> > new file mode 100644
> > index 0000000000000000..5ff31185ab4183f8
> > --- /dev/null
> > +++ b/MdeModulePkg/Include/Ppi/MemoryAttribute.h
> > @@ -0,0 +1,78 @@
> > +/** @file
> >
> > +
> >
> > +Copyright (c) 2023, Google LLC. All rights reserved.<BR>
> >
> > +
> >
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > +
> >
> > +**/
> >
> > +
> >
> > +#ifndef EDKII_MEMORY_ATTRIBUTE_PPI_H_
> >
> > +#define EDKII_MEMORY_ATTRIBUTE_PPI_H_
> >
> > +
> >
> > +#include <Uefi/UefiSpec.h>
> >
> > +
> >
> > +///
> >
> > +/// Global ID for the EDKII_MEMORY_ATTRIBUTE_PPI.
> >
> > +///
> >
> > +#define EDKII_MEMORY_ATTRIBUTE_PPI_GUID \
> >
> > +  { \
> >
> > +    0x1be840de, 0x2d92, 0x41ec, { 0xb6, 0xd3, 0x19, 0x64, 0x13, 0x50, 0x51,
> > 0xfb } \
> >
> > +  }
> >
> > +
> >
> > +///
> >
> > +/// Forward declaration for the EDKII_MEMORY_ATTRIBUTE_PPI.
> >
> > +///
> >
> > +typedef struct _EDKII_MEMORY_ATTRIBUTE_PPI
> > EDKII_MEMORY_ATTRIBUTE_PPI;
> >
> > +
> >
> > +/**
> >
> > +  Set the requested memory permission attributes on a region of memory.
> >
> > +
> >
> > +  BaseAddress and Length must be aligned to EFI_PAGE_SIZE.
> >
> > +
> >
> > +  Both SetMask and ClearMask may contain any combination of
> > EFI_MEMORY_RP,
> >
> > +  EFI_MEMORY_RO and EFI_MEMORY_XP, with the following restrictions:
> >
> > +  - each constant may appear in either SetMask or ClearMask, but not in both;
> >
> > +  - SetMask or ClearMask may be 0x0, but not both.
> >
> > +
> >
> > +  @param[in]  This            The protocol instance pointer.
> >
> > +  @param[in]  BaseAddress     The physical address that is the start address of
> >
> > +                              a memory region.
> >
> > +  @param[in]  Length          The size in bytes of the memory region.
> >
> > +  @param[in]  SetMask         Mask of memory attributes to set.
> >
> > +  @param[in]  ClearMask       Mask of memory attributes to clear.
> >
> > +
> >
> > +  @retval EFI_SUCCESS           The attributes were set for the memory region.
> >
> > +  @retval EFI_INVALID_PARAMETER Length is zero.
> >
> > +                                Invalid combination of SetMask and ClearMask.
> >
> > +                                BaseAddress or Length is not suitably aligned.
> >
> > +  @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.
> >
> > +
> >
> > +**/
> >
> > +typedef
> >
> > +EFI_STATUS
> >
> > +(EFIAPI *EDKII_MEMORY_ATTRIBUTE_SET_PERMISSIONS)(
> >
> > +  IN  EDKII_MEMORY_ATTRIBUTE_PPI  *This,
> >
> > +  IN  EFI_PHYSICAL_ADDRESS        BaseAddress,
> >
> > +  IN  UINT64                      Length,
> >
> > +  IN  UINT64                      SetMask,
> >
> > +  IN  UINT64                      ClearMask
> >
> > +  );
> >
> > +
> >
> > +///
> >
> > +/// This PPI contains a set of services to manage memory permission attributes.
> >
> > +///
> >
> > +struct _EDKII_MEMORY_ATTRIBUTE_PPI {
> >
> > +  EDKII_MEMORY_ATTRIBUTE_SET_PERMISSIONS    SetPermissions;
> >
> > +};
> >
> > +
> >
> > +extern EFI_GUID  gEdkiiMemoryAttributePpiGuid;
> >
> > +
> >
> > +#endif
> >
> > +
> >
> > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec
> > index 95dd077e19b3a901..d65dae18aa81e569 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -528,6 +528,9 @@ [Ppis]
> >    gEdkiiPeiCapsuleOnDiskPpiGuid             = { 0x71a9ea61, 0x5a35, 0x4a5d, { 0xac,
> > 0xef, 0x9c, 0xf8, 0x6d, 0x6d, 0x67, 0xe0 } }
> >
> >    gEdkiiPeiBootInCapsuleOnDiskModePpiGuid   = { 0xb08a11e4, 0xe2b7, 0x4b75,
> > { 0xb5, 0x15, 0xaf, 0x61, 0x6, 0x68, 0xbf, 0xd1  } }
> >
> >
> >
> > +  ## Include/Ppi/MemoryAttribute.h
> >
> > +  gEdkiiMemoryAttributePpiGuid              = { 0x1be840de, 0x2d92, 0x41ec, { 0xb6,
> > 0xd3, 0x19, 0x64, 0x13, 0x50, 0x51, 0xfb } }
> >
> > +
> >
> >  [Protocols]
> >
> >    ## Load File protocol provides capability to load and unload EFI image into
> > memory and execute it.
> >
> >    #  Include/Protocol/LoadPe32Image.h
> >
> > --
> > 2.39.2
>