On 6/2/2023 11:50 AM, Ard Biesheuvel wrote:

On Fri, 2 Jun 2023 at 17:26, Michael Kubacki
<mikuback@...> wrote:

Are there particular areas that could be improved to make it more usable
for you? I'm trying to find actionable improvements that can be made, if
any.

I know it's not perfect but developers can run it with a single keyboard
shortcut and it's been useful internally for eliminating style minutia
from distracting design and correctness conversation in code reviews.

I don't disagree with that. But I just don't find it as important as
other people seem to feel it is, and combined with the flaky CI infra
(I just had to amend and push another PR [0] due to infra failure), I
am finding that just getting changes merged is taking a substantial
amount of time, which I'd prefer to spend on other things.
The coding style that uncrustify imposes is overly rigid, and leaves
no room at all for any discretion on the part of the maintainer.
[0] https://github.com/tianocore/edk2/pull/4470

On 6/2/2023 4:51 AM, Ard Biesheuvel wrote:

Uncrustify checks are too rigid, making them counter-productive:

- it leads to code that is arguably harder to parse visually (e.g.,
the changes to ArmPkg/Include/Chipset/AArch64Mmu.h in commit
429309e0c6b74792)

Looking at commit 7f198321eec0f520373, I see positive changes like in
ArmCrashDumpDxe.c spacing in the ASSERT calls and treatment of
multi-line parameter formatting in the mCpu->RegisterInterruptHandler()
call are consistent.

That carries on for a number of C calls with inconsistent spacing before
opening parentheses and calls such as those to the DEBUG macro which I
find easier to read separating each parameter on a dedicated line.

That may be true. But I personally don't think it is important enough
to a) rigidly enforce, and b) fix up for existing code (but that ship
sailed when we did the mass conversion)

Yeah, (b) is done and enforcement can be opted by package for this reason.

In AArch64Mmu.h, I agree that preserving the (mostly) global column as
opposed to block-specific columns would be easier to vertically scan. Is
that the main issue in the file?

Yes.

Thanks for confirming.

- it forces indentation-only changes to code in the vicinity of actual
changes, making the code history more bloated than necessary (see
commit 7f198321eec0f520373 for an example)

That's true. People adjusted things like indentation before depending on
a given change, but it is predetermined now. Perhaps turning off column
alignment (in general or in a given package) could help reduce noise
from this.

Note that I find this aspect quite important. In my opinion, the git
history *is* the code base. Everything we changed at any point in the
past, including the commit logs, is part of this, and mass whitespace
conversions, or spurious changes just to make the CI happy are
problematic to me. For this reason alone, I should be able to override
CI choices at my discretion as a maintainer.

I think there's a balance to not have wildly inconsistent style (reason the guidelines were introduced in the first place), reduce maintainer overhead for style only suggestions, and keep a clean history. But I completely agree with the importance of git history.

I don't think alignment enforcement is necessary. That might also help
address some of thrash in AArch64Mmu.h.

Interesting. If there is room for configuration here, why does the
existing configuration deviate from the coding style guidelines we had
been using for years? AIUI, we use a private fork of uncrustify for
edk2, right? Are there any problematic aspects in the existing
guidelines that was difficult to automate?

Yes, there were several. Some not entirely related to the guidelines but common implementation patterns. Multi-line parameter indentation in function prototypes was not supported. Special handling of double parentheses in DEBUG macro calls was formatted in very weird ways. Inconsistent use of OPTIONAL before and after commas in parameter lists led to varying outcomes. Inline assembly formatting just didn't work well at all (compared to what was already in the code base) and was disabled. The edk2 configuration is in [1].

[1] - https://github.com/tianocore/edk2/blob/master/.pytool/Plugin/UncrustifyCheck/uncrustify.cfg

- finding out from the web UI what exactly Uncrustify objected to is not
straight-forward.

This should not be necessary. It can be run locally to produce the same
result as in CI.

IDE-specific, but a lot of people use VS Code with the Uncrustify
extension
(https://marketplace.visualstudio.com/items?itemName=zachflower.uncrustify)
and that allows Uncrustify to be run against the open source file by
simply pressing the "code formatter" command.

I usually write the code, stage or commit it and then run this command
on the file. The code is formatted and it gives a normal local diff of
exactly what Uncrustify changed.

I'm sure this all seems quite reasonable if you already bought into
using uncrustify. But for a drive-by contributor, or someone like me
who has been contributing code for many years based on the agreed
coding style guidelines, I struggle to understand why uncrustify is a
reasonable solution to the problem of inconsistent coding style.
I'm all for legible code, don't get me wrong. (And Leif is much more
pedantic in that sense than I am). But in my experience, the
uncrustify rules are too rigid, and arbitrary (two spaces indentation
here, four spaces there, etc etc)
I suppose having a button in the GitHub UI that simply lets us
exercise our discretion as maintainers to deviate from these rules
would go a long way in addressing these concerns. But as it stands
now, having my carefully crafted contributions rejected by an
automated system based on guidelines that deviate from the ones that
we agreed to, without any recourse, is simply not acceptable.

I understand and long time contributors generally done a good job maintaining the style. It is difficult to suggest delta to contributor only changes in future commits if specific code does not meet the rules in prior commits though.

So let's enable AuditMode for ArmPkg, so that interested parties can see
the uncrustify recommendations if desired, but without preventing the
changes from being merged. This leaves it at the discretion of the
ArmPkg maintainers to decide which level of conformance is required.

Cc: Leif Lindholm <quic_llindhol@...>
Cc: "Kinney, Michael D" <michael.d.kinney@...>
Cc: Michael Kubacki <mikuback@...>
Signed-off-by: Ard Biesheuvel <ardb@...>
---
ArmPkg/ArmPkg.ci.yaml | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
index 24db7425051388cf..d3124816118944cb 100644
--- a/ArmPkg/ArmPkg.ci.yaml
+++ b/ArmPkg/ArmPkg.ci.yaml
@@ -239,5 +239,10 @@
],

"AdditionalIncludePaths": [] # Additional paths to spell check

# (wildcards supported)

+ },

+

+ # options defined in .pytool/Plugin/UncrustifyCheck

+ "UncrustifyCheck": {

+ "AuditOnly": True

}

}

Done.

Mike

I am working on it.

Mike

[AMD Official Use Only - General]

Could someone push the merge button on this pr please?

https://github.com/tianocore/edk2/pull/4470

On Fri, 2 Jun 2023 at 17:26, Michael Kubacki
<mikuback@...> wrote:

Are there particular areas that could be improved to make it more usable
for you? I'm trying to find actionable improvements that can be made, if
any.

I know it's not perfect but developers can run it with a single keyboard
shortcut and it's been useful internally for eliminating style minutia
from distracting design and correctness conversation in code reviews.

I don't disagree with that. But I just don't find it as important as
other people seem to feel it is, and combined with the flaky CI infra
(I just had to amend and push another PR [0] due to infra failure), I
am finding that just getting changes merged is taking a substantial
amount of time, which I'd prefer to spend on other things.

The coding style that uncrustify imposes is overly rigid, and leaves
no room at all for any discretion on the part of the maintainer.

[0] https://github.com/tianocore/edk2/pull/4470

On 6/2/2023 4:51 AM, Ard Biesheuvel wrote:

Uncrustify checks are too rigid, making them counter-productive:

- it leads to code that is arguably harder to parse visually (e.g.,
the changes to ArmPkg/Include/Chipset/AArch64Mmu.h in commit
429309e0c6b74792)

Looking at commit 7f198321eec0f520373, I see positive changes like in
ArmCrashDumpDxe.c spacing in the ASSERT calls and treatment of
multi-line parameter formatting in the mCpu->RegisterInterruptHandler()
call are consistent.

That carries on for a number of C calls with inconsistent spacing before
opening parentheses and calls such as those to the DEBUG macro which I
find easier to read separating each parameter on a dedicated line.

That may be true. But I personally don't think it is important enough
to a) rigidly enforce, and b) fix up for existing code (but that ship
sailed when we did the mass conversion)

In AArch64Mmu.h, I agree that preserving the (mostly) global column as
opposed to block-specific columns would be easier to vertically scan. Is
that the main issue in the file?

Yes.

- it forces indentation-only changes to code in the vicinity of actual
changes, making the code history more bloated than necessary (see
commit 7f198321eec0f520373 for an example)

That's true. People adjusted things like indentation before depending on
a given change, but it is predetermined now. Perhaps turning off column
alignment (in general or in a given package) could help reduce noise
from this.

Note that I find this aspect quite important. In my opinion, the git
history *is* the code base. Everything we changed at any point in the
past, including the commit logs, is part of this, and mass whitespace
conversions, or spurious changes just to make the CI happy are
problematic to me. For this reason alone, I should be able to override
CI choices at my discretion as a maintainer.

I don't think alignment enforcement is necessary. That might also help
address some of thrash in AArch64Mmu.h.

Interesting. If there is room for configuration here, why does the
existing configuration deviate from the coding style guidelines we had
been using for years? AIUI, we use a private fork of uncrustify for
edk2, right? Are there any problematic aspects in the existing
guidelines that was difficult to automate?

- finding out from the web UI what exactly Uncrustify objected to is not
straight-forward.

This should not be necessary. It can be run locally to produce the same
result as in CI.

