On Tue, 30 May 2023 at 09:15, Ni, Ray <ray...@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 <a...@kernel.org>
> > Sent: Thursday, May 25, 2023 10:31 PM
> > To: devel@edk2.groups.io
> > Cc: Ard Biesheuvel <a...@kernel.org>; Ni, Ray <ray...@intel.com>; Yao,
> > Jiewen
> > <jiewen....@intel.com>; Gerd Hoffmann <kra...@redhat.com>; Taylor Beebe
> > <t...@taylorbeebe.com>; Oliver Smith-Denny <o...@smith-denny.com>; Bi,
> > Dandan
> > <dandan...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; Kinney,
> > Michael D <michael.d.kin...@intel.com>; Leif Lindholm
> > <quic_llind...@quicinc.com>; Sunil V L <suni...@ventanamicro.com>;
> > Warkentin,
> > Andrei <andrei.warken...@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 <a...@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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105422): https://edk2.groups.io/g/devel/message/105422
Mute This Topic: https://groups.io/mt/99131184/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
