From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.4585.1685431954837558880 for ; Tue, 30 May 2023 00:32:35 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=n7Jogg5b; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 1731D60D54 for ; Tue, 30 May 2023 07:32:34 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7E00EC433EF for ; Tue, 30 May 2023 07:32:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1685431953; bh=jcvbbksjT2nJG2qIC91B9Aw/oYHW5KeSTAyQGMe5kFs=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=n7Jogg5bVVxqqBf4BtkPwBoyxpOiZtbTUPHjqpyQmAOtg5mazopFiam5qeJh1vmVE UnN11/Gtdy6/Te+/ZLuu44Y27g7MqPaEPuWGp7r5U69omQR5V1DxXgTiIGFLAbZsFG t95Ft9qp8zzibJPi/cZ+wNKIxo/DFYBff+wgQkuC6lXJw8Q++fe4HMGgT4urHZkqKu RPXUjYjaHrSqIMQAEqOd0pjt1AA/uK24c0nHeNzakhTdrohXML+WQRB/spBlC5z0V5 5oRl9/Mcre/q/nSEL4YbNKrzrZ2AN9UcoXMRCZzjPqfJbTt891BnFYuRATmmQoXD1y ITG+qDytZEiVQ== Received: by mail-lj1-f173.google.com with SMTP id 38308e7fff4ca-2af2e1725bdso50427221fa.0 for ; Tue, 30 May 2023 00:32:33 -0700 (PDT) X-Gm-Message-State: AC+VfDxIBXkS15G0lD781m9nPLz+9VVnNK27Lg/t6hZC3T/JkW80coQs QveHj7REMRz/fCI3uNzXRBWuGZLneGO8pcQy/Xg= X-Google-Smtp-Source: ACHHUZ7sxlnjF09+HwmCDPowwpX2+q8ZwefQOmUp/Jsm+zxRI4YzL7UZJj2ICipW+0gpyPDWfT+Fiq83F4+8TYdOIdE= X-Received: by 2002:a05:651c:1713:b0:2af:1ff0:9eb3 with SMTP id be19-20020a05651c171300b002af1ff09eb3mr3687008ljb.15.1685431951554; Tue, 30 May 2023 00:32:31 -0700 (PDT) MIME-Version: 1.0 References: <20230525143041.1172989-1-ardb@kernel.org> <20230525143041.1172989-6-ardb@kernel.org> In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 30 May 2023 09:32:20 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI To: "Ni, Ray" Cc: "devel@edk2.groups.io" , "Yao, Jiewen" , Gerd Hoffmann , Taylor Beebe , Oliver Smith-Denny , "Bi, Dandan" , "Gao, Liming" , "Kinney, Michael D" , Leif Lindholm , Sunil V L , "Warkentin, Andrei" Content-Type: text/plain; charset="UTF-8" On Tue, 30 May 2023 at 09:15, Ni, Ray 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 > > Sent: Thursday, May 25, 2023 10:31 PM > > To: devel@edk2.groups.io > > Cc: Ard Biesheuvel ; Ni, Ray ; Yao, Jiewen > > ; Gerd Hoffmann ; Taylor Beebe > > ; Oliver Smith-Denny ; Bi, Dandan > > ; Gao, Liming ; Kinney, > > Michael D ; Leif Lindholm > > ; Sunil V L ; Warkentin, > > Andrei > > 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 > > --- > > 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.
> > > > + > > > > +SPDX-License-Identifier: BSD-2-Clause-Patent > > > > + > > > > +**/ > > > > + > > > > +#ifndef EDKII_MEMORY_ATTRIBUTE_PPI_H_ > > > > +#define EDKII_MEMORY_ATTRIBUTE_PPI_H_ > > > > + > > > > +#include > > > > + > > > > +/// > > > > +/// 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 >