Re: [edk2-devel] [PATCH v1] UefiCpuPkg: Get processor extended information for SmmCpuServiceProtocol

[joeyli via groups.io]

Hi Hongbin1,

On Mon, May 29, 2023 at 02:39:38PM +0800, Zhang, Hongbin1 via groups.io wrote:
> Some features like RAS need to use processor extended information
> under smm, So add code to support it
> 
> Signed-off-by: Hongbin1 Zhang 

I got a ASSERT when booting edk2-stable202308 on a issue machine:

ASSERT 
/home/joeyli/source_code-git/edk2/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c(1478):
 (Index != 0) || (LevelType == 0x01)  

And, the ASSERT can also be reproduced on edk2 master. After reverted this
patch, the ASSERT is gone. 

I have filed a bug here:
https://bugzilla.tianocore.org/show_bug.cgi?id=4598

I have put some tracing information on bugzilla.

Thank a lot!
Joey Lee

> Cc: Eric Dong 
> Cc: Ray Ni 
> Cc: Rahul Kumar 
> Cc: Gerd Hoffmann 
> Cc: Star Zeng 
> Cc: Jiaxin Wu 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index c0e368ea94..8311c3b9de 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -929,7 +929,7 @@ PiCpuSmmEntry (
>  gSmmCpuPrivate->Operation[Index]= SmmCpuNone;
>  
>  if (Index < mNumberOfCpus) {
> -  Status = MpServices->GetProcessorInfo (MpServices, Index, 
> >ProcessorInfo[Index]);
> +  Status = MpServices->GetProcessorInfo (MpServices, Index | 
> CPU_V2_EXTENDED_TOPOLOGY, >ProcessorInfo[Index]);
>ASSERT_EFI_ERROR (Status);
>mCpuHotPlugData.ApicId[Index] = 
> gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId;
>  
> -- 
> 2.37.0.windows.1
> 
> 
> 
> 
> 


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

[edk2-devel] [PATCH] CloudHv: Add CI for CloudHv on AArch64

[Jianyong Wu]

Add the long lost CI for CloudHv on AArch64.

Signed-off-by: Jianyong Wu 
---
 .../.azurepipelines/Ubuntu-GCC5.yml   | 13 
 ArmVirtPkg/PlatformCI/CloudHvBuild.py | 32 +++
 2 files changed, 45 insertions(+)
 create mode 100644 ArmVirtPkg/PlatformCI/CloudHvBuild.py

diff --git a/ArmVirtPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml 
b/ArmVirtPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml
index d1772a65fc..ab8a2db530 100644
--- a/ArmVirtPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml
+++ b/ArmVirtPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml
@@ -140,6 +140,19 @@ jobs:
 Build.Target: "RELEASE"
 Run: false
 
+  CLOUDHV_AARCH64_DEBUG:
+Build.File: "$(package)/PlatformCI/CloudHvBuild.py"
+Build.Arch: "AARCH64"
+Build.Flags: ""
+Build.Target: "DEBUG"
+Run: false
+  CLOUDHV_AARCH64_RELEASE:
+Build.File: "$(package)/PlatformCI/CloudHvBuild.py"
+Build.Arch: "AARCH64"
+Build.Flags: ""
+Build.Target: "RELEASE"
+Run: false
+
 workspace:
   clean: all
 
diff --git a/ArmVirtPkg/PlatformCI/CloudHvBuild.py 
b/ArmVirtPkg/PlatformCI/CloudHvBuild.py
new file mode 100644
index 00..0192cd6577
--- /dev/null
+++ b/ArmVirtPkg/PlatformCI/CloudHvBuild.py
@@ -0,0 +1,32 @@
+# @file
+# Script to Build ArmVirtPkg UEFI firmware
+#
+# Copyright (c) Microsoft Corporation.
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+import os
+import sys
+
+sys.path.append(os.path.dirname(os.path.abspath(__file__)))
+from PlatformBuildLib import SettingsManager
+from PlatformBuildLib import PlatformBuilder
+
+# 
###
 #
+#Common Configuration  
   #
+# 
###
 #
+class CommonPlatform():
+''' Common settings for this platform.  Define static data here and use
+for the different parts of stuart
+'''
+PackagesSupported = ("ArmVirtPkg",)
+ArchSupported = ("AARCH64", "ARM")
+TargetsSupported = ("DEBUG", "RELEASE")
+Scopes = ('armvirt', 'edk2-build')
+WorkspaceRoot = os.path.realpath(os.path.join(
+os.path.dirname(os.path.abspath(__file__)), "..", ".."))
+
+DscName = os.path.join("ArmVirtPkg", "ArmVirtCloudHv.dsc")
+FvQemuArg = "" # ignored
+
+import PlatformBuildLib
+PlatformBuildLib.CommonPlatform = CommonPlatform
-- 
2.34.1



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

[edk2-devel] [PATCH v5 6/6] MdePkg: Use macro CR4_CET_BIT to replace hard code value.

[Sheng Wei]

The macro is used in file LongJump.nasm and SetJump.nasm.

Signed-off-by: Sheng Wei 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Wu Jiaxin 
Cc: Tan Dun 
---
 MdePkg/Library/BaseLib/Ia32/LongJump.nasm | 3 ++-
 MdePkg/Library/BaseLib/Ia32/SetJump.nasm  | 3 ++-
 MdePkg/Library/BaseLib/X64/LongJump.nasm  | 3 ++-
 MdePkg/Library/BaseLib/X64/SetJump.nasm   | 3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/MdePkg/Library/BaseLib/Ia32/LongJump.nasm 
b/MdePkg/Library/BaseLib/Ia32/LongJump.nasm
index 6c13dfe307..df1bf9749e 100644
--- a/MdePkg/Library/BaseLib/Ia32/LongJump.nasm
+++ b/MdePkg/Library/BaseLib/Ia32/LongJump.nasm
@@ -14,6 +14,7 @@
 ;--
 
 %include "Nasm.inc"
+%include "Cet.inc"
 
 SECTION .text
 
@@ -34,7 +35,7 @@ ASM_PFX(InternalLongJump):
 testeax, eax
 jz  CetDone
 mov eax, cr4
-bt  eax, 23; check if CET is enabled
+bt  eax, CR4_CET_BIT   ; check if CET is enabled
 jnc CetDone
 
 mov edx, [esp + 4] ; edx = JumpBuffer
diff --git a/MdePkg/Library/BaseLib/Ia32/SetJump.nasm 
b/MdePkg/Library/BaseLib/Ia32/SetJump.nasm
index 2577373241..0c484f6852 100644
--- a/MdePkg/Library/BaseLib/Ia32/SetJump.nasm
+++ b/MdePkg/Library/BaseLib/Ia32/SetJump.nasm
@@ -14,6 +14,7 @@
 ;--
 
 %include "Nasm.inc"
+%include "Cet.inc"
 
 SECTION .text
 
@@ -42,7 +43,7 @@ ASM_PFX(SetJump):
 testeax, eax
 jz  CetDone
 mov eax, cr4
-bt  eax, 23; check if CET is enabled
+bt  eax, CR4_CET_BIT   ; check if CET is enabled
 jnc CetDone
 
 mov eax, 1
diff --git a/MdePkg/Library/BaseLib/X64/LongJump.nasm 
b/MdePkg/Library/BaseLib/X64/LongJump.nasm
index 2002f65cba..021b49e855 100644
--- a/MdePkg/Library/BaseLib/X64/LongJump.nasm
+++ b/MdePkg/Library/BaseLib/X64/LongJump.nasm
@@ -14,6 +14,7 @@
 ;--
 
 %include "Nasm.inc"
+%include "Cet.inc"
 
 DEFAULT REL
 SECTION .text
@@ -35,7 +36,7 @@ ASM_PFX(InternalLongJump):
 testeax, eax
 jz  CetDone
 mov rax, cr4
-bt  eax, 23  ; check if CET is enabled
+bt  eax, CR4_CET_BIT ; check if CET is enabled
 jnc CetDone
 
 pushrdx  ; save rdx
diff --git a/MdePkg/Library/BaseLib/X64/SetJump.nasm 
b/MdePkg/Library/BaseLib/X64/SetJump.nasm
index 5943a5ebe5..d2c0991e66 100644
--- a/MdePkg/Library/BaseLib/X64/SetJump.nasm
+++ b/MdePkg/Library/BaseLib/X64/SetJump.nasm
@@ -14,6 +14,7 @@
 ;--
 
 %include "Nasm.inc"
+%include "Cet.inc"
 
 DEFAULT REL
 SECTION .text
@@ -44,7 +45,7 @@ ASM_PFX(SetJump):
 testeax, eax
 jz  CetDone
 mov rax, cr4
-bt  eax, 23  ; check if CET is enabled
+bt  eax, CR4_CET_BIT ; check if CET is enabled
 jnc CetDone
 
 mov rax, 1
-- 
2.26.2.windows.1



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

[edk2-devel] [PATCH v5 5/6] UefiCpuPkg: Backup and Restore MSR IA32_U_CET in SMI handler.

[Sheng Wei]

OS may enable CET-IBT feature by set MSR IA32_U_CET.bit2.
If IA32_U_CET.bit2 is set, CPU is in WAIT_FOR_ENDBRANCH state and
 the next assemble code is not ENDBR, it will trigger #CP exception
 when set CR4.CET bit.
SMI handler needs to backup MSR IA32_U_CET and clear MSR IA32_U_CET
 before set CR4.CET bit,
And SMI handler needs to restore MSR IA32_U_CET when exit SMI handler.

Signed-off-by: Sheng Wei 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Wu Jiaxin 
Cc: Tan Dun 
Reviewed-by: Laszlo Ersek 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 15 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm  | 15 +++
 2 files changed, 30 insertions(+)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
index 1da9afab97..9e1155dee6 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
@@ -202,11 +202,21 @@ ASM_PFX(mPatchCetSupported):
 pushedx
 pusheax
 
+mov ecx, MSR_IA32_U_CET
+rdmsr
+pushedx
+pusheax
+
 mov ecx, MSR_IA32_PL0_SSP
 rdmsr
 pushedx
 pusheax
 
+mov ecx, MSR_IA32_U_CET
+xor eax, eax
+xor edx, edx
+wrmsr
+
 mov ecx, MSR_IA32_S_CET
 mov eax, MSR_IA32_CET_SH_STK_EN
 xor edx, edx
@@ -276,6 +286,11 @@ CetDone:
 pop edx
 wrmsr
 
+mov ecx, MSR_IA32_U_CET
+pop eax
+pop edx
+wrmsr
+
 mov ecx, MSR_IA32_S_CET
 pop eax
 pop edx
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
index abf9f1a90a..881d3177f7 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
@@ -217,6 +217,11 @@ ASM_PFX(mPatchCetSupported):
 pushrdx
 pushrax
 
+mov ecx, MSR_IA32_U_CET
+rdmsr
+pushrdx
+pushrax
+
 mov ecx, MSR_IA32_PL0_SSP
 rdmsr
 pushrdx
@@ -227,6 +232,11 @@ ASM_PFX(mPatchCetSupported):
 pushrdx
 pushrax
 
+mov ecx, MSR_IA32_U_CET
+xor eax, eax
+xor edx, edx
+wrmsr
+
 mov ecx, MSR_IA32_S_CET
 mov eax, MSR_IA32_CET_SH_STK_EN
 xor edx, edx
@@ -325,6 +335,11 @@ mCetSupportedAbsAddr:
 pop rdx
 wrmsr
 
+mov ecx, MSR_IA32_U_CET
+pop rax
+pop rdx
+wrmsr
+
 mov ecx, MSR_IA32_S_CET
 pop rax
 pop rdx
-- 
2.26.2.windows.1



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

[edk2-devel] [PATCH v5 4/6] UefiCpuPkg: Only change CR4.CET bit for enable and disable CET.

[Sheng Wei]

Signed-off-by: Sheng Wei 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Wu Jiaxin 
Cc: Tan Dun 
Reviewed-by: Laszlo Ersek 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 10 +++---
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm  | 10 +++---
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
index 6368982433..1da9afab97 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
@@ -237,7 +237,9 @@ CetInterruptDone:
 bts ecx, 16 ; set WP
 mov cr0, ecx
 
-mov eax, 0x668 | CR4_CET
+; set CR4.CET bit for enable CET
+mov eax, cr4
+bts eax, CR4_CET_BIT
 mov cr4, eax
 
 setssbsy
@@ -264,8 +266,10 @@ CetDone:
 cmp al, 0
 jz  CetDone2
 
-mov eax, 0x668
-mov cr4, eax   ; disable CET
+; clear CR4.CET bit for disable CET
+mov eax, cr4
+btr eax, CR4_CET_BIT
+mov cr4, eax
 
 mov ecx, MSR_IA32_PL0_SSP
 pop eax
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
index 9a225bc3be..abf9f1a90a 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
@@ -263,7 +263,9 @@ CetInterruptDone:
 bts ecx, 16 ; set WP
 mov cr0, rcx
 
-mov eax, 0x668 | CR4_CET
+; set CR4.CET bit for enable CET
+mov rax, cr4
+bts rax, CR4_CET_BIT
 mov cr4, rax
 
 setssbsy
@@ -308,8 +310,10 @@ mCetSupportedAbsAddr:
 cmp al, 0
 jz  CetDone2
 
-mov eax, 0x668
-mov cr4, rax   ; disable CET
+; clear CR4.CET bit for disable CET
+mov rax, cr4
+btr rax, CR4_CET_BIT
+mov cr4, rax
 
 mov ecx, MSR_IA32_INTERRUPT_SSP_TABLE_ADDR
 pop rax
-- 
2.26.2.windows.1



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

[edk2-devel] [PATCH v5 3/6] UefiCpuPkg: Use CET macro definitions in Cet.inc for SmiEntry.nasm files.

[Sheng Wei]

Signed-off-by: Sheng Wei 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Wu Jiaxin 
Cc: Tan Dun 
Reviewed-by: Laszlo Ersek 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 14 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm  | 15 +--
 2 files changed, 2 insertions(+), 27 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
index 19de5f614e..6368982433 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
@@ -15,19 +15,7 @@
 
 %include "StuffRsbNasm.inc"
 %include "Nasm.inc"
-
-%define MSR_IA32_S_CET 0x6A2
-%define   MSR_IA32_CET_SH_STK_EN 0x1
-%define   MSR_IA32_CET_WR_SHSTK_EN   0x2
-%define   MSR_IA32_CET_ENDBR_EN  0x4
-%define   MSR_IA32_CET_LEG_IW_EN 0x8
-%define   MSR_IA32_CET_NO_TRACK_EN   0x10
-%define   MSR_IA32_CET_SUPPRESS_DIS  0x20
-%define   MSR_IA32_CET_SUPPRESS  0x400
-%define   MSR_IA32_CET_TRACKER   0x800
-%define MSR_IA32_PL0_SSP   0x6A4
-
-%define CR4_CET0x80
+%include "Cet.inc"
 
 %define MSR_IA32_MISC_ENABLE 0x1A0
 %define MSR_EFER  0xc080
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
index d302ca8d01..9a225bc3be 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
@@ -15,25 +15,12 @@
 
 %include "StuffRsbNasm.inc"
 %include "Nasm.inc"
+%include "Cet.inc"
 
 ;
 ; Variables referenced by C code
 ;
 
-%define MSR_IA32_S_CET 0x6A2
-%define   MSR_IA32_CET_SH_STK_EN 0x1
-%define   MSR_IA32_CET_WR_SHSTK_EN   0x2
-%define   MSR_IA32_CET_ENDBR_EN  0x4
-%define   MSR_IA32_CET_LEG_IW_EN 0x8
-%define   MSR_IA32_CET_NO_TRACK_EN   0x10
-%define   MSR_IA32_CET_SUPPRESS_DIS  0x20
-%define   MSR_IA32_CET_SUPPRESS  0x400
-%define   MSR_IA32_CET_TRACKER   0x800
-%define MSR_IA32_PL0_SSP   0x6A4
-%define MSR_IA32_INTERRUPT_SSP_TABLE_ADDR  0x6A8
-
-%define CR4_CET0x80
-
 %define MSR_IA32_MISC_ENABLE 0x1A0
 %define MSR_EFER  0xc080
 %define MSR_EFER_XD   0x800
-- 
2.26.2.windows.1



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

[edk2-devel] [PATCH v5 2/6] UefiCpuPkg: Use macro CR4_CET_BIT to replace hard code value in Cet.nasm.

[Sheng Wei]

Signed-off-by: Sheng Wei 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Wu Jiaxin 
Cc: Tan Dun 
Reviewed-by: Laszlo Ersek 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Cet.nasm | 5 +++--
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/Cet.nasm  | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Cet.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Cet.nasm
index 9d66b9c5da..3d07da1cd4 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Cet.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Cet.nasm
@@ -5,6 +5,7 @@
 
;---
 
 %include "Nasm.inc"
+%include "Cet.inc"
 
 SECTION .text
 
@@ -16,7 +17,7 @@ ASM_PFX(DisableCet):
 incsspd eax
 
 mov eax, cr4
-btr eax, 23  ; clear CET
+btr eax, CR4_CET_BIT ; clear CET
 mov cr4, eax
 ret
 
@@ -24,7 +25,7 @@ global ASM_PFX(EnableCet)
 ASM_PFX(EnableCet):
 
 mov eax, cr4
-bts eax, 23  ; set CET
+bts eax, CR4_CET_BIT ; set CET
 mov cr4, eax
 
 ; use jmp to skip the check for ret
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/Cet.nasm 
b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/Cet.nasm
index 8bbdbb31cc..700aef4703 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/Cet.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/Cet.nasm
@@ -5,6 +5,7 @@
 
;---
 
 %include "Nasm.inc"
+%include "Cet.inc"
 
 DEFAULT REL
 SECTION .text
@@ -17,7 +18,7 @@ ASM_PFX(DisableCet):
 incsspq rax
 
 mov rax, cr4
-btr eax, 23  ; clear CET
+btr eax, CR4_CET_BIT ; clear CET
 mov cr4, rax
 ret
 
@@ -25,7 +26,7 @@ global ASM_PFX(EnableCet)
 ASM_PFX(EnableCet):
 
 mov rax, cr4
-bts eax, 23  ; set CET
+bts eax, CR4_CET_BIT ; set CET
 mov cr4, rax
 
 ; use jmp to skip the check for ret
-- 
2.26.2.windows.1



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

[edk2-devel] [PATCH v5 0/6] MdePkg: Add macro definitions for CET feature for NASM files.

[Sheng Wei]

Patch V5:
  File cet.inc will be used in both MdePkg UefiCpuPkg.
  Move cet.inc file from UefiCpuPkg to MdePkg.
  Use macro CR4_CET_BIT to replace hard code value for
   both LongJump.nasm and SetJump.nasm.

Patch V4:
  Separate the changes to 5 patches.
1) Add macro definitions for CET feature for NASM files.
2) Use macro CR4_CET_BIT to replace hard code value in Cet.nasm.
3) Use CET macro definitions in Cet.inc for SmiEntry.nasm files.
4) Only change CR4.CET bit for enable/disable CET.
5) Backup and Restore MSR IA32_U_CET in SMI handler.
  Remove some unused code.
It is no need to clear MSR IA32_S_CET,
 because clear CR4.CET bit will disable all CET functions.
Since CET is disabled between clear CR4.CET and run 'rsm',
 it is no need to delay MSR IA32_S_CET restoration.