IDE-specific, but a lot of people use VS Code with the Uncrustify
extension
(https://marketplace.visualstudio.com/items?itemName=zachflower.uncrustify)
and that allows Uncrustify to be run against the open source file by
simply pressing the "code formatter" command.

I usually write the code, stage or commit it and then run this command
on the file. The code is formatted and it gives a normal local diff of
exactly what Uncrustify changed.

I'm sure this all seems quite reasonable if you already bought into
using uncrustify. But for a drive-by contributor, or someone like me
who has been contributing code for many years based on the agreed
coding style guidelines, I struggle to understand why uncrustify is a
reasonable solution to the problem of inconsistent coding style.

I'm all for legible code, don't get me wrong. (And Leif is much more
pedantic in that sense than I am). But in my experience, the
uncrustify rules are too rigid, and arbitrary (two spaces indentation
here, four spaces there, etc etc)

I suppose having a button in the GitHub UI that simply lets us
exercise our discretion as maintainers to deviate from these rules
would go a long way in addressing these concerns. But as it stands
now, having my carefully crafted contributions rejected by an
automated system based on guidelines that deviate from the ones that
we agreed to, without any recourse, is simply not acceptable.

So let's enable AuditMode for ArmPkg, so that interested parties can see
the uncrustify recommendations if desired, but without preventing the
changes from being merged. This leaves it at the discretion of the
ArmPkg maintainers to decide which level of conformance is required.

Cc: Leif Lindholm <quic_llindhol@...>
Cc: "Kinney, Michael D" <michael.d.kinney@...>
Cc: Michael Kubacki <mikuback@...>
Signed-off-by: Ard Biesheuvel <ardb@...>
---
ArmPkg/ArmPkg.ci.yaml | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
index 24db7425051388cf..d3124816118944cb 100644
--- a/ArmPkg/ArmPkg.ci.yaml
+++ b/ArmPkg/ArmPkg.ci.yaml
@@ -239,5 +239,10 @@
],

"AdditionalIncludePaths": [] # Additional paths to spell check

# (wildcards supported)

+ },

+

+ # options defined in .pytool/Plugin/UncrustifyCheck

+ "UncrustifyCheck": {

+ "AuditOnly": True

}

}

Are there particular areas that could be improved to make it more usable for you? I'm trying to find actionable improvements that can be made, if any.

I know it's not perfect but developers can run it with a single keyboard shortcut and it's been useful internally for eliminating style minutia from distracting design and correctness conversation in code reviews.

On 6/2/2023 4:51 AM, Ard Biesheuvel wrote:

Uncrustify checks are too rigid, making them counter-productive:
- it leads to code that is arguably harder to parse visually (e.g.,
the changes to ArmPkg/Include/Chipset/AArch64Mmu.h in commit
429309e0c6b74792)

Looking at commit 7f198321eec0f520373, I see positive changes like in ArmCrashDumpDxe.c spacing in the ASSERT calls and treatment of multi-line parameter formatting in the mCpu->RegisterInterruptHandler() call are consistent.

That carries on for a number of C calls with inconsistent spacing before opening parentheses and calls such as those to the DEBUG macro which I find easier to read separating each parameter on a dedicated line.

In AArch64Mmu.h, I agree that preserving the (mostly) global column as opposed to block-specific columns would be easier to vertically scan. Is that the main issue in the file?

- it forces indentation-only changes to code in the vicinity of actual
changes, making the code history more bloated than necessary (see
commit 7f198321eec0f520373 for an example)

That's true. People adjusted things like indentation before depending on a given change, but it is predetermined now. Perhaps turning off column alignment (in general or in a given package) could help reduce noise from this.

I don't think alignment enforcement is necessary. That might also help address some of thrash in AArch64Mmu.h.

- finding out from the web UI what exactly Uncrustify objected to is not
straight-forward.

This should not be necessary. It can be run locally to produce the same result as in CI.

IDE-specific, but a lot of people use VS Code with the Uncrustify extension (https://marketplace.visualstudio.com/items?itemName=zachflower.uncrustify) and that allows Uncrustify to be run against the open source file by simply pressing the "code formatter" command.

I usually write the code, stage or commit it and then run this command on the file. The code is formatted and it gives a normal local diff of exactly what Uncrustify changed.

So let's enable AuditMode for ArmPkg, so that interested parties can see
the uncrustify recommendations if desired, but without preventing the
changes from being merged. This leaves it at the discretion of the
ArmPkg maintainers to decide which level of conformance is required.
Cc: Leif Lindholm <quic_llindhol@...>
Cc: "Kinney, Michael D" <michael.d.kinney@...>
Cc: Michael Kubacki <mikuback@...>
Signed-off-by: Ard Biesheuvel <ardb@...>
---
ArmPkg/ArmPkg.ci.yaml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
index 24db7425051388cf..d3124816118944cb 100644
--- a/ArmPkg/ArmPkg.ci.yaml
+++ b/ArmPkg/ArmPkg.ci.yaml
@@ -239,5 +239,10 @@
],
"AdditionalIncludePaths": [] # Additional paths to spell check
# (wildcards supported)
+ },
+
+ # options defined in .pytool/Plugin/UncrustifyCheck
+ "UncrustifyCheck": {
+ "AuditOnly": True
}
}

Sorry, this message was accidentally sent while I was adjusting formatting. I reply to the original mail with a clean reply.

Hi Ard,

Are there particular areas that could be improved to make it more usable for you? I'm trying to find actionable improvements that can be made, if any.

I know it's not perfect but developers can run it with a single keyboard shortcut and it's been useful internally for eliminating style minutia from distracting design and correctness conversation in code reviews.

- finding out from the web UI what exactly Uncrustify objected to is not
straight-forward.

This should not be necessary. It can be run locally to produce the same result as in CI.

