Re: [edk2-devel] [PATCH v2 0/3] RISC-V: Support Svpbmt extension

[Andrei Warkentin]

I don’t think a warning message around EFI_MEMORY_UC is useful… it’s typical to 
request such a mapping for device MMIO (e.g. PciHostBridgeDxe, 
NonDiscoverablePciDeviceDxe, etc). On a system without PBMT yeah the M mode 
firmware will have to enforce the correct type, but that’s been true until now 
as well. Warning for _WC may make sense as it’s definitely not a “typical” 
mapping.

A

From: Tuan Phan 
Sent: Monday, February 26, 2024 10:34 PM
To: Warkentin, Andrei ; suni...@ventanamicro.com
Cc: devel@edk2.groups.io; Kinney, Michael D ; 
gaolim...@byosoft.com.cn; Liu, Zhiguang ; 
kra...@redhat.com; ler...@redhat.com; Kumar, Rahul R ; 
Ni, Ray ; Yao, Jiewen ; 
ardb+tianoc...@kernel.org
Subject: Re: [edk2-devel] [PATCH v2 0/3] RISC-V: Support Svpbmt extension

Hi Sunil/ Andrei,
Any comments on this series?

Regards,

On Wed, Feb 14, 2024 at 10:16 PM Tuan Phan via groups.io 
mailto:ventanamicro@groups.io>> wrote:


On Wed, Feb 14, 2024 at 9:43 PM Warkentin, Andrei 
mailto:andrei.warken...@intel.com>> wrote:
Do you mind sharing a GH branch with the patch set?
https://github.com/pttuan/edk2/tree/tphan/riscv_mmu_svpbmt
Tuan

A

> -Original Message-
> From: Tuan Phan mailto:tp...@ventanamicro.com>>
> Sent: Tuesday, February 6, 2024 7:29 PM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D 
> mailto:michael.d.kin...@intel.com>>;
> gaolim...@byosoft.com.cn; Liu, Zhiguang 
> mailto:zhiguang@intel.com>>;
> kra...@redhat.com; 
> ler...@redhat.com; Kumar, Rahul R
> mailto:rahul.r.ku...@intel.com>>; Ni, Ray 
> mailto:ray...@intel.com>>;
> suni...@ventanamicro.com; Yao, Jiewen 
> mailto:jiewen@intel.com>>; Warkentin,
> Andrei mailto:andrei.warken...@intel.com>>; 
> ardb+tianoc...@kernel.org; Tuan Phan
> mailto:tp...@ventanamicro.com>>
> Subject: [PATCH v2 0/3] RISC-V: Support Svpbmt extension
>
> This patchset adds support for RISC-V Svpbmt extension.
>
> The GCD EFI_MEMORY_UC and EFI_MEMORY_WC attributes will be mapped to
> IO and NC mode defined in PBMT field.
>
> v2:
>   - Generated patch for each package.
>
> Tuan Phan (3):
>   MdePkg.dec: RISC-V: Define override bit for Svpbmt extension
>   UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension
>   OvmfPkg/RiscVVirt: Override Svpbmt extension
>
>  MdePkg/MdePkg.dec |  2 ++
>  OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc   |  2 +-
>  .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 25 ++-
>  .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf   |  1 +
>  4 files changed, 28 insertions(+), 2 deletions(-)
>
> --
> 2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116290): https://edk2.groups.io/g/devel/message/116290
Mute This Topic: https://groups.io/mt/104211191/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 1/1] .github/workflows/codeql.yml: Update actions being deprecated

[Michael D Kinney]

Reviewed-by: Michael D Kinney 

> -Original Message-
> From: mikub...@linux.microsoft.com 
> Sent: Friday, March 1, 2024 3:33 PM
> To: devel@edk2.groups.io
> Cc: Sean Brogan ; Joey Vagedes
> ; Kinney, Michael D
> 
> Subject: [PATCH v2 1/1] .github/workflows/codeql.yml: Update actions
> being deprecated
> 
> From: Michael Kubacki 
> 
> Currently CodeQL runs have the following warnings:
> 
>   Node.js 16 actions are deprecated. Please update the following
>   actions to use Node.js 20: actions/setup-python@v4,
>   actions/upload-artifact@v3, actions/cache@v3. For more information
>   see:
>   https://github.blog/changelog/2023-09-22-github-actions-
> transitioning-from-node-16-to-node-20/.
> 
> And:
> 
>   CodeQL Action v2 will be deprecated on December 5th, 2024. Please
>   update all occurrences of the CodeQL Action in your workflow files
>   to v3. For more information, see:
>   https://github.blog/changelog/2024-01-12-code-scanning-deprecation-
> of-codeql-action-v2/
> 
> The first is resolved by updating the actions to the latest versions
> that were released to use Node.js 20. The second is specifically
> referring to the codeql-action/upload-sarif action which is at v2.
> 
> This change updates all of the actions to the latest releases to
> prevent deprecated versions from continuing to be used.
> 
> ---
> 
> The following breaking change was noted in actions/upload-artifact
> that caused some related changes in the workflow:
> 
>   "Due to how Artifacts are created in this new version, it is no
>longer possible to upload to the same named Artifact multiple
>times. You must either split the uploads into multiple Artifacts
>with different names, or only upload once. Otherwise you will
>encounter an error."
> 
> This workflow depended on that behavior previously to append multiple
> logs (e.g. setup log, update log, build log) to the same named
> artifact (named per package). These were appended after each operation
> so they are readily available if the operation failed and no further
> actions are run.
> 
> Now the artifacts must be unique in name. The hyphenation comes in
> because edk2 further builds some packages with both architectures in
> a single build vs separate builds (e.g. IA32 and X64 vs IA32,X64). To
> uniquely name artifacts resulting from those builds, the architecture
> is also placed in the artifact name. For builds with multiple
> architectures the artifact name captures each architecture separated
> by a hyphen.
> 
> Cc: Sean Brogan 
> Cc: Joey Vagedes 
> Cc: Michael D Kinney 
> Signed-off-by: Michael Kubacki 
> ---
> 
> Notes:
> v2 Change:
> 
> Add detailed explanation in the commit message about changes
> in the workflow file.
> 
>  .github/workflows/codeql.yml | 37 +---
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/.github/workflows/codeql.yml
> b/.github/workflows/codeql.yml
> index c91e9d4dbeb3..e0c5f69f6cdf 100644
> --- a/.github/workflows/codeql.yml
> +++ b/.github/workflows/codeql.yml
> @@ -79,7 +79,7 @@ jobs:
>uses: actions/checkout@v4
> 
>  - name: Install Python
> -  uses: actions/setup-python@v4
> +  uses: actions/setup-python@v5
>with:
>  python-version: '3.11'
>  cache: 'pip'
> @@ -136,15 +136,26 @@ jobs:
> 
> print(f'ci_setup_supported={str(ci_setup_supported).lower()}', file=fh)
>  print(f'setup_supported={str(setup_supported).lower()}',
> file=fh)
> 
> +- name: Convert Arch to Log Format
> +  id: convert_arch_hyphen
> +  env:
> +ARCH_LIST: ${{ matrix.ArchList }}
> +  shell: python
> +  run: |
> +import os
> +
> +with open(os.environ['GITHUB_OUTPUT'], 'a') as fh:
> +print(f'arch_list={os.environ["ARCH_LIST"].replace(",", "-
> ")}', file=fh)
> +
>  - name: Setup
>if: steps.get_ci_file_operations.outputs.setup_supported ==
> 'true'
>run: stuart_setup -c .pytool/CISettings.py -t DEBUG -a ${{
> matrix.ArchList }} TOOL_CHAIN_TAG=VS2019
> 
>  - name: Upload Setup Log As An Artifact
> -  uses: actions/upload-artifact@v3
> +  uses: actions/upload-artifact@v4
>if: (success() || failure()) &&
> steps.get_ci_file_operations.outputs.setup_supported == 'true'
>with:
> -name: ${{ matrix.Package }}-Logs
> +name: ${{ matrix.Package }}-${{
> steps.convert_arch_hyphen.outputs.arch_list }}-Setup-Log
>  path: |
>**/SETUPLOG.txt
>retention-days: 7
> @@ -155,10 +166,10 @@ jobs:
>run: stuart_ci_setup -c .pytool/CISettings.py -t DEBUG -a ${{
> matrix.ArchList }} TOOL_CHAIN_TAG=VS2019
> 
>  - name: Upload CI Setup Log As An Artifact
> -  uses: actions/upload-artifact@v3
> +  uses: actions/upload-artifact@v4
>if: (success() || failure()) &&
> steps.get_ci_file_operations.outputs.ci_setup_supported == 'true'
>with:
> -name: ${{ matrix.Package }}-Logs
> +name: 

[edk2-devel] [PATCH v2 1/1] .github/workflows/codeql.yml: Update actions being deprecated

[Michael Kubacki]

From: Michael Kubacki 

Currently CodeQL runs have the following warnings:

  Node.js 16 actions are deprecated. Please update the following
  actions to use Node.js 20: actions/setup-python@v4,
  actions/upload-artifact@v3, actions/cache@v3. For more information
  see:
  
https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/.

And:

  CodeQL Action v2 will be deprecated on December 5th, 2024. Please
  update all occurrences of the CodeQL Action in your workflow files
  to v3. For more information, see:
  
https://github.blog/changelog/2024-01-12-code-scanning-deprecation-of-codeql-action-v2/

The first is resolved by updating the actions to the latest versions
that were released to use Node.js 20. The second is specifically
referring to the codeql-action/upload-sarif action which is at v2.

This change updates all of the actions to the latest releases to
prevent deprecated versions from continuing to be used.

---

The following breaking change was noted in actions/upload-artifact
that caused some related changes in the workflow:

  "Due to how Artifacts are created in this new version, it is no
   longer possible to upload to the same named Artifact multiple
   times. You must either split the uploads into multiple Artifacts
   with different names, or only upload once. Otherwise you will
   encounter an error."

This workflow depended on that behavior previously to append multiple
logs (e.g. setup log, update log, build log) to the same named
artifact (named per package). These were appended after each operation
so they are readily available if the operation failed and no further
actions are run.

Now the artifacts must be unique in name. The hyphenation comes in
because edk2 further builds some packages with both architectures in
a single build vs separate builds (e.g. IA32 and X64 vs IA32,X64). To
uniquely name artifacts resulting from those builds, the architecture
is also placed in the artifact name. For builds with multiple
architectures the artifact name captures each architecture separated
by a hyphen.

Cc: Sean Brogan 
Cc: Joey Vagedes 
Cc: Michael D Kinney 
Signed-off-by: Michael Kubacki 
---

Notes:
v2 Change:

Add detailed explanation in the commit message about changes
in the workflow file.

 .github/workflows/codeql.yml | 37 +---
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml
index c91e9d4dbeb3..e0c5f69f6cdf 100644
--- a/.github/workflows/codeql.yml
+++ b/.github/workflows/codeql.yml
@@ -79,7 +79,7 @@ jobs:
   uses: actions/checkout@v4
 
 - name: Install Python
-  uses: actions/setup-python@v4
+  uses: actions/setup-python@v5
   with:
 python-version: '3.11'
 cache: 'pip'
@@ -136,15 +136,26 @@ jobs:
 print(f'ci_setup_supported={str(ci_setup_supported).lower()}', 
file=fh)
 print(f'setup_supported={str(setup_supported).lower()}', file=fh)
 
+- name: Convert Arch to Log Format
+  id: convert_arch_hyphen
+  env:
+ARCH_LIST: ${{ matrix.ArchList }}
+  shell: python
+  run: |
+import os
+
+with open(os.environ['GITHUB_OUTPUT'], 'a') as fh:
+print(f'arch_list={os.environ["ARCH_LIST"].replace(",", "-")}', 
file=fh)
+
 - name: Setup
   if: steps.get_ci_file_operations.outputs.setup_supported == 'true'
   run: stuart_setup -c .pytool/CISettings.py -t DEBUG -a ${{ 
matrix.ArchList }} TOOL_CHAIN_TAG=VS2019
 
 - name: Upload Setup Log As An Artifact
-  uses: actions/upload-artifact@v3
+  uses: actions/upload-artifact@v4
   if: (success() || failure()) && 
steps.get_ci_file_operations.outputs.setup_supported == 'true'
   with:
-name: ${{ matrix.Package }}-Logs
+name: ${{ matrix.Package }}-${{ 
steps.convert_arch_hyphen.outputs.arch_list }}-Setup-Log
 path: |
   **/SETUPLOG.txt
   retention-days: 7
@@ -155,10 +166,10 @@ jobs:
   run: stuart_ci_setup -c .pytool/CISettings.py -t DEBUG -a ${{ 
matrix.ArchList }} TOOL_CHAIN_TAG=VS2019
 
 - name: Upload CI Setup Log As An Artifact
-  uses: actions/upload-artifact@v3
+  uses: actions/upload-artifact@v4
   if: (success() || failure()) && 
steps.get_ci_file_operations.outputs.ci_setup_supported == 'true'
   with:
-name: ${{ matrix.Package }}-Logs
+name: ${{ matrix.Package }}-${{ 
steps.convert_arch_hyphen.outputs.arch_list }}-CI-Setup-Log
 path: |
   **/CISETUP.txt
   retention-days: 7
@@ -168,10 +179,10 @@ jobs:
   run: stuart_update -c .pytool/CISettings.py -t DEBUG -a ${{ 
matrix.ArchList }} TOOL_CHAIN_TAG=VS2019
 
 - name: Upload Update Log As An Artifact
-  uses: actions/upload-artifact@v3
+  uses: actions/upload-artifact@v4
   if: success() || failure()
   with:
-name: ${{ matrix.Package }}-Logs
+name: ${{ matrix.Package 

Re: [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension

[Tuan Phan]

Thanks for the detailed review. Please see my comments below.

On Fri, Mar 1, 2024 at 4:14 AM Laszlo Ersek  wrote:

> On 3/1/24 02:29, Tuan Phan wrote:
> > The GCD EFI_MEMORY_UC and EFI_MEMORY_WC memory attributes will be
> > supported when Svpbmt extension available.
> >
> > Cc: Gerd Hoffmann 
> > Cc: Laszlo Ersek 
> > Cc: Rahul Kumar 
> > Cc: Ray Ni 
> > Signed-off-by: Tuan Phan 
> > ---
> >  .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 101 +++---
> >  .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf   |   1 +
> >  2 files changed, 88 insertions(+), 14 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > index 826a1d32a1d4..f4419bb8f380 100644
> > --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > @@ -36,6 +36,11 @@
> >  #define PTE_PPN_SHIFT 10
> >  #define RISCV_MMU_PAGE_SHIFT  12
> >
> > +#define RISCV_CPU_FEATURE_PBMT_BITMASK  BIT2
> > +#define PTE_PBMT_NC BIT61
> > +#define PTE_PBMT_IO BIT62
> > +#define PTE_PBMT_MASK   (PTE_PBMT_NC | PTE_PBMT_IO)
> > +
> >  STATIC UINTN  mModeSupport[] = { SATP_MODE_SV57, SATP_MODE_SV48,
> SATP_MODE_SV39, SATP_MODE_OFF };
> >  STATIC UINTN  mMaxRootTableLevel;
> >  STATIC UINTN  mBitPerLevel;
> > @@ -489,32 +494,89 @@ UpdateRegionMapping (
> >  /**
> >Convert GCD attribute to RISC-V page attribute.
> >
> > -  @param  GcdAttributes The GCD attribute.
> > +  @param  GcdAttributes   The GCD attribute.
> > +  @param  RiscVAttribtues The pointer of RISC-V page attribute.
> >
> > -  @return   The RISC-V page attribute.
> > +  @retval EFI_INVALID_PARAMETER   The RiscVAttribtues is NULL or cache
> type mask not valid.
> > +  @retval EFI_SUCCESS The operation succesfully.
> >
> >  **/
> >  STATIC
> > -UINTN
> > +EFI_STATUS
> >  GcdAttributeToPageAttribute (
> > -  IN UINTN  GcdAttributes
> > +  IN UINTN   GcdAttributes,
>
> Just noticing: why is GcdAttributes *not* UINT64 in the first place?
>
> All the bit macros we test against it, such as EFI_MEMORY_RO
> (0x0002ULL) are of type unsigned long long (UINT64).
>
Good catch. Will fix it.

>
> > +  OUT UINTN  *RiscVAttributes
> >)
> >  {
> > -  UINTN  RiscVAttributes;
> > +  UINT64   CacheTypeMask;
> > +  BOOLEAN  PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) &
> RISCV_CPU_FEATURE_PBMT_BITMASK) ? TRUE : FALSE;
>
> - Per the edk2 coding style, locals should not be initialized (separate
> assignment is needed).
>
> - Bitmask checks always need an explicit comparison, such as
>
>   ((a & b) != 0)
>
> or similar. Implicitly interpreting (a & b) as a truth value is not
> appropriate.
>
> - "(whatever) ? TRUE : FALSE" is both bad style and unnecessary.
>
>   BOOLEAN  PmbtExtEnabled;
>
>   PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) &
> RISCV_CPU_FEATURE_PBMT_BITMASK) != 0;
>
> Will fix it.

> >
> > -  RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> > +  if (!RiscVAttributes) {
>
> - The coding style requires an explicit nullity check:
>
>   if (RiscVAttributes == NULL) {
>
Will fix it.

>
> > +return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> >
> >// Determine protection attributes
> >if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> > -RiscVAttributes &= ~(RISCV_PG_W);
> > +*RiscVAttributes &= ~(RISCV_PG_W);
> >}
> >
> >// Process eXecute Never attribute
> >if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
> > -RiscVAttributes &= ~RISCV_PG_X;
> > +*RiscVAttributes &= ~RISCV_PG_X;
> > +  }
> > +
>
> My next comment is unrelated to the patch, it's just something that
> catches my eye, and I think is worth fixing:
>
> RISCV_PG_W is BIT2 (0x0004), and RISCV_PG_X is BIT3 (0x0008).
> Meaning, they are of type *signed int* (INT32). Applying the bit-neg
> operator on them produces a negative value (because it flips the sign
> bit), which is very ugly.
>
> I suggest a separate patch for changing these into
>
>   ~(UINTN)RISCV_PG_W
>   ~(UINTN)RISCV_PG_X
>
> Alternatively, you could do
>
Will fix it in a separate patch along with the above change.

>
>   *RiscVAttributes = RISCV_PG_R;
>   if ((GcdAttributes & EFI_MEMORY_RO) == 0) {
> *RiscVAttributes |= RISCV_PG_W;
>   }
>   if ((GcdAttributes & EFI_MEMORY_XP) == 0) {
> *RiscVAttributes |= RISCV_PG_X;
>   }
>
> Either way: separate patch.
>
> > +  CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK;
> > +  if ((CacheTypeMask != 0) &&
> > +  (((CacheTypeMask - 1) & CacheTypeMask) != 0))
>
> This is not what I recommended in my previous review
> .
>
> Compare:
>
>   (CacheTypeMask != 0) && ...
>
> versus
>
>   (CacheTypeMask == 0) || ...
>
> Both of these ensure that the power-of-two check in the second
> 

Re: [edk2-devel] [PATCH v2 00/10] OvmfPkg/ResetVector: cleanup and add 5-level paging support.

[Laszlo Ersek]

On 3/1/24 08:43, Gerd Hoffmann wrote:
> So I ran with the suggestion by Laszlo to move the page table setup into
> macros and untangle the non-CoCo / TDX / SEV code paths.  The first five
> patches of the series are doing that (without functional changes).
> 
> Support for 5-level paging is added by the following five patches.  This
> way it is indeed easier to understand.  Additional bonus is that the
> patches can be splitted into smaller pieces and 5-level paging for the
> three cases (non-CoCo / TDX / SEC) can be enabled independently.
> 
> The SEV patches (#9 + #10) are included here for completeness, but it is
> probably a good idea to merge them only after 5-level paging support was
> added to BaseMemEncryptSevLib.  This way we can turn on 5-level paging
> support without breaking SEV.
> 
> v2 changes:
>  - remove SetCr3La57 label, use Enable5LevelPaging macro instead.
>  - turn GetSevCBitMaskAbove31 into a macro.
>  - comment fixes.
> 
> Gerd Hoffmann (10):
>   OvmfPkg/ResetVector: improve page table flag names
>   OvmfPkg/ResetVector: add ClearOvmfPageTables macro
>   OvmfPkg/ResetVector: add CreatePageTables4Level macro
>   OvmfPkg/ResetVector: split TDX BSP workflow
>   OvmfPkg/ResetVector: split SEV and non-CoCo workflows
>   OvmfPkg/ResetVector: add 5-level paging support
>   OvmfPkg/ResetVector: print post codes for 4/5 level paging
>   OvmfPkg/ResetVector: wire up 5-level paging for TDX
>   OvmfPkg/ResetVector: leave SEV VC handler installed longer
>   OvmfPkg/ResetVector: wire up 5-level paging for SEV
> 
>  OvmfPkg/ResetVector/ResetVector.inf   |   1 +
>  OvmfPkg/ResetVector/Ia32/AmdSev.asm   |  40 ++-
>  OvmfPkg/ResetVector/Ia32/IntelTdx.asm |  17 +-
>  OvmfPkg/ResetVector/Ia32/PageTables64.asm | 299 +-
>  OvmfPkg/ResetVector/Main.asm  |   4 +
>  OvmfPkg/ResetVector/ResetVector.nasmb |   5 +-
>  6 files changed, 272 insertions(+), 94 deletions(-)
> 

Patches 1 through 8 have been merged as

 8  fded08e74400 OvmfPkg/ResetVector: improve page table flag names
 9  52e44713d23d OvmfPkg/ResetVector: add ClearOvmfPageTables macro
10  4329b5b0cd58 OvmfPkg/ResetVector: add CreatePageTables4Level macro
11  b7a97bfac528 OvmfPkg/ResetVector: split TDX BSP workflow
12  e3bd782373d8 OvmfPkg/ResetVector: split SEV and non-CoCo workflows
13  49b7faba1d6e OvmfPkg/ResetVector: add 5-level paging support
14  318b0d714a7e OvmfPkg/ResetVector: print post codes for 4/5 level paging
15  275d0a39c42a OvmfPkg/ResetVector: wire up 5-level paging for TDX

via .

In patch #8, I moved the "CheckForSev:" label into "%if PG_5_LEVEL" scope, as 
discussed.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116285): https://edk2.groups.io/g/devel/message/116285
Mute This Topic: https://groups.io/mt/104660109/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] UefiCpuPkg/CpuPageTableLib: Fix IN OUT parameters marked as IN

[Laszlo Ersek]

On 2/22/24 03:39, Zhou Jianfeng wrote:
> Some IN OUT parameters in CpuPageTableMap.c were mistakenly marked as IN.
> "IN" replaced with "IN OUT" in the following interfaces:
> 
> PageTableLibSetPte4K(): Pte4K
> PageTableLibSetPleB():  PleB
> PageTableLibSetPle():   Ple
> PageTableLibSetPnle():  Pnle
> 
> Reviewed-by: Ray Ni 
> Signed-off-by: Zhou Jianfeng 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Cc: Rahul Kumar 
> Cc: Gerd Hoffmann 
> ---
>  .../Library/CpuPageTableLib/CpuPageTableMap.c | 32 +--
>  1 file changed, 16 insertions(+), 16 deletions(-)

Merged as

 1  d159e229132b UefiCpuPkg/CpuPageTableLib: Fix IN OUT parameters marked 
as IN

via .

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116284): https://edk2.groups.io/g/devel/message/116284
Mute This Topic: https://groups.io/mt/104524856/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] MdeModulePkg/Core/Pei: Improve the copy performance

[Laszlo Ersek]

On 3/1/24 08:11, Li, Zhihao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4697
> 
> EvacuateTempRam function will copy the temporary memory context to the rebased
> pages and the raw pages. Migrations of rebased PEIMs is from cache to memory,
> while raw PEIMs is from memory to memory. So the migrations of raw PEIMs
> is slower than rebased PEIMs. Experimental data indicates that changing the 
> source
> address of raw PEIMs migration will improve performance by 35%.
> 
> Cc: Liming Gao 
> Signed-off-by: Zhihao Li 
> ---
>  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c 
> b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> index 4cd8c843cd..ca37bde482 100644
> --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> @@ -1,7 +1,7 @@
>  /** @file
>EFI PEI Core dispatch services
>  
> -Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2024, Intel Corporation. All rights reserved.
>  (C) Copyright 2016 Hewlett Packard Enterprise Development LP
>  SPDX-License-Identifier: BSD-2-Clause-Patent
>  
> @@ -1305,7 +1305,7 @@ EvacuateTempRam (
>  );
>  ASSERT_EFI_ERROR (Status);
>  RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER 
> *)(UINTN)FvHeaderAddress;
> -CopyMem (RawDataFvHeader, MigratedFvHeader, 
> (UINTN)FvHeader->FvLength);
> +CopyMem (RawDataFvHeader, FvHeader, (UINTN)FvHeader->FvLength);
>  MigratedFvInfo.FvDataBase = (UINT32)(UINTN)RawDataFvHeader;
>}
>  

Merged as

 7  371940932d29 MdeModulePkg/Core/Pei: Improve the copy performance

via .

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116283): https://edk2.groups.io/g/devel/message/116283
Mute This Topic: https://groups.io/mt/104659917/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 0/4] Support to unregister SMI handler inside SMI handler

[Laszlo Ersek]

On 3/1/24 04:01, Zhiguang Liu wrote:
> This patch set is to support to unregister SMI handler inside SMI handler,
> also add check to not allow unregister SMI handler in other SMI handler.
> This patch set also have the same logic in StandaloneMmPkg.
> Because no change on the first patch, I kept the R-B for it.
> 
> V3:
> Minor change on patch #2 and patch #4:
> gCurrentSmiHandler -> mCurrentSmiHandler
> change ASSERT to return EFI_INVALID_PARAMETER
> 
> Zhiguang Liu (4):
>   MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler
>   MdeModulePkg/SMM: Disallow unregister SMI handler in other SMI handler
>   StandaloneMmPkg: Support to unregister MMI handler inside MMI handler
>   StandaloneMmPkg: Disallow unregister MMI handler in other MMI handler
> 
>  MdeModulePkg/Core/PiSmmCore/Smi.c | 37 ++---
>  StandaloneMmPkg/Core/Mmi.c| 39 +++
>  2 files changed, 57 insertions(+), 19 deletions(-)
> 

Merged as

 3  ae1079b386a5 MdeModulePkg/SMM: Support to unregister SMI handler inside 
SMI handler
 4  17b28722008e MdeModulePkg/SMM: Disallow unregister SMI handler in other 
SMI handler
 5  049ff6c39c73 StandaloneMmPkg: Support to unregister MMI handler inside 
MMI handler
 6  2ec8f0c6407f StandaloneMmPkg: Disallow unregister MMI handler in other 
MMI handler

via .

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116282): https://edk2.groups.io/g/devel/message/116282
Mute This Topic: https://groups.io/mt/104657663/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile

[Laszlo Ersek]

On 3/1/24 03:54, Zhou Jianfeng wrote:
> Add volatile qualifier to page table related variable to prevent
> compiler from optimizing away the variables which may lead to
> unexpected result.
> 
> Signed-off-by: Zhou Jianfeng 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Cc: Rahul Kumar 
> Cc: Gerd Hoffmann 
> Cc: Pedro Falcato 
> Cc: Zhang Di 
> Cc: Tan Dun 
> Cc: Michael Brown 
> ---
>  .../Library/CpuPageTableLib/CpuPageTableMap.c | 36 +--
>  1 file changed, 18 insertions(+), 18 deletions(-)

Merged as

 2  dcffad2491bc UefiCpuPkg/CpuPageTableLib: qualify page table accesses as 
volatile

via .

Unfortunately, the patch was formatted and/or posted incorrectly; git-am 
complained about "corrupt patch", no matter what I did about the 
quoted-printable encoding.

Therefore I had to manually reconstruct the patch (I noted that fact on the 
commit message).

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116281): https://edk2.groups.io/g/devel/message/116281
Mute This Topic: https://groups.io/mt/104661494/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 00/10] OvmfPkg/ResetVector: cleanup and add 5-level paging support.

[Laszlo Ersek]

On 3/1/24 08:43, Gerd Hoffmann wrote:
> [...]

For future patch submissions: please include the Cc: tags in the commit
message bodies; PatchCheck.py (also part of CI) enforces that now.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116280): https://edk2.groups.io/g/devel/message/116280
Mute This Topic: https://groups.io/mt/104660109/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 09/10] OvmfPkg/ResetVector: leave SEV VC handler installed longer

[Laszlo Ersek]

On 3/1/24 15:52, Gerd Hoffmann wrote:
>   Hi,
> 
>>>  OneTimeCall CheckSevFeatures
>>> +cmp byte[WORK_AREA_GUEST_TYPE], 1
>>> +jnz NoSevIa32
>>> +OneTimeCall SevClearVcHandlerAndStack
>>>  
>>> +NoSevIa32:
>>>  ;
>>>  ; Restore initial EAX value into the EAX register
>>>  ;
>>
>> Did you miss Tom's review under v1?
>>
>> https://edk2.groups.io/g/devel/message/116176
> 
> Saw the mail only after sending out v2, updated my local branch
> meanwhile.
> 
>> I'm ready to merge this (adding Tom's R-b, if you, Gerd, confirm that
>> that's what you want).
> 
> As stated in the cover letter I think it's better to not (yet) merge
> patches 9+10 because BaseMemEncryptSevLib is not ready for 5-level
> paging.  That way SEV will work fine (in 4-level paging mode) even when
> building with PcdUse5LevelPageTable=TRUE.

Got it!

Laszlo

> 
> thanks & take care,
>   Gerd
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116279): https://edk2.groups.io/g/devel/message/116279
Mute This Topic: https://groups.io/mt/104660115/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] 回复: [PATCH v1] MdeModulePkg/Core/Pei: Improve the copy performance

[Michael Kubacki]

This looks fine.

Reviewed-by: Michael Kubacki 

On 3/1/2024 7:58 AM, gaoliming wrote:

I agree this change. It should have no negative impact. Reviewed-by: Liming
Gao 

Michael:
   Have you any comments for this change?

Thanks
Liming

-邮件原件-
发件人: Zhihao Li 
发送时间: 2024年3月1日 15:12
收件人: devel@edk2.groups.io
抄送: Liming Gao 
主题: [PATCH v1] MdeModulePkg/Core/Pei: Improve the copy performance

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4697

EvacuateTempRam function will copy the temporary memory context to the
rebased
pages and the raw pages. Migrations of rebased PEIMs is from cache to
memory,
while raw PEIMs is from memory to memory. So the migrations of raw PEIMs
is slower than rebased PEIMs. Experimental data indicates that changing

the

source
address of raw PEIMs migration will improve performance by 35%.

Cc: Liming Gao 
Signed-off-by: Zhihao Li 
---
  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
index 4cd8c843cd..ca37bde482 100644
--- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
+++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
@@ -1,7 +1,7 @@
  /** @file
EFI PEI Core dispatch services

-Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2024, Intel Corporation. All rights reserved.
  (C) Copyright 2016 Hewlett Packard Enterprise Development LP
  SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -1305,7 +1305,7 @@ EvacuateTempRam (
  );
  ASSERT_EFI_ERROR (Status);
  RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
*)(UINTN)FvHeaderAddress;
-CopyMem (RawDataFvHeader, MigratedFvHeader,
(UINTN)FvHeader->FvLength);
+CopyMem (RawDataFvHeader, FvHeader,
(UINTN)FvHeader->FvLength);
  MigratedFvInfo.FvDataBase =
(UINT32)(UINTN)RawDataFvHeader;
}

--
2.26.2.windows.1






-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116278): https://edk2.groups.io/g/devel/message/116278
Mute This Topic: https://groups.io/mt/104662794/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms][PATCH v4 8/8] Platform/Sgi: Add CPPC support for RD-Fremont platform

[Prabin CA]

Enable ACPI CPPC mechanism for RD-Fremont as defined by the ACPI
specification. The implementation uses AMU registers accessible as
Fixed-feature Hardware (FFixedHW) for monitoring the performance.
Non-secure SCMI fastchannels are used to communicate with LCP to set
the desired performance. RD-Fremont platform does not support CPPC
revision 1 and below. So update the _OSC method to let OSPM know about
this fact.

Signed-off-by: Prabin CA 
---
 Platform/ARM/SgiPkg/AcpiTables/RdFremontAcpiTables.inf |   1 +
 Platform/ARM/SgiPkg/AcpiTables/RdFremont/Dsdt.asl  | 162 

 2 files changed, 163 insertions(+)

diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdFremontAcpiTables.inf 
b/Platform/ARM/SgiPkg/AcpiTables/RdFremontAcpiTables.inf
index 7556c1239116..fcaa3299c4ea 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/RdFremontAcpiTables.inf
+++ b/Platform/ARM/SgiPkg/AcpiTables/RdFremontAcpiTables.inf
@@ -48,6 +48,7 @@ [FixedPcd]
   gArmSgiTokenSpaceGuid.PcdGpioController0Interrupt
   gArmSgiTokenSpaceGuid.PcdGtFrame0Gsiv
   gArmSgiTokenSpaceGuid.PcdGtFrame1Gsiv
+  gArmSgiTokenSpaceGuid.PcdOscCppcEnable
   gArmSgiTokenSpaceGuid.PcdOscLpiEnable
   gArmSgiTokenSpaceGuid.PcdSp804DualTimerBaseAddress
   gArmSgiTokenSpaceGuid.PcdSp804DualTimerSize
diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdFremont/Dsdt.asl 
b/Platform/ARM/SgiPkg/AcpiTables/RdFremont/Dsdt.asl
index f921eeb2d99e..b9aca8477ca4 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/RdFremont/Dsdt.asl
+++ b/Platform/ARM/SgiPkg/AcpiTables/RdFremont/Dsdt.asl
@@ -11,6 +11,10 @@
 *   - ACPI 6.5, Chapter 8, Section 8.4.3, Lower Power Idle States
 *   - Arm Functional Fixed Hardware Specification v1.2, Chapter 3, Section 3.1,
 * Idle management and Low Power Idle states
+*   - ACPI 6.5, Chapter 8, Section 8.4.6, Collaborative Processor Performance
+* Control
+*   - Arm Functional Fixed Hardware Specification v1.2, Chapter 3, Section 3.2,
+* Performance management and Collaborative Processor Performance Control
 *
 **/
 
@@ -43,6 +47,20 @@ DefinitionBlock ("DsdtTable.aml", "DSDT", 2, "ARMLTD", 
"ARMSGI",
   Or (STS0, OSC_STS_CAPABILITY_MASKED, STS0)
 }
   }