Patch V3:
  Remove the 3rd patch. mSmmInterruptSspTables is a global variable.
  It is unnecessary to initializ it to zero manually.

Patch V2:
  No function change with Patch V1.
  Split the patch to into 3 separate patches.


Sheng Wei (6):
  MdePkg: Add macro definitions for CET feature for NASM files.
  UefiCpuPkg: Use macro CR4_CET_BIT to replace hard code value in
Cet.nasm.
  UefiCpuPkg: Use CET macro definitions in Cet.inc for SmiEntry.nasm
files.
  UefiCpuPkg: Only change CR4.CET bit for enable and disable CET.
  UefiCpuPkg: Backup and Restore MSR IA32_U_CET in SMI handler.
  MdePkg: Use macro CR4_CET_BIT to replace hard code value.

 MdePkg/Include/Cet.inc   | 26 +
 MdePkg/Library/BaseLib/Ia32/LongJump.nasm|  3 +-
 MdePkg/Library/BaseLib/Ia32/SetJump.nasm |  3 +-
 MdePkg/Library/BaseLib/X64/LongJump.nasm |  3 +-
 MdePkg/Library/BaseLib/X64/SetJump.nasm  |  3 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Cet.nasm  |  5 ++-
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 39 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/Cet.nasm   |  5 ++-
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm  | 40 +++-
 9 files changed, 86 insertions(+), 41 deletions(-)
 create mode 100644 MdePkg/Include/Cet.inc

-- 
2.26.2.windows.1



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

[edk2-devel] [PATCH v5 1/6] MdePkg: Add macro definitions for CET feature for NASM files.

[Sheng Wei]

Signed-off-by: Sheng Wei 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Wu Jiaxin 
Cc: Tan Dun 
Reviewed-by: Laszlo Ersek 
---
 MdePkg/Include/Cet.inc | 26 ++
 1 file changed, 26 insertions(+)
 create mode 100644 MdePkg/Include/Cet.inc

diff --git a/MdePkg/Include/Cet.inc b/MdePkg/Include/Cet.inc
new file mode 100644
index 00..a4038a0682
--- /dev/null
+++ b/MdePkg/Include/Cet.inc
@@ -0,0 +1,26 @@
+;--
+;
+; Copyright (c) 2023, Intel Corporation. All rights reserved.
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+; Abstract:
+;
+;   This file provides macro definitions for CET feature for NASM files.
+;
+;--
+
+%define MSR_IA32_U_CET 0x6A0
+%define MSR_IA32_S_CET 0x6A2
+%define MSR_IA32_CET_SH_STK_EN 0x1
+%define MSR_IA32_CET_WR_SHSTK_EN   0x2
+%define MSR_IA32_CET_ENDBR_EN  0x4
+%define MSR_IA32_CET_LEG_IW_EN 0x8
+%define MSR_IA32_CET_NO_TRACK_EN   0x10
+%define MSR_IA32_CET_SUPPRESS_DIS  0x20
+%define MSR_IA32_CET_SUPPRESS  0x400
+%define MSR_IA32_CET_TRACKER   0x800
+%define MSR_IA32_PL0_SSP   0x6A4
+%define MSR_IA32_INTERRUPT_SSP_TABLE_ADDR  0x6A8
+
+%define CR4_CET_BIT23
+%define CR4_CET0x80
-- 
2.26.2.windows.1



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

Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

Just let you know I just merged this change. Igor can help to follow up the 
suggestions given by Leif and Mike.

Thanks
Abner

