-----Original Message-----
From: Tan, Dun <dun.tan@...>
Sent: Monday, December 5, 2022 11:57 AM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb+tianocore@...>; Yao, Jiewen
<jiewen.yao@...>; Justen, Jordan L <jordan.l.justen@...>;
Gerd Hoffmann <kraxel@...>; Ni, Ray <ray.ni@...>; Sean
Brogan <sean.brogan@...>; Kinney, Michael D
<michael.d.kinney@...>
Subject: RE: [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformCI: Expand Ovmf
PlatformCI for Shell UnitTest

Hi all,
Is there anything else I can do to speed up the review process for this patch
set? Looking forward to your reply.

Thanks,
Dun

-----Original Message-----
From: Tan, Dun
Sent: Monday, November 28, 2022 5:34 PM
To: devel@edk2.groups.io; Tan, Dun <dun.tan@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>; Yao, Jiewen
<jiewen.yao@...>; Justen, Jordan L <jordan.l.justen@...>;
Gerd Hoffmann <kraxel@...>; Ni, Ray <ray.ni@...>
Subject: RE: [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformCI: Expand Ovmf
PlatformCI for Shell UnitTest

Hi all,
Could you please help to review this patch? Thanks a lot!

Thanks,
Dun

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of duntan
Sent: Tuesday, November 22, 2022 7:48 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb+tianocore@...>; Yao, Jiewen
<jiewen.yao@...>; Justen, Jordan L <jordan.l.justen@...>;
Gerd Hoffmann <kraxel@...>; Ni, Ray <ray.ni@...>
Subject: [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformCI: Expand Ovmf
PlatformCI for Shell UnitTest

Expand Ovmf PlatformBuild.py and PlatformBuildLib.py to support building
and running specific Shell target UnitTest modules.
In the new CommonPlatform class:
It provides new class attributes and new methods to support build and run
specific Shell Unit Test modules.

In the new SettingsManager class for stuart_pr_eval:
It calls new API in CommonPlatform to updates PackagesSupported based
on -u ShellUnitTestList input from cmd. The package which contains the
module in ShellUnitTestList will be added into PackagesSupported for
further evaluation.

In the new PlatformBuilder class for stuart_build:
1.In PlatformPostBuild(), it conditionally calls new API in CommonPlatform
to build -u ShellUnitTestList in -p PkgsToBuild and copy them to VirtualDrive
folder. If no -p option, all the modules in -u ShellUnitTestList will be built.
2.In FlashRomImage(), it conditionally calls the new API in CommonPlatform
to write all efi files name in VirtualDrive into startup.nsh and output
UnitTest log into ShellUnitTestLog.
3. After the boot process, it conditionally calls new API in CommonPlatform
to check the UnitTest boot log.

Signed-off-by: Dun Tan <dun.tan@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Gerd Hoffmann <kraxel@...>
Cc: Ray Ni <ray.ni@...>
---
OvmfPkg/PlatformCI/PlatformBuild.py | 112
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
++++++++++++++++++++++++++++++++++++++++++++++++++++++
OvmfPkg/PlatformCI/PlatformBuildLib.py | 51
+++++++++++++++++++++++++++++++++++++++++++++------
2 files changed, 157 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/PlatformCI/PlatformBuild.py
b/OvmfPkg/PlatformCI/PlatformBuild.py
index 6c541cdea4..72cb7e0e9e 100644
--- a/OvmfPkg/PlatformCI/PlatformBuild.py
+++ b/OvmfPkg/PlatformCI/PlatformBuild.py
@@ -6,6 +6,11 @@
##
import os
import sys
+import shutil
+import logging
+import re
+from edk2toolext.environment import shell_environment from
+edk2toolext.environment.multiple_workspace import MultipleWorkspace

sys.path.append(os.path.dirname(os.path.abspath(__file__)))
from PlatformBuildLib import SettingsManager @@ -24,6 +29,10 @@
class CommonPlatform():
Scopes = ('ovmf', 'edk2-build')
WorkspaceRoot = os.path.realpath(os.path.join(
os.path.dirname(os.path.abspath(__file__)), "..", ".."))
+ # Support build and run Shell Unit Test modules
+ UnitTestModuleList = {}
+ RunShellUnitTest = False
+ ShellUnitTestLog = ''

@classmethod
def GetDscName(cls, ArchCsv: str) -> str:
@@ -39,5 +48,108 @@ class CommonPlatform():
dsc += ".dsc"
return dsc

+ @classmethod
+ def UpdatePackagesSupported(cls, ShellUnitTestListArg):
+ ''' Update PackagesSupported by -u ShellUnitTestList from cmd line. '''
+ UnitTestModuleListStr = ','.join(ShellUnitTestListArg)
+ if not re.search(r'.+.inf:.+.dsc', UnitTestModuleListStr):
+ raise Exception('No valid ModulePath:DscPath in the -u
{}'.format(UnitTestModuleListStr))
+ UnitTestModuleList = UnitTestModuleListStr.split(',')
+ PackagesSupported = []
+ for KeyValue in UnitTestModuleList:
+ PkgName = KeyValue.split("Pkg")[0] + 'Pkg'
+ if PkgName not in PackagesSupported:
+ PackagesSupported.append(PkgName)
+ cls.PackagesSupported = tuple(PackagesSupported)
+ print('PackagesSupported for UnitTest is
+ {}'.format(cls.PackagesSupported))
+
+ @classmethod
+ def UpdateUnitTestConfig(cls, args):
+ ''' Update UnitTest config by -u ShellUnitTestList and -p
PkgsToBuildForUT from cmd line.
+ ShellUnitTestList is in this format: {module1:dsc1, module2:dsc2,
module3:dsc2...}.
+ Only the modules which are in the PkgsToBuildForUT list are added
into self.UnitTestModuleList.
+ '''
+ UnitTestModuleListStr = ','.join(args.ShellUnitTestList)
+ if not re.search(r'.+.inf:.+.dsc', UnitTestModuleListStr):
+ raise Exception('No valid ModulePath:DscPath in the -u
{}'.format(args.ShellUnitTestList))
+ UnitTestModuleList = UnitTestModuleListStr.split(',')
+ if args.PkgsToBuildForUT is None or all(['Pkg' not in Pkg for Pkg in
args.PkgsToBuildForUT]):
+ # No invalid Pkgs from input. Build all modules in -u
UnitTestModuleList.
+ for KeyValue in UnitTestModuleList:
+ UnitTestPath = os.path.normpath(KeyValue.split(":")[0])
+ DscPath = os.path.normpath(KeyValue.split(":")[1])
+ cls.UnitTestModuleList[UnitTestPath] = DscPath
+ else:
+ PkgsToBuildForUT = ','.join(args.PkgsToBuildForUT).split(',')
+ for KeyValue in UnitTestModuleList:
+ UnitTestPath = os.path.normpath(KeyValue.split(":")[0])
+ DscPath = os.path.normpath(KeyValue.split(":")[1])
+ PkgName = UnitTestPath.split("Pkg")[0] + 'Pkg'
+ if PkgName in PkgsToBuildForUT:
+ cls.UnitTestModuleList[UnitTestPath] = DscPath
+ if len(cls.UnitTestModuleList) > 0:
+ cls.RunShellUnitTest = True
+ cls.ShellUnitTestLog = os.path.join(cls.WorkspaceRoot, 'Build',
"BUILDLOG_UnitTest.txt")
+ print('UnitTestModuleList is
+ {}'.format(cls.UnitTestModuleList))
+
+ def BuildUnitTest(self):
+ ''' Build specific DSC for modules in UnitTestModuleList '''
+ self.env = shell_environment.GetBuildVars()
+ self.ws = PlatformBuilder.GetWorkspaceRoot(self)
+ self.mws = MultipleWorkspace()
+ self.pp = ''
+ VirtualDrive = os.path.join(self.env.GetValue("BUILD_OUTPUT_BASE"),
"VirtualDrive")
+ os.makedirs(VirtualDrive, exist_ok=True)
+
+ # DSC by self.GetDscName() should have been built in BUILD process.
+ BuiltDsc =
[CommonPlatform.GetDscName(",".join(self.env.GetValue("TARGET_ARCH")
.split(' ')))]
+ for UnitTestPath, DscPath in
CommonPlatform.UnitTestModuleList.items():
+ if DscPath not in BuiltDsc:
+ ModuleName = os.path.split(UnitTestPath)[1].split('.inf')[0]
+ logging.info('Build {0} for {1}'.format(DscPath, ModuleName))
+ BuiltDsc.append(DscPath)
+ Arch = self.env.GetValue("TARGET_ARCH").split(" ")
+ if 'X64' in Arch:
+ UTArch = 'X64'
+ else:
+ UTArch = 'IA32'
+ self.env.AllowOverride("ACTIVE_PLATFORM")
+ self.env.SetValue("ACTIVE_PLATFORM", DscPath, "For UnitTest")
+ self.env.AllowOverride("TARGET_ARCH")
+ self.env.SetValue("TARGET_ARCH", UTArch, "For UnitTest") # Set
UnitTest arch the same as Ovmf Shell module.
+ ret = PlatformBuilder.Build(self) # Build specific dsc
for UnitTest modules
+ if (ret != 0):
+ return ret
+ ret = PlatformBuilder.ParseDscFile(self) # Parse
OUTPUT_DIRECTORY from dsc files
+ if(ret != 0):
+ return ret
+ OutputPath = os.path.normpath(os.path.join(self.ws,
self.env.GetValue("OUTPUT_DIRECTORY")))
+ EfiPath = os.path.join(OutputPath, self.env.GetValue("TARGET") +
"_" + self.env.GetValue("TOOL_CHAIN_TAG"),
+ UTArch, UnitTestPath.split('.inf')[0], "OUTPUT",
ModuleName + '.efi')
+ logging.info('Copy {0}.efi from:{1}'.format(ModuleName, EfiPath))
+ shutil.copy(EfiPath, VirtualDrive)
+ return 0
+
+ @staticmethod
+ def WriteEfiToStartup(EfiFolder, FileObj):
+ ''' Write all the .efi files' name in VirtualDrive into Startup.nsh '''
+ for Root,Dirs,Files in os.walk(EfiFolder):
+ for File in Files:
+ if os.path.splitext(File)[1] == '.efi':
+ FileObj.write("{0} \n".format(File))
+
+ @classmethod
+ def CheckUnitTestLog(cls):
+ ''' Check the boot log for UnitTest '''
+ File = open(cls.ShellUnitTestLog, "r")
+ FileContent = File.readlines()
+ logging.info('Check the UnitTest boot
log:{0}'.format(cls.ShellUnitTestLog))
+ for Index in range(len(FileContent)):
+ if 'FAILURE MESSAGE:' in FileContent[Index]:
+ if FileContent[Index + 1].strip() != '':
+ FailureMessage = FileContent[Index + 1] + FileContent[Index +
2]
+ return FailureMessage
+ return 0
+
import PlatformBuildLib
PlatformBuildLib.CommonPlatform = CommonPlatform diff --git
a/OvmfPkg/PlatformCI/PlatformBuildLib.py
b/OvmfPkg/PlatformCI/PlatformBuildLib.py
index bfef9849c7..b42235b2ac 100644
--- a/OvmfPkg/PlatformCI/PlatformBuildLib.py
+++ b/OvmfPkg/PlatformCI/PlatformBuildLib.py
@@ -103,15 +103,28 @@ class SettingsManager(UpdateSettingsManager,
SetupSettingsManager, PrEvalSetting

return build_these_packages

+ def AddCommandLineOptions(self, parserObj):
+ parserObj.add_argument('-u', '--UnitTest', dest='ShellUnitTestList',
type=str,
+ help='Optional - Key:Value that contains Shell UnitTest list and
corresponding DscPath you want to test.(workspace relative)'
+ 'Can list multiple by doing -u
<UTPath1:DscPath1>,<UTPath2:DscPath2> or -u <UTPath3:DscPath3> -u
<UTPath4:DscPath4>',
+ action="append", default=None)
+
+ def RetrieveCommandLineOptions(self, args):
+ if args.ShellUnitTestList:
+
+ CommonPlatform.UpdatePackagesSupported(args.ShellUnitTestList)
+
def GetPlatformDscAndConfig(self) -> tuple:
''' If a platform desires to provide its DSC then Policy 4 will evaluate if
any of the changes will be built in the dsc.

The tuple should be (<workspace relative path to dsc file>, <input
dictionary of dsc key value pairs>)
+ This Policy 4 can only be applied when PackagesSupported only
contains OvmfPkg Since it doesn't support
+ mutiple packages evaluation.
'''
- dsc = CommonPlatform.GetDscName(",".join(self.ActualArchitectures))
- return (f"OvmfPkg/{dsc}", {})
-
+ if (len(CommonPlatform.PackagesSupported) == 1) and
(CommonPlatform.PackagesSupported[0] == 'OvmfPkg'):
+ dsc =
CommonPlatform.GetDscName(",".join(self.ActualArchitectures))
+ return (f"OvmfPkg/{dsc}", {})
+ return None

#
##########################################################
############################# #
# Actual Configuration for Platform Build #
@@ -126,6 +139,14 @@ class PlatformBuilder( UefiBuilder,
BuildSettingsManager):
help="Optional - CSV of architecture to build. IA32 will use IA32 for
Pei & Dxe. "
"X64 will use X64 for both PEI and DXE. IA32,X64 will use IA32 for
PEI and "
"X64 for DXE. default is IA32,X64")
+ parserObj.add_argument('-p', '--pkg', '--pkg-dir',
dest='PkgsToBuildForUT', type=str,
+ help='Optional - Package list you want to build for UnitTest.efi.
(workspace relative).'
+ 'Can list multiple by doing -p <pkg1>,<pkg2> or -p <pkg3> -p
<pkg4>.If no valid input -p, build and run all -u UnitTest',
+ action="append", default=None)
+ parserObj.add_argument('-u', '--UnitTest', dest='ShellUnitTestList',
type=str,
+ help='Optional - Key:Value that contains Shell UnitTest list and
corresponding DscPath you want to test.(workspace relative)'
+ 'Can list multiple by doing -u
<UTPath1:DscPath1>,<UTPath2:DscPath2> or -u <UTPath3:DscPath3> -u
<UTPath4:DscPath4>',
+ action="append", default=None)

def RetrieveCommandLineOptions(self, args):
''' Retrieve command line options from the argparser '''
@@ -133,6 +154,10 @@ class PlatformBuilder( UefiBuilder,
BuildSettingsManager):
shell_environment.GetBuildVars().SetValue("TARGET_ARCH","
".join(args.build_arch.upper().split(",")), "From CmdLine")
dsc = CommonPlatform.GetDscName(args.build_arch)
shell_environment.GetBuildVars().SetValue("ACTIVE_PLATFORM",
f"OvmfPkg/{dsc}", "From CmdLine")
+ self.RunShellUnitTest = False
+ if args.ShellUnitTestList:
+ CommonPlatform.UpdateUnitTestConfig(args)
+ self.RunShellUnitTest = CommonPlatform.RunShellUnitTest

def GetWorkspaceRoot(self):
''' get WorkspacePath '''
@@ -176,6 +201,11 @@ class PlatformBuilder( UefiBuilder,
BuildSettingsManager):
return 0

def PlatformPostBuild(self):
+ if self.RunShellUnitTest:
+ ret = CommonPlatform.BuildUnitTest(self)
+ if ret !=0:
+ logging.critical("Build UnitTest failed")
+ return ret
return 0

def FlashRomImage(self):
@@ -210,9 +240,13 @@ class PlatformBuilder( UefiBuilder,
BuildSettingsManager):
else:
args += " -pflash " + os.path.join(OutputPath_FV, "OVMF.fd") #
path to firmware

-
if (self.env.GetValue("MAKE_STARTUP_NSH").upper() == "TRUE"):
f = open(os.path.join(VirtualDrive, "startup.nsh"), "w")
+ if self.RunShellUnitTest:
+ # When RunShellUnitTest is True, write all efi files name into
startup.nsh.
+ CommonPlatform.WriteEfiToStartup(VirtualDrive, f)
+ # Output UnitTest log into ShellUnitTestLog.
+ args += " -serial
+ file:{}".format(CommonPlatform.ShellUnitTestLog)
f.write("BOOT SUCCESS !!! \n")
## add commands here
f.write("reset -s\n")
@@ -222,6 +256,11 @@ class PlatformBuilder( UefiBuilder,
BuildSettingsManager):

if ret == 0xc0000005:
#for some reason getting a c0000005 on successful return
- return 0
-
+ ret = 0
+ if self.RunShellUnitTest and ret == 0:
+ # Check the UnitTest boot log.
+ UnitTestResult = CommonPlatform.CheckUnitTestLog()
+ if (UnitTestResult):
+ logging.info("UnitTest failed with this FAILURE
MESSAGE:\n{}".format(UnitTestResult))
+ return UnitTestResult
return ret
--
2.31.1.windows.1

-----Original Message-----
From: mikuback@... <mikuback@...>
Sent: Tuesday, December 6, 2022 1:29 AM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb+tianocore@...>; Yao, Jiewen
<jiewen.yao@...>; Justen, Jordan L <jordan.l.justen@...>;
Gerd Hoffmann <kraxel@...>; Sean Brogan
<sean.brogan@...>; Kinney, Michael D
<michael.d.kinney@...>; Gao, Liming <gaoliming@...>
Subject: [PATCH v1 5/5] OvmfPkg: Add reference to new build instructions

From: Michael Kubacki <michael.kubacki@...>

Adds a reference to the new build instructions on the TianoCore wiki
that currently describe building with containers and Stuart.

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Gerd Hoffmann <kraxel@...>
Cc: Sean Brogan <sean.brogan@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Signed-off-by: Michael Kubacki <michael.kubacki@...>
---
OvmfPkg/PlatformCI/ReadMe.md | 3 +++
OvmfPkg/README | 5 ++++-
2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/PlatformCI/ReadMe.md
b/OvmfPkg/PlatformCI/ReadMe.md
index 1216dee126f1..10fa32ac489f 100644
--- a/OvmfPkg/PlatformCI/ReadMe.md
+++ b/OvmfPkg/PlatformCI/ReadMe.md
@@ -31,6 +31,9 @@ Pytools build system.

## Building with Pytools for OvmfPkg

+If you are unfamiliar with Pytools, it is recommended to first read through
+the generic set of edk2 [Build
Instructions](https://github.com/tianocore/tianocore.github.io/wiki/Build-
Instructions).
+
1. [Optional] Create a Python Virtual Environment - generally once per
workspace

``` bash
diff --git a/OvmfPkg/README b/OvmfPkg/README
index d6e7e328483b..0a408abf019d 100644
--- a/OvmfPkg/README
+++ b/OvmfPkg/README
@@ -53,7 +53,10 @@ these binary outputs:
* OvmfVideo.rom
- This file is not built separately any longer, starting with svn r13520.

-More information on building OVMF can be found at:
+If you are new to building in edk2 or looking for the latest build
+instructions, visit
https://github.com/tianocore/tianocore.github.io/wiki/Build-Instructions
+
+More OVMF-specific build information can be found at:


https://github.com/tianocore/tianocore.github.io/wiki/How%20to%20buil
d%20OVMF

--
2.28.0.windows.1

-----Original Message-----
From: Ryan Afranji <afranji@...>
Sent: Thursday, December 8, 2022 8:51 AM
To: devel@edk2.groups.io
Cc: Nakajima, Jun <jun.nakajima@...>; Xu, Min M
<min.m.xu@...>; Yao, Jiewen <jiewen.yao@...>; Aktas, Erdem
<erdemaktas@...>; vannapurve@...; Ryan Afranji
<afranji@...>
Subject: [V1 PATCH 1/1] OvmfPkg: Add INVD case in #VE handler

According to the Intel GHCI specification document section 2.4.1, the
goal for instructions that do not have a corresponding TDCALL is for the
handler to treat the instruction as a NOP.

INVD does not have a corresponding TDCALL. This patch makes the #VE
handler treat INVD as a NOP.

Signed-off-by: Ryan Afranji <afranji@...>
---
OvmfPkg/Library/CcExitLib/CcExitVeHandler.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c
b/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c
index c35f65a649..3798c2bb13 100644
--- a/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c
+++ b/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c
@@ -541,6 +541,7 @@ CcExitHandleVe (
case EXIT_REASON_MONITOR_INSTRUCTION:
case EXIT_REASON_WBINVD:
case EXIT_REASON_RDPMC:
+ case EXIT_REASON_INVD:
/* Handle as nops. */
break;

--
2.39.0.rc1.256.g54fd8350bd-goog

-----Original Message-----
From: Liu, Zhiguang <zhiguang.liu@...>
Sent: Thursday, December 8, 2022 5:42 PM
To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Gao, Liming <gaoliming@...>; Ni, Ray <ray.ni@...>
Subject: RE: [edk2-devel] [PATCH] MdePkg/BaseCpuLib: Remove assembly for CpuFlushTlb

Hi Liming and Mike,
Could you help review this patch?

Thanks
Zhiguang

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Zhiguang Liu
Sent: Friday, December 2, 2022 2:25 PM
To: devel@edk2.groups.io
Cc: Liu, Zhiguang <zhiguang.liu@...>; Kinney, Michael D
<michael.d.kinney@...>; Gao, Liming <gaoliming@...>;
Ni, Ray <ray.ni@...>
Subject: [edk2-devel] [PATCH] MdePkg/BaseCpuLib: Remove assembly for
CpuFlushTlb

For different compilers, both IA32 and X64 can use Ia32/CpuFlushTlbGcc.c,
which is C code (no inline assembly code).
To simplify, remove other assemly file for CpuFlushTlb, and rename
Ia32/CpuFlushTlbGcc.c to Ia32/CpuFlushTlb.c.

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Signed-off-by: Ray Ni <ray.ni@...>
Signed-off-by: Zhiguang Liu <zhiguang.liu@...>
---
MdePkg/Library/BaseCpuLib/BaseCpuLib.inf | 10 ++----
MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.c | 12 +++----
.../Library/BaseCpuLib/Ia32/CpuFlushTlb.nasm | 31 ------------------
.../Library/BaseCpuLib/Ia32/CpuFlushTlbGcc.c | 25 ---------------
.../Library/BaseCpuLib/X64/CpuFlushTlb.nasm | 32 -------------------
5 files changed, 9 insertions(+), 101 deletions(-) delete mode 100644
MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.nasm
delete mode 100644 MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlbGcc.c
delete mode 100644 MdePkg/Library/BaseCpuLib/X64/CpuFlushTlb.nasm

diff --git a/MdePkg/Library/BaseCpuLib/BaseCpuLib.inf
b/MdePkg/Library/BaseCpuLib/BaseCpuLib.inf
index 6b230f6e6d..0feb592638 100644
--- a/MdePkg/Library/BaseCpuLib/BaseCpuLib.inf
+++ b/MdePkg/Library/BaseCpuLib/BaseCpuLib.inf
@@ -4,7 +4,7 @@
# CPU Library implemented using ASM functions for IA32, X64, ARM,
AARCH64, # PAL CALLs for IPF, and empty functions for EBC.
#
-# Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2007 - 2022, Intel Corporation. All rights
+reserved.<BR>
# Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR> #
Portions copyright (c) 2011 - 2013, ARM Ltd. All rights reserved.<BR> #
Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights
reserved.<BR> @@ -31,16 +31,12 @@

[Sources.IA32]
Ia32/CpuSleep.c | MSFT
- Ia32/CpuFlushTlb.c | MSFT
-
Ia32/CpuSleep.nasm| INTEL
- Ia32/CpuFlushTlb.nasm| INTEL
-
Ia32/CpuSleepGcc.c | GCC
- Ia32/CpuFlushTlbGcc.c | GCC
+ Ia32/CpuFlushTlb.c

[Sources.X64]
- X64/CpuFlushTlb.nasm
+ Ia32/CpuFlushTlb.c
X64/CpuSleep.nasm


diff --git a/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.c
b/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.c
index 549f4eb8a0..17a351d054 100644
--- a/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.c
+++ b/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.c
@@ -1,11 +1,14 @@
/** @file
- CpuFlushTlb function.
+ CpuFlushTlb function for Ia32/X64.

- Copyright (c) 2006 - 2008, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2006 - 2022, Intel Corporation. All rights
+ reserved.<BR> Portions copyright (c) 2008 - 2009, Apple Inc. All
+ rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/

+#include <Library/BaseLib.h>
+
/**
Flushes all the Translation Lookaside Buffers(TLB) entries in a CPU.

@@ -18,8 +21,5 @@ CpuFlushTlb (
VOID
)
{
- _asm {
- mov eax, cr3
- mov cr3, eax
- }
+ AsmWriteCr3 (AsmReadCr3 ());
}
diff --git a/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.nasm
b/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.nasm
deleted file mode 100644
index bc3b68e3f2..0000000000
--- a/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.nasm
+++ /dev/null
@@ -1,31 +0,0 @@
-;------------------------------------------------------------------------------ ; -;
Copyright (c) 2006, Intel Corporation. All rights reserved.<BR> -; SPDX-
License-Identifier: BSD-2-Clause-Patent -; -; Module Name:
-;
-; CpuFlushTlb.Asm
-;
-; Abstract:
-;
-; CpuFlushTlb function
-;
-; Notes:
-;
-;------------------------------------------------------------------------------
-
- SECTION .text
-
-;------------------------------------------------------------------------------
-; VOID
-; EFIAPI
-; CpuFlushTlb (
-; VOID
-; );
-;------------------------------------------------------------------------------
-global ASM_PFX(CpuFlushTlb)
-ASM_PFX(CpuFlushTlb):
- mov eax, cr3
- mov cr3, eax ; moving to CR3 flushes TLB
- ret
-
diff --git a/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlbGcc.c
b/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlbGcc.c
deleted file mode 100644
index ee44f2ea6e..0000000000
--- a/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlbGcc.c
+++ /dev/null
@@ -1,25 +0,0 @@
-/** @file
- CpuFlushTlb function for Ia32/X64 GCC.
-
- Copyright (c) 2006 - 2008, Intel Corporation. All rights reserved.<BR>
- Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
- SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#include <Library/BaseLib.h>
-
-/**
- Flushes all the Translation Lookaside Buffers(TLB) entries in a CPU.
-
- Flushes all the Translation Lookaside Buffers(TLB) entries in a CPU.
-
-**/
-VOID
-EFIAPI
-CpuFlushTlb (
- VOID
- )
-{
- AsmWriteCr3 (AsmReadCr3 ());
-}
diff --git a/MdePkg/Library/BaseCpuLib/X64/CpuFlushTlb.nasm
b/MdePkg/Library/BaseCpuLib/X64/CpuFlushTlb.nasm
deleted file mode 100644
index 8ddf7a2864..0000000000
--- a/MdePkg/Library/BaseCpuLib/X64/CpuFlushTlb.nasm
+++ /dev/null
@@ -1,32 +0,0 @@
-;------------------------------------------------------------------------------ ; -;
Copyright (c) 2006, Intel Corporation. All rights reserved.<BR> -; SPDX-
License-Identifier: BSD-2-Clause-Patent -; -; Module Name:
-;
-; CpuFlushTlb.Asm
-;
-; Abstract:
-;
-; CpuFlushTlb function
-;
-; Notes:
-;
-;------------------------------------------------------------------------------
-
- DEFAULT REL
- SECTION .text
-
-;------------------------------------------------------------------------------
-; VOID
-; EFIAPI
-; CpuFlushTlb (
-; VOID
-; );
-;------------------------------------------------------------------------------
-global ASM_PFX(CpuFlushTlb)
-ASM_PFX(CpuFlushTlb):
- mov rax, cr3
- mov cr3, rax
- ret
-
--
2.31.1.windows.1

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Zhiguang Liu
Sent: Friday, December 2, 2022 2:25 PM
To: devel@edk2.groups.io
Cc: Liu, Zhiguang <zhiguang.liu@...>; Kinney, Michael D
<michael.d.kinney@...>; Gao, Liming <gaoliming@...>;
Ni, Ray <ray.ni@...>
Subject: [edk2-devel] [PATCH] MdePkg/BaseCpuLib: Remove assembly for
CpuFlushTlb

For different compilers, both IA32 and X64 can use Ia32/CpuFlushTlbGcc.c,
which is C code (no inline assembly code).
To simplify, remove other assemly file for CpuFlushTlb, and rename
Ia32/CpuFlushTlbGcc.c to Ia32/CpuFlushTlb.c.

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Signed-off-by: Ray Ni <ray.ni@...>
Signed-off-by: Zhiguang Liu <zhiguang.liu@...>
---
MdePkg/Library/BaseCpuLib/BaseCpuLib.inf | 10 ++----
MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.c | 12 +++----
.../Library/BaseCpuLib/Ia32/CpuFlushTlb.nasm | 31 ------------------
.../Library/BaseCpuLib/Ia32/CpuFlushTlbGcc.c | 25 ---------------
.../Library/BaseCpuLib/X64/CpuFlushTlb.nasm | 32 -------------------
5 files changed, 9 insertions(+), 101 deletions(-) delete mode 100644
MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.nasm
delete mode 100644 MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlbGcc.c
delete mode 100644 MdePkg/Library/BaseCpuLib/X64/CpuFlushTlb.nasm

diff --git a/MdePkg/Library/BaseCpuLib/BaseCpuLib.inf
b/MdePkg/Library/BaseCpuLib/BaseCpuLib.inf
index 6b230f6e6d..0feb592638 100644
--- a/MdePkg/Library/BaseCpuLib/BaseCpuLib.inf
+++ b/MdePkg/Library/BaseCpuLib/BaseCpuLib.inf
@@ -4,7 +4,7 @@
# CPU Library implemented using ASM functions for IA32, X64, ARM,
AARCH64, # PAL CALLs for IPF, and empty functions for EBC.
#
-# Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2007 - 2022, Intel Corporation. All rights
+reserved.<BR>
# Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR> #
Portions copyright (c) 2011 - 2013, ARM Ltd. All rights reserved.<BR> #
Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights
reserved.<BR> @@ -31,16 +31,12 @@

[Sources.IA32]
Ia32/CpuSleep.c | MSFT
- Ia32/CpuFlushTlb.c | MSFT
-
Ia32/CpuSleep.nasm| INTEL
- Ia32/CpuFlushTlb.nasm| INTEL
-
Ia32/CpuSleepGcc.c | GCC
- Ia32/CpuFlushTlbGcc.c | GCC
+ Ia32/CpuFlushTlb.c

[Sources.X64]
- X64/CpuFlushTlb.nasm
+ Ia32/CpuFlushTlb.c
X64/CpuSleep.nasm


diff --git a/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.c
b/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.c
index 549f4eb8a0..17a351d054 100644
--- a/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.c
+++ b/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.c
@@ -1,11 +1,14 @@
/** @file
- CpuFlushTlb function.
+ CpuFlushTlb function for Ia32/X64.

- Copyright (c) 2006 - 2008, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2006 - 2022, Intel Corporation. All rights
+ reserved.<BR> Portions copyright (c) 2008 - 2009, Apple Inc. All
+ rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/

+#include <Library/BaseLib.h>
+
/**
Flushes all the Translation Lookaside Buffers(TLB) entries in a CPU.

@@ -18,8 +21,5 @@ CpuFlushTlb (
VOID
)
{
- _asm {
- mov eax, cr3
- mov cr3, eax
- }
+ AsmWriteCr3 (AsmReadCr3 ());
}
diff --git a/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.nasm
b/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.nasm
deleted file mode 100644
index bc3b68e3f2..0000000000
--- a/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlb.nasm
+++ /dev/null
@@ -1,31 +0,0 @@
-;------------------------------------------------------------------------------ ; -;
Copyright (c) 2006, Intel Corporation. All rights reserved.<BR> -; SPDX-
License-Identifier: BSD-2-Clause-Patent -; -; Module Name:
-;
-; CpuFlushTlb.Asm
-;
-; Abstract:
-;
-; CpuFlushTlb function
-;
-; Notes:
-;
-;------------------------------------------------------------------------------
-
- SECTION .text
-
-;------------------------------------------------------------------------------
-; VOID
-; EFIAPI
-; CpuFlushTlb (
-; VOID
-; );
-;------------------------------------------------------------------------------
-global ASM_PFX(CpuFlushTlb)
-ASM_PFX(CpuFlushTlb):
- mov eax, cr3
- mov cr3, eax ; moving to CR3 flushes TLB
- ret
-
diff --git a/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlbGcc.c
b/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlbGcc.c
deleted file mode 100644
index ee44f2ea6e..0000000000
--- a/MdePkg/Library/BaseCpuLib/Ia32/CpuFlushTlbGcc.c
+++ /dev/null
@@ -1,25 +0,0 @@
-/** @file
- CpuFlushTlb function for Ia32/X64 GCC.
-
- Copyright (c) 2006 - 2008, Intel Corporation. All rights reserved.<BR>
- Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
- SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#include <Library/BaseLib.h>
-
-/**
- Flushes all the Translation Lookaside Buffers(TLB) entries in a CPU.
-
- Flushes all the Translation Lookaside Buffers(TLB) entries in a CPU.
-
-**/
-VOID
-EFIAPI
-CpuFlushTlb (
- VOID
- )
-{
- AsmWriteCr3 (AsmReadCr3 ());
-}
diff --git a/MdePkg/Library/BaseCpuLib/X64/CpuFlushTlb.nasm
b/MdePkg/Library/BaseCpuLib/X64/CpuFlushTlb.nasm
deleted file mode 100644
index 8ddf7a2864..0000000000
--- a/MdePkg/Library/BaseCpuLib/X64/CpuFlushTlb.nasm
+++ /dev/null
@@ -1,32 +0,0 @@
-;------------------------------------------------------------------------------ ; -;
Copyright (c) 2006, Intel Corporation. All rights reserved.<BR> -; SPDX-
License-Identifier: BSD-2-Clause-Patent -; -; Module Name:
-;
-; CpuFlushTlb.Asm
-;
-; Abstract:
-;
-; CpuFlushTlb function
-;
-; Notes:
-;
-;------------------------------------------------------------------------------
-
- DEFAULT REL
- SECTION .text
-
-;------------------------------------------------------------------------------
-; VOID
-; EFIAPI
-; CpuFlushTlb (
-; VOID
-; );
-;------------------------------------------------------------------------------
-global ASM_PFX(CpuFlushTlb)
-ASM_PFX(CpuFlushTlb):
- mov rax, cr3
- mov cr3, rax
- ret
-
--
2.31.1.windows.1

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@...>
Sent: Thursday, December 8, 2022 1:58 PM
To: devel@edk2.groups.io; ardb@...; Kinney, Michael D <michael.d.kinney@...>
Cc: Ni, Ray <ray.ni@...>; Gao, Zhichao <zhichao.gao@...>
Subject: RE: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed device path protocols

Ard,

Thank you for the correction.

If we add that CONST, then the ShellPkg build breaks with an error

c:\work\github\tianocore\edk2\ShellPkg\Application\Shell\ShellProtocol.c(297): error C2220: the following warning is
treated as an error
c:\work\github\tianocore\edk2\ShellPkg\Application\Shell\ShellProtocol.c(297): warning C4090: 'function': different
'const' qualifiers

Which is exactly the line we want to remove to prevent the destructive behavior.

SetDevicePathEndNode (*DevicePath);

If I comment out that line, the ShellPkg build completes with no errors.

I agree that it would be better to update the prototype and get help
from the compiler to find incorrect implementations. Even though
CONST is not in the prototype, from reading the description of the API
it does not state that the contents are modified, so I think the
intent was no modifications.

Your suggested change is safe, but it is incomplete because there
are additional calls through the protocol that are not covered
by your patch. We also do not know how many places this API
is used in downstream projects. This side effect of a write to
a read-only page and potential corruption of a multi-instance
device path looks like a bug to me and we should fix the root
cause and not fix just some of the callers.

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
Sent: Thursday, December 8, 2022 1:40 PM
To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@...>
Cc: Ni, Ray <ray.ni@...>; Gao, Zhichao <zhichao.gao@...>
Subject: Re: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed device path protocols

On Thu, 8 Dec 2022 at 22:15, Michael D Kinney
<michael.d.kinney@...> wrote:

Hi Ard,

There is a difference between returning a pointer to a device path
and modifying the device path contents.

If you add CONST to the argument, then an updated pointer to a device
path can not be returned.

No, this is incorrect.

The function takes a pointer (1) to a pointer(2) to a device path protocol

EFI_DEVICE_PATH_PROTOCOL **

So the function can dereference pointer 1 and modify pointer 2
*unless* it is marked as CONST, i.e.

EFI_DEVICE_PATH_PROTOCOL * CONST *

in which case the pointer is not modifiable, but it is permitted to
dereference that pointer to modify the underlying object.

I am arguing that the prototype should be

EFI_DEVICE_PATH_PROTOCOL CONST **

(which is the same as putting the CONST at the beginning)

where the caller's pointer can be advanced by the callee via the
pointer-to-pointer. But that would still not permit the object to be
modified.

The API clear describes returning an updated device path pointer, so
the API is declared correctly without CONST.

The pointer may be updated but not the object. It really comes down to
the difference between

CONST EFI_DEVICE_PATH_PROTOCOL **
EFI_DEVICE_PATH_PROTOCOL CONST **
EFI_DEVICE_PATH_PROTOCOL * CONST *
EFI_DEVICE_PATH_PROTOCOL **CONST

(where the first two mean the same thing0

The API does not state that the contents of the device path are modified.

An API that uses CONST EFI_DEVICE_PATH* would indicate that the API
should not modify the contents of the device path. For example:

/**
Returns the size of a device path in bytes.

This function returns the size, in bytes, of the device path data structure
specified by DevicePath including the end of device path node.
If DevicePath is NULL or invalid, then 0 is returned.

@param DevicePath A pointer to a device path data structure.

@retval 0 If DevicePath is NULL or invalid.
@retval Others The size of a device path in bytes.

**/
UINTN
EFIAPI
GetDevicePathSize (
IN CONST EFI_DEVICE_PATH_PROTOCOL *DevicePath
);

Yes, but this one is a pointer, not a pointer-to-pointer. Big difference.

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
Sent: Thursday, December 8, 2022 1:40 PM
To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@...>
Cc: Ni, Ray <ray.ni@...>; Gao, Zhichao <zhichao.gao@...>
Subject: Re: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed device path protocols

On Thu, 8 Dec 2022 at 22:15, Michael D Kinney
<michael.d.kinney@...> wrote:

Hi Ard,

There is a difference between returning a pointer to a device path
and modifying the device path contents.

If you add CONST to the argument, then an updated pointer to a device
path can not be returned.

No, this is incorrect.

The function takes a pointer (1) to a pointer(2) to a device path protocol

EFI_DEVICE_PATH_PROTOCOL **

So the function can dereference pointer 1 and modify pointer 2
*unless* it is marked as CONST, i.e.

EFI_DEVICE_PATH_PROTOCOL * CONST *

in which case the pointer is not modifiable, but it is permitted to
dereference that pointer to modify the underlying object.

I am arguing that the prototype should be

EFI_DEVICE_PATH_PROTOCOL CONST **

(which is the same as putting the CONST at the beginning)

where the caller's pointer can be advanced by the callee via the
pointer-to-pointer. But that would still not permit the object to be
modified.

The API clear describes returning an updated device path pointer, so
the API is declared correctly without CONST.

The pointer may be updated but not the object. It really comes down to
the difference between

CONST EFI_DEVICE_PATH_PROTOCOL **
EFI_DEVICE_PATH_PROTOCOL CONST **
EFI_DEVICE_PATH_PROTOCOL * CONST *
EFI_DEVICE_PATH_PROTOCOL **CONST

(where the first two mean the same thing0

The API does not state that the contents of the device path are modified.

An API that uses CONST EFI_DEVICE_PATH* would indicate that the API
should not modify the contents of the device path. For example:

/**
Returns the size of a device path in bytes.

This function returns the size, in bytes, of the device path data structure
specified by DevicePath including the end of device path node.
If DevicePath is NULL or invalid, then 0 is returned.

@param DevicePath A pointer to a device path data structure.

@retval 0 If DevicePath is NULL or invalid.
@retval Others The size of a device path in bytes.

**/
UINTN
EFIAPI
GetDevicePathSize (
IN CONST EFI_DEVICE_PATH_PROTOCOL *DevicePath
);

Yes, but this one is a pointer, not a pointer-to-pointer. Big difference.

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
Sent: Thursday, December 8, 2022 12:12 PM
To: Kinney, Michael D <michael.d.kinney@...>
Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@...>; Gao, Zhichao <zhichao.gao@...>
Subject: Re: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed device path protocols

On Thu, 8 Dec 2022 at 20:20, Kinney, Michael D
<michael.d.kinney@...> wrote:

Hi Ard,

Much of this code has not been updated since initially added in 2010.

Looks like a bug to me that has been there the whole time.

I agree it is a behavior change in the implementation. But unless
new code use of this API looks at the implementation, they would
not know it is destructive and they need to make a copy. This
API is available to external shell apps that use the shell protocol.

Well, not entirely. The function takes EFI_DEVICE_PATH_PROTOCOL** not
CONST EFI_DEVICE_PATH_PROTOCOL**, and so one might argue that the
underlying object is modifiable by the callee. And similarly, that
shell code should not grab a EFI device path protocol pointer from the
database and pass it to a function that does not use a CONST qualified
EFI_DEVICE_PATH_PROTOCOL pointer to accept the argument.

We should get the shell owners to evaluate removing the destructive
behavior.

I suppose changing the prototypes is out of the question, as doing so
would require a new version of the shell protocol?

-----Original Message-----
From: Ard Biesheuvel <ardb@...>
Sent: Thursday, December 8, 2022 10:45 AM
To: Kinney, Michael D <michael.d.kinney@...>
Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@...>; Gao, Zhichao <zhichao.gao@...>
Subject: Re: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed device path protocols

On Thu, 8 Dec 2022 at 19:28, Kinney, Michael D
<michael.d.kinney@...> wrote:

Hi Ard,

From this description, it does not look like it should be modifying the
contents of the device path. Just point to the device path end node that
follows the match found.

/**
Gets the mapping that most closely matches the device path.

This function gets the mapping which corresponds to the device path *DevicePath. If
there is no exact match, then the mapping which most closely matches *DevicePath
is returned, and *DevicePath is updated to point to the remaining portion of the
device path. If there is an exact match, the mapping is returned and *DevicePath
points to the end-of-device-path node.

@param DevicePath On entry, points to a device path pointer. On
exit, updates the pointer to point to the
portion of the device path after the mapping.

@retval NULL No mapping was found.
@return !=NULL Pointer to NULL-terminated mapping. The buffer
is callee allocated and should be freed by the caller.
**/
CONST CHAR16 *
EFIAPI
EfiShellGetMapFromDevicePath (
IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
);

I see this API used in many places, and it looks like it would be
destructive to any multi-instance device path. Multi-instance
device paths are typically used for consoles, so we may not have
noticed this destructive behavior with file system mapping paths.

Did you try removing the call to SetDevicePathEndNode (*DevicePath); ?

No, but that would be a functional change visible to all users of the
current API.

And note that the calling code already has 'DevicePathCopy' variables,
it just doesn't bother using them, so the intent is clearly to pass a
copy, not the actual device path.

-----Original Message-----
From: Ard Biesheuvel <ardb@...>
Sent: Thursday, December 8, 2022 10:45 AM
To: Kinney, Michael D <michael.d.kinney@...>
Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@...>; Gao, Zhichao <zhichao.gao@...>
Subject: Re: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed device path protocols

On Thu, 8 Dec 2022 at 19:28, Kinney, Michael D
<michael.d.kinney@...> wrote:

Hi Ard,

From this description, it does not look like it should be modifying the
contents of the device path. Just point to the device path end node that
follows the match found.

/**
Gets the mapping that most closely matches the device path.

This function gets the mapping which corresponds to the device path *DevicePath. If
there is no exact match, then the mapping which most closely matches *DevicePath
is returned, and *DevicePath is updated to point to the remaining portion of the
device path. If there is an exact match, the mapping is returned and *DevicePath
points to the end-of-device-path node.

@param DevicePath On entry, points to a device path pointer. On
exit, updates the pointer to point to the
portion of the device path after the mapping.

@retval NULL No mapping was found.
@return !=NULL Pointer to NULL-terminated mapping. The buffer
is callee allocated and should be freed by the caller.
**/
CONST CHAR16 *
EFIAPI
EfiShellGetMapFromDevicePath (
IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
);

I see this API used in many places, and it looks like it would be
destructive to any multi-instance device path. Multi-instance
device paths are typically used for consoles, so we may not have
noticed this destructive behavior with file system mapping paths.

Did you try removing the call to SetDevicePathEndNode (*DevicePath); ?

No, but that would be a functional change visible to all users of the
current API.

And note that the calling code already has 'DevicePathCopy' variables,
it just doesn't bother using them, so the intent is clearly to pass a
copy, not the actual device path.