IDE-specific, but a lot of people use VS Code with the Uncrustify extension (https://marketplace.visualstudio.com/items?itemName=zachflower.uncrustify) and that allows Uncrustify to be run against the open source file by simply pressing the "code formatter" command.

I usually write the code stage or commit it and then run this command on the file. The code is formatted and it gives a normal local diff of exactly what Uncrustify changed.


On 6/2/2023 4:51 AM, Ard Biesheuvel wrote:

Uncrustify checks are too rigid, making them counter-productive:
- it leads to code that is arguably harder to parse visually (e.g.,
the changes to ArmPkg/Include/Chipset/AArch64Mmu.h in commit
429309e0c6b74792)

Looking at commit 7f198321eec0f520373, I see positive changes like in ArmCrashDumpDxe.c spacing in the ASSERT calls and treatment of multi-line parameter formatting in the mCpu->RegisterInterruptHandler() call are consistent.

That carries on for a number of C calls with inconsistent spacing before opening parentheses and calls such as those to the DEBUG macro which I find easier to read separating each parameter on a dedicated line.

In AArch64Mmu.h, I agree that preserving the (mostly) global column as opposed to block-specific columns would be easier to vertically scan. Is that the main issue in the file?

- it forces indentation-only changes to code in the vicinity of actual
changes, making the code history more bloated than necessary (see
commit 7f198321eec0f520373 for an example)

- finding out from the web UI what exactly Uncrustify objected to is not
straight-forward.
So let's enable AuditMode for ArmPkg, so that interested parties can see
the uncrustify recommendations if desired, but without preventing the
changes from being merged. This leaves it at the discretion of the
ArmPkg maintainers to decide which level of conformance is required.
Cc: Leif Lindholm <quic_llindhol@...>
Cc: "Kinney, Michael D" <michael.d.kinney@...>
Cc: Michael Kubacki <mikuback@...>
Signed-off-by: Ard Biesheuvel <ardb@...>
---
ArmPkg/ArmPkg.ci.yaml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
index 24db7425051388cf..d3124816118944cb 100644
--- a/ArmPkg/ArmPkg.ci.yaml
+++ b/ArmPkg/ArmPkg.ci.yaml
@@ -239,5 +239,10 @@
],
"AdditionalIncludePaths": [] # Additional paths to spell check
# (wildcards supported)
+ },
+
+ # options defined in .pytool/Plugin/UncrustifyCheck
+ "UncrustifyCheck": {
+ "AuditOnly": True
}
}

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 <ardb@...>
---
ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 50 +-------------------
1 file changed, 2 insertions(+), 48 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c b/ArmPkg/Drivers/CpuDx=
e/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=0D
)=0D
{=0D
- EFI_STATUS Status;=0D
-=0D
DEBUG ((=0D
DEBUG_INFO,=0D
"%a: BaseAddress =3D=3D 0x%lx, Length =3D=3D 0x%lx, Attributes =3D=3D =
0x%lx\n",=0D
@@ -204,28 +202,7 @@ SetMemoryAttributes (
return EFI_UNSUPPORTED;=0D
}=0D
=0D
- if ((Attributes & EFI_MEMORY_RP) !=3D 0) {=0D
- Status =3D ArmSetMemoryRegionNoAccess (BaseAddress, Length);=0D
- if (EFI_ERROR (Status)) {=0D
- return EFI_UNSUPPORTED;=0D
- }=0D
- }=0D
-=0D
- if ((Attributes & EFI_MEMORY_RO) !=3D 0) {=0D
- Status =3D ArmSetMemoryRegionReadOnly (BaseAddress, Length);=0D
- if (EFI_ERROR (Status)) {=0D
- return EFI_UNSUPPORTED;=0D
- }=0D
- }=0D
-=0D
- if ((Attributes & EFI_MEMORY_XP) !=3D 0) {=0D
- Status =3D ArmSetMemoryRegionNoExec (BaseAddress, Length);=0D
- if (EFI_ERROR (Status)) {=0D
- return EFI_UNSUPPORTED;=0D
- }=0D
- }=0D
-=0D
- return EFI_SUCCESS;=0D
+ return ArmSetMemoryAttributes (BaseAddress, Length, Attributes, Attribut=
es);=0D
}=0D
=0D
/**=0D
@@ -267,8 +244,6 @@ ClearMemoryAttributes (
IN UINT64 Attributes=0D
)=0D
{=0D
- EFI_STATUS Status;=0D
-=0D
DEBUG ((=0D
DEBUG_INFO,=0D
"%a: BaseAddress =3D=3D 0x%lx, Length =3D=3D 0x%lx, Attributes =3D=3D =
0x%lx\n",=0D
@@ -288,28 +263,7 @@ ClearMemoryAttributes (
return EFI_UNSUPPORTED;=0D
}=0D
=0D
- if ((Attributes & EFI_MEMORY_RP) !=3D 0) {=0D
- Status =3D ArmClearMemoryRegionNoAccess (BaseAddress, Length);=0D
- if (EFI_ERROR (Status)) {=0D
- return EFI_UNSUPPORTED;=0D
- }=0D
- }=0D
-=0D
- if ((Attributes & EFI_MEMORY_RO) !=3D 0) {=0D
- Status =3D ArmClearMemoryRegionReadOnly (BaseAddress, Length);=0D
- if (EFI_ERROR (Status)) {=0D
- return EFI_UNSUPPORTED;=0D
- }=0D
- }=0D
-=0D
- if ((Attributes & EFI_MEMORY_XP) !=3D 0) {=0D
- Status =3D ArmClearMemoryRegionNoExec (BaseAddress, Length);=0D
- if (EFI_ERROR (Status)) {=0D
- return EFI_UNSUPPORTED;=0D
- }=0D
- }=0D
-=0D
- return EFI_SUCCESS;=0D
+ return ArmSetMemoryAttributes (BaseAddress, Length, 0, Attributes);=0D
}=0D
=0D
EFI_MEMORY_ATTRIBUTE_PROTOCOL mMemoryAttribute =3D {=0D
--=20
2.39.2

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 <ardb@...>
---
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..0000000000000000
--- a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
+++ /dev/null
@@ -1,71 +0,0 @@
-/** @file=0D
- ARM specifc functionality for DxeLoad.=0D
-=0D
-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>=0D
-Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>=0D
-=0D
-SPDX-License-Identifier: BSD-2-Clause-Patent=0D
-=0D
-**/=0D
-=0D
-#include "DxeIpl.h"=0D
-=0D
-#include <Library/ArmMmuLib.h>=0D
-=0D
-/**=0D
- Transfers control to DxeCore.=0D
-=0D
- This function performs a CPU architecture specific operations to execut=
e=0D
- the entry point of DxeCore with the parameters of HobList.=0D
- It also installs EFI_END_OF_PEI_PPI to signal the end of PEI phase.=0D
-=0D
- @param DxeCoreEntryPoint The entry point of DxeCore.=0D
- @param HobList The start of HobList passed to DxeCore=
.=0D
-=0D
-**/=0D
-VOID=0D
-HandOffToDxeCore (=0D
- IN EFI_PHYSICAL_ADDRESS DxeCoreEntryPoint,=0D
- IN EFI_PEI_HOB_POINTERS HobList=0D
- )=0D
-{=0D
- VOID *BaseOfStack;=0D
- VOID *TopOfStack;=0D
- EFI_STATUS Status;=0D
-=0D
- //=0D
- // Allocate 128KB for the Stack=0D
- //=0D
- BaseOfStack =3D AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));=0D
- ASSERT (BaseOfStack !=3D NULL);=0D
-=0D
- if (PcdGetBool (PcdSetNxForStack)) {=0D
- Status =3D ArmSetMemoryRegionNoExec ((UINTN)BaseOfStack, STACK_SIZE);=
=0D
- ASSERT_EFI_ERROR (Status);=0D
- }=0D
-=0D
- //=0D
- // Compute the top of the stack we were allocated. Pre-allocate a UINTN=
=0D
- // for safety.=0D
- //=0D
- TopOfStack =3D (VOID *)((UINTN)BaseOfStack + EFI_SIZE_TO_PAGES (STACK_SI=
ZE) * EFI_PAGE_SIZE - CPU_STACK_ALIGNMENT);=0D
- TopOfStack =3D ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);=0D
-=0D
- //=0D
- // End of PEI phase singal=0D
- //=0D
- Status =3D PeiServicesInstallPpi (&gEndOfPeiSignalPpi);=0D
- ASSERT_EFI_ERROR (Status);=0D
-=0D
- //=0D
- // Update the contents of BSP stack HOB to reflect the real stack info p=
assed to DxeCore.=0D
- //=0D
- UpdateStackHob ((EFI_PHYSICAL_ADDRESS)(UINTN)BaseOfStack, STACK_SIZE);=0D
-=0D
- SwitchStack (=0D
- (SWITCH_STACK_ENTRY_POINT)(UINTN)DxeCoreEntryPoint,=0D
- HobList.Raw,=0D
- NULL,=0D
- TopOfStack=0D
- );=0D
-}=0D
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/Dx=
eIplPeim/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=0D
X64/DxeLoadFunc.c=0D
=0D
-[Sources.ARM, Sources.AARCH64]=0D
- Arm/DxeLoadFunc.c=0D
-=0D
-[Sources.LOONGARCH64,Sources.RISCV64,Sources.EBC]=0D
+[Sources.LOONGARCH64,Sources.RISCV64,Sources.EBC,Sources.ARM,Sources.AARCH=
64]=0D
DxeHandoff.c=0D
=0D
[Packages]=0D
MdePkg/MdePkg.dec=0D
MdeModulePkg/MdeModulePkg.dec=0D
=0D
-[Packages.ARM, Packages.AARCH64]=0D
- ArmPkg/ArmPkg.dec=0D
-=0D
[LibraryClasses]=0D
PcdLib=0D
MemoryAllocationLib=0D
@@ -74,9 +68,6 @@ [LibraryClasses]
PeiServicesTablePointerLib=0D
PerformanceLib=0D
=0D
-[LibraryClasses.ARM, LibraryClasses.AARCH64]=0D
- ArmMmuLib=0D
-=0D
[Ppis]=0D
gEfiDxeIplPpiGuid ## PRODUCES=0D
gEfiPeiDecompressPpiGuid ## PRODUCES=0D
--=20
2.39.2

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 <ardb@...>
---
ArmPkg/Drivers/CpuPei/CpuPei.c | 76 ++++++++++++++++++++
ArmPkg/Drivers/CpuPei/CpuPei.inf | 4 ++
2 files changed, 80 insertions(+)