> -Original Message-
> From: Chang, Abner
> Sent: Wednesday, November 15, 2023 9:20 AM
> To: Mike Maslenkin ; devel@edk2.groups.io;
> ig...@ami.com
> Cc: Leif Lindholm ; Nickle Wang
> 
> Subject: RE: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe:
> Optimize the Redfish Discover flow
>
> Hi Mike and Leif,
> Thanks for your comments on this change. As we are rushing to get this
> change to be pulled in stable release 202312 this week, I will just merge this
> code to master branch and let the discussing keeps going.
> I think there is no functionality difference base on your suggestions, but 
> it's
> about the coding practice and readability.
>
> Hi Igor,
> Could you please resend the V6 after stable tag is released if Mike and Leif's
> comment is reasonable to you?
>
> Thanks
> Abner
>
> > -Original Message-
> > From: Mike Maslenkin 
> > Sent: Wednesday, November 15, 2023 7:53 AM
> > To: devel@edk2.groups.io; ig...@ami.com
> > Cc: Leif Lindholm ; Chang, Abner
> > ; Nickle Wang 
> > Subject: Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe:
> > Optimize the Redfish Discover flow
> >
> > Caution: This message originated from an External Source. Use proper
> caution
> > when opening attachments, clicking links, or responding.
> >
> >
> > On Tue, Nov 14, 2023 at 9:57 PM Igor Kulchytskyy via groups.io
> >  wrote:
> > >
> > > Hi Leif,
> > > Please see my comments below.
> > > Thank you,
> > > Igor
> > >
> > >
> > > -Original Message-
> > > From: Leif Lindholm 
> > > Sent: Tuesday, November 14, 2023 12:26 PM
> > > To: devel@edk2.groups.io; Igor Kulchytskyy 
> > > Cc: Abner Chang ; Nickle Wang
> > 
> > > Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg:
> > RedfishDiscoverDxe: Optimize the Redfish Discover flow
> > >
> > >
> > > **CAUTION: The e-mail below is from an external source. Please exercise
> > caution before opening attachments, clicking links, or following guidance.**
> > >
> > > On 2023-11-14 14:28, Igor Kulchytskyy via groups.io wrote:
> > > > Filter out the network interfaces which are not supported by
> > > > Redfish Host Interface.
> > > >
> > > > Cc: Abner Chang 
> > > > Cc: Nickle Wang 
> > > > Signed-off-by: Igor Kulchytskyy 
> > > > ---
> > > >   RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c  | 163
> > ++--
> > > >   RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h |   6 +
> > > >   2 files changed, 120 insertions(+), 49 deletions(-)
> > > >
> > > > diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > > > index 0f622e05a9..ae83cd3c97 100644
> > > > --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > > > +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > >
> > >
> > > > @@ -1601,10 +1681,22 @@ BuildupNetworkInterface (
> > > > EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL
> > *RestExInstance;
> > > > EFI_TPL  OldTpl;
> > > > BOOLEAN  
> > > > NewNetworkInterfaceInstalled;
> > > > +  UINT8IpType;
> > > > +  UINTNListCount;
> > > >
> > > > +  ListCount= (sizeof (gRequiredProtocol) / sizeof
> > (REDFISH_DISCOVER_REQUIRED_PROTOCOL));
> > > > NewNetworkInterfaceInstalled = FALSE;
> > > > Index= 0;
> > > > -  do {
> > > > +
> > > > +  // Get IP Type to filter out unnecessary network protocol if possible
> > > > +  IpType = GetHiIpProtocolType ();
> > > > +
> > > > +  for (Index = 0; Index < ListCount; Index++) {
> > > > +// Check IP Type and skip an unnecessary network protocol if does
> not
> > match
> > > > +if (IS_TCP4_MATCH (IpType) || IS_TCP6_MATCH (IpType)) {
> > >
> > > The logic of these macros is inverted compared to their names, though.
> > >
> > > You want this test to read
> > >if (!IS_TCP4_MATCH (IpType) && !IS_TCP6_MATCH (IpType)) {
> > >
> > > > +  continue;
> > > > +}
> > > > +
> > > >   Status = gBS->OpenProtocol (
> > > >   // Already in list?
> > > >   ControllerHandle,
> > >
> > > > diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> > b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> > > > index 01454acc1d..3093eea0d5 100644
> > > > --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> > > > +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> > > > @@ -39,6 +39,12 @@
> > > >   #define REDFISH_DISCOVER_VERSION0x0001
> > > >   #define EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_TPL
> TPL_NOTIFY
> > > >
> > > > +#define MAC_COMPARE(ThisNetworkInterface,
> TargetNetworkInterface)

Re: [edk2-devel] [PATCH v2 24/30] OvmfPkg/LoongArchVirt: Add platform boot manager library

[Chao Li]

Well, let's discuss the ARM version name once moved.

I have two plans:

*Plan A:*

Merge the ARM version into PlatformBootmanagerLib and modify the inf 
file to separate X64 and other ARCH, like be:


[Sources.Common]

    QemuKernel.c

    BdsPlatform.h

[Sources.X64, Sources.IA32]

    BdsPlatform.c

    PaltformData.c

[Sources.ARM, Sources.AARCH64, Sources.RISCV64, Source.LOONGARCH64]

    PlatformBm.c

[LibraryClasses.Common]

    BaseLib

    BaseMemoryLib

    BootLogoLib

    DebugLib

    DevicePathLib

    MemoryAllocationLib

    PcdLib

    ...

[LibraryClasses.X64, LibraryClasses.IA32]

    QemuFwCfgLib

    QemuFwCfgS3Lib

    ...

[LibraryClasses.ARM, LibraryClasses.AARCH64, LibraryClasses.RISCV, 
LibraryClasses.LOONGARCH64]


    TpmPlatformHierarchyLib

    ...


The above pseudocode are unverified and will definitely be subject to 
modification.



*Plan B:*

Moved the ARM version into OvmfPkg and got a new name. In my opinion, 
the X86 version takes into account the STR, Tcg2 and Xen platform, so it 
look like more complete(only for X86, just my opinion). In this case, I 
think what about  the X86 version still being named 
PlatformBootManagerLib and the ARM version being named 
PlatformBootManagerLibLight?



Both of the above plans can achieve the goal. I prefer Plan A. I want to 
know your opinions, so hope hear back from you!



Thanks,
Chao
On 2023/11/13 19:08, Laszlo Ersek wrote:

On 11/10/23 10:46, Gerd Hoffmann wrote:

On Fri, Nov 10, 2023 at 03:09:47PM +0800, Chao Li wrote:

Hi Laszlo,

Sorry, I'm not check carefully, it is really **copied**, and we not think
the ARM version is not good enough.

So, can I move this library to OvmfPkg so other ARCH use it easily?

Moving code from ArmVirtPkg to OvmfPkg is fine.

OvmfPkg is the home for both x86 virtual machine bits and shared code.
The later used to be mostly virtio drivers, but with the arrival of
riscv some fdt support code has already moved from ArmVirtPkg to OvmfPkg
so arm and riscv can share it.  Doing the same for loongarch is
perfectly fine.

Agreed!

The naming of course remains tricky. :) It's not easy to come up with
good names for distinguishing various instances of the same library class.

I suggest renaming "OvmfPkg/PlatformBootManagerLib" to
"PlatformBootManagerLibX86", and calling ArmVirtPkg's instance (once
moved) PlatformBootManagerLibGeneric or just PlatformBootManagerLib.

Laszlo








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

[edk2-devel] [PATCH] DynamicTablesPkg: Fix ETE _UID Creation

[Ashish Singhal via groups.io]

Just like CPU _UID, ETE UID also needs to be unique so
use AcpiProcessorUid instead of CpuName

Signed-off-by: Ashish Singhal 
---
 .../Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git 
a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
 
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
index 8228c7845a..724f33c660 100644
--- 
a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
+++ 
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtCpuTopologyLibArm/SsdtCpuTopologyGenerator.c
@@ -359,6 +359,7 @@ CreateAmlCpcNode (
 
   @param [in]  GeneratorThe SSDT Cpu Topology generator.
   @param [in]  ParentNode   Parent node to attach the Cpu node to.
+  @param [in]  GicCInfo CM_ARM_GICC_INFO object used to create the node.
   @param [in]  CpuName  Value used to generate the node name.
   @param [out] EtNodePtr   If not NULL, return the created Cpu node.
 
@@ -372,6 +373,7 @@ EFIAPI
 CreateAmlEtd (
   IN   ACPI_CPU_TOPOLOGY_GENERATOR  *Generator,
   IN   AML_NODE_HANDLE  ParentNode,
+  IN   CM_ARM_GICC_INFO *GicCInfo,
   IN   UINT32   CpuName,
   OUT  AML_OBJECT_NODE_HANDLE   *EtNodePtr OPTIONAL
   )
@@ -397,7 +399,7 @@ CreateAmlEtd (
 
   Status = AmlCodeGenNameInteger (
  "_UID",
- CpuName,
+ GicCInfo->AcpiProcessorUid,
  EtNode,
  NULL
  );
@@ -474,6 +476,7 @@ CreateAmlEtNode (
   Status = CreateAmlEtd (
  Generator,
  Node,
+ GicCInfo,
  CpuName,
  NULL
  );
-- 
2.17.1



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

Re: [edk2-devel] [PATCH 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug

[Ashish Singhal via groups.io]

Hello Abner,

Thanks for the feedback. I have pushed v2 of the patch set.

Thanks
Ashish

From: Chang, Abner 
Sent: Thursday, November 9, 2023 7:06 PM
To: Ashish Singhal ; devel@edk2.groups.io 
; quic_llind...@quicinc.com ; 
ardb+tianoc...@kernel.org ; g...@danielschaefer.me 
; Jeff Brasen 
Subject: RE: [PATCH 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug

External email: Use caution opening links or attachments


[AMD Official Use Only - General]






From: Ashish Singhal 
Sent: Tuesday, November 7, 2023 1:55 AM
To: devel@edk2.groups.io; quic_llind...@quicinc.com; ardb+tianoc...@kernel.org; 
Chang, Abner ; g...@danielschaefer.me; Jeff Brasen 

Subject: Re: [PATCH 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug



Caution: This message originated from an External Source. Use proper caution 
when opening attachments, clicking links, or responding.



Hello,



Hoping to get some feedback on these 2 patches this week.



Thanks

Ashish



From: Ashish Singhal mailto:ashishsin...@nvidia.com>>
Sent: Monday, October 30, 2023 2:24 PM
To: devel@edk2.groups.io 
mailto:devel@edk2.groups.io>>; 
quic_llind...@quicinc.com 
mailto:quic_llind...@quicinc.com>>; 
ardb+tianoc...@kernel.org 
mailto:ardb+tianoc...@kernel.org>>; 
abner.ch...@amd.com 
mailto:abner.ch...@amd.com>>; 
g...@danielschaefer.me 
mailto:g...@danielschaefer.me>>; Jeff Brasen 
mailto:jbra...@nvidia.com>>
Cc: Ashish Singhal mailto:ashishsin...@nvidia.com>>
Subject: [PATCH 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug



Curently, AndroidBootImgLib expects input kernel command line
to never exceed 256 unicode characters where the image header
allows for 512 ascii characters. If image header allows 512
ascii characters, similar number of unicode characters should be
allowed at the minimum.

Signed-off-by: Ashish Singhal 
mailto:ashishsin...@nvidia.com>>
---
 .../AndroidBootImgLib/AndroidBootImgLib.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c 
b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
index 1359a66db2..02769cd0df 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
@@ -321,8 +321,9 @@ AndroidBootImgGetFdt (

 EFI_STATUS
 AndroidBootImgUpdateArgs (
-  IN  VOID  *BootImg,
-  OUT VOID  *KernelArgs
+  IN  VOID*BootImg,
+  OUT VOID*KernelArgs,
+  IN  UINT32  KernelArgsSize
   )
 {
   CHAR8   ImageKernelArgs[ANDROID_BOOTIMG_KERNEL_ARGS_SIZE];
@@ -337,13 +338,13 @@ AndroidBootImgUpdateArgs (
   AsciiStrToUnicodeStrS (
 ImageKernelArgs,
 KernelArgs,
-ANDROID_BOOTIMG_KERNEL_ARGS_SIZE >> 1

+KernelArgsSize
 );
   // Append platform kernel arguments
   if (mAndroidBootImg->AppendArgs) {
 Status = mAndroidBootImg->AppendArgs (
 KernelArgs,
-ANDROID_BOOTIMG_KERNEL_ARGS_SIZE
+KernelArgsSize
 );
   }

@@ -611,11 +612,16 @@ AndroidBootImgBoot (
   MEMORY_DEVICE_PATH KernelDevicePath;
   EFI_HANDLE ImageHandle;
   VOID   *NewKernelArg;
+  UINT32 NewKernelArgSize;
   EFI_LOADED_IMAGE_PROTOCOL  *ImageInfo;
   VOID   *RamdiskData;
   UINTN  RamdiskSize;
   IN  VOID   *FdtBase;

+  if ((Buffer == NULL) || (BufferSize == 0)) {
+return EFI_INVALID_PARAMETER;
+  }
+
   NewKernelArg = NULL;
   ImageHandle  = NULL;

@@ -637,14 +643,15 @@ AndroidBootImgBoot (
 goto Exit;
   }

-  NewKernelArg = AllocateZeroPool (ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
+  NewKernelArgSize = ANDROID_BOOTIMG_KERNEL_ARGS_SIZE;
+  NewKernelArg = AllocateZeroPool (sizeof (CHAR16) * NewKernelArgSize);

I think you can just move the memory allocation code to inside 
AndroidBootImgUpdateArgs then you don’t need the additional arg for 
AndroidBootImgUpdateArgs.

Also Change AndroidBootImgUpdateArgs (Buffer, NewKernelArg); to 
AndroidBootImgUpdateArgs (Buffer, );

With this the code looks simple.

thanks

Abner

   if (NewKernelArg == NULL) {
 DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
 Status = EFI_OUT_OF_RESOURCES;
 goto Exit;
   }

-  Status = AndroidBootImgUpdateArgs (Buffer, NewKernelArg);
+  Status = AndroidBootImgUpdateArgs (Buffer, NewKernelArg, NewKernelArgSize);
   if (EFI_ERROR (Status)) {
 goto Exit;
   }
--
2.17.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111230): https://edk2.groups.io/g/devel/message/111230
Mute This Topic: https://groups.io/mt/102284239/21656

[edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug

[Ashish Singhal via groups.io]

Curently, AndroidBootImgLib expects input kernel command line
to never exceed 256 unicode characters where the image header
allows for 512 ascii characters. If image header allows 512
ascii characters, similar number of unicode characters should be
allowed at the minimum.

Signed-off-by: Ashish Singhal 
---
 .../AndroidBootImgLib/AndroidBootImgLib.c | 31 +++
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c 
b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
index 1359a66db2..f63648e60d 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
@@ -322,11 +322,12 @@ AndroidBootImgGetFdt (
 EFI_STATUS
 AndroidBootImgUpdateArgs (
   IN  VOID  *BootImg,
-  OUT VOID  *KernelArgs
+  OUT VOID  **KernelArgs
   )
 {
   CHAR8   ImageKernelArgs[ANDROID_BOOTIMG_KERNEL_ARGS_SIZE];
   EFI_STATUS  Status;
+  UINT32  NewKernelArgSize;
 
   // Get kernel arguments from Android boot image
   Status = AndroidBootImgGetKernelArgs (BootImg, ImageKernelArgs);
@@ -334,16 +335,23 @@ AndroidBootImgUpdateArgs (
 return Status;
   }
 
+  NewKernelArgSize = ANDROID_BOOTIMG_KERNEL_ARGS_SIZE;
+  *KernelArgs  = AllocateZeroPool (sizeof (CHAR16) * NewKernelArgSize);
+  if (*KernelArgs == NULL) {
+DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
+return EFI_OUT_OF_RESOURCES;
+  }
+
   AsciiStrToUnicodeStrS (
 ImageKernelArgs,
-KernelArgs,
-ANDROID_BOOTIMG_KERNEL_ARGS_SIZE >> 1
+*KernelArgs,
+NewKernelArgSize
 );
   // Append platform kernel arguments
   if (mAndroidBootImg->AppendArgs) {
 Status = mAndroidBootImg->AppendArgs (
-KernelArgs,
-ANDROID_BOOTIMG_KERNEL_ARGS_SIZE
+*KernelArgs,
+NewKernelArgSize
 );
   }
 
@@ -616,6 +624,10 @@ AndroidBootImgBoot (
   UINTN  RamdiskSize;
   IN  VOID   *FdtBase;
 
+  if ((Buffer == NULL) || (BufferSize == 0)) {
+return EFI_INVALID_PARAMETER;
+  }
+
   NewKernelArg = NULL;
   ImageHandle  = NULL;
 
@@ -637,14 +649,7 @@ AndroidBootImgBoot (
 goto Exit;
   }
 
-  NewKernelArg = AllocateZeroPool (ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
-  if (NewKernelArg == NULL) {
-DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
-Status = EFI_OUT_OF_RESOURCES;
-goto Exit;
-  }
-
-  Status = AndroidBootImgUpdateArgs (Buffer, NewKernelArg);
+  Status = AndroidBootImgUpdateArgs (Buffer, );
   if (EFI_ERROR (Status)) {
 goto Exit;
   }
-- 
2.17.1



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

[edk2-devel] [PATCH v2 2/2] EmbeddedPkg: Allow longer android kernel command line

[Ashish Singhal via groups.io]

AndroidBootImgLib allows for platforms to append to kernel command
line but does not allow for the overall kernel command line to go
beyond the limit set by the image header. Address this limitation
by adding a pcd where platform can tell how many extra characters
they expect on their platform in addition to what the image header
specifies.

Signed-off-by: Ashish Singhal 
---
 EmbeddedPkg/EmbeddedPkg.dec | 5 +
 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c   | 2 +-
 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf | 3 ++-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index 341ef5e6a6..94dc3c9b76 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -183,3 +183,8 @@
   # Selection between DT and ACPI as a default
   #
   gEmbeddedTokenSpaceGuid.PcdDefaultDtPref|TRUE|BOOLEAN|0x059
+
+  #
+  # Expected Overflow Android Kernel Command Line Characters
+  #
+  
gEmbeddedTokenSpaceGuid.PcdAndroidKernelCommandLineOverflow|0|UINT32|0x05C
diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c 
b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
index f63648e60d..d16929f2bb 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
@@ -335,7 +335,7 @@ AndroidBootImgUpdateArgs (
 return Status;
   }
 
-  NewKernelArgSize = ANDROID_BOOTIMG_KERNEL_ARGS_SIZE;
+  NewKernelArgSize = ANDROID_BOOTIMG_KERNEL_ARGS_SIZE + PcdGet32 
(PcdAndroidKernelCommandLineOverflow);
   *KernelArgs  = AllocateZeroPool (sizeof (CHAR16) * NewKernelArgSize);
   if (*KernelArgs == NULL) {
 DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf 
b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
index 8eefeef4f9..9754664df5 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
@@ -45,5 +45,6 @@
   gEfiAcpiTableGuid
   gFdtTableGuid
 
-[FeaturePcd]
+[Pcd]
   gEmbeddedTokenSpaceGuid.PcdAndroidBootLoadFile2
+  gEmbeddedTokenSpaceGuid.PcdAndroidKernelCommandLineOverflow
-- 
2.17.1



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

Re: [edk2-devel] [PATCH v4 1/5] UefiCpuPkg: Add macro definitions for CET feature for NASM files.

[Ni, Ray]

If only CpuSmm driver consumes these macros, can we move the macro definitions 
into CpuSmm driver folder?

Thanks,
Ray

From: Sheng, W 
Sent: Monday, November 13, 2023 2:22 PM
To: devel@edk2.groups.io 
Cc: Dong, Eric ; Ni, Ray ; Laszlo Ersek 
; Wu, Jiaxin ; Tan, Dun 

Subject: [PATCH v4 1/5] UefiCpuPkg: Add macro definitions for CET feature for 
NASM files.

Signed-off-by: Sheng Wei 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Wu Jiaxin 
Cc: Tan Dun 
---
 UefiCpuPkg/Include/Cet.inc | 26 ++
 1 file changed, 26 insertions(+)
 create mode 100644 UefiCpuPkg/Include/Cet.inc

diff --git a/UefiCpuPkg/Include/Cet.inc b/UefiCpuPkg/Include/Cet.inc
new file mode 100644
index 00..a4038a0682
--- /dev/null
+++ b/UefiCpuPkg/Include/Cet.inc
@@ -0,0 +1,26 @@
+;--

+;

+; Copyright (c) 2023, Intel Corporation. All rights reserved.

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

+;

+; Abstract:

+;

+;   This file provides macro definitions for CET feature for NASM files.

+;

+;--

+

+%define MSR_IA32_U_CET 0x6A0

+%define MSR_IA32_S_CET 0x6A2

+%define MSR_IA32_CET_SH_STK_EN 0x1

+%define MSR_IA32_CET_WR_SHSTK_EN   0x2

+%define MSR_IA32_CET_ENDBR_EN  0x4

+%define MSR_IA32_CET_LEG_IW_EN 0x8

+%define MSR_IA32_CET_NO_TRACK_EN   0x10

+%define MSR_IA32_CET_SUPPRESS_DIS  0x20

+%define MSR_IA32_CET_SUPPRESS  0x400

+%define MSR_IA32_CET_TRACKER   0x800

+%define MSR_IA32_PL0_SSP   0x6A4

+%define MSR_IA32_INTERRUPT_SSP_TABLE_ADDR  0x6A8

+

+%define CR4_CET_BIT23

+%define CR4_CET0x80

--
2.26.2.windows.1



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

Re: [edk2-devel] [PATCH v4 0/6] StarFive/VisionFive2: Add VisionFive 2 platform

[John Chew]

Hi Sunil,

I have performed the CI check for this series.

It passes the check.

Can you have to merge it when you have the time?

Thanks!

John


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

Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow

[Igor Kulchytskyy via groups.io]

Hi Abner,
I will address Leif's and Mike's comments after stable release.
Thank you,
Igor

Get Outlook for Android


From: Chang, Abner 
Sent: Tuesday, November 14, 2023 8:19:41 PM
To: Mike Maslenkin ; devel@edk2.groups.io 
; Igor Kulchytskyy 
Cc: Leif Lindholm ; Nickle Wang 
Subject: [EXTERNAL] RE: [edk2-devel] [PATCH v5 2/2] RedfishPkg: 
RedfishDiscoverDxe: Optimize the Redfish Discover flow


**CAUTION: The e-mail below is from an external source. Please exercise caution 
before opening attachments, clicking links, or following guidance.**

[AMD Official Use Only - General]

Hi Mike and Leif,
Thanks for your comments on this change. As we are rushing to get this change 
to be pulled in stable release 202312 this week, I will just merge this code to 
master branch and let the discussing keeps going.
I think there is no functionality difference base on your suggestions, but it's 
about the coding practice and readability.

Hi Igor,
Could you please resend the V6 after stable tag is released if Mike and Leif's 
comment is reasonable to you?

Thanks
Abner

> -Original Message-
> From: Mike Maslenkin 
> Sent: Wednesday, November 15, 2023 7:53 AM
> To: devel@edk2.groups.io; ig...@ami.com
> Cc: Leif Lindholm ; Chang, Abner
> ; Nickle Wang 
> Subject: Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe:
> Optimize the Redfish Discover flow
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Tue, Nov 14, 2023 at 9:57 PM Igor Kulchytskyy via groups.io
>  wrote:
> >
> > Hi Leif,
> > Please see my comments below.
> > Thank you,
> > Igor
> >
> >
> > -Original Message-
> > From: Leif Lindholm 
> > Sent: Tuesday, November 14, 2023 12:26 PM
> > To: devel@edk2.groups.io; Igor Kulchytskyy 
> > Cc: Abner Chang ; Nickle Wang
> 
> > Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg:
> RedfishDiscoverDxe: Optimize the Redfish Discover flow
> >
> >
> > **CAUTION: The e-mail below is from an external source. Please exercise
> caution before opening attachments, clicking links, or following guidance.**
> >
> > On 2023-11-14 14:28, Igor Kulchytskyy via groups.io wrote:
> > > Filter out the network interfaces which are not supported by
> > > Redfish Host Interface.
> > >
> > > Cc: Abner Chang 
> > > Cc: Nickle Wang 
> > > Signed-off-by: Igor Kulchytskyy 
> > > ---
> > >   RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c  | 163
> ++--
> > >   RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h |   6 +
> > >   2 files changed, 120 insertions(+), 49 deletions(-)
> > >
> > > diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > > index 0f622e05a9..ae83cd3c97 100644
> > > --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > > +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> >
> >
> > > @@ -1601,10 +1681,22 @@ BuildupNetworkInterface (
> > > EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL
> *RestExInstance;
> > > EFI_TPL  OldTpl;
> > > BOOLEAN  
> > > NewNetworkInterfaceInstalled;
> > > +  UINT8IpType;
> > > +  UINTNListCount;
> > >
> > > +  ListCount= (sizeof (gRequiredProtocol) / sizeof
> (REDFISH_DISCOVER_REQUIRED_PROTOCOL));
> > > NewNetworkInterfaceInstalled = FALSE;
> > > Index= 0;
> > > -  do {
> > > +
> > > +  // Get IP Type to filter out unnecessary network protocol if possible
> > > +  IpType = GetHiIpProtocolType ();
> > > +
> > > +  for (Index = 0; Index < ListCount; Index++) {
> > > +// Check IP Type and skip an unnecessary network protocol if does not
> match
> > > +if (IS_TCP4_MATCH (IpType) || IS_TCP6_MATCH (IpType)) {
> >
> > The logic of these macros is inverted compared to their names, though.
> >
> > You want this test to read
> >if (!IS_TCP4_MATCH (IpType) && !IS_TCP6_MATCH (IpType)) {
> >
> > > +  continue;
> > > +}
> > > +
> > >   Status = gBS->OpenProtocol (
> > >   // Already in list?
> > >   ControllerHandle,
> >
> > > diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> > > index 01454acc1d..3093eea0d5 100644
> > > --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> > > +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> > > @@ -39,6 +39,12 @@
> > >   #define REDFISH_DISCOVER_VERSION0x0001
> > >   #define EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_TPL  TPL_NOTIFY
> > >
> > > +#define MAC_COMPARE(ThisNetworkInterface, TargetNetworkInterface)
> (CompareMem ((VOID *)>MacAddress,
> >MacAddress, 

Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

Hi Mike and Leif,
Thanks for your comments on this change. As we are rushing to get this change 
to be pulled in stable release 202312 this week, I will just merge this code to 
master branch and let the discussing keeps going.
I think there is no functionality difference base on your suggestions, but it's 
about the coding practice and readability.

Hi Igor,
Could you please resend the V6 after stable tag is released if Mike and Leif's 
comment is reasonable to you?

Thanks
Abner

> -Original Message-
> From: Mike Maslenkin 
> Sent: Wednesday, November 15, 2023 7:53 AM
> To: devel@edk2.groups.io; ig...@ami.com
> Cc: Leif Lindholm ; Chang, Abner
> ; Nickle Wang 
> Subject: Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe:
> Optimize the Redfish Discover flow
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Tue, Nov 14, 2023 at 9:57 PM Igor Kulchytskyy via groups.io
>  wrote:
> >
> > Hi Leif,
> > Please see my comments below.
> > Thank you,
> > Igor
> >
> >
> > -Original Message-
> > From: Leif Lindholm 
> > Sent: Tuesday, November 14, 2023 12:26 PM
> > To: devel@edk2.groups.io; Igor Kulchytskyy 
> > Cc: Abner Chang ; Nickle Wang
> 
> > Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg:
> RedfishDiscoverDxe: Optimize the Redfish Discover flow
> >
> >
> > **CAUTION: The e-mail below is from an external source. Please exercise
> caution before opening attachments, clicking links, or following guidance.**
> >
> > On 2023-11-14 14:28, Igor Kulchytskyy via groups.io wrote:
> > > Filter out the network interfaces which are not supported by
> > > Redfish Host Interface.
> > >
> > > Cc: Abner Chang 
> > > Cc: Nickle Wang 
> > > Signed-off-by: Igor Kulchytskyy 
> > > ---
> > >   RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c  | 163
> ++--
> > >   RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h |   6 +
> > >   2 files changed, 120 insertions(+), 49 deletions(-)
> > >
> > > diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > > index 0f622e05a9..ae83cd3c97 100644
> > > --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > > +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> >
> >
> > > @@ -1601,10 +1681,22 @@ BuildupNetworkInterface (
> > > EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL
> *RestExInstance;
> > > EFI_TPL  OldTpl;
> > > BOOLEAN  
> > > NewNetworkInterfaceInstalled;
> > > +  UINT8IpType;
> > > +  UINTNListCount;
> > >
> > > +  ListCount= (sizeof (gRequiredProtocol) / sizeof
> (REDFISH_DISCOVER_REQUIRED_PROTOCOL));
> > > NewNetworkInterfaceInstalled = FALSE;
> > > Index= 0;
> > > -  do {
> > > +
> > > +  // Get IP Type to filter out unnecessary network protocol if possible
> > > +  IpType = GetHiIpProtocolType ();
> > > +
> > > +  for (Index = 0; Index < ListCount; Index++) {
> > > +// Check IP Type and skip an unnecessary network protocol if does not
> match
> > > +if (IS_TCP4_MATCH (IpType) || IS_TCP6_MATCH (IpType)) {
> >
> > The logic of these macros is inverted compared to their names, though.
> >
> > You want this test to read
> >if (!IS_TCP4_MATCH (IpType) && !IS_TCP6_MATCH (IpType)) {
> >
> > > +  continue;
> > > +}
> > > +
> > >   Status = gBS->OpenProtocol (
> > >   // Already in list?
> > >   ControllerHandle,
> >
> > > diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> > > index 01454acc1d..3093eea0d5 100644
> > > --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> > > +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> > > @@ -39,6 +39,12 @@
> > >   #define REDFISH_DISCOVER_VERSION0x0001
> > >   #define EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_TPL  TPL_NOTIFY
> > >
> > > +#define MAC_COMPARE(ThisNetworkInterface, TargetNetworkInterface)
> (CompareMem ((VOID *)>MacAddress,
> >MacAddress, ThisNetworkInterface-
> >HwAddressSize))
> > > +#define VALID_TCP6(TargetNetworkInterface, ThisNetworkInterface)
> (TargetNetworkInterface->IsIpv6 && (ThisNetworkInterface-
> >NetworkProtocolType == ProtocolTypeTcp6))
> > > +#define VALID_TCP4(TargetNetworkInterface, ThisNetworkInterface)
> (!TargetNetworkInterface->IsIpv6 && (ThisNetworkInterface-
> >NetworkProtocolType == ProtocolTypeTcp4))
> > > +#define IS_TCP4_MATCH(IpType)
> ((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp4) && (IpType !=
> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4))
> > > +#define IS_TCP6_MATCH(IpType)
> 

Re: [edk2-devel] CodeQL Analysis in edk2

[Michael Kubacki]

On 11/13/2023 8:42 AM, Laszlo Ersek wrote:

sorry, unfinished thought:

On 11/13/23 14:39, Laszlo Ersek wrote:


- the "sarif emacs" output seems a bit broken, actually, so it's not usable. 
Consider the following entry from the original JSON file:

 }, {
   "ruleId" : "cpp/missing-null-test",
   "ruleIndex" : 0,
   "rule" : {
 "id" : "cpp/missing-null-test",
 "index" : 0
   },
   "message" : {
 "text" : "Value may be null; it should be checked before 
dereferencing."
   },
   "locations" : [ {
 "physicalLocation" : {
   "artifactLocation" : {
 "uri" : 
"MdeModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c",
 "uriBaseId" : "%SRCROOT%",
 "index" : 0
   },
   "region" : {
 "startLine" : 355,
 "startColumn" : 48,
 "endColumn" : 52
   }
 }
   } ],
   "partialFingerprints" : {
 "primaryLocationLineHash" : "f374f6e6dfc92010:1",
 "primaryLocationStartColumnFingerprint" : "43"
   }
 }, {

In the "emacs" output, it appears as:


ModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c:355: 
cpp/missing-null-test Value may be null; it should be checked before 
dereferencing.


Note that the first three characters, "Mde" of "Mde" are lost.


I meant '"Mde" of "ModulePkg"'.


I was able to reproduce this with sarif-tools version 2.0.0.

It impacted other commands like "html" as well.

Applying the "--no-autotrim" option appears to leave the path alone. Can 
you please let me know if that works for you?


Also, yes, I can add this to the CodeQL GitHub workflow.



This issue (first three chars cut) affects all other pathnames in the emacs 
output too.





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

Re: [edk2-devel] [PATCH 0/3] Move gMpInformationHobGuid from StandaloneMmPkg to UefiCpuPkg.

[duntan]

Laszlo,

Thanks for your comments. 
Yes the StatusFlag field of a given ProcessorId does change in the scenario you 
mentioned. I think it's ok to call SwitchBSP() and Enable/DisableAP() after 
creating the hob, since smm elects its own BSP and all CPU will enter smm after 
receiving smi regardless of the StatusFlag. 

So the NumberOfProcessors, the ProcessorId and Location fields remain 
unchanged, the StatusFlag and NumberOfEnabledProcessors may be invalidated. I 
agree that we should document specific fields of the HOB may be invalidated 
between HOB production and HOB consumption. Will send V2 patch set to document 
this in the HOB definition head file.

Thanks,
Dun

-Original Message-
From: Laszlo Ersek  
Sent: Monday, November 13, 2023 10:33 PM
To: devel@edk2.groups.io; Tan, Dun 
Subject: Re: [edk2-devel] [PATCH 0/3] Move gMpInformationHobGuid from 
StandaloneMmPkg to UefiCpuPkg.

On 11/9/23 03:51, duntan wrote:
> Move gMpInformationHobGuid from StandaloneMmPkg to UefiCpuPkg.
> 
> Previously, the HOB is defined, created and consumed only in 
> StandaloneMmPkg. The HOB contains the number of processors and 
> EFI_PROCESSOR_INFORMATION structure. This is the same as the information that 
> PiSmmCpuDxeSmm uses EfiMpServiceProtocolGuid to get.
> 
> The incoming plan is to create gMpInformationHobGuid for both 
> StandaloneMm and legacy DXE_SMM in early phase(for example in 
> CpuMpPei). Then PiSmmCpuDxeSmm can consume the hob, which can simplify code 
> logic in PiSmmCpuDxeSmm driver.
> 
> So move this HOB definition to UefiCpuPkg in this patch series.
> 
> Dun Tan (3):
>   UefiCpuPkg: Create MpInformation.h in UefiCpuPkg
>   StandaloneMmPkg:Add UefiCpuPkg.dec in DependencyCheck
>   StandaloneMmPkg:Remove MpInformation.h
> 
>  StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf  
>  | 1 +
>  
> StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
>  | 1 +
>  StandaloneMmPkg/StandaloneMmPkg.ci.yaml  
>  | 3 ++-
>  StandaloneMmPkg/StandaloneMmPkg.dec  
>  | 1 -
>  {StandaloneMmPkg => UefiCpuPkg}/Include/Guid/MpInformation.h 
>  | 2 +-
>  UefiCpuPkg/UefiCpuPkg.dec
>  | 3 +++
>  6 files changed, 8 insertions(+), 3 deletions(-)  rename 
> {StandaloneMmPkg => UefiCpuPkg}/Include/Guid/MpInformation.h (88%)
> 

From a quick skim, the series looks OK to me, and I also agree that the above 
two MP services calls (GetNumberOfProcessors, GetProcessorInfo) seem to be the 
only ones in PiSmmCpuDxeSmm.

However, what about the following scenario:

- in the PI phase (or HOB producer phase), the HOB is produced

- in the DXE phase, a platform DXE driver uses EFI_MP_SERVICES_PROTOCOL to 
change some aspect of the processors in the system. For example, it calls 
SwitchBSP, or calls EnableDisableAP.

- Then the information in the HOB will be stale, once PiSmmCpuDxeSmm consumes 
it. The EFI_PROCESSOR_INFORMATION.StatusFlag field carries information both 
about the CPU in question being BSP vs. AP, and about the CPU being enabled or 
disabled. So either of SwitchBSP() and
EnableDisableAP() can invalidate the StatusFlag field for a given ProcessorId.

I don't know how StandaloneMmPkg currently deals with this scenario, and I also 
don't know whether StatusFlag actually matters to PiSmmCpuSmmDxe.
Apparently, it doesn't. So technically, the replacement of the protocol with 
the HOB should be fine, but for the general case, we should document somehow 
that specific fields of the HOB may be invalidated between HOB production and 
HOB consumption. If platform code is required to prevent that (i.e., the 
platform is responsible for not calling
SwitchBSP() or EnableDisableAP()), then that requirement should also be 
documented.

Acked-by: Laszlo Ersek 


Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111222): https://edk2.groups.io/g/devel/message/111222
Mute This Topic: https://groups.io/mt/102479007/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] .pytool/UncrustifyCheck: Update to 73.0.8

[Sean]

Reviewed-by: Sean Brogan 


From: devel@edk2.groups.io  on behalf of Michael Kubacki 

Sent: Tuesday, November 14, 2023 12:22:24 PM
To: devel@edk2.groups.io 
Cc: Sean Brogan ; Michael Kubacki 
; Michael D Kinney ; 
Liming Gao 
Subject: [edk2-devel] [PATCH v1 1/3] .pytool/UncrustifyCheck: Update to 73.0.8

From: Michael Kubacki 

Updates to the latest release.

- Includes a fix for preventing endless indentation in struct
  assignment.
- Include Windows Arm, Linux Arm, and Mac OS builds.

Cc: Sean Brogan 
Cc: Michael Kubacki 
Cc: Michael D Kinney 
Cc: Liming Gao 
Signed-off-by: Michael Kubacki 
---
 .pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml 
b/.pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml
index d8c22403b4b1..74f3ffe41acf 100644
--- a/.pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml
+++ b/.pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml
@@ -10,7 +10,7 @@
   "type": "nuget",
   "name": "mu-uncrustify-release",
   "source": 
"https://pkgs.dev.azure.com/projectmu/Uncrustify/_packaging/mu_uncrustify/nuget/v3/index.json;,
-  "version": "73.0.3",
+  "version": "73.0.8",
   "flags": ["set_shell_var", "host_specific"],
   "var_name": "UNCRUSTIFY_CI_PATH"
 }
--
2.42.0.windows.2



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111216): https://edk2.groups.io/g/devel/message/111216
Mute This Topic: https://groups.io/mt/102591691/1686594
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [spbro...@outlook.com]
-=-=-=-=-=-=




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

Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow

[Mike Maslenkin]

On Tue, Nov 14, 2023 at 9:57 PM Igor Kulchytskyy via groups.io
 wrote:
>
> Hi Leif,
> Please see my comments below.
> Thank you,
> Igor
>
>
> -Original Message-
> From: Leif Lindholm 
> Sent: Tuesday, November 14, 2023 12:26 PM
> To: devel@edk2.groups.io; Igor Kulchytskyy 
> Cc: Abner Chang ; Nickle Wang 
> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: 
> RedfishDiscoverDxe: Optimize the Redfish Discover flow
>
>
> **CAUTION: The e-mail below is from an external source. Please exercise 
> caution before opening attachments, clicking links, or following guidance.**
>
> On 2023-11-14 14:28, Igor Kulchytskyy via groups.io wrote:
> > Filter out the network interfaces which are not supported by
> > Redfish Host Interface.
> >
> > Cc: Abner Chang 
> > Cc: Nickle Wang 
> > Signed-off-by: Igor Kulchytskyy 
> > ---
> >   RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c  | 163 
> > ++--
> >   RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h |   6 +
> >   2 files changed, 120 insertions(+), 49 deletions(-)
> >
> > diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c 
> > b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > index 0f622e05a9..ae83cd3c97 100644
> > --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
>
>
> > @@ -1601,10 +1681,22 @@ BuildupNetworkInterface (
> > EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL   *RestExInstance;
> > EFI_TPL  OldTpl;
> > BOOLEAN  
> > NewNetworkInterfaceInstalled;
> > +  UINT8IpType;
> > +  UINTNListCount;
> >
> > +  ListCount= (sizeof (gRequiredProtocol) / sizeof 
> > (REDFISH_DISCOVER_REQUIRED_PROTOCOL));
> > NewNetworkInterfaceInstalled = FALSE;
> > Index= 0;
> > -  do {
> > +
> > +  // Get IP Type to filter out unnecessary network protocol if possible
> > +  IpType = GetHiIpProtocolType ();
> > +
> > +  for (Index = 0; Index < ListCount; Index++) {
> > +// Check IP Type and skip an unnecessary network protocol if does not 
> > match
> > +if (IS_TCP4_MATCH (IpType) || IS_TCP6_MATCH (IpType)) {
>
> The logic of these macros is inverted compared to their names, though.
>
> You want this test to read
>if (!IS_TCP4_MATCH (IpType) && !IS_TCP6_MATCH (IpType)) {
>
> > +  continue;
> > +}
> > +
> >   Status = gBS->OpenProtocol (
> >   // Already in list?
> >   ControllerHandle,
>
> > diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h 
> > b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> > index 01454acc1d..3093eea0d5 100644
> > --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> > +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> > @@ -39,6 +39,12 @@
> >   #define REDFISH_DISCOVER_VERSION0x0001
> >   #define EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_TPL  TPL_NOTIFY
> >
> > +#define MAC_COMPARE(ThisNetworkInterface, TargetNetworkInterface)  
> > (CompareMem ((VOID *)>MacAddress, 
> > >MacAddress, ThisNetworkInterface->HwAddressSize))
> > +#define VALID_TCP6(TargetNetworkInterface, ThisNetworkInterface)   
> > (TargetNetworkInterface->IsIpv6 && 
> > (ThisNetworkInterface->NetworkProtocolType == ProtocolTypeTcp6))
> > +#define VALID_TCP4(TargetNetworkInterface, ThisNetworkInterface)   
> > (!TargetNetworkInterface->IsIpv6 && 
> > (ThisNetworkInterface->NetworkProtocolType == ProtocolTypeTcp4))
> > +#define IS_TCP4_MATCH(IpType)  
> > ((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp4) && (IpType != 
> > REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4))
> > +#define IS_TCP6_MATCH(IpType)  
> > ((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp6) && (IpType != 
> > REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6))
>
> And these macros to test for ==, not !=
>
>
> Igor: First version tested "==", but we agreed that it may not work if we 
> have a wrong value of IpType.
>
> Otherwise the code reads like it does the opposite of what it does.
>
> (You could also keep the logic and call the macros IS_TCP#_MISMATCH, but
> that feels a bit convoluted.)
>
> Igor: I would prefer to go with IS_TCP#_MISMATCH names.
>
> Regards,
>
> Leif

Sorry, could I add my 2 cents?

For me all newly added defines looks bad, just because those
implicitly use reference to a global variable
plus local variable state (i.e  current cycle index).

Could we rewrite code in a simple and straight forward manner, similar to:

if (IpType == REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN) {
  // The protocol type is not specified in SMBIOS table type 42h
  return EFI_UNSUPPORTED;
}

for (Index = 0; Index < ListCount; 

Re: [edk2-devel] [edk2-platforms PATCH v2 1/1] Silicon/Marvell: Retructure package

[Marcin Wojtas via groups.io]

Hi Narinder,

Thanks for the patch.

2 comments: s/Retructure/Restructure/ from the commit title.

czw., 2 lis 2023 o 04:47 Narinder Dhillon  napisał(a):
>
> From: Narinder Dhillon 
>
> Current Marvell package structure makes it difficult to add new silicon
> packages that reuse common elements without creating nested DEC files.
>
> This patch creates a new MarvellSiliconPkg folder and moves the current
> common elements inside it.
>
> Also gMarvellTokenSpaceGuid has been renamed to
> gMarvellSiliconTokenSpaceGuid to align with new package name.
>
> Signed-off-by: Narinder Dhillon 
> ---
>  .../Marvell/Armada70x0Db/Armada70x0Db.dsc | 108 -
>  .../Armada70x0DbBoardDescLib.inf  |   2 +-
>  .../NonDiscoverableInitLib.inf|   2 +-
>  .../Marvell/Armada80x0Db/Armada80x0Db.dsc | 133 ++-
>  .../Armada80x0DbBoardDescLib.inf  |   2 +-
>  .../NonDiscoverableInitLib.inf|   2 +-
>  .../Cn9130DbABoardDescLib.inf |   2 +-
>  .../Cn9132DbABoardDescLib.inf |   2 +-
>  Platform/Marvell/Cn913xDb/Cn9130DbA.dsc.inc   | 100 -
>  Platform/Marvell/Cn913xDb/Cn9131DbA.dsc.inc   |  66 +++---
>  Platform/Marvell/Cn913xDb/Cn9132DbA.dsc.inc   |  66 +++---
>  Platform/Marvell/Cn913xDb/Cn913xDbA.dsc   |   8 +-
>  .../NonDiscoverableInitLib.inf|   2 +-
>  .../Armada80x0McBin/Armada80x0McBin.dsc   | 116 +-
>  .../Armada80x0McBinBoardDescLib.inf   |   2 +-
>  .../NonDiscoverableInitLib.inf|   2 +-
>  .../BoardDescriptionLib.inf   |   2 +-
>  .../Cn913xCEx7Eval/Cn9130Eval.dsc.inc |  40 ++--
>  .../Cn913xCEx7Eval/Cn9131Eval.dsc.inc |  56 ++---
>  .../Cn913xCEx7Eval/Cn9132Eval.dsc.inc |  56 ++---
>  .../Cn913xCEx7Eval/Cn913xCEx7.dsc.inc |  60 ++---
>  .../Cn913xCEx7Eval/Cn913xCEx7Eval.dsc |   6 +-
>  .../NonDiscoverableInitLib.inf|   2 +-
>  .../Applications/EepromCmd/EepromCmd.inf  |   2 +-
>  .../Applications/FirmwareUpdate/FUpdate.inf   |   6 +-
>  .../Applications/SpiTool/SpiFlashCmd.inf  |   6 +-
>  .../Armada7k8k/AcpiTables/Armada70x0Db.inf|   2 +-
>  .../Armada7k8k/AcpiTables/Armada80x0Db.inf|   2 +-
>  .../Armada7k8k/AcpiTables/Armada80x0McBin.inf |   2 +-
>  Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc |  22 +-
>  .../Armada7k8kRngDxe/Armada7k8kRngDxe.inf |   4 +-
>  .../Drivers/PlatInitDxe/PlatInitDxe.inf   |   6 +-
>  .../PlatformFlashAccessLib.inf|   6 +-
>  .../Library/Armada7k8kLib/Armada7k8kLib.inf   |   4 +-
>  .../Armada7k8kMemoryInitPeiLib.inf|  14 +-
>  .../PciHostBridgeLib.inf  |   2 +-
>  .../Armada7k8kPciSegmentLib/PciSegmentLib.inf |   2 +-
>  .../Armada7k8kSampleAtResetLib.inf|   2 +-
>  .../Armada7k8kSoCDescLib.inf  |   4 +-
>  .../RealTimeClockLib/RealTimeClockLib.inf |   4 +-
>  .../Marvell/Documentation/PortingGuide.txt| 114 +-
>  .../Drivers/BoardDesc/MvBoardDescDxe.inf  |  18 +-
>  .../Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf  |   2 +-
>  .../Gpio/MvPca95xxDxe/MvPca95xxDxe.inf|   2 +-
>  .../Drivers/I2c/MvEepromDxe/MvEepromDxe.inf   |   6 +-
>  .../Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf |  14 +-
>  .../Drivers/Net/MvMdioDxe/MvMdioDxe.inf   |   2 +-
>  .../Marvell/Drivers/Net/MvPhyDxe/MvPhyDxe.inf |  12 +-
>  Silicon/Marvell/Drivers/Net/Pp2Dxe/Pp2Dxe.inf |  16 +-
>  .../NonDiscoverableDxe/NonDiscoverableDxe.inf |   2 +-
>  .../Drivers/SdMmc/XenonDxe/XenonDxe.inf   |   2 +-
>  .../SmbiosPlatformDxe/SmbiosPlatformDxe.inf   |  14 +-
>  .../Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf |   8 +-
>  .../Spi/MvSpiFlashDxe/MvSpiFlashDxe.inf   |   2 +-
>  .../Spi/MvSpiOrionDxe/MvSpiOrionDxe.inf   |   8 +-
>  .../Marvell/Library/ComPhyLib/ComPhyLib.inf   |  28 +--
>  Silicon/Marvell/Library/IcuLib/IcuLib.inf |   4 +-
>  Silicon/Marvell/Library/MppLib/MppLib.inf |  94 
>  .../Marvell/Library/MvGpioLib/MvGpioLib.inf   |   2 +-
>  .../Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf |   2 +-
>  Silicon/Marvell/Marvell.dec   | 208 --
>  .../Include/IndustryStandard/MvSmc.h  |   0
>  .../Include/Library/ArmadaBoardDescLib.h  |   0
>  .../Include/Library/ArmadaIcuLib.h|   0
>  .../Include/Library/ArmadaSoCDescLib.h|   0
>  .../Include/Library/MppLib.h  |   0
>  .../Include/Library/MvComPhyLib.h |   0
>  .../Include/Library/MvGpioLib.h   |   0
>  .../Include/Library/NonDiscoverableInitLib.h  |   0
>  .../Include/Library/SampleAtResetLib.h|   0
>  .../Include/Library/UtmiPhyLib.h  |   0
>  .../Include/Protocol/BoardDesc.h  |   0
>  .../Include/Protocol/Eeprom.h |   0
>  .../Include/Protocol/Mdio.h   |   0
>  .../Include/Protocol/MvI2c.h  |   0
>  

[edk2-devel] [PATCH v1 3/3] OvmfPkg: Format with Uncrustify 73.0.8

[Michael Kubacki]

From: Michael Kubacki 

Cc: Ard Biesheuvel 
Cc: Corvin Köhne 
Cc: Gerd Hoffmann 
Cc: Jiewen Yao 
Cc: Rebecca Cran 
Signed-off-by: Michael Kubacki 
---
 OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c |  4 
++--
 OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c | 24 
++--
 OvmfPkg/PlatformPei/MemTypeInfo.c  |  2 +-
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbInfo.c   |  6 
++---
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c 
b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
index 4fc715dc3681..c07e38966e36 100644
--- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
+++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
@@ -658,8 +658,8 @@ InitializeFvAndVariableStoreHeaders (
 
   // UINT32  Size;
   (
-   FixedPcdGet32 (PcdFlashNvStorageVariableSize) -
-   OFFSET_OF (FVB_FV_HDR_AND_VARS_TEMPLATE, VarHdr)
+FixedPcdGet32 (PcdFlashNvStorageVariableSize) -
+OFFSET_OF (FVB_FV_HDR_AND_VARS_TEMPLATE, VarHdr)
   ),
 
   // UINT8   Format;
diff --git 
a/OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c 
b/OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
index 3092a174bc51..0b388819 100644
--- a/OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
+++ b/OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
@@ -49,12 +49,12 @@ STATIC EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL
 STATIC CONST EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  mMmio64Configuration = {
   ACPI_ADDRESS_SPACE_DESCRIPTOR,   // Desc
   (UINT16)(// Len
-   sizeof 
(EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
-   OFFSET_OF (
- 
EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
- ResType
- )
-   ),
+sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
+OFFSET_OF (
+  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
+  ResType
+  )
+),
   ACPI_ADDRESS_SPACE_TYPE_MEM, // ResType
   0,   // GenFlag
   0,   // SpecificFlag
@@ -83,12 +83,12 @@ STATIC CONST EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  
mMmio64Configuration = {
 STATIC CONST EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  mOptionRomConfiguration =   {
   ACPI_ADDRESS_SPACE_DESCRIPTOR,   // Desc
   (UINT16)(// Len
-   sizeof 
(EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
-   OFFSET_OF (
- 
EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
- ResType
- )
-   ),
+sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
+OFFSET_OF (
+  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
+  ResType
+  )
+),
   ACPI_ADDRESS_SPACE_TYPE_MEM, // ResType
   0,   // GenFlag
   0,   // Disable option roms 
SpecificFlag
diff --git a/OvmfPkg/PlatformPei/MemTypeInfo.c 
b/OvmfPkg/PlatformPei/MemTypeInfo.c
index ea049b21cfc0..dfb1bc37a93d 100644
--- a/OvmfPkg/PlatformPei/MemTypeInfo.c
+++ b/OvmfPkg/PlatformPei/MemTypeInfo.c
@@ -196,7 +196,7 @@ OnReadOnlyVariable2Available (
 //
 STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR  mReadOnlyVariable2Notify = {
   (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH |
-   EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),  // Flags
+EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST), // Flags
   , // Guid
   OnReadOnlyVariable2Available  // Notify
 };
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbInfo.c 
b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbInfo.c
index ea5ce837119a..e38f03cae046 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbInfo.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbInfo.c
@@ -75,9 +75,9 @@ EFI_FVB_MEDIA_INFO  mPlatformFvbMediaInfo[] = {
   {
 {
   (FixedPcdGet32 (PcdFlashNvStorageVariableSize) +
-   FixedPcdGet32 (PcdFlashNvStorageFtwWorkingSize) +
-   FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize) +
-   FixedPcdGet32 (PcdOvmfFlashNvStorageEventLogSize)) /
+FixedPcdGet32 (PcdFlashNvStorageFtwWorkingSize) +
+FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize) +
+FixedPcdGet32 (PcdOvmfFlashNvStorageEventLogSize)) /
   FixedPcdGet32 (PcdOvmfFirmwareBlockSize),
   

[edk2-devel] [PATCH v1 2/3] EmulatorPkg: Format with Uncrustify 73.0.8

[Michael Kubacki]

From: Michael Kubacki 

Cc: Andrew Fish 
Cc: Ray Ni 
Signed-off-by: Michael Kubacki 
---
 EmulatorPkg/FvbServicesRuntimeDxe/FvbInfo.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/EmulatorPkg/FvbServicesRuntimeDxe/FvbInfo.c 
b/EmulatorPkg/FvbServicesRuntimeDxe/FvbInfo.c
index bf2f0ac9808c..2234d29def44 100644
--- a/EmulatorPkg/FvbServicesRuntimeDxe/FvbInfo.c
+++ b/EmulatorPkg/FvbServicesRuntimeDxe/FvbInfo.c
@@ -112,9 +112,9 @@ EFI_FVB_MEDIA_INFO  mPlatformFvbMediaInfo[] = {
   {
 {
   (FixedPcdGet32 (PcdFlashNvStorageVariableSize) + \
-   FixedPcdGet32 (PcdFlashNvStorageFtwWorkingSize) + \
-   FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize) + \
-   FixedPcdGet32 (PcdEmuFlashNvStorageEventLogSize)) / FixedPcdGet32 
(PcdEmuFirmwareBlockSize),
+FixedPcdGet32 (PcdFlashNvStorageFtwWorkingSize) + \
+FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize) + \
+FixedPcdGet32 (PcdEmuFlashNvStorageEventLogSize)) / FixedPcdGet32 
(PcdEmuFirmwareBlockSize),
   FixedPcdGet32 (PcdEmuFirmwareBlockSize),
 }
   }
-- 
2.42.0.windows.2



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

[edk2-devel] [PATCH v1 0/3] Update from Uncrustify 73.0.3 to 73.0.8

[Michael Kubacki]

From: Michael Kubacki 

This patch series updates to 73.0.8 Uncrustify release which brings
the following changes:

- A fix for preventing endless indentation in struct assignment.
  - Updates to code areas impacted by the change. This currently
updates 5 files in two packages - EmulatorPkg and OvmfPkg.
- Includes Windows Arm, Linux Arm, and macOS builds.

Note:

After this patch series is merged, I plan to follow up with a patch
to add a .git-blame-ignore-revs file that will include the commits
that only contain Uncrustify formatting changes. GitHub ignores
revisions specified in the file by default.

Cc: Andrew Fish 
Cc: Ard Biesheuvel 
Cc: Corvin Kohne 
Cc: Gerd Hoffmann 
Cc: Jiewen Yao 
Cc: Liming Gao 
Cc: Michael D Kinney 
Cc: Ray Ni 
Cc: Rebecca Cran 
Cc: Sean Brogan 

Michael Kubacki (3):
  .pytool/UncrustifyCheck: Update to 73.0.8
  EmulatorPkg: Format with Uncrustify 73.0.8
  OvmfPkg: Format with Uncrustify 73.0.8

 EmulatorPkg/FvbServicesRuntimeDxe/FvbInfo.c|  6 
++---
 OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c |  4 
++--
 OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c | 24 
++--
 OvmfPkg/PlatformPei/MemTypeInfo.c  |  2 +-
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbInfo.c   |  6 
++---
 .pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml |  2 +-
 6 files changed, 22 insertions(+), 22 deletions(-)

-- 
2.42.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111215): https://edk2.groups.io/g/devel/message/111215
Mute This Topic: https://groups.io/mt/102591690/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 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Michael Kubacki]

On 11/14/2023 11:21 AM, Michael D Kinney wrote:

Hi Ranbir,

First I want to recognize your efforts to collect Coverity issues and 
propose changes to address


them.

I still disagree with adding CpuDealLoop() for any static analysis issues.

There have been previous discussions about adding a PANIC() or FATAL() 
macro that would


perform platform specific actions if a condition is detected where the 
boot of the platform


can not continue.  A platform get to make the choice to log or reboot or 
hang, etc.  Not the


code that detected the condition.

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



After going through hundreds of edk2 static analysis findings, we found 
a small number of cases where an interface such as PanicLib was useful 
and recently added an implementation 
https://github.com/microsoft/mu_basecore/blob/release/202302/MdePkg/Include/Library/PanicLib.h.


For example, the return value from calls to MpInitLibWhoAmI() in 
exception related code often goes unchecked and it's been used there. 
Being able to separate the library instance implementation linked to a 
given module from a more broad library class like DebugLib for these 
cases has also been helpful.


Unfortunately, in order to fix some of these static analysis issues 
correctly, we are going


to have to identify the use of ASSERT() that really is a fatal condition 
that can not continue


and introduce an implementation approach that provides a platform 
handler and


satisfies the static analysis tools.

We also have to evaluate if a return error status with a DEBUG_ERROR 
message would be a better


choice than an ASSERT() that can be filtered out by build options.

Best regards,

Mike

*From:* devel@edk2.groups.io  *On Behalf Of 
*Ranbir Singh

*Sent:* Tuesday, November 14, 2023 7:08 AM
*To:* Laszlo Ersek 
*Cc:* devel@edk2.groups.io; Ni, Ray ; Veeresh Sangolli 

*Subject:* Re: [edk2-devel] [PATCH v2 4/5] 
MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue


On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek > wrote:


On 11/10/23 07:11, Ranbir Singh wrote:
 > EFI_NOT_READY was listed as one of the error return values in the
 > function header of StartPciDevices(). So, I considered it here.
 >
 > Maybe we can go by a dual approach, including CpuDeadLoop() in
 > StartPciDevices() as well as add return value check at the call
site in
 > PciBusDriverBindingStart().

I don't think this makes much sense, given that execution is not
supposed to continue past CpuDeadLoop(), so the new error check would
not be reached.

I think two options are realistic:

- replace the assert() with a CpuDeadLoop() -- this is my preference

- keep the assert, add the "if", and in the caller, handle the
EFI_NOT_READY error. This is workable too. (Keeping the assert is goog
because it shows that we don't expect the "if" to fire.)

Anyway, now that we have no way to verify the change against Coverity, I
don't know if this patch is worth the churn. :( As I said, this ASSERT()
is one of those few justified ones in edk2 core that can indeed never
fail, IMO.

Laszlo

See, Does the following change look acceptable to you ?

     ASSERT (RootBridge != NULL);
+  if (RootBridge == NULL) {

+    CpuDeadLoop ();
+    return EFI_NOT_READY;
+  }

+

which retains the existing assert, adds the NULL pointer check and 
includes CpuDeadLoop () within the if block. As a result of CpuDeadLoop 
(), the return statement afterwards will never reach execution (=> no 
need to handle this return value at the call sites), but will satisfy 
static analysis tools as the "RootBridge" dereference scenario doesn't 
arise thereafter.



 >
 > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek mailto:ler...@redhat.com>
 > >> wrote:
 >
 >     On 11/7/23 07:19, Ranbir Singh wrote:
 >     > From: Ranbir Singh mailto:ranbir.sin...@dell.com>>
 >     >
 >     > The function StartPciDevices has a check
 >     >
 >     >     ASSERT (RootBridge != NULL);
 >     >
 >     > but this comes into play only in DEBUG mode. In Release
mode, there
 >     > is no handling if the RootBridge value is NULL and the code
proceeds
 >     > to unconditionally dereference "RootBridge" which will lead
to CRASH.
 >     >
 >     > Hence, for safety add NULL pointer checks always and return
 >     > EFI_NOT_READY if RootBridge value is NULL which is one of
the return
 >     > values as mentioned in the function description header.
 >     >
 >     > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239

 >     >
 >     >
 >     > 

Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow

[Igor Kulchytskyy via groups.io]

Hi Leif,
Please see my comments below.
Thank you,
Igor


-Original Message-
From: Leif Lindholm 
Sent: Tuesday, November 14, 2023 12:26 PM
To: devel@edk2.groups.io; Igor Kulchytskyy 
Cc: Abner Chang ; Nickle Wang 
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: 
RedfishDiscoverDxe: Optimize the Redfish Discover flow


**CAUTION: The e-mail below is from an external source. Please exercise caution 
before opening attachments, clicking links, or following guidance.**

On 2023-11-14 14:28, Igor Kulchytskyy via groups.io wrote:
> Filter out the network interfaces which are not supported by
> Redfish Host Interface.
>
> Cc: Abner Chang 
> Cc: Nickle Wang 
> Signed-off-by: Igor Kulchytskyy 
> ---
>   RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c  | 163 
> ++--
>   RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h |   6 +
>   2 files changed, 120 insertions(+), 49 deletions(-)
>
> diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c 
> b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> index 0f622e05a9..ae83cd3c97 100644
> --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c


> @@ -1601,10 +1681,22 @@ BuildupNetworkInterface (
> EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL   *RestExInstance;
> EFI_TPL  OldTpl;
> BOOLEAN  
> NewNetworkInterfaceInstalled;
> +  UINT8IpType;
> +  UINTNListCount;
>
> +  ListCount= (sizeof (gRequiredProtocol) / sizeof 
> (REDFISH_DISCOVER_REQUIRED_PROTOCOL));
> NewNetworkInterfaceInstalled = FALSE;
> Index= 0;
> -  do {
> +
> +  // Get IP Type to filter out unnecessary network protocol if possible
> +  IpType = GetHiIpProtocolType ();
> +
> +  for (Index = 0; Index < ListCount; Index++) {
> +// Check IP Type and skip an unnecessary network protocol if does not 
> match
> +if (IS_TCP4_MATCH (IpType) || IS_TCP6_MATCH (IpType)) {

The logic of these macros is inverted compared to their names, though.

You want this test to read
   if (!IS_TCP4_MATCH (IpType) && !IS_TCP6_MATCH (IpType)) {

> +  continue;
> +}
> +
>   Status = gBS->OpenProtocol (
>   // Already in list?
>   ControllerHandle,

> diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h 
> b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> index 01454acc1d..3093eea0d5 100644
> --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
> @@ -39,6 +39,12 @@
>   #define REDFISH_DISCOVER_VERSION0x0001
>   #define EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_TPL  TPL_NOTIFY
>
> +#define MAC_COMPARE(ThisNetworkInterface, TargetNetworkInterface)  
> (CompareMem ((VOID *)>MacAddress, 
> >MacAddress, ThisNetworkInterface->HwAddressSize))
> +#define VALID_TCP6(TargetNetworkInterface, ThisNetworkInterface)   
> (TargetNetworkInterface->IsIpv6 && (ThisNetworkInterface->NetworkProtocolType 
> == ProtocolTypeTcp6))
> +#define VALID_TCP4(TargetNetworkInterface, ThisNetworkInterface)   
> (!TargetNetworkInterface->IsIpv6 && 
> (ThisNetworkInterface->NetworkProtocolType == ProtocolTypeTcp4))
> +#define IS_TCP4_MATCH(IpType)  
> ((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp4) && (IpType != 
> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4))
> +#define IS_TCP6_MATCH(IpType)  
> ((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp6) && (IpType != 
> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6))

And these macros to test for ==, not !=


Igor: First version tested "==", but we agreed that it may not work if we have 
a wrong value of IpType.

Otherwise the code reads like it does the opposite of what it does.

(You could also keep the logic and call the macros IS_TCP#_MISMATCH, but
that feels a bit convoluted.)

Igor: I would prefer to go with IS_TCP#_MISMATCH names.

Regards,

Leif

> +
>   //
>   // GUID definitions
>   //
> --
> 2.37.1.windows.1
> -The information contained in this message may be confidential and 
> proprietary to American Megatrends (AMI). This communication is intended to 
> be read only by the individual or entity to whom it is addressed or by their 
> designee. If the reader of this message is not the intended recipient, you 
> are on notice that any distribution of this message, in any form, is strictly 
> prohibited. Please promptly notify the sender by reply e-mail or by telephone 
> at 770-246-8600, and then delete or destroy all copies of the transmission.
>
>
> 
>
>

-The information contained in this message may be confidential and proprietary 
to American Megatrends (AMI). This communication is 

Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow

[Leif Lindholm]

On 2023-11-14 14:28, Igor Kulchytskyy via groups.io wrote:

Filter out the network interfaces which are not supported by
Redfish Host Interface.

Cc: Abner Chang 
Cc: Nickle Wang 
Signed-off-by: Igor Kulchytskyy 
---
  RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c  | 163 
++--
  RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h |   6 +
  2 files changed, 120 insertions(+), 49 deletions(-)

diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c 
b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
index 0f622e05a9..ae83cd3c97 100644
--- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
+++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c




@@ -1601,10 +1681,22 @@ BuildupNetworkInterface (
EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL   *RestExInstance;
EFI_TPL  OldTpl;
BOOLEAN  
NewNetworkInterfaceInstalled;
+  UINT8IpType;
+  UINTNListCount;

+  ListCount= (sizeof (gRequiredProtocol) / sizeof 
(REDFISH_DISCOVER_REQUIRED_PROTOCOL));
NewNetworkInterfaceInstalled = FALSE;
Index= 0;
-  do {
+
+  // Get IP Type to filter out unnecessary network protocol if possible
+  IpType = GetHiIpProtocolType ();
+
+  for (Index = 0; Index < ListCount; Index++) {
+// Check IP Type and skip an unnecessary network protocol if does not match
+if (IS_TCP4_MATCH (IpType) || IS_TCP6_MATCH (IpType)) {


The logic of these macros is inverted compared to their names, though.

You want this test to read
  if (!IS_TCP4_MATCH (IpType) && !IS_TCP6_MATCH (IpType)) {


+  continue;
+}
+
  Status = gBS->OpenProtocol (
  // Already in list?
  ControllerHandle,



diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h 
b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
index 01454acc1d..3093eea0d5 100644
--- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
+++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h
@@ -39,6 +39,12 @@
  #define REDFISH_DISCOVER_VERSION0x0001
  #define EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_TPL  TPL_NOTIFY

+#define MAC_COMPARE(ThisNetworkInterface, TargetNetworkInterface)  (CompareMem ((VOID 
*)>MacAddress, >MacAddress, 
ThisNetworkInterface->HwAddressSize))
+#define VALID_TCP6(TargetNetworkInterface, ThisNetworkInterface)   
(TargetNetworkInterface->IsIpv6 && (ThisNetworkInterface->NetworkProtocolType 
== ProtocolTypeTcp6))
+#define VALID_TCP4(TargetNetworkInterface, ThisNetworkInterface)   
(!TargetNetworkInterface->IsIpv6 && (ThisNetworkInterface->NetworkProtocolType 
== ProtocolTypeTcp4))
+#define IS_TCP4_MATCH(IpType)  
((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp4) && (IpType != 
REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4))
+#define IS_TCP6_MATCH(IpType)  
((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp6) && (IpType != 
REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6))


And these macros to test for ==, not !=

Otherwise the code reads like it does the opposite of what it does.

(You could also keep the logic and call the macros IS_TCP#_MISMATCH, but 
that feels a bit convoluted.)


Regards,

Leif


+
  //
  // GUID definitions
  //
--
2.37.1.windows.1
-The information contained in this message may be confidential and proprietary 
to American Megatrends (AMI). This communication is intended to be read only by 
the individual or entity to whom it is addressed or by their designee. If the 
reader of this message is not the intended recipient, you are on notice that 
any distribution of this message, in any form, is strictly prohibited. Please 
promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and 
then delete or destroy all copies of the transmission.









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

Re: [edk2-devel] [PATCH] UefiCpuPkg: Use NonSmm BSP as BSP if BSP election is not enabled.

[Laszlo Ersek]

On 11/14/23 17:53, Laszlo Ersek wrote:

> Second, "SMM_DISPATCHER_MP_SYNC_DATA.BspIndex" has type "(volatile)
> UINT32", but WhoAmI() writes an UINTN. On IA32, you're going to corrupt
> memory.

sorry, it's on X64 where you are going to corrupt memory (the UINTN
write is a UINT64 write, but the field to accept it is only UINT32)

Laszlo



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

Re: [edk2-devel] [PATCH] UefiCpuPkg: Use NonSmm BSP as BSP if BSP election is not enabled.

[Laszlo Ersek]

Small patch, but I have several comments :)

On 11/14/23 03:08, Zhiguang Liu wrote:
> Currently, if BSP election is not enabled, will use Core0 as SMM BSP,
> however, Core0 does not always have the highest performance core.
> So, we can used NonSmm BSP as default BSP.

(1) Please consider mentioning in the commit message that the change
from this patch will take effect in SmiRendezvous().

(2) Please put "UefiCpuPkg/PiSmmCpuDxeSmm: ..." in the subject.

>
> Cc: Ray Ni 
> Cc: Rahul Kumar 
> Cc: Gerd Hoffmann 
> Signed-off-by: Zhiguang Liu 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c  | 10 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 48 +++---
>  2 files changed, 34 insertions(+), 24 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 25d058c5b9..a4f83bb122 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -29,6 +29,8 @@ MM_COMPLETIONmSmmStartupThisApToken;
>  //
>  UINT32  *mPackageFirstThreadIndex = NULL;
>
> +extern EFI_MP_SERVICES_PROTOCOL  *mMpServices;
> +
>  /**
>Performs an atomic compare exchange operation to get semaphore.
>The compare exchange operation must be performed using
> @@ -1953,6 +1955,14 @@ InitializeMpSyncData (
>// Enable BSP election by setting BspIndex to -1
>//
>mSmmMpSyncData->BspIndex = (UINT32)-1;
> +} else {
> +  //
> +  // Use NonSMM BSP as SMM BSP
> +  //
> +  ASSERT (mMpServices != NULL);
> +  if (mMpServices != NULL) {
> +mMpServices->WhoAmI (mMpServices, (UINTN 
> *)>BspIndex);
> +  }
>  }
>
>  mSmmMpSyncData->EffectiveSyncMode = mCpuSmmSyncMode;


(3) This branch cannot be tested on OVMF, due to commit 43df61878d94
("OvmfPkg: enable SMM Monarch Election in PiSmmCpuDxeSmm", 2020-03-04).

That's not a problem with the patch of course, just saying that I can't
offer regression testing.


(4) Not checking the return value of WhoAmI() is actually valid here.
Per PI spec, WhoAmI() can only fail if we pass a null pointer for
"ProcessorNumber" (and we don't do that here).

Not sure about static analysis tools though. If they complain, we might
want to cast the return value to (VOID) explicitly:

  (VOID)mMpServices->WhoAmI (...);

Not needed unless those tools complain, of course.


(5) Passing the following argument for the "ProcessorNumber" parameter

  (UINTN *)>BspIndex

is undefined behavior, for two reasons.

First, "SMM_DISPATCHER_MP_SYNC_DATA.BspIndex" is volatile-qualified, but
the access here (or more precisely, inside WhoAmI()) happens through an
lvalue that doesn't necessarily have volatile-qualified type. That's UB.

Second, "SMM_DISPATCHER_MP_SYNC_DATA.BspIndex" has type "(volatile)
UINT32", but WhoAmI() writes an UINTN. On IA32, you're going to corrupt
memory.

Therefore you should to do this:

  UINTN DxeBspNumber;

  MpServices->WhoAmI (mMpServices, );
  if (DxeBspNumber <= MAX_UINT32) {
mSmmMpSyncData->BspIndex = (UINT32)DxeBspNumber;
  }

In other words, use a local variable of the right type, and range check
it. If the range check fails, then just fall back to the current
behavior (leave BspIndex at zero). Otherwise, employ an explicit
assignment, including casting. This will in particular keep "volatile"
happy (and there can't be any overflow either, after the range check).

One more comment below:

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index 1d022a7051..18c77c59e6 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -128,7 +128,8 @@ SPIN_LOCK  *mConfigSmmCodeAccessCheckLock = NULL;
>  EFI_SMRAM_DESCRIPTOR  *mSmmCpuSmramRanges;
>  UINTN mSmmCpuSmramRangeCount;
>
> -UINT8  mPhysicalAddressBits;
> +UINT8 mPhysicalAddressBits;
> +EFI_MP_SERVICES_PROTOCOL  *mMpServices;
>
>  //
>  // Control register contents saved for SMM S3 resume state initialization.
> @@ -603,26 +604,25 @@ PiCpuSmmEntry (
>IN EFI_SYSTEM_TABLE  *SystemTable
>)
>  {
> -  EFI_STATUSStatus;
> -  EFI_MP_SERVICES_PROTOCOL  *MpServices;
> -  UINTN NumberOfEnabledProcessors;
> -  UINTN Index;
> -  VOID  *Buffer;
> -  UINTN BufferPages;
> -  UINTN TileCodeSize;
> -  UINTN TileDataSize;
> -  UINTN TileSize;
> -  UINT8 *Stacks;
> -  VOID  *Registration;
> -  UINT32RegEax;
> -  UINT32RegEbx;
> -  UINT32RegEcx;
> -  UINT32RegEdx;
> -  UINTN FamilyId;
> -  UINTN ModelId;
> -  UINT32Cr3;
> -  EFI_HOB_GUID_TYPE *GuidHob;
> -  SMM_BASE_HOB_DATA 

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Ranbir Singh]

On Tue, Nov 14, 2023 at 9:51 PM Kinney, Michael D <
michael.d.kin...@intel.com> wrote:

> Hi Ranbir,
>
>
>
> First I want to recognize your efforts to collect Coverity issues and
> propose changes to address
>
> them.
>

Thanks Mike.


>
>
> I still disagree with adding CpuDealLoop() for any static analysis issues.
>

A bit of correction here.
CpuDeadLoop() is not exactly an addition for static analysis issues. For
that, the NULL pointer check and return statement in the if block are
sufficient.
However, CpuDeadLoop() is added as suggested by Laszlo to introduce a hang
behaviour in RELEASE builds as well when the situation is anyway not safe
to progress normally. That said, it may not still be required at every
point and hence needs to be assessed on a case to case basis.


>
> There have been previous discussions about adding a PANIC() or FATAL()
> macro that would
>
> perform platform specific actions if a condition is detected where the
> boot of the platform
>
> can not continue.  A platform get to make the choice to log or reboot or
> hang, etc.  Not the
>
> code that detected the condition.
>
>
>
> https://edk2.groups.io/g/devel/message/86597
>
>
>
> Unfortunately, in order to fix some of these static analysis issues
> correctly, we are going
>
> to have to identify the use of ASSERT() that really is a fatal condition
> that can not continue
>
> and introduce an implementation approach that provides a platform handler
> and
>
> satisfies the static analysis tools.
>
>
>
> We also have to evaluate if a return error status with a DEBUG_ERROR
> message would be a better
>
> choice than an ASSERT() that can be filtered out by build options.
>

Note that the existing ASSERT will give a DEBUG message even when
CpuDeadLoop is added in the code later.


>
>
> Best regards,
>
>
>
> Mike
>
>
>

Generally speaking, there now seems to be different views coming from you
and Laszlo. We might have to wait for some sort of agreement to be reached.


>
>
> *From:* devel@edk2.groups.io  * On Behalf Of *Ranbir
> Singh
> *Sent:* Tuesday, November 14, 2023 7:08 AM
> *To:* Laszlo Ersek 
> *Cc:* devel@edk2.groups.io; Ni, Ray ; Veeresh Sangolli <
> veeresh.sango...@dellteam.com>
> *Subject:* Re: [edk2-devel] [PATCH v2 4/5]
> MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue
>
>
>
>
>
>
>
> On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek  wrote:
>
> On 11/10/23 07:11, Ranbir Singh wrote:
> > EFI_NOT_READY was listed as one of the error return values in the
> > function header of StartPciDevices(). So, I considered it here.
> >
> > Maybe we can go by a dual approach, including CpuDeadLoop() in
> > StartPciDevices() as well as add return value check at the call site in
> > PciBusDriverBindingStart().
>
> I don't think this makes much sense, given that execution is not
> supposed to continue past CpuDeadLoop(), so the new error check would
> not be reached.
>
> I think two options are realistic:
>
> - replace the assert() with a CpuDeadLoop() -- this is my preference
>
> - keep the assert, add the "if", and in the caller, handle the
> EFI_NOT_READY error. This is workable too. (Keeping the assert is goog
> because it shows that we don't expect the "if" to fire.)
>
> Anyway, now that we have no way to verify the change against Coverity, I
> don't know if this patch is worth the churn. :( As I said, this ASSERT()
> is one of those few justified ones in edk2 core that can indeed never
> fail, IMO.
>
> Laszlo
>
>
>
> See, Does the following change look acceptable to you ?
>
>
>
> ASSERT (RootBridge != NULL);
> +  if (RootBridge == NULL) {
>
> +CpuDeadLoop ();
> +return EFI_NOT_READY;
> +  }
>
> +
>
>
>
> which retains the existing assert, adds the NULL pointer check and
> includes CpuDeadLoop () within the if block. As a result of CpuDeadLoop (),
> the return statement afterwards will never reach execution (=> no need to
> handle this return value at the call sites), but will satisfy static
> analysis tools as the "RootBridge" dereference scenario doesn't arise
> thereafter.
>
>
>
>
> >
> > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek  > > wrote:
> >
> > On 11/7/23 07:19, Ranbir Singh wrote:
> > > From: Ranbir Singh 
> > >
> > > The function StartPciDevices has a check
> > >
> > > ASSERT (RootBridge != NULL);
> > >
> > > but this comes into play only in DEBUG mode. In Release mode, there
> > > is no handling if the RootBridge value is NULL and the code
> proceeds
> > > to unconditionally dereference "RootBridge" which will lead to
> CRASH.
> > >
> > > Hence, for safety add NULL pointer checks always and return
> > > EFI_NOT_READY if RootBridge value is NULL which is one of the
> return
> > > values as mentioned in the function description header.
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> > 
> > >
> > > Cc: Ray Ni 

Re: [edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues

[Ranbir Singh]

On Mon, Nov 13, 2023 at 9:42 PM Laszlo Ersek  wrote:

> On 11/10/23 05:07, Ranbir Singh wrote:
> > Options before us till now -
> >
> > 1. Add array overrun check and Debug statement before CpuDeadLoop within
>
> This would be useless.
>
> Your current patch implements the following pattern:
>
>   ASSERT (X);
>   if (!X) {
> CpuDeadLoop ();
>   }
>
> Option#1 would mean
>
>   ASSERT (X);
>   if (!X) {
> DEBUG ((DEBUG_ERROR, "%a: condition X failed\n", __func__));
> CpuDeadLoop ();
>   }
>
> Now compare the behavior of both code snippets.
>
> - In DEBUG and NOOPT builds, if X evaluated to FALSE, then the ASSERT()
> would fire. A DEBUG_ERROR message would be logged, including "X", the
> file name, and the line number. ASSERT() would then enter CpuDeadLoop()
> internally, or invoke CpuBreakpoint() -- dependent on platform PCD
> configuration. This applies to both snippets. In particular, the
> explicit DEBUG and CpuDeadLoop() would not be reached, in the second
> snippet. In other words, in DEBUG and NOOPT builds, the difference is
> unobservable.
>
> - In RELEASE builds, the ASSERT() is compiled out of both snippets.
> Furthermore, the explicit DEBUG is compiled out of the second snippet.
> That is, both code snippets would silently enter CpuDeadLoop(). In other
> words, the behavior of both snippets is undistinguishable in RELEASE
> builds too.
>
> In my opinion, the current patch is right.
>
> Reviewed-by: Laszlo Ersek 
>
>
> To elaborate on that more generally:
>
> Core edk2 code has so consistently and so broadly *abused* and *misused*
> ASSERT() for error checking, that now we fail to recognize an *actual
> valid use* of ASSERT() for what it is. For illustration, compare the
> following two code snippets:
>
> (a)
>
>   UINTN Val;
>
>   Val = 1 + 2;
>   ASSERT (Val == 3);
>
> (b)
>
>   VOID *Ptr;
>
>   Ptr = AllocatePool (1024);
>   ASSERT (Ptr != NULL);
>
> Snippet (a) is a *valid* use of an assert. It encapsulates a logical
> predicate about the program's state, based on algorithmic and
> mathematical reasoning. If the predicate turns out not to hold in
> practice, at runtime,that means the programmer has retroactively caught
> an issue in their own thinking, the algorithm is incorrectly designed or
> implemented, and the program's state is effectively unknown at this
> point (the internal invariant in question is broken), and so we must
> stop immediately, because we don't know what the program is going to do
> next. Given that what we thought about the program *thus far* has now
> turned out to be false.
>
> Snippet (b) is an abuse of assert. AllocatePool()'s result depends on
> the environment. Available memory, input data seen from the user or the
> disk or the network thus far, and a bunch of other things not under our
> control. Therefore, we must explicitly deal with AllocatePool() failing.
> Claiming that AllocatePool() will succeed is wrong because we generally
> cannot even *attempt* to prove it.
>
> In case (a) however, we can actually make a plausible attempt at
> *proving* the predicate. The assert is there just in case our proof is
> wrong.
>
> There's an immense difference between situations (a) and (b). I cannot
> emphasize that enough. Case (a) is a provable statement about the
> behavior of an algorithm. Case (b) is a *guess* at input data and
> environment.
>
> The problem with edk2 core is that it has so consistently abused
> ASSERT() for case (b) that now, when we occasionally see a *valid
> instance* of (a), we mistake it for (b).
>
> That's the case here. The ASSERT() seen in this patch is case (a). It's
> just that Coverity cannot prove it itself, because the predicate we
> assert is much more complicated than (1 + 2 == 3). But that doesn't
> change the fact that we have a proof for the invariant in question.
>
> Therefore, the solution is not to try to handle an impossible error
> gracefully. The solution is two-fold:
>
> - Tell coverity that we have a proof, in terms that it understands, to
> shut it up. The terminology for this is CpuDeadLoop(). We're willing to
> hang at runtime even in RELEASE builds, if the condition fails.
>
> - If, at runtime, our proof turns out to be wrong indeed, then we must
> stop immediately (because if 1+2 stops equalling 3, then we can't trust
> *anything else* that our program would do).
>
> (The more generic corollary of my last point, by the way, is that
> ASSERT()s should NEVER be compiled out of programs. And if we actually
> did that, like many other projects do, then this Coverity warning
> wouldn't even exist, in the first place, because Coverity would *always*
> see -- with the ASSERT() being there in all builds -- a range check on
> Index.
>
> Put differently, having to add a CpuDeadLoop() explicitly is just
> "damage control" for the self-inflicted damage of compiling out ASSERTs.)
>
> Laszlo
>
>
Thanks Laszlo for being immensely generous in writing in that much detail.
This must be beneficial to many.

Though you already 

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Michael D Kinney]

Hi Ranbir,

First I want to recognize your efforts to collect Coverity issues and propose 
changes to address
them.

I still disagree with adding CpuDealLoop() for any static analysis issues.

There have been previous discussions about adding a PANIC() or FATAL() macro 
that would
perform platform specific actions if a condition is detected where the boot of 
the platform
can not continue.  A platform get to make the choice to log or reboot or hang, 
etc.  Not the
code that detected the condition.

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

Unfortunately, in order to fix some of these static analysis issues correctly, 
we are going
to have to identify the use of ASSERT() that really is a fatal condition that 
can not continue
and introduce an implementation approach that provides a platform handler and
satisfies the static analysis tools.

We also have to evaluate if a return error status with a DEBUG_ERROR message 
would be a better
choice than an ASSERT() that can be filtered out by build options.

Best regards,

Mike


From: devel@edk2.groups.io  On Behalf Of Ranbir Singh
Sent: Tuesday, November 14, 2023 7:08 AM
To: Laszlo Ersek 
Cc: devel@edk2.groups.io; Ni, Ray ; Veeresh Sangolli 

Subject: Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix 
NULL_RETURNS Coverity issue



On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek 
mailto:ler...@redhat.com>> wrote:
On 11/10/23 07:11, Ranbir Singh wrote:
> EFI_NOT_READY was listed as one of the error return values in the
> function header of StartPciDevices(). So, I considered it here.
>
> Maybe we can go by a dual approach, including CpuDeadLoop() in
> StartPciDevices() as well as add return value check at the call site in
> PciBusDriverBindingStart().

I don't think this makes much sense, given that execution is not
supposed to continue past CpuDeadLoop(), so the new error check would
not be reached.

I think two options are realistic:

- replace the assert() with a CpuDeadLoop() -- this is my preference

- keep the assert, add the "if", and in the caller, handle the
EFI_NOT_READY error. This is workable too. (Keeping the assert is goog
because it shows that we don't expect the "if" to fire.)

Anyway, now that we have no way to verify the change against Coverity, I
don't know if this patch is worth the churn. :( As I said, this ASSERT()
is one of those few justified ones in edk2 core that can indeed never
fail, IMO.

Laszlo

See, Does the following change look acceptable to you ?

ASSERT (RootBridge != NULL);
+  if (RootBridge == NULL) {
+CpuDeadLoop ();
+return EFI_NOT_READY;
+  }
+

which retains the existing assert, adds the NULL pointer check and includes 
CpuDeadLoop () within the if block. As a result of CpuDeadLoop (), the return 
statement afterwards will never reach execution (=> no need to handle this 
return value at the call sites), but will satisfy static analysis tools as the 
"RootBridge" dereference scenario doesn't arise thereafter.


>
> On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek 
> mailto:ler...@redhat.com>
> >> wrote:
>
> On 11/7/23 07:19, Ranbir Singh wrote:
> > From: Ranbir Singh 
> mailto:ranbir.sin...@dell.com>>
> >
> > The function StartPciDevices has a check
> >
> > ASSERT (RootBridge != NULL);
> >
> > but this comes into play only in DEBUG mode. In Release mode, there
> > is no handling if the RootBridge value is NULL and the code proceeds
> > to unconditionally dereference "RootBridge" which will lead to CRASH.
> >
> > Hence, for safety add NULL pointer checks always and return
> > EFI_NOT_READY if RootBridge value is NULL which is one of the return
> > values as mentioned in the function description header.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> 
> >
> > Cc: Ray Ni mailto:ray...@intel.com> 
> >>
> > Co-authored-by: Veeresh Sangolli 
> mailto:veeresh.sango...@dellteam.com>
> 
> >>
> > Signed-off-by: Ranbir Singh 
> mailto:ranbir.sin...@dell.com>>
> > Signed-off-by: Ranbir Singh 
> mailto:rsi...@ventanamicro.com>
> >>
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > index 581e9075ad41..3de80d98370e 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > @@ -772,7 +772,10 @@ StartPciDevices (
> >LIST_ENTRY *CurrentLink;
> >
> >RootBridge = 

Re: [edk2-devel] [PATCH v5 0/2] Fix and optimize the issue if IPv4 installed after RestEx

[Igor Kulchytskyy via groups.io]

Hi Leif,
I addressed your comments in that v5 patch.
Do you have any other concerns?
Thank you,
Igor

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Igor Kulchytskyy 
via groups.io
Sent: Tuesday, November 14, 2023 9:28 AM
To: devel@edk2.groups.io
Subject: [EXTERNAL] [edk2-devel] [PATCH v5 0/2] Fix and optimize the issue if 
IPv4 installed after RestEx


**CAUTION: The e-mail below is from an external source. Please exercise caution 
before opening attachments, clicking links, or following guidance.**

Igor Kulchytskyy (2):
  RedfishPkg: RedfishDiscoverDxe: Fix issue if IPv4 installed after
RestEx
  RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow

 .../RedfishDiscoverDxe/RedfishDiscoverDxe.c   | 132 +-
 .../RedfishDiscoverInternal.h |   6 +
 2 files changed, 105 insertions(+), 33 deletions(-)

--
2.37.1.windows.1
-The information contained in this message may be confidential and proprietary 
to American Megatrends (AMI). This communication is intended to be read only by 
the individual or entity to whom it is addressed or by their designee. If the 
reader of this message is not the intended recipient, you are on notice that 
any distribution of this message, in any form, is strictly prohibited. Please 
promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and 
then delete or destroy all copies of the transmission.





-The information contained in this message may be confidential and proprietary 
to American Megatrends (AMI). This communication is intended to be read only by 
the individual or entity to whom it is addressed or by their designee. If the 
reader of this message is not the intended recipient, you are on notice that 
any distribution of this message, in any form, is strictly prohibited. Please 
promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and 
then delete or destroy all copies of the transmission.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111206): https://edk2.groups.io/g/devel/message/111206
Mute This Topic: https://groups.io/mt/102584124/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/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity issue

[Ranbir Singh]

On Mon, Nov 13, 2023 at 10:03 PM Laszlo Ersek  wrote:

> On 11/9/23 18:39, Ranbir Singh wrote:
> > From: Ranbir Singh 
> >
> > The function SubmitResources has a switch-case code in which the
> > case ACPI_ADDRESS_SPACE_TYPE_MEM: which falls through to
> > case ACPI_ADDRESS_SPACE_TYPE_IO: to include additional common check.
> >
> > While this may be intentional, it may not be evident to any general code
> > reader/developer or static analyis tool why there is no break in between.
> >
> > SubmitResources function is supposed to handle only Mem or IO resources.
> > So, update the ResType parameter check reflecting that and re-model the
> > switch-case code in contention using just one if condition to further
> > handle other parameter checks specific to ACPI_ADDRESS_SPACE_TYPE_MEM.
> > This leads to mostly indentation level code changes. Few ASSERT's later
> > present are henceforth not required.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212
> >
> > Cc: Ray Ni 
> > Signed-off-by: Ranbir Singh 
> > ---
> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 60
> +---
> >  1 file changed, 26 insertions(+), 34 deletions(-)
>
> I have applied this patch locally, and displayed it with
>
>   git show -W -b
>
> Let me make comments on that:
>
> >  /**
> >
> >Submits the I/O and memory resource requirements for the specified
> PCI Root Bridge.
> >
> >@param This  The EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_
> PROTOCOL instance.
> > -  @param RootBridgeHandle  The PCI Root Bridge whose I/O and memory
> resource requirements.
> > +  @param RootBridgeHandle  The PCI Root Bridge whose I/O and memory
> resource requirements
> > are being submitted.
> >@param Configuration The pointer to the PCI I/O and PCI memory
> resource descriptor.
> >
> >@retval EFI_SUCCESSSucceed.
> >@retval EFI_INVALID_PARAMETER  Wrong parameters passed in.
> >  **/
> > @@ -1460,137 +1460,129 @@ EFIAPI
> >  SubmitResources (
> >IN EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL  *This,
> >IN EFI_HANDLERootBridgeHandle,
> >IN VOID  *Configuration
> >)
> >  {
> >LIST_ENTRY *Link;
> >PCI_HOST_BRIDGE_INSTANCE   *HostBridge;
> >PCI_ROOT_BRIDGE_INSTANCE   *RootBridge;
> >EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  *Descriptor;
> >PCI_RESOURCE_TYPE  Type;
> >
> >//
> >// Check the input parameter: Configuration
> >//
> >if (Configuration == NULL) {
> >  return EFI_INVALID_PARAMETER;
> >}
> >
> >HostBridge = PCI_HOST_BRIDGE_FROM_THIS (This);
> >for (Link = GetFirstNode (>RootBridges)
> > ; !IsNull (>RootBridges, Link)
> > ; Link = GetNextNode (>RootBridges, Link)
> > )
> >{
> >  RootBridge = ROOT_BRIDGE_FROM_LINK (Link);
> >  if (RootBridgeHandle == RootBridge->Handle) {
> >DEBUG ((DEBUG_INFO, "PciHostBridge: SubmitResources for %s\n",
> RootBridge->DevicePathStr));
> >//
> >// Check the resource descriptors.
> >// If the Configuration includes one or more invalid resource
> descriptors, all the resource
> >// descriptors are ignored and the function returns
> EFI_INVALID_PARAMETER.
> >//
> >for (Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR
> *)Configuration; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR;
> Descriptor++) {
> > -if (Descriptor->ResType > ACPI_ADDRESS_SPACE_TYPE_BUS) {
> > +if ((Descriptor->ResType != ACPI_ADDRESS_SPACE_TYPE_MEM) &&
> (Descriptor->ResType != ACPI_ADDRESS_SPACE_TYPE_IO)) {
> >return EFI_INVALID_PARAMETER;
> >  }
>
> This is a slight logic change.
>
> Previously, the code did the following:
>
> - Any resource types that were different from MEM, IO, and BUS, would be
>   rejected with EFI_INVALID_PARAMETER.
>
> - MEM and IO would be handled.
>
> - BUS resource types would trigger an ASSERT().
>
> In effect, the code claimed that BUS resource types were *impossible*
> here, due to prior filtering or whatever.
>
> The logic change is that now we reject BUS resource types explicitly.
>
> I think that's not ideal. Here's why:
>
> - If the preexistent ASSERT() about BUS resource types being impossible
>   was right, then now we are occluding that fact, and pretending /
>   suggesting that BUS types are something we *should* handle. This
>   creates a confusion about data flow.
>
> - If the preexistent ASSERT() about BUS resource types being impossible
>   was wrong (i.e., dependent on input data, we could actuall trigger the
>   ASSERT()), then this change is very welcome, but *then* it is a bugfix
>   in its own right! And therefore should be documented separately.
>
> Anyway. I'm getting exhausted.
>

My interpretation was SubmitResources function is supposed to handle only
Mem or IO 

Re: [edk2-devel] [PATCH] MdeModulePkg/Variable: Merge variable header + data update into one step

[Laszlo Ersek]

+Jiewen (I seem to remember Jiewen co-authored a whitepaper on edk2
variables; I could be wrong)

one more comment below:

On 11/14/23 09:23, Gao wrote:
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4597
> 
> When creating a new variable, skip marking VAR_HEADER_VALID_ONLY so that
> variable header + data update can be merged into one flash write. This
> will greatly reduce the time taken for updating a variable and thus
> increase performance. Removing VAR_HEADER_VALID_ONLY marking doesn't
> have any function impact since it's not used by current code to detect
> variable header + data corruption.

That would have been my question; thanks for spelling it out in advance
(in the commit message).

I wouldn't like to start reviewing this right now, but if you don't get
a review in a week or so, feel free to ping me; I'll try to dig into it.

Laszlo

> 
> Cc: Liming Gao 
> Cc: Ray Ni 
> Signed-off-by: Gao Cheng 
> ---
>  .../Universal/Variable/RuntimeDxe/Variable.c  | 45 ++-
>  1 file changed, 4 insertions(+), 41 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c 
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index 7a1331255b..3c360481f4 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -2168,11 +2168,9 @@ UpdateVariable (
>  
>  if (!mVariableModuleGlobal->VariableGlobal.EmuNvMode) {
>//
> -  // Four steps
> -  // 1. Write variable header
> -  // 2. Set variable state to header valid
> -  // 3. Write variable data
> -  // 4. Set variable state to valid
> +  // Two steps
> +  // 1. Write variable header and data
> +  // 2. Set variable state to valid
>//
>//
>// Step 1:
> @@ -2183,7 +2181,7 @@ UpdateVariable (
>   TRUE,
>   Fvb,
>   mVariableModuleGlobal->NonVolatileLastVariableOffset,
> - (UINT32)GetVariableHeaderSize (AuthFormat),
> + (UINT32)VarSize,
>   (UINT8 *)NextVariable
>   );
>  
> @@ -2194,41 +2192,6 @@ UpdateVariable (
>//
>// Step 2:
>//
> -  NextVariable->State = VAR_HEADER_VALID_ONLY;
> -  Status  = UpdateVariableStore (
> -  >VariableGlobal,
> -  FALSE,
> -  TRUE,
> -  Fvb,
> -  
> mVariableModuleGlobal->NonVolatileLastVariableOffset + OFFSET_OF 
> (VARIABLE_HEADER, State),
> -  sizeof (UINT8),
> -  >State
> -  );
> -
> -  if (EFI_ERROR (Status)) {
> -goto Done;
> -  }
> -
> -  //
> -  // Step 3:
> -  //
> -  Status = UpdateVariableStore (
> - >VariableGlobal,
> - FALSE,
> - TRUE,
> - Fvb,
> - mVariableModuleGlobal->NonVolatileLastVariableOffset + 
> GetVariableHeaderSize (AuthFormat),
> - (UINT32)(VarSize - GetVariableHeaderSize (AuthFormat)),
> - (UINT8 *)NextVariable + GetVariableHeaderSize (AuthFormat)
> - );
> -
> -  if (EFI_ERROR (Status)) {
> -goto Done;
> -  }
> -
> -  //
> -  // Step 4:
> -  //
>NextVariable->State = VAR_ADDED;
>Status  = UpdateVariableStore (
>>VariableGlobal,



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

Re: [edk2-devel] edk2 uncrustify update (73.0.8)?

[Rebecca Cran via groups.io]

On 11/14/2023 7:51 AM, Laszlo Ersek via groups.io wrote:


Funnily enough, my stance is quite the opposite. I happen to disagree
with some patterns that uncrustify enforces, but I'm thankful that at
any given state of CI (= using any given version of uncrustify), we
can't have any more debates about patch formatting (that is, it's
especially its central nature that I like). I've found uncrustify
relatively easy to use locally, too.


There's _one_ place we can still have a debate, but I'm hoping we can 
easily agree, and update CI to enforce it.


I'd like to scrub the tree of all the NT-style function documentation 
blocks and replace them with Doxygen style.


As an example of the NT style, see OvmfPkg/QemuVideoDxe/Gop.c:

EFI_STATUS
EFIAPI
QemuVideoGraphicsOutputQueryMode (
  IN  EFI_GRAPHICS_OUTPUT_PROTOCOL  *This,
  IN  UINT32ModeNumber,
  OUT UINTN *SizeOfInfo,
  OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  **Info
  )

/*++

Routine Description:

  Graphics Output protocol interface to query video mode

  Arguments:
This  - Protocol instance pointer.
ModeNumber- The mode number to return information on.
Info  - Caller allocated buffer that returns 
information about ModeNumber.
SizeOfInfo- A pointer to the size, in bytes, of the 
Info buffer.


  Returns:
EFI_SUCCESS   - Mode information returned.
EFI_BUFFER_TOO_SMALL  - The Info buffer was too small.
EFI_DEVICE_ERROR  - A hardware error occurred trying to 
retrieve the video mode.
EFI_NOT_STARTED   - Video display is not initialized. Call 
SetMode ()

EFI_INVALID_PARAMETER - One of the input args was NULL.

--*/
{
  QEMU_VIDEO_PRIVATE_DATA  *Private;
  QEMU_VIDEO_MODE_DATA *ModeData;
...


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111203): https://edk2.groups.io/g/devel/message/111203
Mute This Topic: https://groups.io/mt/102559740/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 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Ranbir Singh]

On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek  wrote:

> On 11/10/23 07:11, Ranbir Singh wrote:
> > EFI_NOT_READY was listed as one of the error return values in the
> > function header of StartPciDevices(). So, I considered it here.
> >
> > Maybe we can go by a dual approach, including CpuDeadLoop() in
> > StartPciDevices() as well as add return value check at the call site in
> > PciBusDriverBindingStart().
>
> I don't think this makes much sense, given that execution is not
> supposed to continue past CpuDeadLoop(), so the new error check would
> not be reached.
>
> I think two options are realistic:
>
> - replace the assert() with a CpuDeadLoop() -- this is my preference
>
> - keep the assert, add the "if", and in the caller, handle the
> EFI_NOT_READY error. This is workable too. (Keeping the assert is goog
> because it shows that we don't expect the "if" to fire.)
>
> Anyway, now that we have no way to verify the change against Coverity, I
> don't know if this patch is worth the churn. :( As I said, this ASSERT()
> is one of those few justified ones in edk2 core that can indeed never
> fail, IMO.
>
> Laszlo
>
>
See, Does the following change look acceptable to you ?

ASSERT (RootBridge != NULL);
+  if (RootBridge == NULL) {
+CpuDeadLoop ();
+return EFI_NOT_READY;
+  }
+

which retains the existing assert, adds the NULL pointer check and includes
CpuDeadLoop () within the if block. As a result of CpuDeadLoop (), the
return statement afterwards will never reach execution (=> no need to
handle this return value at the call sites), but will satisfy static
analysis tools as the "RootBridge" dereference scenario doesn't arise
thereafter.


> >
> > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek  > > wrote:
> >
> > On 11/7/23 07:19, Ranbir Singh wrote:
> > > From: Ranbir Singh 
> > >
> > > The function StartPciDevices has a check
> > >
> > > ASSERT (RootBridge != NULL);
> > >
> > > but this comes into play only in DEBUG mode. In Release mode, there
> > > is no handling if the RootBridge value is NULL and the code
> proceeds
> > > to unconditionally dereference "RootBridge" which will lead to
> CRASH.
> > >
> > > Hence, for safety add NULL pointer checks always and return
> > > EFI_NOT_READY if RootBridge value is NULL which is one of the
> return
> > > values as mentioned in the function description header.
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> > 
> > >
> > > Cc: Ray Ni mailto:ray...@intel.com>>
> > > Co-authored-by: Veeresh Sangolli  > >
> > > Signed-off-by: Ranbir Singh 
> > > Signed-off-by: Ranbir Singh  > >
> > > ---
> > >  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 -
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > > index 581e9075ad41..3de80d98370e 100644
> > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > > @@ -772,7 +772,10 @@ StartPciDevices (
> > >LIST_ENTRY *CurrentLink;
> > >
> > >RootBridge = GetRootBridgeByHandle (Controller);
> > > -  ASSERT (RootBridge != NULL);
> > > +  if (RootBridge == NULL) {
> > > +return EFI_NOT_READY;
> > > +  }
> > > +
> > >ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle;
> > >
> > >CurrentLink = mPciDevicePool.ForwardLink;
> >
> > I don't think this is a good fix.
> >
> > There is one call site, namely in PciBusDriverBindingStart(). That
> call
> > site does not check the return value. (Of course /s)
> >
> > I think that this ASSERT() can indeed never fail. Therefore I suggest
> > CpuDeadLoop() instead.
> >
> > If you insist that CpuDeadLoop() is "too risky" here, then the patch
> is
> > acceptable, but then the StartPciDevices() call site in
> > PciBusDriverBindingStart() must check the error properly: we must not
> > install "gEfiPciEnumerationCompleteProtocolGuid", and the function
> must
> > propagate the error outwards.
> >
> > Laszlo
> >
>
>


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

Re: [edk2-devel] edk2 uncrustify update (73.0.8)?

[Laszlo Ersek]

On 11/13/23 22:33, Pedro Falcato wrote:
> On Mon, Nov 13, 2023 at 8:37 PM Rebecca Cran  wrote:
>>
>> On 11/13/2023 1:08 PM, Michael Kubacki wrote:
>>> Yes. I just did it. It is relatively minor and impacts expected code
>>> areas.
>>>
>>> https://github.com/tianocore/edk2/pull/5043/files
>>
>>
>> Could you update .git-blame-ignore-revs please?
> 
> You can't do that until the merge is done, since we use
> rebase-and-merge for tianocore (and rebases do not keep stable commit
> hashes).
> But I would plead that this should not get merged in general :/

Seeing the cumulative diff in that PR, do you have specific
counter-arguments?

The diff is trivial, IMO. You mentioned "error prone" and "much churn",
which are very valid concerns, but they don't seem to apply here. We can
review a diff of this size (especially if it's split up on Pkg
boundaries), and the github view indicates the change is only in
whitespace amount.

The change in
"OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c"
is a net win; the current formatting is really distracting.

Furthermore, this diff actually highlights some inexplicable syntax in
"EmulatorPkg/FvbServicesRuntimeDxe/FvbInfo.c": those backslashes (not
parts of any macro definition) are an eyesore. They should be fixed
regardless of re-uncrustification.

The version N vs. N+1 concern shouldn't be one; the authoritative
version is what the YAML file in edk2 says.

Thanks!
Laszlo



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

Re: [edk2-devel] edk2 uncrustify update (73.0.8)?

[Laszlo Ersek]

On 11/13/23 20:07, Pedro Falcato wrote:
> On Mon, Nov 13, 2023 at 11:58 AM Laszlo Ersek  wrote:
>>
>> Hi Michael,
>>
>> recently I encountered an uncrustify failure on github.
>>
>> The reason was that my local uncrustify was *more recent* (73.0.8) than
>> the one we use in edk2 CI (which is 73.0.3, per the edk2 file
>> ".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml").
> 
> Wait, you can use upstream uncrustify? I'm just using whatever
> uncrustify version I took from the project-mu fork...

Apologies -- for edk2 purposes (and I don't use uncrustify for anything
else), I consider

"upstream".

> 
>>
>> Updating the version number in the YAML file (i.e., advancing edk2 to
>> version 73.0.8) seems easy enough, but:
>>
>> - Do you think 73.0.8 is mature enough for adoption in edk2?
>>
>>   This upstream uncrustify release was tagged in April (and I can't see
>>   any more recent commits), so I assume it should be stable.
>>
>> - Would the version update require a whole-tree re-uncrustification?
> 
> Please, no. I didn't mind doing an initial reformatting at first, but
> doing this continuously is both 1) problem-prone 2) just amazing
> amounts of churn.
> Let's say I have version N, you have version N+1 - we may never get
> any final, formatted output as your version formats it differently
> from mine.

Your argument against a continuously reformatted code base is well
received; just a small correction: we should never have an N vs. N+1
dilemma. What CI uses is the authoritative version.

> 
> I don't know how the CI is doing its thing atm (I haven't merged
> anything myself to edk2), but the uncrustify check should be relaxed
> to just a warning.

I don't think that's going to happen. Everybody ignores warnings when in
a rush, or when the same warnings pop up for the 10th time.

> There's nothing wrong with what my uncrustify
> version is formatting to, there's nothing wrong with yours either, and
> CI isn't necessarily wrong either.
> 
> And, to be fair, I already find uncrustify a large pain in the butt to
> use (requiring a custom fork really does not help), but I find the
> benefits worth it *locally*, as my coding style is also quite
> different from the NT-esque style.

Funnily enough, my stance is quite the opposite. I happen to disagree
with some patterns that uncrustify enforces, but I'm thankful that at
any given state of CI (= using any given version of uncrustify), we
can't have any more debates about patch formatting (that is, it's
especially its central nature that I like). I've found uncrustify
relatively easy to use locally, too.

All in all I'm not trying to upset the status quo, it's just a question
about a version bump, and how we'd deal with any fallout from that.

Laszlo



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

Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

Reviewed-by: Abner Chang 

> -Original Message-
> From: Igor Kulchytskyy 
> Sent: Tuesday, November 14, 2023 10:28 PM
> To: devel@edk2.groups.io
> Cc: Chang, Abner ; Nickle Wang
> 
> Subject: [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the
> Redfish Discover flow
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Filter out the network interfaces which are not supported by
> Redfish Host Interface.
>
> Cc: Abner Chang 
> Cc: Nickle Wang 
> Signed-off-by: Igor Kulchytskyy 
> ---
>  RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c  | 163
> ++--
>  RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h |   6 +
>  2 files changed, 120 insertions(+), 49 deletions(-)
>
> diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> index 0f622e05a9..ae83cd3c97 100644
> --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> @@ -322,9 +322,16 @@ GetTargetNetworkInterfaceInternal (
>  {
>EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL
> *ThisNetworkInterface;
>
> +  if (IsListEmpty ()) {
> +return NULL;
> +  }
> +
>ThisNetworkInterface =
> (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetFirstNode
> ();
>while (TRUE) {
> -if (CompareMem ((VOID *)>MacAddress,
> >MacAddress, ThisNetworkInterface-
> >HwAddressSize) == 0) {
> +if ((MAC_COMPARE (ThisNetworkInterface, TargetNetworkInterface) == 0)
> &&
> +(VALID_TCP6 (TargetNetworkInterface, ThisNetworkInterface) ||
> + VALID_TCP4 (TargetNetworkInterface, ThisNetworkInterface)))
> +{
>return ThisNetworkInterface;
>  }
>
> @@ -354,6 +361,10 @@ GetTargetNetworkInterfaceInternalByController (
>  {
>EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL
> *ThisNetworkInterface;
>
> +  if (IsListEmpty ()) {
> +return NULL;
> +  }
> +
>ThisNetworkInterface =
> (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetFirstNode
> ();
>while (TRUE) {
>  if (ThisNetworkInterface->OpenDriverControllerHandle ==
> ControllerHandle) {
> @@ -476,6 +487,42 @@ CheckIsIpVersion6 (
>return FALSE;
>  }
>
> +/**
> +  This function returns the  IP type supported by the Host Interface.
> +
> +  @retval 00h is Unknown
> +  01h is Ipv4
> +  02h is Ipv6
> +
> +**/
> +UINT8
> +GetHiIpProtocolType (
> +  VOID
> +  )
> +{
> +  EFI_STATUS Status;
> +  REDFISH_OVER_IP_PROTOCOL_DATA  *Data;
> +  REDFISH_INTERFACE_DATA *DeviceDescriptor;
> +
> +  Data = NULL;
> +  DeviceDescriptor = NULL;
> +  if (mSmbios == NULL) {
> +Status = gBS->LocateProtocol (, NULL, (VOID
> **));
> +if (EFI_ERROR (Status)) {
> +  return
> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN;
> +}
> +  }
> +
> +  Status = RedfishGetHostInterfaceProtocolData (mSmbios,
> , ); // Search for SMBIOS type 42h
> +  if (!EFI_ERROR (Status) && (Data != NULL) &&
> +  (Data->HostIpAssignmentType == RedfishHostIpAssignmentStatic))
> +  {
> +return Data->HostIpAddressFormat;
> +  }
> +
> +  return
> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN;
> +}
> +
>  /**
>This function discover Redfish service through SMBIOS host interface.
>
> @@ -512,6 +559,18 @@ DiscoverRedfishHostInterface (
>
>Status = RedfishGetHostInterfaceProtocolData (mSmbios,
> , ); // Search for SMBIOS type 42h
>if (!EFI_ERROR (Status) && (Data != NULL) && (DeviceDescriptor != NULL)) {
> +if ((Instance->NetworkInterface->NetworkProtocolType ==
> ProtocolTypeTcp4) &&
> +(Data->HostIpAddressFormat !=
> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4)) // IPv4 case
> +{
> +  DEBUG ((DEBUG_ERROR, "%a: Network Interface is IPv4, but Host
> Interface requires Ipv6\n", __func__));
> +  return EFI_UNSUPPORTED;
> +} else if ((Instance->NetworkInterface->NetworkProtocolType ==
> ProtocolTypeTcp6) &&
> +   (Data->HostIpAddressFormat !=
> REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6)) // IPv6 case
> +{
> +  DEBUG ((DEBUG_ERROR, "%a: Network Interface is IPv6, but Host
> Interface requires IPv4\n", __func__));
> +  return EFI_UNSUPPORTED;
> +}
> +
>  //
>  // Check if we can reach out Redfish service using this network 
> interface.
>  // Check with MAC address using Device Descriptor Data Device Type 04
> and Type 05.
> @@ -1102,6 +1161,7 @@ RedfishServiceGetNetworkInterface (
>OUT EFI_REDFISH_DISCOVER_NETWORK_INTERFACE
> **NetworkIntfInstances
>)
>  {
> +  EFI_STATUS   Status;
>EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL
> *ThisNetworkInterfaceIntn;
>EFI_REDFISH_DISCOVER_NETWORK_INTERFACE
> *ThisNetworkInterface;
>EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL   *RestExInstance;
> @@ 

Re: [edk2-devel] [PATCH v5 1/2] RedfishPkg: RedfishDiscoverDxe: Fix issue if IPv4 installed after RestEx

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

Reviewed-by: Abner Chang 

> -Original Message-
> From: Igor Kulchytskyy 
> Sent: Tuesday, November 14, 2023 10:28 PM
> To: devel@edk2.groups.io
> Cc: Chang, Abner ; Nickle Wang
> 
> Subject: [PATCH v5 1/2] RedfishPkg: RedfishDiscoverDxe: Fix issue if IPv4
> installed after RestEx
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Supported function of the driver changed to wait for all network
> interface to be installed.
>
> Cc: Abner Chang 
> Cc: Nickle Wang 
> Signed-off-by: Igor Kulchytskyy 
> ---
>  RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c | 31 ++--
> 
>  1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> index 23da3b968f..0f622e05a9 100644
> --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> @@ -1547,25 +1547,26 @@ TestForRequiredProtocols (
>  ControllerHandle,
>  EFI_OPEN_PROTOCOL_TEST_PROTOCOL
>  );
> +if (EFI_ERROR (Status)) {
> +  return EFI_UNSUPPORTED;
> +}
> +
> +Status = gBS->OpenProtocol (
> +ControllerHandle,
> +gRequiredProtocol[Index].DiscoveredProtocolGuid,
> +(VOID **),
> +This->DriverBindingHandle,
> +ControllerHandle,
> +EFI_OPEN_PROTOCOL_GET_PROTOCOL
> +);
>  if (!EFI_ERROR (Status)) {
> -  Status = gBS->OpenProtocol (
> -  ControllerHandle,
> -  gRequiredProtocol[Index].DiscoveredProtocolGuid,
> -  (VOID **),
> -  This->DriverBindingHandle,
> -  ControllerHandle,
> -  EFI_OPEN_PROTOCOL_GET_PROTOCOL
> -  );
> -  if (EFI_ERROR (Status)) {
> -if (Index == ListCount - 1) {
> -  DEBUG ((DEBUG_INFO, "%a: all required protocols are found on this
> controller handle: %p.\n", __func__, ControllerHandle));
> -  return EFI_SUCCESS;
> -}
> -  }
> +  // Already installed
> +  return EFI_UNSUPPORTED;
>  }
>}
>
> -  return EFI_UNSUPPORTED;
> +  DEBUG ((DEBUG_MANAGEABILITY, "%a: all required protocols are found on
> this controller handle: %p.\n", __func__, ControllerHandle));
> +  return EFI_SUCCESS;
>  }
>
>  /**
> --
> 2.37.1.windows.1
> -The information contained in this message may be confidential and
> proprietary to American Megatrends (AMI). This communication is intended
> to be read only by the individual or entity to whom it is addressed or by 
> their
> designee. If the reader of this message is not the intended recipient, you are
> on notice that any distribution of this message, in any form, is strictly
> prohibited. Please promptly notify the sender by reply e-mail or by telephone
> at 770-246-8600, and then delete or destroy all copies of the transmission.


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

[edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow

[Igor Kulchytskyy via groups.io]

Filter out the network interfaces which are not supported by
Redfish Host Interface.

Cc: Abner Chang 
Cc: Nickle Wang 
Signed-off-by: Igor Kulchytskyy 
---
 RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c  | 163 
++--
 RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h |   6 +
 2 files changed, 120 insertions(+), 49 deletions(-)

diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c 
b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
index 0f622e05a9..ae83cd3c97 100644
--- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
+++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
@@ -322,9 +322,16 @@ GetTargetNetworkInterfaceInternal (
 {
   EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL  *ThisNetworkInterface;

+  if (IsListEmpty ()) {
+return NULL;
+  }
+
   ThisNetworkInterface = (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL 
*)GetFirstNode ();
   while (TRUE) {
-if (CompareMem ((VOID *)>MacAddress, 
>MacAddress, ThisNetworkInterface->HwAddressSize) == 0) 
{
+if ((MAC_COMPARE (ThisNetworkInterface, TargetNetworkInterface) == 0) &&
+(VALID_TCP6 (TargetNetworkInterface, ThisNetworkInterface) ||
+ VALID_TCP4 (TargetNetworkInterface, ThisNetworkInterface)))
+{
   return ThisNetworkInterface;
 }

@@ -354,6 +361,10 @@ GetTargetNetworkInterfaceInternalByController (
 {
   EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL  *ThisNetworkInterface;

+  if (IsListEmpty ()) {
+return NULL;
+  }
+
   ThisNetworkInterface = (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL 
*)GetFirstNode ();
   while (TRUE) {
 if (ThisNetworkInterface->OpenDriverControllerHandle == ControllerHandle) {
@@ -476,6 +487,42 @@ CheckIsIpVersion6 (
   return FALSE;
 }

+/**
+  This function returns the  IP type supported by the Host Interface.
+
+  @retval 00h is Unknown
+  01h is Ipv4
+  02h is Ipv6
+
+**/
+UINT8
+GetHiIpProtocolType (
+  VOID
+  )
+{
+  EFI_STATUS Status;
+  REDFISH_OVER_IP_PROTOCOL_DATA  *Data;
+  REDFISH_INTERFACE_DATA *DeviceDescriptor;
+
+  Data = NULL;
+  DeviceDescriptor = NULL;
+  if (mSmbios == NULL) {
+Status = gBS->LocateProtocol (, NULL, (VOID 
**));
+if (EFI_ERROR (Status)) {
+  return REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN;
+}
+  }
+
+  Status = RedfishGetHostInterfaceProtocolData (mSmbios, , 
); // Search for SMBIOS type 42h
+  if (!EFI_ERROR (Status) && (Data != NULL) &&
+  (Data->HostIpAssignmentType == RedfishHostIpAssignmentStatic))
+  {
+return Data->HostIpAddressFormat;
+  }
+
+  return REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN;
+}
+
 /**
   This function discover Redfish service through SMBIOS host interface.

@@ -512,6 +559,18 @@ DiscoverRedfishHostInterface (

   Status = RedfishGetHostInterfaceProtocolData (mSmbios, , 
); // Search for SMBIOS type 42h
   if (!EFI_ERROR (Status) && (Data != NULL) && (DeviceDescriptor != NULL)) {
+if ((Instance->NetworkInterface->NetworkProtocolType == ProtocolTypeTcp4) 
&&
+(Data->HostIpAddressFormat != 
REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4)) // IPv4 case
+{
+  DEBUG ((DEBUG_ERROR, "%a: Network Interface is IPv4, but Host Interface 
requires Ipv6\n", __func__));
+  return EFI_UNSUPPORTED;
+} else if ((Instance->NetworkInterface->NetworkProtocolType == 
ProtocolTypeTcp6) &&
+   (Data->HostIpAddressFormat != 
REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6)) // IPv6 case
+{
+  DEBUG ((DEBUG_ERROR, "%a: Network Interface is IPv6, but Host Interface 
requires IPv4\n", __func__));
+  return EFI_UNSUPPORTED;
+}
+
 //
 // Check if we can reach out Redfish service using this network interface.
 // Check with MAC address using Device Descriptor Data Device Type 04 and 
Type 05.
@@ -1102,6 +1161,7 @@ RedfishServiceGetNetworkInterface (
   OUT EFI_REDFISH_DISCOVER_NETWORK_INTERFACE  **NetworkIntfInstances
   )
 {
+  EFI_STATUS   Status;
   EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL  *ThisNetworkInterfaceIntn;
   EFI_REDFISH_DISCOVER_NETWORK_INTERFACE   *ThisNetworkInterface;
   EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL   *RestExInstance;
@@ -1141,13 +1201,23 @@ RedfishServiceGetNetworkInterface (

   ThisNetworkInterfaceIntn = (EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL 
*)GetFirstNode ();
   while (TRUE) {
+// If Get Subnet Info failed then skip this interface
+Status = NetworkInterfaceGetSubnetInfo (ThisNetworkInterfaceIntn, 
ImageHandle); // Get subnet info
+if (EFI_ERROR (Status)) {
+  if (IsNodeAtEnd (, 
>Entry)) {
+break;
+  }
+
+  ThisNetworkInterfaceIntn = 
(EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_INTERNAL *)GetNextNode 
(, >Entry);
+  continue;
+}
+
 ThisNetworkInterface->IsIpv6 = FALSE;
 if (CheckIsIpVersion6 (ThisNetworkInterfaceIntn)) {
   ThisNetworkInterface->IsIpv6 = TRUE;
 

[edk2-devel] [PATCH v5 1/2] RedfishPkg: RedfishDiscoverDxe: Fix issue if IPv4 installed after RestEx

[Igor Kulchytskyy via groups.io]

Supported function of the driver changed to wait for all network
interface to be installed.

Cc: Abner Chang 
Cc: Nickle Wang 
Signed-off-by: Igor Kulchytskyy 
---
 RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c | 31 ++--
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c 
b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
index 23da3b968f..0f622e05a9 100644
--- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
+++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
@@ -1547,25 +1547,26 @@ TestForRequiredProtocols (
 ControllerHandle,
 EFI_OPEN_PROTOCOL_TEST_PROTOCOL
 );
+if (EFI_ERROR (Status)) {
+  return EFI_UNSUPPORTED;
+}
+
+Status = gBS->OpenProtocol (
+ControllerHandle,
+gRequiredProtocol[Index].DiscoveredProtocolGuid,
+(VOID **),
+This->DriverBindingHandle,
+ControllerHandle,
+EFI_OPEN_PROTOCOL_GET_PROTOCOL
+);
 if (!EFI_ERROR (Status)) {
-  Status = gBS->OpenProtocol (
-  ControllerHandle,
-  gRequiredProtocol[Index].DiscoveredProtocolGuid,
-  (VOID **),
-  This->DriverBindingHandle,
-  ControllerHandle,
-  EFI_OPEN_PROTOCOL_GET_PROTOCOL
-  );
-  if (EFI_ERROR (Status)) {
-if (Index == ListCount - 1) {
-  DEBUG ((DEBUG_INFO, "%a: all required protocols are found on this 
controller handle: %p.\n", __func__, ControllerHandle));
-  return EFI_SUCCESS;
-}
-  }
+  // Already installed
+  return EFI_UNSUPPORTED;
 }
   }

-  return EFI_UNSUPPORTED;
+  DEBUG ((DEBUG_MANAGEABILITY, "%a: all required protocols are found on this 
controller handle: %p.\n", __func__, ControllerHandle));
+  return EFI_SUCCESS;
 }

 /**
--
2.37.1.windows.1
-The information contained in this message may be confidential and proprietary 
to American Megatrends (AMI). This communication is intended to be read only by 
the individual or entity to whom it is addressed or by their designee. If the 
reader of this message is not the intended recipient, you are on notice that 
any distribution of this message, in any form, is strictly prohibited. Please 
promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and 
then delete or destroy all copies of the transmission.


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

[edk2-devel] [PATCH v5 0/2] Fix and optimize the issue if IPv4 installed after RestEx

[Igor Kulchytskyy via groups.io]

Igor Kulchytskyy (2):
  RedfishPkg: RedfishDiscoverDxe: Fix issue if IPv4 installed after
RestEx
  RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow

 .../RedfishDiscoverDxe/RedfishDiscoverDxe.c   | 132 +-
 .../RedfishDiscoverInternal.h |   6 +
 2 files changed, 105 insertions(+), 33 deletions(-)

--
2.37.1.windows.1
-The information contained in this message may be confidential and proprietary 
to American Megatrends (AMI). This communication is intended to be read only by 
the individual or entity to whom it is addressed or by their designee. If the 
reader of this message is not the intended recipient, you are on notice that 
any distribution of this message, in any form, is strictly prohibited. Please 
promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and 
then delete or destroy all copies of the transmission.


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

Re: [edk2-devel] [PATCH 0/3] Move gMpInformationHobGuid from StandaloneMmPkg to UefiCpuPkg.

[duntan]

Laszlo,

Thanks for your comments. 
Yes the StatusFlag field of a given ProcessorId does change in the scenario you 
mentioned. I think it's ok to call SwitchBSP() and Enable/DisableAP() after 
creating the hob, since smm elects its own BSP and all CPU will enter smm after 
receiving smi regardless of the StatusFlag. 

So the NumberOfProcessors, the ProcessorId and Location fields remain 
unchanged, the StatusFlag and NumberOfEnabledProcessors may be invalidated. I 
agree that we should document specific fields of the HOB may be invalidated 
between HOB production and HOB consumption. Will send V2 patch set to document 
this in the HOB definition head file.

Thanks,
Dun

-Original Message-
From: Laszlo Ersek  
Sent: Monday, November 13, 2023 10:33 PM
To: devel@edk2.groups.io; Tan, Dun 
Subject: Re: [edk2-devel] [PATCH 0/3] Move gMpInformationHobGuid from 
StandaloneMmPkg to UefiCpuPkg.

On 11/9/23 03:51, duntan wrote:
> Move gMpInformationHobGuid from StandaloneMmPkg to UefiCpuPkg.
> 
> Previously, the HOB is defined, created and consumed only in 
> StandaloneMmPkg. The HOB contains the number of processors and 
> EFI_PROCESSOR_INFORMATION structure. This is the same as the information that 
> PiSmmCpuDxeSmm uses EfiMpServiceProtocolGuid to get.
> 
> The incoming plan is to create gMpInformationHobGuid for both 
> StandaloneMm and legacy DXE_SMM in early phase(for example in 
> CpuMpPei). Then PiSmmCpuDxeSmm can consume the hob, which can simplify code 
> logic in PiSmmCpuDxeSmm driver.
> 
> So move this HOB definition to UefiCpuPkg in this patch series.
> 
> Dun Tan (3):
>   UefiCpuPkg: Create MpInformation.h in UefiCpuPkg
>   StandaloneMmPkg:Add UefiCpuPkg.dec in DependencyCheck
>   StandaloneMmPkg:Remove MpInformation.h
> 
>  StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.inf  
>  | 1 +
>  
> StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
>  | 1 +
>  StandaloneMmPkg/StandaloneMmPkg.ci.yaml  
>  | 3 ++-
>  StandaloneMmPkg/StandaloneMmPkg.dec  
>  | 1 -
>  {StandaloneMmPkg => UefiCpuPkg}/Include/Guid/MpInformation.h 
>  | 2 +-
>  UefiCpuPkg/UefiCpuPkg.dec
>  | 3 +++
>  6 files changed, 8 insertions(+), 3 deletions(-)  rename 
> {StandaloneMmPkg => UefiCpuPkg}/Include/Guid/MpInformation.h (88%)
> 

From a quick skim, the series looks OK to me, and I also agree that the above 
two MP services calls (GetNumberOfProcessors, GetProcessorInfo) seem to be the 
only ones in PiSmmCpuDxeSmm.

However, what about the following scenario:

- in the PI phase (or HOB producer phase), the HOB is produced

- in the DXE phase, a platform DXE driver uses EFI_MP_SERVICES_PROTOCOL to 
change some aspect of the processors in the system. For example, it calls 
SwitchBSP, or calls EnableDisableAP.

- Then the information in the HOB will be stale, once PiSmmCpuDxeSmm consumes 
it. The EFI_PROCESSOR_INFORMATION.StatusFlag field carries information both 
about the CPU in question being BSP vs. AP, and about the CPU being enabled or 
disabled. So either of SwitchBSP() and
EnableDisableAP() can invalidate the StatusFlag field for a given ProcessorId.

I don't know how StandaloneMmPkg currently deals with this scenario, and I also 
don't know whether StatusFlag actually matters to PiSmmCpuSmmDxe.
Apparently, it doesn't. So technically, the replacement of the protocol with 
the HOB should be fine, but for the general case, we should document somehow 
that specific fields of the HOB may be invalidated between HOB production and 
HOB consumption. If platform code is required to prevent that (i.e., the 
platform is responsible for not calling
SwitchBSP() or EnableDisableAP()), then that requirement should also be 
documented.

Acked-by: Laszlo Ersek 


Thanks
Laszlo



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

[edk2-devel] [PATCH] MdeModulePkg/Variable: Merge variable header + data update into one step

[Gao]

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

When creating a new variable, skip marking VAR_HEADER_VALID_ONLY so that
variable header + data update can be merged into one flash write. This
will greatly reduce the time taken for updating a variable and thus
increase performance. Removing VAR_HEADER_VALID_ONLY marking doesn't
have any function impact since it's not used by current code to detect
variable header + data corruption.

Cc: Liming Gao 
Cc: Ray Ni 
Signed-off-by: Gao Cheng 
---
 .../Universal/Variable/RuntimeDxe/Variable.c  | 45 ++-
 1 file changed, 4 insertions(+), 41 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c 
b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 7a1331255b..3c360481f4 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -2168,11 +2168,9 @@ UpdateVariable (
 
 if (!mVariableModuleGlobal->VariableGlobal.EmuNvMode) {
   //
-  // Four steps
-  // 1. Write variable header
-  // 2. Set variable state to header valid
-  // 3. Write variable data
-  // 4. Set variable state to valid
+  // Two steps
+  // 1. Write variable header and data
+  // 2. Set variable state to valid
   //
   //
   // Step 1:
@@ -2183,7 +2181,7 @@ UpdateVariable (
  TRUE,
  Fvb,
  mVariableModuleGlobal->NonVolatileLastVariableOffset,
- (UINT32)GetVariableHeaderSize (AuthFormat),
+ (UINT32)VarSize,
  (UINT8 *)NextVariable
  );
 
@@ -2194,41 +2192,6 @@ UpdateVariable (
   //
   // Step 2:
   //
-  NextVariable->State = VAR_HEADER_VALID_ONLY;
-  Status  = UpdateVariableStore (
-  >VariableGlobal,
-  FALSE,
-  TRUE,
-  Fvb,
-  
mVariableModuleGlobal->NonVolatileLastVariableOffset + OFFSET_OF 
(VARIABLE_HEADER, State),
-  sizeof (UINT8),
-  >State
-  );
-
-  if (EFI_ERROR (Status)) {
-goto Done;
-  }
-
-  //
-  // Step 3:
-  //
-  Status = UpdateVariableStore (
- >VariableGlobal,
- FALSE,
- TRUE,
- Fvb,
- mVariableModuleGlobal->NonVolatileLastVariableOffset + 
GetVariableHeaderSize (AuthFormat),
- (UINT32)(VarSize - GetVariableHeaderSize (AuthFormat)),
- (UINT8 *)NextVariable + GetVariableHeaderSize (AuthFormat)
- );
-
-  if (EFI_ERROR (Status)) {
-goto Done;
-  }
-
-  //
-  // Step 4:
-  //
   NextVariable->State = VAR_ADDED;
   Status  = UpdateVariableStore (
   >VariableGlobal,
-- 
2.42.0.windows.2



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