Re: [edk2-devel] [Patch V4 07/15] UefiCpuPkg/PiSmmCpuDxeSmm: Add 2 function to disable/enable CR0.WP
Thanks, Ray. Looking forward to seeing the ideas on this feature! Regards, Kun On 5/24/2023 5:46 PM, Ni, Ray wrote: Kun, Thanks for raising that up We have some ideas. Will post them later. Looking forward to work with community together. Thanks, Ray -Original Message- From: Kun Qin Sent: Thursday, May 25, 2023 2:39 AM To: devel@edk2.groups.io; Tan, Dun Cc: Dong, Eric ; Ni, Ray ; Kumar, Rahul R ; Gerd Hoffmann Subject: Re: [edk2-devel] [Patch V4 07/15] UefiCpuPkg/PiSmmCpuDxeSmm: Add 2 function to disable/enable CR0.WP Hi Dun, Thanks for your reply. That was helpful! Just a follow-up question, is there any plan to support heap guard with PcdCpuSmmRestrictedMemoryAccess enabled after these changes? I think it would be a great value prop for the developers to have both features enabled during firmware validation process. Thanks, Kun On 5/23/2023 2:14 AM, duntan wrote: Hi Kun, I've updated my answers in your original mail. Thanks, Dun -Original Message- From: Kun Qin Sent: Saturday, May 20, 2023 10:00 AM To: devel@edk2.groups.io; Tan, Dun Cc: Dong, Eric ; Ni, Ray ; Kumar, Rahul R ; Gerd Hoffmann Subject: Re: [edk2-devel] [Patch V4 07/15] UefiCpuPkg/PiSmmCpuDxeSmm: Add 2 function to disable/enable CR0.WP Hi Dun, Thanks for the notice on the other thread (v4 04/15). I have a few more questions on this specific patch (and a few associated commits related to it): Why would we allow page table manipulation after `mIsReadOnlyPageTable` is evaluated to TRUE? Dun: `mIsReadOnlyPageTable` is a flag to indicate that current page table has been marked as RO and the new allocated pool should also be RO. We only need to clear Cr0.WP before modify page table. As far as I can tell, `mIsReadOnlyPageTable` is set to TRUE inside `SetPageTableAttributes` function, but then we also have code in `InitializePageTablePool` to expect more page tables to be allocated. Could you please let me when this would happen? Dun: After `SetPageTableAttributes`, in 'SmmCpuFeaturesCompleteSmmReadyToLock()' API of different platform SmmCpuFeaturesLib, EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL may be used to convert memory attribute. Also, in SMI handler after ReadyToLock, EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL still may be used to convert memory attribute. During this process, if page split happens, new page table pool may be allocated. I thought there would not be any new page table memory (i.e. memory attribute update) after ready to lock with restricted memory access option? With these change, it seems to be doable through EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL now, is that correct? If so, would you mind shedding some light on what other behavior changes there might be? Dun: Do you mean that we should check if ReadyToLock in EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL implementation to make sure that page table won't be modified after ReadyToLock? If is, as I mentioned above, page table still may be modified after ReadyToLock. In addition, I might miss it in the patch series. If the newly allocated page memory is marked as read only after the above flag is set to TRUE, how would the callers able to use them? Dun: Caller can clear the Cr0.WP before modifying the page table. Thanks in advance. Regards, Kun On 5/16/2023 2:59 AM, duntan wrote: Add two functions to disable/enable CR0.WP. These two unctions will also be used in later commits. This commit doesn't change any functionality. Signed-off-by: Dun Tan Cc: Eric Dong Cc: Ray Ni Cc: Rahul Kumar Cc: Gerd Hoffmann --- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 24 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 115 ++ - 2 files changed, 90 insertions(+), 49 deletions(-) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h index ba341cadc6..e0c4ca76dc 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h @@ -1565,4 +1565,28 @@ SmmWaitForApArrival ( VOID ); +/** + Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1. + + @param[out] WpEnabled If Cr0.WP is enabled. + @param[out] CetEnabled If CET is enabled. +**/ +VOID +DisableReadOnlyPageWriteProtect ( + OUT BOOLEAN *WpEnabled, + OUT BOOLEAN *CetEnabled + ); + +/** + Enable Write Protect on pages marked as read-only. + + @param[out] WpEnabled If Cr0.WP should be enabled. + @param[out] CetEnabled If CET should be enabled. +**/ +VOID +EnableReadOnlyPageWriteProtect ( + BOOLEAN WpEnabled, + BOOLEAN CetEnabled + ); + #endif diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c index 2faee8f859..4b512edf68 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c +++
Re: [edk2-devel] [PATCH v2 1/2] AMD/AmdMinBoardPkg: Implements PCI hotplug init protocol
[AMD Official Use Only - General]
Hi Abdul,
Is this a duplicate patch?
There was a patch you sent: [edk2-devel] [PATCH 1/1] AMD/AmdMinBoardPkg:
Implements PCI hotplug init protocol, they both create PciHotPlug.c and
PciHotPlug.inf.
Thanks
Abner
> -Original Message-
> From: Abdul Lateef Attar
> Sent: Monday, May 22, 2023 5:29 PM
> To: devel@edk2.groups.io
> Cc: Attar, AbdulLateef (Abdul Lateef) ; Chang,
> Abner
> Subject: [PATCH v2 1/2] AMD/AmdMinBoardPkg: Implements PCI hotplug init
> protocol
>
> From: Abdul Lateef Attar
>
> Implements PCI hotplug init protocol.
> Adds resources padding based on PCD values.
>
> Cc: Abner Chang
>
> Signed-off-by: Abdul Lateef Attar
> ---
> .../AMD/AmdMinBoardPkg/AmdMinBoardPkg.dec | 16 +
> .../AMD/AmdMinBoardPkg/AmdMinBoardPkg.dsc | 14 +-
> .../PciHotPlug/PciHotPlugInit.inf | 39 +++
> .../PciHotPlug/PciHotPlugInit.c | 331 ++
> 4 files changed, 399 insertions(+), 1 deletion(-)
> create mode 100755
> Platform/AMD/AmdMinBoardPkg/PciHotPlug/PciHotPlugInit.inf
> create mode 100755
> Platform/AMD/AmdMinBoardPkg/PciHotPlug/PciHotPlugInit.c
>
> diff --git a/Platform/AMD/AmdMinBoardPkg/AmdMinBoardPkg.dec
> b/Platform/AMD/AmdMinBoardPkg/AmdMinBoardPkg.dec
> index e37b02c4cf5a..65ba08545021 100644
> --- a/Platform/AMD/AmdMinBoardPkg/AmdMinBoardPkg.dec
> +++ b/Platform/AMD/AmdMinBoardPkg/AmdMinBoardPkg.dec
> @@ -17,3 +17,19 @@ [Defines]
>PACKAGE_GUID = 44F9D761-9ECB-43DD-A5AC-177E5048701B
>PACKAGE_VERSION= 0.1
>
> +[Guids]
> + gAmdMinBoardPkgTokenSpaceGuid = {0xd4d23d79, 0x73bf, 0x460a,
> {0xa1, 0xc7, 0x85, 0xa3, 0xca, 0x71, 0xb9, 0x4c}}
> +
> +[PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
> + #
> + # PCI HotPlug Resource Padding
> + #
> + # PCI bus padding, number of bus to reserve, default 2 bus
> +
> gAmdMinBoardPkgTokenSpaceGuid.PcdPciHotPlugResourcePadBus|2|UINT8
> |0x1003
> + # IO Resource padding in bytes, default 4KB
> +
> gAmdMinBoardPkgTokenSpaceGuid.PcdPciHotPlugResourcePadIo|0x10
> 00|UINT32|0x1000
> + # Non-PreFetch Memory padding in bytes, default 1MB
> +
> gAmdMinBoardPkgTokenSpaceGuid.PcdPciHotPlugResourcePadMem|0x0010
> |UINT32|0x1002
> + # PreFetch Memory padding in bytes, default 2MB
> +
> gAmdMinBoardPkgTokenSpaceGuid.PcdPciHotPlugResourcePadPMem|0x002
> 0|UINT32|0x1001
> +
> diff --git a/Platform/AMD/AmdMinBoardPkg/AmdMinBoardPkg.dsc
> b/Platform/AMD/AmdMinBoardPkg/AmdMinBoardPkg.dsc
> index 273cd74f7842..1a8407250c56 100644
> --- a/Platform/AMD/AmdMinBoardPkg/AmdMinBoardPkg.dsc
> +++ b/Platform/AMD/AmdMinBoardPkg/AmdMinBoardPkg.dsc
> @@ -9,7 +9,7 @@
>
> [Defines]
>DSC_SPECIFICATION = 1.30
> - PLATFORM_GUID = 88F8A9AE-2FA0-4D58-A6F9-05F635C05F4E
> + PLATFORM_GUID = 939B559B-269B-4B8F-9637-44DF6575C1E2
>PLATFORM_NAME = AmdMinBoardPkg
>PLATFORM_VERSION= 0.1
>OUTPUT_DIRECTORY= Build/$(PLATFORM_NAME)
> @@ -25,6 +25,16 @@ [Packages]
> [LibraryClasses]
>SpcrDeviceLib|AmdMinBoardPkg/Library/SpcrDeviceLib/SpcrDeviceLib.inf
>
> +[LibraryClasses.common]
> + BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
> +
> BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRe
> pStr.inf
> + DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
> +
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemor
> yAllocationLib.inf
> + PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> +
> RegisterFilterLib|MdePkg/Library/RegisterFilterLibNull/RegisterFilterLibNull.inf
> +
> UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBoo
> tServicesTableLib.inf
> +
> UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryP
> oint.inf
> +
> [LibraryClasses.common.PEIM]
>
> SetCacheMtrrLib|AmdMinBoardPkg/Library/SetCacheMtrrLib/SetCacheMtrrLi
> b.inf
>
> @@ -34,3 +44,5 @@ [Components]
> [Components.IA32]
>AmdMinBoardPkg/Library/SetCacheMtrrLib/SetCacheMtrrLib.inf
>
> +[Components.X64]
> + AmdMinBoardPkg/PciHotPlug/PciHotPlugInit.inf
> diff --git a/Platform/AMD/AmdMinBoardPkg/PciHotPlug/PciHotPlugInit.inf
> b/Platform/AMD/AmdMinBoardPkg/PciHotPlug/PciHotPlugInit.inf
> new file mode 100755
> index ..44564df38718
> --- /dev/null
> +++ b/Platform/AMD/AmdMinBoardPkg/PciHotPlug/PciHotPlugInit.inf
> @@ -0,0 +1,39 @@
> +## @file
> +# This driver implements EFI_PCI_HOT_PLUG_INIT_PROTOCOL.
> +# Adds resource padding information, for PCIe hotplug purposes.
> +#
> +# Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved
> +# SPDX-License-Identifier: BSD-2-Clause-Patent ##
> +
> +[Defines]
> + INF_VERSION= 1.29
> + BASE_NAME = PciHotPlugInit
> + FILE_GUID = 85F78A6D-6438-4BCC-B796-759A48D00C72
> + MODULE_TYPE= DXE_DRIVER
> + VERSION_STRING
Re: [edk2-devel] [PATCH 1/1] AMD/AmdMinBoardPkg: Implements PCI hotplug init protocol
[AMD Official Use Only - General]
Hi Abdul,
Is this change just a initial patch that clone PciHotPlugInit.c to under
AmdMIniBordPkg? Do we have AMD modification on this file? Because I don't see
AMD license in the file header.
Just curious about why do we change the GUID of DSC file?
Thanks
Abner
> -Original Message-
> From: devel@edk2.groups.io On Behalf Of Abdul
> Lateef Attar via groups.io
> Sent: Monday, May 22, 2023 11:54 AM
> To: devel@edk2.groups.io
> Cc: Attar, AbdulLateef (Abdul Lateef) ; Chang,
> Abner
> Subject: [edk2-devel] [PATCH 1/1] AMD/AmdMinBoardPkg: Implements PCI
> hotplug init protocol
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> From: Abdul Lateef Attar
>
> Implements PCI hotplug init protocol.
> Adds resources padding based on PCD values.
>
> Cc: Abner Chang
>
> Signed-off-by: Abdul Lateef Attar
> ---
> .../AMD/AmdMinBoardPkg/AmdMinBoardPkg.dec | 16 +
> .../AMD/AmdMinBoardPkg/AmdMinBoardPkg.dsc | 14 +-
> .../PciHotPlug/PciHotPlugInit.inf | 39 +++
> .../PciHotPlug/PciHotPlugInit.c | 331 ++
> 4 files changed, 399 insertions(+), 1 deletion(-)
> create mode 100755
> Platform/AMD/AmdMinBoardPkg/PciHotPlug/PciHotPlugInit.inf
> create mode 100755
> Platform/AMD/AmdMinBoardPkg/PciHotPlug/PciHotPlugInit.c
>
> diff --git a/Platform/AMD/AmdMinBoardPkg/AmdMinBoardPkg.dec
> b/Platform/AMD/AmdMinBoardPkg/AmdMinBoardPkg.dec
> index e37b02c4cf5a..65ba08545021 100644
> --- a/Platform/AMD/AmdMinBoardPkg/AmdMinBoardPkg.dec
> +++ b/Platform/AMD/AmdMinBoardPkg/AmdMinBoardPkg.dec
> @@ -17,3 +17,19 @@ [Defines]
>PACKAGE_GUID = 44F9D761-9ECB-43DD-A5AC-177E5048701B
>PACKAGE_VERSION= 0.1
>
> +[Guids]
> + gAmdMinBoardPkgTokenSpaceGuid = {0xd4d23d79, 0x73bf, 0x460a,
> {0xa1, 0xc7, 0x85, 0xa3, 0xca, 0x71, 0xb9, 0x4c}}
> +
> +[PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
> + #
> + # PCI HotPlug Resource Padding
> + #
> + # PCI bus padding, number of bus to reserve, default 2 bus
> +
> gAmdMinBoardPkgTokenSpaceGuid.PcdPciHotPlugResourcePadBus|2|UINT8
> |0x1003
> + # IO Resource padding in bytes, default 4KB
> +
> gAmdMinBoardPkgTokenSpaceGuid.PcdPciHotPlugResourcePadIo|0x10
> 00|UINT32|0x1000
> + # Non-PreFetch Memory padding in bytes, default 1MB
> +
> gAmdMinBoardPkgTokenSpaceGuid.PcdPciHotPlugResourcePadMem|0x0010
> |UINT32|0x1002
> + # PreFetch Memory padding in bytes, default 2MB
> +
> gAmdMinBoardPkgTokenSpaceGuid.PcdPciHotPlugResourcePadPMem|0x002
> 0|UINT32|0x1001
> +
> diff --git a/Platform/AMD/AmdMinBoardPkg/AmdMinBoardPkg.dsc
> b/Platform/AMD/AmdMinBoardPkg/AmdMinBoardPkg.dsc
> index 273cd74f7842..1a8407250c56 100644
> --- a/Platform/AMD/AmdMinBoardPkg/AmdMinBoardPkg.dsc
> +++ b/Platform/AMD/AmdMinBoardPkg/AmdMinBoardPkg.dsc
> @@ -9,7 +9,7 @@
>
> [Defines]
>DSC_SPECIFICATION = 1.30
> - PLATFORM_GUID = 88F8A9AE-2FA0-4D58-A6F9-05F635C05F4E
> + PLATFORM_GUID = 939B559B-269B-4B8F-9637-44DF6575C1E2
>PLATFORM_NAME = AmdMinBoardPkg
>PLATFORM_VERSION= 0.1
>OUTPUT_DIRECTORY= Build/$(PLATFORM_NAME)
> @@ -25,6 +25,16 @@ [Packages]
> [LibraryClasses]
>SpcrDeviceLib|AmdMinBoardPkg/Library/SpcrDeviceLib/SpcrDeviceLib.inf
>
> +[LibraryClasses.common]
> + BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
> +
> BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRe
> pStr.inf
> + DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
> +
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemor
> yAllocationLib.inf
> + PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> +
> RegisterFilterLib|MdePkg/Library/RegisterFilterLibNull/RegisterFilterLibNull.inf
> +
> UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBoo
> tServicesTableLib.inf
> +
> UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryP
> oint.inf
> +
> [LibraryClasses.common.PEIM]
>
> SetCacheMtrrLib|AmdMinBoardPkg/Library/SetCacheMtrrLib/SetCacheMtrrLi
> b.inf
>
> @@ -34,3 +44,5 @@ [Components]
> [Components.IA32]
>AmdMinBoardPkg/Library/SetCacheMtrrLib/SetCacheMtrrLib.inf
>
> +[Components.X64]
> + AmdMinBoardPkg/PciHotPlug/PciHotPlugInit.inf
> diff --git a/Platform/AMD/AmdMinBoardPkg/PciHotPlug/PciHotPlugInit.inf
> b/Platform/AMD/AmdMinBoardPkg/PciHotPlug/PciHotPlugInit.inf
> new file mode 100755
> index ..44564df38718
> --- /dev/null
> +++ b/Platform/AMD/AmdMinBoardPkg/PciHotPlug/PciHotPlugInit.inf
> @@ -0,0 +1,39 @@
> +## @file
> +# This driver implements EFI_PCI_HOT_PLUG_INIT_PROTOCOL.
> +# Adds resource padding information, for PCIe hotplug purposes.
> +#
> +# Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved
> +# SPDX-License-Identifier: BSD-2-Clause-Patent ##
> +
>
[edk2-devel] [PATCH v1] BaseTools: toolsetup.bat always execute PYTHON_HOME
From: Gua Guo Ideally behavior is like below order that can support one local build machine, clone multiple Edk2, some of edk2 repo use old tag and some of edk2 repo use new tag, they can both support on one machine. 1. if defined PYTHON_COMMAND only - use PYTHON_COMMAND = user assigned 2. if not defined PYTHON_COMMAND, auto detect py -3 - use PYTHON_COMMAND = py -3 3. if defined PYTHON_COMMAND and PYTHON_HOME, use PYTHON_COMMAND - use PYTHON_COMMAND = user assigned 4. if defined PYTHON_HOME only, - use PYTHON_COMMAND = %PYTHON_HOME%/python.exe SCRIPT_ERROR should return for paraent batch file to consume for error handle. Cc: Rebecca Cran Cc: Liming Gao Cc: Bob Feng Cc: Yuwei Chen Signed-off-by: Gua Guo --- BaseTools/toolsetup.bat | 33 + 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/BaseTools/toolsetup.bat b/BaseTools/toolsetup.bat index 9521f67c02..8aef28192e 100755 --- a/BaseTools/toolsetup.bat +++ b/BaseTools/toolsetup.bat @@ -313,23 +313,25 @@ if not defined PYTHON_COMMAND ( ) ) -if defined PYTHON_HOME ( - if EXIST "%PYTHON_HOME%" ( -set PYTHON_COMMAND=%PYTHON_HOME%\python.exe - ) else ( -echo . -echo !!! ERROR !!! PYTHON_HOME="%PYTHON_HOME%" does not exist. -echo . -goto end +if not defined PYTHON_COMMAND ( + if defined PYTHON_HOME ( +if EXIST "%PYTHON_HOME%" ( + set PYTHON_COMMAND=%PYTHON_HOME%\python.exe +) else ( + echo . + echo !!! ERROR !!! PYTHON_HOME="%PYTHON_HOME%" does not exist. + echo . + goto end +) ) -) -%PYTHON_COMMAND% %BASE_TOOLS_PATH%\Tests\PythonTest.py %PYTHON_VER_MAJOR% %PYTHON_VER_MINOR% >NUL 2>NUL -if %ERRORLEVEL% EQU 1 ( - echo. - echo !!! ERROR !!! Python %PYTHON_VER_MAJOR%.%PYTHON_VER_MINOR% or newer is required. - echo. - goto end + %PYTHON_COMMAND% %BASE_TOOLS_PATH%\Tests\PythonTest.py %PYTHON_VER_MAJOR% %PYTHON_VER_MINOR% >NUL 2>NUL + if %ERRORLEVEL% EQU 1 ( +echo. +echo !!! ERROR !!! Python %PYTHON_VER_MAJOR%.%PYTHON_VER_MINOR% or newer is required. +echo. +goto end + ) ) if %ERRORLEVEL% NEQ 0 ( echo. @@ -447,5 +449,4 @@ set VS2015= set VSTool= set PYTHON_VER_MAJOR= set PYTHON_VER_MINOR= -set SCRIPT_ERROR= popd -- 2.39.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105326): https://edk2.groups.io/g/devel/message/105326 Mute This Topic: https://groups.io/mt/99141348/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes
On Thu, 25 May 2023 at 19:21, Oliver Smith-Denny wrote: > > On 5/25/2023 7:30 AM, Ard Biesheuvel wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4468 > > > > > > > > This is a proof-of-concept RFC that implements a PEI phase PPI to manage > > > > memory permission attributes, and wires it up to the PEI image loader so > > > > that shadowed PEIMs as well as the DXE core are remapped with the > > > > appropriate, restricted memory permission attributes before execution. > > > > > > > > This means that neither shadowed PEIMs nor the DXE core will ever > > > > execute with writable code regions. It also removes the need on the part > > > > of PEI for memory to be mapped with both writable and executable > > > > permissions by default out of reset. Similar work still needs to be done > > > > to address the early DXE phase (before the CPU arch protocol becomes > > > > available), but once that is out of the way as well, platforms should be > > > > able to map all memory non-executable from the beginning. > > > > > > > > This by itself is a major improvement in terms of robustness. It is also > > > > a prerequisite for enabling the WXN MMU control on AArch64, which makes > > > > all writable memory mappings non-executable regardless of the non-exec > > > > page table attribute. > > > > > > > > Patches #1 to #4 are prepatory work. > > > > Patch #5 proposes the memory attribute PPI protocol interface. > > > > Patch #6 implements it for ARM and AARCH64. > > > > Patch #7 wires it up into the PEI image loader. > > > > Patches #8 to #10 update the DxeIpl to use this PPI on ARM/AARCH64 for > > > > mapping the stack NX. > > > > instead of an explicit reference to ArmMmuLib. Other architectures > > > > (except IA32/X64) will seamlessly inherit this once they implement the > > > > PPI as well. > > > > Hi Ard, > Hi, Thanks for taking a look. > Thanks for the RFC. This same issue exists for X64, I saw your mailing > list question about IA32 PEI, do you have plans for extending this to > X64 PEI (I'm not sure its state of readiness)? > Yes, but I need to wrap my head around that code first (I'm not as familiar with low-level X64 as I am with ARM). I though I'd send this RFC out in the meantime to get some alignment on the general shape of things. > If so, do you think it is feasible to merge the X64 DxeLoadFunc with > the generic one you have created? > Hopefully. There are some pieces there that are highly X64 specific, though, but at least the permissions management code could be shared, in which case it can be moved out of the arch-specific source file and into the generic one (this is one of the reasons I merged that code between ARM, AARCH64, RISC-V and LoongArch64). Pure IA32 will simply not implement the PPI, and the long mode switch stuff can remain IA32 specific afaict (although we'll be retaining that only for diagnostic purposes) > Overall, this seems a reasonable approach to me towards making DXE more > protected with the additional benefit of protecting shadowed PEIMs. > > I did wonder if the same discussion we are having on the DXE side about > moving these MMU manipulations to the core instead of the CPU driver > make sense in PEI, too, but with the greatly reduced complexity of the > environment (no GCD, etc.), I don't think it makes sense now and that > focusing on the DXE reorganization and expansion is going to deliver > a much greater impact. > IMHO the modularity has its advantages in some cases, and this particular use case does not force us to deviate from that principle, so I'd rather keep that as a separate discussion. On ARM (and now on X64 for TDX) we already have PEI-less platforms where SEC dispatches the DXE core directly, so I'm not convinced we really need something in between. -- Ard. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105325): https://edk2.groups.io/g/devel/message/105325 Mute This Topic: https://groups.io/mt/99131172/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH 07/10] MdeModulePkg/PeiCore: Apply restricted permissions in image loader
On Thu, 25 May 2023 at 19:21, Oliver Smith-Denny
wrote:
>
> On 5/25/2023 7:30 AM, Ard Biesheuvel wrote:
> > Add a notification callback to the PEI core to grab a reference to the
> > memory attributes PPI as soon as it is registered, and use it in the
> > image loader to set restricted memory permissions after loading the
> > image if the image was loaded into memory.
> >
> > There are two use cases for this:
> > - when the DXE IPL loads the DXE core using the PEI image services, its
> >mappings will be set according to the PE section permission attributes
> >if the image was built with 4k section alignment; this means DXE core
> >will never run with mappings that are both writable and executable.
> > - when PEIMs are shadowed to memory, they will not only be mapped
> >read-only, but any non-exec permissions will also be removed. (Note
> >that this requires the component that installs PEI permanent memory to
> >depex on the memory attributes PPI, to ensure that it is available to
> >manage permissions on permanent memory before it is used to load
> >images)
> >
> > With this logic in place *, there is no longer a need for system memory
> > to be mapped with both write and execute permissions out of reset.
> > Instead, all memory can be mapped with non-executable permissions by
> > default, which means that the stack and other allocations used in PEI or
> > early in DXE will no longer need to be mapped non-exec explicitly.
> >
> > * the DXE core will also need its own method for clearing non-exec
> >permissions on memory ranges, but this is being addressed in a
> >separate series.
> >
> > Signed-off-by: Ard Biesheuvel
> > ---
> > MdeModulePkg/Core/Pei/Image/Image.c | 160
> > MdeModulePkg/Core/Pei/PeiMain.h | 6 +
> > MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
> > 3 files changed, 167 insertions(+)
> >
> > diff --git a/MdeModulePkg/Core/Pei/Image/Image.c
> > b/MdeModulePkg/Core/Pei/Image/Image.c
> > index cee9f09c6ea61e31..3a7de45014b8f772 100644
> > --- a/MdeModulePkg/Core/Pei/Image/Image.c
> > +++ b/MdeModulePkg/Core/Pei/Image/Image.c
> > @@ -18,6 +18,50 @@ EFI_PEI_PPI_DESCRIPTOR gPpiLoadFilePpiList = {
> >
> >
> > };
> >
> >
> >
> > +/**
> >
> > + Provide a callback for when the memory attributes PPI is installed.
> >
> > +
> >
> > + @param PeiServicesAn indirect pointer to the EFI_PEI_SERVICES
> > table
> >
> > +published by the PEI Foundation.
> >
> > + @param NotifyDescriptor The descriptor for the notification event.
> >
> > + @param PpiPointer to the PPI in question.
> >
> > +
> >
> > + @return Always success
> >
> > +
> >
> > +**/
> >
> > +STATIC
> >
> > +EFI_STATUS
> >
> > +EFIAPI
> >
> > +MemoryAttributePpiNotifyCallback (
> >
> > + IN EFI_PEI_SERVICES **PeiServices,
> >
> > + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor,
> >
> > + IN VOID *Ppi
> >
> > + )
> >
> > +{
> >
> > + PEI_CORE_INSTANCE *PrivateData;
> >
> > +
> >
> > + //
> >
> > + // Get PEI Core private data
> >
> > + //
> >
> > + PrivateData = PEI_CORE_INSTANCE_FROM_PS_THIS (PeiServices);
> >
> > +
> >
> > + //
> >
> > + // If there isn't a memory attribute PPI installed, use the one from
> >
> > + // notification
> >
> > + //
> >
> > + if (PrivateData->MemoryAttributePpi == NULL) {
> >
> > +PrivateData->MemoryAttributePpi = (EDKII_MEMORY_ATTRIBUTE_PPI *)Ppi;
> >
> > + }
> >
> > +
> >
> > + return EFI_SUCCESS;
> >
> > +}
> >
> > +
> >
> > +STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mNotifyList = {
> >
> > + EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH |
> > EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
> >
> > + ,
> >
> > + MemoryAttributePpiNotifyCallback
> >
> > +};
> >
> > +
> >
> > /**
> >
> >
> >
> > Support routine for the PE/COFF Loader that reads a buffer from a
> > PE/COFF file.
> >
> > @@ -243,6 +287,106 @@ GetPeCoffImageFixLoadingAssignedAddress (
> > return Status;
> >
> > }
> >
> >
> >
> > +/**
> >
> > + Remap the memory region covering a loaded image so it can be executed.
> >
> > +
> >
> > + @param ImageContextPointer to the image context structure that
> > describes the
> >
> > + PE/COFF image that needs to be examined by this
> > function.
> >
> > + @param FileTypeThe FFS file type of the image
> >
> > + @param ImageAddressThe start of the memory region covering the image
> >
> > + @param ImageSize The size of the memory region covering the image
> >
> > +
> >
> > + @retval EFI_SUCCESS The image is ready to be executed
> >
> > + @retval EFI_OUT_OF_RESOURCES Not enough memory available to update the
> > memory
> >
> > +mapping
> >
> > +
> >
> > +**/
> >
> > +STATIC
> >
> > +EFI_STATUS
> >
> > +RemapLoadedImageForExecution (
> >
> > + IN CONST PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext,
> >
> > + IN EFI_FV_FILETYPE
Re: [edk2-devel] GoogleTest Compatibility with MdePkg's IndustyStandard header files
Pedro and Oliver, Yes. Renaming the struct members is my preferred solution. This is why I did not send this as a code review as an official change request. It was just to complete the set of options to consider * No code changes. Figure out compiler flags to address. STATUS: No complete solution found for all compilers. * Use C-Preprocessor to rename keywords. STATUS: Functional, but very bad style and hard to read and maintain. * Rename C structure fields that collide with C++ keywords STATUS: Functional. May break downstream consumers that have FW code that references those fields. Thanks, Mike > -Original Message- > From: Pedro Falcato > Sent: Thursday, May 25, 2023 11:01 AM > To: Oliver Smith-Denny > Cc: devel@edk2.groups.io; Kinney, Michael D ; > Pop, Aaron > Subject: Re: [edk2-devel] GoogleTest Compatibility with MdePkg's > IndustyStandard header files > > On Thu, May 25, 2023 at 6:43 PM Oliver Smith-Denny > wrote: > > > > Hi Mike, > > > > Thanks for looking for solutions here. This one feels like > > quite a back bend, I'm imagining reading code and coming > > across TpmStruct.CPLUSPLUS_OPERATOR_KEYWORD and having to > > dig around quite a lot to see what goodness is going > > on. Because we would have to update the C files, too, right, > > No, the idea is that current C code can use the > already-existing-and-standard .operator and .xor, and C++ can use > .operator_ and .xor_ (or the macros, although please no?). > But my idea was to leave this as a Tpm.h hack, not in Base.h (new > headers and structs should take C++ into account). > > > depending on the test (there exist tests that want to test > > static functions and so include the C file in the unit test > > file). Perhaps that is an anti-pattern and googletest has > > This is a hacky solution. Either write things in C++, or don't include > .c in .cpp. > C code is not C++ code. There's a lot of C code that does not and > should not compile in C++. > > So: > > 1) Write the actual functionality code in C++. This is not yet > supported in EDK2 (I'm a proponent of this) > 2) Don't make the functions you're testing static, or make them > conditionally static on something > > Note: Adding proper, actual C++ code to EDK2 requires some care, but > could result in actual good changes. I don't know how well this would > be received by the community though. > > > But, that being said, this is an issue we face, so perhaps > > it would be simpler to just rename the members to not conflict > > with the C++ keywords, as previously suggested, even though > > this may differ from the spec, but it would more align with > > EDKII's conventions (shouty case) where the C++ keywords seem > > to be lowercase. With the below patch, we would already be > > I think breaking all sorts of users for this sort of "silly" problems > is not a good option here. There's no actual need for this ATM, apart > from "We want to test this silly code in this new Google Test library > that just appeared upstream". > > But these are just my 2c of course. > > -- > Pedro -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105323): https://edk2.groups.io/g/devel/message/105323 Mute This Topic: https://groups.io/mt/99079638/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] GoogleTest Compatibility with MdePkg's IndustyStandard header files
On Thu, May 25, 2023 at 6:43 PM Oliver Smith-Denny wrote: > > Hi Mike, > > Thanks for looking for solutions here. This one feels like > quite a back bend, I'm imagining reading code and coming > across TpmStruct.CPLUSPLUS_OPERATOR_KEYWORD and having to > dig around quite a lot to see what goodness is going > on. Because we would have to update the C files, too, right, No, the idea is that current C code can use the already-existing-and-standard .operator and .xor, and C++ can use .operator_ and .xor_ (or the macros, although please no?). But my idea was to leave this as a Tpm.h hack, not in Base.h (new headers and structs should take C++ into account). > depending on the test (there exist tests that want to test > static functions and so include the C file in the unit test > file). Perhaps that is an anti-pattern and googletest has This is a hacky solution. Either write things in C++, or don't include .c in .cpp. C code is not C++ code. There's a lot of C code that does not and should not compile in C++. So: 1) Write the actual functionality code in C++. This is not yet supported in EDK2 (I'm a proponent of this) 2) Don't make the functions you're testing static, or make them conditionally static on something Note: Adding proper, actual C++ code to EDK2 requires some care, but could result in actual good changes. I don't know how well this would be received by the community though. > But, that being said, this is an issue we face, so perhaps > it would be simpler to just rename the members to not conflict > with the C++ keywords, as previously suggested, even though > this may differ from the spec, but it would more align with > EDKII's conventions (shouty case) where the C++ keywords seem > to be lowercase. With the below patch, we would already be I think breaking all sorts of users for this sort of "silly" problems is not a good option here. There's no actual need for this ATM, apart from "We want to test this silly code in this new Google Test library that just appeared upstream". But these are just my 2c of course. -- Pedro -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105322): https://edk2.groups.io/g/devel/message/105322 Mute This Topic: https://groups.io/mt/99079638/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] GoogleTest Compatibility with MdePkg's IndustyStandard header files
Hi Mike,
Thanks for looking for solutions here. This one feels like
quite a back bend, I'm imagining reading code and coming
across TpmStruct.CPLUSPLUS_OPERATOR_KEYWORD and having to
dig around quite a lot to see what goodness is going
on. Because we would have to update the C files, too, right,
depending on the test (there exist tests that want to test
static functions and so include the C file in the unit test
file). Perhaps that is an anti-pattern and googletest has
other ways of testing static functions, but in general it
feels odd to change our functional C code for a C++ unit
test framework.
But, that being said, this is an issue we face, so perhaps
it would be simpler to just rename the members to not conflict
with the C++ keywords, as previously suggested, even though
this may differ from the spec, but it would more align with
EDKII's conventions (shouty case) where the C++ keywords seem
to be lowercase. With the below patch, we would already be
changing the header file to not match with the spec, so from
a readability perspective of the header file, I think
Operator is much more readable than CPLUSPLUS_OPERATOR_KEYWORD.
Thanks,
Oliver
On 5/25/2023 10:06 AM, Michael D Kinney wrote:
Hi Pedro,
Thanks for the feedback!
Applying your pattern to edk2, we find all the usage of c++
reserved keywords and replace with a macro. We then define
that macro to either be the actual c++ reserved keyword if
build with a C compiler and rename the keyword if building
with a c++ compiler. The following patches build with VS
and GCC and should be no changes from any FW consumers of
Tpm12.h or Tpm20.h files and should support GoogleTest builds
of unit test case and code under test that will consistently
rename the reserved keyword.
We will see if this resolves Aaron specific use case and if
it does we can move this to a code review.
We can always expand the keyword set as needed in the future
if industry standard specs use C definitions that collide
with additional c++ reserved keywords.
diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
index 6597e441a6e2..b98ff33002b8 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -15,6 +15,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#ifndef __BASE_H__
#define __BASE_H__
+//
+// C++ reserved keyword defines
+//
+#if defined (__cplusplus)
+#define CPLUSPLUS_OPERATOR_KEYWORD operator_
+#define CPLUSPLUS_XOR_KEYWORD xor_
+#else
+#define CPLUSPLUS_OPERATOR_KEYWORD operator
+#define CPLUSPLUS_XOR_KEYWORD xor
+#endif
+
//
// Include processor specific binding
//
diff --git a/MdePkg/Include/IndustryStandard/Tpm12.h
b/MdePkg/Include/IndustryStandard/Tpm12.h
index 155dcc9f5f99..8551dd7c5f42 100644
--- a/MdePkg/Include/IndustryStandard/Tpm12.h
+++ b/MdePkg/Include/IndustryStandard/Tpm12.h
@@ -744,7 +744,7 @@ typedef struct tdTPM_PERMANENT_FLAGS {
BOOLEAN TPMpost;
BOOLEAN TPMpostLock;
BOOLEAN FIPS;
- BOOLEAN operator;
+ BOOLEAN CPLUSPLUS_OPERATOR_KEYWORD;
BOOLEAN enableRevokeEK;
BOOLEAN nvLocked;
BOOLEAN readSRKPub;
diff --git a/MdePkg/Include/IndustryStandard/Tpm20.h
b/MdePkg/Include/IndustryStandard/Tpm20.h
index 4440f3769f26..a96af9cfed63 100644
--- a/MdePkg/Include/IndustryStandard/Tpm20.h
+++ b/MdePkg/Include/IndustryStandard/Tpm20.h
@@ -1247,7 +1247,7 @@ typedef union {
TPMI_AES_KEY_BITSaes;
TPMI_SM4_KEY_BITSSM4;
TPM_KEY_BITS sym;
- TPMI_ALG_HASH xor;
+ TPMI_ALG_HASH CPLUSPLUS_XOR_KEYWORD;
} TPMU_SYM_KEY_BITS;
// Table 123 - TPMU_SYM_MODE Union
@@ -1320,7 +1320,7 @@ typedef struct {
// Table 136 - TPMU_SCHEME_KEYEDHASH Union
typedef union {
TPMS_SCHEME_HMAChmac;
- TPMS_SCHEME_XOR xor;
+ TPMS_SCHEME_XOR CPLUSPLUS_XOR_KEYWORD;
} TPMU_SCHEME_KEYEDHASH;
// Table 137 - TPMT_KEYEDHASH_SCHEME Structure
Mike
-Original Message-
From: Pedro Falcato
Sent: Thursday, May 25, 2023 2:44 AM
To: devel@edk2.groups.io; Kinney, Michael D
Cc: Pop, Aaron
Subject: Re: [edk2-devel] GoogleTest Compatibility with MdePkg's
IndustyStandard header files
On Thu, May 25, 2023 at 1:24 AM Michael D Kinney
wrote:
That is exactly what I did. Along with pragma to disable error on macros
redefining operators.
With that change use of "operator" did not generate a warning or error and
the build completed.
Did not work for "xor", and can not find any additional pragma to suppress
that error.
FWIW, it does work here: https://godbolt.org/z/EEdh9oh53
--
Pedro
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105321): https://edk2.groups.io/g/devel/message/105321
Mute This Topic: https://groups.io/mt/99079638/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
Re: [edk2-devel] [RFC PATCH 07/10] MdeModulePkg/PeiCore: Apply restricted permissions in image loader
On 5/25/2023 7:30 AM, Ard Biesheuvel wrote:
Add a notification callback to the PEI core to grab a reference to the
memory attributes PPI as soon as it is registered, and use it in the
image loader to set restricted memory permissions after loading the
image if the image was loaded into memory.
There are two use cases for this:
- when the DXE IPL loads the DXE core using the PEI image services, its
mappings will be set according to the PE section permission attributes
if the image was built with 4k section alignment; this means DXE core
will never run with mappings that are both writable and executable.
- when PEIMs are shadowed to memory, they will not only be mapped
read-only, but any non-exec permissions will also be removed. (Note
that this requires the component that installs PEI permanent memory to
depex on the memory attributes PPI, to ensure that it is available to
manage permissions on permanent memory before it is used to load
images)
With this logic in place *, there is no longer a need for system memory
to be mapped with both write and execute permissions out of reset.
Instead, all memory can be mapped with non-executable permissions by
default, which means that the stack and other allocations used in PEI or
early in DXE will no longer need to be mapped non-exec explicitly.
* the DXE core will also need its own method for clearing non-exec
permissions on memory ranges, but this is being addressed in a
separate series.
Signed-off-by: Ard Biesheuvel
---
MdeModulePkg/Core/Pei/Image/Image.c | 160
MdeModulePkg/Core/Pei/PeiMain.h | 6 +
MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
3 files changed, 167 insertions(+)
diff --git a/MdeModulePkg/Core/Pei/Image/Image.c
b/MdeModulePkg/Core/Pei/Image/Image.c
index cee9f09c6ea61e31..3a7de45014b8f772 100644
--- a/MdeModulePkg/Core/Pei/Image/Image.c
+++ b/MdeModulePkg/Core/Pei/Image/Image.c
@@ -18,6 +18,50 @@ EFI_PEI_PPI_DESCRIPTOR gPpiLoadFilePpiList = {
};
+/**
+ Provide a callback for when the memory attributes PPI is installed.
+
+ @param PeiServicesAn indirect pointer to the EFI_PEI_SERVICES table
+published by the PEI Foundation.
+ @param NotifyDescriptor The descriptor for the notification event.
+ @param PpiPointer to the PPI in question.
+
+ @return Always success
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+MemoryAttributePpiNotifyCallback (
+ IN EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor,
+ IN VOID *Ppi
+ )
+{
+ PEI_CORE_INSTANCE *PrivateData;
+
+ //
+ // Get PEI Core private data
+ //
+ PrivateData = PEI_CORE_INSTANCE_FROM_PS_THIS (PeiServices);
+
+ //
+ // If there isn't a memory attribute PPI installed, use the one from
+ // notification
+ //
+ if (PrivateData->MemoryAttributePpi == NULL) {
+PrivateData->MemoryAttributePpi = (EDKII_MEMORY_ATTRIBUTE_PPI *)Ppi;
+ }
+
+ return EFI_SUCCESS;
+}
+
+STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mNotifyList = {
+ EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH |
EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
+ ,
+ MemoryAttributePpiNotifyCallback
+};
+
/**
Support routine for the PE/COFF Loader that reads a buffer from a PE/COFF
file.
@@ -243,6 +287,106 @@ GetPeCoffImageFixLoadingAssignedAddress (
return Status;
}
+/**
+ Remap the memory region covering a loaded image so it can be executed.
+
+ @param ImageContextPointer to the image context structure that describes
the
+ PE/COFF image that needs to be examined by this
function.
+ @param FileTypeThe FFS file type of the image
+ @param ImageAddressThe start of the memory region covering the image
+ @param ImageSize The size of the memory region covering the image
+
+ @retval EFI_SUCCESS The image is ready to be executed
+ @retval EFI_OUT_OF_RESOURCES Not enough memory available to update the
memory
+mapping
+
+**/
+STATIC
+EFI_STATUS
+RemapLoadedImageForExecution (
+ IN CONST PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext,
+ IN EFI_FV_FILETYPE FileType,
+ IN PHYSICAL_ADDRESSImageAddress,
+ IN UINT64 ImageSize
+ )
+{
+ PEI_CORE_INSTANCE*Private;
+ EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr;
+ EFI_IMAGE_SECTION_HEADER *Section;
+ PHYSICAL_ADDRESS SectionAddress;
+ EFI_STATUS Status;
+ UINT64 Permissions;
+ UINTNIndex;
+
+ Private = PEI_CORE_INSTANCE_FROM_PS_THIS (GetPeiServicesTablePointer ());
+
+ if (Private->MemoryAttributePpi == NULL) {
+return EFI_SUCCESS;
We will have a gap here before the MemoryAttributePpi is installed,
Re: [edk2-devel] [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes
On 5/25/2023 7:30 AM, Ard Biesheuvel wrote: REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4468 This is a proof-of-concept RFC that implements a PEI phase PPI to manage memory permission attributes, and wires it up to the PEI image loader so that shadowed PEIMs as well as the DXE core are remapped with the appropriate, restricted memory permission attributes before execution. This means that neither shadowed PEIMs nor the DXE core will ever execute with writable code regions. It also removes the need on the part of PEI for memory to be mapped with both writable and executable permissions by default out of reset. Similar work still needs to be done to address the early DXE phase (before the CPU arch protocol becomes available), but once that is out of the way as well, platforms should be able to map all memory non-executable from the beginning. This by itself is a major improvement in terms of robustness. It is also a prerequisite for enabling the WXN MMU control on AArch64, which makes all writable memory mappings non-executable regardless of the non-exec page table attribute. Patches #1 to #4 are prepatory work. Patch #5 proposes the memory attribute PPI protocol interface. Patch #6 implements it for ARM and AARCH64. Patch #7 wires it up into the PEI image loader. Patches #8 to #10 update the DxeIpl to use this PPI on ARM/AARCH64 for mapping the stack NX. instead of an explicit reference to ArmMmuLib. Other architectures (except IA32/X64) will seamlessly inherit this once they implement the PPI as well. Hi Ard, Thanks for the RFC. This same issue exists for X64, I saw your mailing list question about IA32 PEI, do you have plans for extending this to X64 PEI (I'm not sure its state of readiness)? If so, do you think it is feasible to merge the X64 DxeLoadFunc with the generic one you have created? Overall, this seems a reasonable approach to me towards making DXE more protected with the additional benefit of protecting shadowed PEIMs. I did wonder if the same discussion we are having on the DXE side about moving these MMU manipulations to the core instead of the CPU driver make sense in PEI, too, but with the greatly reduced complexity of the environment (no GCD, etc.), I don't think it makes sense now and that focusing on the DXE reorganization and expansion is going to deliver a much greater impact. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105319): https://edk2.groups.io/g/devel/message/105319 Mute This Topic: https://groups.io/mt/99131172/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] GoogleTest Compatibility with MdePkg's IndustyStandard header files
Hi Pedro,
Thanks for the feedback!
Applying your pattern to edk2, we find all the usage of c++
reserved keywords and replace with a macro. We then define
that macro to either be the actual c++ reserved keyword if
build with a C compiler and rename the keyword if building
with a c++ compiler. The following patches build with VS
and GCC and should be no changes from any FW consumers of
Tpm12.h or Tpm20.h files and should support GoogleTest builds
of unit test case and code under test that will consistently
rename the reserved keyword.
We will see if this resolves Aaron specific use case and if
it does we can move this to a code review.
We can always expand the keyword set as needed in the future
if industry standard specs use C definitions that collide
with additional c++ reserved keywords.
diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
index 6597e441a6e2..b98ff33002b8 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -15,6 +15,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#ifndef __BASE_H__
#define __BASE_H__
+//
+// C++ reserved keyword defines
+//
+#if defined (__cplusplus)
+#define CPLUSPLUS_OPERATOR_KEYWORD operator_
+#define CPLUSPLUS_XOR_KEYWORD xor_
+#else
+#define CPLUSPLUS_OPERATOR_KEYWORD operator
+#define CPLUSPLUS_XOR_KEYWORD xor
+#endif
+
//
// Include processor specific binding
//
diff --git a/MdePkg/Include/IndustryStandard/Tpm12.h
b/MdePkg/Include/IndustryStandard/Tpm12.h
index 155dcc9f5f99..8551dd7c5f42 100644
--- a/MdePkg/Include/IndustryStandard/Tpm12.h
+++ b/MdePkg/Include/IndustryStandard/Tpm12.h
@@ -744,7 +744,7 @@ typedef struct tdTPM_PERMANENT_FLAGS {
BOOLEAN TPMpost;
BOOLEAN TPMpostLock;
BOOLEAN FIPS;
- BOOLEAN operator;
+ BOOLEAN CPLUSPLUS_OPERATOR_KEYWORD;
BOOLEAN enableRevokeEK;
BOOLEAN nvLocked;
BOOLEAN readSRKPub;
diff --git a/MdePkg/Include/IndustryStandard/Tpm20.h
b/MdePkg/Include/IndustryStandard/Tpm20.h
index 4440f3769f26..a96af9cfed63 100644
--- a/MdePkg/Include/IndustryStandard/Tpm20.h
+++ b/MdePkg/Include/IndustryStandard/Tpm20.h
@@ -1247,7 +1247,7 @@ typedef union {
TPMI_AES_KEY_BITSaes;
TPMI_SM4_KEY_BITSSM4;
TPM_KEY_BITS sym;
- TPMI_ALG_HASH xor;
+ TPMI_ALG_HASH CPLUSPLUS_XOR_KEYWORD;
} TPMU_SYM_KEY_BITS;
// Table 123 - TPMU_SYM_MODE Union
@@ -1320,7 +1320,7 @@ typedef struct {
// Table 136 - TPMU_SCHEME_KEYEDHASH Union
typedef union {
TPMS_SCHEME_HMAChmac;
- TPMS_SCHEME_XOR xor;
+ TPMS_SCHEME_XOR CPLUSPLUS_XOR_KEYWORD;
} TPMU_SCHEME_KEYEDHASH;
// Table 137 - TPMT_KEYEDHASH_SCHEME Structure
Mike
> -Original Message-
> From: Pedro Falcato
> Sent: Thursday, May 25, 2023 2:44 AM
> To: devel@edk2.groups.io; Kinney, Michael D
> Cc: Pop, Aaron
> Subject: Re: [edk2-devel] GoogleTest Compatibility with MdePkg's
> IndustyStandard header files
>
> On Thu, May 25, 2023 at 1:24 AM Michael D Kinney
> wrote:
> >
> > That is exactly what I did. Along with pragma to disable error on macros
> redefining operators.
> >
> > With that change use of "operator" did not generate a warning or error and
> the build completed.
> >
> > Did not work for "xor", and can not find any additional pragma to suppress
> that error.
>
> FWIW, it does work here: https://godbolt.org/z/EEdh9oh53
>
> --
> Pedro
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105318): https://edk2.groups.io/g/devel/message/105318
Mute This Topic: https://groups.io/mt/99079638/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [edk2-non-osi][PATCH 1/1] Platform/RaspberryPi: Update TF-A to v2.9 to fix rainbow screen on reboot
On Thu, 25 May 2023 at 18:49, Pete Batard wrote: > > The newly released TF-A v2.9 contains a fix for an issue that has been > affecting some Raspberry Pi 3 users, when trying to issue a reboot with > some types of SD cards (See: pftf/RPi3#17, pftf/RPi3#24). > > This fix is documented at: > https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/20297 > > We also use this opportunity to update the TF-A binaries for RPi4. > > Signed-off-by: Pete Batard Thanks Pete Acked-by: Ard Biesheuvel Pushed as 41876073afb7c730..8164ce412e1a1c31 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105317): https://edk2.groups.io/g/devel/message/105317 Mute This Topic: https://groups.io/mt/99133969/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [edk2-non-osi][PATCH 1/1] Platform/RaspberryPi: Update TF-A to v2.9 to fix rainbow screen on reboot
The newly released TF-A v2.9 contains a fix for an issue that has been
affecting some Raspberry Pi 3 users, when trying to issue a reboot with
some types of SD cards (See: pftf/RPi3#17, pftf/RPi3#24).
This fix is documented at:
https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/20297
We also use this opportunity to update the TF-A binaries for RPi4.
Signed-off-by: Pete Batard
---
Platform/RaspberryPi/RPi3/TrustedFirmware/Readme.md | 8
Platform/RaspberryPi/RPi3/TrustedFirmware/bl1.bin | Bin 18853 -> 18853 bytes
Platform/RaspberryPi/RPi3/TrustedFirmware/fip.bin | Bin 53988 -> 53988 bytes
Platform/RaspberryPi/RPi4/TrustedFirmware/Readme.md | 6 +++---
Platform/RaspberryPi/RPi4/TrustedFirmware/bl31.bin | Bin 45163 -> 45163 bytes
5 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/Platform/RaspberryPi/RPi3/TrustedFirmware/Readme.md
b/Platform/RaspberryPi/RPi3/TrustedFirmware/Readme.md
index 4b8373493d55..23348e0dc98b 100644
--- a/Platform/RaspberryPi/RPi3/TrustedFirmware/Readme.md
+++ b/Platform/RaspberryPi/RPi3/TrustedFirmware/Readme.md
@@ -2,14 +2,14 @@ ARM Trusted Firmware for Raspberry Pi 3
===
The `bl1.bin` and `fip.bin` TF-A binaries found in this directory were built
from the
-[official TF-A 2.6
release](https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tag/?h=v2.6)
+[official TF-A 2.9
release](https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tag/?h=v2.9)
through a [GitHub build
script](https://github.com/pftf/pitf/blob/master/.github/workflows/build.yml)
that is designed to provide evidence that these binaries match the vanilla
TF-A source.
-Per the [GitHub Actions log](https://github.com/pftf/pitf/runs/1668471269),
+Per the [GitHub Actions
log](https://github.com/pftf/pitf/actions/runs/5071490963),
the SHA-256 sums for the blobs can be validated to be as follows:
-- `bl1.bin`: `787acb2ca1c99678dcdec70a64b9602f8f8f658a4abb0b3f7edfa5f5efb22f73`
-- `fip.bin`: `fd85f9a230aad88f6a59cf0c5d88e6067f23fb8080c6b8bdc61f1a5f6cbbf9ec`
+- `bl1.bin`: `de32a81d17a3a1251245dd6d9b0a807e6ec54d5b50eecfda31fc9cd2ffb1d5ba`
+- `fip.bin`: `7463cec7914469998d3cfab80b80710e371e8d5673959b303a7c851e37b179bb`
For Raspberry Pi 3 usage, TF-A was built using the command:
```
diff --git a/Platform/RaspberryPi/RPi3/TrustedFirmware/bl1.bin
b/Platform/RaspberryPi/RPi3/TrustedFirmware/bl1.bin
index
cbbde5f744a7e2f50232a2e6c1a9de1d5254669c..e3076b1dc9898fcd6f6db694b2507a69fb371874
100644
GIT binary patch
delta 5103
zcmZ`+3s6*7n*Ptd4K$$GbT24${hz}HkuY?OYK4y)H*x9ubQ#KM^U(q0HbRg5W
z(a8%<<$5q?j1^GFjNMZ+>w<2=mWc$dnM}D!DF%0w&6bU5RzotQsL`uv?{{u@qo!(`
zx}5X>=Rg1X&;LFLIXlQ%GR6A3TGBu4Crfts`BJ(211?{i~K^%%?I
zsp)l$Y8ss;GHE`HE_IaVB0BZ5_C*?7rCoBQegLZD3UymC{*tI^KE69HGt-pr
zdopR)7c##qYv$Qf6J5LcyyX9k$n7M0hfZ3wUz@1u8a}c8CzBwrEoxhRLE80X`54?cNWbcX9A_2<9+RO^=D(1AtQ$
z(y-qPX{hN27CVTFdN3M?zGu~P!62T<-Xo_3gwz+oC`xD0EVgNx#-aB7{o%v
zJk0CAY}${ZP@1u)#jYhqXtzPQ>kDU=`ES}3{a)RPL-i)I8$>_YwOiP5ukKSrE7ucA
zhxCCkgB0z)p-x-idZO6Zc}SP}CV4sY@c)rZ*{cO=t#9Ktt=FB
zcgRc53Ey>Yp4+};(a76mq6N;)@$Gl`v+;FPU?tayh|*(!->W<3-v-<^CCl&~
z;jll+Qb0Jd&?J%Cbt{wf-|(RcV~19oDEWw#)_4N^n)ojgp0@9UX?}tQmNDo@j>#9N
z^WPhC}=zliXJ{=xp7A1qR$=@>Z`(H4%EI>ZRw}_6=z)}1sZT8qJQ2g$N9Pr
zq_^}}L9iuM-_OX`#WFs`$Y3ftKo;K~Pb=w8%am@7_LZSvbgvxDvdV}5zorB#(5JzWWGekvb3wEuU8C7kUX%Vh{Ku
zcn{1X;;f>6SlN=x;u}1#onW_OmVD@IuQd?`gGjgm&>>Xg7yfd!%fw9AxUh
zSLh5XH4<%bgtimvU=xL_5bC!|JYtGWVC=3!v%R4f`p02tsXKK9;VMV%1Em_(tSbqx|
z6hR@ZZqUA*5vKKIYB!-x12-hH|8RzOHHrh>UCa$$xg)d}!As;#5hKirnAmKcm@yxB
ztb%_M9j+`TS}pw3*l?0}S|_j~-f#WKvcEu4!W@bWwIl8HR4^?BTnpd}V-;QC_A@%-
zOI6yJ5bGNwBT}r?-di0`ZUg*HnbI_Nz%5|+S5
zoZxmq(qLS31~XG~l$I@`c9ABwVUOqlctPeImc}<`PWGOZ$R3R$p*aylqRK=&5+v+=
zF1^|njwcG#CRLmb0=EgVcP_ShkBFsPh^4g6wmllJnJ`>oPk9>E8MX<+Qe;
zVb3|>mP?ZN)NN$wp!|5Cr-bxB=YF*(SW5cFd7~P?+e+;hU#Iq9+%tI
zUxsXcsYKBaNQ&+w*0@EAX>=hLJSMU>Q+nUyadhVMI5-`p_HD}M!>Fd5Y>hp_
z#IO(gUUzV-(tb3@i{4cp9w0~+#DPIDPM4`MI9<^lG`w%jK*){YI+FvPij6I3AC{
z;$W$QT14OPgHHD%d=xZk1mV?KI_jCAv{wUW;8`_++Npj6uO|9~6A|DsDXlFM=@)S6
zOLAI%Pl22fFez$fP|ghW(|73?zDx-q~P44P8N54pahnUb6M6JUM%W_pgRCH{*6I
zakxWfeP%u6Zn+yOs@}d~=OE*s@qGKvJ4Z6ZlFmL?4bfx_q*_s)8%gQFnNU+=ZuVb
z3D~v#g`CMTb8m-s2l%d>f06DpK5JzDsDlN}LWqNl`OMNxF?~VY*?IJm$l%&;F
z=nmA5ws%KPV!82qr*~i3O?n=yYmMKVX*|x%=+DgT)mC$IcDG0)-1*pIXER=H{aY3N
z>R%T4tyr)q+1-WsdSRXWb`bZk%9H}8?!DH~+X~BF*WNyjTAez1p
z!oeN&^9Pdi*cf&617gdV2gDxCA0pn>HZ}k6lF4=l1+HM6)07}4}hSk)Id9mUT
z_stX#p+B7H^wf%BS_=QoxRT<1+2qay6~lKFp+no*0>@&@c5@p;eYr9>9xkh77<
zajsVn?LLS4^}pOTe!}o>KtVa{R9ze`=#?|`FUaZnWlXKy!dH%8=XJB<6e!
zb=P0r>5_v6TEr?79K(y$4Q{F)f
zDzeDx`;dkOlwL^)GDZG?qxBWCWQ%WU%%`eOMzg)b$jT|eFp?fx4+?kn8D
z)hQA>>}H0rNjaV6FnO1Hju+f+ryZ=4%HD
zfOrbP{3t;wWWap9tiMBDjqmC+eR!@}_9(G!fxj9Fp$t0X8W
zyF7}XH;8_ey#D{eZy&_B`3l!EZFn_O=aI21GJYEwzl)6X#Crk1
zLy>VzY8ZYdGB$-rKDxs1nMgAITjVc1`#1tUZ;TcAf6SQq@ZV468&|H`;GE_xESWZO
z%CzE=0_RgzTbz@MoRcO_noOG}jh{MZ{jxR7s%n?zO%qUC{>=<
Re: [edk2-devel] [PATCH 1/1] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: refine flash detection
On Tue, 2 May 2023 at 07:59, Gerd Hoffmann wrote:
>
> Check whenever flash is actually writable.
>
This is a bit too terse. Could you explain why this is needed, and why
this approach is suitable?
> Signed-off-by: Gerd Hoffmann
> ---
> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c | 14 +++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> index 54f859de9ff9..a577aea55614 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> @@ -114,9 +114,17 @@ QemuFlashDetected (
>DEBUG ((DEBUG_INFO, "QemuFlashDetected => FD behaves as RAM\n"));
>*Ptr = OriginalUint8;
> } else if (ProbeUint8 == CLEARED_ARRAY_STATUS) {
> - DEBUG ((DEBUG_INFO, "QemuFlashDetected => FD behaves as FLASH\n"));
> - FlashDetected = TRUE;
> - *Ptr = READ_ARRAY_CMD;
> + *Ptr = WRITE_BYTE_CMD;
> + *Ptr = OriginalUint8;
> + *Ptr = READ_STATUS_CMD;
> + ProbeUint8 = *Ptr;
> + *Ptr = READ_ARRAY_CMD;
> + if (ProbeUint8 & 0x10 /* programming error */) {
> +DEBUG ((DEBUG_INFO, "QemuFlashDetected => FD behaves as FLASH,
> write-protected\n"));
> + } else {
> +DEBUG ((DEBUG_INFO, "QemuFlashDetected => FD behaves as FLASH,
> writable\n"));
> +FlashDetected = TRUE;
> + }
> }
>}
>
> --
> 2.40.0
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105315): https://edk2.groups.io/g/devel/message/105315
Mute This Topic: https://groups.io/mt/98633328/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 0/5] ArmVirtPkg: Add dynamic CFI flash detection for Kvmtool guests
On Fri, 19 May 2023 at 16:56, Sami Mujawar wrote: > > Kvmtool allows guest VMs to be launched with or without > a CFI flash device. The guest hardware configuration can > be seen in the device tree that Kvmtool hands off to the > guest firmware. > > Therefore, add support to dynamically detect if a CFI > flash device is present. If CFI is present use the > NorFlashDxe driver as the backend for variable services; > otherwise use emulated runtime variables. > > The last patch in this series fix a crash due to stack > overflow which is observed when running the UEFI shell > command 'dmpstore'. > > The first 4 patches in this series have not been modified > and are resent with the v2 series. > > The changes can be seen at: > https://github.com/samimujawar/edk2/tree/2646_dynamic_cfi_detection_v2 > > Sami Mujawar (5): > ArmPkg: Configure PcdEmuVariableNvModeEnable as a dynamic PCD > ArmVirtPkg: Define variables for emulating runtime variables > ArmVirtPkg: Fallback to variable emulation if no CFI is found > ArmVirtPkg: Dispatch variable service if variable emulation is enabled > ArmVirtPkg/PrePi: Allocate separate stack for Dxe phase > Reviewed-by: Ard Biesheuvel > ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 2 +- > ArmVirtPkg/ArmVirtKvmTool.dsc| 11 -- > ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.c | 13 ++- > ArmVirtPkg/KvmtoolPlatformDxe/KvmtoolPlatformDxe.inf | 4 ++- > ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c | 38 > +--- > ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf | 3 +- > ArmVirtPkg/PrePi/PrePi.c | 4 +-- > 7 files changed, 63 insertions(+), 12 deletions(-) > > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105314): https://edk2.groups.io/g/devel/message/105314 Mute This Topic: https://groups.io/mt/99013766/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 0/3] OvmfPkg: gigabyte page tweaks
On Wed, 17 May 2023 at 12:24, Gerd Hoffmann wrote: > > > > Gerd Hoffmann (3): > OvmfPkg/PlatformInitLib: check PcdUse1GPageTable > OvmfPkg/OvmfPkgIa32X64: enable 1G pages > OvmfPkg/MicrovmX64: enable 1G pages > Acked-by: Ard Biesheuvel > OvmfPkg/Microvm/MicrovmX64.dsc | 3 +++ > OvmfPkg/OvmfPkgIa32X64.dsc | 3 +++ > OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf | 1 + > OvmfPkg/Library/PlatformInitLib/MemDetect.c | 5 + > 4 files changed, 12 insertions(+) > > -- > 2.40.1 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105313): https://edk2.groups.io/g/devel/message/105313 Mute This Topic: https://groups.io/mt/98944976/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/1] OvmfPkg/PciHotPlugInitDxe: Do not reserve IO ports by default.
On Tue, 16 May 2023 at 11:48, Gerd Hoffmann wrote: > > Flip the default for IO address space reservations for PCI(e) bridges > and root ports with hotplug support from TRUE to FALSE. > > PCI(e) bridges will still get IO address space assigned in case: > > (a) Downstream devices actually need IO address space, or > (b) Explicit configuration, using "qemu -device > pcie-root-port,io-reserve=". > > In case IO address space is exhausted edk2 will stop assigning resources > to PCI(e) bridges. This is not limited to IO resources, the affected > bridges will not get any memory resources assigned either. > > This patch solves this issue by not handing out the scare IO address > space, which is in most cases not needed anyway. Result is a more > consistent PCI configuration in virtual machine configurations with many > PCie root ports. > > Signed-off-by: Gerd Hoffmann Reviewed-by: Ard Biesheuvel > --- > OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c > b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c > index 6b2b6797b3b6..69903a600981 100644 > --- a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c > +++ b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c > @@ -589,7 +589,7 @@ GetResourcePadding ( > return EFI_INVALID_PARAMETER; >} > > - DefaultIo = TRUE; > + DefaultIo = FALSE; >DefaultMmio = TRUE; >DefaultPrefMmio = TRUE; > > -- > 2.40.1 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105312): https://edk2.groups.io/g/devel/message/105312 Mute This Topic: https://groups.io/mt/98922836/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 5/7] OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists
On Thu, May 25, 2023 at 7:27 AM Ard Biesheuvel wrote:
> On Wed, 24 May 2023 at 20:13, Tuan Phan wrote:
> >
> >
> >
> > On Mon, Mar 6, 2023 at 9:53 AM Ard Biesheuvel wrote:
> >>
> >> On Mon, 6 Mar 2023 at 18:33, Tuan Phan wrote:
> >> >
> >> > The flash base address can be added to GCD before this driver run.
> >> > So only add it if it has not been done.
> >> >
> >>
> >> How do you end up in this situation?
> >>
> >> You cannot skip this registration, as it is required to get the region
> >> marked as EFI_MEMORY_RUNTIME, and without that, EFI variables will be
> >> broken when running under the OS.
> >
> >
> > Ard,
> > The patch only skips AddMemorySpace if it is already done in the early
> SEC phase for RiscV platform.
> > The EFI_MEMORY_RUNTIME always be set in the next line with
> SetMemorySpaceAttributes.
> >
>
> So how does the SEC phase create GCD regions to begin with?
>
> This really sounds like there is a problem elsewhere tbh.
>
The SEC phase just simply adds that region to the memory hob with
BuildResourceDescriptorHob.
Agree, the problem is VariableRuntimeDxe.efi accessing the region before
this VirtNorFlashDxe starts so when
MMU enabled, edk2 will hang due to page fault exception.
>
>
> >>
> >> > Signed-off-by: Tuan Phan
> >> > ---
> >> > OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c | 25
> +++
> >> > 1 file changed, 16 insertions(+), 9 deletions(-)
> >> >
> >> > diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> >> > index f9a41f6aab0f..8875824f 100644
> >> > --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> >> > +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> >> > @@ -372,10 +372,11 @@ NorFlashFvbInitialize (
> >> >IN NOR_FLASH_INSTANCE *Instance
> >> >)
> >> > {
> >> > - EFI_STATUS Status;
> >> > - UINT32 FvbNumLba;
> >> > - EFI_BOOT_MODE BootMode;
> >> > - UINTN RuntimeMmioRegionSize;
> >> > + EFI_STATUS Status;
> >> > + UINT32 FvbNumLba;
> >> > + EFI_BOOT_MODE BootMode;
> >> > + UINTN RuntimeMmioRegionSize;
> >> > + EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
> >> >
> >> >DEBUG ((DEBUG_BLKIO, "NorFlashFvbInitialize\n"));
> >> >ASSERT ((Instance != NULL));
> >> > @@ -390,13 +391,19 @@ NorFlashFvbInitialize (
> >> >// is written as the base of the flash region (ie:
> Instance->DeviceBaseAddress)
> >> >RuntimeMmioRegionSize = (Instance->RegionBaseAddress -
> Instance->DeviceBaseAddress) + Instance->Size;
> >> >
> >> > - Status = gDS->AddMemorySpace (
> >> > - EfiGcdMemoryTypeMemoryMappedIo,
> >> > + Status = gDS->GetMemorySpaceDescriptor (
> >> >Instance->DeviceBaseAddress,
> >> > - RuntimeMmioRegionSize,
> >> > - EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> >> > +
> >> >);
> >> > - ASSERT_EFI_ERROR (Status);
> >> > + if (Status == EFI_NOT_FOUND) {
> >> > +Status = gDS->AddMemorySpace (
> >> > +EfiGcdMemoryTypeMemoryMappedIo,
> >> > +Instance->DeviceBaseAddress,
> >> > +RuntimeMmioRegionSize,
> >> > +EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> >> > +);
> >> > +ASSERT_EFI_ERROR (Status);
> >> > + }
> >> >
> >> >Status = gDS->SetMemorySpaceAttributes (
> >> >Instance->DeviceBaseAddress,
> >> > --
> >> > 2.25.1
> >> >
> >> >
> >> >
> >> >
> >> > Groups.io Links: You receive all messages sent to this group.
> >> > View/Reply Online (#100754):
> https://edk2.groups.io/g/devel/message/100754
> >> > Mute This Topic: https://groups.io/mt/97430554/1131722
> >> > Group Owner: devel+ow...@edk2.groups.io
> >> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [a...@kernel.org]
> >> >
> >> >
> >> >
> >
> >
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105311): https://edk2.groups.io/g/devel/message/105311
Mute This Topic: https://groups.io/mt/97430554/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX
If the associated PCD is set to TRUE, use the memory attribute PPI to
remap the stack non-executable. This provides a generic method for doing
so, which will be used by ARM and AArch64 as well once they move to the
generic DxeIpl handoff implementation.
Signed-off-by: Ard Biesheuvel
---
MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c | 29 ++--
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 5 +++-
2 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
index a0f85ebea56e6cba..22caabb02840ba88 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
@@ -2,12 +2,15 @@
Generic version of arch-specific functionality for DxeLoad.
Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
+Copyright (c) 2023, Google, LLC. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
#include "DxeIpl.h"
+#include
+
/**
Transfers control to DxeCore.
@@ -25,9 +28,10 @@ HandOffToDxeCore (
IN EFI_PEI_HOB_POINTERS HobList
)
{
- VOID*BaseOfStack;
- VOID*TopOfStack;
- EFI_STATUS Status;
+ VOID*BaseOfStack;
+ VOID*TopOfStack;
+ EFI_STATUS Status;
+ EDKII_MEMORY_ATTRIBUTE_PPI *MemoryPpi;
//
// Allocate 128KB for the Stack
@@ -35,6 +39,25 @@ HandOffToDxeCore (
BaseOfStack = AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));
ASSERT (BaseOfStack != NULL);
+ if (PcdGetBool (PcdSetNxForStack)) {
+Status = PeiServicesLocatePpi (
+ ,
+ 0,
+ NULL,
+ (VOID **)
+ );
+ASSERT_EFI_ERROR (Status);
+
+Status = MemoryPpi->SetPermissions (
+ MemoryPpi,
+ (UINTN)BaseOfStack,
+ STACK_SIZE,
+ EFI_MEMORY_XP,
+ 0
+ );
+ASSERT_EFI_ERROR (Status);
+ }
+
//
// Compute the top of the stack we were allocated. Pre-allocate a UINTN
// for safety.
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 60c998be6c1bad01..7126a96d8378d1f8 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -91,6 +91,7 @@ [Ppis]
gEfiPeiMemoryDiscoveredPpiGuid ## SOMETIMES_CONSUMES
gEdkiiPeiBootInCapsuleOnDiskModePpiGuid ## SOMETIMES_CONSUMES
gEdkiiPeiCapsuleOnDiskPpiGuid## SOMETIMES_CONSUMES # Consumed on
firmware update boot path
+ gEdkiiMemoryAttributePpiGuid ## SOMETIMES_CONSUMES
[Guids]
## SOMETIMES_CONSUMES ## Variable:L"MemoryTypeInformation"
@@ -117,10 +118,12 @@ [Pcd.IA32,Pcd.X64]
gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize##
CONSUMES
[Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
- gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ##
SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ##
SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ##
SOMETIMES_CONSUMES
+[Pcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ##
SOMETIMES_CONSUMES
+
[Depex]
gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105309): https://edk2.groups.io/g/devel/message/105309
Mute This Topic: https://groups.io/mt/99131196/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [RFC PATCH 10/10] MdeModulePkg/DxeIpl ARM AARCH64: Switch to generic handoff code
Now that we have a generic method to manage memory permissions using a
PPI, we can switch to the generic version of the DXE handoff code in
DxeIpl, and drop the ARM specific version.
Signed-off-by: Ard Biesheuvel
---
MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c | 71
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf| 11 +--
2 files changed, 1 insertion(+), 81 deletions(-)
diff --git a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
b/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
deleted file mode 100644
index f62b6dcb38a702d7..
--- a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
+++ /dev/null
@@ -1,71 +0,0 @@
-/** @file
- ARM specifc functionality for DxeLoad.
-
-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
-Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
-
-SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#include "DxeIpl.h"
-
-#include
-
-/**
- Transfers control to DxeCore.
-
- This function performs a CPU architecture specific operations to execute
- the entry point of DxeCore with the parameters of HobList.
- It also installs EFI_END_OF_PEI_PPI to signal the end of PEI phase.
-
- @param DxeCoreEntryPoint The entry point of DxeCore.
- @param HobList The start of HobList passed to DxeCore.
-
-**/
-VOID
-HandOffToDxeCore (
- IN EFI_PHYSICAL_ADDRESS DxeCoreEntryPoint,
- IN EFI_PEI_HOB_POINTERS HobList
- )
-{
- VOID*BaseOfStack;
- VOID*TopOfStack;
- EFI_STATUS Status;
-
- //
- // Allocate 128KB for the Stack
- //
- BaseOfStack = AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));
- ASSERT (BaseOfStack != NULL);
-
- if (PcdGetBool (PcdSetNxForStack)) {
-Status = ArmSetMemoryRegionNoExec ((UINTN)BaseOfStack, STACK_SIZE);
-ASSERT_EFI_ERROR (Status);
- }
-
- //
- // Compute the top of the stack we were allocated. Pre-allocate a UINTN
- // for safety.
- //
- TopOfStack = (VOID *)((UINTN)BaseOfStack + EFI_SIZE_TO_PAGES (STACK_SIZE) *
EFI_PAGE_SIZE - CPU_STACK_ALIGNMENT);
- TopOfStack = ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);
-
- //
- // End of PEI phase singal
- //
- Status = PeiServicesInstallPpi ();
- ASSERT_EFI_ERROR (Status);
-
- //
- // Update the contents of BSP stack HOB to reflect the real stack info
passed to DxeCore.
- //
- UpdateStackHob ((EFI_PHYSICAL_ADDRESS)(UINTN)BaseOfStack, STACK_SIZE);
-
- SwitchStack (
-(SWITCH_STACK_ENTRY_POINT)(UINTN)DxeCoreEntryPoint,
-HobList.Raw,
-NULL,
-TopOfStack
-);
-}
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 7126a96d8378d1f8..f1990eac77607854 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -45,19 +45,13 @@ [Sources.X64]
X64/VirtualMemory.c
X64/DxeLoadFunc.c
-[Sources.ARM, Sources.AARCH64]
- Arm/DxeLoadFunc.c
-
-[Sources.LOONGARCH64,Sources.RISCV64,Sources.EBC]
+[Sources.LOONGARCH64,Sources.RISCV64,Sources.EBC,Sources.ARM,Sources.AARCH64]
DxeHandoff.c
[Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
-[Packages.ARM, Packages.AARCH64]
- ArmPkg/ArmPkg.dec
-
[LibraryClasses]
PcdLib
MemoryAllocationLib
@@ -74,9 +68,6 @@ [LibraryClasses]
PeiServicesTablePointerLib
PerformanceLib
-[LibraryClasses.ARM, LibraryClasses.AARCH64]
- ArmMmuLib
-
[Ppis]
gEfiDxeIplPpiGuid ## PRODUCES
gEfiPeiDecompressPpiGuid ## PRODUCES
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105310): https://edk2.groups.io/g/devel/message/105310
Mute This Topic: https://groups.io/mt/99131200/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [RFC PATCH 08/10] MdeModulePkg/DxeIpl: Merge EBC, RISCV64 and LOONGARCH code
The Risc-V and LoongArch specific versions of the DXE core handoff code
in DxeIpl are essentially copies of the EBC version (modulo the
copyright in the header and some debug prints in the code).
In preparation for introducing a generic PPI based method to implement
the non-executable stack, let's merge these versions, so we only need to
add this logic once.
Signed-off-by: Ard Biesheuvel
---
MdeModulePkg/Core/DxeIplPeim/{Ebc/DxeLoadFunc.c => DxeHandoff.c} | 2 +-
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 10 +--
MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c | 63
MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c | 75
4 files changed, 3 insertions(+), 147 deletions(-)
diff --git a/MdeModulePkg/Core/DxeIplPeim/Ebc/DxeLoadFunc.c
b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
similarity index 92%
rename from MdeModulePkg/Core/DxeIplPeim/Ebc/DxeLoadFunc.c
rename to MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
index c1a16b602452218e..a0f85ebea56e6cba 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Ebc/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
@@ -1,5 +1,5 @@
/** @file
- EBC-specific functionality for DxeLoad.
+ Generic version of arch-specific functionality for DxeLoad.
Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 052ea0ec1a6f2771..60c998be6c1bad01 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -45,17 +45,11 @@ [Sources.X64]
X64/VirtualMemory.c
X64/DxeLoadFunc.c
-[Sources.EBC]
- Ebc/DxeLoadFunc.c
-
[Sources.ARM, Sources.AARCH64]
Arm/DxeLoadFunc.c
-[Sources.RISCV64]
- RiscV64/DxeLoadFunc.c
-
-[Sources.LOONGARCH64]
- LoongArch64/DxeLoadFunc.c
+[Sources.LOONGARCH64,Sources.RISCV64,Sources.EBC]
+ DxeHandoff.c
[Packages]
MdePkg/MdePkg.dec
diff --git a/MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c
b/MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c
deleted file mode 100644
index 95d3af19ea4c9f00..
--- a/MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c
+++ /dev/null
@@ -1,63 +0,0 @@
-/** @file
- LoongArch specifc functionality for DxeLoad.
-
- Copyright (c) 2022, Loongson Technology Corporation Limited. All rights
reserved.
-
- SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#include "DxeIpl.h"
-
-/**
- Transfers control to DxeCore.
-
- This function performs a CPU architecture specific operations to execute
- the entry point of DxeCore with the parameters of HobList.
- It also installs EFI_END_OF_PEI_PPI to signal the end of PEI phase.
-
- @param[in] DxeCoreEntryPoint The entry point of DxeCore.
- @param[in] HobList The start of HobList passed to DxeCore.
-
-**/
-VOID
-HandOffToDxeCore (
- IN EFI_PHYSICAL_ADDRESS DxeCoreEntryPoint,
- IN EFI_PEI_HOB_POINTERS HobList
- )
-{
- VOID*BaseOfStack;
- VOID*TopOfStack;
- EFI_STATUS Status;
-
- //
- // Allocate 128KB for the Stack
- //
- BaseOfStack = AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));
- ASSERT (BaseOfStack != NULL);
-
- //
- // Compute the top of the stack we were allocated. Pre-allocate a UINTN
- // for safety.
- //
- TopOfStack = (VOID *)((UINTN)BaseOfStack + EFI_SIZE_TO_PAGES (STACK_SIZE) *
EFI_PAGE_SIZE - CPU_STACK_ALIGNMENT);
- TopOfStack = ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);
-
- //
- // End of PEI phase signal
- //
- Status = PeiServicesInstallPpi ();
- ASSERT_EFI_ERROR (Status);
-
- //
- // Update the contents of BSP stack HOB to reflect the real stack info
passed to DxeCore.
- //
- UpdateStackHob ((EFI_PHYSICAL_ADDRESS)(UINTN)BaseOfStack, STACK_SIZE);
-
- SwitchStack (
-(SWITCH_STACK_ENTRY_POINT)(UINTN)DxeCoreEntryPoint,
-HobList.Raw,
-NULL,
-TopOfStack
-);
-}
diff --git a/MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c
b/MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c
deleted file mode 100644
index b3567d88f73467e7..
--- a/MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c
+++ /dev/null
@@ -1,75 +0,0 @@
-/** @file
- RISC-V specific functionality for DxeLoad.
-
- Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights
reserved.
-
- SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#include "DxeIpl.h"
-
-/**
- Transfers control to DxeCore.
-
- This function performs a CPU architecture specific operations to execute
- the entry point of DxeCore with the parameters of HobList.
- It also installs EFI_END_OF_PEI_PPI to signal the end of PEI phase.
-
- @param DxeCoreEntryPoint The entry point of DxeCore.
- @param HobList The start of HobList passed to DxeCore.
-
-**/
-VOID
-HandOffToDxeCore (
- IN EFI_PHYSICAL_ADDRESS DxeCoreEntryPoint,
- IN
[edk2-devel] [RFC PATCH 07/10] MdeModulePkg/PeiCore: Apply restricted permissions in image loader
Add a notification callback to the PEI core to grab a reference to the
memory attributes PPI as soon as it is registered, and use it in the
image loader to set restricted memory permissions after loading the
image if the image was loaded into memory.
There are two use cases for this:
- when the DXE IPL loads the DXE core using the PEI image services, its
mappings will be set according to the PE section permission attributes
if the image was built with 4k section alignment; this means DXE core
will never run with mappings that are both writable and executable.
- when PEIMs are shadowed to memory, they will not only be mapped
read-only, but any non-exec permissions will also be removed. (Note
that this requires the component that installs PEI permanent memory to
depex on the memory attributes PPI, to ensure that it is available to
manage permissions on permanent memory before it is used to load
images)
With this logic in place *, there is no longer a need for system memory
to be mapped with both write and execute permissions out of reset.
Instead, all memory can be mapped with non-executable permissions by
default, which means that the stack and other allocations used in PEI or
early in DXE will no longer need to be mapped non-exec explicitly.
* the DXE core will also need its own method for clearing non-exec
permissions on memory ranges, but this is being addressed in a
separate series.
Signed-off-by: Ard Biesheuvel
---
MdeModulePkg/Core/Pei/Image/Image.c | 160
MdeModulePkg/Core/Pei/PeiMain.h | 6 +
MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
3 files changed, 167 insertions(+)
diff --git a/MdeModulePkg/Core/Pei/Image/Image.c
b/MdeModulePkg/Core/Pei/Image/Image.c
index cee9f09c6ea61e31..3a7de45014b8f772 100644
--- a/MdeModulePkg/Core/Pei/Image/Image.c
+++ b/MdeModulePkg/Core/Pei/Image/Image.c
@@ -18,6 +18,50 @@ EFI_PEI_PPI_DESCRIPTOR gPpiLoadFilePpiList = {
};
+/**
+ Provide a callback for when the memory attributes PPI is installed.
+
+ @param PeiServicesAn indirect pointer to the EFI_PEI_SERVICES table
+published by the PEI Foundation.
+ @param NotifyDescriptor The descriptor for the notification event.
+ @param PpiPointer to the PPI in question.
+
+ @return Always success
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+MemoryAttributePpiNotifyCallback (
+ IN EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor,
+ IN VOID *Ppi
+ )
+{
+ PEI_CORE_INSTANCE *PrivateData;
+
+ //
+ // Get PEI Core private data
+ //
+ PrivateData = PEI_CORE_INSTANCE_FROM_PS_THIS (PeiServices);
+
+ //
+ // If there isn't a memory attribute PPI installed, use the one from
+ // notification
+ //
+ if (PrivateData->MemoryAttributePpi == NULL) {
+PrivateData->MemoryAttributePpi = (EDKII_MEMORY_ATTRIBUTE_PPI *)Ppi;
+ }
+
+ return EFI_SUCCESS;
+}
+
+STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mNotifyList = {
+ EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH |
EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
+ ,
+ MemoryAttributePpiNotifyCallback
+};
+
/**
Support routine for the PE/COFF Loader that reads a buffer from a PE/COFF
file.
@@ -243,6 +287,106 @@ GetPeCoffImageFixLoadingAssignedAddress (
return Status;
}
+/**
+ Remap the memory region covering a loaded image so it can be executed.
+
+ @param ImageContextPointer to the image context structure that describes
the
+ PE/COFF image that needs to be examined by this
function.
+ @param FileTypeThe FFS file type of the image
+ @param ImageAddressThe start of the memory region covering the image
+ @param ImageSize The size of the memory region covering the image
+
+ @retval EFI_SUCCESS The image is ready to be executed
+ @retval EFI_OUT_OF_RESOURCES Not enough memory available to update the
memory
+mapping
+
+**/
+STATIC
+EFI_STATUS
+RemapLoadedImageForExecution (
+ IN CONST PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext,
+ IN EFI_FV_FILETYPE FileType,
+ IN PHYSICAL_ADDRESSImageAddress,
+ IN UINT64 ImageSize
+ )
+{
+ PEI_CORE_INSTANCE*Private;
+ EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr;
+ EFI_IMAGE_SECTION_HEADER *Section;
+ PHYSICAL_ADDRESS SectionAddress;
+ EFI_STATUS Status;
+ UINT64 Permissions;
+ UINTNIndex;
+
+ Private = PEI_CORE_INSTANCE_FROM_PS_THIS (GetPeiServicesTablePointer ());
+
+ if (Private->MemoryAttributePpi == NULL) {
+return EFI_SUCCESS;
+ }
+
+ //
+ // PEI phase executables must be able to execute in place from read-only NOR
+ // flash, and so they can be mapped read-only in their entirety.
+ //
+ if ((FileType == EFI_FV_FILETYPE_PEI_CORE) ||
+
[edk2-devel] [RFC PATCH 06/10] ArmPkg/CpuPei: Implement the memory attributes PPI
Implement the newly defined PPI that permits the PEI core and DXE IPL to
manage memory permissions on ranges of DRAM, for doing things like
mapping the stack non-executable, or granting executable permissions to
shadowed PEIMs.
Signed-off-by: Ard Biesheuvel
---
ArmPkg/Drivers/CpuPei/CpuPei.c | 78 ++--
ArmPkg/Drivers/CpuPei/CpuPei.inf | 4 +
2 files changed, 74 insertions(+), 8 deletions(-)
diff --git a/ArmPkg/Drivers/CpuPei/CpuPei.c b/ArmPkg/Drivers/CpuPei/CpuPei.c
index 85ef5ec07b9fdafa..d5996673260544c8 100644
--- a/ArmPkg/Drivers/CpuPei/CpuPei.c
+++ b/ArmPkg/Drivers/CpuPei/CpuPei.c
@@ -3,17 +3,10 @@
Copyright (c) 2006, Intel Corporation. All rights reserved.
Copyright (c) 2011 Hewlett Packard Corporation. All rights reserved.
Copyright (c) 2011-2013, ARM Limited. All rights reserved.
+Copyright (c) 2023, Google, LLC. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
-Module Name:
-
- MemoryInit.c
-
-Abstract:
-
- PEIM to provide fake memory init
-
**/
//
@@ -24,6 +17,7 @@ Module Name:
// The protocols, PPI and GUID definitions for this module
//
#include
+#include
//
// The Library classes this module consumes
@@ -34,6 +28,71 @@ Module Name:
#include
#include
#include
+#include
+
+/**
+ 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] ThisThe 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.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+SetMemoryPermissions (
+ IN EDKII_MEMORY_ATTRIBUTE_PPI *This,
+ IN EFI_PHYSICAL_ADDRESSBaseAddress,
+ IN UINT64 Length,
+ IN UINT64 SetMask,
+ IN UINT64 ClearMask
+ )
+{
+ if ((Length == 0) ||
+ ((SetMask & ClearMask) != 0) ||
+ (((SetMask | ClearMask) & (EFI_MEMORY_RP | EFI_MEMORY_RO |
EFI_MEMORY_XP)) == 0) ||
+ (((SetMask | ClearMask) & ~(UINT64)(EFI_MEMORY_RP | EFI_MEMORY_RO |
EFI_MEMORY_XP)) != 0) ||
+ (((BaseAddress | Length) & EFI_PAGE_MASK) != 0))
+ {
+return EFI_INVALID_PARAMETER;
+ }
+
+ return ArmSetMemoryAttributes (BaseAddress, Length, SetMask, SetMask |
ClearMask);
+}
+
+STATIC CONST EDKII_MEMORY_ATTRIBUTE_PPI mMemoryAttributePpi = {
+ SetMemoryPermissions
+};
+
+STATIC CONST EFI_PEI_PPI_DESCRIPTOR mMemoryAttributePpiDesc = {
+ (EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
+ ,
+ (VOID *)
+};
/*++
@@ -79,5 +138,8 @@ InitializeCpuPeim (
}
}
+ Status = PeiServicesInstallPpi ();
+ ASSERT_EFI_ERROR (Status);
+
return EFI_SUCCESS;
}
diff --git a/ArmPkg/Drivers/CpuPei/CpuPei.inf b/ArmPkg/Drivers/CpuPei/CpuPei.inf
index 648f27adf9402435..2ca4f795c62de394 100644
--- a/ArmPkg/Drivers/CpuPei/CpuPei.inf
+++ b/ArmPkg/Drivers/CpuPei/CpuPei.inf
@@ -3,6 +3,7 @@
#
# This module provides platform specific function to detect boot mode.
# Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.
+# Copyright (c) 2023, Google, LLC. All rights reserved.
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
@@ -28,6 +29,7 @@ [Sources]
CpuPei.c
[Packages]
+ MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
EmbeddedPkg/EmbeddedPkg.dec
ArmPkg/ArmPkg.dec
@@ -37,9 +39,11 @@ [LibraryClasses]
DebugLib
HobLib
ArmLib
+ ArmMmuLib
[Ppis]
gArmMpCoreInfoPpiGuid
+ gEdkiiMemoryAttributePpiGuid
[Guids]
gArmMpCoreInfoGuid
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent
[edk2-devel] [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 ..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] ThisThe 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_ADDRESSBaseAddress,
+ 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_PERMISSIONSSetPermissions;
+};
+
+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 (#105305): https://edk2.groups.io/g/devel/message/105305
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]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [RFC PATCH 04/10] OvmfPkg/RiscVVirt: Remove unimplemented NxForStack configuration
The RISC-V version of the DXE IPL does not implement setting the stack
NX, so before switching to an implementation that will ASSERT() on the
missing support, drop the PCD setting that enables it.
Signed-off-by: Ard Biesheuvel
---
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 6 --
1 file changed, 6 deletions(-)
diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
index 28d9af4d79b9cc35..414d186179fb16e8 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
+++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
@@ -172,12 +172,6 @@ [PcdsFixedAtBuild.common]
gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa, 0x2c,
0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23, 0x31 }
-
- #
- # Enable the non-executable DXE stack. (This gets set up by DxeIpl)
- #
- gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
-
!if $(SECURE_BOOT_ENABLE) == TRUE
# override the default values from SecurityPkg to ensure images from all
sources are verified in secure boot
gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x04
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105304): https://edk2.groups.io/g/devel/message/105304
Mute This Topic: https://groups.io/mt/99131182/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [RFC PATCH 03/10] ArmPkg/CpuPei: Drop bogus DEPEX on PEI permanent memory
Currently, ARM's CPU PEIM depexes on PEI permanent memory being installed, but functionally, it does not actually depend on that at all. So let's drop the DEPEX. Signed-off-by: Ard Biesheuvel --- ArmPkg/Drivers/CpuPei/CpuPei.inf | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ArmPkg/Drivers/CpuPei/CpuPei.inf b/ArmPkg/Drivers/CpuPei/CpuPei.inf index a9f85cbc68b1c52e..648f27adf9402435 100644 --- a/ArmPkg/Drivers/CpuPei/CpuPei.inf +++ b/ArmPkg/Drivers/CpuPei/CpuPei.inf @@ -48,5 +48,4 @@ [FixedPcd] gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize [Depex] - gEfiPeiMemoryDiscoveredPpiGuid - + TRUE -- 2.39.2 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105303): https://edk2.groups.io/g/devel/message/105303 Mute This Topic: https://groups.io/mt/99131179/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [RFC PATCH 02/10] ArmPkg/CpuDxe: Simplify memory attributes protocol implementation
Now that ArmSetMemoryAttributes() permits a mask to be provided, we can
simplify the implementation the UEFI memory attribute protocol
substantially, and just pass on the requested mask to be set or cleared
directly.
Signed-off-by: Ard Biesheuvel
---
ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 50 +---
1 file changed, 2 insertions(+), 48 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
index 61ba8fbbae4ee795..16cc4ef474f9772b 100644
--- a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
+++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
@@ -183,8 +183,6 @@ SetMemoryAttributes (
IN UINT64 Attributes
)
{
- EFI_STATUS Status;
-
DEBUG ((
DEBUG_INFO,
"%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
@@ -204,28 +202,7 @@ SetMemoryAttributes (
return EFI_UNSUPPORTED;
}
- if ((Attributes & EFI_MEMORY_RP) != 0) {
-Status = ArmSetMemoryRegionNoAccess (BaseAddress, Length);
-if (EFI_ERROR (Status)) {
- return EFI_UNSUPPORTED;
-}
- }
-
- if ((Attributes & EFI_MEMORY_RO) != 0) {
-Status = ArmSetMemoryRegionReadOnly (BaseAddress, Length);
-if (EFI_ERROR (Status)) {
- return EFI_UNSUPPORTED;
-}
- }
-
- if ((Attributes & EFI_MEMORY_XP) != 0) {
-Status = ArmSetMemoryRegionNoExec (BaseAddress, Length);
-if (EFI_ERROR (Status)) {
- return EFI_UNSUPPORTED;
-}
- }
-
- return EFI_SUCCESS;
+ return ArmSetMemoryAttributes (BaseAddress, Length, Attributes, Attributes);
}
/**
@@ -267,8 +244,6 @@ ClearMemoryAttributes (
IN UINT64 Attributes
)
{
- EFI_STATUS Status;
-
DEBUG ((
DEBUG_INFO,
"%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
@@ -288,28 +263,7 @@ ClearMemoryAttributes (
return EFI_UNSUPPORTED;
}
- if ((Attributes & EFI_MEMORY_RP) != 0) {
-Status = ArmClearMemoryRegionNoAccess (BaseAddress, Length);
-if (EFI_ERROR (Status)) {
- return EFI_UNSUPPORTED;
-}
- }
-
- if ((Attributes & EFI_MEMORY_RO) != 0) {
-Status = ArmClearMemoryRegionReadOnly (BaseAddress, Length);
-if (EFI_ERROR (Status)) {
- return EFI_UNSUPPORTED;
-}
- }
-
- if ((Attributes & EFI_MEMORY_XP) != 0) {
-Status = ArmClearMemoryRegionNoExec (BaseAddress, Length);
-if (EFI_ERROR (Status)) {
- return EFI_UNSUPPORTED;
-}
- }
-
- return EFI_SUCCESS;
+ return ArmSetMemoryAttributes (BaseAddress, Length, 0, Attributes);
}
EFI_MEMORY_ATTRIBUTE_PROTOCOL mMemoryAttribute = {
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105302): https://edk2.groups.io/g/devel/message/105302
Mute This Topic: https://groups.io/mt/99131176/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [RFC PATCH 01/10] ArmPkg/ArmMmuLib: Extend API to manage memory permissions better
Currently, ArmSetMemoryAttributes () takes a combination of
EFI_MEMORY_xx constants describing the memory type and permission
attributes that should be set on a region of memory. In cases where the
memory type is omitted, we assume that the memory permissions being set
are final, and that existing memory permissions can be discarded.
This is problematic, because we aim to map memory non-executable
(EFI_MEMORY_XP) by default, and only relax this requirement for code
regions that are mapped read-only (EFI_MEMORY_RO). Currently, setting
one permission clears the other, and so code managing these permissions
has to be aware of the existing permissions in order to be able to
preserve them, and this is not always tractable (e.g., the UEFI memory
attribute protocol implements an abstraction that promises to preserve
memory permissions that it is not operating on explicitly).
So let's add an AttributeMask parameter to ArmSetMemoryAttributes(),
which is permitted to be non-zero if no memory type is being provided,
in which case only memory permission attributes covered in the mask will
be affected by the update.
Signed-off-by: Ard Biesheuvel
---
ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 2 +-
ArmPkg/Include/Library/ArmMmuLib.h | 36 +++-
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 52 +++-
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c | 88 +---
ArmPkg/Library/OpteeLib/Optee.c | 2 +-
5 files changed, 165 insertions(+), 15 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
index 2e73719dce04ceb5..2d60c7d24dc05ee9 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
@@ -217,7 +217,7 @@ CpuSetMemoryAttributes (
if (EFI_ERROR (Status) || (RegionArmAttributes != ArmAttributes) ||
((BaseAddress + Length) > (RegionBaseAddress + RegionLength)))
{
-return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes);
+return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes, 0);
} else {
return EFI_SUCCESS;
}
diff --git a/ArmPkg/Include/Library/ArmMmuLib.h
b/ArmPkg/Include/Library/ArmMmuLib.h
index 4cf59a1e376b123c..91d112314fdf4859 100644
--- a/ArmPkg/Include/Library/ArmMmuLib.h
+++ b/ArmPkg/Include/Library/ArmMmuLib.h
@@ -92,11 +92,45 @@ ArmReplaceLiveTranslationEntry (
IN BOOLEAN DisableMmu
);
+/**
+ Set the requested memory permission attributes on a region of memory.
+
+ BaseAddress and Length must be aligned to EFI_PAGE_SIZE.
+
+ If Attributes contains a memory type attribute (EFI_MEMORY_UC/WC/WT/WB), the
+ region is mapped according to this memory type, and additional memory
+ permission attributes (EFI_MEMORY_RP/RO/XP) are taken into account as well,
+ discarding any permission attributes that are currently set for the region.
+ AttributeMask is ignored in this case, and must be set to 0x0.
+
+ If Attributes contains only a combination of memory permission attributes
+ (EFI_MEMORY_RP/RO/XP), each page in the region will retain its existing
+ memory type, even if it is not uniformly set across the region. In this case,
+ AttributesMask may be set to a mask of permission attributes, and memory
+ permissions omitted from this mask will not be updated for any page in the
+ region. All attributes appearing in Attributes must appear in AttributeMask
+ as well. (Attributes & ~AttributeMask must produce 0x0)
+
+ @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] Attributes Mask of memory attributes to set.
+ @param[in] AttributeMask Mask of memory attributes to take into account.
+
+ @retval EFI_SUCCESS The attributes were set for the memory region.
+ @retval EFI_INVALID_PARAMETER BaseAddress or Length is not suitably aligned.
+Invalid combination of Attributes and
+AttributeMask.
+ @retval EFI_OUT_OF_RESOURCES Requested attributes cannot be applied due to
+lack of system resources.
+
+**/
EFI_STATUS
ArmSetMemoryAttributes (
IN EFI_PHYSICAL_ADDRESS BaseAddress,
IN UINT64Length,
- IN UINT64Attributes
+ IN UINT64Attributes,
+ IN UINT64AttributeMask
);
#endif // ARM_MMU_LIB_H_
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 7ed758fbbc699732..22623572b9cb931c 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -469,11 +469,45 @@ GcdAttributeToPageAttribute (
return PageAttributes;
}
+/**
+ Set the requested memory permission attributes on a region of memory.
+
+ BaseAddress and Length
[edk2-devel] [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4468
This is a proof-of-concept RFC that implements a PEI phase PPI to manage
memory permission attributes, and wires it up to the PEI image loader so
that shadowed PEIMs as well as the DXE core are remapped with the
appropriate, restricted memory permission attributes before execution.
This means that neither shadowed PEIMs nor the DXE core will ever
execute with writable code regions. It also removes the need on the part
of PEI for memory to be mapped with both writable and executable
permissions by default out of reset. Similar work still needs to be done
to address the early DXE phase (before the CPU arch protocol becomes
available), but once that is out of the way as well, platforms should be
able to map all memory non-executable from the beginning.
This by itself is a major improvement in terms of robustness. It is also
a prerequisite for enabling the WXN MMU control on AArch64, which makes
all writable memory mappings non-executable regardless of the non-exec
page table attribute.
Patches #1 to #4 are prepatory work.
Patch #5 proposes the memory attribute PPI protocol interface.
Patch #6 implements it for ARM and AARCH64.
Patch #7 wires it up into the PEI image loader.
Patches #8 to #10 update the DxeIpl to use this PPI on ARM/AARCH64 for
mapping the stack NX.
instead of an explicit reference to ArmMmuLib. Other architectures
(except IA32/X64) will seamlessly inherit this once they implement the
PPI as well.
Cc: Ray Ni
Cc: Jiewen Yao
Cc: Gerd Hoffmann
Cc: Taylor Beebe
Cc: Oliver Smith-Denny
Cc: Dandan Bi
Cc: Liming Gao
Cc: "Kinney, Michael D"
Cc: Leif Lindholm
Cc: Sunil V L
Cc: Andrei Warkentin
Ard Biesheuvel (10):
ArmPkg/ArmMmuLib: Extend API to manage memory permissions better
ArmPkg/CpuDxe: Simplify memory attributes protocol implementation
ArmPkg/CpuPei: Drop bogus DEPEX on PEI permanent memory
OvmfPkg/RiscVVirt: Remove unimplemented NxForStack configuration
MdeModulePkg: Define memory attribute PPI
ArmPkg/CpuPei: Implement the memory attributes PPI
MdeModulePkg/PeiCore: Apply restricted permissions in image loader
MdeModulePkg/DxeIpl: Merge EBC, RISCV64 and LOONGARCH code
MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX
MdeModulePkg/DxeIpl ARM AARCH64: Switch to generic handoff code
ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 2 +-
ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 50 +-
ArmPkg/Drivers/CpuPei/CpuPei.c | 78
+-
ArmPkg/Drivers/CpuPei/CpuPei.inf | 7 +-
ArmPkg/Include/Library/ArmMmuLib.h | 36 -
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 52 ++-
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c | 88
+--
ArmPkg/Library/OpteeLib/Optee.c | 2 +-
MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c | 71
-
MdeModulePkg/Core/DxeIplPeim/{Ebc/DxeLoadFunc.c => DxeHandoff.c} | 31 +++-
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 24 +--
MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c | 63
MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c | 75
-
MdeModulePkg/Core/Pei/Image/Image.c | 160
MdeModulePkg/Core/Pei/PeiMain.h | 6 +
MdeModulePkg/Core/Pei/PeiMain.inf| 1 +
MdeModulePkg/Include/Ppi/MemoryAttribute.h | 78
++
MdeModulePkg/MdeModulePkg.dec| 3 +
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 6 -
19 files changed, 523 insertions(+), 310 deletions(-)
delete mode 100644 MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
rename MdeModulePkg/Core/DxeIplPeim/{Ebc/DxeLoadFunc.c => DxeHandoff.c} (62%)
delete mode 100644 MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c
delete mode 100644 MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c
create mode 100644 MdeModulePkg/Include/Ppi/MemoryAttribute.h
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105300): https://edk2.groups.io/g/devel/message/105300
Mute This Topic: https://groups.io/mt/99131172/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 5/7] OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists
On Wed, 24 May 2023 at 20:13, Tuan Phan wrote:
>
>
>
> On Mon, Mar 6, 2023 at 9:53 AM Ard Biesheuvel wrote:
>>
>> On Mon, 6 Mar 2023 at 18:33, Tuan Phan wrote:
>> >
>> > The flash base address can be added to GCD before this driver run.
>> > So only add it if it has not been done.
>> >
>>
>> How do you end up in this situation?
>>
>> You cannot skip this registration, as it is required to get the region
>> marked as EFI_MEMORY_RUNTIME, and without that, EFI variables will be
>> broken when running under the OS.
>
>
> Ard,
> The patch only skips AddMemorySpace if it is already done in the early SEC
> phase for RiscV platform.
> The EFI_MEMORY_RUNTIME always be set in the next line with
> SetMemorySpaceAttributes.
>
So how does the SEC phase create GCD regions to begin with?
This really sounds like there is a problem elsewhere tbh.
>>
>> > Signed-off-by: Tuan Phan
>> > ---
>> > OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c | 25 +++
>> > 1 file changed, 16 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
>> > b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
>> > index f9a41f6aab0f..8875824f 100644
>> > --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
>> > +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
>> > @@ -372,10 +372,11 @@ NorFlashFvbInitialize (
>> >IN NOR_FLASH_INSTANCE *Instance
>> >)
>> > {
>> > - EFI_STATUS Status;
>> > - UINT32 FvbNumLba;
>> > - EFI_BOOT_MODE BootMode;
>> > - UINTN RuntimeMmioRegionSize;
>> > + EFI_STATUS Status;
>> > + UINT32 FvbNumLba;
>> > + EFI_BOOT_MODE BootMode;
>> > + UINTN RuntimeMmioRegionSize;
>> > + EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
>> >
>> >DEBUG ((DEBUG_BLKIO, "NorFlashFvbInitialize\n"));
>> >ASSERT ((Instance != NULL));
>> > @@ -390,13 +391,19 @@ NorFlashFvbInitialize (
>> >// is written as the base of the flash region (ie:
>> > Instance->DeviceBaseAddress)
>> >RuntimeMmioRegionSize = (Instance->RegionBaseAddress -
>> > Instance->DeviceBaseAddress) + Instance->Size;
>> >
>> > - Status = gDS->AddMemorySpace (
>> > - EfiGcdMemoryTypeMemoryMappedIo,
>> > + Status = gDS->GetMemorySpaceDescriptor (
>> >Instance->DeviceBaseAddress,
>> > - RuntimeMmioRegionSize,
>> > - EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
>> > +
>> >);
>> > - ASSERT_EFI_ERROR (Status);
>> > + if (Status == EFI_NOT_FOUND) {
>> > +Status = gDS->AddMemorySpace (
>> > +EfiGcdMemoryTypeMemoryMappedIo,
>> > +Instance->DeviceBaseAddress,
>> > +RuntimeMmioRegionSize,
>> > +EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
>> > +);
>> > +ASSERT_EFI_ERROR (Status);
>> > + }
>> >
>> >Status = gDS->SetMemorySpaceAttributes (
>> >Instance->DeviceBaseAddress,
>> > --
>> > 2.25.1
>> >
>> >
>> >
>> >
>> > Groups.io Links: You receive all messages sent to this group.
>> > View/Reply Online (#100754): https://edk2.groups.io/g/devel/message/100754
>> > Mute This Topic: https://groups.io/mt/97430554/1131722
>> > Group Owner: devel+ow...@edk2.groups.io
>> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [a...@kernel.org]
>> >
>> >
>> >
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105299): https://edk2.groups.io/g/devel/message/105299
Mute This Topic: https://groups.io/mt/97430554/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