+
+  If (And (CAP0, OSC_CAP_CPPC_SUPPORT)) {
+/* CPPC revision 1 and below not supported */
+And (CAP0, Not (OSC_CAP_CPPC_SUPPORT), CAP0)
+Or (STS0, OSC_STS_CAPABILITY_MASKED, STS0)
+  }
+
+  If (And (CAP0, OSC_CAP_CPPC2_SUPPORT)) {
+if (LEqual (FixedPcdGet32 (PcdOscCppcEnable), Zero)) {
+  And (CAP0, Not (OSC_CAP_CPPC2_SUPPORT), CAP0)
+  Or (STS0, OSC_STS_CAPABILITY_MASKED, STS0)
+}
+  }
+
 } Else {
   And (STS0, Not (OSC_STS_MASK), STS0)
   Or (STS0, Or (OSC_STS_FAILURE, OSC_STS_UNRECOGNIZED_REV), STS0)
@@ -116,6 +134,15 @@ DefinitionBlock ("DsdtTable.aml", "DSDT", 2, "ARMLTD", 
"ARMSGI",
 Name (_UID, 0)
 Name (_STA, 0xF)
 
+Name (_CPC, Package()
+  CPPC_PACKAGE_INIT (0x200093000, 0x200093004, 20, 160, 160, 85, 85, 5)
+)
+
+Name (_PSD, Package () {
+  Package ()
+PSD_INIT (0)
+})
+
 Method (_LPI, 0, NotSerialized) {
   Return (\_SB.PLPI)
 }
@@ -131,6 +158,15 @@ DefinitionBlock ("DsdtTable.aml", "DSDT", 2, "ARMLTD", 
"ARMSGI",
 Name (_UID, 1)
 Name (_STA, 0xF)
 
+Name (_CPC, Package()
+  CPPC_PACKAGE_INIT (0x200293000, 0x200293004, 20, 160, 160, 85, 85, 5)
+)
+
+Name (_PSD, Package () {
+  Package ()
+PSD_INIT (1)
+})
+
 Method (_LPI, 0, NotSerialized) {
   Return (\_SB.PLPI)
 }
@@ -146,6 +182,15 @@ DefinitionBlock ("DsdtTable.aml", "DSDT", 2, "ARMLTD", 
"ARMSGI",
 Name (_UID, 2)
 Name (_STA, 0xF)
 
+Name (_CPC, Package()
+  CPPC_PACKAGE_INIT (0x200493000, 0x200493004, 20, 160, 160, 85, 85, 5)
+)
+
+Name (_PSD, Package () {
+  Package ()
+PSD_INIT (2)
+})
+
 Method (_LPI, 0, NotSerialized) {
   Return (\_SB.PLPI)
 }
@@ -161,6 +206,15 @@ DefinitionBlock ("DsdtTable.aml", "DSDT", 2, "ARMLTD", 
"ARMSGI",
 Name (_UID, 3)
 Name (_STA, 0xF)
 
+Name (_CPC, Package()
+  CPPC_PACKAGE_INIT (0x200693000, 0x200693004, 20, 160, 160, 85, 85, 5)
+)
+
+Name (_PSD, Package () {
+  Package ()
+PSD_INIT (3)
+})
+
 Method (_LPI, 0, NotSerialized) {
   Return (\_SB.PLPI)
 }
@@ -176,6 +230,15 @@ DefinitionBlock ("DsdtTable.aml", "DSDT", 2, "ARMLTD", 
"ARMSGI",
 Name (_UID, 4)
 Name (_STA, 0xF)
 
+Name (_CPC, Package()
+  CPPC_PACKAGE_INIT (0x200893000, 0x200893004, 20, 160, 160, 85, 85, 5)
+)
+
+Name (_PSD, Package () {
+  Package ()
+PSD_INIT (4)
+})
+
 Method (_LPI, 0, 

[edk2-devel] [edk2-platforms][PATCH v4 7/8] Platform/Sgi: Low Power Idle States for RD-Fremont

[Prabin CA]

RD-Fremont platform supports two LPI states, LPI1 (Standby WFI) and LPI3
(Power-down). The cluster supports LPI2 (Power-down) state. The LPI
implementation also supports combined power state for core and cluster.

Signed-off-by: Prabin CA 
---
 Platform/ARM/SgiPkg/AcpiTables/RdFremontAcpiTables.inf |   1 +
 Platform/ARM/SgiPkg/AcpiTables/RdFremont/Dsdt.asl  | 154 

 2 files changed, 155 insertions(+)

diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdFremontAcpiTables.inf 
b/Platform/ARM/SgiPkg/AcpiTables/RdFremontAcpiTables.inf
index 9d07001dec96..7556c1239116 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/RdFremontAcpiTables.inf
+++ b/Platform/ARM/SgiPkg/AcpiTables/RdFremontAcpiTables.inf
@@ -48,6 +48,7 @@ [FixedPcd]
   gArmSgiTokenSpaceGuid.PcdGpioController0Interrupt
   gArmSgiTokenSpaceGuid.PcdGtFrame0Gsiv
   gArmSgiTokenSpaceGuid.PcdGtFrame1Gsiv
+  gArmSgiTokenSpaceGuid.PcdOscLpiEnable
   gArmSgiTokenSpaceGuid.PcdSp804DualTimerBaseAddress
   gArmSgiTokenSpaceGuid.PcdSp804DualTimerSize
   gArmSgiTokenSpaceGuid.PcdSp804DualTimerInterrupt
diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdFremont/Dsdt.asl 
b/Platform/ARM/SgiPkg/AcpiTables/RdFremont/Dsdt.asl
index 8812ea877f7a..f921eeb2d99e 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/RdFremont/Dsdt.asl
+++ b/Platform/ARM/SgiPkg/AcpiTables/RdFremont/Dsdt.asl
@@ -8,6 +8,9 @@
 * @par Specification Reference:
 *   - ACPI 6.5, Chapter 5, Section 5.2.11.1, Differentiated System Description
 * Table (DSDT)
+*   - ACPI 6.5, Chapter 8, Section 8.4.3, Lower Power Idle States
+*   - Arm Functional Fixed Hardware Specification v1.2, Chapter 3, Section 3.1,
+* Idle management and Low Power Idle states
 *
 **/
 
@@ -17,6 +20,93 @@
 DefinitionBlock ("DsdtTable.aml", "DSDT", 2, "ARMLTD", "ARMSGI",
  EFI_ACPI_ARM_OEM_REVISION) {
   Scope (_SB) {
+/* _OSC: Operating System Capabilities */
+Method (_OSC, 4, Serialized) {
+  CreateDWordField (Arg3, 0x00, STS0)
+  CreateDWordField (Arg3, 0x04, CAP0)
+
+  /* Platform-wide Capabilities */
+  If (LEqual (Arg0, ToUUID("0811b06e-4a27-44f9-8d60-3cbbc22e7b48"))) {
+/* OSC rev 1 supported, for other version, return failure */
+If (LEqual (Arg1, One)) {
+  And (STS0, Not (OSC_STS_MASK), STS0)
+
+  If (And (CAP0, OSC_CAP_OS_INITIATED_LPI)) {
+/* OS initiated LPI not supported */
+And (CAP0, Not (OSC_CAP_OS_INITIATED_LPI), CAP0)
+Or (STS0, OSC_STS_CAPABILITY_MASKED, STS0)
+  }
+
+  If (And (CAP0, OSC_CAP_PLAT_COORDINATED_LPI)) {
+if (LEqual (FixedPcdGet32 (PcdOscLpiEnable), Zero)) {
+  And (CAP0, Not (OSC_CAP_PLAT_COORDINATED_LPI), CAP0)
+  Or (STS0, OSC_STS_CAPABILITY_MASKED, STS0)
+}
+  }
+} Else {
+  And (STS0, Not (OSC_STS_MASK), STS0)
+  Or (STS0, Or (OSC_STS_FAILURE, OSC_STS_UNRECOGNIZED_REV), STS0)
+}
+  } Else {
+And (STS0, Not (OSC_STS_MASK), STS0)
+Or (STS0, Or (OSC_STS_FAILURE, OSC_STS_UNRECOGNIZED_UUID), STS0)
+  }
+
+  Return (Arg3)
+}
+
+Name (PLPI, Package () {  /* LPI for Processor, support 2 LPI states */
+  0,  // Version
+  0,  // Level Index
+  2,  // Count
+  Package () {// WFI for CPU
+1,// Min residency (uS)
+1,// Wake latency (uS)
+1,// Flags
+0,// Arch Context lost Flags (no loss)
+0,// Residency Counter Frequency
+0,// No parent state
+ResourceTemplate () { // Register Entry method
+  Register (FFixedHW,
+32,   // Bit Width
+0,// Bit Offset
+0x,   // Address
+3,// Access Size
+  )
+},
+ResourceTemplate () { // Null Residency Counter
+  Register (SystemMemory, 0, 0, 0, 0)
+},
+ResourceTemplate () { // Null Usage Counter
+  Register (SystemMemory, 0, 0, 0, 0)
+},
+"LPI1-Core"
+  },
+  Package () {// Power Gating state for CPU
+150,  // Min residency (uS)
+350,  // Wake latency (uS)
+1,// Flags
+1,// Arch Context lost Flags (Core context lost)
+0,// Residency Counter Frequency
+0,// No parent state
+ResourceTemplate () { // Register Entry method
+  Register (FFixedHW,
+32,   // Bit Width
+0,// Bit Offset
+0x4002,   // Address (PwrLvl:core, StateTyp:PwrDn)
+3,// Access Size
+  )
+},
+

[edk2-devel] [edk2-platforms][PATCH v4 4/8] Platform/Sgi: Add ACPI tables for RD-Fremont platform

[Prabin CA]

From: Shriram K 

RD-Fremont is the next platform in the Arm's reference design platform
series. This platform includes 32 CPUs but the fixed virtual platform
(FVP) simulates 16 CPUs of the platform. There is one CPU per cluster in
the system and so the FVP simulates 16 clusters. In preparation for
adding support for this platform, add the initial set of ACPI tables and
reuse existing ACPI tables as applicable to boot a operating system on
this platform.

Signed-off-by: Prabin CA 
---
 Platform/ARM/SgiPkg/AcpiTables/RdFremontAcpiTables.inf |  73 
 Platform/ARM/SgiPkg/AcpiTables/RdFremont/Dsdt.asl  | 196 

 Platform/ARM/SgiPkg/AcpiTables/RdFremont/Madt.aslc | 138 ++
 Platform/ARM/SgiPkg/AcpiTables/RdFremont/Pptt.aslc | 167 +
 4 files changed, 574 insertions(+)

diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdFremontAcpiTables.inf 
b/Platform/ARM/SgiPkg/AcpiTables/RdFremontAcpiTables.inf
new file mode 100644
index ..9d07001dec96
--- /dev/null
+++ b/Platform/ARM/SgiPkg/AcpiTables/RdFremontAcpiTables.inf
@@ -0,0 +1,73 @@
+## @file
+#  ACPI table data and ASL sources required to boot the platform.
+#
+#  Copyright (c) 2024, Arm Ltd. All rights reserved.
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION= 0x0001001A
+  BASE_NAME  = RdFremontAcpiTables
+  FILE_GUID  = c712719a-0aaf-438c-9cdd-35ab4d60207d  # 
gArmSgiAcpiTablesGuid
+  MODULE_TYPE= USER_DEFINED
+  VERSION_STRING = 1.0
+
+[Sources]
+  Dbg2.aslc
+  Fadt.aslc
+  Gtdt.aslc
+  RdFremont/Dsdt.asl
+  RdFremont/Madt.aslc
+  RdFremont/Pptt.aslc
+  Spcr.aslc
+  SsdtEvents.asl
+  SsdtRos.asl
+  SsdtRosVirtioP9.asl
+
+[Packages]
+  ArmPkg/ArmPkg.dec
+  ArmPlatformPkg/ArmPlatformPkg.dec
+  EmbeddedPkg/EmbeddedPkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  MdePkg/MdePkg.dec
+  Platform/ARM/SgiPkg/SgiPlatform.dec
+
+[FixedPcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase
+
+  gArmPlatformTokenSpaceGuid.PcdSerialDbgInterrupt
+  gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase
+  gArmPlatformTokenSpaceGuid.PL011UartInterrupt
+  gArmPlatformTokenSpaceGuid.PcdCoreCount
+  gArmPlatformTokenSpaceGuid.PcdClusterCount
+
+  gArmSgiTokenSpaceGuid.PcdGpioController0BaseAddress
+  gArmSgiTokenSpaceGuid.PcdGpioController0Size
+  gArmSgiTokenSpaceGuid.PcdGpioController0Interrupt
+  gArmSgiTokenSpaceGuid.PcdGtFrame0Gsiv
+  gArmSgiTokenSpaceGuid.PcdGtFrame1Gsiv
+  gArmSgiTokenSpaceGuid.PcdSp804DualTimerBaseAddress
+  gArmSgiTokenSpaceGuid.PcdSp804DualTimerSize
+  gArmSgiTokenSpaceGuid.PcdSp804DualTimerInterrupt
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkSize
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkInterrupt
+  gArmSgiTokenSpaceGuid.PcdVirtioNetBaseAddress
+  gArmSgiTokenSpaceGuid.PcdVirtioNetSize
+  gArmSgiTokenSpaceGuid.PcdVirtioNetInterrupt
+  gArmSgiTokenSpaceGuid.PcdVirtioP9BaseAddress
+  gArmSgiTokenSpaceGuid.PcdVirtioP9Size
+  gArmSgiTokenSpaceGuid.PcdVirtioP9Interrupt
+  gArmSgiTokenSpaceGuid.PcdWdogWS0Gsiv
+  gArmSgiTokenSpaceGuid.PcdWdogWS1Gsiv
+
+  gArmTokenSpaceGuid.PcdArmArchTimerSecIntrNum
+  gArmTokenSpaceGuid.PcdArmArchTimerIntrNum
+  gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum
+  gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum
+  gArmTokenSpaceGuid.PcdGicDistributorBase
+  gArmTokenSpaceGuid.PcdGicRedistributorsBase
+  gArmTokenSpaceGuid.PcdGenericWatchdogControlBase
+  gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase
diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdFremont/Dsdt.asl 
b/Platform/ARM/SgiPkg/AcpiTables/RdFremont/Dsdt.asl
new file mode 100644
index ..8812ea877f7a
--- /dev/null
+++ b/Platform/ARM/SgiPkg/AcpiTables/RdFremont/Dsdt.asl
@@ -0,0 +1,196 @@
+/** @file
+*  Differentiated System Description Table Fields (DSDT)
+*
+*  Copyright (c) 2024, Arm Limited. All rights reserved.
+*
+*  SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+* @par Specification Reference:
+*   - ACPI 6.5, Chapter 5, Section 5.2.11.1, Differentiated System Description
+* Table (DSDT)
+*
+**/
+
+#include "SgiAcpiHeader.h"
+#include "SgiPlatform.h"
+
+DefinitionBlock ("DsdtTable.aml", "DSDT", 2, "ARMLTD", "ARMSGI",
+ EFI_ACPI_ARM_OEM_REVISION) {
+  Scope (_SB) {
+Device (CL00) {   // Cluster 0
+  Name (_HID, "ACPI0010")
+  Name (_UID, 0)
+
+  Device (CP00) { // Neoverse Poseidon core 0
+Name (_HID, "ACPI0007")
+Name (_UID, 0)
+Name (_STA, 0xF)
+  }
+}
+
+Device (CL01) {   // Cluster 1
+  Name (_HID, "ACPI0010")
+  Name (_UID, 1)
+
+  Device (CP01) { // Neoverse Poseidon core 1
+Name (_HID, "ACPI0007")
+Name (_UID, 1)
+Name (_STA, 0xF)
+  }
+}
+
+Device (CL02) {   // Cluster 2
+  Name (_HID, "ACPI0010")
+  Name (_UID, 2)
+
+  Device (CP02) { // Neoverse Poseidon core 2
+Name 

[edk2-devel] [edk2-platforms][PATCH v4 6/8] Platform/Sgi: Extend SMBIOS support for RD-Fremont

[Prabin CA]

Extend the SMBIOS support for RD-Fremont platform. RD-Fremont is a
16 core platform with Poseidon CPU. Each of the CPUs include
64KB L1 Data cache, 64KB L1 Instruction cache and 2MB L2 cache.
The platform also includes system level cache of 32MB and 8GB of RAM.

Signed-off-by: Prabin CA 
---
 Platform/ARM/SgiPkg/Include/SgiPlatform.h | 5 
+
 Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type1SystemInformation.c| 5 
-
 Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type4ProcessorInformation.c | 5 
-
 Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type7CacheInformation.c | 1 +
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c | 6 
++
 5 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/Platform/ARM/SgiPkg/Include/SgiPlatform.h 
b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
index 6fa39d407bc9..acfa45910aed 100644
--- a/Platform/ARM/SgiPkg/Include/SgiPlatform.h
+++ b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
@@ -51,6 +51,10 @@
 #define RD_V2_PART_NUM0x7F2
 #define RD_V2_CONF_ID 0x1
 
+// RD-Fremont Platform Identification values
+#define RD_Fremont_PART_NUM   0x7EE
+#define RD_Fremont_CONF_ID0x1
+
 #define SGI_CONFIG_MASK   0x0F
 #define SGI_CONFIG_SHIFT  0x1C
 #define SGI_PART_NUM_MASK 0xFFF
@@ -90,6 +94,7 @@ typedef enum {
   RdN2Cfg1,
   RdN2Cfg2,
   RdV2,
+  RdFremont,
 } ARM_RD_PRODUCT_ID;
 
 // Arm ProductId look-up table
diff --git 
a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type1SystemInformation.c 
b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type1SystemInformation.c
index edf2a5f63c63..9c28b051ebc2 100644
--- a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type1SystemInformation.c
+++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type1SystemInformation.c
@@ -34,7 +34,8 @@
   "RdN2\0"  \
   "RdN2Cfg1\0"  \
   "RdN2Cfg2\0"  \
-  "RdV2\0"
+  "RdV2\0"  \
+  "RdFremont\0"
 
 typedef enum {
   ManufacturerName = 1,
@@ -74,6 +75,8 @@ STATIC GUID mSmbiosUid[] = {
   {0xd2946d07, 0x8057, 0x4c26, {0xbf, 0x53, 0x78, 0xa6, 0x5b, 0xe1, 0xc1, 
0x60}},
   /* Rd-V2 */
   {0x3b1180a3, 0x0744, 0x4194, {0xae, 0x2e, 0xed, 0xa5, 0xbc, 0x2e, 0x43, 
0x45}},
+  /* Rd-Fremont*/
+  {0x904b28d6, 0x0662, 0x11ed, {0xb9, 0x39, 0x02, 0x42, 0xac, 0x12, 0x00, 
0x02}},
 };
 
 /* System information */
diff --git 
a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type4ProcessorInformation.c 
b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type4ProcessorInformation.c
index ee269f707714..c39c1553f6aa 100644
--- a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type4ProcessorInformation.c
+++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type4ProcessorInformation.c
@@ -44,6 +44,7 @@
   "Neoverse-N2\0"   \
   "Neoverse-N2\0"   \
   "Neoverse-V2\0"   \
+  "Neoverse-Poseidon\0" \
   "000-0\0" /* Serial number */ \
   "783-3\0" \
   "786-1\0" \
@@ -54,7 +55,8 @@
   "7B7-1\0" \
   "7B6-1\0" \
   "7B7-1\0" \
-  "7F2-1\0"
+  "7F2-1\0" \
+  "7EE-1\0"
 
 typedef enum {
   PartNumber = 1,
@@ -181,6 +183,7 @@ InstallType4ProcessorInformation (
   case RdN2:
   case RdN2Cfg1:
   case RdV2:
+  case RdFremont:
 mArmRdSmbiosType4.Base.CoreCount = CoreCount;
 mArmRdSmbiosType4.Base.EnabledCoreCount = CoreCount;
 mArmRdSmbiosType4.Base.ThreadCount = CoreCount;
diff --git 
a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type7CacheInformation.c 
b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type7CacheInformation.c
index 4af72919a3f1..4cdea5b3b763 100644
--- a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type7CacheInformation.c
+++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type7CacheInformation.c
@@ -335,6 +335,7 @@ InstallType7CacheInformation (
 mArmRdSmbiosType7[4].Base.Associativity = CacheAssociativity16Way;
 break;
   case RdV2:
+  case RdFremont:
 /* L1 instruction cache */
 mArmRdSmbiosType7[0].Base.MaximumCacheSize2 = 64;// 64KB
 mArmRdSmbiosType7[0].Base.InstalledSize2 = 64;   // 64KB
diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c 
b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
index 14b06796ae9c..ae31be142d12 100644
--- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
+++ 

[edk2-devel] [edk2-platforms][PATCH v4 5/8] Platform/Sgi: Add initial support for RD-Fremont platform

[Prabin CA]

The RD-Fremont fixed virtual platform simulates 16 CPUs and 8GB of RAM.
Add initial support for this platform by adding the required platform
build configuration files. This platform has considerable differences in
its memory map compared to its predecessors. So add a corresponding
memory map file as well to define the PCDs for its generation of
platforms.

Signed-off-by: Prabin CA 
---
 Platform/ARM/SgiPkg/SgiMemoryMap3.dsc.inc   | 71 
 Platform/ARM/SgiPkg/RdFremont/RdFremont.dsc | 55 +++
 Platform/ARM/SgiPkg/RdFremont/RdFremont.fdf.inc | 10 +++
 3 files changed, 136 insertions(+)

diff --git a/Platform/ARM/SgiPkg/SgiMemoryMap3.dsc.inc 
b/Platform/ARM/SgiPkg/SgiMemoryMap3.dsc.inc
new file mode 100644
index ..06c3b37388c1
--- /dev/null
+++ b/Platform/ARM/SgiPkg/SgiMemoryMap3.dsc.inc
@@ -0,0 +1,71 @@
+#
+#  Copyright (c) 2024, Arm Limited. All rights reserved.
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+
+[PcdsFixedAtBuild.common]
+  # System Peripherals
+  gArmSgiTokenSpaceGuid.PcdSmcCs0Base|0x0800
+  gArmSgiTokenSpaceGuid.PcdSmcCs1Base|0x06
+  gArmSgiTokenSpaceGuid.PcdSysPeriphBase|0x0C00
+  gArmSgiTokenSpaceGuid.PcdSysPeriphSysRegBase|0x0C01
+
+  # SP804 dual timer
+  gArmSgiTokenSpaceGuid.PcdSp804DualTimerBaseAddress|0x0C11
+  gArmSgiTokenSpaceGuid.PcdSp804DualTimerSize|0x0001
+  gArmSgiTokenSpaceGuid.PcdSp804DualTimerInterrupt|216
+
+  # Virtio Disk
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress|0x0C13
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkSize|0x1
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkInterrupt|184
+
+  # GPIO controller
+  gArmSgiTokenSpaceGuid.PcdGpioController0BaseAddress|0x0C1D
+  gArmSgiTokenSpaceGuid.PcdGpioController0Size|0x0001
+  gArmSgiTokenSpaceGuid.PcdGpioController0Interrupt|168
+
+   # Ethernet
+  gArmSgiTokenSpaceGuid.PcdVirtioNetBaseAddress|0x0C15
+  gArmSgiTokenSpaceGuid.PcdVirtioNetInterrupt|186
+
+  # PL031 RealTimeClock
+  gArmPlatformTokenSpaceGuid.PcdPL031RtcBase|0x0C17
+
+  # Virtio P9
+  gArmSgiTokenSpaceGuid.PcdVirtioP9BaseAddress|0x0C19
+  gArmSgiTokenSpaceGuid.PcdVirtioP9Size|0x1
+  gArmSgiTokenSpaceGuid.PcdVirtioP9Interrupt|185
+
+  # PL370 - HDLCD1
+  gArmPlatformTokenSpaceGuid.PcdArmHdLcdBase|0x0EF6
+
+  # PL011 - Serial Debug UART
+  gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase|0x0EF7
+  gArmPlatformTokenSpaceGuid.PcdSerialDbgInterrupt|179
+
+  # PL011 - Serial Terminal
+  gArmPlatformTokenSpaceGuid.PL011UartInterrupt|112
+
+  # System Memory (2GB - 128MB of Trusted DRAM at the top of the 32bit address 
space)
+  gArmTokenSpaceGuid.PcdSystemMemoryBase|0x8000
+  gArmTokenSpaceGuid.PcdSystemMemorySize|0x7800
+
+  # SMMU
+  gArmSgiTokenSpaceGuid.PcdSmmuBase|0x28000
+  gArmSgiTokenSpaceGuid.PcdSmmuSize|0x400
+
+  # Non-Volatile variable storage
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0x06
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64|0x060140
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64|0x060280
+
+  # Address bus width - 64TB address space
+  gArmSgiTokenSpaceGuid.PcdMaxAddressBitsPerChip|46
+
+  # Timer & Watchdog interrupts
+  gArmSgiTokenSpaceGuid.PcdGtFrame0Gsiv|109
+  gArmSgiTokenSpaceGuid.PcdGtFrame1Gsiv|108
+  gArmSgiTokenSpaceGuid.PcdWdogWS0Gsiv|110
+  gArmSgiTokenSpaceGuid.PcdWdogWS1Gsiv|111
diff --git a/Platform/ARM/SgiPkg/RdFremont/RdFremont.dsc 
b/Platform/ARM/SgiPkg/RdFremont/RdFremont.dsc
new file mode 100644
index ..b52d2f59e15d
--- /dev/null
+++ b/Platform/ARM/SgiPkg/RdFremont/RdFremont.dsc
@@ -0,0 +1,55 @@
+#
+#  Copyright (c) 2024, Arm Limited. All rights reserved.
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+
+
+#
+# Defines Section - statements that will be processed to create a Makefile.
+#
+
+[Defines]
+  PLATFORM_NAME  = RdFremont
+  PLATFORM_GUID  = fd140b0f-4467-4314-aa69-cd0bd712e08e
+  PLATFORM_VERSION   = 0.1
+  DSC_SPECIFICATION  = 0x0001001B
+  OUTPUT_DIRECTORY   = Build/$(PLATFORM_NAME)
+  SUPPORTED_ARCHITECTURES= AARCH64
+  BUILD_TARGETS  = NOOPT|DEBUG|RELEASE
+  SKUID_IDENTIFIER   = DEFAULT
+  FLASH_DEFINITION   = Platform/ARM/SgiPkg/SgiPlatform.fdf
+  BOARD_DXE_FV_COMPONENTS= 
Platform/ARM/SgiPkg/RdFremont/RdFremont.fdf.inc
+  BUILD_NUMBER   = 1
+
+# include common definitions from SgiPlatform.dsc
+!include Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
+!include Platform/ARM/SgiPkg/SgiMemoryMap3.dsc.inc
+
+# include common/basic libraries from MdePkg.
+!include MdePkg/MdeLibs.dsc.inc
+
+
+#
+# Pcd Section - list of 

[edk2-devel] [edk2-platforms][PATCH v4 3/8] Platform/Sgi: Introduce a flag to enable PCIe support for RD Platforms

[Prabin CA]

Introducing a flag called PCIE_ENABLE, which can be set to TRUE or
FALSE from the respective .dsc files to enable or disable the
PCIe support. As not all reference design platforms have PCIe support
enabled, this flag is introduced.

Signed-off-by: Prabin CA 
---
 Platform/ARM/SgiPkg/SgiPlatform.dec  |  1 +
 Platform/ARM/SgiPkg/SgiPlatform.dsc.inc  |  6 ++
 Platform/ARM/SgiPkg/RdE1Edge/RdE1Edge.dsc|  4 +++-
 Platform/ARM/SgiPkg/RdN1Edge/RdN1Edge.dsc|  4 +++-
 Platform/ARM/SgiPkg/RdN1EdgeX2/RdN1EdgeX2.dsc|  4 +++-
 Platform/ARM/SgiPkg/RdV1/RdV1.dsc|  4 +++-
 Platform/ARM/SgiPkg/RdV1Mc/RdV1Mc.dsc|  4 +++-
 Platform/ARM/SgiPkg/Sgi575/Sgi575.dsc|  4 +++-
 Platform/ARM/SgiPkg/SgiPlatform.fdf  |  4 +++-
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf  |  5 -
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c | 19 
+++
 11 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec 
b/Platform/ARM/SgiPkg/SgiPlatform.dec
index 4087ff6cad2e..af7887e54126 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.dec
+++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
@@ -31,6 +31,7 @@ [Guids.common]
 [PcdsFeatureFlag.common]
   gArmSgiTokenSpaceGuid.PcdVirtioBlkSupported|FALSE|BOOLEAN|0x0001
   gArmSgiTokenSpaceGuid.PcdVirtioNetSupported|FALSE|BOOLEAN|0x0010
+  gArmSgiTokenSpaceGuid.PcdPcieEnable|FALSE|BOOLEAN|0x002E
 
 [PcdsFixedAtBuild]
   gArmSgiTokenSpaceGuid.PcdDramBlock2Base|0|UINT64|0x0002
diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc 
b/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
index 1cfe07c7e4ed..1bf489ffeb39 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
+++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
@@ -103,6 +103,10 @@ [PcdsFeatureFlag.common]
   gArmSgiTokenSpaceGuid.PcdVirtioNetSupported|TRUE
   gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE
 
+!if $(PCIE_ENABLE) == TRUE
+  gArmSgiTokenSpaceGuid.PcdPcieEnable|TRUE
+!endif
+
 [PcdsFixedAtBuild.common]
   gArmTokenSpaceGuid.PcdVFPEnabled|1
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
@@ -330,6 +334,7 @@ [Components.common]
   # Virtio Network
   OvmfPkg/VirtioNetDxe/VirtioNet.inf
 
+!if $(PCIE_ENABLE) == TRUE
   #
   # Required by PCI
   #
@@ -343,6 +348,7 @@ [Components.common]
 
   gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x8010004F
   }
+!endif
 
   #
   # AHCI Support
diff --git a/Platform/ARM/SgiPkg/RdE1Edge/RdE1Edge.dsc 
b/Platform/ARM/SgiPkg/RdE1Edge/RdE1Edge.dsc
index 32d67d380814..c7463da5203e 100644
--- a/Platform/ARM/SgiPkg/RdE1Edge/RdE1Edge.dsc
+++ b/Platform/ARM/SgiPkg/RdE1Edge/RdE1Edge.dsc
@@ -1,5 +1,5 @@
 #
-#  Copyright (c) 2020-2022, ARM Limited. All rights reserved.
+#  Copyright (c) 2020-2024, Arm Limited. All rights reserved.
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -22,6 +22,8 @@ [Defines]
   BOARD_DXE_FV_COMPONENTS= 
Platform/ARM/SgiPkg/RdE1Edge/RdE1Edge.fdf.inc
   BUILD_NUMBER   = 1
 
+  DEFINE PCIE_ENABLE = TRUE
+
 # include common definitions from SgiPlatform.dsc
 !include Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
 !include Platform/ARM/SgiPkg/SgiMemoryMap.dsc.inc
diff --git a/Platform/ARM/SgiPkg/RdN1Edge/RdN1Edge.dsc 
b/Platform/ARM/SgiPkg/RdN1Edge/RdN1Edge.dsc
index 6c9a64df054f..77efec9d9533 100644
--- a/Platform/ARM/SgiPkg/RdN1Edge/RdN1Edge.dsc
+++ b/Platform/ARM/SgiPkg/RdN1Edge/RdN1Edge.dsc
@@ -1,5 +1,5 @@
 #
-#  Copyright (c) 2020-2022, ARM Limited. All rights reserved.
+#  Copyright (c) 2020-2024, Arm Limited. All rights reserved.
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -22,6 +22,8 @@ [Defines]
   BOARD_DXE_FV_COMPONENTS= 
Platform/ARM/SgiPkg/RdN1Edge/RdN1Edge.fdf.inc
   BUILD_NUMBER   = 1
 
+  DEFINE PCIE_ENABLE = TRUE
+
 # include common definitions from SgiPlatform.dsc
 !include Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
 !include Platform/ARM/SgiPkg/SgiMemoryMap.dsc.inc
diff --git a/Platform/ARM/SgiPkg/RdN1EdgeX2/RdN1EdgeX2.dsc 
b/Platform/ARM/SgiPkg/RdN1EdgeX2/RdN1EdgeX2.dsc
index 10e5bfa29b46..521d88925059 100644
--- a/Platform/ARM/SgiPkg/RdN1EdgeX2/RdN1EdgeX2.dsc
+++ b/Platform/ARM/SgiPkg/RdN1EdgeX2/RdN1EdgeX2.dsc
@@ -1,5 +1,5 @@
 #
-#  Copyright (c) 2020-2022, ARM Limited. All rights reserved.
+#  Copyright (c) 2020-2024, Arm Limited. All rights reserved.
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -22,6 +22,8 @@ [Defines]
   BOARD_DXE_FV_COMPONENTS= 
Platform/ARM/SgiPkg/RdN1EdgeX2/RdN1EdgeX2.fdf.inc
   BUILD_NUMBER   = 1
 
+  DEFINE PCIE_ENABLE = TRUE
+
 # include common definitions from SgiPlatform.dsc
 !include Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
 !include Platform/ARM/SgiPkg/SgiMemoryMap.dsc.inc
diff --git a/Platform/ARM/SgiPkg/RdV1/RdV1.dsc 

[edk2-devel] [edk2-platforms][PATCH v4 2/8] Platform/Sgi: Refactor system memory base and size definitions

[Prabin CA]

In preparation of adding the next generation of reference design
platform that have different memory map, refactor the
PcdSystemMemoryBase and PcdSystemMemorySize PCD definitions from the
common PCD definitions file into the various platform generation
specific memory map PCD definitions file.

Signed-off-by: Prabin CA 
---
 Platform/ARM/SgiPkg/SgiMemoryMap.dsc.inc  | 8 +++-
 Platform/ARM/SgiPkg/SgiMemoryMap2.dsc.inc | 8 +++-
 Platform/ARM/SgiPkg/SgiPlatform.dsc.inc   | 6 +-
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/Platform/ARM/SgiPkg/SgiMemoryMap.dsc.inc 
b/Platform/ARM/SgiPkg/SgiMemoryMap.dsc.inc
index 0c577c42..eab43b23ec6d 100644
--- a/Platform/ARM/SgiPkg/SgiMemoryMap.dsc.inc
+++ b/Platform/ARM/SgiPkg/SgiMemoryMap.dsc.inc
@@ -1,5 +1,5 @@
 #
-#  Copyright (c) 2020 - 2022, Arm Limited. All rights reserved.
+#  Copyright (c) 2020 - 2024, Arm Limited. All rights reserved.
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -67,3 +67,9 @@ [PcdsFixedAtBuild.common]
   gArmSgiTokenSpaceGuid.PcdGpioController0BaseAddress|0x1C1D
   gArmSgiTokenSpaceGuid.PcdGpioController0Size|0x0001
   gArmSgiTokenSpaceGuid.PcdGpioController0Interrupt|136
+
+  # System Memory (1GB - 16MB of Trusted DRAM at the top of the
+  # 32bit address space)
+  gArmTokenSpaceGuid.PcdSystemMemoryBase|0x8000
+  gArmTokenSpaceGuid.PcdSystemMemorySize|0x7F00
+
diff --git a/Platform/ARM/SgiPkg/SgiMemoryMap2.dsc.inc 
b/Platform/ARM/SgiPkg/SgiMemoryMap2.dsc.inc
index de1d8ea24b89..35e27d42d5a2 100644
--- a/Platform/ARM/SgiPkg/SgiMemoryMap2.dsc.inc
+++ b/Platform/ARM/SgiPkg/SgiMemoryMap2.dsc.inc
@@ -1,5 +1,5 @@
 #
-#  Copyright (c) 2020 - 2023, Arm Limited. All rights reserved.
+#  Copyright (c) 2020 - 2024, Arm Limited. All rights reserved.
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -75,3 +75,9 @@ [PcdsFixedAtBuild.common]
 
   # IO virtualization block
   gArmSgiTokenSpaceGuid.PcdIoVirtSocExpBlk0Base|0x108000
+
+  # System Memory (1GB - 16MB of Trusted DRAM at the top of the
+  # 32bit address space)
+  gArmTokenSpaceGuid.PcdSystemMemoryBase|0x8000
+  gArmTokenSpaceGuid.PcdSystemMemorySize|0x7F00
+
diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc 
b/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
index 26ecd9ed59a7..1cfe07c7e4ed 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
+++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
@@ -1,5 +1,5 @@
 #
-#  Copyright (c) 2018 - 2022, ARM Limited. All rights reserved.
+#  Copyright (c) 2018 - 2024, Arm Limited. All rights reserved.
 #  (C) Copyright 2021 Hewlett Packard Enterprise Development LP
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -131,10 +131,6 @@ [PcdsFixedAtBuild.common]
   gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x4
   gArmPlatformTokenSpaceGuid.PcdCPUCoreSecondaryStackSize|0x0
 
-  # System Memory (1GB - 16MB of Trusted DRAM at the top of the 32bit address 
space)
-  gArmTokenSpaceGuid.PcdSystemMemoryBase|0x8000
-  gArmTokenSpaceGuid.PcdSystemMemorySize|0x7F00
-
   # ACPI Table Version
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions|0x20
 
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116271): https://edk2.groups.io/g/devel/message/116271
Mute This Topic: https://groups.io/mt/104668515/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms][PATCH v4 1/8] Platform/Sgi: Update the datatype of PcdSmmuBase from u32 to u64

[Prabin CA]

From: Vivek Gautam 

On RD-N2 and previous generation platforms, the base address was within
32-bit region. However, on upcoming platforms, the SMMUv3 base address
is beyond 32-bit address region. So, update the datatype of SMMUv3 base
PCD.

Signed-off-by: Prabin CA 
---
 Platform/ARM/SgiPkg/SgiPlatform.dec  | 2 +-
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec 
b/Platform/ARM/SgiPkg/SgiPlatform.dec
index 103dff8471a7..4087ff6cad2e 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.dec
+++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
@@ -79,7 +79,7 @@ [PcdsFixedAtBuild]
   gArmSgiTokenSpaceGuid.PcdWdogWS1Gsiv|0|UINT32|0x0014
 
   # SMMU
-  gArmSgiTokenSpaceGuid.PcdSmmuBase|0|UINT32|0x001D
+  gArmSgiTokenSpaceGuid.PcdSmmuBase|0|UINT64|0x001D
   gArmSgiTokenSpaceGuid.PcdSmmuSize|0|UINT32|0x001E
 
   # GPIO Controller
diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c 
b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c
index fa3cfbc730f6..62c212f3c5b0 100644
--- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c
+++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c
@@ -1,6 +1,6 @@
 /** @file
 *
-*  Copyright (c) 2018-2023, ARM Limited. All rights reserved.
+*  Copyright (c) 2018-2024, Arm Limited. All rights reserved.
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
@@ -167,8 +167,8 @@ ArmPlatformGetVirtualMemoryMap (
   VirtualMemoryTable[Index].Attributes  = 
ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
 
   // Sub System Peripherals - SMMU
-  VirtualMemoryTable[++Index].PhysicalBase  = FixedPcdGet32 (PcdSmmuBase);
-  VirtualMemoryTable[Index].VirtualBase = FixedPcdGet32 (PcdSmmuBase);
+  VirtualMemoryTable[++Index].PhysicalBase  = FixedPcdGet64 (PcdSmmuBase);
+  VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdSmmuBase);
   VirtualMemoryTable[Index].Length  = FixedPcdGet32 (PcdSmmuSize);
   VirtualMemoryTable[Index].Attributes  = 
ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
 
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116270): https://edk2.groups.io/g/devel/message/116270
Mute This Topic: https://groups.io/mt/104668514/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms][PATCH v4 0/8] Platform/Sgi: Add support for RD-Fremont platform

[Prabin CA]

Changes since V3:
- Rebase on top of latest upstream branch.

Changes since V2:
- Removed author's signed-off on the patches, which is owned by another author.

Changes since V1:
- Corrected memory map in the DSDT file.

This patch series introduce support for RD-Fremont reference design
platform. This platform includes 32 CPUs, but the fixed virtual platform
(FVP) simulates 16 CPUs of the platform. There is one CPU per cluster in
the system and so the FVP simulates 16 clusters. Each of the CPUs
include 64KB L1 Data cache, 64KB L1 Instruction cache and 2MB L2 cache.
The platform also includes system level cache of 32MB and 8GB of RAM.
Also, this patch series adding the extended SMBIO support for RD-Fremont
platform.

In addition to patches that introduce RD-Fremont platform, there are
three patches that update support for existing platforms. The first
patch in this series changes the data type of PcdSmmuBase from u32 to
u64. The second patch refactor the system memory map base and size
values. The third patch add a flag to enable PCIE support for existing
and future platforms.

This patch series should be applied on top of the patch series
https://edk2.groups.io/g/devel/message/116262

Link to gitlab branch with the patches in this series -
https://gitlab.arm.com/infra-solutions/reference-design/platsw/edk2-platforms/-/commits/topics/rdfremont/

Prabin CA (6):
  Platform/Sgi: Refactor system memory base and size definitions
  Platform/Sgi: Introduce a flag to enable PCIe support for RD Platforms
  Platform/Sgi: Add initial support for RD-Fremont platform
  Platform/Sgi: Extend SMBIOS support for RD-Fremont
  Platform/Sgi: Low Power Idle States for RD-Fremont
  Platform/Sgi: Add CPPC support for RD-Fremont platform

Shriram K (1):
  Platform/Sgi: Add ACPI tables for RD-Fremont platform

Vivek Gautam (1):
  Platform/Sgi: Update the datatype of PcdSmmuBase from u32 to u64

 Platform/ARM/SgiPkg/SgiPlatform.dec   |   
3 +-
 Platform/ARM/SgiPkg/SgiMemoryMap.dsc.inc  |   
8 +-
 Platform/ARM/SgiPkg/SgiMemoryMap2.dsc.inc |   
8 +-
 Platform/ARM/SgiPkg/{SgiMemoryMap2.dsc.inc => SgiMemoryMap3.dsc.inc}  |  
88 ++--
 Platform/ARM/SgiPkg/SgiPlatform.dsc.inc   |  
12 +-
 Platform/ARM/SgiPkg/RdE1Edge/RdE1Edge.dsc |   
4 +-
 Platform/ARM/SgiPkg/{RdV1/RdV1.dsc => RdFremont/RdFremont.dsc}|  
14 +-
 Platform/ARM/SgiPkg/RdN1Edge/RdN1Edge.dsc |   
4 +-
 Platform/ARM/SgiPkg/RdN1EdgeX2/RdN1EdgeX2.dsc |   
4 +-
 Platform/ARM/SgiPkg/RdV1/RdV1.dsc |   
4 +-
 Platform/ARM/SgiPkg/RdV1Mc/RdV1Mc.dsc |   
4 +-
 Platform/ARM/SgiPkg/Sgi575/Sgi575.dsc |   
4 +-
 Platform/ARM/SgiPkg/SgiPlatform.fdf   |   
4 +-
 Platform/ARM/SgiPkg/AcpiTables/RdFremontAcpiTables.inf|  
75 +++
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf   |   
5 +-
 Platform/ARM/SgiPkg/Include/SgiPlatform.h |   
5 +
 Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type1SystemInformation.c|   
5 +-
 Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type4ProcessorInformation.c |   
5 +-
 Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type7CacheInformation.c |   
1 +
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c |   
6 +
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c  |  
25 +-
 Platform/ARM/SgiPkg/AcpiTables/RdFremont/Dsdt.asl | 
512 
 Platform/ARM/SgiPkg/AcpiTables/RdFremont/Madt.aslc| 
138 ++
 Platform/ARM/SgiPkg/AcpiTables/RdFremont/Pptt.aslc| 
167 +++
 Platform/ARM/SgiPkg/RdFremont/RdFremont.fdf.inc   |  
10 +
 25 files changed, 1032 insertions(+), 83 deletions(-)
 copy Platform/ARM/SgiPkg/{SgiMemoryMap2.dsc.inc => SgiMemoryMap3.dsc.inc} (62%)
 copy Platform/ARM/SgiPkg/{RdV1/RdV1.dsc => RdFremont/RdFremont.dsc} (77%)
 create mode 100644 Platform/ARM/SgiPkg/AcpiTables/RdFremontAcpiTables.inf
 create mode 100644 Platform/ARM/SgiPkg/AcpiTables/RdFremont/Dsdt.asl
 create mode 100644 Platform/ARM/SgiPkg/AcpiTables/RdFremont/Madt.aslc
 create mode 100644 Platform/ARM/SgiPkg/AcpiTables/RdFremont/Pptt.aslc
 create mode 100644 Platform/ARM/SgiPkg/RdFremont/RdFremont.fdf.inc

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116269): https://edk2.groups.io/g/devel/message/116269
Mute This Topic: https://groups.io/mt/104668513/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms][PATCH v5 4/6] Platform/Sgi: Add support for RD-N2-Cfg3 platform

[Prabin CA]

The Neoverse RD-N2-Cfg3 platform is a variant of RD-N2 platform with a
different mesh size and GIC ITS count. As part of the initial platform
support, add the corresponding platform and flash description files.
Use PcdPlatformVariant for the RD-N2-Cfg3 platform to specify the
platform variant. RD-N2-Cfg3 has 12 GIC ITS blocks when compared to the
other RD-N2 variants that have 6 GIC ITS blocks.

Signed-off-by: Prabin CA 
---
 Platform/ARM/SgiPkg/RdN2Cfg3/RdN2Cfg3.dsc 
| 58 
 Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf 
|  3 +-
 Platform/ARM/SgiPkg/AcpiTables/{RdN2AcpiTables.inf => RdN2Cfg3AcpiTables.inf} 
| 15 ++---
 Platform/ARM/SgiPkg/AcpiTables/RdN2/Madt.aslc 
| 14 -
 Platform/ARM/SgiPkg/RdN2Cfg3/RdN2Cfg3.fdf.inc 
| 10 
 5 files changed, 88 insertions(+), 12 deletions(-)

diff --git a/Platform/ARM/SgiPkg/RdN2Cfg3/RdN2Cfg3.dsc 
b/Platform/ARM/SgiPkg/RdN2Cfg3/RdN2Cfg3.dsc
new file mode 100644
index ..88293b236a32
--- /dev/null
+++ b/Platform/ARM/SgiPkg/RdN2Cfg3/RdN2Cfg3.dsc
@@ -0,0 +1,58 @@
+#
+#  Copyright (c) 2024, ARM Limited. All rights reserved.
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+
+
+#
+# Defines Section - statements that will be processed to create a Makefile.
+#
+
+[Defines]
+  PLATFORM_NAME  = RdN2Cfg3
+  PLATFORM_GUID  = b890ba7d-a256-4820-9d3a-655acbb737c9
+  PLATFORM_VERSION   = 0.1
+  DSC_SPECIFICATION  = 0x0001001B
+  OUTPUT_DIRECTORY   = Build/$(PLATFORM_NAME)
+  SUPPORTED_ARCHITECTURES= AARCH64
+  BUILD_TARGETS  = NOOPT|DEBUG|RELEASE
+  SKUID_IDENTIFIER   = DEFAULT
+  FLASH_DEFINITION   = Platform/ARM/SgiPkg/SgiPlatform.fdf
+  BOARD_DXE_FV_COMPONENTS= 
Platform/ARM/SgiPkg/RdN2Cfg3/RdN2Cfg3.fdf.inc
+  BUILD_NUMBER   = 1
+
+# include common definitions from SgiPlatform.dsc
+!include Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
+!include Platform/ARM/SgiPkg/SgiMemoryMap2.dsc.inc
+
+# include common/basic libraries from MdePkg.
+!include MdePkg/MdeLibs.dsc.inc
+
+
+#
+# Pcd Section - list of all EDK II PCD Entries defined by this Platform
+#
+
+
+[PcdsFixedAtBuild.common]
+  # GIC configurations
+  gArmTokenSpaceGuid.PcdGicDistributorBase|0x3000
+  gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x3030
+  gArmSgiTokenSpaceGuid.PcdGicSize|0x40
+
+  # ARM Cores and Clusters
+  gArmPlatformTokenSpaceGuid.PcdCoreCount|1
+  gArmPlatformTokenSpaceGuid.PcdClusterCount|16
+
+  # RdN2Cfg3 is the third variant from RdN2 Platform
+  gArmSgiTokenSpaceGuid.PcdPlatformVariant|3
+
+
+#
+# Components Section - list of all EDK II Modules needed by this Platform
+#
+
+
+[Components.common]
+  Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg3AcpiTables.inf
diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf 
b/Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf
index 8025ef58171b..afc38385c051 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf
+++ b/Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf
@@ -1,7 +1,7 @@
 ## @file
 #  ACPI table data and ASL sources required to boot the platform.
 #
-#  Copyright (c) 2020 - 2023, Arm Ltd. All rights reserved.
+#  Copyright (c) 2020 - 2024, Arm Ltd. All rights reserved.
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -67,6 +67,7 @@ [FixedPcd]
   gArmSgiTokenSpaceGuid.PcdMaxAddressBitsPerChip
   gArmSgiTokenSpaceGuid.PcdOscLpiEnable
   gArmSgiTokenSpaceGuid.PcdOscCppcEnable
+  gArmSgiTokenSpaceGuid.PcdPlatformVariant
   gArmSgiTokenSpaceGuid.PcdSp804DualTimerBaseAddress
   gArmSgiTokenSpaceGuid.PcdSp804DualTimerSize
   gArmSgiTokenSpaceGuid.PcdSp804DualTimerInterrupt
diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf 
b/Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg3AcpiTables.inf
similarity index 90%
copy from Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf
copy to Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg3AcpiTables.inf
index 8025ef58171b..a703d5a994f7 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf
+++ b/Platform/ARM/SgiPkg/AcpiTables/RdN2Cfg3AcpiTables.inf
@@ -1,7 +1,7 @@
 ## @file
 #  ACPI table data and ASL sources required to boot the platform.
 #
-#  Copyright (c) 2020 - 2023, Arm Ltd. All rights reserved.
+#  Copyright (c) 2024, Arm Ltd. All rights reserved.
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -9,7 

[edk2-devel] [edk2-platforms][PATCH v5 6/6] Platform/Sgi: Extend SMBIOS support for RD-V2 platform

[Prabin CA]

From: Pranav Madhu 

The Neoverse RD-V2 FVP platform includes 16 CPUs and each CPU has 64KB
of L1 instruction/data cache, 2MB of L2 cache and 32MB of system level
cache. Extend the SMBIOS support for RD-V2 platform with this
configuration and reuse rest of the RD-N2 SMBIOS configuration for the
RD-V2 platform.

Signed-off-by: Prabin CA 
---
 Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type1SystemInformation.c|  7 
+--
 Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type4ProcessorInformation.c |  9 
++---
 Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type7CacheInformation.c | 20 
+++-
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git 
a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type1SystemInformation.c 
b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type1SystemInformation.c
index b7e2238fb39c..edf2a5f63c63 100644
--- a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type1SystemInformation.c
+++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type1SystemInformation.c
@@ -5,7 +5,7 @@
   Reference Design platforms. Type 1 table defines attributes of the
   overall system such as manufacturer, product name, UUID etc.
 
-  Copyright (c) 2021 - 2022, Arm Limited. All rights reserved.
+  Copyright (c) 2021 - 2024, Arm Limited. All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
   @par Specification Reference:
@@ -33,7 +33,8 @@
   "RdV1Mc\0"\
   "RdN2\0"  \
   "RdN2Cfg1\0"  \
-  "RdN2Cfg2\0"
+  "RdN2Cfg2\0"  \
+  "RdV2\0"
 
 typedef enum {
   ManufacturerName = 1,
@@ -71,6 +72,8 @@ STATIC GUID mSmbiosUid[] = {
   {0xa4941d3d, 0xfac3, 0x4ace, {0x9a, 0x7e, 0xce, 0x26, 0x76, 0x64, 0x5e, 
0xda}},
   /* Rd-N2-Cfg2*/
   {0xd2946d07, 0x8057, 0x4c26, {0xbf, 0x53, 0x78, 0xa6, 0x5b, 0xe1, 0xc1, 
0x60}},
+  /* Rd-V2 */
+  {0x3b1180a3, 0x0744, 0x4194, {0xae, 0x2e, 0xed, 0xa5, 0xbc, 0x2e, 0x43, 
0x45}},
 };
 
 /* System information */
diff --git 
a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type4ProcessorInformation.c 
b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type4ProcessorInformation.c
index b59172cf1cb9..ee269f707714 100644
--- a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type4ProcessorInformation.c
+++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type4ProcessorInformation.c
@@ -6,7 +6,7 @@
   family, processor id, maximum operating frequency, and other information
   related to the processor.
 
-  Copyright (c) 2021 - 2022, Arm Limited. All rights reserved.
+  Copyright (c) 2021 - 2024, Arm Limited. All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
   @par Specification Reference:
@@ -27,7 +27,7 @@
 #define SOCKET_TYPE_BASE3
 #define SOCKET_TYPE_NUM 1
 #define PROCESSOR_VERSION_BASE  (SOCKET_TYPE_BASE + SOCKET_TYPE_NUM)
-#define PROCESSOR_VERSION_NUM   10
+#define PROCESSOR_VERSION_NUM   11
 #define SERIAL_NUMBER_BASE  (PROCESSOR_VERSION_BASE + 
PROCESSOR_VERSION_NUM)
 #define TYPE4_STRINGS   \
   "0x000\0" /* Part Number */   \
@@ -43,6 +43,7 @@
   "Neoverse-N2\0"   \
   "Neoverse-N2\0"   \
   "Neoverse-N2\0"   \
+  "Neoverse-V2\0"   \
   "000-0\0" /* Serial number */ \
   "783-3\0" \
   "786-1\0" \
@@ -52,7 +53,8 @@
   "78A-2\0" \
   "7B7-1\0" \
   "7B6-1\0" \
-  "7B7-1\0"
+  "7B7-1\0" \
+  "7F2-1\0"
 
 typedef enum {
   PartNumber = 1,
@@ -178,6 +180,7 @@ InstallType4ProcessorInformation (
 break;
   case RdN2:
   case RdN2Cfg1:
+  case RdV2:
 mArmRdSmbiosType4.Base.CoreCount = CoreCount;
 mArmRdSmbiosType4.Base.EnabledCoreCount = CoreCount;
 mArmRdSmbiosType4.Base.ThreadCount = CoreCount;
diff --git 
a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type7CacheInformation.c 
b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type7CacheInformation.c
index b71ce721e2e8..4af72919a3f1 100644
--- a/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type7CacheInformation.c
+++ b/Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type7CacheInformation.c
@@ -6,7 +6,7 @@
   implemented, cache configuration, ways of associativity and other
   information related to cache memory installed.
 
-  Copyright (c) 2021 - 2022, Arm Limited. All rights reserved.
+  Copyright (c) 2021 - 2024, Arm Limited. All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
   @par Specification Reference:
@@ -334,6 +334,24 @@ InstallType7CacheInformation (
  

[edk2-devel] [edk2-platforms][PATCH v5 5/6] Platform/Sgi: Define RD-V2 platform id values

[Prabin CA]

From: Pranav Madhu 

Add RD-V2 platform identification values including the part number
and configuration number. This information will be used in populating
the SMBIOS tables.

Signed-off-by: Prabin CA 
---
 Platform/ARM/SgiPkg/Include/SgiPlatform.h | 7 ++-
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c | 8 +++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/Platform/ARM/SgiPkg/Include/SgiPlatform.h 
b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
index e83853664c4c..6fa39d407bc9 100644
--- a/Platform/ARM/SgiPkg/Include/SgiPlatform.h
+++ b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
@@ -1,6 +1,6 @@
 /** @file
 *
-*  Copyright (c) 2018 - 2022, Arm Limited. All rights reserved.
+*  Copyright (c) 2018 - 2024, Arm Limited. All rights reserved.
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
@@ -47,6 +47,10 @@
 #define RD_N2_PART_NUM0x7B7
 #define RD_N2_CONF_ID 0x1
 
+// RD-V2 Platform Identification values
+#define RD_V2_PART_NUM0x7F2
+#define RD_V2_CONF_ID 0x1
+
 #define SGI_CONFIG_MASK   0x0F
 #define SGI_CONFIG_SHIFT  0x1C
 #define SGI_PART_NUM_MASK 0xFFF
@@ -85,6 +89,7 @@ typedef enum {
   RdN2,
   RdN2Cfg1,
   RdN2Cfg2,
+  RdV2,
 } ARM_RD_PRODUCT_ID;
 
 // Arm ProductId look-up table
diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c 
b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
index fa006320025b..14b06796ae9c 100644
--- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
+++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
@@ -1,6 +1,6 @@
 /** @file
 *
-*  Copyright (c) 2018 - 2022, Arm Limited. All rights reserved.
+*  Copyright (c) 2018 - 2024, Arm Limited. All rights reserved.
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
@@ -79,6 +79,12 @@ STATIC CONST SGI_PRODUCT_ID_LOOKUP SgiProductIdLookup[] = {
 RD_N2_CONF_ID,
 1
   },
+  {
+RdV2,
+RD_V2_PART_NUM,
+RD_V2_CONF_ID,
+0
+  },
 };
 
 EFI_BOOT_MODE
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116267): https://edk2.groups.io/g/devel/message/116267
Mute This Topic: https://groups.io/mt/104668455/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms][PATCH v5 3/6] Platform/Sgi: Add a PCD to specify platform variant

[Prabin CA]

A new PCD named PcdPlatformVariant is introduced to specify the variant
number of a platform. This PCD can be used to select platform variant
specific configurations. The default value of this PCD is 0 which
selects the base variant.

Signed-off-by: Prabin CA 
---
 Platform/ARM/SgiPkg/SgiPlatform.dec | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec 
b/Platform/ARM/SgiPkg/SgiPlatform.dec
index 1613cc01981e..103dff8471a7 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.dec
+++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
@@ -1,5 +1,5 @@
 #
-#  Copyright (c) 2018 - 2023, Arm Limited. All rights reserved.
+#  Copyright (c) 2018 - 2024, Arm Limited. All rights reserved.
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -51,8 +51,9 @@ [PcdsFixedAtBuild]
   gArmSgiTokenSpaceGuid.PcdVirtioP9Size|0x|UINT32|0x0029
   gArmSgiTokenSpaceGuid.PcdVirtioP9Interrupt|0x|UINT32|0x002A
 
-  # Chip count on the platform
+  # Chip count on the platform and platform variant
   gArmSgiTokenSpaceGuid.PcdChipCount|1|UINT32|0x000B
+  gArmSgiTokenSpaceGuid.PcdPlatformVariant|0|UINT32|0x002D
 
   # GIC
   gArmSgiTokenSpaceGuid.PcdGicSize|0|UINT64|0x000A
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116265): https://edk2.groups.io/g/devel/message/116265
Mute This Topic: https://groups.io/mt/104668452/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms][PATCH v5 2/6] Platform/Sgi: Add VariableFlashInfoLib to fix missing dependency

[Prabin CA]

From: Vijayenthiran Subramaniam 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3479

A recent change in MdeModulePkg [1] introduced VariableFlashInfoLib as a
dependency to support dynamic variable flash information. Add an
instance for the library class VariableFlashInfoLib in
SgiPlatformMm.dsc.inc to resolve this dependency.

[1]: 
https://github.com/tianocore/edk2/commit/8db39c60cdf35e0a53ccdbccf7e152ab41f54f4c

Signed-off-by: Prabin CA 
---
 Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc 
b/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc
index ab54b3b25f4c..0dd9ebbfc16c 100644
--- a/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc
+++ b/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc
@@ -70,6 +70,7 @@ [LibraryClasses.common.MM_STANDALONE]
   
SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
   TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf
   VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
+  
VariableFlashInfoLib|MdeModulePkg/Library/BaseVariableFlashInfoLib/BaseVariableFlashInfoLib.inf
   SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
 !endif
 
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116264): https://edk2.groups.io/g/devel/message/116264
Mute This Topic: https://groups.io/mt/104668449/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms][PATCH v5 1/6] Platform/Sgi: remove +nofp gcc option flag

[Prabin CA]

From: Omkar Anand Kulkarni 

The software executing at a higher privileged level on the reference
design platforms have been updated to allow software executing at EL1
and EL0 to access the Advanced SIMD and floating-point registers (FPEN
field of CPACR_EL1 system register is programmed to allow access). So,
remove the use of +nofp gcc build option flag.

Signed-off-by: Prabin CA 
---
 Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc 
b/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc
index ae0ff7247a6a..ab54b3b25f4c 100644
--- a/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc
+++ b/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc
@@ -145,5 +145,5 @@ [Components.AARCH64]
 #
 
###
 [BuildOptions.AARCH64]
-  GCC:*_*_*_CC_FLAGS = -mstrict-align -march=armv8-a+nofp -D 
DISABLE_NEW_DEPRECATED_INTERFACES
+  GCC:*_*_*_CC_FLAGS = -mstrict-align -march=armv8-a -D 
DISABLE_NEW_DEPRECATED_INTERFACES
   GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116263): https://edk2.groups.io/g/devel/message/116263
Mute This Topic: https://groups.io/mt/104668448/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms][PATCH v5 0/6] Platform/Sgi: Add support for RD-N2-Cfg3 and RD-V2 platform

[Prabin CA]

Changes since V4:
- Rebase on top of latest upstream branch.

Changes since V3:
- Removed author's signed-off on the patches, which is owned by another author.

Changes since V2:
- Removed the patch which introduce fno-stack-protector in the build flag.

Changes since V1:
- Addressed comments from Sami.

This patch series introduces support for two reference design platforms-
RD-N2-Cfg3 and RD-V2. The RD-N2-Cfg3 FVP platform is a variant of RD-N2
platform with a different mesh size and GIC ITS count. It is based on
the Neoverse N2 CPUs and includes 16xMP1 CPUs. RD-N2-Cfg3 has 12 GIC ITS
blocks, 6 more than the other RD-N2 variants.

The Neoverse RD-V2 FVP platform includes 16xMP1 Neoverse V2 CPUs and
each CPU has 64KB of L1 instruction/data cache, 2MB of L2 cache and 32MB
of system level cache. The system architecture of the RD-V2 platform is
similar to the RD-N2 platform, except for the CPU and L2 cache size. So
existing RD-N2 SMBIOS support is extended for RD-V2 platform to reuse
rest of the RD-N2 SMBIOS configuration for the RD-V2 platform.

In addition to patches that introduce support for these two platforms,
there are two patches that update support for existing platforms. The
first patch in this series removes +nofp gcc option flag. The second
patch adds VariableFlashInfoLib to the common dsc file.

Link to gitlb branch with the patches in this series -
https://gitlab.arm.com/infra-solutions/reference-design/platsw/edk2-platforms/-/commits/topics/rdn2cfg3_rdv2_updates

Omkar Anand Kulkarni (1):
  Platform/Sgi: remove +nofp gcc option flag

Prabin CA (2):
  Platform/Sgi: Add a PCD to specify platform variant
  Platform/Sgi: Add support for RD-N2-Cfg3 platform

Pranav Madhu (2):
  Platform/Sgi: Define RD-V2 platform id values
  Platform/Sgi: Extend SMBIOS support for RD-V2 platform

Vijayenthiran Subramaniam (1):
  Platform/Sgi: Add VariableFlashInfoLib to fix missing dependency

 Platform/ARM/SgiPkg/SgiPlatform.dec   
|  5 +-
 Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc 
|  3 +-
 Platform/ARM/SgiPkg/RdN2Cfg3/RdN2Cfg3.dsc 
| 58 
 Platform/ARM/SgiPkg/AcpiTables/RdN2AcpiTables.inf 
|  3 +-
 Platform/ARM/SgiPkg/AcpiTables/{RdN2AcpiTables.inf => RdN2Cfg3AcpiTables.inf} 
| 15 ++---
 Platform/ARM/SgiPkg/Include/SgiPlatform.h 
|  7 ++-
 Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type1SystemInformation.c
|  7 ++-
 Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type4ProcessorInformation.c 
|  9 ++-
 Platform/ARM/SgiPkg/Drivers/SmbiosPlatformDxe/Type7CacheInformation.c 
| 20 ++-
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c 
|  8 ++-
 Platform/ARM/SgiPkg/AcpiTables/RdN2/Madt.aslc 
| 14 -
 Platform/ARM/SgiPkg/RdN2Cfg3/RdN2Cfg3.fdf.inc 
| 10 
 12 files changed, 136 insertions(+), 23 deletions(-)
 create mode 100644 Platform/ARM/SgiPkg/RdN2Cfg3/RdN2Cfg3.dsc
 copy Platform/ARM/SgiPkg/AcpiTables/{RdN2AcpiTables.inf => 
RdN2Cfg3AcpiTables.inf} (90%)
 create mode 100644 Platform/ARM/SgiPkg/RdN2Cfg3/RdN2Cfg3.fdf.inc

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116262): https://edk2.groups.io/g/devel/message/116262
Mute This Topic: https://groups.io/mt/104668446/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 1/1] ArmPkg,MdePkg: move ArmLib.h to MdePkg

[Yao, Jiewen]

Right, if it is only required by ARM, then it should under ARM section.

Thank you
Yao, Jiewen

> -Original Message-
> From: Leif Lindholm 
> Sent: Friday, March 1, 2024 7:45 PM
> To: Yao, Jiewen ; Pierre Gondois
> ; devel@edk2.groups.io
> Cc: Ard Biesheuvel ; Liming Gao
> ; Kinney, Michael D ;
> Sami Mujawar ; Liu, Zhiguang
> 
> Subject: Re: [RFC PATCH 1/1] ArmPkg,MdePkg: move ArmLib.h to MdePkg
> 
> Thank you.
> 
> OK, that's logically consistent.
> So we'd need an ArmLibNull in MdePkg until ArmLib itself migrates there
> (ideally subsumed into BaseLib).
> 
> But the dependency in .inf should still be able to be declared under
> [LibraryClasses.AArch64, LibraryClasses.ARM]?
> 
> Regards,
> 
> Leif
> 
> On 2024-03-01 01:00, Yao, Jiewen wrote:
> > Sure.
> >
> > When we say "dependency", what we really mean is the dependency in INF file,
> not "dependency" in DSC file.
> >
> >  From package release perspective, only INF is the interface to other 
> > package.
> > The DSC is only the package internal stuff, you can create multiple DSCs or
> add/remove DSC freely.
> >
> > Having "dependency" in DSC does not matter.
> > Having dependency in INF is something we should care about.
> >
> > Thank you
> > Yao, Jiewen
> >
> >
> >> -Original Message-
> >> From: Leif Lindholm 
> >> Sent: Tuesday, February 13, 2024 1:38 AM
> >> To: Pierre Gondois ; devel@edk2.groups.io; Yao,
> >> Jiewen 
> >> Cc: Ard Biesheuvel ; Liming Gao
> >> ; Kinney, Michael D
> ;
> >> Sami Mujawar ; Liu, Zhiguang
> >> 
> >> Subject: Re: [RFC PATCH 1/1] ArmPkg,MdePkg: move ArmLib.h to MdePkg
> >>
> >> Jiewen, can you clarify what you said back in
> >> https://edk2.groups.io/g/devel/message/111551
> >> ?
> >>
> >> On 2024-02-12 17:24, Pierre Gondois wrote:
> > A ArmLibNull.inf library might also need to be created. If the
> > OpensslLibFullAccel.inf module will depend on the ArmLib library,
> > a Null implementation will be necessary for non-Arm architectures.
> 
>  Can ArmLib be declared under a [LibraryClasses.AArch64,
>  LibraryClasses.ARM]? Have I forgotten something that we discussed back
>  in ... November?
> >>>
> >>>   From [1], it seems the MdePkg/CryptoPkg should build without a
> dependency
> >>> on the ArmPkg. This is currently not really the case. cf. [2].
> >>>
> >>> However, having a ArmLibNull implementation in the MdePkg would allow to
> >>> avoid going in this direction when providing libraries to CryptoPkg.dsc.
> >>>
> >>> (Just in case, I think this ArmLibNull is a minor point.)
> >>
> >> Well, sure, it is now.
> >> Until we need a RiscV64LibNull, LoongarchLibNull, ...
> >>
> >>> [1] https://edk2.groups.io/g/devel/message/111545
> >>> [2]
> >>>
> >>
> https://github.com/tianocore/edk2/blob/8801c75b4d77c2e6e06b3ddc8560e0db
> >> 590f6342/CryptoPkg/CryptoPkg.dsc#L117
> >>>
> 
> > Otherwise I could apply and run the CryptoPkg/Arm native instructions
> > patchset on top of this patch.
> >
> > ---
> >
> > As a side note, it also seems that:
> > - ArmPkg/Include/Chipset/ArmCortexA5x.h
> >      isn't used anymore in edk2/edk2-platorms
> > - ArmPkg/Include/Chipset/ArmCortexA9.h
> >      is barely used in edk2-platforms.
> > Maybe the files should have been removed/simplified as part of
> > - cffa7925a293 ("ArmPkg: remove ArmCpuLib header and
> implementations")
> > - a913ad02479d ("ArmPlatformPkg: remove ArmVExpressPkg")
> 
>  I think you're right.
>  Well, ArmCortexA9.h is still *used*, but I can't imagine the Arm branch
>  of ArmVExpressLib has been build by anyone for some time. And surely the
>  inclusion of ArmVExpressLibSec in ArmVExpress-FVP-AArch64.dsc is
>  superfluous (such that that .inf can be deleted)?
> >>>
> >>> The file could just be moved in the Library. I assume you/Sami/Ard
> >>> will know more on the usage of the library itself,
> >>
> >> Sami?
> >>
> >> /
> >>   Leif
> >>
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116261): https://edk2.groups.io/g/devel/message/116261
Mute This Topic: https://groups.io/mt/102731845/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 09/10] OvmfPkg/ResetVector: leave SEV VC handler installed longer

[Gerd Hoffmann]

  Hi,

> >  OneTimeCall CheckSevFeatures
> > +cmp byte[WORK_AREA_GUEST_TYPE], 1
> > +jnz NoSevIa32
> > +OneTimeCall SevClearVcHandlerAndStack
> >  
> > +NoSevIa32:
> >  ;
> >  ; Restore initial EAX value into the EAX register
> >  ;
> 
> Did you miss Tom's review under v1?
> 
> https://edk2.groups.io/g/devel/message/116176

Saw the mail only after sending out v2, updated my local branch
meanwhile.

> I'm ready to merge this (adding Tom's R-b, if you, Gerd, confirm that
> that's what you want).

As stated in the cover letter I think it's better to not (yet) merge
patches 9+10 because BaseMemEncryptSevLib is not ready for 5-level
paging.  That way SEV will work fine (in 4-level paging mode) even when
building with PcdUse5LevelPageTable=TRUE.

thanks & take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116260): https://edk2.groups.io/g/devel/message/116260
Mute This Topic: https://groups.io/mt/104660115/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2][PATCH V2 1/2] ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges

[Ard Biesheuvel]

On Wed, 28 Feb 2024 at 17:59, Sami Mujawar  wrote:
>
> +Resending with email address for maintainers.
>
>
>
> Hi Ard, Leif,
>
> This patch adds macros that can be used to validate that the SPI ranges are 
> valid.
> These have been define here so that we do not duplicate it at multiple places.
>
> Can you let me know if I can merge this patch, please?
>

I have no problems with that, please go ahead.

Acked-by: Ard Biesheuvel 

I will note that we will have to clean this up in the future, though:
the architected GIC pieces should not be part of a library header, but
a separate IndustryStandard/ header that is separate from the GIC
library code. But that can wait until later.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116259): https://edk2.groups.io/g/devel/message/116259
Mute This Topic: https://groups.io/mt/103518972/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 08/10] OvmfPkg/ResetVector: wire up 5-level paging for TDX

[Gerd Hoffmann]

  Hi,

> Did you place the "CheckForSev:" label intentionally outside of the %if
> scope? If it was intentional, then I'm OK with it.
> 
> If it was unintended / an oversight, then next question: do you want me
> to move the label into the %if's scope for you, upon merge? Or do you
> like it better as written in your patch, after all?

I've placed it at the start of the SEV block without realizing that we
don't need it in the first place when compiling without 5-level support.

Moving it is fine with me.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116258): https://edk2.groups.io/g/devel/message/116258
Mute This Topic: https://groups.io/mt/104660117/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms][PATCH V4 2/4] Platform/ARM/N1Sdp: Add N1SdpNtFwConfigPei PEI module

[Sami Mujawar]

Hi Sahil,

There are multiple items that need fixing in this patch. Also, the changes in 
Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec from Patch 3/4 must be part of this 
patch.
To save my time from reviewing the whole thing again, I am going to fix that 
before merging.

However, do find my response inline marked [SAMI] and please do go through my 
feedback so that you know what to look for next time.

Regards,

Sami Mujawar

On 04/01/2024, 13:16, "sahil" mailto:sa...@arm.com>> wrote:


This patch adds a PEI to parse NT_FW_CONFIG and pass it to
other PEI modules(as PPI) and DXE modules(as HOB).


Signed-off-by: sahil mailto:sa...@arm.com>>
---
Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.inf | 41 ++
Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c | 132 



[SAMI] I think these files should be moved to 
Silicon/ARM/NeoverseN1Soc/Library/N1SdpNtFwConfigPei.
I will do that before merging.

2 files changed, 173 insertions(+)


diff --git a/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.inf 
b/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.inf
new file mode 100644
index ..363351b5a1df
--- /dev/null
+++ b/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.inf
@@ -0,0 +1,41 @@
+## @file


+# This PEI module parse the NtFwConfig for N1Sdp platform and produce


+# the PPI and HOB.


+#


+# Copyright (c) 2024, ARM Limited. All rights reserved.


+#


+# SPDX-License-Identifier: BSD-2-Clause-Patent


+#


+##


+


+[Defines]


+ INF_VERSION = 0x0001001B


+ BASE_NAME = N1SdpNtFwConfigPei


+ FILE_GUID = CE76D56C-D3A5-4763-9138-DF09E1D1B614


+ MODULE_TYPE = PEIM


+ VERSION_STRING = 1.0


+ ENTRY_POINT = NtFwConfigPeEntryPoint


+


+[Sources]


+ NtFwConfigPei.c


+


+[Packages]


+ EmbeddedPkg/EmbeddedPkg.dec


+ MdePkg/MdePkg.dec


+ Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec


+


+[LibraryClasses]


+ DebugLib


+ FdtLib


+ HobLib


+ PeimEntryPoint


+


+[Ppis]


+ gArmNeoverseN1SocPlatformInfoDescriptorPpiGuid


+ gArmNeoverseN1SocParameterPpiGuid


+


+[Guids]


+ gArmNeoverseN1SocPlatformInfoDescriptorGuid


+


+[Depex]


+ gArmNeoverseN1SocParameterPpiGuid


diff --git a/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c 
b/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c
new file mode 100644
index ..330377d21a79
--- /dev/null
+++ b/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c
@@ -0,0 +1,132 @@
+/** @file


+


+ Copyright (c) 2024, ARM Limited. All rights reserved.


+ SPDX-License-Identifier: BSD-2-Clause-Patent


+


+**/


+


+#include 


+#include 


+#include 


+


+#include 


+#include 


+


+STATIC EFI_PEI_PPI_DESCRIPTOR mPpi;


+


+/**


+ The entrypoint of the module, parse NtFwConfig and produce the PPI and HOB.


+


+ @param[in] FileHandle Handle of the file being invoked.


+ @param[in] PeiServices Describes the list of possible PEI Services.


+


+ @retval EFI_SUCCESS Either no NT_FW_CONFIG was given by EL3 firmware


+ OR the N1Sdp FDT HOB was successfully created.
[SAMI] Would it not be enough to say Success? The documentation for the 
function already mentions what is expected.


+ @retval EFI_NOT_FOUND Error processing the DTB


+ @retval EFI_OUT_OF_RESOURCES Could not allocate memory for the HOB


+ @retval * Other errors are possible.


+**/


+EFI_STATUS


+EFIAPI


+NtFwConfigPeEntryPoint (


+ IN EFI_PEI_FILE_HANDLE FileHandle,


+ IN CONST EFI_PEI_SERVICES **PeiServices


+ )


+{


+ CONST NEOVERSEN1SOC_EL3_FW_HANDOFF_PARAM_PPI *ParamPpi;


+ CONST UINT32 *Property;


+ INT32 Offset;


+ NEOVERSEN1SOC_PLAT_INFO *PlatInfo;


+ INT32 Status;
[SAMI] This should be EFI_STATUS.


+


+ PlatInfo = BuildGuidHob (


+ ,


+ sizeof (*PlatInfo)


+ );


+


+ if (PlatInfo == NULL) {


+ DEBUG ((


+ DEBUG_ERROR,


+ "[%a]: failed to allocate platform info HOB\n",


+ gEfiCallerBaseName


+ ));


+ return EFI_OUT_OF_RESOURCES;


+ }


+


+ Status = PeiServicesLocatePpi (


+ ,


+ 0,


+ NULL,


+ (VOID **)


+ );

[SAMI] I think Locate PPI should be done fist, that way if it fails we do not 
allocate the HOB.
+


+ if (EFI_ERROR (Status)) {


+ DEBUG ((


+ DEBUG_ERROR,


+ "[%a]: failed to locate gArmNeoverseN1SocParameterPpiGuid - %r\n",


+ gEfiCallerBaseName,


+ Status


+ ));


+ return Status;


+ }


+


+ if (fdt_check_header (ParamPpi->NtFwConfig) != 0) {
[SAMI] There should be a check to see if ParamPpi is NULL. Also ParamPpi is 
dereferenced too many times in this function. It is advisable to have a local 
variable instead.


+ DEBUG ((DEBUG_ERROR, "Invalid DTB file %p passed\n", ParamPpi->NtFwConfig));


+ return EFI_NOT_FOUND;


+ }


+


+ Offset = fdt_subnode_offset (ParamPpi->NtFwConfig, 0, "platform-info");


+ if (Offset == -FDT_ERR_NOTFOUND) {


+ DEBUG ((DEBUG_ERROR, "Invalid DTB : platform-info node not found\n"));


+ return EFI_NOT_FOUND;


+ }


+


+ Property = fdt_getprop (ParamPpi->NtFwConfig, 

Re: [edk2-devel] [edk2-platforms][PATCH V4 4/4] Silicon/ARM/NeoverseN1Soc: Consume N1SdpNtFwConfigPei supplied data

[Sami Mujawar]

Hi Sahil,

Thank you for this patch.

This patch can clearly be split into 2, one for the 
Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/* changes and 
the other for the  Silicon/ARM/NeoverseN1Soc/* changes.
I am going to do that before merging.

With that,
Reviewed-by: Sami Mujawar 

Regards,

Sami Mujawar


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116256): https://edk2.groups.io/g/devel/message/116256
Mute This Topic: https://groups.io/mt/103521648/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms][PATCH V4 3/4] Platform/ARM/N1Sdp: Enable N1SdpNtFwConfigPei PEI module for N1Sdp

[Sami Mujawar]

Hi Sahil,

Thank you for this patch.

On Thu, Jan 4, 2024 at 05:16 AM, sahil wrote:

> 
> diff --git a/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
> b/Silicon/ARM/Neov=
> erseN1Soc/NeoverseN1Soc.dec
> index c04162e7e7cd..cca5bf45db67 100644
> --- a/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
> +++ b/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
> @@ -22,6 +22,8 @@
> Include # Root include for the package=0D
> =0D
> [Guids.common]=0D
> + # ARM NeoverseN1Soc Platform Info descriptor=0D
> + gArmNeoverseN1SocPlatformInfoDescriptorGuid =3D { 0x9fa16eb5, 0xce13,
> 0x=
> 4d37, { 0x96, 0xf0, 0x0a, 0xb5, 0xf1, 0xab, 0xff, 0x01 } }=0D
> gArmNeoverseN1SocTokenSpaceGuid =3D { 0xab93eb78, 0x60d7, 0x4099, { 0xac=
> , 0xeb, 0x6d, 0xb5, 0x02, 0x58, 0x7c, 0x24 } }=0D
> =0D
> [PcdsFixedAtBuild]=0D
> @@ -85,4 +87,5 @@
> gArmNeoverseN1SocTokenSpaceGuid.PcdRemotePcieSegmentNumber|2|UINT32|0x00=
> 51=0D
> =0D
> [Ppis]=0D
> + gArmNeoverseN1SocPlatformInfoDescriptorPpiGuid =3D { 0x21D04AD4,
> 0x4D23,=
> 0x426D, { 0x8D, 0x3E, 0x92, 0x23, 0x3E, 0xF4, 0xB9, 0x5E } }=0D
> gArmNeoverseN1SocParameterPpiGuid =3D { 0x4DDD5A72, 0x31AD, 0x4B20, { 0x=
> 8F, 0x5F, 0xB3, 0xE8, 0x24, 0x6F, 0x80, 0x2B } }=0D

These changes should be part of patch 2/4. I am going to move this file to the 
correct patch before merging.

With that fixed,
Reviewed-by: Sami Mujawar 

Regards,

Sami Mujawar


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116255): https://edk2.groups.io/g/devel/message/116255
Mute This Topic: https://groups.io/mt/103521646/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms][PATCH V4 1/4] Silicon/ARM/NeoverseN1Soc: Extract NT_FW_CONFIG address passed by TF-A

[Sami Mujawar]

Hi Sahil,

Thank you for this patch.
These changes look good to me.

Reviewed-by: Sami Mujawar 

Regards,

Sami Mujawar


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116254): https://edk2.groups.io/g/devel/message/116254
Mute This Topic: https://groups.io/mt/103521643/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: 回复: [edk2-devel] [edk2-InfSpecification PATCH 0/2] README: INF_VERSION >= 1.30: declare ProcessLibraryCon...() for SEC modules

[Laszlo Ersek]

On 3/1/24 13:52, gaoliming via groups.io wrote:
> Looks good. Reviewed-by: Liming Gao 

Thank you, merged as commit range a31e3c842bee..1ea6546578fe.

Laszlo

> 
>> -邮件原件-
>> 发件人: devel@edk2.groups.io  代表 Laszlo Ersek
>> 发送时间: 2024年2月25日 5:29
>> 收件人: edk2-devel-groups-io 
>> 抄送: Bob Feng ; Liming Gao
>> ; Michael D Kinney
>> ; Rebecca Cran ; Yuwei
>> Chen 
>> 主题: [edk2-devel] [edk2-InfSpecification PATCH 0/2] README:
>> INF_VERSION >= 1.30: declare ProcessLibraryCon...() for SEC modules
>>
>> The first patch is a minor cleanup, the new feature is documented in the
>> second patch.
>>
>> Rendered version:
>>
>>
>>
> https://lersek.github.io/edk2-InfSpecification/ProcessLibraryConstructorList
> -S
>> EC-991/
>>
>>
> https://github.com/lersek/edk2-InfSpecification/actions/workflows/gitbook-a
>> ction.yml
>>
>> Cc: Bob Feng 
>> Cc: Liming Gao 
>> Cc: Michael D Kinney 
>> Cc: Rebecca Cran 
>> Cc: Yuwei Chen 
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (2):
>>   README.md: fix revision history whitespace
>>   README: INF_VERSION >= 1.30: declare ProcessLibraryCon...() for SEC
>> modules
>>
>>  README.md | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>>
>> base-commit: a31e3c842beed72661b6ccf9dbb34df8d0c1afa6
>>
>>
>>
>>
> 
> 
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116253): https://edk2.groups.io/g/devel/message/116253
Mute This Topic: https://groups.io/mt/104662704/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: 回复: [edk2-devel] [edk2-BuildSpecification PATCH 0/3] AutoGen.h: declare ProcessLibraryConstructorList() for SEC modules

[Laszlo Ersek]

On 3/1/24 13:53, gaoliming via groups.io wrote:
> Looks good. Reviewed-by: Liming Gao 

Thank you, merged as commit range db69f5661cae..7a7165a7d199.

Laszlo

> 
>> -邮件原件-
>> 发件人: devel@edk2.groups.io  代表 Laszlo Ersek
>> 发送时间: 2024年2月25日 5:24
>> 收件人: edk2-devel-groups-io 
>> 抄送: Bob Feng ; Liming Gao
>> ; Michael D Kinney
>> ; Rebecca Cran ; Yuwei
>> Chen 
>> 主题: [edk2-devel] [edk2-BuildSpecification PATCH 0/3] AutoGen.h: declare
>> ProcessLibraryConstructorList() for SEC modules
>>
>> The first two patches are cleanups, the new feature is documented in the
>> third patch.
>>
>> I managed to render these updates to public HTML, following
>>
>> (a) Mike's note from October 2020:
>>
>>   https://edk2.groups.io/g/devel/message/66426
>>   msgid
>> > prd11.prod.outlook.com>
>>
>> (b) the github access token / project secrets steps at
>>
>>
>> https://github.com/ZanderZhao/gitbook-action?tab=readme-ov-file#step2--g
>> enerate-token-and-add-to-secrets
>>
>> Therefore:
>>
>>
>>
> https://lersek.github.io/edk2-BuildSpecification/ProcessLibraryConstructorLi
> s
>> t-SEC-991/
>>
>>
> https://github.com/lersek/edk2-BuildSpecification/actions/workflows/gitbook
>> -action.yml
>>
>> Cc: Bob Feng 
>> Cc: Liming Gao 
>> Cc: Michael D Kinney 
>> Cc: Rebecca Cran 
>> Cc: Yuwei Chen 
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (3):
>>   README.md: fix revision history whitespace
>>   AutoGen.c: list the SEC module type for /
>> (VOID)
>>   AutoGen.h: declare ProcessLibraryConstructorList() for SEC modules
>>
>>  8_pre-build_autogen_stage/83_auto-generated_code.md | 42
>> +++-
>>  README.md   |  5 ++-
>>  2 files changed, 36 insertions(+), 11 deletions(-)
>>
>>
>> base-commit: db69f5661caec977fac9730e21e5a1132f6ff80b
>>
>>
>>
>>
> 
> 
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116252): https://edk2.groups.io/g/devel/message/116252
Mute This Topic: https://groups.io/mt/104662733/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 10/10] OvmfPkg/ResetVector: wire up 5-level paging for SEV

[Laszlo Ersek]

On 3/1/24 08:44, Gerd Hoffmann wrote:
> Turn the GetSevCBitMaskAbove31 OneTimeCall into a macro because we
> need that twice (for 4-level and 5-level paging).  Change include
> order to allow AmdSev.asm macros being used in PageTables64.asm.

I *think* the include order change will not only make the macros
visible, but also rearrange how the code (the binary instructions) are
laid out in the reset vector -- however, that should not be a problem.

Reviewed-by: Laszlo Ersek 


> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  OvmfPkg/ResetVector/Ia32/AmdSev.asm   | 16 
>  OvmfPkg/ResetVector/Ia32/PageTables64.asm | 14 +-
>  OvmfPkg/ResetVector/ResetVector.nasmb |  4 ++--
>  3 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/OvmfPkg/ResetVector/Ia32/AmdSev.asm 
> b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
> index cbb86871636f..c577f5572f04 100644
> --- a/OvmfPkg/ResetVector/Ia32/AmdSev.asm
> +++ b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
> @@ -146,6 +146,14 @@ BITS32
>  jmp %%TerminateHlt
>  %endmacro
>  
> +; Get the C-bit mask above 31.
> +; Modified: EDX
> +;
> +; The value is returned in the EDX
> +%macro GetSevCBitMaskAbove31 0
> +mov edx, dword[SEV_ES_WORK_AREA_ENC_MASK + 4]
> +%endmacro
> +
>  ; Terminate the guest due to unexpected response code.
>  SevEsUnexpectedRespTerminate:
>  TerminateVmgExitTERM_UNEXPECTED_RESP_CODE
> @@ -191,14 +199,6 @@ pageTableEntries4kLoop:
>  SevClearPageEncMaskForGhcbPageExit:
>  OneTimeCallRet SevClearPageEncMaskForGhcbPage
>  
> -; Get the C-bit mask above 31.
> -; Modified: EDX
> -;
> -; The value is returned in the EDX
> -GetSevCBitMaskAbove31:
> -mov   edx, dword[SEV_ES_WORK_AREA_ENC_MASK + 4]
> -OneTimeCallRet GetSevCBitMaskAbove31
> -
>  %endif
>  
>  ; Check if Secure Encrypted Virtualization (SEV) features are enabled.
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm 
> b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index 29ce155eed8d..92d134441abe 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -247,11 +247,23 @@ SevInit:
>  ; SEV workflow
>  ;
>  ClearOvmfPageTables
> +%if PG_5_LEVEL
> +Check5LevelPaging Sev4Level
>  ; If SEV is enabled, the C-bit position is always above 31.
>  ; The mask will be saved in the EDX and applied during the
>  ; the page table build below.
> -OneTimeCall   GetSevCBitMaskAbove31
> +GetSevCBitMaskAbove31
> +CreatePageTables5Level edx
> +Enable5LevelPaging
> +jmp SevCommon
> +Sev4Level:
> +%endif
> +; If SEV is enabled, the C-bit position is always above 31.
> +; The mask will be saved in the EDX and applied during the
> +; the page table build below.
> +GetSevCBitMaskAbove31
>  CreatePageTables4Level edx
> +SevCommon:
>  ; Clear the C-bit from the GHCB page if the SEV-ES is enabled.
>  OneTimeCall   SevClearPageEncMaskForGhcbPage
>  OneTimeCall   SevClearVcHandlerAndStack
> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb 
> b/OvmfPkg/ResetVector/ResetVector.nasmb
> index 2bd80149e58b..ba83bc7b3124 100644
> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
> @@ -92,6 +92,8 @@
>  %define SNP_SEC_MEM_BASE_DESC_3   (CPUID_BASE + CPUID_SIZE + 
> SEV_SNP_KERNEL_HASHES_SIZE)
>  %define SNP_SEC_MEM_SIZE_DESC_3   (FixedPcdGet32 (PcdOvmfPeiMemFvBase) - 
> SNP_SEC_MEM_BASE_DESC_3)
>  
> +%include "Ia32/AmdSev.asm"
> +
>  %ifdef ARCH_X64
>#include 
>  
> @@ -144,8 +146,6 @@
>%include "X64/OvmfSevMetadata.asm"
>  %endif
>  
> -%include "Ia32/AmdSev.asm"
> -
>  %include "Ia16/Real16ToFlat32.asm"
>  %include "Ia16/Init16.asm"
>  



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116251): https://edk2.groups.io/g/devel/message/116251
Mute This Topic: https://groups.io/mt/104660118/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 09/10] OvmfPkg/ResetVector: leave SEV VC handler installed longer

[Laszlo Ersek]

On 3/1/24 08:44, Gerd Hoffmann wrote:
> When running in SEV mode keep the VC handler installed.
> Add a function to uninstall it later.
> 
> This allows using the cpuid instruction in SetCr3ForPageTables64,
> which is needed to check for la57 & 1G page support.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  OvmfPkg/ResetVector/Ia32/AmdSev.asm   | 12 ++--
>  OvmfPkg/ResetVector/Ia32/PageTables64.asm |  1 +
>  OvmfPkg/ResetVector/Main.asm  |  4 
>  3 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/ResetVector/Ia32/AmdSev.asm 
> b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
> index 23e4c5ebbe92..cbb86871636f 100644
> --- a/OvmfPkg/ResetVector/Ia32/AmdSev.asm
> +++ b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
> @@ -320,9 +320,9 @@ NoSevEsVcHlt:
>  NoSevPass:
>  xor   eax, eax
>  
> -SevExit:
>  ;
> -; Clear exception handlers and stack
> +; When NOT running in SEV mode: clear exception handlers and stack here.
> +; Otherwise: SevClearVcHandlerAndStack must be called later.
>  ;
>  push  eax
>  mov   eax, ADDR_OF(IdtrClear)
> @@ -330,8 +330,16 @@ SevExit:
>  pop   eax
>  mov   esp, 0
>  
> +SevExit:
>  OneTimeCallRet CheckSevFeatures
>  
> +SevClearVcHandlerAndStack:
> +; Clear exception handlers and stack
> +mov   eax, ADDR_OF(IdtrClear)
> +lidt  [cs:eax]
> +mov   esp, 0
> +OneTimeCallRet SevClearVcHandlerAndStack
> +
>  ; Start of #VC exception handling routines
>  ;
>  
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm 
> b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index b922c845f297..29ce155eed8d 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -254,6 +254,7 @@ SevInit:
>  CreatePageTables4Level edx
>  ; Clear the C-bit from the GHCB page if the SEV-ES is enabled.
>  OneTimeCall   SevClearPageEncMaskForGhcbPage
> +OneTimeCall   SevClearVcHandlerAndStack
>  jmp SetCr3
>  
>  TdxBspInit:
> diff --git a/OvmfPkg/ResetVector/Main.asm b/OvmfPkg/ResetVector/Main.asm
> index 46cfa87c4c0a..88b25db3bc9e 100644
> --- a/OvmfPkg/ResetVector/Main.asm
> +++ b/OvmfPkg/ResetVector/Main.asm
> @@ -80,7 +80,11 @@ SearchBfv:
>  ; Set the OVMF/SEV work area as appropriate.
>  ;
>  OneTimeCall CheckSevFeatures
> +cmp byte[WORK_AREA_GUEST_TYPE], 1
> +jnz NoSevIa32
> +OneTimeCall SevClearVcHandlerAndStack
>  
> +NoSevIa32:
>  ;
>  ; Restore initial EAX value into the EAX register
>  ;

Did you miss Tom's review under v1?

https://edk2.groups.io/g/devel/message/116176

The patch is identical to its v1 counterpart, which should not be a
problem in itself (Tom mentioned a small, *optional*, simplification,
IIUC); however, I don't understand why you didn't pick up Tom's R-b.

I'm ready to merge this (adding Tom's R-b, if you, Gerd, confirm that
that's what you want).

Having deferred to Tom's judgement on this:

Acked-by: Laszlo Ersek 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116250): https://edk2.groups.io/g/devel/message/116250
Mute This Topic: https://groups.io/mt/104660115/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] 回复: [PATCH v1] MdeModulePkg/Core/Pei: Improve the copy performance

[gaoliming via groups.io]

I agree this change. It should have no negative impact. Reviewed-by: Liming
Gao 

Michael:
  Have you any comments for this change?

Thanks
Liming
> -邮件原件-
> 发件人: Zhihao Li 
> 发送时间: 2024年3月1日 15:12
> 收件人: devel@edk2.groups.io
> 抄送: Liming Gao 
> 主题: [PATCH v1] MdeModulePkg/Core/Pei: Improve the copy performance
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4697
> 
> EvacuateTempRam function will copy the temporary memory context to the
> rebased
> pages and the raw pages. Migrations of rebased PEIMs is from cache to
> memory,
> while raw PEIMs is from memory to memory. So the migrations of raw PEIMs
> is slower than rebased PEIMs. Experimental data indicates that changing
the
> source
> address of raw PEIMs migration will improve performance by 35%.
> 
> Cc: Liming Gao 
> Signed-off-by: Zhihao Li 
> ---
>  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> index 4cd8c843cd..ca37bde482 100644
> --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> @@ -1,7 +1,7 @@
>  /** @file
>EFI PEI Core dispatch services
> 
> -Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2024, Intel Corporation. All rights reserved.
>  (C) Copyright 2016 Hewlett Packard Enterprise Development LP
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -1305,7 +1305,7 @@ EvacuateTempRam (
>  );
>  ASSERT_EFI_ERROR (Status);
>  RawDataFvHeader = (EFI_FIRMWARE_VOLUME_HEADER
> *)(UINTN)FvHeaderAddress;
> -CopyMem (RawDataFvHeader, MigratedFvHeader,
> (UINTN)FvHeader->FvLength);
> +CopyMem (RawDataFvHeader, FvHeader,
> (UINTN)FvHeader->FvLength);
>  MigratedFvInfo.FvDataBase =
> (UINT32)(UINTN)RawDataFvHeader;
>}
> 
> --
> 2.26.2.windows.1





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116249): https://edk2.groups.io/g/devel/message/116249
Mute This Topic: https://groups.io/mt/104662794/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 08/10] OvmfPkg/ResetVector: wire up 5-level paging for TDX

[Laszlo Ersek]

On 3/1/24 08:44, Gerd Hoffmann wrote:
> BSP workflow is quite simliar to the non-coco case.
> 
> TDX_WORK_AREA_PGTBL_READY is used to record the paging mode:
>   1 == 4-level paging
>   2 == 5-level paging
> 
> APs will look at TDX_WORK_AREA_PGTBL_READY to figure whenever
> they should enable 5-level paging or not.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  OvmfPkg/ResetVector/Ia32/IntelTdx.asm | 13 -
>  OvmfPkg/ResetVector/Ia32/PageTables64.asm | 16 
>  2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/ResetVector/Ia32/IntelTdx.asm 
> b/OvmfPkg/ResetVector/Ia32/IntelTdx.asm
> index c6b86019dfb9..7d775591a05b 100644
> --- a/OvmfPkg/ResetVector/Ia32/IntelTdx.asm
> +++ b/OvmfPkg/ResetVector/Ia32/IntelTdx.asm
> @@ -179,7 +179,7 @@ InitTdx:
>  ;
>  ; Modified:  EAX, EDX
>  ;
> -; 0-NonTdx, 1-TdxBsp, 2-TdxAps
> +; 0-NonTdx, 1-TdxBsp, 2-TdxAps, 3-TdxAps5Level
>  ;
>  CheckTdxFeaturesBeforeBuildPagetables:
>  xor eax, eax
> @@ -200,6 +200,17 @@ TdxPostBuildPageTables:
>  mov byte[TDX_WORK_AREA_PGTBL_READY], 1
>  OneTimeCallRet TdxPostBuildPageTables
>  
> +%if PG_5_LEVEL
> +
> +;
> +; Set byte[TDX_WORK_AREA_PGTBL_READY] to 2
> +;
> +TdxPostBuildPageTables5Level:
> +mov byte[TDX_WORK_AREA_PGTBL_READY], 2
> +OneTimeCallRet TdxPostBuildPageTables5Level
> +
> +%endif
> +
>  ;
>  ; Check if TDX is enabled
>  ;
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm 
> b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index e15945da0476..b922c845f297 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -44,6 +44,7 @@ BITS32
>  
>  %define TDX_BSP 1
>  %define TDX_AP  2
> +%define TDX_AP_5_LEVEL  3
>  
>  ;
>  ; For OVMF, build some initial page tables at
> @@ -214,7 +215,14 @@ SetCr3ForPageTables64:
>  jeTdxBspInit
>  cmp   eax, TDX_AP
>  jeSetCr3
> +%if PG_5_LEVEL
> +cmp   eax, TDX_AP_5_LEVEL
> +jne   CheckForSev
> +Enable5LevelPaging
> +jmp   SetCr3
> +%endif
>  
> +CheckForSev:
>  ; Check whether the SEV is active and populate the SevEsWorkArea
>  OneTimeCall   CheckSevFeatures
>  cmp   byte[WORK_AREA_GUEST_TYPE], 1

Minor nit: we don't neet the "CheckForSev:" jump label at all if
PG_5_LEVEL is absent, so the "CheckForSev:" label definition should
still be in the "%if PG_5_LEVEL" scope.

(My proposal under v1 patch#6 was:

%if PG_5_LEVEL
cmp   eax, TDX_AP_5_LEVEL
jne   CheckForSev
Enable5LevelPaging
jmp   SetCr3
CheckForSev:
%endif

)

Did you place the "CheckForSev:" label intentionally outside of the %if
scope? If it was intentional, then I'm OK with it.

If it was unintended / an oversight, then next question: do you want me
to move the label into the %if's scope for you, upon merge? Or do you
like it better as written in your patch, after all?

Reviewed-by: Laszlo Ersek 

Thanks
Laszlo




> @@ -253,6 +261,14 @@ TdxBspInit:
>  ; TDX BSP workflow
>  ;
>  ClearOvmfPageTables
> +%if PG_5_LEVEL
> +Check5LevelPaging Tdx4Level
> +CreatePageTables5Level 0
> +OneTimeCall TdxPostBuildPageTables5Level
> +Enable5LevelPaging
> +jmp SetCr3
> +Tdx4Level:
> +%endif
>  CreatePageTables4Level 0
>  OneTimeCall TdxPostBuildPageTables
>  jmp SetCr3



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116248): https://edk2.groups.io/g/devel/message/116248
Mute This Topic: https://groups.io/mt/104660117/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

回复: [edk2-devel] [edk2-BuildSpecification PATCH 0/3] AutoGen.h: declare ProcessLibraryConstructorList() for SEC modules

[gaoliming via groups.io]

Looks good. Reviewed-by: Liming Gao 

> -邮件原件-
> 发件人: devel@edk2.groups.io  代表 Laszlo Ersek
> 发送时间: 2024年2月25日 5:24
> 收件人: edk2-devel-groups-io 
> 抄送: Bob Feng ; Liming Gao
> ; Michael D Kinney
> ; Rebecca Cran ; Yuwei
> Chen 
> 主题: [edk2-devel] [edk2-BuildSpecification PATCH 0/3] AutoGen.h: declare
> ProcessLibraryConstructorList() for SEC modules
> 
> The first two patches are cleanups, the new feature is documented in the
> third patch.
> 
> I managed to render these updates to public HTML, following
> 
> (a) Mike's note from October 2020:
> 
>   https://edk2.groups.io/g/devel/message/66426
>   msgid
>  prd11.prod.outlook.com>
> 
> (b) the github access token / project secrets steps at
> 
> 
> https://github.com/ZanderZhao/gitbook-action?tab=readme-ov-file#step2--g
> enerate-token-and-add-to-secrets
> 
> Therefore:
> 
> 
>
https://lersek.github.io/edk2-BuildSpecification/ProcessLibraryConstructorLi
s
> t-SEC-991/
> 
>
https://github.com/lersek/edk2-BuildSpecification/actions/workflows/gitbook
> -action.yml
> 
> Cc: Bob Feng 
> Cc: Liming Gao 
> Cc: Michael D Kinney 
> Cc: Rebecca Cran 
> Cc: Yuwei Chen 
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (3):
>   README.md: fix revision history whitespace
>   AutoGen.c: list the SEC module type for /
> (VOID)
>   AutoGen.h: declare ProcessLibraryConstructorList() for SEC modules
> 
>  8_pre-build_autogen_stage/83_auto-generated_code.md | 42
> +++-
>  README.md   |  5 ++-
>  2 files changed, 36 insertions(+), 11 deletions(-)
> 
> 
> base-commit: db69f5661caec977fac9730e21e5a1132f6ff80b
> 
> 
> 
> 





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116247): https://edk2.groups.io/g/devel/message/116247
Mute This Topic: https://groups.io/mt/104662733/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

回复: [edk2-devel] [edk2-InfSpecification PATCH 0/2] README: INF_VERSION >= 1.30: declare ProcessLibraryCon...() for SEC modules

[gaoliming via groups.io]

Looks good. Reviewed-by: Liming Gao 

> -邮件原件-
> 发件人: devel@edk2.groups.io  代表 Laszlo Ersek
> 发送时间: 2024年2月25日 5:29
> 收件人: edk2-devel-groups-io 
> 抄送: Bob Feng ; Liming Gao
> ; Michael D Kinney
> ; Rebecca Cran ; Yuwei
> Chen 
> 主题: [edk2-devel] [edk2-InfSpecification PATCH 0/2] README:
> INF_VERSION >= 1.30: declare ProcessLibraryCon...() for SEC modules
> 
> The first patch is a minor cleanup, the new feature is documented in the
> second patch.
> 
> Rendered version:
> 
> 
>
https://lersek.github.io/edk2-InfSpecification/ProcessLibraryConstructorList
-S
> EC-991/
> 
>
https://github.com/lersek/edk2-InfSpecification/actions/workflows/gitbook-a
> ction.yml
> 
> Cc: Bob Feng 
> Cc: Liming Gao 
> Cc: Michael D Kinney 
> Cc: Rebecca Cran 
> Cc: Yuwei Chen 
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (2):
>   README.md: fix revision history whitespace
>   README: INF_VERSION >= 1.30: declare ProcessLibraryCon...() for SEC
> modules
> 
>  README.md | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> 
> base-commit: a31e3c842beed72661b6ccf9dbb34df8d0c1afa6
> 
> 
> 
> 





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116246): https://edk2.groups.io/g/devel/message/116246
Mute This Topic: https://groups.io/mt/104662704/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 06/10] OvmfPkg/ResetVector: add 5-level paging support

[Laszlo Ersek]

On 3/1/24 08:43, Gerd Hoffmann wrote:
> Add macros to check for 5-level paging and gigabyte page support.
> Enable 5-level paging for the non-confidential-computing case.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  OvmfPkg/ResetVector/ResetVector.inf   |   1 +
>  OvmfPkg/ResetVector/Ia32/PageTables64.asm | 100 ++
>  OvmfPkg/ResetVector/ResetVector.nasmb |   1 +
>  3 files changed, 102 insertions(+)
> 
> diff --git a/OvmfPkg/ResetVector/ResetVector.inf 
> b/OvmfPkg/ResetVector/ResetVector.inf
> index a4154ca90c28..65f71b05a02e 100644
> --- a/OvmfPkg/ResetVector/ResetVector.inf
> +++ b/OvmfPkg/ResetVector/ResetVector.inf
> @@ -64,3 +64,4 @@ [FixedPcd]
>gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase
>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm 
> b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index 84a7b4efc019..2d7fd523e4b1 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -101,6 +101,97 @@ BITS32
>  loop.pageTableEntriesLoop4Level
>  %endmacro
>  
> +;
> +; Check whenever 5-level paging can be used
> +;
> +; Argument: jump label for 4-level paging
> +;
> +%macro Check5LevelPaging 1
> +; check for cpuid leaf 0x07
> +mov eax, 0x00
> +cpuid
> +cmp eax, 0x07
> +jb  %1
> +
> +; check for la57 (aka 5-level paging)
> +mov eax, 0x07
> +mov ecx, 0x00
> +cpuid
> +bt  ecx, 16
> +jnc %1
> +
> +; check for cpuid leaf 0x8001
> +mov eax, 0x8000
> +cpuid
> +cmp eax, 0x8001
> +jb  %1
> +
> +; check for 1g pages
> +mov eax, 0x8001
> +cpuid
> +bt  edx, 26
> +jnc %1
> +%endmacro
> +
> +;
> +; Create page tables for 5-level paging with gigabyte pages
> +;
> +; Argument: upper 32 bits of the page table entries
> +;
> +; We have 6 pages available for the early page tables,
> +; we use four of them:
> +;PT_ADDR(0)  - level 5 directory
> +;PT_ADDR(0x1000) - level 4 directory
> +;PT_ADDR(0x2000) - level 2 directory (0 -> 1GB)
> +;PT_ADDR(0x3000) - level 3 directory
> +;
> +; The level 2 directory for the first gigabyte has the same
> +; physical address in both 4-level and 5-level paging mode,
> +; SevClearPageEncMaskForGhcbPage depends on this.
> +;
> +; The 1 GB -> 4 GB range is mapped using 1G pages in the
> +; level 3 directory.
> +;
> +%macro CreatePageTables5Level 1
> +; level 5
> +mov dword[PT_ADDR (0)], PT_ADDR (0x1000) + PAGE_PDE_DIRECTORY_ATTR
> +mov dword[PT_ADDR (4)], %1
> +
> +; level 4
> +mov dword[PT_ADDR (0x1000)], PT_ADDR (0x3000) + 
> PAGE_PDE_DIRECTORY_ATTR
> +mov dword[PT_ADDR (0x1004)], %1
> +
> +; level 3 (1x -> level 2, 3x 1GB)
> +mov dword[PT_ADDR (0x3000)], PT_ADDR (0x2000) + 
> PAGE_PDE_DIRECTORY_ATTR
> +mov dword[PT_ADDR (0x3004)], %1
> +mov dword[PT_ADDR (0x3008)], (1 << 30) + PAGE_PDE_LARGEPAGE_ATTR
> +mov dword[PT_ADDR (0x300c)], %1
> +mov dword[PT_ADDR (0x3010)], (2 << 30) + PAGE_PDE_LARGEPAGE_ATTR
> +mov dword[PT_ADDR (0x3014)], %1
> +mov dword[PT_ADDR (0x3018)], (3 << 30) + PAGE_PDE_LARGEPAGE_ATTR
> +mov dword[PT_ADDR (0x301c)], %1
> +
> +;
> +; level 2 (512 * 2MB entries => 1GB)
> +;
> +mov ecx, 0x200
> +.pageTableEntriesLoop5Level:
> +mov eax, ecx
> +dec eax
> +shl eax, 21
> +add eax, PAGE_PDE_LARGEPAGE_ATTR
> +mov dword[ecx * 8 + PT_ADDR (0x2000 - 8)], eax
> +mov dword[(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], %1
> +loop.pageTableEntriesLoop5Level
> +%endmacro
> +
> +%macro Enable5LevelPaging 0
> +; set la57 bit in cr4
> +mov eax, cr4
> +bts eax, 12
> +mov cr4, eax
> +%endmacro
> +
>  ;
>  ; Modified:  EAX, EBX, ECX, EDX
>  ;
> @@ -125,6 +216,13 @@ SetCr3ForPageTables64:
>  ; normal (non-CoCo) workflow
>  ;
>  ClearOvmfPageTables
> +%if PG_5_LEVEL
> +Check5LevelPaging Paging4Level
> +CreatePageTables5Level 0
> +Enable5LevelPaging
> +jmp SetCr3
> +Paging4Level:
> +%endif
>  CreatePageTables4Level 0
>  jmp SetCr3
>  
> @@ -152,6 +250,8 @@ TdxBspInit:
>  jmp SetCr3
>  
>  SetCr3:
> +;
> +; common workflow
>  ;
>  ; Set CR3 now that the paging structures are available
>  ;

Nice touch, moving the "common workflow" comment here!

Reviewed-by: Laszlo Ersek 


> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb 
> b/OvmfPkg/ResetVector/ResetVector.nasmb
> index 366a70fb9992..2bd80149e58b 100644
> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
> @@ -53,6 +53,7 @@
>  
>  %define WORK_AREA_GUEST_TYPE  (FixedPcdGet32 (PcdOvmfWorkAreaBase))
>  

[edk2-devel] 回复: [PATCH v4 2/3] MdeModulePkg/DxeIplPeim: rename variable

[gaoliming via groups.io]

Reviewed-by: Liming Gao 

> -邮件原件-
> 发件人: Gerd Hoffmann 
> 发送时间: 2024年2月22日 18:54
> 收件人: devel@edk2.groups.io
> 抄送: Michael Roth ; Jiewen Yao
> ; Liming Gao ; Laszlo
> Ersek ; Tom Lendacky ;
> Paolo Bonzini ; Ard Biesheuvel
> ; Gerd Hoffmann ; Min Xu
> ; Erdem Aktas ; Oliver
> Steffen ; Ard Biesheuvel 
> 主题: [PATCH v4 2/3] MdeModulePkg/DxeIplPeim: rename variable
> 
> Rename Page5LevelSupported to Page5LevelEnabled.
> 
> The variable is set to true in case 5-paging level is enabled (64-bit
> PEI) or will be enabled (32-bit PEI), it does *not* tell whenever the
> 5-level paging is supported by the CPU.
> 
> Signed-off-by: Gerd Hoffmann 
> Reviewed-by: Laszlo Ersek 
> Acked-by: Ard Biesheuvel 
> ---
>  .../Core/DxeIplPeim/X64/VirtualMemory.c   | 22 +--
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index 1d240e95966e..df6196a41cd5 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -696,7 +696,7 @@ CreateIdentityMappingPageTables (
>UINTNTotalPagesNum;
>UINTNBigPageAddress;
>VOID *Hob;
> -  BOOLEAN
> Page5LevelSupport;
> +  BOOLEAN
> Page5LevelEnabled;
>BOOLEAN  Page1GSupport;
>PAGE_TABLE_1G_ENTRY
> *PageDirectory1GEntry;
>UINT64   AddressEncMask;
> @@ -744,15 +744,15 @@ CreateIdentityMappingPageTables (
>  // If cpu has already run in 64bit long mode PEI, Page table Level in
DXE
> must align with previous level.
>  //
>  Cr4.UintN = AsmReadCr4 ();
> -Page5LevelSupport = (Cr4.Bits.LA57 != 0);
> -if (Page5LevelSupport) {
> +Page5LevelEnabled = (Cr4.Bits.LA57 != 0);
> +if (Page5LevelEnabled) {
>ASSERT (PcdGetBool (PcdUse5LevelPageTable));
>  }
>} else {
>  //
>  // If cpu runs in 32bit protected mode PEI, Page table Level in DXE
is
> decided by PCD and feature capability.
>  //
> -Page5LevelSupport = FALSE;
> +Page5LevelEnabled = FALSE;
>  if (PcdGetBool (PcdUse5LevelPageTable)) {
>AsmCpuidEx (
>  CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS,
> @@ -763,12 +763,12 @@ CreateIdentityMappingPageTables (
>  NULL
>  );
>if (EcxFlags.Bits.FiveLevelPage != 0) {
> -Page5LevelSupport = TRUE;
> +Page5LevelEnabled = TRUE;
>}
>  }
>}
> 
> -  DEBUG ((DEBUG_INFO, "AddressBits=%u 5LevelPaging=%u 1GPage=%u\n",
> PhysicalAddressBits, Page5LevelSupport, Page1GSupport));
> +  DEBUG ((DEBUG_INFO, "AddressBits=%u 5LevelPaging=%u
> 1GPage=%u\n", PhysicalAddressBits, Page5LevelEnabled, Page1GSupport));
> 
>//
>// IA-32e paging translates 48-bit linear addresses to 52-bit physical
> addresses
> @@ -776,7 +776,7 @@ CreateIdentityMappingPageTables (
>//  due to either unsupported by HW, or disabled by PCD.
>//
>ASSERT (PhysicalAddressBits <= 52);
> -  if (!Page5LevelSupport && (PhysicalAddressBits > 48)) {
> +  if (!Page5LevelEnabled && (PhysicalAddressBits > 48)) {
>  PhysicalAddressBits = 48;
>}
> 
> @@ -811,7 +811,7 @@ CreateIdentityMappingPageTables (
>//
>// Substract the one page occupied by PML5 entries if 5-Level Paging is
> disabled.
>//
> -  if (!Page5LevelSupport) {
> +  if (!Page5LevelEnabled) {
>  TotalPagesNum--;
>}
> 
> @@ -831,7 +831,7 @@ CreateIdentityMappingPageTables (
>// By architecture only one PageMapLevel4 exists - so lets allocate
> storage for it.
>//
>PageMap = (VOID *)BigPageAddress;
> -  if (Page5LevelSupport) {
> +  if (Page5LevelEnabled) {
>  //
>  // By architecture only one PageMapLevel5 exists - so lets allocate
> storage for it.
>  //
> @@ -853,7 +853,7 @@ CreateIdentityMappingPageTables (
>  PageMapLevel4Entry = (VOID *)BigPageAddress;
>  BigPageAddress+= SIZE_4KB;
> 
> -if (Page5LevelSupport) {
> +if (Page5LevelEnabled) {
>//
>// Make a PML5 Entry
>//
> @@ -947,7 +947,7 @@ CreateIdentityMappingPageTables (
>  ZeroMem (PageMapLevel4Entry, (512 - IndexOfPml4Entries) * sizeof
> (PAGE_MAP_AND_DIRECTORY_POINTER));
>}
> 
> -  if (Page5LevelSupport) {
> +  if (Page5LevelEnabled) {
>  Cr4.UintN = AsmReadCr4 ();
>  Cr4.Bits.LA57 = 1;
>  AsmWriteCr4 (Cr4.UintN);
> --
> 2.43.2





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116244): https://edk2.groups.io/g/devel/message/116244
Mute This Topic: https://groups.io/mt/104662572/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] 回复: [PATCH v4 1/3] MdeModulePkg/DxeIplPeim: fix PcdUse5LevelPageTable assert

[gaoliming via groups.io]

Reviewed-by: Liming Gao 

> -邮件原件-
> 发件人: Gerd Hoffmann 
> 发送时间: 2024年2月22日 18:54
> 收件人: devel@edk2.groups.io
> 抄送: Michael Roth ; Jiewen Yao
> ; Liming Gao ; Laszlo
> Ersek ; Tom Lendacky ;
> Paolo Bonzini ; Ard Biesheuvel
> ; Gerd Hoffmann ; Min Xu
> ; Erdem Aktas ; Oliver
> Steffen ; Ard Biesheuvel 
> 主题: [PATCH v4 1/3] MdeModulePkg/DxeIplPeim: fix
> PcdUse5LevelPageTable assert
> 
> PcdUse5LevelPageTable documentation says:
> 
>   Indicates if 5-Level Paging will be enabled in long mode. 5-Level
>   Paging will not be enabled when the PCD is TRUE but CPU doesn't support
>   5-Level Paging.
> 
> So running in 4-level paging mode with PcdUse5LevelPageTable=TRUE is
> possible.  The only invalid combination is 5-level paging being active
> with PcdUse5LevelPageTable=FALSE.
> 
> Fix the ASSERT accordingly.
> 
> Signed-off-by: Gerd Hoffmann 
> Reviewed-by: Laszlo Ersek 
> Acked-by: Ard Biesheuvel 
> ---
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index 980c2002d4f5..1d240e95966e 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -745,7 +745,9 @@ CreateIdentityMappingPageTables (
>  //
>  Cr4.UintN = AsmReadCr4 ();
>  Page5LevelSupport = (Cr4.Bits.LA57 != 0);
> -ASSERT (PcdGetBool (PcdUse5LevelPageTable) == Page5LevelSupport);
> +if (Page5LevelSupport) {
> +  ASSERT (PcdGetBool (PcdUse5LevelPageTable));
> +}
>} else {
>  //
>  // If cpu runs in 32bit protected mode PEI, Page table Level in DXE
is
> decided by PCD and feature capability.
> --
> 2.43.2





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116243): https://edk2.groups.io/g/devel/message/116243
Mute This Topic: https://groups.io/mt/104662570/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile

[Laszlo Ersek]

On 3/1/24 03:54, Zhou Jianfeng wrote:
> Add volatile qualifier to page table related variable to prevent
> compiler from optimizing away the variables which may lead to
> unexpected result.
> 
> Signed-off-by: Zhou Jianfeng 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Cc: Rahul Kumar 
> Cc: Gerd Hoffmann 
> Cc: Pedro Falcato 
> Cc: Zhang Di 
> Cc: Tan Dun 
> Cc: Michael Brown 
> ---
>  .../Library/CpuPageTableLib/CpuPageTableMap.c | 36 +--
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c 
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index c4e46a6d74..0a380a04cb 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -20,17 +20,17 @@
>  **/
>  VOID
>  PageTableLibSetPte4K (
> -  IN OUT IA32_PTE_4K *Pte4K,
> -  IN UINT64  Offset,
> -  IN IA32_MAP_ATTRIBUTE  *Attribute,
> -  IN IA32_MAP_ATTRIBUTE  *Mask
> +  IN OUT volatile IA32_PTE_4K  *Pte4K,
> +  IN UINT64Offset,
> +  IN IA32_MAP_ATTRIBUTE*Attribute,
> +  IN IA32_MAP_ATTRIBUTE*Mask
>)
>  {
>IA32_PTE_4K  LocalPte4K;
> 
>LocalPte4K.Uint64 = Pte4K->Uint64;
>if (Mask->Bits.PageTableBaseAddressLow || 
> Mask->Bits.PageTableBaseAddressHigh) {
> -LocalPte4K.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS 
> (Attribute) + Offset) | (Pte4K->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);
> +LocalPte4K.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS 
> (Attribute) + Offset) | (LocalPte4K.Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);
>}
> 
>if (Mask->Bits.Present) {
> @@ -94,17 +94,17 @@ PageTableLibSetPte4K (
>  **/
>  VOID
>  PageTableLibSetPleB (
> -  IN OUT IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  *PleB,
> -  IN UINT64 Offset,
> -  IN IA32_MAP_ATTRIBUTE *Attribute,
> -  IN IA32_MAP_ATTRIBUTE *Mask
> +  IN OUT volatile IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  *PleB,
> +  IN UINT64  Offset,
> +  IN IA32_MAP_ATTRIBUTE  *Attribute,
> +  IN IA32_MAP_ATTRIBUTE  *Mask
>)
>  {
>IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;
> 
>LocalPleB.Uint64 = PleB->Uint64;
>if (Mask->Bits.PageTableBaseAddressLow || 
> Mask->Bits.PageTableBaseAddressHigh) {
> -LocalPleB.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS 
> (Attribute) + Offset) | (PleB->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
> +LocalPleB.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS 
> (Attribute) + Offset) | (LocalPleB.Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
>}
> 
>LocalPleB.Bits.MustBeOne = 1;
> @@ -171,11 +171,11 @@ PageTableLibSetPleB (
>  **/
>  VOID
>  PageTableLibSetPle (
> -  IN UINTN   Level,
> -  IN OUT IA32_PAGING_ENTRY   *Ple,
> -  IN UINT64  Offset,
> -  IN IA32_MAP_ATTRIBUTE  *Attribute,
> -  IN IA32_MAP_ATTRIBUTE  *Mask
> +  IN UINTNLevel,
> +  IN OUT volatile IA32_PAGING_ENTRY   *Ple,
> +  IN UINT64   Offset,
> +  IN IA32_MAP_ATTRIBUTE   *Attribute,
> +  IN IA32_MAP_ATTRIBUTE   *Mask
>)
>  {
>if (Level == 1) {
> @@ -195,9 +195,9 @@ PageTableLibSetPle (
>  **/
>  VOID
>  PageTableLibSetPnle (
> -  IN OUT IA32_PAGE_NON_LEAF_ENTRY  *Pnle,
> -  IN IA32_MAP_ATTRIBUTE*Attribute,
> -  IN IA32_MAP_ATTRIBUTE*Mask
> +  IN OUT volatile IA32_PAGE_NON_LEAF_ENTRY  *Pnle,
> +  IN IA32_MAP_ATTRIBUTE *Attribute,
> +  IN IA32_MAP_ATTRIBUTE *Mask
>)
>  {
>IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;

Reviewed-by: Laszlo Ersek 




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116242): https://edk2.groups.io/g/devel/message/116242
Mute This Topic: https://groups.io/mt/104661494/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 4/4] StandaloneMmPkg: Disallow unregister MMI handler in other MMI handler

[Laszlo Ersek]

On 3/1/24 04:01, Zhiguang Liu wrote:
> In last patch, we add code support to unregister MMI handler inside
> itself. However, the code doesn't support unregister MMI handler
> insider other MMI handler. While this is not a must-have usage.
> So add check to disallow unregister MMI handler in other MMI handler.
> 
> Cc: Liming Gao 
> Cc: Jiaxin Wu 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Cc: Ard Biesheuvel 
> Cc: Sami Mujawar 
> Cc: Ray Ni 
> Signed-off-by: Zhiguang Liu 
> ---
>  StandaloneMmPkg/Core/Mmi.c | 32 +++-
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/StandaloneMmPkg/Core/Mmi.c b/StandaloneMmPkg/Core/Mmi.c
> index c1a1d76e85..9e52072bf7 100644
> --- a/StandaloneMmPkg/Core/Mmi.c
> +++ b/StandaloneMmPkg/Core/Mmi.c
> @@ -36,8 +36,9 @@ typedef struct {
>MMI_ENTRY *MmiEntry;
>  } MMI_HANDLER;
>  
> -LIST_ENTRY  mRootMmiHandlerList = INITIALIZE_LIST_HEAD_VARIABLE 
> (mRootMmiHandlerList);
> -LIST_ENTRY  mMmiEntryList   = INITIALIZE_LIST_HEAD_VARIABLE 
> (mMmiEntryList);
> +LIST_ENTRY   mRootMmiHandlerList = INITIALIZE_LIST_HEAD_VARIABLE 
> (mRootMmiHandlerList);
> +LIST_ENTRY   mMmiEntryList   = INITIALIZE_LIST_HEAD_VARIABLE 
> (mMmiEntryList);
> +MMI_HANDLER  *mCurrentMmiHandler = NULL;
>  
>  /**
>Finds the MMI entry for the requested handler type.
> @@ -161,13 +162,19 @@ MmiManage (
>  // get next node before handler is executed, since LIST_ENTRY that
>  // Link points to may be freed if unregister MMI handler.
>  //
> -Link   = Link->ForwardLink;
> -Status = MmiHandler->Handler (
> -   (EFI_HANDLE)MmiHandler,
> -   Context,
> -   CommBuffer,
> -   CommBufferSize
> -   );
> +Link = Link->ForwardLink;
> +//
> +// Assign gCurrentMmiHandle before calling the MMI handler and
> +// set to NULL when it returns.
> +//
> +mCurrentMmiHandler = MmiHandler;
> +Status = MmiHandler->Handler (
> +   (EFI_HANDLE)MmiHandler,
> +   Context,
> +   CommBuffer,
> +   CommBufferSize
> +   );
> +mCurrentMmiHandler = NULL;
>  
>  switch (Status) {
>case EFI_INTERRUPT_PENDING:
> @@ -314,6 +321,13 @@ MmiHandlerUnRegister (
>  return EFI_INVALID_PARAMETER;
>}
>  
> +  //
> +  // Do not allow to unregister MMI Handler inside other MMI Handler
> +  //
> +  if ((mCurrentMmiHandler != NULL) && (mCurrentMmiHandler != MmiHandler)) {
> +return EFI_INVALID_PARAMETER;
> +  }
> +
>MmiEntry = MmiHandler->MmiEntry;
>  
>RemoveEntryList (>Link);

Reviewed-by: Laszlo Ersek 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116241): https://edk2.groups.io/g/devel/message/116241
Mute This Topic: https://groups.io/mt/104657669/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 2/4] MdeModulePkg/SMM: Disallow unregister SMI handler in other SMI handler

[Laszlo Ersek]

On 3/1/24 04:01, Zhiguang Liu wrote:
> In last patch, we add code support to unregister SMI handler inside
> itself. However, the code doesn't support unregister SMI handler
> insider other SMI handler. While this is not a must-have usage.
> So add check to disallow unregister SMI handler in other SMI handler.
> 
> Cc: Liming Gao 
> Cc: Jiaxin Wu 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Signed-off-by: Zhiguang Liu 
> ---
>  MdeModulePkg/Core/PiSmmCore/Smi.c | 29 +
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c 
> b/MdeModulePkg/Core/PiSmmCore/Smi.c
> index 3489c130fd..b3a81ac877 100644
> --- a/MdeModulePkg/Core/PiSmmCore/Smi.c
> +++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
> @@ -8,7 +8,8 @@
>  
>  #include "PiSmmCore.h"
>  
> -LIST_ENTRY  mSmiEntryList = INITIALIZE_LIST_HEAD_VARIABLE (mSmiEntryList);
> +SMI_HANDLER  *mCurrentSmiHandler = NULL;
> +LIST_ENTRY   mSmiEntryList   = INITIALIZE_LIST_HEAD_VARIABLE 
> (mSmiEntryList);
>  
>  SMI_ENTRY  mRootSmiEntry = {
>SMI_ENTRY_SIGNATURE,
> @@ -142,13 +143,18 @@ SmiManage (
>  // Link points to may be freed if unregister SMI handler.
>  //
>  Link = Link->ForwardLink;
> -
> -Status = SmiHandler->Handler (
> -   (EFI_HANDLE)SmiHandler,
> -   Context,
> -   CommBuffer,
> -   CommBufferSize
> -   );
> +//
> +// Assign gCurrentSmiHandle before calling the SMI handler and
> +// set to NULL when it returns.
> +//
> +mCurrentSmiHandler = SmiHandler;
> +Status = SmiHandler->Handler (
> +   (EFI_HANDLE)SmiHandler,
> +   Context,
> +   CommBuffer,
> +   CommBufferSize
> +   );
> +mCurrentSmiHandler = NULL;
>  
>  switch (Status) {
>case EFI_INTERRUPT_PENDING:
> @@ -328,6 +334,13 @@ SmiHandlerUnRegister (
>  return EFI_INVALID_PARAMETER;
>}
>  
> +  //
> +  // Do not allow to unregister SMI Handler inside other SMI Handler
> +  //
> +  if ((mCurrentSmiHandler != NULL) && (mCurrentSmiHandler != SmiHandler)) {
> +return EFI_INVALID_PARAMETER;
> +  }
> +
>SmiEntry = SmiHandler->SmiEntry;
>  
>RemoveEntryList (>Link);

Reviewed-by: Laszlo Ersek 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116240): https://edk2.groups.io/g/devel/message/116240
Mute This Topic: https://groups.io/mt/104657667/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension

[Laszlo Ersek]

On 3/1/24 02:29, Tuan Phan wrote:
> The GCD EFI_MEMORY_UC and EFI_MEMORY_WC memory attributes will be
> supported when Svpbmt extension available.
> 
> Cc: Gerd Hoffmann 
> Cc: Laszlo Ersek 
> Cc: Rahul Kumar 
> Cc: Ray Ni 
> Signed-off-by: Tuan Phan 
> ---
>  .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 101 +++---
>  .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf   |   1 +
>  2 files changed, 88 insertions(+), 14 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c 
> b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> index 826a1d32a1d4..f4419bb8f380 100644
> --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> @@ -36,6 +36,11 @@
>  #define PTE_PPN_SHIFT 10
>  #define RISCV_MMU_PAGE_SHIFT  12
>  
> +#define RISCV_CPU_FEATURE_PBMT_BITMASK  BIT2
> +#define PTE_PBMT_NC BIT61
> +#define PTE_PBMT_IO BIT62
> +#define PTE_PBMT_MASK   (PTE_PBMT_NC | PTE_PBMT_IO)
> +
>  STATIC UINTN  mModeSupport[] = { SATP_MODE_SV57, SATP_MODE_SV48, 
> SATP_MODE_SV39, SATP_MODE_OFF };
>  STATIC UINTN  mMaxRootTableLevel;
>  STATIC UINTN  mBitPerLevel;
> @@ -489,32 +494,89 @@ UpdateRegionMapping (
>  /**
>Convert GCD attribute to RISC-V page attribute.
>  
> -  @param  GcdAttributes The GCD attribute.
> +  @param  GcdAttributes   The GCD attribute.
> +  @param  RiscVAttribtues The pointer of RISC-V page attribute.
>  
> -  @return   The RISC-V page attribute.
> +  @retval EFI_INVALID_PARAMETER   The RiscVAttribtues is NULL or cache type 
> mask not valid.
> +  @retval EFI_SUCCESS The operation succesfully.
>  
>  **/
>  STATIC
> -UINTN
> +EFI_STATUS
>  GcdAttributeToPageAttribute (
> -  IN UINTN  GcdAttributes
> +  IN UINTN   GcdAttributes,

Just noticing: why is GcdAttributes *not* UINT64 in the first place?

All the bit macros we test against it, such as EFI_MEMORY_RO
(0x0002ULL) are of type unsigned long long (UINT64).

> +  OUT UINTN  *RiscVAttributes
>)
>  {
> -  UINTN  RiscVAttributes;
> +  UINT64   CacheTypeMask;
> +  BOOLEAN  PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) & 
> RISCV_CPU_FEATURE_PBMT_BITMASK) ? TRUE : FALSE;

- Per the edk2 coding style, locals should not be initialized (separate
assignment is needed).

- Bitmask checks always need an explicit comparison, such as

  ((a & b) != 0)

or similar. Implicitly interpreting (a & b) as a truth value is not
appropriate.

- "(whatever) ? TRUE : FALSE" is both bad style and unnecessary.

  BOOLEAN  PmbtExtEnabled;

  PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) &
RISCV_CPU_FEATURE_PBMT_BITMASK) != 0;

>  
> -  RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> +  if (!RiscVAttributes) {

- The coding style requires an explicit nullity check:

  if (RiscVAttributes == NULL) {

> +return EFI_INVALID_PARAMETER;
> +  }
> +
> +  *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
>  
>// Determine protection attributes
>if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> -RiscVAttributes &= ~(RISCV_PG_W);
> +*RiscVAttributes &= ~(RISCV_PG_W);
>}
>  
>// Process eXecute Never attribute
>if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
> -RiscVAttributes &= ~RISCV_PG_X;
> +*RiscVAttributes &= ~RISCV_PG_X;
> +  }
> +

My next comment is unrelated to the patch, it's just something that
catches my eye, and I think is worth fixing:

RISCV_PG_W is BIT2 (0x0004), and RISCV_PG_X is BIT3 (0x0008).
Meaning, they are of type *signed int* (INT32). Applying the bit-neg
operator on them produces a negative value (because it flips the sign
bit), which is very ugly.

I suggest a separate patch for changing these into

  ~(UINTN)RISCV_PG_W
  ~(UINTN)RISCV_PG_X

Alternatively, you could do

  *RiscVAttributes = RISCV_PG_R;
  if ((GcdAttributes & EFI_MEMORY_RO) == 0) {
*RiscVAttributes |= RISCV_PG_W;
  }
  if ((GcdAttributes & EFI_MEMORY_XP) == 0) {
*RiscVAttributes |= RISCV_PG_X;
  }

Either way: separate patch.

> +  CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK;
> +  if ((CacheTypeMask != 0) &&
> +  (((CacheTypeMask - 1) & CacheTypeMask) != 0))

This is not what I recommended in my previous review
.

Compare:

  (CacheTypeMask != 0) && ...

versus

  (CacheTypeMask == 0) || ...

Both of these ensure that the power-of-two check in the second
subcondition (i.e., the subtraction of 1) is avoided when CacheTypeMask
is zero. In the first variant, you get (FALSE && ...), in the second
variant, you get (TRUE || ...); therefore, the power-of-two check is
short-circuited for a zero input in both variants.

However, considering the larger CacheTypeMask validation, your variant
is incorrect, because a zero CacheTypeMask will ultimately evaluate the
condition to FALSE -- (FALSE && ...) is FALSE --, and so the "return

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

[Paolo Bonzini]

Il ven 1 mar 2024, 12:10 Michael Brown  ha scritto:

> It feels as though this should be able to be cleanly modelled with a
> single global state array
>
>BOOLEAN mSavedInterruptState[TPL_HIGH_LEVEL]
>

Pretty much, yes. But you only have to write it when the interrupts are
already disabled, so the bitmask works and makes it easier to clear "all
values at NewTpl and above" with just an AND.

>
> (or possibly a bitmask, though using the array avoids having to disable
> interrupts just to write a value).
>
> I still need to think through the subtleties, to make sure it could cope
> with pathological edge cases such as
>
>OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);
>
>...
>
>gBS->RestoreTPL (OldTpl);
>gBS->RestoreTPL (OldTpl);
>
> or
>
>OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL - 1);
>gBS->RaiseTPL (TPL_HIGH_LEVEL);
>
>..
>
>gBS->RestoreTPL (OldTpl);
>
> I think that at least one of the above pathological usage patterns would
> break the existing mInterruptedTplMask patches, since they currently
> clear state in RestoreTPL() and so will not correctly handle a duplicate
> call to RestoreTPL().
>

I think my patch works (modulo the 1 vs. 1U issue) for the second.
Declaring the first to be invalid is a good idea IMO. Also it would only
break in interrupt handlers and would revert to just causing a stack
overflow in the interrupt storm scenario, so it wouldn't be too bad...

Paolo


> I'll try to get a patch put together over the weekend.
>
> Thanks,
>
> Michael
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116238): https://edk2.groups.io/g/devel/message/116238
Mute This Topic: https://groups.io/mt/104642317/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize

[Ard Biesheuvel]

Hi Oliver,

On Tue, 27 Feb 2024 at 21:27, Oliver Smith-Denny
 wrote:
>
> When an ImageRecord is stored by ImagePropertiesRecordLib, it reports the
> CodeSegmentSize as the SizeOfRawData from the image. However, the image
> as loaded into memory is aligned to the SectionAlignment, so
> SizeOfRawData is under the actual size in memory. This is important,
> because the memory attributes table uses these image records to create
> its entries and it will report that the alignment of an image is
> incorrect, even though the actual image is correct.
>
> This was discovered on ARM64, which has a 64k runtime page granularity
> alignment, which is backed by a 64k section alignment for
> DXE_RUNTIME_DRIVERs. The runtime code and data was correctly being
> loaded into memory, however the memory attribute table was incorrectly
> reporting misaligned ranges to the OS, causing attributes to be
> ignored for these sections for OSes using greater than 4k pages.
>
> This patch correctly aligns the CodeSegmentSize to the SectionAlignment
> and the corresponding memory attribute table entries are now correctly
> aligned and pointing to the right places in memory.
>

Can you explain how these can differ in the first place? Our flaky
ELF-to-PE/COFF converter should never generate such images to begin
with (which is probably how we ended up with this problem in the first
place), so I  suppose this is native PE/COFF tooling emitting sections
either using a non-1:1 file:memory mapping, or with unallocated holes
in the file representation?

> Cc: Liming Gao 
> Cc: Leif Lindholm 
> Cc: Ard Biesheuvel 
> Cc: Sami Mujawar 
> Cc: Taylor Beebe 
>
> Signed-off-by: Oliver Smith-Denny 
> ---
>  MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 2 
> +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git 
> a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c 
> b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
> index e53ce086c54c..07ced0e54e38 100644
> --- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
> +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
> @@ -1090,7 +1090,7 @@ CreateImagePropertiesRecord (
>ImageRecordCodeSection->Signature = 
> IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE;
>
>ImageRecordCodeSection->CodeSegmentBase = (UINTN)ImageBase + 
> Section[Index].VirtualAddress;
> -  ImageRecordCodeSection->CodeSegmentSize = Section[Index].SizeOfRawData;
> +  ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE 
> (Section[Index].SizeOfRawData, SectionAlignment);
>

This should be the virtual size, not the file size, right?

>InsertTailList (>CodeSegmentList, 
> >Link);
>ImageRecord->CodeSegmentCount++;
> --
> 2.40.1
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116237): https://edk2.groups.io/g/devel/message/116237
Mute This Topic: https://groups.io/mt/104610770/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile

[Michael Brown]

Reviewed-by: Michael Brown 

Thanks,

Michael

On 01/03/2024 02:54, Zhou Jianfeng wrote:

Add volatile qualifier to page table related variable to prevent
compiler from optimizing away the variables which may lead to
unexpected result.

Signed-off-by: Zhou Jianfeng 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
Cc: Pedro Falcato 
Cc: Zhang Di 
Cc: Tan Dun 
Cc: Michael Brown 
---
  .../Library/CpuPageTableLib/CpuPageTableMap.c | 36 +--
  1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c 
b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index c4e46a6d74..0a380a04cb 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -20,17 +20,17 @@
  **/
  VOID
  PageTableLibSetPte4K (
-  IN OUT IA32_PTE_4K *Pte4K,
-  IN UINT64  Offset,
-  IN IA32_MAP_ATTRIBUTE  *Attribute,
-  IN IA32_MAP_ATTRIBUTE  *Mask
+  IN OUT volatile IA32_PTE_4K  *Pte4K,
+  IN UINT64Offset,
+  IN IA32_MAP_ATTRIBUTE*Attribute,
+  IN IA32_MAP_ATTRIBUTE*Mask
)
  {
IA32_PTE_4K  LocalPte4K;

LocalPte4K.Uint64 = Pte4K->Uint64;
if (Mask->Bits.PageTableBaseAddressLow || 
Mask->Bits.PageTableBaseAddressHigh) {
-LocalPte4K.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + 
Offset) | (Pte4K->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);
+LocalPte4K.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + 
Offset) | (LocalPte4K.Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);
}

if (Mask->Bits.Present) {
@@ -94,17 +94,17 @@ PageTableLibSetPte4K (
  **/
  VOID
  PageTableLibSetPleB (
-  IN OUT IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  *PleB,
-  IN UINT64 Offset,
-  IN IA32_MAP_ATTRIBUTE *Attribute,
-  IN IA32_MAP_ATTRIBUTE *Mask
+  IN OUT volatile IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  *PleB,
+  IN UINT64  Offset,
+  IN IA32_MAP_ATTRIBUTE  *Attribute,
+  IN IA32_MAP_ATTRIBUTE  *Mask
)
  {
IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;

LocalPleB.Uint64 = PleB->Uint64;
if (Mask->Bits.PageTableBaseAddressLow || 
Mask->Bits.PageTableBaseAddressHigh) {
-LocalPleB.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + 
Offset) | (PleB->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
+LocalPleB.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + 
Offset) | (LocalPleB.Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
}

LocalPleB.Bits.MustBeOne = 1;
@@ -171,11 +171,11 @@ PageTableLibSetPleB (
  **/
  VOID
  PageTableLibSetPle (
-  IN UINTN   Level,
-  IN OUT IA32_PAGING_ENTRY   *Ple,
-  IN UINT64  Offset,
-  IN IA32_MAP_ATTRIBUTE  *Attribute,
-  IN IA32_MAP_ATTRIBUTE  *Mask
+  IN UINTNLevel,
+  IN OUT volatile IA32_PAGING_ENTRY   *Ple,
+  IN UINT64   Offset,
+  IN IA32_MAP_ATTRIBUTE   *Attribute,
+  IN IA32_MAP_ATTRIBUTE   *Mask
)
  {
if (Level == 1) {
@@ -195,9 +195,9 @@ PageTableLibSetPle (
  **/
  VOID
  PageTableLibSetPnle (
-  IN OUT IA32_PAGE_NON_LEAF_ENTRY  *Pnle,
-  IN IA32_MAP_ATTRIBUTE*Attribute,
-  IN IA32_MAP_ATTRIBUTE*Mask
+  IN OUT volatile IA32_PAGE_NON_LEAF_ENTRY  *Pnle,
+  IN IA32_MAP_ATTRIBUTE *Attribute,
+  IN IA32_MAP_ATTRIBUTE *Mask
)
  {
IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;
--
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116230): https://edk2.groups.io/g/devel/message/116230
Mute This Topic: https://groups.io/mt/104661494/1770615
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [mc...@ipxe.org]
-=-=-=-=-=-=-=-=-=-=-






-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116236): https://edk2.groups.io/g/devel/message/116236
Mute This Topic: https://groups.io/mt/104661494/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 1/1] ArmPkg,MdePkg: move ArmLib.h to MdePkg

[Leif Lindholm]

Thank you.

OK, that's logically consistent.
So we'd need an ArmLibNull in MdePkg until ArmLib itself migrates there 
(ideally subsumed into BaseLib).


But the dependency in .inf should still be able to be declared under
[LibraryClasses.AArch64, LibraryClasses.ARM]?

Regards,

Leif

On 2024-03-01 01:00, Yao, Jiewen wrote:

Sure.

When we say "dependency", what we really mean is the dependency in INF file, not 
"dependency" in DSC file.

 From package release perspective, only INF is the interface to other package.
The DSC is only the package internal stuff, you can create multiple DSCs or 
add/remove DSC freely.

Having "dependency" in DSC does not matter.
Having dependency in INF is something we should care about.

Thank you
Yao, Jiewen



-Original Message-
From: Leif Lindholm 
Sent: Tuesday, February 13, 2024 1:38 AM
To: Pierre Gondois ; devel@edk2.groups.io; Yao,
Jiewen 
Cc: Ard Biesheuvel ; Liming Gao
; Kinney, Michael D ;
Sami Mujawar ; Liu, Zhiguang

Subject: Re: [RFC PATCH 1/1] ArmPkg,MdePkg: move ArmLib.h to MdePkg

Jiewen, can you clarify what you said back in
https://edk2.groups.io/g/devel/message/111551
?

On 2024-02-12 17:24, Pierre Gondois wrote:

A ArmLibNull.inf library might also need to be created. If the
OpensslLibFullAccel.inf module will depend on the ArmLib library,
a Null implementation will be necessary for non-Arm architectures.


Can ArmLib be declared under a [LibraryClasses.AArch64,
LibraryClasses.ARM]? Have I forgotten something that we discussed back
in ... November?


  From [1], it seems the MdePkg/CryptoPkg should build without a dependency
on the ArmPkg. This is currently not really the case. cf. [2].

However, having a ArmLibNull implementation in the MdePkg would allow to
avoid going in this direction when providing libraries to CryptoPkg.dsc.

(Just in case, I think this ArmLibNull is a minor point.)


Well, sure, it is now.
Until we need a RiscV64LibNull, LoongarchLibNull, ...


[1] https://edk2.groups.io/g/devel/message/111545
[2]


https://github.com/tianocore/edk2/blob/8801c75b4d77c2e6e06b3ddc8560e0db
590f6342/CryptoPkg/CryptoPkg.dsc#L117





Otherwise I could apply and run the CryptoPkg/Arm native instructions
patchset on top of this patch.

---

As a side note, it also seems that:
- ArmPkg/Include/Chipset/ArmCortexA5x.h
     isn't used anymore in edk2/edk2-platorms
- ArmPkg/Include/Chipset/ArmCortexA9.h
     is barely used in edk2-platforms.
Maybe the files should have been removed/simplified as part of
- cffa7925a293 ("ArmPkg: remove ArmCpuLib header and implementations")
- a913ad02479d ("ArmPlatformPkg: remove ArmVExpressPkg")


I think you're right.
Well, ArmCortexA9.h is still *used*, but I can't imagine the Arm branch
of ArmVExpressLib has been build by anyone for some time. And surely the
inclusion of ArmVExpressLibSec in ArmVExpress-FVP-AArch64.dsc is
superfluous (such that that .inf can be deleted)?


The file could just be moved in the Library. I assume you/Sami/Ard
will know more on the usage of the library itself,


Sami?

/
  Leif







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116235): https://edk2.groups.io/g/devel/message/116235
Mute This Topic: https://groups.io/mt/102731845/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [v2] BaseTools/AutoGen: declare ProcessLibraryConstructorList() for SEC modules

[Laszlo Ersek]

Hi Mike,

can you please comment on the Build and Inf spec changes proposed in
this thread?

Thanks,
Laszlo

On 2/24/24 21:59, Laszlo Ersek wrote:
> v1 posting:
> 
>   https://edk2.groups.io/g/devel/message/115193
>   msgid <36593e23-d3e8-b71a-808d-ef94260b5...@redhat.com>
> 
> Bugzilla:
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=991
> 
> In version 2, the feature is structured differently. Following Mike's
> advice, for compatibility, the ProcessLibraryConstructorList()
> declaration in AutoGen.h is now gated on the SEC module having
> INF_VERSION >= 1.30.
> 
> Accordingly,
> 
> - I now update the Build specification and the Inf specification (see
>   patch sets posted in response to this email),
> 
> - edk2 only receives a single patch (for AutoGen), for the time being,
> 
> - the same edk2 patch is being ported to edk2-basetools:
>   https://github.com/tianocore/edk2-basetools/pull/120.
> 
> Next steps: once all of the above is merged, *and* an edk2-basetools
> release has been tagged and published, I'll rework the C code patches
> for edk2 and edk2-platforms, from the v1 patch sets, as follows:
> 
> - all those SEC modules will have to see their INF_VERSIONs bumped to
>   1.30, for triggering the new code generation,
> 
> - pip-requirements.txt/edk2-basetools will need to reference the new
>   edk2-basetools release, for exposing the feature in the first place.
> 
> Laszlo
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116234): https://edk2.groups.io/g/devel/message/116234
Mute This Topic: https://groups.io/mt/104553597/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v8 14/37] UefiCpuPkg: Add CpuMmuLib to UefiCpuPkg

[Laszlo Ersek]

On 3/1/24 02:26, Chao Li wrote:
> Hi Ray, Lazslo,
> 
> This library is almost complete to refactored, it refer to ARM and
> RISC-V version, the API include set/get memory region attribute.
> 
> I have one last question, in ARM and RISC-V version, even LoongArch old
> and current version, they all request a configure interface, which may
> be called in PEI or DXE stage, so should we open configure API? If so,
> it is possible for ARM RISC-V and LongArch's MMU libraries to be meged
> into the same instance if they wished.

Sorry, I've lost all context by now. I don't understand what you mean by
"they all request a configure interface".

Please provide pathnames and function names for the existent code that
you are referring to as examples (and then please restate / rephrase
your question as well).

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116233): https://edk2.groups.io/g/devel/message/116233
Mute This Topic: https://groups.io/mt/103971653/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

[Michael Brown]

On 01/03/2024 09:33, Paolo Bonzini wrote:

On Fri, Mar 1, 2024 at 10:27 AM Michael Brown  wrote:

It's possible that it doesn't matter.  The new logic will effectively
mean that RestoreTPL() will restore not only the TPL but also the
interrupts-enabled state to whatever existed at the time of the
corresponding RaiseTPL().


Right: that's what my comment says

+  // However, when the handler calls RestoreTPL
+  // before returning, we want to keep interrupts disabled.  This
+  // restores the exact state at the beginning of the handler,
+  // before the call to RaiseTPL(): low TPL and interrupts disabled.

but indeed it applies beyond interrupt handlers. It might even be a bugfix.


Right.  I'm leaning towards treating this as a bugfix: essentially 
tightening up the semantics of RestoreTPL() to mean:


- any callbacks in the range OldTpl < Tpl < gEfiCurrentTpl will be 
dispatched with interrupts unconditionally enabled


- the TPL will be restored to OldTpl

- the interrupt state will be restored to the value it had when the TPL 
was last raised from OldTpl


It feels as though this should be able to be cleanly modelled with a 
single global state array


  BOOLEAN mSavedInterruptState[TPL_HIGH_LEVEL]

(or possibly a bitmask, though using the array avoids having to disable 
interrupts just to write a value).


I still need to think through the subtleties, to make sure it could cope 
with pathological edge cases such as


  OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);

  ...

  gBS->RestoreTPL (OldTpl);
  gBS->RestoreTPL (OldTpl);

or

  OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL - 1);
  gBS->RaiseTPL (TPL_HIGH_LEVEL);

  ..

  gBS->RestoreTPL (OldTpl);

I think that at least one of the above pathological usage patterns would 
break the existing mInterruptedTplMask patches, since they currently 
clear state in RestoreTPL() and so will not correctly handle a duplicate 
call to RestoreTPL().


I'll try to get a patch put together over the weekend.

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116232): https://edk2.groups.io/g/devel/message/116232
Mute This Topic: https://groups.io/mt/104642317/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] BaseTools: PcdValueInit enhancement

[Laszlo Ersek]

On 3/1/24 03:10, Yuting Yang wrote:
> Enhance PcdValueInit for storage saving
> 
> Cc: Rebecca Cran rebe...@bsdio.com
> Cc: Liming Gao gaolim...@byosoft.com.cn
> Cc: Bob Feng bob.c.f...@intel.com
> Signed-off-by: Yuting Yang 
> ---
>  .../Source/Python/Workspace/DscBuildData.py   | 43 ++-
>  1 file changed, 33 insertions(+), 10 deletions(-)

This patch looks like line noise.

I don't intend to review it in depth, but the patch is unsuitable for a
cursory read-through.

The original code is unreadable to begin with; it seems that at least
one removed line is 410 characters long.

Please first refactor/reformat the original code so it becomes readable.
Then in a second patch, implement the change, but also *document* in the
commit message what you are trying to do, why, and (at a high level),
how. The current commit message is worthless.

Laszlo

> 
> diff --git a/BaseTools/Source/Python/Workspace/DscBuildData.py 
> b/BaseTools/Source/Python/Workspace/DscBuildData.py
> index 5df184f9c8..e83188f4b3 100644
> --- a/BaseTools/Source/Python/Workspace/DscBuildData.py
> +++ b/BaseTools/Source/Python/Workspace/DscBuildData.py
> @@ -1820,6 +1820,7 @@ class DscBuildData(PlatformBuildClassObject):
>  def GenerateSizeFunction(self, Pcd):
>  CApp = "// Default Value in Dec \n"
>  CApp = CApp + "void Cal_%s_%s_Size(UINT32 *Size){\n" % 
> (Pcd.TokenSpaceGuidCName, Pcd.TokenCName)
> +CAppList = []
>  
>  if Pcd.IsArray() and Pcd.Capacity[-1] != "-1":
>  CApp += "  *Size = (sizeof (%s) > *Size ? sizeof (%s) : 
> *Size);\n" % (Pcd.DatumType,Pcd.DatumType)
> @@ -1869,7 +1870,8 @@ class DscBuildData(PlatformBuildClassObject):
>  (".".join((Pcd.TokenSpaceGuidCName, 
> Pcd.TokenCName, FieldName.strip('.'))), FieldList[FieldName.strip(".")][1], 
> FieldList[FieldName.strip(".")][2]))
>  Value, ValueSize = ParseFieldValue(Value)
>  if not Pcd.IsArray():
> -CApp = CApp + '  __FLEXIBLE_SIZE(*Size, %s, %s, %d / 
> __ARRAY_ELEMENT_SIZE(%s, %s) + ((%d %% __ARRAY_ELEMENT_SIZE(%s, %s)) ? 1 : 
> 0));  // From %s Line %d Value %s \n' % (Pcd.DatumType, FieldName.strip("."), 
> ValueSize, Pcd.DatumType, FieldName.strip("."), ValueSize, Pcd.DatumType, 
> FieldName.strip("."), FieldList[FieldName.strip(".")][1], 
> FieldList[FieldName.strip(".")][2], FieldList[FieldName.strip(".")][0]);
> +CAppInfo = '  __FLEXIBLE_SIZE(*Size, %s, %s, %d / 
> __ARRAY_ELEMENT_SIZE(%s, %s) + ((%d %% __ARRAY_ELEMENT_SIZE(%s, %s)) ? 1 : 
> 0));  // From %s Line %d Value %s \n' % (Pcd.DatumType, FieldName.strip("."), 
> ValueSize, Pcd.DatumType, FieldName.strip("."), ValueSize, Pcd.DatumType, 
> FieldName.strip("."), FieldList[FieldName.strip(".")][1], 
> FieldList[FieldName.strip(".")][2], FieldList[FieldName.strip(".")][0]);
> +CAppList.append(CAppInfo.split(' //')[0])
>  else:
>  NewFieldName = ''
>  FieldName_ori = FieldName.strip('.')
> @@ -1880,7 +1882,8 @@ class DscBuildData(PlatformBuildClassObject):
>  FieldName = NewFieldName + FieldName
>  while '[' in FieldName and not Pcd.IsArray():
>  FieldName = FieldName.rsplit('[', 1)[0]
> -CApp = CApp + '  __FLEXIBLE_SIZE(*Size, %s, %s, %d); 
> // From %s Line %d Value %s\n' % (Pcd.DatumType, FieldName.strip("."), 
> Array_Index + 1, FieldList[FieldName_ori][1], FieldList[FieldName_ori][2], 
> FieldList[FieldName_ori][0])
> +CAppInfo = '  __FLEXIBLE_SIZE(*Size, %s, %s, %d); // 
> From %s Line %d Value %s\n' % (Pcd.DatumType, FieldName.strip("."), 
> Array_Index + 1, FieldList[FieldName_ori][1], FieldList[FieldName_ori][2], 
> FieldList[FieldName_ori][0])
> +CAppList.append(CAppInfo.split(' //')[0])
>  flexisbale_size_statement_cache = set()
>  for skuname in Pcd.SkuOverrideValues:
>  if skuname == TAB_COMMON:
> @@ -1908,7 +1911,10 @@ class DscBuildData(PlatformBuildClassObject):
>  
> (".".join((Pcd.TokenSpaceGuidCName, Pcd.TokenCName, FieldName.strip('.'))), 
> FieldList[FieldName.strip(".")][1], FieldList[FieldName.strip(".")][2]))
>  Value, ValueSize = ParseFieldValue(Value)
>  if not Pcd.IsArray():
> -CApp = CApp + '  __FLEXIBLE_SIZE(*Size, 
> %s, %s, %d / __ARRAY_ELEMENT_SIZE(%s, %s) + ((%d %% __ARRAY_ELEMENT_SIZE(%s, 
> %s)) ? 1 : 0)); // From %s Line %d Value %s\n' % (Pcd.DatumType, 
> FieldName.strip("."), ValueSize, Pcd.DatumType, FieldName.strip("."), 
> ValueSize, Pcd.DatumType, FieldName.strip("."), 
> FieldList[FieldName.strip(".")][1], FieldList[FieldName.strip(".")][2], 
> FieldList[FieldName.strip(".")][0]);
> +

[edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile

[Zhou Jianfeng]

Add volatile qualifier to page table related variable to prevent
compiler from optimizing away the variables which may lead to
unexpected result.

Signed-off-by: Zhou Jianfeng 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
Cc: Pedro Falcato 
Cc: Zhang Di 
Cc: Tan Dun 
Cc: Michael Brown 
---
 .../Library/CpuPageTableLib/CpuPageTableMap.c | 36 +--
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c 
b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index c4e46a6d74..0a380a04cb 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -20,17 +20,17 @@
 **/
 VOID
 PageTableLibSetPte4K (
-  IN OUT IA32_PTE_4K *Pte4K,
-  IN UINT64  Offset,
-  IN IA32_MAP_ATTRIBUTE  *Attribute,
-  IN IA32_MAP_ATTRIBUTE  *Mask
+  IN OUT volatile IA32_PTE_4K  *Pte4K,
+  IN UINT64Offset,
+  IN IA32_MAP_ATTRIBUTE*Attribute,
+  IN IA32_MAP_ATTRIBUTE*Mask
   )
 {
   IA32_PTE_4K  LocalPte4K;

   LocalPte4K.Uint64 = Pte4K->Uint64;
   if (Mask->Bits.PageTableBaseAddressLow || 
Mask->Bits.PageTableBaseAddressHigh) {
-LocalPte4K.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS 
(Attribute) + Offset) | (Pte4K->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);
+LocalPte4K.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS 
(Attribute) + Offset) | (LocalPte4K.Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);
   }

   if (Mask->Bits.Present) {
@@ -94,17 +94,17 @@ PageTableLibSetPte4K (
 **/
 VOID
 PageTableLibSetPleB (
-  IN OUT IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  *PleB,
-  IN UINT64 Offset,
-  IN IA32_MAP_ATTRIBUTE *Attribute,
-  IN IA32_MAP_ATTRIBUTE *Mask
+  IN OUT volatile IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  *PleB,
+  IN UINT64  Offset,
+  IN IA32_MAP_ATTRIBUTE  *Attribute,
+  IN IA32_MAP_ATTRIBUTE  *Mask
   )
 {
   IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;

   LocalPleB.Uint64 = PleB->Uint64;
   if (Mask->Bits.PageTableBaseAddressLow || 
Mask->Bits.PageTableBaseAddressHigh) {
-LocalPleB.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) 
+ Offset) | (PleB->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
+LocalPleB.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) 
+ Offset) | (LocalPleB.Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
   }

   LocalPleB.Bits.MustBeOne = 1;
@@ -171,11 +171,11 @@ PageTableLibSetPleB (
 **/
 VOID
 PageTableLibSetPle (
-  IN UINTN   Level,
-  IN OUT IA32_PAGING_ENTRY   *Ple,
-  IN UINT64  Offset,
-  IN IA32_MAP_ATTRIBUTE  *Attribute,
-  IN IA32_MAP_ATTRIBUTE  *Mask
+  IN UINTNLevel,
+  IN OUT volatile IA32_PAGING_ENTRY   *Ple,
+  IN UINT64   Offset,
+  IN IA32_MAP_ATTRIBUTE   *Attribute,
+  IN IA32_MAP_ATTRIBUTE   *Mask
   )
 {
   if (Level == 1) {
@@ -195,9 +195,9 @@ PageTableLibSetPle (
 **/
 VOID
 PageTableLibSetPnle (
-  IN OUT IA32_PAGE_NON_LEAF_ENTRY  *Pnle,
-  IN IA32_MAP_ATTRIBUTE*Attribute,
-  IN IA32_MAP_ATTRIBUTE*Mask
+  IN OUT volatile IA32_PAGE_NON_LEAF_ENTRY  *Pnle,
+  IN IA32_MAP_ATTRIBUTE *Attribute,
+  IN IA32_MAP_ATTRIBUTE *Mask
   )
 {
   IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;
--
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116230): https://edk2.groups.io/g/devel/message/116230
Mute This Topic: https://groups.io/mt/104661494/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 14/23] Ovmfpkg/CcSvsmLib: Create CcSvsmLib to handle SVSM related services

[Gerd Hoffmann]

On Wed, Feb 28, 2024 at 09:51:41AM -0600, Tom Lendacky wrote:
> On 2/28/24 02:40, Gerd Hoffmann wrote:
> > > +/**
> > > +  Perform a native PVALIDATE operation for the page ranges specified.
> > > +
> > > +  Validate or rescind the validation of the specified pages.
> > > +
> > > +  @param[in]   Info   Pointer to a page state change 
> > > structure
> > > +
> > > +**/
> > > +STATIC
> > > +VOID
> > > +BasePvalidate (
> > > +  IN  SNP_PAGE_STATE_CHANGE_INFO  *Info
> > > +  )
> > 
> > This is not mentioned in the commit message.
> > 
> > Looks like you are moving or copying code from BaseMemEncryptSevLib.
> > 
> > Moving code is best done with a patch doing the move only, without other
> > functional changes.  If that can't be done easily this should explained
> > in the commit message.
> 
> I can leave this as unsupported in this patch and then when switching over
> to using the functions in patch #16, move the code at that time.
> 
> For the VMSA update, that isn't as easy because of the interaction between
> UefiCpuPkg (MpInitLib) and OvmfPkg and requires two separate patches, which
> would cause bisection breakage.

Oh, right, cross-package code move isn't going to work.

> Or I could keep this all here and expand the commit message to indicate that
> the base support is being implemented based off of the existing support.

I think adding clear annotations to the commit messages is fine then.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116229): https://edk2.groups.io/g/devel/message/116229
Mute This Topic: https://groups.io/mt/104512963/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

[Paolo Bonzini]

On Fri, Mar 1, 2024 at 10:27 AM Michael Brown  wrote:
> > My version is attached, feel free to reuse it (either entirely or
> > partially) for a hypothetical v2. Apologies to you and Mike K for the
> > confusion!
>
> I much prefer this version of the patch, because the explanations are
> easier to follow and to reason about.

Thanks! I find the new logic to be quite appealing... now that I
understand it. Hopefully this provides a way to find the best of both
worlds.

> There is an implicit assumption that if interrupts are disabled when
> RaiseTPL() is called then we must have been called from an interrupt
> handler.  How sure are we that this assumption is correct?
>
> It's possible that it doesn't matter.  The new logic will effectively
> mean that RestoreTPL() will restore not only the TPL but also the
> interrupts-enabled state to whatever existed at the time of the
> corresponding RaiseTPL().

Right: that's what my comment says

+  // However, when the handler calls RestoreTPL
+  // before returning, we want to keep interrupts disabled.  This
+  // restores the exact state at the beginning of the handler,
+  // before the call to RaiseTPL(): low TPL and interrupts disabled.

but indeed it applies beyond interrupt handlers. It might even be a bugfix.

> Maybe this is what we want?  If so, then we
> should probably phrase the comments in these terms instead of in terms
> of being called from an interrupt handler.

I think phrasing the comments with reference to interrupt handlers is
fine, but it may be worth adding a comment to either
mInterruptedTplMask or CoreRestoreTpl(), like

+/// NOTE: Strictly speaking, this applies to anything that
+/// calls RaiseTPL() with interrupts disabled, not just
+/// interrupt handlers.  Interrupt handlers are just the case
+/// that we care the most about, because of the potential
+/// for stack overflow.

Paolo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116228): https://edk2.groups.io/g/devel/message/116228
Mute This Topic: https://groups.io/mt/104642317/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

[Michael Brown]

On 01/03/2024 08:37, Paolo Bonzini wrote:

So I am starting to see more and more similarities between the two
approaches.  I went a step further with fresh mind, removing the while
loop... and basically reinvented your and Michael's patch. :) The only
difference in the logic is a slightly different handling of
mInterruptedTplMask in CoreRestoreTpl(), which is a bit safer in my
case.

However, my roundabout way of getting to the same patch resulted in
very different comments. Personally, I found the large text at the
head of mInterruptedTplMask a bit too much, and the ones inside the
function too focused on "how" and not "why". Maybe it's my exposure to
NestedInterruptTplLib, but I find that a much smaller text can achieve
the same purpose, by explaining the logic instead of the individual
steps.

My version is attached, feel free to reuse it (either entirely or
partially) for a hypothetical v2. Apologies to you and Mike K for the
confusion!


I much prefer this version of the patch, because the explanations are 
easier to follow and to reason about.



Minor point (applies to both your and Ray's versions):

- The use of gCpu->GetInterruptState() vs CoreSetInterruptState() is 
inconsistent.  It feels as though CoreGetInterruptState() ought to 
exist.  I assume that the PI spec defines whether interrupts are enabled 
or disabled prior to the CPU arch protocol being installed, so it should 
be possible to have a well-defined return value from 
CoreGetInterruptState().



Major point:

There is an implicit assumption that if interrupts are disabled when 
RaiseTPL() is called then we must have been called from an interrupt 
handler.  How sure are we that this assumption is correct?


It's possible that it doesn't matter.  The new logic will effectively 
mean that RestoreTPL() will restore not only the TPL but also the 
interrupts-enabled state to whatever existed at the time of the 
corresponding RaiseTPL().  Maybe this is what we want?  If so, then we 
should probably phrase the comments in these terms instead of in terms 
of being called from an interrupt handler.



Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116227): https://edk2.groups.io/g/devel/message/116227
Mute This Topic: https://groups.io/mt/104642317/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

[Ni, Ray]

Paolo,
Happy weekends!
Thanks! I will read it on my next Monday.

Thanks,
Ray
> -Original Message-
> From: Paolo Bonzini 
> Sent: Friday, March 1, 2024 4:44 PM
> To: Ni, Ray 
> Cc: devel@edk2.groups.io; Kinney, Michael D ;
> Liming Gao ; Laszlo Ersek ;
> Michael Brown 
> Subject: Re: [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue
> due to nested interrupts
> 
> One fix is needed in the code.
> 
> On Thu, Feb 29, 2024 at 2:04 PM Ray Ni  wrote:
> > +  //
> > +  // Save the "Interrupted TPL" (TPL that was interrupted).
> > +  //
> > +  mInterruptedTplMask |= (UINTN)(1 << gEfiCurrentTpl);
> > +}
> >}
> 
> > +  //
> > +  // Clear interrupted TPL level mask, but do not re-enable interrupts
> here
> > +  // This will return to CoreTimerTick() and interrupts will be
> re-enabled
> > +  // when the timer interrupt handlers returns from interrupt
> context.
> > +  //
> > +  ASSERT ((INTN)gEfiCurrentTpl == HighBitSet64
> (mInterruptedTplMask));
> > +  mInterruptedTplMask &= ~(UINTN)(1 << gEfiCurrentTpl);
> > +}
> >}
> 
> Both of these need to use "1U" to avoid sign extending bit 31 into bits 
> 31..63.
> 
> The same issue is (in three places) present in my own version of the patch. :(
> 
> Paolo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116226): https://edk2.groups.io/g/devel/message/116226
Mute This Topic: https://groups.io/mt/104642317/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

[Paolo Bonzini]

One fix is needed in the code.

On Thu, Feb 29, 2024 at 2:04 PM Ray Ni  wrote:
> +  //
> +  // Save the "Interrupted TPL" (TPL that was interrupted).
> +  //
> +  mInterruptedTplMask |= (UINTN)(1 << gEfiCurrentTpl);
> +}
>}

> +  //
> +  // Clear interrupted TPL level mask, but do not re-enable interrupts 
> here
> +  // This will return to CoreTimerTick() and interrupts will be 
> re-enabled
> +  // when the timer interrupt handlers returns from interrupt context.
> +  //
> +  ASSERT ((INTN)gEfiCurrentTpl == HighBitSet64 (mInterruptedTplMask));
> +  mInterruptedTplMask &= ~(UINTN)(1 << gEfiCurrentTpl);
> +}
>}

Both of these need to use "1U" to avoid sign extending bit 31 into bits 31..63.

The same issue is (in three places) present in my own version of the patch. :(

Paolo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116225): https://edk2.groups.io/g/devel/message/116225
Mute This Topic: https://groups.io/mt/104642317/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

[Paolo Bonzini]

On Fri, Mar 1, 2024 at 4:08 AM Ni, Ray  wrote:
> @@ -161,5 +191,46 @@ CoreRestoreTpl (
>IN EFI_TPL NewTpl
>)
>  {
> +  BOOLEAN InInterruptHandler = FALSE;
> +
> +  //
> +  // Unwind the nested interrupt handlers up to the required
> +  // TPL, paying attention not to overflow the stack.  While
> +  // not strictly necessary according to the specification,
> +  // accept the possibility that multiple RaiseTPL calls are
> +  // undone by a single RestoreTPL
> +  //
> +  while ((INTN)NewTpl <= HighBitSet64 (mInterruptedTplMask)) {
> 1. why "<="? I thought when RestoreTPL() is called there are only two cases:
>a. NewTpl == HighBitSet64 (...)
>b. NewTpl > HighBitSet64 (...)
>   1.a is the case when TimerInterruptHandler() or CoreTimerTick() restores
>   TPL from HIGH to non-HIGH.
>   1.b is the case when the pending event backs call RaiseTPL/RestoreTPL().
>   Because only pending events whose TPL > "Interrupted TPL" can run, the
>   RestoreTPL() call from the event callbacks cannot change the TPL to a value
>   less than or equal to "Interrupted TPL".
>   So, I think "<=" can be "==".
>
> 2. can you explain a bit more about the reason of "while"?

Both are just for extra safety. The required invariant is that all
bits at or below current TPL are cleared, and using "while (... <=
...)" makes it more robust to incorrect usage of gBS->RestoreTPL().

Indeed, the patch at the top of thread also uses "(INTN)gEfiCurrentTpl
> HighBitSet64 (mInterruptedTplMask)", which is <= when you reverse
the condition.  It then asserts inside the conditional that "==" would
be enough.

So I am starting to see more and more similarities between the two
approaches.  I went a step further with fresh mind, removing the while
loop... and basically reinvented your and Michael's patch. :) The only
difference in the logic is a slightly different handling of
mInterruptedTplMask in CoreRestoreTpl(), which is a bit safer in my
case.

However, my roundabout way of getting to the same patch resulted in
very different comments. Personally, I found the large text at the
head of mInterruptedTplMask a bit too much, and the ones inside the
function too focused on "how" and not "why". Maybe it's my exposure to
NestedInterruptTplLib, but I find that a much smaller text can achieve
the same purpose, by explaining the logic instead of the individual
steps.

My version is attached, feel free to reuse it (either entirely or
partially) for a hypothetical v2. Apologies to you and Mike K for the
confusion!

> +
> +if (InterruptedTpl == NewTpl) {
> +  break;
> 3. "break" or "return"? I think we should exit from this function.

Indeed, this should have been a return.

Paolo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116224): https://edk2.groups.io/g/devel/message/116224
Mute This Topic: https://groups.io/mt/104642317/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


From b9f0bc3ef83b40c29e093acda1d0741b8f5610e5 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini 
Date: Fri, 1 Mar 2024 09:11:48 +0100
Subject: [PATCH] MdeModulePkg: fix stack overflow issue due to nested
 interrupts
Content-Type: text/plain; charset=UTF-8

This is a heavily simplified version of the fix in the OvmfPkg
NestedInterruptTplLib.  Putting it in DXE core allows CoreRestoreTpl()
to lower the TPL while keeping interrupts disabled, removing the need
for either DisableInterruptsOnIret() or the complex deferred execution
mechanism.

Instead, CoreRaiseTpl() uses the current state of the interrupt flag to
second guess whether it's being called from an interrupt handler; when
restoring the outer TPL at the end of the handler, interrupts remain
disabled until IRET.  This eliminates the possibility that a nested
invocation of the interrupt handler has the same TPL as the outer one.

Signed-off-by: Michael D Kinney 
Signed-off-by: Ray Ni 
[Rewrote the CoreRestoreTpl part and the commit message. - Paolo]
Signed-off-by: Paolo Bonzini 
---
 MdeModulePkg/Core/Dxe/Event/Tpl.c | 85 ++-
 1 file changed, 72 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Event/Tpl.c b/MdeModulePkg/Core/Dxe/Event/Tpl.c
index b33f80573c..0a4f99521c 100644
--- a/MdeModulePkg/Core/Dxe/Event/Tpl.c
+++ b/MdeModulePkg/Core/Dxe/Event/Tpl.c
@@ -9,6 +9,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include "DxeMain.h"
 #include "Event.h"
 
+///
+/// Bit mask of TPLs that were interrupted (typically during RestoreTPL's
+/// event dispatching, though there are reports that the Windows boot loader
+/// executes stray STIs at TPL_HIGH_LEVEL).  CoreRaiseTpl() sets the
+/// OldTpl-th bit when it detects it was called from and interrupt handler,
+/// because the corresponding CoreRestoreTpl() needs different semantics for
+/// the CPU interrupt state.  See CoreRaiseTpl() and CoreRestoreTpl() below.
+///