diff --git a/ArmPkg/Drivers/CpuPei/CpuPei.c b/ArmPkg/Drivers/CpuPei/CpuPei.c
index 85ef5ec07b9fdafa..1c2b53100f6a424e 100644
--- a/ArmPkg/Drivers/CpuPei/CpuPei.c
+++ b/ArmPkg/Drivers/CpuPei/CpuPei.c
@@ -3,6 +3,7 @@
Copyright (c) 2006, Intel Corporation. All rights reserved.<BR>=0D
Copyright (c) 2011 Hewlett Packard Corporation. All rights reserved.<BR>=0D
Copyright (c) 2011-2013, ARM Limited. All rights reserved.<BR>=0D
+Copyright (c) 2023, Google, LLC. All rights reserved.<BR>=0D
=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
@@ -24,6 +25,7 @@ Module Name:
// The protocols, PPI and GUID definitions for this module=0D
//=0D
#include <Ppi/ArmMpCoreInfo.h>=0D
+#include <Ppi/MemoryAttribute.h>=0D
=0D
//=0D
// The Library classes this module consumes=0D
@@ -34,6 +36,77 @@ Module Name:
#include <Library/PcdLib.h>=0D
#include <Library/HobLib.h>=0D
#include <Library/ArmLib.h>=0D
+#include <Library/ArmMmuLib.h>=0D
+=0D
+/**=0D
+ Set the requested memory permission attributes on a region of memory.=0D
+=0D
+ BaseAddress and Length must be aligned to EFI_PAGE_SIZE.=0D
+=0D
+ Attributes must contain a combination of EFI_MEMORY_RP, EFI_MEMORY_RO an=
d=0D
+ EFI_MEMORY_XP, and specifies the attributes that must be set for the=0D
+ region in question. Attributes that are omitted will be cleared from the=
=0D
+ region only if they are set in AttributeMask.=0D
+=0D
+ AttributeMask must contain a combination of EFI_MEMORY_RP, EFI_MEMORY_RO=
and=0D
+ EFI_MEMORY_XP, and specifies the attributes that the call will operate o=
n.=0D
+ AttributeMask must not be 0x0, and must contain at least the bits set in=
=0D
+ Attributes.=0D
+=0D
+ @param[in] This The protocol instance pointer.=0D
+ @param[in] BaseAddress The physical address that is the start add=
ress=0D
+ of a memory region.=0D
+ @param[in] Length The size in bytes of the memory region.=0D
+ @param[in] Attributes Memory attributes to set or clear.=0D
+ @param[in] AttributeMask Mask of memory attributes to operate on.=0D
+=0D
+ @retval EFI_SUCCESS The attributes were set for the memory reg=
ion.=0D
+ @retval EFI_INVALID_PARAMETER Length is zero.=0D
+ AttributeMask is zero.=0D
+ AttributeMask lacks bits set in Attributes=
.=0D
+ BaseAddress or Length is not suitably alig=
ned.=0D
+ @retval EFI_UNSUPPORTED The processor does not support one or more=
=0D
+ bytes of the memory resource range specifi=
ed=0D
+ by BaseAddress and Length.=0D
+ The bit mask of attributes is not supporte=
d for=0D
+ the memory resource range specified by=0D
+ BaseAddress and Length.=0D
+ @retval EFI_OUT_OF_RESOURCES Requested attributes cannot be applied due=
to=0D
+ lack of system resources.=0D
+=0D
+**/=0D
+STATIC=0D
+EFI_STATUS=0D
+EFIAPI=0D
+SetMemoryPermissions (=0D
+ IN EDKII_MEMORY_ATTRIBUTE_PPI *This,=0D
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,=0D
+ IN UINT64 Length,=0D
+ IN UINT64 Attributes,=0D
+ IN UINT64 AttributeMask=0D
+ )=0D
+{=0D
+ if ((Length =3D=3D 0) ||=0D
+ (AttributeMask =3D=3D 0) ||=0D
+ ((AttributeMask & (EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) =
=3D=3D 0) ||=0D
+ ((Attributes & ~AttributeMask) !=3D 0) ||=0D
+ (((BaseAddress | Length) & EFI_PAGE_MASK) !=3D 0))=0D
+ {=0D
+ return EFI_INVALID_PARAMETER;=0D
+ }=0D
+=0D
+ return ArmSetMemoryAttributes (BaseAddress, Length, Attributes, Attribut=
eMask);=0D
+}=0D
+=0D
+STATIC CONST EDKII_MEMORY_ATTRIBUTE_PPI mMemoryAttributePpi =3D {=0D
+ SetMemoryPermissions=0D
+};=0D
+=0D
+STATIC CONST EFI_PEI_PPI_DESCRIPTOR mMemoryAttributePpiDesc =3D {=0D
+ (EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),=0D
+ &gEdkiiMemoryAttributePpiGuid,=0D
+ (VOID *)&mMemoryAttributePpi=0D
+};=0D
=0D
/*++=0D
=0D
@@ -79,5 +152,8 @@ InitializeCpuPeim (
}=0D
}=0D
=0D
+ Status =3D PeiServicesInstallPpi (&mMemoryAttributePpiDesc);=0D
+ ASSERT_EFI_ERROR (Status);=0D
+=0D
return EFI_SUCCESS;=0D
}=0D
diff --git a/ArmPkg/Drivers/CpuPei/CpuPei.inf b/ArmPkg/Drivers/CpuPei/CpuPe=
i.inf
index a9f85cbc68b1c52e..49b67077ec6166f1 100644
--- a/ArmPkg/Drivers/CpuPei/CpuPei.inf
+++ b/ArmPkg/Drivers/CpuPei/CpuPei.inf
@@ -3,6 +3,7 @@
#=0D
# This module provides platform specific function to detect boot mode.=0D
# Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.<BR>=0D
+# Copyright (c) 2023, Google, LLC. All rights reserved.<BR>=0D
#=0D
# SPDX-License-Identifier: BSD-2-Clause-Patent=0D
#=0D
@@ -28,6 +29,7 @@ [Sources]
CpuPei.c=0D
=0D
[Packages]=0D
+ MdeModulePkg/MdeModulePkg.dec=0D
MdePkg/MdePkg.dec=0D
EmbeddedPkg/EmbeddedPkg.dec=0D
ArmPkg/ArmPkg.dec=0D
@@ -37,9 +39,11 @@ [LibraryClasses]
DebugLib=0D
HobLib=0D
ArmLib=0D
+ ArmMmuLib=0D
=0D
[Ppis]=0D
gArmMpCoreInfoPpiGuid=0D
+ gEdkiiMemoryAttributePpiGuid=0D
=0D
[Guids]=0D
gArmMpCoreInfoGuid=0D
--=20
2.39.2

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 <ardb@...>
---
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/C=
puMmuCommon.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 !=3D ArmAttributes) ||=0D
((BaseAddress + Length) > (RegionBaseAddress + RegionLength)))=0D
{=0D
- return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes);=0D
+ return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes, 0);=
=0D
} else {=0D
return EFI_SUCCESS;=0D
}=0D
diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/Ar=
mMmuLib.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=0D
);=0D
=0D
+/**=0D
+ Set the requested memory permission attributes on a region of memory.=0D
+=0D
+ BaseAddress and Length must be aligned to EFI_PAGE_SIZE.=0D
+=0D
+ If Attributes contains a memory type attribute (EFI_MEMORY_UC/WC/WT/WB),=
the=0D
+ region is mapped according to this memory type, and additional memory=0D
+ permission attributes (EFI_MEMORY_RP/RO/XP) are taken into account as we=
ll,=0D
+ discarding any permission attributes that are currently set for the regi=
on.=0D
+ AttributeMask is ignored in this case, and must be set to 0x0.=0D
+=0D
+ If Attributes contains only a combination of memory permission attribute=
s=0D
+ (EFI_MEMORY_RP/RO/XP), each page in the region will retain its existing=
=0D
+ memory type, even if it is not uniformly set across the region. In this =
case,=0D
+ AttributesMask may be set to a mask of permission attributes, and memory=
=0D
+ permissions omitted from this mask will not be updated for any page in t=
he=0D
+ region. All attributes appearing in Attributes must appear in AttributeM=
ask=0D
+ as well. (Attributes & ~AttributeMask must produce 0x0)=0D
+=0D
+ @param[in] BaseAddress The physical address that is the start addre=
ss of=0D
+ a memory region.=0D
+ @param[in] Length The size in bytes of the memory region.=0D
+ @param[in] Attributes Mask of memory attributes to set.=0D
+ @param[in] AttributeMask Mask of memory attributes to take into accou=
nt.=0D
+=0D
+ @retval EFI_SUCCESS The attributes were set for the memory reg=
ion.=0D
+ @retval EFI_INVALID_PARAMETER BaseAddress or Length is not suitably alig=
ned.=0D
+ Invalid combination of Attributes and=0D
+ AttributeMask.=0D
+ @retval EFI_OUT_OF_RESOURCES Requested attributes cannot be applied due=
to=0D
+ lack of system resources.=0D
+=0D
+**/=0D
EFI_STATUS=0D
ArmSetMemoryAttributes (=0D
IN EFI_PHYSICAL_ADDRESS BaseAddress,=0D
IN UINT64 Length,=0D
- IN UINT64 Attributes=0D
+ IN UINT64 Attributes,=0D
+ IN UINT64 AttributeMask=0D
);=0D
=0D
#endif // ARM_MMU_LIB_H_=0D
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Libr=
ary/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;=0D
}=0D
=0D
+/**=0D
+ Set the requested memory permission attributes on a region of memory.=0D
+=0D
+ BaseAddress and Length must be aligned to EFI_PAGE_SIZE.=0D
+=0D
+ If Attributes contains a memory type attribute (EFI_MEMORY_UC/WC/WT/WB),=
the=0D
+ region is mapped according to this memory type, and additional memory=0D
+ permission attributes (EFI_MEMORY_RP/RO/XP) are taken into account as we=
ll,=0D
+ discarding any permission attributes that are currently set for the regi=
on.=0D
+ AttributeMask is ignored in this case, and must be set to 0x0.=0D
+=0D
+ If Attributes contains only a combination of memory permission attribute=
s=0D
+ (EFI_MEMORY_RP/RO/XP), each page in the region will retain its existing=
=0D
+ memory type, even if it is not uniformly set across the region. In this =
case,=0D
+ AttributesMask may be set to a mask of permission attributes, and memory=
=0D
+ permissions omitted from this mask will not be updated for any page in t=
he=0D
+ region. All attributes appearing in Attributes must appear in AttributeM=
ask=0D
+ as well. (Attributes & ~AttributeMask must produce 0x0)=0D
+=0D
+ @param[in] BaseAddress The physical address that is the start addre=
ss of=0D
+ a memory region.=0D
+ @param[in] Length The size in bytes of the memory region.=0D
+ @param[in] Attributes Mask of memory attributes to set.=0D
+ @param[in] AttributeMask Mask of memory attributes to take into accou=
nt.=0D
+=0D
+ @retval EFI_SUCCESS The attributes were set for the memory reg=
ion.=0D
+ @retval EFI_INVALID_PARAMETER BaseAddress or Length is not suitably alig=
ned.=0D
+ Invalid combination of Attributes and=0D
+ AttributeMask.=0D
+ @retval EFI_OUT_OF_RESOURCES Requested attributes cannot be applied due=
to=0D
+ lack of system resources.=0D
+=0D
+**/=0D
EFI_STATUS=0D
ArmSetMemoryAttributes (=0D
IN EFI_PHYSICAL_ADDRESS BaseAddress,=0D
IN UINT64 Length,=0D
- IN UINT64 Attributes=0D
+ IN UINT64 Attributes,=0D
+ IN UINT64 AttributeMask=0D
)=0D
{=0D
UINT64 PageAttributes;=0D
@@ -490,6 +524,22 @@ ArmSetMemoryAttributes (
PageAttributes &=3D TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK | TT_AF;=
=0D
PageAttributeMask =3D ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK |=0D
TT_PXN_MASK | TT_XN_MASK | TT_AF);=0D
+ if (AttributeMask !=3D 0) {=0D
+ if (((AttributeMask & ~(UINT64)(EFI_MEMORY_RP|EFI_MEMORY_RO|EFI_MEMO=
RY_XP)) !=3D 0) ||=0D
+ ((Attributes & ~AttributeMask) !=3D 0))=0D
+ {=0D
+ return EFI_INVALID_PARAMETER;=0D
+ }=0D
+=0D
+ // Add attributes omitted from AttributeMask to the set of attribute=
s to preserve=0D
+ PageAttributeMask |=3D GcdAttributeToPageAttribute (~AttributeMask) =
&=0D
+ (TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK | TT_AF=
);=0D
+ }=0D
+ } else {=0D
+ ASSERT (AttributeMask =3D=3D 0);=0D
+ if (AttributeMask !=3D 0) {=0D
+ return EFI_INVALID_PARAMETER;=0D
+ }=0D
}=0D
=0D
return UpdateRegionMapping (=0D
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Librar=
y/ArmMmuLib/Arm/ArmMmuLibUpdate.c
index 299d38ad07e85059..61405965a73eaeb8 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
@@ -10,6 +10,7 @@
#include <Uefi.h>=0D
=0D
#include <Library/ArmLib.h>=0D
+#include <Library/ArmMmuLib.h>=0D
#include <Library/BaseLib.h>=0D
#include <Library/BaseMemoryLib.h>=0D
#include <Library/DebugLib.h>=0D
@@ -451,31 +452,96 @@ SetMemoryAttributes (
}=0D
=0D
/**=0D
- Update the permission or memory type attributes on a range of memory.=0D
+ Set the requested memory permission attributes on a region of memory.=0D
=0D
- @param BaseAddress The start of the region.=0D
- @param Length The size of the region.=0D
- @param Attributes A mask of EFI_MEMORY_xx constants.=0D
+ BaseAddress and Length must be aligned to EFI_PAGE_SIZE.=0D
=0D
- @retval EFI_SUCCESS The attributes were set successfully.=0D
- @retval EFI_OUT_OF_RESOURCES The operation failed due to insufficient m=
emory.=0D
+ If Attributes contains a memory type attribute (EFI_MEMORY_UC/WC/WT/WB),=
the=0D
+ region is mapped according to this memory type, and additional memory=0D
+ permission attributes (EFI_MEMORY_RP/RO/XP) are taken into account as we=
ll,=0D
+ discarding any permission attributes that are currently set for the regi=
on.=0D
+ AttributeMask is ignored in this case, and must be set to 0x0.=0D
+=0D
+ If Attributes contains only a combination of memory permission attribute=
s=0D
+ (EFI_MEMORY_RP/RO/XP), each page in the region will retain its existing=
=0D
+ memory type, even if it is not uniformly set across the region. In this =
case,=0D
+ AttributesMask may be set to a mask of permission attributes, and memory=
=0D
+ permissions omitted from this mask will not be updated for any page in t=
he=0D
+ region. All attributes appearing in Attributes must appear in AttributeM=
ask=0D
+ as well. (Attributes & ~AttributeMask must produce 0x0)=0D
+=0D
+ @param[in] BaseAddress The physical address that is the start addre=
ss of=0D
+ a memory region.=0D
+ @param[in] Length The size in bytes of the memory region.=0D
+ @param[in] Attributes Mask of memory attributes to set.=0D
+ @param[in] AttributeMask Mask of memory attributes to take into accou=
nt.=0D
+=0D
+ @retval EFI_SUCCESS The attributes were set for the memory reg=
ion.=0D
+ @retval EFI_INVALID_PARAMETER BaseAddress or Length is not suitably alig=
ned.=0D
+ Invalid combination of Attributes and=0D
+ AttributeMask.=0D
+ @retval EFI_OUT_OF_RESOURCES Requested attributes cannot be applied due=
to=0D
+ lack of system resources.=0D
=0D
**/=0D
EFI_STATUS=0D
ArmSetMemoryAttributes (=0D
IN EFI_PHYSICAL_ADDRESS BaseAddress,=0D
IN UINT64 Length,=0D
- IN UINT64 Attributes=0D
+ IN UINT64 Attributes,=0D
+ IN UINT64 AttributeMask=0D
)=0D
{=0D
+ UINT32 TtEntryMask;=0D
+=0D
+ if (((BaseAddress | Length) & EFI_PAGE_MASK) !=3D 0) {=0D
+ return EFI_INVALID_PARAMETER;=0D
+ }=0D
+=0D
+ if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) =3D=3D 0) {=0D
+ //=0D
+ // No memory type was set in Attributes, so we are going to update the=
=0D
+ // permissions only.=0D
+ //=0D
+ if (AttributeMask !=3D 0) {=0D
+ if (((AttributeMask & ~(UINT64)(EFI_MEMORY_RP|EFI_MEMORY_RO|EFI_MEMO=
RY_XP)) !=3D 0) ||=0D
+ ((Attributes & ~AttributeMask) !=3D 0))=0D
+ {=0D
+ return EFI_INVALID_PARAMETER;=0D
+ }=0D
+ } else {=0D
+ AttributeMask =3D EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP;=0D
+ }=0D
+=0D
+ TtEntryMask =3D 0;=0D
+ if ((AttributeMask & EFI_MEMORY_RP) !=3D 0) {=0D
+ TtEntryMask |=3D TT_DESCRIPTOR_SECTION_AF;=0D
+ }=0D
+=0D
+ if ((AttributeMask & EFI_MEMORY_RO) !=3D 0) {=0D
+ TtEntryMask |=3D TT_DESCRIPTOR_SECTION_AP_MASK;=0D
+ }=0D
+=0D
+ if ((AttributeMask & EFI_MEMORY_XP) !=3D 0) {=0D
+ TtEntryMask |=3D TT_DESCRIPTOR_SECTION_XN_MASK;=0D
+ }=0D
+ } else {=0D
+ ASSERT (AttributeMask =3D=3D 0);=0D
+ if (AttributeMask !=3D 0) {=0D
+ return EFI_INVALID_PARAMETER;=0D
+ }=0D
+=0D
+ TtEntryMask =3D TT_DESCRIPTOR_SECTION_TYPE_MASK |=0D
+ TT_DESCRIPTOR_SECTION_XN_MASK |=0D
+ TT_DESCRIPTOR_SECTION_AP_MASK |=0D
+ TT_DESCRIPTOR_SECTION_AF;=0D
+ }=0D
+=0D
return SetMemoryAttributes (=0D
BaseAddress,=0D
Length,=0D
Attributes,=0D
- TT_DESCRIPTOR_SECTION_TYPE_MASK |=0D
- TT_DESCRIPTOR_SECTION_XN_MASK |=0D
- TT_DESCRIPTOR_SECTION_AP_MASK |=0D
- TT_DESCRIPTOR_SECTION_AF=0D
+ TtEntryMask=0D
);=0D
}=0D
=0D
diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Opte=
e.c
index 48e33cb3d5ee4ab6..46464f17ef06653e 100644
--- a/ArmPkg/Library/OpteeLib/Optee.c
+++ b/ArmPkg/Library/OpteeLib/Optee.c
@@ -86,7 +86,7 @@ OpteeSharedMemoryRemap (
return EFI_BUFFER_TOO_SMALL;=0D
}=0D
=0D
- Status =3D ArmSetMemoryAttributes (PhysicalAddress, Size, EFI_MEMORY_WB)=
;=0D
+ Status =3D ArmSetMemoryAttributes (PhysicalAddress, Size, EFI_MEMORY_WB,=
0);=0D
if (EFI_ERROR (Status)) {=0D
return Status;=0D
}=0D
--=20
2.39.2

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 <ardb@...>
---
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..60400da3521a8272 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.=0D
=0D
Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>=0D
+Copyright (c) 2023, Google, LLC. All rights reserved.<BR>=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
=0D
#include "DxeIpl.h"=0D
=0D
+#include <Ppi/MemoryAttribute.h>=0D
+=0D
/**=0D
Transfers control to DxeCore.=0D
=0D
@@ -25,9 +28,10 @@ HandOffToDxeCore (
IN EFI_PEI_HOB_POINTERS HobList=0D
)=0D
{=0D
- VOID *BaseOfStack;=0D
- VOID *TopOfStack;=0D
- EFI_STATUS Status;=0D
+ VOID *BaseOfStack;=0D
+ VOID *TopOfStack;=0D
+ EFI_STATUS Status;=0D
+ EDKII_MEMORY_ATTRIBUTE_PPI *MemoryPpi;=0D
=0D
//=0D
// Allocate 128KB for the Stack=0D
@@ -35,6 +39,25 @@ HandOffToDxeCore (
BaseOfStack =3D AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));=0D
ASSERT (BaseOfStack !=3D NULL);=0D
=0D
+ if (PcdGetBool (PcdSetNxForStack)) {=0D
+ Status =3D PeiServicesLocatePpi (=0D
+ &gEdkiiMemoryAttributePpiGuid,=0D
+ 0,=0D
+ NULL,=0D
+ (VOID **)&MemoryPpi=0D
+ );=0D
+ ASSERT_EFI_ERROR (Status);=0D
+=0D
+ Status =3D MemoryPpi->SetPermissions (=0D
+ MemoryPpi,=0D
+ (UINTN)BaseOfStack,=0D
+ STACK_SIZE,=0D
+ EFI_MEMORY_XP,=0D
+ EFI_MEMORY_XP=0D
+ );=0D
+ ASSERT_EFI_ERROR (Status);=0D
+ }=0D
+=0D
//=0D
// Compute the top of the stack we were allocated. Pre-allocate a UINTN=
=0D
// for safety.=0D
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/Dx=
eIplPeim/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=0D
gEdkiiPeiBootInCapsuleOnDiskModePpiGuid ## SOMETIMES_CONSUMES=0D
gEdkiiPeiCapsuleOnDiskPpiGuid ## SOMETIMES_CONSUMES # Consume=
d on firmware update boot path=0D
+ gEdkiiMemoryAttributePpiGuid ## SOMETIMES_CONSUMES=0D
=0D
[Guids]=0D
## SOMETIMES_CONSUMES ## Variable:L"MemoryTypeInformation"=0D
@@ -117,10 +118,12 @@ [Pcd.IA32,Pcd.X64]
gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize ##=
CONSUMES=0D
=0D
[Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]=0D
- gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ## SOMETIM=
ES_CONSUMES=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## SOMETIM=
ES_CONSUMES=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ## SOMETIM=
ES_CONSUMES=0D
=0D
+[Pcd]=0D
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ## SOMETIM=
ES_CONSUMES=0D
+=0D
[Depex]=0D
gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid=0D
=0D
--=20
2.39.2

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 <ardb@...>
---
MdeModulePkg/Core/DxeIplPeim/{Ebc/DxeLoadFunc.c =3D> 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=0D
- EBC-specific functionality for DxeLoad.=0D
+ Generic version of arch-specific functionality for DxeLoad.=0D
=0D
Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/Dx=
eIplPeim/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=0D
X64/DxeLoadFunc.c=0D
=0D
-[Sources.EBC]=0D
- Ebc/DxeLoadFunc.c=0D
-=0D
[Sources.ARM, Sources.AARCH64]=0D
Arm/DxeLoadFunc.c=0D
=0D
-[Sources.RISCV64]=0D
- RiscV64/DxeLoadFunc.c=0D
-=0D
-[Sources.LOONGARCH64]=0D
- LoongArch64/DxeLoadFunc.c=0D
+[Sources.LOONGARCH64,Sources.RISCV64,Sources.EBC]=0D
+ DxeHandoff.c=0D
=0D
[Packages]=0D
MdePkg/MdePkg.dec=0D
diff --git a/MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c b/MdeMo=
dulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c
deleted file mode 100644
index 95d3af19ea4c9f00..0000000000000000
--- a/MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c
+++ /dev/null
@@ -1,63 +0,0 @@
-/** @file=0D
- LoongArch specifc functionality for DxeLoad.=0D
-=0D
- Copyright (c) 2022, Loongson Technology Corporation Limited. All rights =
reserved.<BR>=0D
-=0D
- SPDX-License-Identifier: BSD-2-Clause-Patent=0D
-=0D
-**/=0D
-=0D
-#include "DxeIpl.h"=0D
-=0D
-/**=0D
- Transfers control to DxeCore.=0D
-=0D
- This function performs a CPU architecture specific operations to execut=
e=0D
- the entry point of DxeCore with the parameters of HobList.=0D
- It also installs EFI_END_OF_PEI_PPI to signal the end of PEI phase.=0D
-=0D
- @param[in] DxeCoreEntryPoint The entry point of DxeCore.=0D
- @param[in] HobList The start of HobList passed to Dxe=
Core.=0D
-=0D
-**/=0D
-VOID=0D
-HandOffToDxeCore (=0D
- IN EFI_PHYSICAL_ADDRESS DxeCoreEntryPoint,=0D
- IN EFI_PEI_HOB_POINTERS HobList=0D
- )=0D
-{=0D
- VOID *BaseOfStack;=0D
- VOID *TopOfStack;=0D
- EFI_STATUS Status;=0D
-=0D
- //=0D
- // Allocate 128KB for the Stack=0D
- //=0D
- BaseOfStack =3D AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));=0D
- ASSERT (BaseOfStack !=3D NULL);=0D
-=0D
- //=0D
- // Compute the top of the stack we were allocated. Pre-allocate a UINTN=
=0D
- // for safety.=0D
- //=0D
- TopOfStack =3D (VOID *)((UINTN)BaseOfStack + EFI_SIZE_TO_PAGES (STACK_SI=
ZE) * EFI_PAGE_SIZE - CPU_STACK_ALIGNMENT);=0D
- TopOfStack =3D ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);=0D
-=0D
- //=0D
- // End of PEI phase signal=0D
- //=0D
- Status =3D PeiServicesInstallPpi (&gEndOfPeiSignalPpi);=0D
- ASSERT_EFI_ERROR (Status);=0D
-=0D
- //=0D
- // Update the contents of BSP stack HOB to reflect the real stack info p=
assed to DxeCore.=0D
- //=0D
- UpdateStackHob ((EFI_PHYSICAL_ADDRESS)(UINTN)BaseOfStack, STACK_SIZE);=0D
-=0D
- SwitchStack (=0D
- (SWITCH_STACK_ENTRY_POINT)(UINTN)DxeCoreEntryPoint,=0D
- HobList.Raw,=0D
- NULL,=0D
- TopOfStack=0D
- );=0D
-}=0D
diff --git a/MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c b/MdeModule=
Pkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c
deleted file mode 100644
index b3567d88f73467e7..0000000000000000
--- a/MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c
+++ /dev/null
@@ -1,75 +0,0 @@
-/** @file=0D
- RISC-V specific functionality for DxeLoad.=0D
-=0D
- Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All right=
s reserved.<BR>=0D
-=0D
- SPDX-License-Identifier: BSD-2-Clause-Patent=0D
-=0D
-**/=0D
-=0D
-#include "DxeIpl.h"=0D
-=0D
-/**=0D
- Transfers control to DxeCore.=0D
-=0D
- This function performs a CPU architecture specific operations to execut=
e=0D
- the entry point of DxeCore with the parameters of HobList.=0D
- It also installs EFI_END_OF_PEI_PPI to signal the end of PEI phase.=0D
-=0D
- @param DxeCoreEntryPoint The entry point of DxeCore.=0D
- @param HobList The start of HobList passed to DxeCore=
.=0D
-=0D
-**/=0D
-VOID=0D
-HandOffToDxeCore (=0D
- IN EFI_PHYSICAL_ADDRESS DxeCoreEntryPoint,=0D
- IN EFI_PEI_HOB_POINTERS HobList=0D
- )=0D
-{=0D
- VOID *BaseOfStack;=0D
- VOID *TopOfStack;=0D
- EFI_STATUS Status;=0D
-=0D
- //=0D
- //=0D
- // Allocate 128KB for the Stack=0D
- //=0D
- BaseOfStack =3D AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));=0D
- if (BaseOfStack =3D=3D NULL) {=0D
- DEBUG ((DEBUG_ERROR, "%a: Can't allocate memory for stack.", __func__)=
);=0D
- ASSERT (FALSE);=0D
- }=0D
-=0D
- //=0D
- // Compute the top of the stack we were allocated. Pre-allocate a UINTN=
=0D
- // for safety.=0D
- //=0D
- TopOfStack =3D (VOID *)((UINTN)BaseOfStack + EFI_SIZE_TO_PAGES (STACK_SI=
ZE) * EFI_PAGE_SIZE - CPU_STACK_ALIGNMENT);=0D
- TopOfStack =3D ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);=0D
-=0D
- //=0D
- // End of PEI phase signal=0D
- //=0D
- Status =3D PeiServicesInstallPpi (&gEndOfPeiSignalPpi);=0D
- if (EFI_ERROR (Status)) {=0D
- DEBUG ((DEBUG_ERROR, "%a: Fail to signal End of PEI event.", __func__)=
);=0D
- ASSERT (FALSE);=0D
- }=0D
-=0D
- //=0D
- // Update the contents of BSP stack HOB to reflect the real stack info p=
assed to DxeCore.=0D
- //=0D
- UpdateStackHob ((EFI_PHYSICAL_ADDRESS)(UINTN)BaseOfStack, STACK_SIZE);=0D
-=0D
- DEBUG ((DEBUG_INFO, "DXE Core new stack at %x, stack pointer at %x\n", B=
aseOfStack, TopOfStack));=0D
-=0D
- //=0D
- // Transfer the control to the entry point of DxeCore.=0D
- //=0D
- SwitchStack (=0D
- (SWITCH_STACK_ENTRY_POINT)(UINTN)DxeCoreEntryPoint,=0D
- HobList.Raw,=0D
- NULL,=0D
- TopOfStack=0D
- );=0D
-}=0D
--=20
2.39.2

Define a PPI interface that may be used by the PEI core or other PEIMs
to manage permissions on memory ranges. This is primarily intended for
restricting permissions to what is actually needed for correct execution
by the code in question, and for limiting the use of memory mappings
that are both writable and executable at the same time.

Signed-off-by: Ard Biesheuvel <ardb@...>
---
MdeModulePkg/Include/Ppi/MemoryAttribute.h | 83 ++++++++++++++++++++
MdeModulePkg/MdeModulePkg.dec | 3 +
2 files changed, 86 insertions(+)

diff --git a/MdeModulePkg/Include/Ppi/MemoryAttribute.h b/MdeModulePkg/Incl=
ude/Ppi/MemoryAttribute.h
new file mode 100644
index 0000000000000000..83bcc33a76719712
--- /dev/null
+++ b/MdeModulePkg/Include/Ppi/MemoryAttribute.h
@@ -0,0 +1,83 @@
+/** @file=0D
+=0D
+Copyright (c) 2023, Google LLC. All rights reserved.<BR>=0D
+=0D
+SPDX-License-Identifier: BSD-2-Clause-Patent=0D
+=0D
+**/=0D
+=0D
+#ifndef EDKII_MEMORY_ATTRIBUTE_PPI_H_=0D
+#define EDKII_MEMORY_ATTRIBUTE_PPI_H_=0D
+=0D
+#include <Uefi/UefiSpec.h>=0D
+=0D
+///=0D
+/// Global ID for the EDKII_MEMORY_ATTRIBUTE_PPI.=0D
+///=0D
+#define EDKII_MEMORY_ATTRIBUTE_PPI_GUID \=0D
+ { \=0D
+ 0x1be840de, 0x2d92, 0x41ec, { 0xb6, 0xd3, 0x19, 0x64, 0x13, 0x50, 0x51=
, 0xfb } \=0D
+ }=0D
+=0D
+///=0D
+/// Forward declaration for the EDKII_MEMORY_ATTRIBUTE_PPI.=0D
+///=0D
+typedef struct _EDKII_MEMORY_ATTRIBUTE_PPI EDKII_MEMORY_ATTRIBUTE_PPI;=0D
+=0D
+/**=0D
+ Set the requested memory permission attributes on a region of memory.=0D
+=0D
+ BaseAddress and Length must be aligned to EFI_PAGE_SIZE.=0D
+=0D
+ Attributes must contain a combination of EFI_MEMORY_RP, EFI_MEMORY_RO an=
d=0D
+ EFI_MEMORY_XP, and specifies the attributes that must be set for the=0D
+ region in question. Attributes that are omitted will be cleared from the=
=0D
+ region only if they are set in AttributeMask.=0D
+=0D
+ AttributeMask must contain a combination of EFI_MEMORY_RP, EFI_MEMORY_RO=
and=0D
+ EFI_MEMORY_XP, and specifies the attributes that the call will operate o=
n.=0D
+ AttributeMask must not be 0x0, and must contain at least the bits set in=
=0D
+ Attributes.=0D
+=0D
+ @param[in] This The protocol instance pointer.=0D
+ @param[in] BaseAddress The physical address that is the start add=
ress=0D
+ of a memory region.=0D
+ @param[in] Length The size in bytes of the memory region.=0D
+ @param[in] Attributes Memory attributes to set or clear.=0D
+ @param[in] AttributeMask Mask of memory attributes to operate on.=0D
+=0D
+ @retval EFI_SUCCESS The attributes were set for the memory reg=
ion.=0D
+ @retval EFI_INVALID_PARAMETER Length is zero.=0D
+ AttributeMask is zero.=0D
+ AttributeMask lacks bits set in Attributes=
.=0D
+ BaseAddress or Length is not suitably alig=
ned.=0D
+ @retval EFI_UNSUPPORTED The processor does not support one or more=
=0D
+ bytes of the memory resource range specifi=
ed=0D
+ by BaseAddress and Length.=0D
+ The bit mask of attributes is not supporte=
d for=0D
+ the memory resource range specified by=0D
+ BaseAddress and Length.=0D
+ @retval EFI_OUT_OF_RESOURCES Requested attributes cannot be applied due=
to=0D
+ lack of system resources.=0D
+=0D
+**/=0D
+typedef=0D
+EFI_STATUS=0D
+(EFIAPI *EDKII_MEMORY_ATTRIBUTE_SET_PERMISSIONS)(=0D
+ IN EDKII_MEMORY_ATTRIBUTE_PPI *This,=0D
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,=0D
+ IN UINT64 Length,=0D
+ IN UINT64 Attributes,=0D
+ IN UINT64 AttributeMask=0D
+ );=0D
+=0D
+///=0D
+/// This PPI contains a set of services to manage memory permission attrib=
utes.=0D
+///=0D
+struct _EDKII_MEMORY_ATTRIBUTE_PPI {=0D
+ EDKII_MEMORY_ATTRIBUTE_SET_PERMISSIONS SetPermissions;=0D
+};=0D
+=0D
+extern EFI_GUID gEdkiiMemoryAttributePpiGuid;=0D
+=0D
+#endif=0D
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 =3D { 0x71a9ea61, 0x5a35, 0x4a=
5d, { 0xac, 0xef, 0x9c, 0xf8, 0x6d, 0x6d, 0x67, 0xe0 } }=0D
gEdkiiPeiBootInCapsuleOnDiskModePpiGuid =3D { 0xb08a11e4, 0xe2b7, 0x4b=
75, { 0xb5, 0x15, 0xaf, 0x61, 0x6, 0x68, 0xbf, 0xd1 } }=0D
=0D
+ ## Include/Ppi/MemoryAttribute.h=0D
+ gEdkiiMemoryAttributePpiGuid =3D { 0x1be840de, 0x2d92, 0x41=
ec, { 0xb6, 0xd3, 0x19, 0x64, 0x13, 0x50, 0x51, 0xfb } }=0D
+=0D
[Protocols]=0D
## Load File protocol provides capability to load and unload EFI image i=
nto memory and execute it.=0D
# Include/Protocol/LoadPe32Image.h=0D
--=20
2.39.2

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4468=0D
=0D
This is a followup to the RFC that I sent to the edk2-devel list on the=0D
25th of May.=0D
=0D
In an attempt to make some incremental progress, this v2 only covers the=0D
NX remapping of the DXE stack in DxeIpl, using the newly introduced=0D
memory attributes PPI.=0D
=0D
Other use cases are deferred until we can converge on an approach that=0D
works across architectures and platforms. In particular, this means the=0D
following use cases:=0D
- mapping the DXE core code and data regions RO and XP, respectively;=0D
- mapping shadowed PEIMs read-only (including the PEI core itself);=0D
- managing memory permissions after temporary RAM migration;=0D
- reorganizing the X64 PEI flow with respect to page table allocation;=0D
- managing the dispatch order of the PEIM producing the PPI in relation=0D
to its consumers.=0D
=0D
The current series specifies the PPI in patch #1, and wires it up into=0D
DxeIpl to remap the DXE stack non-executable in a generic manner=0D
(patches #2 and #3)=0D
=0D
Patches #4 and #5 implement the PPI for ARM and AArch64.=0D
=0D
Patch #6 switches ARM and AArch64 over to the generic DxeIpl.=0D
=0D
Patch #7 cleans up the ARM implementation of the UEFI memory attributes=0D
protocol, based on the improvements made in patch #4. =0D
=0D
Changes since RFC (in addition to the above):=0D
- update PPI protype to use attributes+mask instead of setmask+clearmask=0D
- drop OVMF patch for RISC-V that has been applied in the meantime=0D
=0D
Cc: Ray Ni <ray.ni@...>=0D
Cc: Jiewen Yao <jiewen.yao@...>=0D
Cc: Gerd Hoffmann <kraxel@...>=0D
Cc: Taylor Beebe <t@...>=0D
Cc: Oliver Smith-Denny <osd@...>=0D
Cc: Dandan Bi <dandan.bi@...>=0D
Cc: Dun Tan <dun.tan@...>=0D
Cc: Liming Gao <gaoliming@...>=0D
Cc: "Kinney, Michael D" <michael.d.kinney@...>=0D
Cc: Leif Lindholm <quic_llindhol@...>=0D
Cc: Michael Kubacki <mikuback@...>=0D
=0D
Ard Biesheuvel (7):=0D
MdeModulePkg: Define memory attribute PPI=0D
MdeModulePkg/DxeIpl: Merge EBC, RISCV64 and LOONGARCH code=0D
MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX=0D
ArmPkg/ArmMmuLib: Extend API to manage memory permissions better=0D
ArmPkg/CpuPei: Implement the memory attributes PPI=0D
MdeModulePkg/DxeIpl ARM AARCH64: Switch to generic handoff code=0D
ArmPkg/CpuDxe: Simplify memory attributes protocol implementation=0D
=0D
ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 2 +-=0D
ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 50 +---=
-------=0D
ArmPkg/Drivers/CpuPei/CpuPei.c | 76 ++++=
+++++++++++++=0D
ArmPkg/Drivers/CpuPei/CpuPei.inf | 4 +=0D
ArmPkg/Include/Library/ArmMmuLib.h | 36 ++++=
+++-=0D
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 52 ++++=
+++++++-=0D
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c | 88 ++++=
+++++++++++++---=0D
ArmPkg/Library/OpteeLib/Optee.c | 2 +-=0D
MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c | 71 ----=
------------=0D
MdeModulePkg/Core/DxeIplPeim/{Ebc/DxeLoadFunc.c =3D> DxeHandoff.c} | 31 ++=
++++-=0D
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 24 ++--=
--=0D
MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c | 63 ----=
----------=0D
MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c | 75 ----=
-------------=0D
MdeModulePkg/Include/Ppi/MemoryAttribute.h | 83 ++++=
++++++++++++++=0D
MdeModulePkg/MdeModulePkg.dec | 3 +=0D
15 files changed, 366 insertions(+), 294 deletions(-)=0D
delete mode 100644 MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c=0D
rename MdeModulePkg/Core/DxeIplPeim/{Ebc/DxeLoadFunc.c =3D> DxeHandoff.c} =
(62%)=0D
delete mode 100644 MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c=
=0D
delete mode 100644 MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c=0D
create mode 100644 MdeModulePkg/Include/Ppi/MemoryAttribute.h=0D
=0D
-- =0D
2.39.2=0D
=0D

Thanks Ard. That is good news.
We may try the patch to see if that will break X86.

Current blocking issue seems IA32 intrinsic and OVMF size. I am not sure if Gerd has any idea on that.

Thank you
Yao, Jiewen

On Wed, 24 May 2023 at 02:41, Minh Nguyen
<minhnguyen1@...> wrote:

These patches helps to add new SMBIOS Tables (Type 16, 17, 19) and refactor SmbiosPlatformDxe.

Minh Nguyen (4):
JadePkg: Correct PCD names for SMBIOS Type 0
JadePkg: Leverage ArmPkg/Smbios (Type 0, 1, 2, 3, 13, 32)
JadePkg: Refactor SmbiosPlatformDxe
JadePkg: Add support SMBIOS Table Type 16, 17, 19

Pushed as 38170a4175a6..6fe5a2309118

I tried to do a build test of Jade.dsc but I got an error related to
PcdValueInit. Any idea what is going on here?

Silicon/Ampere/AmpereSiliconPkg/AmpereSiliconPkg.dec | 14 +-
Platform/Ampere/JadePkg/Jade.dsc | 7 +-
Platform/Ampere/JadePkg/Jade.fdf | 1 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf | 43 +-
Platform/Ampere/JadePkg/Library/OemMiscLib/OemMiscLib.inf | 15 +-
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.h | 201 +++
Silicon/Ampere/AmpereAltraPkg/Include/Library/AmpereCpuLib.h | 46 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c | 1553 ++++++--------------
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxeDataTable.c | 96 ++
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type08/PlatformPortConnectorData.c | 142 ++
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type08/PlatformPortConnectorFunction.c | 57 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type09/PlatformSystemSlotData.c | 268 ++++
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type09/PlatformSystemSlotFunction.c | 58 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type11/PlatformOemStringData.c | 42 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type11/PlatformOemStringFunction.c | 57 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type16/PlatformPhysicalMemoryArrayData.c | 48 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type16/PlatformPhysicalMemoryArrayFunction.c | 44 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type17/PlatformMemoryDeviceData.c | 63 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type17/PlatformMemoryDeviceFunction.c | 475 ++++++
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type19/PlatformMemoryArrayMappedAddressData.c | 47 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type19/PlatformMemoryArrayMappedAddressFunction.c | 150 ++
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type24/PlatformHardwareSecurityData.c | 42 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type24/PlatformHardwareSecurityFunction.c | 57 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type38/PlatformIpmiDeviceData.c | 46 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type38/PlatformIpmiDeviceFunction.c | 39 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type41/PlatformOnboardDevicesExtendedData.c | 47 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type41/PlatformOnboardDevicesExtendedFunction.c | 57 +
Platform/Ampere/JadePkg/Library/OemMiscLib/OemMiscLib.c | 246 +++-
Silicon/Ampere/AmpereAltraPkg/Library/AmpereCpuLib/AmpereCpuLibCommon.c | 84 ++
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxeStrings.uni | 22 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type08/PlatformPortConnector.uni | 22 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type09/PlatformSystemSlot.uni | 20 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type11/PlatformOemString.uni | 11 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type17/PlatformMemoryDevice.uni | 16 +
Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type41/PlatformOnboardDevicesExtended.uni | 10 +
35 files changed, 3005 insertions(+), 1141 deletions(-)
mode change 100644 => 100755 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.h
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxeDataTable.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type08/PlatformPortConnectorData.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type08/PlatformPortConnectorFunction.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type09/PlatformSystemSlotData.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type09/PlatformSystemSlotFunction.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type11/PlatformOemStringData.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type11/PlatformOemStringFunction.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type16/PlatformPhysicalMemoryArrayData.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type16/PlatformPhysicalMemoryArrayFunction.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type17/PlatformMemoryDeviceData.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type17/PlatformMemoryDeviceFunction.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type19/PlatformMemoryArrayMappedAddressData.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type19/PlatformMemoryArrayMappedAddressFunction.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type24/PlatformHardwareSecurityData.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type24/PlatformHardwareSecurityFunction.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type38/PlatformIpmiDeviceData.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type38/PlatformIpmiDeviceFunction.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type41/PlatformOnboardDevicesExtendedData.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type41/PlatformOnboardDevicesExtendedFunction.c
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxeStrings.uni
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type08/PlatformPortConnector.uni
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type09/PlatformSystemSlot.uni
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type11/PlatformOemString.uni
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type17/PlatformMemoryDevice.uni
create mode 100644 Platform/Ampere/JadePkg/Drivers/SmbiosPlatformDxe/Type41/PlatformOnboardDevicesExtended.uni

--
2.39.0

On Thu, 11 May 2023 at 10:10, Minh Nguyen
<minhnguyen1@...> wrote:

These patches support NVMe Hot Plug feature for Ampere Altra and Ampere Altra Max.

Minh Nguyen (2):
AmpereAltraPkg: Add Hot Plug Slot Capable during PCIe port
initialization
AmpereAltraPkg: Change PCIe Amba Link Timeout value

Vu Nguyen (4):
AmpereAltraPkg: Add PCIe Hot Plug library
JadePkg: Support ACPI tables for Hot Plug of Ampere Altra
JadePkg: Support ACPI tables for Hot Plug of Ampere Altra Max
AmpereAltraPkg: Enable NVMe Hot Plug feature

Pushed as a869bae89a6d..38170a4175a6

Thanks,

Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dec | 8 +-
Silicon/Ampere/AmpereSiliconPkg/AmpereSiliconPkg.dec | 13 +
Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dsc.inc | 1 +
Platform/Ampere/JadePkg/Jade.dsc | 66 ++
Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/PcieInitPei.inf | 3 +-
Silicon/Ampere/AmpereAltraPkg/Library/PcieHotPlugLib/PcieHotPlugLib.inf | 37 +
Silicon/Ampere/AmpereAltraPkg/Include/Library/PcieHotPlugLib.h | 162 +++
Silicon/Ampere/AmpereSiliconPkg/Include/Library/PcieHotPlugPortMapLib.h | 81 ++
Silicon/Ampere/AmpereAltraPkg/Drivers/PcieInitPei/PcieInitPei.c | 3 +
Silicon/Ampere/AmpereAltraPkg/Library/Ac01PcieLib/PcieCore.c | 13 +-
Silicon/Ampere/AmpereAltraPkg/Library/PcieHotPlugLib/PcieHotPlugLib.c | 397 ++++++++
Platform/Ampere/JadePkg/Ac02AcpiTables/CommonDevices.asi | 24 +
Platform/Ampere/JadePkg/Ac02AcpiTables/Dsdt.asl | 511 ++++++++++
Platform/Ampere/JadePkg/Ac02AcpiTables/MHPP.asi | 92 ++
Platform/Ampere/JadePkg/Ac02AcpiTables/PCI-S0.asi | 545 ++++++++++
Platform/Ampere/JadePkg/Ac02AcpiTables/PCI-S1.asi | 1072 +++++++++++++++++++
Platform/Ampere/JadePkg/AcpiTables/Dsdt.asl | 499 ++++++++-
Platform/Ampere/JadePkg/AcpiTables/MHPP.asi | 127 +++
Platform/Ampere/JadePkg/AcpiTables/PCI-S0.asi | 548 +++++++++-
Platform/Ampere/JadePkg/AcpiTables/PCI-S1.asi | 1074 +++++++++++++++++++-
20 files changed, 5269 insertions(+), 7 deletions(-)
create mode 100644 Silicon/Ampere/AmpereAltraPkg/Library/PcieHotPlugLib/PcieHotPlugLib.inf
create mode 100644 Silicon/Ampere/AmpereAltraPkg/Include/Library/PcieHotPlugLib.h
create mode 100644 Silicon/Ampere/AmpereSiliconPkg/Include/Library/PcieHotPlugPortMapLib.h
create mode 100644 Silicon/Ampere/AmpereAltraPkg/Library/PcieHotPlugLib/PcieHotPlugLib.c
create mode 100644 Platform/Ampere/JadePkg/Ac02AcpiTables/MHPP.asi
create mode 100644 Platform/Ampere/JadePkg/AcpiTables/MHPP.asi

--
2.39.0